Hello!

On Thu, May 11, 2023 at 06:27:12PM +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 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/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel