code review 157152: Added XTEA block cipher package to src/pkg/crypto

Added XTEA block cipher package to src/pkg/crypto

This is an adaption of the code from http://en.wikipedia.org/wiki/XTEA. The package also implements the block.Cipher
interface so that it can be used with the various block modes.

[midpoint, 2009-11-24 10:00:44]

Hello rsc,

I'd like you to review the following change.

[rsc, 2009-11-30 22:48:07]

Thanks.  Sorry for the delay in getting
back to you - I was away last week.

http://codereview.appspot.com/157152/diff/12/14
File src/pkg/crypto/xtea/Makefile (right):

http://codereview.appspot.com/157152/diff/12/14#newcode5
src/pkg/crypto/xtea/Makefile:5: include $(GOROOT)/src/Make.$(GOARCH)
Please change to 
include ../../../Make.$(GOARCH)

and similarly, below,
include ../../../Make.pkg

we changed the convention recently to accommodate
$(GOROOT) that have spaces in them.

http://codereview.appspot.com/157152/diff/12/15
File src/pkg/crypto/xtea/block.go (right):

http://codereview.appspot.com/157152/diff/12/15#newcode6
src/pkg/crypto/xtea/block.go:6: Implementation adapted from:
A better reference would be the original paper
(see comment in other file).

http://codereview.appspot.com/157152/diff/12/15#newcode15
src/pkg/crypto/xtea/block.go:15: const numRounds = 32
I'd suggest making this 64 and then the loops below
can use numRounds instead of 2*numRounds.

http://codereview.appspot.com/157152/diff/12/15#newcode17
src/pkg/crypto/xtea/block.go:17: // Initialise the cipher context
This probably belongs in the other file.
If an assembly implementation replaced
this file, it wouldn't need to implement
initCipher, which is not speed critical.

Doc comments should be complete sentences.

// initCipher initializes the cipher context.

http://codereview.appspot.com/157152/diff/12/15#newcode38
src/pkg/crypto/xtea/block.go:38: // Read a block into two uint32
// blockToUint32 convers a block into two uint32 values.
// ...

(complete sentences; applies to the rest of the file too)

http://codereview.appspot.com/157152/diff/12/15#newcode49
src/pkg/crypto/xtea/block.go:49: dst[0] = byte((v0 >> 24) & 0xFF);
All the & 0xFF can be dropped, and then the ( ) around the >> too.

dst[0] = byte(v0 >> 24)
etc

http://codereview.appspot.com/157152/diff/12/15#newcode78
src/pkg/crypto/xtea/block.go:78: var ti uint = 63;	// We work backwards through
the table when decrypting. This and a loop variable are used for clarity.
Sure seems like it would be equally clear to say

// Two rounds of XTEA applied per loop.
for i := 2*numRounds; i > 0; {
    i--;
    v1 -= ...
    i--;
    v0 -= ...
}

Same comment above, about encryptBlock.

http://codereview.appspot.com/157152/diff/12/15#newcode82
src/pkg/crypto/xtea/block.go:82: v1 -= (((v0 << 4) ^ (v0 >> 5)) + v0) ^
c.table[ti];
I suggest dropping the parens around the << and >>:

v1 -= ((v0<<4 ^ v0>>5) + v0) ^ c.table[ti];

and the same below.
it helps the readability to drop parens
that aren't necessary.
(actually none of the parens are necessary,
but the others seem to help more.)

http://codereview.appspot.com/157152/diff/12/16
File src/pkg/crypto/xtea/cipher.go (right):

http://codereview.appspot.com/157152/diff/12/16#newcode5
src/pkg/crypto/xtea/cipher.go:5: package xtea
There should be a doc comment here like

// This package implements XTEA encryption, as defined
// in Needham and Wheeler's 1997 technical report, ``Tea extensions.''
package xtea

// For details, see http://www.cix.co.uk/~klockstone/xtea.pdf

http://codereview.appspot.com/157152/diff/12/16#newcode28
src/pkg/crypto/xtea/cipher.go:28: // The key argument should be the XTEA key,
s/,/./

http://codereview.appspot.com/157152/diff/12/16#newcode29
src/pkg/crypto/xtea/cipher.go:29: // Only 128 bit (16 byte) keys are supported.
is it the implementation or the algorithm's limitation?

// The implementation only supports 16-byte keys.

(or 

// XTEA only supports 16-byte keys.

)

http://codereview.appspot.com/157152/diff/12/16#newcode45
src/pkg/crypto/xtea/cipher.go:45: // BlockSize returns the XTEA block size, 16
bytes.
s/16/8/

[midpoint, 2009-12-07 12:54:41]

No worries about the delay, I've also been off for a bit.

Could you take another look please?

Thanks,

http://codereview.appspot.com/157152/diff/12/14
File src/pkg/crypto/xtea/Makefile (right):

http://codereview.appspot.com/157152/diff/12/14#newcode5
src/pkg/crypto/xtea/Makefile:5: include $(GOROOT)/src/Make.$(GOARCH)
On 2009/11/30 22:48:07, rsc wrote:
> Please change to 
> include ../../../Make.$(GOARCH)
> 
> and similarly, below,
> include ../../../Make.pkg
> 
> we changed the convention recently to accommodate
> $(GOROOT) that have spaces in them.

Done.

http://codereview.appspot.com/157152/diff/12/15
File src/pkg/crypto/xtea/block.go (right):

http://codereview.appspot.com/157152/diff/12/15#newcode6
src/pkg/crypto/xtea/block.go:6: Implementation adapted from:
On 2009/11/30 22:48:07, rsc wrote:
> A better reference would be the original paper
> (see comment in other file).
> 

Done.

http://codereview.appspot.com/157152/diff/12/15#newcode15
src/pkg/crypto/xtea/block.go:15: const numRounds = 32
On 2009/11/30 22:48:07, rsc wrote:
> I'd suggest making this 64 and then the loops below
> can use numRounds instead of 2*numRounds.
> 

Done.

http://codereview.appspot.com/157152/diff/12/15#newcode17
src/pkg/crypto/xtea/block.go:17: // Initialise the cipher context
On 2009/11/30 22:48:07, rsc wrote:
> This probably belongs in the other file.
> If an assembly implementation replaced
> this file, it wouldn't need to implement
> initCipher, which is not speed critical.
> 
> Doc comments should be complete sentences.
> 
> // initCipher initializes the cipher context.
> 

Done.

http://codereview.appspot.com/157152/diff/12/15#newcode38
src/pkg/crypto/xtea/block.go:38: // Read a block into two uint32
On 2009/11/30 22:48:07, rsc wrote:
> // blockToUint32 convers a block into two uint32 values.
> // ...
> 
> (complete sentences; applies to the rest of the file too)
> 

Done.

http://codereview.appspot.com/157152/diff/12/15#newcode49
src/pkg/crypto/xtea/block.go:49: dst[0] = byte((v0 >> 24) & 0xFF);
On 2009/11/30 22:48:07, rsc wrote:
> All the & 0xFF can be dropped, and then the ( ) around the >> too.
> 
> dst[0] = byte(v0 >> 24)
> etc
> 

Done.

http://codereview.appspot.com/157152/diff/12/15#newcode78
src/pkg/crypto/xtea/block.go:78: var ti uint = 63;	// We work backwards through
the table when decrypting. This and a loop variable are used for clarity.
On 2009/11/30 22:48:07, rsc wrote:
> Sure seems like it would be equally clear to say
> 
> // Two rounds of XTEA applied per loop.
> for i := 2*numRounds; i > 0; {
>     i--;
>     v1 -= ...
>     i--;
>     v0 -= ...
> }
> 
> Same comment above, about encryptBlock.
> 

Done.
And it certainly does look much better, this was the section of code I was most
worried about. Thanks :)

http://codereview.appspot.com/157152/diff/12/15#newcode82
src/pkg/crypto/xtea/block.go:82: v1 -= (((v0 << 4) ^ (v0 >> 5)) + v0) ^
c.table[ti];
On 2009/11/30 22:48:07, rsc wrote:
> I suggest dropping the parens around the << and >>:
> 
> v1 -= ((v0<<4 ^ v0>>5) + v0) ^ c.table[ti];
> 
> and the same below.
> it helps the readability to drop parens
> that aren't necessary.
> (actually none of the parens are necessary,
> but the others seem to help more.)
> 

I personally liked the brackets because it made the order of operations explicit
for the reader, but I'll stick to the accepted coding standard.
Done.

http://codereview.appspot.com/157152/diff/12/16
File src/pkg/crypto/xtea/cipher.go (right):

http://codereview.appspot.com/157152/diff/12/16#newcode5
src/pkg/crypto/xtea/cipher.go:5: package xtea
On 2009/11/30 22:48:07, rsc wrote:
> There should be a doc comment here like
> 
> // This package implements XTEA encryption, as defined
> // in Needham and Wheeler's 1997 technical report, ``Tea extensions.''
> package xtea
> 
> // For details, see http://www.cix.co.uk/%7Eklockstone/xtea.pdf
> 
> 

