[PATCH 02 of 11] Tests: removed unneeded require from proxy_ssl_keepalive.t

[Maxim Dounin]

Hello!

On Wed, May 03, 2023 at 07:14:35PM +0400, Sergey Kandaurov wrote:

> > On 3 May 2023, at 05:26, Maxim Dounin <mdounin at 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 <mdounin at mdounin.ru> wrote:
> >>> 
> >>> # HG changeset patch
> >>> # User Maxim Dounin <mdounin at 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/