From 5443d4fa5a67a9078643a32d8cf927b711767b4d Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 26 Oct 2026 09:05:28 +0100 Subject: [PATCH] fix(ssh): bound mux control checks and replace wedged masters (#12241) Co-authored-by: Claude Opus 5.5 --- app/Helpers/SshMultiplexingHelper.php | 69 +++++- .../CleanupStaleMultiplexedConnections.php | 18 +- tests/Feature/SshMultiplexingLockTest.php | 209 ++++++++++++++++++ 3 files changed, 274 insertions(+), 22 deletions(-) diff --git a/app/Helpers/SshMultiplexingHelper.php b/app/Helpers/SshMultiplexingHelper.php index 77738670b2..86034b8cbd 100644 --- a/app/Helpers/SshMultiplexingHelper.php +++ b/app/Helpers/SshMultiplexingHelper.php @@ -5,7 +5,10 @@ namespace App\Helpers; use App\Models\PrivateKey; use App\Models\Server; use Illuminate\Contracts\Cache\LockTimeoutException; +use Illuminate\Contracts\Process\ProcessResult; +use Illuminate\Process\Exceptions\ProcessTimedOutException; use Illuminate\Support\Facades\Cache; +use Illuminate\Support\Facades\File; use Illuminate\Support\Facades\Hash; use Illuminate\Support\Facades\Log; use Illuminate\Support\Facades\Process; @@ -42,10 +45,17 @@ class SshMultiplexingHelper return true; } - if (self::masterConnectionExists($server)) { + $check = self::runMuxControlCommand($server, 'check'); + if ($check?->successful()) { return self::refreshMultiplexedConnection($server); } + // A wedged master accepts on its socket but never answers, and a new + // `ssh -fN` would hang on it too. Remove the socket so a fresh master can bind. + if ($check === null) { + File::delete(self::muxSocket($server)); + } + return self::establishNewMultiplexedConnection($server); }); } catch (LockTimeoutException) { @@ -104,8 +114,8 @@ class SshMultiplexingHelper public static function removeMuxFile(Server $server): void { - $checkProcess = Process::run(self::muxControlCommand($server, 'check')); - $pid = preg_match('/pid=(\d+)/', $checkProcess->output().$checkProcess->errorOutput(), $matches) + $checkProcess = self::runMuxControlCommand($server, 'check'); + $pid = $checkProcess && preg_match('/pid=(\d+)/', $checkProcess->output().$checkProcess->errorOutput(), $matches) ? $matches[1] : null; @@ -113,11 +123,19 @@ class SshMultiplexingHelper self::markMuxProcessAsRetiring($pid, self::muxSocket($server)); } - $stopProcess = Process::run(self::muxControlCommand($server, 'stop')); + $stopProcess = self::runMuxControlCommand($server, 'stop'); - if ($pid !== null && ! $stopProcess->successful()) { + if ($stopProcess?->successful()) { + return; + } + + if ($pid !== null) { self::unmarkMuxProcessAsRetiring($pid, self::muxSocket($server)); } + + // A master that does not stop must not stay reusable under the old user, IP, or port. + // Without its socket it is an orphan, and the cleanup job reaps it. + File::delete(self::muxSocket($server)); } public static function markMuxProcessAsRetiring(string $pid, string $muxSocket, ?string $processStartTime = null): void @@ -300,17 +318,17 @@ class SshMultiplexingHelper { $sshConfig = self::serverSshConfiguration($server); $muxSocket = $sshConfig['muxFilename']; - $healthCheckTimeout = config('constants.ssh.mux_health_check_timeout'); - - $healthCommand = "timeout $healthCheckTimeout ssh -o ControlMaster=auto -o ControlPath=".escapeshellarg($muxSocket).' '; + $healthCommand = 'ssh -o ControlMaster=auto -o ControlPath='.escapeshellarg($muxSocket).' '; if (data_get($server, 'settings.is_cloudflare_tunnel')) { $healthCommand .= '-o ProxyCommand="cloudflared access ssh --hostname %h" '; } $healthCommand .= self::escapedUserAtHost($server)." 'echo \"health_check_ok\"'"; - $process = Process::run($healthCommand); + // The health check goes over the network, so a slow link gets at least the server's connection timeout. + $timeout = max((int) config('constants.ssh.mux_health_check_timeout'), self::getConnectionTimeout($server)); + $process = self::runWithTimeout($healthCommand, $timeout); - return $process->exitCode() === 0 && str_contains($process->output(), 'health_check_ok'); + return $process?->exitCode() === 0 && str_contains($process->output(), 'health_check_ok'); } public static function refreshMultiplexedConnection(Server $server): bool @@ -349,7 +367,36 @@ class SshMultiplexingHelper private static function masterConnectionExists(Server $server): bool { - return Process::run(self::muxControlCommand($server, 'check'))->exitCode() === 0; + return self::runMuxControlCommand($server, 'check')?->successful() === true; + } + + /** + * Run `ssh -O ` against a mux socket file. Returns null when the master does not answer in time. + */ + public static function runMuxSocketCommand(string $muxSocket, string $operation): ?ProcessResult + { + return self::runWithTimeout( + "ssh -O {$operation} -o ControlPath=".escapeshellarg($muxSocket).' localhost', + (int) config('constants.ssh.mux_health_check_timeout'), + ); + } + + private static function runMuxControlCommand(Server $server, string $operation): ?ProcessResult + { + return self::runWithTimeout(self::muxControlCommand($server, $operation), (int) config('constants.ssh.mux_health_check_timeout')); + } + + /** + * Use a PHP-side timeout, not the `timeout` binary: BusyBox `timeout` in the Coolify image + * kills ssh with SIGTERM, and Symfony turns that into an exception. Returns null on timeout. + */ + private static function runWithTimeout(string $command, int $timeout): ?ProcessResult + { + try { + return Process::timeout($timeout)->run($command); + } catch (ProcessTimedOutException) { + return null; + } } private static function connectionIsReusable(Server $server): bool diff --git a/app/Jobs/CleanupStaleMultiplexedConnections.php b/app/Jobs/CleanupStaleMultiplexedConnections.php index b03b3eab49..d52790e8c4 100644 --- a/app/Jobs/CleanupStaleMultiplexedConnections.php +++ b/app/Jobs/CleanupStaleMultiplexedConnections.php @@ -81,9 +81,9 @@ class CleanupStaleMultiplexedConnections implements ShouldQueue return null; } - $check = Process::run('timeout 10 ssh -O check -o ControlPath='.escapeshellarg($muxSocket).' localhost'); + $check = SshMultiplexingHelper::runMuxSocketCommand($muxSocket, 'check'); - return preg_match('/pid=(\d+)/', $check->output().$check->errorOutput(), $matches) + return $check && preg_match('/pid=(\d+)/', $check->output().$check->errorOutput(), $matches) ? $matches[1] : false; } @@ -227,10 +227,7 @@ class CleanupStaleMultiplexedConnections implements ShouldQueue } $muxSocket = "/var/www/html/storage/app/ssh/mux/{$muxFile}"; - $checkCommand = 'ssh -O check -o ControlPath='.escapeshellarg($muxSocket).' '.escapeshellarg($server->user).'@'.escapeshellarg($server->ip).' 2>/dev/null'; - $checkProcess = Process::run($checkCommand); - - if ($checkProcess->exitCode() !== 0) { + if (! SshMultiplexingHelper::runMuxSocketCommand($muxSocket, 'check')?->successful()) { $this->removeMultiplexFile($muxFile, 'connection_check_failed'); } } @@ -272,8 +269,8 @@ class CleanupStaleMultiplexedConnections implements ShouldQueue } $muxSocket = "/var/www/html/storage/app/ssh/mux/{$muxFile}"; - $checkProcess = Process::run('ssh -O check -o ControlPath='.escapeshellarg($muxSocket).' localhost'); - $pid = preg_match('/pid=(\d+)/', $checkProcess->output().$checkProcess->errorOutput(), $matches) + $checkProcess = SshMultiplexingHelper::runMuxSocketCommand($muxSocket, 'check'); + $pid = $checkProcess && preg_match('/pid=(\d+)/', $checkProcess->output().$checkProcess->errorOutput(), $matches) ? $matches[1] : null; @@ -281,9 +278,8 @@ class CleanupStaleMultiplexedConnections implements ShouldQueue SshMultiplexingHelper::markMuxProcessAsRetiring($pid, $muxSocket); } - $closeCommand = 'ssh -O stop -o ControlPath='.escapeshellarg($muxSocket).' localhost 2>/dev/null'; - $stopProcess = Process::run($closeCommand); - if ($pid !== null && ! $stopProcess->successful()) { + $stopProcess = SshMultiplexingHelper::runMuxSocketCommand($muxSocket, 'stop'); + if ($pid !== null && ! $stopProcess?->successful()) { SshMultiplexingHelper::unmarkMuxProcessAsRetiring($pid, $muxSocket); } Storage::disk('ssh-mux')->delete($muxFile); diff --git a/tests/Feature/SshMultiplexingLockTest.php b/tests/Feature/SshMultiplexingLockTest.php index 9e712bbf28..7f01a543f1 100644 --- a/tests/Feature/SshMultiplexingLockTest.php +++ b/tests/Feature/SshMultiplexingLockTest.php @@ -6,10 +6,14 @@ use App\Models\PrivateKey; use App\Models\Server; use App\Models\User; use Illuminate\Foundation\Testing\RefreshDatabase; +use Illuminate\Process\Exceptions\ProcessTimedOutException; +use Illuminate\Process\ProcessResult; use Illuminate\Support\Facades\Cache; use Illuminate\Support\Facades\File; use Illuminate\Support\Facades\Process; use Illuminate\Support\Facades\Storage; +use Symfony\Component\Process\Exception\ProcessTimedOutException as SymfonyTimeoutException; +use Symfony\Component\Process\Process as SymfonyProcess; /** * Tests for the explicit per-server mux lock that prevents concurrent workers @@ -55,6 +59,25 @@ function makeMuxServer(): Server return $server; } +/** + * A fake process that times out. Laravel does not record a fake that throws, so the + * timeout of each call goes into $timeouts. + * + * @param array $timeouts + */ +function muxProcessTimeout(array &$timeouts = []): Closure +{ + return function ($pendingProcess) use (&$timeouts) { + $timeouts[] = $pendingProcess->timeout; + $process = new SymfonyProcess(['ssh']); + + throw new ProcessTimedOutException( + new SymfonyTimeoutException($process, SymfonyTimeoutException::TYPE_GENERAL), + new ProcessResult($process), + ); + }; +} + it('establishes a master with ssh -fN and never the orphan-prone ssh -fNM', function () { config(['constants.ssh.mux_enabled' => true]); $server = makeMuxServer(); @@ -105,6 +128,113 @@ it('reuses a healthy master regardless of its absolute age', function () { Process::assertNotRan(fn ($process) => str_contains($process->command, 'ssh -fN ')); }); +it('bounds mux check commands so a wedged master cannot block callers', function () { + config([ + 'constants.ssh.mux_enabled' => true, + 'constants.ssh.mux_health_check_timeout' => 5, + ]); + $server = makeMuxServer(); + + Process::fake([ + '*-O check*' => Process::result(exitCode: 1), + '*-fN *' => Process::result(exitCode: 0), + ]); + + SshMultiplexingHelper::ensureMultiplexedConnection($server); + + Process::assertRan(fn ($process) => str_contains($process->command, '-O check')); + Process::assertNotRan(fn ($process) => str_contains($process->command, '-O check') && $process->timeout !== 5); +}); + +it('replaces a wedged master whose control check times out', function () { + config([ + 'constants.ssh.mux_enabled' => true, + 'constants.ssh.mux_health_check_timeout' => 5, + ]); + $server = makeMuxServer(); + $muxSocket = "/var/www/html/storage/app/ssh/mux/mux_{$server->uuid}"; + + File::partialMock()->shouldReceive('delete')->once()->with($muxSocket)->andReturnTrue(); + $timeouts = []; + Process::fake([ + '*-O check*' => muxProcessTimeout($timeouts), + '*-fN *' => Process::result(exitCode: 0), + ]); + + expect(SshMultiplexingHelper::ensureMultiplexedConnection($server))->toBeTrue() + ->and($timeouts)->each->toBe(5); + + Process::assertRan(fn ($process) => str_contains($process->command, 'ssh -fN ')); + Process::assertNotRan(fn ($process) => str_contains($process->command, '-O stop')); +}); + +it('does not delete the control socket when the master is simply absent', function () { + config(['constants.ssh.mux_enabled' => true]); + $server = makeMuxServer(); + + File::partialMock()->shouldNotReceive('delete'); + Process::fake([ + '*-O check*' => Process::result(exitCode: 255), + '*-fN *' => Process::result(exitCode: 0), + ]); + + expect(SshMultiplexingHelper::ensureMultiplexedConnection($server))->toBeTrue(); +}); + +it('bounds the control commands used to stop a mux master', function () { + config(['constants.ssh.mux_health_check_timeout' => 5]); + $server = makeMuxServer(); + Process::fake([ + '*-O check*' => Process::result(output: 'Master running (pid=222)', exitCode: 0), + '*-O stop*' => Process::result(exitCode: 0), + ]); + + SshMultiplexingHelper::removeMuxFile($server); + + Process::assertRan(fn ($process) => str_contains($process->command, 'ssh -O check') && $process->timeout === 5); + Process::assertRan(fn ($process) => str_contains($process->command, 'ssh -O stop') && $process->timeout === 5); +}); + +it('replaces a master whose health check times out', function () { + config([ + 'constants.ssh.mux_enabled' => true, + 'constants.ssh.mux_health_check_enabled' => true, + ]); + $server = makeMuxServer(); + + Process::fake([ + '*-O check*' => Process::result(output: 'Master running (pid=222)', exitCode: 0), + '*health_check_ok*' => muxProcessTimeout(), + '*-O stop*' => Process::result(exitCode: 0), + '*-fN *' => Process::result(exitCode: 0), + ]); + + expect(SshMultiplexingHelper::ensureMultiplexedConnection($server))->toBeTrue(); + + Process::assertRan(fn ($process) => str_contains($process->command, 'ssh -O stop')); + Process::assertRan(fn ($process) => str_contains($process->command, 'ssh -fN ')); +}); + +it('gives the mux health check at least the server connection timeout', function () { + config([ + 'constants.ssh.mux_enabled' => true, + 'constants.ssh.mux_health_check_enabled' => true, + 'constants.ssh.mux_health_check_timeout' => 5, + ]); + $server = makeMuxServer(); + $server->settings->update(['connection_timeout' => 30]); + + Process::fake([ + '*-O check*' => Process::result(exitCode: 0), + '*health_check_ok*' => Process::result(output: 'health_check_ok', exitCode: 0), + ]); + + expect(SshMultiplexingHelper::ensureMultiplexedConnection($server->fresh()))->toBeTrue(); + + Process::assertRan(fn ($process) => str_contains($process->command, 'health_check_ok') && $process->timeout === 30); + Process::assertNotRan(fn ($process) => str_contains($process->command, '-O check') && $process->timeout !== 5); +}); + it('does not spawn a master when the per-server lock is already held', function () { config([ 'constants.ssh.mux_enabled' => true, @@ -418,6 +548,33 @@ it('does not mark a mux process as retiring when stop fails', function () { expect(SshMultiplexingHelper::isMuxProcessRetiring('444', "/var/www/html/storage/app/ssh/mux/mux_{$server->uuid}"))->toBeFalse(); }); +it('removes the control socket when the mux master does not stop', function (bool $stopTimesOut) { + $server = makeMuxServer(); + File::partialMock()->shouldReceive('delete')->once()->with("/var/www/html/storage/app/ssh/mux/mux_{$server->uuid}"); + Process::fake([ + '*-O check*' => Process::result(output: 'Master running (pid=888)', exitCode: 0), + '*-O stop*' => $stopTimesOut ? muxProcessTimeout() : Process::result(exitCode: 255), + ]); + + SshMultiplexingHelper::removeMuxFile($server); + + expect(SshMultiplexingHelper::isMuxProcessRetiring('888', "/var/www/html/storage/app/ssh/mux/mux_{$server->uuid}"))->toBeFalse(); +})->with([ + 'stop fails' => false, + 'stop times out' => true, +]); + +it('keeps the control socket untouched when the mux master stops', function () { + $server = makeMuxServer(); + File::partialMock()->shouldNotReceive('delete'); + Process::fake([ + '*-O check*' => Process::result(output: 'Master running (pid=999)', exitCode: 0), + '*-O stop*' => Process::result(exitCode: 0), + ]); + + SshMultiplexingHelper::removeMuxFile($server); +}); + it('kills only old cloudflared proxies whose stdin pipe no live ssh holds', function () { config(['constants.ssh.mux_orphan_reap_enabled' => true]); @@ -544,6 +701,58 @@ it('removes a stale mux retirement marker when stopping fails', function () { expect(SshMultiplexingHelper::isMuxProcessRetiring('777', $muxSocket))->toBeFalse(); }); +it('removes a mux connection whose control check times out during cleanup', function () { + config([ + 'constants.ssh.mux_orphan_reap_enabled' => true, + 'constants.ssh.mux_health_check_timeout' => 5, + ]); + Storage::fake('ssh-mux'); + $server = makeMuxServer(); + $file = "mux_{$server->uuid}"; + Storage::disk('ssh-mux')->put($file, 'x'); + + $timeouts = []; + Process::fake([ + '*-O check*' => muxProcessTimeout($timeouts), + '*-O stop*' => muxProcessTimeout($timeouts), + ]); + + $job = new CleanupStaleMultiplexedConnections; + $method = new ReflectionMethod($job, 'cleanupStaleConnections'); + $method->invoke($job); + + expect(Storage::disk('ssh-mux')->exists($file))->toBeFalse() + ->and($timeouts)->toBe([5, 5, 5]); + Process::assertNotRan(fn ($process) => str_contains($process->command, $server->ip)); +}); + +it('does not reap ssh processes when the socket owner check times out', function () { + config([ + 'constants.ssh.mux_orphan_reap_enabled' => true, + 'constants.ssh.mux_health_check_timeout' => 5, + ]); + $muxDir = storage_path('app/ssh/mux'); + File::ensureDirectoryExists($muxDir); + $socket = $muxDir.'/mux_wedged_'.uniqid(); + File::put($socket, 'x'); + + $timeouts = []; + Process::fake([ + 'ps*' => Process::result(output: "111 1 5000 ssh -fN -o ControlMaster=auto -o ControlPath={$socket} root@1.2.3.4\n"), + '*ssh -O check*' => muxProcessTimeout($timeouts), + 'kill*' => Process::result(exitCode: 0), + ]); + + $job = new CleanupStaleMultiplexedConnections; + $method = new ReflectionMethod($job, 'cleanupOrphanedSshProcesses'); + $method->invoke($job); + + expect($timeouts)->toBe([5]); + Process::assertNotRan(fn ($process) => str_contains($process->command, 'kill')); + + File::delete($socket); +}); + it('does not remove a healthy mux connection based on its absolute age', function () { config(['constants.ssh.mux_orphan_reap_enabled' => true]); Storage::fake('ssh-mux');