Correctly handle SSL Client Authentication requests when connecting...

Correctly handle SSL Client Authentication requests when connecting
to an HTTPS/SPDY proxy.  Modify SSLClientSocket classes to correctly set the host_and_port field of the cert_request_info.  Modify HttpNetworkTransaction to use this field when populating the SSL client auth cache.

BUG=59292
TEST=HttpProxyClientSocketPoolTest.SslClientAuth


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=65976

[Ryan Hamilton, 2010-11-02 22:41:27]

Hi Guys,

I think I have Proxy SSL Client Auth working.  Yay!  It turned out to be
relatively straightforward matter of threading the new
ERR_PROXY_SSL_CLIENT_AUTH_CERT_NEEDED error code in all the right places.  I
think we could get rid of this whole code path if we could trust
cert_request_info.host_and_port.  When HttpNetworkTransaction is called back
with a certificate, it stashes it in the ssl_client_auth_cache associated with
the host:port of request_->url, not from the cert_request_info.  If y'all think
that HttpNetworkTransaction could/should trust cert_request_info.host_and_port,
then this could get smaller.

Cheers,

Ryan

Cheers,

Ryan

[Ryan Hamilton, 2010-11-02 23:03:00]

Ugh!  I take it back.  Due to a configuration issue on my workstation, some of
the testing that I thought I was doing through a proxy was actually direct. 
*sigh*   (Turns out that pointing --proxy-pac-url to a URL that does not exists
is not a fatal error :>)

Please ignore this CL until I rectify the problem.

SOrry!

On 2010/11/02 22:41:27, Ryan Hamilton wrote:
> Hi Guys,
> 
> I think I have Proxy SSL Client Auth working.  Yay!  It turned out to be
> relatively straightforward matter of threading the new
> ERR_PROXY_SSL_CLIENT_AUTH_CERT_NEEDED error code in all the right places.  I
> think we could get rid of this whole code path if we could trust
> cert_request_info.host_and_port.  When HttpNetworkTransaction is called back
> with a certificate, it stashes it in the ssl_client_auth_cache associated with
> the host:port of request_->url, not from the cert_request_info.  If y'all
think
> that HttpNetworkTransaction could/should trust
cert_request_info.host_and_port,
> then this could get smaller.
> 
> Cheers,
> 
> Ryan
> 
> Cheers,
> 
> Ryan

[Ryan Hamilton, 2010-11-03 18:12:06]

Ok, once more unto the breach.  After fixing my test setup, this CL does work as
expected.  Would you guys mind reviewing it?

On 2010/11/02 23:03:00, Ryan Hamilton wrote:
> Ugh!  I take it back.  Due to a configuration issue on my workstation, some of
> the testing that I thought I was doing through a proxy was actually direct. 
> *sigh*   (Turns out that pointing --proxy-pac-url to a URL that does not
exists
> is not a fatal error :>)
> 
> Please ignore this CL until I rectify the problem.
> 
> SOrry!
> 
> On 2010/11/02 22:41:27, Ryan Hamilton wrote:
> > Hi Guys,
> > 
> > I think I have Proxy SSL Client Auth working.  Yay!  It turned out to be
> > relatively straightforward matter of threading the new
> > ERR_PROXY_SSL_CLIENT_AUTH_CERT_NEEDED error code in all the right places.  I
> > think we could get rid of this whole code path if we could trust
> > cert_request_info.host_and_port.  When HttpNetworkTransaction is called back
> > with a certificate, it stashes it in the ssl_client_auth_cache associated
with
> > the host:port of request_->url, not from the cert_request_info.  If y'all
> think
> > that HttpNetworkTransaction could/should trust
> cert_request_info.host_and_port,
> > then this could get smaller.
> > 
> > Cheers,
> > 
> > Ryan
> > 
> > Cheers,
> > 
> > Ryan

[wtc, 2010-11-04 21:53:18]

eroman: I have a question for you in
http_network_transaction.cc.  Please search for "eroman" in
my comments below.

rch: thanks for the CL.  Please read my two comments in
http_network_transaction.cc marked with "IMPORTANT" first,
and see if you think my proposed alternative solution is
better.  Perhaps what I proposed is bogus.

As I'm taking tomorrow off, the complete resolution of this
issue may be next week.  Hope that's OK.

http://codereview.chromium.org/4339001/diff/47001/1017
File net/base/net_error_list.h (right):

http://codereview.chromium.org/4339001/diff/47001/1017#newcode217
net/base/net_error_list.h:217: NET_ERROR(PROXY_SSL_CLIENT_AUTH_CERT_NEEDED,
-139)
It may be a good idea to add an IsCertificateRequest
inline function, analogous to IsCertificateError, to
net/base/net_errors.h, so that you don't need to repeat the
expression
  result == ERR_SSL_CLIENT_AUTH_CERT_NEEDED ||
  result == ERR_PROXY_SSL_CLIENT_AUTH_CERT_NEEDED
in several places.

