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

OpenSSL 1.1.1 Support TODO (pls help) #18770

Closed
rvagg opened this issue Feb 13, 2018 · 68 comments
Closed

OpenSSL 1.1.1 Support TODO (pls help) #18770

rvagg opened this issue Feb 13, 2018 · 68 comments
Labels
help wanted Issues that need assistance from volunteers or PRs that need help to proceed.

Comments

@rvagg
Copy link
Member

rvagg commented Feb 13, 2018

@nodejs/crypto

OpenSSL 1.1.1-pre1 was released today. The headline item is TLS 1.3 (worth noting that the spec hasn't quite been finalised yet). This is obviously only a pre-release, not final and not supposed to be entirely bug free.

The OpenSSL team have said previously that 1.1.1 would be API and ABI compatible with 1.1.0. We currently have 1.1.0 support in Node so the theory goes that it shouldn't be too difficult an upgrade path. This is nice because it's possible (but not yet known) that 1.1.1 is the next LTS of OpenSSL, with 1.1.0 going EOL soon. 1.1.0 -> 1.1.1 or just straight to 1.1.1 might have to be our Node 10 strategy (I'm outlining that case here).

So, getting as close to 1.1.1 support as possible even while it's pre-release would be very valuable for us. Maintaining 1.0.2 and 1.1.0 support in the meantime is preferable (perhaps essential thanks to distribution dependencies). There will be a time, after 1.0.2 EOL next year, that we can ditch all the cruft but for now if we can do all 3 then that's what we should do.

Our CI tests 1.0.2 (obviously) and 1.0.2 dynamically linked. It also tests dynamic linking to 1.1.0 in Node 9+ (soon 8 too I think). See https://ci.nodejs.org/job/node-test-commit-linux-containered/ for this happening.

I've also 1.1.1-pre1 to the same containers that are used to run these other dynamic-linked tests and I can turn that on as needed. For now it's too broken to turn on full-time, so this is the call to help fix that!

Node compiles just fine with 1.1.1-pre1 thanks to @davidben's most excellent work in #16130. But it currently fails 55 tests in CI (there may be at least one async-wrap flaky in there).

We need help figuring out whether these are things that we can fix on our end or whether they are upstream problems. If OpenSSL 1.1.1 isn't properly API compatible with 1.1.0 then I'd like us to push back on them to get them to stick to that commitment.

not ok 36 parallel/test-async-wrap-GH13045
not ok 953 parallel/test-https-agent-create-connection
not ok 957 parallel/test-https-agent-session-reuse
not ok 964 parallel/test-https-client-resume
not ok 969 parallel/test-https-agent-additional-options
not ok 1130 parallel/test-http2-https-fallback
not ok 1230 parallel/test-https-drain
not ok 1234 parallel/test-https-eof-for-eom
not ok 1519 parallel/test-tls-alpn-server-client
not ok 1533 parallel/test-tls-client-getephemeralkeyinfo
not ok 1534 parallel/test-tls-client-mindhsize
not ok 1535 parallel/test-tls-client-reject
not ok 1536 parallel/test-tls-addca
not ok 1537 parallel/test-tls-alert-handling
not ok 1539 parallel/test-tls-async-cb-after-socket-end
not ok 1542 parallel/test-tls-close-notify
not ok 1549 parallel/test-tls-connect-stream-writes
not ok 1553 parallel/test-tls-client-resume
not ok 1554 parallel/test-tls-disable-renegotiation
not ok 1555 parallel/test-tls-ecdh
not ok 1556 parallel/test-tls-ecdh-auto
not ok 1558 parallel/test-tls-ecdh-multiple
not ok 1562 parallel/test-tls-env-extra-ca
not ok 1566 parallel/test-tls-client-verify
not ok 1568 parallel/test-tls-getcipher
not ok 1569 parallel/test-tls-connect-given-socket
not ok 1576 parallel/test-tls-dhe
not ok 1577 parallel/test-tls-friendly-error-message
not ok 1583 parallel/test-tls-multi-key
not ok 1584 parallel/test-tls-multi-pfx
not ok 1585 parallel/test-tls-interleave
not ok 1586 parallel/test-tls-invoke-queued
not ok 1590 parallel/test-tls-npn-server-client
not ok 1591 parallel/test-tls-ocsp-callback
not ok 1592 parallel/test-tls-js-stream
not ok 1599 parallel/test-tls-peer-certificate-encoding
not ok 1600 parallel/test-tls-peer-certificate-multi-keys
not ok 1602 parallel/test-tls-net-connect-prefer-path
not ok 1607 parallel/test-tls-securepair-server
not ok 1608 parallel/test-tls-no-rsa-key
not ok 1611 parallel/test-tls-server-verify
not ok 1612 parallel/test-tls-on-empty-socket
not ok 1614 parallel/test-tls-set-ciphers
not ok 1615 parallel/test-tls-sni-option
not ok 1616 parallel/test-tls-sni-server-client
not ok 1618 parallel/test-tls-socket-constructor-alpn-npn-options-parsing
not ok 1619 parallel/test-tls-regr-gh-5108
not ok 1625 parallel/test-tls-ticket
not ok 1626 parallel/test-tls-ticket-cluster
not ok 1648 parallel/test-tls-server-connection-server
not ok 1871 async-hooks/test-tlswrap
not ok 1878 async-hooks/test-writewrap
not ok 1945 parallel/test-tls-set-encoding
not ok 1952 parallel/test-tls-socket-default-options
not ok 2012 sequential/test-benchmark-tls

Full output is captured here https://gist.github.com/rvagg/cdead09ffa269453d728dcf9bc831d3d (it comes from here but that link is not going to survive).

@davidben
Copy link
Contributor

This probably accounts for some of it:
#16130 (comment)
I filed openssl/openssl#4574 for the immediate trigger, but I think Node's reentrancy is also somewhat questionably supportable.

Other bits I suspect come out of them enabling TLS 1.3 by default and the tests being sensitive to there not being a version beyond TLS 1.2.

(But I haven't triaged and am mostly speculating.)

@richardlau richardlau added the help wanted Issues that need assistance from volunteers or PRs that need help to proceed. label Feb 14, 2018
@shigeki
Copy link
Contributor

shigeki commented Feb 14, 2018

I'm now working on build bindings with 1.1.1-pre1 and will look at test errors later.

@shigeki
Copy link
Contributor

shigeki commented Feb 22, 2018

Here is my preliminary work to support OpenSSL-1.1.1 on only Linux and Mac. https://github.com/shigeki/node/tree/openssl111pre1_unix_mac_ok
I can build node binary but several tests are still failed as described above.

Windows is not yet supported because OpenSSL-1.1.1 generates makefiles to work only nmake for Windows and we have to some integrations to gmake. It needs some time to work on Windows.

@rvagg
Copy link
Member Author

rvagg commented Feb 27, 2018

pre2 is out, looks like a ton of pending refactoring and removals got merged in for this one https://github.com/openssl/openssl/commits/master
I haven't tested this yet

@shigeki
Copy link
Contributor

shigeki commented Feb 28, 2018

OpenSSL-1.1.1-pre1 binding without asm support of https://github.com/shigeki/node/tree/WIP_openssl111pre_noasm has almost been done.

Most of CI results of
https://ci.nodejs.org/job/node-test-commit/16544/ are fine except in some platforms. I'm going to work on them with pre2 soon.

Most of test failures is due to cipher suite incompatibilities between TLS1.2 and TLS1.3 and it can be resolved by limiting to use TLS1.2 by default. The issue is now discussing in openssl/openssl#5359.

@rvagg
Copy link
Member Author

rvagg commented Feb 28, 2018

Very nice work @shigeki. I notice there's a failure on testing 1.0.2 support:

../src/node_crypto.cc: In static member function 'static void node::crypto::SecureContext::Init(const v8::FunctionCallbackInfo<v8::Value>&)':
19:03:36 ../src/node_crypto.cc:636:49: error: 'SSL_CTX_set_max_proto_version' was not declared in this scope
19:03:36    SSL_CTX_set_max_proto_version(sc->ctx_, TLS1_2_VERSION);

@ https://ci.nodejs.org/job/node-test-commit-linux-containered/2545/nodes=ubuntu1604_sharedlibs_openssl102_x64/

Is it going to be hard to main backward compatibility?

@shigeki
Copy link
Contributor

shigeki commented Feb 28, 2018

It is the API which is available above 1.1.0 so that
#if OPENSSL_VERSION_NUMBER >= 0x10100000L .. #endif
would be working.

@rvagg
Copy link
Member Author

rvagg commented Sep 6, 2018

So, we are at pre9 now, TLS 1.3 is finalized and ratified as an RFC, and Node 10 LTS is only a month(ish) away. It would be nice to have 1.1.1 in Node 10 before we go LTS but if we have to do it after then so be it. I have a 1.1.1-pre test job for Linux in CI that I'd like to enable so we can start getting ready but we don't quite have compatibility.

@shigeki the commit at the head of upgrade_openssl111_pre6_tls12_only, shigeki@26388fe, we get most of the way there for OpenSSL 1.1.1 as a shared library. It still fails test-tls-handshake-error, test-tls-set-ciphers-error, test-tls-server-verify and test-tls-connect.

Any idea whether these are easy to overcome and what the impact on non-Linux is at the moment? CI can only do dynamic compiles on Linux at the moment but it's a start and we could use it to make a basic assertion that 1.1.1 isn't going to be a breaking upgrade. I don't know if other collaborators would accept a PR with support for a -pre library but I wouldn't mind trying soon if we can.

@bricss
Copy link

bricss commented Sep 11, 2018

OpenSSL 1.1.1 final is here -> https://www.openssl.org/blog/blog/2018/09/11/release111/

@shigeki
Copy link
Contributor

shigeki commented Sep 12, 2018

Here is my prototype on upgrading latest OpenSSL-1.1.1 release with only TLS1.2 support not TLS1.3.

https://github.com/shigeki/node/tree/WIP_upgrade_openssl111_tls12_only

This has one new feature to have options.min_version/max_version to limit the use of TLS versions.
Onetest-tls-server-verify test was failed due to client auth error during renegotiation that need to be investigated.

This is only working on Linux. TODO is to support multi platforms especially on MacOS, Win and s390 that can be checked through CI. I think that it does not have any breaking updates as long as it supports only up to TLS1.2.

Supporting TLS1.3 in Node is an another story. It needs much more works and would have breaking updates because of changes of info callbacks of its TLS state machine and deprecated/new several features for TLS1.3 in OpenSSL-1.1.1.

@shigeki
Copy link
Contributor

shigeki commented Sep 12, 2018

The failure of test-tls-server-verify was identified and an issue was submitted in openssl/openssl#7199.

@rvagg
Copy link
Member Author

rvagg commented Sep 20, 2018

@shigeki it sounds like from openssl/openssl#7199 that we may have some fundamental architecture problems with our TLS layer. It sounds like the TLS 1.3 support problem is related to the test-tls-server-verify failure too, is that right? Does that mean that we're going to have a lot more changes on Node's side to get 1.1.1 in? And if so, do we need to get additional help for that? Is there a risk that we're going to need to have breaking changes just to get 1.1.1 in properly?

@xnox
Copy link

xnox commented Oct 1, 2018

Point of note: On Ubuntu, we are building OpenSSL 1.1.1 with OPENSSL_TLS_SECURITY_LEVEL set to 0 and all TLS protocol versions enabled. It would be nice for node.js 11.0.0 to use 1.1.1 abi/api, and to explicitly limit max TLS version to 1.2 then. This will thus allow us to ship nodejs, and later when things are fixed for 1.3, enable 1.3 without changing base api/abi requirement of the openssl library.

@sam-github
Copy link
Contributor

@rvagg @shigeki At the request of @mhdawson I started to look at #18770 (comment)

There is one more TODO: rebase on master. When rebasing to master the addition of min_/max_version conflicts with #23820 I'm working on this in https://github.com/sam-github/node/tree/WIP_upgrade_openssl111_tls12_only, it builds, but I'm reviewing to make sure its correct.

@rvagg
Copy link
Member Author

rvagg commented Oct 31, 2018

Very nice @sam-github, I'll take a look as soon as I can. A bit busy this week but this is exciting stuff!
How did you go with solving the test-tls-server-verify / SSL_CB_HANDSHAKE_START/DONE thing? I was under the impression that this was a fairly fundamental problem.

@shigeki
Copy link
Contributor

shigeki commented Oct 31, 2018

@sam-github Thanks for your work. I will take a look at it later.

@rvagg sam-github@a1c64c7 is the tentative fix for it but it does not solve the issues of TLS1.3 adoption.

@volschin
Copy link

Please don‘t forget to make the new curves accessible via crypto api.

@shigeki
Copy link
Contributor

shigeki commented Oct 31, 2018

@sam-github I found that this has the AVX-512 errors in building on MacOS as follows. I will fix it so as to disable AVX-512 support on MacOS.

../deps/openssl/config/archs/darwin64-x86_64-cc/asm/crypto/chacha/chacha-x86_64.s:2203:2: error: instruction requires: AVX-512 ISA
 vbroadcasti32x4 L$sigma(%rip),%zmm0
 ^
../deps/openssl/config/archs/darwin64-x86_64-cc/asm/crypto/chacha/chacha-x86_64.s:2204:2: error: instruction requires: AVX-512 ISA
 vbroadcasti32x4 (%rcx),%zmm1
 ^
../deps/openssl/config/archs/darwin64-x86_64-cc/asm/crypto/chacha/chacha-x86_64.s:2205:2: error: instruction requires: AVX-512 ISA
 vbroadcasti32x4 16(%rcx),%zmm2

@sam-github
Copy link
Contributor

sam-github commented Oct 31, 2018

@shigeki I worry that our branches will diverge. Can I push https://github.com/sam-github/node/tree/WIP_upgrade_openssl111_tls12_only to the nodejs/node repo, so we can both push fixups to it, and perhaps PR it so comment is more possible? If you would prefer I (EDIT: can) PR your branch, or anything else, I am happy to do as you suggest.

As for tls: add min/max_version and their defaults, I reworked it slightly to include API docs and not modify it's options argument.

Your change modifies the arguments of tls.SecureContext() which is exported but undocumented. Do we consider this to be semver-major? If so, the min and max arguments could be moved to a field in the pre-existing options argument.

@sam-github
Copy link
Contributor

sam-github commented Oct 31, 2018

@shigeki re:

Onetest-tls-server-verify test was failed due to client auth error during renegotiation that need to be investigated.

I get 100% test pass on linux. Is the failure ephemeral, or was this the failure fixed by tls: workaround handshakedone in renegotiation?

EDIT: I am sorry, I ignore this. Its the same question Rod asked and you answered.

@sam-github
Copy link
Contributor

@volschin Is there anything special to do? How do I check that what curves are available? I tried this:

core/node (WIP_upgrade_openssl111_tls12_only % u+756-7) % ./node -p 'crypto.getCurves()' > _head
core/node (WIP_upgrade_openssl111_tls12_only % u+756-7) % nvm run 11 -p 'crypto.getCurves()' > _11
core/node (WIP_upgrade_openssl111_tls12_only % u+756-7) % diff -u _11 _head
--- _11 2018-10-31 14:14:59.874786762 -0700
+++ _head       2018-10-31 14:13:57.405782356 -0700
@@ -1,5 +1,6 @@
 [ 'Oakley-EC2N-3',
   'Oakley-EC2N-4',
+  'SM2',
   'brainpoolP160r1',
   'brainpoolP160t1',
   'brainpoolP192r1',

@sam-github
Copy link
Contributor

  • ABI stability: I don't know
  • TLSv1.3 being semver-major: I doubt there is any way to know until after its been done. Given how things moved around in OpenSSL, its entirely possible handshake interactions will be subtly but noticeably different when using TLSv1.3. But then, I guess it only has to be backwards compat when using TLSv1.2 and below, when using a new TLS protocol, we'd expect the API to be different (and it will.... they use a different ciphersuite selection mechanism among other things).

@noloader
Copy link

noloader commented Dec 4, 2018

Regarding "Error: instruction requires: AVX-512 ISA", see LLVM Issue 39875, Error: instruction requires: AVX-512 ISA when using -mavx512f. The short of it is, Clang 7.0 is needed to sidestep a problem with the Clang integrated assembler. The root cause is LLVM Issue 36202, llvm-mc doesn't accept AVX512 instructions using %zmm16+.

@sam-github
Copy link
Contributor

sam-github commented Dec 12, 2018

Took a shot at figuring out whether updating OpenSSL on v10.x will break anything.

Ran tests on the v10.x branch: https://ci.nodejs.org/job/node-test-commit/24242

Passed everything, except a build against openssl 1.1.0, where TLS1_3_VERSION macro is missing, which I fixed in both "upgrade" branches. Failure here for the record.

With a v10.x that uses openssl1.1.1a, I tried to test backwards compat, but finding an npm module that:

  1. is native
  2. uses openssl; and
  3. has seen a release in the last year
  4. builds and passes its tests

... was not so easy. I'm happy to take suggestions. I searched for openssl and crypto keywords on npmjs.com, and also went through all the citgm modules tagged "native".

#0  0x00007ffff7a9c357 in kill () at ../sysdeps/unix/syscall-template.S:78
#1  0x0000555555cfc8b9 in uv_kill (pid=<optimized out>, signum=<optimized out>)
    at ../deps/uv/src/unix/process.c:584
#2  0x0000555555c15f73 in node::Kill(v8::FunctionCallbackInfo<v8::Value> const&) ()
#3  0x0000555555e064a8 in v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<false>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, v8::internal::BuiltinArguments) ()
#4  0x0000555555e07a86 in v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) ()                                                
#5  0x000030dc706dbe1d in ?? ()
      ... no symbols in the binding?

... I found a handful of others that didn't build or pass their tests, didn't include them.

@sam-github
Copy link
Contributor

@MylesBorins 👆

@sam-github
Copy link
Contributor

I also took a shot at searching github for openssl extension:.gyp language:JavaScript pushed:>2018-01-01, it didn't really work. I found nothing I didn't already find through npm. Maybe because it doesn't exist? If anyone knows a way to find all repos with a top-level binding.gyp, and that do #include.*openssl somewhere, that'd be really useful.

@kapouer
Copy link
Contributor

kapouer commented Dec 14, 2018

@sam-github uws and grpc both link against openssl. For uws, you can test on node 10 using @kapouer/uws.

@sam-github
Copy link
Contributor

@kapouer I could use some help, what I found was:

  • grpc doesn't support npm test. I poked around a bit, tried to run find
    a way to run the tests, but they always fail because of a missing deps/grpc/etc/roots.pem
  • @kapouer/uws

@noloader
Copy link

noloader commented Dec 14, 2018

@sam-github,

... but they always fail because of a missing deps/grpc/etc/roots.pem ...

Regarding the missing PEM file, the following may help:

echo "-----BEGIN CERTIFICATE-----
MIIDdTCCAl2gAwIBAgILBAAAAAABFUtaw5QwDQYJKoZIhvcNAQEFBQAwVzELMAkG
A1UEBhMCQkUxGTAXBgNVBAoTEEdsb2JhbFNpZ24gbnYtc2ExEDAOBgNVBAsTB1Jv
b3QgQ0ExGzAZBgNVBAMTEkdsb2JhbFNpZ24gUm9vdCBDQTAeFw05ODA5MDExMjAw
MDBaFw0yODAxMjgxMjAwMDBaMFcxCzAJBgNVBAYTAkJFMRkwFwYDVQQKExBHbG9i
YWxTaWduIG52LXNhMRAwDgYDVQQLEwdSb290IENBMRswGQYDVQQDExJHbG9iYWxT
aWduIFJvb3QgQ0EwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQDaDuaZ
jc6j40+Kfvvxi4Mla+pIH/EqsLmVEQS98GPR4mdmzxzdzxtIK+6NiY6arymAZavp
xy0Sy6scTHAHoT0KMM0VjU/43dSMUBUc71DuxC73/OlS8pF94G3VNTCOXkNz8kHp
1Wrjsok6Vjk4bwY8iGlbKk3Fp1S4bInMm/k8yuX9ifUSPJJ4ltbcdG6TRGHRjcdG
snUOhugZitVtbNV4FpWi6cgKOOvyJBNPc1STE4U6G7weNLWLBYy5d4ux2x8gkasJ
U26Qzns3dLlwR5EiUWMWea6xrkEmCMgZK9FGqkjWZCrXgzT/LCrBbBlDSgeF59N8
9iFo7+ryUp9/k5DPAgMBAAGjQjBAMA4GA1UdDwEB/wQEAwIBBjAPBgNVHRMBAf8E
BTADAQH/MB0GA1UdDgQWBBRge2YaRQ2XyolQL30EzTSo//z9SzANBgkqhkiG9w0B
AQUFAAOCAQEA1nPnfE920I2/7LqivjTFKDK1fPxsnCwrvQmeU79rXqoRSLblCKOz
yj1hTdNGCbM+w6DjY1Ub8rrvrTnhQ7k4o+YviiY776BQVvnGCv04zcQLcFGUl5gE
38NflNUVyRRBnMRddWQVDf9VMOyGj/8N7yy5Y0b2qvzfvGn9LhJIZJrglfCm7ymP
AbEVtQwdpf5pLGkkeB6zpxxxYu7KyJesF12KwvhHhm4qxFYxldBniYUr+WymXUad
DKqC5JlR3XC321Y9YeRq4VzW9v493kHMB65jUr9TU/Qr6cf9tveCX4XSQRjbgbME
HMUfpIBvFSDJ3gyICh3WZlXi/EjJKSZp4A==
-----END CERTIFICATE-----" > globalsign-root-r1.pem

wget -q --ca-certificate=globalsign-root-r1.pem https://curl.haxx.se/ca/cacert.pem -O cacert.pem

mkdir -p deps/grpc/etc/
cp cacert.pem deps/grpc/etc/roots.pem

I can only say "may" because I don't know what is expected in roots.pem.

@sam-github
Copy link
Contributor

Helps a bit, still doesn't build. I ran all the gulp targets, twice (in case there was an order dependence). Always fails with:

> [email protected] build /home/sam/s/grpc-node/packages/grpc-native-core                                                                                                          
> node-pre-gyp build                                                                                                                                                         
                                                                                                                                                                             
make: Entering directory '/home/sam/s/grpc-node/packages/grpc-native-core/build'                                                                                             
make: *** No rule to make target 'Release/obj.target/grpc/deps/grpc/src/core/lib/surface/init.o', needed by 'Release/obj.target/libgrpc.a'.  Stop. 

If you can give me directions to test from a clean git clone, I will try again. You can do it, too, might be easier, just build my update_openssl1.1.a-v10.x branch.

@sam-github
Copy link
Contributor

There doesn't seem to be any blockers, or activity, so at @mhdawson 's suggestion, I have PRed the update to OpenSSL 1.1.1a: #25381

Also, I have a branch on top of the basic openssl1.1.1a for TLSv1.2 (and below) where I continue to work through the test suites getting them to either pass when run with TLSv1.3, or identify outstanding work. I'm making progress. I think I've identified how to resolve the main issues, but its hard to know when one of the tests will reveal a nasty new problem, so I will wait until I'm through them before feeling too confident.

@sam-github
Copy link
Contributor

@shigeki @rvagg TLS1.3 update. I've run through all the test-tls- tests, and found the problems with TLS1.3, as well as read the OpenSSL release notes. I've prototyped solutions to all the problems, except the problems with the info callback. For that, I'm trying to change TLSWrap to use SSL_do_handshake() until the handshake completes. I have had some success with simple connections, but there are still problems, and I'm deep in the weeds of node C++ streams, and reentrant C++/JS.

If either of you have any cred with the OpenSSL team, please, please jump in on this thread: https://mta.openssl.org/pipermail/openssl-project/2019-January/thread.html#1204

It looks like nginx and haprox are also affected by the unnecessary API breakage between openssl 1.1.0 and 1.1.1. I can't even post to the relevant mailing list (Matt Caswell claims its moderated, but AFAICT, my emails are just bounced), maybe you two have access?

If the changes made in that thread occur, then the TLS1.3 handshake problem melts away, and I think I can get TLS1.3 working in relatively short order, just a couple new APIs, some docs, etc.

@sam-github
Copy link
Contributor

@nodejs/tsc I've no idea how the OpenSSL core team works, but ☝️ , perhaps an official request from node is in order? I (and others on that thread) believe this to be an API breakage between releases of OpenSSL that are not supposed to have API breakage. There was a judement call made, but with implementation experience available, I think they made it the wrong way.

@noloader
Copy link

noloader commented Jan 23, 2019

@sam-github,

Since you are working in C++ already, maybe Jack Lloyd's Botan would be a better for Node. Botan is a C++ TLS library. It is a cross-platform library and runs on the major platforms, including BSDs, AIX, Linux, OS X and Windows.

Development is active and the code is very clean. Jack also takes a more disciplined approach to the software engineering process (than many other projects found on the web).

One thing it is missing is Andy Polyakov's hand tuned assembly language routines. But Botan uses builtins and intrinsics so performance is not off by much. He uses builtins because of mostly universal compiler support. You can write once, run anywhere.

Something to think about in the future...

@addaleax
Copy link
Member

@noloader Thanks for weighing in, but switching libraries is off-topic for this thread, as well as not easily done (we need support for more than the major platforms, and we provide the OpenSSL API to native add-ons, so changing libraries is a significant breaking change).

@sam-github
Copy link
Contributor

TLS1.3 draft PR: #26209

@kapouer
Copy link
Contributor

kapouer commented Feb 27, 2019

@sam-github openssl 1.1.1b was released with the api fix. Maybe that means node 10 could upgrade to openssl 1.1.1b without breaking abi ?

@sam-github
Copy link
Contributor

node 10 could upgraded to openssl 1.1.1a without breaking ABI or API, and it likely will: #26270

But openssl1.1.1b means I should be able to remove a work-around from Node.js, yes (though the work-around does no harm, it basically just removes TLS1.3 cipher suites, but TLS1.3 isn't supported yet anyhow)(and the workaround goes away in the TLS1.3 PR).

We'll see when #26327 lands if it does so in time to make the next 10.x. It might not, because it would need to be in an 11.x release for a while first, as I understand it. That's OK, once 10.x is using openssl 1.1.1a, then updating to 1.1.1b can be done in a patch release.

/cc @MylesBorins @rvagg

@sam-github
Copy link
Contributor

Actually, I can't remove the work-around from Node.js even with openssl1.1.1b, because its possible to link node.js against a system openssl, and it could be openssl1.1.1a. That's OK. Like I said, the work-around disappears in #26209

@sam-github
Copy link
Contributor

We're up to openssl-1.1.1b in master, #26327, and TLS1.3 has landed, #26209. Thanks for all the people who helped with this.

I think this issue can be closed now, but if someone feels differently, please reopen.

And that's not to imply that help isn't still wanted for tls/crypto support. More people working on it would be very much appreciated!

@panva
Copy link
Member

panva commented Mar 21, 2019

@volschin

@sam-github I‘m not sure, whats working automatically.
getcurves should show up with X25519 and X448. Both curves should be usable inside ECDH class e.g. the Alice and Bob example from the docs.
The complete test changes for X448 in openssl you can see here
rhuijben/openssl@fe93b01

I've opened up #26626 for that particular feature request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Issues that need assistance from volunteers or PRs that need help to proceed.
Projects
None yet
Development

No branches or pull requests