Done.

http://codereview.appspot.com/157152/diff/12/16#newcode28
src/pkg/crypto/xtea/cipher.go:28: // The key argument should be the XTEA key,
On 2009/11/30 22:48:07, rsc wrote:
> s/,/./
> 

Done. (sorry)

http://codereview.appspot.com/157152/diff/12/16#newcode29
src/pkg/crypto/xtea/cipher.go:29: // Only 128 bit (16 byte) keys are supported.
On 2009/11/30 22:48:07, rsc wrote:
> is it the implementation or the algorithm's limitation?
> 
> // The implementation only supports 16-byte keys.
> 
> (or 
> 
> // XTEA only supports 16-byte keys.
> 
> )

It's the algorithm's limitation so I've updated the comment as per your
recommendation.
Done.

http://codereview.appspot.com/157152/diff/12/16#newcode45
src/pkg/crypto/xtea/cipher.go:45: // BlockSize returns the XTEA block size, 16
bytes.
On 2009/11/30 22:48:07, rsc wrote:
> s/16/8/
> 

Done. (oops!)

[rsc, 2009-12-07 18:35:56]

could you please complete the appropriate CLA
as described at http://golang.org/doc/contribute.html#copyright ?
don't bother editing CONTRIBUTORS and AUTHORS - we'll
take care of that - but do let me know once you've filled
out the form.

thanks.
russ

[rsc, 2009-12-07 18:40:22]

just the comment; everything else looks fine.
(waiting on the CLA too.)

http://codereview.appspot.com/157152/diff/4001/4005
File src/pkg/crypto/xtea/cipher.go (right):

http://codereview.appspot.com/157152/diff/4001/4005#newcode7
src/pkg/crypto/xtea/cipher.go:7: // This package implements XTEA encryption, as
defined in Needham and
the formatting got jumbled here.
should be like this:

// This package implements XTEA encryption, as defined
// in Needham and Wheeler's 1996 technical report
// "Tea extensions."
package xtea

// For details, see http://...

the part before the package line is shown by godoc.

[midpoint, 2009-12-07 19:29:32]

I've submitted my CLA online, hopefully that will come through soon.

Thanks,

http://codereview.appspot.com/157152/diff/4001/4005
File src/pkg/crypto/xtea/cipher.go (right):

http://codereview.appspot.com/157152/diff/4001/4005#newcode7
src/pkg/crypto/xtea/cipher.go:7: // This package implements XTEA encryption, as
defined in Needham and
On 2009/12/07 18:40:22, rsc wrote:
> the formatting got jumbled here.
> should be like this:
> 
> // This package implements XTEA encryption, as defined
> // in Needham and Wheeler's 1996 technical report
> // "Tea extensions."
> package xtea
> 
> // For details, see http://...
> 
> the part before the package line is shown by godoc.
> 

Done.

[rsc, 2009-12-07 19:42:57]

so close.

i found your CLA.  i was searching
for "midpoint" instead of your email
address.

http://codereview.appspot.com/157152/diff/4014/5010
File src/pkg/crypto/xtea/cipher.go (right):

http://codereview.appspot.com/157152/diff/4014/5010#newcode6
src/pkg/crypto/xtea/cipher.go:6: // Wheeler's 1997 technical report, "Tea
extensions." package xtea.
delete "package xtea."
and hg upload

[midpoint, 2009-12-07 20:02:02]

I'm really sorry about that, glad I picked something simple to learn this
process.

http://codereview.appspot.com/157152/diff/4014/5010
File src/pkg/crypto/xtea/cipher.go (right):

http://codereview.appspot.com/157152/diff/4014/5010#newcode6
src/pkg/crypto/xtea/cipher.go:6: // Wheeler's 1997 technical report, "Tea
extensions." package xtea.
On 2009/12/07 19:42:57, rsc wrote:
> delete "package xtea."
> and hg upload 
> 
> 

Done.

[rsc, 2009-12-09 08:05:56]

LGTM

[rsc, 2009-12-09 08:06:24]

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

Added XTEA block cipher package to src/pkg/crypto

This is an adaption of the code from http://en.wikipedia.org/wiki/XTEA. The
package also implements the block.Cipher
interface so that it can be used with the various block modes.

R=rsc
http://codereview.appspot.com/157152

Committer: Russ Cox <rsc@golang.org>