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

crypto: expose new method OlmMachine::try_decrypt_room_event #4116

Merged
merged 4 commits into from
Oct 16, 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
116 changes: 114 additions & 2 deletions crates/matrix-sdk-common/src/deserialized_responses.rs
Original file line number Diff line number Diff line change
Expand Up @@ -628,6 +628,59 @@ pub struct UnableToDecryptInfo {
/// `m.megolm.v1.aes-sha2` algorithm.
#[serde(skip_serializing_if = "Option::is_none")]
pub session_id: Option<String>,

/// Reason code for the decryption failure
#[serde(default = "unknown_utd_reason")]
pub reason: UnableToDecryptReason,
}

fn unknown_utd_reason() -> UnableToDecryptReason {
UnableToDecryptReason::Unknown
}
Comment on lines +637 to +639
Copy link
Member

Choose a reason for hiding this comment

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

For what it's worth, you can instead derive(Default) to UnableToDecryptReason, and then mark one variant as the default:

#[default]
#[doc(hidden)]
Unknown,

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I wanted to explicitly avoid implementing Default for UnableToDecryptReason, because then people might start using it outside the deserialization case.


/// Reason code for a decryption failure
#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)]
pub enum UnableToDecryptReason {
/// The reason for the decryption failure is unknown. This is only intended
/// for use when deserializing old UnableToDecryptInfo instances.
#[doc(hidden)]
Unknown,

/// The `m.room.encrypted` event that should have been decrypted is
/// malformed in some way (e.g. unsupported algorithm, missing fields,
/// unknown megolm message type).
MalformedEncryptedEvent,

/// Decryption failed because we're missing the megolm session that was used
/// to encrypt the event.
///
/// TODO: support withheld codes?
MissingMegolmSession,

/// Decryption failed because, while we have the megolm session that was
/// used to encrypt the message, it is ratcheted too far forward.
UnknownMegolmMessageIndex,

/// We found the Megolm session, but were unable to decrypt the event using
/// that session for some reason (e.g. incorrect MAC).
///
/// This represents all `vodozemac::megolm::DecryptionError`s, except
/// `UnknownMessageIndex`, which is represented as
/// `UnknownMegolmMessageIndex`.
MegolmDecryptionFailure,

/// The event could not be deserialized after decryption.
PayloadDeserializationFailure,
Comment on lines +670 to +673
Copy link
Member

Choose a reason for hiding this comment

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

double nit: since those are errors, i'd say "failure" in the variant's name is redundant, and would lightly encourage to remove it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't really agree. If I compare with something like MissingMegolmSession: a missing megolm session is a sensible reason for being unable to decrypt an event. So is a "megolm decryption failure". But just MegolmDecryption on its own doesn't really make sense to me.

I'm prepared to believe there could be better names for these things (UnableToDecryptMegolmMessage? VodozemacDecryptionError?), but I don't think MegolmDecryption would be a good one.


/// Decryption failed because of a mismatch between the identity keys of the
/// device we received the room key from and the identity keys recorded in
/// the plaintext of the room key to-device message.
MismatchedIdentityKeys,

/// An encrypted message wasn't decrypted, because the sender's
/// cross-signing identity did not satisfy the requested
/// `TrustRequirement`.
SenderIdentityNotTrusted(VerificationLevel),
}

/// Deserialization helper for [`SyncTimelineEvent`], for the modern format.
Expand Down Expand Up @@ -705,6 +758,8 @@ impl From<SyncTimelineEventDeserializationHelperV0> for SyncTimelineEvent {

#[cfg(test)]
mod tests {
use std::collections::BTreeMap;

use assert_matches::assert_matches;
use ruma::{
event_id,
Expand All @@ -717,7 +772,8 @@ mod tests {

use super::{
AlgorithmInfo, DecryptedRoomEvent, EncryptionInfo, SyncTimelineEvent, TimelineEvent,
TimelineEventKind, VerificationState,
TimelineEventKind, UnableToDecryptInfo, UnableToDecryptReason, UnsignedDecryptionResult,
UnsignedEventLocation, VerificationState,
};
use crate::deserialized_responses::{DeviceLinkProblem, VerificationLevel};

Expand Down Expand Up @@ -808,7 +864,13 @@ mod tests {
},
verification_state: VerificationState::Verified,
},
unsigned_encryption_info: None,
unsigned_encryption_info: Some(BTreeMap::from([(
UnsignedEventLocation::RelationsReplace,
UnsignedDecryptionResult::UnableToDecrypt(UnableToDecryptInfo {
session_id: Some("xyz".to_owned()),
reason: UnableToDecryptReason::MalformedEncryptedEvent,
}),
)])),
}),
push_actions: Default::default(),
};
Expand Down Expand Up @@ -840,6 +902,12 @@ mod tests {
},
"verification_state": "Verified",
},
"unsigned_encryption_info": {
"RelationsReplace": {"UnableToDecrypt": {
"session_id": "xyz",
"reason": "MalformedEncryptedEvent",
}}
}
}
}
})
Expand Down Expand Up @@ -881,5 +949,49 @@ mod tests {
event.encryption_info().unwrap().algorithm_info,
AlgorithmInfo::MegolmV1AesSha2 { .. }
);

