Repository: oauth2-proxy/oauth2-proxy
Pull request #673 (2020-07-11 02:16:43)

Add option to remove tokens from cookie sessions
Before v6.0.0, there were option scenarios that caused a cipher not to be set (even if a secret was present). This resulted in a version of the SessionState code that ignored the 3 large token fields.

Removing this has resulted in several users having issues with newfound cookie bloat now that their cookies have the 3x oauth tokens.

https://github.com/oauth2-proxy/oauth2-proxy/issues/644

## Description

Adds an option `--cookie-minimal` to make a minimal encoded session for cookie session stores that omits the tokens. It will not pass validation if it is set with conflicting options that require a one of the 3 tokens.

## Motivation and Context

Reduce session cookie bloat for users that are running into header/cookie size issues in their architecture.

## How Has This Been Tested?

Unit tests + interactive tests

## Checklist:

- [x] My change requires a change to the documentation or CHANGELOG.
- [x] I have updated the documentation/CHANGELOG accordingly.
- [x] I have created a feature (non-master) branch for my PR.


JoelSpeed: A couple of conflicts now I've merged the compression PR, once they're fixed I think we are good to go
NickMeves: Gotta love those CHANGELOG conflicts when rebasing 😄 

All set @JoelSpeed 
roman-vynar: Cookie size:
* Before 6.0.0: 160 bytes
* Now like 2800

IMHO, cookie-minimal should be enabled by default once added.
JoelSpeed: > IMHO, cookie-minimal should be enabled by default once added.

Unfortunately I don't think we can make this deafult yet. It would be a major breaking change for a number of people. Anyone who was relying on having the tokens in the session would be broken if we made this change, so for now I'd like to keep it optional.

When we do make the next breaking release, I think we should review this decision. Though by that time I hope we have done some of the session refactoring that we have been planning to allow the claims to be configured by the user anyway, so this flag will likely dissapear.

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index b78a44b1..bc58b625 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -11,6 +11,7 @@
 
 ## Changes since v6.0.0
 
