Hello! On Tue, Dec 19, 2023 at 10:44:00PM +0400, Sergey Kandaurov wrote:
> > > On 19 Dec 2023, at 12:58, Maxim Dounin <mdou...@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 <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)) > > > > Agree. For the sake of completeness: > > # HG changeset patch > # User Sergey Kandaurov <pluk...@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. The const argument can be easily ignored by using -Wno-error=incompatible-function-pointer-types (or just -Wno-error), which seems to be reasonable when trying to build things with ancient libraries. This makes it possible to build with LibreSSL 2.8.0-3.3.6 with minimal efforts. For LibreSSL 3.4.x, the dependency on the SSL internals can be easily eliminated by testing SSL_OP_NO_CLIENT_RENEGOTIATION, which anyway seems to be a reasonable change. > > (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 <pluk...@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 I'm certainly against the idea of explicitly rejecting old versions. As demonstrated above, even versions affected by various changes can be used with minimal efforts, such as disabling -Werror. For the record, here is a patch I tested with: diff --git a/src/event/ngx_event_openssl.c b/src/event/ngx_event_openssl.c --- a/src/event/ngx_event_openssl.c +++ b/src/event/ngx_event_openssl.c @@ -1105,7 +1105,8 @@ ngx_ssl_info_callback(const ngx_ssl_conn BIO *rbio, *wbio; ngx_connection_t *c; -#ifndef SSL_OP_NO_RENEGOTIATION +#if (!defined SSL_OP_NO_RENEGOTIATION \ + && !defined SSL_OP_NO_CLIENT_RENEGOTIATION) if ((where & SSL_CB_HANDSHAKE_START) && SSL_is_server((ngx_ssl_conn_t *) ssl_conn)) @@ -1838,9 +1839,10 @@ ngx_ssl_handshake(ngx_connection_t *c) c->read->ready = 1; c->write->ready = 1; -#ifndef SSL_OP_NO_RENEGOTIATION -#if OPENSSL_VERSION_NUMBER < 0x10100000L -#ifdef SSL3_FLAGS_NO_RENEGOTIATE_CIPHERS +#if (!defined SSL_OP_NO_RENEGOTIATION \ + && !defined SSL_OP_NO_CLIENT_RENEGOTIATION \ + && defined SSL3_FLAGS_NO_RENEGOTIATE_CIPHERS \ + && OPENSSL_VERSION_NUMBER < 0x10100000L) /* initial handshake done, disable renegotiation (CVE-2009-3555) */ if (c->ssl->connection->s3 && SSL_is_server(c->ssl->connection)) { @@ -1848,8 +1850,6 @@ ngx_ssl_handshake(ngx_connection_t *c) } #endif -#endif -#endif #if (defined BIO_get_ktls_send && !NGX_WIN32) @@ -2483,7 +2483,8 @@ ngx_ssl_handle_recv(ngx_connection_t *c, int sslerr; ngx_err_t err; -#ifndef SSL_OP_NO_RENEGOTIATION +#if (!defined SSL_OP_NO_RENEGOTIATION \ + && !defined SSL_OP_NO_CLIENT_RENEGOTIATION) if (c->ssl->renegotiation) { /* 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,7 +45,7 @@ #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 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); -- Maxim Dounin http://mdounin.ru/ _______________________________________________ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel