Network traffic annotation added to websocket_stream.

Network traffic annotation added to websocket_stream.

Network traffic annotation is added to network request of:
net/websockets/websocket_stream.cc.

BUG=656607
Review-Url: https://codereview.chromium.org/2846793003
Cr-Commit-Position: refs/heads/master@{#473170}
Committed: https://chromium.googlesource.com/chromium/src/+/3e0003f69bab6a934d2a964248b20ba14b4e9c49

[Ramin Halavati, 2017-04-27 12:10:26]

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

[Ramin Halavati, 2017-04-27 12:10:27]

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

[Ramin Halavati, 2017-05-03 07:33:36]

On 2017/04/27 12:10:27, Ramin Halavati wrote:
> 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 websocket_stream.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.

Hi Takeshi,

Just gently reminding this.

[tyoshino (SeeGerritForStatus), 2017-05-08 10:04:59]

On 2017/05/03 07:33:36, Ramin Halavati wrote:
> On 2017/04/27 12:10:27, Ramin Halavati wrote:
> > 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 websocket_stream.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.

I'd like to understand the design of the mechanism and the goal more.

What level of granularity is expected? E.g. in case there are both Blink code
and non-Blink code that are using WebSocket, the annotation should be separated
for each of them?

Is it for all kinds of network traffic? WebSocket uses net::URLRequest to issue
a WebSocket handshake but actually it's not an HTTP request.

Is this expected to be stable forever?

Does the ID space intentionally have a flat structure? E.g. have you considered
giving some tree structure for it like UMA?

> > 
> > 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.
> 
> Hi Takeshi,
> 
> Just gently reminding this.

[Ramin Halavati, 2017-05-08 10:51:47]

On 2017/05/08 10:04:59, tyoshino (OOO May15-19) wrote:
> On 2017/05/03 07:33:36, Ramin Halavati wrote:
> > On 2017/04/27 12:10:27, Ramin Halavati wrote:
> > > 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 websocket_stream.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.
> 
> I'd like to understand the design of the mechanism and the goal more.
> 
> What level of granularity is expected? E.g. in case there are both Blink code
> and non-Blink code that are using WebSocket, the annotation should be
separated
> for each of them?
> 
> Is it for all kinds of network traffic? WebSocket uses net::URLRequest to
issue
> a WebSocket handshake but actually it's not an HTTP request.
> 
> Is this expected to be stable forever?
> 
> Does the ID space intentionally have a flat structure? E.g. have you
considered
> giving some tree structure for it like UMA?
> 
> > > 
> > > 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.
> > 
> > Hi Takeshi,
> > 
> > Just gently reminding this.

Hi Takeshi,

As you have seen we still don't have a very precise style guide for annotations
and we hope it would be fine tuned when the first round of annotations gets
reviewed. In cases where a function has different use cases based on input
arguments, it's usually better to pass the annotation to it instead of writing a
broad one. I am not quite familiar with your case of Blink and none-Blink, but
it seems that a separation would be good.

Annotations are for all kind of network requests, because even in case of
handshakes, we should be able to answer why a network request is made.

We expect that when codes get updated, annotations would also get updated. We
still have no mechanism for that.

Unique ids are just for keeping track of annotations and have no meaningful
usage yet. We are planning to add a tree structure to them for cases that an
annotation cannot be defined completely in one function, but until then, they
are flat.

Please tell me if my answers are not clear, or let's have a GVC if it helps.

[tyoshino (SeeGerritForStatus), 2017-05-12 10:11:44]

Description was changed from

==========
Network traffic annotation added to websocket_stream.

Network traffic annotation is added to network request of:
net/websockets/websocket_stream.cc.

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

to

==========
Network traffic annotation added to websocket_stream.

Network traffic annotation is added to network request of:
net/websockets/websocket_stream.cc.

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

[tyoshino (SeeGerritForStatus), 2017-05-12 10:11:44]

tyoshino@chromium.org changed reviewers:
+ ricea@chromium.org

[tyoshino (SeeGerritForStatus), 2017-05-12 10:13:14]

Thank you, Ramin for answering the questions.

Sorry but I couldn't have enough time to fill the boxes and going to enter 1
week vacation.

Adding ricea@ as he might be able to respond earlier though he's also out of
office for the early next week.

[Ramin Halavati, 2017-05-12 10:41:26]

On 2017/05/12 10:13:14, tyoshino (OOO May15-19) wrote:
> Thank you, Ramin for answering the questions.
> 
> Sorry but I couldn't have enough time to fill the boxes and going to enter 1
> week vacation.
> 
> Adding ricea@ as he might be able to respond earlier though he's also out of
> office for the early next week.

OK, have a nice vacation.

ricea@,
Please refer to the first message for intro, or let's have a chat if it helps.

[Adam Rice, 2017-05-19 07:23:45]

If I understand correctly, this should be very similar to the tag used by
ResourceDispatcherHostImpl. Here's a draft:

constexpr net::NetworkTrafficAnnotationTag kTrafficAnnotation =
    net::DefineNetworkTrafficAnnotation("websocket_stream", R"(
        semantics {
          sender: "WebSocket Handshake"
          description:
            "Renderer process initiated WebSocket handshake. The WebSocket "
            "handshake is used to establish a connection between a web page "
            "and a consenting server for bi-directional communication."
          trigger:
            "A handshake is performed every time a new connection is "
            "established via the Javascript or PPAPI WebSocket API. Any web "
            "page or extension can create a WebSocket connection."
          data: "The path and sub-protocols requested when the WebSocket was "
                "created, plus the origin of the creating page."
          destination: OTHER
        }
        policy {
          cookies_allowed: true
          cookies_store: "user or per-app cookie store"
          setting: "These requests cannot be disabled."
          policy_exception_justification:
            "Not implemented. WebSocket is a core web platform API."
        })");

[Ramin Halavati, 2017-05-19 07:29:58]

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

[Ramin Halavati, 2017-05-19 07:29:59]

Thank you Adam, annotation updated, please review.

Martin,
Any comments?

[Adam Rice, 2017-05-19 07:43:49]

PS2 lgtm

[msramek, 2017-05-19 10:40:23]

LGTM

[Ramin Halavati, 2017-05-19 10:40:45]

The CQ bit was checked by rhalavati@chromium.org

[commit-bot: I haz the power, 2017-05-19 10:41:06]

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-05-19 12:54:52]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1495190445751970,
"parent_rev": "6416dea8ddbbd7a285da9ea31bb1a223d91f09c2", "commit_rev":
"3e0003f69bab6a934d2a964248b20ba14b4e9c49"}

[commit-bot: I haz the power, 2017-05-19 12:55:04]

Description was changed from

==========
Network traffic annotation added to websocket_stream.

Network traffic annotation is added to network request of:
net/websockets/websocket_stream.cc.

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

to

==========
Network traffic annotation added to websocket_stream.

Network traffic annotation is added to network request of:
net/websockets/websocket_stream.cc.

BUG=656607

Review-Url: https://codereview.chromium.org/2846793003
Cr-Commit-Position: refs/heads/master@{#473170}
Committed:
https://chromium.googlesource.com/chromium/src/+/3e0003f69bab6a934d2a964248b2...
==========

[commit-bot: I haz the power, 2017-05-19 12:55:05]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/3e0003f69bab6a934d2a964248b2...