-
Notifications
You must be signed in to change notification settings - Fork 17
[PM-24263] Pin protected key envelope unlock #372
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 all commits
bc5900b
2e905c4
b41f5ef
eb70dfc
1e2e95c
68be6e6
d6a18a4
6e9e526
50d8f70
e5bd251
b92010b
9635098
b8056c2
9440825
f8cb804
e6939e6
2a202c1
ba9e2c3
a42b1e7
5447cbb
66c345f
9b2f6c9
88b96b7
7e69b7b
b7d2bc8
acb6d12
723ec63
36d8de9
33da07b
2861a9a
1681df9
0cd85fa
f91f0b8
34ee00e
485b6d8
f122ff0
f22531c
ba10631
69d8c57
8833146
47ced5a
42dccb0
9d0ea55
a3ed9d6
9eceb32
f30c3ce
5f4dc3a
7b17ca3
38c8945
b5dd862
47c7764
b009c81
c9f6111
b9b0f6e
67dd5e9
7635cd0
ec14e51
75be6db
6d8bb8f
38b8958
9f334fb
02cc7b3
f145eb3
99d909c
d47e8c0
1920a49
625b830
24f1431
9eb4ff8
916a46e
8722077
670c6a7
6da4a0b
4e5510b
b00f48b
bf9f8c4
235f3bc
1953ba3
41bb1ad
c41e513
90d2295
de8f957
f6ad513
3c2984b
bdc90b3
80dde40
1f30896
1817ae0
416dbf5
aaad503
2e21906
dbdee15
375fd0d
fd22ded
7ee0278
9b3549a
a5abaae
81138ea
4445aec
25d1907
0d0f59b
e3ee279
bf7bc82
1da0173
87f87a6
15ffca9
144620f
53aeeee
81b5a15
9427634
d1f8029
7f52fb0
53c528b
c51e779
6c8092a
954a6a5
948baa5
b2f5211
f0b6ec5
390463c
a2e243e
616786e
b913762
120f8c6
b1a615a
25700ef
690c6df
fabee16
b81de59
fc6c32b
162a083
2e6e9e5
1986d62
41ba6c3
8e5fa67
1fef06f
b85806a
7848fa6
32aacd7
6fd28c2
d601b0c
6b11472
b8198bf
0892687
3e03ec3
9345408
6fb4e97
f685b35
02f438c
8b447b3
9cad777
ca1a346
2efaf4c
4a99de2
1357d4b
fbdced4
50d2164
8ff72f3
e65a3d4
3355c82
10f97df
06baa9e
6414e1d
9c78d35
01a2903
2c29cad
8e911af
d9c70f7
fab6509
17cdef1
a8f5353
eb6bff0
cd53925
f3c5004
4291314
660aaf1
ecab3dc
9e79c97
b127343
9c293f5
0b97c1a
e30b8b0
cb8bfb5
c41601a
d96a110
6c7c1ec
8f0ce39
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 |
---|---|---|
|
@@ -25,7 +25,10 @@ use crate::{ | |
login_method::UserLoginMethod, | ||
}, | ||
error::NotAuthenticatedError, | ||
key_management::{crypto::InitUserCryptoRequest, SecurityState, SignedSecurityState}, | ||
key_management::{ | ||
crypto::InitUserCryptoRequest, PasswordProtectedKeyEnvelope, SecurityState, | ||
SignedSecurityState, | ||
}, | ||
}; | ||
|
||
/// Represents the user's keys, that are encrypted by the user key, and the signed security state. | ||
|
@@ -309,6 +312,31 @@ impl InternalClient { | |
self.initialize_user_crypto_decrypted_key(decrypted_user_key, key_state) | ||
} | ||
|
||
#[cfg(feature = "internal")] | ||
pub(crate) fn initialize_user_crypto_pin_envelope( | ||
&self, | ||
pin: String, | ||
pin_protected_user_key_envelope: PasswordProtectedKeyEnvelope, | ||
key_state: UserKeyState, | ||
) -> Result<(), EncryptionSettingsError> { | ||
let decrypted_user_key = { | ||
// Note: This block ensures ctx is dropped. Otherwise it would cause a deadlock when | ||
// initializing the user crypto | ||
use crate::key_management::SymmetricKeyId; | ||
let ctx = &mut self.key_store.context_mut(); | ||
let decrypted_user_key_id = pin_protected_user_key_envelope | ||
.unseal(SymmetricKeyId::Local("tmp_unlock_pin"), &pin, ctx) | ||
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. @dani-garcia did we have any convention for local IDs? Or is it just the wild west. 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. I think the approach here is to push the other work dani proposed ( https://bitwarden.atlassian.net/browse/PM-18102), so that we don't have to deal with naming collisions.. 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. Yeah, I'd like to see that merged so we don't have to care about naming in situations like this. |
||
.map_err(|_| EncryptionSettingsError::WrongPin)?; | ||
|
||
// Allowing deprecated here, until a refactor to pass the Local key ids to | ||
// `initialized_user_crypto_decrypted_key` | ||
#[allow(deprecated)] | ||
ctx.dangerous_get_symmetric_key(decrypted_user_key_id)? | ||
.clone() | ||
}; | ||
self.initialize_user_crypto_decrypted_key(decrypted_user_key, key_state) | ||
} | ||
|
||
#[cfg(feature = "secrets")] | ||
pub(crate) fn initialize_crypto_single_org_key( | ||
&self, | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -7,10 +7,11 @@ | |||||
use std::collections::HashMap; | ||||||
|
||||||
use bitwarden_crypto::{ | ||||||
dangerous_get_v2_rotated_account_keys, AsymmetricCryptoKey, CoseSerializable, CryptoError, | ||||||
EncString, Kdf, KeyDecryptable, KeyEncryptable, MasterKey, Pkcs8PrivateKeyBytes, | ||||||
SignatureAlgorithm, SignedPublicKey, SigningKey, SpkiPublicKeyBytes, SymmetricCryptoKey, | ||||||
UnsignedSharedKey, UserKey, | ||||||
dangerous_get_v2_rotated_account_keys, safe::PasswordProtectedKeyEnvelopeError, | ||||||
AsymmetricCryptoKey, CoseSerializable, CryptoError, EncString, Kdf, KeyDecryptable, | ||||||
KeyEncryptable, MasterKey, Pkcs8PrivateKeyBytes, PrimitiveEncryptable, SignatureAlgorithm, | ||||||
SignedPublicKey, SigningKey, SpkiPublicKeyBytes, SymmetricCryptoKey, UnsignedSharedKey, | ||||||
UserKey, | ||||||
}; | ||||||
use bitwarden_encoding::B64; | ||||||
use bitwarden_error::bitwarden_error; | ||||||
|
@@ -23,7 +24,8 @@ use crate::{ | |||||
client::{encryption_settings::EncryptionSettingsError, LoginMethod, UserLoginMethod}, | ||||||
error::StatefulCryptoError, | ||||||
key_management::{ | ||||||
AsymmetricKeyId, SecurityState, SignedSecurityState, SigningKeyId, SymmetricKeyId, | ||||||
non_generic_wrappers::PasswordProtectedKeyEnvelope, AsymmetricKeyId, SecurityState, | ||||||
SignedSecurityState, SigningKeyId, SymmetricKeyId, | ||||||
}, | ||||||
Client, NotAuthenticatedError, OrganizationId, UserId, VaultLockedError, WrongPasswordError, | ||||||
}; | ||||||
|
@@ -39,6 +41,8 @@ pub enum CryptoClientError { | |||||
VaultLocked(#[from] VaultLockedError), | ||||||
#[error(transparent)] | ||||||
Crypto(#[from] bitwarden_crypto::CryptoError), | ||||||
#[error(transparent)] | ||||||
PasswordProtectedKeyEnvelope(#[from] PasswordProtectedKeyEnvelopeError), | ||||||
} | ||||||
|
||||||
/// State used for initializing the user cryptographic state. | ||||||
|
@@ -68,6 +72,7 @@ pub struct InitUserCryptoRequest { | |||||
#[serde(rename_all = "camelCase", deny_unknown_fields)] | ||||||
#[cfg_attr(feature = "uniffi", derive(uniffi::Enum))] | ||||||
#[cfg_attr(feature = "wasm", derive(Tsify), tsify(into_wasm_abi, from_wasm_abi))] | ||||||
#[allow(clippy::large_enum_variant)] | ||||||
pub enum InitUserCryptoMethod { | ||||||
/// Password | ||||||
Password { | ||||||
|
@@ -89,6 +94,13 @@ pub enum InitUserCryptoMethod { | |||||
/// this. | ||||||
pin_protected_user_key: EncString, | ||||||
}, | ||||||
/// PIN Envelope | ||||||
PinEnvelope { | ||||||
/// The user's PIN | ||||||
pin: String, | ||||||
/// The user's symmetric crypto key, encrypted with the PIN-protected key envelope. | ||||||
pin_protected_user_key_envelope: PasswordProtectedKeyEnvelope, | ||||||
}, | ||||||
/// Auth request | ||||||
AuthRequest { | ||||||
/// Private Key generated by the `crate::auth::new_auth_request`. | ||||||
|
@@ -173,6 +185,16 @@ pub(super) async fn initialize_user_crypto( | |||||
key_state, | ||||||
)?; | ||||||
} | ||||||
InitUserCryptoMethod::PinEnvelope { | ||||||
pin, | ||||||
pin_protected_user_key_envelope, | ||||||
} => { | ||||||
client.internal.initialize_user_crypto_pin_envelope( | ||||||
pin, | ||||||
pin_protected_user_key_envelope, | ||||||
key_state, | ||||||
)?; | ||||||
} | ||||||
InitUserCryptoMethod::AuthRequest { | ||||||
request_private_key, | ||||||
method, | ||||||
|
@@ -313,6 +335,38 @@ pub(super) fn update_password( | |||||
}) | ||||||
} | ||||||
|
||||||
/// Request for deriving a pin protected user key | ||||||
#[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 EnrollPinResponse { | ||||||
/// [UserKey] protected by PIN | ||||||
pub pin_protected_user_key_envelope: PasswordProtectedKeyEnvelope, | ||||||
/// PIN protected by [UserKey] | ||||||
pub user_key_encrypted_pin: EncString, | ||||||
} | ||||||
|
||||||
pub(super) fn enroll_pin( | ||||||
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. โ Does this need to be exposed to uniffi? I'm not seeing in the Swift bindings. 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. Thanks for catching this! It seems we still separate out clients which is confusing, but it should be fixed in the latest revision of the PR, and I have verified this gets emitted to the swift bindings locally. |
||||||
client: &Client, | ||||||
pin: String, | ||||||
) -> Result<EnrollPinResponse, CryptoClientError> { | ||||||
let key_store = client.internal.get_key_store(); | ||||||
let mut ctx = key_store.context_mut(); | ||||||
|
||||||
let key_envelope = | ||||||
PasswordProtectedKeyEnvelope(bitwarden_crypto::safe::PasswordProtectedKeyEnvelope::seal( | ||||||
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. suggestion: We already import
Suggested change
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. Ah, my bad I should have not merged this. The imported PasswordProtectedKeyEnvelope is the non-generic wrapper. The contained envelope that uses the long path is the generic struct from crypto. So they are different structs. Maybe they should have different naming after all.. 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. Yea that naming is unfortunate. |
||||||
SymmetricKeyId::User, | ||||||
&pin, | ||||||
&ctx, | ||||||
)?); | ||||||
let encrypted_pin = pin.encrypt(&mut ctx, SymmetricKeyId::User)?; | ||||||
Ok(EnrollPinResponse { | ||||||
pin_protected_user_key_envelope: key_envelope, | ||||||
user_key_encrypted_pin: encrypted_pin, | ||||||
}) | ||||||
} | ||||||
|
||||||
/// Request for deriving a pin protected user key | ||||||
#[derive(Serialize, Deserialize, Debug)] | ||||||
#[serde(rename_all = "camelCase", deny_unknown_fields)] | ||||||
|
@@ -930,6 +984,62 @@ mod tests { | |||||
assert_eq!(client_key, client3_key); | ||||||
} | ||||||
|
||||||
#[tokio::test] | ||||||
async fn test_initialize_user_crypto_pin_envelope() { | ||||||
let user_key = "5yKAZ4TSSEGje54MV5lc5ty6crkqUz4xvl+8Dm/piNLKf6OgRi2H0uzttNTXl9z6ILhkmuIXzGpAVc2YdorHgQ=="; | ||||||
let test_pin = "1234"; | ||||||
|
||||||
let client1 = Client::new(None); | ||||||
initialize_user_crypto( | ||||||
&client1, | ||||||
InitUserCryptoRequest { | ||||||
user_id: Some(UserId::new_v4()), | ||||||
kdf_params: Kdf::PBKDF2 { | ||||||
iterations: 100_000.try_into().unwrap(), | ||||||
}, | ||||||
email: "[email protected]".into(), | ||||||
private_key: make_key_pair(user_key.try_into().unwrap()) | ||||||
.unwrap() | ||||||
.user_key_encrypted_private_key, | ||||||
signing_key: None, | ||||||
security_state: None, | ||||||
method: InitUserCryptoMethod::DecryptedKey { | ||||||
decrypted_user_key: user_key.to_string(), | ||||||
}, | ||||||
}, | ||||||
) | ||||||
.await | ||||||
.unwrap(); | ||||||
|
||||||
let enroll_response = client1.crypto().enroll_pin(test_pin.to_string()).unwrap(); | ||||||
|
||||||
let client2 = Client::new(None); | ||||||
initialize_user_crypto( | ||||||
&client2, | ||||||
InitUserCryptoRequest { | ||||||
user_id: Some(UserId::new_v4()), | ||||||
// NOTE: THIS CHANGES KDF SETTINGS. We ensure in this test that even with different | ||||||
// KDF settings the pin can unlock the user key. | ||||||
kdf_params: Kdf::PBKDF2 { | ||||||
iterations: 600_000.try_into().unwrap(), | ||||||
}, | ||||||
email: "[email protected]".into(), | ||||||
private_key: make_key_pair(user_key.try_into().unwrap()) | ||||||
.unwrap() | ||||||
.user_key_encrypted_private_key, | ||||||
signing_key: None, | ||||||
security_state: None, | ||||||
method: InitUserCryptoMethod::PinEnvelope { | ||||||
pin: test_pin.to_string(), | ||||||
pin_protected_user_key_envelope: enroll_response | ||||||
.pin_protected_user_key_envelope, | ||||||
}, | ||||||
}, | ||||||
) | ||||||
.await | ||||||
.unwrap(); | ||||||
} | ||||||
|
||||||
#[test] | ||||||
fn test_enroll_admin_password_reset() { | ||||||
let client = Client::new(None); | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
use bitwarden_crypto::CryptoError; | ||
use bitwarden_crypto::{CryptoError, Decryptable}; | ||
#[cfg(feature = "internal")] | ||
use bitwarden_crypto::{EncString, UnsignedSharedKey}; | ||
use bitwarden_encoding::B64; | ||
|
@@ -10,18 +10,23 @@ use super::crypto::{ | |
DeriveKeyConnectorRequest, EnrollAdminPasswordResetError, MakeKeyPairResponse, | ||
VerifyAsymmetricKeysRequest, VerifyAsymmetricKeysResponse, | ||
}; | ||
#[cfg(any(feature = "wasm", test))] | ||
use crate::key_management::PasswordProtectedKeyEnvelope; | ||
#[cfg(feature = "internal")] | ||
use crate::key_management::crypto::{ | ||
derive_pin_key, derive_pin_user_key, enroll_admin_password_reset, get_user_encryption_key, | ||
initialize_org_crypto, initialize_user_crypto, update_password, DerivePinKeyResponse, | ||
InitOrgCryptoRequest, InitUserCryptoRequest, UpdatePasswordResponse, | ||
use crate::key_management::{ | ||
crypto::{ | ||
derive_pin_key, derive_pin_user_key, enroll_admin_password_reset, get_user_encryption_key, | ||
initialize_org_crypto, initialize_user_crypto, update_password, DerivePinKeyResponse, | ||
InitOrgCryptoRequest, InitUserCryptoRequest, UpdatePasswordResponse, | ||
}, | ||
SymmetricKeyId, | ||
}; | ||
use crate::{ | ||
client::encryption_settings::EncryptionSettingsError, | ||
error::StatefulCryptoError, | ||
key_management::crypto::{ | ||
get_v2_rotated_account_keys, make_v2_keys_for_v1_user, CryptoClientError, | ||
UserCryptoV2KeysResponse, | ||
enroll_pin, get_v2_rotated_account_keys, make_v2_keys_for_v1_user, CryptoClientError, | ||
EnrollPinResponse, UserCryptoV2KeysResponse, | ||
}, | ||
Client, | ||
}; | ||
|
@@ -81,6 +86,45 @@ impl CryptoClient { | |
) -> Result<UserCryptoV2KeysResponse, StatefulCryptoError> { | ||
get_v2_rotated_account_keys(&self.client) | ||
} | ||
|
||
/// Protects the current user key with the provided PIN. The result can be stored and later | ||
/// used to initialize another client instance by using the PIN and the PIN key with | ||
/// `initialize_user_crypto`. | ||
pub fn enroll_pin(&self, pin: String) -> Result<EnrollPinResponse, CryptoClientError> { | ||
enroll_pin(&self.client, pin) | ||
} | ||
|
||
/// Protects the current user key with the provided PIN. The result can be stored and later | ||
/// used to initialize another client instance by using the PIN and the PIN key with | ||
/// `initialize_user_crypto`. The provided pin is encrypted with the user key. | ||
pub fn enroll_pin_with_encrypted_pin( | ||
&self, | ||
// Note: This will be replaced by `EncString` with https://bitwarden.atlassian.net/browse/PM-24775 | ||
encrypted_pin: String, | ||
) -> Result<EnrollPinResponse, CryptoClientError> { | ||
let encrypted_pin: EncString = encrypted_pin.parse()?; | ||
let pin = encrypted_pin.decrypt( | ||
&mut self.client.internal.get_key_store().context_mut(), | ||
SymmetricKeyId::User, | ||
)?; | ||
enroll_pin(&self.client, pin) | ||
} | ||
|
||
/// Decrypts a `PasswordProtectedKeyEnvelope`, returning the user key, if successful. | ||
/// This is a stop-gap solution, until initialization of the SDK is used. | ||
#[cfg(any(feature = "wasm", test))] | ||
pub fn unseal_password_protected_key_envelope( | ||
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. ๐ก Shall we add some unit test coverage ? 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. Added test for the enroll -> re-enroll with encrypted -> unseal flow. |
||
&self, | ||
pin: String, | ||
envelope: PasswordProtectedKeyEnvelope, | ||
) -> Result<Vec<u8>, CryptoClientError> { | ||
let mut ctx = self.client.internal.get_key_store().context_mut(); | ||
let key_slot = SymmetricKeyId::Local("unseal_password_protected_key_envelope"); | ||
envelope.unseal(key_slot, pin.as_str(), &mut ctx)?; | ||
#[allow(deprecated)] | ||
let key = ctx.dangerous_get_symmetric_key(key_slot)?; | ||
Ok(key.to_encoded().to_vec()) | ||
} | ||
} | ||
|
||
impl CryptoClient { | ||
|
@@ -141,3 +185,40 @@ impl Client { | |
} | ||
} | ||
} | ||
|
||
#[cfg(test)] | ||
mod tests { | ||
use bitwarden_crypto::{BitwardenLegacyKeyBytes, SymmetricCryptoKey}; | ||
|
||
use super::*; | ||
use crate::client::test_accounts::test_bitwarden_com_account; | ||
|
||
#[tokio::test] | ||
async fn test_enroll_pin_envelope() { | ||
// Initialize a test client with user crypto | ||
let client = Client::init_test_account(test_bitwarden_com_account()).await; | ||
let user_key_initial = | ||
SymmetricCryptoKey::try_from(client.crypto().get_user_encryption_key().await.unwrap()) | ||
.unwrap(); | ||
|
||
// Enroll with a PIN, then re-enroll | ||
let pin = "1234"; | ||
let enroll_response = client.crypto().enroll_pin(pin.to_string()).unwrap(); | ||
let re_enroll_response = client | ||
.crypto() | ||
.enroll_pin_with_encrypted_pin(enroll_response.user_key_encrypted_pin.to_string()) | ||
.unwrap(); | ||
|
||
let secret = BitwardenLegacyKeyBytes::from( | ||
client | ||
.crypto() | ||
.unseal_password_protected_key_envelope( | ||
pin.to_string(), | ||
re_enroll_response.pin_protected_user_key_envelope, | ||
) | ||
.unwrap(), | ||
); | ||
let user_key_final = SymmetricCryptoKey::try_from(&secret).unwrap(); | ||
assert_eq!(user_key_initial, user_key_final); | ||
} | ||
} |
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: What error do we return if you pass in the wrong password? It would seem reasonable that we return a similar error for both.
I suspect we derive the master key and then attempt to decrypt the user key which would fail and you'd get a decrypt error? Maybe it would be more elegant to handle that with a better error message in which case both wrong password and pin could use the same error?
Uh oh!
There was an error while loading. Please reload this page.
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.
With master-key decryption you cannot know if:
and all of these would result in the same error. In most cases the true reason would be "wrong password" but there are reasonable cases such as the KDF settings being out of sync, that would lead to the same error path.
With the PasswordProtectedKeyEnvelope, it's only:
so for me it feels more appropriate to use the "WrongPin" error.
(I'm OK changing both to return "WrongPassword", but then we should document that this could also mean that KDF/salt(email) can be wrong for masterkey encrypted items).