Refactor CompositeRolesStore for separation of concerns #80926

ywangd · 2021-11-23T04:45:14Z

The process of role resolving is to build the Role object from an
Authentication object. The high level steps of this process is as the follows:

Locate the role reference for the Authentication, e.g. for regular user,
this means a collection of role names.
Retrieve the role descriptors for the role reference, e.g. search the
security index to get the role descriptors for the role name.
Build the Role object based on the role descriptors. There are also special
cases in the above process. For example, API keys do not have role names, but
two byteReference representing the role descriptors. API keys also have a
nested Role structure for limiting the key's actual privileges based on the
owner's.

Today, this process is managed by a single CompositeRolesStore class and the
steps are not clearly separated. This PR improves the situation by introducing
a new RoleReferenceResolver class that is responsible for turning roleReference
into role descriptors. CompositeRolesStore is now only responsible for buiding
the Role from the descriptors.

The RoleReference is also a new concept introduced by this PR, along with
AuthenticationContext and Subject. They technically belong to the issue of
revisiting Authentication class (#80117). But they are needed here to faciliate
the changes. Their usage will be expanded once we start working on #80117.

A Subject knows how to return a list of RoleReference and the final Role is the
intersection of all the RoleReference. This was a concept for API keys. It is
now generalised in this PR in the light on potential expanded usage of
limitedBy roles.

Relates: #80117

ywangd · 2021-11-23T06:00:17Z

@elasticmachine run elasticsearch-ci/part-2

elasticmachine · 2021-11-24T23:54:08Z

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

ywangd · 2021-11-24T23:58:09Z

...security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java

+        final List<RoleReference> roleReferences = subject.getRoleReferences(anonymousUser);
+        // TODO: Two levels of nesting can be relaxed in future
+        assert roleReferences.size() <= 2 : "only support up to one level of limiting";
+
+        buildRoleFromRoleReference(roleReferences.get(0), ActionListener.wrap(role -> {
+            if (roleReferences.size() == 1) {
+                roleActionListener.onResponse(role);
+            } else {
+                buildRoleFromRoleReference(
+                    roleReferences.get(1),
+                    ActionListener.wrap(
+                        limitedByRole -> roleActionListener.onResponse(LimitedRole.createLimitedRole(role, limitedByRole)),
+                        roleActionListener::onFailure
+                    )
+                );
+            }
+
+        }, roleActionListener::onFailure));


We only support 2 levels of role nesting right now because how LimitedRole works. These code can be generalised to support multiple levels once we re-work how Role nesting is represented (likely need an interface for Role)

ywangd · 2021-11-25T00:01:59Z

...src/main/java/org/elasticsearch/xpack/security/authz/store/DefaultRoleReferenceResolver.java

+            logger.trace(
+                () -> new ParameterizedMessage("Building role from descriptors [{}] for role names [{}]", effectiveDescriptors, roleNames)
+            );
+            // TODO: why not populate negativeLookupCache here with missing roles?


The existing code populates the negativeLookupCache after the Role gets successfully built.

I chose to retain this behaviour. But I wonder why it cannot be populated earlier here straight after roleDescriptor retrevial since we already know what roles are missing at this point?

I don't think there's any reason for that. I think it just ended up there after introducing the RoleRetrievalResult

I am going to leave it as is for now because:

It retains the current behaviour

I'd like to move the effective role descriptor checking (licensing) from the Resolver back to CompositeRolesStore. My thinking is the the Resolver should just resolve the role descriptors and the caller will handle both the license check and both negative and positive caching. But this change has a decent size and potentially delay the process of this PR. So I'd like to handle it in a follow-up (I added a task in Revisit and rationalise the Authentication class #80117).

ywangd · 2021-11-25T00:07:15Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/Subject.java

+        // Realm can be null for run-as user if it does not exist. Pretend it is a user and it will be rejected later in authorization
+        // This is to be consistent with existing behaviour.
+        if (realm == null) {
+            this.type = Type.USER;


This is a bit odd, but it is to retain the existing error reporting behaviour. We could argue that the existing error message is misleading anyway because it says something like

action [...] is unauthorized for user [elastic] run as [foo] with roles [], this action is granted by the cluster privileges [...]

which feels like the fix is to grant some roles to the user foo but the actual problem is that user foo does not exist. I think we should fix it. But it should be a separate PR.

ywangd · 2021-11-25T00:09:26Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/Subject.java

+    /**
+     * Return a List of RoleReferences that represents role definitions associated to the subject.
+     * The final role of this subject should be the intersection of all role references in the list.
+     */
+    public List<RoleReference> getRoleReferences(@Nullable AnonymousUser anonymousUser) {


In the eariler version, I wrapped the List inside a dedicate class. But decided later to just return the list directly. Because (1) I didn't come up with other useful field or method for that class; (2) the usage of the list is quite simple, it must be intersected and order not does not even matter.

I prefer a separate class, but we can handle it as a separate discussion.
My main reason is that I don't think a List means anything without a lot of context, but RoleIntersection does.

However, if we're going to do a separate PR to handle LimitedRole etc, we can bundle up a series of changes there.

Makes sense to me. I raised #81192 and let's sort it out there.

ywangd · 2021-11-25T00:12:16Z

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

+            // Since api key id is unique, it is sufficient and more correct to use it as the names
+            return new RoleKey(Set.of(apiKeyId), apiKeyId + roleKeySourceSuffix);


The existing code use API key role names as the first argument to RoleKey constructor. It's hard to get that information with the new structure and it is also unnecessary. Since API key ID is used as the source of RoleKey. It is already guaranteed that one entry for each single key. So I just use the API key ID again as the first argument.

Why do we include the api key id in the source field? Is that needed?

It is not needed. I just copied it from the existing code. But it was needed there because the names are not guaranteed to be unique. Since we now use the API Key ID as the names, it is no longer necesesary to have it in the source. I'll drop it.

ywangd · 2021-11-25T04:42:24Z

@elasticmachine run elasticsearch-ci/part-2

tvernum

I've reviewed the /main/ code for /core/, but I need to break for the day.

tvernum · 2021-11-29T07:23:49Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/Subject.java

+    /**
+     * Return a List of RoleReferences that represents role definitions associated to the subject.
+     * The final role of this subject should be the intersection of all role references in the list.
+     */
+    public List<RoleReference> getRoleReferences(@Nullable AnonymousUser anonymousUser) {


I prefer a separate class, but we can handle it as a separate discussion.
My main reason is that I don't think a List means anything without a lot of context, but RoleIntersection does.

However, if we're going to do a separate PR to handle LimitedRole etc, we can bundle up a series of changes there.

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/Subject.java

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/store/RoleKey.java

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

tvernum · 2021-11-29T07:37:43Z

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

+            // Since api key id is unique, it is sufficient and more correct to use it as the names
+            return new RoleKey(Set.of(apiKeyId), apiKeyId + roleKeySourceSuffix);


Why do we include the api key id in the source field? Is that needed?

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

...re/src/main/java/org/elasticsearch/xpack/core/security/authz/store/RolesRetrievalResult.java

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/user/User.java

tvernum

I've now reviewed everything except tests.

...security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java

...src/main/java/org/elasticsearch/xpack/security/authz/store/DefaultRoleReferenceResolver.java

tvernum · 2021-11-30T04:18:45Z

...src/main/java/org/elasticsearch/xpack/security/authz/store/DefaultRoleReferenceResolver.java

+            logger.trace(
+                () -> new ParameterizedMessage("Building role from descriptors [{}] for role names [{}]", effectiveDescriptors, roleNames)
+            );
+            // TODO: why not populate negativeLookupCache here with missing roles?


I don't think there's any reason for that. I think it just ended up there after introducing the RoleRetrievalResult

...src/main/java/org/elasticsearch/xpack/security/authz/store/DefaultRoleReferenceResolver.java

ywangd · 2021-12-01T01:29:10Z

This is ready for another look.

tvernum · 2021-12-02T03:39:28Z

It looks like something has confused GitHub - it thinks you have 67 commits with 357 modified files.

What can we do to get this back into a reviewable state?

The process of role resolving is to build the Role object from an Authentication object. The high level steps of this process is as the follows: 1. Locate the role reference for the Authentication, e.g. for regular user, this means a collection of role names. 2. Retrieve the role descriptors for the role reference, e.g. search the security index to get the role descriptors for the role name. 3. Build the Role object based on the role descriptors. There are also special cases in the above process. For example, API keys do not have role names, but two byteReference representing the role descriptors. API keys also have a nested Role structure for limiting the key's actual privileges based on the owner's. Today, this process is managed by a single CompositeRolesStore class and the steps are not clearly separated. This PR improves the situation by introducing a new RoleReferenceResolver class that is responsible for turning roleReference into role descriptors. CompositeRolesStore is now only responsible for buiding the Role from the descriptors. The RoleReference is also a new concept introduced by this PR, along with AuthenticationContext and Subject. They technically belong to the issue of revisiting Authentication class (elastic#80117). But they are needed here to faciliate the changes. Their usage will be expanded once we start working on elastic#80117. A Subject knows how to return a list of RoleReference and the final Role is the intersection of all the RoleReference. This was a concept for API keys. It is now generalised in this PR in the light on potential expanded usage of limitedBy roles. Relates: elastic#80117

ywangd · 2021-12-02T05:52:08Z

What can we do to get this back into a reviewable state?

I force-pushed. It didn't preserve previous commit message but at least get the PR in a sane state. I hope that works with you. Thanks!

ywangd · 2021-12-03T00:24:43Z

@elasticmachine update branch

tvernum

LGTM, though the tests seem a little too narrow in scope.

tvernum · 2021-12-06T07:23:19Z

x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authc/SubjectTests.java

+import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.isA;
+
+public class SubjectTests extends ESTestCase {


I assume this is a result of moving things (including tests) around between classes, but it seems strange that this class only has tests for API Keys.

I would prefer there were more tests.

I added more tests to cover other subject types.

…ion-class

ywangd · 2021-12-06T12:44:56Z

LGTM, though the tests seem a little too narrow in scope.

Thanks Tim. I added more tests for both Subject and RoleReference in the latest commit 31dc580

The variable lives in Subject since elastic#80926. The PR also replace string concatenation with text block to be consistent with other refactoring efforts (elastic#80751).

* Remove unused bwc variable from ApiKeyService The variable lives in Subject since #80926.

Creating tokens using API keys is not properly supported till elastic#80926. Previously the created token always has no previlege. Now the token has the same privilege as the API key itself (similar to user created tokens). Authenticating using the token is considered equivalent to the API key itself. Therefore the "isApiKey" check needs to be updated to cater for both authentications of API key itself and the token created by the API key. This PR updated the isApiKey check and apply it consistently to ensure the behaviour is consistent between an API key and a token created by it. The only exception is for supporting run-as. API key itself can run-as another user. But a token created by the API key cannot perform run-as (elastic#84336) similar to how user/token works.

Creating tokens using API keys is not properly supported till #80926. Previously the created token always has no previlege. Now the token has the same privilege as the API key itself (similar to user created tokens). Authenticating using the token is considered equivalent to the API key itself. Therefore the "isApiKey" check needs to be updated to cater for both authentications of API key itself and the token created by the API key. This PR updates the isApiKey check and apply it consistently to ensure the behaviour is consistent between an API key and a token created by it. The only exception is for supporting run-as. API key itself can run-as another user. But a token created by the API key cannot perform run-as (#84336) similar to how user/token works.

Creating tokens using API keys is not properly supported till elastic#80926. Previously the created token always has no previlege. Now the token has the same privilege as the API key itself (similar to user created tokens). Authenticating using the token is considered equivalent to the API key itself. Therefore the "isApiKey" check needs to be updated to cater for both authentications of API key itself and the token created by the API key. This PR updates the isApiKey check and apply it consistently to ensure the behaviour is consistent between an API key and a token created by it. The only exception is for supporting run-as. API key itself can run-as another user. But a token created by the API key cannot perform run-as (elastic#84336) similar to how user/token works.

Creating tokens using API keys is not properly supported till #80926. Previously the created token always has no previlege. Now the token has the same privilege as the API key itself (similar to user created tokens). Authenticating using the token is considered equivalent to the API key itself. Therefore the "isApiKey" check needs to be updated to cater for both authentications of API key itself and the token created by the API key. This PR updates the isApiKey check and apply it consistently to ensure the behaviour is consistent between an API key and a token created by it. The only exception is for supporting run-as. API key itself can run-as another user. But a token created by the API key cannot perform run-as (#84336) similar to how user/token works.

This PR removes the AuthenticationContext class introduced in elastic#80926 and merges its functions into Authentication. It becomes more apparent that the most useful refactoring in elastic#80926 is the new Subject class, which is also what AuthenticationContext provides most of its value. The AuthenticationContext is essentially just a thin wrapper of two subjects which represents the existing Authentication object in a more structured format. The original plan was to replace Authentication with AuthenticationContext. However, it has practical challenges that the usage of Authentication is too wide spread. It's hard to have a series of scoped changes to replace it. Therefore the new plan is to stick with Authentication, agumenting it with subjects similar to what AuthenticationContext has and remove AuthenticationContext. This PR also deprecates methods that should be replaced by methods of Subjects. In future, the plan is to remove the deprecated methods, also rework the User class so it does not need nest another User to represent run-as (which is another main reason for the original refactor elastic#80926). Overall, the new plan makes it easier to spread the work in a few more tightly scoped PRs while achieving the same original goal.

This PR removes the AuthenticationContext class introduced in #80926 and merges its functions into Authentication. It becomes more apparent that the most useful refactoring in #80926 is the new Subject class, which is also what AuthenticationContext provides most of its value. The AuthenticationContext is essentially just a thin wrapper of two subjects which represents the existing Authentication object in a more structured format. The original plan was to replace Authentication with AuthenticationContext. However, it has practical challenges that the usage of Authentication is too wide spread. It's hard to have a series of scoped changes to replace it. Therefore the new plan is to stick with Authentication, agumenting it with subjects similar to what AuthenticationContext has and remove AuthenticationContext. This PR also deprecates methods that should be replaced by methods of Subjects. In future, the plan is to remove the deprecated methods, also rework the User class so it does not need nest another User to represent run-as (which is another main reason for the original refactor #80926). Overall, the new plan makes it easier to spread the work in a few more tightly scoped PRs while achieving the same original goal.

ywangd added >refactoring :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC v8.1.0 labels Nov 23, 2021

tvernum mentioned this pull request Nov 24, 2021

Revisit and rationalise the Authentication class #80117

Closed

4 tasks

ywangd force-pushed the revisit-authentication-class branch from 340b904 to 1f84e44 Compare November 24, 2021 23:52

ywangd changed the title ~~Separate CompositeRolesStore into roleDescriptors retrieval and role building~~ Refactor CompositeRolesStore for speration of concerns Nov 24, 2021

ywangd marked this pull request as ready for review November 24, 2021 23:54

elasticmachine added the Team:Security Meta label for security team label Nov 24, 2021

ywangd requested a review from tvernum November 24, 2021 23:54

ywangd commented Nov 24, 2021

View reviewed changes

ywangd commented Nov 25, 2021

View reviewed changes

tvernum changed the title ~~Refactor CompositeRolesStore for speration of concerns~~ Refactor CompositeRolesStore for separation of concerns Nov 29, 2021

tvernum reviewed Nov 29, 2021

View reviewed changes

tvernum reviewed Nov 30, 2021

View reviewed changes

ywangd requested a review from tvernum December 1, 2021 01:28

ywangd force-pushed the revisit-authentication-class branch from 4acbdb9 to 4c6bb42 Compare December 2, 2021 05:49

Merge branch 'master' into revisit-authentication-class

5203df0

tvernum approved these changes Dec 6, 2021

View reviewed changes

address feedback to add more tests

31dc580

Merge remote-tracking branch 'origin/master' into revisit-authenticat…

536f946

…ion-class

ywangd merged commit c7aa2b5 into elastic:master Dec 7, 2021

ywangd mentioned this pull request Dec 20, 2021

Remove unused bwc variable from ApiKeyService #81964

Merged

ywangd added a commit that referenced this pull request Jan 10, 2022

Remove unused bwc variable from ApiKeyService (#81964)

7b90d6f

* Remove unused bwc variable from ApiKeyService The variable lives in Subject since #80926.

This was referenced Feb 28, 2022

Fix isApiKey test and apply it consistently #84396

Merged

Fix can access resource checks for API Keys with run as #84277

Merged

ywangd mentioned this pull request Feb 28, 2022

Fix isApiKey test and apply it consistently (#84396) #84404

Merged

ywangd mentioned this pull request Mar 23, 2022

Merge AuthenticationContex into Authentication #85255

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor CompositeRolesStore for separation of concerns #80926

Refactor CompositeRolesStore for separation of concerns #80926

ywangd commented Nov 23, 2021 •

edited

Loading

ywangd commented Nov 23, 2021

elasticmachine commented Nov 24, 2021

ywangd Nov 24, 2021

ywangd Nov 25, 2021

tvernum Nov 30, 2021

ywangd Dec 1, 2021

ywangd Nov 25, 2021

ywangd Nov 25, 2021

tvernum Nov 29, 2021

ywangd Dec 1, 2021

ywangd Nov 25, 2021 •

edited

Loading

tvernum Nov 29, 2021

ywangd Dec 1, 2021

ywangd commented Nov 25, 2021

tvernum left a comment

tvernum Nov 29, 2021

tvernum Nov 29, 2021

tvernum left a comment

tvernum Nov 30, 2021

ywangd commented Dec 1, 2021

tvernum commented Dec 2, 2021

ywangd commented Dec 2, 2021

ywangd commented Dec 3, 2021

tvernum left a comment

tvernum Dec 6, 2021

ywangd Dec 6, 2021

ywangd commented Dec 6, 2021

		// Since api key id is unique, it is sufficient and more correct to use it as the names
		return new RoleKey(Set.of(apiKeyId), apiKeyId + roleKeySourceSuffix);

Refactor CompositeRolesStore for separation of concerns #80926

Refactor CompositeRolesStore for separation of concerns #80926

Conversation

ywangd commented Nov 23, 2021 • edited Loading

ywangd commented Nov 23, 2021

elasticmachine commented Nov 24, 2021

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

ywangd Nov 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywangd commented Nov 25, 2021

tvernum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

ywangd commented Dec 1, 2021

tvernum commented Dec 2, 2021

ywangd commented Dec 2, 2021

ywangd commented Dec 3, 2021

tvernum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywangd commented Dec 6, 2021

ywangd commented Nov 23, 2021 •

edited

Loading

ywangd Nov 25, 2021 •

edited

Loading