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

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

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from
+51 −6
Open

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
4443c3e
disable key backup when both trust via signatures and private key fail
ajbura Feb 3, 2025
2737071
test for enabling backup with decryption key
ajbura Feb 4, 2025
9ee08b1
enable backup with decryption key in legacy crypto
ajbura Feb 4, 2025
7b75d5f
fix formmating
ajbura Feb 4, 2025
a518470
fix typo
ajbura Feb 4, 2025
e678c08
add local variable for backup trust in legacy crypto
ajbura Feb 4, 2025
5ac44be
Update spec/integ/crypto/megolm-backup.spec.ts
ajbura Feb 4, 2025
e71dfe5
Update spec/integ/crypto/megolm-backup.spec.ts
ajbura Feb 4, 2025
168e45c
Update spec/integ/crypto/megolm-backup.spec.ts
ajbura Feb 4, 2025
6459c3b
Update src/rust-crypto/backup.ts
ajbura Feb 4, 2025
923c120
fix white space formatting
ajbura Feb 4, 2025
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
41 changes: 40 additions & 1 deletion spec/integ/crypto/megolm-backup.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1077,7 +1077,46 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("megolm-keys backup (%s)", (backe
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 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);
});
Comment on lines +1080 to +1098
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


it("enables a backup signed by untrusted devices but have decryption key", async () => {
ajbura marked this conversation as resolved.
Show resolved Hide resolved
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();
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
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!,
);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


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: false, matchesDecryptionKey: true });
expect(await aliceCrypto.getActiveSessionBackupVersion()).toEqual(testData.SIGNED_BACKUP_DATA.version);
});

it("does not enable a backup signed by an untrusted device and does not have decryption key", async () => {
ajbura marked this conversation as resolved.
Show resolved Hide resolved
aliceClient = await initTestClient();
const aliceCrypto = aliceClient.getCrypto()!;

Expand Down
8 changes: 4 additions & 4 deletions src/crypto/backup.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,15 +286,15 @@ export class BackupManager {

const trustInfo = await this.isKeyBackupTrusted(backupInfo);

if (trustInfo.usable && !this.backupInfo) {
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) {
Copy link
Member

Choose a reason for hiding this comment

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

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

// may not be the same version: if not, we should switch
if (backupInfo!.version !== this.backupInfo.version) {
logger.log(
Expand Down
2 changes: 1 addition & 1 deletion src/rust-crypto/backup.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,7 @@ export class RustBackupManager extends TypedEventEmitter<RustBackupCryptoEvents,

const trustInfo = await this.isKeyBackupTrusted(backupInfo);

if (!trustInfo.trusted) {
if (!trustInfo.matchesDecryptionKey && !trustInfo.trusted) {
ajbura marked this conversation as resolved.
Show resolved Hide resolved
if (activeVersion !== null) {
logger.log("Key backup present on server but not trusted: disabling key backup");
await this.disableKeyBackup();
Expand Down
Loading