Hello!
On Wed, May 03, 2023 at 07:14:35PM +0400, Sergey Kandaurov wrote:
> > On 3 May 2023, at 05:26, Maxim Dounin <mdou...@mdounin.ru> wrote:
> >
> > Hello!
> >
> > On Tue, May 02, 2023 at 05:49:23PM +0400, Sergey Kandaurov wrote:
> >
> >>
> >>> On 17 Apr 2023, at 07:31, Maxim Dounin <mdou...@mdounin.ru> wrote:
> >>>
> >>> # HG changeset patch
> >>> # User Maxim Dounin <mdou...@mdounin.ru>
> >>> # Date 1681702250 -10800
> >>> # Mon Apr 17 06:30:50 2023 +0300
> >>> # Node ID 6f0148ef1991d92a003c8529c8cce9a8dd49e706
> >>> # Parent a01b7d84f4355073a00f43760fc512e03b4452c3
> >>> Tests: removed unneeded require from proxy_ssl_keepalive.t.
> >>>
> >>> diff --git a/proxy_ssl_keepalive.t b/proxy_ssl_keepalive.t
> >>> --- a/proxy_ssl_keepalive.t
> >>> +++ b/proxy_ssl_keepalive.t
> >>> @@ -22,9 +22,6 @@ use Test::Nginx;
> >>> select STDERR; $| = 1;
> >>> select STDOUT; $| = 1;
> >>>
> >>> -eval { require IO::Socket::SSL; };
> >>> -plan(skip_all => 'IO::Socket::SSL not installed') if $@;
> >>> -
> >>> my $t = Test::Nginx->new()->has(qw/http http_ssl proxy
> >>> upstream_keepalive/)
> >>> ->has_daemon('openssl')->plan(3)
> >>> ->write_file_expand('nginx.conf', <<'EOF');
> >>
> >> We can as well remove it from h2_ssl_proxy_cache.t and h2_ssl_variables.t
> >> after 45c80276d691, HTTP2 package handles that for us.
> >> (Same approach used for the crypto layer of HTTP/3 tests.)
> >
> > Both h2_ssl_proxy_cache.t and h2_ssl_variables.t actually use
> > IO::Socket::SSL, even if the actual use is within
> > Test::Nginx::HTTP2, and will behave incorrectly if IO::Socket::SSL
> > is not available (as currently written, they won't fail, but will
> > skip individual tests with incorrect reasoning). As such, they do
> > need these requires (and the corresponding plan(skip_all) call if
> > IO::Socket::SSL is not available).
> >
>
> I tend to disagree about reasoning: it's most specific, not incorrect.
> Your mileage may vary.
>
> HTTP/2 implementation requires to be negotiated via ALPN
> (NPN was a fallback, before it was decommissioned in 1.21.4),
> otherwise it continues as if it were HTTP/1.1.
> So we have to check in tests that HTTP/2 was negotiated,
> and as such - we can provide most specific skip reasoning.
>
> General checking for IO::Socket:SSL, if not installed, provides
> generic skip reasoning, which in fact doesn't advise that installing
> an arbitrary (old enough) IO::Socket:SSL may not be enough:
>
> $ perl h2_ssl_variables.t
> 1..0 # SKIP IO::Socket::SSL not installed
>
> After removing require from h2_ssl_variables.t:
>
> $ perl h2_ssl_variables.t
> 1..10
> ok 1 # skip OpenSSL NPN support required
> ok 2 # skip OpenSSL ALPN support required
> ok 3 # skip OpenSSL NPN support required
> ok 4 # skip OpenSSL ALPN support required
> ok 5 # skip OpenSSL NPN support required
> ok 6 # skip OpenSSL ALPN support required
> ok 7 # skip OpenSSL NPN support required
> ok 8 # skip OpenSSL ALPN support required
> ok 9 - no alerts
> ok 10 - no sanitizer errors
There are two basic issues here as I see it:
- The reasoning suggests that there is no "OpenSSL NPN support"
and/or "OpenSSL ALPN support", while both are most likely
available on the particular machine. Further, trying to fix this skip
reason will always fail - because updating OpenSSL cannot help
here. I don't see how it can be "most specific" reason, it's
simply an incorrect one, hardly different from saying something
like "TCP/IP support required".
- This will break nicely as long as any of the tests is slightly
modified and won't do ALPN/NPN checks on each request - for
example, when checking SSL protocols or ciphers being negotiated.
Existing code with IO::Socket::SSL checks is perfectly correct,
and I see no reasons to remove them. Especially given that such
checks are trivial for the tests after the patch series in
question.
> This is somewhat similar to IO::Uncompress::Gunzip handling.
> The difference is that gunzip test is run/skipped right here,
> whereas IO::Socket::SSL check and skip are moved apart.
The difference is that gunzip tests are provide correct
skip reasoning if IO::Uncompress::Gunzip is not available, and
will never fail unexpectedly on any modifications since
IO::Uncompress::Gunzip require and skip are in the same function.
[...]
> BTW, it's probably time to remove NPN from tests.
No objections, but it's clearly outside of the scope of this patch
series.
> Removing NPN
> is a good reason to review the SSL flag of HTTP2::new_socket().
> It has received a limited use (cf. h2_ssl.t, h2_ssl_verify_client.t),
> it may have sense to just remove it.
I don't think it's a good idea. Rather, it should be extended to
be in par with functionality provided by http() and mail test modules
with this patch series, that is, make it possible to provide
additional SSL options. This will make it possible to remove
custom socket creation code from h2_ssl.t and
h2_ssl_verify_client.t as well.
--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
