fix: cookie secret related test cases

Signed-off-by: Jan Larwig <jan@larwig.com>

pkg/apis/options/cookie.go

@@ -37,7 +37,7 @@ type Cookie struct {
 	// Name is the name of the cookie
 	Name string `yaml:"name,omitempty"`
 	// Secret is the secret source used to encrypt/sign the cookie value
-	Secret SecretSource `yaml:"secret,omitempty"`
+	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
@@ -101,7 +101,7 @@ func (sa *ScriptAccess) UnmarshalYAML(value *yaml.Node) error {
 
 // GetSecret returns the cookie secret as a string from the SecretSource
 func (c *Cookie) GetSecret() (string, error) {
-	secret, err := c.Secret.GetSecretValue()
+	secret, err := c.Secret.GetRawSecretValue()
 	if err != nil {
 		return "", fmt.Errorf("error getting cookie secret: %w", err)
 	}
@@ -120,6 +120,9 @@ func (c *Cookie) EnsureDefaults() {
 	if c.Expire == 0 {
 		c.Expire = time.Duration(168) * time.Hour
 	}
+	if c.Secret == nil {
+		c.Secret = &SecretSource{}
+	}
 	if c.Insecure == nil {
 		c.Insecure = ptr.To(DefaultCookieInsecure)
 	}

pkg/apis/options/cookie_test.go

@@ -10,7 +10,7 @@ import (
 func TestCookieGetSecret(t *testing.T) {
 	t.Run("returns secret when Secret is set", func(t *testing.T) {
 		c := &Cookie{
-			Secret: SecretSource{
+			Secret: &SecretSource{
 				Value:    []byte("my-secret"),
 				FromFile: "",
 			},
@@ -22,7 +22,7 @@ func TestCookieGetSecret(t *testing.T) {
 
 	t.Run("returns secret when both Secret and SecretFile are set", func(t *testing.T) {
 		c := &Cookie{
-			Secret: SecretSource{
+			Secret: &SecretSource{
 				Value:    []byte("my-secret"),
 				FromFile: "/some/file",
 			},
@@ -43,7 +43,7 @@ func TestCookieGetSecret(t *testing.T) {
 		tmpfile.Close()
 
 		c := &Cookie{
-			Secret: SecretSource{
+			Secret: &SecretSource{
 				Value:    []byte(""),
 				FromFile: tmpfile.Name(),
 			},
@@ -55,7 +55,7 @@ func TestCookieGetSecret(t *testing.T) {
 
 	t.Run("returns error when file does not exist", func(t *testing.T) {
 		c := &Cookie{
-			Secret: SecretSource{
+			Secret: &SecretSource{
 				Value:    []byte(""),
 				FromFile: "/nonexistent/file",
 			},
@@ -63,12 +63,12 @@ func TestCookieGetSecret(t *testing.T) {
 		secret, err := c.GetSecret()
 		assert.Error(t, err)
 		assert.Equal(t, "", secret)
-		assert.Contains(t, err.Error(), "error reading cookie secret file /nonexistent/file:")
+		assert.Contains(t, err.Error(), "error getting cookie secret: error reading secret from file \"/nonexistent/file\": open /nonexistent/file: no such file or directory")
 	})
 
 	t.Run("returns empty when both Secret and SecretFile are empty", func(t *testing.T) {
 		c := &Cookie{
-			Secret: SecretSource{
+			Secret: &SecretSource{
 				Value:    []byte(""),
 				FromFile: "",
 			},

pkg/apis/options/legacy_cookie.go

@@ -52,9 +52,11 @@ func (l *LegacyCookie) convert() Cookie {
 		scriptAccess = ScriptAccessAllowed
 	}
 
-	var secret *SecretSource
+	secret := &SecretSource{}
 	if l.Secret != "" {
-		secret = NewSecretSourceFromString(l.Secret)
+		secret = &SecretSource{
+			Value: []byte(l.Secret),
+		}
 	} else if l.SecretFile != "" {
 		secret = &SecretSource{
 			FromFile: l.SecretFile,
@@ -63,7 +65,7 @@ func (l *LegacyCookie) convert() Cookie {
 
 	return Cookie{
 		Name:                l.Name,
-		Secret:              *secret,
+		Secret:              secret,
 		Domains:             l.Domains,
 		Path:                l.Path,
 		Expire:              l.Expire,

pkg/apis/options/legacy_options_test.go

@@ -372,11 +372,9 @@ var _ = Describe("Legacy Options", func() {
 			Values: []HeaderValue{
 				{
 					ClaimSource: &ClaimSource{
-						Claim:  "user",
-						Prefix: "Basic ",
-						BasicAuthPassword: &SecretSource{
-							Value: []byte(basicAuthSecret),
-						},
+						Claim:             "user",
+						Prefix:            "Basic ",
+						BasicAuthPassword: NewSecretSourceFromString(basicAuthSecret),
 					},
 				},
 			},
@@ -412,11 +410,9 @@ var _ = Describe("Legacy Options", func() {
 			Values: []HeaderValue{
 				{
 					ClaimSource: &ClaimSource{
-						Claim:  "email",
-						Prefix: "Basic ",
-						BasicAuthPassword: &SecretSource{
-							Value: []byte(basicAuthSecret),
-						},
+						Claim:             "email",
+						Prefix:            "Basic ",
+						BasicAuthPassword: NewSecretSourceFromString(basicAuthSecret),
 					},
 				},
 			},
@@ -1096,7 +1092,7 @@ var _ = Describe("Legacy Options", func() {
 		// Test cases and expected outcomes
 		fullCookie := Cookie{
 			Name:                "_oauth2_proxy",
-			Secret:              SecretSource{},
+			Secret:              &SecretSource{},
 			Domains:             nil,
 			Path:                "/",
 			Expire:              time.Duration(168) * time.Hour,

pkg/apis/options/options.go

@@ -36,7 +36,7 @@ type Options struct {
 	HtpasswdUserGroups      []string `flag:"htpasswd-user-group" cfg:"htpasswd_user_groups"`
 
 	Cookie    Cookie         `cfg:",internal"`
-	Session   SessionOptions `cfg:",squash"`
+	Session   SessionOptions `cfg:",internal"`
 	Logging   Logging        `cfg:",squash"`
 	Templates Templates      `cfg:",squash"`
 

pkg/apis/options/secret_source.go

@@ -20,8 +20,8 @@ type SecretSource struct {
 }
 
 func NewSecretSourceFromValue(value []byte) *SecretSource {
-	encoded := make([]byte, base64.RawStdEncoding.EncodedLen(len(value)))
-	base64.RawStdEncoding.Encode(encoded, value)
+	encoded := make([]byte, base64.URLEncoding.EncodedLen(len(value)))
+	base64.URLEncoding.Encode(encoded, value)
 	return &SecretSource{
 		Value: encoded,
 	}
@@ -31,13 +31,9 @@ func NewSecretSourceFromString(s string) *SecretSource {
 	return NewSecretSourceFromValue([]byte(s))
 }
 
-func (ss *SecretSource) GetSecretValue() ([]byte, error) {
+func (ss *SecretSource) GetRawSecretValue() ([]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
+		return ss.Value, nil
 	}
 
 	if ss.FromEnv != "" {
@@ -56,6 +52,23 @@ func (ss *SecretSource) GetSecretValue() ([]byte, error) {
 	return nil, nil
 }
 
+func (ss *SecretSource) GetSecretValue() ([]byte, error) {
+	value, err := ss.GetRawSecretValue()
+	if err != nil {
+		return nil, fmt.Errorf("failed getting raw secret value: %w", err)
+	}
+
+	if value == nil {
+		return nil, fmt.Errorf("failed retrieving secret value: no source defined")
+	}
+
+	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)
+	}
+	return decoded, nil
+}
+
 // EnsureDefaults sets any default values for SecretSource fields.
 func (ss *SecretSource) EnsureDefaults() {
 	// No defaults to set currently

pkg/cookies/cookies.go

@@ -26,9 +26,11 @@ func MakeCookieFromOptions(req *http.Request, value string, opts *options.Cookie
 		domain = opts.Domains[len(opts.Domains)-1]
 	}
 
-	httpOnly := true
+	var httpOnly bool
 	if opts.ScriptAccess == options.ScriptAccessAllowed {
 		httpOnly = false
+	} else {
+		httpOnly = true
 	}
 
 	c := &http.Cookie{

pkg/cookies/cookies_test.go

@@ -90,7 +90,7 @@ var _ = Describe("Cookie Tests", func() {
 		}
 
 		validName := "_oauth2_proxy"
-		validSecret := []byte("secretthirtytwobytes+abcdefghijk")
+		validSecret := &options.SecretSource{Value: []byte("secretthirtytwobytes+abcdefghijk")}
 		domains := []string{"www.cookies.test"}
 
 		now := time.Now()
@@ -112,7 +112,7 @@ var _ = Describe("Cookie Tests", func() {
 				value: "1",
 				opts: options.Cookie{
 					Name:         validName,
-					Secret:       options.SecretSource{Value: validSecret},
+					Secret:       validSecret,
 					Domains:      domains,
 					Path:         "",
 					Expire:       time.Hour,
@@ -128,7 +128,7 @@ var _ = Describe("Cookie Tests", func() {
 				value: "1",
 				opts: options.Cookie{
 					Name:         validName,
-					Secret:       options.SecretSource{Value: validSecret},
+					Secret:       validSecret,
 					Domains:      domains,
 					Path:         "",
 					Expire:       time.Hour * -1,
@@ -144,7 +144,7 @@ var _ = Describe("Cookie Tests", func() {
 				value: "1",
 				opts: options.Cookie{
 					Name:         validName,
-					Secret:       options.SecretSource{Value: validSecret},
+					Secret:       validSecret,
 					Domains:      domains,
 					Path:         "",
 					Expire:       0,

pkg/cookies/csrf_per_request_test.go

@@ -25,7 +25,7 @@ var _ = Describe("CSRF Cookie with non-fixed name Tests", func() {
 	BeforeEach(func() {
 		cookieOpts = &options.Cookie{
 			Name:           cookieName,
-			Secret:         options.SecretSource{Value: cookieSecret},
+			Secret:         &options.SecretSource{Value: cookieSecret},
 			Domains:        []string{cookieDomain},
 			Path:           cookiePath,
 			Expire:         time.Hour,

pkg/cookies/csrf_test.go

@@ -26,7 +26,7 @@ var _ = Describe("CSRF Cookie Tests", func() {
 	BeforeEach(func() {
 		cookieOpts = &options.Cookie{
 			Name:           cookieName,
-			Secret:         options.SecretSource{Value: cookieSecret},
+			Secret:         &options.SecretSource{Value: cookieSecret},
 			Domains:        []string{cookieDomain},
 			Path:           cookiePath,
 			Expire:         time.Hour,

pkg/encryption/utils.go

@@ -9,6 +9,7 @@ import (
 	"hash"
 	"io"
 	"net/http"
+	"slices"
 	"strconv"
 	"strings"
 	"time"
@@ -27,10 +28,8 @@ func SecretBytes(secret string) []byte {
 		// Only return decoded form if a valid AES length
 		// Don't want unintentional decoding resulting in invalid lengths confusing a user
 		// that thought they used a 16, 24, 32 length string
-		for _, i := range []int{16, 24, 32} {
-			if len(b) == i {
-				return b
-			}
+		if slices.Contains([]int{16, 24, 32}, len(b)) {
+			return b
 		}
 	}
 	// If decoding didn't work or resulted in non-AES compliant length,

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.RawStdEncoding.EncodeToString([]byte("basic-password"))),
+									Value:   []byte(base64.URLEncoding.EncodeToString([]byte("basic-password"))),
 									FromEnv: "SECRET_ENV",
 								},
 							},
@@ -461,7 +461,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",
 								},
 							},

pkg/sessions/redis/client_test.go

@@ -9,6 +9,7 @@ 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"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 )
@@ -26,7 +27,7 @@ var _ = Describe("Redis Client Tests", func() {
 		RunClientTests(func(mr *miniredis.Miniredis) options.RedisStoreOptions {
 			return options.RedisStoreOptions{
 				ClusterConnectionURLs: []string{"redis://" + mr.Addr()},
-				UseCluster:            true,
+				UseCluster:            ptr.To(true),
 			}
 		})
 	})

pkg/sessions/session_store_test.go

@@ -39,7 +39,7 @@ var _ = Describe("NewSessionStore", func() {
 		secret := make([]byte, 32)
 		_, err := rand.Read(secret)
 		Expect(err).ToNot(HaveOccurred())
-		var secretValue []byte
+		secretValue := make([]byte, base64.URLEncoding.EncodedLen(len(secret)))
 
 		base64.URLEncoding.Encode(secretValue, secret)
 		Expect(secretValue).ToNot(BeEmpty())
@@ -47,7 +47,7 @@ var _ = Describe("NewSessionStore", func() {
 		// Set default options in CookieOptions
 		cookieOpts = &options.Cookie{
 			Name: "_oauth2_proxy",
-			Secret: options.SecretSource{
+			Secret: &options.SecretSource{
 				Value: secretValue,
 			},
 			Path:         "/",

pkg/sessions/tests/session_store_tests.go

@@ -73,7 +73,7 @@ func RunSessionStoreTests(newSS NewSessionStoreFunc, persistentFastForward Persi
 				Insecure:     ptr.To(false),
 				ScriptAccess: options.ScriptAccessDenied,
 				SameSite:     options.SameSiteDefault,
-				Secret:       options.SecretSource{Value: cookieSecret},
+				Secret:       &options.SecretSource{Value: cookieSecret},
 			}
 
 			expires := time.Now().Add(1 * time.Hour)
@@ -124,7 +124,7 @@ func RunSessionStoreTests(newSS NewSessionStoreFunc, persistentFastForward Persi
 					ScriptAccess: options.ScriptAccessAllowed,
 					Domains:      []string{"example.com"},
 					SameSite:     options.SameSiteStrict,
-					Secret:       options.SecretSource{Value: cookieSecret},
+					Secret:       &options.SecretSource{Value: cookieSecret},
 				}
 
 				var err error
@@ -155,7 +155,7 @@ func RunSessionStoreTests(newSS NewSessionStoreFunc, persistentFastForward Persi
 					Insecure:     ptr.To(false),
 					ScriptAccess: options.ScriptAccessDenied,
 					SameSite:     options.SameSiteDefault,
-					Secret:       options.SecretSource{FromFile: tmpfile.Name()},
+					Secret:       &options.SecretSource{FromFile: tmpfile.Name()},
 				}
 				ss, err = newSS(input.sessionOpts, input.cookieOpts)
 				Expect(err).ToNot(HaveOccurred())

pkg/validation/cookie.go

@@ -49,12 +49,12 @@ func validateCookieName(name string) []string {
 	return msgs
 }
 
-func validateCookieSecret(secret options.SecretSource) []string {
-	if len(secret.Value) == 0 && secret.FromFile == "" {
+func validateCookieSecret(secret *options.SecretSource) []string {
+	if secret == nil || len(secret.Value) == 0 && secret.FromFile == "" {
 		return []string{"missing setting: cookie-secret or cookie-secret-file"}
 	}
 
-	value, err := secret.GetSecretValue()
+	value, err := secret.GetRawSecretValue()
 	if err != nil {
 		return []string{fmt.Sprintf("error retrieving cookie secret: %v", err)}
 	}

pkg/validation/cookie_test.go

@@ -18,11 +18,11 @@ 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 := options.SecretSource{
+	validSecret := &options.SecretSource{
 		Value: []byte("secretthirtytwobytes+abcdefghijk"),
 	}
 	// 6 bytes is not a valid size
-	invalidSecret := options.SecretSource{
+	invalidSecret := &options.SecretSource{
 		Value: []byte("abcdef"),
 	}
 
@@ -90,7 +90,7 @@ func TestValidateCookie(t *testing.T) {
 			name: "with no cookie secret",
 			cookie: options.Cookie{
 				Name: validName,
-				Secret: options.SecretSource{
+				Secret: &options.SecretSource{
 					Value:    nil,
 					FromFile: "",
 				},
@@ -127,7 +127,7 @@ func TestValidateCookie(t *testing.T) {
 			name: "with a valid Base64 secret",
 			cookie: options.Cookie{
 				Name:         validName,
-				Secret:       validBase64Secret,
+				Secret:       &validBase64Secret,
 				Domains:      emptyDomains,
 				Path:         "",
 				Expire:       time.Hour,
@@ -142,7 +142,7 @@ func TestValidateCookie(t *testing.T) {
 			name: "with an invalid Base64 secret",
 			cookie: options.Cookie{
 				Name:         validName,
-				Secret:       invalidBase64Secret,
+				Secret:       &invalidBase64Secret,
 				Domains:      emptyDomains,
 				Path:         "",
 				Expire:       time.Hour,
@@ -307,7 +307,7 @@ func TestValidateCookie(t *testing.T) {
 			name: "with valid secret file",
 			cookie: options.Cookie{
 				Name: validName,
-				Secret: options.SecretSource{
+				Secret: &options.SecretSource{
 					FromFile: tmpfile.Name(),
 				},
 				Domains:      domains,
@@ -324,7 +324,7 @@ func TestValidateCookie(t *testing.T) {
 			name: "with nonexistent secret file",
 			cookie: options.Cookie{
 				Name: validName,
-				Secret: options.SecretSource{
+				Secret: &options.SecretSource{
 					FromFile: "/nonexistent/file.txt",
 				},
 				Domains:      domains,
@@ -335,7 +335,7 @@ func TestValidateCookie(t *testing.T) {
 				SameSite:     "",
 			},
 			refresh:    0,
-			errStrings: []string{"could not read cookie secret file: /nonexistent/file.txt"},
+			errStrings: []string{"error retrieving cookie secret: error reading secret from file \"/nonexistent/file.txt\": open /nonexistent/file.txt: no such file or directory"},
 		},
 	}
 

pkg/validation/header_test.go

@@ -1,8 +1,6 @@
 package validation
 
 import (
-	"encoding/base64"
-
 	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
@@ -29,9 +27,7 @@ var _ = Describe("Headers", func() {
 		Name: "X-Forwarded-Auth",
 		Values: []options.HeaderValue{
 			{
-				SecretSource: &options.SecretSource{
-					Value: []byte(base64.RawStdEncoding.EncodeToString([]byte("secret"))),
-				},
+				SecretSource: options.NewSecretSourceFromString("secret"),
 			},
 		},
 	}
@@ -41,10 +37,8 @@ var _ = Describe("Headers", func() {
 		Values: []options.HeaderValue{
 			{
 				ClaimSource: &options.ClaimSource{
-					Claim: "email",
-					BasicAuthPassword: &options.SecretSource{
-						Value: []byte(base64.RawStdEncoding.EncodeToString([]byte("secret"))),
-					},
+					Claim:             "email",
+					BasicAuthPassword: options.NewSecretSourceFromString("secret"),
 				},
 			},
 		},

pkg/validation/options_test.go

@@ -20,7 +20,7 @@ const (
 )
 
 var (
-	cookieSecret = options.NewSecretSourceFromString("secretthirtytwobytes+abcdefghijk")
+	cookieSecret = &options.SecretSource{Value: []byte("secretthirtytwobytes+abcdefghijk")}
 )
 
 func testOptions() *options.Options {
@@ -128,7 +128,7 @@ func TestCookieRefreshMustBeLessThanCookieExpire(t *testing.T) {
 	o := testOptions()
 	assert.Equal(t, nil, Validate(o))
 
-	o.Cookie.Secret = options.NewSecretSourceFromString("0123456789abcdef")
+	o.Cookie.Secret = &options.SecretSource{Value: []byte("0123456789abcdef")}
 	o.Session.Refresh = o.Cookie.Expire
 	assert.NotEqual(t, nil, Validate(o))
 
@@ -141,23 +141,23 @@ func TestBase64CookieSecret(t *testing.T) {
 	assert.Equal(t, nil, Validate(o))
 
 	// 32 byte, base64 (urlsafe) encoded key
-	o.Cookie.Secret = options.NewSecretSourceFromString("yHBw2lh2Cvo6aI_jn_qMTr-pRAjtq0nzVgDJNb36jgQ=")
+	o.Cookie.Secret = &options.SecretSource{Value: []byte("yHBw2lh2Cvo6aI_jn_qMTr-pRAjtq0nzVgDJNb36jgQ=")}
 	assert.Equal(t, nil, Validate(o))
 
 	// 32 byte, base64 (urlsafe) encoded key, w/o padding
-	o.Cookie.Secret = options.NewSecretSourceFromString("yHBw2lh2Cvo6aI_jn_qMTr-pRAjtq0nzVgDJNb36jgQ")
+	o.Cookie.Secret = &options.SecretSource{Value: []byte("yHBw2lh2Cvo6aI_jn_qMTr-pRAjtq0nzVgDJNb36jgQ")}
 	assert.Equal(t, nil, Validate(o))
 
 	// 24 byte, base64 (urlsafe) encoded key
-	o.Cookie.Secret = options.NewSecretSourceFromString("Kp33Gj-GQmYtz4zZUyUDdqQKx5_Hgkv3")
+	o.Cookie.Secret = &options.SecretSource{Value: []byte("Kp33Gj-GQmYtz4zZUyUDdqQKx5_Hgkv3")}
 	assert.Equal(t, nil, Validate(o))
 
 	// 16 byte, base64 (urlsafe) encoded key
-	o.Cookie.Secret = options.NewSecretSourceFromString("LFEqZYvYUwKwzn0tEuTpLA==")
+	o.Cookie.Secret = &options.SecretSource{Value: []byte("LFEqZYvYUwKwzn0tEuTpLA==")}
 	assert.Equal(t, nil, Validate(o))
 
 	// 16 byte, base64 (urlsafe) encoded key, w/o padding
-	o.Cookie.Secret = options.NewSecretSourceFromString("LFEqZYvYUwKwzn0tEuTpLA")
+	o.Cookie.Secret = &options.SecretSource{Value: []byte("LFEqZYvYUwKwzn0tEuTpLA")}
 	assert.Equal(t, nil, Validate(o))
 }