http://codereview.chromium.org/4339001/diff/47001/1018
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/4339001/diff/47001/1018#newcode95
net/http/http_network_transaction.cc:95: establishing_tunnel_(false) {
BUG: initialize ssl_client_cert_error_ to 'OK'.

http://codereview.chromium.org/4339001/diff/47001/1018#newcode176
net/http/http_network_transaction.cc:176: if (ssl_client_cert_error_ ==
ERR_PROXY_SSL_CLIENT_AUTH_CERT_NEEDED)
IMPORTANT: Is this the only place where you depend on a
separate error code for proxy SSL client auth?

If so, this tells us two things.
1. All we need is the correct host_port value, and it is
available in response_.cert_request_info->host_and_port,
except that SSLClientSocketNSS doesn't include the port in
it (see the TODO(wtc) comment in SSLClientSocketNSS::GetSSLCertRequestInfo).

So it seems that we should fix this bug by passing the port
to SSLClientSocket constructor and the factory method, and
fix that TODO comment in SSLClientSocketNSS::GetSSLCertRequestInfo.

2. We may need to provide two client certs, one for the
proxy and the other for the server, if both the proxy and
the server require SSL client auth.  I haven't thought this
through to know if this is really necessary.  You can deal
with this later.

http://codereview.chromium.org/4339001/diff/47001/1018#newcode177
net/http/http_network_transaction.cc:177: host_port =
proxy_info_.proxy_server().host_port_pair().ToString();
eroman: can you review this change?

http://codereview.chromium.org/4339001/diff/47001/1018#newcode1010
net/http/http_network_transaction.cc:1010: return
ERR_SSL_CLIENT_AUTH_CERT_NEEDED;
IMPORTANT/BUG: we should return 'error' here, right?
Are you trying to avoid changes to the upper layers?

http://codereview.chromium.org/4339001/diff/47001/1019
File net/http/http_network_transaction.h (right):

http://codereview.chromium.org/4339001/diff/47001/1019#newcode251
net/http/http_network_transaction.h:251: int ssl_client_cert_error_;
Nit: ssl_client_cert_error_ => ssl_client_auth_error_

http://codereview.chromium.org/4339001/diff/47001/1020
File net/http/http_proxy_client_socket_pool.cc (right):

http://codereview.chromium.org/4339001/diff/47001/1020#newcode215
net/http/http_proxy_client_socket_pool.cc:215: return
ERR_PROXY_SSL_CLIENT_AUTH_CERT_NEEDED;
This is the only place where we set/return the
ERR_PROXY_SSL_CLIENT_AUTH_CERT_NEEDED error, right?

http://codereview.chromium.org/4339001/diff/47001/1020#newcode255
net/http/http_proxy_client_socket_pool.cc:255: if
(error_response_info_.cert_request_info) {
Should we move the previous line (line 254) into the "if"
statement?  It seems that we need to set the SSL error
response info only when we have an SSL error.

Nit: omit the curly braces {}.

Nit: remove the space before '*' on line 253 above.

http://codereview.chromium.org/4339001/diff/47001/1020#newcode270
net/http/http_proxy_client_socket_pool.cc:270: if
(transport_socket_handle_.get()) {
Just wondering: why do you add the null pointer check here?

http://codereview.chromium.org/4339001/diff/47001/1020#newcode282
net/http/http_proxy_client_socket_pool.cc:282: if
(transport_socket_handle_->socket())
BUG: after the transport_socket_handle_.release() call on
line 279 above, transport_socket_handle_ contains a NULL
pointer, so we can't dereference transport_socket_handle_
here.

http://codereview.chromium.org/4339001/diff/47001/1021
File net/http/http_proxy_client_socket_pool.h (right):

http://codereview.chromium.org/4339001/diff/47001/1021#newcode109
net/http/http_proxy_client_socket_pool.h:109: virtual void
GetAdditionalErrorState(ClientSocketHandle * handle);
Nit: remove the space before '*'.

http://codereview.chromium.org/4339001/diff/47001/1025
File net/http/stream_factory.h (right):

http://codereview.chromium.org/4339001/diff/47001/1025#newcode71
net/http/stream_factory.h:71: // ERR_SSL_CLIENT_AUTH_CERT_NEEDED if the server
requested client auth, or
Nit: add "SSL" before "client auth" in this line and the next
line.

Instead of the input parameter
  int ssl_client_cert_error
you can consider using the enum HttpAuth::Target (defined
in net/http/http_auth.h).  It's a little strange to use an
HTTP-related type for SSL though.  In any case, do not use
a bool.

Nit: ssl_client_cert_error => ssl_client_auth_error
ssl_client_cert_error sounds like an error in the client
certificate.  Rename this parameter in the subclasses, too.

http://codereview.chromium.org/4339001/diff/47001/1026
File net/socket/ssl_client_socket_pool.cc (right):

http://codereview.chromium.org/4339001/diff/47001/1026#newcode255
net/socket/ssl_client_socket_pool.cc:255: // Extract the information needed to
prompt for the proxy client auth.
Nit: client auth => SSL client auth

Nit: feel free to ignore the following suggested change.
You can write the following three "if" statements as
if-else-if-else-if.  In addition, lines 251-253 above
(for computing tunnle_socket) can be moved into the
result == ERR_PROXY_AUTH_REQUESTED case as tunnel_socket
is used only in that case.

[Ryan Hamilton, 2010-11-04 22:57:42]

wtc: thanks for the review.  I'll get started reworking the CL now.  If I'm
lucky, I'll have something for you when you get back.  Have a good day off!

http://codereview.chromium.org/4339001/diff/47001/1018
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/4339001/diff/47001/1018#newcode176
net/http/http_network_transaction.cc:176: if (ssl_client_cert_error_ ==
ERR_PROXY_SSL_CLIENT_AUTH_CERT_NEEDED)
On 2010/11/04 21:53:18, wtc wrote:
> IMPORTANT: Is this the only place where you depend on a
> separate error code for proxy SSL client auth?

Yes.
 
> If so, this tells us two things.
> 1. All we need is the correct host_port value, and it is
> available in response_.cert_request_info->host_and_port,
> except that SSLClientSocketNSS doesn't include the port in
> it (see the TODO(wtc) comment in SSLClientSocketNSS::GetSSLCertRequestInfo).
> 
> So it seems that we should fix this bug by passing the port
> to SSLClientSocket constructor and the factory method, and
> fix that TODO comment in SSLClientSocketNSS::GetSSLCertRequestInfo.

Great, I'm glad you like this approach.  In that case, I think I can
significantly rework this CL.  I think it will be possible to remove the extra
error code.  I'll get started on this.

> 2. We may need to provide two client certs, one for the
> proxy and the other for the server, if both the proxy and
> the server require SSL client auth.  I haven't thought this
> through to know if this is really necessary.  You can deal
> with this later.

I *think* this will not be a problem, though I will definitely test it first.  I
think it will be OK because the 2 client certs will be requested for two
different (albiet nested) SSL connections.

[wtc, 2010-11-04 23:11:27]

http://codereview.chromium.org/4339001/diff/47001/1018
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/4339001/diff/47001/1018#newcode176
net/http/http_network_transaction.cc:176: if (ssl_client_cert_error_ ==
ERR_PROXY_SSL_CLIENT_AUTH_CERT_NEEDED)
On 2010/11/04 22:57:42, Ryan Hamilton wrote:
>
> Great, I'm glad you like this approach.

It sounds like you already considered this approach?

> I think it will be OK because the 2 client certs will be requested for two
> different (albiet nested) SSL connections.

I see.  So the SSL connection to the HTTPS proxy will become
the 'transport socket' for the SSL connection to the server,
and we will use two separate SSLConfig objects for them.
Hmm... I think you're right! :-)

[Ryan Hamilton, 2010-11-04 23:16:22]

http://codereview.chromium.org/4339001/diff/47001/1018
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/4339001/diff/47001/1018#newcode176
net/http/http_network_transaction.cc:176: if (ssl_client_cert_error_ ==
ERR_PROXY_SSL_CLIENT_AUTH_CERT_NEEDED)
On 2010/11/04 23:11:27, wtc wrote:
> On 2010/11/04 22:57:42, Ryan Hamilton wrote:
> >
> > Great, I'm glad you like this approach.
> 
> It sounds like you already considered this approach?

Right, I mentioned it in the initial message I sent out for this CL.  (Of
course, I then sent two more messages, so the first was probably lost.) 

"I think we could get rid of this whole code path if we could trust
cert_request_info.host_and_port.  When HttpNetworkTransaction is called back
with a certificate, it stashes it in the ssl_client_auth_cache associated with
the host:port of request_->url, not from the cert_request_info.  If y'all think
that HttpNetworkTransaction could/should trust  cert_request_info.host_and_port,
then this could get smaller."

I didn't understand issue to the level of detail that you provided, however, so
I didn't feel comfortable going down that path.
 
> > I think it will be OK because the 2 client certs will be requested for two
> > different (albiet nested) SSL connections.
> 
> I see.  So the SSL connection to the HTTPS proxy will become
> the 'transport socket' for the SSL connection to the server,
> and we will use two separate SSLConfig objects for them.
> Hmm... I think you're right! :-)

Awesome.  I'll let you know when I've got a new version of the CL.  thanks!

[Ryan Hamilton, 2010-11-08 17:45:19]

Wan-Teh, can you take a look at the new patch I uploaded to see if this seems
like a better approach to you?  I've not quite finished it because it ended up
more invasive than I would have expected.  It turns out that the SSL client
socket factory is exposed into the "jingle" code.  Trying to add the port at all
call sites would mean going into libjingle.  I'm not sure if that's a "good"
idea.  What do you think?

[eroman, 2010-11-08 22:22:25]

http://codereview.chromium.org/4339001/diff/47001/1018
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/4339001/diff/47001/1018#newcode177
net/http/http_network_transaction.cc:177: host_port =
proxy_info_.proxy_server().host_port_pair().ToString();
On 2010/11/04 21:53:18, wtc wrote:
> eroman: can you review this change?

I don't understand this change, what is the intended host:port you are trying to
get? |prox_info_| could be specifying DIRECT, in which case the host_port_pair
will be empty

[wtc, 2010-11-11 01:11:35]

LGTM.  I requested many changes below, but I trust that you
can make the suggested changes correctly.  So you can check
in the changelist after making the changes.  Just upload a
snapshot so I can review it later.

IMPORTANT: please update the Description of the CL to reflect
the new code.

http://codereview.chromium.org/4339001/diff/76002/jingle/notifier/base/chrome...
File jingle/notifier/base/chrome_async_socket.cc (right):

http://codereview.chromium.org/4339001/diff/76002/jingle/notifier/base/chrome...
jingle/notifier/base/chrome_async_socket.cc:438: transport_socket_.release(),
domain_name, /*TODO(rch)*/-1,
Please pass 443 instead of -1 as the temporary port value.

(I think 443 is the correct value, and you can remove
the TODO comment.)

http://codereview.chromium.org/4339001/diff/76002/jingle/notifier/base/xmpp_c...
File jingle/notifier/base/xmpp_client_socket_factory.h (right):

http://codereview.chromium.org/4339001/diff/76002/jingle/notifier/base/xmpp_c...
jingle/notifier/base/xmpp_client_socket_factory.h:10: #include
"base/basictypes.h"
Move this include into client_socket_factory.h.

http://codereview.chromium.org/4339001/diff/76002/net/http/http_network_trans...
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/4339001/diff/76002/net/http/http_network_trans...
net/http/http_network_transaction.cc:175:
session_->ssl_client_auth_cache()->Add(
Please DCHECK that response_.cert_request_info->host_and_port
and GetHostAndPort(request_->url) are equal.  Also at line
982 below.

http://codereview.chromium.org/4339001/diff/76002/net/http/http_proxy_client_...
File net/http/http_proxy_client_socket_pool.cc (right):

http://codereview.chromium.org/4339001/diff/76002/net/http/http_proxy_client_...
net/http/http_proxy_client_socket_pool.cc:86: error_response_info_() {
Remove this.  We usually omit this kind of member initializer
that takes no argument.

http://codereview.chromium.org/4339001/diff/76002/net/http/http_proxy_client_...
net/http/http_proxy_client_socket_pool.cc:214: return
ERR_SSL_CLIENT_AUTH_CERT_NEEDED;
You can just return 'result' now.

http://codereview.chromium.org/4339001/diff/76002/net/http/http_proxy_client_...
net/http/http_proxy_client_socket_pool.cc:245: LOG(INFO) << "Connected to HTTPS
proxy, using spdy: " << (using_spdy_ ? "yes" : "no");
Use VLOG(1) instead of LOG(INFO).

This line seems longer than 80 characters.

http://codereview.chromium.org/4339001/diff/76002/net/http/http_proxy_client_...
net/http/http_proxy_client_socket_pool.cc:270: if
(transport_socket_handle_.get()) {
Can you explain why you need the null pointer check for
transport_socket_handle_ here?

http://codereview.chromium.org/4339001/diff/76002/net/http/http_proxy_client_...
net/http/http_proxy_client_socket_pool.cc:282: if
(transport_socket_handle_->socket())
I pointed this out before: after the transport_socket_handle_.release()
call on line 279 above, transport_socket_handle_ contains
NULL, so transport_socket_handle_->socket() will crash.

http://codereview.chromium.org/4339001/diff/76002/net/http/http_stream_request.h
File net/http/http_stream_request.h (right):

http://codereview.chromium.org/4339001/diff/76002/net/http/http_stream_reques...
net/http/http_stream_request.h:143: scoped_refptr<SSLSocketParams>
GenerateSslParams(
Can you see if you can rename GenerateSslParams to
GenerateSSLParams without making this CL much larger?

http://codereview.chromium.org/4339001/diff/76002/net/socket/client_socket_po...
File net/socket/client_socket_pool_base_unittest.cc (right):

http://codereview.chromium.org/4339001/diff/76002/net/socket/client_socket_po...
net/socket/client_socket_pool_base_unittest.cc:7: #include "base/basictypes.h"
Please move this #include to client_socket_factory.h.

http://codereview.chromium.org/4339001/diff/76002/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_mac.cc (right):

http://codereview.chromium.org/4339001/diff/76002/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_mac.cc:728:
X509Certificate::GetSSLClientCertificates(hostname_,
Please add a TODO comment to note that we should consider
passing a host-port pair as the first argument to
X509Certificate::GetSSLClientCertificates.

http://codereview.chromium.org/4339001/diff/76002/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_mac.cc:812: std::string peer_id(hostname_);
IMPORTANT: we should include 'port_' in peer_id.  See
http://developer.apple.com/library/mac/documentation/Security/Reference/secur...

If you don't have time to do this, just add a TODO comment.

http://codereview.chromium.org/4339001/diff/76002/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/4339001/diff/76002/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_nss.cc:876: // TODO(wtc): port in |peer_address| is
not the server's port when a proxy is
Please fix this TODO by replacing the base::StringPrintf call
on line 878 with HostPortPair(hostname_, port_).ToString().

http://codereview.chromium.org/4339001/diff/76002/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_nss.cc:2320: #if !defined(USE_OPENSSL)
I think we should remove this ifdef.  By definition, if
we use SSLClientSocketNSS, we are not using OpenSSL.

http://codereview.chromium.org/4339001/diff/76002/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_pool.cc (right):

http://codereview.chromium.org/4339001/diff/76002/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_pool.cc:256: if (result ==
ERR_SSL_CLIENT_AUTH_CERT_NEEDED)
Add curly braces.  This is required by the Style Guide, if
any branch in an if statement has curly braces.

Please rewrite the first comment block so that it applies to
both branches and delete the second comment block.

http://codereview.chromium.org/4339001/diff/76002/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_pool.cc:294: params_->port(),
Nit: merge these two lines.

http://codereview.chromium.org/4339001/diff/76002/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_pool.cc:331: LOG(INFO) << "Connected to host
through proxy, over spdy: " << (using_spdy ? "yes" : "no");
Is this line longer than 80 characters?

Replace LOG(INFO) by VLOG(1).

http://codereview.chromium.org/4339001/diff/76002/remoting/jingle_glue/ssl_so...
File remoting/jingle_glue/ssl_socket_adapter.cc (right):

http://codereview.chromium.org/4339001/diff/76002/remoting/jingle_glue/ssl_so...
remoting/jingle_glue/ssl_socket_adapter.cc:70: transport_socket_,
hostname_.c_str(), /*TODO(rch)*/-1, ssl_config,
Isn't 443 better than -1 as a temporary port value?

[Ryan Hamilton, 2010-11-11 18:57:00]

Thanks for the very thorough review!  I've addressed all of your issues, with
the following exception:

> Please DCHECK that response_.cert_request_info->host_and_port
> and GetHostAndPort(request_->url) are equal.

(See the comment for more detail)

I also had a question about this:

> IMPORTANT: we should include 'port_' in peer_id

I made the change, but I'm not sure if I need to track down any consumers of
this data structure that might be expecting the old format?  Of course, now that
NSS is the default SSL library, this is probably not a huge issue.

I'll send it off to the try bots and will commit (eventually :>) once it
succeeds, unless I hear otherwise from you.

http://codereview.chromium.org/4339001/diff/76002/jingle/notifier/base/chrome...
File jingle/notifier/base/chrome_async_socket.cc (right):

http://codereview.chromium.org/4339001/diff/76002/jingle/notifier/base/chrome...
jingle/notifier/base/chrome_async_socket.cc:438: transport_socket_.release(),
domain_name, /*TODO(rch)*/-1,
On 2010/11/11 01:11:35, wtc wrote:
> Please pass 443 instead of -1 as the temporary port value.
> 
> (I think 443 is the correct value, and you can remove
> the TODO comment.)

Done.

http://codereview.chromium.org/4339001/diff/76002/jingle/notifier/base/xmpp_c...
File jingle/notifier/base/xmpp_client_socket_factory.h (right):

http://codereview.chromium.org/4339001/diff/76002/jingle/notifier/base/xmpp_c...
jingle/notifier/base/xmpp_client_socket_factory.h:10: #include
"base/basictypes.h"
On 2010/11/11 01:11:35, wtc wrote:
> Move this include into client_socket_factory.h.

Done.

http://codereview.chromium.org/4339001/diff/76002/net/http/http_network_trans...
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/4339001/diff/76002/net/http/http_network_trans...
net/http/http_network_transaction.cc:175:
session_->ssl_client_auth_cache()->Add(
On 2010/11/11 01:11:35, wtc wrote:
> Please DCHECK that response_.cert_request_info->host_and_port
> and GetHostAndPort(request_->url) are equal.  Also at line
> 982 below.

I don't think this would be correct.  In the case of an HTTPS proxy, the
response_.cert_request_info->host_and_port would match the host and port of the
proxy, not of the request url.   I could DCHECK that it is either of those two
conditions though?

http://codereview.chromium.org/4339001/diff/76002/net/http/http_proxy_client_...
File net/http/http_proxy_client_socket_pool.cc (right):

http://codereview.chromium.org/4339001/diff/76002/net/http/http_proxy_client_...
net/http/http_proxy_client_socket_pool.cc:86: error_response_info_() {
On 2010/11/11 01:11:35, wtc wrote:
> Remove this.  We usually omit this kind of member initializer
> that takes no argument.

Done.

http://codereview.chromium.org/4339001/diff/76002/net/http/http_proxy_client_...
net/http/http_proxy_client_socket_pool.cc:214: return
ERR_SSL_CLIENT_AUTH_CERT_NEEDED;
On 2010/11/11 01:11:35, wtc wrote:
> You can just return 'result' now.

Done.

http://codereview.chromium.org/4339001/diff/76002/net/http/http_proxy_client_...
net/http/http_proxy_client_socket_pool.cc:245: LOG(INFO) << "Connected to HTTPS
proxy, using spdy: " << (using_spdy_ ? "yes" : "no");
On 2010/11/11 01:11:35, wtc wrote:
> Use VLOG(1) instead of LOG(INFO).
> 
> This line seems longer than 80 characters.

Removed this line.

http://codereview.chromium.org/4339001/diff/76002/net/http/http_proxy_client_...
net/http/http_proxy_client_socket_pool.cc:270: if
(transport_socket_handle_.get()) {
On 2010/11/11 01:11:35, wtc wrote:
> Can you explain why you need the null pointer check for
> transport_socket_handle_ here?

Yes.  If HttpProxyClientSocket::Connect() is called when a SPDY session to the
proxy already exists, then DoSSLConnect will jump to this method without
initializing the transport_socket_handle.

http://codereview.chromium.org/4339001/diff/76002/net/http/http_proxy_client_...
net/http/http_proxy_client_socket_pool.cc:282: if
(transport_socket_handle_->socket())
On 2010/11/11 01:11:35, wtc wrote:
> I pointed this out before: after the transport_socket_handle_.release()
> call on line 279 above, transport_socket_handle_ contains
> NULL, so transport_socket_handle_->socket() will crash.

Nice catch!  It turns out the SpdySession destructor will Disconnect the socket
so we don't need this at all.

http://codereview.chromium.org/4339001/diff/76002/net/http/http_stream_request.h
File net/http/http_stream_request.h (right):

http://codereview.chromium.org/4339001/diff/76002/net/http/http_stream_reques...
net/http/http_stream_request.h:143: scoped_refptr<SSLSocketParams>
GenerateSslParams(
On 2010/11/11 01:11:35, wtc wrote:
> Can you see if you can rename GenerateSslParams to
> GenerateSSLParams without making this CL much larger?

Done.

http://codereview.chromium.org/4339001/diff/76002/net/socket/client_socket_po...
File net/socket/client_socket_pool_base_unittest.cc (right):

http://codereview.chromium.org/4339001/diff/76002/net/socket/client_socket_po...
net/socket/client_socket_pool_base_unittest.cc:7: #include "base/basictypes.h"
On 2010/11/11 01:11:35, wtc wrote:
> Please move this #include to client_socket_factory.h.

Done.

http://codereview.chromium.org/4339001/diff/76002/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_mac.cc (right):

http://codereview.chromium.org/4339001/diff/76002/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_mac.cc:728:
X509Certificate::GetSSLClientCertificates(hostname_,
On 2010/11/11 01:11:35, wtc wrote:
> Please add a TODO comment to note that we should consider
> passing a host-port pair as the first argument to
> X509Certificate::GetSSLClientCertificates.

Done.

http://codereview.chromium.org/4339001/diff/76002/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_mac.cc:812: std::string peer_id(hostname_);
On 2010/11/11 01:11:35, wtc wrote:
> IMPORTANT: we should include 'port_' in peer_id.  See
>
http://developer.apple.com/library/mac/documentation/Security/Reference/secur...
> 
> If you don't have time to do this, just add a TODO comment.

Appending port_ into peer_id is pretty trivial, but do I need to do anything to
change consumers of peer id?  I didn't see calls to SSLGetPeerID in net/...

http://codereview.chromium.org/4339001/diff/76002/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/4339001/diff/76002/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_nss.cc:876: // TODO(wtc): port in |peer_address| is
not the server's port when a proxy is
On 2010/11/11 01:11:35, wtc wrote:
> Please fix this TODO by replacing the base::StringPrintf call
> on line 878 with HostPortPair(hostname_, port_).ToString().

Done.

http://codereview.chromium.org/4339001/diff/76002/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_nss.cc:2320: #if !defined(USE_OPENSSL)
On 2010/11/11 01:11:35, wtc wrote:
> I think we should remove this ifdef.  By definition, if
> we use SSLClientSocketNSS, we are not using OpenSSL.

Done.

http://codereview.chromium.org/4339001/diff/76002/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_pool.cc (right):

http://codereview.chromium.org/4339001/diff/76002/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_pool.cc:256: if (result ==
ERR_SSL_CLIENT_AUTH_CERT_NEEDED)
On 2010/11/11 01:11:35, wtc wrote:
> Add curly braces.  This is required by the Style Guide, if
> any branch in an if statement has curly braces.
> 
> Please rewrite the first comment block so that it applies to
> both branches and delete the second comment block.

Done.

http://codereview.chromium.org/4339001/diff/76002/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_pool.cc:294: params_->port(),
On 2010/11/11 01:11:35, wtc wrote:
> Nit: merge these two lines.

Done.

http://codereview.chromium.org/4339001/diff/76002/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_pool.cc:331: LOG(INFO) << "Connected to host
through proxy, over spdy: " << (using_spdy ? "yes" : "no");
On 2010/11/11 01:11:35, wtc wrote:
> Is this line longer than 80 characters?
> 
> Replace LOG(INFO) by VLOG(1).

Sorry, removed the line.

http://codereview.chromium.org/4339001/diff/76002/remoting/jingle_glue/ssl_so...
File remoting/jingle_glue/ssl_socket_adapter.cc (right):

http://codereview.chromium.org/4339001/diff/76002/remoting/jingle_glue/ssl_so...
remoting/jingle_glue/ssl_socket_adapter.cc:70: transport_socket_,
hostname_.c_str(), /*TODO(rch)*/-1, ssl_config,
On 2010/11/11 01:11:35, wtc wrote:
> Isn't 443 better than -1 as a temporary port value?

Done.

[Ryan Hamilton, 2010-11-11 19:05:44]

On 2010/11/08 22:22:25, eroman wrote:
> http://codereview.chromium.org/4339001/diff/47001/1018
> File net/http/http_network_transaction.cc (right):
> 
> http://codereview.chromium.org/4339001/diff/47001/1018#newcode177
> net/http/http_network_transaction.cc:177: host_port =
> proxy_info_.proxy_server().host_port_pair().ToString();
> On 2010/11/04 21:53:18, wtc wrote:
> > eroman: can you review this change?
> 
> I don't understand this change, what is the intended host:port you are trying
to
> get? |prox_info_| could be specifying DIRECT, in which case the host_port_pair
> will be empty

Sorry for not replying earlier.  The short answer here is that this code no
longer exists in the new version of the CL, so don't sweat it :>

But in the interest of completeness, I think that change was correct, because
the only scenario in which ERR_PROXY_SSL_CLIENT_AUTH_CERT_NEEDED could have been
returned was when proxy_info_ pointed to an HTTPS proxy.  Adding a DCHECK to
that effect would probably have been a good idea, and would have made this point
clear.

[eroman, 2010-11-11 19:18:31]

http://codereview.chromium.org/4339001/diff/177001/net/socket/client_socket_f...
File net/socket/client_socket_factory.h (right):

http://codereview.chromium.org/4339001/diff/177001/net/socket/client_socket_f...
net/socket/client_socket_factory.h:49: uint16 port,
Please change the function signature to take a |const HostPortPair&| instead.

The advantage of HostPortPair is it is more expressive, and allows us to do
assertions more uniformly. With naked "hostname" parameters we have run into
problems needing to explain at every single callsite what sort of data it
contains. i.e. does it include square brackets around IPv6 literals? Is it
already canonicalized? etc... By passing around a HostPortPair instead we can
enforce and document those expectations more uniformly in one chokepoint. And
assert at construction time that it is good.

[Ryan Hamilton, 2010-11-11 19:48:26]

On 2010/11/11 19:18:31, eroman wrote:
>
http://codereview.chromium.org/4339001/diff/177001/net/socket/client_socket_f...
> File net/socket/client_socket_factory.h (right):
> 
>
http://codereview.chromium.org/4339001/diff/177001/net/socket/client_socket_f...
> net/socket/client_socket_factory.h:49: uint16 port,
> Please change the function signature to take a |const HostPortPair&| instead.
> 
> The advantage of HostPortPair is it is more expressive, and allows us to do
> assertions more uniformly. With naked "hostname" parameters we have run into
> problems needing to explain at every single callsite what sort of data it
> contains. i.e. does it include square brackets around IPv6 literals? Is it
> already canonicalized? etc... By passing around a HostPortPair instead we can
> enforce and document those expectations more uniformly in one chokepoint. And
> assert at construction time that it is good.

Sounds good.  I assume I should do this for the Socket constructors as well? 
I'll make it so...

[wtc, 2010-11-12 00:12:55]

LGTM.

Note: I will not be able to review this CL again, so please
make the suggested changes below and check this in.

If you'd like me to review any change, please do that in a
follow-up CL.  Thanks.

http://codereview.chromium.org/4339001/diff/76002/net/http/http_network_trans...
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/4339001/diff/76002/net/http/http_network_trans...
net/http/http_network_transaction.cc:175:
session_->ssl_client_auth_cache()->Add(
On 2010/11/11 18:57:00, Ryan Hamilton wrote:
>
> I don't think this would be correct.  In the case of an HTTPS proxy, the
> response_.cert_request_info->host_and_port would match the host and port of
the
> proxy, not of the request url.   I could DCHECK that it is either of those two
> conditions though?

I see.  No need to add any DCHECK then.

http://codereview.chromium.org/4339001/diff/76002/net/http/http_proxy_client_...
File net/http/http_proxy_client_socket_pool.cc (right):

http://codereview.chromium.org/4339001/diff/76002/net/http/http_proxy_client_...
net/http/http_proxy_client_socket_pool.cc:282: if
(transport_socket_handle_->socket())
On 2010/11/11 18:57:00, Ryan Hamilton wrote:
>
> Nice catch!  It turns out the SpdySession destructor will Disconnect the
socket
> so we don't need this at all.

Good.  Please remove the curly braces as well.

http://codereview.chromium.org/4339001/diff/76002/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_mac.cc (right):

http://codereview.chromium.org/4339001/diff/76002/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_mac.cc:812: std::string peer_id(hostname_);
On 2010/11/11 18:57:00, Ryan Hamilton wrote:
>
> Appending port_ into peer_id is pretty trivial, but do I need to do anything
to
> change consumers of peer id?  I didn't see calls to SSLGetPeerID in net/...

No, you don't need to change the consumers of peer id.
The only consumer of peer id is the SSL library (Apple's
Secure Transport library), which treats peer id as an
opaque blob, and the only thing it does with peer ids is
to compare them for equality.

http://codereview.chromium.org/4339001/diff/255001/jingle/notifier/base/chrom...
File jingle/notifier/base/chrome_async_socket.h (right):

http://codereview.chromium.org/4339001/diff/255001/jingle/notifier/base/chrom...
jingle/notifier/base/chrome_async_socket.h:30: class HostPortPair;
Should this forward declaration be removed? HostPortPair is
not used in this file.

http://codereview.chromium.org/4339001/diff/255001/net/socket/ssl_client_sock...
File net/socket/ssl_client_socket_mac.cc (right):

http://codereview.chromium.org/4339001/diff/255001/net/socket/ssl_client_sock...
net/socket/ssl_client_socket_mac.cc:818: host_and_port_.host()_.data(),
Typo: extra '_'

http://codereview.chromium.org/4339001/diff/255001/net/socket/ssl_client_sock...
net/socket/ssl_client_socket_mac.cc:846: std::string
peer_id(host_and_port_.host());
BUG: this should say
  std::string peer_id(host_and_port_.ToString());
and remove line 849.

http://codereview.chromium.org/4339001/diff/255001/net/socket/ssl_client_sock...
File net/socket/ssl_client_socket_win.h (right):

http://codereview.chromium.org/4339001/diff/255001/net/socket/ssl_client_sock...
net/socket/ssl_client_socket_win.h:34: // The given hostname will be compared
with the name(s) in the server's
Nit: the comment "The given hostname" probably should be
updated.

http://codereview.chromium.org/4339001/diff/255001/remoting/jingle_glue/ssl_s...
File remoting/jingle_glue/ssl_socket_adapter.cc (right):

http://codereview.chromium.org/4339001/diff/255001/remoting/jingle_glue/ssl_s...
remoting/jingle_glue/ssl_socket_adapter.cc:71: transport_socket_,
net::HostPortPair(hostname_, /*TODO(rch)*/443),
I think you can remove the TODO comment.  It should be fine
to hardcode port 443 here.  If not, your TODO comment should
elaborate.

http://codereview.chromium.org/4339001/diff/255001/remoting/jingle_glue/ssl_s...
File remoting/jingle_glue/ssl_socket_adapter.h (right):

http://codereview.chromium.org/4339001/diff/255001/remoting/jingle_glue/ssl_s...
remoting/jingle_glue/ssl_socket_adapter.h:8: #include "base/basictypes.h"
Remove this.

[Ryan Hamilton, 2010-11-12 00:47:30]

http://codereview.chromium.org/4339001/diff/76002/net/http/http_proxy_client_...
File net/http/http_proxy_client_socket_pool.cc (right):

http://codereview.chromium.org/4339001/diff/76002/net/http/http_proxy_client_...
net/http/http_proxy_client_socket_pool.cc:282: if
(transport_socket_handle_->socket())
On 2010/11/12 00:12:55, wtc wrote:
> On 2010/11/11 18:57:00, Ryan Hamilton wrote:
> >
> > Nice catch!  It turns out the SpdySession destructor will Disconnect the
> socket
> > so we don't need this at all.
> 
> Good.  Please remove the curly braces as well.

Done.

http://codereview.chromium.org/4339001/diff/255001/jingle/notifier/base/chrom...
File jingle/notifier/base/chrome_async_socket.h (right):

http://codereview.chromium.org/4339001/diff/255001/jingle/notifier/base/chrom...
jingle/notifier/base/chrome_async_socket.h:30: class HostPortPair;
On 2010/11/12 00:12:55, wtc wrote:
> Should this forward declaration be removed? HostPortPair is
> not used in this file.

Done.

http://codereview.chromium.org/4339001/diff/255001/net/socket/ssl_client_sock...
File net/socket/ssl_client_socket_mac.cc (right):

http://codereview.chromium.org/4339001/diff/255001/net/socket/ssl_client_sock...
net/socket/ssl_client_socket_mac.cc:818: host_and_port_.host()_.data(),
On 2010/11/12 00:12:55, wtc wrote:
> Typo: extra '_'

Done.

http://codereview.chromium.org/4339001/diff/255001/net/socket/ssl_client_sock...
net/socket/ssl_client_socket_mac.cc:846: std::string
peer_id(host_and_port_.host());
On 2010/11/12 00:12:55, wtc wrote:
> BUG: this should say
>   std::string peer_id(host_and_port_.ToString());
> and remove line 849.

Done.

http://codereview.chromium.org/4339001/diff/255001/net/socket/ssl_client_sock...
File net/socket/ssl_client_socket_win.h (right):

http://codereview.chromium.org/4339001/diff/255001/net/socket/ssl_client_sock...
net/socket/ssl_client_socket_win.h:34: // The given hostname will be compared
with the name(s) in the server's
On 2010/11/12 00:12:55, wtc wrote:
> Nit: the comment "The given hostname" probably should be
> updated.

Done.

http://codereview.chromium.org/4339001/diff/255001/remoting/jingle_glue/ssl_s...
File remoting/jingle_glue/ssl_socket_adapter.cc (right):

http://codereview.chromium.org/4339001/diff/255001/remoting/jingle_glue/ssl_s...
remoting/jingle_glue/ssl_socket_adapter.cc:71: transport_socket_,
net::HostPortPair(hostname_, /*TODO(rch)*/443),
On 2010/11/12 00:12:55, wtc wrote:
> I think you can remove the TODO comment.  It should be fine
> to hardcode port 443 here.  If not, your TODO comment should
> elaborate.

Done.

http://codereview.chromium.org/4339001/diff/255001/remoting/jingle_glue/ssl_s...
File remoting/jingle_glue/ssl_socket_adapter.h (right):

http://codereview.chromium.org/4339001/diff/255001/remoting/jingle_glue/ssl_s...
remoting/jingle_glue/ssl_socket_adapter.h:8: #include "base/basictypes.h"
On 2010/11/12 00:12:55, wtc wrote:
> Remove this.

Done.

[eroman, 2010-11-12 01:12:56]

lgtm

http://codereview.chromium.org/4339001/diff/248002/net/http/http_stream_reque...
File net/http/http_stream_request.cc (right):

http://codereview.chromium.org/4339001/diff/248002/net/http/http_stream_reque...
net/http/http_stream_request.cc:549:
HostPortPair(request_info().url.HostNoBrackets(),
How about:
HostPortPair::FromURL(request_info().url)

http://codereview.chromium.org/4339001/diff/248002/net/socket/ssl_client_sock...
File net/socket/ssl_client_socket_mac.h (right):

http://codereview.chromium.org/4339001/diff/248002/net/socket/ssl_client_sock...
net/socket/ssl_client_socket_mac.h:31: // The given hostname will be compared
with the name(s) in the server's
nit: should this comment be adjusted to mention port?

http://codereview.chromium.org/4339001/diff/248002/net/socket/ssl_client_sock...
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/4339001/diff/248002/net/socket/ssl_client_sock...
net/socket/ssl_client_socket_nss.cc:522: LOG(ERROR) << "Setting Snap Start info
" << host_and_port_.host();
nit: would it make sense to use the ToString() instead and show the port? (ditto
to other instance in this file).

http://codereview.chromium.org/4339001/diff/248002/net/socket/ssl_client_sock...
net/socket/ssl_client_socket_nss.cc:1142: VLOG(1) << "The server " <<
host_and_port_.host()
ditto on port.

http://codereview.chromium.org/4339001/diff/248002/net/socket/ssl_client_sock...
net/socket/ssl_client_socket_nss.cc:1186: cert_request_info->host_and_port =
host_and_port_.ToString();
It may make sense in the future for cert_request_info to simply be a
HostPortPair.

http://codereview.chromium.org/4339001/diff/248002/net/socket/ssl_client_sock...
File net/socket/ssl_client_socket_nss.h (right):

http://codereview.chromium.org/4339001/diff/248002/net/socket/ssl_client_sock...
net/socket/ssl_client_socket_nss.h:42: // The given hostname will be compared
with the name(s) in the server's
nit: does this comment need to be updated to mention port?

http://codereview.chromium.org/4339001/diff/248002/net/socket/ssl_client_sock...
File net/socket/ssl_client_socket_win.h (right):

http://codereview.chromium.org/4339001/diff/248002/net/socket/ssl_client_sock...
net/socket/ssl_client_socket_win.h:110: HostPortPair hostname_;
Please change the name to say host_and_port_, or something as such.

http://codereview.chromium.org/4339001/diff/248002/net/socket_stream/socket_s...
File net/socket_stream/socket_stream.cc (right):

http://codereview.chromium.org/4339001/diff/248002/net/socket_stream/socket_s...
net/socket_stream/socket_stream.cc:802: HostPortPair(url_.HostNoBrackets(),
url_.EffectiveIntPort()),
Please use HostPortPair::FromURL(url)

[Ryan Hamilton, 2010-11-12 16:47:26]

I've addressed all the commends and will commit after the most recent trybot
runs go green.  Thanks!

http://codereview.chromium.org/4339001/diff/248002/net/http/http_stream_reque...
File net/http/http_stream_request.cc (right):

http://codereview.chromium.org/4339001/diff/248002/net/http/http_stream_reque...
net/http/http_stream_request.cc:549:
HostPortPair(request_info().url.HostNoBrackets(),
On 2010/11/12 01:12:56, eroman wrote:
> How about:
> HostPortPair::FromURL(request_info().url)

Done.  Awesome!

http://codereview.chromium.org/4339001/diff/248002/net/socket/ssl_client_sock...
File net/socket/ssl_client_socket_mac.h (right):

http://codereview.chromium.org/4339001/diff/248002/net/socket/ssl_client_sock...
net/socket/ssl_client_socket_mac.h:31: // The given hostname will be compared
with the name(s) in the server's
On 2010/11/12 01:12:56, eroman wrote:
> nit: should this comment be adjusted to mention port?

Done, for all 3 SSLClientSocket subclasses.

http://codereview.chromium.org/4339001/diff/248002/net/socket/ssl_client_sock...
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/4339001/diff/248002/net/socket/ssl_client_sock...
net/socket/ssl_client_socket_nss.cc:522: LOG(ERROR) << "Setting Snap Start info
" << host_and_port_.host();
On 2010/11/12 01:12:56, eroman wrote:
> nit: would it make sense to use the ToString() instead and show the port?
(ditto
> to other instance in this file).

Done.  (Just the logging, not the various method calls)

http://codereview.chromium.org/4339001/diff/248002/net/socket/ssl_client_sock...
net/socket/ssl_client_socket_nss.cc:1142: VLOG(1) << "The server " <<
host_and_port_.host()
On 2010/11/12 01:12:56, eroman wrote:
> ditto on port.

Done.

http://codereview.chromium.org/4339001/diff/248002/net/socket/ssl_client_sock...
net/socket/ssl_client_socket_nss.cc:1186: cert_request_info->host_and_port =
host_and_port_.ToString();
On 2010/11/12 01:12:56, eroman wrote:
> It may make sense in the future for cert_request_info to simply be a
> HostPortPair.

agreed.  added a todo.

http://codereview.chromium.org/4339001/diff/248002/net/socket/ssl_client_sock...
File net/socket/ssl_client_socket_nss.h (right):

http://codereview.chromium.org/4339001/diff/248002/net/socket/ssl_client_sock...
net/socket/ssl_client_socket_nss.h:42: // The given hostname will be compared
with the name(s) in the server's
On 2010/11/12 01:12:56, eroman wrote:
> nit: does this comment need to be updated to mention port?

Done.

http://codereview.chromium.org/4339001/diff/248002/net/socket/ssl_client_sock...
File net/socket/ssl_client_socket_win.h (right):

http://codereview.chromium.org/4339001/diff/248002/net/socket/ssl_client_sock...
net/socket/ssl_client_socket_win.h:110: HostPortPair hostname_;
On 2010/11/12 01:12:56, eroman wrote:
> Please change the name to say host_and_port_, or something as such.

Done.

http://codereview.chromium.org/4339001/diff/248002/net/socket_stream/socket_s...
File net/socket_stream/socket_stream.cc (right):

http://codereview.chromium.org/4339001/diff/248002/net/socket_stream/socket_s...
net/socket_stream/socket_stream.cc:802: HostPortPair(url_.HostNoBrackets(),
url_.EffectiveIntPort()),
On 2010/11/12 01:12:56, eroman wrote:
> Please use HostPortPair::FromURL(url)

Done.