Merge branch 'master' into bumpoidc

CHANGELOG.md

@@ -16,6 +16,16 @@
 
 - [#175](https://github.com/pusher/outh2_proxy/pull/175) Bump go-oidc to v2.0.0 (@aeijdenberg).
   - Includes fix for potential signature checking issue when OIDC discovery is skipped.
+- [#155](https://github.com/pusher/outh2_proxy/pull/155) Add RedisSessionStore implementation (@brianv0, @JoelSpeed)
+  - Implement flags to configure the redis session store
+    - `-session-store-type=redis` Sets the store type to redis
+    - `-redis-connection-url` Sets the Redis connection URL
+    - `-redis-use-sentinel=true` Enables Redis Sentinel support
+    - `-redis-sentinel-master-name` Sets the Sentinel master name, if sentinel is enabled
+    - `-redis-sentinel-connection-urls` Defines the Redis Sentinel Connection URLs, if sentinel is enabled
+  - Introduces the concept of a session ticket. Tickets are composed of the cookie name, a session ID, and a secret.
+  - Redis Sessions are stored encrypted with a per-session secret 
+  - Added tests for server based session stores
 - [#168](https://github.com/pusher/outh2_proxy/pull/168) Drop Go 1.11 support in Travis (@JoelSpeed)
 - [#169](https://github.com/pusher/outh2_proxy/pull/169) Update Alpine to 3.9 (@kskewes)
 - [#148](https://github.com/pusher/outh2_proxy/pull/148) Implement SessionStore interface within proxy (@JoelSpeed)

Gopkg.lock

@@ -17,6 +17,25 @@
   revision = "b26d9c308763d68093482582cea63d69be07a0f0"
   version = "v0.3.0"
 
+[[projects]]
+  branch = "master"
+  digest = "1:3cce78d5d0090e3f1162945fba60ba74e72e8422e8e41bb9c701afb67237bb65"
+  name = "github.com/alicebob/gopher-json"
+  packages = ["."]
+  pruneopts = ""
+  revision = "5a6b3ba71ee69b77cf64febf8b5a7526ca5eaef0"
+
+[[projects]]
+  digest = "1:18a07506ddaa87b1612bfd69eef03f510faf122398df3da774d46dcfe751a060"
+  name = "github.com/alicebob/miniredis"
+  packages = [
+    ".",
+    "server",
+  ]
+  pruneopts = ""
+  revision = "3d7aa1333af56ab862d446678d93aaa6803e0938"
+  version = "v2.7.0"
+
 [[projects]]
   digest = "1:512883404c2a99156e410e9880e3bb35ecccc0c07c1159eb204b5f3ef3c431b3"
   name = "github.com/bitly/go-simplejson"
@@ -49,6 +68,22 @@
   revision = "06ea1031745cb8b3dab3f6a236daf2b0aa468b7e"
   version = "v3.2.0"
 
+[[projects]]
+  digest = "1:8c7410dae63c74bd92db09bf33af7e0698b635ab6a397fd8e9e10dfcce3138ac"
+  name = "github.com/go-redis/redis"
+  packages = [
+    ".",
+    "internal",
+    "internal/consistenthash",
+    "internal/hashtag",
+    "internal/pool",
+    "internal/proto",
+    "internal/util",
+  ]
+  pruneopts = ""
+  revision = "d22fde8721cc915a55aeb6b00944a76a92bfeb6e"
+  version = "v6.15.2"
+
 [[projects]]
   branch = "master"
   digest = "1:3b760d3b93f994df8eb1d9ebfad17d3e9e37edcb7f7efaa15b427c0d7a64f4e4"
@@ -57,6 +92,17 @@
   pruneopts = ""
   revision = "1e59b77b52bf8e4b449a57e6f79f21226d571845"
 
+[[projects]]
+  digest = "1:dcf8316121302735c0ac84e05f4686e3b34e284444435e9a206da48d8be18cb1"
+  name = "github.com/gomodule/redigo"
+  packages = [
+    "internal",
+    "redis",
+  ]
+  pruneopts = ""
+  revision = "9c11da706d9b7902c6da69c592f75637793fe121"
+  version = "v2.0.0"
+
 [[projects]]
   digest = "1:b3c5b95e56c06f5aa72cb2500e6ee5f44fcd122872d4fec2023a488e561218bc"
   name = "github.com/hpcloud/tail"
@@ -173,6 +219,19 @@
   pruneopts = ""
   revision = "1d66fa95c997864ba4d8479f56609620fe542928"
 
+[[projects]]
+  branch = "master"
+  digest = "1:378d29a839ff770e9d9150580b4c01ff0a513a296b0487558a7af7c18adab98e"
+  name = "github.com/yuin/gopher-lua"
+  packages = [
+    ".",
+    "ast",
+    "parse",
+    "pm",
+  ]
+  pruneopts = ""
+  revision = "8bfc7677f583b35a5663a9dd934c08f3b5774bbb"
+
 [[projects]]
   branch = "master"
   digest = "1:f6a006d27619a4d93bf9b66fe1999b8c8d1fa62bdc63af14f10fbe6fcaa2aa1a"
@@ -341,9 +400,11 @@
   analyzer-version = 1
   input-imports = [
     "github.com/BurntSushi/toml",
+    "github.com/alicebob/miniredis",
     "github.com/bitly/go-simplejson",
     "github.com/coreos/go-oidc",
     "github.com/dgrijalva/jwt-go",
+    "github.com/go-redis/redis",
     "github.com/mbland/hmacauth",
     "github.com/mreiferson/go-options",
     "github.com/onsi/ginkgo",

Gopkg.toml

@@ -46,3 +46,11 @@
 [[constraint]]
   name = "gopkg.in/natefinch/lumberjack.v2"
   version = "2.1.0"
+
+[[constraint]]
+  name = "github.com/go-redis/redis"
+  version = "v6.15.2"
+
+[[constraint]]
+  name = "github.com/alicebob/miniredis"
+  version = "2.7.0"

docs/configuration/configuration.md

@@ -75,6 +75,10 @@ Usage of oauth2_proxy:
   -pubjwk-url string: JWK pubkey access endpoint: required by login.gov
   -redeem-url string: Token redemption endpoint
   -redirect-url string: the OAuth Redirect URL. ie: "https://internalapp.yourcompany.com/oauth2/callback"
+  -redis-connection-url string: URL of redis server for redis session storage (eg: redis://HOST[:PORT])
+  -redis-sentinel-master-name string: Redis sentinel master name. Used in conjuction with --redis-use-sentinel
+  -redis-sentinel-connection-urls: List of Redis sentinel conneciton URLs (eg redis://HOST[:PORT]). Used in conjuction with --redis-use-sentinel
+  -redis-use-sentinel: Connect to redis via sentinels. Must set --redis-sentinel-master-name and --redis-sentinel-connection-urls to use this feature (default: false)
   -request-logging: Log requests to stdout (default true)
   -request-logging-format: Template for request log lines (see "Logging Configuration" paragraph below)
   -resource string: The resource that is protected (Azure AD only)

docs/configuration/sessions.md

@@ -16,6 +16,7 @@ data in one of the available session storage backends.
 
 At present the available backends are (as passed to `--session-store-type`):
 - [cookie](cookie-storage) (default)
+- [redis](redis-storage)
 
 ### Cookie Storage
 
@@ -32,3 +33,35 @@ The following should be known when using this implementation:
 - Since multiple requests can be made concurrently to the OAuth2 Proxy, this session implementation
 cannot lock sessions and while updating and refreshing sessions, there can be conflicts which force
 users to re-authenticate
+
+
+### Redis Storage
+
+The Redis Storage backend stores sessions, encrypted, in redis. Instead sending all the information
+back the the client for storage, as in the [Cookie storage](cookie-storage), a ticket is sent back
+to the user as the cookie value instead.
+
+A ticket is composed as the following:
+
+`{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, base64url encoded (no padding). The secret is unique for every session.
+- The pair of `{CookieName}-{ticketID}` comprises a ticket handle, and thus, the redis key
+to which the session is stored. The encoded session is encrypted with the secret and stored
+in redis via the `SETEX` command.
+
+Encrypting every session uniquely protects the refresh/access/id tokens stored in the session from
+disclosure.
+
+#### Usage
+
+When using the redis store, specify `--session-store-type=redis` as well as the Redis connection URL, via
+`--redis-connection-url=redis://host[:port][/db-number]`.
+
+You may also configure the store for Redis Sentinel. In this case, you will want to use the 
+`--redis-use-sentinel=true` flag, as well as configure the flags `--redis-sentinel-master-name` 
+and `--redis-sentinel-connection-urls` appropriately.

main.go

@@ -24,6 +24,7 @@ func main() {
 	upstreams := StringArray{}
 	skipAuthRegex := StringArray{}
 	googleGroups := StringArray{}
+	redisSentinelConnectionURLs := StringArray{}
 
 	config := flagSet.String("config", "", "path to config file")
 	showVersion := flagSet.Bool("version", false, "print version string")
@@ -76,6 +77,10 @@ func main() {
 	flagSet.Bool("cookie-httponly", true, "set HttpOnly cookie flag")
 
 	flagSet.String("session-store-type", "cookie", "the session storage provider to use")
+	flagSet.String("redis-connection-url", "", "URL of redis server for redis session storage (eg: redis://HOST[:PORT])")
+	flagSet.Bool("redis-use-sentinel", false, "Connect to redis via sentinels. Must set --redis-sentinel-master-name and --redis-sentinel-connection-urls to use this feature")
+	flagSet.String("redis-sentinel-master-name", "", "Redis sentinel master name. Used in conjuction with --redis-use-sentinel")
+	flagSet.Var(&redisSentinelConnectionURLs, "redis-sentinel-connection-urls", "List of Redis sentinel connection URLs (eg redis://HOST[:PORT]). Used in conjuction with --redis-use-sentinel")
 
 	flagSet.String("logging-filename", "", "File to log requests to, empty for stdout")
 	flagSet.Int("logging-max-size", 100, "Maximum size in megabytes of the log file before rotation")

pkg/apis/options/sessions.go

@@ -9,6 +9,7 @@ type SessionOptions struct {
 	Type   string `flag:"session-store-type" cfg:"session_store_type" env:"OAUTH2_PROXY_SESSION_STORE_TYPE"`
 	Cipher *cookie.Cipher
 	CookieStoreOptions
+	RedisStoreOptions
 }
 
 // CookieSessionStoreType is used to indicate the CookieSessionStore should be
@@ -17,3 +18,15 @@ var CookieSessionStoreType = "cookie"
 
 // CookieStoreOptions contains configuration options for the CookieSessionStore.
 type CookieStoreOptions struct{}
+
+// RedisSessionStoreType is used to indicate the RedisSessionStore should be
+// used for storing sessions.
+var RedisSessionStoreType = "redis"
+
+// RedisStoreOptions contains configuration options for the RedisSessionStore.
+type RedisStoreOptions struct {
+	RedisConnectionURL     string   `flag:"redis-connection-url" cfg:"redis_connection_url" env:"OAUTH2_PROXY_REDIS_CONNECTION_URL"`
+	UseSentinel            bool     `flag:"redis-use-sentinel" cfg:"redis_use_sentinel" env:"OAUTH2_PROXY_REDIS_USE_SENTINEL"`
+	SentinelMasterName     string   `flag:"redis-sentinel-master-name" cfg:"redis_sentinel_master_name" env:"OAUTH2_PROXY_REDIS_SENTINEL_MASTER_NAME"`
+	SentinelConnectionURLs []string `flag:"redis-sentinel-connection-urls" cfg:"redis_sentinel_connection_urls" env:"OAUTH2_PROXY_REDIS_SENTINEL_CONNECTION_URLS"`
+}

pkg/apis/sessions/session_state.go

@@ -203,14 +203,14 @@ func DecodeSessionState(v string, c *cookie.Cipher) (*SessionState, error) {
 			User:  ss.User,
 		}
 	} else {
-		// Backward compatibility with using unecrypted Email
+		// Backward compatibility with using unencrypted Email
 		if ss.Email != "" {
 			decryptedEmail, errEmail := c.Decrypt(ss.Email)
 			if errEmail == nil {
 				ss.Email = decryptedEmail
 			}
 		}
-		// Backward compatibility with using unecrypted User
+		// Backward compatibility with using unencrypted User
 		if ss.User != "" {
 			decryptedUser, errUser := c.Decrypt(ss.User)
 			if errUser == nil {

pkg/sessions/redis/redis_store.go

@@ -0,0 +1,305 @@
+package redis
+
+import (
+	"crypto/aes"
+	"crypto/cipher"
+	"crypto/rand"
+	"encoding/base64"
+	"encoding/hex"
+	"fmt"
+	"io"
+	"net/http"
+	"strings"
+	"time"
+
+	"github.com/go-redis/redis"
+	"github.com/pusher/oauth2_proxy/cookie"
+	"github.com/pusher/oauth2_proxy/pkg/apis/options"
+	"github.com/pusher/oauth2_proxy/pkg/apis/sessions"
+	"github.com/pusher/oauth2_proxy/pkg/cookies"
+)
+
+// TicketData is a structure representing the ticket used in server session storage
+type TicketData struct {
+	TicketID string
+	Secret   []byte
+}
+
+// SessionStore is an implementation of the sessions.SessionStore
+// interface that stores sessions in redis
+type SessionStore struct {
+	CookieCipher  *cookie.Cipher
+	CookieOptions *options.CookieOptions
+	Client        *redis.Client
+}
+
+// NewRedisSessionStore initialises a new instance of the SessionStore from
+// the configuration given
+func NewRedisSessionStore(opts *options.SessionOptions, cookieOpts *options.CookieOptions) (sessions.SessionStore, error) {
+	client, err := newRedisClient(opts.RedisStoreOptions)
+	if err != nil {
+		return nil, fmt.Errorf("error constructing redis client: %v", err)
+	}
+
+	rs := &SessionStore{
+		Client:        client,
+		CookieCipher:  opts.Cipher,
+		CookieOptions: cookieOpts,
+	}
+	return rs, nil
+
+}
+
+func newRedisClient(opts options.RedisStoreOptions) (*redis.Client, error) {
+	if opts.UseSentinel {
+		client := redis.NewFailoverClient(&redis.FailoverOptions{
+			MasterName:    opts.SentinelMasterName,
+			SentinelAddrs: opts.SentinelConnectionURLs,
+		})
+		return client, nil
+	}
+
+	opt, err := redis.ParseURL(opts.RedisConnectionURL)
+	if err != nil {
+		return nil, fmt.Errorf("unable to parse redis url: %s", err)
+	}
+
+	client := redis.NewClient(opt)
+	return client, nil
+}
+
+// Save takes a sessions.SessionState and stores the information from it
+// to redies, and adds a new ticket cookie on the HTTP response writer
+func (store *SessionStore) Save(rw http.ResponseWriter, req *http.Request, s *sessions.SessionState) error {
+	if s.CreatedAt.IsZero() {
+		s.CreatedAt = time.Now()
+	}
+
+	// Old sessions that we are refreshing would have a request cookie
+	// New sessions don't, so we ignore the error. storeValue will check requestCookie
+	requestCookie, _ := req.Cookie(store.CookieOptions.CookieName)
+	value, err := s.EncodeSessionState(store.CookieCipher)
+	if err != nil {
+		return err
+	}
+	ticketString, err := store.storeValue(value, store.CookieOptions.CookieExpire, requestCookie)
+	if err != nil {
+		return err
+	}
+
+	ticketCookie := store.makeCookie(
+		req,
+		ticketString,
+		store.CookieOptions.CookieExpire,
+		s.CreatedAt,
+	)
+
+	http.SetCookie(rw, ticketCookie)
+	return nil
+}
+
+// Load reads sessions.SessionState information from a ticket
+// cookie within the HTTP request object
+func (store *SessionStore) Load(req *http.Request) (*sessions.SessionState, error) {
+	requestCookie, err := req.Cookie(store.CookieOptions.CookieName)
+	if err != nil {
+		return nil, fmt.Errorf("error loading session: %s", err)
+	}
+
+	val, _, ok := cookie.Validate(requestCookie, store.CookieOptions.CookieSecret, store.CookieOptions.CookieExpire)
+	if !ok {
+		return nil, fmt.Errorf("Cookie Signature not valid")
+	}
+	session, err := store.loadSessionFromString(val)
+	if err != nil {
+		return nil, fmt.Errorf("error loading session: %s", err)
+	}
+	return session, nil
+}
+
+// loadSessionFromString loads the session based on the ticket value
+func (store *SessionStore) loadSessionFromString(value string) (*sessions.SessionState, error) {
+	ticket, err := decodeTicket(store.CookieOptions.CookieName, value)
+	if err != nil {
+		return nil, err
+	}
+
+	result, err := store.Client.Get(ticket.asHandle(store.CookieOptions.CookieName)).Result()
+	if err != nil {
+		return nil, err
+	}
+
+	resultBytes := []byte(result)
+	block, err := aes.NewCipher(ticket.Secret)
+	if err != nil {
+		return nil, err
+	}
+	// Use secret as the IV too, because each entry has it's own key
+	stream := cipher.NewCFBDecrypter(block, ticket.Secret)
+	stream.XORKeyStream(resultBytes, resultBytes)
+
+	session, err := sessions.DecodeSessionState(string(resultBytes), store.CookieCipher)
+	if err != nil {
+		return nil, err
+	}
+	return session, nil
+}
+
+// Clear clears any saved session information for a given ticket cookie
+// from redis, and then clears the session
+func (store *SessionStore) Clear(rw http.ResponseWriter, req *http.Request) error {
+	// We go ahead and clear the cookie first, always.
+	clearCookie := store.makeCookie(
+		req,
+		"",
+		time.Hour*-1,
+		time.Now(),
+	)
+	http.SetCookie(rw, clearCookie)
+
+	// If there was an existing cookie we should clear the session in redis
+	requestCookie, err := req.Cookie(store.CookieOptions.CookieName)
+	if err != nil && err == http.ErrNoCookie {
+		// No existing cookie so can't clear redis
+		return nil
+	} else if err != nil {
+		return fmt.Errorf("error retrieving cookie: %v", err)
+	}
+
+	val, _, ok := cookie.Validate(requestCookie, store.CookieOptions.CookieSecret, store.CookieOptions.CookieExpire)
+	if !ok {
+		return fmt.Errorf("Cookie Signature not valid")
+	}
+
+	// We only return an error if we had an issue with redis
+	// If there's an issue decoding the ticket, ignore it
+	ticket, _ := decodeTicket(store.CookieOptions.CookieName, val)
+	if ticket != nil {
+		_, err := store.Client.Del(ticket.asHandle(store.CookieOptions.CookieName)).Result()
+		if err != nil {
+			return fmt.Errorf("error clearing cookie from redis: %s", err)
+		}
+	}
+	return nil
+}
+
+// makeCookie makes a cookie, signing the value if present
+func (store *SessionStore) makeCookie(req *http.Request, value string, expires time.Duration, now time.Time) *http.Cookie {
+	if value != "" {
+		value = cookie.SignedValue(store.CookieOptions.CookieSecret, store.CookieOptions.CookieName, value, now)
+	}
+	return cookies.MakeCookieFromOptions(
+		req,
+		store.CookieOptions.CookieName,
+		value,
+		store.CookieOptions,
+		expires,
+		now,
+	)
+}
+
+func (store *SessionStore) storeValue(value string, expiration time.Duration, requestCookie *http.Cookie) (string, error) {
+	ticket, err := store.getTicket(requestCookie)
+	if err != nil {
+		return "", fmt.Errorf("error getting ticket: %v", err)
+	}
+
+	ciphertext := make([]byte, len(value))
+	block, err := aes.NewCipher(ticket.Secret)
+	if err != nil {
+		return "", fmt.Errorf("error initiating cipher block %s", err)
+	}
+
+	// Use secret as the Initialization Vector too, because each entry has it's own key
+	stream := cipher.NewCFBEncrypter(block, ticket.Secret)
+	stream.XORKeyStream(ciphertext, []byte(value))
+
+	handle := ticket.asHandle(store.CookieOptions.CookieName)
+	err = store.Client.Set(handle, ciphertext, expiration).Err()
+	if err != nil {
+		return "", err
+	}
+	return ticket.encodeTicket(store.CookieOptions.CookieName), nil
+}
+
+// getTicket retrieves an existing ticket from the cookie if present,
+// or creates a new ticket
+func (store *SessionStore) getTicket(requestCookie *http.Cookie) (*TicketData, error) {
+	if requestCookie == nil {
+		return newTicket()
+	}
+
+	// An existing cookie exists, try to retrieve the ticket
+	val, _, ok := cookie.Validate(requestCookie, store.CookieOptions.CookieSecret, store.CookieOptions.CookieExpire)
+	if !ok {
+		// Cookie is invalid, create a new ticket
+		return newTicket()
+	}
+
+	// Valid cookie, decode the ticket
+	ticket, err := decodeTicket(store.CookieOptions.CookieName, val)
+	if err != nil {
+		// If we can't decode the ticket we have to create a new one
+		return newTicket()
+	}
+	return ticket, nil
+}
+
+func newTicket() (*TicketData, error) {
+	rawID := make([]byte, 16)
+	if _, err := io.ReadFull(rand.Reader, rawID); err != nil {
+		return nil, fmt.Errorf("failed to create new ticket ID %s", err)
+	}
+	// ticketID is hex encoded
+	ticketID := fmt.Sprintf("%x", rawID)
+
+	secret := make([]byte, aes.BlockSize)
+	if _, err := io.ReadFull(rand.Reader, secret); err != nil {
+		return nil, fmt.Errorf("failed to create initialization vector %s", err)
+	}
+	ticket := &TicketData{
+		TicketID: ticketID,
+		Secret:   secret,
+	}
+	return ticket, nil
+}
+
+func (ticket *TicketData) asHandle(prefix string) string {
+	return fmt.Sprintf("%s-%s", prefix, ticket.TicketID)
+}
+
+func decodeTicket(cookieName string, ticketString string) (*TicketData, error) {
+	prefix := cookieName + "-"
+	if !strings.HasPrefix(ticketString, prefix) {
+		return nil, fmt.Errorf("failed to decode ticket handle")
+	}
+	trimmedTicket := strings.TrimPrefix(ticketString, prefix)
+
+	ticketParts := strings.Split(trimmedTicket, ".")
+	if len(ticketParts) != 2 {
+		return nil, fmt.Errorf("failed to decode ticket")
+	}
+	ticketID, secretBase64 := ticketParts[0], ticketParts[1]
+
+	// ticketID must be a hexadecimal string
+	_, err := hex.DecodeString(ticketID)
+	if err != nil {
+		return nil, fmt.Errorf("server ticket failed sanity checks")
+	}
+
+	secret, err := base64.RawURLEncoding.DecodeString(secretBase64)
+	if err != nil {
+		return nil, fmt.Errorf("failed to decode initialization vector %s", err)
+	}
+	ticketData := &TicketData{
+		TicketID: ticketID,
+		Secret:   secret,
+	}
+	return ticketData, nil
+}
+
+func (ticket *TicketData) encodeTicket(prefix string) string {
+	handle := ticket.asHandle(prefix)
+	ticketString := handle + "." + base64.RawURLEncoding.EncodeToString(ticket.Secret)
+	return ticketString
+}

pkg/sessions/session_store.go

@@ -6,6 +6,7 @@ import (
 	"github.com/pusher/oauth2_proxy/pkg/apis/options"
 	"github.com/pusher/oauth2_proxy/pkg/apis/sessions"
 	"github.com/pusher/oauth2_proxy/pkg/sessions/cookie"
+	"github.com/pusher/oauth2_proxy/pkg/sessions/redis"
 )
 
 // NewSessionStore creates a SessionStore from the provided configuration
@@ -13,6 +14,8 @@ func NewSessionStore(opts *options.SessionOptions, cookieOpts *options.CookieOpt
 	switch opts.Type {
 	case options.CookieSessionStoreType:
 		return cookie.NewCookieSessionStore(opts, cookieOpts)
+	case options.RedisSessionStoreType:
+		return redis.NewRedisSessionStore(opts, cookieOpts)
 	default:
 		return nil, fmt.Errorf("unknown session store type '%s'", opts.Type)
 	}

pkg/sessions/session_store_test.go

@@ -10,6 +10,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/alicebob/miniredis"
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 	"github.com/pusher/oauth2_proxy/cookie"
@@ -18,6 +19,7 @@ import (
 	"github.com/pusher/oauth2_proxy/pkg/cookies"
 	"github.com/pusher/oauth2_proxy/pkg/sessions"
 	sessionscookie "github.com/pusher/oauth2_proxy/pkg/sessions/cookie"
+	"github.com/pusher/oauth2_proxy/pkg/sessions/redis"
 	"github.com/pusher/oauth2_proxy/pkg/sessions/utils"
 )
 
@@ -34,6 +36,7 @@ var _ = Describe("NewSessionStore", func() {
 	var response *httptest.ResponseRecorder
 	var session *sessionsapi.SessionState
 	var ss sessionsapi.SessionStore
+	var mr *miniredis.Miniredis
 
 	CheckCookieOptions := func() {
 		Context("the cookies returned", func() {
@@ -89,19 +92,113 @@ var _ = Describe("NewSessionStore", func() {
 		})
 	}
 
-	SessionStoreInterfaceTests := func() {
+	// The following should only be for server stores
-		Context("when Save is called", func() {
+	PersistentSessionStoreTests := func() {
+		Context("when Clear is called on a persistent store", func() {
+			var resultCookies []*http.Cookie
+
 			BeforeEach(func() {
-				err := ss.Save(response, request, session)
+				req := httptest.NewRequest("GET", "http://example.com/", nil)
+				saveResp := httptest.NewRecorder()
+				err := ss.Save(saveResp, req, session)
+				Expect(err).ToNot(HaveOccurred())
+
+				resultCookies = saveResp.Result().Cookies()
+				for _, c := range resultCookies {
+					request.AddCookie(c)
+				}
+				err = ss.Clear(response, request)
 				Expect(err).ToNot(HaveOccurred())
 			})
 
-			It("sets a `set-cookie` header in the response", func() {
+			Context("attempting to Load", func() {
-				Expect(response.Header().Get("set-cookie")).ToNot(BeEmpty())
+				var loadedAfterClear *sessionsapi.SessionState
+				var loadErr error
+
+				BeforeEach(func() {
+					loadReq := httptest.NewRequest("GET", "http://example.com/", nil)
+					for _, c := range resultCookies {
+						loadReq.AddCookie(c)
+					}
+
+					loadedAfterClear, loadErr = ss.Load(loadReq)
+				})
+
+				It("returns an empty session", func() {
+					Expect(loadedAfterClear).To(BeNil())
+				})
+
+				It("returns an error", func() {
+					Expect(loadErr).To(HaveOccurred())
+				})
 			})
 
-			It("Ensures the session CreatedAt is not zero", func() {
+			CheckCookieOptions()
-				Expect(session.CreatedAt.IsZero()).To(BeFalse())
+		})
+	}
+
+	SessionStoreInterfaceTests := func(persistent bool) {
+		Context("when Save is called", func() {
+			Context("with no existing session", func() {
+				BeforeEach(func() {
+					err := ss.Save(response, request, session)
+					Expect(err).ToNot(HaveOccurred())
+				})
+
+				It("sets a `set-cookie` header in the response", func() {
+					Expect(response.Header().Get("set-cookie")).ToNot(BeEmpty())
+				})
+
+				It("Ensures the session CreatedAt is not zero", func() {
+					Expect(session.CreatedAt.IsZero()).To(BeFalse())
+				})
+			})
+
+			Context("with a broken session", func() {
+				BeforeEach(func() {
+					By("Using a valid cookie with a different providers session encoding")
+					broken := "BrokenSessionFromADifferentSessionImplementation"
+					value := cookie.SignedValue(cookieOpts.CookieSecret, cookieOpts.CookieName, broken, time.Now())
+					cookie := cookies.MakeCookieFromOptions(request, cookieOpts.CookieName, value, cookieOpts, cookieOpts.CookieExpire, time.Now())
+					request.AddCookie(cookie)
+
+					err := ss.Save(response, request, session)
+					Expect(err).ToNot(HaveOccurred())
+				})
+
+				It("sets a `set-cookie` header in the response", func() {
+					Expect(response.Header().Get("set-cookie")).ToNot(BeEmpty())
+				})
+
+				It("Ensures the session CreatedAt is not zero", func() {
+					Expect(session.CreatedAt.IsZero()).To(BeFalse())
+				})
+			})
+
+			Context("with an expired saved session", func() {
+				var err error
+				BeforeEach(func() {
+					By("saving a session")
+					req := httptest.NewRequest("GET", "http://example.com/", nil)
+					saveResp := httptest.NewRecorder()
+					err = ss.Save(saveResp, req, session)
+					Expect(err).ToNot(HaveOccurred())
+
+					By("and clearing the session")
+					for _, c := range saveResp.Result().Cookies() {
+						request.AddCookie(c)
+					}
+					clearResp := httptest.NewRecorder()
+					err = ss.Clear(clearResp, request)
+					Expect(err).ToNot(HaveOccurred())
+
+					By("then saving a request with the cleared session")
+					err = ss.Save(response, request, session)
+				})
+
+				It("no error should occur", func() {
+					Expect(err).ToNot(HaveOccurred())
+				})
 			})
 
 			CheckCookieOptions()
@@ -109,18 +206,15 @@ var _ = Describe("NewSessionStore", func() {
 
 		Context("when Clear is called", func() {
 			BeforeEach(func() {
-				cookie := cookies.MakeCookie(request,
+				req := httptest.NewRequest("GET", "http://example.com/", nil)
-					cookieOpts.CookieName,
+				saveResp := httptest.NewRecorder()
-					"foo",
+				err := ss.Save(saveResp, req, session)
-					cookieOpts.CookiePath,
+				Expect(err).ToNot(HaveOccurred())
-					cookieOpts.CookieDomain,
+
-					cookieOpts.CookieHTTPOnly,
+				for _, c := range saveResp.Result().Cookies() {
-					cookieOpts.CookieSecure,
+					request.AddCookie(c)
-					cookieOpts.CookieExpire,
+				}
-					time.Now(),
+				err = ss.Clear(response, request)
-				)
-				request.AddCookie(cookie)
-				err := ss.Clear(response, request)
 				Expect(err).ToNot(HaveOccurred())
 			})
 
@@ -132,7 +226,38 @@ var _ = Describe("NewSessionStore", func() {
 		})
 
 		Context("when Load is called", func() {
-			var loadedSession *sessionsapi.SessionState
+			LoadSessionTests := func() {
+				var loadedSession *sessionsapi.SessionState
+				BeforeEach(func() {
+					var err error
+					loadedSession, err = ss.Load(request)
+					Expect(err).ToNot(HaveOccurred())
+				})
+
+				It("loads a session equal to the original session", func() {
+					if cookieOpts.CookieSecret == "" {
+						// Only Email and User stored in session when encrypted
+						Expect(loadedSession.Email).To(Equal(session.Email))
+						Expect(loadedSession.User).To(Equal(session.User))
+					} else {
+						// All fields stored in session if encrypted
+
+						// Can't compare time.Time using Equal() so remove ExpiresOn from sessions
+						l := *loadedSession
+						l.CreatedAt = time.Time{}
+						l.ExpiresOn = time.Time{}
+						s := *session
+						s.CreatedAt = time.Time{}
+						s.ExpiresOn = time.Time{}
+						Expect(l).To(Equal(s))
+
+						// Compare time.Time separately
+						Expect(loadedSession.CreatedAt.Equal(session.CreatedAt)).To(BeTrue())
+						Expect(loadedSession.ExpiresOn.Equal(session.ExpiresOn)).To(BeTrue())
+					}
+				})
+			}
+
 			BeforeEach(func() {
 				req := httptest.NewRequest("GET", "http://example.com/", nil)
 				resp := httptest.NewRecorder()
@@ -142,36 +267,57 @@ var _ = Describe("NewSessionStore", func() {
 				for _, cookie := range resp.Result().Cookies() {
 					request.AddCookie(cookie)
 				}
-				loadedSession, err = ss.Load(request)
-				Expect(err).ToNot(HaveOccurred())
 			})
 
-			It("loads a session equal to the original session", func() {
+			Context("before the refresh period", func() {
-				if cookieOpts.CookieSecret == "" {
+				LoadSessionTests()
-					// Only Email and User stored in session when encrypted
-					Expect(loadedSession.Email).To(Equal(session.Email))
-					Expect(loadedSession.User).To(Equal(session.User))
-				} else {
-					// All fields stored in session if encrypted
-
-					// Can't compare time.Time using Equal() so remove ExpiresOn from sessions
-					l := *loadedSession
-					l.CreatedAt = time.Time{}
-					l.ExpiresOn = time.Time{}
-					s := *session
-					s.CreatedAt = time.Time{}
-					s.ExpiresOn = time.Time{}
-					Expect(l).To(Equal(s))
-
-					// Compare time.Time separately
-					Expect(loadedSession.CreatedAt.Equal(session.CreatedAt)).To(BeTrue())
-					Expect(loadedSession.ExpiresOn.Equal(session.ExpiresOn)).To(BeTrue())
-				}
 			})
+
+			// Test TTLs and cleanup of persistent session storage
+			// For non-persistent we rely on the browser cookie lifecycle
+			if persistent {
+				Context("after the refresh period, but before the cookie expire period", func() {
+					BeforeEach(func() {
+						switch ss.(type) {
+						case *redis.SessionStore:
+							mr.FastForward(cookieOpts.CookieRefresh + time.Minute)
+						}
+					})
+
+					LoadSessionTests()
+				})
+
+				Context("after the cookie expire period", func() {
+					var loadedSession *sessionsapi.SessionState
+					var err error
+
+					BeforeEach(func() {
+						switch ss.(type) {
+						case *redis.SessionStore:
+							mr.FastForward(cookieOpts.CookieExpire + time.Minute)
+						}
+
+						loadedSession, err = ss.Load(request)
+						Expect(err).To(HaveOccurred())
+					})
+
+					It("returns an error loading the session", func() {
+						Expect(err).To(HaveOccurred())
+					})
+
+					It("returns an empty session", func() {
+						Expect(loadedSession).To(BeNil())
+					})
+				})
+			}
 		})
+
+		if persistent {
+			PersistentSessionStoreTests()
+		}
 	}
 
-	RunSessionTests := func() {
+	RunSessionTests := func(persistent bool) {
 		Context("with default options", func() {
 			BeforeEach(func() {
 				var err error
@@ -179,7 +325,7 @@ var _ = Describe("NewSessionStore", func() {
 				Expect(err).ToNot(HaveOccurred())
 			})
 
-			SessionStoreInterfaceTests()
+			SessionStoreInterfaceTests(persistent)
 		})
 
 		Context("with non-default options", func() {
@@ -188,7 +334,7 @@ var _ = Describe("NewSessionStore", func() {
 					CookieName:     "_cookie_name",
 					CookiePath:     "/path",
 					CookieExpire:   time.Duration(72) * time.Hour,
-					CookieRefresh:  time.Duration(3600),
+					CookieRefresh:  time.Duration(2) * time.Hour,
 					CookieSecure:   false,
 					CookieHTTPOnly: false,
 					CookieDomain:   "example.com",
@@ -199,7 +345,7 @@ var _ = Describe("NewSessionStore", func() {
 				Expect(err).ToNot(HaveOccurred())
 			})
 
-			SessionStoreInterfaceTests()
+			SessionStoreInterfaceTests(persistent)
 		})
 
 		Context("with a cipher", func() {
@@ -217,7 +363,7 @@ var _ = Describe("NewSessionStore", func() {
 				Expect(err).ToNot(HaveOccurred())
 			})
 
-			SessionStoreInterfaceTests()
+			SessionStoreInterfaceTests(persistent)
 		})
 	}
 
@@ -230,7 +376,7 @@ var _ = Describe("NewSessionStore", func() {
 			CookieName:     "_oauth2_proxy",
 			CookiePath:     "/",
 			CookieExpire:   time.Duration(168) * time.Hour,
-			CookieRefresh:  time.Duration(0),
+			CookieRefresh:  time.Duration(1) * time.Hour,
 			CookieSecure:   true,
 			CookieHTTPOnly: true,
 		}
@@ -260,7 +406,31 @@ var _ = Describe("NewSessionStore", func() {
 		})
 
 		Context("the cookie.SessionStore", func() {
-			RunSessionTests()
+			RunSessionTests(false)
+		})
+	})
+
+	Context("with type 'redis'", func() {
+		BeforeEach(func() {
+			var err error
+			mr, err = miniredis.Run()
+			Expect(err).ToNot(HaveOccurred())
+			opts.Type = options.RedisSessionStoreType
+			opts.RedisConnectionURL = "redis://" + mr.Addr()
+		})
+
+		AfterEach(func() {
+			mr.Close()
+		})
+
+		It("creates a redis.SessionStore", func() {
+			ss, err := sessions.NewSessionStore(opts, cookieOpts)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(ss).To(BeAssignableToTypeOf(&redis.SessionStore{}))
+		})
+
+		Context("the redis.SessionStore", func() {
+			RunSessionTests(true)
 		})
 	})