Add ChaCha20Poly1305Encrypter, based on

Add ChaCha20Poly1305Encrypter, based on
draft-agl-tls-chacha20poly1305-04 and identified by the tag CC12.
        
Add the base class AeadBaseEncrypter for Aes128Gcm12Encrypter and
ChaCha20Poly1305Encrypter. Both subclasses can be defined by
configuring the base class with four parameters.

Merge internal CL: 62224774

Add ChaCha20Poly1305Decrypter, based on
draft-agl-tls-chacha20poly1305-04 and identified by the tag CC12.
This continues the previous CL that added ChaCha20Poly1305Encrypter.

Add the base class AeadBaseDecrypter for Aes128Gcm12Decrypter and
ChaCha20Poly1305Decrypter. Both subclasses can be defined by
configuring the base class with four parameters.

Merge internal CL: 62637549

Note: The chacha20_poly1305_*_nss.cc files will be implemented in
a follow-up CL. Six files created with "svn copy" were added
separately in https://codereview.chromium.org/196743005/ (r256650).

R=agl@chromium.org,rtenneti@chromium.org
BUG=none
TEST=net_unittests

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=256837

[wtc, 2014-03-08 01:08:51]

Sorry about the big CL. Hopefully it's straightforward to
review.

Adam, you've reviewed the internal CLs. You can focus on
the _nss.cc files.

Thanks.

[ramant (doing other things), 2014-03-10 14:36:55]

lgtm

https://codereview.chromium.org/189893002/diff/50001/net/quic/crypto/aes_128_...
File net/quic/crypto/aes_128_gcm_12_decrypter.h (right):

https://codereview.chromium.org/189893002/diff/50001/net/quic/crypto/aes_128_...
net/quic/crypto/aes_128_gcm_12_decrypter.h:36: 
nit: Blank line at the end of a code block.

[agl, 2014-03-10 15:08:32]

LGTM

https://codereview.chromium.org/189893002/diff/70001/net/quic/crypto/aead_bas...
File net/quic/crypto/aead_base_decrypter.h (right):

https://codereview.chromium.org/189893002/diff/70001/net/quic/crypto/aead_bas...
net/quic/crypto/aead_base_decrypter.h:61: #if !defined(USE_NSS)
Is USE_NSS really USE_SYSTEM_NSS? This reads oddly!

https://codereview.chromium.org/189893002/diff/70001/net/quic/crypto/aead_bas...
File net/quic/crypto/aead_base_decrypter_nss.cc (right):

https://codereview.chromium.org/189893002/diff/70001/net/quic/crypto/aead_bas...
net/quic/crypto/aead_base_decrypter_nss.cc:101: const CK_GCM_PARAMS* gcm_params
=
This is GCM specific code in the AEAD base class? Obviously that's wrong but I
think I see why it's needed. Seems worth commenting about.

https://codereview.chromium.org/189893002/diff/70001/net/quic/crypto/chacha20...
File net/quic/crypto/chacha20_poly1305_encrypter_openssl.cc (right):

https://codereview.chromium.org/189893002/diff/70001/net/quic/crypto/chacha20...
net/quic/crypto/chacha20_poly1305_encrypter_openssl.cc:1: // Copyright (c) 2013
The Chromium Authors. All rights reserved.
Note that the copyright year for the new files is all 2013.

https://codereview.chromium.org/189893002/diff/70001/net/quic/crypto/chacha20...
File net/quic/crypto/chacha20_poly1305_encrypter_test.cc (right):

