Merge pull request reactphp#90 from clue-labs/sni

Pass through original host to underlying TcpConnector for TLS setup (fixes SNI on legacy PHP < 5.6)
clue-labs · Mar 14, 2017 · 773417f · 773417f
2 parents a141bb1 + f797b6a
commit 773417f
Show file tree

Hide file tree

Showing 8 changed files with 132 additions and 30 deletions.
diff --git a/README.md b/README.md
@@ -239,6 +239,13 @@ the remote host rejects the connection etc.), it will reject with a
 
 If you want to connect to hostname-port-combinations, see also the following chapter.
 
+> Advanced usage: Internally, the `TcpConnector` allocates an empty *context*
+resource for each stream resource.
+If the destination URI contains a `hostname` query parameter, its value will
+be used to set up the TLS peer name.
+This is used by the `SecureConnector` and `DnsConnector` to verify the peer
+name and can also be used if you want a custom TLS peer name.
+
 ### DNS resolution
 
 The `DnsConnector` class implements the
@@ -288,6 +295,17 @@ $connector = new React\SocketClient\Connector($loop, $dns);
 $connector->connect('www.google.com:80')->then($callback);
 ```
 
+> Advanced usage: Internally, the `DnsConnector` relies on a `Resolver` to
+look up the IP address for the given hostname.
+It will then replace the hostname in the destination URI with this IP and
+append a `hostname` query parameter and pass this updated URI to the underlying
+connector.
+The underlying connector is thus responsible for creating a connection to the
+target IP address, while this query parameter can be used to check the original
+hostname and is used by the `TcpConnector` to set up the TLS peer name.
+If a `hostname` is given explicitly, this query parameter will not be modified,
+which can be useful if you want a custom TLS peer name.
+
 ### Secure TLS connections
 
 The `SecureConnector` class implements the
@@ -333,13 +351,14 @@ $secureConnector = new React\SocketClient\SecureConnector($dnsConnector, $loop,
 ));
 ```
 
-> Advanced usage: Internally, the `SecureConnector` has to set the required
-*context options* on the underlying stream resource.
+> Advanced usage: Internally, the `SecureConnector` relies on setting up the
+required *context options* on the underlying stream resource.
 It should therefor be used with a `TcpConnector` somewhere in the connector
 stack so that it can allocate an empty *context* resource for each stream
-resource.
-Failing to do so may result in some hard to trace race conditions, because all
-stream resources will use a single, shared *default context* resource otherwise.
+resource and verify the peer name.
+Failing to do so may result in a TLS peer name mismatch error or some hard to
+trace race conditions, because all stream resources will use a single, shared
+*default context* resource otherwise.
 
 ### Connection timeouts
 

diff --git a/examples/01-http.php b/examples/01-http.php
@@ -19,15 +19,17 @@
 // time out connection attempt in 3.0s
 $dns = new TimeoutConnector($dns, 3.0, $loop);
 
