From 36ddf19e2e5cd1bb64343e21132376097d5bdb16 Mon Sep 17 00:00:00 2001 From: Tim Ramlot <42113979+inteon@users.noreply.github.com> Date: Mon, 24 Jul 2023 09:42:19 +0200 Subject: [PATCH] improve Trigger, Readiness and PostIssuance Policy chains Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com> --- .../certificates/policies/checks.go | 107 +++++++++--------- .../certificates/policies/checks_test.go | 72 ++++++++---- .../certificates/policies/policies.go | 53 +++++---- .../certificates/issuing/internal/secret.go | 15 ++- .../readiness/readiness_controller_test.go | 8 +- 5 files changed, 147 insertions(+), 108 deletions(-) diff --git a/internal/controller/certificates/policies/checks.go b/internal/controller/certificates/policies/checks.go index 4dcafd34485..6e3afe6d339 100644 --- a/internal/controller/certificates/policies/checks.go +++ b/internal/controller/certificates/policies/checks.go @@ -18,7 +18,6 @@ package policies import ( "bytes" - "crypto/tls" "crypto/x509" "fmt" "strings" @@ -62,26 +61,30 @@ func SecretIsMissingData(input Input) (string, string, bool) { } func SecretPublicKeysDiffer(input Input) (string, string, bool) { - pkData := input.Secret.Data[corev1.TLSPrivateKeyKey] - certData := input.Secret.Data[corev1.TLSCertKey] - // TODO: replace this with a generic decoder that can handle different - // formats such as JKS, P12 etc (i.e. add proper support for keystores) - _, err := tls.X509KeyPair(certData, pkData) + pk, err := pki.DecodePrivateKeyBytes(input.Secret.Data[corev1.TLSPrivateKeyKey]) if err != nil { - return InvalidKeyPair, fmt.Sprintf("Issuing certificate as Secret contains an invalid key-pair: %v", err), true + return InvalidKeyPair, fmt.Sprintf("Issuing certificate as Secret contains invalid private key data: %v", err), true + } + x509Cert, err := pki.DecodeX509CertificateBytes(input.Secret.Data[corev1.TLSCertKey]) + if err != nil { + return InvalidCertificate, fmt.Sprintf("Issuing certificate as Secret contains an invalid certificate: %v", err), true } - return "", "", false -} -func SecretPrivateKeyMatchesSpec(input Input) (string, string, bool) { - if input.Secret.Data == nil || len(input.Secret.Data[corev1.TLSPrivateKeyKey]) == 0 { - return SecretMismatch, "Existing issued Secret does not contain private key data", true + equal, err := pki.PublicKeysEqual(x509Cert.PublicKey, pk.Public()) + if err != nil { + return InvalidKeyPair, fmt.Sprintf("Secret contains an invalid key-pair: %v", err), true + } + if !equal { + return InvalidKeyPair, "Issuing certificate as Secret contains a private key that does not match the certificate", true } - pkBytes := input.Secret.Data[corev1.TLSPrivateKeyKey] - pk, err := pki.DecodePrivateKeyBytes(pkBytes) + return "", "", false +} + +func SecretPrivateKeyMismatchesSpec(input Input) (string, string, bool) { + pk, err := pki.DecodePrivateKeyBytes(input.Secret.Data[corev1.TLSPrivateKeyKey]) if err != nil { - return SecretMismatch, fmt.Sprintf("Existing issued Secret contains invalid private key data: %v", err), true + return InvalidKeyPair, fmt.Sprintf("Issuing certificate as Secret contains invalid private key data: %v", err), true } violations, err := pki.PrivateKeyMatchesSpec(pk, input.Certificate.Spec) @@ -94,13 +97,13 @@ func SecretPrivateKeyMatchesSpec(input Input) (string, string, bool) { return "", "", false } -// SecretKeystoreFormatMatchesSpec - When the keystore is not defined, the keystore +// SecretKeystoreFormatMismatch - When the keystore is not defined, the keystore // related fields are removed from the secret. // When one or more key stores are defined, the // corresponding secrets are generated. // If the private key rotation is set to "Never", the key store related values are re-encoded // as per the certificate specification -func SecretKeystoreFormatMatchesSpec(input Input) (string, string, bool) { +func SecretKeystoreFormatMismatch(input Input) (string, string, bool) { _, issuerProvidesCA := input.Secret.Data[cmmeta.TLSCAKey] if input.Certificate.Spec.Keystores == nil { @@ -154,14 +157,17 @@ func SecretKeystoreFormatMatchesSpec(input Input) (string, string, bool) { return "", "", false } -func SecretIssuerAnnotationsNotUpToDate(input Input) (string, string, bool) { - name := input.Secret.Annotations[cmapi.IssuerNameAnnotationKey] - kind := input.Secret.Annotations[cmapi.IssuerKindAnnotationKey] - group := input.Secret.Annotations[cmapi.IssuerGroupAnnotationKey] - if name != input.Certificate.Spec.IssuerRef.Name || +// SecretIssuerAnnotationsMismatch - When the issuer annotations are defined, +// it must match the issuer ref. +func SecretIssuerAnnotationsMismatch(input Input) (string, string, bool) { + name, ok1 := input.Secret.Annotations[cmapi.IssuerNameAnnotationKey] + kind, ok2 := input.Secret.Annotations[cmapi.IssuerKindAnnotationKey] + group, ok3 := input.Secret.Annotations[cmapi.IssuerGroupAnnotationKey] + if (ok1 || ok2 || ok3) && // only check if an annotation is present + name != input.Certificate.Spec.IssuerRef.Name || !issuerKindsEqual(kind, input.Certificate.Spec.IssuerRef.Kind) || !issuerGroupsEqual(group, input.Certificate.Spec.IssuerRef.Group) { - return IncorrectIssuer, fmt.Sprintf("Issuing certificate as Secret was previously issued by %s", formatIssuerRef(name, kind, group)), true + return IncorrectIssuer, fmt.Sprintf("Issuing certificate as Secret was previously issued by %q", formatIssuerRef(name, kind, group)), true } return "", "", false } @@ -195,7 +201,7 @@ func SecretPublicKeyDiffersFromCurrentCertificateRequest(input Input) (string, s return "", "", false } -func CurrentCertificateRequestNotValidForSpec(input Input) (string, string, bool) { +func CurrentCertificateRequestMismatchesSpec(input Input) (string, string, bool) { if input.CurrentRevisionRequest == nil { // Fallback to comparing the Certificate spec with the issued certificate. // This case is encountered if the CertificateRequest that issued the current @@ -232,7 +238,7 @@ func currentSecretValidForSpec(input Input) (string, string, bool) { } if len(violations) > 0 { - return SecretMismatch, fmt.Sprintf("Existing issued Secret is not up to date for spec: %v", violations), true + return SecretMismatch, fmt.Sprintf("Issuing certificate as Existing issued Secret is not up to date for spec: %v", violations), true } return "", "", false @@ -242,22 +248,19 @@ func currentSecretValidForSpec(input Input) (string, string, bool) { // check whether an X.509 cert currently issued for a Certificate should be // renewed. func CurrentCertificateNearingExpiry(c clock.Clock) Func { - return func(input Input) (string, string, bool) { + x509Cert, err := pki.DecodeX509CertificateBytes(input.Secret.Data[corev1.TLSCertKey]) + if err != nil { + return InvalidCertificate, fmt.Sprintf("Issuing certificate as Secret contains an invalid certificate: %v", err), true + } // Determine if the certificate is nearing expiry solely by looking at // the actual cert, if it exists. We assume that at this point we have // called policy functions that check that input.Secret and // input.Secret.Data exists (SecretDoesNotExist and SecretIsMissingData). - x509cert, err := pki.DecodeX509CertificateBytes(input.Secret.Data[corev1.TLSCertKey]) - if err != nil { - // This case should never happen as it should always be caught by the - // secretPublicKeysMatch function beforehand, but handle it just in case. - return InvalidCertificate, fmt.Sprintf("Failed to decode stored certificate: %v", err), true - } - notBefore := metav1.NewTime(x509cert.NotBefore) - notAfter := metav1.NewTime(x509cert.NotAfter) + notBefore := metav1.NewTime(x509Cert.NotBefore) + notAfter := metav1.NewTime(x509Cert.NotAfter) crt := input.Certificate renewalTime := pki.RenewalTime(notBefore.Time, notAfter.Time, crt.Spec.RenewBefore) @@ -275,21 +278,13 @@ func CurrentCertificateNearingExpiry(c clock.Clock) Func { // issued certificate has actually expired rather than just nearing expiry. func CurrentCertificateHasExpired(c clock.Clock) Func { return func(input Input) (string, string, bool) { - certData, ok := input.Secret.Data[corev1.TLSCertKey] - if !ok { - return MissingData, "Missing Certificate data", true - } - // TODO: replace this with a generic decoder that can handle different - // formats such as JKS, P12 etc (i.e. add proper support for keystores) - cert, err := pki.DecodeX509CertificateBytes(certData) + x509Cert, err := pki.DecodeX509CertificateBytes(input.Secret.Data[corev1.TLSCertKey]) if err != nil { - // This case should never happen as it should always be caught by the - // secretPublicKeysMatch function beforehand, but handle it just in case. - return InvalidCertificate, fmt.Sprintf("Failed to decode stored certificate: %v", err), true + return InvalidCertificate, fmt.Sprintf("Issuing certificate as Secret contains an invalid certificate: %v", err), true } - if c.Now().After(cert.NotAfter) { - return Expired, fmt.Sprintf("Certificate expired on %s", cert.NotAfter.Format(time.RFC1123)), true + if c.Now().After(x509Cert.NotAfter) { + return Expired, fmt.Sprintf("Certificate expired on %s", x509Cert.NotAfter.Format(time.RFC1123)), true } return "", "", false } @@ -328,14 +323,14 @@ func issuerGroupsEqual(l, r string) bool { return l == r } -// SecretTemplateMismatchesSecret will inspect the given Secret's Annotations +// SecretSecretTemplateMismatch will inspect the given Secret's Annotations // and Labels, and compare these maps against those that appear on the given // Certificate's SecretTemplate. // Returns false if all the Certificate's SecretTemplate Annotations and Labels // appear on the Secret, or put another way, the Certificate's SecretTemplate // is a subset of that in the Secret's Annotations/Labels. // Returns true otherwise. -func SecretTemplateMismatchesSecret(input Input) (string, string, bool) { +func SecretSecretTemplateMismatch(input Input) (string, string, bool) { if input.Certificate.Spec.SecretTemplate == nil { return "", "", false } @@ -492,14 +487,14 @@ func SecretManagedLabelsAndAnnotationsManagedFieldsMismatch(fieldManager string) } } -// SecretTemplateMismatchesSecretManagedFields will inspect the given Secret's +// SecretSecretTemplateManagedFieldsMismatch will inspect the given Secret's // managed fields for its Annotations and Labels, and compare this against the // SecretTemplate on the given Certificate. Returns false if Annotations and // Labels match on both the Certificate's SecretTemplate and the Secret's // managed fields, true otherwise. // Also returns true if the managed fields or signed certificate were not able // to be decoded. -func SecretTemplateMismatchesSecretManagedFields(fieldManager string) Func { +func SecretSecretTemplateManagedFieldsMismatch(fieldManager string) Func { return func(input Input) (string, string, bool) { managedLabels, managedAnnotations, err := secretLabelsAndAnnotationsManagedFields(input.Secret, fieldManager) if err != nil { @@ -601,13 +596,13 @@ func SecretCertificateDetailsAnnotationsMismatch(input Input) (string, string, b return "", "", false } -// SecretAdditionalOutputFormatsDataMismatch validates that the Secret has the +// SecretAdditionalOutputFormatsMismatch validates that the Secret has the // expected Certificate AdditionalOutputFormats. // Returns true (violation) if AdditionalOutputFormat(s) are present and any of // the following: // - Secret key is missing // - Secret value is incorrect -func SecretAdditionalOutputFormatsDataMismatch(input Input) (string, string, bool) { +func SecretAdditionalOutputFormatsMismatch(input Input) (string, string, bool) { const message = "Certificate's AdditionalOutputFormats doesn't match Secret Data" for _, format := range input.Certificate.Spec.AdditionalOutputFormats { switch format.Type { @@ -631,7 +626,7 @@ func SecretAdditionalOutputFormatsDataMismatch(input Input) (string, string, boo return "", "", false } -// SecretAdditionalOutputFormatsOwnerMismatch validates that the field manager +// SecretAdditionalOutputFormatsManagedFieldsMismatch validates that the field manager // owns the correct Certificate's AdditionalOutputFormats in the Secret. // Returns true (violation) if: // - missing AdditionalOutputFormat key owned by the field manager @@ -639,7 +634,7 @@ func SecretAdditionalOutputFormatsDataMismatch(input Input) (string, string, boo // // A violation with the reason `ManagedFieldsParseError` should be considered a // non re-triable error. -func SecretAdditionalOutputFormatsOwnerMismatch(fieldManager string) Func { +func SecretAdditionalOutputFormatsManagedFieldsMismatch(fieldManager string) Func { const message = "Certificate's AdditionalOutputFormats doesn't match Secret ManagedFields" return func(input Input) (string, string, bool) { var ( @@ -736,10 +731,10 @@ func SecretOwnerReferenceManagedFieldMismatch(ownerRefEnabled bool, fieldManager } } -// SecretOwnerReferenceValueMismatch validates that the Secret has the expected +// SecretOwnerReferenceMismatch validates that the Secret has the expected // owner reference if it is enabled. Returns true (violation) if: // * owner reference is enabled, but the reference has an incorrect value -func SecretOwnerReferenceValueMismatch(ownerRefEnabled bool) Func { +func SecretOwnerReferenceMismatch(ownerRefEnabled bool) Func { return func(input Input) (string, string, bool) { // If the Owner Reference is not enabled, we don't need to check the value // and can exit early. diff --git a/internal/controller/certificates/policies/checks_test.go b/internal/controller/certificates/policies/checks_test.go index 0111239568f..eaafb88e314 100644 --- a/internal/controller/certificates/policies/checks_test.go +++ b/internal/controller/certificates/policies/checks_test.go @@ -29,6 +29,7 @@ import ( cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" + "github.com/cert-manager/cert-manager/pkg/util/pki" testcrypto "github.com/cert-manager/cert-manager/test/unit/crypto" "github.com/cert-manager/cert-manager/test/unit/gen" "github.com/stretchr/testify/assert" @@ -92,7 +93,7 @@ func Test_NewTriggerPolicyChain(t *testing.T) { }, }, reason: InvalidKeyPair, - message: "Issuing certificate as Secret contains an invalid key-pair: tls: failed to find any PEM data in certificate input", + message: "Issuing certificate as Secret contains invalid private key data: error decoding private key PEM block", reissue: true, }, "trigger issuance as Secret contains corrupt certificate data": { @@ -103,8 +104,8 @@ func Test_NewTriggerPolicyChain(t *testing.T) { corev1.TLSCertKey: []byte("test"), }, }, - reason: InvalidKeyPair, - message: "Issuing certificate as Secret contains an invalid key-pair: tls: failed to find any PEM data in certificate input", + reason: InvalidCertificate, + message: "Issuing certificate as Secret contains an invalid certificate: error decoding certificate PEM block", reissue: true, }, "trigger issuance as Secret contains corrupt private key data": { @@ -118,7 +119,7 @@ func Test_NewTriggerPolicyChain(t *testing.T) { }, }, reason: InvalidKeyPair, - message: "Issuing certificate as Secret contains an invalid key-pair: tls: failed to find any PEM data in key input", + message: "Issuing certificate as Secret contains invalid private key data: error decoding private key PEM block", reissue: true, }, "trigger issuance as Secret contains a non-matching key-pair": { @@ -132,10 +133,10 @@ func Test_NewTriggerPolicyChain(t *testing.T) { }, }, reason: InvalidKeyPair, - message: "Issuing certificate as Secret contains an invalid key-pair: tls: private key does not match public key", + message: "Issuing certificate as Secret contains a private key that does not match the certificate", reissue: true, }, - "trigger issuance as Secret has old/incorrect 'issuer name' annotation": { + "trigger issuance as Secret has old or incorrect 'issuer name' annotation": { certificate: &cmapi.Certificate{Spec: cmapi.CertificateSpec{ SecretName: "something", IssuerRef: cmmeta.ObjectReference{ @@ -156,10 +157,10 @@ func Test_NewTriggerPolicyChain(t *testing.T) { }, }, reason: IncorrectIssuer, - message: "Issuing certificate as Secret was previously issued by Issuer.cert-manager.io/oldissuer", + message: "Issuing certificate as Secret was previously issued by \"Issuer.cert-manager.io/oldissuer\"", reissue: true, }, - "trigger issuance as Secret has old/incorrect 'issuer kind' annotation": { + "trigger issuance as Secret has old or incorrect 'issuer kind' annotation": { certificate: &cmapi.Certificate{Spec: cmapi.CertificateSpec{ SecretName: "something", IssuerRef: cmmeta.ObjectReference{ @@ -182,10 +183,10 @@ func Test_NewTriggerPolicyChain(t *testing.T) { }, }, reason: IncorrectIssuer, - message: "Issuing certificate as Secret was previously issued by OldIssuerKind.cert-manager.io/testissuer", + message: "Issuing certificate as Secret was previously issued by \"OldIssuerKind.cert-manager.io/testissuer\"", reissue: true, }, - "trigger issuance as Secret has old/incorrect 'issuer group' annotation": { + "trigger issuance as Secret has old or incorrect 'issuer group' annotation": { certificate: &cmapi.Certificate{Spec: cmapi.CertificateSpec{ SecretName: "something", IssuerRef: cmmeta.ObjectReference{ @@ -210,7 +211,36 @@ func Test_NewTriggerPolicyChain(t *testing.T) { }, }, reason: IncorrectIssuer, - message: "Issuing certificate as Secret was previously issued by IssuerKind.new.example.com/testissuer", + message: "Issuing certificate as Secret was previously issued by \"IssuerKind.new.example.com/testissuer\"", + reissue: true, + }, + "trigger issuance as private key properties do not meet the requested properties": { + certificate: &cmapi.Certificate{Spec: cmapi.CertificateSpec{SecretName: "something"}}, + secret: &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "something"}, + Data: func() map[string][]byte { + // generate a 521 bit EC private key, which is not the type of key + // configured in the Certificate resource + pk, err := pki.GenerateECPrivateKey(521) + if err != nil { + t.Fatal(err) + } + + pkData, err := pki.EncodePrivateKey(pk, cmapi.PKCS8) + if err != nil { + t.Fatal(err) + } + + return map[string][]byte{ + corev1.TLSPrivateKeyKey: pkData, + corev1.TLSCertKey: testcrypto.MustCreateCert( + t, pkData, + &cmapi.Certificate{Spec: cmapi.CertificateSpec{CommonName: "example.com"}}, + ), + } + }(), + }, + reason: SecretMismatch, + message: "Existing private key is not up to date for spec: [spec.privateKey.algorithm]", reissue: true, }, "trigger if the Secret contains a different private key than was used to sign the CSR": { @@ -344,7 +374,7 @@ func Test_NewTriggerPolicyChain(t *testing.T) { }, }, reason: SecretMismatch, - message: "Existing issued Secret is not up to date for spec: [spec.commonName]", + message: "Issuing certificate as Existing issued Secret is not up to date for spec: [spec.commonName]", reissue: true, }, "do nothing if signed x509 certificate in Secret matches spec (when request does not exist)": { @@ -734,7 +764,7 @@ func Test_SecretSecretTemplateMismatch(t *testing.T) { expReason: SecretTemplateMismatch, expMessage: "Certificate's SecretTemplate Annotations missing or incorrect value on Secret", }, - "if SecretTemplate is non-nil, Secret Annoations match but Labels don't match keys, return true": { + "if SecretTemplate is non-nil, Secret Annotations match but Labels don't match keys, return true": { tmpl: &cmapi.CertificateSecretTemplate{ Annotations: map[string]string{"foo1": "bar1", "foo2": "bar2"}, Labels: map[string]string{"abc": "123", "def": "456"}, @@ -790,7 +820,7 @@ func Test_SecretSecretTemplateMismatch(t *testing.T) { for name, test := range tests { t.Run(name, func(t *testing.T) { - gotReason, gotMessage, gotViolation := SecretTemplateMismatchesSecret(Input{ + gotReason, gotMessage, gotViolation := SecretSecretTemplateMismatch(Input{ Certificate: &cmapi.Certificate{Spec: cmapi.CertificateSpec{SecretTemplate: test.tmpl}}, Secret: test.secret, }) @@ -1103,7 +1133,7 @@ func Test_SecretSecretTemplateManagedFieldsMismatch(t *testing.T) { for name, test := range tests { t.Run(name, func(t *testing.T) { - gotReason, gotMessage, gotViolation := SecretTemplateMismatchesSecretManagedFields(fieldManager)(Input{ + gotReason, gotMessage, gotViolation := SecretSecretTemplateManagedFieldsMismatch(fieldManager)(Input{ Certificate: &cmapi.Certificate{Spec: cmapi.CertificateSpec{SecretTemplate: test.tmpl}}, Secret: &corev1.Secret{ObjectMeta: metav1.ObjectMeta{ManagedFields: test.secretManagedFields}, Data: map[string][]byte{}}, }) @@ -1115,7 +1145,7 @@ func Test_SecretSecretTemplateManagedFieldsMismatch(t *testing.T) { } } -func Test_SecretAdditionalOutputFormatsDataMismatch(t *testing.T) { +func Test_SecretAdditionalOutputFormatsMismatch(t *testing.T) { cert := []byte("a") pk := testcrypto.MustCreatePEMPrivateKey(t) block, _ := pem.Decode(pk) @@ -1372,7 +1402,7 @@ func Test_SecretAdditionalOutputFormatsDataMismatch(t *testing.T) { for name, test := range tests { t.Run(name, func(t *testing.T) { - gotReason, gotMessage, gotViolation := SecretAdditionalOutputFormatsDataMismatch(test.input) + gotReason, gotMessage, gotViolation := SecretAdditionalOutputFormatsMismatch(test.input) assert.Equal(t, test.expReason, gotReason) assert.Equal(t, test.expMessage, gotMessage) assert.Equal(t, test.expViolation, gotViolation) @@ -1380,7 +1410,7 @@ func Test_SecretAdditionalOutputFormatsDataMismatch(t *testing.T) { } } -func Test_SecretAdditionalOutputFormatsOwnerMismatch(t *testing.T) { +func Test_SecretAdditionalOutputFormatsManagedFieldsMismatch(t *testing.T) { const fieldManager = "cert-manager-test" tests := map[string]struct { @@ -1804,7 +1834,7 @@ func Test_SecretAdditionalOutputFormatsOwnerMismatch(t *testing.T) { for name, test := range tests { t.Run(name, func(t *testing.T) { - gotReason, gotMessage, gotViolation := SecretAdditionalOutputFormatsOwnerMismatch(fieldManager)(test.input) + gotReason, gotMessage, gotViolation := SecretAdditionalOutputFormatsManagedFieldsMismatch(fieldManager)(test.input) assert.Equal(t, test.expReason, gotReason) assert.Equal(t, test.expMessage, gotMessage) assert.Equal(t, test.expViolation, gotViolation) @@ -1992,7 +2022,7 @@ func Test_SecretOwnerReferenceManagedFieldMismatch(t *testing.T) { } } -func Test_SecretOwnerReferenceValueMismatch(t *testing.T) { +func Test_SecretOwnerReferenceMismatch(t *testing.T) { crt := gen.Certificate("test-certificate", gen.SetCertificateUID(types.UID("uid-123")), ) @@ -2240,7 +2270,7 @@ func Test_SecretOwnerReferenceValueMismatch(t *testing.T) { for name, test := range tests { t.Run(name, func(t *testing.T) { - gotReason, gotMessage, gotViolation := SecretOwnerReferenceValueMismatch(test.ownerRefEnabled)(test.input) + gotReason, gotMessage, gotViolation := SecretOwnerReferenceMismatch(test.ownerRefEnabled)(test.input) assert.Equal(t, test.expReason, gotReason) assert.Equal(t, test.expMessage, gotMessage) assert.Equal(t, test.expViolation, gotViolation) diff --git a/internal/controller/certificates/policies/policies.go b/internal/controller/certificates/policies/policies.go index 7c379b44ef1..15caa75b749 100644 --- a/internal/controller/certificates/policies/policies.go +++ b/internal/controller/certificates/policies/policies.go @@ -67,14 +67,16 @@ func (c Chain) Evaluate(input Input) (string, string, bool) { // should cause a Certificate to be marked for issuance. func NewTriggerPolicyChain(c clock.Clock) Chain { return Chain{ - SecretDoesNotExist, - SecretIsMissingData, - SecretPublicKeysDiffer, - SecretPrivateKeyMatchesSpec, - SecretIssuerAnnotationsNotUpToDate, - SecretPublicKeyDiffersFromCurrentCertificateRequest, - CurrentCertificateRequestNotValidForSpec, - CurrentCertificateNearingExpiry(c), + SecretDoesNotExist, // Make sure the Secret exists + SecretIsMissingData, // Make sure the Secret has the required keys set + SecretPublicKeysDiffer, // Make sure the PrivateKey and PublicKey match in the Secret + + SecretIssuerAnnotationsMismatch, // Make sure the Secret's IssuerRef annotations match the Certificate spec + + SecretPrivateKeyMismatchesSpec, // Make sure the PrivateKey Type and Size match the Certificate spec + SecretPublicKeyDiffersFromCurrentCertificateRequest, // Make sure the Secret's PublicKey matches the current CertificateRequest + CurrentCertificateRequestMismatchesSpec, // Make sure the current CertificateRequest matches the Certificate spec + CurrentCertificateNearingExpiry(c), // Make sure the Certificate in the Secret is not nearing expiry } } @@ -82,12 +84,16 @@ func NewTriggerPolicyChain(c clock.Clock) Chain { // true, would cause a Certificate to be marked as not ready. func NewReadinessPolicyChain(c clock.Clock) Chain { return Chain{ - SecretDoesNotExist, - SecretIsMissingData, - SecretPublicKeysDiffer, - SecretPublicKeyDiffersFromCurrentCertificateRequest, - CurrentCertificateRequestNotValidForSpec, - CurrentCertificateHasExpired(c), + SecretDoesNotExist, // Make sure the Secret exists + SecretIsMissingData, // Make sure the Secret has the required keys set + SecretPublicKeysDiffer, // Make sure the PrivateKey and PublicKey match in the Secret + + SecretIssuerAnnotationsMismatch, // Make sure the Secret's IssuerRef annotations match the Certificate spec + + SecretPrivateKeyMismatchesSpec, // Make sure the PrivateKey Type and Size match the Certificate spec + SecretPublicKeyDiffersFromCurrentCertificateRequest, // Make sure the Secret's PublicKey matches the current CertificateRequest + CurrentCertificateRequestMismatchesSpec, // Make sure the current CertificateRequest matches the Certificate spec + CurrentCertificateHasExpired(c), // Make sure the Certificate in the Secret has not expired } } @@ -99,13 +105,14 @@ func NewSecretPostIssuancePolicyChain(ownerRefEnabled bool, fieldManager string) SecretBaseLabelsMismatch, // Make sure the managed labels have the correct values SecretCertificateDetailsAnnotationsMismatch, // Make sure the managed certificate details annotations have the correct values SecretManagedLabelsAndAnnotationsManagedFieldsMismatch(fieldManager), // Make sure the only the expected managed labels and annotations exist - SecretTemplateMismatchesSecret, // Make sure the template label and annotation values match the secret - SecretTemplateMismatchesSecretManagedFields(fieldManager), // Make sure the only the expected template labels and annotations exist - SecretAdditionalOutputFormatsDataMismatch, - SecretAdditionalOutputFormatsOwnerMismatch(fieldManager), + SecretSecretTemplateMismatch, // Make sure the template label and annotation values match the secret + SecretSecretTemplateManagedFieldsMismatch(fieldManager), // Make sure the only the expected template labels and annotations exist + SecretAdditionalOutputFormatsMismatch, + SecretAdditionalOutputFormatsManagedFieldsMismatch(fieldManager), + SecretOwnerReferenceMismatch(ownerRefEnabled), SecretOwnerReferenceManagedFieldMismatch(ownerRefEnabled, fieldManager), - SecretOwnerReferenceValueMismatch(ownerRefEnabled), - SecretKeystoreFormatMatchesSpec, + + SecretKeystoreFormatMismatch, } } @@ -113,8 +120,8 @@ func NewSecretPostIssuancePolicyChain(ownerRefEnabled bool, fieldManager string) // temporary certificate is valid. func NewTemporaryCertificatePolicyChain() Chain { return Chain{ - SecretDoesNotExist, - SecretIsMissingData, - SecretPublicKeysDiffer, + SecretDoesNotExist, // Make sure the Secret exists + SecretIsMissingData, // Make sure the Secret has the required keys set + SecretPublicKeysDiffer, // Make sure the PrivateKey and PublicKey match in the Secret } } diff --git a/pkg/controller/certificates/issuing/internal/secret.go b/pkg/controller/certificates/issuing/internal/secret.go index d2047b4f58d..fa6c5923264 100644 --- a/pkg/controller/certificates/issuing/internal/secret.go +++ b/pkg/controller/certificates/issuing/internal/secret.go @@ -188,10 +188,17 @@ func (s *SecretsManager) setValues(crt *cmapi.Certificate, secret *corev1.Secret secret.Annotations[k] = v } - secret.Annotations[cmapi.CertificateNameKey] = data.CertificateName - secret.Annotations[cmapi.IssuerNameAnnotationKey] = data.IssuerName - secret.Annotations[cmapi.IssuerKindAnnotationKey] = data.IssuerKind - secret.Annotations[cmapi.IssuerGroupAnnotationKey] = data.IssuerGroup + // Add the certificate name and issuer details to the secret annotations. + // If the annotations are not set/ empty, we do not use them to determine + // if the secret needs to be updated. + if data.CertificateName != "" { + secret.Annotations[cmapi.CertificateNameKey] = data.CertificateName + } + if data.IssuerName != "" || data.IssuerKind != "" || data.IssuerGroup != "" { + secret.Annotations[cmapi.IssuerNameAnnotationKey] = data.IssuerName + secret.Annotations[cmapi.IssuerKindAnnotationKey] = data.IssuerKind + secret.Annotations[cmapi.IssuerGroupAnnotationKey] = data.IssuerGroup + } secret.Labels[cmapi.PartOfCertManagerControllerLabelKey] = "true" diff --git a/pkg/controller/certificates/readiness/readiness_controller_test.go b/pkg/controller/certificates/readiness/readiness_controller_test.go index 1b44908ca0f..9df9b353f4c 100644 --- a/pkg/controller/certificates/readiness/readiness_controller_test.go +++ b/pkg/controller/certificates/readiness/readiness_controller_test.go @@ -375,7 +375,7 @@ func TestNewReadinessPolicyChain(t *testing.T) { corev1.TLSCertKey: []byte("test"), })), reason: policies.InvalidKeyPair, - message: "Issuing certificate as Secret contains an invalid key-pair: tls: failed to find any PEM data in certificate input", + message: "Issuing certificate as Secret contains invalid private key data: error decoding private key PEM block", violationFound: true, }, "Certificate not Ready as Secret contains corrupt certificate data": { @@ -385,8 +385,8 @@ func TestNewReadinessPolicyChain(t *testing.T) { corev1.TLSPrivateKeyKey: privKey, corev1.TLSCertKey: []byte("test"), })), - reason: policies.InvalidKeyPair, - message: "Issuing certificate as Secret contains an invalid key-pair: tls: failed to find any PEM data in certificate input", + reason: policies.InvalidCertificate, + message: "Issuing certificate as Secret contains an invalid certificate: error decoding certificate PEM block", violationFound: true, }, "Certificate not Ready as Secret contains a non-matching key-pair": { @@ -399,7 +399,7 @@ func TestNewReadinessPolicyChain(t *testing.T) { gen.Certificate("something else", gen.SetCertificateCommonName("example.com"))), })), reason: policies.InvalidKeyPair, - message: "Issuing certificate as Secret contains an invalid key-pair: tls: private key does not match public key", + message: "Issuing certificate as Secret contains a private key that does not match the certificate", violationFound: true, }, "Certificate not Ready when CertificateRequest does not match certificate spec": {