Network traffic annotation added to url_provision_fetcher.

Network traffic annotation added to url_provision_fetcher.

Network traffic annotation is added to network request of
url_provision_fetcher.

BUG=656607
------------------------------------------

Review-Url: https://codereview.chromium.org/2717483002
Cr-Commit-Position: refs/heads/master@{#462805}
Committed: https://chromium.googlesource.com/chromium/src/+/4351859dff7a5920ead34d0f2195a052c4ad8d31

[Ramin Halavati, 2017-02-23 13:13:34]

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

[Ramin Halavati, 2017-02-23 13:13:35]

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 url_provision_fetcher 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.

[chcunningham, 2017-02-23 17:17:16]

chcunningham@chromium.org changed reviewers:
+ timav@chromium.org

[chcunningham, 2017-02-23 17:17:17]

Tima,  can you do this review. I'm not familiar with this class.

[Ramin Halavati, 2017-03-07 14:27:11]

On 2017/02/23 17:17:17, chcunningham wrote:
> Tima,  can you do this review. I'm not familiar with this class.

Hi,

A friendly ping on this.

[Tima Vaisburd, 2017-03-07 18:56:43]

Thank you for reminder!

https://codereview.chromium.org/2717483002/diff/20001/content/browser/media/u...
File content/browser/media/url_provision_fetcher.cc (right):

https://codereview.chromium.org/2717483002/diff/20001/content/browser/media/u...
content/browser/media/url_provision_fetcher.cc:37:
net::DefineNetworkTrafficAnnotation("...", R"(
I'm not familiar with NetworkTrafficAnnotation. What is the meaning of "...",
are you supposed to fill this in?

https://codereview.chromium.org/2717483002/diff/20001/content/browser/media/u...
content/browser/media/url_provision_fetcher.cc:58: traffic_annotation);
I would create another helper method like
   net::NetworkTrafficAnnotationTag CreateTrafficAnnotation(),
wdyt?

[Ramin Halavati, 2017-03-08 08:52:34]

Comments addressed, please review.

https://codereview.chromium.org/2717483002/diff/20001/content/browser/media/u...
File content/browser/media/url_provision_fetcher.cc (right):

https://codereview.chromium.org/2717483002/diff/20001/content/browser/media/u...
content/browser/media/url_provision_fetcher.cc:37:
net::DefineNetworkTrafficAnnotation("...", R"(
On 2017/03/07 18:56:43, Tima Vaisburd wrote:
> I'm not familiar with NetworkTrafficAnnotation. What is the meaning of "...",
> are you supposed to fill this in?

Sorry I have not been very clear in the message. I need you provide suitable
answers for all "..." positions, so that it will correctly specify why this
network connection is created and what it will do? You can refer to currently
existing examples in CS, also please tell me if I can add any clarification to
any of them.

https://codereview.chromium.org/2717483002/diff/20001/content/browser/media/u...
content/browser/media/url_provision_fetcher.cc:58: traffic_annotation);
On 2017/03/07 18:56:43, Tima Vaisburd wrote:
> I would create another helper method like
>    net::NetworkTrafficAnnotationTag CreateTrafficAnnotation(),
> wdyt?

We process this text using a clang tool that extracts it before compilation, and
we avoid any process on it during run-time, so that compiler would optimize them
out of binary. Therefore having a separate function is not preferred unless it
is really necessary or we need to create annotation in one function, then call
another one to continue with network request creation.
One alternative is using a constant outside the class (e.g. at line 17) that
defines this. How do you prefer it?

[Tima Vaisburd, 2017-03-09 00:16:34]

timav@chromium.org changed reviewers:
+ xhwang@chromium.org

[Tima Vaisburd, 2017-03-09 00:16:35]

+xhwang@

https://codereview.chromium.org/2717483002/diff/20001/content/browser/media/u...
File content/browser/media/url_provision_fetcher.cc (right):

https://codereview.chromium.org/2717483002/diff/20001/content/browser/media/u...
content/browser/media/url_provision_fetcher.cc:37:
net::DefineNetworkTrafficAnnotation("...", R"(
Thank you!

 semantics {
  sender: "ProvisionFetcher for Android MediaDrm"
  description: "Android MediaDrm (a part of encrypted content playback) requires
provisioning of the device. This is a request to provisioning server."
  trigger: "NotProvisionedException from MediaDrm"
  data: "Opaque request generated by Android framework"
  destination: GOOGLE_OWNED_SERVICE or OTHER (?)
 }
 policy {
  cookies_allowed: false
 }

I need xhwang@ to review this.

I do not quite understand the policy section, the request happens automatically
but normally only once for the current device. A user can cause this request
again by deleting the provisioning data from the device.

https://codereview.chromium.org/2717483002/diff/20001/content/browser/media/u...
content/browser/media/url_provision_fetcher.cc:58: traffic_annotation);
On 2017/03/08 08:52:34, Ramin Halavati wrote:
> On 2017/03/07 18:56:43, Tima Vaisburd wrote:
> > I would create another helper method like
> >    net::NetworkTrafficAnnotationTag CreateTrafficAnnotation(),
> > wdyt?
> 
> We process this text using a clang tool that extracts it before compilation,
and
> we avoid any process on it during run-time, so that compiler would optimize
them
> out of binary. Therefore having a separate function is not preferred unless it
> is really necessary or we need to create annotation in one function, then call
> another one to continue with network request creation.
> One alternative is using a constant outside the class (e.g. at line 17) that
> defines this. How do you prefer it?

I think it would be better to leave it the way it is for consistency with other
entries. I did not see other entries before my fist comment, sorry.

[Ramin Halavati, 2017-03-09 06:28:47]

Annotation updated. I left some comment inline, please review.

https://codereview.chromium.org/2717483002/diff/20001/content/browser/media/u...
File content/browser/media/url_provision_fetcher.cc (right):

https://codereview.chromium.org/2717483002/diff/20001/content/browser/media/u...
content/browser/media/url_provision_fetcher.cc:37:
net::DefineNetworkTrafficAnnotation("...", R"(
On 2017/03/09 00:16:35, Tima Vaisburd wrote:
> Thank you!
> 
>  semantics {
>   sender: "ProvisionFetcher for Android MediaDrm"
>   description: "Android MediaDrm (a part of encrypted content playback)
requires
> provisioning of the device. This is a request to provisioning server."
>   trigger: "NotProvisionedException from MediaDrm"
>   data: "Opaque request generated by Android framework"
>   destination: GOOGLE_OWNED_SERVICE or OTHER (?)
>  }
>  policy {
>   cookies_allowed: false
>  }
> 
> I need xhwang@ to review this.
> 
> I do not quite understand the policy section, the request happens
automatically
> but normally only once for the current device. A user can cause this request
> again by deleting the provisioning data from the device.

Done.

https://codereview.chromium.org/2717483002/diff/20001/content/browser/media/u...
content/browser/media/url_provision_fetcher.cc:58: traffic_annotation);
On 2017/03/09 00:16:35, Tima Vaisburd wrote:
> On 2017/03/08 08:52:34, Ramin Halavati wrote:
> > On 2017/03/07 18:56:43, Tima Vaisburd wrote:
> > > I would create another helper method like
> > >    net::NetworkTrafficAnnotationTag CreateTrafficAnnotation(),
> > > wdyt?
> > 
> > We process this text using a clang tool that extracts it before compilation,
> and
> > we avoid any process on it during run-time, so that compiler would optimize
> them
> > out of binary. Therefore having a separate function is not preferred unless
it
> > is really necessary or we need to create annotation in one function, then
call
> > another one to continue with network request creation.
> > One alternative is using a constant outside the class (e.g. at line 17) that
> > defines this. How do you prefer it?
> 
> I think it would be better to leave it the way it is for consistency with
other
> entries. I did not see other entries before my fist comment, sorry.

Done.

https://codereview.chromium.org/2717483002/diff/40001/content/browser/media/u...
File content/browser/media/url_provision_fetcher.cc (right):

https://codereview.chromium.org/2717483002/diff/40001/content/browser/media/u...
content/browser/media/url_provision_fetcher.cc:44: trigger:
"NotProvisionedException from MediaDrm"
Please be a little more specific for a not-software-engineer reader.

https://codereview.chromium.org/2717483002/diff/40001/content/browser/media/u...
content/browser/media/url_provision_fetcher.cc:45: data: "Opaque request
generated by Android framework"
Ditto.

https://codereview.chromium.org/2717483002/diff/40001/content/browser/media/u...
content/browser/media/url_provision_fetcher.cc:46: destination:
WEBSITE/GOOGLE_OWNED_SERVICE/OTHER
Isn't it sent to a website that belongs to Google?

https://codereview.chromium.org/2717483002/diff/40001/content/browser/media/u...
content/browser/media/url_provision_fetcher.cc:49: cookies_allowed: false/true
I don't see disabling cookies in the code. If you don't need them, I can create
a CL that disables it and then we set it to false.

https://codereview.chromium.org/2717483002/diff/40001/content/browser/media/u...
content/browser/media/url_provision_fetcher.cc:58:
policy_exception_justification: "..."
If there is no chrome policy implemented to prevent this, you can skip this
part.

[xhwang, 2017-03-11 07:12:00]

I don't feel code review is a good place to discuss the text. I shared a doc and
let's discuss offline first.

[Ramin Halavati, 2017-03-11 16:30:39]

On 2017/03/11 07:12:00, xhwang_slow wrote:
> I don't feel code review is a good place to discuss the text. I shared a doc
and
> let's discuss offline first.

Thanks Xiaohan, I will follow up the discussion there.
I should just note that the text here should not be very technical as it is
required to be understandable by people who are not software engineer.
Also if you feel that this request does different things if invoked by different
origins, I can move the annotation to URLProvisionFetcher's constructor and we
can annotate the origins instead.
You can also refer to a few sample annotations in CS for required detail level.

[Ramin Halavati, 2017-04-04 08:23:52]

Hi Xiaohan,

I updated the CL based on the discussion in doc, please review, and there are
some inline questions.

https://codereview.chromium.org/2717483002/diff/60001/content/browser/media/u...
File content/browser/media/url_provision_fetcher.cc (right):

https://codereview.chromium.org/2717483002/diff/60001/content/browser/media/u...
content/browser/media/url_provision_fetcher.cc:39: sender: "ProvisionFetcher for
Android MediaDrm"
This field should be a rather short and descriptive name of the module. Are any
of these suitable:
Content Decryption Module
Android MediaDrm.
MediaDrm Bridge

https://codereview.chromium.org/2717483002/diff/60001/content/browser/media/u...
content/browser/media/url_provision_fetcher.cc:56: destination: OTHER
My understanding is it may send to websites not owned by Google, am I correct?

https://codereview.chromium.org/2717483002/diff/60001/content/browser/media/u...
content/browser/media/url_provision_fetcher.cc:59: cookies_allowed: false/true
It seems to me that cookies are not specifically disabled here. On desktop the
default state is enabled, if it is the same on Android, can I add another CL
that specifically disables the cookies?

[xhwang, 2017-04-04 17:08:31]

xhwang@chromium.org changed reviewers:
+ slan@chromium.org

[xhwang, 2017-04-04 17:08:32]

+slan: please see the question.

https://codereview.chromium.org/2717483002/diff/60001/content/browser/media/u...
File content/browser/media/url_provision_fetcher.cc (right):

https://codereview.chromium.org/2717483002/diff/60001/content/browser/media/u...
content/browser/media/url_provision_fetcher.cc:39: sender: "ProvisionFetcher for
Android MediaDrm"
On 2017/04/04 08:23:52, Ramin Halavati wrote:
> This field should be a rather short and descriptive name of the module. Are
any
> of these suitable:
> Content Decryption Module
> Android MediaDrm.
> MediaDrm Bridge

This is also used on ChromeCast so "Content Decryption Module" is the best fit.

https://codereview.chromium.org/2717483002/diff/60001/content/browser/media/u...
content/browser/media/url_provision_fetcher.cc:56: destination: OTHER
On 2017/04/04 08:23:52, Ramin Halavati wrote:
> My understanding is it may send to websites not owned by Google, am I correct?

Currently it's mostly Google sites, but this can change in the future. And
fundamentally, Chromium is designed to support different CDMs, which might use
different websites (including those not owned by Google) to do provisioning.

So OTHER sgtm.

https://codereview.chromium.org/2717483002/diff/60001/content/browser/media/u...
content/browser/media/url_provision_fetcher.cc:59: cookies_allowed: false/true
On 2017/04/04 08:23:52, Ramin Halavati wrote:
> It seems to me that cookies are not specifically disabled here. On desktop the
> default state is enabled, if it is the same on Android, can I add another CL
> that specifically disables the cookies?

Sure. Thank you!

https://codereview.chromium.org/2717483002/diff/60001/content/browser/media/u...
content/browser/media/url_provision_fetcher.cc:64: "Android settings, under Site
Settings, Media."
Again, this setting is for Android only. I am not sure whether we have a
permission or have any way to block this on ChromeCast. +slan

On Android, users can disable this feature by either:
- block Protected Media Identifier permissions prompt, or
- disable "Protected Media Identifier permission" via "Android settings, under
Site Settings, Media, Protected Media."

I am not sure how much detail you want to get into. Maybe just something like:

On Android, users can disable this feature by disabling Protected Media
Identifier permissions.

[slan, 2017-04-04 19:30:20]

https://codereview.chromium.org/2717483002/diff/60001/content/browser/media/u...
File content/browser/media/url_provision_fetcher.cc (right):

https://codereview.chromium.org/2717483002/diff/60001/content/browser/media/u...
content/browser/media/url_provision_fetcher.cc:39: sender: "ProvisionFetcher for
Android MediaDrm"
On 2017/04/04 17:08:32, xhwang_slow wrote:
> On 2017/04/04 08:23:52, Ramin Halavati wrote:
> > This field should be a rather short and descriptive name of the module. Are
> any
> > of these suitable:
> > Content Decryption Module
> > Android MediaDrm.
> > MediaDrm Bridge
> 
> This is also used on ChromeCast so "Content Decryption Module" is the best
fit.

+1

https://codereview.chromium.org/2717483002/diff/60001/content/browser/media/u...
content/browser/media/url_provision_fetcher.cc:64: "Android settings, under Site
Settings, Media."
On 2017/04/04 17:08:32, xhwang_slow wrote:
> Again, this setting is for Android only. I am not sure whether we have a
> permission or have any way to block this on ChromeCast. +slan
> 
> On Android, users can disable this feature by either:
> - block Protected Media Identifier permissions prompt, or
> - disable "Protected Media Identifier permission" via "Android settings, under
> Site Settings, Media, Protected Media."
> 
> I am not sure how much detail you want to get into. Maybe just something like:
> 
> On Android, users can disable this feature by disabling Protected Media
> Identifier permissions.

We don't have support for disabling protected media on Cast, nor do we plan on
exposing anything like this to users. Xiaohan's suggestion SGTM.

[Ramin Halavati, 2017-04-05 09:12:06]

All comments addressed. Please review.

https://codereview.chromium.org/2717483002/diff/60001/content/browser/media/u...
File content/browser/media/url_provision_fetcher.cc (right):

https://codereview.chromium.org/2717483002/diff/60001/content/browser/media/u...
content/browser/media/url_provision_fetcher.cc:39: sender: "ProvisionFetcher for
Android MediaDrm"
On 2017/04/04 19:30:20, slan wrote:
> On 2017/04/04 17:08:32, xhwang_slow wrote:
> > On 2017/04/04 08:23:52, Ramin Halavati wrote:
> > > This field should be a rather short and descriptive name of the module.
Are
> > any
> > > of these suitable:
> > > Content Decryption Module
> > > Android MediaDrm.
> > > MediaDrm Bridge
> > 
> > This is also used on ChromeCast so "Content Decryption Module" is the best
> fit.
> 
> +1

Done.

https://codereview.chromium.org/2717483002/diff/60001/content/browser/media/u...
content/browser/media/url_provision_fetcher.cc:56: destination: OTHER
On 2017/04/04 17:08:32, xhwang_slow wrote:
> On 2017/04/04 08:23:52, Ramin Halavati wrote:
> > My understanding is it may send to websites not owned by Google, am I
correct?
> 
> Currently it's mostly Google sites, but this can change in the future. And
> fundamentally, Chromium is designed to support different CDMs, which might use
> different websites (including those not owned by Google) to do provisioning.
> 
> So OTHER sgtm.

Done.

https://codereview.chromium.org/2717483002/diff/60001/content/browser/media/u...
content/browser/media/url_provision_fetcher.cc:59: cookies_allowed: false/true
On 2017/04/04 17:08:32, xhwang_slow wrote:
> On 2017/04/04 08:23:52, Ramin Halavati wrote:
> > It seems to me that cookies are not specifically disabled here. On desktop
the
> > default state is enabled, if it is the same on Android, can I add another CL
> > that specifically disables the cookies?
> 
> Sure. Thank you!

Done in https://codereview.chromium.org/2804563002

https://codereview.chromium.org/2717483002/diff/60001/content/browser/media/u...
content/browser/media/url_provision_fetcher.cc:64: "Android settings, under Site
Settings, Media."
On 2017/04/04 19:30:20, slan wrote:
> On 2017/04/04 17:08:32, xhwang_slow wrote:
> > Again, this setting is for Android only. I am not sure whether we have a
> > permission or have any way to block this on ChromeCast. +slan
> > 
> > On Android, users can disable this feature by either:
> > - block Protected Media Identifier permissions prompt, or
> > - disable "Protected Media Identifier permission" via "Android settings,
under
> > Site Settings, Media, Protected Media."
> > 
> > I am not sure how much detail you want to get into. Maybe just something
like:
> > 
> > On Android, users can disable this feature by disabling Protected Media
> > Identifier permissions.
> 
> We don't have support for disabling protected media on Cast, nor do we plan on
> exposing anything like this to users. Xiaohan's suggestion SGTM.

Done.

[xhwang, 2017-04-05 16:31:24]

lgtm

[slan, 2017-04-05 17:01:59]

lgtm

[Tima Vaisburd, 2017-04-05 18:19:31]

lgtm

[Ramin Halavati, 2017-04-06 07:21:16]

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

[Ramin Halavati, 2017-04-06 07:21:17]

Thank you very much.

Martin,
Any comments?

[msramek, 2017-04-06 13:56:09]

LGTM

[Ramin Halavati, 2017-04-06 14:01:03]

The CQ bit was checked by rhalavati@chromium.org

[commit-bot: I haz the power, 2017-04-06 14:01:21]

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-04-06 14:03:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-06 14:03:20]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Ramin Halavati, 2017-04-07 06:47:20]

