refactor(sdk-crypto): Room key sharing, introduce extensible strategy #3571

BillCarsonFr · 2024-06-18T12:37:40Z

Extracted the logic to collect the devices that should receive a room key.
Introduce a new CollectStrategy that will define how to sort devices that should receive the room_key and those that should receive a witheld code.
The common part of sharing (checking if the session should be rotated) is done by the CollectRecipientsHelper, then the actual selection is delegated depending on th CollectStrategy

There is no functional changes in this PR, a following PR will introduce an IdentityBased strategy that will select devices depending on if they are signed by their owner.

Also created some testing data that can be reused for various tests, will be used by the new strategy also. This is isolated in the first commit.

Public API changes documented in changelogs (optional)

Signed-off-by:

codecov · 2024-06-18T14:39:52Z

Codecov Report

Attention: Patch coverage is 82.19178% with 13 lines in your changes missing coverage. Please review.

Project coverage is 83.87%. Comparing base (b34a2cd) to head (ae0ff21).
Report is 62 commits behind head on main.

Files	Patch %	Lines
...dk-crypto/src/olm/group_sessions/share_strategy.rs	81.15%	13 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3571      +/-   ##
==========================================
+ Coverage   83.84%   83.87%   +0.03%     
==========================================
  Files         253      254       +1     
  Lines       25884    25903      +19     
==========================================
+ Hits        21702    21727      +25     
+ Misses       4182     4176       -6

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

andybalaam

Looks good, with a few questions and comments. I'd feel more comfortable if @poljar had time to look too.

andybalaam · 2024-06-19T12:53:15Z

crates/matrix-sdk-crypto/src/olm/group_sessions/share_strategy.rs

+        let dan_device_that_will_get_the_key = &dan_devices_shared[0];
+        assert_eq!(dan_device_that_will_get_the_key.device_id().as_str(), "JHPUERYQUW");
+
+        // Check withthelds for others


Suggested change

// Check withthelds for others

// Check withhelds for others

andybalaam · 2024-06-19T12:54:20Z

crates/matrix-sdk-crypto/src/olm/group_sessions/share_strategy.rs

+        let (_, code) = share_result
+            .withheld_devices
+            .iter()
+            .find(|(d, _)| d.device_id().as_str() == "FRGNMZVOKA")


Can we use something like KeyDistributionTestData::dan_device() for this to avoid a magic constant?

andybalaam · 2024-06-19T13:07:38Z

crates/matrix-sdk-crypto/src/olm/group_sessions/share_strategy.rs

+        /// conversation. A device is trusted if any of the following is true:
+        ///     - It was manually marked as trusted.
+        ///     - It was marked as verified via interactive verification.
+        ///     - It is signed by it's owner identity, and this identity has


Suggested change

/// - It is signed by it's owner identity, and this identity has

/// - It is signed by its owner identity, and this identity has

andybalaam · 2024-06-19T13:09:56Z

crates/matrix-sdk-crypto/src/olm/group_sessions/share_strategy.rs

+    pub withheld_devices: Vec<(ReadOnlyDevice, WithheldCode)>,
+}
+
+pub(crate) struct CollectRecipientsHelper {}


Is there any need for this struct? Can't we just have collect_session_recipients as a free function?

Agree there.

andybalaam · 2024-06-19T13:12:59Z

crates/matrix-sdk-crypto/src/olm/group_sessions/share_strategy.rs

+        // rotated for other reasons, we also need to check whether any
+        // of the devices in the session got deleted or blacklisted in the
+        // meantime. If so, we should also rotate the session.
+        if !should_rotate {


I would prefer if the contents of this if were a separate function - the containing function is very long.

I would probably prefer the body of the for loop to be a separate function, or even better the body inside the if let Some(shared) conditional to be put into a method on the OutboundGroupSession type.

andybalaam · 2024-06-19T13:13:52Z

crates/matrix-sdk-crypto/src/olm/group_sessions/share_strategy.rs

+    }
+
+    async fn collect_session_recipients_device_based(
+        // &self,


Remove commented code.

poljar

This PR is called a refactor but a non-trivial amount of behavioral changes has snuck in. Some of them seem to be bugs as well.

poljar · 2024-06-20T12:36:35Z

testing/matrix-sdk-test/src/test_json/keys_query_sets.rs

+
+use crate::response_from_file;
+
+/// ============================================================================


I don't think we have a convention of adding such a header, doesn't really add anything I would say.

It does look weird once rednered:

poljar · 2024-06-21T12:52:29Z

crates/matrix-sdk-crypto/src/olm/group_sessions/share_strategy.rs

+    pub withheld_devices: Vec<(ReadOnlyDevice, WithheldCode)>,
+}
+
+pub(crate) struct CollectRecipientsHelper {}


Agree there.

poljar · 2024-06-21T12:54:50Z

crates/matrix-sdk-crypto/src/olm/group_sessions/share_strategy.rs

