From a9339849e59bfabe851135d160294348380ce784 Mon Sep 17 00:00:00 2001 From: Tim Ramlot <42113979+inteon@users.noreply.github.com> Date: Fri, 23 Jun 2023 10:22:46 +0200 Subject: [PATCH 1/4] improve label and annotation checks Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com> --- .../certificates/policies/checks.go | 302 ++++++++++++------ .../certificates/policies/checks_test.go | 208 +++++++----- .../certificates/policies/constants.go | 5 +- .../certificates/policies/policies.go | 4 +- .../issuing/secret_manager_test.go | 241 +++++++++++--- 5 files changed, 542 insertions(+), 218 deletions(-) diff --git a/internal/controller/certificates/policies/checks.go b/internal/controller/certificates/policies/checks.go index 4341c773c54..c560e0a10eb 100644 --- a/internal/controller/certificates/policies/checks.go +++ b/internal/controller/certificates/policies/checks.go @@ -355,143 +355,249 @@ func SecretTemplateMismatchesSecret(input Input) (string, string, bool) { return "", "", false } -// SecretTemplateMismatchesSecretManagedFields will inspect the given Secret's +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 +} + +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 +} + +// 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) { - // Only attempt to decode the signed certificate, if one is available. - var x509cert *x509.Certificate - if len(input.Secret.Data[corev1.TLSCertKey]) > 0 { - var err error - 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 + 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) } - baseAnnotations, err := internalcertificates.AnnotationsForCertificate(x509cert) + // 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) + } + + expCertificateDataAnnotations, err := certificateDataAnnotationsForSecret(input.Secret) 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 + } +} + +// SecretTemplateMismatchesSecretManagedFields 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 { + 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 diff --git a/internal/controller/certificates/policies/checks_test.go b/internal/controller/certificates/policies/checks_test.go index 09aa477e95b..0111239568f 100644 --- a/internal/controller/certificates/policies/checks_test.go +++ b/internal/controller/certificates/policies/checks_test.go @@ -537,7 +537,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 @@ -683,20 +802,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 @@ -741,7 +852,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": { @@ -759,7 +870,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": { @@ -782,7 +893,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": { @@ -805,7 +916,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": { @@ -852,7 +963,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": { @@ -876,7 +987,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": { @@ -899,7 +1010,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": { @@ -922,7 +1033,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": { @@ -968,7 +1079,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"}, }, @@ -978,10 +1089,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": {} } }}`), }}, @@ -990,64 +1099,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{ 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") diff --git a/internal/controller/certificates/policies/constants.go b/internal/controller/certificates/policies/constants.go index 32d556fdd58..c33692745e7 100644 --- a/internal/controller/certificates/policies/constants.go +++ b/internal/controller/certificates/policies/constants.go @@ -52,10 +52,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 37dd60caa41..912495e64c6 100644 --- a/internal/controller/certificates/policies/policies.go +++ b/internal/controller/certificates/policies/policies.go @@ -96,6 +96,9 @@ 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{ + SecretBaseLabelsMismatch, + SecretCertificateDetailsAnnotationsMismatch, + SecretManagedLabelsAndAnnotationsManagedFieldsMismatch(fieldManager), SecretTemplateMismatchesSecret, SecretTemplateMismatchesSecretManagedFields(fieldManager), SecretAdditionalOutputFormatsDataMismatch, @@ -103,7 +106,6 @@ func NewSecretPostIssuancePolicyChain(ownerRefEnabled bool, fieldManager string) SecretOwnerReferenceManagedFieldMismatch(ownerRefEnabled, fieldManager), SecretOwnerReferenceValueMismatch(ownerRefEnabled), SecretKeystoreFormatMatchesSpec, - SecretBaseLabelsAreMissing, } } 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\"}": {} + } + }}`), }}, }, }, From 1649730a0d6dcfcde56639ef236e5a2dacdf6fde Mon Sep 17 00:00:00 2001 From: Tim Ramlot <42113979+inteon@users.noreply.github.com> Date: Thu, 29 Jun 2023 12:54:20 +0100 Subject: [PATCH 2/4] Update internal/controller/certificates/policies/checks.go Co-authored-by: Richard Wall Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com> --- internal/controller/certificates/policies/checks.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/controller/certificates/policies/checks.go b/internal/controller/certificates/policies/checks.go index c560e0a10eb..bbba224d6cc 100644 --- a/internal/controller/certificates/policies/checks.go +++ b/internal/controller/certificates/policies/checks.go @@ -446,7 +446,7 @@ func SecretManagedLabelsAndAnnotationsManagedFieldsMismatch(fieldManager string) delete(managedAnnotations, k) } - // Remove the non cert-manager labels from the managed Annotations so we can compare + // Remove the non cert-manager labels from the managed labels so we can compare // 1 to 1 all the cert-manager labels. for k := range managedLabels { if strings.HasPrefix(k, "cert-manager.io/") || From c16a34e0b10c304402680a80e85261afcbfed973 Mon Sep 17 00:00:00 2001 From: Tim Ramlot <42113979+inteon@users.noreply.github.com> Date: Thu, 29 Jun 2023 18:47:50 +0200 Subject: [PATCH 3/4] use .Delete() Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com> --- internal/controller/certificates/policies/checks.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/internal/controller/certificates/policies/checks.go b/internal/controller/certificates/policies/checks.go index bbba224d6cc..4dcafd34485 100644 --- a/internal/controller/certificates/policies/checks.go +++ b/internal/controller/certificates/policies/checks.go @@ -437,14 +437,12 @@ func SecretManagedLabelsAndAnnotationsManagedFieldsMismatch(fieldManager string) } // Ignore the CertificateName and IssuerRef annotations as these cannot be set by the postIssuance controller. - for _, k := range []string{ + managedAnnotations.Delete( 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 labels so we can compare // 1 to 1 all the cert-manager labels. From bfa61c7804ae5fb9ab602cf23c686802b24b78c9 Mon Sep 17 00:00:00 2001 From: Tim Ramlot <42113979+inteon@users.noreply.github.com> Date: Thu, 29 Jun 2023 18:48:38 +0200 Subject: [PATCH 4/4] add comments explaining what the label and annotation checks do Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com> --- internal/controller/certificates/policies/policies.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/controller/certificates/policies/policies.go b/internal/controller/certificates/policies/policies.go index 912495e64c6..7c379b44ef1 100644 --- a/internal/controller/certificates/policies/policies.go +++ b/internal/controller/certificates/policies/policies.go @@ -96,11 +96,11 @@ 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{ - SecretBaseLabelsMismatch, - SecretCertificateDetailsAnnotationsMismatch, - SecretManagedLabelsAndAnnotationsManagedFieldsMismatch(fieldManager), - SecretTemplateMismatchesSecret, - SecretTemplateMismatchesSecretManagedFields(fieldManager), + 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), SecretOwnerReferenceManagedFieldMismatch(ownerRefEnabled, fieldManager),