|
|
Created:
3 years, 7 months ago by Ramin Halavati Modified:
3 years, 6 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, net-reviews_chromium.org, battre Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionNetwork 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
Patch Set 1 #Patch Set 2 : Annotation updated. #Messages
Total messages: 19 (7 generated)
rhalavati@chromium.org changed reviewers: + tyoshino@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 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.
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.
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.
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.
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@chromium.org changed reviewers: + ricea@chromium.org
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.
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.
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." })");
rhalavati@chromium.org changed reviewers: + msramek@chromium.org
Thank you Adam, annotation updated, please review. Martin, Any comments?
PS2 lgtm
LGTM
The CQ bit was checked by rhalavati@chromium.org
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": 20001, "attempt_start_ts": 1495190445751970, "parent_rev": "6416dea8ddbbd7a285da9ea31bb1a223d91f09c2", "commit_rev": "3e0003f69bab6a934d2a964248b20ba14b4e9c49"}
Message was sent while issue was closed.
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... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/3e0003f69bab6a934d2a964248b2... |