Fix pkg/cosign/errors (#3050)

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 <[email protected]>

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,10 +75,46 @@ func (e *ErrNoMatchingAttestations) Error() string {
 	return e.err.Error()
 }
 
+func (e *ErrNoMatchingAttestations) Unwrap() error {
+	return e.err
+}
+
 type ErrNoCertificateFoundOnSignature struct {
 	err error
 }
 
 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"
+)

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{}

pkg/cosign/verify.go

@@ -27,6 +27,7 @@ import (
 	"encoding/json"
 	"encoding/pem"
 	"fmt"
+	"net/http"
 	"os"
 	"regexp"
 	"strings"
@@ -46,6 +47,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"
@@ -176,9 +178,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 {
@@ -246,10 +248,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 {
@@ -342,56 +343,51 @@ 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
 }
 
 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
@@ -507,10 +503,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
 	}
@@ -596,9 +592,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{}
@@ -620,9 +616,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
 }
@@ -696,9 +694,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()
@@ -765,10 +763,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)
 			}
@@ -957,9 +954,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
 }
@@ -970,16 +967,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
 }
@@ -1024,10 +1021,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 {
@@ -1117,10 +1113,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
 }
@@ -1259,10 +1254,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
 }

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
 }