From 811cc7908ec96e2a6cf3112b6e677838f9caa16e Mon Sep 17 00:00:00 2001 From: Sam Lee Date: Tue, 13 Feb 2024 17:54:19 +0900 Subject: [PATCH 1/8] fix getAltCertChain not considering primary chain as candidate Signed-off-by: Sam Lee --- pkg/controller/acmeorders/sync.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/controller/acmeorders/sync.go b/pkg/controller/acmeorders/sync.go index 44cd10b82..bd362b422 100644 --- a/pkg/controller/acmeorders/sync.go +++ b/pkg/controller/acmeorders/sync.go @@ -706,13 +706,13 @@ func getAltCertChain(ctx context.Context, cl acmecl.Interface, certURL string, p if err != nil { return false, nil, fmt.Errorf("error listing alternate certificate URLs: %w", err) } - // Loop over all alternative chains - for _, altURL := range altURLs { + // Loop over every chains + for _, altURL := range append([]string{certURL}, altURLs...) { altChain, err := cl.FetchCert(ctx, altURL, true) if err != nil { return false, nil, fmt.Errorf("error fetching alternate certificate chain from %s: %w", altURL, err) } - // Loop over each cert in this alternative chain + // Loop over each cert in this chain for _, altCert := range altChain { cert, err := x509.ParseCertificate(altCert) if err != nil { From b9ac41726cff8ba9b62ee69c6a7d651002505a77 Mon Sep 17 00:00:00 2001 From: Sam Lee Date: Tue, 13 Feb 2024 19:08:08 +0900 Subject: [PATCH 2/8] make getAltCertChain checks only topmost certificate Signed-off-by: Sam Lee --- pkg/controller/acmeorders/sync.go | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/pkg/controller/acmeorders/sync.go b/pkg/controller/acmeorders/sync.go index bd362b422..e26c5ee4a 100644 --- a/pkg/controller/acmeorders/sync.go +++ b/pkg/controller/acmeorders/sync.go @@ -712,20 +712,19 @@ func getAltCertChain(ctx context.Context, cl acmecl.Interface, certURL string, p if err != nil { return false, nil, fmt.Errorf("error fetching alternate certificate chain from %s: %w", altURL, err) } - // Loop over each cert in this chain - for _, altCert := range altChain { - cert, err := x509.ParseCertificate(altCert) - if err != nil { - return false, nil, fmt.Errorf("error parsing alternate certificate chain: %w", err) - } - log.V(logf.DebugLevel).WithValues("Issuer CN", cert.Issuer.CommonName).Info("Found alternative ACME bundle") - if cert.Issuer.CommonName == preferredChain { - // if the issuer's CN matched the preferred chain it means this bundle is - // signed by the requested chain - log.V(logf.DebugLevel).WithValues("Issuer CN", cert.Issuer.CommonName).Info("Selecting alternative ACME bundle with a matching Common Name from %s", altURL) - return true, altChain, nil - } + // Check topmost certificate + cert, err := x509.ParseCertificate(altChain[len(altChain)-1]) + if err != nil { + return false, nil, fmt.Errorf("error parsing alternate certificate chain: %w", err) } + log.V(logf.DebugLevel).WithValues("Issuer CN", cert.Issuer.CommonName).Info("Found alternative ACME bundle") + if cert.Issuer.CommonName == preferredChain { + // if the issuer's CN matched the preferred chain it means this bundle is + // signed by the requested chain + log.V(logf.DebugLevel).WithValues("Issuer CN", cert.Issuer.CommonName).Info("Selecting alternative ACME bundle with a matching Common Name from %s", altURL) + return true, altChain, nil + } + } return false, nil, nil From 94509d0490be10953fd97086260d6c37acd8c0c0 Mon Sep 17 00:00:00 2001 From: Sam Lee Date: Tue, 13 Feb 2024 22:12:05 +0900 Subject: [PATCH 3/8] changed term 'alt' to 'preferred' Signed-off-by: Sam Lee --- pkg/controller/acmeorders/sync.go | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/pkg/controller/acmeorders/sync.go b/pkg/controller/acmeorders/sync.go index e26c5ee4a..2462692ed 100644 --- a/pkg/controller/acmeorders/sync.go +++ b/pkg/controller/acmeorders/sync.go @@ -585,17 +585,17 @@ func (c *controller) finalizeOrder(ctx context.Context, cl acmecl.Interface, o * } if issuer.GetSpec().ACME != nil && issuer.GetSpec().ACME.PreferredChain != "" { - preferredChain := issuer.GetSpec().ACME.PreferredChain - found, altChain, err := getAltCertChain(ctx, cl, certURL, preferredChain) + preferredChainName := issuer.GetSpec().ACME.PreferredChain + found, preferredCertChain, err := getPreferredCertChain(ctx, cl, certURL, preferredChainName) if err != nil { return fmt.Errorf("error retrieving alternate chain: %w", err) } if found { - return c.storeCertificateOnStatus(ctx, o, altChain) + return c.storeCertificateOnStatus(ctx, o, preferredCertChain) } // if no match is found we return to the actual cert // it is a *preferred* chain after all - log.V(logf.DebugLevel).Info(fmt.Sprintf("Preferred chain %s not found, fall back to the default cert", preferredChain)) + log.V(logf.DebugLevel).Info(fmt.Sprintf("Preferred chain %s not found, fall back to the default cert", preferredChainName)) } return c.storeCertificateOnStatus(ctx, o, certSlice) @@ -653,12 +653,12 @@ func (c *controller) syncCertificateDataWithOrder(ctx context.Context, cl acmecl } if issuer.GetSpec().ACME != nil && issuer.GetSpec().ACME.PreferredChain != "" { - found, altCerts, err := getAltCertChain(ctx, cl, acmeOrder.CertURL, issuer.GetSpec().ACME.PreferredChain) + found, preferredCertChain, err := getPreferredCertChain(ctx, cl, acmeOrder.CertURL, issuer.GetSpec().ACME.PreferredChain) if err != nil { return err } if found { - return c.storeCertificateOnStatus(ctx, o, altCerts) + return c.storeCertificateOnStatus(ctx, o, preferredCertChain) } } @@ -700,29 +700,29 @@ func getACMEOrder(ctx context.Context, cl acmecl.Interface, o *cmacme.Order) (*a return acmeOrder, nil } -func getAltCertChain(ctx context.Context, cl acmecl.Interface, certURL string, preferredChain string) (bool, [][]byte, error) { +func getPreferredCertChain(ctx context.Context, cl acmecl.Interface, certURL string, preferredChain string) (bool, [][]byte, error) { log := logf.FromContext(ctx) altURLs, err := cl.ListCertAlternates(ctx, certURL) if err != nil { return false, nil, fmt.Errorf("error listing alternate certificate URLs: %w", err) } // Loop over every chains - for _, altURL := range append([]string{certURL}, altURLs...) { - altChain, err := cl.FetchCert(ctx, altURL, true) + for _, chainURL := range append([]string{certURL}, altURLs...) { + certChain, err := cl.FetchCert(ctx, chainURL, true) if err != nil { - return false, nil, fmt.Errorf("error fetching alternate certificate chain from %s: %w", altURL, err) + return false, nil, fmt.Errorf("error fetching alternate certificate chain from %s: %w", chainURL, err) } // Check topmost certificate - cert, err := x509.ParseCertificate(altChain[len(altChain)-1]) + cert, err := x509.ParseCertificate(certChain[len(certChain)-1]) if err != nil { return false, nil, fmt.Errorf("error parsing alternate certificate chain: %w", err) } - log.V(logf.DebugLevel).WithValues("Issuer CN", cert.Issuer.CommonName).Info("Found alternative ACME bundle") + log.V(logf.DebugLevel).WithValues("Issuer CN", cert.Issuer.CommonName).Info("Found ACME bundle") if cert.Issuer.CommonName == preferredChain { // if the issuer's CN matched the preferred chain it means this bundle is // signed by the requested chain - log.V(logf.DebugLevel).WithValues("Issuer CN", cert.Issuer.CommonName).Info("Selecting alternative ACME bundle with a matching Common Name from %s", altURL) - return true, altChain, nil + log.V(logf.DebugLevel).WithValues("Issuer CN", cert.Issuer.CommonName).Info("Selecting preferred ACME bundle with a matching Common Name from %s", chainURL) + return true, certChain, nil } } From ff5c4103a0c55e99c15e6400093c80f599897773 Mon Sep 17 00:00:00 2001 From: Sam Lee Date: Tue, 13 Feb 2024 22:42:00 +0900 Subject: [PATCH 4/8] remove URL verification from alternateCertChain tests Signed-off-by: Sam Lee --- pkg/controller/acmeorders/sync_test.go | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/pkg/controller/acmeorders/sync_test.go b/pkg/controller/acmeorders/sync_test.go index c55b9723f..1d0aea733 100644 --- a/pkg/controller/acmeorders/sync_test.go +++ b/pkg/controller/acmeorders/sync_test.go @@ -651,16 +651,6 @@ rUCGwbCUDI0mxadJ3Bz4WxR6fyNpBK2yAinWEsikxqEt }, FakeFetchCert: func(_ context.Context, url string, bundle bool) ([][]byte, error) { - if url != "http://alturl" { - // This bit just ensures that we - // call it from the correct - // place. This is the same URL - // that is returned from - // FakeCertAlternates that - // should have been called - // before this. - return nil, errors.New("Cert URL is incorrect") - } if !bundle { return nil, errors.New("Expecting to be called with bundle=true") } @@ -698,16 +688,6 @@ rUCGwbCUDI0mxadJ3Bz4WxR6fyNpBK2yAinWEsikxqEt }, FakeFetchCert: func(_ context.Context, url string, bundle bool) ([][]byte, error) { - if url != "http://alturl" { - // This bit just ensures that we - // call it from the correct - // place. This is the same URL - // that is returned from - // FakeCertAlternates that - // should have been called - // before this. - return nil, errors.New("Cert URL is incorrect") - } if !bundle { return nil, errors.New("Expecting to be called with bundle=true") } From 672aad41bf9b5505511213418d0988c3911e7d30 Mon Sep 17 00:00:00 2001 From: Tim Ramlot <42113979+inteon@users.noreply.github.com> Date: Fri, 16 Feb 2024 15:23:22 +0100 Subject: [PATCH 5/8] don't call ListCertAlternates if default chain matches the preferred chain Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com> --- pkg/controller/acmeorders/sync.go | 91 ++++++++++++++++++++++--------- 1 file changed, 65 insertions(+), 26 deletions(-) diff --git a/pkg/controller/acmeorders/sync.go b/pkg/controller/acmeorders/sync.go index 2462692ed..707b68e66 100644 --- a/pkg/controller/acmeorders/sync.go +++ b/pkg/controller/acmeorders/sync.go @@ -586,7 +586,7 @@ func (c *controller) finalizeOrder(ctx context.Context, cl acmecl.Interface, o * if issuer.GetSpec().ACME != nil && issuer.GetSpec().ACME.PreferredChain != "" { preferredChainName := issuer.GetSpec().ACME.PreferredChain - found, preferredCertChain, err := getPreferredCertChain(ctx, cl, certURL, preferredChainName) + found, preferredCertChain, err := getPreferredCertChain(ctx, cl, certURL, certSlice, preferredChainName) if err != nil { return fmt.Errorf("error retrieving alternate chain: %w", err) } @@ -652,16 +652,6 @@ func (c *controller) syncCertificateDataWithOrder(ctx context.Context, cl acmecl return nil } - if issuer.GetSpec().ACME != nil && issuer.GetSpec().ACME.PreferredChain != "" { - found, preferredCertChain, err := getPreferredCertChain(ctx, cl, acmeOrder.CertURL, issuer.GetSpec().ACME.PreferredChain) - if err != nil { - return err - } - if found { - return c.storeCertificateOnStatus(ctx, o, preferredCertChain) - } - - } certs, err := cl.FetchCert(ctx, acmeOrder.CertURL, true) if acmeErr, ok := err.(*acmeapi.Error); ok { if acmeErr.StatusCode >= 400 && acmeErr.StatusCode < 500 { @@ -675,6 +665,16 @@ func (c *controller) syncCertificateDataWithOrder(ctx context.Context, cl acmecl return err } + if issuer.GetSpec().ACME != nil && issuer.GetSpec().ACME.PreferredChain != "" { + found, preferredCertChain, err := getPreferredCertChain(ctx, cl, acmeOrder.CertURL, certs, issuer.GetSpec().ACME.PreferredChain) + if err != nil { + return err + } + if found { + return c.storeCertificateOnStatus(ctx, o, preferredCertChain) + } + } + err = c.storeCertificateOnStatus(ctx, o, certs) if err != nil { return err @@ -700,34 +700,73 @@ func getACMEOrder(ctx context.Context, cl acmecl.Interface, o *cmacme.Order) (*a return acmeOrder, nil } -func getPreferredCertChain(ctx context.Context, cl acmecl.Interface, certURL string, preferredChain string) (bool, [][]byte, error) { +func getPreferredCertChain( + ctx context.Context, + cl acmecl.Interface, + certURL string, + certBundle [][]byte, + preferredChain string, +) (bool, [][]byte, error) { log := logf.FromContext(ctx) - altURLs, err := cl.ListCertAlternates(ctx, certURL) - if err != nil { - return false, nil, fmt.Errorf("error listing alternate certificate URLs: %w", err) - } - // Loop over every chains - for _, chainURL := range append([]string{certURL}, altURLs...) { - certChain, err := cl.FetchCert(ctx, chainURL, true) - if err != nil { - return false, nil, fmt.Errorf("error fetching alternate certificate chain from %s: %w", chainURL, err) + + isMatch := func(name string, chain [][]byte) (bool, error) { + if len(chain) == 0 { + return false, nil } + // Check topmost certificate - cert, err := x509.ParseCertificate(certChain[len(certChain)-1]) + cert, err := x509.ParseCertificate(chain[len(chain)-1]) if err != nil { - return false, nil, fmt.Errorf("error parsing alternate certificate chain: %w", err) + return false, fmt.Errorf("error parsing certificate chain: %w", err) } + log.V(logf.DebugLevel).WithValues("Issuer CN", cert.Issuer.CommonName).Info("Found ACME bundle") if cert.Issuer.CommonName == preferredChain { // if the issuer's CN matched the preferred chain it means this bundle is // signed by the requested chain - log.V(logf.DebugLevel).WithValues("Issuer CN", cert.Issuer.CommonName).Info("Selecting preferred ACME bundle with a matching Common Name from %s", chainURL) - return true, certChain, nil + log.V(logf.DebugLevel).WithValues("Issuer CN", cert.Issuer.CommonName).Info("Selecting preferred ACME bundle with a matching Common Name from %s", name) + return true, nil } + return false, nil } - return false, nil, nil + // Check if the default chain matches the preferred chain + { + match, err := isMatch("default", certBundle) + if err != nil { + return false, nil, err + } + if match { + return true, certBundle, nil + } + } + + // Check if any alternate chain matches the preferred chain + { + altURLs, err := cl.ListCertAlternates(ctx, certURL) + if err != nil { + return false, nil, fmt.Errorf("error listing alternate certificate URLs: %w", err) + } + + for _, chainURL := range altURLs { + certChain, err := cl.FetchCert(ctx, chainURL, true) + if err != nil { + return false, nil, fmt.Errorf("error fetching certificate chain from %s: %w", chainURL, err) + } + + match, err := isMatch(chainURL, certChain) + if err != nil { + return false, nil, err + } + + if match { + return true, certChain, nil + } + } + } + + return false, nil, nil } // updateOrApplyStatus will update the order status. From 205067b834a44b92bcd84accab924dd007eda578 Mon Sep 17 00:00:00 2001 From: Tim Ramlot <42113979+inteon@users.noreply.github.com> Date: Fri, 16 Feb 2024 15:23:45 +0100 Subject: [PATCH 6/8] update tests to match the current Let's encrypt setup Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com> --- pkg/controller/acmeorders/sync_test.go | 232 +++++++++++++++++++------ test/unit/gen/issuer.go | 10 ++ 2 files changed, 192 insertions(+), 50 deletions(-) diff --git a/pkg/controller/acmeorders/sync_test.go b/pkg/controller/acmeorders/sync_test.go index 1d0aea733..12df314b6 100644 --- a/pkg/controller/acmeorders/sync_test.go +++ b/pkg/controller/acmeorders/sync_test.go @@ -36,6 +36,7 @@ import ( cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" testpkg "github.com/cert-manager/cert-manager/pkg/controller/test" schedulertest "github.com/cert-manager/cert-manager/pkg/scheduler/test" + "github.com/cert-manager/cert-manager/pkg/util/pki" "github.com/cert-manager/cert-manager/test/unit/gen" ) @@ -68,7 +69,7 @@ func TestSync(t *testing.T) { })) testIssuerHTTP01TestComPreferredChain := gen.Issuer("testissuer", gen.SetIssuerACME(cmacme.ACMEIssuer{ - PreferredChain: "ISRG Root X1", + PreferredChain: "DST Root CA X3", // This is the common name of the root certificate in the testAltCert Solvers: []cmacme.ACMEChallengeSolver{ { Selector: &cmacme.CertificateDNSNameSelector{ @@ -144,6 +145,124 @@ func TestSync(t *testing.T) { Detail: "some error", } + // testCert is using the following Let's Encrypt chain (X1 is not included): + // leaf -> R3 -> ISRG Root X1 + testCert := `-----BEGIN CERTIFICATE----- +MIIEZjCCA06gAwIBAgISAx0TG3o1EufZi/OTOnR9vqt/MA0GCSqGSIb3DQEBCwUA +MDIxCzAJBgNVBAYTAlVTMRYwFAYDVQQKEw1MZXQncyBFbmNyeXB0MQswCQYDVQQD +EwJSMzAeFw0yMzEyMTkxNjIwMjFaFw0yNDAzMTgxNjIwMjBaMBoxGDAWBgNVBAMT +D2NlcnQtbWFuYWdlci5pbzBZMBMGByqGSM49AgEGCCqGSM49AwEHA0IABCRoMZW8 +FQpb9R2fNhLps2Jms1e058hkiz9PzfyVZT4n0ONmV2OlnNXg7Y3F8v47yc5tq5W6 +8oum8TN+Y2v3u2CjggJXMIICUzAOBgNVHQ8BAf8EBAMCB4AwHQYDVR0lBBYwFAYI +KwYBBQUHAwEGCCsGAQUFBwMCMAwGA1UdEwEB/wQCMAAwHQYDVR0OBBYEFBljJ3oq +zwwTlkvYYFNit4ol03klMB8GA1UdIwQYMBaAFBQusxe3WFbLrlAJQOYfr52LFMLG +MFUGCCsGAQUFBwEBBEkwRzAhBggrBgEFBQcwAYYVaHR0cDovL3IzLm8ubGVuY3Iu +b3JnMCIGCCsGAQUFBzAChhZodHRwOi8vcjMuaS5sZW5jci5vcmcvMF4GA1UdEQRX +MFWCD2NlcnQtbWFuYWdlci5pb4IUZG9jcy5jZXJ0LW1hbmFnZXIuaW+CF25ldGxp +ZnkuY2VydC1tYW5hZ2VyLmlvghN3d3cuY2VydC1tYW5hZ2VyLmlvMBMGA1UdIAQM +MAowCAYGZ4EMAQIBMIIBBgYKKwYBBAHWeQIEAgSB9wSB9ADyAHcASLDja9qmRzQP +5WoC+p0w6xxSActW3SyB2bu/qznYhHMAAAGMgxfC6wAABAMASDBGAiEA1Ac3K8oT +EGY509sNj0/hZ4x5Td6aA3HsElojcF0DOMwCIQDxXgjEDmg0vS4u5BHEndIecmHe +2cMTnTIRM8c9IW0ZTgB3AKLiv9Ye3i8vB6DWTm03p9xlQ7DGtS6i2reK+Jpt9RfY +AAABjIMXwyAAAAQDAEgwRgIhAI9E0vDiqkNXYqtVmQBxM1Nk6eOmeMtZSGoojfBW +IsHBAiEA4S+mvJMqrVQ78UtAT+SGJ9Mr6fb/T45rDmID0PDhuXEwDQYJKoZIhvcN +AQELBQADggEBAJuZ66ArEUG/98Aaz+xYPbpRAfNmllyk9o6exmmZWrAvTBgzCF+D +T+UN8XtOwVW4lTJGHBsXmY9mGtP4lPehGwSD26fJsHTGZYTUGHFwStHrhbu1tyKc +hQg/wgviBN0oRsPWcBqMp0jZkHDNUZPq6fmVGXWeX+sx6Cu+iC8BrdQiEzD8DtrJ +11n6zDjy3mW64D/8MNCGzESbJ9F9N5162Yd3JWHO2eA9FXvcDg6lY2lBitQECqz1 +m8/A7QoPnC9uk/LvEnaqmLbZy7+yK/5+wDbW1y6AbIeo7On1UAXymn5zBTKlEkVm +Q+Rh9actCRTGKaeLO4ar2i59xZ9OnqZhx9c= +-----END CERTIFICATE----- +-----BEGIN CERTIFICATE----- +MIIFFjCCAv6gAwIBAgIRAJErCErPDBinU/bWLiWnX1owDQYJKoZIhvcNAQELBQAw +TzELMAkGA1UEBhMCVVMxKTAnBgNVBAoTIEludGVybmV0IFNlY3VyaXR5IFJlc2Vh +cmNoIEdyb3VwMRUwEwYDVQQDEwxJU1JHIFJvb3QgWDEwHhcNMjAwOTA0MDAwMDAw +WhcNMjUwOTE1MTYwMDAwWjAyMQswCQYDVQQGEwJVUzEWMBQGA1UEChMNTGV0J3Mg +RW5jcnlwdDELMAkGA1UEAxMCUjMwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEK +AoIBAQC7AhUozPaglNMPEuyNVZLD+ILxmaZ6QoinXSaqtSu5xUyxr45r+XXIo9cP +R5QUVTVXjJ6oojkZ9YI8QqlObvU7wy7bjcCwXPNZOOftz2nwWgsbvsCUJCWH+jdx +sxPnHKzhm+/b5DtFUkWWqcFTzjTIUu61ru2P3mBw4qVUq7ZtDpelQDRrK9O8Zutm +NHz6a4uPVymZ+DAXXbpyb/uBxa3Shlg9F8fnCbvxK/eG3MHacV3URuPMrSXBiLxg +Z3Vms/EY96Jc5lP/Ooi2R6X/ExjqmAl3P51T+c8B5fWmcBcUr2Ok/5mzk53cU6cG +/kiFHaFpriV1uxPMUgP17VGhi9sVAgMBAAGjggEIMIIBBDAOBgNVHQ8BAf8EBAMC +AYYwHQYDVR0lBBYwFAYIKwYBBQUHAwIGCCsGAQUFBwMBMBIGA1UdEwEB/wQIMAYB +Af8CAQAwHQYDVR0OBBYEFBQusxe3WFbLrlAJQOYfr52LFMLGMB8GA1UdIwQYMBaA +FHm0WeZ7tuXkAXOACIjIGlj26ZtuMDIGCCsGAQUFBwEBBCYwJDAiBggrBgEFBQcw +AoYWaHR0cDovL3gxLmkubGVuY3Iub3JnLzAnBgNVHR8EIDAeMBygGqAYhhZodHRw +Oi8veDEuYy5sZW5jci5vcmcvMCIGA1UdIAQbMBkwCAYGZ4EMAQIBMA0GCysGAQQB +gt8TAQEBMA0GCSqGSIb3DQEBCwUAA4ICAQCFyk5HPqP3hUSFvNVneLKYY611TR6W +PTNlclQtgaDqw+34IL9fzLdwALduO/ZelN7kIJ+m74uyA+eitRY8kc607TkC53wl +ikfmZW4/RvTZ8M6UK+5UzhK8jCdLuMGYL6KvzXGRSgi3yLgjewQtCPkIVz6D2QQz +CkcheAmCJ8MqyJu5zlzyZMjAvnnAT45tRAxekrsu94sQ4egdRCnbWSDtY7kh+BIm +lJNXoB1lBMEKIq4QDUOXoRgffuDghje1WrG9ML+Hbisq/yFOGwXD9RiX8F6sw6W4 +avAuvDszue5L3sz85K+EC4Y/wFVDNvZo4TYXao6Z0f+lQKc0t8DQYzk1OXVu8rp2 +yJMC6alLbBfODALZvYH7n7do1AZls4I9d1P4jnkDrQoxB3UqQ9hVl3LEKQ73xF1O +yK5GhDDX8oVfGKF5u+decIsH4YaTw7mP3GFxJSqv3+0lUFJoi5Lc5da149p90Ids +hCExroL1+7mryIkXPeFM5TgO9r0rvZaBFOvV2z0gp35Z0+L4WPlbuEjN/lxPFin+ +HlUjr8gRsI3qfJOQFy/9rKIJR0Y/8Omwt/8oTWgy1mdeHmmjk7j1nYsvC9JSQ6Zv +MldlTTKB3zhThV1+XWYp6rjd5JW1zbVWEkLNxE7GJThEUG3szgBVGP7pSWTUTsqX +nLRbwHOoq7hHwg== +-----END CERTIFICATE----- +` + + // testCert is using the following Let's Encrypt chain (DST Root CA X3 is not included): + // leaf -> R3 -> ISRG Root X1 -> DST Root CA X3 + testAltCert := testCert + `-----BEGIN CERTIFICATE----- +MIIFYDCCBEigAwIBAgIQQAF3ITfU6UK47naqPGQKtzANBgkqhkiG9w0BAQsFADA/ +MSQwIgYDVQQKExtEaWdpdGFsIFNpZ25hdHVyZSBUcnVzdCBDby4xFzAVBgNVBAMT +DkRTVCBSb290IENBIFgzMB4XDTIxMDEyMDE5MTQwM1oXDTI0MDkzMDE4MTQwM1ow +TzELMAkGA1UEBhMCVVMxKTAnBgNVBAoTIEludGVybmV0IFNlY3VyaXR5IFJlc2Vh +cmNoIEdyb3VwMRUwEwYDVQQDEwxJU1JHIFJvb3QgWDEwggIiMA0GCSqGSIb3DQEB +AQUAA4ICDwAwggIKAoICAQCt6CRz9BQ385ueK1coHIe+3LffOJCMbjzmV6B493XC +ov71am72AE8o295ohmxEk7axY/0UEmu/H9LqMZshftEzPLpI9d1537O4/xLxIZpL +wYqGcWlKZmZsj348cL+tKSIG8+TA5oCu4kuPt5l+lAOf00eXfJlII1PoOK5PCm+D +LtFJV4yAdLbaL9A4jXsDcCEbdfIwPPqPrt3aY6vrFk/CjhFLfs8L6P+1dy70sntK +4EwSJQxwjQMpoOFTJOwT2e4ZvxCzSow/iaNhUd6shweU9GNx7C7ib1uYgeGJXDR5 +bHbvO5BieebbpJovJsXQEOEO3tkQjhb7t/eo98flAgeYjzYIlefiN5YNNnWe+w5y +sR2bvAP5SQXYgd0FtCrWQemsAXaVCg/Y39W9Eh81LygXbNKYwagJZHduRze6zqxZ +Xmidf3LWicUGQSk+WT7dJvUkyRGnWqNMQB9GoZm1pzpRboY7nn1ypxIFeFntPlF4 +FQsDj43QLwWyPntKHEtzBRL8xurgUBN8Q5N0s8p0544fAQjQMNRbcTa0B7rBMDBc +SLeCO5imfWCKoqMpgsy6vYMEG6KDA0Gh1gXxG8K28Kh8hjtGqEgqiNx2mna/H2ql +PRmP6zjzZN7IKw0KKP/32+IVQtQi0Cdd4Xn+GOdwiK1O5tmLOsbdJ1Fu/7xk9TND +TwIDAQABo4IBRjCCAUIwDwYDVR0TAQH/BAUwAwEB/zAOBgNVHQ8BAf8EBAMCAQYw +SwYIKwYBBQUHAQEEPzA9MDsGCCsGAQUFBzAChi9odHRwOi8vYXBwcy5pZGVudHJ1 +c3QuY29tL3Jvb3RzL2RzdHJvb3RjYXgzLnA3YzAfBgNVHSMEGDAWgBTEp7Gkeyxx ++tvhS5B1/8QVYIWJEDBUBgNVHSAETTBLMAgGBmeBDAECATA/BgsrBgEEAYLfEwEB +ATAwMC4GCCsGAQUFBwIBFiJodHRwOi8vY3BzLnJvb3QteDEubGV0c2VuY3J5cHQu +b3JnMDwGA1UdHwQ1MDMwMaAvoC2GK2h0dHA6Ly9jcmwuaWRlbnRydXN0LmNvbS9E +U1RST09UQ0FYM0NSTC5jcmwwHQYDVR0OBBYEFHm0WeZ7tuXkAXOACIjIGlj26Ztu +MA0GCSqGSIb3DQEBCwUAA4IBAQAKcwBslm7/DlLQrt2M51oGrS+o44+/yQoDFVDC +5WxCu2+b9LRPwkSICHXM6webFGJueN7sJ7o5XPWioW5WlHAQU7G75K/QosMrAdSW +9MUgNTP52GE24HGNtLi1qoJFlcDyqSMo59ahy2cI2qBDLKobkx/J3vWraV0T9VuG +WCLKTVXkcGdtwlfFRjlBz4pYg1htmf5X6DYO8A4jqv2Il9DjXA6USbW1FzXSLr9O +he8Y4IWS6wY7bCkjCWDcRQJMEhg76fsO3txE+FiYruq9RUWhiF1myv4Q6W+CyBFC +Dfvp7OOGAN6dEOM4+qR9sdjoSYKEBpsr6GtPAQw4dy753ec5 +-----END CERTIFICATE----- +` + + decodeAll := func(pemBytes []byte) [][]byte { + var blocks [][]byte + for { + block, rest := pem.Decode(pemBytes) + if block == nil { + break + } + blocks = append(blocks, block.Bytes) + pemBytes = rest + } + return blocks + } + + rawTestCert := decodeAll([]byte(testCert)) + if _, err := pki.ParseSingleCertificateChainPEM([]byte(testCert)); err != nil { + t.Fatalf("error parsing test certificate: %v", err) + } + + rawTestAltCert := decodeAll([]byte(testAltCert)) + if _, err := pki.ParseSingleCertificateChainPEM([]byte(testAltCert)); err != nil { + t.Fatalf("error parsing test certificate: %v", err) + } + testOrderPending := gen.OrderFrom(testOrder, gen.SetOrderStatus(pendingStatus)) testOrderInvalid := testOrderPending.DeepCopy() testOrderInvalid.Status.State = cmacme.Invalid @@ -154,51 +273,13 @@ func TestSync(t *testing.T) { testOrderValid := testOrderPending.DeepCopy() testOrderValid.Status.State = cmacme.Valid // pem encoded word 'test' - testOrderValid.Status.Certificate = []byte(`-----BEGIN CERTIFICATE----- -dGVzdA== ------END CERTIFICATE----- -`) + testOrderValid.Status.Certificate = []byte(testCert) testOrderReady := testOrderPending.DeepCopy() testOrderReady.Status.State = cmacme.Ready - testCert := []byte(`-----BEGIN CERTIFICATE----- -MIIFjTCCA3WgAwIBAgIRANOxciY0IzLc9AUoUSrsnGowDQYJKoZIhvcNAQELBQAw -TzELMAkGA1UEBhMCVVMxKTAnBgNVBAoTIEludGVybmV0IFNlY3VyaXR5IFJlc2Vh -cmNoIEdyb3VwMRUwEwYDVQQDEwxJU1JHIFJvb3QgWDEwHhcNMTYxMDA2MTU0MzU1 -WhcNMjExMDA2MTU0MzU1WjBKMQswCQYDVQQGEwJVUzEWMBQGA1UEChMNTGV0J3Mg -RW5jcnlwdDEjMCEGA1UEAxMaTGV0J3MgRW5jcnlwdCBBdXRob3JpdHkgWDMwggEi -MA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQCc0wzwWuUuR7dyXTeDs2hjMOrX -NSYZJeG9vjXxcJIvt7hLQQWrqZ41CFjssSrEaIcLo+N15Obzp2JxunmBYB/XkZqf -89B4Z3HIaQ6Vkc/+5pnpYDxIzH7KTXcSJJ1HG1rrueweNwAcnKx7pwXqzkrrvUHl -Npi5y/1tPJZo3yMqQpAMhnRnyH+lmrhSYRQTP2XpgofL2/oOVvaGifOFP5eGr7Dc -Gu9rDZUWfcQroGWymQQ2dYBrrErzG5BJeC+ilk8qICUpBMZ0wNAxzY8xOJUWuqgz -uEPxsR/DMH+ieTETPS02+OP88jNquTkxxa/EjQ0dZBYzqvqEKbbUC8DYfcOTAgMB -AAGjggFnMIIBYzAOBgNVHQ8BAf8EBAMCAYYwEgYDVR0TAQH/BAgwBgEB/wIBADBU -BgNVHSAETTBLMAgGBmeBDAECATA/BgsrBgEEAYLfEwEBATAwMC4GCCsGAQUFBwIB -FiJodHRwOi8vY3BzLnJvb3QteDEubGV0c2VuY3J5cHQub3JnMB0GA1UdDgQWBBSo -SmpjBH3duubRObemRWXv86jsoTAzBgNVHR8ELDAqMCigJqAkhiJodHRwOi8vY3Js -LnJvb3QteDEubGV0c2VuY3J5cHQub3JnMHIGCCsGAQUFBwEBBGYwZDAwBggrBgEF -BQcwAYYkaHR0cDovL29jc3Aucm9vdC14MS5sZXRzZW5jcnlwdC5vcmcvMDAGCCsG -AQUFBzAChiRodHRwOi8vY2VydC5yb290LXgxLmxldHNlbmNyeXB0Lm9yZy8wHwYD -VR0jBBgwFoAUebRZ5nu25eQBc4AIiMgaWPbpm24wDQYJKoZIhvcNAQELBQADggIB -ABnPdSA0LTqmRf/Q1eaM2jLonG4bQdEnqOJQ8nCqxOeTRrToEKtwT++36gTSlBGx -A/5dut82jJQ2jxN8RI8L9QFXrWi4xXnA2EqA10yjHiR6H9cj6MFiOnb5In1eWsRM -UM2v3e9tNsCAgBukPHAg1lQh07rvFKm/Bz9BCjaxorALINUfZ9DD64j2igLIxle2 -DPxW8dI/F2loHMjXZjqG8RkqZUdoxtID5+90FgsGIfkMpqgRS05f4zPbCEHqCXl1 -eO5HyELTgcVlLXXQDgAWnRzut1hFJeczY1tjQQno6f6s+nMydLN26WuU4s3UYvOu -OsUxRlJu7TSRHqDC3lSE5XggVkzdaPkuKGQbGpny+01/47hfXXNB7HntWNZ6N2Vw -p7G6OfY+YQrZwIaQmhrIqJZuigsrbe3W+gdn5ykE9+Ky0VgVUsfxo52mwFYs1JKY -2PGDuWx8M6DlS6qQkvHaRUo0FMd8TsSlbF0/v965qGFKhSDeQoMpYnwcmQilRh/0 -ayLThlHLN81gSkJjVrPI0Y8xCVPB4twb1PFUd2fPM3sA1tJ83sZ5v8vgFv2yofKR -PB0t6JzUA81mSqM3kxl5e+IZwhYAyO0OTg3/fs8HqGTNKd9BqoUwSRBzp06JMg5b -rUCGwbCUDI0mxadJ3Bz4WxR6fyNpBK2yAinWEsikxqEt ------END CERTIFICATE----- -`) - rawTestCert, _ := pem.Decode(testCert) - testOrderValidAltCert := gen.OrderFrom(testOrder, gen.SetOrderStatus(pendingStatus)) testOrderValidAltCert.Status.State = cmacme.Valid - testOrderValidAltCert.Status.Certificate = testCert + testOrderValidAltCert.Status.Certificate = []byte(testAltCert) fakeHTTP01ACMECl := &acmecl.FakeACME{ FakeHTTP01ChallengeResponse: func(s string) (string, error) { @@ -251,6 +332,7 @@ rUCGwbCUDI0mxadJ3Bz4WxR6fyNpBK2yAinWEsikxqEt testACMEOrderValid := &acmeapi.Order{} *testACMEOrderValid = *testACMEOrderPending testACMEOrderValid.Status = acmeapi.StatusValid + testACMEOrderValid.CertURL = "http://testurl" // shallow copy testACMEOrderReady := &acmeapi.Order{} *testACMEOrderReady = *testACMEOrderPending @@ -540,8 +622,7 @@ rUCGwbCUDI0mxadJ3Bz4WxR6fyNpBK2yAinWEsikxqEt return testACMEOrderValid, nil }, FakeCreateOrderCert: func(_ context.Context, url string, csr []byte, bundle bool) ([][]byte, string, error) { - testData := []byte("test") - return [][]byte{testData}, "http://testurl", nil + return rawTestCert, testACMEOrderValid.CertURL, nil }, FakeHTTP01ChallengeResponse: func(s string) (string, error) { // TODO: assert s = "token" @@ -617,7 +698,13 @@ rUCGwbCUDI0mxadJ3Bz4WxR6fyNpBK2yAinWEsikxqEt return "key", nil }, FakeFetchCert: func(_ context.Context, url string, bundle bool) ([][]byte, error) { - return [][]byte{[]byte("test")}, nil + if url != testACMEOrderValid.CertURL { + return nil, errors.New("Cert URL is incorrect") + } + if !bundle { + return nil, errors.New("Expecting to be called with bundle=true") + } + return rawTestCert, nil }, }, expectErr: false, @@ -647,14 +734,22 @@ rUCGwbCUDI0mxadJ3Bz4WxR6fyNpBK2yAinWEsikxqEt return "key", nil }, FakeListCertAlternates: func(_ context.Context, url string) ([]string, error) { + if url != testACMEOrderValid.CertURL { + return nil, errors.New("Cert URL is incorrect") + } return []string{"http://alturl"}, nil - }, FakeFetchCert: func(_ context.Context, url string, bundle bool) ([][]byte, error) { + if url != testACMEOrderValid.CertURL && url != "http://alturl" { + return nil, errors.New("Cert URL is incorrect") + } if !bundle { return nil, errors.New("Expecting to be called with bundle=true") } - return [][]byte{rawTestCert.Bytes}, nil + if url == testACMEOrderValid.CertURL { + return rawTestCert, nil + } + return rawTestAltCert, nil }, }, expectErr: false, @@ -677,21 +772,58 @@ rUCGwbCUDI0mxadJ3Bz4WxR6fyNpBK2yAinWEsikxqEt return testACMEOrderValid, nil }, FakeCreateOrderCert: func(_ context.Context, url string, csr []byte, bundle bool) ([][]byte, string, error) { - testData := []byte("test") - return [][]byte{testData}, "http://testurl", nil + return rawTestCert, testACMEOrderValid.CertURL, nil }, FakeListCertAlternates: func(_ context.Context, url string) ([]string, error) { - if url != "http://testurl" { + if url != testACMEOrderValid.CertURL { return nil, errors.New("Cert URL is incorrect") } return []string{"http://alturl"}, nil }, FakeFetchCert: func(_ context.Context, url string, bundle bool) ([][]byte, error) { + if url != "http://alturl" { + return nil, errors.New("Cert URL is incorrect: expected http://alturl got " + url) + } if !bundle { return nil, errors.New("Expecting to be called with bundle=true") } - return [][]byte{rawTestCert.Bytes}, nil + return rawTestAltCert, nil + }, + FakeHTTP01ChallengeResponse: func(s string) (string, error) { + // TODO: assert s = "token" + return "key", nil + }, + }, + }, + "preferred chain is default cert chain": { + order: testOrderReady.DeepCopy(), + builder: &testpkg.Builder{ + CertManagerObjects: []runtime.Object{ + gen.IssuerFrom(testIssuerHTTP01TestComPreferredChain, gen.SetIssuerACMEPreferredChain("ISRG Root X1")), + testOrderReady, testAuthorizationChallengeValid, + }, + ExpectedActions: []testpkg.Action{ + testpkg.NewAction(coretesting.NewUpdateSubresourceAction(cmacme.SchemeGroupVersion.WithResource("orders"), + "status", + testOrderValid.Namespace, testOrderValid)), + }, + ExpectedEvents: []string{ + "Normal Complete Order completed successfully", + }, + }, + acmeClient: &acmecl.FakeACME{ + FakeGetOrder: func(_ context.Context, url string) (*acmeapi.Order, error) { + return testACMEOrderValid, nil + }, + FakeCreateOrderCert: func(_ context.Context, url string, csr []byte, bundle bool) ([][]byte, string, error) { + return rawTestCert, testACMEOrderValid.CertURL, nil + }, + FakeListCertAlternates: func(_ context.Context, url string) ([]string, error) { + return nil, errors.New("should not be called") + }, + FakeFetchCert: func(_ context.Context, url string, bundle bool) ([][]byte, error) { + return nil, errors.New("should not be called") }, FakeHTTP01ChallengeResponse: func(s string) (string, error) { // TODO: assert s = "token" diff --git a/test/unit/gen/issuer.go b/test/unit/gen/issuer.go index 1ab46f34f..deb30c1ce 100644 --- a/test/unit/gen/issuer.go +++ b/test/unit/gen/issuer.go @@ -95,6 +95,16 @@ func SetIssuerACME(a cmacme.ACMEIssuer) IssuerModifier { } } +func SetIssuerACMEPreferredChain(chain string) IssuerModifier { + return func(iss v1.GenericIssuer) { + spec := iss.GetSpec() + if spec.ACME == nil { + spec.ACME = &cmacme.ACMEIssuer{} + } + spec.ACME.PreferredChain = chain + } +} + func SetIssuerACMEURL(url string) IssuerModifier { return func(iss v1.GenericIssuer) { spec := iss.GetSpec() From 0ed660873e15446d612405be0b4e578a8a228a2c Mon Sep 17 00:00:00 2001 From: Tim Ramlot <42113979+inteon@users.noreply.github.com> Date: Fri, 16 Feb 2024 19:49:28 +0100 Subject: [PATCH 7/8] fix incorrect comments and error messages Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com> --- pkg/apis/acme/v1/types_issuer.go | 5 +++-- pkg/controller/acmeorders/sync.go | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/pkg/apis/acme/v1/types_issuer.go b/pkg/apis/acme/v1/types_issuer.go index f20251563..db9415cc9 100644 --- a/pkg/apis/acme/v1/types_issuer.go +++ b/pkg/apis/acme/v1/types_issuer.go @@ -48,8 +48,9 @@ type ACMEIssuer struct { // 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. - // This value picks the first certificate bundle in the ACME alternative - // chains that has a certificate with this value as its issuer's CN + // This value picks the first certificate bundle in the combined set of + // ACME default and alternative chains that has a root-most certificate with + // this value as its issuer's commonname. // +optional // +kubebuilder:validation:MaxLength=64 PreferredChain string `json:"preferredChain,omitempty"` diff --git a/pkg/controller/acmeorders/sync.go b/pkg/controller/acmeorders/sync.go index 707b68e66..143306eb6 100644 --- a/pkg/controller/acmeorders/sync.go +++ b/pkg/controller/acmeorders/sync.go @@ -588,7 +588,7 @@ func (c *controller) finalizeOrder(ctx context.Context, cl acmecl.Interface, o * preferredChainName := issuer.GetSpec().ACME.PreferredChain found, preferredCertChain, err := getPreferredCertChain(ctx, cl, certURL, certSlice, preferredChainName) if err != nil { - return fmt.Errorf("error retrieving alternate chain: %w", err) + return fmt.Errorf("error retrieving preferred chain: %w", err) } if found { return c.storeCertificateOnStatus(ctx, o, preferredCertChain) From f44238fd80176a5d5b3d8eca90bd4fe4e5fa3fbb Mon Sep 17 00:00:00 2001 From: Tim Ramlot <42113979+inteon@users.noreply.github.com> Date: Sun, 18 Feb 2024 20:16:09 +0100 Subject: [PATCH 8/8] run 'make update-crds' Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com> --- deploy/crds/crd-clusterissuers.yaml | 2 +- deploy/crds/crd-issuers.yaml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/deploy/crds/crd-clusterissuers.yaml b/deploy/crds/crd-clusterissuers.yaml index 8d850f68a..ced39f915 100644 --- a/deploy/crds/crd-clusterissuers.yaml +++ b/deploy/crds/crd-clusterissuers.yaml @@ -102,7 +102,7 @@ spec: description: 'Name of the resource being referred to. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names' type: string preferredChain: - description: 'PreferredChain is the chain to use if the ACME server outputs multiple. 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. This value picks the first certificate bundle in the ACME alternative chains that has a certificate with this value as its issuer''s CN' + description: 'PreferredChain is the chain to use if the ACME server outputs multiple. 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. This value picks the first certificate bundle in the combined set of ACME default and alternative chains that has a root-most certificate with this value as its issuer''s commonname.' type: string maxLength: 64 privateKeySecretRef: diff --git a/deploy/crds/crd-issuers.yaml b/deploy/crds/crd-issuers.yaml index 924181324..10797f75b 100644 --- a/deploy/crds/crd-issuers.yaml +++ b/deploy/crds/crd-issuers.yaml @@ -102,7 +102,7 @@ spec: description: 'Name of the resource being referred to. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names' type: string preferredChain: - description: 'PreferredChain is the chain to use if the ACME server outputs multiple. 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. This value picks the first certificate bundle in the ACME alternative chains that has a certificate with this value as its issuer''s CN' + description: 'PreferredChain is the chain to use if the ACME server outputs multiple. 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. This value picks the first certificate bundle in the combined set of ACME default and alternative chains that has a root-most certificate with this value as its issuer''s commonname.' type: string maxLength: 64 privateKeySecretRef: