From bdc4b60c5f29b8439b6de7591e5e99aa584ff62a Mon Sep 17 00:00:00 2001 From: Tim Ramlot <42113979+inteon@users.noreply.github.com> Date: Wed, 14 Jun 2023 20:54:31 +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 | 437 ++++++++++++------ .../certificates/policies/checks_test.go | 329 +++++++++---- .../certificates/policies/constants.go | 13 +- .../certificates/policies/policies.go | 57 ++- .../certificates/issuing/internal/secret.go | 15 +- .../issuing/secret_manager_test.go | 241 ++++++++-- .../readiness/readiness_controller_test.go | 39 +- 7 files changed, 818 insertions(+), 313 deletions(-) diff --git a/internal/controller/certificates/policies/checks.go b/internal/controller/certificates/policies/checks.go index 9e6f1224a97..f4da8f6d9e1 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,15 @@ 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) { +// TODO: support other fieldmanagers to own these Secret fields without +// returning invalidated=true +func SecretKeystoreFormatMismatch(input Input) (string, string, bool) { _, issuerProvidesCA := input.Secret.Data[cmmeta.TLSCAKey] if input.Certificate.Spec.Keystores == nil { @@ -154,19 +159,58 @@ 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 || +// SecretCertificateNameAnnotationMismatch - When the certificate name annotation is +// defined, it must match the certificate name. +func SecretCertificateNameAnnotationMismatch(input Input) (string, string, bool) { + name, ok := input.Secret.Annotations[cmapi.CertificateNameKey] + if ok && // only check if annotation is present + name != input.Certificate.Name { + return IncorrectCertificate, fmt.Sprintf("Issuing certificate as Secret was previously issued for Certificate %q", name), true + } + return "", "", false +} + +// 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 +} + +func SecretPublicKeysDiffersFromCurrentCertificateRequest(input Input) (string, string, bool) { + if input.CurrentRevisionRequest == nil { + return "", "", false + } + pk, err := pki.DecodePrivateKeyBytes(input.Secret.Data[corev1.TLSPrivateKeyKey]) + if err != nil { + return InvalidKeyPair, fmt.Sprintf("Issuing certificate as Secret contains invalid private key data: %v", err), true + } + + csr, err := pki.DecodeX509CertificateRequestBytes(input.CurrentRevisionRequest.Spec.Request) + if err != nil { + return InvalidCertificateRequest, fmt.Sprintf("Failed to decode current CertificateRequest: %v", err), true + } + + equal, err := pki.PublicKeysEqual(csr.PublicKey, pk.Public()) + if err != nil { + return InvalidCertificateRequest, fmt.Sprintf("CertificateRequest's public key is invalid: %v", err), true + } + if !equal { + return InvalidCertificateRequest, "Secret contains a private key that does not match the current CertificateRequest", true } + 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 @@ -203,7 +247,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 @@ -213,22 +257,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) @@ -246,21 +287,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 } @@ -299,14 +332,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 } @@ -326,15 +359,108 @@ func SecretTemplateMismatchesSecret(input Input) (string, string, bool) { return "", "", false } -// SecretTemplateMismatchesSecretManagedFields will inspect the given Secret's +func secretLabelsAndAnnotationsManagedFields(secret *corev1.Secret, fieldManager string) (labels, annotations sets.Set[string], err error) { + managedLabels, managedAnnotations := sets.New[string](), sets.New[string]() + + for _, managedField := range secret.ManagedFields { + // If the managed field isn't owned by the cert-manager controller, ignore. + if managedField.Manager != fieldManager || managedField.FieldsV1 == nil { + continue + } + + // Decode the managed field. + var fieldset fieldpath.Set + if err := fieldset.FromJSON(bytes.NewReader(managedField.FieldsV1.Raw)); err != nil { + return nil, nil, err + } + + // Extract the labels and annotations of the managed fields. + metadata := fieldset.Children.Descend(fieldpath.PathElement{ + FieldName: pointer.String("metadata"), + }) + labels := metadata.Children.Descend(fieldpath.PathElement{ + FieldName: pointer.String("labels"), + }) + annotations := metadata.Children.Descend(fieldpath.PathElement{ + FieldName: pointer.String("annotations"), + }) + + // Gather the annotations and labels on the managed fields. Remove the '.' + // prefix which appears on managed field keys. + labels.Iterate(func(path fieldpath.Path) { + managedLabels.Insert(strings.TrimPrefix(path.String(), ".")) + }) + annotations.Iterate(func(path fieldpath.Path) { + managedAnnotations.Insert(strings.TrimPrefix(path.String(), ".")) + }) + } + + return managedLabels, managedAnnotations, nil +} + +func certificateDataAnnotationsForSecret(secret *corev1.Secret) (annotations map[string]string, err error) { + var certificate *x509.Certificate + if len(secret.Data[corev1.TLSCertKey]) > 0 { + certificate, err = pki.DecodeX509CertificateBytes(secret.Data[corev1.TLSCertKey]) + if err != nil { + return nil, err + } + } + + certificateAnnotations, err := internalcertificates.AnnotationsForCertificate(certificate) + if err != nil { + return nil, err + } + + return certificateAnnotations, nil +} + +// SecretManagedLabelsAndAnnotationsManagedFieldsMismatch 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 and Annotations that are managed by cert-manager. 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 SecretManagedLabelsAndAnnotationsManagedFieldsMismatch(fieldManager string) Func { return func(input Input) (string, string, bool) { + managedLabels, managedAnnotations, err := secretLabelsAndAnnotationsManagedFields(input.Secret, fieldManager) + if err != nil { + return ManagedFieldsParseError, fmt.Sprintf("failed to decode managed fields on Secret: %s", err), true + } + + // Remove the non cert-manager annotations from the managed Annotations so we can compare + // 1 to 1 all the cert-manager annotations. + for k := range managedAnnotations { + if strings.HasPrefix(k, "cert-manager.io/") || + strings.HasPrefix(k, "controller.cert-manager.io/") { + continue + } + + delete(managedAnnotations, k) + } + + // Ignore the CertificateName and IssuerRef annotations as these cannot be set by the postIssuance controller. + for _, k := range []string{ + cmapi.CertificateNameKey, // SecretCertificateNameAnnotationMismatch checks the value + cmapi.IssuerNameAnnotationKey, // SecretIssuerAnnotationsMismatch checks the value + cmapi.IssuerKindAnnotationKey, // SecretIssuerAnnotationsMismatch checks the value + cmapi.IssuerGroupAnnotationKey, // SecretIssuerAnnotationsMismatch checks the value + } { + delete(managedAnnotations, k) + } + + // Remove the non cert-manager labels from the managed Annotations so we can compare + // 1 to 1 all the cert-manager labels. + for k := range managedLabels { + if strings.HasPrefix(k, "cert-manager.io/") || + strings.HasPrefix(k, "controller.cert-manager.io/") { + continue + } + + delete(managedLabels, k) + } + // Only attempt to decode the signed certificate, if one is available. var x509cert *x509.Certificate if len(input.Secret.Data[corev1.TLSCertKey]) > 0 { @@ -347,134 +473,159 @@ func SecretTemplateMismatchesSecretManagedFields(fieldManager string) Func { } } - baseAnnotations, err := internalcertificates.AnnotationsForCertificate(x509cert) + expCertificateDataAnnotations, err := internalcertificates.AnnotationsForCertificate(x509cert) if err != nil { return InvalidCertificate, fmt.Sprintf("Failed getting secret annotations: %v", err), true } - // We don't use the values of these annotations, but we need to make sure - // that the keys are present in the map so that we can compare the sets. - baseAnnotations[cmapi.CertificateNameKey] = "" - baseAnnotations[cmapi.IssuerNameAnnotationKey] = "" - baseAnnotations[cmapi.IssuerKindAnnotationKey] = "" - baseAnnotations[cmapi.IssuerGroupAnnotationKey] = "" - - managedLabels, managedAnnotations := sets.NewString(), sets.NewString() + expLabels := sets.New[string]( + cmapi.PartOfCertManagerControllerLabelKey, // SecretBaseLabelsMismatch checks the value + ) + expAnnotations := sets.New[string]() + for k := range expCertificateDataAnnotations { // SecretCertificateDetailsAnnotationsMismatch checks the value + expAnnotations.Insert(k) + } - for _, managedField := range input.Secret.ManagedFields { - // If the managed field isn't owned by the cert-manager controller, ignore. - if managedField.Manager != fieldManager || managedField.FieldsV1 == nil { - continue + if !managedLabels.Equal(expLabels) { + missingLabels := expLabels.Difference(managedLabels) + if len(missingLabels) > 0 { + return SecretManagedMetadataMismatch, fmt.Sprintf("Secret is missing these Managed Labels: %v", missingLabels.UnsortedList()), true } - // Decode the managed field. - var fieldset fieldpath.Set - if err := fieldset.FromJSON(bytes.NewReader(managedField.FieldsV1.Raw)); err != nil { - return ManagedFieldsParseError, fmt.Sprintf("failed to decode managed fields on Secret: %s", err), true + extraLabels := managedLabels.Difference(expLabels) + return SecretManagedMetadataMismatch, fmt.Sprintf("Secret has these extra Labels: %v", extraLabels.UnsortedList()), true + } + + if !managedAnnotations.Equal(expAnnotations) { + missingAnnotations := expAnnotations.Difference(managedAnnotations) + if len(missingAnnotations) > 0 { + return SecretManagedMetadataMismatch, fmt.Sprintf("Secret is missing these Managed Annotations: %v", missingAnnotations.UnsortedList()), true } - // Extract the labels and annotations of the managed fields. - metadata := fieldset.Children.Descend(fieldpath.PathElement{ - FieldName: pointer.String("metadata"), - }) - labels := metadata.Children.Descend(fieldpath.PathElement{ - FieldName: pointer.String("labels"), - }) - annotations := metadata.Children.Descend(fieldpath.PathElement{ - FieldName: pointer.String("annotations"), - }) - - // Gather the annotations and labels on the managed fields. Remove the '.' - // prefix which appears on managed field keys. - labels.Iterate(func(path fieldpath.Path) { - managedLabels.Insert(strings.TrimPrefix(path.String(), ".")) - }) - annotations.Iterate(func(path fieldpath.Path) { - managedAnnotations.Insert(strings.TrimPrefix(path.String(), ".")) - }) - } - - // Remove the base Annotations from the managed Annotations so we can compare - // 1 to 1 against the SecretTemplate. - for k := range baseAnnotations { - managedAnnotations = managedAnnotations.Delete(k) + extraAnnotations := managedAnnotations.Difference(expAnnotations) + return SecretManagedMetadataMismatch, fmt.Sprintf("Secret has these extra Annotations: %v", extraAnnotations.UnsortedList()), true } - // Remove the base label from the managed Labels so we can - // compare 1 to 1 against the SecretTemplate - managedLabels.Delete(cmapi.PartOfCertManagerControllerLabelKey) + return "", "", false + } +} + +// 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 SecretSecretTemplateManagedFieldsMismatch(fieldManager string) Func { + return func(input Input) (string, string, bool) { + managedLabels, managedAnnotations, err := secretLabelsAndAnnotationsManagedFields(input.Secret, fieldManager) + if err != nil { + return ManagedFieldsParseError, fmt.Sprintf("failed to decode managed fields on Secret: %s", err), true + } - // Check early for Secret Template being nil, and whether managed - // labels/annotations are not. - if input.Certificate.Spec.SecretTemplate == nil { - if len(managedLabels) > 0 || len(managedAnnotations) > 0 { - return SecretTemplateMismatch, "SecretTemplate is nil, but Secret contains extra managed entries", true + // Remove the cert-manager annotations from the managed Annotations so we can compare + // 1 to 1 against the SecretTemplate. + for k := range managedAnnotations { + if !strings.HasPrefix(k, "cert-manager.io/") && + !strings.HasPrefix(k, "controller.cert-manager.io/") { + continue } - // SecretTemplate is nil. Managed annotations and labels are also empty. - // Return false. - return "", "", false + + delete(managedAnnotations, k) } - // SecretTemplate is not nil. Do length checks. - if len(input.Certificate.Spec.SecretTemplate.Labels) != len(managedLabels) || - len(input.Certificate.Spec.SecretTemplate.Annotations) != len(managedAnnotations) { - return SecretTemplateMismatch, "Certificate's SecretTemplate doesn't match Secret", true + // Remove the cert-manager labels from the managed Labels so we can + // compare 1 to 1 against the SecretTemplate + for k := range managedLabels { + if !strings.HasPrefix(k, "cert-manager.io/") && + !strings.HasPrefix(k, "controller.cert-manager.io/") { + continue + } + + delete(managedLabels, k) } - // Check equal unsorted for SecretTemplate keys, and the managed fields - // equivalents. - for _, smap := range []struct { - specMap map[string]string - managedSet sets.String - }{ - {specMap: input.Certificate.Spec.SecretTemplate.Labels, managedSet: managedLabels}, - {specMap: input.Certificate.Spec.SecretTemplate.Annotations, managedSet: managedAnnotations}, - } { + expLabels := sets.New[string]() + expAnnotations := sets.New[string]() + if input.Certificate.Spec.SecretTemplate != nil { + for k := range input.Certificate.Spec.SecretTemplate.Labels { + expLabels.Insert(k) + } + for k := range input.Certificate.Spec.SecretTemplate.Annotations { + expAnnotations.Insert(k) + } + } - specSet := sets.NewString() - for kSpec := range smap.specMap { - specSet.Insert(kSpec) + if !managedLabels.Equal(expLabels) { + missingLabels := expLabels.Difference(managedLabels) + if len(missingLabels) > 0 { + return SecretTemplateMismatch, fmt.Sprintf("Secret is missing these Template Labels: %v", missingLabels.UnsortedList()), true } - if !specSet.Equal(smap.managedSet) { - return SecretTemplateMismatch, "Certificate's SecretTemplate doesn't match Secret", true + extraLabels := managedLabels.Difference(expLabels) + return SecretTemplateMismatch, fmt.Sprintf("Secret has these extra Labels: %v", extraLabels.UnsortedList()), true + } + + if !managedAnnotations.Equal(expAnnotations) { + missingAnnotations := expAnnotations.Difference(managedAnnotations) + if len(missingAnnotations) > 0 { + return SecretTemplateMismatch, fmt.Sprintf("Secret is missing these Template Annotations: %v", missingAnnotations.UnsortedList()), true } + + extraAnnotations := managedAnnotations.Difference(expAnnotations) + return SecretTemplateMismatch, fmt.Sprintf("Secret has these extra Annotations: %v", extraAnnotations.UnsortedList()), true } return "", "", false } } -func SecretBaseLabelsAreMissing(input Input) (string, string, bool) { - // If certificate has not been issued yet or is in invalid state, do not attempt to update metadata - if len(input.Secret.Data[corev1.TLSCertKey]) > 0 { - var err error - _, 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 - } - } - +// NOTE: The presence of the controller.cert-manager.io/fao label is checked +// by the SecretManagedLabelsAndAnnotationsManagedFieldsMismatch function. +func SecretBaseLabelsMismatch(input Input) (string, string, bool) { // check if Secret has the base labels. Currently there is only one base label if input.Secret.Labels == nil { - return SecretBaseLabelsMissing, fmt.Sprintf("missing base label %s", cmapi.PartOfCertManagerControllerLabelKey), true + return "", "", false } - if _, ok := input.Secret.Labels[cmapi.PartOfCertManagerControllerLabelKey]; !ok { - return SecretBaseLabelsMissing, fmt.Sprintf("missing base label %s", cmapi.PartOfCertManagerControllerLabelKey), true + + value, ok := input.Secret.Labels[cmapi.PartOfCertManagerControllerLabelKey] + if !ok || value == "true" { + return "", "", false + } + + return SecretManagedMetadataMismatch, fmt.Sprintf("wrong base label %s value %q, expected \"true\"", cmapi.PartOfCertManagerControllerLabelKey, value), true +} + +// SecretCertificateDetailsAnnotationsMismatch - When the certificate details annotations are +// not matching, the secret is updated. +// NOTE: The presence of the certificate details annotations is checked +// by the SecretManagedLabelsAndAnnotationsManagedFieldsMismatch function. +func SecretCertificateDetailsAnnotationsMismatch(input Input) (string, string, bool) { + dataAnnotations, err := certificateDataAnnotationsForSecret(input.Secret) + if err != nil { + return InvalidCertificate, fmt.Sprintf("Failed getting secret annotations: %v", err), true + } + + for k, v := range dataAnnotations { + existing, ok := input.Secret.Annotations[k] + if !ok || existing == v { + continue + } + + return SecretManagedMetadataMismatch, fmt.Sprintf("Secret metadata %s does not match certificate metadata %s", input.Secret.Annotations[k], v), true } 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 { @@ -498,7 +649,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 @@ -506,7 +657,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 ( @@ -603,10 +754,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 381952cb21c..8e24237729d 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,34 @@ 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 'certificate name' annotation": { + certificate: &cmapi.Certificate{Spec: cmapi.CertificateSpec{ + SecretName: "something", + IssuerRef: cmmeta.ObjectReference{ + Name: "testissuer", + }, + }}, + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "something", + Annotations: map[string]string{ + cmapi.CertificateNameKey: "oldname", + }, + }, + Data: map[string][]byte{ + corev1.TLSPrivateKeyKey: staticFixedPrivateKey, + corev1.TLSCertKey: testcrypto.MustCreateCert(t, staticFixedPrivateKey, + &cmapi.Certificate{Spec: cmapi.CertificateSpec{CommonName: "example.com"}}, + ), + }, + }, + reason: IncorrectCertificate, + message: "Issuing certificate as Secret was previously issued for Certificate \"oldname\"", + reissue: true, + }, + "trigger issuance as Secret has old or incorrect 'issuer name' annotation": { certificate: &cmapi.Certificate{Spec: cmapi.CertificateSpec{ SecretName: "something", IssuerRef: cmmeta.ObjectReference{ @@ -156,10 +181,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 +207,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 +235,61 @@ 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 issuance as current CertificateRequest is not signed with private key": { + certificate: &cmapi.Certificate{Spec: cmapi.CertificateSpec{SecretName: "something"}}, + secret: &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "something"}, + Data: map[string][]byte{ + corev1.TLSPrivateKeyKey: staticFixedPrivateKey, + corev1.TLSCertKey: testcrypto.MustCreateCert( + t, staticFixedPrivateKey, + &cmapi.Certificate{Spec: cmapi.CertificateSpec{CommonName: "example.com"}}, + ), + }, + }, + request: &cmapi.CertificateRequest{Spec: cmapi.CertificateRequestSpec{ + IssuerRef: cmmeta.ObjectReference{ + Name: "testissuer", + Kind: "IssuerKind", + Group: "group.example.com", + }, + Request: testcrypto.MustGenerateCSRImpl(t, testcrypto.MustCreatePEMPrivateKey(t), &cmapi.Certificate{Spec: cmapi.CertificateSpec{ + CommonName: "example.com", + }}), + }}, + reason: InvalidCertificateRequest, + message: "Secret contains a private key that does not match the current CertificateRequest", reissue: true, }, // we only have a basic test here for this as unit tests for the @@ -319,7 +398,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)": { @@ -512,7 +591,126 @@ func Test_NewTriggerPolicyChain(t *testing.T) { } } -func Test_SecretTemplateMismatchesSecret(t *testing.T) { +func Test_SecretManagedLabelsAndAnnotationsManagedFieldsMismatch(t *testing.T) { + const fieldManager = "cert-manager-unit-test" + + var ( + fixedClockStart = time.Now() + fixedClock = fakeclock.NewFakeClock(fixedClockStart) + baseCertBundle = testcrypto.MustCreateCryptoBundle(t, + gen.Certificate("test-certificate", gen.SetCertificateCommonName("cert-manager")), fixedClock) + ) + + tests := map[string]struct { + secretManagedFields []metav1.ManagedFieldsEntry + secretData map[string][]byte + + expReason string + expMessage string + expViolation bool + }{ + "if there are no cert-manager annotations and the certificate data is nil, should return false": { + secretManagedFields: []metav1.ManagedFieldsEntry{ + {Manager: fieldManager, FieldsV1: &metav1.FieldsV1{ + Raw: []byte(`{"f:metadata": { + "f:labels": { + "f:controller.cert-manager.io/fao": {} + } + }}`), + }}, + }, + expReason: "", + expMessage: "", + expViolation: false, + }, + "if optional cert-manager annotations are present with no certificate data, should return false": { + secretManagedFields: []metav1.ManagedFieldsEntry{ + {Manager: fieldManager, FieldsV1: &metav1.FieldsV1{ + Raw: []byte(`{"f:metadata": { + "f:labels": { + "f:controller.cert-manager.io/fao": {} + }, + "f:annotations": { + "f:foo1": {}, + "f:foo2": {}, + "f:cert-manager.io/certificate-name": {}, + "f:cert-manager.io/issuer-name": {}, + "f:cert-manager.io/issuer-kind": {}, + "f:cert-manager.io/issuer-group": {} + } + }}`), + }}, + }, + expReason: "", + expMessage: "", + expViolation: false, + }, + "if cert-manager annotations are present with certificate data, should return false": { + secretManagedFields: []metav1.ManagedFieldsEntry{ + {Manager: fieldManager, FieldsV1: &metav1.FieldsV1{ + Raw: []byte(`{"f:metadata": { + "f:labels": { + "f:controller.cert-manager.io/fao": {} + }, + "f:annotations": { + "f:foo1": {}, + "f:foo2": {}, + "f:cert-manager.io/certificate-name": {}, + "f:cert-manager.io/issuer-name": {}, + "f:cert-manager.io/issuer-kind": {}, + "f:cert-manager.io/issuer-group": {}, + "f:cert-manager.io/common-name": {}, + "f:cert-manager.io/alt-names": {}, + "f:cert-manager.io/ip-sans": {}, + "f:cert-manager.io/uri-sans": {} + } + }}`), + }}, + }, + secretData: map[string][]byte{corev1.TLSCertKey: baseCertBundle.CertBytes}, + expReason: "", + expMessage: "", + expViolation: false, + }, + "if required and optional cert-manager annotations are present with certificate data but certificate data is nil, should return true": { + secretManagedFields: []metav1.ManagedFieldsEntry{ + {Manager: fieldManager, FieldsV1: &metav1.FieldsV1{ + Raw: []byte(`{"f:metadata": { + "f:labels": { + "f:controller.cert-manager.io/fao": {} + }, + "f:annotations": { + "f:foo1": {}, + "f:foo2": {}, + "f:cert-manager.io/certificate-name": {}, + "f:cert-manager.io/issuer-name": {}, + "f:cert-manager.io/issuer-kind": {}, + "f:cert-manager.io/issuer-group": {}, + "f:cert-manager.io/uri-sans": {} + } + }}`), + }}, + }, + expReason: SecretManagedMetadataMismatch, + expMessage: "Secret has these extra Annotations: [cert-manager.io/uri-sans]", + expViolation: true, + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + gotReason, gotMessage, gotViolation := SecretManagedLabelsAndAnnotationsManagedFieldsMismatch(fieldManager)(Input{ + Secret: &corev1.Secret{ObjectMeta: metav1.ObjectMeta{ManagedFields: test.secretManagedFields}, Data: test.secretData}, + }) + + assert.Equal(t, test.expReason, gotReason, "unexpected reason") + assert.Equal(t, test.expMessage, gotMessage, "unexpected message") + assert.Equal(t, test.expViolation, gotViolation, "unexpected violation") + }) + } +} + +func Test_SecretSecretTemplateMismatch(t *testing.T) { tests := map[string]struct { tmpl *cmapi.CertificateSecretTemplate secret *corev1.Secret @@ -590,7 +788,7 @@ func Test_SecretTemplateMismatchesSecret(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"}, @@ -646,7 +844,7 @@ func Test_SecretTemplateMismatchesSecret(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, }) @@ -658,20 +856,12 @@ func Test_SecretTemplateMismatchesSecret(t *testing.T) { } } -func Test_SecretTemplateMismatchesSecretManagedFields(t *testing.T) { +func Test_SecretSecretTemplateManagedFieldsMismatch(t *testing.T) { const fieldManager = "cert-manager-unit-test" - var ( - fixedClockStart = time.Now() - fixedClock = fakeclock.NewFakeClock(fixedClockStart) - baseCertBundle = testcrypto.MustCreateCryptoBundle(t, - gen.Certificate("test-certificate", gen.SetCertificateCommonName("cert-manager")), fixedClock) - ) - tests := map[string]struct { tmpl *cmapi.CertificateSecretTemplate secretManagedFields []metav1.ManagedFieldsEntry - secretData map[string][]byte expReason string expMessage string @@ -716,7 +906,7 @@ func Test_SecretTemplateMismatchesSecretManagedFields(t *testing.T) { }, secretManagedFields: nil, expReason: SecretTemplateMismatch, - expMessage: "Certificate's SecretTemplate doesn't match Secret", + expMessage: "Secret is missing these Template Labels: [abc]", expViolation: true, }, "if template is nil but managed fields is not nil, should return true": { @@ -734,7 +924,7 @@ func Test_SecretTemplateMismatchesSecretManagedFields(t *testing.T) { }}, }, expReason: SecretTemplateMismatch, - expMessage: "SecretTemplate is nil, but Secret contains extra managed entries", + expMessage: "Secret has these extra Labels: [abc]", expViolation: true, }, "if template annotations do not match managed fields, should return true": { @@ -757,7 +947,7 @@ func Test_SecretTemplateMismatchesSecretManagedFields(t *testing.T) { }}, }, expReason: SecretTemplateMismatch, - expMessage: "Certificate's SecretTemplate doesn't match Secret", + expMessage: "Secret is missing these Template Annotations: [foo2]", expViolation: true, }, "if template labels do not match managed fields, should return true": { @@ -780,7 +970,7 @@ func Test_SecretTemplateMismatchesSecretManagedFields(t *testing.T) { }}, }, expReason: SecretTemplateMismatch, - expMessage: "Certificate's SecretTemplate doesn't match Secret", + expMessage: "Secret is missing these Template Labels: [def]", expViolation: true, }, "if template annotations and labels match managed fields, should return false": { @@ -827,7 +1017,7 @@ func Test_SecretTemplateMismatchesSecretManagedFields(t *testing.T) { }}, }, expReason: SecretTemplateMismatch, - expMessage: "Certificate's SecretTemplate doesn't match Secret", + expMessage: "Secret has these extra Annotations: [foo3]", expViolation: true, }, "if template labels is a subset of managed fields, return true": { @@ -851,7 +1041,7 @@ func Test_SecretTemplateMismatchesSecretManagedFields(t *testing.T) { }}, }, expReason: SecretTemplateMismatch, - expMessage: "Certificate's SecretTemplate doesn't match Secret", + expMessage: "Secret has these extra Labels: [ghi]", expViolation: true, }, "if managed fields annotations is a subset of template, return true": { @@ -874,7 +1064,7 @@ func Test_SecretTemplateMismatchesSecretManagedFields(t *testing.T) { }}, }, expReason: SecretTemplateMismatch, - expMessage: "Certificate's SecretTemplate doesn't match Secret", + expMessage: "Secret is missing these Template Annotations: [foo3]", expViolation: true, }, "if managed fields labels is a subset of template, return true": { @@ -897,7 +1087,7 @@ func Test_SecretTemplateMismatchesSecretManagedFields(t *testing.T) { }}, }, expReason: SecretTemplateMismatch, - expMessage: "Certificate's SecretTemplate doesn't match Secret", + expMessage: "Secret is missing these Template Labels: [ghi]", expViolation: true, }, "if managed fields matches template but is split across multiple managed fields, should return false": { @@ -943,7 +1133,7 @@ func Test_SecretTemplateMismatchesSecretManagedFields(t *testing.T) { expMessage: "", expViolation: false, }, - "if managed fields matches template and base cert-manager annotations are present with no certificate data, should return false": { + "if managed fields matches template and cert-manager annotations are present, should return false": { tmpl: &cmapi.CertificateSecretTemplate{ Annotations: map[string]string{"foo1": "bar1", "foo2": "bar2"}, }, @@ -953,10 +1143,8 @@ func Test_SecretTemplateMismatchesSecretManagedFields(t *testing.T) { "f:annotations": { "f:foo1": {}, "f:foo2": {}, - "f:cert-manager.io/certificate-name": {}, - "f:cert-manager.io/issuer-name": {}, - "f:cert-manager.io/issuer-kind": {}, - "f:cert-manager.io/issuer-group": {} + "f:cert-manager.io/foo1": {}, + "f:cert-manager.io/foo2": {} } }}`), }}, @@ -965,64 +1153,13 @@ func Test_SecretTemplateMismatchesSecretManagedFields(t *testing.T) { expMessage: "", expViolation: false, }, - "if managed fields matches template and base cert-manager annotations are present with certificate data, should return false": { - tmpl: &cmapi.CertificateSecretTemplate{ - Annotations: map[string]string{"foo1": "bar1", "foo2": "bar2"}, - }, - secretManagedFields: []metav1.ManagedFieldsEntry{ - {Manager: fieldManager, FieldsV1: &metav1.FieldsV1{ - Raw: []byte(`{"f:metadata": { - "f:annotations": { - "f:foo1": {}, - "f:foo2": {}, - "f:cert-manager.io/certificate-name": {}, - "f:cert-manager.io/issuer-name": {}, - "f:cert-manager.io/issuer-kind": {}, - "f:cert-manager.io/issuer-group": {}, - "f:cert-manager.io/common-name": {}, - "f:cert-manager.io/alt-names": {}, - "f:cert-manager.io/ip-sans": {}, - "f:cert-manager.io/uri-sans": {} - } - }}`), - }}, - }, - secretData: map[string][]byte{corev1.TLSCertKey: baseCertBundle.CertBytes}, - expViolation: false, - }, - "if managed fields matches template and base cert-manager annotations are present with certificate data but certificate data is nil, should return true": { - tmpl: &cmapi.CertificateSecretTemplate{ - Annotations: map[string]string{"foo1": "bar1", "foo2": "bar2"}, - }, - secretManagedFields: []metav1.ManagedFieldsEntry{ - {Manager: fieldManager, FieldsV1: &metav1.FieldsV1{ - Raw: []byte(`{"f:metadata": { - "f:annotations": { - "f:foo1": {}, - "f:foo2": {}, - "f:cert-manager.io/certificate-name": {}, - "f:cert-manager.io/issuer-name": {}, - "f:cert-manager.io/issuer-kind": {}, - "f:cert-manager.io/issuer-group": {}, - "f:cert-manager.io/common-name": {}, - "f:cert-manager.io/alt-names": {}, - "f:cert-manager.io/ip-sans": {}, - "f:cert-manager.io/uri-sans": {} - } - }}`), - }}, - }, - expReason: SecretTemplateMismatch, - expMessage: "Certificate's SecretTemplate doesn't match Secret", - expViolation: true, - }, } 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: test.secretData}, + Secret: &corev1.Secret{ObjectMeta: metav1.ObjectMeta{ManagedFields: test.secretManagedFields}, Data: map[string][]byte{}}, }) assert.Equal(t, test.expReason, gotReason, "unexpected reason") @@ -1032,7 +1169,7 @@ func Test_SecretTemplateMismatchesSecretManagedFields(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) @@ -1289,7 +1426,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) @@ -1297,7 +1434,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 { @@ -1721,7 +1858,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) @@ -1909,7 +2046,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")), ) @@ -2157,7 +2294,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/constants.go b/internal/controller/certificates/policies/constants.go index 6afde6e0e74..5b0f806571f 100644 --- a/internal/controller/certificates/policies/constants.go +++ b/internal/controller/certificates/policies/constants.go @@ -29,12 +29,20 @@ const ( // InvalidCertificate is a policy violation whereby the signed certificate in // the Input Secret could not be parsed or decoded. InvalidCertificate string = "InvalidCertificate" + // InvalidCertificateRequest is a policy violation whereby the CSR in + // the Input CertificateRequest could not be parsed or decoded. + InvalidCertificateRequest string = "InvalidCertificateRequest" + // SecretMismatch is a policy violation reason for a scenario where Secret's // private key does not match spec. SecretMismatch string = "SecretMismatch" + // IncorrectCertificate is a policy violation reason for a scenario where + // the CertificateName annotation on the Secret does not match the Certificate name. + IncorrectCertificate string = "IncorrectCertificate" // IncorrectIssuer is a policy violation reason for a scenario where // Certificate has been issued by incorrect Issuer. IncorrectIssuer string = "IncorrectIssuer" + // RequestChanged is a policy violation reason for a scenario where // CertificateRequest not valid for Certificate's spec. RequestChanged string = "RequestChanged" @@ -48,10 +56,9 @@ const ( // SecretTemplate is not reflected on the target Secret, either by having // extra, missing, or wrong Annotations or Labels. SecretTemplateMismatch string = "SecretTemplateMismatch" - - // SecretBaseLabelsMissing is a policy violation whereby the Secret is + // SecretManagedMetadataMismatch is a policy violation whereby the Secret is // missing labels that should have been added by cert-manager - SecretBaseLabelsMissing string = "SecretBaseLabelsMissing" + SecretManagedMetadataMismatch string = "SecretManagedMetadataMismatch" // AdditionalOutputFormatsMismatch is a policy violation whereby the // Certificate's AdditionalOutputFormats is not reflected on the target diff --git a/internal/controller/certificates/policies/policies.go b/internal/controller/certificates/policies/policies.go index ff8f27cc56b..e9ec6fbffd4 100644 --- a/internal/controller/certificates/policies/policies.go +++ b/internal/controller/certificates/policies/policies.go @@ -67,13 +67,17 @@ 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, - 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 + + SecretCertificateNameAnnotationMismatch, // Make sure the Secret's CertificateName annotation matches the Certificate Name + SecretIssuerAnnotationsMismatch, // Make sure the Secret's IssuerRef annotations match the Certificate spec + + SecretPrivateKeyMismatchesSpec, // Make sure the PrivateKey Type and Size match the Certificate spec + SecretPublicKeysDiffersFromCurrentCertificateRequest, // 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 } } @@ -81,11 +85,17 @@ 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, - 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 + + SecretCertificateNameAnnotationMismatch, // Make sure the Secret's CertificateName annotation matches the Certificate Name + SecretIssuerAnnotationsMismatch, // Make sure the Secret's IssuerRef annotations match the Certificate spec + + SecretPrivateKeyMismatchesSpec, // Make sure the PrivateKey Type and Size match the Certificate spec + SecretPublicKeysDiffersFromCurrentCertificateRequest, // 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 } } @@ -94,14 +104,17 @@ func NewReadinessPolicyChain(c clock.Clock) Chain { // correctness of metadata and output formats of Certificate's Secrets. func NewSecretPostIssuancePolicyChain(ownerRefEnabled bool, fieldManager string) Chain { return Chain{ - SecretTemplateMismatchesSecret, - SecretTemplateMismatchesSecretManagedFields(fieldManager), - SecretAdditionalOutputFormatsDataMismatch, - SecretAdditionalOutputFormatsOwnerMismatch(fieldManager), + SecretBaseLabelsMismatch, + SecretCertificateDetailsAnnotationsMismatch, + SecretManagedLabelsAndAnnotationsManagedFieldsMismatch(fieldManager), + SecretSecretTemplateMismatch, + SecretSecretTemplateManagedFieldsMismatch(fieldManager), + SecretAdditionalOutputFormatsMismatch, + SecretAdditionalOutputFormatsManagedFieldsMismatch(fieldManager), + SecretOwnerReferenceMismatch(ownerRefEnabled), SecretOwnerReferenceManagedFieldMismatch(ownerRefEnabled, fieldManager), - SecretOwnerReferenceValueMismatch(ownerRefEnabled), - SecretKeystoreFormatMatchesSpec, - SecretBaseLabelsAreMissing, + + SecretKeystoreFormatMismatch, } } @@ -109,8 +122,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/issuing/secret_manager_test.go b/pkg/controller/certificates/issuing/secret_manager_test.go index efc77ec5daf..25586c70199 100644 --- a/pkg/controller/certificates/issuing/secret_manager_test.go +++ b/pkg/controller/certificates/issuing/secret_manager_test.go @@ -202,10 +202,15 @@ func Test_ensureSecretData(t *testing.T) { FieldsV1: &metav1.FieldsV1{ Raw: []byte(`{"f:metadata": { "f:annotations": { + "f:cert-manager.io/common-name": {}, + "f:cert-manager.io/alt-names": {}, + "f:cert-manager.io/ip-sans": {}, + "f:cert-manager.io/uri-sans": {}, "f:foo": {}, "f:another-annotation": {} }, "f:labels": { + "f:controller.cert-manager.io/fao": {}, "f:abc": {}, "f:another-label": {} } @@ -241,9 +246,14 @@ func Test_ensureSecretData(t *testing.T) { FieldsV1: &metav1.FieldsV1{ Raw: []byte(`{"f:metadata": { "f:annotations": { + "f:cert-manager.io/common-name": {}, + "f:cert-manager.io/alt-names": {}, + "f:cert-manager.io/ip-sans": {}, + "f:cert-manager.io/uri-sans": {}, "f:foo": {} }, "f:labels": { + "f:controller.cert-manager.io/fao": {}, "f:abc": {} } }}`), @@ -280,9 +290,14 @@ func Test_ensureSecretData(t *testing.T) { FieldsV1: &metav1.FieldsV1{ Raw: []byte(`{"f:metadata": { "f:annotations": { + "f:cert-manager.io/common-name": {}, + "f:cert-manager.io/alt-names": {}, + "f:cert-manager.io/ip-sans": {}, + "f:cert-manager.io/uri-sans": {}, "f:foo": {} }, "f:labels": { + "f:controller.cert-manager.io/fao": {}, "f:abc": {} } }}`), @@ -452,10 +467,27 @@ func Test_ensureSecretData(t *testing.T) { ManagedFields: []metav1.ManagedFieldsEntry{{ Manager: fieldManager, FieldsV1: &metav1.FieldsV1{ - Raw: []byte(`{"f:data": { - "f:tls-combined.pem": {}, - "f:key.der": {} - }}`), + Raw: []byte(` + { + "f:metadata": { + "f:labels": { + "f:controller.cert-manager.io/fao": {} + }, + "f:annotations": { + "f:cert-manager.io/common-name": {}, + "f:cert-manager.io/alt-names": {}, + "f:cert-manager.io/ip-sans": {}, + "f:cert-manager.io/uri-sans": {} + }, + "f:ownerReferences": { + "k:{\"uid\":\"uid-123\"}": {} + } + }, + "f:data": { + "f:tls-combined.pem": {}, + "f:key.der": {} + } + }`), }, }}, }, @@ -483,10 +515,27 @@ func Test_ensureSecretData(t *testing.T) { ManagedFields: []metav1.ManagedFieldsEntry{{ Manager: fieldManager, FieldsV1: &metav1.FieldsV1{ - Raw: []byte(`{"f:data": { - "f:tls-combined.pem": {}, - "f:key.der": {} - }}`), + Raw: []byte(` + { + "f:metadata": { + "f:labels": { + "f:controller.cert-manager.io/fao": {} + }, + "f:annotations": { + "f:cert-manager.io/common-name": {}, + "f:cert-manager.io/alt-names": {}, + "f:cert-manager.io/ip-sans": {}, + "f:cert-manager.io/uri-sans": {} + }, + "f:ownerReferences": { + "k:{\"uid\":\"uid-123\"}": {} + } + }, + "f:data": { + "f:tls-combined.pem": {}, + "f:key.der": {} + } + }`), }, }}, }, @@ -513,10 +562,20 @@ func Test_ensureSecretData(t *testing.T) { ManagedFields: []metav1.ManagedFieldsEntry{ {Manager: fieldManager, FieldsV1: &metav1.FieldsV1{ Raw: []byte(` - {"f:metadata": { + {"f:metadata": { + "f:labels": { + "f:controller.cert-manager.io/fao": {} + }, + "f:annotations": { + "f:cert-manager.io/common-name": {}, + "f:cert-manager.io/alt-names": {}, + "f:cert-manager.io/ip-sans": {}, + "f:cert-manager.io/uri-sans": {} + }, "f:ownerReferences": { - "k:{\"uid\":\"uid-123\"}": {} - }}}`), + "k:{\"uid\":\"uid-123\"}": {} + } + }}`), }}, }, }, @@ -540,10 +599,20 @@ func Test_ensureSecretData(t *testing.T) { ManagedFields: []metav1.ManagedFieldsEntry{ {Manager: fieldManager, FieldsV1: &metav1.FieldsV1{ Raw: []byte(` - {"f:metadata": { + {"f:metadata": { + "f:labels": { + "f:controller.cert-manager.io/fao": {} + }, + "f:annotations": { + "f:cert-manager.io/common-name": {}, + "f:cert-manager.io/alt-names": {}, + "f:cert-manager.io/ip-sans": {}, + "f:cert-manager.io/uri-sans": {} + }, "f:ownerReferences": { - "k:{\"uid\":\"uid-123\"}": {} - }}}`), + "k:{\"uid\":\"uid-123\"}": {} + } + }}`), }}, }, }, @@ -579,10 +648,20 @@ func Test_ensureSecretData(t *testing.T) { ManagedFields: []metav1.ManagedFieldsEntry{ {Manager: fieldManager, FieldsV1: &metav1.FieldsV1{ Raw: []byte(` - {"f:metadata": { + {"f:metadata": { + "f:labels": { + "f:controller.cert-manager.io/fao": {} + }, + "f:annotations": { + "f:cert-manager.io/common-name": {}, + "f:cert-manager.io/alt-names": {}, + "f:cert-manager.io/ip-sans": {}, + "f:cert-manager.io/uri-sans": {} + }, "f:ownerReferences": { - "k:{\"uid\":\"uid-234\"}": {} - }}}`), + "k:{\"uid\":\"uid-123\"}": {} + } + }}`), }}, }, }, @@ -629,10 +708,20 @@ func Test_ensureSecretData(t *testing.T) { ManagedFields: []metav1.ManagedFieldsEntry{ {Manager: fieldManager, FieldsV1: &metav1.FieldsV1{ Raw: []byte(` - {"f:metadata": { + {"f:metadata": { + "f:labels": { + "f:controller.cert-manager.io/fao": {} + }, + "f:annotations": { + "f:cert-manager.io/common-name": {}, + "f:cert-manager.io/alt-names": {}, + "f:cert-manager.io/ip-sans": {}, + "f:cert-manager.io/uri-sans": {} + }, "f:ownerReferences": { - "k:{\"uid\":\"uid-123\"}": {} - }}}`), + "k:{\"uid\":\"uid-123\"}": {} + } + }}`), }}, }, }, @@ -678,10 +767,20 @@ func Test_ensureSecretData(t *testing.T) { ManagedFields: []metav1.ManagedFieldsEntry{ {Manager: fieldManager, FieldsV1: &metav1.FieldsV1{ Raw: []byte(` - {"f:metadata": { + {"f:metadata": { + "f:labels": { + "f:controller.cert-manager.io/fao": {} + }, + "f:annotations": { + "f:cert-manager.io/common-name": {}, + "f:cert-manager.io/alt-names": {}, + "f:cert-manager.io/ip-sans": {}, + "f:cert-manager.io/uri-sans": {} + }, "f:ownerReferences": { - "k:{\"uid\":\"uid-123\"}": {} - }}}`), + "k:{\"uid\":\"uid-123\"}": {} + } + }}`), }}, }, }, @@ -726,10 +825,20 @@ func Test_ensureSecretData(t *testing.T) { ManagedFields: []metav1.ManagedFieldsEntry{ {Manager: fieldManager, FieldsV1: &metav1.FieldsV1{ Raw: []byte(` - {"f:metadata": { + {"f:metadata": { + "f:labels": { + "f:controller.cert-manager.io/fao": {} + }, + "f:annotations": { + "f:cert-manager.io/common-name": {}, + "f:cert-manager.io/alt-names": {}, + "f:cert-manager.io/ip-sans": {}, + "f:cert-manager.io/uri-sans": {} + }, "f:ownerReferences": { - "k:{\"uid\":\"uid-123\"}": {} - }}}`), + "k:{\"uid\":\"uid-123\"}": {} + } + }}`), }}, }, }, @@ -776,10 +885,20 @@ func Test_ensureSecretData(t *testing.T) { ManagedFields: []metav1.ManagedFieldsEntry{ {Manager: fieldManager, FieldsV1: &metav1.FieldsV1{ Raw: []byte(` - {"f:metadata": { + {"f:metadata": { + "f:labels": { + "f:controller.cert-manager.io/fao": {} + }, + "f:annotations": { + "f:cert-manager.io/common-name": {}, + "f:cert-manager.io/alt-names": {}, + "f:cert-manager.io/ip-sans": {}, + "f:cert-manager.io/uri-sans": {} + }, "f:ownerReferences": { - "k:{\"uid\":\"uid-123\"}": {} - }}}`), + "k:{\"uid\":\"uid-123\"}": {} + } + }}`), }}, }, }, @@ -825,10 +944,20 @@ func Test_ensureSecretData(t *testing.T) { ManagedFields: []metav1.ManagedFieldsEntry{ {Manager: fieldManager, FieldsV1: &metav1.FieldsV1{ Raw: []byte(` - {"f:metadata": { + {"f:metadata": { + "f:labels": { + "f:controller.cert-manager.io/fao": {} + }, + "f:annotations": { + "f:cert-manager.io/common-name": {}, + "f:cert-manager.io/alt-names": {}, + "f:cert-manager.io/ip-sans": {}, + "f:cert-manager.io/uri-sans": {} + }, "f:ownerReferences": { - "k:{\"uid\":\"uid-123\"}": {} - }}}`), + "k:{\"uid\":\"uid-123\"}": {} + } + }}`), }}, }, }, @@ -874,10 +1003,20 @@ func Test_ensureSecretData(t *testing.T) { ManagedFields: []metav1.ManagedFieldsEntry{ {Manager: fieldManager, FieldsV1: &metav1.FieldsV1{ Raw: []byte(` - {"f:metadata": { + {"f:metadata": { + "f:labels": { + "f:controller.cert-manager.io/fao": {} + }, + "f:annotations": { + "f:cert-manager.io/common-name": {}, + "f:cert-manager.io/alt-names": {}, + "f:cert-manager.io/ip-sans": {}, + "f:cert-manager.io/uri-sans": {} + }, "f:ownerReferences": { - "k:{\"uid\":\"uid-123\"}": {} - }}}`), + "k:{\"uid\":\"uid-123\"}": {} + } + }}`), }}, }, }, @@ -922,10 +1061,20 @@ func Test_ensureSecretData(t *testing.T) { ManagedFields: []metav1.ManagedFieldsEntry{ {Manager: fieldManager, FieldsV1: &metav1.FieldsV1{ Raw: []byte(` - {"f:metadata": { + {"f:metadata": { + "f:labels": { + "f:controller.cert-manager.io/fao": {} + }, + "f:annotations": { + "f:cert-manager.io/common-name": {}, + "f:cert-manager.io/alt-names": {}, + "f:cert-manager.io/ip-sans": {}, + "f:cert-manager.io/uri-sans": {} + }, "f:ownerReferences": { - "k:{\"uid\":\"uid-123\"}": {} - }}}`), + "k:{\"uid\":\"uid-123\"}": {} + } + }}`), }}, }, }, @@ -972,10 +1121,20 @@ func Test_ensureSecretData(t *testing.T) { ManagedFields: []metav1.ManagedFieldsEntry{ {Manager: fieldManager, FieldsV1: &metav1.FieldsV1{ Raw: []byte(` - {"f:metadata": { + {"f:metadata": { + "f:labels": { + "f:controller.cert-manager.io/fao": {} + }, + "f:annotations": { + "f:cert-manager.io/common-name": {}, + "f:cert-manager.io/alt-names": {}, + "f:cert-manager.io/ip-sans": {}, + "f:cert-manager.io/uri-sans": {} + }, "f:ownerReferences": { - "k:{\"uid\":\"uid-123\"}": {} - }}}`), + "k:{\"uid\":\"uid-123\"}": {} + } + }}`), }}, }, }, diff --git a/pkg/controller/certificates/readiness/readiness_controller_test.go b/pkg/controller/certificates/readiness/readiness_controller_test.go index b4da05de563..0a9f38e43c2 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": { @@ -467,7 +467,38 @@ func TestNewReadinessPolicyChain(t *testing.T) { Name: "testissuer", Kind: "IssuerKind", Group: "group.example.com", + }), + gen.SetCertificateRequestCSR(testcrypto.MustGenerateCSRImpl(t, privKey, + gen.Certificate("something", + gen.SetCertificateCommonName("new.example.com")))), + ), + reason: policies.Expired, + message: "Certificate expired on Sun, 31 Dec 0000 23:00:00 UTC", + violationFound: true, + }, + "Certificate is not Ready when it has expired (no cr)": { + cert: gen.Certificate("something", + gen.SetCertificateCommonName("new.example.com"), + gen.SetCertificateIssuer(cmmeta.ObjectReference{ + Name: "testissuer", + Kind: "IssuerKind", + Group: "group.example.com", })), + secret: gen.Secret("something", + gen.SetSecretAnnotations(map[string]string{ + cmapi.IssuerNameAnnotationKey: "testissuer", + cmapi.IssuerKindAnnotationKey: "IssuerKind", + cmapi.IssuerGroupAnnotationKey: "group.example.com", + }), + gen.SetSecretData( + map[string][]byte{ + corev1.TLSPrivateKeyKey: privKey, + corev1.TLSCertKey: testcrypto.MustCreateCertWithNotBeforeAfter(t, privKey, + gen.Certificate("something", gen.SetCertificateCommonName("new.example.com")), + clock.Now().Add(-3*time.Hour), clock.Now().Add(-1*time.Hour), + ), + }, + )), reason: policies.Expired, message: "Certificate expired on Sun, 31 Dec 0000 23:00:00 UTC", violationFound: true,