From 1e1b571b2865c4bd72bf5a34ce7b34d6ef91a914 Mon Sep 17 00:00:00 2001
From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
Date: Thu, 13 Apr 2023 17:21:38 +0100
Subject: [PATCH] Expose ServerSideSecretStorage independently of Crypto
 (#3280)

There is no reason to indirect secret storage via the Crypto layer, and
exposing it directly means it will work for Element-R.

Fixes: https://github.com/vector-im/element-web/issues/24982
---
 spec/unit/crypto.spec.ts        |  1 -
 spec/unit/matrix-client.spec.ts | 41 +++++++++++++++-
 src/client.ts                   | 87 +++++++++++++++++++--------------
 src/crypto/index.ts             | 35 +++++++++++--
 src/secret-storage.ts           |  3 +-
 5 files changed, 122 insertions(+), 45 deletions(-)

diff --git a/spec/unit/crypto.spec.ts b/spec/unit/crypto.spec.ts
index 50c9aa854..e62bb2eac 100644
--- a/spec/unit/crypto.spec.ts
+++ b/spec/unit/crypto.spec.ts
@@ -1011,7 +1011,6 @@ describe("Crypto", function () {
             jest.setTimeout(10000);
             const client = new TestClient("@a:example.com", "dev").client;
             await client.initCrypto();
-            client.crypto!.getSecretStorageKey = jest.fn().mockResolvedValue(null);
             client.crypto!.isCrossSigningReady = async () => false;
             client.crypto!.baseApis.uploadDeviceSigningKeys = jest.fn().mockResolvedValue(null);
             client.crypto!.baseApis.setAccountData = jest.fn().mockResolvedValue(null);
diff --git a/spec/unit/matrix-client.spec.ts b/spec/unit/matrix-client.spec.ts
index c8139a717..331f80eed 100644
--- a/spec/unit/matrix-client.spec.ts
+++ b/spec/unit/matrix-client.spec.ts
@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-import { mocked } from "jest-mock";
+import { Mocked, mocked } from "jest-mock";
 
 import { logger } from "../../src/logger";
 import { ClientEvent, IMatrixClientCreateOpts, ITurnServerResponse, MatrixClient, Store } from "../../src/client";
@@ -63,6 +63,7 @@ import { QueryDict } from "../../src/utils";
 import { SyncState } from "../../src/sync";
 import * as featureUtils from "../../src/feature";
 import { StubStore } from "../../src/store/stub";
+import { SecretStorageKeyDescriptionAesV1, ServerSideSecretStorageImpl } from "../../src/secret-storage";
 
 jest.useFakeTimers();
 
@@ -2704,4 +2705,42 @@ describe("MatrixClient", function () {
             });
         });
     });
+
+    // these wrappers are deprecated, but we need coverage of them to pass the quality gate
+    describe("SecretStorage wrappers", () => {
+        let mockSecretStorage: Mocked<ServerSideSecretStorageImpl>;
+
+        beforeEach(() => {
+            mockSecretStorage = {
+                getDefaultKeyId: jest.fn(),
+                hasKey: jest.fn(),
+                isStored: jest.fn(),
+            } as unknown as Mocked<ServerSideSecretStorageImpl>;
+            client["_secretStorage"] = mockSecretStorage;
+        });
+
+        it("hasSecretStorageKey", async () => {
+            mockSecretStorage.hasKey.mockResolvedValue(false);
+            expect(await client.hasSecretStorageKey("mykey")).toBe(false);
+            expect(mockSecretStorage.hasKey).toHaveBeenCalledWith("mykey");
+        });
+
+        it("isSecretStored", async () => {
+            const mockResult = { key: {} as SecretStorageKeyDescriptionAesV1 };
+            mockSecretStorage.isStored.mockResolvedValue(mockResult);
+            expect(await client.isSecretStored("mysecret")).toBe(mockResult);
+            expect(mockSecretStorage.isStored).toHaveBeenCalledWith("mysecret");
+        });
+
+        it("getDefaultSecretStorageKeyId", async () => {
+            mockSecretStorage.getDefaultKeyId.mockResolvedValue("bzz");
+            expect(await client.getDefaultSecretStorageKeyId()).toEqual("bzz");
+        });
+
+        it("isKeyBackupKeyStored", async () => {
+            mockSecretStorage.isStored.mockResolvedValue(null);
+            expect(await client.isKeyBackupKeyStored()).toBe(null);
+            expect(mockSecretStorage.isStored).toHaveBeenCalledWith("m.megolm_backup.v1");
+        });
+    });
 });
diff --git a/src/client.ts b/src/client.ts
index 309817f87..66765b092 100644
--- a/src/client.ts
+++ b/src/client.ts
@@ -206,7 +206,12 @@ import { CryptoBackend } from "./common-crypto/CryptoBackend";
 import { RUST_SDK_STORE_PREFIX } from "./rust-crypto/constants";
 import { CryptoApi } from "./crypto-api";
 import { DeviceInfoMap } from "./crypto/DeviceList";
