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

Proposal: crypto: Make the digest parameter required for pbkdf2. #3292

Closed
wants to merge 1 commit into from

Conversation

tomgco
Copy link
Contributor

@tomgco tomgco commented Oct 9, 2015

This change will remove the default behaviour of defaulting to SHA1
as its digest function, instead this will break backwards compatibility
and force implementers to choose a secure digest that is suited for them
at that time.

At this moment of time SHA1 is fine to be used as an HMAC, but this statement
will eventually not be true, as is the nature of cryptography. Instead of changing the default
hash algorithm when we deem one to be not secure (and thus break backwards
compatibility at each change in the future), implementing this fix now (v5?) will remove any future
breakages as it pushes the choice onto the consumer of this api.

I would like to hear your thoughts on this, I understand that this sub-module is at the stability index of 2: stable, however I still think that this should be changed now, instead of waiting till it has to be changed.

Thanks.

This change will remove the default behaviour of defaulting to SHA1
as its digest function, instead this will break backwards compatibility
and force implementors to choose a secure digest that is suited for them
at that time.
@bnoordhuis
Copy link
Member

Deprecating the 'default digest' overload might be reasonable (discussion needed) but it has to go through a deprecation cycle first to give people time to upgrade, i.e., v5 prints a warning, v6 throws.

@bnoordhuis bnoordhuis added crypto Issues and PRs related to the crypto subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Oct 9, 2015
@tomgco
Copy link
Contributor Author

tomgco commented Oct 9, 2015

Deprecating the 'default digest' overload might be reasonable (discussion needed) but it has to go through a deprecation cycle first to give people time to upgrade, i.e., v5 prints a warning, v6 throws.

Shall I create a new PR with the deprecation notice and link it to this issue?

@bnoordhuis
Copy link
Member

I'll leave that up to you. If your reasoning is that this PR can go into v6 once the merge window for that opens, keep in mind that the chances of it applying cleanly will be slim.

tomgco added a commit to tomgco/node that referenced this pull request Jan 22, 2016
As per nodejs#3292, this PR introduces a deprecation notice about removing
the 'default digest' overload which currently defaults to the soon
to be defunct SHA1 digest.

Instead it should be left up to the documentation and implementor to
suggest a suitable digest function.
shigeki pushed a commit that referenced this pull request Jan 22, 2016
As per #3292, this PR introduces a deprecation notice about removing
the 'default digest' overload which currently defaults to the soon
to be defunct SHA1 digest.

Instead it should be left up to the documentation and implementor to
suggest a suitable digest function.

Ref: #3292
PR-URL: #4047
Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
@shigeki
Copy link
Contributor

shigeki commented Jan 22, 2016

Landed in a116358. See #4047

@shigeki shigeki closed this Jan 22, 2016
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
As per nodejs#3292, this PR introduces a deprecation notice about removing
the 'default digest' overload which currently defaults to the soon
to be defunct SHA1 digest.

Instead it should be left up to the documentation and implementor to
suggest a suitable digest function.

Ref: nodejs#3292
PR-URL: nodejs#4047
Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants