code review 125030043: go.crypto/openpgp: return signature error rather than u...

go.crypto/openpgp: return signature error rather than unknown issuer.

In the event that a detached signature fails to verify, the code would
continue trying to find other keys with the same key ID and eventually
conclude that the signature was issued by someone unknown
(ErrUnknownIssuer).

With this change, the signature verification error would be returned
instead. (Technically the last error if several keys had a matching key
id and neither verified.)

Fixes issue 8509.

[agl1, 2014-08-11 20:18:08]

Hello bradfitz@golang.org (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go.crypto

[bradfitz, 2014-08-11 20:32:38]

LGTM



On Mon, Aug 11, 2014 at 1:18 PM, <agl@golang.org> wrote:

> Reviewers: bradfitz,
>
> Message:
> Hello bradfitz@golang.org (cc: golang-codereviews@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go.crypto
>
>
> Description:
> go.crypto/openpgp: return signature error rather than unknown issuer.
>
> In the event that a detached signature fails to verify, the code would
> continue trying to find other keys with the same key ID and eventually
> conclude that the signature was issued by someone unknown
> (ErrUnknownIssuer).
>
> With this change, the signature verification error would be returned
> instead. (Technically the last error if several keys had a matching key
> id and neither verified.)
>
> Fixes issue 8509.
>
> Please review this at https://codereview.appspot.com/125030043/
>
> Affected files (+13, -1 lines):
>   M openpgp/read.go
>   M openpgp/read_test.go
>
>
> Index: openpgp/read.go
> ===================================================================
> --- a/openpgp/read.go
> +++ b/openpgp/read.go
> @@ -405,7 +405,10 @@
>                 }
>         }
>
> -       return nil, errors.ErrUnknownIssuer
> +       if err == nil {
> +               err = errors.ErrUnknownIssuer
> +       }
> +       return nil, err
>  }
>
>  // CheckArmoredDetachedSignature performs the same actions as
> Index: openpgp/read_test.go
> ===================================================================
> --- a/openpgp/read_test.go
> +++ b/openpgp/read_test.go
> @@ -279,6 +279,15 @@
>         testDetachedSignature(t, kring, readerFromHex(detachedSignatureHex),
> signedInput, "binary", testKey1KeyId)
>         testDetachedSignature(t, kring,
readerFromHex(detachedSignatureTextHex),
> signedInput, "text", testKey1KeyId)
>         testDetachedSignature(t, kring,
readerFromHex(detachedSignatureV3TextHex),
> signedInput, "v3", testKey1KeyId)
> +
> +       incorrectSignedInput := signedInput + "X"
> +       _, err := CheckDetachedSignature(kring,
bytes.NewBufferString(incorrectSignedInput),
> readerFromHex(detachedSignatureHex))
> +       if err == nil {
> +               t.Fatal("CheckDetachedSignature returned without error
> for bad signature")
> +       }
> +       if err == errors.ErrUnknownIssuer {
> +               t.Fatal("CheckDetachedSignature returned ErrUnknownIssuer
> when the signer was known, but the signature invalid")
> +       }
>  }
>
>  func TestDetachedSignatureDSA(t *testing.T) {
>
>
>

[agl1, 2014-08-11 22:42:03]

*** Submitted as
https://code.google.com/p/go/source/detail?r=d1adf2fbd405&repo=crypto ***

go.crypto/openpgp: return signature error rather than unknown issuer.

In the event that a detached signature fails to verify, the code would
continue trying to find other keys with the same key ID and eventually
conclude that the signature was issued by someone unknown
(ErrUnknownIssuer).

With this change, the signature verification error would be returned
instead. (Technically the last error if several keys had a matching key
id and neither verified.)

Fixes issue 8509.

LGTM=bradfitz
R=bradfitz
CC=golang-codereviews
https://codereview.appspot.com/125030043