Network traffic annotation added to gcm_channel_status_request.

Network traffic annotation added to gcm_channel_status_request.

Network traffic annotation is added to network request of
gcm_channel_status_request.

BUG=656607
Review-Url: https://codereview.chromium.org/2708863002
Cr-Commit-Position: refs/heads/master@{#452018}
Committed: https://chromium.googlesource.com/chromium/src/+/85dc675418dd4b3ebc77a76c597d3ce5d8eb53b4

[Ramin Halavati, 2017-02-21 13:18:36]

rhalavati@chromium.org changed reviewers:
+ fgorski@chromium.org

[Ramin Halavati, 2017-02-21 13:18:37]

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 gcm_channel_status_request and added annotation template to it.
Please review it 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.

[Peter Beverloo, 2017-02-21 14:04:12]

peter@chromium.org changed reviewers:
+ peter@chromium.org

[Peter Beverloo, 2017-02-21 14:04:13]

Hi Ramin -- hereby a first proposal. It's very difficult to come up with a
comprehensive claim since the GCM Driver is a generalized component that other
features can interact with at will. I've covered the majority use-cases.

https://codereview.chromium.org/2708863002/diff/1/components/gcm_driver/gcm_c...
File components/gcm_driver/gcm_channel_status_request.cc (right):

https://codereview.chromium.org/2708863002/diff/1/components/gcm_driver/gcm_c...
components/gcm_driver/gcm_channel_status_request.cc:72:
net::DefineNetworkTrafficAnnotation("...", R"(
gcm_channel_status_request

https://codereview.chromium.org/2708863002/diff/1/components/gcm_driver/gcm_c...
components/gcm_driver/gcm_channel_status_request.cc:73: semantics {
semantics {
  sender: "gcm_driver"
  description:
    "Google Chrome interacts with Google Cloud Messaging to receive push "
    "messages for various browser features, as well as on behalf of "
    "websites and extensions. The channel status request periodically "
    "confirms with Google servers whether the feature should be enabled."
  trigger:
    "Periodically when Chrome has established and active Google Cloud "
    "Messaging subscriptions. The first request will be issued a minute "
    "after the first subscription activates. Subsequent requests will be "
    "issued each hour with a jitter of 15 minutes. Google can adjust this "
    "interval when it deems that necessary."
  data:
    "A user agent string containing the Chrome version, channel and "
    "platform will be sent to the server. No user identifier is sent "
    "along with the request."
  destination: GOOGLE_OWNED_SERVICE
}
policy {
  cookies_allowed: false
  setting:
    "Support for interacting with Google Cloud Messaging is enabled by "
    "default, and there is no configuration option to completely disable "
    "it. Websites wishing to receive push messages must acquire express "
    "permission from the user for the 'Notification' permission."
}

[Ramin Halavati, 2017-02-21 14:17:50]

Hi Peter,

Thank you very much, annotation updated, please review.
Do you consider a policy to disable this required or not useful?

https://codereview.chromium.org/2708863002/diff/1/components/gcm_driver/gcm_c...
File components/gcm_driver/gcm_channel_status_request.cc (right):

https://codereview.chromium.org/2708863002/diff/1/components/gcm_driver/gcm_c...
components/gcm_driver/gcm_channel_status_request.cc:72:
net::DefineNetworkTrafficAnnotation("...", R"(
On 2017/02/21 14:04:13, Peter Beverloo wrote:
> gcm_channel_status_request

Done.

https://codereview.chromium.org/2708863002/diff/1/components/gcm_driver/gcm_c...
components/gcm_driver/gcm_channel_status_request.cc:73: semantics {
On 2017/02/21 14:04:13, Peter Beverloo wrote:
> semantics {
>   sender: "gcm_driver"
>   description:
>     "Google Chrome interacts with Google Cloud Messaging to receive push "
>     "messages for various browser features, as well as on behalf of "
>     "websites and extensions. The channel status request periodically "
>     "confirms with Google servers whether the feature should be enabled."
>   trigger:
>     "Periodically when Chrome has established and active Google Cloud "
>     "Messaging subscriptions. The first request will be issued a minute "
>     "after the first subscription activates. Subsequent requests will be "
>     "issued each hour with a jitter of 15 minutes. Google can adjust this "
>     "interval when it deems that necessary."
>   data:
>     "A user agent string containing the Chrome version, channel and "
>     "platform will be sent to the server. No user identifier is sent "
>     "along with the request."
>   destination: GOOGLE_OWNED_SERVICE
> }
> policy {
>   cookies_allowed: false
>   setting:
>     "Support for interacting with Google Cloud Messaging is enabled by "
>     "default, and there is no configuration option to completely disable "
>     "it. Websites wishing to receive push messages must acquire express "
>     "permission from the user for the 'Notification' permission."
> }

Done.

[Peter Beverloo, 2017-02-21 14:25:42]

peter@chromium.org changed reviewers:
+ zea@chromium.org

[Peter Beverloo, 2017-02-21 14:25:43]

lgtm, but please give either fgorski@ or zea@ some time (a day?) to give this a
read over if they have a chance.

I can't really judge whether having a policy for disabling the GCM Driver will
be useful. Having the ability to disable all communication with Google services
is.

However, this will have quite some side-effects on features that depend on the
GCM Driver. Not all of them will be able to handle such a scenario well, so it
wouldn't be an entirely trivial change.

[Ramin Halavati, 2017-02-21 14:30:40]

On 2017/02/21 14:25:43, Peter Beverloo wrote:
> lgtm, but please give either fgorski@ or zea@ some time (a day?) to give this
a
> read over if they have a chance.
> 
> I can't really judge whether having a policy for disabling the GCM Driver will
> be useful. Having the ability to disable all communication with Google
services
> is.
> 
> However, this will have quite some side-effects on features that depend on the
> GCM Driver. Not all of them will be able to handle such a scenario well, so it
> wouldn't be an entirely trivial change.

OK, thank you very much.

[fgorski, 2017-02-21 19:08:11]

lgtm

https://codereview.chromium.org/2708863002/diff/20001/components/gcm_driver/g...
File components/gcm_driver/gcm_channel_status_request.cc (right):

https://codereview.chromium.org/2708863002/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_request.cc:86: "adjust this interval
when it deems that necessary."
nit: how do you feel about dropping "that" from this sentence?

[Peter Beverloo, 2017-02-21 20:15:28]

https://codereview.chromium.org/2708863002/diff/20001/components/gcm_driver/g...
File components/gcm_driver/gcm_channel_status_request.cc (right):

https://codereview.chromium.org/2708863002/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_request.cc:86: "adjust this interval
when it deems that necessary."
On 2017/02/21 19:08:11, fgorski wrote:
> nit: how do you feel about dropping "that" from this sentence?

sgtm

[Ramin Halavati, 2017-02-22 12:13:40]

Comment addressed, landing.

https://codereview.chromium.org/2708863002/diff/20001/components/gcm_driver/g...
File components/gcm_driver/gcm_channel_status_request.cc (right):

https://codereview.chromium.org/2708863002/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_request.cc:86: "adjust this interval
when it deems that necessary."
On 2017/02/21 19:08:11, fgorski wrote:
> nit: how do you feel about dropping "that" from this sentence?

Done.

[Ramin Halavati, 2017-02-22 12:13:45]

The CQ bit was checked by rhalavati@chromium.org

[Ramin Halavati, 2017-02-22 12:13:46]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
fgorski@chromium.org
Link to the patchset: https://codereview.chromium.org/2708863002/#ps40001
(title: "nits")

[commit-bot: I haz the power, 2017-02-22 12:14:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 13:00:17]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487765625564140,
"parent_rev": "0d3e50126e2d830e0468ed77d0a99ff42635aa25", "commit_rev":
"85dc675418dd4b3ebc77a76c597d3ce5d8eb53b4"}

[commit-bot: I haz the power, 2017-02-22 13:00:53]

Description was changed from

==========
Network traffic annotation added to gcm_channel_status_request.

Network traffic annotation is added to network request of
gcm_channel_status_request.

BUG=656607
==========

to

==========
Network traffic annotation added to gcm_channel_status_request.

Network traffic annotation is added to network request of
gcm_channel_status_request.

BUG=656607

Review-Url: https://codereview.chromium.org/2708863002
Cr-Commit-Position: refs/heads/master@{#452018}
Committed:
https://chromium.googlesource.com/chromium/src/+/85dc675418dd4b3ebc77a76c597d...
==========

[commit-bot: I haz the power, 2017-02-22 13:00:54]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/85dc675418dd4b3ebc77a76c597d...