From 3e6889b21b4adb1fca66550e41f03d485d5d3a74 Mon Sep 17 00:00:00 2001 From: rene-dekker Date: Thu, 10 Oct 2024 09:16:17 -0700 Subject: [PATCH] If a certificate is external, we must ensure that there is a hash for them. There can be a case that a user uses the same certs for two different components. We want to avoid that components get re-rendered due to having different hashes per reconciliation loop. --- .../certificatemanager/certificatemanager.go | 8 ++-- pkg/render/apiserver_test.go | 3 +- .../certificatebundle.go | 38 ++++++++----------- 3 files changed, 22 insertions(+), 27 deletions(-) diff --git a/pkg/controller/certificatemanager/certificatemanager.go b/pkg/controller/certificatemanager/certificatemanager.go index 0987ef3315..d322df1f3d 100644 --- a/pkg/controller/certificatemanager/certificatemanager.go +++ b/pkg/controller/certificatemanager/certificatemanager.go @@ -329,7 +329,7 @@ func (cm *certificateManager) GetOrCreateKeyPair(cli client.Client, secretName, cm.log.V(3).Info("secret %s has invalid DNS names, the expected names are: %v", secretName, dnsNames) return keyPair, nil } - } else if keyPair == nil { + } else { cm.log.V(1).Info("Keypair wasn't found, create a new one", "namespace", secretNamespace, "name", secretName) } @@ -561,7 +561,7 @@ func HasExpectedDNSNames(secretName, secretNamespace string, cert *x509.Certific // It will include: // - A bundle with Calico's root certificates + any user supplied certificates in /etc/pki/tls/certs/tigera-ca-bundle.crt. func (cm *certificateManager) CreateTrustedBundle(certificates ...certificatemanagement.CertificateInterface) certificatemanagement.TrustedBundle { - return certificatemanagement.CreateTrustedBundle(append([]certificatemanagement.CertificateInterface{cm.keyPair}, certificates...)...) + return certificatemanagement.CreateTrustedBundle(cm.keyPair, certificates...) } // CreateTrustedBundleWithSystemRootCertificates creates a TrustedBundle, which provides standardized methods for mounting a bundle of certificates to trust. @@ -569,11 +569,11 @@ func (cm *certificateManager) CreateTrustedBundle(certificates ...certificateman // - A bundle with Calico's root certificates + any user supplied certificates in /etc/pki/tls/certs/tigera-ca-bundle.crt. // - A system root certificate bundle in /etc/pki/tls/certs/ca-bundle.crt. func (cm *certificateManager) CreateTrustedBundleWithSystemRootCertificates(certificates ...certificatemanagement.CertificateInterface) (certificatemanagement.TrustedBundle, error) { - return certificatemanagement.CreateTrustedBundleWithSystemRootCertificates(append([]certificatemanagement.CertificateInterface{cm.keyPair}, certificates...)...) + return certificatemanagement.CreateTrustedBundleWithSystemRootCertificates(cm.keyPair, certificates...) } func (cm *certificateManager) CreateMultiTenantTrustedBundleWithSystemRootCertificates(certificates ...certificatemanagement.CertificateInterface) (certificatemanagement.TrustedBundle, error) { - return certificatemanagement.CreateMultiTenantTrustedBundleWithSystemRootCertificates(append([]certificatemanagement.CertificateInterface{cm.keyPair}, certificates...)...) + return certificatemanagement.CreateMultiTenantTrustedBundleWithSystemRootCertificates(cm.keyPair, certificates...) } func (cm *certificateManager) LoadTrustedBundle(ctx context.Context, client client.Client, ns string) (certificatemanagement.TrustedBundleRO, error) { diff --git a/pkg/render/apiserver_test.go b/pkg/render/apiserver_test.go index 69f8e6a0cd..bcc1286772 100644 --- a/pkg/render/apiserver_test.go +++ b/pkg/render/apiserver_test.go @@ -29,6 +29,7 @@ import ( "github.com/openshift/library-go/pkg/crypto" calicov3 "github.com/tigera/api/pkg/apis/projectcalico/v3" + operatorv1 "github.com/tigera/operator/api/v1" "github.com/tigera/operator/pkg/apis" "github.com/tigera/operator/pkg/common" @@ -93,7 +94,7 @@ var _ = Describe("API server rendering tests (Calico Enterprise)", func() { kp, err := certificateManager.GetOrCreateKeyPair(cli, render.ProjectCalicoAPIServerTLSSecretName(instance.Variant), common.OperatorNamespace(), dnsNames) Expect(err).NotTo(HaveOccurred()) - trustedBundle = certificatemanagement.CreateTrustedBundle() + trustedBundle = certificatemanagement.CreateTrustedBundle(nil) replicas = 2 cfg = &render.APIServerConfiguration{ diff --git a/pkg/tls/certificatemanagement/certificatebundle.go b/pkg/tls/certificatemanagement/certificatebundle.go index 77085a48d8..7e21fa2cc2 100644 --- a/pkg/tls/certificatemanagement/certificatebundle.go +++ b/pkg/tls/certificatemanagement/certificatebundle.go @@ -48,8 +48,8 @@ type trustedBundle struct { // CreateTrustedBundle creates a TrustedBundle, which provides standardized methods for mounting a bundle of certificates to trust. // It will include: // - A bundle with Calico's root certificates + any user supplied certificates in /etc/pki/tls/certs/tigera-ca-bundle.crt. -func CreateTrustedBundle(certificates ...CertificateInterface) TrustedBundle { - bundle, err := createTrustedBundle(false, TrustedCertConfigMapName, certificates...) +func CreateTrustedBundle(rootCA CertificateInterface, certificates ...CertificateInterface) TrustedBundle { + bundle, err := createTrustedBundle(false, TrustedCertConfigMapName, rootCA, certificates...) if err != nil { panic(err) // This should never happen. } @@ -60,18 +60,18 @@ func CreateTrustedBundle(certificates ...CertificateInterface) TrustedBundle { // It will include: // - A bundle with Calico's root certificates + any user supplied certificates in /etc/pki/tls/certs/tigera-ca-bundle.crt. // - A system root certificate bundle in /etc/pki/tls/certs/ca-bundle.crt. -func CreateTrustedBundleWithSystemRootCertificates(certificates ...CertificateInterface) (TrustedBundle, error) { - return createTrustedBundle(true, TrustedCertConfigMapName, certificates...) +func CreateTrustedBundleWithSystemRootCertificates(rootCA CertificateInterface, certificates ...CertificateInterface) (TrustedBundle, error) { + return createTrustedBundle(true, TrustedCertConfigMapName, rootCA, certificates...) } // CreateMultiTenantTrustedBundleWithSystemRootCertificates creates a TrustedBundle with system root certificates that is // appropraite for a multi-tenant cluster, in which each tenant needs multiple trusted bundles. -func CreateMultiTenantTrustedBundleWithSystemRootCertificates(certificates ...CertificateInterface) (TrustedBundle, error) { - return createTrustedBundle(true, TrustedCertConfigMapNamePublic, certificates...) +func CreateMultiTenantTrustedBundleWithSystemRootCertificates(rootCA CertificateInterface, certificates ...CertificateInterface) (TrustedBundle, error) { + return createTrustedBundle(true, TrustedCertConfigMapNamePublic, rootCA, certificates...) } // createTrustedBundle creates a TrustedBundle, which provides standardized methods for mounting a bundle of certificates to trust. -func createTrustedBundle(includeSystemBundle bool, name string, certificates ...CertificateInterface) (TrustedBundle, error) { +func createTrustedBundle(includeSystemBundle bool, name string, rootCA CertificateInterface, certificates ...CertificateInterface) (TrustedBundle, error) { var systemCertificates []byte var err error if includeSystemBundle { @@ -81,10 +81,14 @@ func createTrustedBundle(includeSystemBundle bool, name string, certificates ... } } + certMap := make(map[string]CertificateInterface) + if rootCA != nil { + certMap[rmeta.AnnotationHash(rootCA.GetCertificatePEM())] = rootCA + } bundle := &trustedBundle{ name: name, systemCertificates: systemCertificates, - certificates: make(map[string]CertificateInterface), + certificates: certMap, } bundle.AddCertificates(certificates...) @@ -94,19 +98,9 @@ func createTrustedBundle(includeSystemBundle bool, name string, certificates ... // AddCertificates Adds the certificates to the bundle. func (t *trustedBundle) AddCertificates(certificates ...CertificateInterface) { for _, cert := range certificates { - // Check if we already trust an issuer of this cert. In practice, this will be 0 or 1 iteration, - // because the issuer is only set when the tigera-ca-private is the issuer. - cur := cert - var skip bool - for cur != nil && !skip { - hash := rmeta.AnnotationHash(cur.GetCertificatePEM()) - cur = cur.GetIssuer() - if _, found := t.certificates[hash]; found { - skip = true - } - } - if cert != nil && !skip { - // Add the leaf certificate + // cert.GetIssuer() is set only for certificates that are signed by our operator CA. + // If a certificate was not signed by our operator, we should add it to the bundle. + if cert != nil && cert.GetIssuer() == nil { hash := rmeta.AnnotationHash(cert.GetCertificatePEM()) t.certificates[hash] = cert } @@ -252,7 +246,7 @@ func getSystemCertificates() ([]byte, error) { if err == nil { return data, nil } - if err != nil && !os.IsNotExist(err) { + if !os.IsNotExist(err) { return nil, fmt.Errorf(fmt.Sprintf("error occurred when loading system root certificate with name %s", filename), err) } }