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

crypto.pbkdf2 with digest param #6

Closed
wants to merge 1 commit into from
Closed

crypto.pbkdf2 with digest param #6

wants to merge 1 commit into from

Conversation

aegyed91
Copy link

Hi @florianheinemann,

i recently updated to node.js v6.0.0 and i see (node:21314) DeprecationWarning: crypto.pbkdf2 without specifying a digest is deprecated. Please specify a digest.

By specifying a digest parameter this log goes away.

@aegyed91
Copy link
Author

aegyed91 commented May 2, 2016

pbkdf2 digest overloading is deprecated #4047

https://nodejs.org/en/blog/release/v6.0.0/?utm_source=nodeweekly&utm_medium=email#notable-changes

@florianheinemann
Copy link
Owner

Hi!

Thanks for the PR! Before fixing the digest to sha512 - I believe it would be great to check if this is the most appropriate one and if this will run on most systems. Also: We might want to leave the option for people to overwrite the default digest.

Any thoughts?

Cheers

@aegyed91
Copy link
Author

aegyed91 commented May 3, 2016

Hi, no problem

i assume whichever digest you go with, it should be working fine on all OS

actually if we choose that path then the iteration number and the keylen could be overridden too. But then you will need to override those options in the verifyAgainst method too.

one more thing, check this out. this is your script rewritten

i think an api like this would be better:

hashPassword(password, [salt], [options])
verifyPassword(password, hashedPassword, [options])

and it should return a promise if you dont specify a callback, with bluebird's .asCallback maybe or without bluebird as a dep. But it is a lot more effective than the native promise implementation

@florianheinemann
Copy link
Owner

See #7 Thank you!

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.

2 participants