Reintegrate certificate selection in HttpNetworkTransaction DoLoop

The HttpNetworkTransaction refactor intercepts the client auth
handling and moves it out of DoLoop. Because HandleCertificateRequest
often switches states, this caused a DCHECK and crash in some
circumstances.

This reintegrates it and adds unit tests to catch the DCHECK. We really
want to test sending a legitimate certificate, as well as more
checking interesting errors, but we cannot import temporary keys yet.

We also add a patch for tlslite to send a non-empty certificate_types.
Apple's SSL implementation raises a protocol error otherwise.

BUG=52744,51132,52778
TEST=SSLClientSocketTest.ConnectClientAuth*,URLRequestTest.ClientAuthTest

Review URL: http://codereview.chromium.org/3141026

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@56983 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 74 insertions, 6 deletions

diff --git a/net/http/http_network_transaction.cc b/net/http/http_network_transaction.cc
index a79a9a4..1ff166c 100644
--- a/net/http/http_network_transaction.cc
+++ b/net/http/http_network_transaction.cc
@@ -442,8 +442,7 @@ void HttpNetworkTransaction::OnNeedsClientAuth(
   DCHECK_EQ(STATE_INIT_STREAM_COMPLETE, next_state_);
 
   response_.cert_request_info = cert_info;
-  int result = HandleCertificateRequest(ERR_SSL_CLIENT_AUTH_CERT_NEEDED);
-  DoCallback(result);
+  OnIOComplete(ERR_SSL_CLIENT_AUTH_CERT_NEEDED);
 }
 
 HttpNetworkTransaction::~HttpNetworkTransaction() {
@@ -579,6 +578,8 @@ int HttpNetworkTransaction::DoInitStreamComplete(int result) {
   if (result == OK) {
     next_state_ = STATE_GENERATE_PROXY_AUTH_TOKEN;
     DCHECK(stream_.get());
+  } else if (result == ERR_SSL_CLIENT_AUTH_CERT_NEEDED) {
+    result = HandleCertificateRequest(result);
   }
 
   // At this point we are done with the stream_request_.
@@ -987,6 +988,7 @@ int HttpNetworkTransaction::HandleCertificateRequest(int error) {
   //   long time while the user selects a certificate.
   //   Second, even if we did keep the connection open, NSS has a bug where
   //   restarting the handshake for ClientAuth is currently broken.
+  DCHECK_EQ(error, ERR_SSL_CLIENT_AUTH_CERT_NEEDED);
 
   if (stream_.get()) {
     // Since we already have a stream, we're being called as part of SSL
@@ -999,10 +1001,8 @@ int HttpNetworkTransaction::HandleCertificateRequest(int error) {
   if (stream_request_.get()) {
     // The server is asking for a client certificate during the initial
     // handshake.
-    DCHECK_EQ(STATE_INIT_STREAM_COMPLETE, next_state_);
     stream_request_->Cancel();
     stream_request_ = NULL;
-    next_state_ = STATE_INIT_STREAM;
   }
 
   // If the user selected one of the certificate in client_certs for this
@@ -1014,6 +1014,9 @@ int HttpNetworkTransaction::HandleCertificateRequest(int error) {
         response_.cert_request_info->client_certs;
     for (size_t i = 0; i < client_certs.size(); ++i) {
       if (client_cert->fingerprint().Equals(client_certs[i]->fingerprint())) {
+        // TODO(davidben): Add a unit test which covers this path; we need to be
+        // able to send a legitimate certificate and also bypass/clear the
+        // SSL session cache.
         ssl_config_.client_cert = client_cert;
         ssl_config_.send_client_cert = true;
         next_state_ = STATE_INIT_STREAM;
diff --git a/net/socket/ssl_client_socket_unittest.cc b/net/socket/ssl_client_socket_unittest.cc
index 6673ec6..b4ce521 100644
--- a/net/socket/ssl_client_socket_unittest.cc
+++ b/net/socket/ssl_client_socket_unittest.cc
@@ -167,8 +167,9 @@ TEST_F(SSLClientSocketTest, ConnectMismatched) {
       log.entries(), -1, net::NetLog::TYPE_SSL_CONNECT));
 }
 
-// TODO(davidben): Also test providing a certificate.
-TEST_F(SSLClientSocketTest, ConnectClientAuthNoCert) {
+// Attempt to connect to a page which requests a client certificate. It should
+// return an error code on connect.
+TEST_F(SSLClientSocketTest, ConnectClientAuthCertRequested) {
   net::TestServer test_server(net::TestServer::TYPE_HTTPS_CLIENT_AUTH,
                               FilePath());
   ASSERT_TRUE(test_server.Start());
@@ -211,6 +212,59 @@ TEST_F(SSLClientSocketTest, ConnectClientAuthNoCert) {
       log.entries(), -1, net::NetLog::TYPE_SSL_CONNECT));
 }
 
+// Connect to a server requesting optional client authentication. Send it a
+// null certificate. It should allow the connection.
+//
+// TODO(davidben): Also test providing an actual certificate.
+TEST_F(SSLClientSocketTest, ConnectClientAuthSendNullCert) {
+  net::TestServer test_server(net::TestServer::TYPE_HTTPS_CLIENT_AUTH,
+                              FilePath());
+  ASSERT_TRUE(test_server.Start());
+
+  net::AddressList addr;
+  ASSERT_TRUE(test_server.GetAddressList(&addr));
+
+  TestCompletionCallback callback;
+  net::CapturingNetLog log(net::CapturingNetLog::kUnbounded);
+  net::ClientSocket* transport = new net::TCPClientSocket(addr, &log);
+  int rv = transport->Connect(&callback);
+  if (rv == net::ERR_IO_PENDING)
+    rv = callback.WaitForResult();
+  EXPECT_EQ(net::OK, rv);
+
+  net::SSLConfig ssl_config = kDefaultSSLConfig;
+  ssl_config.send_client_cert = true;
+  ssl_config.client_cert = NULL;
+
+  scoped_ptr<net::SSLClientSocket> sock(
+      socket_factory_->CreateSSLClientSocket(transport,
+          test_server.host_port_pair().host(), ssl_config));
+
+  EXPECT_FALSE(sock->IsConnected());
+
+  // Our test server accepts certificate-less connections.
+  // TODO(davidben): Add a test which requires them and verify the error.
+  rv = sock->Connect(&callback);
+  EXPECT_TRUE(net::LogContainsBeginEvent(
+      log.entries(), 5, net::NetLog::TYPE_SSL_CONNECT));
+  if (rv != net::OK) {
+    ASSERT_EQ(net::ERR_IO_PENDING, rv);
+    EXPECT_FALSE(sock->IsConnected());
+    EXPECT_FALSE(net::LogContainsEndEvent(
+        log.entries(), -1, net::NetLog::TYPE_SSL_CONNECT));
+
+    rv = callback.WaitForResult();
+    EXPECT_EQ(net::OK, rv);
+  }
+
+  EXPECT_TRUE(sock->IsConnected());
+  EXPECT_TRUE(net::LogContainsEndEvent(
+      log.entries(), -1, net::NetLog::TYPE_SSL_CONNECT));
+
+  sock->Disconnect();
+  EXPECT_FALSE(sock->IsConnected());
+}
+
 // TODO(wtc): Add unit tests for IsConnectedAndIdle:
 //   - Server closes an SSL connection (with a close_notify alert message).
 //   - Server closes the underlying TCP connection directly.
diff --git a/net/url_request/url_request_unittest.cc b/net/url_request/url_request_unittest.cc
index aabb070..2ffb631 100644
--- a/net/url_request/url_request_unittest.cc
+++ b/net/url_request/url_request_unittest.cc
@@ -467,6 +467,17 @@ TEST_F(HTTPSRequestTest, ClientAuthTest) {
     EXPECT_EQ(1, d.on_certificate_requested_count());
     EXPECT_FALSE(d.received_data_before_response());
     EXPECT_EQ(0, d.bytes_received());
+
+    // Send no certificate.
+    // TODO(davidben): Get temporary client cert import (with keys) working on
+    // all platforms so we can test sending a cert as well.
+    r.ContinueWithCertificate(NULL);
+
+    MessageLoop::current()->Run();
+
+    EXPECT_EQ(1, d.response_started_count());
+    EXPECT_FALSE(d.received_data_before_response());
+    EXPECT_NE(0, d.bytes_received());
   }
 }