PKCE Support (oauth2-proxy#1541)

* Add the allowed_email_domains and the allowed_groups on the auth_request endpoint + support standard wildcard char for validation with sub-domain and email-domain. Signed-off-by: Valentin Pichard <github@w3st.fr> * Fix provider data initialisation * 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 * Address PR comments by moving pkce generation * Make PKCE opt-in, move to using the Nonce generater for code verifier * Make PKCE opt-in, move to using the Nonce generater for code verifier * Encrypt CodeVerifier in CSRF Token instead of Session - Update Dex for PKCE support - Expose HTTPBin for further use cases * Correct the tests * Move code challenges into extra params * Correct typo in code challenge method Co-authored-by: Joel Speed <Joel.speed@hotmail.co.uk> * Correct the extra space in docs Co-authored-by: Joel Speed <Joel.speed@hotmail.co.uk> * Address changelog and new line nits * Add generated docs Co-authored-by: Valentin Pichard <github@w3st.fr> Co-authored-by: Joel Speed <joel.speed@hotmail.co.uk>
openstandia · Mar 13, 2022 · ce750e9 · ce750e9
1 parent 45ef848
commit ce750e9
Show file tree

Hide file tree

Showing 27 changed files with 250 additions and 59 deletions.
diff --git a/.gitignore b/.gitignore
@@ -18,6 +18,7 @@ c.out
 _obj
 _test
 .idea/
+.vscode/
 
 # Architecture specific extensions/prefixes
 *.[568vq]

diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,11 @@
 
 ## Release Highlights
 
+- [#1361](https://github.com/oauth2-proxy/oauth2-proxy/pull/1541) PKCE Code Challenge Support - RFC-7636 (@braunsonm)
+  - At this time the `--code-challenge-method` flag can be used to enable it with the method of your choice.
+- Parital support for OAuth2 Authorization Server Metadata for detecting code challenge methods (@braunsonm)
+  - A warning will be displayed when your provider advertises support for PKCE but you have not enabled it.
+
 ## Important Notes
 
 ## Breaking Changes
@@ -23,6 +28,7 @@
 - [#1474](https://github.com/oauth2-proxy/oauth2-proxy/pull/1474) Support configuration of minimal acceptable TLS version (@polarctos)
 - [#1545](https://github.com/oauth2-proxy/oauth2-proxy/pull/1545) Fix issue with query string allowed group panic on skip methods (@andytson)
 - [#1286](https://github.com/oauth2-proxy/oauth2-proxy/pull/1286) Add the `allowed_email_domains` and the `allowed_groups` on the `auth_request` + support standard wildcard char for validation with sub-domain and email-domain. (@w3st3ry @armandpicard)
+- [#1361](https://github.com/oauth2-proxy/oauth2-proxy/pull/1541) PKCE Code Challenge Support - RFC-7636 (@braunsonm)
 
 # V7.2.1
 

diff --git a/contrib/local-environment/docker-compose.yaml b/contrib/local-environment/docker-compose.yaml
@@ -29,8 +29,8 @@ services:
       - httpbin
   dex:
     container_name: dex
-    image: quay.io/dexidp/dex:v2.23.0
-    command: serve /dex.yaml
+    image: ghcr.io/dexidp/dex:v2.30.3
+    command: dex serve /dex.yaml
     ports:
       - 4190:4190/tcp
     hostname: dex
@@ -47,6 +47,8 @@ services:
   httpbin:
     container_name: httpbin
     image: kennethreitz/httpbin
+    ports:
+      - 8080:80/tcp
     networks:
       httpbin: {}
   etcd:

diff --git a/docs/docs/configuration/alpha_config.md b/docs/docs/configuration/alpha_config.md
@@ -419,6 +419,7 @@ Provider holds all configuration for a single provider
 | `validateURL` | _string_ | ValidateURL is the access token validation endpoint |
 | `scope` | _string_ | Scope is the OAuth scope specification |
 | `allowedGroups` | _[]string_ | AllowedGroups is a list of restrict logins to members of this group |
+| `force_code_challenge_method` | _string_ | The forced code challenge method |
 
 ### ProviderType
 #### (`string` alias)

diff --git a/docs/docs/configuration/overview.md b/docs/docs/configuration/overview.md
@@ -84,6 +84,7 @@ An example [oauth2-proxy.cfg](https://github.com/oauth2-proxy/oauth2-proxy/blob/
 | `--client-id` | string | the OAuth Client ID, e.g. `"123456.apps.googleusercontent.com"` | |
 | `--client-secret` | string | the OAuth Client Secret | |
 | `--client-secret-file` | string | the file with OAuth Client Secret | |
+| `--code-challenge-method` | string | use PKCE code challenges with the specified method. Either 'plain' or 'S256' (recommended) | |
 | `--config` | string | path to config file | |
 | `--cookie-domain` | string \| list | Optional cookie domains to force cookies to (e.g. `.yourcompany.com`). The longest domain matching the request's host will be used (or the shortest cookie domain if there is no match). | |
 | `--cookie-expire` | duration | expire timeframe for cookie | 168h0m0s |

diff --git a/oauthproxy.go b/oauthproxy.go
@@ -2,6 +2,7 @@ package main
 
 import (
 	"context"
+	"encoding/base64"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -25,6 +26,7 @@ import (
 	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/app/redirect"
 	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/authentication/basic"
 	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/cookies"
+	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/encryption"
 	proxyhttp "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/http"
 	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/util"
 
@@ -680,7 +682,29 @@ func (p *OAuthProxy) doOAuthStart(rw http.ResponseWriter, req *http.Request, ove
 	extraParams := p.provider.Data().LoginURLParams(overrides)
 	prepareNoCache(rw)
 
-	csrf, err := cookies.NewCSRF(p.CookieOptions)
+	var codeChallenge, codeVerifier, codeChallengeMethod string
+	if p.provider.Data().CodeChallengeMethod != "" {
+		codeChallengeMethod = p.provider.Data().CodeChallengeMethod
+		preEncodedCodeVerifier, err := encryption.Nonce(96)
+		if err != nil {
+			logger.Errorf("Unable to build random string: %v", err)
+			p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error())
+			return
+		}
+		codeVerifier = base64.RawURLEncoding.EncodeToString(preEncodedCodeVerifier)
+
+		codeChallenge, err = encryption.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
+		}
+
+		extraParams.Add("code_challenge", codeChallenge)
+		extraParams.Add("code_challenge_method", codeChallengeMethod)
+	}
+
+	csrf, err := cookies.NewCSRF(p.CookieOptions, codeVerifier)
 	if err != nil {
 		logger.Errorf("Error creating CSRF nonce: %v", err)
 		p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error())
@@ -732,24 +756,24 @@ func (p *OAuthProxy) OAuthCallback(rw http.ResponseWriter, req *http.Request) {
 		return
 	}
 
-	session, err := p.redeemCode(req)
+	csrf, err := cookies.LoadCSRFCookie(req, p.CookieOptions)
 	if err != nil {
-		logger.Errorf("Error redeeming code during OAuth2 callback: %v", err)
-		p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error())
+		logger.Println(req, logger.AuthFailure, "Invalid authentication via OAuth2: unable to obtain CSRF cookie")
+		p.ErrorPage(rw, req, http.StatusForbidden, err.Error(), "Login Failed: Unable to find a valid CSRF token. Please try again.")
 		return
 	}
 
-	err = p.enrichSessionState(req.Context(), session)
+	session, err := p.redeemCode(req, csrf.GetCodeVerifier())
 	if err != nil {
-		logger.Errorf("Error creating session during OAuth2 callback: %v", err)
+		logger.Errorf("Error redeeming code during OAuth2 callback: %v", err)
 		p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error())
 		return
 	}
 
-	csrf, err := cookies.LoadCSRFCookie(req, p.CookieOptions)
+	err = p.enrichSessionState(req.Context(), session)
 	if err != nil {
-		logger.PrintAuthf(session.Email, req, logger.AuthFailure, "Invalid authentication via OAuth2: unable to obtain CSRF cookie")
-		p.ErrorPage(rw, req, http.StatusForbidden, err.Error(), "Login Failed: Unable to find a valid CSRF token. Please try again.")
+		logger.Errorf("Error creating session during OAuth2 callback: %v", err)
+		p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error())
 		return
 	}
 
@@ -799,14 +823,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
 	}

diff --git a/oauthproxy_test.go b/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)
 }
 
@@ -405,7 +405,7 @@ func (patTest *PassAccessTokenTest) Close() {
 func (patTest *PassAccessTokenTest) getCallbackEndpoint() (httpCode int, cookie string) {
 	rw := httptest.NewRecorder()
 
-	csrf, err := cookies.NewCSRF(patTest.proxy.CookieOptions)
+	csrf, err := cookies.NewCSRF(patTest.proxy.CookieOptions, "")
 	if err != nil {
 		panic(err)
 	}

diff --git a/pkg/apis/options/legacy_options.go b/pkg/apis/options/legacy_options.go
@@ -519,6 +519,8 @@ type LegacyProvider struct {
 	JWTKey     string `flag:"jwt-key" cfg:"jwt_key"`
 	JWTKeyFile string `flag:"jwt-key-file" cfg:"jwt_key_file"`
 	PubJWKURL  string `flag:"pubjwk-url" cfg:"pubjwk_url"`
+	// PKCE Code Challenge method to use (either S256 or plain)
+	CodeChallengeMethod string `flag:"code-challenge-method" cfg:"force_code_challenge_method"`
 }
 
 func legacyProviderFlagSet() *pflag.FlagSet {
@@ -563,6 +565,7 @@ func legacyProviderFlagSet() *pflag.FlagSet {
 	flagSet.String("scope", "", "OAuth scope specification")
 	flagSet.String("prompt", "", "OIDC prompt")
 	flagSet.String("approval-prompt", "force", "OAuth approval_prompt")
+	flagSet.String("code-challenge-method", "", "use PKCE code challenges with the specified method. Either 'plain' or 'S256'")
 
 	flagSet.String("acr-values", "", "acr values string:  optional")
 	flagSet.String("jwt-key", "", "private key in PEM format used to sign JWT, so that you can say something like -jwt-key=\"${OAUTH2_PROXY_JWT_KEY}\": required by login.gov")
@@ -621,18 +624,19 @@ func (l *LegacyProvider) convert() (Providers, error) {
 	providers := Providers{}
 
 	provider := Provider{
-		ClientID:          l.ClientID,
-		ClientSecret:      l.ClientSecret,
-		ClientSecretFile:  l.ClientSecretFile,
-		Type:              ProviderType(l.ProviderType),
-		CAFiles:           l.ProviderCAFiles,
-		LoginURL:          l.LoginURL,
-		RedeemURL:         l.RedeemURL,
-		ProfileURL:        l.ProfileURL,
-		ProtectedResource: l.ProtectedResource,
-		ValidateURL:       l.ValidateURL,
-		Scope:             l.Scope,
-		AllowedGroups:     l.AllowedGroups,
+		ClientID:            l.ClientID,
+		ClientSecret:        l.ClientSecret,
+		ClientSecretFile:    l.ClientSecretFile,
+		Type:                ProviderType(l.ProviderType),
+		CAFiles:             l.ProviderCAFiles,
+		LoginURL:            l.LoginURL,
+		RedeemURL:           l.RedeemURL,
+		ProfileURL:          l.ProfileURL,
+		ProtectedResource:   l.ProtectedResource,
+		ValidateURL:         l.ValidateURL,
+		Scope:               l.Scope,
+		AllowedGroups:       l.AllowedGroups,
+		CodeChallengeMethod: l.CodeChallengeMethod,
 	}
 
 	// This part is out of the switch section for all providers that support OIDC

diff --git a/pkg/apis/options/providers.go b/pkg/apis/options/providers.go
@@ -76,6 +76,8 @@ type Provider struct {
 	Scope string `json:"scope,omitempty"`
 	// AllowedGroups is a list of restrict logins to members of this group
 	AllowedGroups []string `json:"allowedGroups,omitempty"`
+	// The forced code challenge method
+	CodeChallengeMethod string `json:"force_code_challenge_method,omitempty"`
 }
 
 // ProviderType is used to enumerate the different provider type options

diff --git a/pkg/cookies/csrf.go b/pkg/cookies/csrf.go
@@ -20,6 +20,7 @@ type CSRF interface {
 	HashOIDCNonce() string
 	CheckOAuthState(string) bool
 	CheckOIDCNonce(string) bool
+	GetCodeVerifier() string
 
 	SetSessionNonce(s *sessions.SessionState)
 
@@ -38,24 +39,30 @@ type csrf struct {
 	// is used to mitigate replay attacks.
 	OIDCNonce []byte `msgpack:"n,omitempty"`
 
+	// CodeVerifier holds the unobfuscated PKCE code verification string
+	// which is used to compare the code challenge when exchanging the
+	// authentication code.
+	CodeVerifier string `msgpack:"cv,omitempty"`
+
 	cookieOpts *options.Cookie
 	time       clock.Clock
 }
 
 // NewCSRF creates a CSRF with random nonces
-func NewCSRF(opts *options.Cookie) (CSRF, error) {
-	state, err := encryption.Nonce()
+func NewCSRF(opts *options.Cookie, codeVerifier string) (CSRF, error) {
+	state, err := encryption.Nonce(32)
 	if err != nil {
 		return nil, err
 	}
-	nonce, err := encryption.Nonce()
+	nonce, err := encryption.Nonce(32)
 	if err != nil {
 		return nil, err
 	}
 
 	return &csrf{
-		OAuthState: state,
-		OIDCNonce:  nonce,
+		OAuthState:   state,
+		OIDCNonce:    nonce,
+		CodeVerifier: codeVerifier,
 
 		cookieOpts: opts,
 	}, nil
@@ -71,6 +78,10 @@ func LoadCSRFCookie(req *http.Request, opts *options.Cookie) (CSRF, error) {
 	return decodeCSRFCookie(cookie, opts)
 }
 
+func (c *csrf) GetCodeVerifier() string {
+	return c.CodeVerifier
+}
+
 // HashOAuthState returns the hash of the OAuth state nonce
 func (c *csrf) HashOAuthState() string {
 	return encryption.HashNonce(c.OAuthState)

diff --git a/pkg/cookies/csrf_test.go b/pkg/cookies/csrf_test.go
@@ -33,7 +33,7 @@ var _ = Describe("CSRF Cookie Tests", func() {
 		}
 
 		var err error
-		publicCSRF, err = NewCSRF(cookieOpts)
+		publicCSRF, err = NewCSRF(cookieOpts, "verifier")
 		Expect(err).ToNot(HaveOccurred())
 
 		privateCSRF = publicCSRF.(*csrf)
@@ -44,14 +44,16 @@ var _ = Describe("CSRF Cookie Tests", func() {
 			Expect(privateCSRF.OAuthState).ToNot(BeEmpty())
 			Expect(privateCSRF.OIDCNonce).ToNot(BeEmpty())
 			Expect(privateCSRF.OAuthState).ToNot(Equal(privateCSRF.OIDCNonce))
+			Expect(privateCSRF.CodeVerifier).To(Equal("verifier"))
 		})
 
 		It("makes unique nonces between multiple CSRFs", func() {
-			other, err := NewCSRF(cookieOpts)
+			other, err := NewCSRF(cookieOpts, "verifier")
 			Expect(err).ToNot(HaveOccurred())
 
 			Expect(privateCSRF.OAuthState).ToNot(Equal(other.(*csrf).OAuthState))
 			Expect(privateCSRF.OIDCNonce).ToNot(Equal(other.(*csrf).OIDCNonce))
+			Expect(privateCSRF.CodeVerifier).To(Equal("verifier"))
 		})
 	})
 
@@ -72,6 +74,7 @@ var _ = Describe("CSRF Cookie Tests", func() {
 			Expect(publicCSRF.CheckOIDCNonce(csrfNonce + csrfState)).To(BeFalse())
 			Expect(publicCSRF.CheckOAuthState("")).To(BeFalse())
 			Expect(publicCSRF.CheckOIDCNonce("")).To(BeFalse())
+			Expect(publicCSRF.GetCodeVerifier()).To(Equal("verifier"))
 		})
 	})
 

diff --git a/pkg/encryption/nonce.go b/pkg/encryption/nonce.go
@@ -8,9 +8,9 @@ import (
 	"golang.org/x/crypto/blake2b"
 )
 
-// Nonce generates a random 32-byte slice to be used as a nonce
-func Nonce() ([]byte, error) {
-	b := make([]byte, 32)
+// Nonce generates a random n-byte slice
+func Nonce(length int) ([]byte, error) {
+	b := make([]byte, length)
 	_, err := rand.Read(b)
 	if err != nil {
 		return nil, err

diff --git a/pkg/encryption/utils.go b/pkg/encryption/utils.go
@@ -12,6 +12,11 @@ import (
 	"time"
 )
 
+const (
+	CodeChallengeMethodPlain = "plain"
+	CodeChallengeMethodS256  = "S256"
+)
+
 // SecretBytes attempts to base64 decode the secret, if that fails it treats the secret as binary
 func SecretBytes(secret string) []byte {
 	b, err := base64.RawURLEncoding.DecodeString(strings.TrimRight(secret, "="))
@@ -75,6 +80,18 @@ func SignedValue(seed string, key string, value []byte, now time.Time) (string,
 	return cookieVal, nil
 }
 
+func GenerateCodeChallenge(method, codeVerifier string) (string, error) {
+	switch method {
+	case CodeChallengeMethodPlain:
+		return codeVerifier, nil
+	case CodeChallengeMethodS256:
+		shaSum := sha256.Sum256([]byte(codeVerifier))
+		return base64.RawURLEncoding.EncodeToString(shaSum[:]), nil
+	default:
+		return "", fmt.Errorf("unknown challenge method: %v", method)
+	}
+}
+
 func cookieSignature(signer func() hash.Hash, args ...string) (string, error) {
 	h := hmac.New(signer, []byte(args[0]))
 	for _, arg := range args[1:] {

diff --git a/pkg/validation/sessions.go b/pkg/validation/sessions.go
@@ -51,7 +51,7 @@ func validateRedisSessionStore(o *options.Options) []string {
 		return []string{fmt.Sprintf("unable to initialize a redis client: %v", err)}
 	}
 
-	n, err := encryption.Nonce()
+	n, err := encryption.Nonce(32)
 	if err != nil {
 		return []string{fmt.Sprintf("unable to generate a redis initialization test key: %v", err)}
 	}
-Original file line number
+Diff line change
@@ Expand Up / @@ -18,6 +18,7 @@ c.out @@
     _obj
     _test
     .idea/
+    .vscode/
     # Architecture specific extensions/prefixes
     *.[568vq]
@@ Expand Down @@