Add Role Mapping Cleanup Security Migration

docs/changelog/114830.yaml

@@ -0,0 +1,5 @@
+pr: 114830
+summary: Add Role Mapping Cleanup Security Migration
+area: Security
+type: bug
+issues: []

server/src/main/java/org/elasticsearch/cluster/metadata/ReservedStateMetadata.java

@@ -91,6 +91,21 @@ public Set<String> conflicts(String handlerName, Set<String> modified) {
         return Collections.unmodifiableSet(intersect);
     }
 
+    /**
+     * Get the reserved keys for the handler name
+     *
+     * @param handlerName handler name to get keys for
+     * @return set of keys for that handler
+     */
+    public Set<String> keys(String handlerName) {
+        ReservedStateHandlerMetadata handlerMetadata = handlers.get(handlerName);
+        if (handlerMetadata == null || handlerMetadata.keys().isEmpty()) {
+            return Collections.emptySet();
+        }
+
+        return Collections.unmodifiableSet(handlerMetadata.keys());
+    }
+
     /**
      * Reads an {@link ReservedStateMetadata} from a {@link StreamInput}
      *

server/src/main/java/org/elasticsearch/index/IndexVersions.java

@@ -118,6 +118,7 @@ private static IndexVersion def(int id, Version luceneVersion) {
     public static final IndexVersion MERGE_ON_RECOVERY_VERSION = def(8_515_00_0, Version.LUCENE_9_11_1);
     public static final IndexVersion UPGRADE_TO_LUCENE_9_12 = def(8_516_00_0, Version.LUCENE_9_12_0);
     public static final IndexVersion ENABLE_IGNORE_ABOVE_LOGSDB = def(8_517_00_0, Version.LUCENE_9_12_0);
+    public static final IndexVersion ADD_ROLE_MAPPING_CLEANUP_MIGRATION = def(8_518_00_0, Version.LUCENE_9_12_0);
     /*
      * STOP! READ THIS FIRST! No, really,
      *        ____ _____ ___  ____  _        ____  _____    _    ____    _____ _   _ ___ ____    _____ ___ ____  ____ _____ _

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/support/mapper/ExpressionRoleMapping.java

@@ -206,15 +206,15 @@ public RoleMapperExpression getExpression() {
      * that match the {@link #getExpression() expression} in this mapping.
      */
     public List<String> getRoles() {
-        return Collections.unmodifiableList(roles);
+        return roles != null ? Collections.unmodifiableList(roles) : Collections.emptyList();
     }
 
     /**
      * The list of {@link RoleDescriptor roles} (specified by a {@link TemplateRoleName template} that evaluates to one or more names)
      * that should be assigned to users that match the {@link #getExpression() expression} in this mapping.
      */
     public List<TemplateRoleName> getRoleTemplates() {
-        return Collections.unmodifiableList(roleTemplates);
+        return roleTemplates != null ? Collections.unmodifiableList(roleTemplates) : Collections.emptyList();
     }
 
     /**
@@ -223,7 +223,7 @@ public List<TemplateRoleName> getRoleTemplates() {
      * This is not used within the mapping process, and does not affect whether the expression matches, nor which roles are assigned.
      */
     public Map<String, Object> getMetadata() {
-        return Collections.unmodifiableMap(metadata);
+        return metadata != null ? Collections.unmodifiableMap(metadata) : Collections.emptyMap();
     }
 
     /**

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/RoleMappingMetadata.java

@@ -191,6 +191,14 @@ public static boolean hasFallbackName(ExpressionRoleMapping expressionRoleMappin
         return expressionRoleMapping.getName().equals(FALLBACK_NAME);
     }
 
+    /**
+     * Check if any of the role mappings have a fallback name
+     * @return true if any role mappings have the fallback name
+     */
+    public boolean hasAnyMappingWithFallbackName() {
+        return roleMappings.stream().anyMatch(RoleMappingMetadata::hasFallbackName);
+    }
+
     /**
      * Parse a role mapping from XContent, restoring the name from a reserved metadata field.
      * Used to parse a role mapping annotated with its name in metadata via @see {@link #copyWithNameInMetadata(ExpressionRoleMapping)}.

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/SecurityFeatures.java

@@ -17,13 +17,14 @@
 import static org.elasticsearch.xpack.security.support.SecuritySystemIndices.SECURITY_MIGRATION_FRAMEWORK;
 import static org.elasticsearch.xpack.security.support.SecuritySystemIndices.SECURITY_PROFILE_ORIGIN_FEATURE;
 import static org.elasticsearch.xpack.security.support.SecuritySystemIndices.SECURITY_ROLES_METADATA_FLATTENED;
+import static org.elasticsearch.xpack.security.support.SecuritySystemIndices.SECURITY_ROLE_MAPPING_CLEANUP;
 import static org.elasticsearch.xpack.security.support.SecuritySystemIndices.VERSION_SECURITY_PROFILE_ORIGIN;
 
 public class SecurityFeatures implements FeatureSpecification {
 
     @Override
     public Set<NodeFeature> getFeatures() {
-        return Set.of(SECURITY_ROLES_METADATA_FLATTENED, SECURITY_MIGRATION_FRAMEWORK);
+        return Set.of(SECURITY_ROLE_MAPPING_CLEANUP, SECURITY_ROLES_METADATA_FLATTENED, SECURITY_MIGRATION_FRAMEWORK);
     }
 
     @Override

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/support/SecurityIndexManager.java

@@ -31,6 +31,7 @@
 import org.elasticsearch.cluster.metadata.IndexMetadata;
 import org.elasticsearch.cluster.metadata.MappingMetadata;
 import org.elasticsearch.cluster.metadata.Metadata;
+import org.elasticsearch.cluster.metadata.ReservedStateMetadata;
 import org.elasticsearch.cluster.routing.IndexRoutingTable;
 import org.elasticsearch.cluster.service.ClusterService;
 import org.elasticsearch.core.TimeValue;
@@ -46,7 +47,9 @@
 import org.elasticsearch.rest.RestStatus;
 import org.elasticsearch.threadpool.Scheduler;
 import org.elasticsearch.xcontent.XContentType;
+import org.elasticsearch.xpack.core.security.authz.RoleMappingMetadata;
 import org.elasticsearch.xpack.security.SecurityFeatures;
+import org.elasticsearch.xpack.security.action.rolemapping.ReservedRoleMappingAction;
 
 import java.time.Instant;
 import java.util.List;
@@ -74,7 +77,7 @@
 public class SecurityIndexManager implements ClusterStateListener {
 
     public static final String SECURITY_VERSION_STRING = "security-version";
-
+    private static final String FILE_SETTINGS_METADATA_NAMESPACE = "file_settings";
     private static final Logger logger = LogManager.getLogger(SecurityIndexManager.class);
 
     /**
@@ -85,6 +88,13 @@ public enum Availability {
         PRIMARY_SHARDS
     }
 
+    public enum RoleMappingsCleanupMigrationStatus {
+        READY,
+        NOT_READY,
+        SKIP,
+        DONE
+    }
+
     private final Client client;
     private final SystemIndexDescriptor systemIndexDescriptor;
 
@@ -267,6 +277,49 @@ private static boolean isCreatedOnLatestVersion(IndexMetadata indexMetadata) {
         return indexVersionCreated != null && indexVersionCreated.onOrAfter(IndexVersion.current());
     }
 
+    /**
+     * Check if a role mappings cleanup migration is needed or has already been performed and if the cluster is ready for a cleanup
+     * migration
+     *
+     * @param clusterState current cluster state
+     * @param migrationsVersion current migration version
+     *
+     * @return RoleMappingsCleanupMigrationStatus
+     */
+    static RoleMappingsCleanupMigrationStatus getRoleMappingsCleanupMigrationStatus(ClusterState clusterState, int migrationsVersion) {
+        // Migration already finished
+        if (migrationsVersion >= SecurityMigrations.CLEANUP_ROLE_MAPPING_DUPLICATES_MIGRATION_VERSION) {
+            return RoleMappingsCleanupMigrationStatus.DONE;
+        }
+
+        ReservedStateMetadata fileSettingsMetadata = clusterState.metadata().reservedStateMetadata().get(FILE_SETTINGS_METADATA_NAMESPACE);
+        boolean hasFileSettingsMetadata = fileSettingsMetadata != null;
+        // If there is no fileSettingsMetadata, there should be no reserved state (this is to catch bugs related to
+        // name changes to FILE_SETTINGS_METADATA_NAMESPACE)
+        assert hasFileSettingsMetadata || clusterState.metadata().reservedStateMetadata().isEmpty();
+
+        // If no file based role mappings available -> migration not needed
+        if (hasFileSettingsMetadata == false || fileSettingsMetadata.keys(ReservedRoleMappingAction.NAME).isEmpty()) {
+            return RoleMappingsCleanupMigrationStatus.SKIP;
+        }
+
+        RoleMappingMetadata roleMappingMetadata = RoleMappingMetadata.getFromClusterState(clusterState);
+
+        // If there are file based role mappings, make sure they have the latest format (name available) and that they have all been
+        // synced to cluster state (same size as the reserved state keys)
+        if (roleMappingMetadata.getRoleMappings().size() == fileSettingsMetadata.keys(ReservedRoleMappingAction.NAME).size()
+            && roleMappingMetadata.hasAnyMappingWithFallbackName() == false) {
+            return RoleMappingsCleanupMigrationStatus.READY;
+        }
+
+        // If none of the above conditions are met, wait for a state change to re-evaluate if the cluster is ready for migration
+        return RoleMappingsCleanupMigrationStatus.NOT_READY;
+    }
+
+    public RoleMappingsCleanupMigrationStatus getRoleMappingsCleanupMigrationStatus() {
+        return state.roleMappingsCleanupMigrationStatus;
+    }
+
     @Override
     public void clusterChanged(ClusterChangedEvent event) {
         if (event.state().blocks().hasGlobalBlock(GatewayService.STATE_NOT_RECOVERED_BLOCK)) {
@@ -284,8 +337,12 @@ public void clusterChanged(ClusterChangedEvent event) {
         Tuple<Boolean, Boolean> available = checkIndexAvailable(event.state());
         final boolean indexAvailableForWrite = available.v1();
         final boolean indexAvailableForSearch = available.v2();
-        final boolean mappingIsUpToDate = indexMetadata == null || checkIndexMappingUpToDate(event.state());
         final int migrationsVersion = getMigrationVersionFromIndexMetadata(indexMetadata);
+        final RoleMappingsCleanupMigrationStatus roleMappingsCleanupMigrationStatus = getRoleMappingsCleanupMigrationStatus(
+            event.state(),
+            migrationsVersion
+        );
+        final boolean mappingIsUpToDate = indexMetadata == null || checkIndexMappingUpToDate(event.state());
         final SystemIndexDescriptor.MappingsVersion minClusterMappingVersion = getMinSecurityIndexMappingVersion(event.state());
         final int indexMappingVersion = loadIndexMappingVersion(systemIndexDescriptor.getAliasName(), event.state());
         final String concreteIndexName = indexMetadata == null
@@ -314,6 +371,7 @@ public void clusterChanged(ClusterChangedEvent event) {
             indexAvailableForWrite,
             mappingIsUpToDate,
             createdOnLatestVersion,
+            roleMappingsCleanupMigrationStatus,
             migrationsVersion,
             minClusterMappingVersion,
             indexMappingVersion,
@@ -438,7 +496,8 @@ private Tuple<Boolean, Boolean> checkIndexAvailable(ClusterState state) {
 
     public boolean isEligibleSecurityMigration(SecurityMigrations.SecurityMigration securityMigration) {
         return state.securityFeatures.containsAll(securityMigration.nodeFeaturesRequired())
-            && state.indexMappingVersion >= securityMigration.minMappingVersion();
+            && state.indexMappingVersion >= securityMigration.minMappingVersion()
+            && securityMigration.checkPreConditions(state);
     }
 
     public boolean isReadyForSecurityMigration(SecurityMigrations.SecurityMigration securityMigration) {
@@ -678,6 +737,7 @@ public static class State {
             null,
             null,
             null,
+            null,
             Set.of()
         );
         public final Instant creationTime;
@@ -686,6 +746,7 @@ public static class State {
         public final boolean indexAvailableForWrite;
         public final boolean mappingUpToDate;
         public final boolean createdOnLatestVersion;
+        public final RoleMappingsCleanupMigrationStatus roleMappingsCleanupMigrationStatus;
         public final Integer migrationsVersion;
         // Min mapping version supported by the descriptors in the cluster
         public final SystemIndexDescriptor.MappingsVersion minClusterMappingVersion;
@@ -704,6 +765,7 @@ public State(
             boolean indexAvailableForWrite,
             boolean mappingUpToDate,
             boolean createdOnLatestVersion,
+            RoleMappingsCleanupMigrationStatus roleMappingsCleanupMigrationStatus,
             Integer migrationsVersion,
             SystemIndexDescriptor.MappingsVersion minClusterMappingVersion,
             Integer indexMappingVersion,
@@ -720,6 +782,7 @@ public State(
             this.mappingUpToDate = mappingUpToDate;
             this.migrationsVersion = migrationsVersion;
             this.createdOnLatestVersion = createdOnLatestVersion;
+            this.roleMappingsCleanupMigrationStatus = roleMappingsCleanupMigrationStatus;
             this.minClusterMappingVersion = minClusterMappingVersion;
             this.indexMappingVersion = indexMappingVersion;
             this.concreteIndexName = concreteIndexName;
@@ -740,6 +803,7 @@ public boolean equals(Object o) {
                 && indexAvailableForWrite == state.indexAvailableForWrite
                 && mappingUpToDate == state.mappingUpToDate
                 && createdOnLatestVersion == state.createdOnLatestVersion
+                && roleMappingsCleanupMigrationStatus == state.roleMappingsCleanupMigrationStatus
                 && Objects.equals(indexMappingVersion, state.indexMappingVersion)
                 && Objects.equals(migrationsVersion, state.migrationsVersion)
                 && Objects.equals(minClusterMappingVersion, state.minClusterMappingVersion)
@@ -762,6 +826,7 @@ public int hashCode() {
                 indexAvailableForWrite,
                 mappingUpToDate,
                 createdOnLatestVersion,
+                roleMappingsCleanupMigrationStatus,
                 migrationsVersion,
                 minClusterMappingVersion,
                 indexMappingVersion,
@@ -786,6 +851,8 @@ public String toString() {
                 + mappingUpToDate
                 + ", createdOnLatestVersion="
                 + createdOnLatestVersion
+                + ", roleMappingsCleanupMigrationStatus="
+                + roleMappingsCleanupMigrationStatus
                 + ", migrationsVersion="
                 + migrationsVersion
                 + ", minClusterMappingVersion="