Skip to content

Commit

Permalink
Refactor based on discussions in sigstore#1650 (sigstore#1674)
Browse files Browse the repository at this point in the history 
* Refactor based on discussions in sigstore#1650

Signed-off-by: Ville Aikas <[email protected]>

* Simplify return signature.

Signed-off-by: Ville Aikas <[email protected]>

* pr feedback.

Signed-off-by: Ville Aikas <[email protected]>

* remove now unnecessary var.

Signed-off-by: Ville Aikas <[email protected]>
  • Loading branch information
@vaikas @mlieberman85
vaikas authored and mlieberman85 committed May 6, 2022
1 parent 68f33f0 commit d1e9005
Show file tree
Hide file tree
Showing 2 changed files with 67 additions and 60 deletions.
12 changes: 6 additions & 6 deletions pkg/cosign/kubernetes/webhook/validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,17 +36,17 @@ import (
"github.com/sigstore/sigstore/pkg/signature"
)

func valid(ctx context.Context, ref name.Reference, keys []*ecdsa.PublicKey, opts ...ociremote.Option) error {
func valid(ctx context.Context, ref name.Reference, keys []*ecdsa.PublicKey, opts ...ociremote.Option) ([]oci.Signature, error) {
if len(keys) == 0 {
// If there are no keys, then verify against the fulcio root.
sps, err := validSignaturesWithFulcio(ctx, ref, fulcioroots.Get(), nil /* rekor */, opts...)
if err != nil {
return err
return nil, err
}
if len(sps) > 0 {
return nil
return sps, nil
}
return errors.New("no valid signatures were found")
return nil, errors.New("no valid signatures were found")
}
// We return nil if ANY key matches
var lastErr error
Expand All @@ -65,11 +65,11 @@ func valid(ctx context.Context, ref name.Reference, keys []*ecdsa.PublicKey, opt
continue
}
if len(sps) > 0 {
return nil
return sps, nil
}
}
logging.FromContext(ctx).Debug("No valid signatures were found.")
return lastErr
return nil, lastErr
}

// For testing
Expand Down
115 changes: 61 additions & 54 deletions pkg/cosign/kubernetes/webhook/validator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
"github.com/pkg/errors"
"github.com/sigstore/cosign/pkg/apis/config"
"github.com/sigstore/cosign/pkg/apis/cosigned/v1alpha1"
"github.com/sigstore/cosign/pkg/oci"
ociremote "github.com/sigstore/cosign/pkg/oci/remote"
"github.com/sigstore/fulcio/pkg/api"
rekor "github.com/sigstore/rekor/pkg/client"
Expand Down Expand Up @@ -167,8 +168,8 @@ func (v *Validator) validatePodSpec(ctx context.Context, ps *corev1.PodSpec, opt
// If there is at least one policy that matches, that means it
// has to be satisfied.
if len(policies) > 0 {
av, fieldErrors := validatePolicies(ctx, ref, kc, policies)
if av != len(policies) {
signatures, fieldErrors := validatePolicies(ctx, ref, kc, policies)
if len(signatures) != len(policies) {
logging.FromContext(ctx).Warnf("Failed to validate at least one policy for %s", ref.Name())
// Do we really want to add all the error details here?
// Seems like we can just say which policy failed, so
Expand All @@ -190,7 +191,7 @@ func (v *Validator) validatePodSpec(ctx context.Context, ps *corev1.PodSpec, opt
// Only say we passed (aka, we skip the traditidional check
// below) if more than one authority was validated, which
// means that there was a matching ClusterImagePolicy.
if av > 0 {
if len(signatures) > 0 {
passedPolicyChecks = true
}
}
Expand All @@ -202,7 +203,7 @@ func (v *Validator) validatePodSpec(ctx context.Context, ps *corev1.PodSpec, opt
continue
}

if err := valid(ctx, ref, containerKeys, ociremote.WithRemoteOptions(remote.WithAuthFromKeychain(kc))); err != nil {
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 @@ -218,14 +219,17 @@ func (v *Validator) validatePodSpec(ctx context.Context, ps *corev1.PodSpec, opt
}

// validatePolicies will go through all the matching Policies and their
// Authorities for a given image. Returns the number of Policies that
// had at least one successful validation against it.
// If there's a policy that did not match, it will be returned in the map
// Authorities for a given image. Returns the map of policy=>Validated
// signatures. From the map you can see the number of matched policies along
// with the signatures that were verified.
// If there's a policy that did not match, it will be returned in the errors map
// along with all the errors that caused it to fail.
// Note that if an image does not match any policies, it's perfectly
// reasonable that the return value is 0, nil since there were no errors, but
// the image was not validated against any matching policy and hence authority.
func validatePolicies(ctx context.Context, ref name.Reference, defaultKC authn.Keychain, policies map[string][]v1alpha1.Authority, _ ...ociremote.Option) (int, map[string][]error) {
func validatePolicies(ctx context.Context, ref name.Reference, kc authn.Keychain, policies map[string][]v1alpha1.Authority, remoteOpts ...ociremote.Option) (map[string][]oci.Signature, map[string][]error) {
// Gather all validated signatures here.
signatures := map[string][]oci.Signature{}
// For a policy that does not pass at least one authority, gather errors
// here so that we can give meaningful errors to the user.
ret := map[string][]error{}
Expand All @@ -234,46 +238,59 @@ func validatePolicies(ctx context.Context, ref name.Reference, defaultKC authn.K
// From the Design document, the part about multiple Policies matching:
// "If multiple policies match a particular image, then ALL of those
// policies must be satisfied for the image to be admitted."
// So we keep a tally to make sure that all the policies matched.
policiesValidated := 0
// If none of the Authorities for a given policy pass the checks, gather
// the errors here. If one passes, do not return the errors.
authorityErrors := []error{}
for p, authorities := range policies {
logging.FromContext(ctx).Debugf("Checking Policy: %s", p)
// Now the part about having multiple Authority sections within a
// policy, any single match will do:
// "When multiple authorities are specified, any of them may be used
// to source the valid signature we are looking for to admit an image.""
authoritiesValidated := 0
for _, authority := range authorities {
logging.FromContext(ctx).Debugf("Checking Authority: %+v", authority)
// TODO(vaikas): We currently only use the defaultKC, we have to look
// at authority.Sources to determine additional information for the
// WithRemoteOptions below, at least the 'TargetRepository'
// https://github.com/sigstore/cosign/issues/1651
opts := ociremote.WithRemoteOptions(remote.WithAuthFromKeychain(defaultKC))

if authority.Key != nil {
// Get the key from authority data
if authorityKeys, fieldErr := parseAuthorityKeys(ctx, authority.Key.Data); fieldErr != nil {
authorityErrors = append(authorityErrors, errors.Wrap(fieldErr, "failed to parse Key values"))
sigs, errs := ValidatePolicy(ctx, ref, kc, authorities, remoteOpts...)
if len(errs) > 0 {
ret[p] = append(ret[p], authorityErrors...)
} else {
signatures[p] = append(signatures[p], sigs...)
}
}
return signatures, ret
}

// ValidatePolicy will go through all the Authorities for a given image and
// return a success if at least one of the Authorities validated the signatures.
// Returns the validated signatures, or the errors encountered.
func ValidatePolicy(ctx context.Context, ref name.Reference, kc authn.Keychain, authorities []v1alpha1.Authority, remoteOpts ...ociremote.Option) ([]oci.Signature, []error) {
// If none of the Authorities for a given policy pass the checks, gather
// the errors here. If one passes, do not return the errors.
authorityErrors := []error{}
for _, authority := range authorities {
logging.FromContext(ctx).Debugf("Checking Authority: %+v", authority)
// TODO(vaikas): We currently only use the kc, we have to look
// at authority.Sources to determine additional information for the
// WithRemoteOptions below, at least the 'TargetRepository'
// https://github.com/sigstore/cosign/issues/1651

switch {
case authority.Key != nil:
// Get the key from authority data
if authorityKeys, fieldErr := parseAuthorityKeys(ctx, authority.Key.Data); fieldErr != nil {
authorityErrors = append(authorityErrors, errors.Wrap(fieldErr, "failed to parse Key values"))
} else {
// TODO(vaikas): What should happen if there are multiple keys
// Is it even allowed? 'valid' returns success if any key
// matches.
// https://github.com/sigstore/cosign/issues/1652
sps, err := valid(ctx, ref, authorityKeys, remoteOpts...)
if err != nil {
authorityErrors = append(authorityErrors, errors.Wrap(err, "failed to validate keys"))
continue
} else {
// TODO(vaikas): What should happen if there are multiple keys
// Is it even allowed? 'valid' returns success if any key
// matches.
// https://github.com/sigstore/cosign/issues/1652
if err := valid(ctx, ref, authorityKeys, opts); err != nil {
authorityErrors = append(authorityErrors, errors.Wrap(err, "failed to validate keys"))
continue
if len(sps) > 0 {
logging.FromContext(ctx).Debugf("validated signature for %s, got %d signatures", len(sps))
return sps, nil
}
// This authority matched, so mark it as validated and
// continue through other policies, no need to look at more
// of the Authorities.
authoritiesValidated++
break
logging.FromContext(ctx).Errorf("no validSignatures found for %s", ref.Name())
authorityErrors = append(authorityErrors, fmt.Errorf("no valid signatures found for %s", ref.Name()))
}
}
case authority.Keyless != nil:
if authority.Keyless != nil && authority.Keyless.URL != nil {
logging.FromContext(ctx).Debugf("Fetching FulcioRoot for %s : From: %s ", ref.Name(), authority.Keyless.URL)
fulcioroot, err := getFulcioCert(authority.Keyless.URL)
Expand All @@ -291,32 +308,22 @@ func validatePolicies(ctx context.Context, ref name.Reference, defaultKC authn.K
continue
}
}
sps, err := validSignaturesWithFulcio(ctx, ref, fulcioroot, rekorClient, opts)
sps, err := validSignaturesWithFulcio(ctx, ref, fulcioroot, rekorClient, remoteOpts...)
if err != nil {
logging.FromContext(ctx).Errorf("failed validSignatures with fulcio for %s: %v", ref.Name(), err)
authorityErrors = append(authorityErrors, errors.Wrap(err, "validate signatures with fulcio"))
} else {
if len(sps) > 0 {
logging.FromContext(ctx).Debugf("validated signature for %s, got %d signatures", len(sps))
// This authority matched, so mark it as validated and
// continue through other policies, no need to look at
// more of the Authorities.
authoritiesValidated++
break
} else {
logging.FromContext(ctx).Errorf("no validSignatures found for %s", ref.Name())
authorityErrors = append(authorityErrors, fmt.Errorf("no valid signatures found for %s", ref.Name()))
return sps, nil
}
logging.FromContext(ctx).Errorf("no validSignatures found for %s", ref.Name())
authorityErrors = append(authorityErrors, fmt.Errorf("no valid signatures found for %s", ref.Name()))
}
}
}
if authoritiesValidated > 0 {
policiesValidated++
} else {
ret[p] = append(ret[p], authorityErrors...)
}
}
return policiesValidated, ret
return nil, authorityErrors
}

// ResolvePodSpecable implements duckv1.PodSpecValidator
Expand Down

0 comments on commit d1e9005

Please sign in to comment.