Add a dedicated error code for TLS 1.3 interference.

net/socket/ssl_client_socket_pool.cc

Index: net/socket/ssl_client_socket_pool.cc
diff --git a/net/socket/ssl_client_socket_pool.cc b/net/socket/ssl_client_socket_pool.cc
index b335e46b1f95330864f82f380a9f69f5e23153ee..be9b07537528eba18c12f8186e074d9fe90b9f5b 100644
--- a/net/socket/ssl_client_socket_pool.cc
+++ b/net/socket/ssl_client_socket_pool.cc
@@ -130,7 +130,9 @@ SSLConnectJob::SSLConnectJob(const std::string& group_name,
                     ? "pm/" + context.ssl_session_cache_shard
                     : context.ssl_session_cache_shard)),
       callback_(
-          base::Bind(&SSLConnectJob::OnIOComplete, base::Unretained(this))) {}
+          base::Bind(&SSLConnectJob::OnIOComplete, base::Unretained(this))),
+      version_interference_probe_(false),
+      version_interference_probe_error_(OK) {}
 
 SSLConnectJob::~SSLConnectJob() {
 }
@@ -236,7 +238,10 @@ int SSLConnectJob::DoTransportConnect() {
 }
 
 int SSLConnectJob::DoTransportConnectComplete(int result) {
-  connection_attempts_ = transport_socket_handle_->connection_attempts();
+  connection_attempts_.insert(
+      connection_attempts_.end(),
+      transport_socket_handle_->connection_attempts().begin(),
+      transport_socket_handle_->connection_attempts().end());
   if (result == OK) {
     next_state_ = STATE_SSL_CONNECT;
     transport_socket_handle_->socket()->GetPeerAddress(&server_address_);
@@ -321,9 +326,15 @@ int SSLConnectJob::DoSSLConnect() {
 
   connect_timing_.ssl_start = base::TimeTicks::Now();
 
+  SSLConfig ssl_config = params_->ssl_config();
+  if (version_interference_probe_) {
+    DCHECK_EQ(SSL_PROTOCOL_VERSION_TLS1_3, ssl_config.version_max);
+    ssl_config.version_max = SSL_PROTOCOL_VERSION_TLS1_2;
+    ssl_config.version_interference_probe = true;

[mmenke, 2017/04/10 19:08:46]

It seems a little bit silly that we're exposing this to the next layer down.  I
don't suppose we could just map successful 1.2 handshakes to the new error code
at this layer?  Or does the next layer down have magic that has to go there?

[davidben, 2017/04/10 19:52:25]

Very little magic, but some. I'd prefer the SSL session caches sharded between
the two modes.

[mmenke, 2017/04/10 19:55:05]

That's unfortunate, but I'll defer to you on that.

+  }
   ssl_socket_ = client_socket_factory_->CreateSSLClientSocket(
-      std::move(transport_socket_handle_), params_->host_and_port(),
-      params_->ssl_config(), context_);
+      std::move(transport_socket_handle_), params_->host_and_port(), ssl_config,
+      context_);
   return ssl_socket_->Connect(callback_);
 }
 
@@ -346,6 +357,32 @@ int SSLConnectJob::DoSSLConnectComplete(int result) {
     return ERR_ALPN_NEGOTIATION_FAILED;
   }
 
+  // Perform a TLS 1.3 version interference probe on various connection
+  // errors. The retry will never produce a successful connection but may map
+  // errors to ERR_SSL_VERSION_INTERFERENCE, which signals a probable
+  // version-interfering middlebox.
+  if (params_->ssl_config().version_max == SSL_PROTOCOL_VERSION_TLS1_3 &&
+      !params_->ssl_config().deprecated_cipher_suites_enabled &&
+      !version_interference_probe_) {
+    if (result == ERR_CONNECTION_CLOSED || result == ERR_SSL_PROTOCOL_ERROR ||
+        result == ERR_SSL_VERSION_OR_CIPHER_MISMATCH ||
+        result == ERR_CONNECTION_RESET ||
+        result == ERR_SSL_BAD_RECORD_MAC_ALERT) {
+      // Report the error code for each time a version interference probe is
+      // triggered.
+      UMA_HISTOGRAM_SPARSE_SLOWLY("Net.SSLVersionInterferenceProbeTrigger",
+                                  std::abs(result));

[mmenke, 2017/04/10 19:08:46]

I think std::abs is declared in <math>?

[davidben, 2017/04/10 19:52:25]

cstdlib, apparently.

+      net_log().AddEventWithNetErrorCode(
+          NetLogEventType::SSL_VERSION_INTERFERENCE_PROBE, result);
+
+      ResetStateForRetry();
+      version_interference_probe_ = true;
+      version_interference_probe_error_ = result;

[mmenke, 2017/04/10 19:08:46]

This was the error that triggered the probe, not the result of the probe, which
is not what I would have expected from the name.

Maybe original_error?  Or version_interference_probe_triggering_error_?  Not
sure.  The comment for the variable is certainly fine.

[davidben, 2017/04/10 19:52:25]

What about version_interference_error_ just so it's not so long and to match the
histogram below?

[mmenke, 2017/04/10 19:55:06]

SGTM

+      next_state_ = GetInitialState(params_->GetConnectionType());
+      return OK;
+    }
+  }
+
   const std::string& host = params_->host_and_port().host();
   bool is_google =
       host == "google.com" ||
@@ -450,6 +487,14 @@ int SSLConnectJob::DoSSLConnectComplete(int result) {
                                 std::abs(result));
   }
 
+  if (result == ERR_SSL_VERSION_INTERFERENCE) {
+    // Record the error code version interference was detected at.
+    DCHECK(version_interference_probe_);
+    DCHECK_NE(OK, version_interference_probe_error_);
+    UMA_HISTOGRAM_SPARSE_SLOWLY("Net.SSLVersionInterferenceError",
+                                std::abs(version_interference_probe_error_));
+  }
+
   if (result == OK || IsCertificateError(result)) {
     SetSocket(std::move(ssl_socket_));
   } else if (result == ERR_SSL_CLIENT_AUTH_CERT_NEEDED) {
@@ -480,6 +525,13 @@ int SSLConnectJob::ConnectInternal() {
   return DoLoop(OK);
 }
 
+void SSLConnectJob::ResetStateForRetry() {
+  transport_socket_handle_.reset();
+  ssl_socket_.reset();
+  error_response_info_ = HttpResponseInfo();
+  server_address_ = IPEndPoint();
+}
+
 SSLClientSocketPool::SSLConnectJobFactory::SSLConnectJobFactory(
     TransportClientSocketPool* transport_pool,
     SOCKSClientSocketPool* socks_pool,