Skip to content

Adds Kubernetes authentication type for Vault Issuer#649

Closed
kunickiaj wants to merge 1 commit intocert-manager:masterfrom
kunickiaj:vault-k8s-auth
Closed

Adds Kubernetes authentication type for Vault Issuer#649
kunickiaj wants to merge 1 commit intocert-manager:masterfrom
kunickiaj:vault-k8s-auth

Conversation

@kunickiaj
Copy link
Copy Markdown
Contributor

@kunickiaj kunickiaj commented Jun 11, 2018 •
edited
Loading

What this PR does / why we need it:

Which issue this PR fixes
fixes #647

Special notes for your reviewer:
So I've actually put together a patch, and just trying to tie up the e2e tests. This requires that the VaultInitializer have a method that configures the kubernetes auth backend which requires at a minimum: the master url and CA cert.

Anyone happen to know what the right way to get these in there are?

Release note:

Adds Kubernetes authentication type for Vault Issuer

This auth method uses the cert-manager's service account token to request a Vault token. See the Vault documentation for instructions on setting up the auth backend as well as the necessary role bindings on the Kubernetes side.

Example configuration for using this auth method:

apiVersion: certmanager.k8s.io/v1alpha1
kind: Issuer
metadata:
  name: vault-issuer
  namespace: default
spec:
  vault:
    path: pki_int/sign/example-dot-com
    server: https://vault
    auth:
      kubernetes:
        role: "cert-manager-role"

@jetstack-bot jetstack-bot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jun 11, 2018
@jetstack-bot jetstack-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 11, 2018
@munnerz
Copy link
Copy Markdown
Member

munnerz commented Jun 12, 2018

/ok-to-test

@jetstack-bot jetstack-bot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 12, 2018
@kunickiaj
Copy link
Copy Markdown
Contributor Author

kunickiaj commented Jun 13, 2018

Are there any guides on how to work with the e2e tests? For testing a new auth backend, the Vault instance needs to have the backend enabled and configured--which requires the k8s master url and CA cert. It wasn't clear from the current setup if there's any way to get those in the VaultInitializer.

@madjam002
Copy link
Copy Markdown

madjam002 commented Jul 9, 2018

@kunickiaj Just a suggestion, would be nice to have the kubernetes auth mount path configurable, just like this PR is trying to do for approle #612

At the moment it is hardcoded to /auth/kubernetes

@kunickiaj
Copy link
Copy Markdown
Contributor Author

kunickiaj commented Jul 9, 2018

Seems like a reasonable request--right now still stuck on how to make this testable given the dependency on k8s cluster parameters. Any ideas on how the test jobs are setup and how this could be done would be welcome.

@jetstack-bot jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 12, 2018
@munnerz
Copy link
Copy Markdown
Member

munnerz commented Jul 12, 2018

/assign @kragniz

Could you take a look at this and help out with the testing environment pieces?

@munnerz munnerz added the area/vault Indicates a PR directly modifies the Vault Issuer code label Jul 25, 2018
@munnerz
Copy link
Copy Markdown
Member

munnerz commented Sep 7, 2018

@kunickiaj sorry for the lack of response here - could you expand a bit more on the issues you're encountering with the test environment?

If you can rebase your PR, we should be able to get these e2e's running (and hopefully passing too!). There's not too much special about the environment (it is provisioned with minikube at the moment) - what specified requirements of that environment do you have? 😄

@kunickiaj
Copy link
Copy Markdown
Contributor Author

kunickiaj commented Sep 7, 2018

I'll get it rebased today. For the e2e environment, the Vault auth backend for k8s needs to have two parameters provided at a minimum: k8s master url and CA cert for the master. If its minikube hopefully that's fairly easy to make deterministic.

@jetstack-bot jetstack-bot added dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 7, 2018
@jetstack-bot jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 21, 2018
@jetstack-bot jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 15, 2018
@kunickiaj
Copy link
Copy Markdown
Contributor Author

kunickiaj commented Oct 15, 2018

Sorry for the delay, rebased the patch, still needs e2e setup.

@jetstack-bot jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 19, 2018
@rblaine95
Copy link
Copy Markdown

rblaine95 commented Nov 13, 2018

This requires that the VaultInitializer have a method that configures the kubernetes auth backend which requires at a minimum: the master url and CA cert.

Anyone happen to know what the right way to get these in there are?

banzaicloud/bank-vaults is a really good Vault Operator that has an automatic configuration toolset

Full disclosure: I'm a bank-vaults contributor

@vdesjardins
Copy link
Copy Markdown
Contributor

vdesjardins commented Nov 14, 2018

@kunickiaj

I didn't experiment with this Vault auth method much yet but maybe you could use the helper method in test/e2e/framework/util.go to fetch the config:

kubeConfig, err := LoadConfig(TestContext.KubeConfig, TestContext.KubeContext)
if (err != nil) {
...
}

apiHost := kubeConfig.Host
caCert := kubeConfig.TLSClientConfig.CAFile

Would it give you what you need to setup the Kubernetes auth backend in Vault afterwards?

munnerz
munnerz reviewed Nov 17, 2018
View reviewed changes
pkg/issuer/vault/issue.go Outdated

defaultCertificateDuration = time.Hour * 24 * 90

kubernetesServiceTokenPath = "/var/run/secrets/kubernetes.io/serviceaccount/token"
Copy link
Copy Markdown
Member

@munnerz munnerz Nov 17, 2018

Choose a reason for hiding this comment

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

Can we change this to be a field specified on the issuer, as a secretRef?

Users can then create a service account with no permissions, dedicated for authentication with Vault on a per-issuer basis.

This would be a lot more secure than handing off/using cert-manager's own service account credentials for all Vault authentication 😄

Copy link
Copy Markdown
Contributor Author

@kunickiaj kunickiaj Feb 15, 2019

Choose a reason for hiding this comment

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

Sure that sounds reasonable. Just keep in mind that that service account will still need a minimum permission of system:auth-delegator in order to call the TokenReview API.

Added Kubernetes authentication type for Vault Issuer
7a59102 
Signed-off-by: Adam Kunicki <adam@streamsets.com>
@kunickiaj
Copy link
Copy Markdown
Contributor Author

kunickiaj commented Mar 4, 2019

Not sure why these checks have been stalling -- anywhere I can check on the status of them?

@Dariusch
Copy link
Copy Markdown

Dariusch commented Mar 10, 2019

hey guys, I am really waiting for the PR to be merged. Is there any way to speed up this PR?

@jetstack-bot jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 15, 2019
@jetstack-bot
Copy link
Copy Markdown
Contributor

jetstack-bot commented Apr 15, 2019

@kunickiaj: PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@retest-bot
Copy link
Copy Markdown

retest-bot commented Jul 14, 2019

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.
Send feedback to jetstack.
/lifecycle stale

@jetstack-bot jetstack-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 14, 2019
@Mattzr
Copy link
Copy Markdown

Mattzr commented Jul 18, 2019

Hi,
Also looking forward to service account authentication with kubernetes !

@kunickiaj
Copy link
Copy Markdown
Contributor Author

kunickiaj commented Jul 18, 2019 via email

@phyrog
Copy link
Copy Markdown

phyrog commented Jul 18, 2019

Ping @munnerz in case you are not getting notifications about this

Can we somehow move this PR forward to get it merged? There is definitely an interest in this feature and it would be sad to see this not getting merged and @kunickiaj giving up due to missing feedback on this.

@retest-bot
Copy link
Copy Markdown

retest-bot commented Aug 17, 2019

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
If this issue is safe to close now please do so with /close.
Send feedback to jetstack.
/lifecycle rotten
/remove-lifecycle stale

@jetstack-bot jetstack-bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Aug 17, 2019
@martinbaillie
Copy link
Copy Markdown

martinbaillie commented Aug 20, 2019

We're considering forking internally at work in order to use this. Is there any reason not to merge the PR into the main project?

/remove-lifecycle rotten

@jetstack-bot jetstack-bot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Aug 20, 2019
@Mattzr
Copy link
Copy Markdown

Mattzr commented Aug 20, 2019 •
edited
Loading

Up, we also need this feature. There's conflicting files that need to be resolved though

@npurdy-tyro
Copy link
Copy Markdown

npurdy-tyro commented Aug 21, 2019

How close is this to being merged? I was about to implement the exact same thing when I stumbled across this PR.

@martinbaillie
Copy link
Copy Markdown

martinbaillie commented Aug 21, 2019

Happy to help @kunickiaj rebase and fix-up again so long as there's commitment to merge it this time from the maintainers?

@munnerz
Copy link
Copy Markdown
Member

munnerz commented Aug 30, 2019

Sorry this hasn't had much attention recently - we have also recently moved some vault initialization things into pkg/internal/vault, so the rebase will probably need to move the changes across.

To clarify, we do want this feature. I'll take a look over the changes today, but otherwise @JoshVanL mind shepherding this one once you're back from holiday? 😄

/assign @JoshVanL

@kunickiaj
Copy link
Copy Markdown
Contributor Author

kunickiaj commented Sep 4, 2019

This is def something we'd like as well, and I apologize I just haven't had the bandwidth to get to refactoring this with the latest code. If anyone else wants to take it upon themselves in the meantime, I'd be more than happy to see that happen.

@JoshVanL
Copy link
Copy Markdown
Contributor

JoshVanL commented Sep 4, 2019

Hello! Yes, happy to pick this up and get it refactored over the next couple of days

@JoshVanL JoshVanL mentioned this pull request Sep 6, 2019
Merged
@JoshVanL
Copy link
Copy Markdown
Contributor

JoshVanL commented Sep 6, 2019

/close
in favor of #2040

@jetstack-bot
Copy link
Copy Markdown
Contributor

jetstack-bot commented Sep 6, 2019

@JoshVanL: Closed this PR.

Details

In response to this:

/close
in favor of #2040

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

This was referenced Feb 27, 2026
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@munnerz munnerz munnerz left review comments

@kragniz kragniz Awaiting requested review from kragniz

@vdesjardins vdesjardins Awaiting requested review from vdesjardins

@wallrj wallrj Awaiting requested review from wallrj

Assignees

@kragniz kragniz

@JoshVanL JoshVanL

Labels

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 area/vault Indicates a PR directly modifies the Vault Issuer code 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. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

New Feature: Support Vault Kubernetes auth backend