mirror of
https://github.com/coder/coder.git
synced 2026-06-05 14:08:20 +00:00
53d287a139
## 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>
116 lines
3.3 KiB
SQL
116 lines
3.3 KiB
SQL
-- name: GetUserLinkByLinkedID :one
|
|
SELECT
|
|
user_links.*
|
|
FROM
|
|
user_links
|
|
INNER JOIN
|
|
users ON user_links.user_id = users.id
|
|
WHERE
|
|
linked_id = $1
|
|
AND
|
|
deleted = false;
|
|
|
|
-- name: GetUserLinkByUserIDLoginType :one
|
|
SELECT
|
|
*
|
|
FROM
|
|
user_links
|
|
WHERE
|
|
user_id = $1 AND login_type = $2;
|
|
|
|
-- name: GetUserLinksByUserID :many
|
|
SELECT * FROM user_links WHERE user_id = $1;
|
|
|
|
-- name: InsertUserLink :one
|
|
INSERT INTO
|
|
user_links (
|
|
user_id,
|
|
login_type,
|
|
linked_id,
|
|
oauth_access_token,
|
|
oauth_access_token_key_id,
|
|
oauth_refresh_token,
|
|
oauth_refresh_token_key_id,
|
|
oauth_expiry,
|
|
claims
|
|
)
|
|
VALUES
|
|
( $1, $2, $3, $4, $5, $6, $7, $8, $9 ) RETURNING *;
|
|
|
|
-- name: UpdateUserLink :one
|
|
UPDATE
|
|
user_links
|
|
SET
|
|
oauth_access_token = $1,
|
|
oauth_access_token_key_id = $2,
|
|
oauth_refresh_token = $3,
|
|
oauth_refresh_token_key_id = $4,
|
|
oauth_expiry = $5,
|
|
claims = $6
|
|
WHERE
|
|
user_id = $7 AND login_type = $8 RETURNING *;
|
|
|
|
-- name: UpdateUserLinkedID :one
|
|
-- Backfills linked_id for legacy user_links that were created before
|
|
-- linked_id tracking was added. Only updates when linked_id is empty
|
|
-- to avoid overwriting a valid binding.
|
|
UPDATE
|
|
user_links
|
|
SET
|
|
linked_id = @linked_id
|
|
WHERE
|
|
user_id = @user_id AND login_type = @login_type AND linked_id = '' RETURNING *;
|
|
|
|
-- name: OIDCClaimFields :many
|
|
-- OIDCClaimFields returns a list of distinct keys in the the merged_claims fields.
|
|
-- This query is used to generate the list of available sync fields for idp sync settings.
|
|
SELECT
|
|
DISTINCT jsonb_object_keys(claims->'merged_claims')
|
|
FROM
|
|
user_links
|
|
WHERE
|
|
-- Only return rows where the top level key exists
|
|
claims ? 'merged_claims' AND
|
|
-- 'null' is the default value for the id_token_claims field
|
|
-- jsonb 'null' is not the same as SQL NULL. Strip these out.
|
|
jsonb_typeof(claims->'merged_claims') != 'null' AND
|
|
login_type = 'oidc'
|
|
AND CASE WHEN @organization_id :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN
|
|
user_links.user_id = ANY(SELECT organization_members.user_id FROM organization_members WHERE organization_id = @organization_id)
|
|
ELSE true
|
|
END
|
|
;
|
|
|
|
-- name: OIDCClaimFieldValues :many
|
|
SELECT
|
|
-- DISTINCT to remove duplicates
|
|
DISTINCT jsonb_array_elements_text(CASE
|
|
-- When the type is an array, filter out any non-string elements.
|
|
-- This is to keep the return type consistent.
|
|
WHEN jsonb_typeof(claims->'merged_claims'->sqlc.arg('claim_field')::text) = 'array' THEN
|
|
(
|
|
SELECT
|
|
jsonb_agg(element)
|
|
FROM
|
|
jsonb_array_elements(claims->'merged_claims'->sqlc.arg('claim_field')::text) AS element
|
|
WHERE
|
|
-- Filtering out non-string elements
|
|
jsonb_typeof(element) = 'string'
|
|
)
|
|
-- Some IDPs return a single string instead of an array of strings.
|
|
WHEN jsonb_typeof(claims->'merged_claims'->sqlc.arg('claim_field')::text) = 'string' THEN
|
|
jsonb_build_array(claims->'merged_claims'->sqlc.arg('claim_field')::text)
|
|
END)
|
|
FROM
|
|
user_links
|
|
WHERE
|
|
-- IDP sync only supports string and array (of string) types
|
|
jsonb_typeof(claims->'merged_claims'->sqlc.arg('claim_field')::text) = ANY(ARRAY['string', 'array'])
|
|
AND login_type = 'oidc'
|
|
AND CASE
|
|
WHEN @organization_id :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN
|
|
user_links.user_id = ANY(SELECT organization_members.user_id FROM organization_members WHERE organization_id = @organization_id)
|
|
ELSE true
|
|
END
|
|
;
|