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

Update to openssl1.1.1a #25381

Closed
wants to merge 11 commits into
base: master
from

Conversation

Projects
7 participants
@sam-github
Copy link
Member

sam-github commented Jan 7, 2019

Most of this work comes from @shigeki, who got openssl 1.1.1 building and running across all Node's platforms.

Last time I tested this branch on ci it passed, and it passes locally.

I've done a fair amount of ABI testing, as well. It looks pretty compatible to me (as OpenSSL intended).

See #18770 (comment) (and around) for more information.

@nodejs/crypto , particularly @rvagg @shigeki

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
@shigeki

This comment has been minimized.

Copy link
Contributor

shigeki commented Jan 8, 2019

@sam-github Thanks for the PR. I could not imagine we can fix an async cipher error and that's a great work. For I'm in a business trip during this week, I'm going to take a detail look at this in later.

@danbev

danbev approved these changes Jan 8, 2019

@sam-github

This comment has been minimized.

Copy link
Member Author

sam-github commented Jan 8, 2019

@shigeki Enjoy your trip, I'll make sure you have time to review.

ci: https://ci.nodejs.org/job/node-test-commit-linux-containered/9865/

Build and test against openssl 1.1.0 was the failure above. I'll look into it.

@sam-github sam-github force-pushed the sam-github:update_openssl1.1.1a branch from 8f1b9bd to e70a5e1 Jan 10, 2019

@sam-github

This comment has been minimized.

Copy link
Member Author

sam-github commented Jan 15, 2019

The last build passed on everything except ARM.

re-ci: https://ci.nodejs.org/job/node-test-commit/25052/

danbev added a commit to danbev/node that referenced this pull request Jan 16, 2019

test: change ciphers from 'RC4' to 'missing'
This commit updates option ciphers from 'RC4' to 'missing' in
test/parallel/test-tls-handshake-error.js.

The motivation for this change is that this test is verifying that a
'no ciphers match' error be thrown, but 'RC4' might be among the ciphers
supported by the OpenSSL version when dynamically linking. I ran into
this specific issue when dynamically linking against OpenSSL 1.1.1 on
RHEL8 using nodejs#25381.

@danbev danbev referenced this pull request Jan 16, 2019

Closed

test: change ciphers from 'RC4' to 'no-such-cipher' #25534

2 of 2 tasks complete
@sam-github

This comment has been minimized.

Copy link
Member Author

sam-github commented Jan 17, 2019

So, been more than a week, green in CI, one approval, I guess I could just land this... but it is a significant enough update I'd like a few more reviews! @nodejs/crypto @shigeki @rvagg @bnoordhuis @indutny

1.1.1 adds some crypto algs and minor features (like zero-length PKCS8 passphrases, @tniessen ), so I'm labelling semver-minor.

@sam-github sam-github force-pushed the sam-github:update_openssl1.1.1a branch from d4bfbfb to fda3b55 Jan 17, 2019

@sam-github

This comment has been minimized.

Copy link
Member Author

sam-github commented Jan 17, 2019

Updated against node/master, squashed fixups, started a fresh CI.

ci: https://ci.nodejs.org/job/node-test-pull-request/20184/

danbev added a commit that referenced this pull request Jan 21, 2019

test: change ciphers from RC4 to no-such-cipher
This commit updates option ciphers from 'RC4' to 'no-such-cipher' in
test/parallel/test-tls-handshake-error.js.

The motivation for this change is that this test is verifying that a
'no ciphers match' error be thrown, but 'RC4' might be among the ciphers
supported by the OpenSSL version when dynamically linking. I ran into
this specific issue when dynamically linking against OpenSSL 1.1.1 on
RHEL8 using #25381.

PR-URL: #25534
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@shigeki
Copy link
Contributor

shigeki left a comment

@sam-github Sorry for my late response. I checked your and my change commits and they are fine but some of my commits are missing commit descriptions.

I made a branch to fill the commit descriptions in https://github.com/shigeki/node/commits/PR25381.
Please rebase this to add the commit descriptions from 8e05aa0, fd5d8cb, 0dd90f3 and 01fcc96.
fd5d8cb is also fixed so as to fix within 50 chars for commit title.

Thanks for making this PR.

@sam-github

This comment has been minimized.

Copy link
Member Author

sam-github commented Jan 21, 2019

@shigeki Thanks for reviewing, I'll go through and rebase and fixup the messages.

@sam-github sam-github force-pushed the sam-github:update_openssl1.1.1a branch from fda3b55 to 572326a Jan 21, 2019

@sam-github

This comment has been minimized.

Copy link
Member Author

sam-github commented Jan 21, 2019

Added commit bodies from @shigeki, rebased, re-ci.

ci: https://ci.nodejs.org/job/node-test-pull-request/20248/

@sam-github

This comment has been minimized.

Copy link
Member Author

sam-github commented Jan 22, 2019

git node land claims commits were pushed after last CI, which I don't think is true, but re-running just in case.

ci: https://ci.nodejs.org/job/node-test-pull-request/20261/

shigeki and others added some commits Mar 7, 2018

deps: add s390 asm rules for OpenSSL-1.1.1
This is a floating patch against OpenSSL-1.1.1 to generate asm files
with Makefile rules.
deps: fix gyp/gypi for openssl-1.1.1
Some of defines and cppflags in the build config of OpenSSL-1.1.1 were
moved to new attributes. Gyp and gypi file generations are needed to be
fixed to include them.
deps: fix MacOS and Win build for OpenSSL-1.1.1
Because llvm on MacOS does not support AVX-512, asm files need to be limited to
AVX-2 support even when they are generated on Linux.  fake_gcc.pl returns the
fake llvm banner version for MacOS as if the assembler supports upto AVX-2.

For Windows, makefiles for nmake were updated in OpenSSL-1.1.1 and they are
rewritten into GNU makefile format by hand.
deps: add only avx2 configs for OpenSSL-1.1.1
OpenSSL-1.1.1 has new support of AVX-512 but AVX-2 asm files still need
to be generated for the older assembler support to keep backward
compatibilities.
deps: upgrade openssl sources to 1.1.1a
This updates all sources in deps/openssl/openssl with openssl-1.1.1a.

sam-github added a commit to sam-github/node that referenced this pull request Jan 24, 2019

deps: fix gyp/gypi for openssl-1.1.1
Some of defines and cppflags in the build config of OpenSSL-1.1.1 were
moved to new attributes. Gyp and gypi file generations are needed to be
fixed to include them.

PR-URL: nodejs#25381
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>

sam-github added a commit to sam-github/node that referenced this pull request Jan 24, 2019

deps: fix MacOS and Win build for OpenSSL-1.1.1
Because llvm on MacOS does not support AVX-512, asm files need to be limited to
AVX-2 support even when they are generated on Linux.  fake_gcc.pl returns the
fake llvm banner version for MacOS as if the assembler supports upto AVX-2.

For Windows, makefiles for nmake were updated in OpenSSL-1.1.1 and they are
rewritten into GNU makefile format by hand.

PR-URL: nodejs#25381
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>

sam-github added a commit to sam-github/node that referenced this pull request Jan 24, 2019

deps: add s390 asm rules for OpenSSL-1.1.1
This is a floating patch against OpenSSL-1.1.1 to generate asm files
with Makefile rules.

PR-URL: nodejs#25381
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>

sam-github added a commit to sam-github/node that referenced this pull request Jan 24, 2019

deps: add only avx2 configs for OpenSSL-1.1.1
OpenSSL-1.1.1 has new support of AVX-512 but AVX-2 asm files still need
to be generated for the older assembler support to keep backward
compatibilities.

PR-URL: nodejs#25381
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>

sam-github added a commit to sam-github/node that referenced this pull request Jan 24, 2019

deps: fix for non GNU assembler in AIX
AIX has own assembler not GNU as that does not support --noexecstack.

PR-URL: nodejs#25381
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>

sam-github added a commit to sam-github/node that referenced this pull request Jan 24, 2019

doc: fix assembler requirement for OpenSSL-1.1.1
Add new requirements of assembler version for AVX-512 support
in OpenSSL-1.1.1.

PR-URL: nodejs#25381
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>

sam-github added a commit to sam-github/node that referenced this pull request Jan 24, 2019

deps: update archs files for OpenSSL-1.1.1a
`cd deps/openssl/config; make` updates all archs dependant files.

PR-URL: nodejs#25381
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>

sam-github added a commit to sam-github/node that referenced this pull request Jan 24, 2019

tls: make ossl 1.1.1 cipher list throw error
Make OpenSSL 1.1.1 error during cipher list setting if it would have
errored with OpenSSL 1.1.0.

Can be dropped after our OpenSSL fixes this upstream.

See: openssl/openssl#7759

PR-URL: nodejs#25381
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>

sam-github added a commit to sam-github/node that referenced this pull request Jan 24, 2019

tls: workaround handshakedone in renegotiation
`SSL_CB_HANDSHAKE_START` and `SSL_CB_HANDSHAKE_DONE` are called
sending HelloRequest in OpenSSL-1.1.1.
We need to check whether this is in a renegotiation state or not.

PR-URL: nodejs#25381
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>

sam-github added a commit to sam-github/node that referenced this pull request Jan 24, 2019

test: assert on client and server side seperately
This gets better coverage of the codes, and is more explicit. It also
works around ordering differences in the errors produced by openssl.
The approach was tested with 1.1.0 and 1.1.1, as well as TLSv1.2 vs
TLSv1.3. OpenSSL 1.1.0 is relevant when node is built against a shared
openssl.

PR-URL: nodejs#25381
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>

@sam-github sam-github referenced this pull request Jan 24, 2019

Closed

[v11.x backport] Update openssl to 1.1.1a #25688

0 of 4 tasks complete

sam-github added a commit to sam-github/openssl that referenced this pull request Jan 25, 2019

Remove unnecessary trailing whitespace
Trim trailing whitespace. It doesn't match OpenSSL coding standards,
AFAICT, and it can cause problems with git tooling.

