code review 12055043: go.crypto/openpgp: test that hashes are compiled in bef...

go.crypto/openpgp: test that hashes are compiled in before using them.

Previously, if Encrypt decided to use a hash function that wasn't
compiled in then it would panic.

[agl1, 2013-08-05 15:18:35]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

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

[bradfitz, 2013-08-05 15:20:26]

LGTM

But pretty weak error message.  "Wanted hash function 7 in this case"  =>
Seven?  (or whatever)

Can we get a table of common one's names and what Go package to _ import?




On Mon, Aug 5, 2013 at 8:18 AM, <agl@golang.org> wrote:

> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go.**crypto/<https://code.google.com/p/go.crypto/>
>
>
> Description:
> go.crypto/openpgp: test that hashes are compiled in before using them.
>
> Previously, if Encrypt decided to use a hash function that wasn't
> compiled in then it would panic.
>
> Please review this at
https://codereview.appspot.**com/12055043/<https://codereview.appspot.com/120...
>
> Affected files:
>   M openpgp/write.go
>
>
> Index: openpgp/write.go
> ==============================**==============================**=======
> --- a/openpgp/write.go
> +++ b/openpgp/write.go
> @@ -224,16 +224,27 @@
>                 }
>         }
>
> -       hashFunc := candidateHashes[0]
> -       // If the hash specified by config is a candidate, we'll use that.
> -       configuredHash := config.Hash()
> -       for _, h := range candidateHashes {
> -               if h == uint8(configuredHash) {
> -                       hashFunc = h
> +       var hash crypto.Hash
> +       for _, hashId := range candidateHashes {
> +               if h, ok := s2k.HashIdToHash(hashId); ok && h.Available() {
> +                       hash = h
>                         break
>                 }
>         }
> -       hash, _ := s2k.HashIdToHash(hashFunc)
> +
> +       // If the hash specified by config is a candidate, we'll use that.
> +       if configuredHash := config.Hash(); configuredHash.Available() {
> +               for _, hashId := range candidateHashes {
> +                       if h, ok := s2k.HashIdToHash(hashId); ok && h ==
> configuredHash {
> +                               hash = h
> +                               break
> +                       }
> +               }
> +       }
> +
> +       if hash == 0 {
> +               return nil, errors.InvalidArgumentError("**cannot encrypt
> because no candidate hash functions are compiled in. (Wanted hash function
> " + strconv.Itoa(int(**candidateHashes[0])) + " in this case.)")
> +       }
>
>         symKey := make([]byte, cipher.KeySize())
>         if _, err := io.ReadFull(config.Random(), symKey); err != nil {
>
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[agl1, 2013-08-05 16:52:30]

On 2013/08/05 15:20:26, bradfitz wrote:
> LGTM
> 
> But pretty weak error message.  "Wanted hash function 7 in this case"  =>
> Seven?  (or whatever)
> 
> Can we get a table of common one's names and what Go package to _ import?

Having a table of package paths seems weird, but I've added a function to s2k to
return the name of a hash.

[bradfitz, 2013-08-05 16:57:51]

https://codereview.appspot.com/12055043/diff/14002/openpgp/s2k/s2k.go
File openpgp/s2k/s2k.go (right):

https://codereview.appspot.com/12055043/diff/14002/openpgp/s2k/s2k.go#newcode177
openpgp/s2k/s2k.go:177: // given OpenPGP hash id, or panics if id is unknown.
HashIdToHash and HashToHashId return comma-ok. Shouldn't this, for consistency?

[agl1, 2013-08-05 18:19:15]

https://codereview.appspot.com/12055043/diff/14002/openpgp/s2k/s2k.go
File openpgp/s2k/s2k.go (right):

https://codereview.appspot.com/12055043/diff/14002/openpgp/s2k/s2k.go#newcode177
openpgp/s2k/s2k.go:177: // given OpenPGP hash id, or panics if id is unknown.
On 2013/08/05 16:57:51, bradfitz wrote:
> HashIdToHash and HashToHashId return comma-ok. Shouldn't this, for
consistency?

Done.

[bradfitz, 2013-08-05 18:21:30]

LGTM



On Mon, Aug 5, 2013 at 11:19 AM, <agl@golang.org> wrote:

>
>
https://codereview.appspot.**com/12055043/diff/14002/**openpgp/s2k/s2k.go<htt...
> File openpgp/s2k/s2k.go (right):
>
> https://codereview.appspot.**com/12055043/diff/14002/**
>
openpgp/s2k/s2k.go#newcode177<https://codereview.appspot.com/12055043/diff/14002/openpgp/s2k/s2k.go#newcode177>
> openpgp/s2k/s2k.go:177: // given OpenPGP hash id, or panics if id is
> unknown.
> On 2013/08/05 16:57:51, bradfitz wrote:
>
>> HashIdToHash and HashToHashId return comma-ok. Shouldn't this, for
>>
> consistency?
>
> Done.
>
>
https://codereview.appspot.**com/12055043/<https://codereview.appspot.com/120...
>

[agl1, 2013-08-05 18:23:08]

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

go.crypto/openpgp: test that hashes are compiled in before using them.

Previously, if Encrypt decided to use a hash function that wasn't
compiled in then it would panic.

R=golang-dev, bradfitz
CC=golang-dev
https://codereview.appspot.com/12055043