fix: fix #1930 for AWS KMS formats (#1946)

* fix: fix #1930 for AWS KMS formats Signed-off-by: Ville Aikas <vaikas@chainguard.dev> * use v2 of aws go-sdk, didn't realize there was one. Signed-off-by: Ville Aikas <vaikas@chainguard.dev> * Fix the lint + add missing authorities section to test crds. doh. Signed-off-by: Ville Aikas <vaikas@chainguard.dev> * can't even Signed-off-by: Ville Aikas <vaikas@chainguard.dev> * Actually validate keyless ca-cert as keyref. Yikes. Signed-off-by: Ville Aikas <vaikas@chainguard.dev> * also v1beta1. Signed-off-by: Ville Aikas <vaikas@chainguard.dev>
sigstore · Jun 2, 2022 · 2ccb1a2 · 2ccb1a2
1 parent ae90c74
commit 2ccb1a2
Show file tree

Hide file tree

Showing 11 changed files with 426 additions and 6 deletions.
diff --git a/config/300-clusterimagepolicy.yaml b/config/300-clusterimagepolicy.yaml
@@ -86,7 +86,7 @@ spec:
                             description: Data contains the inline public key
                             type: string
                           kms:
-                            description: KMS contains the KMS url of the public key
+                            description: KMS contains the KMS url of the public key Supported formats differ based on the KMS system used.
                             type: string
                           secretRef:
                             type: object
@@ -107,7 +107,7 @@ spec:
                                 description: Data contains the inline public key
                                 type: string
                               kms:
-                                description: KMS contains the KMS url of the public key
+                                description: KMS contains the KMS url of the public key Supported formats differ based on the KMS system used.
                                 type: string
                               secretRef:
                                 type: object

diff --git a/go.mod b/go.mod
@@ -8,6 +8,7 @@ require (
 	github.com/ThalesIgnite/crypto11 v1.2.5
 	github.com/armon/go-metrics v0.4.0
 	github.com/armon/go-radix v1.0.0
+	github.com/aws/aws-sdk-go-v2 v1.14.0
 	github.com/awslabs/amazon-ecr-credential-helper/ecr-login v0.0.0-20220228164355-396b2034c795
 	github.com/cenkalti/backoff/v3 v3.2.2
 	github.com/chrismellard/docker-credential-acr-env v0.0.0-20220119192733-fe33c00cee21
@@ -122,7 +123,6 @@ require (
 	github.com/ReneKroon/ttlcache/v2 v2.11.0 // indirect
 	github.com/asaskevich/govalidator v0.0.0-20210307081110-f21760c49a8d // indirect
 	github.com/aws/aws-sdk-go v1.43.45 // indirect
-	github.com/aws/aws-sdk-go-v2 v1.14.0 // indirect
 	github.com/aws/aws-sdk-go-v2/config v1.14.0 // indirect
 	github.com/aws/aws-sdk-go-v2/credentials v1.9.0 // indirect
 	github.com/aws/aws-sdk-go-v2/feature/ec2/imds v1.11.0 // indirect

diff --git a/pkg/apis/policy/v1alpha1/clusterimagepolicy_types.go b/pkg/apis/policy/v1alpha1/clusterimagepolicy_types.go
@@ -104,6 +104,7 @@ type KeyRef struct {
 	// +optional
 	Data string `json:"data,omitempty"`
 	// KMS contains the KMS url of the public key
+	// Supported formats differ based on the KMS system used.
 	// +optional
 	KMS string `json:"kms,omitempty"`
 }

diff --git a/pkg/apis/policy/v1alpha1/clusterimagepolicy_validation.go b/pkg/apis/policy/v1alpha1/clusterimagepolicy_validation.go
@@ -17,13 +17,18 @@ package v1alpha1
 import (
 	"context"
 	"fmt"
+	"net"
 	"path/filepath"
 	"regexp"
+	"strings"
 
+	"github.com/aws/aws-sdk-go-v2/aws/arn"
 	"github.com/sigstore/cosign/pkg/apis/utils"
 	"knative.dev/pkg/apis"
 )
 
+const awsKMSPrefix = "awskms://"
+
 // Validate implements apis.Validatable
 func (c *ClusterImagePolicy) Validate(ctx context.Context) *apis.FieldError {
 	return c.Spec.Validate(ctx).ViaField("spec")
@@ -54,6 +59,7 @@ func (image *ImagePattern) Validate(ctx context.Context) *apis.FieldError {
 	}
 
 	errs = errs.Also(ValidateGlob(image.Glob).ViaField("glob"))
+
 	return errs
 }
 
@@ -104,6 +110,11 @@ func (key *KeyRef) Validate(ctx context.Context) *apis.FieldError {
 	} else if key.KMS != "" && key.SecretRef != nil {
 		errs = errs.Also(apis.ErrMultipleOneOf("data", "kms", "secretref"))
 	}
+	if key.KMS != "" {
+		if strings.HasPrefix(key.KMS, awsKMSPrefix) {
+			errs = errs.Also(validateAWSKMS(key.KMS).ViaField("kms"))
+		}
+	}
 	return errs
 }
 
@@ -122,6 +133,9 @@ func (keyless *KeylessRef) Validate(ctx context.Context) *apis.FieldError {
 		errs = errs.Also(apis.ErrMissingField("identities"))
 	}
 
+	if keyless.CACert != nil {
+		errs = errs.Also(keyless.DeepCopy().CACert.Validate(ctx).ViaField("ca-cert"))
+	}
 	for i, identity := range keyless.Identities {
 		errs = errs.Also(identity.Validate(ctx).ViaFieldIndex("identities", i))
 	}
@@ -209,3 +223,36 @@ func ValidateRegex(regex string) *apis.FieldError {
 
 	return nil
 }
+
+// validateAWSKMS validates that the KMS conforms to AWS
+// KMS format:
+// awskms://$ENDPOINT/$KEYID
+// Where:
+// $ENDPOINT is optional
+// $KEYID is either the key ARN or an alias ARN
+// Reasoning for only supporting these formats is that other
+// formats require additional configuration via ENV variables.
+func validateAWSKMS(kms string) *apis.FieldError {
+	parts := strings.Split(kms, "/")
+	if len(parts) < 4 {
+		return apis.ErrInvalidValue(kms, apis.CurrentField, "malformed AWS KMS format, should be: 'awskms://$ENDPOINT/$KEYID'")
+	}
+	endpoint := parts[2]
+	// missing endpoint is fine, only validate if not empty
+	if endpoint != "" {
+		_, _, err := net.SplitHostPort(endpoint)
+		if err != nil {
+			return apis.ErrInvalidValue(kms, apis.CurrentField, fmt.Sprintf("malformed endpoint: %s", err))
+		}
+	}
+	keyID := parts[3]
+	arn, err := arn.Parse(keyID)
+	if err != nil {
+		return apis.ErrInvalidValue(kms, apis.CurrentField, fmt.Sprintf("failed to parse either key or alias arn: %s", err))
+	}
+	// Only support key or alias ARN.
+	if arn.Resource != "key" && arn.Resource != "alias" {
+		return apis.ErrInvalidValue(kms, apis.CurrentField, fmt.Sprintf("Got ARN: %+v Resource: %s", arn, arn.Resource))
+	}
+	return nil
+}
diff --git a/pkg/apis/policy/v1alpha1/clusterimagepolicy_validation_test.go b/pkg/apis/policy/v1alpha1/clusterimagepolicy_validation_test.go
@@ -16,13 +16,16 @@ package v1alpha1
 
 import (
 	"context"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/require"
 	v1 "k8s.io/api/core/v1"
 	"knative.dev/pkg/apis"
 )
 
+const validPublicKey = "-----BEGIN PUBLIC KEY-----\nMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEaEOVJCFtduYr3xqTxeRWSW32CY/s\nTBNZj4oIUPl8JvhVPJ1TKDPlNcuT4YphSt6t3yOmMvkdQbCj8broX6vijw==\n-----END PUBLIC KEY-----"
+
 func TestImagePatternValidation(t *testing.T) {
 	tests := []struct {
 		name        string
@@ -282,7 +285,7 @@ func TestKeylessValidation(t *testing.T) {
 									Host: "myhost",
 								},
 								CACert: &KeyRef{
-									Data: "---certificate---",
+									Data: validPublicKey,
 								},
 							},
 						},
@@ -384,6 +387,37 @@ func TestAuthoritiesValidation(t *testing.T) {
 				},
 			},
 		},
+		{
+			name:        "Should fail with invalid AWS KMS for Keyful",
+			expectErr:   true,
+			errorString: "invalid value: awskms://localhost:8888/arn:butnotvalid: spec.authorities[0].key.kms\nfailed to parse either key or alias arn: arn: not enough sections",
+			policy: ClusterImagePolicy{
+				Spec: ClusterImagePolicySpec{
+					Images: []ImagePattern{{Glob: "gcr.io/*"}},
+					Authorities: []Authority{
+						{
+							Key:     &KeyRef{KMS: "awskms://localhost:8888/arn:butnotvalid"},
+							Sources: []Source{{OCI: "registry.example.com"}},
+						},
+					},
+				},
+			},
+		},
+		{
+			name:        "Should fail with invalid AWS KMS for Keyless",
+			expectErr:   true,
+			errorString: "invalid value: awskms://localhost:8888/arn:butnotvalid: spec.authorities[0].keyless.ca-cert.kms\nfailed to parse either key or alias arn: arn: not enough sections",
+			policy: ClusterImagePolicy{
+				Spec: ClusterImagePolicySpec{
+					Images: []ImagePattern{{Glob: "gcr.io/*"}},
+					Authorities: []Authority{
+						{
+							Keyless: &KeylessRef{CACert: &KeyRef{KMS: "awskms://localhost:8888/arn:butnotvalid"}},
+						},
+					},
+				},
+			},
+		},
 		{
 			name:        "Should fail when source oci is empty",
 			expectErr:   true,
@@ -710,3 +744,83 @@ func TestIdentitiesValidation(t *testing.T) {
 		})
 	}
 }
+
+func TestAWSKMSValidation(t *testing.T) {
+	// Note the error messages betweeen the kms / cacert validation is
+	// identical, with the only difference being `kms` or `ca-cert.kms`. Reason
+	// for the ca-cert.kms is because it's embedded within the ca-cert that
+	// we pass in. So we put a KMSORCACERT into the err string that we then
+	// replace based on the tests so we don't have to write identical tests
+	// for both of them.
+	tests := []struct {
+		name        string
+		expectErr   bool
+		errorString string
+		kms         string
+	}{
+		{
+			name:        "malformed, only 2 slashes ",
+			expectErr:   true,
+			errorString: "invalid value: awskms://1234abcd-12ab-34cd-56ef-1234567890ab: KMSORCACERT\nmalformed AWS KMS format, should be: 'awskms://$ENDPOINT/$KEYID'",
+			kms:         "awskms://1234abcd-12ab-34cd-56ef-1234567890ab",
+		},
+		{
+			name:        "fails with invalid host",
+			expectErr:   true,
+			errorString: "invalid value: awskms://localhost:::4566/alias/exampleAlias: KMSORCACERT\nmalformed endpoint: address localhost:::4566: too many colons in address",
+			kms:         "awskms://localhost:::4566/alias/exampleAlias",
+		},
+		{
+			name:        "fails with non-arn alias",
+			expectErr:   true,
+			errorString: "invalid value: awskms://localhost:4566/alias/exampleAlias: KMSORCACERT\nfailed to parse either key or alias arn: arn: invalid prefix",
+			kms:         "awskms://localhost:4566/alias/exampleAlias",
+		},
+		{
+			name:        "Should fail when arn is invalid",
+			expectErr:   true,
+			errorString: "invalid value: awskms://localhost:4566/arn:sonotvalid: KMSORCACERT\nfailed to parse either key or alias arn: arn: not enough sections",
+			kms:         "awskms://localhost:4566/arn:sonotvalid",
+		},
+		{
+			name: "works with valid arn key and endpoint",
+			kms:  "awskms://localhost:4566/arn:aws:kms:us-east-2:111122223333:key/1234abcd-12ab-34cd-56ef-1234567890ab",
+		},
+		{
+			name: "works with valid arn key and no endpoint",
+			kms:  "awskms:///arn:aws:kms:us-east-2:111122223333:key/1234abcd-12ab-34cd-56ef-1234567890ab",
+		},
+		{
+			name: "works with valid arn alias and endpoint",
+			kms:  "awskms://localhost:4566/arn:aws:kms:us-east-2:111122223333:alias/ExampleAlias",
+		},
+		{
+			name: "works with valid arn alias and no endpoint",
+			kms:  "awskms:///arn:aws:kms:us-east-2:111122223333:alias/ExampleAlias",
+		},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			// First test with KeyRef
+			keyRef := KeyRef{KMS: test.kms}
+			err := keyRef.Validate(context.TODO())
+			if test.expectErr {
+				require.NotNil(t, err)
+				kmsErrString := strings.Replace(test.errorString, "KMSORCACERT", "kms", 1)
+				require.EqualError(t, err, kmsErrString)
+			} else {
+				require.Nil(t, err)
+			}
+			// Then with Keyless with CACert as KeyRef
+			keylessRef := KeylessRef{CACert: &keyRef}
+			err = keylessRef.Validate(context.TODO())
+			if test.expectErr {
+				require.NotNil(t, err)
+				caCertErrString := strings.Replace(test.errorString, "KMSORCACERT", "ca-cert.kms", 1)
+				require.EqualError(t, err, caCertErrString)
+			} else {
+				require.Nil(t, err)
+			}
+		})
+	}
+}
diff --git a/pkg/apis/policy/v1beta1/clusterimagepolicy_types.go b/pkg/apis/policy/v1beta1/clusterimagepolicy_types.go
@@ -104,6 +104,7 @@ type KeyRef struct {
 	// +optional
 	Data string `json:"data,omitempty"`
 	// KMS contains the KMS url of the public key
+	// Supported formats differ based on the KMS system used.
 	// +optional
 	KMS string `json:"kms,omitempty"`
 }

diff --git a/pkg/apis/policy/v1beta1/clusterimagepolicy_validation.go b/pkg/apis/policy/v1beta1/clusterimagepolicy_validation.go
@@ -17,13 +17,18 @@ package v1beta1
 import (
 	"context"
 	"fmt"
+	"net"
 	"path/filepath"
 	"regexp"
+	"strings"
 
+	"github.com/aws/aws-sdk-go-v2/aws/arn"
 	"github.com/sigstore/cosign/pkg/apis/utils"
 	"knative.dev/pkg/apis"
 )
 
+const awsKMSPrefix = "awskms://"
+
 // Validate implements apis.Validatable
 func (c *ClusterImagePolicy) Validate(ctx context.Context) *apis.FieldError {
 	return c.Spec.Validate(ctx).ViaField("spec")
@@ -105,6 +110,11 @@ func (key *KeyRef) Validate(ctx context.Context) *apis.FieldError {
 	} else if key.KMS != "" && key.SecretRef != nil {
 		errs = errs.Also(apis.ErrMultipleOneOf("data", "kms", "secretref"))
 	}
+	if key.KMS != "" {
+		if strings.HasPrefix(key.KMS, awsKMSPrefix) {
+			errs = errs.Also(validateAWSKMS(key.KMS).ViaField("kms"))
+		}
+	}
 	return errs
 }
 
@@ -123,6 +133,9 @@ func (keyless *KeylessRef) Validate(ctx context.Context) *apis.FieldError {
 		errs = errs.Also(apis.ErrMissingField("identities"))
 	}
 
+	if keyless.CACert != nil {
+		errs = errs.Also(keyless.DeepCopy().CACert.Validate(ctx).ViaField("ca-cert"))
+	}
 	for i, identity := range keyless.Identities {
 		errs = errs.Also(identity.Validate(ctx).ViaFieldIndex("identities", i))
 	}
@@ -203,11 +216,43 @@ func ValidateGlob(glob string) *apis.FieldError {
 }
 
 func ValidateRegex(regex string) *apis.FieldError {
-	// It's a regexp, so pull out the regex
 	_, err := regexp.Compile(regex)
 	if err != nil {
 		return apis.ErrInvalidValue(regex, apis.CurrentField, fmt.Sprintf("regex is invalid: %v", err))
 	}
 
 	return nil
 }
+
+// validateAWSKMS validates that the KMS conforms to AWS
+// KMS format:
+// awskms://$ENDPOINT/$KEYID
+// Where:
+// $ENDPOINT is optional
+// $KEYID is either the key ARN or an alias ARN
+// Reasoning for only supporting these formats is that other
+// formats require additional configuration via ENV variables.
+func validateAWSKMS(kms string) *apis.FieldError {
+	parts := strings.Split(kms, "/")
+	if len(parts) < 4 {
+		return apis.ErrInvalidValue(kms, apis.CurrentField, "malformed AWS KMS format, should be: 'awskms://$ENDPOINT/$KEYID'")
+	}
+	endpoint := parts[2]
+	// missing endpoint is fine, only validate if not empty
+	if endpoint != "" {
+		_, _, err := net.SplitHostPort(endpoint)
+		if err != nil {
+			return apis.ErrInvalidValue(kms, apis.CurrentField, fmt.Sprintf("malformed endpoint: %s", err))
+		}
+	}
+	keyID := parts[3]
+	arn, err := arn.Parse(keyID)
+	if err != nil {
+		return apis.ErrInvalidValue(kms, apis.CurrentField, fmt.Sprintf("failed to parse either key or alias arn: %s", err))
+	}
+	// Only support key or alias ARN.
+	if arn.Resource != "key" && arn.Resource != "alias" {
+		return apis.ErrInvalidValue(kms, apis.CurrentField, fmt.Sprintf("Got ARN: %+v Resource: %s", arn, arn.Resource))
+	}
+	return nil
+}