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

>
> 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/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel