Merge pull request #396 from erikgb/truststore-extract

refactor: extract truststore encoding to internal package

Author: cert-manager-prow[bot]

pkg/bundle/bundle_test.go

@@ -40,6 +40,7 @@ import (
 	fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake"
 
 	trustapi "github.com/cert-manager/trust-manager/pkg/apis/trust/v1alpha1"
+	"github.com/cert-manager/trust-manager/pkg/bundle/internal/truststore"
 	"github.com/cert-manager/trust-manager/pkg/fspkg"
 	"github.com/cert-manager/trust-manager/test/dummy"
 	"github.com/cert-manager/trust-manager/test/gen"
@@ -48,7 +49,7 @@ import (
 func testEncodeJKS(t *testing.T, data string) []byte {
 	t.Helper()
 
-	encoded, err := jksEncoder{password: trustapi.DefaultJKSPassword}.encode(data)
+	encoded, err := truststore.NewJKSEncoder(trustapi.DefaultJKSPassword).Encode(data)
 	if err != nil {
 		t.Error(err)
 	}

pkg/bundle/internal/truststore/types.go

@@ -0,0 +1,139 @@
+/*
+Copyright 2021 The cert-manager Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package truststore
+
+import (
+	"bytes"
+	"crypto/sha256"
+	"encoding/hex"
+	"fmt"
+
+	"github.com/pavlo-v-chernykh/keystore-go/v4"
+	"software.sslmate.com/src/go-pkcs12"
+
+	"github.com/cert-manager/trust-manager/pkg/util"
+)
+
+type Encoder interface {
+	Encode(trustBundle string) ([]byte, error)
+}
+
+func NewJKSEncoder(password string) Encoder {
+	return &jksEncoder{password: password}
+}
+
+type jksEncoder struct {
+	password string
+}
+
+// Encode creates a binary JKS file from the given PEM-encoded trust bundle and Password.
+// Note that the Password is not treated securely; JKS files generally seem to expect a Password
+// to exist and so we have the option for one.
+func (e jksEncoder) Encode(trustBundle string) ([]byte, error) {
+	cas, err := util.DecodeX509CertificateChainBytes([]byte(trustBundle))
+	if err != nil {
+		return nil, fmt.Errorf("failed to decode trust bundle: %w", err)
+	}
+
+	// WithOrderedAliases ensures that trusted certs are added to the JKS file in order,
+	// which makes the files appear to be reliably deterministic.
+	ks := keystore.New(keystore.WithOrderedAliases())
+
+	for _, c := range cas {
+		alias := certAlias(c.Raw, c.Subject.String())
+
+		// Note on CreationTime:
+		// Debian's JKS trust store sets the creation time to match the time that certs are added to the
+		// trust store (i.e., it's effectively time.Now() at the instant the file is generated).
+		// Using that method would make our JKS files in trust-manager non-deterministic, leaving us with
+		// two options if we want to maintain determinism:
+		// - Using something from the cert being added (e.g. NotBefore / NotAfter)
+		// - Using a fixed time (i.e. unix epoch)
+		// We use NotBefore here, arbitrarily.
+
+		err = ks.SetTrustedCertificateEntry(alias, keystore.TrustedCertificateEntry{
+			CreationTime: c.NotBefore,
+			Certificate: keystore.Certificate{
+				Type:    "X509",
+				Content: c.Raw,
+			},
+		})
+
+		if err != nil {
+			// this error should never happen if we set jks.Certificate correctly
+			return nil, fmt.Errorf("failed to add cert with alias %q to trust store: %w", alias, err)
+		}
+	}
+
+	buf := &bytes.Buffer{}
+
+	err = ks.Store(buf, []byte(e.password))
+	if err != nil {
+		return nil, fmt.Errorf("failed to create JKS file: %w", err)
+	}
+
+	return buf.Bytes(), nil
+}
+
+func NewPKCS12Encoder(password string) Encoder {
+	return &pkcs12Encoder{password: password}
+}
+
+type pkcs12Encoder struct {
+	password string
+}
+
+func (e pkcs12Encoder) Encode(trustBundle string) ([]byte, error) {
+	cas, err := util.DecodeX509CertificateChainBytes([]byte(trustBundle))
+	if err != nil {
+		return nil, fmt.Errorf("failed to decode trust bundle: %w", err)
+	}
+
+	var entries []pkcs12.TrustStoreEntry
+	for _, c := range cas {
+		entries = append(entries, pkcs12.TrustStoreEntry{
+			Cert:         c,
+			FriendlyName: certAlias(c.Raw, c.Subject.String()),
+		})
+	}
+
+	encoder := pkcs12.LegacyRC2
+
+	if e.password == "" {
+		encoder = pkcs12.Passwordless
+	}
+
+	return encoder.EncodeTrustStoreEntries(entries, e.password)
+}
+
+// certAlias creates a JKS-safe alias for the given DER-encoded certificate, such that
+// any two certificates will have a different aliases unless they're identical in every way.
+// This unique alias fixes an issue where we used the Issuer field as an alias, leading to
+// different certs being treated as identical.
+// The friendlyName is included in the alias as a UX feature when examining JKS files using a
+// tool like `keytool`.
+func certAlias(derData []byte, friendlyName string) string {
+	certHashBytes := sha256.Sum256(derData)
+	certHash := hex.EncodeToString(certHashBytes[:])
+
+	// Since certHash is the part which actually distinguishes between two
+	// certificates, put it first so that it won't be truncated if a cert
+	// with a really long subject is added. Not sure what the upper limit
+	// for length actually is, but it shouldn't matter here.
+
+	return certHash[:8] + "|" + friendlyName
+}

pkg/bundle/internal/truststore/types_test.go

@@ -0,0 +1,84 @@
+/*
+Copyright 2021 The cert-manager Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package truststore
+
+import (
+	"bytes"
+	"crypto/x509"
+	"encoding/pem"
+	"testing"
+
+	"github.com/pavlo-v-chernykh/keystore-go/v4"
+
+	"github.com/cert-manager/trust-manager/pkg/apis/trust/v1alpha1"
+	"github.com/cert-manager/trust-manager/test/dummy"
+)
+
+func Test_encodeJKSAliases(t *testing.T) {
+	// IMPORTANT: We use TestCertificate1 and TestCertificate2 here because they're defined
+	// to be self-signed and to also use the same Subject, while being different certs.
+	// This test ensures that the aliases we create when adding to a JKS file is different under
+	// these conditions (where the issuer / subject is identical).
+	// Using different dummy certs would allow this test to pass but wouldn't actually test anything useful!
+	bundle := dummy.JoinCerts(dummy.TestCertificate1, dummy.TestCertificate2)
+
+	jksFile, err := jksEncoder{password: v1alpha1.DefaultJKSPassword}.Encode(bundle)
+	if err != nil {
+		t.Fatalf("didn't expect an error but got: %s", err)
+	}
+
+	reader := bytes.NewReader(jksFile)
+
+	ks := keystore.New()
+
+	err = ks.Load(reader, []byte(v1alpha1.DefaultJKSPassword))
+	if err != nil {
+		t.Fatalf("failed to parse generated JKS file: %s", err)
+	}
+
+	entryNames := ks.Aliases()
+
+	if len(entryNames) != 2 {
+		t.Fatalf("expected two certs in JKS file but got %d", len(entryNames))
+	}
+}
+
+func Test_certAlias(t *testing.T) {
+	// We might not ever rely on aliases being stable, but this test seeks
+	// to enforce stability for now. It'll be easy to remove.
+
+	// If this test starts failing after TestCertificate1 is updated, it'll
+	// need to be updated with the new alias for the new cert.
+
+	block, _ := pem.Decode([]byte(dummy.TestCertificate1))
+	if block == nil {
+		t.Fatalf("couldn't parse a PEM block from TestCertificate1")
+	}
+
+	cert, err := x509.ParseCertificate(block.Bytes)
+	if err != nil {
+		t.Fatalf("Dummy certificate TestCertificate1 couldn't be parsed: %s", err)
+	}
+
+	alias := certAlias(cert.Raw, cert.Subject.String())
+
+	expectedAlias := "548b988f|CN=cmct-test-root,O=cert-manager"
+
+	if alias != expectedAlias {
+		t.Fatalf("expected alias to be %q but got %q", expectedAlias, alias)
+	}
+}

pkg/bundle/source.go

@@ -20,20 +20,18 @@ import (
 	"bytes"
 	"context"
 	"crypto/sha256"
-	"encoding/hex"
 	"encoding/pem"
 	"fmt"
 	"slices"
 	"strings"
 
-	jks "github.com/pavlo-v-chernykh/keystore-go/v4"
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
-	"software.sslmate.com/src/go-pkcs12"
 
 	trustapi "github.com/cert-manager/trust-manager/pkg/apis/trust/v1alpha1"
+	"github.com/cert-manager/trust-manager/pkg/bundle/internal/truststore"
 	"github.com/cert-manager/trust-manager/pkg/util"
 )
 
@@ -210,120 +208,22 @@ func (b *bundle) secretBundle(ctx context.Context, ref *trustapi.SourceObjectKey
 	return results.String(), nil
 }
 
-type jksEncoder struct {
-	password string
-}
-
-// encodeJKS creates a binary JKS file from the given PEM-encoded trust bundle and password.
-// Note that the password is not treated securely; JKS files generally seem to expect a password
-// to exist and so we have the option for one.
-func (e jksEncoder) encode(trustBundle string) ([]byte, error) {
-	cas, err := util.DecodeX509CertificateChainBytes([]byte(trustBundle))
-	if err != nil {
-		return nil, fmt.Errorf("failed to decode trust bundle: %w", err)
-	}
-
-	// WithOrderedAliases ensures that trusted certs are added to the JKS file in order,
-	// which makes the files appear to be reliably deterministic.
-	ks := jks.New(jks.WithOrderedAliases())
-
-	for _, c := range cas {
-		alias := certAlias(c.Raw, c.Subject.String())
-
-		// Note on CreationTime:
-		// Debian's JKS trust store sets the creation time to match the time that certs are added to the
-		// trust store (i.e., it's effectively time.Now() at the instant the file is generated).
-		// Using that method would make our JKS files in trust-manager non-deterministic, leaving us with
-		// two options if we want to maintain determinism:
-		// - Using something from the cert being added (e.g. NotBefore / NotAfter)
-		// - Using a fixed time (i.e. unix epoch)
-		// We use NotBefore here, arbitrarily.
-
-		err = ks.SetTrustedCertificateEntry(alias, jks.TrustedCertificateEntry{
-			CreationTime: c.NotBefore,
-			Certificate: jks.Certificate{
-				Type:    "X509",
-				Content: c.Raw,
-			},
-		})
-
-		if err != nil {
-			// this error should never happen if we set jks.Certificate correctly
-			return nil, fmt.Errorf("failed to add cert with alias %q to trust store: %w", alias, err)
-		}
-	}
-
-	buf := &bytes.Buffer{}
-
-	err = ks.Store(buf, []byte(e.password))
-	if err != nil {
-		return nil, fmt.Errorf("failed to create JKS file: %w", err)
-	}
-
-	return buf.Bytes(), nil
-}
-
-// certAlias creates a JKS-safe alias for the given DER-encoded certificate, such that
-// any two certificates will have a different aliases unless they're identical in every way.
-// This unique alias fixes an issue where we used the Issuer field as an alias, leading to
-// different certs being treated as identical.
-// The friendlyName is included in the alias as a UX feature when examining JKS files using a
-// tool like `keytool`.
-func certAlias(derData []byte, friendlyName string) string {
-	certHashBytes := sha256.Sum256(derData)
-	certHash := hex.EncodeToString(certHashBytes[:])
-
-	// Since certHash is the part which actually distinguishes between two
-	// certificates, put it first so that it won't be truncated if a cert
-	// with a really long subject is added. Not sure what the upper limit
-	// for length actually is, but it shouldn't matter here.
-
-	return certHash[:8] + "|" + friendlyName
-}
-
-type pkcs12Encoder struct {
-	password string
-}
-
-func (e pkcs12Encoder) encode(trustBundle string) ([]byte, error) {
-	cas, err := util.DecodeX509CertificateChainBytes([]byte(trustBundle))
-	if err != nil {
-		return nil, fmt.Errorf("failed to decode trust bundle: %w", err)
-	}
-
-	var entries []pkcs12.TrustStoreEntry
-	for _, c := range cas {
-		entries = append(entries, pkcs12.TrustStoreEntry{
-			Cert:         c,
-			FriendlyName: certAlias(c.Raw, c.Subject.String()),
-		})
-	}
-
-	encoder := pkcs12.LegacyRC2
-
-	if e.password == "" {
-		encoder = pkcs12.Passwordless
-	}
-
-	return encoder.EncodeTrustStoreEntries(entries, e.password)
-}
-
 func (b *bundleData) populateData(bundles []string, formats *trustapi.AdditionalFormats) error {
 	b.data = strings.Join(bundles, "\n") + "\n"
 
 	if formats != nil {
 		b.binaryData = make(map[string][]byte)
 
 		if formats.JKS != nil {
-			encoded, err := jksEncoder{password: *formats.JKS.Password}.encode(b.data)
+			encoded, err := truststore.NewJKSEncoder(*formats.JKS.Password).Encode(b.data)
 			if err != nil {
 				return fmt.Errorf("failed to encode JKS: %w", err)
 			}
 			b.binaryData[formats.JKS.Key] = encoded
 		}
 
 		if formats.PKCS12 != nil {
-			encoded, err := pkcs12Encoder{password: *formats.PKCS12.Password}.encode(b.data)
+			encoded, err := truststore.NewPKCS12Encoder(*formats.PKCS12.Password).Encode(b.data)
 			if err != nil {
 				return fmt.Errorf("failed to encode PKCS12: %w", err)
 			}