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

Add support for client/server without ICE #73

Merged
merged 15 commits into from
Dec 6, 2018
Merged

Add support for client/server without ICE #73

merged 15 commits into from
Dec 6, 2018

Conversation

pthatcherg
Copy link
Collaborator

Split RTCQuicTransort into QuicTransportBase, QuicTransport, and RTCQuicTransport
And remove RTC prefix from many things.

…uicTransport; And remove RTC prefix from many things.
This was referenced Nov 2, 2018
index.html Outdated
Let <var>quictransport</var> be a newly constructed
<code><a>QuicTransport</a></code> object.
</li>
<li>Let <var>quictransport</var> have a <dfn>[[\QuicTransportWritableStreams]]</dfn>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The page complains with duplicate definitions of QuicTransportWritableStreams:
"Duplicate definitions of '[[quictransportwritablestreams]]' at: 1."

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm.... how do I define slots in a base class that doesn't have a constructor? This goes for QuicTransportWritableStreams, QuicTransportReadableStreams, and QuicTransportState.

index.html Outdated
internal slot representing a sequence of <code><a>QuicWritableStream</a></code>
objects, initialized to empty.
</li>
<li>Let <var>quictransport</var> have a <dfn>[[\QuicTransportReadableStreams]]</dfn>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The page complains with duplicate definitions of QuicTransportReadableStreams:
"Duplicate definitions of '[[quictransportreadablestreams]]' at: 1."

index.html Outdated
objects, initialized to empty.
</li>
<li>
Let <var>quictransport</var> have a <dfn>[[\QuicTransportState]]</dfn>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also complains about the 3 definitions for QuicTransportState.

index.html Outdated
<section id="quictransport-operation*">
<h3>Operation</h3>
<p>A <code><a>QuicTransport</a></code> instance is constructed
using an URL which is constrained by CORS, just like a WebSocket.</p>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know you're purposely leaving the CORS explanation out for now, but should we at least add a reference to CORS here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a different mechanism for CORS instead (a QUIC transport parameters for web origins).

index.html Outdated Show resolved Hide resolved
index.html Outdated
Let <var>quictransport</var> have a <dfn>[[\QuicTransportState]]</dfn>
internal slot, initialized to <code>"connecting"</code>.
</li>
<li>Run these steps in parallel:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a spec language expert, but should this instead be:
"the user agent MUST run the following steps in parallel:"
?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also not sure if in parallel is the best language here. I'd like to hear others thoughts. What you're essentially trying to specify is that these steps are run off the main thread, correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The outer list already says "MUST run these steps"; this is an inner list in the outer list. It's the same as what's done in the websocket spec, which is what I was using as my model.

Similarly, "in parallel" is how the web socket spec explains it. But, yeah, it basically just means "don't do this on the js thread"

index.html Outdated Show resolved Hide resolved
@shampson
Copy link
Collaborator

shampson commented Nov 7, 2018

@aboba
Copy link
Collaborator

aboba commented Nov 12, 2018

I think this might make more sense as a separate specification. Since client/server is a distinct use case., it has a distinct set of architectural and privacy/security issues. Also, client/server might not fit within the charter of the WebRTC WG or ORTC CG.

@pthatcherg
Copy link
Collaborator Author

Bernard, I see your point about having it be a separate spec and working on it outside the WG. But if we do so, which specification would get the common/base stuff?

@aboba
Copy link
Collaborator

aboba commented Nov 13, 2018

@pthatcherg You can put the client-server material in the same directory (e.g. cs.html) for now and share links. Off the top of my head I’d say to put the common/base stuff into WebRTC-QUIC. Later if another WG adopts client-server QUIC you can copy/move files to the new repository.

@pthatcherg
Copy link
Collaborator Author

