Welcome! Log In Create A New Profile

Advanced

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

Maxim Dounin
May 18, 2023 11:18AM
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
Subject Author Views Posted

[PATCH 00 of 11] SSL tests simplified

Maxim Dounin 564 April 16, 2023 11:46PM

[PATCH 01 of 11] Tests: SIGPIPE handling in mail tests

Maxim Dounin 95 April 16, 2023 11:46PM

[PATCH 03 of 11] Tests: added has_feature() tests for IO::Socket::SSL

Maxim Dounin 98 April 16, 2023 11:46PM

Re: [PATCH 03 of 11] Tests: added has_feature() tests for IO::Socket::SSL

Sergey Kandaurov 105 May 03, 2023 12:22PM

Re: [PATCH 03 of 11] Tests: added has_feature() tests for IO::Socket::SSL

Maxim Dounin 106 May 03, 2023 11:58PM

[PATCH 04 of 11] Tests: fixed server_tokens tests for build names with spaces

Maxim Dounin 99 April 16, 2023 11:46PM

Re: [PATCH 04 of 11] Tests: fixed server_tokens tests for build names with spaces

Sergey Kandaurov 73 May 11, 2023 07:50AM

Re: [PATCH 04 of 11] Tests: fixed server_tokens tests for build names with spaces

Maxim Dounin 87 May 14, 2023 02:54PM

[PATCH 05 of 11] Tests: added has_feature() test for SSL libraries

Maxim Dounin 94 April 16, 2023 11:46PM

[PATCH 09 of 11] Tests: simplified stream SSL tests with IO::Socket::SSL

Maxim Dounin 100 April 16, 2023 11:46PM

[PATCH 06 of 11] Tests: reworked mail SSL tests to use IO::Socket::SSL

Maxim Dounin 112 April 16, 2023 11:46PM

Re: [PATCH 06 of 11] Tests: reworked mail SSL tests to use IO::Socket::SSL

Sergey Kandaurov 99 May 11, 2023 10:40AM

Re: [PATCH 06 of 11] Tests: reworked mail SSL tests to use IO::Socket::SSL

Maxim Dounin 90 May 14, 2023 05:12PM

[PATCH 08 of 11] Tests: reworked stream SSL tests to use IO::Socket::SSL

Maxim Dounin 102 April 16, 2023 11:46PM

[PATCH 07 of 11] Tests: simplified mail_imap_ssl.t

Maxim Dounin 99 April 16, 2023 11:46PM

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

Maxim Dounin 99 April 16, 2023 11:46PM

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

Sergey Kandaurov 94 May 11, 2023 10:28AM

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

Maxim Dounin 115 May 18, 2023 11:18AM

[PATCH 11 of 11] Tests: simplified http SSL tests with IO::Socket::SSL

Maxim Dounin 101 April 16, 2023 11:46PM

[PATCH 0 of 6] SSL tests refactoring fixes

Sergey Kandaurov 79 May 22, 2023 03:58PM

[PATCH 1 of 6] Tests: unbreak ssl_stapling.t after IO::Socket::SSL refactoring

Sergey Kandaurov 90 May 22, 2023 03:58PM

Re: [PATCH 1 of 6] Tests: unbreak ssl_stapling.t after IO::Socket::SSL refactoring

Maxim Dounin 71 May 22, 2023 04:38PM

[PATCH 2 of 6] Tests: unbreak tests with IO::Socket:SSL lacking SSL_session_key

Sergey Kandaurov 78 May 22, 2023 03:58PM

Re: [PATCH 2 of 6] Tests: unbreak tests with IO::Socket:SSL lacking SSL_session_key

Maxim Dounin 69 May 22, 2023 07:44PM

Re: [PATCH 2 of 6] Tests: unbreak tests with IO::Socket:SSL lacking SSL_session_key

Sergey Kandaurov 90 May 23, 2023 06:36AM

Re: [PATCH 2 of 6] Tests: unbreak tests with IO::Socket:SSL lacking SSL_session_key

Maxim Dounin 136 May 23, 2023 09:32AM

[PATCH 3 of 6] Tests: unbreak stream_ssl_variables.t with old IO::Socket::SSL

Sergey Kandaurov 76 May 22, 2023 04:00PM

Re: [PATCH 3 of 6] Tests: unbreak stream_ssl_variables.t with old IO::Socket::SSL

Maxim Dounin 75 May 22, 2023 07:52PM

[PATCH 4 of 6] Tests: avoid specifying PSS in sigalgs unless in TLSv1.3

Sergey Kandaurov 77 May 22, 2023 04:00PM

Re: [PATCH 4 of 6] Tests: avoid specifying PSS in sigalgs unless in TLSv1.3

Maxim Dounin 73 May 22, 2023 09:08PM

Re: [PATCH 4 of 6] Tests: avoid specifying PSS in sigalgs unless in TLSv1.3

Sergey Kandaurov 91 May 23, 2023 08:24AM

[PATCH 5 of 6] Tests: added missing socket_ssl_alpn guard in mail_ssl.t

Sergey Kandaurov 83 May 22, 2023 04:00PM

Re: [PATCH 5 of 6] Tests: added missing socket_ssl_alpn guard in mail_ssl.t

Maxim Dounin 73 May 22, 2023 10:18PM

Re: [PATCH 5 of 6] Tests: added missing socket_ssl_alpn guard in mail_ssl.t

Sergey Kandaurov 130 May 23, 2023 08:56AM

[PATCH 6 of 6] Tests: added missing socket_ssl_reused prerequisites

Sergey Kandaurov 81 May 22, 2023 04:00PM

Re: [PATCH 6 of 6] Tests: added missing socket_ssl_reused prerequisites

Maxim Dounin 68 May 22, 2023 10:40PM



Sorry, you do not have permission to post/reply in this forum.

Online Users

Guests: 243
Record Number of Users: 8 on April 13, 2023
Record Number of Guests: 421 on December 02, 2018
Powered by nginx      Powered by FreeBSD      PHP Powered      Powered by MariaDB      ipv6 ready