Skip to content

Commit

Permalink
Merge pull request #380 from jabdoa2/sort_certs
Browse files Browse the repository at this point in the history 
Sort certificates in bundles to ensure deterministic behaviour
  • Loading branch information
@cert-manager-prow
cert-manager-prow[bot] authored Jul 15, 2024
2 parents ee6582a + 0467e75 commit 8fec2fb
Show file tree
Hide file tree
Showing 7 changed files with 113 additions and 61 deletions.
12 changes: 6 additions & 6 deletions pkg/bundle/bundle_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -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)),
},
},
}
Expand Down
30 changes: 20 additions & 10 deletions pkg/bundle/source.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"encoding/hex"
"encoding/pem"
"fmt"
"slices"
"strings"

jks "github.com/pavlo-v-chernykh/keystore-go/v4"
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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" {
Expand All @@ -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
}
35 changes: 24 additions & 11 deletions pkg/bundle/source_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
Expand Down Expand Up @@ -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,
},
Expand All @@ -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,
},
Expand Down Expand Up @@ -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,
},
Expand Down Expand Up @@ -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,
},
Expand Down Expand Up @@ -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{
Expand All @@ -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": {
Expand All @@ -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,
},
Expand All @@ -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)
})
}
}
2 changes: 1 addition & 1 deletion test/dummy/certificates.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -482,8 +482,8 @@ z40l74JcR+GvcFZWz7/jmJq95YMZ7LawLAr1CaAXxCwsoLbJpbgg4lVo6odACzY=

func DefaultJoinedCerts() string {
return JoinCerts(
TestCertificate1,
TestCertificate2,
TestCertificate1,
TestCertificate3,
)
}
Expand Down
69 changes: 49 additions & 20 deletions test/env/data.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package env
import (
"bytes"
"context"
"encoding/pem"
"fmt"
"strings"

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
})
}

Expand All @@ -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))
}

Expand All @@ -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))
}

Expand Down
14 changes: 7 additions & 7 deletions test/integration/bundle/suite.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
})
Expand All @@ -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)
})
Expand All @@ -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)
})
Expand All @@ -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)
})

Expand Down Expand Up @@ -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)
})
Expand All @@ -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)
})
Expand All @@ -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)
})
Expand Down
Loading

0 comments on commit 8fec2fb

Please sign in to comment.