-import { AddSecretStorageKeyOpts, SecretStorageKeyDescription } from "./secret-storage";
+import {
+    AddSecretStorageKeyOpts,
+    SecretStorageKeyDescription,
+    ServerSideSecretStorage,
+    ServerSideSecretStorageImpl,
+} from "./secret-storage";
 
 export type Store = IStore;
 
@@ -1252,6 +1257,8 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
     private useE2eForGroupCall = true;
     private toDeviceMessageQueue: ToDeviceMessageQueue;
 
+    private _secretStorage: ServerSideSecretStorageImpl;
+
     // A manager for determining which invites should be ignored.
     public readonly ignoredInvites: IgnoredInvites;
 
@@ -1417,6 +1424,7 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
         });
 
         this.ignoredInvites = new IgnoredInvites(this);
+        this._secretStorage = new ServerSideSecretStorageImpl(this, opts.cryptoCallbacks ?? {});
     }
 
     /**
@@ -2226,6 +2234,13 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
         this.on(RoomMemberEvent.Membership, rustCrypto.onRoomMembership.bind(rustCrypto));
     }
 
+    /**
+     * Access the server-side secret storage API for this client.
+     */
+    public get secretStorage(): ServerSideSecretStorage {
+        return this._secretStorage;
+    }
+
     /**
      * Access the crypto API for this client.
      *
@@ -2472,11 +2487,11 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
         return this.crypto.beginKeyVerification(method, userId, deviceId);
     }
 
+    /**
+     * @deprecated Use {@link MatrixClient#secretStorage} and {@link SecretStorage.ServerSideSecretStorage#checkKey}.
+     */
     public checkSecretStorageKey(key: Uint8Array, info: SecretStorageKeyDescription): Promise<boolean> {
-        if (!this.crypto) {
-            throw new Error("End-to-end encryption disabled");
-        }
-        return this.crypto.checkSecretStorageKey(key, info);
+        return this.secretStorage.checkKey(key, info);
     }
 
     /**
@@ -2867,16 +2882,15 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
      * @returns An object with:
      *     keyId: the ID of the key
      *     keyInfo: details about the key (iv, mac, passphrase)
+     *
+     * @deprecated Use {@link MatrixClient#secretStorage} and {@link SecretStorage.ServerSideSecretStorage#addKey}.
      */
     public addSecretStorageKey(
         algorithm: string,
         opts: AddSecretStorageKeyOpts,
         keyName?: string,
     ): Promise<{ keyId: string; keyInfo: SecretStorageKeyDescription }> {
-        if (!this.crypto) {
-            throw new Error("End-to-end encryption disabled");
-        }
-        return this.crypto.addSecretStorageKey(algorithm, opts, keyName);
+        return this.secretStorage.addKey(algorithm, opts, keyName);
     }
 
     /**
@@ -2887,12 +2901,11 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
      * @param keyId - The ID of the key to check
      *     for. Defaults to the default key ID if not provided.
      * @returns Whether we have the key.
+     *
+     * @deprecated Use {@link MatrixClient#secretStorage} and {@link SecretStorage.ServerSideSecretStorage#hasKey}.
      */
     public hasSecretStorageKey(keyId?: string): Promise<boolean> {
-        if (!this.crypto) {
-            throw new Error("End-to-end encryption disabled");
-        }
-        return this.crypto.hasSecretStorageKey(keyId);
+        return this.secretStorage.hasKey(keyId);
     }
 
     /**
@@ -2904,12 +2917,11 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
      * @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 (will throw if no default key is set).
+     *
+     * @deprecated Use {@link MatrixClient#secretStorage} and {@link SecretStorage.ServerSideSecretStorage#store}.
      */
     public storeSecret(name: string, secret: string, keys?: string[]): Promise<void> {
-        if (!this.crypto) {
-            throw new Error("End-to-end encryption disabled");
-        }
-        return this.crypto.storeSecret(name, secret, keys);
+        return this.secretStorage.store(name, secret, keys);
     }
 
     /**
@@ -2920,12 +2932,11 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
      * @param name - the name of the secret
      *
      * @returns the contents of the secret
+     *
+     * @deprecated Use {@link MatrixClient#secretStorage} and {@link SecretStorage.ServerSideSecretStorage#get}.
      */
     public getSecret(name: string): Promise<string | undefined> {
-        if (!this.crypto) {
-            throw new Error("End-to-end encryption disabled");
-        }
-        return this.crypto.getSecret(name);
+        return this.secretStorage.get(name);
     }
 
     /**
@@ -2937,12 +2948,11 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
      * @returns map of key name to key info the secret is encrypted
      *     with, or null if it is not present or not encrypted with a trusted
      *     key
+     *
+     * @deprecated Use {@link MatrixClient#secretStorage} and {@link SecretStorage.ServerSideSecretStorage#isStored}.
      */
     public isSecretStored(name: string): Promise<Record<string, SecretStorageKeyDescription> | null> {
-        if (!this.crypto) {
-            throw new Error("End-to-end encryption disabled");
-        }
-        return this.crypto.isSecretStored(name);
+        return this.secretStorage.isStored(name);
     }
 
     /**
@@ -2968,12 +2978,11 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
      * The Secure Secret Storage API is currently UNSTABLE and may change without notice.
      *
      * @returns The default key ID or null if no default key ID is set
+     *
+     * @deprecated Use {@link MatrixClient#secretStorage} and {@link SecretStorage.ServerSideSecretStorage#getDefaultKeyId}.
      */
     public getDefaultSecretStorageKeyId(): Promise<string | null> {
-        if (!this.crypto) {
-            throw new Error("End-to-end encryption disabled");
-        }
-        return this.crypto.getDefaultSecretStorageKeyId();
+        return this.secretStorage.getDefaultKeyId();
     }
 
     /**
@@ -2982,12 +2991,11 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
      * The Secure Secret Storage API is currently UNSTABLE and may change without notice.
      *
      * @param keyId - The new default key ID
+     *
+     * @deprecated Use {@link MatrixClient#secretStorage} and {@link SecretStorage.ServerSideSecretStorage#setDefaultKeyId}.
      */
     public setDefaultSecretStorageKeyId(keyId: string): Promise<void> {
-        if (!this.crypto) {
-            throw new Error("End-to-end encryption disabled");
-        }
-        return this.crypto.setDefaultSecretStorageKeyId(keyId);
+        return this.secretStorage.setDefaultKeyId(keyId);
     }
 
     /**
@@ -3000,6 +3008,9 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
      * @param privateKey - The private key
      * @param expectedPublicKey - The public key
      * @returns true if the key matches, otherwise false
+     *
+     * @deprecated The use of asymmetric keys for SSSS is deprecated.
+     *     Use {@link SecretStorage.ServerSideSecretStorage#checkKey} for symmetric keys.
      */
     public checkSecretStoragePrivateKey(privateKey: Uint8Array, expectedPublicKey: string): boolean {
         if (!this.crypto) {
@@ -3296,7 +3307,7 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
             await this.crypto.backupManager.prepareKeyBackupVersion(password);
 
         if (opts.secureSecretStorage) {
-            await this.storeSecret("m.megolm_backup.v1", encodeBase64(privateKey));
+            await this.secretStorage.store("m.megolm_backup.v1", encodeBase64(privateKey));
             logger.info("Key backup private key stored in secret storage");
         }
 
@@ -3316,7 +3327,7 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
      *     trusted key
      */
     public isKeyBackupKeyStored(): Promise<Record<string, SecretStorageKeyDescription> | null> {
-        return Promise.resolve(this.isSecretStored("m.megolm_backup.v1"));
+        return Promise.resolve(this.secretStorage.isStored("m.megolm_backup.v1"));
     }
 
     /**
@@ -3581,14 +3592,14 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
         if (!this.crypto) {
             throw new Error("End-to-end encryption disabled");
         }
-        const storedKey = await this.getSecret("m.megolm_backup.v1");
+        const storedKey = await this.secretStorage.get("m.megolm_backup.v1");
 
         // ensure that the key is in the right format.  If not, fix the key and
         // store the fixed version
         const fixedKey = fixBackupKey(storedKey);
         if (fixedKey) {
-            const keys = await this.crypto.getSecretStorageKey();
-            await this.storeSecret("m.megolm_backup.v1", fixedKey, [keys![0]]);
+            const keys = await this.secretStorage.getKey();
+            await this.secretStorage.store("m.megolm_backup.v1", fixedKey, [keys![0]]);
         }
 
         const privKey = decodeBase64(fixedKey || storedKey!);
diff --git a/src/crypto/index.ts b/src/crypto/index.ts
index 6d860c6d2..7ba1726b2 100644
--- a/src/crypto/index.ts
+++ b/src/crypto/index.ts
@@ -476,15 +476,15 @@ export class Crypto extends TypedEventEmitter<CryptoEvent, CryptoEventHandlerMap
             }
 
             // try to get key from secret storage
-            const storedKey = await this.getSecret("m.megolm_backup.v1");
+            const storedKey = await this.secretStorage.get("m.megolm_backup.v1");
 
             if (storedKey) {
                 // ensure that the key is in the right format.  If not, fix the key and
                 // store the fixed version
                 const fixedKey = fixBackupKey(storedKey);
                 if (fixedKey) {
-                    const keys = await this.getSecretStorageKey();
-                    await this.storeSecret("m.megolm_backup.v1", fixedKey, [keys![0]]);
+                    const keys = await this.secretStorage.getKey();
+                    await this.secretStorage.store("m.megolm_backup.v1", fixedKey, [keys![0]]);
                 }
 
                 return olmlib.decodeBase64(fixedKey || storedKey);
@@ -950,7 +950,7 @@ export class Crypto extends TypedEventEmitter<CryptoEvent, CryptoEventHandlerMap
             }
         };
 
-        const oldSSSSKey = await this.getSecretStorageKey();
+        const oldSSSSKey = await this.secretStorage.getKey();
         const [oldKeyId, oldKeyInfo] = oldSSSSKey || [null, null];
         const storageExists =
             !setupNewSecretStorage && oldKeyInfo && oldKeyInfo.algorithm === SECRET_STORAGE_ALGORITHM_V1_AES;
@@ -1100,6 +1100,9 @@ export class Crypto extends TypedEventEmitter<CryptoEvent, CryptoEventHandlerMap
         logger.log("Secure Secret Storage ready");
     }
 
+    /**
+     * @deprecated Use {@link MatrixClient#secretStorage} and {@link SecretStorage.ServerSideSecretStorage#addKey}.
+     */
     public addSecretStorageKey(
         algorithm: string,
         opts: AddSecretStorageKeyOpts,
@@ -1108,22 +1111,37 @@ export class Crypto extends TypedEventEmitter<CryptoEvent, CryptoEventHandlerMap
         return this.secretStorage.addKey(algorithm, opts, keyID);
     }
 
+    /**
+     * @deprecated Use {@link MatrixClient#secretStorage} and {@link SecretStorage.ServerSideSecretStorage#hasKey}.
+     */
     public hasSecretStorageKey(keyID?: string): Promise<boolean> {
         return this.secretStorage.hasKey(keyID);
     }
 
+    /**
+     * @deprecated Use {@link MatrixClient#secretStorage} and {@link SecretStorage.ServerSideSecretStorage#getKey}.
+     */
     public getSecretStorageKey(keyID?: string): Promise<SecretStorageKeyTuple | null> {
         return this.secretStorage.getKey(keyID);
     }
 
+    /**
+     * @deprecated Use {@link MatrixClient#secretStorage} and {@link SecretStorage.ServerSideSecretStorage#store}.
+     */
     public storeSecret(name: string, secret: string, keys?: string[]): Promise<void> {
         return this.secretStorage.store(name, secret, keys);
     }
 
+    /**
+     * @deprecated Use {@link MatrixClient#secretStorage} and {@link SecretStorage.ServerSideSecretStorage#get}.
+     */
     public getSecret(name: string): Promise<string | undefined> {
         return this.secretStorage.get(name);
     }
 
+    /**
+     * @deprecated Use {@link MatrixClient#secretStorage} and {@link SecretStorage.ServerSideSecretStorage#isStored}.
+     */
     public isSecretStored(name: string): Promise<Record<string, SecretStorageKeyDescription> | null> {
         return this.secretStorage.isStored(name);
     }
@@ -1135,14 +1153,23 @@ export class Crypto extends TypedEventEmitter<CryptoEvent, CryptoEventHandlerMap
         return this.secretStorage.request(name, devices);
     }
 
+    /**
+     * @deprecated Use {@link MatrixClient#secretStorage} and {@link SecretStorage.ServerSideSecretStorage#getDefaultKeyId}.
+     */
     public getDefaultSecretStorageKeyId(): Promise<string | null> {
         return this.secretStorage.getDefaultKeyId();
     }
 
+    /**
+     * @deprecated Use {@link MatrixClient#secretStorage} and {@link SecretStorage.ServerSideSecretStorage#setDefaultKeyId}.
+     */
     public setDefaultSecretStorageKeyId(k: string): Promise<void> {
         return this.secretStorage.setDefaultKeyId(k);
     }
 
+    /**
+     * @deprecated Use {@link MatrixClient#secretStorage} and {@link SecretStorage.ServerSideSecretStorage#checkKey}.
+     */
     public checkSecretStorageKey(key: Uint8Array, info: SecretStorageKeyDescription): Promise<boolean> {
         return this.secretStorage.checkKey(key, info);
     }
diff --git a/src/secret-storage.ts b/src/secret-storage.ts
index 547015373..6a631415b 100644
--- a/src/secret-storage.ts
+++ b/src/secret-storage.ts
@@ -452,7 +452,8 @@ export class ServerSideSecretStorageImpl implements ServerSideSecretStorage {
      * @returns Whether we have the key.
      */
     public async hasKey(keyId?: string): Promise<boolean> {
-        return Boolean(await this.getKey(keyId));
+        const key = await this.getKey(keyId);
+        return Boolean(key);
     }
 
     /**