From 19daa7094706d99100cdf03235280b96468a21a1 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Tue, 29 Aug 2023 19:47:24 +0200 Subject: [PATCH 1/8] clear sftp stat cache when opening a write stream Signed-off-by: Robin Appelman --- apps/files_external/lib/Lib/Storage/SFTP.php | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/apps/files_external/lib/Lib/Storage/SFTP.php b/apps/files_external/lib/Lib/Storage/SFTP.php index 532c50808db23..3ab7ccae51d1b 100644 --- a/apps/files_external/lib/Lib/Storage/SFTP.php +++ b/apps/files_external/lib/Lib/Storage/SFTP.php @@ -370,6 +370,7 @@ public function unlink($path) { public function fopen($path, $mode) { try { $absPath = $this->absPath($path); + $connection = $this->getConnection(); switch ($mode) { case 'r': case 'rb': @@ -377,13 +378,14 @@ public function fopen($path, $mode) { return false; } SFTPReadStream::register(); - $context = stream_context_create(['sftp' => ['session' => $this->getConnection()]]); + $context = stream_context_create(['sftp' => ['session' => $connection]]); $handle = fopen('sftpread://' . trim($absPath, '/'), 'r', false, $context); return RetryWrapper::wrap($handle); case 'w': case 'wb': SFTPWriteStream::register(); - $context = stream_context_create(['sftp' => ['session' => $this->getConnection()]]); + $connection->_remove_from_stat_cache($absPath); + $context = stream_context_create(['sftp' => ['session' => $connection]]); return fopen('sftpwrite://' . trim($absPath, '/'), 'w', false, $context); case 'a': case 'ab': @@ -395,7 +397,7 @@ public function fopen($path, $mode) { case 'x+': case 'c': case 'c+': - $context = stream_context_create(['sftp' => ['session' => $this->getConnection()]]); + $context = stream_context_create(['sftp' => ['session' => $connection]]); $handle = fopen($this->constructUrl($path), $mode, false, $context); return RetryWrapper::wrap($handle); } From 9cf732a90b14d1e51c7a6946b34d9b117a25725b Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Tue, 29 Aug 2023 22:14:46 +0200 Subject: [PATCH 2/8] fix error during sftp stream close Signed-off-by: Robin Appelman --- apps/files_external/lib/Lib/Storage/SFTPReadStream.php | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/apps/files_external/lib/Lib/Storage/SFTPReadStream.php b/apps/files_external/lib/Lib/Storage/SFTPReadStream.php index c4749b154538e..7a53f41aa6124 100644 --- a/apps/files_external/lib/Lib/Storage/SFTPReadStream.php +++ b/apps/files_external/lib/Lib/Storage/SFTPReadStream.php @@ -49,6 +49,7 @@ class SFTPReadStream implements File { private $eof = false; private $buffer = ''; + private bool $pendingRead = false; public static function register($protocol = 'sftpread') { if (in_array($protocol, stream_get_wrappers(), true)) { @@ -143,10 +144,12 @@ public function stream_read($count) { private function request_chunk($size) { $packet = pack('Na*N3', strlen($this->handle), $this->handle, $this->internalPosition / 4294967296, $this->internalPosition, $size); + $this->pendingRead = true; return $this->sftp->_send_sftp_packet(NET_SFTP_READ, $packet); } private function read_chunk() { + $this->pendingRead = false; $response = $this->sftp->_get_sftp_packet(); switch ($this->sftp->packet_type) { @@ -195,6 +198,10 @@ public function stream_eof() { } public function stream_close() { + // we still have a read request incoming that needs to be handled before we can close + if ($this->pendingRead) { + $this->sftp->_get_sftp_packet(); + } if (!$this->sftp->_close_handle($this->handle)) { return false; } From 1dfef9fccf52bd8c2a6c2e13580c7d5b0e66ec30 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Tue, 29 Aug 2023 22:15:12 +0200 Subject: [PATCH 3/8] sftp optimize file_put_contents, writeStream and copy Signed-off-by: Robin Appelman --- apps/files_external/lib/Lib/Storage/SFTP.php | 56 +++++++++++++++++++- 1 file changed, 55 insertions(+), 1 deletion(-) diff --git a/apps/files_external/lib/Lib/Storage/SFTP.php b/apps/files_external/lib/Lib/Storage/SFTP.php index 3ab7ccae51d1b..9bcb624c41097 100644 --- a/apps/files_external/lib/Lib/Storage/SFTP.php +++ b/apps/files_external/lib/Lib/Storage/SFTP.php @@ -36,15 +36,18 @@ */ namespace OCA\Files_External\Lib\Storage; +use Icewind\Streams\CountWrapper; use Icewind\Streams\IteratorDirectory; use Icewind\Streams\RetryWrapper; +use OC\Files\Filesystem; +use OC\Files\Storage\Common; use phpseclib\Net\SFTP\Stream; /** * Uses phpseclib's Net\SFTP class and the Net\SFTP\Stream stream wrapper to * provide access to SFTP servers. */ -class SFTP extends \OC\Files\Storage\Common { +class SFTP extends Common { private $host; private $user; private $root; @@ -57,6 +60,8 @@ class SFTP extends \OC\Files\Storage\Common { */ protected $client; + const COPY_CHUNK_SIZE = 8 * 1024 * 1024; + /** * @param string $host protocol://server:port * @return array [$server, $port] @@ -478,4 +483,53 @@ public function constructUrl($path) { $url = 'sftp://' . urlencode($this->user) . '@' . $this->host . ':' . $this->port . $this->root . $path; return $url; } + + public function file_put_contents($path, $data) { + $result = $this->getConnection()->put($this->absPath($path), $data); + if ($result) { + return strlen($data); + } else { + return false; + } + } + + public function writeStream(string $path, $stream, int $size = null): int { + if ($size === null) { + $stream = CountWrapper::wrap($stream, function ($writtenSize) use (&$size) { + $size = $writtenSize; + }); + } + $result = $this->getConnection()->put($this->absPath($path), $stream); + fclose($stream); + if ($result) { + return $size; + } else { + throw new \Exception("Failed to write steam to sftp storage"); + } + } + + public function copy($source, $target) { + if ($this->is_dir($source) || $this->is_dir($target)) { + return parent::copy($source, $target); + } else { + $absSource = $this->absPath($source); + $absTarget = $this->absPath($target); + + $connection = $this->getConnection(); + $size = $connection->size($absSource); + if ($size === false) { + return false; + } + for ($i = 0; $i < $size; $i += self::COPY_CHUNK_SIZE) { + $chunk = $connection->get($absSource, false, $i, self::COPY_CHUNK_SIZE); + if ($chunk === false) { + return false; + } + if (!$connection->put($absTarget, $chunk, \phpseclib\Net\SFTP::SOURCE_STRING, $i)) { + return false; + } + } + return true; + } + } } From b41c8e0ef52d7fdd491d19b13d04c5d5a9546478 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Thu, 31 Aug 2023 15:21:16 +0200 Subject: [PATCH 4/8] more optimized getPermissions/getMetaData Signed-off-by: Robin Appelman --- apps/files_external/lib/Lib/Storage/SFTP.php | 44 ++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/apps/files_external/lib/Lib/Storage/SFTP.php b/apps/files_external/lib/Lib/Storage/SFTP.php index 9bcb624c41097..5dd11b2fe537d 100644 --- a/apps/files_external/lib/Lib/Storage/SFTP.php +++ b/apps/files_external/lib/Lib/Storage/SFTP.php @@ -41,6 +41,8 @@ use Icewind\Streams\RetryWrapper; use OC\Files\Filesystem; use OC\Files\Storage\Common; +use OCP\Constants; +use OCP\Files\FileInfo; use phpseclib\Net\SFTP\Stream; /** @@ -532,4 +534,46 @@ public function copy($source, $target) { return true; } } + + public function getPermissions($path) { + $stat = $this->getConnection()->stat($this->absPath($path)); + if (!$stat) { + return 0; + } + if ($stat['type'] === NET_SFTP_TYPE_DIRECTORY) { + return Constants::PERMISSION_ALL; + } else { + return Constants::PERMISSION_ALL - Constants::PERMISSION_CREATE; + } + } + + public function getMetaData($path) { + $stat = $this->getConnection()->stat($this->absPath($path)); + if (!$stat) { + return null; + } + + if ($stat['type'] === NET_SFTP_TYPE_DIRECTORY) { + $permissions = Constants::PERMISSION_ALL; + } else { + $permissions = Constants::PERMISSION_ALL - Constants::PERMISSION_CREATE; + } + + if ($stat['type'] === NET_SFTP_TYPE_DIRECTORY) { + $stat['size'] = -1; + $stat['mimetype'] = FileInfo::MIMETYPE_FOLDER; + } else { + $stat['mimetype'] = \OC::$server->getMimeTypeDetector()->detectPath($path); + } + + $stat['etag'] = $this->getETag($path); + $stat['storage_mtime'] = $stat['mtime']; + $stat['permissions'] = $permissions; + $stat['name'] = basename($path); + + $keys = ['size', 'mtime', 'mimetype', 'etag', 'storage_mtime', 'permissions', 'name']; + return array_intersect_key($stat, array_flip($keys)); + } + + } From aa2a3ae6c8c7d5edf99a194a430bf6cccd102e39 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Thu, 31 Aug 2023 15:39:56 +0200 Subject: [PATCH 5/8] implement fseek for sftp read stream Signed-off-by: Robin Appelman --- apps/files_external/lib/Lib/Storage/SFTP.php | 5 ++-- .../lib/Lib/Storage/SFTPReadStream.php | 28 ++++++++++++++++++- 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/apps/files_external/lib/Lib/Storage/SFTP.php b/apps/files_external/lib/Lib/Storage/SFTP.php index 5dd11b2fe537d..bc25477bcf674 100644 --- a/apps/files_external/lib/Lib/Storage/SFTP.php +++ b/apps/files_external/lib/Lib/Storage/SFTP.php @@ -381,11 +381,12 @@ public function fopen($path, $mode) { switch ($mode) { case 'r': case 'rb': - if (!$this->file_exists($path)) { + $stat = $this->stat($path); + if (!$stat) { return false; } SFTPReadStream::register(); - $context = stream_context_create(['sftp' => ['session' => $connection]]); + $context = stream_context_create(['sftp' => ['session' => $connection, 'size' => $stat['size']]]); $handle = fopen('sftpread://' . trim($absPath, '/'), 'r', false, $context); return RetryWrapper::wrap($handle); case 'w': diff --git a/apps/files_external/lib/Lib/Storage/SFTPReadStream.php b/apps/files_external/lib/Lib/Storage/SFTPReadStream.php index 7a53f41aa6124..aaae811c57b45 100644 --- a/apps/files_external/lib/Lib/Storage/SFTPReadStream.php +++ b/apps/files_external/lib/Lib/Storage/SFTPReadStream.php @@ -50,6 +50,7 @@ class SFTPReadStream implements File { private $buffer = ''; private bool $pendingRead = false; + private int $size = 0; public static function register($protocol = 'sftpread') { if (in_array($protocol, stream_get_wrappers(), true)) { @@ -76,6 +77,9 @@ protected function loadContext($name) { } else { throw new \BadMethodCallException('Invalid context, session not set'); } + if (isset($context['size'])) { + $this->size = $context['size']; + } return $context; } @@ -119,7 +123,25 @@ public function stream_open($path, $mode, $options, &$opened_path) { } public function stream_seek($offset, $whence = SEEK_SET) { - return false; + switch ($whence) { + case SEEK_SET: + $this->seekTo($offset); + break; + case SEEK_CUR: + $this->seekTo($this->readPosition + $offset); + break; + case SEEK_END: + $this->seekTo($this->size + $offset); + break; + } + return true; + } + + private function seekTo(int $offset) { + $this->internalPosition = $offset; + $this->readPosition = $offset; + $this->buffer = ''; + $this->request_chunk(256 * 1024); } public function stream_tell() { @@ -143,6 +165,10 @@ public function stream_read($count) { } private function request_chunk($size) { + if ($this->pendingRead) { + $this->sftp->_get_sftp_packet(); + } + $packet = pack('Na*N3', strlen($this->handle), $this->handle, $this->internalPosition / 4294967296, $this->internalPosition, $size); $this->pendingRead = true; return $this->sftp->_send_sftp_packet(NET_SFTP_READ, $packet); From e3a39f5117a7054dab4b9ef264247686c55738b9 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Thu, 31 Aug 2023 15:46:13 +0200 Subject: [PATCH 6/8] add sftp ci test Signed-off-by: Robin Appelman --- .github/workflows/sftp.yml | 75 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 75 insertions(+) create mode 100644 .github/workflows/sftp.yml diff --git a/.github/workflows/sftp.yml b/.github/workflows/sftp.yml new file mode 100644 index 0000000000000..939372a53d20d --- /dev/null +++ b/.github/workflows/sftp.yml @@ -0,0 +1,75 @@ +name: SFTP unit tests +on: + push: + branches: + - master + - stable* + paths: + - 'apps/files_external/**' + pull_request: + paths: + - 'apps/files_external/**' + +env: + APP_NAME: files_external + +jobs: + sftp-tests: + runs-on: ubuntu-latest + + if: ${{ github.repository_owner != 'nextcloud-gmbh' }} + + strategy: + # do not stop on another job's failure + fail-fast: false + matrix: + php-versions: ['8.0'] + sftpd: ['openssh'] + + name: php${{ matrix.php-versions }}-${{ matrix.sftpd }} + + steps: + - name: Checkout server + uses: actions/checkout@v3 + with: + submodules: true + + - name: Set up sftpd + run: | + sudo mkdir /tmp/sftp + sudo chown -R 0777 /tmp/sftp + if [[ "${{ matrix.sftpd }}" == 'openssh' ]]; then docker run -p 2222:22 --name sftp -d -v /tmp/sftp:/home/test atmoz/sftp "test:test:::data"; fi + - name: Set up php ${{ matrix.php-versions }} + uses: shivammathur/setup-php@c5fc0d8281aba02c7fda07d3a70cc5371548067d #v2.25.2 + with: + php-version: ${{ matrix.php-versions }} + tools: phpunit:9 + extensions: mbstring, fileinfo, intl, sqlite, pdo_sqlite, zip, gd + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + + - name: Set up Nextcloud + run: | + mkdir data + ./occ maintenance:install --verbose --database=sqlite --database-name=nextcloud --database-host=127.0.0.1 --database-user=root --database-pass=rootpassword --admin-user admin --admin-pass password + ./occ app:enable --force ${{ env.APP_NAME }} + php -S localhost:8080 & + - name: PHPUnit + run: | + echo " true, 'host' => 'localhost:2222','user' => 'test','password' => 'test', 'root' => 'data'];" > apps/${{ env.APP_NAME }}/tests/config.sftp.php + phpunit --configuration tests/phpunit-autotest-external.xml apps/files_external/tests/Storage/SftpTest.php + - name: sftpd logs + if: always() + run: | + ls -l /tmp/sftp + docker logs sftp + + sftp-summary: + runs-on: ubuntu-latest + needs: sftp-tests + + if: always() + + steps: + - name: Summary status + run: if ${{ needs.sftp-tests.result != 'success' }}; then exit 1; fi From c264903cb9142cddcc584776bea73b788339193d Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Fri, 1 Sep 2023 17:34:58 +0200 Subject: [PATCH 7/8] psalm suppress Signed-off-by: Robin Appelman --- apps/files_external/lib/Lib/Storage/SFTP.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/apps/files_external/lib/Lib/Storage/SFTP.php b/apps/files_external/lib/Lib/Storage/SFTP.php index bc25477bcf674..e792cef75a99b 100644 --- a/apps/files_external/lib/Lib/Storage/SFTP.php +++ b/apps/files_external/lib/Lib/Storage/SFTP.php @@ -488,6 +488,7 @@ public function constructUrl($path) { } public function file_put_contents($path, $data) { + /** @psalm-suppress InternalMethod */ $result = $this->getConnection()->put($this->absPath($path), $data); if ($result) { return strlen($data); @@ -502,6 +503,7 @@ public function writeStream(string $path, $stream, int $size = null): int { $size = $writtenSize; }); } + /** @psalm-suppress InternalMethod */ $result = $this->getConnection()->put($this->absPath($path), $stream); fclose($stream); if ($result) { @@ -524,10 +526,12 @@ public function copy($source, $target) { return false; } for ($i = 0; $i < $size; $i += self::COPY_CHUNK_SIZE) { + /** @psalm-suppress InvalidArgument */ $chunk = $connection->get($absSource, false, $i, self::COPY_CHUNK_SIZE); if ($chunk === false) { return false; } + /** @psalm-suppress InternalMethod */ if (!$connection->put($absTarget, $chunk, \phpseclib\Net\SFTP::SOURCE_STRING, $i)) { return false; } From d42d80917089d84d0f07f9d5e561102fe1354e1a Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 4 Sep 2023 16:36:15 +0200 Subject: [PATCH 8/8] sftp psalm fixes Signed-off-by: Robin Appelman --- apps/files_external/lib/Lib/Storage/SFTP.php | 24 +++++++++++-------- .../lib/Lib/Storage/SFTPReadStream.php | 2 +- 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/apps/files_external/lib/Lib/Storage/SFTP.php b/apps/files_external/lib/Lib/Storage/SFTP.php index e792cef75a99b..5c4dcca65cea4 100644 --- a/apps/files_external/lib/Lib/Storage/SFTP.php +++ b/apps/files_external/lib/Lib/Storage/SFTP.php @@ -43,6 +43,7 @@ use OC\Files\Storage\Common; use OCP\Constants; use OCP\Files\FileInfo; +use OCP\Files\IMimeTypeDetector; use phpseclib\Net\SFTP\Stream; /** @@ -61,6 +62,7 @@ class SFTP extends Common { * @var \phpseclib\Net\SFTP */ protected $client; + private IMimeTypeDetector $mimeTypeDetector; const COPY_CHUNK_SIZE = 8 * 1024 * 1024; @@ -118,6 +120,7 @@ public function __construct($params) { $this->root = '/' . ltrim($this->root, '/'); $this->root = rtrim($this->root, '/') . '/'; + $this->mimeTypeDetector = \OC::$server->get(IMimeTypeDetector::class); } /** @@ -392,6 +395,7 @@ public function fopen($path, $mode) { case 'w': case 'wb': SFTPWriteStream::register(); + // the SFTPWriteStream doesn't go through the "normal" methods so it doesn't clear the stat cache. $connection->_remove_from_stat_cache($absPath); $context = stream_context_create(['sftp' => ['session' => $connection]]); return fopen('sftpwrite://' . trim($absPath, '/'), 'w', false, $context); @@ -460,14 +464,14 @@ public function rename($source, $target) { } /** - * {@inheritdoc} + * @return array{mtime: int, size: int, ctime: int}|false */ public function stat($path) { try { $stat = $this->getConnection()->stat($this->absPath($path)); - $mtime = $stat ? $stat['mtime'] : -1; - $size = $stat ? $stat['size'] : 0; + $mtime = $stat ? (int)$stat['mtime'] : -1; + $size = $stat ? (int)$stat['size'] : 0; return ['mtime' => $mtime, 'size' => $size, 'ctime' => -1]; } catch (\Exception $e) { @@ -499,9 +503,12 @@ public function file_put_contents($path, $data) { public function writeStream(string $path, $stream, int $size = null): int { if ($size === null) { - $stream = CountWrapper::wrap($stream, function ($writtenSize) use (&$size) { + $stream = CountWrapper::wrap($stream, function (int $writtenSize) use (&$size) { $size = $writtenSize; }); + if (!$stream) { + throw new \Exception("Failed to wrap stream"); + } } /** @psalm-suppress InternalMethod */ $result = $this->getConnection()->put($this->absPath($path), $stream); @@ -559,26 +566,23 @@ public function getMetaData($path) { } if ($stat['type'] === NET_SFTP_TYPE_DIRECTORY) { - $permissions = Constants::PERMISSION_ALL; + $stat['permissions'] = Constants::PERMISSION_ALL; } else { - $permissions = Constants::PERMISSION_ALL - Constants::PERMISSION_CREATE; + $stat['permissions'] = Constants::PERMISSION_ALL - Constants::PERMISSION_CREATE; } if ($stat['type'] === NET_SFTP_TYPE_DIRECTORY) { $stat['size'] = -1; $stat['mimetype'] = FileInfo::MIMETYPE_FOLDER; } else { - $stat['mimetype'] = \OC::$server->getMimeTypeDetector()->detectPath($path); + $stat['mimetype'] = $this->mimeTypeDetector->detectPath($path); } $stat['etag'] = $this->getETag($path); $stat['storage_mtime'] = $stat['mtime']; - $stat['permissions'] = $permissions; $stat['name'] = basename($path); $keys = ['size', 'mtime', 'mimetype', 'etag', 'storage_mtime', 'permissions', 'name']; return array_intersect_key($stat, array_flip($keys)); } - - } diff --git a/apps/files_external/lib/Lib/Storage/SFTPReadStream.php b/apps/files_external/lib/Lib/Storage/SFTPReadStream.php index aaae811c57b45..7a98c6b2a6d59 100644 --- a/apps/files_external/lib/Lib/Storage/SFTPReadStream.php +++ b/apps/files_external/lib/Lib/Storage/SFTPReadStream.php @@ -137,7 +137,7 @@ public function stream_seek($offset, $whence = SEEK_SET) { return true; } - private function seekTo(int $offset) { + private function seekTo(int $offset): void { $this->internalPosition = $offset; $this->readPosition = $offset; $this->buffer = '';