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 7e44d6d8b1c99..79a00fa1293bd 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; @@ -1036,6 +1039,7 @@ Collection createComponents( serviceAccountService, dlsBitsetCache.get(), restrictedIndices, + buildRoleBuildingExecutor(threadPool, settings), new DeprecationRoleDescriptorConsumer(clusterService, threadPool) ); systemIndices.getMainIndexManager().addStateListener(allRolesStore::onSecurityIndexStateChange); @@ -1267,6 +1271,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 1886a945cbf38..9587533d87d86 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 @@ -2318,6 +2399,7 @@ public void testGetRoleForWorkflowWithRestriction() { mock(ServiceAccountService.class), buildBitsetCache(), TestRestrictedIndices.RESTRICTED_INDICES, + EsExecutors.DIRECT_EXECUTOR_SERVICE, rds -> {} ); @@ -2431,6 +2513,7 @@ public void testGetRoleForWorkflowWithoutRestriction() { mock(ServiceAccountService.class), buildBitsetCache(), TestRestrictedIndices.RESTRICTED_INDICES, + EsExecutors.DIRECT_EXECUTOR_SERVICE, rds -> {} ); @@ -2868,6 +2951,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); @@ -3024,6 +3149,7 @@ private CompositeRolesStore buildCompositeRolesStore( serviceAccountService, documentSubsetBitsetCache, TestRestrictedIndices.RESTRICTED_INDICES, + mockRoleBuildingExecutor, roleConsumer ) { @Override