From 985939c14552f4da748171e40abf94b2a0b5030e Mon Sep 17 00:00:00 2001 From: oliverpool Date: Mon, 10 Jun 2024 10:04:00 +0200 Subject: [PATCH 1/2] test: pkce only for OpenID Connect --- tests/integration/integration_test.go | 11 ++++- tests/integration/oauth_test.go | 68 ++++++++++++++++++++++----- 2 files changed, 65 insertions(+), 14 deletions(-) diff --git a/tests/integration/integration_test.go b/tests/integration/integration_test.go index f6daf0d14..3877a5381 100644 --- a/tests/integration/integration_test.go +++ b/tests/integration/integration_test.go @@ -54,8 +54,8 @@ import ( gouuid "github.com/google/uuid" "github.com/markbates/goth" "github.com/markbates/goth/gothic" - goth_gitlab "github.com/markbates/goth/providers/github" - goth_github "github.com/markbates/goth/providers/gitlab" + goth_github "github.com/markbates/goth/providers/github" + goth_gitlab "github.com/markbates/goth/providers/gitlab" "github.com/santhosh-tekuri/jsonschema/v5" "github.com/stretchr/testify/assert" ) @@ -325,6 +325,13 @@ func authSourcePayloadOAuth2(name string) map[string]string { } } +func authSourcePayloadOpenIDConnect(name, appURL string) map[string]string { + payload := authSourcePayloadOAuth2(name) + payload["oauth2_provider"] = "openidConnect" + payload["open_id_connect_auto_discovery_url"] = appURL + ".well-known/openid-configuration" + return payload +} + func authSourcePayloadGitLab(name string) map[string]string { payload := authSourcePayloadOAuth2(name) payload["oauth2_provider"] = "gitlab" diff --git a/tests/integration/oauth_test.go b/tests/integration/oauth_test.go index 5d5ea40e8..83a92be83 100644 --- a/tests/integration/oauth_test.go +++ b/tests/integration/oauth_test.go @@ -532,7 +532,8 @@ func TestSignInOAuthCallbackSignIn(t *testing.T) { assert.Greater(t, userAfterLogin.LastLoginUnix, userGitLab.LastLoginUnix) } -func TestSignInOAuthCallbackPKCE(t *testing.T) { +func TestSignInOAuthCallbackWithoutPKCEWhenUnsupported(t *testing.T) { + // https://codeberg.org/forgejo/forgejo/issues/4033 defer tests.PrepareTestEnv(t)() // Setup authentication source @@ -557,20 +558,12 @@ func TestSignInOAuthCallbackPKCE(t *testing.T) { 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) + assert.Empty(t, dest.Query().Get("code_challenge_method")) + assert.Empty(t, dest.Query().Get("code_challenge")) // 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))) - + assert.Empty(t, req.URL.Query().Get("code_verifier")) return goth.User{ Provider: gitlabName, UserID: userGitLabUserID, @@ -583,6 +576,57 @@ func TestSignInOAuthCallbackPKCE(t *testing.T) { unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: userGitLab.ID}) } +func TestSignInOAuthCallbackPKCE(t *testing.T) { + onGiteaRun(t, func(t *testing.T, u *url.URL) { + // Setup authentication source + sourceName := "oidc" + authSource := addAuthSource(t, authSourcePayloadOpenIDConnect(sourceName, u.String())) + // Create a user as if it had been previously been created by the authentication source. + userID := "5678" + user := &user_model.User{ + Name: "oidc.user", + Email: "oidc.user@example.com", + Passwd: "oidc.userpassword", + Type: user_model.UserTypeIndividual, + LoginType: auth_model.OAuth2, + LoginSource: authSource.ID, + LoginName: userID, + } + defer createUser(context.Background(), t, user)() + + // initial redirection (to generate the code_challenge) + session := emptyTestSession(t) + req := NewRequest(t, "GET", fmt.Sprintf("/user/oauth2/%s", sourceName)) + 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: sourceName, + UserID: userID, + Email: user.Email, + }, nil + })() + req = NewRequest(t, "GET", fmt.Sprintf("/user/oauth2/%s/callback?code=XYZ&state=XYZ", sourceName)) + resp = session.MakeRequest(t, req, http.StatusSeeOther) + assert.Equal(t, "/", test.RedirectURL(resp)) + unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: user.ID}) + }) +} + func TestSignInOAuthCallbackRedirectToEscaping(t *testing.T) { defer tests.PrepareTestEnv(t)() From 7d6a0bced9c57068f85bb0cedb574937e4d29567 Mon Sep 17 00:00:00 2001 From: oliverpool Date: Mon, 10 Jun 2024 10:09:35 +0200 Subject: [PATCH 2/2] fix: PKCE only for supported providers --- release-notes/8.0.0/feat/3307.md | 2 +- routers/web/auth/oauth.go | 21 +++++++++++++++++++-- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/release-notes/8.0.0/feat/3307.md b/release-notes/8.0.0/feat/3307.md index 6d7dd0141..e243b6234 100644 --- a/release-notes/8.0.0/feat/3307.md +++ b/release-notes/8.0.0/feat/3307.md @@ -1 +1 @@ -Support [Proof Key for Code Exchange (PKCE - RFC7636)](https://www.rfc-editor.org/rfc/rfc7636) for external login sources using the OAuth2 flow. +Support [Proof Key for Code Exchange (PKCE - RFC7636)](https://www.rfc-editor.org/rfc/rfc7636) for external login using the OpenID Connect authentication source. diff --git a/routers/web/auth/oauth.go b/routers/web/auth/oauth.go index c33c8029c..d133d9dae 100644 --- a/routers/web/auth/oauth.go +++ b/routers/web/auth/oauth.go @@ -44,6 +44,9 @@ import ( "github.com/golang-jwt/jwt/v5" "github.com/markbates/goth" "github.com/markbates/goth/gothic" + "github.com/markbates/goth/providers/fitbit" + "github.com/markbates/goth/providers/openidConnect" + "github.com/markbates/goth/providers/zoom" go_oauth2 "golang.org/x/oauth2" ) @@ -888,7 +891,7 @@ func SignInOAuth(ctx *context.Context) { return } - codeChallenge, err := generateCodeChallenge(ctx) + codeChallenge, err := generateCodeChallenge(ctx, provider) if err != nil { ctx.ServerError("SignIn", fmt.Errorf("could not generate code_challenge: %w", err)) return @@ -1238,7 +1241,21 @@ func handleOAuth2SignIn(ctx *context.Context, source *auth.Source, u *user_model } // 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) { +func generateCodeChallenge(ctx *context.Context, provider string) (codeChallenge string, err error) { + // the `code_verifier` is only forwarded by specific providers + // https://codeberg.org/forgejo/forgejo/issues/4033 + p, ok := goth.GetProviders()[provider] + if !ok { + return "", nil + } + switch p.(type) { + default: + return "", nil + case *openidConnect.Provider, *fitbit.Provider, *zoom.Provider: + // those providers forward the `code_verifier` + // a code_challenge can be generated + } + codeVerifier, err := util.CryptoRandomString(43) // 256/log2(62) = 256 bits of entropy (each char having log2(62) of randomness) if err != nil { return "", err