вт, 19 дек. 2023 г. в 09:58, Maxim Dounin <mdounin@mdounin.ru>:

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

if we want to keep code clean, we can move "if LibreSSL >= 3.7" to
"configure" level


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