[PATCH 10 of 11] Tests: reworked http SSL tests to use IO::Socket::SSL

Maxim Dounin mdounin at mdounin.ru
Thu May 18 15:16:25 UTC 2023


Hello!

On Thu, May 11, 2023 at 06:27:12PM +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 1681702264 -10800
> > #      Mon Apr 17 06:31:04 2023 +0300
> > # Node ID 2aaba5bbc0366bffe1f468105b1185cd48efbc93
> > # Parent  90913cb36b512c45cd9a171cbb4320b12ff24b48
> > Tests: reworked http SSL tests to use IO::Socket::SSL.
> > 
> > Relevant infrastructure is provided in Test::Nginx http() functions.
> > This also ensures that SSL handshake and various read and write operations
> > are guarded with timeouts.
> > 
> > The ssl_sni_reneg.t test uses IO::Socket::SSL::_get_ssl_object() to access
> > the Net::SSLeay object directly and trigger renegotation.  While
> > not exactly correct, this seems to be good enough for tests.
> > 
> > Similarly, IO::Socket::SSL::_get_ssl_object() is used in ssl_stapling.t,
> > since SSL_ocsp_staple_callback is called with the socket instead of the
> > Net::SSLeay object.
> > 
> > Similarly, IO::Socket::SSL::_get_ssl_object() is used in ssl_verify_client.t,
> > since there seems to be no way to obtain CA list with IO::Socket::SSL.
> 
> This is one of the reasons why it was written in Net::SSLeay.
> The intention was to avoid using undocumented _get_ssl_object.

Yep, these three tests required use of the undocumented (or, 
rather, explicitly documented to be an internal interface) 
_get_ssl_object().   Other tests, however, don't require use of 
any internal methods.

For these three tests, the possible options would be:

- Use _get_ssl_object() despite being an internal method.

- Preserve the existing Net::SSLeay-based custom code in these 
  tests, and make sure it properly handles SIGPIPE and other 
  errors (existing code seems to try, but fail at least in some 
  cases).

Given that _get_ssl_object() works fine in all know versions of 
IO::Socket::SSL, using _get_ssl_object() seems to be the best 
available option.

> The resulting code throughout the series is sometimes hard to read now.
> Still, if you believe it is better to rewrite it in IO::Socket:SSL,
> I'm ok with it.  You can treat this as a positive review.
> See minor comments below and in other patches.

I can't say it's harder to read than the existing code.  The net 
effect is:

 59 files changed, 1015 insertions(+), 1609 deletions(-)

and a lot more effort saved for fixing the custom code to properly 
handle SIGPIPE and other errors.

> > Notable change to http() request interface is that http_end() now closes
> > the socket.  This is to make sure that SSL connections are properly
> > closed and SSL sessions are not removed from the IO::Socket::SSL session
> > cache.  This affected access_log.t, which was modified accordingly.
> > 
> > diff --git a/access_log.t b/access_log.t
> > --- a/access_log.t
> > +++ b/access_log.t
> > @@ -161,11 +161,11 @@ http_get('/varlog?logname=0');
> > http_get('/varlog?logname=filename');
> > 
> > my $s = http('', start => 1);
> > -http_get('/addr', socket => $s);
> > my $addr = $s->sockhost();
> > my $port = $s->sockport();
> > my $saddr = $s->peerhost();
> > my $sport = $s->peerport();
> > +http_get('/addr', socket => $s);
> > 
> > http_get('/binary');
> > 
> > diff --git a/lib/Test/Nginx.pm b/lib/Test/Nginx.pm
> > --- a/lib/Test/Nginx.pm
> > +++ b/lib/Test/Nginx.pm
> > @@ -838,13 +838,15 @@ sub http($;%) {
> > 	my $s = http_start($request, %extra);
> > 
> > 	return $s if $extra{start} or !defined $s;
> > -	return http_end($s);
> > +	return http_end($s, %extra);
> > }
> > 
> > sub http_start($;%) {
> > 	my ($request, %extra) = @_;
> > 	my $s;
> > 
> > +	my $port = $extra{SSL} ? 8443 : 8080;
> > +
> > 	eval {
> > 		local $SIG{ALRM} = sub { die "timeout\n" };
> > 		local $SIG{PIPE} = sub { die "sigpipe\n" };
> > @@ -852,10 +854,25 @@ sub http_start($;%) {
> > 
> > 		$s = $extra{socket} || IO::Socket::INET->new(
> > 			Proto => 'tcp',
> > -			PeerAddr => '127.0.0.1:' . port(8080)
> > +			PeerAddr => '127.0.0.1:' . port($port),
> > +			%extra
> > 		)
> > 			or die "Can't connect to nginx: $!\n";
> > 
> > +		if ($extra{SSL}) {
> > +			require IO::Socket::SSL;
> > +			IO::Socket::SSL->start_SSL(
> > +				$s,
> > +				SSL_verify_mode =>
> > +					IO::Socket::SSL::SSL_VERIFY_NONE(),
> > +				%extra
> > +			)
> > +				or die $IO::Socket::SSL::SSL_ERROR . "\n";
> > +
> > +			log_in("ssl cipher: " . $s->get_cipher());
> > +			log_in("ssl cert: " . $s->peer_certificate('issuer'));
> > +		}
> > +
> > 		log_out($request);
> > 		$s->print($request);
> > 
> > @@ -879,7 +896,7 @@ sub http_start($;%) {
> > }
> > 
> > sub http_end($;%) {
> > -	my ($s) = @_;
> > +	my ($s, %extra) = @_;
> 
> extra doesn't seem to be used

Yep, thanks, removed.  It's a leftover from an earlier versions of 
the patch, which used to introduce $extra{noclose} to disable 
explicit closing of the socket (instead, access_log.t was 
modified).

> 
> > 	my $reply;
> > 
> > 	eval {
> > @@ -890,6 +907,8 @@ sub http_end($;%) {
> > 		local $/;
> > 		$reply = $s->getline();
> > 
> > +		$s->close();
> > +
> > 		alarm(0);
> > 	};
> > 	alarm(0);
> > diff --git a/ssl_certificate.t b/ssl_certificate.t
> > --- a/ssl_certificate.t
> > +++ b/ssl_certificate.t
> > @@ -17,29 +17,15 @@ use Socket qw/ CRLF /;
> > BEGIN { use FindBin; chdir($FindBin::Bin); }
> > 
> > use lib 'lib';
> > -use Test::Nginx;
> > +use Test::Nginx qw/ :DEFAULT http_end /;
> > 
> > ###############################################################################
> > 
> > select STDERR; $| = 1;
> > select STDOUT; $| = 1;
> > 
> > -eval {
> > -	require Net::SSLeay;
> > -	Net::SSLeay::load_error_strings();
> > -	Net::SSLeay::SSLeay_add_ssl_algorithms();
> > -	Net::SSLeay::randomize();
> > -};
> > -plan(skip_all => 'Net::SSLeay not installed') if $@;
> > -
> > -eval {
> > -	my $ctx = Net::SSLeay::CTX_new() or die;
> > -	my $ssl = Net::SSLeay::new($ctx) or die;
> > -	Net::SSLeay::set_tlsext_host_name($ssl, 'example.org') == 1 or die;
> > -};
> > -plan(skip_all => 'Net::SSLeay with OpenSSL SNI support required') if $@;
> > -
> > -my $t = Test::Nginx->new()->has(qw/http http_ssl geo openssl:1.0.2/)
> > +my $t = Test::Nginx->new()
> > +	->has(qw/http http_ssl geo openssl:1.0.2 socket_ssl_sni/)
> > 	->has_daemon('openssl');
> > 
> > $t->write_file_expand('nginx.conf', <<'EOF');
> > @@ -67,6 +53,7 @@ http {
> >     }
> > 
> >     add_header X-SSL $ssl_server_name:$ssl_session_reused;
> > +    add_header X-SSL-Protocol $ssl_protocol;
> >     ssl_session_cache shared:SSL:1m;
> >     ssl_session_tickets on;
> > 
> > @@ -177,60 +164,63 @@ like(get('password', 8083), qr/password/
> > 
> > # session reuse
> > 
> > -my ($s, $ssl) = get('default', 8080);
> > -my $ses = Net::SSLeay::get_session($ssl);
> > -
> > -like(get('default', 8080, $ses), qr/default:r/, 'session reused');
> > +my $s = session('default', 8080);
> > 
> > TODO: {
> > -# ticket key name mismatch prevents session resumption
> > +local $TODO = 'no TLSv1.3 sessions, old Net::SSLeay'
> > +	if $Net::SSLeay::VERSION < 1.88 && test_tls13();
> > +local $TODO = 'no TLSv1.3 sessions, old IO::Socket::SSL'
> > +	if $IO::Socket::SSL::VERSION < 2.061 && test_tls13();
> > +
> > +like(get('default', 8080, $s), qr/default:r/, 'session reused');
> > +
> > +TODO: {
> > +# automatic ticket ticket key name mismatch prevents session resumption
> 
> "ticket" repetition
> 
> (also a similar comment in stream_ssl_certificate.t isn't touched)

Thanks, overlooked this, restored the original comment.  (I tend 
to think the comment needs to be rewritten to properly clarify 
that session resumption in the test in question only works when 
both server{} blocks use the same session ticket keys, and 
therefore requires either ticket keys explicitly set or 
ssl_session_cache and nginx 1.23.2+, but it certainly shouldn't be 
in this patch.)

> > local $TODO = 'not yet' unless $t->has_version('1.23.2');
> > 
> > -like(get('default', 8081, $ses), qr/default:r/, 'session id context match');
> > +like(get('default', 8081, $s), qr/default:r/, 'session id context match');
> > 
> > }
> > +}
> > 
> > -like(get('default', 8082, $ses), qr/default:\./, 'session id context distinct');
> > +like(get('default', 8082, $s), qr/default:\./, 'session id context distinct');
> > 
> > # errors
> > 
> > -Net::SSLeay::ERR_clear_error();
> > -get_ssl_socket('nx', 8084);
> > -ok(Net::SSLeay::ERR_peek_error(), 'no certificate');
> > +ok(!get('nx', 8084), 'no certificate');
> 
> IIRC this was written so to ensure it is an SSL layer error
> and not some abrupt termination caused by unrelated reason.
> I don't object to simplify it though, if you think it is better.

Yes, understood.  It is not trivial to distinguish SSL and non-SSL 
errors here, and I tend to think it doesn't worth the effort, and 
just checking for an error is good enough.

Thanks for the review, pushed to http://mdounin.ru/hg/nginx-tests.

-- 
Maxim Dounin
http://mdounin.ru/


More information about the nginx-devel mailing list