Unwind the SSL version interference probe.

This takes away one source of SSLConfig variation. We can instead put it
back in SSLConnectJob (the flattened socket pools should avoid the NTLM
bug), but no point when we don't need it anymore. TLS 1.3 has been
shipping for over a year now, and the admin policy expired in M75. Per
chrome/VERSION, trunk is now M76.

This CL does not change whether connections with TLS-1.3-intolerant
networks or servers fail. It only shifts the error back to
ERR_SSL_PROTOCOL_ERROR, ERR_CONNECTION_RESET, or whatever we actually
get out of the server.

Bug: 951205
Change-Id: I95f1730c7b835c9aab1998c0cda56bcfaca45e1b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1574368
Commit-Queue: David Benjamin <davidben@chromium.org>
Commit-Queue: Matt Menke <mmenke@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Steven Valdez <svaldez@chromium.org>
Auto-Submit: David Benjamin <davidben@chromium.org>
Cr-Commit-Position: refs/heads/master@{#653359}

components/error_page/common/localized_error.cc

@@ -252,12 +252,6 @@ const LocalizedErrorMap net_error_options[] = {
    SUGGEST_CONTACT_ADMINISTRATOR,
    SHOW_NO_BUTTONS,
   },
-  {net::ERR_SSL_VERSION_INTERFERENCE,
-   IDS_ERRORPAGES_HEADING_NOT_AVAILABLE,
-   IDS_ERRORPAGES_SUMMARY_CONNECTION_FAILED,
-   SUGGEST_CHECK_CONNECTION | SUGGEST_FIREWALL_CONFIG | SUGGEST_PROXY_CONFIG,
-   SHOW_BUTTON_RELOAD,
-  },
   {net::ERR_TLS13_DOWNGRADE_DETECTED,
    IDS_ERRORPAGES_HEADING_NOT_AVAILABLE,
    IDS_ERRORPAGES_SUMMARY_CONNECTION_FAILED,

net/base/net_error_list.h

@@ -4,8 +4,9 @@
 
 // This file intentionally does not have header guards, it's included
 // inside a macro to generate enum values. The following line silences a
-// presubmit warning that would otherwise be triggered by this:
+// presubmit and Tricium warning that would otherwise be triggered by this:
 // no-include-guard-because-multiply-included
+// NOLINT(build/header_guard)
 
 // This file contains the list of network errors.
 
@@ -395,19 +396,7 @@ NET_ERROR(WS_UPGRADE, -173)
 // visible, because the normal Read() method is used as a fallback.
 NET_ERROR(READ_IF_READY_NOT_IMPLEMENTED, -174)
 
-// This error is emitted if TLS 1.3 is enabled, connecting with it failed, but
-// retrying at a downgraded maximum version succeeded. This could mean:
-//
-// 1. This is a transient network error that will be resolved when the user
-//    reloads.
-//
-// 2. The user is behind a buggy network middlebox, firewall, or proxy which is
-//    interfering with TLS 1.3.
-//
-// 3. The server is buggy and does not implement TLS version negotiation
-//    correctly. TLS 1.3 was tweaked to avoid a common server bug here, so this
-//    is unlikely.
-NET_ERROR(SSL_VERSION_INTERFERENCE, -175)
+// Error -175 was removed (SSL_VERSION_INTERFERENCE).
 
 // No socket buffer space is available.
 NET_ERROR(NO_BUFFER_SPACE, -176)

net/http/http_network_transaction.cc

@@ -133,8 +133,7 @@ HttpNetworkTransaction::HttpNetworkTransaction(RequestPriority priority,
       websocket_handshake_stream_base_create_helper_(nullptr),
       net_error_details_(),
       retry_attempts_(0),
-      num_restarts_(0),
-      ssl_version_interference_error_(OK) {
+      num_restarts_(0) {
 }
 
 HttpNetworkTransaction::~HttpNetworkTransaction() {
@@ -849,41 +848,6 @@ int HttpNetworkTransaction::DoCreateStreamComplete(int result) {
     return HandleHttp11Required(result);
   }
 
-  // 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 (IsSecureRequest() && !HasExceededMaxRetries() &&
-      server_ssl_config_.version_max == SSL_PROTOCOL_VERSION_TLS1_3 &&
-      !server_ssl_config_.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.
-      base::UmaHistogramSparse("Net.SSLVersionInterferenceProbeTrigger",
-                               std::abs(result));
-      net_log_.AddEventWithNetErrorCode(
-          NetLogEventType::SSL_VERSION_INTERFERENCE_PROBE, result);
-
-      retry_attempts_++;
-      server_ssl_config_.version_interference_probe = true;
-      server_ssl_config_.version_max = SSL_PROTOCOL_VERSION_TLS1_2;
-      ssl_version_interference_error_ = result;
-      ResetConnectionAndRequestForResend();
-      return OK;
-    }
-  }
-
-  if (result == ERR_SSL_VERSION_INTERFERENCE) {
-    // Record the error code version interference was detected at.
-    DCHECK(server_ssl_config_.version_interference_probe);
-    DCHECK_NE(OK, ssl_version_interference_error_);
-    base::UmaHistogramSparse("Net.SSLVersionInterferenceError",
-                             std::abs(ssl_version_interference_error_));
-  }
-
   // Handle possible client certificate errors that may have occurred if the
   // stream used SSL for one or more of the layers.
   result = HandleSSLClientAuthError(result);

net/http/http_network_transaction.h

@@ -434,10 +434,6 @@ class NET_EXPORT_PRIVATE HttpNetworkTransaction
   // Number of times the transaction was restarted via a RestartWith* call.
   size_t num_restarts_;
 
-  // The net::Error which triggered a TLS 1.3 version interference probe, or OK
-  // if none was triggered.
-  int ssl_version_interference_error_;
-
   DISALLOW_COPY_AND_ASSIGN(HttpNetworkTransaction);
 };
 

net/http/http_network_transaction_unittest.cc

@@ -4544,15 +4544,6 @@ TEST_F(HttpNetworkTransactionTest,
       ProxyResolutionService::CreateFixedFromPacResult(
           "PROXY myproxy:70", TRAFFIC_ANNOTATION_FOR_TESTS);
 
-  // When TLS 1.3 is enabled, spurious connections are made as part of the SSL
-  // version interference probes.
-  // TODO(crbug.com/906668): Correctly handle version interference probes to
-  // test TLS 1.3.
-  SSLConfig config;
-  config.version_max = SSL_PROTOCOL_VERSION_TLS1_2;
-  session_deps_.ssl_config_service =
-      std::make_unique<TestSSLConfigService>(config);
-
   auto auth_handler_factory = std::make_unique<HttpAuthHandlerMock::Factory>();
   auth_handler_factory->set_do_init_from_challenge(true);
 
@@ -7680,7 +7671,9 @@ TEST_F(HttpNetworkTransactionTest, NTLMOverHttp2) {
 
 // Test that, if we have an NTLM proxy and the origin resets the connection, we
 // do no retry forever checking for TLS version interference. This is a
-// regression test for https://crbug.com/823387.
+// regression test for https://crbug.com/823387. The version interference probe
+// has since been removed, but retain the regression test so we can update it if
+// we add future TLS retries.
 TEST_F(HttpNetworkTransactionTest, NTLMProxyTLSHandshakeReset) {
   // The NTLM test data expects the proxy to be named 'server'. The origin is
   // https://origin/.
@@ -7773,13 +7766,8 @@ TEST_F(HttpNetworkTransactionTest, NTLMProxyTLSHandshakeReset) {
 
   StaticSocketDataProvider data(data_reads, data_writes);
   SSLSocketDataProvider data_ssl(ASYNC, ERR_CONNECTION_RESET);
-  StaticSocketDataProvider data2(data_reads, data_writes);
-  SSLSocketDataProvider data_ssl2(ASYNC, ERR_CONNECTION_RESET);
-  data_ssl2.expected_ssl_version_max = SSL_PROTOCOL_VERSION_TLS1_2;
   session_deps_.socket_factory->AddSocketDataProvider(&data);
   session_deps_.socket_factory->AddSSLSocketDataProvider(&data_ssl);
-  session_deps_.socket_factory->AddSocketDataProvider(&data2);
-  session_deps_.socket_factory->AddSSLSocketDataProvider(&data_ssl2);
 
   // Start the transaction. The proxy responds with an NTLM authentication
   // request.
@@ -7794,7 +7782,8 @@ TEST_F(HttpNetworkTransactionTest, NTLMProxyTLSHandshakeReset) {
   ASSERT_TRUE(response);
   EXPECT_TRUE(CheckNTLMProxyAuth(response->auth_challenge));
 
-  // Configure credentials. The proxy responds with the challenge message.
+  // Configure credentials and restart. The proxy responds with the challenge
+  // message.
   rv = callback.GetResult(trans.RestartWithAuth(
       AuthCredentials(ntlm::test::kDomainUserCombined, ntlm::test::kPassword),
       callback.callback()));
@@ -7804,29 +7793,8 @@ TEST_F(HttpNetworkTransactionTest, NTLMProxyTLSHandshakeReset) {
   ASSERT_TRUE(response);
   EXPECT_FALSE(response->auth_challenge.has_value());
 
-  // Restart once more. The tunnel will be established and the the SSL handshake
-  // will reset. The TLS 1.3 version interference probe will then kick in and
-  // restart the process. The proxy responds with another NTLM authentiation
-  // request, but we don't need to provide credentials as the cached ones work/
-  rv = callback.GetResult(
-      trans.RestartWithAuth(AuthCredentials(), callback.callback()));
-  EXPECT_THAT(rv, IsOk());
-  EXPECT_TRUE(trans.IsReadyToRestartForAuth());
-  response = trans.GetResponseInfo();
-  ASSERT_TRUE(response);
-  EXPECT_FALSE(response->auth_challenge.has_value());
-
-  // The proxy responds with the NTLM challenge message.
-  rv = callback.GetResult(
-      trans.RestartWithAuth(AuthCredentials(), callback.callback()));
-  EXPECT_THAT(rv, IsOk());
-  EXPECT_TRUE(trans.IsReadyToRestartForAuth());
-  response = trans.GetResponseInfo();
-  ASSERT_TRUE(response);
-  EXPECT_FALSE(response->auth_challenge.has_value());
-
-  // Send the NTLM authenticate message. The tunnel is established and the
-  // handshake resets again. We should not retry again.
+  // Restart once more. The tunnel will be established and then the SSL
+  // handshake will reset.
   rv = callback.GetResult(
       trans.RestartWithAuth(AuthCredentials(), callback.callback()));
   EXPECT_THAT(rv, IsError(ERR_CONNECTION_RESET));

net/http/http_stream_factory_unittest.cc

@@ -416,8 +416,7 @@ class CapturePreconnectsTransportSocketPool : public TransportClientSocketPool {
     last_num_streams_ = -1;
     // Group ID that shouldn't match much.
     last_group_id_ = ClientSocketPool::GroupId(
-        HostPortPair(),
-        ClientSocketPool::SocketType::kSslVersionInterferenceProbe,
+        HostPortPair(), ClientSocketPool::SocketType::kSsl,
         PrivacyMode::PRIVACY_MODE_ENABLED);
   }
 

net/log/net_log_event_type_list.h

@@ -510,15 +510,6 @@ EVENT_TYPE(SSL_HANDSHAKE_ERROR)
 EVENT_TYPE(SSL_READ_ERROR)
 EVENT_TYPE(SSL_WRITE_ERROR)
 
-// An SSL connection needs to be retried with a lower protocol version to detect
-// if the error was due to a middlebox interfering with the protocol version we
-// offered.
-// The following parameters are attached to the event:
-//   {
-//     "net_error": <Net integer error code which triggered the probe>,
-//   }
-EVENT_TYPE(SSL_VERSION_INTERFERENCE_PROBE)
-
 // We found that our prediction of the server's certificates was correct and
 // we merged the verification with the SSLHostInfo. (Note: now obsolete.)
 EVENT_TYPE(SSL_VERIFICATION_MERGED)

net/socket/client_socket_pool.cc

@@ -76,10 +76,6 @@ std::string ClientSocketPool::GroupId::ToString() const {
       result = "ssl/" + result;
       break;
 
-    case ClientSocketPool::SocketType::kSslVersionInterferenceProbe:
-      result = "version-interference-probe/ssl/" + result;
-      break;
-
     case ClientSocketPool::SocketType::kFtp:
       result = "ftp/" + result;
       break;
@@ -133,10 +129,7 @@ std::unique_ptr<ConnectJob> ClientSocketPool::CreateConnectJob(
     RequestPriority request_priority,
     SocketTag socket_tag,
     ConnectJob::Delegate* delegate) {
-  bool using_ssl =
-      (group_id.socket_type() == ClientSocketPool::SocketType::kSsl ||
-       group_id.socket_type() ==
-           ClientSocketPool::SocketType::kSslVersionInterferenceProbe);
+  bool using_ssl = group_id.socket_type() == ClientSocketPool::SocketType::kSsl;
   return ConnectJob::CreateConnectJob(
       using_ssl, group_id.destination(), proxy_server,
       socket_params->proxy_annotation_tag(),

net/socket/client_socket_pool.h

@@ -102,9 +102,6 @@ class NET_EXPORT ClientSocketPool : public LowerLayeredPool {
     // destination, though not necessarily to the proxy, if there is one.
     kSsl,
 
-    // This is a connection for probing for SSL-breaking interference.
-    kSslVersionInterferenceProbe,
-
     // This is a connection through an HTTP proxy being used for FTP requests.
     kFtp,
   };

net/socket/client_socket_pool_base_unittest.cc

@@ -790,7 +790,6 @@ TEST_F(ClientSocketPoolBaseTest, GroupSeparation) {
   const ClientSocketPool::SocketType kSocketTypes[] = {
       ClientSocketPool::SocketType::kHttp,
       ClientSocketPool::SocketType::kSsl,
-      ClientSocketPool::SocketType::kSslVersionInterferenceProbe,
       ClientSocketPool::SocketType::kFtp,
   };
 

net/socket/client_socket_pool_manager.cc

@@ -66,7 +66,6 @@ ClientSocketPool::GroupId CreateGroupId(
     ClientSocketPoolManager::SocketGroupType group_type,
     const HostPortPair& endpoint,
     const ProxyInfo& proxy_info,
-    const SSLConfig& ssl_config_for_origin,
     PrivacyMode privacy_mode) {
   // Build the string used to uniquely identify connections of this type.
   // Determine the host and port to connect to.
@@ -77,11 +76,7 @@ ClientSocketPool::GroupId CreateGroupId(
   if (group_type == ClientSocketPoolManager::FTP_GROUP) {
     socket_type = ClientSocketPool::SocketType::kFtp;
   } else if (group_type == ClientSocketPoolManager::SSL_GROUP) {
-    if (!ssl_config_for_origin.version_interference_probe) {
-      socket_type = ClientSocketPool::SocketType::kSsl;
-    } else {
-      socket_type = ClientSocketPool::SocketType::kSslVersionInterferenceProbe;
-    }
+    socket_type = ClientSocketPool::SocketType::kSsl;
   }
 
   return ClientSocketPool::GroupId(endpoint, socket_type, privacy_mode);
@@ -97,10 +92,7 @@ scoped_refptr<ClientSocketPool::SocketParams> CreateSocketParams(
     const SSLConfig& ssl_config_for_origin,
     const SSLConfig& ssl_config_for_proxy,
     const OnHostResolutionCallback& resolution_callback) {
-  bool using_ssl =
-      (group_id.socket_type() == ClientSocketPool::SocketType::kSsl ||
-       group_id.socket_type() ==
-           ClientSocketPool::SocketType::kSslVersionInterferenceProbe);
+  bool using_ssl = group_id.socket_type() == ClientSocketPool::SocketType::kSsl;
   bool using_proxy_ssl = proxy_server.is_http_like() && !proxy_server.is_http();
   return base::MakeRefCounted<ClientSocketPool::SocketParams>(
       proxy_annotation_tag,
@@ -139,8 +131,7 @@ int InitSocketPoolHelper(
   }
 
   ClientSocketPool::GroupId connection_group =
-      CreateGroupId(group_type, origin_host_port, proxy_info,
-                    ssl_config_for_origin, privacy_mode);
+      CreateGroupId(group_type, origin_host_port, proxy_info, privacy_mode);
   scoped_refptr<ClientSocketPool::SocketParams> socket_params =
       CreateSocketParams(connection_group, proxy_info.proxy_server(),
                          proxy_info.traffic_annotation(), ssl_config_for_origin,

net/socket/client_socket_pool_unittest.cc

@@ -27,7 +27,6 @@ TEST(ClientSocketPool, GroupIdOperators) {
   const ClientSocketPool::SocketType kSocketTypes[] = {
       ClientSocketPool::SocketType::kHttp,
       ClientSocketPool::SocketType::kSsl,
-      ClientSocketPool::SocketType::kSslVersionInterferenceProbe,
       ClientSocketPool::SocketType::kFtp,
   };
 
@@ -100,19 +99,6 @@ TEST(ClientSocketPool, GroupIdToString) {
                                       PrivacyMode::PRIVACY_MODE_ENABLED)
                 .ToString());
 
-  EXPECT_EQ("version-interference-probe/ssl/foo:443",
-            ClientSocketPool::GroupId(
-                HostPortPair("foo", 443),
-                ClientSocketPool::SocketType::kSslVersionInterferenceProbe,
-                PrivacyMode::PRIVACY_MODE_DISABLED)
-                .ToString());
-  EXPECT_EQ("pm/version-interference-probe/ssl/bar:444",
-            ClientSocketPool::GroupId(
-                HostPortPair("bar", 444),
-                ClientSocketPool::SocketType::kSslVersionInterferenceProbe,
-                PrivacyMode::PRIVACY_MODE_ENABLED)
-                .ToString());
-
   EXPECT_EQ("ftp/foo:80",
             ClientSocketPool::GroupId(HostPortPair("foo", 80),
                                       ClientSocketPool::SocketType::kFtp,

net/socket/ssl_client_socket_impl.cc

@@ -964,11 +964,6 @@ int SSLClientSocketImpl::DoHandshakeComplete(int result) {
     return OK;
   }
 
-  if (ssl_config_.version_interference_probe) {
-    DCHECK_LT(ssl_config_.version_max, TLS1_3_VERSION);
-    return ERR_SSL_VERSION_INTERFERENCE;
-  }
-
   if (IsCachingEnabled()) {
     ssl_client_session_cache_->ResetLookupCount(GetSessionCacheKey());
   }
@@ -1625,11 +1620,7 @@ void SSLClientSocketImpl::AddCTInfoToSSLInfo(SSLInfo* ssl_info) const {
 }
 
 std::string SSLClientSocketImpl::GetSessionCacheKey() const {
-  std::string result = host_and_port_.ToString();
-
-  result.push_back('/');
-  result.push_back(ssl_config_.version_interference_probe ? '1' : '0');
-  return result;
+  return host_and_port_.ToString();
 }
 
 bool SSLClientSocketImpl::IsRenegotiationAllowed() const {

net/socket/ssl_client_socket_unittest.cc

@@ -3087,37 +3087,6 @@ TEST_F(SSLClientSocketTest, NoDHE) {
   EXPECT_THAT(rv, IsError(ERR_SSL_VERSION_OR_CIPHER_MISMATCH));
 }
 
-// Tests that the version_interference_probe option rejects successful
-// connections and passes errors through.
-TEST_F(SSLClientSocketTest, VersionInterferenceProbe) {
-  ASSERT_TRUE(StartTestServer(SpawnedTestServer::SSLOptions()));
-
-  SSLConfig ssl_config;
-  ssl_config.version_max = SSL_PROTOCOL_VERSION_TLS1_2;
-  ssl_config.version_interference_probe = true;
-
-  // Successful connections map to a dedicated error.
-  int rv;
-  ASSERT_TRUE(CreateAndConnectSSLClientSocket(ssl_config, &rv));
-  EXPECT_THAT(rv, IsError(ERR_SSL_VERSION_INTERFERENCE));
-
-  // Failed connections pass through.
-  TestCompletionCallback callback;
-  std::unique_ptr<StreamSocket> real_transport(
-      new TCPClientSocket(addr(), nullptr, nullptr, NetLogSource()));
-  std::unique_ptr<SynchronousErrorStreamSocket> transport(
-      new SynchronousErrorStreamSocket(std::move(real_transport)));
-  rv = callback.GetResult(transport->Connect(callback.callback()));
-  EXPECT_THAT(rv, IsOk());
-  SynchronousErrorStreamSocket* raw_transport = transport.get();
-  std::unique_ptr<SSLClientSocket> sock(CreateSSLClientSocket(
-      std::move(transport), spawned_test_server()->host_port_pair(),
-      ssl_config));
-  raw_transport->SetNextWriteError(ERR_CONNECTION_RESET);
-  rv = callback.GetResult(sock->Connect(callback.callback()));
-  EXPECT_THAT(rv, IsError(ERR_CONNECTION_RESET));
-}
-
 TEST_F(SSLClientSocketTest, RequireECDHE) {
   // Run test server without ECDHE.
   SpawnedTestServer::SSLOptions ssl_options;