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

[Do not merge] Identity templating support for allowed_domains-option for PKI secret engine's issue API call #6558

Closed
wants to merge 6 commits into from
Closed

[Do not merge] Identity templating support for allowed_domains-option for PKI secret engine's issue API call #6558

wants to merge 6 commits into from

Conversation

andrejvanderzee
Copy link
Contributor

@andrejvanderzee andrejvanderzee commented Apr 9, 2019

This MR applies identity templating to the allowed_domains option of the PKI secret engine's issue API call. For example, the PKI secret engine configuration below only allows domains that are equal to the auth method alias's name.

secret_engine "k8s-apiserver" {
   ...
  path "roles/devops" {
    allowed_domains = "{{identity.entity.aliases.auth_plugin_05c79452.name}}"
    enforce_hostnames = false
    allow_bare_domains = true
    server_flag = false
    client_flag = true
  }
}

This is an improvement over the allow_token_displayname, which is considered insecure and deprecated, in favor of using exported entity data with more attributes like aliases and metadata, from the authentication methods to secret engines.

We have tested the MR at our testing environment, and it works as a solution for #6457.

@andrejvanderzee andrejvanderzee changed the title Identity templating to allowed_domains option for PKI secret engine's issue API call Identity templating support for allowed_domains-option for PKI secret engine's issue API call Apr 9, 2019
@andrejvanderzee
Copy link
Contributor Author

@jefferai Does this PR cover what you suggested about applying identity templating to CN constraints? Thanks.

@andrejvanderzee
Copy link
Contributor Author

@jefferai We still require a way to impose constraints on the CN of a PKI's issue API call. Would this MR based on identity templating cover what you suggested in #4157 (comment)?

Dont mind changing to explicit role parameters either. But the problem is at this moment is that we can only use the deprecated allow_token_displayname to mitigate impersonation via a false CN.

@jefferai
Copy link
Member

jefferai commented May 7, 2019

The mechanics are fine, I think there is just a larger question on which parameters such templating should be activated (existing, new, or both), and whether by default or not.

@dustin-decker
Copy link
Contributor

This PR looks good to me. It satisfies multiple feature requests and @jefferai's request for implementation via identity templating. I think it would be nice to enable identity templating on all RDNs, but CN is definitely the most common one, so can we start with that? I think enabled by default is fine, since it's controlled by the PKI role.

@jdollard
Copy link
Contributor

I've got a need to use support for templated roles in the allowed_uri_sans field too. I'm happy to extend this pull request and add support for this, plus also a unit test, early next week.

@jdollard
Copy link
Contributor

I created #7216 based off this PR. The new pull request includes template support for the allowed_uri_sans field and adds tests. It also avoids a null pointer dereference if EntityInfo returns nil, which may happen if token auth is being used.

I wasn't sure if creating a new PR or trying to get this one updated was the right approach. I decided to go ahead and make another PR, and can always cancel it if this PR is updated.

@jefferai jefferai changed the title Identity templating support for allowed_domains-option for PKI secret engine's issue API call [Do not merge] Identity templating support for allowed_domains-option for PKI secret engine's issue API call Aug 5, 2019
@carnei-ro
Copy link

Waiting for this feature!

@CH-rahulprabhu
Copy link

me too

@adammw
Copy link

adammw commented Apr 28, 2020

What was blocking this feature going forward? Any way to unblock those discussions?

@ncabatoff
Copy link
Collaborator

Closing because the PR author has a newer PR that implements the same feature (#8509).

@pbernal
Copy link
Contributor

pbernal commented Jun 12, 2020

Closing due to Nick's previous comment.

@pbernal pbernal closed this Jun 12, 2020
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.

10 participants