Sort certificates in bundles to ensure deterministic behaviour

pkg/bundle/bundle_test.go

@@ -1007,9 +1007,9 @@ func Test_Reconcile(t *testing.T) {
 			expResult:               ctrl.Result{},
 			expError:                false,
 			expPatches: []interface{}{
-				configMapPatch(baseBundle.Name, trustNamespace, map[string]string{targetKey: dummy.JoinCerts(dummy.TestCertificate1, dummy.TestCertificate2, dummy.TestCertificate3, dummy.TestCertificate5)}, nil, ptr.To(targetKey)),
-				configMapPatch(baseBundle.Name, "ns-1", map[string]string{targetKey: dummy.JoinCerts(dummy.TestCertificate1, dummy.TestCertificate2, dummy.TestCertificate3, dummy.TestCertificate5)}, nil, ptr.To(targetKey)),
-				configMapPatch(baseBundle.Name, "ns-2", map[string]string{targetKey: dummy.JoinCerts(dummy.TestCertificate1, dummy.TestCertificate2, dummy.TestCertificate3, dummy.TestCertificate5)}, nil, ptr.To(targetKey)),
+				configMapPatch(baseBundle.Name, trustNamespace, map[string]string{targetKey: dummy.JoinCerts(dummy.TestCertificate2, dummy.TestCertificate1, dummy.TestCertificate3, dummy.TestCertificate5)}, nil, ptr.To(targetKey)),
+				configMapPatch(baseBundle.Name, "ns-1", map[string]string{targetKey: dummy.JoinCerts(dummy.TestCertificate2, dummy.TestCertificate1, dummy.TestCertificate3, dummy.TestCertificate5)}, nil, ptr.To(targetKey)),
+				configMapPatch(baseBundle.Name, "ns-2", map[string]string{targetKey: dummy.JoinCerts(dummy.TestCertificate2, dummy.TestCertificate1, dummy.TestCertificate3, dummy.TestCertificate5)}, nil, ptr.To(targetKey)),
 			},
 			expBundlePatch: &trustapi.BundleStatus{
 				Conditions: []trustapi.BundleCondition{
@@ -1266,9 +1266,9 @@ func Test_Reconcile(t *testing.T) {
 			expResult: ctrl.Result{},
 			expError:  false,
 			expPatches: []interface{}{
-				configMapPatch(baseBundle.Name, trustNamespace, map[string]string{targetKey: dummy.JoinCerts(dummy.TestCertificate1, dummy.TestCertificate2, dummy.TestCertificate3)}, nil, ptr.To(targetKey)),
-				configMapPatch(baseBundle.Name, "ns-1", map[string]string{targetKey: dummy.JoinCerts(dummy.TestCertificate1, dummy.TestCertificate2, dummy.TestCertificate3)}, nil, ptr.To(targetKey)),
-				configMapPatch(baseBundle.Name, "ns-2", map[string]string{targetKey: dummy.JoinCerts(dummy.TestCertificate1, dummy.TestCertificate2, dummy.TestCertificate3)}, nil, ptr.To(targetKey)),
+				configMapPatch(baseBundle.Name, trustNamespace, map[string]string{targetKey: dummy.JoinCerts(dummy.TestCertificate2, dummy.TestCertificate1, dummy.TestCertificate3)}, nil, ptr.To(targetKey)),
+				configMapPatch(baseBundle.Name, "ns-1", map[string]string{targetKey: dummy.JoinCerts(dummy.TestCertificate2, dummy.TestCertificate1, dummy.TestCertificate3)}, nil, ptr.To(targetKey)),
+				configMapPatch(baseBundle.Name, "ns-2", map[string]string{targetKey: dummy.JoinCerts(dummy.TestCertificate2, dummy.TestCertificate1, dummy.TestCertificate3)}, nil, ptr.To(targetKey)),
 			},
 		},
 	}

pkg/bundle/source.go

@@ -23,6 +23,7 @@ import (
 	"encoding/hex"
 	"encoding/pem"
 	"fmt"
+	"slices"
 	"strings"
 
 	jks "github.com/pavlo-v-chernykh/keystore-go/v4"
@@ -114,7 +115,7 @@ func (b *bundle) buildSourceBundle(ctx context.Context, sources []trustapi.Bundl
 		return bundleData{}, fmt.Errorf("couldn't find any valid certificates in bundle")
 	}
 
-	deduplicatedBundles, err := deduplicateBundles(bundles)
+	deduplicatedBundles, err := deduplicateAndSortBundles(bundles)
 	if err != nil {
 		return bundleData{}, err
 	}
@@ -343,21 +344,19 @@ func (b *bundleData) populateData(bundles []string, formats *trustapi.Additional
 	return nil
 }
 
-// remove duplicate certificates from bundles
-func deduplicateBundles(bundles []string) ([]string, error) {
+// remove duplicate certificates from bundles and sort certificates by hash
+func deduplicateAndSortBundles(bundles []string) ([]string, error) {
 	var block *pem.Block
 
-	var certificatesHashes = make(map[[32]byte]struct{})
-	var dedupCerts []string
+	var certificatesHashes = make(map[[32]byte]string)
 
 	for _, cert := range bundles {
 		certBytes := []byte(cert)
 
-	LOOP:
 		for {
 			block, certBytes = pem.Decode(certBytes)
 			if block == nil {
-				break LOOP
+				break
 			}
 
 			if block.Type != "CERTIFICATE" {
@@ -369,12 +368,23 @@ func deduplicateBundles(bundles []string) ([]string, error) {
 			// 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{}{}
+				certificatesHashes[hash] = string(bytes.Trim(pem.EncodeToMemory(block), "\n"))
 			}
 		}
+	}
+
+	var orderedKeys [][32]byte
+	for key := range certificatesHashes {
+		orderedKeys = append(orderedKeys, key)
+	}
+	slices.SortFunc(orderedKeys, func(a, b [32]byte) int {
+		return bytes.Compare(a[:], b[:])
+	})
 
+	var sortedDeduplicatedCerts []string
+	for _, key := range orderedKeys {
+		sortedDeduplicatedCerts = append(sortedDeduplicatedCerts, certificatesHashes[key])
 	}
 
-	return dedupCerts, nil
+	return sortedDeduplicatedCerts, nil
 }

pkg/bundle/source_test.go

@@ -61,7 +61,7 @@ func Test_buildSourceBundle(t *testing.T) {
 				{InLine: ptr.To(dummy.TestCertificate1 + "\n" + dummy.TestCertificate2 + "\n\n")},
 			},
 			objects:          []runtime.Object{},
-			expData:          dummy.JoinCerts(dummy.TestCertificate1, dummy.TestCertificate2),
+			expData:          dummy.JoinCerts(dummy.TestCertificate2, dummy.TestCertificate1),
 			expError:         false,
 			expNotFoundError: false,
 		},
@@ -98,7 +98,20 @@ func Test_buildSourceBundle(t *testing.T) {
 				ObjectMeta: metav1.ObjectMeta{Name: "configmap"},
 				Data:       map[string]string{"key": dummy.TestCertificate1 + "\n" + dummy.TestCertificate2},
 			}},
-			expData:          dummy.JoinCerts(dummy.TestCertificate1, dummy.TestCertificate2),
+			expData:          dummy.JoinCerts(dummy.TestCertificate2, dummy.TestCertificate1),
+			expError:         false,
+			expNotFoundError: false,
+		},
+		"if single ConfigMap source, return data even when order changes": {
+			// Test uses the same data as the previous one but with different order
+			sources: []trustapi.BundleSource{
+				{ConfigMap: &trustapi.SourceObjectKeySelector{Name: "configmap", KeySelector: trustapi.KeySelector{Key: "key"}}},
+			},
+			objects: []runtime.Object{&corev1.ConfigMap{
+				ObjectMeta: metav1.ObjectMeta{Name: "configmap"},
+				Data:       map[string]string{"key": dummy.TestCertificate2 + "\n" + dummy.TestCertificate1},
+			}},
+			expData:          dummy.JoinCerts(dummy.TestCertificate2, dummy.TestCertificate1),
 			expError:         false,
 			expNotFoundError: false,
 		},
@@ -111,7 +124,7 @@ func Test_buildSourceBundle(t *testing.T) {
 				ObjectMeta: metav1.ObjectMeta{Name: "configmap"},
 				Data:       map[string]string{"key": dummy.TestCertificate1},
 			}},
-			expData:          dummy.JoinCerts(dummy.TestCertificate1, dummy.TestCertificate2),
+			expData:          dummy.JoinCerts(dummy.TestCertificate2, dummy.TestCertificate1),
 			expError:         false,
 			expNotFoundError: false,
 		},
@@ -141,7 +154,7 @@ func Test_buildSourceBundle(t *testing.T) {
 				ObjectMeta: metav1.ObjectMeta{Name: "secret"},
 				Data:       map[string][]byte{"key": []byte(dummy.TestCertificate1 + "\n" + dummy.TestCertificate2)},
 			}},
-			expData:          dummy.JoinCerts(dummy.TestCertificate1, dummy.TestCertificate2),
+			expData:          dummy.JoinCerts(dummy.TestCertificate2, dummy.TestCertificate1),
 			expError:         false,
 			expNotFoundError: false,
 		},
@@ -174,7 +187,7 @@ func Test_buildSourceBundle(t *testing.T) {
 					Data:       map[string][]byte{"key": []byte(dummy.TestCertificate2)},
 				},
 			},
-			expData:          dummy.JoinCerts(dummy.TestCertificate1, dummy.TestCertificate3, dummy.TestCertificate2),
+			expData:          dummy.JoinCerts(dummy.TestCertificate2, dummy.TestCertificate1, dummy.TestCertificate3),
 			expError:         false,
 			expNotFoundError: false,
 		},
@@ -444,13 +457,13 @@ func TestBundlesDeduplication(t *testing.T) {
 				dummy.TestCertificate2,
 			},
 			testBundle: []string{
-				dummy.TestCertificate1,
 				dummy.TestCertificate2,
+				dummy.TestCertificate1,
 			},
 		},
 		"no certs in sources": {
 			bundle:     []string{},
-			testBundle: []string{},
+			testBundle: nil,
 		},
 		"single cert in the first source, joined certs in the second source": {
 			bundle: []string{
@@ -468,8 +481,8 @@ func TestBundlesDeduplication(t *testing.T) {
 				dummy.TestCertificate1,
 			},
 			testBundle: []string{
-				dummy.TestCertificate3,
 				dummy.TestCertificate1,
+				dummy.TestCertificate3,
 			},
 		},
 		"joined, different certs in the first source; joined,different certs in the second source": {
@@ -478,8 +491,8 @@ func TestBundlesDeduplication(t *testing.T) {
 				dummy.JoinCerts(dummy.TestCertificate4, dummy.TestCertificate5),
 			},
 			testBundle: []string{
-				dummy.TestCertificate1,
 				dummy.TestCertificate2,
+				dummy.TestCertificate1,
 				dummy.TestCertificate4,
 				dummy.TestCertificate5,
 			},
@@ -499,12 +512,12 @@ func TestBundlesDeduplication(t *testing.T) {
 		t.Run(name, func(t *testing.T) {
 			t.Parallel()
 
-			resultBundle, err := deduplicateBundles(test.bundle)
+			resultBundle, err := deduplicateAndSortBundles(test.bundle)
 
 			assert.Nil(t, err)
 
 			// check certificates bundle for duplicated certificates
-			assert.ElementsMatch(t, test.testBundle, resultBundle)
+			assert.Equal(t, test.testBundle, resultBundle)
 		})
 	}
 }

test/dummy/certificates.go

@@ -482,8 +482,8 @@ z40l74JcR+GvcFZWz7/jmJq95YMZ7LawLAr1CaAXxCwsoLbJpbgg4lVo6odACzY=
 
 func DefaultJoinedCerts() string {
 	return JoinCerts(
-		TestCertificate1,
 		TestCertificate2,
+		TestCertificate1,
 		TestCertificate3,
 	)
 }

test/env/data.go

@@ -19,6 +19,7 @@ package env
 import (
 	"bytes"
 	"context"
+	"encoding/pem"
 	"fmt"
 	"strings"
 
@@ -209,21 +210,49 @@ func CheckBundleSynced(ctx context.Context, cl client.Client, bundleName string,
 	})
 }
 
