mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-13 08:05:23 +00:00
fix: cap devops role at workspace admin and reject reserved on_behalf_of identities
Extends the job-token cap with three pieces: - `require_devops_role` takes `&ApiAuthed` and rejects job tokens. `is_devops_email` is true for superadmin emails, so every worker-management, instance-config and service-log route was reachable by the same superadmin `WM_TOKEN` that `require_super_admin` already rejects. - A `job_id` claim that does not parse as a uuid rejects the token rather than resolving to `None`, which would clear the job provenance and uncap it. Applies to the internal JWT and the external `jwt_ext_` path. - Defense in depth at store time: `validate_on_behalf_of` refuses the reserved internal sentinels as an `on_behalf_of` on apps/flows/scripts/schedules/triggers, and app execution refuses a policy carrying one — covering already-persisted and forked-app rows that predate the cap. Deploying on behalf of a real user, including a real superadmin, stays allowed; the cap handles that at execution. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
0b62ad31c0
commit
01ddbff6f6
@@ -1934,6 +1934,14 @@ async fn create_app_internal<'a>(
|
||||
}
|
||||
}
|
||||
|
||||
// Reject a forged superadmin run identity in the (possibly preserved) policy.
|
||||
// Done on the non-RLS pool before the transaction below, like the resolution
|
||||
// above, to avoid holding a second connection while `tx` is checked out.
|
||||
windmill_common::auth::validate_on_behalf_of(
|
||||
app.policy.on_behalf_of.as_deref(),
|
||||
app.policy.on_behalf_of_email.as_deref(),
|
||||
)?;
|
||||
|
||||
let mut tx = user_db.clone().begin(&authed).await?;
|
||||
let path = app.path.clone();
|
||||
if &app.path == "" {
|
||||
@@ -2445,6 +2453,22 @@ async fn update_app_internal<'a>(
|
||||
check_scopes(&authed, || format!("apps:write:{}", npath))?;
|
||||
}
|
||||
|
||||
// Reject a forged superadmin run identity in a preserved policy. Mirror the
|
||||
// `should_preserve` gate below (only a preserved value is caller-controlled;
|
||||
// otherwise the policy is rewritten to the deployer's own identity) and run
|
||||
// it on the non-RLS pool before the transaction to avoid a second connection.
|
||||
if let Some(npolicy) = ns.policy.as_ref() {
|
||||
let should_preserve = ns.preserve_on_behalf_of.unwrap_or(false)
|
||||
&& windmill_common::can_preserve_on_behalf_of(&authed)
|
||||
&& npolicy.on_behalf_of.is_some();
|
||||
if should_preserve {
|
||||
windmill_common::auth::validate_on_behalf_of(
|
||||
npolicy.on_behalf_of.as_deref(),
|
||||
npolicy.on_behalf_of_email.as_deref(),
|
||||
)?;
|
||||
}
|
||||
}
|
||||
|
||||
let mut tx = user_db.clone().begin(&authed).await?;
|
||||
|
||||
let mut preserved_on_behalf_of: Option<String> = None;
|
||||
@@ -4278,6 +4302,18 @@ fn get_on_behalf_of(policy: &Policy) -> Result<(String, String)> {
|
||||
)
|
||||
})?
|
||||
.to_string();
|
||||
// Defence in depth against a policy that already carries a forged superadmin
|
||||
// sentinel (deployed before validation existed, or copied verbatim by a
|
||||
// workspace fork): the sentinels are internal-only and never a legitimate app
|
||||
// run identity, so refuse to execute rather than mint a superadmin token.
|
||||
if windmill_common::auth::is_reserved_on_behalf_of_identity(
|
||||
Some(&permissioned_as),
|
||||
Some(&email),
|
||||
) {
|
||||
return Err(Error::BadRequest(
|
||||
"app on_behalf_of is a reserved internal identity and cannot be executed".to_string(),
|
||||
));
|
||||
}
|
||||
Ok((permissioned_as, email))
|
||||
}
|
||||
|
||||
|
||||
@@ -43,12 +43,12 @@ pub struct LogFile {
|
||||
pub json_fmt: bool,
|
||||
}
|
||||
async fn list_files(
|
||||
ApiAuthed { email, .. }: ApiAuthed,
|
||||
authed: ApiAuthed,
|
||||
Extension(db): Extension<DB>,
|
||||
Query(pagination): Query<Pagination>,
|
||||
Query(lq): Query<LogFileQuery>,
|
||||
) -> JsonResult<Vec<LogFile>> {
|
||||
require_devops_role(&db, &email).await?;
|
||||
require_devops_role(&db, &authed).await?;
|
||||
let (per_page, offset) = windmill_common::utils::paginate(pagination);
|
||||
|
||||
let mut sqlb = sql_builder::SqlBuilder::select_from("log_file")
|
||||
@@ -89,13 +89,13 @@ async fn list_files(
|
||||
}
|
||||
|
||||
async fn get_log_file(
|
||||
ApiAuthed { email, .. }: ApiAuthed,
|
||||
authed: ApiAuthed,
|
||||
Extension(db): Extension<DB>,
|
||||
Path(path): Path<windmill_common::utils::StripPath>,
|
||||
) -> windmill_common::error::Result<Response> {
|
||||
use windmill_common::tracing_init::TMP_WINDMILL_LOGS_SERVICE;
|
||||
|
||||
require_devops_role(&db, &email).await?;
|
||||
require_devops_role(&db, &authed).await?;
|
||||
let path = path.to_path();
|
||||
if path.contains("..") {
|
||||
return Err(Error::BadRequest("Invalid path".to_string()));
|
||||
|
||||
Reference in New Issue
Block a user