From 8f9c6d10a79bd38fa3dfc59daec5f9ddd3c392a8 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Thu, 15 Oct 2026 22:59:02 +0200 Subject: [PATCH] fix(ssh): reap mux orphans by socket ownership, not file existence Closes #11883 --- .../CleanupStaleMultiplexedConnections.php | 82 +++++++++++++--- docker/development/Dockerfile | 1 + docker/production/Dockerfile | 1 + tests/Feature/SshMultiplexingLockTest.php | 98 ++++++++++++++++--- 4 files changed, 156 insertions(+), 26 deletions(-) diff --git a/app/Jobs/CleanupStaleMultiplexedConnections.php b/app/Jobs/CleanupStaleMultiplexedConnections.php index 69e151d24f..3f0d275403 100644 --- a/app/Jobs/CleanupStaleMultiplexedConnections.php +++ b/app/Jobs/CleanupStaleMultiplexedConnections.php @@ -28,8 +28,14 @@ class CleanupStaleMultiplexedConnections implements ShouldQueue /** * Kill backgrounded ssh master processes that lost the ControlPath socket * race. Such processes are not masters, so ControlPersist never reaps them - * and they leak memory until the container restarts. A legitimate master - * always owns its socket file; an orphan has none. + * and they hold an authenticated session until the container restarts. + * + * Every `ssh -fN` for a server carries the same ControlPath, so the socket + * file existing says nothing about which process owns it. Ownership is read + * from the socket itself (`ssh -O check` reports the master pid); every + * other process for that ControlPath is an orphan. When the socket file is + * missing, no process owns it. When it exists but does not answer, the + * owner is unknown and nothing is reaped. * * Processes younger than the minimum age are skipped: a freshly forked * master creates its socket a few milliseconds after starting, so a young @@ -39,6 +45,7 @@ class CleanupStaleMultiplexedConnections implements ShouldQueue { $muxDir = storage_path('app/ssh/mux'); $minAge = (int) config('constants.ssh.mux_orphan_min_age'); + $socketOwners = []; foreach ($this->listProcesses() as $process) { // Backgrounded ssh master: current `ssh -fN` or legacy `ssh -fNM`. @@ -51,20 +58,44 @@ class CleanupStaleMultiplexedConnections implements ShouldQueue continue; } - if ($process['etimes'] >= $minAge - && ! file_exists($pathMatch[1]) - && ! SshMultiplexingHelper::isMuxProcessRetiring($process['pid'], $pathMatch[1])) { + $muxSocket = $pathMatch[1]; + if ($process['etimes'] < $minAge + || SshMultiplexingHelper::isMuxProcessRetiring($process['pid'], $muxSocket)) { + continue; + } + + $socketOwners[$muxSocket] ??= $this->muxSocketOwner($muxSocket); + if ($socketOwners[$muxSocket] !== false && $socketOwners[$muxSocket] !== $process['pid']) { $this->reapOrphan('ssh', $process); } } } + /** + * Pid of the master that owns a mux socket. Returns null when the socket + * file does not exist, and false when it exists but its owner is unknown. + */ + private function muxSocketOwner(string $muxSocket): string|false|null + { + if (! file_exists($muxSocket)) { + return null; + } + + $check = Process::run('timeout 10 ssh -O check -o ControlPath='.escapeshellarg($muxSocket).' localhost'); + + return preg_match('/pid=(\d+)/', $check->output().$check->errorOutput(), $matches) + ? $matches[1] + : false; + } + /** * Kill orphaned `cloudflared access ssh` proxy processes. Each is spawned * as the SSH ProxyCommand transport for a Cloudflare Tunnel server and must * die with its parent ssh. When that ssh is killed or orphaned (e.g. a lost * mux master), the cloudflared process can leak and accumulate. A legitimate - * proxy always has a live ssh parent; one without is safe to reap. + * proxy always has a live ssh at the other end of its stdin pipe. Its parent + * is not enough: `ssh -f` forks after connecting, so the proxy of a live + * master is reparented away from ssh. * * Processes younger than the minimum age are skipped so a proxy whose parent * ssh is still starting up, or a transient `ssh -O check` proxy mid-exit, is @@ -84,18 +115,44 @@ class CleanupStaleMultiplexedConnections implements ShouldQueue } foreach ($processes as $process) { - // `cloudflared access ssh`, never the `cloudflared tunnel` daemon. - if (! str_contains($process['args'], 'cloudflared access ssh')) { + // `cloudflared access ssh`, never the `cloudflared tunnel` daemon, nor + // an ssh whose ProxyCommand option contains the same words. + if (isset($sshPids[$process['pid']]) || ! str_contains($process['args'], 'cloudflared access ssh')) { continue; } - // Orphaned when no live ssh process is its parent. - if ($process['etimes'] >= $minAge && ! isset($sshPids[$process['ppid']])) { + if ($process['etimes'] >= $minAge + && ! isset($sshPids[$process['ppid']]) + && ! $this->stdinPipeHeldByAny($process['pid'], array_keys($sshPids))) { $this->reapOrphan('cloudflared', $process); } } } + /** + * Whether one of the given processes holds the other end of a process's + * stdin pipe. An unreadable stdin counts as held, so it is never reaped. + * + * @param list $holderPids + */ + private function stdinPipeHeldByAny(string $pid, array $holderPids): bool + { + $stdin = @readlink("/proc/{$pid}/fd/0"); + if ($stdin === false || ! str_starts_with($stdin, 'pipe:')) { + return true; + } + + foreach ($holderPids as $holderPid) { + foreach (glob("/proc/{$holderPid}/fd/*") ?: [] as $fd) { + if (@readlink($fd) === $stdin) { + return true; + } + } + } + + return false; + } + /** * Reap a detected orphan process. When orphan reaping is disabled (the * default), the orphan is only logged — a dry-run mode that lets operators @@ -115,7 +172,10 @@ class CleanupStaleMultiplexedConnections implements ShouldQueue return; } - Process::run('kill '.escapeshellarg($process['pid'])); + // SIGKILL ssh: a former master unlinks its ControlPath on SIGTERM, even + // when a newer master owns that path now. + $signal = $kind === 'ssh' ? 'kill -9 ' : 'kill '; + Process::run($signal.escapeshellarg($process['pid'])); Log::info("Killed orphaned {$kind} process", [ 'pid' => $process['pid'], 'etimes' => $process['etimes'], diff --git a/docker/development/Dockerfile b/docker/development/Dockerfile index b024916b7d..9d48baf19d 100644 --- a/docker/development/Dockerfile +++ b/docker/development/Dockerfile @@ -71,6 +71,7 @@ RUN apk add --no-cache \ git-lfs \ jq \ lsof \ + procps \ vim # Install PHP extensions diff --git a/docker/production/Dockerfile b/docker/production/Dockerfile index 8c4b349c5e..9f2f5c55b4 100644 --- a/docker/production/Dockerfile +++ b/docker/production/Dockerfile @@ -136,6 +136,7 @@ RUN --mount=type=cache,target=/var/cache/apk \ git-lfs \ jq \ lsof \ + procps \ vim # Install PHP extensions diff --git a/tests/Feature/SshMultiplexingLockTest.php b/tests/Feature/SshMultiplexingLockTest.php index dd0ce783b2..a489af4955 100644 --- a/tests/Feature/SshMultiplexingLockTest.php +++ b/tests/Feature/SshMultiplexingLockTest.php @@ -223,6 +223,7 @@ it('kills only old orphaned ssh masters whose control socket no longer exists', ". "333 1 30 ssh -fN -o ControlMaster=auto -o ControlPath={$youngSocket} root@1.2.3.4 "), + '*ssh -O check*' => Process::result(errorOutput: 'Master running (pid=111)'), 'kill*' => Process::result(exitCode: 0), ]); @@ -238,6 +239,59 @@ it('kills only old orphaned ssh masters whose control socket no longer exists', File::delete($liveSocket); }); +it('kills ssh -fN processes that share a live ControlPath but do not own it', function () { + config(['constants.ssh.mux_orphan_reap_enabled' => true]); + $muxDir = storage_path('app/ssh/mux'); + File::ensureDirectoryExists($muxDir); + + $socket = $muxDir.'/mux_shared_'.uniqid(); + File::put($socket, 'x'); + + Process::fake([ + 'ps*' => Process::result(output: "111 1 5000 ssh -fN -o ControlMaster=auto -o ControlPath={$socket} root@1.2.3.4\n". + "222 1 7000 ssh -fN -o ControlMaster=auto -o ControlPath={$socket} root@1.2.3.4\n". + "333 1 9000 ssh -fN -o ControlMaster=auto -o ControlPath={$socket} root@1.2.3.4\n"), + '*ssh -O check*' => Process::result(errorOutput: 'Master running (pid=111)'), + 'kill*' => Process::result(exitCode: 0), + ]); + + $job = new CleanupStaleMultiplexedConnections; + $method = new ReflectionMethod($job, 'cleanupOrphanedSshProcesses'); + $method->setAccessible(true); + $method->invoke($job); + + Process::assertRan(fn ($process) => $process->command === 'kill -9 '.escapeshellarg('222')); + Process::assertRan(fn ($process) => $process->command === 'kill -9 '.escapeshellarg('333')); + Process::assertNotRan(fn ($process) => str_contains($process->command, 'kill') && str_contains($process->command, '111')); + Process::assertRanTimes(fn ($process) => str_contains($process->command, 'ssh -O check'), 1); + + File::delete($socket); +}); + +it('does not kill ssh -fN processes when the socket owner is unknown', function () { + config(['constants.ssh.mux_orphan_reap_enabled' => true]); + $muxDir = storage_path('app/ssh/mux'); + File::ensureDirectoryExists($muxDir); + + $socket = $muxDir.'/mux_unknown_'.uniqid(); + File::put($socket, 'x'); + + 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*' => Process::result(exitCode: 255, errorOutput: 'Control socket connect: Connection refused'), + 'kill*' => Process::result(exitCode: 0), + ]); + + $job = new CleanupStaleMultiplexedConnections; + $method = new ReflectionMethod($job, 'cleanupOrphanedSshProcesses'); + $method->setAccessible(true); + $method->invoke($job); + + Process::assertNotRan(fn ($process) => str_contains($process->command, 'kill')); + + File::delete($socket); +}); + it('does not reap an ssh master that is intentionally retiring', function () { config(['constants.ssh.mux_orphan_reap_enabled' => true]); $muxDir = storage_path('app/ssh/mux'); @@ -333,31 +387,45 @@ 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('kills only old orphaned cloudflared proxies whose parent ssh is gone', function () { +it('kills only old cloudflared proxies whose stdin pipe no live ssh holds', function () { config(['constants.ssh.mux_orphan_reap_enabled' => true]); + // This test process stands in for a live ssh master: it holds the write end + // of the reparented proxy's stdin pipe. The orphan's writer is closed. + $descriptors = [0 => ['pipe', 'r'], 1 => ['file', '/dev/null', 'w'], 2 => ['file', '/dev/null', 'w']]; + $reparented = proc_open(['sleep', '60'], $descriptors, $reparentedPipes); + $orphan = proc_open(['sleep', '60'], $descriptors, $orphanPipes); + $tunnelSsh = proc_open(['sleep', '60'], $descriptors, $tunnelSshPipes); + fclose($orphanPipes[0]); + fclose($tunnelSshPipes[0]); + $reparentedPid = proc_get_status($reparented)['pid']; + $orphanPid = proc_get_status($orphan)['pid']; + $tunnelSshPid = proc_get_status($tunnelSsh)['pid']; + $sshPid = getmypid(); + Process::fake([ - 'ps*' => Process::result(output: '100 1 5000 ssh -fN -o ControlMaster=auto root@1.2.3.4 -'. - '200 100 5000 cloudflared access ssh --hostname host.example.com -'. - '300 2176 5000 cloudflared access ssh --hostname host.example.com -'. - '400 2176 30 cloudflared access ssh --hostname host.example.com -'. - '2176 1 9000 /usr/bin/some-supervisor -'), + 'ps*' => Process::result(output: "{$sshPid} 1 5000 ssh -fN -o ControlMaster=auto root@1.2.3.4\n". + "200 {$sshPid} 5000 cloudflared access ssh --hostname host.example.com\n". + "{$reparentedPid} 1 5000 cloudflared access ssh --hostname host.example.com\n". + "{$orphanPid} 1 5000 cloudflared access ssh --hostname host.example.com\n". + "400 1 30 cloudflared access ssh --hostname host.example.com\n". + "{$tunnelSshPid} 1 5000 ssh -fN -o ProxyCommand=cloudflared access ssh --hostname %h root@1.2.3.4\n"), 'kill*' => Process::result(exitCode: 0), ]); $job = new CleanupStaleMultiplexedConnections; $method = new ReflectionMethod($job, 'cleanupOrphanedCloudflaredProcesses'); - $method->setAccessible(true); $method->invoke($job); - Process::assertRan(fn ($process) => str_contains($process->command, 'kill') && str_contains($process->command, '300')); - Process::assertNotRan(fn ($process) => str_contains($process->command, 'kill') && str_contains($process->command, '200')); - Process::assertNotRan(fn ($process) => str_contains($process->command, 'kill') && str_contains($process->command, '400')); + proc_terminate($reparented); + proc_terminate($orphan); + proc_terminate($tunnelSsh); + proc_close($reparented); + proc_close($orphan); + proc_close($tunnelSsh); + + Process::assertRan(fn ($process) => $process->command === 'kill '.escapeshellarg((string) $orphanPid)); + Process::assertRanTimes(fn ($process) => str_starts_with($process->command, 'kill'), 1); }); it('dry-run mode logs orphans but kills nothing when reaping is disabled', function () {