fix(recovery): hash recovery key

db-server/index.js

@@ -244,7 +244,8 @@ function createServer(db) {
     op((req) => db.consumeRecoveryCode(req.params.id, req.params.code))
   )
 
-  api.get('/account/:id/recoveryKey', withParams(db.getRecoveryKey))
+  api.get('/account/:id/recoveryKey/:recoveryKeyId', withParams(db.getRecoveryKey))
+  api.get('/account/:id/recoveryKey', withIdAndBody(db.recoveryKeyExists))
   api.del('/account/:id/recoveryKey', withParams(db.deleteRecoveryKey))
   api.post('/account/:id/recoveryKey', withIdAndBody(db.createRecoveryKey))
 

db-server/lib/error.js

@@ -83,6 +83,15 @@ AppError.invalidVerificationMethod = function () {
   )
 }
 
+AppError.recoveryKeyInvalid = () => {
+  return new AppError({
+    code: 400,
+    error: 'Bad Request',
+    errno: 159,
+    message: 'Recovery key is not valid'
+  })
+}
+
 AppError.wrap = function (err) {
   // Don't re-wrap!
   if (err instanceof AppError) {

db-server/test/backend/db_tests.js

@@ -6,6 +6,7 @@
 const assert = require('insist')
 const crypto = require('crypto')
 const P = require('bluebird')
+const util = require('../../../lib/db/util')
 
 const zeroBuffer16 = Buffer.from('00000000000000000000000000000000', 'hex')
 const zeroBuffer32 = Buffer.from('0000000000000000000000000000000000000000000000000000000000000000', 'hex')
@@ -2279,18 +2280,21 @@ module.exports = function (config, DB) {
 
       it('should get account recovery key', () => {
         const options = {
-          id: account.uid
+          id: account.uid,
+          recoveryKeyId: data.recoveryKeyId
         }
         return db.getRecoveryKey(options)
           .then((res) => {
             assert.equal(res.recoveryData, data.recoveryData, 'recovery data set')
-            assert.equal(res.recoveryKeyId.toString('hex'), data.recoveryKeyId.toString('hex'), 'recovery data set')
+            const recoveryKeyIdHash = util.createHash(data.recoveryKeyId)
+            assert.equal(res.recoveryKeyIdHash.toString('hex'), recoveryKeyIdHash.toString('hex'), 'recoveryKeyId set')
           })
       })
 
       it('should fail to get key for incorrect user', () => {
         const options = {
-          id: 'unknown'
+          id: 'unknown',
+          recoveryKeyId: '123'
         }
         return db.getRecoveryKey(options)
           .then(assert.fail, (err) => {
@@ -2298,12 +2302,13 @@ module.exports = function (config, DB) {
           })
       })
 
-      it('should fail to get unknown key', () => {
+      it('should fail to get non-existent key', () => {
         account = createAccount()
         return db.createAccount(account.uid, account)
           .then(() => {
             const options = {
-              id: account.uid
+              id: account.uid,
+              recoveryKeyId: 'unknown'
             }
             return db.getRecoveryKey(options)
               .then(assert.fail, (err) => {
@@ -2312,14 +2317,39 @@ module.exports = function (config, DB) {
           })
       })
 
-      it('should delete account recovery key', () => {
+      it('should fail to get key with invalid recoveryKeyId', () => {
         const options = {
           id: account.uid,
-          recoveryKeyId: data.recoveryKeyId
+          recoveryKeyId: 'incorrect recoveryKeyId'
         }
-        return db.deleteRecoveryKey(options)
+        return db.getRecoveryKey(options)
+          .then(assert.fail, (err) => {
+            assert.equal(err.errno, 159, 'incorrect recoveryKeyId')
+          })
+      })
+
+      it('should return true if recovery key exists', () => {
+        return db.recoveryKeyExists(account.uid)
           .then((res) => {
-            assert.ok(res)
+            assert.equal(res.exists, true, 'key exists')
+          })
+      })
+
+      it('should return false if recovery key doesn\'t exist', () => {
+        account = createAccount()
+        return db.createAccount(account.uid, account)
+          .then(() => {
+            return db.recoveryKeyExists(account.uid)
+              .then((res) => {
+                assert.equal(res.exists, false, 'key doesn\'t exist')
+              })
+          })
+      })
+
+      it('should throw when checking for recovery key on non-existent user', () => {
+        return db.recoveryKeyExists('nonexistent')
+          .then((res) => {
+            assert.equal(res.exists, false, 'key doesn\'t exist')
           })
       })
     })

db-server/test/backend/remote.js

@@ -1749,7 +1749,7 @@ module.exports = function(cfg, makeServer) {
       })
 
       it('should get a recovery key', () => {
-        return client.getThen('/account/' + user.accountId + '/recoveryKey')
+        return client.getThen('/account/' + user.accountId + '/recoveryKey/' + recoveryKey.recoveryKeyId)
           .then((res) => {
             const recoveryKeyResult = res.obj
             assert.equal(recoveryKeyResult.recoveryData, recoveryKey.recoveryData, 'recoveryData match')
@@ -1762,6 +1762,14 @@ module.exports = function(cfg, makeServer) {
             respOkEmpty(r)
           })
       })
+
+      it('should check if recovery key exists', () => {
+        return client.getThen('/account/' + user.accountId + '/recoveryKey')
+          .then((res) => {
+            const result = res.obj
+            assert.equal(result.exists, true, 'recovery key exists')
+          })
+      })
     })
 
     after(() => server.close())

docs/API.md

@@ -2281,3 +2281,41 @@ Content-Length: 2
     * Conditions: if something goes wrong on the server
     * Content-Type : `application/json`
     * Body : `{"code":"InternalError","message":"..."}`
+
+## recoveryKeyExists : `GET /account/:uid/recoveryKey`
+
+Check if this user has a recovery key
+
+### Example
+
+```
+curl \
+    -v \
+    -X GET \
+    -H "Content-Type: application/json" \
+    http://localhost:8000/account/1234567890ab/recoveryKey
+```
+
+### Request
+
+* Method : `GET`
+* Path : `/account/<uid>/recoveryKey`
+    * `uid` : hex
+
+### Response
+
+```
+HTTP/1.1 200 OK
+Content-Type: application/json
+Content-Length: 2
+
+{"exists": true}
+```
+
+* Status Code : `200 OK`
+    * Content-Type : `application/json`
+    * Body : `{"exists": true}`
+* Status Code : `500 Internal Server Error`
+    * Conditions: if something goes wrong on the server
+    * Content-Type : `application/json`
+    * Body : `{"code":"InternalError","message":"..."}`

docs/DB_API.md

@@ -72,8 +72,9 @@ There are a number of methods that a DB storage backend should implement:
     * .consumeRecoveryCode(uid, code)
 * Recovery keys
     * .createRecoveryKey(uid, data)
-    * .getRecoveryKey(uid)
+    * .getRecoveryKey(data)
     * .deleteRecoveryKey(uid)
+    * .recoveryKeyExists(uid)
 * General
     * .ping()
     * .close()
@@ -954,7 +955,7 @@ Returns:
   * Any error from the underlying storage system (wrapped in `error.wrap()`)
   * `error.notFound()` if this user found
 
-## getRecoveryKey(uid)
+## getRecoveryKey(data)
 
 Get the recovery key for this user.
 
@@ -962,6 +963,8 @@ Parameters:
 
 * `uid` (Buffer16):
   The uid of the owning account
+* `recoveryKeyId` (Buffer32):
+  The recoveryKeyId for account
 
 Returns:
 
@@ -988,3 +991,19 @@ Returns:
 * Rejects with:
   * Any error from the underlying storage system (wrapped in `error.wrap()`)
   * `error.notFound()` if this user or recovery key not found
+
+## recoveryKeyExists(uid)
+
+Check to see if a recovery key exists for this user.
+
+Parameters:
+
+* `uid` (Buffer16):
+  The uid of the owning account
+
+Returns:
+
+* Resolves with:
+  * object {"exists": true}
+* Rejects with:
+  * Any error from the underlying storage system (wrapped in `error.wrap()`)

lib/db/mem.js

@@ -1371,7 +1371,7 @@ module.exports = function (log, error) {
 
         recoveryKeys[uid] = {
           uid,
-          recoveryKeyId: data.recoveryKeyId,
+          recoveryKeyIdHash: dbUtil.createHash(data.recoveryKeyId).toString('hex'),
           recoveryData: data.recoveryData
         }
 
@@ -1381,6 +1381,7 @@ module.exports = function (log, error) {
 
   Memory.prototype.getRecoveryKey = function (options) {
     const uid = options.id.toString('hex')
+    const recoveryKeyIdHash = dbUtil.createHash(options.recoveryKeyId).toString('hex')
     return getAccountByUid(uid)
       .then(() => {
         const recoveryKey = recoveryKeys[uid]
@@ -1389,10 +1390,39 @@ module.exports = function (log, error) {
           return P.reject(error.notFound())
         }
 
+        if (recoveryKey.recoveryKeyIdHash !== recoveryKeyIdHash) {
+          return P.reject(error.recoveryKeyInvalid())
+        }
+
         return recoveryKey
       })
   }
 
+  Memory.prototype.recoveryKeyExists = function (uid) {
+    uid = uid.toString('hex')
+    let exists = true
+    return getAccountByUid(uid)
+      .then(() => {
+        const recoveryKey = recoveryKeys[uid]
+
+        if (! recoveryKey) {
+          exists = false
+        }
+
+        return {exists}
+      })
+      .catch((err) => {
+        // To match the mysql implementation, we return false when the
+        // user does not exist.
+        if (err.errno === error.notFound().errno) {
+          exists = false
+          return {exists}
+        }
+
+        throw err
+      })
+  }
+
   Memory.prototype.deleteRecoveryKey = function (options) {
     const uid = options.id.toString('hex')
     return getAccountByUid(uid)

lib/db/mysql.js

@@ -1531,11 +1531,11 @@ module.exports = function (log, error) {
       })
   }
 
-  const CREATE_RECOVERY_KEY = 'CALL createRecoveryKey_2(?, ?, ?)'
+  const CREATE_RECOVERY_KEY = 'CALL createRecoveryKey_3(?, ?, ?)'
   MySql.prototype.createRecoveryKey = function (uid, data) {
-    const recoveryKeyId = data.recoveryKeyId
+    const recoveryKeyIdHash = dbUtil.createHash(data.recoveryKeyId)
     const recoveryData = data.recoveryData
-    return this.write(CREATE_RECOVERY_KEY, [uid, recoveryKeyId, recoveryData])
+    return this.write(CREATE_RECOVERY_KEY, [uid, recoveryKeyIdHash, recoveryData])
       .then(() => {
         return {}
       })
@@ -1548,7 +1548,7 @@ module.exports = function (log, error) {
       })
   }
 
-  const GET_RECOVERY_KEY = 'CALL getRecoveryKey_2(?)'
+  const GET_RECOVERY_KEY = 'CALL getRecoveryKey_3(?)'
   MySql.prototype.getRecoveryKey = function (options) {
     return this.readFirstResult(GET_RECOVERY_KEY, [options.id])
       .then((results) => {
@@ -1557,10 +1557,30 @@ module.exports = function (log, error) {
           throw error.notFound()
         }
 
+        // Currently, a user can only have one recovery key. Instead of
+        // simply returning the key, lets double check that the right recoveryKeyId
+        // was specified and throw a custom error if they don't match.
+        const recoveryKeyIdHash = dbUtil.createHash(options.recoveryKeyId)
+        if (! results.recoveryKeyIdHash.equals(recoveryKeyIdHash)) {
+          throw error.recoveryKeyInvalid()
+        }
+
         return results
       })
   }
 
+  MySql.prototype.recoveryKeyExists = function (uid) {
+    let exists = true
+    return this.read(GET_RECOVERY_KEY, [uid])
+      .then((results) => {
+        if (results[0].length === 0) {
+          exists = false
+        }
+
+        return {exists}
+      })
+  }
+
   const DELETE_RECOVERY_KEY = 'CALL deleteRecoveryKey_2(?)'
   MySql.prototype.deleteRecoveryKey = function (options) {
     return this.write(DELETE_RECOVERY_KEY, [options.id])

lib/db/patch.js

@@ -4,4 +4,4 @@
 
 // The expected patch level of the database. Update if you add a new
 // patch in the ./schema/ directory.
-module.exports.level = 85
+module.exports.level = 86

lib/db/schema/patch-085-086.sql

@@ -0,0 +1,60 @@
+-- This migration updates recoveryKey table to store a hashed value of
+-- the `recoveryKeyId` instead of the raw value. As part of this migration,
+-- we will remove all existing recoveryKeys. This will help to ensure a consistent
+-- migration.
+SET NAMES utf8mb4 COLLATE utf8mb4_bin;
+
+CALL assertPatchLevel('85');
+
+DELETE FROM recoveryKeys;
+
+-- We can drop this since we will be using the `recoveryKeyIdHash`
+-- column instead.
+ALTER TABLE recoveryKeys DROP COLUMN recoveryKeyId,
+ALGORITHM = INPLACE, LOCK = NONE;
+
+ALTER TABLE recoveryKeys ADD COLUMN recoveryKeyIdHash BINARY(32) NOT NULL,
+ALGORITHM = INPLACE, LOCK = NONE;
+
+-- If we are in the process of a rollout and a user calls `createRecoveryKey_2`
+-- then that request will fail. But that is ok since the previous create
+-- procedure would have put the raw recoveryKeyId in database, which
+-- would not work anyways.
+CREATE PROCEDURE `createRecoveryKey_3` (
+  IN `uidArg` BINARY(16),
+  IN `recoveryKeyIdHashArg` BINARY(32),
+  IN `recoveryDataArg` TEXT
+)
+BEGIN
+
+  DECLARE EXIT HANDLER FOR SQLEXCEPTION
+  BEGIN
+    ROLLBACK;
+    RESIGNAL;
+  END;
+
+  START TRANSACTION;
+
+  SET @accountCount = 0;
+
+  -- Signal error if no user found
+  SELECT COUNT(*) INTO @accountCount FROM `accounts` WHERE `uid` = `uidArg`;
+  IF @accountCount = 0 THEN
+    SIGNAL SQLSTATE '45000' SET MYSQL_ERRNO = 1643, MESSAGE_TEXT = 'Can not create recovery key for unknown user.';
+  END IF;
+
+  INSERT INTO recoveryKeys (uid, recoveryKeyIdHash, recoveryData) VALUES (uidArg, recoveryKeyIdHashArg, recoveryDataArg);
+
+  COMMIT;
+END;
+
+CREATE PROCEDURE `getRecoveryKey_3` (
+  IN `uidArg` BINARY(16)
+)
+BEGIN
+
+  SELECT recoveryKeyIdHash, recoveryData FROM recoveryKeys WHERE uid = uidArg;
+
+END;
+
+UPDATE dbMetadata SET value = '86' WHERE name = 'schema-patch-level';

lib/db/schema/patch-086-085.sql

@@ -0,0 +1,14 @@
+-- SET NAMES utf8mb4 COLLATE utf8mb4_bin;
+
+-- ALTER TABLE `recoveryKeys`
+-- ADD COLUMN recoveryKeyId
+-- ALGORITHM = INPLACE, LOCK = NONE;
+
+-- ALTER TABLE `recoveryKeys`
+-- DROP COLUMN recoveryKeyIdHash
+-- ALGORITHM = INPLACE, LOCK = NONE;
+
+-- DROP PROCEDURE createRecoveryKey_3;
+-- DROP PROCEDURE getRecoveryKey_3;
+
+-- UPDATE dbMetadata SET value = '85' WHERE name = 'schema-patch-level';

lib/db/util.js

@@ -187,5 +187,4 @@ module.exports = {
       return items
     }, [])
   }
-
 }