From 83ed011e264f20ffa66a7bf933f2fe3615cf6b67 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Tue, 30 Jun 2026 17:57:26 +0200 Subject: [PATCH] feat(object-store): make GCS service account key optional for Workload Identity (#9842) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit build_gcs_client always called `.with_service_account_key(...)`, so an absent key (the settings UI stores "no key" as the empty JSON object `{}`) was handed to the builder and failed to parse instead of falling through to the object_store crate's InstanceCredentialProvider. Skip the call when the key is blank so GCS uses the instance's ambient credentials (GKE Workload Identity / the GCP metadata server). "Blank" (empty/whitespace/`{}`/`null`) is centralized in a shared `gcs_service_account_key_is_blank` predicate so the build path and the non-super-admin connectivity-test SSRF guard (`validate_object_storage_test`) agree on what counts as "no key" — otherwise a blank key would bypass the guard yet still trigger the ambient-credential fallback, letting an untrusted caller probe arbitrary buckets with the server's instance role. Also clarify the settings UI hint that the key may be left empty for ambient credentials, and add regression tests for the blank-key build path and the guard. Fixes WIN-2110 Co-authored-by: Claude Opus 4.8 (1M context) --- backend/windmill-api-settings/src/lib.rs | 26 ++++++++- backend/windmill-object-store/src/lib.rs | 58 ++++++++++++++++++- .../ObjectStoreConfigSettings.svelte | 5 +- 3 files changed, 86 insertions(+), 3 deletions(-) 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