code review 2762042: crypto: add cast5

crypto: add cast5

CAST5 is the default OpenPGP cipher.

(This won't make Rob any happier about the size of crypto/, of course.)

It already has dst, src in that order but it doesn't have any users yet so I figure it's better than changing it later.

[gri, 2010-10-28 21:41:26]

lisp is for list processing, snobol is for string manipulation, and go is
for encryption...

On Thu, Oct 28, 2010 at 2:25 PM, <agl@golang.org> wrote:

> Reviewers: rsc,
>
> Description:
> crypto: add cast5
>
> CAST5 is the default OpenPGP cipher.
>
> (This won't make Rob any happier about the size of crypto/, of course.)
>
> It already has dst, src in that order but it doesn't have any users yet
> so I figure it's better than changing it later.
>
> Please review this at http://codereview.appspot.com/2762042/
>
> Affected files:
>  M src/pkg/Makefile
>  A src/pkg/crypto/cast5/Makefile
>  A src/pkg/crypto/cast5/cast5.go
>  A src/pkg/crypto/cast5/cast5_test.go
>
>
>

[r2, 2010-10-28 21:55:54]

why are there so many that matter???

-rob

[agl1, 2010-10-28 21:59:50]

On Thu, Oct 28, 2010 at 5:55 PM, Rob 'Commander' Pike <r@google.com> wrote:
> why are there so many that matter???

For years there was a standards gap between DES (too old) and AES
(didn't exist yet).

Additionally, there's an unfortunate sense of nationalism. The
Russians use GOST, the Korens have CAMELLIA (and a new one soon). The
Chinese have required that the LTE cipher be Chinese, etc.

And then it turned out that NIST made a huge mistake and that AES is
nearly impossible to implement in a way which is both fast and
side-channel free. That spurned both hardware implementations by Intel
and ESTREAM...


AGL

[rsc1, 2010-10-28 22:05:22]

LGTM

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

http://codereview.appspot.com/2762042/diff/2001/src/pkg/crypto/cast5/Makefile...
src/pkg/crypto/cast5/Makefile:1: # Copyright 2009 The Go Authors. All rights
reserved.
2010

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

http://codereview.appspot.com/2762042/diff/2001/src/pkg/crypto/cast5/cast5.go...
src/pkg/crypto/cast5/cast5.go:34: // Reset zeros the key material in memory,
although beware that the garbage
i don't think the gc may have copied it
in any event the warning probably doesn't
belong here unless it's in all the others,
which i don't suggest.

http://codereview.appspot.com/2762042/diff/2001/src/pkg/crypto/cast5/cast5.go...
src/pkg/crypto/cast5/cast5.go:139: var schedule keySchedule = keySchedule{
can drop the first keySchedule.  but also,
you can make this 

var schedule = []struct {
    a keyScheduleA
    b keyScheduleB
}{
    {
         keyScheduleA{ ... }

http://codereview.appspot.com/2762042/diff/2001/src/pkg/crypto/cast5/cast5.go...
src/pkg/crypto/cast5/cast5.go:263: var sBox [8][256]uint32 = [8][256]uint32{
var sBox = [8][256]uint32{
and then can drop the [256]uint32 inside

http://codereview.appspot.com/2762042/diff/2001/src/pkg/crypto/cast5/cast5_te...
File src/pkg/crypto/cast5/cast5_test.go (right):

http://codereview.appspot.com/2762042/diff/2001/src/pkg/crypto/cast5/cast5_te...
src/pkg/crypto/cast5/cast5_test.go:20: var basicTests = []basicTest{
var basicTests = []struct {
    key, plainText, cipherText string
}{
    {
        "0123..."

[rsc1, 2010-11-01 20:29:11]

LGTM

[agl1, 2010-11-02 18:02:24]

Hello rsc, gri, r (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[r, 2010-11-02 18:11:15]

http://codereview.appspot.com/2762042/diff/15001/src/pkg/crypto/cast5/cast5.go
File src/pkg/crypto/cast5/cast5.go (right):

http://codereview.appspot.com/2762042/diff/15001/src/pkg/crypto/cast5/cast5.g...
src/pkg/crypto/cast5/cast5.go:5: // This package implements CAST5, as defined in
RFC 2144.
for those of us in the ignorant masses, it would be good to add the line from
your CL:
// CAST5 is the default OpenPGP cipher.

http://codereview.appspot.com/2762042/diff/15001/src/pkg/crypto/cast5/cast5.g...
src/pkg/crypto/cast5/cast5.go:13: const KeySize = 16
are these needed outside the package? i think not.  that is, the methods of
Cipher publish them and that's enough.

http://codereview.appspot.com/2762042/diff/15001/src/pkg/crypto/cast5/cast5.g...
src/pkg/crypto/cast5/cast5.go:22: return nil, os.ErrorString("keys must be 16
bytes")
"CAST5: keys must be 16 bytes"

[agl1, 2010-11-03 14:38:46]

(I sorry that hg keeps spamming things. I think it gets very confused because
I'm also agl@chromium.org. I've edited codereview.py to stop it ever sending
email in the future, I hope.)

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

http://codereview.appspot.com/2762042/diff/2001/src/pkg/crypto/cast5/Makefile...
src/pkg/crypto/cast5/Makefile:1: # Copyright 2009 The Go Authors. All rights
reserved.
On 2010/10/28 22:05:22, rsc1 wrote:
> 2010

Done.

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

http://codereview.appspot.com/2762042/diff/2001/src/pkg/crypto/cast5/cast5.go...
src/pkg/crypto/cast5/cast5.go:34: // Reset zeros the key material in memory,
although beware that the garbage
On 2010/10/28 22:05:22, rsc1 wrote:
> i don't think the gc may have copied it
> in any event the warning probably doesn't
> belong here unless it's in all the others,
> which i don't suggest.

Ok, removed.

http://codereview.appspot.com/2762042/diff/2001/src/pkg/crypto/cast5/cast5.go...
src/pkg/crypto/cast5/cast5.go:139: var schedule keySchedule = keySchedule{
On 2010/10/28 22:05:22, rsc1 wrote:
> can drop the first keySchedule.  but also,
> you can make this 
> 
> var schedule = []struct {
>     a keyScheduleA
>     b keyScheduleB
> }{
>     {
>          keyScheduleA{ ... }

Done.

http://codereview.appspot.com/2762042/diff/2001/src/pkg/crypto/cast5/cast5.go...
src/pkg/crypto/cast5/cast5.go:263: var sBox [8][256]uint32 = [8][256]uint32{
On 2010/10/28 22:05:22, rsc1 wrote:
> var sBox = [8][256]uint32{
> and then can drop the [256]uint32 inside

Done.

http://codereview.appspot.com/2762042/diff/2001/src/pkg/crypto/cast5/cast5_te...
File src/pkg/crypto/cast5/cast5_test.go (right):

http://codereview.appspot.com/2762042/diff/2001/src/pkg/crypto/cast5/cast5_te...
src/pkg/crypto/cast5/cast5_test.go:20: var basicTests = []basicTest{
On 2010/10/28 22:05:22, rsc1 wrote:
> var basicTests = []struct {
>     key, plainText, cipherText string
> }{
>     {
>         "0123..."

Done.

http://codereview.appspot.com/2762042/diff/15001/src/pkg/crypto/cast5/cast5.go
File src/pkg/crypto/cast5/cast5.go (right):

http://codereview.appspot.com/2762042/diff/15001/src/pkg/crypto/cast5/cast5.g...
src/pkg/crypto/cast5/cast5.go:5: // This package implements CAST5, as defined in
RFC 2144.
On 2010/11/02 18:11:15, r wrote:
> for those of us in the ignorant masses, it would be good to add the line from
> your CL:
> // CAST5 is the default OpenPGP cipher.

Done.

http://codereview.appspot.com/2762042/diff/15001/src/pkg/crypto/cast5/cast5.g...
src/pkg/crypto/cast5/cast5.go:13: const KeySize = 16
On 2010/11/02 18:11:15, r wrote:
> are these needed outside the package? i think not.  that is, the methods of
> Cipher publish them and that's enough.

For any direct users, it's very useful. Although they would know the values
anyway, they could write:
  [cast5.KeySize]byte
rather than:
  [16]byte

when it suits them.

http://codereview.appspot.com/2762042/diff/15001/src/pkg/crypto/cast5/cast5.g...
src/pkg/crypto/cast5/cast5.go:22: return nil, os.ErrorString("keys must be 16
bytes")
On 2010/11/02 18:11:15, r wrote:
> "CAST5: keys must be 16 bytes"

Done.

[rsc, 2010-11-03 15:51:34]

On Wed, Nov 3, 2010 at 10:38,  <agl@golang.org> wrote:
> (I sorry that hg keeps spamming things. I think it gets very confused
> because I'm also agl@chromium.org. I've edited codereview.py to stop it
> ever sending email in the future, I hope.)

did you disable updating the reviewlog at all or just the email it sends?
it's useful to have the updates in the review log.

russ

[rsc, 2010-11-03 15:52:30]

also, are you using hg clpatch to move between machines?
that loses the mailed bit, which is only stored locally.
we could try to fix that.

russ

[agl1, 2010-11-03 18:08:07]

On Wed, Nov 3, 2010 at 11:52 AM, Russ Cox <rsc@golang.org> wrote:
> also, are you using hg clpatch to move between machines?
> that loses the mailed bit, which is only stored locally.
> we could try to fix that.

I think that I'm only disabling the sending of email.

I do use clpatch to move around, but only when one of my repos has
wedged itself to the point of dementia. (Which happens pretty
frequently.)


AGL

[agl1, 2010-11-03 18:09:01]

On Wed, Nov 3, 2010 at 11:52 AM, Russ Cox <rsc@golang.org> wrote:
> also, are you using hg clpatch to move between machines?
> that loses the mailed bit, which is only stored locally.
> we could try to fix that.

I think that I'm only disabling the sending of email.

I do use clpatch to move around, but only when one of my repos has
wedged itself to the point of dementia. (Which happens pretty
frequently.)


AGL

[rsc, 2010-12-07 17:02:01]

Appears to have been submitted; removing reviewers.