diff --git a/docs/api/api.md b/docs/api/api.md index a3d999ed..de4bcc05 100644 --- a/docs/api/api.md +++ b/docs/api/api.md @@ -58,10 +58,19 @@ import "github.com/cert-manager/trust-manager/pkg/apis/trust/v1alpha1" ## Constants - + ```go const ( + // DefaultJKSPassword is the default password that Java uses; it's a Java convention to use this exact password. + // Since we're not storing anything secret in the JKS files we generate, this password is not a meaningful security measure + // but seems often to be expected by applications consuming JKS files + DefaultJKSPassword = "changeit" + // DefaultPKCS12Password is the empty string, that will create a password-less PKCS12 truststore. + // Password-less PKCS is the new default Java truststore from Java 18. + // By password-less, it means the certificates are not encrypted, and it contains no MacData for integrity check. + DefaultPKCS12Password = "" + // BundleConditionSynced indicates that the Bundle has successfully synced // all source bundle data to the Bundle target in all Namespaces. BundleConditionSynced string = "Synced" diff --git a/pkg/apis/trust/v1alpha1/types_bundle.go b/pkg/apis/trust/v1alpha1/types_bundle.go index ff0c0d4e..7e26a0d9 100644 --- a/pkg/apis/trust/v1alpha1/types_bundle.go +++ b/pkg/apis/trust/v1alpha1/types_bundle.go @@ -239,6 +239,15 @@ type BundleCondition struct { } const ( + // DefaultJKSPassword is the default password that Java uses; it's a Java convention to use this exact password. + // Since we're not storing anything secret in the JKS files we generate, this password is not a meaningful security measure + // but seems often to be expected by applications consuming JKS files + DefaultJKSPassword = "changeit" + // DefaultPKCS12Password is the empty string, that will create a password-less PKCS12 truststore. + // Password-less PKCS is the new default Java truststore from Java 18. + // By password-less, it means the certificates are not encrypted, and it contains no MacData for integrity check. + DefaultPKCS12Password = "" + // BundleConditionSynced indicates that the Bundle has successfully synced // all source bundle data to the Bundle target in all Namespaces. BundleConditionSynced string = "Synced" diff --git a/pkg/bundle/bundle.go b/pkg/bundle/bundle.go index 3f768e23..3f11a076 100644 --- a/pkg/bundle/bundle.go +++ b/pkg/bundle/bundle.go @@ -33,12 +33,12 @@ import ( "k8s.io/client-go/tools/record" "k8s.io/client-go/util/csaupgrade" "k8s.io/utils/clock" - "k8s.io/utils/ptr" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" trustapi "github.com/cert-manager/trust-manager/pkg/apis/trust/v1alpha1" "github.com/cert-manager/trust-manager/pkg/bundle/internal/ssa_client" + "github.com/cert-manager/trust-manager/pkg/bundle/internal/target" "github.com/cert-manager/trust-manager/pkg/fspkg" ) @@ -69,10 +69,6 @@ type bundle struct { // a cache-backed Kubernetes client client client.Client - // targetCache is a cache.Cache that holds cached ConfigMap and Secret - // resources that are used as targets for Bundles. - targetCache client.Reader - // defaultPackage holds the loaded 'default' certificate package, if one was specified // at startup. defaultPackage *fspkg.Package @@ -86,9 +82,7 @@ type bundle struct { // Options holds options for the Bundle controller. Options - // patchResourceOverwrite allows use to override the patchResource function - // it is used for testing purposes - patchResourceOverwrite func(ctx context.Context, obj interface{}) error + targetReconciler *target.Reconciler } // Reconcile is the top level function for reconciling over synced Bundles. @@ -103,12 +97,7 @@ func (b *bundle) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, return ctrl.Result{}, utilerrors.NewAggregate([]error{resultErr, err}) } - if err := b.client.Status().Patch(ctx, con, patch, &client.SubResourcePatchOptions{ - PatchOptions: client.PatchOptions{ - FieldManager: fieldManager, - Force: ptr.To(true), - }, - }); err != nil { + if err := b.client.Status().Patch(ctx, con, patch, ssa_client.FieldManager, client.ForceOwnership); err != nil { err = fmt.Errorf("failed to apply bundle status patch: %w", err) return ctrl.Result{}, utilerrors.NewAggregate([]error{resultErr, err}) } @@ -260,7 +249,7 @@ func (b *bundle) reconcileBundle(ctx context.Context, req ctrl.Request) (result Kind: string(kind), }, } - err := b.targetCache.List(ctx, targetList, &client.ListOptions{ + err := b.targetReconciler.Cache.List(ctx, targetList, &client.ListOptions{ LabelSelector: labels.SelectorFromSet(map[string]string{ trustapi.BundleLabelKey: bundle.Name, }), @@ -310,12 +299,12 @@ func (b *bundle) reconcileBundle(ctx context.Context, req ctrl.Request) (result if target.Kind == configMapTarget { syncFunc = func(targetLog logr.Logger, target targetResource, shouldExist bool) (bool, error) { - return b.syncConfigMapTarget(ctx, targetLog, &bundle, target.Name, target.Namespace, resolvedBundle, shouldExist) + return b.targetReconciler.SyncConfigMap(ctx, targetLog, &bundle, target.NamespacedName, resolvedBundle.Data, shouldExist) } } if target.Kind == secretTarget { syncFunc = func(targetLog logr.Logger, target targetResource, shouldExist bool) (bool, error) { - return b.syncSecretTarget(ctx, targetLog, &bundle, target.Name, target.Namespace, resolvedBundle, shouldExist) + return b.targetReconciler.SyncSecret(ctx, targetLog, &bundle, target.NamespacedName, resolvedBundle.Data, shouldExist) } } @@ -394,7 +383,8 @@ func (b *bundle) bundleTargetNamespaceSelector(bundleObj *trustapi.Bundle) (labe // to ensure that the apply operations will also remove fields that were // created by the Update operation. func (b *bundle) migrateBundleStatusToApply(ctx context.Context, obj client.Object) (bool, error) { - patch, err := csaupgrade.UpgradeManagedFieldsPatch(obj, sets.New(fieldManager, crRegressionFieldManager), fieldManager, csaupgrade.Subresource("status")) + fieldManager := string(ssa_client.FieldManager) + patch, err := csaupgrade.UpgradeManagedFieldsPatch(obj, sets.New(fieldManager, ssa_client.CRRegressionFieldManager), fieldManager, csaupgrade.Subresource("status")) if err != nil { return false, err } diff --git a/pkg/bundle/bundle_test.go b/pkg/bundle/bundle_test.go index ca4343db..a25dfe7c 100644 --- a/pkg/bundle/bundle_test.go +++ b/pkg/bundle/bundle_test.go @@ -40,6 +40,8 @@ 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/ssa_client" + "github.com/cert-manager/trust-manager/pkg/bundle/internal/target" "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 +50,7 @@ import ( func testEncodeJKS(t *testing.T, data string) []byte { t.Helper() - encoded, err := jksEncoder{password: DefaultJKSPassword}.encode(data) + encoded, err := target.JKSEncoder{Password: trustapi.DefaultJKSPassword}.Encode(data) if err != nil { t.Error(err) } @@ -210,7 +212,7 @@ func Test_Reconcile(t *testing.T) { Labels: baseBundleLabels, Annotations: annotations, OwnerReferences: baseBundleOwnerRef, - ManagedFields: managedFieldEntries(dataEntries, binDataEntries), + ManagedFields: ssa_client.ManagedFieldEntries(dataEntries, binDataEntries), }, Data: data, BinaryData: binData, @@ -247,7 +249,7 @@ func Test_Reconcile(t *testing.T) { Labels: baseBundleLabels, Annotations: annotations, OwnerReferences: baseBundleOwnerRef, - ManagedFields: managedFieldEntries(dataEntries, nil), + ManagedFields: ssa_client.ManagedFieldEntries(dataEntries, nil), }, Data: binaryData, } @@ -493,7 +495,7 @@ func Test_Reconcile(t *testing.T) { KeySelector: trustapi.KeySelector{ Key: "target.jks", }, - Password: ptr.To(DefaultJKSPassword), + Password: ptr.To(trustapi.DefaultJKSPassword), }, }), )}, @@ -566,7 +568,7 @@ func Test_Reconcile(t *testing.T) { KeySelector: trustapi.KeySelector{ Key: "target.jks", }, - Password: ptr.To(DefaultJKSPassword), + Password: ptr.To(trustapi.DefaultJKSPassword), }, }), ), @@ -1304,22 +1306,25 @@ func Test_Reconcile(t *testing.T) { log, ctx := ktesting.NewTestContext(t) b := &bundle{ - client: fakeclient, - targetCache: fakeclient, - recorder: fakerecorder, - clock: fixedclock, + client: fakeclient, + recorder: fakerecorder, + clock: fixedclock, Options: Options{ Log: log, Namespace: trustNamespace, SecretTargetsEnabled: !test.disableSecretTargets, FilterExpiredCerts: true, }, - patchResourceOverwrite: func(ctx context.Context, obj interface{}) error { - logMutex.Lock() - defer logMutex.Unlock() + targetReconciler: &target.Reconciler{ + Client: fakeclient, + Cache: fakeclient, + PatchResourceOverwrite: func(ctx context.Context, obj interface{}) error { + logMutex.Lock() + defer logMutex.Unlock() - resourcePatches = append(resourcePatches, obj) - return nil + resourcePatches = append(resourcePatches, obj) + return nil + }, }, } diff --git a/pkg/bundle/controller.go b/pkg/bundle/controller.go index 40c5cc77..c5be1e43 100644 --- a/pkg/bundle/controller.go +++ b/pkg/bundle/controller.go @@ -37,6 +37,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/source" trustapi "github.com/cert-manager/trust-manager/pkg/apis/trust/v1alpha1" + "github.com/cert-manager/trust-manager/pkg/bundle/internal/target" "github.com/cert-manager/trust-manager/pkg/fspkg" ) @@ -52,11 +53,14 @@ func AddBundleController( targetCache cache.Cache, ) error { b := &bundle{ - client: mgr.GetClient(), - targetCache: targetCache, - recorder: mgr.GetEventRecorderFor("bundles"), - clock: clock.RealClock{}, - Options: opts, + client: mgr.GetClient(), + recorder: mgr.GetEventRecorderFor("bundles"), + clock: clock.RealClock{}, + Options: opts, + targetReconciler: &target.Reconciler{ + Client: mgr.GetClient(), + Cache: targetCache, + }, } if b.Options.DefaultPackageLocation != "" { diff --git a/pkg/bundle/internal/ssa_client/patch.go b/pkg/bundle/internal/ssa_client/patch.go index fe6a98bc..d04a0319 100644 --- a/pkg/bundle/internal/ssa_client/patch.go +++ b/pkg/bundle/internal/ssa_client/patch.go @@ -17,8 +17,19 @@ limitations under the License. package ssa_client import ( + "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/structured-merge-diff/fieldpath" +) + +const ( + FieldManager = client.FieldOwner("trust-manager") + // CRRegressionFieldManager is the field manager that was introduced by a regression in controller-runtime + // version 0.15.0; fixed in 15.1 and 0.16.0: https://github.com/kubernetes-sigs/controller-runtime/pull/2435 + // trust-manager 0.6.0 was released with this regression in controller-runtime, which means that we have to + // take extra care when migrating from CSA to SSA. + CRRegressionFieldManager = "Go-http-client" ) type applyPatch struct { @@ -34,3 +45,32 @@ func (p applyPatch) Data(_ client.Object) ([]byte, error) { func (p applyPatch) Type() types.PatchType { return types.ApplyPatchType } + +func ManagedFieldEntries(fields []string, dataFields []string) []v1.ManagedFieldsEntry { + fieldset := fieldpath.NewSet() + for _, property := range fields { + fieldset.Insert( + fieldpath.MakePathOrDie("data", property), + ) + } + for _, property := range dataFields { + fieldset.Insert( + fieldpath.MakePathOrDie("binaryData", property), + ) + } + + jsonFieldSet, err := fieldset.ToJSON() + if err != nil { + panic(err) + } + + return []v1.ManagedFieldsEntry{ + { + Manager: "trust-manager", + Operation: v1.ManagedFieldsOperationApply, + FieldsV1: &v1.FieldsV1{ + Raw: jsonFieldSet, + }, + }, + } +} diff --git a/pkg/bundle/internal/target/data.go b/pkg/bundle/internal/target/data.go new file mode 100644 index 00000000..21cd96eb --- /dev/null +++ b/pkg/bundle/internal/target/data.go @@ -0,0 +1,161 @@ +/* +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 target + +import ( + "bytes" + "crypto/sha256" + "encoding/hex" + "fmt" + "strings" + + jks "github.com/pavlo-v-chernykh/keystore-go/v4" + "software.sslmate.com/src/go-pkcs12" + + trustapi "github.com/cert-manager/trust-manager/pkg/apis/trust/v1alpha1" + "github.com/cert-manager/trust-manager/pkg/util" +) + +// Data contains the resulting PEM-encoded certificate data from concatenating all the bundle sources together +// and binary data for any additional formats. +type Data struct { + Data string + BinaryData map[string][]byte +} + +func (b *Data) Populate(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) + 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) + if err != nil { + return fmt.Errorf("failed to encode PKCS12: %w", err) + } + b.BinaryData[formats.PKCS12.Key] = encoded + } + } + return nil +} + +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 := 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) +} diff --git a/pkg/bundle/internal/target/data_test.go b/pkg/bundle/internal/target/data_test.go new file mode 100644 index 00000000..45f6dcd4 --- /dev/null +++ b/pkg/bundle/internal/target/data_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 target + +import ( + "bytes" + "crypto/x509" + "encoding/pem" + "testing" + + jks "github.com/pavlo-v-chernykh/keystore-go/v4" + + trustapi "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: 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) + } +} diff --git a/pkg/bundle/target.go b/pkg/bundle/internal/target/target.go similarity index 53% rename from pkg/bundle/target.go rename to pkg/bundle/internal/target/target.go index e9532167..abfbf6d8 100644 --- a/pkg/bundle/target.go +++ b/pkg/bundle/internal/target/target.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package bundle +package target import ( "bytes" @@ -25,6 +25,7 @@ import ( "strings" "github.com/go-logr/logr" + corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -40,114 +41,87 @@ import ( "github.com/cert-manager/trust-manager/pkg/bundle/internal/ssa_client" ) -const ( - // crRegressionFieldManager is the field manager that was introduced by a regression in controller-runtime - // version 0.15.0; fixed in 15.1 and 0.16.0: https://github.com/kubernetes-sigs/controller-runtime/pull/2435 - // trust-manager 0.6.0 was released with this regression in controller-runtime, which means that we have to - // take extra care when migrating from CSA to SSA. - crRegressionFieldManager = "Go-http-client" - fieldManager = "trust-manager" -) +type Reconciler struct { + // a cache-backed Kubernetes client + Client client.Client + + // Cache is a cache.Cache that holds cached ConfigMap and Secret + // resources that are used as targets for Bundles. + Cache client.Reader + + // PatchResourceOverwrite allows use to override the patchResource function + // it is used for testing purposes + PatchResourceOverwrite func(ctx context.Context, obj interface{}) error +} -// syncConfigMapTarget syncs the given data to the target ConfigMap in the given namespace. +// SyncConfigMap syncs the given data to the target ConfigMap in the given namespace. // The name of the ConfigMap is the same as the Bundle. // Ensures the ConfigMap is owned by the given Bundle, and the data is up to date. // Returns true if the ConfigMap has been created or was updated. -func (b *bundle) syncConfigMapTarget( +func (r *Reconciler) SyncConfigMap( ctx context.Context, log logr.Logger, bundle *trustapi.Bundle, - name string, - namespace string, - resolvedBundle bundleData, + name types.NamespacedName, + data Data, shouldExist bool, ) (bool, error) { - configMap := &metav1.PartialObjectMetadata{ + targetObj := &metav1.PartialObjectMetadata{ TypeMeta: metav1.TypeMeta{ Kind: "ConfigMap", APIVersion: "v1", }, } - err := b.targetCache.Get(ctx, client.ObjectKey{Namespace: namespace, Name: name}, configMap) + err := r.Cache.Get(ctx, name, targetObj) if err != nil && !apierrors.IsNotFound(err) { - return false, fmt.Errorf("failed to get ConfigMap %s/%s: %w", namespace, name, err) + return false, fmt.Errorf("failed to get ConfigMap %s: %w", name, err) } - // If the ConfigMap exists, but the Bundle is being deleted, delete the ConfigMap. - if apierrors.IsNotFound(err) && !shouldExist { - return false, nil - } - - // If the ConfigMap should not exist, but it does, delete it. - if !apierrors.IsNotFound(err) && !shouldExist { - // apply empty patch to remove the key - configMapPatch := coreapplyconfig. - ConfigMap(name, namespace). - WithLabels(map[string]string{ - trustapi.BundleLabelKey: bundle.Name, - }). - WithOwnerReferences( - metav1applyconfig.OwnerReference(). - WithAPIVersion(trustapi.SchemeGroupVersion.String()). - WithKind(trustapi.BundleKind). - WithName(bundle.GetName()). - WithUID(bundle.GetUID()). - WithBlockOwnerDeletion(true). - WithController(true), - ) - - if err = b.patchConfigMapResource(ctx, configMapPatch); err != nil { - return false, fmt.Errorf("failed to patch ConfigMap %s/%s: %w", namespace, bundle.Name, err) + if !shouldExist { + // If the ConfigMap is not found and should not exist we are done. + if apierrors.IsNotFound(err) { + return false, nil + } + // If the ConfigMap should not exist, but it does, delete it. + // Apply empty patch to remove the keys + configMap, err := r.patchConfigMap(ctx, newConfigMapPatch(name, *bundle)) + if err != nil { + return false, fmt.Errorf("failed to patch ConfigMap %s: %w", name, err) } + // If the configMap is empty, delete it + if configMap != nil && len(configMap.Data) == 0 && len(configMap.BinaryData) == 0 { + return true, r.Client.Delete(ctx, configMap) + } return true, nil } - target := bundle.Spec.Target - if target.ConfigMap == nil { + if bundle.Spec.Target.ConfigMap == nil { return false, errors.New("target not defined") } // Generated JKS is not deterministic - best we can do here is update if the pem cert has // changed (hence not checking if JKS matches) - dataHash := fmt.Sprintf("%x", sha256.Sum256([]byte(resolvedBundle.data))) - configmapData := map[string]string{ - target.ConfigMap.Key: resolvedBundle.data, - } - configmapBinData := resolvedBundle.binaryData - + dataHash := fmt.Sprintf("%x", sha256.Sum256([]byte(data.Data))) // If the ConfigMap doesn't exist, create it. if !apierrors.IsNotFound(err) { // Exit early if no update is needed - if exit, err := b.needsUpdate(ctx, targetKindConfigMap, log, configMap, bundle, dataHash); err != nil { + if exit, err := r.needsUpdate(ctx, KindConfigMap, log, targetObj, bundle, dataHash); err != nil { return false, err } else if !exit { return false, nil } } - configMapPatch := coreapplyconfig. - ConfigMap(name, namespace). - WithLabels(map[string]string{ - trustapi.BundleLabelKey: bundle.Name, - }). + configMapPatch := newConfigMapPatch(name, *bundle). WithAnnotations(map[string]string{ trustapi.BundleHashAnnotationKey: dataHash, }). - WithOwnerReferences( - metav1applyconfig.OwnerReference(). - WithAPIVersion(trustapi.SchemeGroupVersion.String()). - WithKind(trustapi.BundleKind). - WithName(bundle.GetName()). - WithUID(bundle.GetUID()). - WithBlockOwnerDeletion(true). - WithController(true), - ). - WithData(configmapData). - WithBinaryData(configmapBinData) + WithData(map[string]string{bundle.Spec.Target.ConfigMap.Key: data.Data}). + WithBinaryData(data.BinaryData) - if err = b.patchConfigMapResource(ctx, configMapPatch); err != nil { - return false, fmt.Errorf("failed to patch ConfigMap %s/%s: %w", namespace, bundle.Name, err) + if _, err = r.patchConfigMap(ctx, configMapPatch); err != nil { + return false, fmt.Errorf("failed to patch ConfigMap %s: %w", name, err) } log.V(2).Info("synced bundle to namespace for target ConfigMap") @@ -155,107 +129,80 @@ func (b *bundle) syncConfigMapTarget( return true, nil } -// syncSecretTarget syncs the given data to the target Secret in the given namespace. +// SyncSecret syncs the given data to the target Secret in the given namespace. // The name of the Secret is the same as the Bundle. // Ensures the Secret is owned by the given Bundle, and the data is up to date. // Returns true if the Secret has been created or was updated. -func (b *bundle) syncSecretTarget( +func (r *Reconciler) SyncSecret( ctx context.Context, log logr.Logger, bundle *trustapi.Bundle, - name string, - namespace string, - resolvedBundle bundleData, + name types.NamespacedName, + data Data, shouldExist bool, ) (bool, error) { - secret := &metav1.PartialObjectMetadata{ + targetObj := &metav1.PartialObjectMetadata{ TypeMeta: metav1.TypeMeta{ Kind: "Secret", APIVersion: "v1", }, } - err := b.targetCache.Get(ctx, client.ObjectKey{Namespace: namespace, Name: name}, secret) + err := r.Cache.Get(ctx, name, targetObj) if err != nil && !apierrors.IsNotFound(err) { - return false, fmt.Errorf("failed to get Secret %s/%s: %w", namespace, name, err) + return false, fmt.Errorf("failed to get Secret %s: %w", name, err) } - // If the target obj exists, but the Bundle is being deleted, delete the Secret. - if apierrors.IsNotFound(err) && !shouldExist { - return false, nil - } - - // If the Secret should not exist, but it does, delete it. - if !apierrors.IsNotFound(err) && !shouldExist { - // apply empty patch to remove the key - secretPatch := coreapplyconfig. - Secret(name, namespace). - WithLabels(map[string]string{ - trustapi.BundleLabelKey: bundle.Name, - }). - WithOwnerReferences( - metav1applyconfig.OwnerReference(). - WithAPIVersion(trustapi.SchemeGroupVersion.String()). - WithKind(trustapi.BundleKind). - WithName(bundle.GetName()). - WithUID(bundle.GetUID()). - WithBlockOwnerDeletion(true). - WithController(true), - ) - - if err = b.patchSecretResource(ctx, secretPatch); err != nil { - return false, fmt.Errorf("failed to patch secret %s/%s: %w", namespace, bundle.Name, err) + if !shouldExist { + // If the Secret is not found and should not exist we are done. + if apierrors.IsNotFound(err) { + return false, nil + } + // If the Secret should not exist, but it does, delete it. + // Apply empty patch to remove the keys + secret, err := r.patchSecret(ctx, newSecretPatch(name, *bundle)) + if err != nil { + return false, fmt.Errorf("failed to patch Secret %s: %w", name, err) } + // If the secret is empty, delete it + if secret != nil && len(secret.Data) == 0 { + return true, r.Client.Delete(ctx, secret) + } return true, nil } - target := bundle.Spec.Target - if target.Secret == nil { + if bundle.Spec.Target.Secret == nil { return false, errors.New("target not defined") } // Generated JKS is not deterministic - best we can do here is update if the pem cert has // changed (hence not checking if JKS matches) - dataHash := fmt.Sprintf("%x", sha256.Sum256([]byte(resolvedBundle.data))) + dataHash := fmt.Sprintf("%x", sha256.Sum256([]byte(data.Data))) targetData := map[string][]byte{ - target.Secret.Key: []byte(resolvedBundle.data), + bundle.Spec.Target.Secret.Key: []byte(data.Data), } - - for k, v := range resolvedBundle.binaryData { + for k, v := range data.BinaryData { targetData[k] = v } // If the Secret doesn't exist, create it. if !apierrors.IsNotFound(err) { // Exit early if no update is needed - if exit, err := b.needsUpdate(ctx, targetKindSecret, log, secret, bundle, dataHash); err != nil { + if exit, err := r.needsUpdate(ctx, KindSecret, log, targetObj, bundle, dataHash); err != nil { return false, err } else if !exit { return false, nil } } - secretPatch := coreapplyconfig. - Secret(name, namespace). - WithLabels(map[string]string{ - trustapi.BundleLabelKey: bundle.Name, - }). + secretPatch := newSecretPatch(name, *bundle). WithAnnotations(map[string]string{ trustapi.BundleHashAnnotationKey: dataHash, }). - WithOwnerReferences( - metav1applyconfig.OwnerReference(). - WithAPIVersion(trustapi.SchemeGroupVersion.String()). - WithKind(trustapi.BundleKind). - WithName(bundle.GetName()). - WithUID(bundle.GetUID()). - WithBlockOwnerDeletion(true). - WithController(true), - ). WithData(targetData) - if err = b.patchSecretResource(ctx, secretPatch); err != nil { - return false, fmt.Errorf("failed to patch Secret %s/%s: %w", namespace, bundle.Name, err) + if _, err = r.patchSecret(ctx, secretPatch); err != nil { + return false, fmt.Errorf("failed to patch Secret %s: %w", name, err) } log.V(2).Info("synced bundle to namespace for target Secret") @@ -263,14 +210,14 @@ func (b *bundle) syncSecretTarget( return true, nil } -type targetKind string +type Kind string const ( - targetKindConfigMap targetKind = "ConfigMap" - targetKindSecret targetKind = "Secret" + KindConfigMap Kind = "ConfigMap" + KindSecret Kind = "Secret" ) -func (b *bundle) needsUpdate(ctx context.Context, kind targetKind, log logr.Logger, obj *metav1.PartialObjectMetadata, bundle *trustapi.Bundle, dataHash string) (bool, error) { +func (r *Reconciler) needsUpdate(ctx context.Context, kind Kind, log logr.Logger, obj *metav1.PartialObjectMetadata, bundle *trustapi.Bundle, dataHash string) (bool, error) { needsUpdate := false if !metav1.IsControlledBy(obj, bundle) { needsUpdate = true @@ -288,17 +235,17 @@ func (b *bundle) needsUpdate(ctx context.Context, kind targetKind, log logr.Logg var key string var targetFieldNames []string switch kind { - case targetKindConfigMap: + case KindConfigMap: key = bundle.Spec.Target.ConfigMap.Key targetFieldNames = []string{"data", "binaryData"} - case targetKindSecret: + case KindSecret: key = bundle.Spec.Target.Secret.Key targetFieldNames = []string{"data"} default: return false, fmt.Errorf("unknown targetType: %s", kind) } - properties, err := listManagedProperties(obj, fieldManager, targetFieldNames...) + properties, err := listManagedProperties(obj, string(ssa_client.FieldManager), targetFieldNames...) if err != nil { return false, fmt.Errorf("failed to list managed properties: %w", err) } @@ -313,10 +260,10 @@ func (b *bundle) needsUpdate(ctx context.Context, kind targetKind, log logr.Logg needsUpdate = true } - if kind == targetKindConfigMap { + if kind == KindConfigMap { if bundle.Spec.Target.ConfigMap != nil { // Check if we need to migrate the ConfigMap managed fields to the Apply field operation - if didMigrate, err := b.migrateConfigMapToApply(ctx, obj); err != nil { + if didMigrate, err := r.migrateConfigMapToApply(ctx, obj); err != nil { return false, fmt.Errorf("failed to migrate ConfigMap %s/%s to Apply: %w", obj.Namespace, obj.Name, err) } else if didMigrate { log.V(2).Info("migrated configmap from CSA to SSA") @@ -360,69 +307,78 @@ func listManagedProperties(configmap *metav1.PartialObjectMetadata, fieldManager return properties, nil } -func (b *bundle) patchConfigMapResource(ctx context.Context, applyConfig *coreapplyconfig.ConfigMapApplyConfiguration) error { - if b.patchResourceOverwrite != nil { - return b.patchResourceOverwrite(ctx, applyConfig) +func (r *Reconciler) patchConfigMap(ctx context.Context, applyConfig *coreapplyconfig.ConfigMapApplyConfiguration) (*corev1.ConfigMap, error) { + if r.PatchResourceOverwrite != nil { + return nil, r.PatchResourceOverwrite(ctx, applyConfig) } - configMap, patch, err := ssa_client.GenerateConfigMapPatch(applyConfig) + target, patch, err := ssa_client.GenerateConfigMapPatch(applyConfig) if err != nil { - return fmt.Errorf("failed to generate patch: %w", err) - } - - err = b.client.Patch(ctx, configMap, patch, &client.PatchOptions{ - FieldManager: fieldManager, - Force: ptr.To(true), - }) - if err != nil { - return err - } - - // If the configMap is empty, delete it - if len(configMap.Data) == 0 && len(configMap.BinaryData) == 0 { - return b.client.Delete(ctx, configMap) + return nil, fmt.Errorf("failed to generate patch: %w", err) } - return nil + return target, r.Client.Patch(ctx, target, patch, ssa_client.FieldManager, client.ForceOwnership) } -func (b *bundle) patchSecretResource(ctx context.Context, applyConfig *coreapplyconfig.SecretApplyConfiguration) error { - if b.patchResourceOverwrite != nil { - return b.patchResourceOverwrite(ctx, applyConfig) +func (r *Reconciler) patchSecret(ctx context.Context, applyConfig *coreapplyconfig.SecretApplyConfiguration) (*corev1.Secret, error) { + if r.PatchResourceOverwrite != nil { + return nil, r.PatchResourceOverwrite(ctx, applyConfig) } - secret, patch, err := ssa_client.GenerateSecretPatch(applyConfig) + target, patch, err := ssa_client.GenerateSecretPatch(applyConfig) if err != nil { - return fmt.Errorf("failed to generate patch: %w", err) + return nil, fmt.Errorf("failed to generate patch: %w", err) } - err = b.client.Patch(ctx, secret, patch, &client.PatchOptions{ - FieldManager: fieldManager, - Force: ptr.To(true), - }) - if err != nil { - return err - } + return target, r.Client.Patch(ctx, target, patch, ssa_client.FieldManager, client.ForceOwnership) +} - // If the secret is empty, delete it - if len(secret.Data) == 0 { - return b.client.Delete(ctx, secret) - } +func newConfigMapPatch(name types.NamespacedName, bundle trustapi.Bundle) *coreapplyconfig.ConfigMapApplyConfiguration { + return coreapplyconfig. + ConfigMap(name.Name, name.Namespace). + WithLabels(map[string]string{ + trustapi.BundleLabelKey: bundle.Name, + }). + WithOwnerReferences( + metav1applyconfig.OwnerReference(). + WithAPIVersion(trustapi.SchemeGroupVersion.String()). + WithKind(trustapi.BundleKind). + WithName(bundle.GetName()). + WithUID(bundle.GetUID()). + WithBlockOwnerDeletion(true). + WithController(true), + ) +} - return nil +func newSecretPatch(name types.NamespacedName, bundle trustapi.Bundle) *coreapplyconfig.SecretApplyConfiguration { + return coreapplyconfig. + Secret(name.Name, name.Namespace). + WithLabels(map[string]string{ + trustapi.BundleLabelKey: bundle.Name, + }). + WithOwnerReferences( + metav1applyconfig.OwnerReference(). + WithAPIVersion(trustapi.SchemeGroupVersion.String()). + WithKind(trustapi.BundleKind). + WithName(bundle.GetName()). + WithUID(bundle.GetUID()). + WithBlockOwnerDeletion(true). + WithController(true), + ) } // MIGRATION: This is a migration function that migrates the ownership of // fields from the Update operation to the Apply operation. This is required // to ensure that the apply operations will also remove fields that were // created by the Update operation. -func (b *bundle) migrateConfigMapToApply(ctx context.Context, obj client.Object) (bool, error) { - patch, err := csaupgrade.UpgradeManagedFieldsPatch(obj, sets.New(fieldManager, crRegressionFieldManager), fieldManager) +func (r *Reconciler) migrateConfigMapToApply(ctx context.Context, obj client.Object) (bool, error) { + fieldManager := string(ssa_client.FieldManager) + patch, err := csaupgrade.UpgradeManagedFieldsPatch(obj, sets.New(fieldManager, ssa_client.CRRegressionFieldManager), fieldManager) if err != nil { return false, err } if patch != nil { - return true, b.client.Patch(ctx, obj, client.RawPatch(types.JSONPatchType, patch)) + return true, r.Client.Patch(ctx, obj, client.RawPatch(types.JSONPatchType, patch)) } // No work to be done - already upgraded return false, nil diff --git a/pkg/bundle/target_test.go b/pkg/bundle/internal/target/target_test.go similarity index 90% rename from pkg/bundle/target_test.go rename to pkg/bundle/internal/target/target_test.go index 7457aaeb..f1cc8161 100644 --- a/pkg/bundle/target_test.go +++ b/pkg/bundle/internal/target/target_test.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package bundle +package target import ( "context" @@ -27,15 +27,15 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" coreapplyconfig "k8s.io/client-go/applyconfigurations/core/v1" metav1applyconfig "k8s.io/client-go/applyconfigurations/meta/v1" - "k8s.io/client-go/tools/record" "k8s.io/klog/v2/ktesting" "k8s.io/utils/ptr" fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake" - "sigs.k8s.io/structured-merge-diff/fieldpath" trustapi "github.com/cert-manager/trust-manager/pkg/apis/trust/v1alpha1" + "github.com/cert-manager/trust-manager/pkg/bundle/internal/ssa_client" "github.com/cert-manager/trust-manager/test/dummy" ) @@ -53,35 +53,6 @@ var ( pkcs12Data = []byte("PKCS12") ) -func managedFieldEntries(fields []string, dataFields []string) []metav1.ManagedFieldsEntry { - fieldset := fieldpath.NewSet() - for _, property := range fields { - fieldset.Insert( - fieldpath.MakePathOrDie("data", property), - ) - } - for _, property := range dataFields { - fieldset.Insert( - fieldpath.MakePathOrDie("binaryData", property), - ) - } - - jsonFieldSet, err := fieldset.ToJSON() - if err != nil { - panic(err) - } - - return []metav1.ManagedFieldsEntry{ - { - Manager: "trust-manager", - Operation: metav1.ManagedFieldsOperationApply, - FieldsV1: &metav1.FieldsV1{ - Raw: jsonFieldSet, - }, - }, - } -} - func Test_syncConfigMapTarget(t *testing.T) { dataHash := fmt.Sprintf("%x", sha256.Sum256([]byte(data))) @@ -99,7 +70,6 @@ func Test_syncConfigMapTarget(t *testing.T) { expJKS bool // Expect PKCS12 to exist in the configmap at the end of the sync. expPKCS12 bool - expEvent string // Expect the owner reference of the configmap to point to the bundle. expOwnerReference bool expNeedsUpdate bool @@ -139,7 +109,7 @@ func Test_syncConfigMapTarget(t *testing.T) { Namespace: "test-namespace", Labels: map[string]string{trustapi.BundleLabelKey: bundleName}, Annotations: map[string]string{trustapi.BundleHashAnnotationKey: dataHash}, - ManagedFields: managedFieldEntries(nil, nil), + ManagedFields: ssa_client.ManagedFieldEntries(nil, nil), }, }, namespace: corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "test-namespace"}}, @@ -155,7 +125,7 @@ func Test_syncConfigMapTarget(t *testing.T) { Namespace: "test-namespace", Labels: map[string]string{trustapi.BundleLabelKey: bundleName}, Annotations: map[string]string{trustapi.BundleHashAnnotationKey: dataHash}, - ManagedFields: managedFieldEntries([]string{key}, nil), + ManagedFields: ssa_client.ManagedFieldEntries([]string{key}, nil), }, Data: map[string]string{key: data}, }, @@ -205,7 +175,7 @@ func Test_syncConfigMapTarget(t *testing.T) { BlockOwnerDeletion: ptr.To(true), }, }, - ManagedFields: managedFieldEntries([]string{key}, nil), + ManagedFields: ssa_client.ManagedFieldEntries([]string{key}, nil), }, Data: map[string]string{key: "wrong data"}, }, @@ -231,7 +201,7 @@ func Test_syncConfigMapTarget(t *testing.T) { BlockOwnerDeletion: ptr.To(true), }, }, - ManagedFields: managedFieldEntries([]string{key}, nil), + ManagedFields: ssa_client.ManagedFieldEntries([]string{key}, nil), }, Data: map[string]string{key: data}, }, @@ -259,7 +229,7 @@ func Test_syncConfigMapTarget(t *testing.T) { BlockOwnerDeletion: ptr.To(true), }, }, - ManagedFields: managedFieldEntries([]string{key}, nil), + ManagedFields: ssa_client.ManagedFieldEntries([]string{key}, nil), }, Data: map[string]string{key: data}, }, @@ -287,7 +257,7 @@ func Test_syncConfigMapTarget(t *testing.T) { BlockOwnerDeletion: ptr.To(true), }, }, - ManagedFields: managedFieldEntries([]string{"wrong key"}, nil), + ManagedFields: ssa_client.ManagedFieldEntries([]string{"wrong key"}, nil), }, BinaryData: map[string][]byte{"wrong key": []byte(data)}, }, @@ -313,7 +283,7 @@ func Test_syncConfigMapTarget(t *testing.T) { BlockOwnerDeletion: ptr.To(true), }, }, - ManagedFields: managedFieldEntries([]string{key}, []string{"wrong key"}), + ManagedFields: ssa_client.ManagedFieldEntries([]string{key}, []string{"wrong key"}), }, BinaryData: map[string][]byte{ key: []byte(data), @@ -344,7 +314,7 @@ func Test_syncConfigMapTarget(t *testing.T) { BlockOwnerDeletion: ptr.To(true), }, }, - ManagedFields: managedFieldEntries([]string{key, "wrong key"}, nil), + ManagedFields: ssa_client.ManagedFieldEntries([]string{key, "wrong key"}, nil), }, Data: map[string]string{ key: data, @@ -375,7 +345,7 @@ func Test_syncConfigMapTarget(t *testing.T) { BlockOwnerDeletion: ptr.To(true), }, }, - ManagedFields: managedFieldEntries([]string{key}, nil), + ManagedFields: ssa_client.ManagedFieldEntries([]string{key}, nil), }, Data: map[string]string{key: data}, }, @@ -401,7 +371,7 @@ func Test_syncConfigMapTarget(t *testing.T) { BlockOwnerDeletion: ptr.To(true), }, }, - ManagedFields: managedFieldEntries([]string{key}, nil), + ManagedFields: ssa_client.ManagedFieldEntries([]string{key}, nil), }, Data: map[string]string{key: data}, }, @@ -429,7 +399,7 @@ func Test_syncConfigMapTarget(t *testing.T) { BlockOwnerDeletion: ptr.To(true), }, }, - ManagedFields: managedFieldEntries([]string{key}, nil), + ManagedFields: ssa_client.ManagedFieldEntries([]string{key}, nil), }, Data: map[string]string{key: data}, }, @@ -464,7 +434,7 @@ func Test_syncConfigMapTarget(t *testing.T) { BlockOwnerDeletion: ptr.To(true), }, }, - ManagedFields: managedFieldEntries([]string{key}, nil), + ManagedFields: ssa_client.ManagedFieldEntries([]string{key}, nil), }, Data: map[string]string{ key: data, @@ -515,7 +485,7 @@ func Test_syncConfigMapTarget(t *testing.T) { BlockOwnerDeletion: ptr.To(true), }, }, - ManagedFields: managedFieldEntries([]string{key}, nil), + ManagedFields: ssa_client.ManagedFieldEntries([]string{key}, nil), }, Data: map[string]string{key: data}, }, @@ -544,7 +514,7 @@ func Test_syncConfigMapTarget(t *testing.T) { BlockOwnerDeletion: ptr.To(true), }, }, - ManagedFields: managedFieldEntries([]string{key}, nil), + ManagedFields: ssa_client.ManagedFieldEntries([]string{key}, nil), }, Data: map[string]string{key: data}, }, @@ -564,7 +534,7 @@ func Test_syncConfigMapTarget(t *testing.T) { Namespace: "test-namespace", Labels: map[string]string{trustapi.BundleLabelKey: bundleName}, Annotations: map[string]string{trustapi.BundleHashAnnotationKey: dataHash}, - ManagedFields: managedFieldEntries([]string{key}, nil), + ManagedFields: ssa_client.ManagedFieldEntries([]string{key}, nil), }, Data: map[string]string{key: data}, }, @@ -590,19 +560,17 @@ func Test_syncConfigMapTarget(t *testing.T) { clientBuilder.WithRuntimeObjects(test.object) } - fakeclient := clientBuilder.Build() - fakerecorder := record.NewFakeRecorder(1) + client := clientBuilder.Build() var ( logMutex sync.Mutex resourcePatches []interface{} ) - b := &bundle{ - client: fakeclient, - targetCache: fakeclient, - recorder: fakerecorder, - patchResourceOverwrite: func(ctx context.Context, obj interface{}) error { + r := &Reconciler{ + Client: client, + Cache: client, + PatchResourceOverwrite: func(ctx context.Context, obj interface{}) error { logMutex.Lock() defer logMutex.Unlock() @@ -617,14 +585,14 @@ func Test_syncConfigMapTarget(t *testing.T) { AdditionalFormats: &trustapi.AdditionalFormats{}, }, } - resolvedBundle := bundleData{data: data, binaryData: make(map[string][]byte)} + targetData := Data{Data: data, BinaryData: make(map[string][]byte)} if test.withJKS { spec.Target.AdditionalFormats.JKS = &trustapi.JKS{ KeySelector: trustapi.KeySelector{ Key: jksKey, }, } - resolvedBundle.binaryData[jksKey] = jksData + targetData.BinaryData[jksKey] = jksData } if test.withPKCS12 { spec.Target.AdditionalFormats.PKCS12 = &trustapi.PKCS12{ @@ -632,14 +600,14 @@ func Test_syncConfigMapTarget(t *testing.T) { Key: pkcs12Key, }, } - resolvedBundle.binaryData[pkcs12Key] = pkcs12Data + targetData.BinaryData[pkcs12Key] = pkcs12Data } log, ctx := ktesting.NewTestContext(t) - needsUpdate, err := b.syncConfigMapTarget(ctx, log, &trustapi.Bundle{ + needsUpdate, err := r.SyncConfigMap(ctx, log, &trustapi.Bundle{ ObjectMeta: metav1.ObjectMeta{Name: bundleName}, Spec: spec, - }, bundleName, test.namespace.Name, resolvedBundle, test.shouldExist) + }, types.NamespacedName{Name: bundleName, Namespace: test.namespace.Name}, targetData, test.shouldExist) assert.NoError(t, err) assert.Equalf(t, test.expNeedsUpdate, needsUpdate, "unexpected needsUpdate, exp=%t got=%t", test.expNeedsUpdate, needsUpdate) @@ -686,13 +654,6 @@ func Test_syncConfigMapTarget(t *testing.T) { assert.Equal(t, pkcs12Data, binData) } } - - var event string - select { - case event = <-fakerecorder.Events: - default: - } - assert.Equal(t, test.expEvent, event) }) } } @@ -719,7 +680,6 @@ func Test_syncSecretTarget(t *testing.T) { expJKS bool // Expect PKCS12 to exist in the secret at the end of the sync. expPKCS12 bool - expEvent string // Expect the owner reference of the secret to point to the bundle. expOwnerReference bool expNeedsUpdate bool @@ -759,7 +719,7 @@ func Test_syncSecretTarget(t *testing.T) { Namespace: "test-namespace", Labels: map[string]string{trustapi.BundleLabelKey: bundleName}, Annotations: map[string]string{trustapi.BundleHashAnnotationKey: dataHash}, - ManagedFields: managedFieldEntries(nil, nil), + ManagedFields: ssa_client.ManagedFieldEntries(nil, nil), }, }, namespace: corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "test-namespace"}}, @@ -775,7 +735,7 @@ func Test_syncSecretTarget(t *testing.T) { Namespace: "test-namespace", Labels: map[string]string{trustapi.BundleLabelKey: bundleName}, Annotations: map[string]string{trustapi.BundleHashAnnotationKey: dataHash}, - ManagedFields: managedFieldEntries([]string{key}, nil), + ManagedFields: ssa_client.ManagedFieldEntries([]string{key}, nil), }, Data: map[string][]byte{key: []byte(data)}, }, @@ -825,7 +785,7 @@ func Test_syncSecretTarget(t *testing.T) { BlockOwnerDeletion: ptr.To(true), }, }, - ManagedFields: managedFieldEntries([]string{key}, nil), + ManagedFields: ssa_client.ManagedFieldEntries([]string{key}, nil), }, Data: map[string][]byte{key: []byte("wrong data")}, }, @@ -851,7 +811,7 @@ func Test_syncSecretTarget(t *testing.T) { BlockOwnerDeletion: ptr.To(true), }, }, - ManagedFields: managedFieldEntries([]string{key}, nil), + ManagedFields: ssa_client.ManagedFieldEntries([]string{key}, nil), }, Data: map[string][]byte{key: []byte(data)}, }, @@ -879,7 +839,7 @@ func Test_syncSecretTarget(t *testing.T) { BlockOwnerDeletion: ptr.To(true), }, }, - ManagedFields: managedFieldEntries([]string{key}, nil), + ManagedFields: ssa_client.ManagedFieldEntries([]string{key}, nil), }, Data: map[string][]byte{key: []byte(data)}, }, @@ -907,7 +867,7 @@ func Test_syncSecretTarget(t *testing.T) { BlockOwnerDeletion: ptr.To(true), }, }, - ManagedFields: managedFieldEntries([]string{"wrong key"}, nil), + ManagedFields: ssa_client.ManagedFieldEntries([]string{"wrong key"}, nil), }, Data: map[string][]byte{"wrong key": []byte(data)}, }, @@ -933,7 +893,7 @@ func Test_syncSecretTarget(t *testing.T) { BlockOwnerDeletion: ptr.To(true), }, }, - ManagedFields: managedFieldEntries([]string{key}, []string{"wrong key"}), + ManagedFields: ssa_client.ManagedFieldEntries([]string{key}, []string{"wrong key"}), }, Data: map[string][]byte{ key: []byte(data), @@ -964,7 +924,7 @@ func Test_syncSecretTarget(t *testing.T) { BlockOwnerDeletion: ptr.To(true), }, }, - ManagedFields: managedFieldEntries([]string{key, "wrong key"}, nil), + ManagedFields: ssa_client.ManagedFieldEntries([]string{key, "wrong key"}, nil), }, Data: map[string][]byte{ key: []byte(data), @@ -995,7 +955,7 @@ func Test_syncSecretTarget(t *testing.T) { BlockOwnerDeletion: ptr.To(true), }, }, - ManagedFields: managedFieldEntries([]string{key}, nil), + ManagedFields: ssa_client.ManagedFieldEntries([]string{key}, nil), }, Data: map[string][]byte{key: []byte(data)}, }, @@ -1021,7 +981,7 @@ func Test_syncSecretTarget(t *testing.T) { BlockOwnerDeletion: ptr.To(true), }, }, - ManagedFields: managedFieldEntries([]string{key}, nil), + ManagedFields: ssa_client.ManagedFieldEntries([]string{key}, nil), }, Data: map[string][]byte{key: []byte(data)}, }, @@ -1049,7 +1009,7 @@ func Test_syncSecretTarget(t *testing.T) { BlockOwnerDeletion: ptr.To(true), }, }, - ManagedFields: managedFieldEntries([]string{key}, nil), + ManagedFields: ssa_client.ManagedFieldEntries([]string{key}, nil), }, Data: map[string][]byte{key: []byte(data)}, }, @@ -1084,7 +1044,7 @@ func Test_syncSecretTarget(t *testing.T) { BlockOwnerDeletion: ptr.To(true), }, }, - ManagedFields: managedFieldEntries([]string{key}, nil), + ManagedFields: ssa_client.ManagedFieldEntries([]string{key}, nil), }, Data: map[string][]byte{ key: []byte(data), @@ -1135,7 +1095,7 @@ func Test_syncSecretTarget(t *testing.T) { BlockOwnerDeletion: ptr.To(true), }, }, - ManagedFields: managedFieldEntries([]string{key}, nil), + ManagedFields: ssa_client.ManagedFieldEntries([]string{key}, nil), }, Data: map[string][]byte{key: []byte(data)}, }, @@ -1164,7 +1124,7 @@ func Test_syncSecretTarget(t *testing.T) { BlockOwnerDeletion: ptr.To(true), }, }, - ManagedFields: managedFieldEntries([]string{key}, nil), + ManagedFields: ssa_client.ManagedFieldEntries([]string{key}, nil), }, Data: map[string][]byte{key: []byte(data)}, }, @@ -1184,7 +1144,7 @@ func Test_syncSecretTarget(t *testing.T) { Namespace: "test-namespace", Labels: map[string]string{trustapi.BundleLabelKey: bundleName}, Annotations: map[string]string{trustapi.BundleHashAnnotationKey: dataHash}, - ManagedFields: managedFieldEntries([]string{key}, nil), + ManagedFields: ssa_client.ManagedFieldEntries([]string{key}, nil), }, Data: map[string][]byte{key: []byte(data)}, }, @@ -1210,19 +1170,17 @@ func Test_syncSecretTarget(t *testing.T) { clientBuilder.WithRuntimeObjects(test.object) } - fakeclient := clientBuilder.Build() - fakerecorder := record.NewFakeRecorder(1) + client := clientBuilder.Build() var ( logMutex sync.Mutex resourcePatches []interface{} ) - b := &bundle{ - client: fakeclient, - targetCache: fakeclient, - recorder: fakerecorder, - patchResourceOverwrite: func(ctx context.Context, obj interface{}) error { + r := &Reconciler{ + Client: client, + Cache: client, + PatchResourceOverwrite: func(ctx context.Context, obj interface{}) error { logMutex.Lock() defer logMutex.Unlock() @@ -1237,14 +1195,14 @@ func Test_syncSecretTarget(t *testing.T) { AdditionalFormats: &trustapi.AdditionalFormats{}, }, } - resolvedBundle := bundleData{data: data, binaryData: make(map[string][]byte)} + resolvedBundle := Data{Data: data, BinaryData: make(map[string][]byte)} if test.withJKS { spec.Target.AdditionalFormats.JKS = &trustapi.JKS{ KeySelector: trustapi.KeySelector{ Key: jksKey, }, } - resolvedBundle.binaryData[jksKey] = jksData + resolvedBundle.BinaryData[jksKey] = jksData } if test.withPKCS12 { spec.Target.AdditionalFormats.PKCS12 = &trustapi.PKCS12{ @@ -1252,14 +1210,14 @@ func Test_syncSecretTarget(t *testing.T) { Key: pkcs12Key, }, } - resolvedBundle.binaryData[pkcs12Key] = pkcs12Data + resolvedBundle.BinaryData[pkcs12Key] = pkcs12Data } log, ctx := ktesting.NewTestContext(t) - needsUpdate, err := b.syncSecretTarget(ctx, log, &trustapi.Bundle{ + needsUpdate, err := r.SyncSecret(ctx, log, &trustapi.Bundle{ ObjectMeta: metav1.ObjectMeta{Name: bundleName}, Spec: spec, - }, bundleName, test.namespace.Name, resolvedBundle, test.shouldExist) + }, types.NamespacedName{Name: bundleName, Namespace: test.namespace.Name}, resolvedBundle, test.shouldExist) assert.NoError(t, err) assert.Equalf(t, test.expNeedsUpdate, needsUpdate, "unexpected needsUpdate, exp=%t got=%t", test.expNeedsUpdate, needsUpdate) @@ -1304,13 +1262,6 @@ func Test_syncSecretTarget(t *testing.T) { assert.Equal(t, pkcs12Data, binData) } } - - var event string - select { - case event = <-fakerecorder.Events: - default: - } - assert.Equal(t, test.expEvent, event) }) } } diff --git a/pkg/bundle/source.go b/pkg/bundle/source.go index f6ea3a8f..e988ba14 100644 --- a/pkg/bundle/source.go +++ b/pkg/bundle/source.go @@ -20,41 +20,27 @@ import ( "bytes" "context" "crypto/sha256" - "encoding/hex" "encoding/pem" "fmt" "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/target" "github.com/cert-manager/trust-manager/pkg/util" ) -const ( - // DefaultJKSPassword is the default password that Java uses; it's a Java convention to use this exact password. - // Since we're not storing anything secret in the JKS files we generate, this password is not a meaningful security measure - // but seems often to be expected by applications consuming JKS files - DefaultJKSPassword = "changeit" - // DefaultPKCS12Password is the empty string, that will create a password-less PKCS12 truststore. - // Password-less PKCS is the new default Java truststore from Java 18. - // By password-less, it means the certificates are not encrypted, and it contains no MacData for integrity check. - DefaultPKCS12Password = "" -) - type notFoundError struct{ error } // bundleData holds the result of a call to buildSourceBundle. It contains the resulting PEM-encoded // certificate data from concatenating all the sources together, binary data for any additional formats and // any metadata from the sources which needs to be exposed on the Bundle resource's status field. type bundleData struct { - data string - binaryData map[string][]byte + target.Data defaultCAPackageStringID string } @@ -119,7 +105,7 @@ func (b *bundle) buildSourceBundle(ctx context.Context, sources []trustapi.Bundl return bundleData{}, err } - if err := resolvedBundle.populateData(deduplicatedBundles, formats); err != nil { + if err := resolvedBundle.Data.Populate(deduplicatedBundles, formats); err != nil { return bundleData{}, err } @@ -220,129 +206,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" - - if formats != nil { - b.binaryData = make(map[string][]byte) - - if formats.JKS != nil { - encoded, err := jksEncoder{password: *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) - if err != nil { - return fmt.Errorf("failed to encode PKCS12: %w", err) - } - b.binaryData[formats.PKCS12.Key] = encoded - } - } - return nil -} - // remove duplicate certificates from bundles func deduplicateBundles(bundles []string) ([]string, error) { var block *pem.Block diff --git a/pkg/bundle/source_test.go b/pkg/bundle/source_test.go index 5a91ad29..61dd3bf9 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" @@ -38,6 +37,12 @@ import ( "github.com/cert-manager/trust-manager/test/dummy" ) +const ( + jksKey = "trust.jks" + pkcs12Key = "trust.p12" + data = dummy.TestCertificate1 +) + func Test_buildSourceBundle(t *testing.T) { tests := map[string]struct { sources []trustapi.BundleSource @@ -217,7 +222,7 @@ func Test_buildSourceBundle(t *testing.T) { KeySelector: trustapi.KeySelector{ Key: jksKey, }, - Password: ptr.To(DefaultJKSPassword), + Password: ptr.To(trustapi.DefaultJKSPassword), }, }, objects: []runtime.Object{&corev1.ConfigMap{ @@ -256,7 +261,7 @@ func Test_buildSourceBundle(t *testing.T) { KeySelector: trustapi.KeySelector{ Key: pkcs12Key, }, - Password: ptr.To(DefaultPKCS12Password), + Password: ptr.To(trustapi.DefaultPKCS12Password), }, }, objects: []runtime.Object{&corev1.ConfigMap{ @@ -313,14 +318,14 @@ func Test_buildSourceBundle(t *testing.T) { if test.expPassword != nil { password = *test.expPassword } else { - password = DefaultJKSPassword + password = trustapi.DefaultJKSPassword } } if test.expPKCS12 { if test.expPassword != nil { password = *test.expPassword } else { - password = DefaultPKCS12Password + password = trustapi.DefaultPKCS12Password } } @@ -333,11 +338,11 @@ func Test_buildSourceBundle(t *testing.T) { t.Errorf("unexpected notFoundError, exp=%t got=%v", test.expNotFoundError, err) } - if resolvedBundle.data != test.expData { - t.Errorf("unexpected data, exp=%q got=%q", test.expData, resolvedBundle.data) + if resolvedBundle.Data.Data != test.expData { + t.Errorf("unexpected data, exp=%q got=%q", test.expData, resolvedBundle.Data.Data) } - binData, jksExists := resolvedBundle.binaryData[jksKey] + binData, jksExists := resolvedBundle.Data.BinaryData[jksKey] assert.Equal(t, test.expJKS, jksExists) if test.expJKS { @@ -361,7 +366,7 @@ func Test_buildSourceBundle(t *testing.T) { assert.Equal(t, p.Bytes, cert.Certificate.Content) } - binData, pkcs12Exists := resolvedBundle.binaryData[pkcs12Key] + binData, pkcs12Exists := resolvedBundle.Data.BinaryData[pkcs12Key] assert.Equal(t, test.expPKCS12, pkcs12Exists) if test.expPKCS12 { @@ -377,61 +382,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: 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(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 diff --git a/test/integration/bundle/suite.go b/test/integration/bundle/suite.go index c611bb12..58a8d7fe 100644 --- a/test/integration/bundle/suite.go +++ b/test/integration/bundle/suite.go @@ -352,7 +352,7 @@ var _ = Describe("Integration", func() { jksData, exists := configMap.BinaryData["myfile.jks"] Expect(exists).To(BeTrue(), "should find an entry called myfile.jks") - Expect(testenv.CheckJKSFileSynced(jksData, bundle.DefaultJKSPassword, dummy.DefaultJoinedCerts())).ToNot(HaveOccurred()) + Expect(testenv.CheckJKSFileSynced(jksData, trustapi.DefaultJKSPassword, dummy.DefaultJoinedCerts())).ToNot(HaveOccurred()) } })