Skip to content

[PM-22845] Add account key rotation #313

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
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
23 changes: 21 additions & 2 deletions crates/bitwarden-core/src/key_management/crypto.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@
use base64::{engine::general_purpose::STANDARD, Engine};
use bitwarden_crypto::{
AsymmetricCryptoKey, CoseSerializable, CryptoError, EncString, Encryptable, Kdf,
KeyDecryptable, KeyEncryptable, MasterKey, SignatureAlgorithm, SignedPublicKey, SigningKey,
SymmetricCryptoKey, UnsignedSharedKey, UserKey,
KeyDecryptable, KeyEncryptable, MasterKey, RotateUserKeysResponse, SignatureAlgorithm,
SignedPublicKey, SigningKey, SymmetricCryptoKey, UnsignedSharedKey, UserKey,
};
use bitwarden_error::bitwarden_error;
use schemars::JsonSchema;
Expand Down Expand Up @@ -614,6 +614,25 @@
})
}

/// Gets a set of new wrapped account keys for a user, given a new user key.
///
/// In the current implementation, it just re-encrypts any existing keys. This function expects a
/// user to be a v2 user; that is, they have a signing
pub fn get_v2_rotated_account_keys(
client: &Client,
user_key: String,
) -> Result<RotateUserKeysResponse, CryptoError> {
let key_store = client.internal.get_key_store();
let ctx = key_store.context();

bitwarden_crypto::get_v2_rotated_account_keys(
SymmetricCryptoKey::try_from(user_key)?,
AsymmetricKeyId::UserPrivateKey,
SigningKeyId::UserSigningKey,
&ctx,

Check warning on line 632 in crates/bitwarden-core/src/key_management/crypto.rs

View check run for this annotation

Codecov / codecov/patch

crates/bitwarden-core/src/key_management/crypto.rs#L621-L632

Added lines #L621 - L632 were not covered by tests
)
}

Check warning on line 634 in crates/bitwarden-core/src/key_management/crypto.rs

View check run for this annotation

Codecov / codecov/patch

crates/bitwarden-core/src/key_management/crypto.rs#L634

Added line #L634 was not covered by tests

#[cfg(test)]
mod tests {
use std::num::NonZeroU32;
Expand Down
13 changes: 11 additions & 2 deletions crates/bitwarden-core/src/key_management/crypto_client.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use bitwarden_crypto::CryptoError;
use bitwarden_crypto::{CryptoError, RotateUserKeysResponse};
#[cfg(feature = "internal")]
use bitwarden_crypto::{EncString, UnsignedSharedKey};
#[cfg(feature = "wasm")]
Expand All @@ -18,7 +18,8 @@
};
use crate::{
client::encryption_settings::EncryptionSettingsError,
key_management::crypto::CryptoClientError, Client,
key_management::crypto::{get_v2_rotated_account_keys, CryptoClientError},
Client,
};

/// A client for the crypto operations.
Expand Down Expand Up @@ -69,6 +70,14 @@
) -> Result<MakeUserSigningKeysResponse, CryptoError> {
make_user_signing_keys_for_enrollment(&self.client)
}

/// Creates a rotated set of account keys for the current state
pub fn get_v2_rotated_account_keys(
&self,
user_key: String,
) -> Result<RotateUserKeysResponse, CryptoError> {
get_v2_rotated_account_keys(&self.client, user_key)
}

Check warning on line 80 in crates/bitwarden-core/src/key_management/crypto_client.rs

View check run for this annotation

Codecov / codecov/patch

crates/bitwarden-core/src/key_management/crypto_client.rs#L75-L80

Added lines #L75 - L80 were not covered by tests
}

impl CryptoClient {
Expand Down
49 changes: 49 additions & 0 deletions crates/bitwarden-crypto/src/key_rotation/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
use base64::{engine::general_purpose::STANDARD, Engine};
use schemars::JsonSchema;
use serde::{Deserialize, Serialize};
#[cfg(feature = "wasm")]
use tsify_next::Tsify;

use crate::{
CoseSerializable, CryptoError, EncString, KeyEncryptable, KeyStoreContext, SymmetricCryptoKey,
};

/// Rotated set of account keys
#[derive(Serialize, Deserialize, Debug, JsonSchema)]
#[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 RotateUserKeysResponse {
/// The verifying key
verifying_key: String,
/// Signing key, encrypted with a symmetric key (user key, org key)
signing_key: EncString,
/// The user's public key, signed by the signing key
signed_public_key: String,
// The user's public key, without signature
public_key: String,
// The user's private key, encrypted with the user key
private_key: EncString,
}

/// Re-encrypts the user's keys with the provided symmetric key for a v2 user.
pub fn get_v2_rotated_account_keys<Ids: crate::KeyIds>(
Copy link
Member

Choose a reason for hiding this comment

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

question: Key rotation feels very application bound and not like a low level cryptographic primitive? I think this is further enhanced by bitwarden-core just acting as a wrapper around this function.

I presume this was done to avoid making get_asymetric_key and `get_signing_key public?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes;

As far as I understand we do not want any further new uses that expose keys out of bitwarden-crypto, so this had to go into the crypto crate to achieve that. Is there another pattern we can use here? (Maybe we do allow (non-deprecated) getting wrapped(encrypted) keys out of a context?)

new_user_key: SymmetricCryptoKey,
current_user_private_key_id: Ids::Asymmetric,
current_user_signing_key_id: Ids::Signing,
ctx: &KeyStoreContext<Ids>,
) -> Result<RotateUserKeysResponse, CryptoError> {
let signing_key = ctx.get_signing_key(current_user_signing_key_id)?;
let private_key = ctx.get_asymmetric_key(current_user_private_key_id)?;
let signed_public_key: Vec<u8> = ctx
.make_signed_public_key(current_user_private_key_id, current_user_signing_key_id)?
.into();

Ok(RotateUserKeysResponse {
verifying_key: STANDARD.encode(signing_key.to_verifying_key().to_cose()),
signing_key: signing_key.to_cose().encrypt_with_key(&new_user_key)?,
signed_public_key: STANDARD.encode(&signed_public_key),
public_key: STANDARD.encode(private_key.to_public_key().to_der()?),
private_key: private_key.to_der()?.encrypt_with_key(&new_user_key)?,

Check warning on line 47 in crates/bitwarden-crypto/src/key_rotation/mod.rs

View check run for this annotation

Codecov / codecov/patch

crates/bitwarden-crypto/src/key_rotation/mod.rs#L30-L47

Added lines #L30 - L47 were not covered by tests
})
}

Check warning on line 49 in crates/bitwarden-crypto/src/key_rotation/mod.rs

View check run for this annotation

Codecov / codecov/patch

crates/bitwarden-crypto/src/key_rotation/mod.rs#L49

Added line #L49 was not covered by tests
2 changes: 2 additions & 0 deletions crates/bitwarden-crypto/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ pub use error::CryptoError;
pub(crate) use error::Result;
mod fingerprint;
pub use fingerprint::fingerprint;
mod key_rotation;
pub use key_rotation::*;
mod keys;
pub use keys::*;
mod rsa;
Expand Down
7 changes: 5 additions & 2 deletions crates/bitwarden-crypto/src/store/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,10 @@ impl<Ids: KeyIds> KeyStoreContext<'_, Ids> {
.ok_or_else(|| crate::CryptoError::MissingKeyId(format!("{key_id:?}")))
}

fn get_asymmetric_key(&self, key_id: Ids::Asymmetric) -> Result<&AsymmetricCryptoKey> {
pub(crate) fn get_asymmetric_key(
&self,
key_id: Ids::Asymmetric,
) -> Result<&AsymmetricCryptoKey> {
if key_id.is_local() {
self.local_asymmetric_keys.get(key_id)
} else {
Expand All @@ -344,7 +347,7 @@ impl<Ids: KeyIds> KeyStoreContext<'_, Ids> {
.ok_or_else(|| crate::CryptoError::MissingKeyId(format!("{key_id:?}")))
}

fn get_signing_key(&self, key_id: Ids::Signing) -> Result<&SigningKey> {
pub(crate) fn get_signing_key(&self, key_id: Ids::Signing) -> Result<&SigningKey> {
if key_id.is_local() {
self.local_signing_keys.get(key_id)
} else {
Expand Down
Loading