security: cache negative lookups for native roles

This changes adds a special value for negative role lookups so that we can avoid scenarios
where we overload the cluster due to continually trying to load non-existing roles as is often
the case when `unmapped_groups_as_roles` is used with the active directory realm.

Relates elastic/elasticsearch#3530 

Original commit: elastic/x-pack-elasticsearch@62567b4c22

elasticsearch/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java

@@ -85,17 +85,6 @@ import static org.elasticsearch.xpack.security.SecurityTemplateService.securityI
  */
 public class NativeRolesStore extends AbstractComponent implements ClusterStateListener {
 
-    public static final Setting<Integer> SCROLL_SIZE_SETTING =
-            Setting.intSetting(setting("authz.store.roles.index.scroll.size"), 1000, Property.NodeScope);
-
-    public static final Setting<TimeValue> SCROLL_KEEP_ALIVE_SETTING =
-            Setting.timeSetting(setting("authz.store.roles.index.scroll.keep_alive"), TimeValue.timeValueSeconds(10L), Property.NodeScope);
-
-    private static final Setting<Integer> CACHE_SIZE_SETTING =
-            Setting.intSetting(setting("authz.store.roles.index.cache.max_size"), 10000, Property.NodeScope);
-    private static final Setting<TimeValue> CACHE_TTL_SETTING =
-            Setting.timeSetting(setting("authz.store.roles.index.cache.ttl"), TimeValue.timeValueMinutes(20), Property.NodeScope);
-
     public enum State {
         INITIALIZED,
         STARTING,
@@ -105,7 +94,18 @@ public class NativeRolesStore extends AbstractComponent implements ClusterStateL
         FAILED
     }
 
-    public static final String ROLE_DOC_TYPE = "role";
+    private static final Setting<Integer> SCROLL_SIZE_SETTING =
+            Setting.intSetting(setting("authz.store.roles.index.scroll.size"), 1000, Property.NodeScope);
+
+    private static final Setting<TimeValue> SCROLL_KEEP_ALIVE_SETTING =
+            Setting.timeSetting(setting("authz.store.roles.index.scroll.keep_alive"), TimeValue.timeValueSeconds(10L), Property.NodeScope);
+
+    private static final Setting<Integer> CACHE_SIZE_SETTING =
+            Setting.intSetting(setting("authz.store.roles.index.cache.max_size"), 10000, Property.NodeScope);
+    private static final Setting<TimeValue> CACHE_TTL_SETTING =
+            Setting.timeSetting(setting("authz.store.roles.index.cache.ttl"), TimeValue.timeValueMinutes(20), Property.NodeScope);
+
+    private static final String ROLE_DOC_TYPE = "role";
 
     private final InternalClient client;
     private final AtomicReference<State> state = new AtomicReference<>(State.INITIALIZED);
@@ -342,11 +342,7 @@ public class NativeRolesStore extends AbstractComponent implements ClusterStateL
                     .execute(new ActionListener<IndexResponse>() {
                         @Override
                         public void onResponse(IndexResponse indexResponse) {
-                            boolean created = indexResponse.getResult() == DocWriteResponse.Result.CREATED;
+                            final boolean created = indexResponse.getResult() == DocWriteResponse.Result.CREATED;
-                            if (created) {
-                                listener.onResponse(true);
-                                return;
-                            }
                             clearRoleCache(role.getName(), listener, created);
                         }
 
@@ -360,7 +356,6 @@ public class NativeRolesStore extends AbstractComponent implements ClusterStateL
             logger.error((Supplier<?>) () -> new ParameterizedMessage("unable to put role [{}]", request.name()), e);
             listener.onFailure(e);
         }
-
     }
 
     public void role(String roleName, ActionListener<Role> listener) {
@@ -397,17 +392,19 @@ public class NativeRolesStore extends AbstractComponent implements ClusterStateL
             return usageStats;
         }
 
-        long count = roleCache.count();
+        long count = 0L;
         try (final ReleasableLock ignored = writeLock.acquire()) {
             for (RoleAndVersion rv : roleCache.values()) {
+                if (rv == RoleAndVersion.NON_EXISTENT) {
+                    continue;
+                }
+
+                count++;
                 Role role = rv.getRole();
                 for (Group group : role.indices()) {
                     fls = fls || group.getFieldPermissions().hasFieldLevelSecurity();
                     dls = dls || group.hasQuery();
                 }
-                if (fls && dls) {
-                    break;
-                }
             }
         }
 
@@ -475,29 +472,27 @@ public class NativeRolesStore extends AbstractComponent implements ClusterStateL
                 executeGetRoleRequest(roleId, new ActionListener<GetResponse>() {
                     @Override
                     public void onResponse(GetResponse response) {
+                        final RoleAndVersion roleAndVersion;
                         RoleDescriptor descriptor = transformRole(response);
-                        RoleAndVersion roleAndVersion = null;
                         if (descriptor != null) {
                             logger.debug("loaded role [{}] from index with version [{}]", roleId, response.getVersion());
-                            RoleAndVersion fetchedRoleAndVersion = new RoleAndVersion(descriptor, response.getVersion());
+                            roleAndVersion = new RoleAndVersion(descriptor, response.getVersion());
-                            roleAndVersion = fetchedRoleAndVersion;
+                        } else {
-                            if (fetchedRoleAndVersion != null) {
+                            roleAndVersion = RoleAndVersion.NON_EXISTENT;
-                                /* this is kinda spooky. We use a read/write lock to ensure we don't modify the cache if we hold the write
+                        }
-                                 * lock (fetching stats for instance - which is kinda overkill?) but since we fetching stuff in an async
+
-                                 * fashion we need to make sure that if the cacht got invalidated since we started the request we don't
+                        /* this is kinda spooky. We use a read/write lock to ensure we don't modify the cache if we hold the write
-                                 * put a potential stale result in the cache, hence the numInvalidation.get() comparison to the number of
+                         * lock (fetching stats for instance - which is kinda overkill?) but since we fetching stuff in an async
-                                 * invalidation when we started. we just try to be on the safe side and don't cache potentially stale
+                         * fashion we need to make sure that if the cacht got invalidated since we started the request we don't
-                                 * results*/
+                         * put a potential stale result in the cache, hence the numInvalidation.get() comparison to the number of
-                                try (final ReleasableLock ignored = readLock.acquire()) {
+                         * invalidation when we started. we just try to be on the safe side and don't cache potentially stale
-                                    if (invalidationCounter == numInvalidation.get()) {
+                         * results*/
-                                        roleCache.computeIfAbsent(roleId, (k) -> fetchedRoleAndVersion);
+                        try (final ReleasableLock ignored = readLock.acquire()) {
-                                    }
+                            if (invalidationCounter == numInvalidation.get()) {
-                                } catch (ExecutionException e) {
+                                roleCache.computeIfAbsent(roleId, (k) -> roleAndVersion);
-                                    throw new AssertionError("failed to load constant non-null value", e);
-                                }
-                            } else {
-                                logger.trace("role [{}] was not found", roleId);
                             }
+                        } catch (ExecutionException e) {
+                            throw new AssertionError("failed to load constant non-null value", e);
                         }
                         roleActionListener.onResponse(roleAndVersion);
                     }
@@ -514,7 +509,8 @@ public class NativeRolesStore extends AbstractComponent implements ClusterStateL
         }
     }
 
-    private void executeGetRoleRequest(String role, ActionListener<GetResponse> listener) {
+    // pkg-private for testing
+    void executeGetRoleRequest(String role, ActionListener<GetResponse> listener) {
         try {
             GetRequest request = client.prepareGet(SecurityTemplateService.SECURITY_INDEX_NAME, ROLE_DOC_TYPE, role).request();
             // TODO we use a threaded listener here to make sure we don't execute on a transport thread. This can be removed once
@@ -635,10 +631,18 @@ public class NativeRolesStore extends AbstractComponent implements ClusterStateL
 
     private static class RoleAndVersion {
 
+        private static final RoleAndVersion NON_EXISTENT = new RoleAndVersion();
+
         private final RoleDescriptor roleDescriptor;
         private final Role role;
         private final long version;
 
+        private RoleAndVersion() {
+            roleDescriptor = null;
+            role = null;
+            version = Long.MIN_VALUE;
+        }
+
         RoleAndVersion(RoleDescriptor roleDescriptor, long version) {
             this.roleDescriptor = roleDescriptor;
             this.role = Role.builder(roleDescriptor).build();

elasticsearch/src/test/java/org/elasticsearch/xpack/security/authc/esnative/NativeRealmIntegTests.java

@@ -667,4 +667,33 @@ public class NativeRealmIntegTests extends NativeRealmIntegTestCase {
                 () -> securityClient(client()).prepareSetEnabled("not_a_real_user", false).get());
         assertThat(e.getMessage(), containsString("only existing users can be disabled"));
     }
+
+    public void testNegativeLookupsThenCreateRole() throws Exception {
+        SecurityClient securityClient = new SecurityClient(client());
+        securityClient.preparePutUser("joe", "s3krit".toCharArray(), "unknown_role").get();
+
+        final int negativeLookups = scaledRandomIntBetween(1, 10);
+        for (int i = 0; i < negativeLookups; i++) {
+            if (anonymousEnabled && roleExists) {
+                ClusterHealthResponse response = client()
+                        .filterWithHeader(Collections.singletonMap("Authorization",
+                                basicAuthHeaderValue("joe", new SecuredString("s3krit".toCharArray()))))
+                        .admin().cluster().prepareHealth().get();
+                assertNoTimeout(response);
+            } else {
+                ElasticsearchSecurityException e = expectThrows(ElasticsearchSecurityException.class, () -> client()
+                        .filterWithHeader(Collections.singletonMap("Authorization",
+                                basicAuthHeaderValue("joe", new SecuredString("s3krit".toCharArray()))))
+                        .admin().cluster().prepareHealth().get());
+                assertThat(e.status(), is(RestStatus.FORBIDDEN));
+            }
+        }
+
+        securityClient.preparePutRole("unknown_role").cluster("all").get();
+        ClusterHealthResponse response = client()
+                .filterWithHeader(Collections.singletonMap("Authorization",
+                        basicAuthHeaderValue("joe", new SecuredString("s3krit".toCharArray()))))
+                .admin().cluster().prepareHealth().get();
+        assertNoTimeout(response);
+    }
 }

elasticsearch/src/test/java/org/elasticsearch/xpack/security/authz/IndicesAndAliasesResolverTests.java

@@ -1149,7 +1149,7 @@ public class IndicesAndAliasesResolverTests extends ESTestCase {
         return new AuthorizedIndices(user, rolesListener.actionGet(), action, metaData);
     }
 
-    private static IndexMetaData.Builder indexBuilder(String index) {
+    public static IndexMetaData.Builder indexBuilder(String index) {
         return IndexMetaData.builder(index).settings(Settings.builder()
                 .put(IndexMetaData.SETTING_NUMBER_OF_SHARDS, 1)
                 .put(IndexMetaData.SETTING_NUMBER_OF_REPLICAS, 0));

elasticsearch/src/test/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStoreTests.java

@@ -5,14 +5,45 @@
  */
 package org.elasticsearch.xpack.security.authz.store;
 
+import org.elasticsearch.Version;
+import org.elasticsearch.action.ActionListener;
+import org.elasticsearch.action.get.GetResponse;
+import org.elasticsearch.action.support.PlainActionFuture;
+import org.elasticsearch.cluster.ClusterChangedEvent;
+import org.elasticsearch.cluster.ClusterName;
+import org.elasticsearch.cluster.ClusterState;
+import org.elasticsearch.cluster.metadata.AliasMetaData;
+import org.elasticsearch.cluster.metadata.IndexMetaData;
+import org.elasticsearch.cluster.metadata.MetaData;
+import org.elasticsearch.cluster.routing.IndexRoutingTable;
+import org.elasticsearch.cluster.routing.IndexShardRoutingTable;
+import org.elasticsearch.cluster.routing.RecoverySource;
+import org.elasticsearch.cluster.routing.RoutingTable;
+import org.elasticsearch.cluster.routing.ShardRouting;
+import org.elasticsearch.cluster.routing.UnassignedInfo;
+import org.elasticsearch.cluster.routing.UnassignedInfo.Reason;
 import org.elasticsearch.common.bytes.BytesArray;
+import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.index.Index;
+import org.elasticsearch.index.get.GetResult;
+import org.elasticsearch.index.shard.ShardId;
 import org.elasticsearch.test.ESTestCase;
+import org.elasticsearch.xpack.security.InternalClient;
+import org.elasticsearch.xpack.security.SecurityTemplateService;
 import org.elasticsearch.xpack.security.authz.RoleDescriptor;
+import org.elasticsearch.xpack.security.authz.permission.Role;
 
 import java.io.IOException;
 import java.nio.charset.Charset;
 import java.nio.file.Files;
 import java.nio.file.Path;
+import java.util.Collections;
+import java.util.UUID;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import static org.elasticsearch.cluster.routing.RecoverySource.StoreRecoverySource.EXISTING_STORE_INSTANCE;
+import static org.elasticsearch.xpack.security.authz.IndicesAndAliasesResolverTests.indexBuilder;
+import static org.mockito.Mockito.mock;
 
 public class NativeRolesStoreTests extends ESTestCase {
 
@@ -26,4 +57,75 @@ public class NativeRolesStoreTests extends ESTestCase {
         assertTrue(indicesPrivileges.getFieldPermissions().grantsAccessTo("foo"));
         assertTrue(indicesPrivileges.getFieldPermissions().grantsAccessTo("boo"));
     }
+
+    public void testNegativeLookupsAreCached() {
+        final InternalClient internalClient = mock(InternalClient.class);
+        final AtomicBoolean methodCalled = new AtomicBoolean(false);
+        final NativeRolesStore rolesStore = new NativeRolesStore(Settings.EMPTY, internalClient) {
+            @Override
+            public State state() {
+                return State.STARTED;
+            }
+
+            @Override
+            void executeGetRoleRequest(String role, ActionListener<GetResponse> listener) {
+                if (methodCalled.compareAndSet(false, true)) {
+                    listener.onResponse(new GetResponse(new GetResult(SecurityTemplateService.SECURITY_INDEX_NAME, "role",
+                            role, -1, false, BytesArray.EMPTY, Collections.emptyMap())));
+                } else {
+                    fail("method called more than once!");
+                }
+            }
+        };
+
+        // setup the roles store so the security index exists
+        rolesStore.clusterChanged(new ClusterChangedEvent("negative_lookups", getClusterStateWithSecurityIndex(), getEmptyClusterState()));
+
+        final String roleName = randomAsciiOfLengthBetween(1, 10);
+        PlainActionFuture<Role> future = new PlainActionFuture<>();
+        rolesStore.role(roleName, future);
+        Role role = future.actionGet();
+        assertTrue(methodCalled.get());
+        assertNull(role);
+
+        final int numberOfRetries = scaledRandomIntBetween(1, 20);
+        for (int i = 0; i < numberOfRetries; i++) {
+            future = new PlainActionFuture<>();
+            rolesStore.role(roleName, future);
+            role = future.actionGet();
+            assertTrue(methodCalled.get());
+            assertNull(role);
+        }
+    }
+
+    private ClusterState getClusterStateWithSecurityIndex() {
+        Settings settings = Settings.builder()
+                .put(IndexMetaData.SETTING_VERSION_CREATED, Version.CURRENT)
+                .put(IndexMetaData.SETTING_NUMBER_OF_SHARDS, 1)
+                .put(IndexMetaData.SETTING_NUMBER_OF_REPLICAS, 0)
+                .build();
+        MetaData metaData = MetaData.builder()
+                .put(IndexMetaData.builder(SecurityTemplateService.SECURITY_INDEX_NAME).settings(settings)).build();
+        Index index = new Index(SecurityTemplateService.SECURITY_INDEX_NAME, UUID.randomUUID().toString());
+        ShardRouting shardRouting = ShardRouting.newUnassigned(new ShardId(index, 0), true, EXISTING_STORE_INSTANCE,
+                new UnassignedInfo(Reason.INDEX_CREATED, ""));
+        IndexShardRoutingTable table = new IndexShardRoutingTable.Builder(new ShardId(index, 0))
+                .addShard(shardRouting.initialize(randomAsciiOfLength(8), null, shardRouting.getExpectedShardSize()).moveToStarted())
+                .build();
+        RoutingTable routingTable = RoutingTable.builder()
+                .add(IndexRoutingTable
+                        .builder(index)
+                        .addIndexShard(table)
+                        .build())
+                .build();
+
+        return ClusterState.builder(new ClusterName(NativeRolesStoreTests.class.getName()))
+                .metaData(metaData)
+                .routingTable(routingTable)
+                .build();
+    }
+
+    private ClusterState getEmptyClusterState() {
+        return ClusterState.builder(new ClusterName(NativeRolesStoreTests.class.getName())).build();
+    }
 }