Disable False Start and clear the SSL client auth cache for HTTPS proxies on failure

When performing TLS client auth with an HTTPS proxy, disable TLS false start to better handle SSL handshake failures, such as the HTTPS proxy requiring a client certificate.

In addition, when an HTTPS proxy fails, ensure that it is removed from the SSL client auth cache, so that if the failure was due to an invalid client certificate, the user can be prompted to select one again.

Depends on: http://codereview.chromium.org/6017010/

BUG=66424
TEST=HttpNetworkTransactionTest.Proxy_ClientAuthCertCache

[Ryan Sleevi, 2011-01-07 01:23:37]

rch: This seems like a less-hacky, medium-term solution for working around proxy
errors. agl seems amenable towards disabling false start under this edge case,
which may help. The net effect is that SSL handshake errors during the proxy
connect would be flagged as ERR_PROXY_CONNECTION_FAILED, which will then hit
HttpStreamRequest::ReconsiderProxyAfterError and allow us to remove the cache.

agl: Are you ok with this? This way, false start is only disabled when sending a
client certificate AND using an HTTPS proxy, which is hopefully less frequent
than the general case of sending a client certificate.

wtc: FYI

The known limitation of this approach is that it DOES NOT address the fact that
an HTTPS proxy may choose to renegotiate at any time and request a client
certificate. If the HTTPS proxy uses TLS renegotiation, we're back to the
original problem of not flushing the certificate. Long term, being able to
request renegotiation seems useful, in order to permit use cases presently
accomplished via HTTP 407s with client certs (for example, user can access
foo.internal and bar.internal anonymously-but-through-the-proxy, but attempting
to access partner.external requires them to auth using a client certificate via
SSL renegotiation).

The other limitation is that, from what I can tell,
HttpNetworkTransaction.ssl_config is re-used between the HttpProxyParams and the
SSLParams, meaning that if you're using an HTTPS proxy which does not require
client auth to talk to an SSL endpoint that does, then False Start will be
disabled, when it would otherwise be fine here.

rch: Can you double check to see if I've missed something? If this is the case,
then I will file and fill in the details for the TODO(rch) flagged
http://crbug.com/FIXME

[agl, 2011-01-07 15:28:28]

LGTM

http://codereview.chromium.org/6120002/diff/5001/net/http/http_network_transa...
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/6120002/diff/5001/net/http/http_network_transa...
net/http/http_network_transaction_unittest.cc:8374: 
extra blank line.

[Ryan Hamilton, 2011-01-07 17:25:50]

LGTM with the following minor changes.

I verified that this patch works when applied to tip of trunk (excellent!).  I
assume it also works in conjunction with your other CL as well
http://codereview.chromium.org/6017010/)?

http://codereview.chromium.org/6120002/diff/2001/net/http/http_network_transa...
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/6120002/diff/2001/net/http/http_network_transa...
net/http/http_network_transaction_unittest.cc:8379: //  3) SSL is being used
through the HTTPS proxy.
Can you add a similar test in which the request URL is http:// not https://?

http://codereview.chromium.org/6120002/diff/2001/net/http/http_stream_request.cc
File net/http/http_stream_request.cc (right):

