Unwind the SSL connection holdback experiment and remove related code

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 <errno.h>
@@ skipping 201 matching lines @@
         ERR_print_errors_cb(&LogErrorCallback, NULL);
       } else {
         SSL_CTX_set_keylog_bio(ssl_ctx_.get(), bio);
       }
     }
   }
 
   static std::string GetSessionCacheKey(const SSL* ssl) {
     SSLClientSocketOpenSSL* socket = GetInstance()->GetClientSocketFromSSL(ssl);
     DCHECK(socket);
-    return socket->GetSessionCacheKey();
+    return GetSocketSessionCacheKey(*socket);

[davidben, 2015/03/09 18:02:36]

I think you forgot to restore this function.

   }
 
   static SSLSessionCacheOpenSSL::Config kDefaultSessionCacheConfig;
 
   static int ClientCertRequestCallback(SSL* ssl, void* arg) {
     SSLClientSocketOpenSSL* socket = GetInstance()->GetClientSocketFromSSL(ssl);
     DCHECK(socket);
     return socket->ClientCertRequestCallback(ssl);
   }
 
@@ skipping 130 matching lines @@
     const HostPortPair& host_and_port,
     const SSLConfig& ssl_config,
     const SSLClientSocketContext& context)
     : transport_send_busy_(false),
       transport_recv_busy_(false),
       pending_read_error_(kNoPendingReadResult),
       pending_read_ssl_error_(SSL_ERROR_NONE),
       transport_read_error_(OK),
       transport_write_error_(OK),
       server_cert_chain_(new PeerCertificateChain(NULL)),
-      completed_connect_(false),
+      completed_handshake_(false),
       was_ever_used_(false),
       client_auth_cert_needed_(false),
       cert_verifier_(context.cert_verifier),
       cert_transparency_verifier_(context.cert_transparency_verifier),
       channel_id_service_(context.channel_id_service),
       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_xtn_negotiated_(false),
-      handshake_succeeded_(false),
-      marked_session_as_good_(false),
       transport_security_state_(context.transport_security_state),
       policy_enforcer_(context.cert_policy_enforcer),
       net_log_(transport_->socket()->NetLog()),
       weak_factory_(this) {
 }
 
 SSLClientSocketOpenSSL::~SSLClientSocketOpenSSL() {
   Disconnect();
 }
 
