code review 6535043: go.crypto/scrypt: add package

go.crypto/scrypt: add package

[dchest, 2012-09-18 12:34:56]

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

I'd like you to review this change to
https://code.google.com/p/go.crypto

[agl1, 2012-09-18 12:55:20]

LGTM, thanks!

https://codereview.appspot.com/6535043/diff/4001/scrypt/scrypt.go
File scrypt/scrypt.go (right):

https://codereview.appspot.com/6535043/diff/4001/scrypt/scrypt.go#newcode22
scrypt/scrypt.go:22: func blockCopy(dst, src []byte, n int) {
I wonder that this function may be too short to justify its existence. Maybe
just call copy() instead? Up to you.

https://codereview.appspot.com/6535043/diff/4001/scrypt/scrypt.go#newcode80
scrypt/scrypt.go:80: // N is a CPU/memory cost parameter, must be a power of two
greater than 1.
s/, /, which /

https://codereview.appspot.com/6535043/diff/4001/scrypt/scrypt.go#newcode97
scrypt/scrypt.go:97: return nil, errors.New("scrypt: parameters are too big")
"scrypt: cost parameters too large"

https://codereview.appspot.com/6535043/diff/4001/scrypt/scrypt_test.go
File scrypt/scrypt_test.go (right):

https://codereview.appspot.com/6535043/diff/4001/scrypt/scrypt_test.go#newcode91
scrypt/scrypt_test.go:91: for _, v := range good {
for i, v := range good {

https://codereview.appspot.com/6535043/diff/4001/scrypt/scrypt_test.go#newcode94
scrypt/scrypt_test.go:94: t.Errorf("got unexpected error: %s", err)
t.Errorf("%d: got unexpected error: %s", i, err)
continue

https://codereview.appspot.com/6535043/diff/4001/scrypt/scrypt_test.go#newcode97
scrypt/scrypt_test.go:97: t.Errorf("expected %x, got %x", v.output, k)
t.Errorf("%d: expected %x, got %x", i, v.output, k)

https://codereview.appspot.com/6535043/diff/4001/scrypt/scrypt_test.go#newcod...
scrypt/scrypt_test.go:100: for _, v := range bad {
for i, v := range bad {

https://codereview.appspot.com/6535043/diff/4001/scrypt/scrypt_test.go#newcod...
scrypt/scrypt_test.go:103: t.Errorf("expected error, got nil")
t.Errorf("%d: expected error, got nil", i)

[dchest, 2012-09-18 13:03:12]

PTAL

https://codereview.appspot.com/6535043/diff/4001/scrypt/scrypt.go
File scrypt/scrypt.go (right):

https://codereview.appspot.com/6535043/diff/4001/scrypt/scrypt.go#newcode22
scrypt/scrypt.go:22: func blockCopy(dst, src []byte, n int) {
On 2012/09/18 12:55:20, agl1 wrote:
> I wonder that this function may be too short to justify its existence. Maybe
> just call copy() instead? Up to you.

I tried that, but the code becomes uglier: we need to limit the second slice, so
instead of:

   blockCopy(b[(i+r)*64:], y[(i*2+1)*64:], 64)

we get:

  copy(b[(i+r)*64:], y[(i*2+1)*64:(i*2+1)*64 + 64])

Plus, it indicates that scrypt works in "blocks", and blockCopy and blockXOR are
related.

The compiler inlines this function anyway.

https://codereview.appspot.com/6535043/diff/4001/scrypt/scrypt.go#newcode80
scrypt/scrypt.go:80: // N is a CPU/memory cost parameter, must be a power of two
greater than 1.
On 2012/09/18 12:55:20, agl1 wrote:
> s/, /, which /

Done.

https://codereview.appspot.com/6535043/diff/4001/scrypt/scrypt.go#newcode97
scrypt/scrypt.go:97: return nil, errors.New("scrypt: parameters are too big")
On 2012/09/18 12:55:20, agl1 wrote:
> "scrypt: cost parameters too large"

Done.

[dchest, 2012-09-18 13:04:56]

https://codereview.appspot.com/6535043/diff/4001/scrypt/scrypt_test.go
File scrypt/scrypt_test.go (right):

https://codereview.appspot.com/6535043/diff/4001/scrypt/scrypt_test.go#newcode91
scrypt/scrypt_test.go:91: for _, v := range good {
On 2012/09/18 12:55:20, agl1 wrote:
> for i, v := range good {

Done.

https://codereview.appspot.com/6535043/diff/4001/scrypt/scrypt_test.go#newcode94
scrypt/scrypt_test.go:94: t.Errorf("got unexpected error: %s", err)
On 2012/09/18 12:55:20, agl1 wrote:
> t.Errorf("%d: got unexpected error: %s", i, err)
> continue

Done.

https://codereview.appspot.com/6535043/diff/4001/scrypt/scrypt_test.go#newcode97
scrypt/scrypt_test.go:97: t.Errorf("expected %x, got %x", v.output, k)
On 2012/09/18 12:55:20, agl1 wrote:
> t.Errorf("%d: expected %x, got %x", i, v.output, k)

Done.

https://codereview.appspot.com/6535043/diff/4001/scrypt/scrypt_test.go#newcod...
scrypt/scrypt_test.go:100: for _, v := range bad {
On 2012/09/18 12:55:20, agl1 wrote:
> for i, v := range bad {

Done.

https://codereview.appspot.com/6535043/diff/4001/scrypt/scrypt_test.go#newcod...
scrypt/scrypt_test.go:103: t.Errorf("expected error, got nil")
On 2012/09/18 12:55:20, agl1 wrote:
> t.Errorf("%d: expected error, got nil", i)

Done.

[r, 2012-09-18 14:50:41]

https://codereview.appspot.com/6535043/diff/9002/scrypt/scrypt.go
File scrypt/scrypt.go (right):

https://codereview.appspot.com/6535043/diff/9002/scrypt/scrypt.go#newcode7
scrypt/scrypt.go:7: // Functions".
can you link or at least provide a more detailed reference?
looks like this works:
http://www.bsdcan.org/2009/schedule/attachments/87_scrypt.pdf
maybe there's a more definitive location

https://codereview.appspot.com/6535043/diff/9002/scrypt/scrypt.go#newcode53
scrypt/scrypt.go:53: func integerify(b []byte, r int) uint64 {
ugly word. "integer" is fine - think of it like a conversion.
another choice would be toUint64

[dchest, 2012-09-18 15:03:35]

https://codereview.appspot.com/6535043/diff/9002/scrypt/scrypt.go
File scrypt/scrypt.go (right):

https://codereview.appspot.com/6535043/diff/9002/scrypt/scrypt.go#newcode7
scrypt/scrypt.go:7: // Functions".
On 2012/09/18 14:50:41, r wrote:
> can you link or at least provide a more detailed reference?
> looks like this works:
> http://www.bsdcan.org/2009/schedule/attachments/87_scrypt.pdf
> maybe there's a more definitive location

Done.

https://codereview.appspot.com/6535043/diff/9002/scrypt/scrypt.go#newcode53
scrypt/scrypt.go:53: func integerify(b []byte, r int) uint64 {
On 2012/09/18 14:50:41, r wrote:
> ugly word. "integer" is fine - think of it like a conversion.
> another choice would be toUint64

I tried to follow function names specified in paper.

Alternatively, I can just get rid of this function entirely, since
"binary.LittleEndian.Uint64" already says what it does.

[r, 2012-09-18 15:20:48]

https://codereview.appspot.com/6535043/diff/9002/scrypt/scrypt.go
File scrypt/scrypt.go (right):

https://codereview.appspot.com/6535043/diff/9002/scrypt/scrypt.go#newcode53
scrypt/scrypt.go:53: func integerify(b []byte, r int) uint64 {
i don't mind this function; it captures some clumsy math. but i would like a
name change. the connection to the paper doesn't need to be slavish.

[dchest, 2012-09-18 15:27:17]

PTAL

https://codereview.appspot.com/6535043/diff/9002/scrypt/scrypt.go
File scrypt/scrypt.go (right):

https://codereview.appspot.com/6535043/diff/9002/scrypt/scrypt.go#newcode53
scrypt/scrypt.go:53: func integerify(b []byte, r int) uint64 {
On 2012/09/18 15:20:48, r wrote:
> i don't mind this function; it captures some clumsy math. but i would like a
> name change. the connection to the paper doesn't need to be slavish.

Done. (integer)

[r, 2012-09-18 15:41:21]

LGTM
leaving for agl

[agl1, 2012-09-18 17:51:57]

https://codereview.appspot.com/6535043/diff/10003/scrypt/scrypt.go
File scrypt/scrypt.go (right):

https://codereview.appspot.com/6535043/diff/10003/scrypt/scrypt.go#newcode19
scrypt/scrypt.go:19: const maxInt = int(^uint(0) >> 1)
The size of uint is implementation specific, therefore the behaviour of this
package varies based on implementation.

That seem unfortunate and I wonder whether we shouldn't import "math" and
s/maxInt/math.MaxInt32/.

If you agree, I can make that change when landing.

[r, 2012-09-18 17:57:13]

You don't need math for MaxInt32. Just do the obvious adjustment:

const maxInt32 = int32(^uint32(0)>>1)

-rob

[dchest, 2012-09-18 19:55:55]

On 2012/09/18 17:51:57, agl1 wrote:
> https://codereview.appspot.com/6535043/diff/10003/scrypt/scrypt.go
> File scrypt/scrypt.go (right):
> 
> https://codereview.appspot.com/6535043/diff/10003/scrypt/scrypt.go#newcode19
> scrypt/scrypt.go:19: const maxInt = int(^uint(0) >> 1)
> The size of uint is implementation specific, therefore the behaviour of this
> package varies based on implementation.
> 
> That seem unfortunate and I wonder whether we shouldn't import "math" and
> s/maxInt/math.MaxInt32/.
> 
> If you agree, I can make that change when landing.

I specifically made it depend on the size of int, because this constant is used
to decide how much memory we can allocate with make(). However, once we make int
64-bit, we can allocate much more memory (in theory), so scrypt will continue
working (and accepting higher memory cost) without changing the code.

[dchest, 2012-09-18 20:07:23]

On 2012/09/18 19:55:55, dchest wrote:
> On 2012/09/18 17:51:57, agl1 wrote:
> > https://codereview.appspot.com/6535043/diff/10003/scrypt/scrypt.go
> > File scrypt/scrypt.go (right):
> > 
> > https://codereview.appspot.com/6535043/diff/10003/scrypt/scrypt.go#newcode19
> > scrypt/scrypt.go:19: const maxInt = int(^uint(0) >> 1)
> > The size of uint is implementation specific, therefore the behaviour of this
> > package varies based on implementation.
> > 
> > That seem unfortunate and I wonder whether we shouldn't import "math" and
> > s/maxInt/math.MaxInt32/.
> > 
> > If you agree, I can make that change when landing.
> 
> I specifically made it depend on the size of int, because this constant is
used
> to decide how much memory we can allocate with make(). However, once we make
int
> 64-bit, we can allocate much more memory (in theory), so scrypt will continue
> working (and accepting higher memory cost) without changing the code.

To elaborate, the caller must ensure that the maximum parameter for N is
acceptable for their hardware setup anyway (that is, if they have only 512 MB of
RAM available, they can't pass N=1048576, because it will try to allocate 1
GiB), just like users of PBKDF2 or bcrypt must not call them with the cost too
high for their CPUs to process in reasonable time. I see no reason to limit
scrypt to 2 GiB if the hardware and Go will allow for more in the future.

[agl1, 2012-09-18 20:59:59]

*** Submitted as
http://code.google.com/p/go/source/detail?r=ad8a96e038bb&repo=crypto ***

go.crypto/scrypt: add package

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

Committer: Adam Langley <agl@golang.org>

[pyramids, 2014-03-16 18:53:20]

On 2012/09/18 20:59:59, agl1 wrote:
> *** Submitted as
> http://code.google.com/p/go/source/detail?r=ad8a96e038bb&repo=crypto ***
> 
> go.crypto/scrypt: add package
> 
> R=golang-dev, agl, r
> CC=golang-dev
> http://codereview.appspot.com/6535043
> 
> Committer: Adam Langley <mailto:agl@golang.org>


What is the status regarding inclusion in Go itself?

It does not appear to be included in Go 1.2, despite (assuming I understand this
thread correctly) having been included in the Go sources nearly 18 month ago.

[agl1, 2014-03-16 19:07:55]

On Sun, Mar 16, 2014 at 2:53 PM,  <stein.bjoern@gmail.com> wrote:
> It does not appear to be included in Go 1.2, despite (assuming I
> understand this thread correctly) having been included in the Go sources
> nearly 18 month ago.

There are no plans to move scrypt into the core libraries.


Cheers

AGL

[pyramids, 2014-03-16 19:46:32]

On 2014/03/16 19:07:55, agl1 wrote:
> On Sun, Mar 16, 2014 at 2:53 PM,  <mailto:stein.bjoern@gmail.com> wrote:
> > It does not appear to be included in Go 1.2, despite (assuming I
> > understand this thread correctly) having been included in the Go sources
> > nearly 18 month ago.
> 
> There are no plans to move scrypt into the core libraries.
> 
> 
> Cheers
> 
> AGL

Thanks for the info.
I guess I was under the misconception that the path it's hosted at implied it
should be or become part of the core libraries.