-
Notifications
You must be signed in to change notification settings - Fork 10.4k
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
Update and rename bcryptpasswords.md to passwordhashing.md #775
Conversation
Presenting the tradeoffs between the IETF's and OWASP's top recommendations, as well as adding mention of making your implementation unique.
This looks great! Someone or myself will be able to review it when we can 👍 |
This looks good to me. If you have some time @lirantal could you take a second look, since you were across some of the earlier discussion for this practice? |
Co-authored-by: Kyle Martin <kyle.martin@hotmail.co.nz>
Co-authored-by: Kyle Martin <kyle.martin@hotmail.co.nz>
Co-authored-by: Kyle Martin <kyle.martin@hotmail.co.nz>
Co-authored-by: Kyle Martin <kyle.martin@hotmail.co.nz>
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.
@josh-hemphill Welcome aboard, this is truly an amazing drill-down💜
@js-kyle @josh-hemphill One fundamental suggestion - The new version not only makes this much more exhaustive and advanced but also might make it harder for the average developer to extract actionable items. I would choose the 2-3 core messages including the action item (e.g. use Bcrypt or Script, whatever you think). This shouldn't be longer than 2 paragraphs, we call it "One paragraph" but we're developers and start counting from zero :) Anyway, it should be shorter and simpler than what we have now.
Then all the great advanced content and elaboration should come at the bottom under a title 'Advanced and references'.
Resonates with you?
FYI @js-kyle @goldbergyoni added some comments above |
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.
@josh-hemphill Looks more than perfect to me, amazing work 💜. I believe that this will help 80% of the developers to avoid fatal mistakes and allow the 20% to understand much better and make thoughtful decisions. Consider changing the title, make it more tangible and punchy, see my comment.
This will get merged in 24 hours. Obviously adding you now to the contributor's board, Tweeting about it and putting in the news. What's your Twitter handle?
@js-kyle Great coordination and review
@lirantal You're the king
7bbda49
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.
LGTM
@all-contributors please add @josh-hemphill for content |
I've put up a pull request to add @josh-hemphill! 🎉 |
Update and rename bcryptpasswords.md to passwordhashing.md
closes #325
Presenting the tradeoffs between the IETF's and OWASP's top recommendations, as well as adding mention of making your implementation unique to make your hash different from other's if given the same password.
Mentioned to use IETF's default parameters for the functions.
I thought about saying more about Salt generation, but it was already getting a bit verbose. So any suggestions would be helpful.
I should also mention, the linked issue's original point doesn't apply to the actual
bcrypt
npm module (removed suggesting "other" bcrypt modules), but the discussion deviated into more generally providing the situationality of the top recommended algorithms.