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

feature request: defining work in terms of time (not rounds) #787

Open
phiferd opened this issue Feb 27, 2020 · 4 comments
Open

feature request: defining work in terms of time (not rounds) #787

phiferd opened this issue Feb 27, 2020 · 4 comments
Labels

Comments

@phiferd
Copy link

phiferd commented Feb 27, 2020

As I understand it, the current best practice to selecting the right number of rounds to use is to figure out how much time you're willing to spend computing password hashes, test to see how many rounds that would be, and then use that many rounds until, say, next year, when you need to reevaluate. Would it be better to allow the work parameter to be sent in units of time?

It seems it would be pretty easy to add a new hash method that would accept a minimum time (maybe in addition to a minimum work factor, as a sanity check) to produce an encrypted value. Here's the pseudo code around what I think would be the key changes (https://github.com/kelektiv/node.bcrypt.js/blob/master/src/bcrypt.cc#L235):

 thash(pwd, minTimeMillis, minLogRounds, seed) 
    // do the usual setup as in hash()
    // "seed" is the only portion of the normal salt that is needed for this, I think 
    ...

    done = false;
    log_rounds = minLogRounds;
    startTimeMillis = now
    while (!done) {
        for (k = 0; k < rounds; k++) {
		// do the normal expand state stuff here
	}
        elapsedTimeMillis = now - startTimeMillis;
        if (elapsedTimeMillis < minTimeMillis) 
            rounds = rounds * 2;
            log_rounds = log_rounds + 1;
        else 
            done = true;
   }
   ...

That way, you could use

const hashed = await bcrypt.thash(plainTextPassword, minTimeMillis, minLogRounds);

where minMillis is less likely to need to be periodically re-evaluated and devs can more easily think about how much time they can spare to do the hashing. Also, there may be reasonable default settings for both parameters, which would allow:

const hashed = await bcrypt.thash(plainTextPassword);

The seed parameter could be generated automatically if it's not provided, just like the full salt is generated automatically right now.

The output would look just like the output of hash and would contain all of the usual information (version, seed, logr, encrypted), so there would be no issue with compatibility.

@recrsn recrsn added the feature label Mar 2, 2020
@recrsn
Copy link
Collaborator

recrsn commented Mar 2, 2020

It would be trivial to implement it in a pre-compute phase and determine the number of rounds (if that's what you wish to have). There isn't much value in including it as part of the core API. You are welcome to add an example to the README or the Wiki.

@phiferd
Copy link
Author

phiferd commented Mar 5, 2020

Fair enough. I was just thinking that building it into the library would prevent a lot accidental mistakes by devs that aren't familiar with the details (like finding a blog post from 2012 that says 10 rounds is "good enough").

I guess a helper/wrapper class could compute that value on startup by hashing a dummy password with increasing rounds until the desired time is reached and then cache it. Using a guess-and-check approach would be too expensive every time, which is why I was thinking putting it directly in the main loop made more sense (since there's no significant penalty).

It would be interesting if someone crawled open source projects that use bcrypt to see how many projects have a hardcoded number that is no longer reasonable.

@recrsn
Copy link
Collaborator

recrsn commented Mar 8, 2020

We have 188k dependents now

sujal-ux added a commit to sujal-ux/node.bcrypt.js that referenced this issue Aug 23, 2021
@sujal-ux
Copy link

While going through the issues, I found that this feature mentioned in the request can be so significant in way to provide the most complex hashing with the provided constraints of time. With time, runtime for hashing with same number of rounds while generating salt is decreasing, and number of rounds is actually provided taking the runtime into consideration. So why not the number of rounds with the given time should be maximized with the current and updated versions of bcrypt.
Therefore I worked on this feature and found out the relation between expected time for hashing and number of rounds. It roughly followed the following formula:

 number_of_rounds = log2(expected_time) + 3

So, I made another two functions hashByTime and genSaltByTime, to hash a password by taking expected runtime (milliseconds) in place of rounds as argument . For further details you can check my pull request link.
Hope it will be reviewed and merged soon.

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

No branches or pull requests

3 participants