code review 4331054: crypto/des: new package providing implementations of DE...

crypto/des: new package providing implementations of DES and TDEA
Original code by Chris Lennert <cale...@gmail.com>

[mattn, 2011-04-05 03:09:33]

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

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

[mattn, 2011-04-05 03:10:19]

I'll add the changes of peter's point.

On 2011/04/05 03:09:33, mattn wrote:
> Hello mailto:golang-dev@googlegroups.com (cc:
mailto:golang-dev@googlegroups.com),
> 
> I'd like you to review this change to
> http://go.googlecode.com/hg/

[mattn, 2011-04-05 03:11:25]

and also agl's point.

ref: http://codereview.appspot.com/3643043/

On 2011/04/05 03:10:19, mattn wrote:
> I'll add the changes of peter's point.
> 
> On 2011/04/05 03:09:33, mattn wrote:
> > Hello mailto:golang-dev@googlegroups.com (cc:
> mailto:golang-dev@googlegroups.com),
> > 
> > I'd like you to review this change to
> > http://go.googlecode.com/hg/

[rsc1, 2011-04-05 03:12:49]

Sounds good.  Let us know when you have revised the code.

On Mon, Apr 4, 2011 at 23:11,  <mattn.jp@gmail.com> wrote:
> and also agl's point.
>
> ref: http://codereview.appspot.com/3643043/
>
> On 2011/04/05 03:10:19, mattn wrote:
>>
>> I'll add the changes of peter's point.
>
>> On 2011/04/05 03:09:33, mattn wrote:
>> > Hello mailto:golang-dev@googlegroups.com (cc:
>> mailto:golang-dev@googlegroups.com),
>> >
>> > I'd like you to review this change to
>> > http://go.googlecode.com/hg/
>
>
>
> http://codereview.appspot.com/4331054/
>

[mattn, 2011-04-05 03:28:24]

NOTE) This is agl's post in http://codereview.appspot.com/3643043/

This is just a superficial review highlighting some API and style issues.

http://codereview.appspot.com/4331054/diff/2001/src/pkg/crypto/des/block.go
File src/pkg/crypto/des/block.go (right):

http://codereview.appspot.com/4331054/diff/2001/src/pkg/crypto/des/block.go#n...
src/pkg/crypto/des/block.go:19: right := uint32((permutedSrc << 32) >> 32)
I don't think the shifting is needed here. The cast will truncate.

