Improve "Has Privilege" performance for boolean-only response #86685

albertzaharovits · 2022-05-11T13:55:22Z

Boolean-only privilege checks, i.e. the ones currently used in the "profile has privilege" API,
now benefit from a performance improvement, because the check will now stop upon first
encountering a privilege that is NOT granted over a resource (and return false overall).
Previously, all the privileges were always checked over all the resources in order to assemble
a comprehensive response with all the privileges that are not granted.

elasticsearchmachine · 2022-05-11T13:55:47Z

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

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

albertzaharovits · 2022-05-12T15:30:26Z

...k/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/Role.java

        Set<String> checkForIndexPatterns,
        boolean allowRestrictedIndices,
-        Set<String> checkForPrivileges
+        Set<String> checkForPrivileges,
+        @Nullable ResourcePrivilegesMap.Builder resourcePrivilegesMapBuilder
    );


I would like to take some space to explain my interface decisions for: Role#checkIndicesPrivilegeges, Role#checkApplicationResourcePrivileges, IndicesPermission#checkREsourcePrivileges, and ApplicationPermission#checkResourcePrivileges.

These methods must be aware of the mode of operation, ie whether to short-circuit or not, because they must return early in the short-circuit mode (in addition, the response "type" is different between the 2 modes) . So, one of the parameters of these methods must express this operation mode.

In this case, I think there are two implementation options to go about it:

pass a dedicated parameter, to signal the short-circuit mode, and make the return value contain an optional field which is null when in the short-circuit mode; eg RBACEngine#checkPrivileges(..., boolean runDetailedCheck, ..., ActionListener<PrivilegesCheckResult> listener) where PrivilegesCheckResult has two fields: boolean allChecksSuccess and an optional Details details.

return 2 values, rather than a single one with an optional null field. The first return value is the allChecksSuccess flag. The second returned value is via an out-parameter 👻 . When the out-parameter is non-null, it is used to return the privileges details, ie it works in the regular, non-short-circuit mode; otherwise, if null, no check details are returned and the operation mode is short-circuit.

I had 2 reasons to choose the second option for these methods (unlike RBACEngine#checkPrivileges):

I disliked having a boolean parameter going from the REST handler all the layers through to the permission level inside a role.

ResourcePrivilegesMap has a boolean field allowAll. Its value is currently driven by the individual privilege check results. But In the short-circuit mode, it becomes separated from the check results, as it can be true/false when the individual privilege check results are null. With such a separated allowAll parameter, keeping it still a member of the ResourcePrivilegesMap felt janky (what does it mean to update its value in the non-short-circuit mode, for every privilege result, when its value can technically also be set externally - the encapsulation was gone).

I hope the scare of an out-parameter is not too great.
I think this is the more elegant implementation of the two. I like that these check* methods always return a simple boolean. Also, the out parameter works OK in the application check section in RBACEngine#checkPrivileges and in the LimitedRole class, I think.

I noticed the out parameters on my first reading of the PR. I would probably approach it differently. But what is proposed in this PR works as well. Plus you already considered the other option and ruled it out with reasons. Also your reasoning about the standalone allowAll field and the individual check results of ResourcePrivilegesMap is a good one.

In summary, I am OK with the out parameters.

PS: In future, we may need support caching for LimitedRole (now it is only supported for SimpleRole). For that I am thinking that the result intersecting between role and limited-by-role should be done at the overall PrivilegesCheckResult level rather than the individual checks level (and also move most RBCEngine#checkPrivileges to Role level). This has nothing to do with the current PR. Just thinking out loud in case it is something you also thought about.

In summary, I am OK with the out parameters.

🙇

PS: In future, we may need support caching for LimitedRole (now it is only supported for SimpleRole). For that I am thinking that the result intersecting between role and limited-by-role should be done at the overall PrivilegesCheckResult level rather than the individual checks level (and also move most RBCEngine#checkPrivileges to Role level). This has nothing to do with the current PR. Just thinking out loud in case it is something you also thought about.

No I haven't considered something like that. I'm not sure I follow either. My first approach would be to investigate whether the LimitedRole can be cached like the Role can.

elasticmachine · 2022-05-18T16:34:49Z

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

albertzaharovits · 2022-05-19T08:16:13Z

@ywangd this is ready for review

ywangd

LGTM

Some comments nothing major. Up to you how to deal with them. Thanks!

ywangd · 2022-05-20T05:35:26Z

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

+     * The result of a (has) privilege check. This is not an Elasticsearch authorization result (though clients can base their authorization
+     * decisions on this response). The {@link #allChecksSuccess} field tells if all the privileges are granted over all the resources.


Can you clarify what do you mean by "thi is not an Elasticsearch authorization result"? Do you mean it is literally not the AuthorizationResult class or do you mean the authorization result might be different if you execute a request with the same privileged user?

I mean that this not to be used/interpreted as the Elasticsearch authorization result.

ywangd · 2022-05-20T06:10:43Z

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

+            boolean privilegesGranted = userRole.checkIndicesPrivileges(
                Sets.newHashSet(check.getIndices()),
                check.allowRestrictedIndices(),
-                Sets.newHashSet(check.getPrivileges())
+                Sets.newHashSet(check.getPrivileges()),
+                combineIndicesResourcePrivileges
            );


There is still a split information in the return boolean and the out-parameter. So I think it's helpful to assert the returned value is consistent with the out-parameter when it is not null (here and other call sites).

I had to revert these the assertions on the ResourcePrivilegesMap.Builder .
I have encountered NPEs which I was unable to track down all day.
I suspect there is some stupid magic about working with streams that I was unable to untangle. I don't think there are issues with the main code.

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

ywangd · 2022-05-20T06:37:58Z

...e/src/test/java/org/elasticsearch/xpack/core/security/authz/permission/LimitedRoleTests.java

+        ResourcePrivilegesMap.Builder resourcePrivilegesMapBuilder = randomBoolean() ? ResourcePrivilegesMap.builder() : null;
+        boolean privilegesCheck = role.checkIndicesPrivileges(
+            checkForIndexPatterns,
+            allowRestrictedIndices,
+            checkForPrivileges,
+            resourcePrivilegesMapBuilder
+        );
+        assertThat(privilegesCheck, is(expectedCheckResult));
+
+        if (resourcePrivilegesMapBuilder == null) {
+            resourcePrivilegesMapBuilder = ResourcePrivilegesMap.builder();
+            privilegesCheck = role.checkIndicesPrivileges(
+                checkForIndexPatterns,
+                allowRestrictedIndices,
+                checkForPrivileges,
+                resourcePrivilegesMapBuilder
+            );
+            assertThat(privilegesCheck, is(expectedCheckResult));
+        } else {
+            privilegesCheck = role.checkIndicesPrivileges(checkForIndexPatterns, allowRestrictedIndices, checkForPrivileges, null);
+            assertThat(privilegesCheck, is(expectedCheckResult));
+        }
+        assertThat(resourcePrivilegesMapBuilder.build(), equalTo(expectedAppPrivsByResource));


I think we can just remove the randomisation and just let the code call role.checkIndicesPrivileges twice with null and non-null builder? That is what it does anyway. Removing randomisation probably reads a bit easier.

I prefer we keep it. I think there's a high chance the order can make a difference when we evolve the caching.

ywangd · 2022-05-20T06:44:37Z

.../main/java/org/elasticsearch/xpack/core/security/authz/permission/ResourcePrivilegesMap.java

-    public boolean allAllowed() {
-        return allAllowed;
-    }


I wonder whether there is value to keep this method and let it compute it on the fly from the inner map?

Part of the ethos of this change was to keep the ResourcePrivilegesMap reserved for the check details only, so not include the authorization result (because they can exist separately). The allAllowed result can be confounded with the authorization result. Overall, I think it is better we don't expose it.

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

...security/src/test/java/org/elasticsearch/xpack/security/authz/AuthorizationServiceTests.java

albertzaharovits · 2022-05-23T21:17:28Z

@elasticmachine test this please

albertzaharovits · 2022-05-24T06:05:41Z

@elasticmachine test this please

albertzaharovits · 2022-05-24T07:48:44Z

@elasticmachine run elasticsearch-ci/bwc

albertzaharovits added 2 commits May 11, 2022 16:46

Main code

250c824

Merge branch 'master' into short-circuit-has-privileges

7b6be68

albertzaharovits added >enhancement :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC v8.3.0 labels May 11, 2022

albertzaharovits requested a review from ywangd May 11, 2022 13:55

albertzaharovits self-assigned this May 11, 2022

albertzaharovits and others added 4 commits May 11, 2022 16:55

Update docs/changelog/86685.yaml

9264102

Fix RBACEngineTests

c1236b0

Fix tests

8ac8151

Merge branch 'master' into short-circuit-has-privileges

47a34af

ywangd reviewed May 11, 2022

View reviewed changes

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

albertzaharovits added 3 commits May 12, 2022 14:29

Merge branch 'master' into short-circuit-has-privileges

0fa68b7

Merge branch 'master' into short-circuit-has-privileges

9858f73

Remove allMatch from ResourceMap

523cc9a

albertzaharovits commented May 12, 2022

View reviewed changes

albertzaharovits added 13 commits May 17, 2022 08:27

Merge branch 'master' into short-circuit-has-privileges

75a5da2

Move runDetailedCheck inside the PrivilegesToCheck class

d23d0f2

Merge branch 'master' into short-circuit-has-privileges

a7b7d9d

Fix ProfileIntegTests

f562b4a

Merge branch 'master' into short-circuit-has-privileges

a031a73

Randomization for existing tests

0a77701

Merge branch 'master' into short-circuit-has-privileges

f1c5d12

Nice multiple indices privileges

dff518f

Merge branch 'master' into short-circuit-has-privileges

1e4efa4

LimitRoleTests

175442b

Merge branch 'master' into short-circuit-has-privileges

7138519

Merge branch 'master' into short-circuit-has-privileges

5be2568

Javadocs

fc1b02d

elasticmachine added the Team:Security Meta label for security team label May 18, 2022

Merge branch 'master' into short-circuit-has-privileges

a1728bc

albertzaharovits requested a review from ywangd May 19, 2022 08:15

albertzaharovits added 3 commits May 19, 2022 11:17

Merge branch 'master' into short-circuit-has-privileges

66f82aa

Merge branch 'master' into short-circuit-has-privileges

d441dae

Merge branch 'master' into short-circuit-has-privileges

5f1af99

ywangd approved these changes May 20, 2022

View reviewed changes

albertzaharovits added 7 commits May 23, 2022 21:13

Merge branch 'master' into short-circuit-has-privileges

3e4cc8e

Merge failure

7787256

Meh, exception messages changed order

ce9e417

Nits

bc250d4

More nits

4286fcd

Nits

5b8ab0d

Merge branch 'master' into short-circuit-has-privileges

a18f98a

albertzaharovits added the auto-merge Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label May 23, 2022

Merge branch 'master' into short-circuit-has-privileges

c41ccad

albertzaharovits added 7 commits May 24, 2022 15:02

Fix assert

9e651f9

Merge branch 'master' into short-circuit-has-privileges

3aebda0

CustomAuthorizationEngine

21a8fed

assert...

8258348

Remove assertion

6b2f063

Merge branch 'master' into short-circuit-has-privileges

5d06314

Spotless

f1bc388

elasticsearchmachine merged commit 346abf9 into elastic:master May 24, 2022

albertzaharovits deleted the short-circuit-has-privileges branch May 24, 2022 15:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve "Has Privilege" performance for boolean-only response #86685

Improve "Has Privilege" performance for boolean-only response #86685

albertzaharovits commented May 11, 2022 •

edited

Loading

elasticsearchmachine commented May 11, 2022

albertzaharovits May 12, 2022

ywangd May 16, 2022

albertzaharovits May 18, 2022

elasticmachine commented May 18, 2022

albertzaharovits commented May 19, 2022

ywangd left a comment

ywangd May 20, 2022

albertzaharovits May 23, 2022

ywangd May 20, 2022

albertzaharovits May 24, 2022

ywangd May 20, 2022

albertzaharovits May 23, 2022

ywangd May 20, 2022

albertzaharovits May 23, 2022

albertzaharovits commented May 23, 2022

albertzaharovits commented May 24, 2022

albertzaharovits commented May 24, 2022

		* The result of a (has) privilege check. This is not an Elasticsearch authorization result (though clients can base their authorization
		* decisions on this response). The {@link #allChecksSuccess} field tells if all the privileges are granted over all the resources.

Improve "Has Privilege" performance for boolean-only response #86685

Improve "Has Privilege" performance for boolean-only response #86685

Conversation

albertzaharovits commented May 11, 2022 • edited Loading

elasticsearchmachine commented May 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented May 18, 2022

albertzaharovits commented May 19, 2022

ywangd 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertzaharovits commented May 23, 2022

albertzaharovits commented May 24, 2022

albertzaharovits commented May 24, 2022

albertzaharovits commented May 11, 2022 •

edited

Loading