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

Use display name as CN #6457

Closed
wants to merge 4 commits into from
Closed

Use display name as CN #6457

wants to merge 4 commits into from

Conversation

andrejvanderzee
Copy link
Contributor

@andrejvanderzee andrejvanderzee commented Mar 22, 2019

We require a way to force the display name into certificates because of security and audit logging. Consider the following use-case (vault-helper is our command-line tool that abstracts from raw vault calls used by our tech people):

(1) vault-helper auth -role devops -username me 
password: *****
(2) vault-helper k8s -cluster production

In step (1) we login and the DisplayName is set to me. In step (2) we fetch a client certificate for Kubernetes production environment. This is were it gets shady. If someone manually calls vault production/pki/issue/devops and provides common_name=you, the Kubernetes audit logs will trace back to you, instead of me.

Of course we can correlate k8s audit logs with Vault audit logs, but we rather have the guarantee that no one can temper with the CN.

This PR adds the option use_displayname_cn to the pki/issue/:role endpoint that supports the above use-case.

@andrejvanderzee andrejvanderzee changed the title Use display name as CN. Use display name as CN Mar 22, 2019
@jefferai
Copy link
Member

Relying on display name is very fragile and not very secure. Instead this should use identity templating and get the info from the entity associated with the token.

@jefferai
Copy link
Member

This is more or less a dup of #4157

@andrejvanderzee
Copy link
Contributor Author

andrejvanderzee commented Mar 25, 2019

@jefferai Thanks, thats a duplicate indeed and will have a look at identity templating soon.

Can you explain why allow_token_displayname is not documented on the webpage at all? Cause looking at the source code, we could probably enforce display name by setting these in the role config:

  1. allowed_domains="*.example.com"
  2. allow_glob_domains=false
  3. allow_token_displayname=true

@jefferai
Copy link
Member

Backwards compat. We allowed it a long time ago before realizing it was not a good idea. But we didn't want to bust any roles that had already been configured. Given that you can't configure a role with that option likely it's dead code at this point but it's not doing any harm having it in there.

@andrejvanderzee
Copy link
Contributor Author

Close in favor of #6558.

@andrejvanderzee andrejvanderzee deleted the use_displayname_cn branch April 9, 2019 21:33
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.

2 participants