Surface non-enforced errors

Signed-off-by: Denny Hoang <dhoang@vmware.com>
sigstore · Mar 21, 2022 · f149c6a · f149c6a
1 parent 1aa88d1
commit f149c6a
Show file tree

Hide file tree

Showing 3 changed files with 26 additions and 13 deletions.
diff --git a/pkg/cosign/kubernetes/webhook/validation.go b/pkg/cosign/kubernetes/webhook/validation.go
@@ -121,7 +121,7 @@ func parseAuthorityKeys(ctx context.Context, pubKey string) ([]*ecdsa.PublicKey,
 			keys = append(keys, key.(*ecdsa.PublicKey))
 		}
 	}
-	if keys == nil {
+	if len(keys) == 0 {
 		return nil, apis.ErrGeneric(fmt.Sprintf("malformed authority key data: %v", errs), apis.CurrentField)
 	}
 	return keys, nil

diff --git a/pkg/cosign/kubernetes/webhook/validator.go b/pkg/cosign/kubernetes/webhook/validator.go
@@ -143,7 +143,12 @@ func (v *Validator) validatePodSpec(ctx context.Context, ps *corev1.PodSpec, opt
 			containerKeys := keys
 			config := config.FromContext(ctx)
 			if config != nil {
-				containerKeys = append(containerKeys, getAuthorityKeys(ctx, ref, config)...)
+				authorityKeys, fieldErrors := getAuthorityKeys(ctx, ref, config)
+				if fieldErrors != nil {
+					// TODO:(dennyhoang) Enforce currently non-breaking errors https://github.com/sigstore/cosign/issues/1642
+					logging.FromContext(ctx).Warnf("Failed to fetch authorities for %s : %v", ref.Name(), fieldErrors)
+				}
+				containerKeys = append(containerKeys, authorityKeys...)
 			}
 
 			if err := valid(ctx, ref, containerKeys, ociremote.WithRemoteOptions(remote.WithAuthFromKeychain(kc))); err != nil {
@@ -161,27 +166,24 @@ func (v *Validator) validatePodSpec(ctx context.Context, ps *corev1.PodSpec, opt
 	return errs
 }
 
-func getAuthorityKeys(ctx context.Context, ref name.Reference, config *config.Config) []*ecdsa.PublicKey {
-	keys := make([]*ecdsa.PublicKey, 0)
+func getAuthorityKeys(ctx context.Context, ref name.Reference, config *config.Config) (keys []*ecdsa.PublicKey, errs *apis.FieldError) {
 	authorities, err := config.ImagePolicyConfig.GetAuthorities(ref.Name())
 	if err != nil {
-		logging.FromContext(ctx).Errorf("Failed to fetch authorities for %s : %v", ref.Name(), err)
+		return keys, apis.ErrGeneric(fmt.Sprintf("failed to fetch authorities for %s : %v", ref.Name(), err), apis.CurrentField)
 	} else {
-		logging.FromContext(ctx).Infof("Successfully fetch authorities for %s", ref.Name())
 		for _, authority := range authorities {
 			if authority.Key != nil {
 				// Get the key from authority data
-				if authorityKeys, err := parseAuthorityKeys(ctx, authority.Key.Data); err != nil {
-					logging.FromContext(ctx).Errorf("Failed to fetch keys from the authorities for %s : %v", ref.Name(), err)
+				if authorityKeys, fieldErr := parseAuthorityKeys(ctx, authority.Key.Data); fieldErr != nil {
+					errs = errs.Also(fieldErr)
 				} else {
-					logging.FromContext(ctx).Infof("Successfully added authority keys %s", ref.Name())
 					keys = append(keys, authorityKeys...)
 				}
 			}
 		}
 	}
 
-	return keys
+	return keys, errs
 }
 
 // ResolvePodSpecable implements duckv1.PodSpecValidator

diff --git a/pkg/cosign/kubernetes/webhook/validator_test.go b/pkg/cosign/kubernetes/webhook/validator_test.go
@@ -960,10 +960,12 @@ UoJou2P8sbDxpLiE/v3yLw1/jyOrCPWYHWFXnyyeGlkgSVefG54tNoK7Uw==
 		imagePatterns []v1alpha1.ImagePattern
 		authorities   []v1alpha1.Authority
 		wantKeyLength int
+		expectErr     bool
 	}{
 		{
 			name:          "no authorities",
 			wantKeyLength: 0,
+			expectErr:     false,
 		}, {
 			name: "invalid regex and one key data",
 			imagePatterns: []v1alpha1.ImagePattern{{
@@ -973,6 +975,7 @@ UoJou2P8sbDxpLiE/v3yLw1/jyOrCPWYHWFXnyyeGlkgSVefG54tNoK7Uw==
 				validKeyData,
 			},
 			wantKeyLength: 0,
+			expectErr:     true,
 		}, {
 			name: "unmatching glob and one key data",
 			imagePatterns: []v1alpha1.ImagePattern{{
@@ -982,6 +985,7 @@ UoJou2P8sbDxpLiE/v3yLw1/jyOrCPWYHWFXnyyeGlkgSVefG54tNoK7Uw==
 				validKeyData,
 			},
 			wantKeyLength: 0,
+			expectErr:     false,
 		}, {
 			name: "wildcard glob and one key data",
 			imagePatterns: []v1alpha1.ImagePattern{{
@@ -991,6 +995,7 @@ UoJou2P8sbDxpLiE/v3yLw1/jyOrCPWYHWFXnyyeGlkgSVefG54tNoK7Uw==
 				validKeyData,
 			},
 			wantKeyLength: 1,
+			expectErr:     false,
 		}, {
 			name: "wildcard regex and one key data",
 			imagePatterns: []v1alpha1.ImagePattern{{
@@ -1000,9 +1005,8 @@ UoJou2P8sbDxpLiE/v3yLw1/jyOrCPWYHWFXnyyeGlkgSVefG54tNoK7Uw==
 				validKeyData,
 			},
 			wantKeyLength: 1,
+			expectErr:     false,
 		},
-		// TODO(dennyhoang): Test against authority[].keyless
-		// TODO(dennyhoang): Test with more imagePatterns
 	}
 
 	for _, test := range tests {
@@ -1018,7 +1022,14 @@ UoJou2P8sbDxpLiE/v3yLw1/jyOrCPWYHWFXnyyeGlkgSVefG54tNoK7Uw==
 				},
 			}
 
-			keys := getAuthorityKeys(context.Background(), refName, &config)
+			keys, err := getAuthorityKeys(context.Background(), refName, &config)
+			if test.expectErr && err == nil {
+				t.Error("Did not get wanted error")
+			}
+			if !test.expectErr && err != nil {
+				t.Error("Did get unwanted error")
+			}
+
 			if got := len(keys); got != test.wantKeyLength {
 				t.Errorf("Did not get what I wanted %d, got %d", test.wantKeyLength, got)
 			}