The CQ bit was checked by rhalavati@chromium.org

[commit-bot: I haz the power, 2017-04-07 06:47:38]

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-04-07 06:50:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-07 06:50:17]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Ramin Halavati, 2017-04-07 07:03:49]

The CQ bit was checked by rhalavati@chromium.org

[Ramin Halavati, 2017-04-07 07:03:49]

The patchset sent to the CQ was uploaded after l-g-t-m from slan@chromium.org,
timav@chromium.org, xhwang@chromium.org, msramek@chromium.org
Link to the patchset: https://codereview.chromium.org/2717483002/#ps100001
(title: "Conflict resolved.")

[commit-bot: I haz the power, 2017-04-07 07:04:02]

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-04-07 08:05:15]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491548629625090,
"parent_rev": "80f5122d11253e8c76379813a5352feb024b9ea2", "commit_rev":
"4351859dff7a5920ead34d0f2195a052c4ad8d31"}

[commit-bot: I haz the power, 2017-04-07 08:06:06]

Description was changed from

==========
Network traffic annotation added to url_provision_fetcher.

Network traffic annotation is added to network request of
url_provision_fetcher.

BUG=656607
------------------------------------------
==========

to

==========
Network traffic annotation added to url_provision_fetcher.

Network traffic annotation is added to network request of
url_provision_fetcher.

BUG=656607
------------------------------------------

Review-Url: https://codereview.chromium.org/2717483002
Cr-Commit-Position: refs/heads/master@{#462805}
Committed:
https://chromium.googlesource.com/chromium/src/+/4351859dff7a5920ead34d0f2195...
==========

[commit-bot: I haz the power, 2017-04-07 08:06:12]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/4351859dff7a5920ead34d0f2195...