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

net/http/http_network_transaction.cc

Index: net/http/http_network_transaction.cc
diff --git a/net/http/http_network_transaction.cc b/net/http/http_network_transaction.cc
index 64a4fa7985669de75c1ef7c7a02d99f23602c1cf..263ad7b3dfd70221f448220db19ddc3f4882e3ea 100644
--- a/net/http/http_network_transaction.cc
+++ b/net/http/http_network_transaction.cc
@@ -554,6 +554,10 @@ int HttpNetworkTransaction::DoCreateStreamComplete(int result) {
     return OK;
   }
 
+  // Handle possible handshake errors that may have occurred if the stream
+  // used SSL for one or more of the layers.
+  result = HandleSSLHandshakeError(result);
+
   // At this point we are done with the stream_request_.
   stream_request_.reset();
   return result;
@@ -696,23 +700,6 @@ int HttpNetworkTransaction::DoReadHeadersComplete(int result) {
     result = HandleCertificateRequest(result);
     if (result == OK)
       return result;
-  } else if ((result == ERR_SSL_DECOMPRESSION_FAILURE_ALERT ||
-              result == ERR_SSL_BAD_RECORD_MAC_ALERT) &&
-             ssl_config_.tls1_enabled &&
-             !SSLConfigService::IsKnownStrictTLSServer(request_->url.host())) {
-    // Some buggy servers select DEFLATE compression when offered and then
-    // fail to ever decompress anything. They will send a fatal alert telling
-    // us this. Normally we would pick this up during the handshake because
-    // our Finished message is compressed and we'll never get the server's
-    // Finished if it fails to process ours.
-    //
-    // However, with False Start, we'll believe that the handshake is
-    // complete as soon as we've /sent/ our Finished message. In this case,
-    // we only find out that the server is buggy here, when we try to read
-    // the initial reply.
-    session_->http_stream_factory()->AddTLSIntolerantServer(request_->url);
-    ResetConnectionAndRequestForResend();
-    return OK;
   }
 
   if (result < 0 && result != ERR_CONNECTION_CLOSED)
@@ -1024,11 +1011,45 @@ int HttpNetworkTransaction::HandleCertificateRequest(int error) {
   return OK;
 }
 
+// TODO(rch): This does not currently handle errors when an SSL proxy is
+// being used (http://crbug.com/66424)

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

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.

+int HttpNetworkTransaction::HandleSSLHandshakeError(int error) {
+  DCHECK(request_);
+  if (ssl_config_.send_client_cert &&
+      (error == ERR_SSL_PROTOCOL_ERROR ||
+       error == ERR_BAD_SSL_CLIENT_AUTH_CERT)) {
+    session_->ssl_client_auth_cache()->Remove(
+        GetHostAndPort(request_->url));
+  }
+
+  switch (error) {
+    case ERR_SSL_PROTOCOL_ERROR:
+    case ERR_SSL_VERSION_OR_CIPHER_MISMATCH:
+    case ERR_SSL_DECOMPRESSION_FAILURE_ALERT:
+    case ERR_SSL_BAD_RECORD_MAC_ALERT:
+      if (ssl_config_.tls1_enabled &&
+          !SSLConfigService::IsKnownStrictTLSServer(request_->url.host())) {
+        // This could be a TLS-intolerant server, an SSL 3.0 server that
+        // chose a TLS-only cipher suite or a server with buggy DEFLATE
+        // support. Turn off TLS 1.0, DEFLATE support and retry.
+        session_->http_stream_factory()->AddTLSIntolerantServer(request_->url);
+        ResetConnectionAndRequestForResend();
+        error = OK;
+      }
+      break;
+  }
+  return error;
+}
+
 // This method determines whether it is safe to resend the request after an
 // IO error.  It can only be called in response to request header or body
 // write errors or response header read errors.  It should not be used in
 // other cases, such as a Connect error.
 int HttpNetworkTransaction::HandleIOError(int error) {

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

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).

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

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.
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]

+  // Handle possible SSL handshake errors that may occur during send/receive
+  // due to either False Start or Snap Start.
+  error = HandleSSLHandshakeError(error);
+
   switch (error) {
     // If we try to reuse a connection that the server is in the process of
     // closing, we may end up successfully writing out our request (or a