Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2849263003:
Network traffic annotation added to SDCH Dictionary Fetcher. (Closed)
|
Created:
3 years, 7 months ago by Ramin Halavati Modified:
3 years, 7 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, net-reviews_chromium.org, battre, asanka Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionNetwork traffic annotation added to SDCH Dictionary Fetcher.
Network traffic annotation is added to network request of:
net/url_request/sdch_dictionary_fetcher.cc
BUG=656607
Review-Url: https://codereview.chromium.org/2849263003
Cr-Commit-Position: refs/heads/master@{#473530}
Committed: https://chromium.googlesource.com/chromium/src/+/2196ca40bd22901c702ed726a1a7ff33e062802c
Patch Set 1 #
Total comments: 19
Patch Set 2 : Annotation updated. #
Total comments: 4
Patch Set 3 : Comments addressed. #Messages
Total messages: 18 (8 generated)
rhalavati@chromium.org changed reviewers: + rdsmith@chromium.org
Hi Randy, I separated the changes in https://codereview.chromium.org/2846873002 and moved annotation of SDCH Dictionary Fetcher to this CL, please review. Here is the introduction again: We are annotating all network requests in Chromium with a new NetworkTrafficAnnotation scheme. This allows enterprise users of Chrome to audit the requests and configure Chrome in a way that meets their security policies and expectations. Furthermore, it allows us to generate better debugging data in chrome://net-internals and measure bandwidth consumption on a per-request-type basis. I've modified one file and added annotation template to it. Please review it and suggest the required answers for the empty parts (marked with '...'). Please note that the claims should be thorough and covering all cases. In case you believe that annotation should be passed to the modified function instead of originating from it, please tell me to change the CL accordingly. Please take a look at the protobuf scheme in: https://cs.chromium.org/chromium/src/tools/traffic_annotation/traffic_annotat... for the definition of the annotations. You can find a sample annotation in: https://cs.chromium.org/chromium/src/components/spellcheck/browser/spelling_s... Entriprise policy options are here: https://cs.chromium.org/chromium/src/out/Debug/gen/components/policy/proto/ch... And more description on enterprise policy settings is here: http://dev.chromium.org/administrators/policy-list-3 Please tell me if you need any clarifications from my side. If you want to learn more, see: https://docs.google.com/document/d/1FWEFTHzdqIA1wHUo_DuSpJjppW22otCMPCOA5NumhJU.
Ramin: I'm not sure if the below is what you're looking for, but if it isn't, let me know :-}. One overarching response: We recently disabled SDCH in Chrome (see https://codereview.chromium.org/2785493003) and have no plans to re-enable it, so it may not be worth a lot of effort to do this perfectly. I'll leave that up to you. https://codereview.chromium.org/2849263003/diff/1/net/url_request/sdch_dictio... File net/url_request/sdch_dictionary_fetcher.cc (right): https://codereview.chromium.org/2849263003/diff/1/net/url_request/sdch_dictio... net/url_request/sdch_dictionary_fetcher.cc:283: net::DefineNetworkTrafficAnnotation("...", R"( I believe from reading the proto and definitions that this is the unique id, and hence should be something recognizable and related to where the traffic comes form, and has no other constraints. So I'd suggest "sdch_dictionary_fetch". https://codereview.chromium.org/2849263003/diff/1/net/url_request/sdch_dictio... net/url_request/sdch_dictionary_fetcher.cc:285: sender: "..." So this is tricky and I'm not sure what to do here. A bit of background: SDCH fetches are triggered when a "Get-Dictionary" request is received from a server in the response to *any* URLRequest. That URLRequest may be renderer initiated, or initiated from the browser, or something else. The dictionary is gotten from the server and stored in memory, advertised on some URLRequests following, and then used to decode the results of those URLRequest responses. So that means: * Any server that is being communicated with by Chrome can cause a dictionary fetch to be initiated. * Follow on requests that that dictionary advertises itself as being used for will be affected by the contents of that dictionary. From a security POV, this means that dictionary fetches can be initiated by arbitrary URLRequests and can affect arbitrary (not really, but it's hard to reason about) future URLRequests. On the other hand, I don't *think* there's a way for them to leak information from the browser; the only information in making a dictionary request is "some earlier URLRequest asked us to fetch this dictionary". So maybe this is ok, or maybe you want to disable SDCH in environments that have concerns about these issues. Pulling back to just this line, I suspect you want the "sender" to reflect the sender of the original URLRequest that triggered this fetch, which would require a lot of plumbing. I can guide you in doing that plumbing if you want. Do note that we recently disabled SDCH in Chrome; it's only enabled for non-chromium embedders now. This may mean that you can put something really scary here and assume it'll never show up in traces; if you want to go that way, let me know. https://codereview.chromium.org/2849263003/diff/1/net/url_request/sdch_dictio... net/url_request/sdch_dictionary_fetcher.cc:286: description: "..." The Chrome Network Stack can use less bandwidth and reduce request latency if a dictionary shared between client and server is used to compress content on the server before sending and decompress content on the client after reception. This request is fetching such a dictionary; it is dispatched when the response to a previous URL request indicated that there was a dictionary the client did not currently have in memory that could be used for compression. https://codereview.chromium.org/2849263003/diff/1/net/url_request/sdch_dictio... net/url_request/sdch_dictionary_fetcher.cc:287: trigger: "..." A response to a previous URL request indicated that this URL contained a dictionary that could be used to compress other URL responses. https://codereview.chromium.org/2849263003/diff/1/net/url_request/sdch_dictio... net/url_request/sdch_dictionary_fetcher.cc:288: data: "..." The server is sending a dictionary of common 'phrases' (may not be textual), references to which can be used in future URL request responses, shrinking response size. https://codereview.chromium.org/2849263003/diff/1/net/url_request/sdch_dictio... net/url_request/sdch_dictionary_fetcher.cc:289: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER/LOCAL I think WEBSITE is the right answer here, but not that it really depends on what that nature of the response that triggered this request is. It'll mostly be websites, but it might be a google owned service as well. https://codereview.chromium.org/2849263003/diff/1/net/url_request/sdch_dictio... net/url_request/sdch_dictionary_fetcher.cc:293: setting: "..." Disabled in chrome. Can only be enabled in non-Chromium embedders.
Thank you much Randy, I updated the annotation, please review. There a few inline questions. https://codereview.chromium.org/2849263003/diff/1/net/url_request/sdch_dictio... File net/url_request/sdch_dictionary_fetcher.cc (right): https://codereview.chromium.org/2849263003/diff/1/net/url_request/sdch_dictio... net/url_request/sdch_dictionary_fetcher.cc:283: net::DefineNetworkTrafficAnnotation("...", R"( On 2017/05/02 15:37:36, Randy Smith (Not in Mondays) wrote: > I believe from reading the proto and definitions that this is the unique id, and > hence should be something recognizable and related to where the traffic comes > form, and has no other constraints. So I'd suggest "sdch_dictionary_fetch". Done. https://codereview.chromium.org/2849263003/diff/1/net/url_request/sdch_dictio... net/url_request/sdch_dictionary_fetcher.cc:285: sender: "..." On 2017/05/02 15:37:35, Randy Smith (Not in Mondays) wrote: > So this is tricky and I'm not sure what to do here. A bit of background: SDCH > fetches are triggered when a "Get-Dictionary" request is received from a server > in the response to *any* URLRequest. That URLRequest may be renderer initiated, > or initiated from the browser, or something else. The dictionary is gotten from > the server and stored in memory, advertised on some URLRequests following, and > then used to decode the results of those URLRequest responses. So that means: > * Any server that is being communicated with by Chrome can cause a dictionary > fetch to be initiated. > > * Follow on requests that that dictionary advertises itself as being used for > will be affected by the contents of that dictionary. > > From a security POV, this means that dictionary fetches can be initiated by > arbitrary URLRequests and can affect arbitrary (not really, but it's hard to > reason about) future URLRequests. On the other hand, I don't *think* there's a > way for them to leak information from the browser; the only information in > making a dictionary request is "some earlier URLRequest asked us to fetch this > dictionary". So maybe this is ok, or maybe you want to disable SDCH in > environments that have concerns about these issues. > > Pulling back to just this line, I suspect you want the "sender" to reflect the > sender of the original URLRequest that triggered this fetch, which would require > a lot of plumbing. I can guide you in doing that plumbing if you want. > > Do note that we recently disabled SDCH in Chrome; it's only enabled for > non-chromium embedders now. This may mean that you can put something really > scary here and assume it'll never show up in traces; if you want to go that way, > let me know. Thank you very much for description, but sender here refers to this module that is actually making the network request. So what do you think about just 'SDCH'? https://codereview.chromium.org/2849263003/diff/1/net/url_request/sdch_dictio... net/url_request/sdch_dictionary_fetcher.cc:286: description: "..." On 2017/05/02 15:37:35, Randy Smith (Not in Mondays) wrote: > The Chrome Network Stack can use less bandwidth and reduce request latency if a > dictionary shared between client and server is used to compress content on the > server before sending and decompress content on the client after reception. > This request is fetching such a dictionary; it is dispatched when the response > to a previous URL request indicated that there was a dictionary the client did > not currently have in memory that could be used for compression. Done. https://codereview.chromium.org/2849263003/diff/1/net/url_request/sdch_dictio... net/url_request/sdch_dictionary_fetcher.cc:287: trigger: "..." On 2017/05/02 15:37:36, Randy Smith (Not in Mondays) wrote: > A response to a previous URL request indicated that this URL contained a > dictionary that could be used to compress other URL responses. Done. https://codereview.chromium.org/2849263003/diff/1/net/url_request/sdch_dictio... net/url_request/sdch_dictionary_fetcher.cc:288: data: "..." On 2017/05/02 15:37:36, Randy Smith (Not in Mondays) wrote: > The server is sending a dictionary of common 'phrases' (may not be textual), > references to which can be used in future URL request responses, shrinking > response size. This field should refer to data that this request sends. Should it be request for website's dictionary? https://codereview.chromium.org/2849263003/diff/1/net/url_request/sdch_dictio... net/url_request/sdch_dictionary_fetcher.cc:289: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER/LOCAL On 2017/05/02 15:37:36, Randy Smith (Not in Mondays) wrote: > I think WEBSITE is the right answer here, but not that it really depends on what > that nature of the response that triggered this request is. It'll mostly be > websites, but it might be a google owned service as well. So my understanding is Google has no role in compressing data on server side and the websites creates the dictionary themselves, am I correct? https://codereview.chromium.org/2849263003/diff/1/net/url_request/sdch_dictio... net/url_request/sdch_dictionary_fetcher.cc:293: setting: "..." On 2017/05/02 15:37:36, Randy Smith (Not in Mondays) wrote: > Disabled in chrome. Can only be enabled in non-Chromium embedders. Done.
LGTM with notes below. https://codereview.chromium.org/2849263003/diff/1/net/url_request/sdch_dictio... File net/url_request/sdch_dictionary_fetcher.cc (right): https://codereview.chromium.org/2849263003/diff/1/net/url_request/sdch_dictio... net/url_request/sdch_dictionary_fetcher.cc:285: sender: "..." On 2017/05/03 05:55:40, Ramin Halavati wrote: > On 2017/05/02 15:37:35, Randy Smith (Not in Mondays) wrote: > > So this is tricky and I'm not sure what to do here. A bit of background: SDCH > > fetches are triggered when a "Get-Dictionary" request is received from a > server > > in the response to *any* URLRequest. That URLRequest may be renderer > initiated, > > or initiated from the browser, or something else. The dictionary is gotten > from > > the server and stored in memory, advertised on some URLRequests following, and > > then used to decode the results of those URLRequest responses. So that means: > > * Any server that is being communicated with by Chrome can cause a dictionary > > fetch to be initiated. > > > > * Follow on requests that that dictionary advertises itself as being used for > > will be affected by the contents of that dictionary. > > > > From a security POV, this means that dictionary fetches can be initiated by > > arbitrary URLRequests and can affect arbitrary (not really, but it's hard to > > reason about) future URLRequests. On the other hand, I don't *think* there's > a > > way for them to leak information from the browser; the only information in > > making a dictionary request is "some earlier URLRequest asked us to fetch this > > dictionary". So maybe this is ok, or maybe you want to disable SDCH in > > environments that have concerns about these issues. > > > > Pulling back to just this line, I suspect you want the "sender" to reflect the > > sender of the original URLRequest that triggered this fetch, which would > require > > a lot of plumbing. I can guide you in doing that plumbing if you want. > > > > Do note that we recently disabled SDCH in Chrome; it's only enabled for > > non-chromium embedders now. This may mean that you can put something really > > scary here and assume it'll never show up in traces; if you want to go that > way, > > let me know. > > Thank you very much for description, but sender here refers to this module that > is actually making the network request. So what do you think about just 'SDCH'? That's fine. https://codereview.chromium.org/2849263003/diff/1/net/url_request/sdch_dictio... net/url_request/sdch_dictionary_fetcher.cc:288: data: "..." On 2017/05/03 05:55:40, Ramin Halavati wrote: > On 2017/05/02 15:37:36, Randy Smith (Not in Mondays) wrote: > > The server is sending a dictionary of common 'phrases' (may not be textual), > > references to which can be used in future URL request responses, shrinking > > response size. > > This field should refer to data that this request sends. > Should it be request for website's dictionary? Ah, sorry, data sent to the server. Yes, the URL of the dictionary, as specified in a previous response from a server. https://codereview.chromium.org/2849263003/diff/1/net/url_request/sdch_dictio... net/url_request/sdch_dictionary_fetcher.cc:289: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER/LOCAL On 2017/05/03 05:55:40, Ramin Halavati wrote: > On 2017/05/02 15:37:36, Randy Smith (Not in Mondays) wrote: > > I think WEBSITE is the right answer here, but not that it really depends on > what > > that nature of the response that triggered this request is. It'll mostly be > > websites, but it might be a google owned service as well. > > So my understanding is Google has no role in compressing data on server side and > the websites creates the dictionary themselves, am I correct? Google is one of the websites that has used SDCH, but not the only one (and we're turning it down, I believe). https://codereview.chromium.org/2849263003/diff/20001/net/url_request/sdch_di... File net/url_request/sdch_dictionary_fetcher.cc (right): https://codereview.chromium.org/2849263003/diff/20001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.cc:287: "The Chrome Network Stack can use less bandwidth and reduce " nit: reduces. (My mistake.) https://codereview.chromium.org/2849263003/diff/20001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.cc:306: "This feature is disabled in Chrome and can obly be enabled in non-" nit: only
rhalavati@chromium.org changed reviewers: + msramek@chromium.org
Thank you Randy. Martin, Any comments? https://codereview.chromium.org/2849263003/diff/1/net/url_request/sdch_dictio... File net/url_request/sdch_dictionary_fetcher.cc (right): https://codereview.chromium.org/2849263003/diff/1/net/url_request/sdch_dictio... net/url_request/sdch_dictionary_fetcher.cc:285: sender: "..." On 2017/05/03 12:58:42, Randy Smith (Not in Mondays) wrote: > On 2017/05/03 05:55:40, Ramin Halavati wrote: > > On 2017/05/02 15:37:35, Randy Smith (Not in Mondays) wrote: > > > So this is tricky and I'm not sure what to do here. A bit of background: > SDCH > > > fetches are triggered when a "Get-Dictionary" request is received from a > > server > > > in the response to *any* URLRequest. That URLRequest may be renderer > > initiated, > > > or initiated from the browser, or something else. The dictionary is gotten > > from > > > the server and stored in memory, advertised on some URLRequests following, > and > > > then used to decode the results of those URLRequest responses. So that > means: > > > * Any server that is being communicated with by Chrome can cause a > dictionary > > > fetch to be initiated. > > > > > > * Follow on requests that that dictionary advertises itself as being used > for > > > will be affected by the contents of that dictionary. > > > > > > From a security POV, this means that dictionary fetches can be initiated by > > > arbitrary URLRequests and can affect arbitrary (not really, but it's hard to > > > reason about) future URLRequests. On the other hand, I don't *think* > there's > > a > > > way for them to leak information from the browser; the only information in > > > making a dictionary request is "some earlier URLRequest asked us to fetch > this > > > dictionary". So maybe this is ok, or maybe you want to disable SDCH in > > > environments that have concerns about these issues. > > > > > > Pulling back to just this line, I suspect you want the "sender" to reflect > the > > > sender of the original URLRequest that triggered this fetch, which would > > require > > > a lot of plumbing. I can guide you in doing that plumbing if you want. > > > > > > Do note that we recently disabled SDCH in Chrome; it's only enabled for > > > non-chromium embedders now. This may mean that you can put something really > > > scary here and assume it'll never show up in traces; if you want to go that > > way, > > > let me know. > > > > Thank you very much for description, but sender here refers to this module > that > > is actually making the network request. So what do you think about just > 'SDCH'? > > That's fine. Done. https://codereview.chromium.org/2849263003/diff/1/net/url_request/sdch_dictio... net/url_request/sdch_dictionary_fetcher.cc:289: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER/LOCAL On 2017/05/03 12:58:42, Randy Smith (Not in Mondays) wrote: > On 2017/05/03 05:55:40, Ramin Halavati wrote: > > On 2017/05/02 15:37:36, Randy Smith (Not in Mondays) wrote: > > > I think WEBSITE is the right answer here, but not that it really depends on > > what > > > that nature of the response that triggered this request is. It'll mostly be > > > websites, but it might be a google owned service as well. > > > > So my understanding is Google has no role in compressing data on server side > and > > the websites creates the dictionary themselves, am I correct? > > Google is one of the websites that has used SDCH, but not the only one (and > we're turning it down, I believe). Thank you. https://codereview.chromium.org/2849263003/diff/20001/net/url_request/sdch_di... File net/url_request/sdch_dictionary_fetcher.cc (right): https://codereview.chromium.org/2849263003/diff/20001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.cc:287: "The Chrome Network Stack can use less bandwidth and reduce " On 2017/05/03 12:58:42, Randy Smith (Not in Mondays) wrote: > nit: reduces. (My mistake.) Done. https://codereview.chromium.org/2849263003/diff/20001/net/url_request/sdch_di... net/url_request/sdch_dictionary_fetcher.cc:306: "This feature is disabled in Chrome and can obly be enabled in non-" On 2017/05/03 12:58:43, Randy Smith (Not in Mondays) wrote: > nit: only Done.
LGTM
The CQ bit was checked by rhalavati@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org Link to the patchset: https://codereview.chromium.org/2849263003/#ps40001 (title: "Comments addressed.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by rhalavati@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1495440532753920,
"parent_rev": "5bcd03be11700eb3bd3755593f5a07530940e789", "commit_rev":
"2196ca40bd22901c702ed726a1a7ff33e062802c"}
Message was sent while issue was closed.
Description was changed from ========== Network traffic annotation added to SDCH Dictionary Fetcher. Network traffic annotation is added to network request of: net/url_request/sdch_dictionary_fetcher.cc BUG=656607 ========== to ========== Network traffic annotation added to SDCH Dictionary Fetcher. Network traffic annotation is added to network request of: net/url_request/sdch_dictionary_fetcher.cc BUG=656607 Review-Url: https://codereview.chromium.org/2849263003 Cr-Commit-Position: refs/heads/master@{#473530} Committed: https://chromium.googlesource.com/chromium/src/+/2196ca40bd22901c702ed726a1a7... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/2196ca40bd22901c702ed726a1a7... |
