Add support for alternate certs with PreferredChain in ACME #3208

meyskens · 2020-08-21T15:45:58Z

What this PR does / why we need it:

The ACME API now allows for alternate certificates being fetched signed by different CAs. Let's Encrypt does this by serving both their cross signed and own CA root certificates. We currently do not support specifying a preference for one.
certbot recently added support for this with the upcoming switch to the ISRG CA, they added:

  --preferred-chain PREFERRED_CHAIN
                        If the CA offers multiple certificate chains, prefer
                        the chain with an issuer matching this Subject Common
                        Name. If no match, the default offered chain will be

This PR adds similar behavior to cert-manager, while I didn't look at the exact checks it does and error handling it seems I wrote the exact dame thing 😺

Which issue this PR fixes:
Fixes #1700

Special notes for your reviewer:

Please also look at my fork of the Go library, we should make an upstream PR just like we did with EAB (not yet merged...)
~~Needs unit and e2e tests, thus WIP~~

Release note:

Add support for alternate certs with `prefferedChain` in ACME

meyskens · 2020-08-21T15:50:24Z

ooops... i didn't commit any code

meyskens · 2020-08-21T15:50:54Z

Fixed!

Signed-off-by: Maartje Eyskens <maartje@eyskens.me>

meyskens · 2020-08-24T15:37:39Z

Good e2e test seems a bit too hard as Pebble sets random CA names. Would be good to have this somehow able to do those in the future but a bit out of scope for this PR. So I added a unit test instead

meyskens · 2020-08-24T15:37:45Z

/kind feature

Signed-off-by: Maartje Eyskens <maartje@eyskens.me>

meyskens · 2020-08-24T17:37:51Z

/retest

wallrj

Looks like you're still working on this, so sorry for the premature review, but a few comments anyway, particularly regarding the crypto fork and the go.mod replace.

wallrj · 2020-08-25T11:12:05Z

go.mod

@@ -4,7 +4,7 @@ go 1.12

 replace github.com/prometheus/client_golang => github.com/prometheus/client_golang v0.9.4

-replace golang.org/x/crypto => github.com/munnerz/crypto v0.0.0-20191203200931-e1844778daa5
+replace golang.org/x/crypto => github.com/meyskens/crypto v0.0.0-20200821143559-6ca9aec645f0


Sorry to be a pain, but it makes me nervous to replace this stdlib crypto library with a fork.
But if we must do it, I think:

the fork should live in the cert-manager organization on Github

the reason for the fork should be explained briefly here, and in more detail in the fork's README.

make sure that the fork does not fall behind the upstream repo.

all changes should have links to corresponding upstream issues and pull requests so that upstream can give feedback on our proposed changes.

Ensure that we do thorough code review and testing of our changes in the fork.

the fork should live in the cert-manager organization on Github

Agreed, this should have been done when we first forked this, i don't think this fork was meant to stay for long but unfortunately we have had no feedback yet on our EAB PR.

the reason for the fork should be explained briefly here, and in more detail in the fork's README.

Disagree, this causes a commit that makes it harder to sync between upstream and commit back to upstream, the fork isn't meant to stay once merged into upstream. We should add a note in the GitHub description however

make sure that the fork does not fall behind the upstream repo.

Sure thing

all changes should have links to corresponding upstream issues and pull requests so that upstream can give feedback on our proposed changes.

EAB already has this done, this change will be PRed upstream as soon as it is reviewed by our team.

Ensure that we do thorough code review and testing of our changes in the fork.

See reviewers note ;)

wallrj · 2020-08-25T11:34:00Z

pkg/controller/acmeorders/sync.go

+		// if no match is found we return to the actual cert
+		// it is a *prefered* chain after all
+	}
+


Does FetchCertAlternatives always include the certSlice bundle?
If not, how is a user supposed to prefer the certSlice chain? By omitting the PreferredChain value? That seems unintuitive.

Also, if the user mistypes the PreferredChain, then they get back the default chain, without any warning or errors logged.
I'd be inclined to fail the order if the PreferredChain is not found, but at least log and /or a warning event so that the user can quickly see that the preferred chain is wrong.

Does FetchCertAlternatives always include the certSlice bundle?
If not, how is a user supposed to prefer the certSlice chain? By omitting the PreferredChain value? That seems unintuitive.

