Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[v8.x backport] Support both OpenSSL 1.1.0 and 1.0.2 #18326

Closed
wants to merge 25 commits into from
Closed

[v8.x backport] Support both OpenSSL 1.1.0 and 1.0.2 #18326

wants to merge 25 commits into from

Conversation

MylesBorins
Copy link
Contributor

This is a backport of #16130 which includes a patch from the fedora backport.

Refs: https://src.fedoraproject.org/fork/zvetlik/rpms/nodejs/tree/openssl-110

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. v8.x labels Jan 23, 2018
@MylesBorins MylesBorins changed the title Openssl 1.1.0 8.x [v8.x backport] Support both OpenSSL 1.1.0 and 1.0.2 Jan 23, 2018
@MylesBorins MylesBorins mentioned this pull request Jan 23, 2018
Closed
3 tasks
@MylesBorins
Copy link
Contributor Author

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

@gibfahn gibfahn requested review from bnoordhuis and rvagg January 23, 2018 23:06
@gibfahn
Copy link
Member

gibfahn commented Jan 23, 2018

This being a fairly large change, I'd appreciate a look from @bnoordhuis @rvagg @davidben @kasicka @kapouer

@kapouer
Copy link
Contributor

kapouer commented Jan 23, 2018

@gibfahn i am currently replacing my backport with the one in this PR (openssl-1.1.0-8.x branch). Will forward any issue.

@MylesBorins
Copy link
Contributor Author

MylesBorins commented Jan 23, 2018 •
edited
Loading

thanks @kapouer

@gibfahn are the windows failures the same that we are getting on staging rn or are they new?

edit:

CI against v8.x-staging for comparison https://ci.nodejs.org/job/node-test-commit/15629/

@MylesBorins
Copy link
Contributor Author

Should 464e10c be dropped as it is a doc deprecation?

@kapouer
Copy link
Contributor

kapouer commented Jan 24, 2018

LGTM, builds and test-ci-js pass.

@gibfahn gibfahn force-pushed the v8.x-staging branch 2 times, most recently from 68a631f to aff27a1 Compare January 24, 2018 02:46
@rvagg
Copy link
Member

rvagg commented Jan 31, 2018

A pass on node-test-linux-linked-openssl110 is a good sign at least. Looks like a rebase or something needs to happen here, 79 commits.

There's a failure of parallel/test-tls-server-verify on one of the Windows machines, it's a timeout and I'm not sure how much of a concern it should be.

Re dropping 464e10c, my guess is that's not appropriate to backport to 8 and we should leave that deprecation for 9+.

What's the context on the Fedora patch? Is that something we should be pushing through master first? I'm not seeing it there. Can someone provide a link to a discussion where this came up so we can review whether it's appropriate or not?

@gibfahn
Copy link
Member

gibfahn commented Jan 31, 2018

@rvagg pretty sure it was in #16130 (comment)

@MylesBorins
Copy link
Contributor Author

Rebased against v8.x-staging

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

@MylesBorins
Copy link
Contributor Author

@gibfahn are there still lots of failures on v8.x-staging? Are the failures in the above CI new?

davidben added 10 commits February 7, 2018 01:53
@davidben @MylesBorins
crypto: use X509_STORE_CTX_new
60b8b73 
In OpenSSL 1.1.0, X509_STORE_CTX is opaque and thus cannot be
stack-allocated. This works in OpenSSL 1.1.0 and 1.0.2. Adapted from PR

PR-URL: nodejs#16130
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>
@davidben @MylesBorins
crypto: make node_crypto_bio compat w/ OpenSSL 1.1
7702cc3 
This is cherry-picked from PR nodejs#8491 and then tidied up. The original had
an unnecessarily large diff and messed up some public/private bits.

PR-URL: nodejs#16130
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>
@davidben @MylesBorins
crypto: estimate kExternalSize
f51f3e9 
Based on a build of OpenSSL 1.1.0f.

The exact sizes are not particularly important (the original value was
missing all the objects hanging off anyway), only that V8 garbage
collector be aware that there is some memory usage beyond the sockets
themselves.

PR-URL: nodejs#16130
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>
@davidben @MylesBorins
crypto: remove unnecessary SSLerr calls
7530865 
These are OpenSSL-internal APIs that are no longer accessible in 1.1.0
and weren't necessary. OpenSSL will push its own errors and, if it
doesn't, the calling code would handle it anyway.

PR-URL: nodejs#16130
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>
@davidben @MylesBorins
crypto: account for new 1.1.0 SSL APIs
5dcf534 
This is cherry-picked from PR nodejs#8491 and tidied up. This change does
*not* account for the larger ticket key in OpenSSL 1.1.0. That will be
done in a follow-up commit as the 48-byte ticket key is part of Node's
public API.

rvagg: removed BORINGSSL defines before landing

PR-URL: nodejs#16130
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>
@davidben @MylesBorins
crypto: test DH keys work without a public half
c6decc5 
Add a regression test for openssl/openssl#4384.

PR-URL: nodejs#16130
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>
@davidben @MylesBorins
crypto: use RSA and DH accessors
da9702a 
Parts of this were cherry-picked from PR nodejs#8491. Note that this only
works with OpenSSL 1.0.2 or 1.1.0g or later. 1.1.0g is, as of writing,
not yet released, but the fix is on the branch. See
openssl/openssl#4384.

PR-URL: nodejs#16130
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>
@davidben @MylesBorins
crypto: remove locking callbacks for OpenSSL 1.1.0
3b967ab 
The callbacks are all no-ops in OpenSSL 1.1.0. This isn't necessary (the
functions still exist for compatibility), but silences some warnings and
avoids allocating a few unused mutexes.

PR-URL: nodejs#16130
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>
@davidben @MylesBorins
crypto: make CipherBase 1.1.0-compatible
68ea095 
In OpenSSL 1.1.0, EVP_CIPHER_CTX must be heap-allocated. Once we're
heap-allocating them, there's no need in a separate initialised_ bit.
The presence of ctx_ is sufficient.

PR-URL: nodejs#16130
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>
@davidben @MylesBorins
crypto: make Hash 1.1.0-compatible
0790a4c 
OpenSSL 1.1.0 requires EVP_MD_CTX be heap-allocated.

PR-URL: nodejs#16130
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>
davidben and others added 15 commits February 7, 2018 01:53
@davidben @MylesBorins
crypto: make SignBase compatible with OpenSSL 1.1.0
612c103 
1.1.0 requires EVP_MD_CTX be heap-allocated. In doing so, move the Init
and Update hooks to shared code because they are the same between Verify
and Sign.

PR-URL: nodejs#16130
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>
@davidben @MylesBorins
crypto: Make Hmac 1.1.0-compatible
4485ba0 
OpenSSL 1.1.0 requries HMAC_CTX be heap-allocated.

PR-URL: nodejs#16130
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>
@davidben @MylesBorins
crypto: add compat logic for "DSS1" and "dss1"
bdb4d70 
In OpenSSL 1.1.0, EVP_dss1() is removed. These hash names were exposed
in Node's public API, so add compatibility hooks for them.

PR-URL: nodejs#16130
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>
@davidben @MylesBorins
crypto: hard-code tlsSocket.getCipher().version
7400a06 
This aligns the documentation with reality. This API never did what Node
claims it did.

The SSL_CIPHER_get_version function just isn't useful. In OpenSSL 1.0.2,
it always returned the string "TLSv1/SSLv3" for anything but SSLv2
ciphers, which Node does not support. Note how test-tls-multi-pfx.js
claims that ECDHE-ECDSA-AES256-GCM-SHA384 was added in TLSv1/SSLv3 which
is not true. That cipher is new as of TLS 1.2. The OpenSSL 1.0.2
implementation is:

char *SSL_CIPHER_get_version(const SSL_CIPHER *c)
{
    int i;

    if (c == NULL)
        return ("(NONE)");
    i = (int)(c->id >> 24L);
    if (i == 3)
        return ("TLSv1/SSLv3");
    else if (i == 2)
        return ("SSLv2");
    else
        return ("unknown");
}

In OpenSSL 1.1.0, SSL_CIPHER_get_version changed to actually behave as
Node documented it, but this changes the semantics of the function and
breaks tests. The cipher's minimum protocol version is not a useful
notion to return to the caller here, so just hardcode the string at
"TLSv1/SSLv3" and document it as legacy.

PR-URL: nodejs#16130
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>
@davidben @MylesBorins
test: update test expectations for OpenSSL 1.1.0
ddf4987 
Some errors in the two versions are different. The test-tls-no-sslv3 one
because OpenSSL 1.1.x finally does version negotiation properly. 1.0.x's
logic was somewhat weird and resulted in very inconsistent errors for
SSLv3 in particular.

Also the function codes are capitalized differently, but function codes
leak implementation details, so don't assert on them to begin with.

PR-URL: nodejs#16130
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>
@davidben @MylesBorins
test: remove sha from test expectations
4b4a17b 
"sha" in OpenSSL refers to SHA-0 which was removed from OpenSSL 1.1.0
and is insecure. Replace it with SHA-256 which is present in both 1.0.2
and 1.1.0. Short of shipping a reimplementation in Node, this is an
unavoidable behavior change with 1.1.0.

PR-URL: nodejs#16130
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>
@davidben @MylesBorins
crypto: emulate OpenSSL 1.0 ticket scheme in 1.1
ab02259 
OpenSSL 1.0.x used a 48-byte ticket key, but OpenSSL 1.1.x made it
larger by using a larger HMAC-SHA256 key and using AES-256-CBC to
encrypt. However, Node's public API exposes the 48-byte key. Implement
the ticket key callback to restore the OpenSSL 1.0.x behavior.

PR-URL: nodejs#16130
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>
@davidben @MylesBorins
test: test with a larger RSA key
31cf594 
OpenSSL 1.1.0 rejects RSA keys smaller than 1024 bits by default. Fix
the tests to use larger ones. This test only cares that the PEM blob be
missing a trailing newline. Certificate adapted from
test/fixtures/cert.pem.

