Configurable sender trust checking on decrypting #3701

uhoreg · 2024-07-15T22:05:21Z

Public API changes documented in changelogs (optional)

Signed-off-by:

…_sharing_with_pin' into invisible_crypto/decrypt

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

uhoreg · 2024-07-24T22:20:42Z

I'm not sure how to handle the higher levels. It seems like we should be storing the setting in some struct so that, e.g. messages that come down /sync get decrypted with the right setting, but I'm not sure where it should go. I don't see a relevant struct that has settings, so there isn't a clear place to put the setting.

jmartinesp · 2024-07-25T07:24:35Z

Sorry, I don't have much experience with the SDK and much less with the crypto part of it. Maybe someone else from the team can answer your questions? @poljar , @andybalaam?

uhoreg · 2024-08-09T01:27:07Z

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

@@ -74,6 +74,9 @@ pub enum SenderData {
        /// If false, we had simply accepted the key as this user's latest
        /// key.
        master_key_verified: bool,
+
+        #[serde(default)]
+        identity_needs_user_approval: bool,


I added this so that session_data_finder can find this information, so that the trust level can be calculated. But I think that it would be better to combine this with the master_key_verified field, and make them into a new master_key_status field that is an enum with Unverified, NeedsApproval, and Verified options. Maybe we should even add another option for the case where the user's previous identity was verified, but they rotated their identity. Maybe something like PreviousIdentityVerified.

What do you think?

(If you think it's better to keep it as-is, then I'll write documentation for this field, but I didn't want to write it if I was going to remove it right away.)

You've asked me for my opinion on this question, but this change doesn't seem to appear anywhere in the PR, so I don't think I have enough context on the change to form a sensible opinion.

uhoreg · 2024-08-09T01:44:39Z

I believe the tests fail because get_encryption_info now updates the sender_data, so I'll need to poke more things into the database so that it updates to the right thing. Aside from that, I think that this is ready for re-review.

richvdh · 2024-08-12T13:24:19Z

Discussion: should we throw an error when an identity has been rotated?

For a TOFU user, we should definitely just decrypt as normal (for symmetry with encryption)
For a verified user, we will throw an error on decryption if the identity is replaced (an expected UTD). The application can try to re-decrypt after the user re-verifies/reverts to unverified.

richvdh · 2024-08-12T13:24:50Z

@uhoreg will update this PR a bit and then get @richvdh to review

codecov · 2024-08-15T00:28:50Z

Codecov Report

Attention: Patch coverage is 91.30435% with 4 lines in your changes missing coverage. Please review.

Project coverage is 84.09%. Comparing base (6becbf6) to head (0e9b581).
Report is 106 commits behind head on main.

Files	Patch %	Lines
crates/matrix-sdk-crypto/src/machine/mod.rs	87.50%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3701      +/-   ##
==========================================
- Coverage   84.11%   84.09%   -0.02%     
==========================================
  Files         262      262              
  Lines       27599    27628      +29     
==========================================
+ Hits        23214    23235      +21     
- Misses       4385     4393       +8

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

uhoreg · 2024-08-15T22:38:11Z

@richvdh I think this is ready for review. I do have one question that I'd like an opinion on.

The commit history is a big mess, partially due to being based on a branch that got force-pushed at some point, so this PR should probably get squash-merged.

richvdh

Some initial thoughts.

I'm really struggling to follow this as one big diff. +600 lines isn't huge, but there is actually quite a lot changing here, and following the logic though everywhere is hard.

I think it might be better to start again on the commit history, and break it into smaller changes. For example - let's separate the changes to SenderData from the changes to decryption. Then add the DecryptionSettings parameter without actually implementing it, so that we can see the real changes without all the noise from that.

richvdh · 2024-08-16T16:01:18Z

crates/matrix-sdk-crypto/CHANGELOG.md

@@ -32,6 +32,9 @@ Changes:

 Breaking changes:

+- `OlmMachine::decrypt_room_event` now takes an `EncryptionSettings` argument.


Suggested change

- `OlmMachine::decrypt_room_event` now takes an `EncryptionSettings` argument.

- `OlmMachine::decrypt_room_event` now takes a `DecryptionSettings` argument.

we should probably also document some of the changes to publicly-visible types (MegolmError, VerificationLevel, etc)

richvdh · 2024-08-19T11:17:23Z

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

@@ -115,6 +116,10 @@ pub enum MegolmError {
    /// The storage layer returned an error.
    #[error(transparent)]
    Store(#[from] CryptoStoreError),
+
+    /// The sender's cross-signing identity isn't trusted
+    #[error("message quarantined because sender's cross-signing identity is not trusted: {0}")]


what does "quarantined" mean here? Is it stored somewhere within the store?

richvdh · 2024-08-19T11:18:27Z

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

+
+    /// The sender's cross-signing identity isn't trusted
+    #[error("message quarantined because sender's cross-signing identity is not trusted: {0}")]
+    SenderIdentity(#[from] VerificationLevel),


SenderIdentity feels like it doesn't quite describe the problem here.

Suggested change

SenderIdentity(#[from] VerificationLevel),

SenderIdentityNotTrusted(#[from] VerificationLevel),

richvdh · 2024-08-19T11:19:17Z

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

@@ -115,6 +116,10 @@ pub enum MegolmError {
    /// The storage layer returned an error.
    #[error(transparent)]
    Store(#[from] CryptoStoreError),
+
+    /// The sender's cross-signing identity isn't trusted


Suggested change

/// The sender's cross-signing identity isn't trusted

/// An encrypted message wasn't decrypted, because the sender's cross-signing identity isn't trusted.

(This should maybe also link to DecryptionSettings and say that it only happens under some conditions)

Actually, isn't it more "because the message could not be linked to a verified device" than specific to the sender's identity?

richvdh · 2024-08-19T11:23:06Z

crates/matrix-sdk-crypto/src/types/decryption.rs

+pub enum TrustRequirement {
+    /// Decrypt events from everyone regardless of trust.
+    Untrusted,
+    /// Only decrypt events from cross-signed or legacy sessions (Megolm


Suggested change

/// Only decrypt events from cross-signed or legacy sessions (Megolm

/// Only decrypt events from cross-signed devices or legacy sessions (Megolm

richvdh · 2024-08-19T12:19:13Z

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

+        /// TODO: should this be merged with the master_key_verified field and
+        /// turned into an enum?


Short answer: yes, this really feels like it ought to be a tri-state.

(#3846 added an OwnUserIdentityVerifiedState enum for a similar reason; not sure we should reuse that, but I think it's a pattern we should follow)

A downside is that it breaks backwards-compatibility for the crypto store :/

richvdh · 2024-08-19T12:20:47Z

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

+    ///
+    /// This is used by `check_sender_trust_requirement`, and ensures that the
+    /// sending device is cross-signed.
+    fn check_sender_trusted(&self, encryption_info: &EncryptionInfo) -> MegolmResult<()> {


per a discussion today in #matrix-rust-sdk-dev, helper functions should probably come after the functions they help.

richvdh · 2024-08-19T12:21:46Z

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

+// Helper function that encrypts a message and shares the Megolm session
+// with a recipient


helpers after the functions they help, please

richvdh · 2024-08-19T12:27:54Z

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

+        /// Whether the user's master key was previously verified, but the
+        /// current master key is not.


if this is the actual behaviour (and I'm not sure that it is), it needs a name that reflects it.

richvdh · 2024-08-19T12:29:42Z

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

@@ -74,6 +74,9 @@ pub enum SenderData {
        /// If false, we had simply accepted the key as this user's latest
        /// key.
        master_key_verified: bool,
+
+        #[serde(default)]
+        identity_needs_user_approval: bool,


You've asked me for my opinion on this question, but this change doesn't seem to appear anywhere in the PR, so I don't think I have enough context on the change to form a sensible opinion.

uhoreg · 2024-08-28T00:25:21Z

replaced by #3899

BillCarsonFr and others added 16 commits July 4, 2024 21:36

feature(crypto): Add support for master key local pinning

bb7383d

initial decryption implementation

e08dfab

add test for trust requirement

9415308

feature(crypto): Add support for master key local pinning

67af31a

Review: Do not use msk abbreviation

8253618

Review: Use TryFrom instead of From for serializer helper struct

b7241c3

Review: Fix typo

4d66828

Review: Improve doc

c045d26

Review: Fix data set comments

bfb8818

Review: Improve comments

fb39a9e

Clippy | unneeded clone

1d4cefb

refactor: Rename serializer helper to match new OtherUserIdentityData

de60e6e

implement logic for changing cross-signing keys, and add test

3731190

crypto: refactor extract detection of removed device from outbound

7803617

refactor: Move user loop inside strategy specific branch

9b00de3

feat(crypto): Key distribution errors for Pin violations

e309219

BillCarsonFr force-pushed the valere/invisible_crypto/identity_sharing_with_pin branch from 8fbd9a4 to e309219 Compare July 23, 2024 08:46

uhoreg added 3 commits July 23, 2024 15:46

Merge remote-tracking branch 'origin/valere/invisible_crypto/identity…

945f844

…_sharing_with_pin' into invisible_crypto/decrypt

remove debugging statements

f0e9a18

add some comments

7254e7e

uhoreg commented Jul 23, 2024

View reviewed changes

crates/matrix-sdk-crypto/src/olm/group_sessions/inbound.rs Outdated Show resolved Hide resolved

cargo fmt

3f7b840

uhoreg marked this pull request as ready for review July 24, 2024 14:59

uhoreg requested a review from a team as a code owner July 24, 2024 14:59

uhoreg requested review from jmartinesp and removed request for a team July 24, 2024 14:59

poljar requested review from poljar and removed request for jmartinesp July 25, 2024 07:43

uhoreg requested a review from BillCarsonFr August 9, 2024 00:51

uhoreg added 4 commits August 8, 2024 20:52

revert change to Cargo.lock

b0994a5

remove VerifiedUserIdentity requirement -- not used for now

c53674b

move decryption settings types into new file

773c8fe

add doc and fix compilation of other crates

4cbfd6b

uhoreg commented Aug 9, 2024

View reviewed changes

uhoreg added 3 commits August 8, 2024 21:30

lint

65ba067

fix bindings and more linting

e85bf3b

fix type

14d1e07

uhoreg requested a review from poljar August 9, 2024 01:46

more lint

e5c0c06

uhoreg added 2 commits August 14, 2024 13:35

only error on identity change if sender was previously verified

24a3ebb

fix logic and fix test

a119d79

uhoreg removed request for BillCarsonFr and poljar August 14, 2024 21:03

Merge branch 'main' into invisible_crypto/decrypt

fa61076

uhoreg added 4 commits August 15, 2024 13:12

fix clippy error

368699a

use the right punctuation

0a1f80b

fmt

04c81e3

add/fix comments and add changelog

0e9b581

uhoreg requested a review from richvdh August 15, 2024 20:37

richvdh requested changes Aug 19, 2024

View reviewed changes

uhoreg mentioned this pull request Aug 27, 2024

crypto: Configurable sender trust checking on decrypting #3899

Merged

1 task

uhoreg closed this Aug 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configurable sender trust checking on decrypting #3701

Configurable sender trust checking on decrypting #3701

uhoreg commented Jul 15, 2024 •

edited

Loading

uhoreg commented Jul 24, 2024

jmartinesp commented Jul 25, 2024

uhoreg Aug 9, 2024

richvdh Aug 19, 2024

uhoreg commented Aug 9, 2024

richvdh commented Aug 12, 2024

richvdh commented Aug 12, 2024

codecov bot commented Aug 15, 2024 •

edited

Loading

uhoreg commented Aug 15, 2024

richvdh left a comment

richvdh Aug 16, 2024

richvdh Aug 19, 2024

richvdh Aug 19, 2024

richvdh Aug 19, 2024

richvdh Aug 19, 2024

richvdh Aug 19, 2024

richvdh Aug 19, 2024

richvdh Aug 19, 2024

richvdh Aug 19, 2024

richvdh Aug 19, 2024

richvdh Aug 19, 2024

richvdh Aug 19, 2024

uhoreg commented Aug 28, 2024

		@@ -32,6 +32,9 @@ Changes:

		Breaking changes:

		- `OlmMachine::decrypt_room_event` now takes an `EncryptionSettings` argument.

	SenderIdentity(#[from] VerificationLevel),
	SenderIdentityNotTrusted(#[from] VerificationLevel),

	/// The sender's cross-signing identity isn't trusted
	/// An encrypted message wasn't decrypted, because the sender's cross-signing identity isn't trusted.

	/// Only decrypt events from cross-signed or legacy sessions (Megolm
	/// Only decrypt events from cross-signed devices or legacy sessions (Megolm

		/// TODO: should this be merged with the master_key_verified field and
		/// turned into an enum?

		// Helper function that encrypts a message and shares the Megolm session
		// with a recipient

		/// Whether the user's master key was previously verified, but the
		/// current master key is not.

Configurable sender trust checking on decrypting #3701

Configurable sender trust checking on decrypting #3701

Conversation

uhoreg commented Jul 15, 2024 • edited Loading

uhoreg commented Jul 24, 2024

jmartinesp commented Jul 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uhoreg commented Aug 9, 2024

richvdh commented Aug 12, 2024

richvdh commented Aug 12, 2024

codecov bot commented Aug 15, 2024 • edited Loading

Codecov Report

uhoreg commented Aug 15, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uhoreg commented Aug 28, 2024

uhoreg commented Jul 15, 2024 •

edited

Loading

codecov bot commented Aug 15, 2024 •

edited

Loading