From 20061fc2fd2456621213e3ddec9748a1ef299e7d Mon Sep 17 00:00:00 2001 From: Nikolaj Volgushev Date: Sun, 30 Mar 2025 13:16:52 +0200 Subject: [PATCH 1/6] [Failure Store] manage privileges grant data and failures access --- .../IndexComponentSelectorPredicate.java | 4 ++ .../authz/privilege/IndexPrivilege.java | 18 +++++++- .../FailureStoreSecurityRestIT.java | 43 +++++++++++-------- .../xpack/security/authz/RBACEngine.java | 37 +++++++++++++++- 4 files changed, 79 insertions(+), 23 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/IndexComponentSelectorPredicate.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/IndexComponentSelectorPredicate.java index 0083de155032b..c24f0f5ec057c 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/IndexComponentSelectorPredicate.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/IndexComponentSelectorPredicate.java @@ -34,6 +34,10 @@ public record IndexComponentSelectorPredicate(Set names, Predicate resolve(Set name) { final Set allSelectorAccessPrivileges = new HashSet<>(); final Set dataSelectorAccessPrivileges = new HashSet<>(); final Set failuresSelectorAccessPrivileges = new HashSet<>(); + final Set dataAndFailuresSelectorAccessPrivileges = new HashSet<>(); boolean containsAllAccessPrivilege = name.stream().anyMatch(n -> getNamedOrNull(n) == ALL); for (String part : name) { @@ -383,6 +389,8 @@ private static Set resolve(Set name) { dataSelectorAccessPrivileges.add(indexPrivilege); } else if (indexPrivilege.selectorPredicate == IndexComponentSelectorPredicate.FAILURES) { failuresSelectorAccessPrivileges.add(indexPrivilege); + } else if (indexPrivilege.selectorPredicate == IndexComponentSelectorPredicate.DATA_AND_FAILURES) { + dataAndFailuresSelectorAccessPrivileges.add(indexPrivilege); } else { String errorMessage = "unexpected selector [" + indexPrivilege.selectorPredicate + "]"; assert false : errorMessage; @@ -406,6 +414,7 @@ private static Set resolve(Set name) { allSelectorAccessPrivileges, dataSelectorAccessPrivileges, failuresSelectorAccessPrivileges, + dataAndFailuresSelectorAccessPrivileges, actions ); assertNamesMatch(name, combined); @@ -416,11 +425,13 @@ private static Set combineIndexPrivileges( Set allSelectorAccessPrivileges, Set dataSelectorAccessPrivileges, Set failuresSelectorAccessPrivileges, + Set dataAndFailuresSelectorAccessPrivileges, Set actions ) { assert false == allSelectorAccessPrivileges.isEmpty() || false == dataSelectorAccessPrivileges.isEmpty() || false == failuresSelectorAccessPrivileges.isEmpty() + || false == dataAndFailuresSelectorAccessPrivileges.isEmpty() || false == actions.isEmpty() : "at least one of the privilege sets or actions must be non-empty"; if (false == allSelectorAccessPrivileges.isEmpty()) { @@ -434,6 +445,9 @@ private static Set combineIndexPrivileges( if (false == dataSelectorAccessPrivileges.isEmpty() || false == actions.isEmpty()) { combined.add(union(dataSelectorAccessPrivileges, actions, IndexComponentSelectorPredicate.DATA)); } + if (false == dataAndFailuresSelectorAccessPrivileges.isEmpty()) { + combined.add(union(dataAndFailuresSelectorAccessPrivileges, Set.of(), IndexComponentSelectorPredicate.DATA_AND_FAILURES)); + } if (false == failuresSelectorAccessPrivileges.isEmpty()) { combined.add(union(failuresSelectorAccessPrivileges, Set.of(), IndexComponentSelectorPredicate.FAILURES)); } diff --git a/x-pack/plugin/security/qa/security-trial/src/javaRestTest/java/org/elasticsearch/xpack/security/failurestore/FailureStoreSecurityRestIT.java b/x-pack/plugin/security/qa/security-trial/src/javaRestTest/java/org/elasticsearch/xpack/security/failurestore/FailureStoreSecurityRestIT.java index 85b5fd6d82401..f7cee1cb1e1f7 100644 --- a/x-pack/plugin/security/qa/security-trial/src/javaRestTest/java/org/elasticsearch/xpack/security/failurestore/FailureStoreSecurityRestIT.java +++ b/x-pack/plugin/security/qa/security-trial/src/javaRestTest/java/org/elasticsearch/xpack/security/failurestore/FailureStoreSecurityRestIT.java @@ -124,14 +124,10 @@ public void testGetUserPrivileges() throws IOException { "global": [], "indices": [{ "names": ["*"], - "privileges": ["read"], + "privileges": ["read", "read_failure_store"], "allow_restricted_indices": false - }, - { - "names": ["*"], - "privileges": ["read_failure_store"], - "allow_restricted_indices": false - }], + } + ], "applications": [], "run_as": [] }"""); @@ -208,14 +204,10 @@ public void testGetUserPrivileges() throws IOException { "indices": [ { "names": ["*"], - "privileges": ["read", "write"], - "allow_restricted_indices": false - }, - { - "names": ["*"], - "privileges": ["manage_failure_store", "read_failure_store"], + "privileges": ["manage_failure_store", "read", "read_failure_store", "write"], "allow_restricted_indices": false - }], + } + ], "applications": [], "run_as": [] }"""); @@ -1761,7 +1753,7 @@ public void testFailureStoreAccess() throws Exception { } } - public void testWriteOperations() throws IOException { + public void testWriteAndManageOperations() throws IOException { setupDataStream(); Tuple backingIndices = getSingleDataAndFailureIndices("test1"); String dataIndexName = backingIndices.v1(); @@ -1797,15 +1789,28 @@ public void testWriteOperations() throws IOException { } """); - // user with manage access to data stream does NOT get direct access to failure index - expectThrows(() -> deleteIndex(MANAGE_ACCESS, failureIndexName), 403); + // explain lifecycle API with and without failures selector is granted by manage + assertOK(performRequest(MANAGE_ACCESS, new Request("GET", "test1/_lifecycle/explain"))); + assertOK(performRequest(MANAGE_ACCESS, new Request("GET", "test1::failures/_lifecycle/explain"))); + assertOK(performRequest(MANAGE_ACCESS, new Request("GET", failureIndexName + "/_lifecycle/explain"))); + assertOK(performRequest(MANAGE_ACCESS, new Request("GET", dataIndexName + "/_lifecycle/explain"))); + + // explain lifecycle API is granted by manage_failure_store only for failures selector + expectThrows(() -> performRequest(MANAGE_FAILURE_STORE_ACCESS, new Request("GET", "test1/_lifecycle/explain")), 403); + assertOK(performRequest(MANAGE_FAILURE_STORE_ACCESS, new Request("GET", "test1::failures/_lifecycle/explain"))); + assertOK(performRequest(MANAGE_FAILURE_STORE_ACCESS, new Request("GET", failureIndexName + "/_lifecycle/explain"))); + expectThrows(() -> performRequest(MANAGE_FAILURE_STORE_ACCESS, new Request("GET", dataIndexName + "/_lifecycle/explain")), 403); + + // user with manage access to data stream can delete failure index because manage grants access to both data and failures + expectThrows(() -> deleteIndex(MANAGE_ACCESS, failureIndexName), 400); expectThrows(() -> deleteIndex(MANAGE_ACCESS, dataIndexName), 400); - // manage_failure_store user COULD delete failure index (not valid because it's a write index, but allow security-wise) - expectThrows(() -> deleteIndex(MANAGE_FAILURE_STORE_ACCESS, dataIndexName), 403); + // manage_failure_store user COULD delete failure index (not valid because it's a write index, but allowed security-wise) expectThrows(() -> deleteIndex(MANAGE_FAILURE_STORE_ACCESS, failureIndexName), 400); + expectThrows(() -> deleteIndex(MANAGE_FAILURE_STORE_ACCESS, dataIndexName), 403); expectThrows(() -> deleteDataStream(MANAGE_FAILURE_STORE_ACCESS, dataIndexName), 403); expectThrows(() -> deleteDataStream(MANAGE_FAILURE_STORE_ACCESS, "test1"), 403); + // selectors aren't supported for deletes so we get a 403 expectThrows(() -> deleteDataStream(MANAGE_FAILURE_STORE_ACCESS, "test1::failures"), 403); // manage user can delete data stream diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/RBACEngine.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/RBACEngine.java index faedcacf21d8c..076835a32ec12 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/RBACEngine.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/RBACEngine.java @@ -43,6 +43,7 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.util.CachedSupplier; import org.elasticsearch.common.util.set.Sets; +import org.elasticsearch.core.Tuple; import org.elasticsearch.index.Index; import org.elasticsearch.index.shard.ShardId; import org.elasticsearch.transport.TransportActionProxy; @@ -101,6 +102,7 @@ import java.util.Collections; import java.util.HashMap; import java.util.HashSet; +import java.util.LinkedHashMap; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; @@ -792,7 +794,7 @@ static GetUserPrivilegesResponse buildUserPrivilegesResponseObject(Role userRole } } - final Set indices = new LinkedHashSet<>(); + final LinkedHashSet indices = new LinkedHashSet<>(); for (IndicesPermission.Group group : userRole.indices().groups()) { indices.add(toIndices(group)); } @@ -833,7 +835,7 @@ static GetUserPrivilegesResponse buildUserPrivilegesResponseObject(Role userRole return new GetUserPrivilegesResponse( cluster, conditionalCluster, - indices, + combineIndices(indices), application, runAs, remoteIndices, @@ -841,6 +843,37 @@ static GetUserPrivilegesResponse buildUserPrivilegesResponseObject(Role userRole ); } + private static LinkedHashSet combineIndices( + LinkedHashSet indices + ) { + final LinkedHashMap>, GetUserPrivilegesResponse.Indices> combinedIndices = new LinkedHashMap<>(); + for (GetUserPrivilegesResponse.Indices index : indices) { + final GetUserPrivilegesResponse.Indices existing = combinedIndices.get( + new Tuple<>(index.allowRestrictedIndices(), index.getIndices()) + ); + if (existing == null) { + combinedIndices.put(new Tuple<>(index.allowRestrictedIndices(), index.getIndices()), index); + } else { + Set combined = new LinkedHashSet<>(existing.getPrivileges()); + combined.addAll(index.getPrivileges()); + assert existing.allowRestrictedIndices() == index.allowRestrictedIndices(); + assert Objects.equals(existing.getFieldSecurity(), index.getFieldSecurity()); + assert Objects.equals(existing.getQueries(), index.getQueries()); + combinedIndices.put( + new Tuple<>(index.allowRestrictedIndices(), index.getIndices()), + new GetUserPrivilegesResponse.Indices( + index.getIndices(), + combined, + index.getFieldSecurity(), + index.getQueries(), + index.allowRestrictedIndices() + ) + ); + } + } + return new LinkedHashSet<>(combinedIndices.values()); + } + private static GetUserPrivilegesResponse.Indices toIndices(final IndicesPermission.Group group) { final Set queries = group.getQuery() == null ? Collections.emptySet() : group.getQuery(); final Set fieldSecurity = getFieldGrantExcludeGroups(group); From 35b214b969b31732144b9096b3380851f7605a00 Mon Sep 17 00:00:00 2001 From: Nikolaj Volgushev Date: Sun, 30 Mar 2025 14:20:10 +0200 Subject: [PATCH 2/6] More tests --- .../authz/privilege/IndexPrivilegeTests.java | 70 ++++++++++++++++++- .../authz/store/CompositeRolesStoreTests.java | 15 ++-- 2 files changed, 80 insertions(+), 5 deletions(-) diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/privilege/IndexPrivilegeTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/privilege/IndexPrivilegeTests.java index a6342b9f3e19d..4559b4c6d7a84 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/privilege/IndexPrivilegeTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/privilege/IndexPrivilegeTests.java @@ -229,6 +229,70 @@ public void testResolveBySelectorAccess() { List actualPredicates = actual.stream().map(IndexPrivilege::getSelectorPredicate).toList(); assertThat(actualPredicates, containsInAnyOrder(IndexComponentSelectorPredicate.ALL)); } + { + Set actual = IndexPrivilege.resolveBySelectorAccess( + Set.of("manage", "all", "read", "indices:data/read/search", "view_index_metadata") + ); + assertThat( + actual, + containsInAnyOrder( + resolvePrivilegeAndAssertSingleton(Set.of("manage", "all", "read", "indices:data/read/search", "view_index_metadata")) + ) + ); + List actualPredicates = actual.stream().map(IndexPrivilege::getSelectorPredicate).toList(); + assertThat(actualPredicates, containsInAnyOrder(IndexComponentSelectorPredicate.ALL)); + } + { + Set actual = IndexPrivilege.resolveBySelectorAccess( + Set.of("manage", "read", "indices:data/read/search", "read_failure_store") + ); + assertThat( + actual, + containsInAnyOrder( + IndexPrivilege.MANAGE, + IndexPrivilege.READ_FAILURE_STORE, + resolvePrivilegeAndAssertSingleton(Set.of("read", "indices:data/read/search")) + ) + ); + List actualPredicates = actual.stream().map(IndexPrivilege::getSelectorPredicate).toList(); + assertThat( + actualPredicates, + containsInAnyOrder( + IndexComponentSelectorPredicate.DATA, + IndexComponentSelectorPredicate.FAILURES, + IndexComponentSelectorPredicate.DATA_AND_FAILURES + ) + ); + } + { + Set actual = IndexPrivilege.resolveBySelectorAccess(Set.of("manage", "read", "indices:data/read/search")); + assertThat( + actual, + containsInAnyOrder(IndexPrivilege.MANAGE, resolvePrivilegeAndAssertSingleton(Set.of("read", "indices:data/read/search"))) + ); + List actualPredicates = actual.stream().map(IndexPrivilege::getSelectorPredicate).toList(); + assertThat( + actualPredicates, + containsInAnyOrder(IndexComponentSelectorPredicate.DATA, IndexComponentSelectorPredicate.DATA_AND_FAILURES) + ); + } + { + Set actual = IndexPrivilege.resolveBySelectorAccess( + Set.of("manage", "read", "manage_data_stream_lifecycle", "indices:admin/*") + ); + assertThat( + actual, + containsInAnyOrder( + resolvePrivilegeAndAssertSingleton(Set.of("manage_data_stream_lifecycle", "manage")), + resolvePrivilegeAndAssertSingleton(Set.of("read", "indices:admin/*")) + ) + ); + List actualPredicates = actual.stream().map(IndexPrivilege::getSelectorPredicate).toList(); + assertThat( + actualPredicates, + containsInAnyOrder(IndexComponentSelectorPredicate.DATA, IndexComponentSelectorPredicate.DATA_AND_FAILURES) + ); + } } public void testPrivilegesForRollupFieldCapsAction() { @@ -289,7 +353,11 @@ public void testCrossClusterReplicationPrivileges() { assertThat( Automatons.subsetOf( crossClusterReplication.automaton, - resolvePrivilegeAndAssertSingleton(Set.of("manage", "read", "monitor")).automaton + IndexPrivilege.resolveBySelectorAccess(Set.of("manage", "read", "monitor")) + .stream() + .map(p -> p.automaton) + .reduce((a1, a2) -> Automatons.unionAndMinimize(List.of(a1, a2))) + .get() ), is(true) ); diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStoreTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStoreTests.java index 9092565275439..0dd3535a7a69b 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStoreTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStoreTests.java @@ -2023,11 +2023,18 @@ public void testBuildRoleWithFailureStorePrivilegeCollatesToKeepDlsFlsFromAnothe ); } - public void testBuildRoleNeverSplitsWithoutFailureStoreRelatedPrivileges() { + public void testBuildRoleDoesNotSplitIfAllPrivilegesHaveTheSameSelector() { String indexPattern = randomAlphanumericOfLength(10); - List nonFailurePrivileges = IndexPrivilege.names() + IndexComponentSelectorPredicate predicate = randomFrom( + IndexComponentSelectorPredicate.ALL, + IndexComponentSelectorPredicate.DATA, + IndexComponentSelectorPredicate.FAILURES, + IndexComponentSelectorPredicate.DATA_AND_FAILURES + ); + + List privilegesWithSelector = IndexPrivilege.names() .stream() - .filter(p -> IndexPrivilege.getNamedOrNull(p).getSelectorPredicate() != IndexComponentSelectorPredicate.FAILURES) + .filter(p -> IndexPrivilege.getNamedOrNull(p).getSelectorPredicate() == predicate) .toList(); Set usedPrivileges = new HashSet<>(); @@ -2038,7 +2045,7 @@ public void testBuildRoleNeverSplitsWithoutFailureStoreRelatedPrivileges() { // TODO this is due to an unrelated bug in index collation logic List privileges = randomValueOtherThanMany( p -> p.get(0).equals("none"), - () -> randomNonEmptySubsetOf(nonFailurePrivileges) + () -> randomNonEmptySubsetOf(privilegesWithSelector) ); usedPrivileges.addAll(privileges); indicesPrivileges[i] = builder.indices(indexPattern).privileges(privileges).build(); From 65dd80702e591ad4a1fbc32127612e94fa494ea4 Mon Sep 17 00:00:00 2001 From: Nikolaj Volgushev Date: Sun, 30 Mar 2025 14:24:40 +0200 Subject: [PATCH 3/6] ff --- .../authz/store/CompositeRolesStoreTests.java | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStoreTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStoreTests.java index 0dd3535a7a69b..91a08b212c89c 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStoreTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStoreTests.java @@ -2025,12 +2025,18 @@ public void testBuildRoleWithFailureStorePrivilegeCollatesToKeepDlsFlsFromAnothe public void testBuildRoleDoesNotSplitIfAllPrivilegesHaveTheSameSelector() { String indexPattern = randomAlphanumericOfLength(10); - IndexComponentSelectorPredicate predicate = randomFrom( - IndexComponentSelectorPredicate.ALL, - IndexComponentSelectorPredicate.DATA, - IndexComponentSelectorPredicate.FAILURES, - IndexComponentSelectorPredicate.DATA_AND_FAILURES - ); + IndexComponentSelectorPredicate predicate = (DataStream.isFailureStoreFeatureFlagEnabled()) + ? randomFrom( + IndexComponentSelectorPredicate.ALL, + IndexComponentSelectorPredicate.DATA, + IndexComponentSelectorPredicate.FAILURES, + IndexComponentSelectorPredicate.DATA_AND_FAILURES + ) + : randomFrom( + IndexComponentSelectorPredicate.ALL, + IndexComponentSelectorPredicate.DATA, + IndexComponentSelectorPredicate.DATA_AND_FAILURES + ); List privilegesWithSelector = IndexPrivilege.names() .stream() From 7f195f1cab107b2728d7b6df4dc2f4842bfaa7cf Mon Sep 17 00:00:00 2001 From: Nikolaj Volgushev Date: Sun, 30 Mar 2025 17:58:17 +0200 Subject: [PATCH 4/6] Tests --- .../FailureStoreSecurityRestIT.java | 47 ++++++++++ .../xpack/security/authz/RBACEngine.java | 33 ++++--- .../xpack/security/authz/RBACEngineTests.java | 91 +++++++++++++++++++ 3 files changed, 160 insertions(+), 11 deletions(-) diff --git a/x-pack/plugin/security/qa/security-trial/src/javaRestTest/java/org/elasticsearch/xpack/security/failurestore/FailureStoreSecurityRestIT.java b/x-pack/plugin/security/qa/security-trial/src/javaRestTest/java/org/elasticsearch/xpack/security/failurestore/FailureStoreSecurityRestIT.java index f7cee1cb1e1f7..75cf0e316864c 100644 --- a/x-pack/plugin/security/qa/security-trial/src/javaRestTest/java/org/elasticsearch/xpack/security/failurestore/FailureStoreSecurityRestIT.java +++ b/x-pack/plugin/security/qa/security-trial/src/javaRestTest/java/org/elasticsearch/xpack/security/failurestore/FailureStoreSecurityRestIT.java @@ -211,6 +211,53 @@ public void testGetUserPrivileges() throws IOException { "applications": [], "run_as": [] }"""); + + upsertRole(""" + { + "cluster": ["all"], + "indices": [ + { + "names": ["*", "idx"], + "privileges": ["read", "manage"], + "allow_restricted_indices": false + }, + { + "names": ["idx", "*"], + "privileges": ["manage_data_stream_lifecycle"], + "allow_restricted_indices": false + }, + { + "names": ["*", "idx"], + "privileges": ["write"], + "allow_restricted_indices": true + }, + { + "names": ["idx", "*"], + "privileges": ["manage"], + "allow_restricted_indices": true + } + ] + } + """, "role"); + expectUserPrivilegesResponse(""" + { + "cluster": ["all"], + "global": [], + "indices": [ + { + "names": ["*", "idx"], + "privileges": ["manage", "manage_data_stream_lifecycle", "read"], + "allow_restricted_indices": false + }, + { + "names": ["*", "idx"], + "privileges": ["manage", "write"], + "allow_restricted_indices": true + } + ], + "applications": [], + "run_as": [] + }"""); } public void testHasPrivileges() throws IOException { diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/RBACEngine.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/RBACEngine.java index 076835a32ec12..1b99bd6888c4f 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/RBACEngine.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/RBACEngine.java @@ -794,7 +794,7 @@ static GetUserPrivilegesResponse buildUserPrivilegesResponseObject(Role userRole } } - final LinkedHashSet indices = new LinkedHashSet<>(); + final Set indices = new LinkedHashSet<>(); for (IndicesPermission.Group group : userRole.indices().groups()) { indices.add(toIndices(group)); } @@ -843,10 +843,14 @@ static GetUserPrivilegesResponse buildUserPrivilegesResponseObject(Role userRole ); } - private static LinkedHashSet combineIndices( - LinkedHashSet indices - ) { - final LinkedHashMap>, GetUserPrivilegesResponse.Indices> combinedIndices = new LinkedHashMap<>(); + /** + * Due to selector-processing during role building + * (see {@link org.elasticsearch.xpack.core.security.authz.privilege.IndexPrivilege#resolveBySelectorAccess(Set)}), + * it is possible that multiple index groups with the same indices exist with different privilege sets. To provide a cleaner response, + * this method combines them into one group. + */ + private static Set combineIndices(Set indices) { + final Map>, GetUserPrivilegesResponse.Indices> combinedIndices = new LinkedHashMap<>(); for (GetUserPrivilegesResponse.Indices index : indices) { final GetUserPrivilegesResponse.Indices existing = combinedIndices.get( new Tuple<>(index.allowRestrictedIndices(), index.getIndices()) @@ -854,16 +858,23 @@ private static LinkedHashSet combineIndices( if (existing == null) { combinedIndices.put(new Tuple<>(index.allowRestrictedIndices(), index.getIndices()), index); } else { - Set combined = new LinkedHashSet<>(existing.getPrivileges()); - combined.addAll(index.getPrivileges()); - assert existing.allowRestrictedIndices() == index.allowRestrictedIndices(); - assert Objects.equals(existing.getFieldSecurity(), index.getFieldSecurity()); - assert Objects.equals(existing.getQueries(), index.getQueries()); + Set combinedPrivileges = new HashSet<>(existing.getPrivileges()); + combinedPrivileges.addAll(index.getPrivileges()); + boolean flsDlsMatch = Objects.equals(existing.getFieldSecurity(), index.getFieldSecurity()) + && Objects.equals(existing.getQueries(), index.getQueries()); + assert existing.getIndices().equals(index.getIndices()) + && existing.allowRestrictedIndices() == index.allowRestrictedIndices() + // due to collation & selector resolution code, fls and dls definitions are always the same if the indices match + && flsDlsMatch; + if (false == flsDlsMatch) { + // if the above invariant is violated, due to a bug, bail and return original indices (these will still be correct) + return indices; + } combinedIndices.put( new Tuple<>(index.allowRestrictedIndices(), index.getIndices()), new GetUserPrivilegesResponse.Indices( index.getIndices(), - combined, + combinedPrivileges, index.getFieldSecurity(), index.getQueries(), index.allowRestrictedIndices() diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/RBACEngineTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/RBACEngineTests.java index 9ff64723704e5..2af7511ed70ef 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/RBACEngineTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/RBACEngineTests.java @@ -1415,6 +1415,82 @@ public void testBuildUserPrivilegeResponse() { } } + public void testBuildUserPrivilegeResponseCombinesIndexPrivileges() { + final BytesArray query = new BytesArray(""" + {"term":{"public":true}}"""); + final Role role = Role.builder(RESTRICTED_INDICES, "test", "role") + .add(IndexPrivilegeTests.resolvePrivilegeAndAssertSingleton(Sets.newHashSet("read", "write")), "index-1") + .add(IndexPrivilege.ALL, "index-2") + .add( + new FieldPermissions(new FieldPermissionsDefinition(new String[] { "public.*" }, new String[0])), + Collections.singleton(query), + IndexPrivilege.MANAGE, + true, + "index-1", + "index-2" + ) + .add( + new FieldPermissions(new FieldPermissionsDefinition(new String[] { "public.*" }, new String[0])), + Collections.singleton(query), + IndexPrivilegeTests.resolvePrivilegeAndAssertSingleton(Sets.newHashSet("read", "write")), + true, + "index-2", + "index-1" + ) + .add( + new FieldPermissions(new FieldPermissionsDefinition(new String[] { "public.*" }, new String[0])), + Collections.singleton(query), + IndexPrivilegeTests.resolvePrivilegeAndAssertSingleton(Sets.newHashSet("read_failure_store", "manage_failure_store")), + true, + "index-2", + "index-1" + ) + .add( + FieldPermissions.DEFAULT, + null, + IndexPrivilegeTests.resolvePrivilegeAndAssertSingleton(Sets.newHashSet("read_failure_store")), + false, + "index-2", + "index-1" + ) + .build(); + + final GetUserPrivilegesResponse response = RBACEngine.buildUserPrivilegesResponseObject(role); + + final GetUserPrivilegesResponse.Indices index1 = findIndexPrivilege(response.getIndexPrivileges(), Set.of("index-1"), false); + assertThat(index1.getIndices(), containsInAnyOrder("index-1")); + assertThat(index1.getPrivileges(), containsInAnyOrder("read", "write")); + assertThat(index1.getFieldSecurity(), emptyIterable()); + assertThat(index1.getQueries(), emptyIterable()); + + final GetUserPrivilegesResponse.Indices index2 = findIndexPrivilege(response.getIndexPrivileges(), Set.of("index-2"), false); + assertThat(index2.getIndices(), containsInAnyOrder("index-2")); + assertThat(index2.getPrivileges(), containsInAnyOrder("all")); + assertThat(index2.getFieldSecurity(), emptyIterable()); + assertThat(index2.getQueries(), emptyIterable()); + + Set actualIndexPrivileges = response.getIndexPrivileges(); + assertThat(actualIndexPrivileges, iterableWithSize(4)); + final GetUserPrivilegesResponse.Indices index1And2 = findIndexPrivilege(actualIndexPrivileges, Set.of("index-1", "index-2"), true); + assertThat(index1And2.getIndices(), containsInAnyOrder("index-1", "index-2")); + assertThat(index1And2.getPrivileges(), containsInAnyOrder("read", "write", "read_failure_store", "manage_failure_store", "manage")); + assertThat( + index1And2.getFieldSecurity(), + containsInAnyOrder(new FieldPermissionsDefinition.FieldGrantExcludeGroup(new String[] { "public.*" }, new String[0])) + ); + assertThat(index1And2.getQueries(), containsInAnyOrder(query)); + + final GetUserPrivilegesResponse.Indices index1And2NotRestricted = findIndexPrivilege( + actualIndexPrivileges, + Set.of("index-1", "index-2"), + false + ); + assertThat(index1And2NotRestricted.getIndices(), containsInAnyOrder("index-1", "index-2")); + assertThat(index1And2NotRestricted.getPrivileges(), containsInAnyOrder("read_failure_store")); + assertThat(index1And2NotRestricted.getFieldSecurity(), emptyIterable()); + assertThat(index1And2NotRestricted.getQueries(), emptyIterable()); + } + public void testBackingIndicesAreIncludedForAuthorizedDataStreams() { final String dataStreamName = "my_data_stream"; User user = new User(randomAlphaOfLengthBetween(4, 12)); @@ -2017,6 +2093,21 @@ private static RequestInfo createRequestInfo(TransportRequest request, String ac ); } + private GetUserPrivilegesResponse.Indices findIndexPrivilege( + Set indices, + Set indexNames, + boolean allowRestrictedIndices + ) { + return indices.stream() + .filter( + i -> i.allowRestrictedIndices() == allowRestrictedIndices + && i.getIndices().containsAll(indexNames) + && indexNames.containsAll(i.getIndices()) + ) + .findFirst() + .get(); + } + private GetUserPrivilegesResponse.Indices findIndexPrivilege(Set indices, String name) { return indices.stream().filter(i -> i.getIndices().contains(name)).findFirst().get(); } From f881a484c383f858856e51064fbdd68a78cdeb1f Mon Sep 17 00:00:00 2001 From: Nikolaj Volgushev Date: Sun, 30 Mar 2025 18:10:32 +0200 Subject: [PATCH 5/6] Test --- .../FailureStoreSecurityRestIT.java | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/x-pack/plugin/security/qa/security-trial/src/javaRestTest/java/org/elasticsearch/xpack/security/failurestore/FailureStoreSecurityRestIT.java b/x-pack/plugin/security/qa/security-trial/src/javaRestTest/java/org/elasticsearch/xpack/security/failurestore/FailureStoreSecurityRestIT.java index 75cf0e316864c..12fefe0472007 100644 --- a/x-pack/plugin/security/qa/security-trial/src/javaRestTest/java/org/elasticsearch/xpack/security/failurestore/FailureStoreSecurityRestIT.java +++ b/x-pack/plugin/security/qa/security-trial/src/javaRestTest/java/org/elasticsearch/xpack/security/failurestore/FailureStoreSecurityRestIT.java @@ -91,6 +91,7 @@ protected Settings restAdminSettings() { private static final String WRITE_ACCESS = "write_access"; private static final String MANAGE_ACCESS = "manage_access"; private static final String MANAGE_FAILURE_STORE_ACCESS = "manage_failure_store_access"; + private static final String MANAGE_DATA_STREAM_LIFECYCLE = "manage_data_stream_lifecycle"; private static final SecureString PASSWORD = new SecureString("admin-password"); @Before @@ -1836,12 +1837,32 @@ public void testWriteAndManageOperations() throws IOException { } """); + createUser(MANAGE_DATA_STREAM_LIFECYCLE, PASSWORD, MANAGE_DATA_STREAM_LIFECYCLE); + upsertRole(Strings.format(""" + { + "cluster": ["all"], + "indices": [{"names": ["test*"], "privileges": ["manage_data_stream_lifecycle"]}] + }"""), MANAGE_DATA_STREAM_LIFECYCLE); + createAndStoreApiKey(MANAGE_DATA_STREAM_LIFECYCLE, randomBoolean() ? null : """ + { + "role": { + "cluster": ["all"], + "indices": [{"names": ["test*"], "privileges": ["manage_data_stream_lifecycle"]}] + } + } + """); + // explain lifecycle API with and without failures selector is granted by manage assertOK(performRequest(MANAGE_ACCESS, new Request("GET", "test1/_lifecycle/explain"))); assertOK(performRequest(MANAGE_ACCESS, new Request("GET", "test1::failures/_lifecycle/explain"))); assertOK(performRequest(MANAGE_ACCESS, new Request("GET", failureIndexName + "/_lifecycle/explain"))); assertOK(performRequest(MANAGE_ACCESS, new Request("GET", dataIndexName + "/_lifecycle/explain"))); + assertOK(performRequest(MANAGE_DATA_STREAM_LIFECYCLE, new Request("GET", "test1/_lifecycle/explain"))); + assertOK(performRequest(MANAGE_DATA_STREAM_LIFECYCLE, new Request("GET", "test1::failures/_lifecycle/explain"))); + assertOK(performRequest(MANAGE_DATA_STREAM_LIFECYCLE, new Request("GET", failureIndexName + "/_lifecycle/explain"))); + assertOK(performRequest(MANAGE_DATA_STREAM_LIFECYCLE, new Request("GET", dataIndexName + "/_lifecycle/explain"))); + // explain lifecycle API is granted by manage_failure_store only for failures selector expectThrows(() -> performRequest(MANAGE_FAILURE_STORE_ACCESS, new Request("GET", "test1/_lifecycle/explain")), 403); assertOK(performRequest(MANAGE_FAILURE_STORE_ACCESS, new Request("GET", "test1::failures/_lifecycle/explain"))); @@ -1851,6 +1872,7 @@ public void testWriteAndManageOperations() throws IOException { // user with manage access to data stream can delete failure index because manage grants access to both data and failures expectThrows(() -> deleteIndex(MANAGE_ACCESS, failureIndexName), 400); expectThrows(() -> deleteIndex(MANAGE_ACCESS, dataIndexName), 400); + // manage_failure_store user COULD delete failure index (not valid because it's a write index, but allowed security-wise) expectThrows(() -> deleteIndex(MANAGE_FAILURE_STORE_ACCESS, failureIndexName), 400); expectThrows(() -> deleteIndex(MANAGE_FAILURE_STORE_ACCESS, dataIndexName), 403); From c1b49033a9ff1e08643e7b38f1d54ddc56e2a51a Mon Sep 17 00:00:00 2001 From: Nikolaj Volgushev Date: Tue, 1 Apr 2025 10:07:30 +0200 Subject: [PATCH 6/6] Address review comments --- .../core/security/authz/privilege/IndexPrivilege.java | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/IndexPrivilege.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/IndexPrivilege.java index 37f0dcdc0a05e..8d436c57c55c2 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/IndexPrivilege.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/IndexPrivilege.java @@ -50,7 +50,6 @@ import java.util.Collections; import java.util.HashSet; import java.util.LinkedHashMap; -import java.util.LinkedHashSet; import java.util.Locale; import java.util.Map; import java.util.Objects; @@ -435,13 +434,17 @@ private static Set combineIndexPrivileges( || false == actions.isEmpty() : "at least one of the privilege sets or actions must be non-empty"; if (false == allSelectorAccessPrivileges.isEmpty()) { - assert failuresSelectorAccessPrivileges.isEmpty() && dataSelectorAccessPrivileges.isEmpty() - : "data and failure access must be empty when all access is present"; + assert failuresSelectorAccessPrivileges.isEmpty() + && dataSelectorAccessPrivileges.isEmpty() + && dataAndFailuresSelectorAccessPrivileges.isEmpty() : "data and failure access must be empty when all access is present"; return Set.of(union(allSelectorAccessPrivileges, actions, IndexComponentSelectorPredicate.ALL)); } // linked hash set to preserve order across selectors - final Set combined = new LinkedHashSet<>(); + final Set combined = Sets.newLinkedHashSetWithExpectedSize( + dataAndFailuresSelectorAccessPrivileges.size() + failuresSelectorAccessPrivileges.size() + dataSelectorAccessPrivileges.size() + + actions.size() + ); if (false == dataSelectorAccessPrivileges.isEmpty() || false == actions.isEmpty()) { combined.add(union(dataSelectorAccessPrivileges, actions, IndexComponentSelectorPredicate.DATA)); }