Trailing whitespace remains in PEM test data that is intended to test
this situation, as well as in external/perl/Text-Template-1.46, which is
external source.

See: nodejs/node#25381 (comment)

@sam-github sam-github referenced this pull request Jan 25, 2019

Closed

Remove unnecessary trailing whitespace #8092

2 of 2 tasks complete

Trott added a commit to Trott/io.js that referenced this pull request Jan 26, 2019

doc: remove outdated s_client information in tls.md
There is a description of how to use s_client for testing of
renegotiation limits in the `tls` module documentation. The information
is somewhat out of scope, but it also may be somewhat problematic due to
changes/peculiarities (bugs?) in recent s_client. Remove the text.

Refs: nodejs#25381 (comment)

PR-URL: nodejs#25678
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

addaleax added a commit that referenced this pull request Jan 28, 2019

doc: remove outdated s_client information in tls.md
There is a description of how to use s_client for testing of
renegotiation limits in the `tls` module documentation. The information
is somewhat out of scope, but it also may be somewhat problematic due to
changes/peculiarities (bugs?) in recent s_client. Remove the text.

Refs: #25381 (comment)

PR-URL: #25678
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

targos added a commit that referenced this pull request Jan 28, 2019

deps: upgrade openssl sources to 1.1.1a
This updates all sources in deps/openssl/openssl with openssl-1.1.1a.

PR-URL: #25381
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Backport-PR-URL: #25688

targos added a commit that referenced this pull request Jan 28, 2019

deps: fix gyp/gypi for openssl-1.1.1
Some of defines and cppflags in the build config of OpenSSL-1.1.1 were
moved to new attributes. Gyp and gypi file generations are needed to be
fixed to include them.

PR-URL: #25381
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Backport-PR-URL: #25688

targos added a commit that referenced this pull request Jan 28, 2019

deps: fix MacOS and Win build for OpenSSL-1.1.1
Because llvm on MacOS does not support AVX-512, asm files need to be limited to
AVX-2 support even when they are generated on Linux.  fake_gcc.pl returns the
fake llvm banner version for MacOS as if the assembler supports upto AVX-2.

For Windows, makefiles for nmake were updated in OpenSSL-1.1.1 and they are
rewritten into GNU makefile format by hand.

PR-URL: #25381
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Backport-PR-URL: #25688

targos added a commit that referenced this pull request Jan 28, 2019

deps: add s390 asm rules for OpenSSL-1.1.1
This is a floating patch against OpenSSL-1.1.1 to generate asm files
with Makefile rules.

PR-URL: #25381
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Backport-PR-URL: #25688

targos added a commit that referenced this pull request Jan 28, 2019

deps: add only avx2 configs for OpenSSL-1.1.1
OpenSSL-1.1.1 has new support of AVX-512 but AVX-2 asm files still need
to be generated for the older assembler support to keep backward
compatibilities.

PR-URL: #25381
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Backport-PR-URL: #25688

targos added a commit that referenced this pull request Jan 28, 2019

deps: fix for non GNU assembler in AIX
AIX has own assembler not GNU as that does not support --noexecstack.

PR-URL: #25381
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Backport-PR-URL: #25688

targos added a commit that referenced this pull request Jan 28, 2019

doc: fix assembler requirement for OpenSSL-1.1.1
Add new requirements of assembler version for AVX-512 support
in OpenSSL-1.1.1.

PR-URL: #25381
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Backport-PR-URL: #25688

targos added a commit that referenced this pull request Jan 28, 2019

deps: update archs files for OpenSSL-1.1.1a
`cd deps/openssl/config; make` updates all archs dependant files.

PR-URL: #25381
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Backport-PR-URL: #25688

targos added a commit that referenced this pull request Jan 28, 2019

tls: make ossl 1.1.1 cipher list throw error
Make OpenSSL 1.1.1 error during cipher list setting if it would have
errored with OpenSSL 1.1.0.

Can be dropped after our OpenSSL fixes this upstream.

See: openssl/openssl#7759

PR-URL: #25381
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Backport-PR-URL: #25688

targos added a commit that referenced this pull request Jan 28, 2019

tls: workaround handshakedone in renegotiation
`SSL_CB_HANDSHAKE_START` and `SSL_CB_HANDSHAKE_DONE` are called
sending HelloRequest in OpenSSL-1.1.1.
We need to check whether this is in a renegotiation state or not.

PR-URL: #25381
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Backport-PR-URL: #25688

targos added a commit that referenced this pull request Jan 28, 2019

test: assert on client and server side seperately
This gets better coverage of the codes, and is more explicit. It also
works around ordering differences in the errors produced by openssl.
The approach was tested with 1.1.0 and 1.1.1, as well as TLSv1.2 vs
TLSv1.3. OpenSSL 1.1.0 is relevant when node is built against a shared
openssl.

PR-URL: #25381
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Backport-PR-URL: #25688

@targos targos referenced this pull request Jan 29, 2019

Merged

v11.9.0 proposal #25802

@targos targos added this to Backported in v11.x Jan 30, 2019

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