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

crypto: initial LibreSSL support (WIP) #9376

Closed
wants to merge 1 commit into from
Closed

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Oct 31, 2016

(A bit of weekend fun)

WIP. It mostly works and would do fine for 95% of use cases as is. But I'm not making any guarantees about actually finishing this out, it's at the long-tail of complex debugging so it might need someone either much smarter than me or with more time on their hands or both.

This PR does not indicate official support, or even that I'm advocating official support for LibreSSL! It's merely a POC and RFC and perhaps we can get iterate on this and get LibreSSL semi-officially support without too much pain. But the core team will have to decide whether the complexity required (which IMO isn't too much, considering what it's doing) is worth it in the long run. FWIW I'm not someone who believes that LibreSSL is inherently more safe or secure, I will still trust my deployments to OpenSSL and recommend others do so, particularly now with the significant investment from the Core Infrastructure Initiative, the expanded team, demonstrably improving code quality and significantly more eyes on the code than the alternative forks. However, there are Linux distros (and OpenBSD) that are embracing LibreSSL and there is a non-trivial groundswell of interested parties, so it's worth us considering it. Of course BoringSSL is another candidate for support here, I have no idea of the delta there though.

If we do decide to support it, we could fairly easily add at least one test node to our CI cluster that would keep it compiling and passing tests. Feature-parity can be in the hands of interested parties, we just skip features and tests that are too hard to add and not add the burden to crypto development.

Steps to compile on either Linux or macOS (both have equal support atm):

  1. Get LibreSSL, either on a system that has it installed by default or install it somewhere separate on your own system. LibreSSL has a DESTDIR you can use when you make install to put it in it's own location. I've only tried v2.5.0, there's a couple of APIs in here that are fairly new I think so ymmv if you use something older.
  2. Set up your library path
  • Linux: export LD_LIBRARY_PATH=/path/to/libressl/lib/
  • macOS: export DYLD_LIBRARY_PATH=/path/to/libressl/lib/
  1. ./configure --shared-openssl --shared-openssl-includes=/path/to/libressl/include/ --shared-openssl-libname=ssl,crypto --shared-openssl-libpath=/path/to/libressl/lib/
  2. build, test and profit

Some implementation notes:

  • process.versions.openssl gets set to '2.5.0-LibreSSL', so you can /LibreSSL$/.test(process.versions.openssl).
  • setFreeListLength() isn't supported so it's possible that this may be impacted by high tls memory usage (rss) #1522 but I haven't tested
  • The async OCSP and SNI stuff introduced in tls: use SSL_set_cert_cb for async SNI/OCSP #1464 isn't supported by LibreSSL, by itself that is responsible for the bulk of the diff here.
  • getEphemeralKeyInfo() isn't supported and I can't come up with a satisfactory workaround. That means we can't support minDHSize, introduced in Add a new option to limit DH key size in tls connect #1831. I'm betting it's not going to be hard for someone to come up with a way around this though.
  • Not all tests pass, currently 7 although I did have it at 6 at one stage (not entirely sure which one was different!) this is what's going to need work:
    • parallel/test-tls-alpn-server-client fails - something's up with NPN negotiation
    • parallel/test-tls-no-sslv3 fails - I haven't looked into this (also, FYI, you might want to PATH=/path/to/libressl/bin:$PATH to use libressl's openssl)
    • parallel/test-tls-cnnic-whitelist, parallel/test-tls-ocsp-callback, parallel/test-tls-pfx-gh-5100-regr all segfault, I believe it's the same error, consuming too many bytes or an unexpected certificate parse or something. Needs moar debugging.
    • parallel/test-tls-sni-server-client is not happy, SNI context stuff was one of the bits I had to mess with the most, obviously still not there
    • parallel/test-tls-sni-option fails - might be similar or same reason as above
  • Some tests have been bypassed or adjusted
    • parallel/test-crypto doesn't check for 'sha' in the list of hashes, it doesn't come by default with LibreSSL
    • parallel/test-tls-client-getephemeralkeyinfo is completely disabled cause it's not supported (see above)
    • parallel/test-tls-client-mindhsize is completely disabled cause it's not supported (see above)
    • parallel/test-tls-cnnic-whitelist (also segfaulting) has to do a check for an 'CERT_UNTRUSTED' error instead of a 'UNABLE_TO_GET_ISSUER_CERT_LOCALLY' error.
    • parallel/test-tls-empty-sni-context is disabled, see notes in there about the errors LibreSSL produces that aren't matched by the regexes in there. Disabling isn't a good strategy here, perhaps some error rewriting is needed or just accept the error output as it is?

CI @ https://ci.nodejs.org/job/node-test-commit/5883/ (i.e. everything is still in order when this stuff isn't turned on)

/cc @nodejs/crypto @nodejs/build

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. tls Issues and PRs related to the tls subsystem. labels Oct 31, 2016
} catch (e) {
self.destroy(e);
if (isLibreSSL) {
self._handle.endParser();
Copy link
Member Author

@rvagg rvagg Oct 31, 2016

Choose a reason for hiding this comment

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

This is not quite right, a call to certCbDone() is probably appropriate here somewhere too, my original iteration completely removed certCbDone when LibreSSL but the current incarnation only removes pieces of it so it's still usable.

@lin7sh
Copy link

lin7sh commented Nov 1, 2016

This is so good. node port in openbsd is still in 4.X. I don't know if Libressl is the reason for that or not, but this PR definitely increase the overall platform support

@qbit
Copy link
Contributor

qbit commented Nov 1, 2016

@mko-io The reason the OpenBSD port is still 4.x is because 6.x just became the "recommended" version, and I haven't updated the port yet. :)

@lluixhi
Copy link

lluixhi commented Feb 1, 2017

libressl-2.5.1 includes SSL_get_server_tmp_key

EDIT 4/3: I suppose I should clarify -- SSL_get_server_tmp_key is the function used for GetEphemeralKeyInfo, so hypothetically that should work now.

@rvagg
Copy link
Member Author

rvagg commented Mar 3, 2017

rebased to master

15 failures when using LibreSSL 2.4

>30 failures when using LibreSSL 2.5, I think a bit too much has changed internally and I need to revisit some of the assumptions I've been making. The only difference need in code to make it compile against 2.5 is the implementation of X509_up_ref() in src/node_crypto.cc, which is new (see comments in there). I wouldn't recommend using this against 2.5 at this stage fwiw.

@jbergstroem
Copy link
Member

Someone willing to take this up (@rvagg ?!?!?) should really create a patreon page. Seeing how some distributions are already looking to lean against this patch (!) and a lot of people seems to make noise both in here and the other libressl-related threads I'm sure there's an opportunity to raise money dedicated towards this goal.

@maffblaster
Copy link

I would love to see libressl supported (hate risking non-redistributable binary code) as an alternative to OpenSSL. Please continue!!

@aduskett
Copy link

aduskett commented Aug 3, 2017

I am the Buildroot LibreSSL maintainer, and I would love to be able to compile node against LibreSSL!

It's a shame this hasn't been kept up with!

@BridgeAR
Copy link
Member

Ping @rvagg

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Aug 29, 2017
@BridgeAR
Copy link
Member

Closing due to long inactivity. Please reopen if you want to follow up on this @rvagg. I would like to get this in.

@BridgeAR BridgeAR closed this Sep 12, 2017
@rvagg
Copy link
Member Author

rvagg commented Oct 27, 2017

For anyone stumbling across this: my intention was not to finish this off, doing the final pieces is a bit too much of a stretch for the mental bandwidth I have available. My hope was that someone else might pick this up and finish it off, or at least push it further. If you are such a person then take my code and use it as you like if it's helpful at all. Ping me if you need any help making progress getting something back into the project, I'd be happy to help champion getting LibreSSL compile support merged. My branch is still at https://github.com/rvagg/io.js/tree/libressl and is now behind master by enough distance that rebasing is going to be a bit of a pain unfortunately. Feel free to email me if you want to chat about it (GitHub notifications aren't so great for me atm).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. stalled Issues and PRs that are stalled. tls Issues and PRs related to the tls subsystem. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants