Skip to content

Commit 69d8429

Browse files
EmyrkEmyrk
committed
feat(oidc): add sourcing secondary claims from access_token
Niche edge case use, assumes access_token is jwt
1 parent d0a534e commit 69d8429

File tree

5 files changed

+164
-76
lines changed

5 files changed

+164
-76
lines changed

‎cli/server.go

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,17 @@ func createOIDCConfig(ctx context.Context, logger slog.Logger, vals *codersdk.De
172172
groupAllowList[group] = true
173173
}
174174

175+
secondaryClaimsSrc := coderd.MergedClaimsSourceUserInfo
176+
if !vals.OIDC.IgnoreUserInfo && vals.OIDC.UserInfoFromAccessToken {
177+
return nil, xerrors.Errorf("cannot specify both 'oidc-access-token-claims' and 'oidc-ignore-userinfo'")
178+
}
179+
if vals.OIDC.IgnoreUserInfo {
180+
secondaryClaimsSrc = coderd.MergedClaimsSourceNone
181+
}
182+
if vals.OIDC.UserInfoFromAccessToken {
183+
secondaryClaimsSrc = coderd.MergedClaimsSourceAccessToken
184+
}
185+
175186
return &coderd.OIDCConfig{
176187
OAuth2Config: useCfg,
177188
Provider: oidcProvider,
@@ -187,7 +198,7 @@ func createOIDCConfig(ctx context.Context, logger slog.Logger, vals *codersdk.De
187198
NameField: vals.OIDC.NameField.String(),
188199
EmailField: vals.OIDC.EmailField.String(),
189200
AuthURLParams: vals.OIDC.AuthURLParams.Value,
190-
IgnoreUserInfo: vals.OIDC.IgnoreUserInfo.Value(),
201+
SecondaryClaims: secondaryClaimsSrc,
191202
SignInText: vals.OIDC.SignInText.String(),
192203
SignupsDisabledText: vals.OIDC.SignupsDisabledText.String(),
193204
IconURL: vals.OIDC.IconURL.String(),

‎coderd/coderdtest/oidctest/idp.go

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1465,9 +1465,10 @@ func (f *FakeIDP) internalOIDCConfig(ctx context.Context, t testing.TB, scopes [
14651465
Verifier: oidc.NewVerifier(f.provider.Issuer, &oidc.StaticKeySet{
14661466
PublicKeys: []crypto.PublicKey{f.key.Public()},
14671467
}, verifierConfig),
1468-
UsernameField: "preferred_username",
1469-
EmailField: "email",
1470-
AuthURLParams: map[string]string{"access_type": "offline"},
1468+
UsernameField: "preferred_username",
1469+
EmailField: "email",
1470+
AuthURLParams: map[string]string{"access_type": "offline"},
1471+
SecondaryClaims: coderd.MergedClaimsSourceUserInfo,
14711472
}
14721473

14731474
for _, opt := range opts {

‎coderd/userauth.go

Lines changed: 106 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,14 @@ import (
4646
"github.com/coder/coder/v2/cryptorand"
4747
)
4848

49+
type MergedClaimsSource string
50+
51+
var (
52+
MergedClaimsSourceNone MergedClaimsSource = "none"
53+
MergedClaimsSourceUserInfo MergedClaimsSource = "user_info"
54+
MergedClaimsSourceAccessToken MergedClaimsSource = "access_token"
55+
)
56+
4957
const (
5058
userAuthLoggerName = "userauth"
5159
OAuthConvertCookieValue = "coder_oauth_convert_jwt"
@@ -1042,11 +1050,13 @@ type OIDCConfig struct {
10421050
// AuthURLParams are additional parameters to be passed to the OIDC provider
10431051
// when requesting an access token.
10441052
AuthURLParams map[string]string
1045-
// IgnoreUserInfo causes Coder to only use claims from the ID token to
1046-
// process OIDC logins. This is useful if the OIDC provider does not
1047-
// support the userinfo endpoint, or if the userinfo endpoint causes
1048-
// undesirable behavior.
1049-
IgnoreUserInfo bool
1053+
// SecondaryClaims indicates where to source additional claim information from.
1054+
// The standard is either 'MergedClaimsSourceNone' or 'MergedClaimsSourceUserInfo'.
1055+
//
1056+
// The OIDC compliant way is to use the userinfo endpoint. This option
1057+
// is useful when the userinfo endpoint does not exist or causes undesirable
1058+
// behavior.
1059+
SecondaryClaims MergedClaimsSource
10501060
// SignInText is the text to display on the OIDC login button
10511061
SignInText string
10521062
// IconURL points to the URL of an icon to display on the OIDC login button
@@ -1112,20 +1122,6 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
11121122
return
11131123
}
11141124

1115-
if idToken.Subject == "" {
1116-
logger.Error(ctx, "oauth2: missing 'sub' claim field in OIDC token",
1117-
slog.F("source", "id_token"),
1118-
slog.F("claim_fields", claimFields(idtokenClaims)),
1119-
slog.F("blank", blankFields(idtokenClaims)),
1120-
)
1121-
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
1122-
Message: "OIDC token missing 'sub' claim field or 'sub' claim field is empty.",
1123-
Detail: "'sub' claim field is required to be unique for all users by a given issue, " +
1124-
"an empty field is invalid and this authentication attempt is rejected.",
1125-
})
1126-
return
1127-
}
1128-
11291125
logger.Debug(ctx, "got oidc claims",
11301126
slog.F("source", "id_token"),
11311127
slog.F("claim_fields", claimFields(idtokenClaims)),
@@ -1142,50 +1138,39 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
11421138
// Some providers (e.g. ADFS) do not support custom OIDC claims in the
11431139
// UserInfo endpoint, so we allow users to disable it and only rely on the
11441140
// ID token.
1145-
userInfoClaims := make(map[string]interface{})
1141+
//
11461142
// If user info is skipped, the idtokenClaims are the claims.
11471143
mergedClaims := idtokenClaims
1148-
if !api.OIDCConfig.IgnoreUserInfo {
1149-
userInfo, err := api.OIDCConfig.Provider.UserInfo(ctx, oauth2.StaticTokenSource(state.Token))
1150-
if err == nil {
1151-
err = userInfo.Claims(&userInfoClaims)
1152-
if err != nil {
1153-
logger.Error(ctx, "oauth2: unable to unmarshal user info claims", slog.Error(err))
1154-
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
1155-
Message: "Failed to unmarshal user info claims.",
1156-
Detail: err.Error(),
1157-
})
1158-
return
1159-
}
1160-
logger.Debug(ctx, "got oidc claims",
1161-
slog.F("source", "userinfo"),
1162-
slog.F("claim_fields", claimFields(userInfoClaims)),
1163-
slog.F("blank", blankFields(userInfoClaims)),
1164-
)
1165-
1166-
// Merge the claims from the ID token and the UserInfo endpoint.
1167-
// Information from UserInfo takes precedence.
1168-
mergedClaims = mergeClaims(idtokenClaims, userInfoClaims)
1144+
supplementaryClaims := make(map[string]interface{})
1145+
switch api.OIDCConfig.SecondaryClaims {
1146+
case MergedClaimsSourceUserInfo:
1147+
supplementaryClaims, ok = api.userInfoClaims(ctx, rw, state, logger)
1148+
if !ok {
1149+
return
1150+
}
11691151

1170-
// Log all of the field names after merging.
1171-
logger.Debug(ctx, "got oidc claims",
1172-
slog.F("source", "merged"),
1173-
slog.F("claim_fields", claimFields(mergedClaims)),
1174-
slog.F("blank", blankFields(mergedClaims)),
1175-
)
1176-
} else if !strings.Contains(err.Error(), "user info endpoint is not supported by this provider") {
1177-
logger.Error(ctx, "oauth2: unable to obtain user information claims", slog.Error(err))
1178-
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
1179-
Message: "Failed to obtain user information claims.",
1180-
Detail: "The attempt to fetch claims via the UserInfo endpoint failed: " + err.Error(),
1181-
})
1152+
// The precedence ordering is userInfoClaims > idTokenClaims.
1153+
// Note: Unsure why exactly this is the case. idTokenClaims feels more
1154+
// important?
1155+
mergedClaims = mergeClaims(idtokenClaims, supplementaryClaims)
1156+
case MergedClaimsSourceAccessToken:
1157+
supplementaryClaims, ok = api.accessTokenClaims(ctx, rw, state, logger)
1158+
if !ok {
11821159
return
1183-
} else {
1184-
// The OIDC provider does not support the UserInfo endpoint.
1185-
// This is not an error, but we should log it as it may mean
1186-
// that some claims are missing.
1187-
logger.Warn(ctx, "OIDC provider does not support the user info endpoint, ensure that all required claims are present in the id_token")
11881160
}
1161+
// idTokenClaims take priority over accessTokenClaims. The order should
1162+
// not matter. It is just safer to assume idTokenClaims is the truth,
1163+
// and accessTokenClaims are supplemental.
1164+
mergedClaims = mergeClaims(supplementaryClaims, idtokenClaims)
1165+
case MergedClaimsSourceNone:
1166+
// noop, keep the userInfoClaims empty
1167+
default:
1168+
// This should never happen and is a developer error
1169+
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
1170+
Message: "Invalid source for secondary user claims.",
1171+
Detail: fmt.Sprintf("invalid source: %q", api.OIDCConfig.SecondaryClaims),
1172+
})
1173+
return // Invalid MergedClaimsSource
11891174
}
11901175

11911176
usernameRaw, ok := mergedClaims[api.OIDCConfig.UsernameField]
@@ -1339,7 +1324,7 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
13391324
RoleSync: roleSync,
13401325
UserClaims: database.UserLinkClaims{
13411326
IDTokenClaims: idtokenClaims,
1342-
UserInfoClaims: userInfoClaims,
1327+
UserInfoClaims: supplementaryClaims,
13431328
MergedClaims: mergedClaims,
13441329
},
13451330
}).SetInitAuditRequest(func(params *audit.RequestParams) (*audit.Request[database.User], func()) {
@@ -1373,6 +1358,68 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
13731358
http.Redirect(rw, r, redirect, http.StatusTemporaryRedirect)
13741359
}
13751360

1361+
func (api *API) accessTokenClaims(ctx context.Context, rw http.ResponseWriter, state httpmw.OAuth2State, logger slog.Logger) (accessTokenClaims map[string]interface{}, ok bool) {
1362+
// Assume the access token is a jwt, and signed by the provider.
1363+
accessToken, err := api.OIDCConfig.Verifier.Verify(ctx, state.Token.AccessToken)
1364+
if err != nil {
1365+
logger.Error(ctx, "oauth2: unable to verify access token as secondary claims source", slog.Error(err))
1366+
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
1367+
Message: "Failed to verify access token.",
1368+
Detail: fmt.Sprintf("sourcing secondary claims from access token: %s", err.Error()),
1369+
})
1370+
return nil, false
1371+
}
1372+
1373+
rawClaims := make(map[string]any)
1374+
err = accessToken.Claims(&rawClaims)
1375+
if err != nil {
1376+
logger.Error(ctx, "oauth2: unable to unmarshal access token claims", slog.Error(err))
1377+
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
1378+
Message: "Failed to unmarshal access token claims.",
1379+
Detail: err.Error(),
1380+
})
1381+
return nil, false
1382+
}
1383+
1384+
return rawClaims, true
1385+
}
1386+
1387+
func (api *API) userInfoClaims(ctx context.Context, rw http.ResponseWriter, state httpmw.OAuth2State, logger slog.Logger) (userInfoClaims map[string]interface{}, ok bool) {
1388+
userInfoClaims = make(map[string]interface{})
1389+
userInfo, err := api.OIDCConfig.Provider.UserInfo(ctx, oauth2.StaticTokenSource(state.Token))
1390+
if err == nil {
1391+
err = userInfo.Claims(&userInfoClaims)
1392+
if err != nil {
1393+
logger.Error(ctx, "oauth2: unable to unmarshal user info claims", slog.Error(err))
1394+
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
1395+
Message: "Failed to unmarshal user info claims.",
1396+
Detail: err.Error(),
1397+
})
1398+
return nil, false
1399+
}
1400+
logger.Debug(ctx, "got oidc claims",
1401+
slog.F("source", "userinfo"),
1402+
slog.F("claim_fields", claimFields(userInfoClaims)),
1403+
slog.F("blank", blankFields(userInfoClaims)),
1404+
)
1405+
} else if !strings.Contains(err.Error(), "user info endpoint is not supported by this provider") {
1406+
logger.Error(ctx, "oauth2: unable to obtain user information claims", slog.Error(err))
1407+
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
1408+
Message: "Failed to obtain user information claims.",
1409+
Detail: "The attempt to fetch claims via the UserInfo endpoint failed: " + err.Error(),
1410+
})
1411+
return nil, false
1412+
} else {
1413+
// The OIDC provider does not support the UserInfo endpoint.
1414+
// This is not an error, but we should log it as it may mean
1415+
// that some claims are missing.
1416+
logger.Warn(ctx, "OIDC provider does not support the user info endpoint, ensure that all required claims are present in the id_token",
1417+
slog.Error(err),
1418+
)
1419+
}
1420+
return userInfoClaims, true
1421+
}
1422+
13761423
// claimFields returns the sorted list of fields in the claims map.
13771424
func claimFields(claims map[string]interface{}) []string {
13781425
fields := []string{}

‎coderd/userauth_test.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ func TestOIDCOauthLoginWithExisting(t *testing.T) {
6060

6161
cfg := fake.OIDCConfig(t, nil, func(cfg *coderd.OIDCConfig) {
6262
cfg.AllowSignups = true
63-
cfg.IgnoreUserInfo = true
63+
cfg.SecondaryClaims = coderd.MergedClaimsSourceNone
6464
})
6565

6666
client, _, api := coderdtest.NewWithAPI(t, &coderdtest.Options{
@@ -1301,7 +1301,9 @@ func TestUserOIDC(t *testing.T) {
13011301
cfg.AllowSignups = tc.AllowSignups
13021302
cfg.EmailDomain = tc.EmailDomain
13031303
cfg.IgnoreEmailVerified = tc.IgnoreEmailVerified
1304-
cfg.IgnoreUserInfo = tc.IgnoreUserInfo
1304+
if tc.IgnoreUserInfo {
1305+
cfg.SecondaryClaims = coderd.MergedClaimsSourceUserInfo
1306+
}
13051307
cfg.NameField = "name"
13061308
})
13071309

‎codersdk/deployment.go

Lines changed: 38 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -517,17 +517,27 @@ type OIDCConfig struct {
517517
ClientID serpent.String `json:"client_id" typescript:",notnull"`
518518
ClientSecret serpent.String `json:"client_secret" typescript:",notnull"`
519519
// ClientKeyFile & ClientCertFile are used in place of ClientSecret for PKI auth.
520-
ClientKeyFile serpent.String `json:"client_key_file" typescript:",notnull"`
521-
ClientCertFile serpent.String `json:"client_cert_file" typescript:",notnull"`
522-
EmailDomain serpent.StringArray `json:"email_domain" typescript:",notnull"`
523-
IssuerURL serpent.String `json:"issuer_url" typescript:",notnull"`
524-
Scopes serpent.StringArray `json:"scopes" typescript:",notnull"`
525-
IgnoreEmailVerified serpent.Bool `json:"ignore_email_verified" typescript:",notnull"`
526-
UsernameField serpent.String `json:"username_field" typescript:",notnull"`
527-
NameField serpent.String `json:"name_field" typescript:",notnull"`
528-
EmailField serpent.String `json:"email_field" typescript:",notnull"`
529-
AuthURLParams serpent.Struct[map[string]string] `json:"auth_url_params" typescript:",notnull"`
530-
IgnoreUserInfo serpent.Bool `json:"ignore_user_info" typescript:",notnull"`
520+
ClientKeyFile serpent.String `json:"client_key_file" typescript:",notnull"`
521+
ClientCertFile serpent.String `json:"client_cert_file" typescript:",notnull"`
522+
EmailDomain serpent.StringArray `json:"email_domain" typescript:",notnull"`
523+
IssuerURL serpent.String `json:"issuer_url" typescript:",notnull"`
524+
Scopes serpent.StringArray `json:"scopes" typescript:",notnull"`
525+
IgnoreEmailVerified serpent.Bool `json:"ignore_email_verified" typescript:",notnull"`
526+
UsernameField serpent.String `json:"username_field" typescript:",notnull"`
527+
NameField serpent.String `json:"name_field" typescript:",notnull"`
528+
EmailField serpent.String `json:"email_field" typescript:",notnull"`
529+
AuthURLParams serpent.Struct[map[string]string] `json:"auth_url_params" typescript:",notnull"`
530+
// IgnoreUserInfo & UserInfoFromAccessToken are mutually exclusive. Only 1
531+
// can be set to true. Ideally this would be an enum with 3 states, ['none',
532+
// 'userinfo', 'access_token']. However, for backward compatibility,
533+
// `ignore_user_info` must remain. And `access_token` is a niche, non-spec
534+
// compliant edge case. So it's use is rare, and should not be advised.
535+
IgnoreUserInfo serpent.Bool `json:"ignore_user_info" typescript:",notnull"`
536+
// UserInfoFromAccessToken as mentioned above is an edge case. This allows
537+
// sourcing the user_info from the access token itself instead of a user_info
538+
// endpoint. This assumes the access token is a valid JWT with a set of claims to
539+
// be merged with the id_token.
540+
UserInfoFromAccessToken serpent.Bool `json:"source_user_info_from_access_token" typescript:",notnull"`
531541
OrganizationField serpent.String `json:"organization_field" typescript:",notnull"`
532542
OrganizationMapping serpent.Struct[map[string][]uuid.UUID] `json:"organization_mapping" typescript:",notnull"`
533543
OrganizationAssignDefault serpent.Bool `json:"organization_assign_default" typescript:",notnull"`
@@ -1753,6 +1763,23 @@ func (c *DeploymentValues) Options() serpent.OptionSet {
17531763
Group: &deploymentGroupOIDC,
17541764
YAML: "ignoreUserInfo",
17551765
},
1766+
{
1767+
Name: "OIDC Access Token Claims",
1768+
// This is a niche edge case that should not be advertised. Alternatives should
1769+
// be investigated before turning this on. A properly configured IdP should
1770+
// always have a userinfo endpoint which is preferred.
1771+
Hidden: true,
1772+
Description: "Source supplemental user claims from the 'access_token'. This assumes the " +
1773+
"token is a jwt signed by the same issuer as the id_token. Using this requires setting " +
1774+
"'oidc-ignore-userinfo' to true. This setting is not compliant with the OIDC specification " +
1775+
"and is not recommended. Use at your own risk.",
1776+
Flag: "oidc-access-token-claims",
1777+
Env: "CODER_OIDC_ACCESS_TOKEN_CLAIMS",
1778+
Default: "false",
1779+
Value: &c.OIDC.UserInfoFromAccessToken,
1780+
Group: &deploymentGroupOIDC,
1781+
YAML: "accessTokenClaims",
1782+
},
17561783
{
17571784
Name: "OIDC Organization Field",
17581785
Description: "This field must be set if using the organization sync feature." +

0 commit comments

Comments
 (0)