Enable key upload to backups where we have the decryption key (matrix-org#4677)

ajbura · richvdh · web-flow · commit a1a0463229f6 · 2025-02-14T14:32:33.000Z
* disable key backup when both trust via signatures and private key fail

* test for enabling backup with decryption key

* enable backup with decryption key in legacy crypto

* fix formmating

* fix typo

* add local variable for backup trust in legacy crypto

* Update spec/integ/crypto/megolm-backup.spec.ts

Co-authored-by: Richard van der Hoff &lt;1389908+richvdh@users.noreply.github.com&gt;

* Update spec/integ/crypto/megolm-backup.spec.ts

Co-authored-by: Richard van der Hoff &lt;1389908+richvdh@users.noreply.github.com&gt;

* Update spec/integ/crypto/megolm-backup.spec.ts

Co-authored-by: Richard van der Hoff &lt;1389908+richvdh@users.noreply.github.com&gt;

* Update src/rust-crypto/backup.ts

Co-authored-by: Richard van der Hoff &lt;1389908+richvdh@users.noreply.github.com&gt;

* fix white space formatting

* remove redundant test

* fix trust check while receiving backup secret

* mock room key version request before storing backup key

* fix decryption key gossip test for untrusted backup info

* rename version to latestBackupVersion to match the doc comments

* Update src/rust-crypto/backup.ts

Co-authored-by: Richard van der Hoff &lt;1389908+richvdh@users.noreply.github.com&gt;

* remove test to stop key gossip when signature mismatch

* remove misleading checkKeyBackupAndEnable doc return comment

* Update src/rust-crypto/backup.ts

Co-authored-by: Richard van der Hoff &lt;1389908+richvdh@users.noreply.github.com&gt;

* use requestKeyBackupVersion to get latest version instead of checkKeyBackupAndEnable

* remove comment

* test for backup key gossip when no backup found

* test for backup key gossip when backup request error

* fix lint error

* fix test message typo

Co-authored-by: Richard van der Hoff &lt;1389908+richvdh@users.noreply.github.com&gt;

* refactor repeated test logic into a single reusable function

* improve exceptBackup param and docs

* fix: expect private key inside test

* fix linting

* add return type for backup key retrieve function

Co-authored-by: Richard van der Hoff &lt;1389908+richvdh@users.noreply.github.com&gt;

* improve doc for retrieveBackupPrivateKeyWithDelay

Co-authored-by: Richard van der Hoff &lt;1389908+richvdh@users.noreply.github.com&gt;

* improve expectBackup param description

Co-authored-by: Richard van der Hoff &lt;1389908+richvdh@users.noreply.github.com&gt;

* fix status code and formatting

---------

Co-authored-by: Richard van der Hoff &lt;1389908+richvdh@users.noreply.github.com&gt;
diff --git a/spec/integ/crypto/megolm-backup.spec.ts b/spec/integ/crypto/megolm-backup.spec.ts
@@ -945,7 +945,29 @@ describe("megolm-keys backup", () => {
             expect(await aliceCrypto.getActiveSessionBackupVersion()).toEqual(testData.SIGNED_BACKUP_DATA.version);
         });
 
-        it("does not enable a backup signed by an untrusted device", async () => {
+        it("enables a backup not signed by a trusted device, when we have the decryption key", async () => {
+            aliceClient = await initTestClient();
+            const aliceCrypto = aliceClient.getCrypto()!;
+
+            // download the device list, to match the trusted-device case
+            await aliceClient.startClient();
+            await waitForDeviceList();
+
+            fetchMock.get("path:/_matrix/client/v3/room_keys/version", testData.SIGNED_BACKUP_DATA);
+
+            // Alice does *not* trust the device that signed the backup, but *does* have the decryption key.
+            await aliceCrypto.storeSessionBackupPrivateKey(
+                Buffer.from(testData.BACKUP_DECRYPTION_KEY_BASE64, "base64"),
+                testData.SIGNED_BACKUP_DATA.version!,
+            );
+
+            const result = await aliceCrypto.checkKeyBackupAndEnable();
+            expect(result).toBeTruthy();
+            expect(result!.trustInfo).toEqual({ trusted: false, matchesDecryptionKey: true });
+            expect(await aliceCrypto.getActiveSessionBackupVersion()).toEqual(testData.SIGNED_BACKUP_DATA.version);
+        });
+
+        it("does not enable a backup signed by an untrusted device when we do not have the decryption key", async () => {
             aliceClient = await initTestClient();
             const aliceCrypto = aliceClient.getCrypto()!;
 
diff --git a/spec/integ/crypto/verification.spec.ts b/spec/integ/crypto/verification.spec.ts
@@ -30,6 +30,7 @@ import {
     type ICreateClientOpts,
     type IEvent,
     type MatrixClient,
+    MatrixError,
     MatrixEvent,
     MatrixEventEvent,
 } from "../../../src";
@@ -1264,48 +1265,53 @@ describe("verification", () => {
             expect(encodeBase64(cachedKey!)).toEqual(BACKUP_DECRYPTION_KEY_BASE64);
         });
 
-        it("Should not accept the backup decryption key gossip if private key do not match", async () => {
+        it("Should not accept the backup decryption key gossip when there is no server-side key backup", async () => {
             const requestPromises = mockSecretRequestAndGetPromises();
 
             await doInteractiveVerification();
 
             const requestId = await requestPromises.get("m.megolm_backup.v1");
 
-            await sendBackupGossipAndExpectVersion(requestId!, BACKUP_DECRYPTION_KEY_BASE64, nonMatchingBackupInfo);
-
-            // We are lacking a way to signal that the secret has been received, so we wait a bit..
-            jest.useRealTimers();
-            await new Promise((resolve) => {
-                setTimeout(resolve, 500);
-            });
-            jest.useFakeTimers({ doNotFake: ["queueMicrotask"] });
+            await sendBackupGossipAndExpectVersion(
+                requestId!,
+                BACKUP_DECRYPTION_KEY_BASE64,
+                new MatrixError({ errcode: "M_NOT_FOUND", error: "No backup found" }, 404),
+            );
 
             // the backup secret should not be cached
-            const cachedKey = await aliceClient.getCrypto()!.getSessionBackupPrivateKey();
+            const cachedKey = await retrieveBackupPrivateKeyWithDelay();
             expect(cachedKey).toBeNull();
         });
 
-        it("Should not accept the backup decryption key gossip if backup not trusted", async () => {
+        it("Should not accept the backup decryption key gossip when server-side key backup request errors", async () => {
             const requestPromises = mockSecretRequestAndGetPromises();
 
             await doInteractiveVerification();
 
             const requestId = await requestPromises.get("m.megolm_backup.v1");
 
-            const infoCopy = Object.assign({}, matchingBackupInfo);
-            delete infoCopy.auth_data.signatures;
+            await sendBackupGossipAndExpectVersion(
+                requestId!,
+                BACKUP_DECRYPTION_KEY_BASE64,
+                new Error("Network Error!"),
+            );
 
-            await sendBackupGossipAndExpectVersion(requestId!, BACKUP_DECRYPTION_KEY_BASE64, infoCopy);
+            // the backup secret should not be cached
+            const cachedKey = await retrieveBackupPrivateKeyWithDelay();
+            expect(cachedKey).toBeNull();
+        });
 
-            // We are lacking a way to signal that the secret has been received, so we wait a bit..
-            jest.useRealTimers();
-            await new Promise((resolve) => {
-                setTimeout(resolve, 500);
-            });
-            jest.useFakeTimers({ doNotFake: ["queueMicrotask"] });
+        it("Should not accept the backup decryption key gossip if private key do not match", async () => {
+            const requestPromises = mockSecretRequestAndGetPromises();
+
+            await doInteractiveVerification();
+
+            const requestId = await requestPromises.get("m.megolm_backup.v1");
+
+            await sendBackupGossipAndExpectVersion(requestId!, BACKUP_DECRYPTION_KEY_BASE64, nonMatchingBackupInfo);
 
             // the backup secret should not be cached
-            const cachedKey = await aliceClient.getCrypto()!.getSessionBackupPrivateKey();
+            const cachedKey = await retrieveBackupPrivateKeyWithDelay();
             expect(cachedKey).toBeNull();
         });
 
@@ -1322,15 +1328,8 @@ describe("verification", () => {
                 unknownAlgorithmBackupInfo,
             );
 
-            // We are lacking a way to signal that the secret has been received, so we wait a bit..
-            jest.useRealTimers();
-            await new Promise((resolve) => {
-                setTimeout(resolve, 500);
-            });
-            jest.useFakeTimers({ doNotFake: ["queueMicrotask"] });
-
             // the backup secret should not be cached
-            const cachedKey = await aliceClient.getCrypto()!.getSessionBackupPrivateKey();
+            const cachedKey = await retrieveBackupPrivateKeyWithDelay();
             expect(cachedKey).toBeNull();
         });
 
@@ -1343,26 +1342,38 @@ describe("verification", () => {
 
             await sendBackupGossipAndExpectVersion(requestId!, "InvalidSecret", matchingBackupInfo);
 
+            // the backup secret should not be cached
+            const cachedKey = await retrieveBackupPrivateKeyWithDelay();
+            expect(cachedKey).toBeNull();
+        });
+
+        /**
+         * Waits briefly for secrets to be gossipped, then fetches the backup private key from the crypto stack.
+         */
+        async function retrieveBackupPrivateKeyWithDelay(): Promise<Uint8Array | null> {
             // We are lacking a way to signal that the secret has been received, so we wait a bit..
             jest.useRealTimers();
             await new Promise((resolve) => {
                 setTimeout(resolve, 500);
             });
             jest.useFakeTimers({ doNotFake: ["queueMicrotask"] });
 
-            // the backup secret should not be cached
-            const cachedKey = await aliceClient.getCrypto()!.getSessionBackupPrivateKey();
-            expect(cachedKey).toBeNull();
-        });
+            return aliceClient.getCrypto()!.getSessionBackupPrivateKey();
+        }
 
         /**
          * Common test setup for gossiping secrets.
          * Creates a peer to peer session, sends the secret, mockup the version API, send the secret back from sync, then await for the backup check.
+         *
+         * @param expectBackup - The result to be returned from the `/room_keys/version` request.
+         * - **KeyBackupInfo**: Indicates a successful request, where the response contains the key backup information (HTTP 200).
+         * - **MatrixError**: Represents an error response from the server, indicating an unsuccessful request (non-200 HTTP status).
+         * - **Error**: Indicates an error during the request process itself (e.g., network issues or unexpected failures).
          */
         async function sendBackupGossipAndExpectVersion(
             requestId: string,
             secret: string,
-            expectBackup: KeyBackupInfo,
+            expectBackup: KeyBackupInfo | MatrixError | Error,
         ) {
             const p2pSession = await createOlmSession(testOlmAccount, e2eKeyReceiver);
 
@@ -1382,6 +1393,17 @@ describe("verification", () => {
                     "express:/_matrix/client/v3/room_keys/version",
                     (url, request) => {
                         resolve(undefined);
+                        if (expectBackup instanceof MatrixError) {
+                            return {
+                                status: expectBackup.httpStatus,
+                                body: expectBackup.data,
+                            };
+                        }
+
+                        if (expectBackup instanceof Error) {
+                            return Promise.reject(expectBackup);
+                        }
+
                         return expectBackup;
                     },
                     {
diff --git a/src/rust-crypto/backup.ts b/src/rust-crypto/backup.ts
@@ -161,12 +161,17 @@ export class RustBackupManager extends TypedEventEmitter<RustBackupCryptoEvents,
     public async handleBackupSecretReceived(secret: string): Promise<boolean> {
         // Currently we only receive the decryption key without any key backup version. It is important to
         // check that the secret is valid for the current version before storing it.
-        // We force a check to ensure to have the latest version. We also want to check that the backup is trusted
-        // as we don't want to store the secret if the backup is not trusted, and eventually import megolm keys later from an untrusted backup.
-        const backupCheck = await this.checkKeyBackupAndEnable(true);
+        // We force a check to ensure to have the latest version.
+        let latestBackupInfo: KeyBackupInfo | null;
+        try {
+            latestBackupInfo = await this.requestKeyBackupVersion();
+        } catch (e) {
+            logger.warn("handleBackupSecretReceived: Error checking for latest key backup", e);
+            return false;
+        }
 
-        if (!backupCheck?.backupInfo?.version || !backupCheck.trustInfo.trusted) {
-            // There is no server-side key backup, or the backup is not signed by a trusted cross-signing key or trusted own device.
+        if (!latestBackupInfo?.version) {
+            // There is no server-side key backup.
             // This decryption key is useless to us.
             logger.warn(
                 "handleBackupSecretReceived: Received a backup decryption key, but there is no trusted server-side key backup",
@@ -176,7 +181,7 @@ export class RustBackupManager extends TypedEventEmitter<RustBackupCryptoEvents,
 
         try {
             const backupDecryptionKey = RustSdkCryptoJs.BackupDecryptionKey.fromBase64(secret);
-            const privateKeyMatches = backupInfoMatchesBackupDecryptionKey(backupCheck.backupInfo, backupDecryptionKey);
+            const privateKeyMatches = backupInfoMatchesBackupDecryptionKey(latestBackupInfo, backupDecryptionKey);
             if (!privateKeyMatches) {
                 logger.warn(
                     `handleBackupSecretReceived: Private decryption key does not match the public key of the current remote backup.`,
@@ -187,7 +192,7 @@ export class RustBackupManager extends TypedEventEmitter<RustBackupCryptoEvents,
             logger.info(
                 `handleBackupSecretReceived: A valid backup decryption key has been received and stored in cache.`,
             );
-            await this.saveBackupDecryptionKey(backupDecryptionKey, backupCheck.backupInfo.version);
+            await this.saveBackupDecryptionKey(backupDecryptionKey, latestBackupInfo.version);
             return true;
         } catch (e) {
             logger.warn("handleBackupSecretReceived: Invalid backup decryption key", e);
@@ -303,7 +308,9 @@ export class RustBackupManager extends TypedEventEmitter<RustBackupCryptoEvents,
 
         const trustInfo = await this.isKeyBackupTrusted(backupInfo);
 
-        if (!trustInfo.trusted) {
+        // Per the spec, we should enable key upload if either (a) the backup is signed by a trusted key, or
+        // (b) the public key matches the private decryption key that we have received from 4S.
+        if (!trustInfo.matchesDecryptionKey && !trustInfo.trusted) {
             if (activeVersion !== null) {
                 logger.log("Key backup present on server but not trusted: disabling key backup");
                 await this.disableKeyBackup();
