From d4a655c390a9ce15b76c616c2fb6b4b77b797f8d Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Wed, 9 Sep 2026 14:27:31 +0200 Subject: [PATCH] fix(database): keep S3 import credentials out of activity commands Pass S3 endpoint and keys as helper container env vars and run mc alias via those variables so the stored import command no longer contains access keys or secrets. Start the helper in a separate remote process, use the storage filesystem adapter for object checks, and remove a credential temp file during import cleanup when the path is safe. --- app/Actions/Database/StartDatabaseImport.php | 30 ++++- app/Listeners/CleanupDatabaseImport.php | 4 + .../StartDatabaseImportS3CredentialsTest.php | 104 ++++++++++++++++++ .../CleanupDatabaseImportTest.php | 3 + 4 files changed, 136 insertions(+), 5 deletions(-) create mode 100644 tests/Feature/StartDatabaseImportS3CredentialsTest.php diff --git a/app/Actions/Database/StartDatabaseImport.php b/app/Actions/Database/StartDatabaseImport.php index eb4d4c33c6..4b59fe7911 100644 --- a/app/Actions/Database/StartDatabaseImport.php +++ b/app/Actions/Database/StartDatabaseImport.php @@ -7,7 +7,6 @@ use App\Models\S3Storage; use App\Models\Server; use App\Models\ServiceDatabase; use App\Models\SwarmDocker; -use App\Rules\SafeWebhookUrl; use App\Support\DatabaseBackupFileValidator; use App\Support\DatabaseImport\DatabaseImportCommandBuilder; use App\Support\DatabaseImport\DatabaseImportException; @@ -19,6 +18,7 @@ use Illuminate\Support\Facades\Storage; use Illuminate\Support\Str; use Lorisleiva\Actions\Concerns\AsAction; use Spatie\Activitylog\Models\Activity; +use Throwable; class StartDatabaseImport { @@ -117,17 +117,16 @@ class StartDatabaseImport } $key = ltrim((string) $source->path, '/'); $this->assertS3Path($key); - $disk = Storage::build(['driver' => 's3', 'region' => $storage->region, 'key' => $storage->key, 'secret' => $storage->secret, 'bucket' => $storage->bucket, 'endpoint' => $storage->endpoint, 'use_path_style_endpoint' => true, 'http' => SafeWebhookUrl::httpClientOptions($storage->endpoint)]); + $disk = $storage->filesystem(); if (! $disk->exists($key) || $disk->size($key) > self::MAX_BYTES) { throw new DatabaseImportException('The S3 backup was not found or exceeds the 10 GiB limit.'); } $helper = "s3-restore-{$operation}"; $serverPath = "/tmp/s3-restore-{$operation}"; + $this->startS3HelperWithEnv($storage, $server, $helper, $network); $sourceArg = escapeshellarg("s3temp/{$storage->bucket}/{$key}"); $commandList = [ - 'docker rm -f '.escapeshellarg($helper).' 2>/dev/null || true', - 'docker run -d --network '.escapeshellarg($network).' --name '.escapeshellarg($helper).' '.escapeshellarg(coolifyHelperImage().':'.getHelperVersion()).' sleep 3600', - 'docker exec '.escapeshellarg($helper).' mc alias set s3temp '.escapeshellarg($storage->endpoint).' '.escapeshellarg($storage->key).' '.escapeshellarg($storage->secret), + 'docker exec '.escapeshellarg($helper).' sh -c '.escapeshellarg('mc alias set s3temp "$S3_ENDPOINT" "$S3_ACCESS_KEY" "$S3_SECRET_KEY"'), 'docker exec '.escapeshellarg($helper).' mc cp '.$sourceArg.' /tmp/restore', 'docker cp '.escapeshellarg("{$helper}:/tmp/restore").' '.escapeshellarg($serverPath), 'docker cp '.escapeshellarg($serverPath).' '.escapeshellarg("{$container}:{$containerPath}"), @@ -176,4 +175,25 @@ class StartDatabaseImport throw new DatabaseImportException('The S3 path is invalid.'); } } + + private function startS3HelperWithEnv(S3Storage $storage, Server $server, string $helper, string $network): void + { + $image = escapeshellarg(coolifyHelperImage().':'.getHelperVersion()); + + try { + instant_remote_process([ + 'docker rm -f '.escapeshellarg($helper).' 2>/dev/null || true', + 'docker run -d --network '.escapeshellarg($network) + .' --name '.escapeshellarg($helper) + .' -e S3_ENDPOINT='.escapeshellarg((string) $storage->endpoint) + .' -e S3_ACCESS_KEY='.escapeshellarg((string) $storage->key) + .' -e S3_SECRET_KEY='.escapeshellarg((string) $storage->secret) + .' '.$image.' sleep 3600', + ], $server); + } catch (Throwable) { + instant_remote_process(['docker rm -f '.escapeshellarg($helper).' 2>/dev/null || true'], $server, throwError: false); + + throw new DatabaseImportException('Unable to start the S3 restore helper.'); + } + } } diff --git a/app/Listeners/CleanupDatabaseImport.php b/app/Listeners/CleanupDatabaseImport.php index 958b5793ee..a3a36b8342 100644 --- a/app/Listeners/CleanupDatabaseImport.php +++ b/app/Listeners/CleanupDatabaseImport.php @@ -41,6 +41,10 @@ class CleanupDatabaseImport implements ShouldQueue $commands[] = 'rm -f '.escapeshellarg($data['serverTmpPath']).' 2>/dev/null || true'; } + if (isSafeTmpPath($data['credentialTmpPath'] ?? null)) { + $commands[] = 'rm -f '.escapeshellarg($data['credentialTmpPath']).' 2>/dev/null || true'; + } + if (filled($data['container'] ?? null)) { foreach (['containerTmpPath', 'scriptPath'] as $key) { if (isSafeTmpPath($data[$key] ?? null)) { diff --git a/tests/Feature/StartDatabaseImportS3CredentialsTest.php b/tests/Feature/StartDatabaseImportS3CredentialsTest.php new file mode 100644 index 0000000000..356a16cc07 --- /dev/null +++ b/tests/Feature/StartDatabaseImportS3CredentialsTest.php @@ -0,0 +1,104 @@ +set('cache.default', 'array'); + config()->set('constants.ssh.mux_enabled', false); + InstanceSettings::forceCreate(['id' => 0]); + + $this->team = Team::factory()->create(); + $this->privateKey = PrivateKey::factory()->create(['team_id' => $this->team->id]); + $this->server = Server::factory()->create([ + 'team_id' => $this->team->id, + 'private_key_id' => $this->privateKey->id, + 'user' => 'root', + ]); + $this->destination = StandaloneDocker::firstOrCreate( + ['server_id' => $this->server->id, 'network' => 'coolify'], + ['uuid' => (string) Str::uuid(), 'name' => 'docker'] + ); + $this->project = Project::factory()->create(['team_id' => $this->team->id]); + $this->environment = Environment::factory()->create(['project_id' => $this->project->id]); + $this->database = StandalonePostgresql::create([ + 'uuid' => (string) Str::uuid(), + 'name' => 'db', + 'postgres_user' => 'postgres', + 'postgres_password' => 'password', + 'postgres_db' => 'db', + 'image' => 'postgres:17', + 'status' => 'running', + 'environment_id' => $this->environment->id, + 'destination_id' => $this->destination->id, + 'destination_type' => $this->destination->getMorphClass(), + ]); +}); + +test('s3 import activity command does not contain storage key or secret', function () { + $accessKey = 'AKIA_TEST_ACCESS_KEY_LEAK'; + $secret = 'wJalrXUtnFEMI/K7MDENG/bPxRfiCYTESTSECRET'; + $storage = S3Storage::create([ + 'name' => 'Import S3', + 'region' => 'us-east-1', + 'key' => $accessKey, + 'secret' => $secret, + 'bucket' => 'test-bucket', + 'endpoint' => 'https://8.8.8.8', + 'is_usable' => true, + 'team_id' => $this->team->id, + ]); + + $disk = Mockery::mock(FilesystemAdapter::class); + $disk->shouldReceive('exists')->once()->with('backups/restore.sql')->andReturn(true); + $disk->shouldReceive('size')->once()->with('backups/restore.sql')->andReturn(1024); + $filesystem = Mockery::mock(FilesystemManager::class, [app()])->makePartial(); + $filesystem->shouldReceive('build')->once()->andReturn($disk); + Storage::swap($filesystem); + + Process::fake(); + Queue::fake(); + + $activity = app(StartDatabaseImport::class)->handle( + $this->database, + new DatabaseImportSource('s3', path: 'backups/restore.sql', s3StorageUuid: $storage->uuid), + $this->team->id, + ); + + $command = (string) $activity->getExtraProperty('command'); + + expect($command) + ->not->toContain($accessKey) + ->not->toContain($secret) + ->not->toContain('.env') + ->not->toContain('S3_ACCESS_KEY=') + ->not->toContain('S3_SECRET_KEY=') + ->toContain('mc alias set s3temp "$S3_ENDPOINT" "$S3_ACCESS_KEY" "$S3_SECRET_KEY"'); + + Queue::assertPushed(CoolifyTask::class, function (CoolifyTask $job) { + $cleanup = $job->call_event_data; + + return is_array($cleanup) + && ! array_key_exists('credentialTmpPath', $cleanup) + && filled($cleanup['containerName'] ?? null); + }); +}); diff --git a/tests/Unit/DatabaseImport/CleanupDatabaseImportTest.php b/tests/Unit/DatabaseImport/CleanupDatabaseImportTest.php index f48177d53e..895440e7a0 100644 --- a/tests/Unit/DatabaseImport/CleanupDatabaseImportTest.php +++ b/tests/Unit/DatabaseImport/CleanupDatabaseImportTest.php @@ -46,9 +46,11 @@ test('builds S3, upload, and server-path cleanup commands', function () { expect($listener->commands(importCleanupPayload([ 'containerName' => 's3-restore-op', 'serverTmpPath' => '/tmp/s3-restore-op', + 'credentialTmpPath' => '/tmp/s3-restore-op.env', ])))->toBe([ 'docker rm -f '.escapeshellarg('s3-restore-op').' 2>/dev/null || true', 'rm -f '.escapeshellarg('/tmp/s3-restore-op').' 2>/dev/null || true', + 'rm -f '.escapeshellarg('/tmp/s3-restore-op.env').' 2>/dev/null || true', 'docker exec '.escapeshellarg('postgres-abc').' rm -f '.escapeshellarg('/tmp/restore_op').' 2>/dev/null || true', 'docker exec '.escapeshellarg('postgres-abc').' rm -f '.escapeshellarg('/tmp/restore_op.sh').' 2>/dev/null || true', ]); @@ -73,6 +75,7 @@ test('omits unsafe paths, missing container execs, and empty payloads', function expect($listener->commands(importCleanupPayload([ 'containerName' => 's3-restore-op', 'serverTmpPath' => '/tmp/../etc/passwd', + 'credentialTmpPath' => '/tmp/../etc/shadow', 'containerTmpPath' => '/etc/shadow', 'scriptPath' => '/tmp/../../etc/shadow', ])))->toBe([