From e9fca4d62fe82cc0d9f56b9b549a5e13d07c4669 Mon Sep 17 00:00:00 2001 From: "M. Thiercelin" Date: Mon, 6 Mar 2023 13:52:17 +0100 Subject: [PATCH] Wrap the cause of signature verification errors. Instead of swallowing the cause of verification errors, we use error wrapping to communicate the cause to the caller. --- CHANGELOG.md | 5 +++++ crypto/mime_test.go | 4 ++-- crypto/signature.go | 26 +++++++++++++++++++------ crypto/signature_collector.go | 2 +- crypto/signature_test.go | 36 +++++++++++++++++++---------------- 5 files changed, 48 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e0460c..aedec55 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/crypto/mime_test.go b/crypto/mime_test.go index 22ba171..a7f1192 100644 --- a/crypto/mime_test.go +++ b/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) diff --git a/crypto/signature.go b/crypto/signature.go index c94c966..bc34184 100644 --- a/crypto/signature.go +++ b/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 { - return nil, newSignatureFailed() + if err != nil { + 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) } } diff --git a/crypto/signature_collector.go b/crypto/signature_collector.go index 05f2ecb..fb769a9 100644 --- a/crypto/signature_collector.go +++ b/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() diff --git a/crypto/signature_test.go b/crypto/signature_test.go index eec95f4..d2f9166 100644 --- a/crypto/signature_test.go +++ b/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()) { - t.Fatalf("Expected a verification error") - } + checkVerificationError(t, err, constants.SIGNATURE_FAILED) } 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()) { - t.Fatalf("Expected a verification error") - } + checkVerificationError(t, err, constants.SIGNATURE_FAILED) } 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()) { - t.Fatalf("Expected a verification error") - } + checkVerificationError(t, err, constants.SIGNATURE_FAILED) } 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()) { - t.Fatalf("Expected a verification error") - } + checkVerificationError(t, err, constants.SIGNATURE_FAILED) } 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()) { - t.Fatalf("Expected a verification error") - } + checkVerificationError(t, err, constants.SIGNATURE_FAILED) }