Implements new, more robust design for communicating between SSLConnectJobs.

net/socket/ssl_client_socket_openssl.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.
 
 // OpenSSL binding for SSLClientSocket. The class layout and general principle
 // of operation is derived from SSLClientSocketNSS.
 
 #include "net/socket/ssl_client_socket_openssl.h"
 
 #include <openssl/err.h>
 #include <openssl/opensslv.h>
 #include <openssl/ssl.h>
 
 #include "base/bind.h"
 #include "base/callback_helpers.h"
 #include "base/memory/singleton.h"
 #include "base/metrics/histogram.h"
 #include "base/synchronization/lock.h"
 #include "crypto/ec_private_key.h"
 #include "crypto/openssl_util.h"
 #include "net/base/net_errors.h"
 #include "net/cert/cert_verifier.h"
 #include "net/cert/single_request_cert_verifier.h"
 #include "net/cert/x509_certificate_net_log_param.h"
 #include "net/socket/openssl_ssl_util.h"
+#include "net/socket/ssl_client_socket_pool.h"
 #include "net/socket/ssl_error_params.h"
 #include "net/socket/ssl_session_cache_openssl.h"
 #include "net/ssl/openssl_client_key_store.h"
 #include "net/ssl/ssl_cert_request_info.h"
 #include "net/ssl/ssl_connection_status_flags.h"
 #include "net/ssl/ssl_info.h"
 
 namespace net {
 
 namespace {
@@ skipping 44 matching lines @@
       return SSL_CONNECTION_VERSION_TLS1;
     case 0x0302:
       return SSL_CONNECTION_VERSION_TLS1_1;
     case 0x0303:
       return SSL_CONNECTION_VERSION_TLS1_2;
     default:
       return SSL_CONNECTION_VERSION_UNKNOWN;
   }
 }
 
-// Compute a unique key string for the SSL session cache. |socket| is an
-// input socket object. Return a string.
-std::string GetSocketSessionCacheKey(const SSLClientSocketOpenSSL& socket) {
-  std::string result = socket.host_and_port().ToString();
-  result.append("/");
-  result.append(socket.ssl_session_cache_shard());
-  return result;
-}
 
 }  // namespace
 
 class SSLClientSocketOpenSSL::SSLContext {
  public:
   static SSLContext* GetInstance() { return Singleton<SSLContext>::get(); }
   SSL_CTX* ssl_ctx() { return ssl_ctx_.get(); }
   SSLSessionCacheOpenSSL* session_cache() { return &session_cache_; }
 
   SSLClientSocketOpenSSL* GetClientSocketFromSSL(const SSL* ssl) {
@@ skipping 24 matching lines @@
     // TODO(kristianm): Only select this if ssl_config_.next_proto is not empty.
     // It would be better if the callback were not a global setting,
     // but that is an OpenSSL issue.
     SSL_CTX_set_next_proto_select_cb(ssl_ctx_.get(), SelectNextProtoCallback,
                                      NULL);
   }
 
   static std::string GetSessionCacheKey(const SSL* ssl) {
     SSLClientSocketOpenSSL* socket = GetInstance()->GetClientSocketFromSSL(ssl);
     DCHECK(socket);
-    return GetSocketSessionCacheKey(*socket);
+    return socket->GetSessionCacheKey();
   }
 
   static SSLSessionCacheOpenSSL::Config kDefaultSessionCacheConfig;
 
   static int ClientCertCallback(SSL* ssl, X509** x509, EVP_PKEY** pkey) {
     SSLClientSocketOpenSSL* socket = GetInstance()->GetClientSocketFromSSL(ssl);
     CHECK(socket);
     return socket->ClientCertRequestCallback(ssl, x509, pkey);
   }
 
@@ skipping 180 matching lines @@
 }
 
 SSLClientSocketOpenSSL::SSLClientSocketOpenSSL(
     scoped_ptr<ClientSocketHandle> transport_socket,
     const HostPortPair& host_and_port,
     const SSLConfig& ssl_config,
     const SSLClientSocketContext& context)
     : transport_send_busy_(false),
       transport_recv_busy_(false),
       transport_recv_eof_(false),
+      has_read_(false),
+      has_written_(false),

[wtc, 2014/07/11 00:48:54]

Initialize has_written_ to 0 because it is of the int type now.

[mshelley, 2014/07/11 23:26:26]

Done.

       weak_factory_(this),
       pending_read_error_(kNoPendingReadResult),
       transport_write_error_(OK),
       server_cert_chain_(new PeerCertificateChain(NULL)),
       completed_handshake_(false),
       was_ever_used_(false),
       client_auth_cert_needed_(false),
       cert_verifier_(context.cert_verifier),
       server_bound_cert_service_(context.server_bound_cert_service),
+      is_leader_(false),
       ssl_(NULL),
       transport_bio_(NULL),
       transport_(transport_socket.Pass()),
       host_and_port_(host_and_port),
       ssl_config_(ssl_config),
       ssl_session_cache_shard_(context.ssl_session_cache_shard),
       trying_cached_session_(false),
       next_handshake_state_(STATE_NONE),
       npn_status_(kNextProtoUnsupported),
       channel_id_request_return_value_(ERR_UNEXPECTED),
       channel_id_xtn_negotiated_(false),
-      net_log_(transport_->socket()->NetLog()) {}
+      net_log_(transport_->socket()->NetLog()) {
+}
 
 SSLClientSocketOpenSSL::~SSLClientSocketOpenSSL() {
+  SSLContext* context = SSLContext::GetInstance();
+  context->session_cache()->RemoveFromSSLToCallbackMap(ssl_);

[wtc, 2014/07/11 00:48:54]

It may be better to do this in the Disconnect() method instead.

[mshelley, 2014/07/11 23:26:26]

Done.

   Disconnect();
 }
 
+// Compute a unique key string for the SSL session cache.
+// Return a string.

[wtc, 2014/07/11 00:48:54]

Move this comment to the .h file.

Delete "Return a string." It seems to be stating the obvious.

[mshelley, 2014/07/11 23:26:26]

Done.

+std::string SSLClientSocketOpenSSL::GetSessionCacheKey() const {
+  return FormatSessionCacheKey(host_and_port_.ToString(),
+                               ssl_session_cache_shard_);
+}
+
+bool SSLClientSocketOpenSSL::InSessionCache() const {
+  SSLContext* context = SSLContext::GetInstance();
+  std::string cache_key = GetSessionCacheKey();
+  return context->session_cache()->SSLSessionIsInCache(cache_key);
+}
+
+void SSLClientSocketOpenSSL::WatchSessionForCompletion(
+    const base::Closure& callback) {
+  SSLContext* context = SSLContext::GetInstance();
+  context->session_cache()->RegisterSessionAddedCallback(ssl_, callback);
+}
+
+void SSLClientSocketOpenSSL::SetSocketFailureCallback(
+    const base::Closure& callback) {
+  error_callback_ = callback;
+}
+
+void SSLClientSocketOpenSSL::SetIsLeader() {
+  is_leader_ = true;
+}
+
+void SSLClientSocketOpenSSL::OnSocketFailure() {
+  if (is_leader_) {
+    error_callback_.Run();
+    error_callback_ = base::Closure();
+    is_leader_ = false;
+  }

[wtc, 2014/07/11 00:48:54]

Change this to:

  if (!error_callback_.is_null())
    base::ResetAndReturn(&error_callback_).Run();

[mshelley, 2014/07/11 23:26:26]

Done.

+}
+
 void SSLClientSocketOpenSSL::GetSSLCertRequestInfo(
     SSLCertRequestInfo* cert_request_info) {
   cert_request_info->host_and_port = host_and_port_;
   cert_request_info->cert_authorities = cert_authorities_;
   cert_request_info->cert_key_types = cert_key_types_;
 }
 
 SSLClientSocket::NextProtoStatus SSLClientSocketOpenSSL::GetNextProto(
     std::string* proto, std::string* server_protos) {
   *proto = npn_proto_;
@@ skipping 272 matching lines @@
   crypto::OpenSSLErrStackTracer err_tracer(FROM_HERE);
 
   ssl_ = SSL_new(context->ssl_ctx());
   if (!ssl_ || !context->SetClientSocketForSSL(ssl_, this))
     return ERR_UNEXPECTED;
 
   if (!SSL_set_tlsext_host_name(ssl_, host_and_port_.host().c_str()))
     return ERR_UNEXPECTED;
 
   trying_cached_session_ = context->session_cache()->SetSSLSessionWithKey(
-      ssl_, GetSocketSessionCacheKey(*this));
+      ssl_, GetSessionCacheKey());
 
   BIO* ssl_bio = NULL;
   // 0 => use default buffer sizes.
   if (!BIO_new_bio_pair(&ssl_bio, 0, &transport_bio_, 0))
     return ERR_UNEXPECTED;
   DCHECK(ssl_bio);
   DCHECK(transport_bio_);
 
   SSL_set_bio(ssl_, ssl_bio, ssl_bio);
 
@@ skipping 114 matching lines @@
       network_moved = true;
   } while (rv > 0);
   if (!transport_recv_eof_ && BufferRecv() != ERR_IO_PENDING)
     network_moved = true;
   return network_moved;
 }
 
 int SSLClientSocketOpenSSL::DoHandshake() {
   crypto::OpenSSLErrStackTracer err_tracer(FROM_HERE);
   int net_error = OK;
   int rv = SSL_do_handshake(ssl_);

[wtc, 2014/07/11 00:48:54]

IMPORTANT: if rv <= 0, we should also call OnSocketFailure().

[mshelley, 2014/07/11 23:26:26]

Done.

 
   if (client_auth_cert_needed_) {
     net_error = ERR_SSL_CLIENT_AUTH_CERT_NEEDED;
     // If the handshake already succeeded (because the server requests but
     // doesn't require a client cert), we need to invalidate the SSL session
     // so that we won't try to resume the non-client-authenticated session in
     // the next handshake.  This will cause the server to ask for a client
     // cert again.
     if (rv == 1) {
       // Remove from session cache but don't clear this connection.
@@ skipping 250 matching lines @@
       net_log_.AddByteTransferEvent(NetLog::TYPE_SSL_SOCKET_BYTES_RECEIVED,
                                     rv, user_read_buf_->data());
     }
     return rv;
   }
 
   int total_bytes_read = 0;
   do {
     rv = SSL_read(ssl_, user_read_buf_->data() + total_bytes_read,
                   user_read_buf_len_ - total_bytes_read);
+    // Failure of the first read attempt indicates a failed false start

[wtc, 2014/07/11 00:48:54]

IMPORTANT: understanding this code requires intimate knowledge of False Start
and OpenSSL. We should try to avoid this.

One possible solution is to call OnSocketFailure on any error of
SSL_do_handshake, SSL_read, or SSL_write before we get the "SessionAdded"
notification from the SSL session cache. In other words, any error before the
true completion of the handshake is a handshake failure. We can register a
"SessionAdded" callback that reset error_callback_ and call the Messenger's
OnJobSucceeded method.

Also, we may abort the handshake, too. So we should also call OnSocketFailure in
the Disconnect() method.

[mshelley, 2014/07/11 23:26:26]

Done.

+    // connection.
+    if (!has_read_ && rv != OK &&

[wtc, 2014/07/11 00:48:54]

IMPORTANT: SSL_read is an OpenSSL function, so its return value should not be
compared with OK (a Chromium network stack return code).

Test rv <= 0 instead.

Note: if rv == 0, it means the peer closes the SSL connection either gracefully
or abruptly. It is also a sign of handshake failure.

[mshelley, 2014/07/11 23:26:27]

Done.

+        SSLClientSocketPool::GetEnableConnectJobWaiting())

[wtc, 2014/07/11 00:48:54]

Delete the SSLClientSocketPool::GetEnableConnectJobWaiting() test. The
!error_callback_.is_null() test inside OnSocketFailure() is sufficient.

Also delete the test on line 1163.

[mshelley, 2014/07/11 23:26:26]

Done.

+      OnSocketFailure();
+    has_read_ = true;
     if (rv > 0)
       total_bytes_read += rv;
   } while (total_bytes_read < user_read_buf_len_ && rv > 0);
 
   if (total_bytes_read == user_read_buf_len_) {
     rv = total_bytes_read;
   } else {
     // Otherwise, an error occurred (rv <= 0). The error needs to be handled
     // immediately, while the OpenSSL errors are still available in
     // thread-local storage. However, the handled/remapped error code should
@@ skipping 32 matching lines @@
   if (rv >= 0) {
     net_log_.AddByteTransferEvent(NetLog::TYPE_SSL_SOCKET_BYTES_RECEIVED, rv,
                                   user_read_buf_->data());
   }
   return rv;
 }
 
 int SSLClientSocketOpenSSL::DoPayloadWrite() {
   crypto::OpenSSLErrStackTracer err_tracer(FROM_HERE);
   int rv = SSL_write(ssl_, user_write_buf_->data(), user_write_buf_len_);
-
+  // Failure of the second write attempt indicates a failed false start
+  // connection.
+  if (has_written_ == 1 && rv != OK &&

[wtc, 2014/07/11 00:48:54]

1. Change has_written_ to has_written_ <= 1.

2. Change rv != OK to rv < 0.

[mshelley, 2014/07/11 23:26:27]

Done.

+      SSLClientSocketPool::GetEnableConnectJobWaiting()) {
+    OnSocketFailure();
+  }
+  has_written_++;

[wtc, 2014/07/11 00:48:54]

Nit: in theory this may overflow.

[mshelley, 2014/07/11 23:26:26]

Done.

   if (rv >= 0) {
     net_log_.AddByteTransferEvent(NetLog::TYPE_SSL_SOCKET_BYTES_SENT, rv,
                                   user_write_buf_->data());
     return rv;
   }
 
   int err = SSL_get_error(ssl_, rv);
   return MapOpenSSLError(err, err_tracer);
 }
 
@@ skipping 300 matching lines @@
   DVLOG(2) << "next protocol: '" << npn_proto_ << "' status: " << npn_status_;
   return SSL_TLSEXT_ERR_OK;
 }
 
 scoped_refptr<X509Certificate>
 SSLClientSocketOpenSSL::GetUnverifiedServerCertificateChain() const {
   return server_cert_;
 }
 
 }  // namespace net