> On 19 Dec 2023, at 12:58, Maxim Dounin <mdounin@mdounin.ru> wrote:
>
> Hello!
>
> On Tue, Dec 19, 2023 at 02:09:10AM +0400, Sergey Kandaurov wrote:
>
>>> On 24 Nov 2023, at 00:29, Ilya Shipitsin <chipitsine@gmail.com> wrote:
>>>
>>> # HG changeset patch
>>> # User Ilya Shipitsin <chipitsine@gmail.com>
>>> # Date 1700769135 -3600
>>> # Thu Nov 23 20:52:15 2023 +0100
>>> # Node ID 2001e73ce136d5bfc9bde27d338865b14b8ad436
>>> # Parent 7ec761f0365f418511e30b82e9adf80bc56681df
>>> ssl: SSL_get0_verified_chain is available for LibreSSL >= 3.3.6
>>
>> style: SSL prefix should be uppercase.
>>
>>>
>>> diff -r 7ec761f0365f -r 2001e73ce136 src/event/ngx_event_openssl_stapling.c
>>> --- a/src/event/ngx_event_openssl_stapling.c Thu Oct 26 23:35:09 2023 +0300
>>> +++ b/src/event/ngx_event_openssl_stapling.c Thu Nov 23 20:52:15 2023 +0100
>>> @@ -893,7 +893,8 @@
>>> ocsp->cert_status = V_OCSP_CERTSTATUS_GOOD;
>>> ocsp->conf = ocf;
>>>
>>> -#if (OPENSSL_VERSION_NUMBER >= 0x10100000L && !defined LIBRESSL_VERSION_NUMBER)
>>> +/* minimum OpenSSL 1.1.1 & LibreSSL 3.3.6 */
>>> +#if (OPENSSL_VERSION_NUMBER >= 0x10100000L && !defined LIBRESSL_VERSION_NUMBER) || (defined(LIBRESSL_VERSION_NUMBER) && (LIBRESSL_VERSION_NUMBER >= 0x3030600L))
>>>
>>> ocsp->certs = SSL_get0_verified_chain(c->ssl->connection);
>>>
>>
>> Testing "defined(LIBRESSL_VERSION_NUMBER)" is superfluous.
>> The macro test suffers from a very long line.
>>
>> The correct version test seems to be against LibreSSL 3.5.0, see
>> https://ftp.openbsd.org/pub/OpenBSD/LibreSSL/libressl-3.5.0-relnotes.txt
>>
>> So, the resulting change would be as follows:
>>
>> diff --git a/src/event/ngx_event_openssl_stapling.c b/src/event/ngx_event_openssl_stapling.c
>> --- a/src/event/ngx_event_openssl_stapling.c
>> +++ b/src/event/ngx_event_openssl_stapling.c
>> @@ -893,7 +893,9 @@ ngx_ssl_ocsp_validate(ngx_connection_t *
>> ocsp->cert_status = V_OCSP_CERTSTATUS_GOOD;
>> ocsp->conf = ocf;
>>
>> -#if (OPENSSL_VERSION_NUMBER >= 0x10100000L && !defined LIBRESSL_VERSION_NUMBER)
>> +#if (OPENSSL_VERSION_NUMBER >= 0x10100000L \
>> + && !defined LIBRESSL_VERSION_NUMBER) \
>> + || LIBRESSL_VERSION_NUMBER >= 0x3050000fL
>
> Rather,
>
> +#if (OPENSSL_VERSION_NUMBER >= 0x10100000L \
> + && (!defined LIBRESSL_VERSION_NUMBER \
> + || LIBRESSL_VERSION_NUMBER >= 0x3050000fL))
>
Agree. For the sake of completeness:
# HG changeset patch
# User Sergey Kandaurov <pluknet@nginx.com>
# Date 1703004490 -14400
# Tue Dec 19 20:48:10 2023 +0400
# Node ID 267cee796462f4f6bacf825c8fd24d13845d36f4
# Parent 7a6d52990f2e2d88460a3dc6cc84aac89b7329ea
SSL: using SSL_get0_verified_chain() with LibreSSL 3.5.0+.
Prodded by Ilya Shipitsin.
diff --git a/src/event/ngx_event_openssl_stapling.c b/src/event/ngx_event_openssl_stapling.c
--- a/src/event/ngx_event_openssl_stapling.c
+++ b/src/event/ngx_event_openssl_stapling.c
@@ -893,7 +893,9 @@ ngx_ssl_ocsp_validate(ngx_connection_t *
ocsp->cert_status = V_OCSP_CERTSTATUS_GOOD;
ocsp->conf = ocf;
-#if (OPENSSL_VERSION_NUMBER >= 0x10100000L && !defined LIBRESSL_VERSION_NUMBER)
+#if (OPENSSL_VERSION_NUMBER >= 0x10100000L \
+ && (!defined LIBRESSL_VERSION_NUMBER \
+ || LIBRESSL_VERSION_NUMBER >= 0x3050000fL))
ocsp->certs = SSL_get0_verified_chain(c->ssl->connection);
>>
>> ocsp->certs = SSL_get0_verified_chain(c->ssl->connection);
>>
>>
>> On the other hand, I don't like the resulting style mudness.
>> It may have sense just to drop old LibreSSL versions support:
>> maintaining one or two most recent stable branches should be enough.
>
> +1 on this.
>
> We certainly don't want to maintain support for ancient LibreSSL
> versions. IMO, just two branches is more than enough (and this is
> what I use in my testing, which usually means latest development
> release and latest stable release).
>
> At most, this probably can be three branches - which seems to
> match LibreSSL support policy,
> https://www.libressl.org/releases.html:
>
> : LibreSSL transitions to a new stable release branch every 6 months
> : in coordination with the OpenBSD development schedule. LibreSSL
> : stable branches are updated for 1 year after their corresponding
> : OpenBSD branch is tagged for release. See below for the current
> : stable release branches.
>
> In either case, LibreSSL versions below 3.5.0 are already not
> supported. If I understand correctly, right now the oldest
> supported branch is 3.7.x.
Agree. Also, Repology shows that modern popular distributions,
such as Alpine Linux and FreeBSD, have at least LibreSSL 3.5.x:
https://repology.org/project/libressl/versions
>
>> But anyway, I don't see an obvious win over the existing code:
>> the certificate chain is reconstructed if SSL_get0_verified_chain()
>> is (detected to be) not present, which should be fine in most cases.
>>
>> That said, it doesn't seem to deserve introducing 3-line macro test,
>> or (see OTOH note) breaking old LibreSSL support for no apparent reason.
>
> Reconstruction of the chain implies verification of signatures
> along the chain and can be costly. As such, it certainly would be
> better to use SSL_get0_verified_chain() as long as it is
> available.
Agree.
My point is that not using SSL_get0_verified_chain() should not result
in a broken functionality, as in the OCSP cert validation.
So, intention to start using it in LibreSSL may look an insufficient
argument per se to drop old LibreSSL versions.
Though, dropping them may be orthogonal to SSL_get0_verified_chain().
>
> Also, removing the "!defined LIBRESSL_VERSION_NUMBER" check might
> be seen as positive even without any additional benefits.
>
> Along with that, however, we might want to adjust the
> LIBRESSL_VERSION_NUMBER check in the ngx_event_openssl.h file, so
> OPENSSL_VERSION_NUMBER is set to a better value for old LibreSSL
> versions - for example, to only set OPENSSL_VERSION_NUMBER to
> 0x1010000fL for LibreSSL 3.5.0 or above.
Sounds like a plan if we are fine to drop older LibreSSL versions.
> This might allow to
> preserve limited compatibility with ancient LibreSSL versions
> without additional efforts (not tested though).
>
This won't build with any LibreSSL version in the [2.8.0, 3.5.0) range.
Particularly, SSL_CTX_sess_set_get_cb() has got a const argument in
LibreSSL 2.8.0, which is not backward compatible, see 7337:cab37803ebb3.
Another reason is SSL was made opaque by default in 3.4.x.
(Others seem not to affect building on older versions if pick up 3.5.0:
- X509_up_ref appeared in LibreSSL 2.6.0, X509 made opaque in 3.5.0;
- X509_get0_notAfter appeared in 2.7.0, X509_get_notAfter still there.)
Personally I'm fine to drop ancient LibreSSL versions, because it has
to happen someday and we don't want to maintain them eternally.
Alternative patch for your consideration:
# HG changeset patch
# User Sergey Kandaurov <pluknet@nginx.com>
# Date 1703011348 -14400
# Tue Dec 19 22:42:28 2023 +0400
# Node ID 94d4ef4a2316da66fea084952913ff2b0f84827d
# Parent 7a6d52990f2e2d88460a3dc6cc84aac89b7329ea
SSL: removed compatibility with LibreSSL < 3.5.0.
OPENSSL_VERSION_NUMBER is now redefined to 0x1010000fL for LibreSSL 3.5.0+.
As older versions starting from LibreSSL 2.8.0 won't build with a lesser
OPENSSL_VERSION_NUMBER value (see 7337:cab37803ebb3 for details), they are
now explicitly unsupported.
Besides that, this allows to start using SSL_get0_verified_chain()
with LibreSSL without additional macro tests.
diff --git a/src/event/ngx_event_openssl.h b/src/event/ngx_event_openssl.h
--- a/src/event/ngx_event_openssl.h
+++ b/src/event/ngx_event_openssl.h
@@ -45,10 +45,10 @@
#if (defined LIBRESSL_VERSION_NUMBER && OPENSSL_VERSION_NUMBER == 0x20000000L)
#undef OPENSSL_VERSION_NUMBER
-#if (LIBRESSL_VERSION_NUMBER >= 0x2080000fL)
+#if (LIBRESSL_VERSION_NUMBER >= 0x3050000fL)
#define OPENSSL_VERSION_NUMBER 0x1010000fL
#else
-#define OPENSSL_VERSION_NUMBER 0x1000107fL
+#error LibreSSL too old, need at least 3.5.0
#endif
#endif
diff --git a/src/event/ngx_event_openssl_stapling.c b/src/event/ngx_event_openssl_stapling.c
--- a/src/event/ngx_event_openssl_stapling.c
+++ b/src/event/ngx_event_openssl_stapling.c
@@ -893,7 +893,7 @@ ngx_ssl_ocsp_validate(ngx_connection_t *
ocsp->cert_status = V_OCSP_CERTSTATUS_GOOD;
ocsp->conf = ocf;
-#if (OPENSSL_VERSION_NUMBER >= 0x10100000L && !defined LIBRESSL_VERSION_NUMBER)
+#if OPENSSL_VERSION_NUMBER >= 0x10100000L
ocsp->certs = SSL_get0_verified_chain(c->ssl->connection);
--
Sergey Kandaurov
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
