From fe8bd2fdf334a32002d0399fa00ae9b0a21da6e5 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 14 May 2025 18:01:42 +0100 Subject: [PATCH] refactor(crypto): Break `get_or_update_verification_state` in two Split this into `get_room_event_verification_state` and `get_or_update_sender_data`, which I think is a bit clearer. --- crates/matrix-sdk-crypto/src/machine/mod.rs | 63 ++++++++++++++++--- .../tests/decryption_verification_state.rs | 4 +- 2 files changed, 57 insertions(+), 10 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/machine/mod.rs b/crates/matrix-sdk-crypto/src/machine/mod.rs index 54b7da16f..48957a8e4 100644 --- a/crates/matrix-sdk-crypto/src/machine/mod.rs +++ b/crates/matrix-sdk-crypto/src/machine/mod.rs @@ -1521,20 +1521,50 @@ impl OlmMachine { self.inner.key_request_machine.request_key(room_id, &event).await } - /// Find whether the supplied session is verified, and provide - /// explanation of what is missing/wrong if not. + /// Find whether an event decrypted via the supplied session is verified, + /// and provide explanation of what is missing/wrong if not. + /// + /// Stores the updated [`SenderData`] for the session in the store + /// if we find an updated value for it. + /// + /// # Arguments + /// + /// * `session` - The inbound Megolm session that was used to decrypt the + /// event. + /// * `sender` - The `sender` of that event (as claimed by the envelope of + /// the event). + async fn get_room_event_verification_state( + &self, + session: &InboundGroupSession, + sender: &UserId, + ) -> MegolmResult<(VerificationState, Option)> { + let sender_data = self.get_or_update_sender_data(session, sender).await?; + + let (verification_state, device_id) = + sender_data_to_verification_state(sender_data, session.has_been_imported()); + + Ok((verification_state, device_id)) + } + + /// Get an up-to-date [`SenderData`] for the given session, suitable for + /// determining if messages decrypted using that session are verified. /// /// Checks both the stored verification state of the session and a /// recalculated verification state based on our current knowledge, and /// returns the more trusted of the two. /// - /// Store the updated [`SenderData`] for this session in the store + /// Stores the updated [`SenderData`] for the session in the store /// if we find an updated value for it. - async fn get_or_update_verification_state( + /// + /// # Arguments + /// + /// * `session` - The Megolm session that was used to decrypt the event. + /// * `sender` - The claimed sender of that event. + async fn get_or_update_sender_data( &self, session: &InboundGroupSession, sender: &UserId, - ) -> MegolmResult<(VerificationState, Option)> { + ) -> MegolmResult { /// Whether we should recalculate the Megolm sender's data, given the /// current sender data. We only want to recalculate if it might /// increase trust and allow us to decrypt messages that we @@ -1555,7 +1585,24 @@ impl OlmMachine { } let sender_data = if should_recalculate_sender_data(&session.sender_data) { - // The session is not sure of the sender yet. Calculate it. + // The session is not sure of the sender yet. Try to find a matching device + // belonging to the claimed sender of the recently-received event. + // + // It's worth noting that this could in theory result in unintuitive changes, + // like a session which initially appears to belong to Alice turning into a + // session which belongs to Bob [1]. This could mean that a session initially + // successfully decrypts events from Alice, but then stops decrypting those same + // events once we get an update. + // + // That's ok though: if we get good evidence that the session belongs to Bob, + // it's correct to update the session even if we previously had weak + // evidence it belonged to Alice. + // + // [1] For example: maybe Alice and Bob both publish devices with the *same* + // keys (presumably because they are colluding). Initially we think + // the session belongs to Alice, but then we do a device lookup for + // Bob, we find a matching device with a cross-signature, so prefer + // that. let calculated_sender_data = SenderDataFinder::find_using_curve_key( self.store(), session.sender_key(), @@ -1581,7 +1628,7 @@ impl OlmMachine { session.sender_data.clone() }; - Ok(sender_data_to_verification_state(sender_data, session.has_been_imported())) + Ok(sender_data) } /// Request missing local secrets from our devices (cross signing private @@ -1654,7 +1701,7 @@ impl OlmMachine { sender: &UserId, ) -> MegolmResult { let (verification_state, device_id) = - self.get_or_update_verification_state(session, sender).await?; + self.get_room_event_verification_state(session, sender).await?; let sender = sender.to_owned(); diff --git a/crates/matrix-sdk-crypto/src/machine/tests/decryption_verification_state.rs b/crates/matrix-sdk-crypto/src/machine/tests/decryption_verification_state.rs index 2ec499cf9..ca617988d 100644 --- a/crates/matrix-sdk-crypto/src/machine/tests/decryption_verification_state.rs +++ b/crates/matrix-sdk-crypto/src/machine/tests/decryption_verification_state.rs @@ -358,7 +358,7 @@ async fn test_verification_states_multiple_device() { .unwrap(); let (state, _) = bob - .get_or_update_verification_state(&web_unverified_inbound_session, other_user_id) + .get_room_event_verification_state(&web_unverified_inbound_session, other_user_id) .await .unwrap(); assert_eq!(VerificationState::Unverified(VerificationLevel::UnsignedDevice), state); @@ -376,7 +376,7 @@ async fn test_verification_states_multiple_device() { .unwrap(); let (state, _) = bob - .get_or_update_verification_state(&web_signed_inbound_session, other_user_id) + .get_room_event_verification_state(&web_signed_inbound_session, other_user_id) .await .unwrap();