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

Include role name in Entity Alias metadata #160

Merged
merged 1 commit into from
Jan 2, 2024
Merged

Include role name in Entity Alias metadata #160

merged 1 commit into from
Jan 2, 2024

Conversation

stefan-zh
Copy link
Contributor

@stefan-zh stefan-zh commented Mar 2, 2021

Overview

This PR is suggesting the addition of the role name in the Entity Alias metadata on successful login.

Adding the role name on the Entity Alias metadata makes it easier to manage access to endpoints via ACL templated policies. Using the plugin mount accessor, one can simply use the following rule to restrict access: identity.entity.aliases.<mount_accessor>.metadata.role. Another auth plugin, AppRole, recently implemented this change for the same reason: hashicorp/vault#9529

Also, it already seems like the authors of this plugin had in mind that role is an important metadata that should not be overriden. We can assume this from the following two things. First, the only way to set metadata attributes on the Entity is through the claim_mappings property when creating an OIDC role (claim_mappings is the only way where properties from the OIDC JWT token will be mapped directly into the Entity Alias metadata). Second, we can see from the code that there is special reserved metadata role, which cannot be included in the claim_mappings: https://github.com/hashicorp/vault-plugin-auth-jwt/blob/main/path_role.go#L508. I can think only of 2 reasons why that is the case - 1) role is important to be present on the metadata without being overridden; 2) role must never appear in the metadata and should be excluded.

My assumption is that the developers of this plugin had option 1 in mind. However, as of now, the role metadata is not present on the Entity Alias metadata and cannot be included. Therefore, I am suggesting this PR to add role in the Entity Alias metadata.

Design of Change

The role name is added directly on the Entity Alias metadata on a successful login in the 2 available flows:

  • the OIDC flow with a browser redirect
  • the JWT flow

Related Issues/Pull Requests

None

Contributor Checklist

[ ] Add relevant docs to upstream Vault repository, or sufficient reasoning why docs won’t be added yet
My Docs PR Link
Example
[ ] Add output for any tests not ran in CI to the PR description (eg, acceptance tests)
[X] Backwards compatible

@hashicorp-cla
Copy link

hashicorp-cla commented Mar 2, 2021

CLA assistant check
All committers have signed the CLA.

@stefan-zh stefan-zh marked this pull request as ready for review March 2, 2021 09:12
@stefan-zh
Copy link
Contributor Author

Tagging @fairclothjm and @austingebauer to take a look at this improvement.

@austingebauer
Copy link
Contributor

Hi, @stefan-zh. This seems like a reasonable request. It's been requested before in hashicorp/vault-plugin-auth-kubernetes#111 without objection. I'm double checking that there aren't any gotchas before approving this.

@stefan-zh
Copy link
Contributor Author

Thanks @austingebauer ! Only thing I'm not sure is whether to call it role or role_name as is the case with approle: https://github.com/hashicorp/vault/blob/main/builtin/credential/approle/path_login.go#L366

@austingebauer
Copy link
Contributor

@stefan-zh - My preference is role similar to how it's specified for token metadata. I'd just like it to be consistent for both token and alias metadata (like it is for approle, although using a different key name). How does that sound?

@jbayer
Copy link
Member

jbayer commented Dec 20, 2023

@austingebauer when I was playing with https://github.com/jbayer/vault-clients-for-humans and trying to get client metadata I was wondering what other Auth Methods do for setting up Entity Alias metadata values. As pointed out in this issue, AppRole does call it role_name. What do other Auth Methods like Kubernetes Auth Method do if anything? It would be helpful if there was a consistent attribute name that could be expected to be populated in the Entity Alias metadata. If we use role in this JWT Auth Method, then should we add role in AppRole Auth Method?

@stefan-zh
Copy link
Contributor Author

stefan-zh commented Dec 20, 2023

@stefan-zh - My preference is role similar to how it's specified for token metadata. I'd just like it to be consistent for both token and alias metadata (like it is for approle, although using a different key name). How does that sound?

I have no strong preference here, so I'm fine to whatever we agree to. Only counter-argument I can bring in is that role can be meaningfully attributed to the Role object, while role_name is just the string.

@austingebauer
Copy link
Contributor

@jbayer - I did a survey of the auth methods, and approle is the only one that sets the role name as alias metadata. Adding the role name has been requested in the Kubernetes auth method but has yet to be added. I agree that we should be consistent across auth methods. I think we'll want to add role to approle instead of use role_name here (explanation below). The JWT auth method is more flexible than most auth methods when it comes to configuration of alias metadata via claim mappings.

@stefan-zh - I forgot to mention another reason that I prefer role here is that it's already a reserved key which cannot be used for claim mappings. If we chose to use role_name, there is a chance someone is already using that in their claim mappings. A collision could at worst change their ACL policies if using templating and at best be confusing. Let me know your thoughts on this.

@jbayer
Copy link
Member

jbayer commented Dec 21, 2023

Thanks @austingebauer, your explanation makes sense and I appreciate your summary of the behavior of the other Auth Methods.

@stefan-zh
Copy link
Contributor Author

@austingebauer I agree with you on that point for using role. Then, my PR can be reviewed as is.

@austingebauer
Copy link
Contributor

Thanks, both! Reviewing this PR as is 👍

@austingebauer austingebauer self-requested a review December 22, 2023 17:17
Copy link
Contributor

@austingebauer austingebauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this, @stefan-zh! Looks good to me. It will eventually be released with Vault 1.16.

@austingebauer austingebauer merged commit 3bb8ab9 into hashicorp:main Jan 2, 2024
1 check passed
@stefan-zh stefan-zh deleted the feature/include-role-in-entity-alias-metadata branch January 3, 2024 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants