diff --git a/backend/.sqlx/query-e881975096292b5c4ffaa5fe4256cde74a2aa805c4901e2a00510e70f52dc4a0.json b/backend/.sqlx/query-e881975096292b5c4ffaa5fe4256cde74a2aa805c4901e2a00510e70f52dc4a0.json new file mode 100644 index 0000000000..f6bb8fba4d --- /dev/null +++ b/backend/.sqlx/query-e881975096292b5c4ffaa5fe4256cde74a2aa805c4901e2a00510e70f52dc4a0.json @@ -0,0 +1,22 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT workspace_id FROM datatable_owner_role WHERE dbname = $1", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "workspace_id", + "type_info": "Varchar" + } + ], + "parameters": { + "Left": [ + "Text" + ] + }, + "nullable": [ + false + ] + }, + "hash": "e881975096292b5c4ffaa5fe4256cde74a2aa805c4901e2a00510e70f52dc4a0" +} diff --git a/backend/windmill-api-settings/src/lib.rs b/backend/windmill-api-settings/src/lib.rs index bfd0848bd4..520762a70f 100644 --- a/backend/windmill-api-settings/src/lib.rs +++ b/backend/windmill-api-settings/src/lib.rs @@ -1698,8 +1698,7 @@ async fn setup_custom_instance_pg_database_inner( 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 .map_err(|e| { @@ -1709,6 +1708,21 @@ async fn setup_custom_instance_pg_database_inner( )) })?; + // Re-running setup on a data table database that has fine-grained + // permissions enabled just re-granted the shared role the access the + // feature deliberately took away. Hand the database back to its dedicated + // owner role (idempotent, re-asserts the revoke). + if let Some(w_id) = sqlx::query_scalar!( + "SELECT workspace_id FROM datatable_owner_role WHERE dbname = $1", + dbname + ) + .fetch_optional(db) + .await? + { + windmill_common::datatable_permissions::provision_instance_owner_role(db, &w_id, dbname) + .await?; + } + // The replication attribute lives on a dedicated role used by postgres trigger // connections. The getter creates the role (with its stored password) when the // migration couldn't. diff --git a/backend/windmill-api-workspaces/src/datatable_permissions_api.rs b/backend/windmill-api-workspaces/src/datatable_permissions_api.rs index 483351999e..2ea104a195 100644 --- a/backend/windmill-api-workspaces/src/datatable_permissions_api.rs +++ b/backend/windmill-api-workspaces/src/datatable_permissions_api.rs @@ -159,9 +159,10 @@ pub(crate) async fn check_shared_database_forbid( Ok(()) } -/// The owner role creates/drops the ephemeral roles, so it needs CREATEROLE -/// (or superuser). True by construction for freshly provisioned instance -/// databases; verified here for external resources and legacy instance DBs. +/// External data tables mint their ephemeral roles as the resource's own user, +/// so that user needs CREATEROLE (or superuser). Instance databases don't: +/// their roles are managed by the main pool user, and the shared role is +/// deliberately kept without CREATEROLE — never call this for them. pub(crate) async fn check_owner_can_create_roles( db: &DB, w_id: &str, @@ -249,8 +250,9 @@ async fn set_datatable_permissions( == windmill_common::workspaces::DataTableCatalogResourceType::Instance { provision_instance_owner_role(&db, &w_id, &config.database.resource_path).await?; + } else { + check_owner_can_create_roles(&db, &w_id, &config).await?; } - check_owner_can_create_roles(&db, &w_id, &config).await?; } let args_for_audit = format!("{:?}", perms); audit_log( @@ -317,6 +319,15 @@ async fn check_datatable_permissions_setup( check_datatable_permissions_ee_license().await?; let config = get_datatable_config(&db, &w_id, &datatable_name).await?; check_shared_database_forbid(&db, &w_id, &datatable_name, &config.database, true).await?; + // Only external targets need the CREATEROLE check: running it on an + // instance target would report on the shared role and advise granting it + // CREATEROLE — the very attribute this feature must keep away from the + // role that user SQL runs as. + if config.database.resource_type + != windmill_common::workspaces::DataTableCatalogResourceType::Postgresql + { + return Ok("ok".to_string()); + } check_owner_can_create_roles(&db, &w_id, &config).await?; Ok("ok".to_string()) } diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index c0d0649663..d5272453c6 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -3139,10 +3139,9 @@ async fn edit_datatable_config( } } // An external target must be able to create the enforcement roles. - // Instance targets are skipped: their shared role's CREATEROLE is - // cluster-wide (granted at provisioning, re-checked at grant-save), - // and the instance database may legitimately not be set up yet when - // the config is saved. + // Instance targets are skipped: their roles are managed by the main + // pool user, and the instance database may legitimately not be set up + // yet when the config is saved. if self_enabled && dt.database.resource_type == DataTableCatalogResourceType::Postgresql { if let Err(e) = crate::datatable_permissions_api::check_owner_can_create_roles(&db, &w_id, dt).await diff --git a/backend/windmill-api-workspaces/src/workspaces_extra.rs b/backend/windmill-api-workspaces/src/workspaces_extra.rs index d21c306d61..2308cf040a 100644 --- a/backend/windmill-api-workspaces/src/workspaces_extra.rs +++ b/backend/windmill-api-workspaces/src/workspaces_extra.rs @@ -949,7 +949,8 @@ pub(crate) async fn delete_workspace( .await .map_err(|e| { Error::internal_err(format!( - "cannot delete workspace {w_id}: returning ownership of instance database {dbname} to the shared role failed: {e}. Retry when it is reachable." + "cannot delete workspace {w_id}: returning ownership of instance database \ + {dbname} to the shared role failed: {e}. Retry when it is reachable." )) })?; } diff --git a/backend/windmill-common/src/datatable_permissions.rs b/backend/windmill-common/src/datatable_permissions.rs index 928df26c81..a29101bdc5 100644 --- a/backend/windmill-common/src/datatable_permissions.rs +++ b/backend/windmill-common/src/datatable_permissions.rs @@ -421,7 +421,11 @@ pub fn instance_owner_role_name(dbname: &str) -> String { /// Credentials of a protected instance database's dedicated owner role, if it /// has been provisioned. -pub async fn instance_owner_creds(db: &DB, dbname: &str) -> Result> { +/// +/// Authorization: returns a full-access login for the database and performs no +/// authorization — crate-internal on purpose; it must only ever be reached +/// through the resolvers that gate on admin-ness or effective grants. +pub(crate) async fn instance_owner_creds(db: &DB, dbname: &str) -> Result> { let Some(row) = sqlx::query!( "SELECT role_name, password, workspace_id FROM datatable_owner_role WHERE dbname = $1", dbname @@ -486,7 +490,7 @@ pub async fn provision_instance_owner_role(db: &DB, w_id: &str, dbname: &str) -> } else { client .batch_execute(&format!( - "CREATE ROLE {} LOGIN NOSUPERUSER NOCREATEDB CREATEROLE NOINHERIT NOREPLICATION PASSWORD {}", + "CREATE ROLE {} LOGIN NOSUPERUSER NOCREATEDB NOCREATEROLE NOINHERIT NOREPLICATION PASSWORD {}", quote_ident(&role), quote_literal(&password) )) @@ -521,18 +525,27 @@ pub async fn provision_instance_owner_role(db: &DB, w_id: &str, dbname: &str) -> .execute(db) .await .map_err(|e| Error::internal_err(format!("granting {role} access to {dbname}: {e:#}")))?; + // CDC connects as `custom_instance_replication_user`, which reaches these + // databases only by inheriting `custom_instance_user` — the revoke below + // would take it down with the parent. It is never handed to user SQL, so + // a direct grant keeps triggers/capture working without reopening + // anything. sqlx::query(&format!( - "REVOKE CONNECT ON DATABASE {} FROM custom_instance_user", + "GRANT CONNECT ON DATABASE {} TO custom_instance_replication_user", quote_ident(dbname) )) .execute(db) .await .map_err(|e| { Error::internal_err(format!( - "revoking custom_instance_user access to {dbname}: {e:#}" + "granting custom_instance_replication_user access to {dbname}: {e:#}" )) })?; + // Read the cipher fresh under the lock: a key rotation may have committed + // while this call waited, and a cached pre-rotation cipher would make the + // stored owner password permanently undecryptable. + crate::variables::WORKSPACE_CRYPT_CACHE.remove(w_id); let mc = build_crypt(db, w_id).await?; let encrypted = encrypt(&mc, &password); sqlx::query!( @@ -549,7 +562,36 @@ pub async fn provision_instance_owner_role(db: &DB, w_id: &str, dbname: &str) -> ) .execute(&mut *tx) .await?; + // Commit the row BEFORE locking `custom_instance_user` out: if this call + // dies in between, the fallback to the shared role still works and the + // next call re-asserts the lockout. The reverse order would strand the + // database with no usable credentials at all. tx.commit().await?; + + sqlx::query(&format!( + "REVOKE CONNECT ON DATABASE {} FROM custom_instance_user", + quote_ident(dbname) + )) + .execute(db) + .await + .map_err(|e| { + Error::internal_err(format!( + "revoking custom_instance_user access to {dbname}: {e:#}" + )) + })?; + // Before PG16, CREATEROLE lets a role alter ANY non-superuser role — so a + // caller who takes over `custom_instance_user` from an unprotected data + // table could simply reset this database's owner-role password. Nothing + // needs that attribute any more (instance role management runs as the main + // pool user), so drop it cluster-wide once protection is in use. + sqlx::query("ALTER ROLE custom_instance_user NOCREATEROLE") + .execute(db) + .await + .map_err(|e| { + Error::internal_err(format!( + "removing CREATEROLE from custom_instance_user: {e:#}" + )) + })?; Ok(()) } @@ -607,6 +649,13 @@ pub async fn deprovision_instance_owner_role(db: &DB, dbname: &str) -> Result<() "restoring custom_instance_user access to {dbname}: {e:#}" )) })?; + // The replication role goes back to inheriting the shared role. + let _ = sqlx::query(&format!( + "REVOKE CONNECT ON DATABASE {} FROM custom_instance_replication_user", + quote_ident(dbname) + )) + .execute(db) + .await; } sqlx::query(&format!("DROP ROLE IF EXISTS {}", quote_ident(&role))) .execute(db) diff --git a/backend/windmill-common/src/lib.rs b/backend/windmill-common/src/lib.rs index d3d435aa1c..21fc56c8a6 100644 --- a/backend/windmill-common/src/lib.rs +++ b/backend/windmill-common/src/lib.rs @@ -1205,12 +1205,10 @@ pub async fn create_custom_instance_database( .batch_execute(&format!( // 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. + // to hop to other databases on the cluster. `custom_instance_user` + // is deliberately never granted CREATEROLE — user SQL runs as it, + // and before PG16 that attribute would let it take over any + // non-superuser role on the cluster. "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;