Integrate cookie builder with persistent session manager

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
+	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,
+		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)

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)

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"
@@ -40,18 +39,18 @@ type initLockFunc func(string) sessions.Lock
 type ticket struct {
 	id            string
 	secret        []byte
-	options *options.Cookie
+	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 {
@@ -61,7 +60,7 @@ func newTicket(cookieOpts *options.Cookie) (*ticket, error) {
 	return &ticket{
 		id:            ticketID,
 		secret:        secret,
-		options: cookieOpts,
+		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")
@@ -86,27 +85,20 @@ func decodeTicket(encTicket string, cookieOpts *options.Cookie) (*ticket, error)
 	return &ticket{
 		id:            ticketID,
 		secret:        secret,
-		options: cookieOpts,
+		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) {
+func decodeTicketFromRequest(req *http.Request, cookieBuilder cookies.Builder) (*ticket, error) {
-	requestCookie, err := req.Cookie(cookieOpts.Name)
+	val, err := cookieBuilder.ValidateRequest(req)
 	if err != nil {
-		// Don't wrap this error to allow `err == http.ErrNoCookie` checks
+		return nil, fmt.Errorf("invalid cookie: %w", err)
-		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)
 	}
 
 	// 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(
+	ticketCookie, err := t.cookieBuilder.
-		req,
+		WithSignedValue(true).
-		t.encodeTicket(),
+		WithStart(*s.CreatedAt).
-		t.options.Expire,
+		MakeCookie(req, t.encodeTicket())
-		*s.CreatedAt,
-	)
 	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) {
+func (t *ticket) clearCookie(rw http.ResponseWriter, req *http.Request) error {
-	http.SetCookie(rw, cookies.MakeCookieFromOptions(
+	cookie, err := t.cookieBuilder.
-		req,
+		WithExpiration(time.Hour*-1). // Set the expiration to negative to clear the cookie
-		t.options.Name,
+		MakeCookie(req, "")
-		"",
-		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
+		return fmt.Errorf("could not clear cookie: %v", err)
 	}
-	}
+
-	return cookies.MakeCookieFromOptions(
+	http.SetCookie(rw, cookie)
-		req,
+	return nil
-		t.options.Name,
-		value,
-		t.options,
-		expires,
-		now,
-	), nil
 }
 
 // makeCipher makes a AES-GCM cipher out of the ticket's secret

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 {

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

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 {

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)
 	}

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)