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

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

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"
@@ -26,6 +30,7 @@ import (
 	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/authentication/basic"
 	"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/validation"
 
 	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/ip"
 	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger"
@@ -274,6 +279,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)
@@ -681,11 +687,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 {
@@ -694,6 +720,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)
 }
 
@@ -717,8 +753,17 @@ func (p *OAuthProxy) OAuthCallback(rw http.ResponseWriter, req *http.Request) {
 		p.ErrorPage(rw, req, http.StatusForbidden, message, message)
 		return
 	}
+	session := middlewareapi.GetRequestScope(req).Session
+	if session == nil {
+		logger.Errorf("Error retrieving session containing code verifier: %v", "")
+		if p.provider.Data().CodeChallengeMethod != "" {
+			// Only error the whole callback if code verifier is required
+			p.ErrorPage(rw, req, http.StatusInternalServerError, "")
+			return
+		}
+	}
 
-	session, err := p.redeemCode(req)
+	session, err = p.redeemCode(req, session.CodeVerifier)
 	if err != nil {
 		logger.Errorf("Error redeeming code during OAuth2 callback: %v", err)
 		p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error())
@@ -785,14 +830,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
 	}
@@ -804,10 +849,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 == "" {
@@ -946,7 +1024,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

@@ -114,7 +114,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)
 }
 

pkg/apis/options/options.go

@@ -59,6 +59,8 @@ type Options struct {
 	SSLInsecureSkipVerify bool     `flag:"ssl-insecure-skip-verify" cfg:"ssl_insecure_skip_verify"`
 	SkipAuthPreflight     bool     `flag:"skip-auth-preflight" cfg:"skip_auth_preflight"`
 	ForceJSONErrors       bool     `flag:"force-json-errors" cfg:"force_json_errors"`
+	// Force PKCE Code Challenges if method is not detected via RFC-8414 metadata document
+	ForceCodeChallengeMethod string `flag:"force-code-challenge-method" cfg:"force_code_challenge_method"`
 
 	SignatureKey    string `flag:"signature-key" cfg:"signature_key"`
 	GCPHealthChecks bool   `flag:"gcp-healthchecks" cfg:"gcp_healthchecks"`
@@ -123,6 +125,7 @@ func NewFlagSet() *pflag.FlagSet {
 	flagSet.Bool("ssl-insecure-skip-verify", false, "skip validation of certificates presented when using HTTPS providers")
 	flagSet.Bool("skip-jwt-bearer-tokens", false, "will skip requests that have verified JWT bearer tokens (default false)")
 	flagSet.Bool("force-json-errors", false, "will force JSON errors instead of HTTP error pages or redirects")
+	flagSet.String("force-code-challenge-method", "", "will force PKCE code challenges with the specified method. Either 'plain' or 'S256'")
 	flagSet.StringSlice("extra-jwt-issuers", []string{}, "if skip-jwt-bearer-tokens is set, a list of extra JWT issuer=audience pairs (where the issuer URL has a .well-known/openid-configuration or a .well-known/jwks.json)")
 
 	flagSet.StringSlice("email-domain", []string{}, "authenticate emails with the specified domain (may be given multiple times). Use * to authenticate any email")

pkg/apis/options/providers.go

@@ -70,6 +70,8 @@ type Provider struct {
 	ApprovalPrompt string `json:"approvalPrompt,omitempty"`
 	// AllowedGroups is a list of restrict logins to members of this group
 	AllowedGroups []string `json:"allowedGroups,omitempty"`
+	// Code challenge methods supported by the Provider
+	CodeChallengeMethods []string `json:"code_challenge_methods_supported,omitempty"`
 
 	// AcrValues is a string of acr values
 	AcrValues string `json:"acrValues,omitempty"`

pkg/apis/sessions/session_state.go

@@ -16,8 +16,9 @@ import (
 
 // SessionState is used to store information about the currently authenticated user session
 type SessionState struct {
-	CreatedAt *time.Time `msgpack:"ca,omitempty"`
-	ExpiresOn *time.Time `msgpack:"eo,omitempty"`
+	Authenticated bool       `msgpack:"au,omitempty"`
+	CreatedAt     *time.Time `msgpack:"ca,omitempty"`
+	ExpiresOn     *time.Time `msgpack:"eo,omitempty"`
 
 	AccessToken  string `msgpack:"at,omitempty"`
 	IDToken      string `msgpack:"it,omitempty"`
@@ -29,6 +30,7 @@ type SessionState struct {
 	User              string   `msgpack:"u,omitempty"`
 	Groups            []string `msgpack:"g,omitempty"`
 	PreferredUsername string   `msgpack:"pu,omitempty"`
+	CodeVerifier      string   `msgpack:"cv,omitempty"`
 
 	// Internal helpers, not serialized
 	Clock clock.Clock `msgpack:"-"`

pkg/validation/options.go

@@ -21,6 +21,11 @@ import (
 	"github.com/oauth2-proxy/oauth2-proxy/v7/providers"
 )
 
+const (
+	CodeChallengeMethodPlain = "plain"
+	CodeChallengeMethodS256  = "S256"
+)
+
 // Validate checks that required options are set and validates those that they
 // are of the correct format
 func Validate(o *options.Options) error {
@@ -132,6 +137,16 @@ func Validate(o *options.Options) error {
 				SkipIssuerCheck: o.Providers[0].OIDCConfig.InsecureSkipIssuerVerification,
 			}))
 
+			type DiscoveryClaims struct {
+				// RFC-8414 Authorization Server Metadata
+				CodeChallengeMethods []string `json:"code_challenge_methods_supported"`
+			}
+			var claims DiscoveryClaims
+			if err := provider.Claims(&claims); err != nil {
+				logger.Errorf("error: failed to parse additional OIDC discovery claims: %v, PKCE must be force enabled to be used.", err)
+			}
+
+			o.Providers[0].CodeChallengeMethods = claims.CodeChallengeMethods
 			o.Providers[0].LoginURL = provider.Endpoint().AuthURL
 			o.Providers[0].RedeemURL = provider.Endpoint().TokenURL
 		}
@@ -210,6 +225,7 @@ func parseProviderInfo(o *options.Options, msgs []string) []string {
 	p.ProfileURL, msgs = parseURL(o.Providers[0].ProfileURL, "profile", msgs)
 	p.ValidateURL, msgs = parseURL(o.Providers[0].ValidateURL, "validate", msgs)
 	p.ProtectedResource, msgs = parseURL(o.Providers[0].ProtectedResource, "resource", msgs)
+	p.CodeChallengeMethod = parseCodeChallengeMethod(o)
 
 	// Make the OIDC options available to all providers that support it
 	p.AllowUnverifiedEmail = o.Providers[0].OIDCConfig.InsecureAllowUnverifiedEmail
@@ -332,6 +348,28 @@ func parseProviderInfo(o *options.Options, msgs []string) []string {
 	return msgs
 }
 
+func stringInSlice(element string, list []string) bool {
+	for _, x := range list {
+		if x == element {
+			return true
+		}
+	}
+	return false
+}
+
+// Pick the most appropriate code challenge method for PKCE
+func parseCodeChallengeMethod(o *options.Options) string {
+	switch {
+	case o.ForceCodeChallengeMethod != "":
+		return o.ForceCodeChallengeMethod
+	case o.Providers[0].CodeChallengeMethods == nil:
+		return ""
+	case stringInSlice(CodeChallengeMethodS256, o.Providers[0].CodeChallengeMethods):
+		return CodeChallengeMethodS256
+	}
+	return CodeChallengeMethodPlain
+}
+
 func parseSignatureKey(o *options.Options, msgs []string) []string {
 	if o.SignatureKey == "" {
 		return msgs

pkg/validation/options_test.go

@@ -302,3 +302,36 @@ func TestProviderCAFilesError(t *testing.T) {
 	assert.Error(t, err)
 	assert.Contains(t, err.Error(), "unable to load provider CA file(s)")
 }
+
+func TestForcedMethodS256(t *testing.T) {
+	options := testOptions()
+	options.ForceCodeChallengeMethod = CodeChallengeMethodS256
+	method := parseCodeChallengeMethod(options)
+
+	assert.Equal(t, CodeChallengeMethodS256, method)
+}
+
+func TestForcedMethodPlain(t *testing.T) {
+	options := testOptions()
+	options.ForceCodeChallengeMethod = CodeChallengeMethodPlain
+	method := parseCodeChallengeMethod(options)
+
+	assert.Equal(t, CodeChallengeMethodPlain, method)
+}
+
+func TestPrefersS256(t *testing.T) {
+	options := testOptions()
+	options.Providers[0].CodeChallengeMethods = []string{"plain", "S256"}
+	method := parseCodeChallengeMethod(options)
+
+	assert.Equal(t, CodeChallengeMethodS256, method)
+}
+
+func TestCanOverwriteS256(t *testing.T) {
+	options := testOptions()
+	options.Providers[0].CodeChallengeMethods = []string{"plain", "S256"}
+	options.ForceCodeChallengeMethod = "plain"
+	method := parseCodeChallengeMethod(options)
+
+	assert.Equal(t, CodeChallengeMethodPlain, method)
+}

providers/adfs.go

@@ -66,11 +66,15 @@ func (p *ADFSProvider) Configure(skipScope bool) {
 
 // GetLoginURL Override to double encode the state parameter. If not query params are lost
 // More info here: https://docs.microsoft.com/en-us/powerapps/maker/portals/configure/configure-saml2-settings
-func (p *ADFSProvider) GetLoginURL(redirectURI, state, nonce string) string {
+func (p *ADFSProvider) GetLoginURL(redirectURI, state, nonce, codeChallenge, codeChallengeMethod string) string {
 	extraParams := url.Values{}
 	if !p.SkipNonce {
 		extraParams.Add("nonce", nonce)
 	}
+	if codeChallenge != "" && codeChallengeMethod != "" {
+		extraParams.Add("code_challenge", codeChallenge)
+		extraParams.Add("code_challenge_method", codeChallengeMethod)
+	}
 	loginURL := makeLoginURL(p.Data(), redirectURI, url.QueryEscape(state), extraParams)
 	if p.skipScope {
 		q := loginURL.Query()

providers/adfs_test.go

@@ -164,7 +164,7 @@ var _ = Describe("ADFS Provider Tests", func() {
 			})
 			p.skipScope = true
 
-			result := p.GetLoginURL("https://example.com/adfs/oauth2/", "", "")
+			result := p.GetLoginURL("https://example.com/adfs/oauth2/", "", "", "", "")
 			Expect(result).NotTo(ContainSubstring("scope="))
 		})
 	})
@@ -185,7 +185,7 @@ var _ = Describe("ADFS Provider Tests", func() {
 				})
 
 				Expect(p.Data().Scope).To(Equal(in.expectedScope))
-				result := p.GetLoginURL("https://example.com/adfs/oauth2/", "", "")
+				result := p.GetLoginURL("https://example.com/adfs/oauth2/", "", "", "", "")
 				Expect(result).To(ContainSubstring("scope=" + url.QueryEscape(in.expectedScope)))
 			},
 			Entry("should add slash", scopeTableInput{