Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2798873002:
Network traffic annotation added to update_client. (Closed)
|
Created:
3 years, 8 months ago by Ramin Halavati Modified:
3 years, 7 months ago CC:
chromium-reviews, battre, lazyboy Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionNetwork traffic annotation added to update_client.
Network traffic annotation is added to network requests of
components/update_client/url_fetcher_downloader.cc
components/update_client/utils.cc
BUG=656607
Review-Url: https://codereview.chromium.org/2798873002
Cr-Commit-Position: refs/heads/master@{#473671}
Committed: https://chromium.googlesource.com/chromium/src/+/951a286c80f40d135a074abe2567f30cf9e80067
Patch Set 1 #
Total comments: 28
Patch Set 2 : Annotations updated. #
Total comments: 5
Patch Set 3 : Annotation updated. #
Total comments: 8
Patch Set 4 : Annotations updated. #
Total comments: 1
Messages
Total messages: 34 (10 generated)
rhalavati@chromium.org changed reviewers: + asargent@chromium.org
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 2 files related to update_client and added annotation templates to them. Please review them 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.
asargent@chromium.org changed reviewers: + lazyboy@chromium.org
handing off to lazyboy@ since I'm no longer on chrome team
Hi Ramin, sorry for the late reply, I didn't look close to this: I'm not familiar with update_client at all. Would you be able to pass it through any other update_client OWNERS instead?
rhalavati@chromium.org changed reviewers: + sorin@google.com - asargent@chromium.org, lazyboy@chromium.org
lazybog@, Thanks for the reply. I will try to. sorin@ Could you please review this code?
sorin@chromium.org changed reviewers: + sorin@chromium.org
lgtm thank you!
On 2017/04/07 16:11:52, Sorin Jianu wrote: > lgtm > > thank you! Sorry Sorin, I forgot to repeat the opening message when reviewer changed. We need you to suggest values for all missing parts of this annotation. Parts that are marked either by "..." or separated values by "/". Please refer to the first message or ask me if there are any ambiguities.
Description was changed from ========== Network traffic annotation added to update_client. Network traffic annotation is added to network requests of components/update_client/url_fetcher_downloader.cc components/update_client/utils.cc BUG=656607 ========== to ========== Network traffic annotation added to update_client. Network traffic annotation is added to network requests of components/update_client/url_fetcher_downloader.cc components/update_client/utils.cc BUG=656607 ==========
sorin@chromium.org changed reviewers: - sorin@google.com
I've annotated the fields as needed. Please PTAL. https://codereview.chromium.org/2798873002/diff/1/components/update_client/ur... File components/update_client/url_fetcher_downloader.cc (right): https://codereview.chromium.org/2798873002/diff/1/components/update_client/ur... components/update_client/url_fetcher_downloader.cc:43: sender: "..." Component Updater https://codereview.chromium.org/2798873002/diff/1/components/update_client/ur... components/update_client/url_fetcher_downloader.cc:44: description: "..." The component updater in Chrome is responsible for updating code and data modules such as Flash, CrlSet, Origin Trials, etc. These modules are updated on cycles independent of the Chrome release tracks. It runs in the browser process and communicates with a set of servers using the Omaha protocol to find the latest versions of components, download them, and register them with the rest of Chrome. https://codereview.chromium.org/2798873002/diff/1/components/update_client/ur... components/update_client/url_fetcher_downloader.cc:45: trigger: "..." Manual or automatic software updates. https://codereview.chromium.org/2798873002/diff/1/components/update_client/ur... components/update_client/url_fetcher_downloader.cc:46: data: "..." A CRX representing the specific update payload. https://codereview.chromium.org/2798873002/diff/1/components/update_client/ur... components/update_client/url_fetcher_downloader.cc:47: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER GOOGLE_OWNED_SERVICE https://codereview.chromium.org/2798873002/diff/1/components/update_client/ur... components/update_client/url_fetcher_downloader.cc:51: setting: "..." This feature can't be disabled. https://codereview.chromium.org/2798873002/diff/1/components/update_client/ur... components/update_client/url_fetcher_downloader.cc:53: [POLICY_NAME] { There is no policy associated with this feature. https://codereview.chromium.org/2798873002/diff/1/components/update_client/ut... File components/update_client/utils.cc (right): https://codereview.chromium.org/2798873002/diff/1/components/update_client/ut... components/update_client/utils.cc:195: sender: "..." Component Updater https://codereview.chromium.org/2798873002/diff/1/components/update_client/ut... components/update_client/utils.cc:196: description: "..." The component updater in Chrome is responsible for updating code and data modules such as Flash, CrlSet, Origin Trials, etc. These modules are updated on cycles independent of the Chrome release tracks. It runs in the browser process and communicates with a set of servers using the Omaha protocol to find the latest versions of components, download them, and register them with the rest of Chrome. https://codereview.chromium.org/2798873002/diff/1/components/update_client/ut... components/update_client/utils.cc:197: trigger: "..." Manual or automatic software updates. https://codereview.chromium.org/2798873002/diff/1/components/update_client/ut... components/update_client/utils.cc:198: data: "..." Various OS and Chrome parameters such as version, bitness, release tracks, etc. https://codereview.chromium.org/2798873002/diff/1/components/update_client/ut... components/update_client/utils.cc:199: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER GOOGLE_OWNED_SERVICE https://codereview.chromium.org/2798873002/diff/1/components/update_client/ut... components/update_client/utils.cc:203: setting: "..." This feature can't be disabled. https://codereview.chromium.org/2798873002/diff/1/components/update_client/ut... components/update_client/utils.cc:205: [POLICY_NAME] { There is no policy associated with this feature.
Thank you very much, annotations updated, please review. This is not a problem, but both annotations are very much similar, isn't there any difference between their use cases or triggers? I think the discription is generally saying what the updater does. Can we add something about the specific requests? https://codereview.chromium.org/2798873002/diff/1/components/update_client/ur... File components/update_client/url_fetcher_downloader.cc (right): https://codereview.chromium.org/2798873002/diff/1/components/update_client/ur... components/update_client/url_fetcher_downloader.cc:43: sender: "..." On 2017/04/19 21:40:39, Sorin Jianu wrote: > Component Updater Done. https://codereview.chromium.org/2798873002/diff/1/components/update_client/ur... components/update_client/url_fetcher_downloader.cc:44: description: "..." On 2017/04/19 21:40:39, Sorin Jianu wrote: > The component updater in Chrome is responsible for updating code and data > modules such as Flash, CrlSet, Origin Trials, etc. These modules are updated on > cycles independent of the Chrome release tracks. > It runs in the browser process and communicates with a set of servers > using the Omaha protocol to find the latest versions of components, download > them, and register them with the rest of Chrome. Done. https://codereview.chromium.org/2798873002/diff/1/components/update_client/ur... components/update_client/url_fetcher_downloader.cc:45: trigger: "..." On 2017/04/19 21:40:39, Sorin Jianu wrote: > Manual or automatic software updates. Done. https://codereview.chromium.org/2798873002/diff/1/components/update_client/ur... components/update_client/url_fetcher_downloader.cc:46: data: "..." On 2017/04/19 21:40:39, Sorin Jianu wrote: > A CRX representing the specific update payload. Done. https://codereview.chromium.org/2798873002/diff/1/components/update_client/ur... components/update_client/url_fetcher_downloader.cc:47: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER On 2017/04/19 21:40:39, Sorin Jianu wrote: > GOOGLE_OWNED_SERVICE Done. https://codereview.chromium.org/2798873002/diff/1/components/update_client/ur... components/update_client/url_fetcher_downloader.cc:51: setting: "..." On 2017/04/19 21:40:39, Sorin Jianu wrote: > This feature can't be disabled. Done. https://codereview.chromium.org/2798873002/diff/1/components/update_client/ur... components/update_client/url_fetcher_downloader.cc:53: [POLICY_NAME] { On 2017/04/19 21:40:39, Sorin Jianu wrote: > There is no policy associated with this feature. Done. https://codereview.chromium.org/2798873002/diff/1/components/update_client/ut... File components/update_client/utils.cc (right): https://codereview.chromium.org/2798873002/diff/1/components/update_client/ut... components/update_client/utils.cc:195: sender: "..." On 2017/04/19 21:40:39, Sorin Jianu wrote: > Component Updater Done. https://codereview.chromium.org/2798873002/diff/1/components/update_client/ut... components/update_client/utils.cc:196: description: "..." On 2017/04/19 21:40:39, Sorin Jianu wrote: > The component updater in Chrome is responsible for updating code and data > modules such as Flash, CrlSet, Origin Trials, etc. These modules are updated on > cycles independent of the Chrome release tracks. > It runs in the browser process and communicates with a set of servers > using the Omaha protocol to find the latest versions of components, download > them, and register them with the rest of Chrome. Done. https://codereview.chromium.org/2798873002/diff/1/components/update_client/ut... components/update_client/utils.cc:197: trigger: "..." On 2017/04/19 21:40:39, Sorin Jianu wrote: > Manual or automatic software updates. Done. https://codereview.chromium.org/2798873002/diff/1/components/update_client/ut... components/update_client/utils.cc:198: data: "..." On 2017/04/19 21:40:40, Sorin Jianu wrote: > Various OS and Chrome parameters such as version, bitness, release tracks, etc. Done. https://codereview.chromium.org/2798873002/diff/1/components/update_client/ut... components/update_client/utils.cc:199: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER On 2017/04/19 21:40:39, Sorin Jianu wrote: > GOOGLE_OWNED_SERVICE Done. https://codereview.chromium.org/2798873002/diff/1/components/update_client/ut... components/update_client/utils.cc:203: setting: "..." On 2017/04/19 21:40:39, Sorin Jianu wrote: > This feature can't be disabled. Done. https://codereview.chromium.org/2798873002/diff/1/components/update_client/ut... components/update_client/utils.cc:205: [POLICY_NAME] { On 2017/04/19 21:40:39, Sorin Jianu wrote: > There is no policy associated with this feature. Done.
lgtm Thank you! The difference in annotations occurs in the "data" field. These requests usually happen in pairs: one request retrieves metadata, the other request downloads the corresponding payload.
rhalavati@chromium.org changed reviewers: + msramek@chromium.org
Thank you Sorin, Martin, Any comments?
https://codereview.chromium.org/2798873002/diff/20001/components/update_clien... File components/update_client/url_fetcher_downloader.cc (right): https://codereview.chromium.org/2798873002/diff/20001/components/update_clien... components/update_client/url_fetcher_downloader.cc:53: data: "A CRX representing the specific update payload." Update payload is what is downloaded though, no? This should be about what is uploaded with the request. So I would expect the module name, version, etc.?
thank you! https://codereview.chromium.org/2798873002/diff/20001/components/update_clien... File components/update_client/url_fetcher_downloader.cc (right): https://codereview.chromium.org/2798873002/diff/20001/components/update_clien... components/update_client/url_fetcher_downloader.cc:53: data: "A CRX representing the specific update payload." On 2017/05/04 10:33:14, msramek (recovering) wrote: > Update payload is what is downloaded though, no? This should be about what is That is corrrect, the update payload is downloaded. > uploaded with the request. So I would expect the module name, version, etc.? The request is a GET request for (usually) an obfuscated URL that refers to a CRX.
https://codereview.chromium.org/2798873002/diff/20001/components/update_clien... File components/update_client/url_fetcher_downloader.cc (right): https://codereview.chromium.org/2798873002/diff/20001/components/update_clien... components/update_client/url_fetcher_downloader.cc:53: data: "A CRX representing the specific update payload." On 2017/05/08 16:21:41, Sorin Jianu wrote: > On 2017/05/04 10:33:14, msramek (recovering) wrote: > > Update payload is what is downloaded though, no? This should be about what is > > That is corrrect, the update payload is downloaded. > > > uploaded with the request. So I would expect the module name, version, etc.? > > The request is a GET request for (usually) an obfuscated URL that refers to a > CRX. > > Does 'usually' means it's sometimes something else?
Thank you! https://codereview.chromium.org/2798873002/diff/20001/components/update_clien... File components/update_client/url_fetcher_downloader.cc (right): https://codereview.chromium.org/2798873002/diff/20001/components/update_clien... components/update_client/url_fetcher_downloader.cc:53: data: "A CRX representing the specific update payload." On 2017/05/09 05:32:26, Ramin Halavati wrote: > On 2017/05/08 16:21:41, Sorin Jianu wrote: > > On 2017/05/04 10:33:14, msramek (recovering) wrote: > > > Update payload is what is downloaded though, no? This should be about what > is > > > > That is corrrect, the update payload is downloaded. > > > > > uploaded with the request. So I would expect the module name, version, etc.? > > > > The request is a GET request for (usually) an obfuscated URL that refers to a > > CRX. > > > > > > Does 'usually' means it's sometimes something else? There is no requirement for the the url to be difficult to guess. If it is created by an automated system, the url is obfuscated. But engineers can override this by publishing a non-obfuscated url.
Thank you Sorin. Martin? https://codereview.chromium.org/2798873002/diff/20001/components/update_clien... File components/update_client/url_fetcher_downloader.cc (right): https://codereview.chromium.org/2798873002/diff/20001/components/update_clien... components/update_client/url_fetcher_downloader.cc:53: data: "A CRX representing the specific update payload." On 2017/05/17 16:24:27, Sorin Jianu wrote: > On 2017/05/09 05:32:26, Ramin Halavati wrote: > > On 2017/05/08 16:21:41, Sorin Jianu wrote: > > > On 2017/05/04 10:33:14, msramek (recovering) wrote: > > > > Update payload is what is downloaded though, no? This should be about what > > is > > > > > > That is corrrect, the update payload is downloaded. > > > > > > > uploaded with the request. So I would expect the module name, version, > etc.? > > > > > > The request is a GET request for (usually) an obfuscated URL that refers to > a > > > CRX. > > > > > > > > > > Does 'usually' means it's sometimes something else? > > There is no requirement for the the url to be difficult to guess. If it is > created by an automated system, the url is obfuscated. But engineers can > override this by publishing a non-obfuscated url. Done.
Thanks! I have a few more comments. https://codereview.chromium.org/2798873002/diff/40001/components/update_clien... File components/update_client/url_fetcher_downloader.cc (right): https://codereview.chromium.org/2798873002/diff/40001/components/update_clien... components/update_client/url_fetcher_downloader.cc:54: "The URL that refers to a CRX. It is usually obfuscated, but this " Should we just say "component" instead of "CRX"? I assume that the abbreviation is kinda well known, but still... this is for an external reader. https://codereview.chromium.org/2798873002/diff/40001/components/update_clien... components/update_client/url_fetcher_downloader.cc:55: "may be overriden." I would try to clarify a bit when things are obfuscated and when not. If I understand Sorin's comment correctly, a component is usually published on an obfuscated URL, but for some components we decided to publish them on a non-obfuscated? Then I would say something like "The URL is obfuscated for most components". https://codereview.chromium.org/2798873002/diff/40001/components/update_clien... components/update_client/url_fetcher_downloader.cc:61: policy_exception_justification: "Not implemented." What about the https://www.chromium.org/administrators/policy-list-3#ComponentUpdatesEnabled policy? https://codereview.chromium.org/2798873002/diff/40001/components/update_clien... File components/update_client/utils.cc (right): https://codereview.chromium.org/2798873002/diff/40001/components/update_clien... components/update_client/utils.cc:217: policy_exception_justification: "Not implemented." Ditto here.
Thank you Martin, comments addressed, please review. Sorin, Still LGTM? https://codereview.chromium.org/2798873002/diff/40001/components/update_clien... File components/update_client/url_fetcher_downloader.cc (right): https://codereview.chromium.org/2798873002/diff/40001/components/update_clien... components/update_client/url_fetcher_downloader.cc:54: "The URL that refers to a CRX. It is usually obfuscated, but this " On 2017/05/18 09:45:46, msramek wrote: > Should we just say "component" instead of "CRX"? I assume that the abbreviation > is kinda well known, but still... this is for an external reader. Done. https://codereview.chromium.org/2798873002/diff/40001/components/update_clien... components/update_client/url_fetcher_downloader.cc:54: "The URL that refers to a CRX. It is usually obfuscated, but this " On 2017/05/18 09:45:46, msramek wrote: > Should we just say "component" instead of "CRX"? I assume that the abbreviation > is kinda well known, but still... this is for an external reader. Done. https://codereview.chromium.org/2798873002/diff/40001/components/update_clien... components/update_client/url_fetcher_downloader.cc:61: policy_exception_justification: "Not implemented." On 2017/05/18 09:45:46, msramek wrote: > What about the > https://www.chromium.org/administrators/policy-list-3#ComponentUpdatesEnabled > policy? Done. https://codereview.chromium.org/2798873002/diff/40001/components/update_clien... File components/update_client/utils.cc (right): https://codereview.chromium.org/2798873002/diff/40001/components/update_clien... components/update_client/utils.cc:217: policy_exception_justification: "Not implemented." On 2017/05/18 09:45:46, msramek wrote: > Ditto here. Done.
https://codereview.chromium.org/2798873002/diff/60001/components/update_clien... File components/update_client/url_fetcher_downloader.cc (right): https://codereview.chromium.org/2798873002/diff/60001/components/update_clien... components/update_client/url_fetcher_downloader.cc:60: setting: "This feature cannot be disabled." nit: the protocol buffer comments say to use "NA".
On 2017/05/18 16:13:10, waffles wrote: > https://codereview.chromium.org/2798873002/diff/60001/components/update_clien... > File components/update_client/url_fetcher_downloader.cc (right): > > https://codereview.chromium.org/2798873002/diff/60001/components/update_clien... > components/update_client/url_fetcher_downloader.cc:60: setting: "This feature > cannot be disabled." > nit: the protocol buffer comments say to use "NA". Thank you, but where are you referring to?
On 2017/05/19 04:49:17, Ramin Halavati wrote: > On 2017/05/18 16:13:10, waffles wrote: > > > https://codereview.chromium.org/2798873002/diff/60001/components/update_clien... > > File components/update_client/url_fetcher_downloader.cc (right): > > > > > https://codereview.chromium.org/2798873002/diff/60001/components/update_clien... > > components/update_client/url_fetcher_downloader.cc:60: setting: "This feature > > cannot be disabled." > > nit: the protocol buffer comments say to use "NA". > > Thank you, but where are you referring to? I think waffles@ refers to https://cs.chromium.org/chromium/src/tools/traffic_annotation/traffic_annotat... - and he's right. But we don't follow that in most annotations, so at this point I would just change the documentation.
LGTM.
lgtm Thank you!
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": 60001, "attempt_start_ts": 1495477777506110,
"parent_rev": "9540c6d6f520ad49921f0dc4e6b925324c623dfc", "commit_rev":
"951a286c80f40d135a074abe2567f30cf9e80067"}
Message was sent while issue was closed.
Description was changed from ========== Network traffic annotation added to update_client. Network traffic annotation is added to network requests of components/update_client/url_fetcher_downloader.cc components/update_client/utils.cc BUG=656607 ========== to ========== Network traffic annotation added to update_client. Network traffic annotation is added to network requests of components/update_client/url_fetcher_downloader.cc components/update_client/utils.cc BUG=656607 Review-Url: https://codereview.chromium.org/2798873002 Cr-Commit-Position: refs/heads/master@{#473671} Committed: https://chromium.googlesource.com/chromium/src/+/951a286c80f40d135a074abe2567... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/951a286c80f40d135a074abe2567... |
