From 51fdb8f638bb522f84ff2e65720e6723faf9473f Mon Sep 17 00:00:00 2001 From: Nikolaj Volgushev Date: Tue, 24 Sep 2024 10:22:05 +0200 Subject: [PATCH] Move expensive role building off transport thread (#113020) This PR moves role building off the transport thread to the generic thread pool, since role building can be expensive depending on role structure. Role building is CPU bound so this PR uses a `ThrottledTaskRunner` to limit the number of concurrent requests. I will explore adding a max queue limit in a follow up. Resolves: ES-9505 (cherry picked from commit 6cdd59bfd80af4e205973e5e053296ace183d4a3) --- .../xpack/security/Security.java | 27 ++++ .../authz/store/CompositeRolesStore.java | 80 +++++++++-- .../authz/IndicesAndAliasesResolverTests.java | 2 + .../authz/store/CompositeRolesStoreTests.java | 126 ++++++++++++++++++ 4 files changed, 225 insertions(+), 10 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java index 6f89271fcf844..8f32bcf7ace8a 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java @@ -54,12 +54,15 @@ import org.elasticsearch.common.transport.BoundTransportAddress; import org.elasticsearch.common.util.BigArrays; import org.elasticsearch.common.util.PageCacheRecycler; +import org.elasticsearch.common.util.concurrent.AbstractRunnable; import org.elasticsearch.common.util.concurrent.EsExecutors; import org.elasticsearch.common.util.concurrent.ListenableFuture; import org.elasticsearch.common.util.concurrent.ThreadContext; +import org.elasticsearch.common.util.concurrent.ThrottledTaskRunner; import org.elasticsearch.common.util.set.Sets; import org.elasticsearch.core.IOUtils; import org.elasticsearch.core.Nullable; +import org.elasticsearch.core.Releasable; import org.elasticsearch.env.Environment; import org.elasticsearch.env.NodeMetadata; import org.elasticsearch.features.FeatureService; @@ -1037,6 +1040,7 @@ Collection createComponents( serviceAccountService, dlsBitsetCache.get(), restrictedIndices, + buildRoleBuildingExecutor(threadPool, settings), new DeprecationRoleDescriptorConsumer(clusterService, threadPool) ); systemIndices.getMainIndexManager().addStateListener(allRolesStore::onSecurityIndexStateChange); @@ -1268,6 +1272,29 @@ private void submitPersistentMigrationTask(int migrationsVersion, boolean securi ); } + private static Executor buildRoleBuildingExecutor(ThreadPool threadPool, Settings settings) { + final int allocatedProcessors = EsExecutors.allocatedProcessors(settings); + final ThrottledTaskRunner throttledTaskRunner = new ThrottledTaskRunner("build_roles", allocatedProcessors, threadPool.generic()); + return r -> throttledTaskRunner.enqueueTask(new ActionListener<>() { + @Override + public void onResponse(Releasable releasable) { + try (releasable) { + r.run(); + } + } + + @Override + public void onFailure(Exception e) { + if (r instanceof AbstractRunnable abstractRunnable) { + abstractRunnable.onFailure(e); + } + // should be impossible, GENERIC pool doesn't reject anything + logger.error("unexpected failure running " + r, e); + assert false : new AssertionError("unexpected failure running " + r, e); + } + }); + } + private AuthorizationEngine getAuthorizationEngine() { return findValueFromExtensions("authorization engine", extension -> extension.getAuthorizationEngine(settings)); } diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java index d9778fda6e486..d79a3e31c1bc9 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java @@ -10,6 +10,7 @@ import org.apache.logging.log4j.Logger; import org.elasticsearch.ElasticsearchException; import org.elasticsearch.action.ActionListener; +import org.elasticsearch.action.ActionRunnable; import org.elasticsearch.common.Strings; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.cache.Cache; @@ -65,6 +66,7 @@ import java.util.Objects; import java.util.Optional; import java.util.Set; +import java.util.concurrent.Executor; import java.util.concurrent.atomic.AtomicLong; import java.util.function.Consumer; import java.util.stream.Collectors; @@ -91,6 +93,11 @@ public class CompositeRolesStore { Property.NodeScope ); private static final Logger logger = LogManager.getLogger(CompositeRolesStore.class); + /** + * See {@link #shouldForkRoleBuilding(Set)} + */ + private static final int ROLE_DESCRIPTOR_FORK_THRESHOLD = 100; + private static final int INDEX_PRIVILEGE_FORK_THRESHOLD = 1000; private final RoleProviders roleProviders; private final NativePrivilegeStore privilegeStore; @@ -106,6 +113,7 @@ public class CompositeRolesStore { private final Map internalUserRoles; private final RestrictedIndices restrictedIndices; private final ThreadContext threadContext; + private final Executor roleBuildingExecutor; public CompositeRolesStore( Settings settings, @@ -118,6 +126,7 @@ public CompositeRolesStore( ServiceAccountService serviceAccountService, DocumentSubsetBitsetCache dlsBitsetCache, RestrictedIndices restrictedIndices, + Executor roleBuildingExecutor, Consumer> effectiveRoleDescriptorsConsumer ) { this.roleProviders = roleProviders; @@ -179,6 +188,7 @@ public void providersChanged() { ); this.anonymousUser = new AnonymousUser(settings); this.threadContext = threadContext; + this.roleBuildingExecutor = roleBuildingExecutor; } public void getRoles(Authentication authentication, ActionListener> roleActionListener) { @@ -276,14 +286,31 @@ public void buildRoleFromRoleReference(RoleReference roleReference, ActionListen } else if (RolesRetrievalResult.SUPERUSER == rolesRetrievalResult) { roleActionListener.onResponse(superuserRole); } else { - buildThenMaybeCacheRole( - roleKey, - rolesRetrievalResult.getRoleDescriptors(), - rolesRetrievalResult.getMissingRoles(), - rolesRetrievalResult.isSuccess(), - invalidationCounter, - ActionListener.wrap(roleActionListener::onResponse, failureHandler) - ); + final ActionListener wrapped = ActionListener.wrap(roleActionListener::onResponse, failureHandler); + if (shouldForkRoleBuilding(rolesRetrievalResult.getRoleDescriptors())) { + roleBuildingExecutor.execute( + ActionRunnable.wrap( + wrapped, + l -> buildThenMaybeCacheRole( + roleKey, + rolesRetrievalResult.getRoleDescriptors(), + rolesRetrievalResult.getMissingRoles(), + rolesRetrievalResult.isSuccess(), + invalidationCounter, + l + ) + ) + ); + } else { + buildThenMaybeCacheRole( + roleKey, + rolesRetrievalResult.getRoleDescriptors(), + rolesRetrievalResult.getMissingRoles(), + rolesRetrievalResult.isSuccess(), + invalidationCounter, + wrapped + ); + } } }, failureHandler)); } else { @@ -291,6 +318,38 @@ public void buildRoleFromRoleReference(RoleReference roleReference, ActionListen } } + /** + * Uses heuristics such as presence of application privileges to determine if role building will be expensive + * and therefore warrants forking. + * Package-private for testing. + */ + boolean shouldForkRoleBuilding(Set roleDescriptors) { + // A role with many role descriptors is likely expensive to build + if (roleDescriptors.size() > ROLE_DESCRIPTOR_FORK_THRESHOLD) { + return true; + } + int totalIndexPrivileges = 0; + int totalRemoteIndexPrivileges = 0; + for (RoleDescriptor roleDescriptor : roleDescriptors) { + // Application privileges can also result in big automata; it's difficult to determine how big application privileges + // are so err on the side of caution + if (roleDescriptor.hasApplicationPrivileges()) { + return true; + } + // Index privilege names or remote index privilege names can result in big and complex automata + totalIndexPrivileges += roleDescriptor.getIndicesPrivileges().length; + totalRemoteIndexPrivileges += roleDescriptor.getRemoteIndicesPrivileges().length; + if (totalIndexPrivileges > INDEX_PRIVILEGE_FORK_THRESHOLD || totalRemoteIndexPrivileges > INDEX_PRIVILEGE_FORK_THRESHOLD) { + return true; + } + // Likewise for FLS/DLS + if (roleDescriptor.isUsingDocumentOrFieldLevelSecurity()) { + return true; + } + } + return false; + } + private static boolean includesSuperuserRole(RoleReference roleReference) { if (roleReference instanceof RoleReference.NamedRoleReference namedRoles) { return Arrays.asList(namedRoles.getRoleNames()).contains(ReservedRolesStore.SUPERUSER_ROLE_DESCRIPTOR.getName()); @@ -313,10 +372,11 @@ private void buildThenMaybeCacheRole( ActionListener listener ) { logger.trace( - "Building role from descriptors [{}] for names [{}] from source [{}]", + "Building role from descriptors [{}] for names [{}] from source [{}] on [{}]", roleDescriptors, roleKey.getNames(), - roleKey.getSource() + roleKey.getSource(), + Thread.currentThread().getName() ); buildRoleFromDescriptors( roleDescriptors, diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/IndicesAndAliasesResolverTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/IndicesAndAliasesResolverTests.java index 73a5ce8177153..904fb1cff820a 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/IndicesAndAliasesResolverTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/IndicesAndAliasesResolverTests.java @@ -47,6 +47,7 @@ import org.elasticsearch.common.settings.ClusterSettings; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.time.DateFormatter; +import org.elasticsearch.common.util.concurrent.EsExecutors; import org.elasticsearch.common.util.concurrent.ThreadContext; import org.elasticsearch.core.Tuple; import org.elasticsearch.index.Index; @@ -242,6 +243,7 @@ public void setup() { mock(ServiceAccountService.class), new DocumentSubsetBitsetCache(Settings.EMPTY, mock(ThreadPool.class)), RESTRICTED_INDICES, + EsExecutors.DIRECT_EXECUTOR_SERVICE, rds -> {} ) ); diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStoreTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStoreTests.java index 1a454a18e437a..da903ff7f7177 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStoreTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStoreTests.java @@ -35,6 +35,8 @@ import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.settings.ClusterSettings; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.util.concurrent.AbstractRunnable; +import org.elasticsearch.common.util.concurrent.EsExecutors; import org.elasticsearch.common.util.concurrent.ThreadContext; import org.elasticsearch.common.util.set.Sets; import org.elasticsearch.common.xcontent.XContentHelper; @@ -112,6 +114,9 @@ import org.hamcrest.Description; import org.hamcrest.Matcher; import org.hamcrest.Matchers; +import org.junit.After; +import org.junit.Before; +import org.mockito.Mockito; import java.io.IOException; import java.time.Clock; @@ -127,6 +132,7 @@ import java.util.Map; import java.util.Set; import java.util.concurrent.ExecutionException; +import java.util.concurrent.Executor; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; import java.util.function.BiConsumer; @@ -166,6 +172,7 @@ import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.ArgumentMatchers.isA; +import static org.mockito.Mockito.clearInvocations; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.doCallRealMethod; import static org.mockito.Mockito.mock; @@ -186,6 +193,23 @@ public class CompositeRolesStoreTests extends ESTestCase { TestRestrictedIndices.INTERNAL_SECURITY_MAIN_INDEX_7 ); + private Executor mockRoleBuildingExecutor; + + @Before + public void setup() { + mockRoleBuildingExecutor = mock(Executor.class); + Mockito.doAnswer(invocationOnMock -> { + final AbstractRunnable actionRunnable = (AbstractRunnable) invocationOnMock.getArguments()[0]; + actionRunnable.run(); + return null; + }).when(mockRoleBuildingExecutor).execute(any(Runnable.class)); + } + + @After + public void clear() { + clearInvocations(mockRoleBuildingExecutor); + } + public void testRolesWhenDlsFlsUnlicensed() throws IOException { MockLicenseState licenseState = mock(MockLicenseState.class); when(licenseState.isAllowed(DOCUMENT_LEVEL_SECURITY_FEATURE)).thenReturn(false); @@ -686,6 +710,62 @@ public void testNegativeLookupsCacheDisabled() { verifyNoMoreInteractions(fileRolesStore, reservedRolesStore, nativeRolesStore); } + public void testShouldForkRoleBuilding() { + final CompositeRolesStore compositeRolesStore = new CompositeRolesStore( + SECURITY_ENABLED_SETTINGS, + mock(RoleProviders.class), + mock(NativePrivilegeStore.class), + new ThreadContext(SECURITY_ENABLED_SETTINGS), + mock(), + cache, + mock(ApiKeyService.class), + mock(ServiceAccountService.class), + buildBitsetCache(), + TestRestrictedIndices.RESTRICTED_INDICES, + EsExecutors.DIRECT_EXECUTOR_SERVICE, + mock() + ); + + assertFalse(compositeRolesStore.shouldForkRoleBuilding(Set.of())); + assertFalse( + compositeRolesStore.shouldForkRoleBuilding( + Set.of( + randomValueOtherThanMany( + rd -> rd.isUsingDocumentOrFieldLevelSecurity() || rd.hasApplicationPrivileges(), + RoleDescriptorTestHelper::randomRoleDescriptor + ) + ) + ) + ); + + assertTrue(compositeRolesStore.shouldForkRoleBuilding(generateRoleDescriptors(101))); // RD count above threshold + assertTrue( + compositeRolesStore.shouldForkRoleBuilding( + Set.of( + randomValueOtherThanMany( + rd -> false == rd.isUsingDocumentOrFieldLevelSecurity(), + RoleDescriptorTestHelper::randomRoleDescriptor + ) + ) + ) + ); + assertTrue( + compositeRolesStore.shouldForkRoleBuilding( + Set.of( + randomValueOtherThanMany(rd -> false == rd.hasApplicationPrivileges(), RoleDescriptorTestHelper::randomRoleDescriptor) + ) + ) + ); + } + + private static Set generateRoleDescriptors(int numRoleDescriptors) { + Set roleDescriptors = new HashSet<>(); + for (int i = 0; i < numRoleDescriptors; i++) { + roleDescriptors.add(RoleDescriptorTestHelper.randomRoleDescriptor()); + } + return roleDescriptors; + } + public void testNegativeLookupsAreNotCachedWithFailures() { final FileRolesStore fileRolesStore = mock(FileRolesStore.class); doCallRealMethod().when(fileRolesStore).accept(anySet(), anyActionListener()); @@ -715,6 +795,7 @@ public void testNegativeLookupsAreNotCachedWithFailures() { mock(ServiceAccountService.class), documentSubsetBitsetCache, TestRestrictedIndices.RESTRICTED_INDICES, + EsExecutors.DIRECT_EXECUTOR_SERVICE, effectiveRoleDescriptors::set ); verify(fileRolesStore).addListener(anyConsumer()); // adds a listener in ctor @@ -2319,6 +2400,7 @@ public void testGetRoleForWorkflowWithRestriction() { mock(ServiceAccountService.class), buildBitsetCache(), TestRestrictedIndices.RESTRICTED_INDICES, + EsExecutors.DIRECT_EXECUTOR_SERVICE, rds -> {} ); @@ -2432,6 +2514,7 @@ public void testGetRoleForWorkflowWithoutRestriction() { mock(ServiceAccountService.class), buildBitsetCache(), TestRestrictedIndices.RESTRICTED_INDICES, + EsExecutors.DIRECT_EXECUTOR_SERVICE, rds -> {} ); @@ -2869,6 +2952,48 @@ public void testGetRoleDescriptorsListForInternalUsers() { } } + public void testForkOnExpensiveRole() { + final RoleDescriptor expectedRoleDescriptor = randomValueOtherThanMany( + rd -> false == rd.hasApplicationPrivileges(), + // skip workflow restrictions since these can produce empty, nameless roles + () -> RoleDescriptorTestHelper.builder().allowRestriction(false).build() + ); + final Consumer> rolesHandler = callback -> { + callback.onResponse(RoleRetrievalResult.success(Set.of(expectedRoleDescriptor))); + }; + final Consumer>> privilegesHandler = callback -> callback.onResponse( + Collections.emptyList() + ); + final CompositeRolesStore compositeRolesStore = setupRolesStore(rolesHandler, privilegesHandler); + + final PlainActionFuture future = new PlainActionFuture<>(); + getRoleForRoleNames(compositeRolesStore, List.of(expectedRoleDescriptor.getName()), future); + assertThat(future.actionGet().names(), equalTo(new String[] { expectedRoleDescriptor.getName() })); + + verify(mockRoleBuildingExecutor, times(1)).execute(any()); + } + + public void testDoNotForkOnInexpensiveRole() { + final RoleDescriptor expectedRoleDescriptor = randomValueOtherThanMany( + rd -> rd.isUsingDocumentOrFieldLevelSecurity() || rd.hasApplicationPrivileges(), + // skip workflow restrictions since these can produce empty, nameless roles + () -> RoleDescriptorTestHelper.builder().allowRestriction(false).build() + ); + final Consumer> rolesHandler = callback -> { + callback.onResponse(RoleRetrievalResult.success(Set.of(expectedRoleDescriptor))); + }; + final Consumer>> privilegesHandler = callback -> callback.onResponse( + Collections.emptyList() + ); + final CompositeRolesStore compositeRolesStore = setupRolesStore(rolesHandler, privilegesHandler); + + final PlainActionFuture future = new PlainActionFuture<>(); + getRoleForRoleNames(compositeRolesStore, List.of(expectedRoleDescriptor.getName()), future); + assertThat(future.actionGet().names(), equalTo(new String[] { expectedRoleDescriptor.getName() })); + + verify(mockRoleBuildingExecutor, never()).execute(any()); + } + public void testGetRoleDescriptorsListUsesRoleStoreToResolveRoleWithInternalRoleName() { String roleName = AuthenticationTestHelper.randomInternalRoleName(); RoleDescriptor expectedRoleDescriptor = new RoleDescriptor(roleName, null, null, null); @@ -3025,6 +3150,7 @@ private CompositeRolesStore buildCompositeRolesStore( serviceAccountService, documentSubsetBitsetCache, TestRestrictedIndices.RESTRICTED_INDICES, + mockRoleBuildingExecutor, roleConsumer ) { @Override