вт, 19 дек. 2023 г. в 09:58, Maxim Dounin <mdou...@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 <chipits...@gmail.com> wrote: > > > > > > # HG changeset patch > > > # User Ilya Shipitsin <chipits...@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