Resolve anonymous roles and deduplicate roles during authentication #53453

ywangd · 2020-03-12T08:06:36Z

Anonymous roles resolution and user role deduplication are now performed during authentication instead of authorization. The change ensures:

If anonymous access is enabled, user will be able to see the anonymous roles added in the roles field in the /_security/_authenticate response.
Any duplication in user roles are removed and will not show in the above authenticate response.
In any other case, the response is unchanged.

It also introduces a behaviour change: the anonymous role resolution is now authentication node specific, while it was authorization node specific. Details can be found at #47195 (comment)

No doc changes. Doc update is tracked separatedly.

Resolves: #47195

elasticmachine · 2020-03-12T08:06:38Z

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

ywangd · 2020-03-12T08:26:57Z

...urity/src/main/java/org/elasticsearch/xpack/security/rest/action/RestAuthenticateAction.java

+                    builder.endObject();
+                } else {
+                    authenticateResponse.authentication().toXContent(builder, ToXContent.EMPTY_PARAMS);
+                }


The tricky bits of this work are:

XContent serialization is managed inside Authentication

Information about anonymous user is not available in Authentication.

One approach is to add an new anonymousRoles field to User class and populate it during authentication. But this has wide spread impact as the User object is serialised and sent everywhere. Also the anonymous access is by default off, so it does not seem to justify the wide spread change.

So the new logic is added here in RestAuthenticationAction, which can create the anonymous user object and inject its roles into the XContent object.

One approach is to add an new anonymousRoles field to User class and populate it during authentication.

I think the best way forward is to actually do this ( no need to add a field to User), but evaluate the anonymous roles during authentication (i.e. right after the realms return the User in AuthenticationService#consumeToken ) instead of during authorization.

If we don't want to do this now, but defer it to a later point ( or as part of the effort to refactor the logic around AuthenticationTokens ), I think this fits better in the Transport Action than in the Rest one.

I think this will only work if we move away from adding the new anonymous_role field in the response, am I right?
Otherwise, there is no place to save these role information. We could use User#metadata but it feels hacky.

Yes this implies we follow my other comment and anonymous roles are part of roles

Great. Thanks for the clarification.

@jkakavas There are some issues for adding the anonymous roles into User#roles

AuthenticationService#consumeToken does not cover runAs user, which is handled in lookupRunAsUser.

Both above methods end in finishAuthentication , so this could be a better candidate.

But doing above changes the existing logic. It's adding the anonymous roles to all users. Currently, some system users do not inherit anonymous roles. The code even asserts that XPackUser can only have a single role. There could be other situations where adding anonymous roles are not desired?

The final role resolving is handled in CompositeRolesStore#getRoles. Result of this method is the source of truth as it is what gets used in execution.

In summary, adding roles to all authenticated user has wider impact and it does not feel ideal. I think that the Authorization process may still be a better place for this, because "granting more roles" to user sounds more like "authorization" instead of "authentication". In fact, CompositeRolesStore#getRoles (part of authorization) is where this happens.

The challenge of using result of CompositeRolesStore#getRoles is that it is not easily accessible:

The resulted AuthorizationInfo object does get saved in threadContext as a Transient value. But this does not seem to be accessible once cross the wire.

It is also not possible (or ideal) to alter the User object in authorization process for saving these resolved roles.

Adding a new key in threadContext for saving these roles seem to be overkill and inconsistent.

So I'd like to propose the following approach:

Add a new field for the resolved roles in org.elasticsearch.xpack.core.security.action.user.AuthenticateResponse.

In TransportAuthenticateAction, grab the resolved roles from threadContext and set it to the AuthenticateResponse object. It may be possible to access the transient value here because the transport action runs in the same machine as the "authorization" process, i.e. not cross wire.

RestAuthenticateAction has access to AuthenticateResponse from which the final response can be built.

Please let me know your thoughts. Thanks.

There could be other situations where adding anonymous roles are not desired?

I can think of reserved and internal users. I don't think we need to care about internal users here ( _system, _xpack etc ) as we wouldn't be handling requests from the REST layer as these users. But reserved users ( i.e. kibana ) would be problematic.

I think that the Authorization process may still be a better place for this, because "granting more roles" to user sounds more like "authorization" instead of "authentication". In fact, CompositeRolesStore#getRoles (part of authorization) is where this happens.

But we do resolve and populate the user roles in authentication too which makes it hard to have this mental distinction. I would argue that role resolving based on user attributes ( i.e. role mapping ) is part of authentication. Granting access based on the definition of these roles is authorization.

I still feel that this fits better in AuthenticationService but did not have the cycles to think it through. I think this would benefit from a short live discussion, care to put it on the agenda for our weekly meeting tomorrow? We can reach a consensus there and come back here with that and see this through to completion

I added it under Forward agenda. Thanks.

ywangd · 2020-03-12T11:33:27Z

Had some fun with XContent and update client side AuthenticationResponse and the test.

jkakavas · 2020-03-13T09:12:27Z

...urity/src/main/java/org/elasticsearch/xpack/security/rest/action/RestAuthenticateAction.java

+                    && anonymousUser.roles().length != 0) {
+                    builder.startObject();
+                    authenticateResponse.authentication().toXContentFragment(builder);
+                    builder.array(User.Fields.ANONYMOUS_ROLES.getPreferredName(), anonymousUser.roles());


I think I had a change of heart since #47195 (comment). I think that the fact that these are roles inherited by the anonymous user configuration is more important to the administrator and/or for troubleshooting than it is for the user themselves. As such, if I know/should know that anonymous access is enabled, I can make sense of the response ( as to why more roles are returned ) and if I don't, I probably just care about what roles I have more than why I have them.

Similar to why we don't return "native_roles": {xxx}, "roles_from_role_mapping_based_on_groups": {}, "roles_from_role_mapping_based_on_username": {}, "roles_from_role_mapping_based_on_metadata": {} , I believe we shouldn't be adding anonymous_roles here. The purpose of the API is to tell us which roles the user has, but not why they have them.

You have a point here plus it makes the whole thing easier. I have no objection.

Just some thought process: I am ok with adding a new field. But the name "anonymous_roles" feels too specific for me to like. This does raise the question why we don't have things like native_roles etc. I wanted name it inherited_roles so it can at least to re-used. But if we treat anonymous roles just like another role providers, it should then just be part of the existing roles field. So yes, I'll remove the new field.

jkakavas · 2020-03-13T10:37:01Z

...urity/src/main/java/org/elasticsearch/xpack/security/rest/action/RestAuthenticateAction.java

+                    builder.endObject();
+                } else {
+                    authenticateResponse.authentication().toXContent(builder, ToXContent.EMPTY_PARAMS);
+                }


