From c58b08e7b7affb3c7c26740f02cd47637317dc0f Mon Sep 17 00:00:00 2001 From: Tim Ramlot <42113979+inteon@users.noreply.github.com> Date: Tue, 2 Jul 2024 13:38:35 +0200 Subject: [PATCH] pki match: remove return values that are always nil Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com> --- .../certificates/policies/checks.go | 15 ++-- .../issuing/issuing_controller.go | 5 +- .../keymanager/keymanager_controller.go | 12 +-- pkg/util/pki/match.go | 58 +++++++------- pkg/util/pki/match_test.go | 80 ++++++++----------- 5 files changed, 71 insertions(+), 99 deletions(-) diff --git a/internal/controller/certificates/policies/checks.go b/internal/controller/certificates/policies/checks.go index a73c92ae1..06eacfee4 100644 --- a/internal/controller/certificates/policies/checks.go +++ b/internal/controller/certificates/policies/checks.go @@ -87,10 +87,7 @@ func SecretPrivateKeyMismatchesSpec(input Input) (string, string, bool) { return InvalidKeyPair, fmt.Sprintf("Issuing certificate as Secret contains invalid private key data: %v", err), true } - violations, err := pki.PrivateKeyMatchesSpec(pk, input.Certificate.Spec) - if err != nil { - return SecretMismatch, fmt.Sprintf("Failed to check private key is up to date: %v", err), true - } + violations := pki.PrivateKeyMatchesSpec(pk, input.Certificate.Spec) if len(violations) > 0 { return SecretMismatch, fmt.Sprintf("Existing private key is not up to date for spec: %v", violations), true } @@ -240,14 +237,12 @@ func CurrentCertificateRequestMismatchesSpec(input Input) (string, string, bool) // and is instead called by currentCertificateRequestValidForSpec if no there // is no existing CertificateRequest resource. func currentSecretValidForSpec(input Input) (string, string, bool) { - violations, err := pki.SecretDataAltNamesMatchSpec(input.Secret, input.Certificate.Spec) + x509Cert, err := pki.DecodeX509CertificateBytes(input.Secret.Data[corev1.TLSCertKey]) if err != nil { - // This case should never be reached as we already check the certificate data can - // be parsed in an earlier policy check, but handle it anyway. - // TODO: log a message - return "", "", false + return InvalidCertificate, fmt.Sprintf("Issuing certificate as Secret contains an invalid certificate: %v", err), true } - + // nolint: staticcheck // FuzzyX509AltNamesMatchSpec is used here for backwards compatibility + violations := pki.FuzzyX509AltNamesMatchSpec(x509Cert, input.Certificate.Spec) if len(violations) > 0 { return SecretMismatch, fmt.Sprintf("Issuing certificate as Existing issued Secret is not up to date for spec: %v", violations), true } diff --git a/pkg/controller/certificates/issuing/issuing_controller.go b/pkg/controller/certificates/issuing/issuing_controller.go index 44c9d1084..e250bdde0 100644 --- a/pkg/controller/certificates/issuing/issuing_controller.go +++ b/pkg/controller/certificates/issuing/issuing_controller.go @@ -208,10 +208,7 @@ func (c *controller) ProcessItem(ctx context.Context, key string) error { logf.WithResource(log, nextPrivateKeySecret).Error(err, "failed to parse next private key, waiting for keymanager controller") return nil } - pkViolations, err := pki.PrivateKeyMatchesSpec(pk, crt.Spec) - if err != nil { - return err - } + pkViolations := pki.PrivateKeyMatchesSpec(pk, crt.Spec) if len(pkViolations) > 0 { logf.WithResource(log, nextPrivateKeySecret).Info("stored next private key does not match requirements on Certificate resource, waiting for keymanager controller", "violations", pkViolations) return nil diff --git a/pkg/controller/certificates/keymanager/keymanager_controller.go b/pkg/controller/certificates/keymanager/keymanager_controller.go index 6bae89d5e..09c04366c 100644 --- a/pkg/controller/certificates/keymanager/keymanager_controller.go +++ b/pkg/controller/certificates/keymanager/keymanager_controller.go @@ -212,11 +212,7 @@ func (c *controller) ProcessItem(ctx context.Context, key string) error { return c.deleteSecretResources(ctx, secrets) } - violations, err := pki.PrivateKeyMatchesSpec(pk, crt.Spec) - if err != nil { - log.Error(err, "Internal error verifying if private key matches spec - please open an issue.") - return nil - } + violations := pki.PrivateKeyMatchesSpec(pk, crt.Spec) if len(violations) > 0 { log.V(logf.DebugLevel).Info("Regenerating private key due to change in fields", "violations", violations) c.recorder.Eventf(crt, corev1.EventTypeNormal, reasonDeleted, "Regenerating private key due to change in fields: %v", violations) @@ -246,11 +242,7 @@ func (c *controller) createNextPrivateKeyRotationPolicyNever(ctx context.Context c.recorder.Eventf(crt, corev1.EventTypeWarning, reasonDecodeFailed, "Failed to decode private key stored in Secret %q - generating new key", crt.Spec.SecretName) return c.createAndSetNextPrivateKey(ctx, crt) } - violations, err := pki.PrivateKeyMatchesSpec(pk, crt.Spec) - if err != nil { - c.recorder.Eventf(crt, corev1.EventTypeWarning, reasonDecodeFailed, "Failed to check if private key stored in Secret %q is up to date - generating new key", crt.Spec.SecretName) - return c.createAndSetNextPrivateKey(ctx, crt) - } + violations := pki.PrivateKeyMatchesSpec(pk, crt.Spec) if len(violations) > 0 { c.recorder.Eventf(crt, corev1.EventTypeWarning, reasonCannotRegenerateKey, "User intervention required: existing private key in Secret %q does not match requirements on Certificate resource, mismatching fields: %v, but cert-manager cannot create new private key as the Certificate's .spec.privateKey.rotationPolicy is unset or set to Never. To allow cert-manager to create a new private key you can set .spec.privateKey.rotationPolicy to 'Always' (this will result in the private key being regenerated every time a cert is renewed) ", crt.Spec.SecretName, violations) return nil diff --git a/pkg/util/pki/match.go b/pkg/util/pki/match.go index 9f07d0593..5d7612688 100644 --- a/pkg/util/pki/match.go +++ b/pkg/util/pki/match.go @@ -22,23 +22,26 @@ import ( "crypto/ecdsa" "crypto/ed25519" "crypto/rsa" + "crypto/x509" "crypto/x509/pkix" "encoding/asn1" "fmt" "net" "reflect" - corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/util/sets" cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" "github.com/cert-manager/cert-manager/pkg/util" ) -// PrivateKeyMatchesSpec returns an error if the private key bit size -// doesn't match the provided spec. RSA, Ed25519 and ECDSA are supported. -// If any error is returned, a list of violations will also be returned. -func PrivateKeyMatchesSpec(pk crypto.PrivateKey, spec cmapi.CertificateSpec) ([]string, error) { +// PrivateKeyMatchesSpec returns a list of violations for the provided private +// key against the provided CertificateSpec. It will return an empty list/ nil +// if there are no violations found. RSA, Ed25519 and ECDSA private keys are +// supported. +// The function panics if the CertificateSpec contains an unknown key algorithm, +// since this should have been caught by the CertificateSpec validation already. +func PrivateKeyMatchesSpec(pk crypto.PrivateKey, spec cmapi.CertificateSpec) []string { spec = *spec.DeepCopy() if spec.PrivateKey == nil { spec.PrivateKey = &cmapi.CertificatePrivateKey{} @@ -51,14 +54,16 @@ func PrivateKeyMatchesSpec(pk crypto.PrivateKey, spec cmapi.CertificateSpec) ([] case cmapi.ECDSAKeyAlgorithm: return ecdsaPrivateKeyMatchesSpec(pk, spec) default: - return nil, fmt.Errorf("unrecognised key algorithm type %q", spec.PrivateKey.Algorithm) + // This should never happen as the CertificateSpec validation should + // catch this before it reaches this point. + panic(fmt.Sprintf("[PROGRAMMING ERROR] unrecognised key algorithm type %q", spec.PrivateKey.Algorithm)) } } -func rsaPrivateKeyMatchesSpec(pk crypto.PrivateKey, spec cmapi.CertificateSpec) ([]string, error) { +func rsaPrivateKeyMatchesSpec(pk crypto.PrivateKey, spec cmapi.CertificateSpec) []string { rsaPk, ok := pk.(*rsa.PrivateKey) if !ok { - return []string{"spec.privateKey.algorithm"}, nil + return []string{"spec.privateKey.algorithm"} } var violations []string // TODO: we should not use implicit defaulting here, and instead rely on @@ -73,13 +78,13 @@ func rsaPrivateKeyMatchesSpec(pk crypto.PrivateKey, spec cmapi.CertificateSpec) if rsaPk.N.BitLen() != keySize { violations = append(violations, "spec.privateKey.size") } - return violations, nil + return violations } -func ecdsaPrivateKeyMatchesSpec(pk crypto.PrivateKey, spec cmapi.CertificateSpec) ([]string, error) { +func ecdsaPrivateKeyMatchesSpec(pk crypto.PrivateKey, spec cmapi.CertificateSpec) []string { ecdsaPk, ok := pk.(*ecdsa.PrivateKey) if !ok { - return []string{"spec.privateKey.algorithm"}, nil + return []string{"spec.privateKey.algorithm"} } var violations []string // TODO: we should not use implicit defaulting here, and instead rely on @@ -94,16 +99,16 @@ func ecdsaPrivateKeyMatchesSpec(pk crypto.PrivateKey, spec cmapi.CertificateSpec if expectedKeySize != ecdsaPk.Curve.Params().BitSize { violations = append(violations, "spec.privateKey.size") } - return violations, nil + return violations } -func ed25519PrivateKeyMatchesSpec(pk crypto.PrivateKey) ([]string, error) { +func ed25519PrivateKeyMatchesSpec(pk crypto.PrivateKey) []string { _, ok := pk.(ed25519.PrivateKey) if !ok { - return []string{"spec.privateKey.algorithm"}, nil + return []string{"spec.privateKey.algorithm"} } - return nil, nil + return nil } func ipSlicesMatch(parsedIPs []net.IP, stringIPs []string) bool { @@ -273,17 +278,16 @@ func matchOtherNames(extension []pkix.Extension, specOtherNames []cmapi.OtherNam return true, nil } -// SecretDataAltNamesMatchSpec will compare a Secret resource containing certificate -// data to a CertificateSpec and return a list of 'violations' for any fields that -// do not match their counterparts. +// FuzzyX509AltNamesMatchSpec will compare a X509 Certificate to a CertificateSpec +// and return a list of 'violations' for any fields that do not match their counterparts. +// // This is a purposely less comprehensive check than RequestMatchesSpec as some // issuers override/force certain fields. -func SecretDataAltNamesMatchSpec(secret *corev1.Secret, spec cmapi.CertificateSpec) ([]string, error) { - x509cert, err := DecodeX509CertificateBytes(secret.Data[corev1.TLSCertKey]) - if err != nil { - return nil, err - } - +// +// Deprecated: This function is very fuzzy and makes too many assumptions about +// how the issuer maps a CSR to a certificate. We only keep it for backward compatibility +// reasons, but use other comparison functions when possible. +func FuzzyX509AltNamesMatchSpec(x509cert *x509.Certificate, spec cmapi.CertificateSpec) []string { var violations []string // Perform a 'loose' check on the x509 certificate to determine if the @@ -291,11 +295,11 @@ func SecretDataAltNamesMatchSpec(secret *corev1.Secret, spec cmapi.CertificateSp // This check allows names to move between the DNSNames and CommonName // field freely in order to account for CAs behaviour of promoting DNSNames // to be CommonNames or vice-versa. - expectedDNSNames := sets.New[string](spec.DNSNames...) + expectedDNSNames := sets.New(spec.DNSNames...) if spec.CommonName != "" { expectedDNSNames.Insert(spec.CommonName) } - allDNSNames := sets.New[string](x509cert.DNSNames...) + allDNSNames := sets.New(x509cert.DNSNames...) if x509cert.Subject.CommonName != "" { allDNSNames.Insert(x509cert.Subject.CommonName) } @@ -322,7 +326,7 @@ func SecretDataAltNamesMatchSpec(secret *corev1.Secret, spec cmapi.CertificateSp violations = append(violations, "spec.emailAddresses") } - return violations, nil + return violations } func extractSANExtension(extensions []pkix.Extension) (pkix.Extension, error) { diff --git a/pkg/util/pki/match_test.go b/pkg/util/pki/match_test.go index 119bb8408..56b1a0686 100644 --- a/pkg/util/pki/match_test.go +++ b/pkg/util/pki/match_test.go @@ -18,12 +18,12 @@ package pki_test import ( "crypto" + "crypto/rand" "crypto/x509" "encoding/asn1" "reflect" "testing" - corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" @@ -61,7 +61,6 @@ func TestPrivateKeyMatchesSpec(t *testing.T) { expectedAlgo cmapi.PrivateKeyAlgorithm expectedSize int violations []string - err string }{ "should match if keySize and algorithm are correct (RSA)": { key: mustGenerateRSA(t, 2048), @@ -98,7 +97,7 @@ func TestPrivateKeyMatchesSpec(t *testing.T) { } for name, test := range tests { t.Run(name, func(t *testing.T) { - violations, err := pki.PrivateKeyMatchesSpec( + violations := pki.PrivateKeyMatchesSpec( test.key, cmapi.CertificateSpec{ PrivateKey: &cmapi.CertificatePrivateKey{ @@ -107,16 +106,6 @@ func TestPrivateKeyMatchesSpec(t *testing.T) { }, }, ) - switch { - case err != nil: - if test.err != err.Error() { - t.Errorf("error text did not match, got=%s, exp=%s", err.Error(), test.err) - } - default: - if test.err != "" { - t.Errorf("got no error but expected: %s", test.err) - } - } if !reflect.DeepEqual(violations, test.violations) { t.Errorf("violations did not match, got=%s, exp=%s", violations, test.violations) } @@ -302,11 +291,10 @@ func TestRequestMatchesSpecSubject(t *testing.T) { } } -func TestSecretDataAltNamesMatchSpec(t *testing.T) { +func TestFuzzyX509AltNamesMatchSpec(t *testing.T) { tests := map[string]struct { - data []byte + x509 *x509.Certificate spec cmapi.CertificateSpec - err string violations []string }{ "should match if common name and dns names exactly equal": { @@ -314,7 +302,7 @@ func TestSecretDataAltNamesMatchSpec(t *testing.T) { CommonName: "cn", DNSNames: []string{"at", "least", "one"}, }, - data: selfSignCertificate(t, cmapi.CertificateSpec{ + x509: selfSignCertificate(t, cmapi.CertificateSpec{ CommonName: "cn", DNSNames: []string{"at", "least", "one"}, }), @@ -324,7 +312,7 @@ func TestSecretDataAltNamesMatchSpec(t *testing.T) { CommonName: "cn", DNSNames: []string{"at", "least", "one"}, }, - data: selfSignCertificate(t, cmapi.CertificateSpec{ + x509: selfSignCertificate(t, cmapi.CertificateSpec{ DNSNames: []string{"cn", "at", "least", "one"}, }), }, @@ -333,7 +321,7 @@ func TestSecretDataAltNamesMatchSpec(t *testing.T) { CommonName: "cn", DNSNames: []string{"at", "least", "one"}, }, - data: selfSignCertificate(t, cmapi.CertificateSpec{ + x509: selfSignCertificate(t, cmapi.CertificateSpec{ DNSNames: []string{"at", "least", "one", "cn"}, }), }, @@ -341,7 +329,7 @@ func TestSecretDataAltNamesMatchSpec(t *testing.T) { spec: cmapi.CertificateSpec{ DNSNames: []string{"at", "least", "one"}, }, - data: selfSignCertificate(t, cmapi.CertificateSpec{ + x509: selfSignCertificate(t, cmapi.CertificateSpec{ CommonName: "at", DNSNames: []string{"least", "one"}, }), @@ -351,7 +339,7 @@ func TestSecretDataAltNamesMatchSpec(t *testing.T) { CommonName: "cn", DNSNames: []string{"at", "least", "one"}, }, - data: selfSignCertificate(t, cmapi.CertificateSpec{ + x509: selfSignCertificate(t, cmapi.CertificateSpec{ DNSNames: []string{"at", "least", "one"}, }), violations: []string{"spec.commonName"}, @@ -361,7 +349,7 @@ func TestSecretDataAltNamesMatchSpec(t *testing.T) { CommonName: "cn", DNSNames: []string{"at", "least", "one", "other"}, }, - data: selfSignCertificate(t, cmapi.CertificateSpec{ + x509: selfSignCertificate(t, cmapi.CertificateSpec{ DNSNames: []string{"at", "least", "one"}, }), violations: []string{"spec.commonName", "spec.dnsNames"}, @@ -370,7 +358,7 @@ func TestSecretDataAltNamesMatchSpec(t *testing.T) { spec: cmapi.CertificateSpec{ DNSNames: []string{"at", "least", "one"}, }, - data: selfSignCertificate(t, cmapi.CertificateSpec{ + x509: selfSignCertificate(t, cmapi.CertificateSpec{ CommonName: "cn", DNSNames: []string{"at", "least", "one", "other"}, }), @@ -381,7 +369,7 @@ func TestSecretDataAltNamesMatchSpec(t *testing.T) { CommonName: "cn", DNSNames: []string{"at", "least", "one"}, }, - data: selfSignCertificate(t, cmapi.CertificateSpec{ + x509: selfSignCertificate(t, cmapi.CertificateSpec{ CommonName: "cn", DNSNames: []string{"at", "least", "one", "other"}, }), @@ -391,7 +379,7 @@ func TestSecretDataAltNamesMatchSpec(t *testing.T) { spec: cmapi.CertificateSpec{ DNSNames: []string{"at", "least", "one"}, }, - data: selfSignCertificate(t, cmapi.CertificateSpec{ + x509: selfSignCertificate(t, cmapi.CertificateSpec{ CommonName: "at", DNSNames: []string{"at", "least", "one"}, }), @@ -401,7 +389,7 @@ func TestSecretDataAltNamesMatchSpec(t *testing.T) { CommonName: "cn", DNSNames: []string{"at", "least", "one"}, }, - data: selfSignCertificate(t, cmapi.CertificateSpec{ + x509: selfSignCertificate(t, cmapi.CertificateSpec{ CommonName: "at", DNSNames: []string{"at", "least", "one", "cn"}, }), @@ -410,7 +398,7 @@ func TestSecretDataAltNamesMatchSpec(t *testing.T) { spec: cmapi.CertificateSpec{ IPAddresses: []string{"127.0.0.1"}, }, - data: selfSignCertificate(t, cmapi.CertificateSpec{ + x509: selfSignCertificate(t, cmapi.CertificateSpec{ IPAddresses: []string{"127.0.0.1"}, }), }, @@ -418,7 +406,7 @@ func TestSecretDataAltNamesMatchSpec(t *testing.T) { spec: cmapi.CertificateSpec{ IPAddresses: []string{"127.0.0.1"}, }, - data: selfSignCertificate(t, cmapi.CertificateSpec{ + x509: selfSignCertificate(t, cmapi.CertificateSpec{ IPAddresses: []string{"127.0.2.1"}, }), violations: []string{"spec.ipAddresses"}, @@ -427,7 +415,7 @@ func TestSecretDataAltNamesMatchSpec(t *testing.T) { spec: cmapi.CertificateSpec{ IPAddresses: []string{"127.0.0.1"}, }, - data: selfSignCertificate(t, cmapi.CertificateSpec{ + x509: selfSignCertificate(t, cmapi.CertificateSpec{ CommonName: "127.0.0.1", IPAddresses: []string{"127.0.0.1"}, }), @@ -436,17 +424,7 @@ func TestSecretDataAltNamesMatchSpec(t *testing.T) { } for name, test := range tests { t.Run(name, func(t *testing.T) { - violations, err := pki.SecretDataAltNamesMatchSpec(&corev1.Secret{Data: map[string][]byte{corev1.TLSCertKey: test.data}}, test.spec) - switch { - case err != nil: - if test.err != err.Error() { - t.Errorf("error text did not match, got=%s, exp=%s", err.Error(), test.err) - } - default: - if test.err != "" { - t.Errorf("got no error but expected: %s", test.err) - } - } + violations := pki.FuzzyX509AltNamesMatchSpec(test.x509, test.spec) if !reflect.DeepEqual(violations, test.violations) { t.Errorf("violations did not match, got=%s, exp=%s", violations, test.violations) } @@ -454,23 +432,29 @@ func TestSecretDataAltNamesMatchSpec(t *testing.T) { } } -func selfSignCertificate(t *testing.T, spec cmapi.CertificateSpec) []byte { - pk, err := pki.GenerateRSAPrivateKey(2048) - if err != nil { - t.Fatal(err) - } - +func selfSignCertificate(t *testing.T, spec cmapi.CertificateSpec) *x509.Certificate { template, err := pki.CertificateTemplateFromCertificate(&cmapi.Certificate{Spec: spec}) if err != nil { t.Fatal(err) } - pemData, _, err := pki.SignCertificate(template, template, pk.Public(), pk) + pk, err := pki.GenerateRSAPrivateKey(2048) if err != nil { t.Fatal(err) } - return pemData + // Marshal and unmarshal to ensure all fields are set correctly + certDER, err := x509.CreateCertificate(rand.Reader, template, template, pk.Public(), pk) + if err != nil { + t.Fatal(err) + } + + cert, err := x509.ParseCertificate(certDER) + if err != nil { + t.Fatal(err) + } + + return cert } func mustBuildCertificateRequest(t *testing.T, crt *cmapi.Certificate) *cmapi.CertificateRequest {