-$dns->connect('www.google.com:80')->then(function (ConnectionInterface $connection) {
+$target = isset($argv[1]) ? $argv[1] : 'www.google.com:80';
+
+$dns->connect($target)->then(function (ConnectionInterface $connection) use ($target) {
     $connection->on('data', function ($data) {
         echo $data;
     });
     $connection->on('close', function () {
         echo '[CLOSED]' . PHP_EOL;
     });
 
-    $connection->write("GET / HTTP/1.0\r\nHost: www.google.com\r\n\r\n");
+    $connection->write("GET / HTTP/1.0\r\nHost: $target\r\n\r\n");
 }, 'printf');
 
 $loop->run();
diff --git a/examples/02-https.php b/examples/02-https.php
@@ -21,15 +21,17 @@
 // time out connection attempt in 3.0s
 $tls = new TimeoutConnector($tls, 3.0, $loop);
 
-$tls->connect('www.google.com:443')->then(function (ConnectionInterface $connection) {
+$target = isset($argv[1]) ? $argv[1] : 'www.google.com:443';
+
+$tls->connect($target)->then(function (ConnectionInterface $connection) use ($target) {
     $connection->on('data', function ($data) {
         echo $data;
     });
     $connection->on('close', function () {
         echo '[CLOSED]' . PHP_EOL;
     });
 
-    $connection->write("GET / HTTP/1.0\r\nHost: www.google.com\r\n\r\n");
+    $connection->write("GET / HTTP/1.0\r\nHost: $target\r\n\r\n");
 }, 'printf');
 
 $loop->run();
diff --git a/src/DnsConnector.php b/src/DnsConnector.php
@@ -30,13 +30,12 @@ public function connect($uri)
             return Promise\reject(new \InvalidArgumentException('Given URI "' . $uri . '" is invalid'));
         }
 
-        $that = $this;
         $host = trim($parts['host'], '[]');
         $connector = $this->connector;
 
         return $this
             ->resolveHostname($host)
-            ->then(function ($ip) use ($connector, $parts) {
+            ->then(function ($ip) use ($connector, $host, $parts) {
                 $uri = '';
 
                 // prepend original scheme if known
@@ -66,6 +65,14 @@ public function connect($uri)
                     $uri .= '?' . $parts['query'];
                 }
 
+                // append original hostname as query if resolved via DNS and if
+                // destination URI does not contain "hostname" query param already
+                $args = array();
+                parse_str(isset($parts['query']) ? $parts['query'] : '', $args);
+                if ($host !== $ip && !isset($args['hostname'])) {
+                    $uri .= (isset($parts['query']) ? '&' : '?') . 'hostname=' . rawurlencode($host);
+                }
+
                 // append original fragment if known
                 if (isset($parts['fragment'])) {
                     $uri .= '#' . $parts['fragment'];

diff --git a/src/SecureConnector.php b/src/SecureConnector.php
@@ -30,25 +30,12 @@ public function connect($uri)
         }
 
         $parts = parse_url($uri);
-        if (!$parts || !isset($parts['host']) || $parts['scheme'] !== 'tls') {
+        if (!$parts || !isset($parts['scheme']) || $parts['scheme'] !== 'tls') {
             return Promise\reject(new \InvalidArgumentException('Given URI "' . $uri . '" is invalid'));
         }
 
         $uri = str_replace('tls://', '', $uri);
-        $host = trim($parts['host'], '[]');
-
-        $context = $this->context + array(
-            'SNI_enabled' => true,
-            'peer_name' => $host
-        );
-
-        // legacy PHP < 5.6 ignores peer_name and requires legacy context options instead
-        if (PHP_VERSION_ID < 50600) {
-            $context += array(
-                'SNI_server_name' => $host,
-                'CN_match' => $host
-            );
-        }
+        $context = $this->context;
 
         $encryption = $this->streamEncryption;
         return $this->connector->connect($uri)->then(function (ConnectionInterface $connection) use ($context, $encryption) {

diff --git a/src/TcpConnector.php b/src/TcpConnector.php
@@ -33,13 +33,52 @@ public function connect($uri)
             return Promise\reject(new \InvalidArgumentException('Given URI "' . $ip . '" does not contain a valid host IP'));
         }
 
+        // use context given in constructor
+        $context = array(
+            'socket' => $this->context
+        );
+
+        // parse arguments from query component of URI
+        $args = array();
+        if (isset($parts['query'])) {
+            parse_str($parts['query'], $args);
+        }
+
+        // If an original hostname has been given, use this for TLS setup.
+        // This can happen due to layers of nested connectors, such as a
+        // DnsConnector reporting its original hostname.
+        // These context options are here in case TLS is enabled later on this stream.
+        // If TLS is not enabled later, this doesn't hurt either.
+        if (isset($args['hostname'])) {
+            $context['ssl'] = array(
+                'SNI_enabled' => true,
+                'peer_name' => $args['hostname']
+            );
+
+            // Legacy PHP < 5.6 ignores peer_name and requires legacy context options instead.
+            // The SNI_server_name context option has to be set here during construction,
+            // as legacy PHP ignores any values set later.
+            if (PHP_VERSION_ID < 50600) {
+                $context['ssl'] += array(
+                    'SNI_server_name' => $args['hostname'],
+                    'CN_match' => $args['hostname']
+                );
+            }
+        }
+
+        // HHVM fails to parse URIs with a query but no path, so let's add a dummy path
+        // See also https://3v4l.org/jEhLF
+        if (defined('HHVM_VERSION') && isset($parts['query']) && !isset($parts['path'])) {
+            $uri = str_replace('?', '/?', $uri);
+        }
+
         $socket = @stream_socket_client(
             $uri,
             $errno,
             $errstr,
             0,
             STREAM_CLIENT_CONNECT | STREAM_CLIENT_ASYNC_CONNECT,
-            stream_context_create(array('socket' => $this->context))
+            stream_context_create($context)
         );
 
         if (false === $socket) {

diff --git a/tests/DnsConnectorTest.php b/tests/DnsConnectorTest.php
@@ -30,15 +30,15 @@ public function testPassByResolverIfGivenIp()
     public function testPassThroughResolverIfGivenHost()
     {
         $this->resolver->expects($this->once())->method('resolve')->with($this->equalTo('google.com'))->will($this->returnValue(Promise\resolve('1.2.3.4')));
-        $this->tcp->expects($this->once())->method('connect')->with($this->equalTo('1.2.3.4:80'))->will($this->returnValue(Promise\reject()));
+        $this->tcp->expects($this->once())->method('connect')->with($this->equalTo('1.2.3.4:80?hostname=google.com'))->will($this->returnValue(Promise\reject()));
 
         $this->connector->connect('google.com:80');
     }
 
     public function testPassThroughResolverIfGivenHostWhichResolvesToIpv6()
     {
         $this->resolver->expects($this->once())->method('resolve')->with($this->equalTo('google.com'))->will($this->returnValue(Promise\resolve('::1')));
-        $this->tcp->expects($this->once())->method('connect')->with($this->equalTo('[::1]:80'))->will($this->returnValue(Promise\reject()));
+        $this->tcp->expects($this->once())->method('connect')->with($this->equalTo('[::1]:80?hostname=google.com'))->will($this->returnValue(Promise\reject()));
 
         $this->connector->connect('google.com:80');
     }
@@ -51,6 +51,22 @@ public function testPassByResolverIfGivenCompleteUri()
         $this->connector->connect('scheme://127.0.0.1:80/path?query#fragment');
     }
 
+    public function testPassThroughResolverIfGivenCompleteUri()
+    {
+        $this->resolver->expects($this->once())->method('resolve')->with($this->equalTo('google.com'))->will($this->returnValue(Promise\resolve('1.2.3.4')));
+        $this->tcp->expects($this->once())->method('connect')->with($this->equalTo('scheme://1.2.3.4:80/path?query&hostname=google.com#fragment'))->will($this->returnValue(Promise\reject()));
+
+        $this->connector->connect('scheme://google.com:80/path?query#fragment');
+    }
+
+    public function testPassThroughResolverIfGivenExplicitHost()
+    {
+        $this->resolver->expects($this->once())->method('resolve')->with($this->equalTo('google.com'))->will($this->returnValue(Promise\resolve('1.2.3.4')));
+        $this->tcp->expects($this->once())->method('connect')->with($this->equalTo('scheme://1.2.3.4:80/?hostname=google.de'))->will($this->returnValue(Promise\reject()));
+
+        $this->connector->connect('scheme://google.com:80/?hostname=google.de');
+    }
+
     public function testRejectsImmediatelyIfUriIsInvalid()
     {
         $this->resolver->expects($this->never())->method('resolve');
@@ -85,7 +101,7 @@ public function testCancelDuringTcpConnectionCancelsTcpConnection()
     {
         $pending = new Promise\Promise(function () { }, function () { throw new \Exception(); });
         $this->resolver->expects($this->once())->method('resolve')->with($this->equalTo('example.com'))->will($this->returnValue(Promise\resolve('1.2.3.4')));
-        $this->tcp->expects($this->once())->method('connect')->with($this->equalTo('1.2.3.4:80'))->will($this->returnValue($pending));
+        $this->tcp->expects($this->once())->method('connect')->with($this->equalTo('1.2.3.4:80?hostname=example.com'))->will($this->returnValue($pending));
 
         $promise = $this->connector->connect('example.com:80');
         $promise->cancel();

diff --git a/tests/IntegrationTest.php b/tests/IntegrationTest.php
@@ -10,6 +10,7 @@
 use React\SocketClient\TcpConnector;
 use React\Stream\BufferedSink;
 use Clue\React\Block;
+use React\SocketClient\DnsConnector;
 
 class IntegrationTest extends TestCase
 {
@@ -62,6 +63,35 @@ public function gettingEncryptedStuffFromGoogleShouldWork()
         $this->assertRegExp('#^HTTP/1\.0#', $response);
     }
 
+    /** @test */
+    public function gettingEncryptedStuffFromGoogleShouldWorkIfHostIsResolvedFirst()
+    {
+        if (!function_exists('stream_socket_enable_crypto')) {
+            $this->markTestSkipped('Not supported on your platform (outdated HHVM?)');
+        }
+
+        $loop = new StreamSelectLoop();
+
+        $factory = new Factory();
+        $dns = $factory->create('8.8.8.8', $loop);
+
+        $connector = new DnsConnector(
+            new SecureConnector(
+                new TcpConnector($loop),
+                $loop
+            ),
+            $dns
+        );
+
+        $conn = Block\await($connector->connect('google.com:443'), $loop);
+
+        $conn->write("GET / HTTP/1.0\r\n\r\n");
+
+        $response = Block\await(BufferedSink::createPromise($conn), $loop, self::TIMEOUT);
+
+        $this->assertRegExp('#^HTTP/1\.0#', $response);
+    }
+
     /** @test */
     public function testSelfSignedRejectsIfVerificationIsEnabled()
     {