<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">вт, 19 дек. 2023 г. в 09:58, Maxim Dounin <<a href="mailto:mdounin@mdounin.ru">mdounin@mdounin.ru</a>>:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hello!<br>
<br>
On Tue, Dec 19, 2023 at 02:09:10AM +0400, Sergey Kandaurov wrote:<br>
<br>
> > On 24 Nov 2023, at 00:29, Ilya Shipitsin <<a href="mailto:chipitsine@gmail.com" target="_blank">chipitsine@gmail.com</a>> wrote:<br>
> > <br>
> > # HG changeset patch<br>
> > # User Ilya Shipitsin <<a href="mailto:chipitsine@gmail.com" target="_blank">chipitsine@gmail.com</a>><br>
> > # Date 1700769135 -3600<br>
> > # Thu Nov 23 20:52:15 2023 +0100<br>
> > # Node ID 2001e73ce136d5bfc9bde27d338865b14b8ad436<br>
> > # Parent 7ec761f0365f418511e30b82e9adf80bc56681df<br>
> > ssl: SSL_get0_verified_chain is available for LibreSSL >= 3.3.6<br>
> <br>
> style: SSL prefix should be uppercase.<br>
> <br>
> > <br>
> > diff -r 7ec761f0365f -r 2001e73ce136 src/event/ngx_event_openssl_stapling.c<br>
> > --- a/src/event/ngx_event_openssl_stapling.c Thu Oct 26 23:35:09 2023 +0300<br>
> > +++ b/src/event/ngx_event_openssl_stapling.c Thu Nov 23 20:52:15 2023 +0100<br>
> > @@ -893,7 +893,8 @@<br>
> > ocsp->cert_status = V_OCSP_CERTSTATUS_GOOD;<br>
> > ocsp->conf = ocf;<br>
> > <br>
> > -#if (OPENSSL_VERSION_NUMBER >= 0x10100000L && !defined LIBRESSL_VERSION_NUMBER)<br>
> > +/* minimum OpenSSL 1.1.1 & LibreSSL 3.3.6 */<br>
> > +#if (OPENSSL_VERSION_NUMBER >= 0x10100000L && !defined LIBRESSL_VERSION_NUMBER) || (defined(LIBRESSL_VERSION_NUMBER) && (LIBRESSL_VERSION_NUMBER >= 0x3030600L))<br>
> > <br>
> > ocsp->certs = SSL_get0_verified_chain(c->ssl->connection);<br>
> > <br>
> <br>
> Testing "defined(LIBRESSL_VERSION_NUMBER)" is superfluous.<br>
> The macro test suffers from a very long line.<br>
> <br>
> The correct version test seems to be against LibreSSL 3.5.0, see<br>
> <a href="https://ftp.openbsd.org/pub/OpenBSD/LibreSSL/libressl-3.5.0-relnotes.txt" rel="noreferrer" target="_blank">https://ftp.openbsd.org/pub/OpenBSD/LibreSSL/libressl-3.5.0-relnotes.txt</a><br>
> <br>
> So, the resulting change would be as follows:<br>
> <br>
> diff --git a/src/event/ngx_event_openssl_stapling.c b/src/event/ngx_event_openssl_stapling.c<br>
> --- a/src/event/ngx_event_openssl_stapling.c<br>
> +++ b/src/event/ngx_event_openssl_stapling.c<br>
> @@ -893,7 +893,9 @@ ngx_ssl_ocsp_validate(ngx_connection_t *<br>
> ocsp->cert_status = V_OCSP_CERTSTATUS_GOOD;<br>
> ocsp->conf = ocf;<br>
> <br>
> -#if (OPENSSL_VERSION_NUMBER >= 0x10100000L && !defined LIBRESSL_VERSION_NUMBER)<br>
> +#if (OPENSSL_VERSION_NUMBER >= 0x10100000L \<br>
> + && !defined LIBRESSL_VERSION_NUMBER) \<br>
> + || LIBRESSL_VERSION_NUMBER >= 0x3050000fL<br>
<br>
Rather, <br>
<br>
+#if (OPENSSL_VERSION_NUMBER >= 0x10100000L \<br>
+ && (!defined LIBRESSL_VERSION_NUMBER \<br>
+ || LIBRESSL_VERSION_NUMBER >= 0x3050000fL))<br>
<br>
> <br>
> ocsp->certs = SSL_get0_verified_chain(c->ssl->connection);<br>
> <br>
> <br>
> On the other hand, I don't like the resulting style mudness.<br>
> It may have sense just to drop old LibreSSL versions support:<br>
> maintaining one or two most recent stable branches should be enough.<br>
<br>
+1 on this.<br></blockquote><div><br></div><div>if we want to keep code clean, we can move "if LibreSSL >= 3.7" to "configure" level<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
We certainly don't want to maintain support for ancient LibreSSL <br>
versions. IMO, just two branches is more than enough (and this is <br>
what I use in my testing, which usually means latest development <br>
release and latest stable release).<br>
<br>
At most, this probably can be three branches - which seems to <br>
match LibreSSL support policy, <br>
<a href="https://www.libressl.org/releases.html" rel="noreferrer" target="_blank">https://www.libressl.org/releases.html</a>:<br>
<br>
: LibreSSL transitions to a new stable release branch every 6 months <br>
: in coordination with the OpenBSD development schedule. LibreSSL <br>
: stable branches are updated for 1 year after their corresponding <br>
: OpenBSD branch is tagged for release. See below for the current <br>
: stable release branches.<br>
<br>
In either case, LibreSSL versions below 3.5.0 are already not <br>
supported. If I understand correctly, right now the oldest <br>
supported branch is 3.7.x.<br>
<br>
> But anyway, I don't see an obvious win over the existing code:<br>
> the certificate chain is reconstructed if SSL_get0_verified_chain()<br>
> is (detected to be) not present, which should be fine in most cases.<br>
> <br>
> That said, it doesn't seem to deserve introducing 3-line macro test,<br>
> or (see OTOH note) breaking old LibreSSL support for no apparent reason.<br>
<br>
Reconstruction of the chain implies verification of signatures <br>
along the chain and can be costly. As such, it certainly would be <br>
better to use SSL_get0_verified_chain() as long as it is <br>
available.<br>
<br>
Also, removing the "!defined LIBRESSL_VERSION_NUMBER" check might <br>
be seen as positive even without any additional benefits.<br>
<br>
Along with that, however, we might want to adjust the <br>
LIBRESSL_VERSION_NUMBER check in the ngx_event_openssl.h file, so <br>
OPENSSL_VERSION_NUMBER is set to a better value for old LibreSSL <br>
versions - for example, to only set OPENSSL_VERSION_NUMBER to <br>
0x1010000fL for LibreSSL 3.5.0 or above. This might allow to <br>
preserve limited compatibility with ancient LibreSSL versions <br>
without additional efforts (not tested though).<br>
<br>
-- <br>
Maxim Dounin<br>
<a href="http://mdounin.ru/" rel="noreferrer" target="_blank">http://mdounin.ru/</a><br>
_______________________________________________<br>
nginx-devel mailing list<br>
<a href="mailto:nginx-devel@nginx.org" target="_blank">nginx-devel@nginx.org</a><br>
<a href="https://mailman.nginx.org/mailman/listinfo/nginx-devel" rel="noreferrer" target="_blank">https://mailman.nginx.org/mailman/listinfo/nginx-devel</a><br>
</blockquote></div></div>