Skip to content

[PM-26177] Add methods to create rotateable key set (2/3) #486

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

[PM-26177] Add methods to create rotateable key set (2/3) #486

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a79946b
Add method to create rotateable key set
iinuwa Sep 30, 2025
e74f17c
Fix tests and formatting
iinuwa Oct 1, 2025
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
129 changes: 128 additions & 1 deletion crates/bitwarden-core/src/key_management/crypto.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -586,6 +586,66 @@ pub(super) fn make_key_pair(user_key: B64) -> Result<MakeKeyPairResponse, Crypto
})
}

/// A set of keys where a [UserKey] is protected by an encrypted public/private key-pair.
/// The `UserKey` is used to encrypt/decrypt data, while the public/private key-pair is
/// used to rotate the `UserKey`.
///
/// The `PrivateKey` is protected by an `ExternalKey`, such as a `DeviceKey`, or `PrfKey`,
/// and the `PublicKey` is protected by the `UserKey`. This setup allows:
///
/// - Access to `UserKey` by knowing the `ExternalKey`
/// - Rotation to a `NewUserKey` by knowing the current `UserKey`, without needing access to the
/// `ExternalKey`
#[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 RotateableKeySet {
Copy link
Contributor

Choose a reason for hiding this comment

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

A thought on the API (open for discussion, as pseudo code):

  1. Move RotateableKeySet to crypto crate, then:
impl RotateableKeySet<...> {
  pub fn new(externalKey: SymmetricCryptoKey, key_slot_to_wrap: KeyId) -> Self;
  pub fn unlock(externalKey: SymmetricCryptoKey, key_slot_to_unwrap_to: KeyId) -> Result<KeyId, ...>;
}

That way we would not need to make the functions from the first PR in the set public, and also allow arbitrary keys to be wrapped, not just the user key.

Copy link
Author

@iinuwa iinuwa Oct 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, I agree that's more functional.

Some questions:

  • These methods would need to take &KeyStoreContext (or moved to impl KeyStoreContext and take &self) in order for the KeyId to mean anything, right?
  • Do we prefer the term "external key" over "wrapping key" in this context?
  • Does it make sense to be generic over KeyId and not use SymmetricKeyId directly? Can we provide the same guarantees with asymmetric keys?
e, d = en-/decapsulation key pair
s = symmetric external key
k = key to wrap (symmetric key, or private key)

KeySet = {
  SymmetricWrap(s, d),
  Wrap(k, e),  // symmetric or asymmetric, depending on type of `k`
  Encaps(e, k),
}

In order for Wrap(k, e) to not be tampered by the server, Wrap must provide integrity and authentication, which RSA alone does not, right?

To make sure I understand, and taking in input from the other PRs, we'd want to go with something like (just roughed out):

// caller wrap
let store = client.internal.get_key_store();
let ctx = store.context();
let external_key_id = SymmetricKeyId::Local("external_key");
ctx.derive_symmetric_key_from_prf(prf, external_key_id)?;

let keyset = ctx.create_rotateable_key_set(external_key_id, SymmetricKeyId::User)?;
// or
// let keyset = RotateableKeySet::new(&ctx, external_key_id, SymmetricKeyId::User)?;
// caller unlock
let keyset: RotateableKeySet = ...;
let store = client.internal.get_key_store();
let ctx = store.context();

let external_key_id = SymmetricKeyId::Local("external_key");
ctx.derive_symmetric_key_from_prf(prf, external_key_id)?;

let key_to_unwrap = SymmetricKeyId::Local("key_to_unwrap");
ctx.unlock_rotateable_key_set(external_key_id, keyset, key_to_unwrap);
// or
// keyset.unlock(&ctx, external_key_id, key_to_unwrap);
// implementation

impl KeyStoreContext<...> {
    fn create_rotateable_key_set(external_key_id: SymmetricKeyId, key_to_wrap: KeyId) -> Result<RotateableKeySet> {
        // use ephemeral keys to create key set, don't store in context as key IDs
        let (encapsulation_key, decapsulation_key) = rsa::make_key_pair()
        let wrapped_decapsulation_key = self.encrypt_data_with_symmetric_key(...)
        let encapsulated_key = UnsignedSharedKey::encapsulate_key_unsigned(self.get_key(key_to_wrap));
        // I only how to do this for symmetric keys, not sure about asymmetric
        let wrapped_encapsulation_key = self.encrypt_data_with_symmetric_key(key_to_wrap, encapsulation_key);
        return RotateableKeySet {
            // ...
        }
    }
}

/// [UserKey] protected by encapsulation key
encapsulated_user_key: UnsignedSharedKey,
/// Encapsulation key protected by [UserKey]
encrypted_encapsulation_key: EncString,
/// Decapsulation key protected by wrapping key
wrapped_decapsulation_key: EncString,
}

