session: support redis session store

[edwardzjl]

Signed-off-by: Junlin Zhou jameszhou2108@hotmail.com

This PR allows user to config redis as a session store.

[pdylanross]

@johnbuluba / @arrikto What can be done to get some movement on this change set? I had a minor kubeflow outage last week caused by this service not supporting multiple replicas & this change should fix that issue.

[timjaya]

@arrikto any updates here?

[athamark]

Hello, @edwardzjl thanks for your contribution. We reviewed and merged your PR. Here I will expose a series of minor yet important changes that we made before merging this effort:

1. We re-wrote the commit message to: i) better describe what this commit accomplishes, ii) to reference this PR, iii) to include the reviewer's tag.

2. We added a validSessionStoreType() function in the settings.go to check the validity of the SESSION_STORE_TYPE envvar. Similarly to the ACCESS_TOKEN_AUTHN envvar, we need to ensure that the users have configured AuthService with one of the accepted values early on. When AuthService parses the configurations it should fail to execute if one of the envvar configurations is invalid.

3. According to this go-redis guide (https://redis.uptrace.dev/guide/go-redis.html#connecting-to-redis-server), when AuthService initiates a new redis client we should give the option to the users to also setup the client with a password and a number for the database that we are going to use. This will allow AuthService to also work in more complex deployments where redis requires a password and has a variety of available databases to work with:

   rdb := redis.NewClient(&redis.Options{
      Addr:      "localhost:6379",
      Password: "", // no password set
      DB:        0,  // use default DB
   })

   With that said, we also added two new envvars:

   • SESSION_STORE_REDIS_PWD: for the redis password
   • SESSION_STORE_REDIS_DB: for the number of the database

4. In general, when we introduce a new envvar it is good to showcase how it works. For all three new envvars (SESSION_STORE_TYPE, SESSION_STORE_REDIS_PWD, SESSION_STORE_REDIS_DB) I added a short description in README.md.

5. Right now we have two options for the session stores, in the long run we may add another one. To keep the code that sets up the AuthService clean and readable (main.go), we factored out the initialization of the stores, we introduced the initiateSessionStores() function.

6. Last but not least, since the PR dates back to summer it did not include an upgrade we did for the dependencies (GitHub-PR: #101), so I dropped your go.mod and go.sum changes and run go mod tidy to include all the necessary packages.

Thanks for all the effort and for your contribution!

[edwardzjl]

@athamark Thank you for your review! I apologies for lacking documents and tests, and the code quality issues. I was planning to polish the commit but was a bit busy recently. Learning from your review is a pleasure, thank you!

[musabshak]

The TTL on the redis keys (sessions) is ridiculously high (950 million seconds) - I am trying to figure out how this is getting set. Feels like some bug with https://github.com/rbcervilla/redisstore potentially?

[musabshak]

The TTL on the redis keys (sessions) is ridiculously high (950 million seconds) - I am trying to figure out how this is getting set. Feels like some bug with https://github.com/rbcervilla/redisstore potentially?

The bug is here: https://github.com/arrikto/oidc-authservice/blob/master/sessions/state.go#L52

//=== BUGGY CODE
maxAge := int(20 * time.Minute)

// 1,200,000,000,000. This is an integer representing nanoseconds.
fmt.Println(maxAge)

redisExpirationDuration := time.Duration(maxAge) * time.Second

// 267120h 53m 28.87914496s
// This gets converted to 961,635,208,879 ms by redis client code,
// which is approximately 961 million seconds or 30 years
fmt.Println(redisExpirationDuration)

//=== FIXED CODE
maxAge = int((20 * time.Minute).Seconds())

// 1200 (matches expectation)
fmt.Println(maxAge)

redisExpirationDuration = time.Duration(maxAge) * time.Second

// 20m 0s (matches expectation)
fmt.Println(redisExpirationDuration)

I should probably open a PR with the fix