QUIC - Some sketching of the crypto handshake.

QUIC - Some sketching of the crypto handshake.

Merge internal CL: 42490294

R=wtc@chromium.org, agl@chromium.org

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

[ramant (doing other things), 2013-02-28 19:58:41]

- Did a #if 0 around the calls to crypto_scalarmult_curve25519*. Will enable
that code after landing curve25519-donna.* code.

- Will add a separate CL to delete QuicCryptoServerConfig. Added the
minimalchanges needed for QuicCryptoServerConfig to land CL 42490294.

- Will land a separate CL to separate out QuicCryptoServerConfig and
QuicCryptoClientConfig files (as suggested by rch@) and then delete
QuicCryptoServerConfig changes from chrome.

Would appreciate if you could take a look.

[agl, 2013-02-28 21:26:56]

https://chromiumcodereview.appspot.com/12381018/diff/1/net/quic/crypto/crypto...
File net/quic/crypto/crypto_handshake.h (right):

https://chromiumcodereview.appspot.com/12381018/diff/1/net/quic/crypto/crypto...
net/quic/crypto/crypto_handshake.h:22: class NET_EXPORT_PRIVATE
QuicCryptoClientConfig {
I'm not sure why this differs so markedly from the server-side code.
QuicCryptoConfig has disappeared, extra members (e.g. keepalive_timeout) have
been added etc.

I was hoping that copying the source across would almost work. Did I fail?

[Ryan Hamilton, 2013-02-28 21:45:55]

On 2013/02/28 21:26:56, agl wrote:
>
https://chromiumcodereview.appspot.com/12381018/diff/1/net/quic/crypto/crypto...
> File net/quic/crypto/crypto_handshake.h (right):
> 
>
https://chromiumcodereview.appspot.com/12381018/diff/1/net/quic/crypto/crypto...
> net/quic/crypto/crypto_handshake.h:22: class NET_EXPORT_PRIVATE
> QuicCryptoClientConfig {
> I'm not sure why this differs so markedly from the server-side code.
> QuicCryptoConfig has disappeared, extra members (e.g. keepalive_timeout) have
> been added etc.
> 
> I was hoping that copying the source across would almost work. Did I fail?

I suspect the issue is that this is a merge of just a single CL.  It looks like
at least two others have landed subsequently.  Are you comparing this CL to the
internal CL 42490294, or to ToT?

[agl, 2013-02-28 21:59:28]

Ah, Mr Ryan is right again. I had the current code in mind, not the change that
this is porting.

On the basis that this is mostly a code move, I haven't reviewed in detail. The
bits where TODOs have been added seem reasonable to me. So LGTM from me.

[wtc, 2013-03-01 19:07:58]

Patch set 1 LGTM.

https://chromiumcodereview.appspot.com/12381018/diff/1/net/net.gyp
File net/net.gyp (right):

https://chromiumcodereview.appspot.com/12381018/diff/1/net/net.gyp#newcode712
net/net.gyp:712: 'quic/crypto/key_exchange.h',

These files need to be listed in alphabetical order.

Remove the duplicate entries for curve25519_key_exchange.cc
and curve25519_key_exchange.h.

https://chromiumcodereview.appspot.com/12381018/diff/1/net/quic/crypto/crypto...
File net/quic/crypto/crypto_handshake.cc (right):

https://chromiumcodereview.appspot.com/12381018/diff/1/net/quic/crypto/crypto...
net/quic/crypto/crypto_handshake.cc:11: #include
"net/quic/crypto/curve25519_key_exchange.h"

Remove these two headers. They are currently only used by the
QuicCryptoServerConfig that you omitted.

I think more headers, such as secure_hash.h, can be removed.

https://chromiumcodereview.appspot.com/12381018/diff/1/net/quic/crypto/crypto...
net/quic/crypto/crypto_handshake.cc:19: using crypto::SecureHash;

This can be removed.

https://chromiumcodereview.appspot.com/12381018/diff/1/net/quic/crypto/crypto...
net/quic/crypto/crypto_handshake.cc:86: }

Do we need this? This is not in the internal CL.

https://chromiumcodereview.appspot.com/12381018/diff/1/net/quic/crypto/crypto...
net/quic/crypto/crypto_handshake.cc:111: }

This is not in the internal CL.

https://chromiumcodereview.appspot.com/12381018/diff/1/net/quic/crypto/crypto...
File net/quic/crypto/crypto_handshake_test.cc (right):

https://chromiumcodereview.appspot.com/12381018/diff/1/net/quic/crypto/crypto...
net/quic/crypto/crypto_handshake_test.cc:8: #include
"net/quic/test_tools/mock_random.h"

We should include "net/quic/crypto/quic_random.h".
(List it in alphabetical order.)

https://chromiumcodereview.appspot.com/12381018/diff/1/net/quic/crypto/curve2...
File net/quic/crypto/curve25519_key_exchange.cc (right):

https://chromiumcodereview.appspot.com/12381018/diff/1/net/quic/crypto/curve2...
net/quic/crypto/curve25519_key_exchange.cc:32: 

Nit: delete this blank line. (This is not in the internal CL.)

https://chromiumcodereview.appspot.com/12381018/diff/1/net/quic/crypto/curve2...
net/quic/crypto/curve25519_key_exchange.cc:55: }

It is better to do this as follows:

Curve25519KeyExchange* Curve25519KeyExchange::New(
    const StringPiece& private_key) {
  // TODO(rtenneti): Add support for curve25519.
#if 0
  original code
#else
  return NULL;
#endif
}

Use the same approach for Curve25519KeyExchange::CalculateSharedKey().

https://chromiumcodereview.appspot.com/12381018/diff/1/net/quic/crypto/curve2...
File net/quic/crypto/curve25519_key_exchange.h (right):

https://chromiumcodereview.appspot.com/12381018/diff/1/net/quic/crypto/curve2...
net/quic/crypto/curve25519_key_exchange.h:8: #include <string.h>

Remove this header.

https://chromiumcodereview.appspot.com/12381018/diff/1/net/quic/crypto/curve2...
net/quic/crypto/curve25519_key_exchange.h:37: virtual CryptoTag tag() const
OVERRIDE;

The addition of "virtual" should be merged back to the
internal repository.

https://chromiumcodereview.appspot.com/12381018/diff/1/net/quic/crypto/key_ex...
File net/quic/crypto/key_exchange.h (right):

https://chromiumcodereview.appspot.com/12381018/diff/1/net/quic/crypto/key_ex...
net/quic/crypto/key_exchange.h:8: #include <string.h>

Remove this header.

https://chromiumcodereview.appspot.com/12381018/diff/1/net/quic/quic_crypto_c...
File net/quic/quic_crypto_client_stream.h (right):

https://chromiumcodereview.appspot.com/12381018/diff/1/net/quic/quic_crypto_c...
net/quic/quic_crypto_client_stream.h:10: #include
"net/quic/crypto/crypto_handshake.h"

The internal CL lists this header in the wrong order. Please
fix that in the internal repository.

[ramant (doing other things), 2013-03-01 22:02:04]

Thanks very much Wan-Teh. Made all the changes you have suggested,

https://chromiumcodereview.appspot.com/12381018/diff/1/net/net.gyp
File net/net.gyp (right):

https://chromiumcodereview.appspot.com/12381018/diff/1/net/net.gyp#newcode712
net/net.gyp:712: 'quic/crypto/key_exchange.h',
On 2013/03/01 19:07:58, wtc wrote:
> 
> These files need to be listed in alphabetical order.
> 
> Remove the duplicate entries for curve25519_key_exchange.cc
> and curve25519_key_exchange.h.

Done.

https://chromiumcodereview.appspot.com/12381018/diff/1/net/quic/crypto/crypto...
File net/quic/crypto/crypto_handshake.cc (right):

https://chromiumcodereview.appspot.com/12381018/diff/1/net/quic/crypto/crypto...
net/quic/crypto/crypto_handshake.cc:11: #include
"net/quic/crypto/curve25519_key_exchange.h"
On 2013/03/01 19:07:58, wtc wrote:
> 
> Remove these two headers. They are currently only used by the
> QuicCryptoServerConfig that you omitted.
> 
> I think more headers, such as secure_hash.h, can be removed.

Changed it to key_exchange.h and deleted secure_hash.h.

https://chromiumcodereview.appspot.com/12381018/diff/1/net/quic/crypto/crypto...
net/quic/crypto/crypto_handshake.cc:19: using crypto::SecureHash;
On 2013/03/01 19:07:58, wtc wrote:
> 
> This can be removed.

Done.

https://chromiumcodereview.appspot.com/12381018/diff/1/net/quic/crypto/crypto...
net/quic/crypto/crypto_handshake.cc:86: }
On 2013/03/01 19:07:58, wtc wrote:
> 
> Do we need this? This is not in the internal CL.

Added it for compiling it on other platforms. Will merge this change back into
server.

https://chromiumcodereview.appspot.com/12381018/diff/1/net/quic/crypto/crypto...
net/quic/crypto/crypto_handshake.cc:111: }
On 2013/03/01 19:07:58, wtc wrote:
> 
> This is not in the internal CL.

Added it for compiling it on other platforms. Will merge this change back into
server.

https://chromiumcodereview.appspot.com/12381018/diff/1/net/quic/crypto/crypto...
File net/quic/crypto/crypto_handshake.h (right):

https://chromiumcodereview.appspot.com/12381018/diff/1/net/quic/crypto/crypto...
net/quic/crypto/crypto_handshake.h:22: class NET_EXPORT_PRIVATE
QuicCryptoClientConfig {
On 2013/02/28 21:26:56, agl wrote:
> I'm not sure why this differs so markedly from the server-side code.
> QuicCryptoConfig has disappeared, extra members (e.g. keepalive_timeout) have
> been added etc.
> 
> I was hoping that copying the source across would almost work. Did I fail?

When we merge the next CL, chrome will catch up with server. As rch@ pointed
out, was merging each CL.

https://chromiumcodereview.appspot.com/12381018/diff/1/net/quic/crypto/crypto...
File net/quic/crypto/crypto_handshake_test.cc (right):

https://chromiumcodereview.appspot.com/12381018/diff/1/net/quic/crypto/crypto...
net/quic/crypto/crypto_handshake_test.cc:8: #include
"net/quic/test_tools/mock_random.h"
On 2013/03/01 19:07:58, wtc wrote:
> 
> We should include "net/quic/crypto/quic_random.h".
> (List it in alphabetical order.)

Done.

https://chromiumcodereview.appspot.com/12381018/diff/1/net/quic/crypto/curve2...
File net/quic/crypto/curve25519_key_exchange.cc (right):

https://chromiumcodereview.appspot.com/12381018/diff/1/net/quic/crypto/curve2...
net/quic/crypto/curve25519_key_exchange.cc:32: 
On 2013/03/01 19:07:58, wtc wrote:
> 
> Nit: delete this blank line. (This is not in the internal CL.)

Done.

https://chromiumcodereview.appspot.com/12381018/diff/1/net/quic/crypto/curve2...
net/quic/crypto/curve25519_key_exchange.cc:55: }
On 2013/03/01 19:07:58, wtc wrote:
> 
> It is better to do this as follows:
> 
> Curve25519KeyExchange* Curve25519KeyExchange::New(
>     const StringPiece& private_key) {
>   // TODO(rtenneti): Add support for curve25519.
> #if 0
>   original code
> #else
>   return NULL;
> #endif
> }
> 
> Use the same approach for Curve25519KeyExchange::CalculateSharedKey().

Done.

https://chromiumcodereview.appspot.com/12381018/diff/1/net/quic/crypto/curve2...
File net/quic/crypto/curve25519_key_exchange.h (right):

https://chromiumcodereview.appspot.com/12381018/diff/1/net/quic/crypto/curve2...
net/quic/crypto/curve25519_key_exchange.h:8: #include <string.h>
On 2013/03/01 19:07:58, wtc wrote:
> 
> Remove this header.

Changed it to #include <string>
Done.

https://chromiumcodereview.appspot.com/12381018/diff/1/net/quic/crypto/curve2...
net/quic/crypto/curve25519_key_exchange.h:37: virtual CryptoTag tag() const
OVERRIDE;
On 2013/03/01 19:07:58, wtc wrote:
> 
> The addition of "virtual" should be merged back to the
> internal repository.

Will do. thanks Wan-Teh.

https://chromiumcodereview.appspot.com/12381018/diff/1/net/quic/crypto/key_ex...
File net/quic/crypto/key_exchange.h (right):

https://chromiumcodereview.appspot.com/12381018/diff/1/net/quic/crypto/key_ex...
net/quic/crypto/key_exchange.h:8: #include <string.h>
On 2013/03/01 19:07:58, wtc wrote:
> 
> Remove this header.

Changed it to #include <string> to fix chrome's lint errors.

https://chromiumcodereview.appspot.com/12381018/diff/1/net/quic/quic_crypto_c...
File net/quic/quic_crypto_client_stream.h (right):

https://chromiumcodereview.appspot.com/12381018/diff/1/net/quic/quic_crypto_c...
net/quic/quic_crypto_client_stream.h:10: #include
"net/quic/crypto/crypto_handshake.h"
On 2013/03/01 19:07:58, wtc wrote:
> 
> The internal CL lists this header in the wrong order. Please
> fix that in the internal repository.

Will do asap.

[ramant (doing other things), 2013-03-01 22:03:31]

PTAL.

[wtc, 2013-03-01 23:45:23]

Patch set 2 LGTM. I suggest some small changes. Thanks!

https://codereview.chromium.org/12381018/diff/12007/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/12381018/diff/12007/net/net.gyp#newcode710
net/net.gyp:710: 'quic/crypto/key_exchange.h',

'quic/crypto/key_exchange.h' is still in the wrong place.

https://codereview.chromium.org/12381018/diff/12007/net/quic/crypto/crypto_ha...
File net/quic/crypto/crypto_handshake.cc (right):

https://codereview.chromium.org/12381018/diff/12007/net/quic/crypto/crypto_ha...
net/quic/crypto/crypto_handshake.cc:10: #include
"net/quic/crypto/key_exchange.h"

Are you sure we need crypto_framer.h and key_exchange.h?

https://codereview.chromium.org/12381018/diff/12007/net/quic/crypto/crypto_ha...
File net/quic/crypto/crypto_handshake_test.cc (right):

https://codereview.chromium.org/12381018/diff/12007/net/quic/crypto/crypto_ha...
net/quic/crypto/crypto_handshake_test.cc:16: TEST(QuicCryptoProtocol,
ServerConfig) {

You should delete this test now.

This means lines 7-8 should also be deleted. Just leave
crypto_handshake.h and the gmock and gtest headers for future
tests in this file.

[wtc, 2013-03-02 00:20:16]

Patch set 3 LGTM.

[ramant (doing other things), 2013-03-02 00:34:16]

PTAL.

https://codereview.chromium.org/12381018/diff/12007/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/12381018/diff/12007/net/net.gyp#newcode710
net/net.gyp:710: 'quic/crypto/key_exchange.h',
On 2013/03/01 23:45:23, wtc wrote:
> 
> 'quic/crypto/key_exchange.h' is still in the wrong place.

Done.

https://codereview.chromium.org/12381018/diff/12007/net/quic/crypto/crypto_ha...
File net/quic/crypto/crypto_handshake.cc (right):

https://codereview.chromium.org/12381018/diff/12007/net/quic/crypto/crypto_ha...
net/quic/crypto/crypto_handshake.cc:10: #include
"net/quic/crypto/key_exchange.h"
On 2013/03/01 23:45:23, wtc wrote:
> 
> Are you sure we need crypto_framer.h and key_exchange.h?

Deleted crypto_frame.h. Left key_exchange because STlDeleteValues needs it.

https://codereview.chromium.org/12381018/diff/12007/net/quic/crypto/crypto_ha...
File net/quic/crypto/crypto_handshake_test.cc (right):

https://codereview.chromium.org/12381018/diff/12007/net/quic/crypto/crypto_ha...
net/quic/crypto/crypto_handshake_test.cc:16: TEST(QuicCryptoProtocol,
ServerConfig) {
On 2013/03/01 23:45:23, wtc wrote:
> 
> You should delete this test now.
> 
> This means lines 7-8 should also be deleted. Just leave
> crypto_handshake.h and the gmock and gtest headers for future
> tests in this file.

Done.

[wtc, 2013-03-02 00:43:42]

Patch set 4 LGTM.

https://chromiumcodereview.appspot.com/12381018/diff/1023/net/quic/crypto/cur...
File net/quic/crypto/curve25519_key_exchange.cc (right):

https://chromiumcodereview.appspot.com/12381018/diff/1023/net/quic/crypto/cur...
net/quic/crypto/curve25519_key_exchange.cc:12: // TODO(rtenneti): Remove the
following two lines after support for curve25519

Nit: two lines => line

[ramant (doing other things), 2013-03-02 00:52:08]

https://chromiumcodereview.appspot.com/12381018/diff/1023/net/quic/crypto/cur...
File net/quic/crypto/curve25519_key_exchange.cc (right):

https://chromiumcodereview.appspot.com/12381018/diff/1023/net/quic/crypto/cur...
net/quic/crypto/curve25519_key_exchange.cc:12: // TODO(rtenneti): Remove the
following two lines after support for curve25519
On 2013/03/02 00:43:42, wtc wrote:
> 
> Nit: two lines => line

Done.

[commit-bot: I haz the power, 2013-03-02 00:59:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/12381018/2011

[commit-bot: I haz the power, 2013-03-02 01:33:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/12381018/4025

[commit-bot: I haz the power, 2013-03-02 16:19:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/12381018/4025

[commit-bot: I haz the power, 2013-03-02 17:43:42]

Change committed as 185726