SanitizeProxyAuth: Whitelist all hop-by-hop headers

SanitizeProxyAuth: Whitelist all hop-by-hop headers

We were discarding "Proxy-Authenticate: keep-alive" headers in HTTP/1.0 responses, which broke multi-step connection-level authentication methods for proxies that sent HTTP/1.0 responses.

BUG=463937
Committed: https://crrev.com/34f63b55b51e8fc46ad86334783a665d5f487738
Cr-Commit-Position: refs/heads/master@{#319219}

[Deprecated (see juliatuttle), 2015-03-05 02:05:24]

ttuttle@chromium.org changed reviewers:
+ asanka@chromium.org, cbentzel@chromium.org, rsleevi@chromium.org

[Deprecated (see juliatuttle), 2015-03-05 02:05:24]

ttuttle@chromium.org changed required reviewers:
+ cbentzel@chromium.org

[Deprecated (see juliatuttle), 2015-03-05 02:05:24]

PTAL, cbentzel.

FYI, asanka and rsleevi.

[cbentzel, 2015-03-05 02:08:48]

cbentzel@chromium.org changed reviewers:
+ rch@chromium.org

[cbentzel, 2015-03-05 02:08:49]

LGTM

rch was in initial review as well.

https://codereview.chromium.org/982733002/diff/40001/net/http/proxy_client_so...
File net/http/proxy_client_socket.cc (right):

https://codereview.chromium.org/982733002/diff/40001/net/http/proxy_client_so...
net/http/proxy_client_socket.cc:108: CopyHeaderValues(old_headers, new_headers,
"content-length");
As we chatted, main thing I want to make sure is that this drains the body
between rounds of authentication but doesn't actually run the body when the user
cancels doing another round of auth. Provided that works, OK.

[Deprecated (see juliatuttle), 2015-03-05 02:32:22]

On 2015/03/05 02:08:49, cbentzel wrote:
> LGTM
> 
> rch was in initial review as well.
> 
>
https://codereview.chromium.org/982733002/diff/40001/net/http/proxy_client_so...
> File net/http/proxy_client_socket.cc (right):
> 
>
https://codereview.chromium.org/982733002/diff/40001/net/http/proxy_client_so...
> net/http/proxy_client_socket.cc:108: CopyHeaderValues(old_headers,
new_headers,
> "content-length");
> As we chatted, main thing I want to make sure is that this drains the body
> between rounds of authentication but doesn't actually run the body when the
user
> cancels doing another round of auth. Provided that works, OK.

I am fairly confident that it works; BasicProxyAuthCancelTunnel specifically
says it checks for that.

I've forked the BasicAuthProxy{No,}KeepAlive test cases into {Http10,Http11}
versions, and fixed the *NoKeepAlive* tests to ensure that they validate the
proxy responses as not-keep-alive. (They are not actually making a new
connection, but that's a much bigger fix.)

I've confirmed that without the change to proxy_client_socket.cc, three of the
four test cases (all except no-keep-alive on HTTP/1.0) fail, and that with the
change, they all pass.

[Deprecated (see juliatuttle), 2015-03-05 02:35:22]

The CQ bit was checked by ttuttle@chromium.org

[Deprecated (see juliatuttle), 2015-03-05 02:35:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
cbentzel@chromium.org
Link to the patchset: https://codereview.chromium.org/982733002/#ps80001 (title:
"Split no-keep-alive tests as well")

[commit-bot: I haz the power, 2015-03-05 02:37:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/982733002/80001

[commit-bot: I haz the power, 2015-03-05 04:33:11]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-05 04:33:38]

Patchset 5 (id:??) landed as
https://crrev.com/34f63b55b51e8fc46ad86334783a665d5f487738
Cr-Commit-Position: refs/heads/master@{#319219}

[wtc, 2015-03-06 02:22:40]

wtc@chromium.org changed reviewers:
+ wtc@chromium.org

[wtc, 2015-03-06 02:22:41]

Drive-by review comments: I suggest some improvements to comments.

https://codereview.chromium.org/982733002/diff/80001/net/http/proxy_client_so...
File net/http/proxy_client_socket.cc (right):

https://codereview.chromium.org/982733002/diff/80001/net/http/proxy_client_so...
net/http/proxy_client_socket.cc:99: // behavior.

The comment for ProxyClientSocket::SanitizeProxyAuth in proxy_client_socket.h
should be updated. In particular, that comment should say which headers (at
least give some representative examples) must be stripped and why (for security
reasons?).

https://codereview.chromium.org/982733002/diff/80001/net/http/proxy_client_so...
net/http/proxy_client_socket.cc:110: CopyHeaderValues(old_headers, new_headers,
"proxy-authenticate");

This list of headers looks very ad hoc to me. I'm curious how you determined
which headers must be copied. Some are obvious, such as "proxy-connection" and
"keep-alive", which are needed for keep-alive/persistent connections. Perhaps we
can group the headers, something like:

  // For keep-alive/persistent connections
  ... "connection");
  ... "proxy-connection");
  ... "keep-alive");

  // For chunked encoding or content length.
  ... "trailer");
  ... "transfer-encoding");
  ... "content-length");

  etc.

https://codereview.chromium.org/982733002/diff/80001/net/http/proxy_client_so...
net/http/proxy_client_socket.cc:129: "Connection: close\n"

Nit: it is inconsistent within this file whether the HTTP headers are spelled in
all lowercase or capitalized.