Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
920993002:
Added DISABLE_ALTERNATE_PROTOCOLS load flag, and use it for DRP probe. (Closed)
DescriptionAdded DISABLE_ALTERNATE_PROTOCOLS load flag, and use it for DRP probe.
Adds a new load flag DISABLE_ALTERNATE_PROTOCOLS to prevent use of
alternate protocols for a request. This load flag is used to ensure that
the Data Reduction Proxy probe request goes out over vanilla HTTP so
that it can be intercepted by middleboxes.
BUG=437080
Patch Set 1 #
Total comments: 2
Patch Set 2 : Addressed comments and added test #
Total comments: 2
Messages
Total messages: 17 (2 generated)
sclittle@chromium.org changed reviewers: + bengr@chromium.org, rch@chromium.org
bengr: components/data_reduction_proxy/* rch: net/* I don't know where/how this should be unit tested, any advice? I've manually verified that this prevents QUIC from being used for the DRP probe request. Thanks in advance!
It would be great if you could add a test in quic_network_transaction_tests.cc which verifies that if the load_flag is set that QUIC is not used. https://codereview.chromium.org/920993002/diff/1/net/http/http_stream_factory... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/920993002/diff/1/net/http/http_stream_factory... net/http/http_stream_factory_impl.cc:159: (request_info.load_flags & LOAD_DISABLE_ALTERNATE_PROTOCOLS) == 0) { Since this is in two places, could you consider moving this logic down to HttpStreamFactoryImpl::GetAlternateProtocolRequestFor? (Pass in the load_flags as an additional argument to that method).
Added mmenke, because I believe he wants to clean up the load flags.
The concern here is the probe request going over QUIC (Not HTTP/2, since anything intercepting SSL will intercept that as well)?
On 2015/02/13 16:47:46, mmenke wrote: > The concern here is the probe request going over QUIC (Not HTTP/2, since > anything intercepting SSL will intercept that as well)? This isn't a layering violation, unlike a lot of LoadFlags, but I'd still like to find an alternative. Also, if the probe requests ends up establishing a SPDY connection, we'll just reuse that connection instead of trying to use QUIC, I believe, until the connection is closed for whatever reason.
mmenke@chromium.org changed reviewers: + mmenke@chromium.org
On 2015/02/13 16:50:25, mmenke wrote: > > Also, if the probe requests ends up establishing a SPDY connection, we'll just > reuse that connection instead of trying to use QUIC, I believe, until the > connection is closed for whatever reason. I'm not sure if this is important, but the request is to an http:// url, which chrome does not speak SPDY to, fwiw. (But it if it, it would be via Alternate-Protocol support).
On 2015/02/13 17:59:38, Ryan Hamilton wrote: > On 2015/02/13 16:50:25, mmenke wrote: > > > > Also, if the probe requests ends up establishing a SPDY connection, we'll just > > reuse that connection instead of trying to use QUIC, I believe, until the > > connection is closed for whatever reason. > > I'm not sure if this is important, but the request is to an http:// url, which > chrome does not speak SPDY to, fwiw. (But it if it, it would be via > Alternate-Protocol support). Ahh...And we do speak QUIC over HTTP. And yea, I was just realizing we wouldn't be speaking HTTP/2 if we disabled alternate protocols as well. And we're talking directly to the server, not over the proxy, anyways, so we wouldn't try to re-use the socket even if it were HTTPS. So no concerns there about side effects or correctness.
On 2015/02/13 18:05:01, mmenke wrote: > > Ahh...And we do speak QUIC over HTTP. Indeed... For now at least. > And yea, I was just realizing we wouldn't > be speaking HTTP/2 if we disabled alternate protocols as well. If the URL is an https:// url then with this patch we would still speak SPDY or HTTP/2 since that happens via NPN/ALPN negotiation, not via alternate protocol support in HttpServerProperites. > And we're > talking directly to the server, not over the proxy, anyways, so we wouldn't try > to re-use the socket even if it were HTTPS. So no concerns there about side > effects or correctness.
https://codereview.chromium.org/920993002/diff/1/net/http/http_stream_factory... File net/http/http_stream_factory_impl.cc (right): https://codereview.chromium.org/920993002/diff/1/net/http/http_stream_factory... net/http/http_stream_factory_impl.cc:159: (request_info.load_flags & LOAD_DISABLE_ALTERNATE_PROTOCOLS) == 0) { On 2015/02/12 23:48:37, Ryan Hamilton wrote: > Since this is in two places, could you consider moving this logic down to > HttpStreamFactoryImpl::GetAlternateProtocolRequestFor? (Pass in the load_flags > as an additional argument to that method). Done.
https://codereview.chromium.org/920993002/diff/20001/net/base/load_flags_list.h File net/base/load_flags_list.h (right): https://codereview.chromium.org/920993002/diff/20001/net/base/load_flags_list... net/base/load_flags_list.h:106: // SPDY, QUIC). Oh. If this is what the flag is supposed to do then it needs more work. All this flag does is to disable support for the Alternate-Protocol header which is how we announce QUIC. However, SPDY and HTTP/2 are done via NPN/ALPN negotiation in the TLS handshake. It could be possible to disable this too if you wish, but the current CL would need more work. Of course, SPDY is going away in favor of HTTP/2 and HTTP/2 is going to soon be the up-to-date standard version of HTTP so it's not much of an "alternative protocol" in that sense. If you really need "must be unencrypted http/1.1" perhaps the flag should have a different name? https://codereview.chromium.org/920993002/diff/20001/net/quic/quic_network_tr... File net/quic/quic_network_transaction_unittest.cc (right): https://codereview.chromium.org/920993002/diff/20001/net/quic/quic_network_tr... net/quic/quic_network_transaction_unittest.cc:610: SendRequestAndExpectHttpResponse("hello world"); Please add a comment about why this line is duplicated, since the duplication is critical.
On 2015/02/13 19:44:28, Ryan Hamilton wrote: > https://codereview.chromium.org/920993002/diff/20001/net/base/load_flags_list.h > File net/base/load_flags_list.h (right): > > https://codereview.chromium.org/920993002/diff/20001/net/base/load_flags_list... > net/base/load_flags_list.h:106: // SPDY, QUIC). > Oh. If this is what the flag is supposed to do then it needs more work. All this > flag does is to disable support for the Alternate-Protocol header which is how > we announce QUIC. However, SPDY and HTTP/2 are done via NPN/ALPN negotiation in > the TLS handshake. It could be possible to disable this too if you wish, but the > current CL would need more work. Of course, SPDY is going away in favor of > HTTP/2 and HTTP/2 is going to soon be the up-to-date standard version of HTTP so > it's not much of an "alternative protocol" in that sense. If you really need > "must be unencrypted http/1.1" perhaps the flag should have a different name? > > https://codereview.chromium.org/920993002/diff/20001/net/quic/quic_network_tr... > File net/quic/quic_network_transaction_unittest.cc (right): > > https://codereview.chromium.org/920993002/diff/20001/net/quic/quic_network_tr... > net/quic/quic_network_transaction_unittest.cc:610: > SendRequestAndExpectHttpResponse("hello world"); > Please add a comment about why this line is duplicated, since the duplication is > critical. I don't suppose we can just disable HTTP over QUIC support for the flywheel domain, server-side, and only use it for HTTPS?
On 2015/02/17 16:59:25, mmenke wrote: > On 2015/02/13 19:44:28, Ryan Hamilton wrote: > > > https://codereview.chromium.org/920993002/diff/20001/net/base/load_flags_list.h > > File net/base/load_flags_list.h (right): > > > > > https://codereview.chromium.org/920993002/diff/20001/net/base/load_flags_list... > > net/base/load_flags_list.h:106: // SPDY, QUIC). > > Oh. If this is what the flag is supposed to do then it needs more work. All > this > > flag does is to disable support for the Alternate-Protocol header which is how > > we announce QUIC. However, SPDY and HTTP/2 are done via NPN/ALPN negotiation > in > > the TLS handshake. It could be possible to disable this too if you wish, but > the > > current CL would need more work. Of course, SPDY is going away in favor of > > HTTP/2 and HTTP/2 is going to soon be the up-to-date standard version of HTTP > so > > it's not much of an "alternative protocol" in that sense. If you really need > > "must be unencrypted http/1.1" perhaps the flag should have a different name? > > > > > https://codereview.chromium.org/920993002/diff/20001/net/quic/quic_network_tr... > > File net/quic/quic_network_transaction_unittest.cc (right): > > > > > https://codereview.chromium.org/920993002/diff/20001/net/quic/quic_network_tr... > > net/quic/quic_network_transaction_unittest.cc:610: > > SendRequestAndExpectHttpResponse("hello world"); > > Please add a comment about why this line is duplicated, since the duplication > is > > critical. > > I don't suppose we can just disable HTTP over QUIC support for the flywheel > domain, server-side, and only use it for HTTPS? At least I assume that if we're using the proxy over HTTP the reason is so that the ISP can intercept requests, which it probably can't do if we're using QUIC, anyways (It's technically feasible, of course, I just mean the capability is presumably not deployed).
On 2015/02/17 17:01:24, mmenke wrote: > On 2015/02/17 16:59:25, mmenke wrote: > > On 2015/02/13 19:44:28, Ryan Hamilton wrote: > > > > > > https://codereview.chromium.org/920993002/diff/20001/net/base/load_flags_list.h > > > File net/base/load_flags_list.h (right): > > > > > > > > > https://codereview.chromium.org/920993002/diff/20001/net/base/load_flags_list... > > > net/base/load_flags_list.h:106: // SPDY, QUIC). > > > Oh. If this is what the flag is supposed to do then it needs more work. All > > this > > > flag does is to disable support for the Alternate-Protocol header which is > how > > > we announce QUIC. However, SPDY and HTTP/2 are done via NPN/ALPN negotiation > > in > > > the TLS handshake. It could be possible to disable this too if you wish, but > > the > > > current CL would need more work. Of course, SPDY is going away in favor of > > > HTTP/2 and HTTP/2 is going to soon be the up-to-date standard version of > HTTP > > so > > > it's not much of an "alternative protocol" in that sense. If you really need > > > "must be unencrypted http/1.1" perhaps the flag should have a different > name? > > > > > > > > > https://codereview.chromium.org/920993002/diff/20001/net/quic/quic_network_tr... > > > File net/quic/quic_network_transaction_unittest.cc (right): > > > > > > > > > https://codereview.chromium.org/920993002/diff/20001/net/quic/quic_network_tr... > > > net/quic/quic_network_transaction_unittest.cc:610: > > > SendRequestAndExpectHttpResponse("hello world"); > > > Please add a comment about why this line is duplicated, since the > duplication > > is > > > critical. > > > > I don't suppose we can just disable HTTP over QUIC support for the flywheel > > domain, server-side, and only use it for HTTPS? > > At least I assume that if we're using the proxy over HTTP the reason is so that > the ISP can intercept requests, which it probably can't do if we're using QUIC, > anyways (It's technically feasible, of course, I just mean the capability is > presumably not deployed). The probe request does not go through the proxy, it goes over direct. Essentially what we do right now is prevent "http://check.googlezip.net" from advertising that it supports QUIC; this way ISPs and such can intercept that probe. This isn't really a good long-term solution though; see discussion on http://crbug.com/437080.
On 2015/02/17 18:46:45, sclittle wrote: > On 2015/02/17 17:01:24, mmenke wrote: > > On 2015/02/17 16:59:25, mmenke wrote: > > > On 2015/02/13 19:44:28, Ryan Hamilton wrote: > > > > > > > > > > https://codereview.chromium.org/920993002/diff/20001/net/base/load_flags_list.h > > > > File net/base/load_flags_list.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/920993002/diff/20001/net/base/load_flags_list... > > > > net/base/load_flags_list.h:106: // SPDY, QUIC). > > > > Oh. If this is what the flag is supposed to do then it needs more work. > All > > > this > > > > flag does is to disable support for the Alternate-Protocol header which is > > how > > > > we announce QUIC. However, SPDY and HTTP/2 are done via NPN/ALPN > negotiation > > > in > > > > the TLS handshake. It could be possible to disable this too if you wish, > but > > > the > > > > current CL would need more work. Of course, SPDY is going away in favor of > > > > HTTP/2 and HTTP/2 is going to soon be the up-to-date standard version of > > HTTP > > > so > > > > it's not much of an "alternative protocol" in that sense. If you really > need > > > > "must be unencrypted http/1.1" perhaps the flag should have a different > > name? > > > > > > > > > > > > > > https://codereview.chromium.org/920993002/diff/20001/net/quic/quic_network_tr... > > > > File net/quic/quic_network_transaction_unittest.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/920993002/diff/20001/net/quic/quic_network_tr... > > > > net/quic/quic_network_transaction_unittest.cc:610: > > > > SendRequestAndExpectHttpResponse("hello world"); > > > > Please add a comment about why this line is duplicated, since the > > duplication > > > is > > > > critical. > > > > > > I don't suppose we can just disable HTTP over QUIC support for the flywheel > > > domain, server-side, and only use it for HTTPS? > > > > At least I assume that if we're using the proxy over HTTP the reason is so > that > > the ISP can intercept requests, which it probably can't do if we're using > QUIC, > > anyways (It's technically feasible, of course, I just mean the capability is > > presumably not deployed). > > The probe request does not go through the proxy, it goes over direct. > > Essentially what we do right now is prevent "http://check.googlezip.net" from > advertising that it supports QUIC; this way ISPs and such can intercept that > probe. > > This isn't really a good long-term solution though; see discussion on > http://crbug.com/437080. Please close if you are abandoning this CL.
On 2015/03/09 17:50:41, bengr wrote: > On 2015/02/17 18:46:45, sclittle wrote: > > On 2015/02/17 17:01:24, mmenke wrote: > > > On 2015/02/17 16:59:25, mmenke wrote: > > > > On 2015/02/13 19:44:28, Ryan Hamilton wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/920993002/diff/20001/net/base/load_flags_list.h > > > > > File net/base/load_flags_list.h (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/920993002/diff/20001/net/base/load_flags_list... > > > > > net/base/load_flags_list.h:106: // SPDY, QUIC). > > > > > Oh. If this is what the flag is supposed to do then it needs more work. > > All > > > > this > > > > > flag does is to disable support for the Alternate-Protocol header which > is > > > how > > > > > we announce QUIC. However, SPDY and HTTP/2 are done via NPN/ALPN > > negotiation > > > > in > > > > > the TLS handshake. It could be possible to disable this too if you wish, > > but > > > > the > > > > > current CL would need more work. Of course, SPDY is going away in favor > of > > > > > HTTP/2 and HTTP/2 is going to soon be the up-to-date standard version of > > > HTTP > > > > so > > > > > it's not much of an "alternative protocol" in that sense. If you really > > need > > > > > "must be unencrypted http/1.1" perhaps the flag should have a different > > > name? > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/920993002/diff/20001/net/quic/quic_network_tr... > > > > > File net/quic/quic_network_transaction_unittest.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/920993002/diff/20001/net/quic/quic_network_tr... > > > > > net/quic/quic_network_transaction_unittest.cc:610: > > > > > SendRequestAndExpectHttpResponse("hello world"); > > > > > Please add a comment about why this line is duplicated, since the > > > duplication > > > > is > > > > > critical. > > > > > > > > I don't suppose we can just disable HTTP over QUIC support for the > flywheel > > > > domain, server-side, and only use it for HTTPS? > > > > > > At least I assume that if we're using the proxy over HTTP the reason is so > > that > > > the ISP can intercept requests, which it probably can't do if we're using > > QUIC, > > > anyways (It's technically feasible, of course, I just mean the capability is > > > presumably not deployed). > > > > The probe request does not go through the proxy, it goes over direct. > > > > Essentially what we do right now is prevent "http://check.googlezip.net" from > > advertising that it supports QUIC; this way ISPs and such can intercept that > > probe. > > > > This isn't really a good long-term solution though; see discussion on > > http://crbug.com/437080. > > Please close if you are abandoning this CL. I'm abandoning this CL; tbansal@ took over in https://codereview.chromium.org/981633002/. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
