From 44bccc477be40c981e69c8f1d80ff831c0b3f373 Mon Sep 17 00:00:00 2001 From: Athena Brown Date: Thu, 9 Jan 2025 21:38:09 -0700 Subject: [PATCH 1/4] Add `read_failures` privilege for authorizing failure store This commit adds the `read_failures` privilege and the logic supporting that privilege. The `read_failures` privilege enables read access to failure store indices owned by data streams named in the `indices` field of an indices privileges group, without implying `read` access to that data stream's "normal" backing indices. This is a bit of a mismatch with the existing privilege model, which authorizes actions and indices orthogonally. As of this change, in order to fully authorize an action, *both* action name and requested indices must be considered. Non-read actions to failure store indices, such as management calls, are authorized the same as backing indices; authorization will be granted to manage failure store indices if the user has permission to manage the owning data stream. It is only data visibility that is gated behind the new permission. --- .../security/get-builtin-privileges.asciidoc | 1 + .../metadata/IndexNameExpressionResolver.java | 2 +- .../authz/permission/IndicesPermission.java | 358 ++++++++++-------- .../core/security/authz/permission/Role.java | 1 + .../authz/privilege/IndexPrivilege.java | 12 +- .../authz/privilege/IndexPrivilegeTests.java | 2 +- .../RemoteClusterSecurityEsqlIT.java | 2 +- ...lusterSecurityFcActionAuthorizationIT.java | 2 +- .../ApiKeyWorkflowsRestrictionRestIT.java | 2 +- .../integration/DataStreamSecurityIT.java | 98 +++++ .../xpack/security/authz/RBACEngine.java | 26 +- .../authz/AuthorizationServiceTests.java | 20 +- .../test/privileges/11_builtin.yml | 2 +- 13 files changed, 356 insertions(+), 172 deletions(-) diff --git a/docs/reference/rest-api/security/get-builtin-privileges.asciidoc b/docs/reference/rest-api/security/get-builtin-privileges.asciidoc index 08a03a5b1e830..62c45a379a487 100644 --- a/docs/reference/rest-api/security/get-builtin-privileges.asciidoc +++ b/docs/reference/rest-api/security/get-builtin-privileges.asciidoc @@ -156,6 +156,7 @@ A successful call returns an object with "cluster", "index", and "remote_cluster "read", "read_cross_cluster", "view_index_metadata", + "read_failures", "write" ], "remote_cluster" : [ diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolver.java b/server/src/main/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolver.java index 8b88f7609fa8c..40a3089d2e60b 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolver.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolver.java @@ -1719,7 +1719,7 @@ private static Set expandToOpenClosed( for (int i = 0, n = indexAbstraction.getIndices().size(); i < n; i++) { Index index = indexAbstraction.getIndices().get(i); IndexMetadata indexMetadata = context.state.metadata().index(index); - if (indexMetadata.getState() != excludeState) { + if (indexMetadata != null && indexMetadata.getState() != excludeState) { resources.add( new ResolvedExpression( index.getName(), diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java index 4ea590afff864..80a8646897c47 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java @@ -41,11 +41,14 @@ import java.util.Objects; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; -import java.util.function.BiPredicate; +import java.util.function.BiFunction; import java.util.function.Predicate; import java.util.function.Supplier; import static java.util.Collections.unmodifiableMap; +import static org.elasticsearch.xpack.core.security.authz.privilege.IndexPrivilege.READ; +import static org.elasticsearch.xpack.core.security.authz.privilege.IndexPrivilege.READ_FAILURES; +import static org.elasticsearch.xpack.core.security.authz.privilege.IndexPrivilege.READ_FAILURES_PRIVILEGE_NAME; /** * A permission that is based on privileges for index related actions executed @@ -65,6 +68,22 @@ public final class IndicesPermission { private final Group[] groups; private final boolean hasFieldOrDocumentLevelSecurity; + public enum AuthorizedComponents { + NONE, + DATA, + FAILURES, + ALL; + + public AuthorizedComponents and(AuthorizedComponents other) { + return switch (this) { + case ALL -> ALL; + case NONE -> other; + case DATA -> other == FAILURES || other == ALL ? ALL : DATA; + case FAILURES -> other == DATA || other == ALL ? ALL : FAILURES; + }; + } + } + public static class Builder { RestrictedIndices restrictedIndices; @@ -141,51 +160,31 @@ public boolean hasFieldOrDocumentLevelSecurity() { } private IsResourceAuthorizedPredicate buildIndexMatcherPredicateForAction(String action) { - final Set ordinaryIndices = new HashSet<>(); - final Set restrictedIndices = new HashSet<>(); - final Set grantMappingUpdatesOnIndices = new HashSet<>(); - final Set grantMappingUpdatesOnRestrictedIndices = new HashSet<>(); - final boolean isMappingUpdateAction = isMappingUpdateAction(action); + IsResourceAuthorizedPredicate predicate = new IsResourceAuthorizedPredicate((name, abstraction) -> AuthorizedComponents.NONE); for (final Group group : groups) { - if (group.actionMatcher.test(action)) { - if (group.allowRestrictedIndices) { - restrictedIndices.addAll(Arrays.asList(group.indices())); - } else { - ordinaryIndices.addAll(Arrays.asList(group.indices())); - } - } else if (isMappingUpdateAction && containsPrivilegeThatGrantsMappingUpdatesForBwc(group)) { - // special BWC case for certain privileges: allow put mapping on indices and aliases (but not on data streams), even if - // the privilege definition does not currently allow it - if (group.allowRestrictedIndices) { - grantMappingUpdatesOnRestrictedIndices.addAll(Arrays.asList(group.indices())); - } else { - grantMappingUpdatesOnIndices.addAll(Arrays.asList(group.indices())); - } - } + predicate = predicate.and(group.allowedIndicesPredicate(action)); } - final StringMatcher nameMatcher = indexMatcher(ordinaryIndices, restrictedIndices); - final StringMatcher bwcSpecialCaseMatcher = indexMatcher(grantMappingUpdatesOnIndices, grantMappingUpdatesOnRestrictedIndices); - return new IsResourceAuthorizedPredicate(nameMatcher, bwcSpecialCaseMatcher); + return predicate; } /** * This encapsulates the authorization test for resources. * There is an additional test for resources that are missing or that are not a datastream or a backing index. */ - public static class IsResourceAuthorizedPredicate implements BiPredicate { + public static class IsResourceAuthorizedPredicate { - private final BiPredicate biPredicate; + private final BiFunction biPredicate; // public for tests - public IsResourceAuthorizedPredicate(StringMatcher resourceNameMatcher, StringMatcher additionalNonDatastreamNameMatcher) { - this((String name, @Nullable IndexAbstraction indexAbstraction) -> { - assert indexAbstraction == null || name.equals(indexAbstraction.getName()); - return resourceNameMatcher.test(name) - || (isPartOfDatastream(indexAbstraction) == false && additionalNonDatastreamNameMatcher.test(name)); - }); - } - - private IsResourceAuthorizedPredicate(BiPredicate biPredicate) { + // public IsResourceAuthorizedPredicate(StringMatcher resourceNameMatcher, StringMatcher additionalNonDatastreamNameMatcher) { + // this((String name, @Nullable IndexAbstraction indexAbstraction) -> { + // assert indexAbstraction == null || name.equals(indexAbstraction.getName()); + // return resourceNameMatcher.test(name) + // || (isPartOfDatastream(indexAbstraction) == false && additionalNonDatastreamNameMatcher.test(name)); + // }); + // } + + private IsResourceAuthorizedPredicate(BiFunction biPredicate) { this.biPredicate = biPredicate; } @@ -194,9 +193,11 @@ private IsResourceAuthorizedPredicate(BiPredicate biPr * return a new {@link IsResourceAuthorizedPredicate} instance that is equivalent to the conjunction of * authorization tests of that other instance and this one. */ - @Override - public final IsResourceAuthorizedPredicate and(BiPredicate other) { - return new IsResourceAuthorizedPredicate(this.biPredicate.and(other)); + // @Override + public final IsResourceAuthorizedPredicate and(IsResourceAuthorizedPredicate other) { + return new IsResourceAuthorizedPredicate( + (name, abstraction) -> this.biPredicate.apply(name, abstraction).and(other.biPredicate.apply(name, abstraction)) + ); } /** @@ -204,7 +205,7 @@ public final IsResourceAuthorizedPredicate and(BiPredicate buildIndicesAccessC final Collection concreteIndices = resource.resolveConcreteIndices(lookup); for (Group group : groups) { + AuthorizedComponents authorizedComponents = group.allowedIndicesPredicate(action).test(resource.indexAbstraction); // the group covers the given index OR the given index is a backing index and the group covers the parent data stream - if (resource.checkIndex(group)) { - if (group.checkAction(action) - || (isMappingUpdateAction // for BWC reasons, mapping updates are exceptionally allowed for certain privileges on - // indices and aliases (but not on data streams) - && false == resource.isPartOfDataStream() - && containsPrivilegeThatGrantsMappingUpdatesForBwc(group))) { - granted = true; - // propagate DLS and FLS permissions over the concrete indices - for (String index : concreteIndices) { - final Set fieldPermissions = fieldPermissionsByIndex.compute(index, (k, existingSet) -> { - if (existingSet == null) { - // Most indices rely on the default (empty) field permissions object, so we optimize for that case - // Using an immutable single item set is significantly faster because it avoids any of the hashing - // and backing set creation. - return Set.of(group.getFieldPermissions()); - } else if (existingSet.size() == 1) { - FieldPermissions fp = group.getFieldPermissions(); - if (existingSet.contains(fp)) { - return existingSet; - } - // This index doesn't have a single field permissions object, replace the singleton with a real Set - final Set hashSet = new HashSet<>(existingSet); - hashSet.add(fp); - return hashSet; - } else { - existingSet.add(group.getFieldPermissions()); + if (authorizedComponents != null && authorizedComponents != AuthorizedComponents.NONE) { + granted = true; + // propagate DLS and FLS permissions over the concrete indices + for (String index : concreteIndices) { + final Set fieldPermissions = fieldPermissionsByIndex.compute(index, (k, existingSet) -> { + if (existingSet == null) { + // Most indices rely on the default (empty) field permissions object, so we optimize for that case + // Using an immutable single item set is significantly faster because it avoids any of the hashing + // and backing set creation. + return Set.of(group.getFieldPermissions()); + } else if (existingSet.size() == 1) { + FieldPermissions fp = group.getFieldPermissions(); + if (existingSet.contains(fp)) { return existingSet; } - }); - - DocumentLevelPermissions docPermissions; - if (group.hasQuery()) { - docPermissions = roleQueriesByIndex.computeIfAbsent(index, (k) -> new DocumentLevelPermissions()); - docPermissions.addAll(group.getQuery()); + // This index doesn't have a single field permissions object, replace the singleton with a real Set + final Set hashSet = new HashSet<>(existingSet); + hashSet.add(fp); + return hashSet; } else { - // if more than one permission matches for a concrete index here and if - // a single permission doesn't have a role query then DLS will not be - // applied even when other permissions do have a role query - docPermissions = DocumentLevelPermissions.ALLOW_ALL; - // don't worry about what's already there - just overwrite it, it avoids doing a 2nd hash lookup. - roleQueriesByIndex.put(index, docPermissions); + existingSet.add(group.getFieldPermissions()); + return existingSet; } + }); - if (index.equals(resource.name) == false) { - fieldPermissionsByIndex.put(resource.name, fieldPermissions); - roleQueriesByIndex.put(resource.name, docPermissions); - } + DocumentLevelPermissions docPermissions; + if (group.hasQuery()) { + docPermissions = roleQueriesByIndex.computeIfAbsent(index, (k) -> new DocumentLevelPermissions()); + docPermissions.addAll(group.getQuery()); + } else { + // if more than one permission matches for a concrete index here and if + // a single permission doesn't have a role query then DLS will not be + // applied even when other permissions do have a role query + docPermissions = DocumentLevelPermissions.ALLOW_ALL; + // don't worry about what's already there - just overwrite it, it avoids doing a 2nd hash lookup. + roleQueriesByIndex.put(index, docPermissions); + } + + if (index.equals(resource.name) == false) { + fieldPermissionsByIndex.put(resource.name, fieldPermissions); + roleQueriesByIndex.put(resource.name, docPermissions); } } } @@ -661,43 +643,23 @@ private boolean isActionGranted(final String action, final Map bwcDeprecationLogActions = new ArrayList<>(); for (Group group : groups) { - // the group covers the given index OR the given index is a backing index and the group covers the parent data stream - if (resource.checkIndex(group)) { + AuthorizedComponents authResult = group.allowedIndicesPredicate(action).test(resource.indexAbstraction); + if (authResult != null && authResult != AuthorizedComponents.NONE) { boolean actionCheck = group.checkAction(action); // If action is granted we don't have to check for BWC and can stop at first granting group. if (actionCheck) { granted = true; break; - } else { - // mapping updates are allowed for certain privileges on indices and aliases (but not on data streams), - // outside of the privilege definition - boolean bwcMappingActionCheck = isMappingUpdateAction - && false == resource.isPartOfDataStream() - && containsPrivilegeThatGrantsMappingUpdatesForBwc(group); - bwcGrantMappingUpdate = bwcGrantMappingUpdate || bwcMappingActionCheck; - - if (bwcMappingActionCheck) { - logDeprecatedBwcPrivilegeUsage(action, resource, group, bwcDeprecationLogActions); - } } } } - if (false == granted && bwcGrantMappingUpdate) { - // the action is granted only due to the deprecated behaviour of certain privileges - granted = true; - bwcDeprecationLogActions.forEach(Runnable::run); - } - if (granted == false) { // We stop and return at first not granted resource. return false; @@ -708,34 +670,6 @@ private boolean isActionGranted(final String action, final Map bwcDeprecationLogActions - ) { - for (String privilegeName : group.privilege.name()) { - if (PRIVILEGE_NAME_SET_BWC_ALLOW_MAPPING_UPDATE.contains(privilegeName)) { - bwcDeprecationLogActions.add( - () -> deprecationLogger.warn( - DeprecationCategory.SECURITY, - "[" + resource.name + "] mapping update for ingest privilege [" + privilegeName + "]", - "the index privilege [" - + privilegeName - + "] allowed the update " - + "mapping action [" - + action - + "] on index [" - + resource.name - + "], this privilege " - + "will not permit mapping updates in the next major release - users who require access " - + "to update mappings must be granted explicit privileges" - ) - ); - } - } - } - private boolean isConcreteRestrictedIndex(String indexPattern) { if (Regex.isSimpleMatchPattern(indexPattern) || Automatons.isLuceneRegex(indexPattern)) { return false; @@ -747,8 +681,16 @@ private static boolean isMappingUpdateAction(String action) { return action.equals(TransportPutMappingAction.TYPE.name()) || action.equals(TransportAutoPutMappingAction.TYPE.name()); } - private static boolean containsPrivilegeThatGrantsMappingUpdatesForBwc(Group group) { - return group.privilege().name().stream().anyMatch(PRIVILEGE_NAME_SET_BWC_ALLOW_MAPPING_UPDATE::contains); + private static boolean containsPrivilegeThatGrantsMappingUpdatesForBwc(Set names) { + return names.stream().anyMatch(PRIVILEGE_NAME_SET_BWC_ALLOW_MAPPING_UPDATE::contains); + } + + private static boolean isFailureReadAction(String action) { + return READ_FAILURES.predicate().test(action); + } + + private static boolean containsReadFailuresPrivilege(Group group) { + return group.privilege().name().contains(READ_FAILURES_PRIVILEGE_NAME); } /** @@ -821,6 +763,9 @@ public static class Group { // users. Setting this flag true eliminates the special status for the purpose of this permission - restricted indices still have // to be covered by the "indices" private final boolean allowRestrictedIndices; + // These two flags are just a cache so we don't have to constantly re-check strings + private final boolean hasMappingUpdateBwcPermissions; + private final boolean hasReadFailuresPrivilege; public Group( IndexPrivilege privilege, @@ -848,6 +793,8 @@ public Group( } this.fieldPermissions = Objects.requireNonNull(fieldPermissions); this.query = query; + this.hasMappingUpdateBwcPermissions = containsPrivilegeThatGrantsMappingUpdatesForBwc(privilege.name()); + this.hasReadFailuresPrivilege = this.privilege.name().stream().anyMatch(READ_FAILURES_PRIVILEGE_NAME::equals); } public IndexPrivilege privilege() { @@ -867,6 +814,111 @@ public FieldPermissions getFieldPermissions() { return fieldPermissions; } + IsResourceAuthorizedPredicate allowedIndicesPredicate(String action) { + return new IsResourceAuthorizedPredicate(allowedIndicesChecker(action)); + } + + /** + * Returns a checker which checks if users are permitted to access index abstractions for the given action. + * @param action + * @return A function which checks an index name and returns the components of this abstraction the user is authorized to access + * with the given action. + */ + private BiFunction allowedIndicesChecker(String action) { + boolean isReadAction = READ.predicate().test(action); + boolean isMappingUpdateAction = isMappingUpdateAction(action); + boolean actionMatches = actionMatcher.test(action); + boolean actionAuthorized = actionMatches || (isReadAction && hasReadFailuresPrivilege); + if (actionAuthorized == false) { + return (name, resource) -> { + if (isMappingUpdateAction && hasMappingUpdateBwcPermissions && resource.getParentDataStream() == null) { + for (String privilegeName : this.privilege.name()) { + if (PRIVILEGE_NAME_SET_BWC_ALLOW_MAPPING_UPDATE.contains(privilegeName)) { + // ATHE: Does this log more often? + deprecationLogger.warn( + DeprecationCategory.SECURITY, + "[" + resource.getName() + "] mapping update for ingest privilege [" + privilegeName + "]", + "the index privilege [" + + privilegeName + + "] allowed the update " + + "mapping action [" + + action + + "] on index [" + + resource.getName() + + "], this privilege " + + "will not permit mapping updates in the next major release - users who require access " + + "to update mappings must be granted explicit privileges" + ); + } + } + return AuthorizedComponents.ALL; + } + return AuthorizedComponents.NONE; + }; + } + + return (String abstractionName, IndexAbstraction resource) -> { + assert resource == null || abstractionName.equals(resource.getName()); + return switch (resource) { + case IndexAbstraction.Alias alias -> checkMultiIndexAbstraction(isReadAction, actionMatches, resource); + case DataStream dataStream -> checkMultiIndexAbstraction(isReadAction, actionMatches, resource); + case IndexAbstraction.ConcreteIndex index -> { + final DataStream ds = index.getParentDataStream(); + + if (ds != null) { + boolean isFailureStoreIndex = ds.getFailureIndices().containsIndex(resource.getName()); + + if (isReadAction) { + // If we're trying to read a failure store index, we need to have read_failures for the data stream + if (isFailureStoreIndex) { + if ((hasReadFailuresPrivilege && indexNameMatcher.test(ds.getName()))) { + yield AuthorizedComponents.FAILURES; // And authorize it as a failures index (i.e. no DLS/FLS) + } + } else { // not a failure store index + if (indexNameMatcher.test(ds.getName()) || indexNameMatcher.test(resource.getName())) { + yield AuthorizedComponents.DATA; + } + } + } else { // Not a read action, authenticate as normal + if (checkParentNameAndResourceName(ds.getName(), resource.getName())) { + yield AuthorizedComponents.DATA; + } + } + } else if (indexNameMatcher.test(resource.getName())) { + yield AuthorizedComponents.DATA; + } + yield AuthorizedComponents.NONE; + } + case null -> indexNameMatcher.test(abstractionName) ? AuthorizedComponents.DATA : AuthorizedComponents.NONE; + default -> { + assert false + : "unsupported index abstraction type, add support for your new index abstraction type " + + resource.getClass().getCanonicalName() + + " here"; + throw new IllegalStateException("unsupported index abstraction type: " + resource.getClass().getCanonicalName()); + } + }; + }; + } + + private boolean checkParentNameAndResourceName(String dsName, String indexName) { + return indexNameMatcher.test(dsName) || indexNameMatcher.test(indexName); + } + + private AuthorizedComponents checkMultiIndexAbstraction(boolean isReadAction, boolean actionMatches, IndexAbstraction resource) { + if (indexNameMatcher.test(resource.getName())) { + if (actionMatches && (isReadAction == false || hasReadFailuresPrivilege)) { + // User has both normal read privileges and read_failures OR normal privileges and action is not read + return AuthorizedComponents.ALL; + } else if (actionMatches && hasReadFailuresPrivilege == false) { + return AuthorizedComponents.DATA; + } else if (hasReadFailuresPrivilege) { // action not authorized by typical match + return AuthorizedComponents.FAILURES; + } + } + return AuthorizedComponents.NONE; + } + private boolean checkAction(String action) { return actionMatcher.test(action); } diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/Role.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/Role.java index f52f8f85f006d..d86253a83be53 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/Role.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/Role.java @@ -9,6 +9,7 @@ import org.apache.lucene.util.automaton.Automaton; import org.elasticsearch.TransportVersion; +import org.elasticsearch.action.support.IndexComponentSelector; import org.elasticsearch.cluster.metadata.IndexAbstraction; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.util.set.Sets; 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 7174b2f616c2a..6cd9a5cb94af1 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 @@ -81,6 +81,11 @@ public final class IndexPrivilege extends Privilege { ResolveIndexAction.NAME, TransportResolveClusterAction.NAME ); + // This is a special case: read_failures acts like `read` *only* for failure store indices in authorized data streams. + // This internal action is not used, but having it makes automaton subset checks work as expected with this privilege. + private static final Automaton READ_FAILURES_AUTOMATON = patterns( + "internal:special/read_failures" + ); private static final Automaton READ_CROSS_CLUSTER_AUTOMATON = patterns( "internal:transport/proxy/indices:data/read/*", TransportClusterSearchShardsAction.TYPE.name(), @@ -178,7 +183,11 @@ public final class IndexPrivilege extends Privilege { public static final IndexPrivilege NONE = new IndexPrivilege("none", Automatons.EMPTY); public static final IndexPrivilege ALL = new IndexPrivilege("all", ALL_AUTOMATON); - public static final IndexPrivilege READ = new IndexPrivilege("read", READ_AUTOMATON); + public static final String READ_PRIVILEGE_NAME = "read"; + public static final IndexPrivilege READ = new IndexPrivilege(READ_PRIVILEGE_NAME, READ_AUTOMATON); + public static final String READ_FAILURES_PRIVILEGE_NAME = "read_failures"; + // read_failures is a special case - it should act like `read`, but adjusted to only allow access to failure indices + public static final IndexPrivilege READ_FAILURES = new IndexPrivilege(READ_FAILURES_PRIVILEGE_NAME, READ_FAILURES_AUTOMATON); public static final IndexPrivilege READ_CROSS_CLUSTER = new IndexPrivilege("read_cross_cluster", READ_CROSS_CLUSTER_AUTOMATON); public static final IndexPrivilege CREATE = new IndexPrivilege("create", CREATE_AUTOMATON); public static final IndexPrivilege INDEX = new IndexPrivilege("index", INDEX_AUTOMATON); @@ -221,6 +230,7 @@ public final class IndexPrivilege extends Privilege { entry("create_index", CREATE_INDEX), entry("monitor", MONITOR), entry("read", READ), + entry("read_failures", READ_FAILURES), entry("index", INDEX), entry("delete", DELETE), entry("write", WRITE), 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 073b3b92a43a5..2c44c40e64712 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 @@ -58,7 +58,7 @@ public void testOrderingOfPrivilegeNames() throws Exception { } public void testFindPrivilegesThatGrant() { - assertThat(findPrivilegesThatGrant(TransportSearchAction.TYPE.name()), equalTo(List.of("read", "all"))); + assertThat(findPrivilegesThatGrant(TransportSearchAction.TYPE.name()), equalTo(List.of("read", "read_failures", "all"))); assertThat(findPrivilegesThatGrant(TransportIndexAction.NAME), equalTo(List.of("create_doc", "create", "index", "write", "all"))); assertThat(findPrivilegesThatGrant(TransportUpdateAction.NAME), equalTo(List.of("index", "write", "all"))); assertThat(findPrivilegesThatGrant(TransportDeleteAction.NAME), equalTo(List.of("delete", "write", "all"))); diff --git a/x-pack/plugin/security/qa/multi-cluster/src/javaRestTest/java/org/elasticsearch/xpack/remotecluster/RemoteClusterSecurityEsqlIT.java b/x-pack/plugin/security/qa/multi-cluster/src/javaRestTest/java/org/elasticsearch/xpack/remotecluster/RemoteClusterSecurityEsqlIT.java index d6bad85161fd9..145f6e536b275 100644 --- a/x-pack/plugin/security/qa/multi-cluster/src/javaRestTest/java/org/elasticsearch/xpack/remotecluster/RemoteClusterSecurityEsqlIT.java +++ b/x-pack/plugin/security/qa/multi-cluster/src/javaRestTest/java/org/elasticsearch/xpack/remotecluster/RemoteClusterSecurityEsqlIT.java @@ -676,7 +676,7 @@ public void testCrossClusterQueryWithOnlyRemotePrivs() throws Exception { error.getMessage(), containsString( "action [indices:data/read/esql] is unauthorized for user [remote_search_user] with effective roles [remote_search], " - + "this action is granted by the index privileges [read,read_cross_cluster,all]" + + "this action is granted by the index privileges [read,read_failures,read_cross_cluster,all]" ) ); diff --git a/x-pack/plugin/security/qa/multi-cluster/src/javaRestTest/java/org/elasticsearch/xpack/remotecluster/RemoteClusterSecurityFcActionAuthorizationIT.java b/x-pack/plugin/security/qa/multi-cluster/src/javaRestTest/java/org/elasticsearch/xpack/remotecluster/RemoteClusterSecurityFcActionAuthorizationIT.java index 793313e238651..a7d927d4d9689 100644 --- a/x-pack/plugin/security/qa/multi-cluster/src/javaRestTest/java/org/elasticsearch/xpack/remotecluster/RemoteClusterSecurityFcActionAuthorizationIT.java +++ b/x-pack/plugin/security/qa/multi-cluster/src/javaRestTest/java/org/elasticsearch/xpack/remotecluster/RemoteClusterSecurityFcActionAuthorizationIT.java @@ -435,7 +435,7 @@ public void testUpdateCrossClusterApiKey() throws Exception { + "for user [foo] with assigned roles [role] authenticated by API key id [" + apiKeyId + "] of user [test_user] on indices [index], this action is granted by the index privileges " - + "[view_index_metadata,manage,read,all]" + + "[view_index_metadata,manage,read,read_failures,all]" ) ); diff --git a/x-pack/plugin/security/qa/security-trial/src/javaRestTest/java/org/elasticsearch/xpack/security/apikey/ApiKeyWorkflowsRestrictionRestIT.java b/x-pack/plugin/security/qa/security-trial/src/javaRestTest/java/org/elasticsearch/xpack/security/apikey/ApiKeyWorkflowsRestrictionRestIT.java index e0b9d9ecd0ede..0475b8132919f 100644 --- a/x-pack/plugin/security/qa/security-trial/src/javaRestTest/java/org/elasticsearch/xpack/security/apikey/ApiKeyWorkflowsRestrictionRestIT.java +++ b/x-pack/plugin/security/qa/security-trial/src/javaRestTest/java/org/elasticsearch/xpack/security/apikey/ApiKeyWorkflowsRestrictionRestIT.java @@ -187,7 +187,7 @@ public void testWorkflowsRestrictionAllowsAccess() throws IOException { + apiKeyId + "] of user [" + WORKFLOW_API_KEY_USER - + "] on indices [my-app-b], this action is granted by the index privileges [read,all]" + + "] on indices [my-app-b], this action is granted by the index privileges [read,read_failures,all]" ) ); assertThat(e.getMessage(), not(containsString("access restricted by workflow"))); diff --git a/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/DataStreamSecurityIT.java b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/DataStreamSecurityIT.java index 2039b72c4f49f..002371cdf65b1 100644 --- a/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/DataStreamSecurityIT.java +++ b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/DataStreamSecurityIT.java @@ -8,27 +8,42 @@ package org.elasticsearch.integration; import org.elasticsearch.ElasticsearchSecurityException; +import org.elasticsearch.action.DocWriteRequest; +import org.elasticsearch.action.admin.indices.get.GetIndexRequest; +import org.elasticsearch.action.admin.indices.refresh.RefreshRequest; import org.elasticsearch.action.admin.indices.rollover.RolloverRequest; import org.elasticsearch.action.admin.indices.stats.IndicesStatsRequest; import org.elasticsearch.action.admin.indices.template.put.TransportPutComposableIndexTemplateAction; +import org.elasticsearch.action.bulk.BulkItemResponse; +import org.elasticsearch.action.bulk.BulkRequest; +import org.elasticsearch.action.bulk.BulkResponse; import org.elasticsearch.action.datastreams.CreateDataStreamAction; import org.elasticsearch.action.datastreams.ModifyDataStreamsAction; +import org.elasticsearch.action.index.IndexRequest; import org.elasticsearch.action.search.SearchRequest; import org.elasticsearch.cluster.ClusterState; import org.elasticsearch.cluster.ClusterStateUpdateTask; import org.elasticsearch.cluster.metadata.ComposableIndexTemplate; import org.elasticsearch.cluster.metadata.DataStream; import org.elasticsearch.cluster.metadata.DataStreamAction; +import org.elasticsearch.cluster.metadata.DataStreamFailureStore; +import org.elasticsearch.cluster.metadata.DataStreamOptions; import org.elasticsearch.cluster.metadata.Metadata; +import org.elasticsearch.cluster.metadata.ResettableValue; +import org.elasticsearch.cluster.metadata.Template; import org.elasticsearch.cluster.service.ClusterService; +import org.elasticsearch.common.compress.CompressedXContent; +import org.elasticsearch.core.Strings; import org.elasticsearch.datastreams.DataStreamsPlugin; import org.elasticsearch.index.Index; import org.elasticsearch.index.mapper.extras.MapperExtrasPlugin; import org.elasticsearch.plugins.Plugin; +import org.elasticsearch.rest.RestStatus; import org.elasticsearch.test.SecurityIntegTestCase; import org.elasticsearch.test.SecuritySettingsSource; import org.elasticsearch.test.SecuritySettingsSourceField; import org.elasticsearch.transport.netty4.Netty4Plugin; +import org.elasticsearch.xcontent.XContentType; import org.elasticsearch.xpack.security.LocalStateSecurity; import org.elasticsearch.xpack.wildcard.Wildcard; @@ -43,6 +58,7 @@ import static org.elasticsearch.xpack.core.security.authc.support.UsernamePasswordToken.basicAuthHeaderValue; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasSize; +import static org.hamcrest.Matchers.nullValue; public class DataStreamSecurityIT extends SecurityIntegTestCase { @@ -51,6 +67,31 @@ protected Collection> nodePlugins() { return List.of(LocalStateSecurity.class, Netty4Plugin.class, MapperExtrasPlugin.class, DataStreamsPlugin.class, Wildcard.class); } + @Override + protected String configUsers() { + final String usersPasswdHashed = new String( + getFastStoredHashAlgoForTests().hash(SecuritySettingsSourceField.TEST_PASSWORD_SECURE_STRING) + ); + return super.configUsers() + "only_failures:" + usersPasswdHashed + "\n"; + } + + @Override + protected String configUsersRoles() { + return super.configUsersRoles() + "only_failures:only_failures\n"; + } + + @Override + protected String configRoles() { + // role that has analyze indices privileges only + return Strings.format(""" + %s + only_failures: + indices: + - names: '*' + privileges: [ 'read_failures', 'write' ] + """, super.configRoles()); + } + public void testRemoveGhostReference() throws Exception { var headers = Map.of( BASIC_AUTH_HEADER, @@ -142,4 +183,61 @@ public void onFailure(Exception e) { assertThat(indicesStatsResponse.getIndices().size(), equalTo(shouldBreakIndexName ? 1 : 2)); } + public void testFailureStoreAuthorziation() throws Exception { + var adminHeaders = Map.of( + BASIC_AUTH_HEADER, + basicAuthHeaderValue(SecuritySettingsSource.TEST_USER_NAME, SecuritySettingsSourceField.TEST_PASSWORD_SECURE_STRING) + ); + final var adminClient = client().filterWithHeader(adminHeaders); + var onlyFailHeaders = Map.of( + BASIC_AUTH_HEADER, + basicAuthHeaderValue(SecuritySettingsSource.TEST_USER_NAME, SecuritySettingsSourceField.TEST_PASSWORD_SECURE_STRING) + ); + final var failuresClient = client().filterWithHeader(onlyFailHeaders); + + var putTemplateRequest = new TransportPutComposableIndexTemplateAction.Request("id"); + putTemplateRequest.indexTemplate( + ComposableIndexTemplate.builder() + .indexPatterns(List.of("stuff-*")) + .template( + Template.builder() + .mappings(CompressedXContent.fromJSON("{\"properties\": {\"code\": {\"type\": \"integer\"}}}")) + .dataStreamOptions( + new DataStreamOptions.Template( + ResettableValue.create(new DataStreamFailureStore.Template(ResettableValue.create(true))) + ) + ) + ) + .dataStreamTemplate(new ComposableIndexTemplate.DataStreamTemplate(false, false)) + .build() + ); + assertAcked(adminClient.execute(TransportPutComposableIndexTemplateAction.TYPE, putTemplateRequest).actionGet()); + + String dataStreamName = "stuff-es"; + var request = new CreateDataStreamAction.Request(TEST_REQUEST_TIMEOUT, TEST_REQUEST_TIMEOUT, dataStreamName); + assertAcked(adminClient.execute(CreateDataStreamAction.INSTANCE, request).actionGet()); + + BulkRequest bulkRequest = new BulkRequest(); + bulkRequest.add( + new IndexRequest(dataStreamName).opType(DocWriteRequest.OpType.CREATE) + .source("{\"code\": \"well this aint right\"}", XContentType.JSON), + new IndexRequest(dataStreamName).opType(DocWriteRequest.OpType.CREATE).source("{\"@timestamp\": \"2015-01-01T12:10:30Z\", \"code\": 404}", XContentType.JSON) + ); + BulkResponse bulkResponse = adminClient.bulk(bulkRequest).actionGet(); + assertThat(bulkResponse.getItems().length, equalTo(2)); + String backingIndexPrefix = DataStream.BACKING_INDEX_PREFIX + dataStreamName; + String failureIndexPrefix = DataStream.FAILURE_STORE_PREFIX + dataStreamName; + + for (BulkItemResponse itemResponse : bulkResponse) { + assertThat(itemResponse.getFailure(), nullValue()); + assertThat(itemResponse.status(), equalTo(RestStatus.CREATED)); + // assertThat(itemResponse.getIndex(), anyOf(startsWith(backingIndexPrefix), startsWith(failureIndexPrefix))); + } + + indicesAdmin().refresh(new RefreshRequest(dataStreamName)).actionGet(); + var getResp = failuresClient.admin().indices().getIndex(new GetIndexRequest().indices(dataStreamName + "::*")); + var searchResponse = failuresClient.prepareSearch(dataStreamName + "::failures").get(); + assertThat(searchResponse.getHits().getTotalHits().value(), equalTo(1L)); + } + } 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 c5f4af93ab4b8..a6f8867c69271 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 @@ -874,23 +874,33 @@ static AuthorizedIndices resolveAuthorizedIndicesFromRole( // TODO: can this be done smarter? I think there are usually more indices/aliases in the cluster then indices defined a roles? if (includeDataStreams) { for (IndexAbstraction indexAbstraction : lookup.values()) { - if (predicate.test(indexAbstraction)) { + IndicesPermission.AuthorizedComponents authResult = predicate.test(indexAbstraction); + if (authResult != null && authResult != IndicesPermission.AuthorizedComponents.NONE) { indicesAndAliases.add(indexAbstraction.getName()); if (indexAbstraction.getType() == IndexAbstraction.Type.DATA_STREAM) { - // add data stream and its backing indices for any authorized data streams - for (Index index : indexAbstraction.getIndices()) { - indicesAndAliases.add(index.getName()); + if (authResult == IndicesPermission.AuthorizedComponents.ALL + || authResult == IndicesPermission.AuthorizedComponents.DATA) { + for (Index index : indexAbstraction.getIndices()) { + indicesAndAliases.add(index.getName()); + } } - // TODO: We need to limit if a data stream's failure indices should return here. - for (Index index : ((DataStream) indexAbstraction).getFailureIndices().getIndices()) { - indicesAndAliases.add(index.getName()); + + if (authResult == IndicesPermission.AuthorizedComponents.ALL + || authResult == IndicesPermission.AuthorizedComponents.FAILURES) { + for (Index index : ((DataStream) indexAbstraction).getFailureIndices().getIndices()) { + indicesAndAliases.add(index.getName()); + } } + } } } } else { for (IndexAbstraction indexAbstraction : lookup.values()) { - if (indexAbstraction.getType() != IndexAbstraction.Type.DATA_STREAM && predicate.test(indexAbstraction)) { + IndicesPermission.AuthorizedComponents authResult = predicate.test(indexAbstraction); + if (indexAbstraction.getType() != IndexAbstraction.Type.DATA_STREAM + && authResult != null + && authResult != IndicesPermission.AuthorizedComponents.NONE) { indicesAndAliases.add(indexAbstraction.getName()); } } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/AuthorizationServiceTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/AuthorizationServiceTests.java index c2e9a92e45353..44cfd6ee2dd8a 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/AuthorizationServiceTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/AuthorizationServiceTests.java @@ -987,7 +987,10 @@ public void testUnknownRoleCausesDenial() { ) ) ); - assertThat(securityException, throwableWithMessage(containsString("this action is granted by the index privileges [read,all]"))); + assertThat( + securityException, + throwableWithMessage(containsString("this action is granted by the index privileges [read,read_failures,all]")) + ); verify(auditTrail).accessDenied(eq(requestId), eq(authentication), eq(action), eq(request), authzInfoRoles(Role.EMPTY.names())); verifyNoMoreInteractions(auditTrail); @@ -1033,7 +1036,10 @@ public void testServiceAccountDenial() { throwableWithMessage(containsString("[" + action + "] is unauthorized for service account [" + serviceUser.principal() + "]")) ); verify(auditTrail).accessDenied(eq(requestId), eq(authentication), eq(action), eq(request), authzInfoRoles(role.names())); - assertThat(securityException, throwableWithMessage(containsString("this action is granted by the index privileges [read,all]"))); + assertThat( + securityException, + throwableWithMessage(containsString("this action is granted by the index privileges [read,read_failures,all]")) + ); verifyNoMoreInteractions(auditTrail); } @@ -1083,7 +1089,10 @@ public void testThatRoleWithNoIndicesIsDenied() { containsString("[" + action + "] is unauthorized" + " for user [test user]" + " with effective roles [no_indices]") ) ); - assertThat(securityException, throwableWithMessage(containsString("this action is granted by the index privileges [read,all]"))); + assertThat( + securityException, + throwableWithMessage(containsString("this action is granted by the index privileges [read,read_failures,all]")) + ); verify(auditTrail).accessDenied( eq(requestId), @@ -1536,7 +1545,10 @@ public void testDenialErrorMessagesForSearchAction() { assertThat(securityException, throwableWithMessage(containsString("other-4"))); assertThat(securityException, throwableWithMessage(not(containsString("all-1")))); assertThat(securityException, throwableWithMessage(not(containsString("read-2")))); - assertThat(securityException, throwableWithMessage(containsString(", this action is granted by the index privileges [read,all]"))); + assertThat( + securityException, + throwableWithMessage(containsString(", this action is granted by the index privileges [read,read_failures,all]")) + ); } public void testDenialErrorMessagesForBulkIngest() throws Exception { diff --git a/x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/privileges/11_builtin.yml b/x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/privileges/11_builtin.yml index d03e6925cab1f..aa11ed63602c7 100644 --- a/x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/privileges/11_builtin.yml +++ b/x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/privileges/11_builtin.yml @@ -16,4 +16,4 @@ setup: # I would much prefer we could just check that specific entries are in the array, but we don't have # an assertion for that - length: { "cluster" : 62 } - - length: { "index" : 22 } + - length: { "index" : 23 } From 3ac44e9c74e6ab992422ece38708be8c4c54d220 Mon Sep 17 00:00:00 2001 From: elasticsearchmachine Date: Fri, 10 Jan 2025 04:55:08 +0000 Subject: [PATCH 2/4] [CI] Auto commit changes from spotless --- .../xpack/core/security/authz/permission/Role.java | 1 - .../xpack/core/security/authz/privilege/IndexPrivilege.java | 4 +--- .../org/elasticsearch/integration/DataStreamSecurityIT.java | 3 ++- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/Role.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/Role.java index d86253a83be53..f52f8f85f006d 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/Role.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/Role.java @@ -9,7 +9,6 @@ import org.apache.lucene.util.automaton.Automaton; import org.elasticsearch.TransportVersion; -import org.elasticsearch.action.support.IndexComponentSelector; import org.elasticsearch.cluster.metadata.IndexAbstraction; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.util.set.Sets; 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 6cd9a5cb94af1..4c3ad50a705ff 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 @@ -83,9 +83,7 @@ public final class IndexPrivilege extends Privilege { ); // This is a special case: read_failures acts like `read` *only* for failure store indices in authorized data streams. // This internal action is not used, but having it makes automaton subset checks work as expected with this privilege. - private static final Automaton READ_FAILURES_AUTOMATON = patterns( - "internal:special/read_failures" - ); + private static final Automaton READ_FAILURES_AUTOMATON = patterns("internal:special/read_failures"); private static final Automaton READ_CROSS_CLUSTER_AUTOMATON = patterns( "internal:transport/proxy/indices:data/read/*", TransportClusterSearchShardsAction.TYPE.name(), diff --git a/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/DataStreamSecurityIT.java b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/DataStreamSecurityIT.java index 002371cdf65b1..11facebe446e1 100644 --- a/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/DataStreamSecurityIT.java +++ b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/DataStreamSecurityIT.java @@ -221,7 +221,8 @@ public void testFailureStoreAuthorziation() throws Exception { bulkRequest.add( new IndexRequest(dataStreamName).opType(DocWriteRequest.OpType.CREATE) .source("{\"code\": \"well this aint right\"}", XContentType.JSON), - new IndexRequest(dataStreamName).opType(DocWriteRequest.OpType.CREATE).source("{\"@timestamp\": \"2015-01-01T12:10:30Z\", \"code\": 404}", XContentType.JSON) + new IndexRequest(dataStreamName).opType(DocWriteRequest.OpType.CREATE) + .source("{\"@timestamp\": \"2015-01-01T12:10:30Z\", \"code\": 404}", XContentType.JSON) ); BulkResponse bulkResponse = adminClient.bulk(bulkRequest).actionGet(); assertThat(bulkResponse.getItems().length, equalTo(2)); From 47f150efc3b8e4fb4ef40a494438d4fd9715515e Mon Sep 17 00:00:00 2001 From: Athena Brown Date: Fri, 10 Jan 2025 17:41:19 -0700 Subject: [PATCH 3/4] Finish lining up the two kinds of predicate --- .../authz/permission/IndicesPermission.java | 84 +++++++++++++------ .../core/security/authz/permission/Role.java | 1 - .../authz/privilege/IndexPrivilege.java | 4 +- .../integration/DataStreamSecurityIT.java | 3 +- .../xpack/security/authz/RBACEngine.java | 24 ++---- .../accesscontrol/IndicesPermissionTests.java | 72 ++++++++++++---- 6 files changed, 123 insertions(+), 65 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java index 80a8646897c47..41c22f0264cde 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java @@ -74,12 +74,45 @@ public enum AuthorizedComponents { FAILURES, ALL; - public AuthorizedComponents and(AuthorizedComponents other) { + public AuthorizedComponents combine(AuthorizedComponents other) { + if (other == null) { + return this; + } return switch (this) { case ALL -> ALL; case NONE -> other; - case DATA -> other == FAILURES || other == ALL ? ALL : DATA; - case FAILURES -> other == DATA || other == ALL ? ALL : FAILURES; + case DATA -> other.isFailuresAuthorized() ? ALL : DATA; + case FAILURES -> other.isDataAuthorized() ? ALL : FAILURES; + }; + } + + /** + * @return True if data components are authorized for the resource in question + */ + public boolean isDataAuthorized() { + return switch (this) { + case ALL, DATA -> true; + case NONE, FAILURES -> false; + }; + } + + /** + * @return True if failure components are authorized for the resource in question + */ + public boolean isFailuresAuthorized() { + return switch (this) { + case ALL, FAILURES -> true; + case NONE, DATA -> false; + }; + } + + /** + * @return True if any components, data or failure, are authorized for the resource in question. Mostly for visibility checks. + */ + public boolean isAnyAuthorized() { + return switch (this) { + case ALL, DATA, FAILURES -> true; + case NONE -> false; }; } } @@ -175,16 +208,7 @@ public static class IsResourceAuthorizedPredicate { private final BiFunction biPredicate; - // public for tests - // public IsResourceAuthorizedPredicate(StringMatcher resourceNameMatcher, StringMatcher additionalNonDatastreamNameMatcher) { - // this((String name, @Nullable IndexAbstraction indexAbstraction) -> { - // assert indexAbstraction == null || name.equals(indexAbstraction.getName()); - // return resourceNameMatcher.test(name) - // || (isPartOfDatastream(indexAbstraction) == false && additionalNonDatastreamNameMatcher.test(name)); - // }); - // } - - private IsResourceAuthorizedPredicate(BiFunction biPredicate) { + public IsResourceAuthorizedPredicate(BiFunction biPredicate) { this.biPredicate = biPredicate; } @@ -195,20 +219,33 @@ private IsResourceAuthorizedPredicate(BiFunction this.biPredicate.apply(name, abstraction).and(other.biPredicate.apply(name, abstraction)) - ); + Objects.requireNonNull(other); + return new IsResourceAuthorizedPredicate((name, abstraction) -> { + AuthorizedComponents authResult = this.biPredicate.apply(name, abstraction); + // If we're only authorized for some components, other predicates might authorize us for the rest + return switch (authResult) { + case null -> other.biPredicate.apply(name, abstraction); + case NONE -> other.biPredicate.apply(name, abstraction); // Can't do worse than totally unauthorized, thank u NEXT + case ALL -> AuthorizedComponents.ALL; // Can't do better than ALL, so short circuit + case DATA, FAILURES -> authResult.combine(other.biPredicate.apply(name, abstraction)); + }; + }); } /** - * Verifies if access is authorized to the given {@param indexAbstraction} resource. + * Check which components of the given {@param indexAbstraction} resource is authorized. * The resource must exist. Otherwise, use the {@link #test(String, IndexAbstraction)} method. - * Returns {@code true} if access to the given resource is authorized or {@code false} otherwise. + * @return An object representing which components of this index abstraction the user is authorized to access */ - public final AuthorizedComponents test(IndexAbstraction indexAbstraction) { + public final AuthorizedComponents check(IndexAbstraction indexAbstraction) { return test(indexAbstraction.getName(), indexAbstraction); } + public final boolean test(IndexAbstraction indexAbstraction) { + AuthorizedComponents authResult = check(indexAbstraction); + return authResult != null && authResult.isDataAuthorized(); + } + /** * Verifies if access is authorized to the resource with the given {@param name}. * The {@param indexAbstraction}, which is the resource to be accessed, must be supplied if the resource exists or be {@code null} @@ -218,11 +255,6 @@ public final AuthorizedComponents test(IndexAbstraction indexAbstraction) { public AuthorizedComponents test(String name, @Nullable IndexAbstraction indexAbstraction) { return biPredicate.apply(name, indexAbstraction); } - - private static boolean isPartOfDatastream(IndexAbstraction indexAbstraction) { - return indexAbstraction != null - && (indexAbstraction.getType() == IndexAbstraction.Type.DATA_STREAM || indexAbstraction.getParentDataStream() != null); - } } /** @@ -551,7 +583,7 @@ private Map buildIndicesAccessC final Collection concreteIndices = resource.resolveConcreteIndices(lookup); for (Group group : groups) { - AuthorizedComponents authorizedComponents = group.allowedIndicesPredicate(action).test(resource.indexAbstraction); + AuthorizedComponents authorizedComponents = group.allowedIndicesPredicate(action).check(resource.indexAbstraction); // the group covers the given index OR the given index is a backing index and the group covers the parent data stream if (authorizedComponents != null && authorizedComponents != AuthorizedComponents.NONE) { granted = true; @@ -649,7 +681,7 @@ private boolean isActionGranted(final String action, final Map { final IndexAbstraction indexAbstraction = lookup.get(name); - if (indexAbstraction == null) { - // test access (by name) to a resource that does not currently exist - // the action handler must handle the case of accessing resources that do not exist - return predicate.test(name, null); - } else { - // We check the parent data stream first if there is one. For testing requested indices, this is most likely - // more efficient than checking the index name first because we recommend grant privileges over data stream - // instead of backing indices. - return (indexAbstraction.getParentDataStream() != null && predicate.test(indexAbstraction.getParentDataStream())) - || predicate.test(indexAbstraction); - } + return predicate.test(name, indexAbstraction).isAnyAuthorized(); }); } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/accesscontrol/IndicesPermissionTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/accesscontrol/IndicesPermissionTests.java index ac834911fc4e6..e01926d7bb3b5 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/accesscontrol/IndicesPermissionTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/accesscontrol/IndicesPermissionTests.java @@ -678,6 +678,7 @@ public void testIndicesPermissionHasFieldOrDocumentLevelSecurity() { assertThat(indicesPermission3.hasFieldOrDocumentLevelSecurity(), is(false)); } + // ATHE: Make sure this functionality is tested elsewhere public void testResourceAuthorizedPredicateForDatastreams() { String dataStreamName = "logs-datastream"; Metadata.Builder mb = Metadata.builder( @@ -704,28 +705,67 @@ public void testResourceAuthorizedPredicateForDatastreams() { IndexMetadata.builder("logs-index").settings(indexSettings(IndexVersion.current(), 1, 0)).putAlias(aliasMetadata).build() ) ); - IndicesPermission.IsResourceAuthorizedPredicate predicate = new IndicesPermission.IsResourceAuthorizedPredicate( - StringMatcher.of("other"), - StringMatcher.of(dataStreamName, backingIndex.getName(), concreteIndex.getName(), alias.getName()) - ); - assertThat(predicate.test(dataStream), is(false)); - // test authorization for a missing resource with the datastream's name - assertThat(predicate.test(dataStream.getName(), null), is(true)); - assertThat(predicate.test(backingIndex), is(false)); - // test authorization for a missing resource with the backing index's name - assertThat(predicate.test(backingIndex.getName(), null), is(true)); - assertThat(predicate.test(concreteIndex), is(true)); - assertThat(predicate.test(alias), is(true)); + // IndicesPermission.IsResourceAuthorizedPredicate predicate = new IndicesPermission.IsResourceAuthorizedPredicate( + // (String name, IndexAbstraction abstraction) -> { + // StringMatcher regularNames = StringMatcher.of("other"); + // StringMatcher nonDatastreamNames = StringMatcher.of( + // dataStreamName, + // backingIndex.getName(), + // concreteIndex.getName(), + // alias.getName() + // ); + // if (abstraction.getType() != IndexAbstraction.Type.DATA_STREAM && abstraction.getParentDataStream() == null) { + // return regularNames.test(name) + // ? IndicesPermission.AuthorizedComponents.DATA + // : IndicesPermission.AuthorizedComponents.NONE; + // } else { + // return nonDatastreamNames.test(name) + // ? IndicesPermission.AuthorizedComponents.DATA + // : IndicesPermission.AuthorizedComponents.NONE; + // } + // } + // ); + // assertThat(predicate.test(dataStream), is(false)); + // // test authorization for a missing resource with the datastream's name + // assertThat(predicate.test(dataStream.getName(), null), is(IndicesPermission.AuthorizedComponents.DATA)); + // assertThat(predicate.test(backingIndex), is(false)); + // // test authorization for a missing resource with the backing index's name + // assertThat(predicate.test(backingIndex.getName(), null), is(true)); + // assertThat(predicate.test(concreteIndex), is(true)); + // assertThat(predicate.test(alias), is(true)); } public void testResourceAuthorizedPredicateAnd() { IndicesPermission.IsResourceAuthorizedPredicate predicate1 = new IndicesPermission.IsResourceAuthorizedPredicate( - StringMatcher.of("c", "a"), - StringMatcher.of("b", "d") + (String name, IndexAbstraction abstraction) -> { + StringMatcher regularNames = StringMatcher.of("c", "a"); + StringMatcher nonDatastreamNames = StringMatcher.of("b", "d"); + if (abstraction.getType() != IndexAbstraction.Type.DATA_STREAM && abstraction.getParentDataStream() == null) { + return regularNames.test(name) + ? IndicesPermission.AuthorizedComponents.DATA + : IndicesPermission.AuthorizedComponents.NONE; + } else { + return nonDatastreamNames.test(name) + ? IndicesPermission.AuthorizedComponents.DATA + : IndicesPermission.AuthorizedComponents.NONE; + } + } ); IndicesPermission.IsResourceAuthorizedPredicate predicate2 = new IndicesPermission.IsResourceAuthorizedPredicate( - StringMatcher.of("c", "b"), - StringMatcher.of("a", "d") + (String name, IndexAbstraction abstraction) -> { + StringMatcher regularNames = StringMatcher.of("c", "b"); + StringMatcher nonDatastreamNames = StringMatcher.of("a", "d"); + if (abstraction.getType() != IndexAbstraction.Type.DATA_STREAM && abstraction.getParentDataStream() == null) { + return regularNames.test(name) + ? IndicesPermission.AuthorizedComponents.DATA + : IndicesPermission.AuthorizedComponents.NONE; + } else { + return nonDatastreamNames.test(name) + ? IndicesPermission.AuthorizedComponents.DATA + : IndicesPermission.AuthorizedComponents.NONE; + } + } + ); Metadata.Builder mb = Metadata.builder( DataStreamTestHelper.getClusterStateWithDataStreams( From 1982bf1260ec36f44fa717752a22a13984f3fa01 Mon Sep 17 00:00:00 2001 From: Athena Brown Date: Fri, 10 Jan 2025 18:29:47 -0700 Subject: [PATCH 4/4] Fixup the logging a bit + test fixes --- .../authz/permission/IndicesPermission.java | 62 +++++++++---------- .../accesscontrol/IndicesPermissionTests.java | 10 +++ 2 files changed, 40 insertions(+), 32 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java index 41c22f0264cde..2f1626c2798cd 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java @@ -41,6 +41,7 @@ import java.util.Objects; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.BiFunction; import java.util.function.Predicate; import java.util.function.Supplier; @@ -542,7 +543,7 @@ public IndicesAccessControl authorize( IndexComponentSelector selector = expressionAndSelector.v2() == null ? null : IndexComponentSelector.getByKey(expressionAndSelector.v2()); - // we need to consider the selector here? + final IndexResource resource = new IndexResource(indexOrAlias, lookup.get(indexOrAlias), selector); resources.put(resource.name, resource); totalResourceCount += resource.size(lookup); @@ -575,8 +576,6 @@ private Map buildIndicesAccessC final Map roleQueriesByIndex = Maps.newMapWithExpectedSize(totalResourceCount); final Set grantedResources = Sets.newHashSetWithExpectedSize(totalResourceCount); - final boolean isMappingUpdateAction = isMappingUpdateAction(action); - for (IndexResource resource : requestedResources.values()) { // true if ANY group covers the given index AND the given action boolean granted = false; @@ -672,23 +671,15 @@ private Map buildIndicesAccessC * If action is not granted for at least one resource, this method will return {@code false}. */ private boolean isActionGranted(final String action, final Map requestedResources) { - - final boolean isMappingUpdateAction = isMappingUpdateAction(action); - - // check failure store here too for (IndexResource resource : requestedResources.values()) { // true if ANY group covers the given index AND the given action boolean granted = false; for (Group group : groups) { AuthorizedComponents authResult = group.allowedIndicesPredicate(action).check(resource.indexAbstraction); - if (authResult != null && authResult != AuthorizedComponents.NONE) { - boolean actionCheck = group.checkAction(action); - // If action is granted we don't have to check for BWC and can stop at first granting group. - if (actionCheck) { - granted = true; - break; - } + if (authResult != null && authResult.isAnyAuthorized()) { + granted = true; + break; } } @@ -862,25 +853,32 @@ private BiFunction allowedIndice boolean actionMatches = actionMatcher.test(action); boolean actionAuthorized = actionMatches || (isReadAction && hasReadFailuresPrivilege); if (actionAuthorized == false) { + AtomicBoolean logged = new AtomicBoolean(false); return (name, resource) -> { - if (isMappingUpdateAction && hasMappingUpdateBwcPermissions && resource.getParentDataStream() == null) { - for (String privilegeName : this.privilege.name()) { - if (PRIVILEGE_NAME_SET_BWC_ALLOW_MAPPING_UPDATE.contains(privilegeName)) { - // ATHE: Does this log more often? - deprecationLogger.warn( - DeprecationCategory.SECURITY, - "[" + resource.getName() + "] mapping update for ingest privilege [" + privilegeName + "]", - "the index privilege [" - + privilegeName - + "] allowed the update " - + "mapping action [" - + action - + "] on index [" - + resource.getName() - + "], this privilege " - + "will not permit mapping updates in the next major release - users who require access " - + "to update mappings must be granted explicit privileges" - ); + if (isMappingUpdateAction + && hasMappingUpdateBwcPermissions + && resource != null + && resource.getParentDataStream() == null) { + boolean alreadyLogged = logged.getAndSet(true); + if (alreadyLogged == false) { + for (String privilegeName : this.privilege.name()) { + if (PRIVILEGE_NAME_SET_BWC_ALLOW_MAPPING_UPDATE.contains(privilegeName)) { + // ATHE: Does this log more often? + deprecationLogger.warn( + DeprecationCategory.SECURITY, + "[" + resource.getName() + "] mapping update for ingest privilege [" + privilegeName + "]", + "the index privilege [" + + privilegeName + + "] allowed the update " + + "mapping action [" + + action + + "] on index [" + + resource.getName() + + "], this privilege " + + "will not permit mapping updates in the next major release - users who require access " + + "to update mappings must be granted explicit privileges" + ); + } } } return AuthorizedComponents.ALL; diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/accesscontrol/IndicesPermissionTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/accesscontrol/IndicesPermissionTests.java index e01926d7bb3b5..15192c1f14fab 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/accesscontrol/IndicesPermissionTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/accesscontrol/IndicesPermissionTests.java @@ -580,6 +580,11 @@ public void testAuthorizationForMappingUpdates() { + "] on " + "index [test_write1], this privilege will not permit mapping updates in the next major release - " + "users who require access to update mappings must be granted explicit privileges", + "the index privilege [write] allowed the update mapping action [" + + TransportPutMappingAction.TYPE.name() + + "] on " + + "index [test1], this privilege will not permit mapping updates in the next major release - " + + "users who require access to update mappings must be granted explicit privileges", "the index privilege [write] allowed the update mapping action [" + TransportPutMappingAction.TYPE.name() + "] on " @@ -602,6 +607,11 @@ public void testAuthorizationForMappingUpdates() { + TransportAutoPutMappingAction.TYPE.name() + "] on " + "index [test1], this privilege will not permit mapping updates in the next major release - " + + "users who require access to update mappings must be granted explicit privileges", + "the index privilege [index] allowed the update mapping action [" + + TransportAutoPutMappingAction.TYPE.name() + + "] on " + + "index [test_write1], this privilege will not permit mapping updates in the next major release - " + "users who require access to update mappings must be granted explicit privileges" );