From 83fd293e01b19f7d3fcbb0a60022ef45b8d8ed3d Mon Sep 17 00:00:00 2001 From: tom27052006 <83423411+tom27052006@users.noreply.github.com> Date: Fri, 31 Jul 2026 18:30:09 +0200 Subject: [PATCH 01/10] Add trusted device encryption for SSO logins Lets a user unlock their vault after an SSO login with a key stored on a trusted device instead of a master password, what Bitwarden calls trusted device encryption. Off by default, enabled with `SSO_TRUSTED_DEVICE_ENCRYPTION`, which requires `SSO_ENABLED`. The client generates a key pair plus a device key that never leaves the device and stores three blobs on the device row: the user key wrapped for the device public key, that public key wrapped with the user key, and the device private key wrapped with the device key. The server only keeps and returns them, so a stored trust is worthless without the device itself. A device counts as trusted only while all three are present. Endpoints, matching bitwarden/server's DevicesController: PUT|POST /devices//keys trust a device POST /devices//retrieve-keys the public halves, for rotation POST /devices/update-trust re-wrap after a key rotation POST /devices/untrust drop the trust of some devices POST /devices/lost-trust client reports a drifted trust `GET /devices`, `GET /devices/identifier/` and the login response now report the real trust state instead of a hard-coded `isTrusted: false`. An SSO login response carries `UserDecryptionOptions.TrustedDeviceOption`, which is what makes the clients offer the flow at all. Upstream ties this to the SSO configuration of an organization; SSO is configured for the whole server here, so the setting decides it, and members of no organization can use it too. A password login never gets these options. When the setting is turned off again, devices that are still trusted keep receiving their keys with `IsTdeOffboarding` set, so their owner can still unlock and set a master password instead of being locked out. `HasAdminApproval` and `HasManageResetPasswordPermission` are always false: approval of a new device by an organization admin is not implemented, and announcing it would leave the client waiting for a request nobody can answer. A new device is unlocked by approving it from an already trusted device or with the master password. Two adjacent fixes this depends on: - `POST /accounts/keys` refused to notice that an account already has a key pair and would happily replace it, which makes every existing cipher undecryptable. It now only accepts keys for an account that has none, and tolerates a repeat of the same keys. The trusted device flow is what makes this reachable in practice: a client that misjudges an existing account as new posts a fresh key pair here. - `POST /accounts/set-password` keyed "account already initialized" off the key pair, so an account created without a master password could never gain one. It now keys off the master password itself and refuses to replace an existing key pair, which is what the check was guarding. Rotating the account keys leaves every device holding a wrapped copy of the previous user key. Those are dropped; the rotating device keeps its own private key so its client can re-wrap the new user key right away via `/devices/update-trust`. A client that skips that call ends up with no trusted device rather than a broken unlock. Covers dani-garcia/vaultwarden#7034. --- .env.template | 8 + .../down.sql | 3 + .../up.sql | 3 + .../down.sql | 3 + .../up.sql | 3 + .../down.sql | 3 + .../up.sql | 3 + src/api/core/accounts.rs | 254 +++++++++++++++++- src/api/core/sends.rs | 3 + src/api/identity.rs | 62 ++++- src/config.rs | 4 + src/db/models/device.rs | 187 ++++++++++++- src/db/schema.rs | 3 + 13 files changed, 523 insertions(+), 16 deletions(-) create mode 100644 migrations/mysql/2026-07-31-120000_add_device_trusted_encryption/down.sql create mode 100644 migrations/mysql/2026-07-31-120000_add_device_trusted_encryption/up.sql create mode 100644 migrations/postgresql/2026-07-31-120000_add_device_trusted_encryption/down.sql create mode 100644 migrations/postgresql/2026-07-31-120000_add_device_trusted_encryption/up.sql create mode 100644 migrations/sqlite/2026-07-31-120000_add_device_trusted_encryption/down.sql create mode 100644 migrations/sqlite/2026-07-31-120000_add_device_trusted_encryption/up.sql diff --git a/.env.template b/.env.template index fd7c2fd2..375a99e5 100644 --- a/.env.template +++ b/.env.template @@ -549,6 +549,14 @@ ## Log all the tokens, LOG_LEVEL=debug is required # SSO_DEBUG_TOKENS=false +## Trusted device encryption ("passwordless SSO"), see https://bitwarden.com/help/login-with-sso-trusted-devices/ +## After an SSO login the client may keep a copy of the user key on the device, wrapped for a key +## pair that the device generated, so the vault unlocks without a master password. A second device +## is unlocked either by approving it from an already trusted one or with the master password. +## WARNING: a user who never sets a master password and then loses every trusted device cannot +## recover their vault. This server does not implement the admin approval flow to fall back on. +# SSO_TRUSTED_DEVICE_ENCRYPTION=false + ######################## ### MFA/2FA settings ### ######################## diff --git a/migrations/mysql/2026-07-31-120000_add_device_trusted_encryption/down.sql b/migrations/mysql/2026-07-31-120000_add_device_trusted_encryption/down.sql new file mode 100644 index 00000000..7bd8c602 --- /dev/null +++ b/migrations/mysql/2026-07-31-120000_add_device_trusted_encryption/down.sql @@ -0,0 +1,3 @@ +ALTER TABLE devices DROP COLUMN encrypted_private_key; +ALTER TABLE devices DROP COLUMN encrypted_public_key; +ALTER TABLE devices DROP COLUMN encrypted_user_key; diff --git a/migrations/mysql/2026-07-31-120000_add_device_trusted_encryption/up.sql b/migrations/mysql/2026-07-31-120000_add_device_trusted_encryption/up.sql new file mode 100644 index 00000000..33ef1168 --- /dev/null +++ b/migrations/mysql/2026-07-31-120000_add_device_trusted_encryption/up.sql @@ -0,0 +1,3 @@ +ALTER TABLE devices ADD COLUMN encrypted_user_key TEXT; +ALTER TABLE devices ADD COLUMN encrypted_public_key TEXT; +ALTER TABLE devices ADD COLUMN encrypted_private_key TEXT; diff --git a/migrations/postgresql/2026-07-31-120000_add_device_trusted_encryption/down.sql b/migrations/postgresql/2026-07-31-120000_add_device_trusted_encryption/down.sql new file mode 100644 index 00000000..7bd8c602 --- /dev/null +++ b/migrations/postgresql/2026-07-31-120000_add_device_trusted_encryption/down.sql @@ -0,0 +1,3 @@ +ALTER TABLE devices DROP COLUMN encrypted_private_key; +ALTER TABLE devices DROP COLUMN encrypted_public_key; +ALTER TABLE devices DROP COLUMN encrypted_user_key; diff --git a/migrations/postgresql/2026-07-31-120000_add_device_trusted_encryption/up.sql b/migrations/postgresql/2026-07-31-120000_add_device_trusted_encryption/up.sql new file mode 100644 index 00000000..33ef1168 --- /dev/null +++ b/migrations/postgresql/2026-07-31-120000_add_device_trusted_encryption/up.sql @@ -0,0 +1,3 @@ +ALTER TABLE devices ADD COLUMN encrypted_user_key TEXT; +ALTER TABLE devices ADD COLUMN encrypted_public_key TEXT; +ALTER TABLE devices ADD COLUMN encrypted_private_key TEXT; diff --git a/migrations/sqlite/2026-07-31-120000_add_device_trusted_encryption/down.sql b/migrations/sqlite/2026-07-31-120000_add_device_trusted_encryption/down.sql new file mode 100644 index 00000000..7bd8c602 --- /dev/null +++ b/migrations/sqlite/2026-07-31-120000_add_device_trusted_encryption/down.sql @@ -0,0 +1,3 @@ +ALTER TABLE devices DROP COLUMN encrypted_private_key; +ALTER TABLE devices DROP COLUMN encrypted_public_key; +ALTER TABLE devices DROP COLUMN encrypted_user_key; diff --git a/migrations/sqlite/2026-07-31-120000_add_device_trusted_encryption/up.sql b/migrations/sqlite/2026-07-31-120000_add_device_trusted_encryption/up.sql new file mode 100644 index 00000000..33ef1168 --- /dev/null +++ b/migrations/sqlite/2026-07-31-120000_add_device_trusted_encryption/up.sql @@ -0,0 +1,3 @@ +ALTER TABLE devices ADD COLUMN encrypted_user_key TEXT; +ALTER TABLE devices ADD COLUMN encrypted_public_key TEXT; +ALTER TABLE devices ADD COLUMN encrypted_private_key TEXT; diff --git a/src/api/core/accounts.rs b/src/api/core/accounts.rs index 0cb4d3c0..81a0ceae 100644 --- a/src/api/core/accounts.rs +++ b/src/api/core/accounts.rs @@ -1,4 +1,4 @@ -use std::collections::HashSet; +use std::collections::{HashMap, HashSet}; use chrono::Utc; use rocket::{ @@ -68,6 +68,12 @@ pub fn routes() -> Vec { put_device_token, put_clear_device_token, post_clear_device_token, + put_device_keys, + post_device_keys, + post_device_retrieve_keys, + post_devices_update_trust, + post_devices_untrust, + post_devices_lost_trust, get_tasks, post_auth_request, get_auth_request, @@ -440,8 +446,11 @@ async fn post_set_password(data: Json, headers: Headers, conn: let data: SetPasswordData = data.into_inner(); let mut user = headers.user; - if user.private_key.is_some() { - err!("Account already initialized, cannot set password") + // A trusted device account already has its key pair but no master password, and must still be + // able to add one later, for instance once the server stops offering trusted device encryption. + // What this must never do is hand out a fresh master password for an account that has one. + if !user.password_hash.is_empty() { + err!("Account already has a master password") } // Check against the password hint setting here so if it fails, @@ -449,6 +458,19 @@ async fn post_set_password(data: Json, headers: Headers, conn: let password_hint = clean_password_hint(data.master_password_hint.as_ref()); enforce_password_hint_setting(password_hint.as_ref())?; + // Same reasoning as in `post_keys`: the existing ciphers are encrypted under the existing key + // pair, so an account that has one only gets a password, never new keys. + let keys = match (data.keys, user.private_key.is_some() || user.public_key.is_some()) { + (Some(keys), false) => Some(keys), + (Some(keys), true) + if user.private_key.as_ref() != Some(&keys.encrypted_private_key) + || user.public_key.as_ref() != Some(&keys.public_key) => + { + err!("Account already initialized, cannot replace the account keys") + } + _ => None, + }; + set_kdf_data(&mut user, &data.kdf)?; user.set_password( @@ -461,7 +483,7 @@ async fn post_set_password(data: Json, headers: Headers, conn: .await?; user.password_hint = password_hint; - if let Some(keys) = data.keys { + if let Some(keys) = keys { user.private_key = Some(keys.encrypted_private_key); user.public_key = Some(keys.public_key); } @@ -579,6 +601,25 @@ async fn post_keys(data: Json, headers: Headers, conn: DbConn) -> Json let mut user = headers.user; + // Replacing the key pair of an initialized account would make every existing cipher + // undecryptable, so only accept it while the account has none yet. The clients call this during + // account creation, including the trusted device flow, where a stale client state could + // otherwise send us here for an account that is already set up. Repeating the same keys stays + // allowed so a retried request does not fail. Mirrors the guard in `post_set_password`. + if user.private_key.is_some() || user.public_key.is_some() { + if user.private_key.as_ref() != Some(&data.encrypted_private_key) + || user.public_key.as_ref() != Some(&data.public_key) + { + err!("Account already initialized, cannot replace the account keys") + } + + return Ok(Json(json!({ + "privateKey": user.private_key, + "publicKey": user.public_key, + "object":"keys" + }))); + } + user.private_key = Some(data.encrypted_private_key); user.public_key = Some(data.public_key); @@ -994,6 +1035,12 @@ async fn post_rotatekey(data: Json, headers: Headers, conn: DbConn, nt: let save_result = user.save(&conn).await; + // Every trusted device holds the previous user key wrapped for itself, which unlocks nothing + // anymore. The client of the rotating device re-wraps the new one right after this via + // `POST /devices/update-trust`, so that device keeps its private key; the rest is dropped. A + // client that skips that call simply ends up with no trusted device instead of a broken unlock. + Device::invalidate_wrapped_user_keys(&user.uuid, &headers.device.uuid, &conn).await?; + // 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. @@ -1545,6 +1592,205 @@ async fn post_clear_device_token(device_id: DeviceId, ip: ClientIp, conn: DbConn put_clear_device_token(device_id, ip, conn).await } +// Trusted device encryption, see https://bitwarden.com/help/login-with-sso-trusted-devices/ +// The three key blobs below are generated and encrypted by the client, the server only stores them +// and hands them back on the next login of that same device. It never learns the device key that +// unwraps `encrypted_private_key`, so a stored trust is worth nothing without the device itself. +// https://github.com/bitwarden/server/blob/main/src/Api/Controllers/DevicesController.cs + +#[derive(Debug, Deserialize)] +#[serde(rename_all = "camelCase")] +struct TrustedDeviceKeysData { + encrypted_user_key: String, + encrypted_public_key: String, + encrypted_private_key: String, +} + +/// Marks a device of the current user as trusted. +/// +/// Upstream keys this on the device identifier and does not require it to be the device the request +/// was authenticated with, so neither do we. The keys only ever unlock the vault on the device that +/// holds the matching device key, so writing them for another of your own devices gains nothing. +#[put("/devices//keys", data = "")] +async fn put_device_keys( + device_id: DeviceId, + data: Json, + headers: Headers, + conn: DbConn, +) -> JsonResult { + let data = data.into_inner(); + + if data.encrypted_user_key.is_empty() + || data.encrypted_public_key.is_empty() + || data.encrypted_private_key.is_empty() + { + err!("All three device keys are required to trust a device") + } + + let Some(mut device) = Device::find_by_uuid_and_user(&device_id, &headers.user.uuid, &conn).await else { + err!("No device found") + }; + + device.encrypted_user_key = Some(data.encrypted_user_key); + device.encrypted_public_key = Some(data.encrypted_public_key); + device.encrypted_private_key = Some(data.encrypted_private_key); + device.save(true, &conn).await?; + + Ok(Json(device.to_json())) +} + +// Deprecated upstream in favour of the PUT variant, but still served for older clients +#[post("/devices//keys", data = "")] +async fn post_device_keys( + device_id: DeviceId, + data: Json, + headers: Headers, + conn: DbConn, +) -> JsonResult { + put_device_keys(device_id, data, headers, conn).await +} + +/// The public half of a device's trust, needed by the clients to re-wrap the user key for every +/// trusted device during a key rotation. +#[post("/devices//retrieve-keys")] +async fn post_device_retrieve_keys(device_id: DeviceId, headers: Headers, conn: DbConn) -> JsonResult { + let Some(device) = Device::find_by_uuid_and_user(&device_id, &headers.user.uuid, &conn).await else { + err!("No device found") + }; + + Ok(Json(device.to_protected_json())) +} + +#[derive(Debug, Deserialize)] +#[serde(rename_all = "camelCase")] +struct DeviceTrustUpdateData { + encrypted_user_key: String, + encrypted_public_key: String, +} + +#[derive(Debug, Deserialize)] +#[serde(rename_all = "camelCase")] +struct OtherDeviceTrustUpdateData { + device_id: DeviceId, + #[serde(flatten)] + keys: DeviceTrustUpdateData, +} + +#[derive(Deserialize)] +#[serde(rename_all = "camelCase")] +struct UpdateDevicesTrustData { + #[serde(flatten)] + secret: PasswordOrOtpData, + current_device: DeviceTrustUpdateData, + #[serde(default)] + other_devices: Vec, +} + +/// Re-wraps the user key for the trusted devices after it was replaced by a key rotation. +/// +/// Every trusted device that is not listed loses its trust: its stored copy of the user key is the +/// old one and would no longer unlock anything. +#[post("/devices/update-trust", data = "")] +async fn post_devices_update_trust(data: Json, headers: Headers, conn: DbConn) -> EmptyResult { + let data = data.into_inner(); + + data.secret.validate(&headers.user, true, &conn).await?; + + if data.current_device.encrypted_user_key.is_empty() || data.current_device.encrypted_public_key.is_empty() { + err!("The keys of the current device are required") + } + + let mut updates: HashMap = HashMap::new(); + for other in data.other_devices { + if other.device_id == headers.device.uuid { + err!("The current device cannot also be part of the optional rotation") + } + if other.keys.encrypted_user_key.is_empty() || other.keys.encrypted_public_key.is_empty() { + err!("Both keys are required for every device in the rotation") + } + if updates.insert(other.device_id, other.keys).is_some() { + err!("A device was listed more than once in the rotation") + } + } + + let devices = Device::find_by_user(&headers.user.uuid, &conn).await; + if !devices.iter().any(|device| device.uuid == headers.device.uuid) { + err!("No device found") + } + + // Validate everything before writing anything: a rotation that stops halfway would leave the + // devices wrapping a mix of the old and the new user key. + if let Some(unknown) = updates.keys().find(|device_id| !devices.iter().any(|device| device.uuid == **device_id)) { + err!(format!("Device {unknown} does not belong to this user")) + } + + for mut device in devices { + if device.uuid == headers.device.uuid { + device.encrypted_user_key = Some(data.current_device.encrypted_user_key.clone()); + device.encrypted_public_key = Some(data.current_device.encrypted_public_key.clone()); + } else if !device.is_trusted() { + // Nothing to rotate, and handing an untrusted device two of the three keys would not + // make it trusted anyway. + continue; + } else if let Some(keys) = updates.remove(&device.uuid) { + device.encrypted_user_key = Some(keys.encrypted_user_key); + device.encrypted_public_key = Some(keys.encrypted_public_key); + } else { + device.untrust(); + } + + device.save(true, &conn).await?; + } + + Ok(()) +} + +#[derive(Debug, Deserialize)] +#[serde(rename_all = "camelCase")] +struct UntrustDevicesData { + devices: Vec, +} + +#[post("/devices/untrust", data = "")] +async fn post_devices_untrust(data: Json, headers: Headers, conn: DbConn) -> EmptyResult { + let data = data.into_inner(); + + let mut devices = Device::find_by_user(&headers.user.uuid, &conn).await; + + // Check that the user owns all of them first, so a single foreign id does not leave the request + // half applied. + if let Some(unknown) = + data.devices.iter().find(|device_id| !devices.iter().any(|device| &device.uuid == *device_id)) + { + err!(format!("Device {unknown} does not belong to this user")) + } + + for device in devices.iter_mut().filter(|device| data.devices.contains(&device.uuid)) { + device.untrust(); + device.save(true, &conn).await?; + } + + Ok(()) +} + +/// Reported by a client that still holds a device key but did not get any keys back from us. +/// +/// There is nothing left to clean up at this point, the device already counts as untrusted here. +/// Upstream only writes a log line as well, since this points at the client and the server having +/// drifted apart. +#[expect(clippy::needless_pass_by_value, reason = "Not beneficial for Headers")] +#[post("/devices/lost-trust")] +fn post_devices_lost_trust(headers: Headers) -> EmptyResult { + warn!( + "Device {} ({}) of user {} still holds a device key, but has no trusted device keys on the server", + headers.device.uuid, + DeviceType::from_i32(headers.device.atype), + headers.user.uuid + ); + + Ok(()) +} + #[get("/tasks")] fn get_tasks(_client_headers: ClientHeaders) -> JsonResult { Ok(Json(json!({ diff --git a/src/api/core/sends.rs b/src/api/core/sends.rs index 042ce95b..d8ac5ac5 100644 --- a/src/api/core/sends.rs +++ b/src/api/core/sends.rs @@ -35,6 +35,9 @@ static ANON_PUSH_DEVICE: LazyLock = LazyLock::new(|| { push_token: None, refresh_token: String::new(), twofactor_remember: None, + encrypted_user_key: None, + encrypted_public_key: None, + encrypted_private_key: None, } }); diff --git a/src/api/identity.rs b/src/api/identity.rs index 9212ed8d..a25c83b3 100644 --- a/src/api/identity.rs +++ b/src/api/identity.rs @@ -30,7 +30,7 @@ use crate::{ db::{ DbConn, models::{ - AuthRequest, AuthRequestId, Device, DeviceId, EventType, Invitation, OIDCCodeResponseError, + AuthRequest, AuthRequestId, Device, DeviceId, DeviceType, EventType, Invitation, OIDCCodeResponseError, OrganizationApiKey, OrganizationId, SendId, SsoAuth, SsoUser, TwoFactor, TwoFactorIncomplete, TwoFactorType, User, UserId, }, @@ -356,7 +356,7 @@ async fn sso_login( // We passed 2FA get auth tokens let auth_tokens = sso::redeem(&device, &user, data.client_id, sso_user, sso_auth, user_infos, conn).await?; - authenticated_response(&user, &mut device, auth_tokens, twofactor_token, conn, ip).await + authenticated_response(&user, &mut device, auth_tokens, twofactor_token, true, conn, ip).await } async fn password_login( @@ -478,7 +478,46 @@ async fn password_login( let auth_tokens = auth::AuthTokens::new(&device, &user, AuthMethod::Password, data.client_id); - authenticated_response(&user, &mut device, auth_tokens, twofactor_token, conn, ip).await + authenticated_response(&user, &mut device, auth_tokens, twofactor_token, false, conn, ip).await +} + +/// Trusted device encryption ("passwordless SSO"): instead of deriving the user key from a master +/// password, the client keeps a copy of it on the device, wrapped for a key pair that the device +/// generated. Its presence in the response is what makes the clients offer the flow at all. +/// +/// Upstream ties this to the SSO configuration of an organization; Vaultwarden configures SSO for +/// the whole server, so `SSO_TRUSTED_DEVICE_ENCRYPTION` decides it here. Either way it stays an SSO +/// feature, a password login never gets these options. +/// https://github.com/bitwarden/server/blob/main/src/Identity/IdentityServer/UserDecryptionOptionsBuilder.cs +async fn trusted_device_option(user: &User, device: &Device, conn: &DbConn) -> Option { + let enabled = CONFIG.sso_trusted_device_encryption(); + + // Once the feature is switched off again, a user without a master password would be locked out + // of their own vault. Keep telling their still trusted devices about it so their client can walk + // them through setting one while they can still unlock. + let offboarding = !enabled && device.is_trusted() && user.password_hash.is_empty(); + if !enabled && !offboarding { + return None; + } + + // Any other device of this user that could show an approval prompt. The user unlocks a new + // device from one of these, or with the master password if they have one. + let has_login_approving_device = Device::find_by_user(&user.uuid, conn) + .await + .iter() + .any(|other| other.uuid != device.uuid && DeviceType::from_i32(other.atype).can_approve_login_requests()); + + // Approval by an organization admin is not implemented. Announcing it would leave the client + // waiting on a request that nobody here can answer. + Some(json!({ + "HasAdminApproval": false, + "HasLoginApprovingDevice": has_login_approving_device, + "HasManageResetPasswordPermission": false, + "IsTdeOffboarding": offboarding, + "EncryptedPrivateKey": device.trusted_private_key(), + "EncryptedUserKey": device.trusted_user_key(), + "Object": "trustedDeviceUserDecryptionOption" + })) } async fn authenticated_response( @@ -486,6 +525,7 @@ async fn authenticated_response( device: &mut Device, auth_tokens: auth::AuthTokens, twofactor_token: Option, + sso_login: bool, conn: &DbConn, ip: &ClientIp, ) -> JsonResult { @@ -547,6 +587,16 @@ async fn authenticated_response( Value::Null }; + let mut user_decryption_options = json!({ + "HasMasterPassword": has_master_password, + "MasterPasswordUnlock": master_password_unlock, + "Object": "userDecryptionOptions" + }); + + if sso_login && let Some(option) = trusted_device_option(user, device, conn).await { + user_decryption_options["TrustedDeviceOption"] = option; + } + let mut result = json!({ "access_token": auth_tokens.access_token(), "expires_in": auth_tokens.expires_in(), @@ -562,11 +612,7 @@ async fn authenticated_response( "MasterPasswordPolicy": master_password_policy, "scope": auth_tokens.scope(), "AccountKeys": account_keys, - "UserDecryptionOptions": { - "HasMasterPassword": has_master_password, - "MasterPasswordUnlock": master_password_unlock, - "Object": "userDecryptionOptions" - }, + "UserDecryptionOptions": user_decryption_options, }); if !user.akey.is_empty() { diff --git a/src/config.rs b/src/config.rs index c4457478..72007f3e 100644 --- a/src/config.rs +++ b/src/config.rs @@ -840,6 +840,8 @@ make_config! { sso_client_cache_expiration: u64, true, def, 0; /// Log all tokens |> `LOG_LEVEL=debug` or `LOG_LEVEL=info,vaultwarden::sso=debug` is required sso_debug_tokens: bool, true, def, false; + /// Trusted device encryption |> Let users unlock their vault after an SSO login with a key stored on a trusted device instead of a master password. A user who never sets a master password and then loses every trusted device cannot recover their vault. See: https://bitwarden.com/help/login-with-sso-trusted-devices/ + sso_trusted_device_encryption: bool, true, def, false; }, /// Yubikey settings @@ -1107,6 +1109,8 @@ fn validate_config(cfg: &ConfigItems, on_update: bool) -> Result<(), Error> { validate_internal_sso_issuer_url(&cfg.sso_authority)?; validate_internal_sso_redirect_url(&cfg.sso_callback_path)?; validate_sso_master_password_policy(cfg.sso_master_password_policy.as_ref())?; + } else if cfg.sso_trusted_device_encryption { + err!("`SSO_TRUSTED_DEVICE_ENCRYPTION` requires `SSO_ENABLED` to be set, it only applies to SSO logins") } if cfg._enable_yubico { diff --git a/src/db/models/device.rs b/src/db/models/device.rs index 6c1b686a..3d7391ec 100644 --- a/src/db/models/device.rs +++ b/src/db/models/device.rs @@ -33,6 +33,16 @@ pub struct Device { pub refresh_token: String, pub twofactor_remember: Option, + + // Trusted device encryption. The client generates a key pair per device plus a device key + // that never leaves the device, and stores the three resulting blobs here: + /// The user key, encrypted with `encrypted_public_key`. This is the copy of the user key that + /// lets the device unlock the vault without a master password. + pub encrypted_user_key: Option, + /// The device public key, encrypted with the user key. + pub encrypted_public_key: Option, + /// The device private key, encrypted with the device key. The server never sees the device key. + pub encrypted_private_key: Option, } /// Local methods @@ -53,6 +63,10 @@ impl Device { push_token: None, refresh_token: Device::generate_refresh_token(), twofactor_remember: None, + + encrypted_user_key: None, + encrypted_public_key: None, + encrypted_private_key: None, } } @@ -61,6 +75,44 @@ impl Device { crypto::encode_random_bytes::<64>(&BASE64URL) } + /// A stored key is only usable when it is actually there and non-empty. + fn present(key: Option<&String>) -> Option<&String> { + key.filter(|key| !key.is_empty()) + } + + fn key_json(key: Option<&String>) -> Value { + match Self::present(key) { + Some(key) => Value::String(key.clone()), + None => Value::Null, + } + } + + /// Whether this device holds everything needed to unlock the vault on its own. + /// + /// A client can drop its device key without telling us, so this only says that the server side + /// of the trust is complete. See `DeviceExtensions.IsTrusted` upstream. + pub fn is_trusted(&self) -> bool { + Self::present(self.encrypted_user_key.as_ref()).is_some() + && Self::present(self.encrypted_public_key.as_ref()).is_some() + && Self::present(self.encrypted_private_key.as_ref()).is_some() + } + + /// The wrapped user key, but only while the whole trust is intact. Handing out one half of an + /// incomplete set would just make the client fail later in the unlock. + pub fn trusted_user_key(&self) -> Option<&String> { + self.is_trusted().then_some(self.encrypted_user_key.as_ref()).flatten() + } + + pub fn trusted_private_key(&self) -> Option<&String> { + self.is_trusted().then_some(self.encrypted_private_key.as_ref()).flatten() + } + + pub fn untrust(&mut self) { + self.encrypted_user_key = None; + self.encrypted_public_key = None; + self.encrypted_private_key = None; + } + pub fn to_json(&self) -> Value { json!({ "id": self.uuid, @@ -68,11 +120,28 @@ impl Device { "type": self.atype, "identifier": self.uuid, "creationDate": format_date(&self.created_at), - "isTrusted": false, + "isTrusted": self.is_trusted(), + "encryptedUserKey": Self::key_json(self.encrypted_user_key.as_ref()), + "encryptedPublicKey": Self::key_json(self.encrypted_public_key.as_ref()), "object":"device" }) } + /// Response of `POST /devices//retrieve-keys`, used by the clients to re-wrap the + /// user key for every trusted device during a key rotation. + pub fn to_protected_json(&self) -> Value { + json!({ + "id": self.uuid, + "name": self.name, + "type": self.atype, + "identifier": self.uuid, + "creationDate": format_date(&self.created_at), + "encryptedUserKey": Self::key_json(self.encrypted_user_key.as_ref()), + "encryptedPublicKey": Self::key_json(self.encrypted_public_key.as_ref()), + "object": "protectedDevice" + }) + } + pub fn refresh_twofactor_remember(&mut self) -> String { use crate::auth::{encode_jwt, generate_2fa_remember_claims}; @@ -123,9 +192,9 @@ impl DeviceWithAuthRequest { "identifier": self.device.uuid, "creationDate": format_date(&self.device.created_at), "devicePendingAuthRequest": auth_request, - "isTrusted": false, - "encryptedPublicKey": null, - "encryptedUserKey": null, + "isTrusted": self.device.is_trusted(), + "encryptedPublicKey": Device::key_json(self.device.encrypted_public_key.as_ref()), + "encryptedUserKey": Device::key_json(self.device.encrypted_user_key.as_ref()), "object": "device", }) } @@ -177,6 +246,37 @@ impl Device { .await } + /// Invalidates every copy of the user key that is wrapped for one of the user's devices. + /// + /// Called when the user key itself is replaced, which leaves all of those copies pointing at a + /// key that no longer unlocks anything. `keep_private_key_for` (the device that performed the + /// rotation) keeps its own private key, so its client can immediately re-wrap the new user key + /// via `POST /devices/update-trust`; every other device is untrusted outright. Until that + /// happens no device counts as trusted, so the worst case is an extra login, not a broken vault. + pub async fn invalidate_wrapped_user_keys( + user_uuid: &UserId, + keep_private_key_for: &DeviceId, + conn: &DbConn, + ) -> EmptyResult { + conn.run(move |conn| { + let _: () = diesel::update( + devices::table.filter(devices::user_uuid.eq(user_uuid)).filter(devices::uuid.ne(keep_private_key_for)), + ) + .set(devices::encrypted_private_key.eq::>(None)) + .execute(conn) + .map_res("Error untrusting the devices")?; + + diesel::update(devices::table.filter(devices::user_uuid.eq(user_uuid))) + .set(( + devices::encrypted_user_key.eq::>(None), + devices::encrypted_public_key.eq::>(None), + )) + .execute(conn) + .map_res("Error invalidating the wrapped user keys of the devices") + }) + .await + } + pub async fn find_by_uuid_and_user(uuid: &DeviceId, user_uuid: &UserId, conn: &DbConn) -> Option { conn.run(move |conn| { devices::table @@ -364,6 +464,18 @@ impl DeviceType { _ => DeviceType::UnknownBrowser, } } + + /// Whether a device of this type can answer a login request from another device. + /// + /// The SDK, the server and the CLIs have no interactive prompt to show the request in, so they + /// are the ones left out. Matches `LoginApprovingClientTypes` upstream, which allows the + /// desktop, mobile, web and browser client types. + pub fn can_approve_login_requests(&self) -> bool { + !matches!( + self, + DeviceType::Sdk | DeviceType::Server | DeviceType::WindowsCLI | DeviceType::MacOsCLI | DeviceType::LinuxCLI + ) + } } #[derive( @@ -373,3 +485,70 @@ pub struct DeviceId(String); #[derive(Clone, Debug, DieselNewType, Display, From, FromForm, Serialize, Deserialize, UuidFromParam)] pub struct PushId(pub String); + +#[cfg(test)] +mod tests { + use super::*; + + fn trusted_device() -> Device { + let mut device = Device::new(String::from("device").into(), String::from("user").into(), String::new(), 9); + device.encrypted_user_key = Some(String::from("2.user")); + device.encrypted_public_key = Some(String::from("2.public")); + device.encrypted_private_key = Some(String::from("2.private")); + device + } + + #[test] + fn a_device_is_only_trusted_with_all_three_keys() { + assert!(trusted_device().is_trusted()); + + let keys: [fn(&mut Device) -> &mut Option; 3] = [ + |device| &mut device.encrypted_user_key, + |device| &mut device.encrypted_public_key, + |device| &mut device.encrypted_private_key, + ]; + + for key in keys { + let mut device = trusted_device(); + *key(&mut device) = None; + assert!(!device.is_trusted()); + + let mut device = trusted_device(); + *key(&mut device) = Some(String::new()); + assert!(!device.is_trusted(), "an empty key is as good as a missing one"); + } + } + + #[test] + fn an_incomplete_device_hands_out_no_keys_at_all() { + let mut device = trusted_device(); + assert_eq!(device.trusted_user_key(), Some(&String::from("2.user"))); + assert_eq!(device.trusted_private_key(), Some(&String::from("2.private"))); + + // The public key is not part of the login response, but without it the other two are + // useless to the client, so it must not get them either. + device.encrypted_public_key = None; + assert_eq!(device.trusted_user_key(), None); + assert_eq!(device.trusted_private_key(), None); + } + + #[test] + fn untrusting_clears_every_key() { + let mut device = trusted_device(); + device.untrust(); + + assert!(!device.is_trusted()); + assert_eq!(device.encrypted_user_key, None); + assert_eq!(device.encrypted_public_key, None); + assert_eq!(device.encrypted_private_key, None); + } + + #[test] + fn only_interactive_clients_can_approve_a_login_request() { + for atype in 0..=26 { + let device_type = DeviceType::from_i32(atype); + let expected = !matches!(atype, 21..=25); + assert_eq!(device_type.can_approve_login_requests(), expected, "device type {atype} ({device_type})"); + } + } +} diff --git a/src/db/schema.rs b/src/db/schema.rs index af342186..b1766270 100644 --- a/src/db/schema.rs +++ b/src/db/schema.rs @@ -55,6 +55,9 @@ table! { push_token -> Nullable, refresh_token -> Text, twofactor_remember -> Nullable, + encrypted_user_key -> Nullable, + encrypted_public_key -> Nullable, + encrypted_private_key -> Nullable, } } From dbeec752b86a2ffd18a235c7c7d971cb094e0eee Mon Sep 17 00:00:00 2001 From: tom27052006 <83423411+tom27052006@users.noreply.github.com> Date: Fri, 31 Jul 2026 22:51:51 +0200 Subject: [PATCH 02/10] Add device approval by an organization administrator Completes the trusted device flow for the case it was missing: a member who unlocks with a trusted device, has no other device of their own left to ask, and therefore has no way back into their vault. They can now turn to the administrators of their organization, who hand them their own user key encrypted for the key pair of the asking device. New `atype` on auth_requests, mirroring bitwarden/server's AuthRequestType. It decides who may answer a request and how long it stays open: 15 minutes between the user's own devices, a week for an administrator, and half a day for their answer once given. The purge job applies that per type instead of dropping everything after 15 minutes, and both the answer and the anonymous lookup now refuse an expired request, which they did not before. POST /auth-requests/admin-request ask, one request per org GET /organizations//auth-requests what is waiting for an answer POST /organizations//auth-requests/ approve or deny one POST /organizations//auth-requests/deny deny several POST /organizations//auth-requests answer several Asking requires authentication, so the anonymous `POST /auth-requests` now refuses the type. Answering goes through the organization the request was addressed to and needs admin rights there; the asking user cannot answer their own request through `PUT /auth-requests/`, which would make the whole detour pointless. A denial is saved but not announced, so a request that did not come from the member does not learn that it was seen. The administrator's view leaves out the access code, which is the asking device's proof and none of their business. The administrators are mailed when a request arrives, the member when one of their devices was let in, so an approval nobody asked for does not pass unnoticed. Two fixes without which none of this is reachable from a client: - `UserDecryptionOptions.TrustedDeviceOption` reported `HasAdminApproval` and `HasManageResetPasswordPermission` as a flat false. The clients decide on `hasAdminApproval || hasMasterPassword` whether a login is a returning user or a brand new one, so a member without a master password was shown the screen for creating an account, on every device but the one they first trusted. Both are now derived from the account recovery enrollment and the role. - `PUT /organizations//users//reset-password-enrollment` demanded a master password whenever a key was supplied. An account that unlocks with a trusted device has none, and the clients send nothing but the key when they enroll during registration, so enrolling was impossible for exactly the accounts that need it most. Upstream carves out the same exception, keyed on the organization's SSO configuration rather than on a server-wide setting as here. Enrolling now also accepts a pending invitation, as upstream does, so a just-provisioned member does not stay invited forever with nobody able to confirm them. --- .../down.sql | 1 + .../up.sql | 1 + .../down.sql | 1 + .../up.sql | 1 + .../down.sql | 1 + .../up.sql | 1 + src/api/core/accounts.rs | 201 +++++++++----- src/api/core/organizations.rs | 245 +++++++++++++++++- src/api/identity.rs | 25 +- src/config.rs | 2 + src/db/models/auth_request.rs | 199 +++++++++++++- src/db/models/mod.rs | 2 +- src/db/schema.rs | 1 + src/mail.rs | 48 ++++ .../email/device_approval_requested.hbs | 6 + .../email/device_approval_requested.html.hbs | 16 ++ .../email/trusted_device_admin_approval.hbs | 9 + .../trusted_device_admin_approval.html.hbs | 22 ++ 18 files changed, 695 insertions(+), 87 deletions(-) create mode 100644 migrations/mysql/2026-07-31-130000_add_auth_request_type/down.sql create mode 100644 migrations/mysql/2026-07-31-130000_add_auth_request_type/up.sql create mode 100644 migrations/postgresql/2026-07-31-130000_add_auth_request_type/down.sql create mode 100644 migrations/postgresql/2026-07-31-130000_add_auth_request_type/up.sql create mode 100644 migrations/sqlite/2026-07-31-130000_add_auth_request_type/down.sql create mode 100644 migrations/sqlite/2026-07-31-130000_add_auth_request_type/up.sql create mode 100644 src/static/templates/email/device_approval_requested.hbs create mode 100644 src/static/templates/email/device_approval_requested.html.hbs create mode 100644 src/static/templates/email/trusted_device_admin_approval.hbs create mode 100644 src/static/templates/email/trusted_device_admin_approval.html.hbs diff --git a/migrations/mysql/2026-07-31-130000_add_auth_request_type/down.sql b/migrations/mysql/2026-07-31-130000_add_auth_request_type/down.sql new file mode 100644 index 00000000..3e6ab6ee --- /dev/null +++ b/migrations/mysql/2026-07-31-130000_add_auth_request_type/down.sql @@ -0,0 +1 @@ +ALTER TABLE auth_requests DROP COLUMN atype; diff --git a/migrations/mysql/2026-07-31-130000_add_auth_request_type/up.sql b/migrations/mysql/2026-07-31-130000_add_auth_request_type/up.sql new file mode 100644 index 00000000..90fba887 --- /dev/null +++ b/migrations/mysql/2026-07-31-130000_add_auth_request_type/up.sql @@ -0,0 +1 @@ +ALTER TABLE auth_requests ADD COLUMN atype INTEGER NOT NULL DEFAULT 0; diff --git a/migrations/postgresql/2026-07-31-130000_add_auth_request_type/down.sql b/migrations/postgresql/2026-07-31-130000_add_auth_request_type/down.sql new file mode 100644 index 00000000..3e6ab6ee --- /dev/null +++ b/migrations/postgresql/2026-07-31-130000_add_auth_request_type/down.sql @@ -0,0 +1 @@ +ALTER TABLE auth_requests DROP COLUMN atype; diff --git a/migrations/postgresql/2026-07-31-130000_add_auth_request_type/up.sql b/migrations/postgresql/2026-07-31-130000_add_auth_request_type/up.sql new file mode 100644 index 00000000..90fba887 --- /dev/null +++ b/migrations/postgresql/2026-07-31-130000_add_auth_request_type/up.sql @@ -0,0 +1 @@ +ALTER TABLE auth_requests ADD COLUMN atype INTEGER NOT NULL DEFAULT 0; diff --git a/migrations/sqlite/2026-07-31-130000_add_auth_request_type/down.sql b/migrations/sqlite/2026-07-31-130000_add_auth_request_type/down.sql new file mode 100644 index 00000000..3e6ab6ee --- /dev/null +++ b/migrations/sqlite/2026-07-31-130000_add_auth_request_type/down.sql @@ -0,0 +1 @@ +ALTER TABLE auth_requests DROP COLUMN atype; diff --git a/migrations/sqlite/2026-07-31-130000_add_auth_request_type/up.sql b/migrations/sqlite/2026-07-31-130000_add_auth_request_type/up.sql new file mode 100644 index 00000000..90fba887 --- /dev/null +++ b/migrations/sqlite/2026-07-31-130000_add_auth_request_type/up.sql @@ -0,0 +1 @@ +ALTER TABLE auth_requests ADD COLUMN atype INTEGER NOT NULL DEFAULT 0; diff --git a/src/api/core/accounts.rs b/src/api/core/accounts.rs index 81a0ceae..a831f0df 100644 --- a/src/api/core/accounts.rs +++ b/src/api/core/accounts.rs @@ -20,9 +20,10 @@ use crate::{ db::{ DbConn, DbPool, models::{ - 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, + AuthRequest, AuthRequestId, AuthRequestType, Cipher, CipherId, Device, DeviceId, DeviceType, + DeviceWithAuthRequest, EmergencyAccess, EmergencyAccessId, EventType, Folder, FolderId, Invitation, + Membership, MembershipId, MembershipType, OrgPolicy, OrgPolicyType, Organization, OrganizationId, Send, + SendId, User, UserId, UserKdfType, }, }, mail, @@ -76,6 +77,7 @@ pub fn routes() -> Vec { post_devices_lost_trust, get_tasks, post_auth_request, + post_admin_auth_request, get_auth_request, put_auth_request, get_auth_request_response, @@ -1806,9 +1808,26 @@ struct AuthRequestRequest { device_identifier: DeviceId, email: String, public_key: String, - // Not used for now - // #[serde(alias = "type")] - // _type: i32, + #[serde(default, rename = "type")] + atype: i32, +} + +fn auth_request_json(auth_request: &AuthRequest) -> Value { + json!({ + "id": auth_request.uuid, + "publicKey": auth_request.public_key, + "type": auth_request.atype, + "requestDeviceType": DeviceType::from_i32(auth_request.device_type).to_string(), + "requestDeviceIdentifier": auth_request.request_device_identifier, + "requestIpAddress": auth_request.request_ip, + "key": auth_request.enc_key, + "masterPasswordHash": auth_request.master_password_hash, + "creationDate": format_date(&auth_request.creation_date), + "responseDate": auth_request.response_date.as_ref().map(format_date), + "requestApproved": auth_request.approved.unwrap_or(false), + "origin": CONFIG.domain_origin(), + "object": "auth-request" + }) } #[post("/auth-requests", data = "")] @@ -1820,6 +1839,12 @@ async fn post_auth_request( ) -> JsonResult { let data = data.into_inner(); + // Asking an administrator for approval means telling them who is asking, so that one is only + // available to a caller who has already proven who they are. See `post_admin_auth_request`. + if AuthRequestType::from_i32(data.atype) == Some(AuthRequestType::AdminApproval) { + err!("You must be authenticated to create a request of that type") + } + let Some(user) = User::find_by_mail(&data.email, &conn).await else { err!("AuthRequest doesn't exist", "User not found") }; @@ -1830,8 +1855,14 @@ async fn post_auth_request( _ => err!("AuthRequest doesn't exist", "Device verification failed"), }; + let Some(atype) = AuthRequestType::from_i32(data.atype) else { + err!("Unknown auth request type") + }; + let mut auth_request = AuthRequest::new( user.uuid.clone(), + None, + atype, data.device_identifier.clone(), client_headers.device_type, client_headers.ip.ip.to_string(), @@ -1851,19 +1882,91 @@ async fn post_auth_request( ) .await; - Ok(Json(json!({ - "id": auth_request.uuid, - "publicKey": auth_request.public_key, - "requestDeviceType": DeviceType::from_i32(auth_request.device_type).to_string(), - "requestIpAddress": auth_request.request_ip, - "key": null, - "masterPasswordHash": null, - "creationDate": format_date(&auth_request.creation_date), - "responseDate": null, - "requestApproved": false, - "origin": CONFIG.domain_origin(), - "object": "auth-request" - }))) + Ok(Json(auth_request_json(&auth_request))) +} + +/// Asks the administrators of every organization the user belongs to to let this device in. +/// +/// The way out for someone who unlocks with trusted devices and has no other device left to ask. +/// One request per organization, so whichever administrator gets there first can answer. +/// https://github.com/bitwarden/server/blob/main/src/Api/Auth/Controllers/AuthRequestsController.cs +#[post("/auth-requests/admin-request", data = "")] +async fn post_admin_auth_request(data: Json, headers: Headers, conn: DbConn) -> JsonResult { + let data = data.into_inner(); + + if AuthRequestType::from_i32(data.atype) != Some(AuthRequestType::AdminApproval) { + err!("Invalid auth request type, expected admin approval") + } + + if data.device_identifier != headers.device.uuid { + err!("AuthRequest doesn't exist", "Device verification failed") + } + + let memberships = Membership::find_by_user(&headers.user.uuid, &conn).await; + if memberships.is_empty() { + err!("User does not belong to any organization") + } + + log_user_event( + EventType::UserRequestedDeviceApproval as i32, + &headers.user.uuid, + headers.device.atype, + &headers.ip.ip, + &conn, + ) + .await; + + let mut first_request = None; + for membership in memberships { + let mut auth_request = AuthRequest::new( + headers.user.uuid.clone(), + Some(membership.org_uuid.clone()), + AuthRequestType::AdminApproval, + data.device_identifier.clone(), + headers.device.atype, + headers.ip.ip.to_string(), + data.access_code.clone(), + data.public_key.clone(), + ); + auth_request.save(&conn).await?; + + notify_device_approval_requested(&headers.user, &membership.org_uuid, &conn).await; + + if first_request.is_none() { + first_request = Some(auth_request); + } + } + + // Guaranteed by the emptiness check above + let auth_request = first_request.expect("at least one organization"); + Ok(Json(auth_request_json(&auth_request))) +} + +/// Mails everyone in the organization who could answer the request. Failing to reach them must not +/// undo the request itself, so problems are logged rather than returned. +async fn notify_device_approval_requested(user: &User, org_id: &OrganizationId, conn: &DbConn) { + if !CONFIG.mail_enabled() { + return; + } + + let Some(org) = Organization::find_by_uuid(org_id, conn).await else { + return; + }; + + let approvers = Membership::find_confirmed_by_org(org_id, conn) + .await + .into_iter() + .filter(|member| member.atype <= MembershipType::Admin as i32); + + for approver in approvers { + let Some(admin) = User::find_by_uuid(&approver.user_uuid, conn).await else { + continue; + }; + + if let Err(e) = mail::send_device_approval_requested(&admin.email, &org.name, &user.email, &user.name).await { + error!("Error sending device approval request email: {e:#?}"); + } + } } #[get("/auth-requests/")] @@ -1873,21 +1976,7 @@ async fn get_auth_request(auth_request_id: AuthRequestId, headers: Headers, conn err!("AuthRequest doesn't exist", "Record not found or user uuid does not match") }; - let response_date_utc = auth_request.response_date.map(|response_date| format_date(&response_date)); - - Ok(Json(json!({ - "id": &auth_request_id, - "publicKey": auth_request.public_key, - "requestDeviceType": DeviceType::from_i32(auth_request.device_type).to_string(), - "requestIpAddress": auth_request.request_ip, - "key": auth_request.enc_key, - "masterPasswordHash": auth_request.master_password_hash, - "creationDate": format_date(&auth_request.creation_date), - "responseDate": response_date_utc, - "requestApproved": auth_request.approved, - "origin": CONFIG.domain_origin(), - "object":"auth-request" - }))) + Ok(Json(auth_request_json(&auth_request))) } #[derive(Debug, Deserialize)] @@ -1914,6 +2003,13 @@ async fn put_auth_request( err!("AuthRequest doesn't exist", "Record not found or user uuid does not match") }; + // A request addressed to an administrator is answered through the organization, where the + // permission to do so can actually be checked. Letting the asking user answer it here would + // make the whole detour pointless. + if auth_request.is_admin_approval() { + err!("AuthRequest doesn't exist", "Admin approval requests are answered by the organization") + } + if headers.device.uuid != data.device_identifier { err!("AuthRequest doesn't exist", "Device verification failed") } @@ -1922,8 +2018,11 @@ async fn put_auth_request( err!("An authentication request with the same device already exists") } + if auth_request.is_expired() { + err!("AuthRequest doesn't exist", "Request has expired") + } + let response_date = Utc::now().naive_utc(); - let response_date_utc = format_date(&response_date); if data.request_approved { auth_request.approved = Some(data.request_approved); @@ -1957,19 +2056,7 @@ async fn put_auth_request( .await; } - Ok(Json(json!({ - "id": &auth_request_id, - "publicKey": auth_request.public_key, - "requestDeviceType": DeviceType::from_i32(auth_request.device_type).to_string(), - "requestIpAddress": auth_request.request_ip, - "key": auth_request.enc_key, - "masterPasswordHash": auth_request.master_password_hash, - "creationDate": format_date(&auth_request.creation_date), - "responseDate": response_date_utc, - "requestApproved": auth_request.approved, - "origin": CONFIG.domain_origin(), - "object":"auth-request" - }))) + Ok(Json(auth_request_json(&auth_request))) } #[get("/auth-requests//response?")] @@ -1990,21 +2077,11 @@ async fn get_auth_request_response( err!("AuthRequest doesn't exist", "Invalid device, IP or code") } - let response_date_utc = auth_request.response_date.map(|response_date| format_date(&response_date)); + if auth_request.is_expired() { + err!("AuthRequest doesn't exist", "Request has expired") + } - Ok(Json(json!({ - "id": &auth_request_id, - "publicKey": auth_request.public_key, - "requestDeviceType": DeviceType::from_i32(auth_request.device_type).to_string(), - "requestIpAddress": auth_request.request_ip, - "key": auth_request.enc_key, - "masterPasswordHash": auth_request.master_password_hash, - "creationDate": format_date(&auth_request.creation_date), - "responseDate": response_date_utc, - "requestApproved": auth_request.approved, - "origin": CONFIG.domain_origin(), - "object":"auth-request" - }))) + Ok(Json(auth_request_json(&auth_request))) } // Now unused but not yet removed diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index 989ca47d..197480f3 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -1,5 +1,6 @@ use std::collections::{HashMap, HashSet}; +use chrono::Utc; use num_traits::FromPrimitive; use rocket::{Route, serde::json::Json}; use serde_json::Value; @@ -8,16 +9,17 @@ use crate::{ CONFIG, api::admin::FAKE_ADMIN_UUID, api::{ - EmptyResult, JsonResult, Notify, PasswordOrOtpData, UpdateType, + AnonymousNotify, EmptyResult, JsonResult, Notify, PasswordOrOtpData, UpdateType, core::{CipherSyncData, CipherSyncType, accept_org_invite, log_event, two_factor}, }, auth::{AdminHeaders, Headers, ManagerHeaders, ManagerHeadersLoose, OrgMemberHeaders, OwnerHeaders, decode_invite}, db::{ DbConn, models::{ - Cipher, CipherId, Collection, CollectionCipher, CollectionGroup, CollectionId, CollectionUser, EventType, - Group, GroupId, GroupUser, Invitation, Membership, MembershipId, MembershipStatus, MembershipType, - OrgPolicy, OrgPolicyType, Organization, OrganizationApiKey, OrganizationId, User, UserId, + AuthRequest, AuthRequestId, Cipher, CipherId, Collection, CollectionCipher, CollectionGroup, CollectionId, + CollectionUser, DeviceType, EventType, Group, GroupId, GroupUser, Invitation, Membership, MembershipId, + MembershipStatus, MembershipType, OrgPolicy, OrgPolicyType, Organization, OrganizationApiKey, + OrganizationId, User, UserId, }, }, mail, @@ -97,6 +99,10 @@ pub fn routes() -> Vec { get_reset_password_details, put_reset_password, put_recover_account, + get_organization_auth_requests, + deny_organization_auth_requests, + update_organization_auth_request, + update_many_organization_auth_requests, get_org_export, post_api_key, rotate_api_key, @@ -3153,7 +3159,14 @@ async fn put_reset_password_enrollment( err!("Reset password can't be withdrawn due to an enterprise policy"); } - if reset_password_key.is_some() { + // An account that unlocks with a trusted device has no master password to verify against, and + // the clients send nothing but the key when they enroll as part of that flow. Upstream carves + // out the same exception, keyed on the organization's SSO configuration rather than on a + // server-wide setting as here. + // https://github.com/bitwarden/server/blob/main/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs + let trusted_device_enrollment = CONFIG.sso_trusted_device_encryption() && headers.user.password_hash.is_empty(); + + if reset_password_key.is_some() && !trusted_device_enrollment { PasswordOrOtpData { master_password_hash: reset_request.master_password_hash, otp: reset_request.otp, @@ -3162,21 +3175,233 @@ async fn put_reset_password_enrollment( .await?; } - membership.reset_password_key = reset_password_key; - membership.save(&conn).await?; + let enrolled = reset_password_key.is_some(); + let membership_id = membership.uuid.clone(); - let event_type = if membership.reset_password_key.is_some() { + // Enrolling is where a member who was invited into a trusted device organization turns into a + // real one; upstream accepts the invitation at this point as well. Without it they would stay + // invited forever and no admin could ever confirm them. + if enrolled && membership.status == MembershipStatus::Invited as i32 { + accept_org_invite(&headers.user, membership, reset_password_key, &conn).await?; + } else { + membership.reset_password_key = reset_password_key; + membership.save(&conn).await?; + } + + let event_type = if enrolled { EventType::OrganizationUserResetPasswordEnroll as i32 } else { EventType::OrganizationUserResetPasswordWithdraw as i32 }; - log_event(event_type, &membership.uuid, &org_id, &headers.user.uuid, headers.device.atype, &headers.ip.ip, &conn) + log_event(event_type, &membership_id, &org_id, &headers.user.uuid, headers.device.atype, &headers.ip.ip, &conn) .await; Ok(()) } +// Device approvals. A member who unlocks with a trusted device and has no other device of their own +// left to ask can turn to the administrators of their organization instead. Answering means handing +// them their own user key, encrypted for the key pair of the asking device, which is only possible +// because the member enrolled into account recovery beforehand. +// https://github.com/bitwarden/server/blob/main/src/Api/AdminConsole/Controllers/OrganizationAuthRequestsController.cs + +/// The requests waiting for an answer in this organization. +#[get("/organizations//auth-requests")] +async fn get_organization_auth_requests(org_id: OrganizationId, headers: AdminHeaders, conn: DbConn) -> JsonResult { + if org_id != headers.org_id { + err!("Organization not found", "Organization id's do not match"); + } + + let mut requests = Vec::new(); + for auth_request in AuthRequest::find_pending_admin_approval_by_org(&org_id, &conn).await { + if auth_request.is_expired() { + continue; + } + + // A request whose asker is no longer a member of this organization is none of its business + // anymore, so it is quietly left out instead of being offered for approval. + let (Some(member), Some(user)) = ( + Membership::find_by_user_and_org(&auth_request.user_uuid, &org_id, &conn).await, + User::find_by_uuid(&auth_request.user_uuid, &conn).await, + ) else { + continue; + }; + + requests.push(auth_request.to_json_for_organization(&user.email, &member.uuid)); + } + + Ok(Json(json!({ + "data": requests, + "continuationToken": null, + "object": "list" + }))) +} + +#[derive(Deserialize)] +#[serde(rename_all = "camelCase")] +struct AdminAuthRequestUpdateData { + request_approved: bool, + encrypted_user_key: Option, +} + +#[derive(Deserialize)] +#[serde(rename_all = "camelCase")] +struct BulkDenyAuthRequestData { + ids: Vec, +} + +#[derive(Deserialize)] +#[serde(rename_all = "camelCase")] +struct OrganizationAuthRequestUpdateData { + id: AuthRequestId, + approved: bool, + key: Option, +} + +#[post("/organizations//auth-requests/", data = "", rank = 2)] +async fn update_organization_auth_request( + org_id: OrganizationId, + request_id: AuthRequestId, + data: Json, + headers: AdminHeaders, + conn: DbConn, + ant: AnonymousNotify<'_>, + nt: Notify<'_>, +) -> EmptyResult { + let data = data.into_inner(); + answer_organization_auth_request( + &org_id, + &request_id, + data.request_approved, + data.encrypted_user_key, + &headers, + &conn, + &ant, + &nt, + ) + .await +} + +#[post("/organizations//auth-requests/deny", data = "", rank = 1)] +async fn deny_organization_auth_requests( + org_id: OrganizationId, + data: Json, + headers: AdminHeaders, + conn: DbConn, + ant: AnonymousNotify<'_>, + nt: Notify<'_>, +) -> EmptyResult { + for request_id in data.into_inner().ids { + answer_organization_auth_request(&org_id, &request_id, false, None, &headers, &conn, &ant, &nt).await?; + } + + Ok(()) +} + +#[post("/organizations//auth-requests", data = "")] +async fn update_many_organization_auth_requests( + org_id: OrganizationId, + data: Json>, + headers: AdminHeaders, + conn: DbConn, + ant: AnonymousNotify<'_>, + nt: Notify<'_>, +) -> EmptyResult { + for update in data.into_inner() { + answer_organization_auth_request(&org_id, &update.id, update.approved, update.key, &headers, &conn, &ant, &nt) + .await?; + } + + Ok(()) +} + +#[expect(clippy::too_many_arguments, reason = "Rocket request guards have to be passed through")] +async fn answer_organization_auth_request( + org_id: &OrganizationId, + request_id: &AuthRequestId, + approved: bool, + encrypted_user_key: Option, + headers: &AdminHeaders, + conn: &DbConn, + ant: &AnonymousNotify<'_>, + nt: &Notify<'_>, +) -> EmptyResult { + if org_id != &headers.org_id { + err!("Organization not found", "Organization id's do not match"); + } + + // Only ever reachable through the organization it was addressed to, so an administrator cannot + // answer for an organization they have no say in. + let Some(mut auth_request) = AuthRequest::find_admin_approval_by_org_and_uuid(request_id, org_id, conn).await + else { + err!("AuthRequest doesn't exist", "Record not found or not addressed to this organization") + }; + + if auth_request.approved.is_some() { + err!("This request has already been answered") + } + + if auth_request.is_expired() { + err!("AuthRequest doesn't exist", "Request has expired") + } + + let Some(member) = Membership::find_by_user_and_org(&auth_request.user_uuid, org_id, conn).await else { + err!("AuthRequest doesn't exist", "The requesting user is no longer a member of this organization") + }; + + if approved { + // Without the wrapped user key the answer is worthless: it is the whole point of approving. + let Some(key) = encrypted_user_key.filter(|key| !key.is_empty()) else { + err!("An approved request needs the encrypted user key") + }; + auth_request.enc_key = Some(key); + } + + auth_request.approved = Some(approved); + auth_request.response_date = Some(Utc::now().naive_utc()); + auth_request.save(conn).await?; + + let event_type = if approved { + EventType::OrganizationUserApprovedAuthRequest as i32 + } else { + EventType::OrganizationUserRejectedAuthRequest as i32 + }; + log_event(event_type, &member.uuid, org_id, &headers.user.uuid, headers.device.atype, &headers.ip.ip, conn).await; + + // A denial is deliberately not announced. If the request came from somebody who is not the + // member, telling them that it was seen and refused is more than they should learn. + if !approved { + return Ok(()); + } + + ant.send_auth_response(&auth_request.user_uuid, &auth_request.uuid).await; + nt.send_auth_response(&auth_request.user_uuid, &auth_request.uuid, &headers.device, conn).await; + + if CONFIG.mail_enabled() + && let Some(user) = User::find_by_uuid(&auth_request.user_uuid, conn).await + && let Some(org) = Organization::find_by_uuid(org_id, conn).await + { + let device = + format!("{} - {}", DeviceType::from_i32(auth_request.device_type), auth_request.request_device_identifier); + let approved_at = auth_request.response_date.unwrap_or_else(|| Utc::now().naive_utc()); + + if let Err(e) = mail::send_trusted_device_admin_approval( + &user.email, + &org.name, + &approved_at, + &auth_request.request_ip, + &device, + ) + .await + { + error!("Error sending trusted device approval email: {e:#?}"); + } + } + + Ok(()) +} + // NOTE: It seems clients can't handle uppercase-first keys!! // We need to convert all keys so they have the first character to be a lowercase. // Else the export will be just an empty JSON file. @@ -3214,7 +3439,7 @@ async fn api_key( let org_api_key = if let Some(mut org_api_key) = OrganizationApiKey::find_by_org_uuid(org_id, &conn).await { if rotate { org_api_key.api_key = crate::crypto::generate_api_key(); - org_api_key.revision_date = chrono::Utc::now().naive_utc(); + org_api_key.revision_date = Utc::now().naive_utc(); org_api_key.save(&conn).await.expect("Error rotating organization API Key"); } org_api_key diff --git a/src/api/identity.rs b/src/api/identity.rs index a25c83b3..37cd52ec 100644 --- a/src/api/identity.rs +++ b/src/api/identity.rs @@ -30,9 +30,9 @@ use crate::{ db::{ DbConn, models::{ - AuthRequest, AuthRequestId, Device, DeviceId, DeviceType, EventType, Invitation, OIDCCodeResponseError, - OrganizationApiKey, OrganizationId, SendId, SsoAuth, SsoUser, TwoFactor, TwoFactorIncomplete, - TwoFactorType, User, UserId, + AuthRequest, AuthRequestId, Device, DeviceId, DeviceType, EventType, Invitation, Membership, + MembershipStatus, MembershipType, OIDCCodeResponseError, OrganizationApiKey, OrganizationId, SendId, + SsoAuth, SsoUser, TwoFactor, TwoFactorIncomplete, TwoFactorType, User, UserId, }, }, error::MapResult, @@ -507,12 +507,23 @@ async fn trusted_device_option(user: &User, device: &Device, conn: &DbConn) -> O .iter() .any(|other| other.uuid != device.uuid && DeviceType::from_i32(other.atype).can_approve_login_requests()); - // Approval by an organization admin is not implemented. Announcing it would leave the client - // waiting on a request that nobody here can answer. + let memberships = Membership::find_by_user(&user.uuid, conn).await; + + // An admin can only take over the approval once the member handed them a key to work with, + // which is what enrolling into account recovery does. + let has_admin_approval = + memberships.iter().any(|member| member.reset_password_key.as_ref().is_some_and(|key| !key.is_empty())); + + // Whether the user is on the answering side of that. The clients use it to push someone who + // could approve others, but has no master password themselves, into setting one. + let has_manage_reset_password_permission = memberships.iter().any(|member| { + member.status != MembershipStatus::Revoked as i32 && member.atype <= MembershipType::Admin as i32 + }); + Some(json!({ - "HasAdminApproval": false, + "HasAdminApproval": has_admin_approval, "HasLoginApprovingDevice": has_login_approving_device, - "HasManageResetPasswordPermission": false, + "HasManageResetPasswordPermission": has_manage_reset_password_permission, "IsTdeOffboarding": offboarding, "EncryptedPrivateKey": device.trusted_private_key(), "EncryptedUserKey": device.trusted_user_key(), diff --git a/src/config.rs b/src/config.rs index 72007f3e..09b72907 100644 --- a/src/config.rs +++ b/src/config.rs @@ -1732,6 +1732,7 @@ where reg!("email/change_email_invited", ".html"); reg!("email/change_email", ".html"); reg!("email/delete_account", ".html"); + reg!("email/device_approval_requested", ".html"); reg!("email/emergency_access_invite_accepted", ".html"); reg!("email/emergency_access_invite_confirmed", ".html"); reg!("email/emergency_access_recovery_approved", ".html"); @@ -1753,6 +1754,7 @@ where reg!("email/send_single_org_removed_from_org", ".html"); reg!("email/smtp_test", ".html"); reg!("email/sso_change_email", ".html"); + reg!("email/trusted_device_admin_approval", ".html"); reg!("email/twofactor_email", ".html"); reg!("email/verify_email", ".html"); reg!("email/welcome_must_verify", ".html"); diff --git a/src/db/models/auth_request.rs b/src/db/models/auth_request.rs index a3876661..4a1a1124 100644 --- a/src/db/models/auth_request.rs +++ b/src/db/models/auth_request.rs @@ -1,4 +1,4 @@ -use chrono::{NaiveDateTime, Utc}; +use chrono::{NaiveDateTime, TimeDelta, Utc}; use derive_more::{AsRef, Deref, Display, From}; use diesel::prelude::*; use serde_json::Value; @@ -12,7 +12,7 @@ use crate::{ }; use macros::UuidFromParam; -use super::{DeviceId, OrganizationId, UserId}; +use super::{DeviceId, DeviceType, MembershipId, OrganizationId, UserId}; #[derive(Identifiable, Queryable, Insertable, AsChangeset, Deserialize, Serialize)] #[diesel(table_name = auth_requests)] @@ -22,6 +22,8 @@ pub struct AuthRequest { pub uuid: AuthRequestId, pub user_uuid: UserId, pub organization_uuid: Option, + /// See `AuthRequestType`. Decides who may answer the request and how long it stays open. + pub atype: i32, pub request_device_identifier: DeviceId, pub device_type: i32, // https://github.com/bitwarden/server/blob/9ebe16587175b1c0e9208f84397bb75d0d595510/src/Core/Enums/DeviceType.cs @@ -42,9 +44,50 @@ pub struct AuthRequest { pub authentication_date: Option, } +/// https://github.com/bitwarden/server/blob/main/src/Core/Auth/Enums/AuthRequestType.cs +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum AuthRequestType { + /// A new session asking one of the user's own devices to let it in. + AuthenticateAndUnlock = 0, + /// An existing session asking one of the user's own devices to unlock it. + Unlock = 1, + /// The user asking an administrator of their organization to let a device in, for when no + /// device of their own is around to ask. + AdminApproval = 2, +} + +impl AuthRequestType { + pub fn from_i32(value: i32) -> Option { + match value { + 0 => Some(AuthRequestType::AuthenticateAndUnlock), + 1 => Some(AuthRequestType::Unlock), + 2 => Some(AuthRequestType::AdminApproval), + _ => None, + } + } +} + impl AuthRequest { + /// A request between the user's own devices is short lived, an administrator gets a week to + /// answer, and their answer stays usable for half a day. Same windows as upstream. + /// https://github.com/bitwarden/server/blob/main/src/Core/Settings/GlobalSettings.cs + pub fn user_request_expiration() -> TimeDelta { + TimeDelta::try_minutes(15).unwrap() + } + + pub fn admin_request_expiration() -> TimeDelta { + TimeDelta::try_days(7).unwrap() + } + + pub fn after_admin_approval_expiration() -> TimeDelta { + TimeDelta::try_hours(12).unwrap() + } + + #[expect(clippy::too_many_arguments, reason = "Every field of the request is supplied by the caller")] pub fn new( user_uuid: UserId, + organization_uuid: Option, + atype: AuthRequestType, request_device_identifier: DeviceId, device_type: i32, request_ip: String, @@ -56,7 +99,8 @@ impl AuthRequest { Self { uuid: AuthRequestId(crate::util::get_uuid()), user_uuid, - organization_uuid: None, + organization_uuid, + atype: atype as i32, request_device_identifier, device_type, @@ -73,12 +117,50 @@ impl AuthRequest { } } + pub fn is_admin_approval(&self) -> bool { + self.atype == AuthRequestType::AdminApproval as i32 + } + + pub fn is_expired(&self) -> bool { + let now = Utc::now().naive_utc(); + + if self.is_admin_approval() { + // Once approved the clock restarts, so the user has time to come back and use it. + if let (Some(true), Some(response_date)) = (self.approved, self.response_date) { + return now > response_date + Self::after_admin_approval_expiration(); + } + return now > self.creation_date + Self::admin_request_expiration(); + } + + now > self.creation_date + Self::user_request_expiration() + } + pub fn to_json_for_pending_device(&self) -> Value { json!({ "id": self.uuid, "creationDate": format_date(&self.creation_date), }) } + + /// What an administrator gets to see about a request. Deliberately without the access code: + /// that one is the requesting device's proof, not something the answering side needs. + pub fn to_json_for_organization(&self, email: &str, member_id: &MembershipId) -> Value { + json!({ + "id": self.uuid, + "userId": self.user_uuid, + "organizationUserId": member_id, + "email": email, + "publicKey": self.public_key, + "requestDeviceIdentifier": self.request_device_identifier, + "requestDeviceType": DeviceType::from_i32(self.device_type).to_string(), + "requestIpAddress": self.request_ip, + "key": self.enc_key, + "creationDate": format_date(&self.creation_date), + "requestApproved": self.approved, + "responseDate": self.response_date.as_ref().map(format_date), + "object": "organizationAuthRequest", + }) + } } impl AuthRequest { @@ -155,6 +237,38 @@ impl AuthRequest { .await } + /// Everything an administrator of this organization still has to answer. + pub async fn find_pending_admin_approval_by_org(org_uuid: &OrganizationId, conn: &DbConn) -> Vec { + conn.run(move |conn| { + auth_requests::table + .filter(auth_requests::organization_uuid.eq(org_uuid)) + .filter(auth_requests::atype.eq(AuthRequestType::AdminApproval as i32)) + .filter(auth_requests::approved.is_null()) + .order_by(auth_requests::creation_date.desc()) + .load::(conn) + .expect("Error loading auth_requests") + }) + .await + } + + /// Bound to the organization on purpose: an administrator may only ever reach a request that + /// was addressed to their own organization. + pub async fn find_admin_approval_by_org_and_uuid( + uuid: &AuthRequestId, + org_uuid: &OrganizationId, + conn: &DbConn, + ) -> Option { + conn.run(move |conn| { + auth_requests::table + .filter(auth_requests::uuid.eq(uuid)) + .filter(auth_requests::organization_uuid.eq(org_uuid)) + .filter(auth_requests::atype.eq(AuthRequestType::AdminApproval as i32)) + .first::(conn) + .ok() + }) + .await + } + pub async fn find_created_before(dt: &NaiveDateTime, conn: &DbConn) -> Vec { conn.run(move |conn| { auth_requests::table @@ -179,11 +293,15 @@ impl AuthRequest { } pub async fn purge_expired_auth_requests(conn: &DbConn) { - // delete auth requests older than 15 minutes which is functionally equivalent to upstream: // https://github.com/bitwarden/server/blob/f8ee2270409f7a13125cd414c450740af605a175/src/Sql/dbo/Auth/Stored%20Procedures/AuthRequest_DeleteIfExpired.sql - let expiry_time = Utc::now().naive_utc() - chrono::TimeDelta::try_minutes(15).unwrap(); - for auth_request in Self::find_created_before(&expiry_time, conn).await { - auth_request.delete(conn).await.ok(); + // Nothing can be expired before the shortest window has passed, so that is the cheapest + // way to narrow the table down; which of them really are is decided per type afterwards, + // because a request waiting for an administrator lives a week rather than 15 minutes. + let candidates = Utc::now().naive_utc() - Self::user_request_expiration(); + for auth_request in Self::find_created_before(&candidates, conn).await { + if auth_request.is_expired() { + auth_request.delete(conn).await.ok(); + } } } } @@ -205,3 +323,70 @@ impl AuthRequest { UuidFromParam, )] pub struct AuthRequestId(String); + +#[cfg(test)] +mod tests { + use super::*; + + fn request(atype: AuthRequestType, age: TimeDelta) -> AuthRequest { + let mut auth_request = AuthRequest::new( + String::from("user").into(), + None, + atype, + String::from("device").into(), + 9, + String::from("127.0.0.1"), + String::from("code"), + String::from("2.public"), + ); + auth_request.creation_date = Utc::now().naive_utc() - age; + auth_request + } + + #[test] + fn a_request_between_the_users_own_devices_is_short_lived() { + assert!(!request(AuthRequestType::AuthenticateAndUnlock, TimeDelta::try_minutes(14).unwrap()).is_expired()); + assert!(request(AuthRequestType::AuthenticateAndUnlock, TimeDelta::try_minutes(16).unwrap()).is_expired()); + assert!(request(AuthRequestType::Unlock, TimeDelta::try_minutes(16).unwrap()).is_expired()); + } + + #[test] + fn an_administrator_gets_a_week_to_answer() { + assert!(!request(AuthRequestType::AdminApproval, TimeDelta::try_days(6).unwrap()).is_expired()); + assert!(request(AuthRequestType::AdminApproval, TimeDelta::try_days(8).unwrap()).is_expired()); + } + + #[test] + fn the_answer_of_an_administrator_starts_its_own_clock() { + // Answered right at the end of the week, so the request itself is long past its window. + let mut auth_request = request(AuthRequestType::AdminApproval, TimeDelta::try_days(7).unwrap()); + auth_request.approved = Some(true); + + auth_request.response_date = Some(Utc::now().naive_utc() - TimeDelta::try_hours(11).unwrap()); + assert!(!auth_request.is_expired(), "the user still has time to come back and use it"); + + auth_request.response_date = Some(Utc::now().naive_utc() - TimeDelta::try_hours(13).unwrap()); + assert!(auth_request.is_expired()); + + // A refusal does not extend anything, the request stays dead after its own window. + auth_request.approved = Some(false); + auth_request.response_date = Some(Utc::now().naive_utc()); + assert!(auth_request.is_expired()); + } + + #[test] + fn only_the_admin_approval_type_is_answered_by_an_organization() { + assert!(request(AuthRequestType::AdminApproval, TimeDelta::zero()).is_admin_approval()); + assert!(!request(AuthRequestType::Unlock, TimeDelta::zero()).is_admin_approval()); + assert!(!request(AuthRequestType::AuthenticateAndUnlock, TimeDelta::zero()).is_admin_approval()); + } + + #[test] + fn unknown_request_types_are_rejected() { + assert_eq!(AuthRequestType::from_i32(0), Some(AuthRequestType::AuthenticateAndUnlock)); + assert_eq!(AuthRequestType::from_i32(1), Some(AuthRequestType::Unlock)); + assert_eq!(AuthRequestType::from_i32(2), Some(AuthRequestType::AdminApproval)); + assert_eq!(AuthRequestType::from_i32(3), None); + assert_eq!(AuthRequestType::from_i32(-1), None); + } +} diff --git a/src/db/models/mod.rs b/src/db/models/mod.rs index 0ed8ef91..148a2105 100644 --- a/src/db/models/mod.rs +++ b/src/db/models/mod.rs @@ -20,7 +20,7 @@ mod user; pub use self::archive::Archive; pub use self::attachment::{Attachment, AttachmentId}; -pub use self::auth_request::{AuthRequest, AuthRequestId}; +pub use self::auth_request::{AuthRequest, AuthRequestId, AuthRequestType}; pub use self::cipher::{Cipher, CipherId, RepromptType}; pub use self::collection::{Collection, CollectionCipher, CollectionId, CollectionUser}; pub use self::device::{Device, DeviceId, DeviceType, DeviceWithAuthRequest, PushId}; diff --git a/src/db/schema.rs b/src/db/schema.rs index b1766270..66687ce6 100644 --- a/src/db/schema.rs +++ b/src/db/schema.rs @@ -331,6 +331,7 @@ table! { uuid -> Text, user_uuid -> Text, organization_uuid -> Nullable, + atype -> Integer, request_device_identifier -> Text, device_type -> Integer, request_ip -> Text, diff --git a/src/mail.rs b/src/mail.rs index a7e5e5ae..fe5f217a 100644 --- a/src/mail.rs +++ b/src/mail.rs @@ -531,6 +531,54 @@ pub async fn send_new_device_logged_in(address: &str, ip: &str, dt: &NaiveDateTi send_email(address, &subject, body_html, body_text).await } +/// Tells the administrators of an organization that one of their members is waiting to have a +/// device let in. Trusted device encryption falls back to this when the member has no other device +/// of their own left to ask. +pub async fn send_device_approval_requested( + address: &str, + org_name: &str, + user_email: &str, + user_name: &str, +) -> EmptyResult { + let (subject, body_html, body_text) = get_text( + "email/device_approval_requested", + json!({ + "url": CONFIG.domain(), + "img_src": CONFIG._smtp_img_src(), + "org_name": org_name, + "user_email": user_email, + "user_name": user_name, + }), + )?; + + send_email(address, &subject, body_html, body_text).await +} + +/// The other half of the above: the member learns that a device of theirs was let in, so an +/// approval they did not ask for does not pass unnoticed. +pub async fn send_trusted_device_admin_approval( + address: &str, + org_name: &str, + dt: &NaiveDateTime, + ip: &str, + device: &str, +) -> EmptyResult { + let fmt = "%A, %B %_d, %Y at %r %Z"; + let (subject, body_html, body_text) = get_text( + "email/trusted_device_admin_approval", + json!({ + "url": CONFIG.domain(), + "img_src": CONFIG._smtp_img_src(), + "org_name": org_name, + "datetime": crate::util::format_naive_datetime_local(dt, fmt), + "ip": ip, + "device": device, + }), + )?; + + send_email(address, &subject, body_html, body_text).await +} + pub async fn send_incomplete_2fa_login( address: &str, ip: &str, diff --git a/src/static/templates/email/device_approval_requested.hbs b/src/static/templates/email/device_approval_requested.hbs new file mode 100644 index 00000000..e436c01c --- /dev/null +++ b/src/static/templates/email/device_approval_requested.hbs @@ -0,0 +1,6 @@ +Device Approval Requested + +{{user_name}} ({{user_email}}) is asking to have a new device approved in your {{org_name}} organization. Until an administrator approves it, they cannot get into their vault on that device. + +Review the request in the organization administration of {{{url}}}. +{{> email/email_footer_text }} diff --git a/src/static/templates/email/device_approval_requested.html.hbs b/src/static/templates/email/device_approval_requested.html.hbs new file mode 100644 index 00000000..ee66c98d --- /dev/null +++ b/src/static/templates/email/device_approval_requested.html.hbs @@ -0,0 +1,16 @@ +Device Approval Requested + +{{> email/email_header }} + + + + + + + +
+ {{user_name}} ({{user_email}}) is asking to have a new device approved in your {{org_name}} organization. Until an administrator approves it, they cannot get into their vault on that device. +
+ Review the request in the organization administration of {{{url}}}. +
+{{> email/email_footer }} diff --git a/src/static/templates/email/trusted_device_admin_approval.hbs b/src/static/templates/email/trusted_device_admin_approval.hbs new file mode 100644 index 00000000..af5fbcf2 --- /dev/null +++ b/src/static/templates/email/trusted_device_admin_approval.hbs @@ -0,0 +1,9 @@ +Device Approved + +An administrator of your {{org_name}} organization approved a device for your account on {{datetime}}. + +Device: {{device}} +IP address: {{ip}} + +If this was not you, change your password and contact your administrator. +{{> email/email_footer_text }} diff --git a/src/static/templates/email/trusted_device_admin_approval.html.hbs b/src/static/templates/email/trusted_device_admin_approval.html.hbs new file mode 100644 index 00000000..535cd41a --- /dev/null +++ b/src/static/templates/email/trusted_device_admin_approval.html.hbs @@ -0,0 +1,22 @@ +Device Approved + +{{> email/email_header }} + + + + + + + + + + +
+ An administrator of your {{org_name}} organization approved a device for your account on {{datetime}}. +
+ Device: {{device}}
+ IP address: {{ip}} +
+ If this was not you, change your password and contact your administrator. +
+{{> email/email_footer }} From e910b85a152eff335388c83824d44ca7b54019e4 Mon Sep 17 00:00:00 2001 From: tom27052006 <83423411+tom27052006@users.noreply.github.com> Date: Sat, 1 Aug 2026 00:14:49 +0200 Subject: [PATCH 03/10] Add a device approvals page to the admin panel Stand-in for the page of the same name in the admin console, which lives in the part of bitwarden/clients that is not AGPL licensed and is therefore in no web vault build. Without it the endpoints added earlier had no caller outside of scripts. The server cannot answer these requests by itself, and that is the point: it keeps the organization's private key encrypted with the organization key, and that one exists only as RSA envelopes addressed to each administrator. So the page ships markup and the whole chain runs in the browser against the regular API: master password -> master key PBKDF2-SHA256 -> own user key AES from profile.key -> own private key AES from profile.privateKey -> organization key RSA from profile.organizations[].key -> org private key AES from reset-password-details -> member's user key RSA from reset-password-details -> encryptedUserKey RSA for the public key of the request The master password itself never leaves the page; what goes out is the same login hash any sign-in sends. WebCrypto covers all of it except HKDF-Expand, which is done by hand because WebCrypto always runs the extract step first and Bitwarden uses the master key directly as the pseudorandom key. The page asks for the account of an administrator of the organization, not for the admin token, and says so: the two are not the same person on every installation, and the admin panel has held no user key material until now. --- src/api/admin.rs | 13 + src/api/web.rs | 3 + src/config.rs | 1 + src/static/scripts/admin_device_approvals.js | 342 ++++++++++++++++++ src/static/templates/admin/base.hbs | 3 + .../templates/admin/device_approvals.hbs | 68 ++++ 6 files changed, 430 insertions(+) create mode 100644 src/static/scripts/admin_device_approvals.js create mode 100644 src/static/templates/admin/device_approvals.hbs diff --git a/src/api/admin.rs b/src/api/admin.rs index 7037bfb1..2cd38792 100644 --- a/src/api/admin.rs +++ b/src/api/admin.rs @@ -67,6 +67,7 @@ pub fn routes() -> Vec { users_overview, organizations_overview, delete_organization, + device_approvals, diagnostics, get_diagnostics_config, resend_user_invite, @@ -615,6 +616,18 @@ async fn delete_organization(org_id: OrganizationId, _token: AdminToken, conn: D org.delete(&conn).await } +/// Stand-in for the "Device approvals" page of the admin console, which lives in the part of +/// bitwarden/clients that is not AGPL licensed and is therefore in no web vault build. +/// +/// This page only serves the markup. Everything else happens in the browser against the regular +/// API, because answering a request needs the master password of an administrator of the +/// organization: the server keeps its private key encrypted with a key it does not have. +#[get("/device-approvals")] +fn device_approvals(_token: AdminToken) -> ApiResult> { + let text = AdminTemplateData::new("admin/device_approvals", json!({})).render()?; + Ok(Html(text)) +} + #[derive(Deserialize)] struct GitRelease { tag_name: String, diff --git a/src/api/web.rs b/src/api/web.rs index 5bd4c85d..7bca3583 100644 --- a/src/api/web.rs +++ b/src/api/web.rs @@ -260,6 +260,9 @@ pub fn static_files(filename: &str) -> Result<(ContentType, &'static [u8]), Erro "admin_organizations.js" => { Ok((ContentType::JavaScript, include_bytes!("../static/scripts/admin_organizations.js"))) } + "admin_device_approvals.js" => { + Ok((ContentType::JavaScript, include_bytes!("../static/scripts/admin_device_approvals.js"))) + } "admin_diagnostics.js" => { Ok((ContentType::JavaScript, include_bytes!("../static/scripts/admin_diagnostics.js"))) } diff --git a/src/config.rs b/src/config.rs index 09b72907..aee67802 100644 --- a/src/config.rs +++ b/src/config.rs @@ -1765,6 +1765,7 @@ where reg!("admin/settings"); reg!("admin/users"); reg!("admin/organizations"); + reg!("admin/device_approvals"); reg!("admin/diagnostics"); reg!("404"); diff --git a/src/static/scripts/admin_device_approvals.js b/src/static/scripts/admin_device_approvals.js new file mode 100644 index 00000000..4b386323 --- /dev/null +++ b/src/static/scripts/admin_device_approvals.js @@ -0,0 +1,342 @@ +"use strict"; +/* eslint-env es2017, browser */ +/* global BASE_URL:readable */ + +// Answering a device approval means handing a member their own user key, encrypted for the key +// pair of the device that is asking. The server cannot do that: it holds the organization's +// private key only encrypted with the organization key, and that one exists solely as RSA +// envelopes addressed to each administrator. So the whole chain runs here, in the browser, and +// the master password never leaves this page. +// +// master password -> master key PBKDF2-SHA256(password, email, iterations) +// -> own user key AES from profile.key +// -> own private key AES from profile.privateKey +// -> organization key RSA from profile.organizations[].key +// -> org private key AES from reset-password-details.encryptedPrivateKey +// -> member's user key RSA from reset-password-details.resetPasswordKey +// -> encryptedUserKey RSA for the public key out of the request + +const DEVICE_IDENTIFIER_KEY = "vw_admin_device_approvals_device_id"; + +let session = null; // { token, profile, privateKey } +let requests = []; + +function element(id) { + return document.getElementById(id); +} + +function setStatus(message, kind) { + const box = element("approval-status"); + box.textContent = message; + box.className = message ? `alert alert-${kind || "info"}` : "d-none"; +} + +function fromBase64(value) { + return Uint8Array.from(atob(value), c => c.charCodeAt(0)); +} + +function toBase64(bytes) { + return btoa(String.fromCharCode(...new Uint8Array(bytes))); +} + +function concat(a, b) { + const out = new Uint8Array(a.length + b.length); + out.set(a, 0); + out.set(b, a.length); + return out; +} + +// --------------------------------------------------------------------------- crypto + +async function pbkdf2(password, salt, iterations) { + const key = await crypto.subtle.importKey("raw", password, "PBKDF2", false, ["deriveBits"]); + const bits = await crypto.subtle.deriveBits( + { name: "PBKDF2", salt: salt, iterations: iterations, hash: "SHA-256" }, key, 256); + return new Uint8Array(bits); +} + +// HKDF-Expand only, with the master key used directly as the pseudorandom key. WebCrypto's HKDF +// always runs the extract step first, which would give a different result, so this is by hand. +async function hkdfExpand(prk, info) { + const key = await crypto.subtle.importKey("raw", prk, { name: "HMAC", hash: "SHA-256" }, false, ["sign"]); + const input = concat(new TextEncoder().encode(info), new Uint8Array([1])); + return new Uint8Array(await crypto.subtle.sign("HMAC", key, input)); +} + +async function stretch(masterKey) { + return [await hkdfExpand(masterKey, "enc"), await hkdfExpand(masterKey, "mac")]; +} + +// A user key or organization key is 64 bytes: the AES half followed by the HMAC half. +async function splitKey(key) { + if (key.length === 32) { + return stretch(key); + } + if (key.length === 64) { + return [key.slice(0, 32), key.slice(32)]; + } + throw new Error(`Unexpected key length ${key.length}`); +} + +// EncString type 2: 2.iv|ciphertext|mac +async function decryptSymmetric(encString, encKey, macKey) { + const [kind, rest] = [encString.slice(0, encString.indexOf(".")), encString.slice(encString.indexOf(".") + 1)]; + if (kind !== "2") { + throw new Error(`Expected a symmetrically encrypted value, got type ${kind}`); + } + + const [iv, ciphertext, mac] = rest.split("|").map(fromBase64); + + const macCryptoKey = await crypto.subtle.importKey("raw", macKey, { name: "HMAC", hash: "SHA-256" }, false, ["verify"]); + if (!await crypto.subtle.verify("HMAC", macCryptoKey, mac, concat(iv, ciphertext))) { + throw new Error("The stored value does not match its signature. Wrong master password?"); + } + + const aesKey = await crypto.subtle.importKey("raw", encKey, { name: "AES-CBC" }, false, ["decrypt"]); + return new Uint8Array(await crypto.subtle.decrypt({ name: "AES-CBC", iv: iv }, aesKey, ciphertext)); +} + +// EncString type 4 or 6: RSA-OAEP with SHA-1. Type 6 carries an extra signature we do not need. +async function decryptAsymmetric(encString, privateKey) { + const kind = encString.slice(0, encString.indexOf(".")); + if (kind !== "4" && kind !== "6") { + throw new Error(`Expected an RSA encrypted value, got type ${kind}`); + } + + const data = fromBase64(encString.slice(encString.indexOf(".") + 1).split("|")[0]); + return new Uint8Array(await crypto.subtle.decrypt({ name: "RSA-OAEP" }, privateKey, data)); +} + +async function encryptAsymmetric(plain, publicKeyB64) { + const publicKey = await crypto.subtle.importKey( + "spki", fromBase64(publicKeyB64), { name: "RSA-OAEP", hash: "SHA-1" }, false, ["encrypt"]); + const encrypted = await crypto.subtle.encrypt({ name: "RSA-OAEP" }, publicKey, plain); + return "4." + toBase64(encrypted); +} + +async function importPrivateKey(pkcs8) { + return crypto.subtle.importKey("pkcs8", pkcs8, { name: "RSA-OAEP", hash: "SHA-1" }, false, ["decrypt"]); +} + +// --------------------------------------------------------------------------- api + +async function api(method, path, body, options) { + const settings = options || {}; + const headers = {}; + if (session && !settings.anonymous) { + headers["Authorization"] = `Bearer ${session.token}`; + } + + let payload = null; + if (body !== undefined && body !== null) { + if (settings.form) { + headers["Content-Type"] = "application/x-www-form-urlencoded"; + payload = new URLSearchParams(body).toString(); + } else { + headers["Content-Type"] = "application/json"; + payload = JSON.stringify(body); + } + } + + const response = await fetch(BASE_URL + path, { method: method, headers: headers, body: payload }); + const text = await response.text(); + let parsed = null; + try { + parsed = text ? JSON.parse(text) : null; + } catch (e) { + parsed = { message: text.slice(0, 200) }; + } + + if (!response.ok) { + throw new Error((parsed && (parsed.message || parsed.ErrorModel?.Message)) || `HTTP ${response.status}`); + } + return parsed; +} + +function deviceIdentifier() { + let identifier = localStorage.getItem(DEVICE_IDENTIFIER_KEY); + if (!identifier) { + identifier = crypto.randomUUID(); + localStorage.setItem(DEVICE_IDENTIFIER_KEY, identifier); + } + return identifier; +} + +// --------------------------------------------------------------------------- flow + +async function signIn(email, password) { + const prelogin = await api("POST", "/identity/accounts/prelogin", { email: email }, { anonymous: true }); + if (prelogin.kdf !== 0) { + throw new Error("This account uses Argon2, which this page does not implement. Use APPROVE_DEVICE from the command line."); + } + + const encoder = new TextEncoder(); + const masterKey = await pbkdf2(encoder.encode(password), encoder.encode(email.trim().toLowerCase()), prelogin.kdfIterations); + const passwordHash = toBase64(await pbkdf2(masterKey, encoder.encode(password), 1)); + + const token = await api("POST", "/identity/connect/token", { + grant_type: "password", + client_id: "web", + username: email, + password: passwordHash, + scope: "api offline_access", + deviceIdentifier: deviceIdentifier(), + deviceName: "Vaultwarden admin", + deviceType: 9, + }, { anonymous: true, form: true }); + + if (token.TwoFactorProviders || token.TwoFactorProviders2) { + throw new Error("Two-step login is active for this account, which this page does not implement."); + } + + session = { token: token.access_token }; + + const sync = await api("GET", "/api/sync?excludeDomains=true"); + const profile = sync.profile; + const userKey = await decryptSymmetric(profile.key, ...await stretch(masterKey)); + const privateKey = await importPrivateKey(await decryptSymmetric(profile.privateKey, ...await splitKey(userKey))); + + session = { token: token.access_token, profile: profile, privateKey: privateKey }; +} + +async function loadRequests() { + requests = []; + for (const org of session.profile.organizations) { + let pending; + try { + pending = await api("GET", `/api/organizations/${org.id}/auth-requests`); + } catch (e) { + continue; // not an administrator of this one + } + for (const request of pending.data) { + request.organization = org; + requests.push(request); + } + } +} + +async function memberUserKey(request) { + const org = request.organization; + const orgKey = await decryptAsymmetric(org.key, session.privateKey); + + const details = await api( + "GET", `/api/organizations/${org.id}/users/${request.organizationUserId}/reset-password-details`); + if (!details.resetPasswordKey) { + throw new Error("This member is not enrolled in account recovery, so nobody can hand out their key."); + } + + const orgPrivateKey = await importPrivateKey(await decryptSymmetric(details.encryptedPrivateKey, ...await splitKey(orgKey))); + return decryptAsymmetric(details.resetPasswordKey, orgPrivateKey); +} + +async function answer(request, approved) { + const path = `/api/organizations/${request.organization.id}/auth-requests/${request.id}`; + + if (!approved) { + await api("POST", path, { requestApproved: false }); + setStatus(`Denied the request from ${request.email}.`, "secondary"); + return; + } + + const encryptedUserKey = await encryptAsymmetric(await memberUserKey(request), request.publicKey); + await api("POST", path, { requestApproved: true, encryptedUserKey: encryptedUserKey }); + setStatus(`Approved. ${request.email} can open their vault on that device now.`, "success"); +} + +// --------------------------------------------------------------------------- rendering + +function renderRequests() { + const tbody = element("approval-rows"); + tbody.innerHTML = ""; + + element("approval-empty").classList.toggle("d-none", requests.length > 0); + element("approval-table").classList.toggle("d-none", requests.length === 0); + + requests.forEach((request, index) => { + const row = document.createElement("tr"); + + const cell = (text) => { + const td = document.createElement("td"); + td.textContent = text; + return td; + }; + + row.appendChild(cell(request.email)); + row.appendChild(cell(request.organization.name)); + row.appendChild(cell(request.requestDeviceType)); + row.appendChild(cell(request.requestIpAddress)); + row.appendChild(cell(new Date(request.creationDate).toLocaleString())); + + const actions = document.createElement("td"); + for (const [label, style, approved] of [["Approve", "btn-primary", true], ["Deny", "btn-outline-secondary", false]]) { + const button = document.createElement("button"); + button.type = "button"; + button.className = `btn btn-sm ${style} me-1`; + button.textContent = label; + button.addEventListener("click", () => void handleAnswer(index, approved, button)); + actions.appendChild(button); + } + row.appendChild(actions); + + tbody.appendChild(row); + }); +} + +function busy(on) { + document.querySelectorAll("#approval-rows button, #approval-reload").forEach(b => { b.disabled = on; }); +} + +async function handleAnswer(index, approved, button) { + busy(true); + button.textContent = approved ? "Approving..." : "Denying..."; + try { + await answer(requests[index], approved); + await refresh(); + } catch (e) { + setStatus(e.message, "danger"); + } finally { + busy(false); + } +} + +async function refresh() { + await loadRequests(); + renderRequests(); +} + +// --------------------------------------------------------------------------- wiring + +document.addEventListener("DOMContentLoaded", () => { + element("approval-signin").addEventListener("submit", async (event) => { + event.preventDefault(); + const button = element("approval-signin-button"); + button.disabled = true; + setStatus("Signing in and unlocking the keys...", "info"); + + try { + await signIn(element("approval-email").value.trim(), element("approval-password").value); + element("approval-password").value = ""; + element("approval-signin").classList.add("d-none"); + element("approval-list").classList.remove("d-none"); + element("approval-signed-in-as").textContent = session.profile.email; + await refresh(); + setStatus("", null); + } catch (e) { + session = null; + setStatus(e.message, "danger"); + } finally { + button.disabled = false; + } + }); + + element("approval-reload").addEventListener("click", async () => { + busy(true); + try { + await refresh(); + } catch (e) { + setStatus(e.message, "danger"); + } finally { + busy(false); + } + }); +}); diff --git a/src/static/templates/admin/base.hbs b/src/static/templates/admin/base.hbs index e1dcacb5..26923f45 100644 --- a/src/static/templates/admin/base.hbs +++ b/src/static/templates/admin/base.hbs @@ -45,6 +45,9 @@
  • Organizations
    • +
  • + Device approvals +
  • Diagnostics
    • diff --git a/src/static/templates/admin/device_approvals.hbs b/src/static/templates/admin/device_approvals.hbs new file mode 100644 index 00000000..354e2d55 --- /dev/null +++ b/src/static/templates/admin/device_approvals.hbs @@ -0,0 +1,68 @@ +
    +
    +
    Device approvals
    + +

    + A member who unlocks with a trusted device and has no other device of their own left to + ask can ask an administrator of their organization instead. Answering hands them their + own user key, encrypted for the device that is asking. It only works for members who + enrolled into account recovery. +

    + +
    + +
    +
    +
    + This asks for a vault account, not for the admin token. + The server cannot answer these requests on its own: it holds the organization's + private key only encrypted with a key that never leaves its members. So sign in + below as an administrator of the organization and the whole chain is + unwrapped here in your browser. Your master password is not sent anywhere; only + the same login hash a regular sign-in would send leaves this page. +
    +
    +
    + + +
    +
    + + +
    +
    + +
    +
    + +
    +

    + Signed in as +

    + +

    No requests are waiting for an answer.

    + +
    + + + + + + + + + + + + +
    MemberOrganizationDeviceIP addressAsked atActions
    +
    + +
    + +
    +
    +
    +
    + + From bed0e1c45a80bee0b8cc26f87e5f6cc284aa34d6 Mon Sep 17 00:00:00 2001 From: tom27052006 <83423411+tom27052006@users.noreply.github.com> Date: Sat, 1 Aug 2026 00:52:42 +0200 Subject: [PATCH 04/10] Withhold the trusted device options where they lead nowhere An account that has no keys yet and belongs to no organization cannot finish the trusted device flow. The clients enroll into account recovery as the last step of creating an account that way, unconditionally, and there is nothing to enroll into: the enrollment starts by fetching the organization's public key, which answers 401 without a membership. What the user sees is the screen for a new account, a failure halfway through it, and on the next attempt a refusal because the account keys are already written. Not offering the options for that one combination sends the client to setting a master password instead, which works and leaves the door open: the account can trust a device on its very next login. Everything else is unchanged. An account that already has keys goes through none of this, whether it belongs to an organization or not, so the master password first route and an account that already trusts a device are unaffected. The real fix is to have members in an organization by the time they first sign in, as upstream requires. This only makes the case where that did not happen end somewhere other than a dead end. --- src/api/identity.rs | 41 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 39 insertions(+), 2 deletions(-) diff --git a/src/api/identity.rs b/src/api/identity.rs index 37cd52ec..a29945a4 100644 --- a/src/api/identity.rs +++ b/src/api/identity.rs @@ -481,6 +481,18 @@ async fn password_login( authenticated_response(&user, &mut device, auth_tokens, twofactor_token, false, conn, ip).await } +/// Whether offering the trusted device options can lead anywhere for this account. +/// +/// Creating an account this way ends with enrolling into account recovery, which the clients do +/// unconditionally and which needs an organization to enroll into. An account that has nothing yet +/// and belongs to nowhere would therefore be shown the screen for a new account and get stuck +/// halfway through it, with its keys already written and its device still untrusted. Withholding +/// the options sends it to setting a master password instead, which works and leaves the door to +/// trusted devices open for the next login. +fn trusted_device_flow_is_completable(has_account_keys: bool, in_organization: bool) -> bool { + has_account_keys || in_organization +} + /// Trusted device encryption ("passwordless SSO"): instead of deriving the user key from a master /// password, the client keeps a copy of it on the device, wrapped for a key pair that the device /// generated. Its presence in the response is what makes the clients offer the flow at all. @@ -500,6 +512,12 @@ async fn trusted_device_option(user: &User, device: &Device, conn: &DbConn) -> O return None; } + let memberships = Membership::find_by_user(&user.uuid, conn).await; + + if !trusted_device_flow_is_completable(user.private_key.is_some(), !memberships.is_empty()) { + return None; + } + // Any other device of this user that could show an approval prompt. The user unlocks a new // device from one of these, or with the master password if they have one. let has_login_approving_device = Device::find_by_user(&user.uuid, conn) @@ -507,8 +525,6 @@ async fn trusted_device_option(user: &User, device: &Device, conn: &DbConn) -> O .iter() .any(|other| other.uuid != device.uuid && DeviceType::from_i32(other.atype).can_approve_login_requests()); - let memberships = Membership::find_by_user(&user.uuid, conn).await; - // An admin can only take over the approval once the member handed them a key to work with, // which is what enrolling into account recovery does. let has_admin_approval = @@ -1381,3 +1397,24 @@ async fn authorize(data: AuthorizeData, cookies: &CookieJar<'_>, secure: Secure, Ok(Redirect::temporary(String::from(auth_url))) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn an_account_with_nothing_and_nowhere_to_go_is_not_offered_trusted_devices() { + // The one combination the clients cannot finish: nothing set up yet and no organization + // to enroll into. + assert!(!trusted_device_flow_is_completable(false, false)); + + // A brand new account that was invited somewhere can enroll, so the flow completes. + assert!(trusted_device_flow_is_completable(false, true)); + + // An account that is already set up does not go through account creation at all, with or + // without an organization. This covers the master password first route as well as an + // account that already trusts a device. + assert!(trusted_device_flow_is_completable(true, false)); + assert!(trusted_device_flow_is_completable(true, true)); + } +} From 054a24900406afd5736614fc02866db12bd55f98 Mon Sep 17 00:00:00 2001 From: tom27052006 <83423411+tom27052006@users.noreply.github.com> Date: Sat, 1 Aug 2026 23:07:54 +0200 Subject: [PATCH 05/10] Harden the trusted device flow Findings from a review of the earlier commits. A key rotation dropped the device key pairs along with the wrapped user keys, so the devices listed in `/devices/update-trust` could never be trusted again and silently lost their trust on every rotation. Those key pairs are wrapped with the device key, which a rotation does not touch, so they now stay and the re-wrap works. The invalidation also moved ahead of the new user key: failing after it had been written left devices handing out a key that no longer opens the vault. The three key blobs are now checked against the shape of an `EncString` instead of only for emptiness, as upstream does, which also keeps multi-megabyte values out of those columns. `/auth-requests/admin-request` is rate limited and reuses the open request of a device instead of adding one per attempt, which mailed every administrator again each time. Only confirmed memberships are asked, and answered: an invitation that was never accepted is not a membership yet, a revoked one is not one anymore, and neither should learn the address, the IP and the device of the asker. `UserDecryptionOptions` reports the same condition, so no way out is announced that would be refused when taken. The bulk endpoints skip what they cannot answer rather than failing the whole call after having answered everything before it, matching upstream, and are bounded. The purge does its work per type in the database instead of reading the table and deleting row by row, with indexes to go with it. Also: approving an auth request is limited to the newest one of a device, `GET /auth-requests/` refuses an expired request like the anonymous lookup already did, the approval notification addresses the device that asked rather than the administrator's, and accepting an invitation while enrolling is tied to the trusted device flow it was meant for. --- .env.template | 10 +- .../down.sql | 2 + .../up.sql | 2 + .../down.sql | 2 + .../up.sql | 2 + .../down.sql | 2 + .../up.sql | 2 + src/api/core/accounts.rs | 153 +++++++++++++----- src/api/core/organizations.rs | 118 ++++++++++++-- src/api/identity.rs | 15 +- src/config.rs | 6 +- src/db/models/auth_request.rs | 92 ++++++++--- src/db/models/device.rs | 65 ++++++-- src/mail.rs | 4 +- .../email/device_approval_requested.hbs | 2 +- .../email/device_approval_requested.html.hbs | 2 +- .../email/trusted_device_admin_approval.hbs | 2 +- .../trusted_device_admin_approval.html.hbs | 2 +- src/util.rs | 89 ++++++++++ 19 files changed, 468 insertions(+), 104 deletions(-) create mode 100644 migrations/mysql/2026-08-01-120000_add_auth_request_indexes/down.sql create mode 100644 migrations/mysql/2026-08-01-120000_add_auth_request_indexes/up.sql create mode 100644 migrations/postgresql/2026-08-01-120000_add_auth_request_indexes/down.sql create mode 100644 migrations/postgresql/2026-08-01-120000_add_auth_request_indexes/up.sql create mode 100644 migrations/sqlite/2026-08-01-120000_add_auth_request_indexes/down.sql create mode 100644 migrations/sqlite/2026-08-01-120000_add_auth_request_indexes/up.sql diff --git a/.env.template b/.env.template index 375a99e5..dbae390a 100644 --- a/.env.template +++ b/.env.template @@ -552,9 +552,13 @@ ## Trusted device encryption ("passwordless SSO"), see https://bitwarden.com/help/login-with-sso-trusted-devices/ ## After an SSO login the client may keep a copy of the user key on the device, wrapped for a key ## pair that the device generated, so the vault unlocks without a master password. A second device -## is unlocked either by approving it from an already trusted one or with the master password. -## WARNING: a user who never sets a master password and then loses every trusted device cannot -## recover their vault. This server does not implement the admin approval flow to fall back on. +## is unlocked by approving it from an already trusted one, by asking an administrator of the +## organization (which needs the member enrolled into account recovery), or with the master password. +## WARNING: a user who never sets a master password and then loses every trusted device depends on +## an administrator to get back in, and cannot recover their vault at all without one. +## To turn this off again, clear this setting but leave `SSO_ENABLED` on: users without a master +## password keep receiving their keys while they still have a trusted device, so their client can +## walk them through setting one. Turning off `SSO_ENABLED` instead leaves them no way to log in. # SSO_TRUSTED_DEVICE_ENCRYPTION=false ######################## diff --git a/migrations/mysql/2026-08-01-120000_add_auth_request_indexes/down.sql b/migrations/mysql/2026-08-01-120000_add_auth_request_indexes/down.sql new file mode 100644 index 00000000..38258616 --- /dev/null +++ b/migrations/mysql/2026-08-01-120000_add_auth_request_indexes/down.sql @@ -0,0 +1,2 @@ +DROP INDEX auth_requests_organization_type ON auth_requests; +DROP INDEX auth_requests_creation_date ON auth_requests; diff --git a/migrations/mysql/2026-08-01-120000_add_auth_request_indexes/up.sql b/migrations/mysql/2026-08-01-120000_add_auth_request_indexes/up.sql new file mode 100644 index 00000000..a757bdef --- /dev/null +++ b/migrations/mysql/2026-08-01-120000_add_auth_request_indexes/up.sql @@ -0,0 +1,2 @@ +CREATE INDEX auth_requests_organization_type ON auth_requests (organization_uuid, atype, approved); +CREATE INDEX auth_requests_creation_date ON auth_requests (creation_date); diff --git a/migrations/postgresql/2026-08-01-120000_add_auth_request_indexes/down.sql b/migrations/postgresql/2026-08-01-120000_add_auth_request_indexes/down.sql new file mode 100644 index 00000000..9f880bae --- /dev/null +++ b/migrations/postgresql/2026-08-01-120000_add_auth_request_indexes/down.sql @@ -0,0 +1,2 @@ +DROP INDEX auth_requests_organization_type; +DROP INDEX auth_requests_creation_date; diff --git a/migrations/postgresql/2026-08-01-120000_add_auth_request_indexes/up.sql b/migrations/postgresql/2026-08-01-120000_add_auth_request_indexes/up.sql new file mode 100644 index 00000000..a757bdef --- /dev/null +++ b/migrations/postgresql/2026-08-01-120000_add_auth_request_indexes/up.sql @@ -0,0 +1,2 @@ +CREATE INDEX auth_requests_organization_type ON auth_requests (organization_uuid, atype, approved); +CREATE INDEX auth_requests_creation_date ON auth_requests (creation_date); diff --git a/migrations/sqlite/2026-08-01-120000_add_auth_request_indexes/down.sql b/migrations/sqlite/2026-08-01-120000_add_auth_request_indexes/down.sql new file mode 100644 index 00000000..9f880bae --- /dev/null +++ b/migrations/sqlite/2026-08-01-120000_add_auth_request_indexes/down.sql @@ -0,0 +1,2 @@ +DROP INDEX auth_requests_organization_type; +DROP INDEX auth_requests_creation_date; diff --git a/migrations/sqlite/2026-08-01-120000_add_auth_request_indexes/up.sql b/migrations/sqlite/2026-08-01-120000_add_auth_request_indexes/up.sql new file mode 100644 index 00000000..a757bdef --- /dev/null +++ b/migrations/sqlite/2026-08-01-120000_add_auth_request_indexes/up.sql @@ -0,0 +1,2 @@ +CREATE INDEX auth_requests_organization_type ON auth_requests (organization_uuid, atype, approved); +CREATE INDEX auth_requests_creation_date ON auth_requests (creation_date); diff --git a/src/api/core/accounts.rs b/src/api/core/accounts.rs index a831f0df..22282d28 100644 --- a/src/api/core/accounts.rs +++ b/src/api/core/accounts.rs @@ -22,8 +22,8 @@ use crate::{ models::{ AuthRequest, AuthRequestId, AuthRequestType, Cipher, CipherId, Device, DeviceId, DeviceType, DeviceWithAuthRequest, EmergencyAccess, EmergencyAccessId, EventType, Folder, FolderId, Invitation, - Membership, MembershipId, MembershipType, OrgPolicy, OrgPolicyType, Organization, OrganizationId, Send, - SendId, User, UserId, UserKdfType, + Membership, MembershipId, MembershipStatus, MembershipType, OrgPolicy, OrgPolicyType, Organization, + OrganizationId, Send, SendId, User, UserId, UserKdfType, }, }, mail, @@ -1022,6 +1022,14 @@ async fn post_rotatekey(data: Json, headers: Headers, conn: DbConn, nt: } } + // Every device holds the previous user key wrapped for itself, which unlocks nothing anymore. + // Drop those copies before the new key is written, never after: the other order leaves a window + // in which a device still counts as trusted and hands its owner a key that no longer opens the + // vault. This way a failure here means the rotation simply did not happen. + // The clients re-wrap the new user key for every device right after this via + // `POST /devices/update-trust`; whatever they leave out stays untrusted. + Device::invalidate_wrapped_user_keys(&headers.user.uuid, &conn).await?; + // Update user data let mut user = headers.user; @@ -1037,12 +1045,6 @@ async fn post_rotatekey(data: Json, headers: Headers, conn: DbConn, nt: let save_result = user.save(&conn).await; - // Every trusted device holds the previous user key wrapped for itself, which unlocks nothing - // anymore. The client of the rotating device re-wraps the new one right after this via - // `POST /devices/update-trust`, so that device keeps its private key; the rest is dropped. A - // client that skips that call simply ends up with no trusted device instead of a broken unlock. - Device::invalidate_wrapped_user_keys(&user.uuid, &headers.device.uuid, &conn).await?; - // 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. @@ -1608,6 +1610,20 @@ struct TrustedDeviceKeysData { encrypted_private_key: String, } +/// Refuses anything that does not even have the shape of an `EncString`. +/// +/// The server cannot tell whether a blob decrypts, but storing something that certainly does not +/// only leaves a device that calls itself trusted and fails its owner at the next unlock. Upstream +/// puts `[EncryptedString]` on the same fields. +fn validate_enc_strings(values: &[(&str, &str)]) -> EmptyResult { + for (name, value) in values { + if !crate::util::is_valid_enc_string(value) { + err!(format!("{name} is not a valid encrypted string")) + } + } + Ok(()) +} + /// Marks a device of the current user as trusted. /// /// Upstream keys this on the device identifier and does not require it to be the device the request @@ -1622,12 +1638,11 @@ async fn put_device_keys( ) -> JsonResult { let data = data.into_inner(); - if data.encrypted_user_key.is_empty() - || data.encrypted_public_key.is_empty() - || data.encrypted_private_key.is_empty() - { - err!("All three device keys are required to trust a device") - } + validate_enc_strings(&[ + ("encryptedUserKey", &data.encrypted_user_key), + ("encryptedPublicKey", &data.encrypted_public_key), + ("encryptedPrivateKey", &data.encrypted_private_key), + ])?; let Some(mut device) = Device::find_by_uuid_and_user(&device_id, &headers.user.uuid, &conn).await else { err!("No device found") @@ -1698,18 +1713,20 @@ async fn post_devices_update_trust(data: Json, headers: data.secret.validate(&headers.user, true, &conn).await?; - if data.current_device.encrypted_user_key.is_empty() || data.current_device.encrypted_public_key.is_empty() { - err!("The keys of the current device are required") - } + validate_enc_strings(&[ + ("encryptedUserKey", &data.current_device.encrypted_user_key), + ("encryptedPublicKey", &data.current_device.encrypted_public_key), + ])?; let mut updates: HashMap = HashMap::new(); for other in data.other_devices { if other.device_id == headers.device.uuid { err!("The current device cannot also be part of the optional rotation") } - if other.keys.encrypted_user_key.is_empty() || other.keys.encrypted_public_key.is_empty() { - err!("Both keys are required for every device in the rotation") - } + validate_enc_strings(&[ + ("encryptedUserKey", &other.keys.encrypted_user_key), + ("encryptedPublicKey", &other.keys.encrypted_public_key), + ])?; if updates.insert(other.device_id, other.keys).is_some() { err!("A device was listed more than once in the rotation") } @@ -1730,15 +1747,20 @@ async fn post_devices_update_trust(data: Json, headers: if device.uuid == headers.device.uuid { device.encrypted_user_key = Some(data.current_device.encrypted_user_key.clone()); device.encrypted_public_key = Some(data.current_device.encrypted_public_key.clone()); - } else if !device.is_trusted() { - // Nothing to rotate, and handing an untrusted device two of the three keys would not - // make it trusted anyway. - continue; } else if let Some(keys) = updates.remove(&device.uuid) { + // A rotation clears the wrapped user key of every device, so the listed ones are not + // trusted at this point; their key pair is what they are restored from. Without it + // there is nothing the two keys could belong to. + if !device.holds_private_key() { + continue; + } device.encrypted_user_key = Some(keys.encrypted_user_key); device.encrypted_public_key = Some(keys.encrypted_public_key); - } else { + } else if device.holds_any_key() { + // Not listed, so whatever it still holds wraps the previous user key. device.untrust(); + } else { + continue; } device.save(true, &conn).await?; @@ -1892,6 +1914,10 @@ async fn post_auth_request( /// https://github.com/bitwarden/server/blob/main/src/Api/Auth/Controllers/AuthRequestsController.cs #[post("/auth-requests/admin-request", data = "")] async fn post_admin_auth_request(data: Json, headers: Headers, conn: DbConn) -> JsonResult { + // Every call mails all administrators of every organization involved, so it is worth a limit of + // its own even though the caller is authenticated. + crate::ratelimit::check_limit_unauthenticated(&headers.ip.ip)?; + let data = data.into_inner(); if AuthRequestType::from_i32(data.atype) != Some(AuthRequestType::AdminApproval) { @@ -1902,9 +1928,16 @@ async fn post_admin_auth_request(data: Json, headers: Header err!("AuthRequest doesn't exist", "Device verification failed") } - let memberships = Membership::find_by_user(&headers.user.uuid, &conn).await; + // Only an organization the user really belongs to can answer for them. A pending invitation is + // not a membership yet, and a revoked one is not one anymore; sending either of them the email + // address, the address and the device of the asker is more than they are owed. + let memberships: Vec = Membership::find_by_user(&headers.user.uuid, &conn) + .await + .into_iter() + .filter(|membership| membership.status == MembershipStatus::Confirmed as i32) + .collect(); if memberships.is_empty() { - err!("User does not belong to any organization") + err!("User does not belong to any organization that could approve a device") } log_user_event( @@ -1918,19 +1951,42 @@ async fn post_admin_auth_request(data: Json, headers: Header let mut first_request = None; for membership in memberships { - let mut auth_request = AuthRequest::new( - headers.user.uuid.clone(), - Some(membership.org_uuid.clone()), - AuthRequestType::AdminApproval, - data.device_identifier.clone(), - headers.device.atype, - headers.ip.ip.to_string(), - data.access_code.clone(), - data.public_key.clone(), - ); + // Asking again from the same device replaces the open request instead of adding one, so a + // client that retries does not pile up rows and does not mail the administrators twice. + let existing = AuthRequest::find_pending_admin_approval( + &headers.user.uuid, + &data.device_identifier, + &membership.org_uuid, + &conn, + ) + .await; + let is_new = existing.is_none(); + + let mut auth_request = match existing { + Some(mut auth_request) => { + auth_request.access_code.clone_from(&data.access_code); + auth_request.public_key.clone_from(&data.public_key); + auth_request.device_type = headers.device.atype; + auth_request.request_ip = headers.ip.ip.to_string(); + auth_request.creation_date = Utc::now().naive_utc(); + auth_request + } + None => AuthRequest::new( + headers.user.uuid.clone(), + Some(membership.org_uuid.clone()), + AuthRequestType::AdminApproval, + data.device_identifier.clone(), + headers.device.atype, + headers.ip.ip.to_string(), + data.access_code.clone(), + data.public_key.clone(), + ), + }; auth_request.save(&conn).await?; - notify_device_approval_requested(&headers.user, &membership.org_uuid, &conn).await; + if is_new { + notify_device_approval_requested(&headers.user, &membership.org_uuid, &conn).await; + } if first_request.is_none() { first_request = Some(auth_request); @@ -1976,6 +2032,12 @@ async fn get_auth_request(auth_request_id: AuthRequestId, headers: Headers, conn err!("AuthRequest doesn't exist", "Record not found or user uuid does not match") }; + // The anonymous lookup refuses an expired request, and so does this one: the window an approval + // stays usable in should not depend on which of the two the client happens to poll. + if auth_request.is_expired() { + err!("AuthRequest doesn't exist", "Request has expired") + } + Ok(Json(auth_request_json(&auth_request))) } @@ -2022,6 +2084,21 @@ async fn put_auth_request( err!("AuthRequest doesn't exist", "Request has expired") } + // Only the newest request of a device may be approved. Anyone can create a request for a known + // device, so without this an older one could still be sitting there when the user approves what + // their screen shows, and the answer would go to whoever left it. Same check as upstream. + if data.request_approved + && AuthRequest::find_by_user_and_requested_device( + &headers.user.uuid, + &auth_request.request_device_identifier, + &conn, + ) + .await + .is_none_or(|newest| newest.uuid != auth_request.uuid) + { + err!("This request is no longer valid. Make sure to approve the most recent request.") + } + let response_date = Utc::now().naive_utc(); if data.request_approved { diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index 197480f3..b6a207c6 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -17,8 +17,8 @@ use crate::{ DbConn, models::{ AuthRequest, AuthRequestId, Cipher, CipherId, Collection, CollectionCipher, CollectionGroup, CollectionId, - CollectionUser, DeviceType, EventType, Group, GroupId, GroupUser, Invitation, Membership, MembershipId, - MembershipStatus, MembershipType, OrgPolicy, OrgPolicyType, Organization, OrganizationApiKey, + CollectionUser, Device, DeviceType, EventType, Group, GroupId, GroupUser, Invitation, Membership, + MembershipId, MembershipStatus, MembershipType, OrgPolicy, OrgPolicyType, Organization, OrganizationApiKey, OrganizationId, User, UserId, }, }, @@ -3181,7 +3181,13 @@ async fn put_reset_password_enrollment( // Enrolling is where a member who was invited into a trusted device organization turns into a // real one; upstream accepts the invitation at this point as well. Without it they would stay // invited forever and no admin could ever confirm them. - if enrolled && membership.status == MembershipStatus::Invited as i32 { + // + // Tied to the same condition as the exception above, so that turning the feature off leaves the + // invitation flow exactly as it was: an invitation is otherwise accepted only against the token + // that was mailed out, and that is the only thing proving the address belongs to the account. + if enrolled && trusted_device_enrollment && membership.status == MembershipStatus::Invited as i32 { + // Do not leave the open invitation behind, it would keep the address signup-eligible. + Invitation::take(&headers.user.email, &conn).await; accept_org_invite(&headers.user, membership, reset_password_key, &conn).await?; } else { membership.reset_password_key = reset_password_key; @@ -3219,14 +3225,18 @@ async fn get_organization_auth_requests(org_id: OrganizationId, headers: AdminHe continue; } - // A request whose asker is no longer a member of this organization is none of its business - // anymore, so it is quietly left out instead of being offered for approval. + // A request whose asker is not a confirmed member of this organization is none of its + // business, so it is quietly left out instead of being offered for approval. Same condition + // as when answering, so nothing is shown here that would be refused there. let (Some(member), Some(user)) = ( Membership::find_by_user_and_org(&auth_request.user_uuid, &org_id, &conn).await, User::find_by_uuid(&auth_request.user_uuid, &conn).await, ) else { continue; }; + if member.status != MembershipStatus::Confirmed as i32 { + continue; + } requests.push(auth_request.to_json_for_organization(&user.email, &member.uuid)); } @@ -3259,6 +3269,22 @@ struct OrganizationAuthRequestUpdateData { key: Option, } +/// How many requests one call may answer. A screen full of pending approvals is a handful. +const MAX_BULK_AUTH_REQUESTS: usize = 500; + +/// Whether one entry that cannot be answered takes the whole call down with it. +/// +/// A single request is addressed by its id, so a caller that names a request nobody can answer +/// deserves to hear about it. A batch is a list of what an administrator saw a moment ago, where an +/// entry may well have expired or been answered by a colleague since; upstream processes those as +/// far as it can and passes over the rest. Failing the batch instead would report an error while +/// having already answered everything before the bad entry. +#[derive(Clone, Copy, PartialEq, Eq)] +enum OnUnanswerable { + Fail, + Skip, +} + #[post("/organizations//auth-requests/", data = "", rank = 2)] async fn update_organization_auth_request( org_id: OrganizationId, @@ -3275,6 +3301,7 @@ async fn update_organization_auth_request( &request_id, data.request_approved, data.encrypted_user_key, + OnUnanswerable::Fail, &headers, &conn, &ant, @@ -3292,8 +3319,24 @@ async fn deny_organization_auth_requests( ant: AnonymousNotify<'_>, nt: Notify<'_>, ) -> EmptyResult { - for request_id in data.into_inner().ids { - answer_organization_auth_request(&org_id, &request_id, false, None, &headers, &conn, &ant, &nt).await?; + let ids = data.into_inner().ids; + if ids.len() > MAX_BULK_AUTH_REQUESTS { + err!(format!("At most {MAX_BULK_AUTH_REQUESTS} requests can be answered at once")) + } + + for request_id in ids { + answer_organization_auth_request( + &org_id, + &request_id, + false, + None, + OnUnanswerable::Skip, + &headers, + &conn, + &ant, + &nt, + ) + .await?; } Ok(()) @@ -3308,9 +3351,24 @@ async fn update_many_organization_auth_requests( ant: AnonymousNotify<'_>, nt: Notify<'_>, ) -> EmptyResult { - for update in data.into_inner() { - answer_organization_auth_request(&org_id, &update.id, update.approved, update.key, &headers, &conn, &ant, &nt) - .await?; + let updates = data.into_inner(); + if updates.len() > MAX_BULK_AUTH_REQUESTS { + err!(format!("At most {MAX_BULK_AUTH_REQUESTS} requests can be answered at once")) + } + + for update in updates { + answer_organization_auth_request( + &org_id, + &update.id, + update.approved, + update.key, + OnUnanswerable::Skip, + &headers, + &conn, + &ant, + &nt, + ) + .await?; } Ok(()) @@ -3322,6 +3380,7 @@ async fn answer_organization_auth_request( request_id: &AuthRequestId, approved: bool, encrypted_user_key: Option, + on_unanswerable: OnUnanswerable, headers: &AdminHeaders, conn: &DbConn, ant: &AnonymousNotify<'_>, @@ -3331,30 +3390,47 @@ async fn answer_organization_auth_request( err!("Organization not found", "Organization id's do not match"); } + // Everything below this point is a request that this administrator cannot answer, whether it + // never existed, was already dealt with, or ran out. In a batch that is expected and skipped. + macro_rules! unanswerable { + ($($err:tt)*) => {{ + if on_unanswerable == OnUnanswerable::Skip { + return Ok(()); + } + err!($($err)*) + }}; + } + // Only ever reachable through the organization it was addressed to, so an administrator cannot // answer for an organization they have no say in. let Some(mut auth_request) = AuthRequest::find_admin_approval_by_org_and_uuid(request_id, org_id, conn).await else { - err!("AuthRequest doesn't exist", "Record not found or not addressed to this organization") + unanswerable!("AuthRequest doesn't exist", "Record not found or not addressed to this organization") }; if auth_request.approved.is_some() { - err!("This request has already been answered") + unanswerable!("This request has already been answered"); } if auth_request.is_expired() { - err!("AuthRequest doesn't exist", "Request has expired") + unanswerable!("AuthRequest doesn't exist", "Request has expired"); } - let Some(member) = Membership::find_by_user_and_org(&auth_request.user_uuid, org_id, conn).await else { - err!("AuthRequest doesn't exist", "The requesting user is no longer a member of this organization") + // Answering means acting for a member of this organization, so it has to be one: an invitation + // that was never accepted is not a membership yet, and a revoked one is not one anymore. + let member = match Membership::find_by_user_and_org(&auth_request.user_uuid, org_id, conn).await { + Some(member) if member.status == MembershipStatus::Confirmed as i32 => member, + _ => unanswerable!("AuthRequest doesn't exist", "The requesting user is not a member of this organization"), }; if approved { // Without the wrapped user key the answer is worthless: it is the whole point of approving. let Some(key) = encrypted_user_key.filter(|key| !key.is_empty()) else { - err!("An approved request needs the encrypted user key") + unanswerable!("An approved request needs the encrypted user key") }; + if !crate::util::is_valid_enc_string(&key) { + unanswerable!("encryptedUserKey is not a valid encrypted string"); + } auth_request.enc_key = Some(key); } @@ -3376,7 +3452,15 @@ async fn answer_organization_auth_request( } ant.send_auth_response(&auth_request.user_uuid, &auth_request.uuid).await; - nt.send_auth_response(&auth_request.user_uuid, &auth_request.uuid, &headers.device, conn).await; + + // The device that asked, not the one the administrator happens to be answering from: that one + // belongs to somebody else, and naming it here would both address the notification at a device + // of the wrong account and hand its identifiers to the push relay under a foreign user id. + if let Some(device) = + Device::find_by_uuid_and_user(&auth_request.request_device_identifier, &auth_request.user_uuid, conn).await + { + nt.send_auth_response(&auth_request.user_uuid, &auth_request.uuid, &device, conn).await; + } if CONFIG.mail_enabled() && let Some(user) = User::find_by_uuid(&auth_request.user_uuid, conn).await diff --git a/src/api/identity.rs b/src/api/identity.rs index a29945a4..aad29d1a 100644 --- a/src/api/identity.rs +++ b/src/api/identity.rs @@ -526,14 +526,19 @@ async fn trusted_device_option(user: &User, device: &Device, conn: &DbConn) -> O .any(|other| other.uuid != device.uuid && DeviceType::from_i32(other.atype).can_approve_login_requests()); // An admin can only take over the approval once the member handed them a key to work with, - // which is what enrolling into account recovery does. - let has_admin_approval = - memberships.iter().any(|member| member.reset_password_key.as_ref().is_some_and(|key| !key.is_empty())); + // which is what enrolling into account recovery does. Only a confirmed membership counts, the + // same condition the request itself is created and answered under, so this does not announce a + // way out that would be refused the moment it is taken. + let has_admin_approval = memberships.iter().any(|member| { + member.status == MembershipStatus::Confirmed as i32 + && member.reset_password_key.as_ref().is_some_and(|key| !key.is_empty()) + }); // Whether the user is on the answering side of that. The clients use it to push someone who - // could approve others, but has no master password themselves, into setting one. + // could approve others, but has no master password themselves, into setting one. Matches what + // `AdminHeaders` actually lets through. let has_manage_reset_password_permission = memberships.iter().any(|member| { - member.status != MembershipStatus::Revoked as i32 && member.atype <= MembershipType::Admin as i32 + member.status == MembershipStatus::Confirmed as i32 && member.atype <= MembershipType::Admin as i32 }); Some(json!({ diff --git a/src/config.rs b/src/config.rs index aee67802..f581697f 100644 --- a/src/config.rs +++ b/src/config.rs @@ -1110,7 +1110,11 @@ fn validate_config(cfg: &ConfigItems, on_update: bool) -> Result<(), Error> { validate_internal_sso_redirect_url(&cfg.sso_callback_path)?; validate_sso_master_password_policy(cfg.sso_master_password_policy.as_ref())?; } else if cfg.sso_trusted_device_encryption { - err!("`SSO_TRUSTED_DEVICE_ENCRYPTION` requires `SSO_ENABLED` to be set, it only applies to SSO logins") + err!( + "`SSO_TRUSTED_DEVICE_ENCRYPTION` requires `SSO_ENABLED` to be set, it only applies to SSO logins. \ + To stop offering trusted devices, clear `SSO_TRUSTED_DEVICE_ENCRYPTION` and leave `SSO_ENABLED` on \ + until every user without a master password has set one, otherwise they can no longer log in at all" + ) } if cfg._enable_yubico { diff --git a/src/db/models/auth_request.rs b/src/db/models/auth_request.rs index 4a1a1124..0472f62f 100644 --- a/src/db/models/auth_request.rs +++ b/src/db/models/auth_request.rs @@ -237,6 +237,30 @@ impl AuthRequest { .await } + /// The open request a device already has waiting at this organization, if any. + /// + /// Asking again from the same device updates that one instead of adding another, so a client + /// that retries cannot fill the table or mail the administrators over and over. + pub async fn find_pending_admin_approval( + user_uuid: &UserId, + device_uuid: &DeviceId, + org_uuid: &OrganizationId, + conn: &DbConn, + ) -> Option { + conn.run(move |conn| { + auth_requests::table + .filter(auth_requests::user_uuid.eq(user_uuid)) + .filter(auth_requests::request_device_identifier.eq(device_uuid)) + .filter(auth_requests::organization_uuid.eq(org_uuid)) + .filter(auth_requests::atype.eq(AuthRequestType::AdminApproval as i32)) + .filter(auth_requests::approved.is_null()) + .order_by(auth_requests::creation_date.desc()) + .first::(conn) + .ok() + }) + .await + } + /// Everything an administrator of this organization still has to answer. pub async fn find_pending_admin_approval_by_org(org_uuid: &OrganizationId, conn: &DbConn) -> Vec { conn.run(move |conn| { @@ -269,16 +293,6 @@ impl AuthRequest { .await } - pub async fn find_created_before(dt: &NaiveDateTime, conn: &DbConn) -> Vec { - conn.run(move |conn| { - auth_requests::table - .filter(auth_requests::creation_date.lt(dt)) - .load::(conn) - .expect("Error loading auth_requests") - }) - .await - } - pub async fn delete(&self, conn: &DbConn) -> EmptyResult { conn.run(move |conn| { diesel::delete(auth_requests::table.filter(auth_requests::uuid.eq(&self.uuid))) @@ -292,16 +306,56 @@ impl AuthRequest { ct_eq(&self.access_code, access_code) } + /// Drops everything past its window, which is a different one per type. + /// + /// https://github.com/bitwarden/server/blob/f8ee2270409f7a13125cd414c450740af605a175/src/Sql/dbo/Auth/Stored%20Procedures/AuthRequest_DeleteIfExpired.sql + /// One statement per case rather than reading the table and deleting row by row, so the work + /// stays in the database however many requests have piled up. pub async fn purge_expired_auth_requests(conn: &DbConn) { - // https://github.com/bitwarden/server/blob/f8ee2270409f7a13125cd414c450740af605a175/src/Sql/dbo/Auth/Stored%20Procedures/AuthRequest_DeleteIfExpired.sql - // Nothing can be expired before the shortest window has passed, so that is the cheapest - // way to narrow the table down; which of them really are is decided per type afterwards, - // because a request waiting for an administrator lives a week rather than 15 minutes. - let candidates = Utc::now().naive_utc() - Self::user_request_expiration(); - for auth_request in Self::find_created_before(&candidates, conn).await { - if auth_request.is_expired() { - auth_request.delete(conn).await.ok(); - } + let now = Utc::now().naive_utc(); + let admin = AuthRequestType::AdminApproval as i32; + + let between_devices = now - Self::user_request_expiration(); + let for_an_admin = now - Self::admin_request_expiration(); + let after_approval = now - Self::after_admin_approval_expiration(); + + let result = conn + .run(move |conn| -> EmptyResult { + // Between the user's own devices: 15 minutes from the moment it was asked. + let _: () = diesel::delete( + auth_requests::table + .filter(auth_requests::atype.ne(admin)) + .filter(auth_requests::creation_date.lt(between_devices)), + ) + .execute(conn) + .map_res("Error purging the expired auth requests")?; + + // Approved by an administrator: half a day from the answer, so the user has time to + // come back and use it. + let _: () = diesel::delete( + auth_requests::table + .filter(auth_requests::atype.eq(admin)) + .filter(auth_requests::approved.eq(true)) + .filter(auth_requests::response_date.lt(after_approval)), + ) + .execute(conn) + .map_res("Error purging the approved auth requests")?; + + // Waiting for an administrator, or refused by one: a week from the moment it was + // asked either way, a refusal does not extend anything. + diesel::delete( + auth_requests::table + .filter(auth_requests::atype.eq(admin)) + .filter(auth_requests::approved.is_null().or(auth_requests::approved.eq(false))) + .filter(auth_requests::creation_date.lt(for_an_admin)), + ) + .execute(conn) + .map_res("Error purging the unanswered auth requests") + }) + .await; + + if let Err(e) = result { + error!("Error purging the expired auth requests: {e:#?}"); } } } diff --git a/src/db/models/device.rs b/src/db/models/device.rs index 3d7391ec..c0192c8a 100644 --- a/src/db/models/device.rs +++ b/src/db/models/device.rs @@ -107,6 +107,22 @@ impl Device { self.is_trusted().then_some(self.encrypted_private_key.as_ref()).flatten() } + /// Whether the device still holds the private key of its own key pair. + /// + /// That key is wrapped with the device key, which a rotation of the user key does not touch, so + /// it outlives one. It is what decides whether a device can be handed a freshly wrapped user + /// key and be trusted again, or whether it has to be set up from scratch. + pub fn holds_private_key(&self) -> bool { + Self::present(self.encrypted_private_key.as_ref()).is_some() + } + + /// Whether any part of a trust is stored, complete or not. + pub fn holds_any_key(&self) -> bool { + Self::present(self.encrypted_user_key.as_ref()).is_some() + || Self::present(self.encrypted_public_key.as_ref()).is_some() + || self.holds_private_key() + } + pub fn untrust(&mut self) { self.encrypted_user_key = None; self.encrypted_public_key = None; @@ -249,23 +265,15 @@ impl Device { /// Invalidates every copy of the user key that is wrapped for one of the user's devices. /// /// Called when the user key itself is replaced, which leaves all of those copies pointing at a - /// key that no longer unlocks anything. `keep_private_key_for` (the device that performed the - /// rotation) keeps its own private key, so its client can immediately re-wrap the new user key - /// via `POST /devices/update-trust`; every other device is untrusted outright. Until that - /// happens no device counts as trusted, so the worst case is an extra login, not a broken vault. - pub async fn invalidate_wrapped_user_keys( - user_uuid: &UserId, - keep_private_key_for: &DeviceId, - conn: &DbConn, - ) -> EmptyResult { + /// key that no longer unlocks anything. No device counts as trusted afterwards, so a client + /// that stops here ends up with an extra login rather than a broken unlock. The device key + /// pairs are deliberately left alone: they are wrapped with the device key, which a rotation + /// does not touch, so `POST /devices/update-trust` can hand every device the new user key and + /// restore its trust. Whatever it does not list is dropped there. + /// + /// One statement, so there is no half applied state to reason about. + pub async fn invalidate_wrapped_user_keys(user_uuid: &UserId, conn: &DbConn) -> EmptyResult { conn.run(move |conn| { - let _: () = diesel::update( - devices::table.filter(devices::user_uuid.eq(user_uuid)).filter(devices::uuid.ne(keep_private_key_for)), - ) - .set(devices::encrypted_private_key.eq::>(None)) - .execute(conn) - .map_res("Error untrusting the devices")?; - diesel::update(devices::table.filter(devices::user_uuid.eq(user_uuid))) .set(( devices::encrypted_user_key.eq::>(None), @@ -532,12 +540,37 @@ mod tests { assert_eq!(device.trusted_private_key(), None); } + #[test] + fn a_rotation_leaves_the_device_key_pair_in_place() { + // What `invalidate_wrapped_user_keys` does: the wrapped user key and the public key go, + // the private key stays, because the device key that wraps it is untouched by a rotation. + let mut device = trusted_device(); + device.encrypted_user_key = None; + device.encrypted_public_key = None; + + assert!(!device.is_trusted(), "nothing may unlock until the client re-wraps"); + assert!(device.holds_private_key(), "but the device can still be handed a new user key"); + assert!(device.holds_any_key()); + } + + #[test] + fn a_device_that_never_had_a_trust_holds_nothing() { + let device = Device::new(String::from("device").into(), String::from("user").into(), String::new(), 9); + assert!(!device.holds_private_key()); + assert!(!device.holds_any_key()); + + let mut device = trusted_device(); + device.encrypted_private_key = Some(String::new()); + assert!(!device.holds_private_key(), "an empty key is as good as a missing one"); + } + #[test] fn untrusting_clears_every_key() { let mut device = trusted_device(); device.untrust(); assert!(!device.is_trusted()); + assert!(!device.holds_any_key()); assert_eq!(device.encrypted_user_key, None); assert_eq!(device.encrypted_public_key, None); assert_eq!(device.encrypted_private_key, None); diff --git a/src/mail.rs b/src/mail.rs index fe5f217a..ac4336e4 100644 --- a/src/mail.rs +++ b/src/mail.rs @@ -543,7 +543,9 @@ pub async fn send_device_approval_requested( let (subject, body_html, body_text) = get_text( "email/device_approval_requested", json!({ - "url": CONFIG.domain(), + // The page that can actually answer these, which is in the admin panel rather than in + // the web vault: the one upstream uses is not part of any open source build. + "url": format!("{}/admin/device-approvals", CONFIG.domain()), "img_src": CONFIG._smtp_img_src(), "org_name": org_name, "user_email": user_email, diff --git a/src/static/templates/email/device_approval_requested.hbs b/src/static/templates/email/device_approval_requested.hbs index e436c01c..7c786775 100644 --- a/src/static/templates/email/device_approval_requested.hbs +++ b/src/static/templates/email/device_approval_requested.hbs @@ -2,5 +2,5 @@ Device Approval Requested {{user_name}} ({{user_email}}) is asking to have a new device approved in your {{org_name}} organization. Until an administrator approves it, they cannot get into their vault on that device. -Review the request in the organization administration of {{{url}}}. +Review the request at {{{url}}}. {{> email/email_footer_text }} diff --git a/src/static/templates/email/device_approval_requested.html.hbs b/src/static/templates/email/device_approval_requested.html.hbs index ee66c98d..580b89cb 100644 --- a/src/static/templates/email/device_approval_requested.html.hbs +++ b/src/static/templates/email/device_approval_requested.html.hbs @@ -9,7 +9,7 @@ Device Approval Requested - Review the request in the organization administration of {{{url}}}. + Review the request at {{{url}}}. diff --git a/src/static/templates/email/trusted_device_admin_approval.hbs b/src/static/templates/email/trusted_device_admin_approval.hbs index af5fbcf2..27dbd74a 100644 --- a/src/static/templates/email/trusted_device_admin_approval.hbs +++ b/src/static/templates/email/trusted_device_admin_approval.hbs @@ -5,5 +5,5 @@ An administrator of your {{org_name}} organization approved a device for your ac Device: {{device}} IP address: {{ip}} -If this was not you, change your password and contact your administrator. +If this was not you, contact your administrator and remove the device from your account. {{> email/email_footer_text }} diff --git a/src/static/templates/email/trusted_device_admin_approval.html.hbs b/src/static/templates/email/trusted_device_admin_approval.html.hbs index 535cd41a..94a79717 100644 --- a/src/static/templates/email/trusted_device_admin_approval.html.hbs +++ b/src/static/templates/email/trusted_device_admin_approval.html.hbs @@ -15,7 +15,7 @@ Device Approved - If this was not you, change your password and contact your administrator. + If this was not you, contact your administrator and remove the device from your account. diff --git a/src/util.rs b/src/util.rs index 91f075d1..43bce822 100644 --- a/src/util.rs +++ b/src/util.rs @@ -505,6 +505,95 @@ pub fn is_valid_email(email: &str) -> bool { true } +/// The most an `EncString` we are willing to store may weigh. The largest legitimate one is an +/// RSA-4096 envelope with a MAC, which stays an order of magnitude below this. +const MAX_ENC_STRING_LENGTH: usize = 4096; + +/// Whether a value has the shape of a Bitwarden `EncString`: `.|...`. +/// +/// The server cannot tell whether a blob decrypts, but it can refuse everything that is not even +/// of the right form, which keeps unbounded junk out of the columns that hold key material. +/// Mirrors `EncryptedStringAttribute` upstream. +/// https://github.com/bitwarden/server/blob/main/src/Core/Utilities/EncryptedStringAttribute.cs +pub fn is_valid_enc_string(value: &str) -> bool { + if value.is_empty() || value.len() > MAX_ENC_STRING_LENGTH { + return false; + } + + let Some((enc_type, data)) = value.split_once('.') else { + return false; + }; + + // The number of `|` separated parts each type is made of. + let parts = match enc_type { + // An RSA envelope is the ciphertext by itself. + "3" | "4" => 1, + // An AES value carries its IV, an RSA one from type 5 on carries a MAC. + "0" | "5" | "6" => 2, + // And an AES value from type 1 on carries both. + "1" | "2" => 3, + _ => return false, + }; + + let mut seen = 0; + for part in data.split('|') { + seen += 1; + if seen > parts || part.is_empty() || data_encoding::BASE64.decode(part.as_bytes()).is_err() { + return false; + } + } + + seen == parts +} + +#[cfg(test)] +mod enc_string_tests { + use super::is_valid_enc_string; + + #[test] + fn a_well_formed_enc_string_of_every_type_is_accepted() { + for value in [ + "0.aXY=|Y2lwaGVy", + "1.aXY=|Y2lwaGVy|bWFj", + "2.aXY=|Y2lwaGVy|bWFj", + "3.Y2lwaGVy", + "4.Y2lwaGVy", + "5.Y2lwaGVy|bWFj", + "6.Y2lwaGVy|bWFj", + ] { + assert!(is_valid_enc_string(value), "{value}"); + } + } + + #[test] + fn anything_that_is_not_one_is_refused() { + for value in [ + "", + " ", + "not-an-enc-string", + "2", + "2.", + ".aXY=|Y2lwaGVy|bWFj", + "7.Y2lwaGVy", // no such type + "-1.Y2lwaGVy", // and none below zero either + "2.aXY=|Y2lwaGVy", // type 2 without its mac + "2.aXY=|Y2lwaGVy|bWFj|x", // or with one part too many + "4.Y2lwaGVy|bWFj", // type 4 carries no mac + "2.aXY=||bWFj", // an empty part is not base64 + "4.not base64!", + ] { + assert!(!is_valid_enc_string(value), "{value}"); + } + } + + #[test] + fn an_oversized_value_is_refused() { + let payload = "A".repeat(4096); + assert!(is_valid_enc_string(&format!("4.{}", &payload[..4000]))); + assert!(!is_valid_enc_string(&format!("4.{payload}")), "must not grow without bound"); + } +} + // // Deployment environment methods // From 14139cbd7a89f3574359d6b12df72f0fb7fb6775 Mon Sep 17 00:00:00 2001 From: tom27052006 <83423411+tom27052006@users.noreply.github.com> Date: Sat, 1 Aug 2026 23:44:58 +0200 Subject: [PATCH 06/10] Move the device approvals page to the web vault The stand-in page in the admin panel asked an administrator of an organization for their vault master password inside the panel of the server operator, which are two different roles, and it carried its own partial login: no Argon2, no two-step login, and no fingerprint of the asking device to compare against. The page belongs where that already exists. The web vault has the whole login stack and, in its AGPL part, the same unwrap the approval needs, since account recovery does it too. Upstream keeps only the page itself in the licensed part; the navigation entry and the string for it are already in every build. So the endpoints stay and the page goes, to be added to the web vault build instead. The notification mail points at the route it lives under there. A member who loses every trusted device is not stranded meanwhile: account recovery gets them back in under the same conditions, at the price of a new master password. --- .env.template | 3 + src/api/admin.rs | 13 - src/api/core/accounts.rs | 4 +- src/api/web.rs | 3 - src/config.rs | 1 - src/mail.rs | 7 +- src/static/scripts/admin_device_approvals.js | 342 ------------------ src/static/templates/admin/base.hbs | 3 - .../templates/admin/device_approvals.hbs | 68 ---- 9 files changed, 10 insertions(+), 434 deletions(-) delete mode 100644 src/static/scripts/admin_device_approvals.js delete mode 100644 src/static/templates/admin/device_approvals.hbs diff --git a/.env.template b/.env.template index dbae390a..30ec49f0 100644 --- a/.env.template +++ b/.env.template @@ -559,6 +559,9 @@ ## To turn this off again, clear this setting but leave `SSO_ENABLED` on: users without a master ## password keep receiving their keys while they still have a trusted device, so their client can ## walk them through setting one. Turning off `SSO_ENABLED` instead leaves them no way to log in. +## Answering a device approval needs the "Device approvals" page of the organization settings, +## which a stock web vault does not build. Without it, a member who lost every trusted device is +## recovered through account recovery instead, which works but hands them a new master password. # SSO_TRUSTED_DEVICE_ENCRYPTION=false ######################## diff --git a/src/api/admin.rs b/src/api/admin.rs index 2cd38792..7037bfb1 100644 --- a/src/api/admin.rs +++ b/src/api/admin.rs @@ -67,7 +67,6 @@ pub fn routes() -> Vec { users_overview, organizations_overview, delete_organization, - device_approvals, diagnostics, get_diagnostics_config, resend_user_invite, @@ -616,18 +615,6 @@ async fn delete_organization(org_id: OrganizationId, _token: AdminToken, conn: D org.delete(&conn).await } -/// Stand-in for the "Device approvals" page of the admin console, which lives in the part of -/// bitwarden/clients that is not AGPL licensed and is therefore in no web vault build. -/// -/// This page only serves the markup. Everything else happens in the browser against the regular -/// API, because answering a request needs the master password of an administrator of the -/// organization: the server keeps its private key encrypted with a key it does not have. -#[get("/device-approvals")] -fn device_approvals(_token: AdminToken) -> ApiResult> { - let text = AdminTemplateData::new("admin/device_approvals", json!({})).render()?; - Ok(Html(text)) -} - #[derive(Deserialize)] struct GitRelease { tag_name: String, diff --git a/src/api/core/accounts.rs b/src/api/core/accounts.rs index 22282d28..2852a3bd 100644 --- a/src/api/core/accounts.rs +++ b/src/api/core/accounts.rs @@ -2019,7 +2019,9 @@ async fn notify_device_approval_requested(user: &User, org_id: &OrganizationId, continue; }; - if let Err(e) = mail::send_device_approval_requested(&admin.email, &org.name, &user.email, &user.name).await { + if let Err(e) = + mail::send_device_approval_requested(&admin.email, org_id, &org.name, &user.email, &user.name).await + { error!("Error sending device approval request email: {e:#?}"); } } diff --git a/src/api/web.rs b/src/api/web.rs index 7bca3583..5bd4c85d 100644 --- a/src/api/web.rs +++ b/src/api/web.rs @@ -260,9 +260,6 @@ pub fn static_files(filename: &str) -> Result<(ContentType, &'static [u8]), Erro "admin_organizations.js" => { Ok((ContentType::JavaScript, include_bytes!("../static/scripts/admin_organizations.js"))) } - "admin_device_approvals.js" => { - Ok((ContentType::JavaScript, include_bytes!("../static/scripts/admin_device_approvals.js"))) - } "admin_diagnostics.js" => { Ok((ContentType::JavaScript, include_bytes!("../static/scripts/admin_diagnostics.js"))) } diff --git a/src/config.rs b/src/config.rs index f581697f..61b8c697 100644 --- a/src/config.rs +++ b/src/config.rs @@ -1769,7 +1769,6 @@ where reg!("admin/settings"); reg!("admin/users"); reg!("admin/organizations"); - reg!("admin/device_approvals"); reg!("admin/diagnostics"); reg!("404"); diff --git a/src/mail.rs b/src/mail.rs index ac4336e4..d3de1ef1 100644 --- a/src/mail.rs +++ b/src/mail.rs @@ -536,6 +536,7 @@ pub async fn send_new_device_logged_in(address: &str, ip: &str, dt: &NaiveDateTi /// of their own left to ask. pub async fn send_device_approval_requested( address: &str, + org_id: &OrganizationId, org_name: &str, user_email: &str, user_name: &str, @@ -543,9 +544,9 @@ pub async fn send_device_approval_requested( let (subject, body_html, body_text) = get_text( "email/device_approval_requested", json!({ - // The page that can actually answer these, which is in the admin panel rather than in - // the web vault: the one upstream uses is not part of any open source build. - "url": format!("{}/admin/device-approvals", CONFIG.domain()), + // Straight to the page that answers these, the same route the upstream admin console + // uses for them. + "url": format!("{}/#/organizations/{}/settings/device-approvals", CONFIG.domain(), org_id), "img_src": CONFIG._smtp_img_src(), "org_name": org_name, "user_email": user_email, diff --git a/src/static/scripts/admin_device_approvals.js b/src/static/scripts/admin_device_approvals.js deleted file mode 100644 index 4b386323..00000000 --- a/src/static/scripts/admin_device_approvals.js +++ /dev/null @@ -1,342 +0,0 @@ -"use strict"; -/* eslint-env es2017, browser */ -/* global BASE_URL:readable */ - -// Answering a device approval means handing a member their own user key, encrypted for the key -// pair of the device that is asking. The server cannot do that: it holds the organization's -// private key only encrypted with the organization key, and that one exists solely as RSA -// envelopes addressed to each administrator. So the whole chain runs here, in the browser, and -// the master password never leaves this page. -// -// master password -> master key PBKDF2-SHA256(password, email, iterations) -// -> own user key AES from profile.key -// -> own private key AES from profile.privateKey -// -> organization key RSA from profile.organizations[].key -// -> org private key AES from reset-password-details.encryptedPrivateKey -// -> member's user key RSA from reset-password-details.resetPasswordKey -// -> encryptedUserKey RSA for the public key out of the request - -const DEVICE_IDENTIFIER_KEY = "vw_admin_device_approvals_device_id"; - -let session = null; // { token, profile, privateKey } -let requests = []; - -function element(id) { - return document.getElementById(id); -} - -function setStatus(message, kind) { - const box = element("approval-status"); - box.textContent = message; - box.className = message ? `alert alert-${kind || "info"}` : "d-none"; -} - -function fromBase64(value) { - return Uint8Array.from(atob(value), c => c.charCodeAt(0)); -} - -function toBase64(bytes) { - return btoa(String.fromCharCode(...new Uint8Array(bytes))); -} - -function concat(a, b) { - const out = new Uint8Array(a.length + b.length); - out.set(a, 0); - out.set(b, a.length); - return out; -} - -// --------------------------------------------------------------------------- crypto - -async function pbkdf2(password, salt, iterations) { - const key = await crypto.subtle.importKey("raw", password, "PBKDF2", false, ["deriveBits"]); - const bits = await crypto.subtle.deriveBits( - { name: "PBKDF2", salt: salt, iterations: iterations, hash: "SHA-256" }, key, 256); - return new Uint8Array(bits); -} - -// HKDF-Expand only, with the master key used directly as the pseudorandom key. WebCrypto's HKDF -// always runs the extract step first, which would give a different result, so this is by hand. -async function hkdfExpand(prk, info) { - const key = await crypto.subtle.importKey("raw", prk, { name: "HMAC", hash: "SHA-256" }, false, ["sign"]); - const input = concat(new TextEncoder().encode(info), new Uint8Array([1])); - return new Uint8Array(await crypto.subtle.sign("HMAC", key, input)); -} - -async function stretch(masterKey) { - return [await hkdfExpand(masterKey, "enc"), await hkdfExpand(masterKey, "mac")]; -} - -// A user key or organization key is 64 bytes: the AES half followed by the HMAC half. -async function splitKey(key) { - if (key.length === 32) { - return stretch(key); - } - if (key.length === 64) { - return [key.slice(0, 32), key.slice(32)]; - } - throw new Error(`Unexpected key length ${key.length}`); -} - -// EncString type 2: 2.iv|ciphertext|mac -async function decryptSymmetric(encString, encKey, macKey) { - const [kind, rest] = [encString.slice(0, encString.indexOf(".")), encString.slice(encString.indexOf(".") + 1)]; - if (kind !== "2") { - throw new Error(`Expected a symmetrically encrypted value, got type ${kind}`); - } - - const [iv, ciphertext, mac] = rest.split("|").map(fromBase64); - - const macCryptoKey = await crypto.subtle.importKey("raw", macKey, { name: "HMAC", hash: "SHA-256" }, false, ["verify"]); - if (!await crypto.subtle.verify("HMAC", macCryptoKey, mac, concat(iv, ciphertext))) { - throw new Error("The stored value does not match its signature. Wrong master password?"); - } - - const aesKey = await crypto.subtle.importKey("raw", encKey, { name: "AES-CBC" }, false, ["decrypt"]); - return new Uint8Array(await crypto.subtle.decrypt({ name: "AES-CBC", iv: iv }, aesKey, ciphertext)); -} - -// EncString type 4 or 6: RSA-OAEP with SHA-1. Type 6 carries an extra signature we do not need. -async function decryptAsymmetric(encString, privateKey) { - const kind = encString.slice(0, encString.indexOf(".")); - if (kind !== "4" && kind !== "6") { - throw new Error(`Expected an RSA encrypted value, got type ${kind}`); - } - - const data = fromBase64(encString.slice(encString.indexOf(".") + 1).split("|")[0]); - return new Uint8Array(await crypto.subtle.decrypt({ name: "RSA-OAEP" }, privateKey, data)); -} - -async function encryptAsymmetric(plain, publicKeyB64) { - const publicKey = await crypto.subtle.importKey( - "spki", fromBase64(publicKeyB64), { name: "RSA-OAEP", hash: "SHA-1" }, false, ["encrypt"]); - const encrypted = await crypto.subtle.encrypt({ name: "RSA-OAEP" }, publicKey, plain); - return "4." + toBase64(encrypted); -} - -async function importPrivateKey(pkcs8) { - return crypto.subtle.importKey("pkcs8", pkcs8, { name: "RSA-OAEP", hash: "SHA-1" }, false, ["decrypt"]); -} - -// --------------------------------------------------------------------------- api - -async function api(method, path, body, options) { - const settings = options || {}; - const headers = {}; - if (session && !settings.anonymous) { - headers["Authorization"] = `Bearer ${session.token}`; - } - - let payload = null; - if (body !== undefined && body !== null) { - if (settings.form) { - headers["Content-Type"] = "application/x-www-form-urlencoded"; - payload = new URLSearchParams(body).toString(); - } else { - headers["Content-Type"] = "application/json"; - payload = JSON.stringify(body); - } - } - - const response = await fetch(BASE_URL + path, { method: method, headers: headers, body: payload }); - const text = await response.text(); - let parsed = null; - try { - parsed = text ? JSON.parse(text) : null; - } catch (e) { - parsed = { message: text.slice(0, 200) }; - } - - if (!response.ok) { - throw new Error((parsed && (parsed.message || parsed.ErrorModel?.Message)) || `HTTP ${response.status}`); - } - return parsed; -} - -function deviceIdentifier() { - let identifier = localStorage.getItem(DEVICE_IDENTIFIER_KEY); - if (!identifier) { - identifier = crypto.randomUUID(); - localStorage.setItem(DEVICE_IDENTIFIER_KEY, identifier); - } - return identifier; -} - -// --------------------------------------------------------------------------- flow - -async function signIn(email, password) { - const prelogin = await api("POST", "/identity/accounts/prelogin", { email: email }, { anonymous: true }); - if (prelogin.kdf !== 0) { - throw new Error("This account uses Argon2, which this page does not implement. Use APPROVE_DEVICE from the command line."); - } - - const encoder = new TextEncoder(); - const masterKey = await pbkdf2(encoder.encode(password), encoder.encode(email.trim().toLowerCase()), prelogin.kdfIterations); - const passwordHash = toBase64(await pbkdf2(masterKey, encoder.encode(password), 1)); - - const token = await api("POST", "/identity/connect/token", { - grant_type: "password", - client_id: "web", - username: email, - password: passwordHash, - scope: "api offline_access", - deviceIdentifier: deviceIdentifier(), - deviceName: "Vaultwarden admin", - deviceType: 9, - }, { anonymous: true, form: true }); - - if (token.TwoFactorProviders || token.TwoFactorProviders2) { - throw new Error("Two-step login is active for this account, which this page does not implement."); - } - - session = { token: token.access_token }; - - const sync = await api("GET", "/api/sync?excludeDomains=true"); - const profile = sync.profile; - const userKey = await decryptSymmetric(profile.key, ...await stretch(masterKey)); - const privateKey = await importPrivateKey(await decryptSymmetric(profile.privateKey, ...await splitKey(userKey))); - - session = { token: token.access_token, profile: profile, privateKey: privateKey }; -} - -async function loadRequests() { - requests = []; - for (const org of session.profile.organizations) { - let pending; - try { - pending = await api("GET", `/api/organizations/${org.id}/auth-requests`); - } catch (e) { - continue; // not an administrator of this one - } - for (const request of pending.data) { - request.organization = org; - requests.push(request); - } - } -} - -async function memberUserKey(request) { - const org = request.organization; - const orgKey = await decryptAsymmetric(org.key, session.privateKey); - - const details = await api( - "GET", `/api/organizations/${org.id}/users/${request.organizationUserId}/reset-password-details`); - if (!details.resetPasswordKey) { - throw new Error("This member is not enrolled in account recovery, so nobody can hand out their key."); - } - - const orgPrivateKey = await importPrivateKey(await decryptSymmetric(details.encryptedPrivateKey, ...await splitKey(orgKey))); - return decryptAsymmetric(details.resetPasswordKey, orgPrivateKey); -} - -async function answer(request, approved) { - const path = `/api/organizations/${request.organization.id}/auth-requests/${request.id}`; - - if (!approved) { - await api("POST", path, { requestApproved: false }); - setStatus(`Denied the request from ${request.email}.`, "secondary"); - return; - } - - const encryptedUserKey = await encryptAsymmetric(await memberUserKey(request), request.publicKey); - await api("POST", path, { requestApproved: true, encryptedUserKey: encryptedUserKey }); - setStatus(`Approved. ${request.email} can open their vault on that device now.`, "success"); -} - -// --------------------------------------------------------------------------- rendering - -function renderRequests() { - const tbody = element("approval-rows"); - tbody.innerHTML = ""; - - element("approval-empty").classList.toggle("d-none", requests.length > 0); - element("approval-table").classList.toggle("d-none", requests.length === 0); - - requests.forEach((request, index) => { - const row = document.createElement("tr"); - - const cell = (text) => { - const td = document.createElement("td"); - td.textContent = text; - return td; - }; - - row.appendChild(cell(request.email)); - row.appendChild(cell(request.organization.name)); - row.appendChild(cell(request.requestDeviceType)); - row.appendChild(cell(request.requestIpAddress)); - row.appendChild(cell(new Date(request.creationDate).toLocaleString())); - - const actions = document.createElement("td"); - for (const [label, style, approved] of [["Approve", "btn-primary", true], ["Deny", "btn-outline-secondary", false]]) { - const button = document.createElement("button"); - button.type = "button"; - button.className = `btn btn-sm ${style} me-1`; - button.textContent = label; - button.addEventListener("click", () => void handleAnswer(index, approved, button)); - actions.appendChild(button); - } - row.appendChild(actions); - - tbody.appendChild(row); - }); -} - -function busy(on) { - document.querySelectorAll("#approval-rows button, #approval-reload").forEach(b => { b.disabled = on; }); -} - -async function handleAnswer(index, approved, button) { - busy(true); - button.textContent = approved ? "Approving..." : "Denying..."; - try { - await answer(requests[index], approved); - await refresh(); - } catch (e) { - setStatus(e.message, "danger"); - } finally { - busy(false); - } -} - -async function refresh() { - await loadRequests(); - renderRequests(); -} - -// --------------------------------------------------------------------------- wiring - -document.addEventListener("DOMContentLoaded", () => { - element("approval-signin").addEventListener("submit", async (event) => { - event.preventDefault(); - const button = element("approval-signin-button"); - button.disabled = true; - setStatus("Signing in and unlocking the keys...", "info"); - - try { - await signIn(element("approval-email").value.trim(), element("approval-password").value); - element("approval-password").value = ""; - element("approval-signin").classList.add("d-none"); - element("approval-list").classList.remove("d-none"); - element("approval-signed-in-as").textContent = session.profile.email; - await refresh(); - setStatus("", null); - } catch (e) { - session = null; - setStatus(e.message, "danger"); - } finally { - button.disabled = false; - } - }); - - element("approval-reload").addEventListener("click", async () => { - busy(true); - try { - await refresh(); - } catch (e) { - setStatus(e.message, "danger"); - } finally { - busy(false); - } - }); -}); diff --git a/src/static/templates/admin/base.hbs b/src/static/templates/admin/base.hbs index 26923f45..e1dcacb5 100644 --- a/src/static/templates/admin/base.hbs +++ b/src/static/templates/admin/base.hbs @@ -45,9 +45,6 @@
  • Organizations
    • -
  • - Device approvals -
  • Diagnostics
    • diff --git a/src/static/templates/admin/device_approvals.hbs b/src/static/templates/admin/device_approvals.hbs deleted file mode 100644 index 354e2d55..00000000 --- a/src/static/templates/admin/device_approvals.hbs +++ /dev/null @@ -1,68 +0,0 @@ -
    -
    -
    Device approvals
    - -

    - A member who unlocks with a trusted device and has no other device of their own left to - ask can ask an administrator of their organization instead. Answering hands them their - own user key, encrypted for the device that is asking. It only works for members who - enrolled into account recovery. -

    - -
    - -
    -
    -
    - This asks for a vault account, not for the admin token. - The server cannot answer these requests on its own: it holds the organization's - private key only encrypted with a key that never leaves its members. So sign in - below as an administrator of the organization and the whole chain is - unwrapped here in your browser. Your master password is not sent anywhere; only - the same login hash a regular sign-in would send leaves this page. -
    -
    -
    - - -
    -
    - - -
    -
    - -
    -
    - -
    -

    - Signed in as -

    - -

    No requests are waiting for an answer.

    - -
    - - - - - - - - - - - - -
    MemberOrganizationDeviceIP addressAsked atActions
    -
    - -
    - -
    -
    -
    -
    - - From 4e562f240c7b8072ca7e3cfc486fb1920ca10d8d Mon Sep 17 00:00:00 2001 From: tom27052006 <83423411+tom27052006@users.noreply.github.com> Date: Tue, 4 Aug 2026 20:51:42 +0200 Subject: [PATCH 07/10] Leave the admin approvals out of what a device is waiting on A request addressed to an administrator is answered through the organization and stays open for a week, so counting it as the pending request of its device hid the short lived one the user was actually being shown and made approving from another device fail for as long as it was open. It is now excluded, along with requests past their window, as upstream does. --- src/api/core/accounts.rs | 3 ++- src/db/models/auth_request.rs | 11 +++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/api/core/accounts.rs b/src/api/core/accounts.rs index 2852a3bd..d1cdf384 100644 --- a/src/api/core/accounts.rs +++ b/src/api/core/accounts.rs @@ -2177,7 +2177,8 @@ async fn get_auth_requests_pending(headers: Headers, conn: DbConn) -> JsonResult Ok(Json(json!({ "data": auth_requests .iter() - .filter(|request| request.approved.is_none()) + // The same set a device answers for itself, see `find_by_user_and_requested_device`. + .filter(|request| request.approved.is_none() && !request.is_admin_approval() && !request.is_expired()) .map(|request| { let response_date_utc = request.response_date.map(|response_date| format_date(&response_date)); diff --git a/src/db/models/auth_request.rs b/src/db/models/auth_request.rs index 0472f62f..bda821fe 100644 --- a/src/db/models/auth_request.rs +++ b/src/db/models/auth_request.rs @@ -220,16 +220,27 @@ impl AuthRequest { .await } + /// The request a device is currently waiting on, if it is still open and still within its + /// window. + /// + /// Only the types a device answers for itself. A request addressed to an administrator is + /// answered through the organization and stays open for a week, so counting it here would let + /// it shadow the short lived request the user is actually being shown. + /// https://github.com/bitwarden/server/blob/main/src/Infrastructure.EntityFramework/Auth/Repositories/Queries/DeviceWithPendingAuthByUserIdQuery.cs pub async fn find_by_user_and_requested_device( user_uuid: &UserId, device_uuid: &DeviceId, conn: &DbConn, ) -> Option { + let oldest = Utc::now().naive_utc() - Self::user_request_expiration(); + conn.run(move |conn| { auth_requests::table .filter(auth_requests::user_uuid.eq(user_uuid)) .filter(auth_requests::request_device_identifier.eq(device_uuid)) + .filter(auth_requests::atype.ne(AuthRequestType::AdminApproval as i32)) .filter(auth_requests::approved.is_null()) + .filter(auth_requests::creation_date.gt(oldest)) .order_by(auth_requests::creation_date.desc()) .first::(conn) .ok() From 8098211cecc826fbb0f556f56088976999c5cec0 Mon Sep 17 00:00:00 2001 From: tom27052006 <83423411+tom27052006@users.noreply.github.com> Date: Sat, 29 Aug 2026 17:29:45 +0200 Subject: [PATCH 08/10] Fix trusted device review findings --- src/api/core/accounts.rs | 336 ++++++++++++++++++++++++++++------ src/api/core/organizations.rs | 36 ++-- src/api/identity.rs | 294 ++++++++++++++++++++++++----- src/api/notifications.rs | 9 +- src/api/push.rs | 16 +- src/auth.rs | 35 ++++ src/db/models/auth_request.rs | 41 ++++- src/db/models/device.rs | 116 ++++++++---- src/db/models/organization.rs | 42 +++++ src/util.rs | 123 ++++++++++--- 10 files changed, 857 insertions(+), 191 deletions(-) diff --git a/src/api/core/accounts.rs b/src/api/core/accounts.rs index d1cdf384..fb3abf08 100644 --- a/src/api/core/accounts.rs +++ b/src/api/core/accounts.rs @@ -1,4 +1,4 @@ -use std::collections::{HashMap, HashSet}; +use std::collections::HashSet; use chrono::Utc; use rocket::{ @@ -22,8 +22,8 @@ use crate::{ models::{ AuthRequest, AuthRequestId, AuthRequestType, Cipher, CipherId, Device, DeviceId, DeviceType, DeviceWithAuthRequest, EmergencyAccess, EmergencyAccessId, EventType, Folder, FolderId, Invitation, - Membership, MembershipId, MembershipStatus, MembershipType, OrgPolicy, OrgPolicyType, Organization, - OrganizationId, Send, SendId, User, UserId, UserKdfType, + Membership, MembershipId, MembershipStatus, OrgPolicy, OrgPolicyType, Organization, OrganizationId, Send, + SendId, User, UserId, UserKdfType, }, }, mail, @@ -816,6 +816,20 @@ struct RotateAccountUnlockData { emergency_access_unlock_data: Vec, master_password_unlock_data: MasterPasswordUnlockData, organization_account_recovery_unlock_data: Vec, + /// The user key, re-wrapped for every device that unlocks the vault without a master password. + /// + /// Absent rather than empty tells the two generations of clients apart: one that sends this + /// rotates the trust of its devices right here, an older one does it afterwards through + /// `POST /devices/update-trust` and leaves this out entirely. See `post_rotatekey`. + device_key_unlock_data: Option>, +} + +#[derive(Deserialize)] +#[serde(rename_all = "camelCase")] +struct UpdateDeviceKeysData { + device_id: DeviceId, + encrypted_user_key: String, + encrypted_public_key: String, } #[derive(Deserialize)] @@ -845,6 +859,55 @@ struct RotateAccountData { sends: Vec, } +/// Works out what a key rotation has to write to the user's devices. +/// +/// Returns the devices that keep their trust, each with the user key freshly wrapped for it. +/// Whatever the user owns beyond that list ends up untrusted, so the caller can hand the result to +/// `Device::replace_trust` and be done in one transaction. +/// +/// Mirrors `DeviceRotationValidator` upstream, which refuses a rotation that would quietly drop the +/// trust of a device the user still relies on. Untrusting is the client's own separate step, and +/// the current ones take it before they get here. +/// https://github.com/bitwarden/server/blob/main/src/Api/KeyManagement/Validators/DeviceRotationValidator.cs +fn validate_device_keydata( + updates: &[UpdateDeviceKeysData], + existing_devices: &[Device], +) -> ApiResult> { + let mut listed: HashSet<&DeviceId> = HashSet::with_capacity(updates.len()); + let mut rotated = Vec::with_capacity(updates.len()); + + for update in updates { + if !listed.insert(&update.device_id) { + err!("A device was listed more than once in the rotation") + } + + let Some(device) = existing_devices.iter().find(|device| device.uuid == update.device_id) else { + err!(format!("Device {} does not belong to this user", update.device_id)) + }; + + validate_enc_strings(&[ + ("encryptedUserKey", &update.encrypted_user_key), + ("encryptedPublicKey", &update.encrypted_public_key), + ])?; + + // Without its own key pair a device has nothing these two keys could belong to, so it + // cannot be put back into a trust and is left to be untrusted instead. + if device.holds_private_key() { + rotated.push(( + update.device_id.clone(), + update.encrypted_user_key.clone(), + update.encrypted_public_key.clone(), + )); + } + } + + if existing_devices.iter().any(|device| device.is_trusted() && !listed.contains(&device.uuid)) { + err!("All existing trusted devices must be included in the rotation") + } + + Ok(rotated) +} + fn validate_keydata( data: &KeyData, existing_ciphers: &[Cipher], @@ -949,6 +1012,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_devices = Device::find_by_user(user_id, &conn).await; validate_keydata( &data, @@ -960,6 +1024,11 @@ async fn post_rotatekey(data: Json, headers: Headers, conn: DbConn, nt: &headers.user, )?; + let rotated_devices = match data.account_unlock_data.device_key_unlock_data.as_deref() { + Some(updates) => Some(validate_device_keydata(updates, &existing_devices)?), + None => None, + }; + // Update folder data for folder_data in data.account_data.folders { // Skip `null` folder id entries. @@ -1023,12 +1092,19 @@ async fn post_rotatekey(data: Json, headers: Headers, conn: DbConn, nt: } // Every device holds the previous user key wrapped for itself, which unlocks nothing anymore. - // Drop those copies before the new key is written, never after: the other order leaves a window - // in which a device still counts as trusted and hands its owner a key that no longer opens the - // vault. This way a failure here means the rotation simply did not happen. - // The clients re-wrap the new user key for every device right after this via - // `POST /devices/update-trust`; whatever they leave out stays untrusted. - Device::invalidate_wrapped_user_keys(&headers.user.uuid, &conn).await?; + // Settle that here rather than after the account itself: by this point the ciphers have already + // been rewritten under the new user key, so a device that holds the new one is the half that + // still works if what follows fails. The other order would leave a device counting itself + // trusted while handing its owner the key it just stopped needing. + match rotated_devices { + // The current clients send the re-wrapped user key for every trusted device along with the + // rotation, so their trust survives it. Anything they left out is untrusted here. + Some(rotated) => Device::replace_trust(&headers.user.uuid, rotated, &conn).await?, + // A client old enough to leave the field out does this afterwards through + // `POST /devices/update-trust`. Until it does, no device counts as trusted, so the worst it + // costs its owner is another login rather than an unlock that fails. + None => Device::invalidate_wrapped_user_keys(&headers.user.uuid, &conn).await?, + } // Update user data let mut user = headers.user; @@ -1707,6 +1783,10 @@ struct UpdateDevicesTrustData { /// /// Every trusted device that is not listed loses its trust: its stored copy of the user key is the /// old one and would no longer unlock anything. +/// +/// The current clients do this as part of the rotation itself and never come here; this is the +/// route the older ones take, and the only one that can rotate the trust of a single device without +/// rotating the account. See `post_rotatekey`. #[post("/devices/update-trust", data = "")] async fn post_devices_update_trust(data: Json, headers: Headers, conn: DbConn) -> EmptyResult { let data = data.into_inner(); @@ -1718,55 +1798,48 @@ async fn post_devices_update_trust(data: Json, headers: ("encryptedPublicKey", &data.current_device.encrypted_public_key), ])?; - let mut updates: HashMap = HashMap::new(); - for other in data.other_devices { - if other.device_id == headers.device.uuid { - err!("The current device cannot also be part of the optional rotation") - } - validate_enc_strings(&[ - ("encryptedUserKey", &other.keys.encrypted_user_key), - ("encryptedPublicKey", &other.keys.encrypted_public_key), - ])?; - if updates.insert(other.device_id, other.keys).is_some() { - err!("A device was listed more than once in the rotation") - } - } - let devices = Device::find_by_user(&headers.user.uuid, &conn).await; if !devices.iter().any(|device| device.uuid == headers.device.uuid) { err!("No device found") } - // Validate everything before writing anything: a rotation that stops halfway would leave the - // devices wrapping a mix of the old and the new user key. - if let Some(unknown) = updates.keys().find(|device_id| !devices.iter().any(|device| device.uuid == **device_id)) { - err!(format!("Device {unknown} does not belong to this user")) - } + // The current device is written whatever it holds now, as upstream does: it is the one the + // caller is speaking from and just proved it can unlock. + let mut updates = vec![( + headers.device.uuid.clone(), + data.current_device.encrypted_user_key, + data.current_device.encrypted_public_key, + )]; + let mut listed: HashSet = HashSet::from([headers.device.uuid.clone()]); - for mut device in devices { - if device.uuid == headers.device.uuid { - device.encrypted_user_key = Some(data.current_device.encrypted_user_key.clone()); - device.encrypted_public_key = Some(data.current_device.encrypted_public_key.clone()); - } else if let Some(keys) = updates.remove(&device.uuid) { - // A rotation clears the wrapped user key of every device, so the listed ones are not - // trusted at this point; their key pair is what they are restored from. Without it - // there is nothing the two keys could belong to. - if !device.holds_private_key() { - continue; + // Validate everything before writing anything, so one bad entry cannot leave the devices + // wrapping a mix of the old and the new user key. + for other in data.other_devices { + if !listed.insert(other.device_id.clone()) { + if other.device_id == headers.device.uuid { + err!("The current device cannot also be part of the optional rotation") } - device.encrypted_user_key = Some(keys.encrypted_user_key); - device.encrypted_public_key = Some(keys.encrypted_public_key); - } else if device.holds_any_key() { - // Not listed, so whatever it still holds wraps the previous user key. - device.untrust(); - } else { - continue; + err!("A device was listed more than once in the rotation") } - device.save(true, &conn).await?; + let Some(device) = devices.iter().find(|device| device.uuid == other.device_id) else { + err!(format!("Device {} does not belong to this user", other.device_id)) + }; + + validate_enc_strings(&[ + ("encryptedUserKey", &other.keys.encrypted_user_key), + ("encryptedPublicKey", &other.keys.encrypted_public_key), + ])?; + + // A rotation clears the wrapped user key of every device, so the listed ones are not + // trusted at this point; their key pair is what they are restored from. Without it there is + // nothing the two keys could belong to, so the device is left to be untrusted instead. + if device.holds_private_key() { + updates.push((other.device_id, other.keys.encrypted_user_key, other.keys.encrypted_public_key)); + } } - Ok(()) + Device::replace_trust(&headers.user.uuid, updates, &conn).await } #[derive(Debug, Deserialize)] @@ -1779,22 +1852,16 @@ struct UntrustDevicesData { async fn post_devices_untrust(data: Json, headers: Headers, conn: DbConn) -> EmptyResult { let data = data.into_inner(); - let mut devices = Device::find_by_user(&headers.user.uuid, &conn).await; + let owned: HashSet = + Device::find_by_user(&headers.user.uuid, &conn).await.into_iter().map(|device| device.uuid).collect(); // Check that the user owns all of them first, so a single foreign id does not leave the request // half applied. - if let Some(unknown) = - data.devices.iter().find(|device_id| !devices.iter().any(|device| &device.uuid == *device_id)) - { + if let Some(unknown) = data.devices.iter().find(|device_id| !owned.contains(*device_id)) { err!(format!("Device {unknown} does not belong to this user")) } - for device in devices.iter_mut().filter(|device| data.devices.contains(&device.uuid)) { - device.untrust(); - device.save(true, &conn).await?; - } - - Ok(()) + Device::untrust_many(&headers.user.uuid, data.devices, &conn).await } /// Reported by a client that still holds a device key but did not get any keys back from us. @@ -1834,14 +1901,46 @@ struct AuthRequestRequest { atype: i32, } +/// Upstream puts `[StringLength(25)]` on the access code, so no client sends more than that. +/// https://github.com/bitwarden/server/blob/main/src/Core/Auth/Models/Api/Request/AuthRequest/AuthRequestCreateRequestModel.cs +const MAX_ACCESS_CODE_LENGTH: usize = 25; + +/// A base64 SPKI RSA-4096 public key is under a kilobyte; this leaves room for whatever comes next. +const MAX_REQUEST_PUBLIC_KEY_LENGTH: usize = 4096; + +impl AuthRequestRequest { + /// Both of these end up stored, and the admin approval route stores a copy per organization the + /// user belongs to, so neither may be unbounded. The public key is handed to the answering + /// client as base64 to wrap a key against; one that is not base64 at all would break the page + /// listing the requests rather than just this one. + fn validate(&self) -> EmptyResult { + if self.access_code.is_empty() || self.access_code.len() > MAX_ACCESS_CODE_LENGTH { + err!("Invalid access code") + } + + if self.public_key.is_empty() + || self.public_key.len() > MAX_REQUEST_PUBLIC_KEY_LENGTH + || data_encoding::BASE64.decode(self.public_key.as_bytes()).is_err() + { + err!("Invalid public key") + } + + Ok(()) + } +} + fn auth_request_json(auth_request: &AuthRequest) -> Value { json!({ "id": auth_request.uuid, "publicKey": auth_request.public_key, "type": auth_request.atype, "requestDeviceType": DeviceType::from_i32(auth_request.device_type).to_string(), + // The clients read the raw enum value as well, to pick an icon for the asking device. + "requestDeviceTypeValue": auth_request.device_type, "requestDeviceIdentifier": auth_request.request_device_identifier, "requestIpAddress": auth_request.request_ip, + // Not recorded here, but the clients read it, so it is answered rather than missing. + "requestCountryName": null, "key": auth_request.enc_key, "masterPasswordHash": auth_request.master_password_hash, "creationDate": format_date(&auth_request.creation_date), @@ -1867,6 +1966,8 @@ async fn post_auth_request( err!("You must be authenticated to create a request of that type") } + data.validate()?; + let Some(user) = User::find_by_mail(&data.email, &conn).await else { err!("AuthRequest doesn't exist", "User not found") }; @@ -1928,6 +2029,8 @@ async fn post_admin_auth_request(data: Json, headers: Header err!("AuthRequest doesn't exist", "Device verification failed") } + data.validate()?; + // Only an organization the user really belongs to can answer for them. A pending invitation is // not a membership yet, and a revoked one is not one anymore; sending either of them the email // address, the address and the device of the asker is more than they are owed. @@ -2009,10 +2112,12 @@ async fn notify_device_approval_requested(user: &User, org_id: &OrganizationId, return; }; + // The same set that may answer the request, see `ManageResetPasswordHeaders`. Mailing anyone + // else would tell them who is asking for something they cannot do anything about. let approvers = Membership::find_confirmed_by_org(org_id, conn) .await .into_iter() - .filter(|member| member.atype <= MembershipType::Admin as i32); + .filter(Membership::has_manage_reset_password_permission); for approver in approvers { let Some(admin) = User::find_by_uuid(&approver.user_uuid, conn).await else { @@ -2112,7 +2217,7 @@ async fn put_auth_request( auth_request.save(&conn).await?; ant.send_auth_response(&auth_request.user_uuid, &auth_request.uuid).await; - nt.send_auth_response(&auth_request.user_uuid, &auth_request.uuid, &headers.device, &conn).await; + nt.send_auth_response(&auth_request.user_uuid, &auth_request.uuid, Some(&headers.device), &conn).await; log_user_event( EventType::OrganizationUserApprovedAuthRequest as i32, @@ -2209,3 +2314,116 @@ pub async fn purge_auth_requests(pool: DbPool) { error!("Failed to get DB connection while purging auth requests"); } } + +#[cfg(test)] +mod tests { + use super::*; + + fn device(id: &str, trusted: bool) -> Device { + let mut device = Device::new(id.to_owned().into(), String::from("user").into(), String::new(), 9); + if trusted { + device.encrypted_user_key = Some(String::from("4.b2xkdXNlcmtleQ==")); + device.encrypted_public_key = Some(String::from("2.aXY=|Y2lwaGVy|bWFj")); + device.encrypted_private_key = Some(String::from("2.aXY=|Y2lwaGVy|bWFj")); + } + device + } + + fn update(device_id: &str) -> UpdateDeviceKeysData { + UpdateDeviceKeysData { + device_id: device_id.to_owned().into(), + encrypted_user_key: String::from("4.bmV3dXNlcmtleQ=="), + encrypted_public_key: String::from("2.aXY=|bmV3|bWFj"), + } + } + + /// The ids and keys the rotation would write, so a test can say what it expects in one line. + fn rotated(result: &[(DeviceId, String, String)]) -> Vec { + result.iter().map(|(device_id, user_key, _)| format!("{device_id}={user_key}")).collect() + } + + #[test] + fn a_trusted_device_that_is_listed_keeps_its_trust() { + let devices = [device("a", true), device("b", true)]; + let updates = [update("a"), update("b")]; + + let result = validate_device_keydata(&updates, &devices).unwrap(); + assert_eq!( + rotated(&result), + ["a=4.bmV3dXNlcmtleQ==", "b=4.bmV3dXNlcmtleQ=="], + "both are re-wrapped, neither keeps the previous user key" + ); + } + + #[test] + fn a_trusted_device_that_is_left_out_takes_the_rotation_down_with_it() { + // Silently dropping the trust of a device the user still relies on is not the server's call + // to make; the client untrusts it first if that is what it means. + let devices = [device("a", true), device("b", true)]; + + let err = validate_device_keydata(&[update("a")], &devices).unwrap_err(); + assert!(format!("{err}").contains("All existing trusted devices must be included")); + } + + #[test] + fn a_device_of_somebody_else_is_refused() { + let devices = [device("a", true)]; + + let err = validate_device_keydata(&[update("a"), update("stranger")], &devices).unwrap_err(); + assert!(format!("{err}").contains("does not belong to this user")); + } + + #[test] + fn the_same_device_may_not_be_listed_twice() { + // Two entries for one device means one of the two keys is dropped without anyone noticing + // which, so neither is taken. + let devices = [device("a", true)]; + + let err = validate_device_keydata(&[update("a"), update("a")], &devices).unwrap_err(); + assert!(format!("{err}").contains("listed more than once")); + } + + #[test] + fn a_key_that_is_not_an_encrypted_string_is_refused() { + let devices = [device("a", true)]; + + let mut broken = update("a"); + broken.encrypted_user_key = String::from("not an enc string"); + let err = validate_device_keydata(&[broken], &devices).unwrap_err(); + assert!(format!("{err}").contains("encryptedUserKey")); + + let mut broken = update("a"); + broken.encrypted_public_key = String::new(); + let err = validate_device_keydata(&[broken], &devices).unwrap_err(); + assert!(format!("{err}").contains("encryptedPublicKey")); + } + + #[test] + fn a_device_without_its_own_key_pair_is_not_given_a_user_key() { + // Half a trust is worth nothing to the client and would only fail at the next unlock, so + // the device is dropped from the rotation and ends up untrusted instead. + let devices = [device("a", true), device("b", false)]; + + let result = validate_device_keydata(&[update("a"), update("b")], &devices).unwrap(); + assert_eq!(rotated(&result), ["a=4.bmV3dXNlcmtleQ=="]); + } + + #[test] + fn a_user_who_trusts_no_device_rotates_nothing() { + let devices = [device("a", false)]; + + let result = validate_device_keydata(&[], &devices).unwrap(); + assert!(result.is_empty(), "and the leftovers of `a` are cleared by the write that follows"); + } + + #[test] + fn a_partially_trusted_device_does_not_have_to_be_listed() { + // It cannot unlock anything as it stands, so leaving it out is not the loss of a trust. + let mut half = device("b", true); + half.encrypted_user_key = None; + let devices = [device("a", true), half]; + + let result = validate_device_keydata(&[update("a")], &devices).unwrap(); + assert_eq!(rotated(&result), ["a=4.bmV3dXNlcmtleQ=="]); + } +} diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index b6a207c6..5d6b759f 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -12,13 +12,16 @@ use crate::{ AnonymousNotify, EmptyResult, JsonResult, Notify, PasswordOrOtpData, UpdateType, core::{CipherSyncData, CipherSyncType, accept_org_invite, log_event, two_factor}, }, - auth::{AdminHeaders, Headers, ManagerHeaders, ManagerHeadersLoose, OrgMemberHeaders, OwnerHeaders, decode_invite}, + auth::{ + AdminHeaders, Headers, ManageResetPasswordHeaders, ManagerHeaders, ManagerHeadersLoose, OrgMemberHeaders, + OwnerHeaders, decode_invite, + }, db::{ DbConn, models::{ AuthRequest, AuthRequestId, Cipher, CipherId, Collection, CollectionCipher, CollectionGroup, CollectionId, - CollectionUser, Device, DeviceType, EventType, Group, GroupId, GroupUser, Invitation, Membership, - MembershipId, MembershipStatus, MembershipType, OrgPolicy, OrgPolicyType, Organization, OrganizationApiKey, + CollectionUser, DeviceType, EventType, Group, GroupId, GroupUser, Invitation, Membership, MembershipId, + MembershipStatus, MembershipType, OrgPolicy, OrgPolicyType, Organization, OrganizationApiKey, OrganizationId, User, UserId, }, }, @@ -3214,7 +3217,11 @@ async fn put_reset_password_enrollment( /// The requests waiting for an answer in this organization. #[get("/organizations//auth-requests")] -async fn get_organization_auth_requests(org_id: OrganizationId, headers: AdminHeaders, conn: DbConn) -> JsonResult { +async fn get_organization_auth_requests( + org_id: OrganizationId, + headers: ManageResetPasswordHeaders, + conn: DbConn, +) -> JsonResult { if org_id != headers.org_id { err!("Organization not found", "Organization id's do not match"); } @@ -3290,7 +3297,7 @@ async fn update_organization_auth_request( org_id: OrganizationId, request_id: AuthRequestId, data: Json, - headers: AdminHeaders, + headers: ManageResetPasswordHeaders, conn: DbConn, ant: AnonymousNotify<'_>, nt: Notify<'_>, @@ -3314,7 +3321,7 @@ async fn update_organization_auth_request( async fn deny_organization_auth_requests( org_id: OrganizationId, data: Json, - headers: AdminHeaders, + headers: ManageResetPasswordHeaders, conn: DbConn, ant: AnonymousNotify<'_>, nt: Notify<'_>, @@ -3346,7 +3353,7 @@ async fn deny_organization_auth_requests( async fn update_many_organization_auth_requests( org_id: OrganizationId, data: Json>, - headers: AdminHeaders, + headers: ManageResetPasswordHeaders, conn: DbConn, ant: AnonymousNotify<'_>, nt: Notify<'_>, @@ -3381,7 +3388,7 @@ async fn answer_organization_auth_request( approved: bool, encrypted_user_key: Option, on_unanswerable: OnUnanswerable, - headers: &AdminHeaders, + headers: &ManageResetPasswordHeaders, conn: &DbConn, ant: &AnonymousNotify<'_>, nt: &Notify<'_>, @@ -3453,14 +3460,11 @@ async fn answer_organization_auth_request( ant.send_auth_response(&auth_request.user_uuid, &auth_request.uuid).await; - // The device that asked, not the one the administrator happens to be answering from: that one - // belongs to somebody else, and naming it here would both address the notification at a device - // of the wrong account and hand its identifiers to the push relay under a foreign user id. - if let Some(device) = - Device::find_by_uuid_and_user(&auth_request.request_device_identifier, &auth_request.user_uuid, conn).await - { - nt.send_auth_response(&auth_request.user_uuid, &auth_request.uuid, &device, conn).await; - } + // No acting device: the answer did not come from one of this user's devices, so every one of + // them, the one that asked above all, should hear about it. Naming the administrator's device + // here would leave it out of a notification meant for somebody else's account and hand its + // identifiers to the push relay under a foreign user id. + nt.send_auth_response(&auth_request.user_uuid, &auth_request.uuid, None, conn).await; if CONFIG.mail_enabled() && let Some(user) = User::find_by_uuid(&auth_request.user_uuid, conn).await diff --git a/src/api/identity.rs b/src/api/identity.rs index 84d332b4..a56cc65c 100644 --- a/src/api/identity.rs +++ b/src/api/identity.rs @@ -31,8 +31,8 @@ use crate::{ DbConn, models::{ AuthRequest, AuthRequestId, Device, DeviceId, DeviceType, EventType, Invitation, Membership, - MembershipStatus, MembershipType, OIDCCodeResponseError, OrganizationApiKey, OrganizationId, SendId, - SsoAuth, SsoUser, TwoFactor, TwoFactorIncomplete, TwoFactorType, User, UserId, + MembershipStatus, OIDCCodeResponseError, OrgPolicy, OrgPolicyType, Organization, OrganizationApiKey, + OrganizationId, SendId, SsoAuth, SsoUser, TwoFactor, TwoFactorIncomplete, TwoFactorType, User, UserId, }, }, error::MapResult, @@ -481,16 +481,98 @@ async fn password_login( authenticated_response(&user, &mut device, auth_tokens, twofactor_token, false, conn, ip).await } -/// Whether offering the trusted device options can lead anywhere for this account. +/// Whether the account creation the clients run when nothing else is on offer can succeed here. /// -/// Creating an account this way ends with enrolling into account recovery, which the clients do -/// unconditionally and which needs an organization to enroll into. An account that has nothing yet -/// and belongs to nowhere would therefore be shown the screen for a new account and get stuck -/// halfway through it, with its keys already written and its device still untrusted. Withholding -/// the options sends it to setting a master password instead, which works and leaves the door to -/// trusted devices open for the next login. -fn trusted_device_flow_is_completable(has_account_keys: bool, in_organization: bool) -> bool { - has_account_keys || in_organization +/// A client that gets the trusted device options, but neither a master password nor an approval an +/// administrator could give, decides it is looking at a fresh account and walks it through +/// creation: generate the account keys and post them, enrol into the account recovery of the +/// organization behind the SSO login, then trust the device. Every one of those has to be able to +/// go through. If enrolment is refused, the keys are already written, and the next login walks into +/// the same screen and fails at posting them a second time, leaving an account that can never be +/// unlocked at all. +/// +/// So the same conditions the enrolment endpoint enforces are checked here, before the client has +/// written anything. Withholding the options instead sends it to setting a master password, which +/// works and leaves the door to trusted devices open for the next login. +async fn account_creation_can_succeed(user: &User, conn: &DbConn) -> bool { + // `POST /accounts/keys` refuses to replace the keys of an account that has them, and the + // clients post a freshly generated pair without looking. + if user.private_key.is_some() || user.public_key.is_some() { + return false; + } + + // The organization the client enrols into is the one `GET /organizations//auto-enroll-status` + // hands it, so ask the same question here. + let Some(membership) = Membership::find_main_user_org(&user.uuid, conn).await else { + return false; + }; + + // What `check_reset_password_applicable` demands of that organization. + if !CONFIG.mail_enabled() { + return false; + } + if !OrgPolicy::find_by_org_and_type(&membership.org_uuid, OrgPolicyType::ResetPassword, conn) + .await + .is_some_and(|policy| policy.enabled) + { + return false; + } + + // Enrolling wraps the user key for the organization, so it needs its public key. + Organization::find_by_uuid(&membership.org_uuid, conn) + .await + .is_some_and(|org| org.public_key.is_some_and(|key| !key.is_empty())) +} + +/// The ways an account could get through the trusted device flow, which is what decides whether +/// offering it leads anywhere. +#[expect( + clippy::struct_excessive_bools, + reason = "Four independent facts about one account, not a state that could be an enum" +)] +struct TrustedDeviceWaysIn { + /// This device already holds the keys, so it unlocks without asking anyone. + device_is_trusted: bool, + /// A master password to fall back on. + has_master_password: bool, + /// An administrator of an organization who could let a new device in, which they can only do + /// once the member enrolled into account recovery. + has_admin_approval: bool, + /// Nothing set up yet, but the account creation the clients run in that case would go through. + can_create_account: bool, +} + +impl TrustedDeviceWaysIn { + /// Whether the trusted device options belong in a login response, and in which of their two + /// roles. + /// + /// `Some(true)` means they are only there to walk a user without a master password off the + /// feature after it was switched off; `None` means they are withheld, because nothing the + /// client could do with them would work. + /// + /// The order mirrors how the clients read them: a trusted device unlocks straight away, + /// otherwise an administrator to ask or a master password to type is offered, and only when + /// there is neither does the client decide it is looking at a fresh account and try to create + /// one. + fn offer(&self, enabled: bool) -> Option { + // Once the feature is switched off again, a user without a master password would be locked + // out of their own vault. Keep telling their still trusted devices about it so their client + // can walk them through setting one while they can still unlock. + let offboarding = !enabled && self.offboarding_candidate(); + if !(enabled || offboarding) { + return None; + } + + let leads_somewhere = + self.device_is_trusted || self.has_admin_approval || self.has_master_password || self.can_create_account; + leads_somewhere.then_some(offboarding) + } + + /// A user who is still on a trusted device and has no master password to fall back on, and so + /// has to be told when the feature goes away. + fn offboarding_candidate(&self) -> bool { + self.device_is_trusted && !self.has_master_password + } } /// Trusted device encryption ("passwordless SSO"): instead of deriving the user key from a master @@ -504,20 +586,37 @@ fn trusted_device_flow_is_completable(has_account_keys: bool, in_organization: b async fn trusted_device_option(user: &User, device: &Device, conn: &DbConn) -> Option { let enabled = CONFIG.sso_trusted_device_encryption(); - // Once the feature is switched off again, a user without a master password would be locked out - // of their own vault. Keep telling their still trusted devices about it so their client can walk - // them through setting one while they can still unlock. - let offboarding = !enabled && device.is_trusted() && user.password_hash.is_empty(); - if !enabled && !offboarding { + let mut ways_in = TrustedDeviceWaysIn { + device_is_trusted: device.is_trusted(), + has_master_password: !user.password_hash.is_empty(), + has_admin_approval: false, + can_create_account: false, + }; + + // Answered ahead of everything else so a server that does not offer trusted devices, and has no + // user left on them, does no work for the feature at all. + if !(enabled || ways_in.offboarding_candidate()) { return None; } let memberships = Membership::find_by_user(&user.uuid, conn).await; - if !trusted_device_flow_is_completable(user.private_key.is_some(), !memberships.is_empty()) { - return None; + // An admin can only take over the approval once the member handed them a key to work with, + // which is what enrolling into account recovery does. Only a confirmed membership counts, the + // same condition the request itself is created and answered under, so this does not announce a + // way out that would be refused the moment it is taken. + ways_in.has_admin_approval = memberships.iter().any(|member| { + member.status == MembershipStatus::Confirmed as i32 + && member.reset_password_key.as_ref().is_some_and(|key| !key.is_empty()) + }); + + // Only worth asking when nothing cheaper already lets the client in. + if !(ways_in.device_is_trusted || ways_in.has_admin_approval || ways_in.has_master_password) { + ways_in.can_create_account = account_creation_can_succeed(user, conn).await; } + let offboarding = ways_in.offer(enabled)?; + // Any other device of this user that could show an approval prompt. The user unlocks a new // device from one of these, or with the master password if they have one. let has_login_approving_device = Device::find_by_user(&user.uuid, conn) @@ -525,24 +624,14 @@ async fn trusted_device_option(user: &User, device: &Device, conn: &DbConn) -> O .iter() .any(|other| other.uuid != device.uuid && DeviceType::from_i32(other.atype).can_approve_login_requests()); - // An admin can only take over the approval once the member handed them a key to work with, - // which is what enrolling into account recovery does. Only a confirmed membership counts, the - // same condition the request itself is created and answered under, so this does not announce a - // way out that would be refused the moment it is taken. - let has_admin_approval = memberships.iter().any(|member| { - member.status == MembershipStatus::Confirmed as i32 - && member.reset_password_key.as_ref().is_some_and(|key| !key.is_empty()) - }); - // Whether the user is on the answering side of that. The clients use it to push someone who - // could approve others, but has no master password themselves, into setting one. Matches what - // `AdminHeaders` actually lets through. - let has_manage_reset_password_permission = memberships.iter().any(|member| { - member.status == MembershipStatus::Confirmed as i32 && member.atype <= MembershipType::Admin as i32 - }); + // could approve others, but has no master password themselves, into setting one. Upstream reads + // a `ManageResetPassword` permission here, which in Vaultwarden's role model only the + // administrators of an organization have. + let has_manage_reset_password_permission = memberships.iter().any(Membership::has_manage_reset_password_permission); Some(json!({ - "HasAdminApproval": has_admin_approval, + "HasAdminApproval": ways_in.has_admin_approval, "HasLoginApprovingDevice": has_login_approving_device, "HasManageResetPasswordPermission": has_manage_reset_password_permission, "IsTdeOffboarding": offboarding, @@ -1407,19 +1496,132 @@ async fn authorize(data: AuthorizeData, cookies: &CookieJar<'_>, secure: Secure, mod tests { use super::*; + /// A `TrustedDeviceWaysIn` plus the server setting, so the cases below read as what they are. + #[expect(clippy::struct_excessive_bools, reason = "Mirrors the struct under test")] + struct Account { + enabled: bool, + device_is_trusted: bool, + has_master_password: bool, + has_admin_approval: bool, + can_create_account: bool, + } + + impl Account { + /// A user of a server that offers trusted devices, on a device it does not know yet, with + /// nothing set up: the shape everything below varies from. + fn new() -> Self { + Self { + enabled: true, + device_is_trusted: false, + has_master_password: false, + has_admin_approval: false, + can_create_account: false, + } + } + + fn offer(&self) -> Option { + TrustedDeviceWaysIn { + device_is_trusted: self.device_is_trusted, + has_master_password: self.has_master_password, + has_admin_approval: self.has_admin_approval, + can_create_account: self.can_create_account, + } + .offer(self.enabled) + } + } + + #[test] + fn a_server_that_does_not_offer_trusted_devices_says_nothing_about_them() { + for (device_is_trusted, has_master_password) in [(false, false), (false, true), (true, true)] { + let account = Account { + enabled: false, + device_is_trusted, + has_master_password, + ..Account::new() + }; + assert_eq!(account.offer(), None); + } + } + #[test] - fn an_account_with_nothing_and_nowhere_to_go_is_not_offered_trusted_devices() { - // The one combination the clients cannot finish: nothing set up yet and no organization - // to enroll into. - assert!(!trusted_device_flow_is_completable(false, false)); - - // A brand new account that was invited somewhere can enroll, so the flow completes. - assert!(trusted_device_flow_is_completable(false, true)); - - // An account that is already set up does not go through account creation at all, with or - // without an organization. This covers the master password first route as well as an - // account that already trusts a device. - assert!(trusted_device_flow_is_completable(true, false)); - assert!(trusted_device_flow_is_completable(true, true)); + fn a_user_left_on_a_trusted_device_is_walked_off_the_feature() { + // The feature is gone but this device still unlocks and its owner has no master password. + // They are told so, so their client can walk them through setting one while they still can. + let account = Account { + enabled: false, + device_is_trusted: true, + ..Account::new() + }; + assert_eq!(account.offer(), Some(true), "offboarding"); + + // With the feature on, the same device is simply trusted. + let account = Account { + device_is_trusted: true, + ..Account::new() + }; + assert_eq!(account.offer(), Some(false)); + } + + #[test] + fn an_account_with_no_way_through_the_flow_is_not_offered_it() { + // Nothing set up, nobody to ask, and account creation would fail at the enrolment: the one + // combination that would leave the account half built. The client is sent to setting a + // master password instead. + assert_eq!(Account::new().offer(), None); + } + + #[test] + fn every_way_through_the_flow_is_offered_it() { + // A device that can unlock right now. + assert_eq!( + Account { + device_is_trusted: true, + ..Account::new() + } + .offer(), + Some(false) + ); + + // An administrator to ask, which needs the member to be enrolled in account recovery. + assert_eq!( + Account { + has_admin_approval: true, + ..Account::new() + } + .offer(), + Some(false) + ); + + // A master password to fall back on. + assert_eq!( + Account { + has_master_password: true, + ..Account::new() + } + .offer(), + Some(false) + ); + + // A fresh account in an organization that can actually take the enrolment. + assert_eq!( + Account { + can_create_account: true, + ..Account::new() + } + .offer(), + Some(false) + ); + } + + #[test] + fn a_user_with_a_master_password_is_never_offboarded() { + // There is nothing to walk them off, they can unlock either way. + let account = Account { + enabled: false, + device_is_trusted: true, + has_master_password: true, + ..Account::new() + }; + assert_eq!(account.offer(), None); } } diff --git a/src/api/notifications.rs b/src/api/notifications.rs index 8bfcd518..2cd6ecf8 100644 --- a/src/api/notifications.rs +++ b/src/api/notifications.rs @@ -529,11 +529,14 @@ impl WebSocketUsers { } } + /// `acting_device` is the device of this user that answered the request, and is the one left out + /// of the notification. An answer that came from outside their devices, as an approval by an + /// administrator of their organization does, passes `None` so that all of them hear about it. pub async fn send_auth_response( &self, user_id: &UserId, auth_request_id: &AuthRequestId, - device: &Device, + acting_device: Option<&Device>, conn: &DbConn, ) { // Skip any processing if both WebSockets and Push are not active @@ -543,14 +546,14 @@ impl WebSocketUsers { let data = create_update( vec![("Id".into(), auth_request_id.to_string().into()), ("UserId".into(), user_id.to_string().into())], UpdateType::AuthRequestResponse, - Some(device.uuid.clone()), + acting_device.map(|device| device.uuid.clone()), ); if CONFIG.enable_websocket() { self.send_update(user_id, &data).await; } if CONFIG.push_enabled() { - push_auth_response(user_id, auth_request_id, device, conn).await; + push_auth_response(user_id, auth_request_id, acting_device, conn).await; } } } diff --git a/src/api/push.rs b/src/api/push.rs index e87a0985..87c52ede 100644 --- a/src/api/push.rs +++ b/src/api/push.rs @@ -317,13 +317,23 @@ pub async fn push_auth_request(user_id: &UserId, auth_request_id: &str, device: } } -pub async fn push_auth_response(user_id: &UserId, auth_request_id: &AuthRequestId, device: &Device, conn: &DbConn) { +/// `acting_device` is the device that answered, and is the one device left out of the notification, +/// since it already knows. An answer that did not come from a device of this user at all, as an +/// approval by an administrator of their organization does, leaves it out: naming a device of +/// somebody else here would both hand its identifiers to the push relay under a foreign user id and +/// tell the wrong device to ignore the answer. +pub async fn push_auth_response( + user_id: &UserId, + auth_request_id: &AuthRequestId, + acting_device: Option<&Device>, + conn: &DbConn, +) { if Device::check_user_has_push_device(user_id, conn).await { tokio::task::spawn(send_to_push_relay(json!({ "userId": user_id, "organizationId": null, - "deviceId": device.push_uuid, // Should be the records unique uuid of the acting device (unique uuid per user/device) - "identifier": device.uuid, // Should be the acting device id (aka uuid per device/app) + "deviceId": acting_device.and_then(|device| device.push_uuid.as_ref()), // Should be the records unique uuid of the acting device (unique uuid per user/device) + "identifier": acting_device.map(|device| &device.uuid), // Should be the acting device id (aka uuid per device/app) "type": UpdateType::AuthRequestResponse as i32, "payload": { "userId": user_id, diff --git a/src/auth.rs b/src/auth.rs index 762088e5..6431178f 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -843,6 +843,41 @@ impl<'r> FromRequest<'r> for AdminHeaders { } } +/// A member who may act on the account recovery of an organization, which is also what answering +/// its device approvals comes down to. +/// +/// Upstream guards those endpoints on a permission, `ManageResetPassword`, rather than on a role, +/// so this asks `Membership::has_manage_reset_password_permission` instead of naming roles here. +/// Today that permission belongs to the administrators of an organization and to nobody else, which +/// makes this the same set of callers as `AdminHeaders`; keeping it apart is what lets a custom role +/// hold the permission later without every endpoint having to be revisited. +/// https://github.com/bitwarden/server/blob/main/src/Api/AdminConsole/Controllers/OrganizationAuthRequestsController.cs +pub struct ManageResetPasswordHeaders { + pub device: Device, + pub user: User, + pub ip: ClientIp, + pub org_id: OrganizationId, +} + +#[rocket::async_trait] +impl<'r> FromRequest<'r> for ManageResetPasswordHeaders { + type Error = &'static str; + + async fn from_request(request: &'r Request<'_>) -> Outcome { + let headers = try_outcome!(OrgHeaders::from_request(request).await); + if headers.membership.has_manage_reset_password_permission() { + Outcome::Success(Self { + device: headers.device, + user: headers.user, + ip: headers.ip, + org_id: headers.membership.org_uuid, + }) + } else { + err_handler!("You need permission to manage account recovery to call this endpoint") + } + } +} + // col_id is usually the fourth path param ("/organizations//collections/"), // but there could be cases where it is a query value. // First check the path, if this is not a valid uuid, try the query values. diff --git a/src/db/models/auth_request.rs b/src/db/models/auth_request.rs index bda821fe..d1a1a993 100644 --- a/src/db/models/auth_request.rs +++ b/src/db/models/auth_request.rs @@ -142,8 +142,13 @@ impl AuthRequest { }) } - /// What an administrator gets to see about a request. Deliberately without the access code: - /// that one is the requesting device's proof, not something the answering side needs. + /// What an administrator gets to see about a request that is waiting for them, which is the + /// public key of the asking device and enough about it to recognise it. Same shape as + /// `PendingOrganizationAuthRequestResponseModel` upstream. + /// + /// Deliberately no access code, which is the asking device's own proof, and no wrapped key: a + /// request that is still waiting has none, and handing one out here would be crypto material + /// the answering side has no use for. pub fn to_json_for_organization(&self, email: &str, member_id: &MembershipId) -> Value { json!({ "id": self.uuid, @@ -154,11 +159,10 @@ impl AuthRequest { "requestDeviceIdentifier": self.request_device_identifier, "requestDeviceType": DeviceType::from_i32(self.device_type).to_string(), "requestIpAddress": self.request_ip, - "key": self.enc_key, + // Not recorded here, but the clients read it, so it is answered rather than missing. + "requestCountryName": null, "creationDate": format_date(&self.creation_date), - "requestApproved": self.approved, - "responseDate": self.response_date.as_ref().map(format_date), - "object": "organizationAuthRequest", + "object": "pending-org-auth-request", }) } } @@ -252,12 +256,18 @@ impl AuthRequest { /// /// Asking again from the same device updates that one instead of adding another, so a client /// that retries cannot fill the table or mail the administrators over and over. + /// + /// A request past its window does not count: it is one nobody can answer any more, and reviving + /// it by moving its date forward would leave the user waiting on a request the administrators + /// were never told about. Asking again after it ran out is a new request, and is announced. pub async fn find_pending_admin_approval( user_uuid: &UserId, device_uuid: &DeviceId, org_uuid: &OrganizationId, conn: &DbConn, ) -> Option { + let oldest = Utc::now().naive_utc() - Self::admin_request_expiration(); + conn.run(move |conn| { auth_requests::table .filter(auth_requests::user_uuid.eq(user_uuid)) @@ -265,6 +275,7 @@ impl AuthRequest { .filter(auth_requests::organization_uuid.eq(org_uuid)) .filter(auth_requests::atype.eq(AuthRequestType::AdminApproval as i32)) .filter(auth_requests::approved.is_null()) + .filter(auth_requests::creation_date.gt(oldest)) .order_by(auth_requests::creation_date.desc()) .first::(conn) .ok() @@ -421,6 +432,24 @@ mod tests { assert!(request(AuthRequestType::AdminApproval, TimeDelta::try_days(8).unwrap()).is_expired()); } + #[test] + fn a_request_nobody_answered_in_time_is_not_still_pending() { + // `find_pending_admin_approval` decides whether asking again reuses the open request or + // starts a new one, and filters on the same window as this. A request past it must not come + // back: reviving it by moving its date forward would leave the user waiting on something + // the administrators were never told about, because only a new request mails them. + let mut auth_request = + request(AuthRequestType::AdminApproval, AuthRequest::admin_request_expiration() + TimeDelta::seconds(1)); + assert_eq!(auth_request.approved, None, "still unanswered"); + assert!(auth_request.is_expired()); + + // One minute short of the window is still the same request, and asking again updates it + // rather than mailing everyone a second time. + auth_request.creation_date = + Utc::now().naive_utc() - AuthRequest::admin_request_expiration() + TimeDelta::minutes(1); + assert!(!auth_request.is_expired()); + } + #[test] fn the_answer_of_an_administrator_starts_its_own_clock() { // Answered right at the end of the week, so the request itself is long past its window. diff --git a/src/db/models/device.rs b/src/db/models/device.rs index c0192c8a..2ecb14d2 100644 --- a/src/db/models/device.rs +++ b/src/db/models/device.rs @@ -116,19 +116,6 @@ impl Device { Self::present(self.encrypted_private_key.as_ref()).is_some() } - /// Whether any part of a trust is stored, complete or not. - pub fn holds_any_key(&self) -> bool { - Self::present(self.encrypted_user_key.as_ref()).is_some() - || Self::present(self.encrypted_public_key.as_ref()).is_some() - || self.holds_private_key() - } - - pub fn untrust(&mut self) { - self.encrypted_user_key = None; - self.encrypted_public_key = None; - self.encrypted_private_key = None; - } - pub fn to_json(&self) -> Value { json!({ "id": self.uuid, @@ -264,12 +251,13 @@ impl Device { /// Invalidates every copy of the user key that is wrapped for one of the user's devices. /// - /// Called when the user key itself is replaced, which leaves all of those copies pointing at a - /// key that no longer unlocks anything. No device counts as trusted afterwards, so a client - /// that stops here ends up with an extra login rather than a broken unlock. The device key - /// pairs are deliberately left alone: they are wrapped with the device key, which a rotation - /// does not touch, so `POST /devices/update-trust` can hand every device the new user key and - /// restore its trust. Whatever it does not list is dropped there. + /// Called when the user key itself is replaced and the client did not say what to put in their + /// place, which leaves all of those copies pointing at a key that no longer unlocks anything. No + /// device counts as trusted afterwards, so a client that stops here ends up with an extra login + /// rather than a broken unlock. The device key pairs are deliberately left alone: they are + /// wrapped with the device key, which a rotation does not touch, so `POST /devices/update-trust` + /// can hand every device the new user key and restore its trust. Whatever it does not list is + /// dropped there. /// /// One statement, so there is no half applied state to reason about. pub async fn invalidate_wrapped_user_keys(user_uuid: &UserId, conn: &DbConn) -> EmptyResult { @@ -285,6 +273,82 @@ impl Device { .await } + /// Drops every stored key of the named devices, in one statement so it cannot half apply. + /// + /// The caller has already checked that each id belongs to this user. + pub async fn untrust_many(user_uuid: &UserId, device_ids: Vec, conn: &DbConn) -> EmptyResult { + if device_ids.is_empty() { + return Ok(()); + } + + conn.run(move |conn| { + diesel::update( + devices::table.filter(devices::user_uuid.eq(user_uuid)).filter(devices::uuid.eq_any(device_ids)), + ) + .set(( + devices::encrypted_user_key.eq::>(None), + devices::encrypted_public_key.eq::>(None), + devices::encrypted_private_key.eq::>(None), + )) + .execute(conn) + .map_res("Error untrusting the devices") + }) + .await + } + + /// Replaces the trust of every device of the user in one go: the listed ones are re-wrapped for + /// the current user key, everything else loses whatever it still holds. + /// + /// This is what both a key rotation and `POST /devices/update-trust` come down to. The caller + /// has already checked that every id belongs to this user, that none is listed twice, and that + /// no device is asked to keep a trust it cannot complete; this only writes. + /// + /// One transaction, so the devices cannot be left split between the old and the new user key, + /// which is a state no client can tell apart from a working one until an unlock fails. + pub async fn replace_trust( + user_uuid: &UserId, + updates: Vec<(DeviceId, String, String)>, + conn: &DbConn, + ) -> EmptyResult { + conn.run(move |conn| { + conn.transaction(|conn| -> EmptyResult { + let keep: Vec = updates.iter().map(|(device_id, ..)| device_id.clone()).collect(); + + // Whatever the untouched devices hold wraps the previous user key, or is one half of + // a trust that was never finished. Either way it unlocks nothing and must not stay. + let cleared = ( + devices::encrypted_user_key.eq::>(None), + devices::encrypted_public_key.eq::>(None), + devices::encrypted_private_key.eq::>(None), + ); + let outdated = devices::table.filter(devices::user_uuid.eq(&user_uuid)); + let _: () = if keep.is_empty() { + diesel::update(outdated).set(cleared).execute(conn) + } else { + diesel::update(outdated.filter(devices::uuid.ne_all(keep))).set(cleared).execute(conn) + } + .map_res("Error untrusting the devices left out of the rotation")?; + + // The device key pair is deliberately not touched here: it is wrapped with the + // device key, which the server never sees and a rotation never changes. + for (device_id, encrypted_user_key, encrypted_public_key) in updates { + let _: () = diesel::update( + devices::table.filter(devices::uuid.eq(device_id)).filter(devices::user_uuid.eq(&user_uuid)), + ) + .set(( + devices::encrypted_user_key.eq(Some(encrypted_user_key)), + devices::encrypted_public_key.eq(Some(encrypted_public_key)), + )) + .execute(conn) + .map_res("Error rotating the wrapped user key of a device")?; + } + + Ok(()) + }) + }) + .await + } + pub async fn find_by_uuid_and_user(uuid: &DeviceId, user_uuid: &UserId, conn: &DbConn) -> Option { conn.run(move |conn| { devices::table @@ -550,32 +614,18 @@ mod tests { assert!(!device.is_trusted(), "nothing may unlock until the client re-wraps"); assert!(device.holds_private_key(), "but the device can still be handed a new user key"); - assert!(device.holds_any_key()); } #[test] fn a_device_that_never_had_a_trust_holds_nothing() { let device = Device::new(String::from("device").into(), String::from("user").into(), String::new(), 9); assert!(!device.holds_private_key()); - assert!(!device.holds_any_key()); let mut device = trusted_device(); device.encrypted_private_key = Some(String::new()); assert!(!device.holds_private_key(), "an empty key is as good as a missing one"); } - #[test] - fn untrusting_clears_every_key() { - let mut device = trusted_device(); - device.untrust(); - - assert!(!device.is_trusted()); - assert!(!device.holds_any_key()); - assert_eq!(device.encrypted_user_key, None); - assert_eq!(device.encrypted_public_key, None); - assert_eq!(device.encrypted_private_key, None); - } - #[test] fn only_interactive_clients_can_approve_a_login_request() { for atype in 0..=26 { diff --git a/src/db/models/organization.rs b/src/db/models/organization.rs index bdb69864..f9628a62 100644 --- a/src/db/models/organization.rs +++ b/src/db/models/organization.rs @@ -277,6 +277,20 @@ impl Membership { } } + /// Whether this membership may act on the account recovery of the organization's members: + /// reset their master password, and answer the device approvals they ask their organization for. + /// + /// Upstream is a permission of its own, `ManageResetPassword`, which an administrator has by + /// virtue of the role and a custom role can be granted separately. Vaultwarden folds the custom + /// role into `Manager` and drops the permissions that came with it, so only the administrators + /// are left holding it. Asking here rather than comparing roles at each call site keeps that one + /// decision in one place for when custom roles arrive. + /// https://github.com/bitwarden/server/blob/main/src/Core/Context/CurrentContext.cs + pub fn has_manage_reset_password_permission(&self) -> bool { + self.status == MembershipStatus::Confirmed as i32 + && MembershipType::from_i32(self.atype).is_some_and(|atype| atype >= MembershipType::Admin) + } + pub fn restore(&mut self) -> bool { if self.status < MembershipStatus::Invited as i32 { self.status += ACTIVATE_REVOKE_DIFF; @@ -1285,4 +1299,32 @@ mod tests { assert!(MembershipType::Manager > MembershipType::User); assert!(MembershipType::Manager == MembershipType::from_str("4").unwrap()); } + + #[test] + fn only_a_confirmed_administrator_manages_account_recovery() { + let mut membership = Membership::new(String::from("user").into(), String::from("org").into(), None); + + for (atype, expected) in [ + (MembershipType::Owner, true), + (MembershipType::Admin, true), + // The custom role is folded into the manager one, losing whatever permissions came + // with it, so it cannot be assumed to hold this one. + (MembershipType::Manager, false), + (MembershipType::User, false), + ] { + membership.atype = atype as i32; + + for status in [MembershipStatus::Revoked, MembershipStatus::Invited, MembershipStatus::Accepted] { + let status = status as i32; + membership.status = status; + assert!( + !membership.has_manage_reset_password_permission(), + "a membership that is not confirmed manages nothing, status {status}" + ); + } + + membership.status = MembershipStatus::Confirmed as i32; + assert_eq!(membership.has_manage_reset_password_permission(), expected, "type {}", atype as i32); + } + } } diff --git a/src/util.rs b/src/util.rs index b8b86bf6..75eaa0d7 100644 --- a/src/util.rs +++ b/src/util.rs @@ -543,40 +543,81 @@ pub fn is_valid_email(email: &str) -> bool { true } -/// The most an `EncString` we are willing to store may weigh. The largest legitimate one is an -/// RSA-4096 envelope with a MAC, which stays an order of magnitude below this. +/// The most an `EncString` we are willing to store may weigh. +/// +/// Upstream puts no length on the fields this guards; this is ours, so a client cannot park +/// megabytes in a column that is supposed to hold a wrapped key. The largest thing that legitimately +/// lands there is a device's RSA-2048 private key wrapped with AES-CBC plus a MAC, around 1.7 kB, so +/// this leaves room to spare. const MAX_ENC_STRING_LENGTH: usize = 4096; +/// The number of `|` separated parts an `EncString` of the given `EncryptionType` is made of. +/// +/// - `3` Rsa2048_OaepSha256_B64 and `4` Rsa2048_OaepSha1_B64 are the ciphertext by itself, and +/// `7` XChaCha20Poly1305_B64 is one blob of COSE bytes; +/// - `0` AesCbc256_B64 is `iv|ct`, while `5` Rsa2048_OaepSha256_HmacSha256_B64 and +/// `6` Rsa2048_OaepSha1_HmacSha256_B64 are `rsaCt|mac`; +/// - `1` AesCbc128_HmacSha256_B64 and `2` AesCbc256_HmacSha256_B64 are `iv|ct|mac`. +/// +/// https://github.com/bitwarden/server/blob/main/src/Core/Enums/EncryptionType.cs +fn enc_string_parts(enc_type: u8) -> Option { + match enc_type { + 3 | 4 | 7 => Some(1), + 0 | 5 | 6 => Some(2), + 1 | 2 => Some(3), + _ => None, + } +} + +/// Whether a single part is base64, as permissively as upstream reads it. +/// +/// Upstream accepts a final character whose unused padding bits are not zero, because such values +/// exist in the wild; a strict decoder rejects them. Everything else is the usual shape: a multiple +/// of four characters from the base64 alphabet, with at most two `=` closing it off. +/// https://github.com/bitwarden/server/blob/main/src/Core/Utilities/EncryptedStringAttribute.cs +fn is_valid_base64_permissive(value: &str) -> bool { + if value.is_empty() || !value.len().is_multiple_of(4) { + return false; + } + + // A group of four holds at least two characters of data, so at most two may be padded away. + let data = value.strip_suffix("==").or_else(|| value.strip_suffix('=')).unwrap_or(value); + + !data.is_empty() && data.bytes().all(|b| b.is_ascii_alphanumeric() || b == b'+' || b == b'/') +} + /// Whether a value has the shape of a Bitwarden `EncString`: `.|...`. /// /// The server cannot tell whether a blob decrypts, but it can refuse everything that is not even /// of the right form, which keeps unbounded junk out of the columns that hold key material. -/// Mirrors `EncryptedStringAttribute` upstream. +/// Mirrors `EncryptedStringAttribute` upstream, including its header-less legacy form, but not its +/// acceptance of a type spelled out by name (`AesCbc256_B64.…`), which no client has ever written. /// https://github.com/bitwarden/server/blob/main/src/Core/Utilities/EncryptedStringAttribute.cs pub fn is_valid_enc_string(value: &str) -> bool { if value.is_empty() || value.len() > MAX_ENC_STRING_LENGTH { return false; } - let Some((enc_type, data)) = value.split_once('.') else { - return false; - }; - - // The number of `|` separated parts each type is made of. - let parts = match enc_type { - // An RSA envelope is the ciphertext by itself. - "3" | "4" => 1, - // An AES value carries its IV, an RSA one from type 5 on carries a MAC. - "0" | "5" | "6" => 2, - // And an AES value from type 1 on carries both. - "1" | "2" => 3, - _ => return false, + let (parts, data) = if let Some((enc_type, data)) = value.split_once('.') { + let Some(parts) = enc_type.parse::().ok().and_then(enc_string_parts) else { + return false; + }; + (parts, data) + } else { + // Without a header the type is guessed from the number of parts, the same two candidates + // upstream picks between: three means it carries a MAC, anything else is read as iv|ct. + let parts = if value.matches('|').count() == 2 { + 3 + } else { + 2 + }; + (parts, value) }; let mut seen = 0; for part in data.split('|') { seen += 1; - if seen > parts || part.is_empty() || data_encoding::BASE64.decode(part.as_bytes()).is_err() { + if seen > parts || !is_valid_base64_permissive(part) { return false; } } @@ -591,18 +632,33 @@ mod enc_string_tests { #[test] fn a_well_formed_enc_string_of_every_type_is_accepted() { for value in [ - "0.aXY=|Y2lwaGVy", - "1.aXY=|Y2lwaGVy|bWFj", - "2.aXY=|Y2lwaGVy|bWFj", - "3.Y2lwaGVy", - "4.Y2lwaGVy", - "5.Y2lwaGVy|bWFj", - "6.Y2lwaGVy|bWFj", + "0.aXY=|Y2lwaGVy", // AesCbc256_B64 + "1.aXY=|Y2lwaGVy|bWFj", // AesCbc128_HmacSha256_B64 + "2.aXY=|Y2lwaGVy|bWFj", // AesCbc256_HmacSha256_B64 + "3.Y2lwaGVy", // Rsa2048_OaepSha256_B64 + "4.Y2lwaGVy", // Rsa2048_OaepSha1_B64 + "5.Y2lwaGVy|bWFj", // Rsa2048_OaepSha256_HmacSha256_B64 + "6.Y2lwaGVy|bWFj", // Rsa2048_OaepSha1_HmacSha256_B64 + "7.Y29zZWJ5dGVz", // XChaCha20Poly1305_B64, one blob of COSE bytes + "07.Y29zZWJ5dGVz", // the header is read as a number, not matched as text + "aXY=|Y2lwaGVy", // header-less legacy form, read as iv|ct + "aXY=|Y2lwaGVy|bWFj", // and as iv|ct|mac when it has three parts + "3.Y2lwaGVyLysvdGV4dA==", // the whole base64 alphabet, padded + "3.Y2lwaGVyLysvdGV4dGE=", // and with a single pad character ] { assert!(is_valid_enc_string(value), "{value}"); } } + #[test] + fn a_non_canonical_final_character_is_accepted() { + // The unused padding bits of the last character are not zero. A strict decoder refuses + // these, upstream deliberately does not, and such values exist in the wild. + assert!(is_valid_enc_string("3.QR==")); + assert!(is_valid_enc_string("3.QUJDRR==")); + assert!(is_valid_enc_string("2.aXY=|QR==|bWFj")); + } + #[test] fn anything_that_is_not_one_is_refused() { for value in [ @@ -612,13 +668,24 @@ mod enc_string_tests { "2", "2.", ".aXY=|Y2lwaGVy|bWFj", - "7.Y2lwaGVy", // no such type + "8.Y2lwaGVy", // no such type + "255.Y2lwaGVy", // nor at the top of the byte the header is read as + "256.Y2lwaGVy", // nor past it "-1.Y2lwaGVy", // and none below zero either "2.aXY=|Y2lwaGVy", // type 2 without its mac "2.aXY=|Y2lwaGVy|bWFj|x", // or with one part too many "4.Y2lwaGVy|bWFj", // type 4 carries no mac + "7.Y29zZQ==|bWFj", // and neither does type 7 "2.aXY=||bWFj", // an empty part is not base64 "4.not base64!", + "4.Y2lwaGV", // a length that is not a multiple of four + "4.Y2lwaGVy=", // a stray pad character breaks that length + "4.====", // padding only + "4.Y2lw=GVy", // padding in the middle + "4.Y2lw-GVy", // url-safe base64 is a different alphabet + "4.Y2lw GVy", // whitespace is not part of it either + "aXY=", // header-less, but only one part + "aXY=|Y2lwaGVy|bWFj|Zm91cg==", // header-less with four ] { assert!(!is_valid_enc_string(value), "{value}"); } @@ -629,6 +696,12 @@ mod enc_string_tests { let payload = "A".repeat(4096); assert!(is_valid_enc_string(&format!("4.{}", &payload[..4000]))); assert!(!is_valid_enc_string(&format!("4.{payload}")), "must not grow without bound"); + + // A wrapped RSA-2048 device private key, the largest thing that legitimately arrives here, + // has to fit with room to spare. + let private_key = format!("2.{}|{}|{}", "A".repeat(24), "B".repeat(1652), "C".repeat(44)); + assert!(private_key.len() < 2048); + assert!(is_valid_enc_string(&private_key)); } } From b953744be7f16a71eabd5808a41035fc26663c53 Mon Sep 17 00:00:00 2001 From: tom27052006 <83423411+tom27052006@users.noreply.github.com> Date: Sat, 29 Aug 2026 19:08:42 +0200 Subject: [PATCH 09/10] Address further trusted device review findings --- src/api/core/accounts.rs | 192 +++++++++++++++++++++++++------ src/api/core/organizations.rs | 15 +-- src/api/identity.rs | 89 +++++++++++++-- src/auth.rs | 4 +- src/db/models/event.rs | 2 +- src/db/models/organization.rs | 207 ++++++++++++++++++++++++++++++++-- 6 files changed, 449 insertions(+), 60 deletions(-) diff --git a/src/api/core/accounts.rs b/src/api/core/accounts.rs index fb3abf08..2f4b6483 100644 --- a/src/api/core/accounts.rs +++ b/src/api/core/accounts.rs @@ -1,4 +1,4 @@ -use std::collections::HashSet; +use std::collections::{HashMap, HashSet}; use chrono::Utc; use rocket::{ @@ -22,8 +22,8 @@ use crate::{ models::{ AuthRequest, AuthRequestId, AuthRequestType, Cipher, CipherId, Device, DeviceId, DeviceType, DeviceWithAuthRequest, EmergencyAccess, EmergencyAccessId, EventType, Folder, FolderId, Invitation, - Membership, MembershipId, MembershipStatus, OrgPolicy, OrgPolicyType, Organization, OrganizationId, Send, - SendId, User, UserId, UserKdfType, + Membership, MembershipId, OrgPolicy, OrgPolicyType, Organization, OrganizationId, Send, SendId, User, + UserId, UserKdfType, }, }, mail, @@ -45,6 +45,7 @@ pub fn routes() -> Vec { post_keys, post_password, post_set_password, + put_update_tde_offboarding_password, post_kdf, post_rotatekey, post_sstamp, @@ -522,6 +523,88 @@ async fn post_set_password(data: Json, headers: Headers, conn: }))) } +#[derive(Deserialize)] +#[serde(rename_all = "camelCase")] +struct UpdateTdeOffboardingPasswordData { + new_master_password_hash: String, + /// The user key the account already has, re-wrapped for the master key derived from the new + /// password. The vault is not re-encrypted, so this is the only thing that changes about it. + key: String, + master_password_hint: Option, +} + +/// Gives an account that unlocks with a trusted device the master password it needs once the server +/// stops offering trusted devices. +/// +/// This is the endpoint the clients take when a login answered `IsTdeOffboarding`, see +/// `trusted_device_option`. It is deliberately not `/accounts/set-password`: the account is fully +/// set up by this point, so the only thing being added is a second way to unlock the user key it +/// already has. The account key pair and the vault are left exactly as they are, and unlike +/// `/accounts/keys` there is nothing here that could replace them. +/// +/// Upstream keys this on the organization having switched its SSO member decryption away from +/// trusted devices; Vaultwarden configures SSO for the whole server, so the same state is +/// `SSO_ENABLED` without `SSO_TRUSTED_DEVICE_ENCRYPTION`, which is exactly when a login starts +/// answering `IsTdeOffboarding`. +/// https://github.com/bitwarden/server/blob/main/src/Core/Auth/UserFeatures/TdeOffboardingPassword/TdeOffboardingPasswordCommand.cs +#[put("/accounts/update-tde-offboarding-password", data = "")] +async fn put_update_tde_offboarding_password( + data: Json, + headers: Headers, + conn: DbConn, + nt: Notify<'_>, +) -> EmptyResult { + let data = data.into_inner(); + let mut user = headers.user; + + // Adding a master password to an account that has one is changing it, which is + // `/accounts/password` and asks for the current one first. Without this an authenticated caller + // could replace the password of the account they are on, and a second offboarding call would + // overwrite the password the first one just set. + if !user.password_hash.is_empty() { + err!("Account already has a master password") + } + + // The way out of trusted devices only exists while the server still takes SSO logins but no + // longer offers trusted devices. A server that still offers them has nothing to offboard from, + // and one without SSO never had the flow at all. + if !CONFIG.sso_enabled() || CONFIG.sso_trusted_device_encryption() { + err!("Trusted device offboarding is not available on this server") + } + + // A user key that is not an encrypted string unlocks nothing, and this is the only copy the + // master password can reach. Storing it would leave an account that logs in and then cannot + // open its own vault. + if !crate::util::is_valid_enc_string(&data.key) { + err!("key is not a valid encrypted string") + } + + let password_hint = clean_password_hint(data.master_password_hint.as_ref()); + enforce_password_hint_setting(password_hint.as_ref())?; + + // The KDF is left alone: the client derived the master key from the settings the account + // already has, and sends nothing to change them by, as upstream does here. + user.set_password(&data.new_master_password_hash, Some(data.key), true, None, &conn).await?; + user.password_hint = password_hint; + + log_user_event( + EventType::UserTdeOffboardingPasswordSet as i32, + &user.uuid, + headers.device.atype, + &headers.ip.ip, + &conn, + ) + .await; + + user.save(&conn).await?; + + // Upstream logs every session out at this point. The account unlocks a different way from now + // on, so the sessions that were opened against a trusted device do not carry over. + nt.send_logout(&user, None, &conn).await; + + Ok(()) +} + #[get("/accounts/profile")] async fn profile(headers: Headers, conn: DbConn) -> Json { Json(headers.user.to_json(&conn).await) @@ -873,36 +956,38 @@ fn validate_device_keydata( updates: &[UpdateDeviceKeysData], existing_devices: &[Device], ) -> ApiResult> { - let mut listed: HashSet<&DeviceId> = HashSet::with_capacity(updates.len()); - let mut rotated = Vec::with_capacity(updates.len()); + // Everything the client sent is checked before any of it is used, so a request that is + // malformed anywhere is refused as a whole rather than answered in part. + let mut listed: HashMap<&DeviceId, &UpdateDeviceKeysData> = HashMap::with_capacity(updates.len()); for update in updates { - if !listed.insert(&update.device_id) { + if listed.insert(&update.device_id, update).is_some() { err!("A device was listed more than once in the rotation") } - let Some(device) = existing_devices.iter().find(|device| device.uuid == update.device_id) else { + if !existing_devices.iter().any(|device| device.uuid == update.device_id) { err!(format!("Device {} does not belong to this user", update.device_id)) - }; + } validate_enc_strings(&[ ("encryptedUserKey", &update.encrypted_user_key), ("encryptedPublicKey", &update.encrypted_public_key), ])?; - - // Without its own key pair a device has nothing these two keys could belong to, so it - // cannot be put back into a trust and is left to be untrusted instead. - if device.holds_private_key() { - rotated.push(( - update.device_id.clone(), - update.encrypted_user_key.clone(), - update.encrypted_public_key.clone(), - )); - } } - if existing_devices.iter().any(|device| device.is_trusted() && !listed.contains(&device.uuid)) { - err!("All existing trusted devices must be included in the rotation") + // Walked over the devices that are trusted right now rather than over what was sent, because a + // rotation may only carry an existing trust over to the new user key. Trusting a device is a + // step of its own, `PUT /devices//keys`, taken by the device itself once it holds the + // device key that these two keys are wrapped for. An entry for anything else is passed over, + // as upstream does; the clients only ever send the devices we reported as trusted. + let mut rotated = Vec::new(); + + for device in existing_devices.iter().filter(|device| device.is_trusted()) { + let Some(update) = listed.get(&device.uuid) else { + err!("All existing trusted devices must be included in the rotation") + }; + + rotated.push((device.uuid.clone(), update.encrypted_user_key.clone(), update.encrypted_public_key.clone())); } Ok(rotated) @@ -2031,13 +2116,14 @@ async fn post_admin_auth_request(data: Json, headers: Header data.validate()?; - // Only an organization the user really belongs to can answer for them. A pending invitation is - // not a membership yet, and a revoked one is not one anymore; sending either of them the email - // address, the address and the device of the asker is more than they are owed. + // Only an organization that could actually answer is asked. Approving means handing the member + // their own user key, which an administrator can only do with the key that enrolling into + // account recovery left them, so an organization without one has nothing to offer and does not + // need the email address, the address and the device of the asker. let memberships: Vec = Membership::find_by_user(&headers.user.uuid, &conn) .await .into_iter() - .filter(|membership| membership.status == MembershipStatus::Confirmed as i32) + .filter(Membership::can_use_admin_approval) .collect(); if memberships.is_empty() { err!("User does not belong to any organization that could approve a device") @@ -2054,21 +2140,30 @@ async fn post_admin_auth_request(data: Json, headers: Header let mut first_request = None; for membership in memberships { - // Asking again from the same device replaces the open request instead of adding one, so a - // client that retries does not pile up rows and does not mail the administrators twice. + // Repeating the very same request is answered with the row it already has, so a client that + // sends it twice does not pile up rows and does not mail the administrators again. + // + // What identifies the request is the key pair the client generated for it: an approval is + // the user key wrapped for that public key, and the fingerprint an administrator reads out + // is derived from it. A client that asks again with a new key pair is therefore asking + // something else, and giving it the id of the pending request would let an administrator + // who is still looking at the old one approve it for a key the requester has thrown away. + // Upstream never reuses a request at all, it creates one per attempt. + // https://github.com/bitwarden/server/blob/main/src/Core/Auth/Services/Implementations/AuthRequestService.cs let existing = AuthRequest::find_pending_admin_approval( &headers.user.uuid, &data.device_identifier, &membership.org_uuid, &conn, ) - .await; + .await + .filter(|request| request.public_key == data.public_key && request.access_code == data.access_code); let is_new = existing.is_none(); let mut auth_request = match existing { Some(mut auth_request) => { - auth_request.access_code.clone_from(&data.access_code); - auth_request.public_key.clone_from(&data.public_key); + // Only what says where the request is being made from, never the keys it is made + // with; those are what the id stands for. auth_request.device_type = headers.device.atype; auth_request.request_ip = headers.ip.ip.to_string(); auth_request.creation_date = Utc::now().naive_utc(); @@ -2117,7 +2212,7 @@ async fn notify_device_approval_requested(user: &User, org_id: &OrganizationId, let approvers = Membership::find_confirmed_by_org(org_id, conn) .await .into_iter() - .filter(Membership::has_manage_reset_password_permission); + .filter(Membership::can_manage_reset_password_now); for approver in approvers { let Some(admin) = User::find_by_uuid(&approver.user_uuid, conn).await else { @@ -2419,11 +2514,42 @@ mod tests { #[test] fn a_partially_trusted_device_does_not_have_to_be_listed() { // It cannot unlock anything as it stands, so leaving it out is not the loss of a trust. - let mut half = device("b", true); - half.encrypted_user_key = None; - let devices = [device("a", true), half]; + let devices = [device("a", true), half_trusted("b")]; let result = validate_device_keydata(&[update("a")], &devices).unwrap(); assert_eq!(rotated(&result), ["a=4.bmV3dXNlcmtleQ=="]); } + + /// A device left holding nothing but its own key pair, which is what a rotation by a client too + /// old to send `deviceKeyUnlockData` leaves behind. It does not unlock anything as it stands. + fn half_trusted(id: &str) -> Device { + let mut device = device(id, true); + device.encrypted_user_key = None; + device.encrypted_public_key = None; + assert!(!device.is_trusted(), "not trusted"); + assert!(device.holds_private_key(), "but still holds its key pair"); + device + } + + #[test] + fn a_rotation_does_not_trust_a_device_that_was_not_trusted() { + // Trusting a device is `PUT /devices//keys`, taken by the device itself once it holds + // the device key these blobs are wrapped for. A rotation only carries an existing trust + // over to the new user key, so listing an untrusted device here gains it nothing, even + // though its key pair is still around for the trust it could be given later. + let devices = [device("a", true), half_trusted("b")]; + + let result = validate_device_keydata(&[update("a"), update("b")], &devices).unwrap(); + assert_eq!(rotated(&result), ["a=4.bmV3dXNlcmtleQ=="], "`b` is passed over and cleared by the write"); + } + + #[test] + fn a_rotation_cannot_hand_a_user_their_first_trusted_device() { + // The same the other way round: with nothing to carry over, a rotation writes no trust at + // all, however much the request offers. + let devices = [half_trusted("a"), half_trusted("b")]; + + let result = validate_device_keydata(&[update("a"), update("b")], &devices).unwrap(); + assert!(result.is_empty(), "no device was trusted before the rotation, so none is after it"); + } } diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index 5d6b759f..e037181e 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -3232,16 +3232,16 @@ async fn get_organization_auth_requests( continue; } - // A request whose asker is not a confirmed member of this organization is none of its - // business, so it is quietly left out instead of being offered for approval. Same condition - // as when answering, so nothing is shown here that would be refused there. + // A request this organization could not answer anyway is none of its business, so it is + // quietly left out instead of being offered for approval. Same condition as when answering, + // so nothing is shown here that would be refused there. let (Some(member), Some(user)) = ( Membership::find_by_user_and_org(&auth_request.user_uuid, &org_id, &conn).await, User::find_by_uuid(&auth_request.user_uuid, &conn).await, ) else { continue; }; - if member.status != MembershipStatus::Confirmed as i32 { + if !member.can_use_admin_approval() { continue; } @@ -3423,10 +3423,11 @@ async fn answer_organization_auth_request( unanswerable!("AuthRequest doesn't exist", "Request has expired"); } - // Answering means acting for a member of this organization, so it has to be one: an invitation - // that was never accepted is not a membership yet, and a revoked one is not one anymore. + // Answering means acting for a member of this organization with the key their enrollment into + // account recovery left behind: an invitation that was never accepted is not a membership yet, + // a revoked one is not one anymore, and without that key there is nothing to answer with. let member = match Membership::find_by_user_and_org(&auth_request.user_uuid, org_id, conn).await { - Some(member) if member.status == MembershipStatus::Confirmed as i32 => member, + Some(member) if member.can_use_admin_approval() => member, _ => unanswerable!("AuthRequest doesn't exist", "The requesting user is not a member of this organization"), }; diff --git a/src/api/identity.rs b/src/api/identity.rs index a56cc65c..84913283 100644 --- a/src/api/identity.rs +++ b/src/api/identity.rs @@ -31,8 +31,8 @@ use crate::{ DbConn, models::{ AuthRequest, AuthRequestId, Device, DeviceId, DeviceType, EventType, Invitation, Membership, - MembershipStatus, OIDCCodeResponseError, OrgPolicy, OrgPolicyType, Organization, OrganizationApiKey, - OrganizationId, SendId, SsoAuth, SsoUser, TwoFactor, TwoFactorIncomplete, TwoFactorType, User, UserId, + OIDCCodeResponseError, OrgPolicy, OrgPolicyType, Organization, OrganizationApiKey, OrganizationId, SendId, + SsoAuth, SsoUser, TwoFactor, TwoFactorIncomplete, TwoFactorType, User, UserId, }, }, error::MapResult, @@ -507,6 +507,15 @@ async fn account_creation_can_succeed(user: &User, conn: &DbConn) -> bool { return false; }; + // That lookup only rules out the `Revoked` status itself, which revoking never actually writes: + // it shifts the status out of the active range instead, so a revoked membership comes back from + // it like any other. The enrolment endpoint runs behind `OrgMemberHeaders` and turns exactly + // those away, so offering the flow on the strength of one would walk the client into the half + // built account this whole function exists to avoid. + if !membership.is_active() { + return false; + } + // What `check_reset_password_applicable` demands of that organization. if !CONFIG.mail_enabled() { return false; @@ -602,13 +611,10 @@ async fn trusted_device_option(user: &User, device: &Device, conn: &DbConn) -> O let memberships = Membership::find_by_user(&user.uuid, conn).await; // An admin can only take over the approval once the member handed them a key to work with, - // which is what enrolling into account recovery does. Only a confirmed membership counts, the - // same condition the request itself is created and answered under, so this does not announce a - // way out that would be refused the moment it is taken. - ways_in.has_admin_approval = memberships.iter().any(|member| { - member.status == MembershipStatus::Confirmed as i32 - && member.reset_password_key.as_ref().is_some_and(|key| !key.is_empty()) - }); + // which is what enrolling into account recovery does. The same condition the request itself is + // created and answered under, so this does not announce a way out that would be refused the + // moment it is taken. + ways_in.has_admin_approval = memberships.iter().any(Membership::can_use_admin_approval); // Only worth asking when nothing cheaper already lets the client in. if !(ways_in.device_is_trusted || ways_in.has_admin_approval || ways_in.has_master_password) { @@ -628,7 +634,13 @@ async fn trusted_device_option(user: &User, device: &Device, conn: &DbConn) -> O // could approve others, but has no master password themselves, into setting one. Upstream reads // a `ManageResetPassword` permission here, which in Vaultwarden's role model only the // administrators of an organization have. - let has_manage_reset_password_permission = memberships.iter().any(Membership::has_manage_reset_password_permission); + // + // Every active membership counts, not only the confirmed one that may act on the permission + // today: an administrator provisioned into the organization by this very login holds the role + // before anybody has confirmed them, and this is the login that has to tell them to set a + // master password. See `has_manage_reset_password_role_for_tde`. + let has_manage_reset_password_permission = + memberships.iter().any(Membership::has_manage_reset_password_role_for_tde); Some(json!({ "HasAdminApproval": ways_in.has_admin_approval, @@ -1495,6 +1507,7 @@ async fn authorize(data: AuthorizeData, cookies: &CookieJar<'_>, secure: Secure, #[cfg(test)] mod tests { use super::*; + use crate::db::models::MembershipStatus; /// A `TrustedDeviceWaysIn` plus the server setting, so the cases below read as what they are. #[expect(clippy::struct_excessive_bools, reason = "Mirrors the struct under test")] @@ -1624,4 +1637,60 @@ mod tests { }; assert_eq!(account.offer(), None); } + + /// What `trusted_device_option` reads off the memberships of the user logging in. + fn has_admin_approval(memberships: &[Membership]) -> bool { + memberships.iter().any(Membership::can_use_admin_approval) + } + + fn membership(org: &str, status: MembershipStatus, enrolled: bool) -> Membership { + let mut membership = Membership::new(String::from("user").into(), org.to_owned().into(), None); + membership.status = status as i32; + membership.reset_password_key = enrolled.then(|| String::from("2.aXY=|Y2lwaGVy|bWFj")); + membership + } + + #[test] + fn enrolling_into_trusted_devices_leaves_an_administrator_to_ask() { + // Invited into an organization that unlocks with trusted devices, before enrolling: nobody + // holds a key to approve with yet. + let mut memberships = [membership("org", MembershipStatus::Invited, false)]; + assert!(!has_admin_approval(&memberships)); + + // Enrolling is what `put_reset_password_enrollment` does for an account without a master + // password: it writes the key and accepts the invitation in the same step. Confirming the + // member is an administrator's own, later decision, and until they get round to it the + // member is stuck here. + memberships[0].status = MembershipStatus::Accepted as i32; + memberships[0].reset_password_key = Some(String::from("2.aXY=|Y2lwaGVy|bWFj")); + + assert!(has_admin_approval(&memberships), "the enrolment is what an administrator answers with"); + + // Losing the trusted device at that point is the case this covers: no master password, no + // device that unlocks, and an administrator to ask is the only way back in. + let account = Account { + has_admin_approval: has_admin_approval(&memberships), + ..Account::new() + }; + assert_eq!(account.offer(), Some(false), "the flow leads somewhere, so it is offered"); + } + + #[test] + fn one_organization_that_could_approve_is_enough() { + // A member of several organizations only needs one of them to hold a key for them. + let memberships = [ + membership("invited", MembershipStatus::Invited, true), + membership("not-enrolled", MembershipStatus::Confirmed, false), + membership("enrolled", MembershipStatus::Accepted, true), + ]; + assert!(has_admin_approval(&memberships)); + + // Take that one away and there is nobody left to ask, however many organizations remain. + let memberships = [ + membership("invited", MembershipStatus::Invited, true), + membership("not-enrolled", MembershipStatus::Confirmed, false), + membership("revoked", MembershipStatus::Revoked, true), + ]; + assert!(!has_admin_approval(&memberships)); + } } diff --git a/src/auth.rs b/src/auth.rs index 6431178f..39a030bd 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -847,7 +847,7 @@ impl<'r> FromRequest<'r> for AdminHeaders { /// its device approvals comes down to. /// /// Upstream guards those endpoints on a permission, `ManageResetPassword`, rather than on a role, -/// so this asks `Membership::has_manage_reset_password_permission` instead of naming roles here. +/// so this asks `Membership::can_manage_reset_password_now` instead of naming roles here. /// Today that permission belongs to the administrators of an organization and to nobody else, which /// makes this the same set of callers as `AdminHeaders`; keeping it apart is what lets a custom role /// hold the permission later without every endpoint having to be revisited. @@ -865,7 +865,7 @@ impl<'r> FromRequest<'r> for ManageResetPasswordHeaders { async fn from_request(request: &'r Request<'_>) -> Outcome { let headers = try_outcome!(OrgHeaders::from_request(request).await); - if headers.membership.has_manage_reset_password_permission() { + if headers.membership.can_manage_reset_password_now() { Outcome::Success(Self { device: headers.device, user: headers.user, diff --git a/src/db/models/event.rs b/src/db/models/event.rs index 86cbf5d0..58befaf4 100644 --- a/src/db/models/event.rs +++ b/src/db/models/event.rs @@ -58,7 +58,7 @@ pub enum EventType { // UserUpdatedTempPassword = 1008, // Not supported // UserMigratedKeyToKeyConnector = 1009, // Not supported UserRequestedDeviceApproval = 1010, - // UserTdeOffboardingPasswordSet = 1011, // Not supported + UserTdeOffboardingPasswordSet = 1011, // Cipher CipherCreated = 1100, diff --git a/src/db/models/organization.rs b/src/db/models/organization.rs index f9628a62..4980362b 100644 --- a/src/db/models/organization.rs +++ b/src/db/models/organization.rs @@ -277,8 +277,18 @@ impl Membership { } } - /// Whether this membership may act on the account recovery of the organization's members: - /// reset their master password, and answer the device approvals they ask their organization for. + /// Whether this membership is in one of the active states rather than a revoked one. + /// + /// Revoking does not write `Revoked`, it shifts the status the membership is to be restored to + /// out of the active range, so a revoked row reads `-128`, `-127` or `-126` and never `-1`. + /// `MembershipStatus::from_i32` only knows the three active values, which is exactly how + /// `OrgHeaders` turns a revoked member away, so it is what decides it here too. Comparing + /// against `Revoked` instead would let every one of those stored values through. + pub fn is_active(&self) -> bool { + MembershipStatus::from_i32(self.status).is_some() + } + + /// The role side of account recovery, without asking what the membership's standing is. /// /// Upstream is a permission of its own, `ManageResetPassword`, which an administrator has by /// virtue of the role and a custom role can be granted separately. Vaultwarden folds the custom @@ -286,9 +296,56 @@ impl Membership { /// are left holding it. Asking here rather than comparing roles at each call site keeps that one /// decision in one place for when custom roles arrive. /// https://github.com/bitwarden/server/blob/main/src/Core/Context/CurrentContext.cs - pub fn has_manage_reset_password_permission(&self) -> bool { - self.status == MembershipStatus::Confirmed as i32 - && MembershipType::from_i32(self.atype).is_some_and(|atype| atype >= MembershipType::Admin) + fn has_manage_reset_password_role(&self) -> bool { + MembershipType::from_i32(self.atype).is_some_and(|atype| atype >= MembershipType::Admin) + } + + /// Whether this membership may act on the account recovery of the organization's members right + /// now: reset their master password, and answer the device approvals they ask their + /// organization for. + /// + /// This is the authorization question, so it asks for a membership that is fully established. + /// An invitation that was never accepted and one that is still waiting to be confirmed are not + /// yet somebody the organization has put in charge of its members' keys. + pub fn can_manage_reset_password_now(&self) -> bool { + self.status == MembershipStatus::Confirmed as i32 && self.has_manage_reset_password_role() + } + + /// Whether a login should tell the client that this member is on the answering side of account + /// recovery, which is what makes it walk a member who has no master password into setting one. + /// + /// A weaker question than `can_manage_reset_password_now`, and deliberately so: it decides what + /// the account is told about itself, not what it may do. Upstream answers it for every active + /// membership, invited and accepted included, because a member who was just provisioned into + /// the organization by their first SSO login holds the role before anyone confirms them, and + /// waiting until then would let an administrator through the trusted device flow without ever + /// being asked for the master password their own role requires of them. + /// + /// A revoked membership is not active and never counts, here or anywhere else. + /// https://github.com/bitwarden/server/blob/main/src/Identity/IdentityServer/UserDecryptionOptionsBuilder.cs + pub fn has_manage_reset_password_role_for_tde(&self) -> bool { + self.is_active() && self.has_manage_reset_password_role() + } + + /// Whether the administrators of this organization can let a new device of this member in. + /// + /// Approving means handing the member their own user key, wrapped for the asking device. The + /// only copy of it the organization has is the one enrolling into account recovery left behind, + /// so without that key there is nothing to approve with, whatever the member's standing is. + /// + /// Enrolling is also what turns an invitation into a membership in the trusted device flow, so + /// the state this has to cover is `Accepted` and not just `Confirmed`: a member who set up + /// trusted devices and then lost the device before an administrator got round to confirming + /// them would otherwise have no way back into their own vault. Upstream asks for the enrollment + /// alone and lets any membership row through; the two ends of the range are kept out here + /// because an invitation is not a membership yet and a revoked one is not one anymore, so + /// neither should have its device let in. + /// https://github.com/bitwarden/server/blob/main/src/Identity/IdentityServer/UserDecryptionOptionsBuilder.cs + pub fn can_use_admin_approval(&self) -> bool { + matches!( + MembershipStatus::from_i32(self.status), + Some(MembershipStatus::Accepted | MembershipStatus::Confirmed) + ) && self.reset_password_key.as_ref().is_some_and(|key| !key.is_empty()) } pub fn restore(&mut self) -> bool { @@ -1318,13 +1375,149 @@ mod tests { let status = status as i32; membership.status = status; assert!( - !membership.has_manage_reset_password_permission(), + !membership.can_manage_reset_password_now(), "a membership that is not confirmed manages nothing, status {status}" ); } membership.status = MembershipStatus::Confirmed as i32; - assert_eq!(membership.has_manage_reset_password_permission(), expected, "type {}", atype as i32); + assert_eq!(membership.can_manage_reset_password_now(), expected, "type {}", atype as i32); + } + } + + #[test] + fn the_trusted_device_role_signal_covers_a_member_nobody_confirmed_yet() { + let mut membership = Membership::new(String::from("user").into(), String::from("org").into(), None); + + for (atype, holds_role) in [ + (MembershipType::Owner, true), + (MembershipType::Admin, true), + (MembershipType::Manager, false), + (MembershipType::User, false), + ] { + membership.atype = atype as i32; + + // Every active membership answers the same, so an administrator who was provisioned by + // the login that is asking is told to set a master password straight away. + for status in [MembershipStatus::Invited, MembershipStatus::Accepted, MembershipStatus::Confirmed] { + let status = status as i32; + membership.status = status; + assert_eq!( + membership.has_manage_reset_password_role_for_tde(), + holds_role, + "type {}, status {status}", + atype as i32 + ); + } + + // Revoked never counts, whatever the role says. + membership.status = MembershipStatus::Revoked as i32; + assert!(!membership.has_manage_reset_password_role_for_tde(), "revoked, type {}", atype as i32); + + // Nor do the internal statuses a revoked membership is actually stored as, which keep + // the role it is to be restored to. + for was in [MembershipStatus::Invited, MembershipStatus::Accepted, MembershipStatus::Confirmed] { + let was = was as i32; + membership.status = was; + assert!(membership.revoke(), "revoking a {was} membership"); + assert!( + !membership.has_manage_reset_password_role_for_tde(), + "revoked from {was}, stored as {}", + membership.status + ); + } + } + } + + #[test] + fn a_revoked_membership_is_not_active_whatever_it_was_revoked_from() { + let mut membership = Membership::new(String::from("user").into(), String::from("org").into(), None); + + for status in [MembershipStatus::Invited, MembershipStatus::Accepted, MembershipStatus::Confirmed] { + let status = status as i32; + membership.status = status; + assert!(membership.is_active(), "status {status}"); + + // Revoking keeps the status it is to be restored to and shifts it out of the active + // range, so what is stored is never `Revoked` itself. Comparing against that value is + // what would let these through. + assert!(membership.revoke(), "revoking status {status}"); + assert_ne!( + membership.status, + MembershipStatus::Revoked as i32, + "revoked from {status} is not stored as -1" + ); + assert!(!membership.is_active(), "revoked from {status}, stored as {}", membership.status); + + assert!(membership.restore(), "restoring status {status}"); + assert_eq!(membership.status, status, "restored to what it was"); + assert!(membership.is_active()); + } + + // The value the responses show for a revoked membership does not count either. + membership.status = MembershipStatus::Revoked as i32; + assert!(!membership.is_active()); + } + + #[test] + fn only_a_confirmed_membership_may_act_on_account_recovery() { + // The two questions are deliberately not the same one: being told to set a master password + // is not being allowed to reset somebody else's. + let mut membership = Membership::new(String::from("user").into(), String::from("org").into(), None); + membership.atype = MembershipType::Admin as i32; + + for status in [MembershipStatus::Invited, MembershipStatus::Accepted] { + let status = status as i32; + membership.status = status; + assert!(membership.has_manage_reset_password_role_for_tde(), "the login signal covers status {status}"); + assert!(!membership.can_manage_reset_password_now(), "but the endpoints do not, status {status}"); + } + + membership.status = MembershipStatus::Confirmed as i32; + assert!(membership.has_manage_reset_password_role_for_tde()); + assert!(membership.can_manage_reset_password_now()); + } + + #[test] + fn admin_approval_needs_an_accepted_membership_and_an_enrollment() { + let mut membership = Membership::new(String::from("user").into(), String::from("org").into(), None); + + for (status, enrolled, expected, why) in [ + // The invitation was never taken up, so there is no membership to act for yet. + (MembershipStatus::Invited, true, false, "an invitation is not a membership"), + // Where the trusted device enrollment leaves a member until an administrator confirms + // them. Losing the device in that window must not cost them their vault. + (MembershipStatus::Accepted, true, true, "an accepted member enrolled in account recovery"), + (MembershipStatus::Confirmed, true, true, "a confirmed member enrolled in account recovery"), + // Belonging to the organization is not the point, holding the key it would answer with + // is; without an enrollment there is nothing an administrator could hand back. + (MembershipStatus::Accepted, false, false, "accepted, but not enrolled"), + (MembershipStatus::Confirmed, false, false, "confirmed, but not enrolled"), + // Revoking takes the access away but leaves the key behind, which must not keep letting + // new devices in. + (MembershipStatus::Revoked, true, false, "a revoked membership is not one anymore"), + ] { + membership.status = status as i32; + membership.reset_password_key = enrolled.then(|| String::from("2.aXY=|Y2lwaGVy|bWFj")); + + assert_eq!(membership.can_use_admin_approval(), expected, "{why}"); + } + + // Nothing to wrap the user key with, so the same as never having enrolled. + membership.status = MembershipStatus::Confirmed as i32; + membership.reset_password_key = Some(String::new()); + assert!(!membership.can_use_admin_approval(), "an empty key is not an enrollment"); + + // Revoking a member who was enrolled keeps their key, and stores a status that is not + // `Revoked` itself. None of those may keep letting new devices in. + membership.reset_password_key = Some(String::from("2.aXY=|Y2lwaGVy|bWFj")); + for was in [MembershipStatus::Accepted, MembershipStatus::Confirmed] { + let was = was as i32; + membership.status = was; + assert!(membership.can_use_admin_approval(), "enrolled and active, status {was}"); + + assert!(membership.revoke(), "revoking status {was}"); + assert!(!membership.can_use_admin_approval(), "revoked from {was}, stored as {}", membership.status); } } } From 1e743fcec3679c7ef69c30e3f79cd93e8df0c36b Mon Sep 17 00:00:00 2001 From: tom27052006 <83423411+tom27052006@users.noreply.github.com> Date: Sat, 29 Aug 2026 19:20:04 +0200 Subject: [PATCH 10/10] Ignore the COSE acronym in the spell checker --- .typos.toml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.typos.toml b/.typos.toml index 87c0c4a6..741a31ae 100644 --- a/.typos.toml +++ b/.typos.toml @@ -14,9 +14,9 @@ extend-ignore-re = [ # In SMTP it's called HELO, so ignore it "(?i)helo_name", "Server name sent during.+HELO", - # COSE Is short for CBOR Object Signing and Encryption, ignore these specific items - "COSEKey", - "COSEAlgorithm", + # COSE Is short for CBOR Object Signing and Encryption, which covers the type names taken from + # it as well as the acronym on its own + "COSE", # Ignore this specific string as it's valid "Ensure they are valid OTPs", # This word is misspelled upstream