Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
769043003:
Sanitize headers in Proxy Authentication Required responses (Closed)
|
Created:
6 years ago by Deprecated (see juliatuttle) Modified:
5 years, 11 months ago CC:
cbentzel+watch_chromium.org, chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSanitize headers in Proxy Authentication Required responses
BUG=431504
Committed: https://crrev.com/7933c117fd16b192e70609c331641e9112af5e42
Cr-Commit-Position: refs/heads/master@{#310014}
Patch Set 1 #Patch Set 2 : Rearrange some things #
Total comments: 4
Patch Set 3 : Fix unittests #Patch Set 4 : Fix net_unittests #
Total comments: 24
Patch Set 5 : #
Total comments: 2
Patch Set 6 : Don't concatenate multiple Proxy-Auth headers #Patch Set 7 : Make code actually compile :D #Patch Set 8 : Fix Keep-Alive #Patch Set 9 : Fix other unit test cases #Patch Set 10 : Add a couple EXPECTs back #
Total comments: 3
Patch Set 11 : Add test case for sanitizing proxy auth headers #Patch Set 12 : rebase #
Total comments: 5
Patch Set 13 : reformat something #
Total comments: 12
Patch Set 14 : rebase and fix codereview issues #
Total comments: 2
Patch Set 15 : Fix sleevi's nit #
Messages
Total messages: 46 (2 generated)
ttuttle@chromium.org changed reviewers: + asanka@chromium.org, rch@chromium.org, rsleevi@chromium.org
PTAL, folks.
https://codereview.chromium.org/769043003/diff/20001/net/http/proxy_client_so... File net/http/proxy_client_socket.cc (right): https://codereview.chromium.org/769043003/diff/20001/net/http/proxy_client_so... net/http/proxy_client_socket.cc:78: return true; I have trouble convincing myself a blacklist is the right approach here. Why can you not strip out ALL headers except for WWW-Authenticate?
Should we have test coverage for this new behavior? https://codereview.chromium.org/769043003/diff/20001/net/http/proxy_client_so... File net/http/proxy_client_socket.cc (right): https://codereview.chromium.org/769043003/diff/20001/net/http/proxy_client_so... net/http/proxy_client_socket.cc:78: return true; On 2014/12/01 21:28:49, Ryan Sleevi wrote: > I have trouble convincing myself a blacklist is the right approach here. > > Why can you not strip out ALL headers except for WWW-Authenticate? Sounds reasonable! You probably want Proxy-Authenticate, though, right?
Could someone +cc me on the bug? On 2014/12/02 01:50:53, Ryan Hamilton wrote: > Should we have test coverage for this new behavior? +1 > https://codereview.chromium.org/769043003/diff/20001/net/http/proxy_client_so... > File net/http/proxy_client_socket.cc (right): > > https://codereview.chromium.org/769043003/diff/20001/net/http/proxy_client_so... > net/http/proxy_client_socket.cc:78: return true; > On 2014/12/01 21:28:49, Ryan Sleevi wrote: > > I have trouble convincing myself a blacklist is the right approach here. > > > > Why can you not strip out ALL headers except for WWW-Authenticate? > > Sounds reasonable! You probably want Proxy-Authenticate, though, right?
On Tue, Dec 2, 2014 at 11:55 AM, <asanka@chromium.org> wrote: > Could someone +cc me on the bug? > Done To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/769043003/diff/20001/net/http/proxy_client_so... File net/http/proxy_client_socket.cc (right): https://codereview.chromium.org/769043003/diff/20001/net/http/proxy_client_so... net/http/proxy_client_socket.cc:78: return true; On 2014/12/02 01:50:52, Ryan Hamilton wrote: > On 2014/12/01 21:28:49, Ryan Sleevi wrote: > > I have trouble convincing myself a blacklist is the right approach here. > > > > Why can you not strip out ALL headers except for WWW-Authenticate? > > Sounds reasonable! You probably want Proxy-Authenticate, though, right? Yeah, agreed. Is there a way that is more efficient than RemoveHeader-ing each of the non-Proxy-Authenticate headers (which is O(n^2)) and less kludgey than reassembling new fake raw headers with just the Proxy-Authenticate header?
https://codereview.chromium.org/769043003/diff/20001/net/http/proxy_client_so... File net/http/proxy_client_socket.cc (right): https://codereview.chromium.org/769043003/diff/20001/net/http/proxy_client_so... net/http/proxy_client_socket.cc:78: return true; On 2014/12/02 20:10:21, ttuttle wrote: > On 2014/12/02 01:50:52, Ryan Hamilton wrote: > > On 2014/12/01 21:28:49, Ryan Sleevi wrote: > > > I have trouble convincing myself a blacklist is the right approach here. > > > > > > Why can you not strip out ALL headers except for WWW-Authenticate? > > > > Sounds reasonable! You probably want Proxy-Authenticate, though, right? > > Yeah, agreed. > > Is there a way that is more efficient than RemoveHeader-ing each of the > non-Proxy-Authenticate headers (which is O(n^2)) and less kludgey than > reassembling new fake raw headers with just the Proxy-Authenticate header? As far as kludges go, the "assemble new fake raw headers" seems to be pretty simple. Since it's what's done in SanitizeProxyRedirect, that seems like a good approach to me.
I'm good with sanitizing the response headers, but is there also an underlying problem where the proxy tunnel response isn't distinguished from a server response? I feel like sanitization shouldn't be necessary although it wouldn't hurt. I'm not too worried about the proxy + HTTP (i.e. non-tunnel) case.
On 2014/12/02 20:29:01, asanka wrote: > I'm good with sanitizing the response headers, but is there also an underlying > problem where the proxy tunnel response isn't distinguished from a server > response? I feel like sanitization shouldn't be necessary although it wouldn't > hurt. > > I'm not too worried about the proxy + HTTP (i.e. non-tunnel) case. In the non-tunnel case, the proxy totally 0wn the response. It writes the response body and headers. If the proxy is malicious, you're totally screwed and there is nothing that can be done. "Thankfully" HTTP is totally insecure so anyone on your network also totally 0wns your http responses :>
On 2014/12/02 21:16:16, Ryan Hamilton wrote: > On 2014/12/02 20:29:01, asanka wrote: > > I'm good with sanitizing the response headers, but is there also an underlying > > problem where the proxy tunnel response isn't distinguished from a server > > response? I feel like sanitization shouldn't be necessary although it wouldn't > > hurt. > > > > I'm not too worried about the proxy + HTTP (i.e. non-tunnel) case. > > In the non-tunnel case, the proxy totally 0wn the response. It writes the > response body and headers. If the proxy is malicious, you're totally screwed and > there is nothing that can be done. "Thankfully" HTTP is totally insecure so > anyone on your network also totally 0wns your http responses :> Oh, you said "not worried". *facepalm*
On 2014/12/02 21:25:03, Ryan Hamilton wrote: > On 2014/12/02 21:16:16, Ryan Hamilton wrote: > > On 2014/12/02 20:29:01, asanka wrote: > > > I'm good with sanitizing the response headers, but is there also an > underlying > > > problem where the proxy tunnel response isn't distinguished from a server > > > response? I feel like sanitization shouldn't be necessary although it > wouldn't > > > hurt. > > > > > > I'm not too worried about the proxy + HTTP (i.e. non-tunnel) case. > > > > In the non-tunnel case, the proxy totally 0wn the response. It writes the > > response body and headers. If the proxy is malicious, you're totally screwed > and > > there is nothing that can be done. "Thankfully" HTTP is totally insecure so > > anyone on your network also totally 0wns your http responses :> > > Oh, you said "not worried". *facepalm* Everything is beautiful except for one spot in HttpNetworkTransaction that deliberately undermines the separation between the layers. I made a simple attempt at sanitizing the response by creating new raw headers, but a bunch of unit tests are failing because I've mangled things like keep-alive headers and prevented us from displaying the proxy error pages in cases where we expected to see them. Is it okay if I change these for the new expected behavior? Does anyone see any repercussions that I might not?
I am long on the record of thinking that displaying proxy error pages over HTTPS is bad news bears. But I have lost that battle long, long ago. It is a Chrome-specific, non-standard behavior (e.g. FF doesn't do it for their HTTPS proxies), so I say break any of that at will, and make the proxies - which I think are just Google's internal corporate proxies - learn to suck it up and work around it :) (It can totally break things like content encoding, SDCH, etc if the proxy uses these. To that, I say, "too bad"!) On Dec 2, 2014 9:52 PM, <ttuttle@chromium.org> wrote: > On 2014/12/02 21:25:03, Ryan Hamilton wrote: > >> On 2014/12/02 21:16:16, Ryan Hamilton wrote: >> > On 2014/12/02 20:29:01, asanka wrote: >> > > I'm good with sanitizing the response headers, but is there also an >> underlying >> > > problem where the proxy tunnel response isn't distinguished from a >> server >> > > response? I feel like sanitization shouldn't be necessary although it >> wouldn't >> > > hurt. >> > > >> > > I'm not too worried about the proxy + HTTP (i.e. non-tunnel) case. >> > >> > In the non-tunnel case, the proxy totally 0wn the response. It writes >> the >> > response body and headers. If the proxy is malicious, you're totally >> screwed >> and >> > there is nothing that can be done. "Thankfully" HTTP is totally >> insecure so >> > anyone on your network also totally 0wns your http responses :> >> > > Oh, you said "not worried". *facepalm* >> > > Everything is beautiful except for one spot in HttpNetworkTransaction that > deliberately undermines the separation between the layers. > > I made a simple attempt at sanitizing the response by creating new raw > headers, > but a bunch of unit tests are failing because I've mangled things like > keep-alive headers and prevented us from displaying the proxy error pages > in > cases where we expected to see them. Is it okay if I change these for the > new > expected behavior? Does anyone see any repercussions that I might not? > > https://codereview.chromium.org/769043003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Tue, Dec 2, 2014 at 2:00 PM, Ryan Sleevi <rsleevi@chromium.org> wrote: > I am long on the record of thinking that displaying proxy error pages over > HTTPS is bad news bears. But I have lost that battle long, long ago. > I thought that while you may have initially lost that battle that you subsequently won the rematch :> Specifically, we no longer display proxy errors for tunnel requests. We do follow 302s and of course we do handle 407s. But I think that, as this bug suggests, we handle 407s buggily. Do you think we're still displaying proxy error pages in some codepath? > It is a Chrome-specific, non-standard behavior (e.g. FF doesn't do it for > their HTTPS proxies), so I say break any of that at will, and make the > proxies - which I think are just Google's internal corporate proxies - > learn to suck it up and work around it :) > Pretty sure they already did :> > (It can totally break things like content encoding, SDCH, etc if the proxy > uses these. To that, I say, "too bad"!) > Hear! Hear! To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Following up from an IM conversation, I believe this CL from ttutle (2 years ago) removed the "bad news bears" code. https://chromiumcodereview.appspot.com/10825030 If I had noticed the potential problem with 407s, that CL would have been a good time to fix this bug too. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Okay, I've patched the unittests for the new expectations (no response body, and ERR_TUNNEL_CONNECTION_FAILED instead of OK or ERR_PROXY_AUTH_UNSUPPORTED if you try to read the body). PTAL everyone.
I did some RFC confirmation (re: Proxy-Authenticate), but rch@ may be more familiar on the HTTP/1.1 response issue I call out below. Note that asanka@ should check out the comment I left on the unit test. https://codereview.chromium.org/769043003/diff/60001/net/http/http_network_tr... File net/http/http_network_transaction_unittest.cc (left): https://codereview.chromium.org/769043003/diff/60001/net/http/http_network_tr... net/http/http_network_transaction_unittest.cc:2480: // proxy connection, when setting up an SSL tunnel. Is my reading of your deletion of this test correct, in that we no longer use the same proxy connection for multiple auth leg exchanges? https://codereview.chromium.org/769043003/diff/60001/net/http/http_proxy_clie... File net/http/http_proxy_client_socket.cc (right): https://codereview.chromium.org/769043003/diff/60001/net/http/http_proxy_clie... net/http/http_proxy_client_socket.cc:487: // We're not using an HTTPS proxy, or we couldn't sanitize the redirect. nit: "Pronouns in Comments considered harmful" - https://groups.google.com/a/chromium.org/d/topic/chromium-dev/NH-S6KCkr2M/dis... That said, this comment describes the code, which violates http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Implementatio... (That is, the if conditional says exactly what your plain english comment is saying). So delete this comment. https://codereview.chromium.org/769043003/diff/60001/net/http/proxy_client_so... File net/http/proxy_client_socket.cc (right): https://codereview.chromium.org/769043003/diff/60001/net/http/proxy_client_so... net/http/proxy_client_socket.cc:84: // ...but not more than one. BUG: This is NOT required by the spec. Per RFC 7235, Proxy-Authenticate follows the <n><m>#list form (see https://tools.ietf.org/html/rfc7235#section-4.3 ) See also https://tools.ietf.org/html/rfc7230#section-7 and https://tools.ietf.org/html/rfc7230#section-3.2.2 Namely, a sender MAY take a #list and send across multiple headers. And the client MAY coalesce. So the right way to do it is to enumerate all of the header values, join them into a single comma-separated value, and use that as the synthetic Proxy-Authenticate response. https://codereview.chromium.org/769043003/diff/60001/net/http/proxy_client_so... net/http/proxy_client_socket.cc:111: "HTTP/1.1 302 Found\n" Note: While this may seem trivial, it's going to require someone to go back to the HTTP/1.1 spec and ensure our response is a fully conformant 1.1 response. There are difference in requirements for requests (e.g. 1.1 requests MUST have a Host header), and I haven't checked if 1.1 responses have similar requirements. This may be the correct thing to do, but a reviewer needs to vet this is true.
https://codereview.chromium.org/769043003/diff/60001/net/http/http_network_tr... File net/http/http_network_transaction_unittest.cc (left): https://codereview.chromium.org/769043003/diff/60001/net/http/http_network_tr... net/http/http_network_transaction_unittest.cc:2480: // proxy connection, when setting up an SSL tunnel. On 2014/12/08 22:04:39, Ryan Sleevi wrote: > Is my reading of your deletion of this test correct, in that we no longer use > the same proxy connection for multiple auth leg exchanges? ... because that would be Bad (tm). I broke this behavior in the past, I think, and it was Bad(tm) https://codereview.chromium.org/769043003/diff/60001/net/http/http_proxy_clie... File net/http/http_proxy_client_socket.cc (right): https://codereview.chromium.org/769043003/diff/60001/net/http/http_proxy_clie... net/http/http_proxy_client_socket.cc:500: return ERR_HTTPS_PROXY_TUNNEL_RESPONSE; I think you've tried to make use of an early-return of the error here. Yay! I love early-return. But I think you need to nuke the {}s at 492/497 and refrormat? https://codereview.chromium.org/769043003/diff/60001/net/http/proxy_client_so... File net/http/proxy_client_socket.cc (right): https://codereview.chromium.org/769043003/diff/60001/net/http/proxy_client_so... net/http/proxy_client_socket.cc:91: "Content-Length: 0\n" Looks like the content-length should not be 0, since there is actually a response body. https://codereview.chromium.org/769043003/diff/60001/net/http/proxy_client_so... net/http/proxy_client_socket.cc:92: "Connection: close\n" I'm not sure that you want to make this connection close because that prevents the socket from being reused, which I think you need for Negotiate/NTLM auth. https://codereview.chromium.org/769043003/diff/60001/net/spdy/spdy_proxy_clie... File net/spdy/spdy_proxy_client_socket.cc (right): https://codereview.chromium.org/769043003/diff/60001/net/spdy/spdy_proxy_clie... net/spdy/spdy_proxy_client_socket.cc:419: return ERR_TUNNEL_CONNECTION_FAILED; Early return for the win!
https://codereview.chromium.org/769043003/diff/60001/net/http/proxy_client_so... File net/http/proxy_client_socket.cc (right): https://codereview.chromium.org/769043003/diff/60001/net/http/proxy_client_so... net/http/proxy_client_socket.cc:111: "HTTP/1.1 302 Found\n" On 2014/12/08 22:04:40, Ryan Sleevi wrote: > Note: While this may seem trivial, it's going to require someone to go back to > the HTTP/1.1 spec and ensure our response is a fully conformant 1.1 response. > There are difference in requirements for requests (e.g. 1.1 requests MUST have a > Host header), and I haven't checked if 1.1 responses have similar requirements. > > This may be the correct thing to do, but a reviewer needs to vet this is true. From my reading, I think there are no required response headers: Response = Status-Line ; Section 6.1 *(( general-header ; Section 4.5 | response-header ; Section 6.2 | entity-header ) CRLF) ; Section 7.1 CRLF [ message-body ] ; Section 7.2
On 2014/12/08 22:53:48, Ryan Hamilton wrote: > https://codereview.chromium.org/769043003/diff/60001/net/http/proxy_client_so... > File net/http/proxy_client_socket.cc (right): > > https://codereview.chromium.org/769043003/diff/60001/net/http/proxy_client_so... > net/http/proxy_client_socket.cc:111: "HTTP/1.1 302 Found\n" > On 2014/12/08 22:04:40, Ryan Sleevi wrote: > > Note: While this may seem trivial, it's going to require someone to go back to > > the HTTP/1.1 spec and ensure our response is a fully conformant 1.1 response. > > There are difference in requirements for requests (e.g. 1.1 requests MUST have > a > > Host header), and I haven't checked if 1.1 responses have similar > requirements. > > > > This may be the correct thing to do, but a reviewer needs to vet this is true. > > From my reading, I think there are no required response headers: > > Response = Status-Line ; Section 6.1 > *(( general-header ; Section 4.5 > | response-header ; Section 6.2 > | entity-header ) CRLF) ; Section 7.1 > CRLF > [ message-body ] ; Section 7.2 Note: After the 7230-et-al updates, normative requirements are not expressed in the ABNF but prosaically (e.g. https://tools.ietf.org/html/rfc7230#section-5.4 ). That's why it'll require a careful eye :)
PTAL. https://codereview.chromium.org/769043003/diff/60001/net/http/http_network_tr... File net/http/http_network_transaction_unittest.cc (left): https://codereview.chromium.org/769043003/diff/60001/net/http/http_network_tr... net/http/http_network_transaction_unittest.cc:2480: // proxy connection, when setting up an SSL tunnel. On 2014/12/08 22:04:39, Ryan Sleevi wrote: > Is my reading of your deletion of this test correct, in that we no longer use > the same proxy connection for multiple auth leg exchanges? Yes; our fake response does not have a keep-alive header, so we don't. I could try to preserve it, and then the "drain body for auth response" logic would probably still work even if the headers are fake. https://codereview.chromium.org/769043003/diff/60001/net/http/http_proxy_clie... File net/http/http_proxy_client_socket.cc (right): https://codereview.chromium.org/769043003/diff/60001/net/http/http_proxy_clie... net/http/http_proxy_client_socket.cc:487: // We're not using an HTTPS proxy, or we couldn't sanitize the redirect. On 2014/12/08 22:04:39, Ryan Sleevi wrote: > nit: "Pronouns in Comments considered harmful" - > https://groups.google.com/a/chromium.org/d/topic/chromium-dev/NH-S6KCkr2M/dis... > > That said, this comment describes the code, which violates > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Implementatio... > > (That is, the if conditional says exactly what your plain english comment is > saying). So delete this comment. Done. https://codereview.chromium.org/769043003/diff/60001/net/http/http_proxy_clie... net/http/http_proxy_client_socket.cc:500: return ERR_HTTPS_PROXY_TUNNEL_RESPONSE; On 2014/12/08 22:51:59, Ryan Hamilton wrote: > I think you've tried to make use of an early-return of the error here. Yay! I > love early-return. But I think you need to nuke the {}s at 492/497 and > refrormat? What's wrong with the {}s? They're to prevent "jumping past assignment" errors from the switch. https://codereview.chromium.org/769043003/diff/60001/net/http/proxy_client_so... File net/http/proxy_client_socket.cc (right): https://codereview.chromium.org/769043003/diff/60001/net/http/proxy_client_so... net/http/proxy_client_socket.cc:91: "Content-Length: 0\n" On 2014/12/08 22:51:59, Ryan Hamilton wrote: > Looks like the content-length should not be 0, since there is actually a > response body. The goal was to avoid providing the response body, since it can contain scripts that would execute as the origin we were trying to CONNECT to. https://codereview.chromium.org/769043003/diff/60001/net/http/proxy_client_so... net/http/proxy_client_socket.cc:92: "Connection: close\n" On 2014/12/08 22:51:59, Ryan Hamilton wrote: > I'm not sure that you want to make this connection close because that prevents > the socket from being reused, which I think you need for Negotiate/NTLM auth. Sigh, okay. https://codereview.chromium.org/769043003/diff/60001/net/http/proxy_client_so... net/http/proxy_client_socket.cc:111: "HTTP/1.1 302 Found\n" On 2014/12/08 22:04:40, Ryan Sleevi wrote: > Note: While this may seem trivial, it's going to require someone to go back to > the HTTP/1.1 spec and ensure our response is a fully conformant 1.1 response. > There are difference in requirements for requests (e.g. 1.1 requests MUST have a > Host header), and I haven't checked if 1.1 responses have similar requirements. > > This may be the correct thing to do, but a reviewer needs to vet this is true. I can make them both 1.0, but that would break keep-alive, which we need for auth. If you want, I can just leave *this* one 1.0.
https://codereview.chromium.org/769043003/diff/60001/net/http/http_network_tr... File net/http/http_network_transaction_unittest.cc (left): https://codereview.chromium.org/769043003/diff/60001/net/http/http_network_tr... net/http/http_network_transaction_unittest.cc:2480: // proxy connection, when setting up an SSL tunnel. On 2014/12/09 at 15:31:15, ttuttle wrote: > On 2014/12/08 22:04:39, Ryan Sleevi wrote: > > Is my reading of your deletion of this test correct, in that we no longer use > > the same proxy connection for multiple auth leg exchanges? > > Yes; our fake response does not have a keep-alive header, so we don't. I could try to preserve it, and then the "drain body for auth response" logic would probably still work even if the headers are fake. Respecting keep-alive is important for connection based authentication schemes like NTLM / Negotiate. Unlike Basic, servers often maintain per connection state to support multi-leg authentication handshakes and that state would be lost if we use a new socket. https://codereview.chromium.org/769043003/diff/80001/net/http/proxy_client_so... File net/http/proxy_client_socket.cc (right): https://codereview.chromium.org/769043003/diff/80001/net/http/proxy_client_so... net/http/proxy_client_socket.cc:84: auth = auth + ", " + value; Note that we consider proxy-authenticate to be a non-coalescing header (see https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_util...). Unfortunately we can't rely on HTTP auth headers not containing naked commas.
https://codereview.chromium.org/769043003/diff/60001/net/http/proxy_client_so... File net/http/proxy_client_socket.cc (right): https://codereview.chromium.org/769043003/diff/60001/net/http/proxy_client_so... net/http/proxy_client_socket.cc:91: "Content-Length: 0\n" On 2014/12/09 15:31:15, ttuttle wrote: > On 2014/12/08 22:51:59, Ryan Hamilton wrote: > > Looks like the content-length should not be 0, since there is actually a > > response body. > > The goal was to avoid providing the response body, since it can contain scripts > that would execute as the origin we were trying to CONNECT to. Oh! I'm sorry. I misread line 94 to be appending a response body via auth.c_str() but in re-reading it's obvious that I was confused. Sorry!
Okay, I think Keep-Alive works now. https://codereview.chromium.org/769043003/diff/80001/net/http/proxy_client_so... File net/http/proxy_client_socket.cc (right): https://codereview.chromium.org/769043003/diff/80001/net/http/proxy_client_so... net/http/proxy_client_socket.cc:84: auth = auth + ", " + value; On 2014/12/09 17:33:02, asanka wrote: > Note that we consider proxy-authenticate to be a non-coalescing header (see > https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_util...). > Unfortunately we can't rely on HTTP auth headers not containing naked commas. Oh, alright. I will reproduce each header as a separate one in the fake headers then.
PTAL, folks. https://codereview.chromium.org/769043003/diff/60001/net/http/http_network_tr... File net/http/http_network_transaction_unittest.cc (left): https://codereview.chromium.org/769043003/diff/60001/net/http/http_network_tr... net/http/http_network_transaction_unittest.cc:2480: // proxy connection, when setting up an SSL tunnel. On 2014/12/09 17:33:02, asanka (gone till 12-22) wrote: > On 2014/12/09 at 15:31:15, ttuttle wrote: > > On 2014/12/08 22:04:39, Ryan Sleevi wrote: > > > Is my reading of your deletion of this test correct, in that we no longer > use > > > the same proxy connection for multiple auth leg exchanges? > > > > Yes; our fake response does not have a keep-alive header, so we don't. I could > try to preserve it, and then the "drain body for auth response" logic would > probably still work even if the headers are fake. > > Respecting keep-alive is important for connection based authentication schemes > like NTLM / Negotiate. Unlike Basic, servers often maintain per connection state > to support multi-leg authentication handshakes and that state would be lost if > we use a new socket. Gross, but fixed. https://codereview.chromium.org/769043003/diff/60001/net/http/proxy_client_so... File net/http/proxy_client_socket.cc (right): https://codereview.chromium.org/769043003/diff/60001/net/http/proxy_client_so... net/http/proxy_client_socket.cc:91: "Content-Length: 0\n" On 2014/12/10 19:53:42, Ryan Hamilton wrote: > On 2014/12/09 15:31:15, ttuttle wrote: > > On 2014/12/08 22:51:59, Ryan Hamilton wrote: > > > Looks like the content-length should not be 0, since there is actually a > > > response body. > > > > The goal was to avoid providing the response body, since it can contain > scripts > > that would execute as the origin we were trying to CONNECT to. > > Oh! I'm sorry. I misread line 94 to be appending a response body via > auth.c_str() but in re-reading it's obvious that I was confused. Sorry! I've removed the content-length header entirely, since we error out if the caller reads the body anyway. https://codereview.chromium.org/769043003/diff/60001/net/http/proxy_client_so... net/http/proxy_client_socket.cc:92: "Connection: close\n" On 2014/12/09 15:31:15, ttuttle wrote: > On 2014/12/08 22:51:59, Ryan Hamilton wrote: > > I'm not sure that you want to make this connection close because that prevents > > the socket from being reused, which I think you need for Negotiate/NTLM auth. > > Sigh, okay. I've passed along Connection headers along with the Proxy-Authenticate ones, so keep-alive (or close, if the proxy wants) should work.
https://codereview.chromium.org/769043003/diff/60001/net/http/http_proxy_clie... File net/http/http_proxy_client_socket.cc (right): https://codereview.chromium.org/769043003/diff/60001/net/http/http_proxy_clie... net/http/http_proxy_client_socket.cc:500: return ERR_HTTPS_PROXY_TUNNEL_RESPONSE; On 2014/12/09 15:31:15, ttuttle wrote: > On 2014/12/08 22:51:59, Ryan Hamilton wrote: > > I think you've tried to make use of an early-return of the error here. Yay! I > > love early-return. But I think you need to nuke the {}s at 492/497 and > > refrormat? > > What's wrong with the {}s? They look ugly :> > They're to prevent "jumping past assignment" errors > from the switch. Ugh. I see. In that case, I would suggest one of two approaches: 1) simply inline the call to http_stream_parser_->IsConnectionReused() 2) put the {}s around the entire case 407: block, which I think is more common than simply putting braces around two lines in the middle of a block. https://codereview.chromium.org/769043003/diff/180001/net/http/proxy_client_s... File net/http/proxy_client_socket.cc (right): https://codereview.chromium.org/769043003/diff/180001/net/http/proxy_client_s... net/http/proxy_client_socket.cc:105: I'm curious about the keep-alive behavior in the face of non-zero content-length in the response. Perhaps this issue never comes up. But how do we ensure that we drain the socket when the proxy replies back with a response body? If we re-write the content-lenght, I think that we'll assume there is no more to read. Then when we send the next request, we'll read the body of the previous response as the headers for this new response. Is that a problem?
https://codereview.chromium.org/769043003/diff/180001/net/http/proxy_client_s... File net/http/proxy_client_socket.cc (right): https://codereview.chromium.org/769043003/diff/180001/net/http/proxy_client_s... net/http/proxy_client_socket.cc:105: On 2014/12/10 22:09:21, Ryan Hamilton wrote: > I'm curious about the keep-alive behavior in the face of non-zero content-length > in the response. Perhaps this issue never comes up. But how do we ensure that we > drain the socket when the proxy replies back with a response body? If we > re-write the content-lenght, I think that we'll assume there is no more to read. > Then when we send the next request, we'll read the body of the previous response > as the headers for this new response. > > Is that a problem? Good catch! The answer is minimally, ttuttle should write a unit test for that :D I reached the same conclusion you did, and I think it will indeed be a problem. I think our IsConnectedAndIdle bit that tries to peek the socket will fail, and so we'd end up not doing Keep-Alive on the socket. https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_prox... has the logic, and I think it'd just cause us to close the socket rather than drain the socket.
https://codereview.chromium.org/769043003/diff/180001/net/http/proxy_client_s... File net/http/proxy_client_socket.cc (right): https://codereview.chromium.org/769043003/diff/180001/net/http/proxy_client_s... net/http/proxy_client_socket.cc:105: On 2014/12/10 22:57:01, Ryan Sleevi wrote: > On 2014/12/10 22:09:21, Ryan Hamilton wrote: > > I'm curious about the keep-alive behavior in the face of non-zero > content-length > > in the response. Perhaps this issue never comes up. But how do we ensure that > we > > drain the socket when the proxy replies back with a response body? If we > > re-write the content-lenght, I think that we'll assume there is no more to > read. > > Then when we send the next request, we'll read the body of the previous > response > > as the headers for this new response. > > > > Is that a problem? > > Good catch! > > The answer is minimally, ttuttle should write a unit test for that :D > > I reached the same conclusion you did, and I think it will indeed be a problem. > I think our IsConnectedAndIdle bit that tries to peek the socket will fail, and > so we'd end up not doing Keep-Alive on the socket. > > https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_prox... > has the logic, and I think it'd just cause us to close the socket rather than > drain the socket. Huh. I am confused at why the BasicAuthProxyKeepAlive test is still passing, then.
PTAL, everyone. On 2014/12/11 00:20:04, ttuttle wrote: > https://codereview.chromium.org/769043003/diff/180001/net/http/proxy_client_s... > File net/http/proxy_client_socket.cc (right): > > https://codereview.chromium.org/769043003/diff/180001/net/http/proxy_client_s... > net/http/proxy_client_socket.cc:105: > On 2014/12/10 22:57:01, Ryan Sleevi wrote: > > On 2014/12/10 22:09:21, Ryan Hamilton wrote: > > > I'm curious about the keep-alive behavior in the face of non-zero > > content-length > > > in the response. Perhaps this issue never comes up. But how do we ensure > that > > we > > > drain the socket when the proxy replies back with a response body? If we > > > re-write the content-lenght, I think that we'll assume there is no more to > > read. > > > Then when we send the next request, we'll read the body of the previous > > response > > > as the headers for this new response. > > > > > > Is that a problem? > > > > Good catch! > > > > The answer is minimally, ttuttle should write a unit test for that :D > > > > I reached the same conclusion you did, and I think it will indeed be a > problem. > > I think our IsConnectedAndIdle bit that tries to peek the socket will fail, > and > > so we'd end up not doing Keep-Alive on the socket. > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_prox... > > has the logic, and I think it'd just cause us to close the socket rather than > > drain the socket. > > Huh. I am confused at why the BasicAuthProxyKeepAlive test is still passing, > then. The content length is also stored in the HttpStreamParser, which remembers it even if we modify the headers shown to the client of the HttpNetworkTransaction. I think we're good here.
I'm still shaky on the testing here. Why aren't you testing for a hypothetical multi-legged auth? We should make sure it works over a single connection still. I don't see any test covering that? https://codereview.chromium.org/769043003/diff/220001/net/http/http_network_t... File net/http/http_network_transaction_unittest.cc (right): https://codereview.chromium.org/769043003/diff/220001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:2669: MockRead(SYNCHRONOUS, ERR_UNEXPECTED), // Should not be reached. Wait, why shouldn't it be reached? We should drain the socket and then send the new request, right?
Hmm. I added the existing keep-alive test back, I thought? I guess that only covers multiple attempts at auth, not multi-*legged* auth? Are they measurably different? I'll take a look at it tomorrow. On Wed, Dec 17, 2014 at 5:37 PM, <rsleevi@chromium.org> wrote: > I'm still shaky on the testing here. > > Why aren't you testing for a hypothetical multi-legged auth? We should > make sure > it works over a single connection still. I don't see any test covering > that? > > > https://codereview.chromium.org/769043003/diff/220001/net/ > http/http_network_transaction_unittest.cc > File net/http/http_network_transaction_unittest.cc (right): > > https://codereview.chromium.org/769043003/diff/220001/net/ > http/http_network_transaction_unittest.cc#newcode2669 > net/http/http_network_transaction_unittest.cc:2669: > MockRead(SYNCHRONOUS, ERR_UNEXPECTED), // Should not be reached. > Wait, why shouldn't it be reached? We should drain the socket and then > send the new request, right? > > https://codereview.chromium.org/769043003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/12/17 19:02:30, ttuttle wrote: > The content length is also stored in the HttpStreamParser, which remembers it > even if we modify the headers shown to the client of the HttpNetworkTransaction. > I think we're good here. I don't quite understand how this works, though you may well be right that it does :> I'm looking at code in HttpNetworkTransaction::PrepareForAuthRestart. It looks like it's checking the headers to see if it's keep-alive but then is asking the stream if it can find the end of the response. Is there any possibility that those will disagree? If so, is that problematic?
https://codereview.chromium.org/769043003/diff/60001/net/http/http_proxy_clie... File net/http/http_proxy_client_socket.cc (right): https://codereview.chromium.org/769043003/diff/60001/net/http/http_proxy_clie... net/http/http_proxy_client_socket.cc:500: return ERR_HTTPS_PROXY_TUNNEL_RESPONSE; On 2014/12/10 22:09:21, Ryan Hamilton wrote: > On 2014/12/09 15:31:15, ttuttle wrote: > > On 2014/12/08 22:51:59, Ryan Hamilton wrote: > > > I think you've tried to make use of an early-return of the error here. Yay! > I > > > love early-return. But I think you need to nuke the {}s at 492/497 and > > > refrormat? > > > > What's wrong with the {}s? > > They look ugly :> > > > They're to prevent "jumping past assignment" errors > > from the switch. > > Ugh. I see. In that case, I would suggest one of two approaches: > 1) simply inline the call to http_stream_parser_->IsConnectionReused() > 2) put the {}s around the entire case 407: block, which I think is more common > than simply putting braces around two lines in the middle of a block. ping. https://codereview.chromium.org/769043003/diff/220001/net/http/proxy_client_s... File net/http/proxy_client_socket.cc (right): https://codereview.chromium.org/769043003/diff/220001/net/http/proxy_client_s... net/http/proxy_client_socket.cc:99: CopyHeaderValues(old_headers, new_headers, "Connection"); I sure would have thought that you'd need both Connection and Content-length (or a synthetic C-L). Do you understand why Content-Length is not required?
https://codereview.chromium.org/769043003/diff/220001/net/http/proxy_client_s... File net/http/proxy_client_socket.cc (right): https://codereview.chromium.org/769043003/diff/220001/net/http/proxy_client_s... net/http/proxy_client_socket.cc:99: CopyHeaderValues(old_headers, new_headers, "Connection"); On 2014/12/19 21:26:59, Ryan Hamilton wrote: > I sure would have thought that you'd need both Connection and Content-length (or > a synthetic C-L). Do you understand why Content-Length is not required? Agreed. This is why I have trouble understanding how this doesn't break something. If a proxy sends a response along with the initial body, and we're going to re-use that connection, we need the presence of Keep-Alive (either implicit or explicit) and the ability to drain the body. I don't think the tests send dummy bodies, and so it falls under the "IsConnectedAndIdle" check and decides it doesn't need to drain the body. But we should.
On 2014/12/19 21:30:25, Ryan Sleevi wrote: > https://codereview.chromium.org/769043003/diff/220001/net/http/proxy_client_s... > File net/http/proxy_client_socket.cc (right): > > https://codereview.chromium.org/769043003/diff/220001/net/http/proxy_client_s... > net/http/proxy_client_socket.cc:99: CopyHeaderValues(old_headers, new_headers, > "Connection"); > On 2014/12/19 21:26:59, Ryan Hamilton wrote: > > I sure would have thought that you'd need both Connection and Content-length > (or > > a synthetic C-L). Do you understand why Content-Length is not required? > > Agreed. This is why I have trouble understanding how this doesn't break > something. > > If a proxy sends a response along with the initial body, and we're going to > re-use that connection, we need the presence of Keep-Alive (either implicit or > explicit) and the ability to drain the body. We do preserve the Keep-Alive header over now. Content-Length isn't required because it's saved in the HttpStreamParser, which knows how to read to the end of the body. > I don't think the tests send dummy bodies, and so it falls under the > "IsConnectedAndIdle" check and decides it doesn't need to drain the body. But we > should. I thought they did. BasicAuthProxyKeepAlive returns "0123456789" as the body for the first failure, for example.
PTAL, rch and rsleevi. https://codereview.chromium.org/769043003/diff/60001/net/http/http_proxy_clie... File net/http/http_proxy_client_socket.cc (right): https://codereview.chromium.org/769043003/diff/60001/net/http/http_proxy_clie... net/http/http_proxy_client_socket.cc:500: return ERR_HTTPS_PROXY_TUNNEL_RESPONSE; On 2014/12/19 21:26:58, Ryan Hamilton wrote: > On 2014/12/10 22:09:21, Ryan Hamilton wrote: > > On 2014/12/09 15:31:15, ttuttle wrote: > > > On 2014/12/08 22:51:59, Ryan Hamilton wrote: > > > > I think you've tried to make use of an early-return of the error here. > Yay! > > I > > > > love early-return. But I think you need to nuke the {}s at 492/497 and > > > > refrormat? > > > > > > What's wrong with the {}s? > > > > They look ugly :> > > > > > They're to prevent "jumping past assignment" errors > > > from the switch. > > > > Ugh. I see. In that case, I would suggest one of two approaches: > > 1) simply inline the call to http_stream_parser_->IsConnectionReused() > > 2) put the {}s around the entire case 407: block, which I think is more common > > than simply putting braces around two lines in the middle of a block. > > ping. Done. https://codereview.chromium.org/769043003/diff/220001/net/http/proxy_client_s... File net/http/proxy_client_socket.cc (right): https://codereview.chromium.org/769043003/diff/220001/net/http/proxy_client_s... net/http/proxy_client_socket.cc:99: CopyHeaderValues(old_headers, new_headers, "Connection"); On 2014/12/19 21:26:59, Ryan Hamilton wrote: > I sure would have thought that you'd need both Connection and Content-length (or > a synthetic C-L). Do you understand why Content-Length is not required? Content-Length is read by the HttpStreamParser. We never directly examine it in HttpProxyClientSocket.
I think this LGTM. Sleevi? https://codereview.chromium.org/769043003/diff/220001/net/http/proxy_client_s... File net/http/proxy_client_socket.cc (right): https://codereview.chromium.org/769043003/diff/220001/net/http/proxy_client_s... net/http/proxy_client_socket.cc:99: CopyHeaderValues(old_headers, new_headers, "Connection"); On 2014/12/19 21:50:46, ttuttle wrote: > On 2014/12/19 21:26:59, Ryan Hamilton wrote: > > I sure would have thought that you'd need both Connection and Content-length > (or > > a synthetic C-L). Do you understand why Content-Length is not required? > > Content-Length is read by the HttpStreamParser. We never directly examine it in > HttpProxyClientSocket. Ah, I missed the last part. Ok, makes sense.
https://codereview.chromium.org/769043003/diff/240001/net/http/http_network_t... File net/http/http_network_transaction_unittest.cc (left): https://codereview.chromium.org/769043003/diff/240001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:2553: EXPECT_EQ(10, response->headers->GetContentLength()); Rather than deleting these, you should be asserting there is no body, right? https://codereview.chromium.org/769043003/diff/240001/net/http/http_network_t... File net/http/http_network_transaction_unittest.cc (right): https://codereview.chromium.org/769043003/diff/240001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:2664: MockRead("X-Foo: bar\r\n"), Can you explicitly add a test for Set-Cookie behaviour? I agree that it's useful to test X-Foo, but we also want to explicitly test headers to ensure they're not also "special-cased" by HttpStreamParser (such as Content-Length, which tripped up Ryan and I) https://codereview.chromium.org/769043003/diff/240001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:2667: MockRead(SYNCHRONOUS, ERR_UNEXPECTED), // Should not be reached. I'd feel better for this test if you explicitly added content bytes. That is, your "should not be reached" may indirectly cause line 2692 to be reached for reasons unrelated to the 407 on 2663. That is, by adding a body to be read, you can then assert that the response on 2683 *doesn't* include a body, to ensure that the body is stripped out. Does that make sense? https://codereview.chromium.org/769043003/diff/240001/net/http/http_proxy_cli... File net/http/http_proxy_client_socket.cc (right): https://codereview.chromium.org/769043003/diff/240001/net/http/http_proxy_cli... net/http/http_proxy_client_socket.cc:488: return ERR_TUNNEL_CONNECTION_FAILED; What's the content of |response_| in this case, and how do we ensure it isn't used? Should it be cleared? Ditto line 504-506 https://codereview.chromium.org/769043003/diff/240001/net/http/proxy_client_s... File net/http/proxy_client_socket.cc (right): https://codereview.chromium.org/769043003/diff/240001/net/http/proxy_client_s... net/http/proxy_client_socket.cc:94: const char* kHeaders = "HTTP/1.1 407 Proxy Authentication Required\n\n"; const char[] const kHeaders https://codereview.chromium.org/769043003/diff/240001/net/http/proxy_client_s... net/http/proxy_client_socket.cc:96: HttpUtil::AssembleRawHeaders(kHeaders, strlen(kHeaders))); s/strlen/arraysize https://codereview.chromium.org/769043003/diff/240001/net/spdy/spdy_proxy_cli... File net/spdy/spdy_proxy_client_socket.cc (right): https://codereview.chromium.org/769043003/diff/240001/net/spdy/spdy_proxy_cli... net/spdy/spdy_proxy_client_socket.cc:433: return ERR_TUNNEL_CONNECTION_FAILED; Ditto the comments re: |response_| sanitization
PTAL. https://codereview.chromium.org/769043003/diff/240001/net/http/http_network_t... File net/http/http_network_transaction_unittest.cc (left): https://codereview.chromium.org/769043003/diff/240001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:2553: EXPECT_EQ(10, response->headers->GetContentLength()); On 2014/12/19 22:06:25, Ryan Sleevi wrote: > Rather than deleting these, you should be asserting there is no body, right? Well, at least asserting that there is no Content-Length. https://codereview.chromium.org/769043003/diff/240001/net/http/http_network_t... File net/http/http_network_transaction_unittest.cc (right): https://codereview.chromium.org/769043003/diff/240001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:2664: MockRead("X-Foo: bar\r\n"), On 2014/12/19 22:06:25, Ryan Sleevi wrote: > Can you explicitly add a test for Set-Cookie behaviour? I agree that it's useful > to test X-Foo, but we also want to explicitly test headers to ensure they're not > also "special-cased" by HttpStreamParser (such as Content-Length, which tripped > up Ryan and I) Done. I don't see any way to test it besides just adding a Set-Cookie header and checking for it in the response headers, so that's what I did. https://codereview.chromium.org/769043003/diff/240001/net/http/http_proxy_cli... File net/http/http_proxy_client_socket.cc (right): https://codereview.chromium.org/769043003/diff/240001/net/http/http_proxy_cli... net/http/http_proxy_client_socket.cc:488: return ERR_TUNNEL_CONNECTION_FAILED; On 2014/12/19 22:06:25, Ryan Sleevi wrote: > What's the content of |response_| in this case, and how do we ensure it isn't > used? Should it be cleared? Ditto line 504-506 I could delete the response headers, but I *think* the error code should be enough to make sure it isn't used. https://codereview.chromium.org/769043003/diff/240001/net/http/proxy_client_s... File net/http/proxy_client_socket.cc (right): https://codereview.chromium.org/769043003/diff/240001/net/http/proxy_client_s... net/http/proxy_client_socket.cc:94: const char* kHeaders = "HTTP/1.1 407 Proxy Authentication Required\n\n"; On 2014/12/19 22:06:25, Ryan Sleevi wrote: > const char[] const kHeaders The compiler gets rather cranky at this. It wants the [] after kHeaders, but won't let me put a const after that. Does "const char const kHeaders[]" do it, or is that just redundantly making it a not-const array of const chars? (Can you even have a non-const array in this sense?) https://codereview.chromium.org/769043003/diff/240001/net/http/proxy_client_s... net/http/proxy_client_socket.cc:96: HttpUtil::AssembleRawHeaders(kHeaders, strlen(kHeaders))); On 2014/12/19 22:06:25, Ryan Sleevi wrote: > s/strlen/arraysize Done.
lgtm https://codereview.chromium.org/769043003/diff/260001/net/http/http_network_t... File net/http/http_network_transaction_unittest.cc (right): https://codereview.chromium.org/769043003/diff/260001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:2687: ASSERT_TRUE(response != NULL); nullptr although cleaner to just ASSERT_TRUE(response), which will work for any of our container types as well (i.e. it won't require calling .get() and will instead use the bool conversions)
lgtm
https://codereview.chromium.org/769043003/diff/260001/net/http/http_network_t... File net/http/http_network_transaction_unittest.cc (right): https://codereview.chromium.org/769043003/diff/260001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:2687: ASSERT_TRUE(response != NULL); On 2015/01/02 22:56:02, Ryan Sleevi (ooo until 1-6) wrote: > nullptr > > although cleaner to just > > ASSERT_TRUE(response), which will work for any of our container types as well > (i.e. it won't require calling .get() and will instead use the bool conversions) Done.
The CQ bit was checked by ttuttle@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/769043003/280001
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/7933c117fd16b192e70609c331641e9112af5e42 Cr-Commit-Position: refs/heads/master@{#310014} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
