code review 5013043: crypto/blowfish: exposing the blowfish key schedule

crypto/blowfish: exposing the blowfish key schedule

Mostly useful for the coming crypto/bcrypt package

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

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

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

[agl, 2011-09-14 18:53:37]

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

http://codereview.appspot.com/5013043/diff/4001/src/pkg/crypto/blowfish/block...
src/pkg/crypto/blowfish/block.go:12: func ExpandKey(key []byte, c *Cipher) {
Looks like this should be a method on Cipher?

http://codereview.appspot.com/5013043/diff/4001/src/pkg/crypto/blowfish/ciphe...
File src/pkg/crypto/blowfish/cipher.go (right):

http://codereview.appspot.com/5013043/diff/4001/src/pkg/crypto/blowfish/ciphe...
src/pkg/crypto/blowfish/cipher.go:35: err := initCipher(key, &result)
if err := initCipher(key, &result); err != nil {
  return nil, err
}

http://codereview.appspot.com/5013043/diff/4001/src/pkg/crypto/blowfish/ciphe...
src/pkg/crypto/blowfish/cipher.go:46: func NewSaltedCipher(key []byte, salt
[]byte) (*Cipher, os.Error) {
func NewSaltedCipher(key, salt []byte) (*Cipher, os.Error) {

http://codereview.appspot.com/5013043/diff/4001/src/pkg/crypto/blowfish/ciphe...
src/pkg/crypto/blowfish/cipher.go:48: err := initCipher(key, &result)
ditto with the if

[jmhodges, 2011-09-15 10:37:39]

On 2011/09/14 18:53:37, agl wrote:
>
http://codereview.appspot.com/5013043/diff/4001/src/pkg/crypto/blowfish/block.go
> File src/pkg/crypto/blowfish/block.go (right):
> 
>
http://codereview.appspot.com/5013043/diff/4001/src/pkg/crypto/blowfish/block...
> src/pkg/crypto/blowfish/block.go:12: func ExpandKey(key []byte, c *Cipher) {
> Looks like this should be a method on Cipher?
> 

I thought about it, but I didn't want to clutter up the documentation for Cipher
itself. Is there precedence either way? I'm lacking proper taste, maybe.

>
http://codereview.appspot.com/5013043/diff/4001/src/pkg/crypto/blowfish/ciphe...
> File src/pkg/crypto/blowfish/cipher.go (right):
> 
>
http://codereview.appspot.com/5013043/diff/4001/src/pkg/crypto/blowfish/ciphe...
> src/pkg/crypto/blowfish/cipher.go:35: err := initCipher(key, &result)
> if err := initCipher(key, &result); err != nil {
>   return nil, err
> }
> 

I had to rework this restriction in NewSaltedCipher. It turns out the blowfish
algorithm used in every bcrypt implementation ignores key length completely. I
emailed Schneier about the >56 byte case, and he said it was no big deal to
ignore it. Which is good, because I don't want to be the guy that causes every
Go web app in the world to tell people their passwords are too long. The 4 byte
case is harder for me to drop (and I haven't yet), but I'm thinking seriously
about going with compatibility here.
 
>
http://codereview.appspot.com/5013043/diff/4001/src/pkg/crypto/blowfish/ciphe...
> src/pkg/crypto/blowfish/cipher.go:46: func NewSaltedCipher(key []byte, salt
> []byte) (*Cipher, os.Error) {
> func NewSaltedCipher(key, salt []byte) (*Cipher, os.Error) {
> 
>
http://codereview.appspot.com/5013043/diff/4001/src/pkg/crypto/blowfish/ciphe...
> src/pkg/crypto/blowfish/cipher.go:48: err := initCipher(key, &result)
> ditto with the if

[jmhodges, 2011-09-15 10:51:04]

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

Please take another look.

[rsc, 2011-09-15 15:23:55]

Some documentation nits; otherwise looks good.

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

http://codereview.appspot.com/5013043/diff/8003/src/pkg/crypto/blowfish/block...
src/pkg/crypto/blowfish/block.go:10: // by the crypto/bcrypt package to re-use
the blowfish key schedule during its
s;crypto/;;
s/re-use/reuse/
s/blowfish/Blowfish/g to match rest of doc comments.

[jmhodges, 2011-09-16 00:57:32]

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

Please take another look.

[agl1, 2011-09-16 20:45:27]

LGTM. Will land tomorrow.

http://codereview.appspot.com/5013043/diff/13002/src/pkg/crypto/blowfish/blow...
File src/pkg/crypto/blowfish/blowfish_test.go (right):

http://codereview.appspot.com/5013043/diff/13002/src/pkg/crypto/blowfish/blow...
src/pkg/crypto/blowfish/blowfish_test.go:195: key := []byte{}
var key []byte (I'll fix that up during landing.)

[jmhodges, 2011-09-16 20:51:20]

Thank you.

On Fri, Sep 16, 2011 at 1:45 PM,  <agl@golang.org> wrote:
> LGTM. Will land tomorrow.
>
>
>
http://codereview.appspot.com/5013043/diff/13002/src/pkg/crypto/blowfish/blow...
> File src/pkg/crypto/blowfish/blowfish_test.go (right):
>
>
http://codereview.appspot.com/5013043/diff/13002/src/pkg/crypto/blowfish/blow...
> src/pkg/crypto/blowfish/blowfish_test.go:195: key := []byte{}
> var key []byte (I'll fix that up during landing.)
>
> http://codereview.appspot.com/5013043/
>

[agl1, 2011-09-19 14:21:48]

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

    crypto/blowfish: exposing the blowfish key schedule

Mostly useful for the coming crypto/bcrypt package

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

Committer: Adam Langley <agl@golang.org>