fix: Improve heuristic for --openssl-legacy-provider (#19320) #19522
Conversation
|
Thanks for taking the time to open a PR!
|
eagleflo
requested review from
jennifer-shehane
and removed request for
a team
|
|
packages/server/lib/plugins/index.js
Outdated
| // unconditionally, but it doesn't exist if Node was built against OpenSSL | ||
| // version 1.x. | ||
| if (semver.satisfies(config.resolvedNodeVersion, '>=17.0.0') && | ||
| !process.versions.openssl.startsWith("1")) { |
There was a problem hiding this comment.
Given this issue is caused by Cypress's usage of wepback 4 + Node 17, the "best" solution for this issue would be to upgrade to webpack 5. I am looking into that work, however, for a (hopefully temporary) go-forward I think you change is on the right track since webpack uses the md4 cyrpto algorithm internally.
I am wonder if we need to make this solution a bit more robust though. The original change here assumed default the OpenSSL Library provided in Node 17 to be the OpenSSL 3.0.0 which introduced the --openssl-legacy-provider flag. Your change is assuming assuming Node 17 + OpenSSL 1.x.
In your summary / linked issues to Cloudflare, you noted that Node can be built with the --shared-openssl build parameter. This lead me to look further into Node's build options & theirbuild documentation. It looks like the OpenSSL library / configuration( & the cyrpto library) could be pretty open-ended if someone wanted to build node with custom paths.
Would it be possible for users to use a custom OpenSSL library that doesn't accept the --openssl-legacy-provider flag? Would that cause process.versions.openssl to be undefined?
There was a problem hiding this comment.
Also, can we update the existing tests to add a validation check for this?
There was a problem hiding this comment.
Your change is assuming assuming Node 17 + OpenSSL 1.x.
My change does not assume Node 17 + OpenSSL 1.x, it just checks for that possibility and skips adding the option if that happens to be the case. The option is still applied on Node 17 + OpenSSL 3.x.
I fully agree that the best course of action for Cypress would be to upgrade to Webpack 5.61+. It's the right thing to do regardless from project health viewpoint. This whole workaround could then be removed.
Regarding uncertainty about building Node with custom versions of OpenSSL, Node.js core team says building against any shared libraries is effectively unsupported. I wouldn't support anything beyond OpenSSL 1.x and OpenSSL 3.x for this hopefully temporary fix.
If you wanted to make this more robust, I'd require crypto and check whether crypto.constants.OPENSSL_VERSION_NUMBER > 0x30000000. But that's essentially a more complicated way of testing the same thing.
Regarding cases where process.versions.openssl would be undefined, in theory it's possible to build Node.js without SSL at all. (This is obviously unsupported by most of the ecosystem and no major distro ships Node.js built like this.) Via this issue we've established that Cypress transitively relies on crypto being available; if you wanted to check for that, I'd do it much earlier.
There was a problem hiding this comment.
My change does not assume Node 17 + OpenSSL 1.x, it just checks for that possibility and skips adding the option if that happens to be the case.
To me it seems like it might be safer to check for the possibility of OpenSSL 3.x and add it if its there. @mjhenkes thoughts?
There was a problem hiding this comment.
This option will be needed with future versions of OpenSSL until you upgrade Webpack. If OpenSSL were to increase their major version again checking explicitly for OpenSSL 3 would break.
There was a problem hiding this comment.
True, but there's no guarantee the next MVB of OpenSSL will support the Legacy Provider and it's corresponding --openssl-legacy-provider flag either. 🤷🏻♀️ This is good conversation btw. I appreciate your responsiveness.
There was a problem hiding this comment.
I appreciate you making sure it's the appropriate fix.
We know for sure that the --openssl-legacy-provider option is not there with OpenSSL version 1.x. I also want to emphasize that we are now fixing existing breakage -- something many people have now encountered in the wild, even based on reports from the very small subset of users that engage with the project via GitHub.
We have no idea when or if OpenSSL 4 will be released -- and by the time OpenSSL 4 might be released, I really truly hope Cypress has long since moved on from webpack@4 and removed this workaround altogether!
Regarding support for the legacy provider, they just added it in OpenSSL 3 to draw a clear line between believed-safe and known-broken algorithms and ciphers. They had the opportunity to completely throw these algorithms away here, but they chose to keep supporting them to let their clients decide when and how to upgrade. They wisely mitigated the risk by not loading this provider by default and having their clients explicitly opt-in to it.
I would not expect this legacy provider to disappear anytime soon, as otherwise the currently clear upgrade path from OpenSSL 1.x (which is quite literally everywhere, for years to come) would suffer. This is of vital importance to the OpenSSL team since there still are multiple CVEs found in OpenSSL every year -- they need to keep providing as smooth upgrade experience as possible.
(I suspect that's also the reason why many rolling release distros have chosen to build Node.js with --shared-openssl in the first place -- they believe they will be better off updating OpenSSL systemwide rather than relying on the Node.js team for a new build. Unfortunately these very infrequent big OpenSSL upgrades then take more time to roll out since you have to fix and rebuild large parts of the whole distro. Arch Linux is looking to upgrade at the beginning of this year.)
There was a problem hiding this comment.
I suppose one other way of solving this would be to copy the approach I took in fixing wrangler by spawning node --help and grepping for --openssl-legacy-provider. It's going to be slower and much harder to test, but maybe slightly more robust.
I have no insight into your plans and schedule wrt. webpack 5 adoption, so if this workaround is going to stay here for a longer while it might be wiser to take that approach. Who knows how long Node.js will keep that option around.
There was a problem hiding this comment.
We can go head with what is here -- a new node process would be overkill. Based on your response, we know for sure --openssl-legacy-provider option is not supported with OpenSSL version 1.x and could potentially be supported in OpenSSL >3.x. Like you said v4 likely won't come quickly and we should be (fingers crossed) on a newer version of webpack by the time this would happen.
eagleflo
force-pushed
the
issue-19320
branch
3 times, most recently
from
16cb477 to
02db9c7
Compare
User facing changelog
--openssl-legacy-provideris neededAdditional details
Node 17+ can still be built against OpenSSL version 1 with
--shared-opensslbuild parameter. In this case the option--openssl-legacy-providerdoes not work (and is not needed). We cannot assume that all versions of Node 17 are built with OpenSSL 3.How has the user experience changed?
Native Node v17 as built by Arch Linux and Homebrew now works with Cypress again (after being broken in 9.1.1).
PR Tasks
cypress-documentation?type definitions?cypress.schema.json?