mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-07 08:02:40 +00:00
fix(apps): deep-match provenance + declarable s3 read scopes for on-behalf app reads
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
cfc3f292ad
commit
ecd45de7aa
+2
-2
@@ -1,6 +1,6 @@
|
||||
{
|
||||
"db_name": "PostgreSQL",
|
||||
"query": "SELECT EXISTS (\n SELECT 1 FROM v2_job_completed c JOIN v2_job j USING (id)\n WHERE j.workspace_id = $2\n AND c.started_at > now() - interval '3 hours'\n AND c.result @> ('{\"s3\":\"' || $1 || '\"}')::jsonb\n AND j.trigger_kind = 'app'\n AND j.trigger = $3\n AND j.created_by = $4\n )",
|
||||
"query": "SELECT EXISTS (\n SELECT 1 FROM v2_job_completed c JOIN v2_job j USING (id)\n WHERE j.workspace_id = $2\n AND c.started_at > now() - interval '3 hours'\n AND jsonb_path_exists(c.result, '$.** ? (@.s3 == $k)', jsonb_build_object('k', $1::text))\n AND j.trigger_kind = 'app'\n AND j.trigger = $3\n AND j.created_by = $4\n )",
|
||||
"describe": {
|
||||
"columns": [
|
||||
{
|
||||
@@ -21,5 +21,5 @@
|
||||
null
|
||||
]
|
||||
},
|
||||
"hash": "75d1618e12c4dfe5e9632ba8dc45b5aeb572e51ebd0732e6ebf80197acb3e577"
|
||||
"hash": "6b23ae17cd480bf420145b82e46a2bf109d9115775509fb2f1646215094e6e70"
|
||||
}
|
||||
Generated
+1
@@ -13900,6 +13900,7 @@ dependencies = [
|
||||
"flate2",
|
||||
"futures",
|
||||
"git-version",
|
||||
"globset",
|
||||
"hex",
|
||||
"hmac",
|
||||
"http 1.4.2",
|
||||
|
||||
@@ -519,3 +519,186 @@ async fn test_preview_is_not_app_provenanced(db: Pool<Postgres>) -> anyhow::Resu
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Seed a completed app-marked job (`trigger_kind='app'` + `trigger=<app>`) by
|
||||
/// `created_by`, whose result is the caller-supplied JSON — used to seed s3 objects
|
||||
/// nested inside the result rather than at the top level.
|
||||
async fn seed_completed_job_with_result(
|
||||
db: &Pool<Postgres>,
|
||||
created_by: &str,
|
||||
app_trigger: &str,
|
||||
result_json: &str,
|
||||
) -> anyhow::Result<()> {
|
||||
sqlx::query(
|
||||
r#"
|
||||
WITH j AS (
|
||||
INSERT INTO v2_job (id, workspace_id, kind, runnable_path, created_by,
|
||||
permissioned_as, trigger_kind, trigger)
|
||||
VALUES (gen_random_uuid(), 'test-workspace', 'script', 'u/test-user/query_to_s3',
|
||||
$1, 'u/test-user', 'app'::job_trigger_kind, $2)
|
||||
RETURNING id
|
||||
)
|
||||
INSERT INTO v2_job_completed (id, workspace_id, duration_ms, status, result, started_at)
|
||||
SELECT id, 'test-workspace', 1, 'success', $3::jsonb, now() FROM j
|
||||
"#,
|
||||
)
|
||||
.bind(created_by)
|
||||
.bind(app_trigger)
|
||||
.bind(result_json)
|
||||
.execute(db)
|
||||
.await?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// A runnable may return its s3 object nested (e.g. `[{"s3":..}]` for a query result,
|
||||
/// or under a field) rather than as the top-level value. The provenance gate must
|
||||
/// still match the produced key wherever it sits in the result, otherwise the
|
||||
/// viewer's own app run is denied "File restricted".
|
||||
#[sqlx::test(fixtures("base"))]
|
||||
async fn test_deployed_app_s3_nested_result_provenance(db: Pool<Postgres>) -> 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");
|
||||
|
||||
const NESTED_APP: &str = "u/test-user/s3nested";
|
||||
const ARRAY_KEY: &str = "results/nested_array.parquet";
|
||||
const OBJECT_KEY: &str = "results/nested_object.parquet";
|
||||
const UNPRODUCED_KEY: &str = "results/never_produced.parquet";
|
||||
|
||||
let resp = authed(client().post(format!("{ws}/apps/create")), ADMIN_TOKEN)
|
||||
.json(&json!({
|
||||
"path": NESTED_APP,
|
||||
"summary": "s3 nested-result provenance test",
|
||||
"value": {},
|
||||
"policy": { "execution_mode": "anonymous", "triggerables": {} }
|
||||
}))
|
||||
.send()
|
||||
.await?;
|
||||
assert_eq!(resp.status(), 201, "app create: {}", resp.text().await?);
|
||||
|
||||
// The viewer's own app runs produced these keys, nested in the result.
|
||||
seed_completed_job_with_result(
|
||||
&db,
|
||||
"test-user-2",
|
||||
NESTED_APP,
|
||||
&format!(r#"[{{"s3":"{ARRAY_KEY}"}}]"#),
|
||||
)
|
||||
.await?;
|
||||
seed_completed_job_with_result(
|
||||
&db,
|
||||
"test-user-2",
|
||||
NESTED_APP,
|
||||
&format!(r#"{{"file":{{"s3":"{OBJECT_KEY}"}}}}"#),
|
||||
)
|
||||
.await?;
|
||||
|
||||
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");
|
||||
|
||||
let body = get(
|
||||
format!("download_s3_file/{NESTED_APP}?s3={ARRAY_KEY}"),
|
||||
USER_TOKEN,
|
||||
)
|
||||
.await?
|
||||
.text()
|
||||
.await?;
|
||||
assert!(
|
||||
!denied(&body),
|
||||
"s3 key nested in an array result must clear the gate: {body}"
|
||||
);
|
||||
|
||||
let body = get(
|
||||
format!("download_s3_file/{NESTED_APP}?s3={OBJECT_KEY}"),
|
||||
USER_TOKEN,
|
||||
)
|
||||
.await?
|
||||
.text()
|
||||
.await?;
|
||||
assert!(
|
||||
!denied(&body),
|
||||
"s3 key nested in an object result must clear the gate: {body}"
|
||||
);
|
||||
|
||||
// A key that no job produced stays denied — deep matching does not weaken the gate.
|
||||
let body = get(
|
||||
format!("download_s3_file/{NESTED_APP}?s3={UNPRODUCED_KEY}"),
|
||||
USER_TOKEN,
|
||||
)
|
||||
.await?
|
||||
.text()
|
||||
.await?;
|
||||
assert!(
|
||||
denied(&body),
|
||||
"a key no job produced must stay denied: {body}"
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// `s3_read_scopes` lets an app author declare globs of keys the app may read
|
||||
/// on-behalf for files it displays but does not produce in a viewer's session (e.g.
|
||||
/// results a separate job persisted to S3). In-scope keys clear the confused-deputy
|
||||
/// gate; out-of-scope keys stay denied. (The read is still bounded by the on-behalf
|
||||
/// identity's S3 permissions downstream — not exercised here, which asserts the gate.)
|
||||
#[sqlx::test(fixtures("base"))]
|
||||
async fn test_deployed_app_s3_read_scopes(db: Pool<Postgres>) -> 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");
|
||||
|
||||
const SCOPED_APP: &str = "u/test-user/s3scoped";
|
||||
const IN_SCOPE_KEY: &str = "f/sensitive/results/query.csv";
|
||||
const OUT_OF_SCOPE_KEY: &str = "f/other/secret.csv";
|
||||
|
||||
let resp = authed(client().post(format!("{ws}/apps/create")), ADMIN_TOKEN)
|
||||
.json(&json!({
|
||||
"path": SCOPED_APP,
|
||||
"summary": "s3 read-scopes test",
|
||||
"value": {},
|
||||
"policy": {
|
||||
"execution_mode": "anonymous",
|
||||
"triggerables": {},
|
||||
"s3_read_scopes": [{ "path_glob": "f/sensitive/**" }]
|
||||
}
|
||||
}))
|
||||
.send()
|
||||
.await?;
|
||||
assert_eq!(resp.status(), 201, "app create: {}", resp.text().await?);
|
||||
|
||||
let get = |route: String| {
|
||||
let url = format!("{ws}/apps_u/{route}");
|
||||
authed(client().get(url), USER_TOKEN).send()
|
||||
};
|
||||
let denied = |body: &str| body.contains("File restricted");
|
||||
|
||||
// In-scope key clears the gate even though no job produced it.
|
||||
let body = get(format!("download_s3_file/{SCOPED_APP}?s3={IN_SCOPE_KEY}"))
|
||||
.await?
|
||||
.text()
|
||||
.await?;
|
||||
assert!(
|
||||
!denied(&body),
|
||||
"a key matching a read scope must clear the gate: {body}"
|
||||
);
|
||||
|
||||
// A key outside the declared scope stays denied.
|
||||
let body = get(format!(
|
||||
"download_s3_file/{SCOPED_APP}?s3={OUT_OF_SCOPE_KEY}"
|
||||
))
|
||||
.await?
|
||||
.text()
|
||||
.await?;
|
||||
assert!(
|
||||
denied(&body),
|
||||
"a key outside every read scope must be denied: {body}"
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -87,6 +87,7 @@ tower-cookies.workspace = true
|
||||
tower-http.workspace = true
|
||||
hyper.workspace = true
|
||||
itertools.workspace = true
|
||||
globset.workspace = true
|
||||
reqwest.workspace = true
|
||||
serde.workspace = true
|
||||
sqlx.workspace = true
|
||||
|
||||
@@ -29167,6 +29167,23 @@ components:
|
||||
type: string
|
||||
resource:
|
||||
type: string
|
||||
s3_read_scopes:
|
||||
type: array
|
||||
description: >
|
||||
Author-declared literal globs (globset syntax, e.g. `f/sensitive/**`) of S3
|
||||
keys a deployed app may read on-behalf of its on_behalf_of identity for files
|
||||
it displays but does not produce in a viewer's session. Unlike the workspace
|
||||
advanced-permission rules, template variables (`{folder_read}` etc.) are not
|
||||
expanded, and `storage` is matched exactly (a scope with no storage covers the
|
||||
primary storage). Reads remain bounded by the on_behalf_of identity's advanced
|
||||
S3 permissions.
|
||||
items:
|
||||
type: object
|
||||
properties:
|
||||
path_glob:
|
||||
type: string
|
||||
storage:
|
||||
type: string
|
||||
execution_mode:
|
||||
type: string
|
||||
enum: [viewer, publisher, anonymous]
|
||||
|
||||
@@ -307,6 +307,23 @@ pub struct S3Key {
|
||||
storage: Option<String>,
|
||||
}
|
||||
|
||||
/// Author-declared surface of S3 keys a deployed app may read on-behalf of its
|
||||
/// `on_behalf_of` identity, for files the app displays but does NOT produce within
|
||||
/// a viewer's session (e.g. results a separate job persisted to S3). `path_glob` is a
|
||||
/// literal glob (globset syntax: `*` within a path segment, `**` across segments — e.g.
|
||||
/// `f/sensitive/**`); unlike the workspace advanced-permission rules it does NOT expand
|
||||
/// `{folder_read}`/`{username}`/`{group}` templates. `storage` is matched exactly, so a
|
||||
/// scope with no `storage` covers the primary storage only. This only widens the
|
||||
/// confused-deputy provenance gate; the read is still executed on-behalf of
|
||||
/// `on_behalf_of` and bounded by that identity's advanced S3 permissions downstream, so
|
||||
/// a scope can never grant more than the on-behalf identity already has.
|
||||
#[derive(Serialize, Deserialize, Debug, Clone)]
|
||||
pub struct S3ReadScope {
|
||||
path_glob: String,
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
storage: Option<String>,
|
||||
}
|
||||
|
||||
#[derive(Serialize, Deserialize, Debug, Clone, Default)]
|
||||
pub struct Policy {
|
||||
pub on_behalf_of: Option<String>,
|
||||
@@ -322,6 +339,8 @@ pub struct Policy {
|
||||
pub execution_mode: ExecutionMode,
|
||||
pub s3_inputs: Option<Vec<S3Input>>,
|
||||
pub allowed_s3_keys: Option<Vec<S3Key>>,
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
pub s3_read_scopes: Option<Vec<S3ReadScope>>,
|
||||
// WIN-2006: publisher opt-in to iframe sandbox isolation (alpha). When true the
|
||||
// app is isolated from each viewer's Windmill session: low-code renders in an
|
||||
// opaque-origin iframe with a scoped embed token, raw renders its bundle in an
|
||||
@@ -3398,6 +3417,7 @@ async fn upload_s3_file_from_app(
|
||||
.unwrap_or_default(),
|
||||
}]),
|
||||
allowed_s3_keys: None,
|
||||
s3_read_scopes: None,
|
||||
sandbox: None,
|
||||
})
|
||||
} else {
|
||||
@@ -3800,6 +3820,7 @@ async fn get_on_behalf_authed_from_app(
|
||||
on_behalf_of_email: None,
|
||||
s3_inputs: None,
|
||||
allowed_s3_keys: Some(force_allowed_s3_keys),
|
||||
s3_read_scopes: None,
|
||||
sandbox: None,
|
||||
}
|
||||
} else {
|
||||
@@ -3823,6 +3844,7 @@ async fn get_on_behalf_authed_from_app(
|
||||
on_behalf_of_email: None,
|
||||
s3_inputs: None,
|
||||
allowed_s3_keys: None,
|
||||
s3_read_scopes: None,
|
||||
sandbox: None,
|
||||
})
|
||||
};
|
||||
@@ -3875,12 +3897,24 @@ async fn check_if_allowed_to_access_s3_file_from_app(
|
||||
// tokens are excluded (untrusted app JS stays confined below).
|
||||
Ok(())
|
||||
} else {
|
||||
// Author-mode/embed: confine reads to the app's declared keys or files THIS
|
||||
// app produced, else a viewer could launder the author's S3 perms via an
|
||||
// arbitrary file_key (confused deputy). Provenance is the un-forgeable
|
||||
// app-origination marker (`trigger_kind='app'` + `trigger=<this app>`);
|
||||
// `created_by=<caller>` is ANDed only as a per-viewer isolation filter (it
|
||||
// can narrow — one viewer can't read another's result — never forge).
|
||||
// Author-mode/embed: confine reads to the app's declared keys/scopes or files
|
||||
// THIS app produced, else a viewer could launder the author's S3 perms via an
|
||||
// arbitrary file_key (confused deputy). Three ways a key clears — all still run
|
||||
// the read on-behalf of `on_behalf_of`, bounded by that identity's advanced S3
|
||||
// permissions downstream, so none can grant more than the on-behalf identity has:
|
||||
// 1. `allowed_s3_keys`: exact static keys (image/pdf/download components).
|
||||
// 2. `s3_read_scopes`: author-declared globs, for files the app displays but
|
||||
// does not produce in the viewer's session (e.g. results a separate job
|
||||
// persisted to S3).
|
||||
// 3. Provenance: an un-forgeable app-origination marker (`trigger_kind='app'` +
|
||||
// `trigger=<this app>`) on a recent completed job; `created_by=<caller>` is
|
||||
// ANDed only as a per-viewer isolation filter (it can narrow — one viewer
|
||||
// can't read another's result — never forge). The s3 key is matched anywhere
|
||||
// in the result (a runnable may return it nested, e.g. `[{"s3":..}]`), not
|
||||
// only at the top level. Because the match is deep, a component that echoes
|
||||
// viewer-controlled input into its result can surface any key that appears
|
||||
// there — the on-behalf identity's advanced-permission bound above is the
|
||||
// backstop that keeps this within that identity's own grants.
|
||||
let creator = opt_authed
|
||||
.as_ref()
|
||||
.map(|authed| authed.username.clone())
|
||||
@@ -3888,13 +3922,22 @@ async fn check_if_allowed_to_access_s3_file_from_app(
|
||||
let allowed = policy.allowed_s3_keys.as_ref().is_some_and(|keys| {
|
||||
keys.iter()
|
||||
.any(|key| key.s3_path == file_query.s3 && key.storage == file_query.storage)
|
||||
}) || policy.s3_read_scopes.as_ref().is_some_and(|scopes| {
|
||||
scopes.iter().any(|scope| {
|
||||
scope.storage == file_query.storage
|
||||
&& globset::Glob::new(&scope.path_glob)
|
||||
.map_err(|e| {
|
||||
tracing::error!("Invalid s3_read_scope glob {}: {e}", scope.path_glob)
|
||||
})
|
||||
.is_ok_and(|g| g.compile_matcher().is_match(&file_query.s3))
|
||||
})
|
||||
}) || {
|
||||
sqlx::query_scalar!(
|
||||
r#"SELECT EXISTS (
|
||||
SELECT 1 FROM v2_job_completed c JOIN v2_job j USING (id)
|
||||
WHERE j.workspace_id = $2
|
||||
AND c.started_at > now() - interval '3 hours'
|
||||
AND c.result @> ('{"s3":"' || $1 || '"}')::jsonb
|
||||
AND jsonb_path_exists(c.result, '$.** ? (@.s3 == $k)', jsonb_build_object('k', $1::text))
|
||||
AND j.trigger_kind = 'app'
|
||||
AND j.trigger = $3
|
||||
AND j.created_by = $4
|
||||
|
||||
@@ -1,9 +1,9 @@
|
||||
<script lang="ts">
|
||||
import { Alert } from '$lib/components/common'
|
||||
import { Alert, Button } from '$lib/components/common'
|
||||
import Badge from '$lib/components/common/badge/Badge.svelte'
|
||||
import Toggle from '$lib/components/Toggle.svelte'
|
||||
import { enterpriseLicense, userStore, workspaceStore } from '$lib/stores'
|
||||
import { Loader2 } from 'lucide-svelte'
|
||||
import { Loader2, Plus, X } from 'lucide-svelte'
|
||||
|
||||
import Tooltip from '$lib/components/Tooltip.svelte'
|
||||
|
||||
@@ -99,6 +99,16 @@
|
||||
|
||||
let dirtyPath = $state(false)
|
||||
|
||||
// `s3_read_scopes` is author-declared and must survive `updatePolicy` (which
|
||||
// recomputes `allowed_s3_keys`), so it is edited directly on `policy` and
|
||||
// persisted on the next deploy — reassign the array so the change is reactive.
|
||||
function addReadScope() {
|
||||
policy.s3_read_scopes = [...(policy.s3_read_scopes ?? []), { path_glob: '' }]
|
||||
}
|
||||
function removeReadScope(i: number) {
|
||||
policy.s3_read_scopes = (policy.s3_read_scopes ?? []).filter((_, idx) => idx !== i)
|
||||
}
|
||||
|
||||
async function appExists(customPath: string) {
|
||||
return await AppService.customPathExists({
|
||||
workspace: opWs!,
|
||||
@@ -287,6 +297,53 @@
|
||||
{/if}
|
||||
</Alert>
|
||||
|
||||
{#if policy.execution_mode !== 'viewer'}
|
||||
<div class="mt-8">
|
||||
<div class="flex items-center gap-2">
|
||||
<h2 class="text-sm font-semibold">S3 read paths (on-behalf)</h2>
|
||||
<Tooltip>
|
||||
Literal globs (e.g. <code>f/sensitive/**</code> — <code>*</code> within a path segment,
|
||||
<code>**</code> across segments) of S3 keys this deployed app may read on-behalf of its
|
||||
on-behalf-of identity, for files it displays but does not produce during a viewer's session
|
||||
(e.g. results a separate job persisted to S3). Applies to the primary storage. Unlike the
|
||||
workspace advanced-permission rules, template variables such as
|
||||
<code>{'{folder_read}'}</code>
|
||||
are not expanded. Reads are still bounded by the on-behalf identity's S3 permissions — a path
|
||||
here never grants more access than that identity already has. Files the app produces itself,
|
||||
or static file components, do not need an entry.
|
||||
</Tooltip>
|
||||
</div>
|
||||
<div class="text-xs text-secondary mt-1 mb-2">
|
||||
Leave empty if the app only shows files it produces itself. Takes effect on next deploy.
|
||||
</div>
|
||||
<div class="flex flex-col gap-2">
|
||||
{#each policy.s3_read_scopes ?? [] as _scope, i (i)}
|
||||
<div class="flex items-center gap-2">
|
||||
<div class="grow">
|
||||
<TextInput
|
||||
bind:value={policy.s3_read_scopes[i].path_glob}
|
||||
placeholder="f/sensitive/**"
|
||||
/>
|
||||
</div>
|
||||
<Button
|
||||
variant="subtle"
|
||||
unifiedSize="sm"
|
||||
iconOnly
|
||||
startIcon={{ icon: X }}
|
||||
title="Remove path"
|
||||
on:click={() => removeReadScope(i)}
|
||||
/>
|
||||
</div>
|
||||
{/each}
|
||||
</div>
|
||||
<div class="mt-2">
|
||||
<Button variant="default" unifiedSize="sm" startIcon={{ icon: Plus }} on:click={addReadScope}>
|
||||
Add path
|
||||
</Button>
|
||||
</div>
|
||||
</div>
|
||||
{/if}
|
||||
|
||||
<div class="mt-10"></div>
|
||||
|
||||
<div class="flex items-center gap-2">
|
||||
|
||||
Reference in New Issue
Block a user