Upgrade to OpenSSL-1.1.0h#19794
Conversation
shigeki
added
semver-major
nodejs-github-bot
added
build
shigeki
force-pushed
the
upgrade_openssl110h
branch
from
cea6e02 to
d4ab4a0
Compare
There was a problem hiding this comment.
It seems the new hash needs to be #Dealing-with-Protocol-Methods
There was a problem hiding this comment.
Fixed the hash link. I resolved some conflicts but it needs a more fix . Fixed in 9cf8473b2c75f043f81cf82785d43b4e03dda967
There was a problem hiding this comment.
asssember -> assembler
There was a problem hiding this comment.
Fixed in 07d5ac52ee66ada4e4a716edb0d4880360b122d6.
There was a problem hiding this comment.
Fixed in cb6aee5a3d8201fb656a8e971b3741d85976a9d0.
There was a problem hiding this comment.
platforms . -> platforms.
There was a problem hiding this comment.
openssl_common.gypi -> `openssl_common.gypi`?
There was a problem hiding this comment.
each arch files -> each arch file?
There was a problem hiding this comment.
I do not see (node_byteoder: little) in the rendered table. Should it be there? Is it parsed as a hidden comment?
There was a problem hiding this comment.
This is a note. I fixed it as a footnote.
|
Are there any more docs to review except Should |
|
@vsemozhetbyt Thanks for fixing my English. Please review |
|
CI of https://ci.nodejs.org/job/node-test-pull-request/14044/ will be fine except |
There was a problem hiding this comment.
It is not in the table still:
Maybe it should be placed in a cell or its own column should be added?
There was a problem hiding this comment.
Sorry, I missed to view and check markdown. Fixed to include it in the cell as f8cdc0f.
There was a problem hiding this comment.
enviroment -> environment
There was a problem hiding this comment.
sources. -> sources for consistency with other headings?
There was a problem hiding this comment.
Get a new source... and extract them
-> Get a new source... and extract all files?
or
-> Get new source files... and extract them?
There was a problem hiding this comment.
Fixed. My choice is the former.
There was a problem hiding this comment.
sources files -> source files?
There was a problem hiding this comment.
4. -> 5.
Commits -> Commit?
There was a problem hiding this comment.
Updates -> Update or This updates?
There was a problem hiding this comment.
Fixed. My choice is the former.
There was a problem hiding this comment.
run test it -> run tests or test it?
There was a problem hiding this comment.
Fixed. My choice is the former.
|
@shigeki so I take it from nodejs/build#1210 that you're not able to maintain backward compatibility with 1.0.2? As per nodejs/TSC#479 the hope was that we would maintain the ability to still compile against 1.0.2 in the same way that Node 8/9 can compile against 1.1.0 now. The problem is going to be with Linux distros that insist on dynamically compiling OpenSSL against what they ship, so if they don't ship OpenSSL 1.1.0 then they won't be able to ship Node 10. I'm not sure which distros, if any, that is actually going to impact, however. |
|
@rvagg It can be possible to be compatible between 1.0.2 and 1.1.0 at this moment. But I am pessimistic to maintain it until the EOLS of 1.0.2 at the end of 2019 unless we keep freezing new features of OpenSSL-1.1.x. Node8/9 could do it since we did not add no new crypto/tls features specific to 1.1.0. I'm fearing that we lose a chance to remove the support of 1.0.2 in Node10 in the future. If it needs a large discussion, I can make back compatibilities with 1.0.2 in this PR and submit a separated PR or issues to remove 1.0.2 support. |
|
If it's not too much work I'd like to see 1.0.2 support maintained @shigeki. I think liberal use of "this feature is not supported by 1.0.2" runtime errors is fine so we can move forward embracing 1.1.x features and just make them not available to builds against 1.0.2, similar to how we've handled FIPS and how others have handled LibreSSL support. @bnoordhuis @indutny could either of you weigh in on this? Perhaps it's just not worth it and we should make a clean break? @kapouer are you available to offer an opinion here since you have a foot in the dynamic linking camp? If Node 10 goes out with no ability to compile against 1.0.2 how many yelps are we going to hear? |
shigeki
left a comment
shigeki
left a comment
There was a problem hiding this comment.
@vsemozhetbyt Thanks for reviewing my English. I fixed in 9a62a0c.
shigeki
force-pushed
the
upgrade_openssl110h
branch
from
9a62a0c to
53a63ef
Compare
|
Btw, if the plan is for this to go in to 10.0.0, it should land no later than April 10th. After that, I'll only pull in tsc approved semver-majors |
|
OK, no response to my 1.0.2 proposition so how about this: let's just land pure 1.1.0 support and ditch 1.0.2 support. It's something that could be added in afterward, during 10.x Current if enough people yelp. I'll go ahead and pull 1.0.2 out of CI for 10+ and I'll update nodejs/TSC#479 to change the plan and get that in front of the TSC. @shigeki anything else we need to get this landed? |
| o['variables']['node_use_openssl'] = b(not options.without_ssl) | ||
| o['variables']['node_shared_openssl'] = b(options.shared_openssl) | ||
| o['variables']['openssl_no_asm'] = 1 if options.openssl_no_asm else 0 | ||
| variables = o['variables']; |
|
a near perfect CI run @ https://ci.nodejs.org/job/node-test-commit/17482/, one failure, a known Raspberry Pi problem unrelated to this 👍 great work @shigeki |
|
I will wait for anyone's reviews until the next Monday night in JST. |
7 participants
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesThis has very big patches due to the source updates from OpenSSL-1.0.2o to 1.1.0h and generated asm files which are not necessary to be reviewed in this PR.
In order to see the differences easily for reviewers, I made two branches in which those changes are removed. Please refer the branch diffs as below in reviewing for it has just several thousands kb diffs.
shigeki/node@raw_upgrade_openssl110h...shigeki:no_archfiles_upgrade_openssl110h
Note that this has a new build requirement of assembler for asm support. Especially nasm is needed to build Windows that is required by OpenSSL. Please refer 9e38498 for details.
CC @nodejs/crypto @nodejs/tsc
The following is the description written in
deps/openssl/README.mdThis has a new binding scheme in builing OpenSSL-1.1.0 library with
Node.js. OpenSSL-1.1.0 uses a new build system with perl for various
supported platforms. See
openssl/Configurations/READMEandopenssl/Configurations/README.designin the OpenSSL source fordetails.
In order to build OpenSSL library without perl in the build of Node.js
for various supported platforms, platform dependent files (e.g. asm
and header files ) are pre-generated and stored into the
config/archsdirectory.config/Makefileandconfig/generate_gypi.plMakefile has supported platform list and generates and copies
platform dependent files (e.g. asm files) into arch directory with
generate.pl. Platform dependent gypi files also created obtaining
build information from
configdata.pmthat is generated withConfigurein the OpenSSL build system.For Windows,
Configuregenerates makefile that is only available tonmake command.
config/Makefile_VC-WIN32andconfig/Makefile_VC-WIN64Aare made created by hand for the use ofGNU make. If make rules or targets are changed in the version up of
OpenSSL, they should be also updated.
gyp and gypi files (
openssl*.{gyp,gypi})openssl.gyphas two targets of openssl and openssl-cli referredfrom
node.gyp. They includes asm and no_asm gypi files with archdependent gypi according to its build options and platforms . The
gyp data which is common with asm and no_asm are stored in
openssl_common.gypi.
header files (
config/*.{h,h.tmpl})bn_conf.h,dso_conf.handopensslconf.hare platform dependentin the OpenSSL sources. They are replaced with
config/*.h.tmplfiles to include the file in the
../../../config/and referred toeach arch files that depends on asm and no-asm option.
Supported architectures for use of ASM
Here is a list of supported architectures for use of ASM in OpenSSL.
These are listed in config/Makefile.
Please refer config/opensslconf_asm.h for details.
Upgrading OpenSSL
Please refer config/README.md .