Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
jkakavas opened this issue Sep 27, 2019 · 10 comments · Fixed by #53453 or #61355
Closed
Assignees
Labels
>bug :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC

Comments

@jkakavas
Copy link
Member

Authenticated users inherit the roles that are specified for anonymous users. There is already an issue tracking making this more obvious in the documentation.

The _authenticate API should also return the anonymous roles in the response along with the roles that the authenticated user is assigned and/or mapped to.

@jkakavas jkakavas added >bug :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC labels Sep 27, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security

@tvernum
Copy link
Contributor

tvernum commented Mar 11, 2020

@ywangd I've assigned this to you, no rush, but I think we should get it sorted when we can.

I don't know whether we want to add the anonymous role to theroles field, or add a new anonymous_role field, or something like that, but I think you and @jkakavas could come up with an agreement on that.

@ywangd
Copy link
Member

ywangd commented Mar 11, 2020

Thanks @tvernum

@jkakavas From the surface, I'd prefer a new field for it because of clarity. Otherwise users may have new confusion from another direction, i.e. from "where are my missing roles" to "what are these extra roles". I'd personally name it to something like inherited_roles. But I understand it could be a pre-mature "optimization" unless we could extend this concept to tokens, api keys or something else.

Not sure whether there is any backwards compatibility issue for adding a new field. Hope it's not too much to justify a new field.

@jkakavas
Copy link
Member Author

We generally are fine with adding fields to responses, these are not considered breaking changes and we have the tooling in our serialization logic to take care of version guards. We recently ( in a minor in 6.x ) added the realm information in the _authenticate response too.

I agree that the new field fits the principle of least astonishment better

@ywangd
Copy link
Member

ywangd commented Mar 19, 2020

We had a fair amount of discussion since last post here. This issue is getting more involved and here is a status update based on the discussion during team meeting:

TLDR: We prefer to have anonymous roles listed under existing roles field. I personally prefer to solve the immediate problem first and create a separate issue for relocating the logic of anonymous role resolving.

First of all, on the contrary to the initial discussion, we now agree that the anonymous roles should be just part of the existing roles field instead of a new one. I think the single strong argument is from @jkakavas as the follows:

The purpose of the API is to tell us which roles the user has, but not why they have them.

The second question is where the anonymous role should be handled. The lifecycle of a role goes through following phases:

  1. Get role name (RoleMapper)
  2. Retrieve role descriptor (RolesStore, BiConsumer functional interface)
  3. Build role object (CompositeRolesStore, which is not a RolesStore)

In general, step 1 is handled in authentication phase, while step 2 and 3 are handled in authorization phase. However, the anonymous role is a special case in that its role names are handled in authorization phase, specifically CompositeRolesStore#getRoles. Consequently, these role names are not available in the authentication object. This has two implications:

  1. The anonymous role names are inconvenient to access when constructing the response for /_security/_authenticate. This belongs to the bug we are trying to fix here.
  2. The authentication object is no longer the source of truth for user roles, which is a deeper problem and can possibly be handled in a separate issue.

If we just focus on issue 1, we can add new logic in TransportAuthenticateAction to retrieve the full list of user roles from the transient data in threadContext, details can be found here.

For issue 2, we need to move some logic from CompositeRolesStore#getRoles to the authentication phase. Based on my reading, there is no a single choke point in authentication where this logic can be applied. We need at least add it in two places, Authenticator#handleNullToken and Authenticator#finishAuthentication. There will also be a behaviour change (possibly breaking change?). The anonymous role settings are node scope, current handling in the authorization phase means they could be different depends on which node performs authorization. If we move this logic into authentication, the anonymous role will then just depend on the node where authentication is performed, because the roles are transfered across wires as part of authentication and honored by all nodes.

Base on above analysis, I prefer to fix issue 1 first and create a separate issue to deal with issue 2. This way we treat the symptom first for immediate user benefit and buy more time to think through on issue 2 and its implications. Once issue 2 is fixed, it should be simple to leverage it to refactor issue 1 (most likely deleting code).

@jkakavas
Copy link
Member Author

There will also be a behaviour change (possibly breaking change?). The anonymous role settings are node scope, current handling in the authorization phase means they could be different depends on which node performs authorization. If we move this logic into authentication, the anonymous role will then just depend on the node where authentication is performed, because the roles are transfered across wires as part of authentication and honored by all nodes.

Authentication is by design node specific so I don't see any issue here.

Base on above analysis, I prefer to fix issue 1 first and create a separate issue to deal with issue 2.

++

@ywangd
Copy link
Member

ywangd commented Mar 19, 2020

Authentication is by design node specific so I don't see any issue here.

But that specific node will be different after the code change. Say we have a 2-node cluster, A and B and they have different anonymous role settings, a and b, respectively. The user authenticates against node A and request is also passed to node B from A.

Currently, the logic is as follows:

user  -->   A   -->   B
           (a)       (b)  <- runtime anonymous role

After the change, it will become

user  -->   A   -->   B
           (a)       (a)  <- runtime anonymous role

Note that the runtime anonymous role is change to a instead of b for node B . This could be what we want since it feels more inline with other authentication behaviours, i.e. authenticating node specific. But the behaviour is different nonetheless.

@jkakavas
Copy link
Member Author

Gotcha. I think I see it as current behavior is a bug and we should fix it, rather than we're making a breaking change in expected behavior.

@ywangd
Copy link
Member

ywangd commented Mar 20, 2020

I have a change of heart again. I'd like to tackle both issues together. Because fixing issue 1 alone feels hacky and the solution does not do exactly what I want in all cases. The role names are sanitized, deduplicated and compacted during authorization. Although they do represent the final runtime privileges, they could read quite different from those in the authentication object.
Given we consider the behaviour change of issue 2 as bug fixing, plus the fact that the combine solution is better than individual ones, I am working towards having both of them fixed.

ywangd added a commit that referenced this issue Apr 30, 2020
…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 #47195 (comment)
ywangd added a commit to ywangd/elasticsearch that referenced this issue Apr 30, 2020
…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)
ywangd added a commit that referenced this issue Apr 30, 2020
…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)
@ywangd
Copy link
Member

ywangd commented Jun 10, 2020

Reopenning the issue since the change is reverted with #57853

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC
Projects
None yet
4 participants