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

Changes the vault issuer Kubernetes auth path to require the full *mount* path #2349

Merged
merged 2 commits into from
Nov 13, 2019

Conversation

JoshVanL
Copy link
Contributor

@JoshVanL JoshVanL commented Nov 11, 2019

ACTION REQUIRED
Users who have previously set the Kubernetes Auth Mount Path will need to update their manifests to include the entire mount path. The `/login` endpoint is added for you.

Changes the Vault Kubernetes Auth Path to require the entire mount path. `/login` is added to all mount paths when authenticating.
The default auth path has now changed from `kubernetes` to `/v1/auth/kubernetes`

fixes #2205

@jetstack-bot jetstack-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. labels Nov 11, 2019
@jetstack-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoshVanL

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jetstack-bot jetstack-bot added area/api Indicates a PR directly modifies the 'pkg/apis' directory area/deploy Indicates a PR modifies deployment configuration approved Indicates a PR has been approved by an approver from all required OWNERS files. area/testing Issues relating to testing size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 11, 2019
@JoshVanL JoshVanL added this to Review in progress in v0.12 Nov 11, 2019
Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>
@jetstack-bot jetstack-bot added the kind/documentation Categorizes issue or PR as related to documentation. label Nov 11, 2019
@JoshVanL
Copy link
Contributor Author

/assign @munnerz

@@ -38,5 +38,5 @@ const (

// Default mount path location for Kubernetes ServiceAccount authentication
// (/v1/auth/kubernetes/login)
DefaultVaultKubernetesAuthMountPath = "kubernetes"
DefaultVaultKubernetesAuthMountPath = "/v1/auth/kubernetes/login"
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to hardcode the login part of this? From what I understand, the /v1/auth/kubernetes path is the mount point of the kubernetes auth backend - I don't know if we ever need (or even want) users to override the suffix. cc @mam8270 have you got any thoughts on this?

Copy link

Choose a reason for hiding this comment

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

Login can be a bit more nuanced now with the vault enterprise "namespaces" feature, which essentially allows vault inside vault for the different namespaces (including auth endpoints), so i think you would want at least some mechanism for overriding of the login url. I agree you would lose some safety though.
https://learn.hashicorp.com/vault/operations/namespaces#policy-with-namespaces

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's the conclusion here? If the path is wrong, a user can always override the path completely which is the point of this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the link @mam8270 - I'm still a bit unclear on this though, the link you reference looks like it refers to prefixes on the Vault path, not the endpoint within the Kubernetes auth backend that is used to actually authenticate (i.e. /login).

Does this enterprise 'namespacing' feature allow this endpoint to be changed? If so, we will keep this line as is.

If not, we can make this field refer to the mount point of the auth backend instead of the particular endpoint used to authenticate (i.e. /v1/auth/kubernetes vs /v1/auth/kubernetes/login).

We want to allow people as much flexibility as possible, but if it isn't possible to specify a value other than login, we can gain a bit of 'safety' and save users making configuration errors.

Copy link
Member

Choose a reason for hiding this comment

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

To note, mountPath is the name of the field here.. which IMO implies /v1/auth/kubernetes should be a valid value, and /v1/auth/kubernetes/login isn't (as it is not hte mount path)

Copy link
Member

Choose a reason for hiding this comment

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

We're going to go with /v1/auth/kubernetes for now due to the field being named mountPath. If we need more flexibility in future, we can introduce a path field instead.

@munnerz
Copy link
Member

munnerz commented Nov 11, 2019

Could you also add ACTION REQUIRED at the start of your release note to note that users of the kubernetes auth backend will need to update their issuer resources?

@jetstack-bot jetstack-bot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Nov 12, 2019
Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>
@JoshVanL JoshVanL changed the title Changes the vault issuer Kubernetes auth path to require the full path Changes the vault issuer Kubernetes auth path to require the full *mount* path Nov 12, 2019
@JoshVanL
Copy link
Contributor Author

/retest

@munnerz
Copy link
Member

munnerz commented Nov 13, 2019

/lgtm

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 13, 2019
@jetstack-bot jetstack-bot merged commit 1bfec37 into cert-manager:master Nov 13, 2019
v0.12 automation moved this from Review in progress to Done Nov 13, 2019
@jetstack-bot jetstack-bot added this to the v0.12 milestone Nov 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/api Indicates a PR directly modifies the 'pkg/apis' directory area/deploy Indicates a PR modifies deployment configuration area/testing Issues relating to testing dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. kind/documentation Categorizes issue or PR as related to documentation. lgtm Indicates that a PR is ready to be merged. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
No open projects
v0.12
  
Done
Development

Successfully merging this pull request may close these issues.

Support non root auth urls when logging into vault
4 participants