diff --git a/playwright/docker-compose.yml b/playwright/docker-compose.yml index ffedbfd5..09b94658 100644 --- a/playwright/docker-compose.yml +++ b/playwright/docker-compose.yml @@ -24,9 +24,11 @@ services: environment: - ADMIN_TOKEN - DATABASE_URL + - DOMAIN - I_REALLY_WANT_VOLATILE_STORAGE - LOG_LEVEL - LOGIN_RATELIMIT_MAX_BURST + - ROCKET_PORT - SIGNUPS_VERIFY - SMTP_HOST - SMTP_FROM diff --git a/playwright/tests/passkey.spec.ts b/playwright/tests/passkey.spec.ts index 6c0bc3ff..a19a57b6 100644 --- a/playwright/tests/passkey.spec.ts +++ b/playwright/tests/passkey.spec.ts @@ -455,6 +455,58 @@ test.describe('Passkey grant is rejected when SSO_ONLY is on', () => { const body: any = await res.json(); expect(body?.message ?? '').toMatch(/SSO sign-in is required/i); }); + + test('Login page hides "Log in with passkey" button under SSO_ONLY', async ({ page }) => { + // Defends the `.vw-passkey-login` hide rule in + // `src/static/templates/scss/vaultwarden.scss.hbs` (under + // `sso_enabled && sso_only`). Without the hide, the SPA renders + // the affordance and clicking it dead-ends with the server's + // "SSO sign-in is required" response from the assertion-options + // endpoint above — UX dead end. + await utils.cleanLanding(page); + // The button is in the DOM but %vw-hide applies `display: none`, so + // `toBeHidden()` (not `toHaveCount(0)`) is the right check — but pin + // presence first so a future class rename can't make it pass vacuously + // (an absent element also satisfies toBeHidden()). + await expect(page.locator('.vw-passkey-login')).toHaveCount(1); + await expect(page.locator('.vw-passkey-login')).toBeHidden(); + }); + + test('/api/config omits pm-2035-passkey-unlock under SSO_ONLY', async ({ request }) => { + // Server-side gate at `build_feature_states` (mod.rs). The bundled web + // vault's `WebAuthnPrfUnlockService.isPrfUnlockAvailable` short-circuits + // to false when the flag is absent, hiding the lock-screen "Unlock with + // passkey" option client-side. + const configRes = await request.get('/api/config'); + expect(configRes.status()).toBe(200); + const config: any = await configRes.json(); + expect(config.featureStates, 'featureStates must be present in /api/config').toBeTruthy(); + expect(config.featureStates['pm-2035-passkey-unlock']).toBeUndefined(); + }); + + test('/css/vaultwarden.css emits the Add-passkey-button hide selector under SSO_ONLY', async ({ request }) => { + // Defends the SCSS conditional in + // `src/static/templates/scss/vaultwarden.scss.hbs` that emits + // `app-webauthn-login-settings > button[bitbutton]` under + // `(and sso_enabled sso_only)`. Mirrors Bitwarden's upstream + // template gate `*ngIf="hasData && !limitReached && !requireSsoPolicyEnabled"` + // on the "Turn on" / "New passkey" Add button — vaultwarden + // doesn't surface org policies to the client, so we apply the + // same hide via CSS. The credentials list + per-row Remove + // buttons (`button[bitlink]`, deeper in the ``) stay + // rendered so users can revoke legacy credentials. + // + // String check rather than live DOM: the management page + // requires authentication, and a browser flow under SSO_ONLY + // would need the full Keycloak setup, so this pins the rule at the + // CSS layer. Rendered-UI coverage of the hide *under SSO_ONLY* is + // covered separately; the `account-lifecycle-sso` project runs + // SSO_ONLY=false. + const cssRes = await request.get('/css/vaultwarden.css'); + expect(cssRes.status()).toBe(200); + const css = await cssRes.text(); + expect(css).toContain('app-webauthn-login-settings>button[bitbutton]'); + }); }); test.describe('Passkey enrolment is rejected when SSO_ONLY is on', () => { diff --git a/src/api/core/accounts.rs b/src/api/core/accounts.rs index b494d07c..43bdc967 100644 --- a/src/api/core/accounts.rs +++ b/src/api/core/accounts.rs @@ -26,6 +26,7 @@ use crate::{ WebAuthnCredential, WebAuthnCredentialId, }, }, + error::Error, mail, util::{NumberOrString, deser_opt_nonempty_str, format_date}, }; @@ -874,7 +875,7 @@ async fn post_rotatekey(data: Json, headers: Headers, conn: DbConn, nt: // We only rotate the reset password key if it is set. existing_memberships.retain(|m| m.reset_password_key.is_some()); let mut existing_sends = Send::find_by_user(user_id, &conn).await; - let existing_webauthn_credentials = WebAuthnCredential::find_by_user(user_id, &conn).await; + let existing_webauthn_credentials = WebAuthnCredential::find_by_user(user_id, &conn).await?; validate_keydata( &data, @@ -893,7 +894,7 @@ async fn post_rotatekey(data: Json, headers: Headers, conn: DbConn, nt: .map(|c| c.uuid.clone()) .collect::>(); let current_prf_ids = WebAuthnCredential::find_by_user(user_id, &conn) - .await + .await? .into_iter() .filter(WebAuthnCredential::has_prf_keyset) .map(|c| c.uuid) @@ -940,29 +941,6 @@ async fn post_rotatekey(data: Json, headers: Headers, conn: DbConn, nt: membership.save(&conn).await?; } - // Update passkey-login credential keys (the PRF "rotateable key set") so that - // passwordless decryption keeps working after the account key is rotated. - // The client only sends credentials whose PRF keyset is fully enabled. - // - for passkey_data in data.account_unlock_data.passkey_unlock_data { - let Some(mut credential) = WebAuthnCredential::find_by_uuid_and_user(&passkey_data.id, user_id, &conn).await - else { - err!("Passkey doesn't exist") - }; - // Refuse to write rotation data to a credential that isn't in the - // "Enabled" PRF state. Otherwise a credential with only the two - // rotated columns set (and `encrypted_private_key` still NULL) would - // be left as a partial keyset that `prf_status` reports as Supported. - if !credential.has_prf_keyset() { - err!("Passkey is not in a PRF-enabled state") - } - // Mutate only the two columns `update_keys` persists; other fields on - // the loaded credential are not written back. - credential.encrypted_public_key = Some(passkey_data.encrypted_public_key); - credential.encrypted_user_key = Some(passkey_data.encrypted_user_key); - credential.update_keys(&conn).await?; - } - // Update send data for send_data in data.account_data.sends { let Some(send) = existing_sends.iter_mut().find(|s| &s.uuid == send_data.id.as_ref().unwrap()) else { @@ -996,7 +974,7 @@ async fn post_rotatekey(data: Json, headers: Headers, conn: DbConn, nt: // this guard, the new akey commits and the orphan credential can never // unlock the vault again. let post_loop_prf_ids = WebAuthnCredential::find_by_user(user_id, &conn) - .await + .await? .into_iter() .filter(WebAuthnCredential::has_prf_keyset) .map(|c| c.uuid) @@ -1020,12 +998,62 @@ async fn post_rotatekey(data: Json, headers: Headers, conn: DbConn, nt: let save_result = user.save(&conn).await; + // Rewrap the passkey-login PRF blobs ONLY if `user.save` committed the new + // account key. The proper fix is a single transaction over the user-record + // write and every per-credential `update_keys` (the function-head comment + // already calls this out); until that's in place, this ordering is the + // strict improvement over running the loop before `user.save`: + // • `user.save` fails → skip the rewrap entirely. The user's old master + // password still works and the credential blobs still match the + // (uncommitted) old account key. + // • `user.save` commits but a credential rewrap fails mid-loop → the + // user has the new master password, the rewrapped credentials match + // the new akey, and the not-yet-rewrapped credentials are stranded + // (their PRF blobs reference the old akey). Recoverable: log in via + // the new master password and re-enrol the stranded credentials. + // Running the loop before `user.save` had the inverse failure: rewrapped + // credentials referenced an akey that was never committed, so they + // couldn't unlock at all and the user could only log in via the still- + // old master password — strictly worse. + let rewrap_result: EmptyResult = if save_result.is_ok() { + let mut outcome: EmptyResult = Ok(()); + for passkey_data in data.account_unlock_data.passkey_unlock_data { + let Some(mut credential) = + WebAuthnCredential::find_by_uuid_and_user(&passkey_data.id, &user.uuid, &conn).await + else { + outcome = Err(Error::new("Passkey doesn't exist", "Passkey doesn't exist")); + break; + }; + // Refuse to write rotation data to a credential that isn't in the + // "Enabled" PRF state. Otherwise a credential with only the two + // rotated columns set (and `encrypted_private_key` still NULL) + // would be left as a partial keyset that `prf_status` reports as + // Supported. + if !credential.has_prf_keyset() { + outcome = + Err(Error::new("Passkey is not in a PRF-enabled state", "Passkey is not in a PRF-enabled state")); + break; + } + // Mutate only the two columns `update_keys` persists; other + // fields on the loaded credential are not written back. + credential.encrypted_public_key = Some(passkey_data.encrypted_public_key); + credential.encrypted_user_key = Some(passkey_data.encrypted_user_key); + if let Err(e) = credential.update_keys(&conn).await { + outcome = Err(e); + break; + } + } + outcome + } else { + Ok(()) + }; + // Prevent logging out the client where the user requested this endpoint from. // If you do logout the user it will causes issues at the client side. // Adding the device uuid will prevent this. nt.send_logout(&user, Some(&headers.device), &conn).await; - save_result + save_result.and(rewrap_result) } #[post("/accounts/security-stamp", data = "")] diff --git a/src/api/core/ciphers.rs b/src/api/core/ciphers.rs index 5a36dffc..b38cbc56 100644 --- a/src/api/core/ciphers.rs +++ b/src/api/core/ciphers.rs @@ -191,8 +191,13 @@ async fn sync(data: SyncData, headers: Headers, client_version: Option Vec { let mut eq_domains_routes = routes![get_settings_domains, post_settings_domains, put_settings_domains]; let mut hibp_routes = routes![hibp_breach]; @@ -207,7 +210,7 @@ fn alive(_conn: DbConn) -> Json { #[get("/now")] pub fn now() -> Json { - Json(crate::util::format_date(&chrono::Utc::now().naive_utc())) + Json(crate::util::format_date(&Utc::now().naive_utc())) } #[get("/version")] @@ -216,11 +219,11 @@ fn version() -> Json<&'static str> { } #[get("/webauthn")] -async fn get_api_webauthn(headers: Headers, conn: DbConn) -> Json { +async fn get_api_webauthn(headers: Headers, conn: DbConn) -> JsonResult { let user = headers.user; let data: Vec = WebAuthnCredential::find_by_user(&user.uuid, &conn) - .await + .await? .into_iter() .map(|wac| { json!({ @@ -235,17 +238,19 @@ async fn get_api_webauthn(headers: Headers, conn: DbConn) -> Json { }) .collect(); - Json(json!({ + Ok(Json(json!({ "object": "list", "data": data, "continuationToken": null - })) + }))) } #[derive(Debug, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] struct WebAuthnPasskeyRegistrationChallenge { token: String, + created_at: i64, + user_security_stamp: String, state: PasskeyRegistration, } @@ -253,10 +258,26 @@ struct WebAuthnPasskeyRegistrationChallenge { #[serde(rename_all = "camelCase")] struct WebAuthnPasskeyAssertionChallenge { token: String, + created_at: i64, + user_security_stamp: String, state: PasskeyAuthentication, } -fn passkey_registration_challenge_state(data: &str, token: Option<&str>) -> ApiResult { +fn passkey_management_challenge_is_fresh(created_at: i64) -> bool { + // Lower-bound-only: `created_at` is server-stamped at challenge issuance + // and cannot legitimately be in the future. An upper bound here would only + // matter under a backward clock-jump (NTP correction), and in that case + // it would *extend* the validity window past the documented TTL — strictly + // worse than the one-sided check used by `WebAuthnLoginChallenge::take`. + let cutoff = Utc::now().timestamp() - WEBAUTHN_PASSKEY_CHALLENGE_TTL_SECONDS; + created_at >= cutoff +} + +fn passkey_registration_challenge_state( + data: &str, + token: Option<&str>, + user_security_stamp: &str, +) -> ApiResult { // Persisted challenge rows are always the `{token, state}` wrapper — // nothing in the current code path writes the bare `PasskeyRegistration` // shape. Reject a row that doesn't deserialise (corrupted, stale schema) @@ -268,10 +289,20 @@ fn passkey_registration_challenge_state(data: &str, token: Option<&str>) -> ApiR if !token.is_some_and(|t| crypto::ct_eq(t, &saved.token)) { err!("Invalid registration challenge. Please try again.") } + if !passkey_management_challenge_is_fresh(saved.created_at) { + err!("Invalid registration challenge. Please try again.") + } + if !crypto::ct_eq(user_security_stamp, &saved.user_security_stamp) { + err!("Invalid registration challenge. Please try again.") + } Ok(saved.state) } -fn passkey_assertion_challenge_state(data: &str, token: &str) -> ApiResult { +fn passkey_assertion_challenge_state( + data: &str, + token: &str, + user_security_stamp: &str, +) -> ApiResult { // Same shape contract as `passkey_registration_challenge_state` above — // reject undecodable rows with the generic message rather than leaking // the underlying serde error. @@ -281,6 +312,12 @@ fn passkey_assertion_challenge_state(data: &str, token: &str) -> ApiResult = all_creds .into_iter() .filter_map(|wac| { @@ -342,20 +379,16 @@ async fn post_api_webauthn_attestation_options( asc.resident_key = Some(webauthn_rs_proto::ResidentKeyRequirement::Required); } - // Drop any abandoned challenge from a previous, unfinished registration - // attempt so these rows cannot accumulate. `TwoFactor::save` below uses - // `replace_into` keyed on `uuid` (not on `(user_uuid, atype)`), so without - // this delete, sqlite/mysql would insert a sibling row each retry. - if let Some(tf) = - TwoFactor::find_by_user_and_type(&user.uuid, TwoFactorType::WebauthnPasskeyRegisterChallenge as i32, &conn) - .await - { - tf.delete(&conn).await?; - } + // Atomically drop any abandoned challenge from a previous, unfinished + // registration attempt so only one in-flight challenge state per user + // exists at any time. + TwoFactor::take_by_user_and_type(&user.uuid, TwoFactorType::WebauthnPasskeyRegisterChallenge as i32, &conn).await?; let token = get_uuid(); let saved_challenge = WebAuthnPasskeyRegistrationChallenge { token: token.clone(), + created_at: Utc::now().timestamp(), + user_security_stamp: user.security_stamp, state, }; @@ -401,7 +434,7 @@ async fn post_api_webauthn_assertion_options( data.validate(&user, true, &conn).await?; let credentials: Vec = WebAuthnCredential::find_by_user(&user.uuid, &conn) - .await + .await? .into_iter() .filter(|wac| wac.supports_prf) .filter_map(|wac| serde_json::from_str(&wac.credential).ok()) @@ -413,16 +446,16 @@ async fn post_api_webauthn_assertion_options( let (response, state) = WEBAUTHN.start_passkey_authentication(&credentials)?; - if let Some(tf) = - TwoFactor::find_by_user_and_type(&user.uuid, TwoFactorType::WebauthnPasskeyAssertionChallenge as i32, &conn) - .await - { - tf.delete(&conn).await?; - } + // Atomically drop any abandoned challenge from a previous attempt — see + // the comment on `post_api_webauthn_attestation_options`. + TwoFactor::take_by_user_and_type(&user.uuid, TwoFactorType::WebauthnPasskeyAssertionChallenge as i32, &conn) + .await?; let token = get_uuid(); let saved_challenge = WebAuthnPasskeyAssertionChallenge { token: token.clone(), + created_at: Utc::now().timestamp(), + user_security_stamp: user.security_stamp, state, }; TwoFactor::new( @@ -457,12 +490,20 @@ async fn post_api_webauthn( data: Json, headers: Headers, conn: DbConn, + nt: Notify<'_>, ) -> ApiResult { crate::ratelimit::check_limit_login(&headers.ip.ip)?; let data: WebAuthnLoginCredentialCreateRequest = data.into_inner(); let user = headers.user; + // Same gate the `*-options` endpoints use (lines 333, 423). Without it, a + // misconfigured `DOMAIN` that passes startup's `http://` prefix check but + // fails `Url::parse` panics on the `WEBAUTHN` `LazyLock` initializer below. + if !CONFIG.is_webauthn_2fa_supported() { + err!("Webauthn is not supported on this server. Set `DOMAIN` to a valid URL with a parseable host.") + } + if CONFIG.sso_enabled() && CONFIG.sso_only() { err!("Passkeys cannot be created when SSO sign-in is required") } @@ -471,20 +512,27 @@ async fn post_api_webauthn( // finishes for the same registration row cannot both succeed and create // duplicate `web_authn_credentials` entries — only the caller whose DELETE // removes the row proceeds. + let Some(mut current_user) = User::find_by_uuid(&user.uuid, &conn).await else { + err!("User not found") + }; + let type_ = TwoFactorType::WebauthnPasskeyRegisterChallenge as i32; - let Some(tf) = TwoFactor::take_by_user_and_type(&user.uuid, type_, &conn).await else { + let Some(tf) = TwoFactor::take_by_user_and_type(&user.uuid, type_, &conn).await? else { err!("No registration challenge found. Please try again.") }; - let state = passkey_registration_challenge_state(&tf.data, data.token.as_deref())?; + let state = passkey_registration_challenge_state(&tf.data, data.token.as_deref(), ¤t_user.security_stamp)?; let credential = WEBAUTHN.finish_passkey_registration(&data.device_response.into(), &state)?; let credential_id_hash = passkey_credential_id_hash(&credential); - if WebAuthnCredential::credential_id_hash_exists(&credential_id_hash, &conn).await { - err!("Passkey is already registered") - } + // Duplicate detection is enforced by the `web_authn_credentials` table's + // unique index on `credential_id_hash`: `WebAuthnCredential::save` below + // maps the resulting `UniqueViolation` to "Passkey is already registered", + // so an explicit pre-check would just double the queries on the happy + // path and (because it collapses transient read errors to `false`) is + // not even a reliable guard. WebAuthnCredential::new( - user.uuid, + current_user.uuid.clone(), data.name, serde_json::to_string(&credential)?, credential_id_hash, @@ -496,6 +544,9 @@ async fn post_api_webauthn( .save(&conn) .await?; + current_user.update_revision(&conn).await?; + nt.send_user_update(UpdateType::SyncVault, ¤t_user, headers.device.push_uuid.as_ref(), &conn).await; + Ok(Status::Ok) } @@ -514,12 +565,20 @@ async fn put_api_webauthn( data: Json, headers: Headers, conn: DbConn, + nt: Notify<'_>, ) -> ApiResult { crate::ratelimit::check_limit_login(&headers.ip.ip)?; let data: WebAuthnLoginCredentialUpdateRequest = data.into_inner(); let user = headers.user; + // Same gate the `*-options` endpoints use (lines 333, 423). Without it, a + // misconfigured `DOMAIN` that passes startup's `http://` prefix check but + // fails `Url::parse` panics on the `WEBAUTHN` `LazyLock` initializer below. + if !CONFIG.is_webauthn_2fa_supported() { + err!("Webauthn is not supported on this server. Set `DOMAIN` to a valid URL with a parseable host.") + } + if CONFIG.sso_enabled() && CONFIG.sso_only() { err!("Passkeys cannot be updated when SSO sign-in is required") } @@ -538,16 +597,20 @@ async fn put_api_webauthn( // updates for the same assertion row cannot both succeed and apply // different blob payloads — only the caller whose DELETE removes the row // proceeds. + let Some(mut current_user) = User::find_by_uuid(&user.uuid, &conn).await else { + err!("User not found") + }; + let type_ = TwoFactorType::WebauthnPasskeyAssertionChallenge as i32; - let Some(tf) = TwoFactor::take_by_user_and_type(&user.uuid, type_, &conn).await else { + let Some(tf) = TwoFactor::take_by_user_and_type(&user.uuid, type_, &conn).await? else { err!("No assertion challenge found. Please try again.") }; - let state = passkey_assertion_challenge_state(&tf.data, &data.token)?; + let state = passkey_assertion_challenge_state(&tf.data, &data.token, ¤t_user.security_stamp)?; let credential_response = data.device_response.into(); let mut parsed_credentials: Vec<(WebAuthnCredential, Passkey)> = - WebAuthnCredential::find_by_user(&user.uuid, &conn) - .await + WebAuthnCredential::find_by_user(¤t_user.uuid, &conn) + .await? .into_iter() .filter_map(|wac| { let passkey: Passkey = serde_json::from_str(&wac.credential).ok()?; @@ -581,6 +644,9 @@ async fn put_api_webauthn( matched_wac.encrypted_private_key = Some(encrypted_private_key); matched_wac.update_prf_keyset(&conn).await?; + current_user.update_revision(&conn).await?; + nt.send_user_update(UpdateType::SyncVault, ¤t_user, headers.device.push_uuid.as_ref(), &conn).await; + Ok(Status::Ok) } @@ -597,16 +663,20 @@ async fn post_api_webauthn_delete( uuid: WebAuthnCredentialId, headers: Headers, conn: DbConn, + nt: Notify<'_>, ) -> ApiResult { crate::ratelimit::check_limit_login(&headers.ip.ip)?; let data: PasswordOrOtpData = data.into_inner(); - let user = headers.user; + let mut user = headers.user; data.validate(&user, true, &conn).await?; WebAuthnCredential::delete_by_uuid_and_user(&uuid, &user.uuid, &conn).await?; + user.update_revision(&conn).await?; + nt.send_user_update(UpdateType::SyncVault, &user, headers.device.push_uuid.as_ref(), &conn).await; + Ok(Status::Ok) } @@ -659,23 +729,53 @@ mod tests { fn registration_challenge_accepts_wrapped_state_with_matching_token() { let saved = WebAuthnPasskeyRegistrationChallenge { token: String::from("token"), + created_at: Utc::now().timestamp(), + user_security_stamp: String::from("stamp"), state: registration_state(), }; let data = serde_json::to_string(&saved).unwrap(); - assert!(passkey_registration_challenge_state(&data, Some("token")).is_ok()); + assert!(passkey_registration_challenge_state(&data, Some("token"), "stamp").is_ok()); } #[test] fn registration_challenge_rejects_wrapped_state_without_matching_token() { let saved = WebAuthnPasskeyRegistrationChallenge { token: String::from("token"), + created_at: Utc::now().timestamp(), + user_security_stamp: String::from("stamp"), state: registration_state(), }; let data = serde_json::to_string(&saved).unwrap(); - assert!(passkey_registration_challenge_state(&data, Some("wrong")).is_err()); - assert!(passkey_registration_challenge_state(&data, None).is_err()); + assert!(passkey_registration_challenge_state(&data, Some("wrong"), "stamp").is_err()); + assert!(passkey_registration_challenge_state(&data, None, "stamp").is_err()); + } + + #[test] + fn registration_challenge_rejects_expired_state() { + let saved = WebAuthnPasskeyRegistrationChallenge { + token: String::from("token"), + created_at: Utc::now().timestamp() - WEBAUTHN_PASSKEY_CHALLENGE_TTL_SECONDS - 1, + user_security_stamp: String::from("stamp"), + state: registration_state(), + }; + let data = serde_json::to_string(&saved).unwrap(); + + assert!(passkey_registration_challenge_state(&data, Some("token"), "stamp").is_err()); + } + + #[test] + fn registration_challenge_rejects_stale_account_revision() { + let saved = WebAuthnPasskeyRegistrationChallenge { + token: String::from("token"), + created_at: Utc::now().timestamp(), + user_security_stamp: String::from("old-stamp"), + state: registration_state(), + }; + let data = serde_json::to_string(&saved).unwrap(); + + assert!(passkey_registration_challenge_state(&data, Some("token"), "new-stamp").is_err()); } /// `passkey_registration_challenge_state` has no legacy unwrapped fallback — @@ -689,9 +789,9 @@ mod tests { fn registration_challenge_rejects_unwrapped_legacy_state() { let data = serde_json::to_string(®istration_state()).unwrap(); - assert!(passkey_registration_challenge_state(&data, None).is_err()); - assert!(passkey_registration_challenge_state(&data, Some("any-token")).is_err()); - assert!(passkey_registration_challenge_state(&data, Some("")).is_err()); + assert!(passkey_registration_challenge_state(&data, None, "stamp").is_err()); + assert!(passkey_registration_challenge_state(&data, Some("any-token"), "stamp").is_err()); + assert!(passkey_registration_challenge_state(&data, Some(""), "stamp").is_err()); } #[test] @@ -699,12 +799,42 @@ mod tests { let (_response, state) = webauthn().start_passkey_authentication(&[passkey()]).unwrap(); let saved = WebAuthnPasskeyAssertionChallenge { token: String::from("token"), + created_at: Utc::now().timestamp(), + user_security_stamp: String::from("stamp"), state, }; let data = serde_json::to_string(&saved).unwrap(); - assert!(passkey_assertion_challenge_state(&data, "token").is_ok()); - assert!(passkey_assertion_challenge_state(&data, "wrong").is_err()); + assert!(passkey_assertion_challenge_state(&data, "token", "stamp").is_ok()); + assert!(passkey_assertion_challenge_state(&data, "wrong", "stamp").is_err()); + } + + #[test] + fn assertion_challenge_rejects_expired_state() { + let (_response, state) = webauthn().start_passkey_authentication(&[passkey()]).unwrap(); + let saved = WebAuthnPasskeyAssertionChallenge { + token: String::from("token"), + created_at: Utc::now().timestamp() - WEBAUTHN_PASSKEY_CHALLENGE_TTL_SECONDS - 1, + user_security_stamp: String::from("stamp"), + state, + }; + let data = serde_json::to_string(&saved).unwrap(); + + assert!(passkey_assertion_challenge_state(&data, "token", "stamp").is_err()); + } + + #[test] + fn assertion_challenge_rejects_stale_account_revision() { + let (_response, state) = webauthn().start_passkey_authentication(&[passkey()]).unwrap(); + let saved = WebAuthnPasskeyAssertionChallenge { + token: String::from("token"), + created_at: Utc::now().timestamp(), + user_security_stamp: String::from("old-stamp"), + state, + }; + let data = serde_json::to_string(&saved).unwrap(); + + assert!(passkey_assertion_challenge_state(&data, "token", "new-stamp").is_err()); } #[test] @@ -766,19 +896,22 @@ mod tests { let (_response, state) = webauthn().start_passkey_authentication(&[passkey()]).unwrap(); let bare = serde_json::to_string(&state).unwrap(); - assert!(passkey_assertion_challenge_state(&bare, "any-token").is_err()); - assert!(passkey_assertion_challenge_state(&bare, "").is_err()); + assert!(passkey_assertion_challenge_state(&bare, "any-token", "stamp").is_err()); + assert!(passkey_assertion_challenge_state(&bare, "", "stamp").is_err()); } - /// `build_feature_states` must emit `pm-2035-passkey-unlock = true` - /// unconditionally — without it, the web vault's - /// `WebAuthnPrfUnlockService.isPrfUnlockAvailable` short-circuits to false - /// and the lock-screen "Unlock with passkey" option never renders even for - /// a user with a PRF-enabled passkey enrolled. + /// `build_feature_states` must emit `pm-2035-passkey-unlock = true` when + /// account passkeys are allowed; without it, the web vault's + /// `WebAuthnPrfUnlockService.isPrfUnlockAvailable` short-circuits to false. #[test] - fn feature_states_emits_passkey_unlock_flag_unconditionally() { - assert_eq!(build_feature_states("").get("pm-2035-passkey-unlock"), Some(&true)); - assert_eq!(build_feature_states("some-unrelated-flag").get("pm-2035-passkey-unlock"), Some(&true)); + fn feature_states_emits_passkey_unlock_flag_when_allowed() { + assert_eq!(build_feature_states("", true).get("pm-2035-passkey-unlock"), Some(&true)); + assert_eq!(build_feature_states("some-unrelated-flag", true).get("pm-2035-passkey-unlock"), Some(&true)); + } + + #[test] + fn feature_states_omits_passkey_unlock_flag_when_disallowed() { + assert!(!build_feature_states("", false).contains_key("pm-2035-passkey-unlock")); } /// `build_feature_states` must also emit `pm-19148-innovation-archive` @@ -786,7 +919,8 @@ mod tests { /// alongside the passkey-unlock entry. #[test] fn feature_states_emits_innovation_archive_flag_unconditionally() { - assert_eq!(build_feature_states("").get("pm-19148-innovation-archive"), Some(&true)); + assert_eq!(build_feature_states("", true).get("pm-19148-innovation-archive"), Some(&true)); + assert_eq!(build_feature_states("", false).get("pm-19148-innovation-archive"), Some(&true)); } /// Valid experimental flags from the SUPPORTED list pass through; invalid @@ -795,13 +929,13 @@ mod tests { #[test] fn feature_states_passes_through_valid_experimental_flag() { let probe = crate::config::SUPPORTED_FEATURE_FLAGS.iter().next().expect("at least one supported flag"); - let states = build_feature_states(probe); + let states = build_feature_states(probe, true); assert_eq!(states.get(*probe), Some(&true)); } #[test] fn feature_states_drops_unknown_experimental_flag() { - let states = build_feature_states("definitely-not-a-real-bitwarden-flag"); + let states = build_feature_states("definitely-not-a-real-bitwarden-flag", true); assert!(!states.contains_key("definitely-not-a-real-bitwarden-flag")); } } @@ -815,24 +949,29 @@ mod tests { /// Client (v2026.2.1): https://github.com/bitwarden/clients/blob/f96380c3138291a028bdd2c7a5fee540d5c98ba5/libs/common/src/enums/feature-flag.enum.ts#L12 /// Android (v2026.2.1): https://github.com/bitwarden/android/blob/6902c19c0093fa476bbf74ccaa70c9f14afbb82f/core/src/main/kotlin/com/bitwarden/core/data/manager/model/FlagKey.kt#L31 /// iOS (v2026.2.1): https://github.com/bitwarden/ios/blob/cdd9ba1770ca2ffc098d02d12cc3208e3a830454/BitwardenShared/Core/Platform/Models/Enum/FeatureFlag.swift#L7 -fn build_feature_states(experimental_client_feature_flags: &str) -> std::collections::HashMap { +fn build_feature_states( + experimental_client_feature_flags: &str, + account_passkeys_allowed: bool, +) -> std::collections::HashMap { let mut feature_states = parse_experimental_client_feature_flags(experimental_client_feature_flags, &FeatureFlagFilter::ValidOnly); feature_states.insert("pm-19148-innovation-archive".to_owned(), true); // Gates the web-vault's `Unlock with passkey` lock-screen option (and the // matching desktop/mobile UI). `WebAuthnPrfUnlockService.isPrfUnlockAvailable` - // short-circuits to `false` when this flag is absent or unset, hiding the - // option even for users with a PRF-enabled passkey enrolled. Vaultwarden - // supports PRF-passkey unlock end-to-end via `userDecryption.webAuthnPrfOptions` - // on /sync, so the flag is advertised unconditionally. - feature_states.insert("pm-2035-passkey-unlock".to_owned(), true); + // short-circuits to `false` when this flag is absent or unset. Vaultwarden + // advertises it whenever account passkeys are allowed; SSO_ONLY suppresses + // it to match Bitwarden's "Require SSO" passkey restriction. + if account_passkeys_allowed { + feature_states.insert("pm-2035-passkey-unlock".to_owned(), true); + } feature_states } #[get("/config")] fn config() -> Json { let domain = CONFIG.domain(); - let feature_states = build_feature_states(&CONFIG.experimental_client_feature_flags()); + let feature_states = + build_feature_states(&CONFIG.experimental_client_feature_flags(), !(CONFIG.sso_enabled() && CONFIG.sso_only())); Json(json!({ // Note: The clients use this version to handle backwards compatibility concerns diff --git a/src/api/identity.rs b/src/api/identity.rs index 0cbb16b1..191fffff 100644 --- a/src/api/identity.rs +++ b/src/api/identity.rs @@ -8,12 +8,10 @@ use rocket::{ serde::json::Json, }; use serde_json::Value; -use webauthn_rs::prelude::{Base64UrlSafeData, DiscoverableAuthentication, DiscoverableKey, Passkey}; -use webauthn_rs_proto::{ - AuthenticationExtensionsClientOutputs, AuthenticatorAssertionResponseRaw, PublicKeyCredential, -}; +use webauthn_rs::prelude::{DiscoverableAuthentication, DiscoverableKey, Passkey}; +use webauthn_rs_proto::PublicKeyCredential; -use crate::api::core::two_factor::webauthn::WEBAUTHN; +use crate::api::core::two_factor::webauthn::{PublicKeyCredentialCopy, WEBAUTHN}; use crate::{ CONFIG, api::{ @@ -1119,45 +1117,6 @@ async fn register_finish(data: Json, conn: DbConn) -> JsonResult { register(data, true, conn).await } -// Copied from webauthn-rs to rename clientDataJSON -> clientDataJson for Bitwarden compatibility -#[derive(Debug, Deserialize)] -#[serde(rename_all = "camelCase")] -struct AssertionResponseCopy { - pub authenticator_data: Base64UrlSafeData, - #[serde(rename = "clientDataJson", alias = "clientDataJSON")] - pub client_data_json: Base64UrlSafeData, - pub signature: Base64UrlSafeData, - pub user_handle: Option, -} - -#[derive(Debug, Deserialize)] -#[serde(rename_all = "camelCase")] -struct PublicKeyCredentialCopy { - pub id: String, - pub raw_id: Base64UrlSafeData, - pub response: AssertionResponseCopy, - pub r#type: String, - #[allow(dead_code)] - pub extensions: Option, -} - -impl From for PublicKeyCredential { - fn from(p: PublicKeyCredentialCopy) -> Self { - Self { - id: p.id, - raw_id: p.raw_id, - response: AuthenticatorAssertionResponseRaw { - authenticator_data: p.response.authenticator_data, - client_data_json: p.response.client_data_json, - signature: p.response.signature, - user_handle: p.response.user_handle, - }, - extensions: AuthenticationExtensionsClientOutputs::default(), - type_: p.r#type, - } - } -} - fn passkey_credential_id(passkey: &Passkey) -> ApiResult { serde_json::to_value(passkey.cred_id())? .as_str() @@ -1294,7 +1253,7 @@ async fn webauthn_login(data: ConnectData, user_id: &mut Option, conn: & // or which user the assertion later claims — a caller with a valid token // cannot repeatedly replay it with malformed bodies. let token = WebAuthnLoginChallengeId::from(data.token.as_ref().unwrap().clone()); - let Some(saved_challenge) = WebAuthnLoginChallenge::take(&token, conn).await else { + let Some(saved_challenge) = WebAuthnLoginChallenge::take(&token, conn).await? else { err!( AUTH_FAILED, format!("IP: {}. Missing or expired passkey login challenge.", ip.ip), @@ -1353,15 +1312,28 @@ async fn webauthn_login(data: ConnectData, user_id: &mut Option, conn: & let username = user.email.clone(); - // Load this user's passkey-login credentials. - let parsed_credentials: Vec<(WebAuthnCredential, Passkey)> = WebAuthnCredential::find_by_user(&user.uuid, conn) - .await - .into_iter() - .filter_map(|wac| { - let passkey: Passkey = serde_json::from_str(&wac.credential).ok()?; - Some((wac, passkey)) - }) - .collect(); + // Load this user's passkey-login credentials. A DB transient (deadlock- + // victim, conn drop, lock timeout) is converted to AUTH_FAILED here rather + // than propagating: this endpoint is unauthenticated, so a panic or a + // distinct DB-error response would let an attacker amplify DB pressure + // into worker DoS or fingerprint server state. + let parsed_credentials: Vec<(WebAuthnCredential, Passkey)> = + match WebAuthnCredential::find_by_user(&user.uuid, conn).await { + Ok(creds) => creds + .into_iter() + .filter_map(|wac| { + let passkey: Passkey = serde_json::from_str(&wac.credential).ok()?; + Some((wac, passkey)) + }) + .collect(), + Err(e) => err!( + AUTH_FAILED, + format!("IP: {}. Username: {username}. DB error loading passkey credentials: {e:#?}", ip.ip), + ErrorEvent { + event: EventType::UserFailedLogIn + } + ), + }; if parsed_credentials.is_empty() { err!( @@ -1436,6 +1408,11 @@ async fn webauthn_login(data: ConnectData, user_id: &mut Option, conn: & }; // Persist any signature-counter advance from this assertion. + // + // Exempt from the `update_revision` + `send_user_update(SyncVault, ...)` + // pair the four management endpoints in `src/api/core/mod.rs` emit after + // credential mutations: the signature counter is not part of any sync + // payload the clients read, so a notify here would be a no-op. if passkey.update_credential(&authentication_result) == Some(true) { matched_wac.credential = serde_json::to_string(&passkey)?; matched_wac.update_credential(conn).await?; diff --git a/src/db/models/two_factor.rs b/src/db/models/two_factor.rs index 1d8f311a..33acc3bf 100644 --- a/src/db/models/two_factor.rs +++ b/src/db/models/two_factor.rs @@ -7,7 +7,7 @@ use webauthn_rs_proto::{AttestationFormat, RegisteredExtensions}; use crate::{ api::{EmptyResult, core::two_factor::webauthn::WebauthnRegistration}, db::{DbConn, schema::twofactor}, - error::MapResult, + error::{Error, MapResult}, }; use super::UserId; @@ -150,16 +150,21 @@ impl TwoFactor { .await } - /// Atomically fetch and delete the row for this user+type. Returns Some - /// only when the caller's DELETE actually removed a row, so two concurrent - /// callers (e.g. a double-clicked enrollment finish) cannot both proceed - /// with the same single-use challenge state — the loser sees None. The - /// surrounding transaction rolls back the SELECT+DELETE pair atomically - /// on a DB error, leaving the row intact rather than silently consuming it. - pub async fn take_by_user_and_type(user_uuid: &UserId, atype: i32, conn: &DbConn) -> Option { + /// Atomically fetch and delete the row for this user+type. Three outcomes: + /// - `Ok(Some(_))` — winner of the SELECT+DELETE race; the row has been + /// removed and the caller may consume the single-use challenge state. + /// - `Ok(None)` — token absent, already consumed by a concurrent caller + /// (e.g. a double-clicked enrolment finish), or never existed. The + /// caller should treat this as a normal "stale challenge" response. + /// - `Err(_)` — DB degradation (deadlock, conn drop, lock timeout). The + /// surrounding transaction rolled back atomically, so the row is + /// intact rather than silently consumed; propagating via `?` lets the + /// caller surface a 5xx instead of an indistinguishable 4xx stale-token + /// response. + pub async fn take_by_user_and_type(user_uuid: &UserId, atype: i32, conn: &DbConn) -> Result, Error> { let user_uuid = user_uuid.clone(); conn.run(move |conn| { - let result = conn.transaction::, diesel::result::Error, _>(|conn| { + conn.transaction::, diesel::result::Error, _>(|conn| { let tf = twofactor::table .filter(twofactor::user_uuid.eq(&user_uuid)) .filter(twofactor::atype.eq(atype)) @@ -171,18 +176,8 @@ impl TwoFactor { let deleted = diesel::delete(twofactor::table.filter(twofactor::uuid.eq(&existing.uuid))).execute(conn)?; Ok(tf.filter(|_| deleted == 1)) - }); - match result { - Ok(opt) => opt, - Err(e) => { - // Surface the underlying error so DB degradation - // (deadlock, conn drop, lock timeout) is operator- - // visible rather than indistinguishable from a normal - // "row consumed by a concurrent caller" result. - error!("TwoFactor::take_by_user_and_type failed for user {user_uuid} atype {atype}: {e:#?}"); - None - } - } + }) + .map_res("Error taking twofactor challenge") }) .await } diff --git a/src/db/models/web_authn_credential.rs b/src/db/models/web_authn_credential.rs index 9f8302cb..726542c0 100644 --- a/src/db/models/web_authn_credential.rs +++ b/src/db/models/web_authn_credential.rs @@ -6,7 +6,7 @@ use macros::UuidFromParam; use crate::api::EmptyResult; use crate::db::schema::{web_authn_credentials, web_authn_login_challenges}; use crate::db::{DbConn, DbPool}; -use crate::error::MapResult; +use crate::error::{Error, MapResult}; use crate::util::get_uuid; use super::UserId; @@ -84,7 +84,7 @@ impl WebAuthnCredential { match result { Err(diesel::result::Error::DatabaseError(diesel::result::DatabaseErrorKind::UniqueViolation, _)) => { - Err(crate::error::Error::new( + Err(Error::new( "Passkey is already registered", "Duplicate WebAuthn credential ID", )) @@ -94,17 +94,6 @@ impl WebAuthnCredential { }} } - pub async fn credential_id_hash_exists(credential_id_hash: &str, conn: &DbConn) -> bool { - db_run! { conn: { - web_authn_credentials::table - .filter(web_authn_credentials::credential_id_hash.eq(credential_id_hash)) - .select(web_authn_credentials::credential_id_hash) - .first::(conn) - .optional() - .is_ok_and(|credential| credential.is_some()) - }} - } - /// Persist the serialized passkey blob after a successful assertion advances /// its signature counter. Touches only the `credential` column so a concurrent /// key rotation cannot clobber it (see [`Self::update_keys`]). @@ -148,12 +137,17 @@ impl WebAuthnCredential { }} } - pub async fn find_by_user(user_uuid: &UserId, conn: &DbConn) -> Vec { + /// Surface DB errors so callers can convert them into proper failure + /// responses instead of panicking inside `conn.run`'s blocking closure. + /// In particular, this is reachable from the unauthenticated + /// `webauthn_login` grant in `src/api/identity.rs`, where a transient + /// DB error must not crash the worker. + pub async fn find_by_user(user_uuid: &UserId, conn: &DbConn) -> Result, Error> { db_run! { conn: { web_authn_credentials::table .filter(web_authn_credentials::user_uuid.eq(user_uuid)) .load::(conn) - .expect("Error loading web_authn_credentials") + .map_res("Error loading web_authn_credentials") }} } @@ -223,9 +217,20 @@ impl WebAuthnLoginChallenge { }} } - /// Fetch and delete a pending challenge (single-use). Returns `None` when the - /// token is unknown, has already been consumed, or the challenge has expired. - pub async fn take(id: &WebAuthnLoginChallengeId, conn: &DbConn) -> Option { + /// Fetch and delete a pending challenge (single-use). Three outcomes: + /// - `Ok(Some(_))` — winner of the SELECT+DELETE race; the row has been + /// removed and the caller may verify the assertion against the state. + /// - `Ok(None)` — token unknown, already consumed by a concurrent caller, + /// or the row was current but past the TTL cutoff (post-transaction + /// filter). All three collapse to a single "stale or invalid challenge" + /// path so the caller can't distinguish them — a small AUTH_FAILED + /// information-leak hardening for the unauthenticated login endpoint. + /// - `Err(_)` — DB degradation (deadlock, conn drop, lock timeout). The + /// surrounding transaction rolled back atomically, so the row is intact + /// rather than silently consumed; propagating via `?` lets the caller + /// surface a 5xx instead of an indistinguishable 4xx stale-token + /// response. + pub async fn take(id: &WebAuthnLoginChallengeId, conn: &DbConn) -> Result, Error> { db_run! { conn: { // Single-use is enforced by the `deleted == 1` row-count guard, not // by isolation: concurrent callers may all see the row in the @@ -235,7 +240,7 @@ impl WebAuthnLoginChallenge { // ensures the SELECT+DELETE pair rolls back atomically on a DB // error, leaving the challenge intact rather than silently // consuming it. - let taken = match conn + let taken = conn .transaction::, diesel::result::Error, _>(|conn| { let challenge = web_authn_login_challenges::table .filter(web_authn_login_challenges::id.eq(id)) @@ -246,20 +251,11 @@ impl WebAuthnLoginChallenge { ) .execute(conn)?; Ok(challenge.filter(|_| deleted == 1)) - }) { - Ok(opt) => opt, - Err(e) => { - // Surface the underlying error so a degrading DB - // (deadlock, conn drop, lock timeout) is operator- - // visible instead of masquerading as a stale-token - // rejection at the caller. - error!("WebAuthnLoginChallenge::take failed for id {id}: {e:#?}"); - None - } - }; + }) + .map_res("Error taking web_authn_login_challenge")?; let cutoff = Utc::now().naive_utc() - TimeDelta::seconds(WEBAUTHN_LOGIN_CHALLENGE_TTL_SECONDS); - taken.filter(|c| c.created_at >= cutoff) + Ok(taken.filter(|c| c.created_at >= cutoff)) }} } diff --git a/src/static/templates/scss/vaultwarden.scss.hbs b/src/static/templates/scss/vaultwarden.scss.hbs index 81c7faa8..7fb0d3d5 100644 --- a/src/static/templates/scss/vaultwarden.scss.hbs +++ b/src/static/templates/scss/vaultwarden.scss.hbs @@ -63,6 +63,19 @@ app-root ng-component > form > div:nth-child(1) > div > button[buttontype="secon .vw-passkey-login { @extend %vw-hide; } + +/* Mirror Bitwarden's `app-webauthn-login-settings` template gate + `*ngIf="hasData && !limitReached && !requireSsoPolicyEnabled"` on the + "Turn on" / "New passkey" Add button. Bitwarden suppresses just that + button under Require SSO; the credentials `
` and per-row Remove + buttons stay rendered so users can revoke legacy credentials from the + UI. Vaultwarden doesn't surface org policies to the client, so we apply + the same hide via CSS. The Add button is the only direct-child + `button[bitbutton]` of the component (per-row buttons use `bitlink` and + live deeper inside `
`). */ +app-webauthn-login-settings > button[bitbutton] { + @extend %vw-hide; +} {{/if}} /* Hide the or text followed by the two buttons hidden above */