From fe12332ef6b78ff9d505cf4ae0584dd33a19ba7d Mon Sep 17 00:00:00 2001 From: Vijay Budhram Date: Wed, 29 Aug 2018 16:55:35 -0400 Subject: [PATCH] fix(recovery): hash recovery key --- db-server/index.js | 3 +- db-server/lib/error.js | 9 +++++ db-server/test/backend/db_tests.js | 48 +++++++++++++++++++----- db-server/test/backend/remote.js | 10 ++++- docs/API.md | 38 +++++++++++++++++++ docs/DB_API.md | 25 +++++++++++-- lib/db/mem.js | 32 +++++++++++++++- lib/db/mysql.js | 28 ++++++++++++-- lib/db/patch.js | 2 +- lib/db/schema/patch-085-086.sql | 60 ++++++++++++++++++++++++++++++ lib/db/schema/patch-086-085.sql | 14 +++++++ lib/db/util.js | 1 - 12 files changed, 249 insertions(+), 21 deletions(-) create mode 100644 lib/db/schema/patch-085-086.sql create mode 100644 lib/db/schema/patch-086-085.sql diff --git a/db-server/index.js b/db-server/index.js index c088e36..3c373a7 100644 --- a/db-server/index.js +++ b/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)) diff --git a/db-server/lib/error.js b/db-server/lib/error.js index fd738c6..d77240c 100644 --- a/db-server/lib/error.js +++ b/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) { diff --git a/db-server/test/backend/db_tests.js b/db-server/test/backend/db_tests.js index 7548443..600afe3 100644 --- a/db-server/test/backend/db_tests.js +++ b/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') }) }) }) diff --git a/db-server/test/backend/remote.js b/db-server/test/backend/remote.js index e465369..1efb4c5 100644 --- a/db-server/test/backend/remote.js +++ b/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()) diff --git a/docs/API.md b/docs/API.md index 62672d3..51aa253 100644 --- a/docs/API.md +++ b/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//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":"..."}` \ No newline at end of file diff --git a/docs/DB_API.md b/docs/DB_API.md index 7375917..1c2d238 100644 --- a/docs/DB_API.md +++ b/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: @@ -987,4 +990,20 @@ Returns: * Empty object `{}` * Rejects with: * Any error from the underlying storage system (wrapped in `error.wrap()`) - * `error.notFound()` if this user or recovery key not found \ No newline at end of file + * `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()`) \ No newline at end of file diff --git a/lib/db/mem.js b/lib/db/mem.js index 4027a28..52288cd 100644 --- a/lib/db/mem.js +++ b/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) diff --git a/lib/db/mysql.js b/lib/db/mysql.js index aeeff29..cd526c1 100644 --- a/lib/db/mysql.js +++ b/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]) diff --git a/lib/db/patch.js b/lib/db/patch.js index ba04261..f7a7337 100644 --- a/lib/db/patch.js +++ b/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 diff --git a/lib/db/schema/patch-085-086.sql b/lib/db/schema/patch-085-086.sql new file mode 100644 index 0000000..551931d --- /dev/null +++ b/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'; diff --git a/lib/db/schema/patch-086-085.sql b/lib/db/schema/patch-086-085.sql new file mode 100644 index 0000000..fe43b0a --- /dev/null +++ b/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'; diff --git a/lib/db/util.js b/lib/db/util.js index 350e3c3..8a7abd5 100644 --- a/lib/db/util.js +++ b/lib/db/util.js @@ -187,5 +187,4 @@ module.exports = { return items }, []) } - }