Redis sessions #224

sporkmonger · 2019-07-12T00:39:59Z

Problem

Session state can become quite large and can easily exceed the maximum cookie size. I previously tried to fix it with #150 but that proved to be a bad solution prone to failures.

Solution

This introduces optional support for a Redis backend for sessions. Setting a SESSION_REDIS_CONNECTION environment variable to e.g. redis://primary.sso-sessions.example-cloud.com:6379 will switch the session backend from cookie sessions to redis sessions. The default remains cookie sessions.

Notes

This also avoids the issue of large amounts of session state being retransmitted on every request. Session state is encrypted using the existing cipher mechanism and session keys. As discussed in #158, I recommend renaming the cipher key configuration name to be more generic, but this PR doesn't make that change. This PR does not use the ticket system mentioned in #158, and instead opts for the simpler approach of encrypting using the session secret held on the auth server. I have tested this PR in conjunction with #118 and it works well.

codecov · 2019-07-12T00:42:15Z

Codecov Report

Merging #224 into master will decrease coverage by 2.61%.
The diff coverage is 20%.

@@            Coverage Diff             @@
##           master     #224      +/-   ##
==========================================
- Coverage   62.23%   59.62%   -2.62%     
==========================================
  Files          50       51       +1     
  Lines        4067     4225     +158     
==========================================
- Hits         2531     2519      -12     
- Misses       1349     1510     +161     
- Partials      187      196       +9

Impacted Files	Coverage Δ
internal/proxy/options.go	`83.59% <ø> (ø)`	⬆️
internal/proxy/proxy.go	`22.72% <0%> (-1.67%)`	⬇️
internal/auth/options.go	`52.89% <0%> (-25.68%)`	⬇️
internal/proxy/oauthproxy.go	`41.27% <0%> (-9.71%)`	⬇️
internal/pkg/sessions/redis_store.go	`29.72% <29.72%> (ø)`
internal/auth/configuration.go	`49.26% <43.75%> (-0.47%)`	⬇️
internal/auth/mux.go	`71.18% <50%> (-3.82%)`	⬇️
internal/pkg/sessions/session_state.go	`48.57% <0%> (-36.43%)`	⬇️
... and 8 more

jphines · 2019-07-12T13:11:09Z

@sporkmonger this is much cleaner than I expected, which is fantastic!

I'll take a look at this today/this weekend, provide some review, and kick the tires.

Thank you for your work on this effort, it is a fantastic set of functionality. I (we) greatly appreciate your contributions!

sporkmonger · 2019-07-15T08:41:36Z

Sure thing. I had some trouble figuring out how to mock the Redis client, which is why there's a bit of a coverage drop I believe.

sporkmonger · 2019-07-15T08:43:14Z

internal/auth/configuration.go

@@ -35,6 +38,9 @@ import (
 // PROVIDER_*_CLIENT_SECRET
 // PROVIDER_*_SCOPE
 //
+// PROVIDER_*_AZURE_TENANT
+// PROVIDER_*_AZURE_PROMPT


This snuck in somehow, not sure how critical it is to keep this isolated since this PR is the main blocker for Azure, but I can remove if needed.

Definitely not critical to remove, but I think I'd personally prefer if it was pulled back into the Azure PR and removed here, if that's ok!

sporkmonger · 2019-08-26T19:03:36Z

@jphines Any updates?

sylr · 2019-09-09T07:36:11Z

@sporkmonger it seems there currently is a conflict, could you resolve it ?

@jphines Do you have time to review this ?

jphines · 2019-09-09T14:20:36Z

My apologizes, I've failed to prioritize review here appropriately. I've asked @Jusshersmith to review these changes in light of my inability to find the time to do so.

sporkmonger · 2019-09-11T20:12:31Z

Yeah, I'll try to address the conflict shortly.

Jusshersmith · 2019-09-12T10:52:23Z

internal/auth/configuration.go

@@ -84,6 +90,9 @@ func DefaultAuthConfig() Configuration {
 				Secure:   true,
 				HTTPOnly: true,
 			},
+			RedisConfig: RedisConfig{
+				UseSentinel: false,


To clarify: is this just included for explicitness around this setting? (Unless I'm mistaken, it's not strictly required, right?)

Yes, that's correct. It's been awhile but I think it might fail if the RedisConfig struct doesn't get initialized, but the UseSentinel default is the false zero value.

Jusshersmith · 2019-09-12T12:26:21Z

internal/auth/mux.go

+		} else {
+			opts = append(opts, SetCookieStore(config.SessionConfig, idpSlug))
+		}
+		authenticator, err := NewAuthenticator(config, opts...)


What do you think about maybe creating a new SetSessionStore function that houses this logic, which would be passed in with the slice of functions to NewAuthenticator?

Which either essentially calls the above, or returns one of the SetRedisStore or SetCookieStore funcs?

However, this kind of pattern is used elsewhere in the codebase! 🤔

Jusshersmith · 2019-09-13T09:30:25Z

internal/auth/configuration.go

@@ -35,6 +38,9 @@ import (
 // PROVIDER_*_CLIENT_SECRET
 // PROVIDER_*_SCOPE
 //
+// PROVIDER_*_AZURE_TENANT
+// PROVIDER_*_AZURE_PROMPT


Definitely not critical to remove, but I think I'd personally prefer if it was pulled back into the Azure PR and removed here, if that's ok!

Jusshersmith · 2019-09-13T11:02:11Z

internal/proxy/proxy.go

+		setSessionStore := SetCookieStore
+		if opts.RedisConnectionURL != "" || opts.RedisUseSentinel {
+			setSessionStore = SetRedisStore
+		}


This is nice and concise, but I wonder if it's worth standardising to match the other similar use cases above, e.g:

if opts.RedisConnectionURL != "" || opts.RedisUseSentinel { optFuncs = append(optFuncs, SetRedisStore(opts) } else { optFuncs = append(optFuncs, SetCookieStore(opts) }

Or, at least closer to whatever method we end up using on the authenticator side to set the session store.

Jusshersmith · 2019-09-13T11:11:51Z

internal/auth/configuration_test.go

+			CheckFunc: func(c Configuration, t *testing.T) {
+				redisConf := c.SessionConfig.RedisConfig
+				assertEq([]string{"redis://master.example.com:6379"}, redisConf.ConnectionURLs, t)
+			},


I wonder if it's also worth adding in a call to RedisConfig.Validate() and/or RedisConfig.Enabled() here? It seems like it could be included fairly easily, and I think would help get the error cases within them tested.

Jusshersmith · 2019-09-13T11:40:58Z

This looks great @sporkmonger, and thank you for your patience! I've left some comments to start with!

sylr · 2019-10-03T12:44:59Z

Any updates ?

sylr · 2019-10-22T15:44:56Z

Guys ?

sporkmonger · 2019-10-24T02:00:04Z

It's taken long enough that... we ended up moving from Azure AD to Okta in the meantime.

sylr · 2019-10-24T09:34:24Z

😭

Jusshersmith · 2019-10-28T19:55:23Z

Thank you for all of your great work on this @sporkmonger! This seems like it could still be an extremely valuable addition, and would be a shame for all of this work to go to waste. I don't think we at BuzzFeed necessarily have the capacity to push it over the finish line in the immediate future, but I think we can keep this open for a while still to allow someone to pick it up if possible -- providing you're happy with that @sporkmonger.

sporkmonger · 2019-10-28T19:56:55Z

Totally. My bandwidth for stuff like this goes up and down a lot, but the next time I've got some spare cycles I'll try to get this addressed. If someone else wants to pick it up and get it over the finish line, I'm equally OK with that too.

Bob Aman added 5 commits June 20, 2019 15:28

Rough implementation of Redis config options

63b6feb

Construct the redis client from supplied options

d5ffeb4

Redis store wired up to OAuth proxy.

d6c6e64

Configure Redis

b5c3167

go mod tidy

f8a9efd

sporkmonger commented Jul 15, 2019

View reviewed changes

sylr mentioned this pull request Jul 23, 2019

sso-auth: Azure AD provider #118

Open

jphines requested a review from Jusshersmith September 9, 2019 14:19

Jusshersmith reviewed Sep 13, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redis sessions #224

Redis sessions #224

sporkmonger commented Jul 12, 2019

codecov bot commented Jul 12, 2019

jphines commented Jul 12, 2019

sporkmonger commented Jul 15, 2019

sporkmonger Jul 15, 2019

Jusshersmith Sep 13, 2019

sporkmonger commented Aug 26, 2019

sylr commented Sep 9, 2019

jphines commented Sep 9, 2019

sporkmonger commented Sep 11, 2019

Jusshersmith Sep 12, 2019

sporkmonger Sep 13, 2019

Jusshersmith Sep 12, 2019

Jusshersmith Sep 12, 2019

Jusshersmith Sep 13, 2019

Jusshersmith Sep 13, 2019

Jusshersmith Sep 13, 2019 •

edited

Jusshersmith commented Sep 13, 2019

sylr commented Oct 3, 2019

sylr commented Oct 22, 2019

sporkmonger commented Oct 24, 2019

sylr commented Oct 24, 2019

Jusshersmith commented Oct 28, 2019

sporkmonger commented Oct 28, 2019

Redis sessions #224

Are you sure you want to change the base?

Redis sessions #224

Conversation

sporkmonger commented Jul 12, 2019

Problem

Solution

Notes

codecov bot commented Jul 12, 2019

Codecov Report

jphines commented Jul 12, 2019

sporkmonger commented Jul 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sporkmonger commented Aug 26, 2019

sylr commented Sep 9, 2019

jphines commented Sep 9, 2019

sporkmonger commented Sep 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jusshersmith Sep 13, 2019 • edited

Choose a reason for hiding this comment

Jusshersmith commented Sep 13, 2019

sylr commented Oct 3, 2019

sylr commented Oct 22, 2019

sporkmonger commented Oct 24, 2019

sylr commented Oct 24, 2019

Jusshersmith commented Oct 28, 2019

sporkmonger commented Oct 28, 2019

Jusshersmith Sep 13, 2019 •

edited