From df0cf59f97aa788f6afffd2e045878e42c4c5591 Mon Sep 17 00:00:00 2001 From: Jan Larwig Date: Sun, 28 Dec 2025 20:33:03 +0100 Subject: [PATCH] fix: cookie secret source and header value conversion workflow Signed-off-by: Jan Larwig --- docs/docs/configuration/alpha_config.md | 21 +++++- docs/docs/configuration/alpha_config.md.tmpl | 14 ++++ main.go | 9 +-- main_test.go | 8 +- pkg/apis/options/claim_source.go | 45 +++++++++++ pkg/apis/options/cookie.go | 4 + pkg/apis/options/header.go | 21 +----- pkg/apis/options/secret_source.go | 27 +++++++ pkg/apis/options/secret_source_test.go | 79 ++++++++++++++++++++ 9 files changed, 196 insertions(+), 32 deletions(-) create mode 100644 pkg/apis/options/claim_source.go create mode 100644 pkg/apis/options/secret_source_test.go diff --git a/docs/docs/configuration/alpha_config.md b/docs/docs/configuration/alpha_config.md index ec06feec..34223fa5 100644 --- a/docs/docs/configuration/alpha_config.md +++ b/docs/docs/configuration/alpha_config.md @@ -285,6 +285,20 @@ longer available when using alpha configuration: - `cookie-csrf-per-request-limit`/`cookie_csrf_per_request_limit` - `cookie-csrf-expire`/`cookie_csrf_expire` +### Legacy Session options +- `session-cookie-minimal`/`session_cookie_minimal` +- `session-store-type`/`session_store_type` +- `redis-cluster-connection-urls`/`redis_cluster_connection_urls` +- `redis-connection-url`/`redis_connection_url` +- `redis-insecure-skip-tls-verify`/`redis_insecure_skip_tls_verify` +- `redis-password`/`redis_password` +- `redis-sentinel-password`/`redis_sentinel_password` +- `redis-sentinel-master-name`/`redis_sentinel_master_name` +- `redis-sentinel-connection-urls`/`redis_sentinel_connection_urls` +- `redis-use-cluster`/`redis_use_cluster` +- `redis-use-sentinel`/`redis_use_sentinel` +- `redis-connection-idle-timeout`/`redis_connection_idle_timeout` + and all provider-specific options, i.e. any option whose name includes `oidc`, `azure`, `bitbucket`, `github`, `gitlab`, `google` or `keycloak`. Attempting to use any of these options via flags or via config when `--alpha-config` is @@ -380,6 +394,7 @@ Cookie contains configuration options relating session and CSRF cookies | `insecure` | _bool_ | Insecure indicates whether the cookie allows to be sent over HTTP
Default is false, which requires HTTPS | | `scriptAccess` | _[ScriptAccess](#scriptaccess)_ | ScriptAccess is a wrapper enum for HTTPOnly; indicates whether the
cookie is accessible to JavaScript. Default is deny which translates
to true for HTTPOnly, which helps mitigate certain XSS attacks | | `sameSite` | _[SameSiteMode](#samesitemode)_ | SameSite sets the SameSite attribute on the cookie | +| `sameSite` | _[SameSiteMode](#samesitemode)_ | CSRFSameSite sets the SameSite attribute on the csrf cookies | | `csrfPerRequest` | _bool_ | 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 | | `csrfPerRequestLimit` | _int_ | 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 | | `csrfExpire` | _duration_ | CSRFExpire sets the duration before a CSRF token expires | @@ -689,14 +704,16 @@ RedisStoreOptions contains configuration options for the RedisSessionStore. (**Appears on:** [Cookie](#cookie)) - +SameSiteMode is an enum representing the different SameSite modes for cookies +Available modes are "lax", "strict", "none", and "" (default browser behavior) ### ScriptAccess #### (`string` alias) (**Appears on:** [Cookie](#cookie)) - +ScriptAccess is an enum representing whether a cookie is accessible to JavaScript +Available modes are "allow", "deny" (default behavior) ### SecretSource diff --git a/docs/docs/configuration/alpha_config.md.tmpl b/docs/docs/configuration/alpha_config.md.tmpl index d8002205..20d98b14 100644 --- a/docs/docs/configuration/alpha_config.md.tmpl +++ b/docs/docs/configuration/alpha_config.md.tmpl @@ -285,6 +285,20 @@ longer available when using alpha configuration: - `cookie-csrf-per-request-limit`/`cookie_csrf_per_request_limit` - `cookie-csrf-expire`/`cookie_csrf_expire` +### Legacy Session options +- `session-cookie-minimal`/`session_cookie_minimal` +- `session-store-type`/`session_store_type` +- `redis-cluster-connection-urls`/`redis_cluster_connection_urls` +- `redis-connection-url`/`redis_connection_url` +- `redis-insecure-skip-tls-verify`/`redis_insecure_skip_tls_verify` +- `redis-password`/`redis_password` +- `redis-sentinel-password`/`redis_sentinel_password` +- `redis-sentinel-master-name`/`redis_sentinel_master_name` +- `redis-sentinel-connection-urls`/`redis_sentinel_connection_urls` +- `redis-use-cluster`/`redis_use_cluster` +- `redis-use-sentinel`/`redis_use_sentinel` +- `redis-connection-idle-timeout`/`redis_connection_idle_timeout` + and all provider-specific options, i.e. any option whose name includes `oidc`, `azure`, `bitbucket`, `github`, `gitlab`, `google` or `keycloak`. Attempting to use any of these options via flags or via config when `--alpha-config` is diff --git a/main.go b/main.go index 797ad725..13d18886 100644 --- a/main.go +++ b/main.go @@ -189,14 +189,7 @@ func printConvertedConfig(opts *options.Options) error { return fmt.Errorf("only single provider conversion is supported") } - // Generic interface for loading arbitrary yaml structure - var buffer map[string]interface{} - - if err := options.Decode(alphaConfig, &buffer); err != nil { - return fmt.Errorf("unable to decode alpha config into interface: %w", err) - } - - data, err := yaml.Marshal(buffer) + data, err := yaml.Marshal(alphaConfig) if err != nil { return fmt.Errorf("unable to marshal config: %v", err) } diff --git a/main_test.go b/main_test.go index 2f0d7a72..654cd0ac 100644 --- a/main_test.go +++ b/main_test.go @@ -24,7 +24,7 @@ http_address="127.0.0.1:4180" upstreams="http://httpbin" set_basic_auth="true" -basic_auth_password="c3VwZXItc2VjcmV0LXBhc3N3b3Jk" +basic_auth_password="super-secret-password" client_id="oauth2-proxy" client_secret="b2F1dGgyLXByb3h5LWNsaWVudC1zZWNyZXQK" @@ -56,7 +56,7 @@ injectRequestHeaders: claim: user prefix: "Basic " basicAuthPassword: - value: YzNWd1pYSXRjMlZqY21WMExYQmhjM04zYjNKaw== + value: c3VwZXItc2VjcmV0LXBhc3N3b3Jk - name: X-Forwarded-Groups preserveRequestValue: false values: @@ -84,7 +84,7 @@ injectResponseHeaders: claim: user prefix: "Basic " basicAuthPassword: - value: "YzNWd1pYSXRjMlZqY21WMExYQmhjM04zYjNKaw==" + value: c3VwZXItc2VjcmV0LXBhc3N3b3Jk server: bindAddress: "127.0.0.1:4180" cookie: @@ -156,7 +156,7 @@ redirect_url="http://localhost:4180/oauth2/callback" ClaimSource: &options.ClaimSource{ Claim: "user", Prefix: "Basic ", - BasicAuthPassword: options.NewSecretSourceFromString("c3VwZXItc2VjcmV0LXBhc3N3b3Jk"), + BasicAuthPassword: &options.SecretSource{Value: []byte("c3VwZXItc2VjcmV0LXBhc3N3b3Jk")}, }, }, }, diff --git a/pkg/apis/options/claim_source.go b/pkg/apis/options/claim_source.go new file mode 100644 index 00000000..561e2810 --- /dev/null +++ b/pkg/apis/options/claim_source.go @@ -0,0 +1,45 @@ +package options + +import "fmt" + +// ClaimSource allows loading a header value from a claim within the session +type ClaimSource struct { + // Claim is the name of the claim in the session that the value should be + // loaded from. Available claims: `access_token` `id_token` `created_at` + // `expires_on` `refresh_token` `email` `user` `groups` `preferred_username`. + Claim string `yaml:"claim,omitempty"` + + // Prefix is an optional prefix that will be prepended to the value of the + // claim if it is non-empty. + Prefix string `yaml:"prefix,omitempty"` + + // BasicAuthPassword converts this claim into a basic auth header. + // Note the value of claim will become the basic auth username and the + // basicAuthPassword will be used as the password value. + BasicAuthPassword *SecretSource `yaml:"basicAuthPassword,omitempty"` +} + +// MarshalYAML implements the yaml.Marshaler interface for ClaimSource. +// This is only necessary for the conversion workflow from toml to yaml +func (c *ClaimSource) MarshalYAML() (interface{}, error) { + if c == nil { + return nil, nil + } + + mapped := make(map[string]interface{}) + if c.Claim != "" { + mapped["claim"] = c.Claim + } + if c.Prefix != "" { + mapped["prefix"] = c.Prefix + } + if c.BasicAuthPassword != nil { + basicAuthPasswordYAML, err := c.BasicAuthPassword.MarshalYAML() + if err != nil { + return nil, fmt.Errorf("error marshaling basicAuthPassword for ClaimSource: %w", err) + } + mapped["basicAuthPassword"] = basicAuthPasswordYAML + } + + return mapped, nil +} diff --git a/pkg/apis/options/cookie.go b/pkg/apis/options/cookie.go index 329cb76f..4f1b9333 100644 --- a/pkg/apis/options/cookie.go +++ b/pkg/apis/options/cookie.go @@ -15,6 +15,8 @@ const ( DefaultCSRFPerRequest bool = false ) +// SameSiteMode is an enum representing the different SameSite modes for cookies +// Available modes are "lax", "strict", "none", and "" (default browser behavior) type SameSiteMode string const ( @@ -24,6 +26,8 @@ const ( SameSiteDefault SameSiteMode = "" ) +// ScriptAccess is an enum representing whether a cookie is accessible to JavaScript +// Available modes are "allow", "deny" (default behavior) type ScriptAccess string const ( diff --git a/pkg/apis/options/header.go b/pkg/apis/options/header.go index d9509de0..f0d29c1f 100644 --- a/pkg/apis/options/header.go +++ b/pkg/apis/options/header.go @@ -1,6 +1,8 @@ package options -import "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/util/ptr" +import ( + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/util/ptr" +) const ( // DefaultHeaderPreserveRequestValue is the default value for Header.PreserveRequestValue @@ -46,23 +48,6 @@ type HeaderValue struct { *ClaimSource `yaml:"claimSource,omitempty"` } -// ClaimSource allows loading a header value from a claim within the session -type ClaimSource struct { - // Claim is the name of the claim in the session that the value should be - // loaded from. Available claims: `access_token` `id_token` `created_at` - // `expires_on` `refresh_token` `email` `user` `groups` `preferred_username`. - Claim string `yaml:"claim,omitempty"` - - // Prefix is an optional prefix that will be prepended to the value of the - // claim if it is non-empty. - Prefix string `yaml:"prefix,omitempty"` - - // BasicAuthPassword converts this claim into a basic auth header. - // Note the value of claim will become the basic auth username and the - // basicAuthPassword will be used as the password value. - BasicAuthPassword *SecretSource `yaml:"basicAuthPassword,omitempty"` -} - // EnsureDefaults sets any default values for Header fields. func (h *Header) EnsureDefaults() { if h.PreserveRequestValue == nil { diff --git a/pkg/apis/options/secret_source.go b/pkg/apis/options/secret_source.go index f3b35f2b..34fd0ba6 100644 --- a/pkg/apis/options/secret_source.go +++ b/pkg/apis/options/secret_source.go @@ -62,6 +62,10 @@ func (ss *SecretSource) GetSecretValue() ([]byte, error) { return nil, fmt.Errorf("failed retrieving secret value: no source defined") } + if len(ss.Value) == 0 && ss.FromFile != "" { + return value, nil + } + decoded := make([]byte, base64.URLEncoding.DecodedLen(len(value))) if _, err := base64.URLEncoding.Decode(decoded, value); err != nil { return nil, fmt.Errorf("error decoding secret value: %w", err) @@ -69,6 +73,29 @@ func (ss *SecretSource) GetSecretValue() ([]byte, error) { return decoded, nil } +// MarshalYAML implements the yaml.Marshaler interface for SecretSource. +// This is only necessary for the conversion workflow from toml to yaml +func (ss *SecretSource) MarshalYAML() (interface{}, error) { + if ss == nil { + return nil, nil + } + + if ss.FromFile != "" { + return map[string]string{ + "fromFile": ss.FromFile, + }, nil + } + + encodedValue, err := ss.GetRawSecretValue() + if err != nil { + return nil, fmt.Errorf("error getting raw secret value for marshaling: %w", err) + } + + return map[string]string{ + "value": string(encodedValue), + }, nil +} + // EnsureDefaults sets any default values for SecretSource fields. func (ss *SecretSource) EnsureDefaults() { // No defaults to set currently diff --git a/pkg/apis/options/secret_source_test.go b/pkg/apis/options/secret_source_test.go new file mode 100644 index 00000000..40250798 --- /dev/null +++ b/pkg/apis/options/secret_source_test.go @@ -0,0 +1,79 @@ +package options + +import ( + "os" + "testing" + + "github.com/stretchr/testify/assert" +) + +const ( + testSecretValue = "bXktc2VjcmV0" // base64 for "my-secret" + testFileSecretContent = "file-secret" +) + +func TestSecretSourceGetSecretValue(t *testing.T) { + t.Run("returns secret when Value is set", func(t *testing.T) { + ss := &SecretSource{ + Value: []byte(testSecretValue), + FromFile: "", + } + secret, err := ss.GetSecretValue() + assert.NoError(t, err) + assert.Equal(t, "my-secret", string(secret)) + }) + + t.Run("returns secret when both Value and FromFile are set", func(t *testing.T) { + ss := &SecretSource{ + Value: []byte(testSecretValue), + FromFile: "/some/file", + } + secret, err := ss.GetSecretValue() + assert.NoError(t, err) + assert.Equal(t, "my-secret", string(secret)) + }) + + t.Run("reads from file when only FromFile is set", func(t *testing.T) { + // Create a temporary file + tmpfile, err := os.CreateTemp("", "secret-source-test") + assert.NoError(t, err) + defer os.Remove(tmpfile.Name()) + + _, err = tmpfile.Write([]byte(testFileSecretContent)) + assert.NoError(t, err) + tmpfile.Close() + + ss := &SecretSource{ + FromFile: tmpfile.Name(), + } + secret, err := ss.GetSecretValue() + assert.NoError(t, err) + assert.Equal(t, "file-secret", string(secret)) + }) + + t.Run("returns error when file does not exist", func(t *testing.T) { + ss := &SecretSource{ + FromFile: "/nonexistent/file", + } + secret, err := ss.GetSecretValue() + assert.Error(t, err) + assert.Nil(t, secret) + }) + + t.Run("returns error when no source is defined", func(t *testing.T) { + ss := &SecretSource{} + secret, err := ss.GetSecretValue() + assert.Error(t, err) + assert.Nil(t, secret) + }) + + t.Run("returns error when Value is not valid base64", func(t *testing.T) { + ss := &SecretSource{ + Value: []byte("invalid-base64"), + FromFile: "", + } + secret, err := ss.GetSecretValue() + assert.Error(t, err) + assert.Nil(t, secret) + }) +}