Repository: oauth2-proxy/oauth2-proxy
Pull request #1949 (2022-12-25 20:39:54)

Issue 978: Fix Custom cookie name breaks redis for session
…://github.com/oauth2-proxy/oauth2-proxy/issues/978)

Allow cookie names with dots in redis sessions

## Description

Ticket decoding was failing when cookie name had dots in it and using redis. To avoid such issues, ticket ID is encoded with base64. The ticket is now encoded using the format: {encoding version}.{ticketID base64}.{ticketSecret base 64}. This will allow to decode properly for different enconding versions. 

## Motivation and Context

Some users configure cookie name with dots and oauth2-proxy fails.
See issue [978](https://github.com/oauth2-proxy/oauth2-proxy/issues/978).

## How Has This Been Tested?

Needs to be tested.

## 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.


tuunit: Hi @miguelborges99, do you think we can get the tests done until friday? This way we would be able to merge it and release it as part of the 7.5.1 patch release on friday.
tuunit: > Hi @miguelborges99, do you think we can get the tests done until friday? This way we would be able to merge it and release it as part of the 7.5.1 patch release on friday.

@miguelborges99 will you be able to work on the comments mentioned in the review? Otherwise you could give me collaborator access on your fork and I'll be able to finalize the PR for you :)
JoelSpeed: @miguelborges99 If you can fix up the changelog then I think we can get this merged
miguelborges99: > @miguelborges99 If you can fix up the changelog and wrap the errors I highlighted, then I think we can get this merged

Sorry, I can only see this probably in the weekend.
miguelborges99: Sorry for the delay. The request for changes were done.
tuunit: > @tuunit : Sorry for the delay. The request for changes were done.

No worries, its on your free time therefore I'm thankful you keep the PRs up to date :)

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 20ed65ec..93e40a37 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -17,6 +17,7 @@
 - [#2282](https://github.com/oauth2-proxy/oauth2-proxy/pull/2282) Fixed checking Google Groups membership using Google Application Credentials (@kvanzuijlen)
 - [#2183](https://github.com/oauth2-proxy/oauth2-proxy/pull/2183) Allowing relative redirect url though an option (@axel7083)
 - [#1866](https://github.com/oauth2-proxy/oauth2-proxy/pull/1866) Add support for unix socker as upstream (@babs)
+- [#1949](https://github.com/oauth2-proxy/oauth2-proxy/pull/1949) Allow cookie names with dots in redis sessions (@miguelborges99)
 - [#2297](https://github.com/oauth2-proxy/oauth2-proxy/pull/2297) Add nightly build and push (@tuunit)
 
 # V7.5.1
diff --git a/pkg/sessions/persistence/ticket.go b/pkg/sessions/persistence/ticket.go
index 70bb58a0..5020ada9 100644
--- a/pkg/sessions/persistence/ticket.go
+++ b/pkg/sessions/persistence/ticket.go
@@ -67,22 +67,68 @@ func newTicket(cookieOpts *options.Cookie) (*ticket, error) {
 
 // encodeTicket encodes the Ticket to a string for usage in cookies
 func (t *ticket) encodeTicket() string {
-	return fmt.Sprintf("%s.%s", t.id, base64.RawURLEncoding.EncodeToString(t.secret))
+	return fmt.Sprintf("v2.%s.%s", base64.RawURLEncoding.EncodeToString([]byte(t.id)),
+		base64.RawURLEncoding.EncodeToString(t.secret))
+}
+
+// decodeTicketID Tickets are encoded with format: {encoding version}.{ticketID base64}.{ticketSecret base 64}.
+// Tickets from old oauth2-proxy versions do not have the same format, and this method tries
+// to decode the ticket ID part based on the encoding version, or lack of it.
+func decodeTicketID(ticketParts []string) (string, error) {
+	switch {
+	case len(ticketParts) == 2:
+		// old ticket encoding
+		return ticketParts[0], nil
+	case len(ticketParts) == 3 && ticketParts[0] == "v2":
+		// v2 ticket encoding
+		ticketID, err := base64.RawURLEncoding.DecodeString(ticketParts[1])
+		if err != nil {
+			return "", fmt.Errorf("failed to decode ticket Id: %v", err)
+		}
+		return string(ticketID), nil
+	default:
+		return "", errors.New("failed to decode ticket Id")
+	}
+}
+
+// decodeTicketSecret Tickets are encoded with format: {encoding version}.{ticketID base64}.{ticketSecret base 64}.
+// Tickets from old oauth2-proxy versions do not have the same format, and this method tries
+// to decode the ticket secret part based on the encoding version, or lack of it.
+func decodeTicketSecret(ticketParts []string) ([]byte, error) {
+	switch {
+	case len(ticketParts) == 2:
+		// old ticket encoding
+		secret, err := base64.RawURLEncoding.DecodeString(ticketParts[1])
+		if err != nil {
+			return nil, fmt.Errorf("failed to decode encryption secret: %v", err)
+		}
+		return secret, nil
+	case len(ticketParts) == 3 && ticketParts[0] == "v2":
+		// new ticket encode
+		secret, err := base64.RawURLEncoding.DecodeString(ticketParts[2])
+		if err != nil {
+			return nil, fmt.Errorf("failed to decode encryption secret: %v", err)
+		}
+		return secret, nil
+	default:
+		return nil, errors.New("failed to decode encryption secret")
+	}
 }
 
 // decodeTicket decodes an encoded ticket string
 func decodeTicket(encTicket string, cookieOpts *options.Cookie) (*ticket, error) {
 	ticketParts := strings.Split(encTicket, ".")
-	if len(ticketParts) != 2 {
+	if len(ticketParts) != 2 && len(ticketParts) != 3 {
 		return nil, errors.New("failed to decode ticket")
 	}
-	ticketID, secretBase64 := ticketParts[0], ticketParts[1]
-
-	secret, err := base64.RawURLEncoding.DecodeString(secretBase64)
-	if err != nil {
-		return nil, fmt.Errorf("failed to decode encryption secret: %v", err)
+	ticketID, errTicketID := decodeTicketID(ticketParts)
+	if errTicketID != nil {
+		return nil, fmt.Errorf("failed to decode ticket: %v", errTicketID)
+	}
+	secret, errSecret := decodeTicketSecret(ticketParts)
+	if errSecret != nil {
+		return nil, fmt.Errorf("failed to decode ticket: %v", errSecret)
 	}
-
 	return &ticket{
 		id:      ticketID,
 		secret:  secret,


Test patch:
diff --git a/pkg/sessions/persistence/ticket_test.go b/pkg/sessions/persistence/ticket_test.go
index 6b868b90..885f9958 100644
--- a/pkg/sessions/persistence/ticket_test.go
+++ b/pkg/sessions/persistence/ticket_test.go
@@ -43,8 +43,8 @@ var _ = Describe("Session Ticket Tests", func() {
 						Name: "dummy",
 					},
 				},
-				encodedTicket: fmt.Sprintf("%s.%s",
-					"dummy-0123456789abcdef",
+				encodedTicket: fmt.Sprintf("v2.%s.%s",
+					base64.RawURLEncoding.EncodeToString([]byte("dummy-0123456789abcdef")),
 					base64.RawURLEncoding.EncodeToString([]byte("0123456789abcdef"))),
 				expectedError: nil,
 			}),
@@ -56,7 +56,7 @@ var _ = Describe("Session Ticket Tests", func() {
 			Entry("with an invalid base64 encoded secret", ticketTableInput{
 				ticket:        nil,
 				encodedTicket: "dummy-0123456789abcdef.@)#($*@)#(*$@)#(*$",
-				expectedError: fmt.Errorf("failed to decode encryption secret: illegal base64 data at input byte 0"),
+				expectedError: fmt.Errorf("failed to decode ticket: failed to decode encryption secret: illegal base64 data at input byte 0"),
 			}),
 		)
 	})