Skip to content

Commit

Permalink
Add ability to inline secrets from SecretRef to configmap. (#1595)
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 authored Mar 11, 2022
1 parent 1849111 commit 3eadb75
Show file tree
Hide file tree
Showing 4 changed files with 315 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 3eadb75

Please sign in to comment.