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

[Feature/Extensions] Include backend roles in on-behalf-of token in CreateOnBehalfOfTokenAction #2865

Closed
Tracked by #2573
cwperks opened this issue Jun 15, 2023 · 9 comments
Assignees
Labels
bug Something isn't working triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable.

Comments

@cwperks
Copy link
Member

cwperks commented Jun 15, 2023

For on-behalf-of tokens its important that the roles in the token refer to the mapped roles of the user which corresponds to the same mappedRoles in the PrivilegesEvaluator here. The mapped roles are ultimately what is used to evaluate privileges and in order to accurately compute the mapped roles the IP Address of the caller is required which would only be available in the node that receives the REST Request and issues an on-behalf-of token.

For the Create Token endpoint the handler for the endpoint should likewise call mapRoles so that it has access to the correct caller's information when computing the mapped roles to embed in the token.

As we were scoping out work for extensions we identified that many plugins rely on backend roles and one way of communicating backend roles to an extension is through a claim in the token. While it may not be necessarily required to have backend_roles as a claim in an on-behalf-of token, if they are to be used for extensions then that is the chosen mechanism for communicating that data to an extension.

The NoopAuthenticationBackend and AuthCredentials classes also need to be updated to support Security Roles (not only Backend Roles.

@cwperks cwperks added bug Something isn't working untriaged Require the attention of the repository maintainers and may need to be prioritized labels Jun 15, 2023
@peternied
Copy link
Member

peternied commented Jun 15, 2023

Ensuring we have the full list of mapped roles of the user when the token is generated, 100% agree. I'd be game to codify this with a language change throughout the codebase toward mappedRoles - including migrating to security index v8.

Backend roles are controlled outside of the OpenSearch ecosystem, changes to the backend roles on a user are handled by the roles mapping configured on the cluster. If there are cases where missing backend roles would cause an action to be authorized instead of denied that sounds like a CVE.

@cwperks
Copy link
Member Author

cwperks commented Jun 15, 2023

backend roles are utilized in the roles mapping process to map to (internal) security roles - that is roles that are defined in the roles section of the security index. There are 4 types of role mappings:

  1. Direct user mapping - i.e. UserA is mapped to role1
  2. Host mapping - i.e. all requests from 127.0.0.1 are mapped to role2
  3. Backend Role mapping - i.e. backendRoleA is mapped to roleA
  4. And_Backend_Role mapping - i.e. userA must have both backendRole1 AND backendRole2 to be mapped to roleB

backend roles do not directly give permissions, backend roles resolve to roles via role mappings.

@davidlago
Copy link

If there are cases where missing backend roles would cause an action to be authorized instead of denied that sounds like a CVE.

@peternied I'm not sure I follow... what do you mean by "missing a backend role", as in we did not capture it correctly when the call came in, or we did not appropriately map it to an internal role, or it just got unassigned at the external IdP by mistake?

@cwperks just to be clear: adding this is for backwards compatibility reasons, right? I never quite got why plugins needed to tap into the backend roles to meet their additional AuthZ requirements on top of what core+security plugin provide.

@peternied
Copy link
Member

what do you mean by "missing a backend role"

Here is a quick setup, for a couple of cases GreenGiant, with BackendRoles of "Green" and "Giant". There is a backend role mapping for "Giant" to "Jolly". Their user object will kind of look like { BERoles: ["Green", "Giant"], MappedRoles: ["Jolly"] }. If this information was serialized as part of a plugins object, BeanStalk, if a plugin were operating on this object the following scenarios come to mind.

Backend role is added

GreenGiant joins the marketing team and gets "Telemarketer" added to their backend roles, no mapping.

  • BeanStalk could fail as the same because the BERoles no longer match.
  • Beanstalk could allow the mismatch because BeanStalk's BERoles are a subset of GreenGiants BERoles.

Backend role is removed/altered

GreenGiant is rebranded to be Red, the Green backend role is switched to Red.

  • BeanStalk could fail as the same because the BERoles no longer match.
  • BeanStalk could fails as its BERoles contain values that are not in GreenGiants BERoles.
  • BeakStalk could allow the mismatch because some of the BERoles are in GreenGiant's BERoles.

No Backend Roles (proposed)

OnBehalfOf token is created for GreenGiant without any backend role data.

  • BeanStalk could fail as the same because the BERoles no longer match.
  • BeanStalk could fail as the same because the none of the roles in BERoles match GreenGiant's BERoles.

Far as Security plugin is concerned the only access control that we will enforce is around if GreenGiant is Jolly, not of the other changes have an impact unless they alter the mapped roles. In the cases I've outlined, access isn't granted if a user does not have a role, and if that was the case that would be a security bug.

When an OnBehalfOf token is used against a resource that depend on BERoles it would deny access. Rather than change the token, the plugin could transition to a better mechanism than BERoles at all.

@davidlago
Copy link

Far as Security plugin is concerned the only access control that we will enforce is around if GreenGiant is Jolly, not of the other changes have an impact unless they alter the mapped roles. In the cases I've outlined, access isn't granted if a user does not have a role, and if that was the case that would be a security bug.

Gotcha, we are on the same page ☝️ The use of backend roles to do authorization in some plugins is something we want to stop doing. The question I still have is whether we need to add them for backwards compatibility reasons

@cwperks just to be clear: adding this is for backwards compatibility reasons, right? I never quite got why plugins needed to tap into the backend roles to meet their additional AuthZ requirements on top of what core+security plugin provide.

Right now, users of plugins who do rely on these backend roles have their cluster permissions set up so that they manage these things externally in their IdPs without worrying much about mappings (barring plugin-level permissions of course). And perhaps that is why this shortcut was taken? But regardless of why, if we shift away from back-end roles, that constitutes a breaking change for end users... or am I missing something?

@cwperks
Copy link
Member Author

cwperks commented Jun 16, 2023

@davidlago See this section on AD security on the documentation website: https://opensearch.org/docs/latest/observing-your-data/ad/security/#advanced-limit-access-by-backend-role

For the case of internal users, admins would add backend roles manually through the security pages in OSD and add the same backend role to all users they would like to share a detector with. IMO its odd that you can add backend roles to an internal user, but that's the way it is.

@davidlago
Copy link

but that's the way it is.

Yep! I think we are in agreement that the current system is not what we want. Now the decision is whether/how to include those back-end roles in the OBO tokens for backwards compatibility reasons only.

@scrawfor99
Copy link
Collaborator

[Triage] Hi @cwperks, thank you for filing this thorough discussion issue. There is a lot of good discussion here. I am going to assign it to you for resolution since you seem to be driving.

@scrawfor99 scrawfor99 added triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable. and removed untriaged Require the attention of the repository maintainers and may need to be prioritized labels Jun 19, 2023
@davidlago
Copy link

Closing in favor of #2616

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable.
Projects
None yet
Development

No branches or pull requests

4 participants