Skip to content

Commit 212206e

Browse files
WyriHaximusWyriHaximus
authored
Merge pull request #177 from clue-labs/tls-cancellation
Improve TLS error messages and cancellation forwarding during TLS handshake after TCP/IP connection
2 parents 12d266a + 3abb49d commit 212206e

File tree

6 files changed

+232
-15
lines changed

6 files changed

+232
-15
lines changed

‎src/SecureConnector.php‎

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -40,8 +40,10 @@ public function connect($uri)
4040
$context = $this->context;
4141

4242
$encryption = $this->streamEncryption;
43-
return $this->connector->connect($uri)->then(function (ConnectionInterface $connection) use ($context, $encryption) {
43+
$connected = false;
44+
$promise = $this->connector->connect($uri)->then(function (ConnectionInterface $connection) use ($context, $encryption, $uri, &$promise, &$connected) {
4445
// (unencrypted) TCP/IP connection succeeded
46+
$connected = true;
4547

4648
if (!$connection instanceof Connection) {
4749
$connection->close();
@@ -54,11 +56,29 @@ public function connect($uri)
5456
}
5557

5658
// try to enable encryption
57-
return $encryption->enable($connection)->then(null, function ($error) use ($connection) {
59+
return $promise = $encryption->enable($connection)->then(null, function ($error) use ($connection, $uri) {
5860
// establishing encryption failed => close invalid connection and return error
5961
$connection->close();
60-
throw $error;
62+
63+
throw new \RuntimeException(
64+
'Connection to ' . $uri . ' failed during TLS handshake: ' . $error->getMessage(),
65+
$error->getCode()
66+
);
6167
});
6268
});
69+
70+
return new \React\Promise\Promise(
71+
function ($resolve, $reject) use ($promise) {
72+
$promise->then($resolve, $reject);
73+
},
74+
function ($_, $reject) use (&$promise, $uri, &$connected) {
75+
if ($connected) {
76+
$reject(new \RuntimeException('Connection to ' . $uri . ' cancelled during TLS handshake'));
77+
}
78+
79+
$promise->cancel();
80+
$promise = null;
81+
}
82+
);
6383
}
6484
}

‎src/SecureServer.php‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -184,6 +184,11 @@ function ($conn) use ($that) {
184184
$that->emit('connection', array($conn));
185185
},
186186
function ($error) use ($that, $connection) {
187+
$error = new \RuntimeException(
188+
'Connection from ' . $connection->getRemoteAddress() . ' failed during TLS handshake: ' . $error->getMessage(),
189+
$error->getCode()
190+
);
191+
187192
$that->emit('error', array($error));
188193
$connection->end();
189194
}

‎src/StreamEncryption.php‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -136,15 +136,20 @@ public function toggleCrypto($socket, Deferred $deferred, $toggle, $method)
136136
if (true === $result) {
137137
$deferred->resolve();
138138
} else if (false === $result) {
139+
// overwrite callback arguments for PHP7+ only, so they do not show
140+
// up in the Exception trace and do not cause a possible cyclic reference.
141+
$d = $deferred;
142+
$deferred = null;
143+
139144
if (\feof($socket) || $error === null) {
140145
// EOF or failed without error => connection closed during handshake
141-
$deferred->reject(new UnexpectedValueException(
146+
$d->reject(new UnexpectedValueException(
142147
'Connection lost during TLS handshake',
143148
\defined('SOCKET_ECONNRESET') ? \SOCKET_ECONNRESET : 0
144149
));
145150
} else {
146151
// handshake failed with error message
147-
$deferred->reject(new UnexpectedValueException(
152+
$d->reject(new UnexpectedValueException(
148153
'Unable to complete TLS handshake: ' . $error
149154
));
150155
}

‎tests/FunctionalSecureServerTest.php‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -420,9 +420,12 @@ public function testEmitsErrorIfConnectionIsClosedBeforeHandshake()
420420

421421
$error = Block\await($errorEvent, $loop, self::TIMEOUT);
422422

423+
// Connection from tcp://127.0.0.1:39528 failed during TLS handshake: Connection lost during TLS handshak
423424
$this->assertTrue($error instanceof \RuntimeException);
424-
$this->assertEquals('Connection lost during TLS handshake', $error->getMessage());
425+
$this->assertStringStartsWith('Connection from tcp://', $error->getMessage());
426+
$this->assertStringEndsWith('failed during TLS handshake: Connection lost during TLS handshake', $error->getMessage());
425427
$this->assertEquals(defined('SOCKET_ECONNRESET') ? SOCKET_ECONNRESET : 0, $error->getCode());
428+
$this->assertNull($error->getPrevious());
426429
}
427430

428431
public function testEmitsErrorIfConnectionIsClosedWithIncompleteHandshake()
@@ -445,9 +448,12 @@ public function testEmitsErrorIfConnectionIsClosedWithIncompleteHandshake()
445448

446449
$error = Block\await($errorEvent, $loop, self::TIMEOUT);
447450

451+
// Connection from tcp://127.0.0.1:39528 failed during TLS handshake: Connection lost during TLS handshak
448452
$this->assertTrue($error instanceof \RuntimeException);
449-
$this->assertEquals('Connection lost during TLS handshake', $error->getMessage());
453+
$this->assertStringStartsWith('Connection from tcp://', $error->getMessage());
454+
$this->assertStringEndsWith('failed during TLS handshake: Connection lost during TLS handshake', $error->getMessage());
450455
$this->assertEquals(defined('SOCKET_ECONNRESET') ? SOCKET_ECONNRESET : 0, $error->getCode());
456+
$this->assertNull($error->getPrevious());
451457
}
452458

453459
public function testEmitsNothingIfConnectionIsIdle()

‎tests/IntegrationTest.php‎

Lines changed: 40 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -280,6 +280,46 @@ function ($e) use (&$wait) {
280280
$this->assertEquals(0, gc_collect_cycles());
281281
}
282282

283+
/**
284+
* @requires PHP 7
285+
*/
286+
public function testWaitingForInvalidTlsConnectionShouldNotCreateAnyGarbageReferences()
287+
{
288+
if (class_exists('React\Promise\When')) {
289+
$this->markTestSkipped('Not supported on legacy Promise v1 API');
290+
}
291+
292+
$loop = Factory::create();
293+
$connector = new Connector($loop, array(
294+
'tls' => array(
295+
'verify_peer' => true
296+
)
297+
));
298+
299+
gc_collect_cycles();
300+
301+
$wait = true;
302+
$promise = $connector->connect('tls://self-signed.badssl.com:443')->then(
303+
null,
304+
function ($e) use (&$wait) {
305+
$wait = false;
306+
throw $e;
307+
}
308+
);
309+
310+
// run loop for short period to ensure we detect DNS error
311+
Block\sleep(0.1, $loop);
312+
if ($wait) {
313+
Block\sleep(0.4, $loop);
314+
if ($wait) {
315+
$this->fail('Connection attempt did not fail');
316+
}
317+
}
318+
unset($promise);
319+
320+
$this->assertEquals(0, gc_collect_cycles());
321+
}
322+
283323
public function testWaitingForSuccessfullyClosedConnectionShouldNotCreateAnyGarbageReferences()
284324
{
285325
if (class_exists('React\Promise\When')) {
@@ -303,10 +343,6 @@ function ($conn) {
303343

304344
public function testConnectingFailsIfTimeoutIsTooSmall()
305345
{
306-
if (!function_exists('stream_socket_enable_crypto')) {
307-
$this->markTestSkipped('Not supported on your platform (outdated HHVM?)');
308-
}
309-
310346
$loop = Factory::create();
311347

312348
$connector = new Connector($loop, array(

‎tests/SecureConnectorTest.php‎

Lines changed: 149 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
namespace React\Tests\Socket;
44

55
use React\Promise;
6+
use React\Promise\Deferred;
67
use React\Socket\SecureConnector;
78

89
class SecureConnectorTest extends TestCase
@@ -51,16 +52,33 @@ public function testConnectionToInvalidSchemeWillReject()
5152

5253
public function testCancelDuringTcpConnectionCancelsTcpConnection()
5354
{
54-
$pending = new Promise\Promise(function () { }, function () { throw new \Exception(); });
55+
$pending = new Promise\Promise(function () { }, $this->expectCallableOnce());
56+
$this->tcp->expects($this->once())->method('connect')->with('example.com:80')->willReturn($pending);
57+
58+
$promise = $this->connector->connect('example.com:80');
59+
$promise->cancel();
60+
}
61+
62+
/**
63+
* @expectedException RuntimeException
64+
* @expectedExceptionMessage Connection cancelled
65+
*/
66+
public function testCancelDuringTcpConnectionCancelsTcpConnectionAndRejectsWithTcpRejection()
67+
{
68+
$pending = new Promise\Promise(function () { }, function () { throw new \RuntimeException('Connection cancelled'); });
5569
$this->tcp->expects($this->once())->method('connect')->with($this->equalTo('example.com:80'))->will($this->returnValue($pending));
5670

5771
$promise = $this->connector->connect('example.com:80');
5872
$promise->cancel();
5973

60-
$promise->then($this->expectCallableNever(), $this->expectCallableOnce());
74+
$this->throwRejection($promise);
6175
}
6276

63-
public function testConnectionWillBeClosedAndRejectedIfConnectioIsNoStream()
77+
/**
78+
* @expectedException UnexpectedValueException
79+
* @expectedExceptionMessage Base connector does not use internal Connection class exposing stream resource
80+
*/
81+
public function testConnectionWillBeClosedAndRejectedIfConnectionIsNoStream()
6482
{
6583
$connection = $this->getMockBuilder('React\Socket\ConnectionInterface')->getMock();
6684
$connection->expects($this->once())->method('close');
@@ -69,6 +87,133 @@ public function testConnectionWillBeClosedAndRejectedIfConnectioIsNoStream()
6987

7088
$promise = $this->connector->connect('example.com:80');
7189

72-
$promise->then($this->expectCallableNever(), $this->expectCallableOnce());
90+
$this->throwRejection($promise);
91+
}
92+
93+
public function testStreamEncryptionWillBeEnabledAfterConnecting()
94+
{
95+
$connection = $this->getMockBuilder('React\Socket\Connection')->disableOriginalConstructor()->getMock();
96+
97+
$encryption = $this->getMockBuilder('React\Socket\StreamEncryption')->disableOriginalConstructor()->getMock();
98+
$encryption->expects($this->once())->method('enable')->with($connection)->willReturn(new \React\Promise\Promise(function () { }));
99+
100+
$ref = new \ReflectionProperty($this->connector, 'streamEncryption');
101+
$ref->setAccessible(true);
102+
$ref->setValue($this->connector, $encryption);
103+
104+
$pending = new Promise\Promise(function () { }, function () { throw new \RuntimeException('Connection cancelled'); });
105+
$this->tcp->expects($this->once())->method('connect')->with($this->equalTo('example.com:80'))->willReturn(Promise\resolve($connection));
106+
107+
$promise = $this->connector->connect('example.com:80');
108+
}
109+
110+
public function testConnectionWillBeRejectedIfStreamEncryptionFailsAndClosesConnection()
111+
{
112+
$connection = $this->getMockBuilder('React\Socket\Connection')->disableOriginalConstructor()->getMock();
113+
$connection->expects($this->once())->method('close');
114+
115+
$encryption = $this->getMockBuilder('React\Socket\StreamEncryption')->disableOriginalConstructor()->getMock();
116+
$encryption->expects($this->once())->method('enable')->willReturn(Promise\reject(new \RuntimeException('TLS error', 123)));
117+
118+
$ref = new \ReflectionProperty($this->connector, 'streamEncryption');
119+
$ref->setAccessible(true);
120+
$ref->setValue($this->connector, $encryption);
121+
122+
$pending = new Promise\Promise(function () { }, function () { throw new \RuntimeException('Connection cancelled'); });
123+
$this->tcp->expects($this->once())->method('connect')->with($this->equalTo('example.com:80'))->willReturn(Promise\resolve($connection));
124+
125+
$promise = $this->connector->connect('example.com:80');
126+
127+
try {
128+
$this->throwRejection($promise);
129+
} catch (\RuntimeException $e) {
130+
$this->assertEquals('Connection to example.com:80 failed during TLS handshake: TLS error', $e->getMessage());
131+
$this->assertEquals(123, $e->getCode());
132+
$this->assertNull($e->getPrevious());
133+
}
134+
}
135+
136+
/**
137+
* @expectedException RuntimeException
138+
* @expectedExceptionMessage Connection to example.com:80 cancelled during TLS handshake
139+
*/
140+
public function testCancelDuringStreamEncryptionCancelsEncryptionAndClosesConnection()
141+
{
142+
$connection = $this->getMockBuilder('React\Socket\Connection')->disableOriginalConstructor()->getMock();
143+
$connection->expects($this->once())->method('close');
144+
145+
$pending = new Promise\Promise(function () { }, function () {
146+
throw new \Exception('Ignored');
147+
});
148+
$encryption = $this->getMockBuilder('React\Socket\StreamEncryption')->disableOriginalConstructor()->getMock();
149+
$encryption->expects($this->once())->method('enable')->willReturn($pending);
150+
151+
$ref = new \ReflectionProperty($this->connector, 'streamEncryption');
152+
$ref->setAccessible(true);
153+
$ref->setValue($this->connector, $encryption);
154+
155+
$this->tcp->expects($this->once())->method('connect')->with($this->equalTo('example.com:80'))->willReturn(Promise\resolve($connection));
156+
157+
$promise = $this->connector->connect('example.com:80');
158+
$promise->cancel();
159+
160+
$this->throwRejection($promise);
161+
}
162+
163+
public function testRejectionDuringConnectionShouldNotCreateAnyGarbageReferences()
164+
{
165+
if (class_exists('React\Promise\When')) {
166+
$this->markTestSkipped('Not supported on legacy Promise v1 API');
167+
}
168+
169+
gc_collect_cycles();
170+
171+
$tcp = new Deferred();
172+
$this->tcp->expects($this->once())->method('connect')->willReturn($tcp->promise());
173+
174+
$promise = $this->connector->connect('example.com:80');
175+
$tcp->reject(new \RuntimeException());
176+
unset($promise, $tcp);
177+
178+
$this->assertEquals(0, gc_collect_cycles());
179+
}
180+
181+
public function testRejectionDuringTlsHandshakeShouldNotCreateAnyGarbageReferences()
182+
{
183+
if (class_exists('React\Promise\When')) {
184+
$this->markTestSkipped('Not supported on legacy Promise v1 API');
185+
}
186+
187+
gc_collect_cycles();
188+
189+
$connection = $this->getMockBuilder('React\Socket\Connection')->disableOriginalConstructor()->getMock();
190+
191+
$tcp = new Deferred();
192+
$this->tcp->expects($this->once())->method('connect')->willReturn($tcp->promise());
193+
194+
$tls = new Deferred();
195+
$encryption = $this->getMockBuilder('React\Socket\StreamEncryption')->disableOriginalConstructor()->getMock();
196+
$encryption->expects($this->once())->method('enable')->willReturn($tls->promise());
197+
198+
$ref = new \ReflectionProperty($this->connector, 'streamEncryption');
199+
$ref->setAccessible(true);
200+
$ref->setValue($this->connector, $encryption);
201+
202+
$promise = $this->connector->connect('example.com:80');
203+
$tcp->resolve($connection);
204+
$tls->reject(new \RuntimeException());
205+
unset($promise, $tcp, $tls);
206+
207+
$this->assertEquals(0, gc_collect_cycles());
208+
}
209+
210+
private function throwRejection($promise)
211+
{
212+
$ex = null;
213+
$promise->then(null, function ($e) use (&$ex) {
214+
$ex = $e;
215+
});
216+
217+
throw $ex;
73218
}
74219
}

0 commit comments

Comments
 (0)