From 1e2cea48d0cff6af72b580dd5d6dc5e8810bdcf7 Mon Sep 17 00:00:00 2001 From: javanna Date: Thu, 15 Jan 2015 18:07:38 +0100 Subject: [PATCH] Authorization: enforced some assumptions through asserts - made sure that clear_scroll all gets converted to the correspoinding shield cluster action in both action filter and transport filter (used to happen only on the action filter before): introduced the context of ShieldActionMapper that allows to convert action names based on an incoming request and its action name (will be useful for analyze api too) - made sure that potential clear_scroll all errors contain the shield action name rather than the es core original one - made it clearer that the only indices actions known not to be indices requests are scroll related ones, which we assert on and grant. Everything else gets denied. - made it clearer that the only indices request whose indices might end up being resolved to an empty set is analyze request, as its index is optional - simplified permissions check in Permission.Group by asserting on index argument not null Original commit: elastic/x-pack-elasticsearch@7c01159b03305bc007d0a4465376d1d5213fd7bb --- .../shield/action/ShieldActionFilter.java | 17 ++-- .../shield/action/ShieldActionMapper.java | 33 ++++++++ .../shield/action/ShieldActionModule.java | 1 + .../authz/InternalAuthorizationService.java | 79 +++++++++++-------- .../shield/authz/Permission.java | 6 +- .../transport/ServerTransportFilter.java | 28 ++++--- .../action/ShieldActionFilterTests.java | 2 +- .../transport/ServerTransportFilterTests.java | 3 +- .../test/ShieldClearScrollTests.java | 20 ++--- 9 files changed, 123 insertions(+), 66 deletions(-) create mode 100644 src/main/java/org/elasticsearch/shield/action/ShieldActionMapper.java diff --git a/src/main/java/org/elasticsearch/shield/action/ShieldActionFilter.java b/src/main/java/org/elasticsearch/shield/action/ShieldActionFilter.java index b59566b97c5..0bdb609fdad 100644 --- a/src/main/java/org/elasticsearch/shield/action/ShieldActionFilter.java +++ b/src/main/java/org/elasticsearch/shield/action/ShieldActionFilter.java @@ -46,16 +46,19 @@ public class ShieldActionFilter extends AbstractComponent implements ActionFilte private final AuthorizationService authzService; private final SignatureService signatureService; private final AuditTrail auditTrail; + private final ShieldActionMapper actionMapper; private volatile boolean licenseEnabled; @Inject - public ShieldActionFilter(Settings settings, AuthenticationService authcService, AuthorizationService authzService, SignatureService signatureService, AuditTrail auditTrail, LicenseEventsNotifier licenseEventsNotifier) { + public ShieldActionFilter(Settings settings, AuthenticationService authcService, AuthorizationService authzService, SignatureService signatureService, + AuditTrail auditTrail, LicenseEventsNotifier licenseEventsNotifier, ShieldActionMapper actionMapper) { super(settings); this.authcService = authcService; this.authzService = authzService; this.signatureService = signatureService; this.auditTrail = auditTrail; + this.actionMapper = actionMapper; licenseEventsNotifier.register(new LicensesClientService.Listener() { @Override public void onEnabled() { @@ -92,9 +95,11 @@ public class ShieldActionFilter extends AbstractComponent implements ActionFilte the {@link Rest} filter and the {@link ServerTransport} filter respectively), it's safe to assume a system user here if a request is not associated with any other user. */ - User user = authcService.authenticate(action, request, User.SYSTEM); - authzService.authorize(user, action, request); - request = unsign(user, action, request); + + String shieldAction = actionMapper.action(action, request); + User user = authcService.authenticate(shieldAction, request, User.SYSTEM); + authzService.authorize(user, shieldAction, request); + request = unsign(user, shieldAction, request); chain.proceed(action, request, new SigningListener(this, listener)); } catch (Throwable t) { listener.onFailure(t); @@ -125,9 +130,7 @@ public class ShieldActionFilter extends AbstractComponent implements ActionFilte if (request instanceof ClearScrollRequest) { ClearScrollRequest clearScrollRequest = (ClearScrollRequest) request; boolean isClearAllScrollRequest = clearScrollRequest.scrollIds().contains("_all"); - if (isClearAllScrollRequest) { - authzService.authorize(user, CLUSTER_PERMISSION_SCROLL_CLEAR_ALL_NAME, request); - } else { + if (!isClearAllScrollRequest) { List signedIds = clearScrollRequest.scrollIds(); List unsignedIds = new ArrayList<>(signedIds.size()); for (String signedId : signedIds) { diff --git a/src/main/java/org/elasticsearch/shield/action/ShieldActionMapper.java b/src/main/java/org/elasticsearch/shield/action/ShieldActionMapper.java new file mode 100644 index 00000000000..19382937a2e --- /dev/null +++ b/src/main/java/org/elasticsearch/shield/action/ShieldActionMapper.java @@ -0,0 +1,33 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ +package org.elasticsearch.shield.action; + +import org.elasticsearch.action.search.ClearScrollAction; +import org.elasticsearch.action.search.ClearScrollRequest; +import org.elasticsearch.transport.TransportRequest; + +/** + * This class analyzes an incoming request and its action name, and returns the shield action name for it. + * In many cases the action name is the same as the original one used in es core, but in some exceptional cases it might need + * to be converted. For instance a clear_scroll that targets all opened scrolls gets converted to a different action that requires + * cluster privileges instead of the default indices privileges, still valid for clear scrolls that target specific scroll ids. + */ +public class ShieldActionMapper { + + /** + * Returns the shield specific action name given the incoming action name and request + */ + public String action(String action, TransportRequest request) { + if (action.equals(ClearScrollAction.NAME)) { + assert request instanceof ClearScrollRequest; + boolean isClearAllScrollRequest = ((ClearScrollRequest) request).scrollIds().contains("_all"); + if (isClearAllScrollRequest) { + return ShieldActionFilter.CLUSTER_PERMISSION_SCROLL_CLEAR_ALL_NAME; + } + } + return action; + } +} diff --git a/src/main/java/org/elasticsearch/shield/action/ShieldActionModule.java b/src/main/java/org/elasticsearch/shield/action/ShieldActionModule.java index fe3da115198..b6d0a13c358 100644 --- a/src/main/java/org/elasticsearch/shield/action/ShieldActionModule.java +++ b/src/main/java/org/elasticsearch/shield/action/ShieldActionModule.java @@ -39,6 +39,7 @@ public class ShieldActionModule extends AbstractShieldModule implements PreProce @Override protected void configure(boolean clientMode) { if (!clientMode) { + bind(ShieldActionMapper.class).asEagerSingleton(); // we need to ensure that there's only a single instance of this filter. bind(ShieldActionFilter.class).asEagerSingleton(); } diff --git a/src/main/java/org/elasticsearch/shield/authz/InternalAuthorizationService.java b/src/main/java/org/elasticsearch/shield/authz/InternalAuthorizationService.java index 4978a1a35e1..ea7a8316de5 100644 --- a/src/main/java/org/elasticsearch/shield/authz/InternalAuthorizationService.java +++ b/src/main/java/org/elasticsearch/shield/authz/InternalAuthorizationService.java @@ -7,6 +7,9 @@ package org.elasticsearch.shield.authz; import org.elasticsearch.action.CompositeIndicesRequest; import org.elasticsearch.action.IndicesRequest; +import org.elasticsearch.action.admin.indices.analyze.AnalyzeRequest; +import org.elasticsearch.action.search.ClearScrollAction; +import org.elasticsearch.action.search.SearchScrollAction; import org.elasticsearch.cluster.ClusterService; import org.elasticsearch.cluster.metadata.MetaData; import org.elasticsearch.common.base.Predicate; @@ -15,6 +18,7 @@ import org.elasticsearch.common.collect.ImmutableList; import org.elasticsearch.common.component.AbstractComponent; import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.search.action.SearchServiceTransportAction; import org.elasticsearch.shield.User; import org.elasticsearch.shield.audit.AuditTrail; import org.elasticsearch.shield.authz.indicesresolver.DefaultIndicesResolver; @@ -22,7 +26,6 @@ import org.elasticsearch.shield.authz.indicesresolver.IndicesResolver; import org.elasticsearch.shield.authz.store.RolesStore; import org.elasticsearch.transport.TransportRequest; -import java.util.Collections; import java.util.Iterator; import java.util.Set; @@ -99,7 +102,7 @@ public class InternalAuthorizationService extends AbstractComponent implements A } // first, we'll check if the action is a cluster action. If it is, we'll only check it - // agaist the cluster permissions + // against the cluster permissions if (Privilege.Cluster.ACTION_MATCHER.apply(action)) { Permission.Cluster cluster = permission.cluster(); if (cluster != null && cluster.check(action)) { @@ -114,25 +117,39 @@ public class InternalAuthorizationService extends AbstractComponent implements A throw denial(user, action, request); } - Set indexNames = resolveIndices(user, action, request); - if (indexNames == null) { - // the only time this will be null, is for those requests that are - // categorized as indices request but they're actully not (for example, scroll) - // in these cases, we only grant/deny based on the action name (performed above) - grant(user, action, request); - return; - } - - Permission.Indices indices = permission.indices(); - if (indices == null || indices.isEmpty()) { + // some APIs are indices requests that are not actually associated with indices. For example, + // search scroll request, is categorized under the indices context, but doesn't hold indices names + // (in this case, the security check on the indices was done on the search request that initialized + // the scroll... and we rely on the signed scroll id to provide security over this request). + // so we only check indices if indeed the request is an actual IndicesRequest, if it's not, + // we just grant it if it's a scroll, deny otherwise + if (!(request instanceof IndicesRequest) && !(request instanceof CompositeIndicesRequest)) { + if (isScrollRelatedAction(action)) { + //note that clear scroll shard level actions can originate from a clear scroll all, which doesn't require any + //indices permission as it's categorized under cluster. This is why the scroll check is performed + //even before checking if the user has any indices permission. + grant(user, action, request); + return; + } + assert false : "only scroll related requests are known indices api that don't support retrieving the indices they relate to"; throw denial(user, action, request); } + if (permission.indices() == null || permission.indices().isEmpty()) { + throw denial(user, action, request); + } + + Set indexNames = resolveIndices(user, action, request); + //Note: some APIs (e.g. analyze) are categorized under indices, but their indices are optional. + //In that case the resolved indices set is empty (for now). See https://github.com/elasticsearch/elasticsearch-shield/issues/566 + assert !indexNames.isEmpty() || request instanceof AnalyzeRequest + : "no indices request other than the analyze api has optional indices thus the resolved indices must not be empty"; + // now... every index that is associated with the request, must be granted // by at least one indices permission group for (String index : indexNames) { boolean granted = false; - for (Permission.Indices.Group group : indices) { + for (Permission.Indices.Group group : permission.indices()) { if (group.check(action, index)) { granted = true; break; @@ -180,25 +197,23 @@ public class InternalAuthorizationService extends AbstractComponent implements A private Set resolveIndices(User user, String action, TransportRequest request) { MetaData metaData = clusterService.state().metaData(); - - // some APIs are indices requests that are not actually associated with indices. For example, - // search scroll request, is categorized under the indices context, but doesn't hold indices names - // (in this case, the security check on the indices was done on the search request that initialized - // the scroll... and we rely on the signed scroll id to provide security over this request). - - // so we only check indices if indeed the request is an actual IndicesRequest, if it's not, we only - // perform the check on the action name. - Set indices = null; - - if (request instanceof IndicesRequest || request instanceof CompositeIndicesRequest) { - indices = Collections.emptySet(); - for (IndicesResolver resolver : indicesResolvers) { - if (resolver.requestType().isInstance(request)) { - indices = resolver.resolve(user, action, request, metaData); - break; - } + for (IndicesResolver resolver : indicesResolvers) { + if (resolver.requestType().isInstance(request)) { + return resolver.resolve(user, action, request, metaData); } } - return indices; + assert false : "we should be able to resolve indices for any known request that requires indices privileges"; + throw denial(user, action, request); + } + + private static boolean isScrollRelatedAction(String action) { + return action.equals(SearchScrollAction.NAME) || + action.equals(SearchServiceTransportAction.SCAN_SCROLL_ACTION_NAME) || + action.equals(SearchServiceTransportAction.FETCH_ID_SCROLL_ACTION_NAME) || + action.equals(SearchServiceTransportAction.QUERY_FETCH_SCROLL_ACTION_NAME) || + action.equals(SearchServiceTransportAction.QUERY_SCROLL_ACTION_NAME) || + action.equals(SearchServiceTransportAction.FREE_CONTEXT_SCROLL_ACTION_NAME) || + action.equals(ClearScrollAction.NAME) || + action.equals(SearchServiceTransportAction.CLEAR_SCROLL_CONTEXTS_ACTION_NAME); } } diff --git a/src/main/java/org/elasticsearch/shield/authz/Permission.java b/src/main/java/org/elasticsearch/shield/authz/Permission.java index 010f14414ba..9ebcd50cf6c 100644 --- a/src/main/java/org/elasticsearch/shield/authz/Permission.java +++ b/src/main/java/org/elasticsearch/shield/authz/Permission.java @@ -348,7 +348,6 @@ public interface Permission { } public static class Group { - private final Privilege.Index privilege; private final Predicate actionMatcher; private final String[] indices; @@ -371,10 +370,9 @@ public interface Permission { } public boolean check(String action, String index) { - return actionMatcher.apply(action) && !(index != null && !indexNameMatcher.apply(index)); + assert index != null; + return actionMatcher.apply(action) && indexNameMatcher.apply(index); } - - } } diff --git a/src/main/java/org/elasticsearch/shield/transport/ServerTransportFilter.java b/src/main/java/org/elasticsearch/shield/transport/ServerTransportFilter.java index a057e9e637d..c337fb8acba 100644 --- a/src/main/java/org/elasticsearch/shield/transport/ServerTransportFilter.java +++ b/src/main/java/org/elasticsearch/shield/transport/ServerTransportFilter.java @@ -7,6 +7,7 @@ package org.elasticsearch.shield.transport; import org.elasticsearch.common.inject.Inject; import org.elasticsearch.shield.User; +import org.elasticsearch.shield.action.ShieldActionMapper; import org.elasticsearch.shield.authc.AuthenticationException; import org.elasticsearch.shield.authc.AuthenticationService; import org.elasticsearch.shield.authz.AuthorizationService; @@ -35,25 +36,28 @@ public interface ServerTransportFilter { private final AuthenticationService authcService; private final AuthorizationService authzService; + private final ShieldActionMapper actionMapper; @Inject - public NodeProfile(AuthenticationService authcService, AuthorizationService authzService) { + public NodeProfile(AuthenticationService authcService, AuthorizationService authzService, ShieldActionMapper actionMapper) { this.authcService = authcService; this.authzService = authzService; + this.actionMapper = actionMapper; } @Override public void inbound(String action, TransportRequest request) { - /** - here we don't have a fallback user, as all incoming request are - expected to have a user attached (either in headers or in context) - We can make this assumption because in nodes we also have the - {@link ClientTransportFilter.Node} that makes sure all outgoing requsts - from all the nodes are attached with a user (either a serialize user - an authentication token + /* + here we don't have a fallback user, as all incoming request are + expected to have a user attached (either in headers or in context) + We can make this assumption because in nodes we also have the + {@link ClientTransportFilter.Node} that makes sure all outgoing requsts + from all the nodes are attached with a user (either a serialize user + an authentication token */ - User user = authcService.authenticate(action, request, null); - authzService.authorize(user, action, request); + String shieldAction = actionMapper.action(action, request); + User user = authcService.authenticate(shieldAction, request, null); + authzService.authorize(user, shieldAction, request); } } @@ -66,8 +70,8 @@ public interface ServerTransportFilter { public static class ClientProfile extends NodeProfile { @Inject - public ClientProfile(AuthenticationService authcService, AuthorizationService authzService) { - super(authcService, authzService); + public ClientProfile(AuthenticationService authcService, AuthorizationService authzService, ShieldActionMapper actionMapper) { + super(authcService, authzService, actionMapper); } @Override diff --git a/src/test/java/org/elasticsearch/shield/action/ShieldActionFilterTests.java b/src/test/java/org/elasticsearch/shield/action/ShieldActionFilterTests.java index 7c74923db75..3c2e40d2137 100644 --- a/src/test/java/org/elasticsearch/shield/action/ShieldActionFilterTests.java +++ b/src/test/java/org/elasticsearch/shield/action/ShieldActionFilterTests.java @@ -47,7 +47,7 @@ public class ShieldActionFilterTests extends ElasticsearchTestCase { signatureService = mock(SignatureService.class); auditTrail = mock(AuditTrail.class); licenseEventsNotifier = new MockLicenseEventsNotifier(); - filter = new ShieldActionFilter(ImmutableSettings.EMPTY, authcService, authzService, signatureService, auditTrail, licenseEventsNotifier); + filter = new ShieldActionFilter(ImmutableSettings.EMPTY, authcService, authzService, signatureService, auditTrail, licenseEventsNotifier, new ShieldActionMapper()); } @Test diff --git a/src/test/java/org/elasticsearch/shield/transport/ServerTransportFilterTests.java b/src/test/java/org/elasticsearch/shield/transport/ServerTransportFilterTests.java index 6fbb7d99495..81c9cc7be15 100644 --- a/src/test/java/org/elasticsearch/shield/transport/ServerTransportFilterTests.java +++ b/src/test/java/org/elasticsearch/shield/transport/ServerTransportFilterTests.java @@ -6,6 +6,7 @@ package org.elasticsearch.shield.transport; import org.elasticsearch.shield.User; +import org.elasticsearch.shield.action.ShieldActionMapper; import org.elasticsearch.shield.authc.AuthenticationException; import org.elasticsearch.shield.authc.AuthenticationService; import org.elasticsearch.shield.authz.AuthorizationException; @@ -31,7 +32,7 @@ public class ServerTransportFilterTests extends ElasticsearchTestCase { public void init() throws Exception { authcService = mock(AuthenticationService.class); authzService = mock(AuthorizationService.class); - filter = new ServerTransportFilter.NodeProfile(authcService, authzService); + filter = new ServerTransportFilter.NodeProfile(authcService, authzService, new ShieldActionMapper()); } @Test diff --git a/src/test/java/org/elasticsearch/test/ShieldClearScrollTests.java b/src/test/java/org/elasticsearch/test/ShieldClearScrollTests.java index 60bc7b4839c..f545fe939af 100644 --- a/src/test/java/org/elasticsearch/test/ShieldClearScrollTests.java +++ b/src/test/java/org/elasticsearch/test/ShieldClearScrollTests.java @@ -14,7 +14,7 @@ import org.elasticsearch.action.search.MultiSearchResponse; import org.elasticsearch.action.search.SearchPhaseExecutionException; import org.elasticsearch.shield.authc.support.SecuredString; import org.elasticsearch.shield.authz.AuthorizationException; -import org.elasticsearch.test.junit.annotations.TestLogging; +import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -24,6 +24,7 @@ import java.util.List; import static org.elasticsearch.shield.authc.support.UsernamePasswordToken.basicAuthHeaderValue; import static org.elasticsearch.test.ElasticsearchIntegrationTest.ClusterScope; import static org.elasticsearch.test.ElasticsearchIntegrationTest.Scope; +import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertThrows; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.is; @@ -77,8 +78,13 @@ public class ShieldClearScrollTests extends ShieldIntegrationTest { scrollIds = getScrollIds(multiSearchResponse); } + @After + public void clearScrolls() { + //clear all scroll ids from the default admin user, just in case any of test fails + client().prepareClearScroll().addScrollId("_all").get(); + } + @Test - @TestLogging("shield:TRACE") public void testThatClearingAllScrollIdsWorks() throws Exception { String shieldUser = "allowed_user:change_me"; String basicAuth = basicAuthHeaderValue("allowed_user", new SecuredString("change_me".toCharArray())); @@ -95,15 +101,11 @@ public class ShieldClearScrollTests extends ShieldIntegrationTest { public void testThatClearingAllScrollIdsRequirePermissions() throws Exception { String shieldUser = "denied_user:change_me"; String basicAuth = basicAuthHeaderValue("denied_user", new SecuredString("change_me".toCharArray())); - try { - internalCluster().transportClient().prepareClearScroll() + + assertThrows(internalCluster().transportClient().prepareClearScroll() .putHeader("shield.user", shieldUser) .putHeader("Authorization", basicAuth) - .addScrollId("_all").get(); - fail("Expected AuthorizationException but did not happen"); - } catch (AuthorizationException exc) { - // yay, we weren't allowed! - } + .addScrollId("_all"), AuthorizationException.class, "action [cluster:admin/indices/scroll/clear_all] is unauthorized for user [denied_user]"); // deletion of scroll ids should work ClearScrollResponse clearByIdScrollResponse = client().prepareClearScroll().setScrollIds(scrollIds).get();