Improve error pages on client certificate failures.

net/socket/ssl_client_socket_unittest.cc

Index: net/socket/ssl_client_socket_unittest.cc
diff --git a/net/socket/ssl_client_socket_unittest.cc b/net/socket/ssl_client_socket_unittest.cc
index c4e3192996b78f931e4a68084b1df7347f27a565..70cf9f66d8564f3c7aad5de1cdd3ba5def7aabeb 100644
--- a/net/socket/ssl_client_socket_unittest.cc
+++ b/net/socket/ssl_client_socket_unittest.cc
@@ -288,8 +288,8 @@ void ReadBufferingStreamSocket::OnReadCompleted(int result) {
 // Simulates synchronously receiving an error during Read() or Write()
 class SynchronousErrorStreamSocket : public WrappedStreamSocket {
  public:
-  explicit SynchronousErrorStreamSocket(
-      std::unique_ptr<StreamSocket> transport);
+  explicit SynchronousErrorStreamSocket(std::unique_ptr<StreamSocket> transport)
+      : WrappedStreamSocket(std::move(transport)) {}
   ~SynchronousErrorStreamSocket() override {}
 
   // Socket implementation:
@@ -321,23 +321,15 @@ class SynchronousErrorStreamSocket : public WrappedStreamSocket {
   }
 
  private:
-  bool have_read_error_;
-  int pending_read_error_;
+  bool have_read_error_ = false;
+  int pending_read_error_ = OK;
 
-  bool have_write_error_;
-  int pending_write_error_;
+  bool have_write_error_ = false;
+  int pending_write_error_ = OK;
 
   DISALLOW_COPY_AND_ASSIGN(SynchronousErrorStreamSocket);
 };
 
-SynchronousErrorStreamSocket::SynchronousErrorStreamSocket(
-    std::unique_ptr<StreamSocket> transport)
-    : WrappedStreamSocket(std::move(transport)),
-      have_read_error_(false),
-      pending_read_error_(OK),
-      have_write_error_(false),
-      pending_write_error_(OK) {}
-
 int SynchronousErrorStreamSocket::Read(IOBuffer* buf,
                                        int buf_len,
                                        const CompletionCallback& callback) {
@@ -360,7 +352,8 @@ int SynchronousErrorStreamSocket::Write(IOBuffer* buf,
 // semantics).
 class FakeBlockingStreamSocket : public WrappedStreamSocket {
  public:
-  explicit FakeBlockingStreamSocket(std::unique_ptr<StreamSocket> transport);
+  explicit FakeBlockingStreamSocket(std::unique_ptr<StreamSocket> transport)
+      : WrappedStreamSocket(std::move(transport)) {}
   ~FakeBlockingStreamSocket() override {}
 
   // Socket implementation:
@@ -381,6 +374,10 @@ class FakeBlockingStreamSocket : public WrappedStreamSocket {
   void BlockReadResult();
   void UnblockReadResult();
 
+  // Replaces the pending read with |data|. Returns true on success or false if
+  // the caller's reads were too small.
+  bool ReplaceReadResult(const std::string& data);
+
   // Waits for the blocked Read() call to be complete at the underlying
   // transport.
   void WaitForReadResult();
@@ -400,24 +397,30 @@ class FakeBlockingStreamSocket : public WrappedStreamSocket {
   // Handles completion from the underlying transport read.
   void OnReadCompleted(int result);
 
+  // Finishes the current read.
+  void ReturnReadResult();
+
   // True if read callbacks are blocked.
-  bool should_block_read_;
+  bool should_block_read_ = false;
 
   // The buffer for the pending read, or NULL if not consumed.
   scoped_refptr<IOBuffer> pending_read_buf_;
 
+  // The size of the pending read buffer, or -1 if not set.
+  int pending_read_buf_len_ = -1;
+
   // The user callback for the pending read call.
   CompletionCallback pending_read_callback_;
 
   // The result for the blocked read callback, or ERR_IO_PENDING if not
   // completed.
-  int pending_read_result_;
+  int pending_read_result_ = ERR_IO_PENDING;
 
   // WaitForReadResult() wait loop.
   std::unique_ptr<base::RunLoop> read_loop_;
 
   // True if write calls are blocked.
-  bool should_block_write_;
+  bool should_block_write_ = false;
 
   // The buffer for the pending write, or NULL if not scheduled.
   scoped_refptr<IOBuffer> pending_write_buf_;
@@ -426,20 +429,12 @@ class FakeBlockingStreamSocket : public WrappedStreamSocket {
   CompletionCallback pending_write_callback_;
 
   // The length for the pending write, or -1 if not scheduled.
-  int pending_write_len_;
+  int pending_write_len_ = -1;
 
   // WaitForWrite() wait loop.
   std::unique_ptr<base::RunLoop> write_loop_;
 };
 
-FakeBlockingStreamSocket::FakeBlockingStreamSocket(
-    std::unique_ptr<StreamSocket> transport)
-    : WrappedStreamSocket(std::move(transport)),
-      should_block_read_(false),
-      pending_read_result_(ERR_IO_PENDING),
-      should_block_write_(false),
-      pending_write_len_(-1) {}
-
 int FakeBlockingStreamSocket::Read(IOBuffer* buf,
                                    int len,
                                    const CompletionCallback& callback) {
@@ -450,16 +445,16 @@ int FakeBlockingStreamSocket::Read(IOBuffer* buf,
 
   int rv = transport_->Read(buf, len, base::Bind(
       &FakeBlockingStreamSocket::OnReadCompleted, base::Unretained(this)));
-  if (rv == ERR_IO_PENDING) {
+  if (rv == ERR_IO_PENDING || should_block_read_) {
     // Save the callback to be called later.
     pending_read_buf_ = buf;
+    pending_read_buf_len_ = len;
     pending_read_callback_ = callback;
-  } else if (should_block_read_) {
-    // Save the callback and read result to be called later.
-    pending_read_buf_ = buf;
-    pending_read_callback_ = callback;
-    OnReadCompleted(rv);
-    rv = ERR_IO_PENDING;
+    // Save the read result.
+    if (rv != ERR_IO_PENDING) {
+      OnReadCompleted(rv);
+      rv = ERR_IO_PENDING;
+    }
   }
   return rv;
 }
@@ -497,15 +492,23 @@ void FakeBlockingStreamSocket::UnblockReadResult() {
   DCHECK(should_block_read_);
   should_block_read_ = false;
 
-  // If the operation is still pending in the underlying transport, immediately
-  // return - OnReadCompleted() will handle invoking the callback once the
-  // transport has completed.
-  if (pending_read_result_ == ERR_IO_PENDING)
-    return;
-  int result = pending_read_result_;
-  pending_read_buf_ = nullptr;
-  pending_read_result_ = ERR_IO_PENDING;
-  base::ResetAndReturn(&pending_read_callback_).Run(result);
+  // If the operation has since completed, return the result to the caller.
+  if (pending_read_result_ != ERR_IO_PENDING)
+    ReturnReadResult();
+}
+
+bool FakeBlockingStreamSocket::ReplaceReadResult(const std::string& data) {
+  DCHECK(should_block_read_);
+  DCHECK_NE(ERR_IO_PENDING, pending_read_result_);
+  DCHECK(pending_read_buf_);
+  DCHECK_NE(-1, pending_read_buf_len_);
+
+  if (static_cast<size_t>(pending_read_buf_len_) < data.size())
+    return false;
+
+  memcpy(pending_read_buf_->data(), data.data(), data.size());
+  pending_read_result_ = data.size();
+  return true;
 }
 
 void FakeBlockingStreamSocket::WaitForReadResult() {
@@ -561,20 +564,24 @@ void FakeBlockingStreamSocket::OnReadCompleted(int result) {
   DCHECK_EQ(ERR_IO_PENDING, pending_read_result_);
   DCHECK(!pending_read_callback_.is_null());
 
-  if (should_block_read_) {
-    // Store the result so that the callback can be invoked once Unblock() is
-    // called.
-    pending_read_result_ = result;
+  pending_read_result_ = result;
 
-    // Stop the WaitForReadResult() call if any.
+  if (should_block_read_) {
+    // Defer the result until UnblockReadResult is called.
     if (read_loop_)
       read_loop_->Quit();
-  } else {
-    // Either the Read() was never blocked or UnblockReadResult() was called
-    // before the Read() completed. Either way, return the result to the caller.
-    pending_read_buf_ = nullptr;
-    base::ResetAndReturn(&pending_read_callback_).Run(result);
+    return;
   }
+
+  ReturnReadResult();
+}
+
+void FakeBlockingStreamSocket::ReturnReadResult() {
+  int result = pending_read_result_;
+  pending_read_result_ = ERR_IO_PENDING;
+  pending_read_buf_ = nullptr;
+  pending_read_buf_len_ = -1;
+  base::ResetAndReturn(&pending_read_callback_).Run(result);
 }
 
 // CountingStreamSocket wraps an existing StreamSocket and maintains a count of
@@ -1020,6 +1027,25 @@ class SSLClientSocketChannelIDTest : public SSLClientSocketTest {
   std::unique_ptr<ChannelIDService> channel_id_service_;
 };
 
+// Returns a serialized unencrypted TLS 1.2 alert record for the given alert
+// value.
+std::string FormatTLS12Alert(uint8_t alert) {
+  std::string ret;
+  // ContentType.alert
+  ret.push_back(21);
+  // Record-layer version. Assume TLS 1.2.
+  ret.push_back(0x03);
+  ret.push_back(0x03);
+  // Record length.
+  ret.push_back(0);
+  ret.push_back(2);
+  // AlertLevel.fatal.
+  ret.push_back(2);
+  // The alert itself.
+  ret.push_back(alert);
+  return ret;
+}
+
 }  // namespace
 
 TEST_F(SSLClientSocketTest, Connect) {
@@ -3279,9 +3305,6 @@ TEST_F(SSLClientSocketTest, SendGoodCert) {
   SSLConfig ssl_config;
   ssl_config.send_client_cert = true;
   ssl_config.client_cert = ImportCertFromFile(certs_dir, "client_1.pem");
-
-  // This is required to ensure that signing works with the client
-  // certificate's private key.
   ssl_config.client_private_key =
       LoadPrivateKeyOpenSSL(certs_dir.AppendASCII("client_1.key"));
 
@@ -3472,4 +3495,279 @@ TEST_F(SSLClientSocketTest, PKPMoreImportantThanCT) {
   EXPECT_TRUE(sock_->IsConnected());
 }
 
+// Test that handshake_failure alerts at the ServerHello are mapped to
+// ERR_SSL_VERSION_OR_CIPHER_MISMATCH.
+TEST_F(SSLClientSocketTest, HandshakeFailureServerHello) {
+  ASSERT_TRUE(StartTestServer(SpawnedTestServer::SSLOptions()));
+
+  TestCompletionCallback callback;
+  std::unique_ptr<StreamSocket> real_transport(
+      new TCPClientSocket(addr(), NULL, NULL, NetLog::Source()));
+  std::unique_ptr<FakeBlockingStreamSocket> transport(
+      new FakeBlockingStreamSocket(std::move(real_transport)));
+  FakeBlockingStreamSocket* raw_transport = transport.get();
+  int rv = callback.GetResult(transport->Connect(callback.callback()));
+  ASSERT_THAT(rv, IsOk());
+
+  std::unique_ptr<SSLClientSocket> sock(CreateSSLClientSocket(
+      std::move(transport), spawned_test_server()->host_port_pair(),
+      SSLConfig()));
+
+  // Connect. Stop before the client processes ServerHello.
+  raw_transport->BlockReadResult();
+  rv = sock->Connect(callback.callback());
+  ASSERT_THAT(rv, IsError(ERR_IO_PENDING));
+  raw_transport->WaitForReadResult();
+
+  // Replace it with an alert.
+  raw_transport->ReplaceReadResult(
+      FormatTLS12Alert(40 /* AlertDescription.handshake_failure */));
+  raw_transport->UnblockReadResult();
+
+  rv = callback.GetResult(rv);
+  EXPECT_THAT(rv, IsError(ERR_SSL_VERSION_OR_CIPHER_MISMATCH));
+}
+
+// Test that handshake_failure alerts after the ServerHello but without a
+// CertificateRequest are mapped to ERR_SSL_PROTOCOL_ERROR.
+TEST_F(SSLClientSocketTest, HandshakeFailureNoClientCerts) {
+  ASSERT_TRUE(StartTestServer(SpawnedTestServer::SSLOptions()));
+
+  TestCompletionCallback callback;
+  std::unique_ptr<StreamSocket> real_transport(
+      new TCPClientSocket(addr(), NULL, NULL, NetLog::Source()));
+  std::unique_ptr<FakeBlockingStreamSocket> transport(
+      new FakeBlockingStreamSocket(std::move(real_transport)));
+  FakeBlockingStreamSocket* raw_transport = transport.get();
+  int rv = callback.GetResult(transport->Connect(callback.callback()));
+  ASSERT_THAT(rv, IsOk());
+
+  std::unique_ptr<SSLClientSocket> sock(CreateSSLClientSocket(
+      std::move(transport), spawned_test_server()->host_port_pair(),
+      SSLConfig()));
+
+  // Connect. Stop before the client processes ServerHello.
+  raw_transport->BlockReadResult();
+  rv = sock->Connect(callback.callback());
+  ASSERT_THAT(rv, IsError(ERR_IO_PENDING));
+  raw_transport->WaitForReadResult();
+
+  // Release the ServerHello and wait for the client to write its second flight.
+  raw_transport->BlockWrite();
+  raw_transport->UnblockReadResult();
+  raw_transport->WaitForWrite();
+
+  // Wait for the server's final flight.
+  raw_transport->BlockReadResult();
+  raw_transport->UnblockWrite();
+  raw_transport->WaitForReadResult();
+
+  // Replace it with an alert.
+  raw_transport->ReplaceReadResult(
+      FormatTLS12Alert(40 /* AlertDescription.handshake_failure */));
+  raw_transport->UnblockReadResult();
+
+  rv = callback.GetResult(rv);
+  EXPECT_THAT(rv, IsError(ERR_SSL_PROTOCOL_ERROR));
+}
+
+// Test that handshake_failure alerts after the ServerHello map to
+// ERR_BAD_SSL_CLIENT_AUTH_CERT if a client certificate was requested but not
+// supplied. TLS does not have an alert for this case, so handshake_failure is
+// common. See https://crbug.com/646567.
+TEST_F(SSLClientSocketTest, LateHandshakeFailureMissingClientCerts) {
+  // Request a client certificate.
+  SpawnedTestServer::SSLOptions ssl_options;
+  ssl_options.request_client_certificate = true;
+  ASSERT_TRUE(StartTestServer(ssl_options));
+
+  TestCompletionCallback callback;
+  std::unique_ptr<StreamSocket> real_transport(
+      new TCPClientSocket(addr(), NULL, NULL, NetLog::Source()));
+  std::unique_ptr<FakeBlockingStreamSocket> transport(
+      new FakeBlockingStreamSocket(std::move(real_transport)));
+  FakeBlockingStreamSocket* raw_transport = transport.get();
+  int rv = callback.GetResult(transport->Connect(callback.callback()));
+  ASSERT_THAT(rv, IsOk());
+
+  // Send no client certificate.
+  SSLConfig config;
+  config.send_client_cert = true;
+  std::unique_ptr<SSLClientSocket> sock(CreateSSLClientSocket(
+      std::move(transport), spawned_test_server()->host_port_pair(), config));
+
+  // Connect. Stop before the client processes ServerHello.
+  raw_transport->BlockReadResult();
+  rv = sock->Connect(callback.callback());
+  ASSERT_THAT(rv, IsError(ERR_IO_PENDING));
+  raw_transport->WaitForReadResult();
+
+  // Release the ServerHello and wait for the client to write its second flight.
+  raw_transport->BlockWrite();
+  raw_transport->UnblockReadResult();
+  raw_transport->WaitForWrite();
+
+  // Wait for the server's final flight.
+  raw_transport->BlockReadResult();
+  raw_transport->UnblockWrite();
+  raw_transport->WaitForReadResult();
+
+  // Replace it with an alert.
+  raw_transport->ReplaceReadResult(
+      FormatTLS12Alert(40 /* AlertDescription.handshake_failure */));
+  raw_transport->UnblockReadResult();
+
+  rv = callback.GetResult(rv);
+  EXPECT_THAT(rv, IsError(ERR_BAD_SSL_CLIENT_AUTH_CERT));
+}
+
+// Test that handshake_failure alerts after the ServerHello map to
+// ERR_SSL_PROTOCOL_ERROR if received after sending a client certificate. It is
+// assumed servers will send a more appropriate alert in this case.
+TEST_F(SSLClientSocketTest, LateHandshakeFailureSendClientCerts) {
+  // Request a client certificate.
+  SpawnedTestServer::SSLOptions ssl_options;
+  ssl_options.request_client_certificate = true;
+  ASSERT_TRUE(StartTestServer(ssl_options));
+
+  TestCompletionCallback callback;
+  std::unique_ptr<StreamSocket> real_transport(
+      new TCPClientSocket(addr(), NULL, NULL, NetLog::Source()));
+  std::unique_ptr<FakeBlockingStreamSocket> transport(
+      new FakeBlockingStreamSocket(std::move(real_transport)));
+  FakeBlockingStreamSocket* raw_transport = transport.get();
+  int rv = callback.GetResult(transport->Connect(callback.callback()));
+  ASSERT_THAT(rv, IsOk());
+
+  // Send a client certificate.
+  base::FilePath certs_dir = GetTestCertsDirectory();
+  SSLConfig config;
+  config.send_client_cert = true;
+  config.client_cert = ImportCertFromFile(certs_dir, "client_1.pem");
+  config.client_private_key =
+      LoadPrivateKeyOpenSSL(certs_dir.AppendASCII("client_1.key"));
+  std::unique_ptr<SSLClientSocket> sock(CreateSSLClientSocket(
+      std::move(transport), spawned_test_server()->host_port_pair(), config));
+
+  // Connect. Stop before the client processes ServerHello.
+  raw_transport->BlockReadResult();
+  rv = sock->Connect(callback.callback());
+  ASSERT_THAT(rv, IsError(ERR_IO_PENDING));
+  raw_transport->WaitForReadResult();
+
+  // Release the ServerHello and wait for the client to write its second flight.
+  raw_transport->BlockWrite();
+  raw_transport->UnblockReadResult();
+  raw_transport->WaitForWrite();
+
+  // Wait for the server's final flight.
+  raw_transport->BlockReadResult();
+  raw_transport->UnblockWrite();
+  raw_transport->WaitForReadResult();
+
+  // Replace it with an alert.
+  raw_transport->ReplaceReadResult(
+      FormatTLS12Alert(40 /* AlertDescription.handshake_failure */));
+  raw_transport->UnblockReadResult();
+
+  rv = callback.GetResult(rv);
+  EXPECT_THAT(rv, IsError(ERR_SSL_PROTOCOL_ERROR));
+}
+
+// Test that access_denied alerts are mapped to ERR_SSL_PROTOCOL_ERROR if
+// received on a connection not requesting client certificates. This is an
+// incorrect use of the alert but is common. See https://crbug.com/630883.
+TEST_F(SSLClientSocketTest, AccessDeniedNoClientCerts) {
+  ASSERT_TRUE(StartTestServer(SpawnedTestServer::SSLOptions()));
+
+  TestCompletionCallback callback;
+  std::unique_ptr<StreamSocket> real_transport(
+      new TCPClientSocket(addr(), NULL, NULL, NetLog::Source()));
+  std::unique_ptr<FakeBlockingStreamSocket> transport(
+      new FakeBlockingStreamSocket(std::move(real_transport)));
+  FakeBlockingStreamSocket* raw_transport = transport.get();
+  int rv = callback.GetResult(transport->Connect(callback.callback()));
+  ASSERT_THAT(rv, IsOk());
+
+  std::unique_ptr<SSLClientSocket> sock(CreateSSLClientSocket(
+      std::move(transport), spawned_test_server()->host_port_pair(),
+      SSLConfig()));
+
+  // Connect. Stop before the client processes ServerHello.
+  raw_transport->BlockReadResult();
+  rv = sock->Connect(callback.callback());
+  ASSERT_THAT(rv, IsError(ERR_IO_PENDING));
+  raw_transport->WaitForReadResult();
+
+  // Release the ServerHello and wait for the client to write its second flight.
+  raw_transport->BlockWrite();
+  raw_transport->UnblockReadResult();
+  raw_transport->WaitForWrite();
+
+  // Wait for the server's final flight.
+  raw_transport->BlockReadResult();
+  raw_transport->UnblockWrite();
+  raw_transport->WaitForReadResult();
+
+  // Replace it with an alert.
+  raw_transport->ReplaceReadResult(
+      FormatTLS12Alert(49 /* AlertDescription.access_denied */));
+  raw_transport->UnblockReadResult();
+
+  rv = callback.GetResult(rv);
+  EXPECT_THAT(rv, IsError(ERR_SSL_PROTOCOL_ERROR));
+}
+
+// Test that access_denied alerts are mapped to ERR_BAD_SSL_CLIENT_AUTH_CERT if
+// received on a connection requesting client certificates.
+TEST_F(SSLClientSocketTest, AccessDeniedClientCerts) {
+  // Request a client certificate.
+  SpawnedTestServer::SSLOptions ssl_options;
+  ssl_options.request_client_certificate = true;
+  ASSERT_TRUE(StartTestServer(ssl_options));
+
+  TestCompletionCallback callback;
+  std::unique_ptr<StreamSocket> real_transport(
+      new TCPClientSocket(addr(), NULL, NULL, NetLog::Source()));
+  std::unique_ptr<FakeBlockingStreamSocket> transport(
+      new FakeBlockingStreamSocket(std::move(real_transport)));
+  FakeBlockingStreamSocket* raw_transport = transport.get();
+  int rv = callback.GetResult(transport->Connect(callback.callback()));
+  ASSERT_THAT(rv, IsOk());
+
+  // Send a client certificate.
+  base::FilePath certs_dir = GetTestCertsDirectory();
+  SSLConfig config;
+  config.send_client_cert = true;
+  config.client_cert = ImportCertFromFile(certs_dir, "client_1.pem");
+  config.client_private_key =
+      LoadPrivateKeyOpenSSL(certs_dir.AppendASCII("client_1.key"));
+  std::unique_ptr<SSLClientSocket> sock(CreateSSLClientSocket(
+      std::move(transport), spawned_test_server()->host_port_pair(), config));
+
+  // Connect. Stop before the client processes ServerHello.
+  raw_transport->BlockReadResult();
+  rv = sock->Connect(callback.callback());
+  ASSERT_THAT(rv, IsError(ERR_IO_PENDING));
+  raw_transport->WaitForReadResult();
+
+  // Release the ServerHello and wait for the client to write its second flight.
+  raw_transport->BlockWrite();
+  raw_transport->UnblockReadResult();
+  raw_transport->WaitForWrite();
+
+  // Wait for the server's final flight.
+  raw_transport->BlockReadResult();
+  raw_transport->UnblockWrite();
+  raw_transport->WaitForReadResult();
+
+  // Replace it with an alert.
+  raw_transport->ReplaceReadResult(
+      FormatTLS12Alert(49 /* AlertDescription.access_denied */));
+  raw_transport->UnblockReadResult();
+
+  rv = callback.GetResult(rv);
+  EXPECT_THAT(rv, IsError(ERR_BAD_SSL_CLIENT_AUTH_CERT));
+}
+
 }  // namespace net