|
|
Created:
12 years, 2 months ago by Luit Modified:
12 years, 1 month ago Reviewers:
CC:
agl1, rsc, golang-dev Visibility:
Public. |
Descriptiongo.crypto/pbkdf2: New package
Implements PKCS#5 v2.0 and potentially the future v2.1.
Patch Set 1 #Patch Set 2 : diff -r 943eeb94bdc1 https://code.google.com/p/go.crypto #Patch Set 3 : diff -r 943eeb94bdc1 https://code.google.com/p/go.crypto #
Total comments: 16
Patch Set 4 : diff -r 943eeb94bdc1 https://code.google.com/p/go.crypto #Patch Set 5 : diff -r 943eeb94bdc1 https://code.google.com/p/go.crypto #
Total comments: 2
Patch Set 6 : diff -r 943eeb94bdc1 https://code.google.com/p/go.crypto #MessagesTotal messages: 16
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
Sign in to reply to this message.
http://codereview.appspot.com/5587050/diff/3005/pbkdf2/pbkdf2.go File pbkdf2/pbkdf2.go (right): http://codereview.appspot.com/5587050/diff/3005/pbkdf2/pbkdf2.go#newcode37 pbkdf2/pbkdf2.go:37: therefor increases security, but choosing one that's too high might make key "Using a higher iteration count will increase the cost of an exhaustive search but will also make derivation proportionally slower." http://codereview.appspot.com/5587050/diff/3005/pbkdf2/pbkdf2.go#newcode49 pbkdf2/pbkdf2.go:49: // given hashing function. New(sha1.New) will create a key derivation function s/hashing/hash/ http://codereview.appspot.com/5587050/diff/3005/pbkdf2/pbkdf2.go#newcode53 pbkdf2/pbkdf2.go:53: // The returned function takes four arguments; first two of type []byte and The types are given in the function. Rather than explain here, it would be better to given them better names on line 56: password, salt, count and outputLen, say. http://codereview.appspot.com/5587050/diff/3005/pbkdf2/pbkdf2.go#newcode68 pbkdf2/pbkdf2.go:68: binary.Write(PRF, binary.BigEndian, uint32(block)) in an inner loop like this, serialising block by hand may well be better. http://codereview.appspot.com/5587050/diff/3005/pbkdf2/pbkdf2.go#newcode69 pbkdf2/pbkdf2.go:69: U := PRF.Sum(nil) by reslicing and reusing U, you can save a lot of allocations here and below.
Sign in to reply to this message.
Thanks for doing this. I am looking forward to having this in a standard place. http://codereview.appspot.com/5587050/diff/3005/pbkdf2/pbkdf2.go File pbkdf2/pbkdf2.go (right): http://codereview.appspot.com/5587050/diff/3005/pbkdf2/pbkdf2.go#newcode56 pbkdf2/pbkdf2.go:56: func New(h func() hash.Hash) func(P, S []byte, c, dkLen int) []byte { There's no benefit to splitting the arguments here. If someone really wants a function curried like this, it is easy for them to construct it. Please make this a simple function instead of a function that returns a function. Also, please name the arguments in a way that is useful for users of this library who are not familiar with the RFC, which will be the majority of the users. // Key returns a key derived using the given password, salt, // iteration count, key length, and hash function (sha1.New for PBKDF v2.0). func Key(password, salt []byte, iter, keylen int, h func() hash.Hash) []byte Please use lower-case variables for PRF and DK too. In the implementation, declare var buf [4]byte outside the loop, and then the binary.Write becomes binary.BigEndian.PutUint32(buf[:], uint32(block)) prf.Write(buf[:4]) http://codereview.appspot.com/5587050/diff/3005/pbkdf2/pbkdf2.go#newcode60 pbkdf2/pbkdf2.go:60: l := (dkLen + hLen - 1) / hLen Please make this nblock; it looks too much like 1. http://codereview.appspot.com/5587050/diff/3005/pbkdf2/pbkdf2.go#newcode63 pbkdf2/pbkdf2.go:63: for block := 1; block <= l; block += 1 { ,s/ += 1/++/g
Sign in to reply to this message.
On 2012/01/31 16:31:21, agl1 wrote: > http://codereview.appspot.com/5587050/diff/3005/pbkdf2/pbkdf2.go#newcode53 > pbkdf2/pbkdf2.go:53: // The returned function takes four arguments; first two of > type []byte and > The types are given in the function. Rather than explain here, it would be > better to given them better names on line 56: password, salt, count and > outputLen, say. My idea with this was to keep it identical to the RFC spec. > http://codereview.appspot.com/5587050/diff/3005/pbkdf2/pbkdf2.go#newcode68 > pbkdf2/pbkdf2.go:68: binary.Write(PRF, binary.BigEndian, uint32(block)) > in an inner loop like this, serialising block by hand may well be better. > > http://codereview.appspot.com/5587050/diff/3005/pbkdf2/pbkdf2.go#newcode69 > pbkdf2/pbkdf2.go:69: U := PRF.Sum(nil) > by reslicing and reusing U, you can save a lot of allocations here and below. This loop isn't run that often. Only CEIL(dkLen / hLen) times. Are those still an issue in that case? -Luit
Sign in to reply to this message.
On Tue, Jan 31, 2012 at 11:50, <luitvd@gmail.com> wrote: > My idea with this was to keep it identical to the RFC spec. I appreciate that, but I am not really worried about catering to people who have read the spec. Those are exactly the kind of people who don't need this written for them. I am worried about people like me, who find out that they are supposed to compute a PBKDF2 hash and just want to get their work done. It is more important to have a simple, easy to use API than it is to mimic the spec. Russ
Sign in to reply to this message.
On Tue, Jan 31, 2012 at 11:50, <luitvd@gmail.com> wrote: > This loop isn't run that often. Only CEIL(dkLen / hLen) times. Are those > still an issue in that case? If it's worth writing up in a standard library, it's worth writing up well. Russ
Sign in to reply to this message.
Good points all around. Now what to call this modified function? New doesn't sound to appropriate to me with the function signature you proposed. -Luit
Sign in to reply to this message.
On Tue, Jan 31, 2012 at 12:10, <luitvd@gmail.com> wrote: > Good points all around. Now what to call this modified function? New > doesn't sound to appropriate to me with the function signature you > proposed. How about Key? Then the call looks like pbkdf2.Key, which is in fact what it returns. Russ
Sign in to reply to this message.
Hello agl@golang.org, rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
Hello agl@golang.org, rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
LGTM. You can still save significant numbers of allocations, but I can fix that up when landing if you prefer. http://codereview.appspot.com/5587050/diff/12001/pbkdf2/pbkdf2.go File pbkdf2/pbkdf2.go (right): http://codereview.appspot.com/5587050/diff/12001/pbkdf2/pbkdf2.go#newcode62 pbkdf2/pbkdf2.go:62: for n := 2; n <= iter; n += 1 { n++
Sign in to reply to this message.
On 2012/01/31 21:06:54, agl1 wrote: > LGTM. You can still save significant numbers of allocations, but I can fix that > up when landing if you prefer. It's fine by me if you want to optimize it a little. I'll watch the hg blame in a few days and see what's changed. Cheers, -Luit
Sign in to reply to this message.
Hello agl@golang.org, rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
LGTM Leaving for agl
Sign in to reply to this message.
Looking forward to agl's improvements. -Luit http://codereview.appspot.com/5587050/diff/3005/pbkdf2/pbkdf2.go File pbkdf2/pbkdf2.go (right): http://codereview.appspot.com/5587050/diff/3005/pbkdf2/pbkdf2.go#newcode37 pbkdf2/pbkdf2.go:37: therefor increases security, but choosing one that's too high might make key On 2012/01/31 16:31:21, agl1 wrote: > "Using a higher iteration count will increase the cost of an exhaustive search > but will also make derivation proportionally slower." Done. http://codereview.appspot.com/5587050/diff/3005/pbkdf2/pbkdf2.go#newcode49 pbkdf2/pbkdf2.go:49: // given hashing function. New(sha1.New) will create a key derivation function On 2012/01/31 16:31:21, agl1 wrote: > s/hashing/hash/ Done. http://codereview.appspot.com/5587050/diff/3005/pbkdf2/pbkdf2.go#newcode53 pbkdf2/pbkdf2.go:53: // The returned function takes four arguments; first two of type []byte and On 2012/01/31 16:31:21, agl1 wrote: > The types are given in the function. Rather than explain here, it would be > better to given them better names on line 56: password, salt, count and > outputLen, say. Done. http://codereview.appspot.com/5587050/diff/3005/pbkdf2/pbkdf2.go#newcode56 pbkdf2/pbkdf2.go:56: func New(h func() hash.Hash) func(P, S []byte, c, dkLen int) []byte { On 2012/01/31 16:41:15, rsc wrote: > There's no benefit to splitting the arguments here. > If someone really wants a function curried like this, > it is easy for them to construct it. > Please make this a simple function instead of a > function that returns a function. > Also, please name the arguments in a way that is useful for > users of this library who are not familiar with the RFC, which > will be the majority of the users. > > // Key returns a key derived using the given password, salt, > // iteration count, key length, and hash function (sha1.New for PBKDF v2.0). > func Key(password, salt []byte, iter, keylen int, h func() hash.Hash) []byte > > Please use lower-case variables for PRF and DK too. > In the implementation, declare > > var buf [4]byte > > outside the loop, and then the binary.Write becomes > > binary.BigEndian.PutUint32(buf[:], uint32(block)) > prf.Write(buf[:4]) > Done. http://codereview.appspot.com/5587050/diff/3005/pbkdf2/pbkdf2.go#newcode60 pbkdf2/pbkdf2.go:60: l := (dkLen + hLen - 1) / hLen On 2012/01/31 16:41:15, rsc wrote: > Please make this nblock; it looks too much like 1. Done. http://codereview.appspot.com/5587050/diff/3005/pbkdf2/pbkdf2.go#newcode63 pbkdf2/pbkdf2.go:63: for block := 1; block <= l; block += 1 { On 2012/01/31 16:41:15, rsc wrote: > ,s/ += 1/++/g Done. http://codereview.appspot.com/5587050/diff/3005/pbkdf2/pbkdf2.go#newcode68 pbkdf2/pbkdf2.go:68: binary.Write(PRF, binary.BigEndian, uint32(block)) On 2012/01/31 16:31:21, agl1 wrote: > in an inner loop like this, serialising block by hand may well be better. Done. http://codereview.appspot.com/5587050/diff/3005/pbkdf2/pbkdf2.go#newcode69 pbkdf2/pbkdf2.go:69: U := PRF.Sum(nil) On 2012/01/31 16:31:21, agl1 wrote: > by reslicing and reusing U, you can save a lot of allocations here and below. Done. http://codereview.appspot.com/5587050/diff/12001/pbkdf2/pbkdf2.go File pbkdf2/pbkdf2.go (right): http://codereview.appspot.com/5587050/diff/12001/pbkdf2/pbkdf2.go#newcode62 pbkdf2/pbkdf2.go:62: for n := 2; n <= iter; n += 1 { On 2012/01/31 21:06:54, agl1 wrote: > n++ Done.
Sign in to reply to this message.
*** Submitted as b5c95223a8c6 *** go.crypto/pbkdf2: New package Implements PKCS#5 v2.0 and potentially the future v2.1. R=agl, rsc CC=golang-dev http://codereview.appspot.com/5587050 Committer: Adam Langley <agl@golang.org>
Sign in to reply to this message.
|