-
Notifications
You must be signed in to change notification settings - Fork 17
[PM-24683] Add updateKdf function #383
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
base: main
Are you sure you want to change the base?
Changes from 59 commits
44773f7
0712878
8ca20a8
28a8423
b089ea0
0d35393
a08179d
b3647f2
36d3136
7c8664d
9c7d50d
70ad9d3
110f4db
ff86adf
fa253a3
4a95dc5
cd37e31
3bf880f
93a3d0c
fa1b16b
af83c57
e780d69
f0c4f2a
3e6a46a
7c34725
042678c
9676069
bcc183a
6398e13
e816975
21c72e0
3dc3577
367f9f0
3d9b257
69ed59c
866912b
08d10fe
0aba54b
4e41a29
7973f12
5a4f314
7e425ca
c9aaa8c
2591773
c908173
9f2c473
ed3be99
48bfbda
b4d89e2
aeaeff5
7cf85b4
3212002
1a3dd14
55cc2b7
dd5dd3e
20b5a06
18e7913
5cf50d2
6cdb29b
77c2d68
a487fff
ecfde52
8d5c8c4
d5a7180
285132a
aec1105
16f73ce
47da2c9
0b99b78
77a142e
c703c10
db8e8b0
139d2b1
a0ede96
198c29c
06f160a
35c0e8d
07b25ed
f2dff60
b0030e4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
use bitwarden_api_api::models::MasterPasswordUnlockResponseModel; | ||
use serde::{Deserialize, Serialize}; | ||
|
||
#[derive(Serialize, Deserialize, Debug, PartialEq)] | ||
pub struct IdentityUserDecryptionOptionsResponseModel { | ||
Hinton marked this conversation as resolved.
Show resolved
Hide resolved
|
||
#[serde( | ||
rename = "masterPasswordUnlock", | ||
skip_serializing_if = "Option::is_none" | ||
)] | ||
pub master_password_unlock: Option<MasterPasswordUnlockResponseModel>, | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,7 @@ use crate::{ | |
client::{encryption_settings::EncryptionSettingsError, LoginMethod, UserLoginMethod}, | ||
error::StatefulCryptoError, | ||
key_management::{ | ||
master_password::{MasterPasswordAuthenticationData, MasterPasswordUnlockData}, | ||
AsymmetricKeyId, SecurityState, SignedSecurityState, SigningKeyId, SymmetricKeyId, | ||
}, | ||
Client, NotAuthenticatedError, VaultLockedError, WrongPasswordError, | ||
|
@@ -39,6 +40,8 @@ pub enum CryptoClientError { | |
VaultLocked(#[from] VaultLockedError), | ||
#[error(transparent)] | ||
Crypto(#[from] bitwarden_crypto::CryptoError), | ||
#[error("Invalid KDF settings")] | ||
InvalidKdfSettings, | ||
} | ||
|
||
/// State used for initializing the user cryptographic state. | ||
|
@@ -265,6 +268,64 @@ pub(super) async fn get_user_encryption_key(client: &Client) -> Result<String, C | |
Ok(user_key.to_base64()) | ||
} | ||
|
||
/// Response from the `update_kdf` function | ||
#[derive(Serialize, Deserialize, Debug)] | ||
#[serde(rename_all = "camelCase", deny_unknown_fields)] | ||
#[cfg_attr(feature = "uniffi", derive(uniffi::Record))] | ||
#[cfg_attr(feature = "wasm", derive(Tsify), tsify(into_wasm_abi, from_wasm_abi))] | ||
pub struct UpdateKdfResponse { | ||
/// The authentication data for the new KDF setting | ||
master_password_authentication_data: MasterPasswordAuthenticationData, | ||
/// The unlock data for the new KDF setting | ||
master_password_unlock_data: MasterPasswordUnlockData, | ||
/// The authentication data for the KDF setting prior to the change | ||
old_master_password_authentication_data: MasterPasswordAuthenticationData, | ||
} | ||
|
||
pub(super) fn make_update_kdf( | ||
client: &Client, | ||
password: &str, | ||
new_kdf: &Kdf, | ||
) -> Result<UpdateKdfResponse, CryptoClientError> { | ||
let key_store = client.internal.get_key_store(); | ||
let ctx = key_store.context(); | ||
// FIXME: [PM-18099] Once MasterKey deals with KeyIds, this should be updated | ||
#[allow(deprecated)] | ||
let user_key = ctx.dangerous_get_symmetric_key(SymmetricKeyId::User)?; | ||
|
||
let login_method = client | ||
.internal | ||
.get_login_method() | ||
.ok_or(NotAuthenticatedError)?; | ||
let email = match login_method.as_ref() { | ||
LoginMethod::User( | ||
UserLoginMethod::Username { email, .. } | UserLoginMethod::ApiKey { email, .. }, | ||
) => email, | ||
#[cfg(feature = "secrets")] | ||
LoginMethod::ServiceAccount(_) => return Err(NotAuthenticatedError)?, | ||
}; | ||
|
||
let authentication_data = MasterPasswordAuthenticationData::derive(password, new_kdf, email) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These derive functions will only fail if the KDF settings are insufficient right? If that's the case we should rename the error variant to make it more clear, because the error is unrelated to the master password. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That makes sense. The variant is now just "InvalidKdfSettings", and discards the returned |
||
.map_err(|_| CryptoClientError::InvalidKdfSettings)?; | ||
let unlock_data = MasterPasswordUnlockData::derive(password, new_kdf, email, user_key) | ||
.map_err(|_| CryptoClientError::InvalidKdfSettings)?; | ||
let old_authentication_data = MasterPasswordAuthenticationData::derive( | ||
password, | ||
&client | ||
.internal | ||
.get_kdf() | ||
.map_err(|_| NotAuthenticatedError)?, | ||
email, | ||
) | ||
.map_err(|_| CryptoClientError::InvalidKdfSettings)?; | ||
|
||
Ok(UpdateKdfResponse { | ||
master_password_authentication_data: authentication_data, | ||
master_password_unlock_data: unlock_data, | ||
old_master_password_authentication_data: old_authentication_data, | ||
}) | ||
} | ||
|
||
/// Response from the `update_password` function | ||
#[derive(Serialize, Deserialize, Debug)] | ||
#[serde(rename_all = "camelCase", deny_unknown_fields)] | ||
|
@@ -277,7 +338,7 @@ pub struct UpdatePasswordResponse { | |
new_key: EncString, | ||
} | ||
|
||
pub(super) fn update_password( | ||
pub(super) fn make_update_password( | ||
client: &Client, | ||
new_password: String, | ||
) -> Result<UpdatePasswordResponse, CryptoClientError> { | ||
|
@@ -307,7 +368,7 @@ pub(super) fn update_password( | |
let password_hash = new_master_key.derive_master_key_hash( | ||
new_password.as_bytes(), | ||
bitwarden_crypto::HashPurpose::ServerAuthorization, | ||
)?; | ||
); | ||
|
||
Ok(UpdatePasswordResponse { | ||
password_hash, | ||
|
@@ -737,6 +798,100 @@ mod tests { | |
"pgEBAlAmkP0QgfdMVbIujX55W/yNAycEgQIgBiFYIEM6JxBmjWQTruAm3s6BTaJy1q6BzQetMBacNeRJ0kxR"; | ||
const TEST_VECTOR_SECURITY_STATE_V2: &str = "hFgepAEnAxg8BFAmkP0QgfdMVbIujX55W/yNOgABOH8CoFgkomhlbnRpdHlJZFBHOOw2BI9OQoNq+Vl1xZZKZ3ZlcnNpb24CWEAlchbJR0vmRfShG8On7Q2gknjkw4Dd6MYBLiH4u+/CmfQdmjNZdf6kozgW/6NXyKVNu8dAsKsin+xxXkDyVZoG"; | ||
|
||
#[tokio::test] | ||
async fn test_update_kdf() { | ||
let client = Client::new(None); | ||
|
||
let priv_key: EncString = "2.kmLY8NJVuiKBFJtNd/ZFpA==|qOodlRXER+9ogCe3yOibRHmUcSNvjSKhdDuztLlucs10jLiNoVVVAc+9KfNErLSpx5wmUF1hBOJM8zwVPjgQTrmnNf/wuDpwiaCxNYb/0v4FygPy7ccAHK94xP1lfqq7U9+tv+/yiZSwgcT+xF0wFpoxQeNdNRFzPTuD9o4134n8bzacD9DV/WjcrXfRjbBCzzuUGj1e78+A7BWN7/5IWLz87KWk8G7O/W4+8PtEzlwkru6Wd1xO19GYU18oArCWCNoegSmcGn7w7NDEXlwD403oY8Oa7ylnbqGE28PVJx+HLPNIdSC6YKXeIOMnVs7Mctd/wXC93zGxAWD6ooTCzHSPVV50zKJmWIG2cVVUS7j35H3rGDtUHLI+ASXMEux9REZB8CdVOZMzp2wYeiOpggebJy6MKOZqPT1R3X0fqF2dHtRFPXrNsVr1Qt6bS9qTyO4ag1/BCvXF3P1uJEsI812BFAne3cYHy5bIOxuozPfipJrTb5WH35bxhElqwT3y/o/6JWOGg3HLDun31YmiZ2HScAsUAcEkA4hhoTNnqy4O2s3yVbCcR7jF7NLsbQc0MDTbnjxTdI4VnqUIn8s2c9hIJy/j80pmO9Bjxp+LQ9a2hUkfHgFhgHxZUVaeGVth8zG2kkgGdrp5VHhxMVFfvB26Ka6q6qE/UcS2lONSv+4T8niVRJz57qwctj8MNOkA3PTEfe/DP/LKMefke31YfT0xogHsLhDkx+mS8FCc01HReTjKLktk/Jh9mXwC5oKwueWWwlxI935ecn+3I2kAuOfMsgPLkoEBlwgiREC1pM7VVX1x8WmzIQVQTHd4iwnX96QewYckGRfNYWz/zwvWnjWlfcg8kRSe+68EHOGeRtC5r27fWLqRc0HNcjwpgHkI/b6czerCe8+07TWql4keJxJxhBYj3iOH7r9ZS8ck51XnOb8tGL1isimAJXodYGzakwktqHAD7MZhS+P02O+6jrg7d+yPC2ZCuS/3TOplYOCHQIhnZtR87PXTUwr83zfOwAwCyv6KP84JUQ45+DItrXLap7nOVZKQ5QxYIlbThAO6eima6Zu5XHfqGPMNWv0bLf5+vAjIa5np5DJrSwz9no/hj6CUh0iyI+SJq4RGI60lKtypMvF6MR3nHLEHOycRUQbZIyTHWl4QQLdHzuwN9lv10ouTEvNr6sFflAX2yb6w3hlCo7oBytH3rJekjb3IIOzBpeTPIejxzVlh0N9OT5MZdh4sNKYHUoWJ8mnfjdM+L4j5Q2Kgk/XiGDgEebkUxiEOQUdVpePF5uSCE+TPav/9FIRGXGiFn6NJMaU7aBsDTFBLloffFLYDpd8/bTwoSvifkj7buwLYM+h/qcnfdy5FWau1cKav+Blq/ZC0qBpo658RTC8ZtseAFDgXoQZuksM10hpP9bzD04Bx30xTGX81QbaSTNwSEEVrOtIhbDrj9OI43KH4O6zLzK+t30QxAv5zjk10RZ4+5SAdYndIlld9Y62opCfPDzRy3ubdve4ZEchpIKWTQvIxq3T5ogOhGaWBVYnkMtM2GVqvWV//46gET5SH/MdcwhACUcZ9kCpMnWH9CyyUwYvTT3UlNyV+DlS27LMPvaw7tx7qa+GfNCoCBd8S4esZpQYK/WReiS8=|pc7qpD42wxyXemdNPuwxbh8iIaryrBPu8f/DGwYdHTw=".parse().unwrap(); | ||
|
||
let kdf = Kdf::PBKDF2 { | ||
iterations: 100_000.try_into().unwrap(), | ||
}; | ||
|
||
initialize_user_crypto( | ||
& client, | ||
InitUserCryptoRequest { | ||
user_id: Some(uuid::Uuid::new_v4()), | ||
kdf_params: kdf.clone(), | ||
email: "[email protected]".into(), | ||
private_key: priv_key.to_owned(), | ||
signing_key: None, | ||
security_state: None, | ||
method: InitUserCryptoMethod::Password { | ||
password: "asdfasdfasdf".into(), | ||
user_key: "2.u2HDQ/nH2J7f5tYHctZx6Q==|NnUKODz8TPycWJA5svexe1wJIz2VexvLbZh2RDfhj5VI3wP8ZkR0Vicvdv7oJRyLI1GyaZDBCf9CTBunRTYUk39DbZl42Rb+Xmzds02EQhc=|rwuo5wgqvTJf3rgwOUfabUyzqhguMYb3sGBjOYqjevc=".parse().unwrap(), | ||
}, | ||
}, | ||
) | ||
.await | ||
.unwrap(); | ||
|
||
let new_kdf = Kdf::PBKDF2 { | ||
iterations: 600_000.try_into().unwrap(), | ||
}; | ||
let new_kdf_response = make_update_kdf(&client, "123412341234", &new_kdf).unwrap(); | ||
|
||
let client2 = Client::new(None); | ||
|
||
initialize_user_crypto( | ||
&client2, | ||
InitUserCryptoRequest { | ||
user_id: Some(uuid::Uuid::new_v4()), | ||
kdf_params: new_kdf.clone(), | ||
email: "[email protected]".into(), | ||
private_key: priv_key.to_owned(), | ||
signing_key: None, | ||
security_state: None, | ||
method: InitUserCryptoMethod::Password { | ||
password: "123412341234".into(), | ||
user_key: new_kdf_response | ||
.master_password_unlock_data | ||
.master_key_wrapped_user_key, | ||
}, | ||
}, | ||
) | ||
.await | ||
.unwrap(); | ||
|
||
let new_hash = client2 | ||
.kdf() | ||
.hash_password( | ||
"[email protected]".into(), | ||
"123412341234".into(), | ||
new_kdf.clone(), | ||
bitwarden_crypto::HashPurpose::ServerAuthorization, | ||
) | ||
.await | ||
.unwrap(); | ||
|
||
assert_eq!( | ||
new_hash, | ||
new_kdf_response | ||
.master_password_authentication_data | ||
.master_password_authentication_hash | ||
); | ||
|
||
let client_key = { | ||
let key_store = client.internal.get_key_store(); | ||
let ctx = key_store.context(); | ||
#[allow(deprecated)] | ||
ctx.dangerous_get_symmetric_key(SymmetricKeyId::User) | ||
.unwrap() | ||
.to_base64() | ||
}; | ||
|
||
let client2_key = { | ||
let key_store = client2.internal.get_key_store(); | ||
let ctx = key_store.context(); | ||
#[allow(deprecated)] | ||
ctx.dangerous_get_symmetric_key(SymmetricKeyId::User) | ||
.unwrap() | ||
.to_base64() | ||
}; | ||
|
||
assert_eq!(client_key, client2_key); | ||
} | ||
|
||
#[tokio::test] | ||
async fn test_update_password() { | ||
let client = Client::new(None); | ||
|
@@ -765,7 +920,7 @@ mod tests { | |
.await | ||
.unwrap(); | ||
|
||
let new_password_response = update_password(&client, "123412341234".into()).unwrap(); | ||
let new_password_response = make_update_password(&client, "123412341234".into()).unwrap(); | ||
|
||
let client2 = Client::new(None); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Why not use
#[serde(rename_all = "camelCase")]
rather than renaming the field?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This response model was actually leftover from a previous upstream PR. Removed since its not needed.