-std::string SSLClientSocketOpenSSL::GetSessionCacheKey() const {
-  std::string result = host_and_port_.ToString();
-  result.append("/");
-  result.append(ssl_session_cache_shard_);
-
-  // Shard the session cache based on maximum protocol version. This causes
-  // fallback connections to use a separate session cache.

[davidben, 2015/03/09 18:02:36]

GetSocketSessionCacheKey got lost but, when you bring it back (or perhaps leave
it as a private method?), this needs to stay there.

-  result.append("/");
-  switch (ssl_config_.version_max) {
-    case SSL_PROTOCOL_VERSION_SSL3:
-      result.append("ssl3");
-      break;
-    case SSL_PROTOCOL_VERSION_TLS1:
-      result.append("tls1");
-      break;
-    case SSL_PROTOCOL_VERSION_TLS1_1:
-      result.append("tls1.1");
-      break;
-    case SSL_PROTOCOL_VERSION_TLS1_2:
-      result.append("tls1.2");
-      break;
-    default:
-      NOTREACHED();
-  }
-
-  return result;
-}
-
-bool SSLClientSocketOpenSSL::InSessionCache() const {
-  SSLContext* context = SSLContext::GetInstance();
-  std::string cache_key = GetSessionCacheKey();
-  return context->session_cache()->SSLSessionIsInCache(cache_key);
-}
-
-void SSLClientSocketOpenSSL::SetHandshakeCompletionCallback(
-    const base::Closure& callback) {
-  handshake_completion_callback_ = callback;
-}
-
 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) {
   *proto = npn_proto_;
@@ skipping 52 matching lines @@
   SSL_enable_fastradio_padding(ssl_,
                                ssl_config_.fastradio_padding_enabled &&
                                    ssl_config_.fastradio_padding_eligible);
 
   GotoState(STATE_HANDSHAKE);
   rv = DoHandshakeLoop(OK);
   if (rv == ERR_IO_PENDING) {
     user_connect_callback_ = callback;
   } else {
     net_log_.EndEventWithNetErrorCode(NetLog::TYPE_SSL_CONNECT, rv);
-    if (rv < OK)
-      OnHandshakeCompletion();
   }
 
   return rv > OK ? OK : rv;
 }
 
 void SSLClientSocketOpenSSL::Disconnect() {
-  // If a handshake was pending (Connect() had been called), notify interested
-  // parties that it's been aborted now. If the handshake had already
-  // completed, this is a no-op.
-  OnHandshakeCompletion();
   if (ssl_) {
     // Calling SSL_shutdown prevents the session from being marked as
     // unresumable.
     SSL_shutdown(ssl_);
     SSL_free(ssl_);
     ssl_ = NULL;
   }
   if (transport_bio_) {
     BIO_free_all(transport_bio_);
     transport_bio_ = NULL;
@@ skipping 18 matching lines @@
   user_write_buf_len_    = 0;
 
   pending_read_error_ = kNoPendingReadResult;
   pending_read_ssl_error_ = SSL_ERROR_NONE;
   pending_read_error_info_ = OpenSSLErrorInfo();
 
   transport_read_error_ = OK;
   transport_write_error_ = OK;
 
   server_cert_verify_result_.Reset();
-  completed_connect_ = false;
+  completed_handshake_ = false;
 
   cert_authorities_.clear();
   cert_key_types_.clear();
   client_auth_cert_needed_ = false;
 
   start_cert_verification_time_ = base::TimeTicks();
 
   npn_status_ = kNextProtoUnsupported;
   npn_proto_.clear();
 
   channel_id_xtn_negotiated_ = false;
   channel_id_request_handle_.Cancel();
 }
 
 bool SSLClientSocketOpenSSL::IsConnected() const {
   // If the handshake has not yet completed.
-  if (!completed_connect_)
+  if (!completed_handshake_)
     return false;
   // If an asynchronous operation is still pending.
   if (user_read_buf_.get() || user_write_buf_.get())
     return true;
 
   return transport_->socket()->IsConnected();
 }
 
 bool SSLClientSocketOpenSSL::IsConnectedAndIdle() const {
   // If the handshake has not yet completed.
-  if (!completed_connect_)
+  if (!completed_handshake_)
     return false;
   // If an asynchronous operation is still pending.
   if (user_read_buf_.get() || user_write_buf_.get())
     return false;
   // If there is data waiting to be sent, or data read from the network that
   // has not yet been consumed.
   if (BIO_pending(transport_bio_) > 0 ||
       BIO_wpending(transport_bio_) > 0) {
     return false;
   }
@@ skipping 91 matching lines @@
 
   int rv = DoReadLoop();
 
   if (rv == ERR_IO_PENDING) {
     user_read_callback_ = callback;
   } else {
     if (rv > 0)
       was_ever_used_ = true;
     user_read_buf_ = NULL;
     user_read_buf_len_ = 0;
-    if (rv <= 0) {
-      // Failure of a read attempt may indicate a failed false start
-      // connection.
-      OnHandshakeCompletion();
-    }
   }
 
   return rv;
 }
 
 int SSLClientSocketOpenSSL::Write(IOBuffer* buf,
                                   int buf_len,
                                   const CompletionCallback& callback) {
   user_write_buf_ = buf;
   user_write_buf_len_ = buf_len;
 
   int rv = DoWriteLoop();
 
   if (rv == ERR_IO_PENDING) {
     user_write_callback_ = callback;
   } else {
     if (rv > 0)
       was_ever_used_ = true;
     user_write_buf_ = NULL;
     user_write_buf_len_ = 0;
-    if (rv < 0) {
-      // Failure of a write attempt may indicate a failed false start
-      // connection.
-      OnHandshakeCompletion();
-    }
   }
 
   return rv;
 }
 
 int SSLClientSocketOpenSSL::SetReceiveBufferSize(int32 size) {
   return transport_->socket()->SetReceiveBufferSize(size);
 }
 
 int SSLClientSocketOpenSSL::SetSendBufferSize(int32 size) {
   return transport_->socket()->SetSendBufferSize(size);
 }
 
 int SSLClientSocketOpenSSL::Init() {
   DCHECK(!ssl_);
   DCHECK(!transport_bio_);
 
   SSLContext* context = SSLContext::GetInstance();
   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;
 
-  // 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());
+      ssl_, GetSocketSessionCacheKey(*this));
 
   send_buffer_ = new GrowableIOBuffer();
   send_buffer_->SetCapacity(KDefaultOpenSSLBufferSize);
   recv_buffer_ = new GrowableIOBuffer();
   recv_buffer_->SetCapacity(KDefaultOpenSSLBufferSize);
 
   BIO* ssl_bio = NULL;
 
   // SSLClientSocketOpenSSL retains ownership of the BIO buffers.
   if (!BIO_new_bio_pair_external_buf(
           &ssl_bio, send_buffer_->capacity(),
           reinterpret_cast<uint8_t*>(send_buffer_->data()), &transport_bio_,
           recv_buffer_->capacity(),
           reinterpret_cast<uint8_t*>(recv_buffer_->data())))
     return ERR_UNEXPECTED;
   DCHECK(ssl_bio);
   DCHECK(transport_bio_);
 
   // Install a callback on OpenSSL's end to plumb transport errors through.
-  BIO_set_callback(ssl_bio, BIOCallback);
+  BIO_set_callback(ssl_bio, &SSLClientSocketOpenSSL::BIOCallback);
   BIO_set_callback_arg(ssl_bio, reinterpret_cast<char*>(this));
 
   SSL_set_bio(ssl_, ssl_bio, ssl_bio);
 
   // OpenSSL defaults some options to on, others to off. To avoid ambiguity,
   // set everything we care about to an absolute value.
   SslSetClearMask options;
   options.ConfigureFlag(SSL_OP_NO_SSLv2, true);
   bool ssl3_enabled = (ssl_config_.version_min == SSL_PROTOCOL_VERSION_SSL3);
   options.ConfigureFlag(SSL_OP_NO_SSLv3, !ssl3_enabled);
@@ skipping 116 matching lines @@
   return OK;
 }
 
 void SSLClientSocketOpenSSL::DoReadCallback(int rv) {
   // Since Run may result in Read being called, clear |user_read_callback_|
   // up front.
   if (rv > 0)
     was_ever_used_ = true;
   user_read_buf_ = NULL;
   user_read_buf_len_ = 0;
-  if (rv <= 0) {
-    // Failure of a read attempt may indicate a failed false start
-    // connection.
-    OnHandshakeCompletion();
-  }
   base::ResetAndReturn(&user_read_callback_).Run(rv);
 }
 
 void SSLClientSocketOpenSSL::DoWriteCallback(int rv) {
   // Since Run may result in Write being called, clear |user_write_callback_|
   // up front.
   if (rv > 0)
     was_ever_used_ = true;
   user_write_buf_ = NULL;
   user_write_buf_len_ = 0;
-  if (rv < 0) {
-    // Failure of a write attempt may indicate a failed false start
-    // connection.
-    OnHandshakeCompletion();
-  }
   base::ResetAndReturn(&user_write_callback_).Run(rv);
 }
 
-void SSLClientSocketOpenSSL::OnHandshakeCompletion() {
-  if (!handshake_completion_callback_.is_null())
-    base::ResetAndReturn(&handshake_completion_callback_).Run();
-}
-
 bool SSLClientSocketOpenSSL::DoTransportIO() {
   bool network_moved = false;
   int rv;
   // Read and write as much data as possible. The loop is necessary because
   // Write() may return synchronously.
   do {
     rv = BufferSend();
     if (rv != ERR_IO_PENDING && rv != 0)
       network_moved = true;
   } while (rv > 0);
@@ skipping 294 matching lines @@
   }
 
   if (result == OK) {
     // Only check Certificate Transparency if there were no other errors with
     // the connection.
     VerifyCT();
 
     // 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 << ")";
   }
 
-  completed_connect_ = true;
-
+  completed_handshake_ = true;
   // Exit DoHandshakeLoop and return the result to the caller to Connect.
   DCHECK_EQ(STATE_NONE, next_handshake_state_);
   return result;
 }
 
 void SSLClientSocketOpenSSL::DoConnectCallback(int rv) {
-  if (rv < OK)
-    OnHandshakeCompletion();
   if (!user_connect_callback_.is_null()) {
     CompletionCallback c = user_connect_callback_;
     user_connect_callback_.Reset();
     c.Run(rv > OK ? OK : rv);
   }
 }
 
 void SSLClientSocketOpenSSL::UpdateServerCert() {
   // TODO(vadimt): Remove ScopedTracker below once crbug.com/424386 is fixed.
   tracked_objects::ScopedTracker tracking_profile(
@@ skipping 190 matching lines @@
     }
 
     bool network_moved = DoTransportIO();
     if (network_moved && next_handshake_state_ == STATE_HANDSHAKE) {
       // In general we exit the loop if rv is ERR_IO_PENDING.  In this
       // special case we keep looping even if rv is ERR_IO_PENDING because
       // the transport IO may allow DoHandshake to make progress.
       rv = OK;  // This causes us to stay in the loop.
     }
   } while (rv != ERR_IO_PENDING && next_handshake_state_ != STATE_NONE);
-
   return rv;
 }
 
 int SSLClientSocketOpenSSL::DoReadLoop() {
   bool network_moved;
   int rv;
   do {
     rv = DoPayloadRead();
     network_moved = DoTransportIO();
   } while (rv == ERR_IO_PENDING && network_moved);
@@ skipping 100 matching lines @@
                                          pending_read_error_info_));
     pending_read_ssl_error_ = SSL_ERROR_NONE;
     pending_read_error_info_ = OpenSSLErrorInfo();
   }
   return rv;
 }
 
 int SSLClientSocketOpenSSL::DoPayloadWrite() {
   crypto::OpenSSLErrStackTracer err_tracer(FROM_HERE);
   int rv = SSL_write(ssl_, user_write_buf_->data(), user_write_buf_len_);
+

[davidben, 2015/03/09 18:02:36]

Spurious? (It was probably a spurious change before too. *shrug*)

   if (rv >= 0) {
     net_log_.AddByteTransferEvent(NetLog::TYPE_SSL_SOCKET_BYTES_SENT, rv,
                                   user_write_buf_->data());
     return rv;
   }
 
   int ssl_error = SSL_get_error(ssl_, rv);
   OpenSSLErrorInfo error_info;
   int net_error = MapOpenSSLErrorWithDetails(ssl_error, err_tracer,
                                              &error_info);
@@ skipping 233 matching lines @@
                     NetLog::IntegerCallback("cert_count", 0));
   return 1;
 }
 
 int SSLClientSocketOpenSSL::CertVerifyCallback(X509_STORE_CTX* store_ctx) {
   // TODO(vadimt): Remove ScopedTracker below once crbug.com/424386 is fixed.
   tracked_objects::ScopedTracker tracking_profile(
       FROM_HERE_WITH_EXPLICIT_FUNCTION(
           "424386 SSLClientSocketOpenSSL::CertVerifyCallback"));
 
-  if (!completed_connect_) {
+  if (!completed_handshake_) {
     // If the first handshake hasn't completed then we accept any certificates
     // because we verify after the handshake.
     return 1;
   }
 
   // Disallow the server certificate to change in a renegotiation.
   if (server_cert_chain_->empty()) {
     LOG(ERROR) << "Received invalid certificate chain between handshakes";
     return 0;
   }
@@ skipping 110 matching lines @@
       FROM_HERE_WITH_EXPLICIT_FUNCTION(
           "424386 SSLClientSocketOpenSSL::BIOCallback"));
 
   SSLClientSocketOpenSSL* socket = reinterpret_cast<SSLClientSocketOpenSSL*>(
       BIO_get_callback_arg(bio));
   CHECK(socket);
   return socket->MaybeReplayTransportError(
       bio, cmd, argp, argi, argl, retvalue);
 }
 
-// static
-void SSLClientSocketOpenSSL::InfoCallback(const SSL* ssl,
-                                          int type,
-                                          int /*val*/) {
-  // TODO(vadimt): Remove ScopedTracker below once crbug.com/424386 is fixed.
-  tracked_objects::ScopedTracker tracking_profile(
-      FROM_HERE_WITH_EXPLICIT_FUNCTION(
-          "424386 SSLClientSocketOpenSSL::InfoCallback"));
-
-  if (type == SSL_CB_HANDSHAKE_DONE) {
-    SSLClientSocketOpenSSL* ssl_socket =
-        SSLContext::GetInstance()->GetClientSocketFromSSL(ssl);
-    ssl_socket->handshake_succeeded_ = true;
-    ssl_socket->CheckIfHandshakeFinished();
-  }
-}
-
-// Determines if both the handshake and certificate verification have completed
-// successfully, 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 (handshake_succeeded_ && marked_session_as_good_)
-    OnHandshakeCompletion();
-}
-
 void SSLClientSocketOpenSSL::AddSCTInfoToSSLInfo(SSLInfo* ssl_info) const {
   for (ct::SCTList::const_iterator iter =
        ct_verify_result_.verified_scts.begin();
        iter != ct_verify_result_.verified_scts.end(); ++iter) {
     ssl_info->signed_certificate_timestamps.push_back(
         SignedCertificateTimestampAndStatus(*iter, ct::SCT_STATUS_OK));
   }
   for (ct::SCTList::const_iterator iter =
        ct_verify_result_.invalid_scts.begin();
        iter != ct_verify_result_.invalid_scts.end(); ++iter) {
     ssl_info->signed_certificate_timestamps.push_back(
         SignedCertificateTimestampAndStatus(*iter, ct::SCT_STATUS_INVALID));
   }
   for (ct::SCTList::const_iterator iter =
        ct_verify_result_.unknown_logs_scts.begin();
        iter != ct_verify_result_.unknown_logs_scts.end(); ++iter) {
     ssl_info->signed_certificate_timestamps.push_back(
         SignedCertificateTimestampAndStatus(*iter,
                                             ct::SCT_STATUS_LOG_UNKNOWN));
   }
 }
 
 scoped_refptr<X509Certificate>
 SSLClientSocketOpenSSL::GetUnverifiedServerCertificateChain() const {
   return server_cert_;
 }
 
 }  // namespace net