Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2797683002:
Network traffic annotation added to install_signer. (Closed)
|
Created:
3 years, 8 months ago by Ramin Halavati Modified:
3 years, 7 months ago CC:
chromium-apps-reviews_chromium.org, chromium-reviews, extensions-reviews_chromium.org, Sorin Jianu Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionNetwork traffic annotation added to install_signer.
Network traffic annotation is added to network request of:
chrome/browser/extensions/install_signer.cc
BUG=656607
Review-Url: https://codereview.chromium.org/2797683002
Cr-Commit-Position: refs/heads/master@{#472774}
Committed: https://chromium.googlesource.com/chromium/src/+/882e84aeece1a7166cd898354e21008aefabf00e
Patch Set 1 #
Total comments: 12
Patch Set 2 : Annotation updated. #
Total comments: 4
Patch Set 3 : Comment addressed. #Patch Set 4 : Annotation updated. #Messages
Total messages: 38 (13 generated)
rhalavati@chromium.org changed reviewers: + rockot@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 install_signer in extensions 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.
rockot@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Deferring to Devlin as extensions TL. I'm not sure how you want to fill this metadata in or this case. I would also note as a general comment that this seems like an annoying burden to place on every URLFetcher instantiation, and in particular, having these large inline proto strings (rather than a real data structure that can be populated using compiled code) for each one seems quite unfortunate.
On 2017/04/10 21:16:41, Ken Rockot wrote: > Deferring to Devlin as extensions TL. I'm not sure how you want to fill this > metadata in or this case. > > I would also note as a general comment that this seems like an annoying burden > to place on every URLFetcher instantiation, and in particular, having these > large inline proto strings (rather than a real data structure that can be > populated using compiled code) for each one seems quite unfortunate. Thank you Ken, There has been a lot of discussions with network stack team and other related parties on how annotations should be done. The goals were being through, easy to edit, and adding least burden to binary size and execution speed and memory. Here is a brief log of the discussions and I would be glad to discuss any alternative suggestions: https://docs.google.com/document/d/1FWEFTHzdqIA1wHUo_DuSpJjppW22otCMPCOA5NumhJU.
On 2017/04/11 05:22:33, Ramin Halavati wrote: > On 2017/04/10 21:16:41, Ken Rockot wrote: > > Deferring to Devlin as extensions TL. I'm not sure how you want to fill this > > metadata in or this case. > > > > I would also note as a general comment that this seems like an annoying burden > > to place on every URLFetcher instantiation, and in particular, having these > > large inline proto strings (rather than a real data structure that can be > > populated using compiled code) for each one seems quite unfortunate. > > Thank you Ken, > > There has been a lot of discussions with network stack team and other related > parties on how annotations should be done. The goals were being through, easy to > edit, and adding least burden to binary size and execution speed and memory. > Here is a brief log of the discussions and I would be glad to discuss any > alternative suggestions: > https://docs.google.com/document/d/1FWEFTHzdqIA1wHUo_DuSpJjppW22otCMPCOA5NumhJU. Hi Devlin, A friendly reminder on this.
rdevlin.cronin@chromium.org changed reviewers: + sorin@chromium.org
I don't know about cookies usage for this. +sorin@ might? https://codereview.chromium.org/2797683002/diff/1/chrome/browser/extensions/i... File chrome/browser/extensions/install_signer.cc (right): https://codereview.chromium.org/2797683002/diff/1/chrome/browser/extensions/i... chrome/browser/extensions/install_signer.cc:380: sender: "..." extension install signer https://codereview.chromium.org/2797683002/diff/1/chrome/browser/extensions/i... chrome/browser/extensions/install_signer.cc:381: description: "..." Fetches the signatures for installed extensions https://codereview.chromium.org/2797683002/diff/1/chrome/browser/extensions/i... chrome/browser/extensions/install_signer.cc:382: trigger: "..." Chrome detects an extension that requires installation verification. https://codereview.chromium.org/2797683002/diff/1/chrome/browser/extensions/i... chrome/browser/extensions/install_signer.cc:383: data: "..." The ids of the extensions that need to be verified, as well as a salted hash of the user's machine id. https://codereview.chromium.org/2797683002/diff/1/chrome/browser/extensions/i... chrome/browser/extensions/install_signer.cc:384: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER GOOGLE_OWNED_SERVICE https://codereview.chromium.org/2797683002/diff/1/chrome/browser/extensions/i... chrome/browser/extensions/install_signer.cc:390: chrome_policy { This setting can be disabled by not installing any extensions. ExtensionInstallBlacklist: '*'
Description was changed from ========== Network traffic annotation added to install_signer. Network traffic annotation is added to network request of: chrome/browser/extensions/install_signer.cc BUG=656607 ========== to ========== Network traffic annotation added to install_signer. Network traffic annotation is added to network request of: chrome/browser/extensions/install_signer.cc BUG=656607 ==========
sorin@chromium.org changed reviewers: + asargent@chromium.org
Antony Sargent might know someone that knows.
On 2017/04/19 22:50:01, Sorin Jianu wrote: > Antony Sargent might know someone that knows. I don't think this request needs to use cookies. Furthermore, this entire class can go away because the entire feature (verifying extension ids are known to the webstore at install time) is obsoleted by content verification (which makes sure extension content has been signed by a webstore private key).
On 2017/04/20 00:03:55, asargent_no_longer_on_chrome wrote: > On 2017/04/19 22:50:01, Sorin Jianu wrote: > > Antony Sargent might know someone that knows. > > I don't think this request needs to use cookies. Furthermore, this entire class > can go away because the entire feature (verifying extension ids are known to the > webstore at install time) is obsoleted by content verification (which makes sure > extension content has been signed by a webstore private key). This class is still used in install_verifier.cc and extension_data_collection.cc, if they are going to be refactored soon and this class would be removed, I can add an annotation tag stating it, otherwise it's better to have it annotated now. What do you suggest?
On 2017/04/20 07:55:17, Ramin Halavati wrote: > On 2017/04/20 00:03:55, asargent_no_longer_on_chrome wrote: > > On 2017/04/19 22:50:01, Sorin Jianu wrote: > > > Antony Sargent might know someone that knows. > > > > I don't think this request needs to use cookies. Furthermore, this entire > class > > can go away because the entire feature (verifying extension ids are known to > the > > webstore at install time) is obsoleted by content verification (which makes > sure > > extension content has been signed by a webstore private key). > > This class is still used in install_verifier.cc and > extension_data_collection.cc, > if they are going to be refactored soon and this class would be removed, I can > add > an annotation tag stating it, otherwise it's better to have it annotated now. > What do you suggest? The removal is non-trivial, so if we're pushing to have all these fetches documented, we should probably document this one in the meantime.
Thank you very much, annotation updated, please review. I assumed the cookie_store is 'user', is it correct? https://codereview.chromium.org/2797683002/diff/1/chrome/browser/extensions/i... File chrome/browser/extensions/install_signer.cc (right): https://codereview.chromium.org/2797683002/diff/1/chrome/browser/extensions/i... chrome/browser/extensions/install_signer.cc:380: sender: "..." On 2017/04/19 20:27:07, Devlin wrote: > extension install signer Done. https://codereview.chromium.org/2797683002/diff/1/chrome/browser/extensions/i... chrome/browser/extensions/install_signer.cc:381: description: "..." On 2017/04/19 20:27:07, Devlin wrote: > Fetches the signatures for installed extensions Done. https://codereview.chromium.org/2797683002/diff/1/chrome/browser/extensions/i... chrome/browser/extensions/install_signer.cc:382: trigger: "..." On 2017/04/19 20:27:07, Devlin wrote: > Chrome detects an extension that requires installation verification. Done. https://codereview.chromium.org/2797683002/diff/1/chrome/browser/extensions/i... chrome/browser/extensions/install_signer.cc:383: data: "..." On 2017/04/19 20:27:07, Devlin wrote: > The ids of the extensions that need to be verified, as well as a salted hash of > the user's machine id. Done. https://codereview.chromium.org/2797683002/diff/1/chrome/browser/extensions/i... chrome/browser/extensions/install_signer.cc:384: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER On 2017/04/19 20:27:07, Devlin wrote: > GOOGLE_OWNED_SERVICE Done. https://codereview.chromium.org/2797683002/diff/1/chrome/browser/extensions/i... chrome/browser/extensions/install_signer.cc:390: chrome_policy { On 2017/04/19 20:27:07, Devlin wrote: > This setting can be disabled by not installing any extensions. > ExtensionInstallBlacklist: '*' Done.
On 2017/04/21 05:50:41, Ramin Halavati wrote: > I assumed the cookie_store is 'user', is it correct? This would be my assumption, too, but I'n not 100% sure. Is there a way to verify this from the code?
On 2017/04/24 15:19:24, Devlin wrote: > On 2017/04/21 05:50:41, Ramin Halavati wrote: > > I assumed the cookie_store is 'user', is it correct? > > This would be my assumption, too, but I'n not 100% sure. Is there a way to > verify this from the code? I think it's "user" as InstallSigner receives a context_getter in its constructor (here: https://cs.chromium.org/chromium/src/chrome/browser/extensions/install_signer...) and the constructor is called InstallVerifier::BeginFetch which sends the default storage partition (here: https://cs.chromium.org/chromium/src/chrome/browser/extensions/install_verifi...) and the default storage partition is "user".
lgtm
rhalavati@chromium.org changed reviewers: + battre@chromium.org, msramek@chromium.org - asargent@chromium.org
Thank you very much. Martin/Dominic, Any comments?
sorin@chromium.org changed reviewers: - sorin@chromium.org
https://codereview.chromium.org/2797683002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/install_signer.cc (right): https://codereview.chromium.org/2797683002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/install_signer.cc:388: "salted hash of the user's machine id." If I'm reading the code correctly, we send RLZ if there is one (i.e. promotional instance), and nothing otherwise? How and when is the salt generated? (To be clear, my questions are aimed at assessing the stability of this ID.)
https://codereview.chromium.org/2797683002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/install_signer.cc (right): https://codereview.chromium.org/2797683002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/install_signer.cc:388: "salted hash of the user's machine id." On 2017/05/03 16:08:27, msramek (recovering) wrote: > If I'm reading the code correctly, we send RLZ if there is one (i.e. promotional > instance), and nothing otherwise? > > How and when is the salt generated? > > (To be clear, my questions are aimed at assessing the stability of this ID.) Yeah, the machine id comes from RLZ. We don't send the raw machine id from RLZ, but rather a hash of it + a salt. The salt is generated right above this call (line 361 of this patch set) through crypto::RandBytes(), which should be strongly random. By bundling the 25 (I think?) bit RLZ machine id with a 32-bit random and hashing them in sha256, I think we have sufficient guarantees that there's no way to determine the machine id, even a) by Google/the webstore or b) between unrelated install signature calls (i.e., this id is intentionally unstable). This seems to only be included to verify the validity of the response we receive.
On 2017/05/08 15:19:02, Devlin (catching up) wrote: > https://codereview.chromium.org/2797683002/diff/20001/chrome/browser/extensio... > File chrome/browser/extensions/install_signer.cc (right): > > https://codereview.chromium.org/2797683002/diff/20001/chrome/browser/extensio... > chrome/browser/extensions/install_signer.cc:388: "salted hash of the user's > machine id." > On 2017/05/03 16:08:27, msramek (recovering) wrote: > > If I'm reading the code correctly, we send RLZ if there is one (i.e. > promotional > > instance), and nothing otherwise? > > > > How and when is the salt generated? > > > > (To be clear, my questions are aimed at assessing the stability of this ID.) > > Yeah, the machine id comes from RLZ. We don't send the raw machine id from RLZ, > but rather a hash of it + a salt. The salt is generated right above this call > (line 361 of this patch set) through crypto::RandBytes(), which should be > strongly random. By bundling the 25 (I think?) bit RLZ machine id with a 32-bit > random and hashing them in sha256, I think we have sufficient guarantees that > there's no way to determine the machine id, even a) by Google/the webstore or b) > between unrelated install signature calls (i.e., this id is intentionally > unstable). This seems to only be included to verify the validity of the > response we receive. Martin, Could you please suggest the required change for data. I can't decide what level of details is required here.
https://codereview.chromium.org/2797683002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/install_signer.cc (right): https://codereview.chromium.org/2797683002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/install_signer.cc:388: "salted hash of the user's machine id." On 2017/05/08 15:19:02, Devlin (catching up) wrote: > On 2017/05/03 16:08:27, msramek (recovering) wrote: > > If I'm reading the code correctly, we send RLZ if there is one (i.e. > promotional > > instance), and nothing otherwise? > > > > How and when is the salt generated? > > > > (To be clear, my questions are aimed at assessing the stability of this ID.) > > Yeah, the machine id comes from RLZ. We don't send the raw machine id from RLZ, > but rather a hash of it + a salt. The salt is generated right above this call > (line 361 of this patch set) through crypto::RandBytes(), which should be > strongly random. By bundling the 25 (I think?) bit RLZ machine id with a 32-bit > random and hashing them in sha256, I think we have sufficient guarantees that > there's no way to determine the machine id, even a) by Google/the webstore or b) > between unrelated install signature calls (i.e., this id is intentionally > unstable). This seems to only be included to verify the validity of the > response we receive. Thanks, Devlin! Ramin, then I would perhaps add that this is computed by the RLZ library, included to verify the validity of the response, and emphasize that this is not reversible (Devlin's points a) and b) ).
Thanks Martin, Devlin. Comment addressed, please review. https://codereview.chromium.org/2797683002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/install_signer.cc (right): https://codereview.chromium.org/2797683002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/install_signer.cc:388: "salted hash of the user's machine id." On 2017/05/17 22:22:04, msramek wrote: > On 2017/05/08 15:19:02, Devlin (catching up) wrote: > > On 2017/05/03 16:08:27, msramek (recovering) wrote: > > > If I'm reading the code correctly, we send RLZ if there is one (i.e. > > promotional > > > instance), and nothing otherwise? > > > > > > How and when is the salt generated? > > > > > > (To be clear, my questions are aimed at assessing the stability of this ID.) > > > > Yeah, the machine id comes from RLZ. We don't send the raw machine id from > RLZ, > > but rather a hash of it + a salt. The salt is generated right above this call > > (line 361 of this patch set) through crypto::RandBytes(), which should be > > strongly random. By bundling the 25 (I think?) bit RLZ machine id with a > 32-bit > > random and hashing them in sha256, I think we have sufficient guarantees that > > there's no way to determine the machine id, even a) by Google/the webstore or > b) > > between unrelated install signature calls (i.e., this id is intentionally > > unstable). This seems to only be included to verify the validity of the > > response we receive. > > Thanks, Devlin! > > Ramin, then I would perhaps add that this is computed by the RLZ library, > included to verify the validity of the response, and emphasize that this is not > reversible (Devlin's points a) and b) ). Done.
On 2017/05/18 05:01:14, Ramin Halavati wrote: > Thanks Martin, Devlin. > Comment addressed, please review. > > https://codereview.chromium.org/2797683002/diff/20001/chrome/browser/extensio... > File chrome/browser/extensions/install_signer.cc (right): > > https://codereview.chromium.org/2797683002/diff/20001/chrome/browser/extensio... > chrome/browser/extensions/install_signer.cc:388: "salted hash of the user's > machine id." > On 2017/05/17 22:22:04, msramek wrote: > > On 2017/05/08 15:19:02, Devlin (catching up) wrote: > > > On 2017/05/03 16:08:27, msramek (recovering) wrote: > > > > If I'm reading the code correctly, we send RLZ if there is one (i.e. > > > promotional > > > > instance), and nothing otherwise? > > > > > > > > How and when is the salt generated? > > > > > > > > (To be clear, my questions are aimed at assessing the stability of this > ID.) > > > > > > Yeah, the machine id comes from RLZ. We don't send the raw machine id from > > RLZ, > > > but rather a hash of it + a salt. The salt is generated right above this > call > > > (line 361 of this patch set) through crypto::RandBytes(), which should be > > > strongly random. By bundling the 25 (I think?) bit RLZ machine id with a > > 32-bit > > > random and hashing them in sha256, I think we have sufficient guarantees > that > > > there's no way to determine the machine id, even a) by Google/the webstore > or > > b) > > > between unrelated install signature calls (i.e., this id is intentionally > > > unstable). This seems to only be included to verify the validity of the > > > response we receive. > > > > Thanks, Devlin! > > > > Ramin, then I would perhaps add that this is computed by the RLZ library, > > included to verify the validity of the response, and emphasize that this is > not > > reversible (Devlin's points a) and b) ). > > Done. Martin, Annotation updated, please review.
Thanks! I like the current version a lot. LGTM.
The CQ bit was checked by rhalavati@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2797683002/#ps60001 (title: "Annotation updated.")
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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": 1495106748827030,
"parent_rev": "15fb9f6cd7ef8820ca37a1da0e38b66975ace717", "commit_rev":
"882e84aeece1a7166cd898354e21008aefabf00e"}
Message was sent while issue was closed.
Description was changed from ========== Network traffic annotation added to install_signer. Network traffic annotation is added to network request of: chrome/browser/extensions/install_signer.cc BUG=656607 ========== to ========== Network traffic annotation added to install_signer. Network traffic annotation is added to network request of: chrome/browser/extensions/install_signer.cc BUG=656607 Review-Url: https://codereview.chromium.org/2797683002 Cr-Commit-Position: refs/heads/master@{#472774} Committed: https://chromium.googlesource.com/chromium/src/+/882e84aeece1a7166cd898354e21... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/882e84aeece1a7166cd898354e21... |
