From d5ac1ba27e0e859e71198b380d0a7a8a44446aaf Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Fri, 6 Nov 2026 13:44:03 +0100 Subject: [PATCH] fix(storage): require a saved SFTP host key, saved only by an admin validation - Restores, downloads, and the import check refuse an SFTP storage without a saved host key. - Only Validate, Save, Create, and the API validate endpoint save a new key; backup and revalidation jobs do not. - An endpoint change or a host key reset marks the storage unusable. - Save the key only when the endpoint did not change during the connection test. --- .../Controllers/Api/S3StoragesController.php | 2 +- app/Livewire/Project/Database/ImportForm.php | 5 +- app/Livewire/Storage/Create.php | 2 +- app/Livewire/Storage/Form.php | 12 ++- app/Models/S3Storage.php | 97 +++++++++++++++---- .../views/livewire/storage/form.blade.php | 4 +- ...seImportFormFileCheckAuthorizationTest.php | 32 ++++++ tests/Feature/SftpHostKeyPinningTest.php | 97 ++++++++++++++++++- tests/Unit/RcloneTransferTest.php | 2 +- tests/Unit/S3ReplicaDownloadTest.php | 2 +- 10 files changed, 218 insertions(+), 37 deletions(-) diff --git a/app/Http/Controllers/Api/S3StoragesController.php b/app/Http/Controllers/Api/S3StoragesController.php index da6801832a..b213e8d382 100644 --- a/app/Http/Controllers/Api/S3StoragesController.php +++ b/app/Http/Controllers/Api/S3StoragesController.php @@ -107,7 +107,7 @@ class S3StoragesController extends Controller private function validateStorageConnection(S3Storage $storage): array { try { - $storage->testConnection(shouldSave: true); + $storage->testConnection(shouldSave: true, acceptNewHostKey: true); return ['valid' => true, 'error' => null]; } catch (\Throwable $e) { diff --git a/app/Livewire/Project/Database/ImportForm.php b/app/Livewire/Project/Database/ImportForm.php index a7c0a4be37..e983ab5948 100644 --- a/app/Livewire/Project/Database/ImportForm.php +++ b/app/Livewire/Project/Database/ImportForm.php @@ -612,10 +612,9 @@ class ImportForm extends Component return; } - // Test connection - $s3Storage->testConnection(); - + // Before testConnection(), which saves the host key of an SFTP storage in memory only. $disk = $s3Storage->filesystem(); + $s3Storage->testConnection(); // Check if file exists if (! $disk->exists($cleanPath)) { diff --git a/app/Livewire/Storage/Create.php b/app/Livewire/Storage/Create.php index 0ab185563c..c876385e9c 100644 --- a/app/Livewire/Storage/Create.php +++ b/app/Livewire/Storage/Create.php @@ -145,7 +145,7 @@ class Create extends Component $this->storage->endpoint = $this->endpoint; } $this->storage->team_id = currentTeam()->id; - $this->storage->testConnection(); + $this->storage->testConnection(acceptNewHostKey: true); $this->storage->save(); return redirectRoute($this, 'storage.show', [$this->storage->uuid]); diff --git a/app/Livewire/Storage/Form.php b/app/Livewire/Storage/Form.php index 301f2555ad..37246cd9a9 100644 --- a/app/Livewire/Storage/Form.php +++ b/app/Livewire/Storage/Form.php @@ -230,7 +230,7 @@ class Form extends Component $testedStorage->private_key_id = $testedStorage->type === BackupStorageType::Sftp ? $this->selectedPrivateKeyId() : null; $testedStorage->secret = $testedStorage->private_key_id ? '' : $this->secretToUse(); - $testedStorage->testConnection(); + $testedStorage->testConnection(acceptNewHostKey: true); $switchedToVirtualHostedStyle = $this->usePathStyleEndpoint && ! $testedStorage->usesPathStyleEndpoint(); $this->usePathStyleEndpoint = $testedStorage->usesPathStyleEndpoint(); @@ -277,7 +277,7 @@ class Form extends Component $this->storage->save(); // Test connection with new values - if this fails, transaction will rollback - $this->storage->testConnection(shouldSave: false); + $this->storage->testConnection(shouldSave: false, acceptNewHostKey: true); // If we get here, the connection test succeeded $this->storage->is_usable = true; @@ -303,14 +303,16 @@ class Form extends Component } /** - * Removes the saved SFTP host key, for a server whose key changed on purpose. The next connection test saves the - * key that the server presents then. + * Removes the saved SFTP host key, for a server whose key changed on purpose. The next connection test that an admin + * starts (Validate) saves the key that the server presents then; backups do not. */ public function resetHostKey(): void { $this->authorize('update', $this->storage); - $this->storage->forceFill(['sftp_host_key' => null])->save(); + $this->storage->forceFill(['sftp_host_key' => null, 'is_usable' => false])->save(); + $this->isUsable = false; + $this->dispatch('storage-status-changed', isUsable: false); $this->dispatch('success', 'Host key removed.', 'Validate the connection to save the current host key of the server.'); } diff --git a/app/Models/S3Storage.php b/app/Models/S3Storage.php index e349b0f95c..0a2c977fbf 100644 --- a/app/Models/S3Storage.php +++ b/app/Models/S3Storage.php @@ -81,6 +81,8 @@ class S3Storage extends BaseModel */ public const RCLONE_REMOTE = 'storage'; + public const SFTP_HOST_KEY_MISSING_MESSAGE = 'The SFTP host key of this storage is not saved yet. Validate the connection in the storage settings to save it.'; + /** * The key of the rclone password obscuring (fs/config/obscure in rclone). It only hides the password from a casual * look at the config file; it is not encryption. @@ -364,7 +366,7 @@ class S3Storage extends BaseModel $host = trim((string) parse_url((string) $this->endpoint, PHP_URL_HOST), '[]'); $options = match ($this->storageType()) { // host_keys makes rclone refuse another host key, and offer only the algorithm of the saved key. - BackupStorageType::Sftp => ['type' => 'sftp', 'host' => $host, 'port' => $this->endpointPort(), 'shell_type' => 'none', ...($this->sftp_host_key ? ['host_keys' => $this->sftp_host_key] : [])], + BackupStorageType::Sftp => ['type' => 'sftp', 'host' => $host, 'port' => $this->endpointPort(), 'shell_type' => 'none', 'host_keys' => $this->requireSftpHostKey()], BackupStorageType::Ftps => ['type' => 'ftp', 'host' => $host, 'port' => $this->endpointPort(), 'explicit_tls' => 'true'], BackupStorageType::Webdav => ['type' => 'webdav', 'url' => $this->endpoint, 'vendor' => 'other'], BackupStorageType::S3 => throw new \LogicException('S3 storages use the MinIO client.'), @@ -471,9 +473,12 @@ class S3Storage extends BaseModel $previous = $attributes['endpoint'] ?? null; $server = fn (?string $endpoint): array => [strtolower(trim((string) parse_url((string) $endpoint, PHP_URL_HOST), '[]')), parse_url((string) $endpoint, PHP_URL_PORT)]; - return $previous !== null && $server($previous) !== $server($value) - ? ['endpoint' => $value, 'sftp_host_key' => null] - : ['endpoint' => $value]; + if ($previous === null || $server($previous) === $server($value)) { + return ['endpoint' => $value]; + } + + // Without the host key, the storage needs a new connection test before Coolify uses it. + return ['endpoint' => $value, 'sftp_host_key' => null, ...(($attributes['sftp_host_key'] ?? null) !== null ? ['is_usable' => false] : [])]; }, ); } @@ -506,7 +511,12 @@ class S3Storage extends BaseModel ); } - public function testConnection(bool $shouldSave = false) + /** + * @param bool $acceptNewHostKey Only when an admin validates the storage: an SFTP storage without a saved host + * key then saves the presented key. Background tests refuse such a storage, so a + * reset or changed key is never trusted without the admin. + */ + public function testConnection(bool $shouldSave = false, bool $acceptNewHostKey = false) { try { if ($this->isS3()) { @@ -538,11 +548,15 @@ class S3Storage extends BaseModel throw new \RuntimeException('Storage settings are not allowed: '.$validator->errors()->first()); } - $disk = $this->filesystem(); + if ($this->storageType() === BackupStorageType::Sftp && ! $acceptNewHostKey) { + $this->requireSftpHostKey(); + } + + $disk = $this->storageType() === BackupStorageType::Sftp ? $this->sftpFilesystem() : $this->filesystem(); $disk->files(); $adapter = $disk->getAdapter(); if ($adapter instanceof SftpFilesystemAdapter && $this->sftp_host_key === null) { - $this->sftp_host_key = $adapter->presentedHostKey(); + $this->pinSftpHostKey((string) $adapter->presentedHostKey(), $shouldSave); } } @@ -584,22 +598,17 @@ class S3Storage extends BaseModel /** * The files of the storage, as Coolify reads them (connection tests, restores, downloads, retention). SFTP and FTPS * connect to the validated IP of the host, and WebDAV pins it, so a DNS change cannot send them to another address. - * SFTP also checks the saved host key. + * SFTP also checks the saved host key, and refuses to connect before testConnection() saved it. */ public function filesystem(): FilesystemAdapter { + if ($this->storageType() === BackupStorageType::Sftp) { + $this->requireSftpHostKey(); + } + return match ($this->storageType()) { BackupStorageType::S3 => $this->s3Filesystem(), - BackupStorageType::Sftp => $this->adapterFilesystem(new SftpFilesystemAdapter(PinnedSftpConnectionProvider::create( - host: trim((string) parse_url((string) $this->endpoint, PHP_URL_HOST), '[]'), - address: $this->pinnedHost(), - port: $this->endpointPort(), - username: (string) $this->key, - password: $this->sftpPrivateKey() ? null : (string) $this->secret, - privateKey: $this->sftpPrivateKey()?->private_key, - pinnedHostKey: $this->sftp_host_key, - timeout: self::CONNECTION_TIMEOUT_SECONDS, - ), $this->basePath())), + BackupStorageType::Sftp => $this->sftpFilesystem(), BackupStorageType::Ftps => $this->adapterFilesystem(new FtpsFilesystemAdapter( host: trim((string) parse_url((string) $this->endpoint, PHP_URL_HOST), '[]'), port: $this->endpointPort(), @@ -612,6 +621,58 @@ class S3Storage extends BaseModel }; } + /** + * Without a saved host key, it accepts any key. Only testConnection() uses it so, to save the presented key. + */ + private function sftpFilesystem(): FilesystemAdapter + { + return $this->adapterFilesystem(new SftpFilesystemAdapter(PinnedSftpConnectionProvider::create( + host: trim((string) parse_url((string) $this->endpoint, PHP_URL_HOST), '[]'), + address: $this->pinnedHost(), + port: $this->endpointPort(), + username: (string) $this->key, + password: $this->sftpPrivateKey() ? null : (string) $this->secret, + privateKey: $this->sftpPrivateKey()?->private_key, + pinnedHostKey: $this->sftp_host_key, + timeout: self::CONNECTION_TIMEOUT_SECONDS, + ), $this->basePath())); + } + + private function requireSftpHostKey(): string + { + if (blank($this->sftp_host_key)) { + throw new \RuntimeException(self::SFTP_HOST_KEY_MISSING_MESSAGE); + } + + return $this->sftp_host_key; + } + + /** + * Saves the host key of the first connection only while the saved storage still has the tested endpoint and no + * key, so a test that started before an endpoint change cannot save the key of the old server. + */ + private function pinSftpHostKey(string $hostKey, bool $shouldSave): void + { + if ($hostKey === '') { + throw new \RuntimeException('The SFTP server did not present a host key.'); + } + if ($shouldSave && $this->exists) { + $saved = static::query()->whereKey($this->getKey()) + ->where('endpoint', $this->getAttributes()['endpoint'] ?? null) + ->whereNull('sftp_host_key') + ->update(['sftp_host_key' => $hostKey]); + if ($saved === 0) { + throw new \RuntimeException('The storage settings changed during the connection test. Validate the connection again.'); + } + $this->sftp_host_key = $hostKey; + $this->syncOriginalAttribute('sftp_host_key'); + + return; + } + + $this->sftp_host_key = $hostKey; + } + private function pinnedHost(): string { return SafeWebhookUrl::pinnedAddress((string) $this->endpoint, $this->storageType()->endpointSchemes(), $this->trustedInternalHosts()); diff --git a/resources/views/livewire/storage/form.blade.php b/resources/views/livewire/storage/form.blade.php index 128ddcaf23..ea16aeba3c 100644 --- a/resources/views/livewire/storage/form.blade.php +++ b/resources/views/livewire/storage/form.blade.php @@ -87,8 +87,8 @@ @if ($storage->type === App\Enums\BackupStorageType::Sftp)
+ :value="$storage->sftpHostKeyFingerprint() ?? 'Not saved yet. Validate the connection to save it.'" + helper="Coolify saves the host key of the server when you validate the connection, and refuses the server when the key changes. Backups do not save a new key. Reset it only when you know why the key changed." /> @can('update', $storage) @if ($storage->sftp_host_key) Reset host key diff --git a/tests/Feature/DatabaseImportFormFileCheckAuthorizationTest.php b/tests/Feature/DatabaseImportFormFileCheckAuthorizationTest.php index f56da8fe8f..fa46b16ab2 100644 --- a/tests/Feature/DatabaseImportFormFileCheckAuthorizationTest.php +++ b/tests/Feature/DatabaseImportFormFileCheckAuthorizationTest.php @@ -1,9 +1,11 @@ assertNotDispatched('error') ->assertSet('filename', null); }); + +it('does not check a file on an SFTP storage without a saved host key', function () { + $storage = S3Storage::create([ + 'team_id' => $this->team->id, + 'type' => BackupStorageType::Sftp, + 'name' => 'SFTP backups', + 'region' => 'us-east-1', + 'key' => 'backup', + 'secret' => 'password', + 'endpoint' => 'sftp://1.1.1.1:2222/backups', + 'is_usable' => true, + ]); + $connected = false; + app()->bind(SftpConnectionProvider::class, function () use (&$connected) { + $connected = true; + + throw new RuntimeException('Connected.'); + }); + + importFormFileCheckAs($this->team, 'owner') + ->set('s3StorageId', $storage->id) + ->set('s3Path', 'backups/db.dmp') + ->call('checkS3File') + ->assertDispatched('error') + ->assertSet('s3FileSize', null); + + expect($connected)->toBeFalse() + ->and($storage->fresh()->sftp_host_key)->toBeNull(); +}); diff --git a/tests/Feature/SftpHostKeyPinningTest.php b/tests/Feature/SftpHostKeyPinningTest.php index 25e2cc6866..7c806ed5a6 100644 --- a/tests/Feature/SftpHostKeyPinningTest.php +++ b/tests/Feature/SftpHostKeyPinningTest.php @@ -1,11 +1,13 @@ team); - $storage->testConnection(shouldSave: true); + $storage->testConnection(shouldSave: true, acceptNewHostKey: true); expect($storage->fresh()->sftp_host_key)->toBe(SFTP_TEST_HOST_KEY) ->and($storage->sftpHostKeyFingerprint())->toBe(SFTP_TEST_HOST_KEY_FINGERPRINT) @@ -92,7 +94,7 @@ it('saves the key type of an RSA host key, not the signature algorithm', functio fakeSftpServer('rsa-sha2-512 '.$blob); $storage = sftpHostKeyStorage($this->team); - $storage->testConnection(shouldSave: true); + $storage->testConnection(shouldSave: true, acceptNewHostKey: true); expect($storage->fresh()->sftp_host_key)->toBe('ssh-rsa '.$blob); }); @@ -123,9 +125,58 @@ it('pins the saved host key in the rclone config', function () { $storage = sftpHostKeyStorage($this->team, ['sftp_host_key' => SFTP_TEST_HOST_KEY]); expect($storage->rcloneConfig())->toContain("\nhost_keys = ".SFTP_TEST_HOST_KEY."\n"); +}); - $storage->forceFill(['sftp_host_key' => null]); - expect($storage->rcloneConfig())->not->toContain('host_keys'); +it('refuses to connect to an SFTP storage without a saved host key', function () { + fakeSftpServer(SFTP_TEST_HOST_KEY, parameters: $parameters); + $storage = sftpHostKeyStorage($this->team); + + expect(fn () => $storage->filesystem()->files())->toThrow(RuntimeException::class, S3Storage::SFTP_HOST_KEY_MISSING_MESSAGE) + ->and(fn () => $storage->rcloneConfig())->toThrow(RuntimeException::class, S3Storage::SFTP_HOST_KEY_MISSING_MESSAGE) + ->and($parameters)->toBeNull() + ->and($storage->fresh()->sftp_host_key)->toBeNull(); +}); + +it('does not need a host key for other storage types', function (BackupStorageType $type, string $endpoint) { + $storage = sftpHostKeyStorage($this->team, ['type' => $type, 'endpoint' => $endpoint]); + + expect($storage->rcloneConfig())->toStartWith('[storage]') + ->and($storage->filesystem())->toBeInstanceOf(FilesystemAdapter::class); +})->with([ + 'ftps' => [BackupStorageType::Ftps, 'ftps://1.1.1.1/backups'], + 'webdav' => [BackupStorageType::Webdav, 'https://1.1.1.1/backups'], +]); + +it('does not save a host key on an endpoint that changed during the connection test', function () { + fakeSftpServer(SFTP_TEST_HOST_KEY); + $storage = sftpHostKeyStorage($this->team); + S3Storage::query()->whereKey($storage->id)->update(['endpoint' => 'sftp://1.0.0.1:2222/backups']); + + expect(fn () => $storage->testConnection(shouldSave: true, acceptNewHostKey: true))->toThrow(RuntimeException::class); + + $storage = $storage->fresh(); + expect($storage->sftp_host_key)->toBeNull() + ->and($storage->endpoint)->toBe('sftp://1.0.0.1:2222/backups') + ->and($storage->is_usable)->toBeFalse(); +}); + +it('does not replace a host key that was saved during the connection test', function () { + fakeSftpServer(SFTP_TEST_HOST_KEY); + $storage = sftpHostKeyStorage($this->team); + $otherKey = 'ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIOMqqnkVzrm0SdG6UOoqKLsabgH5C9okWi0dh2l1GbjE'; + S3Storage::query()->whereKey($storage->id)->update(['sftp_host_key' => $otherKey]); + + expect(fn () => $storage->testConnection(shouldSave: true, acceptNewHostKey: true))->toThrow(RuntimeException::class); + + expect($storage->fresh()->sftp_host_key)->toBe($otherKey); +}); + +it('asks for a new connection test when the host key is removed', function () { + $storage = sftpHostKeyStorage($this->team, ['sftp_host_key' => SFTP_TEST_HOST_KEY]); + + $storage->update(['endpoint' => 'sftp://1.0.0.1:2222/backups']); + + expect($storage->fresh()->is_usable)->toBeFalse(); }); it('removes the saved host key when the server host or port changes', function (string $endpoint, bool $keeps) { @@ -156,12 +207,48 @@ it('lets an admin reset the host key, and the next connection test saves it agai session(['currentTeam' => $this->team]); $component = Livewire::test(Form::class, ['storage' => $storage])->call('resetHostKey'); - expect($storage->fresh()->sftp_host_key)->toBeNull(); + expect($storage->fresh()->sftp_host_key)->toBeNull() + ->and($storage->fresh()->is_usable)->toBeFalse(); $component->call('testConnection')->assertDispatched('success'); expect($storage->fresh()->sftp_host_key)->toBe(SFTP_TEST_HOST_KEY); }); +it('does not save a new host key from a background connection test', function () { + fakeSftpServer(SFTP_TEST_HOST_KEY, parameters: $parameters); + $storage = sftpHostKeyStorage($this->team); + + expect(fn () => $storage->testConnection(shouldSave: true)) + ->toThrow(RuntimeException::class, S3Storage::SFTP_HOST_KEY_MISSING_MESSAGE); + + $storage->refresh(); + expect($parameters)->toBeNull() + ->and($storage->sftp_host_key)->toBeNull() + ->and($storage->is_usable)->toBeFalse(); +}); + +it('does not save the host key of an unusable storage when the revalidation job runs', function () { + fakeSftpServer(SFTP_TEST_HOST_KEY, parameters: $parameters); + $storage = sftpHostKeyStorage($this->team, ['is_usable' => false]); + + (new RevalidateUnusableS3StoragesJob)->handle(); + + $storage->refresh(); + expect($parameters)->toBeNull() + ->and($storage->sftp_host_key)->toBeNull() + ->and($storage->is_usable)->toBeFalse(); +}); + +it('still checks a saved host key in a background connection test', function () { + fakeSftpServer(SFTP_TEST_HOST_KEY, parameters: $parameters); + $storage = sftpHostKeyStorage($this->team, ['sftp_host_key' => SFTP_TEST_HOST_KEY, 'is_usable' => false]); + + $storage->testConnection(shouldSave: true); + + expect($storage->fresh()->is_usable)->toBeTrue() + ->and($parameters['hostFingerprint'])->toBe(SFTP_TEST_HOST_KEY_FINGERPRINT); +}); + it('does not let a member reset the host key', function () { $storage = sftpHostKeyStorage($this->team, ['sftp_host_key' => SFTP_TEST_HOST_KEY]); $this->actingAs($this->member); diff --git a/tests/Unit/RcloneTransferTest.php b/tests/Unit/RcloneTransferTest.php index 01a1f8a12b..5742287639 100644 --- a/tests/Unit/RcloneTransferTest.php +++ b/tests/Unit/RcloneTransferTest.php @@ -30,7 +30,7 @@ function rcloneTransferTestServer(string $user = 'root'): Server function rcloneTransferTestStorage(): S3Storage { $storage = new S3Storage; - $storage->forceFill(['type' => 'sftp', 'endpoint' => 'sftp://1.1.1.1/backups', 'key' => 'backup', 'secret' => 'password']); + $storage->forceFill(['type' => 'sftp', 'endpoint' => 'sftp://1.1.1.1/backups', 'key' => 'backup', 'secret' => 'password', 'sftp_host_key' => 'ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAILhw/KsYTF9TQOwQX1EAM3gI5oxDS9Daz5nNuCixSRXT']); return $storage; } diff --git a/tests/Unit/S3ReplicaDownloadTest.php b/tests/Unit/S3ReplicaDownloadTest.php index f78fbc06f4..b7ebe4726d 100644 --- a/tests/Unit/S3ReplicaDownloadTest.php +++ b/tests/Unit/S3ReplicaDownloadTest.php @@ -153,7 +153,7 @@ it('streams an SFTP replica to the response without a temporary copy, at most th public function disconnect(): void {} }); $storage = new S3Storage; - $storage->forceFill(['type' => 'sftp', 'endpoint' => 'sftp://1.1.1.1/backups', 'key' => 'backup', 'secret' => 'password']); + $storage->forceFill(['type' => 'sftp', 'endpoint' => 'sftp://1.1.1.1/backups', 'key' => 'backup', 'secret' => 'password', 'sftp_host_key' => 'ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAILhw/KsYTF9TQOwQX1EAM3gI5oxDS9Daz5nNuCixSRXT']); $response = replicaDownloadTestExecution([$storage])->s3Download(1, 1);