8000 fix(coderd)!: add CODER_OIDC_IGNORE_USERINFO configuration option (#6… · coder/coder@9c4ccd7 · GitHub
[go: up one dir, main page]
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 EED3 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)
    0