Close handle when closing the stream

[kensanata]

Summary

At the end of the connection, each side sends a close_notify alert to inform the peer that the connection is closed. IO::Socket::SSL does that, if its close method is called. This change to Mojo::IOLoop::Stream makes sure close() is called.

Motivation

I'm maintaining a Gemini server. The spec was recently updated to make the close_notify call at the end of a TLS connection mandatory:

As per RFCs 5246 and 8446, Gemini servers MUST send a TLS close_notify prior to closing the connection after sending a complete response. This is essential to disambiguate completed responses from responses closed prematurely due to network error or attack.

RFC 5246 section 7.2.1:

Unless some other fatal alert has been transmitted, each party is required to send a close_notify alert before closing the write side of the connection.

RFC 8446 section 6.1:

The client and the server must share knowledge that the connection is ending in order to avoid a truncation attack. … Each party MUST send a "close_notify" alert before closing its write side of the connection, unless it has already sent some error alert.

My server kept failing the test.

Here's a server using Mojo::IOLoop that fails the test:

use Mojo::IOLoop;
Mojo::IOLoop->server(
  {port => 1965, tls => 1} =>
  sub {
    my ($loop, $stream) = @_;
    $stream->on(
      read => sub {
	my ($stream, $bytes) = @_;
	# $stream->write("HTTP/1.1 200 OK\r\n\r\n");
	$stream->write("20 text/plain\r\n");
	$stream->write("Hello\n");
	$stream->close_gracefully();
      });
  });
Mojo::IOLoop->start unless Mojo::IOLoop->is_running;

Run test using gnutls-cli:

$ (echo; sleep 1) | gnutls-cli --insecure localhost:1965
...
20 text/plain
Hello
*** Fatal error: The TLS connection was non-properly terminated.
*** Server has terminated the connection abnormally.

Here is a IO::Socket::SSL server that passes the test:

use strict;
use IO::Socket::SSL;
my $srv = IO::Socket::SSL->new(
  LocalAddr => '0.0.0.0:1965',
  Listen => 10,
  SSL_cert_file => 'cert.pem',
  SSL_key_file => 'key.pem',
    );
while (1) {
  my $cl = $srv->accept or die "failed to accept or ssl handshake: $!,$SSL_ERROR";
  my $req = <$cl>;
  print $cl "20 text/plain\r\nHello\n";
  $cl->close;
}

Run test using gnutls-cli:

$ (echo; sleep 1) | gnutls-cli --insecure localhost:1965
...
20 text/plain
Hello
- Peer has closed the GnuTLS connection

The key is the close call at the end. If I remove it, the server still works, but the test for client_notify now fails.

The Mojo::IOLoop based server passes the test if I make the change I'm proposing to merge.

References

None

[fskale]

Your patch will prevent the stream->on_close event to be handled properly ! (IMHO) Also, you example server code it's missing both, the tls_cert and the tls_key option ! (IMHO)

[kensanata]

Your patch will prevent the stream->on_close event to be handled properly ! (IMHO)

I'm not sure what you mean. Are you suggesting I should change the order? The following also works for my purpose:

sub close {
  my $self = shift;
  return unless my $reactor = $self->reactor;
  return unless my $handle  = delete $self->timeout(0)->{handle};
  $reactor->remove($handle);
  $self->emit('close');
  $handle->close; # ✨ new ✨
}

I'll be happy to change it, if that's what you mean.

Also, you example server code it's missing both, the tls_cert and the tls_key option ! (IMHO)

As Mojolicious comes with built-in self-signed certificates, this is not a problem, and the Gemini protocol explicitly supports self-signed certificates. The code works as-is, as a Gemini server (not as a HTTPS server, of course). My actual project is Phoebe, where all of this is taken care of, of course.

[fskale]

I missed the API doc referencing the builtin cert. My fault ;-) Sörry. But, to be clear, the code emits "close", so you can deal with it in the server code, not in the API itself, or am i wrong… A snippet of my server code (HTTP check)…

