Parse certificate chain CA Issuer by JoshVanL · Pull Request #3985 · cert-manager/cert-manager

JoshVanL · 2021-05-10T19:32:42Z

The CA issuer now constructs a certificate chain after signing, and populates the CertificateRequest.Status.CA with the root most certificate if available. Correctly passes down CA certificate when chaining CA Issuers together.

This PR is branched from #3982 and should be merged first. EDIT: This should say "and that PR should be merged first" instead of "and should be merged first"

Updates SignCSRTemplate to use new ParseCertificateChain func, and passes through CA certificate when chaining CA Issuers together.

/assign @maelvls @munnerz @SgtCoDFish @jakexks

a valid flat chain. Returns a chain and optional CA Signed-off-by: joshvanl <vleeuwenjoshua@gmail.com>

vault Signed-off-by: joshvanl <vleeuwenjoshua@gmail.com>

runtime Signed-off-by: joshvanl <vleeuwenjoshua@gmail.com>

jetstack-bot · 2021-05-10T19:32:46Z

[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

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [JoshVanL]

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

JoshVanL · 2021-05-11T08:05:37Z

flake
/retest

SgtCoDFish

A few suggestions and questions from me; I think it was a great idea to split the original PR up into separate ones 👍

SgtCoDFish · 2021-05-11T09:23:02Z

pkg/controller/certificaterequests/acme/acme_test.go

+		Subject: pkix.Name{
+			CommonName: "root",
+		},
+		PublicKeyAlgorithm: x509.RSA,


suggestion: should this be EC not RSA, based on rootPK above?

(actually, does this need to be specified in the template? I'd imagine it would be filled in automatically when the cert is signed, although I've not tested that)

SgtCoDFish · 2021-05-11T09:25:26Z

pkg/controller/certificaterequests/ca/ca_test.go

 	"github.com/jetstack/cert-manager/pkg/controller"
 	"github.com/jetstack/cert-manager/pkg/controller/certificaterequests"
 	"github.com/jetstack/cert-manager/pkg/controller/certificaterequests/util"
+	controllertest "github.com/jetstack/cert-manager/pkg/controller/test"


suggestion: is this not a re-import duplication of the line below it?

SgtCoDFish · 2021-05-11T09:26:37Z

pkg/controller/certificaterequests/ca/ca_test.go

+		Subject: pkix.Name{
+			CommonName: name,
+		},
+		PublicKeyAlgorithm: x509.RSA,


suggestion: we're passed a crypto.Signer here; is it safe to just pass RSA for the key algo? (as before, is setting this field needed at all?)

SgtCoDFish · 2021-05-11T09:27:22Z

pkg/controller/certificaterequests/ca/ca_test.go


 	// Build root RSA CA
-	skRSA, err := pki.GenerateRSAPrivateKey(2048)
+	rootPK, err := pki.GenerateRSAPrivateKey(2048)


suggestion: could we use P-256 ECDSA keys here instead if we're refactoring the variable names?

SgtCoDFish · 2021-05-11T09:27:52Z

pkg/controller/certificaterequests/ca/ca_test.go

 	if err != nil {
-		t.Error(err)
-		t.FailNow()
+		t.Fatal()


suggestion: would it be better to include the error when we call t.Fatal()?

Suggested change

t.Fatal()

t.Fatal(err)

SgtCoDFish · 2021-05-11T10:26:20Z

pkg/util/pki/parse_test.go

+	root := mustCreateBundle(t, nil, "root")
+	int1 := mustCreateBundle(t, root, "int-1")
+	int2 := mustCreateBundle(t, int1, "int-2")
+	leaf := mustCreateBundle(t, int2, "leaf")
+	random := mustCreateBundle(t, nil, "random")


suggestion: would it be worth adding a test for a different branch off the same root?

e.g. using some random diagram I found, we're currently testing A->B->E->H which is good. Could we also test that ParseCertificateChain works correctly with an input set of, say, {A, B, C, E, G} which should produce A->C->G?

I may have misunderstood- I thought that this scenario would error- if the input was {A, B, C, E, G} from the diagram, we would end up with a list of chain nodes {A, B, E} and {C, G} that cannot be merged and would error here?

(caveat: I didn't trace through the algorithm in depth with {A, B, C, E, G}, because ultimately it'll be unit tests that check this stuff and not humans 😁 )

I guess what I'm saying is that A->C->G is a valid chain, and the tree as presented in that diagram is a totally valid PKI hierarchy/tree which could be passed to the function, but we don't have a test which checks its operation.

It's reasonable IMO to do any of the following:

if we can construct two (partial or complete) chains to the same root from given input, we'll error (so {A, B, C, E, G} fails because there's A->C->G and A->B->E)

or if we can construct two full chains from root to leaf we'll error (so {A, B, C, E, G, H} fails because there's A->B->E->H and A->C->G but {A, B, C, E, G} succeeds with A->C->G since A->B->E doesn't end in a leaf)

or we return all chains we can construct (so {A, B, C, E, G} returns {{A->C->G}, {A->B->E},{A->B},{A->C}}

or we return all complete chains we can construct (so {A, B, C, E, G} returns {{A->C->G}} and {A, B, C, E, G, H} returns {{A->B->E->H}, {A->C->G}})

I think I'd prefer 2 as an implementation, but this is quite academic because in most cases we'd only ever be passed, say {A, A, B, B, E, H} which the algorithm should handle fine.

Whichever approach we take, I think my main point is that it would be super valuable to have a test case which takes an input of {A, B, C, E, G} and one which takes input of {A, B, C, E, G, H}; if they both end up erroring that's fine.

Thanks for detailed explanation. Indeed having multiple branches in the same bundle is totally valid and is likely wanted for services with multiple intermediates; they may not even share the same roots, but do trust them. The intention here though is that we expect a single chain when a certificate is signed, i.e. the issuer returns the single chain from root to the new signed certificate.

I think managing these larger bundle trees is a separate concern, and falls under the "trust distribution" planned work (whatever that may look like). Given this, I would like to continue with 1 as the implementation. In future it may be that other functions are made to cover those other cases.

To make it clear this is the intention of this function and wouldn't be appropriate for other cases, I'm going to rename it to ParseSingleCertificateChain. Open to suggestions for better names 😄

I'll also add some tests to demonstrate that this function should fail with branching chains.

SgtCoDFish · 2021-05-11T10:32:30Z

pkg/util/pki/parse_test.go

+			bundle, err := ParseCertificateChainPEM(test.inputBundle)
+			if (err != nil) != test.expErr {
+				t.Errorf("unexpected error, exp=%t got=%v",
+					test.expErr, err)
+			}


question: do you think we're doing enough testing that the whitespace is correct in the output PEM we expect?

I think we're relying on the PEM output from SignCertificate in mustCreateBundle having the required \n at the end; that's probably not likely to change, but given that ParseCertificateChain is so important, would it be good to make sure in these tests that if we pass a cert with no newline at the end it's handled correctly and we don't end up with a line like:

-----END CERTIFICATE----------BEGIN CERTIFICATE-----

Hmm, I don't think this is too much of a concern. Considering we do all the checks I think this should be fine.

SgtCoDFish · 2021-05-11T10:32:52Z

pkg/util/pki/parse.go

+					continue
+				}
+
+				// attempt to add both chain together


nitpick: spelling

Suggested change

// attempt to add both chain together

// attempt to add both chains together

SgtCoDFish · 2021-05-11T10:41:07Z

pkg/util/pki/parse.go

+				}
+
+				// attempt to add both chain together
+				chain, ok := chains[i].tryMergeChain(chains[j])


question: especially with larger RSA key sizes (4096 being common for roots, which we'll be using here) checking signatures can be quite an expensive operation, and we're potentially going to be doing it a lot in tryMergeChain for larger chains. [1]

we can maybe remove the need to do so many signature checks by ordering the nodes differently before we check signatures; we have access to the issuer and subject for every cert in the chain, and in most cases (i.e. when the chain is sane) we'll be able to order certs based on their issuer DN. we still need to check the signatures for correctness reasons, but ordering based on the DNs will save a lot of signature checks.

do you think this optimization is worth it now? I don't think it's needed for this PR (although we need to be careful of a potential performance regression here with longer chains). I could look at adding it in the future if you don't think it's required here.

[1] looking way into the future, quantum-resistant TLS will likely be even more computationally expensive.

Yep, I think it is worth while thinking about and keeping an eye on. I'm not keen on pre optimising though given we don't know if this will be the limiting factor.

yeah totally reasonable, I'm sure this will perform fine as implemented 👍

SgtCoDFish · 2021-05-11T10:41:38Z

pkg/util/pki/parse.go

+//
+// An error is returned if the passed bundle is not a valid flat tree chain,
+// the bundle is malformed, or the chain is broken.
+func ParseCertificateChain(certs []*x509.Certificate) (PEMBundle, error) {


praise: I think this will clean up a lot of stuff, and it seems super valuable to me.

irbekrm

Amazing code, I wish we did work more work like this!

I have added a comment where I think we should not modify the slice inside the for loop.

pkg/util/pki/parse.go

intention better Signed-off-by: joshvanl <vleeuwenjoshua@gmail.com>

target Secret Signed-off-by: joshvanl <vleeuwenjoshua@gmail.com>

Signed-off-by: joshvanl <vleeuwenjoshua@gmail.com>

certificate down Signed-off-by: joshvanl <vleeuwenjoshua@gmail.com>

Signed-off-by: joshvanl <vleeuwenjoshua@gmail.com>

JoshVanL · 2021-05-12T17:18:03Z

/retest

jakexks · 2021-05-13T11:50:38Z

/lgtm

jakexks · 2021-05-13T11:58:06Z

/hold

jakexks · 2021-05-13T12:22:33Z

/unhold

JoshVanL added 3 commits May 10, 2021 19:06

Adds ParseCertificateChain to parse and test a pem bundle to ensure its

744906e

a valid flat chain. Returns a chain and optional CA Signed-off-by: joshvanl <vleeuwenjoshua@gmail.com>

Changes vault issuer to use ParseCertificateChain from response from

d17626c

vault Signed-off-by: joshvanl <vleeuwenjoshua@gmail.com>

Change ParseCertificateChain test func to use ECDSA keys to speed up

8869343

runtime Signed-off-by: joshvanl <vleeuwenjoshua@gmail.com>

jetstack-bot assigned jakexks May 10, 2021

jetstack-bot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label May 10, 2021

jetstack-bot assigned maelvls and munnerz May 10, 2021

jetstack-bot added the dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. label May 10, 2021

jetstack-bot assigned SgtCoDFish May 10, 2021

jetstack-bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 10, 2021

This was referenced May 11, 2021

Handle Vault issuer working as intermediate correctly #3433

Merged

v1.4 Roadmap #3908

Closed

SgtCoDFish requested changes May 11, 2021

View reviewed changes

irbekrm suggested changes May 11, 2021

View reviewed changes

pkg/util/pki/parse.go Outdated Show resolved Hide resolved

JoshVanL added the kind/bug Categorizes issue or PR as related to a bug. label May 11, 2021

jetstack-bot removed the needs-kind Indicates a PR lacks a `kind/foo` label and requires one. label May 11, 2021

james-w added this to the v1.4 milestone May 12, 2021

JoshVanL added 5 commits May 12, 2021 14:12

Change ParseCertificateChain to ParseSingleCertificateChain to show

68aeb33

intention better Signed-off-by: joshvanl <vleeuwenjoshua@gmail.com>

Adds SecretTLSKeyPairAndCA to parse a certificate chain and CA from a

9622b66

target Secret Signed-off-by: joshvanl <vleeuwenjoshua@gmail.com>

Updates SignCSRTemplate to use ParseCertificateChain

d327d40

Signed-off-by: joshvanl <vleeuwenjoshua@gmail.com>

Updates CA issuer to updates SignCSRTemplate and propagate CA

ea2cfdc

certificate down Signed-off-by: joshvanl <vleeuwenjoshua@gmail.com>

Changes CR CA controller to use ECDSA keys

58a2531

Signed-off-by: joshvanl <vleeuwenjoshua@gmail.com>

JoshVanL force-pushed the parse-certificate-chain-ca branch from c819d09 to 58a2531 Compare May 12, 2021 14:07

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

JoshVanL added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label May 13, 2021

jetstack-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 13, 2021

jetstack-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 13, 2021

jetstack-bot merged commit 96ea5e5 into cert-manager:master May 13, 2021

maelvls mentioned this pull request Nov 22, 2021

Does the certificate chain include a root CA certificate by default? #3967

Closed

maelvls mentioned this pull request Mar 6, 2022

Retry on conflict for the end-to-end test "added an additional dnsName" #4924

Merged

	// attempt to add both chain together
	// attempt to add both chains together

Conversation

JoshVanL commented May 10, 2021 • edited by SgtCoDFish Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jetstack-bot commented May 10, 2021

Uh oh!

JoshVanL commented May 11, 2021

Uh oh!

SgtCoDFish 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!

irbekrm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JoshVanL commented May 12, 2021

Uh oh!

jakexks commented May 13, 2021

Uh oh!

jakexks commented May 13, 2021

Uh oh!

jakexks commented May 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

JoshVanL commented May 10, 2021 •

edited by SgtCoDFish

Loading