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

Unnecessary bcrypt #14

Open
brendanlong opened this issue Dec 2, 2017 · 3 comments
Open

Unnecessary bcrypt #14

brendanlong opened this issue Dec 2, 2017 · 3 comments

Comments

@brendanlong
Copy link

This storage backend uses bcrypt to hash the tokens, but it shouldn't be necessary. The purpose of bcrypt is to take a low-entropy string and turn it into a high-entropy key, but we don't have that problem since the tokens are randomly generated and by default already have 2^128 bits of entropy.

Note that the number-based tokens would not be secure, but I'm not convinced they're secure even with bcrypt, since they only have 4-bytes of entropy.

The other reason for hashing is to protect people's other accounts if their password is leaked, but since this is a random token, we don't have that problem either.

Hashing the token does one thing for security, which is preventing an attacker with read access to the database from logging in, but you could get just as much protection with one round of SHA-1.

In case you're thinking this doesn't matter:

  • Installing the bcrypt library adds a significant amount of complexity as shown by the -bcrypt-node versions of each backend
  • Running bcrypt with difficulty=10 slows auth down to the point where it can only handle around 2-4 requests per second, which is a massive slowdown for something that's completely unnecessary.
@iplanwebsites
Copy link

iplanwebsites commented Nov 1, 2019

+1 . Have you guys found a workaround to avoid using Bcrypt?

@netpedro-com
Copy link

@iplanwebsites consider replace passwordless-mongostore by passwordless-mongostore-bcrypt-node

@brendanlong
Copy link
Author

Removing this entirely might be complicated if you want to maintain backwards compatibility, but two easy fixes for the performance issue would be to replace the hard-coded difficulty of 10 with either the lowest-allowed value (probably 1 or 4), or add an option so people can set it themselves.

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

No branches or pull requests

3 participants