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

Reshare Megolm session after the other party unwedges #3604

Merged
merged 13 commits into from
Jun 28, 2024
4 changes: 3 additions & 1 deletion crates/matrix-sdk-crypto/src/gossiping/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -642,7 +642,9 @@ impl GossipMachine {
// information is recorded there.
} else if let Some(outbound) = outbound_session {
match outbound.is_shared_with(&device.inner) {
ShareState::Shared(message_index) => Ok(Some(message_index)),
ShareState::Shared { message_index, olm_wedging_index: _ } => {
Ok(Some(message_index))
}
ShareState::SharedButChangedSenderKey => Err(KeyForwardDecision::ChangedSenderKey),
ShareState::NotShared => Err(KeyForwardDecision::OutboundSessionNotShared),
}
Expand Down
16 changes: 14 additions & 2 deletions crates/matrix-sdk-crypto/src/identities/device.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,9 @@ use crate::{
olm::{
InboundGroupSession, OutboundGroupSession, Session, ShareInfo, SignedJsonObject, VerifyJson,
},
store::{Changes, CryptoStoreWrapper, DeviceChanges, Result as StoreResult},
store::{
caches::SequenceNumber, Changes, CryptoStoreWrapper, DeviceChanges, Result as StoreResult,
},
types::{
events::{
forwarded_room_key::ForwardedRoomKeyContent,
Expand Down Expand Up @@ -89,6 +91,10 @@ pub struct ReadOnlyDevice {
/// Default to epoch for migration purpose.
#[serde(default = "default_timestamp")]
first_time_seen_ts: MilliSecondsSinceUnixEpoch,
/// The number of times the device has tried to unwedge Olm sessions with
/// us.
#[serde(default)]
pub(crate) olm_wedging_index: SequenceNumber,
}

fn default_timestamp() -> MilliSecondsSinceUnixEpoch {
Expand Down Expand Up @@ -580,6 +586,7 @@ impl ReadOnlyDevice {
deleted: Arc::new(AtomicBool::new(false)),
withheld_code_sent: Arc::new(AtomicBool::new(false)),
first_time_seen_ts: MilliSecondsSinceUnixEpoch::now(),
olm_wedging_index: Default::default(),
}
}

Expand Down Expand Up @@ -833,7 +840,11 @@ impl ReadOnlyDevice {

match self.encrypt(store, event_type, content).await {
Ok((session, encrypted)) => Ok(MaybeEncryptedRoomKey::Encrypted {
share_info: ShareInfo::new_shared(session.sender_key().to_owned(), message_index),
share_info: ShareInfo::new_shared(
session.sender_key().to_owned(),
message_index,
self.olm_wedging_index,
),
used_session: session,
message: encrypted.cast(),
}),
Expand Down Expand Up @@ -971,6 +982,7 @@ impl TryFrom<&DeviceKeys> for ReadOnlyDevice {
trust_state: Arc::new(RwLock::new(LocalTrust::Unset)),
withheld_code_sent: Arc::new(AtomicBool::new(false)),
first_time_seen_ts: MilliSecondsSinceUnixEpoch::now(),
olm_wedging_index: Default::default(),
};

device.verify_device_keys(device_keys)?;
Expand Down
26 changes: 19 additions & 7 deletions crates/matrix-sdk-crypto/src/olm/account.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ use crate::{
error::{EventError, OlmResult, SessionCreationError},
identities::ReadOnlyDevice,
requests::UploadSigningKeysRequest,
store::{Changes, Store},
store::{Changes, DeviceChanges, Store},
types::{
events::{
olm_v1::AnyDecryptedOlmEvent,
Expand Down Expand Up @@ -1327,12 +1327,24 @@ impl Account {
// we might try to create the same session again.
// TODO: separate the session cache from the storage so we only add
// it to the cache but don't store it.
store
.save_changes(Changes {
sessions: vec![result.session.clone()],
..Default::default()
})
.await?;
let mut changes =
Changes { sessions: vec![result.session.clone()], ..Default::default() };
uhoreg marked this conversation as resolved.
Show resolved Hide resolved

// Any new Olm session will bump the Olm wedging index for the
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be also wrapped manually, though it doesn't look as comical as the other doc comment, wouldn't hurt to let cargo fmt format this as well.

// sender's device, if we have their device, which will cause us
// to re-send existing Megolm sessions to them the next time we
// use the session. If we don't have their device, this means
// that we haven't tried to send them any Megolm sessions yet,
// so we don't need to worry about it.
if let Some(device) = store.get_device_from_curve_key(sender, sender_key).await? {
Copy link
Contributor

Choose a reason for hiding this comment

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

This section is clustered together as well, please add some whitespace to make this easier to read.

let mut read_only_device = device.inner;
read_only_device.olm_wedging_index.increment();

changes.devices =
DeviceChanges { changed: vec![read_only_device], ..Default::default() };
}

store.save_changes(changes).await?;

Ok((SessionType::New(result.session), result.plaintext))
}
Expand Down
46 changes: 34 additions & 12 deletions crates/matrix-sdk-crypto/src/olm/group_sessions/outbound.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ use super::SessionCreationError;
use crate::types::events::room::encrypted::MegolmV2AesSha2Content;
use crate::{
session_manager::CollectStrategy,
store::caches::SequenceNumber,
types::{
events::{
room::encrypted::{
Expand All @@ -67,10 +68,19 @@ const ROTATION_PERIOD: Duration = ONE_WEEK;
const ROTATION_MESSAGES: u64 = 100;

#[derive(Debug, Clone, Copy, PartialEq, Eq)]
/// Information about whether a session was shared with a device.
pub(crate) enum ShareState {
/// The session was not shared with the device.
NotShared,
/// The session was shared with the device with the given device ID, but
/// with a different curve25519 key.
SharedButChangedSenderKey,
Shared(u32),
/// The session was shared with the device, at the given message index. The
/// `olm_wedging_index` is the value of the `olm_wedging_index` from the
/// `ReadOnlyDevice` at the time that we last shared the session with the
/// device, and indicates whether we need to re-share the session with the
/// device.
Shared { message_index: u32, olm_wedging_index: SequenceNumber },
Copy link
Contributor

Choose a reason for hiding this comment

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

This still isn't documented.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I misunderstood your original comment and thought that switching to named fields would avoid the need for documentation. I've added docs, and while I was there, added docs for the rest of the enum too.

}

/// Settings for an encrypted room.
Expand Down Expand Up @@ -169,8 +179,12 @@ pub enum ShareInfo {

impl ShareInfo {
/// Helper to create a SharedWith info
pub fn new_shared(sender_key: Curve25519PublicKey, message_index: u32) -> Self {
ShareInfo::Shared(SharedWith { sender_key, message_index })
pub fn new_shared(
sender_key: Curve25519PublicKey,
message_index: u32,
olm_wedging_index: SequenceNumber,
) -> Self {
ShareInfo::Shared(SharedWith { sender_key, message_index, olm_wedging_index })
}

/// Helper to create a Withheld info
Expand All @@ -185,6 +199,9 @@ pub struct SharedWith {
pub sender_key: Curve25519PublicKey,
/// The message index that the device received.
pub message_index: u32,
/// The Olm wedging index of the device at the time the session was shared.
#[serde(default)]
pub olm_wedging_index: SequenceNumber,
}

impl OutboundGroupSession {
Expand Down Expand Up @@ -527,7 +544,10 @@ impl OutboundGroupSession {
d.get(device.device_id()).map(|s| match s {
ShareInfo::Shared(s) => {
if device.curve25519_key() == Some(s.sender_key) {
ShareState::Shared(s.message_index)
ShareState::Shared {
message_index: s.message_index,
olm_wedging_index: s.olm_wedging_index,
}
} else {
ShareState::SharedButChangedSenderKey
}
Expand All @@ -551,7 +571,10 @@ impl OutboundGroupSession {
Some(match info {
ShareInfo::Shared(info) => {
if device.curve25519_key() == Some(info.sender_key) {
ShareState::Shared(info.message_index)
ShareState::Shared {
message_index: info.message_index,
olm_wedging_index: info.olm_wedging_index,
}
} else {
ShareState::SharedButChangedSenderKey
}
Expand Down Expand Up @@ -598,12 +621,10 @@ impl OutboundGroupSession {
sender_key: Curve25519PublicKey,
index: u32,
) {
self.shared_with_set
.write()
.unwrap()
.entry(user_id.to_owned())
.or_default()
.insert(device_id.to_owned(), ShareInfo::new_shared(sender_key, index));
self.shared_with_set.write().unwrap().entry(user_id.to_owned()).or_default().insert(
device_id.to_owned(),
ShareInfo::new_shared(sender_key, index, Default::default()),
);
}

/// Mark the session as shared with the given user/device pair, starting
Expand All @@ -615,7 +636,8 @@ impl OutboundGroupSession {
device_id: &DeviceId,
sender_key: Curve25519PublicKey,
) {
let share_info = ShareInfo::new_shared(sender_key, self.message_index().await);
let share_info =
ShareInfo::new_shared(sender_key, self.message_index().await, Default::default());
self.shared_with_set
.write()
.unwrap()
Expand Down
Loading
Loading