Add tests for session cache and false start behavior.

net/socket/ssl_client_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.
 
 #include "net/socket/ssl_client_socket.h"
 
 #include "base/callback_helpers.h"
 #include "base/memory/ref_counted.h"
 #include "base/run_loop.h"
 #include "net/base/address_list.h"
@@ skipping 581 matching lines @@
   SSLClientSocketTest()
       : socket_factory_(ClientSocketFactory::GetDefaultFactory()),
         cert_verifier_(new MockCertVerifier),
         transport_security_state_(new TransportSecurityState) {
     cert_verifier_->set_default_result(OK);
     context_.cert_verifier = cert_verifier_.get();
     context_.transport_security_state = transport_security_state_.get();
   }
 
  protected:
-  // Sets up a TCP connection to a HTTPS server. To actually do the SSL
-  // handshake, follow up with call to CreateAndConnectSSLClientSocket() below.
-  bool ConnectToTestServer(SpawnedTestServer::SSLOptions& ssl_options) {
+  bool StartTestServer(const SpawnedTestServer::SSLOptions& ssl_options) {
     test_server_.reset(new SpawnedTestServer(
         SpawnedTestServer::TYPE_HTTPS, ssl_options, base::FilePath()));
     if (!test_server_->Start()) {
       LOG(ERROR) << "Could not start SpawnedTestServer";
       return false;
     }
 
     if (!test_server_->GetAddressList(&addr_)) {
       LOG(ERROR) << "Could not get SpawnedTestServer address list";
       return false;
     }
+    return true;
+  }
+
+  // Sets up a TCP connection to a HTTPS server. To actually do the SSL
+  // handshake, follow up with call to CreateAndConnectSSLClientSocket() below.
+  bool ConnectToTestServer(const SpawnedTestServer::SSLOptions& ssl_options) {
+    if (!StartTestServer(ssl_options))
+      return false;
 
     transport_.reset(new TCPClientSocket(addr_, &log_, NetLog::Source()));
     int rv = callback_.GetResult(transport_->Connect(callback_.callback()));
     if (rv != OK) {
       LOG(ERROR) << "Could not connect to SpawnedTestServer";
       return false;
     }
     return true;
   }
 
@@ skipping 27 matching lines @@
     *result = callback_.GetResult(sock_->Connect(callback_.callback()));
     return true;
   }
 
   ClientSocketFactory* socket_factory_;
   scoped_ptr<MockCertVerifier> cert_verifier_;
   scoped_ptr<TransportSecurityState> transport_security_state_;
   SSLClientSocketContext context_;
   scoped_ptr<SSLClientSocket> sock_;
   CapturingNetLog log_;
+  AddressList addr_;
+  scoped_ptr<SpawnedTestServer> test_server_;

[Ryan Sleevi, 2014/06/02 20:52:45]

You don't actually need to expose these as protected (allowing mutation), do
you? You could just expose getters for them.

In the above cases, we mutate them in derived classes, but here, I think you're
just exposing for visibility, right?

If so, expose simple getters (const?)

[davidben, 2014/06/03 19:02:47]

Done.

 
  private:
   scoped_ptr<StreamSocket> transport_;
-  scoped_ptr<SpawnedTestServer> test_server_;
   TestCompletionCallback callback_;
-  AddressList addr_;
 };
 
 // Verifies the correctness of GetSSLCertRequestInfo.
 class SSLClientSocketCertRequestInfoTest : public SSLClientSocketTest {
  protected:
   // Creates a test server with the given SSLOptions, connects to it and returns
   // the SSLCertRequestInfo reported by the socket.
   scoped_refptr<SSLCertRequestInfo> GetCertRequest(
       SpawnedTestServer::SSLOptions ssl_options) {
     SpawnedTestServer test_server(
@@ skipping 27 matching lines @@
     EXPECT_FALSE(sock->IsConnected());
     EXPECT_TRUE(
         test_server.host_port_pair().Equals(request_info->host_and_port));
 
     return request_info;
   }
 };
 
 class SSLClientSocketFalseStartTest : public SSLClientSocketTest {
  protected:
-  void TestFalseStart(const SpawnedTestServer::SSLOptions& server_options,
-                      const SSLConfig& client_config,
-                      bool expect_false_start) {
-    SpawnedTestServer test_server(SpawnedTestServer::TYPE_HTTPS,
-                                  server_options,
-                                  base::FilePath());
-    ASSERT_TRUE(test_server.Start());
+  // Creates an SSLClientSocket with |client_config| attached to a
+  // FakeBlockingStreamSocket, returning both in |*out_raw_transport| and
+  // |*out_sock|. The FakeBlockingStreamSocket is owned by the SSLClientSocket,
+  // so |*out_raw_transport| is a raw pointer.
+  //
+  // The client socket will begin a connect using |callback| but
+  // stop before the server's finished message is received. The finished message
+  // will be blocked in |*out_raw_transport|.
+  //
+  // If the client successfully false started with the server,
+  // |callback.WaitForResult()| will return OK without unblocking transport
+  // reads.
+  //
+  // Must be called after StartTestServer is called.

[Ryan Sleevi, 2014/06/02 20:52:45]

I'm guessing to continue the handshake, the caller needs to ublock
out_raw_transport? If so, worth documenting.

[davidben, 2014/06/03 19:02:47]

Done.

+  void CreateAndConnectUntilServerFinishedReceived(
+      const SSLConfig& client_config,
+      TestCompletionCallback* callback,
+      FakeBlockingStreamSocket** out_raw_transport,
+      scoped_ptr<SSLClientSocket>* out_sock) {
+    CHECK(test_server_);
 
-    AddressList addr;
-    ASSERT_TRUE(test_server.GetAddressList(&addr));
-
-    TestCompletionCallback callback;
     scoped_ptr<StreamSocket> real_transport(
-        new TCPClientSocket(addr, NULL, NetLog::Source()));
+        new TCPClientSocket(addr_, NULL, NetLog::Source()));
     scoped_ptr<FakeBlockingStreamSocket> transport(
         new FakeBlockingStreamSocket(real_transport.Pass()));
-    int rv = callback.GetResult(transport->Connect(callback.callback()));
+    int rv = callback->GetResult(transport->Connect(callback->callback()));
     EXPECT_EQ(OK, rv);
 
     FakeBlockingStreamSocket* raw_transport = transport.get();
-    scoped_ptr<SSLClientSocket> sock(
+    scoped_ptr<SSLClientSocket> sock =
         CreateSSLClientSocket(transport.PassAs<StreamSocket>(),
-                              test_server.host_port_pair(),
-                              client_config));
+                              test_server_->host_port_pair(),
+                              client_config);
 
     // Connect. Stop before the client processes the first server leg
     // (ServerHello, etc.)
     raw_transport->BlockReadResult();
-    rv = sock->Connect(callback.callback());
+    rv = sock->Connect(callback->callback());
     EXPECT_EQ(ERR_IO_PENDING, rv);
     raw_transport->WaitForReadResult();
 
     // Release the ServerHello and wait for the client to write
     // ClientKeyExchange, etc. (A proxy for waiting for the entirety of the
     // server's leg to complete, since it may span multiple reads.)
-    EXPECT_FALSE(callback.have_result());
+    EXPECT_FALSE(callback->have_result());
     raw_transport->BlockWrite();
     raw_transport->UnblockReadResult();
     raw_transport->WaitForWrite();
 
     // And, finally, release that and block the next server leg
-    // (ChangeCipherSpec, Finished). Note: callback.have_result() may or may not
-    // be true at this point depending on whether the SSL implementation waits
-    // for the client second leg to clear the internal write buffer and hit the
-    // network.
+    // (ChangeCipherSpec, Finished). Note: callback->have_result() may or may
+    // not be true at this point depending on whether the SSL implementation
+    // waits for the client second leg to clear the internal write buffer and
+    // hit the network.
     raw_transport->BlockReadResult();
     raw_transport->UnblockWrite();
 
+    *out_raw_transport = raw_transport;
+    *out_sock = sock.Pass();
+  }
+
+  void TestFalseStart(const SpawnedTestServer::SSLOptions& server_options,
+                      const SSLConfig& client_config,
+                      bool expect_false_start) {
+    ASSERT_TRUE(StartTestServer(server_options));
+
+    TestCompletionCallback callback;
+    FakeBlockingStreamSocket* raw_transport;
+    scoped_ptr<SSLClientSocket> sock;
+    ASSERT_NO_FATAL_FAILURE(CreateAndConnectUntilServerFinishedReceived(
+        client_config, &callback, &raw_transport, &sock));
+
     if (expect_false_start) {
       // When False Starting, the handshake should complete before receiving the
       // Change Cipher Spec and Finished messages.
-      rv = callback.GetResult(rv);
+      int rv = callback.WaitForResult();

[Ryan Sleevi, 2014/06/02 20:52:45]

Should you first ASSERT_TRUE(callback.have_result())? Otherwise, don't you
create the opportunity to deadlock here (as long as the blocking transport is,
well, blocked?)

[davidben, 2014/06/03 19:02:47]

It didn't end up being true for NSS. There's a comment although... actually I
think it's totally wrong. I was very confused about nss_memio's behavior at
time.

I poked at it some and it's from the NSS task runner. The event loop iteration
on the network task runner writes out the client second leg (and triggers
WaitForWrite) is not the same event loop iteration where it learns the handshake
is complete. (Core::DoTransportIO is called before Core::DoConnectCallback.)

Certificate verification being potentially asynchronous would also prevent us
from reliably asserting here I think.

I've moved and updated the comment to not be false.

       EXPECT_EQ(OK, rv);
       EXPECT_TRUE(sock->IsConnected());
 
       const char request_text[] = "GET / HTTP/1.0\r\n\r\n";
       static const int kRequestTextSize =
           static_cast<int>(arraysize(request_text) - 1);
       scoped_refptr<IOBuffer> request_buffer(new IOBuffer(kRequestTextSize));
       memcpy(request_buffer->data(), request_text, kRequestTextSize);
 
       // Write the request.
@@ skipping 1673 matching lines @@
 }
 
 TEST_F(SSLClientSocketFalseStartTest, FalseStartEnabled) {
   // False Start requires NPN and a forward-secret cipher suite.
   SpawnedTestServer::SSLOptions server_options;
   server_options.key_exchanges =
       SpawnedTestServer::SSLOptions::KEY_EXCHANGE_DHE_RSA;
   server_options.enable_npn = true;
   SSLConfig client_config;
   client_config.next_protos.push_back("http/1.1");
-  TestFalseStart(server_options, client_config, true);
+  ASSERT_NO_FATAL_FAILURE(
+      TestFalseStart(server_options, client_config, true));
 }
 
 // Test that False Start is disabled without NPN.
 TEST_F(SSLClientSocketFalseStartTest, NoNPN) {
   SpawnedTestServer::SSLOptions server_options;
   server_options.key_exchanges =
       SpawnedTestServer::SSLOptions::KEY_EXCHANGE_DHE_RSA;
   SSLConfig client_config;
   client_config.next_protos.clear();
-  TestFalseStart(server_options, client_config, false);
+  ASSERT_NO_FATAL_FAILURE(
+      TestFalseStart(server_options, client_config, false));
 }
 
 // Test that False Start is disabled without a forward-secret cipher suite.
 TEST_F(SSLClientSocketFalseStartTest, NoForwardSecrecy) {
   SpawnedTestServer::SSLOptions server_options;
   server_options.key_exchanges =
       SpawnedTestServer::SSLOptions::KEY_EXCHANGE_RSA;
   server_options.enable_npn = true;
   SSLConfig client_config;
   client_config.next_protos.push_back("http/1.1");
-  TestFalseStart(server_options, client_config, false);
+  ASSERT_NO_FATAL_FAILURE(
+      TestFalseStart(server_options, client_config, false));
+}
+
+// Test that sessions are resumable after receiving the server Finished message.
+TEST_F(SSLClientSocketFalseStartTest, SessionResumption) {
+  // Start a server.
+  SpawnedTestServer::SSLOptions server_options;
+  server_options.key_exchanges =
+      SpawnedTestServer::SSLOptions::KEY_EXCHANGE_DHE_RSA;
+  server_options.enable_npn = true;
+  SSLConfig client_config;
+  client_config.next_protos.push_back("http/1.1");
+
+  // Let a full handshake complete with False Start.
+  ASSERT_NO_FATAL_FAILURE(
+      TestFalseStart(server_options, client_config, true));
+
+  // Make a second connection.
+  TestCompletionCallback callback;
+  scoped_ptr<StreamSocket> transport2(
+      new TCPClientSocket(addr_, &log_, NetLog::Source()));
+  EXPECT_EQ(OK, callback.GetResult(transport2->Connect(callback.callback())));
+  scoped_ptr<SSLClientSocket> sock2 = CreateSSLClientSocket(
+      transport2.Pass(), test_server_->host_port_pair(), client_config);
+  EXPECT_EQ(OK, callback.GetResult(sock2->Connect(callback.callback())));

[Ryan Sleevi, 2014/06/02 20:52:45]

ASSERT_TRUE(sock2.get())?

[davidben, 2014/06/03 19:02:47]

Done.

+
+  // It should resume the session.
+  SSLInfo ssl_info;
+  EXPECT_TRUE(sock2->GetSSLInfo(&ssl_info));
+  EXPECT_EQ(SSLInfo::HANDSHAKE_RESUME, ssl_info.handshake_type);
+}
+
+// Test that sessions are not resumable before receiving the server Finished
+// message.
+TEST_F(SSLClientSocketFalseStartTest, NoSessionResumptionBeforeFinish) {
+  // Start a server.
+  SpawnedTestServer::SSLOptions server_options;
+  server_options.key_exchanges =
+      SpawnedTestServer::SSLOptions::KEY_EXCHANGE_DHE_RSA;
+  server_options.enable_npn = true;
+  ASSERT_TRUE(StartTestServer(server_options));
+
+  SSLConfig client_config;
+  client_config.next_protos.push_back("http/1.1");
+
+  // Start a handshake up to the server Finished message.
+  TestCompletionCallback callback;
+  FakeBlockingStreamSocket* raw_transport1;
+  scoped_ptr<SSLClientSocket> sock1;
+  ASSERT_NO_FATAL_FAILURE(CreateAndConnectUntilServerFinishedReceived(
+      client_config, &callback, &raw_transport1, &sock1));
+  // Although raw_transport1 has the server Finished blocked, the handshake
+  // still completes.
+  EXPECT_EQ(OK, callback.WaitForResult());

[Ryan Sleevi, 2014/06/02 20:52:45]

Is this meant to be testing false starting worked?

if so, same comment as above re: have_result()

[davidben, 2014/06/03 19:02:47]

More just for waiting for the connect callback to complete. FalseStartEnabled
should already test that false starting worked since it's the same test. But the
case this tries to test is session resumption during the window between
Connect() completing and server Finished actually being processed, so I want to
wait for Connect() to complete first.

+
+  // Drop the old socket. This is needed because the Python test server can't
+  // service two sockets in parallel.
+  sock1.reset();
+
+  // Start a second connection.
+  scoped_ptr<StreamSocket> transport2(
+      new TCPClientSocket(addr_, &log_, NetLog::Source()));
+  EXPECT_EQ(OK, callback.GetResult(transport2->Connect(callback.callback())));
+  scoped_ptr<SSLClientSocket> sock2 = CreateSSLClientSocket(
+      transport2.Pass(), test_server_->host_port_pair(), client_config);
+  EXPECT_EQ(OK, callback.GetResult(sock2->Connect(callback.callback())));
+
+  // No session resumption because the first connection never received a server
+  // Finished message.
+  SSLInfo ssl_info;
+  EXPECT_TRUE(sock2->GetSSLInfo(&ssl_info));
+  EXPECT_EQ(SSLInfo::HANDSHAKE_FULL, ssl_info.handshake_type);
 }
 
 // Connect to a server using channel id. It should allow the connection.
 TEST_F(SSLClientSocketChannelIDTest, SendChannelID) {
   SpawnedTestServer::SSLOptions ssl_options;
 
   ASSERT_TRUE(ConnectToTestServer(ssl_options));
 
   EnableChannelID();
   SSLConfig ssl_config = kDefaultSSLConfig;
@@ skipping 26 matching lines @@
 
   // TODO(haavardm@opera.com): Due to differences in threading, Linux returns
   // ERR_UNEXPECTED while Mac and Windows return ERR_PROTOCOL_ERROR. Accept all
   // error codes for now.
   // http://crbug.com/373670
   EXPECT_NE(OK, rv);
   EXPECT_FALSE(sock_->IsConnected());
 }
 
 }  // namespace net