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

Add token ACL templating support #7761

Closed
wants to merge 2 commits into from
Closed

Conversation

nbrownus
Copy link
Contributor

@nbrownus nbrownus commented Oct 30, 2019

Adds the ability to target token.metadata.* in policy templates

@hashicorp-cla
Copy link

hashicorp-cla commented Oct 30, 2019

CLA assistant check
All committers have signed the CLA.

@nbrownus
Copy link
Contributor Author

I do not believe the test-go-race failures are a part of this change.

@jefferai
Copy link
Member

I don't think we want to merge this; display name is usually unreliable and can often be set by relatively unprivileged users.

What about Identity doesn't work here? What's the use case?

@nbrownus
Copy link
Contributor Author

nbrownus commented Oct 30, 2019

More than happy to drop display_name. Our use case is specifically around token.metadata. I did not find a way to reasonably manipulate identity via a plugin that did not impose additional work on the policy writer.

Identity looks like a great way to deal with dynamic policy, but the requirement for mount accessor, which is dynamically keyed, makes it cumbersome to deal with in an environment where the auth plugin needs to change on any cadence.

Being able to target properties on the token would be incredibly helpful, even if those properties were limited to logical.Auth.InternalData somehow.

Very willing to make any changes you all deem necessary, avoiding dynamically generated identifiers in dynamic (templated) policies is our goal.

@VioletHynes
Copy link
Contributor

Hi there! Thanks for the PR. We discussed this internally, as I wanted to make sure to get you an answer for this PR as it's on the older side. Unfortunately, this isn't something we're planning to support.

See also: #10460 + #10682

In particular, there are two parts of our responses to these that explain our position and hopefully offer an alternative. Hopefully the following answers satisfy you:

I think you can achieve the result you're aiming for by creating an entity and alias for each role that needs distinct policies, put the metadata into the entity, and bind the entity alias to the token role.

and

Thank you for the contribution. Unfortunately I'm afraid that we have decided to refuse it. We view this change as dangerous, since a user can create child tokens with whatever arbitrary metadata they like. This means policy templating using token metadata is vulnerable to any attacker who's aware of what policies are in use. I expect that you could come up with safeguards in your environment to protect against this (e.g. not granting access to the create token endpoints), but we as vault devs have to be vigilant against introducing features that allow for insecure by default behaviour.

Thanks for raising this, and we appreciate the contribution. However, we feel like the 'right' way to do this is using the identity system. Token metadata is quite low privilege.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants