Skip to content
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

[PM-10929] Support Key Connector #959

Merged
merged 8 commits into from
Aug 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
15 changes: 11 additions & 4 deletions crates/bitwarden-core/src/auth/client_auth.rs
Original file line number Diff line number Diff line change
@@ -1,16 +1,18 @@
#[cfg(feature = "internal")]
use bitwarden_crypto::{AsymmetricEncString, DeviceKey, EncString, Kdf, TrustDeviceResponse};
use bitwarden_crypto::{
AsymmetricEncString, CryptoError, DeviceKey, EncString, Kdf, TrustDeviceResponse,
};

#[cfg(feature = "internal")]
use crate::auth::login::NewAuthRequestResponse;
#[cfg(feature = "secrets")]
use crate::auth::login::{login_access_token, AccessTokenLoginRequest, AccessTokenLoginResponse};
#[cfg(feature = "internal")]
use crate::auth::{
auth_request::{approve_auth_request, new_auth_request},
key_connector::{make_key_connector_keys, KeyConnectorResponse},
login::{
login_api_key, login_password, send_two_factor_email, ApiKeyLoginRequest,
ApiKeyLoginResponse, PasswordLoginRequest, PasswordLoginResponse, TwoFactorEmailRequest,
ApiKeyLoginResponse, NewAuthRequestResponse, PasswordLoginRequest, PasswordLoginResponse,
TwoFactorEmailRequest,
},
password::{
password_strength, satisfies_policy, validate_password, validate_password_user_key,
Expand Down Expand Up @@ -79,6 +81,11 @@
make_register_tde_keys(self.client, email, org_public_key, remember_device)
}

pub fn make_key_connector_keys(&self) -> Result<KeyConnectorResponse, CryptoError> {
Copy link
Member

Choose a reason for hiding this comment

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

Why not return a Result? Equivalently, why does this method return a specific error type rather than the base error enum?

Copy link
Member Author

Choose a reason for hiding this comment

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

We're transitioning away from a single massive enum as that meshes badly with the new crate structure. And it's generally a better experience knowing what errors you get vs needing to validate all errors.

let mut rng = rand::thread_rng();
make_key_connector_keys(&mut rng)
}

Check warning on line 87 in crates/bitwarden-core/src/auth/client_auth.rs

View check run for this annotation

Codecov / codecov/patch

crates/bitwarden-core/src/auth/client_auth.rs#L84-L87

Added lines #L84 - L87 were not covered by tests

pub async fn register(&self, input: &RegisterRequest) -> Result<()> {
register(self.client, input).await
}
Expand Down
42 changes: 42 additions & 0 deletions crates/bitwarden-core/src/auth/key_connector.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
use bitwarden_crypto::{CryptoError, MasterKey, RsaKeyPair};

#[cfg_attr(feature = "uniffi", derive(uniffi::Record))]
pub struct KeyConnectorResponse {
pub master_key: String,
pub encrypted_user_key: String,
pub keys: RsaKeyPair,
}

pub(super) fn make_key_connector_keys(
mut rng: impl rand::RngCore,
) -> Result<KeyConnectorResponse, CryptoError> {
let master_key = MasterKey::generate(&mut rng);
let (user_key, encrypted_user_key) = master_key.make_user_key()?;
let keys = user_key.make_key_pair()?;

Ok(KeyConnectorResponse {
master_key: master_key.to_base64(),
encrypted_user_key: encrypted_user_key.to_string(),
keys,
})
Comment on lines +13 to +21
Copy link
Member Author

Choose a reason for hiding this comment

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

This is quite different compared to our current approach, but helps avoiding the weird steps in our ts project @jlf0dev.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps I don't understand the key connector flow, but I had thought the encrypting keys were generated by key connect -- are they just stored there?

Copy link
Member

Choose a reason for hiding this comment

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

Correct - just stored there.

}

#[cfg(test)]
mod tests {
use rand::SeedableRng;
use rand_chacha::ChaCha8Rng;

use super::*;

#[test]
fn test_make_key_connector_keys() {
let mut rng = ChaCha8Rng::from_seed([0u8; 32]);

let result = make_key_connector_keys(&mut rng).unwrap();

assert_eq!(
result.master_key,
"PgDvL4lfQNZ/W7joHwmloSyEDsPOmn87GBvhiO9xGh4="
);
}
}
4 changes: 4 additions & 0 deletions crates/bitwarden-core/src/auth/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@ pub use register::{RegisterKeyResponse, RegisterRequest};
mod tde;
#[cfg(feature = "internal")]
pub use tde::RegisterTdeKeyResponse;
#[cfg(feature = "internal")]
mod key_connector;
#[cfg(feature = "internal")]
pub use key_connector::KeyConnectorResponse;

#[cfg(feature = "internal")]
use crate::error::Result;
Expand Down
13 changes: 6 additions & 7 deletions crates/bitwarden-core/src/mobile/client_crypto.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#[cfg(feature = "internal")]
use bitwarden_crypto::{AsymmetricEncString, EncString};

use super::crypto::{derive_key_connector, DeriveKeyConnectorRequest};
use crate::Client;
#[cfg(feature = "internal")]
use crate::{
Expand All @@ -17,40 +18,38 @@
}

impl<'a> ClientCrypto<'a> {
#[cfg(feature = "internal")]
pub async fn initialize_user_crypto(&self, req: InitUserCryptoRequest) -> Result<()> {
initialize_user_crypto(self.client, req).await
}

#[cfg(feature = "internal")]
pub async fn initialize_org_crypto(&self, req: InitOrgCryptoRequest) -> Result<()> {
initialize_org_crypto(self.client, req).await
}

#[cfg(feature = "internal")]
pub async fn get_user_encryption_key(&self) -> Result<String> {
get_user_encryption_key(self.client).await
}

#[cfg(feature = "internal")]
pub fn update_password(&self, new_password: String) -> Result<UpdatePasswordResponse> {
update_password(self.client, new_password)
}

#[cfg(feature = "internal")]
pub fn derive_pin_key(&self, pin: String) -> Result<DerivePinKeyResponse> {
derive_pin_key(self.client, pin)
}

#[cfg(feature = "internal")]
pub fn derive_pin_user_key(&self, encrypted_pin: EncString) -> Result<EncString> {
derive_pin_user_key(self.client, encrypted_pin)
}

#[cfg(feature = "internal")]
pub fn enroll_admin_password_reset(&self, public_key: String) -> Result<AsymmetricEncString> {
enroll_admin_password_reset(self.client, public_key)
}

/// Derive the master key for migrating to the key connector
pub fn derive_key_connector(&self, request: DeriveKeyConnectorRequest) -> Result<String> {
derive_key_connector(request)
}

Check warning on line 52 in crates/bitwarden-core/src/mobile/client_crypto.rs

View check run for this annotation

Codecov / codecov/patch

crates/bitwarden-core/src/mobile/client_crypto.rs#L50-L52

Added lines #L50 - L52 were not covered by tests
}

impl<'a> Client {
Expand Down
81 changes: 60 additions & 21 deletions crates/bitwarden-core/src/mobile/crypto.rs
Original file line number Diff line number Diff line change
@@ -1,19 +1,18 @@
use std::collections::HashMap;

use bitwarden_crypto::{AsymmetricEncString, EncString};
#[cfg(feature = "internal")]
use bitwarden_crypto::{Kdf, KeyDecryptable, KeyEncryptable, MasterKey, SymmetricCryptoKey};
use bitwarden_crypto::{
AsymmetricEncString, EncString, Kdf, KeyDecryptable, KeyEncryptable, MasterKey,
SymmetricCryptoKey,
};
use schemars::JsonSchema;
use serde::{Deserialize, Serialize};

#[cfg(feature = "internal")]
use crate::client::{LoginMethod, UserLoginMethod};
use crate::{
client::{LoginMethod, UserLoginMethod},
error::{Error, Result},
Client,
};

#[cfg(feature = "internal")]
#[derive(Serialize, Deserialize, Debug, JsonSchema)]
#[serde(rename_all = "camelCase", deny_unknown_fields)]
#[cfg_attr(feature = "uniffi", derive(uniffi::Record))]
Expand All @@ -28,7 +27,6 @@
pub method: InitUserCryptoMethod,
}

#[cfg(feature = "internal")]
#[derive(Serialize, Deserialize, Debug, JsonSchema)]
#[serde(rename_all = "camelCase", deny_unknown_fields)]
#[cfg_attr(feature = "uniffi", derive(uniffi::Enum))]
Expand Down Expand Up @@ -64,9 +62,14 @@
/// The user's symmetric crypto key, encrypted with the Device Key.
device_protected_user_key: AsymmetricEncString,
},
KeyConnector {
/// Base64 encoded master key, retrieved from the key connector.
master_key: String,
/// The user's encrypted symmetric crypto key
user_key: String,
},
}

#[cfg(feature = "internal")]
#[derive(Serialize, Deserialize, Debug, JsonSchema)]
#[serde(rename_all = "camelCase", deny_unknown_fields)]
#[cfg_attr(feature = "uniffi", derive(uniffi::Enum))]
Expand All @@ -83,7 +86,6 @@
},
}

#[cfg(feature = "internal")]
pub async fn initialize_user_crypto(client: &Client, req: InitUserCryptoRequest) -> Result<()> {
use bitwarden_crypto::{DeviceKey, PinKey};

Expand Down Expand Up @@ -151,6 +153,17 @@
.internal
.initialize_user_crypto_decrypted_key(user_key, private_key)?;
}
InitUserCryptoMethod::KeyConnector {
master_key,
user_key,

Check warning on line 158 in crates/bitwarden-core/src/mobile/crypto.rs

View check run for this annotation

Codecov / codecov/patch

crates/bitwarden-core/src/mobile/crypto.rs#L157-L158

Added lines #L157 - L158 were not covered by tests
} => {
let master_key = MasterKey::new(SymmetricCryptoKey::try_from(master_key)?);
let user_key: EncString = user_key.parse()?;

Check warning on line 161 in crates/bitwarden-core/src/mobile/crypto.rs

View check run for this annotation

Codecov / codecov/patch

crates/bitwarden-core/src/mobile/crypto.rs#L160-L161

Added lines #L160 - L161 were not covered by tests

client
.internal
.initialize_user_crypto_master_key(master_key, user_key, private_key)?;

Check warning on line 165 in crates/bitwarden-core/src/mobile/crypto.rs

View check run for this annotation

Codecov / codecov/patch

crates/bitwarden-core/src/mobile/crypto.rs#L163-L165

Added lines #L163 - L165 were not covered by tests
}
}

client
Expand All @@ -166,7 +179,6 @@
Ok(())
}

#[cfg(feature = "internal")]
#[derive(Serialize, Deserialize, Debug, JsonSchema)]
#[serde(rename_all = "camelCase", deny_unknown_fields)]
#[cfg_attr(feature = "uniffi", derive(uniffi::Record))]
Expand All @@ -175,22 +187,19 @@
pub organization_keys: HashMap<uuid::Uuid, AsymmetricEncString>,
}

#[cfg(feature = "internal")]
pub async fn initialize_org_crypto(client: &Client, req: InitOrgCryptoRequest) -> Result<()> {
let organization_keys = req.organization_keys.into_iter().collect();
client.internal.initialize_org_crypto(organization_keys)?;
Ok(())
}

#[cfg(feature = "internal")]
pub async fn get_user_encryption_key(client: &Client) -> Result<String> {
let enc = client.internal.get_encryption_settings()?;
let user_key = enc.get_key(&None)?;

Ok(user_key.to_base64())
}

#[cfg(feature = "internal")]
#[derive(Serialize, Deserialize, Debug, JsonSchema)]
#[serde(rename_all = "camelCase", deny_unknown_fields)]
#[cfg_attr(feature = "uniffi", derive(uniffi::Record))]
Expand Down Expand Up @@ -233,7 +242,6 @@
})
}

#[cfg(feature = "internal")]
#[derive(Serialize, Deserialize, Debug, JsonSchema)]
#[serde(rename_all = "camelCase", deny_unknown_fields)]
#[cfg_attr(feature = "uniffi", derive(uniffi::Record))]
Expand All @@ -244,7 +252,6 @@
encrypted_pin: EncString,
}

#[cfg(feature = "internal")]
pub fn derive_pin_key(client: &Client, pin: String) -> Result<DerivePinKeyResponse> {
let enc = client.internal.get_encryption_settings()?;
let user_key = enc.get_key(&None)?;
Expand All @@ -262,7 +269,6 @@
})
}

#[cfg(feature = "internal")]
pub fn derive_pin_user_key(client: &Client, encrypted_pin: EncString) -> Result<EncString> {
let enc = client.internal.get_encryption_settings()?;
let user_key = enc.get_key(&None)?;
Expand All @@ -276,7 +282,6 @@
derive_pin_protected_user_key(&pin, &login_method, user_key)
}

#[cfg(feature = "internal")]
fn derive_pin_protected_user_key(
pin: &str,
login_method: &LoginMethod,
Expand All @@ -296,7 +301,6 @@
Ok(derived_key.encrypt_user_key(user_key)?)
}

#[cfg(feature = "internal")]
pub(super) fn enroll_admin_password_reset(
client: &Client,
public_key: String,
Expand All @@ -314,8 +318,30 @@
)?)
}

#[cfg_attr(feature = "uniffi", derive(uniffi::Record))]
pub struct DeriveKeyConnectorRequest {
/// Encrypted user key, used to validate the master key
pub user_key_encrypted: EncString,

pub password: String,
pub kdf: Kdf,
pub email: String,
}

/// Derive the master key for migrating to the key connector
pub(super) fn derive_key_connector(request: DeriveKeyConnectorRequest) -> Result<String> {
let master_key = MasterKey::derive(&request.password, &request.email, &request.kdf)?;
master_key
.decrypt_user_key(request.user_key_encrypted)
.map_err(|_| "wrong password")?;

Ok(master_key.to_base64())
}

#[cfg(test)]
mod tests {
use std::num::NonZeroU32;

use super::*;
use crate::Client;

Expand Down Expand Up @@ -495,11 +521,8 @@
);
}

#[cfg(feature = "internal")]
#[test]
fn test_enroll_admin_password_reset() {
use std::num::NonZeroU32;

use base64::{engine::general_purpose::STANDARD, Engine};
use bitwarden_crypto::AsymmetricCryptoKey;

Expand Down Expand Up @@ -534,4 +557,20 @@
let expected = enc.get_key(&None).unwrap();
assert_eq!(&decrypted, &expected.to_vec());
}

#[test]
fn test_derive_key_connector() {
let request = DeriveKeyConnectorRequest {
password: "asdfasdfasdf".to_string(),
email: "[email protected]".to_string(),
kdf: Kdf::PBKDF2 {
iterations: NonZeroU32::new(600_000).unwrap(),
},
user_key_encrypted: "2.Q/2PhzcC7GdeiMHhWguYAQ==|GpqzVdr0go0ug5cZh1n+uixeBC3oC90CIe0hd/HWA/pTRDZ8ane4fmsEIcuc8eMKUt55Y2q/fbNzsYu41YTZzzsJUSeqVjT8/iTQtgnNdpo=|dwI+uyvZ1h/iZ03VQ+/wrGEFYVewBUUl/syYgjsNMbE=".parse().unwrap(),
};

let result = derive_key_connector(request).unwrap();

assert_eq!(result, "ySXq1RVLKEaV1eoQE/ui9aFKIvXTl9PAXwp1MljfF50=");
}
}
18 changes: 17 additions & 1 deletion crates/bitwarden-crypto/src/keys/master_key.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
use std::num::NonZeroU32;

use base64::{engine::general_purpose::STANDARD, Engine};
use generic_array::{typenum::U32, GenericArray};
use rand::Rng;
use schemars::JsonSchema;
use serde::{Deserialize, Serialize};

Expand Down Expand Up @@ -64,10 +66,20 @@
pub struct MasterKey(SymmetricCryptoKey);

impl MasterKey {
pub fn new(key: SymmetricCryptoKey) -> MasterKey {
pub fn new(key: SymmetricCryptoKey) -> Self {

Check warning on line 69 in crates/bitwarden-crypto/src/keys/master_key.rs

View check run for this annotation

Codecov / codecov/patch

crates/bitwarden-crypto/src/keys/master_key.rs#L69

Added line #L69 was not covered by tests
Self(key)
}

/// Generate a new random master key. Primarily used for KeyConnector.
pub fn generate(mut rng: impl rand::RngCore) -> Self {
let mut key = Box::pin(GenericArray::<u8, U32>::default());

rng.fill(key.as_mut_slice());

// Master Keys never contains a mac_key.
Self::new(SymmetricCryptoKey { key, mac_key: None })
}

/// Derives a users master key from their password, email and KDF.
///
/// Note: the email is trimmed and converted to lowercase before being used.
Expand Down Expand Up @@ -101,6 +113,10 @@
pub fn decrypt_user_key(&self, user_key: EncString) -> Result<SymmetricCryptoKey> {
decrypt_user_key(&self.0, user_key)
}

pub fn to_base64(&self) -> String {
self.0.to_base64()
}
}

/// Helper function to encrypt a user key with a master or pin key.
Expand Down
Loading
Loading