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.

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

Fix resetEncryption to remove secrets in 4S #4683

Merged
merged 5 commits into from
Feb 5, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 8 additions & 0 deletions spec/unit/rust-crypto/rust-crypto.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2246,6 +2246,8 @@ describe("RustCrypto", () => {
setDefaultKeyId: jest.fn(),
hasKey: jest.fn().mockResolvedValue(false),
getKey: jest.fn().mockResolvedValue(null),
store: jest.fn(),
getDefaultKeyId: jest.fn().mockResolvedValue("defaultKeyId"),
} as unknown as ServerSideSecretStorage;

fetchMock.post("path:/_matrix/client/v3/keys/upload", { one_time_key_counts: {} });
Expand Down Expand Up @@ -2284,6 +2286,12 @@ describe("RustCrypto", () => {
const authUploadDeviceSigningKeys = jest.fn();
await rustCrypto.resetEncryption(authUploadDeviceSigningKeys);

// The secrets in 4S should be deleted
expect(secretStorage.store).toHaveBeenCalledWith("m.cross_signing.master", null);
expect(secretStorage.store).toHaveBeenCalledWith("m.cross_signing.self_signing", null);
expect(secretStorage.store).toHaveBeenCalledWith("m.cross_signing.user_signing", null);
expect(secretStorage.store).toHaveBeenCalledWith("m.megolm_backup.v1", null);
expect(secretStorage.store).toHaveBeenCalledWith("m.secret_storage.key.defaultKeyId", null);
// A new key backup should be created
expect(newKeyBackupInfo.auth_data).toBeTruthy();
// The new cross signing keys should be uploaded
Expand Down
16 changes: 13 additions & 3 deletions spec/unit/secret-storage.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -243,11 +243,16 @@ describe("ServerSideSecretStorageImpl", function () {
});

describe("store", () => {
it("should ignore keys with unknown algorithm", async function () {
const accountDataAdapter = mockAccountDataClient();
let secretStorage: ServerSideSecretStorage;
let accountDataAdapter: Mocked<AccountDataClient>;

beforeEach(() => {
accountDataAdapter = mockAccountDataClient();
const mockCallbacks = { getSecretStorageKey: jest.fn() } as Mocked<SecretStorageCallbacks>;
const secretStorage = new ServerSideSecretStorageImpl(accountDataAdapter, mockCallbacks);
secretStorage = new ServerSideSecretStorageImpl(accountDataAdapter, mockCallbacks);
});

it("should ignore keys with unknown algorithm", async function () {
// stub out getAccountData to return a key with an unknown algorithm
const storedKey = { algorithm: "badalg" } as SecretStorageKeyDescriptionCommon;
async function mockGetAccountData<K extends keyof AccountDataEvents>(
Expand All @@ -274,6 +279,11 @@ describe("ServerSideSecretStorageImpl", function () {
// eslint-disable-next-line no-console
expect(console.warn).toHaveBeenCalledWith(expect.stringContaining("unknown algorithm"));
});

it("should set the secret with an empty object when the value is null", async function () {
await secretStorage.store("mySecret", null);
expect(accountDataAdapter.setAccountData).toHaveBeenCalledWith("mySecret", {});
});
});

describe("setDefaultKeyId", function () {
Expand Down
9 changes: 9 additions & 0 deletions src/rust-crypto/rust-crypto.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1508,6 +1508,15 @@ export class RustCrypto extends TypedEventEmitter<RustCryptoEvents, CryptoEventH
// Disable backup, and delete all the backups from the server
await this.backupManager.deleteAllKeyBackupVersions();

// Remove the stored secrets in the secret storage
await this.secretStorage.store("m.cross_signing.master", null);
await this.secretStorage.store("m.cross_signing.self_signing", null);
await this.secretStorage.store("m.cross_signing.user_signing", null);
await this.secretStorage.store("m.megolm_backup.v1", null);

// Remove the recovery key
const defaultKeyId = await this.secretStorage.getDefaultKeyId();
if (defaultKeyId) await this.secretStorage.store(`m.secret_storage.key.${defaultKeyId}`, null);
// Disable the recovery key and the secret storage
await this.secretStorage.setDefaultKeyId(null);

Expand Down
26 changes: 14 additions & 12 deletions src/secret-storage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -279,14 +279,18 @@ export interface ServerSideSecretStorage {
* Store an encrypted secret on the server.
*
* Details of the encryption keys to be used must previously have been stored in account data
* (for example, via {@link ServerSideSecretStorage#addKey}.
* (for example, via {@link ServerSideSecretStorageImpl#addKey}. {@link SecretStorageCallbacks#getSecretStorageKey} will be called to obtain a secret storage
* key to decrypt the secret.
*
* If the secret is `null`, the secret value in the account data will be set to an empty object.
* This is considered as "removing" the secret.
*
* @param name - The name of the secret - i.e., the "event type" to be stored in the account data
* @param secret - The secret contents.
* @param keys - The IDs of the keys to use to encrypt the secret, or null/undefined to use the default key
* (will throw if no default key is set).
*/
store(name: string, secret: string, keys?: string[] | null): Promise<void>;
store(name: string, secret: string | null, keys?: string[] | null): Promise<void>;

/**
* Get a secret from storage, and decrypt it.
Expand Down Expand Up @@ -503,17 +507,15 @@ export class ServerSideSecretStorageImpl implements ServerSideSecretStorage {
}

/**
* Store an encrypted secret on the server.
*
* Details of the encryption keys to be used must previously have been stored in account data
* (for example, via {@link ServerSideSecretStorageImpl#addKey}. {@link SecretStorageCallbacks#getSecretStorageKey} will be called to obtain a secret storage
* key to decrypt the secret.
*
* @param name - The name of the secret - i.e., the "event type" to be stored in the account data
* @param secret - The secret contents.
* @param keys - The IDs of the keys to use to encrypt the secret, or null/undefined to use the default key.
* Implementation of {@link ServerSideSecretStorage#store}.
*/
public async store(name: SecretStorageKey, secret: string, keys?: string[] | null): Promise<void> {
public async store(name: SecretStorageKey, secret: string | null, keys?: string[] | null): Promise<void> {
if (secret === null) {
// remove secret
await this.accountDataAdapter.setAccountData(name, {});
return;
}

const encrypted: Record<string, AESEncryptedSecretStoragePayload> = {};

if (!keys) {
Expand Down