From 66f2793f8b9df5f0a529036edf1203354a492fdf Mon Sep 17 00:00:00 2001 From: Thomas <27960254+thomiceli@users.noreply.github.com> Date: Thu, 11 Jun 2026 01:18:33 +0700 Subject: [PATCH] Add PKCE for OAuth providers (#720) * PKCE Signed-off-by: Thomas Miceli --- go.mod | 4 +- go.sum | 13 +++ internal/auth/oauth/openid.go | 5 ++ internal/auth/oauth/pkce.go | 51 +++++++++++ internal/auth/oauth/provider.go | 2 + internal/web/handlers/auth/auth_test.go | 1 - internal/web/handlers/auth/oauth.go | 2 +- internal/web/handlers/auth/oauth_test.go | 108 +++++++++++++++++++++++ 8 files changed, 183 insertions(+), 3 deletions(-) create mode 100644 internal/auth/oauth/pkce.go delete mode 100644 internal/web/handlers/auth/auth_test.go create mode 100644 internal/web/handlers/auth/oauth_test.go diff --git a/go.mod b/go.mod index 5645ed1..20289f9 100644 --- a/go.mod +++ b/go.mod @@ -20,6 +20,7 @@ require ( github.com/labstack/echo/v4 v4.15.2 github.com/markbates/goth v1.82.0 github.com/meilisearch/meilisearch-go v0.36.3 + github.com/oauth2-proxy/mockoidc v0.0.0-20240214162133-caebfff84d25 github.com/pquerna/otp v1.5.0 github.com/prometheus/client_golang v1.23.2 github.com/rs/zerolog v1.35.1 @@ -30,6 +31,7 @@ require ( github.com/yuin/goldmark-highlighting/v2 v2.0.0-20230729083705-37449abec8cc go.abhg.dev/goldmark/mermaid v0.6.0 golang.org/x/crypto v0.53.0 + golang.org/x/oauth2 v0.36.0 golang.org/x/text v0.38.0 gopkg.in/yaml.v3 v3.0.1 gorm.io/driver/mysql v1.6.0 @@ -72,6 +74,7 @@ require ( github.com/glebarez/go-sqlite v1.22.0 // indirect github.com/go-asn1-ber/asn1-ber v1.5.8-0.20250403174932-29230038a667 // indirect github.com/go-chi/chi/v5 v5.3.0 // indirect + github.com/go-jose/go-jose/v3 v3.0.1 // indirect github.com/go-playground/locales v0.14.1 // indirect github.com/go-playground/universal-translator v0.18.1 // indirect github.com/go-sql-driver/mysql v1.10.0 // indirect @@ -113,7 +116,6 @@ require ( github.com/xrash/smetrics v0.0.0-20250705151800-55b8f293f342 // indirect go.etcd.io/bbolt v1.4.3 // indirect golang.org/x/net v0.55.0 // indirect - golang.org/x/oauth2 v0.36.0 // indirect golang.org/x/sync v0.21.0 // indirect golang.org/x/sys v0.46.0 // indirect golang.org/x/time v0.15.0 // indirect diff --git a/go.sum b/go.sum index 01ebe14..cd6be56 100644 --- a/go.sum +++ b/go.sum @@ -97,6 +97,8 @@ github.com/go-asn1-ber/asn1-ber v1.5.8-0.20250403174932-29230038a667 h1:BP4M0CvQ github.com/go-asn1-ber/asn1-ber v1.5.8-0.20250403174932-29230038a667/go.mod h1:hEBeB/ic+5LoWskz+yKT7vGhhPYkProFKoKdwZRWMe0= github.com/go-chi/chi/v5 v5.3.0 h1:halUjDxhshgXHMrao5bB8eNBXo/rnzwr8m5m36glehM= github.com/go-chi/chi/v5 v5.3.0/go.mod h1:R+tYY2hNuVUUjxoPtqUdgBqevM9s9njzkTLutVsOCto= +github.com/go-jose/go-jose/v3 v3.0.1 h1:pWmKFVtt+Jl0vBZTIpz/eAKwsm6LkIxDVVbFHKkchhA= +github.com/go-jose/go-jose/v3 v3.0.1/go.mod h1:RNkWWRld676jZEYoV3+XK8L2ZnNSvIsxFMht0mSX+u8= github.com/go-json-experiment/json v0.0.0-20250725192818-e39067aee2d2 h1:iizUGZ9pEquQS5jTGkh4AqeeHCMbfbjeb0zMt0aEFzs= github.com/go-json-experiment/json v0.0.0-20250725192818-e39067aee2d2/go.mod h1:TiCD2a1pcmjd7YnhGH0f/zKNcCD06B029pHhzV23c2M= github.com/go-ldap/ldap/v3 v3.4.13 h1:+x1nG9h+MZN7h/lUi5Q3UZ0fJ1GyDQYbPvbuH38baDQ= @@ -127,6 +129,7 @@ github.com/golang-jwt/jwt/v5 v5.3.1 h1:kYf81DTWFe7t+1VvL7eS+jKFVWaUnK9cB1qbwn63Y github.com/golang-jwt/jwt/v5 v5.3.1/go.mod h1:fxCRLWMO43lRc8nhHWY6LGqRcf+1gQWArsqaEUEa5bE= github.com/golang/snappy v1.0.0 h1:Oy607GVXHs7RtbggtPBnr2RmDArIsAefDwvrdWvRhGs= github.com/golang/snappy v1.0.0/go.mod h1:/XxbfmMg8lxefKM7IXC3fBNl/7bRcc72aCRzEWrmP2Q= +github.com/google/go-cmp v0.5.0/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= github.com/google/go-cmp v0.7.0 h1:wk8382ETsv4JYUZwIsn6YpYiWiBsYLSJiTsyBybVuN8= github.com/google/go-cmp v0.7.0/go.mod h1:pXiqmnSA92OHEEa9HXL2W4E7lf9JzCmGVUdgjX3N/iU= github.com/google/go-tpm v0.9.8 h1:slArAR9Ft+1ybZu0lBwpSmpwhRXaa85hWtMinMyRAWo= @@ -217,6 +220,8 @@ github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 h1:C3w9PqII01/Oq github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822/go.mod h1:+n7T8mK8HuQTcFwEeznm/DIxMOiR9yIdICNftLE1DvQ= github.com/ncruces/go-strftime v1.0.0 h1:HMFp8mLCTPp341M/ZnA4qaf7ZlsbTc+miZjCLOFAw7w= github.com/ncruces/go-strftime v1.0.0/go.mod h1:Fwc5htZGVVkseilnfgOVb9mKy6w1naJmn9CehxcKcls= +github.com/oauth2-proxy/mockoidc v0.0.0-20240214162133-caebfff84d25 h1:9bCMuD3TcnjeqjPT2gSlha4asp8NvgcFRYExCaikCxk= +github.com/oauth2-proxy/mockoidc v0.0.0-20240214162133-caebfff84d25/go.mod h1:eDjgYHYDJbPLBLsyZ6qRaugP0mX8vePOhZ5id1fdzJw= github.com/philhofer/fwd v1.2.0 h1:e6DnBTl7vGY+Gz322/ASL4Gyp1FspeMvx1RNDoToZuM= github.com/philhofer/fwd v1.2.0/go.mod h1:RqIHx9QI14HlwKwm98g9Re5prTQ6LdeRQn+gXJFxsJM= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= @@ -241,6 +246,7 @@ github.com/russross/blackfriday/v2 v2.1.0 h1:JIOH55/0cWyOuilr9/qlrm0BSXldqnqwMsf github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI= +github.com/stretchr/testify v1.6.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu7U= github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U= @@ -275,26 +281,33 @@ go.uber.org/mock v0.6.0 h1:hyF9dfmbgIX5EfOdasqLsWD6xqpNZlXblLB/Dbnwv3Y= go.uber.org/mock v0.6.0/go.mod h1:KiVJ4BqZJaMj4svdfmHM0AUx4NJYO8ZNpPnZn1Z+BBU= go.yaml.in/yaml/v2 v2.4.4 h1:tuyd0P+2Ont/d6e2rl3be67goVK4R6deVxCUX5vyPaQ= go.yaml.in/yaml/v2 v2.4.4/go.mod h1:gMZqIpDtDqOfM0uNfy0SkpRhvUryYH0Z6wdMYcacYXQ= +golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w= +golang.org/x/crypto v0.0.0-20190911031432-227b76d455e7/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI= golang.org/x/crypto v0.53.0 h1:QZ4Muo8THX6CizN2vPPd5fBGHyogrdK9fG4wLPFUsto= golang.org/x/crypto v0.53.0/go.mod h1:DNLU434OwVakk9PzuwV8w62mAJpRJL3vsgcfp4Qnsio= golang.org/x/mod v0.36.0 h1:JJjpVx6myfUsUdAzZuOSTTmRE0PfZeNWzzvKrP7amb4= golang.org/x/mod v0.36.0/go.mod h1:moc6ELqsWcOw5Ef3xVprK5ul/MvtVvkIXLziUOICjUQ= +golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg= golang.org/x/net v0.55.0 h1:bcvxaJn3e1U6InsFWt1JUq1aSjnRxLzT2rtD2KfkDF8= golang.org/x/net v0.55.0/go.mod h1:L5U2KuzuOe1lY7Z+aWVIKK6qEeJXnXV9yzGA+WCHJww= golang.org/x/oauth2 v0.36.0 h1:peZ/1z27fi9hUOFCAZaHyrpWG5lwe0RJEEEeH0ThlIs= golang.org/x/oauth2 v0.36.0/go.mod h1:YDBUJMTkDnJS+A4BP4eZBjCqtokkg1hODuPjwiGPO7Q= golang.org/x/sync v0.21.0 h1:HLII4xRRTtCRkxYp4HNFF0Js/Og6q2i++KXbg0gHCwM= golang.org/x/sync v0.21.0/go.mod h1:9xrNwdLfx4jkKbNva9FpL6vEN7evnE43NNNJQ2LF3+0= +golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= +golang.org/x/sys v0.0.0-20190412213103-97732733099d/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.46.0 h1:noSf2Fq6F8DBgS+LysIkx7rIExoNHJsxOAtPp4rthXw= golang.org/x/sys v0.46.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= golang.org/x/term v0.44.0 h1:0rLvDRCtNj0gZkyIXhCyOb2OAzEhLVqc4B+hrsBhrmc= golang.org/x/term v0.44.0/go.mod h1:7ze4MdzUzLXpSAoFP1H0bOI9aXDqveSvatT5vKcFh2Y= +golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.38.0 h1:sXmwo9DwP3OK9EZ7PqAdaooSGozfl/3a6/xJcbzPRhE= golang.org/x/text v0.38.0/go.mod h1:YXZt3QhHUKYT53r2lLKFIVi6Ao1jdzrTR/KQ09qyxF4= golang.org/x/time v0.15.0 h1:bbrp8t3bGUeFOx08pvsMYRTCVSMk89u4tKbNOZbp88U= golang.org/x/time v0.15.0/go.mod h1:Y4YMaQmXwGQZoFaVFk4YpCt4FLQMYKZe9oeV/f4MSno= golang.org/x/tools v0.45.0 h1:18qN3FAooORvApf5XjCXgsuayZOEtXf6JK18I3+ONa8= golang.org/x/tools v0.45.0/go.mod h1:LuUGqqaXcXMEFEruIVJVm5mgDD8vww/z/SR1gQ4uE/0= +golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= google.golang.org/protobuf v1.36.11 h1:fV6ZwhNocDyBLK0dj+fg8ektcVegBBuEolpbTQyBNVE= google.golang.org/protobuf v1.36.11/go.mod h1:HTf+CrKn2C3g5S8VImy6tdcUvCska2kB7j23XfzDpco= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= diff --git a/internal/auth/oauth/openid.go b/internal/auth/oauth/openid.go index 2533b2a..1f7d457 100644 --- a/internal/auth/oauth/openid.go +++ b/internal/auth/oauth/openid.go @@ -8,6 +8,7 @@ import ( "github.com/markbates/goth" "github.com/markbates/goth/gothic" "github.com/markbates/goth/providers/openidConnect" + "github.com/rs/zerolog/log" "github.com/thomiceli/opengist/internal/config" "github.com/thomiceli/opengist/internal/db" "github.com/thomiceli/opengist/internal/web/context" @@ -39,6 +40,10 @@ func (p *OIDCProvider) RegisterProvider() error { } func (p *OIDCProvider) BeginAuthHandler(ctx *context.Context) { + if err := enablePKCE(ctx, OpenIDConnectString); err != nil { + log.Error().Err(err).Msg("Cannot enable PKCE for OIDC provider") + } + ctxValue := gocontext.WithValue(ctx.Request().Context(), gothic.ProviderParamKey, OpenIDConnectString) ctx.SetRequest(ctx.Request().WithContext(ctxValue)) diff --git a/internal/auth/oauth/pkce.go b/internal/auth/oauth/pkce.go new file mode 100644 index 0000000..3ce9113 --- /dev/null +++ b/internal/auth/oauth/pkce.go @@ -0,0 +1,51 @@ +package oauth + +import ( + "github.com/markbates/goth" + "github.com/markbates/goth/providers/openidConnect" + "github.com/thomiceli/opengist/internal/web/context" + "golang.org/x/oauth2" +) + +const codeVerifierSessionKey = "oauth_code_verifier" + +func enablePKCE(ctx *context.Context, providerName string) error { + gothProvider, err := goth.GetProvider(providerName) + if err != nil { + return err + } + + oidcProvider, ok := gothProvider.(*openidConnect.Provider) + if !ok { + return nil + } + + verifier := oauth2.GenerateVerifier() + + sess := ctx.GetSession() + sess.Values[codeVerifierSessionKey] = verifier + ctx.SaveSession(sess) + + oidcProvider.SetAuthCodeOptions(map[string]string{ + "code_challenge": oauth2.S256ChallengeFromVerifier(verifier), + "code_challenge_method": "S256", + }) + + return nil +} + +func injectCodeVerifier(ctx *context.Context) { + sess := ctx.GetSession() + verifier, ok := sess.Values[codeVerifierSessionKey].(string) + if !ok || verifier == "" { + return + } + + req := ctx.Request() + q := req.URL.Query() + q.Set("code_verifier", verifier) + req.URL.RawQuery = q.Encode() + + delete(sess.Values, codeVerifierSessionKey) + ctx.SaveSession(sess) +} diff --git a/internal/auth/oauth/provider.go b/internal/auth/oauth/provider.go index 9b1d511..c22c31f 100644 --- a/internal/auth/oauth/provider.go +++ b/internal/auth/oauth/provider.go @@ -52,6 +52,8 @@ func DefineProvider(provider string, url string) (Provider, error) { } func CompleteUserAuth(ctx *context.Context) (CallbackProvider, error) { + injectCodeVerifier(ctx) + user, err := gothic.CompleteUserAuth(ctx.Response(), ctx.Request()) if err != nil { return nil, err diff --git a/internal/web/handlers/auth/auth_test.go b/internal/web/handlers/auth/auth_test.go deleted file mode 100644 index 426ea7b..0000000 --- a/internal/web/handlers/auth/auth_test.go +++ /dev/null @@ -1 +0,0 @@ -package auth_test diff --git a/internal/web/handlers/auth/oauth.go b/internal/web/handlers/auth/oauth.go index 116b6fb..2e1adbc 100644 --- a/internal/web/handlers/auth/oauth.go +++ b/internal/web/handlers/auth/oauth.go @@ -62,7 +62,7 @@ func Oauth(ctx *context.Context) error { func OauthCallback(ctx *context.Context) error { provider, err := oauth.CompleteUserAuth(ctx) if err != nil { - ctx.AddFlash(ctx.Tr("auth.oauth.no-provider"), "error") + ctx.AddFlash(fmt.Sprintf("%s: %s", ctx.Tr("auth.oauth.no-provider"), err.Error()), "error") return ctx.Redirect(302, "/login") } diff --git a/internal/web/handlers/auth/oauth_test.go b/internal/web/handlers/auth/oauth_test.go new file mode 100644 index 0000000..4e1d7fa --- /dev/null +++ b/internal/web/handlers/auth/oauth_test.go @@ -0,0 +1,108 @@ +package auth_test + +import ( + "encoding/json" + "net/http" + "net/http/cookiejar" + "net/url" + "strings" + "testing" + + "github.com/oauth2-proxy/mockoidc" + "github.com/stretchr/testify/require" + "github.com/thomiceli/opengist/internal/config" + "github.com/thomiceli/opengist/internal/web/test" +) + +type oidcUser struct { + *mockoidc.MockUser +} + +func (u *oidcUser) Userinfo(scope []string) ([]byte, error) { + data, err := u.MockUser.Userinfo(scope) + if err != nil { + return nil, err + } + var claims map[string]interface{} + if err := json.Unmarshal(data, &claims); err != nil { + return nil, err + } + claims["sub"] = u.Subject + return json.Marshal(claims) +} + +func TestOIDCLoginPKCE(t *testing.T) { + m, err := mockoidc.Run() + require.NoError(t, err, "could not start mock OIDC server") + defer func() { _ = m.Shutdown() }() + + s := test.Setup(t) + defer test.Teardown(t) + + config.C.OIDCProviderName = "mock" + config.C.OIDCClientKey = m.ClientID + config.C.OIDCSecret = m.ClientSecret + config.C.OIDCDiscoveryUrl = m.DiscoveryEndpoint() + config.C.OIDCGroupClaimName = "groups" + + base := s.StartHttpServer(t) + + login := func(t *testing.T, tamper func(*url.URL)) (*http.Response, *url.URL) { + t.Helper() + + m.QueueUser(&oidcUser{MockUser: &mockoidc.MockUser{ + Subject: "alice-id", + Email: "alice@example.com", + PreferredUsername: "alice", + EmailVerified: true, + }}) + + jar, err := cookiejar.New(nil) + require.NoError(t, err) + + var authorizeURL *url.URL + client := &http.Client{ + Jar: jar, + CheckRedirect: func(req *http.Request, via []*http.Request) error { + if strings.HasPrefix(req.URL.String(), m.AuthorizationEndpoint()) { + if tamper != nil { + tamper(req.URL) + } + captured := *req.URL + authorizeURL = &captured + } + if len(via) >= 15 { + return http.ErrUseLastResponse + } + return nil + }, + } + + resp, err := client.Get(base + "/oauth/openid-connect") + require.NoError(t, err) + return resp, authorizeURL + } + + t.Run("valid challenge completes login", func(t *testing.T) { + resp, authorizeURL := login(t, nil) + defer resp.Body.Close() + + require.NotNil(t, authorizeURL, "no redirect to the OIDC authorization endpoint was observed") + require.NotEmpty(t, authorizeURL.Query().Get("code_challenge"), "code_challenge missing from the authorization request") + require.Equal(t, "S256", authorizeURL.Query().Get("code_challenge_method")) + + require.Equal(t, http.StatusOK, resp.StatusCode) + require.Equal(t, "/oauth/register", resp.Request.URL.Path) + }) + + t.Run("mismatched challenge fails the token exchange", func(t *testing.T) { + resp, _ := login(t, func(u *url.URL) { + q := u.Query() + q.Set("code_challenge", "this-does-not-match-the-stored-verifier") + u.RawQuery = q.Encode() + }) + defer resp.Body.Close() + + require.Equal(t, "/login", resp.Request.URL.Path) + }) +}