From 542a484031183430fa3b628b2e6e9ad195635b47 Mon Sep 17 00:00:00 2001 From: Jay Modi Date: Tue, 25 Oct 2016 16:00:27 -0400 Subject: [PATCH] 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@62567b4c22a365282f5bf0ddc475358fd6cd1f06 --- .../authz/store/NativeRolesStore.java | 88 +++++++-------- .../authc/esnative/NativeRealmIntegTests.java | 29 +++++ .../authz/IndicesAndAliasesResolverTests.java | 2 +- .../authz/store/NativeRolesStoreTests.java | 102 ++++++++++++++++++ 4 files changed, 178 insertions(+), 43 deletions(-) diff --git a/elasticsearch/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java b/elasticsearch/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java index 5816024982b..31c87aed02f 100644 --- a/elasticsearch/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java +++ b/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 SCROLL_SIZE_SETTING = - Setting.intSetting(setting("authz.store.roles.index.scroll.size"), 1000, Property.NodeScope); - - public static final Setting SCROLL_KEEP_ALIVE_SETTING = - Setting.timeSetting(setting("authz.store.roles.index.scroll.keep_alive"), TimeValue.timeValueSeconds(10L), Property.NodeScope); - - private static final Setting CACHE_SIZE_SETTING = - Setting.intSetting(setting("authz.store.roles.index.cache.max_size"), 10000, Property.NodeScope); - private static final Setting 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 SCROLL_SIZE_SETTING = + Setting.intSetting(setting("authz.store.roles.index.scroll.size"), 1000, Property.NodeScope); + + private static final Setting SCROLL_KEEP_ALIVE_SETTING = + Setting.timeSetting(setting("authz.store.roles.index.scroll.keep_alive"), TimeValue.timeValueSeconds(10L), Property.NodeScope); + + private static final Setting CACHE_SIZE_SETTING = + Setting.intSetting(setting("authz.store.roles.index.cache.max_size"), 10000, Property.NodeScope); + private static final Setting 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 = new AtomicReference<>(State.INITIALIZED); @@ -342,11 +342,7 @@ public class NativeRolesStore extends AbstractComponent implements ClusterStateL .execute(new ActionListener() { @Override public void onResponse(IndexResponse indexResponse) { - boolean created = indexResponse.getResult() == DocWriteResponse.Result.CREATED; - if (created) { - listener.onResponse(true); - return; - } + final boolean created = indexResponse.getResult() == DocWriteResponse.Result.CREATED; 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 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() { @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 = fetchedRoleAndVersion; - if (fetchedRoleAndVersion != null) { - /* 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 - * put a potential stale result in the cache, hence the numInvalidation.get() comparison to the number of - * invalidation when we started. we just try to be on the safe side and don't cache potentially stale - * results*/ - try (final ReleasableLock ignored = readLock.acquire()) { - if (invalidationCounter == numInvalidation.get()) { - roleCache.computeIfAbsent(roleId, (k) -> fetchedRoleAndVersion); - } - } catch (ExecutionException e) { - throw new AssertionError("failed to load constant non-null value", e); - } - } else { - logger.trace("role [{}] was not found", roleId); + roleAndVersion = new RoleAndVersion(descriptor, response.getVersion()); + } else { + 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 + * put a potential stale result in the cache, hence the numInvalidation.get() comparison to the number of + * invalidation when we started. we just try to be on the safe side and don't cache potentially stale + * results*/ + try (final ReleasableLock ignored = readLock.acquire()) { + if (invalidationCounter == numInvalidation.get()) { + roleCache.computeIfAbsent(roleId, (k) -> roleAndVersion); } + } 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 listener) { + // pkg-private for testing + void executeGetRoleRequest(String role, ActionListener 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(); diff --git a/elasticsearch/src/test/java/org/elasticsearch/xpack/security/authc/esnative/NativeRealmIntegTests.java b/elasticsearch/src/test/java/org/elasticsearch/xpack/security/authc/esnative/NativeRealmIntegTests.java index f13aaca88d7..fae54b824e5 100644 --- a/elasticsearch/src/test/java/org/elasticsearch/xpack/security/authc/esnative/NativeRealmIntegTests.java +++ b/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); + } } diff --git a/elasticsearch/src/test/java/org/elasticsearch/xpack/security/authz/IndicesAndAliasesResolverTests.java b/elasticsearch/src/test/java/org/elasticsearch/xpack/security/authz/IndicesAndAliasesResolverTests.java index 12be38a9744..15fefd1912e 100644 --- a/elasticsearch/src/test/java/org/elasticsearch/xpack/security/authz/IndicesAndAliasesResolverTests.java +++ b/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)); diff --git a/elasticsearch/src/test/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStoreTests.java b/elasticsearch/src/test/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStoreTests.java index b5590627359..2a200075c15 100644 --- a/elasticsearch/src/test/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStoreTests.java +++ b/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 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 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(); + } }