Split SecretStorage into two parts (matrix-org#3267)

* Pull `SecretStorageCallbacks` out of `ICryptoCallbacks`

* Pull the storage part of SecretStorage out to a new class

* Move SecretSharing to a separate class

* Move `ISecretRequest` into `SecretSharing.ts`

* Pull out ISecretStorage interface, and use it

* Mark old `SecretStorage` as deprecated, and rename accesses to it

* Move a `SecretStorage` unit test into its own file

* Use new `SecretStorage` in a couple of places

* add some more unit tests

* Fix test file name

... to match the unit under test

* even more tests

* Add a load of comments

* Rename classes

* Fix some broken tsdoc links

* fix broken test

* Fix compaints about superlinear regex

* just one more test

Author: richvdh

spec/unit/crypto/secrets.spec.ts

@@ -27,12 +27,7 @@ import { ClientEvent, ICreateClientOpts, ICrossSigningKey, MatrixClient } from "
 import { DeviceInfo } from "../../../src/crypto/deviceinfo";
 import { ISignatures } from "../../../src/@types/signed";
 import { ICurve25519AuthData } from "../../../src/crypto/keybackup";
-import {
-    SecretStorageKeyDescription,
-    SECRET_STORAGE_ALGORITHM_V1_AES,
-    AccountDataClient,
-} from "../../../src/secret-storage";
-import { SecretStorage } from "../../../src/crypto/SecretStorage";
+import { SecretStorageKeyDescription, SECRET_STORAGE_ALGORITHM_V1_AES } from "../../../src/secret-storage";
 
 async function makeTestClient(
     userInfo: { userId: string; deviceId: string },
@@ -81,22 +76,6 @@ describe("Secrets", function () {
         return global.Olm.init();
     });
 
-    it("should allow storing a default key", async function () {
-        const accountDataAdapter = {
-            getAccountDataFromServer: jest.fn().mockResolvedValue(null),
-            setAccountData: jest.fn().mockResolvedValue({}),
-        };
-        const secretStorage = new SecretStorage(accountDataAdapter as unknown as AccountDataClient, {}, undefined);
-        const result = await secretStorage.addKey("m.secret_storage.v1.aes-hmac-sha2");
-
-        // it should have made up a 32-character key id
-        expect(result.keyId.length).toEqual(32);
-        expect(accountDataAdapter.setAccountData).toHaveBeenCalledWith(
-            `m.secret_storage.key.${result.keyId}`,
-            result.keyInfo,
-        );
-    });
-
     it("should store and retrieve a secret", async function () {
         const key = new Uint8Array(16);
         for (let i = 0; i < 16; i++) key[i] = i;

spec/unit/secret-storage.spec.ts

@@ -0,0 +1,270 @@
+/*
+Copyright 2019, 2022-2023 The Matrix.org Foundation C.I.C.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+import { Mocked } from "jest-mock";
+
+import {
+    AccountDataClient,
+    PassphraseInfo,
+    SecretStorageCallbacks,
+    SecretStorageKeyDescriptionAesV1,
+    SecretStorageKeyDescriptionCommon,
+    ServerSideSecretStorageImpl,
+    trimTrailingEquals,
+} from "../../src/secret-storage";
+import { calculateKeyCheck } from "../../src/crypto/aes";
+import { randomString } from "../../src/randomstring";
+
+describe("ServerSideSecretStorageImpl", function () {
+    describe(".addKey", function () {
+        it("should allow storing a default key", async function () {
+            const accountDataAdapter = mockAccountDataClient();
+            const secretStorage = new ServerSideSecretStorageImpl(accountDataAdapter, {});
+            const result = await secretStorage.addKey("m.secret_storage.v1.aes-hmac-sha2");
+
+            // it should have made up a 32-character key id
+            expect(result.keyId.length).toEqual(32);
+            expect(accountDataAdapter.setAccountData).toHaveBeenCalledWith(
+                `m.secret_storage.key.${result.keyId}`,
+                result.keyInfo,
+            );
+        });
+
+        it("should allow storing a key with an explicit id", async function () {
+            const accountDataAdapter = mockAccountDataClient();
+            const secretStorage = new ServerSideSecretStorageImpl(accountDataAdapter, {});
+            const result = await secretStorage.addKey("m.secret_storage.v1.aes-hmac-sha2", {}, "myKeyId");
+
+            // it should have made up a 32-character key id
+            expect(result.keyId).toEqual("myKeyId");
+            expect(accountDataAdapter.setAccountData).toHaveBeenCalledWith(
+                "m.secret_storage.key.myKeyId",
+                result.keyInfo,
+            );
+        });
+
+        it("should allow storing a key with a name", async function () {
+            const accountDataAdapter = mockAccountDataClient();
+            const secretStorage = new ServerSideSecretStorageImpl(accountDataAdapter, {});
+            const result = await secretStorage.addKey("m.secret_storage.v1.aes-hmac-sha2", { name: "mykey" });
+
+            expect(result.keyInfo.name).toEqual("mykey");
+
+            expect(accountDataAdapter.setAccountData).toHaveBeenCalledWith(
+                `m.secret_storage.key.${result.keyId}`,
+                result.keyInfo,
+            );
+        });
+
+        it("should allow storing a key with a passphrase", async function () {
+            const accountDataAdapter = mockAccountDataClient();
+            const secretStorage = new ServerSideSecretStorageImpl(accountDataAdapter, {});
+            const passphrase: PassphraseInfo = {
+                algorithm: "m.pbkdf2",
+                iterations: 125,
+                salt: "saltygoodness",
+                bits: 256,
+            };
+            const result = await secretStorage.addKey("m.secret_storage.v1.aes-hmac-sha2", {
+                passphrase,
+            });
+
+            expect(result.keyInfo.passphrase).toEqual(passphrase);
+
+            expect(accountDataAdapter.setAccountData).toHaveBeenCalledWith(
+                `m.secret_storage.key.${result.keyId}`,
+                result.keyInfo,
+            );
+        });
+
+        it("should complain about invalid algorithm", async function () {
+            const accountDataAdapter = mockAccountDataClient();
+            const secretStorage = new ServerSideSecretStorageImpl(accountDataAdapter, {});
+            await expect(() => secretStorage.addKey("bad_alg")).rejects.toThrow("Unknown key algorithm");
+        });
+    });
+
+    describe("getKey", function () {
+        it("should return the specified key", async function () {
+            const accountDataAdapter = mockAccountDataClient();
+            const secretStorage = new ServerSideSecretStorageImpl(accountDataAdapter, {});
+
+            const storedKey = { iv: "iv", mac: "mac" } as SecretStorageKeyDescriptionAesV1;
+            async function mockGetAccountData<T extends Record<string, any>>(eventType: string): Promise<T> {
+                if (eventType === "m.secret_storage.key.my_key") {
+                    return storedKey as unknown as T;
+                } else {
+                    throw new Error(`unexpected eventType ${eventType}`);
+                }
+            }
+            accountDataAdapter.getAccountDataFromServer.mockImplementation(mockGetAccountData);
+
+            const result = await secretStorage.getKey("my_key");
+            expect(result).toEqual(["my_key", storedKey]);
+        });
+
+        it("should return the default key if none is specified", async function () {
+            const accountDataAdapter = mockAccountDataClient();
+            const secretStorage = new ServerSideSecretStorageImpl(accountDataAdapter, {});
+
+            const storedKey = { iv: "iv", mac: "mac" } as SecretStorageKeyDescriptionAesV1;
+            async function mockGetAccountData<T extends Record<string, any>>(eventType: string): Promise<T> {
+                if (eventType === "m.secret_storage.default_key") {
+                    return { key: "default_key_id" } as unknown as T;
+                } else if (eventType === "m.secret_storage.key.default_key_id") {
+                    return storedKey as unknown as T;
+                } else {
+                    throw new Error(`unexpected eventType ${eventType}`);
+                }
+            }
+            accountDataAdapter.getAccountDataFromServer.mockImplementation(mockGetAccountData);
+
+            const result = await secretStorage.getKey();
+            expect(result).toEqual(["default_key_id", storedKey]);
+        });
+
+        it("should return null if the key is not found", async function () {
+            const accountDataAdapter = mockAccountDataClient();
+            const secretStorage = new ServerSideSecretStorageImpl(accountDataAdapter, {});
+            // @ts-ignore
+            accountDataAdapter.getAccountDataFromServer.mockResolvedValue(null);
+
+            const result = await secretStorage.getKey("my_key");
+            expect(result).toEqual(null);
+        });
+    });
+
+    describe("checkKey", function () {
+        it("should return true for a correct key check", async function () {
+            const secretStorage = new ServerSideSecretStorageImpl({} as AccountDataClient, {});
+
+            const myKey = new TextEncoder().encode(randomString(32));
+            const { iv, mac } = await calculateKeyCheck(myKey);
+
+            const keyInfo: SecretStorageKeyDescriptionAesV1 = {
+                name: "my key",
+                passphrase: {} as PassphraseInfo,
+                algorithm: "m.secret_storage.v1.aes-hmac-sha2",
+                iv,
+                mac,
+            };
+
+            const result = await secretStorage.checkKey(myKey, keyInfo);
+            expect(result).toBe(true);
+        });
+
+        it("should return false for an incorrect key check", async function () {
+            const secretStorage = new ServerSideSecretStorageImpl({} as AccountDataClient, {});
+
+            const { iv, mac } = await calculateKeyCheck(new TextEncoder().encode("badkey"));
+
+            const keyInfo: SecretStorageKeyDescriptionAesV1 = {
+                name: "my key",
+                passphrase: {} as PassphraseInfo,
+                algorithm: "m.secret_storage.v1.aes-hmac-sha2",
+                iv,
+                mac,
+            };
+
+            const result = await secretStorage.checkKey(new TextEncoder().encode("goodkey"), keyInfo);
+            expect(result).toBe(false);
+        });
+
+        it("should raise for an unknown algorithm", async function () {
+            const secretStorage = new ServerSideSecretStorageImpl({} as AccountDataClient, {});
+            const keyInfo: SecretStorageKeyDescriptionAesV1 = {
+                name: "my key",
+                passphrase: {} as PassphraseInfo,
+                algorithm: "bad_alg",
+                iv: "iv",
+                mac: "mac",
+            };
+
+            await expect(() => secretStorage.checkKey(new TextEncoder().encode("goodkey"), keyInfo)).rejects.toThrow(
+                "Unknown algorithm",
+            );
+        });
+
+        // XXX: really???
+        it("should return true for an absent mac", async function () {
+            const secretStorage = new ServerSideSecretStorageImpl({} as AccountDataClient, {});
+            const keyInfo: SecretStorageKeyDescriptionAesV1 = {
+                name: "my key",
+                passphrase: {} as PassphraseInfo,
+                algorithm: "m.secret_storage.v1.aes-hmac-sha2",
+                iv: "iv",
+                mac: "",
+            };
+
+            const result = await secretStorage.checkKey(new TextEncoder().encode("goodkey"), keyInfo);
+            expect(result).toBe(true);
+        });
+    });
+
+    describe("store", () => {
+        it("should ignore keys with unknown algorithm", async function () {
+            const accountDataAdapter = mockAccountDataClient();
+            const mockCallbacks = { getSecretStorageKey: jest.fn() } as Mocked<SecretStorageCallbacks>;
+            const secretStorage = new ServerSideSecretStorageImpl(accountDataAdapter, mockCallbacks);
+
+            // stub out getAccountData to return a key with an unknown algorithm
+            const storedKey = { algorithm: "badalg" } as SecretStorageKeyDescriptionCommon;
+            async function mockGetAccountData<T extends Record<string, any>>(eventType: string): Promise<T> {
+                if (eventType === "m.secret_storage.key.keyid") {
+                    return storedKey as unknown as T;
+                } else {
+                    throw new Error(`unexpected eventType ${eventType}`);
+                }
+            }
+            accountDataAdapter.getAccountDataFromServer.mockImplementation(mockGetAccountData);
+
+            // suppress the expected warning on the console
+            jest.spyOn(console, "warn").mockImplementation();
+
+            // now attempt the store
+            await secretStorage.store("mysecret", "supersecret", ["keyid"]);
+
+            // we should have stored... nothing
+            expect(accountDataAdapter.setAccountData).toHaveBeenCalledWith("mysecret", { encrypted: {} });
+
+            // ... and emitted a warning.
+            // eslint-disable-next-line no-console
+            expect(console.warn).toHaveBeenCalledWith(expect.stringContaining("unknown algorithm"));
+        });
+    });
+});
+
+describe("trimTrailingEquals", () => {
+    it("should strip trailing =", () => {
+        expect(trimTrailingEquals("ab=c===")).toEqual("ab=c");
+    });
+
+    it("should leave strings without trailing = alone", () => {
+        expect(trimTrailingEquals("ab=c")).toEqual("ab=c");
+    });
+
+    it("should leave the empty string alone", () => {
+        const result = trimTrailingEquals("");
+        expect(result).toEqual("");
+    });
+});
+
+function mockAccountDataClient(): Mocked<AccountDataClient> {
+    return {
+        getAccountDataFromServer: jest.fn().mockResolvedValue(null),
+        setAccountData: jest.fn().mockResolvedValue({}),
+    } as unknown as Mocked<AccountDataClient>;
+}

src/crypto/CrossSigning.ts

@@ -25,13 +25,12 @@ import { logger } from "../logger";
 import { IndexedDBCryptoStore } from "../crypto/store/indexeddb-crypto-store";
 import { decryptAES, encryptAES } from "./aes";
 import { DeviceInfo } from "./deviceinfo";
-import { SecretStorage } from "./SecretStorage";
 import { ICrossSigningKey, ISignedKey, MatrixClient } from "../client";
 import { OlmDevice } from "./OlmDevice";
 import { ICryptoCallbacks } from ".";
 import { ISignatures } from "../@types/signed";
 import { CryptoStore, SecretStorePrivateKeys } from "./store/base";
-import { SecretStorageKeyDescription } from "../secret-storage";
+import { ServerSideSecretStorage, SecretStorageKeyDescription } from "../secret-storage";
 
 const KEY_REQUEST_TIMEOUT_MS = 1000 * 60;
 
@@ -164,7 +163,7 @@ export class CrossSigningInfo {
      *     key
      */
     public async isStoredInSecretStorage(
-        secretStorage: SecretStorage<MatrixClient | undefined>,
+        secretStorage: ServerSideSecretStorage,
     ): Promise<Record<string, object> | null> {
         // check what SSSS keys have encrypted the master key (if any)
         const stored = (await secretStorage.isStored("m.cross_signing.master")) || {};
@@ -192,7 +191,7 @@ export class CrossSigningInfo {
      */
     public static async storeInSecretStorage(
         keys: Map<string, Uint8Array>,
-        secretStorage: SecretStorage<undefined>,
+        secretStorage: ServerSideSecretStorage,
     ): Promise<void> {
         for (const [type, privateKey] of keys) {
             const encodedKey = encodeBase64(privateKey);
@@ -209,7 +208,10 @@ export class CrossSigningInfo {
      * @param secretStorage - The secret store using account data
      * @returns The private key
      */
-    public static async getFromSecretStorage(type: string, secretStorage: SecretStorage): Promise<Uint8Array | null> {
+    public static async getFromSecretStorage(
+        type: string,
+        secretStorage: ServerSideSecretStorage,
+    ): Promise<Uint8Array | null> {
         const encodedKey = await secretStorage.get(`m.cross_signing.${type}`);
         if (!encodedKey) {
             return null;