[Openvpn-devel] Fix compilation with LibreSSL

  TLS 1.3 is not ready yet in LibreSSL.
Also SSL_get1_supported_ciphers() has been just added into master (not yet
released).

Signed-off-by: Stefan Strogin <stefan.strogin@gmail.com>
---
 src/openvpn/ssl_openssl.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
  

Am 23.02.19 um 19:02 schrieb Stefan Strogin:
> TLS 1.3 is not ready yet in LibreSSL.
> Also SSL_get1_supported_ciphers() has been just added into master (not yet
> released).

So I written already a rant about LibreSSL in this trac ticket
(https://community.openvpn.net/openvpn/ticket/1159), I will just copy
the relevant part here:

Biggest problem is that we have no one who really tests or works with us
doing LibreSSL. There have been multiple people saying that will work
with us and maintain LibreSSL but that has not materialised yet. And I
am getting quite fed up with way that LibreSSL does API compatiblity. It
claims to support OpenSSL 2.0.0 API when it clearly doesn't. This also
means that every new feature needs an or !LIBRE_SSL. I would be okay to
add FEATURE macros for libressl if they actually support new featues
like TLS1.3 etc but the current way is just a pain to maintain.
Also the patch should be send to the mailing list for proper review.

The || !defined(LIBRESSL_VERSION_NUMBER) means that I have no idea of
knowing when it is safe to remove the code. The idea of having the
OPENSSL_VERSION macros is to remove the ifdefs when we drop support for
old version but the unversioned LIBRESSL sounds like we would have to
keep ancient OpenSSL APIs forever in the code to support LibreSSL.

Your comment that TLS 1.3 and SSL_get1_supported_ciphers will become
available with some later LibreSSL, means that the current patch is
wrong here and we will need to patch it again.

In summary I really do not like this stuff. But I also accept that
having these defines in there makes same sense to have at least
unofficial support.  If you can send a follow up patch that at least
uses some feature or libressl version in the ifdef, we have an easier
time identifying those parts in the future to be obsolete.

Arne

> Signed-off-by: Stefan Strogin <stefan.strogin@gmail.com>
> ---
>  src/openvpn/ssl_openssl.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
> index ddb78da7..fcaac080 100644
> --- a/src/openvpn/ssl_openssl.c
> +++ b/src/openvpn/ssl_openssl.c
> @@ -465,7 +465,7 @@ tls_ctx_restrict_ciphers_tls13(struct tls_root_ctx *ctx, const char *ciphers)
>          return;
>      }
>  
> -#if (OPENSSL_VERSION_NUMBER < 0x1010100fL)
> +#if (OPENSSL_VERSION_NUMBER < 0x1010100fL) || defined(LIBRESSL_VERSION_NUMBER)
>      crypto_msg(M_WARN, "Not compiled with OpenSSL 1.1.1 or higher. "
>                 "Ignoring TLS 1.3 only tls-ciphersuites '%s' setting.",
>                 ciphers);
> @@ -1998,7 +1998,7 @@ show_available_tls_ciphers_list(const char *cipher_list,
>          crypto_msg(M_FATAL, "Cannot create SSL_CTX object");
>      }
>  
> -#if (OPENSSL_VERSION_NUMBER >= 0x1010100fL)
> +#if (OPENSSL_VERSION_NUMBER >= 0x1010100fL && !defined(LIBRESSL_VERSION_NUMBER))
>      if (tls13)
>      {
>          SSL_CTX_set_min_proto_version(tls_ctx.ctx, TLS1_3_VERSION);
> @@ -2019,7 +2019,7 @@ show_available_tls_ciphers_list(const char *cipher_list,
>          crypto_msg(M_FATAL, "Cannot create SSL object");
>      }
>  
> -#if (OPENSSL_VERSION_NUMBER < 0x1010000fL)
> +#if (OPENSSL_VERSION_NUMBER < 0x1010000fL) || defined(LIBRESSL_VERSION_NUMBER)
>      STACK_OF(SSL_CIPHER) *sk = SSL_get_ciphers(ssl);
>  #else
>      STACK_OF(SSL_CIPHER) *sk = SSL_get1_supported_ciphers(ssl);
>
  

Hi Arne,

Thanks for the reply. Please see my comments and a clarifying question below.

On 24/02/2019 03:28, Arne Schwabe wrote:
> And I
> am getting quite fed up with way that LibreSSL does API compatiblity. It
> claims to support OpenSSL 2.0.0 API when it clearly doesn't.

Sorry, but LibreSSL officially claims only compatibility with OpenSSL 1.0.1:
https://github.com/libressl-portable/portable
However, practically most of OpenSSL 1.1 API is supported by LibreSSL.

> The || !defined(LIBRESSL_VERSION_NUMBER) means that I have no idea of
> knowing when it is safe to remove the code. The idea of having the
> OPENSSL_VERSION macros is to remove the ifdefs when we drop support for
> old version but the unversioned LIBRESSL sounds like we would have to
> keep ancient OpenSSL APIs forever in the code to support LibreSSL.
> 
> Your comment that TLS 1.3 and SSL_get1_supported_ciphers will become
> available with some later LibreSSL, means that the current patch is
> wrong here and we will need to patch it again.
> 
> In summary I really do not like this stuff. But I also accept that
> having these defines in there makes same sense to have at least
> unofficial support.  If you can send a follow up patch that at least
> uses some feature or libressl version in the ifdef, we have an easier
> time identifying those parts in the future to be obsolete.

I see your point.
Regarding TLS 1.3. LibreSSL 2.9.0 (current development release) defines
OPENSSL_NO_TLS1_3, but LibreSSL <2.9.0 doesn't. Therefore it could be:

#if (OPENSSL_VERSION_NUMBER < 0x1010100fL) \
    || (defined(LIBRESSL_VERSION_NUMBER) && LIBRESSL_VERSION_NUMBER < 0x2090000fL) \
    || defined(OPENSSL_NO_TLS1_3)
    crypto_msg(M_WARN, "Not compiled with OpenSSL 1.1.1 or higher. "
               "Ignoring TLS 1.3 only tls-ciphersuites '%s' setting.",
(...)

alternatively we can just use TLS1_3_VERSION instead of all version numbers, i.e.:

#if !defined(TLS1_3_VERSION)
    crypto_msg(M_WARN, "Not compiled with OpenSSL 1.1.1 or higher. "
               "Ignoring TLS 1.3 only tls-ciphersuites '%s' setting.",
(...)

AFAIK in theory OpenSSL >=1.1.1 can be built without TLS 1.3 support, unlikely
in practice though.
Which ifdef will you prefer?

Regarding SSL_get1_supported_ciphers. [Based upon current LibreSSL release schedule
(2.9.x is a current development branch which will be part of OpenBSD 6.5) and that
this method was added soon after 2.9.0 release], I think it will be correct:
#if (OPENSSL_VERSION_NUMBER < 0x1010000fL) || \
    (defined(LIBRESSL_VERSION_NUMBER) && LIBRESSL_VERSION_NUMBER < 0x2090100fL)

I also see other 'defined(LIBRESSL_VERSION_NUMBER)' ifdefs that don't specify
exact LibreSSL version number. I agree that it is not correct, and it is not nice
to force deprecated functions on users of newer LibreSSL, when newer API is
available. So if you don't mind I'll try to fix them as well in a follow-up patch.

--
Stefan

> 
> Arne
> 
>> Signed-off-by: Stefan Strogin <stefan.strogin@gmail.com>
>> ---
>>  src/openvpn/ssl_openssl.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
>> index ddb78da7..fcaac080 100644
>> --- a/src/openvpn/ssl_openssl.c
>> +++ b/src/openvpn/ssl_openssl.c
>> @@ -465,7 +465,7 @@ tls_ctx_restrict_ciphers_tls13(struct tls_root_ctx *ctx, const char *ciphers)
>>          return;
>>      }
>>  
>> -#if (OPENSSL_VERSION_NUMBER < 0x1010100fL)
>> +#if (OPENSSL_VERSION_NUMBER < 0x1010100fL) || defined(LIBRESSL_VERSION_NUMBER)
>>      crypto_msg(M_WARN, "Not compiled with OpenSSL 1.1.1 or higher. "
>>                 "Ignoring TLS 1.3 only tls-ciphersuites '%s' setting.",
>>                 ciphers);
>> @@ -1998,7 +1998,7 @@ show_available_tls_ciphers_list(const char *cipher_list,
>>          crypto_msg(M_FATAL, "Cannot create SSL_CTX object");
>>      }
>>  
>> -#if (OPENSSL_VERSION_NUMBER >= 0x1010100fL)
>> +#if (OPENSSL_VERSION_NUMBER >= 0x1010100fL && !defined(LIBRESSL_VERSION_NUMBER))
>>      if (tls13)
>>      {
>>          SSL_CTX_set_min_proto_version(tls_ctx.ctx, TLS1_3_VERSION);
>> @@ -2019,7 +2019,7 @@ show_available_tls_ciphers_list(const char *cipher_list,
>>          crypto_msg(M_FATAL, "Cannot create SSL object");
>>      }
>>  
>> -#if (OPENSSL_VERSION_NUMBER < 0x1010000fL)
>> +#if (OPENSSL_VERSION_NUMBER < 0x1010000fL) || defined(LIBRESSL_VERSION_NUMBER)
>>      STACK_OF(SSL_CIPHER) *sk = SSL_get_ciphers(ssl);
>>  #else
>>      STACK_OF(SSL_CIPHER) *sk = SSL_get1_supported_ciphers(ssl);
>>
>
  

Am 25.02.19 um 07:29 schrieb Stefan Strogin:
> Hi Arne,
> 
> Thanks for the reply. Please see my comments and a clarifying question below.
> 
> On 24/02/2019 03:28, Arne Schwabe wrote:
>> And I
>> am getting quite fed up with way that LibreSSL does API compatiblity. It
>> claims to support OpenSSL 2.0.0 API when it clearly doesn't.
> 
> Sorry, but LibreSSL officially claims only compatibility with OpenSSL 1.0.1:
> https://github.com/libressl-portable/portable
> However, practically most of OpenSSL 1.1 API is supported by LibreSSL.

I am speaking about the value of OPENSSL_VERSION_NUMBER and iirc that
one is set to something that equals OpenSSL 2.0.0.

Which is the main cause of these problems that we run into.


> AFAIK in theory OpenSSL >=1.1.1 can be built without TLS 1.3 support, unlikely
> in practice though.
> Which ifdef will you prefer?



Hm, both have advantages. But if we can fix the rare corner case of
OpenSSL 1.1.1 without TLS_13 then lets go for the TLS1_3_VERSION variant.

> 
> Regarding SSL_get1_supported_ciphers. [Based upon current LibreSSL release schedule
> (2.9.x is a current development branch which will be part of OpenBSD 6.5) and that
> this method was added soon after 2.9.0 release], I think it will be correct:
> #if (OPENSSL_VERSION_NUMBER < 0x1010000fL) || \
>     (defined(LIBRESSL_VERSION_NUMBER) && LIBRESSL_VERSION_NUMBER < 0x2090100fL)


Are there really Libressl versions that do not define
LIBRESSL_VERSION_NUMBER that are worth supporting? Otherwise I would
prefer to have only the LIBRESSL_VERSION_NUMBER < 0x2090100fL or
inverted and >,. Need to double check what is true for undefined things.

> 
> I also see other 'defined(LIBRESSL_VERSION_NUMBER)' ifdefs that don't specify
> exact LibreSSL version number. I agree that it is not correct, and it is not nice
> to force deprecated functions on users of newer LibreSSL, when newer API is
> available. So if you don't mind I'll try to fix them as well in a follow-up patch.

That would be good improvement indeed.

Arne
  

On 25/02/2019 13:10, Arne Schwabe wrote:
> Am 25.02.19 um 07:29 schrieb Stefan Strogin:
> Hm, both have advantages. But if we can fix the rare corner case of
> OpenSSL 1.1.1 without TLS_13 then lets go for the TLS1_3_VERSION variant.

Thanks, I'll use this one then.

> Are there really Libressl versions that do not define
> LIBRESSL_VERSION_NUMBER that are worth supporting? Otherwise I would
> prefer to have only the LIBRESSL_VERSION_NUMBER < 0x2090100fL or
> inverted and >,. Need to double check what is true for undefined things.

"After all replacements due to macro expansion and the defined unary operator have been performed, all remaining identifiers (including those lexically identical to keywords) are replaced with the pp-number 0" - C99, 6.10.1.
So "#if (LIBRESSL_VERSION_NUMBER < 0x2090100fL)" will be true with undefined
LIBRESSL_VERSION_NUMBER.

--
Stefan