From e1d93950ad050b9a84b2612894e4fcc658bbd5bd Mon Sep 17 00:00:00 2001 From: oliverpool Date: Thu, 18 Apr 2024 09:55:08 +0200 Subject: [PATCH] feat: implement PKCE when acting as oauth2 client (for user login) Closes #2766 --- release-notes/8.0.0/feat/3307.md | 1 + routers/web/auth/oauth.go | 36 +++++++++++-- routers/web/auth/oauth_test.go | 7 +++ services/auth/source/oauth2/source_callout.go | 27 ++++++++-- tests/integration/oauth_test.go | 54 +++++++++++++++++++ 5 files changed, 119 insertions(+), 6 deletions(-) create mode 100644 release-notes/8.0.0/feat/3307.md diff --git a/release-notes/8.0.0/feat/3307.md b/release-notes/8.0.0/feat/3307.md new file mode 100644 index 0000000000..6d7dd01415 --- /dev/null +++ b/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. diff --git a/routers/web/auth/oauth.go b/routers/web/auth/oauth.go index b48345684b..3ad7a4738e 100644 --- a/routers/web/auth/oauth.go +++ b/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) diff --git a/routers/web/auth/oauth_test.go b/routers/web/auth/oauth_test.go index 4339d9d1eb..3726daee93 100644 --- a/routers/web/auth/oauth_test.go +++ b/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) +} diff --git a/services/auth/source/oauth2/source_callout.go b/services/auth/source/oauth2/source_callout.go index 8d70bee248..f95a80fc19 100644 --- a/services/auth/source/oauth2/source_callout.go +++ b/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() diff --git a/tests/integration/oauth_test.go b/tests/integration/oauth_test.go index 1da1c6f9c0..46beddb5f3 100644 --- a/tests/integration/oauth_test.go +++ b/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)()