From 990c65b6de0d0fc5704dbeb98521b9e7d7740529 Mon Sep 17 00:00:00 2001 From: Slobodan Adamovic Date: Wed, 18 Dec 2024 15:11:33 +0100 Subject: [PATCH 1/4] Test queryable built-in role synchronization --- .../QueryableBuiltInRolesSynchronizer.java | 8 + .../QueryableReservedRolesProvider.java | 2 +- ...ueryableBuiltInRolesSynchronizerTests.java | 513 ++++++++++++++++++ .../QueryableBuiltInRolesUtilsTests.java | 4 +- 4 files changed, 524 insertions(+), 3 deletions(-) create mode 100644 x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/support/QueryableBuiltInRolesSynchronizerTests.java diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/support/QueryableBuiltInRolesSynchronizer.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/support/QueryableBuiltInRolesSynchronizer.java index 60163434e212f..479cbaf4b852b 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/support/QueryableBuiltInRolesSynchronizer.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/support/QueryableBuiltInRolesSynchronizer.java @@ -452,6 +452,14 @@ static class MarkRolesAsSyncedTask implements ClusterStateTaskListener { this.newRoleDigests = newRoleDigests; } + public Map getExpectedRoleDigests() { + return expectedRoleDigests; + } + + public Map getNewRoleDigests() { + return newRoleDigests; + } + Tuple> execute(ClusterState state) { IndexMetadata indexMetadata = state.metadata().index(concreteSecurityIndexName); if (indexMetadata == null) { diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/support/QueryableReservedRolesProvider.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/support/QueryableReservedRolesProvider.java index 710e94b7ac879..1786d20410990 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/support/QueryableReservedRolesProvider.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/support/QueryableReservedRolesProvider.java @@ -23,7 +23,7 @@ * The reserved roles are static and do not change during runtime, hence this provider will never notify any listeners. *

*/ -public final class QueryableReservedRolesProvider implements QueryableBuiltInRoles.Provider { +public class QueryableReservedRolesProvider implements QueryableBuiltInRoles.Provider { private final Supplier reservedRolesSupplier; diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/support/QueryableBuiltInRolesSynchronizerTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/support/QueryableBuiltInRolesSynchronizerTests.java new file mode 100644 index 0000000000000..8b7c580b82ffa --- /dev/null +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/support/QueryableBuiltInRolesSynchronizerTests.java @@ -0,0 +1,513 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +package org.elasticsearch.xpack.security.support; + +import org.elasticsearch.action.ActionListener; +import org.elasticsearch.action.DocWriteResponse; +import org.elasticsearch.action.support.WriteRequest; +import org.elasticsearch.cluster.ClusterChangedEvent; +import org.elasticsearch.cluster.ClusterName; +import org.elasticsearch.cluster.ClusterState; +import org.elasticsearch.cluster.block.ClusterBlocks; +import org.elasticsearch.cluster.metadata.IndexMetadata; +import org.elasticsearch.cluster.metadata.Metadata; +import org.elasticsearch.cluster.node.DiscoveryNodeRole; +import org.elasticsearch.cluster.node.DiscoveryNodeUtils; +import org.elasticsearch.cluster.node.DiscoveryNodes; +import org.elasticsearch.cluster.node.VersionInformation; +import org.elasticsearch.cluster.service.ClusterService; +import org.elasticsearch.cluster.service.MasterServiceTaskQueue; +import org.elasticsearch.common.Priority; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.util.concurrent.EsExecutors; +import org.elasticsearch.common.util.concurrent.ThreadContext; +import org.elasticsearch.features.FeatureService; +import org.elasticsearch.index.IndexVersions; +import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.test.VersionUtils; +import org.elasticsearch.test.index.IndexVersionUtils; +import org.elasticsearch.threadpool.ThreadPool; +import org.elasticsearch.xpack.core.security.action.role.BulkRolesResponse; +import org.elasticsearch.xpack.core.security.authz.RoleDescriptor; +import org.elasticsearch.xpack.core.security.authz.store.ReservedRolesStore; +import org.elasticsearch.xpack.core.security.test.TestRestrictedIndices; +import org.elasticsearch.xpack.security.authz.store.FileRolesStore; +import org.elasticsearch.xpack.security.authz.store.NativeRolesStore; +import org.elasticsearch.xpack.security.support.QueryableBuiltInRolesSynchronizer.MarkRolesAsSyncedTask; +import org.elasticsearch.xpack.security.test.SecurityTestUtils; +import org.junit.Before; +import org.junit.BeforeClass; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.List; +import java.util.Set; + +import static org.elasticsearch.gateway.GatewayService.STATE_NOT_RECOVERED_BLOCK; +import static org.elasticsearch.xpack.security.support.QueryableBuiltInRolesSynchronizer.QUERYABLE_BUILT_IN_ROLES_FEATURE; +import static org.elasticsearch.xpack.security.support.QueryableBuiltInRolesUtilsTests.buildQueryableBuiltInRoles; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.argThat; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; +import static org.mockito.Mockito.verifyNoMoreInteractions; +import static org.mockito.Mockito.when; + +public class QueryableBuiltInRolesSynchronizerTests extends ESTestCase { + + private static final ClusterName CLUSTER_NAME = new ClusterName("queryable-built-in-roles-synchronizer-tests"); + private static final ClusterState EMPTY_CLUSTER_STATE = new ClusterState.Builder(CLUSTER_NAME).build(); + + private QueryableBuiltInRolesSynchronizer synchronizer; + private NativeRolesStore nativeRolesStore; + private ClusterService clusterService; + private FeatureService featureService; + private ThreadPool threadPool; + + private MasterServiceTaskQueue taskQueue; + + private QueryableReservedRolesProvider reservedRolesProvider; + + @BeforeClass + public static void setupReservedRolesStore() { + new ReservedRolesStore(); // initialize the store + } + + @Before + public void setupSynchronizer() { + threadPool = mock(ThreadPool.class); + when(threadPool.getThreadContext()).thenReturn(new ThreadContext(Settings.EMPTY)); + when(threadPool.generic()).thenReturn(EsExecutors.DIRECT_EXECUTOR_SERVICE); + + nativeRolesStore = mock(NativeRolesStore.class); + clusterService = mock(ClusterService.class); + taskQueue = mockTaskQueue(clusterService); + + QueryableBuiltInRolesProviderFactory rolesProviderFactory = mock(QueryableBuiltInRolesProviderFactory.class); + reservedRolesProvider = mock(QueryableReservedRolesProvider.class); + when(rolesProviderFactory.createProvider(any(), any())).thenReturn(reservedRolesProvider); + + featureService = mock(FeatureService.class); + + synchronizer = new QueryableBuiltInRolesSynchronizer( + clusterService, + featureService, + rolesProviderFactory, + nativeRolesStore, + mock(ReservedRolesStore.class), + mock(FileRolesStore.class), + threadPool + ); + } + + private void assertInitialState() { + verifyNoInteractions(nativeRolesStore); + verifyNoInteractions(featureService); + verifyNoInteractions(taskQueue); + + verify(reservedRolesProvider, times(1)).addListener(any()); + verifyNoMoreInteractions(reservedRolesProvider); + + verify(clusterService, times(1)).addLifecycleListener(any()); + verify(clusterService, times(1)).createTaskQueue(eq("mark-built-in-roles-as-synced-task-queue"), eq(Priority.LOW), any()); + verifyNoMoreInteractions(clusterService); + + verify(threadPool, times(1)).generic(); + verifyNoMoreInteractions(threadPool); + } + + public void testSuccessfulSync() { + assertInitialState(); + + ClusterState clusterState = markShardsAvailable(createClusterStateWithOpenSecurityIndex()).nodes(localNodeMaster()) + .blocks(emptyClusterBlocks()) + .build(); + + when(clusterService.state()).thenReturn(clusterState); + when(featureService.clusterHasFeature(any(), eq(QUERYABLE_BUILT_IN_ROLES_FEATURE))).thenReturn(true); + + final QueryableBuiltInRoles builtInRoles = buildQueryableBuiltInRoles( + Set.of( + ReservedRolesStore.SUPERUSER_ROLE_DESCRIPTOR, + ReservedRolesStore.roleDescriptor("viewer"), + ReservedRolesStore.roleDescriptor("editor") + ) + ); + when(reservedRolesProvider.getRoles()).thenReturn(builtInRoles); + mockEnabledNativeStore(builtInRoles.roleDescriptors(), Set.of()); + + synchronizer.clusterChanged(event(clusterState)); + + // called once on successful sync to update the digests in the cluster state + verify(taskQueue, times(1)).submitTask(any(), argThat(task -> task.getNewRoleDigests().equals(builtInRoles.rolesDigest())), any()); + verify(nativeRolesStore, times(1)).isEnabled(); + verify(featureService, times(1)).clusterHasFeature(any(), eq(QUERYABLE_BUILT_IN_ROLES_FEATURE)); + verify(reservedRolesProvider, times(1)).getRoles(); + verify(nativeRolesStore, times(1)).putRoles( + eq(WriteRequest.RefreshPolicy.IMMEDIATE), + eq(builtInRoles.roleDescriptors()), + eq(false), + any() + ); + verify(clusterService, times(2)).state(); + verifyNoMoreInteractions(nativeRolesStore, featureService, taskQueue, reservedRolesProvider, threadPool, clusterService); + } + + public void testNotMaster() { + assertInitialState(); + + ClusterState clusterState = spy( + markShardsAvailable(createClusterStateWithOpenSecurityIndex()).nodes(localNodeNotMaster()).blocks(emptyClusterBlocks()).build() + ); + + when(clusterService.state()).thenReturn(clusterState); + when(featureService.clusterHasFeature(any(), eq(QUERYABLE_BUILT_IN_ROLES_FEATURE))).thenReturn(true); + + final QueryableBuiltInRoles builtInRoles = buildQueryableBuiltInRoles(Set.of(ReservedRolesStore.SUPERUSER_ROLE_DESCRIPTOR)); + when(reservedRolesProvider.getRoles()).thenReturn(builtInRoles); + mockEnabledNativeStore(builtInRoles.roleDescriptors(), Set.of()); + + synchronizer.clusterChanged(event(clusterState)); + + verify(clusterState, times(0)).clusterRecovered(); + verify(reservedRolesProvider, times(0)).getRoles(); + verifyNoMoreInteractions(nativeRolesStore, featureService, taskQueue, reservedRolesProvider, threadPool, clusterService); + } + + public void testClusterNotRecovered() { + assertInitialState(); + + ClusterState clusterState = spy( + markShardsAvailable(createClusterStateWithOpenSecurityIndex()).nodes(localNodeMaster()) + .blocks(stateNotRecoveredClusterBlocks()) + .build() + ); + + when(clusterService.state()).thenReturn(clusterState); + when(featureService.clusterHasFeature(any(), eq(QUERYABLE_BUILT_IN_ROLES_FEATURE))).thenReturn(true); + + final QueryableBuiltInRoles builtInRoles = buildQueryableBuiltInRoles(Set.of(ReservedRolesStore.SUPERUSER_ROLE_DESCRIPTOR)); + when(reservedRolesProvider.getRoles()).thenReturn(builtInRoles); + mockEnabledNativeStore(builtInRoles.roleDescriptors(), Set.of()); + + synchronizer.clusterChanged(event(clusterState)); + + verify(clusterState, times(1)).clusterRecovered(); + verify(reservedRolesProvider, times(0)).getRoles(); + verifyNoMoreInteractions(nativeRolesStore, featureService, taskQueue, reservedRolesProvider, threadPool, clusterService); + } + + public void testNativeRolesDisabled() { + assertInitialState(); + + ClusterState clusterState = spy( + markShardsAvailable(createClusterStateWithOpenSecurityIndex()).nodes(localNodeMaster()).blocks(emptyClusterBlocks()).build() + ); + + when(clusterService.state()).thenReturn(clusterState); + when(featureService.clusterHasFeature(any(), eq(QUERYABLE_BUILT_IN_ROLES_FEATURE))).thenReturn(true); + + when(reservedRolesProvider.getRoles()).thenReturn(buildQueryableBuiltInRoles(Set.of(ReservedRolesStore.SUPERUSER_ROLE_DESCRIPTOR))); + mockDisabledNativeStore(); + + synchronizer.clusterChanged(event(clusterState)); + + verify(clusterState, times(1)).clusterRecovered(); + verify(nativeRolesStore, times(1)).isEnabled(); + verify(clusterState, times(2)).nodes(); + verifyNoMoreInteractions(nativeRolesStore, featureService, taskQueue, reservedRolesProvider, threadPool, clusterService); + } + + public void testMixedVersionsCluster() { + assertInitialState(); + + ClusterState clusterState = spy( + markShardsAvailable(createClusterStateWithOpenSecurityIndex()).nodes(mixedVersionNodes()).blocks(emptyClusterBlocks()).build() + ); + + when(clusterService.state()).thenReturn(clusterState); + when(featureService.clusterHasFeature(any(), eq(QUERYABLE_BUILT_IN_ROLES_FEATURE))).thenReturn(true); + + final QueryableBuiltInRoles builtInRoles = buildQueryableBuiltInRoles(Set.of(ReservedRolesStore.SUPERUSER_ROLE_DESCRIPTOR)); + when(reservedRolesProvider.getRoles()).thenReturn(builtInRoles); + mockEnabledNativeStore(builtInRoles.roleDescriptors(), Set.of()); + + synchronizer.clusterChanged(event(clusterState)); + + verify(clusterState, times(1)).clusterRecovered(); + verify(nativeRolesStore, times(1)).isEnabled(); + verify(clusterState, times(4)).nodes(); + verifyNoMoreInteractions(nativeRolesStore, featureService, taskQueue, reservedRolesProvider, threadPool, clusterService); + + } + + public void testNoDataNodes() { + assertInitialState(); + + ClusterState clusterState = spy( + markShardsAvailable(createClusterStateWithOpenSecurityIndex()).nodes(noDataNodes()).blocks(emptyClusterBlocks()).build() + ); + + when(clusterService.state()).thenReturn(clusterState); + when(featureService.clusterHasFeature(any(), eq(QUERYABLE_BUILT_IN_ROLES_FEATURE))).thenReturn(true); + + final QueryableBuiltInRoles builtInRoles = buildQueryableBuiltInRoles(Set.of(ReservedRolesStore.SUPERUSER_ROLE_DESCRIPTOR)); + when(reservedRolesProvider.getRoles()).thenReturn(builtInRoles); + mockEnabledNativeStore(builtInRoles.roleDescriptors(), Set.of()); + + synchronizer.clusterChanged(event(clusterState)); + + verify(clusterState, times(1)).clusterRecovered(); + verify(nativeRolesStore, times(1)).isEnabled(); + verify(clusterState, times(3)).nodes(); + verifyNoMoreInteractions(nativeRolesStore, featureService, taskQueue, reservedRolesProvider, threadPool, clusterService); + } + + public void testFeatureNotSupported() { + assertInitialState(); + + ClusterState clusterState = spy( + markShardsAvailable(createClusterStateWithOpenSecurityIndex()).nodes(localNodeMaster()).blocks(emptyClusterBlocks()).build() + ); + + when(clusterService.state()).thenReturn(clusterState); + when(featureService.clusterHasFeature(any(), eq(QUERYABLE_BUILT_IN_ROLES_FEATURE))).thenReturn(false); + + final QueryableBuiltInRoles builtInRoles = buildQueryableBuiltInRoles(Set.of(ReservedRolesStore.SUPERUSER_ROLE_DESCRIPTOR)); + when(reservedRolesProvider.getRoles()).thenReturn(builtInRoles); + mockEnabledNativeStore(builtInRoles.roleDescriptors(), Set.of()); + + synchronizer.clusterChanged(event(clusterState)); + + verify(clusterState, times(1)).clusterRecovered(); + verify(nativeRolesStore, times(1)).isEnabled(); + verify(clusterState, times(4)).nodes(); + verify(featureService, times(1)).clusterHasFeature(eq(clusterState), eq(QUERYABLE_BUILT_IN_ROLES_FEATURE)); + verifyNoMoreInteractions(nativeRolesStore, featureService, taskQueue, reservedRolesProvider, threadPool, clusterService); + } + + public void testSecurityIndexDeleted() { + assertInitialState(); + + ClusterState previousClusterState = spy( + markShardsAvailable(createClusterStateWithOpenSecurityIndex()).nodes(localNodeMaster()).blocks(emptyClusterBlocks()).build() + ); + ClusterState currentClusterState = spy( + ClusterState.builder(CLUSTER_NAME) + .nodes(localNodeMaster()) + .blocks(emptyClusterBlocks()) + .metadata(Metadata.builder().generateClusterUuidIfNeeded()) + .build() + ); + when(clusterService.state()).thenReturn(currentClusterState); + + synchronizer.clusterChanged(event(currentClusterState, previousClusterState)); + + verify(previousClusterState, times(1)).metadata(); + verify(currentClusterState, times(1)).metadata(); + verifyNoMoreInteractions(nativeRolesStore, featureService, taskQueue, reservedRolesProvider, threadPool, clusterService); + } + + public void testSecurityIndexCreated() { + assertInitialState(); + + ClusterState currentClusterState = spy( + markShardsAvailable(createClusterStateWithOpenSecurityIndex()).nodes(localNodeMaster()).blocks(emptyClusterBlocks()).build() + ); + + ClusterState previousClusterState = spy( + ClusterState.builder(CLUSTER_NAME) + .nodes(localNodeMaster()) + .blocks(emptyClusterBlocks()) + .metadata(Metadata.builder().generateClusterUuidIfNeeded()) + .build() + ); + + when(clusterService.state()).thenReturn(currentClusterState); + when(featureService.clusterHasFeature(any(), eq(QUERYABLE_BUILT_IN_ROLES_FEATURE))).thenReturn(true); + + final QueryableBuiltInRoles builtInRoles = buildQueryableBuiltInRoles(Set.of(ReservedRolesStore.SUPERUSER_ROLE_DESCRIPTOR)); + when(reservedRolesProvider.getRoles()).thenReturn(builtInRoles); + mockEnabledNativeStore(builtInRoles.roleDescriptors(), Set.of()); + + synchronizer.clusterChanged(event(currentClusterState, previousClusterState)); + + verify(taskQueue, times(1)).submitTask(any(), argThat(task -> task.getNewRoleDigests().equals(builtInRoles.rolesDigest())), any()); + verify(nativeRolesStore, times(1)).isEnabled(); + verify(featureService, times(1)).clusterHasFeature(any(), eq(QUERYABLE_BUILT_IN_ROLES_FEATURE)); + verify(reservedRolesProvider, times(1)).getRoles(); + verify(nativeRolesStore, times(1)).putRoles( + eq(WriteRequest.RefreshPolicy.IMMEDIATE), + eq(builtInRoles.roleDescriptors()), + eq(false), + any() + ); + verify(clusterService, times(2)).state(); + verifyNoMoreInteractions(nativeRolesStore, featureService, taskQueue, reservedRolesProvider, threadPool, clusterService); + } + + public void testSecurityIndexClosed() { + assertInitialState(); + + ClusterState clusterState = spy( + markShardsAvailable(createClusterStateWithClosedSecurityIndex()).nodes(localNodeMaster()).blocks(emptyClusterBlocks()).build() + ); + + when(clusterService.state()).thenReturn(clusterState); + when(featureService.clusterHasFeature(any(), eq(QUERYABLE_BUILT_IN_ROLES_FEATURE))).thenReturn(false); + + final QueryableBuiltInRoles builtInRoles = buildQueryableBuiltInRoles(Set.of(ReservedRolesStore.SUPERUSER_ROLE_DESCRIPTOR)); + when(reservedRolesProvider.getRoles()).thenReturn(builtInRoles); + mockEnabledNativeStore(builtInRoles.roleDescriptors(), Set.of()); + + synchronizer.clusterChanged(event(clusterState)); + + verify(clusterState, times(1)).clusterRecovered(); + verify(nativeRolesStore, times(1)).isEnabled(); + verify(clusterState, times(4)).nodes(); + verify(featureService, times(1)).clusterHasFeature(eq(clusterState), eq(QUERYABLE_BUILT_IN_ROLES_FEATURE)); + verifyNoMoreInteractions(nativeRolesStore, featureService, taskQueue, reservedRolesProvider, threadPool, clusterService); + } + + private static ClusterState.Builder markShardsAvailable(ClusterState.Builder clusterStateBuilder) { + final ClusterState cs = clusterStateBuilder.build(); + return ClusterState.builder(cs) + .routingTable( + SecurityTestUtils.buildIndexRoutingTable( + cs.metadata().index(TestRestrictedIndices.INTERNAL_SECURITY_MAIN_INDEX_7).getIndex() + ) + ); + } + + @SuppressWarnings("unchecked") + private static MasterServiceTaskQueue mockTaskQueue(ClusterService clusterService) { + final MasterServiceTaskQueue masterServiceTaskQueue = mock(MasterServiceTaskQueue.class); + when(clusterService.createTaskQueue(eq("mark-built-in-roles-as-synced-task-queue"), eq(Priority.LOW), any())) + .thenReturn(masterServiceTaskQueue); + return masterServiceTaskQueue; + } + + private ClusterChangedEvent event(ClusterState clusterState) { + return new ClusterChangedEvent("test-event", clusterState, EMPTY_CLUSTER_STATE); + } + + private ClusterChangedEvent event(ClusterState currentClusterState, ClusterState previousClusterState) { + return new ClusterChangedEvent("test-event", currentClusterState, previousClusterState); + } + + private static DiscoveryNodes localNodeMaster() { + return nodes(randomIntBetween(1, 3), true).build(); + } + + private static DiscoveryNodes localNodeNotMaster() { + return nodes(randomIntBetween(1, 3), false).build(); + } + + private static DiscoveryNodes noDataNodes() { + return nodes(0, true).build(); + } + + private DiscoveryNodes mixedVersionNodes() { + VersionInformation oldVersion = new VersionInformation( + VersionUtils.randomCompatibleVersion(random(), VersionUtils.getPreviousVersion()), + IndexVersions.MINIMUM_COMPATIBLE, + IndexVersionUtils.randomCompatibleVersion(random()) + ); + return nodes(randomIntBetween(1, 3), true).add( + DiscoveryNodeUtils.builder("old-data-node") + .name("old-data-node") + .roles(Set.of(DiscoveryNodeRole.DATA_ROLE)) + .version(oldVersion) + .build() + ).build(); + } + + private static ClusterBlocks emptyClusterBlocks() { + return ClusterBlocks.EMPTY_CLUSTER_BLOCK; + } + + private static ClusterBlocks stateNotRecoveredClusterBlocks() { + return ClusterBlocks.builder().addGlobalBlock(STATE_NOT_RECOVERED_BLOCK).build(); + } + + private static DiscoveryNodes.Builder nodes(int dataNodes, boolean localNodeMaster) { + final DiscoveryNodes.Builder nodesBuilder = DiscoveryNodes.builder(); + + int totalNodes = 0; + List dataNodeIds = new ArrayList<>(); + for (int i = 0; i < dataNodes; i++) { + String dataNodeId = "data-id-" + totalNodes++; + dataNodeIds.add(dataNodeId); + nodesBuilder.add( + DiscoveryNodeUtils.builder(dataNodeId).name("data-node-" + i).roles(Set.of(DiscoveryNodeRole.DATA_ROLE)).build() + ); + } + + String masterNodeId = "master-id-" + totalNodes++; + nodesBuilder.add(DiscoveryNodeUtils.builder(masterNodeId).name("master-node").roles(Set.of(DiscoveryNodeRole.MASTER_ROLE)).build()); + + final String localNodeId; + if (localNodeMaster) { + localNodeId = masterNodeId; + } else { + localNodeId = randomFrom(dataNodeIds); + } + return nodesBuilder.localNodeId(localNodeId).masterNodeId(masterNodeId); + } + + private void mockDisabledNativeStore() { + when(nativeRolesStore.isEnabled()).thenReturn(false); + } + + @SuppressWarnings({ "unchecked", "rawtypes" }) + private void mockEnabledNativeStore(final Collection rolesToUpsert, final Collection rolesToDelete) { + when(nativeRolesStore.isEnabled()).thenReturn(true); + doAnswer(i -> { + ((ActionListener) i.getArgument(3)).onResponse( + new BulkRolesResponse( + rolesToUpsert.stream() + .map(role -> BulkRolesResponse.Item.success(role.getName(), DocWriteResponse.Result.CREATED)) + .toList() + ) + ); + return null; + }).when(nativeRolesStore) + .putRoles(eq(WriteRequest.RefreshPolicy.IMMEDIATE), eq(rolesToUpsert), eq(false), any(ActionListener.class)); + + doAnswer(i -> { + ((ActionListener) i.getArgument(3)).onResponse( + new BulkRolesResponse( + rolesToDelete.stream().map(role -> BulkRolesResponse.Item.success(role, DocWriteResponse.Result.DELETED)).toList() + ) + ); + return null; + }).when(nativeRolesStore) + .deleteRoles(eq(rolesToDelete), eq(WriteRequest.RefreshPolicy.IMMEDIATE), eq(false), any(ActionListener.class)); + } + + private static ClusterState.Builder createClusterStateWithOpenSecurityIndex() { + return SecurityIndexManagerTests.createClusterState( + TestRestrictedIndices.INTERNAL_SECURITY_MAIN_INDEX_7, + SecuritySystemIndices.SECURITY_MAIN_ALIAS, + IndexMetadata.State.OPEN + ); + } + + private static ClusterState.Builder createClusterStateWithClosedSecurityIndex() { + return SecurityIndexManagerTests.createClusterState( + TestRestrictedIndices.INTERNAL_SECURITY_MAIN_INDEX_7, + SecuritySystemIndices.SECURITY_MAIN_ALIAS, + IndexMetadata.State.CLOSE + ); + } +} diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/support/QueryableBuiltInRolesUtilsTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/support/QueryableBuiltInRolesUtilsTests.java index 5b4787f25ae7f..933512d3426c4 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/support/QueryableBuiltInRolesUtilsTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/support/QueryableBuiltInRolesUtilsTests.java @@ -281,12 +281,12 @@ private static Map randomlyOrderedSupermanMetadata() { return metadata; } - private static QueryableBuiltInRoles buildQueryableBuiltInRoles(Set roles) { + public static QueryableBuiltInRoles buildQueryableBuiltInRoles(Set roles) { final Map digests = buildDigests(roles); return new QueryableBuiltInRoles(digests, roles); } - private static Map buildDigests(Set roles) { + public static Map buildDigests(Set roles) { final Map digests = new HashMap<>(); for (RoleDescriptor role : roles) { digests.put(role.getName(), QueryableBuiltInRolesUtils.calculateHash(role)); From 8b5156da6600784bb47c404cf0e3d078282fa275 Mon Sep 17 00:00:00 2001 From: Slobodan Adamovic Date: Thu, 19 Dec 2024 00:25:52 +0100 Subject: [PATCH 2/4] fix issue where synchronizationInProgress would stay true without being reset --- muted-tests.yml | 3 --- .../QueryableBuiltInRolesSynchronizer.java | 22 +++++++++---------- 2 files changed, 11 insertions(+), 14 deletions(-) diff --git a/muted-tests.yml b/muted-tests.yml index 35a9b31685794..09ca71687d78b 100644 --- a/muted-tests.yml +++ b/muted-tests.yml @@ -276,9 +276,6 @@ tests: - class: org.elasticsearch.xpack.ccr.rest.ShardChangesRestIT method: testShardChangesNoOperation issue: https://github.com/elastic/elasticsearch/issues/118800 -- class: org.elasticsearch.xpack.security.QueryableReservedRolesIT - method: testDeletingAndCreatingSecurityIndexTriggersSynchronization - issue: https://github.com/elastic/elasticsearch/issues/118806 - class: org.elasticsearch.index.engine.RecoverySourcePruneMergePolicyTests method: testPruneSome issue: https://github.com/elastic/elasticsearch/issues/118728 diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/support/QueryableBuiltInRolesSynchronizer.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/support/QueryableBuiltInRolesSynchronizer.java index 479cbaf4b852b..bfc5b564e2e50 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/support/QueryableBuiltInRolesSynchronizer.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/support/QueryableBuiltInRolesSynchronizer.java @@ -201,18 +201,18 @@ public void clusterChanged(ClusterChangedEvent event) { private void syncBuiltInRoles(final QueryableBuiltInRoles roles) { if (synchronizationInProgress.compareAndSet(false, true)) { - final Map indexedRolesDigests = readIndexedBuiltInRolesDigests(clusterService.state()); - if (roles.rolesDigest().equals(indexedRolesDigests)) { - logger.debug("Security index already contains the latest built-in roles indexed, skipping synchronization"); - return; - } - executor.execute(() -> doSyncBuiltinRoles(indexedRolesDigests, roles, ActionListener.wrap(v -> { - logger.info("Successfully synced [" + roles.roleDescriptors().size() + "] built-in roles to .security index"); - synchronizationInProgress.set(false); - }, e -> { - handleException(e); + try { + final Map indexedRolesDigests = readIndexedBuiltInRolesDigests(clusterService.state()); + if (roles.rolesDigest().equals(indexedRolesDigests)) { + logger.debug("Security index already contains the latest built-in roles indexed, skipping roles synchronization"); + } else { + executor.execute(() -> doSyncBuiltinRoles(indexedRolesDigests, roles, ActionListener.wrap(v -> { + logger.info("Successfully synced [" + roles.roleDescriptors().size() + "] built-in roles to .security index"); + }, QueryableBuiltInRolesSynchronizer::handleException))); + } + } finally { synchronizationInProgress.set(false); - }))); + } } } From 8b46332cbe36f3c561f2798955864fe3385f53f6 Mon Sep 17 00:00:00 2001 From: Slobodan Adamovic Date: Thu, 19 Dec 2024 14:13:11 +0100 Subject: [PATCH 3/4] properly fix the release of synchronizationInProgress --- .../QueryableBuiltInRolesSynchronizer.java | 21 ++++++++++--------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/support/QueryableBuiltInRolesSynchronizer.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/support/QueryableBuiltInRolesSynchronizer.java index bfc5b564e2e50..befba8f346320 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/support/QueryableBuiltInRolesSynchronizer.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/support/QueryableBuiltInRolesSynchronizer.java @@ -201,17 +201,18 @@ public void clusterChanged(ClusterChangedEvent event) { private void syncBuiltInRoles(final QueryableBuiltInRoles roles) { if (synchronizationInProgress.compareAndSet(false, true)) { - try { - final Map indexedRolesDigests = readIndexedBuiltInRolesDigests(clusterService.state()); - if (roles.rolesDigest().equals(indexedRolesDigests)) { - logger.debug("Security index already contains the latest built-in roles indexed, skipping roles synchronization"); - } else { - executor.execute(() -> doSyncBuiltinRoles(indexedRolesDigests, roles, ActionListener.wrap(v -> { - logger.info("Successfully synced [" + roles.roleDescriptors().size() + "] built-in roles to .security index"); - }, QueryableBuiltInRolesSynchronizer::handleException))); - } - } finally { + final Map indexedRolesDigests = readIndexedBuiltInRolesDigests(clusterService.state()); + if (roles.rolesDigest().equals(indexedRolesDigests)) { + logger.debug("Security index already contains the latest built-in roles indexed, skipping roles synchronization"); synchronizationInProgress.set(false); + } else { + executor.execute(() -> doSyncBuiltinRoles(indexedRolesDigests, roles, ActionListener.wrap(v -> { + logger.info("Successfully synced [" + roles.roleDescriptors().size() + "] built-in roles to .security index"); + synchronizationInProgress.set(false); + }, e -> { + synchronizationInProgress.set(false); + handleException(e); + }))); } } } From a5035f01379cbbb9faf2da122beca4b787deae77 Mon Sep 17 00:00:00 2001 From: Slobodan Adamovic Date: Thu, 19 Dec 2024 23:28:28 +0100 Subject: [PATCH 4/4] address review feedback --- .../QueryableBuiltInRolesSynchronizer.java | 38 +++++++++++-------- ...ueryableBuiltInRolesSynchronizerTests.java | 21 ++++++++-- 2 files changed, 40 insertions(+), 19 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/support/QueryableBuiltInRolesSynchronizer.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/support/QueryableBuiltInRolesSynchronizer.java index befba8f346320..2173e74fb8b93 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/support/QueryableBuiltInRolesSynchronizer.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/support/QueryableBuiltInRolesSynchronizer.java @@ -199,20 +199,32 @@ public void clusterChanged(ClusterChangedEvent event) { } } + /** + * @return {@code true} if the synchronization of built-in roles is in progress, {@code false} otherwise + */ + public boolean isSynchronizationInProgress() { + return synchronizationInProgress.get(); + } + private void syncBuiltInRoles(final QueryableBuiltInRoles roles) { if (synchronizationInProgress.compareAndSet(false, true)) { - final Map indexedRolesDigests = readIndexedBuiltInRolesDigests(clusterService.state()); - if (roles.rolesDigest().equals(indexedRolesDigests)) { - logger.debug("Security index already contains the latest built-in roles indexed, skipping roles synchronization"); - synchronizationInProgress.set(false); - } else { - executor.execute(() -> doSyncBuiltinRoles(indexedRolesDigests, roles, ActionListener.wrap(v -> { - logger.info("Successfully synced [" + roles.roleDescriptors().size() + "] built-in roles to .security index"); + try { + final Map indexedRolesDigests = readIndexedBuiltInRolesDigests(clusterService.state()); + if (roles.rolesDigest().equals(indexedRolesDigests)) { + logger.debug("Security index already contains the latest built-in roles indexed, skipping roles synchronization"); synchronizationInProgress.set(false); - }, e -> { - synchronizationInProgress.set(false); - handleException(e); - }))); + } else { + executor.execute(() -> doSyncBuiltinRoles(indexedRolesDigests, roles, ActionListener.wrap(v -> { + logger.info("Successfully synced [" + roles.roleDescriptors().size() + "] built-in roles to .security index"); + synchronizationInProgress.set(false); + }, e -> { + handleException(e); + synchronizationInProgress.set(false); + }))); + } + } catch (Exception e) { + logger.error("Failed to sync built-in roles", e); + synchronizationInProgress.set(false); } } } @@ -453,10 +465,6 @@ static class MarkRolesAsSyncedTask implements ClusterStateTaskListener { this.newRoleDigests = newRoleDigests; } - public Map getExpectedRoleDigests() { - return expectedRoleDigests; - } - public Map getNewRoleDigests() { return newRoleDigests; } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/support/QueryableBuiltInRolesSynchronizerTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/support/QueryableBuiltInRolesSynchronizerTests.java index 8b7c580b82ffa..e3c6c0cdd4e5b 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/support/QueryableBuiltInRolesSynchronizerTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/support/QueryableBuiltInRolesSynchronizerTests.java @@ -51,6 +51,7 @@ import static org.elasticsearch.gateway.GatewayService.STATE_NOT_RECOVERED_BLOCK; import static org.elasticsearch.xpack.security.support.QueryableBuiltInRolesSynchronizer.QUERYABLE_BUILT_IN_ROLES_FEATURE; import static org.elasticsearch.xpack.security.support.QueryableBuiltInRolesUtilsTests.buildQueryableBuiltInRoles; +import static org.hamcrest.Matchers.equalTo; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.argThat; import static org.mockito.ArgumentMatchers.eq; @@ -145,6 +146,7 @@ public void testSuccessfulSync() { ); when(reservedRolesProvider.getRoles()).thenReturn(builtInRoles); mockEnabledNativeStore(builtInRoles.roleDescriptors(), Set.of()); + assertThat(synchronizer.isSynchronizationInProgress(), equalTo(false)); synchronizer.clusterChanged(event(clusterState)); @@ -159,8 +161,9 @@ public void testSuccessfulSync() { eq(false), any() ); - verify(clusterService, times(2)).state(); + verify(clusterService, times(3)).state(); verifyNoMoreInteractions(nativeRolesStore, featureService, taskQueue, reservedRolesProvider, threadPool, clusterService); + assertThat(synchronizer.isSynchronizationInProgress(), equalTo(false)); } public void testNotMaster() { @@ -339,6 +342,7 @@ public void testSecurityIndexCreated() { final QueryableBuiltInRoles builtInRoles = buildQueryableBuiltInRoles(Set.of(ReservedRolesStore.SUPERUSER_ROLE_DESCRIPTOR)); when(reservedRolesProvider.getRoles()).thenReturn(builtInRoles); mockEnabledNativeStore(builtInRoles.roleDescriptors(), Set.of()); + assertThat(synchronizer.isSynchronizationInProgress(), equalTo(false)); synchronizer.clusterChanged(event(currentClusterState, previousClusterState)); @@ -352,8 +356,9 @@ public void testSecurityIndexCreated() { eq(false), any() ); - verify(clusterService, times(2)).state(); + verify(clusterService, times(3)).state(); verifyNoMoreInteractions(nativeRolesStore, featureService, taskQueue, reservedRolesProvider, threadPool, clusterService); + assertThat(synchronizer.isSynchronizationInProgress(), equalTo(false)); } public void testSecurityIndexClosed() { @@ -390,10 +395,17 @@ private static ClusterState.Builder markShardsAvailable(ClusterState.Builder clu } @SuppressWarnings("unchecked") - private static MasterServiceTaskQueue mockTaskQueue(ClusterService clusterService) { + private MasterServiceTaskQueue mockTaskQueue(ClusterService clusterService) { final MasterServiceTaskQueue masterServiceTaskQueue = mock(MasterServiceTaskQueue.class); when(clusterService.createTaskQueue(eq("mark-built-in-roles-as-synced-task-queue"), eq(Priority.LOW), any())) .thenReturn(masterServiceTaskQueue); + doAnswer(i -> { + assertThat(synchronizer.isSynchronizationInProgress(), equalTo(true)); + MarkRolesAsSyncedTask task = i.getArgument(1); + var result = task.execute(clusterService.state()); + task.success(result.v2()); + return null; + }).when(masterServiceTaskQueue).submitTask(any(), any(), any()); return masterServiceTaskQueue; } @@ -419,7 +431,7 @@ private static DiscoveryNodes noDataNodes() { private DiscoveryNodes mixedVersionNodes() { VersionInformation oldVersion = new VersionInformation( - VersionUtils.randomCompatibleVersion(random(), VersionUtils.getPreviousVersion()), + VersionUtils.randomVersionBetween(random(), null, VersionUtils.getPreviousVersion()), IndexVersions.MINIMUM_COMPATIBLE, IndexVersionUtils.randomCompatibleVersion(random()) ); @@ -473,6 +485,7 @@ private void mockDisabledNativeStore() { private void mockEnabledNativeStore(final Collection rolesToUpsert, final Collection rolesToDelete) { when(nativeRolesStore.isEnabled()).thenReturn(true); doAnswer(i -> { + assertThat(synchronizer.isSynchronizationInProgress(), equalTo(true)); ((ActionListener) i.getArgument(3)).onResponse( new BulkRolesResponse( rolesToUpsert.stream()