From ceb4581f9193b4edc64fef8ec3d93733a9dd58cc Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Fri, 17 Jan 2020 17:56:01 +0000 Subject: [PATCH] Convert secret storage to new account data API This converts all secret storage to use a newer account data API which uses cached data in stored when available, but also knows how to ask the homeserver in case it's invoked during early client startup before the initial sync. As a consequence, it means most secret storage APIs are now async. Part of https://github.com/vector-im/riot-web/issues/11901 --- spec/unit/crypto/secrets.spec.js | 22 +++++---- src/client.js | 27 +++++++++-- src/crypto/CrossSigning.js | 4 +- src/crypto/SecretStorage.js | 82 +++++++++++++++----------------- src/crypto/index.js | 11 ++--- 5 files changed, 84 insertions(+), 62 deletions(-) diff --git a/spec/unit/crypto/secrets.spec.js b/spec/unit/crypto/secrets.spec.js index 254adf0c0..f72b2bb09 100644 --- a/spec/unit/crypto/secrets.spec.js +++ b/spec/unit/crypto/secrets.spec.js @@ -26,6 +26,12 @@ async function makeTestClient(userInfo, options) { userInfo.userId, userInfo.deviceId, undefined, undefined, options, )).client; + // Make it seem as if we've synced and thus the store can be trusted to + // contain valid account data. + client.isInitialSyncComplete = function() { + return true; + }; + await client.initCrypto(); return client; @@ -103,11 +109,11 @@ describe("Secrets", function() { }), ]); - expect(secretStorage.isStored("foo")).toBe(false); + expect(await secretStorage.isStored("foo")).toBe(false); await secretStorage.store("foo", "bar", ["abc"]); - expect(secretStorage.isStored("foo")).toBe(true); + expect(await secretStorage.isStored("foo")).toBe(true); expect(await secretStorage.get("foo")).toBe("bar"); expect(getKey).toHaveBeenCalled(); @@ -268,8 +274,8 @@ describe("Secrets", function() { const secretStorage = bob._crypto._secretStorage; expect(crossSigning.getId()).toBeTruthy(); - expect(crossSigning.isStoredInSecretStorage(secretStorage)).toBeTruthy(); - expect(secretStorage.hasKey()).toBeTruthy(); + expect(await crossSigning.isStoredInSecretStorage(secretStorage)).toBeTruthy(); + expect(await secretStorage.hasKey()).toBeTruthy(); }); it("bootstraps when cross-signing keys in secret storage", async function() { @@ -284,8 +290,8 @@ describe("Secrets", function() { }, { cryptoCallbacks: { - getSecretStorageKey: request => { - const defaultKeyId = bob.getDefaultSecretStorageKeyId(); + getSecretStorageKey: async request => { + const defaultKeyId = await bob.getDefaultSecretStorageKeyId(); expect(Object.keys(request.keys)).toEqual([defaultKeyId]); return [defaultKeyId, storagePrivateKey]; }, @@ -324,7 +330,7 @@ describe("Secrets", function() { await bob.bootstrapSecretStorage(); expect(crossSigning.getId()).toBeTruthy(); - expect(crossSigning.isStoredInSecretStorage(secretStorage)).toBeTruthy(); - expect(secretStorage.hasKey()).toBeTruthy(); + expect(await crossSigning.isStoredInSecretStorage(secretStorage)).toBeTruthy(); + expect(await secretStorage.hasKey()).toBeTruthy(); }); }); diff --git a/src/client.js b/src/client.js index 8df4157fc..b556e962d 100644 --- a/src/client.js +++ b/src/client.js @@ -498,6 +498,18 @@ MatrixClient.prototype.getSyncStateData = function() { return this._syncApi.getSyncStateData(); }; +/** + * Whether the initial sync has completed. + * @return {boolean} True if at least on sync has happened. + */ +MatrixClient.prototype.isInitialSyncComplete = function() { + const state = this.getSyncState(); + if (!state) { + return false; + } + return state === "PREPAED" || state === "SYNCING"; +}; + /** * Return whether the client is configured for a guest account. * @return {boolean} True if this is a guest access_token (or no token is supplied). @@ -1944,14 +1956,23 @@ MatrixClient.prototype.getAccountData = function(eventType) { /** * Get account data event of given type for the current user. This variant - * bypasses the local store and gets account data directly from the homeserver, - * which can be useful very early in startup before the initial sync. + * gets account data directly from the homeserver if the local store is not + * ready, which can be useful very early in startup before the initial sync. * @param {string} eventType The event type * @return {module:client.Promise} Resolves: The contents of the given account * data event. * @return {module:http-api.MatrixError} Rejects: with an error response. */ -MatrixClient.prototype.getAccountDataFromServer = function(eventType) { +MatrixClient.prototype.getAccountDataFromServer = async function(eventType) { + if (this.isInitialSyncComplete()) { + const event = this.store.getAccountData(eventType); + if (!event) { + return null; + } + // The network version below returns just the content, so this branch + // does the same to match. + return event.getContent(); + } const path = utils.encodeUri("/user/$userId/account_data/$type", { $userId: this.credentials.userId, $type: eventType, diff --git a/src/crypto/CrossSigning.js b/src/crypto/CrossSigning.js index b37fa6d43..dc99a3c19 100644 --- a/src/crypto/CrossSigning.js +++ b/src/crypto/CrossSigning.js @@ -113,10 +113,10 @@ export class CrossSigningInfo extends EventEmitter { * @param {SecretStorage} secretStorage The secret store using account data * @returns {boolean} Whether all private keys were found in storage */ - isStoredInSecretStorage(secretStorage) { + async isStoredInSecretStorage(secretStorage) { let stored = true; for (const type of ["master", "self_signing", "user_signing"]) { - stored &= secretStorage.isStored(`m.cross_signing.${type}`, false); + stored &= await secretStorage.isStored(`m.cross_signing.${type}`, false); } return stored; } diff --git a/src/crypto/SecretStorage.js b/src/crypto/SecretStorage.js index 584c5c4c0..54e189157 100644 --- a/src/crypto/SecretStorage.js +++ b/src/crypto/SecretStorage.js @@ -36,12 +36,12 @@ export class SecretStorage extends EventEmitter { this._incomingRequests = {}; } - getDefaultKeyId() { - const defaultKeyEvent = this._baseApis.getAccountData( + async getDefaultKeyId() { + const defaultKey = await this._baseApis.getAccountDataFromServer( 'm.secret_storage.default_key', ); - if (!defaultKeyEvent) return null; - return defaultKeyEvent.getContent().key; + if (!defaultKey) return null; + return defaultKey.key; } setDefaultKeyId(keyId) { @@ -112,7 +112,11 @@ export class SecretStorage extends EventEmitter { if (!keyId) { do { keyId = randomString(32); - } while (this._baseApis.getAccountData(`m.secret_storage.key.${keyId}`)); + } while ( + await this._baseApis.getAccountDataFromServer( + `m.secret_storage.key.${keyId}`, + ) + ); } await this._crossSigningInfo.signObject(keyData, 'master'); @@ -130,18 +134,20 @@ export class SecretStorage extends EventEmitter { * @param {string} [keyId = default key's ID] The ID of the key to sign. * Defaults to the default key ID if not provided. */ - async signKey(keyId = this.getDefaultKeyId()) { + async signKey(keyId) { + if (!keyId) { + keyId = await this.getDefaultKeyId(); + } if (!keyId) { throw new Error("signKey requires a key ID"); } - const keyInfoEvent = this._baseApis.getAccountData( + const keyInfo = await this._baseApis.getAccountDataFromServer( `m.secret_storage.key.${keyId}`, ); - if (!keyInfoEvent) { + if (!keyInfo) { throw new Error(`Key ${keyId} does not exist in account data`); } - const keyInfo = keyInfoEvent.getContent(); await this._crossSigningInfo.signObject(keyInfo, 'master'); await this._baseApis.setAccountData( @@ -156,15 +162,17 @@ export class SecretStorage extends EventEmitter { * for. Defaults to the default key ID if not provided. * @return {boolean} Whether we have the key. */ - hasKey(keyId = this.getDefaultKeyId()) { + async hasKey(keyId) { + if (!keyId) { + keyId = await this.getDefaultKeyId(); + } if (!keyId) { return false; } - const keyInfo = this._baseApis.getAccountData( + return !!this._baseApis.getAccountDataFromServer( "m.secret_storage.key." + keyId, ); - return keyInfo && keyInfo.getContent(); } /** @@ -179,7 +187,7 @@ export class SecretStorage extends EventEmitter { const encrypted = {}; if (!keys) { - const defaultKeyId = this.getDefaultKeyId(); + const defaultKeyId = await this.getDefaultKeyId(); if (!defaultKeyId) { throw new Error("No keys specified and no default key present"); } @@ -192,28 +200,27 @@ export class SecretStorage extends EventEmitter { for (const keyId of keys) { // get key information from key storage - const keyInfo = this._baseApis.getAccountData( + const keyInfo = await this._baseApis.getAccountDataFromServer( "m.secret_storage.key." + keyId, ); if (!keyInfo) { throw new Error("Unknown key: " + keyId); } - const keyInfoContent = keyInfo.getContent(); // check signature of key info pkVerify( - keyInfoContent, + keyInfo, this._crossSigningInfo.getId('master'), this._crossSigningInfo.userId, ); // encrypt secret, based on the algorithm - switch (keyInfoContent.algorithm) { + switch (keyInfo.algorithm) { case SECRET_STORAGE_ALGORITHM_V1: { const encryption = new global.Olm.PkEncryption(); try { - encryption.set_recipient_key(keyInfoContent.pubkey); + encryption.set_recipient_key(keyInfo.pubkey); encrypted[keyId] = encryption.encrypt(secret); } finally { encryption.free(); @@ -222,7 +229,7 @@ export class SecretStorage extends EventEmitter { } default: logger.warn("unknown algorithm for secret storage key " + keyId - + ": " + keyInfoContent.algorithm); + + ": " + keyInfo.algorithm); // do nothing if we don't understand the encryption algorithm } } @@ -260,25 +267,22 @@ export class SecretStorage extends EventEmitter { * @return {string} the contents of the secret */ async get(name) { - const secretInfo = this._baseApis.getAccountData(name); + const secretInfo = await this._baseApis.getAccountDataFromServer(name); if (!secretInfo) { return; } - - const secretContent = secretInfo.getContent(); - - if (!secretContent.encrypted) { + if (!secretInfo.encrypted) { throw new Error("Content is not encrypted!"); } // get possible keys to decrypt const keys = {}; - for (const keyId of Object.keys(secretContent.encrypted)) { + for (const keyId of Object.keys(secretInfo.encrypted)) { // get key information from key storage - const keyInfo = this._baseApis.getAccountData( + const keyInfo = await this._baseApis.getAccountDataFromServer( "m.secret_storage.key." + keyId, - ).getContent(); - const encInfo = secretContent.encrypted[keyId]; + ); + const encInfo = secretInfo.encrypted[keyId]; switch (keyInfo.algorithm) { case SECRET_STORAGE_ALGORITHM_V1: if (keyInfo.pubkey && encInfo.ciphertext && encInfo.mac @@ -297,7 +301,7 @@ export class SecretStorage extends EventEmitter { // fetch private key from app [keyId, decryption] = await this._getSecretStorageKey(keys); - const encInfo = secretContent.encrypted[keyId]; + const encInfo = secretInfo.encrypted[keyId]; // We don't actually need the decryption object if it's a passthrough // since we just want to return the key itself. @@ -323,32 +327,24 @@ export class SecretStorage extends EventEmitter { * * @return {boolean} whether or not the secret is stored */ - isStored(name, checkKey) { + async isStored(name, checkKey) { // check if secret exists - const secretInfo = this._baseApis.getAccountData(name); - if (!secretInfo) { + const secretInfo = await this._baseApis.getAccountDataFromServer(name); + if (!secretInfo || !secretInfo.encrypted) { return false; } if (checkKey === undefined) checkKey = true; - const secretContent = secretInfo.getContent(); - - if (!secretContent.encrypted) { - return false; - } - // check if secret is encrypted by a known/trusted secret and // encryption looks sane - for (const keyId of Object.keys(secretContent.encrypted)) { + for (const keyId of Object.keys(secretInfo.encrypted)) { // get key information from key storage - const keyEvent = this._baseApis.getAccountData( + const keyInfo = await this._baseApis.getAccountDataFromServer( "m.secret_storage.key." + keyId, ); - if (!keyEvent) return false; - const keyInfo = keyEvent.getContent(); if (!keyInfo) return false; - const encInfo = secretContent.encrypted[keyId]; + const encInfo = secretInfo.encrypted[keyId]; if (checkKey) { pkVerify( keyInfo, diff --git a/src/crypto/index.js b/src/crypto/index.js index bab0a2a2f..2369d58c1 100644 --- a/src/crypto/index.js +++ b/src/crypto/index.js @@ -359,15 +359,14 @@ Crypto.prototype.bootstrapSecretStorage = async function({ const appCallbacks = Object.assign({}, this._baseApis._cryptoCallbacks); try { - if ( - !this._crossSigningInfo.getId() || - !this._crossSigningInfo.isStoredInSecretStorage(this._secretStorage) - ) { + const inStorage = + await this._crossSigningInfo.isStoredInSecretStorage(this._secretStorage); + if (!this._crossSigningInfo.getId() || !inStorage) { logger.log( "Cross-signing public and/or private keys not found, " + "checking secret storage for private keys", ); - if (this._crossSigningInfo.isStoredInSecretStorage(this._secretStorage)) { + if (inStorage) { logger.log("Cross-signing private keys found in secret storage"); await this.checkOwnCrossSigningTrust(); } else { @@ -390,7 +389,7 @@ Crypto.prototype.bootstrapSecretStorage = async function({ // Check if Secure Secret Storage has a default key. If we don't have one, create // the default key (which will also be signed by the cross-signing master key). - if (!this.hasSecretStorageKey()) { + if (!await this.hasSecretStorageKey()) { let newKeyId; if (keyBackupInfo) { logger.log("Secret storage default key not found, using key backup key");