Skip to content

fix(crypto): Move to secure defaults for PBKDF2#107

Open
JoeNorth wants to merge 6 commits into
hapijs:masterfrom
JoeNorth:master
Open

fix(crypto): Move to secure defaults for PBKDF2#107
JoeNorth wants to merge 6 commits into
hapijs:masterfrom
JoeNorth:master

Conversation

@JoeNorth
Copy link
Copy Markdown

This library is used in a number of open source projects which commonly run in FIPS environments. As such, there are a number of restrictions on the algorithms that can and can't be used. In this case, with the move to FIPS 140-3, HMAC-SHA1 is not an approved algorithm.

The library does not currently make this a configurable option which this PR does not change however it does move from HMAC-SHA1 to HMAC-SHA256 so that it is more broadly usable.

Additionally, the default iteration count of 1 for both encryption and integrity does not align with best practices. This PR increases the default to 10000 as a more secure default.

HMAC-SHA1 is no longer an approved algorithm for use in FIPS
environments. Since this is not a configurable option this should use a
secure default.
The default of 1 iterations is not a secure default. Increasing this to
align with industry standards as a secure default.
Resolves issue with test "exposes all methods and classes as named
imports'
Previously tested a node-version specific message. Updated to use a
stable prefix instead.
@damusix
Copy link
Copy Markdown

damusix commented Apr 20, 2026

@JoeNorth thanks for the contribution. Can you check this last detail from the CI failures for backward compatibility?

@Marsup we should upgrade the test matrix from 22 forward for this repo. Node 26 release this year and Node 20 ends LTS.

For backwards compatibility with older versions of node.
@JoeNorth
Copy link
Copy Markdown
Author

The latest changes passed locally going back to node 14 but at that point there are no arm64 builds that I can test with.

Copy link
Copy Markdown

@kanongil kanongil left a comment

Choose a reason for hiding this comment

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

Thanks for the suggestions, but I don't see how these changes materially increases security.

This password is not intended to be anything that can be remembered, but rather a token that the server keeps to encrypt, decrypt and validate the passed secrets / iron tokens. The main difference from regular PBKDF2 usage for user passwords, is that we don't care that the password can potentially be derived from the hashed value. If a hacker obtains the hashed value, it is already game over. As such, the defaults are quite sound. With minPasswordlength: 32 and assuming random letters and numbers the effective key bit count is around 192. Changing the digest algorithm or iteration count only multiples the effort, but retains the effective bit count.

Regardless of the security aspects of this PR, we cannot accept it as-is, since it is impossible to unseal() tokens issued with the current release.

@kanongil
Copy link
Copy Markdown

Oh wait, I forget that the SHA-1 output is only 160 bits. As such, SHA-256 is a definite upgrade, but the breaking change is still an issue.

@kanongil
Copy link
Copy Markdown

I expect the solution for backwards compatibility, is to add an extra option that allows callers to customise the digest algorithm, with a default to the current SHA-1 choice. The default can then be changed in a future breaking release.

Note that this will still be a problem for migrations, because the implementation only supports rotating passwords, not changing the secret derivation algorithm.

@JoeNorth
Copy link
Copy Markdown
Author

@kanongil would a major version bump be an option to get this released?

For a bit more context on this, this library is in use by some large OSS projects like OpenSearch Dashboards and Elastic’s Kibana. When used in a FIPS environment that has to comply with newer changes around minimum iteration counts and the deprecation of HMAC-SHA1 it’s a bit of a bind.

The iteration count can be fixed easy enough in those downstream projects by overriding the defaults but the hardcoded SHA1 requires rebuilding from source with a patch to change to SHA256.

@JoeNorth
Copy link
Copy Markdown
Author

Good timing on this. An option to change from SHA1 to SHA256 would be a big help.

@kanongil
Copy link
Copy Markdown

kanongil commented Apr 21, 2026

Yes, a digestAlgorithm option (with default sha1) next to iterations is probably the best way to handle this right now.

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.

3 participants