I added a mechanism for CORS (an origin transport parameter that the server can use to reject the connection, just like websocket's "origin: " header), and I fixed a typo.

The only thing Seth brought up that I don't know how to fix is the fact that we have internal slots that need to be defined for both subclasses, so maybe we should define that in the base class. But they are defined by the constructor, which the base class doesn't have.

Does anyone know the right way to do that in the WebIDL world?

@annevk
Copy link
Member

annevk commented Nov 13, 2018

Internal slots aren't defined through IDL, you need to use prose.

index.html Outdated
<!-- TODO: Register the transport parameter with IANA -->
and value equal to the value of an <a
href="https://fetch.spec.whatwg.org/#origin-header">origin
header used for HTTP fetching</a></li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be defined more formally. Basically you need to obtain an origin somehow (probably from the relevant global object) and then serialize it. I don't think you need to reference Fetch for this, but you will need to reference HTML.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks for pointing me in the right direction. Did I get it right this time?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, you want to use similar prose as is used to define self.origin, not define it in terms of self.origin. Relevant global object is also relative to some other object, which I'm guessing should be the object being constructed here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean I should the phrase "the global object's origin"? Or do I need to say something like "the origin of the global object relative to the QuicTransport being constructed"?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like

Let serializedOrigin be quictransport's relevant global object's origin, serialized.

and then link "relevant global object", "origin", and "serialized" to the appropriate concepts.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I went with that. But the examples I saw in https://html.spec.whatwg.org/multipage/webappapis.html all said "relevant settings object" instead of "relevant global object". I went with "relevant settings object". Is that OK? Or should I use "relevant global object"?

@pthatcherg
Copy link
Collaborator Author

When I said the "WebIDL world", perhaps I should have said the "the W3C". What's the right way to do this sort of thing? Is there precedent in other specs I can look at?

@annevk
Copy link
Member

annevk commented Nov 14, 2018

@pthatcherg usually it's "has an associated" or some such, e.g., see https://dom.spec.whatwg.org/#document.

@pthatcherg
Copy link
Collaborator Author

Ah, great, thanks for the help on the "has an associated" bit. I'll change to using that language.

@pthatcherg
Copy link
Collaborator Author

I realized a problem with the origin transport parameter: it's not encrypted. Is that OK (it's like ws://, but not like wss://)?

The alternative is that we'd have to do something like allocate a QUIC stream (say, stream ID 1) and send the origin over that. But that starts to get into the "defining protocol on top of QUIC" territory that I'd like to avoid. Plus, it requires another round trip.

@annevk
Copy link
Member

annevk commented Nov 15, 2018

Given that there's active effort to encrypt SNI, that sounds suboptimal.

@pthatcherg
Copy link
Collaborator Author

I put a TODO to address the issue that the origin transport parameter isn't encrypted.

@pthatcherg
Copy link
Collaborator Author

For privacy, I've made the origin value "null". The specs for the origin header indicate this is a valid value when in a "privacy-sensitive context". Someday, if QUIC gets encrypted transport parameters, I guess we could change it.

@annevk
Copy link
Member

annevk commented Nov 30, 2018

It sounds like you're reading the Origin RFC, browsers aren't using that (anymore). I think if anything you'd want to follow WebSockets here.

@pthatcherg
Copy link
Collaborator Author

WebSockets defined at https://html.spec.whatwg.org/multipage/web-sockets.html, right? That refers to RFC 6455 for the WebSocket protocol, which refers to RFC 6454 for the origin header, which (in section 7.3) says:

Whenever a user agent issues an HTTP request from a "privacy-
sensitive" context, the user agent MUST send the value "null" in the
Origin header field.

Is that not the latest set of standards to use? If not, what is the latest set of standards to use?

Put all the client/server stuff into a separate file.
@pthatcherg
Copy link
Collaborator Author

Bernard, I split the client/server parts into cs.html. See if that's what you had in mind. I'm sure I didn't get it quite right (especially with the boiler plate), so please let me know what I need to fix. I also wasn't sure how to make the p2p one dependent on the cs one.

@johnwason
Copy link

Does the ICE-free client-server option utilize the standard QUIC HTTP port with an upgrade system like websockets? Many firewalls will block anything that is a non-HTTP port.

Also, having an option to directly send and receive messages using the "streams as messages" concept without creating QuicTransportStreams would be helpful. For something like an online game that sends large numbers of time sensitive packets, using the current proposed design would cause significant overhead creating and destroying streams.

@aboba
Copy link
Collaborator

aboba commented Dec 3, 2018

@johnwason The client-server API is now available for inspection here:
https://w3c.github.io/webrtc-quic/cs.html

I am opening a new Issue based on your comment relating firewall traversal:
#91

I have moved your comment relating to unreliable transport to this issue:
#77

@annevk
Copy link
Member

annevk commented Dec 4, 2018

@pthatcherg it's a bit annoying, but for establishing a connection it references https://fetch.spec.whatwg.org/#concept-websocket-establish.

Anyway, I suppose you could simply have a bit here that simply checks the server is okay with a connection from a browser, but it does kinda go against the model the web platform has had to date, which is very much origin-centric.

@pthatcherg
Copy link
Collaborator Author

OK, so according to https://fetch.spec.whatwg.org/#concept-websocket-establish, you can only have a null origin if you have an "opaque origin", which is not our case. I guess I'll just put in an empty value for now instead of null.

Could we encrypt it using the same mechanism that encrypted SNI uses (apparently, DNS) but fallback to an empty value rather than an unencrypted value when it's not possible to use the same mechanism?

@pthatcherg
Copy link
Collaborator Author

I merged with master (which was perhaps a mistake) and now the entire PR is just the changes to the origin transport parameter :).

@aboba
Copy link
Collaborator

aboba commented Dec 6, 2018

Any reason we shouldn't merge this?

@aboba aboba closed this Dec 6, 2018
@pthatcherg
Copy link
Collaborator Author

What do you want me to do with the change I made to make it an origin of "" instead of "null"?

@aboba
Copy link
Collaborator

aboba commented Dec 6, 2018

@pthatcherg Can you apply the change to the cs.html file?

@aboba aboba reopened this Dec 6, 2018
@aboba aboba merged commit 958338e into master Dec 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants