Merge pull request oauth2-proxy#414 from ti-mo/cookie-secret-cipher-x…

…authrequest Always encrypt sessions regardless of configuration
weepee-org · May 24, 2020 · 03a0e1a · 03a0e1a
2 parents 0c9795a + 276d1c6
commit 03a0e1a
Show file tree

Hide file tree

Showing 4 changed files with 49 additions and 40 deletions.
diff --git 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
@@ -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
@@ -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
@@ -15,7 +15,7 @@ import (
 )
 
 const (
-	cookieSecret = "foobar"
+	cookieSecret = "secretthirtytwobytes+abcdefghijk"
 	clientID     = "bazquux"
 	clientSecret = "xyzzyplugh"
 )