-
Notifications
You must be signed in to change notification settings - Fork 28.9k
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
deps: switch openssl to quictls/openssl #37601
Conversation
This comment has been minimized.
This comment has been minimized.
@richardlau ... I'm not sure I would label this semver-major, actually. semver-minor yes, but there are no breaking changes other than, perhaps, the version number identifier. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
This is great news, thanks @jasnell and the whole Node team! |
@jasnell if this is semver-major will replacing the fork also be semver-major? |
@MylesBorins I don't think it should be semver major. |
@jasnell I've replaced the semver-major tag with semver-minor + dont-land-on-* tags. |
https://github.com/nodejs/node/blob/master/doc/guides/maintaining-openssl.md probably should be updated. It would also be good to point to a specific commit/tag rather than the branch https://github.com/quictls/openssl/tree/OpenSSL_1_1_1j+quic in case of divergence (e.g. more quic commits added to the branch). |
@richardlau ... yeah, I've got those edits pending, I'm just making sure that CI passes in case there are any other bits that need to be changed (there shouldn't be but just in case). |
@richardlau one point... the repo is not yet tagging specific releases, and for now, we absolutely want to work forward with the tip of the development in the branch, so I'll avoid pointing to a specific commit or tag for now. Later that will anchor to a specific tag, however. |
3c1fd9e
to
abc2768
Compare
@jasnell The reason to prefer specific commit/tag is to make it easier to determine if any upstream (quic) fixes are in our code or are waiting for another update (i.e. we're now behind). So as long as that's done before this lands I'm good. |
A few things I think we need to answer/address before landing this include:
|
Yes, the idea there would be to default set the
See the discussion on this in the original PR post. @richsalz might be able to weigh in a bit more here.
Discussed in the PR post. I'm not sure how/where to add documentation to that effect. |
Ah right, yeah, will note that for sure. I'm actually waiting for another commit to land before moving forward with this to land so once that lands up stream and I pull it in here I'll note the commit sha reference in the commit message. |
Please take a look at the README/FAQ at https://github.com/quictls/openssl and see if it answers your questions. If not, please open an issue. It also talks about different SONAME and |
I will be updating the quictls/openssl commit in this PR later on today with the commit sha |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rubber Stamp LGTM
I will give it another couple of days for review, then if there are no objections, I'll land on Monday. |
As of quictls/openssl@0c70d48 Source: https://github.com/quictls/openssl/tree/OpenSSL_1_1_1j+quic Signed-off-by: James M Snell <jasnell@gmail.com> PR-URL: #37601 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
Signed-off-by: James M Snell <jasnell@gmail.com> PR-URL: #37601 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
PR-URL: #37601 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
PR-URL: #37601 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
Landed in 66f0eb7...7abdc3e Heads up to all @nodejs/crypto folks... the changes the source of openssl updates! Please see the updating guide for details. |
As of quictls/openssl@0c70d48 Source: https://github.com/quictls/openssl/tree/OpenSSL_1_1_1j+quic Signed-off-by: James M Snell <jasnell@gmail.com> PR-URL: #37601 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
Signed-off-by: James M Snell <jasnell@gmail.com> PR-URL: #37601 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
PR-URL: #37601 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
PR-URL: #37601 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
Notable changes: * crypto: * add optional callback to crypto.sign and crypto.verify (Filip Skokan) #37500 * support JWK objects in create\*Key (Filip Skokan) #37254 * deps: * update archs files for OpenSSL-1.1.1+quic (James M Snell) #37601 * switch openssl to quictls/openssl (James M Snell) #37601 * doc: * update maintaining-openssl guide (James M Snell) #37601 * fs: * improve fsPromises writeFile performance (Nitzan Uziely) #37610 * add promisified readFile benchmark (Nitzan Uziely) #37608 * improve fsPromises readFile performance (Nitzan Uziely) #37608 * lib: * implement AbortSignal.abort() (James M Snell) #37693 * node-api: * define version 8 (Gabriel Schulhof) #37652 * test: * update dom/abort tests (James M Snell) #37693 * fixup test to account for quic openssl version (James M Snell) #37601 * worker: * add setEnvironmentData/getEnvironmentData (James M Snell) #37486
PR-URL: #37766 Notable changes: * crypto: * add optional callback to crypto.sign and crypto.verify (Filip Skokan) #37500 * support JWK objects in create\*Key (Filip Skokan) #37254 * deps: * update archs files for OpenSSL-1.1.1+quic (James M Snell) #37601 * switch openssl to quictls/openssl (James M Snell) #37601 * doc: * update maintaining-openssl guide (James M Snell) #37601 * fs: * improve fsPromises writeFile performance (Nitzan Uziely) #37610 * add promisified readFile benchmark (Nitzan Uziely) #37608 * improve fsPromises readFile performance (Nitzan Uziely) #37608 * lib: * implement AbortSignal.abort() (James M Snell) #37693 * node-api: * define version 8 (Gabriel Schulhof) #37652 * test: * update dom/abort tests (James M Snell) #37693 * fixup test to account for quic openssl version (James M Snell) #37601 * worker: * add setEnvironmentData/getEnvironmentData (James M Snell) #37486
As of quictls/openssl@0c70d48 Source: https://github.com/quictls/openssl/tree/OpenSSL_1_1_1j+quic Signed-off-by: James M Snell <jasnell@gmail.com> PR-URL: #37601 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
Signed-off-by: James M Snell <jasnell@gmail.com> PR-URL: #37601 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
PR-URL: #37601 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
PR-URL: #37601 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
PR-URL: #37766 Notable changes: * crypto: * add optional callback to crypto.sign and crypto.verify (Filip Skokan) #37500 * support JWK objects in create\*Key (Filip Skokan) #37254 * deps: * update archs files for OpenSSL-1.1.1+quic (James M Snell) #37601 * switch openssl to quictls/openssl (James M Snell) #37601 * doc: * update maintaining-openssl guide (James M Snell) #37601 * fs: * improve fsPromises writeFile performance (Nitzan Uziely) #37610 * add promisified readFile benchmark (Nitzan Uziely) #37608 * improve fsPromises readFile performance (Nitzan Uziely) #37608 * lib: * implement AbortSignal.abort() (James M Snell) #37693 * node-api: * define version 8 (Gabriel Schulhof) #37652 * test: * update dom/abort tests (James M Snell) #37693 * fixup test to account for quic openssl version (James M Snell) #37601 * worker: * add setEnvironmentData/getEnvironmentData (James M Snell) #37486
PR-URL: #37766 Notable changes: * crypto: * add optional callback to crypto.sign and crypto.verify (Filip Skokan) #37500 * support JWK objects in create\*Key (Filip Skokan) #37254 * deps: * switch openssl to quictls/openssl (James M Snell) #37601 * fs: * improve fsPromises writeFile performance (Nitzan Uziely) #37610 * improve fsPromises readFile performance (Nitzan Uziely) #37608 * lib: * implement AbortSignal.abort() (James M Snell) #37693 * node-api: * define version 8 (Gabriel Schulhof) #37652 * worker: * add setEnvironmentData/getEnvironmentData (James M Snell) #37486
PR-URL: #37766 Notable changes: * crypto: * add optional callback to crypto.sign and crypto.verify (Filip Skokan) #37500 * support JWK objects in create\*Key (Filip Skokan) #37254 * deps: * switch openssl to quictls/openssl (James M Snell) #37601 * fs: * improve fsPromises writeFile performance (Nitzan Uziely) #37610 * improve fsPromises readFile performance (Nitzan Uziely) #37608 * lib: * implement AbortSignal.abort() (James M Snell) #37693 * node-api: * define version 8 (Gabriel Schulhof) #37652 * worker: * add setEnvironmentData/getEnvironmentData (James M Snell) #37486
PR-URL: #37766 Notable changes: * crypto: * add optional callback to crypto.sign and crypto.verify (Filip Skokan) #37500 * support JWK objects in create\*Key (Filip Skokan) #37254 * deps: * switch openssl to quictls/openssl (James M Snell) #37601 * fs: * improve fsPromises writeFile performance (Nitzan Uziely) #37610 * improve fsPromises readFile performance (Nitzan Uziely) #37608 * lib: * implement AbortSignal.abort() (James M Snell) #37693 * node-api: * define version 8 (Gabriel Schulhof) #37652 * worker: * add setEnvironmentData/getEnvironmentData (James M Snell) #37486
PR-URL: #37766 Notable changes: * crypto: * add optional callback to crypto.sign and crypto.verify (Filip Skokan) #37500 * support JWK objects in create\*Key (Filip Skokan) #37254 * deps: * update to cjs-module-lexer@1.1.0 (Guy Bedford) #37712 * switch openssl to quictls/openssl (James M Snell) #37601 * fs: * improve fsPromises writeFile performance (Nitzan Uziely) #37610 * improve fsPromises readFile performance (Nitzan Uziely) #37608 * lib: * implement AbortSignal.abort() (James M Snell) #37693 * node-api: * define version 8 (Gabriel Schulhof) #37652 * worker: * add setEnvironmentData/getEnvironmentData (James M Snell) #37486
PR-URL: #37766 Notable changes: * crypto: * add optional callback to crypto.sign and crypto.verify (Filip Skokan) #37500 * support JWK objects in create\*Key (Filip Skokan) #37254 * deps: * update to cjs-module-lexer@1.1.0 (Guy Bedford) #37712 * switch openssl to quictls/openssl (James M Snell) #37601 * fs: * improve fsPromises writeFile performance (Nitzan Uziely) #37610 * improve fsPromises readFile performance (Nitzan Uziely) #37608 * lib: * implement AbortSignal.abort() (James M Snell) #37693 * node-api: * define version 8 (Gabriel Schulhof) #37652 * worker: * add setEnvironmentData/getEnvironmentData (James M Snell) #37486
/cc @nodejs/tsc @nodejs/crypto
Yesterday, Akamai and Microsoft announced that they are providing, and will be supporting, a temporary supported fork of OpenSSL 1.1.1 and 3.0.0 that includes the necessary bits to support QUIC implementations.
See @richsalz's tweet here for details: https://twitter.com/RichSalz/status/1367349918671773697
Critically, this fork is intended only as a stopgap to support platforms that wish to deliver QUIC support while the main OpenSSL project is focused on the release of 3.0.0. The only differences this fork will have relative to the main OpenSSL project is the additional pieces necessary for QUIC.
Because both Akamai and Microsoft are using this code in production, they are committing to keeping it up to date with the main OpenSSL releases, so there should not be any lag time between upstream updates and updates in the fork.
Further, Akamai and Microsoft will be requiring that any contributions made to this fork are limited to the QUIC support, and that all other non-QUIC related changes have to be made upstream in the main project. All potential contributors will have to sign and submit the OpenSSL Contributors License Agreement (which I will be doing myself later today).
Now to the point: This pull request replaces the vendored-in OpenSSL distribution in core with the Akamai/Microsoft fork. This does not yet re-introduce the QUIC implementation in Node.js but does make the underlying APIs to do so available by default. If this gets approval and lands, I will start reintroducing the QUIC subsystem in Node.js incrementally over the next few weeks (I have a number of implementation improvements that I've been working on in the interim).
With this version of OpenSSL vendored into core, the QUIC support would be enabled by default as opposed to being opt-in as it was before. For operating systems that use their own distros of Node.js that use the operating system provided openssl (such as RedHat), the QUIC support would not be available unless those distributions provide the option to install the QUIC-enabled openssl alternative.
I'm not sure if it's in there yet, but if not I will be openingI have opened a PRsoon to add it, butthat adds a#DEFINE
flagwillto be used to conditionally disable building the QUIC support on such platforms.As for ongoing support, as I mentioned, I will be submitting a signed OpenSSL CLA later today, and will be committing to supporting this for as long as necessary, helping to resolve any issues that may come up while using the fork.
It should be highlighed that for as long as the QUIC subsystem in Node.js is considered experimental, direct use by any Node.js Native Addons of the additional QUIC APIs offered by the fork would also be considered experimental and At Your Own Risk.
Obviously, Long Term Support is a consideration here and I have raised that as a concern in discussions with Akamai and Microsoft. I have communicated to them that if this fork ends up going out in an LTS covered release line that we would need to commit to 30 months of maintenance. While they have not made any guarantees, they are willing to work with us on this, especially if we contribute back to help with that maintenance (thus, part of the reason I'll be signing the CLA). Also, as I said, both companies are using this in production systems today and have their own support and maintenance requirements. Bottom line is that LTS should not be a problem here, especially given that the QUIC support will be considered experimental for quite some time as it matures.
The eventual long term goal is to switch back to the official OpenSSL release once it does include the necessary QUIC support. That will be some time after the 3.0.0 release, with no clear estimate on when that will be.
Source: https://github.com/quictls/openssl/tree/OpenSSL_1_1_1j+quic
Signed-off-by: James M Snell jasnell@gmail.com