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