This CL corrects a bug in which the OnHandshakeComplete callback for an ssl session was never

called if that session had an empty session id (i.e. the session wasn't added to the cache).

This CL sets the OpenSSL info_callback such that CheckIfSessionAdded will be run whenever
an OpenSSL session has finished its handshake. Previously, CheckIfSessionAdded was called in
NewSessionCallbackStatic. NewSessionCallbackStatic is only called when a session is
added to the cache. Thus, leading connections with sessions that were not added to the cache would
force their pending jobs to wait indefinitely instead of letting the jobs proceed at the completion
of the leading job's connection.

R=mmenke@chromium.org,rsleevi@chromium.org,wtc@chromium.org, davidben@chromium.org


BUG=398967

Review URL: https://codereview.chromium.org/416683002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@288024 0039d316-1c4b-4281-b951-d872f2087c98

net/socket/ssl_client_socket_nss.cc

@@ -2843,6 +2843,7 @@ void SSLClientSocketNSS::SetHandshakeCompletionCallback(
     const base::Closure& callback) {
   NOTIMPLEMENTED();
 }
+
 void SSLClientSocketNSS::GetSSLCertRequestInfo(
     SSLCertRequestInfo* cert_request_info) {
   EnterFunction("");

net/socket/ssl_client_socket_openssl.cc

@@ -363,6 +363,8 @@ SSLClientSocketOpenSSL::SSLClientSocketOpenSSL(
       next_handshake_state_(STATE_NONE),
       npn_status_(kNextProtoUnsupported),
       channel_id_xtn_negotiated_(false),
+      ran_handshake_finished_callback_(false),
+      marked_session_as_good_(false),
       net_log_(transport_->socket()->NetLog()) {
 }
 
@@ -435,14 +437,6 @@ int SSLClientSocketOpenSSL::Connect(const CompletionCallback& callback) {
     return rv;
   }
 
-  if (!handshake_completion_callback_.is_null()) {
-    SSLContext* context = SSLContext::GetInstance();
-    context->session_cache()->SetSessionAddedCallback(
-        ssl_,
-        base::Bind(&SSLClientSocketOpenSSL::OnHandshakeCompletion,
-                   base::Unretained(this)));
-  }
-
   // Set SSL to client mode. Handshake happens in the loop below.
   SSL_set_connect_state(ssl_);
 
@@ -465,8 +459,6 @@ void SSLClientSocketOpenSSL::Disconnect() {
   // completed, this is a no-op.
   OnHandshakeCompletion();
   if (ssl_) {
-    SSLContext* context = SSLContext::GetInstance();
-    context->session_cache()->RemoveSessionAddedCallback(ssl_);
     // Calling SSL_shutdown prevents the session from being marked as
     // unresumable.
     SSL_shutdown(ssl_);
@@ -687,6 +679,18 @@ int SSLClientSocketOpenSSL::SetSendBufferSize(int32 size) {
   return transport_->socket()->SetSendBufferSize(size);
 }
 
+// static
+void SSLClientSocketOpenSSL::InfoCallback(const SSL* ssl,
+                                          int result,
+                                          int /*unused*/) {
+  SSLClientSocketOpenSSL* ssl_socket =
+      SSLContext::GetInstance()->GetClientSocketFromSSL(ssl);
+  if (result == SSL_CB_HANDSHAKE_DONE) {
+    ssl_socket->ran_handshake_finished_callback_ = true;
+    ssl_socket->CheckIfHandshakeFinished();
+  }
+}
+
 int SSLClientSocketOpenSSL::Init() {
   DCHECK(!ssl_);
   DCHECK(!transport_bio_);
@@ -701,6 +705,9 @@ int SSLClientSocketOpenSSL::Init() {
   if (!SSL_set_tlsext_host_name(ssl_, host_and_port_.host().c_str()))
     return ERR_UNEXPECTED;
 
+  // Set an OpenSSL callback to monitor this SSL*'s connection.
+  SSL_set_info_callback(ssl_, &InfoCallback);
+
   trying_cached_session_ = context->session_cache()->SetSSLSessionWithKey(
       ssl_, GetSessionCacheKey());
 
@@ -1032,6 +1039,8 @@ int SSLClientSocketOpenSSL::DoVerifyCertComplete(int result) {
     // TODO(joth): Work out if we need to remember the intermediate CA certs
     // when the server sends them to us, and do so here.
     SSLContext::GetInstance()->session_cache()->MarkSSLSessionAsGood(ssl_);
+    marked_session_as_good_ = true;
+    CheckIfHandshakeFinished();
   } else {
     DVLOG(1) << "DoVerifyCertComplete error " << ErrorToString(result)
              << " (" << result << ")";
@@ -1577,6 +1586,19 @@ long SSLClientSocketOpenSSL::MaybeReplayTransportError(
   return retvalue;
 }
 
+// Determines if the session for |ssl_| is in the cache, and calls the
+// handshake completion callback if that is the case.
+//
+// CheckIfHandshakeFinished is called twice per connection: once after
+// MarkSSLSessionAsGood, when the certificate has been verified, and
+// once via an OpenSSL callback when the handshake has completed. On the
+// second call, when the certificate has been verified and the handshake
+// has completed, the connection's handshake completion callback is run.
+void SSLClientSocketOpenSSL::CheckIfHandshakeFinished() {
+  if (ran_handshake_finished_callback_ && marked_session_as_good_)
+    OnHandshakeCompletion();
+}
+
 // static
 long SSLClientSocketOpenSSL::BIOCallback(
     BIO *bio,

net/socket/ssl_client_socket_openssl.h

@@ -106,6 +106,10 @@ class SSLClientSocketOpenSSL : public SSLClientSocket {
   friend class SSLClientSocket;
   friend class SSLContext;
 
+  // Callback that is run by OpenSSL to obtain information about the
+  // state of the SSL handshake.
+  static void InfoCallback(const SSL* ssl, int result, int unused);
+
   int Init();
   void DoReadCallback(int result);
   void DoWriteCallback(int result);
@@ -168,6 +172,8 @@ class SSLClientSocketOpenSSL : public SSLClientSocket {
                           const char *argp, int argi, long argl,
                           long retvalue);
 
+  void CheckIfHandshakeFinished();
+
   bool transport_send_busy_;
   bool transport_recv_busy_;
 
@@ -268,6 +274,11 @@ class SSLClientSocketOpenSSL : public SSLClientSocket {
   std::string channel_id_cert_;
   // True if channel ID extension was negotiated.
   bool channel_id_xtn_negotiated_;
+  // True if InfoCallback has been run with result = SSL_CB_HANDSHAKE_DONE.
+  bool ran_handshake_finished_callback_;
+  // True if MarkSSLSessionAsGood has been called for this socket's
+  // connection's SSL session.
+  bool marked_session_as_good_;
   // The request handle for |channel_id_service_|.
   ChannelIDService::RequestHandle channel_id_request_handle_;
   BoundNetLog net_log_;

net/socket/ssl_client_socket_unittest.cc

@@ -2721,9 +2721,6 @@ TEST_F(SSLClientSocketTest, HandshakeCallbackIsRun_WithSuccess) {
   sock->SetHandshakeCompletionCallback(base::Bind(
       &SSLClientSocketTest::RecordCompletedHandshake, base::Unretained(this)));
 
-  if (sock->IsConnected())
-    LOG(ERROR) << "SSL Socket prematurely connected";
-
   rv = callback.GetResult(sock->Connect(callback.callback()));
 
   EXPECT_EQ(OK, rv);
@@ -2731,6 +2728,42 @@ TEST_F(SSLClientSocketTest, HandshakeCallbackIsRun_WithSuccess) {
   EXPECT_TRUE(ran_handshake_completion_callback_);
 }
 
+// Tests that the completion callback is run with connections
+// that do not cache their session.
+TEST_F(SSLClientSocketTest, HandshakeCallbackIsRun_WithDisabledSessionCache) {
+  SpawnedTestServer::SSLOptions ssl_options;
+  ssl_options.disable_session_cache = true;
+  SpawnedTestServer test_server(
+      SpawnedTestServer::TYPE_HTTPS, ssl_options, base::FilePath());
+  ASSERT_TRUE(test_server.Start());
+
+  AddressList addr;
+  ASSERT_TRUE(test_server.GetAddressList(&addr));
+
+  scoped_ptr<StreamSocket> transport(
+      new TCPClientSocket(addr, NULL, NetLog::Source()));
+
+  TestCompletionCallback callback;
+  int rv = transport->Connect(callback.callback());
+  if (rv == ERR_IO_PENDING)
+    rv = callback.WaitForResult();
+  EXPECT_EQ(OK, rv);
+
+  SSLConfig ssl_config = kDefaultSSLConfig;
+  ssl_config.false_start_enabled = false;
+
+  scoped_ptr<SSLClientSocket> sock(CreateSSLClientSocket(
+      transport.Pass(), test_server.host_port_pair(), ssl_config));
+
+  sock->SetHandshakeCompletionCallback(base::Bind(
+      &SSLClientSocketTest::RecordCompletedHandshake, base::Unretained(this)));
+
+  rv = callback.GetResult(sock->Connect(callback.callback()));
+
+  EXPECT_EQ(OK, rv);
+  EXPECT_TRUE(sock->IsConnected());
+  EXPECT_TRUE(ran_handshake_completion_callback_);
+}
 #endif  // defined(USE_OPENSSL)
 
 TEST_F(SSLClientSocketFalseStartTest, FalseStartEnabled) {

net/socket/ssl_session_cache_openssl.cc

@@ -253,31 +253,6 @@ class SSLSessionCacheOpenSSLImpl {
     return session_is_good;
   }
 
-  void SetSessionAddedCallback(SSL* ssl, const base::Closure& callback) {
-    // Add |ssl| to the SSLToCallbackMap.
-    ssl_to_callback_map_.insert(SSLToCallbackMap::value_type(
-        ssl, CallbackAndCompletionCount(callback, 0)));
-  }
-
-  // Determines if the session for |ssl| is in the cache, and calls the
-  // appropriate callback if that is the case.
-  //
-  // The session must be both MarkedAsGood and Added in order for the
-  // callback to be run. These two events can occur in either order.
-  void CheckIfSessionFinished(SSL* ssl) {
-    SSLToCallbackMap::iterator it = ssl_to_callback_map_.find(ssl);
-    if (it == ssl_to_callback_map_.end())
-      return;
-    // Increment the session's completion count.
-    if (++it->second.count == 2) {
-      base::Closure callback = it->second.callback;
-      ssl_to_callback_map_.erase(it);
-      callback.Run();
-    }
-  }
-
-  void RemoveSessionAddedCallback(SSL* ssl) { ssl_to_callback_map_.erase(ssl); }
-
   void MarkSSLSessionAsGood(SSL* ssl) {
     SSL_SESSION* session = SSL_get_session(ssl);
     CHECK(session);
@@ -285,8 +260,6 @@ class SSLSessionCacheOpenSSLImpl {
     // Mark the session as good, allowing it to be used for future connections.
     SSL_SESSION_set_ex_data(
         session, GetSSLSessionExIndex(), reinterpret_cast<void*>(1));
-
-    CheckIfSessionFinished(ssl);
   }
 
   // Flush all entries from the cache.
@@ -302,30 +275,12 @@ class SSLSessionCacheOpenSSLImpl {
   }
 
  private:
-  // CallbackAndCompletionCounts are used to group a callback that should be
-  // run when a certain sesssion is added to the session cache with an integer
-  // indicating the status of that session.
-  struct CallbackAndCompletionCount {
-    CallbackAndCompletionCount(const base::Closure& completion_callback,
-                               int completion_count)
-        : callback(completion_callback), count(completion_count) {}
-
-    const base::Closure callback;
-    // |count| < 2 means that the ssl session associated with this object
-    // has not been added to the session cache or has not been marked as good.
-    // |count| is incremented when a session is added to the cache or marked as
-    // good, thus |count| == 2 means that the session is ready for use.
-    int count;
-  };
-
   // Type for list of SSL_SESSION handles, ordered in MRU order.
   typedef std::list<SSL_SESSION*> MRUSessionList;
   // Type for a dictionary from unique cache keys to session list nodes.
   typedef base::hash_map<std::string, MRUSessionList::iterator> KeyIndex;
   // Type for a dictionary from SessionId values to key index nodes.
   typedef base::hash_map<SessionId, KeyIndex::iterator> SessionIdIndex;
-  // Type for a map from SSL* to associated callbacks
-  typedef std::map<SSL*, CallbackAndCompletionCount> SSLToCallbackMap;
 
   // Return the key associated with a given session, or the empty string if
   // none exist. This shall only be used for debugging.
@@ -405,7 +360,6 @@ class SSLSessionCacheOpenSSLImpl {
   static int NewSessionCallbackStatic(SSL* ssl, SSL_SESSION* session) {
     SSLSessionCacheOpenSSLImpl* cache = GetCache(ssl->ctx);
     cache->OnSessionAdded(ssl, session);
-    cache->CheckIfSessionFinished(ssl);
     return 1;
   }
 
@@ -529,7 +483,6 @@ class SSLSessionCacheOpenSSLImpl {
 
   SSL_CTX* ctx_;
   SSLSessionCacheOpenSSL::Config config_;
-  SSLToCallbackMap ssl_to_callback_map_;
 
   // method to get the index which can later be used with SSL_CTX_get_ex_data()
   // or SSL_CTX_set_ex_data().
@@ -568,15 +521,6 @@ bool SSLSessionCacheOpenSSL::SSLSessionIsInCache(
   return impl_->SSLSessionIsInCache(cache_key);
 }
 
-void SSLSessionCacheOpenSSL::RemoveSessionAddedCallback(SSL* ssl) {
-  impl_->RemoveSessionAddedCallback(ssl);
-}
-
-void SSLSessionCacheOpenSSL::SetSessionAddedCallback(SSL* ssl,
-                                                     const base::Closure& cb) {
-  impl_->SetSessionAddedCallback(ssl, cb);
-}
-
 void SSLSessionCacheOpenSSL::MarkSSLSessionAsGood(SSL* ssl) {
   return impl_->MarkSSLSessionAsGood(ssl);
 }

net/socket/ssl_session_cache_openssl.h

@@ -117,13 +117,6 @@ class NET_EXPORT SSLSessionCacheOpenSSL {
   // Return true iff a cached session was associated with the given |cache_key|.
   bool SSLSessionIsInCache(const std::string& cache_key) const;
 
-  // Informs the cache that it should run a callback when |ssl|'s session is
-  // added to the cache.
-  void SetSessionAddedCallback(SSL* ssl, const base::Closure& callback);
-
-  // Removes the entry for |ssl| from cache's callback map.
-  void RemoveSessionAddedCallback(SSL* ssl);
-
   // Indicates that the SSL session associated with |ssl| is "good" - that is,
   // that all associated cryptographic parameters that were negotiated,
   // including the peer's certificate, were successfully validated. Because

net/test/spawned_test_server/base_test_server.cc

@@ -101,7 +101,9 @@ BaseTestServer::SSLOptions::SSLOptions()
       tls_intolerance_type(TLS_INTOLERANCE_ALERT),
       fallback_scsv_enabled(false),
       staple_ocsp_response(false),
-      enable_npn(false) {}
+      enable_npn(false),
+      disable_session_cache(false) {
+}
 
 BaseTestServer::SSLOptions::SSLOptions(
     BaseTestServer::SSLOptions::ServerCertificate cert)
@@ -116,7 +118,9 @@ BaseTestServer::SSLOptions::SSLOptions(
       tls_intolerance_type(TLS_INTOLERANCE_ALERT),
       fallback_scsv_enabled(false),
       staple_ocsp_response(false),
-      enable_npn(false) {}
+      enable_npn(false),
+      disable_session_cache(false) {
+}
 
 BaseTestServer::SSLOptions::~SSLOptions() {}
 
@@ -474,6 +478,8 @@ bool BaseTestServer::GenerateArguments(base::DictionaryValue* arguments) const {
       arguments->Set("staple-ocsp-response", base::Value::CreateNullValue());
     if (ssl_options_.enable_npn)
       arguments->Set("enable-npn", base::Value::CreateNullValue());
+    if (ssl_options_.disable_session_cache)
+      arguments->Set("disable-session-cache", base::Value::CreateNullValue());
   }
 
   return GenerateAdditionalArguments(arguments);

net/test/spawned_test_server/base_test_server.h

@@ -203,6 +203,10 @@ class BaseTestServer {
 
     // Whether to enable NPN support.
     bool enable_npn;
+
+    // Whether to disable TLS session caching. When session caching is
+    // disabled, the server will generate a new Session ID for every connection.
+    bool disable_session_cache;
   };
 
   // Pass as the 'host' parameter during construction to server on 127.0.0.1

net/tools/testserver/testserver.py

@@ -157,7 +157,7 @@ class HTTPSServer(tlslite.api.TLSSocketServerMixIn,
                ssl_bulk_ciphers, ssl_key_exchanges, enable_npn,
                record_resume_info, tls_intolerant,
                tls_intolerance_type, signed_cert_timestamps,
-               fallback_scsv_enabled, ocsp_response):
+               fallback_scsv_enabled, ocsp_response, disable_session_cache):
     self.cert_chain = tlslite.api.X509CertChain()
     self.cert_chain.parsePemList(pem_cert_and_key)
     # Force using only python implementation - otherwise behavior is different
@@ -201,7 +201,10 @@ class HTTPSServer(tlslite.api.TLSSocketServerMixIn,
       self.ssl_handshake_settings.tlsIntolerant = (3, tls_intolerant)
       self.ssl_handshake_settings.tlsIntoleranceType = tls_intolerance_type
 
-    if record_resume_info:
+
+    if disable_session_cache:
+      self.session_cache = None
+    elif record_resume_info:
       # If record_resume_info is true then we'll replace the session cache with
       # an object that records the lookups and inserts that it sees.
       self.session_cache = RecordingSSLSessionCache()
@@ -1986,7 +1989,8 @@ class ServerRunner(testserver_base.TestServerRunner):
                              self.options.signed_cert_timestamps_tls_ext.decode(
                                  "base64"),
                              self.options.fallback_scsv,
-                             stapled_ocsp_response)
+                             stapled_ocsp_response,
+                             self.options.disable_session_cache)
         print 'HTTPS server started on https://%s:%d...' % \
             (host, server.server_port)
       else:
@@ -2086,6 +2090,11 @@ class ServerRunner(testserver_base.TestServerRunner):
 
   def add_options(self):
     testserver_base.TestServerRunner.add_options(self)
+    self.option_parser.add_option('--disable-session-cache',
+                                  action='store_true',
+                                  dest='disable_session_cache',
+                                  help='tells the server to disable the'
+                                  'TLS session cache.')
     self.option_parser.add_option('-f', '--ftp', action='store_const',
                                   const=SERVER_FTP, default=SERVER_HTTP,
                                   dest='server_type',