FetchCertAlternatives gives all alternative chains this can be 0 or 100. PreferredChain picks the one with the correct root issuer. If not present it will use the "normal" one this allows the ACME server to be free to select which chain it offers in the non alternative reply

pkg/internal/apis/acme/types_issuer.go

wallrj · 2020-08-25T11:38:28Z

pkg/controller/acmeorders/sync.go

+				cert, err := x509.ParseCertificate(certPEM)
+				if err != nil {
+					return fmt.Errorf("error parsing alternate certificates: %w", err)
+				}


Does this return the first or the last certificate in a multi-pem bundle?

it returns all certs in the multi-pem bundle which were in the bundle of multi multi-pem bundles... gets confusing but that's how it works

Ah sorry, I didn't read the code properly. Didn't see the nested for loop.

I wondered if there was a helper function that could take a slice of certificates and construct a chain, with a leaf and a "root" (where root is the root of this chain rather than a global root cert).
It would return an error if the certificates in the bundle did not form a chain.
Then we could compare the CN to that of the chain root.

wallrj · 2020-08-25T11:39:48Z

pkg/apis/acme/v1/types_issuer.go

+	// PreferredChain is no guarantee that this one gets delivered by the ACME
+	// endpoint.
+	// For example, for Let's Encrypt's DST crosssign you would use:
+	// "DST Root CA X3" or "ISRG Root X1" for the newer Let's Encrypt root CA.


Explain that this must match the CommonName of the first (or is it last?) certificate in the chain, where the first certificate is the chain root CA and the last certificate is the leaf certificate.

It returns if any cert's issuer in the chain matches it, ACME does not have to (and LE does not) give the root CA cert back only leaf and intermediate so we look at the issuer field, analog to certbot

Added a note

Ok. In my mind, it should only match the highest level CA certificate in the bundle and only match a CA certificate which can be chained to the leaf certificate.

Or to put it another way, you should get an error if you happen to enter the CN of the leaf certificate here.
And you should get an error if the bundle contains any certificates which are not involved in a chain of signatures leading to the leaf cert.

wallrj · 2020-08-25T11:40:47Z

pkg/acme/client/middleware/logger.go

+	l.log.V(logf.TraceLevel).Info("Calling FetchCertAlternatives")
+
+	ctx, cancel := context.WithTimeout(ctx, timeout)
+	defer cancel()


Seems a bit weird to add a timeout in "logging" middleware, but I see that that is done elsewhere, so fine.

This always has been a weird middleware... never liked it but it has it's reasons to be here

Signed-off-by: Maartje Eyskens <maartje@eyskens.me>

wallrj

A few more questions below.
But if you think my suggestions are overkill, or should be deferred for a later release, I'm happy to give a lgtm.

wallrj · 2020-08-26T09:05:15Z

pkg/controller/acmeorders/sync.go

+				cert, err := x509.ParseCertificate(certPEM)
+				if err != nil {
+					return fmt.Errorf("error parsing alternate certificates: %w", err)
+				}


Ah sorry, I didn't read the code properly. Didn't see the nested for loop.

wallrj · 2020-08-26T09:12:42Z

pkg/apis/acme/v1/types_issuer.go

+	// PreferredChain is no guarantee that this one gets delivered by the ACME
+	// endpoint.
+	// For example, for Let's Encrypt's DST crosssign you would use:
+	// "DST Root CA X3" or "ISRG Root X1" for the newer Let's Encrypt root CA.


Ok. In my mind, it should only match the highest level CA certificate in the bundle and only match a CA certificate which can be chained to the leaf certificate.

Or to put it another way, you should get an error if you happen to enter the CN of the leaf certificate here.
And you should get an error if the bundle contains any certificates which are not involved in a chain of signatures leading to the leaf cert.

wallrj · 2020-08-26T09:15:15Z

pkg/controller/acmeorders/sync.go

+				cert, err := x509.ParseCertificate(certPEM)
+				if err != nil {
+					return fmt.Errorf("error parsing alternate certificates: %w", err)
+				}


I wondered if there was a helper function that could take a slice of certificates and construct a chain, with a leaf and a "root" (where root is the root of this chain rather than a global root cert).
It would return an error if the certificates in the bundle did not form a chain.
Then we could compare the CN to that of the chain root.

