Network traffic annotation added to web_resource_service.

Network traffic annotation added to web_resource_service.

Network traffic annotation is added to network request of
web_resource_service.

BUG=656607
Review-Url: https://codereview.chromium.org/2717473002
Cr-Commit-Position: refs/heads/master@{#462026}
Committed: https://chromium.googlesource.com/chromium/src/+/24b32ef4c23c9603eba6a820c786377d6d80ef2c

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

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

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

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

[Robert Sesek, 2017-02-27 22:51:15]

Hm, I don't really know how to answer these questions. (I'm not really sure I
should be an OWNER here, since the part of this component I used to own has been
removed from Chromium). But the main issue is that this is kind of an abstract
component that is used by multiple clients to fetch web data, so the answers may
vary by caller.

[Ramin Halavati, 2017-02-28 10:13:11]

On 2017/02/27 22:51:15, Robert Sesek wrote:
> Hm, I don't really know how to answer these questions. (I'm not really sure I
> should be an OWNER here, since the part of this component I used to own has
been
> removed from Chromium). But the main issue is that this is kind of an abstract
> component that is used by multiple clients to fetch web data, so the answers
may
> vary by caller.

Thanks Robert, I checked for use cases of this class and it seems that it is not
used anywhere anymore.
I removed web_resource_service.h and web_resource_service.cc and updated the
BUILD.gn and it perfectly compiled for linux and android.
Do you think we can remove the class instead of annotating it?

[battre, 2017-02-28 19:37:14]

This might be a Chrome OS thing...

Am 28.02.2017 11:13 vorm. schrieb <rhalavati@chromium.org>:

> On 2017/02/27 22:51:15, Robert Sesek wrote:
> > Hm, I don't really know how to answer these questions. (I'm not really
> sure I
> > should be an OWNER here, since the part of this component I used to own
> has
> been
> > removed from Chromium). But the main issue is that this is kind of an
> abstract
> > component that is used by multiple clients to fetch web data, so the
> answers
> may
> > vary by caller.
>
> Thanks Robert, I checked for use cases of this class and it seems that it
> is not
> used anywhere anymore.
> I removed web_resource_service.h and web_resource_service.cc and updated
> the
> BUILD.gn and it perfectly compiled for linux and android.
> Do you think we can remove the class instead of annotating it?
>
> https://codereview.chromium.org/2717473002/
>

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

[Robert Sesek, 2017-02-28 20:51:18]

On 2017/02/28 10:13:11, Ramin Halavati wrote:
> On 2017/02/27 22:51:15, Robert Sesek wrote:
> > Hm, I don't really know how to answer these questions. (I'm not really sure
I
> > should be an OWNER here, since the part of this component I used to own has
> been
> > removed from Chromium). But the main issue is that this is kind of an
abstract
> > component that is used by multiple clients to fetch web data, so the answers
> may
> > vary by caller.
> 
> Thanks Robert, I checked for use cases of this class and it seems that it is
not
> used anywhere anymore.
> I removed web_resource_service.h and web_resource_service.cc and updated the
> BUILD.gn and it perfectly compiled for linux and android.
> Do you think we can remove the class instead of annotating it?

It's used at least here:
https://cs.chromium.org/chromium/src/chrome/browser/plugins/plugins_resource_....
I thought I spotted another use in ChromeOS (as battre notes) but it's not
showing up in cs now.

[Ramin Halavati, 2017-03-01 07:53:50]

On 2017/02/28 20:51:18, Robert Sesek wrote:
> On 2017/02/28 10:13:11, Ramin Halavati wrote:
> > On 2017/02/27 22:51:15, Robert Sesek wrote:
> > > Hm, I don't really know how to answer these questions. (I'm not really
sure
> I
> > > should be an OWNER here, since the part of this component I used to own
has
> > been
> > > removed from Chromium). But the main issue is that this is kind of an
> abstract
> > > component that is used by multiple clients to fetch web data, so the
answers
> > may
> > > vary by caller.
> > 
> > Thanks Robert, I checked for use cases of this class and it seems that it is
> not
> > used anywhere anymore.
> > I removed web_resource_service.h and web_resource_service.cc and updated the
> > BUILD.gn and it perfectly compiled for linux and android.
> > Do you think we can remove the class instead of annotating it?
> 
> It's used at least here:
>
https://cs.chromium.org/chromium/src/chrome/browser/plugins/plugins_resource_....
> I thought I spotted another use in ChromeOS (as battre notes) but it's not
> showing up in cs now.

Thank you Robert.

Dominic, do you suggest that I close this CL (and other cases that are only used
in ChromeOS) or replace annotation with NO_TRAFFIC_ANNOTATION_YET tag?

[Ramin Halavati, 2017-03-09 08:03:19]

The CQ bit was checked by rhalavati@chromium.org to run a CQ dry run

[Ramin Halavati, 2017-03-09 08:03:33]

The CQ bit was unchecked by rhalavati@chromium.org

[commit-bot: I haz the power, 2017-03-09 08:03:33]

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

[Ramin Halavati, 2017-03-09 08:03:40]

The CQ bit was checked by rhalavati@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 08:04:05]

Dry run: 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-03-09 08:10:03]

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

[commit-bot: I haz the power, 2017-03-09 08:10:03]

Dry run: Try jobs failed on following builders:
  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-03-09 08:56:23]

The CQ bit was checked by rhalavati@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 08:56:43]

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

[Ramin Halavati, 2017-03-09 08:57:45]

The CQ bit was checked by rhalavati@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 08:58:04]

Dry run: 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-03-09 09:12:35]

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

[commit-bot: I haz the power, 2017-03-09 09:12:35]

Dry run: Try jobs failed on following builders:
  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-03-09 09:21:50]

The CQ bit was checked by rhalavati@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 09:22:08]

Dry run: 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-03-09 09:38:59]

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

[commit-bot: I haz the power, 2017-03-09 09:39:00]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Ramin Halavati, 2017-03-09 10:00:16]

The CQ bit was checked by rhalavati@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 10:00:25]

Dry run: 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-03-09 10:57:17]

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

[commit-bot: I haz the power, 2017-03-09 10:57:18]

Dry run: This issue passed the CQ dry run.

[Ramin Halavati, 2017-03-09 11:51:23]

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

[Ramin Halavati, 2017-03-09 11:51:24]

rsesek@:
After trying several approaches, I finally moved the annotation to the
constructor of WebResourceService as it was needed in Windows build and it's a
priority for now. Please review components/web_resource/*

bauerb@:
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 plugins_resource_service.cc 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.

[Bernhard Bauer, 2017-03-14 10:07:55]

https://codereview.chromium.org/2717473002/diff/80001/chrome/browser/plugins/...
File chrome/browser/plugins/plugins_resource_service.cc (right):

https://codereview.chromium.org/2717473002/diff/80001/chrome/browser/plugins/...
chrome/browser/plugins/plugins_resource_service.cc:23: sender: "..."
pugins_resource_service

https://codereview.chromium.org/2717473002/diff/80001/chrome/browser/plugins/...
chrome/browser/plugins/plugins_resource_service.cc:24: description: "..."
Fetches updates to the list of plugins known to Chrome. This list contains for a
given plugin the minimum version not containing known security vulnerabilities,
and can be used to inform the user that their plugins need to be updated.

https://codereview.chromium.org/2717473002/diff/80001/chrome/browser/plugins/...
chrome/browser/plugins/plugins_resource_service.cc:25: trigger: "..."
Triggered at regular intervals (once per day).

https://codereview.chromium.org/2717473002/diff/80001/chrome/browser/plugins/...
chrome/browser/plugins/plugins_resource_service.cc:26: data: "..."
No data is sent as part of the request.

https://codereview.chromium.org/2717473002/diff/80001/chrome/browser/plugins/...
chrome/browser/plugins/plugins_resource_service.cc:27: destination:
WEBSITE/GOOGLE_OWNED_SERVICE/OTHER
GOOGLE_OWNED_SERVICE

https://codereview.chromium.org/2717473002/diff/80001/chrome/browser/plugins/...
chrome/browser/plugins/plugins_resource_service.cc:30: cookies_allowed:
false/true
false

https://codereview.chromium.org/2717473002/diff/80001/chrome/browser/plugins/...
chrome/browser/plugins/plugins_resource_service.cc:31: cookies_store: "..."
user

https://codereview.chromium.org/2717473002/diff/80001/chrome/browser/plugins/...
chrome/browser/plugins/plugins_resource_service.cc:39:
policy_exception_justification: "..."
Being able to identify plugins with known security vulnerabilities is a security
feature. How the browser _deals_ with vulnerable plugins can be configured via
the AllowOutdatedPlugins policy.

[Ramin Halavati, 2017-03-14 11:37:11]

Annotation updated, please review.

https://codereview.chromium.org/2717473002/diff/80001/chrome/browser/plugins/...
File chrome/browser/plugins/plugins_resource_service.cc (right):

https://codereview.chromium.org/2717473002/diff/80001/chrome/browser/plugins/...
chrome/browser/plugins/plugins_resource_service.cc:23: sender: "..."
On 2017/03/14 10:07:55, Bernhard Bauer wrote:
> pugins_resource_service

Done.

https://codereview.chromium.org/2717473002/diff/80001/chrome/browser/plugins/...
chrome/browser/plugins/plugins_resource_service.cc:24: description: "..."
On 2017/03/14 10:07:55, Bernhard Bauer wrote:
> Fetches updates to the list of plugins known to Chrome. This list contains for
a
> given plugin the minimum version not containing known security
vulnerabilities,
> and can be used to inform the user that their plugins need to be updated.

Done.

https://codereview.chromium.org/2717473002/diff/80001/chrome/browser/plugins/...
chrome/browser/plugins/plugins_resource_service.cc:25: trigger: "..."
On 2017/03/14 10:07:55, Bernhard Bauer wrote:
> Triggered at regular intervals (once per day).

Done.

https://codereview.chromium.org/2717473002/diff/80001/chrome/browser/plugins/...
chrome/browser/plugins/plugins_resource_service.cc:26: data: "..."
On 2017/03/14 10:07:55, Bernhard Bauer wrote:
> No data is sent as part of the request.

Done.

https://codereview.chromium.org/2717473002/diff/80001/chrome/browser/plugins/...
chrome/browser/plugins/plugins_resource_service.cc:27: destination:
WEBSITE/GOOGLE_OWNED_SERVICE/OTHER
On 2017/03/14 10:07:55, Bernhard Bauer wrote:
> GOOGLE_OWNED_SERVICE

Done.

https://codereview.chromium.org/2717473002/diff/80001/chrome/browser/plugins/...
chrome/browser/plugins/plugins_resource_service.cc:30: cookies_allowed:
false/true
On 2017/03/14 10:07:55, Bernhard Bauer wrote:
> false

Done.

https://codereview.chromium.org/2717473002/diff/80001/chrome/browser/plugins/...
chrome/browser/plugins/plugins_resource_service.cc:31: cookies_store: "..."
On 2017/03/14 10:07:55, Bernhard Bauer wrote:
> user

Done.

https://codereview.chromium.org/2717473002/diff/80001/chrome/browser/plugins/...
chrome/browser/plugins/plugins_resource_service.cc:39:
policy_exception_justification: "..."
On 2017/03/14 10:07:55, Bernhard Bauer wrote:
> Being able to identify plugins with known security vulnerabilities is a
security
> feature. How the browser _deals_ with vulnerable plugins can be configured via
> the AllowOutdatedPlugins policy.

Done.

[Bernhard Bauer, 2017-03-14 12:20:17]

https://codereview.chromium.org/2717473002/diff/100001/chrome/browser/plugins...
File chrome/browser/plugins/plugins_resource_service.cc (right):

https://codereview.chromium.org/2717473002/diff/100001/chrome/browser/plugins...
chrome/browser/plugins/plugins_resource_service.cc:39: AllowOutdatedPlugins:
true
This policy does not disable the network request to update the list of plugins,
it just silences local warnings.

[Ramin Halavati, 2017-03-14 12:25:46]

Comment addressed, please review.

https://codereview.chromium.org/2717473002/diff/100001/chrome/browser/plugins...
File chrome/browser/plugins/plugins_resource_service.cc (right):

https://codereview.chromium.org/2717473002/diff/100001/chrome/browser/plugins...
chrome/browser/plugins/plugins_resource_service.cc:39: AllowOutdatedPlugins:
true
On 2017/03/14 12:20:17, Bernhard Bauer wrote:
> This policy does not disable the network request to update the list of
plugins,
> it just silences local warnings.

Sorry, thank you.

[Bernhard Bauer, 2017-03-14 12:28:17]

lgtm

[Ramin Halavati, 2017-03-14 12:29:47]

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

[Ramin Halavati, 2017-03-14 12:29:48]

Thank you.

battre@, msramek@:
Do you have any comments?

[Ramin Halavati, 2017-04-04 08:39:46]

Robert,

Could you please review the 3 files in components/web_resource ?

[Robert Sesek, 2017-04-04 16:25:29]

lgtm

[msramek, 2017-04-05 09:31:42]

LGTM

[Ramin Halavati, 2017-04-05 09:40:44]

The CQ bit was checked by rhalavati@chromium.org

[commit-bot: I haz the power, 2017-04-05 09:41:05]

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-05 10:26:58]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1491385244083030,
"parent_rev": "0bcdfefa2ac3788c16294b423ba990227b8f8f0e", "commit_rev":
"24b32ef4c23c9603eba6a820c786377d6d80ef2c"}

[commit-bot: I haz the power, 2017-04-05 10:27:43]

Description was changed from

==========
Network traffic annotation added to web_resource_service.

Network traffic annotation is added to network request of
web_resource_service.

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

to

==========
Network traffic annotation added to web_resource_service.

Network traffic annotation is added to network request of
web_resource_service.

BUG=656607

Review-Url: https://codereview.chromium.org/2717473002
Cr-Commit-Position: refs/heads/master@{#462026}
Committed:
https://chromium.googlesource.com/chromium/src/+/24b32ef4c23c9603eba6a820c786...
==========

[commit-bot: I haz the power, 2017-04-05 10:27:44]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/24b32ef4c23c9603eba6a820c786...