+- [#673](https://github.com/oauth2-proxy/oauth2-proxy/pull/673) Add --session-cookie-minimal option to create session cookies with no tokens (@NickMeves)
 - [#632](https://github.com/oauth2-proxy/oauth2-proxy/pull/632) Reduce session size by encoding with MessagePack and using LZ4 compression (@NickMeves)
 - [#675](https://github.com/oauth2-proxy/oauth2-proxy/pull/675) Fix required ruby version and deprecated option for building docs (@mkontani)
 - [#669](https://github.com/oauth2-proxy/oauth2-proxy/pull/669) Reduce docker context to improve build times (@JoelSpeed)
diff --git a/docs/configuration/configuration.md b/docs/configuration/configuration.md
index 4642e6fa..6bf5c4f5 100644
--- a/docs/configuration/configuration.md
+++ b/docs/configuration/configuration.md
@@ -107,6 +107,7 @@ An example [oauth2-proxy.cfg]({{ site.gitweb }}/contrib/oauth2-proxy.cfg.example
 | `--resource` | string | The resource that is protected (Azure AD only) | |
 | `--reverse-proxy` | bool | are we running behind a reverse proxy, controls whether headers like X-Real-Ip are accepted | false |
 | `--scope` | string | OAuth scope specification | |
+| `--session-cookie-minimal` | bool | strip OAuth tokens from cookie session stores if they aren't needed (cookie session store only) | false |
 | `--session-store-type` | string | [Session data storage backend](configuration/sessions); redis or cookie | cookie |
 | `--set-xauthrequest` | bool | set X-Auth-Request-User, X-Auth-Request-Email and X-Auth-Request-Preferred-Username response headers (useful in Nginx auth_request mode) | false |
 | `--set-authorization-header` | bool | set Authorization Bearer response header (useful in Nginx auth_request mode) | false |
diff --git a/pkg/apis/options/options.go b/pkg/apis/options/options.go
index 4dd7f1d2..8b9f84b5 100644
--- a/pkg/apis/options/options.go
+++ b/pkg/apis/options/options.go
@@ -145,19 +145,17 @@ func (o *Options) SetRealClientIPParser(s ipapi.RealClientIPParser) { o.realClie
 // NewOptions constructs a new Options with defaulted values
 func NewOptions() *Options {
 	return &Options{
-		ProxyPrefix:         "/oauth2",
-		ProviderType:        "google",
-		PingPath:            "/ping",
-		ProxyWebSockets:     true,
-		HTTPAddress:         "127.0.0.1:4180",
-		HTTPSAddress:        ":443",
-		RealClientIPHeader:  "X-Real-IP",
-		ForceHTTPS:          false,
-		DisplayHtpasswdForm: true,
-		Cookie:              cookieDefaults(),
-		Session: SessionOptions{
-			Type: "cookie",
-		},
+		ProxyPrefix:                      "/oauth2",
+		ProviderType:                     "google",
+		PingPath:                         "/ping",
+		ProxyWebSockets:                  true,
+		HTTPAddress:                      "127.0.0.1:4180",
+		HTTPSAddress:                     ":443",
+		RealClientIPHeader:               "X-Real-IP",
+		ForceHTTPS:                       false,
+		DisplayHtpasswdForm:              true,
+		Cookie:                           cookieDefaults(),
+		Session:                          sessionOptionsDefaults(),
 		AzureTenant:                      "common",
 		SetXAuthRequest:                  false,
 		SkipAuthPreflight:                false,
@@ -241,6 +239,7 @@ func NewFlagSet() *pflag.FlagSet {
 	flagSet.String("ping-user-agent", "", "special User-Agent that will be used for basic health checks")
 	flagSet.Bool("proxy-websockets", true, "enables WebSocket proxying")
 	flagSet.String("session-store-type", "cookie", "the session storage provider to use")
+	flagSet.Bool("session-cookie-minimal", false, "strip OAuth tokens from cookie session stores if they aren't needed (cookie session store only)")
 	flagSet.String("redis-connection-url", "", "URL of redis server for redis session storage (eg: redis://HOST[:PORT])")
 	flagSet.Bool("redis-use-sentinel", false, "Connect to redis via sentinels. Must set --redis-sentinel-master-name and --redis-sentinel-connection-urls to use this feature")
 	flagSet.String("redis-sentinel-master-name", "", "Redis sentinel master name. Used in conjunction with --redis-use-sentinel")
diff --git a/pkg/apis/options/sessions.go b/pkg/apis/options/sessions.go
index 7ff656fe..70482ee5 100644
--- a/pkg/apis/options/sessions.go
+++ b/pkg/apis/options/sessions.go
@@ -2,8 +2,9 @@ package options
 
 // SessionOptions contains configuration options for the SessionStore providers.
 type SessionOptions struct {
-	Type  string            `flag:"session-store-type" cfg:"session_store_type"`
-	Redis RedisStoreOptions `cfg:",squash"`
+	Type   string             `flag:"session-store-type" cfg:"session_store_type"`
+	Cookie CookieStoreOptions `cfg:",squash"`
+	Redis  RedisStoreOptions  `cfg:",squash"`
 }
 
 // CookieSessionStoreType is used to indicate the CookieSessionStore should be
@@ -14,6 +15,11 @@ var CookieSessionStoreType = "cookie"
 // used for storing sessions.
 var RedisSessionStoreType = "redis"
 
+// CookieStoreOptions contains configuration options for the CookieSessionStore.
+type CookieStoreOptions struct {
+	Minimal bool `flag:"session-cookie-minimal" cfg:"session_cookie_minimal"`
+}
+
 // RedisStoreOptions contains configuration options for the RedisSessionStore.
 type RedisStoreOptions struct {
 	ConnectionURL          string   `flag:"redis-connection-url" cfg:"redis_connection_url"`
@@ -25,3 +31,12 @@ type RedisStoreOptions struct {
 	CAPath                 string   `flag:"redis-ca-path" cfg:"redis_ca_path"`
 	InsecureSkipTLSVerify  bool     `flag:"redis-insecure-skip-tls-verify" cfg:"redis_insecure_skip_tls_verify"`
 }
+
+func sessionOptionsDefaults() SessionOptions {
+	return SessionOptions{
+		Type: CookieSessionStoreType,
+		Cookie: CookieStoreOptions{
+			Minimal: false,
+		},
+	}
+}
diff --git a/pkg/sessions/cookie/session_store.go b/pkg/sessions/cookie/session_store.go
index 69b55d11..18717ed1 100644
--- a/pkg/sessions/cookie/session_store.go
+++ b/pkg/sessions/cookie/session_store.go
@@ -30,6 +30,7 @@ var _ sessions.SessionStore = &SessionStore{}
 type SessionStore struct {
 	Cookie       *options.Cookie
 	CookieCipher encryption.Cipher
+	Minimal      bool
 }
 
 // Save takes a sessions.SessionState and stores the information from it
@@ -39,7 +40,7 @@ func (s *SessionStore) Save(rw http.ResponseWriter, req *http.Request, ss *sessi
 		now := time.Now()
 		ss.CreatedAt = &now
 	}
-	value, err := cookieForSession(ss, s.CookieCipher)
+	value, err := s.cookieForSession(ss)
 	if err != nil {
 		return err
 	}
@@ -85,8 +86,17 @@ func (s *SessionStore) Clear(rw http.ResponseWriter, req *http.Request) error {
 }
 
 // cookieForSession serializes a session state for storage in a cookie
-func cookieForSession(s *sessions.SessionState, c encryption.Cipher) ([]byte, error) {
-	return s.EncodeSessionState(c, true)
+func (s *SessionStore) cookieForSession(ss *sessions.SessionState) ([]byte, error) {
+	if s.Minimal && (ss.AccessToken != "" || ss.IDToken != "" || ss.RefreshToken != "") {
+		minimal := *ss
+		minimal.AccessToken = ""
+		minimal.IDToken = ""
+		minimal.RefreshToken = ""
+
+		return minimal.EncodeSessionState(s.CookieCipher, true)
+	}
+
+	return ss.EncodeSessionState(s.CookieCipher, true)
 }
 
 // sessionFromCookie deserializes a session from a cookie value
@@ -146,6 +156,7 @@ func NewCookieSessionStore(opts *options.SessionOptions, cookieOpts *options.Coo
 	return &SessionStore{
 		CookieCipher: cipher,
 		Cookie:       cookieOpts,
+		Minimal:      opts.Cookie.Minimal,
 	}, nil
 }
 
diff --git a/pkg/validation/options.go b/pkg/validation/options.go
index cefbb31a..d0f4ba06 100644
--- a/pkg/validation/options.go
+++ b/pkg/validation/options.go
@@ -27,6 +27,7 @@ import (
 // are of the correct format
 func Validate(o *options.Options) error {
 	msgs := validateCookie(o.Cookie)
+	msgs = append(msgs, validateSessionCookieMinimal(o)...)
 
 	if o.SSLInsecureSkipVerify {
 		insecureTransport := &http.Transport{
diff --git a/pkg/validation/sessions.go b/pkg/validation/sessions.go
new file mode 100644
index 00000000..db4ba9fd
--- /dev/null
+++ b/pkg/validation/sessions.go
@@ -0,0 +1,32 @@
+package validation
+
+import (
+	"time"
+
+	"github.com/oauth2-proxy/oauth2-proxy/pkg/apis/options"
+)
+
+func validateSessionCookieMinimal(o *options.Options) []string {
+	if !o.Session.Cookie.Minimal {
+		return []string{}
+	}
+
+	msgs := []string{}
+	if o.PassAuthorization {
+		msgs = append(msgs,
+			"pass_authorization_header requires oauth tokens in sessions. session_cookie_minimal cannot be set")
+	}
+	if o.SetAuthorization {
+		msgs = append(msgs,
+			"set_authorization_header requires oauth tokens in sessions. session_cookie_minimal cannot be set")
+	}
+	if o.PassAccessToken {
+		msgs = append(msgs,
+			"pass_access_token requires oauth tokens in sessions. session_cookie_minimal cannot be set")
+	}
+	if o.Cookie.Refresh != time.Duration(0) {
+		msgs = append(msgs,
+			"cookie_refresh > 0 requires oauth tokens in sessions. session_cookie_minimal cannot be set")
+	}
+	return msgs
+}


Test patch:
diff --git a/pkg/validation/sessions_test.go b/pkg/validation/sessions_test.go
new file mode 100644
index 00000000..a6ffdbd0
--- /dev/null
+++ b/pkg/validation/sessions_test.go
@@ -0,0 +1,121 @@
+package validation
+
+import (
+	"testing"
+	"time"
+
+	"github.com/oauth2-proxy/oauth2-proxy/pkg/apis/options"
+	. "github.com/onsi/gomega"
+)
+
+func Test_validateSessionCookieMinimal(t *testing.T) {
+	const (
+		passAuthorizationMsg = "pass_authorization_header requires oauth tokens in sessions. session_cookie_minimal cannot be set"
+		setAuthorizationMsg  = "set_authorization_header requires oauth tokens in sessions. session_cookie_minimal cannot be set"
+		passAccessTokenMsg   = "pass_access_token requires oauth tokens in sessions. session_cookie_minimal cannot be set"
+		cookieRefreshMsg     = "cookie_refresh > 0 requires oauth tokens in sessions. session_cookie_minimal cannot be set"
+	)
+
+	testCases := map[string]struct {
+		opts       *options.Options
+		errStrings []string
+	}{
+		"No minimal cookie session": {
+			opts: &options.Options{
+				Session: options.SessionOptions{
+					Cookie: options.CookieStoreOptions{
+						Minimal: false,
+					},
+				},
+			},
+			errStrings: []string{},
+		},
+		"No minimal cookie session & passAuthorization": {
+			opts: &options.Options{
+				Session: options.SessionOptions{
+					Cookie: options.CookieStoreOptions{
+						Minimal: false,
+					},
+				},
+				PassAuthorization: true,
+			},
+			errStrings: []string{},
+		},
+		"Minimal cookie session no conflicts": {
+			opts: &options.Options{
+				Session: options.SessionOptions{
+					Cookie: options.CookieStoreOptions{
+						Minimal: true,
+					},
+				},
+			},
+			errStrings: []string{},
+		},
+		"PassAuthorization conflict": {
+			opts: &options.Options{
+				Session: options.SessionOptions{
+					Cookie: options.CookieStoreOptions{
+						Minimal: true,
+					},
+				},
+				PassAuthorization: true,
+			},
+			errStrings: []string{passAuthorizationMsg},
+		},
+		"SetAuthorization conflict": {
+			opts: &options.Options{
+				Session: options.SessionOptions{
+					Cookie: options.CookieStoreOptions{
+						Minimal: true,
+					},
+				},
+				SetAuthorization: true,
+			},
+			errStrings: []string{setAuthorizationMsg},
+		},
+		"PassAccessToken conflict": {
+			opts: &options.Options{
+				Session: options.SessionOptions{
+					Cookie: options.CookieStoreOptions{
+						Minimal: true,
+					},
+				},
+				PassAccessToken: true,
+			},
+			errStrings: []string{passAccessTokenMsg},
+		},
+		"CookieRefresh conflict": {
+			opts: &options.Options{
+				Cookie: options.Cookie{
+					Refresh: time.Hour,
+				},
+				Session: options.SessionOptions{
+					Cookie: options.CookieStoreOptions{
+						Minimal: true,
+					},
+				},
+			},
+			errStrings: []string{cookieRefreshMsg},
+		},
+		"Multiple conflicts": {
+			opts: &options.Options{
+				Session: options.SessionOptions{
+					Cookie: options.CookieStoreOptions{
+						Minimal: true,
+					},
+				},
+				PassAuthorization: true,
+				PassAccessToken:   true,
+			},
+			errStrings: []string{passAuthorizationMsg, passAccessTokenMsg},
+		},
+	}
+
+	for testName, tc := range testCases {
+		t.Run(testName, func(t *testing.T) {
+			errStrings := validateSessionCookieMinimal(tc.opts)
+			g := NewWithT(t)
+			g.Expect(errStrings).To(ConsistOf(tc.errStrings))
+		})
+	}
+}