Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make bcrypt rounds configurable #9044

Closed
wants to merge 14 commits into from
Closed

Make bcrypt rounds configurable #9044

wants to merge 14 commits into from

Conversation

@dhrubins
Copy link
Contributor

@dhrubins dhrubins commented Aug 25, 2017

Background: the bcrypt work factor or cost factor is a way of setting the difficulty of cracking a bcrypt'ed password if the databse is ever breached. The current hardcoded setting of 10 rounds in accounts-password is a bit low, given the availability of GPUs and cloud password cracking services. Every operator should choose the right value for their threat model, but 12-14 is likely sufficient for most services. At Legal Robot, we use 14.

For more detail, see the discussion at meteor/meteor-feature-requests/issues/55

This PR adds a configuration option for those using accounts-password that want to increase the bcrypt rounds with little impact to existing users. During the password check, after a password is validated, the bcrypt'ed password is replaced with an updated value if the current number of bcrypt rounds exceeds that of the stored bcrypt value. The default behavior is to not change anything, but the value for bcryptRounds is easily configurable by setting Accounts.config({ bcryptRounds: 14 }) on the server.

dhrubins added 2 commits Aug 25, 2017
Copy link
Member

@hwillson hwillson left a comment

Thanks for submitting these changes @dhrubins! This makes a lot of sense. At some point we should also consider bumping the default of 10 (but we can leave that for now). Outside of the minor syntax changes I've requested, would you be able to:

  1. Add jsdoc comments describing this new option, here.
  2. Add a small entry to the History.md file describing these changes.

Thanks again!

@@ -67,6 +67,15 @@ Accounts._checkPassword = function (user, password) {

if (! bcryptCompare(password, user.services.password.bcrypt)) {
result.error = handleError("Incorrect password", false);
} else {

This comment has been minimized.

@hwillson

hwillson Sep 5, 2017
Member

Since there is only one if under else here, could you merge them into an else if to improve readability a bit?

This comment has been minimized.

@dhrubins

dhrubins Sep 6, 2017
Author Contributor

Makes sense - done in a5206a6

@@ -67,6 +67,15 @@ Accounts._checkPassword = function (user, password) {

if (! bcryptCompare(password, user.services.password.bcrypt)) {
result.error = handleError("Incorrect password", false);
} else {
// password checks out, but user bcrypt may need update
if (user.services.password.bcrypt && Accounts._bcryptRounds > Number(user.services.password.bcrypt.substring(4, 6))) {

This comment has been minimized.

@hwillson

hwillson Sep 5, 2017
Member

Please wrap your code changes at 80 characters.

This comment has been minimized.

@dhrubins

dhrubins Sep 6, 2017
Author Contributor

No problem - done in b5cd99a

dhrubins and others added 7 commits Sep 6, 2017
} else if (
user.services.password.bcrypt &&
Accounts._bcryptRounds >
Number(user.services.password.bcrypt.substring(4, 6))

This comment has been minimized.

@benjamn

benjamn Sep 6, 2017
Member

Will this work if bcryptRounds >= 100 (i.e., more than two digits)?

This comment has been minimized.

@benjamn

benjamn Sep 6, 2017
Member

While you're at it, let's just extract a helper function called something like getRoundsFromBcryptHash?

This comment has been minimized.

@dhrubins

dhrubins Sep 6, 2017
Author Contributor

No, that code will break with a work factor above 99, but it shouldn't be possible to implement bcrypt with such a high work factor since it gets expanded geometrically (rounds = 2^[work factor]). This FreeBSD implementation of bcrypt references a maximum of 31 rounds. I'm not sure if 31 is implementation-specific or the theoretical maximum for bcrypt, but the effective maximum for most current server hardware is probably 16 or 17. From benchmarking on my MacBook Pro, hashing with a work factor of 20 takes about 2.4 minutes and 31 would take ~12 years. The rule of thumb I'm aware of (totally unattributed) is that hashing should take ~1 second with current server hardware. If I can find a source for that rule of thumb, I'll do a PR for this in the Guide.

This comment has been minimized.

@dhrubins

dhrubins Sep 6, 2017
Author Contributor

Sure, I'm happy to move that into a helper function. At the risk of over-building but with a mind toward implementing other hashing algorithms, I'll grab both the algorithm and whatever parameters go with that algorithm.

@@ -67,6 +67,20 @@ Accounts._checkPassword = function (user, password) {

if (! bcryptCompare(password, user.services.password.bcrypt)) {
result.error = handleError("Incorrect password", false);
} else if (
user.services.password.bcrypt &&
Accounts._bcryptRounds >

This comment has been minimized.

@benjamn

benjamn Sep 6, 2017
Member

Maybe != instead of just >?

This comment has been minimized.

@dhrubins

dhrubins Sep 6, 2017
Author Contributor

Yep, will fix

@hwillson
Copy link
Member

@hwillson hwillson commented Oct 4, 2017

@dhrubins Have you had a chance to dive back into this? Let us know if you'll have some time to wrap this up. Thanks!

@hwillson hwillson self-assigned this Oct 11, 2017
@hwillson hwillson dismissed their stale review Oct 17, 2017

I've implemented my suggested review comments, so someone else should do a final review.

@hwillson
Copy link
Member

@hwillson hwillson commented Oct 17, 2017

I've addressed the outstanding review comments and added tests, so this should now be ready for a final review. One thing to note - I bumped accounts-base to 1.3.6 even though devel is currently at 1.3.4, because PR #9042 is bumping accounts-base to 1.3.5. Depending on the merge order of these PR's, this might need to be adjusted.

'passwords - allow custom bcrypt rounds',
function (test, done) {
function getUserHashRounds(user) {
return Number(user.services.password.bcrypt.substring(4, 6));

This comment has been minimized.

@benjamn

benjamn Oct 18, 2017
Member

Let's use parseInt here to avoid interpreting leading 0s as octal.

This comment has been minimized.

@hwillson

hwillson Oct 19, 2017
Member

Done - and I've adjusted rounds extraction to use split string segments. Thanks!

@benjamn
Copy link
Member

@benjamn benjamn commented Oct 19, 2017

Rebased and pushed to devel as 1687537.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.