Hello!

On Wed, May 03, 2023 at 08:20:02PM +0400, Sergey Kandaurov wrote:

> > On 17 Apr 2023, at 07:31, Maxim Dounin <mdounin@mdounin.ru> wrote:
> >
> > # HG changeset patch
> > # User Maxim Dounin <mdounin@mdounin.ru>
> > # Date 1681702252 -10800
> > # Mon Apr 17 06:30:52 2023 +0300
> > # Node ID f704912ed09f3494a815709710c3744b0adca50b
> > # Parent 6f0148ef1991d92a003c8529c8cce9a8dd49e706
> > Tests: added has_feature() tests for IO::Socket::SSL.
> >
> > The following distinct features supported:
> >
> > - "socket_ssl", which requires IO::Socket::SSL and also implies
> > existance of the IO::Socket::SSL::SSL_VERIFY_NONE() symbol.
> > It is used by most of the tests.
> >
>
> SSL_VERIFY_NONE was added in IO::Socket::SSL 1.31 (2009.09.25).
> The check was added primarily for then supported CentOS 5.
> Now CentOS 5 is long obsolete, SSL_VERIFY_NONE can be dropped.

As implemented, with has(socket_ssl) at least version 1.31 of
IO::Socket::SSL is required to run SSL tests. It still works
correctly with older version though, and just skips SSL tests, so
the test suite can be run on old platforms if needed.

> Most popular modern distributions have something of IO::Socket::SSL 2.0xx,
> the oldest still supported CentOS 7 has IO::Socket::SSL 1.94.
>
> > - "socket_ssl_sni", which requires IO::Socket::SSL with the can_client_sni()
> > function (1.84), and SNI support available in Net::SSLeay and the OpenSSL
> > library being used. Used by ssl_sni.t, ssl_sni_sessions.t,
> > stream_ssl_preread.t. Additional Net::SSLeay testing is believed to be
> > unneeded and was removed.
> >
>
> I agree that Net::SSLeay is believed to be redundant: properly implemented
> IO::Socket::SSL should detect Net::SSLeay is too old and behave accordingly.
>
> Note that you removed can_client_sni() from h2_ssl_verify_client.t.
> In fact, it is replaced with "socket_ssl_alpn". ALPN support was added
> to IO::Socket::SSL after SNI, so it should work in practice, although not
> evident. Formally, the "socket_ssl_sni" prerequisite needs to be added.

It was dropped intentionally, since ALPN support basically implies
SNI support. I don't think trying to catch clients with ALPN but
without SNI support worth the effort - there are no such clients.

> On the other way, can_client_sni() was added in 1.83, while SNI support
> factually was added noticeably earlier - in version 1.56, via SSL_hostname.
> Using IO::Socket::SSL 1.82 allowed me to run the following tests
> that would be otherwise skipped due to socket_ssl_sni:
>
> ssl_sni.t - replacing with "socket_ssl" is enough
> stream_ssl_preread.t - replacing with "socket_ssl" is enough
> ssl_sni_sessions.t - also needs SSL_VERIFY_NONE in get_ssl_socket()
> to stop whining on old IO::Socket::SSL that presumably doesn't get it
> from $ctx. It won't run anyway though due to IO::Socket::SSL version
> is too old to support TLSv1.3 sessions.
>
> If we tend to drop anything older than 1.94 (CentOS 7), then we can freely
> drop "socket_ssl_sni" from the patch. SSL_hostname is already there.

I'm fine with skipping tests with older versions, but failing
tests without clear reasoning looks wrong to me.

That is, we can probably merge "socket_ssl_sni" requirements into
"socket_ssl", but simply dropping them will result in failures on
systems with old IO::Socket::SSL versions and/or systems with old
OpenSSL. And this is something I would like to avoid - even if
that's not something tested on a regular basis.

Either way, current distinction with separate SNI tests looks good
enough for me, and I would rather keep it as is. Especially given
that it's basically effortless with this patch series.

[...]

> > - "socket_ssl_alpn", which requires IO::Socket::SSL with ALPN support (2.009),
> > and ALPN support in Net::SSLeay and the OpenSSL library being used.
> > Used by h2_ssl.t, h2_ssl_verify_client.t, stream_ssl_alpn.t,
> > stream_ssl_preread_alpn.t.
> >
> > - "socket_ssl_sslversion", which requires IO::Socket::SSL with
> > the get_sslversion() and get_sslversion_int() methods (1.964).
> > Used by mail_imap_ssl.t.
>
> I don't like that the whole mail_imap_ssl.t is skipped on < 1.964
> (even though it is not quite new and most distributions have it now),
> because of a minor optional feature that could not be tested.
> In principle, we can rewrite the test similar to $ssl_protocol
> to avoid dependency on 1.964:

In no particular order:

- It's a skip, not a failure.

- IO::Socket::SSL 1.964 is rather old, and if only an older
version of IO::Socket::SSL is available - this likely means that
SSL isn't a specific feature being considered.

- That's not only IMAP SSL test we have, and skipping it shouldn't
be a problem: we still test at least some IMAP SSL functionality
even on platforms with such outdated IO::Socket::SSL.

- We still need "socket_ssl_sslversion" for other tests, notably
mail_ssl_session_reuse.t and stream_ssl_session_reuse.t, see the
following patches.

Overall, I don't think it worth the effort. If you think it worth
it, consider submitting a patch once this patch series settles.

--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel