code review 4964078: crypto/bcrypt: new package

crypto/bcrypt: new package

A port of Provos and Mazières's adapative hashing algorithm. See http://www.usenix.org/events/usenix99/provos/provos_html/node1.html

[jmhodges, 2011-09-12 09:12:37]

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

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

[jmhodges, 2011-09-12 09:24:56]

I also have a nice performance improvement for blowfish.encryptBlock and
decryptBlock, but I'd like to let the dust settle around it's public API before
I dig into its private functions.

[agl1, 2011-09-12 14:24:03]

I'm ok with the interface changes to crypto/blowfish.

http://codereview.appspot.com/4964078/diff/2001/src/pkg/crypto/bcrypt/Makefile
File src/pkg/crypto/bcrypt/Makefile (right):

http://codereview.appspot.com/4964078/diff/2001/src/pkg/crypto/bcrypt/Makefil...
src/pkg/crypto/bcrypt/Makefile:1: # Copyright 2010 The Go Authors. All rights
reserved.
2011

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

http://codereview.appspot.com/4964078/diff/2001/src/pkg/crypto/bcrypt/base64....
src/pkg/crypto/bcrypt/base64.go:1: package bcrypt
Needs copyright header.

http://codereview.appspot.com/4964078/diff/2001/src/pkg/crypto/bcrypt/base64....
src/pkg/crypto/bcrypt/base64.go:11: func cleanBase64Encode(src []byte) []byte {
this seems like a verbose function name for an internal function.

http://codereview.appspot.com/4964078/diff/2001/src/pkg/crypto/bcrypt/base64....
src/pkg/crypto/bcrypt/base64.go:16: n -= 1
n--

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

http://codereview.appspot.com/4964078/diff/2001/src/pkg/crypto/bcrypt/bcrypt....
src/pkg/crypto/bcrypt/bcrypt.go:1: // Copyright 2010 The Go Authors. All rights
reserved.
2011

http://codereview.appspot.com/4964078/diff/2001/src/pkg/crypto/bcrypt/bcrypt....
src/pkg/crypto/bcrypt/bcrypt.go:6: // algorithm.
include the URL?

http://codereview.appspot.com/4964078/diff/2001/src/pkg/crypto/bcrypt/bcrypt....
src/pkg/crypto/bcrypt/bcrypt.go:68: func Equal(hashedPassword, password []byte)
bool {
I think that the name of this function should try to minimise the chance that
someone passes the arguments in the wrong order.

CompareHashedAndPlain?

Also, why not just return an os.Error and thus nil is success? Then EqualVerbose
can go away.

http://codereview.appspot.com/4964078/diff/2001/src/pkg/crypto/bcrypt/bcrypt....
src/pkg/crypto/bcrypt/bcrypt.go:108: for n < maxSaltSize {
io.ReadFull

http://codereview.appspot.com/4964078/diff/2001/src/pkg/crypto/bcrypt/bcrypt....
src/pkg/crypto/bcrypt/bcrypt.go:133: n, err = decodeCost(p,
hashedSecret[offset:])
rather than keeping an offset, it's more common to reslice:

hashedSecret = hashedSecret[n:]

http://codereview.appspot.com/4964078/diff/2001/src/pkg/crypto/bcrypt/bcrypt....
src/pkg/crypto/bcrypt/bcrypt.go:209: copy(arr[n:], []byte(fmt.Sprintf("%0d",
p.cost)))
see package strconv rather than Sprintf

http://codereview.appspot.com/4964078/diff/2001/src/pkg/crypto/bcrypt/bcrypt....
src/pkg/crypto/bcrypt/bcrypt.go:224: func decodeVersion(p *hashed, sbytes
[]byte) (int, os.Error) {
why not make this a method on hashed?

http://codereview.appspot.com/4964078/diff/2001/src/pkg/crypto/bcrypt/bcrypt....
src/pkg/crypto/bcrypt/bcrypt.go:226: return -1, os.NewError(fmt.Sprintf("invalid
salt: %s", sbytes))
You can concat strings with + rather can Sprintf

http://codereview.appspot.com/4964078/diff/2001/src/pkg/crypto/bcrypt/bcrypt....
src/pkg/crypto/bcrypt/bcrypt.go:241: func decodeCost(p *hashed, sbytes []byte)
(int, os.Error) {
why not make this a method on hashed?

http://codereview.appspot.com/4964078/diff/2001/src/pkg/crypto/bcrypt/bcrypt_...
File src/pkg/crypto/bcrypt/bcrypt_test.go (right):

http://codereview.appspot.com/4964078/diff/2001/src/pkg/crypto/bcrypt/bcrypt_...
src/pkg/crypto/bcrypt/bcrypt_test.go:1: // Copyright 2010 The Go Authors. All
rights reserved.
2011

[rsc, 2011-09-12 16:46:47]

Please split the blowfish changes into a separate CL.  Thanks.

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

http://codereview.appspot.com/4964078/diff/2001/src/pkg/crypto/blowfish/block...
src/pkg/crypto/blowfish/block.go:8: // state of it. It's unlikely that you need
this. Example usage can be
If it's worth exporting, it's worth documenting.
If it's not worth documenting, it's not worth exporting.

Please rewrite this comment to say what ExpandZeroKey does.
"changes the state" is not terribly specific, and the "unlikely that
you need this" is similarly unhelpful to a prospective caller.
Why is it called ExpandZeroKey and not ExpandKey, for
example?  If it's a zero key, why do I need to pass a key?
And so on.

[jmhodges, 2011-09-14 02:39:57]

On 2011/09/12 16:46:47, rsc wrote:
> Please split the blowfish changes into a separate CL.  Thanks.
> 

Here we go: http://codereview.appspot.com/5013043

[jmhodges, 2011-09-14 06:55:30]

On 2011/09/12 14:24:03, agl1 wrote:
> I'm ok with the interface changes to crypto/blowfish.
> 
> http://codereview.appspot.com/4964078/diff/2001/src/pkg/crypto/bcrypt/Makefile
> File src/pkg/crypto/bcrypt/Makefile (right):
> 
>
http://codereview.appspot.com/4964078/diff/2001/src/pkg/crypto/bcrypt/Makefil...
> src/pkg/crypto/bcrypt/Makefile:1: # Copyright 2010 The Go Authors. All rights
> reserved.
> 2011
> 

Done.

>
http://codereview.appspot.com/4964078/diff/2001/src/pkg/crypto/bcrypt/base64.go
> File src/pkg/crypto/bcrypt/base64.go (right):
> 
>
http://codereview.appspot.com/4964078/diff/2001/src/pkg/crypto/bcrypt/base64....
> src/pkg/crypto/bcrypt/base64.go:1: package bcrypt
> Needs copyright header.
> 

Done.

>
http://codereview.appspot.com/4964078/diff/2001/src/pkg/crypto/bcrypt/base64....
> src/pkg/crypto/bcrypt/base64.go:11: func cleanBase64Encode(src []byte) []byte
{
> this seems like a verbose function name for an internal function.
> 

Changed to base64Encode and base64Decode.

>
http://codereview.appspot.com/4964078/diff/2001/src/pkg/crypto/bcrypt/base64....
> src/pkg/crypto/bcrypt/base64.go:16: n -= 1
> n--
> 

Done.

>
http://codereview.appspot.com/4964078/diff/2001/src/pkg/crypto/bcrypt/bcrypt.go
> File src/pkg/crypto/bcrypt/bcrypt.go (right):
> 
>
http://codereview.appspot.com/4964078/diff/2001/src/pkg/crypto/bcrypt/bcrypt....
> src/pkg/crypto/bcrypt/bcrypt.go:1: // Copyright 2010 The Go Authors. All
rights
> reserved.
> 2011
> 

Done.

>
http://codereview.appspot.com/4964078/diff/2001/src/pkg/crypto/bcrypt/bcrypt....
> src/pkg/crypto/bcrypt/bcrypt.go:6: // algorithm.
> include the URL?
> 

Done.

>
http://codereview.appspot.com/4964078/diff/2001/src/pkg/crypto/bcrypt/bcrypt....
> src/pkg/crypto/bcrypt/bcrypt.go:68: func Equal(hashedPassword, password
[]byte)
> bool {
> I think that the name of this function should try to minimise the chance that
> someone passes the arguments in the wrong order.
> 
> CompareHashedAndPlain?
> 

Went with CompareHashAndPassword. A little verbose, maybe.

> Also, why not just return an os.Error and thus nil is success? Then
EqualVerbose
> can go away.
> 

Done. Not super keen on this, but I see the precedence.

>
http://codereview.appspot.com/4964078/diff/2001/src/pkg/crypto/bcrypt/bcrypt....
> src/pkg/crypto/bcrypt/bcrypt.go:108: for n < maxSaltSize {
> io.ReadFull
> 

Oh, right. Done. Better.

>
http://codereview.appspot.com/4964078/diff/2001/src/pkg/crypto/bcrypt/bcrypt....
> src/pkg/crypto/bcrypt/bcrypt.go:133: n, err = decodeCost(p,
> hashedSecret[offset:])
> rather than keeping an offset, it's more common to reslice:
> 
> hashedSecret = hashedSecret[n:]
> 

Done.

>
http://codereview.appspot.com/4964078/diff/2001/src/pkg/crypto/bcrypt/bcrypt....
> src/pkg/crypto/bcrypt/bcrypt.go:209: copy(arr[n:], []byte(fmt.Sprintf("%0d",
> p.cost)))
> see package strconv rather than Sprintf
> 

strconv, unfortunately, doesn't allow for leading zeros like Sprintf which is
required for compatibility. Adding a test for that. Sprintf does generate more
garbage than I'd like in this path, but it's currently overwhelmed by the
blowfish garbage. Once we fix that, I'll see if we need to do the zero work here
instead of using Sprintf.

>
http://codereview.appspot.com/4964078/diff/2001/src/pkg/crypto/bcrypt/bcrypt....
> src/pkg/crypto/bcrypt/bcrypt.go:224: func decodeVersion(p *hashed, sbytes
> []byte) (int, os.Error) {
> why not make this a method on hashed?
> 

Done.

>
http://codereview.appspot.com/4964078/diff/2001/src/pkg/crypto/bcrypt/bcrypt....
> src/pkg/crypto/bcrypt/bcrypt.go:226: return -1,
os.NewError(fmt.Sprintf("invalid
> salt: %s", sbytes))
> You can concat strings with + rather can Sprintf
> 

Done.

>
http://codereview.appspot.com/4964078/diff/2001/src/pkg/crypto/bcrypt/bcrypt....
> src/pkg/crypto/bcrypt/bcrypt.go:241: func decodeCost(p *hashed, sbytes []byte)
> (int, os.Error) {
> why not make this a method on hashed?
> 

Done.

>
http://codereview.appspot.com/4964078/diff/2001/src/pkg/crypto/bcrypt/bcrypt_...
> File src/pkg/crypto/bcrypt/bcrypt_test.go (right):
> 
>
http://codereview.appspot.com/4964078/diff/2001/src/pkg/crypto/bcrypt/bcrypt_...
> src/pkg/crypto/bcrypt/bcrypt_test.go:1: // Copyright 2010 The Go Authors. All
> rights reserved.
> 2011

Done.

[jmhodges, 2011-09-14 08:39:38]

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

Please take another look.

[jmhodges, 2011-09-16 01:19:40]

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

Please take another look.

[agl1, 2011-09-16 20:43:47]

I think this is basically there minus nits. I'll land the next revision unless
anyone else objects.

http://codereview.appspot.com/4964078/diff/19001/src/pkg/crypto/bcrypt/base64.go
File src/pkg/crypto/bcrypt/base64.go (right):

http://codereview.appspot.com/4964078/diff/19001/src/pkg/crypto/bcrypt/base64...
src/pkg/crypto/bcrypt/base64.go:12: var alphabet =
"./ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789"
s/var/const/

http://codereview.appspot.com/4964078/diff/19001/src/pkg/crypto/bcrypt/bcrypt.go
File src/pkg/crypto/bcrypt/bcrypt.go (right):

http://codereview.appspot.com/4964078/diff/19001/src/pkg/crypto/bcrypt/bcrypt...
src/pkg/crypto/bcrypt/bcrypt.go:28: var MismatchedHashAndPassword =
os.NewError("crypto:bcrypt: hashedPassword is not the hash of the given
password")
The convention seems to be that errors have a name that ends in Error

http://codereview.appspot.com/4964078/diff/19001/src/pkg/crypto/bcrypt/bcrypt...
src/pkg/crypto/bcrypt/bcrypt.go:28: var MismatchedHashAndPassword =
os.NewError("crypto:bcrypt: hashedPassword is not the hash of the given
password")
s!crypto:!crypto/!

http://codereview.appspot.com/4964078/diff/19001/src/pkg/crypto/bcrypt/bcrypt...
src/pkg/crypto/bcrypt/bcrypt.go:85: // MinCost, instead. Use Equal, as defined
in this package, to compare the
Equal has been renamed, right?

[jmhodges, 2011-09-16 21:14:22]

On 2011/09/16 20:43:47, agl1 wrote:
> I think this is basically there minus nits. I'll land the next revision unless
> anyone else objects.
> 
>
http://codereview.appspot.com/4964078/diff/19001/src/pkg/crypto/bcrypt/base64.go
> File src/pkg/crypto/bcrypt/base64.go (right):
> 
>
http://codereview.appspot.com/4964078/diff/19001/src/pkg/crypto/bcrypt/base64...
> src/pkg/crypto/bcrypt/base64.go:12: var alphabet =
> "./ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789"
> s/var/const/
> 
Done.

>
http://codereview.appspot.com/4964078/diff/19001/src/pkg/crypto/bcrypt/bcrypt.go
> File src/pkg/crypto/bcrypt/bcrypt.go (right):
> 
>
http://codereview.appspot.com/4964078/diff/19001/src/pkg/crypto/bcrypt/bcrypt...
> src/pkg/crypto/bcrypt/bcrypt.go:28: var MismatchedHashAndPassword =
> os.NewError("crypto:bcrypt: hashedPassword is not the hash of the given
> password")
> The convention seems to be that errors have a name that ends in Error
> 

Done.

>
http://codereview.appspot.com/4964078/diff/19001/src/pkg/crypto/bcrypt/bcrypt...
> src/pkg/crypto/bcrypt/bcrypt.go:28: var MismatchedHashAndPassword =
> os.NewError("crypto:bcrypt: hashedPassword is not the hash of the given
> password")
> s!crypto:!crypto/!
> 

Done.

>
http://codereview.appspot.com/4964078/diff/19001/src/pkg/crypto/bcrypt/bcrypt...
> src/pkg/crypto/bcrypt/bcrypt.go:85: // MinCost, instead. Use Equal, as defined
> in this package, to compare the
> Equal has been renamed, right?

Yep! Done.

[jmhodges, 2011-09-16 21:15:45]

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

Please take another look.

[dchest, 2011-09-16 23:27:52]

http://codereview.appspot.com/4964078/diff/26001/src/pkg/crypto/bcrypt/bcrypt.go
File src/pkg/crypto/bcrypt/bcrypt.go (right):

http://codereview.appspot.com/4964078/diff/26001/src/pkg/crypto/bcrypt/bcrypt...
src/pkg/crypto/bcrypt/bcrypt.go:132: io.ReadFull(rand.Reader, unencodedSalt)
Missing error check here.

[dchest, 2011-09-16 23:53:44]

http://codereview.appspot.com/4964078/diff/26001/src/pkg/crypto/bcrypt/bcrypt.go
File src/pkg/crypto/bcrypt/bcrypt.go (right):

http://codereview.appspot.com/4964078/diff/26001/src/pkg/crypto/bcrypt/bcrypt...
src/pkg/crypto/bcrypt/bcrypt.go:145: n, err := p.decodeVersion(hashedSecret)
Before doing p.decodeVersion/p.decodeCost shouldn't there be a check for length
(with return nil, HashTooShortError) as later in this function? I think both
methods will panic if hashedSecret is too short.

[jmhodges, 2011-09-17 00:50:49]

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

Please take another look.

[jmhodges, 2011-09-17 00:53:32]

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

Please take another look.

[jmhodges, 2011-09-17 00:53:44]

On 2011/09/16 23:53:44, dchest wrote:
>
http://codereview.appspot.com/4964078/diff/26001/src/pkg/crypto/bcrypt/bcrypt.go
> File src/pkg/crypto/bcrypt/bcrypt.go (right):
> 
>
http://codereview.appspot.com/4964078/diff/26001/src/pkg/crypto/bcrypt/bcrypt...
> src/pkg/crypto/bcrypt/bcrypt.go:145: n, err := p.decodeVersion(hashedSecret)
> Before doing p.decodeVersion/p.decodeCost shouldn't there be a check for
length
> (with return nil, HashTooShortError) as later in this function? I think both
> methods will panic if hashedSecret is too short.

Done.

[jmhodges, 2011-09-17 00:53:58]

On 2011/09/16 23:27:52, dchest wrote:
>
http://codereview.appspot.com/4964078/diff/26001/src/pkg/crypto/bcrypt/bcrypt.go
> File src/pkg/crypto/bcrypt/bcrypt.go (right):
> 
>
http://codereview.appspot.com/4964078/diff/26001/src/pkg/crypto/bcrypt/bcrypt...
> src/pkg/crypto/bcrypt/bcrypt.go:132: io.ReadFull(rand.Reader, unencodedSalt)
> Missing error check here.

Good catch. Done.

[agl1, 2011-09-19 14:29:13]

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

    crypto/bcrypt: new package

A port of Provos and Mazières's adapative hashing algorithm. See
http://www.usenix.org/events/usenix99/provos/provos_html/node1.html

R=bradfitz, agl, rsc, dchest
CC=golang-dev
http://codereview.appspot.com/4964078

Committer: Adam Langley <agl@golang.org>

[agl1, 2011-09-19 14:30:30]

On Mon, Sep 19, 2011 at 10:29 AM,  <agl@golang.org> wrote:
> *** Submitted as
> http://code.google.com/p/go/source/detail?r=d8c1aeda704a ***

I tweaked a couple of things before landing:

I removed "Works" from the end of one of the test names

I made TestCost not run in short mode because it was taking > 3
seconds on my workstation. That would probably have been a problem on
slower machines.


Cheers

AGL

[jmhodges, 2011-09-19 16:46:15]

SGTM.

On Mon, Sep 19, 2011 at 7:30 AM, Adam Langley <agl@golang.org> wrote:
> before landing:
>
> I removed "Works" from the end of one of the test names
>
> I made TestCost not run in short mode because it was taking > 3
> seconds on my workstation. That would probably have been a problem on
> slower machines.
>
>