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

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

When doing HTTP proxy auth, we may need to continue or retry auth on a
fresh connection. This may happen if the server did not allow keep-alive
or if the server timed out while we were prompting the user.

In response to this, we may choose to continue the existing
HttpAuthHandler state on a new connection (which would require the
server not bind auth state to the connection) or to restart it (if the
server did do so).

The existing logic in HttpProxyClientSocketWrapper distinguished these
cases with the ERR_UNABLE_TO_REUSE_CONNECTION_FOR_PROXY_AUTH error code;
if we had not send anything out of the HttpAuthController yet, we assume
that it's okay to continue without a reset. (We can't simply reset it
every time because a server which does not support connection reuse at
all would never make progress.)

If we had extracted a token from the HttpAuthController and hit an error
reading the response, HttpProxyClientSocketWrapper would forward the
error up to be retried at a higher level. However, there was no higher
level logic to retry it. The usual socket reuse race condition retry
acts on a different set of signals. Instead, we were relying on the SSL
cipher fallback (which triggers on a similar set of errors) to restart
everything from the top.

Instead, if we see an error code indicative of such a race, retry at the
HttpProxyClientSocketWrapper level, after telling the HttpAuthController
to drop all state from the current HttpAuthHandler. This unblocks
removing the now otherwise unused SSL cipher fallback.

BUG=684730
Review-Url: https://codereview.chromium.org/2688173002
Cr-Commit-Position: refs/heads/master@{#465009}
Committed: https://chromium.googlesource.com/chromium/src/+/8c7089a4184074fb5edcc9865906147e185f0514

[davidben, 2017-02-10 17:36:05]

The CQ bit was checked by davidben@chromium.org to run a CQ dry run

[davidben, 2017-02-10 17:36:37]

davidben@chromium.org changed reviewers:
+ asanka@chromium.org

[davidben, 2017-02-10 17:36:38]

RFC for whether this is the correct approach or not.

[commit-bot: I haz the power, 2017-02-10 17:36:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-10 19:10:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-10 19:10:40]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

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

https://codereview.chromium.org/2688173002/diff/1/net/http/http_proxy_client_...
File net/http/http_proxy_client_socket_wrapper.cc (right):

https://codereview.chromium.org/2688173002/diff/1/net/http/http_proxy_client_...
net/http/http_proxy_client_socket_wrapper.cc:596:
http_auth_controller_->OnConnectionClosed();
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]

https://codereview.chromium.org/2688173002/diff/1/net/http/http_proxy_client_...
File net/http/http_proxy_client_socket_wrapper.cc (right):

https://codereview.chromium.org/2688173002/diff/1/net/http/http_proxy_client_...
net/http/http_proxy_client_socket_wrapper.cc:596:
http_auth_controller_->OnConnectionClosed();
On 2017/03/02 22:38:52, asanka wrote:
> 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.

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]

LGTM. Apologies for the unreasonably long delay.

https://codereview.chromium.org/2688173002/diff/1/net/http/http_proxy_client_...
File net/http/http_proxy_client_socket_wrapper.cc (right):

https://codereview.chromium.org/2688173002/diff/1/net/http/http_proxy_client_...
net/http/http_proxy_client_socket_wrapper.cc:596:
http_auth_controller_->OnConnectionClosed();
On 2017/04/07 18:11:46, davidben wrote:
> On 2017/03/02 22:38:52, asanka wrote:
> > 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.
> 
> 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?

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.

[davidben, 2017-04-17 19:29:12]

The CQ bit was checked by davidben@chromium.org

[davidben, 2017-04-17 19:29:13]

The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org
Link to the patchset: https://codereview.chromium.org/2688173002/#ps40001
(title: "unnecessary virtual")

[commit-bot: I haz the power, 2017-04-17 19:29:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-17 20:38:25]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492457352775420,
"parent_rev": "8b182de3a769150b6eca11e185741a704726d046", "commit_rev":
"2776f968396df3b581a4b5020101c77cd3289061"}

[commit-bot: I haz the power, 2017-04-17 20:38:39]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492457352775420,
"parent_rev": "5e4b5aa0e19c52343f9e7cfe6d1f584eeab460d6", "commit_rev":
"8c7089a4184074fb5edcc9865906147e185f0514"}

[commit-bot: I haz the power, 2017-04-17 20:39:37]

Description was changed from

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

When doing HTTP proxy auth, we may need to continue or retry auth on a
fresh connection. This may happen if the server did not allow keep-alive
or if the server timed out while we were prompting the user.

In response to this, we may choose to continue the existing
HttpAuthHandler state on a new connection (which would require the
server not bind auth state to the connection) or to restart it (if the
server did do so).

The existing logic in HttpProxyClientSocketWrapper distinguished these
cases with the ERR_UNABLE_TO_REUSE_CONNECTION_FOR_PROXY_AUTH error code;
if we had not send anything out of the HttpAuthController yet, we assume
that it's okay to continue without a reset. (We can't simply reset it
every time because a server which does not support connection reuse at
all would never make progress.)

If we had extracted a token from the HttpAuthController and hit an error
reading the response, HttpProxyClientSocketWrapper would forward the
error up to be retried at a higher level. However, there was no higher
level logic to retry it. The usual socket reuse race condition retry
acts on a different set of signals. Instead, we were relying on the SSL
cipher fallback (which triggers on a similar set of errors) to restart
everything from the top.

Instead, if we see an error code indicative of such a race, retry at the
HttpProxyClientSocketWrapper level, after telling the HttpAuthController
to drop all state from the current HttpAuthHandler. This unblocks
removing the now otherwise unused SSL cipher fallback.

BUG=684730
==========

to

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

When doing HTTP proxy auth, we may need to continue or retry auth on a
fresh connection. This may happen if the server did not allow keep-alive
or if the server timed out while we were prompting the user.

In response to this, we may choose to continue the existing
HttpAuthHandler state on a new connection (which would require the
server not bind auth state to the connection) or to restart it (if the
server did do so).

The existing logic in HttpProxyClientSocketWrapper distinguished these
cases with the ERR_UNABLE_TO_REUSE_CONNECTION_FOR_PROXY_AUTH error code;
if we had not send anything out of the HttpAuthController yet, we assume
that it's okay to continue without a reset. (We can't simply reset it
every time because a server which does not support connection reuse at
all would never make progress.)

If we had extracted a token from the HttpAuthController and hit an error
reading the response, HttpProxyClientSocketWrapper would forward the
error up to be retried at a higher level. However, there was no higher
level logic to retry it. The usual socket reuse race condition retry
acts on a different set of signals. Instead, we were relying on the SSL
cipher fallback (which triggers on a similar set of errors) to restart
everything from the top.

Instead, if we see an error code indicative of such a race, retry at the
HttpProxyClientSocketWrapper level, after telling the HttpAuthController
to drop all state from the current HttpAuthHandler. This unblocks
removing the now otherwise unused SSL cipher fallback.

BUG=684730

Review-Url: https://codereview.chromium.org/2688173002
Cr-Commit-Position: refs/heads/master@{#465009}
Committed:
https://chromium.googlesource.com/chromium/src/+/8c7089a4184074fb5edcc9865906...
==========

[commit-bot: I haz the power, 2017-04-17 20:39:40]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/8c7089a4184074fb5edcc9865906...