From 59a06fd2f907cc21f2df4dedd0b6f82099ccfe5e Mon Sep 17 00:00:00 2001 From: Ville Aikas Date: Mon, 14 Mar 2022 10:35:09 +0200 Subject: [PATCH 1/2] Validate a public key in a secret is valid. Fixes: #1596 secret key contains valid public key. Signed-off-by: Ville Aikas --- .../clusterimagepolicy/clusterimagepolicy.go | 9 ++- .../clusterimagepolicy_test.go | 68 +++++++++++++++---- 2 files changed, 62 insertions(+), 15 deletions(-) diff --git a/pkg/reconciler/clusterimagepolicy/clusterimagepolicy.go b/pkg/reconciler/clusterimagepolicy/clusterimagepolicy.go index 0bea1b82f46..620f90c1066 100644 --- a/pkg/reconciler/clusterimagepolicy/clusterimagepolicy.go +++ b/pkg/reconciler/clusterimagepolicy/clusterimagepolicy.go @@ -20,6 +20,7 @@ import ( "github.com/sigstore/cosign/pkg/apis/config" "github.com/sigstore/cosign/pkg/apis/cosigned/v1alpha1" + "github.com/sigstore/cosign/pkg/apis/utils" 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" @@ -124,14 +125,14 @@ func (r *Reconciler) inlineSecrets(ctx context.Context, cip *v1alpha1.ClusterIma for _, authority := range ret.Spec.Authorities { if authority.Key != nil && authority.Key.SecretRef != nil { if err := r.inlineAndTrackSecret(ctx, ret, authority.Key); err != nil { - logging.FromContext(ctx).Errorf("Failed to read secret %q: %w", authority.Key.SecretRef.Name) + logging.FromContext(ctx).Errorf("Failed to read secret %q: %v", authority.Key.SecretRef.Name, err) return nil, err } } if authority.Keyless != nil && authority.Keyless.CAKey != nil && authority.Keyless.CAKey.SecretRef != nil { 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) + logging.FromContext(ctx).Errorf("Failed to read secret %q: %v", authority.Keyless.CAKey.SecretRef.Name, err) return nil, err } } @@ -168,6 +169,10 @@ func (r *Reconciler) inlineAndTrackSecret(ctx context.Context, cip *v1alpha1.Clu } for k, v := range secret.Data { logging.FromContext(ctx).Infof("inlining secret %q key %q", keyref.SecretRef.Name, k) + if !utils.IsValidKey(v) { + return fmt.Errorf("secret %q contains an invalid public key", keyref.SecretRef.Name) + } + keyref.Data = string(v) keyref.SecretRef = nil } diff --git a/pkg/reconciler/clusterimagepolicy/clusterimagepolicy_test.go b/pkg/reconciler/clusterimagepolicy/clusterimagepolicy_test.go index dff1f6f3c91..3e92dd7e5ec 100644 --- a/pkg/reconciler/clusterimagepolicy/clusterimagepolicy_test.go +++ b/pkg/reconciler/clusterimagepolicy/clusterimagepolicy_test.go @@ -44,14 +44,13 @@ const ( 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----- + validPublicKeyData = `-----BEGIN PUBLIC KEY----- MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAExB6+H6054/W1SJgs5JR6AJr6J35J RCTfQ5s1kD+hGMSE1rH7s46hmXEeyhnlRnaGF8eMU/SBJE/2NKPnxE7WzQ== -----END PUBLIC KEY-----` @@ -71,10 +70,10 @@ RCTfQ5s1kD+hGMSE1rH7s46hmXEeyhnlRnaGF8eMU/SBJE/2NKPnxE7WzQ== 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\"}}]}"}]` + inlinedSecretKeyPatch = `[{"op":"replace","path":"/data/test-cip","value":"{\"images\":[{\"glob\":\"ghcr.io/example/*\",\"regex\":\"\"}],\"authorities\":[{\"key\":{\"data\":\"-----BEGIN PUBLIC KEY-----\\nMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAExB6+H6054/W1SJgs5JR6AJr6J35J\\nRCTfQ5s1kD+hGMSE1rH7s46hmXEeyhnlRnaGF8eMU/SBJE/2NKPnxE7WzQ==\\n-----END PUBLIC KEY-----\"}}]}"}]` // 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\"}}}]}"}]` + inlinedSecretKeylessPatch = `[{"op":"replace","path":"/data/test-cip-2","value":"{\"images\":[{\"glob\":\"ghcr.io/example/*\",\"regex\":\"\"}],\"authorities\":[{\"keyless\":{\"ca-key\":{\"data\":\"-----BEGIN PUBLIC KEY-----\\nMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAExB6+H6054/W1SJgs5JR6AJr6J35J\\nRCTfQ5s1kD+hGMSE1rH7s46hmXEeyhnlRnaGF8eMU/SBJE/2NKPnxE7WzQ==\\n-----END PUBLIC KEY-----\"}}}]}"}]` ) func TestReconcile(t *testing.T) { @@ -108,7 +107,7 @@ func TestReconcile(t *testing.T) { }), WithAuthority(v1alpha1.Authority{ Key: &v1alpha1.KeyRef{ - Data: inlineKeyData, + Data: validPublicKeyData, }}))}, WantCreates: []runtime.Object{ makeConfigMap(), @@ -132,7 +131,7 @@ func TestReconcile(t *testing.T) { }), WithAuthority(v1alpha1.Authority{ Key: &v1alpha1.KeyRef{ - Data: inlineKeyData, + Data: validPublicKeyData, }})), makeConfigMap(), }, @@ -149,7 +148,7 @@ func TestReconcile(t *testing.T) { }), WithAuthority(v1alpha1.Authority{ Key: &v1alpha1.KeyRef{ - Data: inlineKeyData, + Data: validPublicKeyData, }})), makeDifferentConfigMap(), }, @@ -189,7 +188,7 @@ func TestReconcile(t *testing.T) { }), WithAuthority(v1alpha1.Authority{ Key: &v1alpha1.KeyRef{ - Data: inlineKeyData, + Data: validPublicKeyData, }}), WithClusterImagePolicyDeletionTimestamp), makeConfigMap(), @@ -214,7 +213,7 @@ func TestReconcile(t *testing.T) { }), WithAuthority(v1alpha1.Authority{ Key: &v1alpha1.KeyRef{ - Data: inlineKeyData, + Data: validPublicKeyData, }}), WithClusterImagePolicyDeletionTimestamp), makeConfigMapWithTwoEntries(), @@ -353,7 +352,7 @@ func TestReconcile(t *testing.T) { AssertTrackingSecret(system.Namespace(), keySecretName), }, }, { - Name: "Key with secret, secret exists, inlined", + Name: "Key with secret, secret exists, invalid public key", Key: testKey, SkipNamespaceValidation: true, // Cluster scoped @@ -371,7 +370,35 @@ func TestReconcile(t *testing.T) { }}), ), makeConfigMapWithTwoEntries(), - makeSecret(keySecretName, keySecretValue), + makeSecret(keySecretName, "garbage secret value, not a public key"), + }, + WantErr: true, + WantEvents: []string{ + Eventf(corev1.EventTypeWarning, "InternalError", `secret "publickey-key" contains an invalid public key`), + }, + 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, + }, + }}), + ), + makeConfigMapWithTwoEntriesNotPublicKeyFromSecret(), + makeSecret(keySecretName, validPublicKeyData), }, WantPatches: []clientgotesting.PatchActionImpl{ makePatch(inlinedSecretKeyPatch), @@ -399,7 +426,7 @@ func TestReconcile(t *testing.T) { }}), ), makeConfigMapWithTwoEntries(), - makeSecret(keylessSecretName, keylessSecretValue), + makeSecret(keylessSecretName, validPublicKeyData), }, WantPatches: []clientgotesting.PatchActionImpl{ makePatch(inlinedSecretKeylessPatch), @@ -464,7 +491,7 @@ func makeDifferentConfigMap() *corev1.ConfigMap { } } -// Same as MakeConfigMap but a placeholder for secont entry so we can remove it. +// Same as MakeConfigMap but a placeholder for second entry so we can remove it. func makeConfigMapWithTwoEntries() *corev1.ConfigMap { return &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ @@ -478,6 +505,21 @@ func makeConfigMapWithTwoEntries() *corev1.ConfigMap { } } +// Same as MakeConfigMapWithTwoEntries but the inline data is not the secret +// so we will replace it with the secret data +func makeConfigMapWithTwoEntriesNotPublicKeyFromSecret() *corev1.ConfigMap { + return &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: system.Namespace(), + Name: config.ImagePoliciesConfigName, + }, + Data: map[string]string{ + cipName: `{"images":[{"glob":"ghcr.io/example/*","regex":""}],"authorities":[{"key":{"data":"NOT A REAL PUBLIC KEY"}}]}`, + cipName2: "remove me please", + }, + } +} + func makePatch(patch string) clientgotesting.PatchActionImpl { return clientgotesting.PatchActionImpl{ ActionImpl: clientgotesting.ActionImpl{ From f882d4bdbb2eca022bd605e11a14bfc1fca33348 Mon Sep 17 00:00:00 2001 From: Ville Aikas Date: Mon, 14 Mar 2022 10:57:26 +0200 Subject: [PATCH 2/2] Remove unused variable. Signed-off-by: Ville Aikas --- .../clusterimagepolicy_test.go | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/pkg/reconciler/clusterimagepolicy/clusterimagepolicy_test.go b/pkg/reconciler/clusterimagepolicy/clusterimagepolicy_test.go index 3e92dd7e5ec..e808fb36455 100644 --- a/pkg/reconciler/clusterimagepolicy/clusterimagepolicy_test.go +++ b/pkg/reconciler/clusterimagepolicy/clusterimagepolicy_test.go @@ -39,15 +39,14 @@ import ( ) const ( - cipName = "test-cip" - testKey = "test-cip" - cipName2 = "test-cip-2" - testKey2 = "test-cip-2" - keySecretName = "publickey-key" - keylessSecretName = "publickey-keyless" - keylessSecretValue = "keyless cakeyref secret here" - 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" + keylessSecretName = "publickey-keyless" + glob = "ghcr.io/example/*" + kms = "azure-kms://foo/bar" // Just some public key that was laying around, only format matters. validPublicKeyData = `-----BEGIN PUBLIC KEY-----