More avoidance for loadAuthorizedIndices #81237

ywangd · 2021-12-02T05:36:00Z

This PR replaces the concrete list of authorizeIndices with a lazily load class
so that loading is not triggered if not necessary.

For example, a search targeting a concret name, GET index/_search no longer
triggers the loading. But names with wildcard will still trigger the loading.

However if we organize the indices with data streams and alias, it is possible
to achieve similar wildcard effect while still avoid the loading. That is,
searches like GET alias/_search or GET data_stream/_search will not
trigger the loading even they target multiple indices behind the single name.

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

ywangd · 2021-12-08T00:56:57Z

@elasticmachine update branch

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

...ugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/AuthorizationEngine.java

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

albertzaharovits · 2022-01-05T16:58:15Z

I think delaying evaluating the authorizedIndices will turn out to be a great optimization in practice because many requests do not contain wildcards.
I'm not sure about the switch to a predicate though (#81237 (comment)) . I need to think it through a bit more, but maybe you can clarify your thoughts about this particular change.

albertzaharovits · 2022-01-05T16:58:38Z

TBC I'm generally in favor of this change.

…-authorized-indices

ywangd · 2022-01-24T03:36:03Z

@albertzaharovits I made a rather big change based on the discussion. It is still along the agreement we reached last time and overall a simplication. Instead of changing the signatures of existing Classes (AuthorizationEngine, IndexAbstractionResolver etc), the PR now introduces a new Set implemetation which lazily builds the authorized indices when necessary, but otherwise uses predicate for contains test. So the idea does not change, but the changeset is much smaller. Here is a summary for how it should work for different scenarios:

Concrete names, e.g. index1, or index1,index2,index3. Performance should be much better because the predicate testing for the individual names should much faster than loading all authorized indices. [1]
Simple wildcard, e.g. *, or logs-*,metrics-*. No change to performance because the authorizedIndices set will just be built when there are genuine wildcard expansion.
The more interesting case is a mix of concrete names and wildcards, e.g. index1,logs-*. The performance could be slightly worse because the code first runs the predicate for index1 and then still needs to load authorizedIndices for logs-*. So the performance lost is one additional predicate execution. Obviously, the performance can keep degrade with longer list of concrete index names. [2]

I think the change should have a postive impact on performance overall because above Scenario 1 is very common and the potential gain is significant (especially when the cluster is large in size). Scenario 3 does have certain performance penalty but it is mitigated by (1) We can advise users to list wildcard patterns first in the requested names; (2) It is unlikely for the list of concrete names to be very long which means the performance drop should not be anyway significant (compared to loading authorizedIndices).

[1] An edge case of this scenario is that all requested names are for backing indices and the user does not have privileges over data stream. In this case, the index pattern predicate can run twice for each give name (first for the data stream name, a second time for the backing index name). However, it still unlikely an issue because (1) it still saves time in loading authorized indices which is likely much more expensive than running the predicate couple more times; (2) We do not recommend granting privileges only over the backing indices. If the user does not have privileges over the backing indices either, the request will end up be a 403 failure and performance is not critical in the failure case.

[2] We can avoid the additional predicate executions by looping through the requested names earlier and find out whether it has any wildcard. However, this loop itself also has its cost. And if the names are all concrete, it ends up doing useless work. In short, it is still a tradeoff between different scenarios.

elasticmachine · 2022-01-24T03:36:16Z

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

albertzaharovits · 2022-01-25T16:34:42Z

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

-            for (IndexAbstraction indexAbstraction : lookup.values()) {
-                if (indexAbstraction.getType() != IndexAbstraction.Type.DATA_STREAM && predicate.test(indexAbstraction)) {
-                    indicesAndAliases.add(indexAbstraction.getName());
+            final Set<String> authorizedIndices = Collections.unmodifiableSet(indicesAndAliases);


The AuthorizedIndicesSet should protect against modification to this, so maybe avoid the wrapping in an unmodifiable set.

Yes it is not strictly necessary. The Set is passed into timeChecker.accept in the next line. I was pretending that if we don't know anything about timeChecker and want to have maximum protection against modification, then we need to wrap it in an unmodifiableSet. I think it was just me being unnecessarily paranoid. So I dropped the wrapping.

albertzaharovits · 2022-01-25T16:35:40Z

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

+
+        @Override
+        public boolean containsAll(Collection<?> c) {
+            return false;


I think we should implement this in some way.

Thanks for catching it. I meant to but somehow missed it. Fixed by just delegating to contains

albertzaharovits · 2022-01-25T16:47:47Z

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

+            if (authorizedIndices == null) {
+                authorizedIndices = supplier.get();
+            }
+            return authorizedIndices;


I would slightly prefer we make this check thread safe, like in the CachingAsyncSupplier.
We're not using it in a multi-thread fashion, and I don't think we would ever do, and even if we did we only risk performance problems with building the set multiple times (but no correctness problems).
What do you think?

This Set is wrapped in a CachingAsyncSupplier, i.e. resolvedIndicesAsyncSupplier (not authorizedIndicesSupplier which I will remove, see below). The resolvedIndicesAsyncSupplier is what gets passed to other places so that this Set is never directly used by anything else. Since resolvedIndicesAsyncSupplier already guarantees thread safety, I think the Set itself can be kept simple. I added the following comment to the class to clarify its usage:

* NOTE that the lazy loading does not guarantee to run only once and is not meant to be used by multi-threads * because loading multiple times can incur performance penalty (but not correctness).

I don't think this implementation can be considered thread safe. There's no memory barrier here, so there's no guarantee about the state of the authorizedIndices field when viewed from another thread.

The options are:

simply state that it is not thread-safe and if callers require thread safety they need to synchronize

mark authorizedIndices as volatile

You are right. It is indeed not thread safe. I changed the documentation to state that and warned about changing how this class is used.

albertzaharovits · 2022-01-25T16:58:05Z

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

-                    })
-                );
-            });
+            final AsyncSupplier<Set<String>> authorizedIndicesSupplier = new CachingAsyncSupplier<>(


Do you think we still need the CachingAsyncSupplier wrapper magic?
I would ditch it, it's hard to follow and useless now.

I agree it is redundant now and should be removed.

albertzaharovits

LGTM this is looking cool!
Only had small points.

…-authorized-indices

elasticsearchmachine · 2022-01-27T00:15:40Z

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

ywangd · 2022-01-27T00:21:56Z

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

+    public RBACEngine(
+        Settings settings,
+        CompositeRolesStore rolesStore,
+        LoadAuthorizedIndicesTimeChecker.Factory authzIndicesTimerFactory


Moving TimeChecker here means it is no longer applied to custom AuthorizationEngine. I think it is OK. The intention was always to measure the performance of RBACEngine.

tvernum

There seems to be a lack of tests that this change actually achieves anything. The existing tests verify that you haven't broken any existing behaviour, but there's no test that would fail if someone changed the behaviour back so it was no longer lazy.

docs/changelog/81237.yaml

tvernum · 2022-01-30T23:36:38Z

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

+            if (authorizedIndices == null) {
+                authorizedIndices = supplier.get();
+            }
+            return authorizedIndices;


I don't think this implementation can be considered thread safe. There's no memory barrier here, so there's no guarantee about the state of the authorizedIndices field when viewed from another thread.

The options are:

simply state that it is not thread-safe and if callers require thread safety they need to synchronize

mark authorizedIndices as volatile

tvernum · 2022-01-30T23:38:35Z

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

+        @Override
+        public boolean contains(Object o) {
+            if (authorizedIndices == null) {
+                return predicate.test((String) o);


Strictly speaking this violates the guarantees on the Set interface. You should return false if o is not a String (rather than throw CCE).

Suggested change

return predicate.test((String) o);

if (o instanceof String s) {

return predicate.test(s);

} else {

return false;

}

I thought about it. But JDK doc says it can throw ClassCastException . So I didn't do anything. Also adding instanceof check can in theory hide subtle usage error. So I am not sure if it helps with anything.

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

…-authorized-indices

ywangd · 2022-01-31T01:20:30Z

There seems to be a lack of tests that this change actually achieves anything. The existing tests verify that you haven't broken any existing behaviour, but there's no test that would fail if someone changed the behaviour back so it was no longer lazy.

I added two assertions to existing tests to ensure that the returned Set from resolveAuthorizedIndicesFromRole is an instance of AuthorizedIndicesSet. Also added a new test to ensure the lazy behaviour of AuthorizedIndicesSet.

PS: Benchmark showed that the change helped a great deal for requests with a single concrete name.

...ck/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/RBACEngineTests.java

…ecurity/authz/RBACEngineTests.java Co-authored-by: Tim Vernum <[email protected]>

…#81237) This PR replaces the concrete list of authorizeIndices with a lazily load class so that loading is not triggered if not necessary. For example, a search targeting a concret name, GET index/_search no longer triggers the loading. But names with wildcard will still trigger the loading. However if we organize the indices with data streams and alias, it is possible to achieve similar wildcard effect while still avoid the loading. That is, searches like GET alias/_search or GET data_stream/_search will not trigger the loading even they target multiple indices behind the single name.

…#88149) * Avoid loadAuthorizedIndices for requests with concrete names (#81237) This PR replaces the concrete list of authorizeIndices with a lazily load class so that loading is not triggered if not necessary. For example, a search targeting a concret name, GET index/_search no longer triggers the loading. But names with wildcard will still trigger the loading. However if we organize the indices with data streams and alias, it is possible to achieve similar wildcard effect while still avoid the loading. That is, searches like GET alias/_search or GET data_stream/_search will not trigger the loading even they target multiple indices behind the single name. * fix compilation Co-authored-by: Elastic Machine <[email protected]>

WIP: Working code, needs clean up

b89cfce

ywangd added :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC v8.1.0 labels Dec 2, 2021

ywangd commented Dec 2, 2021

View reviewed changes

...ugin/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationService.java Outdated Show resolved Hide resolved

ywangd added the >enhancement label Dec 2, 2021

Not expose availableIndices to authorizationEngine

109cff8

Merge branch 'master' into more-avoidance-load-authorized-indices

28da09c

ywangd commented Dec 9, 2021

View reviewed changes

.../src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java Outdated Show resolved Hide resolved

albertzaharovits reviewed Jan 5, 2022

View reviewed changes

...ugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/AuthorizationEngine.java Outdated Show resolved Hide resolved

albertzaharovits reviewed Jan 5, 2022

View reviewed changes

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

ywangd mentioned this pull request Jan 18, 2022

Authorization for Internal Requests like Stats or Rollover is Slow in Large Clusters #79632

Closed

ywangd added 10 commits January 20, 2022 16:01

Merge remote-tracking branch 'origin/master' into more-avoidance-load…

3d078a1

…-authorized-indices

revert unrelated changes

a6097f1

spotless

63e52ce

address feedback

8b9aef3

tweak

48ff6e4

Simplify

9fd5f66

Merge remote-tracking branch 'origin/master' into more-avoidance-load…

ff5823d

…-authorized-indices

Minimized change by having lazily loaded set

af6426c

remove unnecessary file

4c0b360

fix compilation

cfb8e58

ywangd marked this pull request as ready for review January 24, 2022 03:36

elasticmachine added the Team:Security Meta label for security team label Jan 24, 2022

ywangd requested review from tvernum and albertzaharovits January 24, 2022 03:36

albertzaharovits reviewed Jan 25, 2022

View reviewed changes

albertzaharovits approved these changes Jan 25, 2022

View reviewed changes

albertzaharovits mentioned this pull request Jan 25, 2022

Only expand authorized indices for requests with wildcards #72118

Closed

ywangd added 2 commits January 27, 2022 10:34

Merge remote-tracking branch 'origin/master' into more-avoidance-load…

9a899fc

…-authorized-indices

address feedback

44deef5

Update docs/changelog/81237.yaml

def6cb4

ywangd commented Jan 27, 2022

View reviewed changes

tvernum reviewed Jan 30, 2022

View reviewed changes

ywangd added 2 commits January 31, 2022 11:08

Merge remote-tracking branch 'origin/master' into more-avoidance-load…

74daa90

…-authorized-indices

address feedback

8c67059

ywangd requested a review from tvernum January 31, 2022 01:21

tvernum approved these changes Jan 31, 2022

View reviewed changes

...ck/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/RBACEngineTests.java Outdated Show resolved Hide resolved

Update x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/s…

59e1034

…ecurity/authz/RBACEngineTests.java Co-authored-by: Tim Vernum <[email protected]>

ywangd merged commit 5265827 into elastic:master Jan 31, 2022

ywangd mentioned this pull request Jul 14, 2022

Avoid loadAuthorizedIndices for requests with concrete names (#81237) #88149

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More avoidance for loadAuthorizedIndices #81237

More avoidance for loadAuthorizedIndices #81237

ywangd commented Dec 2, 2021

ywangd commented Dec 8, 2021

albertzaharovits commented Jan 5, 2022

albertzaharovits commented Jan 5, 2022

ywangd commented Jan 24, 2022

elasticmachine commented Jan 24, 2022

albertzaharovits Jan 25, 2022

ywangd Jan 27, 2022

albertzaharovits Jan 25, 2022

ywangd Jan 27, 2022

albertzaharovits Jan 25, 2022

ywangd Jan 27, 2022

tvernum Jan 30, 2022

ywangd Jan 31, 2022

albertzaharovits Jan 25, 2022

ywangd Jan 27, 2022

albertzaharovits left a comment

elasticsearchmachine commented Jan 27, 2022

ywangd Jan 27, 2022

tvernum left a comment

tvernum Jan 30, 2022

tvernum Jan 30, 2022 •

edited

Loading

ywangd Jan 31, 2022

ywangd commented Jan 31, 2022

-                return predicate.test((String) o);
+                if (o instanceof String s) {
+                    return predicate.test(s);
+                } else {
+                    return false;
+                }

More avoidance for loadAuthorizedIndices #81237

More avoidance for loadAuthorizedIndices #81237

Conversation

ywangd commented Dec 2, 2021

ywangd commented Dec 8, 2021

albertzaharovits commented Jan 5, 2022

albertzaharovits commented Jan 5, 2022

ywangd commented Jan 24, 2022

elasticmachine commented Jan 24, 2022

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

albertzaharovits left a comment

Choose a reason for hiding this comment

elasticsearchmachine commented Jan 27, 2022

Choose a reason for hiding this comment

tvernum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvernum Jan 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywangd commented Jan 31, 2022

tvernum Jan 30, 2022 •

edited

Loading