// Test that the previous format, with an undecryptable unsigned event, can also
// be deserialized.
let serialized = json!({
"event": {
"content": {"body": "secret", "msgtype": "m.text"},
"event_id": "$xxxxx:example.org",
"origin_server_ts": 2189,
"room_id": "!someroom:example.com",
"sender": "@carl:example.com",
"type": "m.room.message",
},
"encryption_info": {
"sender": "@sender:example.com",
"sender_device": null,
"algorithm_info": {
"MegolmV1AesSha2": {
"curve25519_key": "xxx",
"sender_claimed_keys": {}
}
},
"verification_state": "Verified",
},
"unsigned_encryption_info": {
"RelationsReplace": {"UnableToDecrypt": {"session_id": "xyz"}}
}
});
let event: SyncTimelineEvent = serde_json::from_value(serialized).unwrap();
assert_eq!(event.event_id(), Some(event_id!("$xxxxx:example.org").to_owned()));
assert_matches!(
event.encryption_info().unwrap().algorithm_info,
AlgorithmInfo::MegolmV1AesSha2 { .. }
);
assert_matches!(event.kind, TimelineEventKind::Decrypted(decrypted) => {
assert_matches!(decrypted.unsigned_encryption_info, Some(map) => {
assert_eq!(map.len(), 1);
let (location, result) = map.into_iter().next().unwrap();
assert_eq!(location, UnsignedEventLocation::RelationsReplace);
assert_matches!(result, UnsignedDecryptionResult::UnableToDecrypt(utd_info) => {
assert_eq!(utd_info.session_id, Some("xyz".to_owned()));
assert_eq!(utd_info.reason, UnableToDecryptReason::Unknown);
})
});
});
}
}
6 changes: 6 additions & 0 deletions crates/matrix-sdk-crypto/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,12 @@

Changes:

