From eaba7ff633c9547efc2b44c49b5282f2096f4200 Mon Sep 17 00:00:00 2001 From: ignacionelson Date: Thu, 10 Sep 2026 16:16:31 -0300 Subject: [PATCH] Let an AWS-hosted install authenticate as its own IAM role MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Requested by @ToMMy86 in #1773: an install running on ECS, EC2 or EKS already has a role attached, and making it also create an IAM user with a long-lived access key is both extra work and a worse security posture than the one AWS offers. The AWS SDK resolves credentials from its default provider chain whenever none is supplied, and Laravel's FilesystemManager already omits the `credentials` entry when the key and secret are empty — so the upload path needed almost nothing. What blocked it was ours: - `isConfigured()` demanded a key and a secret for S3, so a credential-less row was never "configured" and every upload silently stayed on the local disk. - `access_key` was `required_if:provider,s3` on both the save and the connection test. - `probeS3()` built an explicit `credentials` array, so Test connection would have failed even once uploads worked. An explicit `use_instance_role` column rather than "the key was left blank", because blank already means "keep the credential you have" on this form — neither the secret nor the GCS key file is ever sent back to the browser. Ticking it deletes the stored key and secret rather than leaving them in the row for the next database dump. Unchanged for everyone else: MinIO, Backblaze, Wasabi and any other S3-compatible service still authenticate with a key and secret, and the region is still required — the chain resolves credentials, not regions. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01QmyH342d8MuW3pDuE9mbtS --- .../ExternalStorageSettingsController.php | 63 +++++++-- .../Settings/ExternalStorageConfigApplier.php | 20 ++- .../Settings/ExternalStorageSettings.php | 12 +- ...ole_to_external_storage_settings_table.php | 35 +++++ .../js/pages/system/settings/storage.tsx | 69 +++++++--- .../Platform/ExternalStorageSettingsTest.php | 127 +++++++++++++++++- .../Platform/GoogleCloudStorageTest.php | 2 +- 7 files changed, 297 insertions(+), 31 deletions(-) create mode 100644 database/migrations/2026_09_10_090000_add_use_instance_role_to_external_storage_settings_table.php diff --git a/app/Modules/Platform/Http/Controllers/ExternalStorageSettingsController.php b/app/Modules/Platform/Http/Controllers/ExternalStorageSettingsController.php index fb850d16..0cca1781 100644 --- a/app/Modules/Platform/Http/Controllers/ExternalStorageSettingsController.php +++ b/app/Modules/Platform/Http/Controllers/ExternalStorageSettingsController.php @@ -17,6 +17,7 @@ use Illuminate\Http\RedirectResponse; use Illuminate\Http\Request; use Illuminate\Support\Facades\Artisan; use Illuminate\Validation\Rule; +use Illuminate\Validation\Rules\RequiredIf; use Inertia\Inertia; use Inertia\Response; use RuntimeException; @@ -43,6 +44,7 @@ class ExternalStorageSettingsController extends Controller return Inertia::render('system/settings/storage', [ 'active' => $settings->active, 'provider' => $settings->provider->value, + 'use_instance_role' => $settings->use_instance_role, // Never name a top-level Inertia prop "key" — Inertia's React // renderer spreads page props onto the component via // `{ key: , ...props }`, and a prop @@ -78,10 +80,18 @@ class ExternalStorageSettingsController extends Controller 'bucket' => ['required', 'string', 'max:255'], 'root' => ['nullable', 'string', 'max:255'], + // 'sometimes' for the same reason as 'provider' above: absent + // means false, which is what every payload written before this + // choice existed meant. + 'use_instance_role' => ['sometimes', 'boolean'], + // Required only for the provider that uses them, so switching // to GCS does not demand an AWS region that means nothing. - 'access_key' => ['required_if:provider,s3', 'nullable', 'string', 'max:255'], + 'access_key' => [self::requiredForStaticS3($request), 'nullable', 'string', 'max:255'], 'secret' => ['nullable', 'string', 'max:255'], + // Still required when the machine's own role is doing the + // authenticating: the credential chain resolves credentials, + // not which region the bucket is in. 'region' => ['required_if:provider,s3', 'nullable', 'string', 'max:255'], 'endpoint' => ['nullable', 'string', 'max:255'], 'use_path_style' => ['required', 'boolean'], @@ -94,10 +104,13 @@ class ExternalStorageSettingsController extends Controller $settings = ExternalStorageSettings::current(); + $useInstanceRole = (bool) ($validated['use_instance_role'] ?? false); + $settings->fill([ 'active' => $validated['active'], 'provider' => $validated['provider'], - 'key' => $validated['access_key'] ?? null, + 'use_instance_role' => $useInstanceRole, + 'key' => $useInstanceRole ? null : ($validated['access_key'] ?? null), 'bucket' => $validated['bucket'], 'region' => $validated['region'] ?? null, 'endpoint' => $validated['endpoint'] ?? null, @@ -108,7 +121,16 @@ class ExternalStorageSettingsController extends Controller // A blank credential keeps whatever is already stored — neither // field is ever round-tripped to the browser (only the has_* // flags are), so blank means "unchanged", not "cleared". - if (is_string($validated['secret'] ?? null) && $validated['secret'] !== '') { + // + // Except when the machine's own role takes over, which is the one + // thing that does clear it. The whole point of the setting is that + // no long-lived AWS credential is kept here, and a secret left + // sitting in the row unused would still be in the next database + // dump — and would silently come back the moment the box is + // unticked. + if ($useInstanceRole) { + $settings->secret = null; + } elseif (is_string($validated['secret'] ?? null) && $validated['secret'] !== '') { $settings->secret = $validated['secret']; } @@ -144,7 +166,8 @@ class ExternalStorageSettingsController extends Controller $validated = $request->validate([ 'provider' => ['sometimes', Rule::enum(StorageProvider::class)], 'bucket' => ['required', 'string', 'max:255'], - 'access_key' => ['required_if:provider,s3', 'nullable', 'string', 'max:255'], + 'use_instance_role' => ['sometimes', 'boolean'], + 'access_key' => [self::requiredForStaticS3($request), 'nullable', 'string', 'max:255'], 'secret' => ['nullable', 'string', 'max:255'], 'region' => ['required_if:provider,s3', 'nullable', 'string', 'max:255'], 'endpoint' => ['nullable', 'string', 'max:255'], @@ -174,13 +197,21 @@ class ExternalStorageSettingsController extends Controller $config = [ 'version' => 'latest', 'region' => $validated['region'], - 'credentials' => [ - 'key' => $validated['access_key'], - 'secret' => (string) $this->storedIfBlank($validated, 'secret'), - ], 'use_path_style_endpoint' => (bool) ($validated['use_path_style'] ?? false), ]; + // Omitted entirely, not left blank: an S3Client handed a + // 'credentials' array is told to use it, so an empty one fails + // instead of falling through to the default credential provider + // chain. This is what makes the button test the same thing the + // uploads will do — see ExternalStorageConfigApplier::applyS3(). + if (! ($validated['use_instance_role'] ?? false)) { + $config['credentials'] = [ + 'key' => $validated['access_key'], + 'secret' => (string) $this->storedIfBlank($validated, 'secret'), + ]; + } + if (is_string($validated['endpoint'] ?? null) && $validated['endpoint'] !== '') { $config['endpoint'] = $validated['endpoint']; } @@ -210,6 +241,22 @@ class ExternalStorageSettingsController extends Controller iterator_to_array($bucket->objects(['maxResults' => 1]), false); } + /** + * An access key is only demanded of an S3 backend that is actually + * going to authenticate with one. Shared by both the save and the + * connection test so the two cannot disagree about what is required. + */ + private static function requiredForStaticS3(Request $request): RequiredIf + { + // Rule::requiredIf(), not a closure rule: this has to fire when + // the field is missing from the payload altogether, and a closure + // rule is not implicit — it never runs on an absent attribute. + return Rule::requiredIf( + fn (): bool => $request->input('provider') === StorageProvider::S3->value + && ! $request->boolean('use_instance_role') + ); + } + /** * A credential field left blank means "keep what is stored" on save, * so the connection test has to read it the same way — otherwise diff --git a/app/Modules/Platform/Settings/ExternalStorageConfigApplier.php b/app/Modules/Platform/Settings/ExternalStorageConfigApplier.php index e6941b58..9c01b11c 100644 --- a/app/Modules/Platform/Settings/ExternalStorageConfigApplier.php +++ b/app/Modules/Platform/Settings/ExternalStorageConfigApplier.php @@ -50,7 +50,9 @@ class ExternalStorageConfigApplier // account's private key included — in the same database whose dump // the `encrypted` cast exists to survive. Both are now read straight // from the row, by the provider branch that uses them. - private const CACHE_KEY = 'platform.external_storage_settings.v3'; + // v4: use_instance_role joined the shape. Not a credential — it is + // the fact that there isn't one — so it caches like the rest. + private const CACHE_KEY = 'platform.external_storage_settings.v4'; public function __construct( private readonly CapabilityRegistry $capabilities, @@ -93,8 +95,16 @@ class ExternalStorageConfigApplier */ private function applyS3(array $resolved): void { - Config::set('filesystems.disks.files_external.key', $resolved['key']); - Config::set('filesystems.disks.files_external.secret', $this->credential('secret')); + // Left null on purpose when the machine's own role is doing the + // authenticating. Laravel's FilesystemManager::formatS3Config() + // only builds a `credentials` entry when both a key and a secret + // are non-empty, and the AWS SDK falls back to its default + // credential provider chain — ECS task role, EC2 instance + // profile, EKS/IRSA, environment — whenever none is supplied. + // Passing an empty string instead of nothing would be a + // credential, and would fail rather than fall through. + Config::set('filesystems.disks.files_external.key', $resolved['use_instance_role'] ? null : $resolved['key']); + Config::set('filesystems.disks.files_external.secret', $resolved['use_instance_role'] ? null : $this->credential('secret')); Config::set('filesystems.disks.files_external.region', $resolved['region']); Config::set('filesystems.disks.files_external.endpoint', $resolved['endpoint']); Config::set('filesystems.disks.files_external.use_path_style_endpoint', $resolved['use_path_style']); @@ -172,13 +182,14 @@ class ExternalStorageConfigApplier * filled in and active, nothing more. Callers AND the capability check * live and uncached — see class docblock. * - * @return array{configured: bool, provider: string, key: string|null, region: string|null, bucket: string|null, endpoint: string|null, use_path_style: bool, root: string|null} + * @return array{configured: bool, provider: string, use_instance_role: bool, key: string|null, region: string|null, bucket: string|null, endpoint: string|null, use_path_style: bool, root: string|null} */ private function resolve(): array { $blank = [ 'configured' => false, 'provider' => StorageProvider::S3->value, + 'use_instance_role' => false, 'key' => null, 'region' => null, 'bucket' => null, 'endpoint' => null, 'use_path_style' => false, 'root' => null, @@ -202,6 +213,7 @@ class ExternalStorageConfigApplier return [ 'configured' => true, 'provider' => $settings->provider->value, + 'use_instance_role' => $settings->use_instance_role, 'key' => $settings->key, 'region' => $settings->region, 'bucket' => $settings->bucket, diff --git a/app/Modules/Platform/Settings/ExternalStorageSettings.php b/app/Modules/Platform/Settings/ExternalStorageSettings.php index a49293d3..8aaf248b 100644 --- a/app/Modules/Platform/Settings/ExternalStorageSettings.php +++ b/app/Modules/Platform/Settings/ExternalStorageSettings.php @@ -17,6 +17,7 @@ use Illuminate\Database\Eloquent\Model; * @property int $id * @property bool $active * @property StorageProvider $provider + * @property bool $use_instance_role * @property string|null $key * @property string|null $secret * @property string|null $key_file @@ -33,6 +34,7 @@ class ExternalStorageSettings extends Model protected $fillable = [ 'active', 'provider', + 'use_instance_role', 'key', 'secret', 'key_file', @@ -55,6 +57,7 @@ class ExternalStorageSettings extends Model protected $attributes = [ 'active' => false, 'provider' => 's3', + 'use_instance_role' => false, 'use_path_style' => false, ]; @@ -63,6 +66,7 @@ class ExternalStorageSettings extends Model return [ 'active' => 'boolean', 'provider' => StorageProvider::class, + 'use_instance_role' => 'boolean', 'secret' => 'encrypted', 'key_file' => 'encrypted', 'use_path_style' => 'boolean', @@ -88,8 +92,14 @@ class ExternalStorageSettings extends Model // What counts as "filled in" is per provider, because the two // authenticate with different things entirely: S3 wants a key and // a secret, GCS wants a service account key file. + // + // Unless S3 is being asked to authenticate as the machine it is + // running on, in which case there is no credential to fill in at + // all and demanding one would leave the disk permanently + // "unconfigured" — which fails silently, by leaving every new + // upload on the local disk rather than by reporting anything. return match ($this->provider) { - StorageProvider::S3 => $this->filled('key') && $this->filled('secret'), + StorageProvider::S3 => $this->use_instance_role || ($this->filled('key') && $this->filled('secret')), StorageProvider::Gcs => $this->filled('key_file'), }; } diff --git a/database/migrations/2026_09_10_090000_add_use_instance_role_to_external_storage_settings_table.php b/database/migrations/2026_09_10_090000_add_use_instance_role_to_external_storage_settings_table.php new file mode 100644 index 00000000..08be8f3b --- /dev/null +++ b/database/migrations/2026_09_10_090000_add_use_instance_role_to_external_storage_settings_table.php @@ -0,0 +1,35 @@ +boolean('use_instance_role')->default(false)->after('provider'); + }); + } + + public function down(): void + { + Schema::table('external_storage_settings', function (Blueprint $table) { + $table->dropColumn('use_instance_role'); + }); + } +}; diff --git a/resources/js/pages/system/settings/storage.tsx b/resources/js/pages/system/settings/storage.tsx index 536c1e48..61c2b630 100644 --- a/resources/js/pages/system/settings/storage.tsx +++ b/resources/js/pages/system/settings/storage.tsx @@ -17,6 +17,7 @@ import AppLayout from '@/layouts/app-layout'; interface StorageSettingsProps { active: boolean; provider: string; + use_instance_role: boolean; access_key: string; has_secret: boolean; has_key_file: boolean; @@ -33,6 +34,7 @@ const FORM_ID = 'storage-settings-form'; export default function StorageSettings({ active, provider, + use_instance_role, access_key, has_secret, has_key_file, @@ -54,6 +56,7 @@ export default function StorageSettings({ const { data, setData, patch, processing, recentlySuccessful, errors } = useForm({ active: active, provider: provider, + use_instance_role: use_instance_role, access_key: access_key, secret: '', key_file: '', @@ -65,6 +68,7 @@ export default function StorageSettings({ }); const isGcs = data.provider === 'gcs'; + const usesInstanceRole = !isGcs && data.use_instance_role; const submit: FormEventHandler = (e) => { e.preventDefault(); @@ -83,6 +87,7 @@ export default function StorageSettings({ route('system-settings.storage.test'), { provider: data.provider, + use_instance_role: data.use_instance_role, access_key: data.access_key, secret: data.secret, key_file: data.key_file, @@ -158,24 +163,56 @@ export default function StorageSettings({ ) : ( -
-
- - setData('access_key', e.target.value)} /> - -
-
- - setData('secret', e.target.value)} + <> +
+ setData('use_instance_role', checked === true)} /> - +
+ +

+ {t( + 'For installations running on AWS with a role already attached — an ECS task role, an EC2 instance profile, EKS/IRSA. ProjectSend asks the AWS SDK for temporary credentials instead of storing an access key. Leave this off for MinIO, Backblaze, Wasabi and anything else that needs a key and secret.', + )} +

+ {usesInstanceRole && has_secret && ( +

+ {t('Saving will delete the access key and secret currently stored here.')} +

+ )} + +
-
+ + {!usesInstanceRole && ( +
+
+ + setData('access_key', e.target.value)} + /> + +
+
+ + setData('secret', e.target.value)} + /> + +
+
+ )} + )}
diff --git a/tests/Feature/Platform/ExternalStorageSettingsTest.php b/tests/Feature/Platform/ExternalStorageSettingsTest.php index 5128adee..44b7e62c 100644 --- a/tests/Feature/Platform/ExternalStorageSettingsTest.php +++ b/tests/Feature/Platform/ExternalStorageSettingsTest.php @@ -41,7 +41,7 @@ test('the secret never reaches the cache store', function () { Cache::flush(); app(ExternalStorageConfigApplier::class)->apply(); - $cached = Cache::get('platform.external_storage_settings.v3'); + $cached = Cache::get('platform.external_storage_settings.v4'); expect($cached)->toBeArray() ->and(json_encode($cached))->not->toContain('super-secret-access-key'); @@ -178,6 +178,67 @@ test('a real upload lands on the external disk once the backend is active, and l expect($localFile->disk)->toBe('files'); }); +test('apply() supplies no credentials at all when authenticating as the server role', function () { + // Nothing to leave blank and nothing to read: Laravel only builds a + // `credentials` entry for the S3 client when both halves are + // non-empty, and the AWS SDK falls back to its default credential + // provider chain when none is given. An empty string here would be a + // credential, and would fail instead of falling through. + ExternalStorageSettings::current()->fill([ + 'active' => true, + 'use_instance_role' => true, + 'bucket' => 'my-bucket', + 'region' => 'us-east-1', + ])->save(); + + app(ExternalStorageConfigApplier::class)->flush(); + app(ExternalStorageConfigApplier::class)->apply(); + + expect(config('filesystems.disks.files_external.key'))->toBeNull() + ->and(config('filesystems.disks.files_external.secret'))->toBeNull() + ->and(config('filesystems.disks.files_external.bucket'))->toBe('my-bucket') + ->and(config('filesystems.disks.files_external.region'))->toBe('us-east-1'); +}); + +test('a leftover stored secret is never applied once the server role is authenticating', function () { + // The row is written straight here, bypassing the controller that + // clears the credential — the runtime must not fall back to a secret + // it was told to stop using. + ExternalStorageSettings::current()->fill([ + 'active' => true, + 'use_instance_role' => true, + 'key' => 'AKIALEFTOVER', + 'secret' => 'leftover-secret', + 'bucket' => 'my-bucket', + 'region' => 'us-east-1', + ])->save(); + + app(ExternalStorageConfigApplier::class)->flush(); + app(ExternalStorageConfigApplier::class)->apply(); + + expect(config('filesystems.disks.files_external.key'))->toBeNull() + ->and(config('filesystems.disks.files_external.secret'))->toBeNull(); +}); + +test('new uploads go to the external disk with no stored credentials when the server role is authenticating', function () { + // The silent failure this guards: isConfigured() demanding a key and + // a secret would leave the disk "unconfigured" forever, and every + // upload would keep landing on the local disk without saying so. + ExternalStorageSettings::current()->fill([ + 'active' => true, + 'use_instance_role' => true, + 'bucket' => 'my-bucket', + 'region' => 'us-east-1', + ])->save(); + + app(ExternalStorageConfigApplier::class)->flush(); + + $event = new ResolvingUploadDisk($this->admin); + Event::dispatch($event); + + expect($event->disk)->toBe('files_external'); +}); + test('staff can save external storage settings', function () { config()->set('projectsend.edition', Edition::Community); @@ -206,6 +267,70 @@ test('saving with a blank secret keeps the previously stored secret', function ( ->and($settings->bucket)->toBe('renamed-bucket'); }); +test('saving with the server role asked for needs no access key, and deletes the stored credentials', function () { + $this->actingAs($this->admin)->patch('/system/settings/storage', validStorageSettingsPayload([ + 'access_key' => 'AKIAEXAMPLE', + 'secret' => 'shh', + ]))->assertRedirect(); + + $payload = validStorageSettingsPayload(['use_instance_role' => true]); + unset($payload['access_key'], $payload['secret']); + + $this->actingAs($this->admin)->patch('/system/settings/storage', $payload) + ->assertSessionHasNoErrors(); + + $settings = ExternalStorageSettings::current(); + expect($settings->use_instance_role)->toBeTrue() + ->and($settings->key)->toBeNull() + ->and($settings->secret)->toBeNull() + ->and($settings->isConfigured())->toBeTrue(); + + // And nothing is left in the row for a database dump to carry. + expect(DB::table('external_storage_settings')->value('secret'))->toBeNull(); +}); + +test('an access key is still required when it is missing rather than blank', function () { + // Not the same case as an empty string: a validation rule that only + // runs on a present attribute would let this through. + $payload = validStorageSettingsPayload(); + unset($payload['access_key']); + + $this->actingAs($this->admin)->patch('/system/settings/storage', $payload) + ->assertSessionHasErrors(['access_key']); +}); + +test('the connection test runs without an access key when the server role is authenticating', function () { + // The button has to accept the same configuration the save does — + // otherwise there is no way to check an IAM-role setup before + // switching uploads over to it, which is the whole point of it. + // + // Credentials are put in the environment so the SDK's default chain + // resolves instantly from there rather than reaching for the EC2 + // metadata service, and the endpoint is a closed local port so the + // request fails at once. This asserts the request is *made*, offline + // and in well under a second — not that a bucket exists. + putenv('AWS_ACCESS_KEY_ID=AKIAENVEXAMPLE'); + putenv('AWS_SECRET_ACCESS_KEY=env-secret'); + + try { + $response = $this->actingAs($this->admin)->post('/system/settings/storage/test', [ + 'provider' => 's3', + 'use_instance_role' => true, + 'bucket' => 'my-bucket', + 'region' => 'us-east-1', + 'endpoint' => 'http://127.0.0.1:1', + 'use_path_style' => true, + ]); + + $response->assertRedirect()->assertSessionHasNoErrors(); + + expect(session('storage_test_result'))->toBeString(); + } finally { + putenv('AWS_ACCESS_KEY_ID'); + putenv('AWS_SECRET_ACCESS_KEY'); + } +}); + test('saving external storage settings rejects invalid input', function () { $this->actingAs($this->admin)->patch('/system/settings/storage', validStorageSettingsPayload([ 'access_key' => '', diff --git a/tests/Feature/Platform/GoogleCloudStorageTest.php b/tests/Feature/Platform/GoogleCloudStorageTest.php index fe4c4640..a24cdd8c 100644 --- a/tests/Feature/Platform/GoogleCloudStorageTest.php +++ b/tests/Feature/Platform/GoogleCloudStorageTest.php @@ -183,7 +183,7 @@ test('the service account key never reaches the cache store', function () { Cache::flush(); app(ExternalStorageConfigApplier::class)->apply(); - $cached = Cache::get('platform.external_storage_settings.v3'); + $cached = Cache::get('platform.external_storage_settings.v4'); // Asserted to exist before it is searched: a renamed cache key would // otherwise make this pass by finding nothing at all, which is how a