feat(cookie): add feature support for cookie-secret-file (#3104)

* feat: add feature support for cookie-secret-file --------- Signed-off-by: Jan Larwig <jan@larwig.com> Co-Authored-By: Sandy Chen <Yuxuan.Chen@morganstanley.com> Co-authored-by: Jan Larwig <jan@larwig.com>
2025-08-06 22:42:56 +02:00 · 2025-07-23 01:59:55 +09:00
parent 137e59d526
commit dc8b1623a2
10 changed files with 226 additions and 36 deletions
--- 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

--- 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`<br/>toml: `cookie_refresh`                               | duration       | refresh the cookie after this duration; `0` to disable; not supported by all providers&nbsp;[^1]                                                                                                                                                  |                   |
 | flag: `--cookie-samesite`<br/>toml: `cookie_samesite`                             | string         | set SameSite cookie attribute (`"lax"`, `"strict"`, `"none"`, or `""`).                                                                                                                                                                           | `""`              |
 | flag: `--cookie-secret`<br/>toml: `cookie_secret`                                 | string         | the seed string for secure cookies (optionally base64 encoded)                                                                                                                                                                                    |                   |
+| flag: `--cookie-secret-file`<br/>toml: `cookie_secret_file`                       | string         | For defining a separate cookie secret file to read the encryption key from                                                                                                                                                                        |                   |
 | flag: `--cookie-secure`<br/>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)
--- 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
+}
--- a/pkg/apis/options/cookie_test.go
+++ 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)
+	})
+}
--- 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))
 }
--- 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)
 	}
--- 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)
 	}
--- 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)
--- 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 {
--- 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"})