One approach is to add an new anonymousRoles field to User class and populate it during authentication.

I think the best way forward is to actually do this ( no need to add a field to User), but evaluate the anonymous roles during authentication (i.e. right after the realms return the User in AuthenticationService#consumeToken ) instead of during authorization.

If we don't want to do this now, but defer it to a later point ( or as part of the effort to refactor the logic around AuthenticationTokens ), I think this fits better in the Transport Action than in the Rest one.

ywangd · 2020-03-20T10:06:49Z

The PR is updated according to the latest discussions. Specifically, the anonymous role handling is now moved from authorization phase to authentication phase. The existing behaviours are mostly preserved, e.g. API key is not affected, system users do not inherit anonymous roles.

There is one changed behaviour: anonymous roles are now authentication node specific instead of authorization node. We generally feel this is more aligned with other authentication behaviours and do not consider it as breaking change but rather a bug fix.

jkakavas · 2020-03-24T08:00:16Z

...-security/src/test/java/org/elasticsearch/xpack/security/EnableSecurityOnBasicLicenseIT.java

@@ -126,7 +126,7 @@ private void checkAuthentication() throws IOException {
        final Map<String, Object> auth = getAsMap("/_security/_authenticate");
        // From file realm, configured in build.gradle
        assertThat(ObjectPath.evaluate(auth, "username"), equalTo("security_test_user"));
-        assertThat(ObjectPath.evaluate(auth, "roles"), contains("security_test_role"));
+        assertThat(ObjectPath.evaluate(auth, "roles"), contains("security_test_role", "anonymous"));


Do we guarantee order ? if not maybe containsInAnyOrder() is better here ?

Updated. The order is preserved. But I don't think we need guarantee it.

jkakavas · 2020-03-24T08:06:49Z

...gin/security/src/main/java/org/elasticsearch/xpack/security/authc/AuthenticationService.java

+            }
+        }
+
+        private String[] mergeAnonymousRoles(String[] existingRoles) {


nit: this implies that we have some special handling when merging the anonymous roles, while we just merge the two String arrays. I get that this is cleaner than doing it twice above but maybe a more generic mergeRoles where you pass both existingRoles and anonymoysUser.roles? Just a suggestion though

This is a good suggestion. It is cleaner that way. I updated.

jkakavas · 2020-03-24T08:15:43Z

...gin/security/src/main/java/org/elasticsearch/xpack/security/authc/AuthenticationService.java

+
+        private String[] mergeAnonymousRoles(String[] existingRoles) {
+            String[] mergedRoles = new String[existingRoles.length + anonymousUser.roles().length];
+            System.arraycopy(existingRoles, 0, mergedRoles, 0, existingRoles.length);


Shouldn't we create a Set here and Collections.addAll so that we don't end up with duplicate roles in case the anonymous roles contains a role the user already has ?

I intentially didn't do deduplication here. Deduplication, filtering out un-resolvable roles, preempting the superuser role are all done in CompositeRolesStore#getRoles, which comes after authentication.

Even without the anonymous roles, it is still possible to create an user with duplicated roles, e.g.
{"roles":["x","x","x"],"password":...} and these roles will all show up in the authentication response. So I decided to retain the existing behaviour.

I don't think we should entertain this existing behavior for no reason, and since we go in the trouble of merging the roles we should do de-duplication here . We could tackle this in a follow up where we for instance should not allow a user to be created with "roles" :["x","x","x"] in the first place as this leniency makes no sense and I would argue that if someone created a user with "roles" :["x","x"] , they probably meant "roles" :["x","y"] and mistyped , so it's better to tell them up front/

Merging only happens when anonymous access is enabled. If we de-duplicate here, it must also be applied when anonymous access is not enabled. So I re-arranged the code.

jkakavas · 2020-03-24T08:18:20Z

Added @tvernum as a reviewer to get his view on the behavior change since he wasn't in the meeting where we discussed this and decided to proceed this way

…role

ywangd · 2020-04-09T01:26:47Z

Per discussion: updated to pull role deduplication earlier in the process. This is only necessary for FileUserRolesStore and NativeUsersStore. UserRoleMapper interfaces already declares user roles to be returned as a Set.

…role

ywangd · 2020-04-22T08:45:50Z

ping @tvernum

tvernum

LGTM

…role

…lastic#53453) Anonymous roles resolution and user role deduplication are now performed during authentication instead of authorization. The change ensures: * If anonymous access is enabled, user will be able to see the anonymous roles added in the roles field in the /_security/_authenticate response. * Any duplication in user roles are removed and will not show in the above authenticate response. * In any other case, the response is unchanged. It also introduces a behaviour change: the anonymous role resolution is now authentication node specific, previously it was authorization node specific. Details can be found at elastic#47195 (comment)

…53453) (#55995) Anonymous roles resolution and user role deduplication are now performed during authentication instead of authorization. The change ensures: * If anonymous access is enabled, user will be able to see the anonymous roles added in the roles field in the /_security/_authenticate response. * Any duplication in user roles are removed and will not show in the above authenticate response. * In any other case, the response is unchanged. It also introduces a behaviour change: the anonymous role resolution is now authentication node specific, previously it was authorization node specific. Details can be found at #47195 (comment)

…cation (elastic#53453) (elastic#55995)" This reverts commit 84a2f1a.

…cation (elastic#53453)" This reverts commit 3fa765a.

…cation (#53453)" (#57853) This reverts commit 3fa765a.

…cation (elastic#53453) (elastic#55995)" This reverts commit 84a2f1a.

…cation (#53453) (#55995)" (#57857) This reverts commit 84a2f1a.

…cation (#53453) (#55995)" (#57858) This reverts commit 84a2f1a.

ywangd added >enhancement WIP :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC v8.0.0 v7.7.0 labels Mar 12, 2020

ywangd commented Mar 12, 2020

View reviewed changes

ywangd removed the WIP label Mar 12, 2020

ywangd requested a review from jkakavas March 12, 2020 08:29

jkakavas requested changes Mar 13, 2020

View reviewed changes

ywangd mentioned this pull request Mar 19, 2020

Authenticate API should return the roles that are inhereted from enabled anonymous access #47195

Closed

ywangd force-pushed the es-47195-anonymous-role branch from 149c6f8 to 19a3307 Compare March 20, 2020 05:03

ywangd added 2 commits March 20, 2020 16:22

Move anonymous role merging from authz to authc

8dac757

Update rest tests

09df4fb

ywangd force-pushed the es-47195-anonymous-role branch from 3dca628 to 09df4fb Compare March 20, 2020 05:23

ywangd added 4 commits March 20, 2020 16:43

Add unit tests for authenticationService

e24072a

Remove unnecessary authorizationService test

2cf8516

Fix test

8c5531b

style

903b805

ywangd requested a review from jkakavas March 20, 2020 10:06

ywangd added >bug and removed >enhancement labels Mar 20, 2020

jkakavas requested changes Mar 24, 2020

View reviewed changes

jkakavas requested a review from tvernum March 24, 2020 08:17

ywangd added 3 commits March 24, 2020 19:31

Merge remote-tracking branch 'origin/master' into es-47195-anonymous-…

c171757

…role

Address feedback

c43f8ba

Checkstyle

857c9e4

ywangd added 2 commits April 8, 2020 19:20

Remove unnecessary tests

6af416c

Update tests

2892218

Merge remote-tracking branch 'origin/master' into es-47195-anonymous-…

c6a8aa1

…role

tvernum approved these changes Apr 29, 2020

View reviewed changes

Merge remote-tracking branch 'origin/master' into es-47195-anonymous-…

c63c49f

…role

ywangd merged commit 3fa765a into elastic:master Apr 30, 2020

ywangd added the backport pending label Apr 30, 2020

ywangd mentioned this pull request Apr 30, 2020

Resolve anonymous roles and deduplicate roles during authentication (#53453) #55995

Merged

jkakavas removed the backport pending label May 5, 2020

tvernum mentioned this pull request Jun 5, 2020

Anonymous role resolution change may break stored Authentication objects #57711

Closed

ywangd added a commit to ywangd/elasticsearch that referenced this pull request Jun 5, 2020

Revert "Resolve anonymous roles and deduplicate roles during authenti…

9df8697

…cation (elastic#53453) (elastic#55995)" This reverts commit 84a2f1a.

ywangd added a commit to ywangd/elasticsearch that referenced this pull request Jun 9, 2020

Revert "Resolve anonymous roles and deduplicate roles during authenti…

786b855

…cation (elastic#53453)" This reverts commit 3fa765a.

ywangd mentioned this pull request Jun 9, 2020

Revert "Resolve anonymous roles and deduplicate roles during authenti… #57853

Merged

ywangd added a commit that referenced this pull request Jun 9, 2020

Revert "Resolve anonymous roles and deduplicate roles during authenti…

bf268c5

…cation (#53453)" (#57853) This reverts commit 3fa765a.

ywangd mentioned this pull request Jun 9, 2020

Revert "Resolve anonymous roles and deduplicate roles during authenti… #57857

Merged

ywangd added a commit to ywangd/elasticsearch that referenced this pull request Jun 9, 2020

Revert "Resolve anonymous roles and deduplicate roles during authenti…

e6ebb67

…cation (elastic#53453) (elastic#55995)" This reverts commit 84a2f1a.

ywangd mentioned this pull request Jun 9, 2020

Revert "Resolve anonymous roles and deduplicate roles during authenti… #57858

Merged

ywangd added a commit that referenced this pull request Jun 9, 2020

Revert "Resolve anonymous roles and deduplicate roles during authenti…

1a5e3d1

…cation (#53453) (#55995)" (#57857) This reverts commit 84a2f1a.

ywangd added a commit that referenced this pull request Jun 10, 2020

Revert "Resolve anonymous roles and deduplicate roles during authenti…

72a6441

…cation (#53453) (#55995)" (#57858) This reverts commit 84a2f1a.

ywangd mentioned this pull request Aug 20, 2020

Report anonymous roles in authenticate response #61355

Merged

Mpdreamz mentioned this pull request Nov 16, 2020

7.10.1 Meta Ticket elastic/elasticsearch-net#5096

Closed

61 tasks

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

ywangd mentioned this pull request Feb 4, 2022

Authentication under domains #82639

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve anonymous roles and deduplicate roles during authentication #53453

Resolve anonymous roles and deduplicate roles during authentication #53453

ywangd commented Mar 12, 2020 •

edited

Loading

elasticmachine commented Mar 12, 2020

ywangd Mar 12, 2020

jkakavas Mar 13, 2020

ywangd Mar 13, 2020

jkakavas Mar 13, 2020

ywangd Mar 13, 2020

ywangd Mar 16, 2020

jkakavas Mar 17, 2020

ywangd Mar 18, 2020

ywangd commented Mar 12, 2020

jkakavas Mar 13, 2020

ywangd Mar 13, 2020

jkakavas Mar 13, 2020

ywangd commented Mar 20, 2020

jkakavas Mar 24, 2020

ywangd Mar 24, 2020

jkakavas Mar 24, 2020

ywangd Mar 24, 2020

jkakavas Mar 24, 2020

ywangd Mar 24, 2020

jkakavas Mar 24, 2020

ywangd Mar 24, 2020

jkakavas commented Mar 24, 2020

ywangd commented Apr 9, 2020

ywangd commented Apr 22, 2020

tvernum left a comment

Resolve anonymous roles and deduplicate roles during authentication #53453

Resolve anonymous roles and deduplicate roles during authentication #53453

Conversation

ywangd commented Mar 12, 2020 • edited Loading

elasticmachine commented Mar 12, 2020

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 commented Mar 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywangd commented Mar 20, 2020

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

jkakavas commented Mar 24, 2020

ywangd commented Apr 9, 2020

ywangd commented Apr 22, 2020

tvernum left a comment

Choose a reason for hiding this comment

ywangd commented Mar 12, 2020 •

edited

Loading