Ensure that when using False Start + client auth, bad client certificates are not cached

If an SSL handshake fails when client certificates are used, ensure that the client certificate selected is removed from the SSL client auth cache. This ensures that the user is prompted to select a certificate again, as the cause of the failure may have been due to selecting the wrong certificate or selecting no certificate when one is required.

The existing logic worked when TLS False Start was disabled, but could fail when False Start was used or when the peer requests renegotiation. This changes ensures the client certificate is removed from the cache by moving the cache removal layer from the HttpStreamRequest to the HttpNetworkTransaction.

BUG=66424
TEST=HttpNetworkTransactionTest.ClientAuthCertCache*

[Ryan Sleevi, 2011-01-03 15:26:29]

rch, wtc: Please take a look at this unfortunately delayed CL.

rch: I've added a TODO(you), because this code does not support HTTPS proxies
that require client auth. I'm not happy about that, and certainly not trying to
create more work for you, I just didn't want to delay this CL any further
chasing after the perfect. It's something I do plan to work on, unless you
suggest otherwise, but I didn't want to hold you up from implementing it
yourself if necessary.

I'm not thrilled with the duplication of code between the tests, and may pull
some of the duplication into a helper method, but if it's acceptable, I'll hold
off.

Also, please check the style, most notably commenting in the
SSLSocketDataProviders. Is commenting the async bool acceptable, or is that an
agl-ism and not recommended?

[Ryan Hamilton, 2011-01-04 23:40:47]

On 2011/01/03 15:26:29, Ryan Sleevi wrote:
> 
> rch: I've added a TODO(you), because this code does not support HTTPS proxies
> that require client auth. I'm not happy about that, and certainly not trying
to
> create more work for you, I just didn't want to delay this CL any further
> chasing after the perfect. It's something I do plan to work on, unless you
> suggest otherwise, but I didn't want to hold you up from implementing it
> yourself if necessary.

Heh, as it happens, the reason I need this fixed is for an HTTPS Proxy that
requires client auth :>  I like this CL overall, but I wonder how you intend to
handle the HTTPS Proxy case.  In this CL, it does not seem like the certificate
request (which includes the host:port of either the proxy or the origin) is
necessarily visible in HandleSSLHandshakeError.  What's your thinking about how
to thread this through to HttpNetworkTransaction?

[Ryan Sleevi, 2011-01-05 00:11:21]

That's what I was thinking you were doing (based on the hostname from your log),
and part of why I spent so long futzing with this. I think this CL is still good
for the non-HTTPS, False Start SSL endpoint, and while existing, is much more
likely to be tickled with the NULL auth cache, which is why I wanted to get it
up.

Since you asked though.... my current in-progress work is (to be submitted as
several CLs, don't worry)

1) Change SSLCertRequestInfo to indicate whether or not a certificate was
definitively requested. Presently, NULL is used as part of the HttpResponseInfo
to indicate no certificate requested, and the request information is only
gathered when ERR_SSL_CLIENT_AUTH_CERT_NEEDED. If
ERR_SSL_CLIENT_AUTH_CERT_NEEDED isn't returned, then the HttpResponseInfo won't
contain anything about the request. As exposed via SSLClientSocket* today,
there's no way when collecting the results to know if a certificate was
requested for the most-recent handshake. It might be stale info (eg: first
handshake, subsequent renegotiation hasn't required one) and there is no way to
distinguish between empty client_certs == no request and empty client_certs ==
no CA names sent in the CertificateRequest/No matching client certs.

2) Change SSLClientSocket* to expose this information independent of Connect()
status. Presently, the only way to find out if a certificate was requested is
when ERR_SSL_CLIENT_AUTH_CERT_NEEDED is returned. Rather than only exposing this
information during such an error, always expose it. This is more
semantic/documentation changes than implementation.

3) Change [Http]StreamRequest to expose the (new) SSLCertRequestInfo regardless
of return code as part of the public interface, similar to exposing the spdy
status or npn status. As the different layers are built up (HTTPS proxy ->
(write "CONNECT" || SPDY handshake) -> endpoint), the information will be
updated with the current layer's results. So if the proxy requires client auth,
ERR_SSL_CLIENT_AUTH_CERT_NEEDED will be returned, and GetSSLCertRequestInfo()
returns the proxy's request. After the proxy is connected, and the endpoint
handshake is occurring, GetSSLCertRequestInfo() returns the endpoints
information. The reason for exposing it as a method/accessor rather than adding
a method to the Delegate() is two-fold: One, not all cert requests are terminal
(whereas all of the StreamRequest::Delegate methods are), and Two, the absence
of a request is just as meaningful as the presence of a request, so it means
it'd be a method that is always called, with a bool indicating whether or not a
cert was actually requested. I looked at exposing it via the ClientSocketHandle
as part of the extended error information, but a certificate request isn't in
and of itself an error. 

4) Update the HttpNetworkTransaction::HandleSSLHandshakeError to look in
|stream_| or |stream_request_| for the cert request info (whichever is valid),
and use that to make the decision about who to eject from the cache, as opposed
to request_.


As far as the above design goes, it should work for both layers. It has two
limitations I can spot:
1) If the HTTPS proxy performs a renegotiation handshake after the connection is
established, it can be hard for the HttpStream to distinguish which layer is
renegotiating. This is already a problem w/r/t TLS intolerance / handshake
errors, but it would be a known limitation.
2) I've been reading over the SPDY implementation trying to understand the
protocol, so I'm not sure yet if this will be an issue. I'm fairly confident it
won't be for HTTP[S] proxies, so it's just SPDY that concerns me. My concern is
that if no Read() happens on the SPDY connection before the endpoint SSL
handshake happens, then it's possible for (Proxy Snap Start, SPDY Connect,
Endpoint Snap Start) to all be sent at once, and the SSL protocol errors may not
be distinguishable between the Proxy and the Endpoint. This would apply to both
client auth and to TLS intolerance, but I suspect it could be an issue once Snap
Start is enabled.


Part of the reason for exposing the client cert request info all the time is to
address http://crbug.com/29784.

[Ryan Hamilton, 2011-01-05 17:35:02]

LGTM, with a couple of test nits.  Please make sure that this looks good to wtc,
of course.

http://codereview.chromium.org/6017010/diff/13001/net/http/http_network_trans...
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/6017010/diff/13001/net/http/http_network_trans...
net/http/http_network_transaction_unittest.cc:8218: // [ssl_]data1 = Aborted
handshake for client auth, TLSv1
Would it make sense to elaborate on the comment here?  If I'm reading this code
right, the test does the following:
1) Attempts to establish an SSL connection without providing a SSL client
certificate.  This causes a failure with the error code
ERR_SSL_CLIENT_AUTH_CERT_NEEDED.
2) Reconnect, and decline to provide a certificate (which should have similar
behavior to providing a client certificate that the server rejects) by setting a
NULL entry in the SSL cache.   This should result in ERR_SSL_PROTOCOL_ERROR and
the entry from the cache should be removed.

Is that right?

I assume, also, that the second test is basically doing the same thing, except
that the ssl errors are coming not from the connect, but rather from the reads?

http://codereview.chromium.org/6017010/diff/13001/net/http/http_network_trans...
net/http/http_network_transaction_unittest.cc:8220: // [ssl_]data3 = Failed
handshake, SSLv3 fallback (http://crbug.com/66517)
Is ssl_data3 used?  I only see one place where ERR_SSL_PROTOCOL_ERROR is
checked, but I would have expected to see 2 places...

[Ryan Hamilton, 2011-01-05 17:37:33]

On 2011/01/05 00:11:21, Ryan Sleevi wrote:
> That's what I was thinking you were doing (based on the hostname from your
log),
> and part of why I spent so long futzing with this. I think this CL is still
good
> for the non-HTTPS, False Start SSL endpoint, and while existing, is much more
> likely to be tickled with the NULL auth cache, which is why I wanted to get it
> up.

Sounds like the right approach.

> Since you asked though.... my current in-progress work is (to be submitted as
> several CLs, don't worry)
> 
> 1) Change SSLCertRequestInfo to indicate whether or not a certificate was
> definitively requested. Presently, NULL is used as part of the
HttpResponseInfo
> to indicate no certificate requested, and the request information is only
> gathered when ERR_SSL_CLIENT_AUTH_CERT_NEEDED. If
> ERR_SSL_CLIENT_AUTH_CERT_NEEDED isn't returned, then the HttpResponseInfo
won't
> contain anything about the request. As exposed via SSLClientSocket* today,
> there's no way when collecting the results to know if a certificate was
> requested for the most-recent handshake. It might be stale info (eg: first
> handshake, subsequent renegotiation hasn't required one) and there is no way
to
> distinguish between empty client_certs == no request and empty client_certs ==
> no CA names sent in the CertificateRequest/No matching client certs.
> 
> 2) Change SSLClientSocket* to expose this information independent of Connect()
> status. Presently, the only way to find out if a certificate was requested is
> when ERR_SSL_CLIENT_AUTH_CERT_NEEDED is returned. Rather than only exposing
this
> information during such an error, always expose it. This is more
> semantic/documentation changes than implementation.
> 
> 3) Change [Http]StreamRequest to expose the (new) SSLCertRequestInfo
regardless
> of return code as part of the public interface, similar to exposing the spdy
> status or npn status. As the different layers are built up (HTTPS proxy ->
> (write "CONNECT" || SPDY handshake) -> endpoint), the information will be
> updated with the current layer's results. So if the proxy requires client
auth,
> ERR_SSL_CLIENT_AUTH_CERT_NEEDED will be returned, and GetSSLCertRequestInfo()
> returns the proxy's request. After the proxy is connected, and the endpoint
> handshake is occurring, GetSSLCertRequestInfo() returns the endpoints
> information. The reason for exposing it as a method/accessor rather than
adding
> a method to the Delegate() is two-fold: One, not all cert requests are
terminal
> (whereas all of the StreamRequest::Delegate methods are), and Two, the absence
> of a request is just as meaningful as the presence of a request, so it means
> it'd be a method that is always called, with a bool indicating whether or not
a
> cert was actually requested. I looked at exposing it via the
ClientSocketHandle
> as part of the extended error information, but a certificate request isn't in
> and of itself an error. 
> 
> 4) Update the HttpNetworkTransaction::HandleSSLHandshakeError to look in
> |stream_| or |stream_request_| for the cert request info (whichever is valid),
> and use that to make the decision about who to eject from the cache, as
opposed
> to request_.
> 
> 
> As far as the above design goes, it should work for both layers. It has two
> limitations I can spot:
> 1) If the HTTPS proxy performs a renegotiation handshake after the connection
is
> established, it can be hard for the HttpStream to distinguish which layer is
> renegotiating. This is already a problem w/r/t TLS intolerance / handshake
> errors, but it would be a known limitation.
> 2) I've been reading over the SPDY implementation trying to understand the
> protocol, so I'm not sure yet if this will be an issue. I'm fairly confident
it
> won't be for HTTP[S] proxies, so it's just SPDY that concerns me. My concern
is
> that if no Read() happens on the SPDY connection before the endpoint SSL
> handshake happens, then it's possible for (Proxy Snap Start, SPDY Connect,
> Endpoint Snap Start) to all be sent at once, and the SSL protocol errors may
not
> be distinguishable between the Proxy and the Endpoint. This would apply to
both
> client auth and to TLS intolerance, but I suspect it could be an issue once
Snap
> Start is enabled.

*oof*  That sounds like a good path, but wow, it sounds like a lot of work. 
Thanks for working on this!  What is your guess about when we might expect to
see HTTPS proxies + client auth + caching working again?

> Part of the reason for exposing the client cert request info all the time is
to
> address http://crbug.com/29784.

[Ryan Hamilton, 2011-01-05 19:30:01]

Would it be worth adding a block of code like this:

    if (proxy_info_.is_https())
      session_->ssl_client_auth_cache()->Remove(
          proxy_info_.proxy_server().host_port_pair().ToString());

to HandleSSLHandshakeError?  It's "wrong" in the sense that it'll clear the
cache for the proxy mapping, but at least is will clear the cache when neeeded
and so perhaps it's ok in the short term?

(After applying your patch, I'm able to workaround this bug in the HTTPS proxy
case by visiting https://proxy:port in the address bar.  This ends up clearing
the cache and prompting for the cert correctly.  So once your CL lands we'll
have a grotty workaround)

[Ryan Sleevi, 2011-01-06 07:13:51]

On 2011/01/05 19:30:01, Ryan Hamilton wrote:
> Would it be worth adding a block of code like this:
> 
>     if (proxy_info_.is_https())
>       session_->ssl_client_auth_cache()->Remove(
>           proxy_info_.proxy_server().host_port_pair().ToString());
> 
> to HandleSSLHandshakeError?  It's "wrong" in the sense that it'll clear the
> cache for the proxy mapping, but at least is will clear the cache when neeeded
> and so perhaps it's ok in the short term?
 
It would allow a malicious endpoint to constantly force the client to
re-authenticate with a proxy. As well, any navigating to a broken endpoint (eg:
TLS intolerant, DEFLATE intolerant) would also force a proxy re-auth. Is that a
security/usability concern?

It feels dirty/hacky, but there is no question it would work and clear the
cache. It's up to you + wtc to judge the pragmatic trade-offs - I'm not sure how
many end users will be affected by it either way.

> (After applying your patch, I'm able to workaround this bug in the HTTPS proxy
> case by visiting https://proxy:port in the address bar.  This ends up clearing
> the cache and prompting for the cert correctly.  So once your CL lands we'll
> have a grotty workaround)

Right, directly accessing the site should work fine, and as long as SPDY doesn't
care about HTTP framing on the same port, it's fine in the short-term.

[Ryan Hamilton, 2011-01-06 17:34:34]

On Wed, Jan 5, 2011 at 11:13 PM, <rsleevi@chromium.org> wrote:

> On 2011/01/05 19:30:01, Ryan Hamilton wrote:
>
>> Would it be worth adding a block of code like this:
>>
>
>      if (proxy_info_.is_https())
>>       session_->ssl_client_auth_cache()->Remove(
>>           proxy_info_.proxy_server().host_port_pair().ToString());
>>
>
>  to HandleSSLHandshakeError?  It's "wrong" in the sense that it'll clear
>> the
>> cache for the proxy mapping, but at least is will clear the cache when
>> neeeded
>> and so perhaps it's ok in the short term?
>>
>
> It would allow a malicious endpoint to constantly force the client to
> re-authenticate with a proxy. As well, any navigating to a broken endpoint
> (eg:
> TLS intolerant, DEFLATE intolerant) would also force a proxy re-auth. Is
> that a
> security/usability concern?
>
> It feels dirty/hacky, but there is no question it would work and clear the
> cache. It's up to you + wtc to judge the pragmatic trade-offs - I'm not
> sure how
> many end users will be affected by it either way.


Agreed that it is ugly, dirty, and hideous.  I think it affects very few
people (other than me :>) currently, so I'd love to see something quick and
dirty happen soon rather than waiting for your elegant solution later (which
will of course happen in any case).  wtc, what's your take?


> (After applying your patch, I'm able to workaround this bug in the HTTPS
>> proxy
>> case by visiting https://proxy:port in the address bar.  This ends up
>> clearing
>> the cache and prompting for the cert correctly.  So once your CL lands
>> we'll
>> have a grotty workaround)
>>
>
> Right, directly accessing the site should work fine, and as long as SPDY
> doesn't
> care about HTTP framing on the same port, it's fine in the short-term.


Great.

[Ryan Sleevi, 2011-01-06 23:55:19]

+cc agl: In response to your e-mail, this is a CL that addresses client auth +
false start without having to disable false start.

[agl, 2011-01-07 15:34:43]

On Thu, Jan 6, 2011 at 6:55 PM,  <rsleevi@chromium.org> wrote:
> +cc agl: In response to your e-mail, this is a CL that addresses client auth
> +
> false start without having to disable false start.
>
> http://codereview.chromium.org/6017010/

Do we intend to merge this for Chrome 9? (Is there a bug opened for the merge?)


AGL

[Ryan Sleevi, 2011-01-09 07:10:37]

On 2011/01/07 15:34:43, agl wrote:
> Do we intend to merge this for Chrome 9? (Is there a bug opened for the
merge?)
> 
> 
> AGL

Beats me. I'd like to, and feel better since this adds tests to cover it, but
the bug is currently flagged as M10.

[rch (use chromium not google), 2011-01-10 17:04:07]

On Sat, Jan 8, 2011 at 11:10 PM, <rsleevi@chromium.org> wrote:

> On 2011/01/07 15:34:43, agl wrote:
>
>> Do we intend to merge this for Chrome 9? (Is there a bug opened for the
>>
> merge?)
>
>
>  AGL
>>
>
> Beats me. I'd like to, and feel better since this adds tests to cover it,
> but
> the bug is currently flagged as M10.


Since this is a regression, I think we should definitely merge it to the
branch.  Do you know how to make that happen?

[agl, 2011-01-10 17:19:26]

On Mon, Jan 10, 2011 at 12:04 PM, Ryan Hamilton <rch@google.com> wrote:
> Since this is a regression, I think we should definitely merge it to the
> branch.  Do you know how to make that happen?

At the moment I believe that the branch is closed to try and track
down a crasher. However, I've marked the bug and will merge when the
branch reopens.


AGL

[agl, 2011-01-10 22:08:23]

On Mon, Jan 10, 2011 at 12:19 PM, Adam Langley <agl@chromium.org> wrote:
> At the moment I believe that the branch is closed to try and track
> down a crasher. However, I've marked the bug and will merge when the
> branch reopens.

The branch is open, however this change doesn't appear to have landed
in the tree yet.


AGL

[Ryan Sleevi, 2011-01-10 23:18:24]

On 2011/01/10 22:08:23, agl wrote:
> The branch is open, however this change doesn't appear to have landed
> in the tree yet.
> 
> 
> AGL

agl: Was holding off for wtc's OK, as Ryan's LGTM was conditional on wtc's
review. If wtc's review is not needed (or if he's ok with it), I can land this
evening into trunk. If it's OK for merge (does laforge or someone need to
sign-off?), then I can look to merge it into m9 tonight as well.

[agl, 2011-01-10 23:22:42]

On Mon, Jan 10, 2011 at 6:18 PM,  <rsleevi@chromium.org> wrote:
> agl: Was holding off for wtc's OK, as Ryan's LGTM was conditional on wtc's
> review. If wtc's review is not needed (or if he's ok with it), I can land
> this
> evening into trunk. If it's OK for merge (does laforge or someone need to
> sign-off?), then I can look to merge it into m9 tonight as well.

You should ping wtc directly (although he might still be away) to see
if he can look at it. Otherwise, we should land tomorrow.

(and, yes, laforge needs to sign off on all merges to the branch.)


AGL

[wtc, 2011-01-10 23:40:58]

rsleevi: thanks for writing this CL.  It seems correct.
I'd like to suggest some changes and ask some questions
first.

I didn't review http_network_transaction_unittest.cc in
detail.

In the description of your CL, "moving the cache removal
logic up a layer" is not clear.  Please be more specific
(move from HttpStreamRequest to HttpNetworkTransaction?
or centralize from multiple methods to HandleIOError?).

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

http://codereview.chromium.org/6017010/diff/25001/net/http/http_network_trans...
net/http/http_network_transaction.cc:1015: // being used
(http://crbug.com/66424)
You should point out what the issue is.  I believe the issue
is that GetHostAndPort(request_->url) is the wrong auth target
for an SSL proxy.

http://codereview.chromium.org/6017010/diff/25001/net/http/http_network_trans...
net/http/http_network_transaction.cc:1048: int
HttpNetworkTransaction::HandleIOError(int error) {
1. Having HandleIOError call HandleSSLHandshakeError means
we may be calling HandleSSLHandshakeError where it's not
necessary, for example, in DoInitStreamComplete.  This is
probably OK.  I just wanted to make sure you considered this
point.

Or do we actually need to call HandleSSLHandshakeError in
DoInitStreamComplete because we removed the
HandleSSLHandshakeError call from
HttpStreamRequest::DoInitConnectionComplete?

2. If HandleIOError calls HandleSSLHandshakeError, then the
function name "HandleIOError" is no longer accurate.  The
function should be renamed HandleSSLHandshakeAndIOError
(or a better name).

http://codereview.chromium.org/6017010/diff/25001/net/http/http_network_trans...
net/http/http_network_transaction.cc:1066: case
ERR_SSL_SNAP_START_NPN_MISPREDICTION:
Please move this case into HandleSSLHandshakeError.

http://codereview.chromium.org/6017010/diff/25001/net/http/http_network_trans...
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/6017010/diff/25001/net/http/http_network_trans...
net/http/http_network_transaction_unittest.cc:8217:
TEST_F(HttpNetworkTransactionTest, Direct_ClientAuthCertCache_NoFalseStart) {
Nit: name the unit tests with "ClientAuthCertCache" first:
  ClientAuthCertCache_Direct_NoFalseStart
  ClientAuthCertCache_Direct_FalseStart
because these are unit tests for ClientAuthCertCache.

http://codereview.chromium.org/6017010/diff/25001/net/socket/socket_test_util.cc
File net/socket/socket_test_util.cc (right):

http://codereview.chromium.org/6017010/diff/25001/net/socket/socket_test_util...
net/socket/socket_test_util.cc:19: #include "net/base/ssl_cert_request_info.h"
Nit: list "net/base/ssl_cert_request_info.h" before
"net/base/ssl_info.h".

http://codereview.chromium.org/6017010/diff/25001/net/socket/socket_test_util...
net/socket/socket_test_util.cc:545: cert_request_info->client_certs.clear();
It's better to add a Reset() method to SSLCertRequestInfo
and then just call
  cert_request_info->Reset();

See line 532 above.

[wtc, 2011-01-10 23:52:30]

rsleevi: just wanted to point out that, except for the
TODO(rch) issue with an SSL proxy, and the issue of having
HandleIOError call HandleSSLHandshakeError (which may result
in handling SSL handshake errors where it's not necessary),
all the other issues I found are just nits.  So your CL is
actually in good shape and you may commit it on the trunk
after addressing my comments without a new review.  (Just
upload a new patch set before you do so.)

Please do not merge to a release branch until the CL has
been tested on the trunk for at least a couple of days.

[Ryan Sleevi, 2011-01-11 01:37:47]

wtc: Thanks for the feedback. I've made all the changes you requested but one,
with regards to renaming HandleIOError. In addition to my response below, I
added a comment to the file explaining this, which is hopefully acceptable.

I'll look to land this and http://codereview.chromium.org/6120002/ once the bots
are green and the tree is open.

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

http://codereview.chromium.org/6017010/diff/25001/net/http/http_network_trans...
net/http/http_network_transaction.cc:1048: int
HttpNetworkTransaction::HandleIOError(int error) {
On 2011/01/10 23:40:58, wtc wrote:
> 1. Having HandleIOError call HandleSSLHandshakeError means
> we may be calling HandleSSLHandshakeError where it's not
> necessary, for example, in DoInitStreamComplete.  This is
> probably OK.  I just wanted to make sure you considered this
> point.
> 
> Or do we actually need to call HandleSSLHandshakeError in
> DoInitStreamComplete because we removed the
> HandleSSLHandshakeError call from
> HttpStreamRequest::DoInitConnectionComplete?

If IO can happen, then a check for SSL errors needs to happen, since an SSL
renegotiation may be initiated at any time by the peer. In practice, AFAICT, no
IO happens for the InitStream scenario, but given that HandleIOError is checking
for IO-layer errors, then it's appropriate to also be checking for issues at the
SSL transport layer.

> 
> 2. If HandleIOError calls HandleSSLHandshakeError, then the
> function name "HandleIOError" is no longer accurate.  The
> function should be renamed HandleSSLHandshakeAndIOError
> (or a better name).

I would have thought that, from the perspective of HttpNetworkTransaction, SSL
errors are no different than IO errors. That is, all SSL handshake errors (or
SSL errors, for that matter), are just a different type of IO error. This is
because, at any time in the Read stream, an SSL handshake error may occur. 

The main reason that I split it into two methods, rather than sticking all of
the logic in HandleIOError(), is that the comment of HandleIOError was
unambiguously clear that it should not be called in the Connect() case. Since a
non-false-start connection may complete as part of DoCreateStreamComplete() -
that is, an underlying call to Connect() - I split it into the two methods so
that the former could be called for both Connect() [which can be hit when false
start is disabled] and Read/Write [which can be hit either when using False
Start or the peer requests renegotiation]

[Ryan Sleevi, 2011-01-11 13:57:25]

Between tree closures and a Mac compile issue, I was not able to get this landed
last night. I was hoping to land this morning, but the tree is still closed and
won't be open in time for me to land. While I should be able to land it this
evening, if needed sooner (to be able to merge), I'd ask one of you to land on
my behalf. At the moment, this is based off r71009, but should be find for ToT.

[agl, 2011-01-11 18:49:11]

On Tue, Jan 11, 2011 at 8:57 AM,  <rsleevi@chromium.org> wrote:
> evening, if needed sooner (to be able to merge), I'd ask one of you to land
> on
> my behalf. At the moment, this is based off r71009, but should be find for
> ToT.

Have sent to try server. Will land shortly.


AGL

[agl, 2011-01-11 20:01:51]

On Tue, Jan 11, 2011 at 1:49 PM, Adam Langley <agl@chromium.org> wrote:
> Have sent to try server. Will land shortly.

Landed as r71071.


AGL

[Ryan Hamilton, 2011-01-11 21:04:36]

Awesome, thanks!  Would you like to land
http://codereview.chromium.org/6120002/ also, or shall I?  (That's the CL
which disables false start for HTTPS Proxies that request client auth)

On Tue, Jan 11, 2011 at 12:01 PM, Adam Langley <agl@chromium.org> wrote:

> On Tue, Jan 11, 2011 at 1:49 PM, Adam Langley <agl@chromium.org> wrote:
> > Have sent to try server. Will land shortly.
>
> Landed as r71071.
>
>
> AGL
>

[agl, 2011-01-11 21:06:49]

On Tue, Jan 11, 2011 at 4:04 PM, Ryan Hamilton <rch@chromium.org> wrote:
> Awesome, thanks!  Would you like to
> land http://codereview.chromium.org/6120002/ also, or shall I?  (That's the
> CL which disables false start for HTTPS Proxies that request client auth)

Yes, I'll land that as well. I think that we should also merge this
for Chrome 9. Does anyone disagree?


AGL

[rch (use chromium not google), 2011-01-11 21:22:31]

On Tue, Jan 11, 2011 at 1:06 PM, Adam Langley <agl@chromium.org> wrote:

> On Tue, Jan 11, 2011 at 4:04 PM, Ryan Hamilton <rch@chromium.org> wrote:
> > Awesome, thanks!  Would you like to
> > land http://codereview.chromium.org/6120002/ also, or shall I?  (That's
> the
> > CL which disables false start for HTTPS Proxies that request client auth)
>
> Yes, I'll land that as well. I think that we should also merge this
> for Chrome 9. Does anyone disagree?
>

Awesome.  (I totally agree with your proposal to merge it to Chrome 9)

[agl, 2011-01-11 22:06:28]

On Tue, Jan 11, 2011 at 4:22 PM, Ryan Hamilton <rch@google.com> wrote:
> Awesome.  (I totally agree with your proposal to merge it to Chrome 9)

Landed as r71096.


AGL

[wtc, 2011-01-12 00:21:09]

LGTM.

http://codereview.chromium.org/6017010/diff/58001/net/base/ssl_cert_request_i...
File net/base/ssl_cert_request_info.h (right):

http://codereview.chromium.org/6017010/diff/58001/net/base/ssl_cert_request_i...
net/base/ssl_cert_request_info.h:25: // Resets the SSLCertRequestInfo as if no
certificate had been requested.
You just need to say "to zero/null/empty values".  Actually
the Reset method doesn't need documenting...

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

http://codereview.chromium.org/6017010/diff/58001/net/http/http_network_trans...
net/http/http_network_transaction.cc:1065: // cause SSL handshake errors to be
delayed until the first Read on the
This should be "first Write or Read", right?

http://codereview.chromium.org/6017010/diff/58001/net/socket/socket_test_util.cc
File net/socket/socket_test_util.cc (right):

http://codereview.chromium.org/6017010/diff/58001/net/socket/socket_test_util...
net/socket/socket_test_util.cc:541: cert_request_info->client_certs =
data_->cert_request_info->client_certs;
Would it be better to just do a struct copy here?
    *cert_request_info = *data_->cert_request_info;

[Ryan Sleevi, 2011-01-12 00:40:07]

http://codereview.chromium.org/6017010/diff/58001/net/socket/socket_test_util.cc
File net/socket/socket_test_util.cc (right):

http://codereview.chromium.org/6017010/diff/58001/net/socket/socket_test_util...
net/socket/socket_test_util.cc:541: cert_request_info->client_certs =
data_->cert_request_info->client_certs;
On 2011/01/12 00:21:09, wtc wrote:
> Would it be better to just do a struct copy here?
>     *cert_request_info = *data_->cert_request_info;

Yes. However, because SSLCertRequestInfo is RefCountedThreadSafe, the implicit
assignment operator is private (because RefCountedThreadSafe operator= is
private). Looking at the usage in net/, it probably doesn't need to be
refcounted at all, since most of the layers that use the SSLCertRequestInfo make
copies before returning to the caller (for safety reasons, presumably, since the
request info may change)

This was the cause of my Mac compile woes in the the earlier patchset, and
rather than shoe-horn in more changes, I decided it was better to submit a
separate CL and comprehensively address it. It also didn't seem appropriate to
use this CL to bring in that change, and I wanted to get this landed before M10
(and M9), rather than introduce another dependency.

[agl, 2011-01-12 17:53:55]

On Tue, Jan 11, 2011 at 5:06 PM, Adam Langley <agl@chromium.org> wrote:
> Landed as r71096.

laforge has rejected these patches for merging to Chrome 9. Does
anyone care to press the case?


AGL

[agl, 2011-01-12 18:10:44]

On Wed, Jan 12, 2011 at 1:05 PM, Ryan Hamilton <rch@chromium.org> wrote:
> Yes, I really do!  Should I contact him directly, or is there a thread I
> should jump into?

You should contact him directly. The commits are r71071 and r71096.


AGL

[rsleevi-old, 2011-01-12 18:23:24]

I won't really be able to reply in-depth until this evening, but this is
going from memory:

While the issue was originally detected in the context of False Start and
HTTPS proxies, I'm fairly confident the bug also affected situations where
the peer doesn't request a certificate for the initial SSL handshake, let's
the client send the request, and then requests renegotiation. If the client
certificate supplied is unacceptable then, it wouldn't be flushed from the
cache.

The above scenario is the dominant means of Apache + mod_ssl client auth
when using a VHost (secure.site.com) or a secure directory (site.com/login).
This is because Apache needs to process the Host header for the vhost or the
request URI for the secure directory, and then applies the
directory-specific configuration by means of requesting an SSL
renegotiation.

I unfortunately can't check the sources right now, but IIRC, the only place
that was removing from the cache was the HttpStreamRequest, which goes away
before the request has been written. HttpNetworkTransaction then uses
HttpStream to send the request. HttpNetworkTransaction handled the peer
requesting a client certificate when none was supplied
(ERR_SSL_CLIENT_AUTH_CERT_NEEDED), but didn't handle if the user selected a
bad one (ERR_BAD_SSL_CLIENT_AUTH_CERT/ERR_SSL_PROTOCOL)

The CL for this issue addressed that case, by making HttpNetworkTransaction
responsible for flushing, rather than the short-lived HttpStreamRequest, so
I think this one would affect more users.

The second CL (specifically for HTTPS proxies) I don't feel strongly about.

On Wed, Jan 12, 2011 at 12:53 PM, Adam Langley <agl@chromium.org> wrote:

> On Tue, Jan 11, 2011 at 5:06 PM, Adam Langley <agl@chromium.org> wrote:
> > Landed as r71096.
>
> laforge has rejected these patches for merging to Chrome 9. Does
> anyone care to press the case?
>
>
> AGL
>

[Ryan Hamilton, 2011-01-12 18:36:47]

Yes, I really do!  Should I contact him directly, or is there a thread I
should jump into?

On Wed, Jan 12, 2011 at 9:53 AM, Adam Langley <agl@chromium.org> wrote:

> On Tue, Jan 11, 2011 at 5:06 PM, Adam Langley <agl@chromium.org> wrote:
> > Landed as r71096.
>
> laforge has rejected these patches for merging to Chrome 9. Does
> anyone care to press the case?
>
>
> AGL
>

[rch (use chromium not google), 2011-01-12 18:37:54]

On Wed, Jan 12, 2011 at 10:10 AM, Adam Langley <agl@chromium.org> wrote:

> On Wed, Jan 12, 2011 at 1:05 PM, Ryan Hamilton <rch@chromium.org> wrote:
> > Yes, I really do!  Should I contact him directly, or is there a thread I
> > should jump into?
>
> You should contact him directly. The commits are r71071 and r71096.
>

Thanks to wtc, laforge agreed to let these in.  AGL/rsleevi, I can drover
them unless either of you would like to?

Cheers,

Ryan

[agl, 2011-01-12 18:42:15]

On Wed, Jan 12, 2011 at 1:37 PM, Ryan Hamilton <rch@google.com> wrote:
> Thanks to wtc, laforge agreed to let these in.  AGL/rsleevi, I can drover
> them unless either of you would like to?

Go for it.


AGL

[rch (use chromium not google), 2011-01-12 18:49:55]

Will do!

On Wed, Jan 12, 2011 at 10:42 AM, Adam Langley <agl@chromium.org> wrote:

> On Wed, Jan 12, 2011 at 1:37 PM, Ryan Hamilton <rch@google.com> wrote:
> > Thanks to wtc, laforge agreed to let these in.  AGL/rsleevi, I can drover
> > them unless either of you would like to?
>
> Go for it.
>
>
> AGL
>

[rch (use chromium not google), 2011-01-12 19:08:01]

Ok, I have merged both of these changes as 71200 and 71201.

Rockin'

On Wed, Jan 12, 2011 at 10:49 AM, Ryan Hamilton <rch@google.com> wrote:

> Will do!
>
> On Wed, Jan 12, 2011 at 10:42 AM, Adam Langley <agl@chromium.org> wrote:
>
>> On Wed, Jan 12, 2011 at 1:37 PM, Ryan Hamilton <rch@google.com> wrote:
>> > Thanks to wtc, laforge agreed to let these in.  AGL/rsleevi, I can
>> drover
>> > them unless either of you would like to?
>>
>> Go for it.
>>
>>
>> AGL
>>
>
>

[agl, 2011-01-13 16:02:04]

LGTM