Invisible Crypto | Share Room Keys: Update the Identity Based sharing strategy to use the tofu_trusted flag

[BillCarsonFr]

Part of https://github.com/element-hq/crypto-internal/issues/282 (Invisible Crypto).

#3563 added a new IdentityBasedStrategy room-key sharing strategy. #3564 added a "tofu" flag to ReadOnlyUserIdentity. We now need to update share_room_keys to check the tofu flag.

Add new cases to OlmError, to be returned by share_room_keys:

• OlmError::UnconfirmedIdentityChange(Vec<OwnedUserId>) if the user has not yet acknowledged an identity change for the recipient; and: Update 2024-08-08: we no longer want this behaviour.
• OlmError::SendingFromUnverifiedDevice if our current device has not yet been verified.

[richvdh]

@BillCarsonFr:

Add new errors that share_room_keys will throw based on UnconfirmedIdentityChange(Vec<OwnedUserId>) SendingFromUnverifiedDevice.

I don't understand this. What does "based on UnconfirmedIdentityChange(Vec<OwnedUserId>) SendingFromUnverifiedDevice" mean?

[BillCarsonFr]

@BillCarsonFr:

Add new errors that share_room_keys will throw based on UnconfirmedIdentityChange(Vec<OwnedUserId>) SendingFromUnverifiedDevice.

I don't understand this. What does "based on UnconfirmedIdentityChange(Vec<OwnedUserId>) SendingFromUnverifiedDevice" mean?

It's some new error variants that would be part of this

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

Lines 32 to 72 in 8e8d793

	pub enum OlmError {
	/// The event that should have been decrypted is malformed.
	#[error(transparent)]
	EventError(#[from] EventError),
	/// The received decrypted event couldn't be deserialized.
	#[error(transparent)]
	JsonError(#[from] SerdeError),
	/// The received room key couldn't be converted into a valid Megolm session.
	#[error(transparent)]
	SessionCreation(#[from] SessionCreationError),
	/// The room key that should be exported can't be converted into a
	/// `m.forwarded_room_key` event.
	#[error(transparent)]
	SessionExport(#[from] SessionExportError),
	/// The storage layer returned an error.
	#[error("failed to read or write to the crypto store {0}")]
	Store(#[from] CryptoStoreError),
	/// The session with a device has become corrupted.
	#[error(
	"decryption failed likely because an Olm session from {0} with sender key {1} was wedged"
	)]
	SessionWedged(OwnedUserId, Curve25519PublicKey),
	/// An Olm message got replayed while the Olm ratchet has already moved
	/// forward.
	#[error("decryption failed because an Olm message from {0} with sender key {1} was replayed")]
	ReplayedMessage(OwnedUserId, Curve25519PublicKey),
	/// Encryption failed because the device does not have a valid Olm session
	/// with us.
	#[error(
	"encryption failed because the device does not \
	have a valid Olm session with us"
	)]
	MissingSession,
	}

It will be thrown by share_room_key (returns an olm result).

In the happy path UnconfirmedIdentityChange shouldn't happen. The UI is supposed to show you asap when an identity has changed, and mark the new identity back as tofu trusted.
But to prevent some race or a HS trying to take advantage by changing just before you send this error can be thrown.
It also contains the list of identities affected to give proper feedback.

for SendingFromUnverifiedDevice, the UI should at least block sending in e2e room if the current device not verified. So happy path should not happen, but can still be an error

[richvdh]

Thanks. I've updated the description. Can you confirm?

[andybalaam]

@uhoreg will shepherd this one.

[richvdh]

See also #3793 which will update the sharing code to throw an error if the other user's identity was previously verified, and changes.

[richvdh]

Per recent discussions, we no longer want to throw an error on message send if a TOFU-trusted user rotates their identity. In short, if identity trust matters to you so much that you are worried about the race between sending a message and seeing an identity change notification, then you should have verified.

[BillCarsonFr]

Per recent discussions, we no longer want to throw an error on message send if a TOFU-trusted user rotates their identity. In short, if identity trust matters to you so much that you are worried about the race between sending a message and seeing an identity change notification, then you should have verified.

In the recent discussions we said it was fine because we are going to show a warning notice to the user that the identity has changed. It's not that important if the warning notice is showned it a bit after the message is sent, what is important is that the warning is shown.

What if we cannot ensure that the warning notice can be shown by the client? For example on mobile if the user do a quick reply/suggestion from the notification (that means sending without opening the app)? Or using the share sheet to share an image from another app.

In these cases the client will not be able to show the notice as it doesn't control the UI. Are we confident that the next time the user opens the app that the warning notice will still be on screen (and not way back in history?)

I am wondering if we might want a config option to throw on pin violation?
Might be usefull for bots too? Will make it easier to detect and handle pin violation?

[richvdh]

What if we cannot ensure that the warning notice can be shown by the client? For example on mobile if the user do a quick reply/suggestion from the notification (that means sending without opening the app)? Or using the share sheet to share an image from another app.

In these cases the client will not be able to show the notice as it doesn't control the UI. Are we confident that the next time the user opens the app that the warning notice will still be on screen (and not way back in history?)

We discussed this. The conclusion was that it was acceptable even if the warning was lost in backfill. The general point is: if you are paranoid, you should be verifying the other user.