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

doc: Fix 'pbkdf2Sync' encoding problem #1805

Merged
2 commits merged into from Sep 17, 2018
Merged

doc: Fix 'pbkdf2Sync' encoding problem #1805

2 commits merged into from Sep 17, 2018

Conversation

ghost
Copy link

@ghost ghost commented Sep 13, 2018

Ref: #1796.

Since the '/newUser' request is encoded by 'sha512', and your
corresponding '/auth' must also use 'sha512' as the digest encoding
as well. Otherwise your decoding isn't right.

@ghost
Copy link
Author

ghost commented Sep 13, 2018

/cc:@trevorah

@ghost ghost requested review from tniessen and lpinca September 13, 2018 06:06
@vsemozhetbyt
Copy link
Contributor

cc @nodejs/crypto

@trevorah
Copy link
Contributor

😳 whoops, sorry I didnt notice that in my original PR. Good spot @Maledong!

Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated: 512 is an unusual key size (note that the key size parameter is in bytes, not bits). This might have originated from the misconception that it must match the output size of the hash function?

locale/en/docs/guides/simple-profiling.md Outdated Show resolved Hide resolved
@trevorah
Copy link
Contributor

@tniessen I picked sha512 for this guide because its used in the crypto examples (https://nodejs.org/api/crypto.html#crypto_crypto_pbkdf2_password_salt_iterations_keylen_digest_callback) which this guide appears to mirror.

@tniessen
Copy link
Member

@trevorah SHA512 is a good choice, just the output key size is unusual (512 bytes!). This works equally well:

crypto.pbkdf2Sync('password', 'salt', 10000, 32, 'sha512')

PBKDF2 is usually used to derive symmetric keys or to store passwords, neither of these applications requires keys with a size of 512 bytes.

@trevorah
Copy link
Contributor

@tniessen hmm... weird. The only crypto.pbkdf2* examples that use a 512 key size are the ones that set crypto.DEFAULT_ENCODING = 'hex'. All the others use a 64 byte key size. Maybe the original author wanted to show some difference when setting DEFAULT_ENCODING?

@tniessen
Copy link
Member

tniessen commented Sep 13, 2018

Uhh DEFAULT_ENCODING... What a design mistake. It still doesn't really matter, if you set the DEFAULT_ENCODING to hex, the output will still be a 512-byte (1024 hex digits) sequence, just encoded. But again, it is unrelated, so doesn't need to be changed in this PR. Also, it's not wrong, just unusual.

Ref: #1796.
Since the '/newUser' request is encoded by 'sha512', and your
corresponding '/auth' must also use 'sha512' as the digest encoding
as well.
@ghost
Copy link
Author

ghost commented Sep 13, 2018

@trevorah:No problem. Just found that by accident when reading lines :)

@ghost ghost requested review from vsemozhetbyt and fhemberger September 14, 2018 02:41
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doc format LGTM)

@ghost ghost merged commit 9f95cf9 into nodejs:master Sep 17, 2018
@ghost ghost deleted the FixMissingcryto branch September 17, 2018 01:20
@ghost
Copy link
Author

ghost commented Sep 17, 2018

Thanks all!

This pull request was closed.
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.

None yet

5 participants