mirror of
https://github.com/coollabsio/coolify.git
synced 2026-11-06 13:10:37 -04:00
fix(backups): sign S3 uploads with the storage region
Closes #12264 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5.5
parent
ab65fdccf9
commit
1e71561239
@@ -301,6 +301,7 @@ class StartDatabaseImport
|
||||
.' -e S3_ENDPOINT='.escapeshellarg((string) $storage->endpoint)
|
||||
.' -e S3_ACCESS_KEY='.escapeshellarg((string) $storage->key)
|
||||
.' -e S3_SECRET_KEY='.escapeshellarg((string) $storage->secret)
|
||||
.' '.$storage->minioClientRegionOption()
|
||||
.' '.$image.' sleep 86400',
|
||||
], $server);
|
||||
} catch (Throwable) {
|
||||
|
||||
@@ -914,7 +914,7 @@ class DatabaseBackupJob implements ShouldBeEncrypted, ShouldQueue
|
||||
// Remove a leftover helper in the same batch, so a replayed batch does not fail with a name conflict.
|
||||
$commands[] = "docker rm -f backup-of-{$this->backup_log_uuid} >/dev/null 2>&1 || true";
|
||||
$mount = escapeshellarg($this->backupMountSource().':'.$this->backup_location.':ro');
|
||||
$commands[] = "docker run -d --network {$safeNetwork} --name backup-of-{$this->backup_log_uuid} --rm -v {$mount} {$fullImageName}";
|
||||
$commands[] = "docker run -d --network {$safeNetwork} --name backup-of-{$this->backup_log_uuid} --rm {$s3->minioClientRegionOption()} -v {$mount} {$fullImageName}";
|
||||
|
||||
// Escape S3 credentials to prevent command injection
|
||||
$escapedEndpoint = escapeshellarg($endpoint);
|
||||
|
||||
@@ -437,7 +437,7 @@ class VolumeBackupJob implements ShouldBeEncrypted, ShouldQueue
|
||||
try {
|
||||
instant_remote_process([
|
||||
'docker rm -f '.escapeshellarg($containerName).' >/dev/null 2>&1 || true',
|
||||
'docker run -d --name '.escapeshellarg($containerName).' --rm -v '
|
||||
'docker run -d --name '.escapeshellarg($containerName).' --rm '.$s3->minioClientRegionOption().' -v '
|
||||
.escapeshellarg($backupLocation.':'.$backupLocation.':ro').' '.escapeshellarg($image),
|
||||
'docker exec '.escapeshellarg($containerName).' mc alias set'.$resolveOptions.' temporary '
|
||||
.escapeshellarg($s3->endpoint).' '.escapeshellarg($s3->key).' '.escapeshellarg($s3->secret),
|
||||
@@ -476,6 +476,7 @@ class VolumeBackupJob implements ShouldBeEncrypted, ShouldQueue
|
||||
.' | sed -n '.escapeshellarg('s/.*"size":\([0-9][0-9]*\).*/\1/p');
|
||||
|
||||
return 'docker run --rm --name '.escapeshellarg($containerName)
|
||||
.' '.$s3->minioClientRegionOption()
|
||||
.' -v '.escapeshellarg($source.':/volume:ro')
|
||||
.' '.escapeshellarg($image)
|
||||
.' sh -c '.escapeshellarg($streamScript);
|
||||
|
||||
@@ -273,6 +273,15 @@ class S3Storage extends BaseModel
|
||||
]);
|
||||
}
|
||||
|
||||
/**
|
||||
* The `docker run` option that makes the MinIO client sign requests for the storage region, as filesystem() does.
|
||||
* Without it, mc signs for us-east-1, and providers that enforce the region reject the requests.
|
||||
*/
|
||||
public function minioClientRegionOption(): string
|
||||
{
|
||||
return '-e '.escapeshellarg('MC_REGION='.$this->region);
|
||||
}
|
||||
|
||||
/**
|
||||
* The bundled MinIO container is a trusted internal S3 target, not a user-supplied webhook destination.
|
||||
*
|
||||
|
||||
@@ -14,6 +14,7 @@ use App\Models\Server;
|
||||
use App\Models\StandaloneDocker;
|
||||
use App\Models\Team;
|
||||
use App\Models\User;
|
||||
use Illuminate\Filesystem\FilesystemAdapter;
|
||||
use Illuminate\Foundation\Testing\RefreshDatabase;
|
||||
use Illuminate\Support\Collection;
|
||||
use Illuminate\Support\Facades\Event;
|
||||
@@ -82,6 +83,31 @@ function fakeMultiS3Deletes(): Collection
|
||||
* @return array{0: ScheduledDatabaseBackupExecution, 1: Collection<int, string>}
|
||||
*/
|
||||
function runMultiS3BackupJob(array $failingStorages): array
|
||||
{
|
||||
$backup = createMultiS3Backup();
|
||||
$commands = collect();
|
||||
Process::fake(function ($process) use ($commands) {
|
||||
$command = is_array($process->command) ? implode(' ', $process->command) : $process->command;
|
||||
$commands->push($command);
|
||||
|
||||
return Process::result(output: str_contains($command, 'du -b') ? '128' : '');
|
||||
});
|
||||
|
||||
$job = Mockery::mock(DatabaseBackupJob::class.'[upload_to_s3]', [$backup])->shouldAllowMockingProtectedMethods();
|
||||
$job->shouldReceive('upload_to_s3')->andReturnUsing(function (S3Storage $storage) use ($failingStorages) {
|
||||
if (in_array($storage->name, $failingStorages, true)) {
|
||||
throw new RuntimeException('access denied');
|
||||
}
|
||||
});
|
||||
$job->handle();
|
||||
|
||||
return [ScheduledDatabaseBackupExecution::query()->sole(), $commands];
|
||||
}
|
||||
|
||||
/**
|
||||
* Creates an S3-only Postgres backup schedule with the destinations "First" and "Second".
|
||||
*/
|
||||
function createMultiS3Backup(): ScheduledDatabaseBackup
|
||||
{
|
||||
$team = Team::factory()->create();
|
||||
$server = Server::factory()->create([
|
||||
@@ -107,23 +133,8 @@ function runMultiS3BackupJob(array $failingStorages): array
|
||||
'team_id' => $team->id,
|
||||
])->save();
|
||||
$backup->syncS3Storages([$first->id, $second->id]);
|
||||
$commands = collect();
|
||||
Process::fake(function ($process) use ($commands) {
|
||||
$command = is_array($process->command) ? implode(' ', $process->command) : $process->command;
|
||||
$commands->push($command);
|
||||
|
||||
return Process::result(output: str_contains($command, 'du -b') ? '128' : '');
|
||||
});
|
||||
|
||||
$job = Mockery::mock(DatabaseBackupJob::class.'[upload_to_s3]', [$backup->fresh()])->shouldAllowMockingProtectedMethods();
|
||||
$job->shouldReceive('upload_to_s3')->andReturnUsing(function (S3Storage $storage) use ($failingStorages) {
|
||||
if (in_array($storage->name, $failingStorages, true)) {
|
||||
throw new RuntimeException('access denied');
|
||||
}
|
||||
});
|
||||
$job->handle();
|
||||
|
||||
return [ScheduledDatabaseBackupExecution::query()->sole(), $commands];
|
||||
return $backup->fresh();
|
||||
}
|
||||
|
||||
it('applies S3 retention to each destination separately', function () {
|
||||
@@ -301,6 +312,26 @@ describe('backup job', function () {
|
||||
'failed upload' => [['First']],
|
||||
]);
|
||||
|
||||
it('signs each upload with the region of its destination', function () {
|
||||
$backup = createMultiS3Backup();
|
||||
S3Storage::query()->update(['endpoint' => 'https://8.8.8.8']);
|
||||
S3Storage::query()->where('name', 'First')->update(['region' => 'ru-central-1']);
|
||||
$disk = Mockery::mock(FilesystemAdapter::class);
|
||||
$disk->shouldReceive('files')->andReturn([]);
|
||||
$sshDisk = Storage::disk('ssh-keys');
|
||||
Storage::shouldReceive('disk')->with('ssh-keys')->andReturn($sshDisk);
|
||||
Storage::shouldReceive('build')->andReturn($disk);
|
||||
Process::fake(fn ($process) => Process::result(output: str_contains($process->command, 'du -b') ? '128' : ''));
|
||||
|
||||
(new DatabaseBackupJob($backup))->handle();
|
||||
|
||||
expect(ScheduledDatabaseBackupExecution::query()->sole()->s3_uploaded)->toBeTrue();
|
||||
Process::assertRan(fn ($process) => str_contains($process->command, "-e 'MC_REGION=ru-central-1'")
|
||||
&& str_contains($process->command, 'temporary/first-bucket/'));
|
||||
Process::assertRan(fn ($process) => str_contains($process->command, "-e 'MC_REGION=us-east-1'")
|
||||
&& str_contains($process->command, 'temporary/second-bucket/'));
|
||||
});
|
||||
|
||||
it('keeps the local file and names the failed destination when one upload fails', function () {
|
||||
[$execution, $commands] = runMultiS3BackupJob(['Second']);
|
||||
|
||||
|
||||
@@ -157,3 +157,35 @@ test('s3 import activity stores the cleanup names and paths but no credential va
|
||||
|
||||
Queue::assertPushed(CoolifyTask::class, fn (CoolifyTask $job) => $job->call_event_data === $stored);
|
||||
});
|
||||
|
||||
test('s3 import helper signs requests with the storage region', function () {
|
||||
$storage = S3Storage::create([
|
||||
'name' => 'Import S3',
|
||||
'region' => 'ru-central-1',
|
||||
'key' => 'key',
|
||||
'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')->andReturn(true);
|
||||
$disk->shouldReceive('size')->andReturn(1024);
|
||||
$filesystem = Mockery::mock(FilesystemManager::class, [app()])->makePartial();
|
||||
$filesystem->shouldReceive('build')->andReturn($disk);
|
||||
Storage::swap($filesystem);
|
||||
|
||||
Process::fake();
|
||||
Queue::fake();
|
||||
|
||||
app(StartDatabaseImport::class)->handle(
|
||||
$this->database,
|
||||
new DatabaseImportSource('s3', path: 'backups/restore.sql', s3StorageUuid: $storage->uuid),
|
||||
$this->team->id,
|
||||
);
|
||||
|
||||
Process::assertRan(fn ($process) => str_contains($process->command, "-e 'MC_REGION=ru-central-1'")
|
||||
&& str_contains($process->command, '-e S3_ENDPOINT='));
|
||||
});
|
||||
|
||||
@@ -116,6 +116,23 @@ it('archives locally and uploads to every destination instead of streaming', fun
|
||||
]);
|
||||
});
|
||||
|
||||
it('signs each upload with the region of its destination', function () {
|
||||
[$backup, $first] = createMultiDestinationVolumeBackup();
|
||||
$first->update(['region' => 'ru-central-1']);
|
||||
fakeMultiDestinationS3Disks();
|
||||
Process::fake([
|
||||
'*du -b*' => '128',
|
||||
'*' => '',
|
||||
]);
|
||||
|
||||
(new VolumeBackupJob($backup))->handle();
|
||||
|
||||
Process::assertRan(fn ($process) => str_contains($process->command, "-e 'MC_REGION=ru-central-1'")
|
||||
&& str_contains($process->command, 'temporary/first-bucket/'));
|
||||
Process::assertRan(fn ($process) => str_contains($process->command, "-e 'MC_REGION=us-east-1'")
|
||||
&& str_contains($process->command, 'temporary/second-bucket/'));
|
||||
});
|
||||
|
||||
it('keeps the local archive and cleans the partial upload when one destination fails', function () {
|
||||
[$backup, $first, $second] = createMultiDestinationVolumeBackup(['disable_local_backup' => true]);
|
||||
fakeMultiDestinationS3Disks();
|
||||
|
||||
@@ -1976,7 +1976,7 @@ it('streams S3-only volume backups with Content-MD5 on Object Lock buckets and w
|
||||
[$application, $volume] = createVolumeBackupApplication($team);
|
||||
$s3Storage = S3Storage::create([
|
||||
'name' => 'Streaming destination',
|
||||
'region' => 'us-east-1',
|
||||
'region' => 'ru-central-1',
|
||||
'key' => 'key',
|
||||
'secret' => 'secret',
|
||||
'bucket' => 'bucket',
|
||||
@@ -2008,6 +2008,7 @@ it('streams S3-only volume backups with Content-MD5 on Object Lock buckets and w
|
||||
$expectedFilename = 'volume-app-data-1786795200.tar.gz';
|
||||
Process::assertRan(fn ($process) => str_contains($process->command, 'tar -I')
|
||||
&& str_contains($process->command, 'mc pipe')
|
||||
&& str_contains($process->command, "-e 'MC_REGION=ru-central-1'")
|
||||
&& ! str_contains($process->command, '--quiet')
|
||||
&& str_contains($process->command, 'lock=$(mc retention info --default --json --resolve')
|
||||
&& str_contains($process->command, '*) checksum=--checksum=MD5 ;; *) checksum= ;; esac && (compressor=')
|
||||
|
||||
Reference in New Issue
Block a user