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@4972df459f

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));
+    }
+}

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
+                if (AuthorizationUtils.shouldReplaceUserWithSystem(threadContext, action)) {
-                /*
-                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)) {
                     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);

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<String> 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:
+     *
+     * <ul>
+     *     <li>the action is an internal action and no user is associated with the request</li>
+     *     <li>the action is an internal action and the system user is already associated with the request</li>
+     *     <li>the action is an internal action and the thread context contains a non-internal action as the originating action</li>
+     * </ul>
+     *
+     * @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);
+    }
+}

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;

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<String> 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 <T extends TransportResponse> void sendRequest(DiscoveryNode node, String action, TransportRequest request, TransportRequestOptions options, TransportResponseHandler<T> 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...
+        // which means that the user is copied over to system actions so we need to change the user
-        if ((clientFilter instanceof ClientTransportFilter.Node) && INTERNAL_PREDICATE.test(action)) {
+        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 {