Merge pull request #188 from clue-labs/handshake-address

Avoid possibility of missing remote address when TLS handshake fails

Author: jsor

src/SecureServer.php

@@ -169,28 +169,30 @@ public function handleConnection(ConnectionInterface $connection)
     {
         if (!$connection instanceof Connection) {
             $this->emit('error', array(new \UnexpectedValueException('Base server does not use internal Connection class exposing stream resource')));
-            $connection->end();
+            $connection->close();
             return;
         }
 
         foreach ($this->context as $name => $value) {
             \stream_context_set_option($connection->stream, 'ssl', $name, $value);
         }
 
+        // get remote address before starting TLS handshake in case connection closes during handshake
+        $remote = $connection->getRemoteAddress();
         $that = $this;
 
         $this->encryption->enable($connection)->then(
             function ($conn) use ($that) {
                 $that->emit('connection', array($conn));
             },
-            function ($error) use ($that, $connection) {
+            function ($error) use ($that, $connection, $remote) {
                 $error = new \RuntimeException(
-                    'Connection from ' . $connection->getRemoteAddress() . ' failed during TLS handshake: ' . $error->getMessage(),
+                    'Connection from ' . $remote . ' failed during TLS handshake: ' . $error->getMessage(),
                     $error->getCode()
                 );
 
                 $that->emit('error', array($error));
-                $connection->end();
+                $connection->close();
             }
         );
     }

tests/SecureServerTest.php

@@ -4,6 +4,7 @@
 
 use React\Socket\SecureServer;
 use React\Socket\TcpServer;
+use React\Promise\Promise;
 
 class SecureServerTest extends TestCase
 {
@@ -74,14 +75,14 @@ public function testCloseWillBePassedThroughToTcpServer()
         $server->close();
     }
 
-    public function testConnectionWillBeEndedWithErrorIfItIsNotAStream()
+    public function testConnectionWillBeClosedWithErrorIfItIsNotAStream()
     {
         $loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock();
 
         $tcp = new TcpServer(0, $loop);
 
         $connection = $this->getMockBuilder('React\Socket\ConnectionInterface')->getMock();
-        $connection->expects($this->once())->method('end');
+        $connection->expects($this->once())->method('close');
 
         $server = new SecureServer($tcp, $loop, array());
 
@@ -90,6 +91,74 @@ public function testConnectionWillBeEndedWithErrorIfItIsNotAStream()
         $tcp->emit('connection', array($connection));
     }
 
+    public function testConnectionWillTryToEnableEncryptionAndWaitForHandshake()
+    {
+        $loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock();
+
+        $tcp = new TcpServer(0, $loop);
+
+        $connection = $this->getMockBuilder('React\Socket\Connection')->disableOriginalConstructor()->getMock();
+        $connection->expects($this->once())->method('getRemoteAddress')->willReturn('tcp://127.0.0.1:1234');
+        $connection->expects($this->never())->method('close');
+
+        $server = new SecureServer($tcp, $loop, array());
+
+        $pending = new Promise(function () { });
+
+        $encryption = $this->getMockBuilder('React\Socket\StreamEncryption')->disableOriginalConstructor()->getMock();
+        $encryption->expects($this->once())->method('enable')->willReturn($pending);
+
+        $ref = new \ReflectionProperty($server, 'encryption');
+        $ref->setAccessible(true);
+        $ref->setValue($server, $encryption);
+
+        $ref = new \ReflectionProperty($server, 'context');
+        $ref->setAccessible(true);
+        $ref->setValue($server, array());
+
+        $server->on('error', $this->expectCallableNever());
+        $server->on('connection', $this->expectCallableNever());
+
+        $tcp->emit('connection', array($connection));
+    }
+
+    public function testConnectionWillBeClosedWithErrorIfEnablingEncryptionFails()
+    {
+        $loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock();
+
+        $tcp = new TcpServer(0, $loop);
+
+        $connection = $this->getMockBuilder('React\Socket\Connection')->disableOriginalConstructor()->getMock();
+        $connection->expects($this->once())->method('getRemoteAddress')->willReturn('tcp://127.0.0.1:1234');
+        $connection->expects($this->once())->method('close');
+
+        $server = new SecureServer($tcp, $loop, array());
+
+        $error = new \RuntimeException('Original');
+
+        $encryption = $this->getMockBuilder('React\Socket\StreamEncryption')->disableOriginalConstructor()->getMock();
+        $encryption->expects($this->once())->method('enable')->willReturn(\React\Promise\reject($error));
+
+        $ref = new \ReflectionProperty($server, 'encryption');
+        $ref->setAccessible(true);
+        $ref->setValue($server, $encryption);
+
+        $ref = new \ReflectionProperty($server, 'context');
+        $ref->setAccessible(true);
+        $ref->setValue($server, array());
+
+        $error = null;
+        $server->on('error', $this->expectCallableOnce());
+        $server->on('error', function ($e) use (&$error) {
+            $error = $e;
+        });
+
+        $tcp->emit('connection', array($connection));
+
+        $this->assertInstanceOf('RuntimeException', $error);
+        $this->assertEquals('Connection from tcp://127.0.0.1:1234 failed during TLS handshake: Original', $error->getMessage());
+    }
+
     public function testSocketErrorWillBeForwarded()
     {
         $loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock();