Merge pull request #819 from mozilla/rfk/config-scrypt-backlog

Improve operational affordances for scrypt max-pending limit

bin/key_server.js

@@ -8,14 +8,9 @@ var config = require('../config').root()
 function main() {
   var log = require('../log')(config.log.level)
 
-  function logMemoryStats() {
-    log.stat(
-      {
-        stat: 'mem',
-        rss: this.rss,
-        heapUsed: this.heapUsed
-      }
-    )
+  function logStatInfo() {
+    log.stat(server.stat())
+    log.stat(Password.stat())
   }
 
   log.event('config', config)
@@ -25,6 +20,7 @@ function main() {
 
   var error = require('../error')
   var Token = require('../tokens')(log, config.tokenLifetimes)
+  var Password = require('../crypto/password')(log, config)
 
   var CC = require('compute-cluster')
   var signer = new CC({ module: __dirname + '/signer.js' })
@@ -35,7 +31,7 @@ function main() {
   var Server = require('../server')
   var server = null
   var mailer = null
-  var memInterval = null
+  var statsInterval = null
   var database = null
   var customs = null
 
@@ -68,6 +64,7 @@ function main() {
                 signer,
                 db,
                 mailer,
+                Password,
                 config,
                 customs
               )
@@ -78,7 +75,7 @@ function main() {
                   log.info({ op: 'server.start.1', msg: 'running on ' + server.info.uri })
                 }
               )
-              memInterval = setInterval(logMemoryStats.bind(server.load), 15000)
+              statsInterval = setInterval(logStatInfo, 15000)
             },
             function (err) {
               log.error({ op: 'DB.connect', err: { message: err.message } })
@@ -101,7 +98,7 @@ function main() {
 
   function shutdown() {
     log.info({ op: 'shutdown' })
-    clearInterval(memInterval)
+    clearInterval(statsInterval)
     server.stop(
       function () {
         customs.close()

config/config.js

@@ -146,6 +146,13 @@ module.exports = function (fs, path, url, convict) {
         env: 'TOOBUSY_MAX_LAG'
       }
     },
+    scrypt: {
+      maxPending: {
+        doc: "Max number of scrypt hash operations that can be pending",
+        default: 0,
+        env: 'SCRYPT_MAX_PENDING'
+      }
+    },
     i18n: {
       defaultLanguage: {
         format: String,

crypto/password.js

@@ -4,61 +4,76 @@
 
 var P = require('../promise')
 var hkdf = require('./hkdf')
-var scrypt = require('./scrypt')
 var butil = require('./butil')
 
-var hashVersions = {
-  0: function (authPW, authSalt) {
-    return P(butil.xorBuffers(authPW, authSalt))
-  },
-  1: function (authPW, authSalt) {
-    return scrypt.hash(authPW, authSalt, 65536, 8, 1, 32)
+module.exports = function(log, config) {
+
+  var scrypt = require('./scrypt')(log, config)
+
+  var hashVersions = {
+    0: function (authPW, authSalt) {
+      return P(butil.xorBuffers(authPW, authSalt))
+    },
+    1: function (authPW, authSalt) {
+      return scrypt.hash(authPW, authSalt, 65536, 8, 1, 32)
+    }
   }
-}
 
-function Password(authPW, authSalt, version) {
-  version = typeof(version) === 'number' ? version : 1
-  this.authPW = authPW
-  this.authSalt = authSalt
-  this.version = version
-  this.stretchPromise = hashVersions[version](authPW, authSalt)
-  this.verifyHashPromise = this.stretchPromise.then(hkdfVerify)
-}
+  function Password(authPW, authSalt, version) {
+    version = typeof(version) === 'number' ? version : 1
+    this.authPW = authPW
+    this.authSalt = authSalt
+    this.version = version
+    this.stretchPromise = hashVersions[version](authPW, authSalt)
+    this.verifyHashPromise = this.stretchPromise.then(hkdfVerify)
+  }
 
-Password.prototype.stretchedPassword = function () {
-  return this.stretchPromise
-}
+  Password.prototype.stretchedPassword = function () {
+    return this.stretchPromise
+  }
 
-Password.prototype.verifyHash = function () {
-  return this.verifyHashPromise
-}
+  Password.prototype.verifyHash = function () {
+    return this.verifyHashPromise
+  }
 
-Password.prototype.matches = function (verifyHash) {
-  return this.verifyHash().then(
-    function (hash) {
-      return butil.buffersAreEqual(hash, verifyHash)
+  Password.prototype.matches = function (verifyHash) {
+    return this.verifyHash().then(
+      function (hash) {
+        return butil.buffersAreEqual(hash, verifyHash)
+      }
+    )
+  }
+
+  Password.prototype.unwrap = function (wrapped, context) {
+    context = context || 'wrapwrapKey'
+    return this.stretchedPassword().then(
+      function (stretched) {
+        return hkdf(stretched, context, null, 32)
+          .then(
+            function (wrapper) {
+              return butil.xorBuffers(wrapper, wrapped)
+            }
+          )
+      }
+    )
+  }
+  Password.prototype.wrap = Password.prototype.unwrap
+
+  function hkdfVerify(stretched) {
+    return hkdf(stretched, 'verifyHash', null, 32)
+  }
+
+  Password.stat = function () {
+    // Reset the high-water-mark whenever it is read.
+    var numPendingHWM = scrypt.numPendingHWM
+    scrypt.numPendingHWM = scrypt.numPending
+    return {
+      stat: 'scrypt',
+      maxPending: scrypt.maxPending,
+      numPending: scrypt.numPending,
+      numPendingHWM: numPendingHWM
     }
-  )
+  }
+
+  return Password
 }
-
-Password.prototype.unwrap = function (wrapped, context) {
-  context = context || 'wrapwrapKey'
-  return this.stretchedPassword().then(
-    function (stretched) {
-      return hkdf(stretched, context, null, 32)
-        .then(
-          function (wrapper) {
-            return butil.xorBuffers(wrapper, wrapped)
-          }
-        )
-    }
-  )
-}
-Password.prototype.wrap = Password.prototype.unwrap
-
-function hkdfVerify(stretched) {
-  return hkdf(stretched, 'verifyHash', null, 32)
-}
-
-
-module.exports = Password

crypto/scrypt.js

@@ -3,39 +3,53 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 var P = require('../promise')
-var scrypt = require('scrypt-hash')
+var scrypt_hash = require('scrypt-hash')
  
 // The maximum numer of hash operations allowed concurrently.
-// This can be customized by setting module.exports.MAX_PENDING
+// This can be customized by setting the `maxPending` attribute on the
+// exported object, or by setting the `scrypt.maxPending` config option.
 const DEFAULT_MAX_PENDING = 100
 
-// The current number of hash operations in progress.
-var num_pending = 0
+module.exports = function(log, config) {
 
-/**  hash - Creates an scrypt hash asynchronously
- *
- * @param {Buffer} input The input for scrypt
- * @param {Buffer} salt The salt for the hash
- * @returns {Object} d.promise Deferred promise
- */
-function hash(input, salt, N, r, p, len) {
-  var d = P.defer()
-  var MAX_PENDING = module.exports.MAX_PENDING
-  if (MAX_PENDING > 0 && num_pending > MAX_PENDING) {
-    d.reject(new Error('too many pending scrypt hashes'))
-  } else {
-    num_pending += 1
-    scrypt(input, salt, N, r, p, len,
-      function (err, hash) {
-        num_pending -= 1
-        return err ? d.reject(err) : d.resolve(hash.toString('hex'))
-      }
-    )
+  var scrypt = {
+    hash: hash,
+    // The current number of hash operations in progress.
+    numPending: 0,
+    // The high-water-mark on number of hash operations in progress.
+    numPendingHWM: 0,
+    // The maximum number of hash operations that may be in progress.
+    maxPending: DEFAULT_MAX_PENDING
+  }
+  if (config.scrypt && config.scrypt.hasOwnProperty("maxPending")) {
+    scrypt.maxPending = config.scrypt.maxPending
   }
-  return d.promise
-}
 
-module.exports = {
-  hash: hash,
-  MAX_PENDING: DEFAULT_MAX_PENDING
+  /**  hash - Creates an scrypt hash asynchronously
+   *
+   * @param {Buffer} input The input for scrypt
+   * @param {Buffer} salt The salt for the hash
+   * @returns {Object} d.promise Deferred promise
+   */
+  function hash(input, salt, N, r, p, len) {
+    var d = P.defer()
+    if (scrypt.maxPending > 0 && scrypt.numPending > scrypt.maxPending) {
+      log.warn({ op: 'scrypt.maxPendingExceeded' })
+      d.reject(new Error('too many pending scrypt hashes'))
+    } else {
+      scrypt.numPending += 1
+      if (scrypt.numPending > scrypt.numPendingHWM) {
+        scrypt.numPendingHWM = scrypt.numPending
+      }
+      scrypt_hash(input, salt, N, r, p, len,
+        function (err, hash) {
+          scrypt.numPending -= 1
+          return err ? d.reject(err) : d.resolve(hash.toString('hex'))
+        }
+      )
+    }
+    return d.promise
+  }
+
+  return scrypt
 }

routes/account.js

@@ -6,7 +6,6 @@ var validators = require('./validators')
 var HEX_STRING = validators.HEX_STRING
 var BASE64_JWT = validators.BASE64_JWT
 
-var Password = require('../crypto/password')
 var butil = require('../crypto/butil')
 
 module.exports = function (
@@ -18,6 +17,7 @@ module.exports = function (
   error,
   db,
   mailer,
+  Password,
   redirectDomain,
   verifierVersion,
   isProduction,

routes/index.js

@@ -16,6 +16,7 @@ module.exports = function (
   signer,
   db,
   mailer,
+  Password,
   config,
   customs
   ) {
@@ -33,6 +34,7 @@ module.exports = function (
     error,
     db,
     mailer,
+    Password,
     config.smtp.redirectDomain,
     config.verifierVersion,
     isProduction,
@@ -46,6 +48,7 @@ module.exports = function (
     isA,
     error,
     db,
+    Password,
     config.smtp.redirectDomain,
     mailer,
     config.verifierVersion,

routes/password.js

@@ -6,7 +6,6 @@ var validators = require('./validators')
 var HEX_STRING = validators.HEX_STRING
 
 var crypto = require('crypto')
-var Password = require('../crypto/password')
 var butil = require('../crypto/butil')
 
 module.exports = function (
@@ -14,6 +13,7 @@ module.exports = function (
   isA,
   error,
   db,
+  Password,
   redirectDomain,
   mailer,
   verifierVersion,

server/server.js

@@ -205,6 +205,14 @@ module.exports = function (path, url, Hapi) {
       }
     )
 
+    server.stat = function() {
+      return {
+        stat: 'mem',
+        rss: server.load.rss,
+        heapUsed: server.load.heapUsed
+      }
+    }
+
     return server
   }
 

test/local/password_tests.js

@@ -3,7 +3,9 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 var test = require('../ptaptest')
-var Password = require('../../crypto/password')
+var log = {}
+var config = {}
+var Password = require('../../crypto/password')(log, config)
 
 test(
   'password version zero',
@@ -71,3 +73,14 @@ test(
     )
   }
 )
+
+test(
+  'scrypt queue stats can be reported',
+  function (t) {
+    var stat = Password.stat()
+    t.equal(stat.stat, 'scrypt')
+    t.ok(stat.hasOwnProperty('numPending'))
+    t.ok(stat.hasOwnProperty('numPendingHWM'))
+    t.end()
+  }
+)

test/local/scrypt_tests.js

@@ -4,7 +4,13 @@
 
 var test = require('../ptaptest')
 var promise = require('../../promise')
-var scrypt = require('../../crypto/scrypt')
+var config = { scrypt: { maxPending: 5 } }
+var log = {
+  buffer: [],
+  warn: function(obj){ log.buffer.push(obj) },
+}
+
+var scrypt = require('../../crypto/scrypt')(log, config)
 
 test(
   'scrypt basic',
@@ -26,22 +32,22 @@ test(
   function (t) {
     var K1 = Buffer('f84913e3d8e6d624689d0a3e9678ac8dcc79d2c2f3d9641488cd9d6ef6cd83dd', 'hex')
     var salt = Buffer('identity.mozilla.com/picl/v1/scrypt')
-    // Set a lower MAX_PENDING so the test runs quicker.
-    var orig_max_pending = scrypt.MAX_PENDING;
-    scrypt.MAX_PENDING = 5;
-    // Send many concurent requests.
+    // Check the we're using the lower maxPending setting from config.
+    t.equal(scrypt.maxPending, 5, 'maxPending is correctly set from config')
+    // Send many concurrent requests.
     // Not yielding the event loop ensures they will pile up quickly.
     var promises = [];
     for (var i = 0; i < 10; i++) {
       promises.push(scrypt.hash(K1, salt, 65536, 8, 1, 32))
     }
-    scrypt.MAX_PENDING = orig_max_pending;
     return promise.all(promises).then(
       function () {
         t.fail('too many pending scrypt hashes were allowed')
       },
       function (err) {
         t.equal(err.message, 'too many pending scrypt hashes')
+        t.equal(scrypt.numPendingHWM, 6, 'HWM should be maxPending+1')
+        t.equal(log.buffer[0].op, 'scrypt.maxPendingExceeded')
       }
     );
   }