feat(sdk): Rotate session keys when a member leaves the room

crates/matrix-sdk-crypto/src/session_manager/group_sessions/mod.rs

@@ -123,6 +123,7 @@ impl GroupSessionCache {
     ///
     /// * `room_id` - The id of the room for which we should get the outbound
     ///   group session.
+    #[cfg(test)]
     fn get(&self, room_id: &RoomId) -> Option<OutboundGroupSession> {
         self.sessions.read().get(room_id).cloned()
     }
@@ -159,7 +160,7 @@ impl GroupSessionManager {
     }
 
     pub async fn invalidate_group_session(&self, room_id: &RoomId) -> StoreResult<bool> {
-        if let Some(s) = self.sessions.get(room_id) {
+        if let Some(s) = self.sessions.get_or_load(room_id).await {
             s.invalidate_session();
 
             let mut changes = Changes::default();

crates/matrix-sdk/src/encryption/mod.rs

@@ -65,7 +65,10 @@ use ruma::{
         uiaa::{AuthData, AuthType, OAuthParams, UiaaInfo},
     },
     assign,
-    events::room::{MediaSource, ThumbnailInfo},
+    events::room::{
+        MediaSource, ThumbnailInfo,
+        member::{MembershipChange, OriginalSyncRoomMemberEvent},
+    },
 };
 #[cfg(feature = "experimental-send-custom-to-device")]
 use ruma::{events::AnyToDeviceEventContent, serde::Raw, to_device::DeviceIdOrAllDevices};
@@ -87,7 +90,7 @@ use self::{
     verification::{SasVerification, Verification, VerificationRequest},
 };
 use crate::{
-    Client, Error, HttpError, Result, RumaApiError, TransmissionProgress,
+    Client, Error, HttpError, Result, Room, RumaApiError, TransmissionProgress,
     attachment::Thumbnail,
     client::{ClientInner, WeakClient},
     cross_process_lock::CrossProcessLockGuard,
@@ -1875,6 +1878,8 @@ impl Encryption {
         }));
 
         tasks.receive_historic_room_key_bundles = bundle_receiver_task;
+
+        self.setup_room_membership_session_discard_handler();
     }
 
     /// Waits for end-to-end encryption initialization tasks to finish, if any
@@ -1948,6 +1953,60 @@ impl Encryption {
         }
     }
 
+    /// Sets up a handler to rotate room keys when a user leaves a room.
+    ///
+    /// Previously, it was sufficient to check if we need to rotate the room key
+    /// prior to sending a message. However, the history sharing feature
+    /// ([MSC4268]) breaks this logic:
+    ///
+    /// 1. Alice sends a message M1 in room X;
+    /// 2. Bob invites Charlie, who joins and immediately leaves the room;
+    /// 3. Alice sends another message M2 in room X.
+    ///
+    /// Under the old logic, Alice would not rotate her key after Charlie
+    /// leaves, resulting in M2 being encrypted with the same session as M1.
+    /// This would allow Charlie to decrypt M2 if he ever gains access to
+    /// the event.
+    ///
+    /// This handler listens for changes to the room membership, and discards
+    /// the current room key if the event is a `leave` event.
+    ///
+    /// [MSC4268]: https://github.com/matrix-org/matrix-spec-proposals/pull/4268
+    fn setup_room_membership_session_discard_handler(&self) {
+        let client = WeakClient::from_client(&self.client);
+        self.client.add_event_handler(|ev: OriginalSyncRoomMemberEvent, room: Room| async move {
+            let Some(client) = client.get() else {
+                // The main client has been dropped.
+                return;
+            };
+            let Some(user_id) = client.user_id() else {
+                // We aren't logged in, so this shouldn't ever happen.
+                return;
+            };
+            let olm = client.olm_machine().await;
+            let Some(olm) = olm.as_ref() else {
+                warn!("Cannot discard session - Olm machine is not available");
+                return;
+            };
+
+            if !matches!(ev.membership_change(), MembershipChange::Left) || ev.sender == user_id {
+                // We can ignore non-leave events and those that we sent.
+                return;
+            }
+
+            debug!(room_id = ?room.room_id(), member_id = ?ev.sender, "Discarding session as a user left the room");
+
+            // Attempt to discard the current room key. This won't do anything if we don't have one,
+            // but that's fine since we will create a new room key whenever we try to send a message.
+            if let Err(e) = olm.discard_room_key(room.room_id()).await {
+                warn!(
+                    room_id = ?room.room_id(),
+                    "Error discarding room key after member leave: {e:?}"
+                );
+            }
+        });
+    }
+
     /// Encrypts then send the given content via the `/sendToDevice` end-point
     /// using Olm encryption.
     ///

