Make it build using openssl 1.1.0 #8491

kroeckx · 2016-09-11T12:21:11Z

Checklist

make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

crypto

Description of change

This is to make it possible to build against OpenSSL 1.1.0 (#4270)

It currently has 2 new test failures when build against 1.0.2, because OpenSSL generates a different error message in 1.0.2 and 1.1.0. I'm not sure how to deal with that.

In test/parallel/test-crypto.js the message is changed from "asn1 encoding routines:ASN1_CHECK_TLEN:wrong tag" to "asn1 encoding routines:asn1_check_tlen:wrong tag". (The function name in openssl got renamed from all capital letters to small letters.)

In test/parallel/test-tls-junk-server.js, the error message changed from "/unknown protocol/" to "/wrong version number/".

It also fixes 3 current issues with the test suite against the 1.0.2 version.

When building against the 1.1.0 version there are various tests that fail. There are a total of 11 test failures.

Some tests assumes the TLS ticket is 48 bytes. This was always just an implementation details and you could already ask OpenSSL what the size was. The encryption got changed from AES128 to AES256 in 1.1.0 and so the size changed. I have no idea how to get that size to the right places in the test suite.

Some of the other failures are related to ciphers that are disabled by default. For instance anonymous ciphers like AECDH are disabled and you need to enable them by selecting a different security level, which can for instance be done by adding @SECLEVEL=0 to the cipher list, but that's not compatible with the 1.0.x version. Ciphers like 3DES and RC4 are disabled by default.

Other tests fails because the key size that's used is too small.

I'm not sure yet why some of the other tests fail.

There are also a bunch of warnings about deprecated functions. I left them like they are. The proper way to deal with it will probably break compatibility.

PS: During make test I'm getting invalid opcode traps, this seems unrelated to my changes and doesn't seem to cause test suite failures.

indutny · 2016-09-11T12:31:36Z

src/node_crypto.cc

@@ -2435,7 +2545,6 @@ void SSLWrap<Base>::DestroySSL() {
    return;

  SSL_free(ssl_);
-  env_->isolate()->AdjustAmountOfExternalAllocatedMemory(-kExternalSize);


Why should it be removed?

Ah, we have no insight into the size of the structure anymore...

There is no way to tell what the external size of it is with OpenSSL 1.1.0. And it was wrong with 1.0.X in the first place.

indutny · 2016-09-11T12:35:14Z

Thank you, @kroeckx ! We'll review it ASAP.

cc @nodejs/crypto

Trott · 2016-09-11T16:51:35Z

It currently has 2 new test failures when build against 1.0.2, because OpenSSL generates a different error message in 1.0.2 and 1.1.0. I'm not sure how to deal with that.

I believe that when a dependency error message changes, we can just update the test with the new message. For error messages generated by Node.js itself, a change to the message text is currently considered semver-major. But I don't think that's the case if OpenSSL or V8 or libuv changes the message. At least, that's my understanding. /cc @jasnell for confirmation....

kroeckx · 2016-09-11T18:13:18Z

It contains 1 change in error message generated by itself as far as I know, from "Ticket keys length must be 48 bytes" to "Ticket keys length incorrect".

jbergstroem · 2016-09-21T10:54:25Z

Is it reasonable to schedule this for 7.x? I'd like to :)

addaleax · 2016-09-21T10:57:31Z

We can add it to the milestone (I’m doing that) but I guess it depends a lot on how busy @nodejs/crypto is?

jasnell · 2016-09-21T15:50:59Z

So long as the update is not semver-major, it should be possible to do for v7.x. If it is semver-major, then it would require CTC review and approval to land in v7.x

Fishrock123 · 2016-10-05T21:05:10Z

Ping @nodejs/crypto

jasnell · 2016-10-17T17:23:10Z

Ping. Need a status update on this. This would need to move forward this week in order to make it into v7.0.0. We're a week out from the release.

bnoordhuis · 2016-10-18T10:10:44Z

I think we already decided this won't land in v7.0.0. Maybe later in a minor release. I don't have time to review at any rate.

jasnell · 2016-10-18T22:49:03Z

Clearing the v7 milestone from this.

agl · 2016-10-27T01:00:37Z

src/node_crypto.h

        // SSL_CTX_free() will attempt to free the cert_store as well.
        // Since we want our root_cert_store to stay around forever
        // we just clear the field. Hopefully OpenSSL will not modify this
        // struct in future versions.
-        ctx_->cert_store = nullptr;
+        // XXX: Maybe we should just call X509_STORE_up_ref() instead?
+        SSL_CTX_set_cert_store(ctx_, nullptr);


(I happened to be looking at this line today and wondered how this change handled the problem.)

I don't think this works: SSL_CTX_set_cert_store will free the existing X509_STORE before assigning nullptr. But the point of this code was to avoid that because root_cert_store is a global that shouldn't be freed.

Rather I think the answer is to use X509_STORE_up_ref before assigning root_cert_store in the first place. Sadly, in 1.0.1, X509_STORE contained a reference count but it was ignored. In 1.0.2 it's used, but there's no good way to increment it. It's only in 1.1.0 that X509_STORE_up_ref was finally added. (Albeit with an unhelpful signtaure that doesn't return the pointer itself.)

(I'm also quite suspicious of the ownership semantics of root_cert_store anyway.)

I expect I'll send a change this week to try and clear this up and will try to #ifdef things so that it'll work with 1.1.0 too.

Is this fixed by the first commit in #8334?

I guess it handled it by marking it to be fixed (the XXX) and then forgetting all about it.

I think OpenSSL should have called X509_STORE_up_ref() itself in the SSL_CTX_set_cert_store(). It's at least inconsistent that it calls free (which decrements the counter), but doesn't increase the counter.

@sam-github I think the first commit in #8334 is a step in the right direction, but that there's more to be done. (Although sadness about reaching into the structure to fiddle with the refcount; but I don't think there's another way until 1.1.0.)

Nonetheless I support landing that change.

@sam-github I built upon the commit that you referenced and fixed the other issues that I hinted at in #9409.

Instead of using the same session over and over, evict it when the socket emits error. This could be used as a mitigation of #3692, until OpenSSL fix will be merged/released. See: #3692 PR-URL: #4982 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>

This accessor exists in OpenSSL 1.0.2, so it may be used already. This is cherry-picked from PR #8491. PR-URL: #15348 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

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.

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.

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.

This is cherry-picked from PR #8491 and then tidied up. The original had an unnecessarily large diff and messed up some public/private bits. PR-URL: #16130 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Rod Vagg <rod@vagg.org>

This is cherry-picked from PR #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: #16130 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Rod Vagg <rod@vagg.org>

Parts of this were cherry-picked from PR #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: #16130 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Rod Vagg <rod@vagg.org>

This is cherry-picked from PR #8491 and then tidied up. The original had an unnecessarily large diff and messed up some public/private bits. PR-URL: #16130 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Rod Vagg <rod@vagg.org>

This is cherry-picked from PR #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: #16130 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Rod Vagg <rod@vagg.org>

Parts of this were cherry-picked from PR #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: #16130 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Rod Vagg <rod@vagg.org>

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>

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>

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>

This is cherry-picked from PR #8491 and then tidied up. The original had an unnecessarily large diff and messed up some public/private bits. PR-URL: #16130 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Rod Vagg <rod@vagg.org>

This is cherry-picked from PR #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: #16130 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Rod Vagg <rod@vagg.org>

Parts of this were cherry-picked from PR #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: #16130 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Rod Vagg <rod@vagg.org>

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>

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>

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>

This is cherry-picked from PR #8491 and then tidied up. The original had an unnecessarily large diff and messed up some public/private bits. PR-URL: #16130 Backport-PR-URL: #18622 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Rod Vagg <rod@vagg.org>

This is cherry-picked from PR #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: #16130 Backport-PR-URL: #18622 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Rod Vagg <rod@vagg.org>

Parts of this were cherry-picked from PR #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: #16130 Backport-PR-URL: #18622 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Rod Vagg <rod@vagg.org>

Make it build using openssl 1.1.0

62cc7b8

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. tls Issues and PRs related to the tls subsystem. labels Sep 11, 2016

kroeckx mentioned this pull request Sep 11, 2016

Discussion: OpenSSL 1.1.0 planning #4270

Closed

indutny reviewed Sep 11, 2016
View reviewed changes

Trott force-pushed the master branch from b0df363 to c5ce7f4 Compare September 21, 2016 00:09

addaleax added this to the 7.0.0 milestone Sep 21, 2016

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02

jasnell removed this from the 7.0.0 milestone Oct 18, 2016

agl reviewed Oct 27, 2016

View reviewed changes

kroeckx mentioned this pull request Oct 27, 2016

Make SSL_CTX_set_cert_store increase the reference counter openssl/openssl#1795

Closed

kapouer mentioned this pull request Dec 11, 2016

Missing make rule for test/fixtures/keys/0-dns-* #10228

Closed

MylesBorins pushed a commit that referenced this pull request Oct 25, 2017

crypto: use SSL_SESSION_get_id

6b08128

This accessor exists in OpenSSL 1.0.2, so it may be used already. This is cherry-picked from PR #8491. PR-URL: #15348 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

davidben added a commit to davidben/node that referenced this pull request Nov 2, 2017

crypto: make node_crypto_bio 1.1.0-compatible.

e9e70ba

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.

Reinis mentioned this pull request Jan 5, 2019

the electron does not build elprans/electron-overlay#35

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make it build using openssl 1.1.0 #8491

Make it build using openssl 1.1.0 #8491

kroeckx commented Sep 11, 2016 •

edited

Loading

indutny Sep 11, 2016

indutny Sep 11, 2016

kroeckx Sep 11, 2016

indutny commented Sep 11, 2016

Trott commented Sep 11, 2016

kroeckx commented Sep 11, 2016

jbergstroem commented Sep 21, 2016

addaleax commented Sep 21, 2016

jasnell commented Sep 21, 2016

Fishrock123 commented Oct 5, 2016

jasnell commented Oct 17, 2016

bnoordhuis commented Oct 18, 2016

jasnell commented Oct 18, 2016

agl Oct 27, 2016

sam-github Oct 27, 2016

kroeckx Oct 27, 2016

agl Oct 28, 2016

agl Nov 1, 2016

Make it build using openssl 1.1.0 #8491

Make it build using openssl 1.1.0 #8491

Conversation

kroeckx commented Sep 11, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

indutny commented Sep 11, 2016

Trott commented Sep 11, 2016

kroeckx commented Sep 11, 2016

jbergstroem commented Sep 21, 2016

addaleax commented Sep 21, 2016

jasnell commented Sep 21, 2016

Fishrock123 commented Oct 5, 2016

jasnell commented Oct 17, 2016

bnoordhuis commented Oct 18, 2016

jasnell commented Oct 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kroeckx commented Sep 11, 2016 •

edited

Loading