Hello!

On Mon, Sep 05, 2022 at 10:44:06PM +0400, Sergey Kandaurov wrote:

> > On 1 Sep 2022, at 20:49, Maxim Dounin <mdounin@mdounin.ru> wrote:
> >
> > # HG changeset patch
> > # User Maxim Dounin <mdounin@mdounin.ru>
> > # Date 1662050858 -10800
> > # Thu Sep 01 19:47:38 2022 +0300
> > # Node ID d73286c43b44f3161ca4de1d9d1cbb070c6da4a7
> > # Parent 63a4b5ffd440c526bc96c6879dc1b6b489975d98
> > Win32: fixed build on Windows with OpenSSL 3.0.x (ticket #2379).
>
> BTW, win32 build on Windows XP with OpenSSL 3.0.x is currently broken
> for another reason: due to a missing InterlockedOr64 implementation.
> See the related fix, expected to appear in upcoming OpenSSL 3.0.6:
> ce3951fc30c7bc7c3dbacba19d87c79d9af9da0d

Well, that's exactly the reason why I've fixed 64-bit build with
MSVC 2010, the 2nd patch in this series. But see ticket's
comment:1, the patch committed into OpenSSL is incomplete and only
fixes MSVC 2008 and older, but won't fix Windows XP builds in newer
compilers.

> Now I have to configure OpenSSL with "no-threads" to pass to this error.

We actually use "no-threads" in Unix builds, see b329c0ab1a48. On
win32 we do use threads (for cache loader and cache manager), but
this shouldn't require threads support in OpenSSL. That is, it
might be a good idea to use "no-threads" in win32 builds too,
especially given that this was the default before OpenSSL 1.1.0.
Patch below.

> > SSL_sendfile() expects integer file descriptor as an argument, but nginx
> > uses OS file handles (HANDLE) to work with files on Windows, and passing
> > HANDLE instead of an integer correctly results in build failure. Since
> > SSL_sendfile() is not expected to work on Windows anyway, the code is now
> > disabled on Windows with appropriate compile-time checks.
> >
> > diff -r 63a4b5ffd440 -r d73286c43b44 src/event/ngx_event_openssl.c
> > --- a/src/event/ngx_event_openssl.c Thu Sep 01 19:45:22 2022 +0300
> > +++ b/src/event/ngx_event_openssl.c Thu Sep 01 19:47:38 2022 +0300
> > @@ -1770,7 +1770,7 @@ ngx_ssl_handshake(ngx_connection_t *c)
> > #endif
> > #endif
> >
> > -#ifdef BIO_get_ktls_send
> > +#if (defined BIO_get_ktls_send && !NGX_WIN32)
> >
> > if (BIO_get_ktls_send(SSL_get_wbio(c->ssl->connection)) == 1) {
> > ngx_log_debug0(NGX_LOG_DEBUG_EVENT, c->log, 0,
> > @@ -1915,7 +1915,7 @@ ngx_ssl_try_early_data(ngx_connection_t
> > c->read->ready = 1;
> > c->write->ready = 1;
> >
> > -#ifdef BIO_get_ktls_send
> > +#if (defined BIO_get_ktls_send && !NGX_WIN32)
> >
> > if (BIO_get_ktls_send(SSL_get_wbio(c->ssl->connection)) == 1) {
> > ngx_log_debug0(NGX_LOG_DEBUG_EVENT, c->log, 0,
> > @@ -2944,7 +2944,7 @@ ngx_ssl_write_early(ngx_connection_t *c,
> > static ssize_t
> > ngx_ssl_sendfile(ngx_connection_t *c, ngx_buf_t *file, size_t size)
> > {
> > -#ifdef BIO_get_ktls_send
> > +#if (defined BIO_get_ktls_send && !NGX_WIN32)
> >
> > int sslerr, flags;
> > ssize_t n;
> >
>
> This could be simplified if replaced #ifdef with #if.
> BIO_get_ktls_send is documented to be a macro (and so tested here).
> When OpenSSL isn't configured with KTLS, the macro is explanded to 0.
> Replacement allows optimize ngx_ssl_sendfile() at compile time, as well.

While BIO_get_ktls_send is documented to be a macro, it's not
required to expand to "(0)" if KTLS is not supported.

Further, it's not going to actually work for positive tests, since
non-numeric macro values will evaluate to 0, effectively disabling
SSL_sendfile() for all builds.

> I see that it's convention in nginx to test external macros using #ifdef.
> In certain cases we use an exception there if it does or even does not
> make sense, such as when testing SSL_CTRL_SET_ECDH_AUTO (though that's
> rather a typo there). Using #if BIO_get_ktls_send looks reasonable to me.

Sure, "#if SSL_CTRL_SET_ECDH_AUTO" looks like a typo (patch
below), but it used to work, since SSL_CTRL_SET_ECDH_AUTO is a
non-zero numeric constant when defined. This is not the case with
BIO_get_ktls_send.

> Another way (though, a less obvious for the reader) is to replace
> #if/ifdef BIO_get_ktls_send with a more convenient #ifndef OPENSSL_NO_KTLS.
> This macro is set when KTLS isn't supported and not configured for OpenSSL.
> As per INSTALL.md in the root of OpenSSL distribution, the enable-ktls option
> "is forced off on systems that do not support the Kernel TLS data-path".
> This makes no matter how OpenSSL is configured, with or without this option,
> if it's claimed in OpenSSL to be unsupported by platform.
> I tested to configure enable-ktls on win32: that's appeared to be true.
> Unfortunately, OPENSSL_NO_KTLS is used to be documented (even for runtime
> BIO_get_ktls_send() checks) only in sources, such as in apps/s_server.c.

This is not going to work for older OpenSSL versions without KTLS
support. At most, you can replace the !NGX_WIN32 part with
!defined OPENSSL_NO_KTLS, but I don't think it is a good change
for this win32-specific issue.

# HG changeset patch
# User Maxim Dounin <mdounin@mdounin.ru>
# Date 1662432499 -10800
# Tue Sep 06 05:48:19 2022 +0300
# Node ID de9fe82b0c7789474bb6284f13ebd3f903b129b0
# Parent 9cf231508a8dbc2492e0d1cd06d7b1258eb5f435
SSL: fixed incorrect usage of #if instead of #ifdef.

In 2014ed60f17f, "#if SSL_CTRL_SET_ECDH_AUTO" test was incorrectly used
instead of "#ifdef SSL_CTRL_SET_ECDH_AUTO". There is no practical
difference, since SSL_CTRL_SET_ECDH_AUTO evaluates to a non-zero numeric
value when defined, but anyway it's better to correctly test if the value
is defined.

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
@@ -1426,7 +1426,7 @@ ngx_ssl_ecdh_curve(ngx_conf_t *cf, ngx_s

SSL_CTX_set_options(ssl->ctx, SSL_OP_SINGLE_ECDH_USE);

-#if SSL_CTRL_SET_ECDH_AUTO
+#ifdef SSL_CTRL_SET_ECDH_AUTO
/* not needed in OpenSSL 1.1.0+ */
SSL_CTX_set_ecdh_auto(ssl->ctx, 1);
#endif

# HG changeset patch
# User Maxim Dounin <mdounin@mdounin.ru>
# Date 1662434808 -10800
# Tue Sep 06 06:26:48 2022 +0300
# Node ID 5e493208769dd754253f567405c05b3cf5f05c8a
# Parent 2642d97294f0e09c1fd67e434aae759768805617
Win32: disabled threads support in OpenSSL builds.

Threads are disabled during UNIX builds (see b329c0ab1a48), and also not
needed for Windows builds.

This used to be the default before OpenSSL 1.1.0.

diff -r 2642d97294f0 -r 5e493208769d auto/lib/openssl/makefile.msvc
--- a/auto/lib/openssl/makefile.msvc Tue Sep 06 05:48:19 2022 +0300
+++ b/auto/lib/openssl/makefile.msvc Tue Sep 06 06:26:48 2022 +0300
@@ -6,7 +6,7 @@
all:
cd $(OPENSSL)

- perl Configure VC-WIN32 no-shared \
+ perl Configure VC-WIN32 no-shared no-threads \
--prefix="%cd%/openssl" \
--openssldir="%cd%/openssl/ssl" \
$(OPENSSL_OPT)


--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org