[PATCH] ssl: SSL_get0_verified_chain is available for LibreSSL >= 3.3.6

[Maxim Dounin]

Hello!

On Tue, Dec 19, 2023 at 02:09:10AM +0400, Sergey Kandaurov wrote:

> > On 24 Nov 2023, at 00:29, Ilya Shipitsin <chipitsine at gmail.com> wrote:
> > 
> > # HG changeset patch
> > # User Ilya Shipitsin <chipitsine at 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))

>  
>      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.

> 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.

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.  This might allow to 
preserve limited compatibility with ancient LibreSSL versions 
without additional efforts (not tested though).

-- 
Maxim Dounin
http://mdounin.ru/