-
Notifications
You must be signed in to change notification settings - Fork 341
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
chore: bcrypt cost adjustment #670
Conversation
What's the impact on performance? |
Do bcrypt's security measures have relevance to this situation? I'm more concerned about the potential risks of a weak PRNG than demonstrating proof of effort/work here. |
Benchmarks with 12 cost
Benchmarks with 11 cost
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I support merging this issue as it is both redundant and disruptive to development.
I suggest that we open an issue to further discuss the security concerns. We should consider using a single round of bcrypt or switching to a simpler hashing function for this particular case. If that is not feasible, then let's determine the best default values and add comments to explain our decisions.
This reverts e0c50ec (gnolang#670) It breaks decoding of existing accounts. For instance if you addpkg, it gives: Enter password. ciphertext decryption failed Changing bcryptSecurityParameter = 12 (from 11) makes it work again. Better change until we find a better solution.
This reverts commit e0c50ec.
Co-authored-by: grepsuzette <grepsuzette@users.noreply.github.com>
In this PR, we reduce the
bcrypt
cost from12
to11
.@zivkovicmilos expressed concerns that
CreateAccount
was too slow.It seems like this is safe to do. As far as i understand this refers to local storage.