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 decorator for websockets authentication #124

Merged
merged 3 commits into from
Sep 15, 2020

Conversation

kinow
Copy link
Member

@kinow kinow commented Feb 23, 2020

These changes partially address #110

The default tornado.web.authenticated decorator, with the settings from JupyterHub that we have in our application, will try to redirected unauthenticated requests to the log-in form.

This redirect may succeed if the user is already logged-in, and is using a browser. But for a JS client initiating the WebSocket communication, it fails in the backend:

RuntimeError: Method not supported for Web Sockets
2020-02-24 10:51:05,126 tornado.application ERROR    Uncaught exception GET /user/kinow/subscriptions (127.0.0.1)
HTTPServerRequest(protocol='http', host='localhost:8000', method='GET', uri='/user/kinow/subscriptions', version='HTTP/1.1', remote_ip='127.0.0.1')
Traceback (most recent call last):
  File "/home/kinow/Development/python/workspace/cylc-uiserver/venv/lib/python3.7/site-packages/tornado/websocket.py", line 956, in _accept_connection
    open_result = handler.open(*handler.open_args, **handler.open_kwargs)
  File "/home/kinow/Development/python/workspace/cylc-uiserver/venv/lib/python3.7/site-packages/tornado/web.py", line 3162, in wrapper
    url = self.get_login_url()
  File "/home/kinow/Development/python/workspace/cylc-uiserver/venv/lib/python3.7/site-packages/jupyterhub/services/auth.py", line 830, in get_login_url
    state = self.hub_auth.set_state_cookie(self, next_url=self.request.uri)
  File "/home/kinow/Development/python/workspace/cylc-uiserver/venv/lib/python3.7/site-packages/jupyterhub/services/auth.py", line 701, in set_state_cookie
    handler.set_secure_cookie(cookie_name, b64_state, **kwargs)
  File "/home/kinow/Development/python/workspace/cylc-uiserver/venv/lib/python3.7/site-packages/tornado/web.py", line 716, in set_secure_cookie
    **kwargs
  File "/home/kinow/Development/python/workspace/cylc-uiserver/venv/lib/python3.7/site-packages/tornado/websocket.py", line 627, in _raise_not_supported_for_websockets
    raise RuntimeError("Method not supported for Web Sockets")
RuntimeError: Method not supported for Web Sockets

For the WebSocket client, it's better to simply fail, and avoid polluting users' logs. So here we simply return HTTP status code 403, and log in debug level that there was an unauthenticated request to WebSocket.

Requirements check-list

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Appropriate tests are included (unit and/or functional).
  • Appropriate change log entry included.
  • I have opened a documentation PR at Adding initial docs for websockets security cylc-doc#116

@kinow kinow self-assigned this Feb 23, 2020
@kinow kinow changed the title Add decorator for websockets auth Add decorator for websockets authentication Feb 23, 2020
@kinow kinow added this to the 0.3 milestone Feb 23, 2020
@kinow kinow linked an issue Feb 23, 2020 that may be closed by this pull request
@kinow
Copy link
Member Author

kinow commented Feb 23, 2020

What the logs look like if we use the tornado.web.authenticated and someone tries to open a WebSocket without logging in first.

image

def wrapper( # type: ignore
self: RequestHandler, *args, **kwargs
) -> Optional[Awaitable[None]]:
if not self.current_user:
Copy link
Member Author

Choose a reason for hiding this comment

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

Up to this point, this is the same as tornado.web.authenticated. Then after that we skip all the redirect and just log + return error.

@kinow kinow force-pushed the add-decorator-for-websockets-auth branch from c66580b to 2aabb16 Compare February 23, 2020 23:32
@kinow
Copy link
Member Author

kinow commented Feb 23, 2020

And what happens if someone tries to open the connection to our /subscriptions endpoint without authenticating first (i.e. no valid cookie in the request).

image

(ready state 3 means connection closed - https://developer.mozilla.org/en-US/docs/Web/API/WebSocket/readyState)

@codecov-io
Copy link

codecov-io commented Feb 23, 2020

Codecov Report

Merging #124 into master will increase coverage by 0.12%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #124      +/-   ##
==========================================
+ Coverage   52.56%   52.68%   +0.12%     
==========================================
  Files           6        6              
  Lines         371      372       +1     
  Branches       58       58              
==========================================
+ Hits          195      196       +1     
  Misses        173      173              
  Partials        3        3
Impacted Files Coverage Δ
cylc/uiserver/handlers.py 78.2% <100%> (+0.28%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d35ff4...5ba8f85. Read the comment docs.

@kinow
Copy link
Member Author

kinow commented Feb 24, 2020

Phew, all done! Works on my environment (famous last words), changelog created for 0.3 (which I also created and set a random release date), and a tentative documentation created too. Will ask for reviewers later after 0.2 release.

@kinow kinow force-pushed the add-decorator-for-websockets-auth branch from 5ba8f85 to adc7196 Compare February 26, 2020 21:05
@kinow kinow marked this pull request as ready for review July 9, 2020 22:33
@kinow kinow force-pushed the add-decorator-for-websockets-auth branch from adc7196 to e7fdf5f Compare July 29, 2020 02:21
@kinow
Copy link
Member Author

kinow commented Jul 29, 2020

Rebased.

@codecov-commenter
Copy link

codecov-commenter commented Jul 29, 2020

Codecov Report

Merging #124 into master will increase coverage by 0.12%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #124      +/-   ##
==========================================
+ Coverage   50.75%   50.88%   +0.12%     
==========================================
  Files           6        6              
  Lines         396      397       +1     
  Branches       64       64              
==========================================
+ Hits          201      202       +1     
  Misses        192      192              
  Partials        3        3              
Impacted Files Coverage Δ
cylc/uiserver/handlers.py 77.38% <100.00%> (+0.27%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7e1f6df...442e298. Read the comment docs.

Copy link
Member

@dwsutherland dwsutherland left a comment

Choose a reason for hiding this comment

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

LGTM, although I couldn't exactly reproduce the backend error..

I suppose I'd need to use a non-browser client I suppose?

The subscription/web-socket connection stays open when you logout at the moment..

@kinow
Copy link
Member Author

kinow commented Jul 29, 2020

I suppose I'd need to use a non-browser client I suppose?

Or create a JavaScript client, without being logged in to the UIS. Doing so, will fail as the handler won't have the authentication token.

The subscription/web-socket connection stays open when you logout at the moment..

Hmm, good point. I will test that, and if reproduced, will create an issue for that. This one should take care of authentication for the websockets, but not sure if it makes any difference after you logged out. Will confirm later with Cylc UI 👍

Thanks @dwsutherland !

Bruno P. Kinoshita added 3 commits August 4, 2020 10:24
@kinow kinow force-pushed the add-decorator-for-websockets-auth branch from e7fdf5f to 442e298 Compare August 3, 2020 22:25
@kinow
Copy link
Member Author

kinow commented Aug 3, 2020

Rebased

Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@hjoliver hjoliver merged commit b72960a into cylc:master Sep 15, 2020
@kinow kinow deleted the add-decorator-for-websockets-auth branch September 15, 2020 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Secure the WebSocket endpoint
5 participants