From 8b8022bac49a56c5ba16c8ee980790c20c0b3c3d Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Tue, 29 Sep 2026 19:16:50 +0200 Subject: [PATCH] fix(proxy): run proxy commands without cd for non-root SSH users The proxy directory is 9999:root 0700 on the Coolify host, so a non-root SSH user cannot cd into it, open redirects in it, or expand globs there (#4255). - Start/restart proxy with absolute `docker compose -f` and `docker stack deploy -c` paths instead of `cd $proxy_path` - Write the Caddyfile via `tee` instead of a shell redirect - Check for and prune config backups with `find` instead of globs - Read the config size with `stat -c %s` instead of `wc -c <` - Add feature tests for non-root proxy commands and record the lesson --- .ai/lessons.md | 4 + app/Actions/Proxy/GetProxyConfiguration.php | 2 +- app/Actions/Proxy/SaveProxyConfiguration.php | 12 +- app/Actions/Proxy/StartProxy.php | 12 +- app/Jobs/RestartProxyJob.php | 12 +- .../Proxy/NonRootProxyCommandsTest.php | 166 ++++++++++++++++++ 6 files changed, 191 insertions(+), 17 deletions(-) create mode 100644 tests/Feature/Proxy/NonRootProxyCommandsTest.php diff --git a/.ai/lessons.md b/.ai/lessons.md index eff6d13e5c..03429efdad 100644 --- a/.ai/lessons.md +++ b/.ai/lessons.md @@ -87,3 +87,7 @@ - Container ownership labels are `coolify.applicationUuid`, `coolify.serviceUuid`, `coolify.service.subUuid`, and `coolify.databaseUuid`. Numeric ids change when a server moves to another instance. - Use `resolveContainerOwner()`, `resolveServiceContainerOwner()`, and `containersOwnedBy()` / `dockerPsByOwnerCommands()` from `bootstrap/helpers/docker.php`. Order: UUID label, then `com.docker.compose.project` (the UUID only since July 2024), then the local numeric id label of older containers. - Read owners from the flat label list. `Arr::undot()` breaks `com.docker.compose.project` because `com.docker.compose.project.config_files` is nested under it. + +## Non-root SSH users: keep file access behind sudo +- `parseCommandsByLineForSudo()` does not prefix `cd` or `echo`, and the SSH user's shell opens redirects (`>`, `<`) and expands globs. On the Coolify host, `/data/coolify` is `9999:root 0700`, so these fail for a non-root user. +- In remote commands, use absolute paths (`docker compose -f /docker-compose.yml`), `echo ... | tee > /dev/null`, and `find` instead of globs. diff --git a/app/Actions/Proxy/GetProxyConfiguration.php b/app/Actions/Proxy/GetProxyConfiguration.php index b15b51b7da..c1249ba496 100644 --- a/app/Actions/Proxy/GetProxyConfiguration.php +++ b/app/Actions/Proxy/GetProxyConfiguration.php @@ -113,7 +113,7 @@ class GetProxyConfiguration $readLimit = self::MAX_CONFIGURATION_SIZE_BYTES + 1; $result = instant_remote_process([ "mkdir -p $proxy_path", - "if [ ! -f {$configurationPath} ]; then exit 0; elif [ \"$(wc -c < {$configurationPath})\" -gt ".self::MAX_CONFIGURATION_SIZE_BYTES." ]; then echo '__COOLIFY_PROXY_CONFIG_TOO_LARGE__'; else head -c {$readLimit} {$configurationPath}; fi", + "if [ ! -f {$configurationPath} ]; then exit 0; elif [ \"$(stat -c %s {$configurationPath})\" -gt ".self::MAX_CONFIGURATION_SIZE_BYTES." ]; then echo '__COOLIFY_PROXY_CONFIG_TOO_LARGE__'; else head -c {$readLimit} {$configurationPath}; fi", ], $server, false); if ($result === '__COOLIFY_PROXY_CONFIG_TOO_LARGE__' || strlen($result ?? '') > self::MAX_CONFIGURATION_SIZE_BYTES) { diff --git a/app/Actions/Proxy/SaveProxyConfiguration.php b/app/Actions/Proxy/SaveProxyConfiguration.php index 75775c8d6e..7196301729 100644 --- a/app/Actions/Proxy/SaveProxyConfiguration.php +++ b/app/Actions/Proxy/SaveProxyConfiguration.php @@ -46,10 +46,14 @@ class SaveProxyConfiguration $timestamp = now()->format('Y-m-d_H-i-s'); $backup_file = "docker-compose.{$timestamp}.{$short_hash}.yml"; $commands[] = "mkdir -p $backup_path"; - // Skip backup if a file with the same hash already exists (identical content) - $commands[] = "ls $backup_path/docker-compose.*.$short_hash.yml 1>/dev/null 2>&1 || cp -f $proxy_path/docker-compose.yml $backup_path/$backup_file 2>/dev/null || true"; - // Prune old backups, keep only the most recent ones - $commands[] = 'cd '.$backup_path.' && ls -1t docker-compose.*.yml 2>/dev/null | tail -n +'.((int) self::MAX_BACKUPS + 1).' | xargs rm -f 2>/dev/null || true'; + // Skip backup if a file with the same hash already exists (identical content). + // find instead of a shell glob, and no `|` with `||`: the non-root sudo parser would need bash -c, + // and the SSH user may not be able to read the directory (#4255). + $commands[] = "if [ -z \"$(find $backup_path -maxdepth 1 -name 'docker-compose.*.$short_hash.yml')\" ]; then"; + $commands[] = " cp -f $proxy_path/docker-compose.yml $backup_path/$backup_file 2>/dev/null || true"; + $commands[] = 'fi'; + // Prune old backups, keep only the most recent ones (the timestamp in the name sorts by age) + $commands[] = "find $backup_path -maxdepth 1 -name 'docker-compose.*.yml' | sort -r | tail -n +".((int) self::MAX_BACKUPS + 1).' | xargs -r rm -f'; } $commands[] = "echo '$docker_compose_yml_base64' | base64 -d | tee $proxy_path/docker-compose.yml > /dev/null"; diff --git a/app/Actions/Proxy/StartProxy.php b/app/Actions/Proxy/StartProxy.php index 2639ef35ef..c546a4252e 100644 --- a/app/Actions/Proxy/StartProxy.php +++ b/app/Actions/Proxy/StartProxy.php @@ -35,27 +35,27 @@ class StartProxy $commands = collect([]); $proxy_path = $server->proxyPath(); + // Absolute paths: a non-root SSH user may not be able to enter the proxy directory (#4255). + $compose_file = rtrim($proxy_path, '/').'/docker-compose.yml'; SaveProxyConfiguration::run($server, $configuration); $server->markProxyConfigurationApplied($configuration); if ($server->isSwarmManager()) { $commands = $commands->merge([ "mkdir -p $proxy_path/dynamic", - "cd $proxy_path", "echo 'Creating required Docker Compose file.'", "echo 'Starting coolify-proxy.'", - 'docker stack deploy --detach=true -c docker-compose.yml coolify-proxy', + "docker stack deploy --detach=true -c $compose_file coolify-proxy", "echo 'Successfully started coolify-proxy.'", ]); } else { $caddyfile = 'import /dynamic/*.caddy'; $commands = $commands->merge([ "mkdir -p $proxy_path/dynamic", - "cd $proxy_path", - "echo '$caddyfile' > $proxy_path/dynamic/Caddyfile", + "echo '$caddyfile' | tee $proxy_path/dynamic/Caddyfile > /dev/null", "echo 'Creating required Docker Compose file.'", "echo 'Pulling docker image.'", - 'docker compose pull', + "docker compose -f $compose_file pull", 'if docker ps -a --format "{{.Names}}" | grep -q "^coolify-proxy$"; then', " echo 'Stopping and removing existing coolify-proxy.'", ' docker stop coolify-proxy 2>/dev/null || true', @@ -75,7 +75,7 @@ class StartProxy $commands = $commands->merge(ensureProxyNetworksExist($server)); $commands = $commands->merge([ "echo 'Starting coolify-proxy.'", - 'docker compose up -d --wait --remove-orphans', + "docker compose -f $compose_file up -d --wait --remove-orphans", "echo 'Successfully started coolify-proxy.'", ]); $commands = $commands->merge(connectProxyToNetworks($server)); diff --git a/app/Jobs/RestartProxyJob.php b/app/Jobs/RestartProxyJob.php index d314bd5bb7..b74d8f83a7 100644 --- a/app/Jobs/RestartProxyJob.php +++ b/app/Jobs/RestartProxyJob.php @@ -88,6 +88,8 @@ class RestartProxyJob implements ShouldBeEncrypted, ShouldQueue { $containerName = $this->server->isSwarm() ? 'coolify-proxy_traefik' : 'coolify-proxy'; $proxy_path = $this->server->proxyPath(); + // Absolute paths: a non-root SSH user may not be able to enter the proxy directory (#4255). + $compose_file = rtrim($proxy_path, '/').'/docker-compose.yml'; $stopTimeout = 30; SaveProxyConfiguration::run($this->server, $configuration); @@ -125,10 +127,9 @@ class RestartProxyJob implements ShouldBeEncrypted, ShouldQueue $commands = $commands->merge([ "echo 'Starting proxy (Swarm mode)...'", "mkdir -p $proxy_path/dynamic", - "cd $proxy_path", "echo 'Creating required Docker Compose file.'", "echo 'Starting coolify-proxy.'", - 'docker stack deploy --detach=true -c docker-compose.yml coolify-proxy', + "docker stack deploy --detach=true -c $compose_file coolify-proxy", "echo 'Successfully started coolify-proxy.'", ]); } else { @@ -136,17 +137,16 @@ class RestartProxyJob implements ShouldBeEncrypted, ShouldQueue $commands = $commands->merge([ "echo 'Starting proxy...'", "mkdir -p $proxy_path/dynamic", - "cd $proxy_path", - "echo '$caddyfile' > $proxy_path/dynamic/Caddyfile", + "echo '$caddyfile' | tee $proxy_path/dynamic/Caddyfile > /dev/null", "echo 'Creating required Docker Compose file.'", "echo 'Pulling docker image.'", - 'docker compose pull', + "docker compose -f $compose_file pull", ]); // Ensure required networks exist BEFORE docker compose up $commands = $commands->merge(ensureProxyNetworksExist($this->server)); $commands = $commands->merge([ "echo 'Starting coolify-proxy.'", - 'docker compose up -d --wait --remove-orphans', + "docker compose -f $compose_file up -d --wait --remove-orphans", "echo 'Successfully started coolify-proxy.'", ]); $commands = $commands->merge(connectProxyToNetworks($this->server)); diff --git a/tests/Feature/Proxy/NonRootProxyCommandsTest.php b/tests/Feature/Proxy/NonRootProxyCommandsTest.php new file mode 100644 index 0000000000..49cd73584f --- /dev/null +++ b/tests/Feature/Proxy/NonRootProxyCommandsTest.php @@ -0,0 +1,166 @@ + 0]); + config(['constants.ssh.mux_enabled' => false]); + Process::fake(); + + $team = Team::factory()->create(); + $this->server = Server::factory()->create([ + 'team_id' => $team->id, + 'user' => 'cooluser', + 'private_key_id' => PrivateKey::factory()->create(['team_id' => $team->id])->id, + ]); + $this->server->settings->update(['is_reachable' => true, 'is_usable' => true, 'force_disabled' => false]); +}); + +/** + * @return array + */ +function nonRootProxyScriptLines(): array +{ + $lines = []; + Process::assertRan(function ($process) use (&$lines) { + array_push($lines, ...explode("\n", $process->command)); + + return true; + }); + + return $lines; +} + +function expectNoUnprivilegedProxyFileAccess(array $lines): void +{ + foreach ($lines as $line) { + expect(trim($line))->not->toStartWith('cd '); + // A redirect or a glob outside `sudo bash -c` runs as the SSH user. + expect($line)->not->toMatch('/<\s*\S*\/data\/coolify|>\s*\/data\/coolify/'); + if (! str_starts_with(trim($line), 'sudo bash -c')) { + expect($line)->not->toMatch('/\/data\/coolify\S*\*/'); + } + } +} + +function useNonRootProxy(Server $server, string $proxyType): void +{ + $server->proxy->type = $proxyType; + $server->proxy->status = 'exited'; + $server->save(); + $server->refresh(); +} + +it('starts the proxy without entering proxy directories as the SSH user', function (string $proxyType, string $proxyPath) { + useNonRootProxy($this->server, $proxyType); + + StartProxy::run($this->server, async: false, force: true); + + $lines = nonRootProxyScriptLines(); + expectNoUnprivilegedProxyFileAccess($lines); + expect($lines) + ->toContain("sudo docker compose -f {$proxyPath}/docker-compose.yml pull") + ->toContain("sudo docker compose -f {$proxyPath}/docker-compose.yml up -d --wait --remove-orphans"); +})->with([ + 'traefik' => [ProxyTypes::TRAEFIK->value, '/data/coolify/proxy'], + 'caddy' => [ProxyTypes::CADDY->value, '/data/coolify/proxy/caddy'], +]); + +it('writes the Caddyfile through sudo', function () { + useNonRootProxy($this->server, ProxyTypes::CADDY->value); + + StartProxy::run($this->server, async: false, force: true); + + expect(nonRootProxyScriptLines()) + ->toContain("echo 'import /dynamic/*.caddy' | sudo tee /data/coolify/proxy/caddy/dynamic/Caddyfile > /dev/null"); +}); + +it('deploys the swarm proxy stack with an absolute compose file path', function () { + useNonRootProxy($this->server, ProxyTypes::TRAEFIK->value); + $this->server->settings->update(['is_swarm_manager' => true]); + $this->server->refresh(); + + StartProxy::run($this->server, async: false, force: true); + + $lines = nonRootProxyScriptLines(); + expectNoUnprivilegedProxyFileAccess($lines); + expect($lines)->toContain('sudo docker stack deploy --detach=true -c /data/coolify/proxy/docker-compose.yml coolify-proxy'); +}); + +it('builds restart commands without entering proxy directories as the SSH user', function (string $proxyType, string $proxyPath) { + useNonRootProxy($this->server, $proxyType); + $configuration = $this->server->proxyType() === ProxyTypes::CADDY->value + ? "services:\n caddy:\n image: lucaslorentz/caddy-docker-proxy:2.13-alpine\n" + : "services:\n traefik:\n image: traefik:v3.6\n"; + + $method = new ReflectionMethod(RestartProxyJob::class, 'buildRestartCommands'); + $commands = $method->invoke(new RestartProxyJob($this->server), $configuration); + $lines = parseCommandsByLineForSudo(collect($commands), $this->server); + + expectNoUnprivilegedProxyFileAccess($lines); + expect($lines) + ->toContain("sudo docker compose -f {$proxyPath}/docker-compose.yml pull") + ->toContain("sudo docker compose -f {$proxyPath}/docker-compose.yml up -d --wait --remove-orphans"); +})->with([ + 'traefik' => [ProxyTypes::TRAEFIK->value, '/data/coolify/proxy'], + 'caddy' => [ProxyTypes::CADDY->value, '/data/coolify/proxy/caddy'], +]); + +it('backs up and prunes proxy configurations through sudo without bash', function () { + useNonRootProxy($this->server, ProxyTypes::CADDY->value); + $this->server->proxy->last_saved_settings = 'a1b2c3d4e5f6'; + $this->server->save(); + + SaveProxyConfiguration::run($this->server, "services:\n caddy:\n image: lucaslorentz/caddy-docker-proxy:2.13-alpine\n"); + + $lines = nonRootProxyScriptLines(); + $backups = '/data/coolify/proxy/caddy/backups'; + expectNoUnprivilegedProxyFileAccess($lines); + // Alpine servers can run without bash, so the parser must not need `sudo bash -c` here. + expect(collect($lines)->filter(fn (string $line) => str_contains($line, 'bash -c')))->toBeEmpty() + ->and($lines) + ->toContain("if sudo [ -z \"$(sudo find {$backups} -maxdepth 1 -name 'docker-compose.*.a1b2c3d4.yml')\" ]; then") + ->toContain("sudo find {$backups} -maxdepth 1 -name 'docker-compose.*.yml' | sudo sort -r | sudo tail -n +11 | sudo xargs -r rm -f") + ->and(collect($lines)->first(fn (string $line) => str_contains($line, "cp -f /data/coolify/proxy/caddy/docker-compose.yml {$backups}/"))) + ->toStartWith('sudo '); +}); + +it('runs the same proxy commands without sudo for a root SSH user', function (string $proxyType, string $proxyPath) { + $this->server->update(['user' => 'root']); + useNonRootProxy($this->server, $proxyType); + $this->server->proxy->last_saved_settings = 'a1b2c3d4e5f6'; + $this->server->save(); + + StartProxy::run($this->server, async: false, force: true); + + $lines = nonRootProxyScriptLines(); + expect(collect($lines)->filter(fn (string $line) => str_contains($line, 'sudo')))->toBeEmpty() + ->and(collect($lines)->filter(fn (string $line) => str_starts_with(trim($line), 'cd ')))->toBeEmpty() + ->and($lines) + ->toContain("docker compose -f {$proxyPath}/docker-compose.yml pull") + ->toContain("docker compose -f {$proxyPath}/docker-compose.yml up -d --wait --remove-orphans") + ->and(collect($lines)->filter(fn (string $line) => str_starts_with($line, 'find ') && str_contains($line, '/backups -maxdepth 1')))->not->toBeEmpty(); +})->with([ + 'traefik' => [ProxyTypes::TRAEFIK->value, '/data/coolify/proxy'], + 'caddy' => [ProxyTypes::CADDY->value, '/data/coolify/proxy/caddy'], +]);