Merge pull request #303 from arsenalzp/main

Improve certificate deduplication operation

pkg/bundle/sync.go

@@ -21,6 +21,7 @@ import (
 	"context"
 	"crypto/sha256"
 	"encoding/hex"
+	"encoding/pem"
 	"errors"
 	"fmt"
 	"strings"
@@ -130,7 +131,12 @@ func (b *bundle) buildSourceBundle(ctx context.Context, bundle *trustapi.Bundle)
 		return bundleData{}, fmt.Errorf("couldn't find any valid certificates in bundle")
 	}
 
-	if err := resolvedBundle.populateData(bundles, bundle.Spec.Target); err != nil {
+	deduplicatedBundles, err := deduplicateBundles(bundles)
+	if err != nil {
+		return bundleData{}, err
+	}
+
+	if err := resolvedBundle.populateData(deduplicatedBundles, bundle.Spec.Target); err != nil {
 		return bundleData{}, err
 	}
 
@@ -783,3 +789,39 @@ func (b *bundle) migrateConfigMapToApply(ctx context.Context, obj client.Object,
 	cm.SetManagedFields(managedFields)
 	return true, b.directClient.Update(ctx, &cm)
 }
+
+// remove duplicate certificates from bundles
+func deduplicateBundles(bundles []string) ([]string, error) {
+	var block *pem.Block
+
+	var certificatesHashes = make(map[[32]byte]struct{})
+	var dedupCerts []string
+
+	for _, cert := range bundles {
+		certBytes := []byte(cert)
+
+	LOOP:
+		for {
+			block, certBytes = pem.Decode([]byte(certBytes))
+			if block == nil {
+				break LOOP
+			}
+
+			if block.Type != "CERTIFICATE" {
+				return nil, fmt.Errorf("couldn't decode PEM block containing certificate")
+			}
+
+			// calculate hash sum of the given certificate
+			hash := sha256.Sum256(block.Bytes)
+			// check existence of the hash
+			if _, ok := certificatesHashes[hash]; !ok {
+				// neew to trim a newline which is added by Encoder
+				dedupCerts = append(dedupCerts, string(bytes.Trim(pem.EncodeToMemory(block), "\n")))
+				certificatesHashes[hash] = struct{}{}
+			}
+		}
+
+	}
+
+	return dedupCerts, nil
+}

pkg/bundle/sync_test.go

@@ -1729,3 +1729,80 @@ func Test_certAlias(t *testing.T) {
 		t.Fatalf("expected alias to be %q but got %q", expectedAlias, alias)
 	}
 }
+
+func TestBundlesDeduplication(t *testing.T) {
+	tests := map[string]struct {
+		name       string
+		bundle     []string
+		testBundle []string
+	}{
+		"single, different cert per source": {
+			bundle: []string{
+				dummy.TestCertificate1,
+				dummy.TestCertificate2,
+			},
+			testBundle: []string{
+				dummy.TestCertificate1,
+				dummy.TestCertificate2,
+			},
+		},
+		"no certs in sources": {
+			bundle:     []string{},
+			testBundle: []string{},
+		},
+		"single cert in the first source, joined certs in the second source": {
+			bundle: []string{
+				dummy.TestCertificate1,
+				dummy.JoinCerts(dummy.TestCertificate1, dummy.TestCertificate3),
+			},
+			testBundle: []string{
+				dummy.TestCertificate1,
+				dummy.TestCertificate3,
+			},
+		},
+		"joined certs in the first source, single cert in the second source": {
+			bundle: []string{
+				dummy.JoinCerts(dummy.TestCertificate1, dummy.TestCertificate3),
+				dummy.TestCertificate1,
+			},
+			testBundle: []string{
+				dummy.TestCertificate3,
+				dummy.TestCertificate1,
+			},
+		},
+		"joined, different certs in the first source; joined,different certs in the second source": {
+			bundle: []string{
+				dummy.JoinCerts(dummy.TestCertificate1, dummy.TestCertificate2),
+				dummy.JoinCerts(dummy.TestCertificate4, dummy.TestCertificate5),
+			},
+			testBundle: []string{
+				dummy.TestCertificate1,
+				dummy.TestCertificate2,
+				dummy.TestCertificate4,
+				dummy.TestCertificate5,
+			},
+		},
+		"all certs are joined ones and equal ones in all sources": {
+			bundle: []string{
+				dummy.JoinCerts(dummy.TestCertificate1, dummy.TestCertificate1),
+				dummy.JoinCerts(dummy.TestCertificate1, dummy.TestCertificate1),
+			},
+			testBundle: []string{
+				dummy.TestCertificate1,
+			},
+		},
+	}
+	for name, test := range tests {
+		test := test
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+
+			resultBundle, err := deduplicateBundles(test.bundle)
+
+			assert.Nil(t, err)
+
+			// check certificates bundle for duplicated certificates
+			assert.ElementsMatch(t, test.testBundle, resultBundle)
+		})
+	}
+}

pkg/util/cert_pool.go

@@ -17,7 +17,6 @@ limitations under the License.
 package util
 
 import (
-	"crypto/sha256"
 	"crypto/x509"
 	"encoding/pem"
 	"fmt"
@@ -26,36 +25,18 @@ import (
 
 // CertPool is a set of certificates.
 type certPool struct {
-	certificatesHashes map[[32]byte]struct{}
-	certificates       []*x509.Certificate
-	filterExpired      bool
+	certificates  []*x509.Certificate
+	filterExpired bool
 }
 
 // newCertPool returns a new, empty CertPool.
 func newCertPool(filterExpired bool) *certPool {
 	return &certPool{
-		certificatesHashes: make(map[[32]byte]struct{}),
-		certificates:       make([]*x509.Certificate, 0),
-		filterExpired:      filterExpired,
+		certificates:  make([]*x509.Certificate, 0),
+		filterExpired: filterExpired,
 	}
 }
 
-// Check if the given certificate was added to certificates bundle already
-func (cp *certPool) isCertificateDuplicate(certData []byte) bool {
-	// calculate hash sum of the given certificate
-	hash := sha256.Sum256(certData)
-
-	// check a hash existence
-	if _, ok := cp.certificatesHashes[hash]; ok {
-		return ok
-	}
-
-	// put certificate hash into a set of hashes
-	cp.certificatesHashes[hash] = struct{}{}
-
-	return false
-}
-
 // Append certificate to a pool
 func (cp *certPool) appendCertFromPEM(PEMdata []byte) error {
 	if PEMdata == nil {
@@ -94,9 +75,7 @@ func (cp *certPool) appendCertFromPEM(PEMdata []byte) error {
 			continue
 		}
 
-		if !cp.isCertificateDuplicate(block.Bytes) {
-			cp.certificates = append(cp.certificates, certificate)
-		}
+		cp.certificates = append(cp.certificates, certificate)
 	}
 
 	return nil

pkg/util/cert_pool_test.go

@@ -30,69 +30,6 @@ func TestNewCertPool(t *testing.T) {
 	}
 }
 
-func TestCertificatesDeduplication(t *testing.T) {
-	// create a pool
-	certPool := newCertPool(false)
-
-	// list of certificates
-	certificateList := [...]struct {
-		certificateName string
-		certificate     string
-	}{
-		{
-			"TestCertificate3Duplicate", // this certificate is duplicate of TestCertificate3
-			dummy.TestCertificate3Duplicate,
-		},
-		{
-			"TestCertificate1",
-			dummy.TestCertificate1,
-		},
-		{
-			"TestCertificate2",
-			dummy.TestCertificate2,
-		},
-		{
-			"TestCertificate3",
-			dummy.TestCertificate3,
-		},
-		{
-			"TestCertificate5Duplicate", // this certificate is duplicate of TestCertificate5
-			dummy.TestCertificate5Duplicate,
-		},
-		{
-			"TestCertificate4",
-			dummy.TestCertificate4,
-		},
-		{
-			"TestCertificate5",
-			dummy.TestCertificate5,
-		},
-	}
-
-	// certificates bundle structure
-	certificateBundle := []struct {
-		certificateName string
-		certificate     string
-	}{}
-
-	// populate certificates bundle
-	for _, crt := range certificateList {
-		if !certPool.isCertificateDuplicate([]byte(crt.certificate)) {
-			certificateBundle = append(certificateBundle, crt)
-		}
-	}
-
-	// create a new pool
-	newCertPool := newCertPool(false)
-
-	// check certificates bundle for duplicated certificates
-	for _, crt := range certificateBundle {
-		if newCertPool.isCertificateDuplicate([]byte(crt.certificate)) {
-			t.Errorf("duplicate certificate found %s\n", crt.certificateName)
-		}
-	}
-}
-
 func TestAppendCertFromPEM(t *testing.T) {
 	// list of certificates
 	certificateList := [...]struct {