From 8652d794e5874ba88ab30d1f2f8f89bca7109f75 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garci=CC=81a?= Date: Thu, 3 Oct 2024 16:53:59 +0200 Subject: [PATCH] Introduce mutable context --- crates/bitwarden-core/src/lib.rs | 135 ------------- .../benches/new_encryptable.rs | 4 +- .../bitwarden-crypto/src/service/context.rs | 184 ++++++++++++++---- .../bitwarden-crypto/src/service/key_ref.rs | 11 +- crates/bitwarden-crypto/src/service/mod.rs | 49 ++++- 5 files changed, 198 insertions(+), 185 deletions(-) diff --git a/crates/bitwarden-core/src/lib.rs b/crates/bitwarden-core/src/lib.rs index bafb793da..12b0df3c3 100644 --- a/crates/bitwarden-core/src/lib.rs +++ b/crates/bitwarden-core/src/lib.rs @@ -19,138 +19,3 @@ mod util; pub use bitwarden_crypto::ZeroizingAllocator; pub use client::{Client, ClientSettings, DeviceType}; - -#[allow(warnings)] -#[cfg(test)] -mod testcrypto { - - // TEST IMPL OF THE NEW ENCRYPTABLE/DECRYPTABLE TRAITS - // Note that these never touch the keys at all, they just use the context and key references to - // encrypt/decrypt - - use bitwarden_crypto::{ - key_refs, service::*, AsymmetricCryptoKey, CryptoError, EncString, KeyEncryptable, - SymmetricCryptoKey, - }; - - key_refs! { - #[symmetric] - pub enum MySymmKeyRef { - User, - Organization(uuid::Uuid), - #[local] - Local(&'static str), - } - - #[asymmetric] - pub enum MyAsymmKeyRef { - UserPrivateKey, - #[local] - Local(&'static str), - } - } - - #[derive(Clone)] - struct Cipher { - key: Option, - name: EncString, - } - - #[derive(Clone)] - struct CipherView { - key: Option, - name: String, - } - - impl UsesKey for Cipher { - fn uses_key(&self) -> MySymmKeyRef { - MySymmKeyRef::User - } - } - impl UsesKey for CipherView { - fn uses_key(&self) -> MySymmKeyRef { - MySymmKeyRef::User - } - } - - const CIPHER_KEY: MySymmKeyRef = MySymmKeyRef::Local("cipher_key"); - - impl Encryptable for CipherView { - fn encrypt( - &self, - ctx: &mut CryptoServiceContext, - key: MySymmKeyRef, - ) -> Result { - let cipher_key = match &self.key { - Some(cipher_key) => ctx.decrypt_symmetric_key(key, CIPHER_KEY, cipher_key)?, - None => key, - }; - - Ok(Cipher { - key: self.key.clone(), - name: self.name.as_str().encrypt(ctx, cipher_key)?, - }) - } - } - - impl Decryptable for Cipher { - fn decrypt( - &self, - ctx: &mut CryptoServiceContext, - key: MySymmKeyRef, - ) -> Result { - let cipher_key = match &self.key { - Some(cipher_key) => ctx.decrypt_symmetric_key(key, CIPHER_KEY, cipher_key)?, - None => key, - }; - - Ok(CipherView { - key: self.key.clone(), - name: self.name.decrypt(ctx, cipher_key)?, - }) - } - } - - #[test] - fn test_cipher() { - let user_key = SymmetricCryptoKey::generate(rand::thread_rng()); - - let org_id = uuid::Uuid::parse_str("91b000b6-81ce-47f4-9802-3390e0b895ed").unwrap(); - let org_key = SymmetricCryptoKey::generate(rand::thread_rng()); - - let cipher_key = SymmetricCryptoKey::generate(rand::thread_rng()); - let cipher_key_user_enc = cipher_key.to_vec().encrypt_with_key(&user_key).unwrap(); - let cipher_view = CipherView { - key: Some(cipher_key_user_enc.clone()), - name: "test".to_string(), - }; - - let service: CryptoService = CryptoService::new(); - // Ideally we'd decrypt the keys directly into the service, but that's not implemented yet - #[allow(deprecated)] - { - service.insert_symmetric_key(MySymmKeyRef::User, user_key); - service.insert_symmetric_key(MySymmKeyRef::Organization(org_id), org_key); - } - - let cipher_enc2 = service.encrypt(cipher_view.clone()).unwrap(); - - let cipher_view2 = service.decrypt(&cipher_enc2).unwrap(); - - assert_eq!(cipher_view.name, cipher_view2.name); - - // We can also decrypt a value by tagging it with the key - let text = String::from("test!"); - - let text_enc = service - .encrypt(text.as_str().using_key(MySymmKeyRef::User)) - .unwrap(); - - // And decrypt values in parallel - let mut data = Vec::with_capacity(250); - for _ in 0..data.capacity() { - data.push("hello world, this is an encryption test!".using_key(MySymmKeyRef::User)); - } - service.encrypt_list(&data).unwrap(); - } -} diff --git a/crates/bitwarden-crypto/benches/new_encryptable.rs b/crates/bitwarden-crypto/benches/new_encryptable.rs index 3bd43f75c..b719484f1 100644 --- a/crates/bitwarden-crypto/benches/new_encryptable.rs +++ b/crates/bitwarden-crypto/benches/new_encryptable.rs @@ -146,7 +146,9 @@ impl Encryptable for CipherVi key: MySymmKeyRef, ) -> Result { let cipher_key = match &self.key { - Some(cipher_key) => ctx.decrypt_symmetric_key(key, CIPHER_KEY, cipher_key)?, + Some(cipher_key) => { + ctx.decrypt_symmetric_key_with_symmetric_key(key, CIPHER_KEY, cipher_key)? + } None => key, }; diff --git a/crates/bitwarden-crypto/src/service/context.rs b/crates/bitwarden-crypto/src/service/context.rs index f3e8c8877..81a902164 100644 --- a/crates/bitwarden-crypto/src/service/context.rs +++ b/crates/bitwarden-crypto/src/service/context.rs @@ -1,66 +1,159 @@ -use std::sync::RwLockReadGuard; +use std::sync::{RwLockReadGuard, RwLockWriteGuard}; use rsa::Oaep; use super::Keys; use crate::{ service::{key_store::KeyStore, AsymmetricKeyRef, SymmetricKeyRef}, - AsymmetricCryptoKey, AsymmetricEncString, CryptoError, EncString, SymmetricCryptoKey, + AsymmetricCryptoKey, AsymmetricEncString, CryptoError, EncString, Result, SymmetricCryptoKey, }; -pub struct CryptoServiceContext<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> { - // We hold a RwLock read guard to avoid having any nested - //calls locking it again and potentially causing a deadlock - pub(super) global_keys: RwLockReadGuard<'a, Keys>, +pub trait GlobalAccessMode<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> { + fn get(&self) -> &Keys; + fn get_mut(&mut self) -> Result<&mut Keys>; +} + +pub struct ReadOnlyGlobal<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef>( + pub(super) RwLockReadGuard<'a, Keys>, +); + +impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> + GlobalAccessMode<'a, SymmKeyRef, AsymmKeyRef> for ReadOnlyGlobal<'a, SymmKeyRef, AsymmKeyRef> +{ + fn get(&self) -> &Keys { + &self.0 + } + + fn get_mut(&mut self) -> Result<&mut Keys> { + // TODO: This should be a custom error + Err(crate::CryptoError::MissingKey2( + "Cannot modify in read-only mode".to_string(), + )) + } +} + +pub struct ReadWriteGlobal<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef>( + pub(super) RwLockWriteGuard<'a, Keys>, +); + +impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> + GlobalAccessMode<'a, SymmKeyRef, AsymmKeyRef> for ReadWriteGlobal<'a, SymmKeyRef, AsymmKeyRef> +{ + fn get(&self) -> &Keys { + &self.0 + } + + fn get_mut(&mut self) -> Result<&mut Keys> { + Ok(&mut self.0) + } +} + +pub struct CryptoServiceContext< + 'a, + SymmKeyRef: SymmetricKeyRef, + AsymmKeyRef: AsymmetricKeyRef, + AccessMode: GlobalAccessMode<'a, SymmKeyRef, AsymmKeyRef> = ReadOnlyGlobal< + 'a, + SymmKeyRef, + AsymmKeyRef, + >, +> { + pub(super) global: AccessMode, pub(super) local_symmetric_keys: Box>, pub(super) local_asymmetric_keys: Box>, + + pub(super) _phantom: std::marker::PhantomData<&'a ()>, } -impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> - CryptoServiceContext<'a, SymmKeyRef, AsymmKeyRef> +impl< + 'a, + SymmKeyRef: SymmetricKeyRef, + AsymmKeyRef: AsymmetricKeyRef, + AccessMode: GlobalAccessMode<'a, SymmKeyRef, AsymmKeyRef>, + > CryptoServiceContext<'a, SymmKeyRef, AsymmKeyRef, AccessMode> { pub fn clear(&mut self) { + // Clear global keys if we have write access + if let Ok(keys) = self.global.get_mut() { + keys.symmetric_keys.clear(); + keys.asymmetric_keys.clear(); + } + self.local_symmetric_keys.clear(); self.local_asymmetric_keys.clear(); } - pub fn decrypt_symmetric_key( + /// TODO: All these encrypt x key with x key look like they need to be made generic, + /// but I haven't found the best way to do that yet. + + pub fn decrypt_symmetric_key_with_symmetric_key( &mut self, encryption_key: SymmKeyRef, new_key_ref: SymmKeyRef, encrypted_key: &EncString, - ) -> Result { + ) -> Result { let mut new_key_material = self.decrypt_data_with_symmetric_key(encryption_key, encrypted_key)?; - let new_key = SymmetricCryptoKey::try_from(new_key_material.as_mut_slice())?; - self.local_symmetric_keys.insert(new_key_ref, new_key); + self.set_symmetric_key( + new_key_ref, + SymmetricCryptoKey::try_from(new_key_material.as_mut_slice())?, + )?; // Returning the new key reference for convenience Ok(new_key_ref) } - pub fn encrypt_symmetric_key( + pub fn encrypt_symmetric_key_with_symmetric_key( &self, encryption_key: SymmKeyRef, key_to_encrypt: SymmKeyRef, - ) -> Result { + ) -> Result { let key_to_encrypt = self.get_symmetric_key(key_to_encrypt)?; self.encrypt_data_with_symmetric_key(encryption_key, &key_to_encrypt.to_vec()) } + pub fn decrypt_symmetric_key_with_asymmetric_key( + &mut self, + encryption_key: AsymmKeyRef, + new_key_ref: SymmKeyRef, + encrypted_key: &AsymmetricEncString, + ) -> Result { + let mut new_key_material = + self.decrypt_data_with_asymmetric_key(encryption_key, encrypted_key)?; + + self.set_symmetric_key( + new_key_ref, + SymmetricCryptoKey::try_from(new_key_material.as_mut_slice())?, + )?; + + // Returning the new key reference for convenience + Ok(new_key_ref) + } + + pub fn encrypt_symmetric_key_with_asymmetric_key( + &self, + encryption_key: AsymmKeyRef, + key_to_encrypt: SymmKeyRef, + ) -> Result { + let key_to_encrypt = self.get_symmetric_key(key_to_encrypt)?; + self.encrypt_data_with_asymmetric_key(encryption_key, &key_to_encrypt.to_vec()) + } + pub fn decrypt_asymmetric_key( &mut self, encryption_key: AsymmKeyRef, new_key_ref: AsymmKeyRef, encrypted_key: &AsymmetricEncString, - ) -> Result { + ) -> Result { let new_key_material = self.decrypt_data_with_asymmetric_key(encryption_key, encrypted_key)?; - let new_key = AsymmetricCryptoKey::from_der(&new_key_material)?; - self.local_asymmetric_keys.insert(new_key_ref, new_key); + self.set_asymmetric_key( + new_key_ref, + AsymmetricCryptoKey::from_der(&new_key_material)?, + )?; // Returning the new key reference for convenience Ok(new_key_ref) @@ -70,7 +163,7 @@ impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> &self, encryption_key: AsymmKeyRef, key_to_encrypt: AsymmKeyRef, - ) -> Result { + ) -> Result { let encryption_key = self.get_asymmetric_key(encryption_key)?; let key_to_encrypt = self.get_asymmetric_key(key_to_encrypt)?; @@ -80,11 +173,16 @@ impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> ) } + pub fn has_symmetric_key(&self, key_ref: SymmKeyRef) -> bool { + self.get_symmetric_key(key_ref).is_ok() + } + + pub fn has_asymmetric_key(&self, key_ref: AsymmKeyRef) -> bool { + self.get_asymmetric_key(key_ref).is_ok() + } + #[deprecated(note = "This function should never be used outside this crate")] - pub fn dangerous_get_symmetric_key( - &self, - key_ref: SymmKeyRef, - ) -> Result<&SymmetricCryptoKey, crate::CryptoError> { + pub fn dangerous_get_symmetric_key(&self, key_ref: SymmKeyRef) -> Result<&SymmetricCryptoKey> { self.get_symmetric_key(key_ref) } @@ -92,39 +190,51 @@ impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> pub fn dangerous_get_asymmetric_key( &self, key_ref: AsymmKeyRef, - ) -> Result<&AsymmetricCryptoKey, crate::CryptoError> { + ) -> Result<&AsymmetricCryptoKey> { self.get_asymmetric_key(key_ref) } - fn get_symmetric_key( - &self, - key_ref: SymmKeyRef, - ) -> Result<&SymmetricCryptoKey, crate::CryptoError> { + fn get_symmetric_key(&self, key_ref: SymmKeyRef) -> Result<&SymmetricCryptoKey> { if key_ref.is_local() { self.local_symmetric_keys.get(key_ref) } else { - self.global_keys.symmetric_keys.get(key_ref) + self.global.get().symmetric_keys.get(key_ref) } .ok_or_else(|| crate::CryptoError::MissingKey2(format!("{key_ref:?}"))) } - fn get_asymmetric_key( - &self, - key_ref: AsymmKeyRef, - ) -> Result<&AsymmetricCryptoKey, crate::CryptoError> { + fn get_asymmetric_key(&self, key_ref: AsymmKeyRef) -> Result<&AsymmetricCryptoKey> { if key_ref.is_local() { self.local_asymmetric_keys.get(key_ref) } else { - self.global_keys.asymmetric_keys.get(key_ref) + self.global.get().asymmetric_keys.get(key_ref) } .ok_or_else(|| crate::CryptoError::MissingKey2(format!("{key_ref:?}"))) } + fn set_symmetric_key(&mut self, key_ref: SymmKeyRef, key: SymmetricCryptoKey) -> Result<()> { + if key_ref.is_local() { + self.local_symmetric_keys.insert(key_ref, key); + } else { + self.global.get_mut()?.symmetric_keys.insert(key_ref, key); + } + Ok(()) + } + + fn set_asymmetric_key(&mut self, key_ref: AsymmKeyRef, key: AsymmetricCryptoKey) -> Result<()> { + if key_ref.is_local() { + self.local_asymmetric_keys.insert(key_ref, key); + } else { + self.global.get_mut()?.asymmetric_keys.insert(key_ref, key); + } + Ok(()) + } + pub(super) fn decrypt_data_with_symmetric_key( &self, key: SymmKeyRef, data: &EncString, - ) -> Result, crate::CryptoError> { + ) -> Result> { let key = self.get_symmetric_key(key)?; match data { @@ -155,7 +265,7 @@ impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> &self, key: SymmKeyRef, data: &[u8], - ) -> Result { + ) -> Result { let key = self.get_symmetric_key(key)?; EncString::encrypt_aes256_hmac( data, @@ -168,7 +278,7 @@ impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> &self, key: AsymmKeyRef, data: &AsymmetricEncString, - ) -> Result, crate::CryptoError> { + ) -> Result> { let key = self.get_asymmetric_key(key)?; use AsymmetricEncString::*; @@ -191,7 +301,7 @@ impl<'a, SymmKeyRef: SymmetricKeyRef, AsymmKeyRef: AsymmetricKeyRef> &self, key: AsymmKeyRef, data: &[u8], - ) -> Result { + ) -> Result { let key = self.get_asymmetric_key(key)?; AsymmetricEncString::encrypt_rsa2048_oaep_sha1(data, key) } diff --git a/crates/bitwarden-crypto/src/service/key_ref.rs b/crates/bitwarden-crypto/src/service/key_ref.rs index 5c4cd307b..a26b6d895 100644 --- a/crates/bitwarden-crypto/src/service/key_ref.rs +++ b/crates/bitwarden-crypto/src/service/key_ref.rs @@ -10,16 +10,17 @@ pub mod __internal { use crate::CryptoKey; - /// This trait represents a key reference that can be used to identify cryptographic keys in the key - /// store. It is used to abstract over the different types of keys that can be used in the system, - /// an end user would not implement this trait directly, and would instead use the `SymmetricKeyRef` - /// and `AsymmetricKeyRef` traits. + /// This trait represents a key reference that can be used to identify cryptographic keys in the + /// key store. It is used to abstract over the different types of keys that can be used in + /// the system, an end user would not implement this trait directly, and would instead use + /// the `SymmetricKeyRef` and `AsymmetricKeyRef` traits. pub trait KeyRef: Debug + Clone + Copy + Hash + Eq + PartialEq + Ord + PartialOrd + Send + Sync + 'static { type KeyValue: CryptoKey + Send + Sync + ZeroizeOnDrop; - /// Returns whether the key is local to the current context or shared globally by the service. + /// Returns whether the key is local to the current context or shared globally by the + /// service. fn is_local(&self) -> bool; } } diff --git a/crates/bitwarden-crypto/src/service/mod.rs b/crates/bitwarden-crypto/src/service/mod.rs index 793935b94..b54f879d1 100644 --- a/crates/bitwarden-crypto/src/service/mod.rs +++ b/crates/bitwarden-crypto/src/service/mod.rs @@ -7,7 +7,8 @@ mod encryptable; pub mod key_ref; mod key_store; -pub use context::CryptoServiceContext; +use context::ReadWriteGlobal; +pub use context::{CryptoServiceContext, ReadOnlyGlobal}; pub use encryptable::{Decryptable, Encryptable, UsesKey, UsingKey, UsingKeyExt}; use key_ref::{AsymmetricKeyRef, KeyRef, SymmetricKeyRef}; pub use key_store::create_key_store; @@ -28,9 +29,7 @@ impl std::fmt::Debug } } -// This is just a wrapper around the keys so we only deal with one RwLock -pub(crate) struct Keys -{ +pub struct Keys { symmetric_keys: Box>, asymmetric_keys: Box>, } @@ -88,13 +87,49 @@ impl .insert(key_ref, key); } - /// Initiate an encryption/decryption context. This is an advanced API, use with care. - /// Prefer to instead use `encrypt`/`decrypt`/`encrypt_list`/`decrypt_list` methods. + /// Initiate an encryption/decryption context. This context will have read only access to the + /// global keys, and will have its own local key stores with read/write access. This + /// context-local store will be cleared up when the context is dropped. + /// + /// This is an advanced API, use with care. Prefer to instead use + /// `encrypt`/`decrypt`/`encrypt_list`/`decrypt_list` methods. + /// + /// One of the pitfalls of the current implementations is that keys stored in the context-local + /// store only get cleared automatically when dropped, and not between operations. This + /// means that if you are using the same context for multiple operations, you may want to + /// clear it manually between them. pub fn context(&'_ self) -> CryptoServiceContext<'_, SymmKeyRef, AsymmKeyRef> { CryptoServiceContext { - global_keys: self.key_stores.read().expect("RwLock is poisoned"), + global: ReadOnlyGlobal(self.key_stores.read().expect("RwLock is poisoned")), + local_symmetric_keys: create_key_store(), + local_asymmetric_keys: create_key_store(), + _phantom: std::marker::PhantomData, + } + } + + /// Initiate an encryption/decryption context. This context will have MUTABLE access to the + /// global keys, and will have its own local key stores with read/write access. This + /// context-local store will be cleared up when the context is dropped. + /// + /// This is an advanced API, use with care and ONLY when needing to modify the global keys. + /// + /// The same pitfalls as `context` apply here, but with the added risk of accidentally + /// modifying the global keys and leaving the service in an inconsistent state. + /// + /// TODO: We should work towards making this pub(crate) + pub fn context_mut( + &'_ self, + ) -> CryptoServiceContext< + '_, + SymmKeyRef, + AsymmKeyRef, + ReadWriteGlobal<'_, SymmKeyRef, AsymmKeyRef>, + > { + CryptoServiceContext { + global: ReadWriteGlobal(self.key_stores.write().expect("RwLock is poisoned")), local_symmetric_keys: create_key_store(), local_asymmetric_keys: create_key_store(), + _phantom: std::marker::PhantomData, } }