Skip to content

Commit a1a0463

Browse files
ajburarichvdh
ajbura
and
richvdh
authored
Enable key upload to backups where we have the decryption key (#4677)
* 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 <1389908+richvdh@users.noreply.github.com> * Update spec/integ/crypto/megolm-backup.spec.ts Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * Update spec/integ/crypto/megolm-backup.spec.ts Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * Update src/rust-crypto/backup.ts Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * 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 <1389908+richvdh@users.noreply.github.com> * 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 <1389908+richvdh@users.noreply.github.com> * 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 <1389908+richvdh@users.noreply.github.com> * 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 <1389908+richvdh@users.noreply.github.com> * improve doc for retrieveBackupPrivateKeyWithDelay Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * improve expectBackup param description Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * fix status code and formatting --------- Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
1 parent 30aa66e commit a1a0463

File tree

3 files changed

+94
-43
lines changed

3 files changed

+94
-43
lines changed

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

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -945,7 +945,29 @@ describe("megolm-keys backup", () => {
945945
expect(await aliceCrypto.getActiveSessionBackupVersion()).toEqual(testData.SIGNED_BACKUP_DATA.version);
946946
});
947947

948-
it("does not enable a backup signed by an untrusted device", async () => {
948+
it("enables a backup not signed by a trusted device, when we have the decryption key", async () => {
949+
aliceClient = await initTestClient();
950+
const aliceCrypto = aliceClient.getCrypto()!;
951+
952+
// download the device list, to match the trusted-device case
953+
await aliceClient.startClient();
954+
await waitForDeviceList();
955+
956+
fetchMock.get("path:/_matrix/client/v3/room_keys/version", testData.SIGNED_BACKUP_DATA);
957+
958+
// Alice does *not* trust the device that signed the backup, but *does* have the decryption key.
959+
await aliceCrypto.storeSessionBackupPrivateKey(
960+
Buffer.from(testData.BACKUP_DECRYPTION_KEY_BASE64, "base64"),
961+
testData.SIGNED_BACKUP_DATA.version!,
962+
);
963+
964+
const result = await aliceCrypto.checkKeyBackupAndEnable();
965+
expect(result).toBeTruthy();
966+
expect(result!.trustInfo).toEqual({ trusted: false, matchesDecryptionKey: true });
967+
expect(await aliceCrypto.getActiveSessionBackupVersion()).toEqual(testData.SIGNED_BACKUP_DATA.version);
968+
});
969+
970+
it("does not enable a backup signed by an untrusted device when we do not have the decryption key", async () => {
949971
aliceClient = await initTestClient();
950972
const aliceCrypto = aliceClient.getCrypto()!;
951973

‎spec/integ/crypto/verification.spec.ts‎

Lines changed: 56 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ import {
3030
type ICreateClientOpts,
3131
type IEvent,
3232
type MatrixClient,
33+
MatrixError,
3334
MatrixEvent,
3435
MatrixEventEvent,
3536
} from "../../../src";
@@ -1264,48 +1265,53 @@ describe("verification", () => {
12641265
expect(encodeBase64(cachedKey!)).toEqual(BACKUP_DECRYPTION_KEY_BASE64);
12651266
});
12661267

1267-
it("Should not accept the backup decryption key gossip if private key do not match", async () => {
1268+
it("Should not accept the backup decryption key gossip when there is no server-side key backup", async () => {
12681269
const requestPromises = mockSecretRequestAndGetPromises();
12691270

12701271
await doInteractiveVerification();
12711272

12721273
const requestId = await requestPromises.get("m.megolm_backup.v1");
12731274

1274-
await sendBackupGossipAndExpectVersion(requestId!, BACKUP_DECRYPTION_KEY_BASE64, nonMatchingBackupInfo);
1275-
1276-
// We are lacking a way to signal that the secret has been received, so we wait a bit..
1277-
jest.useRealTimers();
1278-
await new Promise((resolve) => {
1279-
setTimeout(resolve, 500);
1280-
});
1281-
jest.useFakeTimers({ doNotFake: ["queueMicrotask"] });
1275+
await sendBackupGossipAndExpectVersion(
1276+
requestId!,
1277+
BACKUP_DECRYPTION_KEY_BASE64,
1278+
new MatrixError({ errcode: "M_NOT_FOUND", error: "No backup found" }, 404),
1279+
);
12821280

12831281
// the backup secret should not be cached
1284-
const cachedKey = await aliceClient.getCrypto()!.getSessionBackupPrivateKey();
1282+
const cachedKey = await retrieveBackupPrivateKeyWithDelay();
12851283
expect(cachedKey).toBeNull();
12861284
});
12871285

1288-
it("Should not accept the backup decryption key gossip if backup not trusted", async () => {
1286+
it("Should not accept the backup decryption key gossip when server-side key backup request errors", async () => {
12891287
const requestPromises = mockSecretRequestAndGetPromises();
12901288

12911289
await doInteractiveVerification();
12921290

12931291
const requestId = await requestPromises.get("m.megolm_backup.v1");
12941292

1295-
const infoCopy = Object.assign({}, matchingBackupInfo);
1296-
delete infoCopy.auth_data.signatures;
1293+
await sendBackupGossipAndExpectVersion(
1294+
requestId!,
1295+
BACKUP_DECRYPTION_KEY_BASE64,
1296+
new Error("Network Error!"),
1297+
);
12971298

1298-
await sendBackupGossipAndExpectVersion(requestId!, BACKUP_DECRYPTION_KEY_BASE64, infoCopy);
1299+
// the backup secret should not be cached
1300+
const cachedKey = await retrieveBackupPrivateKeyWithDelay();
1301+
expect(cachedKey).toBeNull();
1302+
});
12991303

1300-
// We are lacking a way to signal that the secret has been received, so we wait a bit..
1301-
jest.useRealTimers();
1302-
await new Promise((resolve) => {
1303-
setTimeout(resolve, 500);
1304-
});
1305-
jest.useFakeTimers({ doNotFake: ["queueMicrotask"] });
1304+
it("Should not accept the backup decryption key gossip if private key do not match", async () => {
1305+
const requestPromises = mockSecretRequestAndGetPromises();
1306+
1307+
await doInteractiveVerification();
1308+
1309+
const requestId = await requestPromises.get("m.megolm_backup.v1");
1310+
1311+
await sendBackupGossipAndExpectVersion(requestId!, BACKUP_DECRYPTION_KEY_BASE64, nonMatchingBackupInfo);
13061312

13071313
// the backup secret should not be cached
1308-
const cachedKey = await aliceClient.getCrypto()!.getSessionBackupPrivateKey();
1314+
const cachedKey = await retrieveBackupPrivateKeyWithDelay();
13091315
expect(cachedKey).toBeNull();
13101316
});
13111317

@@ -1322,15 +1328,8 @@ describe("verification", () => {
13221328
unknownAlgorithmBackupInfo,
13231329
);
13241330

1325-
// We are lacking a way to signal that the secret has been received, so we wait a bit..
1326-
jest.useRealTimers();
1327-
await new Promise((resolve) => {
1328-
setTimeout(resolve, 500);
1329-
});
1330-
jest.useFakeTimers({ doNotFake: ["queueMicrotask"] });
1331-
13321331
// the backup secret should not be cached
1333-
const cachedKey = await aliceClient.getCrypto()!.getSessionBackupPrivateKey();
1332+
const cachedKey = await retrieveBackupPrivateKeyWithDelay();
13341333
expect(cachedKey).toBeNull();
13351334
});
13361335

@@ -1343,26 +1342,38 @@ describe("verification", () => {
13431342

13441343
await sendBackupGossipAndExpectVersion(requestId!, "InvalidSecret", matchingBackupInfo);
13451344

1345+
// the backup secret should not be cached
1346+
const cachedKey = await retrieveBackupPrivateKeyWithDelay();
1347+
expect(cachedKey).toBeNull();
1348+
});
1349+
1350+
/**
1351+
* Waits briefly for secrets to be gossipped, then fetches the backup private key from the crypto stack.
1352+
*/
1353+
async function retrieveBackupPrivateKeyWithDelay(): Promise<Uint8Array | null> {
13461354
// We are lacking a way to signal that the secret has been received, so we wait a bit..
13471355
jest.useRealTimers();
13481356
await new Promise((resolve) => {
13491357
setTimeout(resolve, 500);
13501358
});
13511359
jest.useFakeTimers({ doNotFake: ["queueMicrotask"] });
13521360

1353-
// the backup secret should not be cached
1354-
const cachedKey = await aliceClient.getCrypto()!.getSessionBackupPrivateKey();
1355-
expect(cachedKey).toBeNull();
1356-
});
1361+
return aliceClient.getCrypto()!.getSessionBackupPrivateKey();
1362+
}
13571363

13581364
/**
13591365
* Common test setup for gossiping secrets.
13601366
* 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.
1367+
*
1368+
* @param expectBackup - The result to be returned from the `/room_keys/version` request.
1369+
* - **KeyBackupInfo**: Indicates a successful request, where the response contains the key backup information (HTTP 200).
1370+
* - **MatrixError**: Represents an error response from the server, indicating an unsuccessful request (non-200 HTTP status).
1371+
* - **Error**: Indicates an error during the request process itself (e.g., network issues or unexpected failures).
13611372
*/
13621373
async function sendBackupGossipAndExpectVersion(
13631374
requestId: string,
13641375
secret: string,
1365-
expectBackup: KeyBackupInfo,
1376+
expectBackup: KeyBackupInfo | MatrixError | Error,
13661377
) {
13671378
const p2pSession = await createOlmSession(testOlmAccount, e2eKeyReceiver);
13681379

@@ -1382,6 +1393,17 @@ describe("verification", () => {
13821393
"express:/_matrix/client/v3/room_keys/version",
13831394
(url, request) => {
13841395
resolve(undefined);
1396+
if (expectBackup instanceof MatrixError) {
1397+
return {
1398+
status: expectBackup.httpStatus,
1399+
body: expectBackup.data,
1400+
};
1401+
}
1402+
1403+
if (expectBackup instanceof Error) {
1404+
return Promise.reject(expectBackup);
1405+
}
1406+
13851407
return expectBackup;
13861408
},
13871409
{

‎src/rust-crypto/backup.ts‎

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -161,12 +161,17 @@ export class RustBackupManager extends TypedEventEmitter<RustBackupCryptoEvents,
161161
public async handleBackupSecretReceived(secret: string): Promise<boolean> {
162162
// Currently we only receive the decryption key without any key backup version. It is important to
163163
// check that the secret is valid for the current version before storing it.
164-
// We force a check to ensure to have the latest version. We also want to check that the backup is trusted
165-
// 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.
166-
const backupCheck = await this.checkKeyBackupAndEnable(true);
164+
// We force a check to ensure to have the latest version.
165+
let latestBackupInfo: KeyBackupInfo | null;
166+
try {
167+
latestBackupInfo = await this.requestKeyBackupVersion();
168+
} catch (e) {
169+
logger.warn("handleBackupSecretReceived: Error checking for latest key backup", e);
170+
return false;
171+
}
167172

168-
if (!backupCheck?.backupInfo?.version || !backupCheck.trustInfo.trusted) {
169-
// There is no server-side key backup, or the backup is not signed by a trusted cross-signing key or trusted own device.
173+
if (!latestBackupInfo?.version) {
174+
// There is no server-side key backup.
170175
// This decryption key is useless to us.
171176
logger.warn(
172177
"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,
176181

177182
try {
178183
const backupDecryptionKey = RustSdkCryptoJs.BackupDecryptionKey.fromBase64(secret);
179-
const privateKeyMatches = backupInfoMatchesBackupDecryptionKey(backupCheck.backupInfo, backupDecryptionKey);
184+
const privateKeyMatches = backupInfoMatchesBackupDecryptionKey(latestBackupInfo, backupDecryptionKey);
180185
if (!privateKeyMatches) {
181186
logger.warn(
182187
`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,
187192
logger.info(
188193
`handleBackupSecretReceived: A valid backup decryption key has been received and stored in cache.`,
189194
);
190-
await this.saveBackupDecryptionKey(backupDecryptionKey, backupCheck.backupInfo.version);
195+
await this.saveBackupDecryptionKey(backupDecryptionKey, latestBackupInfo.version);
191196
return true;
192197
} catch (e) {
193198
logger.warn("handleBackupSecretReceived: Invalid backup decryption key", e);
@@ -303,7 +308,9 @@ export class RustBackupManager extends TypedEventEmitter<RustBackupCryptoEvents,
303308

304309
const trustInfo = await this.isKeyBackupTrusted(backupInfo);
305310

306-
if (!trustInfo.trusted) {
311+
// Per the spec, we should enable key upload if either (a) the backup is signed by a trusted key, or
312+
// (b) the public key matches the private decryption key that we have received from 4S.
313+
if (!trustInfo.matchesDecryptionKey && !trustInfo.trusted) {
307314
if (activeVersion !== null) {
308315
logger.log("Key backup present on server but not trusted: disabling key backup");
309316
await this.disableKeyBackup();

0 commit comments

Comments
 (0)