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

DRAFT: Changes the default hash function from md4 to sha-256 #1118

Closed
wants to merge 1 commit into from

Conversation

MymmiJ
Copy link

@MymmiJ MymmiJ commented Jan 20, 2023

MD4 is a legacy algorithm and therefore not supported by node 18. This simple fix ensures that node 18 can work with the codebase but does not fully fix the issue, taking npm install from 10% to 24% completion at the webpack:all stage.

There seems to be another issue somewhere in the codebase, I thought maybe ably-common but could not find any other issues there.

Info taken from:
Stack Overflow on initial error message - best answer is #3.
OpenSSL Legacy Algorithms

MD4 is a legacy algorithm and therefore not supported by node 18. This change ensures that
node 18 can work with the codebase
@SimonWoolf SimonWoolf requested review from owenpearson, Peter-Maguire and dpiatek and removed request for dpiatek, Peter-Maguire and owenpearson January 23, 2023 17:38
@Peter-Maguire
Copy link
Contributor

This seems to break webpack building on older versions of node, maybe there's an algorithm that is supported on both?

@MymmiJ
Copy link
Author

MymmiJ commented Jan 24, 2023

This seems to break webpack building on older versions of node, maybe there's an algorithm that is supported on both?

It doesn't fully work on the LTS either, it seems like the ModuleConcatenationPlugin in use causes issues but I can't find out where this can be changed.

I've tried AES and RSA as hashFunctions, but I think this is a point at which webpack is consciously deciding to leave older versions behind in particular for this issue, and there might not be a good, robust and simple solution. The 'best' code solution I can see is to try and replace the node core modules that use OpenSSL with polyfill classes.

The other solutions I see are to 1. drop support for node 18 and announce that it will be coming in ably-js v2, or 2. Use export NODE_OPTIONS=--openssl-legacy-provider somewhere in the Gruntfile (after exploring our/webpack's use of openssl; hopefully we are not using md4 in places where security is important anyway!)

@Peter-Maguire
Copy link
Contributor

We could add something to the README explaining that the library can only be built on node <18

@MymmiJ
Copy link
Author

MymmiJ commented Jan 24, 2023

We could add something to the README explaining that the library can only be built on node <18

This seems like the best option, will make reference to export NODE_OPTIONS=--openssl-legacy-provider in README comments also.

@MymmiJ MymmiJ closed this Jan 24, 2023
@owenpearson
Copy link
Member

@MymmiJ fyi documentation about building the library should go in CONTRIBUTING.md. The distribution we publish to NPM and the CDN will continue to be built with node 16.x so no need to worry about security implications. Also, I've opened #1115 which adds Node 18.x testing to CI, probably better to do it like that than to change the Gruntfile and risk changing behaviour for other node versions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants