From 3981b2eb1b3bf77cdc1a5cbf43090e6b9706aa1f Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Thu, 17 Sep 2026 15:28:31 +0200 Subject: [PATCH] fix(datatables): refuse repointing the external cluster while it is in use, and keep verify-ca working for pg_dump Co-Authored-By: Claude Opus 5 (1M context) --- .../windmill-api-workspaces/src/workspaces.rs | 78 +++++++++++++++++-- .../src/external_instance_pg.rs | 33 +++++++- 2 files changed, 102 insertions(+), 9 deletions(-) diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index 863ea9e670..db3ef7633f 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -3364,18 +3364,80 @@ fn apply_pg_tls_env( return Ok(Some(file)); } // Only a connection that asked to be verified against the system trust store. Without a file, - // libpq's own default would look for `~/.postgresql/root.crt` and refuse a verify-* mode. - if pg_db.accept_invalid_certs == Some(false) - && matches!( - pg_db.sslmode.as_deref(), - Some("verify-full") | Some("verify-ca") - ) - { - cmd.env("PGSSLROOTCERT", "system"); + // libpq's own default would look for `~/.postgresql/root.crt` and refuse a verify-* mode. libpq + // takes the special `system` value with verify-full only, so verify-ca needs the bundle itself. + if pg_db.accept_invalid_certs == Some(false) { + match pg_db.sslmode.as_deref() { + Some("verify-full") => { + cmd.env("PGSSLROOTCERT", "system"); + } + Some("verify-ca") => { + if let Some(bundle) = system_ca_bundle() { + cmd.env("PGSSLROOTCERT", bundle); + } + } + _ => {} + } } Ok(None) } +fn system_ca_bundle() -> Option { + std::env::var_os("SSL_CERT_FILE") + .map(std::path::PathBuf::from) + .into_iter() + .chain( + [ + "/etc/ssl/certs/ca-certificates.crt", + "/etc/pki/tls/certs/ca-bundle.crt", + "/etc/ssl/cert.pem", + "/etc/ssl/ca-bundle.pem", + ] + .map(std::path::PathBuf::from), + ) + .find(|path| path.is_file()) +} + +#[cfg(test)] +mod pg_tls_env_tests { + use super::apply_pg_tls_env; + use windmill_common::PgDatabase; + + fn root_cert_env(sslmode: &str) -> Option { + let pg_db = PgDatabase { + host: "db".to_string(), + user: None, + password: None, + port: None, + sslmode: Some(sslmode.to_string()), + dbname: "d".to_string(), + root_certificate_pem: None, + accept_invalid_certs: Some(false), + use_iam_auth: None, + region: None, + }; + let mut cmd = tokio::process::Command::new("psql"); + apply_pg_tls_env(&mut cmd, &pg_db).unwrap(); + cmd.as_std() + .get_envs() + .find(|(k, _)| *k == "PGSSLROOTCERT") + .and_then(|(_, v)| v.map(|v| v.to_os_string())) + } + + #[test] + fn system_roots_only_through_verify_full() { + assert_eq!( + root_cert_env("verify-full").as_deref(), + Some("system".as_ref()) + ); + // libpq refuses `sslrootcert=system` with verify-ca, which would fail every dump and restore. + assert_ne!( + root_cert_env("verify-ca").as_deref(), + Some("system".as_ref()) + ); + } +} + /// GUC names the server backing `pg_db` knows about. /// /// Asked through psql rather than a tokio-postgres connection so the lookup reaches diff --git a/backend/windmill-common/src/external_instance_pg.rs b/backend/windmill-common/src/external_instance_pg.rs index 18cc1327c4..f1c2d5ebf4 100644 --- a/backend/windmill-common/src/external_instance_pg.rs +++ b/backend/windmill-common/src/external_instance_pg.rs @@ -278,11 +278,42 @@ pub async fn check_external_instance_pg_write( ensure_external_instance_pg_removable(db).await } Some(value) => { - crate::external_instance_pg_oss::validate_external_instance_pg_setting(value) + crate::external_instance_pg_oss::validate_external_instance_pg_setting(value)?; + ensure_external_instance_pg_not_repointed(db, value).await } } } +/// Refuse pointing the setting at another host or port while databases live on the current one. +/// Data tables name databases, not clusters, so they would silently resolve to whatever the new +/// cluster holds under the same names. Other fields (admin login, sslmode) may change freely. +async fn ensure_external_instance_pg_not_repointed( + db: &DB, + value: &serde_json::Value, +) -> Result<()> { + let Some(current) = read_external_instance_pg_config(db).await? else { + return Ok(()); + }; + let Ok(desired) = serde_json::from_value::(value.clone()) else { + return Ok(()); + }; + let address = |c: &ExternalInstancePg| (c.host.trim().to_lowercase(), c.port.unwrap_or(5432)); + if address(¤t) == address(&desired) { + return Ok(()); + } + let state = read_external_instance_pg_state(db).await?; + let usages = external_instance_database_usages(db).await?; + if state.databases.is_empty() && usages.is_empty() { + return Ok(()); + } + Err(Error::BadRequest(format!( + "The external instance cluster at {}:{} still holds databases in use. Drop them and repoint \ + what uses them before pointing {EXTERNAL_INSTANCE_PG_SETTING} at another cluster.", + current.host.trim(), + current.port.unwrap_or(5432) + ))) +} + /// Converge the external cluster on the configured login: check what it can do, create or update /// Windmill's two roles with the stored passwords, and report anything that would get in the way. /// With `rotate_passwords`, generate new passwords first. Safe to run again; running it again is