http://codereview.appspot.com/4331054/diff/2001/src/pkg/crypto/des/block.go#n...
src/pkg/crypto/des/block.go:32: for i := 0; i < 8; i++ {
There is copy() for this.

http://codereview.appspot.com/4331054/diff/2001/src/pkg/crypto/des/block.go#n...
src/pkg/crypto/des/block.go:44: right := uint32((permutedSrc << 32) >> 32)
ditto about the shift

http://codereview.appspot.com/4331054/diff/2001/src/pkg/crypto/des/block.go#n...
src/pkg/crypto/des/block.go:58: for i := 0; i < 8; i++ {
ditto about copy()

http://codereview.appspot.com/4331054/diff/2001/src/pkg/crypto/des/block.go#n...
src/pkg/crypto/des/block.go:65: rightExpanded :=
permuteBlock(uint32ToBytes(right), expansionFunction[0:])
you don't need the '0' in [0:]

http://codereview.appspot.com/4331054/diff/2001/src/pkg/crypto/des/block.go#n...
src/pkg/crypto/des/block.go:69: for ; i < 8; i++ {
i := 0

http://codereview.appspot.com/4331054/diff/2001/src/pkg/crypto/des/block.go#n...
src/pkg/crypto/des/block.go:71: // determine the proper S-box row and column
from the 6-bits of sBoxCoordValue
please wrap comment.

http://codereview.appspot.com/4331054/diff/2001/src/pkg/crypto/des/block.go#n...
src/pkg/crypto/des/block.go:84: var bitIndex = bitNumber - 1
:= for these

http://codereview.appspot.com/4331054/diff/2001/src/pkg/crypto/des/block.go#n...
src/pkg/crypto/des/block.go:100: part1 := (last << (4 + uint32(ksRotations[i])))
>> 4
Does the "4 +" not cancel with the ">> 4"? (Or are you deliberately truncating
the word using that?)

http://codereview.appspot.com/4331054/diff/2001/src/pkg/crypto/des/block.go#n...
src/pkg/crypto/des/block.go:128: buf := bytes.NewBuffer(nil)
I would pass in the slice which is returned to save on the allocation. Also,
ret[0] = byte(block >> 24)
ret[1] = byte(block >> 16)
...

http://codereview.appspot.com/4331054/diff/2001/src/pkg/crypto/des/block.go#n...
src/pkg/crypto/des/block.go:135: buf := bytes.NewBuffer(nil)
likewise

http://codereview.appspot.com/4331054/diff/2001/src/pkg/crypto/des/cipher.go
File src/pkg/crypto/des/cipher.go (right):

http://codereview.appspot.com/4331054/diff/2001/src/pkg/crypto/des/cipher.go#...
src/pkg/crypto/des/cipher.go:24: subkeys []uint64
Since this is always 16 elements, it could be an array.

http://codereview.appspot.com/4331054/diff/2001/src/pkg/crypto/des/cipher.go#...
src/pkg/crypto/des/cipher.go:29: k := len(key)
this is just

if len(key) != 8 {
  return nil, KeySizeError(k)
}

http://codereview.appspot.com/4331054/diff/2001/src/pkg/crypto/des/cipher.go#...
src/pkg/crypto/des/cipher.go:51: func (c *DESCipher) Encrypt(src, dst []byte) {
encryptBlock(c.subkeys, src, dst) }
dst should be the first argument.

http://codereview.appspot.com/4331054/diff/2001/src/pkg/crypto/des/cipher.go#...
src/pkg/crypto/des/cipher.go:54: func (c *DESCipher) Decrypt(src, dst []byte) {
decryptBlock(c.subkeys, src, dst) }
dst should be the first argument.

http://codereview.appspot.com/4331054/diff/2001/src/pkg/crypto/des/cipher.go#...
src/pkg/crypto/des/cipher.go:68: type TDEACipher struct {
I think this would be better named TripleDESCipher (and likewise throughout)

http://codereview.appspot.com/4331054/diff/2001/src/pkg/crypto/des/cipher.go#...
src/pkg/crypto/des/cipher.go:75: k := len(key)
ditto about the if statement.

http://codereview.appspot.com/4331054/diff/2001/src/pkg/crypto/des/cipher.go#...
src/pkg/crypto/des/cipher.go:99: func (c *TDEACipher) Encrypt(src, dst []byte) {
dst is first.

http://codereview.appspot.com/4331054/diff/2001/src/pkg/crypto/des/cipher.go#...
src/pkg/crypto/des/cipher.go:106: func (c *TDEACipher) Decrypt(src, dst []byte)
{
dst first.

http://codereview.appspot.com/4331054/diff/2001/src/pkg/crypto/des/const.go
File src/pkg/crypto/des/const.go (right):

http://codereview.appspot.com/4331054/diff/2001/src/pkg/crypto/des/const.go#n...
src/pkg/crypto/des/const.go:89: [4][16]uint8{
I don't think that the "[4][16]uint8" is needed.

[mattn, 2011-04-05 04:26:18]

NOTE) This is copy of comments in http://codereview.appspot.com/3643043/


http://codereview.appspot.com/3643043/diff/2001/src/pkg/crypto/des/block.go
File src/pkg/crypto/des/block.go (right):

http://codereview.appspot.com/3643043/diff/2001/src/pkg/crypto/des/block.go#n...
src/pkg/crypto/des/block.go:19: right := uint32((permutedSrc << 32) >> 32)
On 2010/12/17 16:03:53, agl1 wrote:
> I don't think the shifting is needed here. The cast will truncate.

Done.

http://codereview.appspot.com/3643043/diff/2001/src/pkg/crypto/des/block.go#n...
src/pkg/crypto/des/block.go:32: for i := 0; i < 8; i++ {
On 2010/12/17 16:03:53, agl1 wrote:
> There is copy() for this.

Done.

http://codereview.appspot.com/3643043/diff/2001/src/pkg/crypto/des/block.go#n...
src/pkg/crypto/des/block.go:44: right := uint32((permutedSrc << 32) >> 32)
On 2010/12/17 16:03:53, agl1 wrote:
> ditto about the shift

Done.

http://codereview.appspot.com/3643043/diff/2001/src/pkg/crypto/des/block.go#n...
src/pkg/crypto/des/block.go:58: for i := 0; i < 8; i++ {
On 2010/12/17 16:03:53, agl1 wrote:
> ditto about copy()

Done.

http://codereview.appspot.com/3643043/diff/2001/src/pkg/crypto/des/block.go#n...
src/pkg/crypto/des/block.go:65: rightExpanded :=
permuteBlock(uint32ToBytes(right), expansionFunction[0:])
On 2010/12/17 16:03:53, agl1 wrote:
> you don't need the '0' in [0:]

Done.

http://codereview.appspot.com/3643043/diff/2001/src/pkg/crypto/des/block.go#n...
src/pkg/crypto/des/block.go:69: for ; i < 8; i++ {
On 2010/12/17 16:03:53, agl1 wrote:
> i := 0

Done.

http://codereview.appspot.com/3643043/diff/2001/src/pkg/crypto/des/block.go#n...
src/pkg/crypto/des/block.go:69: for ; i < 8; i++ {
On 2011/01/09 20:42:19, B-Ranger wrote:
> On 2010/12/17 16:03:53, agl1 wrote:
> > i := 0
> 
> I'd prefer 
> i:=uint8(0) and delete L.67
> this way i will not exist longer than needed

Done.

http://codereview.appspot.com/3643043/diff/2001/src/pkg/crypto/des/block.go#n...
src/pkg/crypto/des/block.go:71: // determine the proper S-box row and column
from the 6-bits of sBoxCoordValue
On 2010/12/17 16:03:53, agl1 wrote:
> please wrap comment.

Done.

http://codereview.appspot.com/3643043/diff/2001/src/pkg/crypto/des/block.go#n...
src/pkg/crypto/des/block.go:84: var bitIndex = bitNumber - 1
On 2010/12/17 16:03:53, agl1 wrote:
> := for these

Done.

http://codereview.appspot.com/3643043/diff/2001/src/pkg/crypto/des/block.go#n...
src/pkg/crypto/des/block.go:100: part1 := (last << (4 + uint32(ksRotations[i])))
>> 4
If change as your said, test don't pass.

On 2010/12/17 16:03:53, agl1 wrote:
> Does the "4 +" not cancel with the ">> 4"? (Or are you deliberately truncating
> the word using that?)

http://codereview.appspot.com/3643043/diff/2001/src/pkg/crypto/des/block.go#n...
src/pkg/crypto/des/block.go:128: buf := bytes.NewBuffer(nil)
On 2010/12/17 16:03:53, agl1 wrote:
> I would pass in the slice which is returned to save on the allocation. Also,
> ret[0] = byte(block >> 24)
> ret[1] = byte(block >> 16)
> ...

Done.

http://codereview.appspot.com/3643043/diff/2001/src/pkg/crypto/des/block.go#n...
src/pkg/crypto/des/block.go:135: buf := bytes.NewBuffer(nil)
On 2010/12/17 16:03:53, agl1 wrote:
> likewise

Done.

http://codereview.appspot.com/3643043/diff/2001/src/pkg/crypto/des/cipher.go
File src/pkg/crypto/des/cipher.go (right):

http://codereview.appspot.com/3643043/diff/2001/src/pkg/crypto/des/cipher.go#...
src/pkg/crypto/des/cipher.go:24: subkeys []uint64
On 2010/12/17 16:03:53, agl1 wrote:
> Since this is always 16 elements, it could be an array.

Done.

http://codereview.appspot.com/3643043/diff/2001/src/pkg/crypto/des/cipher.go#...
src/pkg/crypto/des/cipher.go:29: k := len(key)
On 2010/12/17 16:03:53, agl1 wrote:
> this is just
> 
> if len(key) != 8 {
>   return nil, KeySizeError(k)
> }

Done.

http://codereview.appspot.com/3643043/diff/2001/src/pkg/crypto/des/cipher.go#...
src/pkg/crypto/des/cipher.go:43: // It is necessary to satisfy the Block
interface in the
On 2011/01/09 20:42:19, B-Ranger wrote:
> please delete the "necessary" comment

Done.

http://codereview.appspot.com/3643043/diff/2001/src/pkg/crypto/des/cipher.go#...
src/pkg/crypto/des/cipher.go:51: func (c *DESCipher) Encrypt(src, dst []byte) {
encryptBlock(c.subkeys, src, dst) }
On 2010/12/17 16:03:53, agl1 wrote:
> dst should be the first argument.

Done.

http://codereview.appspot.com/3643043/diff/2001/src/pkg/crypto/des/cipher.go#...
src/pkg/crypto/des/cipher.go:54: func (c *DESCipher) Decrypt(src, dst []byte) {
decryptBlock(c.subkeys, src, dst) }
On 2010/12/17 16:03:53, agl1 wrote:
> dst should be the first argument.

Done.

http://codereview.appspot.com/3643043/diff/2001/src/pkg/crypto/des/cipher.go#...
src/pkg/crypto/des/cipher.go:68: type TDEACipher struct {
On 2010/12/17 16:03:53, agl1 wrote:
> I think this would be better named TripleDESCipher (and likewise throughout)

Done.

http://codereview.appspot.com/3643043/diff/2001/src/pkg/crypto/des/cipher.go#...
src/pkg/crypto/des/cipher.go:75: k := len(key)
On 2010/12/17 16:03:53, agl1 wrote:
> ditto about the if statement.

Done.

http://codereview.appspot.com/3643043/diff/2001/src/pkg/crypto/des/cipher.go#...
src/pkg/crypto/des/cipher.go:99: func (c *TDEACipher) Encrypt(src, dst []byte) {
On 2010/12/17 16:03:53, agl1 wrote:
> dst is first.

Done.

http://codereview.appspot.com/3643043/diff/2001/src/pkg/crypto/des/cipher.go#...
src/pkg/crypto/des/cipher.go:106: func (c *TDEACipher) Decrypt(src, dst []byte)
{
On 2010/12/17 16:03:53, agl1 wrote:
> dst first.

Done.

http://codereview.appspot.com/3643043/diff/2001/src/pkg/crypto/des/const.go
File src/pkg/crypto/des/const.go (right):

http://codereview.appspot.com/3643043/diff/2001/src/pkg/crypto/des/const.go#n...
src/pkg/crypto/des/const.go:89: [4][16]uint8{
On 2010/12/17 16:03:53, agl1 wrote:
> I don't think that the "[4][16]uint8" is needed.

Done.

http://codereview.appspot.com/3643043/diff/2001/src/pkg/crypto/des/des_test.go
File src/pkg/crypto/des/des_test.go (right):

http://codereview.appspot.com/3643043/diff/2001/src/pkg/crypto/des/des_test.g...
src/pkg/crypto/des/des_test.go:19: CryptTest{
Sorry I can't understand 3

On 2011/01/09 20:42:19, B-Ranger wrote:
> 1. type specification 'CryptTest' is not necessary
> 2. please put multiline closing braces on next line
> 3. Let's get some external sources for test vectors and minimize the number of
> homebrewn vectors.

http://codereview.appspot.com/3643043/diff/2001/src/pkg/crypto/des/des_test.g...
src/pkg/crypto/des/des_test.go:1229: 
Sorry, please point that.

On 2011/01/09 20:42:19, B-Ranger wrote:
> Lots of your functions are just plain copies of the one above differing only
in
> the use of "encrypt" and "decrypt". Please minimize code duplication.

[rsc, 2011-04-05 05:49:41]

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

http://codereview.appspot.com/4331054/diff/4001/src/pkg/crypto/des/block.go#n...
src/pkg/crypto/des/block.go:13: func encryptBlock(subkeys []uint64, src, dst
[]byte) {
dst, src

http://codereview.appspot.com/4331054/diff/4001/src/pkg/crypto/des/block.go#n...
src/pkg/crypto/des/block.go:14: // perform initial permutation
There are many things you can do to
shorten this function.  Parallel assignments,
range statements, don't allocate by calling uint64ToBytes.
Combining all of these you end up with something like:

func encryptBlock(subkeys []uint64, dst, src []byte) {
    b := binary.BigEndian.Uint64(src)
    b = permuteBlock(b, initialPermutation[:])
    left, right := uint32(b>>32), uint32(b)
    for _, key := range subkeys {
        left, right = right, left ^ feistel(right, key)
    }
    b = permuteBlock(uint64(right)<<32 | uint64(left), finalPerm[:])
    binary.BigEndian.PutUint64(dst, b)
}

http://codereview.appspot.com/4331054/diff/4001/src/pkg/crypto/des/block.go#n...
src/pkg/crypto/des/block.go:38: func decryptBlock(subkeys []uint64, src []byte,
dst []byte) {
dst, src

http://codereview.appspot.com/4331054/diff/4001/src/pkg/crypto/des/block.go#n...
src/pkg/crypto/des/block.go:39: // perform initial permutation
All the cleanup of encryptBlock applies here too.

http://codereview.appspot.com/4331054/diff/4001/src/pkg/crypto/des/block.go#n...
src/pkg/crypto/des/block.go:82: func permuteBlock(src []byte, permutation
[]uint8) (block uint64) {
Operating on []byte here is forcing allocations
that are quite expensive.  Just take a uint64.
That makes the math easier anyway:

func permuteBlock(b uint64, perm []uint8) uint64 {
    var c uint64
    for i, bit := range perm {
        c |= ((b>>(bit-1))&1) << uint(len(perm) - 1 - i)
    }
}

http://codereview.appspot.com/4331054/diff/4001/src/pkg/crypto/des/block.go#n...
src/pkg/crypto/des/block.go:100: part1 := (last << (4 + uint32(ksRotations[i])))
>> 4
The shift counts do not have to be the same type
as the thing being shifted.  They just have to be unsigned.

part1 := (last << (4+ksRotations[i])) >> 4

http://codereview.appspot.com/4331054/diff/4001/src/pkg/crypto/des/block.go#n...
src/pkg/crypto/des/block.go:127: func uint32ToBytes(block uint32) []byte {
Delete this function and the next one.
They allocate and then they use reflection
to unpack 4 or 8 bytes.  This will completely
dwarf the rest of the crypto processing.
Rewrite the code not to use them.  See suggestions above.

http://codereview.appspot.com/4331054/diff/4001/src/pkg/crypto/des/cipher.go
File src/pkg/crypto/des/cipher.go (right):

http://codereview.appspot.com/4331054/diff/4001/src/pkg/crypto/des/cipher.go#...
src/pkg/crypto/des/cipher.go:30: switch k {
if k != 8

http://codereview.appspot.com/4331054/diff/4001/src/pkg/crypto/des/cipher.go#...
src/pkg/crypto/des/cipher.go:47: // Encrypts the 8-byte buffer src and stores
the result in dst.
Encrypt encrypts ...

http://codereview.appspot.com/4331054/diff/4001/src/pkg/crypto/des/cipher.go#...
src/pkg/crypto/des/cipher.go:51: func (c *DESCipher) Encrypt(src, dst []byte) {
encryptBlock(c.subkeys, src, dst) }
dst, src.

please use a multi-line function body

http://codereview.appspot.com/4331054/diff/4001/src/pkg/crypto/des/cipher.go#...
src/pkg/crypto/des/cipher.go:53: // Decrypts the 8-byte buffer src and stores
the result in dst.
Decrypt decrypts

http://codereview.appspot.com/4331054/diff/4001/src/pkg/crypto/des/cipher.go#...
src/pkg/crypto/des/cipher.go:54: func (c *DESCipher) Decrypt(src, dst []byte) {
decryptBlock(c.subkeys, src, dst) }
dst, src.

please use a multi-line function body.

http://codereview.appspot.com/4331054/diff/4001/src/pkg/crypto/des/cipher.go#...
src/pkg/crypto/des/cipher.go:67: // A TDEACipher is an instance of TDEA
encryption.
// A TDEACipher is an instance of TDEA (triple-DES) encryption.

http://codereview.appspot.com/4331054/diff/4001/src/pkg/crypto/des/cipher.go#...
src/pkg/crypto/des/cipher.go:76: switch k {
if k != 3*8

http://codereview.appspot.com/4331054/diff/4001/src/pkg/crypto/des/cipher.go#...
src/pkg/crypto/des/cipher.go:95: // Encrypts the 8-byte buffer src and stores
the result in dst.
// Encrypt encrypts ...

http://codereview.appspot.com/4331054/diff/4001/src/pkg/crypto/des/cipher.go#...
src/pkg/crypto/des/cipher.go:99: func (c *TDEACipher) Encrypt(src, dst []byte) {
This is backward.  dst, src.

http://codereview.appspot.com/4331054/diff/4001/src/pkg/crypto/des/cipher.go#...
src/pkg/crypto/des/cipher.go:105: // Decrypts the 8-byte buffer src and stores
the result in dst.
// Decrypt decrypts...

http://codereview.appspot.com/4331054/diff/4001/src/pkg/crypto/des/cipher.go#...
src/pkg/crypto/des/cipher.go:106: func (c *TDEACipher) Decrypt(src, dst []byte)
{
dst, src

http://codereview.appspot.com/4331054/diff/4001/src/pkg/crypto/des/const.go
File src/pkg/crypto/des/const.go (right):

http://codereview.appspot.com/4331054/diff/4001/src/pkg/crypto/des/const.go#n...
src/pkg/crypto/des/const.go:20: 63, 55, 47, 39, 31, 23, 15, 7}
please move all these } onto the lines
by themselves.  you will have to insert a ,
at the end of this line.
same for the others below

http://codereview.appspot.com/4331054/diff/4001/src/pkg/crypto/des/const.go#n...
src/pkg/crypto/des/const.go:89: [4][16]uint8{
change this line to just
    {
same for all the lines below,  
    [4][16]uint8{  ->  {
    [16]uint8{  ->  {

http://codereview.appspot.com/4331054/diff/4001/src/pkg/crypto/des/des_test.go
File src/pkg/crypto/des/des_test.go (right):

http://codereview.appspot.com/4331054/diff/4001/src/pkg/crypto/des/des_test.g...
src/pkg/crypto/des/des_test.go:19: CryptTest{
CryptTest{ -> {
everywhere

http://codereview.appspot.com/4331054/diff/4001/src/pkg/crypto/des/des_test.g...
src/pkg/crypto/des/des_test.go:22: []byte{0x8c, 0xa6, 0x4d, 0xe9, 0xc1, 0xb1,
0x23, 0xa7}},
move second } onto next line
everywhere

http://codereview.appspot.com/4331054/diff/4001/src/pkg/crypto/des/des_test.g...
src/pkg/crypto/des/des_test.go:1266: c := &DESCipher{tt.key, make([]uint64, 16)}
If possible use the public API.

http://codereview.appspot.com/4331054/diff/4001/src/pkg/crypto/des/des_test.g...
src/pkg/crypto/des/des_test.go:1278: for j, v := range result {
The texts are short enough that you should be able to use

if !bytes.Equal(result, tt.in) {
    t.Errorf("#%d: result=%x want %x", result, tt.in)
}

I shortened "weak key test %d" to "#%d" because the
test log will make clear that this is TestWeakKeys.

Same shortening applies to all the others.

http://codereview.appspot.com/4331054/diff/4001/src/pkg/crypto/des/des_test.g...
src/pkg/crypto/des/des_test.go:1291: c := &DESCipher{key, make([]uint64, 16)}
If possible use the public API.

c, _ := NewDESCipher(key)
out = make([]byte, len(in))
c.Encrypt(out, in)
return

http://codereview.appspot.com/4331054/diff/4001/src/pkg/crypto/des/des_test.g...
src/pkg/crypto/des/des_test.go:1315: c := &DESCipher{tt.key, make([]uint64, 16)}
c, _ := NewDESCipher(tt.key)

http://codereview.appspot.com/4331054/diff/4001/src/pkg/crypto/des/des_test.g...
src/pkg/crypto/des/des_test.go:1331: c := &DESCipher{tt.key, make([]uint64, 16)}
c, _ := NewDESCipher(tt.key)

http://codereview.appspot.com/4331054/diff/4001/src/pkg/crypto/des/des_test.g...
src/pkg/crypto/des/des_test.go:1347: cipher1, _ := NewDESCipher(tt.key[0:8])
Is this not just

c, _ := NewTDEACipher(tt.key)

?

Same below.

[mattn, 2011-04-05 06:24:49]

Russ, sorry.

It seems that block.go is not uploaded in previous time.

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

http://codereview.appspot.com/4331054/diff/4001/src/pkg/crypto/des/block.go#n...
src/pkg/crypto/des/block.go:13: func encryptBlock(subkeys []uint64, src, dst
[]byte) {
On 2011/04/05 05:49:41, rsc wrote:
> dst, src

Done.

[mattn, 2011-04-05 06:51:26]

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

http://codereview.appspot.com/4331054/diff/4001/src/pkg/crypto/des/block.go#n...
src/pkg/crypto/des/block.go:14: // perform initial permutation
On 2011/04/05 05:49:41, rsc wrote:
> There are many things you can do to
> shorten this function.  Parallel assignments,
> range statements, don't allocate by calling uint64ToBytes.
> Combining all of these you end up with something like:
> 
> func encryptBlock(subkeys []uint64, dst, src []byte) {
>     b := binary.BigEndian.Uint64(src)
>     b = permuteBlock(b, initialPermutation[:])
>     left, right := uint32(b>>32), uint32(b)
>     for _, key := range subkeys {
>         left, right = right, left ^ feistel(right, key)
>     }
>     b = permuteBlock(uint64(right)<<32 | uint64(left), finalPerm[:])
>     binary.BigEndian.PutUint64(dst, b)
> }
> 

Done.

http://codereview.appspot.com/4331054/diff/4001/src/pkg/crypto/des/block.go#n...
src/pkg/crypto/des/block.go:38: func decryptBlock(subkeys []uint64, src []byte,
dst []byte) {
On 2011/04/05 05:49:41, rsc wrote:
> dst, src

Done.

http://codereview.appspot.com/4331054/diff/4001/src/pkg/crypto/des/block.go#n...
src/pkg/crypto/des/block.go:39: // perform initial permutation
Please let me the code.
Sorry, I don't conversant about this issue.

On 2011/04/05 05:49:41, rsc wrote:
> All the cleanup of encryptBlock applies here too.

http://codereview.appspot.com/4331054/diff/4001/src/pkg/crypto/des/block.go#n...
src/pkg/crypto/des/block.go:82: func permuteBlock(src []byte, permutation
[]uint8) (block uint64) {
On 2011/04/05 05:49:41, rsc wrote:
> Operating on []byte here is forcing allocations
> that are quite expensive.  Just take a uint64.
> That makes the math easier anyway:
> 
> func permuteBlock(b uint64, perm []uint8) uint64 {
>     var c uint64
>     for i, bit := range perm {
>         c |= ((b>>(bit-1))&1) << uint(len(perm) - 1 - i)
>     }
> }

Done.

http://codereview.appspot.com/4331054/diff/4001/src/pkg/crypto/des/block.go#n...
src/pkg/crypto/des/block.go:100: part1 := (last << (4 + uint32(ksRotations[i])))
>> 4
On 2011/04/05 05:49:41, rsc wrote:
> The shift counts do not have to be the same type
> as the thing being shifted.  They just have to be unsigned.
> 
> part1 := (last << (4+ksRotations[i])) >> 4

Done.

http://codereview.appspot.com/4331054/diff/4001/src/pkg/crypto/des/block.go#n...
src/pkg/crypto/des/block.go:127: func uint32ToBytes(block uint32) []byte {
it seems this function is used in feistel.

[agl1, 2011-04-05 13:51:15]

Stopping at this point because it appears the rsc said many of the same things.

(Note: we appeared to review this to the point that the last author gave up. I
don't want to do that again so I'm happy to have you commit this and then I'll
do a pass, in tree, myself.)


Cheers

http://codereview.appspot.com/4331054/diff/5007/src/pkg/crypto/des/Makefile
File src/pkg/crypto/des/Makefile (right):

http://codereview.appspot.com/4331054/diff/5007/src/pkg/crypto/des/Makefile#n...
src/pkg/crypto/des/Makefile:1: # Copyright 2010 The Go Authors. All rights
reserved.
2011

http://codereview.appspot.com/4331054/diff/5007/src/pkg/crypto/des/block.go
File src/pkg/crypto/des/block.go (right):

http://codereview.appspot.com/4331054/diff/5007/src/pkg/crypto/des/block.go#n...
src/pkg/crypto/des/block.go:1: // Copyright 2010 The Go Authors. All rights
reserved.
2011

http://codereview.appspot.com/4331054/diff/5007/src/pkg/crypto/des/block.go#n...
src/pkg/crypto/des/block.go:8: func encryptBlock(subkeys [16]uint64, dst, src
[]byte) {
this is going to copy subkeys for every call I think. []uint64 might speed
things up.

http://codereview.appspot.com/4331054/diff/5007/src/pkg/crypto/des/block.go#n...
src/pkg/crypto/des/block.go:19: right = left ^ feistel(right, subkeys[i])
left, right = right, left ^ feistel(right, subkeys[i])

http://codereview.appspot.com/4331054/diff/5007/src/pkg/crypto/des/block.go#n...
src/pkg/crypto/des/block.go:31: func decryptBlock(subkeys [16]uint64, dst, src
[]byte) {
ditto with the copying.

http://codereview.appspot.com/4331054/diff/5007/src/pkg/crypto/des/block.go#n...
src/pkg/crypto/des/block.go:43: right = left ^ feistel(right, subkeys[15-i])
ditto with the parallel assignment.

[rsc, 2011-04-05 14:40:42]

Okay, let's leave this for Adam.

Russ

[agl1, 2011-04-05 14:59:44]

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

crypto/des: new package providing implementations of DES and TDEA
Original code by Chris Lennert <cale...@gmail.com>

R=rsc, agl1
CC=golang-dev
http://codereview.appspot.com/4331054

Committer: Adam Langley <agl@golang.org>

[agl1, 2011-04-05 15:00:22]

On Tue, Apr 5, 2011 at 10:40 AM, Russ Cox <rsc@golang.org> wrote:
> Okay, let's leave this for Adam.

I've removed the addition of the package to the top-level Makefile and
submitted. I'll do a cleanup CL in the next couple of days.


Cheers

AGL