|
|
Created:
3 years, 9 months ago by Ramin Halavati Modified:
3 years, 7 months ago CC:
chromium-reviews, chromoting-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionNetwork traffic annotation added to chromium_url_request.
Network traffic annotation is added to network request of
chromium_url_request.
BUG=65660
Review-Url: https://codereview.chromium.org/2729423003
Cr-Commit-Position: refs/heads/master@{#470289}
Committed: https://chromium.googlesource.com/chromium/src/+/6826b7bed9c59eb51ba0e7904951f465eafed3f2
Patch Set 1 #Patch Set 2 : Annotation moved to ChromiumURLRequest constructor. #Patch Set 3 : Annotation templates added. #
Total comments: 82
Patch Set 4 : Annotations updated. #
Total comments: 2
Patch Set 5 : Annotations updated. #Patch Set 6 : Support links updated. #
Total comments: 9
Patch Set 7 : Annotation updated. #
Total comments: 1
Patch Set 8 : Annotation updated. #
Messages
Total messages: 39 (13 generated)
rhalavati@chromium.org changed reviewers: + jamiewalch@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 one file in remoting/base 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.
sergeyu@chromium.org changed reviewers: + sergeyu@chromium.org
ChromiumUrlRequests implements UrlRequest. It's a generic interface used for several different requests, so I'm not sure one policy is enough here, it may need to be passed from the caller. Note that it most cases this code isn't shipped as part of chrome. There are the following packages where this code can be used: 1. Chrome Remote Desktop Host for Windows, Mac and Linux: it's a package that can be installed separately from chrome. The host still relies on Chrome policies (prefixed with RemoteAccessHost). 2. Chrome Remote Desktop Host for Chrome OS, built in into chrome binary on Chrome OS. This is the only case when ChromiumUrlRequests is used in chrome. 3. Chrome Remote Desktop client for Android/iOS: apps for Android & iOS (separate from chrome). Currently the recommended way to block all 3 use-cases is by using DNS configuration, see https://support.google.com/chrome/a/answer/2799701?hl=en Do we need to add a policy for (1) and (2)? Note that there is also PepperUrlRequest that implements the same interface using Pepper API. It is used in CRD desktop client (Chrome Webapp). Not sure there is anything that needs to be done about it.
rhalavati@chromium.org changed reviewers: + battre@chromium.org, msramek@chromium.org
Thank you very much Sergey for the detailed explanation. As this is not part of Chromium build, I think we can postpone required changes and annotations to when we start annotating other packages and ChromeOS. But as case-1 is in Windows and related to Chrome policies which is the main target of this phase, we may need to add the extra annotation parameter to constructor of ChromiumUrlRequest and add NOT_ANNOTATED_YET flag to all other usages at this point. battre@, msramek@: What do you think?
On 2017/03/07 09:59:24, Ramin Halavati wrote: > Thank you very much Sergey for the detailed explanation. > > As this is not part of Chromium build, I think we can postpone required changes > and annotations to when we start annotating other packages and ChromeOS. > > But as case-1 is in Windows and related to Chrome policies which is the main > target of this phase, we may need to add the extra annotation parameter to > constructor of ChromiumUrlRequest and add NOT_ANNOTATED_YET flag to all other > usages at this point. > > battre@, msramek@: > What do you think? I tried adding annotation tag to constructor of ChromiumUrlRequest and to do so, we also need to either add it to constructor of UrlRequestFactory (https://cs.chromium.org/chromium/src/remoting/base/url_request.h) or add a constructor without annotation tag to it. This cascades to modifying at least 10 files in remoting, while we still are not going to annotate them correctly. As this class is not now part of Chromium's main process and is only used in a separate package, I think it's neater if we add NO_TRAFFIC_ANNOTATION_YET tag here (in ChromiumUrlRequest) and cascade and update it later when we are annotating other packages and remoting.
On 2017/03/08 08:14:59, Ramin Halavati wrote: > On 2017/03/07 09:59:24, Ramin Halavati wrote: > > Thank you very much Sergey for the detailed explanation. > > > > As this is not part of Chromium build, I think we can postpone required > changes > > and annotations to when we start annotating other packages and ChromeOS. > > > > But as case-1 is in Windows and related to Chrome policies which is the main > > target of this phase, we may need to add the extra annotation parameter to > > constructor of ChromiumUrlRequest and add NOT_ANNOTATED_YET flag to all other > > usages at this point. > > > > battre@, msramek@: > > What do you think? > > I tried adding annotation tag to constructor of ChromiumUrlRequest and to do so, > we also need to either add it to constructor of UrlRequestFactory > (https://cs.chromium.org/chromium/src/remoting/base/url_request.h) or add a > constructor without annotation tag to it. > This cascades to modifying at least 10 files in remoting, while we still are not > going to annotate them correctly. > > As this class is not now part of Chromium's main process and is only used in a > separate package, I think it's neater if we add NO_TRAFFIC_ANNOTATION_YET tag > here (in ChromiumUrlRequest) and cascade and update it later when we are > annotating other packages and remoting. This SGTM.
The CQ bit was checked by rhalavati@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
Sergey, As this class is compiled with Chrome and we are adding a presubmit checker to ensure required annotations of all files that are compiled with Chrome, we need to do it now. (The rest of the code would also be eventually annotated, probably in the next quarter.) I moved the annotation to constructor of ChromiumURLRequest and made all necessary changes in other files (and run a CQ dry_run to ensure nothing is missing), please review them. Also 3 files require annotations, please suggest content: remoting/protocol/port_allocator.cc remoting/protocol/http_ice_config_request.cc remoting/base/telemetry_log_writer.cc
On 2017/03/10 09:19:40, Ramin Halavati wrote: > Sergey, > > As this class is compiled with Chrome and we are adding a presubmit checker to > ensure required annotations of all files that are compiled with Chrome, we need > to do it now. (The rest of the code would also be eventually annotated, probably > in the next quarter.) > > I moved the annotation to constructor of ChromiumURLRequest and made all > necessary changes in other files (and run a CQ dry_run to ensure nothing is > missing), please review them. > > Also 3 files require annotations, please suggest content: > remoting/protocol/port_allocator.cc > remoting/protocol/http_ice_config_request.cc > remoting/base/telemetry_log_writer.cc Hi, A friendly reminder on this.
https://codereview.chromium.org/2729423003/diff/40001/remoting/base/telemetry... File remoting/base/telemetry_log_writer.cc (right): https://codereview.chromium.org/2729423003/diff/40001/remoting/base/telemetry... remoting/base/telemetry_log_writer.cc:73: net::DefineNetworkTrafficAnnotation("...", R"( CRD_telemetry_log or something like that. https://codereview.chromium.org/2729423003/diff/40001/remoting/base/telemetry... remoting/base/telemetry_log_writer.cc:75: sender: "..." "Chrome Remote Desktop" https://codereview.chromium.org/2729423003/diff/40001/remoting/base/telemetry... remoting/base/telemetry_log_writer.cc:76: description: "..." "Telemetry logs for Chrome Remote Desktop" https://codereview.chromium.org/2729423003/diff/40001/remoting/base/telemetry... remoting/base/telemetry_log_writer.cc:77: trigger: "..." "Chrome Remote Desktop is being used" https://codereview.chromium.org/2729423003/diff/40001/remoting/base/telemetry... remoting/base/telemetry_log_writer.cc:78: data: "..." "Anonymous usage statistics" https://codereview.chromium.org/2729423003/diff/40001/remoting/base/telemetry... remoting/base/telemetry_log_writer.cc:79: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER GOOGLE_OWNED_SERVICE https://codereview.chromium.org/2729423003/diff/40001/remoting/base/telemetry... remoting/base/telemetry_log_writer.cc:82: cookies_allowed: false/true false https://codereview.chromium.org/2729423003/diff/40001/remoting/base/telemetry... remoting/base/telemetry_log_writer.cc:83: cookies_store: "..." N/A https://codereview.chromium.org/2729423003/diff/40001/remoting/base/telemetry... remoting/base/telemetry_log_writer.cc:84: setting: "..." "Block Chrome Remote Desktop" https://codereview.chromium.org/2729423003/diff/40001/remoting/base/telemetry... remoting/base/telemetry_log_writer.cc:86: [POLICY_NAME] { This code is currently used in the CRD app on Android, so there are not policies to block it. The recommended way to block CRD everywhere is by DNS configuration (documented at https://support.google.com/chrome/a/answer/2799701?hl=en) In the future the same code will be used for the host. There is chrome policy to disable it, but we could potentially add one (in either case the same policy still applies). https://codereview.chromium.org/2729423003/diff/40001/remoting/base/telemetry... remoting/base/telemetry_log_writer.cc:91: policy_exception_justification: "..." The product is shipped separately from Chrome, except on Chrome OS. There are other means to block this request. https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/http_... File remoting/protocol/http_ice_config_request.cc (right): https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/http_... remoting/protocol/http_ice_config_request.cc:102: net::DefineNetworkTrafficAnnotation("...", R"( "CRD ICE Config Request" https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/http_... remoting/protocol/http_ice_config_request.cc:104: sender: "..." Chrome Remote Desktop https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/http_... remoting/protocol/http_ice_config_request.cc:105: description: "..." "Request used by Chrome Remote Desktop to fetch ICE configuration which contains list of STUN & TURN servers and TURN credentials" https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/http_... remoting/protocol/http_ice_config_request.cc:106: trigger: "..." Chrome Remote Desktop usage. https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/http_... remoting/protocol/http_ice_config_request.cc:107: data: "..." None https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/http_... remoting/protocol/http_ice_config_request.cc:108: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER GOOGLE_OWNED_SERVICE https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/http_... remoting/protocol/http_ice_config_request.cc:111: cookies_allowed: false/true false https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/http_... remoting/protocol/http_ice_config_request.cc:112: cookies_store: "..." N/A https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/http_... remoting/protocol/http_ice_config_request.cc:113: setting: "..." Block Chrome Remote Desktop (see https://support.google.com/chrome/a/answer/2799701?hl=en). https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/http_... remoting/protocol/http_ice_config_request.cc:115: [POLICY_NAME] { RemoteAccessHostFirewallTraversal (only applicable on the host side, doesn't have effect in Android and iOS client apps). https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/http_... remoting/protocol/http_ice_config_request.cc:116: policy_options {mode: MANDATORY/RECOMMENDED/UNSET} It's not clear to me what this means. https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/http_... remoting/protocol/http_ice_config_request.cc:117: [POLICY_NAME]: ... //(value to disable it) 0, false https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/http_... remoting/protocol/http_ice_config_request.cc:120: policy_exception_justification: "..." The product is shipped separate from Chrome, except on Chrome OS. https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/port_... File remoting/protocol/port_allocator.cc (right): https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/port_... remoting/protocol/port_allocator.cc:155: net::DefineNetworkTrafficAnnotation("...", R"( "CRD Relay Session Request" https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/port_... remoting/protocol/port_allocator.cc:157: sender: "..." Chrome Remote Desktop https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/port_... remoting/protocol/port_allocator.cc:158: description: "..." "Request used by Chrome Remote Desktop to allocate relay session." https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/port_... remoting/protocol/port_allocator.cc:159: trigger: "..." Chrome Remote Desktop usage. https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/port_... remoting/protocol/port_allocator.cc:160: data: "..." Relay token (received over XMPP) https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/port_... remoting/protocol/port_allocator.cc:161: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER GOOGLE_OWNED_SERVICE https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/port_... remoting/protocol/port_allocator.cc:164: cookies_allowed: false/true false https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/port_... remoting/protocol/port_allocator.cc:165: cookies_store: "..." N/A https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/port_... remoting/protocol/port_allocator.cc:166: setting: "..." Block Chrome Remote Desktop (see https://support.google.com/chrome/a/answer/2799701?hl=en). https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/port_... remoting/protocol/port_allocator.cc:168: [POLICY_NAME] { RemoteAccessHostFirewallTraversal (only applicable on the host side, doesn't have effect in Android and iOS client apps). https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/port_... remoting/protocol/port_allocator.cc:170: [POLICY_NAME]: ... //(value to disable it) 0, false https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/port_... remoting/protocol/port_allocator.cc:173: policy_exception_justification: "..." The product is shipped separate from Chrome, except on Chrome OS.
Hi Sergey, Sorry for long delay, I was OOO until today. Annotations are updated, please review. https://codereview.chromium.org/2729423003/diff/40001/remoting/base/telemetry... File remoting/base/telemetry_log_writer.cc (right): https://codereview.chromium.org/2729423003/diff/40001/remoting/base/telemetry... remoting/base/telemetry_log_writer.cc:73: net::DefineNetworkTrafficAnnotation("...", R"( On 2017/03/17 18:59:32, Sergey Ulanov wrote: > CRD_telemetry_log or something like that. Done. https://codereview.chromium.org/2729423003/diff/40001/remoting/base/telemetry... remoting/base/telemetry_log_writer.cc:75: sender: "..." On 2017/03/17 18:59:31, Sergey Ulanov wrote: > "Chrome Remote Desktop" Done. https://codereview.chromium.org/2729423003/diff/40001/remoting/base/telemetry... remoting/base/telemetry_log_writer.cc:76: description: "..." On 2017/03/17 18:59:31, Sergey Ulanov wrote: > "Telemetry logs for Chrome Remote Desktop" Done. https://codereview.chromium.org/2729423003/diff/40001/remoting/base/telemetry... remoting/base/telemetry_log_writer.cc:77: trigger: "..." On 2017/03/17 18:59:31, Sergey Ulanov wrote: > "Chrome Remote Desktop is being used" Can it be more specific? https://codereview.chromium.org/2729423003/diff/40001/remoting/base/telemetry... remoting/base/telemetry_log_writer.cc:78: data: "..." On 2017/03/17 18:59:31, Sergey Ulanov wrote: > "Anonymous usage statistics" Done. https://codereview.chromium.org/2729423003/diff/40001/remoting/base/telemetry... remoting/base/telemetry_log_writer.cc:79: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER On 2017/03/17 18:59:31, Sergey Ulanov wrote: > GOOGLE_OWNED_SERVICE Done. https://codereview.chromium.org/2729423003/diff/40001/remoting/base/telemetry... remoting/base/telemetry_log_writer.cc:82: cookies_allowed: false/true On 2017/03/17 18:59:31, Sergey Ulanov wrote: > false Cookies are not specifically disabled (they are enabled by default). If you don't need them, can I create a separate CL that disables them? https://codereview.chromium.org/2729423003/diff/40001/remoting/base/telemetry... remoting/base/telemetry_log_writer.cc:83: cookies_store: "..." On 2017/03/17 18:59:32, Sergey Ulanov wrote: > N/A Acknowledged. https://codereview.chromium.org/2729423003/diff/40001/remoting/base/telemetry... remoting/base/telemetry_log_writer.cc:84: setting: "..." On 2017/03/17 18:59:32, Sergey Ulanov wrote: > "Block Chrome Remote Desktop" Done. https://codereview.chromium.org/2729423003/diff/40001/remoting/base/telemetry... remoting/base/telemetry_log_writer.cc:86: [POLICY_NAME] { On 2017/03/17 18:59:31, Sergey Ulanov wrote: > This code is currently used in the CRD app on Android, so there are not policies > to block it. The recommended way to block CRD everywhere is by DNS configuration > (documented at https://support.google.com/chrome/a/answer/2799701?hl=en) > > In the future the same code will be used for the host. There is chrome policy to > disable it, but we could potentially add one (in either case the same policy > still applies). Done. https://codereview.chromium.org/2729423003/diff/40001/remoting/base/telemetry... remoting/base/telemetry_log_writer.cc:91: policy_exception_justification: "..." On 2017/03/17 18:59:31, Sergey Ulanov wrote: > The product is shipped separately from Chrome, except on Chrome OS. > There are other means to block this request. Done. https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/http_... File remoting/protocol/http_ice_config_request.cc (right): https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/http_... remoting/protocol/http_ice_config_request.cc:102: net::DefineNetworkTrafficAnnotation("...", R"( On 2017/03/17 18:59:32, Sergey Ulanov wrote: > "CRD ICE Config Request" Done. https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/http_... remoting/protocol/http_ice_config_request.cc:104: sender: "..." On 2017/03/17 18:59:33, Sergey Ulanov wrote: > Chrome Remote Desktop Done. https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/http_... remoting/protocol/http_ice_config_request.cc:105: description: "..." On 2017/03/17 18:59:33, Sergey Ulanov wrote: > "Request used by Chrome Remote Desktop to fetch ICE configuration which contains > list of STUN & TURN servers and TURN credentials" Done. https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/http_... remoting/protocol/http_ice_config_request.cc:106: trigger: "..." On 2017/03/17 18:59:32, Sergey Ulanov wrote: > Chrome Remote Desktop usage. Can it be more specific? Like on startup or every 10 minutes? https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/http_... remoting/protocol/http_ice_config_request.cc:107: data: "..." On 2017/03/17 18:59:32, Sergey Ulanov wrote: > None Done. https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/http_... remoting/protocol/http_ice_config_request.cc:108: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER On 2017/03/17 18:59:32, Sergey Ulanov wrote: > GOOGLE_OWNED_SERVICE Done. https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/http_... remoting/protocol/http_ice_config_request.cc:111: cookies_allowed: false/true On 2017/03/17 18:59:32, Sergey Ulanov wrote: > false Again, cookies are not specifically disabled. Are they needed or I can create another CL that disables them? https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/http_... remoting/protocol/http_ice_config_request.cc:112: cookies_store: "..." On 2017/03/17 18:59:32, Sergey Ulanov wrote: > N/A Acknowledged. https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/http_... remoting/protocol/http_ice_config_request.cc:113: setting: "..." On 2017/03/17 18:59:32, Sergey Ulanov wrote: > Block Chrome Remote Desktop (see > https://support.google.com/chrome/a/answer/2799701?hl=en). Done. https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/http_... remoting/protocol/http_ice_config_request.cc:115: [POLICY_NAME] { On 2017/03/17 18:59:32, Sergey Ulanov wrote: > RemoteAccessHostFirewallTraversal > (only applicable on the host side, doesn't have effect in Android and iOS client > apps). Done. https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/http_... remoting/protocol/http_ice_config_request.cc:116: policy_options {mode: MANDATORY/RECOMMENDED/UNSET} On 2017/03/17 18:59:32, Sergey Ulanov wrote: > It's not clear to me what this means. Acknowledged. https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/http_... remoting/protocol/http_ice_config_request.cc:117: [POLICY_NAME]: ... //(value to disable it) On 2017/03/17 18:59:32, Sergey Ulanov wrote: > 0, false Done. https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/http_... remoting/protocol/http_ice_config_request.cc:120: policy_exception_justification: "..." On 2017/03/17 18:59:32, Sergey Ulanov wrote: > The product is shipped separate from Chrome, except on Chrome OS. Done. https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/port_... File remoting/protocol/port_allocator.cc (right): https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/port_... remoting/protocol/port_allocator.cc:155: net::DefineNetworkTrafficAnnotation("...", R"( On 2017/03/17 18:59:33, Sergey Ulanov wrote: > "CRD Relay Session Request" Done. https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/port_... remoting/protocol/port_allocator.cc:157: sender: "..." On 2017/03/17 18:59:33, Sergey Ulanov wrote: > Chrome Remote Desktop Done. https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/port_... remoting/protocol/port_allocator.cc:158: description: "..." On 2017/03/17 18:59:33, Sergey Ulanov wrote: > "Request used by Chrome Remote Desktop to allocate relay session." Done. https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/port_... remoting/protocol/port_allocator.cc:159: trigger: "..." On 2017/03/17 18:59:33, Sergey Ulanov wrote: > Chrome Remote Desktop usage. Again, please elaborate if possible. https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/port_... remoting/protocol/port_allocator.cc:160: data: "..." On 2017/03/17 18:59:33, Sergey Ulanov wrote: > Relay token (received over XMPP) Please either elaborate here or in description. This is going to be read by administrators that are not necessarily very technical, and it should be understandable for them. https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/port_... remoting/protocol/port_allocator.cc:161: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER On 2017/03/17 18:59:33, Sergey Ulanov wrote: > GOOGLE_OWNED_SERVICE Done. https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/port_... remoting/protocol/port_allocator.cc:164: cookies_allowed: false/true On 2017/03/17 18:59:33, Sergey Ulanov wrote: > false Again, cookies are not specifically disabled. Can I disable them in another CL? https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/port_... remoting/protocol/port_allocator.cc:165: cookies_store: "..." On 2017/03/17 18:59:33, Sergey Ulanov wrote: > N/A Acknowledged. https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/port_... remoting/protocol/port_allocator.cc:166: setting: "..." On 2017/03/17 18:59:33, Sergey Ulanov wrote: > Block Chrome Remote Desktop (see > https://support.google.com/chrome/a/answer/2799701?hl=en). Done. https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/port_... remoting/protocol/port_allocator.cc:168: [POLICY_NAME] { On 2017/03/17 18:59:33, Sergey Ulanov wrote: > RemoteAccessHostFirewallTraversal > (only applicable on the host side, doesn't have effect in Android and iOS client > apps). Done. https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/port_... remoting/protocol/port_allocator.cc:170: [POLICY_NAME]: ... //(value to disable it) On 2017/03/17 18:59:33, Sergey Ulanov wrote: > 0, false Done. https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/port_... remoting/protocol/port_allocator.cc:173: policy_exception_justification: "..." On 2017/03/17 18:59:33, Sergey Ulanov wrote: > The product is shipped separate from Chrome, except on Chrome OS. Done.
lgtm https://codereview.chromium.org/2729423003/diff/40001/remoting/base/telemetry... File remoting/base/telemetry_log_writer.cc (right): https://codereview.chromium.org/2729423003/diff/40001/remoting/base/telemetry... remoting/base/telemetry_log_writer.cc:82: cookies_allowed: false/true On 2017/04/03 10:01:24, Ramin Halavati wrote: > On 2017/03/17 18:59:31, Sergey Ulanov wrote: > > false > > Cookies are not specifically disabled (they are enabled by default). If you > don't need them, can I create a separate CL that disables them? We don't need them. Please disable. https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/http_... File remoting/protocol/http_ice_config_request.cc (right): https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/http_... remoting/protocol/http_ice_config_request.cc:106: trigger: "..." On 2017/04/03 10:01:25, Ramin Halavati wrote: > On 2017/03/17 18:59:32, Sergey Ulanov wrote: > > Chrome Remote Desktop usage. > > Can it be more specific? Like on startup or every 10 minutes? When a Chrome Remote Desktop session is being connected and periodically while a session is active, as necessary. Currently the API issues credentials that expire every 24 hours, so this request will only be sent again while session is active more than 24 hours and it needs to renegotiate the ICE connection (which is not always the case, e.g. it will not happen if direct connection is used). The 24 hour period is controlled by the server and may change. https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/http_... remoting/protocol/http_ice_config_request.cc:111: cookies_allowed: false/true On 2017/04/03 10:01:25, Ramin Halavati wrote: > On 2017/03/17 18:59:32, Sergey Ulanov wrote: > > false > > Again, cookies are not specifically disabled. Are they needed or I can create > another CL that disables them? They are not needed. Yes, please disable them. https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/port_... File remoting/protocol/port_allocator.cc (right): https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/port_... remoting/protocol/port_allocator.cc:160: data: "..." On 2017/04/03 10:01:25, Ramin Halavati wrote: > On 2017/03/17 18:59:33, Sergey Ulanov wrote: > > Relay token (received over XMPP) > > Please either elaborate here or in description. This is going to be read by > administrators that are not necessarily very technical, and it should be > understandable for them. Maybe "A temporary authentication token issues by Google services (over XMPP connection)"? https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/port_... remoting/protocol/port_allocator.cc:164: cookies_allowed: false/true On 2017/04/03 10:01:25, Ramin Halavati wrote: > On 2017/03/17 18:59:33, Sergey Ulanov wrote: > > false > > Again, cookies are not specifically disabled. Can I disable them in another CL? Yes, please https://codereview.chromium.org/2729423003/diff/60001/remoting/base/telemetry... File remoting/base/telemetry_log_writer.cc (right): https://codereview.chromium.org/2729423003/diff/60001/remoting/base/telemetry... remoting/base/telemetry_log_writer.cc:91: "could potentially add one." Not sure the second sentence is necessary. If you do keep then maybe replace "add one" with "add a policy to control it"
rhalavati@chromium.org changed reviewers: + jainabhishek@chromium.org
Sergey: Thank you very much, all comments addressed. I added this CL to disable cookies for ChromiumUrlRequest: https://codereview.chromium.org/2792283002 Just please add more details on trigger cases of telemetry_log_writer and port_allocator. jainabhishek@: We have 3 links to (https://support.google.com/chrome/a/answer/2799701?hl=en) in this CL and there is a presubmit warning suggesting changing them to a "p=" form. I couldn't find out how to solve it. Could you please hint on it? https://codereview.chromium.org/2729423003/diff/40001/remoting/base/telemetry... File remoting/base/telemetry_log_writer.cc (right): https://codereview.chromium.org/2729423003/diff/40001/remoting/base/telemetry... remoting/base/telemetry_log_writer.cc:82: cookies_allowed: false/true On 2017/04/04 00:16:47, Sergey Ulanov wrote: > On 2017/04/03 10:01:24, Ramin Halavati wrote: > > On 2017/03/17 18:59:31, Sergey Ulanov wrote: > > > false > > > > Cookies are not specifically disabled (they are enabled by default). If you > > don't need them, can I create a separate CL that disables them? > > We don't need them. Please disable. Done, in https://codereview.chromium.org/2792283002 https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/http_... File remoting/protocol/http_ice_config_request.cc (right): https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/http_... remoting/protocol/http_ice_config_request.cc:106: trigger: "..." On 2017/04/04 00:16:47, Sergey Ulanov wrote: > On 2017/04/03 10:01:25, Ramin Halavati wrote: > > On 2017/03/17 18:59:32, Sergey Ulanov wrote: > > > Chrome Remote Desktop usage. > > > > Can it be more specific? Like on startup or every 10 minutes? > > When a Chrome Remote Desktop session is being connected and periodically while a > session is active, as necessary. > > Currently the API issues credentials that expire every 24 hours, so this request > will only be sent again while session is active more than 24 hours and it needs > to renegotiate the ICE connection (which is not always the case, e.g. it will > not happen if direct connection is used). The 24 hour period is controlled by > the server and may change. Done. https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/http_... remoting/protocol/http_ice_config_request.cc:111: cookies_allowed: false/true On 2017/04/04 00:16:47, Sergey Ulanov wrote: > On 2017/04/03 10:01:25, Ramin Halavati wrote: > > On 2017/03/17 18:59:32, Sergey Ulanov wrote: > > > false > > > > Again, cookies are not specifically disabled. Are they needed or I can create > > another CL that disables them? > > They are not needed. Yes, please disable them. Done, in https://codereview.chromium.org/2792283002 https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/port_... File remoting/protocol/port_allocator.cc (right): https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/port_... remoting/protocol/port_allocator.cc:160: data: "..." On 2017/04/04 00:16:48, Sergey Ulanov wrote: > On 2017/04/03 10:01:25, Ramin Halavati wrote: > > On 2017/03/17 18:59:33, Sergey Ulanov wrote: > > > Relay token (received over XMPP) > > > > Please either elaborate here or in description. This is going to be read by > > administrators that are not necessarily very technical, and it should be > > understandable for them. > > Maybe "A temporary authentication token issues by Google services (over XMPP > connection)"? Done. https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/port_... remoting/protocol/port_allocator.cc:164: cookies_allowed: false/true On 2017/04/04 00:16:47, Sergey Ulanov wrote: > On 2017/04/03 10:01:25, Ramin Halavati wrote: > > On 2017/03/17 18:59:33, Sergey Ulanov wrote: > > > false > > > > Again, cookies are not specifically disabled. Can I disable them in another > CL? > > Yes, please Done, in https://codereview.chromium.org/2792283002 https://codereview.chromium.org/2729423003/diff/60001/remoting/base/telemetry... File remoting/base/telemetry_log_writer.cc (right): https://codereview.chromium.org/2729423003/diff/60001/remoting/base/telemetry... remoting/base/telemetry_log_writer.cc:91: "could potentially add one." On 2017/04/04 00:16:48, Sergey Ulanov wrote: > Not sure the second sentence is necessary. > If you do keep then maybe replace "add one" with "add a policy to control it" Done.
Hello Ramin P-Link for this article is https://support.google.com/chrome/?p=remote_desktop Using these links instead of direct help center article allow us to optimize and consolidate help center article without a need for code update. Hope this works, Please let me know in case more info is required. Thanks Abhishek Jain On Tue, Apr 4, 2017 at 12:37 AM <rhalavati@chromium.org> wrote: Sergey: Thank you very much, all comments addressed. I added this CL to disable cookies for ChromiumUrlRequest: https://codereview.chromium.org/2792283002 Just please add more details on trigger cases of telemetry_log_writer and port_allocator. jainabhishek@: We have 3 links to (https://support.google.com/chrome/a/answer/2799701?hl=en) in this CL and there is a presubmit warning suggesting changing them to a "p=" form. I couldn't find out how to solve it. Could you please hint on it? https://codereview.chromium.org/2729423003/diff/40001/remoting/base/telemetry... File remoting/base/telemetry_log_writer.cc (right): https://codereview.chromium.org/2729423003/diff/40001/remoting/base/telemetry... remoting/base/telemetry_log_writer.cc:82: cookies_allowed: false/true On 2017/04/04 00:16:47, Sergey Ulanov wrote: > On 2017/04/03 10:01:24, Ramin Halavati wrote: > > On 2017/03/17 18:59:31, Sergey Ulanov wrote: > > > false > > > > Cookies are not specifically disabled (they are enabled by default). If you > > don't need them, can I create a separate CL that disables them? > > We don't need them. Please disable. Done, in https://codereview.chromium.org/2792283002 https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/http_... File remoting/protocol/http_ice_config_request.cc (right): https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/http_... remoting/protocol/http_ice_config_request.cc:106: trigger: "..." On 2017/04/04 00:16:47, Sergey Ulanov wrote: > On 2017/04/03 10:01:25, Ramin Halavati wrote: > > On 2017/03/17 18:59:32, Sergey Ulanov wrote: > > > Chrome Remote Desktop usage. > > > > Can it be more specific? Like on startup or every 10 minutes? > > When a Chrome Remote Desktop session is being connected and periodically while a > session is active, as necessary. > > Currently the API issues credentials that expire every 24 hours, so this request > will only be sent again while session is active more than 24 hours and it needs > to renegotiate the ICE connection (which is not always the case, e.g. it will > not happen if direct connection is used). The 24 hour period is controlled by > the server and may change. Done. https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/http_... remoting/protocol/http_ice_config_request.cc:111: cookies_allowed: false/true On 2017/04/04 00:16:47, Sergey Ulanov wrote: > On 2017/04/03 10:01:25, Ramin Halavati wrote: > > On 2017/03/17 18:59:32, Sergey Ulanov wrote: > > > false > > > > Again, cookies are not specifically disabled. Are they needed or I can create > > another CL that disables them? > > They are not needed. Yes, please disable them. Done, in https://codereview.chromium.org/2792283002 https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/port_... File remoting/protocol/port_allocator.cc (right): https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/port_... remoting/protocol/port_allocator.cc:160: data: "..." On 2017/04/04 00:16:48, Sergey Ulanov wrote: > On 2017/04/03 10:01:25, Ramin Halavati wrote: > > On 2017/03/17 18:59:33, Sergey Ulanov wrote: > > > Relay token (received over XMPP) > > > > Please either elaborate here or in description. This is going to be read by > > administrators that are not necessarily very technical, and it should be > > understandable for them. > > Maybe "A temporary authentication token issues by Google services (over XMPP > connection)"? Done. https://codereview.chromium.org/2729423003/diff/40001/remoting/protocol/port_... remoting/protocol/port_allocator.cc:164: cookies_allowed: false/true On 2017/04/04 00:16:47, Sergey Ulanov wrote: > On 2017/04/03 10:01:25, Ramin Halavati wrote: > > On 2017/03/17 18:59:33, Sergey Ulanov wrote: > > > false > > > > Again, cookies are not specifically disabled. Can I disable them in another > CL? > > Yes, please Done, in https://codereview.chromium.org/2792283002 https://codereview.chromium.org/2729423003/diff/60001/remoting/base/telemetry... File remoting/base/telemetry_log_writer.cc (right): https://codereview.chromium.org/2729423003/diff/60001/remoting/base/telemetry... remoting/base/telemetry_log_writer.cc:91: "could potentially add one." On 2017/04/04 00:16:48, Sergey Ulanov wrote: > Not sure the second sentence is necessary. > If you do keep then maybe replace "add one" with "add a policy to control it" Done. https://codereview.chromium.org/2729423003/ -- Thanks and Regards Abhishek Jain Product Support Specialist - Chrome -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Thank you very much Abhishek. Links updated.
On 2017/04/05 07:50:00, Ramin Halavati wrote: > Thank you very much Abhishek. Links updated. Hi Sergey, A friendly reminder on adding more details on trigger cases of telemetry_log_writer and port_allocator.
On 2017/04/11 06:08:00, Ramin Halavati wrote: > On 2017/04/05 07:50:00, Ramin Halavati wrote: > > Thank you very much Abhishek. Links updated. > > Hi Sergey, > > A friendly reminder on adding more details on trigger cases of > telemetry_log_writer and port_allocator. LGTM
Sergey, Thank you very much, but I am sorry I still have a few ambiguities, I added 3 inline questions. Please review. https://codereview.chromium.org/2729423003/diff/100001/remoting/base/telemetr... File remoting/base/telemetry_log_writer.cc (right): https://codereview.chromium.org/2729423003/diff/100001/remoting/base/telemetr... remoting/base/telemetry_log_writer.cc:77: trigger: "Chrome Remote Desktop is being used." Is there any frequency to it, or e.g. when closing CRD, opening CRD? Can we be more specific? https://codereview.chromium.org/2729423003/diff/100001/remoting/protocol/port... File remoting/protocol/port_allocator.cc (right): https://codereview.chromium.org/2729423003/diff/100001/remoting/protocol/port... remoting/protocol/port_allocator.cc:160: "session." Since this is read by non-very-technical admins, can we be a little more specific on what are relay sessions? https://codereview.chromium.org/2729423003/diff/100001/remoting/protocol/port... remoting/protocol/port_allocator.cc:161: trigger: "Chrome Remote Desktop usage." Can we be more specific on WHEN?
Sorry I missed your questions last time. https://codereview.chromium.org/2729423003/diff/100001/remoting/base/telemetr... File remoting/base/telemetry_log_writer.cc (right): https://codereview.chromium.org/2729423003/diff/100001/remoting/base/telemetr... remoting/base/telemetry_log_writer.cc:77: trigger: "Chrome Remote Desktop is being used." On 2017/04/13 06:59:04, Ramin Halavati wrote: > Is there any frequency to it, or e.g. when closing CRD, opening CRD? > Can we be more specific? These requests sent periodically when a session is connected, i.e. CRD app is open and is connected to a host. https://codereview.chromium.org/2729423003/diff/100001/remoting/protocol/port... File remoting/protocol/port_allocator.cc (right): https://codereview.chromium.org/2729423003/diff/100001/remoting/protocol/port... remoting/protocol/port_allocator.cc:160: "session." On 2017/04/13 06:59:04, Ramin Halavati wrote: > Since this is read by non-very-technical admins, can we be a little more > specific on what are relay sessions? Maybe reword as: "Request is sent by Chrome Remote Desktop to allocate relay session. Returned relay session credentials are used over UDP to connect to Google-owned relay servers, which is required for NAT traversal." Or do we need more details? https://codereview.chromium.org/2729423003/diff/100001/remoting/protocol/port... remoting/protocol/port_allocator.cc:161: trigger: "Chrome Remote Desktop usage." On 2017/04/13 06:59:04, Ramin Halavati wrote: > Can we be more specific on WHEN? Start of each Chrome Remote Desktop and during connection when peer-to-peer transport needs to be reconnected.
Thank you very much. Martin, Any comments? https://codereview.chromium.org/2729423003/diff/100001/remoting/base/telemetr... File remoting/base/telemetry_log_writer.cc (right): https://codereview.chromium.org/2729423003/diff/100001/remoting/base/telemetr... remoting/base/telemetry_log_writer.cc:77: trigger: "Chrome Remote Desktop is being used." On 2017/04/18 18:40:51, Sergey Ulanov wrote: > On 2017/04/13 06:59:04, Ramin Halavati wrote: > > Is there any frequency to it, or e.g. when closing CRD, opening CRD? > > Can we be more specific? > > These requests sent periodically when a session is connected, i.e. CRD app is > open and is connected to a host. Done. https://codereview.chromium.org/2729423003/diff/100001/remoting/protocol/port... File remoting/protocol/port_allocator.cc (right): https://codereview.chromium.org/2729423003/diff/100001/remoting/protocol/port... remoting/protocol/port_allocator.cc:160: "session." On 2017/04/18 18:40:51, Sergey Ulanov wrote: > On 2017/04/13 06:59:04, Ramin Halavati wrote: > > Since this is read by non-very-technical admins, can we be a little more > > specific on what are relay sessions? > > Maybe reword as: > "Request is sent by Chrome Remote Desktop to allocate relay session. Returned > relay session credentials are used over UDP to connect to Google-owned relay > servers, which is required for NAT traversal." > > Or do we need more details? Thank you, that's more than enough! https://codereview.chromium.org/2729423003/diff/100001/remoting/protocol/port... remoting/protocol/port_allocator.cc:161: trigger: "Chrome Remote Desktop usage." On 2017/04/18 18:40:51, Sergey Ulanov wrote: > On 2017/04/13 06:59:04, Ramin Halavati wrote: > > Can we be more specific on WHEN? > > Start of each Chrome Remote Desktop and during connection when peer-to-peer > transport needs to be reconnected. Done.
LGTM with a comment. https://codereview.chromium.org/2729423003/diff/120001/remoting/base/telemetr... File remoting/base/telemetry_log_writer.cc (right): https://codereview.chromium.org/2729423003/diff/120001/remoting/base/telemetr... remoting/base/telemetry_log_writer.cc:80: data: "Anonymous usage statistics." This is a list of ChromotingEvent? Do we want to elaborate on that? Since we're trying to provide transparency, I would prefer to be a bit more explicit than "trust us, it's anonymous" :)
LGTM with a comment.
On 2017/04/19 12:07:28, msramek (recovering) wrote: > LGTM with a comment. Jamie, Since Surgey is OOO for another 10 days, could you please check Martin's comment?
On 2017/05/04 07:31:00, Ramin Halavati wrote: > On 2017/04/19 12:07:28, msramek (recovering) wrote: > > LGTM with a comment. > > Jamie, > > Since Surgey is OOO for another 10 days, could you please check Martin's > comment? ChromotingEvent is defined in remoting/base/chromoting_event.h. Would a reference to that file here be sufficient?
Thank you very much, comment addressed. Martin?
Patchset #8 (id:140001) has been deleted
LGTM, thanks!
The CQ bit was checked by rhalavati@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org Link to the patchset: https://codereview.chromium.org/2729423003/#ps160001 (title: "Annotation updated.")
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": 1494328887814440, "parent_rev": "abd2c36fd30e3b814a4f0f5ce024dd583e71af1a", "commit_rev": "6826b7bed9c59eb51ba0e7904951f465eafed3f2"}
Message was sent while issue was closed.
Description was changed from ========== Network traffic annotation added to chromium_url_request. Network traffic annotation is added to network request of chromium_url_request. BUG=65660 ========== to ========== Network traffic annotation added to chromium_url_request. Network traffic annotation is added to network request of chromium_url_request. BUG=65660 Review-Url: https://codereview.chromium.org/2729423003 Cr-Commit-Position: refs/heads/master@{#470289} Committed: https://chromium.googlesource.com/chromium/src/+/6826b7bed9c59eb51ba0e7904951... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001) as https://chromium.googlesource.com/chromium/src/+/6826b7bed9c59eb51ba0e7904951... |