From 36b739b830986a038273028c6c675d4034fd88b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damir=20Jeli=C4=87?= Date: Tue, 14 Mar 2023 15:50:03 +0100 Subject: [PATCH 1/2] Improve the docs of the inbound group session type --- Cargo.lock | 2 +- .../src/gossiping/machine.rs | 4 +- .../src/identities/device.rs | 4 +- crates/matrix-sdk-crypto/src/machine.rs | 2 +- .../src/olm/group_sessions/inbound.rs | 144 +++++++++++++----- crates/matrix-sdk-crypto/src/store/caches.rs | 2 +- 6 files changed, 114 insertions(+), 44 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 42a8953df9e..0bff28179c8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4403,7 +4403,7 @@ dependencies = [ [[package]] name = "ruma-push-gateway-api" version = "0.7.1" -source = "git+https://github.com/ruma/ruma?rev=a399a4017a9f0b249ce848a652f2e4fe65435eaf#a399a4017a9f0b249ce848a652f2e4fe65435eaf" +source = "git+https://github.com/ruma/ruma?rev=8eea3e05490fa9a318f9ed66c3a75272e6ef0ee5#8eea3e05490fa9a318f9ed66c3a75272e6ef0ee5" dependencies = [ "js_int", "ruma-common", diff --git a/crates/matrix-sdk-crypto/src/gossiping/machine.rs b/crates/matrix-sdk-crypto/src/gossiping/machine.rs index 553f0d64463..05c6adf81cf 100644 --- a/crates/matrix-sdk-crypto/src/gossiping/machine.rs +++ b/crates/matrix-sdk-crypto/src/gossiping/machine.rs @@ -323,7 +323,7 @@ impl GossipMachine { user_id = ?device.user_id(), device_id = ?device.device_id(), session_id = session.session_id(), - room_id = ?session.room_id, + room_id = ?session.room_id(), ?message_index, "Serving a room key request", ); @@ -882,7 +882,7 @@ impl GossipMachine { info!( ?sender_key, claimed_sender_key = ?session.sender_key(), - room_id = ?session.room_id, + room_id = ?session.room_id(), session_id = session.session_id(), algorithm = ?session.algorithm(), "Received a forwarded room key but we already have a better version of it", diff --git a/crates/matrix-sdk-crypto/src/identities/device.rs b/crates/matrix-sdk-crypto/src/identities/device.rs index e5ea4899b44..f0a0932577c 100644 --- a/crates/matrix-sdk-crypto/src/identities/device.rs +++ b/crates/matrix-sdk-crypto/src/identities/device.rs @@ -181,7 +181,7 @@ impl Device { // know that the room key belongs to this device. Ok(false) } else if let Some(key) = - session.signing_keys.get(&DeviceKeyAlgorithm::Ed25519).and_then(|k| k.ed25519()) + session.signing_keys().get(&DeviceKeyAlgorithm::Ed25519).and_then(|k| k.ed25519()) { // Room keys are received as an `m.room.encrypted` event using the `m.olm` // algorithm. Upon decryption of the `m.room.encrypted` event, the @@ -237,7 +237,7 @@ impl Device { // // [1]: https://spec.matrix.org/v1.5/client-server-api/#molmv1curve25519-aes-sha2 let ed25519_comparison = self.ed25519_key().map(|k| k == key); - let curve25519_comparison = self.curve25519_key().map(|k| k == session.sender_key); + let curve25519_comparison = self.curve25519_key().map(|k| k == session.sender_key()); match (ed25519_comparison, curve25519_comparison) { // If we have any of the keys but they don't turn out to match, refuse to decrypt diff --git a/crates/matrix-sdk-crypto/src/machine.rs b/crates/matrix-sdk-crypto/src/machine.rs index e43ab2a841d..aabc973bd61 100644 --- a/crates/matrix-sdk-crypto/src/machine.rs +++ b/crates/matrix-sdk-crypto/src/machine.rs @@ -2522,7 +2522,7 @@ pub(crate) mod tests { .unwrap() .into(); let signing_keys = SigningKeys::from([(DeviceKeyAlgorithm::Ed25519, fake_key)]); - inbound.signing_keys = signing_keys.into(); + inbound.creator_info.signing_keys = signing_keys.into(); let content = json!({}); let content = outbound.encrypt(content, "m.dummy").await; diff --git a/crates/matrix-sdk-crypto/src/olm/group_sessions/inbound.rs b/crates/matrix-sdk-crypto/src/olm/group_sessions/inbound.rs index 5ba13c7c7ee..03412652e37 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/inbound.rs +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/inbound.rs @@ -60,25 +60,85 @@ use crate::{ // sessions that were created between some time period, this should only be set // for non-imported sessions. -/// Inbound group session. +/// Information about the creator of an inbound group session. +#[derive(Clone)] +pub(crate) struct SessionCreatorInfo { + /// The Curve25519 key of the session creator. + /// + /// If the session was received directly from the creator through an + /// `m.room_key` event, this key corresponds to the long-term Curve25519 + /// identity key of the Olm Session that was utilized to decrypt the + /// event. + /// + /// However, if the session was forwarded to us using an + /// `m.forwarded_room_key`, this key is just a copy of the claimed + /// Curve25519 key from the content of the event. + pub curve25519_key: Curve25519PublicKey, + + /// A mapping of DeviceKeyAlgorithm to the public signing keys of the + /// [`Device`] that sent the room key to us. + /// + /// If the session was received directly from the creator via an + /// `m.room_key` event, this map is taken from the plaintext value of + /// the decrypted Olm event, and is a copy of the + /// [`DecryptedOlmV1Event::keys`] field as defined in the [spec]. + /// + /// If the session was forwarded to us using an `m.forwarded_room_key`, this + /// map is a copy of the claimed Ed25519 key from the content of the + /// event. + /// + /// [spec]: https://spec.matrix.org/unstable/client-server-api/#molmv1curve25519-aes-sha2 + pub signing_keys: Arc>, +} + +/// A structure representing an inbound group session. /// -/// Inbound group sessions are used to exchange room messages between a group of -/// participants. Inbound group sessions are used to decrypt the room messages. +/// Inbound group sessions, also known as "room keys", are used to facilitate +/// the exchange of room messages among a group of participants. The inbound +/// variant of the group session is used to decrypt the room messages. +/// +/// This struct wraps the `vodozemac` type of the same name, and adds additional +/// Matrix- specific data to it. Additionally, the wrapper ensures thread-safe +/// access of the vodozemac type. #[derive(Clone)] pub struct InboundGroupSession { inner: Arc>, - history_visibility: Arc>, - /// The SessionId associated to this GroupSession - pub session_id: Arc, + + /// A copy of [`InnerSession::session_id`] to avoid having to acquire a lock + /// to get to the sesison ID. + session_id: Arc, + + /// A copy of [`InnerSession::first_known_index`] to avoid having to acquire + /// a lock to get to the first known index. first_known_index: u32, - /// The sender_key associated to this GroupSession - pub sender_key: Curve25519PublicKey, - /// Map of DeviceKeyAlgorithm to the public ed25519 key of the account - pub signing_keys: Arc>, + + /// Information about the creator of the room key or + /// [`InboundGroupSession`]. The thrustworthiness of this field depends + /// on how the session was received. + pub(crate) creator_info: SessionCreatorInfo, + /// The Room this GroupSession belongs to pub room_id: Arc, + + /// A flag remembering if the InboundGroupSession was received directly as a + /// `m.room_key` event or indirectly via a forward or file import. + /// + /// If the session is considered to be imported the + /// `InboundGroupSession::creator_info` field is not proven to be + /// correct. imported: bool, + + /// The messaging algorithm of this [`InboundGroupSession`] as defined by + /// the [spec]. Will be one of the `m.megolm.*` algorithms. + /// + /// [spec]: https://spec.matrix.org/unstable/client-server-api/#messaging-algorithms algorithm: Arc, + + /// The history visibility of the room at the time when the room key was + /// created. + history_visibility: Arc>, + + /// Was this room key backed up to the server. backed_up: Arc, } @@ -89,10 +149,10 @@ impl InboundGroupSession { /// /// # Arguments /// - /// * `sender_key` - The public curve25519 key of the account that - /// sent us the session + /// * `sender_key` - The public Curve25519 key of the account that + /// sent us the session. /// - /// * `signing_key` - The public ed25519 key of the account that + /// * `signing_key` - The public Ed25519 key of the account that /// sent us the session. /// /// * `room_id` - The id of the room that the session is used in. @@ -121,8 +181,10 @@ impl InboundGroupSession { history_visibility: history_visibility.into(), session_id: session_id.into(), first_known_index, - sender_key, - signing_keys: keys.into(), + creator_info: SessionCreatorInfo { + curve25519_key: sender_key, + signing_keys: keys.into(), + }, room_id: room_id.into(), imported: false, algorithm: encryption_algorithm.into(), @@ -173,9 +235,9 @@ impl InboundGroupSession { PickledInboundGroupSession { pickle, - sender_key: self.sender_key, - signing_key: (*self.signing_keys).clone(), - room_id: (*self.room_id).to_owned(), + sender_key: self.creator_info.curve25519_key, + signing_key: (*self.creator_info.signing_keys).clone(), + room_id: self.room_id().to_owned(), imported: self.imported, backed_up: self.backed_up(), history_visibility: self.history_visibility.as_ref().clone(), @@ -193,7 +255,7 @@ impl InboundGroupSession { /// Get the sender key that this session was received from. pub fn sender_key(&self) -> Curve25519PublicKey { - self.sender_key + self.creator_info.curve25519_key } /// Has the session been backed up to the server. @@ -214,7 +276,7 @@ impl InboundGroupSession { /// Get the map of signing keys this session was received from. pub fn signing_keys(&self) -> &SigningKeys { - &self.signing_keys + &self.creator_info.signing_keys } /// Export this session at the given message index. @@ -226,11 +288,11 @@ impl InboundGroupSession { ExportedRoomKey { algorithm: self.algorithm().to_owned(), - room_id: (*self.room_id).to_owned(), - sender_key: self.sender_key, + room_id: self.room_id().to_owned(), + sender_key: self.creator_info.curve25519_key, session_id: self.session_id().to_owned(), forwarding_curve25519_key_chain: vec![], - sender_claimed_keys: (*self.signing_keys).clone(), + sender_claimed_keys: (*self.creator_info.signing_keys).clone(), session_key, } } @@ -254,10 +316,12 @@ impl InboundGroupSession { Ok(InboundGroupSession { inner: Mutex::new(session).into(), session_id: session_id.into(), - sender_key: pickle.sender_key, + creator_info: SessionCreatorInfo { + curve25519_key: pickle.sender_key, + signing_keys: pickle.signing_key.into(), + }, history_visibility: pickle.history_visibility.into(), first_known_index, - signing_keys: pickle.signing_key.into(), room_id: (*pickle.room_id).into(), backed_up: AtomicBool::from(pickle.backed_up).into(), algorithm: pickle.algorithm.into(), @@ -420,7 +484,7 @@ impl PartialEq for InboundGroupSession { pub struct PickledInboundGroupSession { /// The pickle string holding the InboundGroupSession. pub pickle: InboundGroupSessionPickle, - /// The public curve25519 key of the account that sent us the session + /// The public Curve25519 key of the account that sent us the session #[serde(deserialize_with = "deserialize_curve_key", serialize_with = "serialize_curve_key")] pub sender_key: Curve25519PublicKey, /// The public ed25519 key of the account that sent us the session. @@ -455,10 +519,12 @@ impl TryFrom<&ExportedRoomKey> for InboundGroupSession { Ok(InboundGroupSession { inner: Mutex::new(session).into(), session_id: key.session_id.to_owned().into(), - sender_key: key.sender_key, + creator_info: SessionCreatorInfo { + curve25519_key: key.sender_key, + signing_keys: key.sender_claimed_keys.to_owned().into(), + }, history_visibility: None.into(), first_known_index, - signing_keys: key.sender_claimed_keys.to_owned().into(), room_id: key.room_id.to_owned().into(), imported: true, algorithm: key.algorithm.to_owned().into(), @@ -476,14 +542,16 @@ impl From<&ForwardedMegolmV1AesSha2Content> for InboundGroupSession { InboundGroupSession { inner: Mutex::new(session).into(), session_id, - sender_key: value.claimed_sender_key, + creator_info: SessionCreatorInfo { + curve25519_key: value.claimed_sender_key, + signing_keys: SigningKeys::from([( + DeviceKeyAlgorithm::Ed25519, + value.claimed_ed25519_key.into(), + )]) + .into(), + }, history_visibility: None.into(), first_known_index, - signing_keys: SigningKeys::from([( - DeviceKeyAlgorithm::Ed25519, - value.claimed_ed25519_key.into(), - )]) - .into(), room_id: value.room_id.to_owned().into(), imported: true, algorithm: EventEncryptionAlgorithm::MegolmV1AesSha2.into(), @@ -501,10 +569,12 @@ impl From<&ForwardedMegolmV2AesSha2Content> for InboundGroupSession { InboundGroupSession { inner: Mutex::new(session).into(), session_id, - sender_key: value.claimed_sender_key, + creator_info: SessionCreatorInfo { + curve25519_key: value.claimed_sender_key, + signing_keys: value.claimed_signing_keys.to_owned().into(), + }, history_visibility: None.into(), first_known_index, - signing_keys: value.claimed_signing_keys.to_owned().into(), room_id: value.room_id.to_owned().into(), imported: true, algorithm: EventEncryptionAlgorithm::MegolmV1AesSha2.into(), @@ -604,7 +674,7 @@ mod test { assert_eq!(inbound.compare(&inbound).await, SessionOrdering::Equal); assert_eq!(inbound.compare(©).await, SessionOrdering::Equal); - copy.sender_key = + copy.creator_info.curve25519_key = Curve25519PublicKey::from_base64("XbmrPa1kMwmdtNYng1B2gsfoo8UtF+NklzsTZiaVKyY") .unwrap(); diff --git a/crates/matrix-sdk-crypto/src/store/caches.rs b/crates/matrix-sdk-crypto/src/store/caches.rs index ae2e3d959b2..05de94fbf0f 100644 --- a/crates/matrix-sdk-crypto/src/store/caches.rs +++ b/crates/matrix-sdk-crypto/src/store/caches.rs @@ -89,7 +89,7 @@ impl GroupSessionStore { /// already in the store. pub fn add(&self, session: InboundGroupSession) -> bool { self.entries - .entry((*session.room_id).to_owned()) + .entry(session.room_id().to_owned()) .or_default() .insert(session.session_id().to_owned(), session) .is_none() From 0985043a6adfe32ddb276dad95f000703b86e3e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damir=20Jeli=C4=87?= Date: Tue, 14 Mar 2023 17:01:03 +0100 Subject: [PATCH 2/2] Apply suggestions from code review Co-authored-by: Denis Kasak --- .../src/olm/group_sessions/inbound.rs | 40 +++++++++++-------- 1 file changed, 23 insertions(+), 17 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/olm/group_sessions/inbound.rs b/crates/matrix-sdk-crypto/src/olm/group_sessions/inbound.rs index 03412652e37..3e36f855a75 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/inbound.rs +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/inbound.rs @@ -63,20 +63,24 @@ use crate::{ /// Information about the creator of an inbound group session. #[derive(Clone)] pub(crate) struct SessionCreatorInfo { - /// The Curve25519 key of the session creator. + /// The Curve25519 identity key of the session creator. /// - /// If the session was received directly from the creator through an - /// `m.room_key` event, this key corresponds to the long-term Curve25519 - /// identity key of the Olm Session that was utilized to decrypt the - /// event. + /// If the session was received directly from its creator device through an + /// `m.room_key` event (and therefore, session sender == session creator), + /// this key equals the Curve25519 device identity key of that device. Since + /// this key is one of three keys used to establish the Olm session through + /// which encrypted to-device messages (including `m.room_key`) are sent, + /// this constitutes a proof that this inbound group session is owned by + /// that particular Curve25519 key. /// - /// However, if the session was forwarded to us using an - /// `m.forwarded_room_key`, this key is just a copy of the claimed - /// Curve25519 key from the content of the event. + /// However, if the session was simply forwarded to us in an + /// `m.forwarded_room_key` event (in which case sender != creator), this key + /// is just a *claim* made by the session sender of what the actual creator + /// device is. pub curve25519_key: Curve25519PublicKey, /// A mapping of DeviceKeyAlgorithm to the public signing keys of the - /// [`Device`] that sent the room key to us. + /// [`Device`] that sent us the session. /// /// If the session was received directly from the creator via an /// `m.room_key` event, this map is taken from the plaintext value of @@ -97,9 +101,11 @@ pub(crate) struct SessionCreatorInfo { /// the exchange of room messages among a group of participants. The inbound /// variant of the group session is used to decrypt the room messages. /// -/// This struct wraps the `vodozemac` type of the same name, and adds additional -/// Matrix- specific data to it. Additionally, the wrapper ensures thread-safe +/// This struct wraps the [vodozemac] type of the same name, and adds additional +/// Matrix-specific data to it. Additionally, the wrapper ensures thread-safe /// access of the vodozemac type. +/// +/// [vodozemac]: https://matrix-org.github.io/vodozemac/vodozemac/index.html #[derive(Clone)] pub struct InboundGroupSession { inner: Arc>, @@ -112,19 +118,19 @@ pub struct InboundGroupSession { /// a lock to get to the first known index. first_known_index: u32, - /// Information about the creator of the room key or - /// [`InboundGroupSession`]. The thrustworthiness of this field depends + /// Information about the creator of the [`InboundGroupSession`] ("room + /// key"). The trustworthiness of the information in this field depends /// on how the session was received. pub(crate) creator_info: SessionCreatorInfo, /// The Room this GroupSession belongs to pub room_id: Arc, - /// A flag remembering if the InboundGroupSession was received directly as a - /// `m.room_key` event or indirectly via a forward or file import. + /// A flag recording whether the `InboundGroupSession` was received directly + /// as a `m.room_key` event or indirectly via a forward or file import. /// - /// If the session is considered to be imported the - /// `InboundGroupSession::creator_info` field is not proven to be + /// If the session is considered to be imported, the information contained + /// in the `InboundGroupSession::creator_info` field is not proven to be /// correct. imported: bool,