git.iko.soy/typeable/oauth2-proxy
on master help sign in
Merge pull request #576 from oauth2-proxy/cookie-validation

Separate Cookie validation

commitfb1bef27573a9ce5d16ade661cd4ad8d77e93fcb authorJoel Speed <Joel.speed@hotmail.co.uk> dateSun 5 Jul 2020, 09:35 (6 years ago) parent016f4aa3e13f31
15 files changed+434−143
CHANGELOG.md+1 pkg/apis/options/cookie.go+38−3 pkg/apis/options/options.go+3−20 pkg/cookies/cookies.go+1−1 pkg/sessions/cookie/session_store.go+12−12 pkg/sessions/cookie/session_store_test.go+1−1 pkg/sessions/redis/redis_store.go+25−25 pkg/sessions/redis/session_store_test.go+3−3 pkg/sessions/session_store.go+1−1 pkg/sessions/session_store_test.go+2−2 pkg/sessions/tests/session_store_tests.go+4−4 pkg/validation/cookie.go+68 pkg/validation/cookie_test.go+273 pkg/validation/options.go+2−56 pkg/validation/options_test.go−15
CHANGELOG.md+1
@@ -8,6 +8,7 @@
88
99## Changes since v6.0.0
1010
11+- [#576](https://github.com/oauth2-proxy/oauth2-proxy/pull/576) Separate Cookie validation out of main options validation (@JoelSpeed)
1112- [#656](https://github.com/oauth2-proxy/oauth2-proxy/pull/656) Split long session cookies more precisely (@NickMeves)
1213- [#619](https://github.com/oauth2-proxy/oauth2-proxy/pull/619) Improve Redirect to HTTPs behaviour (@JoelSpeed)
1314- [#654](https://github.com/oauth2-proxy/oauth2-proxy/pull/654) Close client connections after each redis test (@JoelSpeed)
pkg/apis/options/cookie.go+38−3
@@ -1,9 +1,13 @@
11package options
22
3−import "time"
3+import (
4+ "time"
45
5−// CookieOptions contains configuration options relating to Cookie configuration
6−type CookieOptions struct {
6+ "github.com/spf13/pflag"
7+)
8+
9+// Cookie contains configuration options relating to Cookie configuration
10+type Cookie struct {
711 Name string `flag:"cookie-name" cfg:"cookie_name"`
812 Secret string `flag:"cookie-secret" cfg:"cookie_secret"`
913 Domains []string `flag:"cookie-domain" cfg:"cookie_domains"`
@@ -14,3 +18,34 @@ type CookieOptions struct {
1418 HTTPOnly bool `flag:"cookie-httponly" cfg:"cookie_httponly"`
1519 SameSite string `flag:"cookie-samesite" cfg:"cookie_samesite"`
1620}
21+
22+func cookieFlagSet() *pflag.FlagSet {
23+ flagSet := pflag.NewFlagSet("cookie", pflag.ExitOnError)
24+
25+ flagSet.String("cookie-name", "_oauth2_proxy", "the name of the cookie that the oauth_proxy creates")
26+ flagSet.String("cookie-secret", "", "the seed string for secure cookies (optionally base64 encoded)")
27+ 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).")
28+ flagSet.String("cookie-path", "/", "an optional cookie path to force cookies to (ie: /poc/)*")
29+ flagSet.Duration("cookie-expire", time.Duration(168)*time.Hour, "expire timeframe for cookie")
30+ flagSet.Duration("cookie-refresh", time.Duration(0), "refresh the cookie after this duration; 0 to disable")
31+ flagSet.Bool("cookie-secure", true, "set secure (HTTPS) cookie flag")
32+ flagSet.Bool("cookie-httponly", true, "set HttpOnly cookie flag")
33+ flagSet.String("cookie-samesite", "", "set SameSite cookie attribute (ie: \"lax\", \"strict\", \"none\", or \"\"). ")
34+
35+ return flagSet
36+}
37+
38+// cookieDefaults creates a Cookie populating each field with its default value
39+func cookieDefaults() Cookie {
40+ return Cookie{
41+ Name: "_oauth2_proxy",
42+ Secret: "",
43+ Domains: nil,
44+ Path: "/",
45+ Expire: time.Duration(168) * time.Hour,
46+ Refresh: time.Duration(0),
47+ Secure: true,
48+ HTTPOnly: true,
49+ SameSite: "",
50+ }
51+}
pkg/apis/options/options.go+3−20
@@ -59,7 +59,7 @@ type Options struct {
5959 Banner string `flag:"banner" cfg:"banner"`
6060 Footer string `flag:"footer" cfg:"footer"`
6161
62− Cookie CookieOptions `cfg:",squash"`
62+ Cookie Cookie `cfg:",squash"`
6363 Session SessionOptions `cfg:",squash"`
6464 Logging Logging `cfg:",squash"`
6565
@@ -153,14 +153,7 @@ func NewOptions() *Options {
153153 RealClientIPHeader: "X-Real-IP",
154154 ForceHTTPS: false,
155155 DisplayHtpasswdForm: true,
156− Cookie: CookieOptions{
157− Name: "_oauth2_proxy",
158− Secure: true,
159− HTTPOnly: true,
160− Expire: time.Duration(168) * time.Hour,
161− Refresh: time.Duration(0),
162− Path: "/",
163− },
156+ Cookie: cookieDefaults(),
164157 Session: SessionOptions{
165158 Type: "cookie",
166159 },
@@ -245,17 +238,6 @@ func NewFlagSet() *pflag.FlagSet {
245238 flagSet.String("ping-path", "/ping", "the ping endpoint that can be used for basic health checks")
246239 flagSet.String("ping-user-agent", "", "special User-Agent that will be used for basic health checks")
247240 flagSet.Bool("proxy-websockets", true, "enables WebSocket proxying")
248−
249− flagSet.String("cookie-name", "_oauth2_proxy", "the name of the cookie that the oauth_proxy creates")
250− flagSet.String("cookie-secret", "", "the seed string for secure cookies (optionally base64 encoded)")
251− 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).")
252− flagSet.String("cookie-path", "/", "an optional cookie path to force cookies to (ie: /poc/)*")
253− flagSet.Duration("cookie-expire", time.Duration(168)*time.Hour, "expire timeframe for cookie")
254− flagSet.Duration("cookie-refresh", time.Duration(0), "refresh the cookie after this duration; 0 to disable")
255− flagSet.Bool("cookie-secure", true, "set secure (HTTPS) cookie flag")
256− flagSet.Bool("cookie-httponly", true, "set HttpOnly cookie flag")
257− flagSet.String("cookie-samesite", "", "set SameSite cookie attribute (ie: \"lax\", \"strict\", \"none\", or \"\"). ")
258−
259241 flagSet.String("session-store-type", "cookie", "the session storage provider to use")
260242 flagSet.String("redis-connection-url", "", "URL of redis server for redis session storage (eg: redis://HOST[:PORT])")
261243 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")
@@ -292,6 +274,7 @@ func NewFlagSet() *pflag.FlagSet {
292274
293275 flagSet.String("user-id-claim", "email", "which claim contains the user ID")
294276
277+ flagSet.AddFlagSet(cookieFlagSet())
295278 flagSet.AddFlagSet(loggingFlagSet())
296279
297280 return flagSet
pkg/cookies/cookies.go+1−1
@@ -38,7 +38,7 @@ func MakeCookie(req *http.Request, name string, value string, path string, domai
3838
3939// MakeCookieFromOptions constructs a cookie based on the given *options.CookieOptions,
4040// value and creation time
41−func MakeCookieFromOptions(req *http.Request, name string, value string, cookieOpts *options.CookieOptions, expiration time.Duration, now time.Time) *http.Cookie {
41+func MakeCookieFromOptions(req *http.Request, name string, value string, cookieOpts *options.Cookie, expiration time.Duration, now time.Time) *http.Cookie {
4242 domain := GetCookieDomain(req, cookieOpts.Domains)
4343
4444 if domain != "" {
pkg/sessions/cookie/session_store.go+12−12
@@ -28,8 +28,8 @@ var _ sessions.SessionStore = &SessionStore{}
2828// SessionStore is an implementation of the sessions.SessionStore
2929// interface that stores sessions in client side cookies
3030type SessionStore struct {
31− CookieOptions *options.CookieOptions
32− CookieCipher encryption.Cipher
31+ Cookie *options.Cookie
32+ CookieCipher encryption.Cipher
3333}
3434
3535// Save takes a sessions.SessionState and stores the information from it
@@ -50,12 +50,12 @@ func (s *SessionStore) Save(rw http.ResponseWriter, req *http.Request, ss *sessi
5050// Load reads sessions.SessionState information from Cookies within the
5151// HTTP request object
5252func (s *SessionStore) Load(req *http.Request) (*sessions.SessionState, error) {
53− c, err := loadCookie(req, s.CookieOptions.Name)
53+ c, err := loadCookie(req, s.Cookie.Name)
5454 if err != nil {
5555 // always http.ErrNoCookie
56− return nil, fmt.Errorf("cookie %q not present", s.CookieOptions.Name)
56+ return nil, fmt.Errorf("cookie %q not present", s.Cookie.Name)
5757 }
58− val, _, ok := encryption.Validate(c, s.CookieOptions.Secret, s.CookieOptions.Expire)
58+ val, _, ok := encryption.Validate(c, s.Cookie.Secret, s.Cookie.Expire)
5959 if !ok {
6060 return nil, errors.New("cookie signature not valid")
6161 }
@@ -71,7 +71,7 @@ func (s *SessionStore) Load(req *http.Request) (*sessions.SessionState, error) {
7171// clear the session
7272func (s *SessionStore) Clear(rw http.ResponseWriter, req *http.Request) error {
7373 // matches CookieName, CookieName_<number>
74− var cookieNameRegex = regexp.MustCompile(fmt.Sprintf("^%s(_\\d+)?$", s.CookieOptions.Name))
74+ var cookieNameRegex = regexp.MustCompile(fmt.Sprintf("^%s(_\\d+)?$", s.Cookie.Name))
7575
7676 for _, c := range req.Cookies() {
7777 if cookieNameRegex.MatchString(c.Name) {
@@ -105,9 +105,9 @@ func (s *SessionStore) setSessionCookie(rw http.ResponseWriter, req *http.Reques
105105// authentication details
106106func (s *SessionStore) makeSessionCookie(req *http.Request, value string, now time.Time) []*http.Cookie {
107107 if value != "" {
108− value = encryption.SignedValue(s.CookieOptions.Secret, s.CookieOptions.Name, []byte(value), now)
108+ value = encryption.SignedValue(s.Cookie.Secret, s.Cookie.Name, []byte(value), now)
109109 }
110− c := s.makeCookie(req, s.CookieOptions.Name, value, s.CookieOptions.Expire, now)
110+ c := s.makeCookie(req, s.Cookie.Name, value, s.Cookie.Expire, now)
111111
112112 if len(c.String()) > maxCookieLength {
113113 return splitCookie(c)
@@ -120,7 +120,7 @@ func (s *SessionStore) makeCookie(req *http.Request, name string, value string,
120120 req,
121121 name,
122122 value,
123− s.CookieOptions,
123+ s.Cookie,
124124 expiration,
125125 now,
126126 )
@@ -128,15 +128,15 @@ func (s *SessionStore) makeCookie(req *http.Request, name string, value string,
128128
129129// NewCookieSessionStore initialises a new instance of the SessionStore from
130130// the configuration given
131−func NewCookieSessionStore(opts *options.SessionOptions, cookieOpts *options.CookieOptions) (sessions.SessionStore, error) {
131+func NewCookieSessionStore(opts *options.SessionOptions, cookieOpts *options.Cookie) (sessions.SessionStore, error) {
132132 cipher, err := encryption.NewBase64Cipher(encryption.NewCFBCipher, encryption.SecretBytes(cookieOpts.Secret))
133133 if err != nil {
134134 return nil, fmt.Errorf("error initialising cipher: %v", err)
135135 }
136136
137137 return &SessionStore{
138− CookieCipher: cipher,
139− CookieOptions: cookieOpts,
138+ CookieCipher: cipher,
139+ Cookie: cookieOpts,
140140 }, nil
141141}
142142
pkg/sessions/cookie/session_store_test.go+1−1
@@ -25,7 +25,7 @@ func TestSessionStore(t *testing.T) {
2525
2626var _ = Describe("Cookie SessionStore Tests", func() {
2727 tests.RunSessionStoreTests(
28− func(opts *options.SessionOptions, cookieOpts *options.CookieOptions) (sessionsapi.SessionStore, error) {
28+ func(opts *options.SessionOptions, cookieOpts *options.Cookie) (sessionsapi.SessionStore, error) {
2929 // Set the connection URL
3030 opts.Type = options.CookieSessionStoreType
3131 return NewCookieSessionStore(opts, cookieOpts)
pkg/sessions/redis/redis_store.go+25−25
@@ -32,14 +32,14 @@ type TicketData struct {
3232// SessionStore is an implementation of the sessions.SessionStore
3333// interface that stores sessions in redis
3434type SessionStore struct {
35− CookieCipher encryption.Cipher
36− CookieOptions *options.CookieOptions
37− Client Client
35+ CookieCipher encryption.Cipher
36+ Cookie *options.Cookie
37+ Client Client
3838}
3939
4040// NewRedisSessionStore initialises a new instance of the SessionStore from
4141// the configuration given
42−func NewRedisSessionStore(opts *options.SessionOptions, cookieOpts *options.CookieOptions) (sessions.SessionStore, error) {
42+func NewRedisSessionStore(opts *options.SessionOptions, cookieOpts *options.Cookie) (sessions.SessionStore, error) {
4343 cipher, err := encryption.NewBase64Cipher(encryption.NewCFBCipher, encryption.SecretBytes(cookieOpts.Secret))
4444 if err != nil {
4545 return nil, fmt.Errorf("error initialising cipher: %v", err)
@@ -51,9 +51,9 @@ func NewRedisSessionStore(opts *options.SessionOptions, cookieOpts *options.Cook
5151 }
5252
5353 rs := &SessionStore{
54− Client: client,
55− CookieCipher: cipher,
56− CookieOptions: cookieOpts,
54+ Client: client,
55+ CookieCipher: cipher,
56+ Cookie: cookieOpts,
5757 }
5858 return rs, nil
5959
@@ -145,13 +145,13 @@ func (store *SessionStore) Save(rw http.ResponseWriter, req *http.Request, s *se
145145
146146 // Old sessions that we are refreshing would have a request cookie
147147 // New sessions don't, so we ignore the error. storeValue will check requestCookie
148− requestCookie, _ := req.Cookie(store.CookieOptions.Name)
148+ requestCookie, _ := req.Cookie(store.Cookie.Name)
149149 value, err := s.EncodeSessionState(store.CookieCipher)
150150 if err != nil {
151151 return err
152152 }
153153 ctx := req.Context()
154− ticketString, err := store.storeValue(ctx, value, store.CookieOptions.Expire, requestCookie)
154+ ticketString, err := store.storeValue(ctx, value, store.Cookie.Expire, requestCookie)
155155 if err != nil {
156156 return err
157157 }
@@ -159,7 +159,7 @@ func (store *SessionStore) Save(rw http.ResponseWriter, req *http.Request, s *se
159159 ticketCookie := store.makeCookie(
160160 req,
161161 ticketString,
162− store.CookieOptions.Expire,
162+ store.Cookie.Expire,
163163 *s.CreatedAt,
164164 )
165165
@@ -170,12 +170,12 @@ func (store *SessionStore) Save(rw http.ResponseWriter, req *http.Request, s *se
170170// Load reads sessions.SessionState information from a ticket
171171// cookie within the HTTP request object
172172func (store *SessionStore) Load(req *http.Request) (*sessions.SessionState, error) {
173− requestCookie, err := req.Cookie(store.CookieOptions.Name)
173+ requestCookie, err := req.Cookie(store.Cookie.Name)
174174 if err != nil {
175175 return nil, fmt.Errorf("error loading session: %s", err)
176176 }
177177
178− val, _, ok := encryption.Validate(requestCookie, store.CookieOptions.Secret, store.CookieOptions.Expire)
178+ val, _, ok := encryption.Validate(requestCookie, store.Cookie.Secret, store.Cookie.Expire)
179179 if !ok {
180180 return nil, fmt.Errorf("cookie signature not valid")
181181 }
@@ -189,12 +189,12 @@ func (store *SessionStore) Load(req *http.Request) (*sessions.SessionState, erro
189189
190190// loadSessionFromString loads the session based on the ticket value
191191func (store *SessionStore) loadSessionFromString(ctx context.Context, value string) (*sessions.SessionState, error) {
192− ticket, err := decodeTicket(store.CookieOptions.Name, value)
192+ ticket, err := decodeTicket(store.Cookie.Name, value)
193193 if err != nil {
194194 return nil, err
195195 }
196196
197− resultBytes, err := store.Client.Get(ctx, ticket.asHandle(store.CookieOptions.Name))
197+ resultBytes, err := store.Client.Get(ctx, ticket.asHandle(store.Cookie.Name))
198198 if err != nil {
199199 return nil, err
200200 }
@@ -227,7 +227,7 @@ func (store *SessionStore) Clear(rw http.ResponseWriter, req *http.Request) erro
227227 http.SetCookie(rw, clearCookie)
228228
229229 // If there was an existing cookie we should clear the session in redis
230− requestCookie, err := req.Cookie(store.CookieOptions.Name)
230+ requestCookie, err := req.Cookie(store.Cookie.Name)
231231 if err != nil && err == http.ErrNoCookie {
232232 // No existing cookie so can't clear redis
233233 return nil
@@ -235,17 +235,17 @@ func (store *SessionStore) Clear(rw http.ResponseWriter, req *http.Request) erro
235235 return fmt.Errorf("error retrieving cookie: %v", err)
236236 }
237237
238− val, _, ok := encryption.Validate(requestCookie, store.CookieOptions.Secret, store.CookieOptions.Expire)
238+ val, _, ok := encryption.Validate(requestCookie, store.Cookie.Secret, store.Cookie.Expire)
239239 if !ok {
240240 return fmt.Errorf("cookie signature not valid")
241241 }
242242
243243 // We only return an error if we had an issue with redis
244244 // If there's an issue decoding the ticket, ignore it
245− ticket, _ := decodeTicket(store.CookieOptions.Name, string(val))
245+ ticket, _ := decodeTicket(store.Cookie.Name, string(val))
246246 if ticket != nil {
247247 ctx := req.Context()
248− err := store.Client.Del(ctx, ticket.asHandle(store.CookieOptions.Name))
248+ err := store.Client.Del(ctx, ticket.asHandle(store.Cookie.Name))
249249 if err != nil {
250250 return fmt.Errorf("error clearing cookie from redis: %s", err)
251251 }
@@ -256,13 +256,13 @@ func (store *SessionStore) Clear(rw http.ResponseWriter, req *http.Request) erro
256256// makeCookie makes a cookie, signing the value if present
257257func (store *SessionStore) makeCookie(req *http.Request, value string, expires time.Duration, now time.Time) *http.Cookie {
258258 if value != "" {
259− value = encryption.SignedValue(store.CookieOptions.Secret, store.CookieOptions.Name, []byte(value), now)
259+ value = encryption.SignedValue(store.Cookie.Secret, store.Cookie.Name, []byte(value), now)
260260 }
261261 return cookies.MakeCookieFromOptions(
262262 req,
263− store.CookieOptions.Name,
263+ store.Cookie.Name,
264264 value,
265− store.CookieOptions,
265+ store.Cookie,
266266 expires,
267267 now,
268268 )
@@ -284,12 +284,12 @@ func (store *SessionStore) storeValue(ctx context.Context, value string, expirat
284284 stream := cipher.NewCFBEncrypter(block, ticket.Secret)
285285 stream.XORKeyStream(ciphertext, []byte(value))
286286
287− handle := ticket.asHandle(store.CookieOptions.Name)
287+ handle := ticket.asHandle(store.Cookie.Name)
288288 err = store.Client.Set(ctx, handle, ciphertext, expiration)
289289 if err != nil {
290290 return "", err
291291 }
292− return ticket.encodeTicket(store.CookieOptions.Name), nil
292+ return ticket.encodeTicket(store.Cookie.Name), nil
293293}
294294
295295// getTicket retrieves an existing ticket from the cookie if present,
@@ -300,14 +300,14 @@ func (store *SessionStore) getTicket(requestCookie *http.Cookie) (*TicketData, e
300300 }
301301
302302 // An existing cookie exists, try to retrieve the ticket
303− val, _, ok := encryption.Validate(requestCookie, store.CookieOptions.Secret, store.CookieOptions.Expire)
303+ val, _, ok := encryption.Validate(requestCookie, store.Cookie.Secret, store.Cookie.Expire)
304304 if !ok {
305305 // Cookie is invalid, create a new ticket
306306 return newTicket()
307307 }
308308
309309 // Valid cookie, decode the ticket
310− ticket, err := decodeTicket(store.CookieOptions.Name, string(val))
310+ ticket, err := decodeTicket(store.Cookie.Name, string(val))
311311 if err != nil {
312312 // If we can't decode the ticket we have to create a new one
313313 return newTicket()
pkg/sessions/redis/session_store_test.go+3−3
@@ -58,7 +58,7 @@ var _ = Describe("Redis SessionStore Tests", func() {
5858 })
5959
6060 tests.RunSessionStoreTests(
61− func(opts *options.SessionOptions, cookieOpts *options.CookieOptions) (sessionsapi.SessionStore, error) {
61+ func(opts *options.SessionOptions, cookieOpts *options.Cookie) (sessionsapi.SessionStore, error) {
6262 // Set the connection URL
6363 opts.Type = options.RedisSessionStoreType
6464 opts.Redis.ConnectionURL = "redis://" + mr.Addr()
@@ -87,7 +87,7 @@ var _ = Describe("Redis SessionStore Tests", func() {
8787 })
8888
8989 tests.RunSessionStoreTests(
90− func(opts *options.SessionOptions, cookieOpts *options.CookieOptions) (sessionsapi.SessionStore, error) {
90+ func(opts *options.SessionOptions, cookieOpts *options.Cookie) (sessionsapi.SessionStore, error) {
9191 // Set the sentinel connection URL
9292 sentinelAddr := "redis://" + ms.Addr()
9393 opts.Type = options.RedisSessionStoreType
@@ -109,7 +109,7 @@ var _ = Describe("Redis SessionStore Tests", func() {
109109
110110 Context("with cluster", func() {
111111 tests.RunSessionStoreTests(
112− func(opts *options.SessionOptions, cookieOpts *options.CookieOptions) (sessionsapi.SessionStore, error) {
112+ func(opts *options.SessionOptions, cookieOpts *options.Cookie) (sessionsapi.SessionStore, error) {
113113 clusterAddr := "redis://" + mr.Addr()
114114 opts.Type = options.RedisSessionStoreType
115115 opts.Redis.ClusterConnectionURLs = []string{clusterAddr}
pkg/sessions/session_store.go+1−1
@@ -10,7 +10,7 @@ import (
1010)
1111
1212// NewSessionStore creates a SessionStore from the provided configuration
13−func NewSessionStore(opts *options.SessionOptions, cookieOpts *options.CookieOptions) (sessions.SessionStore, error) {
13+func NewSessionStore(opts *options.SessionOptions, cookieOpts *options.Cookie) (sessions.SessionStore, error) {
1414 switch opts.Type {
1515 case options.CookieSessionStoreType:
1616 return cookie.NewCookieSessionStore(opts, cookieOpts)
pkg/sessions/session_store_test.go+2−2
@@ -24,7 +24,7 @@ func TestSessionStore(t *testing.T) {
2424
2525var _ = Describe("NewSessionStore", func() {
2626 var opts *options.SessionOptions
27− var cookieOpts *options.CookieOptions
27+ var cookieOpts *options.Cookie
2828
2929 BeforeEach(func() {
3030 opts = &options.SessionOptions{}
@@ -36,7 +36,7 @@ var _ = Describe("NewSessionStore", func() {
3636 Expect(err).ToNot(HaveOccurred())
3737
3838 // Set default options in CookieOptions
39− cookieOpts = &options.CookieOptions{
39+ cookieOpts = &options.Cookie{
4040 Name: "_oauth2_proxy",
4141 Secret: base64.URLEncoding.EncodeToString(secret),
4242 Path: "/",
pkg/sessions/tests/session_store_tests.go+4−4
@@ -21,7 +21,7 @@ import (
2121// Ginkgo has unpacked the tests.
2222// Interfaces have to be wrapped in closures otherwise nil pointers are thrown.
2323type testInput struct {
24− cookieOpts *options.CookieOptions
24+ cookieOpts *options.Cookie
2525 ss sessionStoreFunc
2626 session *sessionsapi.SessionState
2727 request *http.Request
@@ -38,7 +38,7 @@ type PersistentStoreFastForwardFunc func(time.Duration) error
3838
3939// NewSessionStoreFunc allows any session store implementation to configure their
4040// own session store before each test.
41−type NewSessionStoreFunc func(sessionOpts *options.SessionOptions, cookieOpts *options.CookieOptions) (sessionsapi.SessionStore, error)
41+type NewSessionStoreFunc func(sessionOpts *options.SessionOptions, cookieOpts *options.Cookie) (sessionsapi.SessionStore, error)
4242
4343func RunSessionStoreTests(newSS NewSessionStoreFunc, persistentFastForward PersistentStoreFastForwardFunc) {
4444 Describe("Session Store Suite", func() {
@@ -62,7 +62,7 @@ func RunSessionStoreTests(newSS NewSessionStoreFunc, persistentFastForward Persi
6262 Expect(err).ToNot(HaveOccurred())
6363
6464 // Set default options in CookieOptions
65− cookieOpts := &options.CookieOptions{
65+ cookieOpts := &options.Cookie{
6666 Name: "_oauth2_proxy",
6767 Path: "/",
6868 Expire: time.Duration(168) * time.Hour,
@@ -111,7 +111,7 @@ func RunSessionStoreTests(newSS NewSessionStoreFunc, persistentFastForward Persi
111111
112112 Context("with non-default options", func() {
113113 BeforeEach(func() {
114− input.cookieOpts = &options.CookieOptions{
114+ input.cookieOpts = &options.Cookie{
115115 Name: "_cookie_name",
116116 Path: "/path",
117117 Expire: time.Duration(72) * time.Hour,
pkg/validation/cookie.gonew file+68
@@ -0,0 +1,68 @@
1+package validation
2+
3+import (
4+ "fmt"
5+ "net/http"
6+ "sort"
7+
8+ "github.com/oauth2-proxy/oauth2-proxy/pkg/apis/options"
9+ "github.com/oauth2-proxy/oauth2-proxy/pkg/encryption"
10+)
11+
12+func validateCookie(o options.Cookie) []string {
13+ msgs := validateCookieSecret(o.Secret)
14+
15+ if o.Refresh >= o.Expire {
16+ msgs = append(msgs, fmt.Sprintf(
17+ "cookie_refresh (%q) must be less than cookie_expire (%q)",
18+ o.Refresh.String(),
19+ o.Expire.String()))
20+ }
21+
22+ switch o.SameSite {
23+ case "", "none", "lax", "strict":
24+ default:
25+ msgs = append(msgs, fmt.Sprintf("cookie_samesite (%q) must be one of ['', 'lax', 'strict', 'none']", o.SameSite))
26+ }
27+
28+ // Sort cookie domains by length, so that we try longer (and more specific) domains first
29+ sort.Slice(o.Domains, func(i, j int) bool {
30+ return len(o.Domains[i]) > len(o.Domains[j])
31+ })
32+
33+ msgs = append(msgs, validateCookieName(o.Name)...)
34+ return msgs
35+}
36+
37+func validateCookieName(name string) []string {
38+ msgs := []string{}
39+
40+ cookie := &http.Cookie{Name: name}
41+ if cookie.String() == "" {
42+ msgs = append(msgs, fmt.Sprintf("invalid cookie name: %q", name))
43+ }
44+
45+ if len(name) > 256 {
46+ msgs = append(msgs, fmt.Sprintf("cookie name should be under 256 characters: cookie name is %d characters", len(name)))
47+ }
48+ return msgs
49+}
50+
51+func validateCookieSecret(secret string) []string {
52+ if secret == "" {
53+ return []string{"missing setting: cookie-secret"}
54+ }
55+
56+ secretBytes := encryption.SecretBytes(secret)
57+ // Check if the secret is a valid length
58+ switch len(secretBytes) {
59+ case 16, 24, 32:
60+ // Valid secret size found
61+ return []string{}
62+ }
63+ // Invalid secret size found, return a message
64+ return []string{fmt.Sprintf(
65+ "cookie_secret must be 16, 24, or 32 bytes to create an AES cipher, but is %d bytes",
66+ len(secretBytes)),
67+ }
68+}
pkg/validation/cookie_test.gonew file+273
@@ -0,0 +1,273 @@
1+package validation
2+
3+import (
4+ "strings"
5+ "testing"
6+ "time"
7+
8+ "github.com/oauth2-proxy/oauth2-proxy/pkg/apis/options"
9+ . "github.com/onsi/gomega"
10+)
11+
12+func TestValidateCookie(t *testing.T) {
13+ alphabet := "abcdefghijklmnopqrstuvwxyz"
14+
15+ validName := "_oauth2_proxy"
16+ invalidName := "_oauth2;proxy" // Separater character not allowed
17+ // 10 times the alphabet should be longer than 256 characters
18+ longName := strings.Repeat(alphabet, 10)
19+ validSecret := "secretthirtytwobytes+abcdefghijk"
20+ invalidSecret := "abcdef" // 6 bytes is not a valid size
21+ validBase64Secret := "c2VjcmV0dGhpcnR5dHdvYnl0ZXMrYWJjZGVmZ2hpams" // Base64 encoding of "secretthirtytwobytes+abcdefghijk"
22+ invalidBase64Secret := "YWJjZGVmCg" // Base64 encoding of "abcdef"
23+ emptyDomains := []string{}
24+ domains := []string{
25+ "a.localhost",
26+ "ba.localhost",
27+ "ca.localhost",
28+ "cba.localhost",
29+ "a.cba.localhost",
30+ }
31+
32+ invalidNameMsg := "invalid cookie name: \"_oauth2;proxy\""
33+ longNameMsg := "cookie name should be under 256 characters: cookie name is 260 characters"
34+ missingSecretMsg := "missing setting: cookie-secret"
35+ invalidSecretMsg := "cookie_secret must be 16, 24, or 32 bytes to create an AES cipher, but is 6 bytes"
36+ invalidBase64SecretMsg := "cookie_secret must be 16, 24, or 32 bytes to create an AES cipher, but is 10 bytes"
37+ refreshLongerThanExpireMsg := "cookie_refresh (\"1h0m0s\") must be less than cookie_expire (\"15m0s\")"
38+ invalidSameSiteMsg := "cookie_samesite (\"invalid\") must be one of ['', 'lax', 'strict', 'none']"
39+
40+ testCases := []struct {
41+ name string
42+ cookie options.Cookie
43+ errStrings []string
44+ }{
45+ {
46+ name: "with valid configuration",
47+ cookie: options.Cookie{
48+ Name: validName,
49+ Secret: validSecret,
50+ Domains: domains,
51+ Path: "",
52+ Expire: time.Hour,
53+ Refresh: 15 * time.Minute,
54+ Secure: true,
55+ HTTPOnly: false,
56+ SameSite: "",
57+ },
58+ errStrings: []string{},
59+ },
60+ {
61+ name: "with no cookie secret",
62+ cookie: options.Cookie{
63+ Name: validName,
64+ Secret: "",
65+ Domains: emptyDomains,
66+ Path: "",
67+ Expire: time.Hour,
68+ Refresh: 15 * time.Minute,
69+ Secure: true,
70+ HTTPOnly: false,
71+ SameSite: "",
72+ },
73+ errStrings: []string{
74+ missingSecretMsg,
75+ },
76+ },
77+ {
78+ name: "with an invalid cookie secret",
79+ cookie: options.Cookie{
80+ Name: validName,
81+ Secret: invalidSecret,
82+ Domains: emptyDomains,
83+ Path: "",
84+ Expire: time.Hour,
85+ Refresh: 15 * time.Minute,
86+ Secure: true,
87+ HTTPOnly: false,
88+ SameSite: "",
89+ },
90+ errStrings: []string{
91+ invalidSecretMsg,
92+ },
93+ },
94+ {
95+ name: "with a valid Base64 secret",
96+ cookie: options.Cookie{
97+ Name: validName,
98+ Secret: validBase64Secret,
99+ Domains: emptyDomains,
100+ Path: "",
101+ Expire: time.Hour,
102+ Refresh: 15 * time.Minute,
103+ Secure: true,
104+ HTTPOnly: false,
105+ SameSite: "",
106+ },
107+ errStrings: []string{},
108+ },
109+ {
110+ name: "with an invalid Base64 secret",
111+ cookie: options.Cookie{
112+ Name: validName,
113+ Secret: invalidBase64Secret,
114+ Domains: emptyDomains,
115+ Path: "",
116+ Expire: time.Hour,
117+ Refresh: 15 * time.Minute,
118+ Secure: true,
119+ HTTPOnly: false,
120+ SameSite: "",
121+ },
122+ errStrings: []string{
123+ invalidBase64SecretMsg,
124+ },
125+ },
126+ {
127+ name: "with an invalid name",
128+ cookie: options.Cookie{
129+ Name: invalidName,
130+ Secret: validSecret,
131+ Domains: emptyDomains,
132+ Path: "",
133+ Expire: time.Hour,
134+ Refresh: 15 * time.Minute,
135+ Secure: true,
136+ HTTPOnly: false,
137+ SameSite: "",
138+ },
139+ errStrings: []string{
140+ invalidNameMsg,
141+ },
142+ },
143+ {
144+ name: "with a name that is too long",
145+ cookie: options.Cookie{
146+ Name: longName,
147+ Secret: validSecret,
148+ Domains: emptyDomains,
149+ Path: "",
150+ Expire: time.Hour,
151+ Refresh: 15 * time.Minute,
152+ Secure: true,
153+ HTTPOnly: false,
154+ SameSite: "",
155+ },
156+ errStrings: []string{
157+ longNameMsg,
158+ },
159+ },
160+ {
161+ name: "with refresh longer than expire",
162+ cookie: options.Cookie{
163+ Name: validName,
164+ Secret: validSecret,
165+ Domains: emptyDomains,
166+ Path: "",
167+ Expire: 15 * time.Minute,
168+ Refresh: time.Hour,
169+ Secure: true,
170+ HTTPOnly: false,
171+ SameSite: "",
172+ },
173+ errStrings: []string{
174+ refreshLongerThanExpireMsg,
175+ },
176+ },
177+ {
178+ name: "with samesite \"none\"",
179+ cookie: options.Cookie{
180+ Name: validName,
181+ Secret: validSecret,
182+ Domains: emptyDomains,
183+ Path: "",
184+ Expire: time.Hour,
185+ Refresh: 15 * time.Minute,
186+ Secure: true,
187+ HTTPOnly: false,
188+ SameSite: "none",
189+ },
190+ errStrings: []string{},
191+ },
192+ {
193+ name: "with samesite \"lax\"",
194+ cookie: options.Cookie{
195+ Name: validName,
196+ Secret: validSecret,
197+ Domains: emptyDomains,
198+ Path: "",
199+ Expire: time.Hour,
200+ Refresh: 15 * time.Minute,
201+ Secure: true,
202+ HTTPOnly: false,
203+ SameSite: "none",
204+ },
205+ errStrings: []string{},
206+ },
207+ {
208+ name: "with samesite \"strict\"",
209+ cookie: options.Cookie{
210+ Name: validName,
211+ Secret: validSecret,
212+ Domains: emptyDomains,
213+ Path: "",
214+ Expire: time.Hour,
215+ Refresh: 15 * time.Minute,
216+ Secure: true,
217+ HTTPOnly: false,
218+ SameSite: "none",
219+ },
220+ errStrings: []string{},
221+ },
222+ {
223+ name: "with samesite \"invalid\"",
224+ cookie: options.Cookie{
225+ Name: validName,
226+ Secret: validSecret,
227+ Domains: emptyDomains,
228+ Path: "",
229+ Expire: time.Hour,
230+ Refresh: 15 * time.Minute,
231+ Secure: true,
232+ HTTPOnly: false,
233+ SameSite: "invalid",
234+ },
235+ errStrings: []string{
236+ invalidSameSiteMsg,
237+ },
238+ },
239+ {
240+ name: "with a combination of configuration errors",
241+ cookie: options.Cookie{
242+ Name: invalidName,
243+ Secret: invalidSecret,
244+ Domains: domains,
245+ Path: "",
246+ Expire: 15 * time.Minute,
247+ Refresh: time.Hour,
248+ Secure: true,
249+ HTTPOnly: false,
250+ SameSite: "invalid",
251+ },
252+ errStrings: []string{
253+ invalidNameMsg,
254+ invalidSecretMsg,
255+ refreshLongerThanExpireMsg,
256+ invalidSameSiteMsg,
257+ },
258+ },
259+ }
260+
261+ for _, tc := range testCases {
262+ t.Run(tc.name, func(t *testing.T) {
263+ errStrings := validateCookie(tc.cookie)
264+ g := NewWithT(t)
265+
266+ g.Expect(errStrings).To(ConsistOf(tc.errStrings))
267+ // Check domains were sorted to the right lengths
268+ for i := 0; i < len(tc.cookie.Domains)-1; i++ {
269+ g.Expect(len(tc.cookie.Domains[i])).To(BeNumerically(">=", len(tc.cookie.Domains[i+1])))
270+ }
271+ })
272+ }
273+}
pkg/validation/options.go+2−56
@@ -10,14 +10,12 @@ import (
1010 "net/url"
1111 "os"
1212 "regexp"
13− "sort"
1413 "strings"
1514
1615 "github.com/coreos/go-oidc"
1716 "github.com/dgrijalva/jwt-go"
1817 "github.com/mbland/hmacauth"
1918 "github.com/oauth2-proxy/oauth2-proxy/pkg/apis/options"
20− "github.com/oauth2-proxy/oauth2-proxy/pkg/encryption"
2119 "github.com/oauth2-proxy/oauth2-proxy/pkg/ip"
2220 "github.com/oauth2-proxy/oauth2-proxy/pkg/logger"
2321 "github.com/oauth2-proxy/oauth2-proxy/pkg/requests"
@@ -28,7 +26,8 @@ import (
2826// Validate checks that required options are set and validates those that they
2927// are of the correct format
3028func Validate(o *options.Options) error {
31− msgs := make([]string, 0)
29+ msgs := validateCookie(o.Cookie)
30+
3231 if o.SSLInsecureSkipVerify {
3332 insecureTransport := &http.Transport{
3433 TLSClientConfig: &tls.Config{InsecureSkipVerify: true},
@@ -49,30 +48,6 @@ func Validate(o *options.Options) error {
4948 }
5049 }
5150
52− if o.Cookie.Secret == "" {
53− msgs = append(msgs, "missing setting: cookie-secret")
54− } else {
55− validCookieSecretSize := false
56− for _, i := range []int{16, 24, 32} {
57− if len(encryption.SecretBytes(o.Cookie.Secret)) == i {
58− validCookieSecretSize = true
59− }
60− }
61− var decoded bool
62− if string(encryption.SecretBytes(o.Cookie.Secret)) != o.Cookie.Secret {
63− decoded = true
64− }
65− if !validCookieSecretSize {
66− var suffix string
67− if decoded {
68− suffix = " note: cookie secret was base64 decoded"
69− }
70− msgs = append(msgs,
71− fmt.Sprintf("Cookie secret must be 16, 24, or 32 bytes to create an AES cipher. Got %d bytes.%s",
72− len(encryption.SecretBytes(o.Cookie.Secret)), suffix))
73− }
74− }
75−
7651 if o.ClientID == "" {
7752 msgs = append(msgs, "missing setting: client-id")
7853 }
@@ -222,14 +197,6 @@ func Validate(o *options.Options) error {
222197 }
223198 msgs = parseProviderInfo(o, msgs)
224199
225− if o.Cookie.Refresh >= o.Cookie.Expire {
226− msgs = append(msgs, fmt.Sprintf(
227− "cookie_refresh (%s) must be less than "+
228− "cookie_expire (%s)",
229− o.Cookie.Refresh.String(),
230− o.Cookie.Expire.String()))
231− }
232−
233200 if len(o.GoogleGroups) > 0 || o.GoogleAdminEmail != "" || o.GoogleServiceAccountJSON != "" {
234201 if len(o.GoogleGroups) < 1 {
235202 msgs = append(msgs, "missing setting: google-group")
@@ -242,20 +209,7 @@ func Validate(o *options.Options) error {
242209 }
243210 }
244211
245− switch o.Cookie.SameSite {
246− case "", "none", "lax", "strict":
247− default:
248− msgs = append(msgs, fmt.Sprintf("cookie_samesite (%s) must be one of ['', 'lax', 'strict', 'none']", o.Cookie.SameSite))
249− }
250−
251− // Sort cookie domains by length, so that we try longer (and more specific)
252− // domains first
253− sort.Slice(o.Cookie.Domains, func(i, j int) bool {
254− return len(o.Cookie.Domains[i]) > len(o.Cookie.Domains[j])
255− })
256−
257212 msgs = parseSignatureKey(o, msgs)
258− msgs = validateCookieName(o, msgs)
259213 msgs = configureLogger(o.Logging, msgs)
260214
261215 if o.ReverseProxy {
@@ -442,14 +396,6 @@ func newVerifierFromJwtIssuer(jwtIssuer jwtIssuer) (*oidc.IDTokenVerifier, error
442396 return verifier, nil
443397}
444398
445−func validateCookieName(o *options.Options, msgs []string) []string {
446− cookie := &http.Cookie{Name: o.Cookie.Name}
447− if cookie.String() == "" {
448− return append(msgs, fmt.Sprintf("invalid cookie name: %q", o.Cookie.Name))
449− }
450− return msgs
451−}
452−
453399// jwtIssuer hold parsed JWT issuer info that's used to construct a verifier.
454400type jwtIssuer struct {
455401 issuerURI string
pkg/validation/options_test.go−15
@@ -2,7 +2,6 @@ package validation
22
33import (
44 "crypto"
5− "fmt"
65 "io/ioutil"
76 "net/url"
87 "os"
@@ -291,20 +290,6 @@ func TestValidateSignatureKeyUnsupportedAlgorithm(t *testing.T) {
291290 " unsupported signature hash algorithm: "+o.SignatureKey)
292291}
293292
294−func TestValidateCookie(t *testing.T) {
295− o := testOptions()
296− o.Cookie.Name = "_valid_cookie_name"
297− assert.Equal(t, nil, Validate(o))
298−}
299−
300−func TestValidateCookieBadName(t *testing.T) {
301− o := testOptions()
302− o.Cookie.Name = "_bad_cookie_name{}"
303− err := Validate(o)
304− assert.Equal(t, err.Error(), "invalid configuration:\n"+
305− fmt.Sprintf(" invalid cookie name: %q", o.Cookie.Name))
306−}
307−
308293func TestSkipOIDCDiscovery(t *testing.T) {
309294 o := testOptions()
310295 o.ProviderType = "oidc"