Add embedder-specific headers to HTTP CONNECT tunnel request

Add embedder-specific headers to HTTP CONNECT tunnel request

Adds a mechanism by which a net/ embedder can add headers to
a HTTP CONNECT tunnel request.

BUG=400872
Committed: https://crrev.com/39e40610845c395edde654e40fc8d88e466aea81
Cr-Commit-Position: refs/heads/master@{#294252}

[bengr, 2014-08-28 18:28:06]

Patchset #1 (id:1) has been deleted

[bengr, 2014-08-28 18:31:08]

Patchset #1 (id:20001) has been deleted

[bengr, 2014-08-28 18:34:48]

Patchset #1 (id:40001) has been deleted

[bengr, 2014-08-30 01:14:24]

bengr@chromium.org changed reviewers:
+ marq@google.com, mmenke@chromium.org

[bengr, 2014-08-30 01:14:24]

mmenke: net/*, chrome/browser/io_thread*
marq: components/data_reduction_proxcy

[marq (ping after 24h), 2014-09-01 16:45:17]

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

[marq (ping after 24h), 2014-09-01 16:45:18]

components/ LGTM.

[marq (ping after 24h), 2014-09-01 16:45:52]

marq@chromium.org changed reviewers:
- marq@google.com

[mmenke, 2014-09-02 14:57:08]

mmenke@chromium.org changed reviewers:
+ mef@chromium.org

[mmenke, 2014-09-02 14:57:08]

[+mef]:  Mind taking this one?

I'm rather sad to see more complicated hooks like this - would much rather see
us just pre-populating the AUTH cache, though I assume there are reasons that
does not work.

[mmenke, 2014-09-02 15:06:44]

On 2014/09/02 14:57:08, mmenke wrote:
> [+mef]:  Mind taking this one?
> 
> I'm rather sad to see more complicated hooks like this - would much rather see
> us just pre-populating the AUTH cache, though I assume there are reasons that
> does not work.

Actually, just to verify:  Why can't we go back to proxy auth, and pre-populate
the auth cache?  I'm fine with being more strict about hiding AUTH information
from net-internals and devtools, if that's needed (Probably be best to just hide
them in the case of the data reduction proxy, though).

[mef, 2014-09-02 15:25:26]

On 2014/09/02 14:57:08, mmenke wrote:
> [+mef]:  Mind taking this one?
> 
> I'm rather sad to see more complicated hooks like this - would much rather see
> us just pre-populating the AUTH cache, though I assume there are reasons that
> does not work.

Will do.

[bengr, 2014-09-02 15:25:41]

On 2014/09/02 15:06:44, mmenke wrote:
> On 2014/09/02 14:57:08, mmenke wrote:
> > [+mef]:  Mind taking this one?
> > 
> > I'm rather sad to see more complicated hooks like this - would much rather
see
> > us just pre-populating the AUTH cache, though I assume there are reasons
that
> > does not work.
> 
> Actually, just to verify:  Why can't we go back to proxy auth, and
pre-populate
> the auth cache?  I'm fine with being more strict about hiding AUTH information
> from net-internals and devtools, if that's needed (Probably be best to just
hide
> them in the case of the data reduction proxy, though).

Three reasons:
First, some middle boxes strip proxy-authorization headers. This is mdw@'s
discovery so ask him for examples.
Second,  we will need similar hooks in a follow-up CL that acts on response
headers from a CONNECT request. These will be akin to the Chrome-Proxy header
processing in HTTP proxies.
Third, though not needed right now, we intend to use these hooks as a general
purpose control channel to the proxy, not just for auth.

[mef, 2014-09-02 17:08:36]

https://codereview.chromium.org/517693002/diff/80001/net/http/http_network_se...
File net/http/http_network_session.h (right):

https://codereview.chromium.org/517693002/diff/80001/net/http/http_network_se...
net/http/http_network_session.h:125: before_proxy_tunnel_request_callback;
Is there particular reason why it is a separate callback and not part of say
network_delegate?

[mmenke, 2014-09-02 20:11:25]

On 2014/09/02 17:08:36, mef wrote:
>
https://codereview.chromium.org/517693002/diff/80001/net/http/http_network_se...
> File net/http/http_network_session.h (right):
> 
>
https://codereview.chromium.org/517693002/diff/80001/net/http/http_network_se...
> net/http/http_network_session.h:125: before_proxy_tunnel_request_callback;
> Is there particular reason why it is a separate callback and not part of say
> network_delegate?

What ever happened to the idea of using a custom proxy service, and getting the
headers from there?

It's not exactly lovely, but it could include the extra headers in ProxyInfo,
which could then be copied to HttpProxySocketParams.

[bengr, 2014-09-05 02:55:03]

PTAL. The CL reflects the discussion on net-dev. I plan to add a test for
OnHeadersReceived.

[mef, 2014-09-05 17:22:51]

This looks much cleaner. It seems that it customary to use "blah* const" for
member pointers.

https://codereview.chromium.org/517693002/diff/120001/net/http/http_proxy_cli...
File net/http/http_proxy_client_socket_pool.h (right):

https://codereview.chromium.org/517693002/diff/120001/net/http/http_proxy_cli...
net/http/http_proxy_client_socket_pool.h:277: HttpProxyTunnelDelegate*
http_proxy_tunnel_delegate_;
Should this be HttpProxyTunnelDelegate* const?

https://codereview.chromium.org/517693002/diff/120001/net/http/http_proxy_tun...
File net/http/http_proxy_tunnel_delegate.h (right):

https://codereview.chromium.org/517693002/diff/120001/net/http/http_proxy_tun...
net/http/http_proxy_tunnel_delegate.h:21: // Provides the embedder an
opportunity to add extra request headers.
Would it make sense to comment on parameters here and below, or are they
sufficiently self-explanatory?

[bengr, 2014-09-07 21:02:24]

mef: PTAL

https://codereview.chromium.org/517693002/diff/120001/net/http/http_proxy_cli...
File net/http/http_proxy_client_socket_pool.h (right):

https://codereview.chromium.org/517693002/diff/120001/net/http/http_proxy_cli...
net/http/http_proxy_client_socket_pool.h:277: HttpProxyTunnelDelegate*
http_proxy_tunnel_delegate_;
On 2014/09/05 17:22:50, mef wrote:
> Should this be HttpProxyTunnelDelegate* const?

Done.

[mef, 2014-09-08 15:06:48]

https://codereview.chromium.org/517693002/diff/160001/net/base/proxy_delegate.h
File net/base/proxy_delegate.h (right):

https://codereview.chromium.org/517693002/diff/160001/net/base/proxy_delegate...
net/base/proxy_delegate.h:39: // Called after a proxy connection. Allows the
delegate to read/write
nit: indent

https://codereview.chromium.org/517693002/diff/160001/net/http/http_proxy_cli...
File net/http/http_proxy_client_socket.cc (right):

https://codereview.chromium.org/517693002/diff/160001/net/http/http_proxy_cli...
net/http/http_proxy_client_socket.cc:458:
proxy_delegate_->OnTunnelHeadersReceived(HostPortPair::FromURL(request_.url),
if (proxy_delegate_)

https://codereview.chromium.org/517693002/diff/160001/net/http/http_proxy_cli...
File net/http/http_proxy_client_socket.h (right):

https://codereview.chromium.org/517693002/diff/160001/net/http/http_proxy_cli...
net/http/http_proxy_client_socket.h:168: const HostPortPair proxy_server_;
Maybe add comment about ownership of proxy_delegate_?
Also, should it be const?

https://codereview.chromium.org/517693002/diff/160001/net/http/http_proxy_cli...
File net/http/http_proxy_client_socket_pool.h (right):

https://codereview.chromium.org/517693002/diff/160001/net/http/http_proxy_cli...
net/http/http_proxy_client_socket_pool.h:96: ProxyDelegate* proxy_delegate_;
const

https://codereview.chromium.org/517693002/diff/160001/net/socket/client_socke...
File net/socket/client_socket_pool_manager_impl.h (right):

https://codereview.chromium.org/517693002/diff/160001/net/socket/client_socke...
net/socket/client_socket_pool_manager_impl.h:152: ProxyDelegate*
proxy_delegate_;
const?

[mmenke, 2014-09-08 17:38:50]

Quick comments.  I think this approach looks fine.

https://codereview.chromium.org/517693002/diff/160001/net/base/proxy_delegate.h
File net/base/proxy_delegate.h (right):

https://codereview.chromium.org/517693002/diff/160001/net/base/proxy_delegate...
net/base/proxy_delegate.h:9: #include "url/gurl.h"
Can forward declare this.

https://codereview.chromium.org/517693002/diff/160001/net/base/proxy_delegate...
net/base/proxy_delegate.h:55: const HttpResponseHeaders& response_headers) = 0;
private:

DISALLOW_COPY_AND_ASSIGN?  (+ include macros.h for it)

https://codereview.chromium.org/517693002/diff/160001/net/http/http_network_s...
File net/http/http_network_session.h (right):

https://codereview.chromium.org/517693002/diff/160001/net/http/http_network_s...
net/http/http_network_session.h:124: net::ProxyDelegate* proxy_delegate;
nit:  net:: not needed.

https://codereview.chromium.org/517693002/diff/160001/net/http/http_network_t...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/517693002/diff/160001/net/http/http_network_t...
net/http/http_network_transaction_unittest.cc:569: 0, 0, NULL, host_resolver,
NULL, NULL, NULL, NULL) {}
nit:  +2 indent

https://codereview.chromium.org/517693002/diff/160001/net/http/http_proxy_cli...
File net/http/http_proxy_client_socket.cc (right):

https://codereview.chromium.org/517693002/diff/160001/net/http/http_proxy_cli...
net/http/http_proxy_client_socket.cc:410: if (proxy_delegate_) {
I think it makes more sense to move this below the
"auth_->AddAuthorizationHeader()" call.  That lets the delegate decide whether
it wants to override auth headers added by the network stack or not.

https://codereview.chromium.org/517693002/diff/160001/net/http/http_proxy_cli...
net/http/http_proxy_client_socket.cc:412: &authorization_headers);
Should have a couple tests for this.  Don't believe I saw one.  (Setting a
header when there wasn't one before, overriding the ones set by auth_)

https://codereview.chromium.org/517693002/diff/160001/net/http/http_proxy_cli...
net/http/http_proxy_client_socket.cc:458:
proxy_delegate_->OnTunnelHeadersReceived(HostPortPair::FromURL(request_.url),
On 2014/09/08 15:06:48, mef wrote:
> if (proxy_delegate_) 

If this passed tests, we should probably have a test where there's no proxy
delegate.

https://codereview.chromium.org/517693002/diff/160001/net/http/http_proxy_cli...
net/http/http_proxy_client_socket.cc:460: *(response_.headers));
nit:  Parentheses not needed.

https://codereview.chromium.org/517693002/diff/160001/net/http/http_proxy_cli...
File net/http/http_proxy_client_socket_pool_unittest.cc (right):

https://codereview.chromium.org/517693002/diff/160001/net/http/http_proxy_cli...
net/http/http_proxy_client_socket_pool_unittest.cc:236:
on_before_tunnel_request_called_ = true;
Should check this actually works (Maybe add a header with "foo:
<proxy_server_as_string>, and make sure the right value appears in the request?)

https://codereview.chromium.org/517693002/diff/160001/net/http/http_proxy_cli...
net/http/http_proxy_client_socket_pool_unittest.cc:242: const
net::HttpResponseHeaders& response_headers) OVERRIDE {
Again, should validate these some way.

https://codereview.chromium.org/517693002/diff/160001/net/http/http_proxy_cli...
net/http/http_proxy_client_socket_pool_unittest.cc:250: bool
on_before_tunnel_headers_received_called() {
These two should be const

[mmenke, 2014-09-08 17:39:29]

Oh, and I'm going to defer to Misha on this review, these are just issues I
noticed as I glanced over the CL.

On 2014/09/08 17:38:50, mmenke wrote:
> Quick comments.  I think this approach looks fine.
> 
>
https://codereview.chromium.org/517693002/diff/160001/net/base/proxy_delegate.h
> File net/base/proxy_delegate.h (right):
> 
>
https://codereview.chromium.org/517693002/diff/160001/net/base/proxy_delegate...
> net/base/proxy_delegate.h:9: #include "url/gurl.h"
> Can forward declare this.
> 
>
https://codereview.chromium.org/517693002/diff/160001/net/base/proxy_delegate...
> net/base/proxy_delegate.h:55: const HttpResponseHeaders& response_headers) =
0;
> private:
> 
> DISALLOW_COPY_AND_ASSIGN?  (+ include macros.h for it)
> 
>
https://codereview.chromium.org/517693002/diff/160001/net/http/http_network_s...
> File net/http/http_network_session.h (right):
> 
>
https://codereview.chromium.org/517693002/diff/160001/net/http/http_network_s...
> net/http/http_network_session.h:124: net::ProxyDelegate* proxy_delegate;
> nit:  net:: not needed.
> 
>
https://codereview.chromium.org/517693002/diff/160001/net/http/http_network_t...
> File net/http/http_network_transaction_unittest.cc (right):
> 
>
https://codereview.chromium.org/517693002/diff/160001/net/http/http_network_t...
> net/http/http_network_transaction_unittest.cc:569: 0, 0, NULL, host_resolver,
> NULL, NULL, NULL, NULL) {}
> nit:  +2 indent
> 
>
https://codereview.chromium.org/517693002/diff/160001/net/http/http_proxy_cli...
> File net/http/http_proxy_client_socket.cc (right):
> 
>
https://codereview.chromium.org/517693002/diff/160001/net/http/http_proxy_cli...
> net/http/http_proxy_client_socket.cc:410: if (proxy_delegate_) {
> I think it makes more sense to move this below the
> "auth_->AddAuthorizationHeader()" call.  That lets the delegate decide whether
> it wants to override auth headers added by the network stack or not.
> 
>
https://codereview.chromium.org/517693002/diff/160001/net/http/http_proxy_cli...
> net/http/http_proxy_client_socket.cc:412: &authorization_headers);
> Should have a couple tests for this.  Don't believe I saw one.  (Setting a
> header when there wasn't one before, overriding the ones set by auth_)
> 
>
https://codereview.chromium.org/517693002/diff/160001/net/http/http_proxy_cli...
> net/http/http_proxy_client_socket.cc:458:
> proxy_delegate_->OnTunnelHeadersReceived(HostPortPair::FromURL(request_.url),
> On 2014/09/08 15:06:48, mef wrote:
> > if (proxy_delegate_) 
> 
> If this passed tests, we should probably have a test where there's no proxy
> delegate.
> 
>
https://codereview.chromium.org/517693002/diff/160001/net/http/http_proxy_cli...
> net/http/http_proxy_client_socket.cc:460: *(response_.headers));
> nit:  Parentheses not needed.
> 
>
https://codereview.chromium.org/517693002/diff/160001/net/http/http_proxy_cli...
> File net/http/http_proxy_client_socket_pool_unittest.cc (right):
> 
>
https://codereview.chromium.org/517693002/diff/160001/net/http/http_proxy_cli...
> net/http/http_proxy_client_socket_pool_unittest.cc:236:
> on_before_tunnel_request_called_ = true;
> Should check this actually works (Maybe add a header with "foo:
> <proxy_server_as_string>, and make sure the right value appears in the
request?)
> 
>
https://codereview.chromium.org/517693002/diff/160001/net/http/http_proxy_cli...
> net/http/http_proxy_client_socket_pool_unittest.cc:242: const
> net::HttpResponseHeaders& response_headers) OVERRIDE {
> Again, should validate these some way.
> 
>
https://codereview.chromium.org/517693002/diff/160001/net/http/http_proxy_cli...
> net/http/http_proxy_client_socket_pool_unittest.cc:250: bool
> on_before_tunnel_headers_received_called() {
> These two should be const

[bengr, 2014-09-09 01:28:49]

https://codereview.chromium.org/517693002/diff/160001/net/base/proxy_delegate.h
File net/base/proxy_delegate.h (right):

https://codereview.chromium.org/517693002/diff/160001/net/base/proxy_delegate...
net/base/proxy_delegate.h:9: #include "url/gurl.h"
On 2014/09/08 17:38:49, mmenke wrote:
> Can forward declare this.

Done.

https://codereview.chromium.org/517693002/diff/160001/net/base/proxy_delegate...
net/base/proxy_delegate.h:39: // Called after a proxy connection. Allows the
delegate to read/write
On 2014/09/08 15:06:48, mef wrote:
> nit: indent

Done.

https://codereview.chromium.org/517693002/diff/160001/net/base/proxy_delegate...
net/base/proxy_delegate.h:55: const HttpResponseHeaders& response_headers) = 0;
On 2014/09/08 17:38:49, mmenke wrote:
> private:
> 
> DISALLOW_COPY_AND_ASSIGN?  (+ include macros.h for it)

Done.

https://codereview.chromium.org/517693002/diff/160001/net/http/http_network_s...
File net/http/http_network_session.h (right):

https://codereview.chromium.org/517693002/diff/160001/net/http/http_network_s...
net/http/http_network_session.h:124: net::ProxyDelegate* proxy_delegate;
On 2014/09/08 17:38:49, mmenke wrote:
> nit:  net:: not needed.

Done.

https://codereview.chromium.org/517693002/diff/160001/net/http/http_network_t...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/517693002/diff/160001/net/http/http_network_t...
net/http/http_network_transaction_unittest.cc:569: 0, 0, NULL, host_resolver,
NULL, NULL, NULL, NULL) {}
On 2014/09/08 17:38:50, mmenke wrote:
> nit:  +2 indent

Done.

https://codereview.chromium.org/517693002/diff/160001/net/http/http_proxy_cli...
File net/http/http_proxy_client_socket.cc (right):

https://codereview.chromium.org/517693002/diff/160001/net/http/http_proxy_cli...
net/http/http_proxy_client_socket.cc:410: if (proxy_delegate_) {
On 2014/09/08 17:38:50, mmenke wrote:
> I think it makes more sense to move this below the
> "auth_->AddAuthorizationHeader()" call.  That lets the delegate decide whether
> it wants to override auth headers added by the network stack or not.

Done.

https://codereview.chromium.org/517693002/diff/160001/net/http/http_proxy_cli...
net/http/http_proxy_client_socket.cc:412: &authorization_headers);
On 2014/09/08 17:38:50, mmenke wrote:
> Should have a couple tests for this.  Don't believe I saw one.  (Setting a
> header when there wasn't one before, overriding the ones set by auth_)

I didn't override auth, but did test that I could add a header, and did test
that a NULL delegate would be handled properly.

https://codereview.chromium.org/517693002/diff/160001/net/http/http_proxy_cli...
net/http/http_proxy_client_socket.cc:458:
proxy_delegate_->OnTunnelHeadersReceived(HostPortPair::FromURL(request_.url),
On 2014/09/08 15:06:48, mef wrote:
> if (proxy_delegate_) 

Done.

https://codereview.chromium.org/517693002/diff/160001/net/http/http_proxy_cli...
net/http/http_proxy_client_socket.cc:458:
proxy_delegate_->OnTunnelHeadersReceived(HostPortPair::FromURL(request_.url),
On 2014/09/08 17:38:50, mmenke wrote:
> On 2014/09/08 15:06:48, mef wrote:
> > if (proxy_delegate_) 
> 
> If this passed tests, we should probably have a test where there's no proxy
> delegate.

Done.

https://codereview.chromium.org/517693002/diff/160001/net/http/http_proxy_cli...
net/http/http_proxy_client_socket.cc:460: *(response_.headers));
On 2014/09/08 17:38:50, mmenke wrote:
> nit:  Parentheses not needed.

Done.

https://codereview.chromium.org/517693002/diff/160001/net/http/http_proxy_cli...
File net/http/http_proxy_client_socket.h (right):

https://codereview.chromium.org/517693002/diff/160001/net/http/http_proxy_cli...
net/http/http_proxy_client_socket.h:168: const HostPortPair proxy_server_;
On 2014/09/08 15:06:48, mef wrote:
> Maybe add comment about ownership of proxy_delegate_?
> Also, should it be const?

Done.

https://codereview.chromium.org/517693002/diff/160001/net/http/http_proxy_cli...
File net/http/http_proxy_client_socket_pool.h (right):

https://codereview.chromium.org/517693002/diff/160001/net/http/http_proxy_cli...
net/http/http_proxy_client_socket_pool.h:96: ProxyDelegate* proxy_delegate_;
On 2014/09/08 15:06:48, mef wrote:
> const

Done.

https://codereview.chromium.org/517693002/diff/160001/net/http/http_proxy_cli...
File net/http/http_proxy_client_socket_pool_unittest.cc (right):

https://codereview.chromium.org/517693002/diff/160001/net/http/http_proxy_cli...
net/http/http_proxy_client_socket_pool_unittest.cc:236:
on_before_tunnel_request_called_ = true;
On 2014/09/08 17:38:50, mmenke wrote:
> Should check this actually works (Maybe add a header with "foo:
> <proxy_server_as_string>, and make sure the right value appears in the
request?)

Done.

https://codereview.chromium.org/517693002/diff/160001/net/http/http_proxy_cli...
net/http/http_proxy_client_socket_pool_unittest.cc:242: const
net::HttpResponseHeaders& response_headers) OVERRIDE {
On 2014/09/08 17:38:50, mmenke wrote:
> Again, should validate these some way.

Done.

https://codereview.chromium.org/517693002/diff/160001/net/http/http_proxy_cli...
net/http/http_proxy_client_socket_pool_unittest.cc:250: bool
on_before_tunnel_headers_received_called() {
On 2014/09/08 17:38:50, mmenke wrote:
> These two should be const

Done.

https://codereview.chromium.org/517693002/diff/160001/net/socket/client_socke...
File net/socket/client_socket_pool_manager_impl.h (right):

https://codereview.chromium.org/517693002/diff/160001/net/socket/client_socke...
net/socket/client_socket_pool_manager_impl.h:152: ProxyDelegate*
proxy_delegate_;
On 2014/09/08 15:06:48, mef wrote:
> const?

Done.

[mef, 2014-09-10 15:05:25]

LGTM, but you'll need mmenke's approval for io_thread.

I'm also a bit confused about fairly random application of "const" for
ProxyDelegate* parameters and member variables. 

I see that different files seem to follow different patterns, but I can't think
of a good reason why.

https://codereview.chromium.org/517693002/diff/180001/net/base/proxy_delegate.h
File net/base/proxy_delegate.h (right):

https://codereview.chromium.org/517693002/diff/180001/net/base/proxy_delegate...
net/base/proxy_delegate.h:26: 
nit: no need for empty line.

https://codereview.chromium.org/517693002/diff/180001/net/http/http_proxy_cli...
File net/http/http_proxy_client_socket_pool_unittest.cc (right):

https://codereview.chromium.org/517693002/diff/180001/net/http/http_proxy_cli...
net/http/http_proxy_client_socket_pool_unittest.cc:291: 
nit: spurious nl.

[mmenke, 2014-09-10 15:18:57]

io_thread LGTM.

[bengr, 2014-09-10 19:19:00]

mef@: I'll take another look at const inconsistencies in the follow-up CL what
wires up the remaining delegate methods.

https://codereview.chromium.org/517693002/diff/180001/net/base/proxy_delegate.h
File net/base/proxy_delegate.h (right):

https://codereview.chromium.org/517693002/diff/180001/net/base/proxy_delegate...
net/base/proxy_delegate.h:26: 
On 2014/09/10 15:05:25, mef wrote:
> nit: no need for empty line.

Done.

https://codereview.chromium.org/517693002/diff/180001/net/http/http_proxy_cli...
File net/http/http_proxy_client_socket_pool_unittest.cc (right):

https://codereview.chromium.org/517693002/diff/180001/net/http/http_proxy_cli...
net/http/http_proxy_client_socket_pool_unittest.cc:291: 
On 2014/09/10 15:05:25, mef wrote:
> nit: spurious nl.

Done.

[bengr, 2014-09-10 21:48:41]

The CQ bit was checked by bengr@chromium.org

[commit-bot: I haz the power, 2014-09-10 21:49:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/bengr@chromium.org/517693002/240001

[commit-bot: I haz the power, 2014-09-10 23:11:10]

Committed patchset #10 (id:240001) as 1a4e99f0360a229582c70756dc67ba114c94ed2b

[commit-bot: I haz the power, 2014-09-10 23:13:52]

Patchset 10 (id:??) landed as
https://crrev.com/39e40610845c395edde654e40fc8d88e466aea81
Cr-Commit-Position: refs/heads/master@{#294252}