mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
fix(coderd)!: restrict OIDC email fallback to first-time account linking (#25712)
## Problem `findLinkedUser` in `coderd/userauth.go` falls back to email-based user lookup when no `linked_id` match is found. This fallback was used for **all logins**, not just first-time linking. An attacker who registers the victim's email at the IdP (with a different OIDC subject) bypasses the `linked_id` check and gets matched to the victim's Coder account. Combined with the `email_verified` type assertion bypass (PLAT-228), this creates a chained account-takeover vector. ## Fix Restrict the email fallback in `findLinkedUser` so that when a user found by email already has a `user_link` with a non-empty `linked_id` that **differs** from the current login's `linked_id`, the function returns no user. This blocks account takeover while preserving: - **First-time linking**: No existing `user_link` exists, email fallback works as before. - **Legacy links**: Empty `linked_id` (pre-migration), email fallback still works. - **Normal logins**: Matching `linked_id` resolves via the primary path, no fallback needed. Also adds a `UpdateUserLinkedID` query to backfill `linked_id` on legacy links (only when currently empty) during login, gradually migrating them to the secure path. The `findLinkedUser` signature now accepts `loginType` explicitly instead of relying on `user.LoginType`, ensuring the correct link is checked in the legacy lookup. ## Breaking change Marked `release/breaking`. An account whose `user_link` already has a populated `linked_id` that does not match the subject the IdP presents will now be denied login (403) instead of silently resolving via the email fallback. The most likely trigger is changing `CODER_OIDC_ISSUER_URL` (the `linked_id` is `issuer||subject`), or two identities sharing one email. Accounts with an empty (legacy) `linked_id` are unaffected and are backfilled on their next login. Fixes: https://linear.app/codercom/issue/PLAT-229 <details><summary>Implementation details</summary> ### Files changed - `coderd/userauth.go`: Core fix in `findLinkedUser` + backfill logic in `oauthLogin` - `coderd/database/queries/user_links.sql`: New `UpdateUserLinkedID` query - `coderd/database/dbauthz/dbauthz.go`: Authorization for new query (`ActionUpdate` on the user object, matching `InsertUserLink`) - `coderd/userauth_test.go`: New OIDC and GitHub tests - Generated files: `queries.sql.go`, `querier.go`, `dbmock.go`, `querymetrics.go` ### New tests - `TestUserOIDC/OIDCEmailFallbackBlockedByExistingLink`: Attacker with a different `sub` but the same email is rejected (403) when the victim has an existing link (covers signups enabled and disabled). - `TestUserOIDC/OIDCFirstTimeLinkByEmailAllowed`: User created via SCIM/API (no `user_link`) can still link via email on first OIDC login, and the `linked_id` is populated. - `TestUserOIDC/OIDCLegacyLinkBackfill`: User with empty `linked_id` can login and their `linked_id` is backfilled with the correct value. - `TestUserOIDC/OIDCEmailFallbackBlockedByIssuerChange`: Existing link recorded under a previous issuer is rejected (403) after the issuer changes (documents the breaking behavior). - `TestUserOAuth2Github/EmailFallbackBlockedByExistingLink`: GitHub attacker with a different user ID but the victim's email is rejected (403). </details> > [!NOTE] > This PR was authored by Coder Agents on behalf of @f0ssel. --------- Co-authored-by: Coder Agents <agents@coder.com>
This commit is contained in:
Garrett Delfosse
co-authored by
Coder Agents
Coder Agents
parent
76bf462bbf
commit
53d287a139
@@ -386,6 +386,67 @@ func TestUserOAuth2Github(t *testing.T) {
|
||||
|
||||
require.Equal(t, http.StatusForbidden, resp.StatusCode)
|
||||
})
|
||||
t.Run("EmailFallbackBlockedByExistingLink", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
// A victim already has a GitHub link bound to a specific GitHub user
|
||||
// ID. An attacker authenticates with a different GitHub user ID but
|
||||
// the victim's verified email. The email fallback must not hand the
|
||||
// attacker the victim's account, even with signups enabled.
|
||||
owner, db := coderdtest.NewWithDatabase(t, &coderdtest.Options{
|
||||
GithubOAuth2Config: &coderd.GithubOAuth2Config{
|
||||
OAuth2Config: &testutil.OAuth2Config{},
|
||||
AllowSignups: true,
|
||||
AllowEveryone: true,
|
||||
ListOrganizationMemberships: func(_ context.Context, _ *http.Client) ([]*github.Membership, error) {
|
||||
return []*github.Membership{}, nil
|
||||
},
|
||||
TeamMembership: func(_ context.Context, _ *http.Client, _, _, _ string) (*github.Membership, error) {
|
||||
return nil, xerrors.New("no teams")
|
||||
},
|
||||
AuthenticatedUser: func(_ context.Context, _ *http.Client) (*github.User, error) {
|
||||
// Attacker's GitHub ID differs from the victim's link.
|
||||
return &github.User{
|
||||
ID: github.Int64(200),
|
||||
Login: github.String("attacker"),
|
||||
Name: github.String("Attacker"),
|
||||
}, nil
|
||||
},
|
||||
ListEmails: func(_ context.Context, _ *http.Client) ([]*github.UserEmail, error) {
|
||||
return []*github.UserEmail{{
|
||||
Email: github.String("victim@coder.com"),
|
||||
Verified: github.Bool(true),
|
||||
Primary: github.Bool(true),
|
||||
}}, nil
|
||||
},
|
||||
},
|
||||
})
|
||||
|
||||
// Seed the victim with an existing GitHub link (a different linked_id).
|
||||
victim := dbgen.User(t, db, database.User{
|
||||
Email: "victim@coder.com",
|
||||
LoginType: database.LoginTypeGithub,
|
||||
})
|
||||
const victimLinkedID = "100"
|
||||
dbgen.UserLink(t, db, database.UserLink{
|
||||
UserID: victim.ID,
|
||||
LoginType: database.LoginTypeGithub,
|
||||
LinkedID: victimLinkedID,
|
||||
})
|
||||
|
||||
resp := oauth2Callback(t, owner)
|
||||
require.Equal(t, http.StatusForbidden, resp.StatusCode,
|
||||
"attacker with a different GitHub ID must not authenticate as the victim")
|
||||
|
||||
// The victim's link must be untouched.
|
||||
victimLink, err := db.GetUserLinkByUserIDLoginType(dbauthz.AsSystemRestricted(context.Background()), database.GetUserLinkByUserIDLoginTypeParams{
|
||||
UserID: victim.ID,
|
||||
LoginType: database.LoginTypeGithub,
|
||||
})
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, victimLinkedID, victimLink.LinkedID,
|
||||
"victim's linked_id must remain unchanged")
|
||||
})
|
||||
t.Run("Signup", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
auditor := audit.NewMock()
|
||||
@@ -1624,6 +1685,244 @@ func TestUserOIDC(t *testing.T) {
|
||||
require.Equal(t, codersdk.UserStatusActive, me.Status)
|
||||
})
|
||||
|
||||
// Tests that an attacker with a different OIDC subject but the same
|
||||
// email cannot hijack an existing linked account. The email fallback
|
||||
// must be restricted to first-time linking only.
|
||||
t.Run("OIDCEmailFallbackBlockedByExistingLink", 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(),
|
||||
)
|
||||
|
||||
for _, tc := range []struct {
|
||||
name string
|
||||
allowSignups bool
|
||||
}{
|
||||
{"SignupsDisabled", false},
|
||||
{"SignupsEnabled", true},
|
||||
} {
|
||||
tc := tc
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
cfg := fake.OIDCConfig(t, nil, func(cfg *coderd.OIDCConfig) {
|
||||
cfg.AllowSignups = tc.allowSignups
|
||||
})
|
||||
|
||||
logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug)
|
||||
owner, db := coderdtest.NewWithDatabase(t, &coderdtest.Options{
|
||||
OIDCConfig: cfg,
|
||||
Logger: &logger,
|
||||
})
|
||||
|
||||
// Create a victim user with an existing OIDC link.
|
||||
// Use the fake IDP's issuer so the linked_id format is
|
||||
// realistic (same issuer, different subject).
|
||||
victim := dbgen.User(t, db, database.User{
|
||||
LoginType: database.LoginTypeOIDC,
|
||||
})
|
||||
victimLinkedID := fake.IssuerURL().String() + "||" + "victim-subject"
|
||||
dbgen.UserLink(t, db, database.UserLink{
|
||||
UserID: victim.ID,
|
||||
LoginType: database.LoginTypeOIDC,
|
||||
LinkedID: victimLinkedID,
|
||||
})
|
||||
|
||||
// Attacker tries to login with a different subject but the
|
||||
// same email. The email fallback is blocked because the victim
|
||||
// already has a user_link with a different linked_id.
|
||||
_, resp := fake.AttemptLogin(t, owner, jwt.MapClaims{
|
||||
"email": victim.Email,
|
||||
"sub": "attacker-subject",
|
||||
})
|
||||
require.Equal(t, http.StatusForbidden, resp.StatusCode,
|
||||
"attacker must not authenticate as the victim")
|
||||
|
||||
// Verify the victim's link is unchanged.
|
||||
victimLink, err := db.GetUserLinkByUserIDLoginType(dbauthz.AsSystemRestricted(context.Background()), database.GetUserLinkByUserIDLoginTypeParams{
|
||||
UserID: victim.ID,
|
||||
LoginType: database.LoginTypeOIDC,
|
||||
})
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, victimLinkedID, victimLink.LinkedID,
|
||||
"victim's linked_id must remain unchanged")
|
||||
})
|
||||
}
|
||||
})
|
||||
|
||||
// Tests that a first-time OIDC user can still link via email when no
|
||||
// user_link exists (e.g. a dormant OIDC user created via SCIM or API).
|
||||
t.Run("OIDCFirstTimeLinkByEmailAllowed", 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
|
||||
})
|
||||
|
||||
logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug)
|
||||
owner, db := coderdtest.NewWithDatabase(t, &coderdtest.Options{
|
||||
OIDCConfig: cfg,
|
||||
Logger: &logger,
|
||||
})
|
||||
|
||||
// Create a user with OIDC login type but NO user_link.
|
||||
// This simulates a user created via SCIM or the API.
|
||||
user := dbgen.User(t, db, database.User{
|
||||
LoginType: database.LoginTypeOIDC,
|
||||
})
|
||||
|
||||
// Login with a new OIDC subject and matching email.
|
||||
// This should succeed because no user_link exists.
|
||||
sub := uuid.NewString()
|
||||
client, resp := fake.AttemptLogin(t, owner, jwt.MapClaims{
|
||||
"email": user.Email,
|
||||
"sub": sub,
|
||||
})
|
||||
require.Equal(t, http.StatusOK, resp.StatusCode)
|
||||
|
||||
me, err := client.User(ctx, "me")
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, user.ID, me.ID,
|
||||
"should authenticate as the existing user")
|
||||
|
||||
// Verify the created link has a populated linked_id.
|
||||
link, err := db.GetUserLinkByUserIDLoginType(
|
||||
dbauthz.AsSystemRestricted(context.Background()),
|
||||
database.GetUserLinkByUserIDLoginTypeParams{
|
||||
UserID: user.ID,
|
||||
LoginType: database.LoginTypeOIDC,
|
||||
})
|
||||
require.NoError(t, err)
|
||||
expectedLinkedID := fake.IssuerURL().String() + "||" + sub
|
||||
require.Equal(t, expectedLinkedID, link.LinkedID,
|
||||
"link should have the correct linked_id after first-time linking")
|
||||
})
|
||||
|
||||
// Tests that a legacy user with an empty linked_id can still login
|
||||
// and that their linked_id is backfilled with the correct value.
|
||||
t.Run("OIDCLegacyLinkBackfill", 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
|
||||
})
|
||||
|
||||
logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug)
|
||||
owner, db := coderdtest.NewWithDatabase(t, &coderdtest.Options{
|
||||
OIDCConfig: cfg,
|
||||
Logger: &logger,
|
||||
})
|
||||
|
||||
// Create a legacy user with an empty linked_id.
|
||||
user := dbgen.User(t, db, database.User{
|
||||
LoginType: database.LoginTypeOIDC,
|
||||
})
|
||||
dbgen.UserLink(t, db, database.UserLink{
|
||||
UserID: user.ID,
|
||||
LoginType: database.LoginTypeOIDC,
|
||||
LinkedID: "", // Legacy: empty linked_id
|
||||
})
|
||||
|
||||
sub := uuid.NewString()
|
||||
client, resp := fake.AttemptLogin(t, owner, jwt.MapClaims{
|
||||
"email": user.Email,
|
||||
"sub": sub,
|
||||
})
|
||||
require.Equal(t, http.StatusOK, resp.StatusCode)
|
||||
|
||||
me, err := client.User(ctx, "me")
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, user.ID, me.ID,
|
||||
"legacy user should still be able to login via email fallback")
|
||||
|
||||
// Verify the linked_id was backfilled with the correct value.
|
||||
link, err := db.GetUserLinkByUserIDLoginType(
|
||||
dbauthz.AsSystemRestricted(context.Background()),
|
||||
database.GetUserLinkByUserIDLoginTypeParams{
|
||||
UserID: user.ID,
|
||||
LoginType: database.LoginTypeOIDC,
|
||||
})
|
||||
require.NoError(t, err)
|
||||
expectedLinkedID := fake.IssuerURL().String() + "||" + sub
|
||||
require.Equal(t, expectedLinkedID, link.LinkedID,
|
||||
"linked_id should be backfilled with the correct value after login")
|
||||
})
|
||||
|
||||
// Tests that changing the OIDC issuer URL blocks an existing user whose
|
||||
// linked_id was recorded under the old issuer. This is a deliberate
|
||||
// breaking change: before this fix the email fallback silently rescued
|
||||
// such users. Now the login is rejected because the existing link's
|
||||
// linked_id (old issuer) differs from the newly computed one (new issuer).
|
||||
t.Run("OIDCEmailFallbackBlockedByIssuerChange", 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
|
||||
})
|
||||
|
||||
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 was created under a different (old) issuer
|
||||
// but with the same subject the IdP presents on login.
|
||||
user := dbgen.User(t, db, database.User{
|
||||
LoginType: database.LoginTypeOIDC,
|
||||
})
|
||||
const sub = "stable-subject"
|
||||
oldLinkedID := "https://old-issuer.example.com||" + sub
|
||||
dbgen.UserLink(t, db, database.UserLink{
|
||||
UserID: user.ID,
|
||||
LoginType: database.LoginTypeOIDC,
|
||||
LinkedID: oldLinkedID,
|
||||
})
|
||||
|
||||
// Login presents the same subject but the current issuer, so the
|
||||
// computed linked_id differs from the stored one and is blocked.
|
||||
_, resp := fake.AttemptLogin(t, owner, jwt.MapClaims{
|
||||
"email": user.Email,
|
||||
"sub": sub,
|
||||
})
|
||||
require.Equal(t, http.StatusForbidden, resp.StatusCode,
|
||||
"issuer change must block the email fallback for an existing link")
|
||||
|
||||
// The stored link must remain unchanged.
|
||||
link, err := db.GetUserLinkByUserIDLoginType(dbauthz.AsSystemRestricted(ctx), database.GetUserLinkByUserIDLoginTypeParams{
|
||||
UserID: user.ID,
|
||||
LoginType: database.LoginTypeOIDC,
|
||||
})
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, oldLinkedID, link.LinkedID,
|
||||
"linked_id must not be modified when the login is blocked")
|
||||
})
|
||||
|
||||
t.Run("OIDCConvert", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
|
||||
Reference in New Issue
Block a user