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 1/7] 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 2/7] 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 3/7] 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 4/7] 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 5/7] 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 6/7] 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 7/7] 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()