Merge pull request 'feat: implement PKCE when acting as oauth2 client (for user login)' (#3307) from oliverpool/forgejo:pkce into forgejo

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/3307
Reviewed-by: twenty-panda <twenty-panda@noreply.codeberg.org>
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>

release-notes/8.0.0/feat/3307.md

@@ -0,0 +1 @@
+Support [Proof Key for Code Exchange (PKCE - RFC7636)](https://www.rfc-editor.org/rfc/rfc7636) for external login sources using the OAuth2 flow.

routers/web/auth/oauth.go

@@ -5,6 +5,7 @@ package auth
 
 import (
 	go_context "context"
+	"crypto/sha256"
 	"encoding/base64"
 	"errors"
 	"fmt"
@@ -860,13 +861,19 @@ func SignInOAuth(ctx *context.Context) {
 		return
 	}
 
-	if err = authSource.Cfg.(*oauth2.Source).Callout(ctx.Req, ctx.Resp); err != nil {
+	codeChallenge, err := generateCodeChallenge(ctx)
+	if err != nil {
+		ctx.ServerError("SignIn", fmt.Errorf("could not generate code_challenge: %w", err))
+		return
+	}
+
+	if err = authSource.Cfg.(*oauth2.Source).Callout(ctx.Req, ctx.Resp, codeChallenge); err != nil {
 		if strings.Contains(err.Error(), "no provider for ") {
 			if err = oauth2.ResetOAuth2(ctx); err != nil {
 				ctx.ServerError("SignIn", err)
 				return
 			}
-			if err = authSource.Cfg.(*oauth2.Source).Callout(ctx.Req, ctx.Resp); err != nil {
+			if err = authSource.Cfg.(*oauth2.Source).Callout(ctx.Req, ctx.Resp, codeChallenge); err != nil {
 				ctx.ServerError("SignIn", err)
 			}
 			return
@@ -1203,6 +1210,27 @@ func handleOAuth2SignIn(ctx *context.Context, source *auth.Source, u *user_model
 	ctx.Redirect(setting.AppSubURL + "/user/two_factor")
 }
 
+// generateCodeChallenge stores a code verifier in the session and returns a S256 code challenge for PKCE
+func generateCodeChallenge(ctx *context.Context) (codeChallenge string, err error) {
+	codeVerifier, err := util.CryptoRandomString(43) // 256/log2(62) = 256 bits of entropy (each char having log2(62) of randomness)
+	if err != nil {
+		return "", err
+	}
+	if err = ctx.Session.Set("CodeVerifier", codeVerifier); err != nil {
+		return "", err
+	}
+	return encodeCodeChallenge(codeVerifier)
+}
+
+func encodeCodeChallenge(codeVerifier string) (string, error) {
+	hasher := sha256.New()
+	_, err := io.WriteString(hasher, codeVerifier)
+	codeChallenge := base64.RawURLEncoding.EncodeToString(hasher.Sum(nil))
+	return codeChallenge, err
+}
+
+// OAuth2UserLoginCallback attempts to handle the callback from the OAuth2 provider and if successful
+// login the user
 func oAuth2UserLoginCallback(ctx *context.Context, authSource *auth.Source, request *http.Request, response http.ResponseWriter) (*user_model.User, goth.User, error) {
 	gothUser, err := oAuth2FetchUser(ctx, authSource, request, response)
 	if err != nil {
@@ -1239,7 +1267,9 @@ func oAuth2FetchUser(ctx *context.Context, authSource *auth.Source, request *htt
 	}
 
 	// Proceed to authenticate through goth.
-	gothUser, err := oauth2Source.Callback(request, response)
+	codeVerifier, _ := ctx.Session.Get("CodeVerifier").(string)
+	_ = ctx.Session.Delete("CodeVerifier")
+	gothUser, err := oauth2Source.Callback(request, response, codeVerifier)
 	if err != nil {
 		if err.Error() == "securecookie: the value is too long" || strings.Contains(err.Error(), "Data too long") {
 			log.Error("OAuth2 Provider %s returned too long a token. Current max: %d. Either increase the [OAuth2] MAX_TOKEN_LENGTH or reduce the information returned from the OAuth2 provider", authSource.Name, setting.OAuth2.MaxTokenLength)

routers/web/auth/oauth_test.go

@@ -93,3 +93,10 @@ func TestNewAccessTokenResponse_OIDCToken(t *testing.T) {
 	assert.Equal(t, user.Email, oidcToken.Email)
 	assert.Equal(t, user.IsActive, oidcToken.EmailVerified)
 }
+
+func TestEncodeCodeChallenge(t *testing.T) {
+	// test vector from https://datatracker.ietf.org/doc/html/rfc7636#page-18
+	codeChallenge, err := encodeCodeChallenge("dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk")
+	assert.NoError(t, err)
+	assert.Equal(t, "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM", codeChallenge)
+}

services/auth/source/oauth2/source_callout.go

@@ -5,16 +5,25 @@ package oauth2
 
 import (
 	"net/http"
+	"net/url"
 
 	"github.com/markbates/goth"
 	"github.com/markbates/goth/gothic"
 )
 
 // Callout redirects request/response pair to authenticate against the provider
-func (source *Source) Callout(request *http.Request, response http.ResponseWriter) error {
+func (source *Source) Callout(request *http.Request, response http.ResponseWriter, codeChallengeS256 string) error {
 	// not sure if goth is thread safe (?) when using multiple providers
 	request.Header.Set(ProviderHeaderKey, source.authSource.Name)
 
+	var querySuffix string
+	if codeChallengeS256 != "" {
+		querySuffix = "&" + url.Values{
+			"code_challenge_method": []string{"S256"},
+			"code_challenge":        []string{codeChallengeS256},
+		}.Encode()
+	}
+
 	// don't use the default gothic begin handler to prevent issues when some error occurs
 	// normally the gothic library will write some custom stuff to the response instead of our own nice error page
 	// gothic.BeginAuthHandler(response, request)
@@ -24,17 +33,29 @@ func (source *Source) Callout(request *http.Request, response http.ResponseWrite
 
 	url, err := gothic.GetAuthURL(response, request)
 	if err == nil {
-		http.Redirect(response, request, url, http.StatusTemporaryRedirect)
+		// hacky way to set the code_challenge, but no better way until
+		// https://github.com/markbates/goth/issues/516 is resolved
+		http.Redirect(response, request, url+querySuffix, http.StatusTemporaryRedirect)
 	}
 	return err
 }
 
 // Callback handles OAuth callback, resolve to a goth user and send back to original url
 // this will trigger a new authentication request, but because we save it in the session we can use that
-func (source *Source) Callback(request *http.Request, response http.ResponseWriter) (goth.User, error) {
+func (source *Source) Callback(request *http.Request, response http.ResponseWriter, codeVerifier string) (goth.User, error) {
 	// not sure if goth is thread safe (?) when using multiple providers
 	request.Header.Set(ProviderHeaderKey, source.authSource.Name)
 
+	if codeVerifier != "" {
+		// hacky way to set the code_verifier...
+		// Will be picked up inside CompleteUserAuth: params := req.URL.Query()
+		// https://github.com/markbates/goth/pull/474/files
+		request = request.Clone(request.Context())
+		q := request.URL.Query()
+		q.Add("code_verifier", codeVerifier)
+		request.URL.RawQuery = q.Encode()
+	}
+
 	gothRWMutex.RLock()
 	defer gothRWMutex.RUnlock()
 

tests/integration/oauth_test.go

@@ -6,9 +6,12 @@ package integration
 import (
 	"bytes"
 	"context"
+	"crypto/sha256"
+	"encoding/base64"
 	"fmt"
 	"io"
 	"net/http"
+	"net/url"
 	"testing"
 
 	auth_model "code.gitea.io/gitea/models/auth"
@@ -470,6 +473,57 @@ func TestSignInOAuthCallbackSignIn(t *testing.T) {
 	assert.Greater(t, userAfterLogin.LastLoginUnix, userGitLab.LastLoginUnix)
 }
 
+func TestSignInOAuthCallbackPKCE(t *testing.T) {
+	defer tests.PrepareTestEnv(t)()
+
+	// Setup authentication source
+	gitlabName := "gitlab"
+	gitlab := addAuthSource(t, authSourcePayloadGitLabCustom(gitlabName))
+	// Create a user as if it had been previously been created by the authentication source.
+	userGitLabUserID := "5678"
+	userGitLab := &user_model.User{
+		Name:        "gitlabuser",
+		Email:       "gitlabuser@example.com",
+		Passwd:      "gitlabuserpassword",
+		Type:        user_model.UserTypeIndividual,
+		LoginType:   auth_model.OAuth2,
+		LoginSource: gitlab.ID,
+		LoginName:   userGitLabUserID,
+	}
+	defer createUser(context.Background(), t, userGitLab)()
+
+	// initial redirection (to generate the code_challenge)
+	session := emptyTestSession(t)
+	req := NewRequest(t, "GET", fmt.Sprintf("/user/oauth2/%s", gitlabName))
+	resp := session.MakeRequest(t, req, http.StatusTemporaryRedirect)
+	dest, err := url.Parse(resp.Header().Get("Location"))
+	assert.NoError(t, err)
+	assert.Equal(t, "S256", dest.Query().Get("code_challenge_method"))
+	codeChallenge := dest.Query().Get("code_challenge")
+	assert.NotEmpty(t, codeChallenge)
+
+	// callback (to check the initial code_challenge)
+	defer mockCompleteUserAuth(func(res http.ResponseWriter, req *http.Request) (goth.User, error) {
+		codeVerifier := req.URL.Query().Get("code_verifier")
+		assert.NotEmpty(t, codeVerifier)
+		assert.Greater(t, len(codeVerifier), 40, codeVerifier)
+
+		sha2 := sha256.New()
+		io.WriteString(sha2, codeVerifier)
+		assert.Equal(t, codeChallenge, base64.RawURLEncoding.EncodeToString(sha2.Sum(nil)))
+
+		return goth.User{
+			Provider: gitlabName,
+			UserID:   userGitLabUserID,
+			Email:    userGitLab.Email,
+		}, nil
+	})()
+	req = NewRequest(t, "GET", fmt.Sprintf("/user/oauth2/%s/callback?code=XYZ&state=XYZ", gitlabName))
+	resp = session.MakeRequest(t, req, http.StatusSeeOther)
+	assert.Equal(t, "/", test.RedirectURL(resp))
+	unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: userGitLab.ID})
+}
+
 func TestSignInOAuthCallbackRedirectToEscaping(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()