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

Update hash function for node 17 compatibility #752

Closed
wants to merge 2 commits into from
Closed

Update hash function for node 17 compatibility #752

wants to merge 2 commits into from

Conversation

tobua
Copy link
Contributor

@tobua tobua commented Jan 6, 2022

When running a RN project currently with node 17 it will fail because a deprecated hash function is used with the node crypto library. The error is ERR_OSSL_EVP_UNSUPPORTED and will always appear unless a flag is explicitly set. Only the metro-cache package is affected. The stable version node 17 has been around for 3 months and will become active LTS in April of this year.

Summary

Switching from a 128 bit md4 hash to another 128 bit md5 hash that's safe is enough to fix the issue. However, note that I'm neither a hash function nor a metro cache expert.

Test plan

Tested with stable node 17, with LTS node 16 and also works fine with lowest supported node 12 version.
An already built project can be upgraded without the need to clear the cache.

Fixes #722

Couldn't find any related issues in the main RN repository.

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Jan 6, 2022
@robhogan
Copy link
Contributor

robhogan commented Jan 6, 2022

Thanks for the PR - and yep as confirmed below, clearly an issue with our (any) use of md4.

$ node -e "require('crypto').createHash('md4');"
node:internal/crypto/hash:67
  this[kHandle] = new _Hash(algorithm, xofLen);
                  ^

Error: error:0308010C:digital envelope routines::unsupported
    at new Hash (node:internal/crypto/hash:67:19)
    at Object.createHash (node:crypto:130:10)
    at [eval]:1:19
    at Script.runInThisContext (node:vm:129:12)
    at Object.runInThisContext (node:vm:305:38)
    at node:internal/process/execution:75:19
    at [eval]-wrapper:6:22
    at evalScript (node:internal/process/execution:74:60)
    at node:internal/main/eval_string:27:3 {
  opensslErrorStack: [ 'error:03000086:digital envelope routines::initialization error' ],
  library: 'digital envelope routines',
  reason: 'unsupported',
  code: 'ERR_OSSL_EVP_UNSUPPORTED'
}

Node.js v17.3.0

@facebook-github-bot
Copy link
Contributor

@rh389 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@robhogan
Copy link
Contributor

robhogan commented Jan 6, 2022

We switched from md5->md4 in a4c417a for a marginal performance gain.

Here's a very quick-and-dirty benchmark on node 16 on my MBP, which looks like a small (<4%) perf hit when purely hashing:

➜  ~ REPEATS=2000000 node bench1.js
md4 took 6.284265818953514s with 2000000 repeats
md5 took 6.490958640933036s with 2000000 repeats
sha1 took 6.5310690860748295s with 2000000 repeats
bench1.js
const crypto = require('crypto');
const { performance } = require('perf_hooks');

const content = JSON.stringify(performance); // length: 356

for (const algo of ['md4', 'md5', 'sha1']) {
  const start = performance.now();
  for (let i = 0; i < process.env.REPEATS; i++) {
	crypto.createHash(algo).update(content).digest();
  }
  console.log(`${algo} took ${(performance.now() - start) / 1000}s with ${process.env.REPEATS} repeats`);
}

But introducing JSON.stringify and canonicalize into the loop (as we do in stableHash) and that drops to around 2%.

~ REPEATS=2000000 node bench.js
md4 took 23.43095500099659s with 2000000 repeats
md5 took 23.90644327402115s with 2000000 repeats
sha1 took 24.07677574503422s with 2000000 repeats
bench2.js
const crypto = require('crypto');
const { performance } = require('perf_hooks');

function canonicalize(key, value) {
  if (
    value === null ||
    typeof value !== 'object' ||
    Array.isArray(value)
  ) {
    return value;
  }

  const keys = Object.keys(value).sort();
  const length = keys.length;
  const object = {};

  for (let i = 0; i < length; i++) {
    object[keys[i]] = value[keys[i]];
  }

  return object;
}

for (const algo of ['md4', 'md5', 'sha1']) {
  const start = performance.now();
  for (let i = 0; i < process.env.REPEATS; i++) {
	crypto.createHash(algo).update(JSON.stringify(performance, canonicalize)).digest();
  }
  console.log(`${algo} took ${(performance.now() - start) / 1000}s with ${process.env.REPEATS} repeats`);
}

@tobua
Copy link
Contributor Author