testing/matrix-sdk-integration-testing/src/tests/e2ee/shared_history.rs

@@ -37,6 +37,7 @@ use matrix_sdk_ui::{
     },
 };
 use similar_asserts::assert_eq;
+use tempfile::tempdir;
 use tracing::{Instrument, info};
 
 use crate::{
@@ -1127,6 +1128,255 @@ async fn test_history_share_on_invite_respects_history_visibility() -> Result<()
     Ok(())
 }
 
+/// Test that when a user leaves a room that uses history sharing, the room key
+/// is rotated so they cannot decrypt future messages.
+#[tokio::test(flavor = "multi_thread", worker_threads = 4)]
+async fn test_history_share_on_invite_room_key_rotation() -> Result<()> {
+    let alice_span = tracing::info_span!("alice");
+    let bob_span = tracing::info_span!("bob");
+    let charlie_span = tracing::info_span!("charlie");
+
+    let alice =
+        create_encryption_enabled_client("alice", false).instrument(alice_span.clone()).await?;
+    let bob = create_encryption_enabled_client("bob", false).instrument(bob_span.clone()).await?;
+    let charlie = create_encryption_enabled_client("charlie", false)
+        .await
+        .expect("Failed to create Charlie's client");
+
+    // 1. Alice creates a room with `shared` history visibility and invites Bob.
+    let alice_room = alice
+        .create_room(assign!(CreateRoomRequest::new(), {
+            preset: Some(RoomPreset::PublicChat),
+        }))
+        .instrument(alice_span.clone())
+        .await?;
+    alice_room.enable_encryption().instrument(alice_span.clone()).await?;
+
+    alice_room.invite_user_by_id(bob.user_id().unwrap()).instrument(alice_span.clone()).await?;
+
+    bob.sync_once().instrument(bob_span.clone()).await?;
+    let bob_room = bob.join_room_by_id(alice_room.room_id()).instrument(bob_span.clone()).await?;
+
+    alice.sync_once().instrument(alice_span.clone()).await?;
+
+    // 2. Bob sends M1, which Charlie should be able to read later as Alice will
+    //    send them a key bundle.
+    let event_id_a = bob_room
+        .send(RoomMessageEventContent::text_plain("Charlie is cool!"))
+        .into_future()
+        .instrument(bob_span.clone())
+        .await?
+        .response
+        .event_id;
+
+    // Store the session ID for later comparison.
+    let event_m1 = bob_room.event(&event_id_a, None).instrument(bob_span.clone()).await?;
+    let event_m1_session_id = event_m1
+        .encryption_info()
+        .and_then(|info| info.session_id())
+        .expect("Bob should be able to check the session ID of event M1");
+
+    // 3. Alice invites Charlie; Charlie joins and receives the keys for M1.
+    alice.sync_once().instrument(alice_span.clone()).await?;
+    alice_room.invite_user_by_id(charlie.user_id().unwrap()).instrument(alice_span.clone()).await?;
+
+    let sync_response = charlie.sync_once().instrument(charlie_span.clone()).await?;
+    assert_received_room_key_bundle(sync_response);
+
+    let charlie_room =
+        charlie.join_room_by_id(alice_room.room_id()).instrument(charlie_span.clone()).await?;
+
+    charlie.sync_once().instrument(charlie_span.clone()).await?;
+
+    // Sanity check: Charlie can decrypt message M1 via the bundle.
+    let event_a = charlie_room.event(&event_id_a, None).instrument(charlie_span.clone()).await?;
+    assert!(
+        event_a.encryption_info().is_some(),
+        "Charlie should be able to decrypt message M1 via the key bundle"
+    );
+
+    // 4. Charlie leaves the room.
+    charlie_room.leave().instrument(charlie_span.clone()).await?;
+
+    // Bob syncs to learn about Charlie's departure, which should trigger key
+    // rotation.
+    bob.sync_once().instrument(bob_span.clone()).await?;
+
+    // 5. Bob sends M2. Because key rotation should have been performed, this should
+    //    be using a fresh session that hasn't been shared with Charlie.
+    let event_id_b = bob_room
+        .send(RoomMessageEventContent::text_plain("Charlie is mean!"))
+        .into_future()
+        .instrument(bob_span.clone())
+        .await?
+        .response
+        .event_id;
+
+    // Ensure the two session IDs of M1 and M2 are different
+    let event_b = bob_room.event(&event_id_b, None).instrument(bob_span.clone()).await?;
+    let event_m2_session_id = event_b
+        .encryption_info()
+        .and_then(|info| info.session_id())
+        .expect("Bob should be able to check the session ID of event M2");
+
+    assert_ne!(event_m1_session_id, event_m2_session_id, "Session was not rotated");
+
+    // 6. Charlie rejoins the room via ID.
+    charlie.sync_once().instrument(charlie_span.clone()).await?;
+    let charlie_room =
+        charlie.join_room_by_id(alice_room.room_id()).instrument(charlie_span.clone()).await?;
+
+    // 7. Charlie attempts to decrypt M2. He should not be able to, because the
+    //    session was rotated after he left the room.
+    let event_b = charlie_room.event(&event_id_b, None).instrument(charlie_span.clone()).await?;
+    assert!(
+        event_b.encryption_info().is_none(),
+        "Charlie should not be able to decrypt message M2 after rejoining"
+    );
+
+    Ok(())
+}
+
+/// A variant of the above test that verifies the room key is rotated on member
+/// leave even when the client shuts down for a short while, potentially
+/// resulting in state deltas appearing in the `/sync` response, as opposed to
+/// `timeline`.
+#[tokio::test(flavor = "multi_thread", worker_threads = 4)]
+async fn test_history_share_on_invite_room_key_rotation_with_shutdown() -> Result<()> {
+    let alice_span = tracing::info_span!("alice");
+    let bob_span = tracing::info_span!("bob");
+    let charlie_span = tracing::info_span!("charlie");
+
+    let alice =
+        create_encryption_enabled_client("alice", false).instrument(alice_span.clone()).await?;
+
+    let bob_sqlite_dir = tempdir()?;
+    let bob = SyncTokenAwareClient::new(
+        TestClientBuilder::new("bob")
+            .use_sqlite_dir(bob_sqlite_dir.path())
+            .encryption_settings(EncryptionSettings {
+                auto_enable_cross_signing: true,
+                ..Default::default()
+            })
+            .enable_share_history_on_invite(true)
+            .build()
+            .await?,
+    );
+    bob.encryption().wait_for_e2ee_initialization_tasks().await;
+
+    let charlie = create_encryption_enabled_client("charlie", false)
+        .await
+        .expect("Failed to create Charlie's client");
+
+    // 1. Alice creates a room with `shared` history visibility and invites Bob.
+    let alice_room = alice
+        .create_room(assign!(CreateRoomRequest::new(), {
+            preset: Some(RoomPreset::PublicChat),
+        }))
+        .instrument(alice_span.clone())
+        .await?;
+    alice_room.enable_encryption().instrument(alice_span.clone()).await?;
+
+    alice_room.invite_user_by_id(bob.user_id().unwrap()).instrument(alice_span.clone()).await?;
+
+    bob.sync_once().instrument(bob_span.clone()).await?;
+    let bob_room = bob.join_room_by_id(alice_room.room_id()).instrument(bob_span.clone()).await?;
+
+    alice.sync_once().instrument(alice_span.clone()).await?;
+
+    // 2. Bob sends M1, which Charlie should be able to read later as Alice will
+    //    send them a key bundle.
+    let event_id_a = bob_room
+        .send(RoomMessageEventContent::text_plain("Charlie is cool!"))
+        .into_future()
+        .instrument(bob_span.clone())
+        .await?
+        .response
+        .event_id;
+
+    // Store the session ID for later comparison.
+    let event_m1 = bob_room.event(&event_id_a, None).instrument(bob_span.clone()).await?;
+    let event_m1_session_id = event_m1
+        .encryption_info()
+        .and_then(|info| info.session_id())
+        .expect("Bob should be able to check the session ID of event M1");
+
+    // 4. Alice invites Charlie; Charlie joins and receives the keys for M1.
+    alice.sync_once().instrument(alice_span.clone()).await?;
+    alice_room.invite_user_by_id(charlie.user_id().unwrap()).instrument(alice_span.clone()).await?;
+
+    let sync_response = charlie.sync_once().instrument(charlie_span.clone()).await?;
+    assert_received_room_key_bundle(sync_response);
+
+    let charlie_room =
+        charlie.join_room_by_id(alice_room.room_id()).instrument(charlie_span.clone()).await?;
+
+    charlie.sync_once().instrument(charlie_span.clone()).await?;
+
+    // Sanity check: Charlie can decrypt message M1 via the bundle.
+    let event_a = charlie_room.event(&event_id_a, None).instrument(charlie_span.clone()).await?;
+    assert!(
+        event_a.encryption_info().is_some(),
+        "Charlie should be able to decrypt message M1 via the key bundle"
+    );
+
+    // 5. Charlie leaves the room.
+    charlie_room.leave().instrument(charlie_span.clone()).await?;
+
+    // 6. Bob starts back up, and syncs to learn about Charlie's departure, which
+    //    should trigger key rotation.
+    let bob = SyncTokenAwareClient::new(
+        TestClientBuilder::new("bob")
+            .use_sqlite_dir(bob_sqlite_dir.path())
+            .encryption_settings(EncryptionSettings {
+                auto_enable_cross_signing: true,
+                ..Default::default()
+            })
+            .enable_share_history_on_invite(true)
+            .duplicate(&bob)
+            .instrument(bob_span.clone())
+            .await?,
+    );
+    bob.encryption().wait_for_e2ee_initialization_tasks().await;
+    bob.sync_once().instrument(bob_span.clone()).await?;
+
+    let bob_room = bob.join_room_by_id(alice_room.room_id()).instrument(bob_span.clone()).await?;
+
+    // 7. Bob sends M2. Because key rotation should have been performed, this should
+    //    be using a fresh session that hasn't been shared with Charlie.
+    let event_id_b = bob_room
+        .send(RoomMessageEventContent::text_plain("Charlie is mean!"))
+        .into_future()
+        .instrument(bob_span.clone())
+        .await?
+        .response
+        .event_id;
+
+    // Ensure the two session IDs of M1 and M2 are different
+    let event_b = bob_room.event(&event_id_b, None).instrument(bob_span.clone()).await?;
+    let event_m2_session_id = event_b
+        .encryption_info()
+        .and_then(|info| info.session_id())
+        .expect("Bob should be able to check the session ID of event M2");
+
+    assert_ne!(event_m1_session_id, event_m2_session_id, "Session was not rotated");
+
+    // 8. Charlie rejoins the room via ID.
+    charlie.sync_once().instrument(charlie_span.clone()).await?;
+    let charlie_room =
+        charlie.join_room_by_id(alice_room.room_id()).instrument(charlie_span.clone()).await?;
+
+    // 9. Charlie attempts to decrypt M2. He should not be able to, because the
+    //    session was rotated after he left the room.
+    let event_b = charlie_room.event(&event_id_b, None).instrument(charlie_span.clone()).await?;
+    assert!(
+        event_b.encryption_info().is_none(),
+        "Charlie should not be able to decrypt message M2 after rejoining"
+    );
+
+    Ok(())
+}
+
 /// Creates a new encryption-enabled client with the given username and
 /// settings.
 ///