Superuser fastpath for indexAccessControl #78498

ywangd · 2021-09-30T06:54:12Z

This PR adds a fast path for loading authorizedIndices and computing
indicesAccessControl if the role has all access to all indices.

A role is considered to have all access to all indices if any of its
IndicesPermission#Group satisfy the following criteria:

Any of the index patterns is a simple match-all wildcard, i.e. "*"
It allows access to restricted indices
It grants the "all" index privilege
It has no DLS or FLS

An example of such role is the builtin superuser role.

Note the fastpath does not apply to roles that have "effective" but not
direct "all access of all indices". For example, if the "effective"
access is achieved by combining multiple Groups belong to the role, or
combining multiple index patterns within a single Group, the fastpath
will not apply.

This fast path is provided so that we have a reference baseline for
authorization related performance which is useful for both production
and troubleshooting.

This PR adds a fast path for loading authorizedIndices and computing indicesAccessControl if the role has all access to all indices. A role is considered to have all access to all indices if any of its IndicesPermission#Group satisfy the following criteria: 1. Any of the index patterns is a simple match-all wildcard, i.e. "*" 2. It allows access to restricted indices 3. It grants the "all" index privilege 4. It has no DLS or FLS An example of such role is the builtin superuser role. Note the fastpath does not apply to roles that have "effective" but not direct "all access of all indices". For example, if the "effective" access is achieved by combining multiple Groups belong to the role, or combining multiple index patterns within a single Group. This fast path is provided so that we have a reference baseline for authorization related performance which is useful for both production use and troubleshooting.

elasticmachine · 2021-09-30T06:54:15Z

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

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

This reverts commit 914f56e.

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

…-for-index-authorization

ywangd · 2021-10-01T04:47:59Z

@elasticmachine update branch

ywangd · 2021-10-01T05:23:03Z

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

@@ -118,6 +118,10 @@ private StringMatcher indexMatcher(Collection<String> ordinaryIndices, Collectio
        return groups;
    }

+    public boolean isTotal() {
+        return Arrays.stream(groups).anyMatch(Group::isTotal);


We can make this stricter to require the groups to have only one member, which is the case for superuser role and the API keys created by it.

I think the condition is OK like this.

But this being a public method irks me a little bit.
I think we can avoid it by including this check at the beginning of the authorize method.
We get one less method to worry about for the caller, especially since the isTotal is fuzzy (isTotal can be false but the permission to actually grant all indices).

And we also avoid the companion isTotal public method on the Role and LimitedRole (hopefully).

Updated as suggested.

albertzaharovits · 2021-10-12T19:51:06Z

...ain/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/IndicesAccessControl.java

+
+        @Override
+        public IndexAccessControl getIndexPermissions(String index) {
+            return ALLOW_ALL_INDEX_ACCESS_CONTROL;


I like it that we now return an IndexAccessControl instance instead of null, for the "allow all" case.

albertzaharovits · 2021-10-12T20:11:10Z

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

-        return new IndexAuthorizationResult(true, accessControl);
+        // Fast path if the role can access all indices, e.g. superuser
+        if (role.allowAllIndices()) {
+            return new IndexAuthorizationResult(true, IndicesAccessControl.allowAll());


My suggestion from the other comments is to push the fast-path one level down in the role.authorize call, for slight reasons of interface clarity, but I also prefer it because at this level we avoid introducing a bypass for such an important operation as authorization (I feel that the bypass inside the authorize method is more tolerable mentally).

albertzaharovits

I had more of a style suggestion, in order to limit the portion of the code that has to "know" of the bypass. I hope you find it useful.

I think I would've implemented it as a bypass for the static superuser role only, but I now believe being more generic and inspecting the index permissions is superior, although slightly harder to explain in words.

tvernum · 2021-10-15T01:42:52Z

...ain/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/IndicesAccessControl.java

+            return limitedByIndicesAccessControl;
+        } else if (limitedByIndicesAccessControl instanceof AllowAllIndicesAccessControl) {
+            return this;
+        }


We seem to have 3 different (overlapping) methods for deciding whether an IndicesAccessControl object is the allow all object.

instanceof AllowAllIndicesAccessControl

== AllowAllIndicesAccessControl.ALLOW_ALL_INDICES_ACCESS_CONTROL

isAllowAll() (implemented via method 2)

Can we standardise?

I removed the 3rd method (it is actually not used ...) and thus remove 2 as well. Option 1 is the standard now.

tvernum · 2021-10-15T01:46:25Z

...ain/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/IndicesAccessControl.java

+    private static class AllowAllIndicesAccessControl extends IndicesAccessControl {
+
+        private static final IndicesAccessControl ALLOW_ALL_INDICES_ACCESS_CONTROL = new AllowAllIndicesAccessControl();
+        private static final IndexAccessControl ALLOW_ALL_INDEX_ACCESS_CONTROL = new IndexAccessControl(true, null, null);


Can we make this a non-static field?

Since ALLOW_ALL_INDICES_ACCESS_CONTROL is a singleton, having a non-static field for ALLOW_ALL_INDEX_ACCESS_CONTROL makes no difference in terms on memory usage or execution time.
However, it makes the code much cleaner because then we can have 1 static field for the singleton INSTANCE which is the one you want to use when you need an instance of AllowAllIndicesAccessControl and the other field becomes a member field and there's no chance of ambiguity because it's clearly an internal implementation detail.

Yep. updated as suggested.

tvernum · 2021-10-15T01:50:27Z

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

-        return new IndexAuthorizationResult(true, accessControl);
+        // Fast path if the role can access all indices, e.g. superuser
+        if (role.allowAllIndices()) {
+            return new IndexAuthorizationResult(true, IndicesAccessControl.allowAll());


tvernum · 2021-10-15T02:09:22Z

I don't love the idea of optimizing for superuser because we want to discourage people from using superuser for anything other than maintenance.
However, if we follow Albert's suggestions and push the optimizations into Role.authorize then (in the future) we can look at how to adapt it for other more reasonable use cases (for example a user with monitor over *)

…-for-index-authorization

ywangd · 2021-10-15T05:22:11Z

@albertzaharovits @tvernum

Thanks for looking into this PR. Sorry there was some messiness. I agree on the main suggestion which is to push down the short circuit logic to Role and IndicesPermission. I have updated the code accordingly so that all special handling is inside IndicesPermission.

However, if we follow Albert's suggestions and push the optimizations into Role.authorize then (in the future) we can look at how to adapt it for other more reasonable use cases (for example a user with monitor over *)

This is a good point. Being able to short circuit for more scenarios will definitely be a bonus.

…-for-index-authorization

tvernum · 2021-10-15T06:53:24Z

...ain/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/IndicesAccessControl.java

+
+        private static final IndicesAccessControl INSTANCE = new AllowAllIndicesAccessControl();
+
+        private final IndexAccessControl ALLOW_ALL_INDEX_ACCESS_CONTROL = new IndexAccessControl(true, null, null);


Nit: Since this is no longer static it shouldn't have an ALL_CAPS_NAME either.

albertzaharovits · 2021-10-15T08:03:32Z

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

@@ -336,7 +336,7 @@ private void authorizeAction(final RequestInfo requestInfo, final String request
        if (ClusterPrivilegeResolver.isClusterAction(action)) {
            final ActionListener<AuthorizationResult> clusterAuthzListener =
                wrapPreservingContext(new AuthorizationResultListener<>(result -> {
-                        threadContext.putTransient(INDICES_PERMISSIONS_KEY, IndicesAccessControl.ALLOW_ALL);
+                        threadContext.putTransient(INDICES_PERMISSIONS_KEY, IndicesAccessControl.allowAll());
                        listener.onResponse(null);


I like how GH throws in a main source code change in between test ones, to keep us on our toes about tests.

albertzaharovits

LGTM, Thanks Yang!

…-for-index-authorization

elasticsearchmachine · 2021-10-18T06:49:37Z

💔 Backport failed

Status	Branch	Result
❌	7.x	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 78498

This PR adds a fast path for computing indicesAccessControl if the role has all access to all indices. A role is considered to have all access to all indices if any of its IndicesPermission#Group satisfy the following criteria: 1. Any of the index patterns is a simple match-all wildcard, i.e. "*" 2. It allows access to restricted indices 3. It grants the "all" index privilege 4. It has no DLS or FLS An example of such role is the builtin superuser role. Note the fastpath does not apply to roles that have "effective" but not direct "all access of all indices". For example, if the "effective" access is achieved by combining multiple Groups belong to the role, or combining multiple index patterns within a single Group. This fast path is provided so that we have a reference baseline for authorization related performance which is useful for both production use and troubleshooting.

* upstream/master: Changing test keytab to use aes256-cts-hmac-sha1-96 instead of des3-cbc-sha1-kd (elastic#78703) Add support for configuring HNSW parameters (elastic#79193) Deprecate resolution loss on date field (elastic#78921) Add Optional to Configure bind user (elastic#78303) Adapt BWC after backporting elastic#78765 (elastic#79350) [DOCS] Add deprecation notice for reset password tool (elastic#78793) added test for flattened type in top_metrics.yml (elastic#78960) [DOCS] Fixes indentation issue in GET trained models API docs. (elastic#79347) Fix parsing of PBES2 encrypted PKCS#8 keys (elastic#78904) Mute testReindex (elastic#79343) Node level can match action (elastic#78765) Fix duplicate license header in source files (elastic#79236) AllowAll for indicesAccessControl (elastic#78498) Better logging and internal user handling for operator privileges (elastic#79331) # Conflicts: # server/src/main/java/org/elasticsearch/index/mapper/MappingParser.java

ywangd added >enhancement :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC v8.0.0 v7.16.0 labels Sep 30, 2021

ywangd requested review from tvernum and albertzaharovits September 30, 2021 06:54

elasticmachine added the Team:Security Meta label for security team label Sep 30, 2021

ywangd added 2 commits September 30, 2021 17:38

fix bugs

b014814

fix test

914f56e

ywangd commented Sep 30, 2021

View reviewed changes

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

ywangd added 2 commits September 30, 2021 18:55

Revert "fix test"

8a35041

This reverts commit 914f56e.

adjust the fastpath location for indicesAccessControl

1e3dbdf

ywangd commented Sep 30, 2021

View reviewed changes

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

Merge remote-tracking branch 'origin/master' into superuser-fast-path…

120aeaf

…-for-index-authorization

elasticmachine and others added 2 commits October 1, 2021 14:48

Merge branch 'master' into superuser-fast-path-for-index-authorization

175260b

remove fastpath for load authorized indices

2bc6593

ywangd commented Oct 1, 2021

View reviewed changes

ywangd changed the title ~~Superuser fastpath for index authorization~~ Superuser fastpath for indexAccessControl Oct 11, 2021

albertzaharovits reviewed Oct 12, 2021

View reviewed changes

tvernum reviewed Oct 15, 2021

View reviewed changes

ywangd added 2 commits October 15, 2021 15:32

Merge remote-tracking branch 'origin/master' into superuser-fast-path…

800cf1e

…-for-index-authorization

address feedback to push shortcircuit down a level

69b3ed3

ywangd requested a review from tvernum October 15, 2021 05:24

ywangd requested a review from albertzaharovits October 15, 2021 05:24

Merge remote-tracking branch 'origin/master' into superuser-fast-path…

95737ac

…-for-index-authorization

tvernum approved these changes Oct 15, 2021

View reviewed changes

albertzaharovits reviewed Oct 15, 2021

View reviewed changes

albertzaharovits approved these changes Oct 15, 2021

View reviewed changes

ywangd added 2 commits October 18, 2021 15:43

Merge remote-tracking branch 'origin/master' into superuser-fast-path…

7f055b1

…-for-index-authorization

address feedback

a0b0a62

ywangd added the auto-backport-and-merge Automatically create backport pull requests and merge when ready label Oct 18, 2021

ywangd merged commit 5ae1518 into elastic:master Oct 18, 2021

ywangd mentioned this pull request Oct 18, 2021

AllowAll for indicesAccessControl (#78498) #79338

Merged

jakelandis added v8.0.0-beta1 and removed v8.0.0 labels Oct 27, 2021

albertzaharovits mentioned this pull request Dec 15, 2021

Improve Authorization performance in clusters with a large number of indices #67987

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Superuser fastpath for indexAccessControl #78498

Superuser fastpath for indexAccessControl #78498

ywangd commented Sep 30, 2021 •

edited

Loading

elasticmachine commented Sep 30, 2021

ywangd commented Oct 1, 2021

ywangd Oct 1, 2021

albertzaharovits Oct 12, 2021 •

edited

Loading

albertzaharovits Oct 12, 2021

ywangd Oct 15, 2021

albertzaharovits Oct 12, 2021 •

edited

Loading

albertzaharovits Oct 12, 2021

tvernum Oct 15, 2021

albertzaharovits left a comment

tvernum Oct 15, 2021

ywangd Oct 15, 2021

tvernum Oct 15, 2021

ywangd Oct 15, 2021

tvernum Oct 15, 2021

tvernum commented Oct 15, 2021 •

edited

Loading

ywangd commented Oct 15, 2021

tvernum Oct 15, 2021

ywangd Oct 18, 2021

albertzaharovits Oct 15, 2021

albertzaharovits left a comment

elasticsearchmachine commented Oct 18, 2021


		private static final IndicesAccessControl INSTANCE = new AllowAllIndicesAccessControl();

		private final IndexAccessControl ALLOW_ALL_INDEX_ACCESS_CONTROL = new IndexAccessControl(true, null, null);

Superuser fastpath for indexAccessControl #78498

Superuser fastpath for indexAccessControl #78498

Conversation

ywangd commented Sep 30, 2021 • edited Loading

elasticmachine commented Sep 30, 2021

ywangd commented Oct 1, 2021

Choose a reason for hiding this comment

albertzaharovits Oct 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertzaharovits Oct 12, 2021 • edited Loading

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

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

tvernum commented Oct 15, 2021 • edited Loading

ywangd commented Oct 15, 2021

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 Oct 18, 2021

💔 Backport failed

ywangd commented Sep 30, 2021 •

edited

Loading

albertzaharovits Oct 12, 2021 •

edited

Loading

albertzaharovits Oct 12, 2021 •

edited

Loading

tvernum commented Oct 15, 2021 •

edited

Loading