From 917d3706320f574cb1aa6f4510c976a689269153 Mon Sep 17 00:00:00 2001 From: Ville Aikas Date: Fri, 11 Mar 2022 15:36:06 +0200 Subject: [PATCH] Add ability to inline secrets from SecretRef to configmap. Use tracker to keep track of changes to secrets, for example if a secret doesn't exist initially, once it shows up. Tested obvs with UT but also tested on a real cluster that the tracker is keeping track of changes to secrets. Fix #1573 Signed-off-by: Ville Aikas --- .../clusterimagepolicy/clusterimagepolicy.go | 72 ++++-- .../clusterimagepolicy_test.go | 219 +++++++++++++++++- .../clusterimagepolicy/controller.go | 11 + pkg/reconciler/testing/v1alpha1/factory.go | 37 ++- 4 files changed, 316 insertions(+), 23 deletions(-) diff --git a/pkg/reconciler/clusterimagepolicy/clusterimagepolicy.go b/pkg/reconciler/clusterimagepolicy/clusterimagepolicy.go index 8f30beb56d70..ccbc5f4bdf6c 100644 --- a/pkg/reconciler/clusterimagepolicy/clusterimagepolicy.go +++ b/pkg/reconciler/clusterimagepolicy/clusterimagepolicy.go @@ -16,16 +16,15 @@ package clusterimagepolicy import ( "context" - "errors" + "fmt" "github.com/sigstore/cosign/pkg/apis/config" - "k8s.io/apimachinery/pkg/types" - "github.com/sigstore/cosign/pkg/apis/cosigned/v1alpha1" clusterimagepolicyreconciler "github.com/sigstore/cosign/pkg/client/injection/reconciler/cosigned/v1alpha1/clusterimagepolicy" "github.com/sigstore/cosign/pkg/reconciler/clusterimagepolicy/resources" apierrs "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/kubernetes" corev1listers "k8s.io/client-go/listers/core/v1" "knative.dev/pkg/logging" @@ -53,8 +52,9 @@ var _ clusterimagepolicyreconciler.Finalizer = (*Reconciler)(nil) // ReconcileKind implements Interface.ReconcileKind. func (r *Reconciler) ReconcileKind(ctx context.Context, cip *v1alpha1.ClusterImagePolicy) reconciler.Event { - if !willItBlend(cip) { - return errors.New("i can't do that yet, only support keys inlined or KMS") + cipCopy, err := r.inlineSecrets(ctx, cip) + if err != nil { + return err } // See if the CM holding configs exists existing, err := r.configmaplister.ConfigMaps(system.Namespace()).Get(config.ImagePoliciesConfigName) @@ -64,7 +64,7 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, cip *v1alpha1.ClusterIma return err } // Does not exist, create it. - cm, err := resources.NewConfigMap(system.Namespace(), config.ImagePoliciesConfigName, cip) + cm, err := resources.NewConfigMap(system.Namespace(), config.ImagePoliciesConfigName, cipCopy) if err != nil { logging.FromContext(ctx).Errorf("Failed to construct configmap: %v", err) return err @@ -74,7 +74,7 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, cip *v1alpha1.ClusterIma } // Check if we need to update the configmap or not. - patchBytes, err := resources.CreatePatch(system.Namespace(), config.ImagePoliciesConfigName, existing.DeepCopy(), cip) + patchBytes, err := resources.CreatePatch(system.Namespace(), config.ImagePoliciesConfigName, existing.DeepCopy(), cipCopy) if err != nil { logging.FromContext(ctx).Errorf("Failed to create patch: %v", err) return err @@ -116,18 +116,60 @@ func (r *Reconciler) FinalizeKind(ctx context.Context, cip *v1alpha1.ClusterImag return nil } -// Checks to see if we can deal with format yet. This is missing support -// for things like Secret resolution, so we can't do those yet. As more things -// are supported, remove them from here. -func willItBlend(cip *v1alpha1.ClusterImagePolicy) bool { - for _, authority := range cip.Spec.Authorities { +// inlineSecrets will go through the CIP and try to read the referenced +// secrets and convert them into inlined data. Makes a copy of the CIP +// before modifying it and returns the copy. +func (r *Reconciler) inlineSecrets(ctx context.Context, cip *v1alpha1.ClusterImagePolicy) (*v1alpha1.ClusterImagePolicy, error) { + ret := cip.DeepCopy() + for _, authority := range ret.Spec.Authorities { if authority.Key != nil && authority.Key.SecretRef != nil { - return false + if err := r.inlineAndTrackSecret(ctx, ret, authority.Key); err != nil { + logging.FromContext(ctx).Errorf("Failed to read secret %q: %w", authority.Key.SecretRef.Name) + return nil, err + } } if authority.Keyless != nil && authority.Keyless.CAKey != nil && authority.Keyless.CAKey.SecretRef != nil { - return false + if err := r.inlineAndTrackSecret(ctx, ret, authority.Keyless.CAKey); err != nil { + logging.FromContext(ctx).Errorf("Failed to read secret %q: %w", authority.Keyless.CAKey.SecretRef.Name) + return nil, err + } } } - return true + return ret, nil +} + +// inlineSecret will take in a KeyRef and tries to read the Secret, finding the +// first key from it and will inline it in place of Data and then clear out +// the SecretRef and return it. +// Additionally, we set up a tracker so we will be notified if the secret +// is modified. +// There's still some discussion about how to handle multiple keys in a secret +// for now, just grab one from it. For reference, the discussion is here: +// TODO(vaikas): https://github.com/sigstore/cosign/issues/1573 +func (r *Reconciler) inlineAndTrackSecret(ctx context.Context, cip *v1alpha1.ClusterImagePolicy, keyref *v1alpha1.KeyRef) error { + if err := r.tracker.TrackReference(tracker.Reference{ + APIVersion: "v1", + Kind: "Secret", + Namespace: system.Namespace(), + Name: keyref.SecretRef.Name, + }, cip); err != nil { + return fmt.Errorf("Failed to track changes to secret %q : %w", keyref.SecretRef.Name, err) + } + secret, err := r.secretlister.Secrets(system.Namespace()).Get(keyref.SecretRef.Name) + if err != nil { + return err + } + if len(secret.Data) == 0 { + return fmt.Errorf("secret %q contains no data", keyref.SecretRef.Name) + } + if len(secret.Data) > 1 { + return fmt.Errorf("secret %q contains multiple data entries, only one is supported", keyref.SecretRef.Name) + } + for k, v := range secret.Data { + logging.FromContext(ctx).Infof("inlining secret %q key %q", keyref.SecretRef.Name, k) + keyref.Data = string(v) + keyref.SecretRef = nil + } + return nil } diff --git a/pkg/reconciler/clusterimagepolicy/clusterimagepolicy_test.go b/pkg/reconciler/clusterimagepolicy/clusterimagepolicy_test.go index b362731d447e..dff1f6f3c916 100644 --- a/pkg/reconciler/clusterimagepolicy/clusterimagepolicy_test.go +++ b/pkg/reconciler/clusterimagepolicy/clusterimagepolicy_test.go @@ -31,6 +31,7 @@ import ( "knative.dev/pkg/controller" logtesting "knative.dev/pkg/logging/testing" "knative.dev/pkg/system" + "knative.dev/pkg/tracker" . "github.com/sigstore/cosign/pkg/reconciler/testing/v1alpha1" . "knative.dev/pkg/reconciler/testing" @@ -38,12 +39,16 @@ import ( ) const ( - cipName = "test-cip" - testKey = "test-cip" - cipName2 = "test-cip-2" - testKey2 = "test-cip-2" - glob = "ghcr.io/example/*" - kms = "azure-kms://foo/bar" + cipName = "test-cip" + testKey = "test-cip" + cipName2 = "test-cip-2" + testKey2 = "test-cip-2" + keySecretName = "publickey-key" + keySecretValue = "keyref secret here" + keylessSecretName = "publickey-keyless" + keylessSecretValue = "keyless cakeyref secret here" + glob = "ghcr.io/example/*" + kms = "azure-kms://foo/bar" // Just some public key that was laying around, only format matters. inlineKeyData = `-----BEGIN PUBLIC KEY----- @@ -61,9 +66,15 @@ RCTfQ5s1kD+hGMSE1rH7s46hmXEeyhnlRnaGF8eMU/SBJE/2NKPnxE7WzQ== // configmap objectmeta, no data. removeDataPatch = `[{"op":"remove","path":"/data"}]` - // THis is the patch for removing only a single entry from a map that has + // This is the patch for removing only a single entry from a map that has // two entries but only one is being removed. removeSingleEntryPatch = `[{"op":"remove","path":"/data/test-cip-2"}]` + + // This is the patch for inlined secret for key ref data + inlinedSecretKeyPatch = `[{"op":"replace","path":"/data/test-cip","value":"{\"images\":[{\"glob\":\"ghcr.io/example/*\",\"regex\":\"\"}],\"authorities\":[{\"key\":{\"data\":\"keyref secret here\"}}]}"}]` + + // This is the patch for inlined secret for keyless cakey ref data + inlinedSecretKeylessPatch = `[{"op":"replace","path":"/data/test-cip-2","value":"{\"images\":[{\"glob\":\"ghcr.io/example/*\",\"regex\":\"\"}],\"authorities\":[{\"keyless\":{\"ca-key\":{\"data\":\"keyless cakeyref secret here\"}}}]}"}]` ) func TestReconcile(t *testing.T) { @@ -215,6 +226,187 @@ func TestReconcile(t *testing.T) { WantEvents: []string{ Eventf(corev1.EventTypeNormal, "FinalizerUpdate", `Updated "test-cip-2" finalizers`), }, + }, { + Name: "Key with secret, secret does not exist.", + Key: testKey, + + SkipNamespaceValidation: true, // Cluster scoped + Objects: []runtime.Object{ + NewClusterImagePolicy(cipName, + WithFinalizer, + WithImagePattern(v1alpha1.ImagePattern{ + Glob: glob, + }), + WithAuthority(v1alpha1.Authority{ + Key: &v1alpha1.KeyRef{ + SecretRef: &corev1.SecretReference{ + Name: keySecretName, + }, + }}), + ), + makeConfigMapWithTwoEntries(), + }, + WantErr: true, + WantEvents: []string{ + Eventf(corev1.EventTypeWarning, "InternalError", `secret "publickey-key" not found`), + }, + PostConditions: []func(*testing.T, *TableRow){ + AssertTrackingSecret(system.Namespace(), keySecretName), + }, + }, { + Name: "Keyless with secret, secret does not exist.", + Key: testKey, + + SkipNamespaceValidation: true, // Cluster scoped + Objects: []runtime.Object{ + NewClusterImagePolicy(cipName, + WithFinalizer, + WithImagePattern(v1alpha1.ImagePattern{ + Glob: glob, + }), + WithAuthority(v1alpha1.Authority{ + Keyless: &v1alpha1.KeylessRef{ + CAKey: &v1alpha1.KeyRef{ + SecretRef: &corev1.SecretReference{ + Name: keylessSecretName, + }, + }, + }}), + ), + makeConfigMapWithTwoEntries(), + }, + WantErr: true, + WantEvents: []string{ + Eventf(corev1.EventTypeWarning, "InternalError", `secret "publickey-keyless" not found`), + }, + PostConditions: []func(*testing.T, *TableRow){ + AssertTrackingSecret(system.Namespace(), keylessSecretName), + }, + }, { + Name: "Key with secret, no data.", + Key: testKey, + + SkipNamespaceValidation: true, // Cluster scoped + Objects: []runtime.Object{ + NewClusterImagePolicy(cipName, + WithFinalizer, + WithImagePattern(v1alpha1.ImagePattern{ + Glob: glob, + }), + WithAuthority(v1alpha1.Authority{ + Key: &v1alpha1.KeyRef{ + SecretRef: &corev1.SecretReference{ + Name: keySecretName, + }, + }}), + ), + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: system.Namespace(), + Name: keySecretName, + }, + }, + makeConfigMapWithTwoEntries(), + }, + WantErr: true, + WantEvents: []string{ + Eventf(corev1.EventTypeWarning, "InternalError", `secret "publickey-key" contains no data`), + }, + PostConditions: []func(*testing.T, *TableRow){ + AssertTrackingSecret(system.Namespace(), keySecretName), + }, + }, { + Name: "Key with secret, multiple data entries.", + Key: testKey, + + SkipNamespaceValidation: true, // Cluster scoped + Objects: []runtime.Object{ + NewClusterImagePolicy(cipName, + WithFinalizer, + WithImagePattern(v1alpha1.ImagePattern{ + Glob: glob, + }), + WithAuthority(v1alpha1.Authority{ + Key: &v1alpha1.KeyRef{ + SecretRef: &corev1.SecretReference{ + Name: keySecretName, + }, + }}), + ), + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: system.Namespace(), + Name: keySecretName, + }, + Data: map[string][]byte{ + "first": []byte("first data"), + "second": []byte("second data"), + }, + }, + makeConfigMapWithTwoEntries(), + }, + WantErr: true, + WantEvents: []string{ + Eventf(corev1.EventTypeWarning, "InternalError", `secret "publickey-key" contains multiple data entries, only one is supported`), + }, + PostConditions: []func(*testing.T, *TableRow){ + AssertTrackingSecret(system.Namespace(), keySecretName), + }, + }, { + Name: "Key with secret, secret exists, inlined", + Key: testKey, + + SkipNamespaceValidation: true, // Cluster scoped + Objects: []runtime.Object{ + NewClusterImagePolicy(cipName, + WithFinalizer, + WithImagePattern(v1alpha1.ImagePattern{ + Glob: glob, + }), + WithAuthority(v1alpha1.Authority{ + Key: &v1alpha1.KeyRef{ + SecretRef: &corev1.SecretReference{ + Name: keySecretName, + }, + }}), + ), + makeConfigMapWithTwoEntries(), + makeSecret(keySecretName, keySecretValue), + }, + WantPatches: []clientgotesting.PatchActionImpl{ + makePatch(inlinedSecretKeyPatch), + }, + PostConditions: []func(*testing.T, *TableRow){ + AssertTrackingSecret(system.Namespace(), keySecretName), + }, + }, { + Name: "Keyless with secret, secret exists, inlined", + Key: testKey2, + + SkipNamespaceValidation: true, // Cluster scoped + Objects: []runtime.Object{ + NewClusterImagePolicy(cipName2, + WithFinalizer, + WithImagePattern(v1alpha1.ImagePattern{ + Glob: glob, + }), + WithAuthority(v1alpha1.Authority{ + Keyless: &v1alpha1.KeylessRef{ + CAKey: &v1alpha1.KeyRef{ + SecretRef: &corev1.SecretReference{ + Name: keylessSecretName, + }}, + }}), + ), + makeConfigMapWithTwoEntries(), + makeSecret(keylessSecretName, keylessSecretValue), + }, + WantPatches: []clientgotesting.PatchActionImpl{ + makePatch(inlinedSecretKeylessPatch), + }, + PostConditions: []func(*testing.T, *TableRow){ + AssertTrackingSecret(system.Namespace(), keylessSecretName), + }, }, {}} logger := logtesting.TestLogger(t) @@ -223,6 +415,7 @@ func TestReconcile(t *testing.T) { secretlister: listers.GetSecretLister(), configmaplister: listers.GetConfigMapLister(), kubeclient: fakekubeclient.Get(ctx), + tracker: ctx.Value(TrackerKey).(tracker.Interface), } return clusterimagepolicy.NewReconciler(ctx, logger, fakecosignclient.Get(ctx), listers.GetClusterImagePolicyLister(), @@ -234,6 +427,18 @@ func TestReconcile(t *testing.T) { )) } +func makeSecret(name, secret string) *corev1.Secret { + return &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: system.Namespace(), + Name: name, + }, + Data: map[string][]byte{ + "publicKey": []byte(secret), + }, + } +} + func makeConfigMap() *corev1.ConfigMap { return &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ diff --git a/pkg/reconciler/clusterimagepolicy/controller.go b/pkg/reconciler/clusterimagepolicy/controller.go index e46480202db3..72782b5ce54e 100644 --- a/pkg/reconciler/clusterimagepolicy/controller.go +++ b/pkg/reconciler/clusterimagepolicy/controller.go @@ -17,6 +17,7 @@ package clusterimagepolicy import ( "context" + corev1 "k8s.io/api/core/v1" "k8s.io/client-go/tools/cache" kubeclient "knative.dev/pkg/client/injection/kube/client" "knative.dev/pkg/configmap" @@ -68,6 +69,16 @@ func NewController( clusterimagepolicyInformer.Informer().AddEventHandler(controller.HandleAll(impl.Enqueue)) + nsSecretInformer.Informer().AddEventHandler(controller.HandleAll( + // Call the tracker's OnChanged method, but we've seen the objects + // coming through this path missing TypeMeta, so ensure it is properly + // populated. + controller.EnsureTypeMeta( + r.tracker.OnChanged, + corev1.SchemeGroupVersion.WithKind("Secret"), + ), + )) + // When the underlying ConfigMap changes,perform a global resync on // ClusterImagePolicies to make sure their state is correctly reflected // in the ConfigMap. This is admittedly a bit heavy handed, but I don't diff --git a/pkg/reconciler/testing/v1alpha1/factory.go b/pkg/reconciler/testing/v1alpha1/factory.go index 2dcbd32ab18a..06f1c2519f95 100644 --- a/pkg/reconciler/testing/v1alpha1/factory.go +++ b/pkg/reconciler/testing/v1alpha1/factory.go @@ -26,6 +26,7 @@ import ( "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/tools/record" @@ -68,6 +69,7 @@ func MakeFactory(ctor Ctor, unstructured bool, logger *zap.SugaredLogger) reconc ctx, client := fakecosignclient.With(ctx, ls.GetCosignObjects()...) ctx, dynamicClient := fakedynamicclient.With(ctx, NewScheme(), ToUnstructured(t, r.Objects)...) + ctx = context.WithValue(ctx, TrackerKey, &reconcilertesting.FakeTracker{}) // The dynamic client's support for patching is BS. Implement it // here via PrependReactor (this can be overridden below by the @@ -91,7 +93,7 @@ func MakeFactory(ctor Ctor, unstructured bool, logger *zap.SugaredLogger) reconc // Set up our Controller from the fakes. c := ctor(ctx, &ls, configMapWatcher) - + r.Ctx = ctx // If the reconcilers is leader aware, then promote it. if la, ok := c.(reconciler.LeaderAware); ok { if la.Promote(reconciler.UniversalBucket(), func(reconciler.Bucket, types.NamespacedName) {}) != nil { @@ -153,3 +155,36 @@ func ToUnstructured(t *testing.T, objs []runtime.Object) (us []runtime.Object) { } return } + +type key struct{} + +// TrackerKey is used to looking a FakeTracker in a context.Context +var TrackerKey key = struct{}{} + +// AssertTrackingSecret will ensure the provided Secret is being tracked +func AssertTrackingSecret(namespace, name string) func(*testing.T, *reconcilertesting.TableRow) { + gvk := corev1.SchemeGroupVersion.WithKind("Secret") + return AssertTrackingObject(gvk, namespace, name) +} + +// AssertTrackingObject will ensure the following objects are being tracked +func AssertTrackingObject(gvk schema.GroupVersionKind, namespace, name string) func(*testing.T, *reconcilertesting.TableRow) { + apiVersion, kind := gvk.ToAPIVersionAndKind() + + return func(t *testing.T, r *reconcilertesting.TableRow) { + tracker := r.Ctx.Value(TrackerKey).(*reconcilertesting.FakeTracker) + refs := tracker.References() + + for _, ref := range refs { + if ref.APIVersion == apiVersion && + ref.Name == name && + ref.Namespace == namespace && + ref.Kind == kind { + return + } + } + + t.Errorf("Object was not tracked - %s, Name=%s, Namespace=%s", gvk.String(), name, namespace) + } + +}