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