Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WebTransport #389

Closed
3 of 5 tasks
vasilvv opened this issue Jun 21, 2019 · 27 comments
Closed
3 of 5 tasks

WebTransport #389

vasilvv opened this issue Jun 21, 2019 · 27 comments
Assignees
Labels
Progress: propose closing we think it should be closed but are waiting on some feedback or consensus Provenance: Fugu Review type: CG early review An early review of general direction from a Community Group Topic: Architecture Architecture related topics Topic: networking Topic: Streams Any kind of stream-like feature Venue: IETF Proposed at the IETF, but not in a specific working group

Comments

@vasilvv
Copy link

vasilvv commented Jun 21, 2019

おはようTAG!

I'm requesting a TAG review of:

Further details:

You should also know that...

We'd prefer the TAG provide feedback as (please select one):

  • open issues in our GitHub repo for each point of feedback
  • open a single issue in our GitHub repo for the entire review
  • leave review feedback as a comment in this issue and @-notify @vasilvv
@dbaron dbaron added this to the 2019-07-03-telecon milestone Jun 22, 2019
@cynthia cynthia added Progress: unreviewed Review type: CG early review An early review of general direction from a Community Group Topic: networking Topic: Streams Any kind of stream-like feature Venue: IETF Proposed at the IETF, but not in a specific working group labels Jun 26, 2019
@ylafon ylafon self-assigned this Jun 26, 2019
@dbaron
Copy link
Member

dbaron commented Jul 3, 2019

A few very preliminary comments from spending an hour or two starting to look over the various documents:

From looking at the explainer, it seems like the "Detailed design discussion" and "Alternative designs considered" sections could perhaps be fleshed out a little bit more. One example of something I'd have liked to learn from those sections that I didn't would be how this proposal differs in scope from the previous RtcQuicTransport proposal; I think there are probably a bunch of other big picture things that are in your head, aren't obvious, and would be useful for the TAG to understand while reviewing.

It would likely also be useful for the explainer to explain how this spec relates to the WHATWG Streams specification.

It would also be useful to understand why the spec currently doesn't expose these in workers, and whether there's a future plan to do so.

Is there a section somewhere, either in the explainer or one of the two specs, that explains how the two specs (the one in WICG and the one in IETF) fit together?


A few very detailed comments on bits that I noticed while jumping around to look at some examples of things:

I'd note that the if (!!chunk) in one of the examples is a C++-ism and the !! is not idiomatic in JS (although not really needed in C++ either).

receiveDatagrams() may only be called once at a time. If a promised returned from a previous call is still unresolved, the user agent MUST return a new promise rejected with an InvalidStateError.

This seems a little bit odd, and likely to be a bit of a programming hazard (producing rejections in unexpected ways, depending on timing/races). Would it make more sense to return an empty sequence? That said... in the context of understanding why this method is asynchronous -- would it even be guaranteed to be empty? If it's asynchronous because the datagrams are on their way over from another thread or process... might asking for more not yield more?

Also, it seems like the type of receiveDatagrams is slightly wrong, since the prose says that it allows nulls in the result. It seems like it should be Promise<Sequence<Uint8Array?>>.

@dbaron
Copy link
Member

dbaron commented Jul 3, 2019

(I realize those detailed comments may quickly become irrelevant if there's a big refactor to integrate with the streams spec, as one of the other links suggested.)

@pthatcherg
Copy link

Responses to dbaron's comments from 10 days ago:

RTCQuicTransport is the p2p impl of a WebTransport. I made w3c/webtransport#41 to track making that more clear.

The explainer has now been updated to use WHATWG streams. We plan to update the spec to use them as well after some more discussion on how WHATWG streams should work (in particular for creating send streams and bidirectional streams).

We definitely want WebTransports to work in WebWorkers, we simply didn't think to say that explicitly.

The W3C spec references the IETF specs. Victor made w3c/webtransport#37 to track that.

I can't find any example of "!!". Perhaps that went away when we moved to WHATWG streams in the explainer.

On the nullability of receiveDatagrams(): you're right, that's a mistake. But we decided on a different approach that doesn't return null (see w3c/p2p-webtransport#124). We just need a PR for it.

On the goofiness of receiveDatagrams() only being called once at a time: that will probably go away with
the switch to WHATWG streams.

@dbaron
Copy link
Member

dbaron commented Sep 10, 2019

@cynthia and I are currently looking at this in a breakout.

So far I'd note that we found that there are additional documents that seem critical to understanding this, in particular:

@kenchris kenchris added the Progress: pending external feedback The TAG is waiting on response to comments/questions asked by the TAG during the review label Jan 14, 2020
@plinss
Copy link
Member

plinss commented Jan 15, 2020

dbaron to invite Martin Thomson to a future call

@alice alice removed this from the 2020-01-13-week milestone Jan 27, 2020
@cynthia
Copy link
Member

cynthia commented Mar 3, 2020

@ylafon and I discussed during our Wellington F2F.

We think this proposal is extremely interesting, and will bring a whole new set of capabilities to the web. However, we also consider this a pretty powerful feature - and was wondering about the implications of the constructor magically connecting, hence not being able to be a promise. (which would allow gating it behind a permission) We also had some concerns about port scanning through this mechanism (e.g. through timing based testing, albeit it would only work on UDP ports for now) but it does feel like gating this behind a permission would be good enough as a mitigation.

@cynthia
Copy link
Member

cynthia commented Mar 3, 2020

@plinss @dbaron ping on the Martin Thomson call issue noted above.

@aboba
Copy link

aboba commented Mar 4, 2020

@cynthia The WebTransport constructor was modeled after the WebSockets constructor which also immediately establishes a connection.

To address the port-scanning issue we could prevent WebTransport from connecting to ports on the "bad ports" list.

@cynthia
Copy link
Member

cynthia commented Mar 5, 2020

@aboba WebSockets is less powerful, and hence was never gated. This is a significantly more powerful API, so I think different considerations has to apply. As for the bad port blacklisting, those I believe are TCP ports; not sure if that blacklist would apply here.

@vasilvv
Copy link
Author

vasilvv commented Mar 5, 2020

What are the specific ways in which WebTransport is more powerful than WebSocket that are of security concern here?

@annevk
Copy link
Member

annevk commented May 14, 2020

w3c/webtransport#110 seems somewhat relevant for the TAG review. (Also applies to some of the other class names I realized, e.g., QuicTransport.)

At least from Mozilla's review of this I don't think we found reason to permission-gate this and even "bad ports" might not be needed due to the specifics of the networking handshake. @martinthomson might recall this better. Having said that, I believe @ricea still plans on doing a stream-based version of WebSocket and it seems this should be more analogous to that than to WebSocket itself.

@martinthomson
Copy link
Contributor

Yes, I think that we can avoid the port list and the permissions. Port scanning can be addressed by having the connection fail with a minimum timeout and no error codes that expose how it failed (maybe with some exceptions that we can carefully vet). After that, you leak no information until the server consents to communicate with this origin.

The big concerns are that we keep this and the new WebSocket stuff in sync so that they share what they need to (and that we don't do the same work twice). I'm assured that there is good coordination with Google between the teams, so that much is good.

@aboba
Copy link

aboba commented May 14, 2020

@martinthomson Are you referring to WebSocketStream? There are some differences, but they were closed as "won't fix":
w3c/webtransport#50
w3c/webtransport#51

@annevk
Copy link
Member

annevk commented May 14, 2020

In light of https://docs.google.com/document/d/1La1ehXw76HP6n1uUeks-WJGFgAnpX2tCjKts7QFJ57Y/edit those make sense I think, w3c/webtransport#54 appears unaddressed still.

@martinthomson
Copy link
Contributor

Not sure that "Peter talked to Adam" is sufficient for a decision, though I appreciate that this is a personal submission, so it does have a different standard.

Now sure how the WebSocketStream API is necessarily distinct based on the proposal.

@ylafon
Copy link
Member

ylafon commented May 26, 2020

Sangwhan and just discussed this and we are fine with not gating with a permission to avoid port scanning as long as there is no way to infer what is happening by doing time-based analysis (connecting to a filtered port takes far more time than a blocked one or one responding with another protocol).

@plinss plinss added this to the 2020-09-07-f2f milestone Aug 24, 2020
@cynthia
Copy link
Member

cynthia commented Sep 24, 2020

@dbaron and I discussed this during a breakout today, and concluded that given the above port scanning mitigations are in place we are happy to see this proposal move forward. Thanks for bringing this to our attention, and we plan to close this after discussing with the group.

We'd like the group to actively engage in review with relevant experts while development progresses. (In particular, networking is one of our weak points, so we might have missed something important.)

@cynthia cynthia added Progress: propose closing we think it should be closed but are waiting on some feedback or consensus and removed Progress: breakout labels Sep 24, 2020
@plinss plinss removed this from the 2020-09-21-F2F-Cork milestone Oct 14, 2020
@plinss plinss removed the Progress: pending external feedback The TAG is waiting on response to comments/questions asked by the TAG during the review label Oct 19, 2020
@plinss plinss added this to the 2020-10-19-week milestone Oct 19, 2020
@plinss plinss removed this from the 2020-10-19-week milestone Nov 9, 2020
@cynthia cynthia closed this as completed May 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Progress: propose closing we think it should be closed but are waiting on some feedback or consensus Provenance: Fugu Review type: CG early review An early review of general direction from a Community Group Topic: Architecture Architecture related topics Topic: networking Topic: Streams Any kind of stream-like feature Venue: IETF Proposed at the IETF, but not in a specific working group
Projects
None yet
Development

No branches or pull requests