Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Regularly rotate leaf node encryption keys #1108

Merged
merged 8 commits into from
Oct 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions bindings_ffi/src/mls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
ALTER TABLE GROUPS
DROP COLUMN rotated_at_ns;

Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
ALTER TABLE GROUPS
ADD COLUMN rotated_at_ns BIGINT NOT NULL DEFAULT 0;

2 changes: 1 addition & 1 deletion xmtp_mls/migrations/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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/`.
2 changes: 1 addition & 1 deletion xmtp_mls/src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -456,7 +456,7 @@ where
&self.context
}

///
#[allow(clippy::borrowed_box)]
pub fn smart_contract_signature_verifier(&self) -> &Box<dyn SmartContractSignatureVerifier> {
&self.scw_verifier
}
Expand Down
4 changes: 4 additions & 0 deletions xmtp_mls/src/configuration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
164 changes: 164 additions & 0 deletions xmtp_mls/src/groups/intents.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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},
};
Expand All @@ -34,6 +39,7 @@ use super::{
group_membership::GroupMembership,
group_mutable_metadata::MetadataField,
group_permissions::{MembershipPolicies, MetadataPolicies, PermissionsPolicies},
GroupError, MlsGroup,
};

#[derive(Debug, Error)]
Expand All @@ -48,6 +54,52 @@ pub enum IntentError {
Generic(String),
}

impl MlsGroup {
pub fn queue_intent(
&self,
intent_kind: IntentKind,
intent_data: Vec<u8>,
) -> Result<StoredGroupIntent, GroupError> {
self.context.store.transaction(|provider| {
let conn = provider.conn_ref();
self.queue_intent_with_conn(conn, intent_kind, intent_data)
})
}

pub fn queue_intent_with_conn(
&self,
conn: &DbConnection,
intent_kind: IntentKind,
intent_data: Vec<u8>,
) -> Result<StoredGroupIntent, GroupError> {
if intent_kind == IntentKind::SendMessage {
self.maybe_insert_key_update_intent(conn)?;
}

let intent = conn.insert_group_intent(NewGroupIntent::new(
intent_kind,
self.group_id.clone(),
intent_data,
))?;

if intent_kind != IntentKind::SendMessage {
conn.update_rotated_at_ns(self.group_id.clone())?;
Copy link
Contributor Author

@richardhuaaa richardhuaaa Oct 3, 2024

Choose a reason for hiding this comment

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

We update the timestamp here when the intent is queued, rather than when the commit is successful, because otherwise we may queue up multiple key rotations if the user sends multiple messages.

The intent is not technically successful at this point - it's possible that the commit publishing fails later on after a few retries. I'm assuming that this should be rare and not exploitable.

}

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.queue_intent_with_conn(conn, IntentKind::KeyUpdate, vec![])?;
}
Ok(())
}
}

#[derive(Debug, Clone)]
pub struct SendMessageIntentData {
pub message: Vec<u8>,
Expand Down Expand Up @@ -669,6 +721,15 @@ impl TryFrom<Vec<u8>> 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]
Expand Down Expand Up @@ -709,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<TestClient>,
group: &MlsGroup,
num_messages: usize,
) -> Vec<GroupMessage> {
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<TestClient>,
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());
}
}
2 changes: 1 addition & 1 deletion xmtp_mls/src/groups/message_history.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

publish_intents will stop at the first commit - so if a key rotation or an add_missing_installations is queued up first, the message history reply won't actually be sent. Instead, use publish_messages after a prepare_message call, just like apps do.

tracing::error!("error publishing sync group intents: {:?}", err);
}
Ok(())
Expand Down
Loading