Skip to content
This repository was archived by the owner on Sep 11, 2024. It is now read-only.

Commit 1a469f4

Browse files
t3chguyt3chguy
authored
Fix a fresh login creating a new key backup (#12106)
Co-authored-by: Richard van der Hoff <[email protected]> Co-authored-by: Valere <[email protected]> fix repeated requests to enter 4S key during cross-signing reset (#12059)
1 parent 951c0d8 commit 1a469f4

File tree

15 files changed

+302
-348
lines changed

15 files changed

+302
-348
lines changed

‎playwright/e2e/crypto/backups.spec.ts

-57
This file was deleted.

‎playwright/e2e/crypto/crypto.spec.ts

+37
Original file line numberDiff line numberDiff line change
@@ -212,6 +212,43 @@ test.describe("Cryptography", function () {
212212
});
213213
}
214214

215+
test("Can reset cross-signing keys", async ({ page, app, user: aliceCredentials }) => {
216+
const secretStorageKey = await enableKeyBackup(app);
217+
218+
// Fetch the current cross-signing keys
219+
async function fetchMasterKey() {
220+
return await test.step("Fetch master key from server", async () => {
221+
const k = await app.client.evaluate(async (cli) => {
222+
const userId = cli.getUserId();
223+
const keys = await cli.downloadKeysForUsers([userId]);
224+
return Object.values(keys.master_keys[userId].keys)[0];
225+
});
226+
console.log(`fetchMasterKey: ${k}`);
227+
return k;
228+
});
229+
}
230+
const masterKey1 = await fetchMasterKey();
231+
232+
// Find the "reset cross signing" button, and click it
233+
await app.settings.openUserSettings("Security & Privacy");
234+
await page.locator("div.mx_CrossSigningPanel_buttonRow").getByRole("button", { name: "Reset" }).click();
235+
236+
// Confirm
237+
await page.getByRole("button", { name: "Clear cross-signing keys" }).click();
238+
239+
// Enter the 4S key
240+
await page.getByPlaceholder("Security Key").fill(secretStorageKey);
241+
await page.getByRole("button", { name: "Continue" }).click();
242+
243+
await expect(async () => {
244+
const masterKey2 = await fetchMasterKey();
245+
expect(masterKey1).not.toEqual(masterKey2);
246+
}).toPass();
247+
248+
// The dialog should have gone away
249+
await expect(page.locator(".mx_Dialog")).toHaveCount(1);
250+
});
251+
215252
test("creating a DM should work, being e2e-encrypted / user verification", async ({
216253
page,
217254
app,

‎playwright/e2e/crypto/utils.ts

+25-2
Original file line numberDiff line numberDiff line change
@@ -103,12 +103,35 @@ export async function checkDeviceIsCrossSigned(app: ElementAppPage): Promise<voi
103103
}
104104

105105
/**
106-
* Check that the current device is connected to the key backup.
106+
* Check that the current device is connected to the expected key backup.
107+
* Also checks that the decryption key is known and cached locally.
108+
*
109+
* @param page - the page to check
110+
* @param expectedBackupVersion - the version of the backup we expect to be connected to.
111+
* @param checkBackupKeyInCache - whether to check that the backup key is cached locally.
107112
*/
108-
export async function checkDeviceIsConnectedKeyBackup(page: Page) {
113+
export async function checkDeviceIsConnectedKeyBackup(
114+
page: Page,
115+
expectedBackupVersion: string,
116+
checkBackupKeyInCache: boolean,
117+
): Promise<void> {
109118
await page.getByRole("button", { name: "User menu" }).click();
110119
await page.locator(".mx_UserMenu_contextMenu").getByRole("menuitem", { name: "Security & Privacy" }).click();
111120
await expect(page.locator(".mx_Dialog").getByRole("button", { name: "Restore from Backup" })).toBeVisible();
121+
122+
// expand the advanced section to see the active version in the reports
123+
await page.locator(".mx_SecureBackupPanel_advanced").locator("..").click();
124+
125+
if (checkBackupKeyInCache) {
126+
const cacheDecryptionKeyStatusElement = page.locator(".mx_SecureBackupPanel_statusList tr:nth-child(2) td");
127+
await expect(cacheDecryptionKeyStatusElement).toHaveText("cached locally, well formed");
128+
}
129+
130+
await expect(page.locator(".mx_SecureBackupPanel_statusList tr:nth-child(5) td")).toHaveText(
131+
expectedBackupVersion + " (Algorithm: m.megolm_backup.v1.curve25519-aes-sha2)",
132+
);
133+
134+
await expect(page.locator(".mx_SecureBackupPanel_statusList tr:nth-child(6) td")).toHaveText(expectedBackupVersion);
112135
}
113136

114137
/**

‎playwright/e2e/crypto/verification.spec.ts

+18-5
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,9 @@ import { Bot } from "../../pages/bot";
3333
test.describe("Device verification", () => {
3434
let aliceBotClient: Bot;
3535

36+
/** The backup version that was set up by the bot client. */
37+
let expectedBackupVersion: string;
38+
3639
test.beforeEach(async ({ page, homeserver, credentials }) => {
3740
// Visit the login page of the app, to load the matrix sdk
3841
await page.goto("/#/login");
@@ -49,9 +52,13 @@ test.describe("Device verification", () => {
4952
bootstrapSecretStorage: true,
5053
});
5154
aliceBotClient.setCredentials(credentials);
52-
await aliceBotClient.prepareClient();
55+
const mxClientHandle = await aliceBotClient.prepareClient();
5356

5457
await page.waitForTimeout(20000);
58+
59+
expectedBackupVersion = await mxClientHandle.evaluate(async (mxClient) => {
60+
return await mxClient.getCrypto()!.getActiveSessionBackupVersion();
61+
});
5562
});
5663

5764
// Click the "Verify with another device" button, and have the bot client auto-accept it.
@@ -87,7 +94,9 @@ test.describe("Device verification", () => {
8794
await checkDeviceIsCrossSigned(app);
8895

8996
// Check that the current device is connected to key backup
90-
await checkDeviceIsConnectedKeyBackup(page);
97+
// For now we don't check that the backup key is in cache because it's a bit flaky,
98+
// as we need to wait for the secret gossiping to happen and the settings dialog doesn't refresh automatically.
99+
await checkDeviceIsConnectedKeyBackup(page, expectedBackupVersion, false);
91100
});
92101

93102
test("Verify device with QR code during login", async ({ page, app, credentials, homeserver }) => {
@@ -130,7 +139,9 @@ test.describe("Device verification", () => {
130139
await checkDeviceIsCrossSigned(app);
131140

132141
// Check that the current device is connected to key backup
133-
await checkDeviceIsConnectedKeyBackup(page);
142+
// For now we don't check that the backup key is in cache because it's a bit flaky,
143+
// as we need to wait for the secret gossiping to happen and the settings dialog doesn't refresh automatically.
144+
await checkDeviceIsConnectedKeyBackup(page, expectedBackupVersion, false);
134145
});
135146

136147
test("Verify device with Security Phrase during login", async ({ page, app, credentials, homeserver }) => {
@@ -150,7 +161,8 @@ test.describe("Device verification", () => {
150161
await checkDeviceIsCrossSigned(app);
151162

152163
// Check that the current device is connected to key backup
153-
await checkDeviceIsConnectedKeyBackup(page);
164+
// The backup decryption key should be in cache also, as we got it directly from the 4S
165+
await checkDeviceIsConnectedKeyBackup(page, expectedBackupVersion, true);
154166
});
155167

156168
test("Verify device with Security Key during login", async ({ page, app, credentials, homeserver }) => {
@@ -172,7 +184,8 @@ test.describe("Device verification", () => {
172184
await checkDeviceIsCrossSigned(app);
173185

174186
// Check that the current device is connected to key backup
175-
await checkDeviceIsConnectedKeyBackup(page);
187+
// The backup decryption key should be in cache also, as we got it directly from the 4S
188+
await checkDeviceIsConnectedKeyBackup(page, expectedBackupVersion, true);
176189
});
177190

178191
test("Handle incoming verification request with SAS", async ({ page, credentials, homeserver, toasts }) => {

‎playwright/element-web-test.ts

-4
Original file line numberDiff line numberDiff line change
@@ -260,7 +260,3 @@ export const expect = baseExpect.extend({
260260
return { pass: true, message: () => "", name: "toMatchScreenshot" };
261261
},
262262
});
263-
264-
test.use({
265-
permissions: ["clipboard-read"],
266-
});

‎playwright/pages/ElementAppPage.ts

-13
Original file line numberDiff line numberDiff line change
@@ -51,19 +51,6 @@ export class ElementAppPage {
5151
return this.settings.closeDialog();
5252
}
5353

54-
public async getClipboard(): Promise<string> {
55-
return await this.page.evaluate(() => navigator.clipboard.readText());
56-
}
57-
58-
/**
59-
* Find an open dialog by its title
60-
*/
61-
public async getDialogByTitle(title: string, timeout = 5000): Promise<Locator> {
62-
const dialog = this.page.locator(".mx_Dialog");
63-
await dialog.getByRole("heading", { name: title }).waitFor({ timeout });
64-
return dialog;
65-
}
66-
6754
/**
6855
* Opens the given room by name. The room must be visible in the
6956
* room list, but the room list may be folded horizontally, and the

‎src/SecurityManager.ts

+30-22
Original file line numberDiff line numberDiff line change
@@ -299,6 +299,28 @@ export async function promptForBackupPassphrase(): Promise<Uint8Array> {
299299
return key;
300300
}
301301

302+
/**
303+
* Carry out an operation that may require multiple accesses to secret storage, caching the key.
304+
*
305+
* Use this helper to wrap an operation that may require multiple accesses to secret storage; the user will be prompted
306+
* to enter the 4S key or passphrase on the first access, and the key will be cached for the rest of the operation.
307+
*
308+
* @param func - The operation to be wrapped.
309+
*/
310+
export async function withSecretStorageKeyCache<T>(func: () => Promise<T>): Promise<T> {
311+
secretStorageBeingAccessed = true;
312+
try {
313+
return await func();
314+
} finally {
315+
// Clear secret storage key cache now that work is complete
316+
secretStorageBeingAccessed = false;
317+
if (!isCachingAllowed()) {
318+
secretStorageKeys = {};
319+
secretStorageKeyInfo = {};
320+
}
321+
}
322+
}
323+
302324
/**
303325
* This helper should be used whenever you need to access secret storage. It
304326
* ensures that secret storage (and also cross-signing since they each depend on
@@ -319,14 +341,13 @@ export async function promptForBackupPassphrase(): Promise<Uint8Array> {
319341
* @param {Function} [func] An operation to perform once secret storage has been
320342
* bootstrapped. Optional.
321343
* @param {bool} [forceReset] Reset secret storage even if it's already set up
322-
* @param {bool} [setupNewKeyBackup] Reset secret storage even if it's already set up
323344
*/
324-
export async function accessSecretStorage(
325-
func = async (): Promise<void> => {},
326-
forceReset = false,
327-
setupNewKeyBackup = true,
328-
): Promise<void> {
329-
secretStorageBeingAccessed = true;
345+
export async function accessSecretStorage(func = async (): Promise<void> => {}, forceReset = false): Promise<void> {
346+
await withSecretStorageKeyCache(() => doAccessSecretStorage(func, forceReset));
347+
}
348+
349+
/** Helper for {@link #accessSecretStorage} */
350+
export async function doAccessSecretStorage(func = async (): Promise<void> => {}, forceReset = false): Promise<void> {
330351
try {
331352
const cli = MatrixClientPeg.safeGet();
332353
if (!(await cli.hasSecretStorageKey()) || forceReset) {
@@ -357,12 +378,7 @@ export async function accessSecretStorage(
357378
throw new Error("Secret storage creation canceled");
358379
}
359380
} else {
360-
const crypto = cli.getCrypto();
361-
if (!crypto) {
362-
throw new Error("End-to-end encryption is disabled - unable to access secret storage.");
363-
}
364-
365-
await crypto.bootstrapCrossSigning({
381+
await cli.bootstrapCrossSigning({
366382
authUploadDeviceSigningKeys: async (makeRequest): Promise<void> => {
367383
const { finished } = Modal.createDialog(InteractiveAuthDialog, {
368384
title: _t("encryption|bootstrap_title"),
@@ -375,9 +391,8 @@ export async function accessSecretStorage(
375391
}
376392
},
377393
});
378-
await crypto.bootstrapSecretStorage({
394+
await cli.bootstrapSecretStorage({
379395
getKeyBackupPassphrase: promptForBackupPassphrase,
380-
setupNewKeyBackup,
381396
});
382397

383398
const keyId = Object.keys(secretStorageKeys)[0];
@@ -403,13 +418,6 @@ export async function accessSecretStorage(
403418
logger.error(e);
404419
// Re-throw so that higher level logic can abort as needed
405420
throw e;
406-
} finally {
407-
// Clear secret storage key cache now that work is complete
408-
secretStorageBeingAccessed = false;
409-
if (!isCachingAllowed()) {
410-
secretStorageKeys = {};
411-
secretStorageKeyInfo = {};
412-
}
413421
}
414422
}
415423

‎src/async-components/views/dialogs/security/CreateKeyBackupDialog.tsx

+20-17
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ limitations under the License.
1717

1818
import React from "react";
1919
import { logger } from "matrix-js-sdk/src/logger";
20+
import { IKeyBackupInfo } from "matrix-js-sdk/src/crypto/keybackup";
2021

2122
import { MatrixClientPeg } from "../../../../MatrixClientPeg";
2223
import { _t } from "../../../../languageHandler";
@@ -74,25 +75,24 @@ export default class CreateKeyBackupDialog extends React.PureComponent<IProps, I
7475
this.setState({
7576
error: undefined,
7677
});
78+
let info: IKeyBackupInfo | undefined;
7779
const cli = MatrixClientPeg.safeGet();
7880
try {
79-
// We don't want accessSecretStorage to create a backup for us - we
80-
// will create one ourselves in the closure we pass in by calling
81-
// resetKeyBackup.
82-
const setupNewKeyBackup = false;
83-
const forceReset = false;
84-
85-
await accessSecretStorage(
86-
async (): Promise<void> => {
87-
const crypto = cli.getCrypto();
88-
if (!crypto) {
89-
throw new Error("End-to-end encryption is disabled - unable to create backup.");
90-
}
91-
await crypto.resetKeyBackup();
92-
},
93-
forceReset,
94-
setupNewKeyBackup,
95-
);
81+
await accessSecretStorage(async (): Promise<void> => {
82+
// `accessSecretStorage` will have bootstrapped secret storage if necessary, so we can now
83+
// set up key backup.
84+
//
85+
// XXX: `bootstrapSecretStorage` also sets up key backup as a side effect, so there is a 90% chance
86+
// this is actually redundant.
87+
//
88+
// The only time it would *not* be redundant would be if, for some reason, we had working 4S but no
89+
// working key backup. (For example, if the user clicked "Delete Backup".)
90+
info = await cli.prepareKeyBackupVersion(null /* random key */, {
91+
secureSecretStorage: true,
92+
});
93+
info = await cli.createKeyBackupVersion(info);
94+
});
95+
await cli.scheduleAllGroupSessionsForBackup();
9696
this.setState({
9797
phase: Phase.Done,
9898
});
@@ -102,6 +102,9 @@ export default class CreateKeyBackupDialog extends React.PureComponent<IProps, I
102102
// delete the version, disable backup, or do nothing? If we just
103103
// disable without deleting, we'll enable on next app reload since
104104
// it is trusted.
105+
if (info?.version) {
106+
cli.deleteKeyBackupVersion(info.version);
107+
}
105108
this.setState({
106109
error: true,
107110
});

0 commit comments

Comments
 (0)