+}
+
+impl CollectRecipientsHelper {
+    pub(crate) async fn collect_session_recipients(


It would have been much nicer if you first moved this function to this new file in a separate commit and only then started to modify it. It's a bit annoying to see what has changed and what was just moved.

For reference, I need to pick out the diff manually via:

git diff origin/main:crates/matrix-sdk-crypto/src/session_manager/group_sessions.rs crates/matrix-sdk-crypto/src/olm/group_sessions/share_strategy.rs

Even with this, the diff isn't smart enough to notice what was split out where.

Can you clarify? Should I drop everything and do a new PR and do that?

No, please don't open a new PR, it took quite a bit effort to review it and please don't throw that work now away.

Can you clarify?

Let us introduce a simplified example, let's suppose that you have a file with this function:

// main.rs fn greet() { println!("Hello, world!"); }

Now you need to refactor and slightly modify this function, you need add a parameter to it, you need to do the following changes:

Delete the function from main.rs.

Create a new file greetings.rs.

Add the function to greetings.rs

Modify the function to take a parameter

// greetings.rs fn greet(name: &str) { println!("Hello, {}!", name); }

If you do all 4 steps in a single commit the diff will look like:

--- main.rs +++ main.rs @@ -1,4 +1,0 @@ -// main.rs - -fn greet() { - println!("Hello, world!"); -} --- /dev/null +++ greetings.rs @@ -0,0 +1,4 @@ +// greetings.rs + +fn greet(name: &str) { + println!("Hello, {}!", name); +}

The modifications are a bit hard to spot, aren't they? Sure the function is 3 lines long, you can easily spot them if you pay attention and compare the green and red lines carefully.

Now if you instead commit step 1-3 first and commit step 4 separately you get two diffs which look like this:

--- main.rs +++ main.rs @@ -1,4 +1,0 @@ -// main.rs - -fn greet() { - println!("Hello, world!"); -} --- /dev/null +++ greetings.rs @@ -0,0 +1,4 @@ +// greetings.rs + +fn greet() { + println!("Hello, world!"); +}

and:

--- greetings.rs +++ greetings.rs @@ -1,4 +1,4 @@ // greetings.rs -fn greet() { - println!("Hello, world!"); +fn greet(name: &str) { + println!("Hello, {}!", name); }

This makes the changes to the function stand out quite clearly, the first commit can be quickly checked if the move is done correctly, something Rust helps us with as well, and then we can focus our attention to the meaningful functional modifications.

poljar · 2024-06-21T13:02:44Z

crates/matrix-sdk-crypto/src/olm/group_sessions/share_strategy.rs

+        let sharing_strategy_changed =
+            outbound.settings().sharing_strategy != settings.sharing_strategy;
+
+        let mut should_rotate =


This function seems to have received a downgrade in the documentation department, why was this comment removed:

matrix-rust-sdk/crates/matrix-sdk-crypto/src/session_manager/group_sessions.rs

Lines 373 to 381 in 2b1bddb

// To protect the room history we need to rotate the session if either:

//

// 1. Any user left the room.

// 2. Any of the users' devices got deleted or blacklisted.

// 3. The history visibility changed.

// 4. The encryption algorithm changed.

//

// This is calculated in the following code and stored in this variable.

let mut should_rotate = user_left || visibility_changed || algorithm_changed;

poljar · 2024-06-21T13:03:51Z

crates/matrix-sdk-crypto/src/olm/group_sessions/share_strategy.rs

+        let sharing_strategy_changed =
+            outbound.settings().sharing_strategy != settings.sharing_strategy;
+
+        let mut should_rotate =


This function seems to have received a downgrade in the documentation department, why was this comment removed:

matrix-rust-sdk/crates/matrix-sdk-crypto/src/session_manager/group_sessions.rs

Lines 373 to 381 in 2b1bddb

// To protect the room history we need to rotate the session if either:

//

// 1. Any user left the room.

// 2. Any of the users' devices got deleted or blacklisted.

// 3. The history visibility changed.

// 4. The encryption algorithm changed.

//

// This is calculated in the following code and stored in this variable.

let mut should_rotate = user_left || visibility_changed || algorithm_changed;

poljar · 2024-06-21T13:55:10Z

crates/matrix-sdk-crypto/src/olm/group_sessions/share_strategy.rs

+        // rotated for other reasons, we also need to check whether any
+        // of the devices in the session got deleted or blacklisted in the
+        // meantime. If so, we should also rotate the session.
+        if !should_rotate {


I would probably prefer the body of the for loop to be a separate function, or even better the body inside the if let Some(shared) conditional to be put into a method on the OutboundGroupSession type.

poljar · 2024-06-21T14:06:25Z

crates/matrix-sdk-crypto/src/session_manager/group_sessions.rs

-        trace!(should_rotate, "Done calculating group session recipients");
-
-        Ok(CollectRecipientsResult { should_rotate, devices, withheld_devices })
+        CollectRecipientsHelper::collect_session_recipients(settings, &self.store, users, outbound)


This really feels like needless indirection. Perhaps it would make sense to make the collect_session_recipients() a constructor of CollectRecipientsResult if you don't like the free standing function suggestion.

Thanks for the direction. I am not sure what's best but I think I will go with the free standing

poljar · 2024-06-21T14:10:32Z

crates/matrix-sdk-crypto/src/olm/group_sessions/share_strategy.rs

+        })
+    }
+
+    async fn collect_session_recipients_device_based(


I think it makes sense for this to be a free standing method or to be bound to the DeviceBasedStrategy enum variant, though you'll have to change the variant to be a tuple variant and add a struct for it, so perhaps a free standing function again makes more sense.

Furthermore, this could get a better name? It's not a different version of collect_session_recipients(), perhaps it could be called collect_recipient_devices().

poljar · 2024-06-21T14:22:18Z

crates/matrix-sdk-crypto/src/olm/group_sessions/share_strategy.rs

+                }
+            });
+
+            if !recipients.is_empty() {


Why was this conditional added? Furthermore if we think that this is useful, why was it added only for the recipients set and not for the withheld_recipients set? Because of the allocation on the user id and the or_default()?

It was annoying in the tests to have these empty buckets of devices. Also was not usefull because we checked earlier for removed users so no need to maintain an empty bucket when looking for left devices.

poljar · 2024-06-21T14:40:05Z

crates/matrix-sdk-crypto/src/olm/group_sessions/share_strategy.rs

+    pub withheld_devices: Vec<(ReadOnlyDevice, WithheldCode)>,
+}
+
+struct KeySharingResult {


Perhaps a better name is RecipientDevices?

BillCarsonFr · 2024-06-24T21:20:56Z

Will close and try to re-organize that PR into a new PR, that hopefully will match expectations

BillCarsonFr · 2024-06-25T09:33:13Z

Closed in favor of #3605

testing: Add a set of keys query set to use for crypto machine tests

3587a3d

BillCarsonFr force-pushed the valere/invisible_crypto/refactor_share branch from d584668 to 7f4d686 Compare June 18, 2024 12:38

BillCarsonFr changed the title ~~Valere/invisible crypto/refactor share~~ refactor(sdk-crypto): Room key sharing, introduce extensible strategy Jun 18, 2024

BillCarsonFr force-pushed the valere/invisible_crypto/refactor_share branch from 7f4d686 to 81a9f95 Compare June 18, 2024 12:39

refactor(sdk-crypto): Room key sharing, introduce extensible strategy

05ba146

BillCarsonFr force-pushed the valere/invisible_crypto/refactor_share branch from 81a9f95 to 05ba146 Compare June 18, 2024 12:58

BillCarsonFr marked this pull request as ready for review June 18, 2024 12:58

BillCarsonFr requested a review from a team as a code owner June 18, 2024 12:58

BillCarsonFr requested review from andybalaam and poljar and removed request for a team June 18, 2024 12:58

BillCarsonFr added 3 commits June 18, 2024 15:09

refactor(sdk-crypto): Room key sharing, Fix bindings

bdb7a0e

cleaning: unused import

9111b0e

refactor(sdk-crypto): Room key sharing, Fix test

fa35a6d

BillCarsonFr added 3 commits June 18, 2024 16:54

review: clippy fix

cbcb02b

testing: crypto - share room key - test session rotation

4d9fe1e

review: simplify share room key refactoring, remove unneeded trait

d806542

andybalaam approved these changes Jun 19, 2024

View reviewed changes

poljar requested changes Jun 21, 2024

View reviewed changes

BillCarsonFr added 2 commits June 24, 2024 10:50

review: fix typo

fa8caef

Code review: quick extract function and add new test for removed device

ae0ff21

BillCarsonFr closed this Jun 25, 2024

poljar deleted the valere/invisible_crypto/refactor_share branch November 5, 2024 11:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(sdk-crypto): Room key sharing, introduce extensible strategy #3571

refactor(sdk-crypto): Room key sharing, introduce extensible strategy #3571

BillCarsonFr commented Jun 18, 2024 •

edited

Loading

codecov bot commented Jun 18, 2024 •

edited

Loading

andybalaam left a comment

andybalaam Jun 19, 2024

andybalaam Jun 19, 2024

andybalaam Jun 19, 2024

andybalaam Jun 19, 2024

poljar Jun 21, 2024

andybalaam Jun 19, 2024

poljar Jun 21, 2024

andybalaam Jun 19, 2024

poljar left a comment

poljar Jun 20, 2024

poljar Jun 21, 2024

poljar Jun 21, 2024

BillCarsonFr Jun 24, 2024

poljar Jun 25, 2024 •

edited

Loading

poljar Jun 21, 2024

poljar Jun 21, 2024

poljar Jun 21, 2024

poljar Jun 21, 2024

BillCarsonFr Jun 24, 2024

poljar Jun 21, 2024

poljar Jun 21, 2024

BillCarsonFr Jun 24, 2024

poljar Jun 21, 2024

BillCarsonFr commented Jun 24, 2024

BillCarsonFr commented Jun 25, 2024

	/// - It is signed by it's owner identity, and this identity has
	/// - It is signed by its owner identity, and this identity has


		use crate::response_from_file;

		/// ============================================================================

	// To protect the room history we need to rotate the session if either:
	//
	// 1. Any user left the room.
	// 2. Any of the users' devices got deleted or blacklisted.
	// 3. The history visibility changed.
	// 4. The encryption algorithm changed.
	//
	// This is calculated in the following code and stored in this variable.
	let mut should_rotate = user_left \|\| visibility_changed \|\| algorithm_changed;

refactor(sdk-crypto): Room key sharing, introduce extensible strategy #3571

refactor(sdk-crypto): Room key sharing, introduce extensible strategy #3571

Conversation

BillCarsonFr commented Jun 18, 2024 • edited Loading

codecov bot commented Jun 18, 2024 • edited Loading

Codecov Report

andybalaam 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

poljar 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

poljar Jun 25, 2024 • edited Loading

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

BillCarsonFr commented Jun 24, 2024

BillCarsonFr commented Jun 25, 2024

BillCarsonFr commented Jun 18, 2024 •

edited

Loading

codecov bot commented Jun 18, 2024 •

edited

Loading

poljar Jun 25, 2024 •

edited

Loading