diff --git a/app/Helpers/SshMultiplexingHelper.php b/app/Helpers/SshMultiplexingHelper.php index 54d5714a6..aa9d06996 100644 --- a/app/Helpers/SshMultiplexingHelper.php +++ b/app/Helpers/SshMultiplexingHelper.php @@ -8,6 +8,7 @@ use Illuminate\Support\Facades\Hash; use Illuminate\Support\Facades\Log; use Illuminate\Support\Facades\Process; +use Illuminate\Support\Facades\Storage; class SshMultiplexingHelper { @@ -209,12 +210,37 @@ private static function isMultiplexingEnabled(): bool private static function validateSshKey(PrivateKey $privateKey): void { $keyLocation = $privateKey->getKeyLocation(); - $checkKeyCommand = "ls $keyLocation 2>/dev/null"; - $keyCheckProcess = Process::run($checkKeyCommand); + $filename = "ssh_key@{$privateKey->uuid}"; + $disk = Storage::disk('ssh-keys'); - if ($keyCheckProcess->exitCode() !== 0) { + $needsRewrite = false; + + if (! $disk->exists($filename)) { + $needsRewrite = true; + } else { + $diskContent = $disk->get($filename); + if ($diskContent !== $privateKey->private_key) { + Log::warning('SSH key file content does not match database, resyncing', [ + 'key_uuid' => $privateKey->uuid, + ]); + $needsRewrite = true; + } + } + + if ($needsRewrite) { $privateKey->storeInFileSystem(); } + + // Ensure correct permissions (SSH requires 0600) + if (file_exists($keyLocation)) { + $currentPerms = fileperms($keyLocation) & 0777; + if ($currentPerms !== 0600 && ! chmod($keyLocation, 0600)) { + Log::warning('Failed to set SSH key file permissions to 0600', [ + 'key_uuid' => $privateKey->uuid, + 'path' => $keyLocation, + ]); + } + } } private static function getCommonSshOptions(Server $server, string $sshKeyLocation, int $connectionTimeout, int $serverInterval, bool $isScp = false): string diff --git a/app/Models/PrivateKey.php b/app/Models/PrivateKey.php index 7163ae7b5..1521678f3 100644 --- a/app/Models/PrivateKey.php +++ b/app/Models/PrivateKey.php @@ -5,6 +5,7 @@ use App\Traits\HasSafeStringAttribute; use DanHarrin\LivewireRateLimiting\WithRateLimiting; use Illuminate\Support\Facades\DB; +use Illuminate\Support\Facades\Log; use Illuminate\Support\Facades\Storage; use Illuminate\Validation\ValidationException; use OpenApi\Attributes as OA; @@ -65,6 +66,20 @@ protected static function booted() } }); + static::saved(function ($key) { + if ($key->wasChanged('private_key')) { + try { + $key->storeInFileSystem(); + refresh_server_connection($key); + } catch (\Exception $e) { + Log::error('Failed to resync SSH key after update', [ + 'key_uuid' => $key->uuid, + 'error' => $e->getMessage(), + ]); + } + } + }); + static::deleted(function ($key) { self::deleteFromStorage($key); }); @@ -185,29 +200,54 @@ public function storeInFileSystem() { $filename = "ssh_key@{$this->uuid}"; $disk = Storage::disk('ssh-keys'); + $keyLocation = $this->getKeyLocation(); + $lockFile = $keyLocation.'.lock'; // Ensure the storage directory exists and is writable $this->ensureStorageDirectoryExists(); - // Attempt to store the private key - $success = $disk->put($filename, $this->private_key); - - if (! $success) { - throw new \Exception("Failed to write SSH key to filesystem. Check disk space and permissions for: {$this->getKeyLocation()}"); + // Use file locking to prevent concurrent writes from corrupting the key + $lockHandle = fopen($lockFile, 'c'); + if ($lockHandle === false) { + throw new \Exception("Failed to open lock file for SSH key: {$lockFile}"); } - // Verify the file was actually created and has content - if (! $disk->exists($filename)) { - throw new \Exception("SSH key file was not created: {$this->getKeyLocation()}"); - } + try { + if (! flock($lockHandle, LOCK_EX)) { + throw new \Exception("Failed to acquire lock for SSH key: {$keyLocation}"); + } - $storedContent = $disk->get($filename); - if (empty($storedContent) || $storedContent !== $this->private_key) { - $disk->delete($filename); // Clean up the bad file - throw new \Exception("SSH key file content verification failed: {$this->getKeyLocation()}"); - } + // Attempt to store the private key + $success = $disk->put($filename, $this->private_key); - return $this->getKeyLocation(); + if (! $success) { + throw new \Exception("Failed to write SSH key to filesystem. Check disk space and permissions for: {$keyLocation}"); + } + + // Verify the file was actually created and has content + if (! $disk->exists($filename)) { + throw new \Exception("SSH key file was not created: {$keyLocation}"); + } + + $storedContent = $disk->get($filename); + if (empty($storedContent) || $storedContent !== $this->private_key) { + $disk->delete($filename); // Clean up the bad file + throw new \Exception("SSH key file content verification failed: {$keyLocation}"); + } + + // Ensure correct permissions for SSH (0600 required) + if (file_exists($keyLocation) && ! chmod($keyLocation, 0600)) { + Log::warning('Failed to set SSH key file permissions to 0600', [ + 'key_uuid' => $this->uuid, + 'path' => $keyLocation, + ]); + } + + return $keyLocation; + } finally { + flock($lockHandle, LOCK_UN); + fclose($lockHandle); + } } public static function deleteFromStorage(self $privateKey) @@ -254,12 +294,6 @@ public function updatePrivateKey(array $data) return DB::transaction(function () use ($data) { $this->update($data); - try { - $this->storeInFileSystem(); - } catch (\Exception $e) { - throw new \Exception('Failed to update SSH key: '.$e->getMessage()); - } - return $this; }); } diff --git a/app/Models/Server.php b/app/Models/Server.php index 508b9833b..527c744a5 100644 --- a/app/Models/Server.php +++ b/app/Models/Server.php @@ -135,7 +135,7 @@ protected static function booted() $server->forceFill($payload); }); static::saved(function ($server) { - if ($server->privateKey?->isDirty()) { + if ($server->wasChanged('private_key_id') || $server->privateKey?->isDirty()) { refresh_server_connection($server->privateKey); } }); diff --git a/tests/Unit/SshKeyValidationTest.php b/tests/Unit/SshKeyValidationTest.php new file mode 100644 index 000000000..adc6847d1 --- /dev/null +++ b/tests/Unit/SshKeyValidationTest.php @@ -0,0 +1,208 @@ +diskRoot = sys_get_temp_dir().'/coolify-ssh-test-'.Str::uuid(); + File::ensureDirectoryExists($this->diskRoot); + config(['filesystems.disks.ssh-keys.root' => $this->diskRoot]); + app('filesystem')->forgetDisk('ssh-keys'); + } + + protected function tearDown(): void + { + File::deleteDirectory($this->diskRoot); + parent::tearDown(); + } + + private function makePrivateKey(string $keyContent = 'TEST_KEY_CONTENT'): PrivateKey + { + $privateKey = new class extends PrivateKey + { + public int $storeCallCount = 0; + + public function refresh() + { + return $this; + } + + public function getKeyLocation() + { + return Storage::disk('ssh-keys')->path("ssh_key@{$this->uuid}"); + } + + public function storeInFileSystem() + { + $this->storeCallCount++; + $filename = "ssh_key@{$this->uuid}"; + $disk = Storage::disk('ssh-keys'); + $disk->put($filename, $this->private_key); + $keyLocation = $disk->path($filename); + chmod($keyLocation, 0600); + + return $keyLocation; + } + }; + + $privateKey->uuid = (string) Str::uuid(); + $privateKey->private_key = $keyContent; + + return $privateKey; + } + + private function callValidateSshKey(PrivateKey $privateKey): void + { + $reflection = new \ReflectionMethod(SshMultiplexingHelper::class, 'validateSshKey'); + $reflection->setAccessible(true); + $reflection->invoke(null, $privateKey); + } + + public function test_validate_ssh_key_rewrites_stale_file_and_fixes_permissions() + { + $privateKey = $this->makePrivateKey('NEW_PRIVATE_KEY_CONTENT'); + + $filename = "ssh_key@{$privateKey->uuid}"; + $disk = Storage::disk('ssh-keys'); + $disk->put($filename, 'OLD_PRIVATE_KEY_CONTENT'); + $keyPath = $disk->path($filename); + chmod($keyPath, 0644); + + $this->callValidateSshKey($privateKey); + + $this->assertSame('NEW_PRIVATE_KEY_CONTENT', $disk->get($filename)); + $this->assertSame(1, $privateKey->storeCallCount); + $this->assertSame(0600, fileperms($keyPath) & 0777); + } + + public function test_validate_ssh_key_creates_missing_file() + { + $privateKey = $this->makePrivateKey('MY_KEY_CONTENT'); + + $filename = "ssh_key@{$privateKey->uuid}"; + $disk = Storage::disk('ssh-keys'); + $this->assertFalse($disk->exists($filename)); + + $this->callValidateSshKey($privateKey); + + $this->assertTrue($disk->exists($filename)); + $this->assertSame('MY_KEY_CONTENT', $disk->get($filename)); + $this->assertSame(1, $privateKey->storeCallCount); + } + + public function test_validate_ssh_key_skips_rewrite_when_content_matches() + { + $privateKey = $this->makePrivateKey('SAME_KEY_CONTENT'); + + $filename = "ssh_key@{$privateKey->uuid}"; + $disk = Storage::disk('ssh-keys'); + $disk->put($filename, 'SAME_KEY_CONTENT'); + $keyPath = $disk->path($filename); + chmod($keyPath, 0600); + + $this->callValidateSshKey($privateKey); + + $this->assertSame(0, $privateKey->storeCallCount, 'Should not rewrite when content matches'); + $this->assertSame('SAME_KEY_CONTENT', $disk->get($filename)); + } + + public function test_validate_ssh_key_fixes_permissions_without_rewrite() + { + $privateKey = $this->makePrivateKey('KEY_CONTENT'); + + $filename = "ssh_key@{$privateKey->uuid}"; + $disk = Storage::disk('ssh-keys'); + $disk->put($filename, 'KEY_CONTENT'); + $keyPath = $disk->path($filename); + chmod($keyPath, 0644); + + $this->callValidateSshKey($privateKey); + + $this->assertSame(0, $privateKey->storeCallCount, 'Should not rewrite when content matches'); + $this->assertSame(0600, fileperms($keyPath) & 0777, 'Should fix permissions even without rewrite'); + } + + public function test_store_in_file_system_enforces_correct_permissions() + { + $privateKey = $this->makePrivateKey('KEY_FOR_PERM_TEST'); + $privateKey->storeInFileSystem(); + + $filename = "ssh_key@{$privateKey->uuid}"; + $keyPath = Storage::disk('ssh-keys')->path($filename); + + $this->assertSame(0600, fileperms($keyPath) & 0777); + } + + public function test_store_in_file_system_lock_file_persists() + { + // Use the real storeInFileSystem to verify lock file behavior + $disk = Storage::disk('ssh-keys'); + $uuid = (string) Str::uuid(); + $filename = "ssh_key@{$uuid}"; + $keyLocation = $disk->path($filename); + $lockFile = $keyLocation.'.lock'; + + $privateKey = new class extends PrivateKey + { + public function refresh() + { + return $this; + } + + public function getKeyLocation() + { + return Storage::disk('ssh-keys')->path("ssh_key@{$this->uuid}"); + } + + protected function ensureStorageDirectoryExists() + { + // No-op in test — directory already exists + } + }; + + $privateKey->uuid = $uuid; + $privateKey->private_key = 'LOCK_TEST_KEY'; + + $privateKey->storeInFileSystem(); + + // Lock file should persist (not be deleted) to prevent flock race conditions + $this->assertFileExists($lockFile, 'Lock file should persist after storeInFileSystem'); + } + + public function test_server_model_detects_private_key_id_changes() + { + $reflection = new \ReflectionMethod(\App\Models\Server::class, 'booted'); + $filename = $reflection->getFileName(); + $startLine = $reflection->getStartLine(); + $endLine = $reflection->getEndLine(); + $source = implode('', array_slice(file($filename), $startLine - 1, $endLine - $startLine + 1)); + + $this->assertStringContainsString( + "wasChanged('private_key_id')", + $source, + 'Server saved event should detect private_key_id changes' + ); + } +}