Support for client certs in ssl_server_socket.

net/socket/ssl_server_socket_unittest.cc

Index: net/socket/ssl_server_socket_unittest.cc
diff --git a/net/socket/ssl_server_socket_unittest.cc b/net/socket/ssl_server_socket_unittest.cc
index ac2d44ec4136fb88c0573646e91e0e9697eaf69e..e4eb5d617eb1a762a5cd32d7c53b822dfac467ef 100644
--- a/net/socket/ssl_server_socket_unittest.cc
+++ b/net/socket/ssl_server_socket_unittest.cc
@@ -15,6 +15,10 @@
 
 #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>
@@ -31,6 +35,7 @@
 #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"
@@ -40,6 +45,7 @@
 #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"
@@ -47,9 +53,11 @@
 #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"
@@ -59,6 +67,12 @@ 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()
@@ -110,13 +124,19 @@ class FakeDataChannel {
   // 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();
@@ -255,6 +275,64 @@ class FakeSocket : public StreamSocket {
   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.
@@ -302,8 +380,10 @@ class SSLServerSocketTest : public PlatformTest {
   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:
@@ -314,25 +394,11 @@ class SSLServerSocketTest : public PlatformTest {
     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;
@@ -340,18 +406,86 @@ class SSLServerSocketTest : public PlatformTest {
     // 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_;
@@ -362,7 +496,9 @@ class SSLServerSocketTest : public PlatformTest {
   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
@@ -378,21 +514,19 @@ TEST_F(SSLServerSocketTest, Initialize) {
 TEST_F(SSLServerSocketTest, Handshake) {
   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);
+  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;
   int client_ret = client_socket_->Connect(connect_callback.callback());
-  EXPECT_TRUE(client_ret == OK || client_ret == ERR_IO_PENDING);
+  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.

 
-  if (client_ret == ERR_IO_PENDING) {
-    EXPECT_EQ(OK, connect_callback.WaitForResult());
-  }
-  if (server_ret == ERR_IO_PENDING) {
-    EXPECT_EQ(OK, handshake_callback.WaitForResult());
-  }
+  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;
@@ -412,16 +546,105 @@ TEST_F(SSLServerSocketTest, Handshake) {
   EXPECT_TRUE(is_aead);
 }
 
-TEST_F(SSLServerSocketTest, DataTransfer) {
+// 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;
   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);
 
@@ -499,13 +722,13 @@ TEST_F(SSLServerSocketTest, DataTransfer) {
 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);
 
@@ -521,7 +744,6 @@ TEST_F(SSLServerSocketTest, ClientWriteAfterServerClose) {
   // 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);
@@ -552,11 +774,10 @@ 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);
 
@@ -616,9 +837,9 @@ TEST_F(SSLServerSocketTest, RequireEcdheFlag) {
   Initialize();
 
   TestCompletionCallback connect_callback;
-  TestCompletionCallback handshake_callback;
-
   int client_ret = client_socket_->Connect(connect_callback.callback());
+
+  TestCompletionCallback handshake_callback;
   int server_ret = server_socket_->Handshake(handshake_callback.callback());
 
   client_ret = connect_callback.GetResult(client_ret);