diff --git a/backend/windmill-api-settings/src/lib.rs b/backend/windmill-api-settings/src/lib.rs index a7e1322ad9..8339df7b76 100644 --- a/backend/windmill-api-settings/src/lib.rs +++ b/backend/windmill-api-settings/src/lib.rs @@ -406,7 +406,11 @@ async fn validate_object_storage_test(settings: &ObjectSettings) -> error::Resul ) } ObjectSettings::Gcs(gcs) => { - if gcs.service_account_key.is_empty() { + // Mirror `build_gcs_client`'s blank-key check (shared predicate): a blank/`{}` key falls + // back to the instance's ambient credentials there, so it must be rejected here too — + // otherwise an untrusted caller could probe with the server's identity (the very + // SSRF/credential-exfil this function guards against). + if windmill_object_store::gcs_service_account_key_is_blank(&gcs.service_account_key) { return Err(error::Error::NotAuthorized( "Testing GCS storage without a service account key requires a super admin" .to_string(), @@ -2187,6 +2191,26 @@ mod object_storage_test_hardening { ); } + #[tokio::test] + async fn rejects_gcs_blank_service_account_key() { + // A blank key makes build_gcs_client fall back to the instance's ambient credentials, so an + // untrusted caller must not be allowed to test with it. The `serviceAccountKey` field is + // serialized via serde's `as_string` (`to_string` of the JSON value), so the settings UI's + // "no key" empty object arrives as `"{}"` and a null as `"null"` — both must be rejected. + for key in [serde_json::json!({}), serde_json::json!(null)] { + let settings: ObjectSettings = serde_json::from_value(serde_json::json!({ + "type": "Gcs", + "bucket": "b", + "serviceAccountKey": key + })) + .unwrap(); + assert!( + validate_object_storage_test(&settings).await.is_err(), + "blank key {key:?} should be rejected" + ); + } + } + fn ip(s: &str) -> IpAddr { s.parse().unwrap() } diff --git a/backend/windmill-object-store/src/lib.rs b/backend/windmill-object-store/src/lib.rs index cf30efe116..553eb69917 100644 --- a/backend/windmill-object-store/src/lib.rs +++ b/backend/windmill-object-store/src/lib.rs @@ -494,6 +494,23 @@ fn build_azure_blob_client( return Ok(Arc::new(store)); } +/// Whether a GCS `service_account_key` carries no static credentials, in which case the client +/// should fall back to the instance's ambient credentials (GKE Workload Identity / metadata server) +/// instead of being handed an unparseable key. Besides an empty/whitespace string, the settings UI +/// stores "no key" as an empty JSON object `{}` (and `serde_json` may yield `null`), so treat those +/// as absent too. Shared with the connectivity-test SSRF guard so both agree on what "no key" means. +pub fn gcs_service_account_key_is_blank(service_account_key: &str) -> bool { + let trimmed = service_account_key.trim(); + if trimmed.is_empty() { + return true; + } + match serde_json::from_str::(trimmed) { + Ok(serde_json::Value::Null) => true, + Ok(serde_json::Value::Object(map)) => map.is_empty(), + _ => false, + } +} + #[cfg(feature = "parquet")] async fn build_gcs_client(gcs_resource_ref: &GcsResource) -> error::Result> { let gcs_resource = gcs_resource_ref.clone(); @@ -509,7 +526,12 @@ async fn build_gcs_client(gcs_resource_ref: &GcsResource) -> error::Result