Mojo::IOLoop::TLS retries TLS handshake after it is already finished

[noxxi]

• Mojolicious version: 7.33
• Perl version: 5.22
• Operating system: Linux 4.4

Steps to reproduce the behavior

see noxxi/p5-io-socket-ssl#59

Expected behavior

TLS handshake attempt should stop after accept_SSL in IO::Socket::SSL returned success

Actual behavior

The handler responsible for the TLS handshake is not removed. This means when new data arrive (i.e. application data) the handler will be called again and will again call accept_SSL even though the handshake was already done. This will result in failure in accept_SSL.

[noxxi]

The following patch might fix the problem:

diff -ur Mojolicious-7.33/lib/Mojo/IOLoop/TLS.pm Mojolicious-7.33.fixed/lib/Mojo/IOLoop/TLS.pm
--- Mojolicious-7.33/lib/Mojo/IOLoop/TLS.pm     2017-03-05 23:05:14.000000000 +0100
+++ Mojolicious-7.33.fixed/lib/Mojo/IOLoop/TLS.pm       2017-06-12 08:11:07.658707214 +0200
@@ -82,8 +82,10 @@
 sub _tls {
   my ($self, $handle, $server) = @_;

-  return $self->emit(upgrade => delete $self->{handle})
-    if $server ? $handle->accept_SSL : $handle->connect_SSL;
+  if ($server ? $handle->accept_SSL : $handle->connect_SSL) {
+    $self->reactor->remove($handle);
+    return $self->emit(upgrade => delete $self->{handle});
+  }

   # Switch between reading and writing
   my $err = $IO::Socket::SSL::SSL_ERROR;

[kraih]

You make it sound like TLS should not work at all in Mojo::IOLoop, but so far we have not had a single bug report for our built-in web servers or the user agent. And all our unit tests seem to pass just fine too.

[noxxi]

I think the problem only happens if the upgrade callback does not remove or change the io handler, like in this example:

#!/usr/bin/perl
use Mojo::Base -strict;

use Mojo::IOLoop;
use Mojo::IOLoop::TLS;
use Mojo::IOLoop::Server;

my $server = Mojo::IOLoop::Server->new;

$server->on(accept => sub {
  warn "accepted";
  my $handle = pop;
  my $tls = Mojo::IOLoop::TLS->new($handle);
  $tls->on(upgrade => sub { warn "upgrade done: ".pop; });
  $tls->on(error => sub { warn "error: ".pop });
  $tls->negotiate(server => 1);
});

$server->listen(port => 8443);
$server->start;
Mojo::IOLoop->start;

If a TLS client connects to this server the handshake is successful. If the client then sends some data (i.e. TLS application data) the io handler will be triggered. If the application did not setup its own handler the one setup in negotiate will be used which causes Mojo::IOLoop::TLS::_tls to be called which again tries to call accept_SSL, even though the TLS handshake was already finished.

Since most code actually wants to deal with the data from the peer after the TLS handshake is done it will change the io handler away from the one setup in negotiate. In this case the problem will not happen, i.e. it will only happen if the original handler is left there unchanged. This explains why it works in all of your servers and tests.

[kraih]

Thank you for the explanation.