.....
$stream->on(
        read => sub($stream) {
       .........
       $stream->close;
 .........
$stream->on(
        close => sub($stream) {
          $stream->stop;
          ......

Why would i stop the stream at the read event, when it can be done by using the emitted "close" event ?

[kensanata]

Does it work for you?

use Mojo::IOLoop;
Mojo::IOLoop->server(
  {port => 1965, tls => 1} =>
  sub {
    my ($loop, $stream) = @_;
    $stream->on(
      read => sub {
	my ($stream, $bytes) = @_;
	# $stream->write("HTTP/1.1 200 OK\r\n\r\n");
	$stream->write("20 text/plain\r\n");
	$stream->write("Hello\n");
	$stream->close_gracefully();
      });
    # ✨ new ✨
    $stream->on(
      close => sub {
	my ($stream) = @_;
	$stream->stop();
      });
  });
Mojo::IOLoop->start unless Mojo::IOLoop->is_running;

Test:

(echo; sleep 1) | gnutls-cli --insecure localhost:1965

Result:

20 text/plain
Hello
*** Fatal error: The TLS connection was non-properly terminated.
*** Server has terminated the connection abnormally.

I'm not sure how the reactor watch would end up calling close on the handle, though.

I tried an alternative, which also doesn't work:

use Mojo::IOLoop;
Mojo::IOLoop->server(
  {port => 1965, tls => 1} =>
  sub {
    my ($loop, $stream) = @_;
    $stream->on(
      read => sub {
	my ($stream, $bytes) = @_;
	# $stream->write("HTTP/1.1 200 OK\r\n\r\n");
	$stream->write("20 text/plain\r\n");
	$stream->write("Hello\n");
	$stream->close_gracefully();
      });
    # ✨ new ✨
    $stream->on(
      close => sub {
	my ($stream) = @_;
	$stream->handle->close();
      });
  });
Mojo::IOLoop->start unless Mojo::IOLoop->is_running;

Test:

(echo; sleep 1) | gnutls-cli --insecure localhost:1965

Result:

20 text/plain
Hello
*** Fatal error: The TLS connection was non-properly terminated.
*** Server has terminated the connection abnormally.

And on the server:

Mojo::Reactor::Poll: I/O watcher failed: Can't call method "close" on an undefined value at mojo-ioloop-server.pl line 18.

By the time the close call runs, the handle is already gone.

[kensanata]

Why would i stop the stream at the read event, when it can be done by using the emitted "close" event ?

As an aside, I think the default behaviour is simply wrong. If there is a different fix to the problem, then that's great. But I think the issue needs a fix because the RFC demands correct closing of TLS connections, and IO::Socket::SSL has the capability of doing that. You could argue that perhaps IO::Socket::SSL is the correct place to fix this issue? That I do not know.

[kraih]

Seems like something that could be tested with a unit test.

[shadowcat-mst]

It appears that IO::Socket::SSL explicitly stops the normal shutdown handshake happening if ->close gets called implicitly via DESTROY rather than directly via user code: https://metacpan.org/dist/IO-Socket-SSL/source/lib/IO/Socket/SSL.pm#L2123

I'm not sure whether that makes sense but it looks like to do graceful shutdown either IO::Socket::SSL needs to change or Mojo needs to call the close method itself.

... thinking about it this feels kinda like a DBI InactiveDestroy type thing except with an excessively big hammer applied to achieve the goal ... which, if so, probably means the current SSL.pm behaviour can't be changed without an extra flag but doesn't leave me any less sure what would be "right" here.

[kensanata]

use strict;
use Mojo::IOLoop;

Mojo::IOLoop->server(
  {port => 3000, tls => 1} =>
  sub {
    my ($loop, $stream) = @_;
    $stream->on(
      read => sub {
	my ($stream, $bytes) = @_;
	$stream->write("Hello\n");
	$stream->close_gracefully();
      });
  });

my $handle;

Mojo::IOLoop->client(
  {port => 3000, tls => 1, tls_options => { SSL_verify_mode => 0x00 }} =>
  sub {
    my ($loop, $err, $stream) = @_;
    $stream->on(
      read =>
      sub {
	my ($stream, $bytes) = @_;
	print "$bytes";
	# keep a reference the IO::Socket::SSL
	$handle = $stream->{handle};
      });
    $stream->on(
      close =>
      sub {
	my ($stream) = @_;
	print "Closed\n";
	print $handle . "\n";
	my $ssl = ${*$handle}{_SSL_object};
	print "$ssl\n";
	# Returns the shutdown mode of $ssl.
	#  to decode the return value (bitmask) use:
	#  0 - No shutdown setting, yet
	#  1 - SSL_SENT_SHUTDOWN
	#  2 - SSL_RECEIVED_SHUTDOWN
	# See <http://www.openssl.org/docs/ssl/SSL_set_shutdown.html>
	print "Shutdown: " . ((Net::SSLeay::get_shutdown($ssl) & 2) ? "yes" : "no") . "\n";
      });
    $stream->write("\r\n");
  });

Mojo::IOLoop->timer(3 => sub { Mojo::IOLoop->stop });
Mojo::IOLoop->start unless Mojo::IOLoop->is_running;

When I run it:

Hello
Closed
IO::Socket::SSL=GLOB(0x55962f7ac650)
94103552451056
Shutdown: no

When I close the handle before emitting the close event, this doesn't work, of course, so using this code in Stream.pm:

sub close {
  my $self = shift;
  return unless my $reactor = $self->reactor;
  return unless my $handle  = delete $self->timeout(0)->{handle};
  $reactor->remove($handle);
  $self->emit('close');
  $handle->close;
}

I get the following result:

Hello
Closed
IO::Socket::SSL=GLOB(0x55e997d3ff40)
94461763063344
Shutdown: yes

[kensanata]

I'm still interested in getting this merged.

[kraih]

I'm still interested in getting this merged.

The PR is still missing a unit test and needs to be rebased.

[kensanata]

I think I need help running the existing tests. I'm assuming this test should go into ioloop_tls.t. What am I missing in this:

perl Makefile.PL
make test
# Result: PASS
TEST_TLS=1 make test TEST_FILES=t/mojo/ioloop_tls.t

I'm getting: "Can't call method "write" on an undefined value at t/mojo/ioloop_tls.t line 98." (And then it hangs.)

Regenerating the certs in t/mojo/certs using the openssl commands provided in the ioloop_tls.t didn't help but I also didn't examine the certs.

[kensanata]

I can only run the beginning of the test, so bear with me.

I'm running:

TEST_TLS=1 make test TEST_FILES=t/mojo/ioloop_tls.t TEST_VERBOSE=1

Without the change to Mojo/IOLoop/Stream.pm:

ok 1 - right content
ok 2 - right content
not ok 3 - SSL received shutdown

#   Failed test 'SSL received shutdown'
#   at t/mojo/ioloop_tls.t line 96.
#          got: '0'
#     expected: '2'

With the change to Mojo/IOLoop/Stream.pm:

ok 1 - right content
ok 2 - right content
ok 3 - SSL received shutdown

make test still passes:

All tests successful.
Files=103, Tests=4266, 45 wallclock secs ( 1.67 usr  0.19 sys + 34.71 cusr  2.89 csys = 39.46 CPU)
Result: PASS