-
Notifications
You must be signed in to change notification settings - Fork 17
[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
Changes from 4 commits
7fa725f
0387b44
6c1a57d
f4bd1e2
ae4590e
c7dc40a
eaf788e
c7854b4
2c65c3d
db94fd3
77a2ebe
14f5d3f
4fea3dc
46d2202
b20e5a4
566dc47
678a9e3
b891ec4
3dd6366
24ec20d
6f17cd4
9922265
5f98cab
429066a
162e218
35677ac
381b547
d89a17d
80fe0b7
3885e36
e6816f2
96548c2
620d3c2
959b2e7
bf269f6
5902ea2
cd8077c
e063963
a0ee4d7
d10af3d
5d6ae55
16c6248
d7dea45
8a00fff
4f2b05a
080c6ea
f76daa8
9e44be8
2c64b9c
6e85d6b
35821d9
d01b953
e3dea12
da0a0d8
4bc1214
7c4f562
ee78cb7
f4007c1
1e25d0e
17758a4
a85f84f
932f9b8
2511017
8ce7e26
95827df
6fa7eff
dfaaca9
736f744
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 |
---|---|---|
|
@@ -56,6 +56,7 @@ pub(crate) async fn login_api_key( | |
user_key, | ||
private_key, | ||
None, | ||
None, | ||
)?; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -55,6 +55,7 @@ pub(crate) async fn login_password( | |
user_key, | ||
private_key, | ||
None, | ||
None, | ||
)?; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -80,6 +80,7 @@ mod tests { | |
user_key.parse().unwrap(), | ||
private_key, | ||
None, | ||
None, | ||
) | ||
.unwrap(); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
#[cfg(feature = "internal")] | ||
use bitwarden_crypto::{EncString, UnsignedSharedKey}; | ||
use bitwarden_crypto::{security_state::SignedSecurityState, EncString, UnsignedSharedKey}; | ||
use bitwarden_crypto::{CryptoError, Pkcs8PrivateKeyBytes}; | ||
#[cfg(any(feature = "internal", feature = "secrets"))] | ||
use bitwarden_crypto::{KeyStore, SymmetricCryptoKey}; | ||
use bitwarden_error::bitwarden_error; | ||
|
@@ -30,6 +31,9 @@ pub enum EncryptionSettingsError { | |
#[error("Invalid signing key")] | ||
InvalidSigningKey, | ||
|
||
#[error("Invalid security state")] | ||
InvalidSecurityState, | ||
|
||
#[error(transparent)] | ||
MissingPrivateKey(#[from] MissingPrivateKeyError), | ||
|
||
|
@@ -50,49 +54,85 @@ impl EncryptionSettings { | |
user_key: SymmetricCryptoKey, | ||
private_key: EncString, | ||
signing_key: Option<EncString>, | ||
security_state: Option<SignedSecurityState>, | ||
store: &KeyStore<KeyIds>, | ||
) -> Result<(), EncryptionSettingsError> { | ||
use bitwarden_crypto::{AsymmetricCryptoKey, CoseSerializable, KeyDecryptable, SigningKey}; | ||
use bitwarden_crypto::{AsymmetricCryptoKey, KeyDecryptable}; | ||
use log::warn; | ||
|
||
use crate::key_management::{AsymmetricKeyId, SigningKeyId, SymmetricKeyId}; | ||
|
||
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()) | ||
.map_err(|_| { | ||
warn!("Invalid private key"); | ||
}) | ||
.ok() | ||
|
||
// Some( | ||
// AsymmetricCryptoKey::from_der(&dec) | ||
// .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)] | ||
{ | ||
let mut ctx = store.context_mut(); | ||
ctx.set_symmetric_key(SymmetricKeyId::User, user_key)?; | ||
if let Some(private_key) = private_key { | ||
use crate::key_management::{AsymmetricKeyId, SymmetricKeyId}; | ||
dani-garcia marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// 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. | ||
let is_v2_user = matches!(user_key, SymmetricCryptoKey::XChaCha20Poly1305Key(_)); | ||
if is_v2_user { | ||
// For v2 users, we mandate the signing key and security state to be present | ||
// The private key must also be valid. | ||
|
||
use bitwarden_crypto::{ | ||
security_state::SecurityState, CoseKeyBytes, CoseSerializable, SigningKey, | ||
}; | ||
|
||
// Both of these are required for v2 users | ||
let signing_key = signing_key.ok_or(EncryptionSettingsError::Crypto( | ||
CryptoError::SecurityDowngrade("Signing key is required for v2 users".to_string()), | ||
))?; | ||
let security_state = security_state.ok_or(EncryptionSettingsError::Crypto( | ||
CryptoError::SecurityDowngrade( | ||
"Security state is required for v2 users".to_string(), | ||
), | ||
))?; | ||
dani-garcia marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// 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 | ||
quexten marked this conversation as resolved.
Show resolved
Hide resolved
|
||
.verify_and_unwrap(&signing_key.to_verifying_key()) | ||
.map_err(|_| EncryptionSettingsError::InvalidSecurityState)?; | ||
|
||
#[allow(deprecated)] | ||
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. Making the entire block as allow deprecated since each of the context functions requires this. This will be removed when we remove external access to these set functions. Question for reviewers: Would it make sense to split these into a non-deprecated |
||
{ | ||
use crate::key_management::SigningKeyId; | ||
|
||
let mut ctx = store.context_mut(); | ||
ctx.set_symmetric_key(SymmetricKeyId::User, user_key)?; | ||
ctx.set_asymmetric_key(AsymmetricKeyId::UserPrivateKey, private_key)?; | ||
} | ||
|
||
if let Some(signing_key) = signing_key { | ||
ctx.set_signing_key(SigningKeyId::UserSigningKey, signing_key)?; | ||
} | ||
} else { | ||
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(&Pkcs8PrivateKeyBytes::from(dec)) | ||
.map_err(|_| { | ||
warn!("Invalid private key"); | ||
}) | ||
.ok() | ||
|
||
// Some( | ||
// AsymmetricCryptoKey::from_der(&dec) | ||
// .map_err(|_| EncryptionSettingsError::InvalidPrivateKey)?, | ||
// ) | ||
}; | ||
|
||
// FIXME: [PM-18098] When this is part of crypto we won't need to use deprecated methods | ||
#[allow(deprecated)] | ||
{ | ||
let mut ctx = store.context_mut(); | ||
ctx.set_symmetric_key(SymmetricKeyId::User, user_key)?; | ||
if let Some(private_key) = private_key { | ||
ctx.set_asymmetric_key(AsymmetricKeyId::UserPrivateKey, private_key)?; | ||
} | ||
} | ||
} | ||
|
||
Ok(()) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -126,6 +126,7 @@ pub fn test_bitwarden_com_account() -> TestAccount { | |
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::<EncString>().unwrap().to_owned(), | ||
|
||
signing_key: None, | ||
security_state: None, | ||
|
||
method: InitUserCryptoMethod::Password { | ||
password: "asdfasdfasdf".to_owned(), | ||
|
@@ -185,6 +186,7 @@ pub fn test_legacy_user_key_account() -> TestAccount { | |
email: "[email protected]".to_owned(), | ||
private_key: "2.leBIE5u0aQUeXi++JzAnrA==|P8x+hs00RJx7epw+49qVtBhLJxE/JTL5dEHg6kq5pbZLdUY8ZvWK49v0EqgHbv1r298N9+msoO9hmdSIVIAZyycemYDSoc1rX4S1KpS/ZMA/Vd3VLFb+o13Ts62GFQ5ygHKgQZfzjU6jO5P/B/0igzFoxyJDomhW5NBC1P9+e/5qNRZN8loKvAaWc/7XtpRayPQqWx+AgYc2ntb1GF5hRVrW4M47bG5ZKllbJWtQKg2sXIy2lDBbKLRFWF4RFzNVcXQGMoPdWLY0f3uTwUH01dyGmFFMbOvfBEuYqmZyPdd93ve8zuFOEqkj46Ulpq2CVG8NvZARTwsdKl6XB0wGuHFoTsDJT2SJGl67pBBKsVRGxy059QW+9hAIB+emIV0T/7+0rvdeSXZ4AbG+oXGEXFTkHefwJKfeT0MBTAjYKr7ZRLgqvf7n39+nCEJU4l22kp8FmjcWIU7AgNipdGHC+UT2yfOcYlvgBgWDcMXcbVDMyus9105RgcW6PHozUj7yjbohI/A3XWmAFufP6BSnmEFCKoik78X/ry09xwiH2rN4KVXe/k9LpRNB2QBGIVsfgCrkxjeE8r0nA59Rvwrhny1z5BkvMW/N1KrGuafg/IYgegx72gJNuZPZlFu1Vs7HxySHmzYvm3DPV7bzCaAxxNtvZmQquNIEnsDQfjJO76iL1JCtDqNJVzGLHTMTr7S5hkOcydcH3kfKwZdA1ULVd2qu0SwOUEP/ECjU/cS5INy6WPYzNMAe/g2DISpQjNwBb5K17PIiGOR7/Q/A6E8pVnkHiAXuUFr9aLOYN9BWSu5Z+BPHH65na2FDmssix5WV09I2sUBfvdNCjkrUGdYgo8E+vOTn35x9GJHF45uhmgC1yAn/+/RSpORlrSVJ7NNP11dn3htUpSsIy/b7ituAu8Ry5mhicFU8CXJL4NeMlXThUt8P++wxs4wMkBvJ8J9NJAVKbAOA2o+GOdjbh6Ww3IRegkurWh4oL/dFSx0LpaXJuw6HFT/LzticPlSwHtUP11hZ81seMsXmkSZd8IugRFfwpPl7N6PVRWDOKxLf4gPqcnJ11TvfasXy1uolV2vZCPbrbbVzQMPdVwL/OzwfhqsIgQZI8rsDMK5D2EX8MaT8MDfGcsYcVTL9PmuZYLpOUnnHX0A1opAAa9iPw3d+eWB/GAyLvKPnMTUqVNos8HcCktXckCshihA8QuBJOwg3m0j2LPSZ5Jvf8gbXauBmt9I4IlJq0xfpgquYY1WNnO8IcWE4N9W+ASvOr9gnduA6CkDeAlyMUFmdpkeCjGMcsV741bTCPApSQlL3/TOT1cjK3iejWpz0OaVHXyg02hW2fNkOfYfr81GvnLvlHxIg4Prw89gKuWU+kQk82lFQo6QQpqbCbJC2FleurD8tYoSY0srhuioVInffvTxw2NMF7FQEqUcsK9AMKSEiDqzBi35Um/fiE3JL4XZBFw8Xzl7X3ab5nlg8X+xD5uSZY+oxD3sDVXjLaQ5JUoys+MCm0FkUj85l0zT6rvM4QLhU1RDK1U51T9HJhh8hsFJsqL4abRzwEWG7PSi859zN4UsgyuQfmBJv/n7QAFCbrJhVBlGB1TKLZRzvgmKoxTYTG3cJFkjetLcUTwrwC9naxAQRfF4=|ufHf73IzJ707dx44w4fjkuD7tDa50OwmmkxcypAT9uQ=".parse::<EncString>().unwrap().to_owned(), | ||
signing_key: None, | ||
security_state: None, | ||
method: InitUserCryptoMethod::Password { | ||
password: "asdfasdfasdf".to_owned(), | ||
user_key: "0.8UClLa8IPE1iZT7chy5wzQ==|6PVfHnVk5S3XqEtQemnM5yb4JodxmPkkWzmDRdfyHtjORmvxqlLX40tBJZ+CKxQWmS8tpEB5w39rbgHg/gqs0haGdZG4cPbywsgGzxZ7uNI=".parse().unwrap(), | ||
|
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.
Suggestion: If
signing_key
andsecurity_state
will both always be defined when using a chacha key, I wonder if it would simplify the logic if instead of having two optional values, we combined both parameters into a struct and made that optional instead.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.
I've moved the entire set of keys into enum variants now. The type of symmetric key is also enforced by the type system.
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.
Note this required an
allow(large_enum_variant)
. I feel that this is OK since we only have one instance of this enum in the sdk.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.
Looks good to me, yeah that warning can be safely ignored for this case, I think.