Crypto: Return an error when sharing a room key to a verified user, who has an unverified device (optional encryption setting) #3810

BillCarsonFr · 2024-08-07T07:27:10Z

Suggest review commit-by-commit.

Public API changes documented in changelogs (optional)

codecov · 2024-08-08T14:36:54Z

Codecov Report

Attention: Patch coverage is 86.84211% with 10 lines in your changes missing coverage. Please review.

Project coverage is 84.09%. Comparing base (4ece38a) to head (5f24f8e).
Report is 13 commits behind head on main.

Files	Patch %	Lines
...c/session_manager/group_sessions/share_strategy.rs	85.71%	9 Missing ⚠️
crates/matrix-sdk-crypto/src/identities/device.rs	83.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3810      +/-   ##
==========================================
+ Coverage   84.07%   84.09%   +0.01%     
==========================================
  Files         262      262              
  Lines       27544    27581      +37     
==========================================
+ Hits        23158    23193      +35     
- Misses       4386     4388       +2

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

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

BillCarsonFr · 2024-08-08T14:59:20Z

crates/matrix-sdk-crypto/src/identities/device.rs

+    /// Whitelisted devices will receive group sessions regardless of their
+    /// verification status.
+    pub fn is_whitelisted(&self) -> bool {
+        self.local_trust_state() == LocalTrust::Ignored


The LocalTrust::Ignored actually is really Whitelist, as it says that the trust state should be ignored. Should it be renamed to Whitelisted? to be consistent with LocalTrust::BlackListed?

yeahh, this feels pretty confusing to me, and could do with clean up.

That said: "whitelisted"/"blacklisted" are somewhat problematic terms, and we might take the opportunity to rename both.

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

BillCarsonFr · 2024-08-08T15:06:37Z

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

@@ -228,16 +313,61 @@ fn split_recipients_withhelds_for_user(
        user_devices.into_values().partition_map(|d| {
            if d.is_blacklisted() {


This if/else sequence is getting harder and harder to read with all the possible settings/outcome.
We might want to get rid of the partition/Either system.
Also the set of settings can create invalid configuration, like only_allow_trusted_devices + error_on_unsigned_device_of_verified_users doesn't really make sense.
Will get worse with more options

richvdh

Looks broadly good to me. Some initial thoughts, but nothing major -- except my question about checking own_identity.is_verified().

I'll take a closer look tomorrow.

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

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

richvdh

A few more, more nitpicky comments. I still need to take a closer look at the tests.

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

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

richvdh · 2024-08-09T10:08:51Z

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

+        /// Once the problematic devices are blacklisted or whitelisted the
+        /// caller can retry to share a second time. This has to be done
+        /// once per problematic device.
+        #[serde(default)]


is this persisted somewhere, that makes the default necessary?

EncryptionSettings is persisted in the OutboundSession at least, and maybe some clients could persist the encryption settings? All the things we put as serializable could be persisted... (are we serializing things for other reasons? like for bindings to pass things back and forth?)
We really need to improve that part of the sdk, it's too easy to make a change that would break applications.
I thought about having tests with json exports of all the serializable things that are persisted, that just tries to load them back and see if it breaks. This would be minimum. Or we need to think about a specific trait for things that are expected to be serialized persisted, with a version of some form

EncryptionSettings is persisted in the OutboundSession at least

Hrm... does that means that, once we enable the new behaviour, it won't work until megolm sessions are replaced?

I don't think so. But there is something odd with the settings how they are now.

The settings persisted is used mainly (i think) in the code to detect if the session should be rotated.
I think that only part of the EncryptionSetting should be persisted (rotation_period, rotation_period_msgs, and history_visibility?).
The old only_allow_trusted_devices that was in the EncryptionSetting (replaced now by CollectStrategy) shouldn't be persisted. Because anyhow if a change on this setting change the set of devices that should receive it, then is_session_overshared_for_user will detect it and rotate the session.

history_visibility is different because you want to rotate if you decide that this session could be shared now as part of history sharing for example, event if it doesn't change right now who get the key

I'm not really following all of this. It sounds like we shouldn't be persisting EncryptionSettings at all to me.

If you change the rotation_period you would want the session to be rotated immediatly instead of having to wait for the old one to be rotated based on the old rotation period. Same for history_visibility, you want to make history sharable from the point you change to that setting and not make early history available?

Both of those things sound like we should be taking notice of the current EncryptionSettings, not whatever was persisted to the store.

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

richvdh · 2024-08-09T10:20:54Z

crates/matrix-sdk-crypto/src/identities/device.rs

+    /// Whitelisted devices will receive group sessions regardless of their
+    /// verification status.
+    pub fn is_whitelisted(&self) -> bool {
+        self.local_trust_state() == LocalTrust::Ignored


yeahh, this feels pretty confusing to me, and could do with clean up.

That said: "whitelisted"/"blacklisted" are somewhat problematic terms, and we might take the opportunity to rename both.

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

richvdh · 2024-08-13T16:55:38Z

bindings/matrix-sdk-crypto-ffi/src/lib.rs

+    /// Should fail to send when a verified user has unverified devices.
+    pub error_on_verified_user_problem: bool,


This is a slightly rubbish name; ultimately I want to use it for both unverified devices (#3792) and verified users that become unverified (#3793).

bnjbvr

On request, I've just skimmed the PR and looked at Rust idioms. I've got some small suggestions to improve readability of the code, which I think should be addressed; confusing a bool for another could be particularly bad in the context of crypto, IMO. LGTM otherwise!

crates/matrix-sdk-base/src/client.rs

bnjbvr · 2024-08-14T10:53:06Z

crates/matrix-sdk-crypto/src/identities/device.rs

-                        }
+                match device_identity {
+                    UserIdentityData::Own(_) => {
+                        own_identity.is_verified() && own_identity.is_device_signed(self).is_ok()


preexisting, and can be done in another PR: having is_device_signed return a Result when all the callers check for is_ok() makes me think it should just return a boolean instead of a Result 👀

Agreed. Let's fix that in another PR.

bnjbvr · 2024-08-14T10:53:50Z

crates/matrix-sdk-crypto/src/identities/user.rs

+    /// * `identity` - The identity of another user which we want to check has
+    ///   been verified.
+    pub fn is_identity_verified(&self, identity: &OtherUserIdentityData) -> bool {
+        self.is_verified() && self.is_identity_signed(identity).is_ok()


same here for is_identity_signed

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

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

bnjbvr

Super nice, thanks!

Use a for loop rather than `partition_map`. We're about to add a third list, so partition_map won't work. (partition_map ends up using Vec::push under the hood, so this is pretty much equivalent.)

... and use it to remove a bit of duplicated code.

The list of boolean arguments is confusing. We may as well just construct the `DeviceBasedStrategy` directly.

Again, the list of boolean arguments is confusing.

Add (as yet unimplemented) `error_on_verified_user_problem` option

This thing was confusing. What is "legacy" about it?

BillCarsonFr changed the title ~~Valere/invisible crypto/error on unsigned device of verified user~~ Crypto: Return an error when sharing a room key to a verified user, who has an unverified device (optional encryption setting) Aug 7, 2024

BillCarsonFr force-pushed the valere/invisible_crypto/verification_pin branch 2 times, most recently from e715223 to 1b05380 Compare August 7, 2024 13:06

BillCarsonFr force-pushed the valere/invisible_crypto/error_on_unsigned_device_of_verified_user branch from b643839 to 6e6f71d Compare August 7, 2024 15:38

BillCarsonFr mentioned this pull request Aug 8, 2024

Crypto: return error when sharing keys with a user that was previously verified but is not any more #3816

Merged

Base automatically changed from valere/invisible_crypto/verification_pin to main August 8, 2024 10:10

BillCarsonFr force-pushed the valere/invisible_crypto/error_on_unsigned_device_of_verified_user branch from 6e6f71d to 9874e40 Compare August 8, 2024 14:18

BillCarsonFr force-pushed the valere/invisible_crypto/error_on_unsigned_device_of_verified_user branch from 9874e40 to de4cc83 Compare August 8, 2024 14:39

BillCarsonFr marked this pull request as ready for review August 8, 2024 14:56

BillCarsonFr requested review from a team as code owners August 8, 2024 14:56

BillCarsonFr requested review from poljar and richvdh and removed request for a team August 8, 2024 14:56

BillCarsonFr commented Aug 8, 2024

View reviewed changes

crates/matrix-sdk-crypto/src/error.rs Outdated Show resolved Hide resolved

BillCarsonFr commented Aug 8, 2024

View reviewed changes

crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs Outdated Show resolved Hide resolved

BillCarsonFr commented Aug 8, 2024

View reviewed changes

richvdh reviewed Aug 8, 2024

View reviewed changes

richvdh reviewed Aug 9, 2024

View reviewed changes

richvdh marked this pull request as draft August 12, 2024 11:04

crypto: extract function that checks if session is shared too much

7784f3e

richvdh force-pushed the valere/invisible_crypto/error_on_unsigned_device_of_verified_user branch 2 times, most recently from fbc66d6 to e1ad36f Compare August 13, 2024 16:53

richvdh reviewed Aug 13, 2024

View reviewed changes

richvdh force-pushed the valere/invisible_crypto/error_on_unsigned_device_of_verified_user branch from e1ad36f to 37329ea Compare August 13, 2024 17:23

richvdh marked this pull request as ready for review August 14, 2024 09:32

bnjbvr requested changes Aug 14, 2024

View reviewed changes

richvdh force-pushed the valere/invisible_crypto/error_on_unsigned_device_of_verified_user branch from ea1d8a2 to 5dbc599 Compare August 14, 2024 12:04

richvdh requested a review from bnjbvr August 14, 2024 12:17

bnjbvr approved these changes Aug 14, 2024

View reviewed changes

richvdh and others added 11 commits August 14, 2024 14:01

crypto: minor cleanups in is_session_overshared_for_user

cf046da

crypto: rafactor split_recipients_withhelds_for_user

326e87d

Use a for loop rather than `partition_map`. We're about to add a third list, so partition_map won't work. (partition_map ends up using Vec::push under the hood, so this is pretty much equivalent.)

crypto: add OwnUserIdentityData::is_identity_verified

f555e07

... and use it to remove a bit of duplicated code.

crypto: remove CollectStrategy::new_device_based

ede5e5a

The list of boolean arguments is confusing. We may as well just construct the `DeviceBasedStrategy` directly.

crypto: change EncryptionSettings::new to take a CollectStrategy

cfe16c4

Again, the list of boolean arguments is confusing.

crypto: extend CollectionStrategy::DeviceBasedStrategy

2e4e11c

Add (as yet unimplemented) `error_on_verified_user_problem` option

crypto: key sharing error for verified user with unverified devices

afe4015

crypto: test: factor out create_test_outbound_group_session helper

cdb90e5

crypto: test: factor out redundant variable

2ed73b8

This thing was confusing. What is "legacy" about it?

crypto: test: add tests for error_on_verified_user_problem

56df4b3

crypto: Update changelog

5f24f8e

richvdh force-pushed the valere/invisible_crypto/error_on_unsigned_device_of_verified_user branch from 38a4477 to 5f24f8e Compare August 14, 2024 13:42

richvdh removed the request for review from poljar August 14, 2024 13:54

richvdh enabled auto-merge (rebase) August 14, 2024 13:54

richvdh merged commit 1e8dd5d into main Aug 14, 2024
40 checks passed

richvdh deleted the valere/invisible_crypto/error_on_unsigned_device_of_verified_user branch August 14, 2024 13:57

This was referenced Aug 16, 2024

crypto(feat): Key distribution errors for pin violations #3662

Closed

Expose room_key_recipient_strategy through higher-level SDK crates #3864

Merged

uhoreg mentioned this pull request Sep 5, 2024

Update to latest Rust SDK matrix-org/matrix-rust-sdk-crypto-wasm#141

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Crypto: Return an error when sharing a room key to a verified user, who has an unverified device (optional encryption setting) #3810

Crypto: Return an error when sharing a room key to a verified user, who has an unverified device (optional encryption setting) #3810

BillCarsonFr commented Aug 7, 2024 •

edited by richvdh

Loading

codecov bot commented Aug 8, 2024 •

edited

Loading

BillCarsonFr Aug 8, 2024

richvdh Aug 9, 2024

BillCarsonFr Aug 8, 2024

richvdh left a comment

richvdh left a comment

richvdh Aug 9, 2024

BillCarsonFr Aug 9, 2024

richvdh Aug 9, 2024

BillCarsonFr Aug 9, 2024

richvdh Aug 9, 2024

BillCarsonFr Aug 9, 2024

richvdh Aug 12, 2024

richvdh Aug 9, 2024

richvdh Aug 13, 2024

bnjbvr left a comment

bnjbvr Aug 14, 2024

richvdh Aug 14, 2024

bnjbvr Aug 14, 2024

bnjbvr left a comment

		@@ -228,16 +313,61 @@ fn split_recipients_withhelds_for_user(
		user_devices.into_values().partition_map(\|d\| {
		if d.is_blacklisted() {

		/// Should fail to send when a verified user has unverified devices.
		pub error_on_verified_user_problem: bool,

Crypto: Return an error when sharing a room key to a verified user, who has an unverified device (optional encryption setting) #3810

Crypto: Return an error when sharing a room key to a verified user, who has an unverified device (optional encryption setting) #3810

Conversation

BillCarsonFr commented Aug 7, 2024 • edited by richvdh Loading

codecov bot commented Aug 8, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh left a comment

Choose a reason for hiding this comment

richvdh 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

bnjbvr 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

bnjbvr left a comment

Choose a reason for hiding this comment

BillCarsonFr commented Aug 7, 2024 •

edited by richvdh

Loading

codecov bot commented Aug 8, 2024 •

edited

Loading