Fix: detect OpenSSL/LibreSSL from official C headers

[ysbaddaden]

Detecting the OpenSSL version using pkg-config is prone to bugs when a libssl.pc or libcrypto.pc definition aren't available, or pkg-config itself isn't available. Using a C preprocessor to extract the official OPENSSL_VERSION_NUMBER and LIBRESSL_VERSION_NUMBER constants from the openssl/opensslv.h header should always be correct.

The version numbers are a little cryptic when compared to their text counterpart, but this is what's expected by OpenSSL, so we should follow.

Of course, if pkg-config if installed we use it to detect the correct version (linking will use it).

fixes #7244

[ysbaddaden]

I manually tested OpenSSL versions 1.0.1f, 1.0.2h, 1.1.0e and 1.1.1.

Can someone test LibreSSL?

[RX14]

Needs a format, and it's broken on osx :(

I suspect that osx might not even package openssl headers by default.

[ysbaddaden]

Yeah, macOS doesn't, but brew installed recent, supported, versions of OpenSSL:

/usr/local/Cellar/openssl/1.0.2l/include/openssl/opensslv.h
/usr/local/Cellar/openssl/1.0.2q/include/openssl/opensslv.h

But pkg-config isn't installed so I guess that's only working because we're conservative, and expect the obsolete 0.9.8 by default. Maybe with pkg-config we could find the newest versions? Or we should fallback to the obsolete 0.9.8 version number?

[RX14]

OpenSSL has become a cost centre for the standard library, and I'd like to propose replacing it with mbedtls.

mbedtls, previously known as polarssl, is an established tls library which:

• has a simple, easy to use API
• has a good security history
• has the financial support of a large company (ARM)
• is actively developed
• is already packaged for debian, ubuntu, centos, fedora, arch linux, *bsd ports, and many others
• is easily portable to new architectures and operating systems
• and most importantly, has a stable ABI guarantee

I suggest that we can switch to using polarssl exclusively as our TLS implementation, and it would save a lot of headaches going forward.

Here's how I suggest switching:

• Add a new TLS module to abstract the TLS API from the implementation. This would allow us to change the underlying library used for tls and crypto operations in the standard library after 1.0, without making any breaking changes.
• Extract the OpenSSL module to a shard, where the community can develop it.
• Implement the TLS module with mbedtls.

[ysbaddaden]

I'm all in favor to abstract a TLS module in Crystal's stdlib, allowing to choose or switch to any library (OpenSSL, BearSSL, GnuTLS, ...) by using a selected shard.

I have some concerns with mbedtls. Looking at TLS linux distributions, there are differently named libraries: libpolarssl.so, libmbedtls.so, and some different ABI versions, which will probably be confusing. Packaged versions of PolarSSL are far outdated (1.3.9 in Debian Jessie for example). It probably requires to switch to mbedtls which may be available or not. One must enable jessie-backports to install it on Debian Jessie, and it's an old 2.4.2 release, that didn't get any update since sept 2017, despite seeing some CVE in newer releases... I'm not sure it has the kind of extended support that old OpenSSL releases can have (in RHEL, Ubuntu or Debian).

OpenSSL is ubiquitous. The API doesn't break much. I spend a couple hours once a year to fix some removed deprecations when a new version is released, and that's it. A real issue has been detecting and supporting LibreSSL, which is supposed to be an OpenSSL 1.0.0 replacement, but isn't exactly, and choosing to use text version numbers, instead of extracting the real version numbers was probably a bad choice. The last issue is that some OS are stuck with a far outdated 0.9.8 for which we should just drop support, like Ruby did 2 years ago: ruby/openssl@4eb4b32.

[ysbaddaden]

Note that the crystal brew depends on openssl, which does install a supported version (1.0.2) but:

openssl is keg-only, which means it was not symlinked into /usr/local,

This means we need to set some things up to use the library we asked to install (sic).

Anyway: CI/darwin is fixed by defaulting to 0.9.8.

[straight-shoota]

@RX14 Let's make that a separate thread, first of all focusing on extracting an implementation-independent TLS API.

[ysbaddaden]

Alright, we have 2 choices here:

1. Default the version to 0.9.8 when no header can be found;

2. Configure CI/darwin to use the OpenSSL library we asked to install (using pkg-config and PKG_CONFIG_PATH) at least with macOS 10.12; 10.13 may work better since it ships libressl.

We can also fail compilation when headers can't be found. It's an error after all.

I believe the TLS interface and mbedtls are different topics :)

[RX14]

I much prefer version 2.

We shouldn't support unmaintained openssl versions.

[ysbaddaden]

Squashed and reordered, with a patch to fix LibreSSL support. Manually tested against:

• OpenSSL 1.0.1f (extended support), 1.0.2h, 1.1.0e and 1.1.1 (officially supported);
• LibreSSL 2.2.7 (macOS high sierra), 2.7.5, 2.8.3 and 2.9.0 (officially supported).

[bcardiff]

• I would prefer to have semantic version formatted strings used all around :-(
• The brew formula only depends on pkgconfig on build, when bottle the dependency is not installed. Maybe that should change?
• I don't see where https://github.com/Homebrew/homebrew-core/blob/master/Formula/crystal.rb depends on openssl (ref: Fix: detect OpenSSL/LibreSSL from official C headers #7245 (comment))

[ysbaddaden]

I would prefer to have semantic version formatted strings used all around

Yes, it's ugly, but that's the way to distinguish versions for OpenSSL, let's not fight it (we lose). We can't expect the pkgconfig definition to be present. OPENSSL_VERSION_TEXT is an arbitrary text and parsing it to extract the version will probably break. We could build the version text by parsing OPENSSL_VERSION_NUMBER, but is that really worth the hassle?

The brew formula only depends on pkgconfig on build, when bottle the dependency is not installed. Maybe that should change?

I don't know. Can someone run the openssl specs from this pull request on a recent macOS release (high sierra or later)? Maybe the libressl integration got a little better... or maybe it happens to work because libressl keeps openssl 1.0.0 ABI compatibility, and still doesn't provide any header (:hankey:) ?

I don't see where Formula/crystal.rb depends on openssl

The formula may not, but the log of brew install crystal does installs openssl on CircleCI. I don't know why. Maybe it's being updated because it was installed in the CircleCI VM?

[straight-shoota]

Openssl doesn't even follow semver ;)

[bcardiff]

I would prefer to have semantic version formatted strings used all around

Yes, it's ugly, but that's the way to distinguish versions for OpenSSL, let's not fight it (we lose). We can't expect the pkgconfig definition to be present. OPENSSL_VERSION_TEXT is an arbitrary text and parsing it to extract the version will probably break. We could build the version text by parsing OPENSSL_VERSION_NUMBER, but is that really worth the hassle?

Oh well...

I don't see where Formula/crystal.rb depends on openssl

The formula may not, but the log of brew install crystal does installs openssl on CircleCI. I don't know why. Maybe it's being updated because it was installed in the CircleCI VM?

ruby formula which is updated to 2.3.7 seems to be the one that installs openssl.

[RX14]

* The brew formula only depends on pkgconfig on build, when bottle the dependency is not installed. Maybe that should change?

Yes, it should. Crystal's debian packages already depend on pkg-config. A lot of Crystal assumes pkg-config is available in practice.

The crystal homebrew formula should ensure that the correct PKG_CONFIG_PATH is set for the crystal compiler to see openssl, however it decides to do that.

[ysbaddaden]

Looking at some openssl bindings, I noticed that the rust crate had some very nice ideas:

1. We can have less ugly version numbers, using a 0x1_00_01_00_0 notation for "major minor fix patch status" (see OPENSSL_VERSION_NUMBER documentation). It's definitely better than 0x10001000.

2. We can check the default homebrew directory on macOS (/usr/local/opt/openssl) and even ask brew about it with brew --prefix openssl. That's simple and should just work, without requiring any configuration by the user —it's a shame that openssl is poorly managed on macOS, but that's Apple's fault, not Crystal users' fault.

3. We could improve error reporting. Ask to run brew install openssl on macOS when homebrew is installed, or remind to install the the development packages like libssl-dev or openssl-devel on Linux, and so on.

[RX14]

2. We can check the default homebrew directory on macOS (/usr/local/opt/openssl) and even ask brew about it with brew --prefix openssl. That's simple and should just work, without requiring any configuration by the user —it's a shame that openssl is poorly managed on macOS, but that's Apple's fault, not Crystal users' fault.

No, I refuse to merge this. This hack belongs in Homebrew, not Crystal. If Crystal even needs to know Homebrew exists, I will strongly argue against merging it.

Homebrew should wrap crystal (or pkg-config), and export the correct PKG_CONFIG_DIR so that the build environment is correct. Crystal should build against and link to openssl correctly when a C compiler would build against and link to openssl correctly, nothing more. It is not Crystal's job to fix the build environment! Layering violations cause more work for us maintainers, and more edge cases for users.

[ysbaddaden]

I don't care about homebrew and macOS, but lots of developers do use them. Adding a few lines to a script file to help the most common scenario ain't much.

Especially since this patch will break all macOS+brew installations, because it only worked by chance until now —defaulting to a 0.9.8/1.0.0 version that happened to be the system installed one.

[RX14]

I don't care about homebrew and macOS, but lots of developers do use them. Adding a few lines to a script file to help the most common scenario ain't much.

I'm not advocating breaking anything, I'm just advocating moving the hack where it belongs.

[ysbaddaden]

Then I let someone that's actually using macOS and brew to handle the fix properly in the crystal brew formula. For example:

• patching src/openssl/version.sh to search for /usr/local/opt/openssl then fallback to brew --prefix openssl (unless pkg-config passed);
• make pkg-config a mandatory requirement and fix PKG_CONFIG_PATH in the crystal wrapper shell script (hopefully we won't need a fix for each and every binding in stdlib);
• whatever other solution that works.

Until some as an acceptable solution for both crystal and homebrew, we should wait before merging this PR, or just merge it with the brew fix. I'm fine with whatever, as long as we do fix openssl for homebrew users in the end.

[RX14]

• make pkg-config a mandatory requirement and fix PKG_CONFIG_PATH in the crystal wrapper shell script (hopefully we won't need a fix for each and every binding in stdlib);

This was what I was proposing. Either pkg-config, or headers in the default paths (i.e. no CFLAGS or LDFLAGS required, i.e. most sane linux distros/BSDs)

Looking at homebrew: pkg-config is compiled with a default pkg-config path which contains the default homebrew locations. So I think just editing the homebrew formula to install pkg-config should be enough.

Can you test CI without any osx hacks and just install homebrew's pkg-config and see if that works?

[RX14]

Where is OPENSSL_DIR coming from?

[ysbaddaden]

pkg-config is compiled with a default pkg-config path (...) I think just editing the homebrew formula to install pkg-config should be enough.

No it won't. openssl.pc isn't in a standard location (like /usr/local/lib/pkgconfig) but in a specific installation folder for openssl (/usr/local/opt/openssl/lib/pkgconfig), this is on purpose.

Where is OPENSSL_DIR coming from?

Just a simple mean to quickly select an installed version. Could be removed, but it's nice to just type OPENSSL_DIR=/opt/openssl-1.1.1. It's simpler and less error prone than typing PKG_CONFIG_PATH=/opt/openssl-1.1.1/lib/pkgconfig:$PKG_CONFIG_PATH (or wait, was it lib/pkg-config or PKGCONFIG_DIR? I always forget and dig stackoverflow), especially when trying to test one specific version, among many installed versions.

pkgconfig is a nice tool when things are installed globally on the system, yet awful when they're not —and we circle with brew's openssl installation not being in a standard location 😭

[RX14]

No it won't. openssl.pc isn't in a standard location (like /usr/local/lib/pkgconfig) but in a specific installation folder for openssl (/usr/local/opt/openssl/lib/pkgconfig), this is on purpose.

I don't understand why they'd do this :/ If Homebrew wants to be awkward, we need to discuss with Homebrew what the proper fix is. Hacking around it without understanding Homebrew's point of view seems ill-advised.

Just a simple mean to quickly select an installed version. Could be removed, but it's nice to just type OPENSSL_DIR=/opt/openssl-1.1.1. It's simpler and less error prone than typing PKG_CONFIG_PATH=/opt/openssl-1.1.1/lib/pkgconfig:$PKG_CONFIG_PATH (or wait, was it lib/pkg-config or PKGCONFIG_DIR? I always forget and dig stackoverflow), especially when trying to test one specific version, among many installed versions.

Then can it become CRYSTAL_OPENSSL_DIR? I can imagine conflicts with that name...

[ysbaddaden]

• OpenSSL linking issues due to removal of keg_only status Homebrew/legacy-homebrew#41613
• Homebrew/legacy-homebrew@2e191b1
• link: don't link openssl. Homebrew/brew#597

[RX14]

This just need to be merged without the macos-specific stuff and then use a wrapper script to fix the build environment in Homebrew. I don't see why homebrew wouldn't accept that. For now the PKG_CONFIG_PATH workaround should be added in CI with a note to remove.

[InfRandomness]

Hey, sorry to dig-up such an old thread,
I've read the discussion and it looks like this got very close to being merged, I was wondering what was the status of this as of today (3 years later)
my goal being to try to push crystal on freebsd to a better state

[dmgk]

Echoing @InfRandomness question above.

On FreeBSD, openssl is available both from the base system and from ports, and Crystal currently requires patching to use openssl from the base (#7244).

It looks that HomeBrew issue was resolved by Homebrew/homebrew-core#48028, which seemed to be the only objection to merging this PR.

[straight-shoota]

I wouldn't mind polishing up this PR and merging it.

But overall, I'd rather like a more flexible, general approach for configuring lib bindings. There should be easy options to specify which libraries and versions to link to at compile time. This should technically work without having OpenSSL sources locally installed (or rather, despite), for example for cross-compiling workflows.
This problem is by far not a specific for libssl/libcrypto but applies to many other library bindings, hence I'd look for a more generic solution.

[ysbaddaden]

Honestly, I don't see any alternative, especially a generic one. There is #8336 but it only works for types and "constants", so we still need checking versions to know whether a function is available or not. It would help avoid the shell script, though, which does just that (preprocess the C headers to extract the version constants).

[asterite]

We can always improve #8336 or do something similar that checks whether a function is define. This can be done by creating a small C program and check if it compiles or not.