code review 56530043: crypto/galois: Alternative GCM/GMAC implementation.

crypto/galois: Alternative GCM/GMAC implementation.

Replaces NewGCM in crypto/cipher, but is not backwards compatible.

Includes:
        - AES-NI support for modern processors.
        - Support for variable length nonces.
        - Support for variable length tags.
        - Streaming support.

[lcurley, 2014-01-27 08:23:18]

https://codereview.appspot.com/56530043/diff/120001/src/pkg/crypto/galois/has...
File src/pkg/crypto/galois/hash.go (right):

https://codereview.appspot.com/56530043/diff/120001/src/pkg/crypto/galois/has...
src/pkg/crypto/galois/hash.go:107: func (h *hash) Reset() {
h.extra must also be reset.

[lcurley, 2014-01-27 08:25:36]

Hello golang-codereviews@googlegroups.com,

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

[hanwen-google, 2014-01-27 08:57:51]

Couldnt the old API be implemented in terms of this one? The compatibility
promise precludes removing API parts (even though they may seem scarcely used).
Also, leaving Verify() to the discretion of the user is asking for problems.

https://codereview.appspot.com/56530043/diff/140001/src/pkg/crypto/galois/cip...
File src/pkg/crypto/galois/cipher.go (right):

https://codereview.appspot.com/56530043/diff/140001/src/pkg/crypto/galois/cip...
src/pkg/crypto/galois/cipher.go:210: func xorWords(dst, x, y []byte) {
you can get nice speed-up if you xor in 32 or 64 bit chunks. See
crypto/cipher/xor*.go

https://codereview.appspot.com/56530043/diff/140001/src/pkg/crypto/galois/gin...
File src/pkg/crypto/galois/gint128_amd64.s (right):

https://codereview.appspot.com/56530043/diff/140001/src/pkg/crypto/galois/gin...
src/pkg/crypto/galois/gint128_amd64.s:23: //
http://download-software.intel.com/sites/default/files/article/165685/clmul-w...
I had a look at this doc, but it says

NO LICENSE, EXPRESS OR IMPLIED, BY ESTOPPEL OR OTHERWISE, TO ANY INTELLECTUAL
PROPERTY RIGHTS IS GRANTED BY THIS DOCUMENT

so I'm not sure if it is OK to directly derive from code in this doc.

[minux1, 2014-01-27 08:58:57]

this change break Go 1 API contract by removing the NewGCM API
from crypto/cipher, so it's a no go.

You can change the implementation, but can't do any backward
incompatible change (NewGCM is released, so it's part of the
contract now).

Have you discussed the changes on the mailing lists?

[lcurley, 2014-01-27 09:16:02]

I sent out a message to golang-dev at the same time as publishing this review.

Unfortunately, I had no idea about the compatibility guarantee. The streaming
and variable-length tag support cannot be worked into the old API. The problem
with the variable-length tag is that it is appended to the end of the
variable-length ciphertext, making it impossible to differentiate the two.

I'm going to ask how to proceed on the mailing list.

https://codereview.appspot.com/56530043/diff/140001/src/pkg/crypto/galois/cip...
File src/pkg/crypto/galois/cipher.go (right):

https://codereview.appspot.com/56530043/diff/140001/src/pkg/crypto/galois/cip...
src/pkg/crypto/galois/cipher.go:210: func xorWords(dst, x, y []byte) {
On 2014/01/27 08:57:52, hanwen-google wrote:
> you can get nice speed-up if you xor in 32 or 64 bit chunks. See
> crypto/cipher/xor*.go

I originally was using fastXorWords but benchmarks showed no impact. xorWords is
only called once for Sum so it's not worth the effort.

https://codereview.appspot.com/56530043/diff/140001/src/pkg/crypto/galois/gin...
File src/pkg/crypto/galois/gint128_amd64.s (right):

https://codereview.appspot.com/56530043/diff/140001/src/pkg/crypto/galois/gin...
src/pkg/crypto/galois/gint128_amd64.s:23: //
http://download-software.intel.com/sites/default/files/article/165685/clmul-w...
On 2014/01/27 08:57:52, hanwen-google wrote:
> I had a look at this doc, but it says
> 
> NO LICENSE, EXPRESS OR IMPLIED, BY ESTOPPEL OR OTHERWISE, TO ANY INTELLECTUAL
> PROPERTY RIGHTS IS GRANTED BY THIS DOCUMENT
> 
> so I'm not sure if it is OK to directly derive from code in this doc.

Thanks for bringing this up. I have no idea how open source projects deal with
licensing issues like this one and wanted to leave it to the experts.

[lcurley, 2014-02-03 06:07:53]

My fault about the backwards-compatibility thing. I'm going to reimplement the
Intel assembly code and avoid changing the API.