crypto: Implement `OlmMachine::{set_,}room_settings` #3042

richvdh · 2024-01-19T18:41:55Z

We need to make sure we guard against servers downgrading the encryption settings, or breaking them with unsupported algorithms. It's not entirely clear what the expectation is here, but legacy crypto in matrix-js-sdk blocks any changes to the settings at all, so we'll follow suit for now.

This PR also extends the supported RoomSettings. Suggest reviewing each commit separately.

Public API changes documented in changelogs (optional)

Signed-off-by:

richvdh · 2024-01-19T18:43:51Z

crates/matrix-sdk-crypto/src/machine.rs

+    /// If the settings are valid, they will be persisted to the crypto store.
+    /// These settings are not used directly by this library, but the saved
+    /// settings can be retrieved via [`get_room_settings`].
+    pub async fn set_room_settings(


I'm not completely sure if this is the best place for this API: OlmMachine already has a million methods and adding more isn't my favourite thing. But the only other likely-looking place seems to be the Store itself, and given this is application-level logic, that doesn't feel right.

I agree with your reasoning. Let's keep it here for the moment. We can refactor later.

codecov · 2024-01-19T18:52:20Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d85c7b2) 83.67% compared to head (0b01a2a) 83.72%.
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3042      +/-   ##
==========================================
+ Coverage   83.67%   83.72%   +0.04%     
==========================================
  Files         222      222              
  Lines       23288    23310      +22     
==========================================
+ Hits        19487    19516      +29     
+ Misses       3801     3794       -7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

We need to make sure we guard against servers downgrading the encryption settings, or breaking them with unsupported algorithms. It's not *entirely* clear what the expectation is here, but legacy crypto in matrix-js-sdk blocks any changes to the settings at all, so we'll follow suit for now.

Hywan

The PR is really good. Commits are well written, the patch contains a lot of docs (I can't highlight enough how much I like that), the tests are good… Just a few tiny nitty gritty things , but overall it looks great. Thanks!

Hywan · 2024-01-22T09:14:54Z

crates/matrix-sdk-crypto/src/store/integration_tests.rs

@@ -855,12 +856,15 @@ macro_rules! cryptostore_integration_tests {
                let settings_1 = RoomSettings {
                    algorithm: EventEncryptionAlgorithm::MegolmV1AesSha2,
                    only_allow_trusted_devices: true,
+                    session_rotation_period: Some(Duration::from_millis(10000)),


It's probably a bit more Rust idiomatic to write this as follows:

- Duration::from_millis(10000)), + Duration::from_millis(10_000)),

I also wonder if we could simply use : from_secs(10)?

Hywan · 2024-01-22T09:22:02Z

crates/matrix-sdk-crypto/src/machine.rs

+    /// encryption algorithm or whether to encrypt only for trusted devices.
+    ///
+    /// These settings can be modified via [`OlmMachine::set_room_settings`].
+    pub async fn get_room_settings(&self, room_id: &RoomId) -> StoreResult<Option<RoomSettings>> {


The get_ prefix isn't really Rust idiomatic. I know it's present in the store API, let's not propagate this error :-]. Can you rename get_room_settings to room_settings please?

Hywan · 2024-01-22T09:22:43Z

crates/matrix-sdk-crypto/src/machine.rs

+    /// If the settings are valid, they will be persisted to the crypto store.
+    /// These settings are not used directly by this library, but the saved
+    /// settings can be retrieved via [`get_room_settings`].
+    pub async fn set_room_settings(


I agree with your reasoning. Let's keep it here for the moment. We can refactor later.

Hywan · 2024-01-22T09:23:28Z

crates/matrix-sdk-crypto/src/machine.rs

+        // `set_room_settings`. By way of an easy way to do so, we start a
+        // StoreTransaction. There's no need to commit() it: we're just using it as a
+        // lock guard.


Looks good to me 👍. Thanks for the comment, that's the kind of comment I like!

Hywan · 2024-01-22T09:28:52Z

crates/matrix-sdk-crypto/src/machine.rs

+        machine
+            .set_room_settings(room_id!("!test:localhost"), &RoomSettings::default())
+            .await
+            .unwrap();


This code is not necessary for this test. I understand the idea, but it looks we can safely remove it.

Hywan · 2024-01-22T09:31:50Z

crates/matrix-sdk-crypto/src/store/mod.rs

+
+    /// The maximum number of messages an encryption session should be used for,
+    /// before it is rotated.
+    pub session_rotation_period_msgs: Option<usize>,


Can we use long names please, like session_rotation_period_for_messages maybe?

We can, but it might be worth noting that this name is based on the name in the matrix protocol (rotation_period_msgs, https://spec.matrix.org/v1.9/client-server-api/#mroomencryption)

I'm not really sure I can think of a better name than session_rotation_period_messages. number_of_messages_to_rotate_session_after feels very cumbersome.

Oh OK. Then session_rotation_period_messages is fine for me. Thanks!

We can, but it might be worth noting that this name is based on the name in the matrix protocol (rotation_period_msgs, https://spec.matrix.org/v1.9/client-server-api/#mroomencryption)

There is also a rotation_period_msgs in EncryptionSettings:

matrix-rust-sdk/crates/matrix-sdk-crypto/src/olm/group_sessions/outbound.rs

Line 82 in 1d6c01f

pub rotation_period_msgs: u64,

... though why it uses a u64 I don't know.

Oh OK. Then session_rotation_period_messages is fine for me. Thanks!

Ok!

richvdh · 2024-01-23T11:05:33Z

@Hywan I think I've addressed all your comments. Could you take another look?

Hywan

Thanks!

Signed-off-by: Richard van der Hoff <[email protected]>

richvdh requested a review from a team as a code owner January 19, 2024 18:41

richvdh requested review from andybalaam and removed request for a team January 19, 2024 18:41

richvdh commented Jan 19, 2024

View reviewed changes

richvdh force-pushed the rav/room_settings branch from d81456a to 78b5b2a Compare January 19, 2024 18:58

crypto: expand RoomSettings to cover session rotation settings.

b4b460a

richvdh force-pushed the rav/room_settings branch 3 times, most recently from 7ed63ae to 532897a Compare January 19, 2024 19:07

richvdh added 2 commits January 19, 2024 19:15

memorystore: implement room settings saving

5e0e752

richvdh force-pushed the rav/room_settings branch from 532897a to 2314a74 Compare January 19, 2024 19:15

Hywan requested changes Jan 22, 2024

View reviewed changes

Address review comments

ac24f62

richvdh changed the title ~~crypto: Implement OlmMachine::{get,set}_room_settings~~ crypto: Implement OlmMachine::{set_,}room_settings Jan 22, 2024

Rename session_rotation_period_msgs

75ea0ad

richvdh requested a review from Hywan January 23, 2024 11:05

richvdh mentioned this pull request Jan 24, 2024

Element R: Persistent room list element-hq/element-web#26108

Closed

5 tasks

Hywan approved these changes Jan 25, 2024

View reviewed changes

Merge branch 'main' into rav/room_settings

0b01a2a

Signed-off-by: Richard van der Hoff <[email protected]>

Hywan enabled auto-merge January 25, 2024 15:19

Hywan merged commit 4d2c3e3 into main Jan 25, 2024
34 checks passed

Hywan deleted the rav/room_settings branch January 25, 2024 15:28

poljar mentioned this pull request Feb 2, 2024

Have the crypto crate handle some subset of state events #1325

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

crypto: Implement `OlmMachine::{set_,}room_settings` #3042

crypto: Implement `OlmMachine::{set_,}room_settings` #3042

richvdh commented Jan 19, 2024 •

edited

Loading

richvdh Jan 19, 2024

Hywan Jan 22, 2024

codecov bot commented Jan 19, 2024 •

edited

Loading

Hywan left a comment

Hywan Jan 22, 2024

richvdh Jan 22, 2024

Hywan Jan 22, 2024

Hywan Jan 22, 2024

Hywan Jan 22, 2024

Hywan Jan 22, 2024

Hywan Jan 22, 2024

richvdh Jan 22, 2024

Hywan Jan 22, 2024

richvdh Jan 22, 2024 •

edited

Loading

richvdh Jan 22, 2024

richvdh commented Jan 23, 2024

Hywan left a comment •

edited

Loading

crypto: Implement OlmMachine::{set_,}room_settings #3042

crypto: Implement OlmMachine::{set_,}room_settings #3042

Conversation

richvdh commented Jan 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 19, 2024 • edited Loading

Codecov Report

Hywan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh commented Jan 23, 2024

Hywan left a comment • edited Loading

Choose a reason for hiding this comment

crypto: Implement `OlmMachine::{set_,}room_settings` #3042

crypto: Implement `OlmMachine::{set_,}room_settings` #3042

richvdh commented Jan 19, 2024 •

edited

Loading

codecov bot commented Jan 19, 2024 •

edited

Loading

richvdh Jan 22, 2024 •

edited

Loading

Hywan left a comment •

edited

Loading