Skip to content

Commit

Permalink
Add ability to inline secrets from SecretRef to configmap.
Browse files Browse the repository at this point in the history 
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 <[email protected]>
  • Loading branch information
@vaikas
vaikas committed Mar 11, 2022
1 parent 0d7bace commit 917d370
Show file tree
Hide file tree
Showing 4 changed files with 316 additions and 23 deletions.
72 changes: 57 additions & 15 deletions pkg/reconciler/clusterimagepolicy/clusterimagepolicy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
}
219 changes: 212 additions & 7 deletions pkg/reconciler/clusterimagepolicy/clusterimagepolicy_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,19 +31,24 @@ 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"
_ "knative.dev/pkg/system/testing"
)

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-----
Expand All @@ -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) {
Expand Down Expand Up @@ -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)
Expand All @@ -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(),
Expand All @@ -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{
Expand Down
Loading

0 comments on commit 917d370

Please sign in to comment.