diff --git a/CHANGELOG.md b/CHANGELOG.md index 50dbc9b9..ea7b0b09 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,8 @@ - [#2605](https://github.com/oauth2-proxy/oauth2-proxy/pull/2605) fix: show login page on broken cookie (@Primexz) - [#2743](https://github.com/oauth2-proxy/oauth2-proxy/pull/2743) feat: allow use more possible google admin-sdk api scopes (@BobDu) - [#2359](https://github.com/oauth2-proxy/oauth2-proxy/pull/2359) feat: add SourceHut (sr.ht) provider(@bitfehler) --[#2524](https://github.com/oauth2-proxy/oauth2-proxy/pull/2524) fix: regex substitution for $ signs in upstream path handling before running envsubst (@dashkan / @tuunit) +- [#2524](https://github.com/oauth2-proxy/oauth2-proxy/pull/2524) fix: regex substitution for $ signs in upstream path handling before running envsubst (@dashkan / @tuunit) +- [#3104](https://github.com/oauth2-proxy/oauth2-proxy/pull/3104) feat(cookie): add feature support for cookie-secret-file (@sandy2008) # V7.10.0 diff --git a/docs/docs/configuration/overview.md b/docs/docs/configuration/overview.md index 6a8f52e5..7c216dfb 100644 --- a/docs/docs/configuration/overview.md +++ b/docs/docs/configuration/overview.md @@ -128,6 +128,7 @@ Provider specific options can be found on their respective subpages. | flag: `--cookie-refresh`
toml: `cookie_refresh` | duration | refresh the cookie after this duration; `0` to disable; not supported by all providers [^1] | | | flag: `--cookie-samesite`
toml: `cookie_samesite` | string | set SameSite cookie attribute (`"lax"`, `"strict"`, `"none"`, or `""`). | `""` | | flag: `--cookie-secret`
toml: `cookie_secret` | string | the seed string for secure cookies (optionally base64 encoded) | | +| flag: `--cookie-secret-file`
toml: `cookie_secret_file` | string | For defining a separate cookie secret file to read the encryption key from | | | flag: `--cookie-secure`
toml: `cookie_secure` | bool | set [secure (HTTPS only) cookie flag](https://owasp.org/www-community/controls/SecureFlag) | true | [^1]: The following providers support `--cookie-refresh`: ADFS, Azure, GitLab, Google, Keycloak and all other Identity Providers which support the full [OIDC specification](https://openid.net/specs/openid-connect-core-1_0.html#RefreshTokens) diff --git a/pkg/apis/options/cookie.go b/pkg/apis/options/cookie.go index 22b74a6c..3653b7d0 100644 --- a/pkg/apis/options/cookie.go +++ b/pkg/apis/options/cookie.go @@ -1,8 +1,11 @@ package options import ( + "errors" + "os" "time" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger" "github.com/spf13/pflag" ) @@ -10,6 +13,7 @@ import ( type Cookie struct { Name string `flag:"cookie-name" cfg:"cookie_name"` Secret string `flag:"cookie-secret" cfg:"cookie_secret"` + SecretFile string `flag:"cookie-secret-file" cfg:"cookie_secret_file"` Domains []string `flag:"cookie-domain" cfg:"cookie_domains"` Path string `flag:"cookie-path" cfg:"cookie_path"` Expire time.Duration `flag:"cookie-expire" cfg:"cookie_expire"` @@ -18,8 +22,8 @@ type Cookie struct { HTTPOnly bool `flag:"cookie-httponly" cfg:"cookie_httponly"` SameSite string `flag:"cookie-samesite" cfg:"cookie_samesite"` CSRFPerRequest bool `flag:"cookie-csrf-per-request" cfg:"cookie_csrf_per_request"` - CSRFExpire time.Duration `flag:"cookie-csrf-expire" cfg:"cookie_csrf_expire"` CSRFPerRequestLimit int `flag:"cookie-csrf-per-request-limit" cfg:"cookie_csrf_per_request_limit"` + CSRFExpire time.Duration `flag:"cookie-csrf-expire" cfg:"cookie_csrf_expire"` } func cookieFlagSet() *pflag.FlagSet { @@ -27,6 +31,7 @@ func cookieFlagSet() *pflag.FlagSet { flagSet.String("cookie-name", "_oauth2_proxy", "the name of the cookie that the oauth_proxy creates") flagSet.String("cookie-secret", "", "the seed string for secure cookies (optionally base64 encoded)") + flagSet.String("cookie-secret-file", "", "For defining a separate cookie secret file to read the encryption key from") flagSet.StringSlice("cookie-domain", []string{}, "Optional cookie domains to force cookies to (ie: `.yourcompany.com`). The longest domain matching the request's host will be used (or the shortest cookie domain if there is no match).") flagSet.String("cookie-path", "/", "an optional cookie path to force cookies to (ie: /poc/)*") flagSet.Duration("cookie-expire", time.Duration(168)*time.Hour, "expire timeframe for cookie") @@ -43,16 +48,33 @@ func cookieFlagSet() *pflag.FlagSet { // cookieDefaults creates a Cookie populating each field with its default value func cookieDefaults() Cookie { return Cookie{ - Name: "_oauth2_proxy", - Secret: "", - Domains: nil, - Path: "/", - Expire: time.Duration(168) * time.Hour, - Refresh: time.Duration(0), - Secure: true, - HTTPOnly: true, - SameSite: "", - CSRFPerRequest: false, - CSRFExpire: time.Duration(15) * time.Minute, + Name: "_oauth2_proxy", + Secret: "", + SecretFile: "", + Domains: nil, + Path: "/", + Expire: time.Duration(168) * time.Hour, + Refresh: time.Duration(0), + Secure: true, + HTTPOnly: true, + SameSite: "", + CSRFPerRequest: false, + CSRFPerRequestLimit: 0, + CSRFExpire: time.Duration(15) * time.Minute, } } + +// 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 + } + + fileSecret, err := os.ReadFile(c.SecretFile) + if err != nil { + logger.Errorf("error reading cookie secret file %s: %s", c.SecretFile, err) + return "", errors.New("could not read cookie secret file") + } + + return string(fileSecret), nil +} diff --git a/pkg/apis/options/cookie_test.go b/pkg/apis/options/cookie_test.go new file mode 100644 index 00000000..a1486fed --- /dev/null +++ b/pkg/apis/options/cookie_test.go @@ -0,0 +1,70 @@ +package options + +import ( + "os" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestCookieGetSecret(t *testing.T) { + t.Run("returns secret when Secret is set", func(t *testing.T) { + c := &Cookie{ + Secret: "my-secret", + SecretFile: "", + } + secret, err := c.GetSecret() + assert.NoError(t, err) + assert.Equal(t, "my-secret", secret) + }) + + t.Run("returns secret when both Secret and SecretFile are set", func(t *testing.T) { + c := &Cookie{ + Secret: "my-secret", + SecretFile: "/some/file", + } + secret, err := c.GetSecret() + assert.NoError(t, err) + assert.Equal(t, "my-secret", secret) + }) + + t.Run("reads from file when only SecretFile is set", func(t *testing.T) { + // Create a temporary file + tmpfile, err := os.CreateTemp("", "cookie-secret-test") + assert.NoError(t, err) + defer os.Remove(tmpfile.Name()) + + _, err = tmpfile.Write([]byte("file-secret")) + assert.NoError(t, err) + tmpfile.Close() + + c := &Cookie{ + Secret: "", + SecretFile: tmpfile.Name(), + } + secret, err := c.GetSecret() + assert.NoError(t, err) + assert.Equal(t, "file-secret", secret) + }) + + t.Run("returns error when file does not exist", func(t *testing.T) { + c := &Cookie{ + Secret: "", + SecretFile: "/nonexistent/file", + } + secret, err := c.GetSecret() + assert.Error(t, err) + assert.Equal(t, "", secret) + assert.Contains(t, err.Error(), "could not read cookie secret file") + }) + + t.Run("returns empty when both Secret and SecretFile are empty", func(t *testing.T) { + c := &Cookie{ + Secret: "", + SecretFile: "", + } + secret, err := c.GetSecret() + assert.NoError(t, err) + assert.Equal(t, "", secret) + }) +} diff --git a/pkg/cookies/csrf.go b/pkg/cookies/csrf.go index eab87869..3b8efaf3 100644 --- a/pkg/cookies/csrf.go +++ b/pkg/cookies/csrf.go @@ -219,13 +219,22 @@ func (c *csrf) encodeCookie() (string, error) { return "", err } - return encryption.SignedValue(c.cookieOpts.Secret, c.cookieName(), encrypted, c.time.Now()) + secret, err := c.cookieOpts.GetSecret() + if err != nil { + return "", fmt.Errorf("error getting cookie secret: %v", err) + } + return encryption.SignedValue(secret, c.cookieName(), encrypted, c.time.Now()) } // decodeCSRFCookie validates the signature then decrypts and decodes a CSRF // cookie into a CSRF struct func decodeCSRFCookie(cookie *http.Cookie, opts *options.Cookie) (*csrf, error) { - val, t, ok := encryption.Validate(cookie, opts.Secret, opts.Expire) + secret, err := opts.GetSecret() + if err != nil { + return nil, fmt.Errorf("error getting cookie secret: %v", err) + } + + val, t, ok := encryption.Validate(cookie, secret, opts.Expire) if !ok { return nil, errors.New("CSRF cookie failed validation") } @@ -235,15 +244,18 @@ func decodeCSRFCookie(cookie *http.Cookie, opts *options.Cookie) (*csrf, error) return nil, err } - // Valid cookie, Unmarshal the CSRF + return unmarshalCSRF(decrypted, opts, t) +} + +// unmarshalCSRF unmarshals decrypted data into a CSRF struct +func unmarshalCSRF(decrypted []byte, opts *options.Cookie, csrfTime time.Time) (*csrf, error) { clock := clock.Clock{} - clock.Set(t) + clock.Set(csrfTime) + csrf := &csrf{cookieOpts: opts, time: clock} - err = msgpack.Unmarshal(decrypted, csrf) - if err != nil { + if err := msgpack.Unmarshal(decrypted, csrf); err != nil { return nil, fmt.Errorf("error unmarshalling data to CSRF: %v", err) } - return csrf, nil } @@ -290,5 +302,9 @@ func decrypt(data []byte, opts *options.Cookie) ([]byte, error) { } func makeCipher(opts *options.Cookie) (encryption.Cipher, error) { - return encryption.NewCFBCipher(encryption.SecretBytes(opts.Secret)) + secret, err := opts.GetSecret() + if err != nil { + return nil, fmt.Errorf("error getting cookie secret: %v", err) + } + return encryption.NewCFBCipher(encryption.SecretBytes(secret)) } diff --git a/pkg/sessions/cookie/session_store.go b/pkg/sessions/cookie/session_store.go index 3947177f..095bc0e7 100644 --- a/pkg/sessions/cookie/session_store.go +++ b/pkg/sessions/cookie/session_store.go @@ -54,16 +54,18 @@ func (s *SessionStore) Load(req *http.Request) (*sessions.SessionState, error) { // always http.ErrNoCookie return nil, err } - val, _, ok := encryption.Validate(c, s.Cookie.Secret, s.Cookie.Expire) + + secret, err := s.Cookie.GetSecret() + if err != nil { + return nil, fmt.Errorf("error getting cookie secret: %v", err) + } + + val, _, ok := encryption.Validate(c, secret, s.Cookie.Expire) if !ok { return nil, errors.New("cookie signature not valid") } - session, err := sessions.DecodeSessionState(val, s.CookieCipher, true) - if err != nil { - return nil, err - } - return session, nil + return sessions.DecodeSessionState(val, s.CookieCipher, true) } // Clear clears any saved session information by writing a cookie to @@ -121,7 +123,11 @@ func (s *SessionStore) makeSessionCookie(req *http.Request, value []byte, now ti strValue := string(value) if strValue != "" { var err error - strValue, err = encryption.SignedValue(s.Cookie.Secret, s.Cookie.Name, value, now) + secret, err := s.Cookie.GetSecret() + if err != nil { + return nil, fmt.Errorf("error getting cookie secret: %v", err) + } + strValue, err = encryption.SignedValue(secret, s.Cookie.Name, value, now) if err != nil { return nil, err } @@ -146,7 +152,11 @@ func (s *SessionStore) makeCookie(req *http.Request, name string, value string, // NewCookieSessionStore initialises a new instance of the SessionStore from // the configuration given func NewCookieSessionStore(opts *options.SessionOptions, cookieOpts *options.Cookie) (sessions.SessionStore, error) { - cipher, err := encryption.NewCFBCipher(encryption.SecretBytes(cookieOpts.Secret)) + secret, err := cookieOpts.GetSecret() + if err != nil { + return nil, fmt.Errorf("error getting cookie secret: %v", err) + } + cipher, err := encryption.NewCFBCipher(encryption.SecretBytes(secret)) if err != nil { return nil, fmt.Errorf("error initialising cipher: %v", err) } diff --git a/pkg/sessions/persistence/ticket.go b/pkg/sessions/persistence/ticket.go index 581a7f45..7855db45 100644 --- a/pkg/sessions/persistence/ticket.go +++ b/pkg/sessions/persistence/ticket.go @@ -146,7 +146,11 @@ func decodeTicketFromRequest(req *http.Request, cookieOpts *options.Cookie) (*ti } // An existing cookie exists, try to retrieve the ticket - val, _, ok := encryption.Validate(requestCookie, cookieOpts.Secret, cookieOpts.Expire) + secret, err := cookieOpts.GetSecret() + if err != nil { + return nil, fmt.Errorf("error getting cookie secret: %v", err) + } + val, _, ok := encryption.Validate(requestCookie, secret, cookieOpts.Expire) if !ok { return nil, fmt.Errorf("session ticket cookie failed validation: %v", err) } diff --git a/pkg/validation/cookie.go b/pkg/validation/cookie.go index b515809d..5f2dd8ac 100644 --- a/pkg/validation/cookie.go +++ b/pkg/validation/cookie.go @@ -3,6 +3,7 @@ package validation import ( "fmt" "net/http" + "os" "sort" "time" @@ -11,7 +12,7 @@ import ( ) func validateCookie(o options.Cookie) []string { - msgs := validateCookieSecret(o.Secret) + msgs := validateCookieSecret(o.Secret, o.SecretFile) if o.Expire != time.Duration(0) && o.Refresh >= o.Expire { msgs = append(msgs, fmt.Sprintf( @@ -49,9 +50,27 @@ func validateCookieName(name string) []string { return msgs } -func validateCookieSecret(secret string) []string { - if secret == "" { - return []string{"missing setting: cookie-secret"} +func validateCookieSecret(secret string, secretFile string) []string { + if secret == "" && secretFile == "" { + 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)), + } } secretBytes := encryption.SecretBytes(secret) diff --git a/pkg/validation/cookie_test.go b/pkg/validation/cookie_test.go index 1f0dc5cd..d11134da 100644 --- a/pkg/validation/cookie_test.go +++ b/pkg/validation/cookie_test.go @@ -1,6 +1,7 @@ package validation import ( + "os" "strings" "testing" "time" @@ -29,9 +30,23 @@ func TestValidateCookie(t *testing.T) { "a.cba.localhost", } + // Create a temporary file for the valid secret file test + tmpfile, err := os.CreateTemp("", "cookie-secret-test") + if err != nil { + t.Fatalf("Failed to create temporary file: %v", err) + } + defer os.Remove(tmpfile.Name()) + + // Write a valid 32-byte secret to the file + _, err = tmpfile.Write([]byte(validSecret)) + if err != nil { + t.Fatalf("Failed to write to temporary file: %v", err) + } + tmpfile.Close() + invalidNameMsg := "invalid cookie name: \"_oauth2;proxy\"" longNameMsg := "cookie name should be under 256 characters: cookie name is 260 characters" - missingSecretMsg := "missing setting: cookie-secret" + missingSecretMsg := "missing setting: cookie-secret or cookie-secret-file" invalidSecretMsg := "cookie_secret must be 16, 24, or 32 bytes to create an AES cipher, but is 6 bytes" invalidBase64SecretMsg := "cookie_secret must be 16, 24, or 32 bytes to create an AES cipher, but is 10 bytes" refreshLongerThanExpireMsg := "cookie_refresh (\"1h0m0s\") must be less than cookie_expire (\"15m0s\")" @@ -271,6 +286,38 @@ func TestValidateCookie(t *testing.T) { }, 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: true, + HTTPOnly: true, + SameSite: "", + }, + 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: true, + HTTPOnly: true, + SameSite: "", + }, + errStrings: []string{"could not read cookie secret file: /nonexistent/file.txt"}, + }, } for _, tc := range testCases { diff --git a/pkg/validation/options_test.go b/pkg/validation/options_test.go index 2d5e9560..5c242e02 100644 --- a/pkg/validation/options_test.go +++ b/pkg/validation/options_test.go @@ -48,7 +48,7 @@ func TestNewOptions(t *testing.T) { assert.NotEqual(t, nil, err) expected := errorMsg([]string{ - "missing setting: cookie-secret", + "missing setting: cookie-secret or cookie-secret-file", "provider has empty id: ids are required for all providers", "provider missing setting: client-id", "missing setting: client-secret or client-secret-file"})