Enforce single-use tickets and remove session counting histogram.

Bug: 631988
Change-Id: I16e2aaa227e36c3d025c6663b10f6a15e4a32428
Reviewed-on: https://chromium-review.googlesource.com/692946Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Commit-Queue: Steven Valdez <svaldez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#506882}

net/socket/ssl_client_socket_impl.cc

@@ -446,7 +446,6 @@ SSLClientSocketImpl::SSLClientSocketImpl(
       host_and_port_(host_and_port),
       ssl_config_(ssl_config),
       ssl_session_cache_shard_(context.ssl_session_cache_shard),
-      ssl_session_cache_lookup_count_(0),
       next_handshake_state_(STATE_NONE),
       disconnected_(false),
       negotiated_protocol_(kProtoUnknown),
@@ -864,8 +863,8 @@ int SSLClientSocketImpl::Init() {
   }
 
   if (!ssl_session_cache_shard_.empty()) {
-    bssl::UniquePtr<SSL_SESSION> session = context->session_cache()->Lookup(
-        GetSessionCacheKey(), &ssl_session_cache_lookup_count_);
+    bssl::UniquePtr<SSL_SESSION> session =
+        context->session_cache()->Lookup(GetSessionCacheKey());
     if (session)
       SSL_set_session(ssl_.get(), session.get());
   }
@@ -1124,17 +1123,6 @@ int SSLClientSocketImpl::DoHandshakeComplete(int result) {
     negotiated_protocol_ = NextProtoFromString(proto);
   }
 
-  // If we got a session from the session cache, log how many concurrent
-  // handshakes that session was used in before we finished our handshake. This
-  // is only recorded if the session from the cache was actually used, and only
-  // if the ALPN protocol is h2 (under the assumption that TLS 1.3 servers will
-  // be speaking h2). See https://crbug.com/631988.
-  if (ssl_session_cache_lookup_count_ && negotiated_protocol_ == kProtoHTTP2 &&
-      SSL_session_reused(ssl_.get())) {
-    UMA_HISTOGRAM_EXACT_LINEAR("Net.SSLSessionConcurrentLookupCount",
-                               ssl_session_cache_lookup_count_, 20);
-  }
-
   RecordNegotiatedProtocol();
   RecordChannelIDSupport();
 

net/socket/ssl_client_socket_impl.h

@@ -305,7 +305,6 @@ class SSLClientSocketImpl : public SSLClientSocket,
   // session cache. i.e. sessions created with one value will not attempt to
   // resume on the socket with a different value.
   const std::string ssl_session_cache_shard_;
-  int ssl_session_cache_lookup_count_;
 
   enum State {
     STATE_NONE,

net/ssl/ssl_client_session_cache.cc

@@ -36,8 +36,7 @@ size_t SSLClientSessionCache::size() const {
 }
 
 bssl::UniquePtr<SSL_SESSION> SSLClientSessionCache::Lookup(
-    const std::string& cache_key,
-    int* count) {
+    const std::string& cache_key) {
   base::AutoLock lock(lock_);
 
   // Expire stale sessions.
@@ -47,27 +46,18 @@ bssl::UniquePtr<SSL_SESSION> SSLClientSessionCache::Lookup(
     FlushExpiredSessions();
   }
 
-  // Set count to 0 if there's no session in the cache.
-  if (count != nullptr)
-    *count = 0;
-
   auto iter = cache_.Get(cache_key);
   if (iter == cache_.end())
     return nullptr;
 
-  SSL_SESSION* session = iter->second.session.get();
-  if (IsExpired(session, clock_->Now().ToTimeT())) {
+  time_t now = clock_->Now().ToTimeT();
+  bssl::UniquePtr<SSL_SESSION> session = iter->second.Pop();
+  if (iter->second.ExpireSessions(now))
     cache_.Erase(iter);
-    return nullptr;
-  }
 
-  iter->second.lookups++;
-  if (count != nullptr) {
-    *count = iter->second.lookups;
-  }
-
-  SSL_SESSION_up_ref(session);
-  return bssl::UniquePtr<SSL_SESSION>(session);
+  if (IsExpired(session.get(), now))
+    session = nullptr;
+  return session;
 }
 
 void SSLClientSessionCache::ResetLookupCount(const std::string& cache_key) {
@@ -78,8 +68,6 @@ void SSLClientSessionCache::ResetLookupCount(const std::string& cache_key) {
   auto iter = cache_.Get(cache_key);
   if (iter == cache_.end())
     return;
-
-  iter->second.lookups = 0;
 }
 
 void SSLClientSessionCache::Insert(const std::string& cache_key,
@@ -87,9 +75,10 @@ void SSLClientSessionCache::Insert(const std::string& cache_key,
   base::AutoLock lock(lock_);
 
   SSL_SESSION_up_ref(session);
-  Entry entry;
-  entry.session = bssl::UniquePtr<SSL_SESSION>(session);
-  cache_.Put(cache_key, std::move(entry));
+  auto iter = cache_.Get(cache_key);
+  if (iter == cache_.end())
+    iter = cache_.Put(cache_key, Entry());
+  iter->second.Push(bssl::UniquePtr<SSL_SESSION>(session));
 }
 
 void SSLClientSessionCache::Flush() {
@@ -129,23 +118,27 @@ void SSLClientSessionCache::DumpMemoryStats(
   size_t undeduped_cert_size = 0;
   size_t undeduped_cert_count = 0;
   for (const auto& pair : cache_) {
-    undeduped_cert_count +=
-        sk_CRYPTO_BUFFER_num(pair.second.session.get()->certs);
+    for (const auto& session : pair.second.sessions) {
+      if (!session)
+        continue;
+      undeduped_cert_count += sk_CRYPTO_BUFFER_num(session->certs);
+    }
   }
   // Use a flat_set here to avoid malloc upon insertion.
   base::flat_set<const CRYPTO_BUFFER*> crypto_buffer_set;
   crypto_buffer_set.reserve(undeduped_cert_count);
   for (const auto& pair : cache_) {
-    const SSL_SESSION* session = pair.second.session.get();
-    size_t pair_cert_count = sk_CRYPTO_BUFFER_num(session->certs);
-    for (size_t i = 0; i < pair_cert_count; ++i) {
-      const CRYPTO_BUFFER* cert = sk_CRYPTO_BUFFER_value(session->certs, i);
-      undeduped_cert_size += CRYPTO_BUFFER_len(cert);
-      auto result = crypto_buffer_set.insert(cert);
-      if (!result.second)
+    for (const auto& session : pair.second.sessions) {
+      if (!session)
         continue;
-      cert_size += CRYPTO_BUFFER_len(cert);
-      cert_count++;
+      for (const CRYPTO_BUFFER* cert : session->certs) {
+        undeduped_cert_size += CRYPTO_BUFFER_len(cert);
+        auto result = crypto_buffer_set.insert(cert);
+        if (!result.second)
+          continue;
+        cert_size += CRYPTO_BUFFER_len(cert);
+        cert_count++;
+      }
     }
   }
   cache_dump->AddScalar(base::trace_event::MemoryAllocatorDump::kNameSize,
@@ -165,15 +158,51 @@ void SSLClientSessionCache::DumpMemoryStats(
                         undeduped_cert_count);
 }
 
-SSLClientSessionCache::Entry::Entry() : lookups(0) {}
+SSLClientSessionCache::Entry::Entry() {}
 SSLClientSessionCache::Entry::Entry(Entry&&) = default;
 SSLClientSessionCache::Entry::~Entry() = default;
 
+void SSLClientSessionCache::Entry::Push(bssl::UniquePtr<SSL_SESSION> session) {
+  if (sessions[0] != nullptr &&
+      SSL_SESSION_should_be_single_use(sessions[0].get())) {
+    sessions[1] = std::move(sessions[0]);
+  }
+  sessions[0] = std::move(session);
+}
+
+bssl::UniquePtr<SSL_SESSION> SSLClientSessionCache::Entry::Pop() {
+  if (sessions[0] == nullptr)
+    return nullptr;
+  SSL_SESSION* session = sessions[0].get();
+  SSL_SESSION_up_ref(session);
+  if (SSL_SESSION_should_be_single_use(session)) {
+    sessions[0] = std::move(sessions[1]);
+    sessions[1] = nullptr;
+  }
+  return bssl::UniquePtr<SSL_SESSION>(session);
+}
+
+bool SSLClientSessionCache::Entry::ExpireSessions(time_t now) {
+  if (sessions[0] == nullptr)
+    return true;
+
+  if (SSLClientSessionCache::IsExpired(sessions[0].get(), now)) {
+    return true;
+  }
+
+  if (sessions[1] != nullptr &&
+      SSLClientSessionCache::IsExpired(sessions[1].get(), now)) {
+    sessions[1] = nullptr;
+  }
+
+  return false;
+}
+
 void SSLClientSessionCache::FlushExpiredSessions() {
   time_t now = clock_->Now().ToTimeT();
   auto iter = cache_.begin();
   while (iter != cache_.end()) {
-    if (IsExpired(iter->second.session.get(), now)) {
+    if (iter->second.ExpireSessions(now)) {
       iter = cache_.Erase(iter);
     } else {
       ++iter;

net/ssl/ssl_client_session_cache.h

@@ -43,13 +43,14 @@ class NET_EXPORT SSLClientSessionCache : public base::MemoryCoordinatorClient {
   explicit SSLClientSessionCache(const Config& config);
   ~SSLClientSessionCache() override;
 
+  // Returns true if |entry| is expired as of |now|.
+  static bool IsExpired(SSL_SESSION* session, time_t now);
+
   size_t size() const;
 
   // Returns the session associated with |cache_key| and moves it to the front
-  // of the MRU list. Returns nullptr if there is none. If |count| is non-null,
-  // |*count| will contain the number of times this session has been looked up
-  // (including this call).
-  bssl::UniquePtr<SSL_SESSION> Lookup(const std::string& cache_key, int* count);
+  // of the MRU list. Returns nullptr if there is none.
+  bssl::UniquePtr<SSL_SESSION> Lookup(const std::string& cache_key);
 
   // Resets the count returned by Lookup to 0 for the session associated with
   // |cache_key|.
@@ -75,16 +76,24 @@ class NET_EXPORT SSLClientSessionCache : public base::MemoryCoordinatorClient {
     Entry(Entry&&);
     ~Entry();
 
-    int lookups;
-    bssl::UniquePtr<SSL_SESSION> session;
+    // Adds a new session onto this entry, dropping the oldest one if two are
+    // already stored.
+    void Push(bssl::UniquePtr<SSL_SESSION> session);
+
+    // Retrieves the latest session from the entry, removing it if its
+    // single-use.
+    bssl::UniquePtr<SSL_SESSION> Pop();
+
+    // Removes any expired sessions, returning true if this entry can be
+    // deleted.
+    bool ExpireSessions(time_t now);
+
+    bssl::UniquePtr<SSL_SESSION> sessions[2] = {nullptr};
   };
 
   // base::MemoryCoordinatorClient implementation:
   void OnPurgeMemory() override;
 
-  // Returns true if |entry| is expired as of |now|.
-  bool IsExpired(SSL_SESSION* session, time_t now);
-
   // Removes all expired sessions from the cache.
   void FlushExpiredSessions();
 

tools/metrics/histograms/histograms.xml

@@ -44793,6 +44793,9 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
 </histogram>
 
 <histogram name="Net.SSLSessionConcurrentLookupCount">
+  <obsolete>
+    Removed on 2017-10-02.
+  </obsolete>
   <owner>nharper@chromium.org</owner>
   <summary>
     For each SSL connection where we resume a session and negotiate HTTP/2, the