tobua commented Jan 6, 2022

@rh389 webpack switched from md4 to their own xxhash64 implementation because of the node 17 deprecation. I would assume that would be quite a bit faster (relies on WebAssembly) than md4 but the effort seems quite significant.

Update: Using your benchmark I get about a 30% speed-up with the xxhash-wasm npm package. I'm not sure if it has any effect when the hash length is cut in half.

@robhogan
Copy link
Contributor

robhogan commented Jan 6, 2022

xxhash-wasm looks interesting thanks, noted.

For now, apparently stableHash is no longer used for anything performance sensitive anyway, so this PR has a green light and will be merged in an hour or two. Thanks again!

@tobua
Copy link
Contributor Author

tobua commented Jan 6, 2022

@rh389 Awesome, appreciate the quick response!

nevilm-lt pushed a commit to nevilm-lt/metro that referenced this pull request Mar 14, 2022
Summary:
When running a RN project currently with node 17 it will fail because a deprecated hash function is used with the [node crypto library](https://nodejs.org/en/blog/release/v17.0.0/#openssl-3-0). The error is `ERR_OSSL_EVP_UNSUPPORTED` and will always appear unless a flag is explicitly set. Only the `metro-cache` package is affected. The stable version node 17 has been around for 3 months and will become active LTS in April of this year.

**Summary**

Switching from a 128 bit `md4` hash to another 128 bit `md5` hash that's safe is enough to fix the issue. However, note that I'm neither a hash function nor a metro cache expert.

**Test plan**

Tested with stable node 17, with LTS node 16 and also works fine with lowest supported node 12 version.
An already built project can be upgraded without the need to clear the cache.

Fixes facebook#722

Couldn't find any related issues in the main RN repository.

Pull Request resolved: facebook#752

Reviewed By: motiz88

Differential Revision: D33452659

Pulled By: rh389

fbshipit-source-id: f0e289017a9865b155ba88e5275980aeddcfe2ea
nevilm-lt pushed a commit to nevilm-lt/metro that referenced this pull request Apr 21, 2022
Summary:
When running a RN project currently with node 17 it will fail because a deprecated hash function is used with the [node crypto library](https://nodejs.org/en/blog/release/v17.0.0/#openssl-3-0). The error is `ERR_OSSL_EVP_UNSUPPORTED` and will always appear unless a flag is explicitly set. Only the `metro-cache` package is affected. The stable version node 17 has been around for 3 months and will become active LTS in April of this year.

**Summary**

Switching from a 128 bit `md4` hash to another 128 bit `md5` hash that's safe is enough to fix the issue. However, note that I'm neither a hash function nor a metro cache expert.

**Test plan**

Tested with stable node 17, with LTS node 16 and also works fine with lowest supported node 12 version.
An already built project can be upgraded without the need to clear the cache.

Fixes facebook#722

Couldn't find any related issues in the main RN repository.

Pull Request resolved: facebook#752

Reviewed By: motiz88

Differential Revision: D33452659

Pulled By: rh389

fbshipit-source-id: f0e289017a9865b155ba88e5275980aeddcfe2ea
nevilm-lt pushed a commit to nevilm-lt/metro that referenced this pull request Apr 22, 2022
Summary:
When running a RN project currently with node 17 it will fail because a deprecated hash function is used with the [node crypto library](https://nodejs.org/en/blog/release/v17.0.0/#openssl-3-0). The error is `ERR_OSSL_EVP_UNSUPPORTED` and will always appear unless a flag is explicitly set. Only the `metro-cache` package is affected. The stable version node 17 has been around for 3 months and will become active LTS in April of this year.

**Summary**

Switching from a 128 bit `md4` hash to another 128 bit `md5` hash that's safe is enough to fix the issue. However, note that I'm neither a hash function nor a metro cache expert.

**Test plan**

Tested with stable node 17, with LTS node 16 and also works fine with lowest supported node 12 version.
An already built project can be upgraded without the need to clear the cache.

Fixes facebook#722

Couldn't find any related issues in the main RN repository.

Pull Request resolved: facebook#752

Reviewed By: motiz88

Differential Revision: D33452659

Pulled By: rh389

fbshipit-source-id: f0e289017a9865b155ba88e5275980aeddcfe2ea
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

got ERR_OSSL_EVP_UNSUPPORTED when using node 17 and react 17 on expo
3 participants