code review 5550043: crypto/hmac: Add HMAC-SHA224 and HMAC-SHA384/512

crypto/hmac: Add HMAC-SHA224 and HMAC-SHA384/512

First was, apart from adding tests, a single line of code (to add the
constructor function). Adding SHA512-based hashing to crypto/hmac
required minor rework of the package because of a previously hardcoded
block-size in it's implementation. Instead of using a hash.Hash
generator function the constructor function now uses a crypto.Hash
type, which was extended to expose information about block size.

The only standard library package impacted by the change is
crypto/tls, for which the fix is included in this patch. It might be
useful to extend gofix to include this API change too.

[Luit, 2012-01-17 22:27:24]

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

I'd like you to review this change to
https://go.googlecode.com/hg/

[Luit, 2012-01-17 22:48:51]

Now with CC: rsc, agl1 

(sorry if you're not the right people to CC)

[rsc, 2012-01-17 22:56:29]

The implication of the crypto API change is that for some
crypto.Hash h, h.BlockSize() != h.New().BlockSize()
(otherwise you would not need an API change).
Why does that make sense?

Russ

[Luit, 2012-01-17 23:07:56]

I'm not sure I fully understand what you're trying to say, but for h
crypto.Hash; h.New() doesn't have a BlockSize method... Are you saying it should
have one, so hash.Hash needs to be expanded?

-Luit

[agl1, 2012-01-17 23:08:00]

> The implication of the crypto API change is that for some
> crypto.Hash h, h.BlockSize() != h.New().BlockSize()

But there isn't an h.BlockSize() because there's no BlockSize method on
hash.Hash.

http://codereview.appspot.com/5550043/diff/2001/src/pkg/crypto/crypto.go
File src/pkg/crypto/crypto.go (right):

http://codereview.appspot.com/5550043/diff/2001/src/pkg/crypto/crypto.go#newc...
src/pkg/crypto/crypto.go:64: // size. It doesn't require that the hash function
in question in question be
typo.

http://codereview.appspot.com/5550043/diff/2001/src/pkg/crypto/hmac/hmac.go
File src/pkg/crypto/hmac/hmac.go (right):

http://codereview.appspot.com/5550043/diff/2001/src/pkg/crypto/hmac/hmac.go#n...
src/pkg/crypto/hmac/hmac.go:72: hm.size = hm.inner.Size()
if h.New() returns nil, this will crash. It's also worth noting in the function
comment that it will return nil if the hash function isn't linked in.

[Luit, 2012-01-17 23:15:56]

What do you propose to fix the nil-returning New()?

http://codereview.appspot.com/5550043/diff/2001/src/pkg/crypto/hmac/hmac.go
File src/pkg/crypto/hmac/hmac.go (right):

http://codereview.appspot.com/5550043/diff/2001/src/pkg/crypto/hmac/hmac.go#n...
src/pkg/crypto/hmac/hmac.go:72: hm.size = hm.inner.Size()
I did add the underscore-imports for at least md5 and the sha-variations...
still, fair points

[Luit, 2012-01-17 23:17:12]

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

Please take another look.

[Luit, 2012-01-17 23:17:45]

http://codereview.appspot.com/5550043/diff/2001/src/pkg/crypto/crypto.go
File src/pkg/crypto/crypto.go (right):

http://codereview.appspot.com/5550043/diff/2001/src/pkg/crypto/crypto.go#newc...
src/pkg/crypto/crypto.go:64: // size. It doesn't require that the hash function
in question in question be
On 2012/01/17 23:08:00, agl1 wrote:
> typo.

Done.

[agl1, 2012-01-17 23:18:25]

On 2012/01/17 23:15:56, Luit wrote:
> What do you propose to fix the nil-returning New()?

The NewXXX functions can't fail, which is fine. But I think that New() should
now return a (hash.Hash, bool) in order to make it very clear that it can fail.

[Luit, 2012-01-17 23:23:17]

On 2012/01/17 23:18:25, agl1 wrote:
> The NewXXX functions can't fail, which is fine. But I think that New() should
> now return a (hash.Hash, bool) in order to make it very clear that it can
fail.

It starting to look like there could better be a type BlockHash
interface{hash.Hash;BlockSize()} implemented by MD5, SHA, et-al

[agl1, 2012-01-17 23:25:41]

On Tue, Jan 17, 2012 at 6:23 PM,  <luitvd@gmail.com> wrote:
> It starting to look like there could better be a type BlockHash
> interface{hash.Hash;BlockSize()} implemented by MD5, SHA, et-al

If we stuck interface BlockHash in crypto then I think I'd be ok with that.


Cheers

AGL

[r2, 2012-01-17 23:26:56]

I need to be talked into any API changes looming. Go 1 is in the pipe.

-rob

[Luit, 2012-01-17 23:27:51]

On 2012/01/17 23:25:41, agl1 wrote:
> On Tue, Jan 17, 2012 at 6:23 PM,  <mailto:luitvd@gmail.com> wrote:
> > It starting to look like there could better be a type BlockHash
> > interface{hash.Hash;BlockSize()} implemented by MD5, SHA, et-al
> 
> If we stuck interface BlockHash in crypto then I think I'd be ok with that.
> 
> 
> Cheers
> 
> AGL

I'll get to that tomorrow! (in some 10 hours)

-Luit

[Luit, 2012-01-17 23:30:44]

On 2012/01/17 23:26:56, r2 wrote:
> I need to be talked into any API changes looming. Go 1 is in the pipe.
> 
> -rob
> 

I'll update this issue on that too, tomorrow. It'll only hit crypto/hmac AFAICS.
Also, I think it would be nice if someone could write a gofix for this too. I'm
not sure I'm able to.

[rsc, 2012-01-18 03:02:08]

I spent a while looking at this and toying with different
changes that would make these possible.

I think the simplest thing is to expand hash.Hash to include

// BlockSize returns the hash's underlying block size.
// The Write method must be able to accept any amount
// of data, but it may operate more efficiently if all writes
// are a multiple of the block size.
BlockSize() int

That will require adding BlockSize methods that return 1
in adler32, crc32, crc64, and fnv, but that is easily done.

At that point, no changes are necessary in the API of
any other package.  crypto/hmac can keep its API and
use the new BlockSize method to address the TODO.

I do not believe that we should add SHA224, SHA384,
and SHA512 functions to the crypto/hmac package.
These are easily done with hmac.New(sha224.New, key)
once we have addressed the block size issue, and requiring
that invocation keeps from setting the precedent that
every possible hash function is linked in with crypto/hmac.

Russ

[Luit, 2012-01-18 08:38:11]

On 2012/01/18 03:02:08, rsc wrote:
> I think the simplest thing is to expand hash.Hash to include
> 
> At that point, no changes are necessary in the API of
> any other package.  crypto/hmac can keep its API and
> use the new BlockSize method to address the TODO.

Isn't changing hash.Hash an API change too? I think adding a new interface
(either crypto.BlockHash or hash.BlockHash) as hash.Hash + BlockSize() might be
better, having (md5|sha1|sha256|etc.).New() return that interface type, which
will cleanly work as hash.Hash, so no real change on how it's used. We still
need the API change in the crypto/hmac package, but it's much more minor than
what I proposed in these patches so far.

> 
> I do not believe that we should add SHA224, SHA384,
> and SHA512 functions to the crypto/hmac package.
> These are easily done with hmac.New(sha224.New, key)
> once we have addressed the block size issue, and requiring
> that invocation keeps from setting the precedent that
> every possible hash function is linked in with crypto/hmac.
> 
> Russ

SHA-224 is in the crypto/sha256 package, so does that really matter that much?
Also, there's only five approved hash functions for HMAC (and probably one
legacy approved hash function, MD5), so that's just the md5, sha1, sha256 and
sha512 packages being included... Excluding SHA-512 but including SHA-256 seems
odd to me, so I'd vouch for taking out any hash functions, or just including the
approved ones (but all of them). That would still be a gofix-able situation I
think, but I'm not sure because I can't write a gofix rule (yet?).

-Luit

[Luit, 2012-01-18 09:40:40]

Hello golang-dev@googlegroups.com, agl@golang.org, r@google.com (cc:
golang-dev@googlegroups.com, rsc@golang.org),

Please take another look.

[Luit, 2012-01-18 09:41:45]

Hello golang-dev@googlegroups.com, agl@golang.org, r@google.com (cc:
golang-dev@googlegroups.com, rsc@golang.org),

Please take another look.

[Luit, 2012-01-18 09:43:13]

On 2012/01/18 09:41:45, Luit wrote:
> Hello mailto:golang-dev@googlegroups.com, mailto:agl@golang.org,
mailto:r@google.com (cc:
> mailto:golang-dev@googlegroups.com, mailto:rsc@golang.org),
> 
> Please take another look.

My apologies for the mail volume, I am doing something wrong; it's not including
the new changes I made to files that are not in the original patch. Trying
again.

-Luit

[Luit, 2012-01-18 09:44:30]

Hello golang-dev@googlegroups.com, agl@golang.org, r@google.com (cc:
golang-dev@googlegroups.com, rsc@golang.org),

Please take another look.

[Luit, 2012-01-18 09:50:15]

Hello golang-dev@googlegroups.com, agl@golang.org, r@google.com (cc:
golang-dev@googlegroups.com, rsc@golang.org),

Please take another look.

[Luit, 2012-01-18 10:00:04]

Hello golang-dev@googlegroups.com, agl@golang.org, r@google.com (cc:
golang-dev@googlegroups.com, rsc@golang.org),

Please take another look.

[Luit, 2012-01-18 10:11:16]

I'm done now. I'm sorry if the volume of messages in this thread bothers anyone.


Russ, I went ahead and did just that: hash.Hash now has `BlockSize() int` and
everything broken in std by that change now seems to work again. I don't think
this change in API (like for exp/ssh) can be gofix-able. Then again, I'm not
sure exactly what gofix is capable of.

As for the NewSHA(224|384|512) functions, I'm not sure what is best. Maybe even
RIPEMD should be included. If it's an easy thing for gofix to do though, I'd
rather see them all go, and just have hmac.New(h func() hash.Hash, key) in
there. 

IMHO hmac.New(sha1.New, key) is nicer than hmac.NewSHA1(key). It'll also avoid
importing md5 and sha256 (and sha512 now) for nothing. I think it's a good thing
to just scratch them all. We do need a gofix then. Any other thoughts about
that?

-Luit

[rsc, 2012-01-18 14:11:59]

I think the changes look fine, except that I do not want
to expand the crypto/hmac API, as I explained in my mail
earlier.

Thanks.
Russ

http://codereview.appspot.com/5550043/diff/11002/src/pkg/crypto/hmac/hmac.go
File src/pkg/crypto/hmac/hmac.go (right):

http://codereview.appspot.com/5550043/diff/11002/src/pkg/crypto/hmac/hmac.go#...
src/pkg/crypto/hmac/hmac.go:15: "crypto/sha512"
Please delete.  I do not want to add more imports here.

http://codereview.appspot.com/5550043/diff/11002/src/pkg/crypto/hmac/hmac.go#...
src/pkg/crypto/hmac/hmac.go:92: // NewSHA224 returns a new HMAC-SHA224 hash
using the given key.
Delete.

http://codereview.appspot.com/5550043/diff/11002/src/pkg/crypto/hmac/hmac.go#...
src/pkg/crypto/hmac/hmac.go:98: // NewSHA384 returns a new HMAC-SHA384 hash
using the given key.
Delete.

http://codereview.appspot.com/5550043/diff/11002/src/pkg/crypto/hmac/hmac.go#...
src/pkg/crypto/hmac/hmac.go:101: // NewSHA512 returns a new HMAC-SHA512 hash
using the given key.
Delete.

[Luit, 2012-01-18 14:20:16]

On 2012/01/18 14:11:59, rsc wrote:
> I think the changes look fine, except that I do not want
> to expand the crypto/hmac API, as I explained in my mail
> earlier.

Agreed. Still, IMHO NewSHA256 then also doesn't deserve the special position...
And what's the real benefit of having hmac.NewSHA1(key) instead of
hmac.New(sha1.New, key)? Removing NewMD5, NewSHA1 and NewSHA256 would be
something of which gofix could easily avert breaking stuff, and it's much
cleaner (I don't need MD5, SHA1 and SHA256 linked into my program when only
using HMAC-SHA-512).

Fair?

-Luit

[agl1, 2012-01-18 14:21:25]

On Wed, Jan 18, 2012 at 9:12 AM,  <rsc@golang.org> wrote:
> I think the changes look fine, except that I do not want
> to expand the crypto/hmac API, as I explained in my mail
> earlier.

I would prefer if we added a crypto.BlockHash interface with the added
BlockSize() rather than adding to hash.Hash. The crypto hash functions
can return crypto.BlockHash, which would be compatible with hash.Hash
where needed. BlockSize is just a really obscure method for hash.Hash
to have.

(I'll do the work rather than messing luitvd around.)

rsc: if you really prefer adding to hash.Hash, I wont fight it.


Cheers

AGL

[Luit, 2012-01-18 14:28:58]

Don't really mind that as much, AGL, as I learn with every step in this.

Still, I started off trying to implement it as crypto.BlockHash interface (as I
proposed in this thread earlier), though trying this made me realize Russ's
proposal seemed a cleaner fix to me. Still, I agree it's between you and rsc to
decide I guess.

-Luit

[rsc, 2012-01-18 14:34:56]

On Wed, Jan 18, 2012 at 09:21, Adam Langley <agl@golang.org> wrote:
> I would prefer if we added a crypto.BlockHash interface with the added
> BlockSize() rather than adding to hash.Hash. The crypto hash functions
> can return crypto.BlockHash, which would be compatible with hash.Hash
> where needed. BlockSize is just a really obscure method for hash.Hash
> to have.
>
> (I'll do the work rather than messing luitvd around.)
>
> rsc: if you really prefer adding to hash.Hash, I wont fight it.

I really do prefer it.  At this point we are trying to minimize API churn,
and splitting the concept of hash.Hash into two distinct concepts would
cause significantly more churn than adding this new method does.
Everyone writing code would have to decide whether they want to use
a hash.Hash or a crypto.BlockHash, and the effect will ripple through
all the client code.

Like with time.Time, I think the benefits of having a single concept
(just hash.Hash, not separately hash.Hash and crypto.BlockHash)
outweigh the inconvenience of defining BlockSize on CRC-32.

Russ

[rsc, 2012-01-18 14:38:51]

On Wed, Jan 18, 2012 at 09:20,  <luitvd@gmail.com> wrote:
> Agreed. Still, IMHO NewSHA256 then also doesn't deserve the special
> position... And what's the real benefit of having hmac.NewSHA1(key)
> instead of hmac.New(sha1.New, key)? Removing NewMD5, NewSHA1 and
> NewSHA256 would be something of which gofix could easily avert breaking
> stuff, and it's much cleaner (I don't need MD5, SHA1 and SHA256 linked
> into my program when only using HMAC-SHA-512).

I agree that it is cleaner, but I don't want to change the existing API.
We've gotten to the point where we are trying to minimize the
additional pain inflicted on client code, and I expect that NewSHA1
and NewMD5 are commonly used.  We could probably drop NewSHA256
but at this point I'd rather just leave it in.  The win is not big enough to
justify the churn.

Russ

[Luit, 2012-01-18 14:53:42]

Because I can't write one myself I'm not sure, but I think having gofix add a
simple rewrite takes out close to all the pain dropping NewSHA1 and NewMD5 could
inflict. 

Ofcourse, it's up to you (not just rsc specifically, just the Go core team I
mean)

What do you (again, Go core team) want me to do?

-Luit

[Luit, 2012-01-18 14:54:38]

I realise it's close to Go 1, but after Go 1 it'll be impossible. Just my 2
cents.

-Luit

[rsc, 2012-01-18 14:56:58]

On Wed, Jan 18, 2012 at 09:53,  <luitvd@gmail.com> wrote:
> Because I can't write one myself I'm not sure, but I think having gofix
> add a simple rewrite takes out close to all the pain dropping NewSHA1
> and NewMD5 could inflict.
>
> Of course, it's up to you (not just rsc specifically, just the Go core
> team I mean)
>
> What do you (again, Go core team) want me to do?

Please delete the additional import and helpers from crypto/hmac
in this CL.  Then this CL is only about making hmac work with
larger hashes (a certain improvement), and not about changing
the API (a less certain one).

Russ

[Luit, 2012-01-18 14:59:27]

On 2012/01/18 14:56:58, rsc wrote:
> Please delete the additional import and helpers from crypto/hmac
> in this CL.  Then this CL is only about making hmac work with
> larger hashes (a certain improvement), and not about changing
> the API (a less certain one).

Will do. What about SHA256? And if that one is left in, SHA224? That one is
practically the same thing...

Also, I'll rewrite the HMAC test cases to still enable testing bigger hashes. 

-Luit

[rsc, 2012-01-18 15:00:11]

On Wed, Jan 18, 2012 at 09:59,  <luitvd@gmail.com> wrote:
> Will do. What about SHA256? And if that one is left in, SHA224? That one
> is practically the same thing...

Let's leave the API alone for now.

> Also, I'll rewrite the HMAC test cases to still enable testing bigger
> hashes.

Sounds good.

Thanks.
Russ

[Luit, 2012-01-18 15:12:24]

Hello agl@golang.org, r@google.com, rsc@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[Luit, 2012-01-18 15:21:28]

I think this is it then. Who should be on CC for the future issue about possible
dropping of NewMD5, NewSHA1 and NewSHA256?

-Luit

[rsc, 2012-01-18 15:21:57]

On Wed, Jan 18, 2012 at 10:21,  <luitvd@gmail.com> wrote:
> I think this is it then. Who should be on CC for the future issue about
> possible dropping of NewMD5, NewSHA1 and NewSHA256?

You can just send it to golang-dev, but I think it is unlikely.

Russ

[r, 2012-01-18 15:23:23]

again, API change needs to be mentioned in go 1 release notes

[rsc, 2012-01-18 15:27:39]

LGTM

I will send the release note change separately.

[Luit, 2012-01-18 15:32:24]

On 2012/01/18 15:27:39, rsc wrote:
> LGTM
> 
> I will send the release note change separately.

Thanks. On a related note: is there a quicker way to build and test the std
library on changes like this? $GOROOT/src/all.bash takes an immensely long
time...

[r2, 2012-01-18 15:35:17]

On Jan 18, 2012, at 7:32 AM, luitvd@gmail.com wrote:

> On 2012/01/18 15:27:39, rsc wrote:
>> LGTM
> 
>> I will send the release note change separately.
> 
> Thanks. On a related note: is there a quicker way to build and test the
> std library on changes like this? $GOROOT/src/all.bash takes an
> immensely long time...

It takes a two or three minutes for me. "Immensely long" times for software
build in my experience implies times measured in hours or even days (sic). So,
does all.bash take you hours? If so, please explain.

-rob

[rsc, 2012-01-18 15:35:45]

On Wed, Jan 18, 2012 at 10:32,  <luitvd@gmail.com> wrote:
> Thanks. On a related note: is there a quicker way to build and test the
> std library on changes like this? $GOROOT/src/all.bash takes an
> immensely long time...

You can run 'go test std' to repeat the tests if all
you've changed is Go code (and not the compilers, etc).
However, running all.bash once in a while is always
a good idea.

I am pleased that we have created a system where
two minutes feels like an immensely long time.

Russ

[rsc, 2012-01-18 15:36:32]

*** Submitted as http://code.google.com/p/go/source/detail?r=728321e6dbea ***

crypto/hmac: Add HMAC-SHA224 and HMAC-SHA384/512

First was, apart from adding tests, a single line of code (to add the
constructor function). Adding SHA512-based hashing to crypto/hmac
required minor rework of the package because of a previously hardcoded
block-size in it's implementation. Instead of using a hash.Hash
generator function the constructor function now uses a crypto.Hash
type, which was extended to expose information about block size.

The only standard library package impacted by the change is
crypto/tls, for which the fix is included in this patch. It might be
useful to extend gofix to include this API change too.

