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

Overhaul session expiry and generalize token #9505

Closed
philippjfr opened this issue Dec 5, 2019 · 3 comments
Closed

Overhaul session expiry and generalize token #9505

philippjfr opened this issue Dec 5, 2019 · 3 comments

Comments

@philippjfr
Copy link
Contributor

philippjfr commented Dec 5, 2019

This issue attempts to summarize a discussion I had with @bryevdv about overhauling session expiry and provide a token containing a variety of information. The issue with the current mechanism for providing a signed sessions is that it is encoded as part of the Websocket URL which means that it ends up in logs and risks being leaked. Since session ids also never expire this adds additional risks because the other flaw can effectively be exploited until the server is restarted or shut down. Therefore we would like to overhaul both the mechanism by which the token is transmitted and add an expiration to it. This would be done by providing a JWT token to the client which it uses to authenticate itself. This token would then be transmitted using the Websocket subprotocol header.

The JWT token can additionally be used to transmit various information about the original request. One other problem with the bokeh server today is that when running behind a load balancer there is no way to access information about the request such as headers or cookies when the HTTP request and the Websocket connection arrive on different processes. The JWT token could therefore also be used to serialize a variety of information which can then be made available from the session on curdoc().session_context. By default the following should be made available:

  • Session ID
  • Token
  • Token Valid From time
  • Token Expiry Time
  • Authenticated User

We could then consider adding the entire request or providing a mechanism by which a user defines what should be made available, e.g. by providing a function which is given the request and returns a JSON serializable dictionary.

@philippjfr
Copy link
Contributor Author

philippjfr commented Dec 5, 2019

@bryevdv based on my (admittedly limited) googling the Javascript Websocket API doesn't actually support sending a token as part of the header in which case I'm not sure how much better we can do. I'll keep looking though.

@bryevdv
Copy link
Member

bryevdv commented Dec 5, 2019

@philippjfr There are several possibilities, some will require investigation/validation of assumptions:

  • slightly abuse the WS Sec-WebSocket-Protocol header (which is the only header available) to send a token. It was not really meant for this, but apparently people do it. i.e. passing the second arg to the websocket constructor:

     const ws = new Websocket(url, "foo") 
    

    Should generate a header Sec-WebSocket-Protocol: foo which we could use. A tiny proof of concept of this would be very useful as a first step and in particular we should try to determine if there are any de facto or de jure limitations on the subprotocol header length. This would be my preference, if it can be validated

  • Move the token authetication in to the protocol itself. I.e., always make a connection but don't create a session until the browser side has sent the auth token as a first websocket message. This complicatates the ws protocol/handshake but is probably not that bad, and is probably my second choice if the subprotocol header approach can't be used.

  • Store the token in a cookie. This is what Havoc originally proposed. I think this might also be workable but I have less of a feel for how it might work, or how it might change anything tangentially, so it's my least favorite "do something" option.

  • Lastly there is the "do nothing" option, i.e. leave the token in the ws or was URL. This is definitely less than ideals but if all else fails, as long we have a default and short expiry it might be something livable. With a short expiry the risk here is not so much unfettered access to the app, but that that token might be logged and might contain PII inside. Also this could make ws URLS extremely long.

@bryevdv
Copy link
Member

bryevdv commented Dec 6, 2019

@philippjfr Here is a POC of using theSec-WebSocket-Protocol header. So far I only tested on OSX (Safari, FF, Chrome) but it works to xmit at least a ~50kb string.

import tornado.ioloop
import tornado.web
import tornado.websocket
from tornado.template import Template

N = 50000

TEMPLATE = Template(f"""
<!DOCTYPE html>
<html>
<body>
  <script>
    var ws = new WebSocket("ws://localhost:8888/ws", ["bokeh", "a".repeat({N})]);
    ws.onmessage = function(evt) {{
      var m = JSON.parse(evt.data);
      console.log(m)
    }}
  </script>
</body>
</html>
""")

class MainHandler(tornado.web.RequestHandler):
    def get(self):
        self.write(TEMPLATE.generate())

class SimpleWebSocket(tornado.websocket.WebSocketHandler):
    def open(self):
        print("OPEN")

    def select_subprotocol(self, subprotocols):
        assert len(subprotocols) == 2
        assert subprotocols[0] == "bokeh"
        assert len(subprotocols[1]) == N
        print(f"token: {subprotocols[1][:20]}...")
        return "bokeh"

if __name__ == "__main__":
    app = tornado.web.Application(
        [(r"/", MainHandler), (r"/ws", SimpleWebSocket)]
    )
    app.listen(8888)
    tornado.ioloop.IOLoop.current().start()

We should test it out on Windows and Linux but I would expect it to work (I have not found any standards docs mentioning any size limit)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants