Skip to content

[PM-22861] Account security state #322

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weโ€™ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 68 commits into from
Jul 15, 2025
Merged
Show file tree
Hide file tree
Changes from 46 commits
Commits
Show all changes
68 commits
Select commit Hold shift + click to select a range
7fa725f
Implement security context
quexten Jul 3, 2025
0387b44
Cargo fmt
quexten Jul 3, 2025
6c1a57d
Apply clippy fixes
quexten Jul 3, 2025
f4bd1e2
Apply cargo fmt
quexten Jul 3, 2025
ae4590e
Fix build
quexten Jul 3, 2025
c7dc40a
Cleanup
quexten Jul 4, 2025
eaf788e
Update crates/bitwarden-core/src/key_management/crypto.rs
quexten Jul 4, 2025
c7854b4
Merge branch 'km/account-security-version' of github.com:bitwarden/sdโ€ฆ
quexten Jul 4, 2025
2c65c3d
Split init to two functions
quexten Jul 4, 2025
db94fd3
Replace `EncryptionSettingsError::Crypto` with into
quexten Jul 4, 2025
77a2ebe
Fix imports
quexten Jul 4, 2025
14f5d3f
Cargo fmt
quexten Jul 4, 2025
4fea3dc
Cleanup
quexten Jul 4, 2025
46d2202
Cleanup init with enum
quexten Jul 4, 2025
b20e5a4
Cleanup
quexten Jul 4, 2025
566dc47
Fix clippy warnings
quexten Jul 4, 2025
678a9e3
Move security state to core
quexten Jul 4, 2025
b891ec4
Fix version range
quexten Jul 4, 2025
3dd6366
Fix clippy errors
quexten Jul 4, 2025
24ec20d
Fix comment
quexten Jul 4, 2025
6f17cd4
Move serde bytes to workspace
quexten Jul 8, 2025
9922265
Make signing key and security state non-optional
quexten Jul 8, 2025
5f98cab
Remove unused import
quexten Jul 8, 2025
429066a
Add comment
quexten Jul 8, 2025
162e218
Remove unused import
quexten Jul 8, 2025
35677ac
Merge branch 'main' into km/account-security-version
quexten Jul 9, 2025
381b547
Tmp
quexten Jul 9, 2025
d89a17d
Simplify rotation usage, and store security state
quexten Jul 9, 2025
80fe0b7
Fix build
quexten Jul 10, 2025
3885e36
Fix sm build
quexten Jul 10, 2025
e6816f2
Apply clippy fixes
quexten Jul 10, 2025
96548c2
Apply clippy fixes
quexten Jul 10, 2025
620d3c2
Fix test
quexten Jul 10, 2025
959b2e7
Fix tests
quexten Jul 10, 2025
bf269f6
Remove unused import
quexten Jul 10, 2025
5902ea2
Rename to "UserCryptoV2KeysResponse"
quexten Jul 10, 2025
cd8077c
Update crates/bitwarden-core/src/key_management/mod.rs
quexten Jul 11, 2025
e063963
Fix renames
quexten Jul 11, 2025
a0ee4d7
Add error for base64 deserialization
quexten Jul 11, 2025
d10af3d
Clean up key rotation
quexten Jul 11, 2025
5d6ae55
Cleanup
quexten Jul 11, 2025
16c6248
Cleanup get_v2_rotated_account_keys
quexten Jul 11, 2025
d7dea45
Remove arc
quexten Jul 11, 2025
8a00fff
Remove remaining arc references
quexten Jul 11, 2025
4f2b05a
Fix build
quexten Jul 11, 2025
080c6ea
Merge branch 'main' into km/account-security-version
quexten Jul 11, 2025
f76daa8
Re-sort impl after definition
quexten Jul 11, 2025
9e44be8
Update comment
quexten Jul 11, 2025
2c64b9c
Remove json schema
quexten Jul 11, 2025
6e85d6b
Merge branch 'km/account-security-version' of github.com:bitwarden/sdโ€ฆ
quexten Jul 11, 2025
35821d9
Clarify comment
quexten Jul 11, 2025
d01b953
Add test for v1 user trying to rotate using v2 rotation method
quexten Jul 11, 2025
e3dea12
Cargo fmt
quexten Jul 14, 2025
da0a0d8
Extract individual user crypto state values into UserKeyState struct
quexten Jul 14, 2025
4bc1214
Fix build
quexten Jul 14, 2025
7c4f562
Swap to uuid
quexten Jul 14, 2025
ee78cb7
Move impl to struct
quexten Jul 14, 2025
f4007c1
Clean up imports
quexten Jul 14, 2025
1e25d0e
Clean up imports
quexten Jul 14, 2025
17758a4
Fix build
quexten Jul 14, 2025
a85f84f
Add test vectors and tests
quexten Jul 14, 2025
932f9b8
Add comment for signature keys on registration
quexten Jul 14, 2025
2511017
Lift up use
quexten Jul 14, 2025
8ce7e26
Cargo fmt
quexten Jul 14, 2025
95827df
Move stateful crypto error to core crate
quexten Jul 14, 2025
6fa7eff
Fix build on non-internal feature flag
quexten Jul 14, 2025
dfaaca9
Filter import to internal feature only
quexten Jul 14, 2025
736f744
Merge branch 'main' into km/account-security-version
quexten Jul 14, 2025
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ reqwest = { version = ">=0.12.5, <0.13", features = [
], default-features = false }
schemars = { version = ">=0.8.9, <0.9", features = ["uuid1", "chrono"] }
serde = { version = ">=1.0, <2.0", features = ["derive"] }
serde_bytes = { version = ">=0.11.17, <0.12.0" }
serde_json = ">=1.0.96, <2.0"
serde_qs = ">=0.12.0, <0.16"
serde_repr = ">=0.1.12, <0.2"
Expand Down
1 change: 1 addition & 0 deletions crates/bitwarden-core/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ rand = ">=0.8.5, <0.9"
reqwest = { workspace = true }
schemars = { workspace = true }
serde = { workspace = true }
serde_bytes = { workspace = true }
serde_json = { workspace = true }
serde_qs = { workspace = true }
serde_repr = { workspace = true }
Expand Down
11 changes: 9 additions & 2 deletions crates/bitwarden-core/src/auth/auth_request.rs
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ mod tests {
let private_key ="2.yN7l00BOlUE0Sb0M//Q53w==|EwKG/BduQRQ33Izqc/ogoBROIoI5dmgrxSo82sgzgAMIBt3A2FZ9vPRMY+GWT85JiqytDitGR3TqwnFUBhKUpRRAq4x7rA6A1arHrFp5Tp1p21O3SfjtvB3quiOKbqWk6ZaU1Np9HwqwAecddFcB0YyBEiRX3VwF2pgpAdiPbSMuvo2qIgyob0CUoC/h4Bz1be7Qa7B0Xw9/fMKkB1LpOm925lzqosyMQM62YpMGkjMsbZz0uPopu32fxzDWSPr+kekNNyLt9InGhTpxLmq1go/pXR2uw5dfpXc5yuta7DB0EGBwnQ8Vl5HPdDooqOTD9I1jE0mRyuBpWTTI3FRnu3JUh3rIyGBJhUmHqGZvw2CKdqHCIrQeQkkEYqOeJRJVdBjhv5KGJifqT3BFRwX/YFJIChAQpebNQKXe/0kPivWokHWwXlDB7S7mBZzhaAPidZvnuIhalE2qmTypDwHy22FyqV58T8MGGMchcASDi/QXI6kcdpJzPXSeU9o+NC68QDlOIrMVxKFeE7w7PvVmAaxEo0YwmuAzzKy9QpdlK0aab/xEi8V4iXj4hGepqAvHkXIQd+r3FNeiLfllkb61p6WTjr5urcmDQMR94/wYoilpG5OlybHdbhsYHvIzYoLrC7fzl630gcO6t4nM24vdB6Ymg9BVpEgKRAxSbE62Tqacxqnz9AcmgItb48NiR/He3n3ydGjPYuKk/ihZMgEwAEZvSlNxYONSbYrIGDtOY+8Nbt6KiH3l06wjZW8tcmFeVlWv+tWotnTY9IqlAfvNVTjtsobqtQnvsiDjdEVtNy/s2ci5TH+NdZluca2OVEr91Wayxh70kpM6ib4UGbfdmGgCo74gtKvKSJU0rTHakQ5L9JlaSDD5FamBRyI0qfL43Ad9qOUZ8DaffDCyuaVyuqk7cz9HwmEmvWU3VQ+5t06n/5kRDXttcw8w+3qClEEdGo1KeENcnXCB32dQe3tDTFpuAIMLqwXs6FhpawfZ5kPYvLPczGWaqftIs/RXJ/EltGc0ugw2dmTLpoQhCqrcKEBDoYVk0LDZKsnzitOGdi9mOWse7Se8798ib1UsHFUjGzISEt6upestxOeupSTOh0v4+AjXbDzRUyogHww3V+Bqg71bkcMxtB+WM+pn1XNbVTyl9NR040nhP7KEf6e9ruXAtmrBC2ah5cFEpLIot77VFZ9ilLuitSz+7T8n1yAh1IEG6xxXxninAZIzi2qGbH69O5RSpOJuJTv17zTLJQIIc781JwQ2TTwTGnx5wZLbffhCasowJKd2EVcyMJyhz6ru0PvXWJ4hUdkARJs3Xu8dus9a86N8Xk6aAPzBDqzYb1vyFIfBxP0oO8xFHgd30Cgmz8UrSE3qeWRrF8ftrI6xQnFjHBGWD/JWSvd6YMcQED0aVuQkuNW9ST/DzQThPzRfPUoiL10yAmV7Ytu4fR3x2sF0Yfi87YhHFuCMpV/DsqxmUizyiJuD938eRcH8hzR/VO53Qo3UIsqOLcyXtTv6THjSlTopQ+JOLOnHm1w8dzYbLN44OG44rRsbihMUQp+wUZ6bsI8rrOnm9WErzkbQFbrfAINdoCiNa6cimYIjvvnMTaFWNymqY1vZxGztQiMiHiHYwTfwHTXrb9j0uPM=|09J28iXv9oWzYtzK2LBT6Yht4IT4MijEkk0fwFdrVQ4=".parse().unwrap();
client
.internal
.initialize_user_crypto_master_key(master_key, user_key, private_key, None)
.initialize_user_crypto_master_key(master_key, user_key, private_key, None, None)
.unwrap();

let public_key = "MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAvyLRDUwXB4BfQ507D4meFPmwn5zwy3IqTPJO4plrrhnclWahXa240BzyFW9gHgYu+Jrgms5xBfRTBMcEsqqNm7+JpB6C1B6yvnik0DpJgWQw1rwvy4SUYidpR/AWbQi47n/hvnmzI/sQxGddVfvWu1iTKOlf5blbKYAXnUE5DZBGnrWfacNXwRRdtP06tFB0LwDgw+91CeLSJ9py6dm1qX5JIxoO8StJOQl65goLCdrTWlox+0Jh4xFUfCkb+s3px+OhSCzJbvG/hlrSRcUz5GnwlCEyF3v5lfUtV96MJD+78d8pmH6CfFAp2wxKRAbGdk+JccJYO6y6oIXd3Fm7twIDAQAB";
Expand Down Expand Up @@ -232,7 +232,13 @@ mod tests {

existing_device
.internal
.initialize_user_crypto_master_key(master_key, user_key, private_key.clone(), None)
.initialize_user_crypto_master_key(
master_key,
user_key,
private_key.clone(),
None,
None,
)
.unwrap();

// Initialize a new device which will request to be logged in
Expand All @@ -251,6 +257,7 @@ mod tests {
email: email.to_owned(),
private_key,
signing_key: None,
security_state: None,
method: InitUserCryptoMethod::AuthRequest {
request_private_key: auth_req.private_key,
method: AuthRequestMethod::UserKey {
Expand Down
1 change: 1 addition & 0 deletions crates/bitwarden-core/src/auth/login/api_key.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
user_key,
private_key,
None,
None,

Check warning on line 59 in crates/bitwarden-core/src/auth/login/api_key.rs

View check run for this annotation

Codecov / codecov/patch

crates/bitwarden-core/src/auth/login/api_key.rs#L59

Added line #L59 was not covered by tests
)?;
}

Expand Down
1 change: 1 addition & 0 deletions crates/bitwarden-core/src/auth/login/auth_request.rs
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@
email: auth_req.email,
private_key: require!(r.private_key).parse()?,
signing_key: None,
security_state: None,

Check warning on line 124 in crates/bitwarden-core/src/auth/login/auth_request.rs

View check run for this annotation

Codecov / codecov/patch

crates/bitwarden-core/src/auth/login/auth_request.rs#L124

Added line #L124 was not covered by tests
method: InitUserCryptoMethod::AuthRequest {
request_private_key: auth_req.private_key,
method,
Expand Down
1 change: 1 addition & 0 deletions crates/bitwarden-core/src/auth/login/password.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
user_key,
private_key,
None,
None,

Check warning on line 58 in crates/bitwarden-core/src/auth/login/password.rs

View check run for this annotation

Codecov / codecov/patch

crates/bitwarden-core/src/auth/login/password.rs#L58

Added line #L58 was not covered by tests
)?;
}

Expand Down
2 changes: 2 additions & 0 deletions crates/bitwarden-core/src/auth/password/validate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ mod tests {
user_key.parse().unwrap(),
private_key,
None,
None,
)
.unwrap();

Expand Down Expand Up @@ -193,6 +194,7 @@ mod tests {
user_key.parse().unwrap(),
private_key,
None,
None,
)
.unwrap();

Expand Down
1 change: 1 addition & 0 deletions crates/bitwarden-core/src/auth/pin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ mod tests {
user_key.parse().unwrap(),
private_key,
None,
None,
)
.unwrap();

Expand Down
1 change: 1 addition & 0 deletions crates/bitwarden-core/src/auth/tde.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
// Note: Signing keys are not supported on registration yet. This needs to be changed as
// soon as registration is supported.
None,
None,

Check warning on line 48 in crates/bitwarden-core/src/auth/tde.rs

View check run for this annotation

Codecov / codecov/patch

crates/bitwarden-core/src/auth/tde.rs#L48

Added line #L48 was not covered by tests
)?;

Ok(RegisterTdeKeyResponse {
Expand Down
2 changes: 2 additions & 0 deletions crates/bitwarden-core/src/client/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,8 @@ impl Client {
external_client,
key_store: KeyStore::default(),
#[cfg(feature = "internal")]
security_state: RwLock::new(None),
#[cfg(feature = "internal")]
repository_map: StateRegistry::new(),
}),
}
Expand Down
130 changes: 109 additions & 21 deletions crates/bitwarden-core/src/client/encryption_settings.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,22 @@
#[cfg(feature = "internal")]
use bitwarden_crypto::{EncString, UnsignedSharedKey};
use std::sync::RwLock;

#[cfg(feature = "internal")]
use bitwarden_crypto::{
Aes256CbcHmacKey, AsymmetricCryptoKey, CoseKeyBytes, CoseSerializable, EncString,
KeyDecryptable, Pkcs8PrivateKeyBytes, SigningKey, UnsignedSharedKey, XChaCha20Poly1305Key,
};
#[cfg(any(feature = "internal", feature = "secrets"))]
use bitwarden_crypto::{KeyStore, SymmetricCryptoKey};
use bitwarden_error::bitwarden_error;
#[cfg(feature = "internal")]
use log::warn;
use thiserror::Error;
#[cfg(any(feature = "internal", feature = "secrets"))]
use uuid::Uuid;

#[cfg(feature = "internal")]
use crate::key_management::{AsymmetricKeyId, SecurityState, SignedSecurityState, SigningKeyId};
#[cfg(any(feature = "internal", feature = "secrets"))]
use crate::key_management::{KeyIds, SymmetricKeyId};
use crate::{error::UserIdAlreadySetError, MissingPrivateKeyError, VaultLockedError};
Expand All @@ -30,13 +40,31 @@ pub enum EncryptionSettingsError {
#[error("Invalid signing key")]
InvalidSigningKey,

#[error("Invalid security state")]
InvalidSecurityState,

#[error(transparent)]
MissingPrivateKey(#[from] MissingPrivateKeyError),

#[error(transparent)]
UserIdAlreadySetError(#[from] UserIdAlreadySetError),
}

#[allow(clippy::large_enum_variant)]
#[cfg(feature = "internal")]
pub(crate) enum AccountEncryptionKeys {
V1 {
user_key: Aes256CbcHmacKey,
private_key: EncString,
},
V2 {
user_key: XChaCha20Poly1305Key,
private_key: EncString,
signing_key: EncString,
security_state: SignedSecurityState,
},
}

#[allow(missing_docs)]
pub struct EncryptionSettings {}

Expand All @@ -47,21 +75,55 @@ impl EncryptionSettings {
/// discouraged
#[cfg(feature = "internal")]
pub(crate) fn new_decrypted_key(
user_key: SymmetricCryptoKey,
private_key: EncString,
signing_key: Option<EncString>,
encryption_keys: AccountEncryptionKeys,
store: &KeyStore<KeyIds>,
security_state_rwlock: &RwLock<Option<SecurityState>>,
) -> Result<(), EncryptionSettingsError> {
use bitwarden_crypto::{AsymmetricCryptoKey, CoseSerializable, KeyDecryptable, SigningKey};
use log::warn;
// This is an all-or-nothing check. The server cannot pretend a signing key or security
// state to be missing, because they are *always* present when the user key is an
// XChaCha20Poly1305Key. Thus, the server or network cannot lie about the presence of these,
// because otherwise the entire user account will fail to decrypt.
match encryption_keys {
AccountEncryptionKeys::V1 {
user_key,
private_key,
} => {
Self::init_v1(user_key, private_key, store)?;
}
AccountEncryptionKeys::V2 {
user_key,
private_key,
signing_key,
security_state,
} => {
Self::init_v2(
user_key,
private_key,
signing_key,
security_state,
store,
security_state_rwlock,
)?;
}
}

Ok(())
}

use crate::key_management::{AsymmetricKeyId, SigningKeyId, SymmetricKeyId};
#[cfg(feature = "internal")]
fn init_v1(
user_key: Aes256CbcHmacKey,
private_key: EncString,
store: &KeyStore<KeyIds>,
) -> Result<(), EncryptionSettingsError> {
let user_key = SymmetricCryptoKey::Aes256CbcHmacKey(user_key);

let private_key = {
let dec: Vec<u8> = private_key.decrypt_with_key(&user_key)?;
// FIXME: [PM-11690] - Temporarily ignore invalid private keys until we have a recovery
// process in place.
AsymmetricCryptoKey::from_der(&dec.into())

// FIXME: [PM-11690] - Temporarily ignore invalid private keys until we have a
// recovery process in place.
AsymmetricCryptoKey::from_der(&Pkcs8PrivateKeyBytes::from(dec))
.map_err(|_| {
warn!("Invalid private key");
})
Expand All @@ -72,14 +134,6 @@ impl EncryptionSettings {
// .map_err(|_| EncryptionSettingsError::InvalidPrivateKey)?,
// )
};
let signing_key = signing_key
.map(|key| {
use bitwarden_crypto::CryptoError;

let dec: Vec<u8> = key.decrypt_with_key(&user_key)?;
SigningKey::from_cose(&dec.into()).map_err(Into::<CryptoError>::into)
})
.transpose()?;

// FIXME: [PM-18098] When this is part of crypto we won't need to use deprecated methods
#[allow(deprecated)]
Expand All @@ -89,10 +143,44 @@ impl EncryptionSettings {
if let Some(private_key) = private_key {
ctx.set_asymmetric_key(AsymmetricKeyId::UserPrivateKey, private_key)?;
}
}

if let Some(signing_key) = signing_key {
ctx.set_signing_key(SigningKeyId::UserSigningKey, signing_key)?;
}
Ok(())
}

#[cfg(feature = "internal")]
fn init_v2(
user_key: XChaCha20Poly1305Key,
private_key: EncString,
signing_key: EncString,
security_state: SignedSecurityState,
store: &KeyStore<KeyIds>,
sdk_security_state: &RwLock<Option<SecurityState>>,
Copy link
Member

@Hinton Hinton Jul 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

out-of-scope: Hmm, having to pass in the SDK's state and write to it seems quite unfortunate. Outside the scope of this PR though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed in slack, but yes I agree, and we'll change this in tech-debt cleanup.

) -> Result<(), EncryptionSettingsError> {
use crate::key_management::SecurityState;

let user_key = SymmetricCryptoKey::XChaCha20Poly1305Key(user_key);

// For v2 users, we mandate the signing key and security state and the private key to be
// present and valid Everything MUST decrypt.
let signing_key: Vec<u8> = signing_key.decrypt_with_key(&user_key)?;
let signing_key = SigningKey::from_cose(&CoseKeyBytes::from(signing_key))
.map_err(|_| EncryptionSettingsError::InvalidSigningKey)?;
let private_key: Vec<u8> = private_key.decrypt_with_key(&user_key)?;
let private_key = AsymmetricCryptoKey::from_der(&Pkcs8PrivateKeyBytes::from(private_key))
.map_err(|_| EncryptionSettingsError::InvalidPrivateKey)?;

let security_state: SecurityState = security_state
.verify_and_unwrap(&signing_key.to_verifying_key())
.map_err(|_| EncryptionSettingsError::InvalidSecurityState)?;
*sdk_security_state.write().expect("RwLock not poisoned") = Some(security_state);

#[allow(deprecated)]
{
let mut ctx = store.context_mut();
ctx.set_symmetric_key(SymmetricKeyId::User, user_key)?;
ctx.set_asymmetric_key(AsymmetricKeyId::UserPrivateKey, private_key)?;
ctx.set_signing_key(SigningKeyId::UserSigningKey, signing_key)?;
}

Ok(())
Expand Down
Loading