mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
feat: add INSECURE oidc email fallback flag for IdP brokers (#26751)
<!-- Authored by Coder Agents on behalf of @Emyrk. --> Adds an opt-in `CODER_DANGEROUS_OIDC_EMAIL_FALLBACK` flag (alias `--dangerous-oidc-email-fallback`) for IdP brokers that do not issue a stable `sub` for the same user across connections.
This commit is contained in:
Generated
+4
@@ -21285,6 +21285,10 @@ const docTemplate = `{
|
||||
"type": "string"
|
||||
}
|
||||
},
|
||||
"email_fallback": {
|
||||
"description": "EmailFallback allows OIDC logins to fall back to email-based matching\nwhen the ` + "`" + `linked_id` + "`" + ` (issuer+subject) does not match an existing user\nlink. INSECURE: weakens the linked_id check. It exists for IdP\nbrokers that do not issue a stable ` + "`" + `sub` + "`" + ` for the same user across\nconnections.",
|
||||
"type": "boolean"
|
||||
},
|
||||
"email_field": {
|
||||
"type": "string"
|
||||
},
|
||||
|
||||
Generated
+4
@@ -19415,6 +19415,10 @@
|
||||
"type": "string"
|
||||
}
|
||||
},
|
||||
"email_fallback": {
|
||||
"description": "EmailFallback allows OIDC logins to fall back to email-based matching\nwhen the `linked_id` (issuer+subject) does not match an existing user\nlink. INSECURE: weakens the linked_id check. It exists for IdP\nbrokers that do not issue a stable `sub` for the same user across\nconnections.",
|
||||
"type": "boolean"
|
||||
},
|
||||
"email_field": {
|
||||
"type": "string"
|
||||
},
|
||||
|
||||
+34
-6
@@ -1037,7 +1037,7 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) {
|
||||
})
|
||||
return
|
||||
}
|
||||
user, link, err := findLinkedUser(ctx, api.Database, githubLinkedID(ghUser), database.LoginTypeGithub, verifiedEmail.GetEmail())
|
||||
user, link, err := findLinkedUser(ctx, api.Database, githubLinkedID(ghUser), database.LoginTypeGithub, false, verifiedEmail.GetEmail())
|
||||
if errors.Is(err, errLinkedIDAlreadyBound) {
|
||||
logger.Warn(ctx, "oauth2: blocked login, account already linked to different identity",
|
||||
slog.F("email", verifiedEmail.GetEmail()),
|
||||
@@ -1187,6 +1187,12 @@ type OIDCConfig struct {
|
||||
// SignupsDisabledText is the text do display on the static error page.
|
||||
SignupsDisabledText string
|
||||
PKCEMethods []promoauth.Oauth2PKCEChallengeMethod
|
||||
// EmailFallback, when true, allows OIDC logins to fall back to
|
||||
// email-based user matching when the linked_id (issuer+subject) does
|
||||
// not match an existing user link. INSECURE: weakens the linked_id
|
||||
// check. Used for IdP brokers that do not issue a stable `sub` for the
|
||||
// same user across connections.
|
||||
EmailFallback bool
|
||||
}
|
||||
|
||||
// PKCESupported is to prevent nil pointer dereference.
|
||||
@@ -1458,7 +1464,7 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
|
||||
}
|
||||
ctx = slog.With(ctx, slog.F("email", email), slog.F("username", username), slog.F("name", name))
|
||||
|
||||
user, link, err := findLinkedUser(ctx, api.Database, oidcLinkedID(idToken), database.LoginTypeOIDC, email)
|
||||
user, link, err := findLinkedUser(ctx, api.Database, oidcLinkedID(idToken), database.LoginTypeOIDC, api.OIDCConfig.EmailFallback, email)
|
||||
if errors.Is(err, errLinkedIDAlreadyBound) {
|
||||
logger.Warn(ctx, "oauth2: blocked login, account already linked to different identity",
|
||||
slog.F("email", email),
|
||||
@@ -1526,6 +1532,7 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
|
||||
UserInfoClaims: supplementaryClaims,
|
||||
MergedClaims: mergedClaims,
|
||||
},
|
||||
AllowInsecureLinkedIDMismatch: api.OIDCConfig.EmailFallback,
|
||||
}).SetInitAuditRequest(func(params *audit.RequestParams) (*audit.Request[database.User], func()) {
|
||||
return audit.InitRequest[database.User](rw, params)
|
||||
})
|
||||
@@ -1679,6 +1686,13 @@ type oauthLoginParams struct {
|
||||
// It is used to save the user's claims on login.
|
||||
UserClaims database.UserLinkClaims
|
||||
|
||||
// AllowInsecureLinkedIDMismatch, when true, allows the login to proceed
|
||||
// when the existing user_link's linked_id differs from LinkedID. The
|
||||
// existing linked_id is preserved (no overwrite). INSECURE: opt-in
|
||||
// escape hatch for IdP brokers that emit different subjects for the
|
||||
// same user across connections.
|
||||
AllowInsecureLinkedIDMismatch bool
|
||||
|
||||
commitLock sync.Mutex
|
||||
initAuditRequest func(params *audit.RequestParams) *audit.Request[database.User]
|
||||
commits []func()
|
||||
@@ -1929,7 +1943,10 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C
|
||||
// Defense-in-depth: if a concurrent transaction backfilled
|
||||
// linked_id between findLinkedUser and this point, reject the
|
||||
// login with a 403 instead of letting it bubble up as a 500.
|
||||
if link.LinkedID != "" && link.LinkedID != params.LinkedID {
|
||||
// The INSECURE AllowInsecureLinkedIDMismatch escape hatch
|
||||
// preserves the existing linked_id and lets the login proceed;
|
||||
// the warning was already emitted by the caller.
|
||||
if link.LinkedID != "" && link.LinkedID != params.LinkedID && !params.AllowInsecureLinkedIDMismatch {
|
||||
return &idpsync.HTTPError{
|
||||
Code: http.StatusForbidden,
|
||||
Msg: "Account already linked",
|
||||
@@ -2180,7 +2197,16 @@ var errLinkedIDAlreadyBound = xerrors.New("user account is already linked to a d
|
||||
// legacy links (empty linked_id) only. If the user found by email
|
||||
// already has a link with a different linked_id, errLinkedIDAlreadyBound
|
||||
// is returned to prevent account takeover via IdP email reuse.
|
||||
func findLinkedUser(ctx context.Context, db database.Store, linkedID string, loginType database.LoginType, emails ...string) (database.User, database.UserLink, error) {
|
||||
//
|
||||
// When allowInsecureLinkedIDMismatch is true, the linked_id mismatch
|
||||
// check is skipped and the email fallback resolves the login even when
|
||||
// the existing link's linked_id differs from the current login's. The
|
||||
// existing linked_id is left intact (no overwrite). This is an INSECURE
|
||||
// opt-in for IdP brokers that do not issue a stable `sub` for the same
|
||||
// user across connections.
|
||||
//
|
||||
//nolint:revive // allowInsecureLinkedIDMismatch is intentionally a control flag; it gates an INSECURE opt-in.
|
||||
func findLinkedUser(ctx context.Context, db database.Store, linkedID string, loginType database.LoginType, allowInsecureLinkedIDMismatch bool, emails ...string) (database.User, database.UserLink, error) {
|
||||
var (
|
||||
user database.User
|
||||
link database.UserLink
|
||||
@@ -2233,8 +2259,10 @@ func findLinkedUser(ctx context.Context, db database.Store, linkedID string, log
|
||||
|
||||
// Block email fallback when an existing link has a different linked_id.
|
||||
// Prevents account takeover via IdP email reuse; first-time and legacy
|
||||
// (empty linked_id) links pass through.
|
||||
if err == nil && link.LinkedID != "" && link.LinkedID != linkedID {
|
||||
// (empty linked_id) links pass through. The INSECURE
|
||||
// allowInsecureLinkedIDMismatch escape hatch keeps the existing link
|
||||
// (and its original linked_id) and lets the login proceed.
|
||||
if err == nil && link.LinkedID != "" && link.LinkedID != linkedID && !allowInsecureLinkedIDMismatch {
|
||||
return database.User{}, database.UserLink{}, errLinkedIDAlreadyBound
|
||||
}
|
||||
|
||||
|
||||
@@ -2001,6 +2001,176 @@ func TestUserOIDC(t *testing.T) {
|
||||
"linked_id must not be modified when the login is blocked")
|
||||
})
|
||||
|
||||
// Tests the INSECURE OIDC email fallback escape hatch. When the
|
||||
// deployment flag is set, an OIDC login whose subject differs from an
|
||||
// existing user_link's linked_id but whose email matches must be
|
||||
// allowed through. The original linked_id is preserved (no overwrite),
|
||||
// so the user can keep logging in with either subject.
|
||||
t.Run("OIDCInsecureEmailFallbackAllowed", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
ctx := testutil.Context(t, testutil.WaitShort)
|
||||
|
||||
fake := oidctest.NewFakeIDP(t,
|
||||
oidctest.WithRefresh(func(_ string) error {
|
||||
return xerrors.New("refreshing token should never occur")
|
||||
}),
|
||||
oidctest.WithServing(),
|
||||
)
|
||||
cfg := fake.OIDCConfig(t, nil, func(cfg *coderd.OIDCConfig) {
|
||||
cfg.AllowSignups = true
|
||||
cfg.EmailFallback = true
|
||||
})
|
||||
|
||||
logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug)
|
||||
owner, db := coderdtest.NewWithDatabase(t, &coderdtest.Options{
|
||||
OIDCConfig: cfg,
|
||||
Logger: &logger,
|
||||
})
|
||||
|
||||
// Seed a user whose link records the IdP's first connection.
|
||||
user := dbgen.User(t, db, database.User{
|
||||
LoginType: database.LoginTypeOIDC,
|
||||
})
|
||||
originalLinkedID := fake.IssuerURL().String() + "||" + "first-connection-sub"
|
||||
dbgen.UserLink(t, db, database.UserLink{
|
||||
UserID: user.ID,
|
||||
LoginType: database.LoginTypeOIDC,
|
||||
LinkedID: originalLinkedID,
|
||||
})
|
||||
|
||||
// Login with a different subject (the broker emitted a new `sub`
|
||||
// for the same user) but the same email. With EmailFallback
|
||||
// enabled the email match resolves the login despite the linked_id
|
||||
// mismatch.
|
||||
client, resp := fake.AttemptLogin(t, owner, jwt.MapClaims{
|
||||
"email": user.Email,
|
||||
"sub": "second-connection-sub",
|
||||
})
|
||||
require.Equal(t, http.StatusOK, resp.StatusCode,
|
||||
"insecure email fallback must allow login with a mismatched subject")
|
||||
|
||||
me, err := client.User(ctx, "me")
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, user.ID, me.ID,
|
||||
"should authenticate as the existing user")
|
||||
|
||||
// The original linked_id must be preserved, not overwritten by the
|
||||
// new subject. This keeps the next login from the original subject
|
||||
// (which hits the primary linked_id path) working.
|
||||
link, err := db.GetUserLinkByUserIDLoginType(dbauthz.AsSystemRestricted(context.Background()), database.GetUserLinkByUserIDLoginTypeParams{
|
||||
UserID: user.ID,
|
||||
LoginType: database.LoginTypeOIDC,
|
||||
})
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, originalLinkedID, link.LinkedID,
|
||||
"linked_id must be preserved on insecure email fallback")
|
||||
})
|
||||
|
||||
// Tests that with the INSECURE OIDC email fallback enabled, the
|
||||
// original subject's login still resolves via the primary linked_id
|
||||
// path after a fallback login from a different subject. The fallback
|
||||
// path does not overwrite the link, so the original subject keeps
|
||||
// matching directly.
|
||||
t.Run("OIDCInsecureEmailFallbackPreservesOriginalLogin", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
ctx := testutil.Context(t, testutil.WaitShort)
|
||||
|
||||
fake := oidctest.NewFakeIDP(t,
|
||||
oidctest.WithRefresh(func(_ string) error {
|
||||
return xerrors.New("refreshing token should never occur")
|
||||
}),
|
||||
oidctest.WithServing(),
|
||||
)
|
||||
cfg := fake.OIDCConfig(t, nil, func(cfg *coderd.OIDCConfig) {
|
||||
cfg.AllowSignups = true
|
||||
cfg.EmailFallback = true
|
||||
})
|
||||
|
||||
logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug)
|
||||
owner, db := coderdtest.NewWithDatabase(t, &coderdtest.Options{
|
||||
OIDCConfig: cfg,
|
||||
Logger: &logger,
|
||||
})
|
||||
|
||||
user := dbgen.User(t, db, database.User{
|
||||
LoginType: database.LoginTypeOIDC,
|
||||
})
|
||||
const originalSub = "first-connection-sub"
|
||||
originalLinkedID := fake.IssuerURL().String() + "||" + originalSub
|
||||
dbgen.UserLink(t, db, database.UserLink{
|
||||
UserID: user.ID,
|
||||
LoginType: database.LoginTypeOIDC,
|
||||
LinkedID: originalLinkedID,
|
||||
})
|
||||
|
||||
// Fallback login with a different subject.
|
||||
_, resp := fake.AttemptLogin(t, owner, jwt.MapClaims{
|
||||
"email": user.Email,
|
||||
"sub": "second-connection-sub",
|
||||
})
|
||||
require.Equal(t, http.StatusOK, resp.StatusCode)
|
||||
|
||||
// Subsequent login with the original subject must hit the primary
|
||||
// linked_id match and succeed.
|
||||
client, resp := fake.AttemptLogin(t, owner, jwt.MapClaims{
|
||||
"email": user.Email,
|
||||
"sub": originalSub,
|
||||
})
|
||||
require.Equal(t, http.StatusOK, resp.StatusCode)
|
||||
|
||||
me, err := client.User(ctx, "me")
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, user.ID, me.ID)
|
||||
|
||||
link, err := db.GetUserLinkByUserIDLoginType(dbauthz.AsSystemRestricted(context.Background()), database.GetUserLinkByUserIDLoginTypeParams{
|
||||
UserID: user.ID,
|
||||
LoginType: database.LoginTypeOIDC,
|
||||
})
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, originalLinkedID, link.LinkedID,
|
||||
"linked_id must stay anchored to the original subject")
|
||||
})
|
||||
|
||||
// Tests that the INSECURE OIDC email fallback does NOT extend to
|
||||
// signups: an attacker logging in with a brand-new email (no existing
|
||||
// user) still goes through the normal signup gate. The escape hatch is
|
||||
// only about resolving subject-mismatch on existing accounts.
|
||||
t.Run("OIDCInsecureEmailFallbackDoesNotCreateUsers", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
fake := oidctest.NewFakeIDP(t,
|
||||
oidctest.WithRefresh(func(_ string) error {
|
||||
return xerrors.New("refreshing token should never occur")
|
||||
}),
|
||||
oidctest.WithServing(),
|
||||
)
|
||||
cfg := fake.OIDCConfig(t, nil, func(cfg *coderd.OIDCConfig) {
|
||||
cfg.AllowSignups = false
|
||||
cfg.EmailFallback = true
|
||||
})
|
||||
|
||||
logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug)
|
||||
owner, db := coderdtest.NewWithDatabase(t, &coderdtest.Options{
|
||||
OIDCConfig: cfg,
|
||||
Logger: &logger,
|
||||
})
|
||||
|
||||
// Seed an existing user so the deployment's user count is > 0;
|
||||
// otherwise the first signup is always allowed.
|
||||
dbgen.User(t, db, database.User{
|
||||
LoginType: database.LoginTypeOIDC,
|
||||
})
|
||||
|
||||
// New email, no existing user. Signups are disabled, so the
|
||||
// fallback flag must not let the login through.
|
||||
_, resp := fake.AttemptLogin(t, owner, jwt.MapClaims{
|
||||
"email": "stranger@example.com",
|
||||
"sub": "stranger-subject",
|
||||
})
|
||||
require.Equal(t, http.StatusForbidden, resp.StatusCode,
|
||||
"insecure email fallback must not bypass the signup gate")
|
||||
})
|
||||
|
||||
t.Run("OIDCSuspended", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
ctx := testutil.Context(t, testutil.WaitShort)
|
||||
|
||||
Reference in New Issue
Block a user