Vault issuer support by vdesjardins · Pull Request #292 · cert-manager/cert-manager

vdesjardins · 2018-02-05T20:30:14Z

What this PR does / why we need it:
Initial Vault support.

Which issue this PR fixes
fixes #17

Special notes for your reviewer:
Where I work we need support for Vault so I took a shot at it. Please tell me if you guys are interested in what I did. If yes I can write some documentation and rework the code if need be.

Thanks

Add experimental support for Hashicorp Vault issuers

munnerz

First of all, thanks very much for this contribution. It looks like a great start at a Vault issuer, and is definitely something we'll want to merge provided we can resolve some of the finer details.

I've added a few comments, and I'm keen to run e2e tests on this. There's one particular comment (left above) that is blocking end-to-end testing this (a change to the test fixtures, I guess that you made so you can test locally).

It also looks like you've used a different version of dep to update dependencies. I hope our cert-manager-quick-verify job will pick that up 😅 . In the meantime I'll add a update-deps.sh script that ensures versions are correct.

If you've got any questions (either about the review, or more generally), feel free to reach out either here or on the Kubernetes slack (my username is @munnerz).

munnerz · 2018-02-06T10:35:44Z

pkg/apis/certmanager/v1alpha1/types.go

+	// Server is the vault connection address
+	Server string `json:"server"`
+	// Vault URL path to the certificate role
+	Path string `json:"path"`


This implies a 1:1 binding between Issuer and a role within a PKI backend. I think this makes sense to be honest, and I like keeping it simple here, but wanted to highlight it.

Perhaps before 1.0 we could add some sort of 'role binding' support. This would probably be a breaking change... we'll see what users want.

munnerz · 2018-02-06T10:43:47Z

pkg/issuer/vault/issue.go

+
+const (
+	// certificateDuration of 90 days
+	certificateDuration = time.Hour * 24 * 90


Should probably be configurable - perhaps as a field on the issuer?

I agree. Initially, I was hoping for a shorter certificate duration but I hit the default renewBefore in pkg/controller/certificates/sync.go:

const renewBefore = time.Hour * 24 * 30

Maybe renewBefore and certificateDuration should be configurable? For Vault and CA issuers It could make sense I think. For ACME it is fixed to 90 days so It could be enforced at 30 days in that specific issuer.

For ACME it is fixed to 90 days so It could be enforced at 30 days in that specific issuer.

Careful, that's not true for ACME in general and might be over-fitting Let's Encrypt. The protocol allows flexibility in certificate lifetime.

Oh ok sorry about that. Thanks for pointing it out!

No need to be sorry :-)

munnerz · 2018-02-06T10:47:43Z

pkg/issuer/vault/issue.go

+	token, err := vaultTokenRef(v.secretsLister, v.issuerResourcesNamespace, tokenRef.Name, "token")
+	if err != nil {
+		return nil, fmt.Errorf("error reading Vault token from secret %s/%s: %s", v.issuerResourcesNamespace, tokenRef.Name, err.Error())
+	}


We should probably check here to see why it failed (via the k8s.io/apimachinery/pkg/api/errors package).

This isn't something that's handled well in the CA or ACME implementation currently, and I think would take some fairly significant refactoring to add retrospectively (as the top level sync function needs a way to return nil despite an error occurring).

When initially writing cert-manager, I created a TransientError type that could be used to indicate a sync should be retried. This would save all the refactoring, at the expense of needing to check for transient errors at various points (and/or not 'masking' errors with fmt.Errorf)

munnerz · 2018-02-06T10:49:13Z

pkg/issuer/vault/issue.go

+		"exclude_cn_from_sans": "true",
+	}
+
+	url := fmt.Sprintf("/v1%s", v.issuer.GetSpec().Vault.Path)


I think v.issuer.GetSpec().Vault.Path should not be required to start with a /, as vault itself doesn't use this afaik.

If it does start with a /, we could also automatically strip it, thus allowing both. But this fmt.Sprintf should be using /v1/%s imo.

I will change it for a path.Join so it will work in both cases.

munnerz · 2018-02-06T10:49:51Z

pkg/issuer/vault/issue.go

+
+	defer resp.Body.Close()
+
+	vaultResult := VaultResult{}


I guess the vault package doesn't provide any pre-made structs for us to use?

After some digging there is in fact some structs that we could use. Even some helper methods too.

munnerz · 2018-02-06T10:51:34Z

pkg/issuer/vault/issue.go

+	for _, v := range vaultResult.Data.Chain {
+		certBytes.WriteString(v)
+		certBytes.WriteString("\n")
+	}


This is okay I think, but it's worth noting right now we don't expose any way to obtain the CA that signed a Certificate via cert-manager (this is something that would be great to change, but I've not been able to find a way to get a CA certificate from an ACME server (cc @cpu)).

iirc, the certificate chain here doesn't actually include that CA certificate anyway so this should be okay to include here.

I've not been able to find a way to get a CA certificate from an ACME server (cc @cpu)).

@munnerz For the V1 API the new-cert HTTP response includes a Link header with an up relation that points to where you can find the issuer certificate for the issued leaf certificate.

For the V2 API things are much easier. After finalizing an order the URL of the certificate in the order object returns the full PEM encoded certificate chain. When you GET the cert URL you'll get both the leaf and the intermediate(s).

Hope that helps!

munnerz · 2018-02-06T10:52:32Z

pkg/issuer/vault/setup.go

+)
+
+const (
+	successVaultVerified = "VaultVerified"


Not for you to fix as part of this PR: we should probably move these Reason messages into a generic package

munnerz · 2018-02-06T10:53:03Z

pkg/issuer/vault/setup.go

+	glog.Info(messageVaultVerified)
+	v.recorder.Event(v.issuer, v1.EventTypeNormal, successVaultVerified, messageVaultVerified)
+	v.issuer.UpdateStatusCondition(v1alpha1.IssuerConditionReady, v1alpha1.ConditionTrue, successVaultVerified, messageVaultVerified)
+	return nil


We should perform additional checks here if possible - i.e. ensure we can connect to Vault and ensure the path is valid for issuing certs.

munnerz · 2018-02-06T10:53:55Z

test/e2e/certificate/certificate_vault.go

+		}
+	})
+
+})


🙏 ❤️ thanks for adding this

munnerz · 2018-02-06T10:54:41Z

test/fixtures/cert-manager-values.yaml

    tag: build
-    pullPolicy: Never
+    repository: us.gcr.io/projet-labo-1/cert-manager-ingress-shim
+    pullPolicy: Always


This needs switching back - if you do this first, I can /test all this 😄 (can't verify what is in that image, so I'm reluctant to test this on our infrastructure!)

My bad, this file slipped in the commit.

vdesjardins · 2018-02-06T16:23:14Z

Thanks for the review! I'm going to start working on your requested improvements in the coming days.

munnerz · 2018-02-06T17:01:14Z

/ok-to-test

munnerz · 2018-02-21T10:23:01Z

/retest

munnerz · 2018-02-21T12:56:25Z

@vdesjardins would you mind splitting this into an "Update vendor" PR and "Vault issuer support" to make it easier to review?

& if it isn't too much of a pain, keeping your changes split into multiple commits until we're ready to merge would be great 😄 (obviously I'm not asking you to retroactively do this!)

jetstack-ci-bot · 2018-02-21T13:31:32Z

@vdesjardins PR needs rebase

vdesjardins · 2018-02-21T13:32:01Z

No problem! I will split them. I was in fact thinking about doing that after my last push! :-)

…

On Wed, Feb 21, 2018, 07:56 James Munnelly ***@***.***> wrote: @vdesjardins <https://github.com/vdesjardins> would you mind splitting this into an "Update vendor" PR and "Vault issuer support" to make it easier to review? & if it isn't too much of a pain, keeping your changes split into multiple commits until we're ready to merge would be great 😄 (obviously I'm not asking you to retroactively do this!) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#292 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEGERcMx1-6m4-iWKYFk8Zz8EITuKc5ks5tXBJ7gaJpZM4R6DbA> .

mwthink · 2018-04-16T20:37:37Z

Is there anyone actively working on the pending changes? This feature is the only reason I'm not using certmanager at the moment and would be glad to help out if it'll get it out the gate faster.

vdesjardins · 2018-04-16T20:53:31Z

@mwthink yes I'm working on it! I need to finish writing the documentation.

munnerz · 2018-04-25T17:27:36Z

It looks like this PR may touch more dependencies than required... this commit adds nearly 2M lines of code, and removes 1M: a48908c

I'd expect a large addition, but not such a large reduction in LoC.

