> On 17 Apr 2023, at 07:31, Maxim Dounin <mdounin@mdounin.ru> wrote:
>
> # HG changeset patch
> # User Maxim Dounin <mdounin@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.

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.

>
> 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

> 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)

> 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.

[..]

--
Sergey Kandaurov
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel