Only expand authorized indices for requests with wildcards #72118

albertzaharovits · 2021-04-22T17:21:15Z

This is a performance improvement that avoids unnecessarily expanding the list of "authorized indices" while authorizing requests that do not contain wildcards.

This PR, as it currently stands, avoids it for all request types. It can be made simpler, see #72118 (comment) to limit it so it only avoids expansions for bulk ingest requests.

The objectives are:

Do not change the AuthorizationEngine interface.
Do not fork from transport thread when iterating over all the indices in the cluster.
Do not change the authorization algorithm.

BUT, this requires passing the async supplier more deeply to the callees, which greatly complicates the algorithms (think loops - it is a mess).

albertzaharovits · 2021-04-24T19:13:21Z

SocketTimeout @elasticmachine run elasticsearch-ci/1

EDIT: it was actually legit failure in this PR

albertzaharovits · 2021-04-25T08:42:20Z

server/src/main/java/org/elasticsearch/common/util/AsyncSupplier.java

+            }).addListener(listener, EsExecutors.newDirectExecutorService());
+        }
+    }
+}


This was a nested class inside the AuthorizationEngine, but because it is now passed around to more classes, I had to pull it up somewhere.
I also improved the threading of it, as it was harder to reason about the corner cases than to fix it.

albertzaharovits · 2021-04-25T08:43:48Z

...ugin/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationService.java

-                    authzIndicesListener));
-            final AsyncSupplier<ResolvedIndices> resolvedIndicesAsyncSupplier = new CachingAsyncSupplier<>((resolvedIndicesListener) -> {
-                authorizedIndicesSupplier.getAsync(ActionListener.wrap(authorizedIndices -> {
-                    resolveIndexNames(request, metadata, authorizedIndices, resolvedIndicesListener);


This is the core part of this PR!
We don't have to call authzEngine.loadAuthorizedIndices for all indicesAndAliasesResolver.resolve invocations.
The authorized list is only required if the request contains wildcards.

albertzaharovits · 2021-04-25T08:45:00Z

...ugin/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationService.java

            authzEngine.authorizeIndexAction(requestInfo, authzInfo, resolvedIndicesAsyncSupplier,
                metadata.getIndicesLookup(), wrapPreservingContext(new AuthorizationResultListener<>(result ->
-                    handleIndexActionAuthorizationResult(result, requestInfo, requestId, authzInfo, authzEngine, authorizedIndicesSupplier,
+                    handleIndexActionAuthorizationResult(result, requestInfo, requestId, authzInfo, authzEngine,
                        resolvedIndicesAsyncSupplier, metadata, listener),


resolvedIndicesAsyncSupplier is all that's needed, authorizedIndicesSupplier is a superset of it.

albertzaharovits · 2021-04-25T08:45:57Z

...ugin/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationService.java

-        final AuditTrail auditTrail = auditTrailService.get();
-
-        authorizedIndicesSupplier.getAsync(ActionListener.wrap(authorizedIndices -> {
-            resolvedIndicesAsyncSupplier.getAsync(ActionListener.wrap(overallResolvedIndices -> {


overallResolvedIndices is a subset of authorizedIndices, they're not both required.

albertzaharovits · 2021-04-25T08:50:03Z

...ugin/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationService.java

+            for (BulkItemRequest item : request.items()) {
+                String resolvedIndex = resolvedIndexNames.computeIfAbsent(item.index(), key -> {
+                    final ResolvedIndices resolvedIndices =
+                            indicesAndAliasesResolver.resolveIndicesAndAliases(item.request(), metadata, localIndices);


It would be utterly mucky to refactor this to use the "async" variant, the structure of the code is really not amenable to listeners.
Instead, we assume that the indicesAndAliasesResolver.resolveIndicesAndAliases is not in fact async if the authorized indices supplier is not async. This assumption is within our control (but it's not the most elegant thing, I admit).

albertzaharovits · 2021-04-25T08:52:10Z

...security/src/main/java/org/elasticsearch/xpack/security/authz/IndicesAndAliasesResolver.java


-    ResolvedIndices resolveIndicesAndAliases(IndicesRequest indicesRequest, Metadata metadata, List<String> authorizedIndices) {


This got changed a lot in order to support the AsyncSupplier<List<String>> instead of List<String>, and it is the hardest thing to review in this PR.

albertzaharovits · 2021-04-25T08:53:11Z

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

@@ -546,7 +546,7 @@ GetUserPrivilegesResponse buildUserPrivilegesResponseObject(Role userRole) {
        return Collections.unmodifiableList(new ArrayList<>(indicesAndAliases));
    }

-    private void buildIndicesAccessControl(Authentication authentication, String action,


Unused variable.

albertzaharovits · 2021-04-25T09:09:29Z

tests passed @elasticmachine test this please

albertzaharovits · 2021-04-25T09:21:44Z

...security/src/main/java/org/elasticsearch/xpack/security/authz/IndicesAndAliasesResolver.java

+        PlainActionFuture<ResolvedIndices> future = PlainActionFuture.newFuture();
+        // if the supplier is not async, the method is not async, hence get is non-blocking
+        resolve(request, metadata, listener -> listener.onResponse(authorizedIndices), future);
+        return FutureUtils.get(future, 0, TimeUnit.MILLISECONDS);


Sort of a hack:
If the authorized supplier is not in fact async (listener -> listener.onResponse(authorizedIndices)) we know (assume) that the resolve invocation is going to be sync. Hence, we can call FutureUtils.get. But this calls the timeout variant, because the get without timeout variant asserts that the calling thread supports blocking (which it usually doesn't)

albertzaharovits · 2021-04-25T10:50:49Z

tests passed @elasticmachine test this please

albertzaharovits · 2021-04-25T17:11:38Z

Raised #72194
@elasticmachine run elasticsearch-ci/2

albertzaharovits · 2021-04-25T17:35:32Z

...security/src/main/java/org/elasticsearch/xpack/security/authz/IndicesAndAliasesResolver.java

+                    ResolvedIndices.Builder resolvedIndicesBuilder = new ResolvedIndices.Builder();
+                    // cannot pass the async supplier
+                    List<String> replaced = indexAbstractionResolver.resolveIndexAbstractions(split.getLocal(), indicesOptions, metadata,
+                            authorizedIndices, replaceWildcards, indicesRequest.includeDataStreams());


There's an interesting point to be made here.
indexAbstractionResolver.resolveIndexAbstractions can be made to work without the authorizedIndices list expanded, BUT, because it has to iterate over the split.getLocal() list in order, it will do it ON THE STACK. Do we have a stack for all the request index parts?

I've pushed c2466b8 to make indexAbstractionResolver.resolveIndexAbstractions async.

albertzaharovits · 2021-04-25T18:53:26Z

tests passed @elasticmachine test this please

tvernum · 2021-04-26T03:55:11Z

Do not change the AuthorizationEngine interface.

If it helps, you can drop that condition.
There's no problem changing the interface is it's necessary to make this sort of performance change.

albertzaharovits · 2021-04-26T04:42:58Z

Do not change the AuthorizationEngine interface.

If it helps, you can drop that condition.
There's no problem changing the interface is it's necessary to make this sort of performance change.

Thanks Tim!
Yes that would help. I will raise another PR where instead of an "authorized list" there would be an "authorized predicate".
All the authz algorithms have access to the the Metadata already, and they can use that together with the predicate to achieve an equivalent "authorized list".
I think this change would be much much simpler.

This PR is ready for review, I won't work on it further, but I expect the follow-up to be preferable in comparison. I've added you as a reviewer.

One question, though, but we can defer it when both PRs are on the table, should we backport this to 6.8 ? I really hoped this variant would be simpler to code and thought it had the best chances to be backported. I'm gonna keep backporting as a semi-objective for the follow-up as well.

tvernum · 2021-05-14T06:25:02Z

should we backport this to 6.8 ?

I don't think so. This isn't really a bug, and I don't think it's worth the risk of trying to backport in a patch release.

repantis · 2021-07-21T20:58:25Z

Hi @albertzaharovits @tvernum thanks a lot for working on this performance improvement. It came up in the context of improvements to reduce support load. Is the work in this PR and associated PRs still planned?

tvernum · 2021-07-21T23:39:37Z

It's still on our task list, but we don't have the cycles to do anything with it right now.

repantis · 2021-07-23T03:23:14Z

Got it, Tim, thank you for explaining.

albertzaharovits · 2022-01-25T21:33:59Z

Superseded by #81237

Avoid passing authorizedIndices together with resolvedIndices

42922ed

albertzaharovits added the :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC label Apr 22, 2021

albertzaharovits self-assigned this Apr 22, 2021

albertzaharovits added 12 commits April 22, 2021 20:21

Merge branch 'master' into avoid_authz_expand_for_bulks

7daff92

Continued

dc59ac1

Noop

538da36

nitty nit

080071b

Fancy async supplier

0dd0e34

Move CachingAsyncSupplier

c2e7587

Merge branch 'master' into avoid_authz_expand_for_bulks

fc8415e

Refactor resolveIndicesAndAliases to make it amenable for async

ae35032

Noop getPutMappingIndexOrAlias refactor for async

d2b6d40

Done

53069f8

checkstyle

019583b

Polish

726c957

albertzaharovits changed the title ~~Only expand authorized indices for requests with wildcards~~ Do not expand authorized indices for bulk shard requests Apr 24, 2021

albertzaharovits added 2 commits April 24, 2021 23:22

Remote still to do

c1f0edb

the trappy fixes

9c3bc7d

albertzaharovits commented Apr 25, 2021

View reviewed changes

Async indexAbstractionResolver.resolveIndexAbstractions

c2466b8

albertzaharovits changed the title ~~Do not expand authorized indices for bulk shard requests~~ Only expand authorized indices for requests with wildcards Apr 25, 2021

albertzaharovits added the >enhancement label Apr 25, 2021

checkstyle

0435d04

albertzaharovits requested a review from tvernum April 26, 2021 04:43

albertzaharovits mentioned this pull request May 5, 2021

Make RBACEngine.resolveAuthorizedIndicesFromRole Return Set for Lookups #72598

Merged

ywangd mentioned this pull request Sep 29, 2021

Skip loading authorized indices if requests do not need them #78321

Merged

albertzaharovits closed this Jan 25, 2022


		ResolvedIndices resolveIndicesAndAliases(IndicesRequest indicesRequest, Metadata metadata, List<String> authorizedIndices) {

Only expand authorized indices for requests with wildcards #72118

Only expand authorized indices for requests with wildcards #72118

Uh oh!

Conversation

albertzaharovits commented Apr 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The objectives are:

Uh oh!

albertzaharovits commented Apr 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

albertzaharovits Apr 25, 2021

Choose a reason for hiding this comment

Uh oh!

albertzaharovits Apr 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

albertzaharovits Apr 25, 2021

Choose a reason for hiding this comment

Uh oh!

albertzaharovits Apr 25, 2021

Choose a reason for hiding this comment

Uh oh!

albertzaharovits Apr 25, 2021

Choose a reason for hiding this comment

Uh oh!

albertzaharovits Apr 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

albertzaharovits Apr 25, 2021

Choose a reason for hiding this comment

Uh oh!

albertzaharovits commented Apr 25, 2021

Uh oh!

albertzaharovits Apr 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

albertzaharovits commented Apr 25, 2021

Uh oh!

albertzaharovits commented Apr 25, 2021

Uh oh!

albertzaharovits Apr 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

albertzaharovits Apr 25, 2021

Choose a reason for hiding this comment

Uh oh!

albertzaharovits commented Apr 25, 2021

Uh oh!

tvernum commented Apr 26, 2021

Uh oh!

albertzaharovits commented Apr 26, 2021

Uh oh!

tvernum commented May 14, 2021

Uh oh!

repantis commented Jul 21, 2021

Uh oh!

tvernum commented Jul 21, 2021

Uh oh!

repantis commented Jul 23, 2021

Uh oh!

albertzaharovits commented Jan 25, 2022

Uh oh!

Uh oh!

albertzaharovits commented Apr 22, 2021 •

edited

Loading

albertzaharovits commented Apr 24, 2021 •

edited

Loading

albertzaharovits Apr 25, 2021 •

edited

Loading

albertzaharovits Apr 25, 2021 •

edited

Loading

albertzaharovits Apr 25, 2021 •

edited

Loading

albertzaharovits Apr 25, 2021 •

edited

Loading