From 5416f97c32460fa72a24ce91f9ad9d53edd3074a Mon Sep 17 00:00:00 2001 From: Jon Johnson Date: Mon, 12 Jun 2023 11:30:14 -0700 Subject: [PATCH] Fix pkg/cosign/errors 1. Remove ThrowError This function is the identity function, so it doesn't appear to actually do anything? Tests pass after deleting it. 2. Unwrap Errors should implement Unwrap when they wrap another error so that errors.Is/As will work with it. 3. Don't break policy-controller This adds back some public symbols that were removed that prevent policy-controller from bumping its cosign dependency. They are marked deprecated. Signed-off-by: Jon Johnson --- pkg/cosign/errors.go | 65 ++++++++++++++++++++++----- pkg/cosign/errors_test.go | 4 +- pkg/cosign/verify.go | 92 ++++++++++++++++++--------------------- pkg/policy/errors.go | 9 +--- pkg/policy/eval.go | 8 ++-- 5 files changed, 105 insertions(+), 73 deletions(-) diff --git a/pkg/cosign/errors.go b/pkg/cosign/errors.go index 0fe40c40808..f728cd4975d 100644 --- a/pkg/cosign/errors.go +++ b/pkg/cosign/errors.go @@ -14,28 +14,23 @@ package cosign +import "fmt" + // VerificationFailure is the type of Go error that is used by cosign to surface // errors actually related to verification (vs. transient, misconfiguration, // transport, or authentication related issues). -// It is now marked as deprecated and will be removed in favour of defined -// error types with use of the ThrowError function. type VerificationFailure struct { err error } -// ThrowError returns the error type that is passed. It acts as a -// single consistent way of throwing errors from the pkg level. -// As long as the error type is defined before hand, this can be -// used to throw errors up to the calling code without discrimination -// around the error type. -func ThrowError(err interface{ error }) error { - return err -} - func (e *VerificationFailure) Error() string { return e.err.Error() } +func (e *VerificationFailure) Unwrap() error { + return e.err +} + type ErrNoMatchingSignatures struct { err error } @@ -44,6 +39,10 @@ func (e *ErrNoMatchingSignatures) Error() string { return e.err.Error() } +func (e *ErrNoMatchingSignatures) Unwrap() error { + return e.err +} + type ErrImageTagNotFound struct { err error } @@ -52,6 +51,10 @@ func (e *ErrImageTagNotFound) Error() string { return e.err.Error() } +func (e *ErrImageTagNotFound) Unwrap() error { + return e.err +} + type ErrNoSignaturesFound struct { err error } @@ -60,6 +63,10 @@ func (e *ErrNoSignaturesFound) Error() string { return e.err.Error() } +func (e *ErrNoSignaturesFound) Unwrap() error { + return e.err +} + type ErrNoMatchingAttestations struct { err error } @@ -68,6 +75,10 @@ func (e *ErrNoMatchingAttestations) Error() string { return e.err.Error() } +func (e *ErrNoMatchingAttestations) Unwrap() error { + return e.err +} + type ErrNoCertificateFoundOnSignature struct { err error } @@ -75,3 +86,35 @@ type ErrNoCertificateFoundOnSignature struct { func (e *ErrNoCertificateFoundOnSignature) Error() string { return e.err.Error() } + +func (e *ErrNoCertificateFoundOnSignature) Unwrap() error { + return e.err +} + +// NewVerificationError exists for backwards compatibility. +// Deprecated: see [VerificationFailure]. +func NewVerificationError(msg string, args ...interface{}) error { + return &VerificationError{ + message: fmt.Sprintf(msg, args...), + } +} + +// VerificationError exists for backwards compatibility. +// Deprecated: see [VerificationFailure]. +type VerificationError struct { + message string +} + +func (e *VerificationError) Error() string { + return e.message +} + +var ( + // ErrNoMatchingAttestationsMessage exists for backwards compatibility. + // Deprecated: see [ErrNoMatchingAttestations]. + ErrNoMatchingAttestationsMessage = "no matching attestations" + + // ErrNoMatchingAttestationsType exists for backwards compatibility. + // Deprecated: see [ErrNoMatchingAttestations]. + ErrNoMatchingAttestationsType = "NoMatchingAttestations" +) diff --git a/pkg/cosign/errors_test.go b/pkg/cosign/errors_test.go index a75cec5faa3..c7a6d3cd988 100644 --- a/pkg/cosign/errors_test.go +++ b/pkg/cosign/errors_test.go @@ -23,8 +23,8 @@ import ( func TestErrors(t *testing.T) { for _, want := range []error{ - ThrowError(&VerificationFailure{errors.Errorf("not a constant %d", 3)}), - ThrowError(&VerificationFailure{errors.Errorf("not a string %s", "i am a string")}), + &VerificationFailure{errors.Errorf("not a constant %d", 3)}, + &VerificationFailure{errors.Errorf("not a string %s", "i am a string")}, } { t.Run(want.Error(), func(t *testing.T) { verr := &VerificationFailure{} diff --git a/pkg/cosign/verify.go b/pkg/cosign/verify.go index 9d7af1ec828..5a2384c43ce 100644 --- a/pkg/cosign/verify.go +++ b/pkg/cosign/verify.go @@ -27,6 +27,7 @@ import ( "encoding/json" "encoding/pem" "fmt" + "net/http" "os" "regexp" "strings" @@ -45,6 +46,7 @@ import ( "github.com/cyberphone/json-canonicalization/go/src/webpki.org/jsoncanonicalizer" "github.com/google/go-containerregistry/pkg/name" v1 "github.com/google/go-containerregistry/pkg/v1" + "github.com/google/go-containerregistry/pkg/v1/remote/transport" ssldsse "github.com/secure-systems-lab/go-securesystemslib/dsse" ociexperimental "github.com/sigstore/cosign/v2/internal/pkg/oci/remote" @@ -169,9 +171,9 @@ func verifyOCIAttestation(ctx context.Context, verifier signature.Verifier, att } if env.PayloadType != types.IntotoPayloadType { - return ThrowError(&VerificationFailure{ + return &VerificationFailure{ fmt.Errorf("invalid payloadType %s on envelope. Expected %s", env.PayloadType, types.IntotoPayloadType), - }) + } } dssev, err := ssldsse.NewEnvelopeVerifier(&dsse.VerifierAdapter{SignatureVerifier: verifier}) if err != nil { @@ -239,10 +241,9 @@ func ValidateAndUnpackCert(cert *x509.Certificate, co *CheckOpts) (signature.Ver return nil, err } if !contains && len(co.SCT) == 0 { - // TODO: add error type instead of blank string - return nil, ThrowError(&VerificationFailure{ + return nil, &VerificationFailure{ fmt.Errorf("certificate does not include required embedded SCT and no detached SCT was set"), - }) + } } // handle if chains has more than one chain - grab first and print message if len(chains) > 1 { @@ -335,9 +336,9 @@ func CheckCertificatePolicy(cert *x509.Certificate, co *CheckOpts) error { return nil } } - return ThrowError(&VerificationFailure{ + return &VerificationFailure{ fmt.Errorf("none of the expected identities matched what was in the certificate, got subjects [%s] with issuer %s", strings.Join(sans, ", "), oidcIssuer), - }) + } } return nil } @@ -345,46 +346,41 @@ func CheckCertificatePolicy(cert *x509.Certificate, co *CheckOpts) error { func validateCertExtensions(ce CertExtensions, co *CheckOpts) error { if co.CertGithubWorkflowTrigger != "" { if ce.GetCertExtensionGithubWorkflowTrigger() != co.CertGithubWorkflowTrigger { - // TODO: add error type instead of blank string - return ThrowError(&VerificationFailure{ + return &VerificationFailure{ fmt.Errorf("expected GitHub Workflow Trigger not found in certificate"), - }) + } } } if co.CertGithubWorkflowSha != "" { if ce.GetExtensionGithubWorkflowSha() != co.CertGithubWorkflowSha { - // TODO: add error type instead of blank string - return ThrowError(&VerificationFailure{ + return &VerificationFailure{ fmt.Errorf("expected GitHub Workflow SHA not found in certificate"), - }) + } } } if co.CertGithubWorkflowName != "" { if ce.GetCertExtensionGithubWorkflowName() != co.CertGithubWorkflowName { - // TODO: add error type instead of blank string - return ThrowError(&VerificationFailure{ + return &VerificationFailure{ fmt.Errorf("expected GitHub Workflow Name not found in certificate"), - }) + } } } if co.CertGithubWorkflowRepository != "" { if ce.GetCertExtensionGithubWorkflowRepository() != co.CertGithubWorkflowRepository { - // TODO: add error type instead of blank string - return ThrowError(&VerificationFailure{ + return &VerificationFailure{ fmt.Errorf("expected GitHub Workflow Repository not found in certificate"), - }) + } } } if co.CertGithubWorkflowRef != "" { if ce.GetCertExtensionGithubWorkflowRef() != co.CertGithubWorkflowRef { - // TODO: add error type instead of blank string - return ThrowError(&VerificationFailure{ + return &VerificationFailure{ fmt.Errorf("expected GitHub Workflow Ref not found in certificate"), - }) + } } } return nil @@ -500,10 +496,10 @@ func VerifyImageSignatures(ctx context.Context, signedImgRef name.Reference, co // entity that minimizes registry requests when supplied with a digest input digest, err := ociremote.ResolveDigest(signedImgRef, co.RegistryClientOpts...) if err != nil { - if strings.Contains(err.Error(), "MANIFEST_UNKNOWN") { - return nil, false, ThrowError(&ErrImageTagNotFound{ + if terr := (&transport.Error{}); errors.As(err, &terr) && terr.StatusCode == http.StatusNotFound { + return nil, false, &ErrImageTagNotFound{ fmt.Errorf("image tag not found: %w", err), - }) + } } return nil, false, err } @@ -589,9 +585,9 @@ func verifySignatures(ctx context.Context, sigs oci.Signatures, h v1.Hash, co *C } if len(sl) == 0 { - return nil, false, ThrowError(&ErrNoMatchingSignatures{ + return nil, false, &ErrNoMatchingSignatures{ errors.New("no matching signatures"), - }) + } } validationErrs := []string{} @@ -613,9 +609,11 @@ func verifySignatures(ctx context.Context, sigs oci.Signatures, h v1.Hash, co *C checkedSignatures = append(checkedSignatures, sig) } if len(checkedSignatures) == 0 { - return nil, false, ThrowError(&ErrNoMatchingSignatures{ + // TODO: ErrNoMatchingSignatures.Unwrap should return []error, + // or we should replace "...%s" strings.Join with "...%w", errors.Join. + return nil, false, &ErrNoMatchingSignatures{ fmt.Errorf("no matching signatures: %s", strings.Join(validationErrs, "\n ")), - }) + } } return checkedSignatures, bundleVerified, nil } @@ -689,9 +687,9 @@ func verifyInternal(ctx context.Context, sig oci.Signature, h v1.Hash, return false, err } if cert == nil { - return false, ThrowError(&ErrNoCertificateFoundOnSignature{ + return false, &ErrNoCertificateFoundOnSignature{ fmt.Errorf("no certificate found on signature"), - }) + } } // Create a certificate pool for intermediate CA certificates, excluding the root chain, err := sig.Chain() @@ -758,10 +756,9 @@ func verifyInternal(ctx context.Context, sig oci.Signature, h v1.Hash, if err := CheckExpiry(cert, time.Now()); err != nil { // If certificate is expired and not signed timestamp was provided then error the following message. Otherwise throw an expiration error. if co.IgnoreTlog && acceptableRFC3161Time == nil { - // TODO: add error type instead of blank string - return false, ThrowError(&VerificationFailure{ + return false, &VerificationFailure{ fmt.Errorf("expected a signed timestamp to verify an expired certificate"), - }) + } } return false, fmt.Errorf("checking expiry on certificate with bundle: %w", err) } @@ -950,9 +947,9 @@ func verifyImageAttestations(ctx context.Context, atts oci.Signatures, h v1.Hash checkedAttestations = append(checkedAttestations, att) } if len(checkedAttestations) == 0 { - return nil, false, ThrowError(&ErrNoMatchingAttestations{ + return nil, false, &ErrNoMatchingAttestations{ fmt.Errorf("no matching attestations: %s", strings.Join(validationErrs, "\n ")), - }) + } } return checkedAttestations, bundleVerified, nil } @@ -963,16 +960,16 @@ func CheckExpiry(cert *x509.Certificate, it time.Time) error { return t.Format(time.RFC3339) } if cert.NotAfter.Before(it) { - return ThrowError(&VerificationFailure{ + return &VerificationFailure{ fmt.Errorf("certificate expired before signatures were entered in log: %s is before %s", ft(cert.NotAfter), ft(it)), - }) + } } if cert.NotBefore.After(it) { - return ThrowError(&VerificationFailure{ + return &VerificationFailure{ fmt.Errorf("certificate was issued after signatures were entered in log: %s is after %s", ft(cert.NotAfter), ft(it)), - }) + } } return nil } @@ -1017,10 +1014,9 @@ func VerifyBundle(sig oci.Signature, co *CheckOpts) (bool, error) { pubKey, ok := co.RekorPubKeys.Keys[bundle.Payload.LogID] if !ok { - // TODO: add error type instead of blank string - return false, ThrowError(&VerificationFailure{ + return false, &VerificationFailure{ fmt.Errorf("verifying bundle: rekor log public key not found for payload"), - }) + } } err = VerifySET(bundle.Payload, bundle.SignedEntryTimestamp, pubKey.PubKey.(*ecdsa.PublicKey)) if err != nil { @@ -1110,10 +1106,9 @@ func compareSigs(bundleBody string, sig oci.Signature) error { return nil } if bundleSignature != actualSig { - // TODO: add error type instead of blank string - return ThrowError(&VerificationFailure{ + return &VerificationFailure{ fmt.Errorf("signature in bundle does not match signature being verified"), - }) + } } return nil } @@ -1316,10 +1311,9 @@ func VerifySET(bundlePayload cbundle.RekorPayload, signature []byte, pub *ecdsa. // verify the SET against the public key hash := sha256.Sum256(canonicalized) if !ecdsa.VerifyASN1(pub, hash[:], signature) { - // TODO: add error type instead of blank string - return ThrowError(&VerificationFailure{ + return &VerificationFailure{ fmt.Errorf("unable to verify SET"), - }) + } } return nil } diff --git a/pkg/policy/errors.go b/pkg/policy/errors.go index 5359a4bb160..8dccb5d8015 100644 --- a/pkg/policy/errors.go +++ b/pkg/policy/errors.go @@ -22,11 +22,6 @@ func (e *EvaluationFailure) Error() string { return e.err.Error() } -// ThrowError returns the error type that is passed. It acts as a -// single consistent way of throwing errors from the pkg level. -// As long as the error type is defined before hand, this can be -// used to throw errors up to the calling code without discrimination -// around the error type. -func ThrowError(err interface{ error }) error { - return err +func (e *EvaluationFailure) Unwrap() error { + return e.err } diff --git a/pkg/policy/eval.go b/pkg/policy/eval.go index 991f34746ab..9e33a8a005d 100644 --- a/pkg/policy/eval.go +++ b/pkg/policy/eval.go @@ -35,16 +35,16 @@ func EvaluatePolicyAgainstJSON(ctx context.Context, name, policyType string, pol case "cue": cueValidationErr := evaluateCue(ctx, jsonBytes, policyBody) if cueValidationErr != nil { - return nil, ThrowError(&EvaluationFailure{ + return nil, &EvaluationFailure{ fmt.Errorf("failed evaluating cue policy for %s: %w", name, cueValidationErr), - }) + } } case "rego": regoValidationWarn, regoValidationErr := evaluateRego(ctx, jsonBytes, policyBody) if regoValidationErr != nil { - return regoValidationWarn, ThrowError(&EvaluationFailure{ + return regoValidationWarn, &EvaluationFailure{ fmt.Errorf("failed evaluating rego policy for type %s: %w", name, regoValidationErr), - }) + } } // It is possible to return warning messages when the policy is compliant return regoValidationWarn, regoValidationErr