diff --git a/oauthproxy.go b/oauthproxy.go index 803d4b30..6028195d 100644 --- a/oauthproxy.go +++ b/oauthproxy.go @@ -178,11 +178,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 { @@ -419,7 +419,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, })) @@ -1114,9 +1114,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 cb923cee..4ee68c93 100644 --- a/oauthproxy_test.go +++ b/oauthproxy_test.go @@ -820,9 +820,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 @@ -846,9 +846,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 @@ -970,7 +970,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 83f1bc85..a6a4061f 100644 --- a/pkg/apis/options/cookie.go +++ b/pkg/apis/options/cookie.go @@ -2,49 +2,56 @@ 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"` - // SameSite sets the SameSite attribute on the session cookies - SameSite string `yaml:"sameSite,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 SameSiteMode `yaml:"sameSite,omitempty"` // CSRFSameSite sets the SameSite attribute on the csrf cookies - CSRFSameSite string `yaml:"sameSite,omitempty"` + CSRFSameSite 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 @@ -53,18 +60,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 @@ -78,11 +95,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..c236ed36 100644 --- a/pkg/apis/options/legacy_cookie.go +++ b/pkg/apis/options/legacy_cookie.go @@ -39,21 +39,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 + + var secret *SecretSource + if l.Secret != "" { + secret = NewSecretSourceFromString(l.Secret) + } else if l.SecretFile != "" { + secret = &SecretSource{ + FromFile: l.SecretFile, + } + } + return Cookie{ Name: l.Name, - Secret: l.Secret, - SecretFile: l.SecretFile, + Secret: *secret, 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_header.go b/pkg/apis/options/legacy_header.go index 58c337a8..aeec99e4 100644 --- a/pkg/apis/options/legacy_header.go +++ b/pkg/apis/options/legacy_header.go @@ -106,11 +106,9 @@ func getBasicAuthHeader(preferEmailToUser bool, basicAuthPassword string) Header Values: []HeaderValue{ { ClaimSource: &ClaimSource{ - Claim: claim, - Prefix: "Basic ", - BasicAuthPassword: &SecretSource{ - Value: []byte(basicAuthPassword), - }, + Claim: claim, + Prefix: "Basic ", + BasicAuthPassword: NewSecretSourceFromString(basicAuthPassword), }, }, }, diff --git a/pkg/apis/options/legacy_options.go b/pkg/apis/options/legacy_options.go index 53a3087e..b8888232 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"` } @@ -75,6 +78,13 @@ func NewLegacyOptions() *LegacyOptions { CSRFExpire: time.Duration(15) * time.Minute, }, + LegacySessionOptions: LegacySessionOptions{ + Type: "cookie", + Cookie: LegacyCookieStoreOptions{ + Minimal: false, + }, + }, + Options: *NewOptions(), } } @@ -88,6 +98,7 @@ func NewLegacyFlagSet() *pflag.FlagSet { flagSet.AddFlagSet(legacyProviderFlagSet()) flagSet.AddFlagSet(legacyGoogleFlagSet()) flagSet.AddFlagSet(legacyCookieFlagSet()) + flagSet.AddFlagSet(legacySessionFlagSet()) return flagSet } @@ -110,6 +121,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 438e8f53..e4833217 100644 --- a/pkg/apis/options/legacy_options_test.go +++ b/pkg/apis/options/legacy_options_test.go @@ -1096,13 +1096,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..d73ac41d 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,43 @@ type SecretSource struct { FromFile string `yaml:"fromFile,omitempty"` } +func NewSecretSourceFromValue(value []byte) *SecretSource { + encoded := make([]byte, base64.RawStdEncoding.EncodedLen(len(value))) + base64.RawStdEncoding.Encode(encoded, value) + return &SecretSource{ + Value: encoded, + } +} + +func NewSecretSourceFromString(s string) *SecretSource { + return NewSecretSourceFromValue([]byte(s)) +} + +func (ss *SecretSource) GetSecretValue() ([]byte, error) { + if len(ss.Value) > 0 { + var decoded []byte + if _, err := base64.RawStdEncoding.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 6424864e..3a9f92cd 100644 --- a/pkg/cookies/cookies.go +++ b/pkg/cookies/cookies.go @@ -31,8 +31,8 @@ func MakeCookieFromOptions(req *http.Request, value string, opts *options.Cookie 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), } @@ -59,8 +59,8 @@ func GetCookieDomain(req *http.Request, cookieDomains []string) string { return "" } -// ParseSameSite a valid http.SameSite value from a user supplied string for use of making cookies. -func ParseSameSite(v string) http.SameSite { +// Parse a valid http.SameSite value from a user supplied string for use of making cookies. +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 a11dd798..87ef36c3 100644 --- a/pkg/cookies/cookies_suite_test.go +++ b/pkg/cookies/cookies_suite_test.go @@ -13,7 +13,6 @@ const ( csrfNonce = "0987lkjh0987lkjh0987lkjh" cookieName = "cookie_test_12345" - cookieSecret = "3q48hmFH30FJ2HfJF0239UFJCVcl3kj3" cookieDomain = "o2p.cookies.test" cookiePath = "/cookie-tests" @@ -24,6 +23,10 @@ const ( 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 f631956b..6007f0da 100644 --- a/pkg/cookies/cookies_test.go +++ b/pkg/cookies/cookies_test.go @@ -90,7 +90,7 @@ var _ = Describe("Cookie Tests", func() { } validName := "_oauth2_proxy" - validSecret := "secretthirtytwobytes+abcdefghijk" + validSecret := []byte("secretthirtytwobytes+abcdefghijk") domains := []string{"www.cookies.test"} now := time.Now() @@ -111,15 +111,14 @@ var _ = Describe("Cookie Tests", func() { host: "www.cookies.test", 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: "", }, now: now, expectedOutput: int((15 * time.Minute).Seconds()), @@ -128,15 +127,14 @@ var _ = Describe("Cookie Tests", func() { host: "www.cookies.test", 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: "", }, now: now, expectedOutput: -1, @@ -145,15 +143,14 @@ var _ = Describe("Cookie Tests", func() { host: "www.cookies.test", 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: "", }, now: now, expectedOutput: expectedMaxAge, diff --git a/pkg/cookies/csrf_per_request_test.go b/pkg/cookies/csrf_per_request_test.go index b13e376d..bc24a7d7 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 c16fc7cc..a2e97de0 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.CSRFExpire) + _, _, valid := encryption.Validate(cookie, cookieSecret, cookieOpts.CSRFExpire) Expect(valid).To(BeTrue()) }) @@ -304,12 +306,12 @@ var _ = Describe("CSRF Cookie Tests", 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, } diff --git a/pkg/middleware/headers_test.go b/pkg/middleware/headers_test.go index c3c402f0..e8bbe665 100644 --- a/pkg/middleware/headers_test.go +++ b/pkg/middleware/headers_test.go @@ -189,7 +189,7 @@ var _ = Describe("Headers Suite", func() { ClaimSource: &options.ClaimSource{ Claim: "user", BasicAuthPassword: &options.SecretSource{ - Value: []byte(base64.StdEncoding.EncodeToString([]byte("basic-password"))), + Value: []byte(base64.RawStdEncoding.EncodeToString([]byte("basic-password"))), FromEnv: "SECRET_ENV", }, }, diff --git a/pkg/sessions/cookie/session_store.go b/pkg/sessions/cookie/session_store.go index 35a5fa75..5bcda47c 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 ( @@ -158,7 +159,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 79f8f7d1..b131d276 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) } @@ -188,7 +190,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 18dbe934..eda3f698 100644 --- a/pkg/sessions/redis/redis_store_test.go +++ b/pkg/sessions/redis/redis_store_test.go @@ -10,6 +10,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" ) @@ -81,7 +82,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 @@ -102,7 +103,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 @@ -157,7 +158,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 @@ -179,7 +180,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 @@ -228,7 +229,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/session_store_test.go b/pkg/sessions/session_store_test.go index 3b811903..2e982352 100644 --- a/pkg/sessions/session_store_test.go +++ b/pkg/sessions/session_store_test.go @@ -30,24 +30,31 @@ var _ = Describe("NewSessionStore", func() { var cookieOpts *options.Cookie BeforeEach(func() { - opts = &options.SessionOptions{} + opts = &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. secret := make([]byte, 32) _, err := rand.Read(secret) Expect(err).ToNot(HaveOccurred()) + var secretValue []byte + + base64.URLEncoding.Encode(secretValue, secret) + Expect(secretValue).ToNot(BeEmpty()) // Set default options in CookieOptions cookieOpts = &options.Cookie{ - Name: "_oauth2_proxy", - Secret: base64.URLEncoding.EncodeToString(secret), - Path: "/", - Expire: time.Duration(168) * time.Hour, - Refresh: time.Duration(1) * time.Hour, - Secure: ptr.To(true), - HTTPOnly: ptr.To(true), - SameSite: "", + Name: "_oauth2_proxy", + Secret: options.SecretSource{ + Value: secretValue, + }, + Path: "/", + Expire: time.Duration(168) * time.Hour, + Insecure: ptr.To(false), + NotHttpOnly: ptr.To(false), + SameSite: "", } }) diff --git a/pkg/sessions/tests/session_store_tests.go b/pkg/sessions/tests/session_store_tests.go index 2f38b9eb..0f2b5353 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, value, in.cookieOpts) 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/cookie_test.go b/pkg/validation/cookie_test.go index 95e3203e..b919ff2b 100644 --- a/pkg/validation/cookie_test.go +++ b/pkg/validation/cookie_test.go @@ -18,10 +18,22 @@ func TestValidateCookie(t *testing.T) { invalidName := "_oauth2;proxy" // Separater character not allowed // 10 times the alphabet should be longer than 256 characters longName := strings.Repeat(alphabet, 10) - validSecret := "secretthirtytwobytes+abcdefghijk" - invalidSecret := "abcdef" // 6 bytes is not a valid size - validBase64Secret := "c2VjcmV0dGhpcnR5dHdvYnl0ZXMrYWJjZGVmZ2hpams" // Base64 encoding of "secretthirtytwobytes+abcdefghijk" - invalidBase64Secret := "YWJjZGVmCg" // Base64 encoding of "abcdef" + validSecret := options.SecretSource{ + Value: []byte("secretthirtytwobytes+abcdefghijk"), + } + // 6 bytes is not a valid size + invalidSecret := options.SecretSource{ + Value: []byte("abcdef"), + } + + // Base64 encoding of "secretthirtytwobytes+abcdefghijk" + validBase64Secret := options.SecretSource{ + Value: []byte("c2VjcmV0dGhpcnR5dHdvYnl0ZXMrYWJjZGVmZ2hpams"), + } + // Base64 encoding of "abcdef" + invalidBase64Secret := options.SecretSource{ + Value: []byte("YWJjZGVmCg"), + } emptyDomains := []string{} domains := []string{ "a.localhost", @@ -39,7 +51,7 @@ func TestValidateCookie(t *testing.T) { defer os.Remove(tmpfile.Name()) // Write a valid 32-byte secret to the file - _, err = tmpfile.Write([]byte(validSecret)) + _, err = tmpfile.Write(validSecret.Value) if err != nil { t.Fatalf("Failed to write to temporary file: %v", err) } @@ -56,36 +68,40 @@ func TestValidateCookie(t *testing.T) { testCases := []struct { name string cookie options.Cookie + refresh time.Duration errStrings []string }{ { name: "with valid configuration", cookie: 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: validSecret, + Domains: domains, + Path: "", + Expire: time.Hour, + Insecure: ptr.To(false), + NotHttpOnly: ptr.To(true), + SameSite: "", }, + refresh: 15 * time.Minute, errStrings: []string{}, }, { name: "with no cookie secret", cookie: options.Cookie{ - Name: validName, - Secret: "", - Domains: emptyDomains, - Path: "", - Expire: time.Hour, - Refresh: 15 * time.Minute, - Secure: ptr.To(true), - HTTPOnly: ptr.To(false), - SameSite: "", + Name: validName, + Secret: options.SecretSource{ + Value: nil, + FromFile: "", + }, + Domains: emptyDomains, + Path: "", + Expire: time.Hour, + Insecure: ptr.To(false), + NotHttpOnly: ptr.To(true), + SameSite: "", }, + refresh: 15 * time.Minute, errStrings: []string{ missingSecretMsg, }, @@ -93,16 +109,16 @@ func TestValidateCookie(t *testing.T) { { name: "with an invalid cookie secret", cookie: options.Cookie{ - Name: validName, - Secret: invalidSecret, - Domains: emptyDomains, - Path: "", - Expire: time.Hour, - Refresh: 15 * time.Minute, - Secure: ptr.To(true), - HTTPOnly: ptr.To(false), - SameSite: "", + Name: validName, + Secret: invalidSecret, + Domains: emptyDomains, + Path: "", + Expire: time.Hour, + Insecure: ptr.To(false), + NotHttpOnly: ptr.To(true), + SameSite: "", }, + refresh: 15 * time.Minute, errStrings: []string{ invalidSecretMsg, }, @@ -110,31 +126,31 @@ func TestValidateCookie(t *testing.T) { { name: "with a valid Base64 secret", cookie: options.Cookie{ - Name: validName, - Secret: validBase64Secret, - Domains: emptyDomains, - Path: "", - Expire: time.Hour, - Refresh: 15 * time.Minute, - Secure: ptr.To(true), - HTTPOnly: ptr.To(false), - SameSite: "", + Name: validName, + Secret: validBase64Secret, + Domains: emptyDomains, + Path: "", + Expire: time.Hour, + Insecure: ptr.To(false), + NotHttpOnly: ptr.To(true), + SameSite: "", }, + refresh: 15 * time.Minute, errStrings: []string{}, }, { name: "with an invalid Base64 secret", cookie: options.Cookie{ - Name: validName, - Secret: invalidBase64Secret, - Domains: emptyDomains, - Path: "", - Expire: time.Hour, - Refresh: 15 * time.Minute, - Secure: ptr.To(true), - HTTPOnly: ptr.To(false), - SameSite: "", + Name: validName, + Secret: invalidBase64Secret, + Domains: emptyDomains, + Path: "", + Expire: time.Hour, + Insecure: ptr.To(false), + NotHttpOnly: ptr.To(true), + SameSite: "", }, + refresh: 15 * time.Minute, errStrings: []string{ invalidBase64SecretMsg, }, @@ -142,16 +158,16 @@ func TestValidateCookie(t *testing.T) { { name: "with an invalid name", cookie: options.Cookie{ - Name: invalidName, - Secret: validSecret, - Domains: emptyDomains, - Path: "", - Expire: time.Hour, - Refresh: 15 * time.Minute, - Secure: ptr.To(true), - HTTPOnly: ptr.To(false), - SameSite: "", + Name: invalidName, + Secret: validSecret, + Domains: emptyDomains, + Path: "", + Expire: time.Hour, + Insecure: ptr.To(false), + NotHttpOnly: ptr.To(true), + SameSite: "", }, + refresh: 15 * time.Minute, errStrings: []string{ invalidNameMsg, }, @@ -159,16 +175,16 @@ func TestValidateCookie(t *testing.T) { { name: "with a name that is too long", cookie: options.Cookie{ - Name: longName, - Secret: validSecret, - Domains: emptyDomains, - Path: "", - Expire: time.Hour, - Refresh: 15 * time.Minute, - Secure: ptr.To(true), - HTTPOnly: ptr.To(false), - SameSite: "", + Name: longName, + Secret: validSecret, + Domains: emptyDomains, + Path: "", + Expire: time.Hour, + Insecure: ptr.To(false), + NotHttpOnly: ptr.To(true), + SameSite: "", }, + refresh: 15 * time.Minute, errStrings: []string{ longNameMsg, }, @@ -176,16 +192,16 @@ func TestValidateCookie(t *testing.T) { { name: "with refresh longer than expire", cookie: options.Cookie{ - Name: validName, - Secret: validSecret, - Domains: emptyDomains, - Path: "", - Expire: 15 * time.Minute, - Refresh: time.Hour, - Secure: ptr.To(true), - HTTPOnly: ptr.To(false), - SameSite: "", + Name: validName, + Secret: validSecret, + Domains: emptyDomains, + Path: "", + Expire: 15 * time.Minute, + Insecure: ptr.To(false), + NotHttpOnly: ptr.To(true), + SameSite: "", }, + refresh: time.Hour, errStrings: []string{ refreshLongerThanExpireMsg, }, @@ -193,61 +209,61 @@ func TestValidateCookie(t *testing.T) { { name: "with samesite \"none\"", cookie: options.Cookie{ - Name: validName, - Secret: validSecret, - Domains: emptyDomains, - Path: "", - Expire: time.Hour, - Refresh: 15 * time.Minute, - Secure: ptr.To(true), - HTTPOnly: ptr.To(false), - SameSite: "none", + Name: validName, + Secret: validSecret, + Domains: emptyDomains, + Path: "", + Expire: time.Hour, + Insecure: ptr.To(false), + NotHttpOnly: ptr.To(true), + SameSite: "none", }, + refresh: 15 * time.Minute, errStrings: []string{}, }, { name: "with samesite \"lax\"", cookie: options.Cookie{ - Name: validName, - Secret: validSecret, - Domains: emptyDomains, - Path: "", - Expire: time.Hour, - Refresh: 15 * time.Minute, - Secure: ptr.To(true), - HTTPOnly: ptr.To(false), - SameSite: "none", + Name: validName, + Secret: validSecret, + Domains: emptyDomains, + Path: "", + Expire: time.Hour, + Insecure: ptr.To(false), + NotHttpOnly: ptr.To(true), + SameSite: "none", }, + refresh: 15 * time.Minute, errStrings: []string{}, }, { name: "with samesite \"strict\"", cookie: options.Cookie{ - Name: validName, - Secret: validSecret, - Domains: emptyDomains, - Path: "", - Expire: time.Hour, - Refresh: 15 * time.Minute, - Secure: ptr.To(true), - HTTPOnly: ptr.To(false), - SameSite: "none", + Name: validName, + Secret: validSecret, + Domains: emptyDomains, + Path: "", + Expire: time.Hour, + Insecure: ptr.To(false), + NotHttpOnly: ptr.To(true), + SameSite: "none", }, + refresh: 15 * time.Minute, errStrings: []string{}, }, { name: "with samesite \"invalid\"", cookie: options.Cookie{ - Name: validName, - Secret: validSecret, - Domains: emptyDomains, - Path: "", - Expire: time.Hour, - Refresh: 15 * time.Minute, - Secure: ptr.To(true), - HTTPOnly: ptr.To(false), - SameSite: "invalid", + Name: validName, + Secret: validSecret, + Domains: emptyDomains, + Path: "", + Expire: time.Hour, + Insecure: ptr.To(false), + NotHttpOnly: ptr.To(true), + SameSite: "invalid", }, + refresh: 15 * time.Minute, errStrings: []string{ invalidSameSiteMsg, }, @@ -255,16 +271,16 @@ func TestValidateCookie(t *testing.T) { { name: "with a combination of configuration errors", cookie: options.Cookie{ - Name: invalidName, - Secret: invalidSecret, - Domains: domains, - Path: "", - Expire: 15 * time.Minute, - Refresh: time.Hour, - Secure: ptr.To(true), - HTTPOnly: ptr.To(false), - SameSite: "invalid", + Name: invalidName, + Secret: invalidSecret, + Domains: domains, + Path: "", + Expire: 15 * time.Minute, + Insecure: ptr.To(false), + NotHttpOnly: ptr.To(true), + SameSite: "invalid", }, + refresh: time.Hour, errStrings: []string{ invalidNameMsg, invalidSecretMsg, @@ -275,55 +291,57 @@ func TestValidateCookie(t *testing.T) { { name: "with session cookie configuration", cookie: 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: validSecret, + Domains: domains, + Path: "", + Expire: 0, + Insecure: ptr.To(false), + NotHttpOnly: ptr.To(true), + SameSite: "", }, + refresh: 15 * time.Minute, errStrings: []string{}, }, { name: "with valid secret file", cookie: options.Cookie{ - Name: validName, - Secret: "", - SecretFile: tmpfile.Name(), - Domains: domains, - Path: "", - Expire: 24 * time.Hour, - Refresh: 0, - Secure: ptr.To(true), - HTTPOnly: ptr.To(true), - SameSite: "", + Name: validName, + Secret: options.SecretSource{ + FromFile: tmpfile.Name(), + }, + Domains: domains, + Path: "", + Expire: 24 * time.Hour, + Insecure: ptr.To(false), + NotHttpOnly: ptr.To(false), + SameSite: "", }, + refresh: 0, errStrings: []string{}, }, { name: "with nonexistent secret file", cookie: options.Cookie{ - Name: validName, - Secret: "", - SecretFile: "/nonexistent/file.txt", - Domains: domains, - Path: "", - Expire: 24 * time.Hour, - Refresh: 0, - Secure: ptr.To(true), - HTTPOnly: ptr.To(true), - SameSite: "", + Name: validName, + Secret: options.SecretSource{ + FromFile: "/nonexistent/file.txt", + }, + Domains: domains, + Path: "", + Expire: 24 * time.Hour, + Insecure: ptr.To(false), + NotHttpOnly: ptr.To(false), + SameSite: "", }, + refresh: 0, errStrings: []string{"could not read cookie secret file: /nonexistent/file.txt"}, }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - errStrings := validateCookie(tc.cookie) + errStrings := validateCookie(tc.cookie, tc.refresh) g := NewWithT(t) g.Expect(errStrings).To(ConsistOf(tc.errStrings)) diff --git a/pkg/validation/header_test.go b/pkg/validation/header_test.go index 2d9ef6dd..e9566752 100644 --- a/pkg/validation/header_test.go +++ b/pkg/validation/header_test.go @@ -30,7 +30,7 @@ var _ = Describe("Headers", func() { Values: []options.HeaderValue{ { SecretSource: &options.SecretSource{ - Value: []byte(base64.StdEncoding.EncodeToString([]byte("secret"))), + Value: []byte(base64.RawStdEncoding.EncodeToString([]byte("secret"))), }, }, }, @@ -43,7 +43,7 @@ var _ = Describe("Headers", func() { ClaimSource: &options.ClaimSource{ Claim: "email", BasicAuthPassword: &options.SecretSource{ - Value: []byte(base64.StdEncoding.EncodeToString([]byte("secret"))), + Value: []byte(base64.RawStdEncoding.EncodeToString([]byte("secret"))), }, }, }, 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..e0f7bbb0 100644 --- a/pkg/validation/options_test.go +++ b/pkg/validation/options_test.go @@ -14,12 +14,15 @@ import ( ) const ( - cookieSecret = "secretthirtytwobytes+abcdefghijk" clientID = "bazquux" clientSecret = "xyzzyplugh" providerID = "providerID" ) +var ( + cookieSecret = options.NewSecretSourceFromString("secretthirtytwobytes+abcdefghijk") +) + func testOptions() *options.Options { o := options.NewOptions() o.UpstreamServers.Upstreams = append(o.UpstreamServers.Upstreams, options.Upstream{ @@ -125,11 +128,11 @@ func TestCookieRefreshMustBeLessThanCookieExpire(t *testing.T) { o := testOptions() assert.Equal(t, nil, Validate(o)) - o.Cookie.Secret = "0123456789abcdef" - o.Cookie.Refresh = o.Cookie.Expire + o.Cookie.Secret = options.NewSecretSourceFromString("0123456789abcdef") + 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)) } @@ -138,23 +141,23 @@ func TestBase64CookieSecret(t *testing.T) { assert.Equal(t, nil, Validate(o)) // 32 byte, base64 (urlsafe) encoded key - o.Cookie.Secret = "yHBw2lh2Cvo6aI_jn_qMTr-pRAjtq0nzVgDJNb36jgQ=" + o.Cookie.Secret = options.NewSecretSourceFromString("yHBw2lh2Cvo6aI_jn_qMTr-pRAjtq0nzVgDJNb36jgQ=") assert.Equal(t, nil, Validate(o)) // 32 byte, base64 (urlsafe) encoded key, w/o padding - o.Cookie.Secret = "yHBw2lh2Cvo6aI_jn_qMTr-pRAjtq0nzVgDJNb36jgQ" + o.Cookie.Secret = options.NewSecretSourceFromString("yHBw2lh2Cvo6aI_jn_qMTr-pRAjtq0nzVgDJNb36jgQ") assert.Equal(t, nil, Validate(o)) // 24 byte, base64 (urlsafe) encoded key - o.Cookie.Secret = "Kp33Gj-GQmYtz4zZUyUDdqQKx5_Hgkv3" + o.Cookie.Secret = options.NewSecretSourceFromString("Kp33Gj-GQmYtz4zZUyUDdqQKx5_Hgkv3") assert.Equal(t, nil, Validate(o)) // 16 byte, base64 (urlsafe) encoded key - o.Cookie.Secret = "LFEqZYvYUwKwzn0tEuTpLA==" + o.Cookie.Secret = options.NewSecretSourceFromString("LFEqZYvYUwKwzn0tEuTpLA==") assert.Equal(t, nil, Validate(o)) // 16 byte, base64 (urlsafe) encoded key, w/o padding - o.Cookie.Secret = "LFEqZYvYUwKwzn0tEuTpLA" + o.Cookie.Secret = options.NewSecretSourceFromString("LFEqZYvYUwKwzn0tEuTpLA") 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") } diff --git a/pkg/validation/sessions_test.go b/pkg/validation/sessions_test.go index cb54c571..829df089 100644 --- a/pkg/validation/sessions_test.go +++ b/pkg/validation/sessions_test.go @@ -6,6 +6,7 @@ import ( "github.com/Bose/minisentinel" "github.com/alicebob/miniredis/v2" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/util/ptr" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) @@ -30,7 +31,7 @@ var _ = Describe("Sessions", func() { opts: &options.Options{ Session: options.SessionOptions{ Cookie: options.CookieStoreOptions{ - Minimal: false, + Minimal: ptr.To(false), }, }, }, @@ -40,7 +41,7 @@ var _ = Describe("Sessions", func() { opts: &options.Options{ Session: options.SessionOptions{ Cookie: options.CookieStoreOptions{ - Minimal: false, + Minimal: ptr.To(false), }, }, InjectRequestHeaders: []options.Header{ @@ -62,7 +63,7 @@ var _ = Describe("Sessions", func() { opts: &options.Options{ Session: options.SessionOptions{ Cookie: options.CookieStoreOptions{ - Minimal: true, + Minimal: ptr.To(true), }, }, }, @@ -72,7 +73,7 @@ var _ = Describe("Sessions", func() { opts: &options.Options{ Session: options.SessionOptions{ Cookie: options.CookieStoreOptions{ - Minimal: true, + Minimal: ptr.To(true), }, }, InjectRequestHeaders: []options.Header{ @@ -94,7 +95,7 @@ var _ = Describe("Sessions", func() { opts: &options.Options{ Session: options.SessionOptions{ Cookie: options.CookieStoreOptions{ - Minimal: true, + Minimal: ptr.To(true), }, }, InjectResponseHeaders: []options.Header{ @@ -116,7 +117,7 @@ var _ = Describe("Sessions", func() { opts: &options.Options{ Session: options.SessionOptions{ Cookie: options.CookieStoreOptions{ - Minimal: true, + Minimal: ptr.To(true), }, }, InjectRequestHeaders: []options.Header{ @@ -136,12 +137,11 @@ var _ = Describe("Sessions", func() { }), Entry("CookieRefresh conflict", &cookieMinimalTableInput{ opts: &options.Options{ - Cookie: options.Cookie{ - Refresh: time.Hour, - }, + Cookie: options.Cookie{}, Session: options.SessionOptions{ + Refresh: time.Hour, Cookie: options.CookieStoreOptions{ - Minimal: true, + Minimal: ptr.To(true), }, }, }, @@ -151,7 +151,7 @@ var _ = Describe("Sessions", func() { opts: &options.Options{ Session: options.SessionOptions{ Cookie: options.CookieStoreOptions{ - Minimal: true, + Minimal: ptr.To(true), }, }, InjectResponseHeaders: []options.Header{ @@ -323,7 +323,7 @@ var _ = Describe("Sessions", func() { Session: options.SessionOptions{ Type: options.RedisSessionStoreType, Redis: options.RedisStoreOptions{ - UseSentinel: true, + UseSentinel: ptr.To(true), }, }, }, @@ -340,7 +340,7 @@ var _ = Describe("Sessions", func() { Type: options.RedisSessionStoreType, Redis: options.RedisStoreOptions{ Password: "abcdef123", - UseSentinel: true, + UseSentinel: ptr.To(true), }, }, }, @@ -357,7 +357,7 @@ var _ = Describe("Sessions", func() { Type: options.RedisSessionStoreType, Redis: options.RedisStoreOptions{ Password: "zyxwtuv987", - UseSentinel: true, + UseSentinel: ptr.To(true), }, }, }, @@ -371,7 +371,7 @@ var _ = Describe("Sessions", func() { Session: options.SessionOptions{ Type: options.RedisSessionStoreType, Redis: options.RedisStoreOptions{ - UseSentinel: true, + UseSentinel: ptr.To(true), SentinelMasterName: "WRONG", }, }, @@ -386,7 +386,7 @@ var _ = Describe("Sessions", func() { Session: options.SessionOptions{ Type: options.RedisSessionStoreType, Redis: options.RedisStoreOptions{ - UseSentinel: true, + UseSentinel: ptr.To(true), SentinelConnectionURLs: []string{"redis://127.0.0.1:65535"}, }, }, @@ -398,8 +398,8 @@ var _ = Describe("Sessions", func() { Session: options.SessionOptions{ Type: options.RedisSessionStoreType, Redis: options.RedisStoreOptions{ - UseSentinel: true, - UseCluster: true, + UseSentinel: ptr.To(true), + UseCluster: ptr.To(true), }, }, },