feat(oidc): add sourcing secondary claims from access_token

Niche edge case use, assumes access_token is jwt
coder · Emyrk · Feb 24, 2025 · Feb 10, 2025 · Feb 10, 2025 · Feb 10, 2025
commit b4275b3ad8bfba6d8667f5ae5b1ebb57b33d6365
diff --git a/cli/server.go b/cli/server.go
@@ -172,6 +172,17 @@ func createOIDCConfig(ctx context.Context, logger slog.Logger, vals *codersdk.De
 		groupAllowList[group] = true
 	}
 
+	secondaryClaimsSrc := coderd.MergedClaimsSourceUserInfo
+	if !vals.OIDC.IgnoreUserInfo && vals.OIDC.UserInfoFromAccessToken {
+		return nil, xerrors.Errorf("cannot specify both 'oidc-access-token-claims' and 'oidc-ignore-userinfo'")
+	}
+	if vals.OIDC.IgnoreUserInfo {
+		secondaryClaimsSrc = coderd.MergedClaimsSourceNone
+	}
+	if vals.OIDC.UserInfoFromAccessToken {
+		secondaryClaimsSrc = coderd.MergedClaimsSourceAccessToken
+	}
+
 	return &coderd.OIDCConfig{
 		OAuth2Config: useCfg,
 		Provider:     oidcProvider,
@@ -187,7 +198,7 @@ func createOIDCConfig(ctx context.Context, logger slog.Logger, vals *codersdk.De
 		NameField:           vals.OIDC.NameField.String(),
 		EmailField:          vals.OIDC.EmailField.String(),
 		AuthURLParams:       vals.OIDC.AuthURLParams.Value,
-		IgnoreUserInfo:      vals.OIDC.IgnoreUserInfo.Value(),
+		SecondaryClaims:     secondaryClaimsSrc,
 		SignInText:          vals.OIDC.SignInText.String(),
 		SignupsDisabledText: vals.OIDC.SignupsDisabledText.String(),
 		IconURL:             vals.OIDC.IconURL.String(),

diff --git a/coderd/coderdtest/oidctest/idp.go b/coderd/coderdtest/oidctest/idp.go
@@ -1465,9 +1465,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 {

diff --git a/coderd/userauth.go b/coderd/userauth.go
@@ -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"
@@ -1042,11 +1050,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
@@ -1112,20 +1122,6 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	if idToken.Subject == "" {
-		logger.Error(ctx, "oauth2: missing 'sub' claim field in OIDC token",
-			slog.F("source", "id_token"),
-			slog.F("claim_fields", claimFields(idtokenClaims)),
-			slog.F("blank", blankFields(idtokenClaims)),
-		)
-		httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
-			Message: "OIDC token missing 'sub' claim field or 'sub' claim field is empty.",
-			Detail: "'sub' claim field is required to be unique for all users by a given issue, " +
-				"an empty field is invalid and this authentication attempt is rejected.",
-		})
-		return
-	}
-
 	logger.Debug(ctx, "got oidc claims",
 		slog.F("source", "id_token"),
 		slog.F("claim_fields", claimFields(idtokenClaims)),
@@ -1142,50 +1138,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)
+	supplementaryClaims := make(map[string]interface{})
+	switch api.OIDCConfig.SecondaryClaims {
+	case MergedClaimsSourceUserInfo:
+		supplementaryClaims, ok = api.userInfoClaims(ctx, rw, state, logger)
+		if !ok {
+			return
+		}
 
-			// 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(),
-			})
+		// 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
-		} 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")
 		}
+		// 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]
@@ -1339,7 +1324,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()) {
@@ -1373,6 +1358,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{}

diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go
@@ -60,7 +60,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{
@@ -1301,7 +1301,9 @@ func TestUserOIDC(t *testing.T) {
 				cfg.AllowSignups = tc.AllowSignups
 				cfg.EmailDomain = tc.EmailDomain
 				cfg.IgnoreEmailVerified = tc.IgnoreEmailVerified
-				cfg.IgnoreUserInfo = tc.IgnoreUserInfo
+				if tc.IgnoreUserInfo {
+					cfg.SecondaryClaims = coderd.MergedClaimsSourceUserInfo
+				}
 				cfg.NameField = "name"
 			})
 

diff --git a/codersdk/deployment.go b/codersdk/deployment.go
@@ -517,17 +517,27 @@ type OIDCConfig struct {
 	ClientID     serpent.String `json:"client_id" typescript:",notnull"`
 	ClientSecret serpent.String `json:"client_secret" typescript:",notnull"`
 	// ClientKeyFile & ClientCertFile are used in place of ClientSecret for PKI auth.
-	ClientKeyFile             serpent.String                         `json:"client_key_file" typescript:",notnull"`
-	ClientCertFile            serpent.String                         `json:"client_cert_file" typescript:",notnull"`
-	EmailDomain               serpent.StringArray                    `json:"email_domain" typescript:",notnull"`
-	IssuerURL                 serpent.String                         `json:"issuer_url" typescript:",notnull"`
-	Scopes                    serpent.StringArray                    `json:"scopes" typescript:",notnull"`
-	IgnoreEmailVerified       serpent.Bool                           `json:"ignore_email_verified" typescript:",notnull"`
-	UsernameField             serpent.String                         `json:"username_field" typescript:",notnull"`
-	NameField                 serpent.String                         `json:"name_field" typescript:",notnull"`
-	EmailField                serpent.String                         `json:"email_field" typescript:",notnull"`
-	AuthURLParams             serpent.Struct[map[string]string]      `json:"auth_url_params" typescript:",notnull"`
-	IgnoreUserInfo            serpent.Bool                           `json:"ignore_user_info" typescript:",notnull"`
+	ClientKeyFile       serpent.String                    `json:"client_key_file" typescript:",notnull"`
+	ClientCertFile      serpent.String                    `json:"client_cert_file" typescript:",notnull"`
+	EmailDomain         serpent.StringArray               `json:"email_domain" typescript:",notnull"`
+	IssuerURL           serpent.String                    `json:"issuer_url" typescript:",notnull"`
+	Scopes              serpent.StringArray               `json:"scopes" typescript:",notnull"`
+	IgnoreEmailVerified serpent.Bool                      `json:"ignore_email_verified" typescript:",notnull"`
+	UsernameField       serpent.String                    `json:"username_field" typescript:",notnull"`
+	NameField           serpent.String                    `json:"name_field" typescript:",notnull"`
+	EmailField          serpent.String                    `json:"email_field" typescript:",notnull"`
+	AuthURLParams       serpent.Struct[map[string]string] `json:"auth_url_params" typescript:",notnull"`
+	// IgnoreUserInfo & UserInfoFromAccessToken are mutually exclusive. Only 1
+	// can be set to true. Ideally this would be an enum with 3 states, ['none',
+	// 'userinfo', 'access_token']. However, for backward compatibility,
+	// `ignore_user_info` must remain. And `access_token` is a niche, non-spec
+	// compliant edge case. So it's use is rare, and should not be advised.
+	IgnoreUserInfo serpent.Bool `json:"ignore_user_info" typescript:",notnull"`
+	// UserInfoFromAccessToken as mentioned above is an edge case. This allows
+	// sourcing the user_info from the access token itself instead of a user_info
+	// endpoint. This assumes the access token is a valid JWT with a set of claims to
+	// be merged with the id_token.
+	UserInfoFromAccessToken   serpent.Bool                           `json:"source_user_info_from_access_token" typescript:",notnull"`
 	OrganizationField         serpent.String                         `json:"organization_field" typescript:",notnull"`
 	OrganizationMapping       serpent.Struct[map[string][]uuid.UUID] `json:"organization_mapping" typescript:",notnull"`
 	OrganizationAssignDefault serpent.Bool                           `json:"organization_assign_default" typescript:",notnull"`
@@ -1753,6 +1763,23 @@ func (c *DeploymentValues) Options() serpent.OptionSet {
 			Group:       &deploymentGroupOIDC,
 			YAML:        "ignoreUserInfo",
 		},
+		{
+			Name: "OIDC Access Token Claims",
+			// This is a niche edge case that should not be advertised. Alternatives should
+			// be investigated before turning this on. A properly configured IdP should
+			// always have a userinfo endpoint which is preferred.
+			Hidden: true,
+			Description: "Source supplemental user claims from the 'access_token'. This assumes the " +
+				"token is a jwt signed by the same issuer as the id_token. Using this requires setting " +
+				"'oidc-ignore-userinfo' to true. This setting is not compliant with the OIDC specification " +
+				"and is not recommended. Use at your own risk.",
+			Flag:    "oidc-access-token-claims",
+			Env:     "CODER_OIDC_ACCESS_TOKEN_CLAIMS",
+			Default: "false",
+			Value:   &c.OIDC.UserInfoFromAccessToken,
+			Group:   &deploymentGroupOIDC,
+			YAML:    "accessTokenClaims",
+		},
 		{
 			Name: "OIDC Organization Field",
 			Description: "This field must be set if using the organization sync feature." +