Skip to content

[PM-26354] Add methods to un-/wrap asymmetric keys in keystore context (1/3) #485

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

[PM-26354] Add methods to un-/wrap asymmetric keys in keystore context (1/3) #485

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
161 changes: 156 additions & 5 deletions crates/bitwarden-crypto/src/store/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,11 @@ use zeroize::Zeroizing;

use super::KeyStoreInner;
use crate::{
AsymmetricCryptoKey, BitwardenLegacyKeyBytes, ContentFormat, CryptoError, EncString, KeyId,
KeyIds, PublicKeyEncryptionAlgorithm, Result, RotatedUserKeys, Signature, SignatureAlgorithm,
SignedObject, SignedPublicKey, SignedPublicKeyMessage, SigningKey, SymmetricCryptoKey,
UnsignedSharedKey, derive_shareable_key, error::UnsupportedOperationError, signing,
store::backend::StoreBackend,
AsymmetricCryptoKey, AsymmetricPublicCryptoKey, BitwardenLegacyKeyBytes, ContentFormat,
CryptoError, EncString, KeyId, KeyIds, Pkcs8PrivateKeyBytes, PublicKeyEncryptionAlgorithm,
Result, RotatedUserKeys, Signature, SignatureAlgorithm, SignedObject, SignedPublicKey,
SignedPublicKeyMessage, SigningKey, SpkiPublicKeyBytes, SymmetricCryptoKey, UnsignedSharedKey,
derive_shareable_key, error::UnsupportedOperationError, signing, store::backend::StoreBackend,
};

/// The context of a crypto operation using [super::KeyStore]
Expand Down Expand Up @@ -241,6 +241,97 @@ impl<Ids: KeyIds> KeyStoreContext<'_, Ids> {
}
}

/// Decrypt an asymmetric private key into the context by using an already existing symmetric
/// key
///
/// # Arguments
///
/// * `wrapping_key` - The key id used to decrypt the `wrapped_key`. It must already exist in
/// the context
/// * `new_key_id` - The key id where the decrypted key will be stored. If it already exists, it
/// will be overwritten
/// * `wrapped_key` - The key to decrypt
pub fn unwrap_private_key(
&mut self,
wrapping_key: Ids::Symmetric,
new_key_id: Ids::Asymmetric,
wrapped_key: &EncString,
) -> Result<Ids::Asymmetric> {
let bytes = Pkcs8PrivateKeyBytes::from(
self.decrypt_data_with_symmetric_key(wrapping_key, wrapped_key)?,
);
let key = AsymmetricCryptoKey::from_der(&bytes)?;

#[allow(deprecated)]
self.set_asymmetric_key(new_key_id, key)?;

// Returning the new key identifier for convenience
Ok(new_key_id)
}

/// Encrypt and return an asymmetric private key from the context using an already existing
/// symmetric key
///
/// # Arguments
///
/// * `wrapping_key` - The key id used to wrap (encrypt) the `key_to_wrap`. It must already
/// exist in the context
/// * `key_to_wrap` - The key id to wrap. It must already exist in the context
pub fn wrap_private_key(
&self,
wrapping_key: Ids::Symmetric,
key_to_wrap: Ids::Asymmetric,
) -> Result<EncString> {
let key_to_wrap_instance = self.get_asymmetric_key(key_to_wrap)?;
self.encrypt_data_with_symmetric_key(
wrapping_key,
key_to_wrap_instance.to_der()?.as_ref(),
ContentFormat::Pkcs8PrivateKey,
)
}

/// Decrypt an asymmetric public key into the context using an already existing symmetric key
///
/// # Arguments
///
/// * `wrapping_key` - The key id used to decrypt the `wrapped_key`. It must already exist in
/// the context
/// * `wrapped_key` - The key to decrypt
pub fn unwrap_public_key(
Copy link
Contributor

Choose a reason for hiding this comment

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

Imo this is not a public API that we want to offer to teams. We should either deprecate it with a note that this is only to be used for rotateable key sets, or we should move the rotateable keyset in the follow-up PR into the crypto crate and remove this method (or make it pub(crate).

The same applies for wrap_public_key.

Copy link
Author

Choose a reason for hiding this comment

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

We probably don't even need to implement things on the key context then and can go with the keys directly as they are ephemeral for interacting with the key set anyways and shouldn't be held in the key context longer than needed.

I had a question about that: does the context received from store.context() live as long as the store or as long as the reference that is returned?

I don't think wrapping public keys should be a supported public API, and other teams (aside from what's necessary for the rotateable keyset) should not use it.

Any thoughts on moving the rotateable key set to the crypto crate entirely?

That's fine by me! I wasn't sure about where ownership between bitwarden-crypto and bitwarden_core::key_management lies, so I put the methods closest to where they would be used. I initially implemented the rotateable key set without the keystore, but I had to use a bunch of things marked as deprecated. Are those just intended to mark dangerous/internal-but-has-to-be-pub methods?

&mut self,
wrapping_key: Ids::Symmetric,
wrapped_key: &EncString,
) -> Result<AsymmetricPublicCryptoKey> {
let bytes = SpkiPublicKeyBytes::from(
self.decrypt_data_with_symmetric_key(wrapping_key, wrapped_key)?,
);
let key = AsymmetricPublicCryptoKey::from_der(&bytes)?;

// Returning the public key because there's not a KeyId type to just store a public key.
Ok(key)
}

/// Encrypt and return an asymmetric public key from the context using an already existing
/// symmetric key
///
/// # Arguments
///
/// * `wrapping_key` - The key id used to wrap (encrypt) the `key_to_wrap`. It must already
/// exist in the context
/// * `key_to_wrap` - The key id to wrap. It must already exist in the context
pub fn wrap_public_key(
&self,
wrapping_key: Ids::Symmetric,
key_to_wrap: Ids::Asymmetric,
) -> Result<EncString> {
let public_key_to_wrap_instance = self.get_asymmetric_key(key_to_wrap)?.to_public_key();
self.encrypt_data_with_symmetric_key(
wrapping_key,
public_key_to_wrap_instance.to_der()?.as_ref(),
ContentFormat::SPKIPublicKeyDer,
)
}

/// Decapsulate a symmetric key into the context by using an already existing asymmetric key
///
/// # Arguments
Expand Down Expand Up @@ -705,6 +796,66 @@ mod tests {
assert_eq!(unwrapped_key_4, key_xchacha_4_id);
}

#[test]
fn test_asymmetric_wrap_unwrap() {
let store: KeyStore<TestIds> = KeyStore::default();
let mut ctx = store.context_mut();

// AES key
let key_aes_1_id = TestSymmKey::A(2);
let key_aes_1 = SymmetricCryptoKey::make_aes256_cbc_hmac_key();
ctx.set_symmetric_key(key_aes_1_id, key_aes_1.clone())
.unwrap();
// XChaCha20-Poly1305 key
let key_xchacha_2_id = TestSymmKey::A(3);
let key_xchacha_2 = SymmetricCryptoKey::make_xchacha20_poly1305_key();
ctx.set_symmetric_key(key_xchacha_2_id, key_xchacha_2.clone())
.unwrap();
// RSA OAEP SHA-1 keys
let key_rsa_3_id = TestAsymmKey::A(3);
let key_rsa_3 = AsymmetricCryptoKey::make(PublicKeyEncryptionAlgorithm::RsaOaepSha1);
ctx.set_asymmetric_key(key_rsa_3_id, key_rsa_3.clone())
.unwrap();
let key_rsa_4_id = TestAsymmKey::A(4);
let key_rsa_4 = AsymmetricCryptoKey::make(PublicKeyEncryptionAlgorithm::RsaOaepSha1);
ctx.set_asymmetric_key(key_rsa_4_id, key_rsa_4.clone())
.unwrap();

// Wrap the keys
let wrapped_priv_key_1_3 = ctx.wrap_private_key(key_aes_1_id, key_rsa_3_id).unwrap();
let wrapped_pub_key_1_3 = ctx.wrap_public_key(key_aes_1_id, key_rsa_3_id).unwrap();
let wrapped_priv_key_2_4 = ctx
.wrap_private_key(key_xchacha_2_id, key_rsa_4_id)
.unwrap();
let wrapped_pub_key_2_4 = ctx.wrap_public_key(key_xchacha_2_id, key_rsa_4_id).unwrap();

// Unwrap the keys
let unwrapped_priv_key_3 = ctx
.unwrap_private_key(key_aes_1_id, key_rsa_3_id, &wrapped_priv_key_1_3)
.unwrap();
let unwrapped_pub_key_3 = ctx
.unwrap_public_key(key_aes_1_id, &wrapped_pub_key_1_3)
.unwrap();
let unwrapped_priv_key_4 = ctx
.unwrap_private_key(key_xchacha_2_id, key_rsa_4_id, &wrapped_priv_key_2_4)
.unwrap();
let unwrapped_pub_key_4 = ctx
.unwrap_public_key(key_xchacha_2_id, &wrapped_pub_key_2_4)
.unwrap();

// Assert that the unwrapped keys are the same as the original keys
assert_eq!(unwrapped_priv_key_3, key_rsa_3_id);
assert_eq!(
unwrapped_pub_key_3.to_der().unwrap(),
key_rsa_3.to_public_key().to_der().unwrap()
);
assert_eq!(unwrapped_priv_key_4, key_rsa_4_id);
assert_eq!(
unwrapped_pub_key_4.to_der().unwrap(),
key_rsa_4.to_public_key().to_der().unwrap()
);
}

#[test]
fn test_signing() {
let store: KeyStore<TestIds> = KeyStore::default();
Expand Down
Loading