PKCE Support

Adds Code Challenge PKCE support (RFC-7636) and partial
Authorization Server Metadata (RFC-8414) for detecting PKCE support.

- Introduces new option `--force-code-challenge-method` to force a
specific code challenge method (either `S256` or `plain`) for instances
when the server has not implemented RFC-8414 in order to detect
PKCE support on the discovery document.
- In all other cases, if the PKCE support can be determined during discovery
then the `code_challenge_methods_supported` is used and S256 is always
preferred.
- The force command line argument is helpful with some providers like Azure
who supports PKCE but does not list it in their discovery document yet.
- Initial thought was given to just always attempt PKCE since according to spec
additional URL parameters should be dropped by servers which implemented
OAuth 2, however other projects found cases in the wild where this causes 500
errors by buggy implementations.
See: spring-projects/spring-security#7804 (comment)
- Due to the fact that the `code_verifier` must be saved between the redirect and
callback, sessions are now created when the redirect takes place with `Authenticated: false`.
The session will be recreated and marked as `Authenticated` on callback.
- Individual provider implementations can choose to include or ignore code_challenge
and code_verifier function parameters passed to them

Note: Technically speaking `plain` is not required to be implemented since
oauth2-proxy will always be able to handle S256 and servers MUST implement
S256 support.
> If the client is capable of using "S256", it MUST use "S256", as "S256"
> is Mandatory To Implement (MTI) on the server.  Clients are permitted
> to use "plain" only if they cannot support "S256" for some technical
> reason and know via out-of-band configuration that the server supports
> "plain".
Ref: RFC-7636 Sec 4.2

oauth2-proxy will always use S256 unless the user explicitly forces `plain`.

Fixes oauth2-proxy#1361

Author: braunsonm

.vscode/launch.json

@@ -0,0 +1,16 @@
+{
+    // Use IntelliSense to learn about possible attributes.
+    // Hover to view descriptions of existing attributes.
+    // For more information, visit: https://go.microsoft.com/fwlink/?linkid=830387
+    "version": "0.2.0",
+    "configurations": [
+        {
+            "name": "Launch Package",
+            "type": "go",
+            "request": "launch",
+            "mode": "auto",
+            "program": "${fileDirname}",
+            "args": ["--config", "contrib/local-environment/oauth2-proxy.cfg"]
+        }
+    ]
+}

CHANGELOG.md

@@ -2,6 +2,10 @@
 
 ## Release Highlights
 