- Add new method `OlmMachine::try_decrypt_room_event`.
([#4116](https://github.com/matrix-org/matrix-rust-sdk/pull/4116))

- Add reason code to `matrix_sdk_common::deserialized_responses::UnableToDecryptInfo`.
([#4116](https://github.com/matrix-org/matrix-rust-sdk/pull/4116))

- The `UserIdentity` struct has been renamed to `OtherUserIdentity`
([#4036](https://github.com/matrix-org/matrix-rust-sdk/pull/4036]))

Expand Down
12 changes: 12 additions & 0 deletions crates/matrix-sdk-crypto/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ pub use identities::{
OwnUserIdentityData, UserDevices, UserIdentity, UserIdentityData,
};
pub use machine::{CrossSigningBootstrapRequests, EncryptionSyncChanges, OlmMachine};
use matrix_sdk_common::deserialized_responses::{DecryptedRoomEvent, UnableToDecryptInfo};
#[cfg(feature = "qrcode")]
pub use matrix_sdk_qrcode;
pub use olm::{Account, CrossSigningStatus, EncryptionSettings, Session};
Expand Down Expand Up @@ -142,3 +143,14 @@ pub struct DecryptionSettings {
/// [`MegolmError::SenderIdentityNotTrusted`] will be returned.
pub sender_device_trust_requirement: TrustRequirement,
}

/// The result of an attempt to decrypt a room event: either a successful
/// decryption, or information on a failure.
#[derive(Clone, Debug, Serialize, Deserialize)]
pub enum RoomEventDecryptionResult {
/// A successfully-decrypted encrypted event.
Decrypted(DecryptedRoomEvent),

/// We were unable to decrypt the event
UnableToDecrypt(UnableToDecryptInfo),
}
91 changes: 77 additions & 14 deletions crates/matrix-sdk-crypto/src/machine/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ use itertools::Itertools;
use matrix_sdk_common::{
deserialized_responses::{
AlgorithmInfo, DecryptedRoomEvent, DeviceLinkProblem, EncryptionInfo, UnableToDecryptInfo,
UnsignedDecryptionResult, UnsignedEventLocation, VerificationLevel, VerificationState,
UnableToDecryptReason, UnsignedDecryptionResult, UnsignedEventLocation, VerificationLevel,
VerificationState,
},
BoxFuture,
};
Expand Down Expand Up @@ -94,7 +95,7 @@ use crate::{
utilities::timestamp_to_iso8601,
verification::{Verification, VerificationMachine, VerificationRequest},
CrossSigningKeyExport, CryptoStoreError, DecryptionSettings, DeviceData, KeysQueryRequest,
LocalTrust, SignatureError, ToDeviceRequest, TrustRequirement,
LocalTrust, RoomEventDecryptionResult, SignatureError, ToDeviceRequest, TrustRequirement,
};

/// State machine implementation of the Olm/Megolm encryption protocol used for
Expand Down Expand Up @@ -1734,6 +1735,34 @@ impl OlmMachine {
}
}

/// Attempt to decrypt an event from a room timeline, returning information
/// on the failure if it fails.
///
/// # Arguments
///
/// * `event` - The event that should be decrypted.
///
/// * `room_id` - The ID of the room where the event was sent to.
///
/// # Returns
///
/// The decrypted event, if it was successfully decrypted. Otherwise,
/// information on the failure, unless the failure was due to an
/// internal error, in which case, an `Err` result.
pub async fn try_decrypt_room_event(
&self,
raw_event: &Raw<EncryptedEvent>,
room_id: &RoomId,
decryption_settings: &DecryptionSettings,
) -> Result<RoomEventDecryptionResult, CryptoStoreError> {
match self.decrypt_room_event_inner(raw_event, room_id, true, decryption_settings).await {
Ok(decrypted) => Ok(RoomEventDecryptionResult::Decrypted(decrypted)),
Err(err) => Ok(RoomEventDecryptionResult::UnableToDecrypt(megolm_error_to_utd_info(
raw_event, err,
)?)),
}
}

/// Decrypt an event from a room timeline.
///
/// # Arguments
Expand Down Expand Up @@ -1902,18 +1931,13 @@ impl OlmMachine {
*event = serde_json::to_value(decrypted_event.event).ok()?;
Some(UnsignedDecryptionResult::Decrypted(decrypted_event.encryption_info))
}
Err(_) => {
let session_id =
raw_event.deserialize().ok().and_then(|ev| match ev.content.scheme {
RoomEventEncryptionScheme::MegolmV1AesSha2(s) => Some(s.session_id),
#[cfg(feature = "experimental-algorithms")]
RoomEventEncryptionScheme::MegolmV2AesSha2(s) => Some(s.session_id),
RoomEventEncryptionScheme::Unknown(_) => None,
});

Some(UnsignedDecryptionResult::UnableToDecrypt(UnableToDecryptInfo {
session_id,
}))
Err(err) => {
// For now, we throw away crypto store errors and just treat the unsigned event
// as unencrypted. Crypto store errors represent problems with the application
// rather than normal UTD errors, so they should probably be propagated
// rather than swallowed.
let utd_info = megolm_error_to_utd_info(&raw_event, err).ok()?;
Some(UnsignedDecryptionResult::UnableToDecrypt(utd_info))
}
}
})
Expand Down Expand Up @@ -2540,6 +2564,45 @@ pub struct EncryptionSyncChanges<'a> {
pub next_batch_token: Option<String>,
}

/// Convert a [`MegolmError`] into an [`UnableToDecryptInfo`] or a
/// [`CryptoStoreError`].
///
/// Most `MegolmError` codes are converted into a suitable
/// `UnableToDecryptInfo`. The exception is [`MegolmError::Store`], which
/// represents a problem with our datastore rather than with the message itself,
/// and is therefore returned as a `CryptoStoreError`.
fn megolm_error_to_utd_info(
Copy link
Member

Choose a reason for hiding this comment

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

Can you write a tiny doc comment for this, please? (It wasn't clear at a first glance at the call-site why this could return an error...)

raw_event: &Raw<EncryptedEvent>,
error: MegolmError,
) -> Result<UnableToDecryptInfo, CryptoStoreError> {
use MegolmError::*;
let reason = match error {
EventError(_) => UnableToDecryptReason::MalformedEncryptedEvent,
Decode(_) => UnableToDecryptReason::MalformedEncryptedEvent,
MissingRoomKey(_) => UnableToDecryptReason::MissingMegolmSession,
Decryption(DecryptionError::UnknownMessageIndex(_, _)) => {
UnableToDecryptReason::UnknownMegolmMessageIndex
}
Decryption(_) => UnableToDecryptReason::MegolmDecryptionFailure,
JsonError(_) => UnableToDecryptReason::PayloadDeserializationFailure,
MismatchedIdentityKeys(_) => UnableToDecryptReason::MismatchedIdentityKeys,
SenderIdentityNotTrusted(level) => UnableToDecryptReason::SenderIdentityNotTrusted(level),

// Pass through crypto store errors, which indicate a problem with our
// application, rather than a UTD.
Store(error) => Err(error)?,
};

let session_id = raw_event.deserialize().ok().and_then(|ev| match ev.content.scheme {
RoomEventEncryptionScheme::MegolmV1AesSha2(s) => Some(s.session_id),
#[cfg(feature = "experimental-algorithms")]
RoomEventEncryptionScheme::MegolmV2AesSha2(s) => Some(s.session_id),
RoomEventEncryptionScheme::Unknown(_) => None,
});

Ok(UnableToDecryptInfo { session_id, reason })
}

#[cfg(test)]
pub(crate) mod test_helpers;

Expand Down
31 changes: 19 additions & 12 deletions crates/matrix-sdk-crypto/src/machine/tests/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,11 @@

use std::{collections::BTreeMap, iter, ops::Not, sync::Arc, time::Duration};

use assert_matches2::assert_matches;
use assert_matches2::{assert_let, assert_matches};
use futures_util::{pin_mut, FutureExt, StreamExt};
use itertools::Itertools;
use matrix_sdk_common::deserialized_responses::{
UnableToDecryptInfo, UnsignedDecryptionResult, UnsignedEventLocation,
UnableToDecryptInfo, UnableToDecryptReason, UnsignedDecryptionResult, UnsignedEventLocation,
};
use matrix_sdk_test::{async_test, message_like_event_content, ruma_response_from_json, test_json};
use ruma::{
Expand Down Expand Up @@ -71,7 +71,7 @@ use crate::{
utilities::json_convert,
verification::tests::bob_id,
Account, DecryptionSettings, DeviceData, EncryptionSettings, MegolmError, OlmError,
OutgoingRequests, ToDeviceRequest, TrustRequirement,
OutgoingRequests, RoomEventDecryptionResult, ToDeviceRequest, TrustRequirement,
};

mod decryption_verification_state;
Expand Down Expand Up @@ -555,13 +555,11 @@ async fn test_megolm_encryption() {

let decryption_settings =
DecryptionSettings { sender_device_trust_requirement: TrustRequirement::Untrusted };
let decrypted_event = bob
.decrypt_room_event(&event, room_id, &decryption_settings)
.await
.unwrap()
.event
.deserialize()
.unwrap();

let decryption_result =
bob.try_decrypt_room_event(&event, room_id, &decryption_settings).await.unwrap();
assert_let!(RoomEventDecryptionResult::Decrypted(decrypted_event) = decryption_result);
let decrypted_event = decrypted_event.event.deserialize().unwrap();

if let AnyMessageLikeEvent::RoomMessage(MessageLikeEvent::Original(
OriginalMessageLikeEvent { sender, content, .. },
Expand Down Expand Up @@ -678,6 +676,13 @@ async fn test_withheld_unverified() {

let err = decrypt_result.err().unwrap();
assert_matches!(err, MegolmError::MissingRoomKey(Some(WithheldCode::Unverified)));

// Also check `try_decrypt_room_event`.
let decrypt_result =
bob.try_decrypt_room_event(&room_event, room_id, &decryption_settings).await.unwrap();
assert_let!(RoomEventDecryptionResult::UnableToDecrypt(utd_info) = decrypt_result);
assert!(utd_info.session_id.is_some());
Copy link
Member

Choose a reason for hiding this comment

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

could we assert the reason too?

assert_eq!(utd_info.reason, UnableToDecryptReason::MissingMegolmSession);
}

/// Test what happens when we feed an unencrypted event into the decryption
Expand Down Expand Up @@ -1355,7 +1360,8 @@ async fn test_unsigned_decryption() {
assert_matches!(
replace_encryption_result,
UnsignedDecryptionResult::UnableToDecrypt(UnableToDecryptInfo {
session_id: Some(second_room_key_session_id)
session_id: Some(second_room_key_session_id),
reason: UnableToDecryptReason::MissingMegolmSession,
})
);

Expand Down Expand Up @@ -1460,7 +1466,8 @@ async fn test_unsigned_decryption() {
assert_matches!(
thread_encryption_result,
UnsignedDecryptionResult::UnableToDecrypt(UnableToDecryptInfo {
session_id: Some(third_room_key_session_id)
session_id: Some(third_room_key_session_id),
reason: UnableToDecryptReason::MissingMegolmSession,
})
);

Expand Down
Loading