From f0bd61998f303ecdd7a9cc73dbdce7ce8476c0e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20L=C3=BCck?= Date: Wed, 8 Feb 2017 11:23:18 +0100 Subject: [PATCH 1/2] Do not expose $master property Interfering with this (previously public) property could introduce some very subtle bugs, so it's best to simply avoid exposing it. Empirical evidence seems to suggest this isn't used much outside of this package anyway. Inside this package, we can safely replace all references to it with references to child sockets created upon connection. --- src/SecureServer.php | 15 ++++++--------- src/Server.php | 2 +- tests/FunctionalSecureServerTest.php | 13 ------------- tests/FunctionalServerTest.php | 15 +++++++++++++-- tests/SecureServerTest.php | 4 ---- 5 files changed, 20 insertions(+), 29 deletions(-) diff --git a/src/SecureServer.php b/src/SecureServer.php index 2b09f558..188a494b 100644 --- a/src/SecureServer.php +++ b/src/SecureServer.php @@ -56,6 +56,7 @@ class SecureServer extends EventEmitter implements ServerInterface { private $tcp; private $encryption; + private $context; /** * Creates a secure TLS server and starts waiting for incoming connections @@ -106,27 +107,19 @@ class SecureServer extends EventEmitter implements ServerInterface * @param Server $tcp * @param LoopInterface $loop * @param array $context - * @throws ConnectionException * @see Server * @link http://php.net/manual/en/context.ssl.php for TLS context options */ public function __construct(Server $tcp, LoopInterface $loop, array $context) { - if (!is_resource($tcp->master)) { - throw new ConnectionException('TCP server already shut down'); - } - // default to empty passphrase to surpress blocking passphrase prompt $context += array( 'passphrase' => '' ); - foreach ($context as $name => $value) { - stream_context_set_option($tcp->master, 'ssl', $name, $value); - } - $this->tcp = $tcp; $this->encryption = new StreamEncryption($loop); + $this->context = $context; $that = $this; $this->tcp->on('connection', function ($connection) use ($that) { @@ -156,6 +149,10 @@ public function handleConnection(ConnectionInterface $connection) return; } + foreach ($this->context as $name => $value) { + stream_context_set_option($connection->stream, 'ssl', $name, $value); + } + $that = $this; $this->encryption->enable($connection)->then( diff --git a/src/Server.php b/src/Server.php index 36a4eba4..8bb32cbe 100644 --- a/src/Server.php +++ b/src/Server.php @@ -36,7 +36,7 @@ */ class Server extends EventEmitter implements ServerInterface { - public $master; + private $master; private $loop; /** diff --git a/tests/FunctionalSecureServerTest.php b/tests/FunctionalSecureServerTest.php index f41e122c..c6b0afa1 100644 --- a/tests/FunctionalSecureServerTest.php +++ b/tests/FunctionalSecureServerTest.php @@ -368,19 +368,6 @@ public function testEmitsErrorIfConnectionIsNotSecureHandshake() Block\sleep(self::TIMEOUT, $loop); } - /** - * @expectedException React\Socket\ConnectionException - */ - public function testFailsToCreateSecureServerOnClosedSocket() - { - $loop = Factory::create(); - - $server = new Server(0, $loop); - $server->close(); - - new SecureServer($server, $loop, array()); - } - private function getPort(ServerInterface $server) { return parse_url($server->getAddress(), PHP_URL_PORT); diff --git a/tests/FunctionalServerTest.php b/tests/FunctionalServerTest.php index ee450500..680a9b0b 100644 --- a/tests/FunctionalServerTest.php +++ b/tests/FunctionalServerTest.php @@ -229,7 +229,7 @@ public function testEmitsConnectionWithLocalIpv6() $this->assertEquals($server->getAddress(), $local); } - public function testAppliesContextOptionsToSocketStreamResource() + public function testEmitsConnectionWithInheritedContextOptions() { if (defined('HHVM_VERSION') && version_compare(HHVM_VERSION, '3.13', '<')) { // https://3v4l.org/hB4Tc @@ -242,7 +242,18 @@ public function testAppliesContextOptionsToSocketStreamResource() 'backlog' => 4 )); - $all = stream_context_get_options($server->master); + $all = null; + $server->on('connection', function (ConnectionInterface $conn) use (&$all) { + $all = stream_context_get_options($conn->stream); + }); + $port = $this->getPort($server); + + $connector = new TcpConnector($loop); + $promise = $connector->create('127.0.0.1', $port); + + $promise->then($this->expectCallableOnce()); + + Block\sleep(0.1, $loop); $this->assertEquals(array('socket' => array('backlog' => 4)), $all); } diff --git a/tests/SecureServerTest.php b/tests/SecureServerTest.php index 9a47e3c9..f56061a5 100644 --- a/tests/SecureServerTest.php +++ b/tests/SecureServerTest.php @@ -17,7 +17,6 @@ public function testGetAddressWillBePassedThroughToTcpServer() { $tcp = $this->getMockBuilder('React\Socket\Server')->disableOriginalConstructor()->getMock(); $tcp->expects($this->once())->method('getAddress')->willReturn('127.0.0.1:1234'); - $tcp->master = stream_socket_server('tcp://localhost:0'); $loop = $this->getMock('React\EventLoop\LoopInterface'); @@ -30,7 +29,6 @@ public function testCloseWillBePassedThroughToTcpServer() { $tcp = $this->getMockBuilder('React\Socket\Server')->disableOriginalConstructor()->getMock(); $tcp->expects($this->once())->method('close'); - $tcp->master = stream_socket_server('tcp://localhost:0'); $loop = $this->getMock('React\EventLoop\LoopInterface'); @@ -42,7 +40,6 @@ public function testCloseWillBePassedThroughToTcpServer() public function testConnectionWillBeEndedWithErrorIfItIsNotAStream() { $tcp = $this->getMockBuilder('React\Socket\Server')->disableOriginalConstructor()->setMethods(null)->getMock(); - $tcp->master = stream_socket_server('tcp://localhost:0'); $loop = $this->getMock('React\EventLoop\LoopInterface'); @@ -59,7 +56,6 @@ public function testConnectionWillBeEndedWithErrorIfItIsNotAStream() public function testSocketErrorWillBeForwarded() { $tcp = $this->getMockBuilder('React\Socket\Server')->disableOriginalConstructor()->setMethods(null)->getMock(); - $tcp->master = stream_socket_server('tcp://localhost:0'); $loop = $this->getMock('React\EventLoop\LoopInterface'); From e4fb0f21bd1131cd59d7e3155c0ec9be28164179 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20L=C3=BCck?= Date: Wed, 8 Feb 2017 11:56:43 +0100 Subject: [PATCH 2/2] SecureServer supports any underlying ServerInterface (advanced) --- README.md | 21 +++++++++++++-------- src/SecureServer.php | 25 +++++++++++++++---------- tests/SecureServerTest.php | 4 ++-- 3 files changed, 30 insertions(+), 20 deletions(-) diff --git a/README.md b/README.md index 71a17eb8..6f474c99 100644 --- a/README.md +++ b/README.md @@ -295,14 +295,19 @@ Note that the `SecureServer` class is a concrete implementation for TLS sockets. If you want to typehint in your higher-level protocol implementation, you SHOULD use the generic [`ServerInterface`](#serverinterface) instead. -> Advanced usage: Internally, the `SecureServer` has to set the required -context options on the underlying stream resources. -It should therefor be used with an unmodified `Server` instance as first -parameter so that it can allocate an empty context resource which this -class uses to set required TLS context options. -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. +> Advanced usage: Despite allowing any `ServerInterface` as first parameter, +you SHOULD pass an unmodified `Server` instance as first parameter, unless you +know what you're doing. +Internally, the `SecureServer` has to set the required TLS context options on +the underlying stream resources. +These resources are not exposed through any of the interfaces defined in this +package, but only through the `React\Stream\Stream` class. +The unmodified `Server` class is guaranteed to emit connections that implement +the `ConnectionInterface` and also extend the `Stream` class in order to +expose these underlying resources. +If you use a custom `ServerInterface` and its `connection` event does not +meet this requirement, the `SecureServer` will emit an `error` event and +then close the underlying connection. ### ConnectionInterface diff --git a/src/SecureServer.php b/src/SecureServer.php index 188a494b..1e372db0 100644 --- a/src/SecureServer.php +++ b/src/SecureServer.php @@ -95,22 +95,27 @@ class SecureServer extends EventEmitter implements ServerInterface * and/or PHP version. * Passing unknown context options has no effect. * - * Advanced usage: Internally, the `SecureServer` has to set the required - * context options on the underlying stream resources. - * It should therefor be used with an unmodified `Server` instance as first - * parameter so that it can allocate an empty context resource which this - * class uses to set required TLS context options. - * 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. + * Advanced usage: Despite allowing any `ServerInterface` as first parameter, + * you SHOULD pass an unmodified `Server` instance as first parameter, unless you + * know what you're doing. + * Internally, the `SecureServer` has to set the required TLS context options on + * the underlying stream resources. + * These resources are not exposed through any of the interfaces defined in this + * package, but only through the `React\Stream\Stream` class. + * The unmodified `Server` class is guaranteed to emit connections that implement + * the `ConnectionInterface` and also extend the `Stream` class in order to + * expose these underlying resources. + * If you use a custom `ServerInterface` and its `connection` event does not + * meet this requirement, the `SecureServer` will emit an `error` event and + * then close the underlying connection. * - * @param Server $tcp + * @param ServerInterface|Server $tcp * @param LoopInterface $loop * @param array $context * @see Server * @link http://php.net/manual/en/context.ssl.php for TLS context options */ - public function __construct(Server $tcp, LoopInterface $loop, array $context) + public function __construct(ServerInterface $tcp, LoopInterface $loop, array $context) { // default to empty passphrase to surpress blocking passphrase prompt $context += array( diff --git a/tests/SecureServerTest.php b/tests/SecureServerTest.php index f56061a5..854ef852 100644 --- a/tests/SecureServerTest.php +++ b/tests/SecureServerTest.php @@ -15,7 +15,7 @@ public function setUp() public function testGetAddressWillBePassedThroughToTcpServer() { - $tcp = $this->getMockBuilder('React\Socket\Server')->disableOriginalConstructor()->getMock(); + $tcp = $this->getMockBuilder('React\Socket\ServerInterface')->getMock(); $tcp->expects($this->once())->method('getAddress')->willReturn('127.0.0.1:1234'); $loop = $this->getMock('React\EventLoop\LoopInterface'); @@ -27,7 +27,7 @@ public function testGetAddressWillBePassedThroughToTcpServer() public function testCloseWillBePassedThroughToTcpServer() { - $tcp = $this->getMockBuilder('React\Socket\Server')->disableOriginalConstructor()->getMock(); + $tcp = $this->getMockBuilder('React\Socket\ServerInterface')->getMock(); $tcp->expects($this->once())->method('close'); $loop = $this->getMock('React\EventLoop\LoopInterface');