-
Notifications
You must be signed in to change notification settings - Fork 218
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
Properly expire shiro sessions after timeout #1254
Conversation
ff2182c
to
d82d6a7
Compare
Expire JWT after the configured session expiration timeout.
d82d6a7
to
72d5bc0
Compare
|
||
if (!subject.getPrincipals().getRealmNames().contains("jwtRealm") | ||
&& !RequestUtils.getSessionTimeout().isNegative() | ||
&& subject.getSession().getStartTimestamp().before( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to "timeout" now, even if there were legitimate requests that would otherwise keep things alive, is that what you're going for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this was raised as a security concern. If the UI is running in a browser, the session will never timeout due to the background calls.
Not sure what's the usual practice, but requiring regular connections seems legit.
If the default timeout is too low, it can be raised to a higher value in config.
Do you have concerns with that approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, no particular concern, just making sure I understood. The session time is now a hard set number, no amount of application activity would keep it open and at some point the user would get logged out unexpectedly. Not the experience you'd want, but I don't think that's a big deal for the way Reaper's UI is used though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can make this an opt-in feature, so that the default behavior remains the same but users with stronger requirements can enable it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My guess is that changing the default to be more secure is fine. The trade here in my mind is a slight usability downgrade (which I think is actually unlikely to really impact folks) for a security requirement that would serve to keep running the tool viable in more strict environments.
Fixes #1225
Logs out after the configured session timeout, despite the session refresh that the UI does in the background.
I've also added an expiry date to the JWTs which uses the session timeout. So far, the JWTs we were generating never expired.