fix(ssh): reap mux orphans by socket ownership, not file existence

Closes #11883

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<int|string>  $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'],

docker/development/Dockerfile

@@ -71,6 +71,7 @@ RUN apk add --no-cache \
     git-lfs \
     jq \
     lsof \
+    procps \
     vim
 
 # Install PHP extensions

docker/production/Dockerfile

@@ -136,6 +136,7 @@ RUN --mount=type=cache,target=/var/cache/apk \
     git-lfs \
     jq \
     lsof \
+    procps \
     vim
 
 # Install PHP extensions

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 () {