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

Noobaa Account: Replace bcrypt password hashing by argon2 #8213

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

aspandey
Copy link
Contributor

@aspandey aspandey commented Jul 18, 2024

As bcrypt is not under active maintenance, we need to replace it with argon2 hashing module.

Explain the changes

We need to move to argon password hashing from bcrypt. bcrypt is not maintained very well which could cause issue in future.
This task will require two types of changes broadly.

1 - Modify the code so that whenever an account or password is created for an account, it should create a hash using argon. This change is straight forward. Wherever we are using bcrypt we have to replace it with argon2. Just a small difference in the way arguments are passed.

2 - The critical thing is to handle update. What happens when a user updates to this latest version.
All the previous passwords have been stored as bcrypt hash.

We can not write a upgrade script to modify these hashed password from bcrypt to argon.
To hash a password using argon we need original password in plain text, which we can not get just from bcrypt hash . It has to be coming from user.

This we can do in following ways -

  • As soon as user does upgrade, we need to ask users to reset password. Whenever a user resets password a new password hash will be generated by argon and it will replace the bcrypt password stored in database. Although, previous password will be authenticated by bcrypt first.

  • For this we have to keep bcrypt module inside the code for one release and once users are shifted to argon we can remove the dead code in next release.

@aspandey aspandey force-pushed the replace-bcrypt-with-argon branch 3 times, most recently from 5a86b22 to 9433070 Compare July 30, 2024 05:34
@aspandey
Copy link
Contributor Author

1 - I need to update the description of this PR based on our new approach in which we have just removed the bcrypt code.
2 - I need to some more testing.

@guymguym
Copy link
Member

guymguym commented Aug 1, 2024

@aspandey is there a specific need to use argon? I see that in recent years there is already support for password hashing natively in the nodejs crypto module, should we evaluate using it? - https://stackoverflow.com/questions/62908969/password-hashing-in-nodejs-using-built-in-crypto/67038052#67038052

@aspandey
Copy link
Contributor Author

aspandey commented Aug 5, 2024

@aspandey is there a specific need to use argon? I see that in recent years there is already support for password hashing natively in the nodejs crypto module, should we evaluate using it? - https://stackoverflow.com/questions/62908969/password-hashing-in-nodejs-using-built-in-crypto/67038052#67038052

Hey @guymguym I have read the difference between argon/bcrypt and node.js module...at first looks like argon is the better approach to use. However, I am thinking of implementing node.js password encrypting module and will send it as different PR.

As bcrypt is not under active maintenance, we need to
replace it with argon2 hashing module.

Signed-off-by: Ashish Pandey <aspandey@redhat.com>
@aspandey
Copy link
Contributor Author

aspandey commented Aug 5, 2024

@aspandey is there a specific need to use argon? I see that in recent years there is already support for password hashing natively in the nodejs crypto module, should we evaluate using it? - https://stackoverflow.com/questions/62908969/password-hashing-in-nodejs-using-built-in-crypto/67038052#67038052

Hey @guymguym I have read the difference between argon/bcrypt and node.js module...at first looks like argon is the better approach to use. However, I am thinking of implementing node.js password encrypting module and will send it as different PR.

PR with node.js crypto module - #8252

Copy link

github-actions bot commented Nov 3, 2024

This PR had no activity for too long - it will now be labeled stale. Update it to prevent it from getting closed.

@github-actions github-actions bot added the Stale label Nov 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants