Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate authority keys #1623

Merged
merged 14 commits into from
Mar 22, 2022
4 changes: 4 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -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
##################
Expand Down
16 changes: 13 additions & 3 deletions pkg/apis/config/image_policies.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"encoding/json"
"errors"
"fmt"
"regexp"

"github.com/sigstore/cosign/pkg/apis/cosigned/v1alpha1"
corev1 "k8s.io/api/core/v1"
Expand Down Expand Up @@ -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
}
55 changes: 24 additions & 31 deletions pkg/apis/config/image_policies_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand All @@ -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)
Expand All @@ -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)
}
}
10 changes: 10 additions & 0 deletions pkg/apis/config/testdata/config-image-policies.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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-----\"}}]}"

13 changes: 12 additions & 1 deletion pkg/apis/cosigned/v1alpha1/clusterimagepolicy_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ package v1alpha1

import (
"context"
"fmt"
"regexp"
"strings"

"github.com/sigstore/cosign/pkg/apis/utils"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}
61 changes: 37 additions & 24 deletions pkg/apis/cosigned/v1alpha1/clusterimagepolicy_validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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{
Expand Down
36 changes: 21 additions & 15 deletions pkg/cosign/kubernetes/webhook/validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -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...)
Expand Down Expand Up @@ -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 {
Expand Down
35 changes: 34 additions & 1 deletion pkg/cosign/kubernetes/webhook/validator.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Expand All @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we want to make these as apis.FieldError. We usually use those as validation time errors because they are user facing directly. I'd just been thinking that we'll use err for the return value, not apis.FieldError. But then as I started thinking about it some more, the are user facing at least in some cases because they will be returned to the user when they try to deploy something.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also felt similar that it should have been errors initially. I followed using apis.ErrGeneric because getAuthorityKeys/parseAuthorityKeys followed the same logic from getKeys. getKeys returned errors using apis.ErrGeneric. I just tried to surface the lowest level error up and made it a common type. Should I change it such that getAuthorityKeys returns errors only and then have validatePodSpec determine how to interpret generic error such as into apis.FieldError?

Or is this something that can be changed later when we actually start erroring out more concretely? The change seems trivial enough either way you want it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: why do we even need to parse the authority keys here ? The function call comes from validatePodSpec, i assume the parsing of the authorities has been done when creating the image policy config, rather when validating the pod spec.

Copy link
Contributor

@DennyHoang DennyHoang Mar 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we get the ImagePolicyConfig, it contains Policies.[]ClusterImagePolicySpec.[]Authority. The returned Authority contains KeyRef.Data which is a string. parseAuthorityKeys takes that string and parses it into ecdsa.PublicKey. I do not believe we have a ecdsa.PublicKey representation of the key earlier.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, sorry I was thinking that we could be defining the signature as error, I think at least (too lazy to look up as I'm on the phone) we could still be returning apierrors as they are errors but not bleed that into the signature of the function.

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 {
Expand Down
Loading