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.

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

Make it build using openssl 1.1.0 #8491

Closed
wants to merge 1 commit into from

Conversation

kroeckx
Copy link

@kroeckx kroeckx commented Sep 11, 2016

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.

@nodejs-github-bot 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
@@ -2435,7 +2545,6 @@ void SSLWrap<Base>::DestroySSL() {
return;

SSL_free(ssl_);
env_->isolate()->AdjustAmountOfExternalAllocatedMemory(-kExternalSize);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why should it be removed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

indutny commented Sep 11, 2016

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

cc @nodejs/crypto

@mscdex mscdex added c++ Issues and PRs that require attention from people who are familiar with C++. openssl Issues and PRs related to the OpenSSL dependency. and removed 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
@Trott
Copy link
Member

Trott commented Sep 11, 2016

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
Copy link
Author

kroeckx commented Sep 11, 2016

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
Copy link
Member

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

@addaleax addaleax added this to the 7.0.0 milestone Sep 21, 2016
@addaleax
Copy link
Member

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
Copy link
Member

jasnell commented Sep 21, 2016

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
Copy link
Contributor

Ping @nodejs/crypto

@jasnell
Copy link
Member

jasnell commented Oct 17, 2016

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
Copy link
Member

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.

@rvagg rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02
@jasnell jasnell removed this from the 7.0.0 milestone Oct 18, 2016
@jasnell
Copy link
Member

jasnell commented Oct 18, 2016

Clearing the v7 milestone from this.

// 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this fixed by the first commit in #8334?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

kapouer referenced this pull request Dec 7, 2016
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>
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
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
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.
davidben added a commit to davidben/node that referenced this pull request Nov 2, 2017
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.
davidben added a commit to davidben/node that referenced this pull request Nov 2, 2017
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.
rvagg pushed a commit that referenced this pull request Nov 11, 2017
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>
rvagg pushed a commit that referenced this pull request Nov 11, 2017
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>
rvagg pushed a commit that referenced this pull request Nov 11, 2017
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>
evanlucas pushed a commit that referenced this pull request Nov 13, 2017
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>
evanlucas pushed a commit that referenced this pull request Nov 13, 2017
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>
evanlucas pushed a commit that referenced this pull request Nov 13, 2017
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>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 7, 2018
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>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 7, 2018
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>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 7, 2018
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>
MylesBorins pushed a commit that referenced this pull request Feb 7, 2018
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>
MylesBorins pushed a commit that referenced this pull request Feb 7, 2018
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>
MylesBorins pushed a commit that referenced this pull request Feb 7, 2018
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>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 7, 2018
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>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 7, 2018
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>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 7, 2018
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>
gibfahn pushed a commit that referenced this pull request Feb 18, 2018
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>
gibfahn pushed a commit that referenced this pull request Feb 18, 2018
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>
gibfahn pushed a commit that referenced this pull request Feb 18, 2018
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. openssl Issues and PRs related to the OpenSSL dependency. stalled Issues and PRs that are stalled.
Projects
None yet
Development

Successfully merging this pull request may close these issues.