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

@@ -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
 

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

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) {
+// GenerateCodeVerifierString returns a base64 encoded string of n random bytes
-	b := make([]byte, length)
+func GenerateCodeVerifierString(n int) (string, error) {
-	charsetLen := new(big.Int).SetInt64(int64(len(asciiCharset)))
+	data := make([]byte, n)
-	for i := range b {
+	if _, err := io.ReadFull(rand.Reader, data); err != nil {
-		character, err := rand.Int(rand.Reader, charsetLen)
+		return "", err
-		if err != nil {
-			return "", err
-		}
-		b[i] = asciiCharset[character.Int64()]
 	}
-	return string(b), nil
+	return base64.URLEncoding.WithPadding(base64.NoPadding).EncodeToString(data), nil
 }
 
 func GenerateCodeChallenge(method, codeVerifier string) (string, error) {

pkg/encryption/utils_test.go

@@ -130,12 +130,12 @@ func TestValidate(t *testing.T) {
 	assert.Equal(t, validValue, expectedValue)
 }
 
-func TestGenerateRandomASCIIString(t *testing.T) {
+func TestGenerateCodeVerifierString(t *testing.T) {
-	randomString, err := GenerateRandomASCIIString(96)
+	randomString, err := GenerateCodeVerifierString(96)
 	assert.NoError(t, err)
 
-	// Only 8-bit characters
+	// Should be 128 characters long
-	assert.Equal(t, 96, len([]byte(randomString)))
+	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 {