Mutli-user credentials

ryansquared commented

2026-10-14 08:47:53 +00:00

(Migrated from codeberg.org)

Also includes cargo fmt and cargo clippy fixes that were missed from previous PRs.

Also includes `cargo fmt` and `cargo clippy` fixes that were missed from previous PRs.

vkobel commented

2026-10-14 09:19:09 +00:00

(Migrated from codeberg.org)

Review: multi-user-per-credential (@ 2270de34)

Blocking

API session can resolve to the wrong account: src/api/src/middleware.rs:191
validate_session still joins auth_sessions → fido2_credentials on credential_id only. With a shared key this returns one row per owner, and fetch_optional takes whichever comes first. So Bob's session can be authenticated as Alice on any API route. It should read the new s.user_id.
Migrations fail on existing DBs: src/api/migrations/001_initial.sql:336
001 now creates idx_sessions_user_id. On an existing DB, CREATE TABLE IF NOT EXISTS does nothing, so the user_id column doesn't exist until 057 adds it. utils/makefile-run-migrations.sh runs with ON_ERROR_STOP=1, so the run stops at 001 and 056/057 never apply. Move the index into 057.
Decoy login panics: src/gateway/src/handlers/webauthn/login.rs:284
expected_user_id() hits unreachable!() on Decoy { expected_user_id: None }, which is the scope for an unknown or rate-limited username. All four finish paths call it for any Decoy (login.rs:784,865, qr_auth.rs:874,936). The connection drops instead of returning the opaque 401, which also tells an attacker whether a username exists.
Deleting a passkey no longer ends its sessions: src/api/migrations/056_credential_multi_account.sql:13
Dropping fk_auth_sessions_credential also removes its ON DELETE CASCADE, and nothing replaces it. Passkey delete (gateway/src/db.rs:858) and the admin WebAuthn reset now leave existing sessions valid. A composite FK (user_id, credential_id) → fido2_credentials(user_id, credential_id) ON DELETE CASCADE would restore the cascade.
Release approval can be hijacked by reusing a victim's credential ID: src/gateway/src/release_relay.rs:43
Credential IDs are public (they're sent in allowCredentials), and with none-attestation the client chooses the rawId. Mallory registers Alice's credential ID with Mallory's own key, the owner set becomes [alice, mallory], and sole_owner lets Mallory open Alice's approval page. The old code explicitly rejected this.

Should fix

Username-less login rejects shared keys: login.rs:857 (also login.rs:771, qr_auth.rs:924)
The unscoped finish requires exactly one owner and ignores the assertion's userHandle. Legitimate shared-key users always get a 401, and the trick in #5 lets anyone lock a victim out of username-less login. users.fido2_user_handle could pick the right owner. The comment "resolve the user from the assertion's userHandle" is now stale.
Re-adding your own key returns 500: src/gateway/src/handlers/credentials/passkey.rs:324
excludeCredentials and the credential_exists 409 check were removed instead of being scoped to (user_id, credential_id). Re-adding a key now hits the new unique index and returns a generic 500. For a resident key, it silently overwrites the credential on the authenticator and leaves a dead row in the DB.

Nits

Backfill picks an arbitrary owner: src/api/migrations/057_session_user_id.sql:19
The UPDATE … FROM joins on credential_id only, so a shared credential gets any one of its owners. The comment says ambiguous sessions are deleted. This only matters on reruns or out-of-order runs.
Owner-resolution block is copy-pasted 4 times: login.rs:771,855, qr_auth.rs:862,924
The #3 panic is in all four copies. One resolve_owner(owners, scope) -> Option<Uuid> helper would fix both problems.
One query per credential in the release relay: src/gateway/src/release_relay.rs:173
It runs one get_user_ids_by_credential per holder credential (up to 64) inside a time-limited window. The old batched ANY($1) query did this in a single round trip.

#	Issue	Severity	Impact	Fix effort
1	API session resolves to wrong account	Critical	Cross-account auth on all API routes	Trivial
2	001 index breaks upgrade	Critical	Existing DBs can't migrate	Trivial
3	Decoy `unreachable!()`	High	Login crash, reveals whether a username exists	Low
4	Key delete doesn't end sessions	High	Revoked key keeps its sessions	Low
5	Credential-ID reuse in release relay	High	Hijack of release approval	Medium (design)
6	Unscoped login ignores userHandle	Medium	Targeted lockout	Medium
7	Re-add own key returns 500	Medium	Bad UX, dead rows	Low
8	057 backfill picks arbitrary owner	Low	Rerun edge case	Trivial
9	Resolution block duplicated 4×	Low	Copies drift apart	Low
10	Per-credential owner queries	Low	Latency	Trivial

Open questions

Credential-ID reuse (#5/#6): reject a registration whose credential ID already exists with a different public key, or tell owners apart by userHandle? Rejecting closes both.
Same key re-added to the same account (#7): return 409 like before, or silently dedupe?

**Review: multi-user-per-credential (@ 2270de34)** ### Blocking 1. **API session can resolve to the wrong account**: `src/api/src/middleware.rs:191` `validate_session` still joins `auth_sessions → fido2_credentials` on `credential_id` only. With a shared key this returns one row per owner, and `fetch_optional` takes whichever comes first. So Bob's session can be authenticated as Alice on any API route. It should read the new `s.user_id`. 2. **Migrations fail on existing DBs**: `src/api/migrations/001_initial.sql:336` 001 now creates `idx_sessions_user_id`. On an existing DB, `CREATE TABLE IF NOT EXISTS` does nothing, so the `user_id` column doesn't exist until 057 adds it. `utils/makefile-run-migrations.sh` runs with `ON_ERROR_STOP=1`, so the run stops at 001 and 056/057 never apply. Move the index into 057. 3. **Decoy login panics**: `src/gateway/src/handlers/webauthn/login.rs:284` `expected_user_id()` hits `unreachable!()` on `Decoy { expected_user_id: None }`, which is the scope for an unknown or rate-limited username. All four finish paths call it for any Decoy (`login.rs:784,865`, `qr_auth.rs:874,936`). The connection drops instead of returning the opaque 401, which also tells an attacker whether a username exists. 4. **Deleting a passkey no longer ends its sessions**: `src/api/migrations/056_credential_multi_account.sql:13` Dropping `fk_auth_sessions_credential` also removes its `ON DELETE CASCADE`, and nothing replaces it. Passkey delete (`gateway/src/db.rs:858`) and the admin WebAuthn reset now leave existing sessions valid. A composite FK `(user_id, credential_id) → fido2_credentials(user_id, credential_id) ON DELETE CASCADE` would restore the cascade. 5. **Release approval can be hijacked by reusing a victim's credential ID**: `src/gateway/src/release_relay.rs:43` Credential IDs are public (they're sent in `allowCredentials`), and with none-attestation the client chooses the rawId. Mallory registers Alice's credential ID with Mallory's own key, the owner set becomes `[alice, mallory]`, and `sole_owner` lets Mallory open Alice's approval page. The old code explicitly rejected this. ### Should fix 6. **Username-less login rejects shared keys**: `login.rs:857` (also `login.rs:771`, `qr_auth.rs:924`) The unscoped finish requires exactly one owner and ignores the assertion's `userHandle`. Legitimate shared-key users always get a 401, and the trick in #5 lets anyone lock a victim out of username-less login. `users.fido2_user_handle` could pick the right owner. The comment "resolve the user from the assertion's userHandle" is now stale. 7. **Re-adding your own key returns 500**: `src/gateway/src/handlers/credentials/passkey.rs:324` `excludeCredentials` and the `credential_exists` 409 check were removed instead of being scoped to `(user_id, credential_id)`. Re-adding a key now hits the new unique index and returns a generic 500. For a resident key, it silently overwrites the credential on the authenticator and leaves a dead row in the DB. ### Nits 8. **Backfill picks an arbitrary owner**: `src/api/migrations/057_session_user_id.sql:19` The `UPDATE … FROM` joins on `credential_id` only, so a shared credential gets any one of its owners. The comment says ambiguous sessions are deleted. This only matters on reruns or out-of-order runs. 9. **Owner-resolution block is copy-pasted 4 times**: `login.rs:771,855`, `qr_auth.rs:862,924` The #3 panic is in all four copies. One `resolve_owner(owners, scope) -> Option<Uuid>` helper would fix both problems. 10. **One query per credential in the release relay**: `src/gateway/src/release_relay.rs:173` It runs one `get_user_ids_by_credential` per holder credential (up to 64) inside a time-limited window. The old batched `ANY($1)` query did this in a single round trip. | # | Issue | Severity | Impact | Fix effort | |---|---|---|---|---| | 1 | API session resolves to wrong account | Critical | Cross-account auth on all API routes | Trivial | | 2 | 001 index breaks upgrade | Critical | Existing DBs can't migrate | Trivial | | 3 | Decoy `unreachable!()` | High | Login crash, reveals whether a username exists | Low | | 4 | Key delete doesn't end sessions | High | Revoked key keeps its sessions | Low | | 5 | Credential-ID reuse in release relay | High | Hijack of release approval | Medium (design) | | 6 | Unscoped login ignores userHandle | Medium | Targeted lockout | Medium | | 7 | Re-add own key returns 500 | Medium | Bad UX, dead rows | Low | | 8 | 057 backfill picks arbitrary owner | Low | Rerun edge case | Trivial | | 9 | Resolution block duplicated 4× | Low | Copies drift apart | Low | | 10 | Per-credential owner queries | Low | Latency | Trivial | **Open questions** - Credential-ID reuse (#5/#6): reject a registration whose credential ID already exists with a *different* public key, or tell owners apart by `userHandle`? Rejecting closes both. - Same key re-added to the same account (#7): return 409 like before, or silently dedupe?

ryansquared commented

2026-10-14 23:48:11 +00:00

(Migrated from codeberg.org)

Working on QR but non-username'd users shouldn't exist anymore.

ryansquared commented

2026-10-14 23:49:02 +00:00

(Migrated from codeberg.org)

excludeCredentials and the credential_exists 409 check were removed instead of being scoped to (user_id, credential_id). Re-adding a key now hits the new unique index and returns a generic 500. For a resident key, it silently overwrites the credential on the authenticator and leaves a dead row in the DB.

Needs fixing:

We should return a 409 when the key exists in the database already
We must not leave dead rows in the DB

> excludeCredentials and the credential_exists 409 check were removed instead of being scoped to (user_id, credential_id). Re-adding a key now hits the new unique index and returns a generic 500. For a resident key, it silently overwrites the credential on the authenticator and leaves a dead row in the DB. Needs fixing: - We should return a 409 when the key exists in the database already - We must not leave dead rows in the DB

ryan added 2 commits

2026-10-15 05:30:34 +00:00

gateway: return 409 for existing credentials 26fcd19677

gateway::db::passkey_usage_tests: pin by user_id ead94d2b69

vincent commented

2026-10-15 09:18:45 +00:00

Owner

Review: ryansquared/multi-user-per-credential (@ 2e56f17e)

Fixed since last review: API session lookup, 001 index, decoy panic, and session cascade on passkey delete.

Blocking

An org member can get past the forged-registration check: src/gateway/src/db.rs:634 (release_relay.rs:43, src/api/src/cryptographic_bundles/holders.rs:137)
The check accepts a reused credential ID if the new registration's full key record matches the stored one exactly. Quorum bundles give every org member each holder's key record, and for passkeys without a signature counter (most synced passkeys) that record never changes. So an org member can replay it in an attestation-"none" registration, become a co-owner of the holder's credential, and pass sole_owner on the holder's release approval page. Hardware keys have the opposite problem: every login rewrites the record (db.rs:948), so a legitimate second registration never matches. An attestation-"none" registration never proves the client holds the private key, so comparing public data can't fix this. Either require an assertion signed by the existing credential before adding it to another account, or restore the global UNIQUE(credential_id).

Should fix

Login breaks between make migrate and the gateway restart, and on rollback: src/api/migrations/057_session_user_id.sql:32
057 makes auth_sessions.user_id NOT NULL, but the gateway on main inserts sessions without that column. Migrations run as a separate make migrate step (Makefile:486). Until the new gateway is live, or after rolling it back, every login, registration and QR login fails with a 500. Add the column as nullable now and make it NOT NULL in a later release.
Username-less web login rejects any credential with two owners: src/gateway/src/handlers/webauthn/login.rs:826
Agreed that QR is fine, since it always requires a username. But web login still runs unscoped when the username field is empty (frontend/src/composables/useWebAuthn.js:142). That path throws away the assertion's user handle and requires exactly one owner. Pick the account by user handle (users.fido2_user_handle), so a shared or squatted credential can't lock out its owner.
Add-passkey has no excludeCredentials and no same-account 409: src/gateway/src/handlers/credentials/passkey.rs:339 (:430)
As you noted, re-adding a key that's already on the account creates a second credential, or overwrites the resident one and leaves a dead row. An exact same-account duplicate hits the unique index and returns a 500. main passed the user's own credentials as excludeCredentials and returned 409, so restore both, scoped to (user_id, credential_id).

Nits

057 backfill comment is wrong, and migrated DBs miss the users FK: src/api/migrations/057_session_user_id.sql:19 (:12, :26)
The comment says multi-owner sessions are deleted, but UPDATE … FROM assigns one of the owners. That's harmless on the first run, because no shared credentials exist yet. Migrated DBs also lack the REFERENCES users(id) that fresh installs get from 001. Deleting a user still cascades through credentials, but the two schemas differ.
One owner query per credential in the release relay: src/gateway/src/release_relay.rs:174
That's up to 64 queries in a row inside a time-limited window. A single credential_id = ANY($1) query does the same job.
Owner-resolution block copy-pasted 4 times, two copies partly dead: login.rs:761,846, qr_auth.rs:863,932
The Unscoped branches in the two QR copies can never run, because QR requires a username (qr_auth.rs:556). A single resolve_owner(&owners, &scope) -> Option<Uuid> helper would replace all four.
Redundant clippy allows: src/gateway/src/main.rs:7
gateway and api already allow redundant_field_names at the crate root, so the 19 per-module copies do nothing.

#	Issue	Severity	Impact	Fix effort
1	Forged-registration check can be bypassed / never matches real keys	High	Org member co-owns a holder's credential; feature unusable for hardware keys	Medium (design)
2	057 NOT NULL vs old gateway	Medium	Login outage during deploy, rollback blocked	Low
3	Username-less web login ignores user handle	Medium	Shared or squatted credential locks out its owner	Low
4	Add-passkey exclusion and 409 removed	Medium	Duplicate or dead credentials, 500s	Low
5	057 comment and FK drift	Low	Misleading comment, schemas differ	Trivial
6	Per-credential owner queries	Low	Latency	Trivial
7	Resolution block duplicated 4×	Low	Copies drift apart, dead QR branches	Low
8	Redundant clippy allows	Low	Noise	Trivial

Open questions

Which flow legitimately puts one credential ID on two accounts? Every navigator.credentials.create() creates a new credential ID, even on the same physical key, so registering one key to a second account already works on main. If no such flow exists, restoring UNIQUE(credential_id) removes #1, #3 and most of the ownership logic in this branch.
If multi-account stays (#1): A: require a signed assertion from the existing credential before adding it to another account. B: restore UNIQUE(credential_id).
How should 057 roll out (#2)? A: two steps, nullable now and NOT NULL next release. B: accept that rollback across this migration isn't possible.

**Review: ryansquared/multi-user-per-credential (@ 2e56f17e)** Fixed since last review: API session lookup, 001 index, decoy panic, and session cascade on passkey delete. ### Blocking 1. **An org member can get past the forged-registration check**: `src/gateway/src/db.rs:634` (`release_relay.rs:43`, `src/api/src/cryptographic_bundles/holders.rs:137`) The check accepts a reused credential ID if the new registration's full key record matches the stored one exactly. Quorum bundles give every org member each holder's key record, and for passkeys without a signature counter (most synced passkeys) that record never changes. So an org member can replay it in an attestation-"none" registration, become a co-owner of the holder's credential, and pass `sole_owner` on the holder's release approval page. Hardware keys have the opposite problem: every login rewrites the record (`db.rs:948`), so a legitimate second registration never matches. An attestation-"none" registration never proves the client holds the private key, so comparing public data can't fix this. Either require an assertion signed by the existing credential before adding it to another account, or restore the global `UNIQUE(credential_id)`. ### Should fix 2. **Login breaks between `make migrate` and the gateway restart, and on rollback**: `src/api/migrations/057_session_user_id.sql:32` 057 makes `auth_sessions.user_id` NOT NULL, but the gateway on `main` inserts sessions without that column. Migrations run as a separate `make migrate` step (`Makefile:486`). Until the new gateway is live, or after rolling it back, every login, registration and QR login fails with a 500. Add the column as nullable now and make it NOT NULL in a later release. 3. **Username-less web login rejects any credential with two owners**: `src/gateway/src/handlers/webauthn/login.rs:826` Agreed that QR is fine, since it always requires a username. But web login still runs unscoped when the username field is empty (`frontend/src/composables/useWebAuthn.js:142`). That path throws away the assertion's user handle and requires exactly one owner. Pick the account by user handle (`users.fido2_user_handle`), so a shared or squatted credential can't lock out its owner. 4. **Add-passkey has no `excludeCredentials` and no same-account 409**: `src/gateway/src/handlers/credentials/passkey.rs:339` (`:430`) As you noted, re-adding a key that's already on the account creates a second credential, or overwrites the resident one and leaves a dead row. An exact same-account duplicate hits the unique index and returns a 500. `main` passed the user's own credentials as `excludeCredentials` and returned 409, so restore both, scoped to `(user_id, credential_id)`. ### Nits 5. **057 backfill comment is wrong, and migrated DBs miss the `users` FK**: `src/api/migrations/057_session_user_id.sql:19` (`:12`, `:26`) The comment says multi-owner sessions are deleted, but `UPDATE … FROM` assigns one of the owners. That's harmless on the first run, because no shared credentials exist yet. Migrated DBs also lack the `REFERENCES users(id)` that fresh installs get from 001. Deleting a user still cascades through credentials, but the two schemas differ. 6. **One owner query per credential in the release relay**: `src/gateway/src/release_relay.rs:174` That's up to 64 queries in a row inside a time-limited window. A single `credential_id = ANY($1)` query does the same job. 7. **Owner-resolution block copy-pasted 4 times, two copies partly dead**: `login.rs:761,846`, `qr_auth.rs:863,932` The `Unscoped` branches in the two QR copies can never run, because QR requires a username (`qr_auth.rs:556`). A single `resolve_owner(&owners, &scope) -> Option<Uuid>` helper would replace all four. 8. **Redundant clippy allows**: `src/gateway/src/main.rs:7` gateway and api already allow `redundant_field_names` at the crate root, so the 19 per-module copies do nothing. | # | Issue | Severity | Impact | Fix effort | | --- | ------------------------------------------------------------------- | -------- | ---------------------------------------------------------------------------- | --------------- | | 1 | Forged-registration check can be bypassed / never matches real keys | High | Org member co-owns a holder's credential; feature unusable for hardware keys | Medium (design) | | 2 | 057 NOT NULL vs old gateway | Medium | Login outage during deploy, rollback blocked | Low | | 3 | Username-less web login ignores user handle | Medium | Shared or squatted credential locks out its owner | Low | | 4 | Add-passkey exclusion and 409 removed | Medium | Duplicate or dead credentials, 500s | Low | | 5 | 057 comment and FK drift | Low | Misleading comment, schemas differ | Trivial | | 6 | Per-credential owner queries | Low | Latency | Trivial | | 7 | Resolution block duplicated 4× | Low | Copies drift apart, dead QR branches | Low | | 8 | Redundant clippy allows | Low | Noise | Trivial | **Open questions** - Which flow legitimately puts one credential ID on two accounts? Every `navigator.credentials.create()` creates a new credential ID, even on the same physical key, so registering one key to a second account already works on `main`. If no such flow exists, restoring `UNIQUE(credential_id)` removes #1, #3 and most of the ownership logic in this branch. - If multi-account stays (#1): A: require a signed assertion from the existing credential before adding it to another account. B: restore `UNIQUE(credential_id)`. - How should 057 roll out (#2)? A: two steps, nullable now and NOT NULL next release. B: accept that rollback across this migration isn't possible.