http://codereview.chromium.org/6120002/diff/2001/net/http/http_stream_request...
net/http/http_stream_request.cc:890: if (proxy_info()->proxy_server().scheme()
== ProxyServer::SCHEME_HTTPS &&
proxy_info()->is_https()

[Ryan Sleevi, 2011-01-09 08:47:33]

On 2011/01/07 17:25:50, Ryan Hamilton wrote:
> LGTM with the following minor changes.
> 
> I verified that this patch works when applied to tip of trunk (excellent!).  I
> assume it also works in conjunction with your other CL as well
> http://codereview.chromium.org/6017010/%29?

Not directly, no. The only dependency between the two is that I built this patch
on it, so that I could re-use the descriptions and naming scheme from the
unittests. This can go in independent of the other though, if necessary.
However, my reaction would be to put the other in first (which is pending wtc's
feedback)

http://codereview.chromium.org/6120002/diff/2001/net/http/http_network_transa...
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/6120002/diff/2001/net/http/http_network_transa...
net/http/http_network_transaction_unittest.cc:8379: //  3) SSL is being used
through the HTTPS proxy.
On 2011/01/07 17:25:50, Ryan Hamilton wrote:
> Can you add a similar test in which the request URL is http:// not https://? 

Done. In practice it shouldn't matter, but you're right in that it's better to
test to ensure this.

http://codereview.chromium.org/6120002/diff/2001/net/http/http_stream_request.cc
File net/http/http_stream_request.cc (right):

http://codereview.chromium.org/6120002/diff/2001/net/http/http_stream_request...
net/http/http_stream_request.cc:890: if (proxy_info()->proxy_server().scheme()
== ProxyServer::SCHEME_HTTPS &&
On 2011/01/07 17:25:50, Ryan Hamilton wrote:
> proxy_info()->is_https()

Already changed in patchset #2, as I noticed immediately after I sent the
feedback.

[Ryan Hamilton, 2011-01-10 17:02:02]

LGTM!  Thanks!

After you commit this, can you see about getting it merged to the branch?

Let's get this merged to 
On 2011/01/09 08:47:33, Ryan Sleevi wrote:
> On 2011/01/07 17:25:50, Ryan Hamilton wrote:
> > LGTM with the following minor changes.
> > 
> > I verified that this patch works when applied to tip of trunk (excellent!). 
I
> > assume it also works in conjunction with your other CL as well
> > http://codereview.chromium.org/6017010/%2529?
> 
> Not directly, no. The only dependency between the two is that I built this
patch
> on it, so that I could re-use the descriptions and naming scheme from the
> unittests. This can go in independent of the other though, if necessary.
> However, my reaction would be to put the other in first (which is pending
wtc's
> feedback)
> 
>
http://codereview.chromium.org/6120002/diff/2001/net/http/http_network_transa...
> File net/http/http_network_transaction_unittest.cc (right):
> 
>
http://codereview.chromium.org/6120002/diff/2001/net/http/http_network_transa...
> net/http/http_network_transaction_unittest.cc:8379: //  3) SSL is being used
> through the HTTPS proxy.
> On 2011/01/07 17:25:50, Ryan Hamilton wrote:
> > Can you add a similar test in which the request URL is http:// not https://?

> 
> Done. In practice it shouldn't matter, but you're right in that it's better to
> test to ensure this.
> 
>
http://codereview.chromium.org/6120002/diff/2001/net/http/http_stream_request.cc
> File net/http/http_stream_request.cc (right):
> 
>
http://codereview.chromium.org/6120002/diff/2001/net/http/http_stream_request...
> net/http/http_stream_request.cc:890: if (proxy_info()->proxy_server().scheme()
> == ProxyServer::SCHEME_HTTPS &&
> On 2011/01/07 17:25:50, Ryan Hamilton wrote:
> > proxy_info()->is_https()
> 
> Already changed in patchset #2, as I noticed immediately after I sent the
> feedback.

[Ryan Sleevi, 2011-01-11 13:59:06]

I wasn't able to get this landed last night or this morning, as this depends on
http://codereview.chromium.org/6017010/. While I should be able to land this
evening, however, if either of you wish to land before then on my behalf, go
ahead.

[wtc, 2011-01-11 23:31:36]

LGTM.  (I didn't review the unit test.)

I have some nits and an important request for a better
long-term solution below.

The long-term solution I proposed, moving the manipulation
(Add and Remove calls) of ssl_client_auth_cache to the
SSLClientSocket class, requires passing ssl_client_auth_cache
to the SSLClientSocket factory and constructor, a tedious
but straightforward work.

http://codereview.chromium.org/6120002/diff/35001/net/http/http_stream_reques...
File net/http/http_stream_request.cc (right):

http://codereview.chromium.org/6120002/diff/35001/net/http/http_stream_reques...
net/http/http_stream_request.cc:920: // http://crbug.com/FIXME
Please replace "FIXME" with the bug number.

Is ssl_config() the SSL config for the destination server
or the HTTPS proxy?  I believe it is the SSL config for
the destination server.  If so, we should open a bug for
adding an SSL config for HTTPS proxy, or convince ourselves
that the destination server and HTTPS proxy can share SSL
config.

http://codereview.chromium.org/6120002/diff/35001/net/http/http_stream_reques...
net/http/http_stream_request.cc:1012: if (proxy_info()->is_https() &&
ssl_config_->send_client_cert) {
Why don't you test for ERR_PROXY_CONNECTION_FAILED here?

Perhaps we should add a new error code
ERR_PROXY_SSL_CLIENT_AUTH_CERT_NEEDED for this case?

Or perhaps we should move the manipulation of
ssl_client_auth_cache to the SSLClientSocket class, rather
than spreading across HttpNetworkTransaction and
HttpStreamRequest?  That seems like the best long-term
solution.

[Ryan Sleevi, 2011-01-11 23:41:46]

On 2011/01/11 23:31:36, wtc wrote:
> LGTM.  (I didn't review the unit test.)
> 
> I have some nits and an important request for a better
> long-term solution below.
> 
> The long-term solution I proposed, moving the manipulation
> (Add and Remove calls) of ssl_client_auth_cache to the
> SSLClientSocket class, requires passing ssl_client_auth_cache
> to the SSLClientSocket factory and constructor, a tedious
> but straightforward work.

Agreed. This also has the benefit that it will not need to abort the connection
per SSL connect to push up the layer, as if the cache can service the request,
it can be done during the current handshake.

Longer term, this would hopefully support moving the certificate verification to
happen after receiving the Certificate, before sending the client certificate.

> 
>
http://codereview.chromium.org/6120002/diff/35001/net/http/http_stream_reques...
> File net/http/http_stream_request.cc (right):
> 
>
http://codereview.chromium.org/6120002/diff/35001/net/http/http_stream_reques...
> net/http/http_stream_request.cc:920: // http://crbug.com/FIXME
> Please replace "FIXME" with the bug number.
> 
> Is ssl_config() the SSL config for the destination server
> or the HTTPS proxy?  I believe it is the SSL config for
> the destination server.  If so, we should open a bug for
> adding an SSL config for HTTPS proxy, or convince ourselves
> that the destination server and HTTPS proxy can share SSL
> config.

Yes, that's the FIXME that was meant to be filed. I had mentioned it to rch via
e-mail and wanted to confirm with him it was a bug. Over the weekend I wrote a
POC CL, which I've copied you two on, but not filed yet. I'd meant to do so
before committing.

>
http://codereview.chromium.org/6120002/diff/35001/net/http/http_stream_reques...
> net/http/http_stream_request.cc:1012: if (proxy_info()->is_https() &&
> ssl_config_->send_client_cert) {
> Why don't you test for ERR_PROXY_CONNECTION_FAILED here?
> 
> Perhaps we should add a new error code
> ERR_PROXY_SSL_CLIENT_AUTH_CERT_NEEDED for this case?
> 
> Or perhaps we should move the manipulation of
> ssl_client_auth_cache to the SSLClientSocket class, rather
> than spreading across HttpNetworkTransaction and
> HttpStreamRequest?  That seems like the best long-term
> solution.

ERR_PROXY_CONNECTION_FAILED is only returned if an error is encountered during
the initial SSL handshake. If the peer requests renegotiation (for example,
after seeing the CONNECT request), then it's bubbled up as-is.

This way erred on the side of invalidating more than necessary, to guarantee it
caught the edges, rather than less than necessary, and potentially bugging.
However, as http://codereview.chromium.org/6120003/ picked up, there are perhaps
other issues with HTTPS proxies that may need to be addressed first.