Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(sdk-crypto): Add Identity local tofu trust flag. #3610

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 90 additions & 2 deletions crates/matrix-sdk-crypto/src/identities/manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -542,7 +542,7 @@ impl IdentityManager {
*changed_private_identity = self.check_private_identity(&identity).await;
Ok(identity.into())
} else {
let identity = ReadOnlyUserIdentity::new(master_key, self_signing)?;
let identity = ReadOnlyUserIdentity::new(master_key, self_signing, true)?;
Ok(identity.into())
}
}
Expand Down Expand Up @@ -1329,7 +1329,7 @@ pub(crate) mod tests {
use std::ops::Deref;

use futures_util::pin_mut;
use matrix_sdk_test::{async_test, response_from_file};
use matrix_sdk_test::{async_test, response_from_file, test_json};
use ruma::{
api::{client::keys::get_keys::v3::Response as KeysQueryResponse, IncomingResponse},
device_id, user_id, TransactionId,
Expand Down Expand Up @@ -1893,4 +1893,92 @@ pub(crate) mod tests {
.unwrap()
.unwrap();
}

#[async_test]
async fn test_manager_identity_updates() {
use test_json::keys_query_sets::IdentityChangeDataSet as DataSet;

let manager = manager_test_helper(user_id(), device_id()).await;
let other_user = DataSet::user_id();
let devices = manager.store.get_user_devices(other_user).await.unwrap();
assert_eq!(devices.devices().count(), 0);

let identity = manager.store.get_user_identity(other_user).await.unwrap();
assert!(identity.is_none());

manager
.receive_keys_query_response(
&TransactionId::new(),
&DataSet::key_query_with_identity_a(),
)
.await
.unwrap();

let identity = manager.store.get_user_identity(other_user).await.unwrap().unwrap();
let other_identity = identity.other().unwrap();

// There should be now an identity and it should be tofu trusted as it is the
// first time we see an identity for that user
assert!(other_identity.is_tofu_trusted());
let first_device = manager
.store
.get_readonly_device(other_user, DataSet::first_device_id())
.await
.unwrap()
.unwrap();
assert!(first_device.is_cross_signed_by_owner(&identity));

// We receive a new keys update for that user, with a new identity
manager
.receive_keys_query_response(
&TransactionId::new(),
&DataSet::key_query_with_identity_b(),
)
.await
.unwrap();

let identity = manager.store.get_user_identity(other_user).await.unwrap().unwrap();
let other_identity = identity.other().unwrap();

// The previous tofu identity has been replaced, it is not tofu trusted until
// validated by the user
assert!(!other_identity.is_tofu_trusted());

let second_device = manager
.store
.get_readonly_device(other_user, DataSet::second_device_id())
.await
.unwrap()
.unwrap();
// There is a new device signed by the new identity
assert!(second_device.is_cross_signed_by_owner(&identity));

// The first device should not be signed by the new identity
let first_device = manager
.store
.get_readonly_device(other_user, DataSet::first_device_id())
.await
.unwrap()
.unwrap();
assert!(!first_device.is_cross_signed_by_owner(&identity));

let remember_previous_identity = other_identity.clone();
// We receive a new keys update for that user, with no identity anymore
// Notice that there is no server API to delete identity, but we want to test
// here that a home server cannot clear the identity and serve a new one
// after that would get automatically tofu trusted.
manager
.receive_keys_query_response(
&TransactionId::new(),
&DataSet::key_query_with_identity_no_identity(),
)
.await
.unwrap();

let identity = manager.store.get_user_identity(other_user).await.unwrap().unwrap();
let other_identity = identity.other().unwrap();

assert_eq!(other_identity, &remember_previous_identity);
assert!(!other_identity.is_tofu_trusted());
}
}
54 changes: 50 additions & 4 deletions crates/matrix-sdk-crypto/src/identities/user.rs
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,24 @@ pub struct ReadOnlyUserIdentity {
user_id: OwnedUserId,
pub(crate) master_key: Arc<MasterPubkey>,
self_signing_key: Arc<SelfSigningPubkey>,
/// The first time an identity is seen for a given user it will be marked as
/// tofu trusted. If a new identity is detected for that user, we must
/// ensure that this change has been shown and validated by the user. If
/// this boolean is set to `false`, sharing room keys to this user might
/// fail depending on the room key sharing strategy.
#[serde(
default = "tofu_trusted_default",
serialize_with = "atomic_bool_serializer",
deserialize_with = "atomic_bool_deserializer"
)]
tofu_trusted: Arc<AtomicBool>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tofu_trusted doesn't seem like a great name for for a flag that means either "the first identity we saw for this user" or "not the first identity, but the user acknowledged it". TBH, I'm wondering if we should actually split it into two different flags.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or an enum, given that the two flags would be mutually exclusive?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or an enum, given that the two flags would be mutually exclusive?

