Merge pull request #224 from ProtonMail/fix/join-signature-errors

Wrap the cause of signature verification errors.

CHANGELOG.md

@@ -4,6 +4,11 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## Unreleased
+### Changed
+- The `SignatureVerificationError` struct now has a `Cause error` field, which is returned by the the Unwrap function. The cause is also included in the error message.
+	NB: If the caller was relying on the exact message of the error, it might break the flow.
+
 ## [2.6.1] 2023-03-22
 
 ### Security fix

crypto/mime_test.go

@@ -303,7 +303,7 @@ func TestMessageVerificationNotSignedNoVerifier(t *testing.T) {
 
 func TestMessageVerificationNotSignedFailed(t *testing.T) {
 	callbackResults := runScenario(t, "testdata/mime/scenario_13.asc")
-	var expectedErrors = []SignatureVerificationError{newSignatureNotSigned(), newSignatureFailed()}
+	var expectedErrors = []SignatureVerificationError{newSignatureNotSigned(), newSignatureFailed(nil)}
 	compareErrors(expectedErrors, callbackResults.onError, t)
 	expectedStatus := []int{3}
 	compareStatus(expectedStatus, callbackResults.onVerified, t)
@@ -335,7 +335,7 @@ func TestMessageVerificationNoVerifierNoVerifier(t *testing.T) {
 
 func TestMessageVerificationNoVerifierFailed(t *testing.T) {
 	callbackResults := runScenario(t, "testdata/mime/scenario_23.asc")
-	var expectedErrors = []SignatureVerificationError{newSignatureNoVerifier(), newSignatureFailed()}
+	var expectedErrors = []SignatureVerificationError{newSignatureNoVerifier(), newSignatureFailed(nil)}
 	compareErrors(expectedErrors, callbackResults.onError, t)
 	expectedStatus := []int{3}
 	compareStatus(expectedStatus, callbackResults.onVerified, t)

crypto/signature.go

@@ -29,23 +29,33 @@ var allowedHashes = []crypto.Hash{
 type SignatureVerificationError struct {
 	Status  int
 	Message string
+	Cause   error
 }
 
 // Error is the base method for all errors.
 func (e SignatureVerificationError) Error() string {
+	if e.Cause != nil {
+		return fmt.Sprintf("Signature Verification Error: %v caused by %v", e.Message, e.Cause)
+	}
 	return fmt.Sprintf("Signature Verification Error: %v", e.Message)
 }
 
+// Unwrap returns the cause of failure.
+func (e SignatureVerificationError) Unwrap() error {
+	return e.Cause
+}
+
 // ------------------
 // Internal functions
 // ------------------
 
 // newSignatureFailed creates a new SignatureVerificationError, type
 // SignatureFailed.
-func newSignatureFailed() SignatureVerificationError {
+func newSignatureFailed(cause error) SignatureVerificationError {
 	return SignatureVerificationError{
 		Status:  constants.SIGNATURE_FAILED,
 		Message: "Invalid signature",
+		Cause:   cause,
 	}
 }
 
@@ -108,7 +118,7 @@ func verifyDetailsSignature(md *openpgp.MessageDetails, verifierKey *KeyRing) er
 		return newSignatureNoVerifier()
 	}
 	if md.SignatureError != nil {
-		return newSignatureFailed()
+		return newSignatureFailed(md.SignatureError)
 	}
 	if md.Signature == nil ||
 		md.Signature.Hash < allowedHashes[0] ||
@@ -238,21 +248,25 @@ func verifySignature(
 
 			_, err = signatureReader.Seek(0, io.SeekStart)
 			if err != nil {
-				return nil, newSignatureFailed()
+				return nil, newSignatureFailed(err)
 			}
 
 			sig, signer, err = openpgp.VerifyDetachedSignatureAndHash(pubKeyEntries, origText, signatureReader, allowedHashes, config)
 		}
 	}
 
-	if err != nil || sig == nil || signer == nil {
+	if err != nil {
-		return nil, newSignatureFailed()
+		return nil, newSignatureFailed(err)
+	}
+
+	if sig == nil || signer == nil {
+		return nil, newSignatureFailed(errors.New("gopenpgp: no signer or valid signature"))
 	}
 
 	if verificationContext != nil {
 		err := verificationContext.verifyContext(sig)
 		if err != nil {
-			return nil, newSignatureFailed()
+			return nil, newSignatureFailed(err)
 		}
 	}
 

crypto/signature_collector.go

@@ -110,7 +110,7 @@ func (sc *SignatureCollector) Accept(
 		case errors.Is(err, pgpErrors.ErrUnknownIssuer):
 			sc.verified = newSignatureNoVerifier()
 		default:
-			sc.verified = newSignatureFailed()
+			sc.verified = newSignatureFailed(err)
 		}
 	} else {
 		sc.verified = newSignatureNoVerifier()

crypto/signature_test.go

@@ -75,11 +75,25 @@ func TestVerifyTextDetachedSig(t *testing.T) {
 	}
 }
 
+func checkVerificationError(t *testing.T, err error, expectedStatus int) { //nolint: unparam
+	if err == nil {
+		t.Fatalf("Expected a verification error")
+	}
+	castedErr := &SignatureVerificationError{}
+	isType := errors.As(err, castedErr)
+	if !isType {
+		t.Fatalf("Error was not a verification errror: %v", err)
+	}
+	if castedErr.Status != expectedStatus {
+		t.Fatalf("Expected status to be %d got %d", expectedStatus, castedErr.Status)
+	}
+}
+
 func TestVerifyTextDetachedSigWrong(t *testing.T) {
 	fakeMessage := NewPlainMessageFromString("wrong text")
 	verificationError := keyRingTestPublic.VerifyDetached(fakeMessage, textSignature, testTime)
 
-	assert.EqualError(t, verificationError, "Signature Verification Error: Invalid signature")
+	checkVerificationError(t, verificationError, constants.SIGNATURE_FAILED)
 
 	err := &SignatureVerificationError{}
 	_ = errors.As(verificationError, err)
@@ -342,9 +356,7 @@ func Test_VerifyDetachedWithUnknownCriticalContext(t *testing.T) {
 		0,
 	)
 	// then
-	if err == nil || !errors.Is(err, newSignatureFailed()) {
+	checkVerificationError(t, err, constants.SIGNATURE_FAILED)
-		t.Fatalf("Expected a verification error")
-	}
 }
 
 func Test_VerifyDetachedWithUnKnownNonCriticalContext(t *testing.T) {
@@ -423,9 +435,7 @@ func Test_VerifyDetachedWithWrongContext(t *testing.T) {
 		verificationContext,
 	)
 	// then
-	if err == nil || !errors.Is(err, newSignatureFailed()) {
+	checkVerificationError(t, err, constants.SIGNATURE_FAILED)
-		t.Fatalf("Expected a verification error")
-	}
 }
 
 func Test_VerifyDetachedWithMissingNonRequiredContext(t *testing.T) {
@@ -481,9 +491,7 @@ func Test_VerifyDetachedWithMissingRequiredContext(t *testing.T) {
 		verificationContext,
 	)
 	// then
-	if err == nil || !errors.Is(err, newSignatureFailed()) {
+	checkVerificationError(t, err, constants.SIGNATURE_FAILED)
-		t.Fatalf("Expected a verification error")
-	}
 }
 
 func Test_VerifyDetachedWithMissingRequiredContextBeforeCutoff(t *testing.T) {
@@ -553,9 +561,7 @@ func Test_VerifyDetachedWithMissingRequiredContextAfterCutoff(t *testing.T) {
 		verificationContext,
 	)
 	// then
-	if err == nil || !errors.Is(err, newSignatureFailed()) {
+	checkVerificationError(t, err, constants.SIGNATURE_FAILED)
-		t.Fatalf("Expected a verification error")
-	}
 }
 
 func Test_VerifyDetachedWithDoubleContext(t *testing.T) {
@@ -581,7 +587,5 @@ func Test_VerifyDetachedWithDoubleContext(t *testing.T) {
 		verificationContext,
 	)
 	// then
-	if err == nil || !errors.Is(err, newSignatureFailed()) {
+	checkVerificationError(t, err, constants.SIGNATURE_FAILED)
-		t.Fatalf("Expected a verification error")
-	}
 }