From cb58d3e66cfa3a25c808fa6bdbcf561b36bd4f89 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lu=C3=ADs=20Cobucci?= Date: Thu, 21 Apr 2022 11:31:58 +0200 Subject: [PATCH 1/4] Remove dead code This path can never be executed. --- src/PhpseclibV3/SftpConnectionProvider.php | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/PhpseclibV3/SftpConnectionProvider.php b/src/PhpseclibV3/SftpConnectionProvider.php index 082e6d3fb..82c1b422f 100644 --- a/src/PhpseclibV3/SftpConnectionProvider.php +++ b/src/PhpseclibV3/SftpConnectionProvider.php @@ -9,7 +9,6 @@ use phpseclib3\Exception\NoKeyLoadedException; use phpseclib3\Net\SFTP; use phpseclib3\System\SSH\Agent; -use RuntimeException; use Throwable; use function base64_decode; @@ -229,8 +228,6 @@ private function loadPrivateKey(): AsymmetricKey } catch (NoKeyLoadedException $exception) { throw new UnableToLoadPrivateKey(); } - - throw new RuntimeException(); } private function authenticateWithAgent(SFTP $connection): void From b0b5a52d0728b75b3190cb5e3e4454721cd8c469 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lu=C3=ADs=20Cobucci?= Date: Thu, 21 Apr 2022 11:34:07 +0200 Subject: [PATCH 2/4] Fix SFTP retry mechanism As of phpseclib v3 `RuntimeException` objects are thrown, causing the whole retry logic to be bypassed. This makes sure to re-attempt to connect when timeout or connection issues happen. --- src/PhpseclibV3/SftpConnectionProvider.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/PhpseclibV3/SftpConnectionProvider.php b/src/PhpseclibV3/SftpConnectionProvider.php index 82c1b422f..995f6a7d1 100644 --- a/src/PhpseclibV3/SftpConnectionProvider.php +++ b/src/PhpseclibV3/SftpConnectionProvider.php @@ -4,6 +4,7 @@ namespace League\Flysystem\PhpseclibV3; +use League\Flysystem\FilesystemException; use phpseclib3\Crypt\Common\AsymmetricKey; use phpseclib3\Crypt\PublicKeyLoader; use phpseclib3\Exception\NoKeyLoadedException; @@ -137,7 +138,10 @@ private function setupConnection(): SFTP $this->authenticate($connection); } catch (Throwable $exception) { $connection->disconnect(); - throw $exception; + + if ($exception instanceof FilesystemException) { + throw $exception; + } } return $connection; From 3d7706a73d6132789d33c404439afdabd21d2030 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lu=C3=ADs=20Cobucci?= Date: Thu, 21 Apr 2022 17:46:06 +0200 Subject: [PATCH 3/4] Provide way to simulate network issues for tests This introduces a proxy that simulates different network issues and a client to manage it. That is handy to be able to test for issues that can only be reproduced in specific scenarios/environments. The management API provided here is slimmed down to only reproduce peer resets but can be further extended if/when necessary. More information: https://github.com/shopify/toxiproxy --- docker-compose.yml | 12 +++ .../ToxiproxyManagement.php | 75 +++++++++++++++++++ test_files/toxiproxy/toxiproxy.json | 20 +++++ 3 files changed, 107 insertions(+) create mode 100644 src/AdapterTestUtilities/ToxiproxyManagement.php create mode 100644 test_files/toxiproxy/toxiproxy.json diff --git a/docker-compose.yml b/docker-compose.yml index 4e1b723b1..3ab6b7729 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -44,3 +44,15 @@ services: - "2122:21" - "30000-30009:30000-30009" command: "/run.sh -l puredb:/etc/pure-ftpd/pureftpd.pdb -E -j -P localhost" + toxiproxy: + container_name: toxiproxy + restart: unless-stopped + image: ghcr.io/shopify/toxiproxy + command: "-host 0.0.0.0 -config /opt/toxiproxy/config.json" + volumes: + - ./test_files/toxiproxy/toxiproxy.json:/opt/toxiproxy/config.json:ro + ports: + - "8474:8474" # HTTP API + - "8222:8222" # SFTP + - "8121:8121" # FTP + - "8122:8122" # FTPD diff --git a/src/AdapterTestUtilities/ToxiproxyManagement.php b/src/AdapterTestUtilities/ToxiproxyManagement.php new file mode 100644 index 000000000..47a4b1721 --- /dev/null +++ b/src/AdapterTestUtilities/ToxiproxyManagement.php @@ -0,0 +1,75 @@ +apiClient = $apiClient; + } + + public static function forServer(string $apiUri = 'http://localhost:8474'): self + { + return new self( + new Client( + [ + 'base_uri' => $apiUri, + 'base_url' => $apiUri, // Compatibility with older versions of Guzzle + ] + ) + ); + } + + public function removeAllToxics(): void + { + $this->apiClient->post('/reset'); + } + + /** + * Simulates a peer reset on the client->server direction. + * + * @param RegisteredProxies $proxyName + */ + public function resetPeerOnRequest( + string $proxyName, + int $timeoutInMilliseconds + ): void { + $configuration = [ + 'type' => 'reset_peer', + 'stream' => 'upstream', + 'attributes' => ['timeout' => $timeoutInMilliseconds], + ]; + + $this->addToxic($proxyName, $configuration); + } + + /** + * Registers a network toxic for the given proxy. + * + * @param RegisteredProxies $proxyName + * @param Toxic $configuration + */ + private function addToxic(string $proxyName, array $configuration): void + { + $this->apiClient->post('/proxies/' . $proxyName . '/toxics', ['json' => $configuration]); + } +} diff --git a/test_files/toxiproxy/toxiproxy.json b/test_files/toxiproxy/toxiproxy.json new file mode 100644 index 000000000..9396fb838 --- /dev/null +++ b/test_files/toxiproxy/toxiproxy.json @@ -0,0 +1,20 @@ +[ + { + "name": "sftp", + "listen": "[::]:8222", + "upstream": "sftp:22", + "enabled": true + }, + { + "name": "ftp", + "listen": "[::]:8121", + "upstream": "ftp:21", + "enabled": true + }, + { + "name": "ftpd", + "listen": "[::]:8122", + "upstream": "ftpd:21", + "enabled": true + } +] From 464a8b1ade964384b136cbdb63aa54645ac8db07 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lu=C3=ADs=20Cobucci?= Date: Thu, 21 Apr 2022 18:03:49 +0200 Subject: [PATCH 4/4] Verify SFTP retries when dealing with peer resets This verifies that Flysystem SFTP adapter goes through all the configured tries before throwing an `UnableToConnectToSftpHost` exception. Reverting the fix applied in https://github.com/thephpleague/flysystem/pull/1451 makes the test to fail due to an unexpected `RuntimeException`. Applying this for phpseclib v2 is a bit tricky, though, due to the lack of knowledge on what has happened in the flow (e.g. `false` is returned when a connection cannot be established or when an authentication error happened). To make it work, the adapter for that version would need to verify if there's a connection or not all the time - which doesn't sound very good to me. --- .../SftpConnectionProviderTest.php | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/src/PhpseclibV3/SftpConnectionProviderTest.php b/src/PhpseclibV3/SftpConnectionProviderTest.php index 31764c0e5..453bd9cfa 100644 --- a/src/PhpseclibV3/SftpConnectionProviderTest.php +++ b/src/PhpseclibV3/SftpConnectionProviderTest.php @@ -4,6 +4,7 @@ namespace League\Flysystem\PhpseclibV3; +use League\Flysystem\AdapterTestUtilities\ToxiproxyManagement; use phpseclib3\Net\SFTP; use PHPUnit\Framework\TestCase; @@ -241,6 +242,52 @@ public function providing_an_invalid_password(): void $provider->provideConnection(); } + /** + * @test + */ + public function retries_several_times_until_failure(): void + { + $connectivityChecker = new class implements ConnectivityChecker { + /** @var int */ + public $calls = 0; + + public function isConnected(SFTP $connection): bool + { + ++$this->calls; + + return $connection->isConnected(); + } + }; + + $managesConnectionToxics = ToxiproxyManagement::forServer(); + $managesConnectionToxics->resetPeerOnRequest('sftp', 10); + + $maxTries = 5; + + $provider = SftpConnectionProvider::fromArray( + [ + 'host' => 'localhost', + 'username' => 'bar', + 'privateKey' => __DIR__ . '/../../test_files/sftp/id_rsa', + 'passphrase' => 'secret', + 'port' => 8222, + 'maxTries' => $maxTries, + 'timeout' => 1, + 'connectivityChecker' => $connectivityChecker, + ] + ); + + $this->expectException(UnableToConnectToSftpHost::class); + + try { + $provider->provideConnection(); + } finally { + $managesConnectionToxics->removeAllToxics(); + + self::assertSame($maxTries + 1, $connectivityChecker->calls); + } + } + private function computeFingerPrint(string $publicKey): string { $content = explode(' ', $publicKey, 3);