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

crypto/des: new package providing implementations of DES and TDEA

[calennert, 2010-12-17 00:01:09]

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

I'd like you to review this change.

[r2, 2010-12-17 05:23:37]

Did you discuss the design and goals of this package ahead of time, as
contribute.html asks for major new packages?  I didn't see a
discussion about it.  Or is this unsolicited?

-rob

[peterGo, 2010-12-17 05:37:31]

DES and TDEA implementations
http://groups.google.com/group/golang-nuts/browse_thread/thread/17c233032211f136

On 2010/12/17 05:23:37, r2 wrote:
> Did you discuss the design and goals of this package ahead of time, as
> contribute.html asks for major new packages?  I didn't see a
> discussion about it.  Or is this unsolicited?
> 
> -rob

[r2, 2010-12-17 05:40:07]

Cool. Thanks for the background - that was quite a while ago.

-rob

[agl1, 2010-12-17 16:03:53]

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

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)
I don't think the shifting is needed here. The cast will truncate.

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++ {
There is copy() for this.

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)
ditto about the shift

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++ {
ditto about copy()

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:])
you don't need the '0' in [0:]

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++ {
i := 0

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
please wrap comment.

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
:= for these

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
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)
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/3643043/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/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
Since this is always 16 elements, it could be an array.

http://codereview.appspot.com/3643043/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/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) }
dst should be the first argument.

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) }
dst should be the first argument.

http://codereview.appspot.com/3643043/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/3643043/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/3643043/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/3643043/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/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{
I don't think that the "[4][16]uint8" is needed.

[rsc, 2011-01-04 17:06:58]

waiting to review until agl's comments are addressed

[B-Ranger, 2011-01-09 20:42:19]

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:69: for ; i < 8; i++ {
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

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:43: // It is necessary to satisfy the Block
interface in the
please delete the "necessary" comment

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{
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: 
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-01-19 19:15:41]

calennert - are you planning to get back to this?

[calennert, 2011-01-20 02:13:39]

On 2011/01/19 19:15:41, rsc wrote:
> calennert - are you planning to get back to this?

My apologies for the delay.  Yes, I'm working on the changes and should have it
submitted soon.

[rsc, 2011-04-05 02:37:27]

On Mon, Apr 4, 2011 at 19:49, mattn <mattn.jp@gmail.com> wrote:
> I'm still waiting this changes.

If you would like to make a copy of this CL and clean
it up following the comments that have been made
already, please do.  It has been nearly 4 months since
the CL was originally sent in.

Please put

    Original code by Chris Lennert <calennert@gmail.com>

in the CL description.

Russ

[mattn, 2011-04-05 04:20:16]

Fix of above is following new CL.

http://codereview.appspot.com/4331054/

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.

[calennert, 2011-05-05 16:34:46]

*** Abandoned ***