Don't rely on SSL cipher fallback in proxy auth.

net/http/http_proxy_client_socket_wrapper.cc

 // Copyright 2015 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.
 
 #include "net/http/http_proxy_client_socket_wrapper.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ skipping 41 matching lines @@
       respect_limits_(respect_limits),
       connect_timeout_duration_(connect_timeout_duration),
       proxy_negotiation_timeout_duration_(proxy_negotiation_timeout_duration),
       transport_pool_(transport_pool),
       ssl_pool_(ssl_pool),
       transport_params_(transport_params),
       ssl_params_(ssl_params),
       user_agent_(user_agent),
       endpoint_(endpoint),
       spdy_session_pool_(spdy_session_pool),
+      has_restarted_(false),
       tunnel_(tunnel),
       proxy_delegate_(proxy_delegate),
       using_spdy_(false),
       http_auth_controller_(
           tunnel ? new HttpAuthController(
                        HttpAuth::AUTH_PROXY,
                        GURL((ssl_params_.get() ? "https://" : "http://") +
                             GetDestination().host_port_pair().ToString()),
                        http_auth_cache,
                        http_auth_handler_factory)
@@ skipping 493 matching lines @@
 int HttpProxyClientSocketWrapper::DoRestartWithAuth() {
   DCHECK(transport_socket_);
 
   next_state_ = STATE_RESTART_WITH_AUTH_COMPLETE;
   return transport_socket_->RestartWithAuth(base::Bind(
       &HttpProxyClientSocketWrapper::OnIOComplete, base::Unretained(this)));
 }
 
 int HttpProxyClientSocketWrapper::DoRestartWithAuthComplete(int result) {
   DCHECK_NE(ERR_IO_PENDING, result);
+
   // If the connection could not be reused to attempt to send proxy auth
-  // credentials, try reconnecting. If auth credentials were sent, pass the
-  // error on to caller, even if the credentials may have passed a close message
-  // from the server in flight.
-  if (result == ERR_UNABLE_TO_REUSE_CONNECTION_FOR_PROXY_AUTH) {
-    // If can't reuse the connection, attempt to create a new one.
+  // credentials, try reconnecting. Do not reset the HttpAuthController in this
+  // case; the server may, for instance, send "Proxy-Connection: close" and
+  // expect that each leg of the authentication progress on separate
+  // connections.
+  bool reconnect = result == ERR_UNABLE_TO_REUSE_CONNECTION_FOR_PROXY_AUTH;
+
+  // If auth credentials were sent but the connection was closed, the server may
+  // have timed out while the user was selecting credentials. Retry once.
+  if (!has_restarted_ &&
+      (result == ERR_CONNECTION_CLOSED || result == ERR_CONNECTION_RESET ||
+       result == ERR_CONNECTION_ABORTED ||
+       result == ERR_SOCKET_NOT_CONNECTED)) {
+    reconnect = true;
+    has_restarted_ = true;
+
+    // Release any auth state bound to the connection. The new connection will
+    // start the current scheme from scratch.
+    if (http_auth_controller_)
+      http_auth_controller_->OnConnectionClosed();

[asanka, 2017/03/02 22:38:52]

The one case I can think of where this assumption breaks down is the one where:
* The proxy server responds with a 407 over a keep-alive connection.
* The UA prompts the user, gets some credentials, and attempts to respond to the
challenge.
* UA finds out that the connection is dead.

In this case, discarding the handler causes the credentials to also be lost. So
the subsequent CONNECT won't have an authorization header. So we prompt the
user. Ad infinitum.

I was pretty sure there was a test for this, and there is:
AuthAllowsDefaultCredentialsTunnelServerClosesConnection

But unfortunately, the initial server response doesn't specify a Content-Length.
So we hit the ERR_UNABLE_TO_REUSE_CONNECTION_FOR_PROXY_AUTH code path instead of
the ERR_CONNECTION_CLOSED code path, which is why it wasn't failing for this CL.

[davidben, 2017/04/07 18:11:46]

I'm confused. I thought this was the case this codepath was trying to target to
begin with.

What behavior should we have here? To avoid going down a rabbithole, shall we
forgo this plan and just try to preserve existing behavior?

The current state of the world is, on these errors, we try to grab the
HttpStream anew. But it does suggest that the "start over completely" worldview
is at least plausible.

Preserving this behavior at HttpStream is tricky as we lose context for the
error code (the SSL retry is similarly mistargeted). But we could preserve the
behavior with either:

- Rather than OnConnectionClosed, just re-create http_auth_controller_. I do not
understand HttpAuthController will enough to know the difference between that
and OnConnectionClosed. My impression from you was that we did that to preserve
credentials, but apparently not?

- Lift it to a higher level and make SSLConnectJob::DoTunnelConnectComplete try
again if we hit various errors.

And then we could leave a TODO for the credential case.

WDYT?

[asanka, 2017/04/12 23:02:32]

Yeah, I'm okay with landing this and fixing the credential discard issue. While
looking at this I wrote up a CL to do that.

+  }
+
+  if (reconnect) {
+    // Attempt to create a new one.
     transport_socket_.reset();
+
     // Reconnect with HIGHEST priority to get in front of other requests that
     // don't yet have the information |http_auth_controller_| does.
     // TODO(mmenke): This may still result in waiting in line, if there are
     //               other HIGHEST priority requests. Consider a workaround for
     //               that. Starting the new request before releasing the old
     //               socket and using RespectLimits::Disabled would work,
     //               without exceding the the socket pool limits (Since the old
     //               socket would free up the extra socket slot when destroyed).
     priority_ = HIGHEST;
     next_state_ = STATE_BEGIN_CONNECT;
@@ skipping 32 matching lines @@
 const HostResolver::RequestInfo&
 HttpProxyClientSocketWrapper::GetDestination() {
   if (transport_params_) {
     return transport_params_->destination();
   } else {
     return ssl_params_->GetDirectConnectionParams()->destination();
   }
 }
 
 }  // namespace net