Could you take another look? If you drop that commit out of your tree (with something like git rebase), and then run dep ensure -v you should hopefully be able to clean up a lot of those changes 😃

vdesjardins · 2018-04-26T18:51:18Z

I fixed the dependencies. Look much better now 😀

I did some commit cleanup too.

munnerz

Nothing too major now, thanks very much for squashing - far easier to review!

Looking forward to getting this merged in 😃

munnerz · 2018-04-26T18:29:18Z

pkg/apis/certmanager/v1alpha1/types.go

+	// Certificate default Duration
+	Duration time.Duration `json:"duration,omitempty"`
+	// Certificate renew before expiration duration
+	RenewBefore time.Duration `json:"renewBefore,omitempty"`


👍 this looks good to me.

We may in future want to consider switching this to some form of 'CertificatePolicy' resource (ie certificate-class), which can be associated with a set of certificates somehow. This would help admins provide cert-specific default config easily.

For now though, this seems acceptable. We can always retain this in future as the issuer default (where a CertificatePolicy would override the issuers defaults)

munnerz · 2018-04-26T18:32:27Z

pkg/issuer/acme/issue.go

 const (
 	errorIssueError = "IssueError"

+	// default certificateDuration of zero, we let ACME decide


This const doesn't seem to be used in issue.go, and only in acme.go. Can you move it into acme.go?

also rename to defaultCertificateDuration

munnerz · 2018-04-26T18:38:06Z

pkg/issuer/ca/ca.go

+
+	if err := issuer.ValidateDuration(issuerObj); err != nil {
+		return nil, fmt.Errorf("CA %s", err.Error())
+	}


Instead of checking ValidateDuration in the New* functions, we should perform it in Setup() and if they are invalid, mark the Issuer NotReady. This will cause Certificate requests to fail as well as a side effect, and providers a cleaner error reporting interface (we can add a status condition in future to explain why it failed, e.g. a ConfigValid status condition)

munnerz · 2018-04-26T18:39:41Z

test/util/util.go

+// WaitForCertificateEvent waits for an event on the named Certificate to contain
+// an event reason matches the supplied one.
+func WaitForCertificateEvent(client kubernetes.Interface, cert *v1alpha1.Certificate, reason string, timeout time.Duration) error {
+	return wait.PollImmediate(500*time.Millisecond, timeout,


Reduce this to check every 1s or 2s? Not sure what sort of values are used for timeout here.

munnerz · 2018-04-26T18:41:47Z

pkg/controller/clusterissuers/sync.go

-		return err
-	}
-
-	err = i.Setup(ctx)


Making the previously mentioned change to validate config in Setup(), we can revert the changes here and in issuers/sync.go

munnerz · 2018-04-26T18:56:18Z

pkg/issuer/vault/setup.go

+	}
+
+	glog.Info(messageVaultVerified)
+	v.recorder.Event(v.issuer, v1.EventTypeNormal, successVaultVerified, messageVaultVerified)


Don't fire an event here either

munnerz · 2018-04-26T18:57:32Z

pkg/issuer/vault/vault.go

+
+	if err := issuer.ValidateDuration(issuerObj); err != nil {
+		return nil, updateEventAndCondition(fmt.Errorf(messageVaultDurationInvalid, err.Error()), errorVaultDurationInvalid, issuerObj, recorder)
+	}


All validation of the Issuer resource should happen in Setup() - other fields (eg resourceNamespace) can be validated here, but not the issuer itself.

munnerz · 2018-04-26T18:57:47Z

pkg/issuer/vault/vault.go

+	recorder.Event(issuerObj, v1.EventTypeWarning, event, err.Error())
+	issuerObj.UpdateStatusCondition(v1alpha1.IssuerConditionReady, v1alpha1.ConditionFalse, event, err.Error())
+	return err
+}


Probably remove this function after changes above

munnerz · 2018-04-26T18:59:56Z

test/util/util_vault.go

+	}, nil
+}
+
+func (v *VaultInitializer) Setup() error {


This looks stressful to maintain 😅

It may be best to move all the vault specific test utils into their own package so you can add an OWNERS file in there to be pinged for code reviews in future.

Tests for our tests anyone? 😁

This can be deferred for now though.

munnerz · 2018-04-26T19:02:27Z

docs/tutorials/vault/creating-vault-issuers.rst

+the Vault server base URL. The secret created previously is referenced in the issuer
+with its *name* and *key* corresponding to the name of the Kubernetes secret and the
+property name containing the token value respectively.
+


A small addition to explain that these Issuers can be referenced like any other and/or an example of a certificate now referencing the issuer we just created would be great.

I'm envisioning the tutorials section to be quite hand-holdy and explain how to get from point A to point B (i.e. no certs to signed cert).

May also be worth creating an OWNERS file in this directory too

vault remove duration

munnerz · 2018-05-02T12:05:23Z

Let's get this in and see how we get on - we can iterate from there 😄

Thanks so much for all your hard work @vdesjardins! 🙏 👏

/lgtm
/approve

jetstack-bot · 2018-05-02T12:05:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: munnerz

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [munnerz]

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

jetstack-bot requested review from munnerz and simonswine February 5, 2018 20:30

munnerz requested changes Feb 6, 2018

View reviewed changes

vdesjardins force-pushed the master branch from 96ea5c2 to 7b9f8f8 Compare February 6, 2018 16:11

jetstack-bot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 6, 2018

vdesjardins force-pushed the master branch 2 times, most recently from 4a11dce to b4c65b6 Compare February 7, 2018 19:05

vdesjardins force-pushed the master branch 2 times, most recently from c0a4ae8 to a72fdbd Compare February 16, 2018 03:33

munnerz mentioned this pull request Feb 21, 2018

Bundle the CA public key in issued certificate #317

Merged

vdesjardins force-pushed the master branch 3 times, most recently from eada339 to 82a305c Compare February 21, 2018 12:52

jetstack-ci-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 21, 2018

munnerz mentioned this pull request Feb 21, 2018

WIP: Add Vault issuer #174

Closed

vdesjardins force-pushed the master branch from 82a305c to 3043699 Compare March 6, 2018 01:20

jetstack-ci-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 6, 2018

vdesjardins force-pushed the master branch 4 times, most recently from ace1e76 to 48a3bcc Compare March 17, 2018 19:58

jetstack-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 15, 2018

vdesjardins force-pushed the master branch from 581c83f to e8548ee Compare April 16, 2018 19:05

jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 16, 2018

vdesjardins force-pushed the master branch from 309f908 to b0af15c Compare April 16, 2018 23:10

vdesjardins force-pushed the master branch from 559da72 to 839e4e3 Compare April 25, 2018 16:54

vdesjardins force-pushed the master branch 3 times, most recently from f094e55 to 2b91fe2 Compare April 26, 2018 17:43

munnerz reviewed Apr 26, 2018

View reviewed changes

vdesjardins force-pushed the master branch from 935c50a to c64e955 Compare April 27, 2018 03:08

vdesjardins mentioned this pull request Apr 28, 2018

Configurable issuer duration and renewBefore #520

Closed

vdesjardins added 3 commits May 2, 2018 00:34

e2e: add resource's last status to timed out error message

230f59b

Vault issuer support

b353437

vault remove duration

Vault dependencies

98c020c

vdesjardins force-pushed the master branch from c64e955 to 98c020c Compare May 2, 2018 02:37

jetstack-bot assigned munnerz May 2, 2018

jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label May 2, 2018

jetstack-bot merged commit ba36751 into cert-manager:master May 2, 2018

This was referenced Feb 27, 2026

🌱 CNCF mission generation 2026-02-27 kubestellar/console-kb#6

Closed

🌱 CNCF mission generation 2026-02-27 kubestellar/console-kb#11

Merged

+              		}
+              	})
+              })

Conversation

vdesjardins commented Feb 5, 2018 • edited by munnerz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

munnerz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdesjardins commented Feb 6, 2018

Uh oh!

munnerz commented Feb 6, 2018

Uh oh!

munnerz commented Feb 21, 2018

Uh oh!

munnerz commented Feb 21, 2018

Uh oh!

jetstack-ci-bot commented Feb 21, 2018

Uh oh!

vdesjardins commented Feb 21, 2018 via email

Uh oh!

mwthink commented Apr 16, 2018

Uh oh!

vdesjardins commented Apr 16, 2018

Uh oh!

munnerz commented Apr 25, 2018

Uh oh!

vdesjardins commented Apr 26, 2018

Uh oh!

munnerz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdesjardins commented Feb 5, 2018 •

edited by munnerz

Loading