Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to inline secrets from SecretRef to configmap. #1595

Merged
merged 1 commit into from
Mar 11, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 57 additions & 15 deletions pkg/reconciler/clusterimagepolicy/clusterimagepolicy.go
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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll do that in a followup?
#1596

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
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