-
Notifications
You must be signed in to change notification settings - Fork 134
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
Node.js OpenSSL Strategy #479
Conversation
OpenSSL-Strategy.md
Outdated
|
||
## Node.js versions 4 to 9 | ||
|
||
Identical copies of the latest OpenSSL 1.0.2 version are included in Node.js releases from versions 4 to 9. Aside from some manual configuration that is required in order to support GYP builds (instead of the Python-based Configure script that OpenSSL ships) as described in [deps/openssl/doc/UPGRADING.md](https://github.com/nodejs/node/blob/master/deps/openssl/doc/UPGRADING.md), there are 4 floating patches that are applied on top of the plain OpenSSL 1.0.2 source: |
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.
s/Python-based/Perl-based/
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.
resolved (only repo admins get a resolve button)
OpenSSL-Strategy.md
Outdated
* **nodejs.org**: Binaries produced by the Node.js Build infrastructure across all supported architectures use the default `configure` configuration for OpenSSL and therefore compile and statically link in the OpenSSL source as it ships with Node.js. It should be noted that nodejs.org is the source of binaries for **nvm** which is in heavy client use and is also used by Travis-CI to fetch and install Node.js. | ||
* **Linux Distributions**: Binaries shipped with Debian, Ubuntu, CentOS, RHEL, Fedora and the majority of other Linux distributions follow a standard policy of separating dependencies and dynamically linking wherever possible. Therefore, the Node.js packages on these systems are linked via the packaging dependency mechanisms to OpenSSL 1.0.2 packages and the `node` binaries that ship by default on these platforms use the shared OpenSSL 1.0.2 library installed on that by those packages. Therefore, the floating patches do not apply and it is possible that a different version of OpenSSL 1.0.2 is in use by Node.js than the version that was shipped in the source tree. | ||
* **NodeSource Linux Distributions**: Binaries shipped by this heavily used source do not follow Linux distribution policy and use the default `configure` configuration for OpenSSL and therefore compile and statically link in the OpenSSL source as it ships with Node.js. | ||
* **Homebrew**: The [formula](https://github.com/Homebrew/homebrew-core/blob/master/Formula/node.rb) for Node.js as distributed on macOS by the popular `brew` command, by default, will compile with using the default `configure` configuration. It is possible to override this using a `with-openssl` option which will compile against the version of OpenSSL that was most recently installed with `brew` but this is not believed to be in common use. |
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.
Nit: perhaps remote with
in `command, by default, will compile with using the default 'configure'
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.
resolved
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.
Its a good start to providing info/context.
@nodejs/tsc @nodejs/security-wg @nodejs/crypto here's my OpenSSL Policy proposal. I'd like to get this in discussion ASAP as it has a big impact on Node 10 thanks to the expiry of 1.0.2 support 1/2 way through the Node 10 lifecycle. It's not as straightforward as just upgrading, as you'll read. My proposal for 10 is toward the bottom but the context is all above it, so don't skip too much. Relevant to this discussion is that OpenSSL 1.1.1-pre1 has already been released and there is some work underway to test and make Node compatible. TLS 1.3 hasn't been finalized yet and we don't have much of an idea for a release date for 1.1.1. Very unlikely to be prior to Node 10 launch although we may have a clearer idea as we get closer. I'll push on the OpenSSL team as we get closer and just maybe we could shuffle our release timing to get straight to 1.1.1 and skip this 1.1.0 awkwardness. But for now we just have to assume it's not going to be a possibility and plan accordingly. |
Here is the current timeline of forthcoming 1.1.1. https://www.openssl.org/policies/releasestrat.html
And EOSL of 1.1.0 was changed as
TLS 1.3 is one of the main features to be expected to use 1.1.1 and is enabled by default. The date of FIPS support in 1.1.x is not yet to be known. |
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.
I think you sum it up nicely. No FIPS for now and we'll try our best not to break backwards compatibility mid-cycle.
What I'm not sure I agree on is supporting 1.0.2 beyond upstream's EOL date.
OpenSSL-Strategy.md
Outdated
* [c66c3d9fa](https://github.com/nodejs/node/commit/c66c3d9fa3f5bab0bdfe363dd947136cf8a3907f): `deps: fix openssl assembly error on ia32 win32`. A minor tweak to deps/openssl/openssl/crypto/perlasm/x86masm.pl to switch the instruction set referenced in ASM from 486 to 686, only affecting Windows on IA32. | ||
* [42a8de2ac](https://github.com/nodejs/node/commit/42a8de2ac66b6953cbc731fdb0b128b8019643b2): `deps: fix asm build error of openssl in x86_win32`. A fix for deps/openssl/openssl/crypto/perlasm/x86masm.pl for ASM produced for Windows on IA32 as described in https://mta.openssl.org/pipermail/openssl-dev/2015-February/000651.html | ||
* [2eb170874](https://github.com/nodejs/node/commit/2eb170874aa5e84e71b62caab7ac9792fd59c10f): `openssl: fix keypress requirement in apps on win32`. A fix for the `openssl` client application, used by the Node.js test suite, to properly accept stdin without requiring a physical keyboard. As described in <http://openssl.6102.n7.nabble.com/PATCH-s-client-Fix-keypress-requirement-with-redirected-input-on-Windows-td46787.html>. | ||
* [664a65969](https://github.com/nodejs/node/commit/664a6596960655e214fef25e74d3285097703e95): `deps: add -no_rand_screen to openssl s_client`. Adds a `-no_rand_screen` command line option to the `openssl` client applicaiton, used by the Node.js test suite which skips invocation of the `RAND_screen()` call on Windows in order to speed up some of the Node.js TLS tests. Use of this can be found in many of the `test/*/test-{tls,https}-*` test files. |
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.
Typo: s/applicaiton/application/
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.
resolved
OpenSSL-Strategy.md
Outdated
|
||
The OpenSSL FIPS Object Module is compatible with OpenSSL 1.0.2 and Node.js has been able to build with this module since 2015, prior to Node.js 4. It requires some modification of the Node.js internals (see `git grep FIPS -- lib/ src/`) for this to work properly. | ||
|
||
Development and validation of a FIPS software component is time consuming and expensive. The OpenSSL team have yet to commit to a timeframe for development of the next generation of its FIPS Object Module, however they have stated that it is their next priority ["after 1.1.1"](https://www.openssl.org/policies/roadmap.html). Therefore, any user requiring FIPS validated |
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.
Seems like the end is missing.
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.
resolved
f439fe4
to
317e852
Compare
nit fixed @shigeki thanks for the additional info, I hadn't seen that they outlined a 1.1.1 release timeframe and the TLS 1.3 timeframe clarity is helpful. I updated the doc:
@bnoordhuis my concern is with distros that will continue to support 1.0.2 like RedHat beyond its EOL and may still want to bundle Node with 1.0.2, hence the note about continuing support. However, I've changed it now to say:
How does that sound? Ideally we'd continue to support 1.0.2 but we're not going to hold ourselves to that commitment beyond EOL if it's too hard for whatever reason. |
Sounds good! |
👍 on that wording. |
|
||
OpenSSL 1.1.0 represents a fairly major rework of the codebase, at least in comparison to its slow evolution until this point. The external API has some major departures from 1.0.2. However, support for compiling against an external OpenSSL 1.1.0 library (dynamically linked) was [added](https://github.com/nodejs/node/pull/16130) to Node.js' master branch in late 2017. | ||
|
||
Even though OpenSSL 1.1.0 is only supported until August 2018, the API shift is an important stage in Node.js' adaptation. |
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.
The support of 1.1.0 is extended to one year after 1.1.1 release as in https://www.openssl.org/policies/releasestrat.html. So Aug. 2018 is no longer support dead line.
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.
So from that doc it is 8 May 2019 at the latest.
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.
1.1.0 is EOL after 2019-09-11, resolved
As of writing, there are two officially supported versions of OpenSSL as per the [OpenSSL Release Strategy](https://www.openssl.org/policies/releasestrat.html) | ||
|
||
* Version 1.0.2: designated Long-term Support (LTS) which will be supported until 2019-12-31 | ||
* Version 1.1.0: supported until 2018-08-31 |
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.
it is changed as described below.
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.
resolved
As of the time of writing, the strategy for OpenSSL with Node.js 10 is: | ||
|
||
* OpenSSL 1.1.0 to initially be the assumed default compile target. | ||
* Bundle a copy of OpenSSL 1.1.0 in the source tree, along with any floating patches still required for improved Windows support and test-runner speed-ups. |
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.
I think we should add the following a new build requirement especially for Windows.
- A new build requirement of NASM is needed on Windows as in
https://github.com/openssl/openssl/blob/89314d9a22ad0b0ce87e188c486532926aff01ea/NOTES.WIN#L21-L24 .
Microsoft assembler,masm
, is no longer supported in building Node.js with OpenSSL-1.1.x.
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.
A newer gas or llvm version would be required for use of AVX512 features but it has slightly different version requirements between 1.1.0 and 1.1.1.
I think it is one the issues to be resolved before include OpenSSL-1.1.x build binding with Node
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.
resolved by updating BUILDING.md, see
# Node.js OpenSSL Strategy | ||
|
||
The Node.js source tree contains a copy of OpenSSL. By default, Node.js compiles its own copy of OpenSSL and statically links this library into its binary. The `configure` tool allows for dynamic linking against a shared OpenSSL library. Using a combination of the `--shared-openssl`, `--shared-openssl-includes`, `--shared-openssl-libname` and `--shared-openssl-libpath` arguments to `configure`, compilation of Node.js will skip over its bundled copy of the OpenSSL source and link to dynamic library version of OpenSSL, or a suitably compatible OpenSSL equivalent. Whether this linking is possible is left for the compiler to determine at build and linking time. | ||
|
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.
We have a build requirements of assembler version as described in https://github.com/nodejs/node/blob/3a191229418dcc0e21956847993b1702c88a923b/deps/openssl/openssl.gypi#L1038-L1039 and we are using masm (Microsoft assembler) is used building Node.js despite of OpenSSL recommendation in historical reason, which is need to be deprecated in building OpenSSL-1.1.x.
Theses are can be written as below.
The following assembler versions are needed to support in AVX and AVX2 features of OpenSSL-1.0.2 in building Node.js. 'llvm_version>="3.3" or xcode_version>="5.0" or gas_version>="2.23"
In windows, Microsoft assembler, `masm` can be supported in building Node.js with OpenSSL-1.0.2 in despite of OpenSSL recommendation.
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.
Nit: In Windows, we use vcbuild.bat
and the configure_flags
parameter is used to pass the options that you mentioned above for configure
tool. Not sure should we also cover the usage in Windows or not.
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.
resolved like #479 (comment) I think, @shigeki, please comment if I misunderstand.
|
||
OpenSSL 1.1.1 pre-releases are already available as of the time of writing. According to the [OpenSSL release strategy](https://www.openssl.org/policies/releasestrat.html), the 1st of May is the target for "release readiness", with the first possible release date being the 8th of May if the codebase is considered ready for release. | ||
|
||
In addition, the TLS 1.3 specification is in "Last Call" state, due to end on the 2nd of March. This means that it is likely that we will receive a finalized TLS 1.3 specification during March, which would make a May OpenSSL 1.1.1 release feasable. |
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.
Micro-nit: Typo: feasable
-> feasible
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.
resolved
317e852
to
fc077ac
Compare
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
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.
Mostly looks good.
I also wonder if it should be in the nodejs/node repo somewhere, but having it here seems fine too.
@@ -0,0 +1,94 @@ | |||
# Node.js OpenSSL Strategy | |||
|
|||
The Node.js source tree contains a copy of OpenSSL. By default, Node.js compiles its own copy of OpenSSL and statically links this library into its binary. The `configure` tool allows for dynamic linking against a shared OpenSSL library. Using a combination of the `--shared-openssl`, `--shared-openssl-includes`, `--shared-openssl-libname` and `--shared-openssl-libpath` arguments to `configure`, compilation of Node.js will skip over its bundled copy of the OpenSSL source and link to dynamic library version of OpenSSL, or a suitably compatible OpenSSL equivalent. Whether this linking is possible is left for the compiler to determine at build and linking time. |
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.
Nits/suggestions:
-By default, Node.js compiles its own copy of OpenSSL and statically links this library into its binary.
+By default, the Node.js build process compiles its own copy of OpenSSL and statically links this library into the built `node` binary.
-link to dynamic library version of OpenSSL
+link to a dynamic library version of OpenSSL
-at build and link time.
+at build and linking time.
Also it would be good to format to 80 columns, not least so leaving PR comments is easier.
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.
resolved
|
||
The Node.js source tree contains a copy of OpenSSL. By default, Node.js compiles its own copy of OpenSSL and statically links this library into its binary. The `configure` tool allows for dynamic linking against a shared OpenSSL library. Using a combination of the `--shared-openssl`, `--shared-openssl-includes`, `--shared-openssl-libname` and `--shared-openssl-libpath` arguments to `configure`, compilation of Node.js will skip over its bundled copy of the OpenSSL source and link to dynamic library version of OpenSSL, or a suitably compatible OpenSSL equivalent. Whether this linking is possible is left for the compiler to determine at build and linking time. | ||
|
||
## Node.js versions 4 to 9 |
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.
Nit: maybe 4.x to 9.x
for clarity
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.
resolved
|
||
* **nodejs.org**: Binaries produced by the Node.js Build infrastructure across all supported architectures use the default `configure` configuration for OpenSSL and therefore compile and statically link in the OpenSSL source as it ships with Node.js. It should be noted that nodejs.org is the source of binaries for **nvm** which is in heavy client use and is also used by Travis-CI to fetch and install Node.js. | ||
* **Linux Distributions**: Binaries shipped with Debian, Ubuntu, CentOS, RHEL, Fedora and the majority of other Linux distributions follow a standard policy of separating dependencies and dynamically linking wherever possible. Therefore, the Node.js packages on these systems are linked via the packaging dependency mechanisms to OpenSSL 1.0.2 packages and the `node` binaries that ship by default on these platforms use the shared OpenSSL 1.0.2 library installed on that by those packages. Therefore, the floating patches do not apply and it is possible that a different version of OpenSSL 1.0.2 is in use by Node.js than the version that was shipped in the source tree. | ||
* **NodeSource Linux Distributions**: Binaries shipped by this heavily used source do not follow Linux distribution policy and use the default `configure` configuration for OpenSSL and therefore compile and statically link in the OpenSSL source as it ships with Node.js. |
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.
Would be good to add a link, maybe to https://github.com/nodesource/distributions ?
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.
resolved
* [c66c3d9fa](https://github.com/nodejs/node/commit/c66c3d9fa3f5bab0bdfe363dd947136cf8a3907f): `deps: fix openssl assembly error on ia32 win32`. A minor tweak to deps/openssl/openssl/crypto/perlasm/x86masm.pl to switch the instruction set referenced in ASM from 486 to 686, only affecting Windows on IA32. | ||
* [42a8de2ac](https://github.com/nodejs/node/commit/42a8de2ac66b6953cbc731fdb0b128b8019643b2): `deps: fix asm build error of openssl in x86_win32`. A fix for deps/openssl/openssl/crypto/perlasm/x86masm.pl for ASM produced for Windows on IA32 as described in https://mta.openssl.org/pipermail/openssl-dev/2015-February/000651.html | ||
* [2eb170874](https://github.com/nodejs/node/commit/2eb170874aa5e84e71b62caab7ac9792fd59c10f): `openssl: fix keypress requirement in apps on win32`. A fix for the `openssl` client application, used by the Node.js test suite, to properly accept stdin without requiring a physical keyboard. As described in <http://openssl.6102.n7.nabble.com/PATCH-s-client-Fix-keypress-requirement-with-redirected-input-on-Windows-td46787.html>. | ||
* [664a65969](https://github.com/nodejs/node/commit/664a6596960655e214fef25e74d3285097703e95): `deps: add -no_rand_screen to openssl s_client`. Adds a `-no_rand_screen` command line option to the `openssl` client application, used by the Node.js test suite which skips invocation of the `RAND_screen()` call on Windows in order to speed up some of the Node.js TLS tests. Use of this can be found in many of the `test/*/test-{tls,https}-*` test files. |
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.
It would be really useful to have a maintaining-openssl.md
(similar to the existing maintaining-v8.md'](https://github.com/nodejs/node/blob/master/doc/guides/maintaining-V8.md) and [
maintaining-npm.md` guides, that includes a link to this list so we don't forget to update it.
Not really a job for this PR, maybe a request for @shigeki.
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.
There's https://github.com/nodejs/node/wiki/OpenSSL-upgrade-process, although it hasn't been updated since 2016.
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.
Resolved: see https://github.com/nodejs/node/blob/master/deps/openssl/README.md which links to https://github.com/nodejs/node/blob/master/deps/openssl/config/README.md. I have sucessfully followed the documented process multiple times.
|
||
The lack of FIPS support is unfortunate, however, unless a new FIPS module takes an inordinate amount of time, Node.js users requiring FIPS support should be able to use Node.js 8 and switch to a future Node.js version that supports the new FIPS module (ideally Node.js 12). | ||
|
||
This strategy must be communicated to users of Node.js 10 early and often. There is potential for instability and a change in default OpenSSL version is unprecedented and therefore unexpected. The potential for breaking API and/or ABI may also cause disruption, potentially requiring an increment of `NODE_MODULE_VERISION`, which will also be unprecedented within a single release line. It is important that users be aware of this possibility. |
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.
NODE_MODULE_VERISION
-> NODE_MODULE_VERSION
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.
resolved
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 once the 1.1.0 EOL is updated and other comments are addressed. I agree its not optimal to have to do an openssl update, but give the schedules/plans from openssl it seems to be the only reasonable way forward. Ideally we can make the change earlier in the 10.x lifecycle and before it goes LTS.
We could consider waiting for 1.1.1, but the target date does not seem concrete enough to make that a good alternative in my mind.
Thinking about it a bit more, @rvagg what would your take be on waiting 1 month on Node 10.x (target end of May instead of April). It might be worth it to avoid the potential of having to update the NODE_MODULE_VERSION |
Seems indicated to me given the assurance from the OpenSSL team that 1.1.1 will be API and ABI compatible. @rvagg am I right in thinking that if 1.1.1 is not the next LTS release, they might do a 1.1.2 release that is not compatible and declare that LTS? |
We have it in openssl/openssl#5120 (comment). I have the following two concerns about the plan of Node-v10 to start OpenSSL-1.1.0 then move to 1.1.1.
They are different between 1.1.0 and 1.1.1 as below. OpenSSL-1.1.0 minimum assembler version requirements
OpenSSL-1.1.1 minimum assembler version requirements
We are pre-generating assembler files while I am doing with upgrading OpenSSL. If we cannot change build requirements after Node-10 release, it cannot use of AVX512IFMA feature. Otherwise, we needs to pre-generate several kind of assembler files depending on assembler versions. But it leads to be more complex in the future upgrading procedures of OpenSSL.
Shared library users of Node-v10 will continue to use OpenSSL-1.1.0 after we upgrade to 1.1.1 so that we have to maintain two versions of OpenSSL until EOLS of 1.1.0, one year after 1.1.1. For the API simplicity, I think it is better to support one release version of OpenSSL during LTS. |
My PR to upgrade OpenSSL-1.1.0 is coming in the next week after the forth-coming security release of OpenSSL-1.1.0h. Here is my WIP branch at https://github.com/shigeki/node/tree/upgrade_openssl110h_pre |
TLS 1.3 got finalised this week, so 1.1.1 has another box ticked https://www.ietf.org/mail-archive/web/ietf-announce/current/msg17592.html |
Is this ready to land? |
@shigeki you have a blocking objection, are there still updates needed to the doc? |
Most excellent news @nodejs/tsc: https://www.openssl.org/blog/blog/2018/05/18/new-lts/
There's some notes about FIPS too:
As I said in the last TSC meeting, 1.1.1 is pushing further than originally hoped but we're already up to beta 7 as of yesterday and I think things are getting ironed out and ready for release. Beta 8 is due on the 19th of June and they so far haven't planned anything beyond that. There's a release criteria for 1.1.1 at the bottom of https://www.openssl.org/policies/releasestrat.html which basically amounts to not having outstanding major issues tagged for the branch. So cross fingers and we may see a 1.1.1 within the next month. Of course there is a risk of this dragging on closer to October when we hit LTS. After this point an upgrade is going to be really annoying. I think the current indications are that we'll make it long before that but we need to be prepared to think through the implications of that upgrade while in LTS. |
@shigeki still looking to get a first cut of this landed. Are you ok with landing or is there something that needs to be updated first. |
@shigeki ping ? |
|
||
> Although BoringSSL is an open source project, it is not intended for general use, as OpenSSL is. We don't recommend that third parties depend upon it. Doing so is likely to be frustrating because there are no guarantees of API or ABI stability. | ||
|
||
Google have made it clear that BoringSSL is not intended for general use outside of their own internal needs. Node.js will not officially support BoringSSL and unless trivially unobtrusive, the Node.js core team is discouraged from accepting changes that support BoringSSL. The appearance of support for BoringSSL will send the wrong message to users regarding its suitability as an OpenSSL replacement and Node.js' willingness to maintain support. |
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.
I think @codebytere says Electron is using BoringSSL when they embed node. Why shouldn't our stance on any OpenSSL API-alike project be the same as for LibreSSL? No promises on support, but we'll accept PRs that don't burden Node.js in the maintenance of our own use cases.
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.
I actually talked to @codebytere at JSI and suggesred to bring a necessary change into Node.js core, so I am not generally opposed, but if we rephrase this, we should make it very clear that BoringSSL should not be depended upon unless absolutely necessary (which is the case for electron).
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.
Re-reading, I think we are all saying the same thing, unless its a trivial unobtrusive change, we don't want to support BoringSSL. I'm OK with the original wording.
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.
resolved
@rvagg this has been opened for almost a year now. Any chance we can get an update and get this landed? |
@rvagg do you want to get this landed? I'm happy to land it myself if you don't object (after taking another pass through to make sure the comments were addressed). |
This is labelled |
@sam-github would you mind taking this over? It's had plenty of TSC review so far but needs some love to bring it up to date with current status (mostly your work). I'd also be interested in handing over ownership of OpenSSL Evolution in the Strategic Initiatives https://github.com/nodejs/TSC/blob/master/Strategic-Initiatives.md#current-initiatives although I'm not sure it makes sense to put a non-TSC member in charge of it. Alternatively perhaps with this being landed it can be removed as a Strategic initiative. |
I don't think all of our champions need to be a TSC member, although maybe somebody who is the sponsor in the case where a non TSC member is doing most of the work. The sponsor would be somebody that the champion can get help from if needed. In terms of whether it needs to continue as a strategic initiative, once support for TLS 1.3 and OpenSSL 1.1.1 lands in all the target Releases it may be done? |
mm, the TLS 1.3 strategy isn't entirely clear wrt Node 11 and 10 so some finality on that would be good before taking this off the regular-reporting rotation. FIPS, {Boring,Libre}SSL, OpenSSL 2 all prevent this from going away entirely and it'd be good to have documentation and perhaps a person to query. I don't think I'm that person any more though. |
@rvagg I'll update this a bit and push for landing it. It may also be a good time/place to talk about whether to allow building against external OpenSSL that is NOT the same version as the on we use, I mentioned some issues coming up about that feature here: nodejs/node#26319 (comment) |
@sam-github yeah that keeps on coming up and it's a legitimate concern, especially for Linux distribution packagers. IIRC I framed this doc to allow for it but only insofar as the core team are only going to concern themselves with the compatibility of the version bundled but will accept patches from outside to make things work. This is how it's always worked but there's been a blurry line when it involves a lot of changes (like that LibreSSL issue I jumped in to recently and probably shouldn't have!). So, as an example, if @kapouer / Debian wanted to build 12 against OpenSSL 1.1.0 for some reason and we didn't support it, they'd have to submit fixes to get it working again and the core team won't take responsibility for the compatibility issues that ensue (e.g. reports to nodejs/node about TLS 1.3 not working)—although in reality that'd be a case-by-case basis depending on who cared to respond to such a report. If that's not clear in this doc already I think it should be made clear.
Or something like that. It would even allow for compiling master against 1.0.2 if someone cared enough to do that work and keep it updated and the code didn't step all over our toes. |
@rvagg I've got time to integrate comments and update text, stuff like that, so I'm happy to help. But... I thought I could push fixups to your branch, but I can't. Possibly your permissions, but more likely because I don't have push/merge perms on the TSC repo (I guess thats not surprising). So, I'm not sure how I can help. I could just fork TSC, pull your branch from your repo, and PR it again, but I'm not sure duplicating the PR and forking the conversation will be helpful. Any ideas? |
I think this discussion has become stale in its current form so take my commits, edit and make this doc your own and open a new PR to start a fresh discussion. I think that'll be the best way. |
I'll do that over the next day or so, thanks. |
Replaced by #677 |
Here's my proposal for Node.js' OpenSSL Strategy. This document includes the following:
Previous OP:
This is very WIP but I thought I'd better start getting this up rather than let it potentially go stale on my computer if I can't get back to it soon. Missing key pieces, particularly the Node 10 section which I'm going to use to make a recommendation. There's some good context in here thought which might behelpful.I'll ping the TSC and crypto team when it's ready to be read and considered in full.Also, I'm not sure whether this is the best place for this. The other two options I considered were nodejs/Release and nodejs/node/deps/openssl/doc. Happy to hear thoughts on this.