Alternate-Protocol was failing when going through Digest authenticating proxy.

Digest authentication uses a uri field to prevent replay attacks. 

When authenticating to an HTTP proxy to establish a secure tunnel (via CONNECT), the uri should be the hostname of the server and the destination port, such as "www.example.com:443". When authenticating to an HTTP proxy for a non-secure content, the uri should be the path at the server, i.e. "/index.html".

If the site we are trying to connect to previously advertised "Alternate-Protocol: 443:spdy-npn/1" a request to "http://www.example.com" will be attempted on a secure port. 
However, the URL passed into the digest authenticator was an unsecure one, and it decided to have a uri in the form "/index.html" rather than the correct "www.example.com:443". This causes persistent failure with the password and many password prompts.

BUG=49865
TEST=Run with --use-spdy=npn, force connection through a digest authenticating proxy, and browse a site which advertises Alternate-Protocol through http URLs.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=54528

[cbentzel, 2010-07-23 16:51:45]

I just wanted to bring this to your attention - it needs some clean up before a
review (unit tests, combine http->https replacement with PAC query). 

This should probably go into Chrome 6.

[Mike Belshe, 2010-07-23 17:37:59]

LGTM if we add a unit test for it....

http://codereview.chromium.org/3028021/diff/4001/5001
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/3028021/diff/4001/5001#newcode140
net/http/http_network_transaction.cc:140: #if 1
I think we should add a method for "ForceAlternateProtocol()" or
"SimulateAlternateProtocol()" so that tests can use the method and exercise the
code.

http://codereview.chromium.org/3028021/diff/4001/5001#newcode781
net/http/http_network_transaction.cc:781: //                 to combine.
Can you add the example into this comment?
   e.g. The URL is http://www.foo.com/index.html, but we're rewriting to
"https://www.foo.com:443/index.html" ?

[eroman, 2010-07-28 00:23:15]

http://codereview.chromium.org/3028021/diff/9001/10001
File net/http/http_network_transaction.cc (left):

http://codereview.chromium.org/3028021/diff/9001/10001#oldcode657
net/http/http_network_transaction.cc:657: url_canon::Replacements<char>
replacements;
Oh hm, I see this is a move. Well consider simplifying nonetheless per the
comment above.

http://codereview.chromium.org/3028021/diff/9001/10001
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/3028021/diff/9001/10001#newcode196
net/http/http_network_transaction.cc:196: url_canon::Replacements<char>
replacements;
It will be easier if you use GURL::Replacements (which operates on
std::strings):

GURL::Replacements replacements;
replacements.SetScheme("https");
replacements.SetPort(IntToString(443));
return original_url.ReplaceComponents(replacements);

http://codereview.chromium.org/3028021/diff/9001/10001#newcode776
net/http/http_network_transaction.cc:776: GURL proxy_url = request_->url;
I don't like the name "proxy_url".

It sounds too much like "URL for the proxy" whereas really it means "url to send
through the proxy".

http://codereview.chromium.org/3028021/diff/9001/10001#newcode784
net/http/http_network_transaction.cc:784: proxy_url =
UpgradeUrlToHttps(proxy_url);
I don't understand this. Why are we re-writing the URL here? This could be
reached from non npn case no?

[cbentzel, 2010-07-28 18:50:41]

Thanks for the reviews from you.

Eric, I wanted to address this one question you raised, because if my logic is
incorrect then this CL really needs to change.

http://codereview.chromium.org/3028021/diff/9001/10001
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/3028021/diff/9001/10001#newcode784
net/http/http_network_transaction.cc:784: proxy_url =
UpgradeUrlToHttps(proxy_url);
On 2010/07/28 00:23:15, eroman wrote:
> I don't understand this. Why are we re-writing the URL here? This could be
> reached from non npn case no?

The case where this is triggered is:
  HTTP proxy
  SSL will be used for transport
  The original URL's scheme is not https

As far as I know, the only case which matches this criteria is an
Alternate-Protocol upgrade, where the user types in www.google.com but we
decided to attempt an SSL connection anyway. Please let me know if there is a
case I am not thinking of.

The reason the URL rewrite is needed is that Digest authentication schemes
include a uri field in the Proxy-Authorization response which is supposed to
match the uri in the first line of the HTTP request - see
http://tools.ietf.org/html/rfc2617#section-3.2.2

HttpAuthHandleDigest does this in GetRequestMethodAndPath, which uses "CONNECT
<hostname>:<port>" in the case that this is for a proxy and the scheme of the
url is https. However, in the Alternate-Protocol upgrade case, the scheme of the
url is http and it incorrectly generates the request's method and "/index.html"
or equivalent.
 If it's trying to establish a tunnel, the uri should be of the form
"<domain_name>:port", such as "www.google.com:443". If it's doing normally HTTP
proxying, it uses the path at the host. 



This case will get triggered in the Alternate-Protocol upgrade case, rather than
the NPN case. In that case the original URL is "http://www.google.com" but we
decide to go over SSL because of the Alternate-Protocol advertisement.

Let's say there is a digest authenticating proxy in between. Since we have
decided to go over SSL, a CONNECT www.google.com:443 is issued. However,

[cbentzel, 2010-07-28 18:51:51]

Ignore the portion of the comment after "or equivalent." - it was draft comment
which I forgot to erase.

[cbentzel, 2010-07-29 20:56:21]

I modified this CL to match Eric's style complaints, and added a unit test to
exercise it.

The ratio of unit test size to code delta is of course ridiculous.

I'll kick of a try as well.

[vandebo (ex-Chrome), 2010-07-29 21:06:32]

LGTM

http://codereview.chromium.org/3028021/diff/14001/15003
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/3028021/diff/14001/15003#newcode196
net/http/http_network_transaction.cc:196: replacements.SetSchemeStr(new_scheme);
nit: can you not just pass in "https" and "443" directly?

http://codereview.chromium.org/3028021/diff/14001/15004
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/3028021/diff/14001/15004#newcode6348
net/http/http_network_transaction_unittest.cc:6348:
//session_deps.socket_factory.AddSocketDataProvider(&data_4);
extra?

[eroman, 2010-07-29 21:16:21]

http://codereview.chromium.org/3028021/diff/14001/15003
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/3028021/diff/14001/15003#newcode196
net/http/http_network_transaction.cc:196: replacements.SetSchemeStr(new_scheme);
On 2010/07/29 21:06:32, vandebo wrote:
> nit: can you not just pass in "https" and "443" directly?

Oh nuts, looks like having the temps is mandatory (since GURL::Replacements
holds onto a data pointer).

Sorry Chris! I guess my "simplification" suggestion was actually useless.

[eroman, 2010-07-29 21:18:15]

lgtm

[cbentzel, 2010-07-29 21:59:05]

I need to merge with trunk. I'll upload that patch and alert you if there are
any significant changes before landing.

http://codereview.chromium.org/3028021/diff/14001/15003
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/3028021/diff/14001/15003#newcode196
net/http/http_network_transaction.cc:196: replacements.SetSchemeStr(new_scheme);
On 2010/07/29 21:16:21, eroman wrote:
> 
> Sorry Chris! I guess my "simplification" suggestion was actually useless.

I still think it's a tiny bit simpler (don't have to call strlen for example),
so I kept it.

http://codereview.chromium.org/3028021/diff/14001/15004
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/3028021/diff/14001/15004#newcode6348
net/http/http_network_transaction_unittest.cc:6348:
//session_deps.socket_factory.AddSocketDataProvider(&data_4);
On 2010/07/29 21:06:32, vandebo wrote:
> extra?

Yes, thanks for the catch.

[cbentzel, 2010-07-31 11:54:36]

Merged again with trunk, which required a minor change to
http_network_transaction and a bit more of a change to the unit test due to
vandebo's reworking of proxy tunnels. 

It all works, waiting for try's and will actually land this one.