From 8a9ce5f2e01fdbdaab277790bd535ae8d6093529 Mon Sep 17 00:00:00 2001 From: Jan Kantert Date: Tue, 9 Jul 2024 18:21:22 +0200 Subject: [PATCH] sort certificates in bundles to ensure deterministic behaviour Signed-off-by: Jan Kantert --- pkg/bundle/bundle_test.go | 12 +++++------ pkg/bundle/source.go | 43 +++++++++++++++++++++++++++++++------- pkg/bundle/source_test.go | 25 ++++++++++++++++------ test/dummy/certificates.go | 2 +- 4 files changed, 61 insertions(+), 21 deletions(-) diff --git a/pkg/bundle/bundle_test.go b/pkg/bundle/bundle_test.go index ca4343db..d184b2a6 100644 --- a/pkg/bundle/bundle_test.go +++ b/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)), }, }, } diff --git a/pkg/bundle/source.go b/pkg/bundle/source.go index f6ea3a8f..b5141826 100644 --- a/pkg/bundle/source.go +++ b/pkg/bundle/source.go @@ -23,6 +23,7 @@ import ( "encoding/hex" "encoding/pem" "fmt" + "sort" "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,12 +344,29 @@ func (b *bundleData) populateData(bundles []string, formats *trustapi.Additional return nil } -// remove duplicate certificates from bundles -func deduplicateBundles(bundles []string) ([]string, error) { +// ByHash is a type alias for sorting the slice of [32]byte arrays +type ByHash [][32]byte + +// Len returns the length of the slice +func (a ByHash) Len() int { + return len(a) +} + +// Swap swaps two elements in the slice +func (a ByHash) Swap(i, j int) { + a[i], a[j] = a[j], a[i] +} + +// Less compares two elements in the slice +func (a ByHash) Less(i, j int) bool { + return bytes.Compare(a[i][:], a[j][:]) == -1 +} + +// 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) @@ -369,12 +387,21 @@ 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) + } + sort.Sort(ByHash(orderedKeys)) + var sortedDeduplicatedCerts []string + for _, key := range orderedKeys { + sortedDeduplicatedCerts = append(sortedDeduplicatedCerts, certificatesHashes[key]) } - return dedupCerts, nil + return sortedDeduplicatedCerts, nil } diff --git a/pkg/bundle/source_test.go b/pkg/bundle/source_test.go index 5a91ad29..ceee143f 100644 --- a/pkg/bundle/source_test.go +++ b/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, }, @@ -499,7 +512,7 @@ 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) diff --git a/test/dummy/certificates.go b/test/dummy/certificates.go index 30fc7d60..bff48af1 100644 --- a/test/dummy/certificates.go +++ b/test/dummy/certificates.go @@ -482,8 +482,8 @@ z40l74JcR+GvcFZWz7/jmJq95YMZ7LawLAr1CaAXxCwsoLbJpbgg4lVo6odACzY= func DefaultJoinedCerts() string { return JoinCerts( - TestCertificate1, TestCertificate2, + TestCertificate1, TestCertificate3, ) }