From fa7b74a5b6c29452837324b84dd2e11ae08910e1 Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Fri, 24 Jul 2026 16:22:17 +0200 Subject: [PATCH] fix(datatables): cut revoked-role logins, drop shared createrole widening, serialize teardown and shared-db checks Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_013fBWmrMs2DbpU1iYdCvGX6 --- .../src/datatable_permissions_api.rs | 17 +++++++- .../windmill-api-workspaces/src/workspaces.rs | 5 +++ .../src/datatable_permissions.rs | 42 +++++++++++++++++-- backend/windmill-common/src/lib.rs | 15 ++++--- backend/windmill-common/src/workspaces.rs | 16 ++++--- 5 files changed, 79 insertions(+), 16 deletions(-) diff --git a/backend/windmill-api-workspaces/src/datatable_permissions_api.rs b/backend/windmill-api-workspaces/src/datatable_permissions_api.rs index 32bc2f0afa..b98ca00000 100644 --- a/backend/windmill-api-workspaces/src/datatable_permissions_api.rs +++ b/backend/windmill-api-workspaces/src/datatable_permissions_api.rs @@ -26,6 +26,14 @@ use windmill_common::workspaces::{ }; use windmill_common::{PgDatabase, DB}; +/// Transaction-scoped advisory lock serializing every shared-database +/// exclusivity check with the config write it protects. The scan in +/// [`check_shared_database_forbid`] is a plain read: without mutual exclusion, +/// two concurrent writers targeting the same physical database could both +/// observe no conflict and both commit. +pub(crate) const SHARED_DB_CHECK_LOCK: &str = + "SELECT pg_advisory_xact_lock(hashtextextended('wm_dt_shared_db_check', 0))"; + pub(crate) fn routes() -> Router { Router::new() .route( @@ -212,12 +220,17 @@ async fn set_datatable_permissions( check_datatable_permissions_ee_license().await?; } validate_permissions_config(&perms)?; + + let mut tx = db.begin().await?; if perms.enabled { + // Serialize the exclusivity scan with every other writer of datatable + // configs (same lock in `edit_datatable_config`): two concurrent + // enables/creations targeting the same physical database must not both + // pass the pre-write check. Held until commit. + sqlx::query(SHARED_DB_CHECK_LOCK).execute(&mut *tx).await?; check_shared_database_forbid(&db, &w_id, &datatable_name, &config.database, true).await?; check_owner_can_create_roles(&db, &w_id, &config).await?; } - - let mut tx = db.begin().await?; let args_for_audit = format!("{:?}", perms); audit_log( &mut *tx, diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index ec6b06c3eb..f13913a288 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -3035,6 +3035,11 @@ async fn edit_datatable_config( // physical database (enforcement is per database, config is per data // table). Check every data table whose database is new or changed, against // both the stored global state and the other entries of this request. + // Serialized with every other config writer (held until commit) so two + // concurrent saves can't both pass the exclusivity scan. + sqlx::query(crate::datatable_permissions_api::SHARED_DB_CHECK_LOCK) + .execute(&mut *tx) + .await?; let mut default_disabled: Vec = vec![]; let mut database_changed_names: Vec = vec![]; for (name, dt) in new_config.settings.datatables.iter() { diff --git a/backend/windmill-common/src/datatable_permissions.rs b/backend/windmill-common/src/datatable_permissions.rs index ae0225e2cd..af03b10f2b 100644 --- a/backend/windmill-common/src/datatable_permissions.rs +++ b/backend/windmill-common/src/datatable_permissions.rs @@ -547,7 +547,10 @@ async fn harden_instance_databases(db: &DB, target_dbname: &str) -> Result<()> { /// Ensure the caller's ephemeral role exists with up-to-date grants and a /// fresh sliding expiry. Returns `(role_name, cleartext_password)`. -pub async fn ensure_ephemeral_role( +/// Deliberately private: live credentials must only flow out through +/// [`get_datatable_resource_from_db_checked`], which performs the +/// authorization this function assumes already happened. +async fn ensure_ephemeral_role( db: &DB, w_id: &str, datatable: &str, @@ -796,6 +799,7 @@ async fn cleanup_one_expired_role(db: &DB, role: &str, w_id: &str, datatable: &s .map_err(|e| pg_err("checking active sessions", e))? .get(0); if active > 0 { + disable_role_login(&client, role).await; return Ok(()); } if role_exists(&client, role).await? { @@ -862,7 +866,37 @@ pub async fn drop_datatable_ephemeral_roles_best_effort( } } +/// Strip a live role's ability to open new connections. An ordinary role can +/// `ALTER ROLE CURRENT_USER PASSWORD ...` — so a caller keeping a session open +/// could otherwise reconnect with a self-chosen password long after +/// revocation. Only CREATEROLE can restore LOGIN, so this closes the +/// reconnect vector while letting in-flight queries finish; the kept +/// bookkeeping row makes the expiry sweep finish the drop later. +async fn disable_role_login(client: &tokio_postgres::Client, role: &str) { + if !role.starts_with(DATATABLE_EPHEMERAL_ROLE_PREFIX) { + return; + } + if let Err(e) = client + .batch_execute(&format!( + "ALTER ROLE {} NOLOGIN CONNECTION LIMIT 0 PASSWORD NULL", + quote_ident(role) + )) + .await + { + tracing::warn!("disabling login of ephemeral role {role}: {e:#}"); + } +} + async fn teardown_role(db: &DB, w_id: &str, datatable: &str, role: &str) -> Result<()> { + // Same per-role lock as role creation and the expiry sweep: without it, + // teardown could observe a stale role while a resolver is recreating it + // and drop the fresh role right before its bookkeeping row lands, leaving + // callers with credentials for a nonexistent role. + let mut tx = db.begin().await?; + sqlx::query("SELECT pg_advisory_xact_lock(hashtextextended('wm_dt_role:' || $1, 0))") + .bind(role) + .execute(&mut *tx) + .await?; if let Ok(config) = get_datatable_config(db, w_id, datatable).await { let owner: PgDatabase = serde_json::from_value(datatable_shared_resource(db, w_id, &config).await?) @@ -881,7 +915,8 @@ async fn teardown_role(db: &DB, w_id: &str, datatable: &str, role: &str) -> Resu // and deleting the row would permanently orphan a live role that // still holds its old grants (and whose password the caller // already has). With the row intact, the expiry sweep retries the - // drop once the sessions are gone. + // drop once the sessions are gone; meanwhile new logins are cut. + disable_role_login(&client, role).await; return Ok(()); } if role_exists(&client, role).await? { @@ -895,8 +930,9 @@ async fn teardown_role(db: &DB, w_id: &str, datatable: &str, role: &str) -> Resu "DELETE FROM datatable_ephemeral_role WHERE role_name = $1", role ) - .execute(db) + .execute(&mut *tx) .await?; + tx.commit().await?; Ok(()) } diff --git a/backend/windmill-common/src/lib.rs b/backend/windmill-common/src/lib.rs index 481779029f..d3d435aa1c 100644 --- a/backend/windmill-common/src/lib.rs +++ b/backend/windmill-common/src/lib.rs @@ -1203,18 +1203,21 @@ pub async fn create_custom_instance_database( if let Err(e) = client .batch_execute(&format!( - // REVOKE PUBLIC CONNECT + CREATEROLE support the fine-grained data - // table permissions feature: ephemeral per-user roles must not be - // able to hop to other databases on the cluster, and the shared - // owner role creates/drops them. + // REVOKE PUBLIC CONNECT supports the fine-grained data table + // permissions feature: ephemeral per-user roles must not be able + // to hop to other databases on the cluster. CREATEROLE for the + // shared role is deliberately NOT granted here — this path is + // reachable by non-superadmins (fork databases), and on + // Postgres < 16 CREATEROLE would let arbitrary user SQL running + // as the shared role manage unrelated cluster roles; only the + // superadmin settings path grants it. "REVOKE CONNECT ON DATABASE \"{dbname}\" FROM PUBLIC; GRANT CONNECT ON DATABASE \"{dbname}\" TO custom_instance_user; GRANT USAGE ON SCHEMA public TO custom_instance_user; GRANT CREATE ON SCHEMA public TO custom_instance_user; GRANT CREATE ON DATABASE \"{dbname}\" TO custom_instance_user; ALTER DEFAULT PRIVILEGES IN SCHEMA public - GRANT SELECT, INSERT, UPDATE, DELETE ON TABLES TO custom_instance_user; - ALTER ROLE custom_instance_user CREATEROLE;" + GRANT SELECT, INSERT, UPDATE, DELETE ON TABLES TO custom_instance_user;" )) .await { diff --git a/backend/windmill-common/src/workspaces.rs b/backend/windmill-common/src/workspaces.rs index 157b39ecca..4a0c6cb09e 100644 --- a/backend/windmill-common/src/workspaces.rs +++ b/backend/windmill-common/src/workspaces.rs @@ -1114,6 +1114,11 @@ pub async fn get_datatable_config(db: &DB, w_id: &str, name: &str) -> Result