Fixed OpenSSL bindings to recognize LibreSSL #5676

LVMBDV · 2018-02-02T21:13:02Z

Used the C preprocessor from cc to get LIBRESSL_VERSION_NUMBER and LIBRESSL_VERSION_NUMBER which are used to determine OPENSSL_110 and OPENSSL_102, as @RX14 suggested. This fixes #4676. Tested on my Void Linux machine with LibreSSL.

RX14 · 2018-02-02T21:15:43Z

src/openssl/lib_ssl.cr

@@ -2,8 +2,8 @@ require "./lib_crypto"

 {% begin %}
  lib LibSSL
-    OPENSSL_110 = {{ `command -v pkg-config > /dev/null && pkg-config --atleast-version=1.1.0 libssl || printf %s false`.stringify != "false" }}
-    OPENSSL_102 = {{ `command -v pkg-config > /dev/null && pkg-config --atleast-version=1.0.2 libssl || printf %s false`.stringify != "false" }}
+    OPENSSL_110 = {{ LibCrypto::LIBRESSL_VERSION.nil? && (LibCrypto::OPENSSL_VERSION >= 0x1010100) }}


Just {{ LibCrypto::OPENSSL_110 }} should work, surely?

Yeah, my bad.

LVMBDV · 2018-02-02T21:44:38Z

I just realized the LIBRESSL_VERSION_NUMBER macro will return an empty string when OpenSSL is used. I need to fix that :o

LVMBDV · 2018-02-03T10:02:23Z

I think I fixed it. Ready for another review :)

ysbaddaden · 2018-02-03T12:08:04Z

Thanks!

Maybe we could add some LIBRESSL_VERSION checks to enable some features, for example ALPN support was added in LibreSSL 2.5.0 (https://github.com/libressl-portable/openbsd/blob/libressl-v2.5.0/src/lib/libcrypto/opensslv.h).

RX14 · 2018-02-03T12:13:12Z

@ysbaddaden another PR, please. Let's make it compile first before we add new libressl-spcific features.

Also, having library-specific features is absolutely the opposite of the direction I want to do. I want to remove the OpenSSL and make a library-inspecific, lowest common denominator, easy to use TLS module which works with openssl, libressl, libtls, gnutls, etc.

Sija · 2018-02-03T12:15:03Z

FYI, CI fails on OS X with <stdin>:1:10: fatal error: 'openssl/opensslv.h' file not found.

ysbaddaden · 2018-02-03T13:08:21Z

@RX14 Please reactor SSL in crystal, that would be awesome! Yet, there is no other way around the issue right now (and for the months to come).

Fixing LibreSSL compatibility is great, enabling ALPN on LibreSSL would be even better, doing it in this pull request be awesome.

LVMBDV · 2018-02-03T13:48:28Z

I agree with @RX14 on the scope of this PR. I would like it merged until the next release so I can shave some lines off the Void Linux package for Crystal :)

@Sija I don't know anything about where library headers are located in OS X. Could you explain?

luislavena

This looks great! Just had a small issue with cc usage.

luislavena · 2018-02-03T14:10:01Z

src/openssl/lib_crypto.cr

-    OPENSSL_102 = {{ `command -v pkg-config > /dev/null && pkg-config --atleast-version=1.0.2 libcrypto || printf %s false`.stringify != "false" }}
+    # An extra zero is appended to the output of LIBRESSL_VERSION to make it 0 when LibreSSL does not exist on the system.
+    # Any comparisons to it should be affixed with an extra zero as well e.g. `(LIBRESSL_VERSION_NUMBER >= 0x2050500F0)`.
+    LIBRESSL_VERSION = {{ `echo "#include <openssl/opensslv.h>\nLIBRESSL_VERSION_NUMBER" | cc -E -`.chomp.split('\n').last.split('L').first.id + "0" }}


This should be using CC from environment and only then fall back to cc

ENV["CC"]? || "cc"

this is in a shell, just use${CC:-cc}.

That might need to be revised once this work on non-bash/sh platforms like Windows. Better use now ENV than hunt down that later.

It needs to be revised anyway because it uses cc... And because it uses openssl in the first place. There's a million ways that this snippet, this file and this library are platform specific and env vars are the absolute least of them.

RX14 · 2018-02-03T14:25:44Z

@ysbaddaden is alpn available in openssl? Do you mean you just need a different API to enable ALPN in libressl? In that case then yes, that's a good idea and would fit in this PR (but not required).

Sija · 2018-02-03T16:10:37Z

@LVMBDV It should be possible to get this to work by installing openssl using brew (if not already):

brew install openssl

and then setting the CPPFLAGS and LDFLAGS to point to the openssl lib from brew:

export CPPFLAGS=-I/usr/local/opt/openssl/include
export LDFLAGS=-L/usr/local/opt/openssl/lib

RX14 · 2018-02-03T17:56:12Z

@Sija so everyone on mac is just going to get the same cryptic error as CI does unless you run a command nobody knows about? A solution that passes CI isn't enough. We should fall back to the old way if this way fails.

Sija · 2018-02-03T18:48:20Z

@Sija so everyone on mac is just going to get the same cryptic error as CI does unless you run a command nobody knows about? A solution that passes CI isn't enough.

Chill your boots dude. My comment refers to running this snippet on Mac OSX with openssl installed and was meant to help with this specific case, not all of 'em.

We should fall back to the old way if this way fails.

I agree, yet it's IMO different case; to which you haven't provided solutions either, yet.

LVMBDV · 2018-02-03T19:48:24Z

@cc is replaced with an environment variable friendly alternative as @luislavena suggested.
+ "0" is removed from OPENSSL_VERSION as its #define will always be there since this is about openssl in the first place.

About the Mac issue, I think Crystal as a mere compiler is not responsible for finding include paths. I'm sure developers who use Macs are well aware of the platform's eccentricities. Also openssl not being installed is not crystal's problem either, it just has the binding for it in case you want to use openssl with it.

P.S. I guess crystal tool format got really mad at that one space I left at the end of those system() calls. It passed std_spec.

LVMBDV · 2018-02-04T10:00:50Z

Fixed that one space, Travis is happy now. Ready for another review :)

ysbaddaden · 2018-02-04T13:44:06Z

@RX14 ALPN is in OpenSSL >= 1.0.2 and LibreSSL >= 2.5.0 with the same public API, and we already support it in Crystal for OpenSSL >= 1.0.2. It would be very kind to enable it for LibreSSL too. See: https://github.com/crystal-lang/crystal/blob/master/src/openssl/ssl/context.cr#L308

But beware of https://github.com/crystal-lang/crystal/blob/master/src/openssl/ssl/context.cr#L341 (and below) that may not be supported in LibreSSL.

RX14 · 2018-02-04T14:20:01Z

@ysbaddaden ah, thanks, that makes a lot more sense. I thought you were proposing ALPN as a new feature, I was unaware it was already supported. My bad.

RX14 · 2018-02-04T14:22:47Z

Either way we need to fix CI
I think that we should at the very least provide a better error message than this if we're going to fail to compile on OSX without the headers installed (common).
Should we just assume there's no libressl on osx and bypass this issue?

I really don't want to deal with the hundreds of bug reports we'll get if we merge this and everyone on OSX suddenly starts getting these errrors.

LVMBDV · 2018-02-04T17:59:37Z

Fixed the header problem for OS X but a new one popped up. I think crystal is linking against system's (older) openssl libs while using brew's openssl headers (which are newer). This is mentioned briefly in this blog post. I'm trying to fix it while looking at results over at CircleCI, might take slightly longer than usual since I have to wait for it :)

P.S. I might look at supporting ALPN for LibreSSL as well, after this.

LVMBDV · 2018-02-04T20:23:43Z

All checks passed! I was right about brew linking the system openssl instead of brew's more recent one. It must have been that way before I even started working on this. Anyways, I'll work on ALPN support with LibreSSL tomorrow.

RX14 · 2018-02-04T20:36:50Z

src/openssl/lib_crypto.cr

@@ -14,6 +14,10 @@
  end
 {% end %}

+# Check for brew's openssl libs on OS X
+{% if flag?(:darwin) %}
+  @[Link(ldflags: "-L/usr/local/opt/openssl/lib ")]


Homebrew can be installed in many different locations, hardcoding a location inside crystal sourcecode isn't a valid fix.

Surely homebrew should be setting the linker and compiler env vars correctly to link to the correct place? PATH ,LIBRARY_PATH, CPATH etc?

LVMBDV · 2018-02-04T22:57:24Z

Replaced hardcoded paths in darwin blocks with brew --prefix with /usr/local as a fallback.

asterite · 2018-02-04T23:26:05Z

Newest Mac OSX (High Sierra) comes with updated ssl/crypto libraries, there's no need (and in fact it might be wrong) to use homebrew for this.

I just tried changing lib LibSSL in mac to this:

@[Link("ssl")]
@[Link("crypto")]
lib LibSSL
  # ...
end

and it works fine.

But this didn't work well with a previous Mac version, so I don't know what advice to give on that. Maybe force users to update to the latest Mac OSX version is good.

Sija · 2018-02-04T23:46:51Z

@LVMBDV you can use brew --prefix openssl to obtain a full path.

LVMBDV · 2018-02-05T04:59:28Z

@Sija Can I have the output of brew --prefix openssl on your mac?
@asterite Can I have the output of llvm-config --host-target on your (High Sierra) mac?

LVMBDV · 2018-02-05T09:35:41Z

I tried my hand at ALPN support, it passes std_spec but I don't know enough about it to test it myself.

Moreover, I found out that Apple switched to LibreSSL as system default at High Sierra so I need to figure out a way to suppress the brew checks if the system is newer than or equal to High Sierra.

EDIT: OR modify behaviour with a compile time flag like OPENSSL_BREW instead, so users can choose which one to use.

LVMBDV · 2018-02-05T10:16:36Z

A warning can be printed to let the user know about this issue.

{{ p "OpenSSL bindings may fail on older (< 10.13) OS X versions without a homebrew openssl. In such a case, install openssl and set OPENSSL_BREW (i.e. crystal build -D OPENSSL_BREW)." if flag?(:darwin) && !flag?(:OPENSSL_BREW) }}

Should be using brew be the default behaviour or vice versa? CircleCI uses El Capitan so the build will fail if the default is not using brew, so will the rest of Mac users with older versions.

asterite · 2018-02-05T10:31:44Z

I think you missed my comment that homebrew shouldn't be used for this.

asterite · 2018-02-05T10:32:04Z

Oh, sorry, nevermind

Sija · 2018-02-05T11:20:29Z

@LVMBDV here you go:

~ ❯ brew --prefix openssl
/usr/local/opt/openssl

LVMBDV · 2018-03-24T08:07:57Z

Rebased but the deleted circle.yml contained a line to prepend the brew openssl directory to PKG_CONFIG_PATH, circleci fails without that.

RX14 · 2018-03-24T11:17:56Z

@LVMBDV the circle.yml is now in .circleci/config.yml, but perhaps you can edit bin/ci instead?

LVMBDV · 2018-03-24T15:14:36Z

Added the openssl path to PKG_CONFIG_PATH on bin/ci as @RX14 suggested :)

It currently doesn't work with LibreSSL anyway.

Sija · 2018-04-09T17:50:28Z

Seems like GTG, merge?

sdogruyol · 2018-04-13T07:09:22Z

LGTM 👍

oprypin · 2018-04-14T12:12:33Z

I compiled Crystal with this change, then tried to compile my program, got this:

Using compiled compiler at `.build/crystal'
_main.o: In function `__crystal_main':
/home/blaxpirit/repos/crystal/src/openssl/lib_ssl.cr:218: undefined reference to `SSL_library_init'
/home/blaxpirit/repos/crystal/src/openssl/lib_ssl.cr:218: undefined reference to `SSL_load_error_strings'
/home/blaxpirit/repos/crystal/src/openssl/lib_ssl.cr:218: undefined reference to `OPENSSL_add_all_algorithms_noconf'
/home/blaxpirit/repos/crystal/src/openssl/lib_ssl.cr:218: undefined reference to `ERR_load_crypto_strings'
O-penS-S-L-5858S-S-L-5858C-ontext.o: In function `default_method':
/home/blaxpirit/repos/crystal/src/openssl/ssl/context.cr:4: undefined reference to `SSLv23_method'
collect2: error: ld returned 1 exit status
Error: execution of command failed with code: 1: `cc "${@}" -o '/home/blaxpirit/projects/critter/critter'  -rdynamic  -lz `command -v pkg-config > /dev/null && pkg-config --libs --silence-errors libssl || printf %s '-lssl -lcrypto'` `command -v pkg-config > /dev/null && pkg-config --libs --silence-errors libcrypto || printf %s ' -lcrypto'` -lpcre -lm -lgc -lpthread /home/blaxpirit/repos/crystal/src/ext/libcrystal.a -levent -lrt -ldl -L/usr/lib -L/usr/local/lib`

I confirmed that this is the culprit

LLVM 6.0 on Arch Linux

RX14 · 2018-04-14T12:59:36Z

Ugh, I kinda expected this.

oprypin · 2018-04-14T13:06:14Z

Per @RX14's request

$ bin/crystal eval 'require "openssl"; {% pp LibCrypto::OPENSSL_VERSION, LibCrypto::LIBRESSL_VERSION %}'
Using compiled compiler at `.build/crystal'
LibCrypto::OPENSSL_VERSION  # => 269484175
LibCrypto::LIBRESSL_VERSION # => 0
_main.o: In function `__crystal_main':
/home/blaxpirit/repos/crystal/src/openssl/lib_ssl.cr:218: undefined reference to `SSL_library_init'
/home/blaxpirit/repos/crystal/src/openssl/lib_ssl.cr:218: undefined reference to `SSL_load_error_strings'
/home/blaxpirit/repos/crystal/src/openssl/lib_ssl.cr:218: undefined reference to `OPENSSL_add_all_algorithms_noconf'
/home/blaxpirit/repos/crystal/src/openssl/lib_ssl.cr:218: undefined reference to `ERR_load_crypto_strings'
collect2: error: ld returned 1 exit status
Error: execution of command failed with code: 1: `cc "${@}" -o '/tmp/cache-blaxpirit/crystal/crystal-run-eval.tmp'  -rdynamic  `command -v pkg-config > /dev/null && pkg-config --libs --silence-errors libssl || printf %s '-lssl -lcrypto'` `command -v pkg-config > /dev/null && pkg-config --libs --silence-errors libcrypto || printf %s ' -lcrypto'` -lpcre -lgc -lpthread /home/blaxpirit/repos/crystal/src/ext/libcrystal.a -levent -lrt -ldl -L/usr/lib -L/usr/local/lib`

RX14 · 2018-04-14T13:15:37Z

src/openssl/lib_crypto.cr

-@[Link(ldflags: "`command -v pkg-config > /dev/null && pkg-config --libs --silence-errors libcrypto || printf %s '-lcrypto'`")]
+{% begin %}
+  lib LibCrypto
+    OPENSSL_110 = {{ (LibCrypto::LIBRESSL_VERSION == 0) && (LibCrypto::OPENSSL_VERSION >= 0x10101000) }}


Isn't 0x101010 checking if openssl is greater than 1.1.1, not 1.1.0?

0x10101000 → 0x10100000 makes the problem go away

Openssl 1.1.1 is in prerelease, so i'm not sure how this happened.

Maybe someone just got too excited typing alternating ones and zeros 😆

I remember fixing the same thing in somewhere else, version integers are still new to me. My bad, thanks for fixing that.

Maybe someone just got too excited typing alternating ones and zeros

Probably 😆

…)" This reverts commit 72ce28f.

This reverts commit 72ce28f.

…)" (crystal-lang#6062) This reverts commit 72ce28f.

RX14 reviewed Feb 2, 2018

View reviewed changes

luislavena suggested changes Feb 3, 2018

View reviewed changes

RX14 reviewed Feb 4, 2018

View reviewed changes

lvmbdv added 6 commits March 24, 2018 10:57

Fixed OpenSSL bindings to recognize LibreSSL

7fcacfa

Fixed OpenSSL header parsing macros to work with OS X

c8a7ec5

Fixed OpenSSL linking on OS X

4c106f5

Added ALPN support for LibreSSL >= 2.5.0

dd2daac

Added a compilation flag to NOT use OpenSSL libs supplied by homebrew

2be7a5c

Added a compilation flag to use homebrew's LibreSSL instead of OpenSSL

f483d8d

Replaced homebrew specific hacks with pkg-config

eb71557

RX14 approved these changes Mar 24, 2018

View reviewed changes

RX14 referenced this pull request in alpinelinux/aports Mar 26, 2018

testing/crystal: remove depends on libressl

a755086

It currently doesn't work with LibreSSL anyway.

sdogruyol approved these changes Apr 13, 2018

View reviewed changes

sdogruyol merged commit 72ce28f into crystal-lang:master Apr 13, 2018

sdogruyol added the kind:feature label Apr 13, 2018

sdogruyol added this to the Next milestone Apr 13, 2018

RX14 reviewed Apr 14, 2018

View reviewed changes

RX14 mentioned this pull request Apr 14, 2018

Fix linking to openssl #5949

Merged

LVMBDV deleted the fix/4676-libressl-support branch April 20, 2018 15:25

RX14 added a commit to RX14/crystal that referenced this pull request May 4, 2018

Revert "Fixed OpenSSL bindings to recognize LibreSSL (crystal-lang#5676…

1ad5645

…)" This reverts commit 72ce28f.

RX14 added a commit that referenced this pull request May 5, 2018

Revert "Fixed OpenSSL bindings to recognize LibreSSL (#5676)" (#6062)

6c3fc2b

This reverts commit 72ce28f.

asterite pushed a commit that referenced this pull request May 8, 2018

Revert "Fixed OpenSSL bindings to recognize LibreSSL (#5676)" (#6062)

68dab3f

This reverts commit 72ce28f.

asterite pushed a commit that referenced this pull request May 8, 2018

Revert "Fixed OpenSSL bindings to recognize LibreSSL (#5676)" (#6062)

991cbfd

This reverts commit 72ce28f.

chris-huxtable pushed a commit to chris-huxtable/crystal that referenced this pull request Jun 6, 2018

Revert "Fixed OpenSSL bindings to recognize LibreSSL (crystal-lang#5676…

00e5fdb

…)" (crystal-lang#6062) This reverts commit 72ce28f.

LVMBDV mentioned this pull request Oct 7, 2018

Fixed OpenSSL bindings to work with LibreSSL #6917

Merged

Fixed OpenSSL bindings to recognize LibreSSL #5676

Fixed OpenSSL bindings to recognize LibreSSL #5676

Conversation

LVMBDV commented Feb 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LVMBDV commented Feb 2, 2018

LVMBDV commented Feb 3, 2018

ysbaddaden commented Feb 3, 2018

RX14 commented Feb 3, 2018

Sija commented Feb 3, 2018

ysbaddaden commented Feb 3, 2018

LVMBDV commented Feb 3, 2018 • edited Loading

luislavena left a 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

Choose a reason for hiding this comment

RX14 Feb 3, 2018 • edited Loading

Choose a reason for hiding this comment

RX14 commented Feb 3, 2018 • edited Loading

Sija commented Feb 3, 2018 • edited Loading

RX14 commented Feb 3, 2018

Sija commented Feb 3, 2018

LVMBDV commented Feb 3, 2018 • edited Loading

LVMBDV commented Feb 4, 2018

ysbaddaden commented Feb 4, 2018

RX14 commented Feb 4, 2018

RX14 commented Feb 4, 2018

LVMBDV commented Feb 4, 2018 • edited Loading

LVMBDV commented Feb 4, 2018

Choose a reason for hiding this comment

LVMBDV commented Feb 4, 2018

asterite commented Feb 4, 2018

Sija commented Feb 4, 2018

LVMBDV commented Feb 5, 2018

LVMBDV commented Feb 5, 2018 • edited Loading

LVMBDV commented Feb 5, 2018

asterite commented Feb 5, 2018

asterite commented Feb 5, 2018

Sija commented Feb 5, 2018

LVMBDV commented Mar 24, 2018

RX14 commented Mar 24, 2018

LVMBDV commented Mar 24, 2018

Sija commented Apr 9, 2018

sdogruyol commented Apr 13, 2018

oprypin commented Apr 14, 2018

RX14 commented Apr 14, 2018

oprypin commented Apr 14, 2018 • edited Loading

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

LVMBDV commented Feb 3, 2018 •

edited

Loading

RX14 Feb 3, 2018 •

edited

Loading

RX14 commented Feb 3, 2018 •

edited

Loading

Sija commented Feb 3, 2018 •

edited

Loading

LVMBDV commented Feb 3, 2018 •

edited

Loading

LVMBDV commented Feb 4, 2018 •

edited

Loading

LVMBDV commented Feb 5, 2018 •

edited

Loading

oprypin commented Apr 14, 2018 •

edited

Loading