Restrict QUIC session pool when channel ID is present.

Restrict QUIC session pool when channel ID is present.

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

[wtc, 2014-06-27 23:55:51]

Patch set 1 LGTM.

https://codereview.chromium.org/355293003/diff/1/net/quic/quic_client_session.cc
File net/quic/quic_client_session.cc (right):

https://codereview.chromium.org/355293003/diff/1/net/quic/quic_client_session...
net/quic/quic_client_session.cc:498: ssl_info.cert->VerifyNameMatch(hostname,
&unused);

Nit: a comment that summerizes this complicated conditional expression may be a
good idea.

https://codereview.chromium.org/355293003/diff/1/net/quic/quic_client_session.h
File net/quic/quic_client_session.h (right):

https://codereview.chromium.org/355293003/diff/1/net/quic/quic_client_session...
net/quic/quic_client_session.h:90: // Constructs a new session which will own
|connection| and |helper|, but

Please update this comment. The |helper| parameter is gone. It may also make
sense to document the host_port_pair parameter. I guess it is the server's host
and port?

https://codereview.chromium.org/355293003/diff/1/net/quic/quic_client_session...
net/quic/quic_client_session.h:223: const HostPortPair host_port_pair_;

Nit: it may be better to name this field server_host_port_.

https://codereview.chromium.org/355293003/diff/1/net/quic/quic_client_session...
File net/quic/quic_client_session_test.cc (right):

https://codereview.chromium.org/355293003/diff/1/net/quic/quic_client_session...
net/quic/quic_client_session_test.cc:105: const HostPortPair host_port_pair;

1. The member name is missing the trailing '_'.

2. Nit: server_host_port_?

https://codereview.chromium.org/355293003/diff/1/net/quic/quic_client_session...
net/quic/quic_client_session_test.cc:196: /*

Nit: use #if 0 to comment out a block of code. Please add a comment to explain
why it is commented out. Also, DISABLED_ConnectionPooledWithTlsChannelId may be
better because it allows the code to be compiled.

[Ryan Hamilton, 2014-07-01 18:37:17]

I believe I have implemented that last piece of missing functionality in
GetSSLInfo, and have addressed your comments.  PTAL.

https://codereview.chromium.org/355293003/diff/1/net/quic/quic_client_session.cc
File net/quic/quic_client_session.cc (right):

https://codereview.chromium.org/355293003/diff/1/net/quic/quic_client_session...
net/quic/quic_client_session.cc:498: ssl_info.cert->VerifyNameMatch(hostname,
&unused);
On 2014/06/27 23:55:51, wtc wrote:
> 
> Nit: a comment that summerizes this complicated conditional expression may be
a
> good idea.

Done.  Would you prefer a series of early returns:

if (!ssl_info.cert->VerifyNameMatch(hostname, &unused))
  return false;

if (ssl_info.client_cert_sent)
  return false;

if (ssl_info.channel_id_sent &&
    ServerBoundCertService::GetDomainForHost(hostname) !=
    ServerBoundCertService::GetDomainForHost(host_port_pair_.host()))
  return false;

return true;

https://codereview.chromium.org/355293003/diff/1/net/quic/quic_client_session.h
File net/quic/quic_client_session.h (right):

https://codereview.chromium.org/355293003/diff/1/net/quic/quic_client_session...
net/quic/quic_client_session.h:90: // Constructs a new session which will own
|connection| and |helper|, but
On 2014/06/27 23:55:51, wtc wrote:
> 
> Please update this comment. The |helper| parameter is gone. It may also make
> sense to document the host_port_pair parameter. I guess it is the server's
host
> and port?

Done.

https://codereview.chromium.org/355293003/diff/1/net/quic/quic_client_session...
net/quic/quic_client_session.h:223: const HostPortPair host_port_pair_;
On 2014/06/27 23:55:51, wtc wrote:
> 
> Nit: it may be better to name this field server_host_port_.

Done.

https://codereview.chromium.org/355293003/diff/1/net/quic/quic_client_session...
File net/quic/quic_client_session_test.cc (right):

https://codereview.chromium.org/355293003/diff/1/net/quic/quic_client_session...
net/quic/quic_client_session_test.cc:105: const HostPortPair host_port_pair;
On 2014/06/27 23:55:51, wtc wrote:
> 
> 1. The member name is missing the trailing '_'.
> 
> 2. Nit: server_host_port_?

Done.

https://codereview.chromium.org/355293003/diff/1/net/quic/quic_client_session...
net/quic/quic_client_session_test.cc:196: /*
On 2014/06/27 23:55:51, wtc wrote:
> 
> Nit: use #if 0 to comment out a block of code. Please add a comment to explain
> why it is commented out. Also, DISABLED_ConnectionPooledWithTlsChannelId may
be
> better because it allows the code to be compiled.

Sorry, this was not yet implemented since it depended on your CL landing. Since
that is in now, I've actually implemented this test.

[wtc, 2014-07-01 23:00:15]

Patch set 2 LGTM.

https://codereview.chromium.org/355293003/diff/1/net/quic/quic_client_session.cc
File net/quic/quic_client_session.cc (right):

https://codereview.chromium.org/355293003/diff/1/net/quic/quic_client_session...
net/quic/quic_client_session.cc:498: ssl_info.cert->VerifyNameMatch(hostname,
&unused);

On 2014/07/01 18:37:17, Ryan Hamilton wrote:
>
> Done.  Would you prefer a series of early returns:

Yes, I also wanted to suggest that :-) One reason it's hard to parse this code
is the complicated conditional expression.

> if (!ssl_info.cert->VerifyNameMatch(hostname, &unused))
>   return false;
> 
> if (ssl_info.client_cert_sent)
>   return false;
> 
> if (ssl_info.channel_id_sent &&
>     ServerBoundCertService::GetDomainForHost(hostname) !=
>     ServerBoundCertService::GetDomainForHost(host_port_pair_.host()))
>   return false;
> 
> return true;

You need to use curly braces, required by QUIC's local style.

https://codereview.chromium.org/355293003/diff/20001/net/quic/quic_client_ses...
File net/quic/quic_client_session.cc (right):

https://codereview.chromium.org/355293003/diff/20001/net/quic/quic_client_ses...
net/quic/quic_client_session.cc:495: // hosts are from different ETLDs. And of
course, it is prohibited

ETLDs => eTLDs

https://codereview.chromium.org/355293003/diff/20001/net/quic/quic_client_ses...
net/quic/quic_client_session.cc:496: // if the cert is not valid for the new
domain.

cert => server cert

https://codereview.chromium.org/355293003/diff/20001/net/quic/quic_client_ses...
File net/quic/quic_client_session.h (right):

https://codereview.chromium.org/355293003/diff/20001/net/quic/quic_client_ses...
net/quic/quic_client_session.h:90: // Constructs a new session connected to
|host_port_pair| which will own

host_port_pair => server_host_port

https://codereview.chromium.org/355293003/diff/20001/net/quic/quic_client_ses...
net/quic/quic_client_session.h:93: QuicClientSession(const HostPortPair&
server_host_port,

IMPORTANT: the HostPortPair is readily available in the |server_id| parameter,
so this new parameter doesn't seem necessary.

https://codereview.chromium.org/355293003/diff/20001/net/quic/quic_client_ses...
File net/quic/quic_client_session_test.cc (right):

https://codereview.chromium.org/355293003/diff/20001/net/quic/quic_client_ses...
net/quic/quic_client_session_test.cc:72: // signature into |out_signature|. It
returns true on success.

Replace this comment with something like
  // ChannelIDKey implementation:

https://codereview.chromium.org/355293003/diff/20001/net/quic/quic_client_ses...
net/quic/quic_client_session_test.cc:75: return true;

I think it's more realistic to set *out_signature to some string. (I know this
method can even return false because it's not invoked by the test.)

https://codereview.chromium.org/355293003/diff/20001/net/quic/quic_client_ses...
net/quic/quic_client_session_test.cc:78: // SerializeKey returns the serialized
ChannelID public key.

Delete this comment.

https://codereview.chromium.org/355293003/diff/20001/net/quic/quic_client_ses...
net/quic/quic_client_session_test.cc:122: const HostPortPair host_port_pair_;

1. IMPORTANT: we never set this member, so it is an empty string!

2. Please rename this member "server_host_port_", if it refers to the server.

https://codereview.chromium.org/355293003/diff/20001/net/quic/quic_client_ses...
net/quic/quic_client_session_test.cc:203:
details.cert_verify_result.verified_cert);

Nit: why don't we just do

  ASSERT_TRUE(details.cert_verify_result.verified_cert);

If you make this change, make the same change to the next unit test.

https://codereview.chromium.org/355293003/diff/20001/net/quic/quic_client_ses...
net/quic/quic_client_session_test.cc:231: CompleteCryptoHandshake();

It seems safer to call QuicClientSessionPeer::SetChannelIDKey after
CompleteCryptoHandshake.

https://codereview.chromium.org/355293003/diff/20001/net/quic/quic_crypto_cli...
File net/quic/quic_crypto_client_stream.cc (right):

https://codereview.chromium.org/355293003/diff/20001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.cc:113: return channel_id_key_.get() != NULL;

Please add a TODO comment that we should replace this with a boolean data member
channel_id_sent_ to allow us to free channel_id_key_ after we're done using it.

https://codereview.chromium.org/355293003/diff/20001/net/quic/quic_crypto_cli...
File net/quic/quic_crypto_client_stream.h (right):

https://codereview.chromium.org/355293003/diff/20001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.h:48: bool WasChannelIDSent() const;

Please document this method.

For your reference, here is the comment for the same-named method in
net/socket/ssl_client_socket.h:

  // Returns true if a channel ID was sent on this connection.
  // This may be useful for protocols, like SPDY, which allow the same
  // connection to be shared between multiple domains, each of which need
  // a channel ID.

https://codereview.chromium.org/355293003/diff/20001/net/quic/quic_http_strea...
File net/quic/quic_http_stream_test.cc (right):

https://codereview.chromium.org/355293003/diff/20001/net/quic/quic_http_strea...
net/quic/quic_http_stream_test.cc:292: const HostPortPair host_port_pair_;

IMPORTANT: this is an empty string!

[Ryan Hamilton, 2014-07-01 23:26:20]

Thanks! Great comments.

https://codereview.chromium.org/355293003/diff/1/net/quic/quic_client_session.cc
File net/quic/quic_client_session.cc (right):

https://codereview.chromium.org/355293003/diff/1/net/quic/quic_client_session...
net/quic/quic_client_session.cc:498: ssl_info.cert->VerifyNameMatch(hostname,
&unused);
On 2014/07/01 23:00:14, wtc wrote:
> 
> On 2014/07/01 18:37:17, Ryan Hamilton wrote:
> >
> > Done.  Would you prefer a series of early returns:
> 
> Yes, I also wanted to suggest that :-) One reason it's hard to parse this code
> is the complicated conditional expression.

Done.

> > if (!ssl_info.cert->VerifyNameMatch(hostname, &unused))
> >   return false;
> > 
> > if (ssl_info.client_cert_sent)
> >   return false;
> > 
> > if (ssl_info.channel_id_sent &&
> >     ServerBoundCertService::GetDomainForHost(hostname) !=
> >     ServerBoundCertService::GetDomainForHost(host_port_pair_.host()))
> >   return false;
> > 
> > return true;
> 
> You need to use curly braces, required by QUIC's local style.

That's only for shared code. Since this is chrome specific, we should adhere to
the network style, I think.

https://codereview.chromium.org/355293003/diff/20001/net/quic/quic_client_ses...
File net/quic/quic_client_session.cc (right):

https://codereview.chromium.org/355293003/diff/20001/net/quic/quic_client_ses...
net/quic/quic_client_session.cc:495: // hosts are from different ETLDs. And of
course, it is prohibited
On 2014/07/01 23:00:14, wtc wrote:
> 
> ETLDs => eTLDs

Done.

https://codereview.chromium.org/355293003/diff/20001/net/quic/quic_client_ses...
net/quic/quic_client_session.cc:496: // if the cert is not valid for the new
domain.
On 2014/07/01 23:00:14, wtc wrote:
> 
> cert => server cert

Done.

https://codereview.chromium.org/355293003/diff/20001/net/quic/quic_client_ses...
File net/quic/quic_client_session.h (right):

https://codereview.chromium.org/355293003/diff/20001/net/quic/quic_client_ses...
net/quic/quic_client_session.h:90: // Constructs a new session connected to
|host_port_pair| which will own
On 2014/07/01 23:00:14, wtc wrote:
> 
> host_port_pair => server_host_port

Reverted this and just used server_id instead.

https://codereview.chromium.org/355293003/diff/20001/net/quic/quic_client_ses...
net/quic/quic_client_session.h:93: QuicClientSession(const HostPortPair&
server_host_port,
On 2014/07/01 23:00:14, wtc wrote:
> 
> IMPORTANT: the HostPortPair is readily available in the |server_id| parameter,
> so this new parameter doesn't seem necessary.

Done.

https://codereview.chromium.org/355293003/diff/20001/net/quic/quic_client_ses...
File net/quic/quic_client_session_test.cc (right):

https://codereview.chromium.org/355293003/diff/20001/net/quic/quic_client_ses...
net/quic/quic_client_session_test.cc:72: // signature into |out_signature|. It
returns true on success.
On 2014/07/01 23:00:14, wtc wrote:
> 
> Replace this comment with something like
>   // ChannelIDKey implementation:

Done.

https://codereview.chromium.org/355293003/diff/20001/net/quic/quic_client_ses...
net/quic/quic_client_session_test.cc:75: return true;
On 2014/07/01 23:00:15, wtc wrote:
> 
> I think it's more realistic to set *out_signature to some string. (I know this
> method can even return false because it's not invoked by the test.)

Done.

https://codereview.chromium.org/355293003/diff/20001/net/quic/quic_client_ses...
net/quic/quic_client_session_test.cc:78: // SerializeKey returns the serialized
ChannelID public key.
On 2014/07/01 23:00:15, wtc wrote:
> 
> Delete this comment.

Done.

https://codereview.chromium.org/355293003/diff/20001/net/quic/quic_client_ses...
net/quic/quic_client_session_test.cc:122: const HostPortPair host_port_pair_;
On 2014/07/01 23:00:14, wtc wrote:
> 
> 1. IMPORTANT: we never set this member, so it is an empty string!
> 
> 2. Please rename this member "server_host_port_", if it refers to the server.

Ok, fixed this. Turns out I hadn't actually run the channel ID test. It failed
because this was not set. Now that it's fixed, it passed. Nice catch!

https://codereview.chromium.org/355293003/diff/20001/net/quic/quic_client_ses...
net/quic/quic_client_session_test.cc:203:
details.cert_verify_result.verified_cert);
On 2014/07/01 23:00:14, wtc wrote:
> 
> Nit: why don't we just do
> 
>   ASSERT_TRUE(details.cert_verify_result.verified_cert);
> 
> If you make this change, make the same change to the next unit test.

Done.

https://codereview.chromium.org/355293003/diff/20001/net/quic/quic_client_ses...
net/quic/quic_client_session_test.cc:231: CompleteCryptoHandshake();
On 2014/07/01 23:00:14, wtc wrote:
> 
> It seems safer to call QuicClientSessionPeer::SetChannelIDKey after
> CompleteCryptoHandshake.

Done.

https://codereview.chromium.org/355293003/diff/20001/net/quic/quic_crypto_cli...
File net/quic/quic_crypto_client_stream.cc (right):

https://codereview.chromium.org/355293003/diff/20001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.cc:113: return channel_id_key_.get() != NULL;
On 2014/07/01 23:00:15, wtc wrote:
> 
> Please add a TODO comment that we should replace this with a boolean data
member
> channel_id_sent_ to allow us to free channel_id_key_ after we're done using
it.

Done.

https://codereview.chromium.org/355293003/diff/20001/net/quic/quic_crypto_cli...
File net/quic/quic_crypto_client_stream.h (right):

https://codereview.chromium.org/355293003/diff/20001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.h:48: bool WasChannelIDSent() const;
On 2014/07/01 23:00:15, wtc wrote:
> 
> Please document this method.
> 
> For your reference, here is the comment for the same-named method in
> net/socket/ssl_client_socket.h:
> 
>   // Returns true if a channel ID was sent on this connection.
>   // This may be useful for protocols, like SPDY, which allow the same
>   // connection to be shared between multiple domains, each of which need
>   // a channel ID.

Done.

https://codereview.chromium.org/355293003/diff/20001/net/quic/quic_http_strea...
File net/quic/quic_http_stream_test.cc (right):

https://codereview.chromium.org/355293003/diff/20001/net/quic/quic_http_strea...
net/quic/quic_http_stream_test.cc:292: const HostPortPair host_port_pair_;
On 2014/07/01 23:00:15, wtc wrote:
> 
> IMPORTANT: this is an empty string!

Reverted this change.

[Ryan Hamilton, 2014-07-01 23:26:30]

The CQ bit was checked by rch@chromium.org

[commit-bot: I haz the power, 2014-07-01 23:27:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rch@chromium.org/355293003/40001

[wtc, 2014-07-02 00:18:54]

Patch set 3 LGTM. You or I can fix the comment nit later. Thanks!

https://codereview.chromium.org/355293003/diff/40001/net/quic/quic_client_ses...
File net/quic/quic_client_session.cc (right):

https://codereview.chromium.org/355293003/diff/40001/net/quic/quic_client_ses...
net/quic/quic_client_session.cc:495: // if the server cert is not valid for the
new domain.

Nit: reorder the comments or the three tests so that they match. Right now the
last sentence in the comments describes the first test.

[wtc, 2014-07-02 00:47:48]

https://codereview.chromium.org/355293003/diff/40001/net/quic/quic_client_ses...
File net/quic/quic_client_session.cc (right):

https://codereview.chromium.org/355293003/diff/40001/net/quic/quic_client_ses...
net/quic/quic_client_session.cc:494: // hosts are from different eTLDs. And of
course, it is prohibited

I just realized one error: this should be "domains" instead of "eTLDs". eTLDs
are "com", "co.uk", etc. But what you have in mind is "google.com",
"google.co.uk", etc.

[commit-bot: I haz the power, 2014-07-02 07:15:18]

Change committed as 280963

[Ryan Hamilton, 2014-07-02 19:46:38]

Thanks!

https://codereview.chromium.org/355293003/diff/40001/net/quic/quic_client_ses...
File net/quic/quic_client_session.cc (right):

https://codereview.chromium.org/355293003/diff/40001/net/quic/quic_client_ses...
net/quic/quic_client_session.cc:494: // hosts are from different eTLDs. And of
course, it is prohibited
On 2014/07/02 00:47:48, wtc wrote:
> 
> I just realized one error: this should be "domains" instead of "eTLDs". eTLDs
> are "com", "co.uk", etc. But what you have in mind is "google.com",
> "google.co.uk", etc.

Actually eTLD+1 because it's ok to pool i1.foo.google.com and i2.bar.google.com.
 Fixed in https://codereview.chromium.org/362323005.

https://codereview.chromium.org/355293003/diff/40001/net/quic/quic_client_ses...
net/quic/quic_client_session.cc:495: // if the server cert is not valid for the
new domain.
On 2014/07/02 00:18:54, wtc wrote:
> 
> Nit: reorder the comments or the three tests so that they match. Right now the
> last sentence in the comments describes the first test.

Done in https://codereview.chromium.org/362323005.