fix(crypto): upload device keys *before* sending the keys signature during XSigning bootstrapping

There was a bug previously, that the cross-signing bootstrapping could include a signature for a device key that hadn't been uploaded yet.
This fixes it by returning a third (!) request in `bootstrap_cross_signing`, that must be sent as the second in order, and will upload
keys, if required.

Also tweaks documentation. Fixes #2749.

bindings/matrix-sdk-crypto-ffi/src/responses.rs

@@ -4,7 +4,7 @@ use std::collections::HashMap;
 
 use http::Response;
 use matrix_sdk_crypto::{
-    IncomingResponse, KeysBackupRequest, OutgoingRequest,
+    CrossSigningBootstrapRequests, IncomingResponse, KeysBackupRequest, OutgoingRequest,
     OutgoingVerificationRequest as SdkVerificationRequest, RoomMessageRequest, ToDeviceRequest,
     UploadSigningKeysRequest as RustUploadSigningKeysRequest,
 };
@@ -71,17 +71,23 @@ impl From<RustUploadSigningKeysRequest> for UploadSigningKeysRequest {
 
 #[derive(uniffi::Record)]
 pub struct BootstrapCrossSigningResult {
+    /// Optional request to upload some device keys alongside.
+    ///
+    /// Must be sent first if present, and marked with `mark_request_as_sent`.
+    pub upload_keys_request: Option<Request>,
+    /// Request to upload the signing keys. Must be sent second.
     pub upload_signing_keys_request: UploadSigningKeysRequest,
+    /// Request to upload the keys signatures, including for the signing keys
+    /// and optionally for the device keys. Must be sent last.
     pub upload_signature_request: SignatureUploadRequest,
 }
 
-impl From<(RustUploadSigningKeysRequest, RustSignatureUploadRequest)>
-    for BootstrapCrossSigningResult
-{
-    fn from(requests: (RustUploadSigningKeysRequest, RustSignatureUploadRequest)) -> Self {
+impl From<CrossSigningBootstrapRequests> for BootstrapCrossSigningResult {
+    fn from(requests: CrossSigningBootstrapRequests) -> Self {
         Self {
-            upload_signing_keys_request: requests.0.into(),
-            upload_signature_request: requests.1.into(),
+            upload_signing_keys_request: requests.upload_signing_keys_req.into(),
+            upload_keys_request: requests.upload_keys_req.map(Request::from),
+            upload_signature_request: requests.upload_signatures_req.into(),
         }
     }
 }

crates/matrix-sdk-crypto/CHANGELOG.md

@@ -73,3 +73,6 @@
 
 - Stop logging large quantities of data about the `Store` during olm
   decryption.
+
+- Change the return value of `bootstrap_cross_signing` so it returns an extra keys upload request.
+  The three requests must be sent in the order they appear in the return tuple.

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

@@ -81,7 +81,7 @@ pub use identities::{
     Device, LocalTrust, OwnUserIdentity, ReadOnlyDevice, ReadOnlyOwnUserIdentity,
     ReadOnlyUserIdentities, ReadOnlyUserIdentity, UserDevices, UserIdentities, UserIdentity,
 };
-pub use machine::{EncryptionSyncChanges, OlmMachine};
+pub use machine::{CrossSigningBootstrapRequests, EncryptionSyncChanges, OlmMachine};
 #[cfg(feature = "qrcode")]
 pub use matrix_sdk_qrcode;
 pub use olm::{Account, CrossSigningStatus, EncryptionSettings, Session};

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

@@ -29,7 +29,7 @@ use ruma::{
         keys::{
             claim_keys::v3::{Request as KeysClaimRequest, Response as KeysClaimResponse},
             get_keys::v3::Response as KeysQueryResponse,
-            upload_keys,
+            upload_keys::v3::{Request as UploadKeysRequest, Response as UploadKeysResponse},
             upload_signatures::v3::Request as UploadSignaturesRequest,
         },
         sync::sync_events::DeviceLists,
@@ -375,7 +375,7 @@ impl OlmMachine {
         {
             let store_cache = self.inner.store.cache().await?;
             let account = store_cache.account().await?;
-            if let Some(r) = self.keys_for_upload(&*account).await.map(|r| OutgoingRequest {
+            if let Some(r) = self.keys_for_upload(&account).await.map(|r| OutgoingRequest {
                 request_id: TransactionId::new(),
                 request: Arc::new(r.into()),
             }) {
@@ -482,15 +482,17 @@ impl OlmMachine {
     /// Create a new cross signing identity and get the upload request to push
     /// the new public keys to the server.
     ///
-    /// **Warning**: This will delete any existing cross signing keys that might
-    /// exist on the server and thus will reset the trust between all the
-    /// devices.
+    /// **Warning**: if called with `reset`, this will delete any existing cross
+    /// signing keys that might exist on the server and thus will reset the
+    /// trust between all the devices.
     ///
     /// # Returns
     ///
-    /// A pair of requests which should be sent out to the server. Once
-    /// sent, the responses should be passed back to the state machine using
-    /// [`mark_request_as_sent`].
+    /// A triple of requests which should be sent out to the server, in the
+    /// order they appear in the return tuple.
+    ///
+    /// The first request's response, if present, should be passed back to the
+    /// state machine using [`mark_request_as_sent`].
     ///
     /// These requests may require user interactive auth.
     ///
@@ -498,15 +500,18 @@ impl OlmMachine {
     pub async fn bootstrap_cross_signing(
         &self,
         reset: bool,
-    ) -> StoreResult<(UploadSigningKeysRequest, UploadSignaturesRequest)> {
+    ) -> StoreResult<CrossSigningBootstrapRequests> {
         let mut identity = self.inner.user_identity.lock().await;
 
-        if reset || identity.is_empty().await {
+        let (upload_signing_keys_req, upload_signatures_req) = if reset || identity.is_empty().await
+        {
             info!("Creating new cross signing identity");
-            let cache = self.inner.store.cache().await?;
-            let account = cache.account().await?;
-            let (new_identity, upload_signing_keys_req, upload_signatures_req) =
-                account.bootstrap_cross_signing().await;
+
+            let (new_identity, upload_signing_keys_req, upload_signatures_req) = {
+                let cache = self.inner.store.cache().await?;
+                let account = cache.account().await?;
+                account.bootstrap_cross_signing().await
+            };
 
             *identity = new_identity;
 
@@ -514,24 +519,46 @@ impl OlmMachine {
                 "Couldn't create a public version of the identity from a new private identity",
             );
 
-            let changes = Changes {
-                identities: IdentityChanges { new: vec![public.into()], ..Default::default() },
-                private_identity: Some(identity.clone()),
-                ..Default::default()
-            };
+            self.store()
+                .save_changes(Changes {
+                    identities: IdentityChanges { new: vec![public.into()], ..Default::default() },
+                    private_identity: Some(identity.clone()),
+                    ..Default::default()
+                })
+                .await?;
 
-            self.store().save_changes(changes).await?;
-
-            Ok((upload_signing_keys_req, upload_signatures_req))
+            (upload_signing_keys_req, upload_signatures_req)
         } else {
             info!("Trying to upload the existing cross signing identity");
             let upload_signing_keys_req = identity.as_upload_request().await;
-            let account = self.inner.store.static_account();
+
             // TODO remove this expect.
-            let upload_signatures_req =
-                identity.sign_account(account).await.expect("Can't sign device keys");
-            Ok((upload_signing_keys_req, upload_signatures_req))
-        }
+            let upload_signatures_req = identity
+                .sign_account(self.inner.store.static_account())
+                .await
+                .expect("Can't sign device keys");
+
+            (upload_signing_keys_req, upload_signatures_req)
+        };
+
+        // If there are any *device* keys to upload (i.e. the account isn't shared),
+        // upload them before we upload the signatures, since the signatures may
+        // reference keys to be uploaded.
+        let upload_keys_req = {
+            let cache = self.store().cache().await?;
+            let account = cache.account().await?;
+            if account.shared() {
+                None
+            } else {
+                self.keys_for_upload(&account).await.map(OutgoingRequest::from)
+            }
+        };
+
+        Ok(CrossSigningBootstrapRequests {
+            upload_signing_keys_req,
+            upload_keys_req,
+            upload_signatures_req,
+        })
     }
 
     /// Receive a successful keys upload response.
@@ -540,10 +567,7 @@ impl OlmMachine {
     ///
     /// * `response` - The keys upload response of the request that the client
     /// performed.
-    async fn receive_keys_upload_response(
-        &self,
-        response: &upload_keys::v3::Response,
-    ) -> OlmResult<()> {
+    async fn receive_keys_upload_response(&self, response: &UploadKeysResponse) -> OlmResult<()> {
         self.inner
             .store
             .with_transaction(|mut tr| async {
@@ -623,7 +647,7 @@ impl OlmMachine {
     /// the [`OlmMachine`] with the [`receive_keys_upload_response`].
     ///
     /// [`receive_keys_upload_response`]: #method.receive_keys_upload_response
-    async fn keys_for_upload(&self, account: &Account) -> Option<upload_keys::v3::Request> {
+    async fn keys_for_upload(&self, account: &Account) -> Option<UploadKeysRequest> {
         let (device_keys, one_time_keys, fallback_keys) = account.keys_for_upload();
 
         if device_keys.is_none() && one_time_keys.is_empty() && fallback_keys.is_empty() {
@@ -631,7 +655,7 @@ impl OlmMachine {
         } else {
             let device_keys = device_keys.map(|d| d.to_raw());
 
-            Some(assign!(upload_keys::v3::Request::new(), {
+            Some(assign!(UploadKeysRequest::new(), {
                 device_keys, one_time_keys, fallback_keys
             }))
         }
@@ -2048,6 +2072,30 @@ impl OlmMachine {
     }
 }
 
+/// A set of requests to be executed when bootstrapping cross-signing using
+/// [`OlmMachine::bootstrap_cross_signing`].
+#[derive(Debug)]
+pub struct CrossSigningBootstrapRequests {
+    /// An optional request to upload a device key.
+    ///
+    /// Should be sent first, if present.
+    ///
+    /// If present, its result must be processed back with
+    /// `OlmMachine::mark_request_as_sent`.
+    pub upload_keys_req: Option<OutgoingRequest>,
+
+    /// Request to upload the cross-signing keys.
+    ///
+    /// Should be sent second.
+    pub upload_signing_keys_req: UploadSigningKeysRequest,
+
+    /// Request to upload key signatures, including those for the cross-signing
+    /// keys, and maybe some for the optional uploaded key too.
+    ///
+    /// Should be sent last.
+    pub upload_signatures_req: UploadSignaturesRequest,
+}
+
 /// Data contained from a sync response and that needs to be processed by the
 /// OlmMachine.
 #[derive(Debug)]
@@ -2122,7 +2170,7 @@ pub(crate) mod tests {
         Curve25519PublicKey, Ed25519PublicKey,
     };
 
-    use super::testing::response_from_file;
+    use super::{testing::response_from_file, CrossSigningBootstrapRequests};
     use crate::{
         error::EventError,
         machine::{EncryptionSyncChanges, OlmMachine},
@@ -3186,10 +3234,10 @@ pub(crate) mod tests {
     }
 
     async fn setup_cross_signing_for_machine_test_helper(alice: &OlmMachine, bob: &OlmMachine) {
-        let (alice_upload_signing, _) =
+        let CrossSigningBootstrapRequests { upload_signing_keys_req: alice_upload_signing, .. } =
             alice.bootstrap_cross_signing(false).await.expect("Expect Alice x-signing key request");
 
-        let (bob_upload_signing, _) =
+        let CrossSigningBootstrapRequests { upload_signing_keys_req: bob_upload_signing, .. } =
             bob.bootstrap_cross_signing(false).await.expect("Expect Bob x-signing key request");
 
         let bob_device_keys = bob
@@ -3239,8 +3287,11 @@ pub(crate) mod tests {
     }
 
     async fn sign_alice_device_for_machine_test_helper(alice: &OlmMachine, bob: &OlmMachine) {
-        let (upload_signing, upload_signature) =
-            alice.bootstrap_cross_signing(false).await.expect("Expect Alice x-signing key request");
+        let CrossSigningBootstrapRequests {
+            upload_signing_keys_req: upload_signing,
+            upload_signatures_req: upload_signature,
+            ..
+        } = alice.bootstrap_cross_signing(false).await.expect("Expect Alice x-signing key request");
 
         let mut device_keys = alice
             .get_device(alice.user_id(), alice.device_id(), None)
@@ -3320,7 +3371,7 @@ pub(crate) mod tests {
             .bootstrap_cross_signing(false)
             .await
             .expect("Expect Alice x-signing key request")
-            .0
+            .upload_signing_keys_req
             .user_signing_key
             .unwrap()
             .get_first_key_and_id()
@@ -3342,7 +3393,7 @@ pub(crate) mod tests {
             .bootstrap_cross_signing(false)
             .await
             .expect("Expect Alice x-signing key request")
-            .0;
+            .upload_signing_keys_req;
 
         let json = json!({
             "device_keys": {

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

@@ -283,6 +283,12 @@ impl From<SignatureUploadRequest> for OutgoingRequest {
     }
 }
 
+impl From<KeysUploadRequest> for OutgoingRequest {
+    fn from(r: KeysUploadRequest) -> Self {
+        Self { request_id: TransactionId::new(), request: Arc::new(r.into()) }
+    }
+}
+
 /// Enum over all the incoming responses we need to receive.
 #[derive(Debug)]
 pub enum IncomingResponse<'a> {

crates/matrix-sdk/src/encryption/mod.rs

@@ -29,7 +29,9 @@ use futures_util::{
     future::try_join,
     stream::{self, StreamExt},
 };
-use matrix_sdk_base::crypto::{OlmMachine, OutgoingRequest, RoomMessageRequest, ToDeviceRequest};
+use matrix_sdk_base::crypto::{
+    CrossSigningBootstrapRequests, OlmMachine, OutgoingRequest, RoomMessageRequest, ToDeviceRequest,
+};
 use ruma::{
     api::client::{
         backup::add_backup_keys::v3::Response as KeysBackupResponse,
@@ -814,8 +816,11 @@ impl Encryption {
         let olm = self.client.olm_machine().await;
         let olm = olm.as_ref().ok_or(Error::NoOlmMachine)?;
 
-        let (upload_signing_keys_req, upload_signatures_req) =
-            olm.bootstrap_cross_signing(false).await?;
+        let CrossSigningBootstrapRequests {
+            upload_signing_keys_req,
+            upload_keys_req,
+            upload_signatures_req,
+        } = olm.bootstrap_cross_signing(false).await?;
 
         let upload_signing_keys_req = assign!(UploadSigningKeysRequest::new(), {
             auth: auth_data,
@@ -824,6 +829,9 @@ impl Encryption {
             user_signing_key: upload_signing_keys_req.user_signing_key.map(|c| c.to_raw()),
         });
 
+        if let Some(req) = upload_keys_req {
+            self.client.send_outgoing_request(req).await?;
+        }
         self.client.send(upload_signing_keys_req, None).await?;
         self.client.send(upload_signatures_req, None).await?;
 

crates/matrix-sdk/tests/integration/client.rs

@@ -547,6 +547,14 @@ async fn cross_signing_status() {
         .mount(&server)
         .await;
 
+    Mock::given(method("POST"))
+        .and(path("/_matrix/client/r0/keys/upload"))
+        .respond_with(ResponseTemplate::new(200).set_body_json(json!({
+            "one_time_key_counts": {}
+        })))
+        .mount(&server)
+        .await;
+
     let status = client
         .encryption()
         .cross_signing_status()

crates/matrix-sdk/tests/integration/encryption/verification.rs

@@ -266,27 +266,26 @@ fn mock_keys_signature_upload(keys: Arc<Mutex<Keys>>) -> impl Fn(&Request) -> Re
                 // Either merge signatures if an entry is already present, or insert a new
                 // entry.
                 let known_devices = keys.device.entry(user.clone()).or_default();
-                if let Some(device_keys) = known_devices.get_mut(key_id) {
-                    let param: DeviceKeys = serde_json::from_str(raw_key.get()).unwrap();
+                let device_keys = known_devices
+                    .get_mut(key_id)
+                    .expect("trying to add a signature for a missing key");
 
-                    let mut existing: DeviceKeys = device_keys.deserialize().unwrap();
+                let param: DeviceKeys = serde_json::from_str(raw_key.get()).unwrap();
 
-                    for (uid, sigs) in existing.signatures.iter_mut() {
-                        if let Some(new_sigs) = param.signatures.get(uid) {
-                            sigs.extend(new_sigs.clone());
-                        }
+                let mut existing: DeviceKeys = device_keys.deserialize().unwrap();
+
+                for (uid, sigs) in existing.signatures.iter_mut() {
+                    if let Some(new_sigs) = param.signatures.get(uid) {
+                        sigs.extend(new_sigs.clone());
                     }
-                    for (uid, sigs) in param.signatures.iter() {
-                        if !existing.signatures.contains_key(uid) {
-                            existing.signatures.insert(uid.clone(), sigs.clone());
-                        }
-                    }
-
-                    *device_keys = Raw::new(&existing).unwrap();
-                } else {
-                    tracing::warn!("adding a signature to a key that hasn't been uploaded yet, user={user}, key={key_id}");
-                    known_devices.insert(key_id.to_owned(), Raw::from_json(raw_key.to_owned()));
                 }
+                for (uid, sigs) in param.signatures.iter() {
+                    if !existing.signatures.contains_key(uid) {
+                        existing.signatures.insert(uid.clone(), sigs.clone());
+                    }
+                }
+
+                *device_keys = Raw::new(&existing).unwrap();
             }
         }
 

crates/matrix-sdk/tests/integration/matrix_auth.rs

@@ -341,6 +341,14 @@ async fn test_login_with_cross_signing_bootstrapping() {
         .mount(&server)
         .await;
 
+    Mock::given(method("POST"))
+        .and(path("/_matrix/client/r0/keys/upload"))
+        .respond_with(ResponseTemplate::new(200).set_body_json(json!({
+            "one_time_key_counts": {}
+        })))
+        .mount(&server)
+        .await;
+
     let num_calls = Mutex::new(0);
     Mock::given(method("POST"))
         .and(path("/_matrix/client/unstable/keys/device_signing/upload"))