We should create a hash for both byo secrets even if the same pem is …

…used twice.

pkg/controller/certificatemanager/certificatemanager_test.go

@@ -671,6 +671,43 @@ 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:")
+				// While we have the ca + 4 certs, we expect 3 cert blocks (+ 2):
+				// - the certificateManager: this covers for all certs signed by the tigera root ca
+				// - the byo block (+ its ca block)
+				// - the legacy cert that already has ExtKeyUsageClient configured.
+				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/tls/certificatemanagement/certificatebundle.go

@@ -41,15 +41,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(rootCA CertificateInterface, certificates ...CertificateInterface) TrustedBundle {
-	bundle, err := createTrustedBundle(false, TrustedCertConfigMapName, rootCA, 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 +64,18 @@ func CreateTrustedBundle(rootCA CertificateInterface, certificates ...Certificat
 // 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(rootCA CertificateInterface, certificates ...CertificateInterface) (TrustedBundle, error) {
-	return createTrustedBundle(true, TrustedCertConfigMapName, rootCA, 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(rootCA CertificateInterface, certificates ...CertificateInterface) (TrustedBundle, error) {
-	return createTrustedBundle(true, TrustedCertConfigMapNamePublic, rootCA, 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, rootCA CertificateInterface, certificates ...CertificateInterface) (TrustedBundle, error) {
+func createTrustedBundle(includeSystemBundle bool, name string, ca CertificateInterface, certificates ...CertificateInterface) (TrustedBundle, error) {
 	var systemCertificates []byte
 	var err error
 	if includeSystemBundle {
@@ -82,13 +86,14 @@ func createTrustedBundle(includeSystemBundle bool, name string, rootCA Certifica
 	}
 
 	certMap := make(map[string]CertificateInterface)
-	if rootCA != nil {
-		certMap[rmeta.AnnotationHash(rootCA.GetCertificatePEM())] = rootCA
+	if ca != nil {
+		certMap[rmeta.AnnotationHash(ca.GetCertificatePEM())] = ca
 	}
 	bundle := &trustedBundle{
 		name:               name,
+		ca:                 ca,
 		systemCertificates: systemCertificates,
-		certificates:       certMap,
+		certificates:       []CertificateInterface{ca},
 	}
 	bundle.AddCertificates(certificates...)
 
@@ -98,11 +103,13 @@ func createTrustedBundle(includeSystemBundle bool, name string, rootCA Certifica
 // AddCertificates Adds the certificates to the bundle.
 func (t *trustedBundle) AddCertificates(certificates ...CertificateInterface) {
 	for _, cert := range certificates {
-		// 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
+		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
+			}
+			t.certificates = append(t.certificates, cert)
 		}
 	}
 }
@@ -113,8 +120,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)