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

[lui...@gmail.com]

Reviewers: golang-dev_googlegroups.com,

Message:
Hello golan...@googlegroups.com (cc: golan...@googlegroups.com),

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

Description:
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.

Please review this at http://codereview.appspot.com/5550043/

Affected files:
M src/pkg/crypto/crypto.go
M src/pkg/crypto/hmac/hmac.go
M src/pkg/crypto/hmac/hmac_test.go
M src/pkg/crypto/tls/prf.go

[lui...@gmail.com]

Now with CC: rsc, agl1

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

http://codereview.appspot.com/5550043/

[Russ Cox]

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

[lui...@gmail.com]

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

http://codereview.appspot.com/5550043/

[a...@golang.org]

> 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#newcode64
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#newcode72
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.

http://codereview.appspot.com/5550043/

[lui...@gmail.com]

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#newcode72
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

http://codereview.appspot.com/5550043/

[lui...@gmail.com]

Hello golan...@googlegroups.com, a...@golang.org (cc:
golan...@googlegroups.com, r...@golang.org),

Please take another look.

http://codereview.appspot.com/5550043/

[lui...@gmail.com]

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#newcode64
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.

http://codereview.appspot.com/5550043/

[a...@golang.org]

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.

http://codereview.appspot.com/5550043/

[lui...@gmail.com]

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

http://codereview.appspot.com/5550043/

[Adam Langley]

On Tue, Jan 17, 2012 at 6:23 PM, <lui...@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

[Rob 'Commander' Pike]

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

-rob

[lui...@gmail.com]

On 2012/01/17 23:25:41, agl1 wrote:

> On Tue, Jan 17, 2012 at 6:23 PM, <mailto:lui...@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

http://codereview.appspot.com/5550043/

[lui...@gmail.com]

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.

http://codereview.appspot.com/5550043/

[Russ Cox]

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

[lui...@gmail.com]

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

http://codereview.appspot.com/5550043/

[lui...@gmail.com]

Hello golan...@googlegroups.com, a...@golang.org, r...@google.com (cc:

[lui...@gmail.com]

[lui...@gmail.com]

On 2012/01/18 09:41:45, Luit wrote:
> Hello mailto:golan...@googlegroups.com, mailto:a...@golang.org,
mailto:r...@google.com (cc:
> mailto:golan...@googlegroups.com, mailto:r...@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

http://codereview.appspot.com/5550043/

[lui...@gmail.com]

Hello golan...@googlegroups.com, a...@golang.org, r...@google.com (cc:

golan...@googlegroups.com, r...@golang.org),

Please take another look.

http://codereview.appspot.com/5550043/

[lui...@gmail.com]

[lui...@gmail.com]

[lui...@gmail.com]

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

http://codereview.appspot.com/5550043/

[r...@golang.org]

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#newcode15
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#newcode92
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#newcode98
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#newcode101
src/pkg/crypto/hmac/hmac.go:101: // NewSHA512 returns a new HMAC-SHA512
hash using the given key.
Delete.

http://codereview.appspot.com/5550043/

[lui...@gmail.com]

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

http://codereview.appspot.com/5550043/

[Adam Langley]

On Wed, Jan 18, 2012 at 9:12 AM, <r...@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

[lui...@gmail.com]

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

http://codereview.appspot.com/5550043/

[Russ Cox]

On Wed, Jan 18, 2012 at 09:21, Adam Langley <a...@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

[Russ Cox]

On Wed, Jan 18, 2012 at 09:20, <lui...@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

[lui...@gmail.com]

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

http://codereview.appspot.com/5550043/

[lui...@gmail.com]

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

-Luit

http://codereview.appspot.com/5550043/

[Russ Cox]

On Wed, Jan 18, 2012 at 09:53, <lui...@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

[lui...@gmail.com]

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

http://codereview.appspot.com/5550043/

[Russ Cox]

On Wed, Jan 18, 2012 at 09:59, <lui...@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

[lui...@gmail.com]

Hello a...@golang.org, r...@google.com, r...@golang.org (cc:
golan...@googlegroups.com),

[lui...@gmail.com]

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

-Luit

http://codereview.appspot.com/5550043/

[Russ Cox]

On Wed, Jan 18, 2012 at 10:21, <lui...@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...@golang.org]

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

http://codereview.appspot.com/5550043/

[Russ Cox]

LGTM

I will send the release note change separately.

[lui...@gmail.com]

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...

http://codereview.appspot.com/5550043/

[Rob 'Commander' Pike]

On Jan 18, 2012, at 7:32 AM, lui...@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

[Russ Cox]

On Wed, Jan 18, 2012 at 10:32, <lui...@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

[r...@golang.org]

*** 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 <r...@golang.org>

http://codereview.appspot.com/5550043/

[lui...@gmail.com]

> Committer: Russ Cox <mailto:r...@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

http://codereview.appspot.com/5550043/

[lui...@gmail.com]

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

http://codereview.appspot.com/5550043/

[Rob 'Commander' Pike]

On Jan 18, 2012, at 11:04 AM, lui...@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

[lui...@gmail.com]

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

http://codereview.appspot.com/5550043/

[Russ Cox]

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