code review 6497101: go.crypto/nacl: add package

go.crypto/nacl: add package

[agl1, 2012-09-08 15:12:50]

Hello golang-dev@googlegroups.com (cc: dchest@gmail.com,
golang-dev@googlegroups.com),

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

[agl1, 2012-09-08 15:17:04]

Are Seal and Open good verbs for this? NaCl uses `box' and `open'. But box.Box
seems bad.

[dchest, 2012-09-08 17:04:08]

I like the API, and I think "Seal" is a good choice.

https://codereview.appspot.com/6497101/diff/1002/nacl/box/box.go
File nacl/box/box.go (right):

https://codereview.appspot.com/6497101/diff/1002/nacl/box/box.go#newcode41
nacl/box/box.go:41: curve25519.ScalarBaseMult(privateKey, publicKey)
Oops. This should be curve25519.ScalarBaseMult(publicKey, privateKey), otherwise
we'll always get all-zeros publicKey and the corresponding privateKey.

https://codereview.appspot.com/6497101/diff/1002/nacl/box/box.go#newcode50
nacl/box/box.go:50: // processing when using the same pair of key repeatedly.
s/pair of key/pair of keys/

https://codereview.appspot.com/6497101/diff/1002/nacl/box/box.go#newcode67
nacl/box/box.go:67: // precomputed, shared key as generated by Precompute.
I think comma between "precomputed" and "shared" here and in
OpenWithPrecomputation doc-comment isn't needed, but I'm not a native speaker :)

https://codereview.appspot.com/6497101/diff/1002/nacl/box/box.go#newcode68
nacl/box/box.go:68: func SealWithPrecomputation(out, message []byte, nonce
*[24]byte, sharedKey *[32]byte) []byte {
Maybe SealAfterPrecomputation or SealPrecomputed? (ditto for Open...). Not sure,
though.

https://codereview.appspot.com/6497101/diff/1002/nacl/secretbox/secretbox.go
File nacl/secretbox/secretbox.go (right):

https://codereview.appspot.com/6497101/diff/1002/nacl/secretbox/secretbox.go#...
nacl/secretbox/secretbox.go:97: // not overlap.
Maybe:

"Open authenticates and decrypts a box produced by Seal and appends the
message to out, which must not overlap box. The output will be Overhead bytes
smaller than box."

[dchest, 2012-09-08 17:16:42]

This small test case for box_test.go would catch the bug:

        if *publicKey1 == *publicKey2 || *privateKey1 == *privateKey2 {         

                t.Fatalf("generated keys are equal")                            

        }

[agl1, 2012-09-08 18:41:55]

https://codereview.appspot.com/6497101/diff/1002/nacl/box/box.go
File nacl/box/box.go (right):

https://codereview.appspot.com/6497101/diff/1002/nacl/box/box.go#newcode41
nacl/box/box.go:41: curve25519.ScalarBaseMult(privateKey, publicKey)
On 2012/09/08 17:04:08, dchest wrote:
> Oops. This should be curve25519.ScalarBaseMult(publicKey, privateKey),
otherwise
> we'll always get all-zeros publicKey and the corresponding privateKey.

Indeed! Thank you! I guess that this is a problem with passing in the output
value. It can save an allocation, but it does open one up to typos like this :(

I have also added the tests as you suggested.

https://codereview.appspot.com/6497101/diff/1002/nacl/box/box.go#newcode50
nacl/box/box.go:50: // processing when using the same pair of key repeatedly.
On 2012/09/08 17:04:08, dchest wrote:
> s/pair of key/pair of keys/

Done.

https://codereview.appspot.com/6497101/diff/1002/nacl/box/box.go#newcode67
nacl/box/box.go:67: // precomputed, shared key as generated by Precompute.
On 2012/09/08 17:04:08, dchest wrote:
> I think comma between "precomputed" and "shared" here and in
> OpenWithPrecomputation doc-comment isn't needed, but I'm not a native speaker
:)

"precomputed" is probably redundant since we say that it comes from Precompute,
so I've removed it.

https://codereview.appspot.com/6497101/diff/1002/nacl/box/box.go#newcode68
nacl/box/box.go:68: func SealWithPrecomputation(out, message []byte, nonce
*[24]byte, sharedKey *[32]byte) []byte {
On 2012/09/08 17:04:08, dchest wrote:
> Maybe SealAfterPrecomputation or SealPrecomputed? (ditto for Open...). Not
sure,
> though.

SealAfter is better than SealWith. Done.

https://codereview.appspot.com/6497101/diff/1002/nacl/secretbox/secretbox.go
File nacl/secretbox/secretbox.go (right):

https://codereview.appspot.com/6497101/diff/1002/nacl/secretbox/secretbox.go#...
nacl/secretbox/secretbox.go:97: // not overlap.
On 2012/09/08 17:04:08, dchest wrote:
> Maybe:
> 
> "Open authenticates and decrypts a box produced by Seal and appends the
> message to out, which must not overlap box. The output will be Overhead bytes
> smaller than box."

Done.

[dchest, 2012-09-08 20:44:48]

Hmm, I don't see any changes except for secretbox_test.go. Anyway, LGTM once you
upload them.

[r, 2012-09-09 13:57:28]

NaCL, eh?

https://codereview.appspot.com/6497101/diff/8002/nacl/box/box.go
File nacl/box/box.go (right):

https://codereview.appspot.com/6497101/diff/8002/nacl/box/box.go#newcode16
nacl/box/box.go:16: This package is interoperable with NaCl:
http://nacl.cr.yp.to/box.html. */
usual style is to put the */ on a line by itself

https://codereview.appspot.com/6497101/diff/8002/nacl/box/box.go#newcode49
nacl/box/box.go:49: // OpenAfterPrecomputation and SealAfterPrecomputation to
dramatically speed up
s/dramatically//

https://codereview.appspot.com/6497101/diff/8002/nacl/box/box.go#newcode57
nacl/box/box.go:57: // will be Overhead bytes longer than the original and must
not overlap. nonce
s/nonce/The nonce/
to avoid the initial-cap problem.

https://codereview.appspot.com/6497101/diff/8002/nacl/secretbox/secretbox.go
File nacl/secretbox/secretbox.go (right):

https://codereview.appspot.com/6497101/diff/8002/nacl/secretbox/secretbox.go#...
nacl/secretbox/secretbox.go:16: This package is interoperable with NaCl:
http://nacl.cr.yp.to/secretbox.html. */
ditto re: */

[rsc, 2012-09-09 14:15:53]

Finally, we can tell people we support NaCl! :-)

[agl1, 2012-09-09 15:18:17]

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

go.crypto/nacl: add package

R=golang-dev, dchest, r, rsc
CC=golang-dev
http://codereview.appspot.com/6497101

http://codereview.appspot.com/6497101/diff/8002/nacl/box/box.go
File nacl/box/box.go (right):

http://codereview.appspot.com/6497101/diff/8002/nacl/box/box.go#newcode16
nacl/box/box.go:16: This package is interoperable with NaCl:
http://nacl.cr.yp.to/box.html. */
On 2012/09/09 13:57:28, r wrote:
> usual style is to put the */ on a line by itself

Done.

http://codereview.appspot.com/6497101/diff/8002/nacl/box/box.go#newcode49
nacl/box/box.go:49: // OpenAfterPrecomputation and SealAfterPrecomputation to
dramatically speed up
On 2012/09/09 13:57:28, r wrote:
> s/dramatically//

Done.

http://codereview.appspot.com/6497101/diff/8002/nacl/box/box.go#newcode57
nacl/box/box.go:57: // will be Overhead bytes longer than the original and must
not overlap. nonce
On 2012/09/09 13:57:28, r wrote:
> s/nonce/The nonce/
> to avoid the initial-cap problem.

Done.

http://codereview.appspot.com/6497101/diff/8002/nacl/secretbox/secretbox.go
File nacl/secretbox/secretbox.go (right):

http://codereview.appspot.com/6497101/diff/8002/nacl/secretbox/secretbox.go#n...
nacl/secretbox/secretbox.go:16: This package is interoperable with NaCl:
http://nacl.cr.yp.to/secretbox.html. */
On 2012/09/09 13:57:28, r wrote:
> ditto re: */

Done.

[r, 2012-09-09 15:37:25]

On Sun, Sep 9, 2012 at 7:15 AM, Russ Cox <rsc@golang.org> wrote:
> Finally, we can tell people we support NaCl! :-)

Yay!

-rob