fix(datatables): close pg<16 createrole takeover, keep cdc access on protected databases

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013fBWmrMs2DbpU1iYdCvGX6

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"
+}

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.

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())
 }

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

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."
                 ))
             })?;
     }

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<Option<(String, String)>> {
+///
+/// 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<Option<(String, String)>> {
     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)

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;