R=agl, r, rsc, r
CC=golang-dev
http://codereview.appspot.com/5550043

Committer: Russ Cox <rsc@golang.org>

[Luit, 2012-01-18 15:45:27]

On 2012/01/18 15:36:32, rsc wrote:
> *** Submitted as http://code.google.com/p/go/source/detail?r=728321e6dbea ***
> 
> crypto/hmac: Add HMAC-SHA224 and HMAC-SHA384/512
> 
> First was, apart from adding tests, a single line of code (to add the
> constructor function). Adding SHA512-based hashing to crypto/hmac
> required minor rework of the package because of a previously hardcoded
> block-size in it's implementation. Instead of using a hash.Hash
> generator function the constructor function now uses a crypto.Hash
> type, which was extended to expose information about block size.
> 
> The only standard library package impacted by the change is
> crypto/tls, for which the fix is included in this patch. It might be
> useful to extend gofix to include this API change too.
> 
> R=agl, r, rsc, r
> CC=golang-dev
> http://codereview.appspot.com/5550043
> 
> Committer: Russ Cox <mailto:rsc@golang.org>

Shoot, the original commit message doesn't really apply anymore... can't go back
now? ... it's a bit confusing at best, but IMHO just plain wrong now... Didn't
look at it after the first patch-set commit...

-Luit

[Luit, 2012-01-18 19:04:03]

On 2012/01/18 15:35:17, r2 wrote:
> It takes a two or three minutes for me. "Immensely long" times for software
> build in my experience implies times measured in hours or even days (sic). So,
> does all.bash take you hours? If so, please explain.
> 
> -rob

Yeah, well, Go pretty much is my point of reference for compile time I guess, so
I was indeed referring to the 2 to 3 minutes all.bash takes to run the full
build. I think I remember this took much less time a year ago. 

"Immensely long" was a bit of an overstatement, but it's pretty useless to
re-build the whole compiler suite when I'm only hacking on the standard lib. I
just wanted a make install && gotest for each package in standard lib, so I can
run the full suite only for the final check before sending in a patch-set. Don't
need a clean set of compilers for every test case I add or modify. 

Also, this is a quote I'm going to remember: 

"I am pleased that we have created a system where two minutes feels like an
immensely long time." - rsc

That feat is part of why I enjoy trying to help out this language. I hope I'm
doing well. Thanks guys!

-Luit

[r2, 2012-01-18 19:13:53]

On Jan 18, 2012, at 11:04 AM, luitvd@gmail.com wrote:

> On 2012/01/18 15:35:17, r2 wrote:
>> It takes a two or three minutes for me. "Immensely long" times for
> software
>> build in my experience implies times measured in hours or even days
> (sic). So,
>> does all.bash take you hours? If so, please explain.
> 
>> -rob
> 
> Yeah, well, Go pretty much is my point of reference for compile time I
> guess, so I was indeed referring to the 2 to 3 minutes all.bash takes to
> run the full build. I think I remember this took much less time a year
> ago.

I won't argue with you, but I don't believe it's that much different. The build
has gotten bigger but there's been a lot of work to reduce testing times as a
compensation. Still, I agree that all.bash takes longer than we'd like.

-rob

[Luit, 2012-01-18 19:44:49]

On 2012/01/18 19:13:53, r2 wrote:
> I won't argue with you, but I don't believe it's that much different. The
build
> has gotten bigger but there's been a lot of work to reduce testing times as a
> compensation. Still, I agree that all.bash takes longer than we'd like.

Oh, please correct me if I'm wrong, I can handle it. To be fair, I can't imagine
you making an invalid point on anything related to this.

I guess it's just my perception of the speed settling from "damn it's fast!"
down to a more modest "it's fast!", making it feel like a slow-down, even though
it could very well be the opposite. 

I think I should choose my words more carefully.

Cheers,
-Luit

[rsc, 2012-01-18 19:49:15]

all.bash does important things for the developers,
so the 2 minutes (or something like that) is probably
reasonable there.  However, you are right that all.bash
is too slow for ordinary Go users who just want to write
a Go program.  For that use case we will have binary
downloads before long.

Russ