Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Redis sessions #224

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Redis sessions #224

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
63b6feb
Rough implementation of Redis config options
Jun 20, 2019
d5ffeb4
Construct the redis client from supplied options
Jun 21, 2019
d6c6e64
Redis store wired up to OAuth proxy.
Jun 27, 2019
b5c3167
Configure Redis
Jul 12, 2019
f8a9efd
go mod tidy
Jul 12, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ require (
github.com/benbjohnson/clock v0.0.0-20161215174838-7dc76406b6d3
github.com/bmizerany/assert v0.0.0-20160611221934-b7ed37b82869 // indirect
github.com/datadog/datadog-go v0.0.0-20180822151419-281ae9f2d895
github.com/go-redis/redis v6.15.2+incompatible
github.com/imdario/mergo v0.3.7
github.com/kelseyhightower/envconfig v1.3.0
github.com/mccutchen/go-httpbin v1.1.1
Expand Down
7 changes: 7 additions & 0 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,8 @@ github.com/go-logfmt/logfmt v0.3.0/go.mod h1:Qt1PoO58o5twSAckw1HlFXLmHsOX5/0LbT9
github.com/go-logfmt/logfmt v0.4.0/go.mod h1:3RMwSq7FuexP4Kalkev3ejPJsZTpXXBr9+V4qmtdjCk=
github.com/go-playground/locales v0.12.1/go.mod h1:IUMDtCfWo/w/mtMfIE/IG2K+Ey3ygWanZIBtBW0W2TM=
github.com/go-playground/universal-translator v0.16.0/go.mod h1:1AnU7NaIRDWWzGEKwgtJRd2xk99HeFyHw3yid4rvQIY=
github.com/go-redis/redis v6.15.2+incompatible h1:9SpNVG76gr6InJGxoZ6IuuxaCOQwDAhzyXg+Bs+0Sb4=
github.com/go-redis/redis v6.15.2+incompatible/go.mod h1:NAIEuMOZ/fxfXJIrKDQDz8wamY7mA7PouImQ2Jvg6kA=
github.com/go-redsync/redsync v1.2.0/go.mod h1:QClK/s99KRhfKdpxLTMsI5mSu43iLp0NfOneLPie+78=
github.com/go-stack/stack v1.8.0/go.mod h1:v0f6uXyyMGvRgIKkXu+yp6POWl0qKG85gN/melR3HDY=
github.com/gogo/protobuf v1.1.1/go.mod h1:r8qH/GZQm5c6nD/R0oafs1akxWv10x8SbQlK7atdtwQ=
Expand Down Expand Up @@ -133,6 +135,7 @@ github.com/hashicorp/memberlist v0.1.3/go.mod h1:ajVTdAv/9Im8oMAAj5G31PhhMCZJV2p
github.com/hashicorp/memberlist v0.1.4/go.mod h1:ajVTdAv/9Im8oMAAj5G31PhhMCZJV2pPBoIllUwCN7I=
github.com/hashicorp/serf v0.8.2/go.mod h1:6hOLApaqBFA1NXqRQAsxw9QxuDEvNxSQRwA/JwenrHc=
github.com/hashicorp/serf v0.8.3/go.mod h1:UpNcs7fFbpKIyZaUuSW6EPiH+eZC7OuyFD+wc1oal+k=
github.com/hpcloud/tail v1.0.0 h1:nfCOvKYfkgYP8hkirhJocXT2+zOD8yUNjXaWfTlyFKI=
github.com/hpcloud/tail v1.0.0/go.mod h1:ab1qPbhIpdTxEkNHXyeSf5vhxWSCs/tWer42PpOxQnU=
github.com/ijc/Gotty v0.0.0-20170406111628-a8b993ba6abd/go.mod h1:3LVOLeyx9XVvwPgrt2be44XgSqndprz1G18rSk8KD84=
github.com/imdario/mergo v0.3.7 h1:Y+UAYTZ7gDEuOfhxKWy+dvb5dRQ6rJjFSdX2HZY1/gI=
Expand Down Expand Up @@ -202,8 +205,10 @@ github.com/nlopes/slack v0.5.0/go.mod h1:jVI4BBK3lSktibKahxBF74txcK2vyvkza1z/+rR
github.com/oklog/ulid v1.3.1/go.mod h1:CirwcVhetQ6Lv90oh/F+FBtV6XMibvdAFo93nm5qn4U=
github.com/onsi/ginkgo v1.6.0/go.mod h1:lLunBs/Ym6LB5Z9jYTR76FiuTmxDTDusOGeTQH+WWjE=
github.com/onsi/ginkgo v1.7.0/go.mod h1:lLunBs/Ym6LB5Z9jYTR76FiuTmxDTDusOGeTQH+WWjE=
github.com/onsi/ginkgo v1.8.0 h1:VkHVNpR4iVnU8XQR6DBm8BqYjN7CRzw+xKUbVVbbW9w=
github.com/onsi/ginkgo v1.8.0/go.mod h1:lLunBs/Ym6LB5Z9jYTR76FiuTmxDTDusOGeTQH+WWjE=
github.com/onsi/gomega v1.4.3/go.mod h1:ex+gbHU/CVuBBDIJjb2X0qEXbFg53c61hWP/1CpauHY=
github.com/onsi/gomega v1.5.0 h1:izbySO9zDPmjJ8rDjLvkA2zJHIo+HkYXHnf7eN7SSyo=
github.com/onsi/gomega v1.5.0/go.mod h1:ex+gbHU/CVuBBDIJjb2X0qEXbFg53c61hWP/1CpauHY=
github.com/opencontainers/go-digest v1.0.0-rc1/go.mod h1:cMLVZDEM3+U2I4VmLI6N8jQYUd2OVphdqWwCJHrFt2s=
github.com/opencontainers/image-spec v1.0.1/go.mod h1:BtxoFyWECRxE4U/7sNtV5W15zMzWCbyJoFRP3s7yZA0=
Expand Down Expand Up @@ -386,6 +391,7 @@ gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8
gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127 h1:qIbj1fsPNlZgppZ+VLlY7N33q108Sa+fhmuc+sWQYwY=
gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
gopkg.in/errgo.v2 v2.1.0/go.mod h1:hNsd1EY+bozCKY1Ytp96fpM3vjJbqLJn88ws8XvfDNI=
gopkg.in/fsnotify.v1 v1.4.7 h1:xOHLXZwVvI9hhs+cLKq5+I5onOuwQLhQwiu63xxlHs4=
gopkg.in/fsnotify.v1 v1.4.7/go.mod h1:Tz8NjZHkW78fSQdbUxIjBTcgA1z1m8ZHf0WmKUhAMys=
gopkg.in/go-playground/validator.v9 v9.29.0/go.mod h1:+c9/zcJMFNgbLvly1L1V+PpxWdVbfP1avr/N00E2vyQ=
gopkg.in/redis.v3 v3.6.4/go.mod h1:6XeGv/CrsUFDU9aVbUdNykN7k1zVmoeg83KC9RbQfiU=
Expand All @@ -395,6 +401,7 @@ gopkg.in/src-d/go-git-fixtures.v3 v3.1.1/go.mod h1:dLBcvytrw/TYZsNTWCnkNF2DSIlzW
gopkg.in/src-d/go-git-fixtures.v3 v3.5.0/go.mod h1:dLBcvytrw/TYZsNTWCnkNF2DSIlzWYqTe3rJR56Ac7g=
gopkg.in/src-d/go-git.v4 v4.11.0/go.mod h1:Vtut8izDyrM8BUVQnzJ+YvmNcem2J89EmfZYCkLokZk=
gopkg.in/telegram-bot-api.v4 v4.6.4/go.mod h1:5DpGO5dbumb40px+dXcwCpcjmeHNYLpk0bp3XRNvWDM=
gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7 h1:uRGJdciOHaEIrze2W8Q3AKkepLTh2hOroT7a+7czfdQ=
gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7/go.mod h1:dt/ZhP58zS4L8KSrWDmTeBkI65Dw0HsyUHuEVlX15mw=
gopkg.in/warnings.v0 v0.1.2/go.mod h1:jksf8JmL6Qr/oQM2OXTHunEvvTAsrWBLb6OOjuVWRNI=
gopkg.in/yaml.v2 v2.2.1/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI=
Expand Down
42 changes: 40 additions & 2 deletions internal/auth/configuration.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,18 @@ import (

// DefaultAuthConfig specifies all the defaults used to configure sso-auth
// All configuration can be set using environment variables. Below is a list of
// configuration variables via their envivronment configuration
// configuration variables via their environment configuration
//
// SESSION COOKIE_NAME
// SESSION_COOKIE_NAME
// SESSION_COOKIE_SECRET
// SESSION_COOKIE_EXPIRE
// SESSION_COOKIE_DOMAIN
// SESSION_COOKIE_REFRESH
// SESSION_COOKIE_SECURE
// SESSION_COOKIE_HTTPONLY
// SESSION_REDIS_CONNECTION
// SESSION_REDIS_SENTINEL
// SESSION_REDIS_MASTER
// SESSION_LIFETIME
// SESSION_KEY
//
Expand All @@ -35,6 +38,9 @@ import (
// PROVIDER_*_CLIENT_SECRET
// PROVIDER_*_SCOPE
//
// PROVIDER_*_AZURE_TENANT
// PROVIDER_*_AZURE_PROMPT
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

//
// PROVIDER_*_GOOGLE_CREDENTIALS
// PROVIDER_*_GOOGLE_IMPERSONATE
//
Expand Down Expand Up @@ -84,6 +90,9 @@ func DefaultAuthConfig() Configuration {
Secure: true,
HTTPOnly: true,
},
RedisConfig: RedisConfig{
UseSentinel: false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

},
},
LoggingConfig: LoggingConfig{
Enable: true,
Expand Down Expand Up @@ -275,8 +284,34 @@ func (gcc GroupCacheConfig) Validate() error {
return nil
}

type RedisConfig struct {
ConnectionURLs []string `mapstructure:"connection"`
UseSentinel bool `mapstructure:"sentinel"`
SentinelMasterName string `mapstructure:"master"`
}

func (rc RedisConfig) Enabled() bool {
// If redis connection URL is missing, use normal cookie sessions
return rc.UseSentinel || len(rc.ConnectionURLs) >= 1
}

func (rc RedisConfig) Validate() error {
if rc.UseSentinel {
if rc.SentinelMasterName == "" {
return xerrors.New("no sentinel master is configured")
} else if len(rc.ConnectionURLs) == 0 {
return xerrors.New("no sentinel connection URLs are configured")
}
} else if len(rc.ConnectionURLs) > 1 {
return xerrors.New("only one connection URL should be set unless sentinel enabled")
}
// If redis connection URL is missing, use normal cookie sessions
return nil
}

type SessionConfig struct {
CookieConfig CookieConfig `mapstructure:"cookie"`
RedisConfig RedisConfig `mapstructure:"redis"`

SessionLifetimeTTL time.Duration `mapstructure:"lifetime"`
Key string `mapstructure:"key"`
Expand All @@ -298,6 +333,9 @@ func (sc SessionConfig) Validate() error {
if err := sc.CookieConfig.Validate(); err != nil {
return xerrors.Errorf("invalid session.cookie config: %w", err)
}
if err := sc.RedisConfig.Validate(); err != nil {
return xerrors.Errorf("invalid session.redis config: %w", err)
}

return nil
}
Expand Down
51 changes: 51 additions & 0 deletions internal/auth/configuration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,57 @@ func TestEnvironmentOverridesConfiguration(t *testing.T) {
assertEq("foo-client-id", foo.ClientConfig.ID, t)
},
},
{
Name: "Test Sessions",
EnvOverrides: map[string]string{
"SESSION_LIFETIME": "1h",
"SESSION_KEY": "abcdefg",
},
CheckFunc: func(c Configuration, t *testing.T) {
assertEq(1*time.Hour, c.SessionConfig.SessionLifetimeTTL, t)
assertEq("abcdefg", c.SessionConfig.Key, t)
},
},
{
Name: "Test Redis Sessions",
EnvOverrides: map[string]string{
"SESSION_REDIS_CONNECTION": "redis://master.example.com:6379",
},
CheckFunc: func(c Configuration, t *testing.T) {
redisConf := c.SessionConfig.RedisConfig
assertEq([]string{"redis://master.example.com:6379"}, redisConf.ConnectionURLs, t)
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

},
{
Name: "Test Redis Sentinel Sessions",
EnvOverrides: map[string]string{
"SESSION_REDIS_SENTINEL": "true",
"SESSION_REDIS_MASTER": "redis://master.example.com:6379",
"SESSION_REDIS_CONNECTION": "redis://instance-001.example.com:6379,redis://instance-002.example.com:6379,redis://instance-003.example.com:6379",
},
CheckFunc: func(c Configuration, t *testing.T) {
redisConf := c.SessionConfig.RedisConfig
assertEq(true, redisConf.UseSentinel, t)
assertEq("redis://master.example.com:6379", redisConf.SentinelMasterName, t)
assertEq([]string{
"redis://instance-001.example.com:6379",
"redis://instance-002.example.com:6379",
"redis://instance-003.example.com:6379",
}, redisConf.ConnectionURLs, t)
},
},
{
Name: "Test Cookie Sessions",
EnvOverrides: map[string]string{
"SESSION_COOKIE_NAME": "sso_auth_test",
"SESSION_COOKIE_SECRET": "s3kr1t",
},
CheckFunc: func(c Configuration, t *testing.T) {
cookieConf := c.SessionConfig.CookieConfig
assertEq("sso_auth_test", cookieConf.Name, t)
assertEq("s3kr1t", cookieConf.Secret, t)
},
},
{
Name: "Test Multiple Providers",
EnvOverrides: map[string]string{
Expand Down
12 changes: 9 additions & 3 deletions internal/auth/mux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,19 @@ func NewAuthenticatorMux(config Configuration, statsdClient *statsd.Client) (*Au
}

idpSlug := idp.Data().ProviderSlug
authenticator, err := NewAuthenticator(config,
opts := []func(*Authenticator) error{
SetValidator(validator),
SetProvider(idp),
SetCookieStore(config.SessionConfig, idpSlug),
SetStatsdClient(statsdClient),
SetRedirectURL(config.ServerConfig, idpSlug),
)
}

if config.SessionConfig.RedisConfig.Enabled() {
opts = append(opts, SetRedisStore(config.SessionConfig, idpSlug))
} else {
opts = append(opts, SetCookieStore(config.SessionConfig, idpSlug))
}
authenticator, err := NewAuthenticator(config, opts...)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if err != nil {
logger.Error(err, "error creating new Authenticator")
return nil, err
Expand Down
53 changes: 53 additions & 0 deletions internal/auth/options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,3 +149,56 @@ func SetCookieStore(sessionConfig SessionConfig, providerSlug string) func(*Auth
return nil
}
}

// SetRedisStore sets the session store to use redis
func SetRedisStore(sessionConfig SessionConfig, providerSlug string) func(*Authenticator) error {
return func(a *Authenticator) error {
decodedKey, err := base64.StdEncoding.DecodeString(sessionConfig.Key)
if err != nil {
return err
}

codeCipher, err := aead.NewMiscreantCipher([]byte(decodedKey))
if err != nil {
return err
}

cc := sessionConfig.CookieConfig
rc := sessionConfig.RedisConfig
decodedCookieSecret, err := base64.StdEncoding.DecodeString(cc.Secret)
if err != nil {
return err
}

cookieName := fmt.Sprintf("%s_%s", cc.Name, providerSlug)
redisStore, err := sessions.NewRedisStore(cookieName,
func(c *sessions.RedisStore) error {
return sessions.CreateMiscreantCookieCipher(decodedCookieSecret)(&c.CookieStore)
},
func(c *sessions.RedisStore) error {
if rc.UseSentinel {
c.UseSentinel = rc.UseSentinel
c.SentinelMasterName = rc.SentinelMasterName
c.SentinelConnectionURLs = rc.ConnectionURLs
} else {
c.UseSentinel = rc.UseSentinel
c.ConnectionURL = rc.ConnectionURLs[0]
}

c.CookieDomain = cc.Domain
c.CookieHTTPOnly = cc.HTTPOnly
c.CookieExpire = cc.Expire
c.CookieSecure = cc.Secure
return nil
})

if err != nil {
return err
}

a.csrfStore = redisStore
a.sessionStore = redisStore
a.AuthCodeCipher = codeCipher
return nil
}
}