From 2dcda8539ccd80f57b54e1cc78bb5d27612faf9d Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Tue, 23 Mar 2021 13:50:55 +0000 Subject: [PATCH] Integrate cookie builder with persistent session manager --- pkg/sessions/persistence/manager.go | 29 ++++---- pkg/sessions/persistence/manager_test.go | 4 +- pkg/sessions/persistence/ticket.go | 88 ++++++++--------------- pkg/sessions/persistence/ticket_test.go | 25 ++++--- pkg/sessions/redis/redis_store.go | 5 +- pkg/sessions/redis/redis_store_test.go | 13 ++-- pkg/sessions/session_store.go | 3 +- pkg/sessions/tests/session_store_tests.go | 6 +- 8 files changed, 80 insertions(+), 93 deletions(-) diff --git a/pkg/sessions/persistence/manager.go b/pkg/sessions/persistence/manager.go index 3215b257..8827fbb4 100644 --- a/pkg/sessions/persistence/manager.go +++ b/pkg/sessions/persistence/manager.go @@ -1,27 +1,28 @@ package persistence import ( + "errors" "fmt" "net/http" "time" - "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/cookies" ) // Manager wraps a Store and handles the implementation details of the // sessions.SessionStore with its use of session tickets type Manager struct { - Store Store - Options *options.Cookie + Store Store + cookieBuilder cookies.Builder } // NewManager creates a Manager that can wrap a Store and manage the // sessions.SessionStore implementation details -func NewManager(store Store, cookieOpts *options.Cookie) *Manager { +func NewManager(store Store, cookieBuilder cookies.Builder) *Manager { return &Manager{ - Store: store, - Options: cookieOpts, + Store: store, + cookieBuilder: cookieBuilder, } } @@ -33,9 +34,9 @@ func (m *Manager) Save(rw http.ResponseWriter, req *http.Request, s *sessions.Se s.CreatedAtNow() } - tckt, err := decodeTicketFromRequest(req, m.Options) + tckt, err := decodeTicketFromRequest(req, m.cookieBuilder) if err != nil { - tckt, err = newTicket(m.Options) + tckt, err = newTicket(m.cookieBuilder) if err != nil { return fmt.Errorf("error creating a session ticket: %v", err) } @@ -54,7 +55,7 @@ func (m *Manager) Save(rw http.ResponseWriter, req *http.Request, s *sessions.Se // Load reads sessions.SessionState information from a session store. It will // use the session ticket from the http.Request's cookie. func (m *Manager) Load(req *http.Request) (*sessions.SessionState, error) { - tckt, err := decodeTicketFromRequest(req, m.Options) + tckt, err := decodeTicketFromRequest(req, m.cookieBuilder) if err != nil { return nil, err } @@ -70,16 +71,18 @@ func (m *Manager) Load(req *http.Request) (*sessions.SessionState, error) { // Clear clears any saved session information for a given ticket cookie. // Then it clears all session data for that ticket in the Store. func (m *Manager) Clear(rw http.ResponseWriter, req *http.Request) error { - tckt, err := decodeTicketFromRequest(req, m.Options) + tckt, err := decodeTicketFromRequest(req, m.cookieBuilder) if err != nil { // Always clear the cookie, even when we can't load a cookie from // the request tckt = &ticket{ - options: m.Options, + cookieBuilder: m.cookieBuilder, + } + if err := tckt.clearCookie(rw, req); err != nil { + return fmt.Errorf("error creating cookie to clear session: %v", err) } - tckt.clearCookie(rw, req) // Don't raise an error if we didn't have a Cookie - if err == http.ErrNoCookie { + if errors.Is(err, http.ErrNoCookie) { return nil } return fmt.Errorf("error decoding ticket to clear session: %v", err) diff --git a/pkg/sessions/persistence/manager_test.go b/pkg/sessions/persistence/manager_test.go index 791595bd..dd715967 100644 --- a/pkg/sessions/persistence/manager_test.go +++ b/pkg/sessions/persistence/manager_test.go @@ -5,6 +5,7 @@ import ( "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" sessionsapi "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/cookies" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/sessions/tests" . "github.com/onsi/ginkgo" ) @@ -16,7 +17,8 @@ var _ = Describe("Persistence Manager Tests", func() { }) tests.RunSessionStoreTests( func(_ *options.SessionOptions, cookieOpts *options.Cookie) (sessionsapi.SessionStore, error) { - return NewManager(ms, cookieOpts), nil + builder := cookies.NewBuilder(*cookieOpts) + return NewManager(ms, builder), nil }, func(d time.Duration) error { ms.FastForward(d) diff --git a/pkg/sessions/persistence/ticket.go b/pkg/sessions/persistence/ticket.go index 70bb58a0..9848deb3 100644 --- a/pkg/sessions/persistence/ticket.go +++ b/pkg/sessions/persistence/ticket.go @@ -12,7 +12,6 @@ import ( "strings" "time" - "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/cookies" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/encryption" @@ -38,20 +37,20 @@ type initLockFunc func(string) sessions.Lock // session storage. It provides a unique per session decryption secret giving // more security than the shared CookieSecret. type ticket struct { - id string - secret []byte - options *options.Cookie + id string + secret []byte + cookieBuilder cookies.Builder } // newTicket creates a new ticket. The ID & secret will be randomly created // with 16 byte sizes. The ID will be prefixed & hex encoded. -func newTicket(cookieOpts *options.Cookie) (*ticket, error) { +func newTicket(cookieBuilder cookies.Builder) (*ticket, error) { rawID := make([]byte, 16) if _, err := io.ReadFull(rand.Reader, rawID); err != nil { return nil, fmt.Errorf("failed to create new ticket ID: %v", err) } // ticketID is hex encoded - ticketID := fmt.Sprintf("%s-%s", cookieOpts.Name, hex.EncodeToString(rawID)) + ticketID := fmt.Sprintf("%s-%s", cookieBuilder.GetName(), hex.EncodeToString(rawID)) secret := make([]byte, aes.BlockSize) if _, err := io.ReadFull(rand.Reader, secret); err != nil { @@ -59,9 +58,9 @@ func newTicket(cookieOpts *options.Cookie) (*ticket, error) { } return &ticket{ - id: ticketID, - secret: secret, - options: cookieOpts, + id: ticketID, + secret: secret, + cookieBuilder: cookieBuilder, }, nil } @@ -71,7 +70,7 @@ func (t *ticket) encodeTicket() string { } // decodeTicket decodes an encoded ticket string -func decodeTicket(encTicket string, cookieOpts *options.Cookie) (*ticket, error) { +func decodeTicket(encTicket string, cookieBuilder cookies.Builder) (*ticket, error) { ticketParts := strings.Split(encTicket, ".") if len(ticketParts) != 2 { return nil, errors.New("failed to decode ticket") @@ -84,29 +83,22 @@ func decodeTicket(encTicket string, cookieOpts *options.Cookie) (*ticket, error) } return &ticket{ - id: ticketID, - secret: secret, - options: cookieOpts, + id: ticketID, + secret: secret, + cookieBuilder: cookieBuilder, }, nil } // decodeTicketFromRequest retrieves a potential ticket cookie from a request // and decodes it to a ticket. -func decodeTicketFromRequest(req *http.Request, cookieOpts *options.Cookie) (*ticket, error) { - requestCookie, err := req.Cookie(cookieOpts.Name) +func decodeTicketFromRequest(req *http.Request, cookieBuilder cookies.Builder) (*ticket, error) { + val, err := cookieBuilder.ValidateRequest(req) if err != nil { - // Don't wrap this error to allow `err == http.ErrNoCookie` checks - return nil, err - } - - // An existing cookie exists, try to retrieve the ticket - val, _, ok := encryption.Validate(requestCookie, cookieOpts.Secret, cookieOpts.Expire) - if !ok { - return nil, fmt.Errorf("session ticket cookie failed validation: %v", err) + return nil, fmt.Errorf("invalid cookie: %w", err) } // Valid cookie, decode the ticket - return decodeTicket(string(val), cookieOpts) + return decodeTicket(val, cookieBuilder) } // saveSession encodes the SessionState with the ticket's secret and persists @@ -120,7 +112,7 @@ func (t *ticket) saveSession(s *sessions.SessionState, saver saveFunc) error { if err != nil { return fmt.Errorf("failed to encode the session state with the ticket: %v", err) } - return saver(t.id, ciphertext, t.options.Expire) + return saver(t.id, ciphertext, t.cookieBuilder.GetExpiration()) } // loadSession loads a session from the disk store via the passed loadFunc @@ -154,12 +146,10 @@ func (t *ticket) clearSession(clearer clearFunc) error { // setCookie sets the encoded ticket as a cookie func (t *ticket) setCookie(rw http.ResponseWriter, req *http.Request, s *sessions.SessionState) error { - ticketCookie, err := t.makeCookie( - req, - t.encodeTicket(), - t.options.Expire, - *s.CreatedAt, - ) + ticketCookie, err := t.cookieBuilder. + WithSignedValue(true). + WithStart(*s.CreatedAt). + MakeCookie(req, t.encodeTicket()) if err != nil { return err } @@ -170,34 +160,16 @@ func (t *ticket) setCookie(rw http.ResponseWriter, req *http.Request, s *session // clearCookie removes any cookies that would be where this ticket // would set them -func (t *ticket) clearCookie(rw http.ResponseWriter, req *http.Request) { - http.SetCookie(rw, cookies.MakeCookieFromOptions( - req, - t.options.Name, - "", - t.options, - time.Hour*-1, - time.Now(), - )) -} - -// makeCookie makes a cookie, signing the value if present -func (t *ticket) makeCookie(req *http.Request, value string, expires time.Duration, now time.Time) (*http.Cookie, error) { - if value != "" { - var err error - value, err = encryption.SignedValue(t.options.Secret, t.options.Name, []byte(value), now) - if err != nil { - return nil, err - } +func (t *ticket) clearCookie(rw http.ResponseWriter, req *http.Request) error { + cookie, err := t.cookieBuilder. + WithExpiration(time.Hour*-1). // Set the expiration to negative to clear the cookie + MakeCookie(req, "") + if err != nil { + return fmt.Errorf("could not clear cookie: %v", err) } - return cookies.MakeCookieFromOptions( - req, - t.options.Name, - value, - t.options, - expires, - now, - ), nil + + http.SetCookie(rw, cookie) + return nil } // makeCipher makes a AES-GCM cipher out of the ticket's secret diff --git a/pkg/sessions/persistence/ticket_test.go b/pkg/sessions/persistence/ticket_test.go index 6b868b90..9c690de6 100644 --- a/pkg/sessions/persistence/ticket_test.go +++ b/pkg/sessions/persistence/ticket_test.go @@ -8,6 +8,7 @@ import ( "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/cookies" . "github.com/onsi/ginkgo" . "github.com/onsi/ginkgo/extensions/table" . "github.com/onsi/gomega" @@ -27,7 +28,7 @@ var _ = Describe("Session Ticket Tests", func() { enc := in.ticket.encodeTicket() Expect(enc).To(Equal(in.encodedTicket)) - dec, err := decodeTicket(enc, in.ticket.options) + dec, err := decodeTicket(enc, in.ticket.cookieBuilder) Expect(err).ToNot(HaveOccurred()) Expect(dec).To(Equal(in.ticket)) } else { @@ -39,9 +40,9 @@ var _ = Describe("Session Ticket Tests", func() { ticket: &ticket{ id: "dummy-0123456789abcdef", secret: []byte("0123456789abcdef"), - options: &options.Cookie{ + cookieBuilder: cookies.NewBuilder(options.Cookie{ Name: "dummy", - }, + }), }, encodedTicket: fmt.Sprintf("%s.%s", "dummy-0123456789abcdef", @@ -63,7 +64,8 @@ var _ = Describe("Session Ticket Tests", func() { Context("saveSession", func() { It("uses the passed save function", func() { - t, err := newTicket(&options.Cookie{Name: "dummy"}) + builder := cookies.NewBuilder(options.Cookie{Name: "dummy"}) + t, err := newTicket(builder) Expect(err).ToNot(HaveOccurred()) c, err := t.makeCipher() @@ -83,7 +85,8 @@ var _ = Describe("Session Ticket Tests", func() { }) It("errors when the saveFunc errors", func() { - t, err := newTicket(&options.Cookie{Name: "dummy"}) + builder := cookies.NewBuilder(options.Cookie{Name: "dummy"}) + t, err := newTicket(builder) Expect(err).ToNot(HaveOccurred()) err = t.saveSession( @@ -97,7 +100,8 @@ var _ = Describe("Session Ticket Tests", func() { Context("loadSession", func() { It("uses the passed load function", func() { - t, err := newTicket(&options.Cookie{Name: "dummy"}) + builder := cookies.NewBuilder(options.Cookie{Name: "dummy"}) + t, err := newTicket(builder) Expect(err).ToNot(HaveOccurred()) c, err := t.makeCipher() @@ -119,7 +123,8 @@ var _ = Describe("Session Ticket Tests", func() { }) It("errors when the loadFunc errors", func() { - t, err := newTicket(&options.Cookie{Name: "dummy"}) + builder := cookies.NewBuilder(options.Cookie{Name: "dummy"}) + t, err := newTicket(builder) Expect(err).ToNot(HaveOccurred()) data, err := t.loadSession( @@ -136,7 +141,8 @@ var _ = Describe("Session Ticket Tests", func() { Context("clearSession", func() { It("uses the passed clear function", func() { - t, err := newTicket(&options.Cookie{Name: "dummy"}) + builder := cookies.NewBuilder(options.Cookie{Name: "dummy"}) + t, err := newTicket(builder) Expect(err).ToNot(HaveOccurred()) var tracker string @@ -149,7 +155,8 @@ var _ = Describe("Session Ticket Tests", func() { }) It("errors when the clearFunc errors", func() { - t, err := newTicket(&options.Cookie{Name: "dummy"}) + builder := cookies.NewBuilder(options.Cookie{Name: "dummy"}) + t, err := newTicket(builder) Expect(err).ToNot(HaveOccurred()) err = t.clearSession(func(k string) error { diff --git a/pkg/sessions/redis/redis_store.go b/pkg/sessions/redis/redis_store.go index 5beee8cb..6721ac4e 100644 --- a/pkg/sessions/redis/redis_store.go +++ b/pkg/sessions/redis/redis_store.go @@ -10,6 +10,7 @@ import ( "github.com/go-redis/redis/v8" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/cookies" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/sessions/persistence" ) @@ -22,7 +23,7 @@ type SessionStore struct { // NewRedisSessionStore initialises a new instance of the SessionStore and wraps // it in a persistence.Manager -func NewRedisSessionStore(opts *options.SessionOptions, cookieOpts *options.Cookie) (sessions.SessionStore, error) { +func NewRedisSessionStore(opts *options.SessionOptions, cookieBuilder cookies.Builder) (sessions.SessionStore, error) { client, err := NewRedisClient(opts.Redis) if err != nil { return nil, fmt.Errorf("error constructing redis client: %v", err) @@ -31,7 +32,7 @@ func NewRedisSessionStore(opts *options.SessionOptions, cookieOpts *options.Cook rs := &SessionStore{ Client: client, } - return persistence.NewManager(rs, cookieOpts), nil + return persistence.NewManager(rs, cookieBuilder), nil } // Save takes a sessions.SessionState and stores the information from it diff --git a/pkg/sessions/redis/redis_store_test.go b/pkg/sessions/redis/redis_store_test.go index e6c5f4ed..37694a5c 100644 --- a/pkg/sessions/redis/redis_store_test.go +++ b/pkg/sessions/redis/redis_store_test.go @@ -12,6 +12,7 @@ import ( "github.com/go-redis/redis/v8" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" sessionsapi "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/cookies" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/sessions/persistence" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/sessions/tests" @@ -80,7 +81,7 @@ var _ = Describe("Redis SessionStore Tests", func() { // Capture the session store so that we can close the client var err error - ss, err = NewRedisSessionStore(opts, cookieOpts) + ss, err = NewRedisSessionStore(opts, cookies.NewBuilder(*cookieOpts)) return ss, err }, func(d time.Duration) error { @@ -112,7 +113,7 @@ var _ = Describe("Redis SessionStore Tests", func() { // Capture the session store so that we can close the client var err error - ss, err = NewRedisSessionStore(opts, cookieOpts) + ss, err = NewRedisSessionStore(opts, cookies.NewBuilder(*cookieOpts)) return ss, err }, func(d time.Duration) error { @@ -132,7 +133,7 @@ var _ = Describe("Redis SessionStore Tests", func() { // Capture the session store so that we can close the client var err error - ss, err = NewRedisSessionStore(opts, cookieOpts) + ss, err = NewRedisSessionStore(opts, cookies.NewBuilder(*cookieOpts)) return ss, err }, func(d time.Duration) error { @@ -160,7 +161,7 @@ var _ = Describe("Redis SessionStore Tests", func() { // Capture the session store so that we can close the client var err error - ss, err = NewRedisSessionStore(opts, cookieOpts) + ss, err = NewRedisSessionStore(opts, cookies.NewBuilder(*cookieOpts)) return ss, err }, func(d time.Duration) error { @@ -193,7 +194,7 @@ var _ = Describe("Redis SessionStore Tests", func() { // Capture the session store so that we can close the client var err error - ss, err = NewRedisSessionStore(opts, cookieOpts) + ss, err = NewRedisSessionStore(opts, cookies.NewBuilder(*cookieOpts)) return ss, err }, func(d time.Duration) error { @@ -214,7 +215,7 @@ var _ = Describe("Redis SessionStore Tests", func() { // Capture the session store so that we can close the client var err error - ss, err = NewRedisSessionStore(opts, cookieOpts) + ss, err = NewRedisSessionStore(opts, cookies.NewBuilder(*cookieOpts)) return ss, err }, func(d time.Duration) error { diff --git a/pkg/sessions/session_store.go b/pkg/sessions/session_store.go index 3d4b8d97..023f5d08 100644 --- a/pkg/sessions/session_store.go +++ b/pkg/sessions/session_store.go @@ -5,6 +5,7 @@ import ( "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/cookies" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/sessions/cookie" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/sessions/redis" ) @@ -15,7 +16,7 @@ func NewSessionStore(opts *options.SessionOptions, cookieOpts *options.Cookie) ( case options.CookieSessionStoreType: return cookie.NewCookieSessionStore(opts, cookieOpts) case options.RedisSessionStoreType: - return redis.NewRedisSessionStore(opts, cookieOpts) + return redis.NewRedisSessionStore(opts, cookies.NewBuilder(*cookieOpts)) default: return nil, fmt.Errorf("unknown session store type '%s'", opts.Type) } diff --git a/pkg/sessions/tests/session_store_tests.go b/pkg/sessions/tests/session_store_tests.go index 416969a3..95197b2b 100644 --- a/pkg/sessions/tests/session_store_tests.go +++ b/pkg/sessions/tests/session_store_tests.go @@ -11,7 +11,6 @@ import ( "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" sessionsapi "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" cookiesapi "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/cookies" - "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/encryption" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" ) @@ -383,9 +382,10 @@ func SessionStoreInterfaceTests(in *testInput) { BeforeEach(func() { By("Using a valid cookie with a different providers session encoding") broken := "BrokenSessionFromADifferentSessionImplementation" - value, err := encryption.SignedValue(in.cookieOpts.Secret, in.cookieOpts.Name, []byte(broken), time.Now()) + cookieBuilder := cookiesapi.NewBuilder(*in.cookieOpts) + + cookie, err := cookieBuilder.MakeCookie(in.request, broken) Expect(err).ToNot(HaveOccurred()) - cookie := cookiesapi.MakeCookieFromOptions(in.request, in.cookieOpts.Name, value, in.cookieOpts, in.cookieOpts.Expire, time.Now()) in.request.AddCookie(cookie) err = in.ss().Save(in.response, in.request, in.session)