crypto: Extract a test helper function for simulating verification #4129
Conversation
andybalaam
requested review from
poljar and
richvdh
and removed request for
a team
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4129 +/- ##
==========================================
+ Coverage 84.84% 84.87% +0.03%
==========================================
Files 269 269
Lines 28793 28798 +5
==========================================
+ Hits 24429 24442 +13
+ Misses 4364 4356 -8 ☔ View full report in Codecov by Sentry. |
andybalaam
force-pushed
the
andybalaam/extract-simulate_key_query_response_for_verification
branch
from
6ad623f
to
a9d4ac2
Compare
andybalaam
force-pushed
the
andybalaam/extract-simulate_key_query_response_for_verification
branch
from
a9d4ac2
to
67d147b
Compare
andybalaam
deleted the
andybalaam/extract-simulate_key_query_response_for_verification
branch
| msk_json: serde_json::Value, | ||
| ssk_json: serde_json::Value, |
There was a problem hiding this comment.
might be good to name these to indicate that they are the keys for the other user: their_msk_json, etc.
There was a problem hiding this comment.
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.
|
|
||
| /// 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 |
There was a problem hiding this comment.
| /// what happens when a successful verification dance happens and | |
| /// what happens when a successful verification dance happens on another device, by |
| cross_signing_key | ||
| .signatures | ||
| .get_signature(my_user_id, my_user_signing_key_id) | ||
| .expect("There should be a signature for our user"), |
There was a problem hiding this comment.
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.
|
|
||
| // 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(), |
There was a problem hiding this comment.
| 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(), |
| ssk_json: serde_json::Value, | ||
| ) -> KeyQueryResponse { | ||
| // Find the signed key inside the SignatureUploadRequest | ||
| let cross_signing_key: CrossSigningKey = serde_json::from_str( |
| msk_json.get(their_user_id.as_str()).expect("msk should contain their user ID").clone(), | ||
| ) | ||
| .expect("Should not fail to deserialize msk"); | ||
|
|
There was a problem hiding this comment.
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");There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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.
| msk_json: serde_json::Value, | ||
| ssk_json: serde_json::Value, | ||
| ) -> KeyQueryResponse { | ||
| // Find the signed key inside the SignatureUploadRequest |
There was a problem hiding this comment.
| // Find the signed key inside the SignatureUploadRequest | |
| // Extract our signature of the other user's MSK from the SignatureUploadRequest |
| // 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"), | ||
| ); |
There was a problem hiding this comment.
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)]?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. Refactoringsign_user()into two methods, one which returns a request, and another one which returns aCrossSigningKeysounds 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.
I'd really like some help on how to name the function, the internal variables and what the comments should say, please!