From 4e2013e6ba170ae34f7a61a8fd16f36d0afd1640 Mon Sep 17 00:00:00 2001
From: "Vish (Ishaya) Abrams" <vishvananda@gmail.com>
Date: Wed, 6 Nov 2024 06:16:39 -0800
Subject: [PATCH] fix: update code_verifier to use recommended method (#2620)

The [RFC](https://datatracker.ietf.org/doc/html/rfc7636#section-4.1)
says that a code verifier just uses unreserved characters, but the
recommended method is that it is a base64-urlencoded 32-octet url. Some
implementations of PKCE (most notably the one used by salesforce)
require that this is a valid base64 encoded string[1], so this patch
switches to using the recommended approach to make it more compatible.

[1]: https://help.salesforce.com/s/articleView?id=sf.remoteaccess_pkce.htm&type=5
---
 CHANGELOG.md                 |  3 ++-
 oauthproxy.go                |  2 +-
 pkg/encryption/utils.go      | 18 +++++++-----------
 pkg/encryption/utils_test.go |  8 ++++----
 4 files changed, 14 insertions(+), 17 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 19703bcb..f17e3136 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -12,7 +12,8 @@
 - [#2755](https://github.com/oauth2-proxy/oauth2-proxy/pull/2755) feat: add X-Envoy-External-Address as supported header (@bjencks)
 - [#1985](https://github.com/oauth2-proxy/oauth2-proxy/pull/1985) Add support for systemd socket (@isodude)
 - [#2300](https://github.com/oauth2-proxy/oauth2-proxy/pull/2300) Add fix for websocket path rewrite (@rekup)
-- [#2821](https://github.com/oauth2-proxy/oauth2-proxy/pull/2821) feat: add CF-Connecting-IP as supported real ip header
+- [#2821](https://github.com/oauth2-proxy/oauth2-proxy/pull/2821) feat: add CF-Connecting-IP as supported real ip header (@ondrejsika)
+- [#2620](https://github.com/oauth2-proxy/oauth2-proxy/pull/2620) fix: update code_verifier to use recommended method (@vishvananda)
 
 # V7.7.1
 
diff --git a/oauthproxy.go b/oauthproxy.go
index cc2a5ee6..ca9d4e97 100644
--- a/oauthproxy.go
+++ b/oauthproxy.go
@@ -802,7 +802,7 @@ func (p *OAuthProxy) doOAuthStart(rw http.ResponseWriter, req *http.Request, ove
 	)
 	if p.provider.Data().CodeChallengeMethod != "" {
 		codeChallengeMethod = p.provider.Data().CodeChallengeMethod
-		codeVerifier, err = encryption.GenerateRandomASCIIString(96)
+		codeVerifier, err = encryption.GenerateCodeVerifierString(96)
 		if err != nil {
 			logger.Errorf("Unable to build random ASCII string for code verifier: %v", err)
 			p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error())
diff --git a/pkg/encryption/utils.go b/pkg/encryption/utils.go
index 426a3131..df0158b8 100644
--- a/pkg/encryption/utils.go
+++ b/pkg/encryption/utils.go
@@ -7,7 +7,7 @@ import (
 	"encoding/base64"
 	"fmt"
 	"hash"
-	"math/big"
+	"io"
 	"net/http"
 	"strconv"
 	"strings"
@@ -83,17 +83,13 @@ func SignedValue(seed string, key string, value []byte, now time.Time) (string,
 	return cookieVal, nil
 }
 
-func GenerateRandomASCIIString(length int) (string, error) {
-	b := make([]byte, length)
-	charsetLen := new(big.Int).SetInt64(int64(len(asciiCharset)))
-	for i := range b {
-		character, err := rand.Int(rand.Reader, charsetLen)
-		if err != nil {
-			return "", err
-		}
-		b[i] = asciiCharset[character.Int64()]
+// GenerateCodeVerifierString returns a base64 encoded string of n random bytes
+func GenerateCodeVerifierString(n int) (string, error) {
+	data := make([]byte, n)
+	if _, err := io.ReadFull(rand.Reader, data); err != nil {
+		return "", err
 	}
-	return string(b), nil
+	return base64.URLEncoding.WithPadding(base64.NoPadding).EncodeToString(data), nil
 }
 
 func GenerateCodeChallenge(method, codeVerifier string) (string, error) {
diff --git a/pkg/encryption/utils_test.go b/pkg/encryption/utils_test.go
index 9e69df84..0f4320e0 100644
--- a/pkg/encryption/utils_test.go
+++ b/pkg/encryption/utils_test.go
@@ -130,12 +130,12 @@ func TestValidate(t *testing.T) {
 	assert.Equal(t, validValue, expectedValue)
 }
 
-func TestGenerateRandomASCIIString(t *testing.T) {
-	randomString, err := GenerateRandomASCIIString(96)
+func TestGenerateCodeVerifierString(t *testing.T) {
+	randomString, err := GenerateCodeVerifierString(96)
 	assert.NoError(t, err)
 
-	// Only 8-bit characters
-	assert.Equal(t, 96, len([]byte(randomString)))
+	// Should be 128 characters long
+	assert.Equal(t, 128, len([]byte(randomString)))
 
 	// All non-ascii characters removed should still be the original string
 	removedChars := strings.Map(func(r rune) rune {