Ensure external certs have a hash

pkg/controller/apiserver/apiserver_controller.go

@@ -360,7 +360,7 @@ func (r *ReconcileAPIServer) Reconcile(ctx context.Context, request reconcile.Re
 			r.status.SetDegraded(operatorv1.ResourceReadError, "Failed to get certificate", err, reqLogger)
 			return reconcile.Result{}, err
 		} else if prometheusCertificate != nil {
-			trustedBundle = certificatemanagement.CreateTrustedBundle(prometheusCertificate)
+			trustedBundle = certificateManager.CreateTrustedBundle(prometheusCertificate)
 		}
 	}
 

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,19 +561,19 @@ 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.
 // 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 (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) {

pkg/controller/certificatemanager/certificatemanager_test.go

@@ -671,6 +671,39 @@ var _ = Describe("Test CertificateManagement suite", func() {
 				},
 			}))
 		})
+
+		It("should create a hash for both secrets even if the same pem is used twice", func() {
+			By("creating 2 secrets with identical pem in the datastore", func() {
+
+				byoSecretCopy := byoSecret.DeepCopyObject().(*corev1.Secret)
+
+				byoSecret.Name, byoSecret.Namespace = "byo-secret", common.OperatorNamespace()
+				Expect(cli.Create(ctx, byoSecret)).NotTo(HaveOccurred())
+
+				byoSecretCopy.Name, byoSecretCopy.Namespace = "byo-secret-copy", common.OperatorNamespace()
+				Expect(cli.Create(ctx, byoSecretCopy)).NotTo(HaveOccurred())
+			})
+
+			var trustedBundle certificatemanagement.TrustedBundle
+			By("creating the bundle from the certificates", func() {
+				byo, err := certificateManager.GetCertificate(cli, "byo-secret", common.OperatorNamespace())
+				Expect(err).NotTo(HaveOccurred())
+				byoCopy, err := certificateManager.GetCertificate(cli, "byo-secret-copy", common.OperatorNamespace())
+				Expect(err).NotTo(HaveOccurred())
+				Expect(byo.GetIssuer()).NotTo(Equal(certificateManager.KeyPair()))
+				Expect(byoCopy.GetIssuer()).NotTo(Equal(certificateManager.KeyPair()))
+				trustedBundle = certificateManager.CreateTrustedBundle(byo, byoCopy)
+			})
+
+			By("verifying the hash annotations and pem blocks of the bundle", func() {
+				bundle := trustedBundle.ConfigMap("").Data[certificatemanagement.TrustedCertConfigMapKeyName]
+				numBlocks := strings.Count(bundle, "certificate name:")
+				Expect(numBlocks).To(Equal(3))
+				Expect(trustedBundle.HashAnnotations()).To(HaveKey("tigera-operator.hash.operator.tigera.io/tigera-ca-private"))
+				Expect(trustedBundle.HashAnnotations()).To(HaveKey("tigera-operator.hash.operator.tigera.io/byo-secret-copy"))
+				Expect(trustedBundle.HashAnnotations()).To(HaveKey("tigera-operator.hash.operator.tigera.io/byo-secret"))
+			})
+		})
 	})
 })
 

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{

pkg/tls/certificatemanagement/certificatebundle.go

@@ -19,7 +19,6 @@ import (
 	"fmt"
 	"os"
 	"path"
-	"sort"
 
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -41,15 +40,19 @@ type trustedBundle struct {
 	name string
 	// systemCertificates is a bundle of certificates loaded from the host systems root location.
 	systemCertificates []byte
+
+	// This is the CA that signs new certificates that are being created.
+	ca CertificateInterface
+
 	// certificates is a map of key: hash, value: certificate.
-	certificates map[string]CertificateInterface
+	certificates []CertificateInterface
 }
 
 // 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(ca CertificateInterface, certificates ...CertificateInterface) TrustedBundle {
+	bundle, err := createTrustedBundle(false, TrustedCertConfigMapName, ca, certificates...)
 	if err != nil {
 		panic(err) // This should never happen.
 	}
@@ -60,18 +63,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(ca CertificateInterface, certificates ...CertificateInterface) (TrustedBundle, error) {
+	return createTrustedBundle(true, TrustedCertConfigMapName, ca, 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(ca CertificateInterface, certificates ...CertificateInterface) (TrustedBundle, error) {
+	return createTrustedBundle(true, TrustedCertConfigMapNamePublic, ca, 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, ca CertificateInterface, certificates ...CertificateInterface) (TrustedBundle, error) {
 	var systemCertificates []byte
 	var err error
 	if includeSystemBundle {
@@ -83,8 +86,12 @@ func createTrustedBundle(includeSystemBundle bool, name string, certificates ...
 
 	bundle := &trustedBundle{
 		name:               name,
+		ca:                 ca,
 		systemCertificates: systemCertificates,
-		certificates:       make(map[string]CertificateInterface),
+		certificates:       []CertificateInterface{},
+	}
+	if ca != nil {
+		bundle.certificates = append(bundle.certificates, ca)
 	}
 	bundle.AddCertificates(certificates...)
 
@@ -94,21 +101,13 @@ 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 {
+			// Only if a certificate was not signed by our operator, we should add it to the bundle.
+			if cert.GetIssuer() != nil && t.ca != nil &&
+				string(cert.GetIssuer().GetCertificatePEM()) == string(t.ca.GetCertificatePEM()) {
+				continue
 			}
-		}
-		if cert != nil && !skip {
-			// Add the leaf certificate
-			hash := rmeta.AnnotationHash(cert.GetCertificatePEM())
-			t.certificates[hash] = cert
+			t.certificates = append(t.certificates, cert)
 		}
 	}
 }
@@ -119,8 +118,8 @@ func (t *trustedBundle) MountPath() string {
 
 func (t *trustedBundle) HashAnnotations() map[string]string {
 	annotations := make(map[string]string)
-	for hash, cert := range t.certificates {
-		annotations[fmt.Sprintf("%s.hash.operator.tigera.io/%s", cert.GetNamespace(), cert.GetName())] = hash
+	for _, cert := range t.certificates {
+		annotations[fmt.Sprintf("%s.hash.operator.tigera.io/%s", cert.GetNamespace(), cert.GetName())] = rmeta.AnnotationHash(cert.GetCertificatePEM())
 	}
 	if len(t.systemCertificates) > 0 {
 		annotations["hash.operator.tigera.io/system"] = rmeta.AnnotationHash(t.systemCertificates)
@@ -171,17 +170,7 @@ func (t *trustedBundle) Volume() corev1.Volume {
 
 func (t *trustedBundle) ConfigMap(namespace string) *corev1.ConfigMap {
 	pemBuf := bytes.Buffer{}
-
-	// Sort the certificates so that we get a consistent ordering.
-	// This reduces the number of changes we see in the configmap.
-	certs := []CertificateInterface{}
 	for _, cert := range t.certificates {
-		certs = append(certs, cert)
-	}
-	sort.Slice(certs, func(i, j int) bool {
-		return certs[i].GetName() < certs[j].GetName()
-	})
-	for _, cert := range certs {
 		pemBuf.WriteString(fmt.Sprintf("# certificate name: %s/%s\n%s\n\n", cert.GetNamespace(), cert.GetName(), string(cert.GetCertificatePEM())))
 	}
 
@@ -249,12 +238,13 @@ var certFiles = []string{
 func getSystemCertificates() ([]byte, error) {
 	for _, filename := range certFiles {
 		data, err := os.ReadFile(filename)
-		if err == nil {
+		if err != nil {
+			if !os.IsNotExist(err) {
+				return nil, fmt.Errorf(fmt.Sprintf("error occurred when loading system root certificate with name %s", filename), err)
+			}
+		} else {
 			return data, nil
 		}
-		if err != nil && !os.IsNotExist(err) {
-			return nil, fmt.Errorf(fmt.Sprintf("error occurred when loading system root certificate with name %s", filename), err)
-		}
 	}
 	return nil, nil
 }