feat: Include entire certificate chain if provided #1077

mikebryant · 2018-11-14T11:02:19Z

What this PR does / why we need it: Allow a user to provide an entire certificate chain to the ca issuer. Include that chain in all generated certificates

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #1067

Special notes for your reviewer:

Release note:

It is now possible to include a certificate chain in the secret for the `ca` Issuer. This will then be propagated to generated certificates.

mikebryant · 2018-11-14T11:03:51Z

I haven't yet tested this in our live environment, will be doing that shortly, but would appreciate an early review

munnerz · 2018-11-14T11:06:30Z

Looks sensible to me 😄

To be more sure, could you also look at adding an e2e test for this? It should be able to go in here: https://github.com/jetstack/cert-manager/blob/master/test/e2e/suite/issuers/ca/certificate.go - I think you may have to rework how the BeforeEach function works a bit, or otherwise make all of those e2e tests run with a chain as part of the signing keypair 😄

mikebryant · 2018-11-14T11:27:35Z

@munnerz I've updated the e2e ca to be an issuer instead of the root CA

munnerz · 2018-11-14T14:55:22Z

We should also extend the e2e test cases to actually check the chain is included as expected.

This may need us to actually provision a few different types of issuers during tests, i.e. some with only a self signed root keypair, one with a chain, etc. (verifying the expected output afterward).

Thinking about this more, in future it may be sensible for us to extend the IssueResponse struct we use internally to have a Chain field, to allow cert-manager's core Certificate controller populate the secret (instead of misusing the Certificate field like we currently do).

mikebryant · 2018-11-14T15:07:22Z

Yeah, that's a good point. We should have more tests.

And mhm, that sounds like a reasonable refactoring. I guess we could leave that for a later PR?

mikebryant · 2018-11-15T00:30:46Z

@munnerz I've added some e2etests for this (which failed prior to the fix)

munnerz · 2018-11-26T14:12:39Z

pkg/issuer/ca/issue.go

 	}

+	// encode the chain
+	chainPem, err := pki.EncodeX509Chain(signerCertChain)


So it looks like signerCertChain includes the full chain, including the 'root' certificate. If for example the CA here is a self signed root, we'd be bundling that with all leaf certificates generated from that root.

From my understanding, this is not desired behaviour, although I may be wrong. Are there any other similar examples or info on how this usually behaves elsewhere?

Hmm. I don't think we need/want the self signed root. I don't think it makes any difference though - every implementation I've seen, all of the chain certs get imported into a cert pool that can be used to help verify. Importing the root there makes no difference to the outcome.

(In golang for example a verify operation has a certPool of certs that are trusted, and another certPool of arbitrary certs that can be used to form chains to the trusted ones.)

I wonder how I could tell if a cert is self-signed, and exclude it from this...

@munnerz I've made it so the chain will now exclude self-signed certificates. How's that?

mikebryant · 2018-11-26T17:18:49Z

@munnerz I'm not sure what happened with the tests there. The diff from my earlier version which passed the tests is minimal.. Help?

munnerz · 2018-11-29T00:33:19Z

🤦‍♂️ sorry, I've messed up your branch!

retest-bot · 2018-12-09T17:28:13Z