diff --git a/src/api/core/accounts.rs b/src/api/core/accounts.rs index c0065c6b..c200bcd7 100644 --- a/src/api/core/accounts.rs +++ b/src/api/core/accounts.rs @@ -23,7 +23,7 @@ use crate::{ AuthRequest, AuthRequestId, Cipher, CipherId, Device, DeviceId, DeviceType, DeviceWithAuthRequest, EmergencyAccess, EmergencyAccessId, EventType, Folder, FolderId, Invitation, Membership, MembershipId, OrgPolicy, OrgPolicyType, Organization, OrganizationId, Send, SendId, User, UserId, UserKdfType, - WebAuthnCredential, WebAuthnCredentialId, + WebAuthnCredential, WebAuthnCredentialId, is_concurrent_modification, }, }, mail, @@ -1007,15 +1007,34 @@ async fn post_rotatekey(data: Json, headers: Headers, conn: DbConn, nt: if save_result.is_ok() { for credential in &rewrapped_credentials { if let Err(e) = credential.update_keys(&conn).await { + // A concurrent passkey delete (multi-replica) racing rotation is + // the common case here — log at INFO so ops alerts only fire on + // genuine DB transients. The follow-up clear is also a no-op + // for the same reason, so skip it on the benign branch. + if is_concurrent_modification(&e) { + info!( + "post_rotatekey: WebAuthn PRF credential {} for user {} was deleted concurrently with key rotation; skipping rewrap", + credential.uuid, user.uuid + ); + continue; + } warn!( "post_rotatekey: failed to rewrap WebAuthn PRF credential {} for user {} after key rotation: {e:#?}", credential.uuid, user.uuid ); - if let Err(clear_error) = credential.clear_prf_keyset(&conn).await { - warn!( - "post_rotatekey: failed to clear stale WebAuthn PRF keyset for credential {} after rewrap failure: {clear_error:#?}", + match credential.clear_account_key_prf_keyset(&conn).await { + Ok(()) => info!( + "post_rotatekey: cleared stale account-key PRF keyset for credential {} after rewrap failure", credential.uuid - ); + ), + Err(clear_error) if is_concurrent_modification(&clear_error) => info!( + "post_rotatekey: WebAuthn PRF credential {} for user {} was deleted concurrently after rewrap failure; nothing to clear", + credential.uuid, user.uuid + ), + Err(clear_error) => warn!( + "post_rotatekey: failed to clear stale account-key WebAuthn PRF keyset for credential {} after rewrap failure: {clear_error:#?}", + credential.uuid + ), } } } diff --git a/src/api/core/mod.rs b/src/api/core/mod.rs index 97805d6a..e1684495 100644 --- a/src/api/core/mod.rs +++ b/src/api/core/mod.rs @@ -14,7 +14,7 @@ pub use accounts::purge_auth_requests; pub use ciphers::{CipherData, CipherSyncData, CipherSyncType, purge_trashed_ciphers}; pub use emergency_access::{emergency_notification_reminder_job, emergency_request_timeout_job}; pub use events::{event_cleanup_job, log_event, log_user_event}; -pub(crate) use passkeys::{account_passkeys_allowed, check_passkey_endpoint_preconditions, passkey_credential_id_hash}; +pub(crate) use passkeys::{account_passkeys_allowed, passkey_counter, passkey_credential_id_hash}; pub use sends::purge_sends; use chrono::Utc; diff --git a/src/api/core/passkeys.rs b/src/api/core/passkeys.rs index 2f21a636..d76ea9e5 100644 --- a/src/api/core/passkeys.rs +++ b/src/api/core/passkeys.rs @@ -1,22 +1,27 @@ use chrono::Utc; use rocket::{Route, http::Status, serde::json::Json, serde::json::Value}; use webauthn_rs::prelude::{ - CreationChallengeResponse, Credential as WebauthnCredentialData, Passkey, PasskeyAuthentication, + Base64UrlSafeData, CreationChallengeResponse, Credential as WebauthnCredentialData, Passkey, PasskeyAuthentication, PasskeyRegistration, }; -use webauthn_rs_proto::{ExtnState, RequestRegistrationExtensions, UserVerificationPolicy}; +use webauthn_rs_proto::{ + AuthenticatorAttestationResponseRaw, AuthenticatorTransport, ExtnState, RegisterPublicKeyCredential, + RegistrationExtensionsClientOutputs, RequestRegistrationExtensions, UserVerificationPolicy, +}; use crate::{ CONFIG, api::{ ApiResult, JsonResult, Notify, PasswordOrOtpData, UpdateType, - core::two_factor::webauthn::{PublicKeyCredentialCopy, RegisterPublicKeyCredentialCopy, WEBAUTHN}, + core::two_factor::webauthn::{PublicKeyCredentialCopy, WEBAUTHN}, }, auth::Headers, crypto, db::{ DbConn, - models::{TwoFactor, TwoFactorType, User, WebAuthnCredential, WebAuthnCredentialId}, + models::{ + TwoFactor, TwoFactorType, User, WebAuthnCredential, WebAuthnCredentialId, is_concurrent_modification, + }, }, error::Error, util::get_uuid, @@ -83,6 +88,43 @@ struct WebAuthnPasskeyAssertionChallenge { state: PasskeyAuthentication, } +#[derive(Debug, Deserialize)] +#[serde(rename_all = "camelCase")] +struct PasskeyRegisterPublicKeyCredentialCopy { + id: String, + raw_id: Base64UrlSafeData, + response: PasskeyAuthenticatorAttestationResponseRawCopy, + #[serde(default, alias = "clientExtensionResults")] + extensions: RegistrationExtensionsClientOutputs, + r#type: String, +} + +#[derive(Debug, Deserialize)] +#[serde(rename_all = "camelCase")] +struct PasskeyAuthenticatorAttestationResponseRawCopy { + #[serde(rename = "AttestationObject", alias = "attestationObject")] + attestation_object: Base64UrlSafeData, + #[serde(rename = "clientDataJson", alias = "clientDataJSON")] + client_data_json: Base64UrlSafeData, + transports: Option>, +} + +impl From for RegisterPublicKeyCredential { + fn from(r: PasskeyRegisterPublicKeyCredentialCopy) -> Self { + Self { + id: r.id, + raw_id: r.raw_id, + response: AuthenticatorAttestationResponseRaw { + attestation_object: r.response.attestation_object, + client_data_json: r.response.client_data_json, + transports: r.response.transports, + }, + type_: r.r#type, + extensions: r.extensions, + } + } +} + fn passkey_management_challenge_is_fresh(created_at: i64) -> bool { // The timestamp is server-stamped, so a future value only happens after a // clock step backwards or manual DB tampering. Allow a small skew window @@ -156,8 +198,29 @@ fn passkey_count_limit_reached(count: usize) -> bool { count >= MAX_WEBAUTHN_CREDENTIALS } +/// Single source of truth for whether the deployment posture permits account +/// passkeys — gates `/sync`'s `webAuthnPrfOptions`, `/api/config`'s +/// `pm-2035-passkey-unlock` flag, and [`check_passkey_endpoint_preconditions`] +/// so the lock-screen affordance the client renders agrees with the server's +/// actual acceptance posture. pub(crate) fn account_passkeys_allowed() -> bool { - !(CONFIG.sso_enabled() && CONFIG.sso_only()) && CONFIG.is_webauthn_2fa_supported() + account_passkeys_disabled_reason().is_none() +} + +#[derive(Clone, Copy)] +enum AccountPasskeysDisabledReason { + WebAuthnUnsupported, + SsoOnly, +} + +fn account_passkeys_disabled_reason() -> Option { + if !CONFIG.is_webauthn_2fa_supported() { + Some(AccountPasskeysDisabledReason::WebAuthnUnsupported) + } else if CONFIG.sso_enabled() && CONFIG.sso_only() { + Some(AccountPasskeysDisabledReason::SsoOnly) + } else { + None + } } fn request_passkey_prf_extension( @@ -184,7 +247,22 @@ fn passkey_supports_prf(passkey: &Passkey) -> bool { matches!(credential.extensions.hmac_create_secret, ExtnState::Set(true)) } -type PasskeyRegistrationPrfData = (bool, Option, Option, Option); +/// `webauthn-rs`'s `Passkey::cred` is `pub(crate)`, so there is no direct +/// accessor for the stored signature counter; the only path is to clone the +/// struct and convert into `Credential`. Centralised here so a future upstream +/// `Passkey::counter()` accessor is a one-line swap. +pub(crate) fn passkey_counter(passkey: &Passkey) -> u32 { + let credential: WebauthnCredentialData = passkey.clone().into(); + credential.counter +} + +#[derive(Debug, PartialEq)] +struct PasskeyRegistrationPrfData { + supports_prf: bool, + encrypted_user_key: Option, + encrypted_public_key: Option, + encrypted_private_key: Option, +} fn passkey_registration_prf_data( client_supports_prf: bool, @@ -201,7 +279,12 @@ fn passkey_registration_prf_data( if has_key_material { err!("Passkey does not support PRF") } - return Ok((false, None, None, None)); + return Ok(PasskeyRegistrationPrfData { + supports_prf: false, + encrypted_user_key: None, + encrypted_public_key: None, + encrypted_private_key: None, + }); } // Chromium/CDP does not consistently reflect the registration PRF @@ -209,7 +292,12 @@ fn passkey_registration_prf_data( // whether the browser ceremony supports PRF. Store that client capability // signal; only the presence of wrapped key blobs controls unlock. if !has_key_material { - return Ok((true, None, None, None)); + return Ok(PasskeyRegistrationPrfData { + supports_prf: true, + encrypted_user_key: None, + encrypted_public_key: None, + encrypted_private_key: None, + }); } let Some(encrypted_user_key) = encrypted_user_key else { @@ -222,34 +310,42 @@ fn passkey_registration_prf_data( err!("Encrypted private key is required") }; - Ok((true, Some(encrypted_user_key), Some(encrypted_public_key), Some(encrypted_private_key))) + Ok(PasskeyRegistrationPrfData { + supports_prf: true, + encrypted_user_key: Some(encrypted_user_key), + encrypted_public_key: Some(encrypted_public_key), + encrypted_private_key: Some(encrypted_private_key), + }) } -/// Gates every passkey-management entry point in this module on the same -/// three preconditions: -/// • `check_limit_login` — IP-level rate limit shared with the password -/// login path. Runs FIRST so a misconfigured DOMAIN can't be turned -/// into an uncapped error-log generator: every refused request would -/// otherwise short-circuit on `is_webauthn_2fa_supported` without -/// consuming a rate-limit token. -/// • `is_webauthn_2fa_supported` — refuses cleanly when DOMAIN is -/// incompatible with WebAuthn (must run before the `WEBAUTHN` LazyLock -/// is touched, which would otherwise panic). -/// • SSO_ONLY — refuses passkey mutations when the operator has required -/// SSO sign-in. +/// Gates the passkey-management MUTATION endpoints (POST/PUT `/webauthn` and +/// the two `*-options` endpoints) in order: +/// - `check_limit_login` - shared IP rate limit, FIRST so a misconfigured +/// DOMAIN can't become an uncapped error-log generator (a refused request +/// would otherwise short-circuit without consuming a rate-limit token). +/// - `is_webauthn_2fa_supported` - refuses cleanly when DOMAIN is +/// incompatible; must run before the `WEBAUTHN` LazyLock is touched, which +/// would otherwise panic. +/// - SSO_ONLY - refuses passkey mutations under required SSO (Bitwarden parity). /// -/// `action_verb` parameterises the SSO refusal message between the create -/// and update endpoints ("created" / "updated"). The delete endpoint is -/// intentionally NOT gated — see the comment on `post_api_webauthn_delete`. +/// `get_api_webauthn` (read-only) and `post_api_webauthn_delete` +/// (capability-narrowing; gating it would strand credentials) are intentionally +/// NOT gated here. The unauthenticated `get_web_authn_assertion_options` in +/// `identity.rs` inlines the same checks but collapses them to the opaque +/// `PASSKEY_AUTH_FAILED` — the admin-facing wording below must not reach +/// anonymous callers. `action_verb` ("created"/"updated") keeps the SSO refusal +/// message precise. pub(crate) fn check_passkey_endpoint_preconditions(ip: &std::net::IpAddr, action_verb: &str) -> ApiResult<()> { crate::ratelimit::check_limit_login(ip)?; - 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!(format!("Passkeys cannot be {action_verb} when SSO sign-in is required")) + match account_passkeys_disabled_reason() { + Some(AccountPasskeysDisabledReason::WebAuthnUnsupported) => { + err!("Webauthn is not supported on this server. Set `DOMAIN` to a valid URL with a parseable host.") + } + Some(AccountPasskeysDisabledReason::SsoOnly) => { + err!(format!("Passkeys cannot be {action_verb} when SSO sign-in is required")) + } + None => Ok(()), } - Ok(()) } #[post("/webauthn/attestation-options", data = "")] @@ -270,11 +366,24 @@ async fn post_api_webauthn_attestation_options( err!("Maximum number of passkeys reached") } + // Skip credentials whose stored blob fails to deserialize rather than + // propagating: a single corrupt row would otherwise block the user from + // enrolling ANY new passkey. The browser's excludeCredentials list ends + // up missing those entries, but the UNIQUE(user_uuid, credential_id_hash) + // constraint at `save_with_user_limit` still catches a re-enrolment of + // the corrupt row's authenticator. Surface the corruption to ops via + // `warn!` so the bad row can be investigated. let existing_cred_ids: Vec<_> = all_creds .into_iter() - .filter_map(|wac| { - let passkey: Passkey = serde_json::from_str(&wac.credential).ok()?; - Some(passkey.cred_id().to_owned()) + .filter_map(|wac| match serde_json::from_str::(&wac.credential) { + Ok(passkey) => Some(passkey.cred_id().to_owned()), + Err(e) => { + warn!( + "post_api_webauthn_attestation_options: corrupt credential blob for {} (user {}), skipping from excludeCredentials: {e:#?}", + wac.uuid, wac.user_uuid + ); + None + } }) .collect(); @@ -343,11 +452,25 @@ async fn post_api_webauthn_assertion_options( data.validate(&user, true, &conn).await?; + // Skip credentials whose stored blob fails to deserialize rather than + // propagating: a single corrupt PRF row would otherwise block the user + // from EVER initiating PRF unlock with any other healthy passkey. Same + // shape as `post_api_webauthn_attestation_options` above — surface the + // corruption to ops via `warn!` so the bad row can be investigated. let credentials: Vec = WebAuthnCredential::find_by_user(&user.uuid, &conn) .await? .into_iter() .filter(|wac| wac.supports_prf) - .filter_map(|wac| serde_json::from_str(&wac.credential).ok()) + .filter_map(|wac| match serde_json::from_str::(&wac.credential) { + Ok(passkey) => Some(passkey), + Err(e) => { + warn!( + "post_api_webauthn_assertion_options: corrupt credential blob for {} (user {}), skipping PRF-capable passkey: {e:#?}", + wac.uuid, wac.user_uuid + ); + None + } + }) .collect(); if credentials.is_empty() { @@ -386,7 +509,7 @@ async fn post_api_webauthn_assertion_options( #[derive(Debug, Deserialize)] #[serde(rename_all = "camelCase")] struct WebAuthnLoginCredentialCreateRequest { - device_response: RegisterPublicKeyCredentialCopy, + device_response: PasskeyRegisterPublicKeyCredentialCopy, name: String, token: Option, supports_prf: bool, @@ -407,10 +530,11 @@ async fn post_api_webauthn( let data: WebAuthnLoginCredentialCreateRequest = data.into_inner(); let user = headers.user; - // Atomically take the saved challenge state (single-use): concurrent - // 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. + // Consume the exact challenge row only after token/security-stamp + // validation, so a stale tab can't delete the user's current challenge + // with an old token. A failed validation leaves the row until the next + // options request replaces it; UNIQUE(user_uuid, atype) bounds it to one + // stale-but-unredeemable row per user/type. let Some(mut current_user) = User::try_find_by_uuid(&user.uuid, &conn).await? else { err!("User not found") }; @@ -420,20 +544,28 @@ async fn post_api_webauthn( } 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::try_find_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(), ¤t_user.security_stamp)?; + if !tf.delete_by_uuid(&conn).await? { + err!("No registration challenge found. Please try again.") + } + let credential = WEBAUTHN.finish_passkey_registration(&data.device_response.into(), &state)?; let credential_id_hash = passkey_credential_id_hash(credential.cred_id().as_slice()); - let (supports_prf, encrypted_user_key, encrypted_public_key, encrypted_private_key) = - passkey_registration_prf_data( - data.supports_prf, - data.encrypted_user_key, - data.encrypted_public_key, - data.encrypted_private_key, - passkey_supports_prf(&credential), - )?; + let PasskeyRegistrationPrfData { + supports_prf, + encrypted_user_key, + encrypted_public_key, + encrypted_private_key, + } = passkey_registration_prf_data( + data.supports_prf, + data.encrypted_user_key, + data.encrypted_public_key, + data.encrypted_private_key, + passkey_supports_prf(&credential), + )?; // Duplicate detection rests on the UNIQUE `(user_uuid, credential_id_hash)` // index: `save_with_user_limit` below maps the `UniqueViolation` to @@ -492,19 +624,23 @@ async fn put_api_webauthn( err!("Encrypted private key is required") }; - // Atomically take the saved challenge state (single-use): concurrent - // updates for the same assertion row cannot both succeed and apply - // different blob payloads — only the caller whose DELETE removes the row - // proceeds. + // Consume the exact challenge row only after token/security-stamp + // validation, so a stale tab can't delete the user's current challenge + // with an old token. A failed validation leaves the row until the next + // options request replaces it; UNIQUE(user_uuid, atype) bounds it to one + // stale-but-unredeemable row per user/type. let Some(mut current_user) = User::try_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::try_find_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, ¤t_user.security_stamp)?; + if !tf.delete_by_uuid(&conn).await? { + err!("No assertion challenge found. Please try again.") + } let credential_response = data.device_response.into(); @@ -527,27 +663,44 @@ async fn put_api_webauthn( err!("Passkey does not support PRF") } - let mut passkey: Passkey = serde_json::from_str(&matched_wac.credential)?; - - // Persist the (optional) signature-counter advance and the PRF keyset - // together. The assertion challenge was atomically consumed via - // `take_by_user_and_type` above, so a half-applied state would block - // any retry — the helper folds both writes into a single UPDATE. - // - // `advanced_counter` gates the `credential` column write. Passing `false` - // when the counter did not advance avoids clobbering a counter blob a - // parallel replica may have just persisted via `webauthn_login`'s - // counter advance (the per-process DashMap lock does not serialise - // across replicas). The helper surfaces 0-rows as a Simple error so a - // concurrent DELETE doesn't yield a misleading 200 OK with no row. - let advanced_counter = passkey.update_credential(&authentication_result) == Some(true); + let previous_credential = matched_wac.credential.clone(); + let mut passkey: Passkey = serde_json::from_str(&previous_credential)?; + + // Persist the PRF keyset, optionally folding a signature-counter advance + // into the same UPDATE. Backup-state-only WebAuthn changes are not written: + // with blob storage, writing a stale backup-state toggle can roll back a + // counter advanced by a parallel replica. + let previous_counter = passkey_counter(&passkey); + let updated_credential = passkey.update_credential(&authentication_result) == Some(true); + let advanced_counter = updated_credential && authentication_result.counter() > previous_counter; if advanced_counter { matched_wac.credential = serde_json::to_string(&passkey)?; } matched_wac.encrypted_user_key = Some(encrypted_user_key); matched_wac.encrypted_public_key = Some(encrypted_public_key); matched_wac.encrypted_private_key = Some(encrypted_private_key); - matched_wac.update_credential_and_prf_keyset(advanced_counter, &conn).await?; + if let Err(e) = matched_wac + .update_credential_and_prf_keyset( + advanced_counter, + advanced_counter.then_some(previous_credential.as_str()), + &conn, + ) + .await + { + // Surface a CAS-loser race as a 409 + retry hint. This INTENTIONALLY + // diverges from `webauthn_login`, which is unauthenticated and masks CM + // behind a generic 503/INFO so an attacker gets no retry signal; here + // the caller is authenticated and can act on the 409, and operators + // want ERROR-level visibility into PRF-enrol contention. + if is_concurrent_modification(&e) { + err_code!( + "Passkey credential modified concurrently. Try again.", + format!("user {} credential {}: {e:#?}", user.uuid, matched_wac.uuid), + Status::Conflict.code + ) + } + return Err(e); + } current_user.update_revision(&conn).await?; nt.send_user_update(UpdateType::SyncVault, ¤t_user, headers.device.push_uuid.as_ref(), &conn).await; @@ -575,10 +728,13 @@ async fn post_api_webauthn_delete( 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; + let deleted = WebAuthnCredential::delete_by_uuid_and_user(&uuid, &user.uuid, &conn).await?; + if deleted { + user.update_revision(&conn).await?; + nt.send_user_update(UpdateType::SyncVault, &user, headers.device.push_uuid.as_ref(), &conn).await; + } else { + debug!("post_api_webauthn_delete: credential {uuid} was not registered for user {}", user.uuid); + } Ok(Status::Ok) } @@ -597,6 +753,47 @@ mod tests { WebauthnBuilder::new("localhost", &origin).unwrap().rp_name("localhost").build().unwrap() } + #[test] + fn passkey_register_public_key_credential_copy_preserves_transports() { + let transports = vec![AuthenticatorTransport::Internal, AuthenticatorTransport::Hybrid]; + let copy = PasskeyRegisterPublicKeyCredentialCopy { + id: String::from("credential"), + raw_id: Base64UrlSafeData::from([1, 2, 3]), + response: PasskeyAuthenticatorAttestationResponseRawCopy { + attestation_object: Base64UrlSafeData::from([4, 5, 6]), + client_data_json: Base64UrlSafeData::from([7, 8, 9]), + transports: Some(transports.clone()), + }, + extensions: RegistrationExtensionsClientOutputs::default(), + r#type: String::from("public-key"), + }; + + let converted: RegisterPublicKeyCredential = copy.into(); + + assert_eq!(converted.response.transports, Some(transports)); + } + + #[test] + fn passkey_register_public_key_credential_copy_accepts_client_extension_results_alias() { + let copy = serde_json::from_value::(json!({ + "id": "credential", + "rawId": "AQID", + "response": { + "attestationObject": "BAUG", + "clientDataJson": "BwgJ", + "transports": ["internal"] + }, + "clientExtensionResults": {}, + "type": "public-key" + })) + .unwrap(); + + let converted: RegisterPublicKeyCredential = copy.into(); + + assert_eq!(converted.id, "credential"); + assert_eq!(converted.response.transports, Some(vec![AuthenticatorTransport::Internal])); + } + fn passkey() -> Passkey { Credential { cred_id: [1, 2, 3, 4].into(), @@ -655,6 +852,16 @@ mod tests { .into() } + /// Regression guard: `request_passkey_prf_extension` drills into + /// `webauthn-rs`'s `PasskeyRegistration` via the JSON-Value path + /// `rs.extensions.hmacCreateSecret`. The `rs` field is `pub(crate)` in + /// upstream and its serde name is the field identifier — any future + /// webauthn-rs version that renames the wrapper, restructures + /// `RegistrationState`, or moves the extensions sub-object out of it + /// would silently break PRF registration with no compile-time signal. + /// This test exercises the same path the production helper does and + /// asserts the extension lands in both the client-facing challenge + /// AND the round-tripped server-side state. #[test] fn request_passkey_prf_extension_marks_challenge_and_stored_state() { let user_uuid = uuid::Uuid::parse_str("00000000-0000-0000-0000-000000000000").unwrap(); @@ -687,7 +894,12 @@ mod tests { false, ) .unwrap(), - (true, Some("user-key".to_owned()), Some("public-key".to_owned()), Some("private-key".to_owned()),) + PasskeyRegistrationPrfData { + supports_prf: true, + encrypted_user_key: Some("user-key".to_owned()), + encrypted_public_key: Some("public-key".to_owned()), + encrypted_private_key: Some("private-key".to_owned()), + } ); } @@ -727,15 +939,26 @@ mod tests { true, ) .unwrap(), - (true, Some("user-key".to_owned()), Some("public-key".to_owned()), Some("private-key".to_owned()),) + PasskeyRegistrationPrfData { + supports_prf: true, + encrypted_user_key: Some("user-key".to_owned()), + encrypted_public_key: Some("public-key".to_owned()), + encrypted_private_key: Some("private-key".to_owned()), + } ); } #[test] fn passkey_registration_prf_data_records_prf_support_even_without_client_keyset() { - assert_eq!(passkey_registration_prf_data(false, None, None, None, true).unwrap(), (true, None, None, None)); - assert_eq!(passkey_registration_prf_data(true, None, None, None, false).unwrap(), (true, None, None, None)); - assert_eq!(passkey_registration_prf_data(false, None, None, None, false).unwrap(), (false, None, None, None)); + let only_support = |supports_prf| PasskeyRegistrationPrfData { + supports_prf, + encrypted_user_key: None, + encrypted_public_key: None, + encrypted_private_key: None, + }; + assert_eq!(passkey_registration_prf_data(false, None, None, None, true).unwrap(), only_support(true)); + assert_eq!(passkey_registration_prf_data(true, None, None, None, false).unwrap(), only_support(true)); + assert_eq!(passkey_registration_prf_data(false, None, None, None, false).unwrap(), only_support(false)); } #[test] diff --git a/src/api/core/two_factor/webauthn.rs b/src/api/core/two_factor/webauthn.rs index f52a098f..759a5653 100644 --- a/src/api/core/two_factor/webauthn.rs +++ b/src/api/core/two_factor/webauthn.rs @@ -10,7 +10,7 @@ use webauthn_rs::{ }; use webauthn_rs_proto::{ AuthenticationExtensionsClientOutputs, AuthenticatorAssertionResponseRaw, AuthenticatorAttestationResponseRaw, - AuthenticatorTransport, PublicKeyCredential, RegisterPublicKeyCredential, RegistrationExtensionsClientOutputs, + PublicKeyCredential, RegisterPublicKeyCredential, RegistrationExtensionsClientOutputs, RequestAuthenticationExtensions, UserVerificationPolicy, }; @@ -185,39 +185,35 @@ struct EnableWebauthnData { #[derive(Debug, Deserialize)] #[serde(rename_all = "camelCase")] -pub struct RegisterPublicKeyCredentialCopy { +struct RegisterPublicKeyCredentialCopy { pub id: String, pub raw_id: Base64UrlSafeData, pub response: AuthenticatorAttestationResponseRawCopy, - #[serde(default, alias = "clientExtensionResults")] - pub extensions: RegistrationExtensionsClientOutputs, pub r#type: String, } // This is copied from AuthenticatorAttestationResponseRaw to change clientDataJSON to clientDataJson #[derive(Debug, Deserialize)] #[serde(rename_all = "camelCase")] -pub struct AuthenticatorAttestationResponseRawCopy { +struct AuthenticatorAttestationResponseRawCopy { #[serde(rename = "AttestationObject", alias = "attestationObject")] pub attestation_object: Base64UrlSafeData, #[serde(rename = "clientDataJson", alias = "clientDataJSON")] pub client_data_json: Base64UrlSafeData, - pub transports: Option>, } impl From for RegisterPublicKeyCredential { fn from(r: RegisterPublicKeyCredentialCopy) -> Self { - let transports = r.response.transports; Self { id: r.id, raw_id: r.raw_id, response: AuthenticatorAttestationResponseRaw { attestation_object: r.response.attestation_object, client_data_json: r.response.client_data_json, - transports, + transports: None, }, type_: r.r#type, - extensions: r.extensions, + extensions: RegistrationExtensionsClientOutputs::default(), } } } @@ -535,42 +531,61 @@ mod tests { use super::*; #[test] - fn register_public_key_credential_copy_preserves_transports() { - let transports = vec![AuthenticatorTransport::Internal, AuthenticatorTransport::Hybrid]; + fn register_public_key_credential_copy_strips_transports() { let copy = RegisterPublicKeyCredentialCopy { id: String::from("credential"), raw_id: Base64UrlSafeData::from([1, 2, 3]), response: AuthenticatorAttestationResponseRawCopy { attestation_object: Base64UrlSafeData::from([4, 5, 6]), client_data_json: Base64UrlSafeData::from([7, 8, 9]), - transports: Some(transports.clone()), }, - extensions: RegistrationExtensionsClientOutputs::default(), r#type: String::from("public-key"), }; let converted: RegisterPublicKeyCredential = copy.into(); - assert_eq!(converted.response.transports, Some(transports)); + assert_eq!(converted.response.transports, None); } #[test] - fn register_public_key_credential_copy_keeps_absent_transports_absent() { + fn register_public_key_credential_copy_ignores_extra_client_fields() { + let copy = serde_json::from_value::(json!({ + "id": "credential", + "rawId": "AQID", + "response": { + "attestationObject": "BAUG", + "clientDataJson": "BwgJ", + "transports": ["internal", "hybrid"] + }, + "clientExtensionResults": {}, + "type": "public-key" + })) + .unwrap(); + + let converted: RegisterPublicKeyCredential = copy.into(); + + assert_eq!(converted.response.transports, None); + } + + #[test] + fn register_public_key_credential_copy_defaults_extensions() { let copy = RegisterPublicKeyCredentialCopy { id: String::from("credential"), raw_id: Base64UrlSafeData::from([1, 2, 3]), response: AuthenticatorAttestationResponseRawCopy { attestation_object: Base64UrlSafeData::from([4, 5, 6]), client_data_json: Base64UrlSafeData::from([7, 8, 9]), - transports: None, }, - extensions: RegistrationExtensionsClientOutputs::default(), r#type: String::from("public-key"), }; let converted: RegisterPublicKeyCredential = copy.into(); - assert_eq!(converted.response.transports, None); + assert!(converted.extensions.appid.is_none()); + assert!(converted.extensions.cred_props.is_none()); + assert!(converted.extensions.hmac_secret.is_none()); + assert!(converted.extensions.cred_protect.is_none()); + assert!(converted.extensions.min_pin_length.is_none()); } #[test] diff --git a/src/api/identity.rs b/src/api/identity.rs index 3797c171..87b94404 100644 --- a/src/api/identity.rs +++ b/src/api/identity.rs @@ -19,7 +19,7 @@ use crate::{ ApiResult, EmptyResult, JsonResult, core::{ accounts::{PreloginData, RegisterData, kdf_upgrade, prelogin, register}, - check_passkey_endpoint_preconditions, log_user_event, passkey_credential_id_hash, + log_user_event, passkey_counter, passkey_credential_id_hash, two_factor::{ authenticator, duo, duo_oidc, email, enforce_2fa_policy, is_twofactor_provider_usable, webauthn, yubikey, @@ -36,15 +36,18 @@ use crate::{ models::{ AuthRequest, AuthRequestId, Device, DeviceId, EventType, Invitation, OIDCCodeResponseError, OrganizationApiKey, OrganizationId, SsoAuth, SsoUser, TwoFactor, TwoFactorIncomplete, TwoFactorType, User, - UserId, WebAuthnCredential, WebAuthnLoginChallenge, WebAuthnLoginChallengeId, + UserId, WebAuthnCredential, WebAuthnLoginChallenge, WebAuthnLoginChallengeId, is_concurrent_modification, }, }, - error::MapResult, + error::{Error, ErrorEvent, MapResult}, mail, sso, sso::{OIDCCode, OIDCCodeChallenge, OIDCCodeVerifier, OIDCState}, util, }; +const PASSKEY_AUTH_FAILED: &str = "Passkey authentication failed."; +const PASSKEY_SERVER_ERROR: &str = "A server error occurred. Try again later."; + pub fn routes() -> Vec { routes![ login, @@ -1130,6 +1133,11 @@ fn passkey_credential_id(passkey: &Passkey) -> String { data_encoding::BASE64URL_NOPAD.encode(passkey.cred_id().as_slice()) } +/// Workaround for webauthn-rs's `Passkey` type not exposing a public +/// accessor for the stored transport hints; the only path is to clone +/// the struct and convert into `Credential`. Centralised here so a future +/// upstream `Passkey::transports()` accessor is a one-line swap. Mirrors +/// the same pattern in `crate::api::core::passkeys::passkey_counter`. fn passkey_transports(passkey: &Passkey) -> Vec { let credential: Credential = passkey.clone().into(); credential.transports.into_iter().flatten().map(|transport| transport.to_string()).collect() @@ -1192,8 +1200,22 @@ pub(crate) fn build_webauthn_prf_options(credentials: &[WebAuthnCredential]) -> #[get("/accounts/webauthn/assertion-options")] async fn get_web_authn_assertion_options(ip: ClientIp, fetch_metadata: FetchMetadata, conn: DbConn) -> JsonResult { - reject_cross_site_passkey_challenge_request(&fetch_metadata)?; - check_passkey_endpoint_preconditions(&ip.ip, "used")?; + reject_cross_site_passkey_challenge_request(&fetch_metadata, &ip.ip)?; + crate::ratelimit::check_limit_login(&ip.ip)?; + // Mirrors the inline gate in `webauthn_login` (see the matching comment + // there). An unauthenticated caller must not see the verbose + // `Set DOMAIN to a valid URL ...` admin guidance that + // `check_passkey_endpoint_preconditions` writes into the response body — + // collapse it to the opaque PASSKEY_AUTH_FAILED and keep the rationale + // in the server log. The SSO_ONLY branch stays verbose: it's the + // SSO-required signal the bundled web vault routes on, and SSO_ONLY + // status is already observable via the password grant. + if !CONFIG.is_webauthn_2fa_supported() { + return Err(passkey_login_preauth_error(&ip.ip, "Webauthn unsupported on this server.")); + } + if CONFIG.sso_enabled() && CONFIG.sso_only() { + err!("Passkeys cannot be used when SSO sign-in is required") + } // start_discoverable_authentication() requests an empty allow-list // (discoverable credentials) and user verification. @@ -1237,7 +1259,7 @@ impl<'r> FromRequest<'r> for FetchMetadata { /// us when that GET was initiated from another site; reject those before we /// spend a login-rate-limit token or insert a challenge row. Missing headers /// are allowed for older browsers and non-browser clients. -fn reject_cross_site_passkey_challenge_request(fetch_metadata: &FetchMetadata) -> EmptyResult { +fn reject_cross_site_passkey_challenge_request(fetch_metadata: &FetchMetadata, ip: &std::net::IpAddr) -> EmptyResult { let Some(sec_fetch_site) = fetch_metadata.sec_fetch_site.as_deref() else { return Ok(()); }; @@ -1249,62 +1271,122 @@ fn reject_cross_site_passkey_challenge_request(fetch_metadata: &FetchMetadata) - return Ok(()); } - err_code!("Passkey login challenge requests must be same-site", Status::Forbidden.code) + // This branch intentionally avoids `err_code!`: direct cross-site callers + // can hit it before the login rate limiter below, so logging every refusal + // at ERROR would create an unthrottled log-noise path. Keep a debug-level + // breadcrumb for operators running with verbose logs without making this a + // normal error-volume vector. + debug!("Rejecting cross-site passkey challenge request from {ip}; Sec-Fetch-Site={sec_fetch_site}"); + Err(Error::new_msg("Passkey login challenge requests must be same-site").with_code(Status::Forbidden.code)) +} + +/// Centralised constructor for the post-verification AUTH_FAILED error shape: +/// emits a single ERROR-level log line, prepends `IP: {ip}. ` to the log value, +/// and attaches `UserFailedLogIn` for the outer login dispatcher's audit hook. +fn passkey_login_auth_error(ip: &std::net::IpAddr, log_detail: &str) -> Error { + let log_value = format!("IP: {ip}. {log_detail}"); + error!("{PASSKEY_AUTH_FAILED} {log_value}"); + Error::new(PASSKEY_AUTH_FAILED, log_value).with_event(ErrorEvent { + event: EventType::UserFailedLogIn, + }) +} + +/// Pre-verification AUTH_FAILED constructor. Identical to +/// [`passkey_login_auth_error`] without the `UserFailedLogIn` audit event — +/// the client-supplied user handle is not trusted yet, so attaching an event +/// tagged with it would let an unauthenticated caller pollute a victim's +/// audit log by submitting bogus assertions against the victim's UUID. The +/// trade-off intentionally loses the audit signal for genuine sig-fails +/// against known credentials; the server log still captures every refusal. +fn passkey_login_preauth_error(ip: &std::net::IpAddr, log_detail: &str) -> Error { + let log_value = format!("IP: {ip}. {log_detail}"); + error!("{PASSKEY_AUTH_FAILED} {log_value}"); + Error::new(PASSKEY_AUTH_FAILED, log_value) +} + +/// SERVER_ERROR (503) constructor for genuine DB transients and other +/// operator-actionable conditions. Logs at ERROR and attaches a +/// `UserFailedLogIn` event which the dispatcher only realises when `user_id` +/// is set post-verification, so pre-bind callers don't produce a user-scoped +/// audit row. For multi-replica CAS-loser races use +/// [`passkey_login_concurrent_modification_error`] instead — same 503 shape +/// but INFO-level so error alerts only fire on actionable failures. +fn passkey_login_server_error(ip: &std::net::IpAddr, log_detail: &str) -> Error { + let log_value = format!("IP: {ip}. {log_detail}"); + error!("{PASSKEY_SERVER_ERROR} {log_value}"); + Error::new(PASSKEY_SERVER_ERROR, log_value).with_code(Status::ServiceUnavailable.code).with_event(ErrorEvent { + event: EventType::UserFailedLogIn, + }) +} + +/// Same 503 response shape as [`passkey_login_server_error`] but logs the +/// failure at INFO and does NOT attach a `UserFailedLogIn` event. Call ONLY +/// when [`is_concurrent_modification`] confirms a benign CAS-loser race +/// (peer replica or another tab beat us to the row) — the user-visible +/// response is identical to mask retry hints from unauthenticated callers; +/// operators just don't get an ERROR-level alert or audit-event spike for +/// what is a normal multi-replica outcome. +fn passkey_login_concurrent_modification_error(ip: &std::net::IpAddr, log_detail: &str) -> Error { + let log_value = format!("IP: {ip}. {log_detail}"); + info!("{PASSKEY_SERVER_ERROR} {log_value}"); + Error::new(PASSKEY_SERVER_ERROR, log_value).with_code(Status::ServiceUnavailable.code) } async fn webauthn_login(data: ConnectData, user_id: &mut Option, conn: &DbConn, ip: &ClientIp) -> JsonResult { // Credential and account-state failures use a single generic message so // the endpoint cannot be used to probe which accounts exist or have passkeys. - // Server-side availability failures get a distinct temporary-unavailable - // response below so real users are not told their credential failed when - // the backend simply could not complete the login. - const AUTH_FAILED: &str = "Passkey authentication failed."; - const SERVER_ERROR: &str = "A server error occurred. Try again later."; - - /// `err!(AUTH_FAILED, format!(detail), ErrorEvent { event: UserFailedLogIn })` - /// in one line. Credential and account-state failure branches use exactly - /// this triple — a Simple-variant error with the constant user-facing - /// message and a UserFailedLogIn audit event — so the unauthenticated - /// grant cannot be used as an account-state oracle. + // Server-side availability failures get a distinct server-error response + // below so real users are not told their credential failed when the + // backend simply could not complete the login. + /// One-line wrappers around the three module-level error constructors + /// (`passkey_login_auth_error` / `passkey_login_preauth_error` / + /// `passkey_login_server_error`) that provide the early-return ergonomic + /// (`return Err(...)`) and the captured `ip: &ClientIp` binding. The + /// helpers themselves prepend `IP: {ip}.` to the log value and emit the + /// single ERROR-level log line, so all three macros stay consistent on + /// log shape and error variant. /// - /// Scoped INSIDE `webauthn_login` so the macro is lexically unreachable + /// Scoped INSIDE `webauthn_login` so the macros are lexically unreachable /// from any other function. A future contributor adding an auth handler - /// elsewhere in the module cannot accidentally invoke this macro (it - /// would fail to resolve), forcing them to make an explicit decision - /// about what their function's audit-event semantics should be rather - /// than silently inheriting `EventType::UserFailedLogIn` here. + /// elsewhere in the module cannot accidentally invoke these (they would + /// fail to resolve), forcing an explicit choice between + /// `auth_fail!` (post-verification, attaches `UserFailedLogIn` event), + /// `preauth_fail!` (pre-verification, no event — see + /// `passkey_login_preauth_error` doc for the audit-log-pollution + /// rationale), and `server_fail!` (503 server error, attaches + /// `UserFailedLogIn` only when the outer dispatcher has a verified + /// `user_id`). All three capture the enclosing `ip` binding; keep it in + /// scope if this logic is extracted. macro_rules! auth_fail { - ($($fmt_arg:tt)*) => { - err!( - AUTH_FAILED, - format!($($fmt_arg)*), - ErrorEvent { event: EventType::UserFailedLogIn } - ) - }; + ($($fmt_arg:tt)*) => {{ + return Err(passkey_login_auth_error(&ip.ip, &format!($($fmt_arg)*))); + }}; + } + + macro_rules! preauth_fail { + ($($fmt_arg:tt)*) => {{ + return Err(passkey_login_preauth_error(&ip.ip, &format!($($fmt_arg)*))); + }}; } macro_rules! server_fail { - ($($fmt_arg:tt)*) => { - err_code!( - SERVER_ERROR, - format!("IP: {}. {}", ip.ip, format!($($fmt_arg)*)), - Status::ServiceUnavailable.code - ) - }; + ($($fmt_arg:tt)*) => {{ + return Err(passkey_login_server_error(&ip.ip, &format!($($fmt_arg)*))); + }}; } // Validate scope and rate-limit the login. AuthMethod::WebAuthn.check_scope(data.scope.as_ref())?; crate::ratelimit::check_limit_login(&ip.ip)?; - // Match the gate the four management endpoints in `src/api/core/mod.rs` + // Match the gate the four management endpoints in `src/api/core/passkeys.rs` // apply. Without it a misconfigured `DOMAIN` (passes the startup `http://` // prefix check, fails `Url::parse().domain()`) panics the `WEBAUTHN` // `LazyLock` on first touch inside `identify_discoverable_authentication` // below. Returning AUTH_FAILED rather than the descriptive admin message // keeps the unauthenticated grant from leaking server-config detail. if !CONFIG.is_webauthn_2fa_supported() { - auth_fail!("IP: {}. Webauthn unsupported on this server.", ip.ip) + preauth_fail!("Webauthn unsupported on this server.") } // Recover and consume (single-use) the saved challenge state. Every @@ -1319,22 +1401,22 @@ async fn webauthn_login(data: ConnectData, user_id: &mut Option, conn: & // condition rather than a passkey-authentication failure. // // Defense-in-depth on the form-extracted fields: the dispatch site - // pre-validates `data.token`/`data.device_response` via `check_is_some`, + // pre-validates `data.token`/`data.device_response` via `check_is_nonempty`, // but a future refactor of that dispatch could omit the pre-check; an // explicit `let Some(...) else err!(AUTH_FAILED, ...)` keeps the // unauthenticated grant from ever panicking the worker on missing // fields regardless of how the function is reached. let Some(token_str) = data.token.as_ref().filter(|token| !token.is_empty()) else { - auth_fail!("IP: {}. Missing passkey login token.", ip.ip) + preauth_fail!("Missing passkey login token.") }; let token = WebAuthnLoginChallengeId::from(token_str.clone()); let saved_challenge = match WebAuthnLoginChallenge::take(&token, conn).await { Ok(Some(c)) => c, - Ok(None) => auth_fail!("IP: {}. Missing or expired passkey login challenge.", ip.ip), + Ok(None) => preauth_fail!("Missing or expired passkey login challenge."), Err(e) => server_fail!("DB error taking passkey login challenge: {e:#?}"), }; let Ok(state) = serde_json::from_str::(&saved_challenge.challenge) else { - auth_fail!("IP: {}. Corrupt passkey login challenge state.", ip.ip) + preauth_fail!("Corrupt passkey login challenge state.") }; // Parse the authenticator assertion. A malformed body must yield the same @@ -1343,10 +1425,10 @@ async fn webauthn_login(data: ConnectData, user_id: &mut Option, conn: & // Same defense-in-depth as the `data.token` check above — refuse a // missing field with AUTH_FAILED rather than .unwrap-panic. let Some(device_response_str) = data.device_response.as_ref().filter(|response| !response.is_empty()) else { - auth_fail!("IP: {}. Missing passkey device_response.", ip.ip) + preauth_fail!("Missing passkey device_response.") }; let Ok(device_response) = serde_json::from_str::(device_response_str) else { - auth_fail!("IP: {}. Malformed passkey assertion.", ip.ip) + preauth_fail!("Malformed passkey assertion.") }; let credential: PublicKeyCredential = device_response.into(); @@ -1358,7 +1440,7 @@ async fn webauthn_login(data: ConnectData, user_id: &mut Option, conn: & // User-scoped event logging is delayed until the assertion is verified. let (user_uuid, cred_id) = match WEBAUTHN.identify_discoverable_authentication(&credential) { Ok((user_uuid, cred_id)) => (UserId::from(user_uuid.to_string()), cred_id), - Err(e) => auth_fail!("IP: {}. Could not identify passkey credential: {e:?}", ip.ip), + Err(e) => preauth_fail!("Could not identify passkey credential: {e:?}"), }; // Use the Result-returning sibling so a DB transient on the user lookup @@ -1368,15 +1450,19 @@ async fn webauthn_login(data: ConnectData, user_id: &mut Option, conn: & // grant — masking a retry-later condition as a missing-user auth failure. let user = match User::try_find_by_uuid(&user_uuid, conn).await { Ok(Some(u)) => u, - Ok(None) => auth_fail!("IP: {}. No user matches passkey user handle {user_uuid}.", ip.ip), + Ok(None) => preauth_fail!("No user matches passkey user handle {user_uuid}."), Err(e) => server_fail!("DB error loading user {user_uuid}: {e:#?}"), }; - // The load-bearing protection against a `User::delete` cascade racing - // this flow is `WebAuthnCredential::update_credential`'s rowcount check: - // a concurrent cascade deletes the credential rows before the counter + // The load-bearing protection against a concurrent `User::delete` cascade + // racing this flow is `WebAuthnCredential::update_credential`'s rowcount + // check: if the cascade deletes the credential rows before the counter // UPDATE, the UPDATE matches 0 rows, the helper returns an Err, and the - // surrounding failure handling refuses the login. + // surrounding failure handling refuses the login. Re-fetching the USER + // row would not detect the credential deletion (FK guarantees credentials + // ⊆ users, but the cascade can have removed credentials while the users + // row is briefly still present), so the rowcount check is the right place + // to detect the race. // Locate the single credential the assertion claims to be for via the // indexed `(user_uuid, credential_id_hash)` UNIQUE index. The cred_id @@ -1398,7 +1484,7 @@ async fn webauthn_login(data: ConnectData, user_id: &mut Option, conn: & // caller who can read the server log harvest UUID->email // mappings via deliberately-malformed assertions. Ok(Some(wac)) => wac, - Ok(None) => auth_fail!("IP: {}. UserUuid: {user_uuid}. No matching passkey credential.", ip.ip), + Ok(None) => preauth_fail!("UserUuid: {user_uuid}. No matching passkey credential."), Err(e) => server_fail!("UserUuid: {user_uuid}. DB error loading passkey credential: {e:#?}"), }; @@ -1406,14 +1492,15 @@ async fn webauthn_login(data: ConnectData, user_id: &mut Option, conn: & // credential only; the user can retry with another passkey, which // routes through its own one-row lookup. `warn!` lets ops find the // bad row immediately rather than via manual DB inspection. - let mut passkey: Passkey = match serde_json::from_str(&matched_wac.credential) { + let previous_credential = matched_wac.credential.clone(); + let mut passkey: Passkey = match serde_json::from_str(&previous_credential) { Ok(p) => p, Err(e) => { warn!( "webauthn_login: failed to deserialize stored Passkey blob for credential {} (user {}): {e:#?}", matched_wac.uuid, matched_wac.user_uuid ); - auth_fail!("IP: {}. UserUuid: {user_uuid}. Corrupt passkey credential blob.", ip.ip) + preauth_fail!("UserUuid: {user_uuid}. Corrupt passkey credential blob.") } }; @@ -1428,7 +1515,7 @@ async fn webauthn_login(data: ConnectData, user_id: &mut Option, conn: & Ok(result) => result, // Still pre-verification: UUID, not email. (See the // load-credentials branch above for rationale.) - Err(e) => auth_fail!("IP: {}. UserUuid: {user_uuid}. WebAuthn verification failed: {e:?}", ip.ip), + Err(e) => preauth_fail!("UserUuid: {user_uuid}. WebAuthn verification failed: {e:?}"), }; // The assertion is now bound to a registered credential for this user. From @@ -1439,10 +1526,10 @@ async fn webauthn_login(data: ConnectData, user_id: &mut Option, conn: & // confirmed the signature matches that credential, so no second lookup is // needed. *user_id = Some(user.uuid.clone()); - let username = user.email.clone(); + let username = user.email.as_str(); if !user.enabled { - auth_fail!("IP: {}. Username: {username}. Account is disabled.", ip.ip) + auth_fail!("Username: {username}. Account is disabled.") } // Reject an unverified account before doing any server-side persistence. @@ -1452,7 +1539,7 @@ async fn webauthn_login(data: ConnectData, user_id: &mut Option, conn: & // login as an oracle for verification state. The descriptive hint still // reaches legitimate users via password login. if user.verified_at.is_none() && CONFIG.mail_enabled() && CONFIG.signups_verify() { - auth_fail!("IP: {}. Username: {username}. Account is not email-verified.", ip.ip) + auth_fail!("Username: {username}. Account is not email-verified.") } // Compute the 2FA-state gate decision BEFORE committing the @@ -1491,14 +1578,17 @@ async fn webauthn_login(data: ConnectData, user_id: &mut Option, conn: & // holder could otherwise fingerprint the victim's 2FA configuration // (rows-exist-but-disabled vs no-rows-at-all) by observing which // branch fires. - auth_fail!("IP: {}. Username: {username}. No enabled and usable two-factor providers configured.", ip.ip) + auth_fail!("Username: {username}. No enabled and usable two-factor providers configured.") } - // Gate passed — now safe to commit the counter advance. Exempt from the - // `update_revision` + `send_user_update(SyncVault, ...)` pair the four - // management endpoints in `src/api/core/mod.rs` emit: the signature - // counter is not part of any sync payload the clients read, so a - // notify here would be a no-op. + // Gate passed — now safe to commit any credential-state change. Persist + // only signature-counter advances: `Passkey::update_credential` also + // reports backup-state-only changes, and writing a stale full blob for + // those can roll back a counter advanced by a parallel replica. Exempt + // from the `update_revision` + `send_user_update(SyncVault, ...)` pair + // the four management endpoints in `src/api/core/passkeys.rs` emit: the + // signature counter is not part of any sync payload the clients read, so + // a notify here would be a no-op. // // Counter semantics note: the persisted counter is webauthn-rs's // clone-detection primitive, not an audit-trail signal. A successful @@ -1507,70 +1597,62 @@ async fn webauthn_login(data: ConnectData, user_id: &mut Option, conn: & // counter advanced without an issued auth token. This is intentional: // the contract is monotonicity, not strict 1:1 with completed logins. // - // A DB transient on this write is converted to a generic 503 rather than - // propagated as `?`. The endpoint is unauthenticated; raw DB wording would - // leak server internals, but users still need a retry-later signal. - if passkey.update_credential(&authentication_result) == Some(true) { + // A DB transient on this write maps to a generic 503 (same + // unauthenticated-endpoint reason as above). + let previous_counter = passkey_counter(&passkey); + let updated_credential = passkey.update_credential(&authentication_result) == Some(true); + let advanced_counter = updated_credential && authentication_result.counter() > previous_counter; + if advanced_counter { matched_wac.credential = match serde_json::to_string(&passkey) { Ok(credential) => credential, Err(e) => server_fail!("Username: {username}. Failed to serialize updated passkey credential: {e:#?}"), }; - if let Err(e) = matched_wac.update_credential(conn).await { - server_fail!("Username: {username}. DB error persisting signature counter: {e:#?}") + if let Err(e) = matched_wac.update_credential(&previous_credential, conn).await { + if is_concurrent_modification(&e) { + return Err(passkey_login_concurrent_modification_error( + &ip.ip, + &format!("Username: {username}. Passkey credential modified concurrently during counter advance."), + )); + } + return Err(passkey_login_server_error( + &ip.ip, + &format!("Username: {username}. DB error persisting signature counter: {e:#?}"), + )); } } else if let Err(e) = matched_wac.ensure_still_registered(conn).await { - server_fail!("Username: {username}. DB error checking passkey credential presence: {e:#?}") + if is_concurrent_modification(&e) { + return Err(passkey_login_concurrent_modification_error( + &ip.ip, + &format!("Username: {username}. Passkey credential deleted concurrently during login."), + )); + } + return Err(passkey_login_server_error( + &ip.ip, + &format!("Username: {username}. DB error checking passkey credential presence: {e:#?}"), + )); } let device_type = connect_device_type(&data); if needs_2fa_policy_enforcement { - // 2FA-state TOCTOU note: `disable_twofactor` / `activate_authenticator` - // / etc. do NOT acquire the per-user passkey lock, so the snapshot we - // captured above can drift before this enforcement runs. Holding the - // lock longer would not close the race (the disable path is - // unsynchronised regardless). The realistic worst case is a single + // 2FA-state TOCTOU note: the snapshot we captured above can drift + // before this enforcement runs. The realistic worst case is a single // user with two concurrent sessions racing a 2FA change against their - // own passkey login — bounded policy drift, not auth bypass. + // own passkey login — bounded policy drift, not auth bypass. Matches + // the single-snapshot pattern in upstream Bitwarden's + // `WebAuthnGrantValidator` and in vaultwarden's password grant: any + // JWT minted against stale state is rejected on the next API call by + // `FromRequest`'s security_stamp check. if let Err(e) = enforce_2fa_policy(&user, &user.uuid, device_type, &ip.ip, conn).await { server_fail!("Username: {username}. 2FA policy enforcement failed: {e:#?}") } } - // Re-read the mutable account/credential state before any login side - // effects (device persistence, notification mail, success log). The - // process-local passkey lock blocks same-node delete/rotation while this - // request is active; this final DB read closes the common multi-replica - // window where another node deleted the matched credential or rotated the - // account key after our initial credential load. - match User::try_find_by_uuid(&user.uuid, conn).await { - Ok(Some(current_user)) => { - if current_user.security_stamp != user.security_stamp { - server_fail!("Username: {username}. Account key changed during passkey login.") - } - if !current_user.enabled { - auth_fail!("IP: {}. Username: {username}. Account is disabled.", ip.ip) - } - if current_user.verified_at.is_none() && CONFIG.mail_enabled() && CONFIG.signups_verify() { - auth_fail!("IP: {}. Username: {username}. Account is not email-verified.", ip.ip) - } - } - Ok(None) => server_fail!("Username: {username}. Account deleted during passkey login."), - Err(e) => server_fail!("Username: {username}. DB error revalidating account state: {e:#?}"), - } - - let response_wac = - match WebAuthnCredential::find_by_user_and_credential_id_hash(&user.uuid, &credential_id_hash, conn).await { - Ok(Some(wac)) => wac, - Ok(None) => server_fail!("Username: {username}. Passkey credential deleted during login."), - Err(e) => server_fail!("Username: {username}. DB error revalidating passkey credential: {e:#?}"), - }; - // Post-verification downstream calls (get_device / authenticated_response) // must not leak raw Error::Db / Error::Smtp / Error::Lettre details on // this unauthenticated grant. Collapse them to a generic 503 instead of // AUTH_FAILED so a valid passkey user gets the correct "server - // unavailable" guidance. + // error" guidance. let mut device = match get_device(&data, conn, &user).await { Ok(d) => d, Err(e) => server_fail!("Username: {username}. Device lookup/create failed: {e:#?}"), @@ -1587,14 +1669,24 @@ async fn webauthn_login(data: ConnectData, user_id: &mut Option, conn: & Err(e) => server_fail!("Username: {username}. Authenticated response build failed: {e:#?}"), }; - Ok(Json(build_webauthn_login_response(base, &response_wac, &passkey))) + // Reuse the already-verified `matched_wac`/`passkey` instead of re-fetching: + // the row is unchanged since the counter write, and only the (immutable) + // cred_id/transports plus the PRF key blobs are read here. A refetch would + // add a SELECT + parse and could 503 an already-authenticated user if the + // credential were concurrently deleted before the response build. + Ok(Json(build_webauthn_login_response(base, &matched_wac, &passkey))) } fn passkey_policy_provider_usable_or_unknown(tf: &TwoFactor) -> bool { + if !tf.enabled { + // A row whose owner explicitly disabled it (regardless of atype) is + // not a usable second factor — symmetric with the password-grant + // `is_twofactor_provider_usable` gate above and prevents a disabled + // unknown-future-atype row from skipping `enforce_2fa_policy`. + return false; + } match TwoFactorType::from_i32(tf.atype) { - Some(provider_type) => { - tf.is_policy_provider() && tf.enabled && is_twofactor_provider_usable(&provider_type, Some(&tf.data)) - } + Some(provider_type) => tf.is_policy_provider() && is_twofactor_provider_usable(&provider_type, Some(&tf.data)), None => tf.is_policy_provider_or_unknown(), } } @@ -1908,13 +2000,17 @@ mod tests { #[test] fn passkey_challenge_fetch_metadata_allows_same_site_same_origin_or_absent() { + let ip = std::net::IpAddr::from([127, 0, 0, 1]); for sec_fetch_site in [None, Some("same-origin".to_owned()), Some("same-site".to_owned()), Some("none".to_owned())] { assert!( - reject_cross_site_passkey_challenge_request(&FetchMetadata { - sec_fetch_site - }) + reject_cross_site_passkey_challenge_request( + &FetchMetadata { + sec_fetch_site + }, + &ip + ) .is_ok() ); } @@ -1922,16 +2018,63 @@ mod tests { #[test] fn passkey_challenge_fetch_metadata_rejects_cross_origin_contexts() { + let ip = std::net::IpAddr::from([127, 0, 0, 1]); for sec_fetch_site in ["cross-site", "Cross-Site", "unexpected-value"] { assert!( - reject_cross_site_passkey_challenge_request(&FetchMetadata { - sec_fetch_site: Some(sec_fetch_site.to_owned()) - }) + reject_cross_site_passkey_challenge_request( + &FetchMetadata { + sec_fetch_site: Some(sec_fetch_site.to_owned()) + }, + &ip + ) .is_err() ); } } + fn make_twofactor(atype: i32, enabled: bool) -> TwoFactor { + let mut tf = TwoFactor::new( + UserId::from(String::from("00000000-0000-0000-0000-000000000000")), + TwoFactorType::Authenticator, + String::new(), + ); + tf.atype = atype; + tf.enabled = enabled; + tf + } + + #[test] + fn passkey_policy_provider_usable_accepts_enabled_real_provider() { + let tf = make_twofactor(TwoFactorType::Authenticator as i32, true); + + assert!(passkey_policy_provider_usable_or_unknown(&tf)); + } + + #[test] + fn passkey_policy_provider_usable_rejects_disabled_real_provider() { + let tf = make_twofactor(TwoFactorType::Authenticator as i32, false); + + assert!(!passkey_policy_provider_usable_or_unknown(&tf)); + } + + #[test] + fn passkey_policy_provider_usable_rejects_recovery_and_implementation_rows() { + let recovery = make_twofactor(TwoFactorType::RecoveryCode as i32, true); + let challenge = make_twofactor(TwoFactorType::WebauthnPasskeyAssertionChallenge as i32, true); + + assert!(!passkey_policy_provider_usable_or_unknown(&recovery)); + assert!(!passkey_policy_provider_usable_or_unknown(&challenge)); + } + + #[test] + fn passkey_policy_provider_usable_treats_enabled_unknown_provider_as_usable() { + let enabled_unknown = make_twofactor(99, true); + let disabled_unknown = make_twofactor(99, false); + + assert!(passkey_policy_provider_usable_or_unknown(&enabled_unknown)); + assert!(!passkey_policy_provider_usable_or_unknown(&disabled_unknown)); + } + fn make_credential( supports_prf: bool, encrypted_user_key: Option<&str>, diff --git a/src/db/models/mod.rs b/src/db/models/mod.rs index f37a6ff8..db92afd0 100644 --- a/src/db/models/mod.rs +++ b/src/db/models/mod.rs @@ -44,6 +44,7 @@ pub use self::two_factor::{TwoFactor, TwoFactorType}; pub use self::two_factor_duo_context::TwoFactorDuoContext; pub use self::two_factor_incomplete::TwoFactorIncomplete; pub use self::user::{Invitation, SsoUser, User, UserId, UserKdfType, UserStampException}; +pub(crate) use self::web_authn_credential::is_concurrent_modification; pub use self::web_authn_credential::{ WebAuthnCredential, WebAuthnCredentialId, WebAuthnLoginChallenge, WebAuthnLoginChallengeId, }; diff --git a/src/db/models/two_factor.rs b/src/db/models/two_factor.rs index 50e60350..3320a0da 100644 --- a/src/db/models/two_factor.rs +++ b/src/db/models/two_factor.rs @@ -145,6 +145,11 @@ impl TwoFactor { } } + /// Consumes `self` and deletes the row by `uuid` only. The simpler form + /// for callers that already own the [`TwoFactor`] and don't need to + /// observe whether anything was actually deleted (e.g., legacy 2FA + /// management flows where the caller looked the row up via + /// `find_by_user_and_type` immediately before). pub async fn delete(self, conn: &DbConn) -> EmptyResult { conn.run(move |conn| { diesel::delete(twofactor::table.filter(twofactor::uuid.eq(self.uuid))) @@ -154,13 +159,29 @@ impl TwoFactor { .await } - /// Load provider rows while preserving DB transients for the - /// unauthenticated `webauthn_login` grant. Authenticated 2FA flows - /// still use the panicking `find_by_user` sibling (see upstream - /// pattern); the unauthenticated grant must distinguish a transient - /// 503 from a 401 to avoid the grant becoming an account-state oracle - /// and to give legitimate users a "retry later" signal instead of - /// AUTH_FAILED. + /// Borrowing-and-rowcount-returning sibling of [`Self::delete`]. + /// Filters by both `(uuid, user_uuid)` for defense-in-depth and returns + /// `Ok(true)` only if a row was actually deleted. Used by the passkey + /// management endpoints' validate-then-delete pattern, which needs the + /// rowcount to detect a multi-replica race where another node consumed + /// the challenge row between this caller's SELECT and DELETE. + pub async fn delete_by_uuid(&self, conn: &DbConn) -> Result { + let uuid = self.uuid.clone(); + let user_uuid = self.user_uuid.clone(); + conn.run(move |conn| { + diesel::delete(twofactor::table.filter(twofactor::uuid.eq(uuid)).filter(twofactor::user_uuid.eq(user_uuid))) + .execute(conn) + .map(|rows| rows > 0) + .map_res("Error deleting twofactor by uuid") + }) + .await + } + + /// Load provider rows while preserving DB transients for auth paths that + /// must distinguish a transient server failure from an empty provider set. + /// The unauthenticated `webauthn_login` grant additionally maps those + /// transients to a generic 503 so a legitimate user gets a retry-later + /// signal instead of AUTH_FAILED. pub async fn try_find_by_user(user_uuid: &UserId, conn: &DbConn) -> Result, Error> { let user_uuid = user_uuid.clone(); conn.run(move |conn| { @@ -184,6 +205,12 @@ impl TwoFactor { .await } + /// Legacy `Option`-typed lookup that collapses DB transients (deadlock, + /// conn drop) and a genuinely absent row into the same `None`. Used by + /// authenticated 2FA-management flows where the caller already owns the + /// user context and a 5xx-vs-401 distinction at this lookup doesn't + /// matter. For new callers — especially anything reachable from an + /// unauthenticated grant — prefer [`Self::try_find_by_user_and_type`]. pub async fn find_by_user_and_type(user_uuid: &UserId, atype: i32, conn: &DbConn) -> Option { conn.run(move |conn| { twofactor::table @@ -195,6 +222,28 @@ impl TwoFactor { .await } + /// `Result`-typed sibling of [`Self::find_by_user_and_type`] that + /// propagates DB transients as `Err(_)` instead of collapsing them into + /// `Ok(None)`. Required for the unauthenticated `webauthn_login` grant + /// (and any other unauthenticated caller) so a deadlock or conn drop can + /// surface as a 5xx retry signal rather than a 4xx "no such row" — the + /// latter would let the grant become an account-state oracle. + pub async fn try_find_by_user_and_type( + user_uuid: &UserId, + atype: i32, + conn: &DbConn, + ) -> Result, Error> { + conn.run(move |conn| { + twofactor::table + .filter(twofactor::user_uuid.eq(user_uuid)) + .filter(twofactor::atype.eq(atype)) + .first::(conn) + .optional() + .map_res("Error loading twofactor by type") + }) + .await + } + /// 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. diff --git a/src/db/models/user.rs b/src/db/models/user.rs index 3d164ec1..8c6e27d0 100644 --- a/src/db/models/user.rs +++ b/src/db/models/user.rs @@ -352,6 +352,7 @@ impl User { }) .await; delete_result?; + Ok(()) } diff --git a/src/db/models/web_authn_credential.rs b/src/db/models/web_authn_credential.rs index e9c27d03..b43db2be 100644 --- a/src/db/models/web_authn_credential.rs +++ b/src/db/models/web_authn_credential.rs @@ -15,6 +15,18 @@ use super::UserId; const WEBAUTHN_LOGIN_CHALLENGE_TTL_SECONDS: i64 = 300; const WEBAUTHN_LOGIN_CHALLENGE_CLOCK_SKEW_SECONDS: i64 = 30; +/// Sentinel message produced by [`WebAuthnCredential::ensure_credential_present`] +/// when an UPDATE/DELETE matched zero rows. External callers detect this via +/// [`is_concurrent_modification`] rather than touching the string directly — +/// the constant is private to keep the predicate as the single API surface. +const WEBAUTHN_CREDENTIAL_MODIFIED_CONCURRENTLY: &str = "Webauthn credential modified concurrently"; + +/// Predicate for the message produced by [`WebAuthnCredential::ensure_credential_present`]. +/// See the sentinel constant above for the use case. +pub(crate) fn is_concurrent_modification(err: &Error) -> bool { + err.message() == WEBAUTHN_CREDENTIAL_MODIFIED_CONCURRENTLY +} + #[derive(Clone, Debug, Identifiable, Queryable, Insertable)] #[diesel(table_name = web_authn_credentials)] #[diesel(primary_key(uuid))] @@ -119,21 +131,14 @@ impl WebAuthnCredential { .map_err(|_| Error::new("Error counting web_authn_credentials", "Credential count overflow")) } - /// Guard the rowcount returned by an UPDATE against a concurrent - /// credential delete. A 0-row result means a `post_api_webauthn_delete` - /// (or `User::delete` cascade) removed the row inside our window — a - /// concurrent-state outcome that deserves a clean refusal. - /// - /// Returns a Simple error so the routine concurrent-delete race does NOT - /// emit an `error!()` log line from the Rocket responder. The message - /// itself describes an expected concurrent-state outcome, not a server - /// bug, and a multi-replica deployment would otherwise spam operator logs. - /// Visible to sibling modules so credential rewrap paths can share the - /// same rowcount-zero handling rather than re-implementing it for every - /// update statement. + /// Guard an UPDATE/DELETE rowcount against a concurrent credential + /// mutation: 0 rows means a peer deleted the row or a CAS guard rejected a + /// stale counter write. Returns a `Simple` error (not `Db(NotFound)`) so + /// the Rocket responder does not log this routine race at ERROR level. + /// `pub(crate)` so sibling rewrap paths share one rowcount-zero handling. pub(crate) fn ensure_credential_present(rows: usize) -> EmptyResult { if rows == 0 { - return Err(Error::new_msg("Webauthn credential modified concurrently")); + return Err(Error::new_msg(WEBAUTHN_CREDENTIAL_MODIFIED_CONCURRENTLY)); } Ok(()) } @@ -141,9 +146,21 @@ impl WebAuthnCredential { /// 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`]). - pub async fn update_credential(&self, conn: &DbConn) -> EmptyResult { + /// + /// `previous_credential` is the exact blob this request verified against. + /// Including it in the UPDATE predicate prevents a stale replica from writing + /// counter=12 over another replica's already-committed counter=15. The + /// WebAuthn counter lives inside the serialized `Passkey`, so an exact-blob + /// compare-and-swap is the narrowest portable guard across sqlite/mysql/pg. + pub async fn update_credential(&self, previous_credential: &str, conn: &DbConn) -> EmptyResult { + let previous_credential = previous_credential.to_owned(); db_run! { conn: { - let rows: usize = diesel::update(web_authn_credentials::table.filter(web_authn_credentials::uuid.eq(&self.uuid))) + let rows: usize = diesel::update( + web_authn_credentials::table + .filter(web_authn_credentials::uuid.eq(&self.uuid)) + .filter(web_authn_credentials::user_uuid.eq(&self.user_uuid)) + .filter(web_authn_credentials::credential.eq(previous_credential)), + ) .set(web_authn_credentials::credential.eq(&self.credential)) .execute(conn) .map_res("Error updating web_authn_credential signature counter")?; @@ -159,7 +176,11 @@ impl WebAuthnCredential { /// inside our window) so callers can treat the rewrap as a degraded no-op. pub async fn update_keys(&self, conn: &DbConn) -> EmptyResult { db_run! { conn: { - let rows: usize = diesel::update(web_authn_credentials::table.filter(web_authn_credentials::uuid.eq(&self.uuid))) + let rows: usize = diesel::update( + web_authn_credentials::table + .filter(web_authn_credentials::uuid.eq(&self.uuid)) + .filter(web_authn_credentials::user_uuid.eq(&self.user_uuid)), + ) .set(( web_authn_credentials::encrypted_user_key.eq(&self.encrypted_user_key), web_authn_credentials::encrypted_public_key.eq(&self.encrypted_public_key), @@ -170,46 +191,62 @@ impl WebAuthnCredential { }} } - /// Drop all PRF unlock blobs so clients stop advertising unlock-with-passkey - /// for a credential whose key material could not be rewrapped after account - /// key rotation. - pub async fn clear_prf_keyset(&self, conn: &DbConn) -> EmptyResult { + /// Drop the account-key-wrapped PRF unlock blobs so clients stop + /// advertising unlock-with-passkey for a credential whose key material + /// could not be rewrapped after account key rotation. The passkey-wrapped + /// private key is left intact because `update_keys` has not modified it. + pub async fn clear_account_key_prf_keyset(&self, conn: &DbConn) -> EmptyResult { db_run! { conn: { - let rows: usize = diesel::update(web_authn_credentials::table.filter(web_authn_credentials::uuid.eq(&self.uuid))) + let rows: usize = diesel::update( + web_authn_credentials::table + .filter(web_authn_credentials::uuid.eq(&self.uuid)) + .filter(web_authn_credentials::user_uuid.eq(&self.user_uuid)), + ) .set(( web_authn_credentials::encrypted_user_key.eq::>(None), web_authn_credentials::encrypted_public_key.eq::>(None), - web_authn_credentials::encrypted_private_key.eq::>(None), )) .execute(conn) - .map_res("Error clearing web_authn_credential PRF keyset")?; + .map_res("Error clearing web_authn_credential account-key PRF keyset")?; Self::ensure_credential_present(rows) }} } - /// Persist a complete PRF unlock keyset after a user enables vault - /// encryption for an existing passkey-login credential, optionally - /// folding the signature-counter advance from the assertion that - /// authorised the enrolment into the same UPDATE — both the keyset - /// and the counter are written in one statement so a half-applied - /// state is impossible without involving a separate transaction. + /// Persist a complete PRF unlock keyset when a user enables vault + /// encryption for an existing passkey-login credential, optionally folding + /// the assertion's signature-counter advance into the same UPDATE so a + /// half-applied state is impossible. /// - /// `advanced_counter` gates the `credential` blob write. The caller - /// passes `true` only when `Passkey::update_credential` reported a real - /// counter advance; otherwise the column is left untouched so a - /// concurrent counter advance committed by another instance (e.g. a - /// parallel `webauthn_login` in a multi-replica deployment) is not - /// silently overwritten with the stale blob loaded here. - /// - /// A 0-rows result is surfaced as a `Simple` error (NOT `Db(NotFound)`) - /// via [`Self::ensure_credential_present`] so the renderer at - /// `error.rs` does not log a routine concurrent-delete race at ERROR - /// level. - pub async fn update_credential_and_prf_keyset(&self, advanced_counter: bool, conn: &DbConn) -> EmptyResult { + /// `advanced_counter` gates the `credential` blob write; when true, + /// `previous_credential` must be the exact blob this request verified + /// against so the UPDATE rejects stale cross-replica counter writes. + /// Backup-state-only changes are not persisted: writing a stale blob for a + /// backup-state toggle could roll back a counter advanced by a peer replica, + /// and counter monotonicity matters more than that metadata bit. + pub async fn update_credential_and_prf_keyset( + &self, + advanced_counter: bool, + previous_credential: Option<&str>, + conn: &DbConn, + ) -> EmptyResult { + let previous_credential = if advanced_counter { + Some( + previous_credential + .ok_or_else(|| Error::new_msg("Previous WebAuthn credential blob is required for counter updates"))? + .to_owned(), + ) + } else { + None + }; + db_run! { conn: { - let target = web_authn_credentials::table.filter(web_authn_credentials::uuid.eq(&self.uuid)); - let rows: usize = if advanced_counter { - diesel::update(target) + let target = || { + web_authn_credentials::table + .filter(web_authn_credentials::uuid.eq(&self.uuid)) + .filter(web_authn_credentials::user_uuid.eq(&self.user_uuid)) + }; + let rows: usize = if let Some(previous_credential) = previous_credential { + diesel::update(target().filter(web_authn_credentials::credential.eq(previous_credential))) .set(( web_authn_credentials::credential.eq(&self.credential), web_authn_credentials::encrypted_user_key.eq(&self.encrypted_user_key), @@ -219,7 +256,7 @@ impl WebAuthnCredential { .execute(conn) .map_res("Error updating web_authn_credential PRF keyset")? } else { - diesel::update(target) + diesel::update(target()) .set(( web_authn_credentials::encrypted_user_key.eq(&self.encrypted_user_key), web_authn_credentials::encrypted_public_key.eq(&self.encrypted_public_key), @@ -278,24 +315,33 @@ impl WebAuthnCredential { .count() .get_result(conn) .map_res("Error checking web_authn_credential presence")?; + // `uuid` is the PRIMARY KEY, so this filter caps the count at 0 or 1, + // and collapsing `rows > 0` into a 0-or-1 usize satisfies + // `ensure_credential_present`'s UPDATE-rowcount contract. Do NOT copy + // this idiom to a filter on non-unique columns: collapsing COUNT > 1 + // to 1 would mask multi-row anomalies. Self::ensure_credential_present(usize::from(rows > 0)) }} } + /// Idempotent: a request to delete a uuid that's already gone (double-click, + /// concurrent tab, stale client list) returns `Ok(false)` rather than a + /// "modified concurrently" error. The rowcount check belongs only on UPDATE + /// paths where zero rows is the genuine "raced with a cascade" signal. pub async fn delete_by_uuid_and_user( uuid: &WebAuthnCredentialId, user_uuid: &UserId, conn: &DbConn, - ) -> EmptyResult { + ) -> Result { db_run! { conn: { - let rows = diesel::delete( + let rows: usize = diesel::delete( web_authn_credentials::table .filter(web_authn_credentials::uuid.eq(uuid)) .filter(web_authn_credentials::user_uuid.eq(user_uuid)), ) .execute(conn) .map_res("Error removing web_authn_credential")?; - Self::ensure_credential_present(rows) + Ok(rows > 0) }} } @@ -533,6 +579,26 @@ mod tests { assert!(WebAuthnCredential::ensure_credential_present(0).is_err()); } + /// Pins the contract between [`WebAuthnCredential::ensure_credential_present`]'s + /// 0-row error and [`is_concurrent_modification`]. Without this assertion a + /// future refactor that adds `.with_msg(...)` wrapping, replaces + /// `Error::new_msg` with `err!()` (which prepends the user message), or + /// inserts a `.map_res(...)` layer would silently degrade the predicate to + /// always-false — observers (accounts.rs, identity.rs, passkeys.rs) would + /// then fall through to ERROR-level logging on a benign CM race with no + /// other CI signal. + #[test] + fn concurrent_modification_predicate_matches_ensure_credential_present() { + let err = WebAuthnCredential::ensure_credential_present(0).expect_err("0 rows must error"); + assert!( + is_concurrent_modification(&err), + "predicate must match the sentinel ensure_credential_present produces" + ); + + let other = Error::new_msg("Some other failure"); + assert!(!is_concurrent_modification(&other), "predicate must NOT match unrelated errors"); + } + /// Exhaust the 2^4 truth table for `has_prf_keyset` and `prf_status`: /// only `(supports_prf=true, all three blobs Some)` reports /// `has_prf_keyset() == true` / `prf_status() == 0` (Enabled). Any