https://codereview.chromium.org/189893002/diff/70001/net/quic/crypto/chacha20...
net/quic/crypto/chacha20_poly1305_encrypter_test.cc:27: {
"4290bcb154173531f314af57f3be3b5006da371ece272afa1b5dbdd110"
formatting. There's no comma at the end of the line so perhaps the next line
should be indented?

[wtc, 2014-03-11 04:02:01]

Thanks for the review. Please review patch set 6.

You can just review the diffs between patch sets 5 and 6.

https://codereview.chromium.org/189893002/diff/50001/net/quic/crypto/aes_128_...
File net/quic/crypto/aes_128_gcm_12_decrypter.h (right):

https://codereview.chromium.org/189893002/diff/50001/net/quic/crypto/aes_128_...
net/quic/crypto/aes_128_gcm_12_decrypter.h:36: 
On 2014/03/10 14:36:55, ramant wrote:
> nit: Blank line at the end of a code block.

Done.

https://codereview.chromium.org/189893002/diff/70001/net/quic/crypto/aead_bas...
File net/quic/crypto/aead_base_decrypter.h (right):

https://codereview.chromium.org/189893002/diff/70001/net/quic/crypto/aead_bas...
net/quic/crypto/aead_base_decrypter.h:61: #if !defined(USE_NSS)

On 2014/03/10 15:08:33, agl wrote:
> Is USE_NSS really USE_SYSTEM_NSS?

Yes. The meaning of USE_NSS changed over time. Right now USE_NSS means we're
using system NSS. I added a comment to clarify this:

  // USE_NSS means we are using system NSS rather than our copy of NSS.

https://codereview.chromium.org/189893002/diff/70001/net/quic/crypto/aead_bas...
File net/quic/crypto/aead_base_decrypter_nss.cc (right):

https://codereview.chromium.org/189893002/diff/70001/net/quic/crypto/aead_bas...
net/quic/crypto/aead_base_decrypter_nss.cc:101: const CK_GCM_PARAMS* gcm_params
=

On 2014/03/10 15:08:33, agl wrote:
> This is GCM specific code in the AEAD base class? Obviously that's wrong but I
> think I see why it's needed. Seems worth commenting about.

OK, I figured out a way to keep the GCM-specific code in the GCM subclass. This
requires each subclass to pass a PK11_Encrypt/PK11_Decrypt function pointer to
the AEAD base class. I hope you like this solution.

https://codereview.chromium.org/189893002/diff/70001/net/quic/crypto/chacha20...
File net/quic/crypto/chacha20_poly1305_encrypter_openssl.cc (right):

https://codereview.chromium.org/189893002/diff/70001/net/quic/crypto/chacha20...
net/quic/crypto/chacha20_poly1305_encrypter_openssl.cc:1: // Copyright (c) 2013
The Chromium Authors. All rights reserved.

On 2014/03/10 15:08:33, agl wrote:
> Note that the copyright year for the new files is all 2013.

I should have annotated this. I noticed that the commit bot automatically fixes
this line when it adds a new file, including whether "(c)" should be used. So I
know I don't need to get this right.

https://codereview.chromium.org/189893002/diff/70001/net/quic/crypto/chacha20...
File net/quic/crypto/chacha20_poly1305_encrypter_test.cc (right):

https://codereview.chromium.org/189893002/diff/70001/net/quic/crypto/chacha20...
net/quic/crypto/chacha20_poly1305_encrypter_test.cc:27: {
"4290bcb154173531f314af57f3be3b5006da371ece272afa1b5dbdd110"
On 2014/03/10 15:08:33, agl wrote:
> formatting. There's no comma at the end of the line so perhaps the next line
> should be indented?

Done.

https://codereview.chromium.org/189893002/diff/90001/net/quic/crypto/aead_bas...
File net/quic/crypto/aead_base_decrypter_nss.cc (right):

https://codereview.chromium.org/189893002/diff/90001/net/quic/crypto/aead_bas...
net/quic/crypto/aead_base_decrypter_nss.cc:80: }

I just wanted to point out that the AEAD base class still has this tiny bit of
GCM-specific code. It is possible to move this back to the Aes128Gcm12Decrypter
subclass, at the cost of adding another argument to the AEAD base class
constructor.

[wtc, 2014-03-11 04:21:11]

https://codereview.chromium.org/189893002/diff/90001/net/quic/crypto/aead_bas...
File net/quic/crypto/aead_base_decrypter.h (right):

https://codereview.chromium.org/189893002/diff/90001/net/quic/crypto/aead_bas...
net/quic/crypto/aead_base_decrypter.h:84: const EVP_AEAD* aead_alg_;

Note to self: make this member const:

  const EVP_AEAD* const aead_alg_;

https://codereview.chromium.org/189893002/diff/90001/net/quic/crypto/aead_bas...
net/quic/crypto/aead_base_decrypter.h:86: CK_MECHANISM_TYPE aead_mechanism_;

Note to self: make this member const.

https://codereview.chromium.org/189893002/diff/90001/net/quic/crypto/aead_bas...
File net/quic/crypto/aead_base_encrypter.h (right):

https://codereview.chromium.org/189893002/diff/90001/net/quic/crypto/aead_bas...
net/quic/crypto/aead_base_encrypter.h:87: const EVP_AEAD* aead_alg_;

Note to self: make this member const:

  const EVP_AEAD* const aead_alg_;

[wtc, 2014-03-11 15:53:50]

https://codereview.chromium.org/189893002/diff/90001/net/quic/crypto/aead_bas...
File net/quic/crypto/aead_base_decrypter.h (right):

https://codereview.chromium.org/189893002/diff/90001/net/quic/crypto/aead_bas...
net/quic/crypto/aead_base_decrypter.h:87: const PK11_DecryptFunction
pk11_decrypt_;

I just realized that pk11_decrypt_ is essentially a virtual function that I want
a subclass to override. Sigh.

If you prefer that I do this properly, please let me know, and don't review this
patch set.

[agl, 2014-03-11 17:08:10]

lgtm

I didn't mean for you to move the GCM specific code from the base class - just
to add a function explaining it. Since we're moving away from NSS in general, I
think this fine.

[wtc, 2014-03-13 12:54:43]

The CQ bit was checked by wtc@chromium.org

[commit-bot: I haz the power, 2014-03-13 12:55:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wtc@chromium.org/189893002/110002

[commit-bot: I haz the power, 2014-03-13 15:23:56]

Change committed as 256837