/// Create a set of keys to allow access to the user key via the provided
/// symmetric wrapping key while allowing the user key to be rotated.
#[allow(dead_code)]
fn create_rotateable_key_set(
client: &Client,
wrapping_key: SymmetricCryptoKey,
) -> Result<RotateableKeySet, CryptoClientError> {
let key_store = client.internal.get_key_store();
let mut ctx = key_store.context();
let wrapping_key_id = SymmetricKeyId::Local("wrapping_key");
#[allow(deprecated)]
ctx.set_symmetric_key(wrapping_key_id, wrapping_key)?;

// encapsulate user key
let encapsulation_key_id = AsymmetricKeyId::Local("encapsulation_key");
ctx.make_asymmetric_key(encapsulation_key_id)?;
let encapsulated_user_key =
Copy link
Contributor

Choose a reason for hiding this comment

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

To me, this is unintutive behavior. At least the key_to_wrap should be exposed as a key id parameter here?

Copy link
Author

Choose a reason for hiding this comment

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

yes, that would make more sense. I can change that and update the documentation.

ctx.encapsulate_key_unsigned(encapsulation_key_id, SymmetricKeyId::User)?;

// wrap decapsulation key
let wrapped_decapsulation_key = ctx.wrap_private_key(wrapping_key_id, encapsulation_key_id)?;

// wrap encapsulation key with user key
// Note: Usually, a public key is - by definition - public, so this should not be necessary.
// The specific use-case for this function is to enable rotateable key sets, where
// the "public key" is not public, with the intent of preventing the server from being able
// to overwrite the user key unlocked by the rotateable keyset.
let encrypted_encapsulation_key =
ctx.wrap_public_key(SymmetricKeyId::User, encapsulation_key_id)?;

Ok(RotateableKeySet {
encapsulated_user_key,
encrypted_encapsulation_key,
wrapped_decapsulation_key,
})
}

/// Request for `verify_asymmetric_keys`.
#[derive(Serialize, Deserialize, Debug)]
#[serde(rename_all = "camelCase", deny_unknown_fields)]
Expand Down Expand Up @@ -824,7 +884,7 @@ pub(crate) fn get_v2_rotated_account_keys(
mod tests {
use std::num::NonZeroU32;

use bitwarden_crypto::RsaKeyPair;
use bitwarden_crypto::{AsymmetricPublicCryptoKey, RsaKeyPair};

use super::*;
use crate::{Client, client::internal::UserKeyState};
Expand Down Expand Up @@ -1513,4 +1573,71 @@ mod tests {

assert!(get_v2_rotated_account_keys(&client).is_ok());
}

#[tokio::test]
async fn test_rotateable_key_set() {
let client = Client::new(None);

let original_user_key =
SymmetricCryptoKey::try_from(TEST_VECTOR_USER_KEY_V2_B64.to_string()).unwrap();
initialize_user_crypto(
&client,
InitUserCryptoRequest {
user_id: Some(UserId::new_v4()),
kdf_params: Kdf::PBKDF2 {
iterations: 100_000.try_into().unwrap(),
},
email: "test@bitwarden.com".into(),
private_key: TEST_VECTOR_PRIVATE_KEY_V2.parse().unwrap(),
signing_key: Some(TEST_VECTOR_SIGNING_KEY_V2.parse().unwrap()),
security_state: Some(TEST_VECTOR_SECURITY_STATE_V2.parse().unwrap()),
method: InitUserCryptoMethod::DecryptedKey {
decrypted_user_key: TEST_VECTOR_USER_KEY_V2_B64.to_string(),
},
},
)
.await
.unwrap();
let wrapping_key = SymmetricCryptoKey::make_aes256_cbc_hmac_key();
let keyset = create_rotateable_key_set(&client, wrapping_key.clone()).unwrap();
let decapsulation_key_bytes: Vec<u8> = keyset
.wrapped_decapsulation_key
.decrypt_with_key(&wrapping_key)
.unwrap();

// Rotate keys
let new_user_key = SymmetricCryptoKey::make_aes256_cbc_hmac_key();
#[allow(deprecated)]
client
.internal
.get_key_store()
.context_mut()
.set_symmetric_key(SymmetricKeyId::User, new_user_key.clone())
.unwrap();
let encapsulation_key_bytes: Vec<u8> = keyset
.encrypted_encapsulation_key
.decrypt_with_key(&original_user_key)
.unwrap();
let encapsulation_key =
AsymmetricPublicCryptoKey::from_der(&SpkiPublicKeyBytes::from(encapsulation_key_bytes))
.unwrap();
let new_encapsulated_user_key =
UnsignedSharedKey::encapsulate_key_unsigned(&new_user_key, &encapsulation_key).unwrap();

// User key is accessible from wrapping key
let decapsulation_key =
AsymmetricCryptoKey::from_der(&Pkcs8PrivateKeyBytes::from(decapsulation_key_bytes))
.unwrap();

// Both old and new keys should be accessible
let user_key_unwrapped = keyset
.encapsulated_user_key
.decapsulate_key_unsigned(&decapsulation_key)
.unwrap();
let new_user_key_unwrapped = new_encapsulated_user_key
.decapsulate_key_unsigned(&decapsulation_key)
.unwrap();
assert_eq!(user_key_unwrapped, original_user_key);
assert_eq!(new_user_key, new_user_key_unwrapped);
}
}
Loading