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

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)
+      })
     })
   })