Replace bcrypt with scrypt

[morenoh149]

Re http://chargen.matasano.com/chargen/2015/3/26/enough-with-the-salts-updates-on-secure-password-schemes.html

Use https://github.com/barrysteyn/node-scrypt Not sure if this requires an upgrade path for existing hashes using bcrypt @JedWatson please advise. But I definitely think its the right default for new projects.

[estilles]

Scrypt has been my go-to key derivation algorithm since I read Colin Percival's paper on "Stronger Key Derivation Via Sequential Memory-Hard Functions" (check it out here if you're a math geek like me).

Scrypt is awesome because it makes brute-force attackers pay penalties both in CPU and in memory usage. Plus, it has a higher theoretical-safety-to-compute-time factor than bcrypt.

NOTE: If we do switch, beware if improper settings. Weak settings can lead to undesirable results.

An article by IRCMaxel pointed out this weakness. While I DO NOT agree with his assessment that scrypt should not used for password storage, his analysis is spot on. Care must be taken with the settings we use to ensure we take advantage of the time-memory trade-off factor offered by scrypt.

Properly configured scrypt can be more secure that bcrypt.

@morenoh149, I'm very pro scrypt. I'm wondering, though, what is your motivation behind the suggested change? Is it simply to improve security? Is it an attempt to improve speed, in response to issue #1237?

[morenoh149]

I read that blog post today 🐲

Looks like you'd want interactive login which is sub 100ms https://github.com/barrysteyn/node-scrypt#asynchronous-authentication-and-verification (though the blogpost mentions <0.5ms for a site with many users). Then the other half of the equation is how powerful are the machines typically running Keystone. I typically run on Heroku's free tier so the time should be higher. But I think 100ms is a good starting point and successful Keystone sites should reevaluate their security anyway.

[estilles]

@morenoh149, What is your motivation behind the suggested change though? Is it simply to improve security? Is it an attempt to improve speed, in response to issue #1237? I'm just curious.

[morenoh149]

Improve security.

I reread issue #1237 . If your concern is the impact it would have on keystone installation it'd just require an extra step to install OpenSSL on windows machines. https://github.com/barrysteyn/node-scrypt#installation-instructions

[dcousens]

@morenoh149 are you willing to make a PR?

[morenoh149]

@dcousens the code change itself is trivial. The complicated part is how to deal with existing keystone sites. Greenfield sites wouldn't have issues. But there must be an upgrade path for existing sites. I imagine the only way is to prompt all users to reenter their password and then scrypt encrypt it.

[JedWatson]

@morenoh149 is right about needing an upgrade path. I don't see prompting users to re-enter their passwords as a direction we can enforce at a framework level.

I think the right solution here is to give developers the option to select either by setting an option on the Password field.

e.g

User.add({
  password: { type: Types.Password, method: 'scrypt' }
});

We would (for now, at least) default to bcrypt for the method. We can update the yeoman generator to use scrypt if the consensus is that it's better than bcrypt, and then most everyone would just use it by default going forward, but we won't be breaking any existing instances with the change.

(p.s I'm open to suggestions other than method for the option name.)

[dcousens]

(p.s I'm open to suggestions other than method for the option name.)

Its a key stretching algorithm, but I guess just algorithm might be saner?

And I guess the upgrade path could just be (pseudo-code):

var latestAndGreatest = 'scrypt'

function auth(user, password, callback) {
  // ...
  User.findOne(user, function(err, data) {
    if (err) return callback(err)

    var storedPassword = data.password.value
    var storedAlgorithm = data.password.algorithm

    var stretchFn = algorithms[storedAlgorithm]
    var verified = (storedPassword === stretchFn(password))

    // update storage with new algorithm if necessary
    if (storedAlgorithm !== latestAndGreatest) {
      var wantedStretchFn = algorithms[latestAndGreatest]

      user.password = new Password({
        value: wantedStretchFn(password),
        algorithm: latestAndGreatest
      })

       user.save(function(err) {
              if (err) return callback(err)

              callback(null, verified)
       })

     // we must have already updated this, continue as normal
    } else {
       callback(null, verified)
    }
  })
}

[dcousens]

In the upgrade case, that could be an easy way to allow for upgrades without asking users for their passwords as an extra-curricular activity; while still providing you the ability to do a call-out to non-upgraded users after some time period if you feel the threat level is high enough.

[JedWatson]

We could publish a reference implementation for anyone who wants to do a migration using the method you suggest @dcousens, based on the yeoman generated structure.

[JedWatson]

re algorithm I thought of that but method seems easier to type / remember, especially for users who don't have english as their first language. that's just my assumption though.

[creynders]

How 'bout hashProvider, hashLib, hashModule etc.? I think that'll be more easily understood what it's for.

[JedWatson]

Happy for a PR implementing this option as described above, by the way.

[ericelliott]

We can update the yeoman generator to use scrypt if the consensus is that it's better than bcrypt

I don't think there is any scientific consensus that any of script, bcrypt, or pbkdf2 are superior to the others. What I do know is that pbkdf2 is backed by the most academic research, and it's the only option for apps/sites to attain certain types of US government security certification, so I definitely agree with @JedWatson that this needs to be user configurable.

[gautamsi]

Keystone 4 is going in maintenance mode. Expect no major change. see #4913 for details.