Validate authority keys (sigstore#1623)

* Validate signature with authority keys Signed-off-by: Denny Hoang <dhoang@vmware.com> * Remove TODO log Signed-off-by: Denny Hoang <dhoang@vmware.com> * Add authority key info log & ko-apply to makefile Signed-off-by: Denny Hoang <dhoang@vmware.com> * Abstract and test getting authority keys Signed-off-by: Denny Hoang <dhoang@vmware.com> * Add authority regex matching for images Signed-off-by: Denny Hoang <dhoang@vmware.com> * Add more get authority key scenarios Signed-off-by: Denny Hoang <dhoang@vmware.com> * Rename and use variables Signed-off-by: Denny Hoang <dhoang@vmware.com> * Validate and compile regex Signed-off-by: Denny Hoang <dhoang@vmware.com> * Add regex cosigned testdata Signed-off-by: Denny Hoang <dhoang@vmware.com> * Fix lint error Signed-off-by: Denny Hoang <dhoang@vmware.com> * Test authority key integration Signed-off-by: Denny Hoang <dhoang@vmware.com> * Surface non-enforced errors Signed-off-by: Denny Hoang <dhoang@vmware.com> * Fix linting errors Signed-off-by: Denny Hoang <dhoang@vmware.com> * Fix e2e crd test to remove crd between tests Signed-off-by: Denny Hoang <dhoang@vmware.com> Co-authored-by: Denny Hoang <dhoang@vmware.com>
mlieberman85 · May 6, 2022 · a818801 · a818801
1 parent 6de6a24
commit a818801
Show file tree

Hide file tree

Showing 13 changed files with 414 additions and 86 deletions.
diff --git a/Makefile b/Makefile
@@ -185,6 +185,10 @@ ko-local:
 		$(ARTIFACT_HUB_LABELS) \
 		github.com/sigstore/cosign/cmd/cosign/policy_webhook
 
+.PHONY: ko-apply
+ko-apply:
+	LDFLAGS="$(LDFLAGS)" GIT_HASH=$(GIT_HASH) GIT_VERSION=$(GIT_VERSION) ko apply -Bf config/
+
 ##################
 # help
 ##################

diff --git a/pkg/apis/config/image_policies.go b/pkg/apis/config/image_policies.go
@@ -19,6 +19,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"regexp"
 
 	"github.com/sigstore/cosign/pkg/apis/cosigned/v1alpha1"
 	corev1 "k8s.io/api/core/v1"
@@ -87,17 +88,26 @@ func (p *ImagePolicyConfig) GetAuthorities(image string) ([]v1alpha1.Authority,
 		return nil, errors.New("config is nil")
 	}
 
+	var lastError error
 	ret := []v1alpha1.Authority{}
 
 	// TODO(vaikas): this is very inefficient, we should have a better
 	// way to go from image to Authorities, but just seeing if this is even
 	// workable so fine for now.
 	for _, v := range p.Policies {
 		for _, pattern := range v.Images {
-			if GlobMatch(image, pattern.Glob) {
-				ret = append(ret, v.Authorities...)
+			if pattern.Glob != "" {
+				if GlobMatch(image, pattern.Glob) {
+					ret = append(ret, v.Authorities...)
+				}
+			} else if pattern.Regex != "" {
+				if regex, err := regexp.Compile(pattern.Regex); err != nil {
+					lastError = err
+				} else if regex.MatchString(image) {
+					ret = append(ret, v.Authorities...)
+				}
 			}
 		}
 	}
-	return ret, nil
+	return ret, lastError
 }
diff --git a/pkg/apis/config/image_policies_test.go b/pkg/apis/config/image_policies_test.go
@@ -17,6 +17,7 @@ package config
 import (
 	"testing"
 
+	"github.com/sigstore/cosign/pkg/apis/cosigned/v1alpha1"
 	. "knative.dev/pkg/configmap/testing"
 	_ "knative.dev/pkg/system/testing"
 )
@@ -43,35 +44,20 @@ func TestGetAuthorities(t *testing.T) {
 		t.Error("NewImagePoliciesConfigFromConfigMap(example) =", err)
 	}
 	c, err := defaults.GetAuthorities("rando")
-	if err != nil {
-		t.Error("GetMatches Failed =", err)
-	}
-	if len(c) == 0 {
-		t.Error("Wanted a config, got none.")
-	}
+	checkGetMatches(t, c, err)
 	want := "inlinedata here"
 	if got := c[0].Key.Data; got != want {
-		t.Errorf("Did not get what I wanted %q, got %+v", want, c[0].Key.Data)
+		t.Errorf("Did not get what I wanted %q, got %+v", want, got)
 	}
 	// Make sure glob matches 'randomstuff*'
 	c, err = defaults.GetAuthorities("randomstuffhere")
-	if err != nil {
-		t.Error("GetMatches Failed =", err)
-	}
-	if len(c) == 0 {
-		t.Error("Wanted a config, got none.")
-	}
+	checkGetMatches(t, c, err)
 	want = "otherinline here"
 	if got := c[0].Key.Data; got != want {
-		t.Errorf("Did not get what I wanted %q, got %+v", want, c[0].Key.Data)
+		t.Errorf("Did not get what I wanted %q, got %+v", want, got)
 	}
 	c, err = defaults.GetAuthorities("rando3")
-	if err != nil {
-		t.Error("GetMatches Failed =", err)
-	}
-	if len(c) == 0 {
-		t.Error("Wanted a config, got none.")
-	}
+	checkGetMatches(t, c, err)
 	want = "cacert chilling here"
 	if got := c[0].Keyless.CACert.Data; got != want {
 		t.Errorf("Did not get what I wanted %q, got %+v", want, c[0].Keyless.CACert.Data)
@@ -84,28 +70,35 @@ func TestGetAuthorities(t *testing.T) {
 	if got := c[0].Keyless.Identities[0].Subject; got != want {
 		t.Errorf("Did not get what I wanted %q, got %+v", want, c[0].Keyless.Identities[0].Subject)
 	}
+	// Make sure regex matches ".*regexstring.*"
+	c, err = defaults.GetAuthorities("randomregexstringstuff")
+	checkGetMatches(t, c, err)
+	want = inlineKeyData
+	if got := c[0].Key.Data; got != want {
+		t.Errorf("Did not get what I wanted %q, got %+v", want, got)
+	}
+
 	// Test multiline yaml cert
 	c, err = defaults.GetAuthorities("inlinecert")
-	if err != nil {
-		t.Error("GetMatches Failed =", err)
-	}
-	if len(c) == 0 {
-		t.Error("Wanted a config, got none.")
-	}
+	checkGetMatches(t, c, err)
 	want = inlineKeyData
 	if got := c[0].Key.Data; got != want {
-		t.Errorf("Did not get what I wanted %q, got %+v", want, c[0].Key.Data)
+		t.Errorf("Did not get what I wanted %q, got %+v", want, got)
 	}
 	// Test multiline cert but json encoded
 	c, err = defaults.GetAuthorities("ghcr.io/example/*")
+	checkGetMatches(t, c, err)
+	want = inlineKeyData
+	if got := c[0].Key.Data; got != want {
+		t.Errorf("Did not get what I wanted %q, got %+v", want, got)
+	}
+}
+
+func checkGetMatches(t *testing.T, c []v1alpha1.Authority, err error) {
 	if err != nil {
 		t.Error("GetMatches Failed =", err)
 	}
 	if len(c) == 0 {
 		t.Error("Wanted a config, got none.")
 	}
-	want = inlineKeyData
-	if got := c[0].Key.Data; got != want {
-		t.Errorf("Did not get what I wanted %q, got %+v", want, c[0].Key.Data)
-	}
 }
diff --git a/pkg/apis/config/testdata/config-image-policies.yaml b/pkg/apis/config/testdata/config-image-policies.yaml
@@ -62,5 +62,15 @@ data:
             MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAExB6+H6054/W1SJgs5JR6AJr6J35J
             RCTfQ5s1kD+hGMSE1rH7s46hmXEeyhnlRnaGF8eMU/SBJE/2NKPnxE7WzQ==
             -----END PUBLIC KEY-----
+    cluster-image-policy-4: |
+      images:
+      - regex: .*regexstring.*
+      authorities:
+      - key:
+          data: |-
+            -----BEGIN PUBLIC KEY-----
+            MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAExB6+H6054/W1SJgs5JR6AJr6J35J
+            RCTfQ5s1kD+hGMSE1rH7s46hmXEeyhnlRnaGF8eMU/SBJE/2NKPnxE7WzQ==
+            -----END PUBLIC KEY-----
     cluster-image-policy-json: "{\"images\":[{\"glob\":\"ghcr.io/example/*\",\"regex\":\"\"}],\"authorities\":[{\"key\":{\"data\":\"-----BEGIN PUBLIC KEY-----\\nMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAExB6+H6054/W1SJgs5JR6AJr6J35J\\nRCTfQ5s1kD+hGMSE1rH7s46hmXEeyhnlRnaGF8eMU/SBJE/2NKPnxE7WzQ==\\n-----END PUBLIC KEY-----\"}}]}"
 
diff --git a/pkg/apis/cosigned/v1alpha1/clusterimagepolicy_validation.go b/pkg/apis/cosigned/v1alpha1/clusterimagepolicy_validation.go
@@ -16,6 +16,8 @@ package v1alpha1
 
 import (
 	"context"
+	"fmt"
+	"regexp"
 	"strings"
 
 	"github.com/sigstore/cosign/pkg/apis/utils"
@@ -59,7 +61,7 @@ func (image *ImagePattern) Validate(ctx context.Context) *apis.FieldError {
 	}
 
 	if image.Regex != "" {
-		errs = errs.Also(apis.ErrDisallowedFields("regex"))
+		errs = errs.Also(ValidateRegex(image.Regex).ViaField("regex"))
 	}
 
 	return errs
@@ -155,3 +157,12 @@ func ValidateGlob(glob string) *apis.FieldError {
 	}
 	return nil
 }
+
+func ValidateRegex(regex string) *apis.FieldError {
+	_, err := regexp.Compile(regex)
+	if err != nil {
+		return apis.ErrInvalidValue(regex, apis.CurrentField, fmt.Sprintf("regex is invalid: %v", err))
+	}
+
+	return nil
+}
diff --git a/pkg/apis/cosigned/v1alpha1/clusterimagepolicy_validation_test.go b/pkg/apis/cosigned/v1alpha1/clusterimagepolicy_validation_test.go
@@ -32,7 +32,7 @@ func TestImagePatternValidation(t *testing.T) {
 		{
 			name:        "Should fail when both regex and glob are present",
 			expectErr:   true,
-			errorString: "expected exactly one, got both: spec.images[0].glob, spec.images[0].regex\ninvalid value: **: spec.images[0].glob\nglob match supports only a single * as a trailing character\nmissing field(s): spec.authorities\nmust not set the field(s): spec.images[0].regex",
+			errorString: "expected exactly one, got both: spec.images[0].glob, spec.images[0].regex\ninvalid value: **: spec.images[0].glob\nglob match supports only a single * as a trailing character\nmissing field(s): spec.authorities",
 			policy: ClusterImagePolicy{
 				Spec: ClusterImagePolicySpec{
 					Images: []ImagePattern{
@@ -92,6 +92,40 @@ func TestImagePatternValidation(t *testing.T) {
 				Spec: ClusterImagePolicySpec{},
 			},
 		},
+		{
+			name:        "Should fail when regex is invalid: %v",
+			expectErr:   true,
+			errorString: "invalid value: *: spec.images[0].regex\nregex is invalid: error parsing regexp: missing argument to repetition operator: `*`\nmissing field(s): spec.authorities",
+			policy: ClusterImagePolicy{
+				Spec: ClusterImagePolicySpec{
+					Images: []ImagePattern{
+						{
+							Regex: "*",
+						},
+					},
+				},
+			},
+		},
+		{
+			name:      "Should pass when regex is valid: %v",
+			expectErr: false,
+			policy: ClusterImagePolicy{
+				Spec: ClusterImagePolicySpec{
+					Images: []ImagePattern{
+						{
+							Regex: ".*",
+						},
+					},
+					Authorities: []Authority{
+						{
+							Key: &KeyRef{
+								KMS: "kms://key/path",
+							},
+						},
+					},
+				},
+			},
+		},
 	}
 
 	for _, test := range tests {
@@ -198,29 +232,8 @@ func TestKeyValidation(t *testing.T) {
 			},
 		},
 		{
-			name:        "Should fail when regex is given",
-			expectErr:   true,
-			errorString: "must not set the field(s): spec.images[0].regex",
-			policy: ClusterImagePolicy{
-				Spec: ClusterImagePolicySpec{
-					Images: []ImagePattern{
-						{
-							Regex: "myg**lob*",
-						},
-					},
-					Authorities: []Authority{
-						{
-							Key: &KeyRef{
-								KMS: "kms://key/path",
-							},
-						},
-					},
-				},
-			},
-		},
-		{
-			name:      "Should pass when key has only one property",
-			expectErr: false,
+			name:        "Should pass when key has only one property: %v",
+			errorString: "",
 			policy: ClusterImagePolicy{
 				Spec: ClusterImagePolicySpec{
 					Images: []ImagePattern{

diff --git a/pkg/cosign/kubernetes/webhook/validation.go b/pkg/cosign/kubernetes/webhook/validation.go
@@ -29,28 +29,13 @@ import (
 	"knative.dev/pkg/logging"
 
 	"github.com/sigstore/cosign/cmd/cosign/cli/fulcio/fulcioroots"
-	"github.com/sigstore/cosign/pkg/apis/config"
 	"github.com/sigstore/cosign/pkg/cosign"
 	"github.com/sigstore/cosign/pkg/oci"
 	ociremote "github.com/sigstore/cosign/pkg/oci/remote"
 	"github.com/sigstore/sigstore/pkg/signature"
 )
 
 func valid(ctx context.Context, ref name.Reference, keys []*ecdsa.PublicKey, opts ...ociremote.Option) error {
-	// TODO(vaikas): No failures, just logging as to not interfere with the
-	// normal operation. Just starting to plumb things through here.
-	config := config.FromContext(ctx)
-	if config != nil {
-		authorities, err := config.ImagePolicyConfig.GetAuthorities(ref.Name())
-		if err != nil {
-			logging.FromContext(ctx).Errorf("Failed to fetch authorities for %s : %v", ref.Name(), err)
-		} else {
-			for _, authority := range authorities {
-				logging.FromContext(ctx).Infof("TODO: Check authority for image: %s : Authority: %+v ", ref.Name(), authority)
-			}
-		}
-	}
-
 	if len(keys) == 0 {
 		// If there are no keys, then verify against the fulcio root.
 		sps, err := validSignatures(ctx, ref, nil /* verifier */, opts...)
@@ -121,6 +106,27 @@ func getKeys(ctx context.Context, cfg map[string][]byte) ([]*ecdsa.PublicKey, *a
 	return keys, nil
 }
 
+func parseAuthorityKeys(ctx context.Context, pubKey string) ([]*ecdsa.PublicKey, *apis.FieldError) {
+	keys := []*ecdsa.PublicKey{}
+	errs := []error{}
+
+	logging.FromContext(ctx).Debugf("Got public key: %v", pubKey)
+
+	pems := parsePems([]byte(pubKey))
+	for _, p := range pems {
+		key, err := x509.ParsePKIXPublicKey(p.Bytes)
+		if err != nil {
+			errs = append(errs, err)
+		} else {
+			keys = append(keys, key.(*ecdsa.PublicKey))
+		}
+	}
+	if len(keys) == 0 {
+		return nil, apis.ErrGeneric(fmt.Sprintf("malformed authority key data: %v", errs), apis.CurrentField)
+	}
+	return keys, nil
+}
+
 func parsePems(b []byte) []*pem.Block {
 	p, rest := pem.Decode(b)
 	if p == nil {

diff --git a/pkg/cosign/kubernetes/webhook/validator.go b/pkg/cosign/kubernetes/webhook/validator.go
@@ -17,11 +17,13 @@ package webhook
 
 import (
 	"context"
+	"crypto/ecdsa"
 	"fmt"
 
 	"github.com/google/go-containerregistry/pkg/authn/k8schain"
 	"github.com/google/go-containerregistry/pkg/name"
 	"github.com/google/go-containerregistry/pkg/v1/remote"
+	"github.com/sigstore/cosign/pkg/apis/config"
 	ociremote "github.com/sigstore/cosign/pkg/oci/remote"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/client-go/kubernetes"
@@ -138,7 +140,18 @@ func (v *Validator) validatePodSpec(ctx context.Context, ps *corev1.PodSpec, opt
 				continue
 			}
 
-			if err := valid(ctx, ref, keys, ociremote.WithRemoteOptions(remote.WithAuthFromKeychain(kc))); err != nil {
+			containerKeys := keys
+			config := config.FromContext(ctx)
+			if config != nil {
+				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 {
 				errorField := apis.ErrGeneric(err.Error(), "image").ViaFieldIndex(field, i)
 				errorField.Details = c.Image
 				errs = errs.Also(errorField)
@@ -153,6 +166,26 @@ func (v *Validator) validatePodSpec(ctx context.Context, ps *corev1.PodSpec, opt
 	return errs
 }
 
+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 {
+		return keys, apis.ErrGeneric(fmt.Sprintf("failed to fetch authorities for %s : %v", ref.Name(), err), apis.CurrentField)
+	}
+
+	for _, authority := range authorities {
+		if authority.Key != nil {
+			// Get the key from authority data
+			if authorityKeys, fieldErr := parseAuthorityKeys(ctx, authority.Key.Data); fieldErr != nil {
+				errs = errs.Also(fieldErr)
+			} else {
+				keys = append(keys, authorityKeys...)
+			}
+		}
+	}
+
+	return keys, errs
+}
+
 // ResolvePodSpecable implements duckv1.PodSpecValidator
 func (v *Validator) ResolvePodSpecable(ctx context.Context, wp *duckv1.WithPod) {
 	if wp.DeletionTimestamp != nil {