Conversation
|
@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. |
|
@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. |
8f1b9bd to
e70a5e1
Compare
|
The last build passed on everything except ARM. |
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.
|
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 |
d4bfbfb to
fda3b55
Compare
|
Updated against node/master, squashed fixups, started a fresh CI. |
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
left a comment
There was a problem hiding this 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.
|
@shigeki Thanks for reviewing, I'll go through and rebase and fixup the messages. |
fda3b55 to
572326a
Compare
|
Added commit bodies from @shigeki, rebased, re-ci. |
|
|
This updates all sources in deps/openssl/openssl with openssl-1.1.1a.
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.
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.
This is a floating patch against OpenSSL-1.1.1 to generate asm files with Makefile rules.
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.
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
`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
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
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>
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), orvcbuild test(Windows) passes