Add Role Mapping Cleanup Security Migration #114830

jfreden · 2024-10-15T13:25:20Z

This PR adds a cleanup security migration for role mapping duplicates that exist both in cluster state and the .security index. The cleanup task is implemented as a security migration.

The task compares the names of the role mappings in the operator-defined settings.json and the names of the role mappings in the .security index and removes the ones in the .security index.

Issue

This cleanup is done because there was a bug (fixed here) that caused ECK customers to have the same role mappings with the same names present both in cluster state and the .security index. The effective role mapping is the combination of the .security index mapping and the cluster state one. After the bug fix, if the mapping in cluster state is modified (through setting.json), the .security index will still contain the old mapping and this could lead to unintended behaviour.

Dependencies

Reprocess operator file settings on service start #114295
This migration doesn't have dependencies on prior migrations, but since we're adding more migrations, I've also created this PR as dependency/nice to have before merging this: Add refresh .security index call between security migrations #114879

jfreden · 2024-10-15T13:35:15Z

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

@@ -117,6 +117,7 @@ private static IndexVersion def(int id, Version luceneVersion) {
    public static final IndexVersion ENABLE_IGNORE_MALFORMED_LOGSDB = def(8_514_00_0, Version.LUCENE_9_11_1);
    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 ADD_ROLE_MAPPING_MIGRATION = def(8_517_00_0, Version.LUCENE_9_12_0);


This is needed to allow us to check if an index was created on the same version as the migration. This means that the index was created with all the settings/mappings/docs needed, so we don't need to run the migration (it can be short-circuit and skipped since there is nothing to migrate).

...main/java/org/elasticsearch/xpack/security/action/rolemapping/ReservedRoleMappingAction.java

...curity/src/main/java/org/elasticsearch/xpack/security/support/SecurityMigrationExecutor.java

jfreden · 2024-10-15T13:56:14Z

...in/security/src/main/java/org/elasticsearch/xpack/security/support/SecurityIndexManager.java

@@ -267,6 +272,11 @@ private static boolean isCreatedOnLatestVersion(IndexMetadata indexMetadata) {
        return indexVersionCreated != null && indexVersionCreated.onOrAfter(IndexVersion.current());
    }

+    private static Set<String> getFileSettingsMetadataHandlerRoleMappingKeys(ClusterState clusterState) {


This isn't exactly part of the security index, so this doesn't belong here, but I added it here anyway for simplicity. Ideally the security migration state should be tracked in a separate cluster state handler. I think that's an improvement/refactor that should be done in a separate PR if we keep adding migrations on state outside the security index.

The new state handler would still need to handle the main security index manager state, since we rely heavily on state from it in the framework (is the index available for example).

jfreden · 2024-10-16T12:39:32Z

...ling-upgrade/src/test/java/org/elasticsearch/upgrades/SecurityIndexRoleMappingCleanupIT.java

+            createNativeRoleMapping("no_name_conflict", Map.of("meta", "test"));
+            assertAllRoleMappings(client(), "no_name_conflict", "operator_role_mapping_1");
+        } else if (CLUSTER_TYPE == ClusterType.UPGRADED) {
+            waitForSecurityMigrationCompletion(adminClient(), 2);


These tests will fail here until the file service change is merged, since the preconditions for the migration are not met.

elasticsearchmachine · 2024-10-16T12:43:24Z

Pinging @elastic/es-security (Team:Security)

elasticsearchmachine · 2024-10-16T12:46:30Z

Hi @jfreden, I've created a changelog YAML for you.

jfreden · 2024-10-17T09:09:11Z

...n/java/org/elasticsearch/xpack/core/security/authc/support/mapper/ExpressionRoleMapping.java

@@ -189,7 +189,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();


These are not set to null anywhere in production code, but could be in the future (since constructor is public) and can in tests (as I found out).

...ugin/security/src/main/java/org/elasticsearch/xpack/security/support/SecurityMigrations.java

jfreden · 2024-10-17T09:54:08Z

...ugin/security/src/main/java/org/elasticsearch/xpack/security/support/SecurityMigrations.java

+        }
+    }
+
+    public static class CleanupRoleMappingDuplicatesMigration implements SecurityMigration {


The new migration

n1v0lg

Prod code looks solid 💯 I'll take another pass over it Monday and go through tests as well.

...in/security/src/main/java/org/elasticsearch/xpack/security/support/SecurityIndexManager.java

n1v0lg · 2024-10-24T12:22:15Z

...in/security/src/main/java/org/elasticsearch/xpack/security/support/SecurityIndexManager.java

+        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();


Nit: I'd include a message in the assert with a bit of the failure context like: "encountered unknown namespace in reserved state metadata"

...ugin/security/src/main/java/org/elasticsearch/xpack/security/support/SecurityMigrations.java

n1v0lg · 2024-10-24T12:35:09Z

...ugin/security/src/main/java/org/elasticsearch/xpack/security/support/SecurityMigrations.java

+            Map<Boolean, List<ExpressionRoleMapping>> partitionedRoleMappings = Arrays.stream(roleMappings)
+                .collect(
+                    Collectors.partitioningBy(
+                        roleMapping -> roleMapping.getMetadata().get(ExpressionRoleMapping.READ_ONLY_ROLE_MAPPING_METADATA_FLAG) != null


Nit: I think it's fair enough to move this into an isReadOnly() method (or similarly-named) on ExpressionRoleMapping

n1v0lg · 2024-10-24T12:37:21Z

...ugin/security/src/main/java/org/elasticsearch/xpack/security/support/SecurityMigrations.java

+            ActionListener<DeleteRoleMappingResponse> groupListener = new GroupedActionListener<>(
+                names.size(),
+                ActionListener.wrap(responses -> {
+                    Map<Boolean, List<DeleteRoleMappingResponse>> deletesPartitionedByFound = responses.stream()


Nit: we're only after the count here so instead of a partition-by I'd just go with a for loop that counts number not found and number deleted

...ugin/security/src/main/java/org/elasticsearch/xpack/security/support/SecurityMigrations.java

n1v0lg · 2024-10-24T14:39:59Z

...ugin/security/src/main/java/org/elasticsearch/xpack/security/support/SecurityMigrations.java

+                ActionListener.wrap(responses -> {
+                    Map<Boolean, List<DeleteRoleMappingResponse>> deletesPartitionedByFound = responses.stream()
+                        .collect(Collectors.partitioningBy(DeleteRoleMappingResponse::isFound));
+                    if (deletesPartitionedByFound.containsKey(false)) {


partitioningBy will always include entries for both partitions (an empty list if nothing was found) so this would print a WARN even if no role mappings were missed.

this doesn't matter if we use a regular counter var as per my comment above, but if we stick with partitioningBy gotta fix

n1v0lg · 2024-10-24T14:47:21Z

...ugin/security/src/main/java/org/elasticsearch/xpack/security/support/SecurityMigrations.java

-    );
+        @Override
+        public boolean checkPreConditions(SecurityIndexManager.State securityIndexManagerState) {
+            // If there are operator defined role mappings, make sure they've been loaded in to cluster state before launching migration


Nit: need to update the comment to reflect the SKIP condition

n1v0lg · 2024-10-24T14:48:52Z

...ugin/security/src/main/java/org/elasticsearch/xpack/security/support/SecurityMigrations.java

+                .collect(
+                    Collectors.partitioningBy(
+                        roleMapping -> roleMapping.getMetadata().get(ExpressionRoleMapping.READ_ONLY_ROLE_MAPPING_METADATA_FLAG) != null
+                            && (boolean) roleMapping.getMetadata().get(ExpressionRoleMapping.READ_ONLY_ROLE_MAPPING_METADATA_FLAG)


Nit: lets assert the flag is actually boolean to avoid failed cast surprises

n1v0lg · 2024-10-24T14:59:05Z

...ling-upgrade/src/test/java/org/elasticsearch/upgrades/SecurityIndexRoleMappingCleanupIT.java

+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.Matchers.containsInAnyOrder;
+
+public class SecurityIndexRoleMappingCleanupIT extends AbstractUpgradeTestCase {


Speaking of ITs, could you also update this bit: https://github.com/elastic/elasticsearch/blob/main/qa/rolling-upgrade/src/javaRestTest/java/org/elasticsearch/upgrades/FileSettingsRoleMappingUpgradeIT.java#L113?

I ended up changing this to contains and I found a bug in the old file handling. I think we might want to create a patch and backport to versions before 8.15.

The migration never executed because there wasn't a security index (expected behaviour from the migration), but since the role mappings should have been created in the security index it was strange that the index didn't exist. Turns out the call to the native role mapping store here can happen while the SecurityIndexManager is in unrecovered state. This leads to the native role mapping store thinking that the index is not up to date here and nothing is inserted in to the store.

This won't impact the current state of things, so we can proceed, but we should discuss how to address this after this fix.

I think this may have actually gotten fixed here: #92173

n1v0lg

Tests look great, too. A few coverage suggestions -- I'll make one more pass when all is ready. Thanks for iterating on this!

n1v0lg · 2024-10-28T11:28:04Z

...t/java/org/elasticsearch/xpack/security/support/CleanupRoleMappingDuplicatesMigrationIT.java

+        awaitFileSettingsWatcher();
+        // Setup listener to wait for role mapping
+        var fileBasedRoleMappingsWrittenListener = setupClusterStateListener(masterNode, "everyone_kibana_alone");
+        // Write role mappings with fallback name, this should block any security migration


I think the comment is off here: TEST_JSON_WITH_ROLE_MAPPINGS does not have a mapping with a fallback name

n1v0lg · 2024-10-28T11:43:52Z

...t/java/org/elasticsearch/xpack/security/support/CleanupRoleMappingDuplicatesMigrationIT.java

+        waitForMigrationCompletion(SecurityMigrations.CLEANUP_ROLE_MAPPING_DUPLICATES_MIGRATION_VERSION);
+    }
+
+    public void testSkipMigrationNoFileBasedMappings() throws Exception {


Love these tests!

Might also add the other skip case where there are file settings but no role mapping section

...t/java/org/elasticsearch/xpack/security/support/CleanupRoleMappingDuplicatesMigrationIT.java

n1v0lg · 2024-10-28T11:51:39Z

...security/src/test/java/org/elasticsearch/xpack/security/support/SecurityMigrationsTests.java

+            true
+        );
+    }
+


Let's also add a test for a native role mapping with the suffix but without the read-only flag

n1v0lg · 2024-10-28T11:52:48Z

...security/src/test/java/org/elasticsearch/xpack/security/support/SecurityMigrationsTests.java

+    }
+
+    private static ExpressionRoleMapping nativeRoleMapping(String name) {
+        return new ExpressionRoleMapping(name, null, null, null, null, true);


Could we randomize (or otherwise handle) randomBoolean() ? null : Map.of()? Just to cover both cases where metadata is null or explicitly doesn't have the flag set

n1v0lg · 2024-10-28T12:02:12Z

...in/security/src/main/java/org/elasticsearch/xpack/security/support/SecurityIndexManager.java

+     *
+     * @return RoleMappingsCleanupMigrationStatus
+     */
+    static RoleMappingsCleanupMigrationStatus getRoleMappingsCleanupMigrationStatus(ClusterState clusterState, int migrationsVersion) {


It be great to cover this method with some units tests -- the ITs are great but unit tests we can much better cover some of the NOT_READY cases, e.g., size mismatch & some (but not all) mappings with fallback names.

n1v0lg · 2024-10-28T12:04:11Z

...ugin/security/src/main/java/org/elasticsearch/xpack/security/support/SecurityMigrations.java

            }
+            assert indexManager.getRoleMappingsCleanupMigrationStatus() == READY;
+
+            getRoleMappings(client, ActionListener.wrap(roleMappings -> {


I think it's worth covering some of the failure paths with unit tests: in particular, a test where some deletions are 404 or failures showing that we still run through the whole set of names feels important

n1v0lg · 2024-10-28T12:04:45Z

...ugin/security/src/main/java/org/elasticsearch/xpack/security/support/SecurityMigrations.java

+        }
+
+        private void deleteNativeRoleMappings(Client client, List<String> names, ActionListener<Void> listener) {
+            ActionListener<DeleteRoleMappingResponse> groupListener = new GroupedActionListener<>(


Nit: could we add an assertion that names is not empty?

n1v0lg · 2024-10-28T12:10:22Z

...ling-upgrade/src/test/java/org/elasticsearch/upgrades/SecurityIndexRoleMappingCleanupIT.java

+            assertAllRoleMappings(client(), "operator_role_mapping_1", "operator_role_mapping_2");
+        } else if (CLUSTER_TYPE == ClusterType.MIXED) {
+            // Create a native role mapping that doesn't conflict with anything before the migration run
+            createNativeRoleMapping("no_name_conflict", Map.of("meta", "test"));


Let's also add a native mapping with a reserved suffix here (it should not get deleted)

Ah whoops, my bad -- in a mixed cluster createNativeRoleMapping("native_role_mapping" + ExpressionRoleMapping.READ_ONLY_ROLE_MAPPING_SUFFIX, Map.of("meta", "test")); can result in a rejected request -- I think we want to move this to the (CLUSTER_TYPE == ClusterType.OLD) branch

Ahh that's right!

We can't create it there since we won't be bale to validate it. Not all bwc tests will hit the old cluster code because of the assumeTrue statements. I think we should skip it in this case for that reason.

n1v0lg · 2024-10-28T12:28:34Z

...in/security/src/main/java/org/elasticsearch/xpack/security/support/SecurityIndexManager.java

@@ -261,10 +271,63 @@ private SystemIndexDescriptor.MappingsVersion getMinSecurityIndexMappingVersion(
     * Check if the index was created on the latest index version available in the cluster
     */
    private static boolean isCreatedOnLatestVersion(IndexMetadata indexMetadata) {


I think we want to rename this to isCreatedOnLatestMigrationVersion or something similar, since this method no longer checks latest index version per se, but latest migration version

This reverts commit 9533f6f.

jfreden · 2024-10-28T16:16:20Z

@n1v0lg Thanks for the review! I've addressed all the comments and reverted the index settings like we discussed offline.

elasticsearchmachine added the v8.16.0 label Oct 15, 2024

jfreden commented Oct 15, 2024

View reviewed changes

...main/java/org/elasticsearch/xpack/security/action/rolemapping/ReservedRoleMappingAction.java Outdated Show resolved Hide resolved

jfreden commented Oct 15, 2024

View reviewed changes

...curity/src/main/java/org/elasticsearch/xpack/security/support/SecurityMigrationExecutor.java Outdated Show resolved Hide resolved

jfreden force-pushed the add_eck_migration_8_16 branch from 84c38fc to d36b152 Compare October 15, 2024 13:53

jfreden commented Oct 15, 2024

View reviewed changes

jfreden added the v9.0.0 label Oct 16, 2024

Add security migration for cleaning up ECK role mappings

c23b783

jfreden force-pushed the add_eck_migration_8_16 branch from ddfe166 to c23b783 Compare October 16, 2024 12:37

jfreden commented Oct 16, 2024

View reviewed changes

jfreden marked this pull request as ready for review October 16, 2024 12:42

jfreden added the :Security/Security Security issues without another label label Oct 16, 2024

jfreden requested a review from n1v0lg October 16, 2024 12:43

elasticsearchmachine added the Team:Security Meta label for security team label Oct 16, 2024

jfreden added the >bug label Oct 16, 2024

Update docs/changelog/114830.yaml

157be76

brianseeders added v8.17.0 and removed v8.16.0 labels Oct 16, 2024

jfreden added 4 commits October 17, 2024 10:01

fixup! Use new API for role mapping cleanup

c4ad317

fixup! Test

dda3c77

fixup! Test

c6e54e6

fixup! Refactor and add tests

e018eaf

jfreden commented Oct 17, 2024

View reviewed changes

...ugin/security/src/main/java/org/elasticsearch/xpack/security/support/SecurityMigrations.java Show resolved Hide resolved

fixup! Fix cleanup test

3a7e61f

jfreden commented Oct 17, 2024

View reviewed changes

jfreden added 2 commits October 17, 2024 11:57

fixup! Add comments

8b50d79

fixup! merge build.gradle

d3259d5

jfreden added 3 commits October 24, 2024 09:14

Use an enum to track status

3430c81

fixup! checkstyle issue

5a96adf

Merge remote-tracking branch 'upstream/8.16' into add_eck_migration_8_16

2563993

jfreden mentioned this pull request Oct 24, 2024

Improve default behaviour for the security migrations minMappingVersion #115497

Open

jfreden added 2 commits October 24, 2024 10:58

Always include operator file

1709df1

Add another migration test suite

1e01e08

jfreden requested a review from n1v0lg October 24, 2024 12:09

jfreden added the test-full-bwc Trigger full BWC version matrix tests label Oct 24, 2024

jfreden added 3 commits October 24, 2024 14:15

fixup! Spotless

90eb45f

fixup! Add assertion on migration status

b86c0a8

fixup! Skip calc + test

ac9e6da

n1v0lg reviewed Oct 24, 2024

View reviewed changes

jfreden added 5 commits October 25, 2024 11:58

fixup! Review comments and test code

a91f37b

Merge remote-tracking branch 'upstream/8.16' into add_eck_migration_8_16

ce405e2

fixup! Test

304e105

fixup! Bug

1e65bf7

Use index setting to track migration version index created on

9533f6f

jfreden force-pushed the add_eck_migration_8_16 branch from 6f549ea to 9533f6f Compare October 28, 2024 10:21

jfreden added 2 commits October 28, 2024 11:29

fixup! line

d6e466b

fixup! Update name

9112aba

n1v0lg reviewed Oct 28, 2024

View reviewed changes

jfreden added 4 commits October 28, 2024 16:55

fixup! Add tests and review comments

a568a31

Revert "Use index setting to track migration version index created on"

2ed011b

This reverts commit 9533f6f.

fixup! name

e1bb9d0

fixup! Too much revert

3160bd4

jfreden requested a review from n1v0lg October 28, 2024 16:16

fixup! Failing tests

9eb0dbe

jfreden mentioned this pull request Oct 29, 2024

Add ECK Role Mapping Cleanup #115823

Merged

jfreden closed this Oct 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Role Mapping Cleanup Security Migration #114830

Add Role Mapping Cleanup Security Migration #114830

jfreden commented Oct 15, 2024 •

edited

Loading

jfreden Oct 15, 2024 •

edited

Loading

jfreden Oct 15, 2024 •

edited

Loading

jfreden Oct 16, 2024

elasticsearchmachine commented Oct 16, 2024

elasticsearchmachine commented Oct 16, 2024

jfreden Oct 17, 2024

jfreden Oct 17, 2024

n1v0lg left a comment •

edited

Loading

n1v0lg Oct 24, 2024

n1v0lg Oct 24, 2024

n1v0lg Oct 24, 2024

n1v0lg Oct 24, 2024

n1v0lg Oct 24, 2024

n1v0lg Oct 24, 2024

n1v0lg Oct 24, 2024

jfreden Oct 25, 2024 •

edited

Loading

n1v0lg Oct 28, 2024

jfreden Oct 29, 2024

n1v0lg left a comment

n1v0lg Oct 28, 2024

n1v0lg Oct 28, 2024

n1v0lg Oct 28, 2024

n1v0lg Oct 28, 2024

n1v0lg Oct 28, 2024

n1v0lg Oct 28, 2024

n1v0lg Oct 28, 2024

n1v0lg Oct 28, 2024

n1v0lg Oct 28, 2024

jfreden Oct 29, 2024

n1v0lg Oct 28, 2024

jfreden commented Oct 28, 2024

Add Role Mapping Cleanup Security Migration #114830

Add Role Mapping Cleanup Security Migration #114830

Conversation

jfreden commented Oct 15, 2024 • edited Loading

Issue

Dependencies

jfreden Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

jfreden Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticsearchmachine commented Oct 16, 2024

elasticsearchmachine commented Oct 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n1v0lg left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfreden Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n1v0lg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfreden commented Oct 28, 2024

jfreden commented Oct 15, 2024 •

edited

Loading

jfreden Oct 15, 2024 •

edited

Loading

jfreden Oct 15, 2024 •

edited

Loading

n1v0lg left a comment •

edited

Loading

jfreden Oct 25, 2024 •

edited

Loading