-
Notifications
You must be signed in to change notification settings - Fork 184
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 alternative cookie storage #158
Comments
@sporkmonger see also #141 for a discussion on the matter :) |
Hi there, Is anyone interested on working on this ? I'd do it myslef unfortunately I have no experience with Redis nor SSO knowledge. Regards. |
Hi @sylr I don't believe anyone is working on this at the moment. We'd be happy to accept and work through any contributions on this front! |
This commit has an implementation of a Redis-backed cookie store I implemented in a fork of pusher/oauth2_proxy: lsst-dm/legacy-oauth2_proxy@2949456 This implementation introduces a ServerCookiesStore interface:
To which there's one implementation, a RedisCookiesStore. This implementation introduces the concept of a Ticket, which is composed of three parts:
Where:
On In I'm not sure if I will have time to get this into this repo, because I'm working with pusher/oauth2_proxy, but I've been using this with success (although I just did a rebase, and I need to test it some more). |
In the pusher fork of oauth2_proxy, a new session storage interface was implemented, along with a Redis session storage interface. Additional interfaces could be implemented, probably using the Redis interface as a template. It might be a decent chunk of work to merge these as development in the pusher/oauth2_proxy fork has been active lately, but these pull requests can give you an idea on what was changed: oauth2-proxy/oauth2-proxy#147 |
I'm starting to work on this now, will take a close look at the Redis stuff @brianv0 mentioned. |
I probably won't make this change in the upcoming PR, but I think there's a good case to be made to rename |
@sporkmonger that makes sense -- we've done some work to change up the configuration on the authenticator side with the intent that it would be easier to extend in the future. We haven't gotten to doing that yet on the proxy side. You might take a look there to get some idea of where the config options will be going, which should hopefully make it easier to supply configuration for various different session storage backends. |
I initially started porting the code @brianv0 mentioned, but ended up finding it hard to reason about the security guarantees it seemed to be trying to make. Opted to write from scratch with a simpler approach that should be easier to understand and reason about. |
The main security guarantee is that
The It may be more complicated than necessary. When I was implementing this, there was some concern that storing all sessions (and thus, tokens) in Redis without the sessions being individually encrypted would introduce operational risk that doesn't exist in when storing the sessions in cookies, namely that having access to Redis and the |
Took some doing, but I got Redis sessions working. PR coming soon. |
@jphines Some minor feedback: The |
Azure AD has massive tokens. It's obnoxious and unnecessary, but Microsoft isn't going to change this. Cookie compression helps, and cookie spanning solves most of the rest of the problem, but I still find that there are edge cases where it's a problem. For implementors that have these issues, I think it would be beneficial to just simply stop trying to do stateless session storage. If you can't reliably fit everything into 4kb, you really cannot guarantee that you won't have sign-in failures. Sometimes the failures are from browsers, sometimes it's from load balancers and other proxies enforcing their own limits. Regardless of where the limit is enforced, unfortunately, the user experience for a cookie size limit failure is bad.
I think SSO should support usage of a Redis backend. It ought to be pretty straightforward to point SSO to e.g. Elasticache Redis or an equivalent hosted option for most people who run into this issue, and that's going to be much more reliable than trying to jam the whole session directly into cookie state.
The text was updated successfully, but these errors were encountered: