elastic · elasticsearchmachine · Oct 7, 2024 · Oct 7, 2024
diff --git a/docs/changelog/113900.yaml b/docs/changelog/113900.yaml
@@ -0,0 +1,5 @@
+pr: 113900
+summary: Fix BWC for file-settings based role mappings
+area: Authentication
+type: bug
+issues: []
diff --git a/...ava/org/elasticsearch/xpack/core/security/action/rolemapping/GetRoleMappingsResponse.java b/...ava/org/elasticsearch/xpack/core/security/action/rolemapping/GetRoleMappingsResponse.java
@@ -11,6 +11,7 @@
 import org.elasticsearch.xpack.core.security.authc.support.mapper.ExpressionRoleMapping;
 
 import java.io.IOException;
+import java.util.Collection;
 
 /**
  * Response to {@link GetRoleMappingsAction get role-mappings API}.
@@ -21,6 +22,10 @@ public class GetRoleMappingsResponse extends ActionResponse {
 
     private final ExpressionRoleMapping[] mappings;
 
+    public GetRoleMappingsResponse(Collection<ExpressionRoleMapping> mappings) {
+        this(mappings.toArray(new ExpressionRoleMapping[0]));
+    }
+
     public GetRoleMappingsResponse(ExpressionRoleMapping... mappings) {
         this.mappings = mappings;
     }

diff --git a/...n/core/src/main/java/org/elasticsearch/xpack/core/security/authz/RoleMappingMetadata.java b/...n/core/src/main/java/org/elasticsearch/xpack/core/security/authz/RoleMappingMetadata.java
@@ -12,6 +12,7 @@
 import org.elasticsearch.cluster.AbstractNamedDiffable;
 import org.elasticsearch.cluster.ClusterState;
 import org.elasticsearch.cluster.NamedDiff;
+import org.elasticsearch.cluster.block.ClusterBlockLevel;
 import org.elasticsearch.cluster.metadata.Metadata;
 import org.elasticsearch.common.collect.Iterators;
 import org.elasticsearch.common.io.stream.StreamInput;
@@ -57,6 +58,7 @@ public final class RoleMappingMetadata extends AbstractNamedDiffable<Metadata.Cu
     private static final RoleMappingMetadata EMPTY = new RoleMappingMetadata(Set.of());
 
     public static RoleMappingMetadata getFromClusterState(ClusterState clusterState) {
+        clusterState.blocks().globalBlockedRaiseException(ClusterBlockLevel.READ);
         return clusterState.metadata().custom(RoleMappingMetadata.TYPE, RoleMappingMetadata.EMPTY);
     }
 

diff --git a/...src/internalClusterTest/java/org/elasticsearch/integration/RoleMappingFileSettingsIT.java b/...src/internalClusterTest/java/org/elasticsearch/integration/RoleMappingFileSettingsIT.java
@@ -34,6 +34,7 @@
 import org.elasticsearch.xpack.core.security.action.rolemapping.PutRoleMappingAction;
 import org.elasticsearch.xpack.core.security.action.rolemapping.PutRoleMappingRequest;
 import org.elasticsearch.xpack.core.security.action.rolemapping.PutRoleMappingRequestBuilder;
+import org.elasticsearch.xpack.core.security.action.rolemapping.PutRoleMappingResponse;
 import org.elasticsearch.xpack.core.security.authc.RealmConfig;
 import org.elasticsearch.xpack.core.security.authc.support.UserRoleMapper;
 import org.elasticsearch.xpack.core.security.authc.support.mapper.ExpressionRoleMapping;
@@ -58,12 +59,11 @@
 import static org.elasticsearch.indices.recovery.RecoverySettings.INDICES_RECOVERY_MAX_BYTES_PER_SEC_SETTING;
 import static org.elasticsearch.xcontent.XContentType.JSON;
 import static org.elasticsearch.xpack.core.security.test.TestRestrictedIndices.INTERNAL_SECURITY_MAIN_INDEX_7;
+import static org.elasticsearch.xpack.security.authc.support.mapper.ClusterStateRoleMapper.RESERVED_ROLE_MAPPING_SUFFIX;
 import static org.hamcrest.Matchers.allOf;
-import static org.hamcrest.Matchers.contains;
 import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.empty;
-import static org.hamcrest.Matchers.emptyArray;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.hasSize;
 import static org.hamcrest.Matchers.notNullValue;
@@ -270,21 +270,28 @@ private void assertRoleMappingsSaveOK(CountDownLatch savedClusterState, AtomicLo
             assertThat(resolveRolesFuture.get(), containsInAnyOrder("kibana_user", "fleet_user"));
         }
 
-        // the role mappings are not retrievable by the role mapping action (which only accesses "native" i.e. index-based role mappings)
-        var request = new GetRoleMappingsRequest();
-        request.setNames("everyone_kibana", "everyone_fleet");
-        var response = client().execute(GetRoleMappingsAction.INSTANCE, request).get();
-        assertFalse(response.hasMappings());
-        assertThat(response.mappings(), emptyArray());
-
-        // role mappings (with the same names) can also be stored in the "native" store
-        var putRoleMappingResponse = client().execute(PutRoleMappingAction.INSTANCE, sampleRestRequest("everyone_kibana")).actionGet();
-        assertTrue(putRoleMappingResponse.isCreated());
-        putRoleMappingResponse = client().execute(PutRoleMappingAction.INSTANCE, sampleRestRequest("everyone_fleet")).actionGet();
-        assertTrue(putRoleMappingResponse.isCreated());
+        // the role mappings are retrievable by the role mapping action for BWC
+        assertGetResponseHasMappings(true, "everyone_kibana", "everyone_fleet");
+
+        // role mappings (with the same names) can be stored in the "native" store
+        {
+            PutRoleMappingResponse response = client().execute(PutRoleMappingAction.INSTANCE, sampleRestRequest("everyone_kibana"))
+                .actionGet();
+            assertTrue(response.isCreated());
+            response = client().execute(PutRoleMappingAction.INSTANCE, sampleRestRequest("everyone_fleet")).actionGet();
+            assertTrue(response.isCreated());
+        }
+        {
+            // deleting role mappings that exist in the native store and in cluster-state should result in success
+            var response = client().execute(DeleteRoleMappingAction.INSTANCE, deleteRequest("everyone_kibana")).actionGet();
+            assertTrue(response.isFound());
+            response = client().execute(DeleteRoleMappingAction.INSTANCE, deleteRequest("everyone_fleet")).actionGet();
+            assertTrue(response.isFound());
+        }
+
     }
 
-    public void testRoleMappingsApplied() throws Exception {
+    public void testClusterStateRoleMappingsAddedThenDeleted() throws Exception {
         ensureGreen();
 
         var savedClusterState = setupClusterStateListener(internalCluster().getMasterName(), "everyone_kibana");
@@ -293,6 +300,12 @@ public void testRoleMappingsApplied() throws Exception {
         assertRoleMappingsSaveOK(savedClusterState.v1(), savedClusterState.v2());
         logger.info("---> cleanup cluster settings...");
 
+        {
+            // Deleting non-existent native role mappings returns not found even if they exist in config file
+            var response = client().execute(DeleteRoleMappingAction.INSTANCE, deleteRequest("everyone_kibana")).get();
+            assertFalse(response.isFound());
+        }
+
         savedClusterState = setupClusterStateListenerForCleanup(internalCluster().getMasterName());
 
         writeJSONFile(internalCluster().getMasterName(), emptyJSON, logger, versionCounter);
@@ -307,48 +320,85 @@ public void testRoleMappingsApplied() throws Exception {
             clusterStateResponse.getState().metadata().persistentSettings().get(INDICES_RECOVERY_MAX_BYTES_PER_SEC_SETTING.getKey())
         );
 
-        // native role mappings are not affected by the removal of the cluster-state based ones
+        // cluster-state role mapping was removed and is not returned in the API anymore
         {
             var request = new GetRoleMappingsRequest();
             request.setNames("everyone_kibana", "everyone_fleet");
             var response = client().execute(GetRoleMappingsAction.INSTANCE, request).get();
-            assertTrue(response.hasMappings());
-            assertThat(
-                Arrays.stream(response.mappings()).map(ExpressionRoleMapping::getName).toList(),
-                containsInAnyOrder("everyone_kibana", "everyone_fleet")
-            );
+            assertFalse(response.hasMappings());
         }
 
-        // and roles are resolved based on the native role mappings
+        // no role mappings means no roles are resolved
         for (UserRoleMapper userRoleMapper : internalCluster().getInstances(UserRoleMapper.class)) {
             PlainActionFuture<Set<String>> resolveRolesFuture = new PlainActionFuture<>();
             userRoleMapper.resolveRoles(
                 new UserRoleMapper.UserData("anyUsername", null, List.of(), Map.of(), mock(RealmConfig.class)),
                 resolveRolesFuture
             );
-            assertThat(resolveRolesFuture.get(), contains("kibana_user_native"));
+            assertThat(resolveRolesFuture.get(), empty());
         }
+    }
 
-        {
-            var request = new DeleteRoleMappingRequest();
-            request.setName("everyone_kibana");
-            var response = client().execute(DeleteRoleMappingAction.INSTANCE, request).get();
-            assertTrue(response.isFound());
-            request = new DeleteRoleMappingRequest();
-            request.setName("everyone_fleet");
-            response = client().execute(DeleteRoleMappingAction.INSTANCE, request).get();
-            assertTrue(response.isFound());
+    public void testGetRoleMappings() throws Exception {
+        ensureGreen();
+
+        final List<String> nativeMappings = List.of("everyone_kibana", "_everyone_kibana", "zzz_mapping", "123_mapping");
+        for (var mapping : nativeMappings) {
+            client().execute(PutRoleMappingAction.INSTANCE, sampleRestRequest(mapping)).actionGet();
         }
 
-        // no roles are resolved now, because both native and cluster-state based stores have been cleared
-        for (UserRoleMapper userRoleMapper : internalCluster().getInstances(UserRoleMapper.class)) {
-            PlainActionFuture<Set<String>> resolveRolesFuture = new PlainActionFuture<>();
-            userRoleMapper.resolveRoles(
-                new UserRoleMapper.UserData("anyUsername", null, List.of(), Map.of(), mock(RealmConfig.class)),
-                resolveRolesFuture
-            );
-            assertThat(resolveRolesFuture.get(), empty());
+        var savedClusterState = setupClusterStateListener(internalCluster().getMasterName(), "everyone_kibana");
+        writeJSONFile(internalCluster().getMasterName(), testJSON, logger, versionCounter);
+        boolean awaitSuccessful = savedClusterState.v1().await(20, TimeUnit.SECONDS);
+        assertTrue(awaitSuccessful);
+
+        var request = new GetRoleMappingsRequest();
+        var response = client().execute(GetRoleMappingsAction.INSTANCE, request).get();
+        assertTrue(response.hasMappings());
+        assertThat(
+            Arrays.stream(response.mappings()).map(ExpressionRoleMapping::getName).toList(),
+            containsInAnyOrder(
+                "everyone_kibana",
+                "everyone_kibana " + RESERVED_ROLE_MAPPING_SUFFIX,
+                "_everyone_kibana",
+                "everyone_fleet " + RESERVED_ROLE_MAPPING_SUFFIX,
+                "zzz_mapping",
+                "123_mapping"
+            )
+        );
+
+        int readOnlyCount = 0;
+        // assert that cluster-state role mappings come last
+        for (ExpressionRoleMapping mapping : response.mappings()) {
+            readOnlyCount = mapping.getName().endsWith(RESERVED_ROLE_MAPPING_SUFFIX) ? readOnlyCount + 1 : readOnlyCount;
         }
+        // Two sourced from cluster-state
+        assertEquals(readOnlyCount, 2);
+
+        // it's possible to delete overlapping native role mapping
+        assertTrue(client().execute(DeleteRoleMappingAction.INSTANCE, deleteRequest("everyone_kibana")).actionGet().isFound());
+
+        savedClusterState = setupClusterStateListenerForCleanup(internalCluster().getMasterName());
+        writeJSONFile(internalCluster().getMasterName(), emptyJSON, logger, versionCounter);
+        awaitSuccessful = savedClusterState.v1().await(20, TimeUnit.SECONDS);
+        assertTrue(awaitSuccessful);
+
+        final ClusterStateResponse clusterStateResponse = clusterAdmin().state(
+            new ClusterStateRequest(TEST_REQUEST_TIMEOUT).waitForMetadataVersion(savedClusterState.v2().get())
+        ).get();
+
+        assertNull(
+            clusterStateResponse.getState().metadata().persistentSettings().get(INDICES_RECOVERY_MAX_BYTES_PER_SEC_SETTING.getKey())
+        );
+
+        // Make sure remaining native mappings can still be fetched
+        request = new GetRoleMappingsRequest();
+        response = client().execute(GetRoleMappingsAction.INSTANCE, request).get();
+        assertTrue(response.hasMappings());
+        assertThat(
+            Arrays.stream(response.mappings()).map(ExpressionRoleMapping::getName).toList(),
+            containsInAnyOrder("_everyone_kibana", "zzz_mapping", "123_mapping")
+        );
     }
 
     public static Tuple<CountDownLatch, AtomicLong> setupClusterStateListenerForError(
@@ -433,11 +483,8 @@ public void testRoleMappingApplyWithSecurityIndexClosed() throws Exception {
             boolean awaitSuccessful = savedClusterState.v1().await(20, TimeUnit.SECONDS);
             assertTrue(awaitSuccessful);
 
-            // no native role mappings exist
-            var request = new GetRoleMappingsRequest();
-            request.setNames("everyone_kibana", "everyone_fleet");
-            var response = client().execute(GetRoleMappingsAction.INSTANCE, request).get();
-            assertFalse(response.hasMappings());
+            // even if index is closed, cluster-state role mappings are still returned
+            assertGetResponseHasMappings(true, "everyone_kibana", "everyone_fleet");
 
             // cluster state settings are also applied
             var clusterStateResponse = clusterAdmin().state(
@@ -476,6 +523,12 @@ public void testRoleMappingApplyWithSecurityIndexClosed() throws Exception {
         }
     }
 
+    private DeleteRoleMappingRequest deleteRequest(String name) {
+        var request = new DeleteRoleMappingRequest();
+        request.setName(name);
+        return request;
+    }
+
     private PutRoleMappingRequest sampleRestRequest(String name) throws Exception {
         var json = """
             {
@@ -494,4 +547,19 @@ private PutRoleMappingRequest sampleRestRequest(String name) throws Exception {
             return new PutRoleMappingRequestBuilder(null).source(name, parser).request();
         }
     }
+
+    private static void assertGetResponseHasMappings(boolean readOnly, String... mappings) throws InterruptedException, ExecutionException {
+        var request = new GetRoleMappingsRequest();
+        request.setNames(mappings);
+        var response = client().execute(GetRoleMappingsAction.INSTANCE, request).get();
+        assertTrue(response.hasMappings());
+        assertThat(
+            Arrays.stream(response.mappings()).map(ExpressionRoleMapping::getName).toList(),
+            containsInAnyOrder(
+                Arrays.stream(mappings)
+                    .map(mapping -> mapping + (readOnly ? " " + RESERVED_ROLE_MAPPING_SUFFIX : ""))
+                    .toArray(String[]::new)
+            )
+        );
+    }
 }
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
@@ -894,7 +894,8 @@ Collection<Object> createComponents(
             reservedRealm
         );
         components.add(nativeUsersStore);
-        components.add(new PluginComponentBinding<>(NativeRoleMappingStore.class, nativeRoleMappingStore));
+        components.add(clusterStateRoleMapper);
+        components.add(nativeRoleMappingStore);
         components.add(new PluginComponentBinding<>(UserRoleMapper.class, userRoleMapper));
         components.add(reservedRealm);
         components.add(realms);

diff --git a/...org/elasticsearch/xpack/security/action/rolemapping/TransportDeleteRoleMappingAction.java b/...org/elasticsearch/xpack/security/action/rolemapping/TransportDeleteRoleMappingAction.java
@@ -9,24 +9,28 @@
 import org.elasticsearch.action.ActionListener;
 import org.elasticsearch.action.support.ActionFilters;
 import org.elasticsearch.action.support.HandledTransportAction;
+import org.elasticsearch.common.logging.HeaderWarning;
 import org.elasticsearch.common.util.concurrent.EsExecutors;
 import org.elasticsearch.injection.guice.Inject;
 import org.elasticsearch.tasks.Task;
 import org.elasticsearch.transport.TransportService;
 import org.elasticsearch.xpack.core.security.action.rolemapping.DeleteRoleMappingAction;
 import org.elasticsearch.xpack.core.security.action.rolemapping.DeleteRoleMappingRequest;
 import org.elasticsearch.xpack.core.security.action.rolemapping.DeleteRoleMappingResponse;
+import org.elasticsearch.xpack.security.authc.support.mapper.ClusterStateRoleMapper;
 import org.elasticsearch.xpack.security.authc.support.mapper.NativeRoleMappingStore;
 
 public class TransportDeleteRoleMappingAction extends HandledTransportAction<DeleteRoleMappingRequest, DeleteRoleMappingResponse> {
 
     private final NativeRoleMappingStore roleMappingStore;
+    private final ClusterStateRoleMapper clusterStateRoleMapper;
 
     @Inject
     public TransportDeleteRoleMappingAction(
         ActionFilters actionFilters,
         TransportService transportService,
-        NativeRoleMappingStore roleMappingStore
+        NativeRoleMappingStore roleMappingStore,
+        ClusterStateRoleMapper clusterStateRoleMapper
     ) {
         super(
             DeleteRoleMappingAction.NAME,
@@ -36,10 +40,22 @@ public TransportDeleteRoleMappingAction(
             EsExecutors.DIRECT_EXECUTOR_SERVICE
         );
         this.roleMappingStore = roleMappingStore;
+        this.clusterStateRoleMapper = clusterStateRoleMapper;
     }
 
     @Override
     protected void doExecute(Task task, DeleteRoleMappingRequest request, ActionListener<DeleteRoleMappingResponse> listener) {
+        if (clusterStateRoleMapper.hasMapping(request.getName())) {
+            // Since it's allowed to add a mapping with the same name in the native role mapping store as the file_settings namespace,
+            // a warning header is added to signal to the caller that this could be a problem.
+            HeaderWarning.addWarning(
+                "A read only role mapping with the same name ["
+                    + request.getName()
+                    + "] has been previously been defined in a configuration file. The role mapping ["
+                    + request.getName()
+                    + "] defined in the configuration file is read only, will not be deleted, and will remain active."
+            );
+        }
         roleMappingStore.deleteRoleMapping(request, listener.safeMap(DeleteRoleMappingResponse::new));
     }
 }