diff --git a/oauthproxy.go b/oauthproxy.go index 1975c7b9..a58fbfbc 100644 --- a/oauthproxy.go +++ b/oauthproxy.go @@ -176,11 +176,11 @@ func NewOAuthProxy(opts *options.Options, validator func(string) bool) (*OAuthPr logger.Printf("OAuthProxy configured for %s Client ID: %s", provider.Data().ProviderName, opts.Providers[0].ClientID) refresh := "disabled" - if opts.Cookie.Refresh != time.Duration(0) { - refresh = fmt.Sprintf("after %s", opts.Cookie.Refresh) + if opts.Session.Refresh != time.Duration(0) { + refresh = fmt.Sprintf("after %s", opts.Session.Refresh) } - logger.Printf("Cookie settings: name:%s secure(https):%v httponly:%v expiry:%s domains:%s path:%s samesite:%s refresh:%s", opts.Cookie.Name, opts.Cookie.Secure, opts.Cookie.HTTPOnly, opts.Cookie.Expire, strings.Join(opts.Cookie.Domains, ","), opts.Cookie.Path, opts.Cookie.SameSite, refresh) + logger.Printf("Cookie settings: name:%s insecure(http):%v nothttponly:%v expiry:%s domains:%s path:%s samesite:%s refresh:%s", opts.Cookie.Name, opts.Cookie.Insecure, opts.Cookie.NotHttpOnly, opts.Cookie.Expire, strings.Join(opts.Cookie.Domains, ","), opts.Cookie.Path, opts.Cookie.SameSite, refresh) trustedIPs := ip.NewNetSet() for _, ipStr := range opts.TrustedIPs { @@ -416,7 +416,7 @@ func buildSessionChain(opts *options.Options, provider providers.Provider, sessi chain = chain.Append(middleware.NewStoredSessionLoader(&middleware.StoredSessionLoaderOptions{ SessionStore: sessionStore, - RefreshPeriod: opts.Cookie.Refresh, + RefreshPeriod: opts.Session.Refresh, RefreshSession: provider.RefreshSession, ValidateSession: provider.ValidateSession, })) @@ -1097,9 +1097,9 @@ func (p *OAuthProxy) getOAuthRedirectURI(req *http.Request) string { rd.Scheme = schemeHTTP } - // If CookieSecure is true, return `https` no matter what + // If CookieInsecure is false, return `https` no matter what // Not all reverse proxies set X-Forwarded-Proto - if ptr.Deref(p.CookieOptions.Secure, options.DefaultCookieSecure) { + if !ptr.Deref(p.CookieOptions.Insecure, options.DefaultCookieInsecure) { rd.Scheme = schemeHTTPS } return rd.String() diff --git a/oauthproxy_test.go b/oauthproxy_test.go index 20360af4..722d84ee 100644 --- a/oauthproxy_test.go +++ b/oauthproxy_test.go @@ -819,9 +819,9 @@ func NewProcessCookieTest(opts ProcessCookieTestOpts, modifiers ...OptionsModifi for _, modifier := range modifiers { modifier(pcTest.opts) } - // First, set the CookieRefresh option so proxy.AesCipher is created, + // First, set the Session Refresh option so proxy.AesCipher is created, // needed to encrypt the access_token. - pcTest.opts.Cookie.Refresh = time.Hour + pcTest.opts.Session.Refresh = time.Hour err := validation.Validate(pcTest.opts) if err != nil { return nil, err @@ -845,9 +845,9 @@ func NewProcessCookieTest(opts ProcessCookieTestOpts, modifiers ...OptionsModifi } pcTest.proxy.provider = testProvider - // Now, zero-out proxy.CookieRefresh for the cases that don't involve + // Now, zero-out Session Refresh for the cases that don't involve // access_token validation. - pcTest.proxy.CookieOptions.Refresh = time.Duration(0) + pcTest.opts.Session.Refresh = time.Duration(0) pcTest.rw = httptest.NewRecorder() pcTest.req, _ = http.NewRequest("GET", "/", strings.NewReader("")) pcTest.validateUser = true @@ -969,7 +969,7 @@ func TestProcessCookieFailIfRefreshSetAndCookieExpired(t *testing.T) { err = pcTest.SaveSession(startSession) assert.NoError(t, err) - pcTest.proxy.CookieOptions.Refresh = time.Hour + pcTest.opts.Session.Refresh = time.Hour session, err := pcTest.LoadCookiedSession() assert.NotEqual(t, nil, err) if session != nil { diff --git a/pkg/apis/options/cookie.go b/pkg/apis/options/cookie.go index 2d594b75..6269bfbf 100644 --- a/pkg/apis/options/cookie.go +++ b/pkg/apis/options/cookie.go @@ -2,46 +2,53 @@ package options import ( "fmt" - "os" "time" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/util/ptr" + "go.yaml.in/yaml/v3" ) const ( - // DefaultCookieSecure is the default value for Cookie.Secure - DefaultCookieSecure bool = true - // DefaultCookieHTTPOnly is the default value for Cookie.HTTPOnly - DefaultCookieHTTPOnly bool = true + // DefaultCookieInsecure is the default value for Cookie.Insecure + DefaultCookieInsecure bool = false + // DefaultCookieNotHttpOnly is the default value for Cookie.NotHttpOnly + DefaultCookieNotHttpOnly bool = false // DefaultCSRFPerRequest is the default value for Cookie.CSRFPerRequest DefaultCSRFPerRequest bool = false ) +type SameSiteMode string + +const ( + SameSiteLax SameSiteMode = "lax" + SameSiteStrict SameSiteMode = "strict" + SameSiteNone SameSiteMode = "none" + SameSiteDefault SameSiteMode = "" +) + // Cookie contains configuration options relating session and CSRF cookies type Cookie struct { // Name is the name of the cookie Name string `yaml:"name,omitempty"` - // Secret is the secret used to encrypt/sign the cookie value - Secret string `yaml:"secret,omitempty"` - // SecretFile is a file containing the secret used to encrypt/sign the cookie value - // instead of specifying it directly in the config. Secret takes precedence over SecretFile - SecretFile string `yaml:"secretFile,omitempty"` + // Secret is the secret source used to encrypt/sign the cookie value + Secret SecretSource `yaml:"secret,omitempty"` // Domains is a list of domains for which the cookie is valid Domains []string `yaml:"domains,omitempty"` // Path is the path for which the cookie is valid Path string `yaml:"path,omitempty"` // Expire is the duration before the cookie expires Expire time.Duration `yaml:"expire,omitempty"` - // Refresh is the duration after which the cookie is refreshable - Refresh time.Duration `yaml:"refresh,omitempty"` - // Secure indicates whether the cookie is only sent over HTTPS - Secure *bool `yaml:"secure,omitempty"` - // HTTPOnly indicates whether the cookie is inaccessible to JavaScript - HTTPOnly *bool `yaml:"httpOnly,omitempty"` + // Insecure indicates whether the cookie allows to be sent over HTTP + // Default is false, which requires HTTPS + Insecure *bool `yaml:"insecure,omitempty"` + // NotHttpOnly is the inverse of HTTPOnly; indicates whether the cookie is accessible to JavaScript + // Default is false, which helps mitigate certain XSS attacks + NotHttpOnly *bool `yaml:"notHttpOnly,omitempty"` // SameSite sets the SameSite attribute on the cookie - SameSite string `yaml:"sameSite,omitempty"` + SameSite SameSiteMode `yaml:"sameSite,omitempty"` // CSRFPerRequest indicates whether a unique CSRF token is generated for each request // Enables parallel requests from clients (e.g., multiple tabs) + // Default is false, which uses a single CSRF token per session CSRFPerRequest *bool `yaml:"csrfPerRequest,omitempty"` // CSRFPerRequestLimit sets a limit on the number of valid CSRF tokens when CSRFPerRequest is enabled // Used to prevent unbounded memory growth from storing too many tokens @@ -50,18 +57,28 @@ type Cookie struct { CSRFExpire time.Duration `yaml:"csrfExpire,omitempty"` } -// GetSecret returns the cookie secret, reading from file if SecretFile is set -func (c *Cookie) GetSecret() (secret string, err error) { - if c.Secret != "" || c.SecretFile == "" { - return c.Secret, nil +func (m *SameSiteMode) UnmarshalYAML(value *yaml.Node) error { + var s string + if err := value.Decode(&s); err != nil { + return err } + switch SameSiteMode(s) { + case SameSiteLax, SameSiteStrict, SameSiteNone, SameSiteDefault: + *m = SameSiteMode(s) + return nil + default: + return fmt.Errorf("invalid same site mode: %s", s) + } +} - fileSecret, err := os.ReadFile(c.SecretFile) +// GetSecret returns the cookie secret as a string from the SecretSource +func (c *Cookie) GetSecret() (string, error) { + secret, err := c.Secret.GetSecretValue() if err != nil { - return "", fmt.Errorf("error reading cookie secret file %s: %w", c.SecretFile, err) + return "", fmt.Errorf("error getting cookie secret: %w", err) } - return string(fileSecret), nil + return string(secret), nil } // EnsureDefaults sets any default values for the Cookie configuration @@ -75,11 +92,11 @@ func (c *Cookie) EnsureDefaults() { if c.Expire == 0 { c.Expire = time.Duration(168) * time.Hour } - if c.Secure == nil { - c.Secure = ptr.To(DefaultCookieSecure) + if c.Insecure == nil { + c.Insecure = ptr.To(DefaultCookieInsecure) } - if c.HTTPOnly == nil { - c.HTTPOnly = ptr.To(DefaultCookieHTTPOnly) + if c.NotHttpOnly == nil { + c.NotHttpOnly = ptr.To(DefaultCookieNotHttpOnly) } if c.CSRFPerRequest == nil { c.CSRFPerRequest = ptr.To(DefaultCSRFPerRequest) diff --git a/pkg/apis/options/cookie_test.go b/pkg/apis/options/cookie_test.go index 4fb0e62e..053d0e6b 100644 --- a/pkg/apis/options/cookie_test.go +++ b/pkg/apis/options/cookie_test.go @@ -10,8 +10,10 @@ import ( func TestCookieGetSecret(t *testing.T) { t.Run("returns secret when Secret is set", func(t *testing.T) { c := &Cookie{ - Secret: "my-secret", - SecretFile: "", + Secret: SecretSource{ + Value: []byte("my-secret"), + FromFile: "", + }, } secret, err := c.GetSecret() assert.NoError(t, err) @@ -20,8 +22,10 @@ func TestCookieGetSecret(t *testing.T) { t.Run("returns secret when both Secret and SecretFile are set", func(t *testing.T) { c := &Cookie{ - Secret: "my-secret", - SecretFile: "/some/file", + Secret: SecretSource{ + Value: []byte("my-secret"), + FromFile: "/some/file", + }, } secret, err := c.GetSecret() assert.NoError(t, err) @@ -39,8 +43,10 @@ func TestCookieGetSecret(t *testing.T) { tmpfile.Close() c := &Cookie{ - Secret: "", - SecretFile: tmpfile.Name(), + Secret: SecretSource{ + Value: []byte(""), + FromFile: tmpfile.Name(), + }, } secret, err := c.GetSecret() assert.NoError(t, err) @@ -49,8 +55,10 @@ func TestCookieGetSecret(t *testing.T) { t.Run("returns error when file does not exist", func(t *testing.T) { c := &Cookie{ - Secret: "", - SecretFile: "/nonexistent/file", + Secret: SecretSource{ + Value: []byte(""), + FromFile: "/nonexistent/file", + }, } secret, err := c.GetSecret() assert.Error(t, err) @@ -60,8 +68,10 @@ func TestCookieGetSecret(t *testing.T) { t.Run("returns empty when both Secret and SecretFile are empty", func(t *testing.T) { c := &Cookie{ - Secret: "", - SecretFile: "", + Secret: SecretSource{ + Value: []byte(""), + FromFile: "", + }, } secret, err := c.GetSecret() assert.NoError(t, err) diff --git a/pkg/apis/options/legacy_cookie.go b/pkg/apis/options/legacy_cookie.go index 287db87f..2d15f16a 100644 --- a/pkg/apis/options/legacy_cookie.go +++ b/pkg/apis/options/legacy_cookie.go @@ -1,6 +1,7 @@ package options import ( + "encoding/base64" "time" "github.com/spf13/pflag" @@ -39,21 +40,34 @@ func legacyCookieFlagSet() *pflag.FlagSet { flagSet.Bool("cookie-csrf-per-request", false, "When this property is set to true, then the CSRF cookie name is built based on the state and varies per request. If property is set to false, then CSRF cookie has the same name for all requests.") flagSet.Int("cookie-csrf-per-request-limit", 0, "Sets a limit on the number of CSRF requests cookies that oauth2-proxy will create. The oldest cookies will be removed. Useful if users end up with 431 Request headers too large status codes.") flagSet.Duration("cookie-csrf-expire", time.Duration(15)*time.Minute, "expire timeframe for CSRF cookie") + return flagSet } func (l *LegacyCookie) convert() Cookie { + // Invert Secure and HTTPOnly to match the new Cookie struct + // which uses Insecure and NotHttpOnly + insecure := !l.Secure + notHttpOnly := !l.HTTPOnly + + secretValue := make([]byte, 0) + + if l.Secret != "" { + base64.StdEncoding.Encode([]byte(l.Secret), secretValue) + } + return Cookie{ - Name: l.Name, - Secret: l.Secret, - SecretFile: l.SecretFile, + Name: l.Name, + Secret: SecretSource{ + Value: secretValue, + FromFile: l.SecretFile, + }, Domains: l.Domains, Path: l.Path, Expire: l.Expire, - Refresh: l.Refresh, - Secure: &l.Secure, - HTTPOnly: &l.HTTPOnly, - SameSite: l.SameSite, + Insecure: &insecure, + NotHttpOnly: ¬HttpOnly, + SameSite: SameSiteMode(l.SameSite), CSRFPerRequest: &l.CSRFPerRequest, CSRFPerRequestLimit: l.CSRFPerRequestLimit, CSRFExpire: l.CSRFExpire, diff --git a/pkg/apis/options/legacy_options.go b/pkg/apis/options/legacy_options.go index d8033a0d..404debd1 100644 --- a/pkg/apis/options/legacy_options.go +++ b/pkg/apis/options/legacy_options.go @@ -23,6 +23,9 @@ type LegacyOptions struct { // Legacy options for cookie configuration LegacyCookie LegacyCookie `cfg:",squash"` + // Legacy options for session store configuration + LegacySessionOptions LegacySessionOptions `cfg:",squash"` + Options Options `cfg:",squash"` } @@ -74,6 +77,13 @@ func NewLegacyOptions() *LegacyOptions { CSRFExpire: time.Duration(15) * time.Minute, }, + LegacySessionOptions: LegacySessionOptions{ + Type: "cookie", + Cookie: LegacyCookieStoreOptions{ + Minimal: false, + }, + }, + Options: *NewOptions(), } } @@ -87,6 +97,7 @@ func NewLegacyFlagSet() *pflag.FlagSet { flagSet.AddFlagSet(legacyProviderFlagSet()) flagSet.AddFlagSet(legacyGoogleFlagSet()) flagSet.AddFlagSet(legacyCookieFlagSet()) + flagSet.AddFlagSet(legacySessionFlagSet()) return flagSet } @@ -109,6 +120,7 @@ func (l *LegacyOptions) ToOptions() (*Options, error) { } l.Options.Providers = providers l.Options.Cookie = l.LegacyCookie.convert() + l.Options.Session = l.LegacySessionOptions.convert(l.LegacyCookie.Refresh) l.Options.EnsureDefaults() diff --git a/pkg/apis/options/legacy_options_test.go b/pkg/apis/options/legacy_options_test.go index f69e6fea..b6be7a73 100644 --- a/pkg/apis/options/legacy_options_test.go +++ b/pkg/apis/options/legacy_options_test.go @@ -1094,13 +1094,12 @@ var _ = Describe("Legacy Options", func() { // Test cases and expected outcomes fullCookie := Cookie{ Name: "_oauth2_proxy", - Secret: "", + Secret: SecretSource{}, Domains: nil, Path: "/", Expire: time.Duration(168) * time.Hour, - Refresh: time.Duration(0), - Secure: ptr.To(true), - HTTPOnly: ptr.To(true), + Insecure: ptr.To(false), + NotHttpOnly: ptr.To(false), SameSite: "", CSRFPerRequest: ptr.To(false), CSRFPerRequestLimit: 0, diff --git a/pkg/apis/options/legacy_sessions.go b/pkg/apis/options/legacy_sessions.go new file mode 100644 index 00000000..08a2b263 --- /dev/null +++ b/pkg/apis/options/legacy_sessions.go @@ -0,0 +1,79 @@ +package options + +import ( + "time" + + "github.com/spf13/pflag" +) + +// LegacySessionOptions contains configuration options for the SessionStore providers. +type LegacySessionOptions struct { + Type string `flag:"session-store-type" cfg:"session_store_type"` + Cookie LegacyCookieStoreOptions `cfg:",squash"` + Redis LegacyRedisStoreOptions `cfg:",squash"` +} + +// LegacyCookieStoreOptions contains configuration options for the CookieSessionStore. +type LegacyCookieStoreOptions struct { + Minimal bool `flag:"session-cookie-minimal" cfg:"session_cookie_minimal"` +} + +// RedisStoreOptions contains configuration options for the RedisSessionStore. +type LegacyRedisStoreOptions struct { + ConnectionURL string `flag:"redis-connection-url" cfg:"redis_connection_url"` + Username string `flag:"redis-username" cfg:"redis_username"` + Password string `flag:"redis-password" cfg:"redis_password"` + UseSentinel bool `flag:"redis-use-sentinel" cfg:"redis_use_sentinel"` + SentinelPassword string `flag:"redis-sentinel-password" cfg:"redis_sentinel_password"` + SentinelMasterName string `flag:"redis-sentinel-master-name" cfg:"redis_sentinel_master_name"` + SentinelConnectionURLs []string `flag:"redis-sentinel-connection-urls" cfg:"redis_sentinel_connection_urls"` + UseCluster bool `flag:"redis-use-cluster" cfg:"redis_use_cluster"` + ClusterConnectionURLs []string `flag:"redis-cluster-connection-urls" cfg:"redis_cluster_connection_urls"` + CAPath string `flag:"redis-ca-path" cfg:"redis_ca_path"` + InsecureSkipTLSVerify bool `flag:"redis-insecure-skip-tls-verify" cfg:"redis_insecure_skip_tls_verify"` + IdleTimeout int `flag:"redis-connection-idle-timeout" cfg:"redis_connection_idle_timeout"` +} + +func legacySessionFlagSet() *pflag.FlagSet { + flagSet := pflag.NewFlagSet("session", pflag.ExitOnError) + + flagSet.String("session-store-type", "cookie", "the session storage provider to use") + flagSet.Bool("session-cookie-minimal", false, "strip OAuth tokens from cookie session stores if they aren't needed (cookie session store only)") + flagSet.String("redis-connection-url", "", "URL of redis server for redis session storage (eg: redis://[USER[:PASSWORD]@]HOST[:PORT])") + flagSet.String("redis-username", "", "Redis username. Applicable for Redis configurations where ACL has been configured. Will override any username set in `--redis-connection-url`") + flagSet.String("redis-password", "", "Redis password. Applicable for all Redis configurations. Will override any password set in `--redis-connection-url`") + 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-password", "", "Redis sentinel password. Used only for sentinel connection; any redis node passwords need to use `--redis-password`") + flagSet.String("redis-sentinel-master-name", "", "Redis sentinel master name. Used in conjunction with --redis-use-sentinel") + flagSet.String("redis-ca-path", "", "Redis custom CA path") + flagSet.Bool("redis-insecure-skip-tls-verify", false, "Use insecure TLS connection to redis") + flagSet.StringSlice("redis-sentinel-connection-urls", []string{}, "List of Redis sentinel connection URLs (eg redis://[USER[:PASSWORD]@]HOST[:PORT]). Used in conjunction with --redis-use-sentinel") + flagSet.Bool("redis-use-cluster", false, "Connect to redis cluster. Must set --redis-cluster-connection-urls to use this feature") + flagSet.StringSlice("redis-cluster-connection-urls", []string{}, "List of Redis cluster connection URLs (eg redis://[USER[:PASSWORD]@]HOST[:PORT]). Used in conjunction with --redis-use-cluster") + flagSet.Int("redis-connection-idle-timeout", 0, "Redis connection idle timeout seconds, if Redis timeout option is non-zero, the --redis-connection-idle-timeout must be less then Redis timeout option") + + return flagSet +} + +func (l *LegacySessionOptions) convert(legacyCookieRefresh time.Duration) SessionOptions { + return SessionOptions{ + Type: SessionStoreType(l.Type), + Refresh: legacyCookieRefresh, + Cookie: CookieStoreOptions{ + Minimal: &l.Cookie.Minimal, + }, + Redis: RedisStoreOptions{ + ConnectionURL: l.Redis.ConnectionURL, + Password: l.Redis.Password, + UseSentinel: &l.Redis.UseSentinel, + SentinelPassword: l.Redis.SentinelPassword, + SentinelMasterName: l.Redis.SentinelMasterName, + SentinelConnectionURLs: l.Redis.SentinelConnectionURLs, + UseCluster: &l.Redis.UseCluster, + ClusterConnectionURLs: l.Redis.ClusterConnectionURLs, + CAPath: l.Redis.CAPath, + InsecureSkipTLSVerify: &l.Redis.InsecureSkipTLSVerify, + IdleTimeout: l.Redis.IdleTimeout, + }, + } +} diff --git a/pkg/apis/options/load_test.go b/pkg/apis/options/load_test.go index e6d7a890..fca8147e 100644 --- a/pkg/apis/options/load_test.go +++ b/pkg/apis/options/load_test.go @@ -60,6 +60,13 @@ var _ = Describe("Load", func() { CSRFExpire: time.Duration(15) * time.Minute, }, + LegacySessionOptions: LegacySessionOptions{ + Type: "cookie", + Cookie: LegacyCookieStoreOptions{ + Minimal: false, + }, + }, + Options: Options{ BearerTokenLoginFallback: true, ProxyPrefix: "/oauth2", @@ -67,7 +74,6 @@ var _ = Describe("Load", func() { ReadyPath: "/ready", RealClientIPHeader: "X-Real-IP", ForceHTTPS: false, - Session: sessionOptionsDefaults(), Templates: templatesDefaults(), SkipAuthPreflight: false, Logging: loggingDefaults(), diff --git a/pkg/apis/options/options.go b/pkg/apis/options/options.go index 02421eee..3555f55f 100644 --- a/pkg/apis/options/options.go +++ b/pkg/apis/options/options.go @@ -105,7 +105,6 @@ func NewOptions() *Options { ReadyPath: "/ready", RealClientIPHeader: "X-Real-IP", ForceHTTPS: false, - Session: sessionOptionsDefaults(), Templates: templatesDefaults(), SkipAuthPreflight: false, Logging: loggingDefaults(), @@ -144,20 +143,6 @@ func NewFlagSet() *pflag.FlagSet { flagSet.String("ping-path", "/ping", "the ping endpoint that can be used for basic health checks") flagSet.String("ping-user-agent", "", "special User-Agent that will be used for basic health checks") flagSet.String("ready-path", "/ready", "the ready endpoint that can be used for deep health checks") - flagSet.String("session-store-type", "cookie", "the session storage provider to use") - flagSet.Bool("session-cookie-minimal", false, "strip OAuth tokens from cookie session stores if they aren't needed (cookie session store only)") - flagSet.String("redis-connection-url", "", "URL of redis server for redis session storage (eg: redis://[USER[:PASSWORD]@]HOST[:PORT])") - flagSet.String("redis-username", "", "Redis username. Applicable for Redis configurations where ACL has been configured. Will override any username set in `--redis-connection-url`") - flagSet.String("redis-password", "", "Redis password. Applicable for all Redis configurations. Will override any password set in `--redis-connection-url`") - 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-password", "", "Redis sentinel password. Used only for sentinel connection; any redis node passwords need to use `--redis-password`") - flagSet.String("redis-sentinel-master-name", "", "Redis sentinel master name. Used in conjunction with --redis-use-sentinel") - flagSet.String("redis-ca-path", "", "Redis custom CA path") - flagSet.Bool("redis-insecure-skip-tls-verify", false, "Use insecure TLS connection to redis") - flagSet.StringSlice("redis-sentinel-connection-urls", []string{}, "List of Redis sentinel connection URLs (eg redis://[USER[:PASSWORD]@]HOST[:PORT]). Used in conjunction with --redis-use-sentinel") - flagSet.Bool("redis-use-cluster", false, "Connect to redis cluster. Must set --redis-cluster-connection-urls to use this feature") - flagSet.StringSlice("redis-cluster-connection-urls", []string{}, "List of Redis cluster connection URLs (eg redis://[USER[:PASSWORD]@]HOST[:PORT]). Used in conjunction with --redis-use-cluster") - flagSet.Int("redis-connection-idle-timeout", 0, "Redis connection idle timeout seconds, if Redis timeout option is non-zero, the --redis-connection-idle-timeout must be less then Redis timeout option") flagSet.String("signature-key", "", "GAP-Signature request signature key (algorithm:secretkey)") flagSet.Bool("gcp-healthchecks", false, "Enable GCP/GKE healthcheck endpoints") @@ -181,9 +166,8 @@ func (o *Options) EnsureDefaults() { } o.Cookie.EnsureDefaults() - + o.Session.EnsureDefaults() // TBD: Uncomment as we add EnsureDefaults methods - // o.Session.EnsureDefaults() // o.Templates.EnsureDefaults() // o.Logging.EnsureDefaults() } diff --git a/pkg/apis/options/secret_source.go b/pkg/apis/options/secret_source.go index 8b783342..6a671b27 100644 --- a/pkg/apis/options/secret_source.go +++ b/pkg/apis/options/secret_source.go @@ -1,5 +1,11 @@ package options +import ( + "encoding/base64" + "fmt" + "os" +) + // SecretSource references an individual secret value. // Only one source within the struct should be defined at any time. type SecretSource struct { @@ -13,6 +19,31 @@ type SecretSource struct { FromFile string `yaml:"fromFile,omitempty"` } +func (ss *SecretSource) GetSecretValue() ([]byte, error) { + if len(ss.Value) > 0 { + var decoded []byte + if _, err := base64.StdEncoding.Decode(decoded, ss.Value); err != nil { + return nil, fmt.Errorf("error decoding secret value: %w", err) + } + return decoded, nil + } + + if ss.FromEnv != "" { + envValue := os.Getenv(ss.FromEnv) + return []byte(envValue), nil + } + + if ss.FromFile != "" { + fileData, err := os.ReadFile(ss.FromFile) + if err != nil { + return nil, fmt.Errorf("error reading secret from file %q: %w", ss.FromFile, err) + } + return fileData, nil + } + + return nil, nil +} + // EnsureDefaults sets any default values for SecretSource fields. func (ss *SecretSource) EnsureDefaults() { // No defaults to set currently diff --git a/pkg/apis/options/sessions.go b/pkg/apis/options/sessions.go index c90c0ac2..8b51b6ef 100644 --- a/pkg/apis/options/sessions.go +++ b/pkg/apis/options/sessions.go @@ -1,46 +1,101 @@ package options +import ( + "time" + + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/util/ptr" +) + +type SessionStoreType string + +const ( + // CookieSessionStoreType is used to indicate the CookieSessionStore should be + // used for storing sessions. + CookieSessionStoreType SessionStoreType = "cookie" + + // RedisSessionStoreType is used to indicate the RedisSessionStore should be + // used for storing sessions. + RedisSessionStoreType SessionStoreType = "redis" + + // DefaultCookieStoreMinimal is the default value for CookieStoreOptions.Minimal + DefaultCookieStoreMinimal bool = false + + // DefaultRedisStoreUseSentinel is the default value for RedisStoreOptions.UseSentinel + DefaultRedisStoreUseSentinel bool = false + + // DefaultRedisStoreUseCluster is the default value for RedisStoreOptions.UseCluster + DefaultRedisStoreUseCluster bool = false + + // DefaultRedisStoreInsecureSkipTLSVerify is the default value for RedisStoreOptions.InsecureSkipTLSVerify + DefaultRedisStoreInsecureSkipTLSVerify bool = false +) + // SessionOptions contains configuration options for the SessionStore providers. type SessionOptions struct { - Type string `flag:"session-store-type" cfg:"session_store_type"` - Cookie CookieStoreOptions `cfg:",squash"` - Redis RedisStoreOptions `cfg:",squash"` + // Type is the type of session store to use + // Options are "cookie" or "redis" + // Default is "cookie" + Type SessionStoreType `yaml:"type,omitempty"` + // Refresh is the duration after which the session is refreshable + Refresh time.Duration `yaml:"refresh,omitempty"` + // Cookie is the configuration options for the CookieSessionStore + Cookie CookieStoreOptions `yaml:"cookie,omitempty"` + // Redis is the configuration options for the RedisSessionStore + Redis RedisStoreOptions `yaml:"redis,omitempty"` } -// CookieSessionStoreType is used to indicate the CookieSessionStore should be -// used for storing sessions. -var CookieSessionStoreType = "cookie" - -// RedisSessionStoreType is used to indicate the RedisSessionStore should be -// used for storing sessions. -var RedisSessionStoreType = "redis" - // CookieStoreOptions contains configuration options for the CookieSessionStore. type CookieStoreOptions struct { - Minimal bool `flag:"session-cookie-minimal" cfg:"session_cookie_minimal"` + // Minimal indicates whether to use minimal cookies for session storage + // Default is false + Minimal *bool `yaml:"minimal,omitempty"` } // RedisStoreOptions contains configuration options for the RedisSessionStore. type RedisStoreOptions struct { - ConnectionURL string `flag:"redis-connection-url" cfg:"redis_connection_url"` - Username string `flag:"redis-username" cfg:"redis_username"` - Password string `flag:"redis-password" cfg:"redis_password"` - UseSentinel bool `flag:"redis-use-sentinel" cfg:"redis_use_sentinel"` - SentinelPassword string `flag:"redis-sentinel-password" cfg:"redis_sentinel_password"` - SentinelMasterName string `flag:"redis-sentinel-master-name" cfg:"redis_sentinel_master_name"` - SentinelConnectionURLs []string `flag:"redis-sentinel-connection-urls" cfg:"redis_sentinel_connection_urls"` - UseCluster bool `flag:"redis-use-cluster" cfg:"redis_use_cluster"` - ClusterConnectionURLs []string `flag:"redis-cluster-connection-urls" cfg:"redis_cluster_connection_urls"` - CAPath string `flag:"redis-ca-path" cfg:"redis_ca_path"` - InsecureSkipTLSVerify bool `flag:"redis-insecure-skip-tls-verify" cfg:"redis_insecure_skip_tls_verify"` - IdleTimeout int `flag:"redis-connection-idle-timeout" cfg:"redis_connection_idle_timeout"` + // ConnectionURL is the Redis connection URL + ConnectionURL string `yaml:"connectionURL,omitempty"` + // Username is the Redis username + Username string `yaml:"username,omitempty"` + // Password is the Redis password + Password string `yaml:"password,omitempty"` + // UseSentinel indicates whether to use Redis Sentinel + // Default is false + UseSentinel *bool `yaml:"useSentinel,omitempty"` + // SentinelPassword is the Redis Sentinel password + SentinelPassword string `yaml:"sentinelPassword,omitempty"` + // SentinelMasterName is the Redis Sentinel master name + SentinelMasterName string `yaml:"sentinelMasterName,omitempty"` + // SentinelConnectionURLs is a list of Redis Sentinel connection URLs + SentinelConnectionURLs []string `yaml:"sentinelConnectionURLs,omitempty"` + // UseCluster indicates whether to use Redis Cluster + // Default is false + UseCluster *bool `yaml:"useCluster,omitempty"` + // ClusterConnectionURLs is a list of Redis Cluster connection URLs + ClusterConnectionURLs []string `yaml:"clusterConnectionURLs,omitempty"` + // CAPath is the path to the CA certificate for Redis TLS connections + CAPath string `yaml:"caPath,omitempty"` + // InsecureSkipTLSVerify indicates whether to skip TLS verification for Redis connections + InsecureSkipTLSVerify *bool `yaml:"insecureSkipTLSVerify,omitempty"` + // IdleTimeout is the Redis connection idle timeout in seconds + IdleTimeout int `yaml:"idleTimeout,omitempty"` } -func sessionOptionsDefaults() SessionOptions { - return SessionOptions{ - Type: CookieSessionStoreType, - Cookie: CookieStoreOptions{ - Minimal: false, - }, +// EnsureDefaults sets default values for SessionOptions +func (s *SessionOptions) EnsureDefaults() { + if s.Type == "" { + s.Type = CookieSessionStoreType + } + if s.Cookie.Minimal == nil { + s.Cookie.Minimal = ptr.To(DefaultCookieStoreMinimal) + } + if s.Redis.UseSentinel == nil { + s.Redis.UseSentinel = ptr.To(DefaultRedisStoreUseSentinel) + } + if s.Redis.UseCluster == nil { + s.Redis.UseCluster = ptr.To(DefaultRedisStoreUseCluster) + } + if s.Redis.InsecureSkipTLSVerify == nil { + s.Redis.InsecureSkipTLSVerify = ptr.To(DefaultRedisStoreInsecureSkipTLSVerify) } } diff --git a/pkg/cookies/cookies.go b/pkg/cookies/cookies.go index 236890f5..252daeb6 100644 --- a/pkg/cookies/cookies.go +++ b/pkg/cookies/cookies.go @@ -31,8 +31,8 @@ func MakeCookieFromOptions(req *http.Request, name string, value string, opts *o Value: value, Path: opts.Path, Domain: domain, - HttpOnly: ptr.Deref(opts.HTTPOnly, options.DefaultCookieHTTPOnly), - Secure: ptr.Deref(opts.Secure, options.DefaultCookieSecure), + HttpOnly: !ptr.Deref(opts.NotHttpOnly, options.DefaultCookieNotHttpOnly), + Secure: !ptr.Deref(opts.Insecure, options.DefaultCookieInsecure), SameSite: ParseSameSite(opts.SameSite), } @@ -60,7 +60,7 @@ func GetCookieDomain(req *http.Request, cookieDomains []string) string { } // Parse a valid http.SameSite value from a user supplied string for use of making cookies. -func ParseSameSite(v string) http.SameSite { +func ParseSameSite(v options.SameSiteMode) http.SameSite { switch v { case "lax": return http.SameSiteLaxMode diff --git a/pkg/cookies/cookies_suite_test.go b/pkg/cookies/cookies_suite_test.go index f4893cbd..0d152b4d 100644 --- a/pkg/cookies/cookies_suite_test.go +++ b/pkg/cookies/cookies_suite_test.go @@ -13,13 +13,16 @@ const ( csrfNonce = "0987lkjh0987lkjh0987lkjh" cookieName = "cookie_test_12345" - cookieSecret = "3q48hmFH30FJ2HfJF0239UFJCVcl3kj3" cookieDomain = "o2p.cookies.test" cookiePath = "/cookie-tests" nowEpoch = 1609366421 ) +var ( + cookieSecret = []byte("3q48hmFH30FJ2HfJF0239UFJCVcl3kj3") +) + func TestProviderSuite(t *testing.T) { logger.SetOutput(GinkgoWriter) diff --git a/pkg/cookies/cookies_test.go b/pkg/cookies/cookies_test.go index 485e79a7..13131c4b 100644 --- a/pkg/cookies/cookies_test.go +++ b/pkg/cookies/cookies_test.go @@ -92,7 +92,7 @@ var _ = Describe("Cookie Tests", func() { } validName := "_oauth2_proxy" - validSecret := "secretthirtytwobytes+abcdefghijk" + validSecret := []byte("secretthirtytwobytes+abcdefghijk") domains := []string{"www.cookies.test"} now := time.Now() @@ -114,15 +114,14 @@ var _ = Describe("Cookie Tests", func() { name: validName, value: "1", opts: options.Cookie{ - Name: validName, - Secret: validSecret, - Domains: domains, - Path: "", - Expire: time.Hour, - Refresh: 15 * time.Minute, - Secure: ptr.To(true), - HTTPOnly: ptr.To(false), - SameSite: "", + Name: validName, + Secret: options.SecretSource{Value: validSecret}, + Domains: domains, + Path: "", + Expire: time.Hour, + Insecure: ptr.To(false), + NotHttpOnly: ptr.To(true), + SameSite: "", }, expiration: 15 * time.Minute, now: now, @@ -133,15 +132,14 @@ var _ = Describe("Cookie Tests", func() { name: validName, value: "1", opts: options.Cookie{ - Name: validName, - Secret: validSecret, - Domains: domains, - Path: "", - Expire: time.Hour * -1, - Refresh: 15 * time.Minute, - Secure: ptr.To(true), - HTTPOnly: ptr.To(false), - SameSite: "", + Name: validName, + Secret: options.SecretSource{Value: validSecret}, + Domains: domains, + Path: "", + Expire: time.Hour * -1, + Insecure: ptr.To(false), + NotHttpOnly: ptr.To(true), + SameSite: "", }, expiration: time.Hour * -1, now: now, @@ -152,15 +150,14 @@ var _ = Describe("Cookie Tests", func() { name: validName, value: "1", opts: options.Cookie{ - Name: validName, - Secret: validSecret, - Domains: domains, - Path: "", - Expire: 0, - Refresh: 15 * time.Minute, - Secure: ptr.To(true), - HTTPOnly: ptr.To(false), - SameSite: "", + Name: validName, + Secret: options.SecretSource{Value: validSecret}, + Domains: domains, + Path: "", + Expire: 0, + Insecure: ptr.To(false), + NotHttpOnly: ptr.To(true), + SameSite: "", }, expiration: 0, now: now, diff --git a/pkg/cookies/csrf_per_request_test.go b/pkg/cookies/csrf_per_request_test.go index 048c5752..bb0fcaee 100644 --- a/pkg/cookies/csrf_per_request_test.go +++ b/pkg/cookies/csrf_per_request_test.go @@ -25,12 +25,12 @@ var _ = Describe("CSRF Cookie with non-fixed name Tests", func() { BeforeEach(func() { cookieOpts = &options.Cookie{ Name: cookieName, - Secret: cookieSecret, + Secret: options.SecretSource{Value: cookieSecret}, Domains: []string{cookieDomain}, Path: cookiePath, Expire: time.Hour, - Secure: ptr.To(true), - HTTPOnly: ptr.To(true), + Insecure: ptr.To(false), + NotHttpOnly: ptr.To(false), CSRFPerRequest: ptr.To(true), CSRFExpire: time.Duration(5) * time.Minute, } @@ -118,7 +118,10 @@ var _ = Describe("CSRF Cookie with non-fixed name Tests", func() { Value: encoded, } - _, _, valid := encryption.Validate(cookie, cookieOpts.Secret, cookieOpts.Expire) + cookieSecret, err := cookieOpts.GetSecret() + Expect(err).ToNot(HaveOccurred()) + + _, _, valid := encryption.Validate(cookie, cookieSecret, cookieOpts.Expire) Expect(valid).To(BeTrue()) }) }) diff --git a/pkg/cookies/csrf_test.go b/pkg/cookies/csrf_test.go index 7e50d0b1..dfb0bae7 100644 --- a/pkg/cookies/csrf_test.go +++ b/pkg/cookies/csrf_test.go @@ -26,12 +26,12 @@ var _ = Describe("CSRF Cookie Tests", func() { BeforeEach(func() { cookieOpts = &options.Cookie{ Name: cookieName, - Secret: cookieSecret, + Secret: options.SecretSource{Value: cookieSecret}, Domains: []string{cookieDomain}, Path: cookiePath, Expire: time.Hour, - Secure: ptr.To(true), - HTTPOnly: ptr.To(true), + Insecure: ptr.To(false), + NotHttpOnly: ptr.To(false), CSRFPerRequest: ptr.To(false), CSRFExpire: time.Hour, } @@ -119,8 +119,10 @@ var _ = Describe("CSRF Cookie Tests", func() { Name: privateCSRF.cookieName(), Value: encoded, } + cookieSecret, err := cookieOpts.GetSecret() + Expect(err).ToNot(HaveOccurred()) - _, _, valid := encryption.Validate(cookie, cookieOpts.Secret, cookieOpts.Expire) + _, _, valid := encryption.Validate(cookie, cookieSecret, cookieOpts.Expire) Expect(valid).To(BeTrue()) }) }) diff --git a/pkg/sessions/cookie/session_store.go b/pkg/sessions/cookie/session_store.go index 095bc0e7..ee23077d 100644 --- a/pkg/sessions/cookie/session_store.go +++ b/pkg/sessions/cookie/session_store.go @@ -13,6 +13,7 @@ import ( pkgcookies "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/cookies" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/encryption" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/util/ptr" ) const ( @@ -164,7 +165,7 @@ func NewCookieSessionStore(opts *options.SessionOptions, cookieOpts *options.Coo return &SessionStore{ CookieCipher: cipher, Cookie: cookieOpts, - Minimal: opts.Cookie.Minimal, + Minimal: ptr.Deref(opts.Cookie.Minimal, options.DefaultCookieStoreMinimal), }, nil } diff --git a/pkg/sessions/redis/redis_store.go b/pkg/sessions/redis/redis_store.go index 4e846e9b..bfacd03b 100644 --- a/pkg/sessions/redis/redis_store.go +++ b/pkg/sessions/redis/redis_store.go @@ -12,6 +12,7 @@ import ( "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/sessions/persistence" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/util/ptr" "github.com/redis/go-redis/v9" ) @@ -82,13 +83,14 @@ func (store *SessionStore) VerifyConnection(ctx context.Context) error { // NewRedisClient makes a redis.Client (either standalone, sentinel aware, or // redis cluster) func NewRedisClient(opts options.RedisStoreOptions) (Client, error) { - if opts.UseSentinel && opts.UseCluster { + if ptr.Deref(opts.UseSentinel, options.DefaultRedisStoreUseSentinel) && + ptr.Deref(opts.UseCluster, options.DefaultRedisStoreUseCluster) { return nil, fmt.Errorf("options redis-use-sentinel and redis-use-cluster are mutually exclusive") } - if opts.UseSentinel { + if ptr.Deref(opts.UseSentinel, options.DefaultRedisStoreUseSentinel) { return buildSentinelClient(opts) } - if opts.UseCluster { + if ptr.Deref(opts.UseCluster, options.DefaultRedisStoreUseCluster) { return buildClusterClient(opts) } @@ -181,7 +183,7 @@ func buildStandaloneClient(opts options.RedisStoreOptions) (Client, error) { // setupTLSConfig sets the TLSConfig if the TLS option is given in redis.Options func setupTLSConfig(opts options.RedisStoreOptions, opt *redis.Options) error { - if opts.InsecureSkipTLSVerify { + if ptr.Deref(opts.InsecureSkipTLSVerify, options.DefaultRedisStoreInsecureSkipTLSVerify) { if opt.TLSConfig == nil { /* #nosec */ opt.TLSConfig = &tls.Config{} diff --git a/pkg/sessions/redis/redis_store_test.go b/pkg/sessions/redis/redis_store_test.go index 1bff6855..325237b0 100644 --- a/pkg/sessions/redis/redis_store_test.go +++ b/pkg/sessions/redis/redis_store_test.go @@ -9,6 +9,7 @@ import ( sessionsapi "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/sessions/persistence" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/sessions/tests" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/util/ptr" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) @@ -80,7 +81,7 @@ var _ = Describe("Redis SessionStore Tests", func() { sentinelAddr := redisProtocol + ms.Addr() opts.Type = options.RedisSessionStoreType opts.Redis.SentinelConnectionURLs = []string{sentinelAddr} - opts.Redis.UseSentinel = true + opts.Redis.UseSentinel = ptr.To(true) opts.Redis.SentinelMasterName = ms.MasterInfo().Name // Capture the session store so that we can close the client @@ -101,7 +102,7 @@ var _ = Describe("Redis SessionStore Tests", func() { clusterAddr := redisProtocol + mr.Addr() opts.Type = options.RedisSessionStoreType opts.Redis.ClusterConnectionURLs = []string{clusterAddr} - opts.Redis.UseCluster = true + opts.Redis.UseCluster = ptr.To(true) // Capture the session store so that we can close the client var err error @@ -156,7 +157,7 @@ var _ = Describe("Redis SessionStore Tests", func() { sentinelAddr := redisProtocol + ms.Addr() opts.Type = options.RedisSessionStoreType opts.Redis.SentinelConnectionURLs = []string{sentinelAddr} - opts.Redis.UseSentinel = true + opts.Redis.UseSentinel = ptr.To(true) opts.Redis.SentinelMasterName = ms.MasterInfo().Name opts.Redis.Password = redisPassword @@ -178,7 +179,7 @@ var _ = Describe("Redis SessionStore Tests", func() { clusterAddr := redisProtocol + mr.Addr() opts.Type = options.RedisSessionStoreType opts.Redis.ClusterConnectionURLs = []string{clusterAddr} - opts.Redis.UseCluster = true + opts.Redis.UseCluster = ptr.To(true) opts.Redis.Password = redisPassword // Capture the session store so that we can close the client @@ -227,7 +228,7 @@ var _ = Describe("Redis SessionStore Tests", func() { clusterAddr := "redis://" + redisUsername + "@" + mr.Addr() opts.Type = options.RedisSessionStoreType opts.Redis.ClusterConnectionURLs = []string{clusterAddr} - opts.Redis.UseCluster = true + opts.Redis.UseCluster = ptr.To(true) opts.Redis.Username = redisUsername opts.Redis.Password = redisPassword diff --git a/pkg/sessions/redis/redis_store_tls_test.go b/pkg/sessions/redis/redis_store_tls_test.go index 54d94b1f..5bb62909 100644 --- a/pkg/sessions/redis/redis_store_tls_test.go +++ b/pkg/sessions/redis/redis_store_tls_test.go @@ -9,6 +9,7 @@ import ( sessionsapi "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/sessions/persistence" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/sessions/tests" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/util/ptr" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) @@ -69,7 +70,7 @@ var _ = Describe("Redis SessionStore Tests", func() { clusterAddr := redissProtocol + mr.Addr() opts.Type = options.RedisSessionStoreType opts.Redis.ClusterConnectionURLs = []string{clusterAddr} - opts.Redis.UseCluster = true + opts.Redis.UseCluster = ptr.To(true) opts.Redis.CAPath = caPath // Capture the session store so that we can close the client @@ -92,7 +93,7 @@ var _ = Describe("Redis SessionStore Tests", func() { // Set the connection URL opts.Type = options.RedisSessionStoreType opts.Redis.ConnectionURL = redissProtocol + mr.Addr() - opts.Redis.InsecureSkipTLSVerify = true + opts.Redis.InsecureSkipTLSVerify = ptr.To(true) // Capture the session store so that we can close the client ss, err := NewRedisSessionStore(opts, cookieOpts) @@ -111,8 +112,8 @@ var _ = Describe("Redis SessionStore Tests", func() { clusterAddr := redissProtocol + mr.Addr() opts.Type = options.RedisSessionStoreType opts.Redis.ClusterConnectionURLs = []string{clusterAddr} - opts.Redis.UseCluster = true - opts.Redis.InsecureSkipTLSVerify = true + opts.Redis.UseCluster = ptr.To(true) + opts.Redis.InsecureSkipTLSVerify = ptr.To(true) // Capture the session store so that we can close the client var err error @@ -153,7 +154,7 @@ var _ = Describe("Redis SessionStore Tests", func() { // Set the connection URL opts.Type = options.RedisSessionStoreType opts.Redis.ConnectionURL = "redis://127.0.0.1:" + mr.Port() // func (*Miniredis) StartTLS listens on 127.0.0.1 - opts.Redis.InsecureSkipTLSVerify = true + opts.Redis.InsecureSkipTLSVerify = ptr.To(true) // Capture the session store so that we can close the client var err error diff --git a/pkg/sessions/tests/session_store_tests.go b/pkg/sessions/tests/session_store_tests.go index 4ab7315e..80f59bb1 100644 --- a/pkg/sessions/tests/session_store_tests.go +++ b/pkg/sessions/tests/session_store_tests.go @@ -24,6 +24,7 @@ import ( // Interfaces have to be wrapped in closures otherwise nil pointers are thrown. type testInput struct { cookieOpts *options.Cookie + sessionOpts *options.SessionOptions ss sessionStoreFunc session *sessionsapi.SessionState request *http.Request @@ -44,7 +45,6 @@ type NewSessionStoreFunc func(sessionOpts *options.SessionOptions, cookieOpts *o func RunSessionStoreTests(newSS NewSessionStoreFunc, persistentFastForward PersistentStoreFastForwardFunc) { Describe("Session Store Suite", func() { - var opts *options.SessionOptions var ss sessionsapi.SessionStore var input testInput var cookieSecret []byte @@ -55,7 +55,9 @@ func RunSessionStoreTests(newSS NewSessionStoreFunc, persistentFastForward Persi BeforeEach(func() { ss = nil - opts = &options.SessionOptions{} + sessionOpts := &options.SessionOptions{ + Refresh: time.Duration(1) * time.Hour, + } // A secret is required to create a Cipher, validation ensures it is the correct // length before a session store is initialised. @@ -65,14 +67,13 @@ func RunSessionStoreTests(newSS NewSessionStoreFunc, persistentFastForward Persi // Set default options in CookieOptions cookieOpts := &options.Cookie{ - Name: "_oauth2_proxy", - Path: "/", - Expire: time.Duration(168) * time.Hour, - Refresh: time.Duration(1) * time.Hour, - Secure: ptr.To(true), - HTTPOnly: ptr.To(true), - SameSite: "", - Secret: string(cookieSecret), + Name: "_oauth2_proxy", + Path: "/", + Expire: time.Duration(168) * time.Hour, + Insecure: ptr.To(false), + NotHttpOnly: ptr.To(false), + SameSite: options.SameSiteDefault, + Secret: options.SecretSource{Value: cookieSecret}, } expires := time.Now().Add(1 * time.Hour) @@ -90,6 +91,7 @@ func RunSessionStoreTests(newSS NewSessionStoreFunc, persistentFastForward Persi input = testInput{ cookieOpts: cookieOpts, + sessionOpts: sessionOpts, ss: getSessionStore, session: session, request: request, @@ -101,7 +103,7 @@ func RunSessionStoreTests(newSS NewSessionStoreFunc, persistentFastForward Persi Context("with default options", func() { BeforeEach(func() { var err error - ss, err = newSS(opts, input.cookieOpts) + ss, err = newSS(input.sessionOpts, input.cookieOpts) Expect(err).ToNot(HaveOccurred()) }) @@ -113,20 +115,20 @@ func RunSessionStoreTests(newSS NewSessionStoreFunc, persistentFastForward Persi Context("with non-default options", func() { BeforeEach(func() { + input.sessionOpts.Refresh = time.Duration(2) * time.Hour input.cookieOpts = &options.Cookie{ - Name: "_cookie_name", - Path: "/path", - Expire: time.Duration(72) * time.Hour, - Refresh: time.Duration(2) * time.Hour, - Secure: ptr.To(false), - HTTPOnly: ptr.To(false), - Domains: []string{"example.com"}, - SameSite: "strict", - Secret: string(cookieSecret), + Name: "_cookie_name", + Path: "/path", + Expire: time.Duration(72) * time.Hour, + Insecure: ptr.To(true), + NotHttpOnly: ptr.To(true), + Domains: []string{"example.com"}, + SameSite: options.SameSiteStrict, + Secret: options.SecretSource{Value: cookieSecret}, } var err error - ss, err = newSS(opts, input.cookieOpts) + ss, err = newSS(input.sessionOpts, input.cookieOpts) Expect(err).ToNot(HaveOccurred()) }) @@ -145,18 +147,17 @@ func RunSessionStoreTests(newSS NewSessionStoreFunc, persistentFastForward Persi tmpfile.Write(secretBytes) tmpfile.Close() + input.sessionOpts.Refresh = time.Duration(1) * time.Hour input.cookieOpts = &options.Cookie{ - Name: "_oauth2_proxy_file", - Path: "/", - Expire: time.Duration(168) * time.Hour, - Refresh: time.Duration(1) * time.Hour, - Secure: ptr.To(true), - HTTPOnly: ptr.To(true), - SameSite: "", - Secret: "", - SecretFile: tmpfile.Name(), + Name: "_oauth2_proxy_file", + Path: "/", + Expire: time.Duration(168) * time.Hour, + Insecure: ptr.To(false), + NotHttpOnly: ptr.To(false), + SameSite: options.SameSiteDefault, + Secret: options.SecretSource{FromFile: tmpfile.Name()}, } - ss, err = newSS(opts, input.cookieOpts) + ss, err = newSS(input.sessionOpts, input.cookieOpts) Expect(err).ToNot(HaveOccurred()) }) @@ -209,13 +210,13 @@ func CheckCookieOptions(in *testInput) { It("have the correct HTTPOnly set", func() { for _, cookie := range cookies { - Expect(cookie.HttpOnly).To(Equal(*in.cookieOpts.HTTPOnly)) + Expect(cookie.HttpOnly).To(Equal(!(*in.cookieOpts.NotHttpOnly))) } }) It("have the correct secure set", func() { for _, cookie := range cookies { - Expect(cookie.Secure).To(Equal(*in.cookieOpts.Secure)) + Expect(cookie.Secure).To(Equal(!(*in.cookieOpts.Insecure))) } }) @@ -298,7 +299,7 @@ func PersistentSessionStoreInterfaceTests(in *testInput) { Context("after the refresh period, but before the cookie expire period", func() { BeforeEach(func() { - Expect(in.persistentFastForward(in.cookieOpts.Refresh + time.Minute)).To(Succeed()) + Expect(in.persistentFastForward(in.sessionOpts.Refresh + time.Minute)).To(Succeed()) }) LoadSessionTests(in) @@ -421,8 +422,13 @@ func SessionStoreInterfaceTests(in *testInput) { BeforeEach(func() { By("Using a valid cookie with a different providers session encoding") broken := "BrokenSessionFromADifferentSessionImplementation" - value, err := encryption.SignedValue(in.cookieOpts.Secret, in.cookieOpts.Name, []byte(broken), time.Now()) + + cookieSecret, err := in.cookieOpts.GetSecret() Expect(err).ToNot(HaveOccurred()) + + value, err := encryption.SignedValue(cookieSecret, in.cookieOpts.Name, []byte(broken), time.Now()) + Expect(err).ToNot(HaveOccurred()) + cookie := cookiesapi.MakeCookieFromOptions(in.request, in.cookieOpts.Name, value, in.cookieOpts, in.cookieOpts.Expire) in.request.AddCookie(cookie) diff --git a/pkg/validation/cookie.go b/pkg/validation/cookie.go index 5f2dd8ac..76d8dd12 100644 --- a/pkg/validation/cookie.go +++ b/pkg/validation/cookie.go @@ -3,7 +3,6 @@ package validation import ( "fmt" "net/http" - "os" "sort" "time" @@ -11,13 +10,13 @@ import ( "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/encryption" ) -func validateCookie(o options.Cookie) []string { - msgs := validateCookieSecret(o.Secret, o.SecretFile) +func validateCookie(o options.Cookie, refresh time.Duration) []string { + msgs := validateCookieSecret(o.Secret) - if o.Expire != time.Duration(0) && o.Refresh >= o.Expire { + if o.Expire != time.Duration(0) && refresh >= o.Expire { msgs = append(msgs, fmt.Sprintf( "cookie_refresh (%q) must be less than cookie_expire (%q)", - o.Refresh.String(), + refresh.String(), o.Expire.String())) } @@ -50,30 +49,17 @@ func validateCookieName(name string) []string { return msgs } -func validateCookieSecret(secret string, secretFile string) []string { - if secret == "" && secretFile == "" { +func validateCookieSecret(secret options.SecretSource) []string { + if len(secret.Value) == 0 && secret.FromFile == "" { return []string{"missing setting: cookie-secret or cookie-secret-file"} } - if secret == "" && secretFile != "" { - fileData, err := os.ReadFile(secretFile) - if err != nil { - return []string{"could not read cookie secret file: " + secretFile} - } - // Validate the file content as a secret - secretBytes := encryption.SecretBytes(string(fileData)) - switch len(secretBytes) { - case 16, 24, 32: - // Valid secret size found - return []string{} - } - // Invalid secret size found, return a message - return []string{fmt.Sprintf( - "cookie_secret from file must be 16, 24, or 32 bytes to create an AES cipher, but is %d bytes", - len(secretBytes)), - } + + value, err := secret.GetSecretValue() + if err != nil { + return []string{fmt.Sprintf("error retrieving cookie secret: %v", err)} } - secretBytes := encryption.SecretBytes(secret) + secretBytes := encryption.SecretBytes(string(value)) // Check if the secret is a valid length switch len(secretBytes) { case 16, 24, 32: diff --git a/pkg/validation/options.go b/pkg/validation/options.go index ec0e05f0..fac2d532 100644 --- a/pkg/validation/options.go +++ b/pkg/validation/options.go @@ -21,7 +21,7 @@ import ( // Validate checks that required options are set and validates those that they // are of the correct format func Validate(o *options.Options) error { - msgs := validateCookie(o.Cookie) + msgs := validateCookie(o.Cookie, o.Session.Refresh) msgs = append(msgs, validateSessionCookieMinimal(o)...) msgs = append(msgs, validateRedisSessionStore(o)...) msgs = append(msgs, prefixValues("injectRequestHeaders: ", validateHeaders(o.InjectRequestHeaders)...)...) @@ -74,7 +74,7 @@ func Validate(o *options.Options) error { var redirectURL *url.URL redirectURL, msgs = parseURL(o.RawRedirectURL, "redirect", msgs) o.SetRedirectURL(redirectURL) - if o.RawRedirectURL == "" && !ptr.Deref(o.Cookie.Secure, options.DefaultCookieSecure) && !o.ReverseProxy { + if o.RawRedirectURL == "" && ptr.Deref(o.Cookie.Insecure, options.DefaultCookieInsecure) && !o.ReverseProxy { logger.Print("WARNING: no explicit redirect URL: redirects will default to insecure HTTP") } diff --git a/pkg/validation/options_test.go b/pkg/validation/options_test.go index cb13ee82..526eca78 100644 --- a/pkg/validation/options_test.go +++ b/pkg/validation/options_test.go @@ -126,10 +126,10 @@ func TestCookieRefreshMustBeLessThanCookieExpire(t *testing.T) { assert.Equal(t, nil, Validate(o)) o.Cookie.Secret = "0123456789abcdef" - o.Cookie.Refresh = o.Cookie.Expire + o.Session.Refresh = o.Cookie.Expire assert.NotEqual(t, nil, Validate(o)) - o.Cookie.Refresh -= time.Duration(1) + o.Session.Refresh -= time.Duration(1) assert.Equal(t, nil, Validate(o)) } diff --git a/pkg/validation/sessions.go b/pkg/validation/sessions.go index 96ea6d4f..4a1c825d 100644 --- a/pkg/validation/sessions.go +++ b/pkg/validation/sessions.go @@ -9,10 +9,11 @@ import ( "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/encryption" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/sessions/redis" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/util/ptr" ) func validateSessionCookieMinimal(o *options.Options) []string { - if !o.Session.Cookie.Minimal { + if !ptr.Deref(o.Session.Cookie.Minimal, options.DefaultCookieStoreMinimal) { return []string{} } @@ -32,7 +33,7 @@ func validateSessionCookieMinimal(o *options.Options) []string { } } - if o.Cookie.Refresh != time.Duration(0) { + if o.Session.Refresh != time.Duration(0) { msgs = append(msgs, "cookie_refresh > 0 requires oauth tokens in sessions. session_cookie_minimal cannot be set") }