From c3b6146a726d4ad35ee44dce87118b4040caee55 Mon Sep 17 00:00:00 2001 From: jaymode Date: Mon, 1 Feb 2016 13:33:22 -0500 Subject: [PATCH] shield: clean up hack to force switching to the system user for internal action This commit cleans up the hack we had forcefully switching the request to execute under the system user when a internal action gets triggered from a system request. The authorization service now tracks the originating request in the context to allow us to validate if the request should be run as the system user. The system user should be used only when a user action causes an internal action, which needs to be run by the system user. Closes elastic/elasticsearch#1403 Original commit: elastic/x-pack-elasticsearch@4972df459f7d13875e5e50c47fddb058c2f59135 --- .../shield/authz/AuthorizationUtilsTest.java | 54 +++++++++++++++ .../shield/action/ShieldActionFilter.java | 35 +--------- .../shield/authz/AuthorizationUtils.java | 66 +++++++++++++++++++ .../authz/InternalAuthorizationService.java | 11 ++++ .../ShieldServerTransportService.java | 12 ++-- 5 files changed, 137 insertions(+), 41 deletions(-) create mode 100644 elasticsearch/x-pack/license-plugin/src/test/java/org/elasticsearch/shield/authz/AuthorizationUtilsTest.java create mode 100644 elasticsearch/x-pack/shield/src/main/java/org/elasticsearch/shield/authz/AuthorizationUtils.java diff --git a/elasticsearch/x-pack/license-plugin/src/test/java/org/elasticsearch/shield/authz/AuthorizationUtilsTest.java b/elasticsearch/x-pack/license-plugin/src/test/java/org/elasticsearch/shield/authz/AuthorizationUtilsTest.java new file mode 100644 index 00000000000..237e8848cc8 --- /dev/null +++ b/elasticsearch/x-pack/license-plugin/src/test/java/org/elasticsearch/shield/authz/AuthorizationUtilsTest.java @@ -0,0 +1,54 @@ +/* + * 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.authz; + +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.util.concurrent.ThreadContext; +import org.elasticsearch.shield.InternalSystemUser; +import org.elasticsearch.shield.User; +import org.elasticsearch.shield.authc.InternalAuthenticationService; +import org.elasticsearch.test.ESTestCase; +import org.junit.Before; + +import static org.hamcrest.Matchers.is; + +/** + * Unit tests for the AuthorizationUtils class + */ +public class AuthorizationUtilsTest extends ESTestCase { + + private ThreadContext threadContext; + + @Before + public void setupContext() { + threadContext = new ThreadContext(Settings.EMPTY); + } + + public void testSystemUserSwitchNonInternalAction() { + assertThat(AuthorizationUtils.shouldReplaceUserWithSystem(threadContext, randomFrom("indices:foo", "cluster:bar")), is(false)); + } + + public void testSystemUserSwitchWithNullorSystemUser() { + if (randomBoolean()) { + threadContext.putTransient(InternalAuthenticationService.USER_KEY, InternalSystemUser.INSTANCE); + } + assertThat(AuthorizationUtils.shouldReplaceUserWithSystem(threadContext, "internal:something"), is(true)); + } + + public void testSystemUserSwitchWithNonSystemUser() { + User user = new User(randomAsciiOfLength(6), new String[] {}); + threadContext.putTransient(InternalAuthenticationService.USER_KEY, user); + threadContext.putTransient(InternalAuthorizationService.ORIGINATING_ACTION_KEY, randomFrom("indices:foo", "cluster:bar")); + assertThat(AuthorizationUtils.shouldReplaceUserWithSystem(threadContext, "internal:something"), is(true)); + } + + public void testSystemUserSwitchWithNonSystemUserAndInternalAction() { + User user = new User(randomAsciiOfLength(6), new String[] {}); + threadContext.putTransient(InternalAuthenticationService.USER_KEY, user); + threadContext.putTransient(InternalAuthorizationService.ORIGINATING_ACTION_KEY, randomFrom("internal:foo/bar")); + assertThat(AuthorizationUtils.shouldReplaceUserWithSystem(threadContext, "internal:something"), is(false)); + } +} \ No newline at end of file diff --git a/elasticsearch/x-pack/shield/src/main/java/org/elasticsearch/shield/action/ShieldActionFilter.java b/elasticsearch/x-pack/shield/src/main/java/org/elasticsearch/shield/action/ShieldActionFilter.java index 59d32430b4f..258c324c61f 100644 --- a/elasticsearch/x-pack/shield/src/main/java/org/elasticsearch/shield/action/ShieldActionFilter.java +++ b/elasticsearch/x-pack/shield/src/main/java/org/elasticsearch/shield/action/ShieldActionFilter.java @@ -27,6 +27,7 @@ import org.elasticsearch.shield.audit.AuditTrail; import org.elasticsearch.shield.authc.AuthenticationService; import org.elasticsearch.shield.authc.InternalAuthenticationService; import org.elasticsearch.shield.authz.AuthorizationService; +import org.elasticsearch.shield.authz.AuthorizationUtils; import org.elasticsearch.shield.authz.privilege.HealthAndStatsPrivilege; import org.elasticsearch.shield.crypto.CryptoService; import org.elasticsearch.shield.license.ShieldLicenseState; @@ -97,37 +98,7 @@ public class ShieldActionFilter extends AbstractComponent implements ActionFilte threadContext.getTransient(InternalAuthenticationService.USER_KEY) != null; try { if (licenseState.securityEnabled()) { - // FIXME yet another hack. Needed to work around something like - /* - FailedNodeException[total failure in fetching]; nested: ElasticsearchSecurityException[action [internal:gateway/local/started_shards] is unauthorized for user [test_user]]; - at org.elasticsearch.gateway.AsyncShardFetch$1.onFailure(AsyncShardFetch.java:284) - at org.elasticsearch.action.support.TransportAction$1.onFailure(TransportAction.java:84) - at org.elasticsearch.shield.action.ShieldActionFilter.apply(ShieldActionFilter.java:121) - at org.elasticsearch.action.support.TransportAction$RequestFilterChain.proceed(TransportAction.java:133) - at org.elasticsearch.action.support.TransportAction.execute(TransportAction.java:107) - at org.elasticsearch.action.support.TransportAction.execute(TransportAction.java:74) - at org.elasticsearch.gateway.TransportNodesListGatewayStartedShards.list(TransportNodesListGatewayStartedShards.java:78) - at org.elasticsearch.gateway.AsyncShardFetch.asyncFetch(AsyncShardFetch.java:274) - at org.elasticsearch.gateway.AsyncShardFetch.fetchData(AsyncShardFetch.java:124) - at org.elasticsearch.gateway.GatewayAllocator$InternalPrimaryShardAllocator.fetchData(GatewayAllocator.java:156) - at org.elasticsearch.gateway.PrimaryShardAllocator.allocateUnassigned(PrimaryShardAllocator.java:83) - at org.elasticsearch.gateway.GatewayAllocator.allocateUnassigned(GatewayAllocator.java:120) - at org.elasticsearch.cluster.routing.allocation.allocator.ShardsAllocators.allocateUnassigned(ShardsAllocators.java:72) - at org.elasticsearch.cluster.routing.allocation.AllocationService.reroute(AllocationService.java:309) - at org.elasticsearch.cluster.routing.allocation.AllocationService.reroute(AllocationService.java:273) - at org.elasticsearch.cluster.routing.allocation.AllocationService.reroute(AllocationService.java:259) - at org.elasticsearch.cluster.routing.RoutingService$2.execute(RoutingService.java:158) - at org.elasticsearch.cluster.ClusterStateUpdateTask.execute(ClusterStateUpdateTask.java:45) - at org.elasticsearch.cluster.service.InternalClusterService.runTasksForExecutor(InternalClusterService.java:447) - at org.elasticsearch.cluster.service.InternalClusterService$UpdateTask.run(InternalClusterService.java:757) - at org.elasticsearch.common.util.concurrent.EsThreadPoolExecutor$FilterRunnable.run(EsThreadPoolExecutor.java:211) - at org.elasticsearch.common.util.concurrent.PrioritizedEsThreadPoolExecutor$TieBreakingPrioritizedRunnable.runAndClean(PrioritizedEsThreadPoolExecutor.java:237) - at org.elasticsearch.common.util.concurrent.PrioritizedEsThreadPoolExecutor$TieBreakingPrioritizedRunnable.run(PrioritizedEsThreadPoolExecutor.java:200) - at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142) - at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617) - at java.lang.Thread.run(Thread.java:745) - */ - if (INTERNAL_PREDICATE.test(action)) { + if (AuthorizationUtils.shouldReplaceUserWithSystem(threadContext, action)) { try (ThreadContext.StoredContext ctx = threadContext.stashContext()) { String shieldAction = actionMapper.action(action, request); User user = authcService.authenticate(shieldAction, request, InternalSystemUser.INSTANCE); @@ -145,7 +116,6 @@ public class ShieldActionFilter extends AbstractComponent implements ActionFilte } } - /** here we fallback on the system user. Internal system requests are requests that are triggered by the system itself (e.g. pings, update mappings, share relocation, etc...) and were not originated @@ -156,7 +126,6 @@ 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. */ - String shieldAction = actionMapper.action(action, request); User user = authcService.authenticate(shieldAction, request, InternalSystemUser.INSTANCE); authzService.authorize(user, shieldAction, request); diff --git a/elasticsearch/x-pack/shield/src/main/java/org/elasticsearch/shield/authz/AuthorizationUtils.java b/elasticsearch/x-pack/shield/src/main/java/org/elasticsearch/shield/authz/AuthorizationUtils.java new file mode 100644 index 00000000000..2cf4407092e --- /dev/null +++ b/elasticsearch/x-pack/shield/src/main/java/org/elasticsearch/shield/authz/AuthorizationUtils.java @@ -0,0 +1,66 @@ +/* + * 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.authz; + +import org.elasticsearch.common.util.concurrent.ThreadContext; +import org.elasticsearch.shield.InternalSystemUser; +import org.elasticsearch.shield.User; +import org.elasticsearch.shield.authc.InternalAuthenticationService; +import org.elasticsearch.shield.support.AutomatonPredicate; +import org.elasticsearch.shield.support.Automatons; + +import java.util.function.Predicate; + +/** + * + */ +public final class AuthorizationUtils { + + private static final Predicate INTERNAL_PREDICATE = new AutomatonPredicate(Automatons.patterns("internal:*")); + + private AuthorizationUtils() {} + + /** + * This method is used to determine if a request should be executed as the system user, even if the request already + * has a user associated with it. + * + * In order for the system user to be used, one of the following conditions must be true: + * + * + * + * @param threadContext the {@link ThreadContext} that contains the headers and context associated with the request + * @param action the action name that is being executed + * @return true if the system user should be used to execute a request + */ + public static boolean shouldReplaceUserWithSystem(ThreadContext threadContext, String action) { + if (isInternalAction(action) == false) { + return false; + } + + User user = threadContext.getTransient(InternalAuthenticationService.USER_KEY); + if (user == null || InternalSystemUser.is(user)) { + return true; + } + + // we have a internal action being executed by a user that is not the system user, lets verify that there is a + // originating action that is not a internal action + final String originatingAction = threadContext.getTransient(InternalAuthorizationService.ORIGINATING_ACTION_KEY); + if (originatingAction != null && isInternalAction(originatingAction) == false) { + return true; + } + + // either there was no originating action or it was a internal action, we should not replace under these circumstances + return false; + } + + public static boolean isInternalAction(String action) { + return INTERNAL_PREDICATE.test(action); + } +} diff --git a/elasticsearch/x-pack/shield/src/main/java/org/elasticsearch/shield/authz/InternalAuthorizationService.java b/elasticsearch/x-pack/shield/src/main/java/org/elasticsearch/shield/authz/InternalAuthorizationService.java index 593e63ed06d..2fca2cb26c0 100644 --- a/elasticsearch/x-pack/shield/src/main/java/org/elasticsearch/shield/authz/InternalAuthorizationService.java +++ b/elasticsearch/x-pack/shield/src/main/java/org/elasticsearch/shield/authz/InternalAuthorizationService.java @@ -55,6 +55,7 @@ import static org.elasticsearch.shield.support.Exceptions.authorizationError; public class InternalAuthorizationService extends AbstractComponent implements AuthorizationService { public static final String INDICES_PERMISSIONS_KEY = "_indices_permissions"; + static final String ORIGINATING_ACTION_KEY = "_originating_action_name"; private final ClusterService clusterService; private final RolesStore rolesStore; @@ -109,6 +110,9 @@ public class InternalAuthorizationService extends AbstractComponent implements A @Override public void authorize(User user, String action, TransportRequest request) throws ElasticsearchSecurityException { + // prior to doing any authorization lets set the originating action in the context only + setOriginatingAction(action); + // first we need to check if the user is the system. If it is, we'll just authorize the system access if (InternalSystemUser.is(user)) { if (InternalSystemUser.isAuthorized(action)) { @@ -227,6 +231,13 @@ public class InternalAuthorizationService extends AbstractComponent implements A } } + private void setOriginatingAction(String action) { + String originatingAction = threadContext.getTransient(ORIGINATING_ACTION_KEY); + if (originatingAction == null) { + threadContext.putTransient(ORIGINATING_ACTION_KEY, action); + } + } + private GlobalPermission permission(String[] roleNames) { if (roleNames.length == 0) { return GlobalPermission.NONE; diff --git a/elasticsearch/x-pack/shield/src/main/java/org/elasticsearch/shield/transport/ShieldServerTransportService.java b/elasticsearch/x-pack/shield/src/main/java/org/elasticsearch/shield/transport/ShieldServerTransportService.java index 9397e019f33..287c2f147fb 100644 --- a/elasticsearch/x-pack/shield/src/main/java/org/elasticsearch/shield/transport/ShieldServerTransportService.java +++ b/elasticsearch/x-pack/shield/src/main/java/org/elasticsearch/shield/transport/ShieldServerTransportService.java @@ -12,10 +12,9 @@ import org.elasticsearch.common.util.concurrent.ThreadContext; import org.elasticsearch.shield.action.ShieldActionMapper; import org.elasticsearch.shield.authc.AuthenticationService; import org.elasticsearch.shield.authz.AuthorizationService; +import org.elasticsearch.shield.authz.AuthorizationUtils; import org.elasticsearch.shield.authz.accesscontrol.RequestContext; import org.elasticsearch.shield.license.ShieldLicenseState; -import org.elasticsearch.shield.support.AutomatonPredicate; -import org.elasticsearch.shield.support.Automatons; import org.elasticsearch.shield.transport.netty.ShieldNettyTransport; import org.elasticsearch.tasks.Task; import org.elasticsearch.threadpool.ThreadPool; @@ -33,7 +32,6 @@ import org.elasticsearch.transport.TransportSettings; import java.util.Collections; import java.util.HashMap; import java.util.Map; -import java.util.function.Predicate; import java.util.function.Supplier; import static org.elasticsearch.shield.transport.netty.ShieldNettyTransport.TRANSPORT_CLIENT_AUTH_DEFAULT; @@ -49,8 +47,6 @@ import static org.elasticsearch.shield.transport.netty.ShieldNettyTransport.TRAN public class ShieldServerTransportService extends TransportService { public static final String SETTING_NAME = "shield.type"; - // FIXME clean up this hack - static final Predicate INTERNAL_PREDICATE = new AutomatonPredicate(Automatons.patterns("internal:*")); protected final AuthenticationService authcService; protected final AuthorizationService authzService; @@ -78,10 +74,10 @@ public class ShieldServerTransportService extends TransportService { @Override public void sendRequest(DiscoveryNode node, String action, TransportRequest request, TransportRequestOptions options, TransportResponseHandler handler) { - // FIXME this is really just a hack. What happens is that we send a request and we always copy headers over // Sometimes a system action gets executed like a internal create index request or update mappings request - // which means that the user is copied over to system actions and these really fail for internal things... - if ((clientFilter instanceof ClientTransportFilter.Node) && INTERNAL_PREDICATE.test(action)) { + // which means that the user is copied over to system actions so we need to change the user + if ((clientFilter instanceof ClientTransportFilter.Node) && + AuthorizationUtils.shouldReplaceUserWithSystem(threadPool.getThreadContext(), action)) { final ThreadContext.StoredContext original = threadPool.getThreadContext().newStoredContext(); try (ThreadContext.StoredContext ctx = threadPool.getThreadContext().stashContext()) { try {