From 80b78be9591abe4df5861f65c0b027d87a89e3e1 Mon Sep 17 00:00:00 2001 From: Erik Godding Boye Date: Fri, 19 Jul 2024 12:45:28 +0200 Subject: [PATCH] refactor: extract truststore encoding to internal package Signed-off-by: Erik Godding Boye --- pkg/bundle/bundle_test.go | 3 +- pkg/bundle/internal/truststore/types.go | 139 +++++++++++++++++++ pkg/bundle/internal/truststore/types_test.go | 84 +++++++++++ pkg/bundle/source.go | 106 +------------- pkg/bundle/source_test.go | 56 -------- 5 files changed, 228 insertions(+), 160 deletions(-) create mode 100644 pkg/bundle/internal/truststore/types.go create mode 100644 pkg/bundle/internal/truststore/types_test.go diff --git a/pkg/bundle/bundle_test.go b/pkg/bundle/bundle_test.go index b301faff..32b264e4 100644 --- a/pkg/bundle/bundle_test.go +++ b/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) } diff --git a/pkg/bundle/internal/truststore/types.go b/pkg/bundle/internal/truststore/types.go new file mode 100644 index 00000000..c2e101f7 --- /dev/null +++ b/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 +} diff --git a/pkg/bundle/internal/truststore/types_test.go b/pkg/bundle/internal/truststore/types_test.go new file mode 100644 index 00000000..1c931a32 --- /dev/null +++ b/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) + } +} diff --git a/pkg/bundle/source.go b/pkg/bundle/source.go index 6e53e274..5551abae 100644 --- a/pkg/bundle/source.go +++ b/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,104 +208,6 @@ 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" @@ -315,7 +215,7 @@ func (b *bundleData) populateData(bundles []string, formats *trustapi.Additional 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) } @@ -323,7 +223,7 @@ func (b *bundleData) populateData(bundles []string, formats *trustapi.Additional } 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) } diff --git a/pkg/bundle/source_test.go b/pkg/bundle/source_test.go index 7e6755a0..61bc13f6 100644 --- a/pkg/bundle/source_test.go +++ b/pkg/bundle/source_test.go @@ -19,7 +19,6 @@ package bundle import ( "bytes" "context" - "crypto/x509" "encoding/pem" "errors" "testing" @@ -390,61 +389,6 @@ func Test_buildSourceBundle(t *testing.T) { } } -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: trustapi.DefaultJKSPassword}.encode(bundle) - if err != nil { - t.Fatalf("didn't expect an error but got: %s", err) - } - - reader := bytes.NewReader(jksFile) - - ks := jks.New() - - err = ks.Load(reader, []byte(trustapi.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) - } -} - func TestBundlesDeduplication(t *testing.T) { tests := map[string]struct { name string