diff --git a/app/Jobs/CleanupStaleMultiplexedConnections.php b/app/Jobs/CleanupStaleMultiplexedConnections.php index d52790e8c4..fb4cb35ddb 100644 --- a/app/Jobs/CleanupStaleMultiplexedConnections.php +++ b/app/Jobs/CleanupStaleMultiplexedConnections.php @@ -30,7 +30,7 @@ class CleanupStaleMultiplexedConnections implements ShouldQueue * race. Such processes are not masters, so ControlPersist never reaps them * and they hold an authenticated session until the container restarts. * - * Every `ssh -fN` for a server carries the same ControlPath, so the socket + * Every master 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 @@ -48,18 +48,9 @@ class CleanupStaleMultiplexedConnections implements ShouldQueue $socketOwners = []; foreach ($this->listProcesses() as $process) { - // Backgrounded ssh master: current `ssh -fN` or legacy `ssh -fNM`. - if (! preg_match('#(^|/)ssh -fN#', $process['args'])) { - continue; - } - - // Only ever touch ssh processes pointing at Coolify's mux directory. - if (! preg_match('#ControlPath=('.preg_quote($muxDir, '#').'/\S+)#', $process['args'], $pathMatch)) { - continue; - } - - $muxSocket = $pathMatch[1]; - if ($process['etimes'] < $minAge + $muxSocket = $this->masterMuxSocket($process['args'], $muxDir); + if ($muxSocket === null + || $process['etimes'] < $minAge || SshMultiplexingHelper::isMuxProcessRetiring($process['pid'], $muxSocket)) { continue; } @@ -71,6 +62,24 @@ class CleanupStaleMultiplexedConnections implements ShouldQueue } } + /** + * ControlPath of a backgrounded ssh master in Coolify's mux directory, or + * null for any other process. ControlPersist retitles a master to + * `ssh: [mux]`; masters started before the upgrade still run + * as `ssh -fN` or `ssh -fNM` with the ControlPath in their arguments. + */ + private function masterMuxSocket(string $args, string $muxDir): ?string + { + $socket = preg_quote($muxDir, '#').'/\S+'; + + if (preg_match('#^ssh: ('.$socket.') \[mux\]#', $args, $matches) + || (preg_match('#(^|/)ssh -fN#', $args) && preg_match('#ControlPath=('.$socket.')#', $args, $matches))) { + return $matches[1]; + } + + return null; + } + /** * 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. @@ -109,7 +118,8 @@ class CleanupStaleMultiplexedConnections implements ShouldQueue $sshPids = []; foreach ($processes as $process) { // The ssh binary itself, not `cloudflared access ssh` (space before ssh). - if (preg_match('#(^|/)ssh\s#', $process['args'])) { + // A ControlPersist master is titled `ssh: [mux]`. + if (preg_match('#(^|/)ssh[\s:]#', $process['args'])) { $sshPids[$process['pid']] = true; } } diff --git a/tests/Feature/SshMultiplexingLockTest.php b/tests/Feature/SshMultiplexingLockTest.php index b71ac3c45a..32e5ad8944 100644 --- a/tests/Feature/SshMultiplexingLockTest.php +++ b/tests/Feature/SshMultiplexingLockTest.php @@ -409,6 +409,36 @@ it('kills only old orphaned ssh masters whose control socket no longer exists', File::delete($liveSocket); }); +it('kills a ControlPersist master whose control socket was removed', function () { + config(['constants.ssh.mux_orphan_reap_enabled' => true]); + $muxDir = storage_path('app/ssh/mux'); + File::ensureDirectoryExists($muxDir); + + $liveSocket = $muxDir.'/mux_live_'.uniqid(); + $removedSocket = $muxDir.'/mux_removed_'.uniqid(); + File::put($liveSocket, 'x'); + + // ControlPersist sets the title of a backgrounded master to `ssh: [mux]`. + Process::fake([ + 'ps*' => Process::result(output: "111 1 5000 ssh: {$liveSocket} [mux]\n". + "222 1 5000 ssh: {$removedSocket} [mux]\n". + "333 1 5000 ssh: /tmp/other/mux_removed [mux]\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::assertNotRan(fn ($process) => str_contains($process->command, 'kill') && str_contains($process->command, '111')); + Process::assertNotRan(fn ($process) => str_contains($process->command, 'kill') && str_contains($process->command, '333')); + + 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'); @@ -599,9 +629,10 @@ it('kills only old cloudflared proxies whose stdin pipe no live ssh holds', func $orphanPid = proc_get_status($orphan)['pid']; $tunnelSshPid = proc_get_status($tunnelSsh)['pid']; $sshPid = getmypid(); + $muxSocket = storage_path('app/ssh/mux/mux_tunnel'); Process::fake([ - 'ps*' => Process::result(output: "{$sshPid} 1 5000 ssh -fN -o ControlMaster=auto root@1.2.3.4\n". + 'ps*' => Process::result(output: "{$sshPid} 1 5000 ssh: {$muxSocket} [mux]\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".