Skip to content

Commit

Permalink
Expose ServerSideSecretStorage independently of Crypto (#3280)
Browse files Browse the repository at this point in the history 
There is no reason to indirect secret storage via the Crypto layer, and
exposing it directly means it will work for Element-R.

Fixes: element-hq/element-web#24982
  • Loading branch information
@richvdh
richvdh authored Apr 13, 2023
1 parent f400a7b commit 1e1b571
Show file tree
Hide file tree
Showing 5 changed files with 122 additions and 45 deletions.
1 change: 0 additions & 1 deletion spec/unit/crypto.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
41 changes: 40 additions & 1 deletion spec/unit/matrix-client.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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");
});
});
});
87 changes: 49 additions & 38 deletions src/client.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -1417,6 +1424,7 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
});

this.ignoredInvites = new IgnoredInvites(this);
this._secretStorage = new ServerSideSecretStorageImpl(this, opts.cryptoCallbacks ?? {});
}

/**
Expand Down Expand Up @@ -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.
*
Expand Down Expand Up @@ -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);
}

/**
Expand Down Expand Up @@ -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);
}

/**
Expand All @@ -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);
}

/**
Expand All @@ -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);
}

/**
Expand All @@ -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);
}

/**
Expand All @@ -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);
}

/**
Expand All @@ -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();
}

/**
Expand All @@ -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);
}

/**
Expand All @@ -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) {
Expand Down Expand Up @@ -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");
}

Expand All @@ -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"));
}

/**
Expand Down Expand Up @@ -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!);
Expand Down
35 changes: 31 additions & 4 deletions src/crypto/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand All @@ -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);
}
Expand All @@ -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);
}
Expand Down
Loading

0 comments on commit 1e1b571

Please sign in to comment.