Validate a public key in a secret is valid.

Fixes: sigstore#1596 secret key contains valid public key. Signed-off-by: Ville Aikas <[email protected]>
vaikas · Mar 14, 2022 · 59a06fd · 59a06fd
1 parent d22c312
commit 59a06fd
Show file tree

Hide file tree

Showing 2 changed files with 62 additions and 15 deletions.
diff --git 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
@@ -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{