From baf4b0d123215c07f6eced9226ff4056d8eee4f6 Mon Sep 17 00:00:00 2001 From: mulmarta Date: Thu, 2 May 2024 11:58:33 +0200 Subject: [PATCH 1/7] Support receiving own proposals without an error --- mls-rs-uniffi/src/lib.rs | 3 + mls-rs/src/external_client/group.rs | 16 ++-- mls-rs/src/group/commit.rs | 41 ++-------- mls-rs/src/group/external_commit.rs | 4 +- mls-rs/src/group/message_hash.rs | 37 +++++++++ mls-rs/src/group/message_processor.rs | 2 + mls-rs/src/group/message_verifier.rs | 36 +-------- mls-rs/src/group/mod.rs | 87 +++++++++++++++++++-- mls-rs/src/group/proposal_cache.rs | 49 +++++++++++- mls-rs/src/group/snapshot.rs | 13 ++- mls-rs/test_harness_integration/src/main.rs | 13 +-- 11 files changed, 199 insertions(+), 102 deletions(-) create mode 100644 mls-rs/src/group/message_hash.rs diff --git a/mls-rs-uniffi/src/lib.rs b/mls-rs-uniffi/src/lib.rs index 4dc45e1d..193f0102 100644 --- a/mls-rs-uniffi/src/lib.rs +++ b/mls-rs-uniffi/src/lib.rs @@ -271,6 +271,8 @@ pub enum ReceivedMessage { proposal: Arc, }, + /// A proposal previously sent by this member was received. + OwnProposal, /// Validated GroupInfo object. GroupInfo, /// Validated welcome message. @@ -771,6 +773,7 @@ impl Group { let proposal = Arc::new(proposal_message.proposal.into()); Ok(ReceivedMessage::ReceivedProposal { sender, proposal }) } + group::ReceivedMessage::OwnProposal => Ok(ReceivedMessage::OwnProposal), // TODO: group::ReceivedMessage::GroupInfo does not have any // public methods (unless the "ffi" Cargo feature is set). // So perhaps we don't need it? diff --git a/mls-rs/src/external_client/group.rs b/mls-rs/src/external_client/group.rs index a5a7977f..8248240b 100644 --- a/mls-rs/src/external_client/group.rs +++ b/mls-rs/src/external_client/group.rs @@ -450,11 +450,8 @@ impl ExternalGroup { ) .await?; - self.state.proposals.insert( - ProposalRef::from_content(&self.cipher_suite_provider, &auth_content).await?, - proposal, - sender, - ); + let proposal_ref = + ProposalRef::from_content(&self.cipher_suite_provider, &auth_content).await?; let plaintext = PublicMessage { content: auth_content.content, @@ -462,10 +459,14 @@ impl ExternalGroup { membership_tag: None, }; - Ok(MlsMessage::new( + let message = MlsMessage::new( self.group_context().version(), MlsMessagePayload::Plain(plaintext), - )) + ); + + self.state.proposals.insert(proposal_ref, proposal, sender); + + Ok(message) } /// Delete all sent and received proposals cached for commit. @@ -582,7 +583,6 @@ where &self.cipher_suite_provider, message, None, - None, &self.state, ) .await?; diff --git a/mls-rs/src/group/commit.rs b/mls-rs/src/group/commit.rs index c201057c..093dd979 100644 --- a/mls-rs/src/group/commit.rs +++ b/mls-rs/src/group/commit.rs @@ -4,12 +4,9 @@ use alloc::vec; use alloc::vec::Vec; -use core::fmt::{self, Debug}; +use core::fmt::Debug; use mls_rs_codec::{MlsDecode, MlsEncode, MlsSize}; -use mls_rs_core::{ - crypto::{CipherSuiteProvider, SignatureSecretKey}, - error::IntoAnyError, -}; +use mls_rs_core::{crypto::SignatureSecretKey, error::IntoAnyError}; use crate::{ cipher_suite::CipherSuite, @@ -43,6 +40,7 @@ use super::{ confirmation_tag::ConfirmationTag, framing::{Content, MlsMessage, MlsMessagePayload, Sender}, key_schedule::{KeySchedule, WelcomeSecret}, + message_hash::MessageHash, message_processor::{path_update_required, MessageProcessor}, message_signature::AuthenticatedContent, mls_rules::CommitDirection, @@ -71,36 +69,7 @@ pub(super) struct CommitGeneration { pub content: AuthenticatedContent, pub pending_private_tree: TreeKemPrivate, pub pending_commit_secret: PathSecret, - pub commit_message_hash: CommitHash, -} - -#[derive(Clone, PartialEq, MlsEncode, MlsDecode, MlsSize)] -#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] -pub(crate) struct CommitHash( - #[mls_codec(with = "mls_rs_codec::byte_vec")] - #[cfg_attr(feature = "serde", serde(with = "mls_rs_core::vec_serde"))] - Vec, -); - -impl Debug for CommitHash { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - mls_rs_core::debug::pretty_bytes(&self.0) - .named("CommitHash") - .fmt(f) - } -} - -impl CommitHash { - #[cfg_attr(not(mls_build_async), maybe_async::must_be_sync)] - pub(crate) async fn compute( - cs: &CS, - commit: &MlsMessage, - ) -> Result { - cs.hash(&commit.mls_encode_to_vec()?) - .await - .map_err(|e| MlsError::CryptoProviderError(e.into_any_error())) - .map(Self) - } + pub commit_message_hash: MessageHash, } #[cfg_attr( @@ -760,7 +729,7 @@ where content: auth_content, pending_private_tree: provisional_private_tree, pending_commit_secret: commit_secret, - commit_message_hash: CommitHash::compute(&self.cipher_suite_provider, &commit_message) + commit_message_hash: MessageHash::compute(&self.cipher_suite_provider, &commit_message) .await?, }; diff --git a/mls-rs/src/group/external_commit.rs b/mls-rs/src/group/external_commit.rs index 34b10427..a39767b0 100644 --- a/mls-rs/src/group/external_commit.rs +++ b/mls-rs/src/group/external_commit.rs @@ -233,9 +233,7 @@ impl ExternalCommitBuilder { }; let auth_content = AuthenticatedContent::from(plaintext.clone()); - - verify_plaintext_authentication(&cipher_suite, plaintext, None, None, &group.state) - .await?; + verify_plaintext_authentication(&cipher_suite, plaintext, None, &group.state).await?; group .process_event_or_content(EventOrContent::Content(auth_content), true, None) diff --git a/mls-rs/src/group/message_hash.rs b/mls-rs/src/group/message_hash.rs new file mode 100644 index 00000000..64dff244 --- /dev/null +++ b/mls-rs/src/group/message_hash.rs @@ -0,0 +1,37 @@ +use alloc::vec::Vec; +use core::fmt; +use core::fmt::Debug; + +use mls_rs_codec::{MlsDecode, MlsEncode, MlsSize}; +use mls_rs_core::crypto::CipherSuiteProvider; + +use crate::{client::MlsError, error::IntoAnyError, MlsMessage}; + +#[derive(Clone, PartialEq, Eq, MlsEncode, MlsDecode, MlsSize)] +#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] +pub(crate) struct MessageHash( + #[mls_codec(with = "mls_rs_codec::byte_vec")] + #[cfg_attr(feature = "serde", serde(with = "mls_rs_core::vec_serde"))] + Vec, +); + +impl Debug for MessageHash { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + mls_rs_core::debug::pretty_bytes(&self.0) + .named("CommitHash") + .fmt(f) + } +} + +impl MessageHash { + #[cfg_attr(not(mls_build_async), maybe_async::must_be_sync)] + pub(crate) async fn compute( + cs: &CS, + message: &MlsMessage, + ) -> Result { + cs.hash(&message.mls_encode_to_vec()?) + .await + .map_err(|e| MlsError::CryptoProviderError(e.into_any_error())) + .map(Self) + } +} diff --git a/mls-rs/src/group/message_processor.rs b/mls-rs/src/group/message_processor.rs index 20a85f1b..227bda56 100644 --- a/mls-rs/src/group/message_processor.rs +++ b/mls-rs/src/group/message_processor.rs @@ -187,6 +187,8 @@ pub enum ReceivedMessage { Commit(CommitMessageDescription), /// A proposal was received. Proposal(ProposalMessageDescription), + /// A proposal previously sent by this member was received. + OwnProposal, /// Validated GroupInfo object GroupInfo(GroupInfo), /// Validated welcome message diff --git a/mls-rs/src/group/message_verifier.rs b/mls-rs/src/group/message_verifier.rs index 7a2bc59b..2462822f 100644 --- a/mls-rs/src/group/message_verifier.rs +++ b/mls-rs/src/group/message_verifier.rs @@ -38,7 +38,6 @@ pub(crate) async fn verify_plaintext_authentication( cipher_suite_provider: &P, plaintext: PublicMessage, key_schedule: Option<&KeySchedule>, - self_index: Option, state: &GroupState, ) -> Result { let tag = plaintext.membership_tag.clone(); @@ -52,7 +51,7 @@ pub(crate) async fn verify_plaintext_authentication( // Verify the membership tag if needed match &auth_content.content.sender { - Sender::Member(index) => { + Sender::Member(_) => { if let Some(key_schedule) = key_schedule { let expected_tag = &key_schedule .get_membership_tag(&auth_content, context, cipher_suite_provider) @@ -64,10 +63,6 @@ pub(crate) async fn verify_plaintext_authentication( return Err(MlsError::InvalidMembershipTag); } } - - if self_index == Some(LeafIndex(*index)) { - return Err(MlsError::CantProcessMessageFromSelf); - } } _ => { tag.is_none() @@ -333,7 +328,6 @@ mod tests { &env.bob.group.cipher_suite_provider, message, Some(&env.bob.group.key_schedule), - None, &env.bob.group.state, ) .await @@ -381,7 +375,6 @@ mod tests { &env.bob.group.cipher_suite_provider, message, Some(&env.bob.group.key_schedule), - None, &env.bob.group.state, ) .await; @@ -399,7 +392,6 @@ mod tests { &env.bob.group.cipher_suite_provider, message, Some(&env.bob.group.key_schedule), - None, &env.bob.group.state, ) .await; @@ -417,7 +409,6 @@ mod tests { &env.bob.group.cipher_suite_provider, message, Some(&env.bob.group.key_schedule), - None, &env.bob.group.state, ) .await; @@ -485,7 +476,6 @@ mod tests { &test_group.group.cipher_suite_provider, message, Some(&test_group.group.key_schedule), - None, &test_group.group.state, ) .await @@ -506,7 +496,6 @@ mod tests { &test_group.group.cipher_suite_provider, message, Some(&test_group.group.key_schedule), - None, &test_group.group.state, ) .await; @@ -532,7 +521,6 @@ mod tests { &test_group.group.cipher_suite_provider, message, Some(&test_group.group.key_schedule), - None, &test_group.group.state, ) .await; @@ -556,7 +544,6 @@ mod tests { &test_group.group.cipher_suite_provider, message, Some(&test_group.group.key_schedule), - None, &test_group.group.state, ) .await; @@ -601,7 +588,6 @@ mod tests { &test_group.group.cipher_suite_provider, message, Some(&test_group.group.key_schedule), - None, &test_group.group.state, ) .await @@ -625,7 +611,6 @@ mod tests { &test_group.group.cipher_suite_provider, message, Some(&test_group.group.key_schedule), - None, &test_group.group.state, ) .await; @@ -652,29 +637,10 @@ mod tests { &test_group.group.cipher_suite_provider, message, Some(&test_group.group.key_schedule), - None, &test_group.group.state, ) .await; assert_matches!(res, Err(MlsError::MembershipTagForNonMember)); } - - #[maybe_async::test(not(mls_build_async), async(mls_build_async, crate::futures_test))] - async fn plaintext_from_self_fails_verification() { - let mut env = TestEnv::new().await; - - let message = make_signed_plaintext(&mut env.alice.group).await; - - let res = verify_plaintext_authentication( - &env.alice.group.cipher_suite_provider, - message, - Some(&env.alice.group.key_schedule), - Some(LeafIndex::new(env.alice.group.current_member_index())), - &env.alice.group.state, - ) - .await; - - assert_matches!(res, Err(MlsError::CantProcessMessageFromSelf)) - } } diff --git a/mls-rs/src/group/mod.rs b/mls-rs/src/group/mod.rs index 59cfc17b..e3789fce 100644 --- a/mls-rs/src/group/mod.rs +++ b/mls-rs/src/group/mod.rs @@ -39,6 +39,7 @@ use crate::crypto::{HpkePublicKey, HpkeSecretKey}; use crate::extension::ExternalPubExt; +use self::message_hash::MessageHash; #[cfg(feature = "private_message")] use self::mls_rules::{EncryptionOptions, MlsRules}; @@ -114,6 +115,7 @@ pub(crate) mod framing; mod group_info; pub(crate) mod key_schedule; mod membership_tag; +pub(crate) mod message_hash; pub(crate) mod message_processor; pub(crate) mod message_signature; pub(crate) mod message_verifier; @@ -699,14 +701,25 @@ where ) .await?; + let sender = auth_content.content.sender; + let proposal_ref = ProposalRef::from_content(&self.cipher_suite_provider, &auth_content).await?; + let message = self.format_for_wire(auth_content).await?; + self.state .proposals - .insert(proposal_ref, proposal, auth_content.content.sender); + .insert_own( + proposal_ref, + proposal, + sender, + &message, + &self.cipher_suite_provider, + ) + .await?; - self.format_for_wire(auth_content).await + Ok(message) } /// Unique identifier for this group. @@ -1286,7 +1299,7 @@ where message: MlsMessage, ) -> Result { if let Some(pending) = &self.pending_commit { - let message_hash = CommitHash::compute(&self.cipher_suite_provider, &message).await?; + let message_hash = MessageHash::compute(&self.cipher_suite_provider, &message).await?; if message_hash == pending.commit_message_hash { let message_description = self.apply_pending_commit().await?; @@ -1295,6 +1308,19 @@ where } } + #[cfg(feature = "by_ref_proposal")] + if message.wire_format() == WireFormat::PrivateMessage { + let cached_own_proposal = self + .state + .proposals + .contains_own(&self.cipher_suite_provider, &message) + .await?; + + if cached_own_proposal { + return Ok(ReceivedMessage::OwnProposal); + } + } + MessageProcessor::process_incoming_message( self, message, @@ -1618,7 +1644,6 @@ where &self.cipher_suite_provider, message, Some(&self.key_schedule), - Some(self.private_tree.self_index), &self.state, ) .await?; @@ -1838,8 +1863,8 @@ pub(crate) mod test_utils; mod tests { use crate::{ client::test_utils::{ - test_client_with_key_pkg, TestClientBuilder, TEST_CIPHER_SUITE, - TEST_CUSTOM_PROPOSAL_TYPE, TEST_PROTOCOL_VERSION, + test_client_with_key_pkg, test_client_with_key_pkg_custom, TestClientBuilder, + TEST_CIPHER_SUITE, TEST_CUSTOM_PROPOSAL_TYPE, TEST_PROTOCOL_VERSION, }, client_builder::{test_utils::TestClientConfig, ClientBuilder, MlsConfig}, crypto::test_utils::TestCryptoProvider, @@ -4246,4 +4271,54 @@ mod tests { let res = groups[1].group.apply_pending_commit().await; assert_matches!(res, Err(MlsError::PendingCommitNotFound)); } + + #[cfg(feature = "by_ref_proposal")] + #[maybe_async::test(not(mls_build_async), async(mls_build_async, crate::futures_test))] + async fn can_process_own_plaintext_proposal() { + can_process_own_roposal(false).await; + } + + #[cfg(feature = "by_ref_proposal")] + #[maybe_async::test(not(mls_build_async), async(mls_build_async, crate::futures_test))] + async fn can_process_own_ciphertext_proposal() { + can_process_own_roposal(true).await; + } + + #[cfg(feature = "by_ref_proposal")] + #[cfg_attr(not(mls_build_async), maybe_async::must_be_sync)] + async fn can_process_own_roposal(encrypt_proposal: bool) { + let (alice, _) = test_client_with_key_pkg_custom( + TEST_PROTOCOL_VERSION, + TEST_CIPHER_SUITE, + "alice", + |c| c.0.mls_rules.encryption_options.encrypt_control_messages = encrypt_proposal, + ) + .await; + + let mut alice = TestGroup { + group: alice.create_group(Default::default()).await.unwrap(), + }; + + let mut bob = alice.join("bob").await.0.group; + let mut alice = alice.group; + + let upd = alice.propose_update(vec![]).await.unwrap(); + alice.process_incoming_message(upd.clone()).await.unwrap(); + + bob.process_incoming_message(upd).await.unwrap(); + let commit = bob.commit(vec![]).await.unwrap().commit_message; + let update = alice.process_incoming_message(commit).await.unwrap(); + + let ReceivedMessage::Commit(update) = update else { + panic!("expected commit") + }; + + // Check that proposal was applied i.e. alice's index 0 is updated + assert!(update + .state_update + .roster_update + .updated() + .iter() + .any(|member| member.index() == 0)); + } } diff --git a/mls-rs/src/group/proposal_cache.rs b/mls-rs/src/group/proposal_cache.rs index c0512047..c2055d52 100644 --- a/mls-rs/src/group/proposal_cache.rs +++ b/mls-rs/src/group/proposal_cache.rs @@ -19,7 +19,12 @@ use crate::{ }; #[cfg(feature = "by_ref_proposal")] -use crate::group::{proposal_filter::FilterStrategy, ProposalRef, ProtocolVersion}; +use crate::{ + group::{ + message_hash::MessageHash, proposal_filter::FilterStrategy, ProposalRef, ProtocolVersion, + }, + MlsMessage, +}; use crate::tree_kem::leaf_node::LeafNode; @@ -43,11 +48,21 @@ pub struct CachedProposal { } #[cfg(feature = "by_ref_proposal")] -#[derive(Clone, PartialEq)] +#[derive(Clone)] pub(crate) struct ProposalCache { protocol_version: ProtocolVersion, group_id: Vec, pub(crate) proposals: crate::map::SmallMap, + pub(crate) own_proposals: Vec, +} + +#[cfg(feature = "by_ref_proposal")] +impl PartialEq for ProposalCache { + fn eq(&self, other: &Self) -> bool { + self.protocol_version == other.protocol_version + && self.group_id == other.group_id + && self.proposals == other.proposals + } } #[cfg(feature = "by_ref_proposal")] @@ -71,6 +86,7 @@ impl ProposalCache { protocol_version, group_id, proposals: Default::default(), + own_proposals: Default::default(), } } @@ -78,11 +94,13 @@ impl ProposalCache { protocol_version: ProtocolVersion, group_id: Vec, proposals: crate::map::SmallMap, + own_proposals: Vec, ) -> Self { Self { protocol_version, group_id, proposals, + own_proposals, } } @@ -108,6 +126,22 @@ impl ProposalCache { self.proposals.push((proposal_ref, cached_proposal)); } + #[cfg_attr(not(mls_build_async), maybe_async::must_be_sync)] + pub async fn insert_own( + &mut self, + proposal_ref: ProposalRef, + proposal: Proposal, + sender: Sender, + message: &MlsMessage, + cs: &CS, + ) -> Result<(), MlsError> { + self.insert(proposal_ref, proposal, sender); + let message_hash = MessageHash::compute(cs, message).await?; + self.own_proposals.push(message_hash); + + Ok(()) + } + pub fn prepare_commit( &self, sender: Sender, @@ -162,6 +196,17 @@ impl ProposalCache { Ok(proposals) } + + #[cfg_attr(not(mls_build_async), maybe_async::must_be_sync)] + pub async fn contains_own( + &self, + cs: &CS, + message: &MlsMessage, + ) -> Result { + let message_hash = MessageHash::compute(cs, message).await?; + + Ok(self.own_proposals.iter().any(|op| op == &message_hash)) + } } #[cfg(not(feature = "by_ref_proposal"))] diff --git a/mls-rs/src/group/snapshot.rs b/mls-rs/src/group/snapshot.rs index b070d71b..b5e97a9a 100644 --- a/mls-rs/src/group/snapshot.rs +++ b/mls-rs/src/group/snapshot.rs @@ -20,7 +20,10 @@ use crate::{ }; #[cfg(feature = "by_ref_proposal")] -use super::proposal_cache::{CachedProposal, ProposalCache}; +use super::{ + message_hash::MessageHash, + proposal_cache::{CachedProposal, ProposalCache}, +}; use mls_rs_codec::{MlsDecode, MlsEncode, MlsSize}; @@ -50,6 +53,8 @@ pub(crate) struct RawGroupState { pub(crate) context: GroupContext, #[cfg(feature = "by_ref_proposal")] pub(crate) proposals: SmallMap, + #[cfg(feature = "by_ref_proposal")] + pub(crate) own_proposals: Vec, pub(crate) public_tree: TreeKemPublic, pub(crate) interim_transcript_hash: InterimTranscriptHash, pub(crate) pending_reinit: Option, @@ -72,6 +77,8 @@ impl RawGroupState { context: state.context.clone(), #[cfg(feature = "by_ref_proposal")] proposals: state.proposals.proposals.clone(), + #[cfg(feature = "by_ref_proposal")] + own_proposals: state.proposals.own_proposals.clone(), public_tree, interim_transcript_hash: state.interim_transcript_hash.clone(), pending_reinit: state.pending_reinit.clone(), @@ -92,6 +99,7 @@ impl RawGroupState { context.protocol_version, context.group_id.clone(), self.proposals, + self.own_proposals.clone(), ); let mut public_tree = self.public_tree; @@ -121,6 +129,7 @@ impl RawGroupState { context.protocol_version, context.group_id.clone(), self.proposals, + self.own_proposals.clone(), ); Ok(GroupState { @@ -229,6 +238,8 @@ pub(crate) mod test_utils { context: get_test_group_context(epoch_id, cipher_suite).await, #[cfg(feature = "by_ref_proposal")] proposals: Default::default(), + #[cfg(feature = "by_ref_proposal")] + own_proposals: Default::default(), public_tree: Default::default(), interim_transcript_hash: InterimTranscriptHash::from(vec![]), pending_reinit: None, diff --git a/mls-rs/test_harness_integration/src/main.rs b/mls-rs/test_harness_integration/src/main.rs index 5b1a472f..61d242e6 100644 --- a/mls-rs/test_harness_integration/src/main.rs +++ b/mls-rs/test_harness_integration/src/main.rs @@ -17,7 +17,6 @@ use mls_rs::{ BaseInMemoryConfig, ClientBuilder, WithCryptoProvider, WithIdentityProvider, WithMlsRules, }, crypto::SignatureSecretKey, - error::MlsError, external_client::ExternalClient, group::{ExportedTree, Member, ReceivedMessage, Roster, StateUpdate}, identity::{ @@ -692,11 +691,7 @@ impl MlsClientImpl { for proposal_bytes in &request.by_reference { let proposal = MlsMessage::from_bytes(proposal_bytes).map_err(abort)?; - - match group.process_incoming_message(proposal) { - Ok(_) | Err(MlsError::CantProcessMessageFromSelf) => Ok(()), - Err(e) => Err(abort(e)), - }?; + group.process_incoming_message(proposal).map_err(abort)?; } { @@ -788,11 +783,7 @@ impl MlsClientImpl { for proposal in &request.proposal { let proposal = MlsMessage::from_bytes(proposal).map_err(abort)?; - - match group.process_incoming_message(proposal) { - Ok(_) | Err(MlsError::CantProcessMessageFromSelf) => Ok(()), - Err(e) => Err(abort(e)), - }?; + group.process_incoming_message(proposal).map_err(abort)?; } let commit = MlsMessage::from_bytes(&request.commit).map_err(abort)?; From c0ab04d7b6f1acd5c82af51b530e69eb87942222 Mon Sep 17 00:00:00 2001 From: mulmarta Date: Fri, 3 May 2024 14:13:57 +0200 Subject: [PATCH 2/7] Fixup --- mls-rs/src/group/mod.rs | 17 +++++++++++++++++ mls-rs/src/group/proposal_cache.rs | 2 +- 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/mls-rs/src/group/mod.rs b/mls-rs/src/group/mod.rs index e3789fce..709eb8aa 100644 --- a/mls-rs/src/group/mod.rs +++ b/mls-rs/src/group/mod.rs @@ -4321,4 +4321,21 @@ mod tests { .iter() .any(|member| member.index() == 0)); } + + #[cfg(feature = "by_ref_proposal")] + #[maybe_async::test(not(mls_build_async), async(mls_build_async, crate::futures_test))] + async fn commit_clears_proposals() { + let mut groups = test_n_member_group(TEST_PROTOCOL_VERSION, TEST_CIPHER_SUITE, 2).await; + + groups[0].group.propose_update(vec![]).await.unwrap(); + + assert_eq!(groups[0].group.state.proposals.proposals.len(), 1); + assert_eq!(groups[0].group.state.proposals.own_proposals.len(), 1); + + let commit = groups[1].group.commit(vec![]).await.unwrap().commit_message; + groups[0].process_message(commit).await.unwrap(); + + assert!(groups[0].group.state.proposals.proposals.is_empty()); + assert!(groups[0].group.state.proposals.own_proposals.is_empty()); + } } diff --git a/mls-rs/src/group/proposal_cache.rs b/mls-rs/src/group/proposal_cache.rs index c2055d52..564c5837 100644 --- a/mls-rs/src/group/proposal_cache.rs +++ b/mls-rs/src/group/proposal_cache.rs @@ -104,9 +104,9 @@ impl ProposalCache { } } - #[inline] pub fn clear(&mut self) { self.proposals.clear(); + self.own_proposals.clear(); } #[cfg(feature = "private_message")] From c0fba6b27eab3e5b8912337f19349d908b74a262 Mon Sep 17 00:00:00 2001 From: mulmarta <103590845+mulmarta@users.noreply.github.com> Date: Fri, 3 May 2024 14:14:28 +0200 Subject: [PATCH 3/7] Apply suggestions from code review Co-authored-by: Stephane Raux <94983192+stefunctional@users.noreply.github.com> --- mls-rs/src/group/message_hash.rs | 2 +- mls-rs/src/group/proposal_cache.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/mls-rs/src/group/message_hash.rs b/mls-rs/src/group/message_hash.rs index 64dff244..1ee215f1 100644 --- a/mls-rs/src/group/message_hash.rs +++ b/mls-rs/src/group/message_hash.rs @@ -18,7 +18,7 @@ pub(crate) struct MessageHash( impl Debug for MessageHash { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { mls_rs_core::debug::pretty_bytes(&self.0) - .named("CommitHash") + .named("MessageHash") .fmt(f) } } diff --git a/mls-rs/src/group/proposal_cache.rs b/mls-rs/src/group/proposal_cache.rs index 564c5837..7e2df0ae 100644 --- a/mls-rs/src/group/proposal_cache.rs +++ b/mls-rs/src/group/proposal_cache.rs @@ -205,7 +205,7 @@ impl ProposalCache { ) -> Result { let message_hash = MessageHash::compute(cs, message).await?; - Ok(self.own_proposals.iter().any(|op| op == &message_hash)) + Ok(self.own_proposals.contains(&message_hash)) } } From b213f2f456574487bce7c4016b8169a3f221aa19 Mon Sep 17 00:00:00 2001 From: Marta Mularczyk Date: Fri, 14 Jun 2024 09:09:09 +0200 Subject: [PATCH 4/7] Rebase on main and unify received proposal --- mls-rs-uniffi/src/lib.rs | 1 - mls-rs/src/group/message_hash.rs | 2 +- mls-rs/src/group/message_processor.rs | 55 ++++++++++++++++----------- mls-rs/src/group/mod.rs | 19 ++++----- mls-rs/src/group/proposal_cache.rs | 26 +++++++------ mls-rs/src/group/snapshot.rs | 7 +++- 6 files changed, 61 insertions(+), 49 deletions(-) diff --git a/mls-rs-uniffi/src/lib.rs b/mls-rs-uniffi/src/lib.rs index 193f0102..8f339317 100644 --- a/mls-rs-uniffi/src/lib.rs +++ b/mls-rs-uniffi/src/lib.rs @@ -773,7 +773,6 @@ impl Group { let proposal = Arc::new(proposal_message.proposal.into()); Ok(ReceivedMessage::ReceivedProposal { sender, proposal }) } - group::ReceivedMessage::OwnProposal => Ok(ReceivedMessage::OwnProposal), // TODO: group::ReceivedMessage::GroupInfo does not have any // public methods (unless the "ffi" Cargo feature is set). // So perhaps we don't need it? diff --git a/mls-rs/src/group/message_hash.rs b/mls-rs/src/group/message_hash.rs index 1ee215f1..b2ac4d6f 100644 --- a/mls-rs/src/group/message_hash.rs +++ b/mls-rs/src/group/message_hash.rs @@ -7,7 +7,7 @@ use mls_rs_core::crypto::CipherSuiteProvider; use crate::{client::MlsError, error::IntoAnyError, MlsMessage}; -#[derive(Clone, PartialEq, Eq, MlsEncode, MlsDecode, MlsSize)] +#[derive(Clone, PartialEq, Eq, MlsEncode, MlsDecode, MlsSize, Hash)] #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] pub(crate) struct MessageHash( #[mls_codec(with = "mls_rs_codec::byte_vec")] diff --git a/mls-rs/src/group/message_processor.rs b/mls-rs/src/group/message_processor.rs index 227bda56..de17e102 100644 --- a/mls-rs/src/group/message_processor.rs +++ b/mls-rs/src/group/message_processor.rs @@ -27,6 +27,8 @@ use crate::{ }, CipherSuiteProvider, KeyPackage, }; +use mls_rs_codec::{MlsDecode, MlsEncode, MlsSize}; + #[cfg(mls_build_async)] use alloc::boxed::Box; use alloc::vec::Vec; @@ -68,9 +70,6 @@ use super::proposal::CustomProposal; #[cfg(feature = "private_message")] use crate::group::framing::PrivateMessage; -#[cfg(feature = "by_ref_proposal")] -use mls_rs_codec::{MlsDecode, MlsEncode, MlsSize}; - #[derive(Debug)] pub(crate) struct ProvisionalState { pub(crate) public_tree: TreeKemPublic, @@ -187,8 +186,6 @@ pub enum ReceivedMessage { Commit(CommitMessageDescription), /// A proposal was received. Proposal(ProposalMessageDescription), - /// A proposal previously sent by this member was received. - OwnProposal, /// Validated GroupInfo object GroupInfo(GroupInfo), /// Validated welcome message @@ -302,16 +299,18 @@ impl Debug for CommitMessageDescription { } } -#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, MlsEncode, MlsDecode, MlsSize)] +#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] +#[repr(u8)] /// Proposal sender type. pub enum ProposalSender { /// A current member of the group by index in the group state. - Member(u32), + Member(u32) = 1u8, /// An external entity by index within an /// [`ExternalSendersExt`](crate::extension::built_in::ExternalSendersExt). - External(u32), + External(u32) = 2u8, /// A new member proposing their addition to the group. - NewMember, + NewMember = 3u8, } impl TryFrom for ProposalSender { @@ -334,7 +333,8 @@ impl TryFrom for ProposalSender { all(feature = "ffi", not(test)), safer_ffi_gen::ffi_type(clone, opaque) )] -#[derive(Clone)] +#[derive(Clone, MlsEncode, MlsDecode, MlsSize, PartialEq)] +#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] #[non_exhaustive] /// Description of a processed MLS proposal message. pub struct ProposalMessageDescription { @@ -403,6 +403,20 @@ impl ProposalMessageDescription { pub fn proposal_ref(&self) -> Vec { self.proposal_ref.to_vec() } + + #[cfg_attr(not(mls_build_async), maybe_async::must_be_sync)] + pub(crate) async fn new( + cs: &C, + content: &AuthenticatedContent, + proposal: Proposal, + ) -> Result { + Ok(ProposalMessageDescription { + authenticated_data: content.content.authenticated_data.clone(), + proposal, + sender: content.content.sender.try_into()?, + proposal_ref: ProposalRef::from_content(cs, content).await?, + }) + } } #[cfg(not(feature = "by_ref_proposal"))] @@ -589,27 +603,24 @@ pub(crate) trait MessageProcessor: Send + Sync { proposal: &Proposal, cache_proposal: bool, ) -> Result { - let proposal_ref = - ProposalRef::from_content(self.cipher_suite_provider(), auth_content).await?; + let proposal = ProposalMessageDescription::new( + self.cipher_suite_provider(), + auth_content, + proposal.clone(), + ) + .await?; let group_state = self.group_state_mut(); if cache_proposal { - let proposal_ref = proposal_ref.clone(); - group_state.proposals.insert( - proposal_ref.clone(), - proposal.clone(), + proposal.proposal_ref.clone(), + proposal.proposal.clone(), auth_content.content.sender, ); } - Ok(ProposalMessageDescription { - authenticated_data: auth_content.content.authenticated_data.clone(), - proposal: proposal.clone(), - sender: auth_content.content.sender.try_into()?, - proposal_ref, - }) + Ok(proposal) } #[cfg(feature = "state_update")] diff --git a/mls-rs/src/group/mod.rs b/mls-rs/src/group/mod.rs index 709eb8aa..4a32017e 100644 --- a/mls-rs/src/group/mod.rs +++ b/mls-rs/src/group/mod.rs @@ -703,20 +703,15 @@ where let sender = auth_content.content.sender; - let proposal_ref = - ProposalRef::from_content(&self.cipher_suite_provider, &auth_content).await?; + let proposal_desc = + ProposalMessageDescription::new(&self.cipher_suite_provider, &auth_content, proposal) + .await?; let message = self.format_for_wire(auth_content).await?; self.state .proposals - .insert_own( - proposal_ref, - proposal, - sender, - &message, - &self.cipher_suite_provider, - ) + .insert_own(proposal_desc, &message, sender, &self.cipher_suite_provider) .await?; Ok(message) @@ -1313,11 +1308,11 @@ where let cached_own_proposal = self .state .proposals - .contains_own(&self.cipher_suite_provider, &message) + .get_own(&self.cipher_suite_provider, &message) .await?; - if cached_own_proposal { - return Ok(ReceivedMessage::OwnProposal); + if let Some(cached) = cached_own_proposal { + return Ok(ReceivedMessage::Proposal(cached)); } } diff --git a/mls-rs/src/group/proposal_cache.rs b/mls-rs/src/group/proposal_cache.rs index 7e2df0ae..cb2047d6 100644 --- a/mls-rs/src/group/proposal_cache.rs +++ b/mls-rs/src/group/proposal_cache.rs @@ -7,7 +7,7 @@ use alloc::vec::Vec; use super::{ message_processor::ProvisionalState, mls_rules::{CommitDirection, CommitSource, MlsRules}, - GroupState, ProposalOrRef, + GroupState, ProposalMessageDescription, ProposalOrRef, }; use crate::{ client::MlsError, @@ -53,7 +53,7 @@ pub(crate) struct ProposalCache { protocol_version: ProtocolVersion, group_id: Vec, pub(crate) proposals: crate::map::SmallMap, - pub(crate) own_proposals: Vec, + pub(crate) own_proposals: crate::map::SmallMap, } #[cfg(feature = "by_ref_proposal")] @@ -94,7 +94,7 @@ impl ProposalCache { protocol_version: ProtocolVersion, group_id: Vec, proposals: crate::map::SmallMap, - own_proposals: Vec, + own_proposals: crate::map::SmallMap, ) -> Self { Self { protocol_version, @@ -129,15 +129,19 @@ impl ProposalCache { #[cfg_attr(not(mls_build_async), maybe_async::must_be_sync)] pub async fn insert_own( &mut self, - proposal_ref: ProposalRef, - proposal: Proposal, - sender: Sender, + proposal: ProposalMessageDescription, message: &MlsMessage, + sender: Sender, cs: &CS, ) -> Result<(), MlsError> { - self.insert(proposal_ref, proposal, sender); + self.insert( + proposal.proposal_ref.clone(), + proposal.proposal.clone(), + sender, + ); + let message_hash = MessageHash::compute(cs, message).await?; - self.own_proposals.push(message_hash); + self.own_proposals.insert(message_hash, proposal); Ok(()) } @@ -198,14 +202,14 @@ impl ProposalCache { } #[cfg_attr(not(mls_build_async), maybe_async::must_be_sync)] - pub async fn contains_own( + pub async fn get_own( &self, cs: &CS, message: &MlsMessage, - ) -> Result { + ) -> Result, MlsError> { let message_hash = MessageHash::compute(cs, message).await?; - Ok(self.own_proposals.contains(&message_hash)) + Ok(self.own_proposals.get(&message_hash).cloned()) } } diff --git a/mls-rs/src/group/snapshot.rs b/mls-rs/src/group/snapshot.rs index b5e97a9a..c07cb44f 100644 --- a/mls-rs/src/group/snapshot.rs +++ b/mls-rs/src/group/snapshot.rs @@ -31,7 +31,10 @@ use mls_rs_core::crypto::SignatureSecretKey; #[cfg(feature = "tree_index")] use mls_rs_core::identity::IdentityProvider; -use super::{cipher_suite_provider, epoch::EpochSecrets, state_repo::GroupStateRepository}; +use super::{ + cipher_suite_provider, epoch::EpochSecrets, state_repo::GroupStateRepository, + ProposalMessageDescription, +}; #[derive(Debug, PartialEq, Clone, MlsEncode, MlsDecode, MlsSize)] #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] @@ -54,7 +57,7 @@ pub(crate) struct RawGroupState { #[cfg(feature = "by_ref_proposal")] pub(crate) proposals: SmallMap, #[cfg(feature = "by_ref_proposal")] - pub(crate) own_proposals: Vec, + pub(crate) own_proposals: SmallMap, pub(crate) public_tree: TreeKemPublic, pub(crate) interim_transcript_hash: InterimTranscriptHash, pub(crate) pending_reinit: Option, From 1032fd88fbe5a430982f8404592eccfcc1aaf0e9 Mon Sep 17 00:00:00 2001 From: Marta Mularczyk Date: Fri, 14 Jun 2024 09:13:13 +0200 Subject: [PATCH 5/7] Fixup --- mls-rs/src/group/proposal_cache.rs | 5 +++-- mls-rs/src/group/snapshot.rs | 21 +++++++-------------- 2 files changed, 10 insertions(+), 16 deletions(-) diff --git a/mls-rs/src/group/proposal_cache.rs b/mls-rs/src/group/proposal_cache.rs index cb2047d6..193b9317 100644 --- a/mls-rs/src/group/proposal_cache.rs +++ b/mls-rs/src/group/proposal_cache.rs @@ -7,7 +7,7 @@ use alloc::vec::Vec; use super::{ message_processor::ProvisionalState, mls_rules::{CommitDirection, CommitSource, MlsRules}, - GroupState, ProposalMessageDescription, ProposalOrRef, + GroupState, ProposalOrRef, }; use crate::{ client::MlsError, @@ -21,7 +21,8 @@ use crate::{ #[cfg(feature = "by_ref_proposal")] use crate::{ group::{ - message_hash::MessageHash, proposal_filter::FilterStrategy, ProposalRef, ProtocolVersion, + message_hash::MessageHash, proposal_filter::FilterStrategy, ProposalMessageDescription, + ProposalRef, ProtocolVersion, }, MlsMessage, }; diff --git a/mls-rs/src/group/snapshot.rs b/mls-rs/src/group/snapshot.rs index c07cb44f..82d6f94d 100644 --- a/mls-rs/src/group/snapshot.rs +++ b/mls-rs/src/group/snapshot.rs @@ -6,7 +6,8 @@ use crate::{ client::MlsError, client_config::ClientConfig, group::{ - key_schedule::KeySchedule, CommitGeneration, ConfirmationTag, Group, GroupContext, + cipher_suite_provider, epoch::EpochSecrets, key_schedule::KeySchedule, + state_repo::GroupStateRepository, CommitGeneration, ConfirmationTag, Group, GroupContext, GroupState, InterimTranscriptHash, ReInitProposal, TreeKemPublic, }, tree_kem::TreeKemPrivate, @@ -15,27 +16,19 @@ use crate::{ #[cfg(feature = "by_ref_proposal")] use crate::{ crypto::{HpkePublicKey, HpkeSecretKey}, - group::ProposalRef, + group::{ + message_hash::MessageHash, + proposal_cache::{CachedProposal, ProposalCache}, + ProposalMessageDescription, ProposalRef, + }, map::SmallMap, }; -#[cfg(feature = "by_ref_proposal")] -use super::{ - message_hash::MessageHash, - proposal_cache::{CachedProposal, ProposalCache}, -}; - use mls_rs_codec::{MlsDecode, MlsEncode, MlsSize}; - use mls_rs_core::crypto::SignatureSecretKey; #[cfg(feature = "tree_index")] use mls_rs_core::identity::IdentityProvider; -use super::{ - cipher_suite_provider, epoch::EpochSecrets, state_repo::GroupStateRepository, - ProposalMessageDescription, -}; - #[derive(Debug, PartialEq, Clone, MlsEncode, MlsDecode, MlsSize)] #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] pub(crate) struct Snapshot { From d92404822ae3d0a50492679af37a1ed5b193a4e0 Mon Sep 17 00:00:00 2001 From: Marta Mularczyk Date: Fri, 14 Jun 2024 09:22:25 +0200 Subject: [PATCH 6/7] Remove unused field --- mls-rs/src/client.rs | 1 - mls-rs/src/group/proposal_cache.rs | 1 - mls-rs/src/group/test_utils.rs | 2 -- mls-rs/src/key_package/generator.rs | 17 +++-------------- mls-rs/src/key_package/mod.rs | 2 -- 5 files changed, 3 insertions(+), 20 deletions(-) diff --git a/mls-rs/src/client.rs b/mls-rs/src/client.rs index a7031bba..7bcdd11f 100644 --- a/mls-rs/src/client.rs +++ b/mls-rs/src/client.rs @@ -445,7 +445,6 @@ where cipher_suite_provider: &cipher_suite_provider, signing_key: self.signer()?, signing_identity, - identity_provider: &self.config.identity_provider(), }; let key_pkg_gen = key_package_generator diff --git a/mls-rs/src/group/proposal_cache.rs b/mls-rs/src/group/proposal_cache.rs index 193b9317..7e714e45 100644 --- a/mls-rs/src/group/proposal_cache.rs +++ b/mls-rs/src/group/proposal_cache.rs @@ -3630,7 +3630,6 @@ mod tests { cipher_suite_provider: &test_cipher_suite_provider(TEST_CIPHER_SUITE), signing_identity: &signing_identity, signing_key: &secret_key, - identity_provider: &BasicWithCustomProvider::new(BasicIdentityProvider::new()), }; generator diff --git a/mls-rs/src/group/test_utils.rs b/mls-rs/src/group/test_utils.rs index 2800182d..7cfe5217 100644 --- a/mls-rs/src/group/test_utils.rs +++ b/mls-rs/src/group/test_utils.rs @@ -19,7 +19,6 @@ use crate::{ client_builder::test_utils::{TestClientBuilder, TestClientConfig}, crypto::test_utils::test_cipher_suite_provider, extension::ExtensionType, - identity::basic::BasicIdentityProvider, identity::test_utils::get_test_signing_identity, key_package::{KeyPackageGeneration, KeyPackageGenerator}, mls_rules::{CommitOptions, DefaultMlsRules}, @@ -210,7 +209,6 @@ pub(crate) async fn test_member( cipher_suite_provider: &test_cipher_suite_provider(cipher_suite), signing_identity: &signing_identity, signing_key: &signing_key, - identity_provider: &BasicIdentityProvider, }; let key_package = key_package_generator diff --git a/mls-rs/src/key_package/generator.rs b/mls-rs/src/key_package/generator.rs index 4d71094f..abc215f7 100644 --- a/mls-rs/src/key_package/generator.rs +++ b/mls-rs/src/key_package/generator.rs @@ -5,7 +5,7 @@ use alloc::vec; use alloc::vec::Vec; use mls_rs_codec::{MlsDecode, MlsEncode}; -use mls_rs_core::{error::IntoAnyError, identity::IdentityProvider, key_package::KeyPackageData}; +use mls_rs_core::{error::IntoAnyError, key_package::KeyPackageData}; use crate::client::MlsError; use crate::{ @@ -24,16 +24,11 @@ use crate::{ use super::{KeyPackage, KeyPackageRef}; #[derive(Clone, Debug)] -pub struct KeyPackageGenerator<'a, IP, CP> -where - IP: IdentityProvider, - CP: CipherSuiteProvider, -{ +pub struct KeyPackageGenerator<'a, CP: CipherSuiteProvider> { pub protocol_version: ProtocolVersion, pub cipher_suite_provider: &'a CP, pub signing_identity: &'a SigningIdentity, pub signing_key: &'a SignatureSecretKey, - pub identity_provider: &'a IP, } #[derive(Clone, Debug)] @@ -75,11 +70,7 @@ impl KeyPackageGeneration { } } -impl<'a, IP, CP> KeyPackageGenerator<'a, IP, CP> -where - IP: IdentityProvider, - CP: CipherSuiteProvider, -{ +impl<'a, CP: CipherSuiteProvider> KeyPackageGenerator<'a, CP> { #[cfg_attr(not(mls_build_async), maybe_async::must_be_sync)] pub(super) async fn sign(&self, package: &mut KeyPackage) -> Result<(), MlsError> { package @@ -199,7 +190,6 @@ mod tests { cipher_suite_provider: &cipher_suite_provider, signing_identity: &signing_identity, signing_key: &signing_key, - identity_provider: &BasicIdentityProvider, }; let mut capabilities = get_test_capabilities(); @@ -300,7 +290,6 @@ mod tests { cipher_suite_provider: &test_cipher_suite_provider(cipher_suite), signing_identity: &signing_identity, signing_key: &signing_key, - identity_provider: &BasicIdentityProvider, }; let first_key_package = test_generator diff --git a/mls-rs/src/key_package/mod.rs b/mls-rs/src/key_package/mod.rs index b3ef83b0..6253a5cf 100644 --- a/mls-rs/src/key_package/mod.rs +++ b/mls-rs/src/key_package/mod.rs @@ -173,7 +173,6 @@ pub(crate) mod test_utils { use crate::{ crypto::test_utils::test_cipher_suite_provider, group::framing::MlsMessagePayload, - identity::basic::BasicIdentityProvider, identity::test_utils::get_test_signing_identity, tree_kem::{leaf_node::test_utils::get_test_capabilities, Lifetime}, MlsMessage, @@ -206,7 +205,6 @@ pub(crate) mod test_utils { cipher_suite_provider: &test_cipher_suite_provider(cipher_suite), signing_identity: &signing_identity, signing_key: &secret_key, - identity_provider: &BasicIdentityProvider, }; let key_package = generator From a6aa6a578959c484ee31aac3e4319a3805034645 Mon Sep 17 00:00:00 2001 From: Marta Mularczyk Date: Fri, 14 Jun 2024 09:36:07 +0200 Subject: [PATCH 7/7] Remove unused enum variant --- mls-rs-uniffi/src/lib.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/mls-rs-uniffi/src/lib.rs b/mls-rs-uniffi/src/lib.rs index 8f339317..36a0e43d 100644 --- a/mls-rs-uniffi/src/lib.rs +++ b/mls-rs-uniffi/src/lib.rs @@ -270,9 +270,6 @@ pub enum ReceivedMessage { sender: Arc, proposal: Arc, }, - - /// A proposal previously sent by this member was received. - OwnProposal, /// Validated GroupInfo object. GroupInfo, /// Validated welcome message.