From 7285e50b76229586f0f04a4e90f1cd855f34ca53 Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Tue, 4 Aug 2026 19:32:26 +0200 Subject: [PATCH] fix(datatables): redact role passwords from settings exports and harden role naming --- .../src/datatable_permissions.rs | 100 ++++++-- .../windmill-api-workspaces/src/workspaces.rs | 10 +- backend/windmill-api/src/workspaces_export.rs | 5 +- backend/windmill-common/src/workspaces.rs | 235 +++++++++++++++--- .../src/lib/components/DBManagerDrawer.svelte | 4 +- 5 files changed, 290 insertions(+), 64 deletions(-) diff --git a/backend/windmill-api-workspaces/src/datatable_permissions.rs b/backend/windmill-api-workspaces/src/datatable_permissions.rs index 0274c3ebdd..2f8c6c6fd2 100644 --- a/backend/windmill-api-workspaces/src/datatable_permissions.rs +++ b/backend/windmill-api-workspaces/src/datatable_permissions.rs @@ -258,7 +258,31 @@ fn plan_role_changes( ))); } } - let renamed_away: HashSet<&str> = rename_src.values().copied().collect(); + + // Which old role each requested role continues: its rename source if it was + // renamed, else the old role of the same name. Matching on the requested name + // alone would spare an old role that a *different* role is being renamed onto, + // leaving the name occupied when the rename runs. + let claimed: HashSet<&str> = requested + .keys() + .map(|name| { + rename_src + .get(name.as_str()) + .copied() + .unwrap_or(name.as_str()) + }) + .collect(); + + // Drops come first so a name freed in this save can be reused by a rename or + // a create in the same save: the other order aborts on "role already exists". + for (name, role) in old_roles.iter() { + if name == ROOT_DATATABLE_ROLE || claimed.contains(name.as_str()) { + continue; + } + if let Some(pg_role) = role.pg_rolename.as_deref() { + drop_role(&mut statements, pg_role); + } + } let mut roles: BTreeMap = BTreeMap::new(); @@ -355,18 +379,6 @@ fn plan_role_changes( } } - for (name, role) in old_roles.iter() { - if name == ROOT_DATATABLE_ROLE - || renamed_away.contains(name.as_str()) - || requested.contains_key(name) - { - continue; - } - if let Some(pg_role) = role.pg_rolename.as_deref() { - drop_role(&mut statements, pg_role); - } - } - Ok(RolePlan { statements, permissions: DataTablePermissions { enabled: true, roles }, @@ -395,12 +407,14 @@ fn create_role_statement(pg_rolename: &str, password: &str) -> PlannedStatement /// unconditional: it stays a no-op in the common case instead of failing the /// whole transaction. fn grant_connect_statement(pg_rolename: &str, dbname: &str) -> PlannedStatement { + let role_lit = quote_literal(pg_rolename); + let db_lit = quote_literal(dbname); + // The names are passed to `format(%I)` rather than interpolated as quoted + // identifiers: this identifier sits inside a single-quoted EXECUTE string, so + // a `'` in the database name — which comes from a user-editable postgres + // resource — would otherwise close the literal and run as plpgsql of its own. PlannedStatement::plain(format!( - "DO $$ BEGIN\n IF NOT has_database_privilege({}, {}, 'CONNECT') THEN\n EXECUTE 'GRANT CONNECT ON DATABASE {} TO {}';\n END IF;\nEND $$;", - quote_literal(pg_rolename), - quote_literal(dbname), - quote_ident(dbname), - quote_ident(pg_rolename) + "DO $$ BEGIN\n IF NOT has_database_privilege({role_lit}, {db_lit}, 'CONNECT') THEN\n EXECUTE format('GRANT CONNECT ON DATABASE %I TO %I', {db_lit}, {role_lit});\n END IF;\nEND $$;" )) } @@ -786,13 +800,56 @@ mod tests { .unwrap(); assert_eq!( - sql(&plan).iter().filter(|s| s.starts_with("DROP ROLE")).count(), + sql(&plan) + .iter() + .filter(|s| s.starts_with("DROP ROLE")) + .count(), 2 ); assert!(!plan.permissions.enabled); assert!(plan.permissions.roles.is_empty()); } + /// A name freed by a delete must be reusable by a rename in the same save, + /// which only holds if the drops are planned first. + #[test] + fn a_name_freed_in_the_same_save_can_be_reused() { + let old = enabled_with(&["analyst", "reader"]); + let existing: Vec = ["analyst", "reader"] + .iter() + .map(|r| datatable_pg_role_name(W_ID, DT, r)) + .collect(); + // Delete `reader`, rename `analyst` into its place. + let req = SetDatatablePermissions { + enabled: true, + roles: vec![role("root", &[]), role("reader", &[])], + renames: vec![DatatableRoleRename { + from: "analyst".to_string(), + to: "reader".to_string(), + }], + }; + let plan = plan( + Some(&old), + &req, + &existing.iter().map(|s| s.as_str()).collect::>(), + ) + .unwrap(); + + let statements = sql(&plan); + let drop_at = statements + .iter() + .position(|s| s.starts_with("DROP ROLE")) + .expect("the freed role is dropped"); + let rename_at = statements + .iter() + .position(|s| s.starts_with("ALTER ROLE") && s.contains("RENAME TO")) + .expect("the surviving role is renamed"); + assert!( + drop_at < rename_at, + "drop must precede the rename that reuses the name: {statements:?}" + ); + } + #[test] fn a_role_the_config_lost_track_of_is_not_dropped() { let old = enabled_with(&["analyst"]); @@ -832,7 +889,10 @@ mod tests { roles: vec![role("root", &[]), role(bad_role, &[])], renames: vec![], }; - assert!(plan(None, &req, &[]).is_err(), "{bad_role} should be rejected"); + assert!( + plan(None, &req, &[]).is_err(), + "{bad_role} should be rejected" + ); } for bad_tenant in ["alice", "x/alice", "u/", ""] { let req = SetDatatablePermissions { diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index d2f3d2d863..90d7cd3004 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -45,10 +45,11 @@ use windmill_common::workspaces::GitRepositorySettings; use windmill_common::workspaces::WorkspaceDeploymentUISettings; use windmill_common::workspaces::{ check_deploy_rules, check_user_against_rule, get_datatable_resource_from_db, - get_datatable_resource_from_db_unchecked, DatatableAccess, - validate_dev_workspace_id, validate_fork_workspace_id, validate_workspace_name, DataTable, - DataTableCatalogResourceType, DataTableForkBehavior, ProtectionRuleKind, ProtectionRules, - ProtectionRuleset, RuleCheckResult, WorkspaceGitSyncSettings, DEV_WORKSPACE_LOCK_RULE_NAME, + get_datatable_resource_from_db_unchecked, redact_datatable_settings_for_export, + validate_dev_workspace_id, + validate_fork_workspace_id, validate_workspace_name, DataTable, DataTableCatalogResourceType, + DataTableForkBehavior, DatatableAccess, ProtectionRuleKind, ProtectionRules, ProtectionRuleset, + RuleCheckResult, WorkspaceGitSyncSettings, DEV_WORKSPACE_LOCK_RULE_NAME, }; use windmill_common::workspaces::{Ducklake, DucklakeCatalogResourceType}; use windmill_common::PgDatabase; @@ -944,6 +945,7 @@ async fn get_settings( if let Some(git_sync) = settings.git_sync.as_mut() { redact_git_sync_webhook_secrets(git_sync); } + settings.datatable = redact_datatable_settings_for_export(settings.datatable); Ok(Json(settings)) } diff --git a/backend/windmill-api/src/workspaces_export.rs b/backend/windmill-api/src/workspaces_export.rs index a7458e617a..5abb154f0f 100644 --- a/backend/windmill-api/src/workspaces_export.rs +++ b/backend/windmill-api/src/workspaces_export.rs @@ -47,6 +47,7 @@ use windmill_common::runnable_settings::{ConcurrencySettings, DebouncingSettings use windmill_common::scripts::ScriptRunnableSettingsHandle; use windmill_common::utils::require_admin; use windmill_common::variables::decrypt; +use windmill_common::workspaces::redact_datatable_settings_for_export; use windmill_common::worker::WINDMILL_DIR; use windmill_common::{ db::UserDB, @@ -1493,7 +1494,7 @@ pub(crate) async fn tarball_workspace( mute_critical_alerts: row.mute_critical_alerts, color: row.color.clone(), operator_settings: row.operator_settings.clone(), - datatable: row.datatable.clone(), + datatable: redact_datatable_settings_for_export(row.datatable.clone()), slack_team_id: row.slack_team_id.clone(), slack_name: row.slack_name.clone(), slack_command_script: row.slack_command_script.clone(), @@ -1563,7 +1564,7 @@ pub(crate) async fn tarball_workspace( mute_critical_alerts: row.mute_critical_alerts, color: row.color, operator_settings: row.operator_settings, - datatable: row.datatable, + datatable: redact_datatable_settings_for_export(row.datatable), slack_team_id: row.slack_team_id, slack_name: row.slack_name, slack_command_script: row.slack_command_script, diff --git a/backend/windmill-common/src/workspaces.rs b/backend/windmill-common/src/workspaces.rs index 50b5018f60..d956427bba 100644 --- a/backend/windmill-common/src/workspaces.rs +++ b/backend/windmill-common/src/workspaces.rs @@ -1036,16 +1036,58 @@ pub struct DataTableRole { pub tenants: Vec, } +/// Strip the generated role passwords out of a `workspace_settings.datatable` +/// value. +/// +/// Those passwords are direct database logins: anything that hands the settings +/// blob outside the server — the settings endpoint, the workspace tarball, a +/// git-synced `settings.yaml` — must go through this first, or the credential +/// lands somewhere far weaker than the role it protects (a repository's history, +/// a non-admin's export). +pub fn redact_datatable_settings_for_export( + datatable: Option, +) -> Option { + let mut datatable = datatable?; + let Some(datatables) = datatable + .get_mut("datatables") + .and_then(|d| d.as_object_mut()) + else { + return Some(datatable); + }; + for (_, dt) in datatables.iter_mut() { + let Some(roles) = dt + .get_mut("permissions") + .and_then(|p| p.get_mut("roles")) + .and_then(|r| r.as_object_mut()) + else { + continue; + }; + for (_, role) in roles.iter_mut() { + if let Some(role) = role.as_object_mut() { + role.remove("pg_password"); + } + } + } + Some(datatable) +} + /// Postgres caps identifiers at 63 bytes (NAMEDATALEN - 1) and silently /// truncates past it, which would collapse two distinct roles onto one. const PG_IDENTIFIER_MAX_LEN: usize = 63; /// The postgres role backing `role` on `w_id`'s `datatable`. /// -/// Postgres roles are cluster-wide while data table names are per-workspace, so -/// the workspace id has to be part of the name: without it two workspaces that -/// both hold a `main` data table with an `analyst` role would silently share one -/// role, and the second workspace's CREATE would fail. +/// Postgres roles are cluster-wide while data table and role names are scoped to +/// a workspace, so the name is `wm___` plus a hash of +/// the exact triple. Both halves are load-bearing: +/// +/// - Without the workspace id, two workspaces that both hold a `main` data table +/// with an `analyst` role would silently share one Postgres role. +/// - Without the hash, the readable half would not identify the triple: it is +/// sanitized (every character outside `[a-z0-9]` becomes `_`) and truncated to +/// fit Postgres' 63-byte identifier limit, so `analyst-1` and `analyst_1` — and +/// data table `sales_ro` + role `x` against data table `sales` + role `ro_x` — +/// would collapse onto the same role and share its grants. pub fn datatable_pg_role_name(w_id: &str, datatable: &str, role: &str) -> String { fn sanitize(s: &str) -> String { s.chars() @@ -1058,23 +1100,28 @@ pub fn datatable_pg_role_name(w_id: &str, datatable: &str, role: &str) -> String }) .collect() } - let name = format!( + use sha2::{Digest, Sha256}; + // NUL-joined so the digest cannot be replayed by moving characters across the + // field boundaries. + let mut hasher = Sha256::new(); + for part in [w_id, datatable, role] { + hasher.update(part.as_bytes()); + hasher.update([0u8]); + } + let digest = hasher.finalize(); + let discriminator = u32::from_be_bytes([digest[0], digest[1], digest[2], digest[3]]); + + let readable = format!( "wm_{}_{}_{}", sanitize(w_id), sanitize(datatable), sanitize(role) ); - if name.len() <= PG_IDENTIFIER_MAX_LEN { - return name; - } - // Truncating alone would let two long names collapse into one role, so the - // discriminator is derived from the full name. - use sha2::{Digest, Sha256}; - let hash = &Sha256::digest(name.as_bytes())[..4]; + let max_readable = PG_IDENTIFIER_MAX_LEN - 9; // "_" + 8 hex digits format!( "{}_{:08x}", - &name[..PG_IDENTIFIER_MAX_LEN - 9], - u32::from_be_bytes([hash[0], hash[1], hash[2], hash[3]]) + &readable[..readable.len().min(max_readable)], + discriminator ) } @@ -1217,14 +1264,17 @@ pub async fn get_datatable_replication_resource_from_db_unchecked( /// Returns the `(user, password)` to swap into the connection, or `None` when /// the data table's own credentials are to be used — which is every /// unpermissioned data table, and the `root` role of a permissioned one. -async fn resolve_datatable_role( - db: &DB, - w_id: &str, +/// Look up the role a resolution asks for, without authorizing it. +/// +/// `Ok(None)` means the data table is unpermissioned and resolves through its own +/// connection. On a permissioned one an unknown role is an error, and on an +/// unpermissioned one so is naming any role other than `root` — silently ignoring +/// it would run the script with more privileges than it asked for. +fn datatable_role_entry<'a>( + datatable: &'a DataTable, name: &str, - datatable: &DataTable, role: Option<&str>, - access: DatatableAccess<'_>, -) -> Result> { +) -> Result> { let Some(permissions) = datatable.permissions.as_ref().filter(|p| p.enabled) else { return match role { Some(role) if role != ROOT_DATATABLE_ROLE => Err(Error::BadRequest(format!( @@ -1236,7 +1286,7 @@ async fn resolve_datatable_role( }; let role_name = role.unwrap_or(ROOT_DATATABLE_ROLE); - let role_entry = permissions.roles.get(role_name).ok_or_else(|| { + let (role_name, role_entry) = permissions.roles.get_key_value(role_name).ok_or_else(|| { Error::NotFound(format!( "Role '{role_name}' is not defined on data table '{name}'. Defined roles: {}.", permissions @@ -1247,6 +1297,20 @@ async fn resolve_datatable_role( .join(", ") )) })?; + Ok(Some((role_name.as_str(), role_entry))) +} + +async fn resolve_datatable_role( + db: &DB, + w_id: &str, + name: &str, + datatable: &DataTable, + role: Option<&str>, + access: DatatableAccess<'_>, +) -> Result> { + let Some((role_name, role_entry)) = datatable_role_entry(datatable, name, role)? else { + return Ok(None); + }; let allowed = match access { DatatableAccess::Unchecked => true, @@ -2422,26 +2486,123 @@ mod tests { } #[test] - fn datatable_pg_role_names_are_workspace_scoped_and_fit_postgres() { - assert_eq!( - datatable_pg_role_name("acme", "main", "analyst"), - "wm_acme_main_analyst" - ); - // Two workspaces must never land on the same cluster-wide role. + fn datatable_pg_role_names_are_readable_and_never_collide() { + assert!(datatable_pg_role_name("acme", "main", "analyst").starts_with("wm_acme_main_analyst_")); + + // Every pair below sanitizes to the same readable half, so only the hash + // keeps them apart — and they must stay apart: two Windmill roles sharing + // one Postgres login would share its grants. + let collide = [ + // different workspaces + (("acme", "main", "analyst"), ("globex", "main", "analyst")), + // '-' and '_' both sanitize to '_' + (("acme", "main", "analyst-1"), ("acme", "main", "analyst_1")), + // the field separator is itself '_', so the boundary can shift + (("acme", "sales_ro", "x"), ("acme", "sales", "ro_x")), + // case is folded + (("acme", "main", "Analyst"), ("acme", "main", "analyst")), + ]; + for ((w1, d1, r1), (w2, d2, r2)) in collide { + assert_ne!( + datatable_pg_role_name(w1, d1, r1), + datatable_pg_role_name(w2, d2, r2), + "{w1}/{d1}/{r1} vs {w2}/{d2}/{r2}" + ); + } + + // Postgres silently truncates past 63 bytes, which would undo the above. + for name in [ + datatable_pg_role_name(&"w".repeat(60), "main", "analyst"), + datatable_pg_role_name("acme", &"d".repeat(200), &"r".repeat(60)), + ] { + assert!(name.len() <= PG_IDENTIFIER_MAX_LEN, "{name}"); + } assert_ne!( - datatable_pg_role_name("acme", "main", "analyst"), - datatable_pg_role_name("globex", "main", "analyst") - ); - // Postgres truncates past 63 bytes, so long names are hashed rather than - // left to collide. - let long = datatable_pg_role_name("w".repeat(60).as_str(), "main", "analyst"); - assert_eq!(long.len(), PG_IDENTIFIER_MAX_LEN); - assert_ne!( - long, - datatable_pg_role_name("w".repeat(61).as_str(), "main", "analyst") + datatable_pg_role_name(&"w".repeat(60), "main", "analyst"), + datatable_pg_role_name(&"w".repeat(61), "main", "analyst") ); } + fn permissioned(roles: &[(&str, &[&str])]) -> DataTable { + let mut map = std::collections::BTreeMap::new(); + for (name, tenants) in roles { + map.insert( + name.to_string(), + DataTableRole { + pg_rolename: (*name != ROOT_DATATABLE_ROLE) + .then(|| format!("wm_{name}")), + pg_password: (*name != ROOT_DATATABLE_ROLE).then(|| "pwd".to_string()), + tenants: tenants.iter().map(|t| t.to_string()).collect(), + }, + ); + } + DataTable { + database: DataTableDatabase { + resource_type: DataTableCatalogResourceType::Instance, + resource_path: "db".to_string(), + }, + forked_from: None, + migrations_enabled: None, + permissions: Some(DataTablePermissions { enabled: true, roles: map }), + } + } + + #[test] + fn datatable_role_lookup_defaults_to_root_and_rejects_unknown_roles() { + let dt = permissioned(&[(ROOT_DATATABLE_ROLE, &[]), ("analyst", &["u/alice"])]); + + // No role named -> root, which reuses the data table's own connection. + let (name, entry) = datatable_role_entry(&dt, "main", None).unwrap().unwrap(); + assert_eq!(name, ROOT_DATATABLE_ROLE); + assert!(entry.pg_rolename.is_none()); + + let (name, entry) = datatable_role_entry(&dt, "main", Some("analyst")) + .unwrap() + .unwrap(); + assert_eq!(name, "analyst"); + assert_eq!(entry.pg_rolename.as_deref(), Some("wm_analyst")); + + assert!(datatable_role_entry(&dt, "main", Some("nope")).is_err()); + } + + #[test] + fn naming_a_role_on_an_unpermissioned_datatable_is_refused() { + let mut dt = permissioned(&[(ROOT_DATATABLE_ROLE, &[]), ("analyst", &["u/alice"])]); + dt.permissions.as_mut().unwrap().enabled = false; + + // Silently ignoring the role would run the script as the data table's own + // connection — more privilege than it asked for. + assert!(datatable_role_entry(&dt, "main", Some("analyst")).is_err()); + // root and "no role" both mean the existing connection, so they are fine. + assert!(datatable_role_entry(&dt, "main", None).unwrap().is_none()); + assert!(datatable_role_entry(&dt, "main", Some(ROOT_DATATABLE_ROLE)) + .unwrap() + .is_none()); + } + + #[test] + fn datatable_settings_export_drops_role_passwords() { + let settings = serde_json::json!({ + "datatables": { + "main": { + "database": { "resource_type": "instance", "resource_path": "db" }, + "permissions": { "enabled": true, "roles": { + "root": { "tenants": [] }, + "analyst": { "pg_rolename": "wm_x", "pg_password": "s3cret", "tenants": ["u/alice"] } + }} + }, + "other": { "database": { "resource_type": "instance", "resource_path": "db2" } } + } + }); + let redacted = redact_datatable_settings_for_export(Some(settings)).unwrap(); + let analyst = &redacted["datatables"]["main"]["permissions"]["roles"]["analyst"]; + assert!(analyst.get("pg_password").is_none()); + // Everything else survives: the export is still a usable settings file. + assert_eq!(analyst["pg_rolename"], "wm_x"); + assert_eq!(analyst["tenants"][0], "u/alice"); + assert_eq!(redacted["datatables"]["other"]["database"]["resource_path"], "db2"); + } + #[test] fn test_parse_fork_branch() { // Generated fork (`wm-fork-abc`) and dev workspace (`staging`) forms. diff --git a/frontend/src/lib/components/DBManagerDrawer.svelte b/frontend/src/lib/components/DBManagerDrawer.svelte index b760ad4e02..9f60cb5388 100644 --- a/frontend/src/lib/components/DBManagerDrawer.svelte +++ b/frontend/src/lib/components/DBManagerDrawer.svelte @@ -217,7 +217,9 @@ datatable={uriState.selectedDatatable} onSchemaChanged={refreshManager} /> - + {#if $superadmin || $userStore?.is_admin} + + {/if} {/if} {#if enableImportExport}