feat: add sourcing secondary claims from access_token (#16517)

Niche edge case, assumes access_token is jwt. 

Some `access_token`s are JWT's with potential useful claims.
These claims would be nearly equivalent to `user_info` claims.
This is not apart of the oauth spec, so this feature should not be
loudly advertised. If using this feature, alternate solutions are preferred.
This commit is contained in:
Steven Masley
2025-02-24 13:38:20 -06:00
committed by GitHub
parent e005e4e51d
commit 658825cad2
12 changed files with 282 additions and 100 deletions
+5
View File
@@ -12669,6 +12669,7 @@ const docTemplate = `{
"type": "boolean"
},
"ignore_user_info": {
"description": "IgnoreUserInfo \u0026 UserInfoFromAccessToken are mutually exclusive. Only 1\ncan be set to true. Ideally this would be an enum with 3 states, ['none',\n'userinfo', 'access_token']. However, for backward compatibility,\n` + "`" + `ignore_user_info` + "`" + ` must remain. And ` + "`" + `access_token` + "`" + ` is a niche, non-spec\ncompliant edge case. So it's use is rare, and should not be advised.",
"type": "boolean"
},
"issuer_url": {
@@ -12701,6 +12702,10 @@ const docTemplate = `{
"skip_issuer_checks": {
"type": "boolean"
},
"source_user_info_from_access_token": {
"description": "UserInfoFromAccessToken as mentioned above is an edge case. This allows\nsourcing the user_info from the access token itself instead of a user_info\nendpoint. This assumes the access token is a valid JWT with a set of claims to\nbe merged with the id_token.",
"type": "boolean"
},
"user_role_field": {
"type": "string"
},
+5
View File
@@ -11405,6 +11405,7 @@
"type": "boolean"
},
"ignore_user_info": {
"description": "IgnoreUserInfo \u0026 UserInfoFromAccessToken are mutually exclusive. Only 1\ncan be set to true. Ideally this would be an enum with 3 states, ['none',\n'userinfo', 'access_token']. However, for backward compatibility,\n`ignore_user_info` must remain. And `access_token` is a niche, non-spec\ncompliant edge case. So it's use is rare, and should not be advised.",
"type": "boolean"
},
"issuer_url": {
@@ -11437,6 +11438,10 @@
"skip_issuer_checks": {
"type": "boolean"
},
"source_user_info_from_access_token": {
"description": "UserInfoFromAccessToken as mentioned above is an edge case. This allows\nsourcing the user_info from the access token itself instead of a user_info\nendpoint. This assumes the access token is a valid JWT with a set of claims to\nbe merged with the id_token.",
"type": "boolean"
},
"user_role_field": {
"type": "string"
},
+24 -7
View File
@@ -105,6 +105,7 @@ type FakeIDP struct {
// "Authorized Redirect URLs". This can be used to emulate that.
hookValidRedirectURL func(redirectURL string) error
hookUserInfo func(email string) (jwt.MapClaims, error)
hookAccessTokenJWT func(email string, exp time.Time) jwt.MapClaims
// defaultIDClaims is if a new client connects and we didn't preset
// some claims.
defaultIDClaims jwt.MapClaims
@@ -154,6 +155,12 @@ func WithMiddlewares(mws ...func(http.Handler) http.Handler) func(*FakeIDP) {
}
}
func WithAccessTokenJWTHook(hook func(email string, exp time.Time) jwt.MapClaims) func(*FakeIDP) {
return func(f *FakeIDP) {
f.hookAccessTokenJWT = hook
}
}
func WithHookWellKnown(hook func(r *http.Request, j *ProviderJSON) error) func(*FakeIDP) {
return func(f *FakeIDP) {
f.hookWellKnown = hook
@@ -316,8 +323,7 @@ const (
func NewFakeIDP(t testing.TB, opts ...FakeIDPOpt) *FakeIDP {
t.Helper()
block, _ := pem.Decode([]byte(testRSAPrivateKey))
pkey, err := x509.ParsePKCS1PrivateKey(block.Bytes)
pkey, err := FakeIDPKey()
require.NoError(t, err)
idp := &FakeIDP{
@@ -676,8 +682,13 @@ func (f *FakeIDP) newCode(state string) string {
// newToken enforces the access token exchanged is actually a valid access token
// created by the IDP.
func (f *FakeIDP) newToken(email string, expires time.Time) string {
func (f *FakeIDP) newToken(t testing.TB, email string, expires time.Time) string {
accessToken := uuid.NewString()
if f.hookAccessTokenJWT != nil {
claims := f.hookAccessTokenJWT(email, expires)
accessToken = f.encodeClaims(t, claims)
}
f.accessTokens.Store(accessToken, token{
issued: time.Now(),
email: email,
@@ -963,7 +974,7 @@ func (f *FakeIDP) httpHandler(t testing.TB) http.Handler {
email := getEmail(claims)
refreshToken := f.newRefreshTokens(email)
token := map[string]interface{}{
"access_token": f.newToken(email, exp),
"access_token": f.newToken(t, email, exp),
"refresh_token": refreshToken,
"token_type": "Bearer",
"expires_in": int64((f.defaultExpire).Seconds()),
@@ -1465,9 +1476,10 @@ func (f *FakeIDP) internalOIDCConfig(ctx context.Context, t testing.TB, scopes [
Verifier: oidc.NewVerifier(f.provider.Issuer, &oidc.StaticKeySet{
PublicKeys: []crypto.PublicKey{f.key.Public()},
}, verifierConfig),
UsernameField: "preferred_username",
EmailField: "email",
AuthURLParams: map[string]string{"access_type": "offline"},
UsernameField: "preferred_username",
EmailField: "email",
AuthURLParams: map[string]string{"access_type": "offline"},
SecondaryClaims: coderd.MergedClaimsSourceUserInfo,
}
for _, opt := range opts {
@@ -1552,3 +1564,8 @@ d8h4Ht09E+f3nhTEc87mODkl7WJZpHL6V2sORfeq/eIkds+H6CJ4hy5w/bSw8tjf
sz9Di8sGIaUbLZI2rd0CQQCzlVwEtRtoNCyMJTTrkgUuNufLP19RZ5FpyXxBO5/u
QastnN77KfUwdj3SJt44U/uh1jAIv4oSLBr8HYUkbnI8
-----END RSA PRIVATE KEY-----`
func FakeIDPKey() (*rsa.PrivateKey, error) {
block, _ := pem.Decode([]byte(testRSAPrivateKey))
return x509.ParsePKCS1PrivateKey(block.Bytes)
}
+107 -46
View File
@@ -46,6 +46,14 @@ import (
"github.com/coder/coder/v2/cryptorand"
)
type MergedClaimsSource string
var (
MergedClaimsSourceNone MergedClaimsSource = "none"
MergedClaimsSourceUserInfo MergedClaimsSource = "user_info"
MergedClaimsSourceAccessToken MergedClaimsSource = "access_token"
)
const (
userAuthLoggerName = "userauth"
OAuthConvertCookieValue = "coder_oauth_convert_jwt"
@@ -1116,11 +1124,13 @@ type OIDCConfig struct {
// AuthURLParams are additional parameters to be passed to the OIDC provider
// when requesting an access token.
AuthURLParams map[string]string
// IgnoreUserInfo causes Coder to only use claims from the ID token to
// process OIDC logins. This is useful if the OIDC provider does not
// support the userinfo endpoint, or if the userinfo endpoint causes
// undesirable behavior.
IgnoreUserInfo bool
// SecondaryClaims indicates where to source additional claim information from.
// The standard is either 'MergedClaimsSourceNone' or 'MergedClaimsSourceUserInfo'.
//
// The OIDC compliant way is to use the userinfo endpoint. This option
// is useful when the userinfo endpoint does not exist or causes undesirable
// behavior.
SecondaryClaims MergedClaimsSource
// SignInText is the text to display on the OIDC login button
SignInText string
// IconURL points to the URL of an icon to display on the OIDC login button
@@ -1216,50 +1226,39 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
// Some providers (e.g. ADFS) do not support custom OIDC claims in the
// UserInfo endpoint, so we allow users to disable it and only rely on the
// ID token.
userInfoClaims := make(map[string]interface{})
//
// If user info is skipped, the idtokenClaims are the claims.
mergedClaims := idtokenClaims
if !api.OIDCConfig.IgnoreUserInfo {
userInfo, err := api.OIDCConfig.Provider.UserInfo(ctx, oauth2.StaticTokenSource(state.Token))
if err == nil {
err = userInfo.Claims(&userInfoClaims)
if err != nil {
logger.Error(ctx, "oauth2: unable to unmarshal user info claims", slog.Error(err))
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
Message: "Failed to unmarshal user info claims.",
Detail: err.Error(),
})
return
}
logger.Debug(ctx, "got oidc claims",
slog.F("source", "userinfo"),
slog.F("claim_fields", claimFields(userInfoClaims)),
slog.F("blank", blankFields(userInfoClaims)),
)
// Merge the claims from the ID token and the UserInfo endpoint.
// Information from UserInfo takes precedence.
mergedClaims = mergeClaims(idtokenClaims, userInfoClaims)
// Log all of the field names after merging.
logger.Debug(ctx, "got oidc claims",
slog.F("source", "merged"),
slog.F("claim_fields", claimFields(mergedClaims)),
slog.F("blank", blankFields(mergedClaims)),
)
} else if !strings.Contains(err.Error(), "user info endpoint is not supported by this provider") {
logger.Error(ctx, "oauth2: unable to obtain user information claims", slog.Error(err))
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
Message: "Failed to obtain user information claims.",
Detail: "The attempt to fetch claims via the UserInfo endpoint failed: " + err.Error(),
})
supplementaryClaims := make(map[string]interface{})
switch api.OIDCConfig.SecondaryClaims {
case MergedClaimsSourceUserInfo:
supplementaryClaims, ok = api.userInfoClaims(ctx, rw, state, logger)
if !ok {
return
} else {
// The OIDC provider does not support the UserInfo endpoint.
// This is not an error, but we should log it as it may mean
// that some claims are missing.
logger.Warn(ctx, "OIDC provider does not support the user info endpoint, ensure that all required claims are present in the id_token")
}
// The precedence ordering is userInfoClaims > idTokenClaims.
// Note: Unsure why exactly this is the case. idTokenClaims feels more
// important?
mergedClaims = mergeClaims(idtokenClaims, supplementaryClaims)
case MergedClaimsSourceAccessToken:
supplementaryClaims, ok = api.accessTokenClaims(ctx, rw, state, logger)
if !ok {
return
}
// idTokenClaims take priority over accessTokenClaims. The order should
// not matter. It is just safer to assume idTokenClaims is the truth,
// and accessTokenClaims are supplemental.
mergedClaims = mergeClaims(supplementaryClaims, idtokenClaims)
case MergedClaimsSourceNone:
// noop, keep the userInfoClaims empty
default:
// This should never happen and is a developer error
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
Message: "Invalid source for secondary user claims.",
Detail: fmt.Sprintf("invalid source: %q", api.OIDCConfig.SecondaryClaims),
})
return // Invalid MergedClaimsSource
}
usernameRaw, ok := mergedClaims[api.OIDCConfig.UsernameField]
@@ -1413,7 +1412,7 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
RoleSync: roleSync,
UserClaims: database.UserLinkClaims{
IDTokenClaims: idtokenClaims,
UserInfoClaims: userInfoClaims,
UserInfoClaims: supplementaryClaims,
MergedClaims: mergedClaims,
},
}).SetInitAuditRequest(func(params *audit.RequestParams) (*audit.Request[database.User], func()) {
@@ -1447,6 +1446,68 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
http.Redirect(rw, r, redirect, http.StatusTemporaryRedirect)
}
func (api *API) accessTokenClaims(ctx context.Context, rw http.ResponseWriter, state httpmw.OAuth2State, logger slog.Logger) (accessTokenClaims map[string]interface{}, ok bool) {
// Assume the access token is a jwt, and signed by the provider.
accessToken, err := api.OIDCConfig.Verifier.Verify(ctx, state.Token.AccessToken)
if err != nil {
logger.Error(ctx, "oauth2: unable to verify access token as secondary claims source", slog.Error(err))
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
Message: "Failed to verify access token.",
Detail: fmt.Sprintf("sourcing secondary claims from access token: %s", err.Error()),
})
return nil, false
}
rawClaims := make(map[string]any)
err = accessToken.Claims(&rawClaims)
if err != nil {
logger.Error(ctx, "oauth2: unable to unmarshal access token claims", slog.Error(err))
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
Message: "Failed to unmarshal access token claims.",
Detail: err.Error(),
})
return nil, false
}
return rawClaims, true
}
func (api *API) userInfoClaims(ctx context.Context, rw http.ResponseWriter, state httpmw.OAuth2State, logger slog.Logger) (userInfoClaims map[string]interface{}, ok bool) {
userInfoClaims = make(map[string]interface{})
userInfo, err := api.OIDCConfig.Provider.UserInfo(ctx, oauth2.StaticTokenSource(state.Token))
if err == nil {
err = userInfo.Claims(&userInfoClaims)
if err != nil {
logger.Error(ctx, "oauth2: unable to unmarshal user info claims", slog.Error(err))
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
Message: "Failed to unmarshal user info claims.",
Detail: err.Error(),
})
return nil, false
}
logger.Debug(ctx, "got oidc claims",
slog.F("source", "userinfo"),
slog.F("claim_fields", claimFields(userInfoClaims)),
slog.F("blank", blankFields(userInfoClaims)),
)
} else if !strings.Contains(err.Error(), "user info endpoint is not supported by this provider") {
logger.Error(ctx, "oauth2: unable to obtain user information claims", slog.Error(err))
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
Message: "Failed to obtain user information claims.",
Detail: "The attempt to fetch claims via the UserInfo endpoint failed: " + err.Error(),
})
return nil, false
} else {
// The OIDC provider does not support the UserInfo endpoint.
// This is not an error, but we should log it as it may mean
// that some claims are missing.
logger.Warn(ctx, "OIDC provider does not support the user info endpoint, ensure that all required claims are present in the id_token",
slog.Error(err),
)
}
return userInfoClaims, true
}
// claimFields returns the sorted list of fields in the claims map.
func claimFields(claims map[string]interface{}) []string {
fields := []string{}
+46 -4
View File
@@ -61,7 +61,7 @@ func TestOIDCOauthLoginWithExisting(t *testing.T) {
cfg := fake.OIDCConfig(t, nil, func(cfg *coderd.OIDCConfig) {
cfg.AllowSignups = true
cfg.IgnoreUserInfo = true
cfg.SecondaryClaims = coderd.MergedClaimsSourceNone
})
client, _, api := coderdtest.NewWithAPI(t, &coderdtest.Options{
@@ -979,6 +979,7 @@ func TestUserOIDC(t *testing.T) {
Name string
IDTokenClaims jwt.MapClaims
UserInfoClaims jwt.MapClaims
AccessTokenClaims jwt.MapClaims
AllowSignups bool
EmailDomain []string
AssertUser func(t testing.TB, u codersdk.User)
@@ -986,6 +987,7 @@ func TestUserOIDC(t *testing.T) {
AssertResponse func(t testing.TB, resp *http.Response)
IgnoreEmailVerified bool
IgnoreUserInfo bool
UseAccessToken bool
}{
{
Name: "NoSub",
@@ -995,6 +997,32 @@ func TestUserOIDC(t *testing.T) {
AllowSignups: true,
StatusCode: http.StatusBadRequest,
},
{
Name: "AccessTokenMerge",
IDTokenClaims: jwt.MapClaims{
"sub": uuid.NewString(),
},
AccessTokenClaims: jwt.MapClaims{
"email": "kyle@kwc.io",
},
IgnoreUserInfo: true,
AllowSignups: true,
UseAccessToken: true,
StatusCode: http.StatusOK,
AssertUser: func(t testing.TB, u codersdk.User) {
assert.Equal(t, "kyle@kwc.io", u.Email)
},
},
{
Name: "AccessTokenMergeNotJWT",
IDTokenClaims: jwt.MapClaims{
"sub": uuid.NewString(),
},
IgnoreUserInfo: true,
AllowSignups: true,
UseAccessToken: true,
StatusCode: http.StatusBadRequest,
},
{
Name: "EmailOnly",
IDTokenClaims: jwt.MapClaims{
@@ -1377,18 +1405,32 @@ func TestUserOIDC(t *testing.T) {
tc := tc
t.Run(tc.Name, func(t *testing.T) {
t.Parallel()
fake := oidctest.NewFakeIDP(t,
opts := []oidctest.FakeIDPOpt{
oidctest.WithRefresh(func(_ string) error {
return xerrors.New("refreshing token should never occur")
}),
oidctest.WithServing(),
oidctest.WithStaticUserInfo(tc.UserInfoClaims),
)
}
if tc.AccessTokenClaims != nil && len(tc.AccessTokenClaims) > 0 {
opts = append(opts, oidctest.WithAccessTokenJWTHook(func(email string, exp time.Time) jwt.MapClaims {
return tc.AccessTokenClaims
}))
}
fake := oidctest.NewFakeIDP(t, opts...)
cfg := fake.OIDCConfig(t, nil, func(cfg *coderd.OIDCConfig) {
cfg.AllowSignups = tc.AllowSignups
cfg.EmailDomain = tc.EmailDomain
cfg.IgnoreEmailVerified = tc.IgnoreEmailVerified
cfg.IgnoreUserInfo = tc.IgnoreUserInfo
cfg.SecondaryClaims = coderd.MergedClaimsSourceUserInfo
if tc.IgnoreUserInfo {
cfg.SecondaryClaims = coderd.MergedClaimsSourceNone
}
if tc.UseAccessToken {
cfg.SecondaryClaims = coderd.MergedClaimsSourceAccessToken
}
cfg.NameField = "name"
})