diff --git a/CHANGELOG.md b/CHANGELOG.md index 01acb1b8..9fea0495 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,11 @@ - Multiple cookie domains may now be configured. The longest domain that matches will be used. - The config options `cookie_domain` is now `cookie_domains` - The environment variable `OAUTH2_PROXY_COOKIE_DOMAIN` is now `OAUTH2_PROXY_COOKIE_DOMAINS` +- [#414](https://github.com/oauth2-proxy/oauth2-proxy/pull/414) Always encrypt sessions regardless of config + - Previously, sessions were encrypted only when certain options were configured. + This lead to confusion and misconfiguration as it was not obvious when a session should be encrypted. + - Cookie Secrets must now be 16, 24 or 32 bytes. + - If you need to change your secret, this will force users to reauthenticate. ## Changes since v5.1.1 @@ -86,6 +91,7 @@ - [#488](https://github.com/oauth2-proxy/oauth2-proxy/pull/488) Set-Basic-Auth should default to false (@JoelSpeed) - [#494](https://github.com/oauth2-proxy/oauth2-proxy/pull/494) Upstream websockets TLS certificate validation now depends on ssl-upstream-insecure-skip-verify (@yaroslavros) - [#497](https://github.com/oauth2-proxy/oauth2-proxy/pull/497) Restrict access using Github collaborators (@jsclayton) +- [#414](https://github.com/oauth2-proxy/oauth2-proxy/pull/414) Always encrypt sessions regardless of config (@ti-mo) # v5.1.1 diff --git a/oauthproxy_test.go b/oauthproxy_test.go index 20d59f8d..845dcef1 100644 --- a/oauthproxy_test.go +++ b/oauthproxy_test.go @@ -29,6 +29,13 @@ import ( "golang.org/x/net/websocket" ) +const ( + // The rawCookieSecret is 32 bytes and the base64CookieSecret is the base64 + // encoded version of this. + rawCookieSecret = "secretthirtytwobytes+abcdefghijk" + base64CookieSecret = "c2VjcmV0dGhpcnR5dHdvYnl0ZXMrYWJjZGVmZ2hpamsK" +) + func init() { logger.SetFlags(logger.Lshortfile) @@ -166,7 +173,7 @@ func TestRobotsTxt(t *testing.T) { opts := options.NewOptions() opts.ClientID = "asdlkjx" opts.ClientSecret = "alkgks" - opts.Cookie.Secret = "asdkugkj" + opts.Cookie.Secret = rawCookieSecret validation.Validate(opts) proxy := NewOAuthProxy(opts, func(string) bool { return true }) @@ -181,7 +188,7 @@ func TestIsValidRedirect(t *testing.T) { opts := options.NewOptions() opts.ClientID = "skdlfj" opts.ClientSecret = "fgkdsgj" - opts.Cookie.Secret = "ljgiogbj" + opts.Cookie.Secret = base64CookieSecret // Should match domains that are exactly foo.bar and any subdomain of bar.foo opts.WhitelistDomains = []string{ "foo.bar", @@ -794,7 +801,7 @@ func NewSignInPageTest(skipProvider bool) *SignInPageTest { var sipTest SignInPageTest sipTest.opts = options.NewOptions() - sipTest.opts.Cookie.Secret = "adklsj2" + sipTest.opts.Cookie.Secret = rawCookieSecret sipTest.opts.ClientID = "lkdgj" sipTest.opts.ClientSecret = "sgiufgoi" sipTest.opts.SkipProviderButton = skipProvider @@ -1208,7 +1215,7 @@ func TestAuthSkippedForPreflightRequests(t *testing.T) { opts.Upstreams = append(opts.Upstreams, upstream.URL) opts.ClientID = "aljsal" opts.ClientSecret = "jglkfsdgj" - opts.Cookie.Secret = "dkfjgdls" + opts.Cookie.Secret = base64CookieSecret opts.SkipAuthPreflight = true validation.Validate(opts) @@ -1255,7 +1262,7 @@ type SignatureTest struct { func NewSignatureTest() *SignatureTest { opts := options.NewOptions() - opts.Cookie.Secret = "cookie secret" + opts.Cookie.Secret = rawCookieSecret opts.ClientID = "client ID" opts.ClientSecret = "client secret" opts.EmailDomains = []string{"acm.org"} @@ -1402,7 +1409,7 @@ type ajaxRequestTest struct { func newAjaxRequestTest() *ajaxRequestTest { test := &ajaxRequestTest{} test.opts = options.NewOptions() - test.opts.Cookie.Secret = "sdflsw" + test.opts.Cookie.Secret = base64CookieSecret test.opts.ClientID = "gkljfdl" test.opts.ClientSecret = "sdflkjs" validation.Validate(test.opts) diff --git a/pkg/validation/options.go b/pkg/validation/options.go index cd4aa0ca..761d216d 100644 --- a/pkg/validation/options.go +++ b/pkg/validation/options.go @@ -12,7 +12,6 @@ import ( "regexp" "sort" "strings" - "time" "github.com/coreos/go-oidc" "github.com/dgrijalva/jwt-go" @@ -39,9 +38,38 @@ func Validate(o *options.Options) error { } msgs := make([]string, 0) + + var cipher *encryption.Cipher if o.Cookie.Secret == "" { msgs = append(msgs, "missing setting: cookie-secret") + } else { + validCookieSecretSize := false + for _, i := range []int{16, 24, 32} { + if len(encryption.SecretBytes(o.Cookie.Secret)) == i { + validCookieSecretSize = true + } + } + var decoded bool + if string(encryption.SecretBytes(o.Cookie.Secret)) != o.Cookie.Secret { + decoded = true + } + if !validCookieSecretSize { + var suffix string + if decoded { + suffix = " note: cookie secret was base64 decoded" + } + msgs = append(msgs, + fmt.Sprintf("Cookie secret must be 16, 24, or 32 bytes to create an AES cipher. Got %d bytes.%s", + len(encryption.SecretBytes(o.Cookie.Secret)), suffix)) + } else { + var err error + cipher, err = encryption.NewCipher(encryption.SecretBytes(o.Cookie.Secret)) + if err != nil { + msgs = append(msgs, fmt.Sprintf("cookie-secret error: %v", err)) + } + } } + if o.ClientID == "" { msgs = append(msgs, "missing setting: client-id") } @@ -195,38 +223,6 @@ func Validate(o *options.Options) error { } msgs = parseProviderInfo(o, msgs) - var cipher *encryption.Cipher - if o.PassAccessToken || o.SetAuthorization || o.PassAuthorization || (o.Cookie.Refresh != time.Duration(0)) { - validCookieSecretSize := false - for _, i := range []int{16, 24, 32} { - if len(encryption.SecretBytes(o.Cookie.Secret)) == i { - validCookieSecretSize = true - } - } - var decoded bool - if string(encryption.SecretBytes(o.Cookie.Secret)) != o.Cookie.Secret { - decoded = true - } - if !validCookieSecretSize { - var suffix string - if decoded { - suffix = fmt.Sprintf(" note: cookie secret was base64 decoded from %q", o.Cookie.Secret) - } - msgs = append(msgs, fmt.Sprintf( - "cookie_secret must be 16, 24, or 32 bytes "+ - "to create an AES cipher when "+ - "pass_access_token == true or "+ - "cookie_refresh != 0, but is %d bytes.%s", - len(encryption.SecretBytes(o.Cookie.Secret)), suffix)) - } else { - var err error - cipher, err = encryption.NewCipher(encryption.SecretBytes(o.Cookie.Secret)) - if err != nil { - msgs = append(msgs, fmt.Sprintf("cookie-secret error: %v", err)) - } - } - } - o.Session.Cipher = cipher sessionStore, err := sessions.NewSessionStore(&o.Session, &o.Cookie) if err != nil { diff --git a/pkg/validation/options_test.go b/pkg/validation/options_test.go index c7449172..6abd13db 100644 --- a/pkg/validation/options_test.go +++ b/pkg/validation/options_test.go @@ -15,7 +15,7 @@ import ( ) const ( - cookieSecret = "foobar" + cookieSecret = "secretthirtytwobytes+abcdefghijk" clientID = "bazquux" clientSecret = "xyzzyplugh" )