From 4bf94fac2d2c006ed2d6c2760d6001543cda06de Mon Sep 17 00:00:00 2001 From: pannous Date: Sun, 15 Mar 2026 03:06:21 +0100 Subject: [PATCH] fix: prevent sporadic SSH permission denied by validating key content The root cause of sporadic "Permission denied (publickey)" errors was that validateSshKey() only checked if the key file existed on disk, never verifying its content matched the database. When keys were rotated or updated, the stale file persisted and SSH used the wrong key. Changes: - validateSshKey() now refreshes key from DB and compares file content - Server saved event detects private_key_id changes to invalidate mux - PrivateKey storeInFileSystem() uses file locking to prevent races - PrivateKey saved event auto-resyncs file on key content changes - Enforces 0600 permissions on key files Fixes coollabsio/coolify#7724 --- app/Helpers/SshMultiplexingHelper.php | 33 ++++++- app/Models/PrivateKey.php | 67 ++++++++++--- app/Models/Server.php | 2 +- tests/Unit/SshKeyValidationTest.php | 133 ++++++++++++++++++++++++++ 4 files changed, 215 insertions(+), 20 deletions(-) create mode 100644 tests/Unit/SshKeyValidationTest.php diff --git a/app/Helpers/SshMultiplexingHelper.php b/app/Helpers/SshMultiplexingHelper.php index 54d5714a6..b9a3e98f3 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 { @@ -208,13 +209,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); + $privateKey->refresh(); - if ($keyCheckProcess->exitCode() !== 0) { + $keyLocation = $privateKey->getKeyLocation(); + $filename = "ssh_key@{$privateKey->uuid}"; + $disk = Storage::disk('ssh-keys'); + + $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); + } + } } 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..b453e999d 100644 --- a/app/Models/PrivateKey.php +++ b/app/Models/PrivateKey.php @@ -65,6 +65,20 @@ protected static function booted() } }); + static::saved(function ($key) { + if ($key->wasChanged('private_key')) { + try { + $key->storeInFileSystem(); + refresh_server_connection($key); + } catch (\Exception $e) { + \Illuminate\Support\Facades\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 +199,52 @@ 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); + } + + return $keyLocation; + } finally { + flock($lockHandle, LOCK_UN); + fclose($lockHandle); + @unlink($lockFile); + } } public static function deleteFromStorage(self $privateKey) 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..9a6c6c04b --- /dev/null +++ b/tests/Unit/SshKeyValidationTest.php @@ -0,0 +1,133 @@ +assertTrue($reflection->isStatic(), 'validateSshKey should be a static method'); + } + + 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' + ); + } +}