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

changed password hash to bcrypt fixes #454 #457

Closed

Conversation

singhotto
Copy link

fixes #454

I included the bcrypt library for password hashing, but the database already contains passwords hashed with the password-hash library. Instead of removing the old library, I've kept it, and now the verification process utilizes both libraries.

Copy link

linux-foundation-easycla bot commented Feb 27, 2024

Copy link

netlify bot commented Feb 27, 2024

Deploy Preview for endearing-brigadeiros-63f9d0 ready!

Name Link
🔨 Latest commit 77b2997
🔍 Latest deploy log https://app.netlify.com/sites/endearing-brigadeiros-63f9d0/deploys/65df1b6ef8618800082e459e
😎 Deploy Preview https://deploy-preview-457--endearing-brigadeiros-63f9d0.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@maoo
Copy link
Member

maoo commented Feb 27, 2024

Hi @singhotto , thanks for your PR, and welcome to FINOS!

I did implement the fix on https://github.com/finos/git-proxy/pull/453/files ; would you mind reviewing it and rebasing your PR against the fix-auth branch?

Also, please let me know if you encounter any issues with EasyCLA (comment above).

@singhotto
Copy link
Author

singhotto commented Feb 27, 2024

Can bcrypt verify already hashed passwords (existing users in db) by password-hash library?

Not sure I understand your question, sorry; we started looking at bcrypt mainly because password-hash is not maintained, and we don't want to bring security issues from unmaintained third-party libraries in our codebase; for this reason, we want to get rid of it entirely from the codebase.

That said, I like the way you use bcrypt in your PR, and I think it's cleaner than the approach I used, which is why I suggested you adding to my PR.

why do I have to do the easyCLA?

Long story short, to ensure that contributors are able to contribute their IP to us, and they're aware of it; this also leads to less legal risks for contributors. For the longer story, I suggest you reading https://osr.finos.org/docs/bok/artifacts/clas-and-dcos ; hope this answers your question.

@singhotto singhotto force-pushed the fix/changed-password-hash-to-bcrypt branch from 739a9d0 to 3a38dee Compare February 28, 2024 11:36
@maoo
Copy link
Member

maoo commented Feb 29, 2024

@singhotto - I just realised that I edited your comment, instead of replying. Apologies for that, I didn't realise it!

I've refined the fix-auth branch based on your feedback.

You raise a fair point about password already being hashed with password-hash, which cannot be verified with bcrypt; @JamieSlome @coopernetes - is this going to be a problem in your environments? Or can we accept this breaking change?

I appreciate a lot your help on this; would you be interested to continue contributing to this project? If that's the case, I'd be happy to jump on a quick call, figure our the EasyCLA setup and maybe discuss few other activities we're planning to tackle as a team? Feel free to ping me on maoo@finos.org , eager to keep this collaboration going!

@JamieSlome
Copy link
Member

Closing as addressed in #335 👍

Appreciate your support @singhotto and look forward to more of your support on the project ❤️

@JamieSlome JamieSlome closed this Mar 28, 2024
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

Successfully merging this pull request may close these issues.

Password-hash is deprecated, use bcrypt or scrypt instead
3 participants