Fix MlsGroup::delete() function to delete encryption keypairs (open…

…mls#1644)

basic_credential/src/lib.rs

@@ -137,6 +137,18 @@ impl SignatureKeyPair {
             .flatten()
     }
 
+    /// Delete a signature key pair from the key store.
+    pub fn delete<T: StorageProvider<CURRENT_VERSION>>(
+        store: &T,
+        public_key: &[u8],
+        signature_scheme: SignatureScheme,
+    ) -> Result<(), T::Error> {
+        let id = StorageId {
+            value: id(public_key, signature_scheme),
+        };
+        store.delete_signature_key_pair(&id)
+    }
+
     /// Get the public key as byte slice.
     pub fn public(&self) -> &[u8] {
         self.public.as_ref()

memory_storage/src/lib.rs

@@ -203,10 +203,7 @@ impl MemoryStorage {
         log::trace!("{}", std::backtrace::Backtrace::capture());
 
         let value: Vec<Vec<u8>> = match values.get(&storage_key) {
-            Some(list_bytes) => {
-                println!("{}", String::from_utf8(list_bytes.to_vec()).unwrap());
-                serde_json::from_slice(list_bytes).unwrap()
-            }
+            Some(list_bytes) => serde_json::from_slice(list_bytes).unwrap(),
             None => vec![],
         };
 
@@ -426,7 +423,9 @@ impl StorageProvider<CURRENT_VERSION> for MemoryStorage {
         let values = self.values.read().unwrap();
         let key = build_key::<CURRENT_VERSION, &GroupId>(TREE_LABEL, group_id);
 
-        let value = values.get(&key).unwrap();
+        let Some(value) = values.get(&key) else {
+            return Ok(None);
+        };
         let value = serde_json::from_slice(value).unwrap();
 
         Ok(value)
@@ -442,7 +441,9 @@ impl StorageProvider<CURRENT_VERSION> for MemoryStorage {
         let values = self.values.read().unwrap();
         let key = build_key::<CURRENT_VERSION, &GroupId>(GROUP_CONTEXT_LABEL, group_id);
 
-        let value = values.get(&key).unwrap();
+        let Some(value) = values.get(&key) else {
+            return Ok(None);
+        };
         let value = serde_json::from_slice(value).unwrap();
 
         Ok(value)
@@ -458,7 +459,9 @@ impl StorageProvider<CURRENT_VERSION> for MemoryStorage {
         let values = self.values.read().unwrap();
         let key = build_key::<CURRENT_VERSION, &GroupId>(INTERIM_TRANSCRIPT_HASH_LABEL, group_id);
 
-        let value = values.get(&key).unwrap();
+        let Some(value) = values.get(&key) else {
+            return Ok(None);
+        };
         let value = serde_json::from_slice(value).unwrap();
 
         Ok(value)
@@ -474,7 +477,9 @@ impl StorageProvider<CURRENT_VERSION> for MemoryStorage {
         let values = self.values.read().unwrap();
         let key = build_key::<CURRENT_VERSION, &GroupId>(CONFIRMATION_TAG_LABEL, group_id);
 
-        let value = values.get(&key).unwrap();
+        let Some(value) = values.get(&key) else {
+            return Ok(None);
+        };
         let value = serde_json::from_slice(value).unwrap();
 
         Ok(value)
@@ -492,7 +497,9 @@ impl StorageProvider<CURRENT_VERSION> for MemoryStorage {
         let key =
             build_key::<CURRENT_VERSION, &SignaturePublicKey>(SIGNATURE_KEY_PAIR_LABEL, public_key);
 
-        let value = values.get(&key).unwrap();
+        let Some(value) = values.get(&key) else {
+            return Ok(None);
+        };
         let value = serde_json::from_slice(value).unwrap();
 
         Ok(value)

openmls/src/group/mls_group/mod.rs

@@ -369,16 +369,23 @@ impl MlsGroup {
         Ok(build())
     }
 
-    /// Remove the persisted state from storage
-    pub fn delete<StorageProvider: crate::storage::StorageProvider>(
+    /// Remove the persisted state of this group from storage. Note that
+    /// signature key material is not managed by OpenMLS and has to be removed
+    /// from the storage provider separately (if desired).
+    pub fn delete<Storage: crate::storage::StorageProvider>(
         &mut self,
-        storage: &StorageProvider,
-    ) -> Result<(), StorageProvider::Error> {
+        storage: &Storage,
+    ) -> Result<(), Storage::Error> {
         self.group.delete(storage)?;
         storage.delete_group_config(self.group_id())?;
         storage.clear_proposal_queue::<GroupId, ProposalRef>(self.group_id())?;
         storage.delete_own_leaf_nodes(self.group_id())?;
         storage.delete_group_state(self.group_id())?;
+        storage.delete_encryption_epoch_key_pairs(
+            self.group_id(),
+            &self.epoch(),
+            self.own_leaf_index().u32(),
+        )?;
 
         self.proposal_store_mut().empty();
 

openmls/src/group/mls_group/tests_and_kats/tests/mls_group.rs

@@ -1,6 +1,8 @@
 use mls_group::tests_and_kats::utils::setup_client;
+use openmls_basic_credential::SignatureKeyPair;
+use openmls_rust_crypto::MemoryStorage;
 use openmls_test::openmls_test;
-use openmls_traits::OpenMlsProvider as _;
+use openmls_traits::{storage::CURRENT_VERSION, OpenMlsProvider as _};
 use tls_codec::{Deserialize, Serialize};
 
 use crate::{
@@ -1260,7 +1262,7 @@ fn builder_pattern() {
 fn update_group_context_with_unknown_extension<Provider: OpenMlsProvider + Default>() {
     let alice_provider = Provider::default();
     let (alice_credential_with_key, _alice_kpb, alice_signer, _alice_pk) =
-        setup_client("Alice", ciphersuite, &alice_provider);
+        setup_client("Alice", ciphersuite, provider);
 
     // === Define the unknown group context extension and initial data ===
     const UNKNOWN_EXTENSION_TYPE: u16 = 0xff11;
@@ -1288,7 +1290,7 @@ fn update_group_context_with_unknown_extension<Provider: OpenMlsProvider + Defau
 
     // === Alice creates a group ===
     let mut alice_group = MlsGroup::new(
-        &alice_provider,
+        provider,
         &alice_signer,
         &mls_group_create_config,
         alice_credential_with_key,
@@ -1989,3 +1991,36 @@ fn join_multiple_groups_last_resort_extension(
         .expect("error creating group from staged join");
     // done :-)
 }
+
+#[openmls_test]
+fn deletion() {
+    let alice_provider = provider;
+    let (alice_credential_with_key, alice_kpb, alice_signer, alice_pk) =
+        setup_client("alice", ciphersuite, provider);
+
+    // delete the kpb from the provider, as we don't need it
+    <MemoryStorage as openmls_traits::storage::StorageProvider<CURRENT_VERSION>>::
+        delete_key_package(alice_provider.storage(),&alice_kpb.key_package().hash_ref(provider.crypto()).unwrap())
+        .unwrap();
+    <MemoryStorage as openmls_traits::storage::StorageProvider<CURRENT_VERSION>>::
+        delete_encryption_key_pair(alice_provider.storage(),alice_kpb.key_package().leaf_node().encryption_key()).unwrap();
+
+    // alice creates MlsGroup
+    let mut alice_group = MlsGroup::builder()
+        .ciphersuite(ciphersuite)
+        .use_ratchet_tree_extension(true)
+        .build(provider, &alice_signer, alice_credential_with_key)
+        .expect("error creating group for alice using builder");
+
+    SignatureKeyPair::delete(
+        alice_provider.storage(),
+        alice_pk.as_slice(),
+        ciphersuite.signature_algorithm(),
+    )
+    .unwrap();
+
+    // alice deletes the group
+    alice_group.delete(alice_provider.storage()).unwrap();
+
+    assert!(alice_provider.storage().values.read().unwrap().is_empty());
+}