From d40dae9d6719610fc53ecf7803ee4ad0b61209ec Mon Sep 17 00:00:00 2001 From: Tim Ramlot <42113979+inteon@users.noreply.github.com> Date: Wed, 11 Oct 2023 13:47:44 +0200 Subject: [PATCH] Fix DuplicateSecretName issue Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com> --- .../controller/certificates/certificates.go | 87 +++++++++++++++++++ .../certificates/policies/checks.go | 11 +++ .../certificates/policies/constants.go | 3 + .../certificates/policies/policies.go | 6 +- .../trigger/trigger_controller.go | 13 +++ 5 files changed, 118 insertions(+), 2 deletions(-) create mode 100644 internal/controller/certificates/certificates.go diff --git a/internal/controller/certificates/certificates.go b/internal/controller/certificates/certificates.go new file mode 100644 index 000000000..028ccf597 --- /dev/null +++ b/internal/controller/certificates/certificates.go @@ -0,0 +1,87 @@ +/* +Copyright 2022 The cert-manager Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package certificates + +import ( + "context" + "slices" + "sort" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/labels" + + internalinformers "github.com/cert-manager/cert-manager/internal/informers" + cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" + cmlisters "github.com/cert-manager/cert-manager/pkg/client/listers/certmanager/v1" +) + +func CertificateOwnsSecret( + ctx context.Context, + certificateLister cmlisters.CertificateLister, + secretLister internalinformers.SecretLister, + crt *cmapi.Certificate, +) (bool, []string, error) { + crts, err := certificateLister.Certificates(crt.Namespace).List(labels.Everything()) + if err != nil { + return false, nil, err + } + + sort.Slice(crts, func(i, j int) bool { + return crts[i].CreationTimestamp.Before(&crts[j].CreationTimestamp) || + crts[i].Name < crts[j].Name + }) + + var duplicates []string + for _, namespaceCrt := range crts { + // Check if it has the same Secret. + if namespaceCrt.Spec.SecretName == crt.Spec.SecretName { + // If it does, mark the Certificate as having a duplicate Secret. + duplicates = append(duplicates, namespaceCrt.Name) + } + } + + // If there are no duplicates, return early. + if len(duplicates) == 1 && duplicates[0] == crt.Name { + return true, nil, nil + } + + ownerCertificate := duplicates[0] + + // Fetch the Secret and determine if it is owned by any of the Certificates. + secret, err := secretLister.Secrets(crt.Namespace).Get(crt.Spec.SecretName) + if err != nil && !apierrors.IsNotFound(err) { + return false, nil, err + } else if err == nil { + if annotation, hasAnnotation := secret.GetAnnotations()[cmapi.CertificateNameKey]; hasAnnotation && slices.Contains(duplicates, annotation) { + ownerCertificate = annotation + } + } + + // If the Secret does not exist, only the first Certificate in the list + // is the owner of the Secret. + return crt.Name == ownerCertificate, sliceWithoutValue(duplicates, crt.Name), nil +} + +func sliceWithoutValue(slice []string, value string) []string { + result := make([]string, 0, len(slice)-1) + for _, v := range slice { + if v != value { + result = append(result, v) + } + } + return result +} diff --git a/internal/controller/certificates/policies/checks.go b/internal/controller/certificates/policies/checks.go index 10d8ee16c..b7ae5a31d 100644 --- a/internal/controller/certificates/policies/checks.go +++ b/internal/controller/certificates/policies/checks.go @@ -172,6 +172,17 @@ func SecretIssuerAnnotationsMismatch(input Input) (string, string, bool) { return "", "", false } +// SecretCertificateNameAnnotationsMismatch - When the issuer annotations are defined, +// it must match the issuer ref. +func SecretCertificateNameAnnotationsMismatch(input Input) (string, string, bool) { + name, ok := input.Secret.Annotations[cmapi.CertificateNameKey] + if (ok) && // only check if an annotation is present + name != input.Certificate.Name { + return IncorrectCertificate, fmt.Sprintf("Issuing certificate as Secret was previously issued for %q", name), true + } + return "", "", false +} + // SecretPublicKeyDiffersFromCurrentCertificateRequest checks that the current CertificateRequest // contains a CSR that is signed by the key stored in the Secret. A failure is often caused by the // Secret being changed outside of the control of cert-manager, causing the current CertificateRequest diff --git a/internal/controller/certificates/policies/constants.go b/internal/controller/certificates/policies/constants.go index c33692745..dea2c7bb8 100644 --- a/internal/controller/certificates/policies/constants.go +++ b/internal/controller/certificates/policies/constants.go @@ -39,6 +39,9 @@ const ( // IncorrectIssuer is a policy violation reason for a scenario where // Certificate has been issued by incorrect Issuer. IncorrectIssuer string = "IncorrectIssuer" + // IncorrectCertificate is a policy violation reason for a scenario where + // Certificate has been issued by incorrect Certificate. + IncorrectCertificate string = "IncorrectCertificate" // RequestChanged is a policy violation reason for a scenario where // CertificateRequest not valid for Certificate's spec. RequestChanged string = "RequestChanged" diff --git a/internal/controller/certificates/policies/policies.go b/internal/controller/certificates/policies/policies.go index 15caa75b7..217ca095c 100644 --- a/internal/controller/certificates/policies/policies.go +++ b/internal/controller/certificates/policies/policies.go @@ -71,7 +71,8 @@ func NewTriggerPolicyChain(c clock.Clock) Chain { SecretIsMissingData, // Make sure the Secret has the required keys set SecretPublicKeysDiffer, // Make sure the PrivateKey and PublicKey match in the Secret - SecretIssuerAnnotationsMismatch, // Make sure the Secret's IssuerRef annotations match the Certificate spec + SecretIssuerAnnotationsMismatch, // Make sure the Secret's IssuerRef annotations match the Certificate spec + SecretCertificateNameAnnotationsMismatch, // Make sure the Secret's CertificateName annotation matches the Certificate's name SecretPrivateKeyMismatchesSpec, // Make sure the PrivateKey Type and Size match the Certificate spec SecretPublicKeyDiffersFromCurrentCertificateRequest, // Make sure the Secret's PublicKey matches the current CertificateRequest @@ -88,7 +89,8 @@ func NewReadinessPolicyChain(c clock.Clock) Chain { SecretIsMissingData, // Make sure the Secret has the required keys set SecretPublicKeysDiffer, // Make sure the PrivateKey and PublicKey match in the Secret - SecretIssuerAnnotationsMismatch, // Make sure the Secret's IssuerRef annotations match the Certificate spec + SecretIssuerAnnotationsMismatch, // Make sure the Secret's IssuerRef annotations match the Certificate spec + SecretCertificateNameAnnotationsMismatch, // Make sure the Secret's CertificateName annotation matches the Certificate's name SecretPrivateKeyMismatchesSpec, // Make sure the PrivateKey Type and Size match the Certificate spec SecretPublicKeyDiffersFromCurrentCertificateRequest, // Make sure the Secret's PublicKey matches the current CertificateRequest diff --git a/pkg/controller/certificates/trigger/trigger_controller.go b/pkg/controller/certificates/trigger/trigger_controller.go index 988ec9990..161d09354 100644 --- a/pkg/controller/certificates/trigger/trigger_controller.go +++ b/pkg/controller/certificates/trigger/trigger_controller.go @@ -160,6 +160,19 @@ func (c *controller) ProcessItem(ctx context.Context, key string) error { return nil } + // If we detect that the Certificate has a duplicate Secret name set, we don't trigger issuance. + isOwner, duplicates, err := internalcertificates.CertificateOwnsSecret(ctx, c.certificateLister, c.secretLister, crt) + if err != nil { + return err + } + if !isOwner { + log.V(logf.DebugLevel).Info("Certificate has duplicate Secret name with other Certificates in the same namespace, skipping trigger.", "duplicates", duplicates) + + c.scheduledWorkQueue.Add(key, 3*time.Minute) + + return nil + } + input, err := c.dataForCertificate(ctx, crt) if err != nil { return err