From a7d270bf614029670b3391b1ab4e25146b9dabbb Mon Sep 17 00:00:00 2001 From: Richard Hua Date: Wed, 2 Oct 2024 19:45:00 -0700 Subject: [PATCH 1/8] Add rotated_at_ns --- .../down.sql | 3 ++ .../up.sql | 3 ++ xmtp_mls/migrations/README.md | 2 +- xmtp_mls/src/storage/encrypted_store/group.rs | 33 +++++++++++++++++++ .../src/storage/encrypted_store/schema.rs | 1 + 5 files changed, 41 insertions(+), 1 deletion(-) create mode 100644 xmtp_mls/migrations/2024-10-03-004750_add_rotated_at_ns/down.sql create mode 100644 xmtp_mls/migrations/2024-10-03-004750_add_rotated_at_ns/up.sql diff --git a/xmtp_mls/migrations/2024-10-03-004750_add_rotated_at_ns/down.sql b/xmtp_mls/migrations/2024-10-03-004750_add_rotated_at_ns/down.sql new file mode 100644 index 000000000..d114c0a72 --- /dev/null +++ b/xmtp_mls/migrations/2024-10-03-004750_add_rotated_at_ns/down.sql @@ -0,0 +1,3 @@ +ALTER TABLE GROUPS + DROP COLUMN rotated_at_ns; + diff --git a/xmtp_mls/migrations/2024-10-03-004750_add_rotated_at_ns/up.sql b/xmtp_mls/migrations/2024-10-03-004750_add_rotated_at_ns/up.sql new file mode 100644 index 000000000..db8fef9c9 --- /dev/null +++ b/xmtp_mls/migrations/2024-10-03-004750_add_rotated_at_ns/up.sql @@ -0,0 +1,3 @@ +ALTER TABLE GROUPS + ADD COLUMN rotated_at_ns BIGINT NOT NULL DEFAULT 0; + diff --git a/xmtp_mls/migrations/README.md b/xmtp_mls/migrations/README.md index 6612df812..ab8fbc634 100644 --- a/xmtp_mls/migrations/README.md +++ b/xmtp_mls/migrations/README.md @@ -22,4 +22,4 @@ Edit the `up.sql` and `down.sql` files created cargo run --bin update-schema ``` -This updates the generated `schema.rs` file. You can now update the models and queries to reference it in `xmtp_mls/src/storage/encrypted_store/`. +Make sure you run this from `xmtp_mls/`. This updates the generated `schema.rs` file. You can now update the models and queries to reference it in `xmtp_mls/src/storage/encrypted_store/`. diff --git a/xmtp_mls/src/storage/encrypted_store/group.rs b/xmtp_mls/src/storage/encrypted_store/group.rs index 1142e7dff..05c47f68a 100644 --- a/xmtp_mls/src/storage/encrypted_store/group.rs +++ b/xmtp_mls/src/storage/encrypted_store/group.rs @@ -39,6 +39,8 @@ pub struct StoredGroup { pub added_by_inbox_id: String, /// The sequence id of the welcome message pub welcome_id: Option, + /// The last time the leaf node encryption key was rotated + pub rotated_at_ns: i64, } impl_fetch!(StoredGroup, groups, Vec); @@ -62,6 +64,7 @@ impl StoredGroup { purpose, added_by_inbox_id, welcome_id: Some(welcome_id), + rotated_at_ns: 0, } } @@ -80,6 +83,7 @@ impl StoredGroup { purpose: Purpose::Conversation, added_by_inbox_id, welcome_id: None, + rotated_at_ns: 0, } } @@ -98,6 +102,7 @@ impl StoredGroup { purpose: Purpose::Sync, added_by_inbox_id: "".into(), welcome_id: None, + rotated_at_ns: 0, } } } @@ -183,6 +188,34 @@ impl DbConnection { Ok(()) } + pub fn get_rotated_at_ns(&self, group_id: Vec) -> Result { + let last_ts = self.raw_query(|conn| { + let ts = dsl::groups + .find(&group_id) + .select(dsl::rotated_at_ns) + .first(conn) + .optional()?; + Ok(ts) + })?; + + last_ts.ok_or(StorageError::NotFound(format!( + "installation time for group {}", + hex::encode(group_id) + ))) + } + + /// Updates the 'last time checked' we checked for new installations. + pub fn update_rotated_at_ns(&self, group_id: Vec) -> Result<(), StorageError> { + self.raw_query(|conn| { + let now = crate::utils::time::now_ns(); + diesel::update(dsl::groups.find(&group_id)) + .set(dsl::rotated_at_ns.eq(now)) + .execute(conn) + })?; + + Ok(()) + } + pub fn get_installations_time_checked(&self, group_id: Vec) -> Result { let last_ts = self.raw_query(|conn| { let ts = dsl::groups diff --git a/xmtp_mls/src/storage/encrypted_store/schema.rs b/xmtp_mls/src/storage/encrypted_store/schema.rs index 7d835a5b9..24f9bed88 100644 --- a/xmtp_mls/src/storage/encrypted_store/schema.rs +++ b/xmtp_mls/src/storage/encrypted_store/schema.rs @@ -53,6 +53,7 @@ diesel::table! { purpose -> Integer, added_by_inbox_id -> Text, welcome_id -> Nullable, + rotated_at_ns -> BigInt, } } From 97160634a70958447c98845bc8574f2b8c01fa9f Mon Sep 17 00:00:00 2001 From: Richard Hua Date: Wed, 2 Oct 2024 20:50:30 -0700 Subject: [PATCH 2/8] Rotate key on send --- xmtp_mls/src/configuration.rs | 4 ++++ xmtp_mls/src/groups/mod.rs | 20 +++++++++++++------- xmtp_mls/src/groups/sync.rs | 19 +++++++++++++++++-- 3 files changed, 34 insertions(+), 9 deletions(-) diff --git a/xmtp_mls/src/configuration.rs b/xmtp_mls/src/configuration.rs index 039299de2..96a918495 100644 --- a/xmtp_mls/src/configuration.rs +++ b/xmtp_mls/src/configuration.rs @@ -16,6 +16,10 @@ const NS_IN_SEC: i64 = 1_000_000_000; const NS_IN_HOUR: i64 = NS_IN_SEC * 60 * 60; +const NS_IN_DAY: i64 = NS_IN_HOUR * 24; + +pub const GROUP_KEY_ROTATION_INTERVAL_NS: i64 = 30 * NS_IN_DAY; + pub const SYNC_UPDATE_INSTALLATIONS_INTERVAL_NS: i64 = NS_IN_HOUR / 2; // 30 min pub const SEND_MESSAGE_UPDATE_INSTALLATIONS_INTERVAL_NS: i64 = 5 * NS_IN_SEC; diff --git a/xmtp_mls/src/groups/mod.rs b/xmtp_mls/src/groups/mod.rs index 2bcb21172..74920bbf0 100644 --- a/xmtp_mls/src/groups/mod.rs +++ b/xmtp_mls/src/groups/mod.rs @@ -86,7 +86,7 @@ use crate::{ consent_record::{ConsentState, ConsentType, StoredConsentRecord}, db_connection::DbConnection, group::{GroupMembershipState, Purpose, StoredGroup}, - group_intent::{IntentKind, NewGroupIntent}, + group_intent::{IntentKind, NewGroupIntent, StoredGroupIntent}, group_message::{DeliveryStatus, GroupMessageKind, StoredGroupMessage}, sql_key_store, }, @@ -564,6 +564,7 @@ impl MlsGroup { .encode(&mut encoded_envelope) .map_err(GroupError::EncodeError)?; + self.maybe_prepare_key_update()?; let intent_data: Vec = SendMessageIntentData::new(encoded_envelope).into(); let intent = NewGroupIntent::new(IntentKind::SendMessage, self.group_id.clone(), intent_data); @@ -1020,18 +1021,23 @@ impl MlsGroup { Ok(()) } - // Update this installation's leaf key in the group by creating a key update commit - pub async fn key_update(&self, client: &Client) -> Result<(), GroupError> - where - ApiClient: XmtpApi, - { - let conn = self.context.store.conn()?; + pub fn prepare_key_update(&self, conn: &DbConnection) -> Result { let intent = conn.insert_group_intent(NewGroupIntent::new( IntentKind::KeyUpdate, self.group_id.clone(), vec![], ))?; + Ok(intent) + } + + // Update this installation's leaf key in the group by creating a key update commit + pub async fn key_update(&self, client: &Client) -> Result<(), GroupError> + where + ApiClient: XmtpApi, + { + let conn = self.context.store.conn()?; + let intent = self.prepare_key_update(&conn)?; self.sync_until_intent_resolved(&conn.into(), intent.id, client) .await } diff --git a/xmtp_mls/src/groups/sync.rs b/xmtp_mls/src/groups/sync.rs index 4011ed742..0e6c500fd 100644 --- a/xmtp_mls/src/groups/sync.rs +++ b/xmtp_mls/src/groups/sync.rs @@ -19,8 +19,8 @@ use crate::{ client::MessageProcessingError, codecs::{group_updated::GroupUpdatedCodec, ContentCodec}, configuration::{ - GRPC_DATA_LIMIT, MAX_GROUP_SIZE, MAX_INTENT_PUBLISH_ATTEMPTS, MAX_PAST_EPOCHS, - SYNC_UPDATE_INSTALLATIONS_INTERVAL_NS, + GROUP_KEY_ROTATION_INTERVAL_NS, GRPC_DATA_LIMIT, MAX_GROUP_SIZE, + MAX_INTENT_PUBLISH_ATTEMPTS, MAX_PAST_EPOCHS, SYNC_UPDATE_INSTALLATIONS_INTERVAL_NS, }, groups::{intents::UpdateMetadataIntentData, validated_commit::ValidatedCommit}, hpke::{encrypt_welcome, HpkeError}, @@ -975,6 +975,21 @@ impl MlsGroup { Ok(()) } + pub fn maybe_prepare_key_update(&self) -> Result<(), GroupError> { + self.context.store.transaction(|provider| { + let conn = provider.conn_ref(); + let last_rotated_at_ns = conn.get_rotated_at_ns(self.group_id.clone())?; + let now_ns = crate::utils::time::now_ns(); + let elapsed_ns = now_ns - last_rotated_at_ns; + if elapsed_ns > GROUP_KEY_ROTATION_INTERVAL_NS { + self.prepare_key_update(conn)?; + conn.update_rotated_at_ns(self.group_id.clone())?; + } + Ok::<(), GroupError>(()) + })?; + Ok(()) + } + pub async fn maybe_update_installations( &self, provider: &XmtpOpenMlsProvider, From 558ca7d0000d94a8766888cab4897df4fc13c5ef Mon Sep 17 00:00:00 2001 From: Richard Hua Date: Wed, 2 Oct 2024 21:45:02 -0700 Subject: [PATCH 3/8] Refactor to add queue_intent method --- xmtp_mls/src/groups/intents.rs | 47 ++++++++++++++++++++++++++++++++++ xmtp_mls/src/groups/mod.rs | 24 ++++++----------- xmtp_mls/src/groups/sync.rs | 19 ++------------ 3 files changed, 57 insertions(+), 33 deletions(-) diff --git a/xmtp_mls/src/groups/intents.rs b/xmtp_mls/src/groups/intents.rs index f5394fa74..451d16073 100644 --- a/xmtp_mls/src/groups/intents.rs +++ b/xmtp_mls/src/groups/intents.rs @@ -26,6 +26,11 @@ use xmtp_proto::xmtp::mls::database::{ }; use crate::{ + configuration::GROUP_KEY_ROTATION_INTERVAL_NS, + storage::{ + db_connection::DbConnection, + group_intent::{IntentKind, NewGroupIntent, StoredGroupIntent}, + }, types::Address, verified_key_package_v2::{KeyPackageVerificationError, VerifiedKeyPackageV2}, }; @@ -34,6 +39,7 @@ use super::{ group_membership::GroupMembership, group_mutable_metadata::MetadataField, group_permissions::{MembershipPolicies, MetadataPolicies, PermissionsPolicies}, + GroupError, MlsGroup, }; #[derive(Debug, Error)] @@ -48,6 +54,47 @@ pub enum IntentError { Generic(String), } +// Here +impl MlsGroup { + pub fn queue_intent(&self, to_save: NewGroupIntent) -> Result { + self.context.store.transaction(|provider| { + let conn = provider.conn_ref(); + Ok(self.insert_group_intent_with_conn(conn, to_save)?) + }) + } + + pub fn insert_group_intent_with_conn( + &self, + conn: &DbConnection, + to_save: NewGroupIntent, + ) -> Result { + if to_save.kind == IntentKind::SendMessage { + self.maybe_insert_key_update_intent(conn)?; + } + + let intent = conn.insert_group_intent(to_save)?; + + if intent.kind != IntentKind::SendMessage { + conn.update_rotated_at_ns(self.group_id.clone())?; + } + + Ok(intent) + } + + fn maybe_insert_key_update_intent(&self, conn: &DbConnection) -> Result<(), GroupError> { + let last_rotated_at_ns = conn.get_rotated_at_ns(self.group_id.clone())?; + let now_ns = crate::utils::time::now_ns(); + let elapsed_ns = now_ns - last_rotated_at_ns; + if elapsed_ns > GROUP_KEY_ROTATION_INTERVAL_NS { + self.insert_group_intent_with_conn( + conn, + NewGroupIntent::new(IntentKind::KeyUpdate, self.group_id.clone(), vec![]), + )?; + } + Ok(()) + } +} + #[derive(Debug, Clone)] pub struct SendMessageIntentData { pub message: Vec, diff --git a/xmtp_mls/src/groups/mod.rs b/xmtp_mls/src/groups/mod.rs index 74920bbf0..bc07083cb 100644 --- a/xmtp_mls/src/groups/mod.rs +++ b/xmtp_mls/src/groups/mod.rs @@ -86,7 +86,7 @@ use crate::{ consent_record::{ConsentState, ConsentType, StoredConsentRecord}, db_connection::DbConnection, group::{GroupMembershipState, Purpose, StoredGroup}, - group_intent::{IntentKind, NewGroupIntent, StoredGroupIntent}, + group_intent::{IntentKind, NewGroupIntent}, group_message::{DeliveryStatus, GroupMessageKind, StoredGroupMessage}, sql_key_store, }, @@ -564,11 +564,10 @@ impl MlsGroup { .encode(&mut encoded_envelope) .map_err(GroupError::EncodeError)?; - self.maybe_prepare_key_update()?; let intent_data: Vec = SendMessageIntentData::new(encoded_envelope).into(); let intent = NewGroupIntent::new(IntentKind::SendMessage, self.group_id.clone(), intent_data); - intent.store(conn)?; + conn.insert_group_intent(intent)?; // store this unpublished message locally before sending let message_id = calculate_message_id(&self.group_id, message, &now.to_string()); @@ -1021,24 +1020,17 @@ impl MlsGroup { Ok(()) } - pub fn prepare_key_update(&self, conn: &DbConnection) -> Result { - let intent = conn.insert_group_intent(NewGroupIntent::new( - IntentKind::KeyUpdate, - self.group_id.clone(), - vec![], - ))?; - - Ok(intent) - } - // Update this installation's leaf key in the group by creating a key update commit pub async fn key_update(&self, client: &Client) -> Result<(), GroupError> where ApiClient: XmtpApi, { - let conn = self.context.store.conn()?; - let intent = self.prepare_key_update(&conn)?; - self.sync_until_intent_resolved(&conn.into(), intent.id, client) + let intent = self.queue_intent(NewGroupIntent::new( + IntentKind::KeyUpdate, + self.group_id.clone(), + vec![], + ))?; + self.sync_until_intent_resolved(&client.mls_provider()?, intent.id, client) .await } diff --git a/xmtp_mls/src/groups/sync.rs b/xmtp_mls/src/groups/sync.rs index 0e6c500fd..4011ed742 100644 --- a/xmtp_mls/src/groups/sync.rs +++ b/xmtp_mls/src/groups/sync.rs @@ -19,8 +19,8 @@ use crate::{ client::MessageProcessingError, codecs::{group_updated::GroupUpdatedCodec, ContentCodec}, configuration::{ - GROUP_KEY_ROTATION_INTERVAL_NS, GRPC_DATA_LIMIT, MAX_GROUP_SIZE, - MAX_INTENT_PUBLISH_ATTEMPTS, MAX_PAST_EPOCHS, SYNC_UPDATE_INSTALLATIONS_INTERVAL_NS, + GRPC_DATA_LIMIT, MAX_GROUP_SIZE, MAX_INTENT_PUBLISH_ATTEMPTS, MAX_PAST_EPOCHS, + SYNC_UPDATE_INSTALLATIONS_INTERVAL_NS, }, groups::{intents::UpdateMetadataIntentData, validated_commit::ValidatedCommit}, hpke::{encrypt_welcome, HpkeError}, @@ -975,21 +975,6 @@ impl MlsGroup { Ok(()) } - pub fn maybe_prepare_key_update(&self) -> Result<(), GroupError> { - self.context.store.transaction(|provider| { - let conn = provider.conn_ref(); - let last_rotated_at_ns = conn.get_rotated_at_ns(self.group_id.clone())?; - let now_ns = crate::utils::time::now_ns(); - let elapsed_ns = now_ns - last_rotated_at_ns; - if elapsed_ns > GROUP_KEY_ROTATION_INTERVAL_NS { - self.prepare_key_update(conn)?; - conn.update_rotated_at_ns(self.group_id.clone())?; - } - Ok::<(), GroupError>(()) - })?; - Ok(()) - } - pub async fn maybe_update_installations( &self, provider: &XmtpOpenMlsProvider, From 5ef711e4d227a541fc7ad417bd195f8478b43b07 Mon Sep 17 00:00:00 2001 From: Richard Hua Date: Wed, 2 Oct 2024 22:22:05 -0700 Subject: [PATCH 4/8] Refactor remaining callsites --- xmtp_mls/src/groups/intents.rs | 28 +++-- xmtp_mls/src/groups/mod.rs | 103 +++++------------- xmtp_mls/src/groups/sync.rs | 9 +- .../storage/encrypted_store/group_intent.rs | 3 + 4 files changed, 53 insertions(+), 90 deletions(-) diff --git a/xmtp_mls/src/groups/intents.rs b/xmtp_mls/src/groups/intents.rs index 451d16073..c4cdcba3e 100644 --- a/xmtp_mls/src/groups/intents.rs +++ b/xmtp_mls/src/groups/intents.rs @@ -56,25 +56,34 @@ pub enum IntentError { // Here impl MlsGroup { - pub fn queue_intent(&self, to_save: NewGroupIntent) -> Result { + pub fn queue_intent( + &self, + intent_kind: IntentKind, + intent_data: Vec, + ) -> Result { self.context.store.transaction(|provider| { let conn = provider.conn_ref(); - Ok(self.insert_group_intent_with_conn(conn, to_save)?) + Ok(self.queue_intent_with_conn(conn, intent_kind, intent_data)?) }) } - pub fn insert_group_intent_with_conn( + pub fn queue_intent_with_conn( &self, conn: &DbConnection, - to_save: NewGroupIntent, + intent_kind: IntentKind, + intent_data: Vec, ) -> Result { - if to_save.kind == IntentKind::SendMessage { + if intent_kind == IntentKind::SendMessage { self.maybe_insert_key_update_intent(conn)?; } - let intent = conn.insert_group_intent(to_save)?; + let intent = conn.insert_group_intent(NewGroupIntent::new( + intent_kind.clone(), + self.group_id.clone(), + intent_data, + ))?; - if intent.kind != IntentKind::SendMessage { + if intent_kind != IntentKind::SendMessage { conn.update_rotated_at_ns(self.group_id.clone())?; } @@ -86,10 +95,7 @@ impl MlsGroup { let now_ns = crate::utils::time::now_ns(); let elapsed_ns = now_ns - last_rotated_at_ns; if elapsed_ns > GROUP_KEY_ROTATION_INTERVAL_NS { - self.insert_group_intent_with_conn( - conn, - NewGroupIntent::new(IntentKind::KeyUpdate, self.group_id.clone(), vec![]), - )?; + self.queue_intent_with_conn(conn, IntentKind::KeyUpdate, vec![])?; } Ok(()) } diff --git a/xmtp_mls/src/groups/mod.rs b/xmtp_mls/src/groups/mod.rs index bc07083cb..8ece09649 100644 --- a/xmtp_mls/src/groups/mod.rs +++ b/xmtp_mls/src/groups/mod.rs @@ -86,7 +86,7 @@ use crate::{ consent_record::{ConsentState, ConsentType, StoredConsentRecord}, db_connection::DbConnection, group::{GroupMembershipState, Purpose, StoredGroup}, - group_intent::{IntentKind, NewGroupIntent}, + group_intent::IntentKind, group_message::{DeliveryStatus, GroupMessageKind, StoredGroupMessage}, sql_key_store, }, @@ -565,9 +565,7 @@ impl MlsGroup { .map_err(GroupError::EncodeError)?; let intent_data: Vec = SendMessageIntentData::new(encoded_envelope).into(); - let intent = - NewGroupIntent::new(IntentKind::SendMessage, self.group_id.clone(), intent_data); - conn.insert_group_intent(intent)?; + self.queue_intent_with_conn(conn, IntentKind::SendMessage, intent_data)?; // store this unpublished message locally before sending let message_id = calculate_message_id(&self.group_id, message, &now.to_string()); @@ -676,13 +674,11 @@ impl MlsGroup { return Ok(()); } - let intent = provider - .conn_ref() - .insert_group_intent(NewGroupIntent::new( - IntentKind::UpdateGroupMembership, - self.group_id.clone(), - intent_data.into(), - ))?; + let intent = self.queue_intent_with_conn( + provider.conn_ref(), + IntentKind::UpdateGroupMembership, + intent_data.into(), + )?; self.sync_until_intent_resolved(&provider, intent.id, client) .await @@ -727,13 +723,11 @@ impl MlsGroup { .get_membership_update_intent(client, &provider, vec![], inbox_ids) .await?; - let intent = provider - .conn_ref() - .insert_group_intent(NewGroupIntent::new( - IntentKind::UpdateGroupMembership, - self.group_id.clone(), - intent_data.into(), - ))?; + let intent = self.queue_intent_with_conn( + provider.conn_ref(), + IntentKind::UpdateGroupMembership, + intent_data.into(), + )?; self.sync_until_intent_resolved(&provider, intent.id, client) .await @@ -749,16 +743,11 @@ impl MlsGroup { where ApiClient: XmtpApi, { - let conn = self.context.store.conn()?; let intent_data: Vec = UpdateMetadataIntentData::new_update_group_name(group_name).into(); - let intent = conn.insert_group_intent(NewGroupIntent::new( - IntentKind::MetadataUpdate, - self.group_id.clone(), - intent_data, - ))?; + let intent = self.queue_intent(IntentKind::MetadataUpdate, intent_data)?; - self.sync_until_intent_resolved(&conn.into(), intent.id, client) + self.sync_until_intent_resolved(&client.mls_provider()?, intent.id, client) .await } @@ -770,8 +759,6 @@ impl MlsGroup { permission_policy: PermissionPolicyOption, metadata_field: Option, ) -> Result<(), GroupError> { - let conn = client.store().conn()?; - if permission_update_type == PermissionUpdateType::UpdateMetadata && metadata_field.is_none() { @@ -785,13 +772,9 @@ impl MlsGroup { ) .into(); - let intent = conn.insert_group_intent(NewGroupIntent::new( - IntentKind::UpdatePermission, - self.group_id.clone(), - intent_data, - ))?; + let intent = self.queue_intent(IntentKind::UpdatePermission, intent_data)?; - self.sync_until_intent_resolved(&conn.into(), intent.id, client) + self.sync_until_intent_resolved(&client.mls_provider()?, intent.id, client) .await } @@ -818,16 +801,11 @@ impl MlsGroup { where ApiClient: XmtpApi, { - let conn = self.context.store.conn()?; let intent_data: Vec = UpdateMetadataIntentData::new_update_group_description(group_description).into(); - let intent = conn.insert_group_intent(NewGroupIntent::new( - IntentKind::MetadataUpdate, - self.group_id.clone(), - intent_data, - ))?; + let intent = self.queue_intent(IntentKind::MetadataUpdate, intent_data)?; - self.sync_until_intent_resolved(&conn.into(), intent.id, client) + self.sync_until_intent_resolved(&client.mls_provider()?, intent.id, client) .await } @@ -853,17 +831,12 @@ impl MlsGroup { where ApiClient: XmtpApi, { - let conn = self.context.store.conn()?; let intent_data: Vec = UpdateMetadataIntentData::new_update_group_image_url_square(group_image_url_square) .into(); - let intent = conn.insert_group_intent(NewGroupIntent::new( - IntentKind::MetadataUpdate, - self.group_id.clone(), - intent_data, - ))?; + let intent = self.queue_intent(IntentKind::MetadataUpdate, intent_data)?; - self.sync_until_intent_resolved(&conn.into(), intent.id, client) + self.sync_until_intent_resolved(&client.mls_provider()?, intent.id, client) .await } @@ -892,16 +865,11 @@ impl MlsGroup { where ApiClient: XmtpApi, { - let conn = self.context.store.conn()?; let intent_data: Vec = UpdateMetadataIntentData::new_update_group_pinned_frame_url(pinned_frame_url).into(); - let intent = conn.insert_group_intent(NewGroupIntent::new( - IntentKind::MetadataUpdate, - self.group_id.clone(), - intent_data, - ))?; + let intent = self.queue_intent(IntentKind::MetadataUpdate, intent_data)?; - self.sync_until_intent_resolved(&conn.into(), intent.id, client) + self.sync_until_intent_resolved(&client.mls_provider()?, intent.id, client) .await } @@ -966,7 +934,6 @@ impl MlsGroup { where ApiClient: XmtpApi, { - let conn = self.context.store.conn()?; let intent_action_type = match action_type { UpdateAdminListType::Add => AdminListActionType::Add, UpdateAdminListType::Remove => AdminListActionType::Remove, @@ -975,13 +942,9 @@ impl MlsGroup { }; let intent_data: Vec = UpdateAdminListIntentData::new(intent_action_type, inbox_id).into(); - let intent = conn.insert_group_intent(NewGroupIntent::new( - IntentKind::UpdateAdminList, - self.group_id.clone(), - intent_data, - ))?; + let intent = self.queue_intent(IntentKind::UpdateAdminList, intent_data)?; - self.sync_until_intent_resolved(&conn.into(), intent.id, client) + self.sync_until_intent_resolved(&client.mls_provider()?, intent.id, client) .await } @@ -1025,11 +988,7 @@ impl MlsGroup { where ApiClient: XmtpApi, { - let intent = self.queue_intent(NewGroupIntent::new( - IntentKind::KeyUpdate, - self.group_id.clone(), - vec![], - ))?; + let intent = self.queue_intent(IntentKind::KeyUpdate, vec![])?; self.sync_until_intent_resolved(&client.mls_provider()?, intent.id, client) .await } @@ -1384,7 +1343,7 @@ mod tests { }, storage::{ consent_record::ConsentState, - group_intent::{IntentKind, IntentState, NewGroupIntent}, + group_intent::{IntentKind, IntentState}, group_message::{GroupMessageKind, StoredGroupMessage}, }, xmtp_openmls_provider::XmtpOpenMlsProvider, @@ -3159,13 +3118,9 @@ mod tests { return; } - let conn = provider.conn_ref(); - conn.insert_group_intent(NewGroupIntent::new( - IntentKind::UpdateGroupMembership, - group.group_id.clone(), - intent_data.into(), - )) - .unwrap(); + group + .queue_intent(IntentKind::UpdateGroupMembership, intent_data.into()) + .unwrap(); } /** diff --git a/xmtp_mls/src/groups/sync.rs b/xmtp_mls/src/groups/sync.rs index 4011ed742..bbfb944c3 100644 --- a/xmtp_mls/src/groups/sync.rs +++ b/xmtp_mls/src/groups/sync.rs @@ -30,7 +30,7 @@ use crate::{ retry_async, storage::{ db_connection::DbConnection, - group_intent::{IntentKind, IntentState, NewGroupIntent, StoredGroupIntent, ID}, + group_intent::{IntentKind, IntentState, StoredGroupIntent, ID}, group_message::{DeliveryStatus, GroupMessageKind, StoredGroupMessage}, refresh_state::EntityKind, serialization::{db_deserialize, db_serialize}, @@ -1032,12 +1032,11 @@ impl MlsGroup { debug!("Adding missing installations {:?}", intent_data); - let conn = provider.conn_ref(); - let intent = conn.insert_group_intent(NewGroupIntent::new( + let intent = self.queue_intent_with_conn( + provider.conn_ref(), IntentKind::UpdateGroupMembership, - self.group_id.clone(), intent_data.into(), - ))?; + )?; self.sync_until_intent_resolved(provider, intent.id, client) .await diff --git a/xmtp_mls/src/storage/encrypted_store/group_intent.rs b/xmtp_mls/src/storage/encrypted_store/group_intent.rs index 4ff7615e7..db7cc9ec3 100644 --- a/xmtp_mls/src/storage/encrypted_store/group_intent.rs +++ b/xmtp_mls/src/storage/encrypted_store/group_intent.rs @@ -124,6 +124,9 @@ impl Delete for DbConnection { } } +/// NewGroupIntent is the data needed to create a new group intent. +/// Do not use this struct directly outside of the storage module. +/// Use the `queue_intent` method on `MlsGroup` instead. #[derive(Insertable, Debug, PartialEq, Clone)] #[diesel(table_name = group_intents)] pub struct NewGroupIntent { From b4bb6c89cad66648cf18671cd93e6561ac84680f Mon Sep 17 00:00:00 2001 From: Richard Hua Date: Wed, 2 Oct 2024 22:36:30 -0700 Subject: [PATCH 5/8] Fix lints - still need to fix tests --- xmtp_mls/src/client.rs | 2 +- xmtp_mls/src/groups/intents.rs | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/xmtp_mls/src/client.rs b/xmtp_mls/src/client.rs index 57f39137e..51a0de23f 100644 --- a/xmtp_mls/src/client.rs +++ b/xmtp_mls/src/client.rs @@ -456,7 +456,7 @@ where &self.context } - /// + #[allow(clippy::borrowed_box)] pub fn smart_contract_signature_verifier(&self) -> &Box { &self.scw_verifier } diff --git a/xmtp_mls/src/groups/intents.rs b/xmtp_mls/src/groups/intents.rs index c4cdcba3e..efe0daeff 100644 --- a/xmtp_mls/src/groups/intents.rs +++ b/xmtp_mls/src/groups/intents.rs @@ -54,7 +54,6 @@ pub enum IntentError { Generic(String), } -// Here impl MlsGroup { pub fn queue_intent( &self, @@ -63,7 +62,7 @@ impl MlsGroup { ) -> Result { self.context.store.transaction(|provider| { let conn = provider.conn_ref(); - Ok(self.queue_intent_with_conn(conn, intent_kind, intent_data)?) + self.queue_intent_with_conn(conn, intent_kind, intent_data) }) } @@ -78,7 +77,7 @@ impl MlsGroup { } let intent = conn.insert_group_intent(NewGroupIntent::new( - intent_kind.clone(), + intent_kind, self.group_id.clone(), intent_data, ))?; From 710ec3cd8b417325b93374e1fdd077e0478bd6f8 Mon Sep 17 00:00:00 2001 From: Richard Hua Date: Thu, 3 Oct 2024 11:01:19 -0700 Subject: [PATCH 6/8] Add key rotation unit test --- xmtp_mls/src/groups/intents.rs | 112 +++++++++++++++++++++++++++++++++ 1 file changed, 112 insertions(+) diff --git a/xmtp_mls/src/groups/intents.rs b/xmtp_mls/src/groups/intents.rs index efe0daeff..d1f8a1ed7 100644 --- a/xmtp_mls/src/groups/intents.rs +++ b/xmtp_mls/src/groups/intents.rs @@ -721,6 +721,15 @@ impl TryFrom> for PostCommitAction { #[cfg(test)] mod tests { + use openmls::prelude::{MlsMessageBodyIn, MlsMessageIn, ProcessedMessageContent}; + use tls_codec::Deserialize; + use xmtp_cryptography::utils::generate_local_wallet; + use xmtp_proto::xmtp::mls::api::v1::{group_message, GroupMessage}; + + use crate::{ + builder::ClientBuilder, groups::GroupMetadataOptions, utils::test::TestClient, Client, + }; + use super::*; #[test] @@ -761,4 +770,107 @@ mod tests { assert_eq!(intent.field_value, restored_intent.field_value); } + + #[tokio::test(flavor = "multi_thread", worker_threads = 1)] + async fn test_key_rotation_before_first_message() { + let client_a = ClientBuilder::new_test_client(&generate_local_wallet()).await; + let client_b = ClientBuilder::new_test_client(&generate_local_wallet()).await; + + // client A makes a group with client B, and then sends a message to client B. + let group_a = client_a + .create_group(None, GroupMetadataOptions::default()) + .expect("create group"); + group_a + .add_members_by_inbox_id(&client_a, vec![client_b.inbox_id()]) + .await + .unwrap(); + group_a + .send_message(b"First message from A", &client_a) + .await + .unwrap(); + + // No key rotation needed, because A's commit to add B already performs a rotation. + // Group should have a commit to add client B, followed by A's message. + verify_num_payloads_in_group(&client_a, &group_a, 2).await; + + // Client B sends a message to Client A + let groups_b = client_b.sync_welcomes().await.unwrap(); + assert_eq!(groups_b.len(), 1); + let group_b = groups_b[0].clone(); + group_b + .send_message(b"First message from B", &client_b) + .await + .expect("send message"); + + // B must perform a key rotation before sending their first message. + // Group should have a commit to add B, A's message, B's key rotation and then B's message. + let payloads_a = verify_num_payloads_in_group(&client_a, &group_a, 4).await; + let payloads_b = verify_num_payloads_in_group(&client_b, &group_b, 4).await; + + // Verify key rotation payload + for i in 0..payloads_a.len() { + assert_eq!(payloads_a[i].encode_to_vec(), payloads_b[i].encode_to_vec()); + } + verify_commit_updates_leaf_node(&client_a, &group_a, &payloads_a[2]); + + // Client B sends another message to Client A, and Client A sends another message to Client B. + group_b + .send_message(b"Second message from B", &client_b) + .await + .expect("send message"); + group_a + .send_message(b"Second message from A", &client_a) + .await + .expect("send message"); + + // Group should only have 2 additional messages - no more key rotations needed. + verify_num_payloads_in_group(&client_a, &group_a, 6).await; + verify_num_payloads_in_group(&client_b, &group_b, 6).await; + } + + async fn verify_num_payloads_in_group( + client: &Client, + group: &MlsGroup, + num_messages: usize, + ) -> Vec { + let messages = client + .api_client + .query_group_messages(group.group_id.clone(), None) + .await + .unwrap(); + assert_eq!(messages.len(), num_messages); + messages + } + + fn verify_commit_updates_leaf_node( + client: &Client, + group: &MlsGroup, + payload: &GroupMessage, + ) { + let msgv1 = match &payload.version { + Some(group_message::Version::V1(value)) => value, + _ => panic!("error msgv1"), + }; + + let mls_message_in = MlsMessageIn::tls_deserialize_exact(&msgv1.data).unwrap(); + let mls_message = match mls_message_in.extract() { + MlsMessageBodyIn::PrivateMessage(mls_message) => mls_message, + _ => panic!("error mls_message"), + }; + + let provider = client.mls_provider().unwrap(); + let mut openmls_group = group.load_mls_group(&provider).unwrap(); + let decrypted_message = openmls_group + .process_message(&provider, mls_message) + .unwrap(); + + let staged_commit = match decrypted_message.into_content() { + ProcessedMessageContent::StagedCommitMessage(staged_commit) => *staged_commit, + _ => panic!("error staged_commit"), + }; + + // Check there is indeed some updated leaf node, which means the key update works. + let path_update_leaf_node = staged_commit.update_path_leaf_node(); + assert!(path_update_leaf_node.is_some()); + } } From 93665927fcd933fbbb5995283042f27a52956a0d Mon Sep 17 00:00:00 2001 From: Richard Hua Date: Thu, 3 Oct 2024 11:25:54 -0700 Subject: [PATCH 7/8] Fix sync group logic --- xmtp_mls/src/groups/message_history.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xmtp_mls/src/groups/message_history.rs b/xmtp_mls/src/groups/message_history.rs index 3b08eb9a8..79272d37b 100644 --- a/xmtp_mls/src/groups/message_history.rs +++ b/xmtp_mls/src/groups/message_history.rs @@ -265,7 +265,7 @@ where })?; // publish the intent - if let Err(err) = sync_group.publish_intents(&conn.into(), self).await { + if let Err(err) = sync_group.publish_messages(self).await { tracing::error!("error publishing sync group intents: {:?}", err); } Ok(()) From e305e896034b6349989600e9f092ffb4a02c0151 Mon Sep 17 00:00:00 2001 From: Richard Hua Date: Thu, 3 Oct 2024 15:13:54 -0700 Subject: [PATCH 8/8] File issue and add workaround for bindings tests --- bindings_ffi/src/mls.rs | 6 ++++++ xmtp_mls/src/groups/sync.rs | 4 ++++ 2 files changed, 10 insertions(+) diff --git a/bindings_ffi/src/mls.rs b/bindings_ffi/src/mls.rs index b081cf455..f0bc554c6 100644 --- a/bindings_ffi/src/mls.rs +++ b/bindings_ffi/src/mls.rs @@ -2512,6 +2512,9 @@ mod tests { let bo_group = bo.group(alix_group.id()).unwrap(); bo_group.send("bo1".as_bytes().to_vec()).await.unwrap(); + // Temporary workaround for OpenMLS issue - make sure Alix's epoch is up-to-date + // https://github.com/xmtp/libxmtp/issues/1116 + alix_group.sync().await.unwrap(); alix_group.send("alix1".as_bytes().to_vec()).await.unwrap(); // Move the group forward by 3 epochs (as Alix's max_past_epochs is @@ -2715,6 +2718,9 @@ mod tests { log::info!("Caro sending fifth message"); // Caro sends a message in the group caro_group.update_installations().await.unwrap(); + // Temporary workaround for OpenMLS issue - make sure Caro's epoch is up-to-date + // https://github.com/xmtp/libxmtp/issues/1116 + caro_group.sync().await.unwrap(); caro_group .send("Fifth message".as_bytes().to_vec()) .await diff --git a/xmtp_mls/src/groups/sync.rs b/xmtp_mls/src/groups/sync.rs index bbfb944c3..6862568b5 100644 --- a/xmtp_mls/src/groups/sync.rs +++ b/xmtp_mls/src/groups/sync.rs @@ -537,6 +537,10 @@ impl MlsGroup { let intent = provider .conn_ref() .find_group_intent_by_payload_hash(sha256(envelope.data.as_slice())); + tracing::info!( + "Processing envelope with hash {:?}", + hex::encode(sha256(envelope.data.as_slice())) + ); match intent { // Intent with the payload hash matches