Support for client certs in ssl_server_socket.

net/socket/ssl_server_socket_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // This test suite uses SSLClientSocket to test the implementation of
 // SSLServerSocket. In order to establish connections between the sockets
 // we need two additional classes:
 // 1. FakeSocket
 //    Connects SSL socket to FakeDataChannel. This class is just a stub.
 //
 // 2. FakeDataChannel
 //    Implements the actual exchange of data between two FakeSockets.
 //
 // Implementations of these two classes are included in this file.
 
 #include "net/socket/ssl_server_socket.h"
 
+#include <openssl/evp.h>
+#include <openssl/ssl.h>
+#include <openssl/x509.h>

[davidben, 2016/01/25 20:56:11]

Needs to be guarded by USE_OPENSSL (and ordered after all the other includes
since it then depends on build/build_config.h)

[ryanchung, 2016/01/29 23:22:13]

Done.

+
 #include <stdint.h>
 #include <stdlib.h>
 #include <queue>
 #include <utility>
 
 #include "base/compiler_specific.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/message_loop/message_loop.h"
 #include "base/single_thread_task_runner.h"
 #include "base/thread_task_runner_handle.h"
 #include "crypto/nss_util.h"
 #include "crypto/rsa_private_key.h"
+#include "crypto/signature_creator.h"
 #include "net/base/address_list.h"
 #include "net/base/completion_callback.h"
 #include "net/base/host_port_pair.h"
 #include "net/base/io_buffer.h"
 #include "net/base/ip_endpoint.h"
 #include "net/base/net_errors.h"
 #include "net/base/test_data_directory.h"
 #include "net/cert/cert_status_flags.h"
 #include "net/cert/mock_cert_verifier.h"
+#include "net/cert/mock_client_cert_verifier.h"
 #include "net/cert/x509_certificate.h"
 #include "net/http/transport_security_state.h"
 #include "net/log/net_log.h"
 #include "net/socket/client_socket_factory.h"
 #include "net/socket/socket_test_util.h"
 #include "net/socket/ssl_client_socket.h"
 #include "net/socket/stream_socket.h"
+#include "net/ssl/ssl_cert_request_info.h"
 #include "net/ssl/ssl_cipher_suite_names.h"
 #include "net/ssl/ssl_connection_status_flags.h"
 #include "net/ssl/ssl_info.h"
+#include "net/ssl/ssl_private_key.h"
 #include "net/ssl/ssl_server_config.h"
 #include "net/test/cert_test_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "testing/platform_test.h"
 
 namespace net {
 
 namespace {
 
+const char kClientCertFileName[] = "client_1.pem";
+const char kClientPrivateKeyFileName[] = "client_1.pk8";
+const char kWrongClientCertFileName[] = "client_2.pem";
+const char kWrongClientPrivateKeyFileName[] = "client_2.pk8";
+const char kClientCertCAFileName[] = "client_1_ca.pem";
+
 class FakeDataChannel {
  public:
   FakeDataChannel()
       : read_buf_len_(0),
         closed_(false),
         write_called_after_close_(false),
         weak_factory_(this) {
   }
 
   int Read(IOBuffer* buf, int buf_len, const CompletionCallback& callback) {
@@ skipping 31 matching lines @@
                               weak_factory_.GetWeakPtr()));
     return buf_len;
   }
 
   // Closes the FakeDataChannel. After Close() is called, Read() returns 0,
   // indicating EOF, and Write() fails with ERR_CONNECTION_RESET. Note that
   // after the FakeDataChannel is closed, the first Write() call completes
   // asynchronously, which is necessary to reproduce bug 127822.
   void Close() {
     closed_ = true;
+    data_.push(
+        new DrainableIOBuffer(new StringIOBuffer(std::string("0", 1)), 1));
+    if (!read_callback_.is_null()) {
+      base::ThreadTaskRunnerHandle::Get()->PostTask(
+          FROM_HERE, base::Bind(&FakeDataChannel::DoReadCallback,
+                                weak_factory_.GetWeakPtr()));
+    }
   }
 
  private:
   void DoReadCallback() {
     if (read_callback_.is_null() || data_.empty())
       return;
-
     int copied = PropagateData(read_buf_, read_buf_len_);
     CompletionCallback callback = read_callback_;
     read_callback_.Reset();
     read_buf_ = NULL;
     read_buf_len_ = 0;
     callback.Run(copied);
   }
 
   void DoWriteCallback() {
     if (write_callback_.is_null())
@@ skipping 118 matching lines @@
   }
 
  private:
   BoundNetLog net_log_;
   FakeDataChannel* incoming_;
   FakeDataChannel* outgoing_;
 
   DISALLOW_COPY_AND_ASSIGN(FakeSocket);
 };
 
+class TestSSLPrivateKey : public SSLPrivateKey {
+ public:
+  TestSSLPrivateKey(crypto::RSAPrivateKey* rsa_private_key)
+      : rsa_private_key_(rsa_private_key) {}
+
+  Type GetType() override { return SSLPrivateKey::Type::RSA; }
+
+  std::vector<SSLPrivateKey::Hash> GetDigestPreferences() override {
+    static const SSLPrivateKey::Hash kHashes[] = {SSLPrivateKey::Hash::SHA256,
+                                                  SSLPrivateKey::Hash::SHA1};
+    return std::vector<SSLPrivateKey::Hash>(std::begin(kHashes),
+                                            std::end(kHashes));
+  }
+
+  // NOTE: The following algorithm assumes the answer is a power of 2, which is
+  // true for the test keys in use.

[davidben, 2016/01/25 20:56:11]

This comment is no longer accurate.

[ryanchung, 2016/01/29 23:22:13]

Done. Thx.

+  size_t GetMaxSignatureLengthInBytes() override {
+#if defined(USE_OPENSSL)

[davidben, 2016/01/25 20:56:11]

I would just wrap the whole thing in USE_OPENSSL. This interface will never be
consumed by NSS.

[davidben, 2016/01/25 20:56:11]

#include "build/build_config.h" to condition on USE_OPENSSL.

[ryanchung, 2016/01/29 23:22:14]

I assume you mean wrapping the whole class. Done.

[ryanchung, 2016/01/29 23:22:14]

Done.

+    return EVP_PKEY_size(rsa_private_key_->key());
+#else
+    NOTIMPLEMENTED();
+    return 0;
+#endif
+  }
+
+  void SignDigest(Hash hash,
+                  const base::StringPiece& input,
+                  const SignCallback& callback) override {

[davidben, 2016/01/25 20:56:11]

FYI, this implementation will not work for TLS 1.1 and below. I don't hugely
care; hopefully svaldez will have an SSLPrivateKeyOpenSSL very soon for this to
use instead and we'll just remove this.

[ryanchung, 2016/01/29 23:22:13]

Ok. Thanks.

+    std::vector<uint8_t> signature;
+    crypto::SignatureCreator::HashAlgorithm hash_alg;
+    switch (hash) {
+      case Hash::SHA1:
+        hash_alg = crypto::SignatureCreator::SHA1;
+        break;
+
+      case Hash::SHA256:
+        hash_alg = crypto::SignatureCreator::SHA256;
+        break;
+
+      default:
+        FAIL() << "Unsupported hash function";
+    }
+    crypto::SignatureCreator::Sign(
+        rsa_private_key_.get(), hash_alg,
+        reinterpret_cast<const uint8_t*>(input.data()), input.size(),
+        &signature);
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::Bind(callback, OK, signature));
+  }
+
+ private:
+  ~TestSSLPrivateKey() override {}
+
+  scoped_ptr<crypto::RSAPrivateKey> rsa_private_key_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestSSLPrivateKey);
+};
+
 }  // namespace
 
 // Verify the correctness of the test helper classes first.
 TEST(FakeSocketTest, DataTransfer) {
   // Establish channels between two sockets.
   FakeDataChannel channel_1;
   FakeDataChannel channel_2;
   FakeSocket client(&channel_1, &channel_2);
   FakeSocket server(&channel_2, &channel_1);
 
@@ skipping 27 matching lines @@
   EXPECT_GT(read, 0);
   EXPECT_LE(read, written);
   EXPECT_EQ(0, memcmp(kTestData, read_buf->data(), read));
 }
 
 class SSLServerSocketTest : public PlatformTest {
  public:
   SSLServerSocketTest()
       : socket_factory_(ClientSocketFactory::GetDefaultFactory()),
         cert_verifier_(new MockCertVerifier()),
+        client_cert_verifier_(new MockClientCertVerifier()),
         transport_security_state_(new TransportSecurityState) {
-    cert_verifier_->set_default_result(CERT_STATUS_AUTHORITY_INVALID);
+    cert_verifier_->set_default_result(ERR_CERT_AUTHORITY_INVALID);
+    client_cert_verifier_->set_default_result(ERR_CERT_AUTHORITY_INVALID);
   }
 
  protected:
   void Initialize() {
     scoped_ptr<ClientSocketHandle> client_connection(new ClientSocketHandle);
     client_connection->SetSocket(
         scoped_ptr<StreamSocket>(new FakeSocket(&channel_1_, &channel_2_)));
     scoped_ptr<StreamSocket> server_socket(
         new FakeSocket(&channel_2_, &channel_1_));
 
-    base::FilePath certs_dir(GetTestCertsDirectory());
-
-    base::FilePath cert_path = certs_dir.AppendASCII("unittest.selfsigned.der");
-    std::string cert_der;
-    ASSERT_TRUE(base::ReadFileToString(cert_path, &cert_der));
-
-    scoped_refptr<X509Certificate> cert =
-        X509Certificate::CreateFromBytes(cert_der.data(), cert_der.size());
-
-    base::FilePath key_path = certs_dir.AppendASCII("unittest.key.bin");
-    std::string key_string;
-    ASSERT_TRUE(base::ReadFileToString(key_path, &key_string));
-    std::vector<uint8_t> key_vector(
-        reinterpret_cast<const uint8_t*>(key_string.data()),
-        reinterpret_cast<const uint8_t*>(key_string.data() +
-                                         key_string.length()));
-
-    scoped_ptr<crypto::RSAPrivateKey> private_key(
-        crypto::RSAPrivateKey::CreateFromPrivateKeyInfo(key_vector));
+    std::string server_cert_der;
+    scoped_refptr<X509Certificate> server_cert(
+        ReadTestCert("unittest.selfsigned.der", &server_cert_der));
+    scoped_ptr<crypto::RSAPrivateKey> server_private_key(
+        ReadTestKey("unittest.key.bin"));
 
     client_ssl_config_.false_start_enabled = false;
     client_ssl_config_.channel_id_enabled = false;
 
     // Certificate provided by the host doesn't need authority.
     SSLConfig::CertAndStatus cert_and_status;
     cert_and_status.cert_status = CERT_STATUS_AUTHORITY_INVALID;
-    cert_and_status.der_cert = cert_der;
+    cert_and_status.der_cert = server_cert_der;
     client_ssl_config_.allowed_bad_certs.push_back(cert_and_status);
 
     HostPortPair host_and_pair("unittest", 0);
     SSLClientSocketContext context;
     context.cert_verifier = cert_verifier_.get();
     context.transport_security_state = transport_security_state_.get();
+    socket_factory_->ClearSSLSessionCache();
     client_socket_ = socket_factory_->CreateSSLClientSocket(
         std::move(client_connection), host_and_pair, client_ssl_config_,
         context);
-    server_socket_ = CreateSSLServerSocket(std::move(server_socket), cert.get(),
-                                           *private_key, server_ssl_config_);
+    server_socket_ =
+        CreateSSLServerSocket(std::move(server_socket), server_cert.get(),
+                              *server_private_key, server_ssl_config_);
+  }
+
+  void ConfigureClientCertsForClient(const char* cert_file_name,
+                                     const char* private_key_file_name) {
+    scoped_refptr<X509Certificate> cert;
+    scoped_refptr<net::SSLPrivateKey> key;
+    if (cert_file_name && private_key_file_name) {
+      cert = ImportCertFromFile(GetTestCertsDirectory(), cert_file_name);
+      key = new TestSSLPrivateKey(ReadTestKey(private_key_file_name));
+    }
+    client_ssl_config_.send_client_cert = true;
+    client_ssl_config_.client_cert = cert;
+    client_ssl_config_.client_private_key = key;
+  }
+
+  void ConfigureClientCertsForServer(bool cert_expected) {

[davidben, 2016/01/25 20:56:11]

This parameter seems to be always true. (And indeed why would one ever call it
with false?)

[ryanchung, 2016/01/29 23:22:13]

True. Removed parameter.

+    if (!cert_expected)
+      return;
+
+    server_ssl_config_.require_client_cert = true;
+
+#if defined(USE_OPENSSL)

[davidben, 2016/01/25 20:56:11]

This isn't even going to work if we're not USE_OPENSSL, no? I would suggest you
wrap the whole method to make sure no one calls it.

[ryanchung, 2016/01/29 23:22:14]

Done.

+    STACK_OF(X509_NAME)* cert_names;
+    cert_names = SSL_load_client_CA_file(GetTestCertsDirectory()
+                                             .AppendASCII(kClientCertCAFileName)
+                                             .MaybeAsASCII()
+                                             .data());

[davidben, 2016/01/25 20:56:11]

Nit: data -> c_str.
In C++11, it doesn't actually matter, but you do want a NUL-terminated string
here.

[ryanchung, 2016/01/29 23:22:13]

Done.

+    if (cert_names != NULL) {

[davidben, 2016/01/25 20:56:11]

NULL -> nullptr throughout this file.

[davidben, 2016/01/25 20:56:11]

This should be an ASSERT_TRUE or CHECK or something. If you fail to read the
file, it should fail the test.

[ryanchung, 2016/01/29 23:22:14]

Done. Good point.

[ryanchung, 2016/01/29 23:22:14]

Done.

+      for (size_t i = 0; i < sk_X509_NAME_num(cert_names); ++i) {
+        unsigned char* str = NULL;

[davidben, 2016/01/25 20:56:10]

uint8_t

[ryanchung, 2016/01/29 23:22:14]

Done.

+        int length = i2d_X509_NAME(sk_X509_NAME_value(cert_names, i), &str);

[davidben, 2016/01/25 20:56:11]

This leaks memory. You need to OPENSSL_free(str) afterwards.

[ryanchung, 2016/01/29 23:22:13]

Done. Thanks.

+        server_ssl_config_.cert_authorities_.push_back(std::string(
+            reinterpret_cast<const char*>(str), static_cast<size_t>(length)));
+      }
+    }
+#endif
+    scoped_refptr<X509Certificate> expected_client_cert(
+        ImportCertFromFile(GetTestCertsDirectory(), kClientCertFileName));
+    client_cert_verifier_->AddResultForCert(expected_client_cert.get(), OK);
+
+    server_ssl_config_.client_cert_verifier = client_cert_verifier_.get();
+  }
+
+  X509Certificate* ReadTestCert(const base::StringPiece& name,

[davidben, 2016/01/25 20:56:11]

Should return a scoped_refptr, not a raw pointer.

[ryanchung, 2016/01/29 23:22:13]

Done.

+                                std::string* cert_der) {

[davidben, 2016/01/25 20:56:11]

Why do you sometimes use ReadTestCert and sometimes ImportCertFromFile?
(Couldn't this method be replaced with ImportCertFromFile?)

[ryanchung, 2016/01/29 23:22:13]

I'll remove this function.
I believe it was to also get the DER encoded certificate for
SSLConfig::CertAndStatus. But we could get that from GetDEREncoded().

+    base::FilePath certs_dir(GetTestCertsDirectory());
+    base::FilePath cert_path = certs_dir.AppendASCII(name);
+    std::string unused;
+    if (!cert_der)
+      cert_der = &unused;
+    if (!base::ReadFileToString(cert_path, cert_der))
+      return NULL;
+    return X509Certificate::CreateFromBytes(cert_der->data(), cert_der->size());
+  }
+
+  crypto::RSAPrivateKey* ReadTestKey(const base::StringPiece& name) {

[davidben, 2016/01/25 20:56:10]

Should return a scoped_ptr, not a raw pointer.

[ryanchung, 2016/01/29 23:22:13]

Done.

+    base::FilePath certs_dir(GetTestCertsDirectory());
+    base::FilePath key_path = certs_dir.AppendASCII(name);
+    std::string key_string;
+    if (!base::ReadFileToString(key_path, &key_string))
+      return NULL;
+    std::vector<uint8_t> key_vector(
+        reinterpret_cast<const uint8_t*>(key_string.data()),
+        reinterpret_cast<const uint8_t*>(key_string.data() +
+                                         key_string.length()));
+    return crypto::RSAPrivateKey::CreateFromPrivateKeyInfo(key_vector);
   }
 
   FakeDataChannel channel_1_;
   FakeDataChannel channel_2_;
   SSLConfig client_ssl_config_;
   SSLServerConfig server_ssl_config_;
   scoped_ptr<SSLClientSocket> client_socket_;
   scoped_ptr<SSLServerSocket> server_socket_;
   ClientSocketFactory* socket_factory_;
   scoped_ptr<MockCertVerifier> cert_verifier_;
+  scoped_ptr<MockClientCertVerifier> client_cert_verifier_;
   scoped_ptr<TransportSecurityState> transport_security_state_;
+  CertificateList trusted_certs_;

[davidben, 2016/01/25 20:56:11]

Unused?

[ryanchung, 2016/01/29 23:22:14]

Done.

 };
 
 // This test only executes creation of client and server sockets. This is to
 // test that creation of sockets doesn't crash and have minimal code to run
 // under valgrind in order to help debugging memory problems.
 TEST_F(SSLServerSocketTest, Initialize) {
   Initialize();
 }
 
 // This test executes Connect() on SSLClientSocket and Handshake() on
 // SSLServerSocket to make sure handshaking between the two sockets is
 // completed successfully.
 TEST_F(SSLServerSocketTest, Handshake) {
   Initialize();
 
+  TestCompletionCallback handshake_callback;
+  int server_ret = server_socket_->Handshake(handshake_callback.callback());
+  ASSERT_TRUE(server_ret == OK || server_ret == ERR_IO_PENDING);

[davidben, 2016/01/25 20:56:11]

(It's in the original too, but I wouldn't bother with this line.)

[ryanchung, 2016/01/29 23:22:13]

Done.

+
   TestCompletionCallback connect_callback;
-  TestCompletionCallback handshake_callback;
+  int client_ret = client_socket_->Connect(connect_callback.callback());
+  ASSERT_TRUE(client_ret == OK || client_ret == ERR_IO_PENDING);

[davidben, 2016/01/25 20:56:11]

(Ditto.)

[ryanchung, 2016/01/29 23:22:13]

Done.

 
-  int server_ret = server_socket_->Handshake(handshake_callback.callback());
-  EXPECT_TRUE(server_ret == OK || server_ret == ERR_IO_PENDING);
+  client_ret = connect_callback.GetResult(client_ret);
+  server_ret = handshake_callback.GetResult(server_ret);
 
-  int client_ret = client_socket_->Connect(connect_callback.callback());
-  EXPECT_TRUE(client_ret == OK || client_ret == ERR_IO_PENDING);
-
-  if (client_ret == ERR_IO_PENDING) {
-    EXPECT_EQ(OK, connect_callback.WaitForResult());
-  }
-  if (server_ret == ERR_IO_PENDING) {
-    EXPECT_EQ(OK, handshake_callback.WaitForResult());
-  }
+  ASSERT_EQ(OK, client_ret);
+  ASSERT_EQ(OK, server_ret);
 
   // Make sure the cert status is expected.
   SSLInfo ssl_info;
   ASSERT_TRUE(client_socket_->GetSSLInfo(&ssl_info));
   EXPECT_EQ(CERT_STATUS_AUTHORITY_INVALID, ssl_info.cert_status);
 
   // The default cipher suite should be ECDHE and, unless on NSS and the
   // platform doesn't support it, an AEAD.
   uint16_t cipher_suite =
       SSLConnectionStatusToCipherSuite(ssl_info.connection_status);
   const char* key_exchange;
   const char* cipher;
   const char* mac;
   bool is_aead;
   SSLCipherSuiteToStrings(&key_exchange, &cipher, &mac, &is_aead, cipher_suite);
   EXPECT_STREQ("ECDHE_RSA", key_exchange);
   EXPECT_TRUE(is_aead);
 }
 
+// NSS ports don't support client certificates

[davidben, 2016/01/25 20:56:11]

Nit: period at end.

[ryanchung, 2016/01/29 23:22:13]

Done.

+#if defined(USE_OPENSSL)
+
+// This test executes Connect() on SSLClientSocket and Handshake() on
+// SSLServerSocket to make sure handshaking between the two sockets is
+// completed successfully, using client certificate.
+TEST_F(SSLServerSocketTest, HandshakeWithClientCert) {
+  scoped_refptr<X509Certificate> client_cert =
+      ImportCertFromFile(GetTestCertsDirectory(), kClientCertFileName);
+  ConfigureClientCertsForClient(kClientCertFileName, kClientPrivateKeyFileName);
+  ConfigureClientCertsForServer(true);
+  Initialize();
+
+  TestCompletionCallback handshake_callback;
+  int server_ret = server_socket_->Handshake(handshake_callback.callback());
+  ASSERT_TRUE(server_ret == OK || server_ret == ERR_IO_PENDING);

[davidben, 2016/01/25 20:56:11]

(I wouldn't bother with this line. Redundant with line 574.)

[ryanchung, 2016/01/29 23:22:13]

Done.

+
+  TestCompletionCallback connect_callback;
+  int client_ret = client_socket_->Connect(connect_callback.callback());
+  ASSERT_TRUE(client_ret == OK || client_ret == ERR_IO_PENDING);

[davidben, 2016/01/25 20:56:11]

(Ditto.)

[ryanchung, 2016/01/29 23:22:13]

Done.

+
+  client_ret = connect_callback.GetResult(client_ret);
+  server_ret = handshake_callback.GetResult(server_ret);
+
+  ASSERT_EQ(OK, client_ret);
+  ASSERT_EQ(OK, server_ret);
+
+  // Make sure the cert status is expected.
+  SSLInfo ssl_info;
+  client_socket_->GetSSLInfo(&ssl_info);
+  EXPECT_EQ(CERT_STATUS_AUTHORITY_INVALID, ssl_info.cert_status);
+  server_socket_->GetSSLInfo(&ssl_info);
+  EXPECT_TRUE(ssl_info.cert.get());
+  EXPECT_TRUE(client_cert->Equals(ssl_info.cert.get()));
+}
+
+TEST_F(SSLServerSocketTest, HandshakeWithClientCertRequiredNotSupplied) {
+  ConfigureClientCertsForServer(true);
+  Initialize();
+  // Use the default setting for the client socket, which is to not send
+  // a client certificate. This will cause the client to receive an
+  // ERR_SSL_CLIENT_AUTH_CERT_NEEDED error, and allow for inspecting the
+  // requested cert_authorities from the CertificateRequest sent by the
+  // server.
+
+  TestCompletionCallback handshake_callback;
+  int server_ret = server_socket_->Handshake(handshake_callback.callback());
+  ASSERT_EQ(server_ret, ERR_IO_PENDING);

[davidben, 2016/01/25 20:56:11]

(Strictly speaking, not redundant this time, but I wouldn't bother.)

[ryanchung, 2016/01/29 23:22:13]

Done.

+
+  TestCompletionCallback connect_callback;
+  EXPECT_EQ(ERR_SSL_CLIENT_AUTH_CERT_NEEDED,
+            connect_callback.GetResult(
+                client_socket_->Connect(connect_callback.callback())));
+
+  scoped_refptr<SSLCertRequestInfo> request_info = new SSLCertRequestInfo();
+  client_socket_->GetSSLCertRequestInfo(request_info.get());
+
+  // Check that the authority name that arrived in the CertificateRequest
+  // handshake message is as expected.
+  scoped_refptr<X509Certificate> client_cert =
+      ImportCertFromFile(GetTestCertsDirectory(), kClientCertFileName);
+  EXPECT_TRUE(client_cert->IsIssuedByEncoded(request_info->cert_authorities));
+
+  client_socket_->Disconnect();
+
+  EXPECT_EQ(ERR_FAILED, handshake_callback.GetResult(server_ret));

[davidben, 2016/01/25 20:56:11]

Why is this mapping to such a generic error code?

[ryanchung, 2016/01/29 23:22:13]

This is the error code when the connection is closed. In this case, the client
closed the connection.
I assume the server asks the client for a certificate but the client doesn't
even send back an empty certificate and so Disconnect() needs to be called. Is
that correct?
Otherwise, I'm a little confused because SSL_VERIFY_PEER |
SSL_VERIFY_FAIL_IF_NO_PEER_CERT was set on the server socket which means the
server socket should error out (when the client connects without a client
certificate) without needing the client to disconnect.

[davidben, 2016/02/04 00:40:11]

Oh, right. I haven't ported the better error handling from SSLClientSocket over
to SSLServerSocket, so it loses all the transport errors.
Yeah, that's right. We probably should Disconnect() it ourselves or something.
We kinda just let the socket sit there and usually a higher-level part of the
stack destroys the socket which causes it to close (or maybe reset?) on it.

+}
+
+TEST_F(SSLServerSocketTest, HandshakeWithWrongClientCertSupplied) {
+  scoped_refptr<X509Certificate> client_cert =
+      ImportCertFromFile(GetTestCertsDirectory(), kClientCertFileName);
+  ConfigureClientCertsForClient(kWrongClientCertFileName,
+                                kWrongClientPrivateKeyFileName);
+  ConfigureClientCertsForServer(true);
+  Initialize();
+
+  TestCompletionCallback handshake_callback;
+  int server_ret = server_socket_->Handshake(handshake_callback.callback());
+  EXPECT_EQ(server_ret, ERR_IO_PENDING);

[davidben, 2016/01/25 20:56:11]

(Ditto.)

[ryanchung, 2016/01/29 23:22:13]

Done.

+
+  TestCompletionCallback connect_callback;
+  int client_ret = client_socket_->Connect(connect_callback.callback());
+
+  EXPECT_EQ(ERR_BAD_SSL_CLIENT_AUTH_CERT,
+            connect_callback.GetResult(client_ret));
+  EXPECT_EQ(ERR_BAD_SSL_CLIENT_AUTH_CERT,
+            handshake_callback.GetResult(server_ret));
+}
+#endif  // defined(USE_OPENSSL)
+
 TEST_F(SSLServerSocketTest, DataTransfer) {
   Initialize();
 
+  // Establish connection.
   TestCompletionCallback connect_callback;
-  TestCompletionCallback handshake_callback;
-
-  // Establish connection.
   int client_ret = client_socket_->Connect(connect_callback.callback());
   ASSERT_TRUE(client_ret == OK || client_ret == ERR_IO_PENDING);
 
+  TestCompletionCallback handshake_callback;
   int server_ret = server_socket_->Handshake(handshake_callback.callback());
   ASSERT_TRUE(server_ret == OK || server_ret == ERR_IO_PENDING);
 
   client_ret = connect_callback.GetResult(client_ret);
   ASSERT_EQ(OK, client_ret);
   server_ret = handshake_callback.GetResult(server_ret);
   ASSERT_EQ(OK, server_ret);
 
   const int kReadBufSize = 1024;
   scoped_refptr<StringIOBuffer> write_buf =
@@ skipping 57 matching lines @@
   EXPECT_EQ(0, memcmp(write_buf->data(), read_buf->data(), write_buf->size()));
 }
 
 // A regression test for bug 127822 (http://crbug.com/127822).
 // If the server closes the connection after the handshake is finished,
 // the client's Write() call should not cause an infinite loop.
 // NOTE: this is a test for SSLClientSocket rather than SSLServerSocket.
 TEST_F(SSLServerSocketTest, ClientWriteAfterServerClose) {
   Initialize();
 
-  TestCompletionCallback connect_callback;
-  TestCompletionCallback handshake_callback;
 
   // Establish connection.
+  TestCompletionCallback connect_callback;
   int client_ret = client_socket_->Connect(connect_callback.callback());
   ASSERT_TRUE(client_ret == OK || client_ret == ERR_IO_PENDING);
 
+  TestCompletionCallback handshake_callback;
   int server_ret = server_socket_->Handshake(handshake_callback.callback());
   ASSERT_TRUE(server_ret == OK || server_ret == ERR_IO_PENDING);
 
   client_ret = connect_callback.GetResult(client_ret);
   ASSERT_EQ(OK, client_ret);
   server_ret = handshake_callback.GetResult(server_ret);
   ASSERT_EQ(OK, server_ret);
 
   scoped_refptr<StringIOBuffer> write_buf = new StringIOBuffer("testing123");
 
   // The server closes the connection. The server needs to write some
   // data first so that the client's Read() calls from the transport
   // socket won't return ERR_IO_PENDING.  This ensures that the client
   // will call Read() on the transport socket again.
   TestCompletionCallback write_callback;
-
   server_ret = server_socket_->Write(
       write_buf.get(), write_buf->size(), write_callback.callback());
   EXPECT_TRUE(server_ret > 0 || server_ret == ERR_IO_PENDING);
 
   server_ret = write_callback.GetResult(server_ret);
   EXPECT_GT(server_ret, 0);
 
   server_socket_->Disconnect();
 
   // The client writes some data. This should not cause an infinite loop.
@@ skipping 10 matching lines @@
   base::MessageLoop::current()->Run();
 }
 
 // This test executes ExportKeyingMaterial() on the client and server sockets,
 // after connecting them, and verifies that the results match.
 // This test will fail if False Start is enabled (see crbug.com/90208).
 TEST_F(SSLServerSocketTest, ExportKeyingMaterial) {
   Initialize();
 
   TestCompletionCallback connect_callback;
-  TestCompletionCallback handshake_callback;
-
   int client_ret = client_socket_->Connect(connect_callback.callback());
   ASSERT_TRUE(client_ret == OK || client_ret == ERR_IO_PENDING);
 
+  TestCompletionCallback handshake_callback;
   int server_ret = server_socket_->Handshake(handshake_callback.callback());
   ASSERT_TRUE(server_ret == OK || server_ret == ERR_IO_PENDING);
 
   if (client_ret == ERR_IO_PENDING) {
     ASSERT_EQ(OK, connect_callback.WaitForResult());
   }
   if (server_ret == ERR_IO_PENDING) {
     ASSERT_EQ(OK, handshake_callback.WaitForResult());
   }
 
@@ skipping 39 matching lines @@
   };
   client_ssl_config_.disabled_cipher_suites.assign(
       kEcdheCiphers, kEcdheCiphers + arraysize(kEcdheCiphers));
 
   // Require ECDHE on the server.
   server_ssl_config_.require_ecdhe = true;
 
   Initialize();
 
   TestCompletionCallback connect_callback;
+  int client_ret = client_socket_->Connect(connect_callback.callback());
+
   TestCompletionCallback handshake_callback;
-
-  int client_ret = client_socket_->Connect(connect_callback.callback());
   int server_ret = server_socket_->Handshake(handshake_callback.callback());
 
   client_ret = connect_callback.GetResult(client_ret);
   server_ret = handshake_callback.GetResult(server_ret);
 
   ASSERT_EQ(ERR_SSL_VERSION_OR_CIPHER_MISMATCH, client_ret);
   ASSERT_EQ(ERR_SSL_VERSION_OR_CIPHER_MISMATCH, server_ret);
 }
 
 }  // namespace net