diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index 878eca802f..79337fb350 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -c10f9194d243403cf0f85f72b9b5eaf6949d39fe \ No newline at end of file +c9fc5ccbb8a581dd4a62059c0ac28e59502c9ef1 \ No newline at end of file diff --git a/backend/tests/preserve_on_behalf_of.rs b/backend/tests/preserve_on_behalf_of.rs index d808348937..c15c516775 100644 --- a/backend/tests/preserve_on_behalf_of.rs +++ b/backend/tests/preserve_on_behalf_of.rs @@ -2810,3 +2810,169 @@ async fn test_schedule_permissions_superadmin_not_in_workspace( Ok(()) } + +// ============================================================================ +// Forged-superadmin on_behalf_of guard (GHSA-hfh4-cx4h-3fcr) +// ============================================================================ + +/// A `wm_deployers` member must not be able to deploy a runnable whose preserved +/// on_behalf_of resolves to a superadmin identity it does not genuinely name: +/// the reserved internal sentinels, or a superadmin's email pinned onto an +/// unrelated principal. Deploying on behalf of a *consistently named* real user — +/// even a real superadmin, e.g. a git-sync round-trip of superadmin-authored +/// content — stays allowed; that is the intended deployer capability. +#[sqlx::test(fixtures("preserve_on_behalf_of"))] +async fn test_reject_forged_superadmin_on_behalf_of(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + let base = format!("http://localhost:{port}/api/w/test-workspace"); + + // Reserved internal sentinel (grants is_super_admin at execution by email). + const SENTINEL: &str = "superadmin_secret@windmill.dev"; + // Reserved sentinel matched on permissioned_as. + const SYNC_SENTINEL: &str = "superadmin_sync@windmill.dev"; + // Real instance superadmin, present only in `password` (not a workspace member). + const REAL_SA: &str = "superadmin-external@windmill.dev"; + + // App: deployer cannot pin the sentinel email. + let resp = authed( + client().post(format!("{base}/apps/create")), + "DEPLOYER_TOKEN", + ) + .json(&new_app_with_on_behalf_of( + "u/deployer-user/app_sentinel", + Some("u/original-user"), + Some(SENTINEL), + true, + )) + .send() + .await?; + assert_eq!( + resp.status(), + 400, + "deployer must not pin the sentinel as an app on_behalf_of_email: {}", + resp.text().await? + ); + + // App: a real superadmin on_behalf_of is *allowed* at deploy (deployers may + // deploy on behalf of any real user). The escalation is closed at execution + // by the job-token cap, not by restricting what can be stored, so even a + // superadmin email pinned onto an unrelated principal deploys fine here. + let resp = authed( + client().post(format!("{base}/apps/create")), + "DEPLOYER_TOKEN", + ) + .json(&new_app_with_on_behalf_of( + "u/deployer-user/app_real_sa", + Some("u/original-user"), + Some(REAL_SA), + true, + )) + .send() + .await?; + assert_eq!( + resp.status(), + 201, + "a real superadmin on_behalf_of is allowed at deploy (capped at execution): {}", + resp.text().await? + ); + + // App: a consistently named real superadmin identity is likewise allowed. + let resp = authed( + client().post(format!("{base}/apps/create")), + "DEPLOYER_TOKEN", + ) + .json(&new_app_with_on_behalf_of( + "u/deployer-user/app_consistent_sa", + Some("u/superadmin-external"), + Some(REAL_SA), + true, + )) + .send() + .await?; + assert_eq!( + resp.status(), + 201, + "deployer may preserve a consistently named superadmin identity: {}", + resp.text().await? + ); + + // Flow: deployer cannot pin the sentinel email. + let resp = authed( + client().post(format!("{base}/flows/create")), + "DEPLOYER_TOKEN", + ) + .json(&new_flow_with_on_behalf_of( + "u/deployer-user/flow_sentinel", + Some(SENTINEL), + true, + )) + .send() + .await?; + assert_eq!( + resp.status(), + 400, + "deployer must not pin the sentinel as a flow on_behalf_of_email: {}", + resp.text().await? + ); + + // Script: deployer cannot pin the sentinel email. + let resp = authed( + client().post(format!("{base}/scripts/create")), + "DEPLOYER_TOKEN", + ) + .json(&new_script_with_on_behalf_of( + "u/deployer-user/script_sentinel", + Some(SENTINEL), + true, + )) + .send() + .await?; + assert_eq!( + resp.status(), + 400, + "deployer must not pin the sentinel as a script on_behalf_of_email: {}", + resp.text().await? + ); + + // Schedule: deployer cannot preserve the sync sentinel as permissioned_as. + let resp = authed( + client().post(format!("{base}/scripts/create")), + "DEPLOYER_TOKEN", + ) + .json(&new_script_with_on_behalf_of( + "u/deployer-user/sched_guard_script", + None, + false, + )) + .send() + .await?; + assert_eq!(resp.status(), 201, "{}", resp.text().await?); + + let resp = authed( + client().post(format!("{base}/schedules/create")), + "DEPLOYER_TOKEN", + ) + .json(&json!({ + "path": "u/deployer-user/schedule_sync_sentinel", + "schedule": "0 0 */6 * * *", + "timezone": "UTC", + "script_path": "u/deployer-user/sched_guard_script", + "is_flow": false, + "enabled": false, + "permissioned_as": SYNC_SENTINEL, + "preserve_permissioned_as": true + })) + .send() + .await?; + assert_eq!( + resp.status(), + 400, + "deployer must not preserve the sync sentinel as a schedule permissioned_as: {}", + resp.text().await? + ); + + Ok(()) +} diff --git a/backend/tests/wm_token_superadmin_guard.rs b/backend/tests/wm_token_superadmin_guard.rs index 5518a9bd9d..b3a0db4eab 100644 --- a/backend/tests/wm_token_superadmin_guard.rs +++ b/backend/tests/wm_token_superadmin_guard.rs @@ -307,3 +307,60 @@ async fn test_wm_token_rejected_by_direct_super_admin_gates( Ok(()) } + +/// The instance-level `devops` role must be capped like superadmin. +/// `is_devops_email` returns true for superadmin emails, so every +/// `require_devops_role` route (worker management, instance config, service logs) +/// is reachable by exactly the same superadmin `WM_TOKEN` unless it is capped too +/// (GHSA-hfh4-cx4h-3fcr). +#[sqlx::test(fixtures("preserve_on_behalf_of"))] +async fn test_wm_token_rejected_by_require_devops_role(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + set_jwt_secret().await; + + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + let base = format!("http://localhost:{port}/api"); + + let sa_wm = wm_token("test@windmill.dev", true).await; + let resp = authed(client().get(format!("{base}/service_logs/list_files")), &sa_wm) + .send() + .await?; + assert_eq!( + resp.status(), + 401, + "superadmin WM_TOKEN must not reach a require_devops_role route: {}", + resp.text().await? + ); + + // No false positive: a real superadmin API token (no job_id) still reaches it. + let resp = authed( + client().get(format!("{base}/service_logs/list_files")), + "SECRET_TOKEN", + ) + .send() + .await?; + assert_eq!( + resp.status(), + 200, + "a real superadmin token must still reach the devops route: {}", + resp.text().await? + ); + + // The advisory's own PoC route: the full user directory, gated solely by + // `require_super_admin` with no per-route job-token denylist. + let resp = authed( + client().get(format!("{base}/users/list_as_super_admin")), + &sa_wm, + ) + .send() + .await?; + assert_eq!( + resp.status(), + 401, + "superadmin WM_TOKEN must not list all users: {}", + resp.text().await? + ); + + Ok(()) +} diff --git a/backend/windmill-api-auth/src/auth.rs b/backend/windmill-api-auth/src/auth.rs index 0806fc2bd9..72bf3e8527 100644 --- a/backend/windmill-api-auth/src/auth.rs +++ b/backend/windmill-api-auth/src/auth.rs @@ -223,7 +223,19 @@ impl AuthCache { read_only: false, job_id: None, }; - let job_id = claims.job_id.and_then(|j| uuid::Uuid::from_str(&j).ok()); + // Fail closed: a `job_id` claim that does not parse must reject + // the token rather than resolve to `None`, which would clear the + // job provenance and uncap the token (GHSA-hfh4-cx4h-3fcr). + let job_id = match claims.job_id { + Some(j) => match uuid::Uuid::from_str(&j) { + Ok(job_id) => Some(job_id), + Err(_) => { + tracing::error!("JWT auth error: job_id claim is not a uuid"); + return None; + } + }, + None => None, + }; AUTH_CACHE.insert( key, ExpiringAuthCache { diff --git a/backend/windmill-api-auth/src/lib.rs b/backend/windmill-api-auth/src/lib.rs index 0a9eee4af5..eb9cf2de53 100644 --- a/backend/windmill-api-auth/src/lib.rs +++ b/backend/windmill-api-auth/src/lib.rs @@ -542,10 +542,24 @@ pub fn build_scope_path_predicate( } } -pub async fn require_devops_role(db: &DB, email: &str) -> error::Result<()> { - let is_devops = is_devops_email(db, email).await?; - - if is_devops { +/// Assert the caller holds the instance-level `devops` role under their own +/// credentials. +/// +/// `devops` is instance-level and [`is_devops_email`] is also true for +/// superadmins, so this gate is reachable by the same job token that +/// [`require_super_admin`] rejects, and is capped the same way +/// (GHSA-hfh4-cx4h-3fcr). +pub async fn require_devops_role(db: &DB, authed: &ApiAuthed) -> error::Result<()> { + if authed.job_id.is_some() { + return Err(Error::NotAuthorized( + "This endpoint cannot be called with a job token ($WM_TOKEN). If a script \ + genuinely needs this, create a dedicated token from the User settings drawer \ + (the 'Tokens' section), store it as a secret, and use that token explicitly \ + instead of $WM_TOKEN." + .to_owned(), + )); + } + if is_devops_email(db, &authed.email).await? { Ok(()) } else { Err(Error::NotAuthorized( diff --git a/backend/windmill-api-configs/src/lib.rs b/backend/windmill-api-configs/src/lib.rs index f776712063..bf7fb6c540 100644 --- a/backend/windmill-api-configs/src/lib.rs +++ b/backend/windmill-api-configs/src/lib.rs @@ -117,7 +117,7 @@ async fn get_config( Path(name): Path, Extension(db): Extension, ) -> error::JsonResult> { - require_devops_role(&db, &authed.email).await?; + require_devops_role(&db, &authed).await?; let config = sqlx::query_as!(Config, "SELECT name, config FROM config WHERE name = $1", name) .fetch_optional(&db) @@ -133,7 +133,7 @@ async fn update_config( authed: ApiAuthed, Json(config): Json, ) -> error::Result { - require_devops_role(&db, &authed.email).await?; + require_devops_role(&db, &authed).await?; #[cfg(not(feature = "enterprise"))] let config = if name.starts_with("worker__") { @@ -212,7 +212,7 @@ async fn delete_config( Extension(db): Extension, authed: ApiAuthed, ) -> error::Result { - require_devops_role(&db, &authed.email).await?; + require_devops_role(&db, &authed).await?; let mut tx = db.begin().await?; @@ -280,7 +280,7 @@ async fn native_kubernetes_autoscaling_healthcheck( authed: ApiAuthed, Extension(db): Extension, ) -> Result<(), windmill_autoscaling::kubernetes_integration_ee::KubeError> { - require_devops_role(&db, &authed.email).await.map_err(|e| { + require_devops_role(&db, &authed).await.map_err(|e| { windmill_autoscaling::kubernetes_integration_ee::KubeError::Other(e.to_string()) })?; @@ -317,7 +317,7 @@ async fn list_configs( authed: ApiAuthed, Extension(db): Extension, ) -> error::JsonResult> { - require_devops_role(&db, &authed.email).await?; + require_devops_role(&db, &authed).await?; let configs = sqlx::query_as!(Config, "SELECT name, config FROM config") .fetch_all(&db) .await?; @@ -342,7 +342,7 @@ async fn list_all_workspace_dependencies( authed: ApiAuthed, Extension(db): Extension, ) -> error::JsonResult> { - require_devops_role(&db, &authed.email).await?; + require_devops_role(&db, &authed).await?; let deps = sqlx::query!( r#"SELECT workspace_id, name, language AS "language: windmill_common::scripts::ScriptLang" FROM workspace_dependencies @@ -374,7 +374,7 @@ async fn list_all_dedicated_with_deps( authed: ApiAuthed, Extension(db): Extension, ) -> error::JsonResult> { - require_devops_role(&db, &authed.email).await?; + require_devops_role(&db, &authed).await?; let rows = sqlx::query!( r#"SELECT DISTINCT ON (workspace_id, path) diff --git a/backend/windmill-api-flows/src/flows.rs b/backend/windmill-api-flows/src/flows.rs index a9376d02ed..691b8b02a0 100644 --- a/backend/windmill-api-flows/src/flows.rs +++ b/backend/windmill-api-flows/src/flows.rs @@ -591,6 +591,19 @@ async fn create_flow( } } + // Reject a forged superadmin run identity: only a preserved value is + // caller-controlled (otherwise `resolve_on_behalf_of_email` stores the + // deployer's own email below). A flow stores no permissioned_as, so this is + // the sentinel guard. + if nf.preserve_on_behalf_of.unwrap_or(false) + && windmill_common::can_preserve_on_behalf_of(&authed) + { + windmill_common::auth::validate_on_behalf_of( + None, + nf.on_behalf_of_email.as_deref(), + )?; + } + let mut tx = user_db.clone().begin(&authed).await?; check_path_conflict(&mut tx, &w_id, &nf.path).await?; @@ -1031,6 +1044,17 @@ async fn update_flow( validate_flow(&nf).await?; + // Reject a forged superadmin run identity in a preserved value (otherwise + // `resolve_on_behalf_of_email` stores the deployer's own email below). + if nf.preserve_on_behalf_of.unwrap_or(false) + && windmill_common::can_preserve_on_behalf_of(&authed) + { + windmill_common::auth::validate_on_behalf_of( + None, + nf.on_behalf_of_email.as_deref(), + )?; + } + let authed = maybe_refresh_folders(&flow_path, &w_id, authed, &db).await; let mut tx = user_db.clone().begin(&authed).await?; diff --git a/backend/windmill-api-schedule/src/lib.rs b/backend/windmill-api-schedule/src/lib.rs index cd79725b74..fb92acff83 100644 --- a/backend/windmill-api-schedule/src/lib.rs +++ b/backend/windmill-api-schedule/src/lib.rs @@ -306,6 +306,13 @@ async fn create_schedule( ) .await?; + // Reject a forged superadmin run identity in a preserved permissioned_as + // (the sentinel guard; the email is derived from it so it always belongs). + windmill_common::auth::validate_on_behalf_of( + Some(&resolved_permissioned_as), + Some(&resolved_email), + )?; + let schedule = sqlx::query_as!( Schedule, r#" @@ -518,6 +525,13 @@ async fn edit_schedule( authed.email.clone() }; + // Reject a forged superadmin run identity in a preserved permissioned_as + // (the sentinel guard; the email is derived from it so it always belongs). + windmill_common::auth::validate_on_behalf_of( + Some(&resolved_permissioned_as), + Some(&resolved_email), + )?; + let schedule = sqlx::query_as!( Schedule, r#" diff --git a/backend/windmill-api-scripts/src/scripts.rs b/backend/windmill-api-scripts/src/scripts.rs index fd9899ea10..fa1e63f853 100644 --- a/backend/windmill-api-scripts/src/scripts.rs +++ b/backend/windmill-api-scripts/src/scripts.rs @@ -978,6 +978,17 @@ async fn create_script_internal<'c>( } } } + // Reject a forged superadmin run identity in a preserved value (otherwise + // `resolve_on_behalf_of_email` stores the deployer's own email below). A + // script stores no permissioned_as, so this is the sentinel guard. + if ns.preserve_on_behalf_of.unwrap_or(false) + && windmill_common::can_preserve_on_behalf_of(&authed) + { + windmill_common::auth::validate_on_behalf_of( + None, + ns.on_behalf_of_email.as_deref(), + )?; + } if sqlx::query_scalar!( "SELECT 1 FROM script WHERE hash = $1 AND workspace_id = $2", hash.0, diff --git a/backend/windmill-api-settings/src/lib.rs b/backend/windmill-api-settings/src/lib.rs index 53f7c634e0..ca4504b9bf 100644 --- a/backend/windmill-api-settings/src/lib.rs +++ b/backend/windmill-api-settings/src/lib.rs @@ -1275,7 +1275,7 @@ async fn restart_worker_group( authed: ApiAuthed, Path(worker_group): Path, ) -> error::Result { - require_devops_role(&db, &authed.email).await?; + require_devops_role(&db, &authed).await?; sqlx::query!( "INSERT INTO notify_event (channel, payload) VALUES ('restart_worker_group', $1)", @@ -1443,7 +1443,7 @@ pub async fn get_critical_alerts( authed: ApiAuthed, Query(params): Query, ) -> JsonResult { - require_devops_role(&db, &authed.email).await?; + require_devops_role(&db, &authed).await?; windmill_alerting::get_critical_alerts(db, params, None).await } @@ -1459,7 +1459,7 @@ pub async fn acknowledge_critical_alert( authed: ApiAuthed, Path(id): Path, ) -> error::Result { - require_devops_role(&db, &authed.email).await?; + require_devops_role(&db, &authed).await?; windmill_alerting::acknowledge_critical_alert(db, None, id).await } diff --git a/backend/windmill-api-workers/src/lib.rs b/backend/windmill-api-workers/src/lib.rs index d65316555e..a7a9b67358 100644 --- a/backend/windmill-api-workers/src/lib.rs +++ b/backend/windmill-api-workers/src/lib.rs @@ -102,7 +102,7 @@ async fn list_worker_pings( Extension(user_db): Extension, Query(query): Query, ) -> JsonResult> { - let has_devops_role = require_devops_role(&db, &authed.email).await.is_ok(); + let has_devops_role = require_devops_role(&db, &authed).await.is_ok(); if *HIDE_WORKERS_FOR_NON_ADMINS && !has_devops_role { return Ok(Json(vec![])); } @@ -158,7 +158,7 @@ async fn exists_workers_with_tags( // When TAGS_ARE_SENSITIVE is enabled, filter tags based on workspace visibility if *TAGS_ARE_SENSITIVE { - let has_devops_role = require_devops_role(&db, &authed.email).await.is_ok(); + let has_devops_role = require_devops_role(&db, &authed).await.is_ok(); if !has_devops_role { if let Some(ref workspace) = tags_query.workspace { // Filter to only tags visible in this workspace @@ -212,7 +212,7 @@ async fn get_custom_tags( return Ok(Json(all_tags)); } if *TAGS_ARE_SENSITIVE { - let has_devops_role = require_devops_role(&db, &authed.email).await.is_ok(); + let has_devops_role = require_devops_role(&db, &authed).await.is_ok(); if !has_devops_role { return Ok(Json(vec![])); } @@ -249,7 +249,7 @@ async fn get_queue_metrics( authed: ApiAuthed, Extension(db): Extension, ) -> JsonResult> { - require_devops_role(&db, &authed.email).await?; + require_devops_role(&db, &authed).await?; let queue_metrics = sqlx::query_as!( QueueMetric, @@ -274,7 +274,7 @@ async fn get_queue_counts( authed: ApiAuthed, Extension(db): Extension, ) -> JsonResult> { - require_devops_role(&db, &authed.email).await?; + require_devops_role(&db, &authed).await?; let queue_counts = windmill_common::queue::get_queue_counts(&db).await; Ok(Json(queue_counts)) } @@ -283,7 +283,7 @@ async fn get_queue_running_counts( authed: ApiAuthed, Extension(db): Extension, ) -> JsonResult> { - require_devops_role(&db, &authed.email).await?; + require_devops_role(&db, &authed).await?; let queue_running_counts = windmill_common::queue::get_queue_running_counts(&db).await; Ok(Json(queue_running_counts)) } @@ -308,7 +308,7 @@ async fn get_workspace_fairness_events( authed: ApiAuthed, Extension(db): Extension, ) -> JsonResult> { - require_devops_role(&db, &authed.email).await?; + require_devops_role(&db, &authed).await?; // No cloud-host gate — workspace fairness is an Enterprise feature // available on any multi-tenant EE deployment. Non-EE / non-enabled diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index 093d533618..1e9bf11327 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -3768,9 +3768,8 @@ async fn list_workspaces_as_super_admin( Extension(db): Extension, Extension(user_db): Extension, Query(pagination): Query, - ApiAuthed { email, .. }: ApiAuthed, ) -> JsonResult> { - require_devops_role(&db, &email).await?; + require_devops_role(&db, &authed).await?; let (per_page, offset) = paginate(pagination); let mut tx = user_db.begin(&authed).await?; diff --git a/backend/windmill-api/src/apps.rs b/backend/windmill-api/src/apps.rs index 596d28ed64..1a4d334de8 100644 --- a/backend/windmill-api/src/apps.rs +++ b/backend/windmill-api/src/apps.rs @@ -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 = 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)) } diff --git a/backend/windmill-api/src/service_logs.rs b/backend/windmill-api/src/service_logs.rs index b1902f7c1a..99dc7c41a8 100644 --- a/backend/windmill-api/src/service_logs.rs +++ b/backend/windmill-api/src/service_logs.rs @@ -43,12 +43,12 @@ pub struct LogFile { pub json_fmt: bool, } async fn list_files( - ApiAuthed { email, .. }: ApiAuthed, + authed: ApiAuthed, Extension(db): Extension, Query(pagination): Query, Query(lq): Query, ) -> JsonResult> { - 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, Path(path): Path, ) -> windmill_common::error::Result { 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())); diff --git a/backend/windmill-common/src/auth.rs b/backend/windmill-common/src/auth.rs index bd6bf1ef9d..ea77f16454 100644 --- a/backend/windmill-common/src/auth.rs +++ b/backend/windmill-common/src/auth.rs @@ -301,6 +301,49 @@ pub async fn is_super_admin_email<'c>(db: impl sqlx::PgExecutor<'c>, email: &str Ok(is_admin) } +/// The three reserved internal identities that grant instance-superadmin at +/// execution: `superadmin_secret@` / `superadmin_notification@` (matched on the +/// email) and `superadmin_sync@` (matched on `permissioned_as`). They belong to +/// no real user, so a stored `on_behalf_of` (app policy, flow/script, +/// schedule, trigger) must never carry one as either field — it would be a +/// forged superadmin run identity. Mirror of the `is_super_admin` derivation in +/// [`fetch_authed_from_permissioned_as_inner`]. +pub fn is_reserved_on_behalf_of_identity( + permissioned_as: Option<&str>, + on_behalf_of_email: Option<&str>, +) -> bool { + const RESERVED: [&str; 3] = [ + SUPERADMIN_SECRET_EMAIL, + SUPERADMIN_NOTIFICATION_EMAIL, + SUPERADMIN_SYNC_EMAIL, + ]; + [permissioned_as, on_behalf_of_email] + .into_iter() + .flatten() + .any(|v| RESERVED.contains(&v)) +} + +/// Guard a caller-supplied `on_behalf_of` before it is persisted on a deployable +/// object (app policy, flow/script, schedule, trigger): reject the reserved +/// internal sentinels, which no legitimate deploy ever carries. The actual +/// escalation is closed at execution by the job-token cap in +/// [`require_super_admin`] — even a superadmin `on_behalf_of` yields a token +/// capped at workspace admin — so this is a cheap, non-breaking early guard, not +/// the primary defense. It deliberately does *not* restrict deploying on behalf +/// of a real user (including a real superadmin, e.g. git-sync of +/// superadmin-authored content), which is the intended `wm_deployers` capability. +pub fn validate_on_behalf_of( + permissioned_as: Option<&str>, + on_behalf_of_email: Option<&str>, +) -> Result<()> { + if is_reserved_on_behalf_of_identity(permissioned_as, on_behalf_of_email) { + return Err(Error::BadRequest( + "on_behalf_of cannot be a reserved internal identity".to_string(), + )); + } + Ok(()) +} + pub async fn is_devops_email(db: &DB, email: &str) -> Result { if is_super_admin_email(db, email).await? { return Ok(true); @@ -678,7 +721,40 @@ pub mod aws { #[cfg(test)] mod tests { - use super::is_user_token; + use super::{is_reserved_on_behalf_of_identity, is_user_token}; + use crate::users::{ + SUPERADMIN_NOTIFICATION_EMAIL, SUPERADMIN_SECRET_EMAIL, SUPERADMIN_SYNC_EMAIL, + }; + + #[test] + fn reserved_on_behalf_of_identity_matches_every_sentinel_in_either_field() { + // Matched on the email (secret / notification) or on permissioned_as (sync). + assert!(is_reserved_on_behalf_of_identity( + None, + Some(SUPERADMIN_SECRET_EMAIL) + )); + assert!(is_reserved_on_behalf_of_identity( + None, + Some(SUPERADMIN_NOTIFICATION_EMAIL) + )); + assert!(is_reserved_on_behalf_of_identity( + Some(SUPERADMIN_SYNC_EMAIL), + None + )); + // A sentinel smuggled as a raw-email permissioned_as (schedules/triggers + // derive the email from it) is caught too. + assert!(is_reserved_on_behalf_of_identity( + Some(SUPERADMIN_SECRET_EMAIL), + None + )); + // Ordinary identities pass. + assert!(!is_reserved_on_behalf_of_identity(None, None)); + assert!(!is_reserved_on_behalf_of_identity( + Some("u/alice"), + Some("alice@example.com") + )); + assert!(!is_reserved_on_behalf_of_identity(Some("g/team"), None)); + } #[test] fn user_tokens_are_editable() { diff --git a/backend/windmill-trigger/src/handler.rs b/backend/windmill-trigger/src/handler.rs index ed3b89fc65..d8f5ebca55 100644 --- a/backend/windmill-trigger/src/handler.rs +++ b/backend/windmill-trigger/src/handler.rs @@ -520,6 +520,14 @@ async fn create_trigger( } } + // Reject a forged superadmin run identity in a preserved permissioned_as + // (the sentinel guard; a trigger's email is derived from it at execution). + let resolved_permissioned_as = new_trigger.base.resolve_permissioned_as(&authed); + windmill_common::auth::validate_on_behalf_of( + Some(&resolved_permissioned_as), + None, + )?; + let on_behalf_of_info = windmill_common::check_on_behalf_of_preservation( new_trigger.base.permissioned_as.as_deref(), new_trigger.base.preserve_permissioned_as.unwrap_or(false), @@ -754,6 +762,15 @@ async fn update_trigger( let new_path = edit_trigger.base.path.to_string(); let labels = edit_trigger.base.labels.clone(); + + // Reject a forged superadmin run identity in a preserved permissioned_as + // (the sentinel guard; a trigger's email is derived from it at execution). + let resolved_permissioned_as = edit_trigger.base.resolve_permissioned_as(&authed); + windmill_common::auth::validate_on_behalf_of( + Some(&resolved_permissioned_as), + None, + )?; + let on_behalf_of_info = windmill_common::check_on_behalf_of_preservation( edit_trigger.base.permissioned_as.as_deref(), edit_trigger.base.preserve_permissioned_as.unwrap_or(false), diff --git a/docs/followup-onbehalf-execution-privilege-hardening.md b/docs/followup-onbehalf-execution-privilege-hardening.md index 3574270199..77c336fbc7 100644 --- a/docs/followup-onbehalf-execution-privilege-hardening.md +++ b/docs/followup-onbehalf-execution-privilege-hardening.md @@ -8,12 +8,19 @@ A job's `WM_TOKEN` (whose identity is an app/flow/schedule/trigger `on_behalf_of that a `wm_deployers` member controls) could satisfy superadmin authorization. #10124 makes a `WM_TOKEN` never count as a global superadmin: -- `ApiAuthed.job_id` is stamped from the resolved token; `require_super_admin(db, &ApiAuthed)` - and `is_super_admin_authed(db, &ApiAuthed)` reject `job_id.is_some()`. -- All `require_super_admin` sites and the direct `is_super_admin_email(&authed.email)` - boolean gates on **request handlers** (workspace deletion, fork drops, dev-workspace - attach/archive, object-storage SSRF exemption, custom dbname, EE GHES + connected - repositories, CUSTOM_INSTANCE_DB) were migrated. +- `ApiAuthed.job_id` is stamped from the resolved token; `require_super_admin(db, &ApiAuthed)`, + `is_super_admin_authed(db, &ApiAuthed)` and `require_devops_role(db, &ApiAuthed)` reject + `job_id.is_some()`. +- All `require_super_admin` / `require_devops_role` sites and the direct + `is_super_admin_email(&authed.email)` boolean gates on **request handlers** (workspace + deletion, fork drops, dev-workspace attach/archive, object-storage SSRF exemption, custom + dbname, EE GHES + connected repositories, CUSTOM_INSTANCE_DB) were migrated. +- A `job_id` claim that does not parse as a uuid now rejects the token instead of + resolving to `None` (which would have cleared the provenance and uncapped it). +- Defense in depth at store time: `validate_on_behalf_of` refuses the reserved internal + sentinels (`superadmin_secret@`, `superadmin_notification@`, `superadmin_sync@`) as an + `on_behalf_of` on apps/flows/scripts/schedules/triggers, and app execution refuses a + policy carrying one — which also covers already-persisted and forked-app rows. ## What is deliberately left for this follow-up @@ -58,20 +65,14 @@ job's *preserved on-behalf email* rather than from a request `ApiAuthed`, so the `is_super_admin`/`is_job_token` bool from callers. Only independently authenticated superadmins should get the exemption. -### 3. `is_devops_email` / `require_devops_role` - -- `windmill-common/src/auth.rs` (~L305) `is_devops_email` returns true for superadmin - emails, so a superadmin-email `WM_TOKEN` passes as devops on worker-management/config/ - service-log routes. Lower severity; make the devops check job-token-aware too. - ## Guiding principle `on_behalf_of` is attacker-influenced (a `wm_deployers` member sets it). It must never grant *global* superadmin privileges. Where a privilege decision happens on a request, -gate it on `ApiAuthed.job_id` (`require_super_admin` / `is_super_admin_authed`). Where it -happens at execution on a stored/preserved identity, validate the privilege at -**create/update time against the real actor** instead, or thread job-token provenance -into the execution path. +gate it on `ApiAuthed.job_id` (`require_super_admin` / `is_super_admin_authed` / +`require_devops_role`). Where it happens at execution on a stored/preserved identity, +validate the privilege at **create/update time against the real actor** instead, or thread +job-token provenance into the execution path. ## Tests to add