jupyter / enhancement-proposals

Enhancement proposals for the Jupyter Ecosystem
https://jupyter.org/enhancement-proposals
BSD 3-Clause "New" or "Revised" License
115 stars 65 forks source link

JEP: Websocket token authentication with subprotocols #121

Open minrk opened 5 months ago

minrk commented 5 months ago

JEP for #119

SylvainCorlay commented 3 months ago

I guess that the retry pattern will just be a bit more complicated when dealing with the aligned kernel subprotocol?

minrk commented 3 months ago

I guess that the retry pattern will just be a bit more complicated when dealing with the aligned kernel subprotocol?

I'll need to check. In all cases, the first request should include the kernel subprotocol and the token subprotocol, with the kernel subprotocol first indicating highest priority. There will be these possible cases to handle in terms of server support:

Which means the first request reliably determines whether the token can be in the subprotocol. If the first request fails, the token shall be in the URL parameter. The second request then determines kernel subprotocol support (the first request in current jupyterlab), and in the event of a server supporting neither subprotocol, a third and final request is needed to make a successful connection.

So if token auth is supported by the server, retries are no longer needed to check for support of the kernel subprotocol, which is a plus, I suppose. But in order to handle all possible cases, there could be up to 2 retries instead of the current 1, assuming I'm correct that clients can't meaningfully distinguish between a websocket that fails due to missing auth vs unsupported protocols. It could be simplified if the two failures turn out to be distinguishable in the client.

It may have been a good idea to define a subprotocol version string for the older wire format so that servers could explicitly declare that they only support the older wire format. But I'm not sure if that's worth discussing at this point, because that would reduce the number of cases where the retry is needed.

minrk commented 3 months ago

I've run some tests, and browsers don't appear to record the http response (essentially, the WebSocket in browser seems to pretend that websockets are not built on top of HTTP, so expose nothing about the HTTP requests/responses to client code). So clients need to treat all connection failures as indistinguishable:

I'm not necessarily proposing we do this, but so folks can have an informed opinion, if the client knows whether the token subprotocol is supported before attempting websocket requests, the conditions look like this:

So it is simpler, especially since the current subprotocol retries can be eliminated if the token subprocol is known or assumed to be supported. But it adds the preflight specification to somehow communicate that token-authenticated websockets are supported, which we haven't decided on, and don't currently have a mechanism for.

vidartf commented 3 months ago

@minrk is it possible to (ab)use the reason field in the close event? Or will that undermine some of the security considerations of websockets?

minrk commented 3 months ago

is it possible to (ab)use the reason field in the close event?

Yeah, I hadn't thought of that, but we could. I don't think it's abuse, I think it's actually what close code/reason are intended for. So instead of not accepting the connection in the first place, accept the connection and immediately close with a code (e.g. 4403 - 4000 + status code, since unregistered websocket codes should be in 4000-4999). This has an advantage in that it would actually give us a place for communicating the reason for the close, which is helpful, and maybe what we should have been doing all along.

There are backward-compatibility downsides to the transition, at least: