Redis session store

[brianv0]

Description

Implements a redis-backed session store.
Cookies are implemented as tickets.

A ticket is composed of:

{CookieName}-{ticketID}.{secret}

Where:

• the CookieName is the OAuth2 cookie name (_oauth2_proxy by default)
• the ticketID is a 128 bit random number, hex-encoded
• the secret is a 128 bit random number, base64 encoded

The pair of {CookieName}-{ticketID} comprises a ticket handle, and thus, the redis key. The handle is used to store an encoded session in redis via SETEX. The encoded session is encrypted according to the secret, which is unique for every session. This preserves the security aspects of oauth2_proxy.

Motivation and Context

#138

How Has This Been Tested?

The session storage has been added to the suites for a session store. An additional behavior has been added to ensure the behavior that persistent keys, when cleared, are actually cleared.

Checklist:

• My change requires a change to the documentation or CHANGELOG.
• I have updated the documentation/CHANGELOG accordingly.
• I have created a feature (non-master) branch for my PR.

[JoelSpeed]

This is great, though quite big 🙈 I've left some feedback on it inline, let me know what you think

[JoelSpeed]

This sentence doesn't make sense I don't think?

[JoelSpeed]

@brianv0 In order to push forward with this mini project, I did some work to rebase, clean up and integrate this code with the rest of the proxy, I've pushed my changes to the redis-session-store branch. I'm going to build an image from my branch and do some extended real world testing.

I've been very granular with the commits I've added on top of yours, I would suggest you reset your branch to the head of my branch, push that to your fork so that we can maintain this PR and your attribution for the feature, and perhaps squash some of the commits as you see fit?

I also did end up adding cookie signing to the redis implementation. We talked internally and decided that we would like to make signed cookies a requirement for all implementations to try and prevent brute force attacks on backends by guessing potential ticket names. There is a small performance impact though I think this should be minimal.

Let me know if you have any questions about what I've done

[brianv0]

Thanks, you beat me to his looks great. I'm fine with the commit granularity and I don't feel a need to squash them but I'd be happy too if you want.

On the loadSessionFromString, I think we'd like to eventually make this public as a feature related to the JWT passthrough, but we are happy to keep that as a feature on a fork if it's not desirable in the code base. We allow another application to write to the redis store and send back a ticket as a way of issuing access tokens. But like I said, with this feature and the code in #65 (or some fixes of it), we can apply our own patches and be happy.

I will do some testing but I might not be able to do much until next Monday.

[brianv0]

I updated the documentation and changelog. I wasn't sure if we should mention the storage type of redis in the help string for session-store-type, if there are some preferences on that.

[JoelSpeed]

Hey @brianv0, I'm out at KubeCon this week so testing may be limited here too

I've added a couple of suggestions to improve the docs and still have one outstanding question about the clearing behaviour. Thanks for updating the docs btw, everything else is looking great now!

[JoelSpeed]

Was just trying to test this with a redis-sentinel cluster, then found out you have to set sentinel up slightly differently so just added a commit to my branch that adds sentinel support, gonna test that now

@brianv0 you may want to cherry pick the above linked commit if you think it's good?

[JoelSpeed]

Going to be testing this today, quay.io/pusher/oauth2_proxy:pull-155 now points to 11a7ecf, will report back with the results of my testing

[JoelSpeed]

I found a bug when saving sessions where an existing expired session was present so I've added a test case and fixed it in 9dc1a96, pushing an updated image for retesting

[JoelSpeed]

The result of my testing today has lead to another commit to be cherrypicked 48edce3, This one adds a test that the session should be stored in the persistent storage until the CookieExpire has elapsed. The setting of CookieExpire should really be SessionExpire but it has historically been used as a setting for users to set how long the session should be refreshable for. If we set it shorter than this period, say to the sessions actual duration, we can't use the refreshing feature of some of the providers, I think this fixes that issue (at least in my testing it does), WDYT @brianv0?

[JoelSpeed]

I forgot to mention, I think it is working now!!! Going to do a more widespread test across the Pusher infrastructure tomorrow and will report back!

[brianv0]

Okay great, I'll get those committed, I will be trying this out today hopefully as well. I think I had some code which may have been lost in translation and one of these might have been hard for me to see due to how long our tokens live in testing, but it sounds like we're close

[JoelSpeed]

@brianv0 re your comment about the redundant code, I fixed what I had intended to do in 66bbf14, does that look better? 🙈

[JoelSpeed]

Found an issue in the session clearing which I've fixed in this commit 6d7f0ab

[JoelSpeed]

One final thing 🙈 I noticed when attempting to deploy to a production system that there was no migration between different backends for the session storage and as such people had to clear cookies to make the proxy work. I added a test and a fix that should make this work in this commit 131206c

But with that, we are now running a build from my branch redis-sessions-store on our production systems at Pusher and it seems to be working 🎉

[JoelSpeed]

@brianv0 I'm happy with the implementation and testing that has been done on our side now.

If you could cherry pick the last few commits from our branch, then I think we are good to merge 🎉

[brianv0]

Okay I think we're good now

[JoelSpeed]

LGTM! 🎉 Thanks for all your work and patience with me on this one @brianv0!