From de8da9ddf67f36977e43d2168c4847fd9923c875 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Tue, 22 Nov 2016 15:54:51 -0800 Subject: [PATCH] Simplify Encryptor API Surface At least, the portion of it that we use. Moved salting within the encryptor, so it does not need to be managed externally. KeyringController now caches the password instead of a passwordDerivedKey, since it is ignorant of the salt. Encryptor payload is now in a JSON format, so its portions are both base64 encoded *and* labeled appropriately. The format is `{ "data": "0x0", "iv": "0x0", "salt": "string" }`. Fixes #843 Fixes #859 --- app/scripts/keyring-controller.js | 50 ++++++++-------------------- app/scripts/lib/encryptor.js | 26 ++++++++++----- test/unit/keyring-controller-test.js | 10 +++--- 3 files changed, 37 insertions(+), 49 deletions(-) diff --git a/app/scripts/keyring-controller.js b/app/scripts/keyring-controller.js index 00c04ea9b..b24c4bac5 100644 --- a/app/scripts/keyring-controller.js +++ b/app/scripts/keyring-controller.js @@ -113,34 +113,25 @@ module.exports = class KeyringController extends EventEmitter { }) } - migrateAndGetKey (password) { - let key + migrateOldVaultIfAny (password) { const shouldMigrate = !!this.configManager.getWallet() && !this.configManager.getVault() - return this.loadKey(password) - .then((derivedKey) => { - key = derivedKey - this.key = key - return this.idStoreMigrator.migratedVaultForPassword(password) - }) + return this.idStoreMigrator.migratedVaultForPassword(password) .then((serialized) => { if (serialized && shouldMigrate) { const keyring = this.restoreKeyring(serialized) this.keyrings.push(keyring) this.configManager.setSelectedAccount(keyring.getAccounts()[0]) return this.persistAllKeyrings() - .then(() => { return key }) + .then(() => { return }) } - return key + return }) } createNewVault (password, cb) { - const configManager = this.configManager - const salt = this.getSalt() - configManager.setSalt(salt) - - return this.migrateAndGetKey(password) + return this.migrateOldVaultIfAny(password) .then(() => { + this.password = password return this.persistAllKeyrings() }) .then(() => { @@ -184,8 +175,8 @@ module.exports = class KeyringController extends EventEmitter { } submitPassword (password, cb) { - this.migrateAndGetKey(password) - .then((key) => { + this.migrateOldVaultIfAny(password) + .then(() => { return this.unlockKeyrings(password) }) .then((keyrings) => { @@ -200,21 +191,6 @@ module.exports = class KeyringController extends EventEmitter { }) } - loadKey (password) { - const salt = this.getSalt() - return this.encryptor.keyFromPassword(password + salt) - .then((key) => { - this.key = key - this.configManager.setSalt(salt) - return key - }) - } - - getSalt () { - const vault = this.configManager.getVault() - const salt = vault.salt || this.encryptor.generateSalt() - } - addNewKeyring (type, opts, cb) { const Keyring = this.getKeyringClassForType(type) const keyring = new Keyring(opts) @@ -290,7 +266,7 @@ module.exports = class KeyringController extends EventEmitter { data: keyring.serialize(), } }) - return this.encryptor.encryptWithKey(this.key, serialized) + return this.encryptor.encrypt(this.password, serialized) .then((encryptedString) => { this.configManager.setVault(encryptedString) return true @@ -299,7 +275,7 @@ module.exports = class KeyringController extends EventEmitter { unlockKeyrings (password) { const encryptedVault = this.configManager.getVault() - return this.encryptor.decryptWithKey(key, encryptedVault) + return this.encryptor.decrypt(this.password, encryptedVault) .then((vault) => { vault.forEach(this.restoreKeyring.bind(this)) return this.keyrings @@ -407,7 +383,7 @@ module.exports = class KeyringController extends EventEmitter { }) } - function estimateGas(txData, blockGasLimitHex, cb) { + function estimateGas (txData, blockGasLimitHex, cb) { const txParams = txData.txParams // check if gasLimit is already specified txData.gasLimitSpecified = Boolean(txParams.gas) @@ -419,7 +395,7 @@ module.exports = class KeyringController extends EventEmitter { query.estimateGas(txParams, cb) } - function checkForGasError(txData, estimatedGasHex) { + function checkForGasError (txData, estimatedGasHex) { txData.estimatedGas = estimatedGasHex // all gas used - must be an error if (estimatedGasHex === txData.txParams.gas) { @@ -428,7 +404,7 @@ module.exports = class KeyringController extends EventEmitter { cb() } - function setTxGas(txData, blockGasLimitHex) { + function setTxGas (txData, blockGasLimitHex) { const txParams = txData.txParams // if OOG, nothing more to do if (txData.simulationFails) { diff --git a/app/scripts/lib/encryptor.js b/app/scripts/lib/encryptor.js index df72b62c0..8bacab766 100644 --- a/app/scripts/lib/encryptor.js +++ b/app/scripts/lib/encryptor.js @@ -26,10 +26,16 @@ module.exports = { // Takes a Pojo, returns cypher text. function encrypt (password, dataObj) { - return keyFromPassword(password) + const salt = this.generateSalt() + + return keyFromPassword(password + salt) .then(function (passwordDerivedKey) { return encryptWithKey(passwordDerivedKey, dataObj) }) + .then(function (payload) { + payload.salt = salt + return JSON.stringify(payload) + }) } function encryptWithKey (key, dataObj) { @@ -44,22 +50,26 @@ function encryptWithKey (key, dataObj) { var buffer = new Uint8Array(buf) var vectorStr = encodeBufferToBase64(vector) var vaultStr = encodeBufferToBase64(buffer) - return `${vaultStr}\\${vectorStr}` + return { + data: vaultStr, + iv: vectorStr, + } }) } // Takes encrypted text, returns the restored Pojo. function decrypt (password, text) { - return keyFromPassword(password) + const payload = JSON.parse(text) + const salt = payload.salt + return keyFromPassword(password + salt) .then(function (key) { - return decryptWithKey(key, text) + return decryptWithKey(key, payload) }) } -function decryptWithKey (key, text) { - const parts = text.split('\\') - const encryptedData = decodeBase64ToBuffer(parts[0]) - const vector = decodeBase64ToBuffer(parts[1]) +function decryptWithKey (key, payload) { + const encryptedData = decodeBase64ToBuffer(payload.data) + const vector = decodeBase64ToBuffer(payload.iv) return crypto.subtle.decrypt({name: 'AES-GCM', iv: vector}, key, encryptedData) .then(function (result) { const decryptedData = new Uint8Array(result) diff --git a/test/unit/keyring-controller-test.js b/test/unit/keyring-controller-test.js index 437441e0e..c32141cc6 100644 --- a/test/unit/keyring-controller-test.js +++ b/test/unit/keyring-controller-test.js @@ -82,13 +82,15 @@ describe('KeyringController', function() { }) - describe('#migrateAndGetKey', function() { + describe('#migrateOldVaultIfAny', function() { it('should return the key for that password', function(done) { - keyringController.migrateAndGetKey(password) - .then((key) => { - assert(key, 'a key is returned') + keyringController.migrateOldVaultIfAny(password) + .then(() => { done() }) + .catch((reason) => { + assert.ifError(reason) + }) }) })