-// CheckBundleSyncedStartsWith is similar to CheckBundleSynced but only checks that the synced bundle starts with the given data,
+// CheckBundleSyncedContains is similar to CheckBundleSynced but only checks that the synced bundle contains the given data,
 // along with checking that the rest of the data contains at least one valid certificate
-func CheckBundleSyncedStartsWith(ctx context.Context, cl client.Client, name string, namespace string, startingData string) error {
+func CheckBundleSyncedContains(ctx context.Context, cl client.Client, name string, namespace string, containedData string) error {
 	return checkBundleSyncedInternal(ctx, cl, name, namespace, func(got string) error {
-		if !strings.HasPrefix(got, startingData) {
-			return fmt.Errorf("received data didn't start with expected data")
+		var block *pem.Block
+		certBytes := []byte(containedData)
+
+		for {
+			block, certBytes = pem.Decode(certBytes)
+			if block == nil {
+				break
+			}
+
+			if block.Type != "CERTIFICATE" {
+				return fmt.Errorf("couldn't decode PEM block containing certificate")
+			}
+
+			if !(strings.Contains(got, string(bytes.Trim(pem.EncodeToMemory(block), "\n")))) {
+				return fmt.Errorf("did not find all certs")
+			}
 		}
 
-		remaining := strings.TrimPrefix(got, startingData)
-
+		certBytes = []byte(got)
 		// check that there are a nonzero number of valid certs remaining
+		found := false
+
+		for {
+			block, certBytes = pem.Decode(certBytes)
+			if block == nil {
+				break
+			}
+
+			if block.Type != "CERTIFICATE" {
+				return fmt.Errorf("couldn't decode PEM block containing certificate")
+			}
+
+			if strings.Contains(containedData, string(bytes.Trim(pem.EncodeToMemory(block), "\n"))) {
+				found = true
+			}
+		}
 
-		_, err := util.ValidateAndSanitizePEMBundle([]byte(remaining))
-		if err != nil {
-			return fmt.Errorf("received data didn't have any valid certs after valid starting data: %w", err)
+		if !found {
+			return fmt.Errorf("did not find additional valid certs")
 		}
 
 		return nil
@@ -263,10 +292,10 @@ func CheckBundleSyncedAllNamespaces(ctx context.Context, cl client.Client, name
 	})
 }
 
-// CheckBundleSyncedAllNamespacesStartsWith calls CheckBundleSyncedStartsWith for all namespaces and returns an error if any of them failed
-func CheckBundleSyncedAllNamespacesStartsWith(ctx context.Context, cl client.Client, name string, startingData string) error {
+// CheckBundleSyncedAllNamespacesContains calls CheckBundleSyncedContains for all namespaces and returns an error if any of them failed
+func CheckBundleSyncedAllNamespacesContains(ctx context.Context, cl client.Client, name string, containedData string) error {
 	return checkBundleSyncedAllNamespacesInternal(ctx, cl, func(namespace string) error {
-		return CheckBundleSyncedStartsWith(ctx, cl, name, namespace, startingData)
+		return CheckBundleSyncedContains(ctx, cl, name, namespace, containedData)
 	})
 }
 
@@ -281,14 +310,14 @@ func EventuallyBundleHasSyncedToNamespace(ctx context.Context, cl client.Client,
 	).Should(BeNil(), fmt.Sprintf("checking bundle %s has synced to namespace %s", bundleName, namespace))
 }
 
-// EventuallyBundleHasSyncedToNamespaceStartsWith tries to assert that the given bundle is synced correctly to the given namespace
+// EventuallyBundleHasSyncedToNamespaceContains tries to assert that the given bundle is synced correctly to the given namespace
 // until either the assertion passes or the timeout is triggered
-func EventuallyBundleHasSyncedToNamespaceStartsWith(ctx context.Context, cl client.Client, bundleName string, namespace string, startingData string) {
+func EventuallyBundleHasSyncedToNamespaceContains(ctx context.Context, cl client.Client, bundleName string, namespace string, containedData string) {
 	Eventually(
-		CheckBundleSyncedStartsWith,
+		CheckBundleSyncedContains,
 		EventuallyTimeout, EventuallyPollInterval, ctx,
 	).WithArguments(
-		ctx, cl, bundleName, namespace, startingData,
+		ctx, cl, bundleName, namespace, containedData,
 	).Should(BeNil(), fmt.Sprintf("checking bundle %s has synced to namespace %s", bundleName, namespace))
 }
 
@@ -303,14 +332,14 @@ func EventuallyBundleHasSyncedAllNamespaces(ctx context.Context, cl client.Clien
 	).Should(BeNil(), fmt.Sprintf("checking bundle %s has synced to all namespaces", bundleName))
 }
 
-// EventuallyBundleHasSyncedAllNamespacesStartsWith tries to assert that the given bundle is synced correctly to every namespace
+// EventuallyBundleHasSyncedAllNamespacesContains tries to assert that the given bundle is synced correctly to every namespace
 // until either the assertion passes or the timeout is triggered
-func EventuallyBundleHasSyncedAllNamespacesStartsWith(ctx context.Context, cl client.Client, bundleName string, startingData string) {
+func EventuallyBundleHasSyncedAllNamespacesContains(ctx context.Context, cl client.Client, bundleName string, containedData string) {
 	Eventually(
-		CheckBundleSyncedAllNamespacesStartsWith,
+		CheckBundleSyncedAllNamespacesContains,
 		EventuallyTimeout, EventuallyPollInterval, ctx,
 	).WithArguments(
-		ctx, cl, bundleName, startingData,
+		ctx, cl, bundleName, containedData,
 	).Should(BeNil(), fmt.Sprintf("checking bundle %s has synced to all namespaces with correct starting data", bundleName))
 }
 

test/integration/bundle/suite.go

@@ -172,7 +172,7 @@ var _ = Describe("Integration", func() {
 			})
 		})()).To(Succeed())
 
-		expectedData := dummy.JoinCerts(dummy.TestCertificate1, dummy.TestCertificate2, dummy.TestCertificate3, dummy.TestCertificate4)
+		expectedData := dummy.JoinCerts(dummy.TestCertificate2, dummy.TestCertificate1, dummy.TestCertificate4, dummy.TestCertificate3)
 
 		testenv.EventuallyBundleHasSyncedAllNamespaces(ctx, cl, testBundle.Name, expectedData)
 	})
@@ -194,7 +194,7 @@ var _ = Describe("Integration", func() {
 			})
 		})()).To(Succeed())
 
-		expectedData := dummy.JoinCerts(dummy.TestCertificate1, dummy.TestCertificate2, dummy.TestCertificate3, dummy.TestCertificate4)
+		expectedData := dummy.JoinCerts(dummy.TestCertificate2, dummy.TestCertificate1, dummy.TestCertificate4, dummy.TestCertificate3)
 
 		testenv.EventuallyBundleHasSyncedAllNamespaces(ctx, cl, testBundle.Name, expectedData)
 	})
@@ -206,7 +206,7 @@ var _ = Describe("Integration", func() {
 			testBundle.Spec.Sources = append(testBundle.Spec.Sources, trustapi.BundleSource{InLine: &newInLine})
 		})()).To(Succeed())
 
-		expectedData := dummy.JoinCerts(dummy.TestCertificate1, dummy.TestCertificate2, dummy.TestCertificate3, dummy.TestCertificate4)
+		expectedData := dummy.JoinCerts(dummy.TestCertificate2, dummy.TestCertificate1, dummy.TestCertificate4, dummy.TestCertificate3)
 
 		testenv.EventuallyBundleHasSyncedAllNamespaces(ctx, cl, testBundle.Name, expectedData)
 	})
@@ -216,7 +216,7 @@ var _ = Describe("Integration", func() {
 			testBundle.Spec.Sources = append(testBundle.Spec.Sources, trustapi.BundleSource{UseDefaultCAs: ptr.To(true)})
 		})()).To(Succeed())
 
-		expectedData := dummy.JoinCerts(dummy.TestCertificate1, dummy.TestCertificate2, dummy.TestCertificate3, dummy.TestCertificate5)
+		expectedData := dummy.JoinCerts(dummy.TestCertificate2, dummy.TestCertificate1, dummy.TestCertificate3, dummy.TestCertificate5)
 		testenv.EventuallyBundleHasSyncedAllNamespaces(ctx, cl, testBundle.Name, expectedData)
 	})
 
@@ -251,7 +251,7 @@ var _ = Describe("Integration", func() {
 			}
 		})()).To(Succeed())
 
-		expectedData := dummy.JoinCerts(dummy.TestCertificate1, dummy.TestCertificate2)
+		expectedData := dummy.JoinCerts(dummy.TestCertificate2, dummy.TestCertificate1)
 
 		testenv.EventuallyBundleHasSyncedAllNamespaces(ctx, cl, testBundle.Name, expectedData)
 	})
@@ -265,7 +265,7 @@ var _ = Describe("Integration", func() {
 
 		Expect(cl.Update(ctx, &configMap)).NotTo(HaveOccurred())
 
-		expectedData := dummy.JoinCerts(dummy.TestCertificate4, dummy.TestCertificate2, dummy.TestCertificate3)
+		expectedData := dummy.JoinCerts(dummy.TestCertificate2, dummy.TestCertificate4, dummy.TestCertificate3)
 
 		testenv.EventuallyBundleHasSyncedAllNamespaces(ctx, cl, testBundle.Name, expectedData)
 	})
@@ -291,7 +291,7 @@ var _ = Describe("Integration", func() {
 			testBundle.Spec.Sources[2].InLine = &newInLine
 		})()).To(Succeed())
 
-		expectedData := dummy.JoinCerts(dummy.TestCertificate1, dummy.TestCertificate2, dummy.TestCertificate4)
+		expectedData := dummy.JoinCerts(dummy.TestCertificate2, dummy.TestCertificate1, dummy.TestCertificate4)
 
 		testenv.EventuallyBundleHasSyncedAllNamespaces(ctx, cl, testBundle.Name, expectedData)
 	})