From bbc1a9a1d8d04e7be7cedb6e7d69ce3a5331de56 Mon Sep 17 00:00:00 2001 From: Kyle Carberry Date: Mon, 16 Jan 2023 16:06:39 -0600 Subject: [PATCH] fix: use `UserInfo` endpoint with OIDC (#5735) This resolves a user issue surfaced in Discord: https://discord.com/channels/747933592273027093/1064566338875576361/1064566338875576361 Both methods of obtaining claims need to be used according to the OIDC specification. --- cli/server.go | 1 + coderd/coderdtest/coderdtest.go | 19 +++++++++- coderd/userauth.go | 30 +++++++++++++++ coderd/userauth_test.go | 65 +++++++++++++++++++++------------ 4 files changed, 90 insertions(+), 25 deletions(-) diff --git a/cli/server.go b/cli/server.go index bd8b7fcdf4..5165478f21 100644 --- a/cli/server.go +++ b/cli/server.go @@ -543,6 +543,7 @@ func Server(vip *viper.Viper, newAPI func(context.Context, *coderd.Options) (*co Endpoint: oidcProvider.Endpoint(), Scopes: cfg.OIDC.Scopes.Value, }, + Provider: oidcProvider, Verifier: oidcProvider.Verifier(&oidc.Config{ ClientID: cfg.OIDC.ClientID.Value, }), diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index c4bc90a9b7..1d4a268b86 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -887,7 +887,23 @@ func (o *OIDCConfig) EncodeClaims(t *testing.T, claims jwt.MapClaims) string { return base64.StdEncoding.EncodeToString([]byte(signed)) } -func (o *OIDCConfig) OIDCConfig() *coderd.OIDCConfig { +func (o *OIDCConfig) OIDCConfig(t *testing.T, userInfoClaims jwt.MapClaims) *coderd.OIDCConfig { + // By default, the provider can be empty. + // This means it won't support any endpoints! + provider := &oidc.Provider{} + if userInfoClaims != nil { + resp, err := json.Marshal(userInfoClaims) + require.NoError(t, err) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write(resp) + })) + t.Cleanup(srv.Close) + cfg := &oidc.ProviderConfig{ + UserInfoURL: srv.URL, + } + provider = cfg.NewProvider(context.Background()) + } return &coderd.OIDCConfig{ OAuth2Config: o, Verifier: oidc.NewVerifier(o.issuer, &oidc.StaticKeySet{ @@ -895,6 +911,7 @@ func (o *OIDCConfig) OIDCConfig() *coderd.OIDCConfig { }, &oidc.Config{ SkipClientIDCheck: true, }), + Provider: provider, UsernameField: "preferred_username", } } diff --git a/coderd/userauth.go b/coderd/userauth.go index 9076213c12..3fbc1c8f00 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -204,6 +204,7 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) { type OIDCConfig struct { httpmw.OAuth2Config + Provider *oidc.Provider Verifier *oidc.IDTokenVerifier // EmailDomains are the domains to enforce when a user authenticates. EmailDomain []string @@ -258,6 +259,35 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { }) return } + + // Not all claims are necessarily embedded in the `id_token`. + // In GitLab, the username is left empty and must be fetched in UserInfo. + // + // The OIDC specification says claims can be in either place, so we fetch + // user info and merge the two claim sets to be sure we have all of + // the correct data. + userInfo, err := api.OIDCConfig.Provider.UserInfo(ctx, oauth2.StaticTokenSource(state.Token)) + if err == nil { + userInfoClaims := map[string]interface{}{} + err = userInfo.Claims(&userInfoClaims) + if err != nil { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Failed to unmarshal user info claims.", + Detail: err.Error(), + }) + return + } + for k, v := range userInfoClaims { + claims[k] = v + } + } else if !strings.Contains(err.Error(), "user info endpoint is not supported by this provider") { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Failed to obtain user information claims.", + Detail: "The OIDC provider returned no claims as part of the `id_token`. The attempt to fetch claims via the UserInfo endpoint failed: " + err.Error(), + }) + return + } + usernameRaw, ok := claims[api.OIDCConfig.UsernameField] var username string if ok { diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go index c1a0f96ac8..0cb16afa33 100644 --- a/coderd/userauth_test.go +++ b/coderd/userauth_test.go @@ -480,7 +480,8 @@ func TestUserOIDC(t *testing.T) { for _, tc := range []struct { Name string - Claims jwt.MapClaims + IDTokenClaims jwt.MapClaims + UserInfoClaims jwt.MapClaims AllowSignups bool EmailDomain []string Username string @@ -489,7 +490,7 @@ func TestUserOIDC(t *testing.T) { IgnoreEmailVerified bool }{{ Name: "EmailOnly", - Claims: jwt.MapClaims{ + IDTokenClaims: jwt.MapClaims{ "email": "kyle@kwc.io", }, AllowSignups: true, @@ -497,7 +498,7 @@ func TestUserOIDC(t *testing.T) { Username: "kyle", }, { Name: "EmailNotVerified", - Claims: jwt.MapClaims{ + IDTokenClaims: jwt.MapClaims{ "email": "kyle@kwc.io", "email_verified": false, }, @@ -505,7 +506,7 @@ func TestUserOIDC(t *testing.T) { StatusCode: http.StatusForbidden, }, { Name: "EmailNotAString", - Claims: jwt.MapClaims{ + IDTokenClaims: jwt.MapClaims{ "email": 3.14159, "email_verified": false, }, @@ -513,7 +514,7 @@ func TestUserOIDC(t *testing.T) { StatusCode: http.StatusBadRequest, }, { Name: "EmailNotVerifiedIgnored", - Claims: jwt.MapClaims{ + IDTokenClaims: jwt.MapClaims{ "email": "kyle@kwc.io", "email_verified": false, }, @@ -523,7 +524,7 @@ func TestUserOIDC(t *testing.T) { IgnoreEmailVerified: true, }, { Name: "NotInRequiredEmailDomain", - Claims: jwt.MapClaims{ + IDTokenClaims: jwt.MapClaims{ "email": "kyle@kwc.io", "email_verified": true, }, @@ -534,7 +535,7 @@ func TestUserOIDC(t *testing.T) { StatusCode: http.StatusForbidden, }, { Name: "EmailDomainCaseInsensitive", - Claims: jwt.MapClaims{ + IDTokenClaims: jwt.MapClaims{ "email": "kyle@KWC.io", "email_verified": true, }, @@ -544,20 +545,20 @@ func TestUserOIDC(t *testing.T) { }, StatusCode: http.StatusTemporaryRedirect, }, { - Name: "EmptyClaims", - Claims: jwt.MapClaims{}, - AllowSignups: true, - StatusCode: http.StatusBadRequest, + Name: "EmptyClaims", + IDTokenClaims: jwt.MapClaims{}, + AllowSignups: true, + StatusCode: http.StatusBadRequest, }, { Name: "NoSignups", - Claims: jwt.MapClaims{ + IDTokenClaims: jwt.MapClaims{ "email": "kyle@kwc.io", "email_verified": true, }, StatusCode: http.StatusForbidden, }, { Name: "UsernameFromEmail", - Claims: jwt.MapClaims{ + IDTokenClaims: jwt.MapClaims{ "email": "kyle@kwc.io", "email_verified": true, }, @@ -566,7 +567,7 @@ func TestUserOIDC(t *testing.T) { StatusCode: http.StatusTemporaryRedirect, }, { Name: "UsernameFromClaims", - Claims: jwt.MapClaims{ + IDTokenClaims: jwt.MapClaims{ "email": "kyle@kwc.io", "email_verified": true, "preferred_username": "hotdog", @@ -578,7 +579,7 @@ func TestUserOIDC(t *testing.T) { // Services like Okta return the email as the username: // https://developer.okta.com/docs/reference/api/oidc/#base-claims-always-present Name: "UsernameAsEmail", - Claims: jwt.MapClaims{ + IDTokenClaims: jwt.MapClaims{ "email": "kyle@kwc.io", "email_verified": true, "preferred_username": "kyle@kwc.io", @@ -589,7 +590,7 @@ func TestUserOIDC(t *testing.T) { }, { // See: https://github.com/coder/coder/issues/4472 Name: "UsernameIsEmail", - Claims: jwt.MapClaims{ + IDTokenClaims: jwt.MapClaims{ "preferred_username": "kyle@kwc.io", }, Username: "kyle", @@ -597,23 +598,37 @@ func TestUserOIDC(t *testing.T) { StatusCode: http.StatusTemporaryRedirect, }, { Name: "WithPicture", - Claims: jwt.MapClaims{ - "email": "kyle@kwc.io", - "email_verified": true, - "username": "kyle", - "picture": "/example.png", + IDTokenClaims: jwt.MapClaims{ + "email": "kyle@kwc.io", + "email_verified": true, + "preferred_username": "kyle", + "picture": "/example.png", }, Username: "kyle", AllowSignups: true, AvatarURL: "/example.png", StatusCode: http.StatusTemporaryRedirect, + }, { + Name: "WithUserInfoClaims", + IDTokenClaims: jwt.MapClaims{ + "email": "kyle@kwc.io", + "email_verified": true, + }, + UserInfoClaims: jwt.MapClaims{ + "preferred_username": "potato", + "picture": "/example.png", + }, + Username: "potato", + AllowSignups: true, + AvatarURL: "/example.png", + StatusCode: http.StatusTemporaryRedirect, }} { tc := tc t.Run(tc.Name, func(t *testing.T) { t.Parallel() conf := coderdtest.NewOIDCConfig(t, "") - config := conf.OIDCConfig() + config := conf.OIDCConfig(t, tc.UserInfoClaims) config.AllowSignups = tc.AllowSignups config.EmailDomain = tc.EmailDomain config.IgnoreEmailVerified = tc.IgnoreEmailVerified @@ -621,7 +636,7 @@ func TestUserOIDC(t *testing.T) { client := coderdtest.New(t, &coderdtest.Options{ OIDCConfig: config, }) - resp := oidcCallback(t, client, conf.EncodeClaims(t, tc.Claims)) + resp := oidcCallback(t, client, conf.EncodeClaims(t, tc.IDTokenClaims)) assert.Equal(t, tc.StatusCode, resp.StatusCode) ctx, _ := testutil.Context(t) @@ -647,7 +662,7 @@ func TestUserOIDC(t *testing.T) { conf := coderdtest.NewOIDCConfig(t, "") - config := conf.OIDCConfig() + config := conf.OIDCConfig(t, nil) config.AllowSignups = true client := coderdtest.New(t, &coderdtest.Options{ @@ -705,6 +720,7 @@ func TestUserOIDC(t *testing.T) { verifier := oidc.NewVerifier("", &oidc.StaticKeySet{ PublicKeys: []crypto.PublicKey{}, }, &oidc.Config{}) + provider := &oidc.Provider{} client := coderdtest.New(t, &coderdtest.Options{ OIDCConfig: &coderd.OIDCConfig{ @@ -715,6 +731,7 @@ func TestUserOIDC(t *testing.T) { "id_token": "invalid", }), }, + Provider: provider, Verifier: verifier, }, })