Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2801643002:
Network traffic annotation added to base_ping_manager. (Closed)
|
Created:
3 years, 8 months ago by Ramin Halavati Modified:
3 years, 7 months ago CC:
chromium-reviews, vakh+watch_chromium.org, droger+watchlist_chromium.org, grt+watch_chromium.org, sdefresne+watchlist_chromium.org, timvolodine, blundell+watchlist_chromium.org, battre Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionNetwork traffic annotation added to base_ping_manager.
Network traffic annotation is added to network requests of
components/safe_browsing/base_ping_manager.cc
BUG=656607
Review-Url: https://codereview.chromium.org/2801643002
Cr-Commit-Position: refs/heads/master@{#472026}
Committed: https://chromium.googlesource.com/chromium/src/+/e4141878bf424924807965df7e3b89d230dc4733
Patch Set 1 #
Total comments: 4
Patch Set 2 : Commets addressed. #
Total comments: 9
Patch Set 3 : Comments addressed. #
Messages
Total messages: 27 (10 generated)
rhalavati@chromium.org changed reviewers: + asanka@chromium.org, shess@chromium.org
asanka@: Please review DEPS as owner of /net. shess@: 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 base_ping_manager and added 2 annotation templates to it. 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.
DEPS lgtm
shess@chromium.org changed reviewers: + nparker@chromium.org
Bouncing to Nathan, I think it's fine, but really have no idea.
On 2017/04/10 16:37:30, Scott Hess wrote: > Bouncing to Nathan, I think it's fine, but really have no idea. Thank you, I just note that we need suggested values for all missing data.
https://codereview.chromium.org/2801643002/diff/1/components/safe_browsing/ba... File components/safe_browsing/base_ping_manager.cc (right): https://codereview.chromium.org/2801643002/diff/1/components/safe_browsing/ba... components/safe_browsing/base_ping_manager.cc:116: sender: "..." Sender: Safe browsing extended reporting desc: When a user is opted in to automatically reporting "possible security incidents to Google," and they reach a bad page that's flagged by Safe Browsing, Chrome will send a report to Google with information about the threat. This helps Safe Browsing learn where threats originate and thus protect more users. trigger: When an red interstitial is show, and the user is opted-in Data: URLs and referrers from from the page along with other security-relevant data from the page contents Cookies: yes, safe browsing cookie store. Can turn it off by policy by disallowing "safe browsing extended reporting" https://codereview.chromium.org/2801643002/diff/1/components/safe_browsing/ba... components/safe_browsing/base_ping_manager.cc:163: net::DefineNetworkTrafficAnnotation("...", R"( Same as above. We'll eventually consolidate these two reports.
Annotation updated, please review. https://codereview.chromium.org/2801643002/diff/1/components/safe_browsing/ba... File components/safe_browsing/base_ping_manager.cc (right): https://codereview.chromium.org/2801643002/diff/1/components/safe_browsing/ba... components/safe_browsing/base_ping_manager.cc:116: sender: "..." On 2017/04/14 21:32:23, Nathan Parker wrote: > Sender: Safe browsing extended reporting > desc: When a user is opted in to automatically reporting "possible security > incidents to Google," and they reach a bad page that's flagged by Safe Browsing, > Chrome will send a report to Google with information about the threat. This > helps Safe Browsing learn where threats originate and thus protect more users. > > trigger: When an red interstitial is show, and the user is opted-in > > Data: URLs and referrers from from the page along with other security-relevant > data from the page contents > > Cookies: yes, safe browsing cookie store. > > Can turn it off by policy by disallowing "safe browsing extended reporting" > Done. https://codereview.chromium.org/2801643002/diff/1/components/safe_browsing/ba... components/safe_browsing/base_ping_manager.cc:163: net::DefineNetworkTrafficAnnotation("...", R"( On 2017/04/14 21:32:23, Nathan Parker wrote: > Same as above. We'll eventually consolidate these two reports. As they were the same, merged them into one constant in global namespace.
lgtm
rhalavati@chromium.org changed reviewers: + msramek@chromium.org
Thank you very much. Martin, Any comments?
Sorry for the long delays on this. I wanted to wait until we have all those discussions about SBER and phishing first; and then I was OOO. I'd like us to expand this annotation a bit. https://codereview.chromium.org/2801643002/diff/20001/components/safe_browsin... File components/safe_browsing/base_ping_manager.cc (right): https://codereview.chromium.org/2801643002/diff/20001/components/safe_browsin... components/safe_browsing/base_ping_manager.cc:71: "When an red interstitial is show, and the user is opted-in." typo: s/an/a/ https://codereview.chromium.org/2801643002/diff/20001/components/safe_browsin... components/safe_browsing/base_ping_manager.cc:73: "URLs and referrers from from the page along with other security-" Nathan, the plural (URLs) means URLs of subresources? Can we be explicit about that? Could we also elaborate a bit on the security-relevant data? I know that SBER is evolving a lot these days, so I'm not asking for a full enumeration, just a few interesting examples. Thanks! https://codereview.chromium.org/2801643002/diff/20001/components/safe_browsin... components/safe_browsing/base_ping_manager.cc:83: "Google' in Chrome's settings under 'Privcay'. The feature is " typo: Privacy
https://codereview.chromium.org/2801643002/diff/20001/components/safe_browsin... File components/safe_browsing/base_ping_manager.cc (right): https://codereview.chromium.org/2801643002/diff/20001/components/safe_browsin... components/safe_browsing/base_ping_manager.cc:73: "URLs and referrers from from the page along with other security-" On 2017/05/04 10:56:15, msramek (recovering) wrote: > Nathan, the plural (URLs) means URLs of subresources? Can we be explicit about > that? > > Could we also elaborate a bit on the security-relevant data? I know that SBER is > evolving a lot these days, so I'm not asking for a full enumeration, just a few > interesting examples. Thanks! Sure, how about: data: The report includes the URL and referrer chain of the page. If the warning is triggered by a subresource on a partially loaded page, the report will include the URL and referrer chain of sub frames and resources loaded into the page. It may also include a subset of headers for resources loaded, and some Google ad identifiers to help block malicious ads. https://codereview.chromium.org/2801643002/diff/20001/components/safe_browsin... components/safe_browsing/base_ping_manager.cc:84: "enabled by default." The feature is DISABLED by default.
Patchset #3 (id:40001) has been deleted
All comment addressed, please review. https://codereview.chromium.org/2801643002/diff/20001/components/safe_browsin... File components/safe_browsing/base_ping_manager.cc (right): https://codereview.chromium.org/2801643002/diff/20001/components/safe_browsin... components/safe_browsing/base_ping_manager.cc:71: "When an red interstitial is show, and the user is opted-in." On 2017/05/04 10:56:15, msramek (recovering) wrote: > typo: s/an/a/ Done. https://codereview.chromium.org/2801643002/diff/20001/components/safe_browsin... components/safe_browsing/base_ping_manager.cc:73: "URLs and referrers from from the page along with other security-" On 2017/05/04 15:44:43, Nathan Parker wrote: > On 2017/05/04 10:56:15, msramek (recovering) wrote: > > Nathan, the plural (URLs) means URLs of subresources? Can we be explicit about > > that? > > > > Could we also elaborate a bit on the security-relevant data? I know that SBER > is > > evolving a lot these days, so I'm not asking for a full enumeration, just a > few > > interesting examples. Thanks! > > Sure, how about: > > data: The report includes the URL and referrer chain of the page. If the warning > is triggered by a subresource on a partially loaded page, the report will > include the URL and referrer chain of sub frames and resources loaded into the > page. It may also include a subset of headers for resources loaded, and some > Google ad identifiers to help block malicious ads. Done. https://codereview.chromium.org/2801643002/diff/20001/components/safe_browsin... components/safe_browsing/base_ping_manager.cc:83: "Google' in Chrome's settings under 'Privcay'. The feature is " On 2017/05/04 10:56:16, msramek (recovering) wrote: > typo: Privacy Done. https://codereview.chromium.org/2801643002/diff/20001/components/safe_browsin... components/safe_browsing/base_ping_manager.cc:84: "enabled by default." On 2017/05/04 15:44:43, Nathan Parker wrote: > The feature is DISABLED by default. Done.
LGTM
And thanks for expanding :)
The CQ bit was checked by rhalavati@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org, nparker@chromium.org Link to the patchset: https://codereview.chromium.org/2801643002/#ps60001 (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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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": 1494911461001120,
"parent_rev": "35750723170c212e5b588861b1b39abe7c46c253", "commit_rev":
"e4141878bf424924807965df7e3b89d230dc4733"}
Message was sent while issue was closed.
Description was changed from ========== Network traffic annotation added to base_ping_manager. Network traffic annotation is added to network requests of components/safe_browsing/base_ping_manager.cc BUG=656607 ========== to ========== Network traffic annotation added to base_ping_manager. Network traffic annotation is added to network requests of components/safe_browsing/base_ping_manager.cc BUG=656607 Review-Url: https://codereview.chromium.org/2801643002 Cr-Commit-Position: refs/heads/master@{#472026} Committed: https://chromium.googlesource.com/chromium/src/+/e4141878bf424924807965df7e3b... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/e4141878bf424924807965df7e3b... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
