From 6325e41aec29e5e02e16f0b8df0dbc1ae5b38531 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 16 Mar 2026 21:27:10 +0100 Subject: [PATCH] fix(ssh): handle chmod failures gracefully and simplify key management - Log warnings instead of silently failing when chmod 0600 fails - Remove redundant refresh() call before SSH key validation - Remove storeInFileSystem() call from updatePrivateKey() transaction - Remove @unlink() of lock file after filesystem store - Refactor unit tests to use real temp disk and anonymous class stub instead of reflection-only checks --- app/Helpers/SshMultiplexingHelper.php | 9 +- app/Models/PrivateKey.php | 17 +- tests/Unit/SshKeyValidationTest.php | 267 ++++++++++++++------------ 3 files changed, 155 insertions(+), 138 deletions(-) diff --git a/app/Helpers/SshMultiplexingHelper.php b/app/Helpers/SshMultiplexingHelper.php index b9a3e98f3..aa9d06996 100644 --- a/app/Helpers/SshMultiplexingHelper.php +++ b/app/Helpers/SshMultiplexingHelper.php @@ -209,8 +209,6 @@ private static function isMultiplexingEnabled(): bool private static function validateSshKey(PrivateKey $privateKey): void { - $privateKey->refresh(); - $keyLocation = $privateKey->getKeyLocation(); $filename = "ssh_key@{$privateKey->uuid}"; $disk = Storage::disk('ssh-keys'); @@ -236,8 +234,11 @@ private static function validateSshKey(PrivateKey $privateKey): void // Ensure correct permissions (SSH requires 0600) if (file_exists($keyLocation)) { $currentPerms = fileperms($keyLocation) & 0777; - if ($currentPerms !== 0600) { - chmod($keyLocation, 0600); + if ($currentPerms !== 0600 && ! chmod($keyLocation, 0600)) { + Log::warning('Failed to set SSH key file permissions to 0600', [ + 'key_uuid' => $privateKey->uuid, + 'path' => $keyLocation, + ]); } } } diff --git a/app/Models/PrivateKey.php b/app/Models/PrivateKey.php index b453e999d..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; @@ -71,7 +72,7 @@ protected static function booted() $key->storeInFileSystem(); refresh_server_connection($key); } catch (\Exception $e) { - \Illuminate\Support\Facades\Log::error('Failed to resync SSH key after update', [ + Log::error('Failed to resync SSH key after update', [ 'key_uuid' => $key->uuid, 'error' => $e->getMessage(), ]); @@ -235,15 +236,17 @@ public function storeInFileSystem() } // Ensure correct permissions for SSH (0600 required) - if (file_exists($keyLocation)) { - chmod($keyLocation, 0600); + 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); - @unlink($lockFile); } } @@ -291,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/tests/Unit/SshKeyValidationTest.php b/tests/Unit/SshKeyValidationTest.php index fbcf7725d..adc6847d1 100644 --- a/tests/Unit/SshKeyValidationTest.php +++ b/tests/Unit/SshKeyValidationTest.php @@ -20,126 +20,26 @@ */ class SshKeyValidationTest extends TestCase { - public function test_validate_ssh_key_method_exists() + private string $diskRoot; + + protected function setUp(): void { - $reflection = new \ReflectionMethod(SshMultiplexingHelper::class, 'validateSshKey'); - $this->assertTrue($reflection->isStatic(), 'validateSshKey should be a static method'); - } + parent::setUp(); - public function test_validate_ssh_key_accepts_private_key_parameter() - { - $reflection = new \ReflectionMethod(SshMultiplexingHelper::class, 'validateSshKey'); - $parameters = $reflection->getParameters(); - - $this->assertCount(1, $parameters); - $this->assertEquals('privateKey', $parameters[0]->getName()); - - $type = $parameters[0]->getType(); - $this->assertNotNull($type); - $this->assertEquals(PrivateKey::class, $type->getName()); - } - - public function test_store_in_file_system_sets_correct_permissions() - { - // Verify that storeInFileSystem enforces chmod 0600 via code inspection - $reflection = new \ReflectionMethod(PrivateKey::class, 'storeInFileSystem'); - $this->assertTrue( - $reflection->isPublic(), - 'storeInFileSystem should be public' - ); - - // Verify the method source contains chmod for 0600 - $filename = $reflection->getFileName(); - $startLine = $reflection->getStartLine(); - $endLine = $reflection->getEndLine(); - $source = implode('', array_slice(file($filename), $startLine - 1, $endLine - $startLine + 1)); - - $this->assertStringContainsString('chmod', $source, 'storeInFileSystem should set file permissions'); - $this->assertStringContainsString('0600', $source, 'storeInFileSystem should enforce 0600 permissions'); - } - - public function test_store_in_file_system_uses_file_locking() - { - // Verify the method uses flock to prevent race conditions - $reflection = new \ReflectionMethod(PrivateKey::class, 'storeInFileSystem'); - $filename = $reflection->getFileName(); - $startLine = $reflection->getStartLine(); - $endLine = $reflection->getEndLine(); - $source = implode('', array_slice(file($filename), $startLine - 1, $endLine - $startLine + 1)); - - $this->assertStringContainsString('flock', $source, 'storeInFileSystem should use file locking'); - $this->assertStringContainsString('LOCK_EX', $source, 'storeInFileSystem should use exclusive locks'); - } - - public function test_validate_ssh_key_checks_content_not_just_existence() - { - // Verify validateSshKey compares file content with DB value - $reflection = new \ReflectionMethod(SshMultiplexingHelper::class, 'validateSshKey'); - $filename = $reflection->getFileName(); - $startLine = $reflection->getStartLine(); - $endLine = $reflection->getEndLine(); - $source = implode('', array_slice(file($filename), $startLine - 1, $endLine - $startLine + 1)); - - // Should read file content and compare, not just check existence with `ls` - $this->assertStringNotContainsString('ls $keyLocation', $source, 'Should not use ls to check key existence'); - $this->assertStringContainsString('private_key', $source, 'Should compare against DB key content'); - $this->assertStringContainsString('refresh', $source, 'Should refresh key from database'); - } - - public function test_server_model_detects_private_key_id_changes() - { - // Verify the Server model's saved event checks for 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', - $source, - 'Server saved event should detect private_key_id changes via wasChanged()' - ); - $this->assertStringContainsString( - 'private_key_id', - $source, - 'Server saved event should specifically check private_key_id' - ); - } - - public function test_private_key_saved_event_resyncs_on_key_change() - { - // Verify PrivateKey model resyncs file and mux on key content change - $reflection = new \ReflectionMethod(PrivateKey::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')", - $source, - 'PrivateKey saved event should detect key content changes' - ); - $this->assertStringContainsString( - 'refresh_server_connection', - $source, - 'PrivateKey saved event should invalidate mux connections' - ); - $this->assertStringContainsString( - 'storeInFileSystem', - $source, - 'PrivateKey saved event should resync key file' - ); - } - - public function test_validate_ssh_key_rewrites_stale_file_and_fixes_permissions() - { - $diskRoot = sys_get_temp_dir().'/coolify-ssh-test-'.Str::uuid(); - File::ensureDirectoryExists($diskRoot); - config(['filesystems.disks.ssh-keys.root' => $diskRoot]); + $this->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; @@ -168,22 +68,141 @@ public function storeInFileSystem() }; $privateKey->uuid = (string) Str::uuid(); - $privateKey->private_key = 'NEW_PRIVATE_KEY_CONTENT'; + $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'); - $staleKeyPath = $disk->path($filename); - chmod($staleKeyPath, 0644); + $keyPath = $disk->path($filename); + chmod($keyPath, 0644); - $reflection = new \ReflectionMethod(SshMultiplexingHelper::class, 'validateSshKey'); - $reflection->setAccessible(true); - $reflection->invoke(null, $privateKey); + $this->callValidateSshKey($privateKey); $this->assertSame('NEW_PRIVATE_KEY_CONTENT', $disk->get($filename)); $this->assertSame(1, $privateKey->storeCallCount); - $this->assertSame(0600, fileperms($staleKeyPath) & 0777); + $this->assertSame(0600, fileperms($keyPath) & 0777); + } - File::deleteDirectory($diskRoot); + 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' + ); } }