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

Support expiration for session creation #9074

Closed
bryevdv opened this issue Jul 13, 2019 · 11 comments
Closed

Support expiration for session creation #9074

bryevdv opened this issue Jul 13, 2019 · 11 comments

Comments

@bryevdv
Copy link
Member

bryevdv commented Jul 13, 2019

Encode an optional expiration timestamp with signed session id's and refuse to create the session if the id is expired. Propose adding server option --session-expiration-duration which will be added to UTC now() when the signed session id is created, and then checked against UTC now() when the session is to be created in the server.

@bryevdv bryevdv added this to the short-term milestone Jul 13, 2019
@bryevdv bryevdv changed the title Support expiration for signed sessions Support expiration for signed session ids Jul 13, 2019
@bryevdv bryevdv modified the milestones: short-term, 1.4 Aug 4, 2019
@bryevdv
Copy link
Member Author

bryevdv commented Sep 22, 2019

@bokeh/dev would appreciate some ideas here. I have pushed off #3458 to 2.0 since it is going to be a larger effort and possibly also have breaking changes. But I would like to introduce session expiration now in a relatively forward compatible way. With #3458 I propose to use JWT tokens which an have fields for "expiration" and "not before". Is just adding the server option --session-expiration-duration now the best plan? The value duration could be added to our current signed session id's in a ad-hoc way now, and be used inside JWT tokens once those are introduced.

@p-himik
Copy link
Contributor

p-himik commented Sep 22, 2019

I don't know if it's really relevant here, but maybe adding features to the existing authentication implementation is not worth it given that it doesn't rely on the state of the server.
A decent article describing the issues of JWT (IIRC Tornado uses signed cookies and not JWT, but for our purposes they're largely the same): http://cryto.net/~joepie91/blog/2016/06/13/stop-using-jwt-for-sessions/
But if creating an authentication functionality that relies on the server is out of the question, JWT is probably applicable. Just remember that you can't invalidate them and that you have to store them in cookies (so you have to keep them small).

@bryevdv
Copy link
Member Author

bryevdv commented Sep 22, 2019

@p-himik I have definitely seen articles that mention putting JWT tokens as Bearer tokens in the Authorization header, and I can't offhand think of a reason why that shouldn't work? Do you have a reference about them only being usable in cookies?

Edit: for instance at https://jwt.io/introduction/

Whenever the user wants to access a protected route or resource, the user agent should send the JWT, typically in the Authorization header using the Bearer schema.

FWIW here are some objectives, I would definitely welcome input on anything I am off-base about:

  • The current signed session ids are irrevocable. And worse, they have no expiry. Use session tokens in the auth header #3458 is about keeping tokens out of URLs, and this issue is about affording a capability for expiry so that if tokens are leaked damage can be mitigated

  • I'd like to keep the ability to open independent sessions in different browser tabs. Storing the tokens in a cookie as opposed to a header seems like it would contravene this? If we make the doc handler look for a cookie first every time, and one was set previously, now it will open that old session instead of making a new one. That does not seem desirable to me. I suppose if the request handler finds a cookie it woucl delete it immediately but seems to invite race conditions. Keeping things in headers seems more self contained and also easier to test

I'm 100% open to any solution that offers:

  • affords "not before" end "expiry" capability
  • keeps tokens/session ids out of URLs (as they are now)
  • does not leave users "logged in" so to speak (closing/reopening should generate a new session)

This is not an area of expertise for me so I welcome any corrections (e.g. maybe I just have some basic misunderstanding of how cookies would apply here)

@bryevdv
Copy link
Member Author

bryevdv commented Sep 22, 2019

@p-himik Also I think in the helpful article you linked that the situation here might actually fall fairly well under the final "So what are JWT tokens good for?" section:

Screen Shot 2019-09-22 at 1 21 44 PM

@p-himik
Copy link
Contributor

p-himik commented Sep 22, 2019

Full disclosure - I'm not a security expert as well. I have some anecdotal experience and have read a decent amount of articles on the topic. But don't take my word for granted.

The Authorization header is not preserved between requests by itself - you have to always set it yourself. This means that, if a service doesn't use cookies at all, when a user refreshes a tab they will be prompted to login again. To prevent that, you have to use cookies. And if you already use them and the authentication server is the same as the application server, there's no point in using a separate header. After all, it will force you to set it manually for each request.
But if the servers are different, it's another deal. You probably will still use cookies, only for the auth server. And all of the requests to the app server will have the Authorization header, just as that JWT Introduction page says.

The current signed session ids are irrevocable

And they continue to be, as long as Bokeh uses an authentication solution that doesn't rely on some state on some server (i.e. sessions in some kind of DB). But it should still be possible to implement expiration date without any issues.

a capability for expiry so that if tokens are leaked damage can be mitigated

If the real concern of this issue is to mitigate the damage, I don't think expiry is a good solution. After all, you will have to ask users to login again pretty often, regardless of whether the token has somehow been leaked or not.
A better solution IMO is to follow best practices where possible while keeping the implementation minimal and sufficient only for some really simple cases (i.e. when bokeh serve is enough). If a user requires some extra functionality (like mitigating damage from leaked tokens), they will have to roll out their own solution (embedding Bokeh, using a DB, allowing registration and password recovery, allowing signing out from all devices etc.)
But if you still want to add the expiry capability just for the sake of it, it's fine.

Storing the tokens in a cookie as opposed to a header seems like it would contravene this?

Yes. That's why Bokeh session ID and user (authentication) session ID must be different.

keeps tokens/session ids out of URLs (as they are now)

User session IDs must be passed to the auth server only inside cookies.
But I can see a scenario where having a Bokeh session in URL is beneficial. E.g. shared work on something. Of course, it would be a completely separate feature, partly because you'd probably want to be able to share the session only with some particular user while preventing others from using it even if the URL has gone public.
If Bokeh and user session IDs are kept separate on the client side but validated against each other on the backend side, I don't see any issue with having Bokeh session IDs in the URL. As an alternative to validation, we could use (user_name, bokeh_session_id) tuple for Bokeh session identification. This way, a different user will get a different session even if the Bokeh session ID is the same and shared in a URL.

does not leave users "logged in" so to speak (closing/reopening should generate a new session)

Why is this a desired behavior? Opening a new tab would require a user to login again. I can also see how just refreshing a page is used by users all the time to get the document at its initial state (the Reset button has its issues and limitations) - this workflow would definitely be disrupted and make such users furious.

the situation here might actually fall fairly well under the final "So what are JWT tokens good for?" section

Given my concern just above, I fail to see that.
Another place where some kind of token is applicable is when a server doesn't have any sort of permanent state for sessions. And that's exactly why Tornado has such authentication mechanism. Nobody that has a DB of any kind should use that mechanism.

@bryevdv
Copy link
Member Author

bryevdv commented Sep 22, 2019

@p-himik Thank you for this valuable discussion. I will need to take some time to digest fully. But I did want to quickly offer some additional context that may be missing:

First, all of this discussion really only applies to situations where authentication has happened upstream. I.e., using the new auth hooks recently made available. In that scenario, being authenticated means that the Bokeh server will agree to create a session. In that sense, the current session id (which I was proposing to use JWT tokens for instead) really are one-time just permission tokens.

In particular, in those scenarios, a user could stay logged in! It just means reloads get them new sessions on the Bokeh server:

  • User logs in, via whatever the app creator set up in the auth module
  • can load/embed a new (bokeh server) session
  • can reload page to get another new (bokeh server) session without relogging in

Here is video that illustrates this:

ScreenFlow

I reloaded several times, without having to log in. But I got a new (bokeh server) session each time. Logout/expiration of that side of things is handled by whatever the app creator implements in the auth module. As soon as the auth module hooks say a use is not logged in, a reload will shunt back to the login screen.

The important thing to note is that that bokeh session id/tokens are just one-time permissions and I think its good to keep it that way. They are orthogonal to user auth, really an internal detail, which is also why I think a short expiry is both useful and not a burden on users.

That covers a lot of ground, at least I think it is sufficient for a great many types of apps. What is given up?

  • you can't make changes to a bokeh app and reload and have those changes persist (without explicitly writing app code to sync to some external state). trying to implement that would be a huge problem, that I can't tackle. It's especially hard when you consider multiple bokeh servers behind a load balancer. Just going to have to punt here.

Does this change anything for you? FWIW I was mostly considering JWT because there are implementations of it.

@bryevdv
Copy link
Member Author

bryevdv commented Sep 22, 2019

Also I should add about the expiry. It is not expiration of the kind "boot the user out to a login screen after X amount of time". That is something we could try to figure out later. Here it is only "this one-time permission for (bokeh) session creation is good for the next X minutes and no longer"

Also maybe another succinct summary: Being authenticated is a an authorization to create new bokeh server sessions, for as long as you are logged in, and the session ids/tokens are the internal mechanism to convey permission to create those new sessions. (But the bokeh server itself does not specify policy or implementation for authorization, app authors do in the auth module)

Also worth mentioning: I consider session sharing ala google docs to be explicitly out of scope.

@p-himik
Copy link
Contributor

p-himik commented Sep 25, 2019

@bryevdv Thanks! I think I'm still a bit confused, although it's hard to express the exact reason.
But if you have a way to proceed, I'll probably just look at the resulting PR and see if I have any questions/concerns left.

@bryevdv
Copy link
Member Author

bryevdv commented Sep 25, 2019

Thanks @p-himik I think most of that is my lack of vocabulary and experience. I should have a simple PR for a pre-2.0 version in the next week or so.

@bryevdv bryevdv changed the title Support expiration for signed session ids Support expiration for session creation Oct 11, 2019
@bryevdv bryevdv modified the milestones: 1.4, 2.0 Oct 29, 2019
@bryevdv
Copy link
Member Author

bryevdv commented Oct 29, 2019

I think this is not going to be a huge effort but I want to clean up some functions that would end up mis-named without some changes. Rather than rush this in the release this week, pushing off to 2.0

@bryevdv
Copy link
Member Author

bryevdv commented Feb 14, 2020

This was closed in #9536

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