Hello!
On Tue, Dec 19, 2023 at 10:44:00PM +0400, Sergey Kandaurov wrote:
>
> > On 19 Dec 2023, at 12:58, Maxim Dounin <mdounin@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 <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))
> >
>
> Agree. For the sake of completeness:
>
> # HG changeset patch
> # User Sergey Kandaurov <pluknet@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 <pluknet@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
