Fix Security's expression resolver to not remove unavailable but authorized names #92625

albertzaharovits · 2023-01-03T11:01:05Z

For explicit (non-wildcard) names in expressions from requests with the ignoreUnavailable request option set to true, the Security filter removes names for "unavailable" resources. This behavior is theoretically correct, but in practice the cluster state might not be recovered at the point in time when the Security filter does the rewrite (this causes problems, see #90215); In any case, the logic to remove "unavailable" names is unnecessarily duplicated from Core's expression resolver.

This PR makes the expression resolver in the Security filter to NOT:

remove missing but authorized names (indices, aliases, datastreams)
remove authorized datastreams if context disallows datastreams
remove backing indices of authorized datastreams if context disallows datastreams

It will only remove names of unauthorized resources, and let the unavailable names go through to be handled (to be ignored or throw "not found" exception) by the expression resolver in Core.

Fixes: #90215

This reverts commit 2b5a075.

This reverts commit dfabeab.

albertzaharovits · 2023-01-05T14:20:15Z

...y/src/internalClusterTest/java/org/elasticsearch/xpack/security/authz/IndexAliasesTests.java

+                .setIndicesOptions(IndicesOptions.lenientExpandOpen())::get,
+            GetAliasesAction.NAME,
+            "aliases_only"
+        );


The ignore_unavailable request option does not currently work for "unavailable" aliases, see:

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

Line 375 in 844bb4c

private static List<String> replaceWildcardsWithAuthorizedAliases(String[] aliases, List<String> authorizedAliases) {

.

It worked in this particular test by coincidence.
In this particular test the test_1 index is authorized but non-existent, so the request was previously authorized after the request was rewritten to empty. But now this is not the case anymore.
test_1 is authorized, so it is not considered "unavailable" from Security's POV,

From end-user's perspective, strictly speaking, this is a slight behaviour change:

Status code from 404 to 403

Response content from containing found aliases (and missing aliases) to unauthorized error response

Just want to call it out explicitly. But I don't think it is a real concern because the existing response is not a 200 anyway and it feels like an edge case.

Yes, it's indeed a behavior change for an edge case.

Rereading this now, after a week, I realise I haven't done a good job explaining what's going on here.
Let me try again:

The get aliases action in this test uses the "lenient expand open" request option, which means that missing explicit indices are ignored (and that wildcards can expand to empty).
The test_1 index does not exist, and it is the only index in the request, so when retrieving the non_authorized alias for it, because of the request option, this previously returned a 200 empty result, because there were ultimately no indices to return the aliases for.
But, after the change, Security does not care if test_1 exists or not (Core does, and if Security lets the request through, given the request option, it will return the empty result). For Security, the test_1 is authorized, but the alias non_authorized is not, hence it will then return a 403 because of the alias. For indices, the request option ignore_unavailable silently discards unauthorized index names, but the option never discards unauthorized aliases (but maybe it should?).

Therefore, clients will notice behavior differences for this API, when all the indices are authorized but missing, the request option tells to ignore missing indices, and there are unauthorized aliases (missing or not). Before they got a 200 empty, after they get a 403.
They got

Before they got a 200 empty, after they get a 403.

Does it return 200 today? At REST layer, the response code is set to 404 if there is any explicitly requested alias not found.

elasticsearch/server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetAliasesAction.java

Lines 130 to 133 in c0e702c

if (missingAliases.isEmpty()) {

status = RestStatus.OK;

} else {

status = RestStatus.NOT_FOUND;

You're correct. It does return 404, in the existing code, with Security enabled, when user is only authorized for the test_1 index that does not exist, but it is not authorized for the non_authorized alias.

albertzaharovits · 2023-01-05T14:47:36Z

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

        }, name -> {
            final IndexAbstraction indexAbstraction = lookup.get(name);
            if (indexAbstraction == null) {
-                return false;
-            }
-            if (includeDataStreams) {
+                // missing but authorized resources should be handled downstream in the action handler, not here in the Security filter
+                return predicate.testMissingResource(name);
+            } 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.
-                if (indexAbstraction.getParentDataStream() != null && predicate.test(indexAbstraction.getParentDataStream())) {
-                    return true;
-                } else {
-                    return predicate.test(indexAbstraction);
-                }
-            } else {
-                return indexAbstraction.getType() != IndexAbstraction.Type.DATA_STREAM && predicate.test(indexAbstraction);
+                return (indexAbstraction.getParentDataStream() != null && predicate.test(indexAbstraction.getParentDataStream()))
+                    || predicate.test(indexAbstraction);
            }


This is the core of the change proposed by this PR.

This predicate here is used to remove explicit names when rewriting the request if ignore_unavailable is true, see:

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

Line 112 in 844bb4c

} else if (indicesOptions.ignoreUnavailable() == false || isAuthorized.test(indexAbstraction)) {

After this expression rewrite, the request goes into the authz stage, where every expression item is checked again:

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

Line 510 in 844bb4c

private boolean isActionGranted(final String action, final Map<String, IndexResource> requestedResources) {

The intent is that the test here, ie:

final IndexAbstraction indexAbstraction = lookup.get(name); if (indexAbstraction == null) { // missing but authorized resources should be handled downstream in the action handler, not here in the Security filter return predicate.testMissingResource(name); } 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); }

be equivalent to the test in IndicesPermission#isActionGranted, such that only unauthorized names be removed when rewriting the expression with ignore_unavailable=true.

Can we unify the loigc of test and testMissingResource into a single method? It is better that callers do not have to worry about deciding on which method to use. It should be possible if the predicate takes an object that encapsulate both name and indexAbstraction, something similar to IndicesPermission#IndexResource.

I think ideally it would be great to reuse the existing IndexResource and unify the logic between this method and isActionGranted. It will likely help understand this part of code. But it does not seem to be an easy job. So we can tackle it later gradually. But I think we can avoid having both test and testMissingResource as suggest earlier. It also helps the IsResourceAuthorizedPredicate to be just a simple Predicate and avoid having to implement its own and method.

There's a subtle problem with making the IsResourceAuthorizedPredicate take an IndexResource parameter.
The problem is that, when the resource exists, it matters if the "is resource authorized" predicate is called in the context of wildcard expansion, or in the context of checking explicit names.

The difference is that the predicate in the context of checking explicit names first tests on the parent datastream (if applicable) and then it tests on the resource itself. This is not the case for the predicate in the wildcard expansion context.

So, in essence, the IsResourceAuthorizedPredicate would still have to implement two slightly different test methods.

Not exactly what you asked for, but I could be hidding this whole

final IndexAbstraction indexAbstraction = lookup.get(name); if (indexAbstraction == null) { // missing but authorized resources should be handled downstream in the action handler, not here in the Security filter return predicate.testMissingResource(name); } 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); }

block inside a method in the IsResourceAuthorizedPredicate class, something of the sorts of #testExplicitName. This should be equivalent (and someday replace) the isActionGranted method.
What do you think?

My preference is to avoid having a second test method for IsResourceAuthorizedPredicate. It is surprising that a predicate has more than one test method and you need some effort in deciding which one to use.

Based on your comment here #92625 (comment)

Can we make IsResourceAuthorizedPredicate implement BiPredicate if you prefer not having a IndexResource like class?

As we've discussed earlier today, I've reimplemented the IsResourceAuthorizedPredicate as a BiPredicate<String, IndexAbstraction>, see 878ba9e :

public static class IsResourceAuthorizedPredicate implements BiPredicate<String, IndexAbstraction> { private final BiPredicate<String, IndexAbstraction> 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<String, IndexAbstraction> biPredicate) { this.biPredicate = biPredicate; } public final IsResourceAuthorizedPredicate and(IsResourceAuthorizedPredicate other) { return new IsResourceAuthorizedPredicate(this.biPredicate.and(other.biPredicate)); } public final boolean test(IndexAbstraction indexAbstraction) { return test(indexAbstraction.getName(), indexAbstraction); } @Override public boolean test(String name, @Nullable IndexAbstraction indexAbstraction) { return biPredicate.test(name, indexAbstraction); } private static boolean isPartOfDatastream(IndexAbstraction indexAbstraction) { return indexAbstraction != null && (indexAbstraction.getType() == IndexAbstraction.Type.DATA_STREAM || indexAbstraction.getParentDataStream() != null); } }

For convenience, I also added a test(IndexAbstraction indexAbstraction) in addition to test(String name, @Nullable IndexAbstraction indexAbstraction), to avoid the test(index.getName(), index) pattern.

However, I'm hesitant to change the predicate further, in order to move the logic that tests the parent datastream of a backing index. I still can't see any serious issues, but I very much prefer that such a change be in a separate PR.

OK let's keep it separate. Thanks for consideration.

albertzaharovits · 2023-01-05T14:49:28Z

...n/eql/qa/security/src/javaRestTest/java/org/elasticsearch/xpack/eql/EqlRestValidationIT.java

+    }
+
+    @Override
+    protected String getInexistentWildcardErrorMessage() {


Some EQL tests assert the audit log messages, and some will change because the transport request, which gets audited, is not rewritten anymore to erase authorized but unavailable indices.

elasticsearchmachine · 2023-01-05T14:51:10Z

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

elasticsearchmachine · 2023-01-05T14:51:10Z

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

albertzaharovits · 2023-01-16T16:28:42Z

@jakelandis and @ywangd Thank you for the review, I acknowledge it must've been laborious.
I answered all your points, but I haven't pushed any code changes.
@ywangd We probably need to sync on the IsResourceAuthorizedPredicate's interface, because I'm undecided on the approach, but we can also figure it out over GH too.

albertzaharovits · 2023-01-18T14:42:13Z

@elasticmachine update branch

albertzaharovits · 2023-01-18T16:51:40Z

@elasticmachine update branch

albertzaharovits · 2023-01-18T17:39:37Z

@elasticmachine update branch

jakelandis

LGTM , but would appreciate a second set of eyes for final approval.

ywangd

LGTM

A few minor comments for your consideration.

.../src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java

ywangd · 2023-01-19T01:38:57Z

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

        }, name -> {
            final IndexAbstraction indexAbstraction = lookup.get(name);
            if (indexAbstraction == null) {
-                return false;
-            }
-            if (includeDataStreams) {
+                // missing but authorized resources should be handled downstream in the action handler, not here in the Security filter
+                return predicate.testMissingResource(name);
+            } 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.
-                if (indexAbstraction.getParentDataStream() != null && predicate.test(indexAbstraction.getParentDataStream())) {
-                    return true;
-                } else {
-                    return predicate.test(indexAbstraction);
-                }
-            } else {
-                return indexAbstraction.getType() != IndexAbstraction.Type.DATA_STREAM && predicate.test(indexAbstraction);
+                return (indexAbstraction.getParentDataStream() != null && predicate.test(indexAbstraction.getParentDataStream()))
+                    || predicate.test(indexAbstraction);
            }


OK let's keep it separate. Thanks for consideration.

...ity/src/test/java/org/elasticsearch/xpack/security/authz/IndicesAndAliasesResolverTests.java

...c/test/java/org/elasticsearch/xpack/security/authz/accesscontrol/IndicesPermissionTests.java

albertzaharovits · 2023-01-19T13:55:29Z

https://gradle-enterprise.elastic.co/s/73ulifdoml74q tracked by #91450

albertzaharovits · 2023-01-19T13:55:41Z

@elasticmachine run elasticsearch-ci/part-2

Create IsAuthorizedPredicate

f68436c

albertzaharovits self-assigned this Jan 3, 2023

elasticsearchmachine added the v8.7.0 label Jan 3, 2023

Use IsAuthorizedPredicate

ce1e4d2

albertzaharovits force-pushed the fix-authz-unavailable-idx branch from 112f864 to ce1e4d2 Compare January 3, 2023 11:03

albertzaharovits added 3 commits January 3, 2023 15:56

Nit

ff50f52

Rename

12b09a3

Refactor resolver predicate

6c116f0

albertzaharovits force-pushed the fix-authz-unavailable-idx branch from a8954ee to 6c116f0 Compare January 3, 2023 17:07

albertzaharovits added 9 commits January 4, 2023 14:18

Fix tests

d72e2c3

Fix IndexAliasesTests

dfabeab

Spotless

2b5a075

Revert "Spotless"

268af93

This reverts commit 2b5a075.

Revert "Fix IndexAliasesTests"

40742e9

This reverts commit dfabeab.

Different fix for IndexAliasesTests

e6181e4

Fix audit assert test

b5fb2ee

Fix EqlRestValidationIT

1172505

Predicate tests

519850d

albertzaharovits changed the title ~~Fix authz unavailable idx~~ Fix Security's expression resolver to not remove unavailable but authorized names Jan 5, 2023

albertzaharovits commented Jan 5, 2023

View reviewed changes

albertzaharovits requested review from jakelandis and ywangd January 5, 2023 14:50

albertzaharovits added :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC >bug labels Jan 5, 2023

albertzaharovits marked this pull request as ready for review January 5, 2023 14:50

elasticsearchmachine added the Team:Security Meta label for security team label Jan 5, 2023

albertzaharovits added 2 commits January 16, 2023 16:31

Comment

03e7cfb

\n in error message

a23d03c

Spotless

696af03

albertzaharovits requested review from jakelandis and ywangd January 16, 2023 16:31

albertzaharovits added 4 commits January 18, 2023 14:30

IsResourceAuthorizedPredicate

878ba9e

Merge branch 'main' into fix-authz-unavailable-idx

4c6507c

Spotless

e565948

Javadoc

43778bc

Merge branch 'main' into fix-authz-unavailable-idx

1e728ba

Merge branch 'main' into fix-authz-unavailable-idx

1fe1a19

Merge branch 'main' into fix-authz-unavailable-idx

3b67ff9

albertzaharovits mentioned this pull request Jan 18, 2023

Resolve just authorized indices #91557

Closed

jakelandis approved these changes Jan 18, 2023

View reviewed changes

ywangd approved these changes Jan 19, 2023

View reviewed changes

albertzaharovits added 6 commits January 19, 2023 09:26

Merge branch 'main' into fix-authz-unavailable-idx

94b8c3c

IsResourceAuthorizedPredicate override for "and" method

ace99f9

Nit

a4eb493

Nit

3b1a777

nit

b8cd97e

Merge branch 'main' into fix-authz-unavailable-idx

f8a10be

albertzaharovits mentioned this pull request Jan 19, 2023

Only run the authz check on non-wildcard names #93085

Open

albertzaharovits merged commit 54a78c3 into elastic:main Jan 19, 2023

albertzaharovits deleted the fix-authz-unavailable-idx branch January 19, 2023 14:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Security's expression resolver to not remove unavailable but authorized names #92625

Fix Security's expression resolver to not remove unavailable but authorized names #92625

albertzaharovits commented Jan 3, 2023 •

edited

Loading

albertzaharovits Jan 5, 2023

ywangd Jan 16, 2023

albertzaharovits Jan 16, 2023

ywangd Jan 17, 2023

albertzaharovits Jan 18, 2023

albertzaharovits Jan 5, 2023

ywangd Jan 16, 2023

albertzaharovits Jan 16, 2023

ywangd Jan 17, 2023

albertzaharovits Jan 18, 2023

albertzaharovits Jan 18, 2023

ywangd Jan 19, 2023

albertzaharovits Jan 5, 2023

elasticsearchmachine commented Jan 5, 2023

elasticsearchmachine commented Jan 5, 2023

albertzaharovits commented Jan 16, 2023

albertzaharovits commented Jan 18, 2023

albertzaharovits commented Jan 18, 2023

albertzaharovits commented Jan 18, 2023

jakelandis left a comment

ywangd left a comment

ywangd Jan 19, 2023

albertzaharovits commented Jan 19, 2023

albertzaharovits commented Jan 19, 2023

	if (missingAliases.isEmpty()) {
	status = RestStatus.OK;
	} else {
	status = RestStatus.NOT_FOUND;

Fix Security's expression resolver to not remove unavailable but authorized names #92625

Fix Security's expression resolver to not remove unavailable but authorized names #92625

Conversation

albertzaharovits commented Jan 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticsearchmachine commented Jan 5, 2023

elasticsearchmachine commented Jan 5, 2023

albertzaharovits commented Jan 16, 2023

albertzaharovits commented Jan 18, 2023

albertzaharovits commented Jan 18, 2023

albertzaharovits commented Jan 18, 2023

jakelandis left a comment

Choose a reason for hiding this comment

ywangd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertzaharovits commented Jan 19, 2023

albertzaharovits commented Jan 19, 2023

albertzaharovits commented Jan 3, 2023 •

edited

Loading