+- [#1361](https://github.com/oauth2-proxy/oauth2-proxy/issues/1361) PKCE Code Challenge Support - RFC-7636 (@braunsonm)
+  - The `--force-code-challenge-method` flag can be used when the providers discovery document does not advertise PKCE support.
+- Parital support for OAuth2 Authorization Server Metadata for detecting code challenge methods (@braunsonm)
+
 ## Important Notes
 
 ## Breaking Changes

docs/docs/configuration/overview.md

@@ -104,6 +104,7 @@ An example [oauth2-proxy.cfg](https://github.com/oauth2-proxy/oauth2-proxy/blob/
 | `--flush-interval` | duration | period between flushing response buffers when streaming responses | `"1s"` |
 | `--force-https` | bool | enforce https redirect | `false` |
 | `--force-json-errors` | bool | force JSON errors instead of HTTP error pages or redirects | `false` |
+| `--force-code-challenge-method` | will force PKCE code challenges with the specified method (if not automatically detected by the discovery document). Either 'plain' or 'S256' | |
 | `--banner` | string | custom (html) banner string. Use `"-"` to disable default banner. | |
 | `--footer` | string | custom (html) footer string. Use `"-"` to disable default footer. | |
 | `--github-org` | string | restrict logins to members of this organisation | |

oauthproxy.go

@@ -2,9 +2,13 @@ package main
 
 import (
 	"context"
+	"crypto/rand"
+	"crypto/sha256"
+	"encoding/base64"
 	"encoding/json"
 	"errors"
 	"fmt"
+	"math/big"
 	"net"
 	"net/http"
 	"net/url"
@@ -27,6 +31,7 @@ import (
 	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/cookies"
 	proxyhttp "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/http"
 	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/util"
+	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/validation"
 
 	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/ip"
 	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger"
@@ -119,6 +124,7 @@ func NewOAuthProxy(opts *options.Options, validator func(string) bool) (*OAuthPr
 	if err != nil {
 		return nil, fmt.Errorf("error intiailising provider: %v", err)
 	}
+	provider.Data().CodeChallengeMethod = providers.ParseCodeChallengeMethod(opts)
 
 	pageWriter, err := pagewriter.NewWriter(pagewriter.Opts{
 		TemplatesPath:    opts.Templates.Path,
@@ -280,6 +286,7 @@ func (p *OAuthProxy) buildServeMux(proxyPrefix string) {
 	r := mux.NewRouter().UseEncodedPath()
 	// Everything served by the router must go through the preAuthChain first.
 	r.Use(p.preAuthChain.Then)
+	r.Use(p.sessionChain.Then)
 
 	// Register the robots path writer
 	r.Path(robotsPath).HandlerFunc(p.pageWriter.WriteRobotsTxt)
@@ -687,11 +694,31 @@ func (p *OAuthProxy) OAuthStart(rw http.ResponseWriter, req *http.Request) {
 		return
 	}
 
+	var codeChallenge, codeVerifier, codeChallengeMethod string
+	if p.provider.Data().CodeChallengeMethod != "" {
+		codeChallengeMethod = p.provider.Data().CodeChallengeMethod
+		codeVerifier, err = generateRandomString(128)
+		if err != nil {
+			logger.Errorf("Unable to build random string: %v", err)
+			p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error())
+			return
+		}
+
+		codeChallenge, err = generateCodeChallenge(p.provider.Data().CodeChallengeMethod, codeVerifier)
+		if err != nil {
+			logger.Errorf("Error creating code challenge: %v", err)
+			p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error())
+			return
+		}
+	}
+
 	callbackRedirect := p.getOAuthRedirectURI(req)
 	loginURL := p.provider.GetLoginURL(
 		callbackRedirect,
 		encodeState(csrf.HashOAuthState(), appRedirect),
 		csrf.HashOIDCNonce(),
+		codeChallenge,
+		codeChallengeMethod,
 	)
 
 	if _, err := csrf.SetCookie(rw, req); err != nil {
@@ -700,6 +727,16 @@ func (p *OAuthProxy) OAuthStart(rw http.ResponseWriter, req *http.Request) {
 		return
 	}
 
+	// A session is created with `Authenticated: false` to store the code verifier
+	// for token redemption
+	session := &sessionsapi.SessionState{CodeVerifier: codeVerifier}
+	err = p.SaveSession(rw, req, session)
+	if err != nil {
+		logger.Errorf("Error saving session state for %s: %v", req.RemoteAddr, err)
+		p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error())
+		return
+	}
+
 	http.Redirect(rw, req, loginURL, http.StatusFound)
 }
 
@@ -724,7 +761,20 @@ func (p *OAuthProxy) OAuthCallback(rw http.ResponseWriter, req *http.Request) {
 		return
 	}
 
-	session, err := p.redeemCode(req)
+	session := middlewareapi.GetRequestScope(req).Session
+	var codeVerifier string
+	if session == nil {
+		logger.Errorf("Error retrieving session containing code verifier")
+		if p.provider.Data().CodeChallengeMethod != "" {
+			// Only error the whole callback if code verifier is required
+			p.ErrorPage(rw, req, http.StatusInternalServerError, "")
+			return
+		}
+	} else {
+		codeVerifier = session.CodeVerifier
+	}
+
+	session, err = p.redeemCode(req, codeVerifier)
 	if err != nil {
 		logger.Errorf("Error redeeming code during OAuth2 callback: %v", err)
 		p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error())
@@ -791,14 +841,14 @@ func (p *OAuthProxy) OAuthCallback(rw http.ResponseWriter, req *http.Request) {
 	}
 }
 
-func (p *OAuthProxy) redeemCode(req *http.Request) (*sessionsapi.SessionState, error) {
+func (p *OAuthProxy) redeemCode(req *http.Request, codeVerifier string) (*sessionsapi.SessionState, error) {
 	code := req.Form.Get("code")
 	if code == "" {
 		return nil, providers.ErrMissingCode
 	}
 
 	redirectURI := p.getOAuthRedirectURI(req)
-	s, err := p.provider.Redeem(req.Context(), redirectURI, code)
+	s, err := p.provider.Redeem(req.Context(), redirectURI, code, codeVerifier)
 	if err != nil {
 		return nil, err
 	}
@@ -810,10 +860,43 @@ func (p *OAuthProxy) redeemCode(req *http.Request) (*sessionsapi.SessionState, e
 	if s.ExpiresOn == nil {
 		s.ExpiresIn(p.CookieOptions.Expire)
 	}
+	s.Authenticated = true
 
 	return s, nil
 }
 
+func generateCodeChallenge(method, codeVerifier string) (string, error) {
+	switch method {
+	case validation.CodeChallengeMethodPlain:
+		return codeVerifier, nil
+	case validation.CodeChallengeMethodS256:
+		shaSum := sha256.Sum256([]byte(codeVerifier))
+		return base64.RawURLEncoding.EncodeToString(shaSum[:]), nil
+	default:
+		return "", fmt.Errorf("unknown challenge method: %v", method)
+	}
+}
+
+const runes string = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789.-_~"
+
+// generateRandomString returns a securely generated random ASCII string.
+// It reads random numbers from crypto/rand and searches for printable characters.
+// It will return an error if the system's secure random number generator fails to
+// function correctly, in which case the caller must not continue.
+// From: https://gist.github.com/dopey/c69559607800d2f2f90b1b1ed4e550fb
+func generateRandomString(n int) (string, error) {
+	ret := make([]byte, n)
+	for i := 0; i < n; i++ {
+		num, err := rand.Int(rand.Reader, big.NewInt(int64(len(runes))))
+		if err != nil {
+			return "", err
+		}
+		ret[i] = runes[num.Int64()]
+	}
+
+	return string(ret), nil
+}
+
 func (p *OAuthProxy) enrichSessionState(ctx context.Context, s *sessionsapi.SessionState) error {
 	var err error
 	if s.Email == "" {
@@ -952,7 +1035,7 @@ func (p *OAuthProxy) getAuthenticatedSession(rw http.ResponseWriter, req *http.R
 		return session, nil
 	}
 
-	if session == nil {
+	if session == nil || !session.Authenticated {
 		return nil, ErrNeedsLogin
 	}
 

oauthproxy_test.go

@@ -115,7 +115,7 @@ func Test_redeemCode(t *testing.T) {
 	}
 
 	req := httptest.NewRequest(http.MethodGet, "/", nil)
-	_, err = proxy.redeemCode(req)
+	_, err = proxy.redeemCode(req, "")
 	assert.Equal(t, providers.ErrMissingCode, err)
 }
 
@@ -242,7 +242,8 @@ func TestBasicAuthPassword(t *testing.T) {
 	rw := httptest.NewRecorder()
 	req, _ := http.NewRequest("GET", "/", nil)
 	err = proxy.sessionStore.Save(rw, req, &sessions.SessionState{
-		Email: emailAddress,
+		Email:         emailAddress,
+		Authenticated: true,
 	})
 	assert.NoError(t, err)
 
@@ -285,11 +286,12 @@ func TestPassGroupsHeadersWithGroups(t *testing.T) {
 	groups := []string{"a", "b"}
 	created := time.Now()
 	session := &sessions.SessionState{
-		User:        userName,
-		Groups:      groups,
-		Email:       emailAddress,
-		AccessToken: "oauth_token",
-		CreatedAt:   &created,
+		User:          userName,
+		Groups:        groups,
+		Email:         emailAddress,
+		AccessToken:   "oauth_token",
+		CreatedAt:     &created,
+		Authenticated: true,
 	}
 
 	proxy, err := NewOAuthProxy(opts, func(email string) bool {
@@ -930,28 +932,31 @@ func TestUserInfoEndpointAccepted(t *testing.T) {
 		{
 			name: "Full session",
 			session: &sessions.SessionState{
-				User:        "john.doe",
-				Email:       "[email protected]",
-				Groups:      []string{"example", "groups"},
-				AccessToken: "my_access_token",
+				User:          "john.doe",
+				Email:         "[email protected]",
+				Groups:        []string{"example", "groups"},
+				AccessToken:   "my_access_token",
+				Authenticated: true,
 			},
 			expectedResponse: "{\"user\":\"john.doe\",\"email\":\"[email protected]\",\"groups\":[\"example\",\"groups\"]}\n",
 		},
 		{
 			name: "Minimal session",
 			session: &sessions.SessionState{
-				User:   "john.doe",
-				Email:  "[email protected]",
-				Groups: []string{"example", "groups"},
+				User:          "john.doe",
+				Email:         "[email protected]",
+				Groups:        []string{"example", "groups"},
+				Authenticated: true,
 			},
 			expectedResponse: "{\"user\":\"john.doe\",\"email\":\"[email protected]\",\"groups\":[\"example\",\"groups\"]}\n",
 		},
 		{
 			name: "No groups",
 			session: &sessions.SessionState{
-				User:        "john.doe",
-				Email:       "[email protected]",
-				AccessToken: "my_access_token",
+				User:          "john.doe",
+				Email:         "[email protected]",
+				AccessToken:   "my_access_token",
+				Authenticated: true,
 			},
 			expectedResponse: "{\"user\":\"john.doe\",\"email\":\"[email protected]\"}\n",
 		},
@@ -963,6 +968,7 @@ func TestUserInfoEndpointAccepted(t *testing.T) {
 				Email:             "[email protected]",
 				Groups:            []string{"example", "groups"},
 				AccessToken:       "my_access_token",
+				Authenticated:     true,
 			},
 			expectedResponse: "{\"user\":\"john.doe\",\"email\":\"[email protected]\",\"groups\":[\"example\",\"groups\"],\"preferredUsername\":\"john\"}\n",
 		},
@@ -1024,7 +1030,7 @@ func TestAuthOnlyEndpointAccepted(t *testing.T) {
 
 	created := time.Now()
 	startSession := &sessions.SessionState{
-		Email: "[email protected]", AccessToken: "my_access_token", CreatedAt: &created}
+		Email: "[email protected]", AccessToken: "my_access_token", CreatedAt: &created, Authenticated: true}
 	err = test.SaveSession(startSession)
 	assert.NoError(t, err)
 
@@ -1154,7 +1160,7 @@ func TestAuthOnlyEndpointSetXAuthRequestHeaders(t *testing.T) {
 
 	created := time.Now()
 	startSession := &sessions.SessionState{
-		User: "oauth_user", Groups: []string{"oauth_groups"}, Email: "[email protected]", AccessToken: "oauth_token", CreatedAt: &created}
+		User: "oauth_user", Groups: []string{"oauth_groups"}, Email: "[email protected]", AccessToken: "oauth_token", CreatedAt: &created, Authenticated: true}
 	err = pcTest.SaveSession(startSession)
 	assert.NoError(t, err)
 
@@ -1247,7 +1253,7 @@ func TestAuthOnlyEndpointSetBasicAuthTrueRequestHeaders(t *testing.T) {
 
 	created := time.Now()
 	startSession := &sessions.SessionState{
-		User: "oauth_user", Email: "[email protected]", AccessToken: "oauth_token", CreatedAt: &created}
+		User: "oauth_user", Email: "[email protected]", AccessToken: "oauth_token", CreatedAt: &created, Authenticated: true}
 	err = pcTest.SaveSession(startSession)
 	assert.NoError(t, err)
 
@@ -1327,7 +1333,7 @@ func TestAuthOnlyEndpointSetBasicAuthFalseRequestHeaders(t *testing.T) {
 
 	created := time.Now()
 	startSession := &sessions.SessionState{
-		User: "oauth_user", Email: "[email protected]", AccessToken: "oauth_token", CreatedAt: &created}
+		User: "oauth_user", Email: "[email protected]", AccessToken: "oauth_token", CreatedAt: &created, Authenticated: true}
 	err = pcTest.SaveSession(startSession)
 	assert.NoError(t, err)
 
@@ -1502,7 +1508,7 @@ func (st *SignatureTest) MakeRequestWithExpectedKey(method, body, key string) er
 	req.Header = st.header
 
 	state := &sessions.SessionState{
-		Email: "[email protected]", AccessToken: "my_access_token"}
+		Email: "[email protected]", AccessToken: "my_access_token", Authenticated: true}
 	err = proxy.SaveSession(st.rw, req, state)
 	if err != nil {
 		return err
@@ -2451,10 +2457,11 @@ func TestProxyAllowedGroups(t *testing.T) {
 			created := time.Now()
 
 			session := &sessions.SessionState{
-				Groups:      tt.groups,
-				Email:       emailAddress,
-				AccessToken: "oauth_token",
-				CreatedAt:   &created,
+				Groups:        tt.groups,
+				Email:         emailAddress,
+				AccessToken:   "oauth_token",
+				CreatedAt:     &created,
+				Authenticated: true,
 			}
 
 			upstreamServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
@@ -2587,10 +2594,11 @@ func TestAuthOnlyAllowedGroups(t *testing.T) {
 			created := time.Now()
 
 			session := &sessions.SessionState{
-				Groups:      tc.groups,
-				Email:       emailAddress,
-				AccessToken: "oauth_token",
-				CreatedAt:   &created,
+				Groups:        tc.groups,
+				Email:         emailAddress,
+				AccessToken:   "oauth_token",
+				CreatedAt:     &created,
+				Authenticated: true,
 			}
 
 			test, err := NewAuthOnlyEndpointTest(tc.querystring, func(opts *options.Options) {
@@ -2683,10 +2691,11 @@ func TestAuthOnlyAllowedGroupsWithSkipMethods(t *testing.T) {
 			if tc.withSession {
 				created := time.Now()
 				session := &sessions.SessionState{
-					Groups:      tc.groups,
-					Email:       "test",
-					AccessToken: "oauth_token",
-					CreatedAt:   &created,
+					Groups:        tc.groups,
+					Email:         "test",
+					AccessToken:   "oauth_token",
+					CreatedAt:     &created,
+					Authenticated: true,
 				}
 				err = test.SaveSession(session)
 			}