Skip to content

Commit 9c4ccd7

Browse files
johnstcnjohnstcn
authored
fix(coderd)!: add CODER_OIDC_IGNORE_USERINFO configuration option (#6922)
* add CODER_OIDC_IGNORE_USERINFO option * chore: update docs for CODER_OIDC_IGNORE_USERINFO w.r.t ADFS * fix!: codersdk: fix incorrectly named OIDC_GROUP_MAPPING -> CODER_OIDC_GROUP_MAPPING
1 parent 929589d commit 9c4ccd7

File tree

13 files changed

+209
-42
lines changed

13 files changed

+209
-42
lines changed

‎cli/server.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -731,6 +731,7 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd.
731731
UsernameField: cfg.OIDC.UsernameField.String(),
732732
EmailField: cfg.OIDC.EmailField.String(),
733733
AuthURLParams: cfg.OIDC.AuthURLParams.Value,
734+
IgnoreUserInfo: cfg.OIDC.IgnoreUserInfo.Value(),
734735
GroupField: cfg.OIDC.GroupField.String(),
735736
GroupMapping: cfg.OIDC.GroupMapping.Value,
736737
SignInText: cfg.OIDC.SignInText.String(),

‎cli/server_test.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1086,6 +1086,7 @@ func TestServer(t *testing.T) {
10861086
require.Equal(t, "preferred_username", deploymentConfig.Values.OIDC.UsernameField.Value())
10871087
require.Equal(t, "email", deploymentConfig.Values.OIDC.EmailField.Value())
10881088
require.Equal(t, map[string]string{"access_type": "offline"}, deploymentConfig.Values.OIDC.AuthURLParams.Value)
1089+
require.False(t, deploymentConfig.Values.OIDC.IgnoreUserInfo.Value())
10891090
require.Empty(t, deploymentConfig.Values.OIDC.GroupField.Value())
10901091
require.Empty(t, deploymentConfig.Values.OIDC.GroupMapping.Value)
10911092
require.Equal(t, "OpenID Connect", deploymentConfig.Values.OIDC.SignInText.Value())
@@ -1125,6 +1126,7 @@ func TestServer(t *testing.T) {
11251126
"--oidc-username-field", "not_preferred_username",
11261127
"--oidc-email-field", "not_email",
11271128
"--oidc-auth-url-params", `{"prompt":"consent"}`,
1129+
"--oidc-ignore-userinfo",
11281130
"--oidc-group-field", "serious_business_unit",
11291131
"--oidc-group-mapping", `{"serious_business_unit": "serious_business_unit"}`,
11301132
"--oidc-sign-in-text", "Sign In With Coder",
@@ -1169,6 +1171,7 @@ func TestServer(t *testing.T) {
11691171
require.True(t, deploymentConfig.Values.OIDC.IgnoreEmailVerified.Value())
11701172
require.Equal(t, "not_preferred_username", deploymentConfig.Values.OIDC.UsernameField.Value())
11711173
require.Equal(t, "not_email", deploymentConfig.Values.OIDC.EmailField.Value())
1174+
require.True(t, deploymentConfig.Values.OIDC.IgnoreUserInfo.Value())
11721175
require.Equal(t, map[string]string{"prompt": "consent"}, deploymentConfig.Values.OIDC.AuthURLParams.Value)
11731176
require.Equal(t, "serious_business_unit", deploymentConfig.Values.OIDC.GroupField.Value())
11741177
require.Equal(t, map[string]string{"serious_business_unit": "serious_business_unit"}, deploymentConfig.Values.OIDC.GroupMapping.Value)

‎cli/testdata/coder_server_--help.golden

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -280,13 +280,17 @@ can safely ignore these settings.
280280
Change the OIDC default 'groups' claim field. By default, will be
281281
'groups' if present in the oidc scopes argument.
282282

283-
--oidc-group-mapping struct[map[string]string], $OIDC_GROUP_MAPPING (default: {})
283+
--oidc-group-mapping struct[map[string]string], $CODER_OIDC_GROUP_MAPPING (default: {})
284284
A map of OIDC group IDs and the group in Coder it should map to. This
285285
is useful for when OIDC providers only return group IDs.
286286

287287
--oidc-ignore-email-verified bool, $CODER_OIDC_IGNORE_EMAIL_VERIFIED
288288
Ignore the email_verified claim from the upstream provider.
289289

290+
--oidc-ignore-userinfo bool, $CODER_OIDC_IGNORE_USERINFO (default: false)
291+
Ignore the userinfo endpoint and only use the ID token for user
292+
information.
293+
290294
--oidc-issuer-url string, $CODER_OIDC_ISSUER_URL
291295
Issuer URL to use for Login with OIDC.
292296

‎coderd/apidoc/docs.go

Lines changed: 3 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/apidoc/swagger.json

Lines changed: 3 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/userauth.go

Lines changed: 91 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"fmt"
88
"net/http"
99
"net/mail"
10+
"sort"
1011
"strconv"
1112
"strings"
1213

@@ -483,6 +484,11 @@ type OIDCConfig struct {
483484
// AuthURLParams are additional parameters to be passed to the OIDC provider
484485
// when requesting an access token.
485486
AuthURLParams map[string]string
487+
// IgnoreUserInfo causes Coder to only use claims from the ID token to
488+
// process OIDC logins. This is useful if the OIDC provider does not
489+
// support the userinfo endpoint, or if the userinfo endpoint causes
490+
// undesirable behavior.
491+
IgnoreUserInfo bool
486492
// GroupField selects the claim field to be used as the created user's
487493
// groups. If the group field is the empty string, then no group updates
488494
// will ever come from the OIDC provider.
@@ -551,46 +557,62 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
551557
return
552558
}
553559

560+
api.Logger.Debug(ctx, "got oidc claims",
561+
slog.F("source", "id_token"),
562+
slog.F("claim_fields", claimFields(claims)),
563+
slog.F("blank", blankFields(claims)),
564+
)
565+
554566
// Not all claims are necessarily embedded in the `id_token`.
555567
// In GitLab, the username is left empty and must be fetched in UserInfo.
556568
//
557569
// The OIDC specification says claims can be in either place, so we fetch
558-
// user info and merge the two claim sets to be sure we have all of
559-
// the correct data.
560-
userInfo, err := api.OIDCConfig.Provider.UserInfo(ctx, oauth2.StaticTokenSource(state.Token))
561-
if err == nil {
562-
userInfoClaims := map[string]interface{}{}
563-
err = userInfo.Claims(&userInfoClaims)
564-
if err != nil {
570+
// user info if required and merge the two claim sets to be sure we have
571+
// all of the correct data.
572+
//
573+
// Some providers (e.g. ADFS) do not support custom OIDC claims in the
574+
// UserInfo endpoint, so we allow users to disable it and only rely on the
575+
// ID token.
576+
if !api.OIDCConfig.IgnoreUserInfo {
577+
userInfo, err := api.OIDCConfig.Provider.UserInfo(ctx, oauth2.StaticTokenSource(state.Token))
578+
if err == nil {
579+
userInfoClaims := map[string]interface{}{}
580+
err = userInfo.Claims(&userInfoClaims)
581+
if err != nil {
582+
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
583+
Message: "Failed to unmarshal user info claims.",
584+
Detail: err.Error(),
585+
})
586+
return
587+
}
588+
api.Logger.Debug(ctx, "got oidc claims",
589+
slog.F("source", "userinfo"),
590+
slog.F("claim_fields", claimFields(userInfoClaims)),
591+
slog.F("blank", blankFields(userInfoClaims)),
592+
)
593+
594+
// Merge the claims from the ID token and the UserInfo endpoint.
595+
// Information from UserInfo takes precedence.
596+
claims = mergeClaims(claims, userInfoClaims)
597+
598+
// Log all of the field names after merging.
599+
api.Logger.Debug(ctx, "got oidc claims",
600+
slog.F("source", "merged"),
601+
slog.F("claim_fields", claimFields(claims)),
602+
slog.F("blank", blankFields(claims)),
603+
)
604+
} else if !strings.Contains(err.Error(), "user info endpoint is not supported by this provider") {
565605
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
566-
Message: "Failed to unmarshal user info claims.",
567-
Detail: err.Error(),
606+
Message: "Failed to obtain user information claims.",
607+
Detail: "The attempt to fetch claims via the UserInfo endpoint failed: " + err.Error(),
568608
})
569609
return
610+
} else {
611+
// The OIDC provider does not support the UserInfo endpoint.
612+
// This is not an error, but we should log it as it may mean
613+
// that some claims are missing.
614+
api.Logger.Warn(ctx, "OIDC provider does not support the user info endpoint, ensure that all required claims are present in the id_token")
570615
}
571-
for k, v := range userInfoClaims {
572-
claims[k] = v
573-
}
574-
} else if !strings.Contains(err.Error(), "user info endpoint is not supported by this provider") {
575-
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
576-
Message: "Failed to obtain user information claims.",
577-
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(),
578-
})
579-
return
580-
}
581-
582-
// Log all of the field names returned in the ID token claims, and the
583-
// userinfo returned from the provider.
584-
{
585-
fields := make([]string, 0, len(claims))
586-
for f := range claims {
587-
fields = append(fields, f)
588-
}
589-
590-
api.Logger.Debug(ctx, "got oidc claims",
591-
slog.F("user_info", userInfo),
592-
slog.F("claim_fields", fields),
593-
)
594616
}
595617

596618
usernameRaw, ok := claims[api.OIDCConfig.UsernameField]
@@ -657,7 +679,7 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
657679
group, ok := groupInterface.(string)
658680
if !ok {
659681
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
660-
Message: fmt.Sprintf("Invalid group type. Expected string, got: %t", emailRaw),
682+
Message: fmt.Sprintf("Invalid group type. Expected string, got: %T", groupInterface),
661683
})
662684
return
663685
}
@@ -761,6 +783,42 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
761783
http.Redirect(rw, r, redirect, http.StatusTemporaryRedirect)
762784
}
763785

786+
// claimFields returns the sorted list of fields in the claims map.
787+
func claimFields(claims map[string]interface{}) []string {
788+
fields := []string{}
789+
for field := range claims {
790+
fields = append(fields, field)
791+
}
792+
sort.Strings(fields)
793+
return fields
794+
}
795+
796+
// blankFields returns the list of fields in the claims map that are
797+
// an empty string.
798+
func blankFields(claims map[string]interface{}) []string {
799+
fields := make([]string, 0)
800+
for field, value := range claims {
801+
if valueStr, ok := value.(string); ok && valueStr == "" {
802+
fields = append(fields, field)
803+
}
804+
}
805+
sort.Strings(fields)
806+
return fields
807+
}
808+
809+
// mergeClaims merges the claims from a and b and returns the merged set.
810+
// claims from b take precedence over claims from a.
811+
func mergeClaims(a, b map[string]interface{}) map[string]interface{} {
812+
c := make(map[string]interface{})
813+
for k, v := range a {
814+
c[k] = v
815+
}
816+
for k, v := range b {
817+
c[k] = v
818+
}
819+
return c
820+
}
821+
764822
type oauthLoginParams struct {
765823
User database.User
766824
Link database.UserLink

‎coderd/userauth_test.go

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -501,6 +501,7 @@ func TestUserOIDC(t *testing.T) {
501501
AvatarURL string
502502
StatusCode int
503503
IgnoreEmailVerified bool
504+
IgnoreUserInfo bool
504505
}{{
505506
Name: "EmailOnly",
506507
IDTokenClaims: jwt.MapClaims{
@@ -643,6 +644,48 @@ func TestUserOIDC(t *testing.T) {
643644
},
644645
AllowSignups: true,
645646
StatusCode: http.StatusTemporaryRedirect,
647+
}, {
648+
Name: "UserInfoOverridesIDTokenClaims",
649+
IDTokenClaims: jwt.MapClaims{
650+
"email": "internaluser@internal.domain",
651+
"email_verified": false,
652+
},
653+
UserInfoClaims: jwt.MapClaims{
654+
"email": "externaluser@external.domain",
655+
"email_verified": true,
656+
"preferred_username": "user",
657+
},
658+
Username: "user",
659+
AllowSignups: true,
660+
IgnoreEmailVerified: false,
661+
StatusCode: http.StatusTemporaryRedirect,
662+
}, {
663+
Name: "InvalidUserInfo",
664+
IDTokenClaims: jwt.MapClaims{
665+
"email": "internaluser@internal.domain",
666+
"email_verified": false,
667+
},
668+
UserInfoClaims: jwt.MapClaims{
669+
"email": 1,
670+
},
671+
AllowSignups: true,
672+
IgnoreEmailVerified: false,
673+
StatusCode: http.StatusInternalServerError,
674+
}, {
675+
Name: "IgnoreUserInfo",
676+
IDTokenClaims: jwt.MapClaims{
677+
"email": "user@internal.domain",
678+
"email_verified": true,
679+
"preferred_username": "user",
680+
},
681+
UserInfoClaims: jwt.MapClaims{
682+
"email": "user.mcname@external.domain",
683+
"preferred_username": "Mr. User McName",
684+
},
685+
Username: "user",
686+
IgnoreUserInfo: true,
687+
AllowSignups: true,
688+
StatusCode: http.StatusTemporaryRedirect,
646689
}} {
647690
tc := tc
648691
t.Run(tc.Name, func(t *testing.T) {
@@ -654,6 +697,7 @@ func TestUserOIDC(t *testing.T) {
654697
config.AllowSignups = tc.AllowSignups
655698
config.EmailDomain = tc.EmailDomain
656699
config.IgnoreEmailVerified = tc.IgnoreEmailVerified
700+
config.IgnoreUserInfo = tc.IgnoreUserInfo
657701

658702
client := coderdtest.New(t, &coderdtest.Options{
659703
Auditor: auditor,

‎codersdk/deployment.go

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -258,6 +258,7 @@ type OIDCConfig struct {
258258
UsernameField clibase.String `json:"username_field" typescript:",notnull"`
259259
EmailField clibase.String `json:"email_field" typescript:",notnull"`
260260
AuthURLParams clibase.Struct[map[string]string] `json:"auth_url_params" typescript:",notnull"`
261+
IgnoreUserInfo clibase.Bool `json:"ignore_user_info" typescript:",notnull"`
261262
GroupField clibase.String `json:"groups_field" typescript:",notnull"`
262263
GroupMapping clibase.Struct[map[string]string] `json:"group_mapping" typescript:",notnull"`
263264
SignInText clibase.String `json:"sign_in_text" typescript:",notnull"`
@@ -882,6 +883,16 @@ when required by your organization's security policy.`,
882883
Group: &deploymentGroupOIDC,
883884
YAML: "authURLParams",
884885
},
886+
{
887+
Name: "OIDC Ignore UserInfo",
888+
Description: "Ignore the userinfo endpoint and only use the ID token for user information.",
889+
Flag: "oidc-ignore-userinfo",
890+
Env: "CODER_OIDC_IGNORE_USERINFO",
891+
Default: "false",
892+
Value: &c.OIDC.IgnoreUserInfo,
893+
Group: &deploymentGroupOIDC,
894+
YAML: "ignoreUserInfo",
895+
},
885896
{
886897
Name: "OIDC Group Field",
887898
Description: "Change the OIDC default 'groups' claim field. By default, will be 'groups' if present in the oidc scopes argument.",
@@ -901,7 +912,7 @@ when required by your organization's security policy.`,
901912
Name: "OIDC Group Mapping",
902913
Description: "A map of OIDC group IDs and the group in Coder it should map to. This is useful for when OIDC providers only return group IDs.",
903914
Flag: "oidc-group-mapping",
904-
Env: "OIDC_GROUP_MAPPING",
915+
Env: "CODER_OIDC_GROUP_MAPPING",
905916
Default: "{}",
906917
Value: &c.OIDC.GroupMapping,
907918
Group: &deploymentGroupOIDC,

‎docs/admin/auth.md

Lines changed: 30 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,10 @@ while signing in via OIDC as a new user. Coder will log the claim fields
144144
returned by the upstream identity provider in a message containing the
145145
string `got oidc claims`, as well as the user info returned.
146146

147+
> **Note:** If you need to ensure that Coder only uses information from
148+
> the ID token and does not hit the UserInfo endpoint, you can set the
149+
> configuration option `CODER_OIDC_IGNORE_USERINFO=true`.
150+
147151
### Email Addresses
148152

149153
By default, Coder will look for the OIDC claim named `email` and use that
@@ -262,17 +266,37 @@ Below are some details specific to individual OIDC providers.
262266
steps as described [here.](https://learn.microsoft.com/en-us/windows-server/identity/ad-fs/development/msal/adfs-msal-web-app-web-api#app-registration-in-ad-fs)
263267
- **Server Application**: Note the Client ID.
264268
- **Configure Application Credentials**: Note the Client Secret.
265-
- **Configure Web API**: Ensure the Client ID is set as the relying party identifier.
266-
- **Application Permissions**: Allow access to the claims `openid`, `email`, and `profile`.
269+
- **Configure Web API**: Set the Client ID as the relying party identifier.
270+
- **Application Permissions**: Allow access to the claims `openid`, `email`, `profile`, and `allatclaims`.
267271
1. Visit your ADFS server's `/.well-known/openid-configuration` URL and note
268272
the value for `issuer`.
269273
> **Note:** This is usually of the form `https://adfs.corp/adfs/.well-known/openid-configuration`
270274
1. In Coder's configuration file (or Helm values as appropriate), set the following
271275
environment variables or their corresponding CLI arguments:
276+
272277
- `CODER_OIDC_ISSUER_URL`: the `issuer` value from the previous step.
273278
- `CODER_OIDC_CLIENT_ID`: the Client ID from step 1.
274279
- `CODER_OIDC_CLIENT_SECRET`: the Client Secret from step 1.
275-
- `CODER_OIDC_AUTH_URL_PARAMS`: set to `{"resource":"urn:microsoft:userinfo"}` ([see here](https://learn.microsoft.com/en-us/windows-server/identity/ad-fs/overview/ad-fs-openid-connect-oauth-flows-scenarios#:~:text=scope%E2%80%AFopenid.-,resource,-optional)). OIDC logins will fail if this is not set.
276-
1. Ensure that Coder has the required OIDC claims by performing either of the below:
277-
- Configure your federation server to reuturn both the `email` and `preferred_username` fields by [creating a custom claim rule](https://learn.microsoft.com/en-us/windows-server/identity/ad-fs/operations/create-a-rule-to-send-ldap-attributes-as-claims), or
278-
- Set `CODER_OIDC_EMAIL_FIELD="upn"`. This will use the User Principal Name as the user email, which is [guaranteed to be unique in an Active Directory Forest](https://learn.microsoft.com/en-us/windows/win32/ad/naming-properties#upn-format).
280+
- `CODER_OIDC_AUTH_URL_PARAMS`: set to
281+
282+
```console
283+
{"resource":"$CLIENT_ID"}
284+
```
285+
286+
where `$CLIENT_ID` is the Client ID from step 1 ([see here](https://learn.microsoft.com/en-us/windows-server/identity/ad-fs/overview/ad-fs-openid-connect-oauth-flows-scenarios#:~:text=scope%E2%80%AFopenid.-,resource,-optional)).
287+
This is required for the upstream OIDC provider to return the requested claims.
288+
289+
- `CODER_OIDC_IGNORE_USERINFO`: Set to `true`.
290+
291+
1. Configure [Issuance Transform Rules](https://learn.microsoft.com/en-us/windows-server/identity/ad-fs/operations/create-a-rule-to-send-ldap-attributes-as-claims)
292+
on your federation server to send the following claims:
293+
294+
- `preferred_username`: You can use e.g. "Display Name" as required.
295+
- `email`: You can use e.g. the LDAP attribute "E-Mail-Addresses" as required.
296+
- `email_verified`: Create a custom claim rule:
297+
298+
```console
299+
=> issue(Type = "email_verified", Value = "true")
300+
```
301+
302+
- (Optional) If using Group Sync, send the required groups in the configured groups claim field. See [here](https://stackoverflow.com/a/55570286) for an example.

‎docs/api/general.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -251,6 +251,7 @@ curl -X GET http://coder-server:8080/api/v2/deployment/config \
251251
"user": {}
252252
},
253253
"ignore_email_verified": true,
254+
"ignore_user_info": true,
254255
"issuer_url": "string",
255256
"scopes": ["string"],
256257
"sign_in_text": "string",

0 commit comments

Comments
 (0)