Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2697193003:
Network traffic annotation added to safe_browsing (Closed)
|
Created:
3 years, 10 months ago by Ramin Halavati Modified:
3 years, 9 months ago CC:
chromium-reviews, grt+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNetwork traffic annotation added to safe_browsing.
Network traffic annotation added to four files in Safe Browsing.
BUG=656607
Review-Url: https://codereview.chromium.org/2697193003
Cr-Commit-Position: refs/heads/master@{#455737}
Committed: https://chromium.googlesource.com/chromium/src/+/70960300c81513c83f1ea4b84af11effb6b1476f
Patch Set 1 #
Total comments: 6
Patch Set 2 : Comments addressed. #Patch Set 3 : nits #
Total comments: 14
Patch Set 4 : Annotation updated. #Patch Set 5 : Minor update. #Patch Set 6 : Minor update. #Patch Set 7 : Unittests updated. #
Total comments: 8
Patch Set 8 : Comments addressed. #
Total comments: 4
Patch Set 9 : Comments addressed. #
Messages
Total messages: 26 (10 generated)
rhalavati@chromium.org changed reviewers: + mattm@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 in safe browsing and added annotation template to them. Please review them and suggest the required answers for the empty parts. 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/cl... 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.
mattm@chromium.org changed reviewers: + nparker@chromium.org
+nparker, I could take a stab at providing the annotation content but thought you might want to own it? https://codereview.chromium.org/2697193003/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/threat_details_cache.cc (right): https://codereview.chromium.org/2697193003/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/threat_details_cache.cc:116: current_fetch_->SetLoadFlags(net::LOAD_ONLY_FROM_CACHE | Are annotations needed on LOAD_ONLY_FROM_CACHE fetches? https://codereview.chromium.org/2697193003/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/two_phase_uploader.cc (right): https://codereview.chromium.org/2697193003/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/two_phase_uploader.cc:173: // Create traffic annotation tag. Although TwoPhaseUploader is only used in one place currently, it just implements a non-safebrowsing-specific protocol (so it depends entirely on what URL and data it's created with), so it may be better to pass the annotations in?
https://codereview.chromium.org/2697193003/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/two_phase_uploader.cc (right): https://codereview.chromium.org/2697193003/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/two_phase_uploader.cc:175: net::DefineNetworkTrafficAnnotation("...", R"( Is there a doc describing the intention of these fields, and how they'll be used?
On 2017/02/18 22:16:55, Nathan Parker wrote: > https://codereview.chromium.org/2697193003/diff/1/chrome/browser/safe_browsin... > File chrome/browser/safe_browsing/two_phase_uploader.cc (right): > > https://codereview.chromium.org/2697193003/diff/1/chrome/browser/safe_browsin... > chrome/browser/safe_browsing/two_phase_uploader.cc:175: > net::DefineNetworkTrafficAnnotation("...", R"( > Is there a doc describing the intention of these fields, and how they'll be > used? Ramin's comment on the codereview has links
Annotation changed to an input for TwoPhaseUploader. Please review changes in TwoPhaseUploader.* and add annotations for download_feedback.cc and threat_details_cache.cc. https://codereview.chromium.org/2697193003/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/threat_details_cache.cc (right): https://codereview.chromium.org/2697193003/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/threat_details_cache.cc:116: current_fetch_->SetLoadFlags(net::LOAD_ONLY_FROM_CACHE | On 2017/02/18 02:23:45, mattm wrote: > Are annotations needed on LOAD_ONLY_FROM_CACHE fetches? No, they are required for all network requests. https://codereview.chromium.org/2697193003/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/two_phase_uploader.cc (right): https://codereview.chromium.org/2697193003/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/two_phase_uploader.cc:173: // Create traffic annotation tag. On 2017/02/18 02:23:45, mattm wrote: > Although TwoPhaseUploader is only used in one place currently, it just > implements a non-safebrowsing-specific protocol (so it depends entirely on what > URL and data it's created with), so it may be better to pass the annotations in? Done. https://codereview.chromium.org/2697193003/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/two_phase_uploader.cc:175: net::DefineNetworkTrafficAnnotation("...", R"( On 2017/02/18 22:16:55, Nathan Parker wrote: > Is there a doc describing the intention of these fields, and how they'll be > used? Yes, there are references on the first email on this CL. If there are any ambiguities, please tell me to describe them.
Description was changed from ========== Network traffic annotation added to content::BrowserThread::safe_browsing::ThreatDetailsCacheCollector and TwoPhaseUploaderImpl. Network traffic annotation added to two files in Safe Browsing. BUG=656607 ========== to ========== Network traffic annotation added to safe_browsing. Network traffic annotation added to four files in Safe Browsing. BUG=656607 ==========
https://codereview.chromium.org/2697193003/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/download_feedback.cc (right): https://codereview.chromium.org/2697193003/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/download_feedback.cc:135: sender: "..." "Safe Browsing Download Protection Feedback" https://codereview.chromium.org/2697193003/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/download_feedback.cc:136: description: "..." "When a user downloads a binary that Safe Browsing declares as suspicious, opted-in clients may upload that binary to Safe Browsing to improve the classification. This helps protect users from malware and unwanted software." https://codereview.chromium.org/2697193003/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/download_feedback.cc:137: trigger: "..." "When a download-protection verdict is !SAFE, and the server requests the binary be uploaded, and the user is opted in to extended-reporting, the browser will upload the binary to Google." https://codereview.chromium.org/2697193003/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/download_feedback.cc:142: cookies_allowed: false/true true https://codereview.chromium.org/2697193003/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/download_feedback.cc:143: cookies_store: "..." Safe Browsing cookie store https://codereview.chromium.org/2697193003/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/download_feedback.cc:144: setting: "..." I'm not sure where we say that this is enabled by the user checking the box "Automatically report details of possible security incidents to Google," and that there's a policy where the admin can prevent that box from getting checked. https://codereview.chromium.org/2697193003/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/threat_details_cache.cc (right): https://codereview.chromium.org/2697193003/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/threat_details_cache.cc:88: sender: "..." This really isn't a network request, so I'm not sure what to put here. We're just fetching things from the cache to package up into a report. That report _does_ get sent, but not from here. This is also part of safe browsing extended reporting, for users opted-in.
Annotations updated, please review. https://codereview.chromium.org/2697193003/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/download_feedback.cc (right): https://codereview.chromium.org/2697193003/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/download_feedback.cc:135: sender: "..." On 2017/02/23 01:16:52, Nathan Parker wrote: > "Safe Browsing Download Protection Feedback" Done. https://codereview.chromium.org/2697193003/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/download_feedback.cc:136: description: "..." On 2017/02/23 01:16:53, Nathan Parker wrote: > "When a user downloads a binary that Safe Browsing declares as suspicious, > opted-in clients may upload that binary to Safe Browsing to improve the > classification. This helps protect users from malware and unwanted software." Done. https://codereview.chromium.org/2697193003/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/download_feedback.cc:137: trigger: "..." On 2017/02/23 01:16:52, Nathan Parker wrote: > "When a download-protection verdict is !SAFE, and the server requests the binary > be uploaded, and the user is opted in to extended-reporting, the browser will > upload the binary to Google." Done. https://codereview.chromium.org/2697193003/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/download_feedback.cc:142: cookies_allowed: false/true On 2017/02/23 01:16:53, Nathan Parker wrote: > true Done. https://codereview.chromium.org/2697193003/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/download_feedback.cc:143: cookies_store: "..." On 2017/02/23 01:16:53, Nathan Parker wrote: > Safe Browsing cookie store Done. https://codereview.chromium.org/2697193003/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/download_feedback.cc:144: setting: "..." On 2017/02/23 01:16:53, Nathan Parker wrote: > I'm not sure where we say that this is enabled by the user checking the box > "Automatically report details of possible security incidents to Google," and > that there's a policy where the admin can prevent that box from getting checked. Done. https://codereview.chromium.org/2697193003/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/threat_details_cache.cc (right): https://codereview.chromium.org/2697193003/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/threat_details_cache.cc:88: sender: "..." On 2017/02/23 01:16:53, Nathan Parker wrote: > This really isn't a network request, so I'm not sure what to put here. We're > just fetching things from the cache to package up into a report. That report > _does_ get sent, but not from here. > > This is also part of safe browsing extended reporting, for users opted-in. I think we can add an annotation specifying this. I updated it.
On 2017/02/23 08:18:24, Ramin Halavati wrote: > Annotations updated, please review. > > https://codereview.chromium.org/2697193003/diff/40001/chrome/browser/safe_bro... > File chrome/browser/safe_browsing/download_feedback.cc (right): > > https://codereview.chromium.org/2697193003/diff/40001/chrome/browser/safe_bro... > chrome/browser/safe_browsing/download_feedback.cc:135: sender: "..." > On 2017/02/23 01:16:52, Nathan Parker wrote: > > "Safe Browsing Download Protection Feedback" > > Done. > > https://codereview.chromium.org/2697193003/diff/40001/chrome/browser/safe_bro... > chrome/browser/safe_browsing/download_feedback.cc:136: description: "..." > On 2017/02/23 01:16:53, Nathan Parker wrote: > > "When a user downloads a binary that Safe Browsing declares as suspicious, > > opted-in clients may upload that binary to Safe Browsing to improve the > > classification. This helps protect users from malware and unwanted software." > > Done. > > https://codereview.chromium.org/2697193003/diff/40001/chrome/browser/safe_bro... > chrome/browser/safe_browsing/download_feedback.cc:137: trigger: "..." > On 2017/02/23 01:16:52, Nathan Parker wrote: > > "When a download-protection verdict is !SAFE, and the server requests the > binary > > be uploaded, and the user is opted in to extended-reporting, the browser will > > upload the binary to Google." > > Done. > > https://codereview.chromium.org/2697193003/diff/40001/chrome/browser/safe_bro... > chrome/browser/safe_browsing/download_feedback.cc:142: cookies_allowed: > false/true > On 2017/02/23 01:16:53, Nathan Parker wrote: > > true > > Done. > > https://codereview.chromium.org/2697193003/diff/40001/chrome/browser/safe_bro... > chrome/browser/safe_browsing/download_feedback.cc:143: cookies_store: "..." > On 2017/02/23 01:16:53, Nathan Parker wrote: > > Safe Browsing cookie store > > Done. > > https://codereview.chromium.org/2697193003/diff/40001/chrome/browser/safe_bro... > chrome/browser/safe_browsing/download_feedback.cc:144: setting: "..." > On 2017/02/23 01:16:53, Nathan Parker wrote: > > I'm not sure where we say that this is enabled by the user checking the box > > "Automatically report details of possible security incidents to Google," and > > that there's a policy where the admin can prevent that box from getting > checked. > > Done. > > https://codereview.chromium.org/2697193003/diff/40001/chrome/browser/safe_bro... > File chrome/browser/safe_browsing/threat_details_cache.cc (right): > > https://codereview.chromium.org/2697193003/diff/40001/chrome/browser/safe_bro... > chrome/browser/safe_browsing/threat_details_cache.cc:88: sender: "..." > On 2017/02/23 01:16:53, Nathan Parker wrote: > > This really isn't a network request, so I'm not sure what to put here. We're > > just fetching things from the cache to package up into a report. That report > > _does_ get sent, but not from here. > > > > This is also part of safe browsing extended reporting, for users opted-in. > > I think we can add an annotation specifying this. I updated it. Hi, A friendly reminder on this review.
lgtm LGTM after fixing comments here. https://codereview.chromium.org/2697193003/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/download_feedback.cc (right): https://codereview.chromium.org/2697193003/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_feedback.cc:155: "report details of possible security incdients to Google.' in " spelling: incidents https://codereview.chromium.org/2697193003/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_feedback.cc:157: "is enabled by default." should be: "is disabled by default" https://codereview.chromium.org/2697193003/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/threat_details_cache.cc (right): https://codereview.chromium.org/2697193003/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/threat_details_cache.cc:105: "is enabled by default." is disabled by default https://codereview.chromium.org/2697193003/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/threat_details_cache.cc:109: value: false I'm not sure what "false" means in this context. If you set the policy to false, it indeed prevent these requests.
rhalavati@chromium.org changed reviewers: + battre@chromium.org, msramek@chromium.org
All comments addressed. Thank you for reviewing. https://codereview.chromium.org/2697193003/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/download_feedback.cc (right): https://codereview.chromium.org/2697193003/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_feedback.cc:155: "report details of possible security incdients to Google.' in " On 2017/03/06 17:48:54, Nathan Parker wrote: > spelling: incidents > Done. https://codereview.chromium.org/2697193003/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_feedback.cc:157: "is enabled by default." On 2017/03/06 17:48:54, Nathan Parker wrote: > should be: > "is disabled by default" Done. https://codereview.chromium.org/2697193003/diff/120001/chrome/browser/safe_br... File chrome/browser/safe_browsing/threat_details_cache.cc (right): https://codereview.chromium.org/2697193003/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/threat_details_cache.cc:105: "is enabled by default." On 2017/03/06 17:48:54, Nathan Parker wrote: > is disabled by default Done. https://codereview.chromium.org/2697193003/diff/120001/chrome/browser/safe_br... chrome/browser/safe_browsing/threat_details_cache.cc:109: value: false On 2017/03/06 17:48:54, Nathan Parker wrote: > I'm not sure what "false" means in this context. If you set the policy to > false, it indeed prevent these requests. Here we need the value that disables the feature, hence I think 'false' is correct. I also changed |value| as we will use chrome_settings.promo instead of cloud_policy.promo to parse this part and it has a slightly different notation.
LGTM % nits. https://codereview.chromium.org/2697193003/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/download_feedback.cc (right): https://codereview.chromium.org/2697193003/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_feedback.cc:144: "in to extended-reporting." nit: the dash is superfluous https://codereview.chromium.org/2697193003/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/threat_details_cache.cc (right): https://codereview.chromium.org/2697193003/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/threat_details_cache.cc:90: "This request fetches differnt items from safe browsing cache " typo: different
Comments addressed, landing. https://codereview.chromium.org/2697193003/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/download_feedback.cc (right): https://codereview.chromium.org/2697193003/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_feedback.cc:144: "in to extended-reporting." On 2017/03/09 12:06:08, msramek wrote: > nit: the dash is superfluous Done. https://codereview.chromium.org/2697193003/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/threat_details_cache.cc (right): https://codereview.chromium.org/2697193003/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/threat_details_cache.cc:90: "This request fetches differnt items from safe browsing cache " On 2017/03/09 12:06:08, msramek wrote: > typo: different Done.
The CQ bit was checked by rhalavati@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nparker@chromium.org, msramek@chromium.org Link to the patchset: https://codereview.chromium.org/2697193003/#ps160001 (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
Failed to request the patch to try. Please note that binary files are still unsupported at the moment, this is being worked on. This could be a Rietveld flake (see http://crbug.com/401833), so if this CL has no binary files, please re-check commit box. Thanks for your patience. Transient error: [Errno 104] Connection reset by peer
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": 160001, "attempt_start_ts": 1489067995618730,
"parent_rev": "d0242a5f146addaccb54a3e53257ee26ee330304", "commit_rev":
"70960300c81513c83f1ea4b84af11effb6b1476f"}
Message was sent while issue was closed.
Description was changed from ========== Network traffic annotation added to safe_browsing. Network traffic annotation added to four files in Safe Browsing. BUG=656607 ========== to ========== Network traffic annotation added to safe_browsing. Network traffic annotation added to four files in Safe Browsing. BUG=656607 Review-Url: https://codereview.chromium.org/2697193003 Cr-Commit-Position: refs/heads/master@{#455737} Committed: https://chromium.googlesource.com/chromium/src/+/70960300c81513c83f1ea4b84af1... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/70960300c81513c83f1ea4b84af1... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
