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

Token identity support #6267

Merged
merged 16 commits into from
Jul 1, 2019
Merged

Token identity support #6267

merged 16 commits into from
Jul 1, 2019

Conversation

michelvocks
Copy link
Contributor

@michelvocks michelvocks commented Feb 20, 2019

With the support of ACL Templating, which allows users to dynamically build paths for the ACL system, it is requested to support the identity system for the token backend. If, for example, tokens created via a token role should have access to a dynamic path where a key/value secret is stored, the entity metadata could be used to dynamically form the related ACL policy.

One example use-case is Nomad’s integration with Vault. Nomad can automatically create Vault tokens with predefined policies which can be a subset of the originally provided policies. However, to use the ACL templating engine to dynamically build the policies paths, it is required that the entity attached to a newly created token can be dynamically modified.

This PR allows the modification of the entity alias of a token during token creation via a token role. Additionally, the entity alias modification is scoped by a predefined list of allowed entity aliases attached to the token role.

vault/token_store.go Outdated Show resolved Hide resolved
@michelvocks michelvocks force-pushed the token_identity_support branch from 45f3fc4 to 06a30ab Compare April 5, 2019 13:36
@michelvocks michelvocks changed the title [WIP] Token identity support Token identity support Apr 9, 2019
@michelvocks michelvocks marked this pull request as ready for review April 9, 2019 07:31
@michelvocks michelvocks force-pushed the token_identity_support branch from 5879275 to 7575a5a Compare April 25, 2019 15:40
@michelvocks michelvocks force-pushed the token_identity_support branch from 464ecd5 to 0856c6b Compare May 14, 2019 06:35
@michelvocks michelvocks added this to the 1.2 milestone May 14, 2019
@michelvocks michelvocks force-pushed the token_identity_support branch from 0856c6b to 17cc14a Compare May 28, 2019 18:46
@michelvocks michelvocks force-pushed the token_identity_support branch from 17cc14a to 82d2abf Compare June 6, 2019 14:07
@michelvocks michelvocks force-pushed the token_identity_support branch from b759a8d to 8726f53 Compare June 7, 2019 14:14
@chrishoffman chrishoffman self-requested a review June 13, 2019 13:41
@tyrannosaurus-becks
Copy link
Contributor

I think docs might also need to be updated here.

@tyrannosaurus-becks
Copy link
Contributor

I think this part needs to be updated too.

@tyrannosaurus-becks
Copy link
Contributor

Also the sample responses in the docs.

@michelvocks
Copy link
Contributor Author

@tyrannosaurus-becks Thanks for the review. I've updated the PR.

Also the sample responses in the docs.

I don't really understand why the sample responses should be updated?
The default behavior and output should not change.

Cheers,
Michel

@tyrannosaurus-becks
Copy link
Contributor

@michelvocks I was thinking the read role doc might need to be updated. Mainly because I thought, what if someone were building a client in another language against these API endpoints, maybe they would wonder what an entity_alias looks like in a read response. I think some of the things in our API, generally, can be tricky - durations, for instance, can be sent as strings like "10s" but then in a read response I think they can be reflected as an int showing nanoseconds. Anyways, I think there are a couple of other sample responses where it might be nice to add examples of the new fields if they're included.

@michelvocks michelvocks force-pushed the token_identity_support branch from be14a2c to 42717af Compare June 25, 2019 07:31
@michelvocks
Copy link
Contributor Author

@tyrannosaurus-becks Thanks for the clarification. That makes sense. I've added a few more samples. Would be great if you could have another look.

Copy link
Contributor

@chrishoffman chrishoffman left a comment

Choose a reason for hiding this comment

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

Looking good.

vault/token_store.go Outdated Show resolved Hide resolved
vault/token_store.go Show resolved Hide resolved
vault/token_store.go Outdated Show resolved Hide resolved
vault/token_store.go Outdated Show resolved Hide resolved
vault/token_store.go Outdated Show resolved Hide resolved
vault/token_store.go Outdated Show resolved Hide resolved
@chrishoffman
Copy link
Contributor

Can you also add a description to this PR? It would be good to talk about what the use-case is here in case we ever want to reference it in the future.

vault/token_store.go Outdated Show resolved Hide resolved
jefferai
jefferai previously approved these changes Jun 27, 2019
Copy link
Member

@jefferai jefferai left a comment

Choose a reason for hiding this comment

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

Two minor things, looks good otherwise!

Copy link
Contributor

@tyrannosaurus-becks tyrannosaurus-becks left a comment

Choose a reason for hiding this comment

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

I second that!

@michelvocks michelvocks dismissed stale reviews from tyrannosaurus-becks and jefferai via a7b05ae June 27, 2019 16:42
chrishoffman
chrishoffman previously approved these changes Jun 28, 2019
@michelvocks michelvocks force-pushed the token_identity_support branch from a7b05ae to 9e9ef84 Compare July 1, 2019 09:19
@michelvocks michelvocks merged commit e7ed739 into master Jul 1, 2019
@michelvocks michelvocks deleted the token_identity_support branch July 1, 2019 09:39
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.

5 participants