wallrj · 2020-08-26T09:17:50Z

pkg/controller/acmeorders/sync.go

+		// if no match is found we return to the actual cert
+		// it is a *prefered* chain after all
+	}
+


wallrj

Discussed at standup where @meyskens explained that we are checking the issuer common name rather than the subject common name.
This means that the preferredChain value will only match a bundle where there is a certificate issued by that name so it should never match the leaf certifciate.

We can improve this further by performing extra checks to ensure that we only match CA certificates and that we only match the top-level CA certificate in the chain.

I've also created an issue to document this feature #3219

/lgtm

jetstack-bot · 2020-08-26T11:02:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: meyskens, wallrj

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:

~~OWNERS~~ [meyskens]

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 a review from munnerz August 21, 2020 15:46

jetstack-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 21, 2020

meyskens force-pushed the acme-multi-chain branch from cfd98f5 to e67b97a Compare August 21, 2020 15:50

meyskens changed the title ~~Add support for alternate certs with PrefferedChain in ACME~~ WIP Add support for alternate certs with PrefferedChain in ACME Aug 21, 2020

jetstack-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 21, 2020

Add support for alternate certs with PrefferedChain in ACME

90d6a54

Signed-off-by: Maartje Eyskens <maartje@eyskens.me>

meyskens force-pushed the acme-multi-chain branch from e67b97a to 90d6a54 Compare August 21, 2020 15:56

meyskens added this to the v1.0 milestone Aug 24, 2020

meyskens added 2 commits August 24, 2020 14:54

Fix interface

abc4675

Signed-off-by: Maartje Eyskens <maartje@eyskens.me>

Add a unit test for alt chains

75e31fe

Signed-off-by: Maartje Eyskens <maartje@eyskens.me>

meyskens changed the title ~~WIP Add support for alternate certs with PrefferedChain in ACME~~ Add support for alternate certs with PrefferedChain in ACME Aug 24, 2020

jetstack-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 24, 2020

jetstack-bot added kind/feature Categorizes issue or PR as related to a new feature. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Aug 24, 2020

fmt

824e075

Signed-off-by: Maartje Eyskens <maartje@eyskens.me>

wallrj requested changes Aug 25, 2020

View reviewed changes

meyskens added 3 commits August 26, 2020 10:22

Implement feedback

c81531d

Signed-off-by: Maartje Eyskens <maartje@eyskens.me>

Add note on fork

2ba8949

Signed-off-by: Maartje Eyskens <maartje@eyskens.me>

format go mod file

8581837

Signed-off-by: Maartje Eyskens <maartje@eyskens.me>

meyskens requested a review from wallrj August 26, 2020 08:25

wallrj reviewed Aug 26, 2020

View reviewed changes

wallrj mentioned this pull request Aug 26, 2020

Document the Let's Encrypt preferred chain feature #3219

Closed

wallrj approved these changes Aug 26, 2020

View reviewed changes

jetstack-bot assigned wallrj Aug 26, 2020

jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 26, 2020

jetstack-bot merged commit 6a2f953 into cert-manager:master Aug 26, 2020

wallrj mentioned this pull request Aug 26, 2020

Move fork github.com/meyskens/crypto to github.com/cert-manager/crypto #3220

Closed

inteon mentioned this pull request Feb 13, 2024

bugfix: wrong certificate chain is used if preferredChain is configured #6755

Merged

inteon changed the title ~~Add support for alternate certs with PrefferedChain in ACME~~ Add support for alternate certs with PreferredChain in ACME Feb 19, 2024

Add support for alternate certs with PreferredChain in ACME #3208

Add support for alternate certs with PreferredChain in ACME #3208

Uh oh!

Conversation

meyskens commented Aug 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meyskens commented Aug 21, 2020

Uh oh!

meyskens commented Aug 21, 2020

Uh oh!

meyskens commented Aug 24, 2020

Uh oh!

meyskens commented Aug 24, 2020

Uh oh!

meyskens commented Aug 24, 2020

Uh oh!

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

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!

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

wallrj left a comment

Choose a reason for hiding this comment

Uh oh!

jetstack-bot commented Aug 26, 2020

Uh oh!

Uh oh!

meyskens commented Aug 21, 2020 •

edited

Loading