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.

Sign up for GitHub

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

crypto: Extract a test helper function for simulating verification #4129

Merged
merged 1 commit into from
Oct 18, 2024
Merged

crypto: Extract a test helper function for simulating verification #4129

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
169 changes: 135 additions & 34 deletions crates/matrix-sdk-crypto/src/identities/user.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1189,7 +1189,15 @@ where
#[cfg(any(test, feature = "testing"))]
#[allow(dead_code)]
pub(crate) mod testing {
use ruma::{api::client::keys::get_keys::v3::Response as KeyQueryResponse, user_id};
use matrix_sdk_test::ruma_response_from_json;
use ruma::{
api::client::keys::{
get_keys::v3::Response as KeyQueryResponse,
upload_signatures::v3::Request as SignatureUploadRequest,
},
user_id, UserId,
};
use serde_json::json;

use super::{OtherUserIdentityData, OwnUserIdentity, OwnUserIdentityData};
#[cfg(test)]
Expand Down Expand Up @@ -1266,18 +1274,126 @@ pub(crate) mod testing {
OtherUserIdentityData::new(master_key.try_into().unwrap(), self_signing.try_into().unwrap())
.unwrap()
}

/// When we want to test identities that are verified, we need to simulate
/// the verification process. This function supports that by simulating
/// what happens when a successful verification dance happens and
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
/// what happens when a successful verification dance happens and
/// what happens when a successful verification dance happens on another device, by

/// providing the /keys/query response we would get when that happened.
///
/// signature_upload_request will be the result of calling
/// [`super::OtherUserIdentity::verify`].
///
/// # Example
///
/// ```ignore
/// let signature_upload_request = their_identity.verify().await.unwrap();
///
/// let msk_json = json!({
/// "their_user_id": {
/// "keys": { "ed25519:blah": "blah" }
/// "signatures": {
/// "their_user_id": { "ed25519:blah": "blah", ... }
/// }
/// "usage": [ "master" ],
/// "user_id": "their_user_id"
/// }
/// });
///
/// let ssk_json = json!({
/// "their_user_id": {
/// "keys": { "ed25519:blah": "blah" },
/// "signatures": {
/// "their_user_id": { "ed25519:blah": "blah" }
/// },
/// "usage": [ "self_signing" ],
/// "user_id": "their_user_id"
/// }
/// })
///
/// let response = simulate_key_query_response_for_verification(
/// signature_upload_request,
/// my_identity,
/// my_user_id,
/// their_user_id,
/// msk_json,
/// ssk_json
/// ).await;
///
/// olm_machine
/// .mark_request_as_sent(
/// &TransactionId::new(),
/// crate::IncomingResponse::KeysQuery(&kq_response),
/// )
/// .await
/// .unwrap();
/// ```
pub fn simulate_key_query_response_for_verification(
signature_upload_request: SignatureUploadRequest,
my_identity: OwnUserIdentity,
my_user_id: &UserId,
their_user_id: &UserId,
msk_json: serde_json::Value,
ssk_json: serde_json::Value,
Comment on lines +1335 to +1336
Copy link
Member

Choose a reason for hiding this comment

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

might be good to name these to indicate that they are the keys for the other user: their_msk_json, etc.

Copy link
Member

Choose a reason for hiding this comment

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

We could do with better documentation on these parameters. Maybe use a bullet list under a # Arguments section in the doc-comment.

In particular, it's important to note that msk_json and ssk_json are the other user's cross-signing keys as we would expect to see them in a /keys/query response: the MSK is self-signed, and the SSK is signed by the MSK.

) -> KeyQueryResponse {
// Find the signed key inside the SignatureUploadRequest
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
// Find the signed key inside the SignatureUploadRequest
// Extract our signature of the other user's MSK from the SignatureUploadRequest

let cross_signing_key: CrossSigningKey = serde_json::from_str(
Copy link
Member

Choose a reason for hiding this comment

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

maybe signed_cross_signing_key

signature_upload_request
.signed_keys
.get(their_user_id)
.expect("Signature upload request should contain a key for their user ID")
.iter()
.next()
.expect("There should be a key in the signature upload request")
.1
.get(),
)
.expect("Should not fail to deserialize the key");

// Find their master key that we want to update inside their msk JSON
let mut their_msk: CrossSigningKey = serde_json::from_value(
msk_json.get(their_user_id.as_str()).expect("msk should contain their user ID").clone(),
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
msk_json.get(their_user_id.as_str()).expect("msk should contain their user ID").clone(),
msk_json.get(their_user_id.as_str()).expect("msk JSON should contain their user ID").clone(),

)
.expect("Should not fail to deserialize msk");

Copy link
Member

Choose a reason for hiding this comment

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

It is important for this method to work correctly that the signed key in signature_upload_request is the same as that in msk_json. We could maybe assert as much.

something like:

        let (their_msk_id, _) = their_msk.get_first_key_and_id().expect("MSK JSON should contain at least one key");
        let (signed_msk_id, _) = signed_cross_signing_key..get_first_key_and_id().expect("signature data should contain at least one key");
        assert_eq!(signed_msk_id, their_msk_id, "MSK data did not match the key that was signed in signature_upload_request");

Copy link
Member

Choose a reason for hiding this comment

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

It is important for this method to work correctly that the signed key in signature_upload_request is the same as that in msk_json

oh also, put something about that in the docs

// Find our own user signing key
let my_user_signing_key_id = my_identity
.user_signing_key()
.keys()
.iter()
.next()
.expect("There should be a user signing key")
.0;
Comment on lines +1358 to +1365
Copy link
Member

Choose a reason for hiding this comment

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

we could probably manage without figuring out our own USK id: we could just pull out the first (and only signature) in signed_cross_signing_key. Would mean we don't need to provide my_identity, which would be a win.


// Add the signature from the SignatureUploadRequest to their master key, under
// our user ID
their_msk.signatures.add_signature(
my_user_id.to_owned(),
my_user_signing_key_id.to_owned(),
cross_signing_key
.signatures
.get_signature(my_user_id, my_user_signing_key_id)
.expect("There should be a signature for our user"),
Comment on lines +1372 to +1375
Copy link
Member

Choose a reason for hiding this comment

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

suggest moving this up to next to where we construct cross_signing_key. It's only needed for this one operation and conceptually they fit together.

);
Comment on lines +1367 to +1376
Copy link
Member

@richvdh richvdh Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Another thought on this: a much more intuitive way to generate this signature is to build it ourselves, rather than abusing OtherUserIdentity::verify (which of course requires the offending identity to have been imported into the olm machine).

Building the signature ourselves requires getting hold of our own private USK, but that's not so bad:

    async fn get_private_usk(machine: &OlmMachine) -> Ed25519SecretKey {
        Ed25519SecretKey::from_base64(
            machine
                .export_cross_signing_keys()
                .await
                .unwrap()
                .unwrap()
                .user_signing_key
                .as_ref()
                .unwrap(),
        )
        .unwrap()
    }

And then signing the MSK is something like

        use crate::olm::utility::SignJson;
        let signature = my_usk.sign_json(serde_json::to_value(&msk_to_update).unwrap()).unwrap(),
        let my_user_signing_key_id = DeviceKeyId::from_parts(
                DeviceKeyAlgorithm::Ed25519,
                my_usk.public_key().to_base64().as_str().into(),
        );

        msk_to_update.signatures.add_signature(
            my_user_id.to_owned(),
            my_user_signing_key_id,
            signature
        );

The problem with that is that crate::olm::utility::SignJson is pub(crate), but maybe we could add a #[cfg(test)]?

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem with that is that crate::olm::utility::SignJson is pub(crate), but maybe we could add a #[cfg(test)]?

Why is that a problem this PR is completely inside the crypto crate?

Copy link
Member

Choose a reason for hiding this comment

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

Why is that a problem this PR is completely inside the crypto crate?

Sorry, I'm confused. It's private, not pub(crate).

utility::{SignedJsonObject, VerifyJson} are exported from olm at pub(crate); maybe SignJson should be too.

Copy link
Member Author

Choose a reason for hiding this comment

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

This seems like a good plan but I don't have time to work on it now. Shall we make an issue for it, or just leave the info here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I'm confused. It's private, not pub(crate).

I think I left that private for a reason and implemented higher level methods like sign_device() on things that can sign devices. It is supposed to encode in our type system which things are allowed to sign which objects.

Exposing the general signing mechanism and re-implementing sign_user() in a test helper doesn't sound that great to me. Refactoring sign_user() into two methods, one which returns a request, and another one which returns a CrossSigningKey sounds better to me.

Copy link
Member

Choose a reason for hiding this comment

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

This seems like a good plan but I don't have time to work on it now. Shall we make an issue for it, or just leave the info here?

I can't see that an issue is likely to be very useful. It will just clutter things up and we won't remember to look for it when we come to do the work.

Copy link
Member

Choose a reason for hiding this comment

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

I think I left that private for a reason and implemented higher level methods like sign_device() on things that can sign devices. It is supposed to encode in our type system which things are allowed to sign which objects.

Exposing the general signing mechanism and re-implementing sign_user() in a test helper doesn't sound that great to me. Refactoring sign_user() into two methods, one which returns a request, and another one which returns a CrossSigningKey sounds better to me.

I don't really agree with this: I think that having to construct the higher-level objects makes testing unnecessarily difficult, and makes tests fragile, complicated, and generally hard to maintain.

Still, if nobody has time to do the work on this, the discussion seems to be moot.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really agree with this: I think that having to construct the higher-level objects makes testing unnecessarily difficult, and makes tests fragile, complicated, and generally hard to maintain.

That's a fair argument, but the example Rust code you posted requires the same higher-level objects to be created as my suggestion would.


// Create a JSON response as if the verification has happened
ruma_response_from_json(&json!({
"device_keys": {}, // Don't need devices here, even though they would exist
"failures": {},
"master_keys": {
their_user_id: their_msk,
},
"self_signing_keys": ssk_json,
}))
}
}

#[cfg(test)]
pub(crate) mod tests {
use std::{collections::HashMap, sync::Arc};

use assert_matches::assert_matches;
use matrix_sdk_test::{async_test, ruma_response_from_json, test_json};
use ruma::{
api::client::keys::get_keys::v3::Response as KeyQueryResponse, device_id, user_id,
TransactionId,
};
use matrix_sdk_test::{async_test, test_json};
use ruma::{device_id, user_id, TransactionId};
use serde_json::{json, Value};
use tokio::sync::Mutex;

Expand All @@ -1288,7 +1404,12 @@ pub(crate) mod tests {
};
use crate::{
identities::{
manager::testing::own_key_query, user::OtherUserIdentityDataSerializer, Device,
manager::testing::own_key_query,
user::{
testing::simulate_key_query_response_for_verification,
OtherUserIdentityDataSerializer,
},
Device,
},
olm::{Account, PrivateCrossSigningIdentity},
store::{CryptoStoreWrapper, MemoryStore},
Expand Down Expand Up @@ -1610,7 +1731,6 @@ pub(crate) mod tests {
machine.bootstrap_cross_signing(false).await.unwrap();

let my_id = machine.get_identity(my_user_id, None).await.unwrap().unwrap().own().unwrap();
let usk_key_id = my_id.inner.user_signing_key().keys().iter().next().unwrap().0;

let keys_query = DataSet::key_query_with_identity_a();
let txn_id = TransactionId::new();
Expand All @@ -1632,33 +1752,14 @@ pub(crate) mod tests {
// Manually verify for the purpose of this test
let sig_upload = other_identity.verify().await.unwrap();

let raw_extracted =
sig_upload.signed_keys.get(other_user_id).unwrap().iter().next().unwrap().1.get();

let new_signature: CrossSigningKey = serde_json::from_str(raw_extracted).unwrap();

let mut msk_to_update: CrossSigningKey =
serde_json::from_value(DataSet::msk_b().get("@bob:localhost").unwrap().clone())
.unwrap();

msk_to_update.signatures.add_signature(
my_user_id.to_owned(),
usk_key_id.to_owned(),
new_signature.signatures.get_signature(my_user_id, usk_key_id).unwrap(),
let kq_response = simulate_key_query_response_for_verification(
sig_upload,
my_id,
my_user_id,
other_user_id,
DataSet::msk_b(),
DataSet::ssk_b(),
);

// we want to update bob device keys with the new signature
let data = json!({
"device_keys": {}, // For the purpose of this test we don't need devices here
"failures": {},
"master_keys": {
DataSet::user_id(): msk_to_update
,
},
"self_signing_keys": DataSet::ssk_b(),
});

let kq_response: KeyQueryResponse = ruma_response_from_json(&data);
machine.mark_request_as_sent(&TransactionId::new(), &kq_response).await.unwrap();

// The identity should not need any user approval now
Expand Down
4 changes: 3 additions & 1 deletion crates/matrix-sdk-crypto/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,9 @@ mod verification;
pub mod testing {
pub use crate::identities::{
device::testing::get_device,
user::testing::{get_other_identity, get_own_identity},
user::testing::{
get_other_identity, get_own_identity, simulate_key_query_response_for_verification,
},
};
}

Expand Down
57 changes: 25 additions & 32 deletions crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -527,13 +527,15 @@ mod tests {
use ruma::{
device_id, events::room::history_visibility::HistoryVisibility, room_id, TransactionId,
};
use serde_json::json;

use crate::{
error::SessionRecipientCollectionError,
olm::OutboundGroupSession,
session_manager::{
group_sessions::share_strategy::collect_session_recipients, CollectStrategy,
},
testing::simulate_key_query_response_for_verification,
types::events::room_key_withheld::WithheldCode,
CrossSigningKeyExport, EncryptionSettings, LocalTrust, OlmError, OlmMachine,
};
Expand Down Expand Up @@ -1337,41 +1339,32 @@ mod tests {
.verify()
.await
.unwrap();
let raw_extracted =
verification_request.signed_keys.get(user2).unwrap().iter().next().unwrap().1.get();
let signed_key: crate::types::CrossSigningKey =
serde_json::from_str(raw_extracted).unwrap();
let new_signatures = signed_key.signatures.get(KeyDistributionTestData::me_id()).unwrap();
let mut master_key = machine
.get_identity(user2, None)

let master_key =
&machine.get_identity(user2, None).await.unwrap().unwrap().other().unwrap().master_key;

let my_identity = machine
.get_identity(KeyDistributionTestData::me_id(), None)
.await
.unwrap()
.unwrap()
.other()
.unwrap()
.master_key
.as_ref()
.clone();

for (key_id, signature) in new_signatures.iter() {
master_key.as_mut().signatures.add_signature(
KeyDistributionTestData::me_id().to_owned(),
key_id.to_owned(),
signature.as_ref().unwrap().ed25519().unwrap(),
);
}
let json = serde_json::json!({
"device_keys": {},
"failures": {},
"master_keys": {
user2: master_key,
},
"user_signing_keys": {},
"self_signing_keys": MaloIdentityChangeDataSet::updated_key_query().self_signing_keys,
}
.expect("Should not fail to find own identity")
.expect("Our own identity should not be missing")
.own()
.expect("Our own identity should be of type Own");

let msk = json!({ user2: serde_json::to_value(master_key).expect("Should not fail to serialize")});
let ssk =
serde_json::to_value(&MaloIdentityChangeDataSet::updated_key_query().self_signing_keys)
.expect("Should not fail to serialize");

let kq_response = simulate_key_query_response_for_verification(
verification_request,
my_identity,
KeyDistributionTestData::me_id(),
user2,
msk,
ssk,
);

let kq_response = matrix_sdk_test::ruma_response_from_json(&json);
machine
.mark_request_as_sent(
&TransactionId::new(),
Expand Down
Loading