PR-URL: nodejs#16130
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>
@davidben @MylesBorins
test: revise test-tls-econnreset for OpenSSL 1.1.0
28a9fac 
This test is testing what happens to the server if the client shuts off
the connection (so the server sees ECONNRESET), but the way it does it
is convoluted. It uses a static RSA key exchange with a tiny (384-bit)
RSA key. The server doesn't notice (since it is static RSA, the client
acts on the key first), so the client tries to encrypt a premaster and
fails:

  rsa routines:RSA_padding_add_PKCS1_type_2:data too large for key size
  SSL routines:ssl3_send_client_key_exchange:bad rsa encrypt

OpenSSL happens not to send an alert in this case, so we get ECONNRESET
with no alert. This is quite fragile and, notably, breaks in OpenSSL
1.1.0 now that small RSA keys are rejected by libssl. Instead, test by
just connecting a TCP socket and immediately closing it.

PR-URL: nodejs#16130
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>
@davidben @MylesBorins
crypto: remove deprecated ECDH calls w/ OpenSSL 1.1
11120dd 
These are both no-ops in OpenSSL 1.1.0.

PR-URL: nodejs#16130
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>
@davidben @MylesBorins
test: configure certs in tests
7eb8740 
OpenSSL 1.1.0 disables anonymous ciphers unless building with
enable-weak-crypto. Avoid unnecessary dependencies on these ciphers in
tests.

PR-URL: nodejs#16130
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>
@davidben @MylesBorins
test: fix test-https-agent-session-eviction for 1.1
736dd79 
This test is testing the workaround for an OpenSSL 1.0.x bug, which was
fixed in 1.1.0. With the bug fixed, the test expectations need to change
slightly.

PR-URL: nodejs#16130
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>
@davidben @MylesBorins
crypto: make ALPN the same for OpenSSL 1.0.2 & 1.1.0
48a355d 
This is kind of hairy. OpenSSL 1.0.2 ignored the return value and always
treated everything as SSL_TLSEXT_ERR_NOACK (so the comment was wrong and
Node was never sending a warning alert). OpenSSL 1.1.0 honors
SSL_TLSEXT_ERR_NOACK vs SSL_TLSEXT_ERR_FATAL_ALERT and treats everything
unknown as SSL_TLSEXT_ERR_FATAL_ALERT.

Since this is a behavior change (tests break too), start by aligning
everything on SSL_TLSEXT_ERR_NOACK. If sending no_application_protocol
is desirable in the future, this can by changed to
SSL_TLSEXT_ERR_FATAL_ALERT with whatever deprecation process is
appropriate.

However, note that, contrary to
https://rt.openssl.org/Ticket/Display.html?id=3463#txn-54498,
SSL_TLSEXT_ERR_FATAL_ALERT is *not* useful to a server with no fallback
protocol. Even if such mismatches were rejected, such a server must
*still* account for the fallback protocol case when the client does not
advertise ALPN at all. Thus this may not be worth bothering.

PR-URL: nodejs#16130
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>
@davidben @MylesBorins
crypto: clear some SSL_METHOD deprecation warnings
37142c6 
Fixing the rest will be rather involved. I think the cleanest option is
to deprecate the method string APIs which are weird to begin with.

PR-URL: nodejs#16130
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>
@MylesBorins
crypto: remove leftover initialization
dfaf1ec 
Refs: https://src.fedoraproject.org/fork/zvetlik/rpms/nodejs/blob/6141424e6629e53e3e628a16aec98018720ae5f3/f/0027-Fix-leftover-initialised.patch
@MylesBorins
Copy link
Contributor Author

ci: https://ci.nodejs.org/job/node-test-pull-request/12997/ one more time

@MylesBorins
Copy link
Contributor Author

CI is green, single failure is known flake current flagged on 8.x-staging. I'm going to land.

@nodejs/crypto please LMK if you have concerns

@MylesBorins
Copy link
Contributor Author

MylesBorins commented Feb 7, 2018 •
edited
Loading

landed in 3a648b7...e4e0cf6

@MylesBorins MylesBorins closed this Feb 7, 2018
@MylesBorins
Copy link
Contributor Author

MylesBorins commented Feb 7, 2018 •
edited
Loading

nvm, dfaf1ec is a new commit and needs explicit signoff

@nodejs/crypto can someone please sign off on this PR

one more CI so we can have something green! https://ci.nodejs.org/job/node-test-pull-request/13012/

edit: not letting me reopen, which is annoying. Please put and LGTM in the comments

@MylesBorins MylesBorins mentioned this pull request Feb 7, 2018
Closed
@MylesBorins
Copy link
Contributor Author

new PR opened in #18622

@kasicka kasicka mentioned this pull request Feb 7, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnoordhuis bnoordhuis Awaiting requested review from bnoordhuis

@rvagg rvagg Awaiting requested review from rvagg

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@MylesBorins @gibfahn @kapouer @rvagg @nodejs-github-bot @davidben