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.

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) {

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)) {

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]);

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.');
     }
 

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());

resources/views/livewire/storage/form.blade.php

@@ -87,8 +87,8 @@
                 @if ($storage->type === App\Enums\BackupStorageType::Sftp)
                     <div class="flex items-end gap-2 lg:col-span-2">
                         <x-forms.input label="Host key fingerprint" disabled
-                            :value="$storage->sftpHostKeyFingerprint() ?? 'Not saved yet. The next connection test saves it.'"
-                            helper="Coolify saves the host key of the server on the first successful connection, and refuses the server when the key changes. Reset it only when you know why the key changed." />
+                            :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)
                                 <x-forms.button type="button" wire:click="resetHostKey">Reset host key</x-forms.button>

tests/Feature/DatabaseImportFormFileCheckAuthorizationTest.php

@@ -1,9 +1,11 @@
 <?php
 
+use App\Enums\BackupStorageType;
 use App\Livewire\Project\Database\ImportForm;
 use App\Models\Environment;
 use App\Models\InstanceSettings;
 use App\Models\Project;
+use App\Models\S3Storage;
 use App\Models\Server;
 use App\Models\StandaloneDocker;
 use App\Models\StandalonePostgresql;
@@ -11,6 +13,7 @@ use App\Models\Team;
 use App\Models\User;
 use Illuminate\Foundation\Testing\RefreshDatabase;
 use Illuminate\Support\Str;
+use League\Flysystem\PhpseclibV3\SftpConnectionProvider;
 use Livewire\Features\SupportTesting\Testable;
 use Livewire\Livewire;
 
@@ -102,3 +105,32 @@ it('does nothing when an admin checks an empty server path', function () {
         ->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();
+});

tests/Feature/SftpHostKeyPinningTest.php

@@ -1,11 +1,13 @@
 <?php
 
 use App\Enums\BackupStorageType;
+use App\Jobs\RevalidateUnusableS3StoragesJob;
 use App\Livewire\Storage\Form;
 use App\Models\InstanceSettings;
 use App\Models\S3Storage;
 use App\Models\Team;
 use App\Models\User;
+use Illuminate\Filesystem\FilesystemAdapter;
 use Illuminate\Foundation\Testing\RefreshDatabase;
 use League\Flysystem\PhpseclibV3\ConnectionProvider;
 use League\Flysystem\PhpseclibV3\SftpConnectionProvider;
@@ -80,7 +82,7 @@ it('saves the host key on the first successful connection', function () {
     fakeSftpServer('ssh-ed25519 '.explode(' ', SFTP_TEST_HOST_KEY)[1], parameters: $parameters);
     $storage = sftpHostKeyStorage($this->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);

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;
 }

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);