Implements new, more robust design for communicating between SSLConnectJobs.

net/socket/ssl_session_cache_openssl.cc

 // Copyright 2013 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_session_cache_openssl.h"
 
 #include <list>
 #include <map>
 
 #include <openssl/rand.h>
 #include <openssl/ssl.h>
 
+#include "base/callback.h"
 #include "base/containers/hash_tables.h"
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/synchronization/lock.h"
 
 namespace net {
 
 namespace {
 
 // A helper class to lazily create a new EX_DATA index to map SSL_CTX handles
@@ skipping 206 matching lines @@
       return false;  // Session has not yet been marked good. Treat as a miss.
 
     // Move to front of MRU list.
     ordering_.push_front(session);
     ordering_.erase(it->second);
     it->second = ordering_.begin();
 
     return SSL_set_session(ssl, session) == 1;
   }
 
+  bool SSLSessionIsInCache(const std::string& cache_key) const {
+    base::AutoLock locked(lock_);
+    KeyIndex::const_iterator it = key_index_.find(cache_key);
+    if (it == key_index.end())
+      return false;
+
+    SSL_SESSION* session = *it->second;
+    DCHECK(session);
+    DVLOG(2) << "Lookup session: " << session << " for " << cache_key;

[wtc, 2014/07/18 01:17:15]

Delete this log message.

[mshelley, 2014/07/18 21:08:33]

Done.

+
+    void* session_is_good =
+        SSL_SESSION_get_ex_data(session, GetSSLSessionExIndex());
+
+    if (!session_is_good)
+      return false;  // Session has not yet been marked good. Treat as a miss.
+    return true;

[wtc, 2014/07/18 01:17:16]

You can replace these three lines with:
    return session_is_good;

[mshelley, 2014/07/18 21:08:33]

Done.

+  }
+
+  void SetSessionAddedCallback(SSL* ssl, const base::Closure& callback) {
+    // Add this SSL* to the SSLtoCallbackMap.
+    ssl_to_callback_map_[ssl] = CallbackAndCompletionCount(callback, 0);
+  }
+
+  // Determines if the session for |ssl| is in the cache, and calls the
+  // appropriate callback if that is the case.
+  void CheckIfSessionAdded(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) {
+      // The session has been MarkedAsGood and Added, so it can be used.
+      // These two events can occur in either order.
+      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);
     if (!session)

[wtc, 2014/07/18 01:17:16]

IMPORTANT: we should find out whether |session| may be NULL here. It seems that
|session| cannot be NULL based on the code at the beginning of ssl3_client_hello
in third_party/openssl/openssl/ssl/s3_clnt.c:

        if (s->state == SSL3_ST_CW_CLNT_HELLO_A)
                {
                SSL_SESSION *sess = s->session;
                if ((sess == NULL) ||
                        (sess->ssl_version != s->version) ||
#ifdef OPENSSL_NO_TLSEXT
                        !sess->session_id_length ||
#else
                        (!sess->session_id_length && !sess->tlsext_tick) ||
#endif
                        (sess->not_resumable))
                        {
                        if (!s->session_creation_enabled)
                                {
                               
ssl3_send_alert(s,SSL3_AL_FATAL,SSL_AD_HANDSHAKE_FAILURE);
                               
SSLerr(SSL_F_SSL3_CLIENT_HELLO,SSL_R_SESSION_MAY_NOT_BE_CREATED);
                                goto err;
                                }
                        if (!ssl_get_new_session(s,0))
                                goto err;
                        }
                /* else use the pre-loaded session */

If |session| is NULL, we will return early without calling
CheckIfSessionAdded(ssl). This means the SessionAdded callback will not be
called. The messenger will not be notified until SSLClientSocketOpenSSL calls
its error_callback_ for some reason.

[mshelley, 2014/07/18 21:08:33]

The code you cited indicates to me that session cannot be null. 

However, if the session were null, that essentially would mean that it's not
possible to do a resumption handshake for the connection correct? If that's the
case, a simple solution may be to just add: 
 
     if (!session) {
       base::Closure callback = ssl_to_callback_map_[ssl].callback;
       ssl_to_callback_map_.erase[ssl];
       callback.Run(); 
       return;
     } 

This will allow any pending jobs to proceed immediately because there's no point
in them waiting for the leader to connect, given that they cannot do a
resumption handshake.     

Perhaps it would be safest just to add those lines? I don't see how the session
could be null, but it also seems strange that MarkSSLSessionAsGood would contain
the early return if it wasn't somehow possible.

       return;
 
     // Mark the session as good, allowing it to be used for future connections.
     SSL_SESSION_set_ex_data(
         session, GetSSLSessionExIndex(), reinterpret_cast<void*>(1));
+
+    CheckIfSessionAdded(ssl);
   }
 
   // Flush all entries from the cache.
   void Flush() {
     base::AutoLock lock(lock_);
     id_index_.clear();
     key_index_.clear();
     while (!ordering_.empty()) {
       SSL_SESSION* session = ordering_.front();
       ordering_.pop_front();
       SSL_SESSION_free(session);
     }
   }
 
  private:
+  // CallbackAndCompletionCounts are used to group callback a callback

[wtc, 2014/07/18 01:17:15]

group callback a callback => group a callback

[mshelley, 2014/07/18 21:08:33]

Done.

+  // that should be run when a certian sesssion is added to the session
+  // cache with an integer indicating the status of that session.
+  struct CallbackAndCompletionCount {
+    CallbackAndCompletionCount(base::Closure completion_callback,

[wtc, 2014/07/18 01:17:15]

I think this input parameter should be a const reference.

[mshelley, 2014/07/18 21:08:33]

Done.

+                               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. |count| == 2 means that the

[wtc, 2014/07/18 01:17:16]

The first sentence in this comment block is not accurate. It is missing "or has
not been marked as good".

Since the first sentence is stating the fact as the second sentence (in opposite
ways), you can also just delete the first sentence.

More imortant, this comment should state that |count| is incremented when the
session is added to the cache and when the session is marked as good. This will
explain why we're testing |count| == 2.

[mshelley, 2014/07/18 21:08:33]

Done.

+    // session has been added to the cache and 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.
   std::string SessionKey(SSL_SESSION* session) {
     if (!session)
       return std::string("<null-session>");
 
     if (session->session_id_length == 0)
       return std::string("<empty-session-id>");
 
@@ skipping 57 matching lines @@
     DCHECK(result);
     return reinterpret_cast<SSLSessionCacheOpenSSLImpl*>(result);
   }
 
   // Called by OpenSSL when a new |session| was created and added to a given
   // |ssl| connection. Note that the session's reference count was already
   // incremented before the function is entered. The function must return 1
   // to indicate that it took ownership of the session, i.e. that the caller
   // should not decrement its reference count after completion.
   static int NewSessionCallbackStatic(SSL* ssl, SSL_SESSION* session) {
-    GetCache(ssl->ctx)->OnSessionAdded(ssl, session);
+    SSLSessionCacheOpenSSLImpl* cache = GetCache(ssl->ctx);
+    cache->OnSessionAdded(ssl, session);
+    cache->CheckIfSessionAdded(ssl);

[wtc, 2014/07/18 01:17:15]

IMPORTANT: a server may indicate that a session should not be cached by
returning an empty session_id in the Server Hello handshake message. See this
excerpt from the TLS 1.2 RFC:

   session_id
      This is the identity of the session corresponding to this
      connection.  ...
      ......................................................  The server
      may return an empty session_id to indicate that the session will
      not be cached and therefore cannot be resumed.  ...

In this case, OpenSSL won't call NewSessionCallbackStatic. See the
ssl_update_cache function in third_party/openssl/openssl/ssl/ssl_lib.c:

        /* If the session_id_length is 0, we are not supposed to cache it,
         * and it would be rather hard to do anyway :-) */
        if (s->session->session_id_length == 0) return;

and so our completion count for |ssl| won't become 2.

So I think we'll need to set an OpenSSL callback to receive handshake completion
notification. The only thing I can find is SSL_CTX_set_info_callback, which will
be called at handshake completion with the following arguments (see
third_party/openssl/openssl/ssl/s3_clnt.c):

                        if (cb != NULL) cb(s,SSL_CB_HANDSHAKE_DONE,1);

[mshelley, 2014/07/18 21:08:33]

Hmm yeah I see, this is definitely an issue. I'm adding a TODO comment for now;
I'll start looking into it.

     return 1;
   }
 
   // Called by OpenSSL to indicate that a session must be removed from the
   // cache. This happens when SSL_CTX is destroyed.
   static void RemoveSessionCallbackStatic(SSL_CTX* ctx, SSL_SESSION* session) {
     GetCache(ctx)->OnSessionRemoved(session);
   }
 
   // Called by OpenSSL to generate a new session ID. This happens during a
@@ skipping 103 matching lines @@
       if (id_index_.find(SessionId(id, id_len)) == id_index_.end())
         return true;
     }
     DLOG(ERROR) << "Couldn't generate unique session ID of " << id_len
                 << "bytes after " << kMaxTries << " tries.";
     return false;
   }
 
   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().
-  base::Lock lock_;  // Protects access to containers below.
+  mutable base::Lock lock_;  // Protects access to containers below.
 
   MRUSessionList ordering_;
   KeyIndex key_index_;
   SessionIdIndex id_index_;
 
   size_t expiration_check_;
 };
 
 SSLSessionCacheOpenSSL::~SSLSessionCacheOpenSSL() { delete impl_; }
 
 size_t SSLSessionCacheOpenSSL::size() const { return impl_->size(); }
 
 void SSLSessionCacheOpenSSL::Reset(SSL_CTX* ctx, const Config& config) {
   if (impl_)
     delete impl_;
 
   impl_ = new SSLSessionCacheOpenSSLImpl(ctx, config);
 }
 
 bool SSLSessionCacheOpenSSL::SetSSLSession(SSL* ssl) {
   return impl_->SetSSLSession(ssl);
 }
 
 bool SSLSessionCacheOpenSSL::SetSSLSessionWithKey(
     SSL* ssl,
     const std::string& cache_key) {
   return impl_->SetSSLSessionWithKey(ssl, cache_key);
 }
 
+bool SSLSessionCacheOpenSSL::SSLSessionIsInCache(
+    const std::string& cache_key) const {
+  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);
 }
 
 void SSLSessionCacheOpenSSL::Flush() { impl_->Flush(); }
 
 }  // namespace net