diff --git a/backend/Cargo.lock b/backend/Cargo.lock index ea2a6eedde..21e390f4ea 100644 --- a/backend/Cargo.lock +++ b/backend/Cargo.lock @@ -13885,6 +13885,8 @@ dependencies = [ "futures", "gethostname", "git-version", + "hex", + "hmac", "lazy_static", "once_cell", "opentelemetry 0.30.0", diff --git a/backend/Cargo.toml b/backend/Cargo.toml index d367cde7eb..85b129ea0b 100644 --- a/backend/Cargo.toml +++ b/backend/Cargo.toml @@ -326,6 +326,8 @@ async-nats.workspace = true aws-sdk-sqs.workspace = true aws-config.workspace = true aws-credential-types.workspace = true +hmac.workspace = true +hex.workspace = true [workspace.dependencies] diff --git a/backend/tests/app_s3_onbehalf.rs b/backend/tests/app_s3_onbehalf.rs index 2e1085edd3..9dd96e30f1 100644 --- a/backend/tests/app_s3_onbehalf.rs +++ b/backend/tests/app_s3_onbehalf.rs @@ -144,6 +144,131 @@ async fn test_deployed_app_s3_onbehalf_provenance(db: Pool) -> anyhow: Ok(()) } +/// Mint a presigned bearer (`exp=..&sig=..`) exactly as `sign_s3_objects` does: +/// `HMAC-SHA256(workspace_key, "file_key={s3}&exp={exp}")` (no storage param, since +/// these routes send none). `validate_s3_signature` is `private`-gated, so this test +/// only runs with the `private` feature. +#[cfg(feature = "private")] +fn mint_presigned(workspace_key: &str, s3: &str, exp: i64) -> String { + use hmac::{Hmac, Mac}; + use sha2::Sha256; + let mut mac = Hmac::::new_from_slice(workspace_key.as_bytes()).unwrap(); + mac.update(format!("file_key={s3}&exp={exp}").as_bytes()); + let sig = hex::encode(mac.finalize().into_bytes()); + format!("exp={exp}&sig={sig}") +} + +/// A presigned S3 object (bearer minted by `signS3Objects`) bypasses the provenance +/// gate on EVERY app-scoped display route, not just the raw `download_s3_file` +/// download: a valid signature clears the gate on preview/count/metadata/csv routes, +/// while an unsigned key stays denied and a forged/expired signature is rejected. +#[cfg(feature = "private")] +#[sqlx::test(fixtures("base"))] +async fn test_deployed_app_s3_presigned_bypasses_gate(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + let ws = format!("http://localhost:{port}/api/w/test-workspace"); + + let resp = authed(client().post(format!("{ws}/apps/create")), ADMIN_TOKEN) + .json(&json!({ + "path": APP, + "summary": "s3 presigned test", + "value": {}, + "policy": { + "execution_mode": "anonymous", + "triggerables": {}, + "allowed_s3_keys": [{ "s3_path": DECLARED }] + } + })) + .send() + .await?; + assert_eq!(resp.status(), 201, "app create: {}", resp.text().await?); + + let workspace_key: String = sqlx::query_scalar( + "SELECT key FROM workspace_key WHERE workspace_id = 'test-workspace' AND kind = 'cloud'", + ) + .fetch_one(&db) + .await?; + let exp = chrono::Utc::now().timestamp() + 3600; + let presigned = mint_presigned(&workspace_key, NON_PROVENANCE, exp); + + let get = |route: String, token: &'static str| { + let url = format!("{ws}/apps_u/{route}"); + authed(client().get(url), token).send() + }; + let denied = |body: &str| body.contains("File restricted"); + + // Control: NON_PROVENANCE without a signature is denied by the gate. + let body = get( + format!("download_s3_file/{APP}?s3={NON_PROVENANCE}"), + USER_TOKEN, + ) + .await? + .text() + .await?; + assert!( + denied(&body), + "unsigned non-provenance key must be denied: {body}" + ); + + // Every display route: a valid presigned key clears the gate (falls through to + // the storage read, which fails with a storage error, never "File restricted"). + // `read_bytes_*` are required on load_file_preview. + let routes = [ + format!("download_s3_file/{APP}?s3={NON_PROVENANCE}&{presigned}"), + format!("load_table_count/{APP}?file_key={NON_PROVENANCE}&{presigned}"), + format!("load_csv_preview/{APP}?file_key={NON_PROVENANCE}&limit=5&offset=0&{presigned}"), + format!("load_parquet_preview/{APP}?file_key={NON_PROVENANCE}&limit=5&offset=0&{presigned}"), + format!("load_file_metadata/{APP}?file_key={NON_PROVENANCE}&{presigned}"), + format!( + "load_file_preview/{APP}?file_key={NON_PROVENANCE}&read_bytes_from=0&read_bytes_length=4096&{presigned}" + ), + format!("download_s3_parquet_file_as_csv/{APP}?file_key={NON_PROVENANCE}&{presigned}"), + ]; + for route in routes { + let body = get(route.clone(), USER_TOKEN).await?.text().await?; + assert!( + !denied(&body), + "presigned key must bypass the gate on {route}: {body}" + ); + } + + // A tampered signature must NOT bypass: presence of `sig` commits to validation, + // so a wrong sig is rejected outright ("Invalid signature") rather than falling + // back to the provenance gate. + let forged = format!("exp={exp}&sig={}", "00".repeat(32)); + let body = get( + format!("download_s3_file/{APP}?s3={NON_PROVENANCE}&{forged}"), + USER_TOKEN, + ) + .await? + .text() + .await?; + assert!( + body.contains("Invalid signature"), + "forged signature must be rejected: {body}" + ); + + // An expired-but-valid signature is rejected on expiry, not bypassed. + let past = chrono::Utc::now().timestamp() - 10; + let expired = mint_presigned(&workspace_key, NON_PROVENANCE, past); + let body = get( + format!("download_s3_file/{APP}?s3={NON_PROVENANCE}&{expired}"), + USER_TOKEN, + ) + .await? + .text() + .await?; + assert!( + body.contains("Signature expired"), + "expired signature must be rejected: {body}" + ); + + Ok(()) +} + /// Seed a completed job whose result carries an s3 object. `app_trigger` sets the /// app-origination marker exactly as `execute_component` stamps it: `Some(app_path)` /// => `trigger_kind = 'app'` + `trigger = ` (an app-launched run); diff --git a/backend/windmill-api/openapi.yaml b/backend/windmill-api/openapi.yaml index 96b3f96bd6..4df03ae467 100644 --- a/backend/windmill-api/openapi.yaml +++ b/backend/windmill-api/openapi.yaml @@ -12139,6 +12139,8 @@ paths: in: query schema: type: string + - $ref: "#/components/parameters/S3Sig" + - $ref: "#/components/parameters/S3Exp" responses: "200": description: FileMetadata @@ -12191,6 +12193,8 @@ paths: in: query schema: type: string + - $ref: "#/components/parameters/S3Sig" + - $ref: "#/components/parameters/S3Exp" responses: "200": description: FilePreview @@ -12241,6 +12245,8 @@ paths: in: query schema: type: string + - $ref: "#/components/parameters/S3Sig" + - $ref: "#/components/parameters/S3Exp" responses: "200": description: Parquet Preview @@ -12293,6 +12299,8 @@ paths: in: query schema: type: string + - $ref: "#/components/parameters/S3Sig" + - $ref: "#/components/parameters/S3Exp" responses: "200": description: Csv Preview @@ -12325,6 +12333,8 @@ paths: in: query schema: type: string + - $ref: "#/components/parameters/S3Sig" + - $ref: "#/components/parameters/S3Exp" responses: "200": description: Table count @@ -12354,6 +12364,8 @@ paths: in: query schema: type: string + - $ref: "#/components/parameters/S3Sig" + - $ref: "#/components/parameters/S3Exp" responses: "200": description: The downloaded file @@ -22660,6 +22672,18 @@ components: required: true schema: type: string + S3Sig: + name: sig + in: query + description: HMAC signature of a presigned S3 object (bypasses the app provenance gate) + schema: + type: string + S3Exp: + name: exp + in: query + description: Expiry timestamp of a presigned S3 object signature + schema: + type: string CustomPath: name: custom_path in: path diff --git a/backend/windmill-api/src/apps.rs b/backend/windmill-api/src/apps.rs index 8169a42677..fe10229af3 100644 --- a/backend/windmill-api/src/apps.rs +++ b/backend/windmill-api/src/apps.rs @@ -3896,6 +3896,10 @@ async fn check_if_allowed_to_access_s3_file_from_app( windmill_api_auth::scopes::has_app_embed_sentinel(authed.scopes.as_deref()) }); + // A valid presigned bearer is a self-authorizing capability, so it short-circuits + // the provenance gate. OSS builds cannot validate signatures (no workspace-key + // HMAC), so there the bearer is ignored and the request falls through to the + // checks below — the same path these routes took before presigning. if file_query.sig.is_some() { #[cfg(feature = "private")] { @@ -3908,13 +3912,11 @@ async fn check_if_allowed_to_access_s3_file_from_app( &db, ) .await?; - Ok(()) + return Ok(()); } - #[cfg(not(feature = "private"))] - return Err(Error::InternalErr( - "Internal error: signature validation is not supported in open source mode".to_string(), - )); - } else if matches!(policy.execution_mode, ExecutionMode::Viewer) && !is_app_embed { + } + + if matches!(policy.execution_mode, ExecutionMode::Viewer) && !is_app_embed { // Viewer mode: the on-behalf identity IS the viewer, so the downstream // get_workspace_s3_resource_and_check_paths already bounds the read by // their own perms — no provenance gate (it would over-restrict). Embed @@ -4051,14 +4053,25 @@ async fn download_s3_file_from_app( .await } +// Presigned bearer params (`exp=..&sig=..`) extracted as a second `Query` so the +// app-scoped preview/count/metadata routes honor a presigned key the same way the +// raw `download_s3_file` route does. #[cfg(feature = "parquet")] -fn app_s3_file_query(s3: String, storage: Option) -> AppS3FileQuery { +#[derive(Deserialize)] +struct AppS3Sig { + sig: Option, + #[cfg(feature = "private")] + exp: Option, +} + +#[cfg(feature = "parquet")] +fn app_s3_file_query(s3: String, storage: Option, sig: AppS3Sig) -> AppS3FileQuery { AppS3FileQuery { s3, storage, - sig: None, + sig: sig.sig, #[cfg(feature = "private")] - exp: None, + exp: sig.exp, } } @@ -4154,9 +4167,10 @@ async fn app_download_s3_parquet_file_as_csv( Extension(db): Extension, Path((w_id, path)): Path<(String, StripPath)>, Query(query): Query, + Query(sig): Query, ) -> Result { let path = path.to_path(); - let file_query = app_s3_file_query(query.file_key.clone(), query.storage.clone()); + let file_query = app_s3_file_query(query.file_key.clone(), query.storage.clone(), sig); let job_authed = app_s3_on_behalf_and_provenance(&db, &path, &w_id, &opt_authed, &file_query).await?; crate::job_helpers_oss::download_s3_parquet_file_as_csv_internal( @@ -4179,9 +4193,10 @@ async fn app_load_file_metadata( Extension(db): Extension, Path((w_id, path)): Path<(String, StripPath)>, Query(query): Query, + Query(sig): Query, ) -> Result { let path = path.to_path(); - let file_query = app_s3_file_query(query.file_key.clone(), query.storage.clone()); + let file_query = app_s3_file_query(query.file_key.clone(), query.storage.clone(), sig); let job_authed = app_s3_on_behalf_and_provenance(&db, &path, &w_id, &opt_authed, &file_query).await?; let resp = @@ -4195,9 +4210,10 @@ async fn app_load_file_preview( Extension(db): Extension, Path((w_id, path)): Path<(String, StripPath)>, Query(query): Query, + Query(sig): Query, ) -> Result { let path = path.to_path(); - let file_query = app_s3_file_query(query.file_key.clone(), query.storage.clone()); + let file_query = app_s3_file_query(query.file_key.clone(), query.storage.clone(), sig); let job_authed = app_s3_on_behalf_and_provenance(&db, &path, &w_id, &opt_authed, &file_query).await?; let resp = @@ -4211,10 +4227,11 @@ async fn app_load_table_count( Extension(db): Extension, Path((w_id, path)): Path<(String, StripPath)>, Query(query): Query, + Query(sig): Query, ) -> Result { let path = path.to_path(); let (file_key, inner) = query.into_inner(); - let file_query = app_s3_file_query(file_key.clone(), inner.storage.clone()); + let file_query = app_s3_file_query(file_key.clone(), inner.storage.clone(), sig); let job_authed = app_s3_on_behalf_and_provenance(&db, &path, &w_id, &opt_authed, &file_query).await?; let resp = @@ -4229,10 +4246,11 @@ async fn app_load_parquet_preview( Extension(db): Extension, Path((w_id, path)): Path<(String, StripPath)>, Query(query): Query, + Query(sig): Query, ) -> Result { let path = path.to_path(); let (file_key, inner) = query.into_inner(); - let file_query = app_s3_file_query(file_key.clone(), inner.storage.clone()); + let file_query = app_s3_file_query(file_key.clone(), inner.storage.clone(), sig); let job_authed = app_s3_on_behalf_and_provenance(&db, &path, &w_id, &opt_authed, &file_query).await?; let resp = crate::job_helpers_oss::load_preview_internal( @@ -4248,10 +4266,11 @@ async fn app_load_csv_preview( Extension(db): Extension, Path((w_id, path)): Path<(String, StripPath)>, Query(query): Query, + Query(sig): Query, ) -> Result { let path = path.to_path(); let (file_key, inner) = query.into_inner(); - let file_query = app_s3_file_query(file_key.clone(), inner.storage.clone()); + let file_query = app_s3_file_query(file_key.clone(), inner.storage.clone(), sig); let job_authed = app_s3_on_behalf_and_provenance(&db, &path, &w_id, &opt_authed, &file_query).await?; let resp = crate::job_helpers_oss::load_preview_internal( diff --git a/frontend/src/lib/components/DisplayResult.svelte b/frontend/src/lib/components/DisplayResult.svelte index 4e0c5ef8aa..ae44015cb8 100644 --- a/frontend/src/lib/components/DisplayResult.svelte +++ b/frontend/src/lib/components/DisplayResult.svelte @@ -1060,6 +1060,7 @@ {appPath} s3resource={s3object?.s3} storage={s3object?.storage} + presigned={s3object?.presigned} /> {/key} {:else if s3object?.s3?.endsWith('.png') || s3object?.s3?.endsWith('.jpeg') || s3object?.s3?.endsWith('.jpg') || s3object?.s3?.endsWith('.webp')} @@ -1116,6 +1117,7 @@ {appPath} s3resource={s3object?.s3} storage={s3object?.storage} + presigned={s3object?.presigned} />{:else}