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

consider removing salt #6

Open
nvdk opened this issue Nov 10, 2021 · 2 comments
Open

consider removing salt #6

nvdk opened this issue Nov 10, 2021 · 2 comments

Comments

@nvdk
Copy link
Member

nvdk commented Nov 10, 2021

while this would be a breaking change, it does not seem necessary to add a salt to the password as the bcrypt algorithm already does this itself. As I understand the salt is included in the resulting hash.

Motivation for the removal:

  • Removing the application salt limits the amount of configuration that is required to set up this service
  • Removing the salts also allows us to simplify the service a tiny bit
  • the bcrypt algorithm is limited to 72 bytes of input, anything after is truncated, for longer passwords only a short part of the salt is included anyway
@aatauil
Copy link

aatauil commented Nov 29, 2021

@nvdk worth looking at this comment aatauil/mu-node-authentication-service#1 (comment) I left it out of the js version as I also thought the bcrypt salt would be enough but apparently there is still a possible vulnerability to rainbow tables

@nvdk
Copy link
Member Author

nvdk commented Dec 2, 2021

I don't think that analysis holds for bcrypt as it creates and stores its own salt. I mean for long passwords the input salt won't even be considered for the hash, since only the first 72 bytes count.

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

2 participants