Enable key upload to backups where we have the decryption key #4677

ajbura · 2025-02-03T07:53:00Z

Fixes #4676

Signed-off-by: Ajay Bura [email protected]

Checklist

Tests written for new code (and old code if feasible).
New or updated public/exported symbols have accurate TSDoc documentation.
Linter and other CI checks pass.
Sign-off given on the changes (see CONTRIBUTING.md).

richvdh

Putting this on hold while we try to understand what bug it is trying to fix, per #4676 (comment).

In any case, we'll need to see an integration test which demonstrates the problem.

richvdh

ok, the fix looks like it makes sense, but the main thing this is missing is a regression test.

Please could you take a look at matrix-js-sdk/spec/integ/crypto/megolm-backup.spec.ts. In there there are some existing tests for checkKeyBackupAndEnable: please could you take a look at them and see if you can add another which covers your usecase.

richvdh

looks generally good to me, thanks.

A few minor requests.

richvdh · 2025-02-04T12:20:00Z

src/crypto/backup.ts

+        if ((trustInfo.usable || trustInfo.trusted_locally) && !this.backupInfo) {
            logger.log(`Found usable key backup v${backupInfo!.version}: enabling key backups`);
            await this.enableKeyBackup(backupInfo!);
-        } else if (!trustInfo.usable && this.backupInfo) {
+        } else if (!trustInfo.usable && !trustInfo.trusted_locally && this.backupInfo) {
            logger.log("No usable key backup: disabling key backup");
            this.disableKeyBackup();
-        } else if (!trustInfo.usable && !this.backupInfo) {
+        } else if (!trustInfo.usable && !trustInfo.trusted_locally && !this.backupInfo) {
            logger.log("No usable key backup: not enabling key backup");
-        } else if (trustInfo.usable && this.backupInfo) {
+        } else if ((trustInfo.usable || trustInfo.trusted_locally) && this.backupInfo) {


this code is schedule for removal very soon so maybe not the most important thing, but it would be good to factor out a local variable for trustInfo.usable || trustInfo.trusted_locally

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

richvdh · 2025-02-04T12:22:35Z

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

+        it("enables a backup signed by trusted devices and have decryption key", async () => {
+            aliceClient = await initTestClient();
+            const aliceCrypto = aliceClient.getCrypto()!;
+
+            await aliceClient.startClient();
+            await aliceCrypto.storeSessionBackupPrivateKey(
+                Buffer.from(testData.BACKUP_DECRYPTION_KEY_BASE64, "base64"),
+                testData.SIGNED_BACKUP_DATA.version!,
+            );
+            await waitForDeviceList();
+            await aliceCrypto.setDeviceVerified(testData.TEST_USER_ID, testData.TEST_DEVICE_ID);
+
+            fetchMock.get("path:/_matrix/client/v3/room_keys/version", testData.SIGNED_BACKUP_DATA);
+
+            const result = await aliceCrypto.checkKeyBackupAndEnable();
+            expect(result).toBeTruthy();
+            expect(result!.trustInfo).toEqual({ trusted: true, matchesDecryptionKey: true });
+            expect(await aliceCrypto.getActiveSessionBackupVersion()).toEqual(testData.SIGNED_BACKUP_DATA.version);
+        });


this test feels a bit redundant to me. Is there a particular reason you feel it is important?

Not any particular reason, I added it as the trust is logical OR of verified signature and decryption key match, so i feel like it is a good idea to cover all 4 combination to stay safe.

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

richvdh · 2025-02-04T12:24:57Z

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

+            await aliceClient.startClient();
+            await aliceCrypto.storeSessionBackupPrivateKey(
+                Buffer.from(testData.BACKUP_DECRYPTION_KEY_BASE64, "base64"),
+                testData.SIGNED_BACKUP_DATA.version!,
+            );
+            await waitForDeviceList();


Suggested change

await aliceClient.startClient();

await aliceCrypto.storeSessionBackupPrivateKey(

Buffer.from(testData.BACKUP_DECRYPTION_KEY_BASE64, "base64"),

testData.SIGNED_BACKUP_DATA.version!,

);

await waitForDeviceList();

// download the device list, to match the trusted-device case

await aliceClient.startClient();

await waitForDeviceList();

// 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!,

);

I am not sure why test is failing after flipping the statements. Can you please help me with it

src/rust-crypto/backup.ts

Co-authored-by: Richard van der Hoff <[email protected]>

disable key backup when both trust via signatures and private key fail

4443c3e

ajbura requested a review from a team as a code owner February 3, 2025 07:53

ajbura requested review from uhoreg and richvdh February 3, 2025 07:53

github-actions bot added the Z-Community-PR Issue is solved by a community member's PR label Feb 3, 2025

github-actions bot deployed to PR Documentation Preview February 3, 2025 07:54 View deployment

richvdh requested changes Feb 3, 2025

View reviewed changes

richvdh changed the title ~~Disable key backup only when both trust via signatures and private key fail~~ Enable key upload to backups where we have the decryption key Feb 3, 2025

richvdh reviewed Feb 3, 2025

View reviewed changes

ajbura added 3 commits February 4, 2025 10:14

test for enabling backup with decryption key

2737071

enable backup with decryption key in legacy crypto

9ee08b1

fix formmating

7b75d5f

github-actions bot deployed to PR Documentation Preview February 4, 2025 05:15 View deployment

ajbura requested a review from richvdh February 4, 2025 05:19

fix typo

a518470

github-actions bot deployed to PR Documentation Preview February 4, 2025 11:14 View deployment

richvdh reviewed Feb 4, 2025

View reviewed changes

richvdh added the T-Enhancement label Feb 4, 2025

ajbura and others added 6 commits February 4, 2025 19:21

add local variable for backup trust in legacy crypto

e678c08

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

5ac44be

Co-authored-by: Richard van der Hoff <[email protected]>

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

e71dfe5

Co-authored-by: Richard van der Hoff <[email protected]>

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

168e45c

Co-authored-by: Richard van der Hoff <[email protected]>

Update src/rust-crypto/backup.ts

6459c3b

Co-authored-by: Richard van der Hoff <[email protected]>

fix white space formatting

923c120

github-actions bot deployed to PR Documentation Preview February 4, 2025 14:51 View deployment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable key upload to backups where we have the decryption key #4677

Enable key upload to backups where we have the decryption key #4677

ajbura commented Feb 3, 2025 •

edited

Loading

richvdh left a comment

richvdh left a comment

richvdh left a comment

richvdh Feb 4, 2025

richvdh Feb 4, 2025

ajbura Feb 4, 2025

richvdh Feb 4, 2025

ajbura Feb 4, 2025

Enable key upload to backups where we have the decryption key #4677

Are you sure you want to change the base?

Enable key upload to backups where we have the decryption key #4677

Conversation

ajbura commented Feb 3, 2025 • edited Loading

Checklist

richvdh left a comment

Choose a reason for hiding this comment

richvdh left a comment

Choose a reason for hiding this comment

richvdh left a comment

Choose a reason for hiding this comment

richvdh Feb 4, 2025

Choose a reason for hiding this comment

richvdh Feb 4, 2025

Choose a reason for hiding this comment

ajbura Feb 4, 2025

Choose a reason for hiding this comment

richvdh Feb 4, 2025

Choose a reason for hiding this comment

ajbura Feb 4, 2025

Choose a reason for hiding this comment

ajbura commented Feb 3, 2025 •

edited

Loading