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.
 //
@@ skipping 12 matching lines @@
 #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/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));

[davidben, 2015/12/14 23:56:50]

What's this for?

[ryanchung, 2015/12/16 22:40:02]

This is for the client to be able to disconnect and abort the handshake so that
the server socket won't hang waiting for the handshake. DoReadCallback requires
data to be not null.

Affected test: HandshakeWithClientCertRequiredNotSupplied

+    if (!read_callback_.is_null()) {
+      base::MessageLoop::current()->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 {

[davidben, 2015/12/14 23:56:50]

(Ooh, thanks! I might pull this into a utility later to try to get our client
auth tests going.)

+ 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>(kHashes,
+                                            kHashes + arraysize(kHashes));
+  }
+
+  // NOTE: The following algorithm assumes the answer is a power of 2, which is
+  // true for the test keys in use.
+  size_t GetMaxSignatureLengthInBytes() override {
+    std::vector<uint8> public_key_info;

[davidben, 2015/12/14 23:56:50]

New code should use uint8_t.

[ryanchung, 2015/12/16 22:40:02]

Done.

+    rsa_private_key_->ExportPublicKey(&public_key_info);
+    uint result = 1;
+    while ((result << 1) < public_key_info.size())
+      result <<= 1;

[davidben, 2015/12/14 23:56:49]

This also assumes the exponent is small and that the DER wrapping bits aren't
far off. It'd be nice if we could do this without reading into these, but:

#if defined(USE_OPENSSL)
  return EVP_PKEY_size(rsa_private_key_->key());
#else
  NOTIMPLEMENTED();
  return 0;
#endif

[ryanchung, 2015/12/16 22:40:02]

Done.

+    return result;
+  }
+
+  void SignDigest(Hash hash,
+                  const base::StringPiece& input,
+                  const SignCallback& callback) override {
+    std::vector<uint8> 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*>(input.data()),
+                                   input.size(), &signature);
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::Bind(callback, OK, signature));
+  }
+
+ private:
+  ~TestSSLPrivateKey() override {}
+  void CompleteSignDigest(Error err, const std::vector<uint8_t>& signature) {}

[davidben, 2015/12/14 23:56:50]

Nit: newline

[davidben, 2015/12/14 23:56:50]

Unused?

[ryanchung, 2015/12/16 22:40:02]

Done.

+  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 24 matching lines @@
   EXPECT_LE(written, kTestDataSize);
 
   read = callback.WaitForResult();
   EXPECT_GT(read, 0);
   EXPECT_LE(read, written);
   EXPECT_EQ(0, memcmp(kTestData, read_buf->data(), read));
 }
 
 class SSLServerSocketTest : public PlatformTest {
  public:
+  enum ClientCertSupply {

[davidben, 2015/12/14 23:56:50]

Sort of an odd name. Why not just call it ClientCert or ClientCertificate or
something?

[ryanchung, 2015/12/16 22:40:01]

Done.

+    kNoneSupplied = 0,

[davidben, 2015/12/14 23:56:50]

This one is never used.

[ryanchung, 2015/12/16 22:40:02]

Done.

+    kCorrectCertSupplied = 1,
+    kWrongCertSupplied = 2

[davidben, 2015/12/14 23:56:50]

enums NAMED_LIKE_MACROS

https://www.chromium.org/developers/coding-style#Naming

[davidben, 2015/12/14 23:56:50]

Why specify values? I don't think you actually care about the values.

[ryanchung, 2015/12/16 22:40:02]

Done.

+  };
+
+  enum ClientCertExpect { kNoneExpected = 0, kCertRequired = 2 };

[davidben, 2015/12/14 23:56:50]

Ditto to both.

[ryanchung, 2015/12/16 22:40:02]

Done.

+
   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);
+    client_cert_verifier_->set_default_result(CERT_STATUS_AUTHORITY_INVALID);

[davidben, 2015/12/14 23:56:50]

ERR_CERT_AUTHORITY_INVALID.

It looks like the line above had this bug too. set_default_result takes a net
error code, not a CertStatus.

[ryanchung, 2015/12/16 22:40:02]

Done.

   }
 
  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> key_vector(
-        reinterpret_cast<const uint8*>(key_string.data()),
-        reinterpret_cast<const uint8*>(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(
         client_connection.Pass(), host_and_pair, client_ssl_config_, context);
     server_socket_ =
-        CreateSSLServerSocket(server_socket.Pass(), cert.get(),
-                              private_key.get(), server_ssl_config_);
+        CreateSSLServerSocket(server_socket.Pass(), server_cert.get(),
+                              server_private_key.get(), server_ssl_config_);
+  }
+
+  void InitializeClientCertsForClient(ClientCertSupply supply) {

[davidben, 2015/12/14 23:56:50]

Calling this method and the one below InitializeBlah is confusing since you have
to then call plain Initialize. ConfigureClientCertsForClient?

[davidben, 2015/12/14 23:56:50]

This all seems like it ought to use a switch-case. It means that when you add a
new entry, the compiler will notice.

Alternatively, perhaps just have it take two const char*s and pass in the file
names?

[ryanchung, 2015/12/16 22:40:02]

Done.

[ryanchung, 2015/12/16 22:40:02]

Will take two const char* as parameters.

+    scoped_refptr<X509Certificate> cert;
+    scoped_refptr<net::SSLPrivateKey> key;
+    if (supply != kNoneSupplied) {
+      const char* cert_file_name = supply == kCorrectCertSupplied
+                                       ? kClientCertFileName
+                                       : kWrongClientCertFileName;
+      const char* private_key_file_name = supply == kCorrectCertSupplied
+                                              ? kClientPrivateKeyFileName
+                                              : kWrongClientPrivateKeyFileName;
+      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 InitializeClientCertsForServer(ClientCertExpect expect) {
+    if (expect == kNoneExpected)
+      return;
+
+    server_ssl_config_.require_client_cert = true;
+
+    if (expect == kCertRequired) {

[davidben, 2015/12/14 23:56:50]

This is weird. There's only two values for expect.

[ryanchung, 2015/12/16 22:40:01]

Done. Will use a bool instead.

+      scoped_refptr<X509Certificate> expected_client_ca_cert(
+          ImportCertFromFile(GetTestCertsDirectory(), kClientCertCAFileName));
+      CertificateList ca_list;
+      ca_list.push_back(expected_client_ca_cert);
+      server_ssl_config_.client_cert_ca_list = ca_list;
+      scoped_refptr<X509Certificate> expected_client_cert(
+          ImportCertFromFile(GetTestCertsDirectory(), kClientCertFileName));
+      CertVerifyResult ignored;

[davidben, 2015/12/14 23:56:50]

What's this for?

[ryanchung, 2015/12/16 22:40:02]

Sorry, this isn't used after simplifying the client cert verifier. Removed.

+      ignored.verified_cert = expected_client_cert;
+      ignored.cert_status = 0;
+      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,
+                                std::string* cert_der) {
+    base::FilePath certs_dir(GetTestCertsDirectory());
+    base::FilePath cert_path = certs_dir.AppendASCII(name);
+    std::string unneeded;

[davidben, 2015/12/14 23:56:50]

Nit: unneeded -> unused is I think more common?

[ryanchung, 2015/12/16 22:40:02]

Done.

+    if (!cert_der)
+      cert_der = &unneeded;
+    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) {
+    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> key_vector(

[davidben, 2015/12/14 23:56:50]

New code should use uint8_t.

[ryanchung, 2015/12/16 22:40:02]

Done.

+        reinterpret_cast<const uint8*>(key_string.data()),
+        reinterpret_cast<const uint8*>(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_;
 };
 
 // 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
@@ skipping 29 matching lines @@
       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);
 }
 
+// TODO(dougsteed) The following tests using client certificates cannot
+// be performed if NSS with platform-based client auth is in use. That's because
+// the tests use SSLClientSocket to make requests against the server, and on
+// those builds, that class does not support supplying of a test key and cert.
+// An alternative approach that would broaden the applicability of these tests
+// would be to build and use the openssl flavor of SSLClientSocket, even
+// on NSS platforms.
+#if defined(USE_OPENSSL) || !defined(NSS_PLATFORM_CLIENT_AUTH)

[davidben, 2015/12/14 23:56:50]

This comment is out of date. I think you just want #if defined(USE_OPENSSL) now.
Probably something as simple as

// NSS ports don't support client certificates.

(The only NSS port remaining is iOS which doesn't do client auth at all.)

[ryanchung, 2015/12/16 22:40:02]

Done.

+
+// 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);
+  InitializeClientCertsForClient(kCorrectCertSupplied);
+  InitializeClientCertsForServer(kCertRequired);
+  Initialize();
+
+  TestCompletionCallback connect_callback;
+  TestCompletionCallback handshake_callback;
+
+  int server_ret = server_socket_->Handshake(handshake_callback.callback());
+  EXPECT_TRUE(server_ret == OK || server_ret == ERR_IO_PENDING);
+
+  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());
+  }
+
+  // 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.client_cert_sent);
+  EXPECT_TRUE(ssl_info.cert.get());
+  EXPECT_TRUE(client_cert->Equals(ssl_info.cert.get()));
+}
+
+TEST_F(SSLServerSocketTest, HandshakeWithClientCertRequiredNotSupplied) {
+  InitializeClientCertsForServer(kCertRequired);
+  Initialize();
+  // We use the default setting for the client socket. This causes the client to
+  // get SSL_CLIENT_AUTH_CERT_NEEDED. This code path allows us to access the

[davidben, 2015/12/14 23:56:49]

ERR_SSL_CLIENT_AUTH_CERT_NEEDED

[ryanchung, 2015/12/16 22:40:02]

Done.

+  // cert_authorities from the CertificateRequest.
+
+  TestCompletionCallback connect_callback;
+  TestCompletionCallback handshake_callback;
+  int server_ret = server_socket_->Handshake(handshake_callback.callback());
+  EXPECT_TRUE(server_ret == ERR_IO_PENDING);

[davidben, 2015/12/14 23:56:50]

EXPECT_EQ

[ryanchung, 2015/12/16 22:40:02]

Done.

+
+  int client_ret = client_socket_->Connect(connect_callback.callback());
+  EXPECT_TRUE(client_ret == ERR_SSL_CLIENT_AUTH_CERT_NEEDED ||
+              client_ret == ERR_IO_PENDING);
+
+  if (client_ret == ERR_IO_PENDING) {
+    EXPECT_EQ(ERR_SSL_CLIENT_AUTH_CERT_NEEDED,
+              connect_callback.WaitForResult());
+  }

[davidben, 2015/12/14 23:56:50]

This whole thing (line 609 down) can be written:

  EXPECT_EQ(ERR_SSL_CLIENT_AUTH_CERT_NEEDED,
            connect_callback.GetResult(
                client_socket_->Connect(connect_callback.callback())));

[ryanchung, 2015/12/16 22:40:02]

Done.

+
+  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();
+
+  if (server_ret == ERR_IO_PENDING) {
+    server_ret = handshake_callback.WaitForResult();
+    EXPECT_TRUE(server_ret == ERR_CONNECTION_CLOSED ||
+                server_ret == ERR_FAILED);
+  }

[davidben, 2015/12/14 23:56:50]

This should probably be:

  server_ret = handshake_callback.GetResult(server_ret);
  EXPECT_TRUE(.....);

Why do you get the different error codes? Could you add a comment for why
there's two possibilities?

[ryanchung, 2015/12/16 22:40:02]

This was probably due to previous support for both NSS and OpenSSL. I've removed
the extra.

+}
+
+TEST_F(SSLServerSocketTest, HandshakeWithWrongClientCertSupplied) {
+  scoped_refptr<X509Certificate> client_cert =
+      ImportCertFromFile(GetTestCertsDirectory(), kClientCertFileName);
+  InitializeClientCertsForClient(kWrongCertSupplied);
+  InitializeClientCertsForServer(kCertRequired);
+  Initialize();
+
+  TestCompletionCallback connect_callback;
+  TestCompletionCallback handshake_callback;
+
+  int server_ret = server_socket_->Handshake(handshake_callback.callback());
+  EXPECT_TRUE(server_ret == ERR_IO_PENDING);
+
+  int client_ret = client_socket_->Connect(connect_callback.callback());
+  EXPECT_TRUE(client_ret == ERR_BAD_SSL_CLIENT_AUTH_CERT ||
+              client_ret == ERR_IO_PENDING);
+
+  if (client_ret == ERR_IO_PENDING) {
+    EXPECT_EQ(ERR_BAD_SSL_CLIENT_AUTH_CERT, connect_callback.WaitForResult());
+  }

[davidben, 2015/12/14 23:56:50]

Ditto on using GetResult.

[ryanchung, 2015/12/16 22:40:02]

Done.

+
+  server_ret = handshake_callback.WaitForResult();

[davidben, 2015/12/14 23:56:50]

Ditto on using GetResult.

[ryanchung, 2015/12/16 22:40:02]

Done.

+  // We get a different result on NSS and OpenSSL. That's because an error
+  // mapping with OpenSSL makes an assumption that is true for SSLClientSocket
+  // but not SSLServerSocket (namely that peer cert rejection only occurs due to
+  // a cert change during renego).
+  EXPECT_TRUE(server_ret == ERR_BAD_SSL_CLIENT_AUTH_CERT ||
+              server_ret == ERR_SSL_SERVER_CERT_CHANGED);

[davidben, 2015/12/14 23:56:50]

It seems this has been fixed, right?

[ryanchung, 2015/12/16 22:40:02]

Done.

+}
+#endif  // defined(USE_OPENSSL) || !defined(NSS_PLATFORM_CLIENT_AUTH)

[davidben, 2015/12/14 23:56:50]

(Don't forget to remove NSS_PLATFORM_CLIENT_AUTH here too.)

[ryanchung, 2015/12/16 22:40:02]

Done.

+
 TEST_F(SSLServerSocketTest, DataTransfer) {
   Initialize();
 
   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);
 
@@ skipping 197 matching lines @@
   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