I did think about that, but we still have the problem of what to call the damn thing

Copy link
Member

@dkasak dkasak Jun 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I discussed this with @BillCarsonFr and the semantics of the flag are roughly whether the identity is pinned or not, so perhaps the name could be pinned.

However, the way it's currently implemented—where we immediately replace the stored identity as soon as a new one is returned from the server, but with this flag set to false—means that a server can delete your pin by simply presenting a new identity and then switching back to the old one, resulting in the flag switching from true to false.

Valere is going to experiment with an alternative approach, where we

  • Remember the old pinned identity as long as a new identity doesn't become pinned, replacing the old one
  • Using the logic current != pinned to control whether the user should be notified / prompted to confirm

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dkasak Alternative for MSK pinning #3639

}

// The tofu_trusted field introduced a new field in the database
// schema, for backwards compatibility we assume that if the identity is
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// schema, for backwards compatibility we assume that if the identity is
// schema. For backwards compatibility, we assume that if the identity is

// already in the database we considered it as tofu trusted.
fn tofu_trusted_default() -> Arc<AtomicBool> {
Arc::new(true.into())
}

impl PartialEq for ReadOnlyUserIdentity {
Expand All @@ -395,6 +413,7 @@ impl PartialEq for ReadOnlyUserIdentity {
&& self.master_key == other.master_key
&& self.self_signing_key == other.self_signing_key
&& self.master_key.signatures() == other.master_key.signatures()
&& self.is_tofu_trusted() == other.is_tofu_trusted()
}
}

Expand All @@ -406,19 +425,22 @@ impl ReadOnlyUserIdentity {
/// * `master_key` - The master key of the user identity.
///
/// * `self signing key` - The self signing key of user identity.
/// * `tofu_trusted` - Is this identity tofu trusted.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're sticking with a single flag, we will need to explain what "tofu trusted" actually means each time we reference it.

///
/// Returns a `SignatureError` if the self signing key fails to be correctly
/// verified by the given master key.
pub(crate) fn new(
master_key: MasterPubkey,
self_signing_key: SelfSigningPubkey,
tofu_trusted: bool,
) -> Result<Self, SignatureError> {
master_key.verify_subkey(&self_signing_key)?;

Ok(Self {
user_id: master_key.user_id().into(),
master_key: master_key.into(),
self_signing_key: self_signing_key.into(),
tofu_trusted: AtomicBool::new(tofu_trusted).into(),
})
}

Expand All @@ -429,7 +451,12 @@ impl ReadOnlyUserIdentity {
let self_signing_key =
identity.self_signing_key.lock().await.as_ref().unwrap().public_key().clone().into();

Self { user_id: identity.user_id().into(), master_key, self_signing_key }
Self {
user_id: identity.user_id().into(),
master_key,
self_signing_key,
tofu_trusted: AtomicBool::new(true).into(),
}
}

/// Get the user id of this identity.
Expand All @@ -447,6 +474,21 @@ impl ReadOnlyUserIdentity {
&self.self_signing_key
}

/// Check if our identity is verified.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This certainly doesn't mean that it is "verified", which would imply we have done the cross-signing dance.

pub fn is_tofu_trusted(&self) -> bool {
self.tofu_trusted.load(Ordering::SeqCst)
}

/// Mark this identity as tofu trusted by the user
pub fn mark_as_tofu_trusted(&self) {
self.tofu_trusted.store(true, Ordering::SeqCst)
}

#[cfg(test)]
pub fn mark_as_not_tofu_trusted(&self) {
self.tofu_trusted.store(false, Ordering::SeqCst)
}

/// Update the identity with a new master key and self signing key.
///
/// # Arguments
Expand All @@ -465,7 +507,7 @@ impl ReadOnlyUserIdentity {
) -> Result<bool, SignatureError> {
master_key.verify_subkey(&self_signing_key)?;

let new = Self::new(master_key, self_signing_key)?;
let new = Self::new(master_key, self_signing_key, false)?;
let changed = new != *self;

*self = new;
Expand Down Expand Up @@ -776,8 +818,12 @@ pub(crate) mod testing {
let self_signing: CrossSigningKey =
response.self_signing_keys.get(user_id).unwrap().deserialize_as().unwrap();

ReadOnlyUserIdentity::new(master_key.try_into().unwrap(), self_signing.try_into().unwrap())
.unwrap()
ReadOnlyUserIdentity::new(
master_key.try_into().unwrap(),
self_signing.try_into().unwrap(),
true,
)
.unwrap()
}
}

Expand Down
Loading