From 504f018f42662296bbb2fe3db1b77749d97f5c25 Mon Sep 17 00:00:00 2001 From: Chris Marshall Date: Mon, 24 Aug 2026 17:02:53 -0400 Subject: [PATCH 1/2] feat(config): expand environment variables in pgdog.toml and users.toml MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Configuration files can now reference the process environment, so secrets and per-environment values no longer have to be baked into the files on disk: ```toml [admin] password = "${PGDOG_ADMIN_PASSWORD}" [general] shutdown_timeout = ${PGDOG_SHUTDOWN_TIMEOUT:-60000} ``` `$VAR` and `${VAR}` are substituted from the environment, `${VAR:-value}` supplies a fallback, and `$$` is a literal `$`. Lookups are lenient: a reference to a variable that isn't set is left in the document verbatim rather than failing the load. `users.toml` is the file most likely to contain a stray `$` — a password like `sup$rsecret` keeps working instead of turning into a startup failure or, worse, a silently truncated credential. The one behaviour change to be aware of is that a literal `$$` in an existing value now collapses to a single `$`; that is unavoidable once any escape exists. Expansion runs on the document source before it is parsed, so a variable is interpolated as TOML rather than as a string. `${PASSWORD}` in value position still needs its surrounding quotes, and a value containing `"` or a newline will change how the rest of the document parses. This is what allows bare `shutdown_timeout = ${VAR}` to work, and it is documented on `expand`. Implementation notes: - New `pgdog-config::expand` module. `expand()` is infallible and returns `Cow::Borrowed` when there is nothing to substitute, so the common case costs no allocation. - `FromToml::from_toml` replaces bare `toml::from_str` at the three sites that parse config text read from disk: both branches of `ConfigAndUsers::load` and `bootstrap_logger`. Every other `toml::from_str` in the tree parses a test literal, where expansion is unwanted, and is untouched. - The trait carries a blanket impl over `DeserializeOwned`, so no per-type boilerplate is needed. `from_toml`, not `from_str`, to avoid colliding with the crate's many `std::str::FromStr` impls. - `Error::config` now receives the expanded text, so the line numbers it reports stay correct when a variable's value contains a newline. - `ConfigAndUsers` keeps `config_text`/`users_text` as the raw, unexpanded source. Resolved secrets must not be written back to disk when the config is reloaded or backed up. Adds a dependency on `shellexpand`. --- Cargo.lock | 48 +++++++++++++++++ pgdog-config/Cargo.toml | 1 + pgdog-config/src/core.rs | 12 ++--- pgdog-config/src/expand.rs | 104 +++++++++++++++++++++++++++++++++++++ pgdog-config/src/lib.rs | 2 + pgdog/src/main.rs | 4 +- 6 files changed, 163 insertions(+), 8 deletions(-) create mode 100644 pgdog-config/src/expand.rs diff --git a/Cargo.lock b/Cargo.lock index 2824157e4..96e6b0311 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1537,6 +1537,27 @@ dependencies = [ "ctutils", ] +[[package]] +name = "dirs" +version = "6.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c3e8aa94d75141228480295a7d0e7feb620b1a5ad9f12bc40be62411e38cce4e" +dependencies = [ + "dirs-sys", +] + +[[package]] +name = "dirs-sys" +version = "0.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e01a3366d27ee9890022452ee61b2b63a67e6f13f58900b651ff5665f0bb1fab" +dependencies = [ + "libc", + "option-ext", + "redox_users", + "windows-sys 0.61.2", +] + [[package]] name = "displaydoc" version = "0.2.5" @@ -2901,6 +2922,12 @@ dependencies = [ "vcpkg", ] +[[package]] +name = "option-ext" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "04744f49eae99ab78e0d5c0b603ab218f515ea8cfe5a456d7629ad883a3b6e7d" + [[package]] name = "ordered-float" version = "4.6.0" @@ -3079,6 +3106,7 @@ dependencies = [ "schemars", "serde", "serde_json", + "shellexpand", "tempfile", "thiserror", "toml", @@ -3552,6 +3580,17 @@ dependencies = [ "bitflags", ] +[[package]] +name = "redox_users" +version = "0.5.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a4e608c6638b9c18977b00b475ac1f28d14e84b27d8d42f70e0bf1e3dec127ac" +dependencies = [ + "getrandom 0.2.17", + "libredox", + "thiserror", +] + [[package]] name = "ref-cast" version = "1.0.25" @@ -4154,6 +4193,15 @@ dependencies = [ "lazy_static", ] +[[package]] +name = "shellexpand" +version = "3.1.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "32824fab5e16e6c4d86dc1ba84489390419a39f97699852b66480bb87d297ed8" +dependencies = [ + "dirs", +] + [[package]] name = "shlex" version = "1.3.0" diff --git a/pgdog-config/Cargo.toml b/pgdog-config/Cargo.toml index 12e1ff052..48862ab2a 100644 --- a/pgdog-config/Cargo.toml +++ b/pgdog-config/Cargo.toml @@ -17,6 +17,7 @@ once_cell = "*" schemars.workspace = true indexmap.workspace = true derive_more = { version = "2", features = ["display", "from_str"] } +shellexpand = "3.1.2" [dev-dependencies] tempfile = "3.23.0" diff --git a/pgdog-config/src/core.rs b/pgdog-config/src/core.rs index 491cd9742..4e1524fd2 100644 --- a/pgdog-config/src/core.rs +++ b/pgdog-config/src/core.rs @@ -26,6 +26,8 @@ use super::sharding::{OmnishardedTables, ShardedMappingDeprecated}; use super::users::{Admin, Plugin, Users}; use super::vault::Vault; +use crate::FromToml; + #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] pub struct ConfigAndUsers { /// parsed pgdog.toml or default [Config] @@ -49,10 +51,9 @@ impl ConfigAndUsers { pub fn load(config_path: &Path, users_path: &Path) -> Result { let config_text = read_to_string(config_path).ok(); let mut config: Config = if let Some(text) = &config_text { - let config = match toml::from_str(text) { + let config = match Config::from_toml(text) { Ok(config) => config, - Err(err) => { - let error = Error::config(text, err); + Err(error) => { error!("failed to load {}: {}", config_path.display(), error); return Err(error); } @@ -73,10 +74,9 @@ impl ConfigAndUsers { let users_text = read_to_string(users_path).ok(); let mut users: Users = if let Some(text) = &users_text { - let users: Users = match toml::from_str(text) { + let users: Users = match Users::from_toml(text) { Ok(config) => config, - Err(err) => { - let error = Error::config(text, err); + Err(error) => { error!("failed to load {}: {}", users_path.display(), error); return Err(error); } diff --git a/pgdog-config/src/expand.rs b/pgdog-config/src/expand.rs new file mode 100644 index 000000000..6cb482066 --- /dev/null +++ b/pgdog-config/src/expand.rs @@ -0,0 +1,104 @@ +//! Environment variable expansion in configuration files. + +use std::borrow::Cow; +use std::convert::Infallible; +use std::env::var; + +use serde::de::DeserializeOwned; + +use crate::Error; + +/// Expand `$VAR` and `${VAR}` references in a configuration file against the +/// process environment. +/// +/// References to variables that aren't set are left in the document verbatim, so +/// values that merely contain a `$` (passwords, most commonly) survive +/// untouched. Write `$$` for a literal `$`, and `${VAR:-value}` to supply a +/// fallback. +/// +/// **Note:** expansion happens on the document source, before it's parsed, so a +/// variable is interpolated as TOML rather than as a string. `${PASSWORD}` in +/// value position needs surrounding quotes, and a value containing `"` or a +/// newline changes how the rest of the document parses. +pub fn expand(source: &str) -> Cow<'_, str> { + shellexpand::env_with_context(source, |name| Ok::<_, Infallible>(var(name).ok())) + .expect("lookup is infallible") +} + +/// Parse a TOML configuration document, expanding environment variables first. +pub trait FromToml: DeserializeOwned { + /// Parse `source` as TOML, [`expand`]ing environment variables first. + /// + /// # Errors + /// + /// Returns [`Error::MissingField`] if the expanded document isn't valid TOML + /// or doesn't match the shape of `Self`. + fn from_toml(source: &str) -> Result { + let expanded = expand(source); + toml::from_str(&expanded).map_err(|err| Error::config(&expanded, err)) + } +} + +impl FromToml for T {} + +#[cfg(test)] +mod test { + use super::*; + use crate::test_utils::{remove_env_var, set_env_var}; + use crate::{Config, Users}; + + #[test] + fn test_expand() { + let _set = set_env_var("PGDOG_TEST_VAR", "expanded"); + let _unset = remove_env_var("PGDOG_TEST_MISSING"); + + assert_eq!(expand("${PGDOG_TEST_VAR}"), "expanded"); + assert_eq!(expand("$PGDOG_TEST_VAR/db"), "expanded/db"); + assert_eq!(expand("${PGDOG_TEST_MISSING}"), "${PGDOG_TEST_MISSING}"); + assert_eq!(expand("${PGDOG_TEST_MISSING:-fallback}"), "fallback"); + assert_eq!(expand("sup$rsecret"), "sup$rsecret"); + assert_eq!(expand("p$$w0rd"), "p$w0rd"); + } + + #[test] + fn test_from_toml_expands() { + let _password = set_env_var("PGDOG_TEST_PASSWORD", "not a real secret"); + let _timeout = set_env_var("PGDOG_TEST_SHUTDOWN_TIMEOUT", "1_000"); + + let source = r#" +[admin] +password = "${PGDOG_TEST_PASSWORD}" + +[general] +shutdown_timeout = ${PGDOG_TEST_SHUTDOWN_TIMEOUT} +"#; + + let config = Config::from_toml(source).unwrap(); + assert_eq!(config.admin.password, "not a real secret"); + assert_eq!(config.general.shutdown_timeout, 1_000); + } + + #[test] + fn test_from_toml_leaves_unset_alone() { + let _unset = remove_env_var("PGDOG_TEST_MISSING"); + + let source = r#" +[[users]] +name = "pgdog" +database = "pgdog" +password = "${PGDOG_TEST_MISSING}" +"#; + + let users = Users::from_toml(source).unwrap(); + assert_eq!( + users.users[0].password.as_deref(), + Some("${PGDOG_TEST_MISSING}") + ); + } + + #[test] + fn test_from_toml_reports_errors() { + let err = Config::from_toml("[general]\nnot_a_field = 1\n").unwrap_err(); + assert!(matches!(err, Error::MissingField(..)), "{err:?}"); + } +} diff --git a/pgdog-config/src/lib.rs b/pgdog-config/src/lib.rs index b70ecb55d..c0ed63d8a 100644 --- a/pgdog-config/src/lib.rs +++ b/pgdog-config/src/lib.rs @@ -4,6 +4,7 @@ pub mod core; pub mod data_types; pub mod database; pub mod error; +pub mod expand; pub mod general; pub mod memory; pub mod networking; @@ -30,6 +31,7 @@ pub use database::{ Database, EnumeratedDatabase, LoadBalancingStrategy, ReadWriteSplit, ReadWriteStrategy, Role, }; pub use error::Error; +pub use expand::{FromToml, expand}; pub use general::{General, LogFormat, QuerySizeLimitAction}; pub use memory::*; pub use networking::{MultiTenant, Tcp, TlsVerifyMode}; diff --git a/pgdog/src/main.rs b/pgdog/src/main.rs index 223997e97..9eb30a003 100644 --- a/pgdog/src/main.rs +++ b/pgdog/src/main.rs @@ -27,7 +27,7 @@ use tracing::{error, info, warn}; use util::pgdog_version; use arc_swap::ArcSwapOption; -use pgdog_config::{General, LogFormat}; +use pgdog_config::{FromToml, General, LogFormat}; use tracing::level_filters::LevelFilter; use tracing::subscriber::Interest; use tracing::{Event, Metadata, Subscriber}; @@ -339,7 +339,7 @@ fn build_runtime(workers: usize, stack_size: usize) -> std::io::Result(&config).ok()) + .and_then(|config| config::Config::from_toml(&config).ok()) .map(|config| config.general) .unwrap_or_default(); From bd7859b96acfac07add64fc15940d44189160275 Mon Sep 17 00:00:00 2001 From: Chris Marshall Date: Wed, 2 Sep 2026 11:37:00 -0400 Subject: [PATCH 2/2] refactor(config): replace shellexpand with simple scanner The expand environment variable feature for configuration files previously used shellexpand to expand references before being parsed as toml. shellexpand supports expanding references that include just a $ so it is being replaced here with a simple scanner over the toml input string that only allows bracketed variable references. The replacement expand function works with the former fallback syntax. Another big bonus for this change is that requiring brackets means that the new function can also ensure that there is a closing bracket before performing a substitution. --- Cargo.lock | 48 -------------- pgdog-config/Cargo.toml | 1 - pgdog-config/src/expand.rs | 127 +++++++++++++++++++++++++++++++++---- 3 files changed, 116 insertions(+), 60 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 96e6b0311..2824157e4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1537,27 +1537,6 @@ dependencies = [ "ctutils", ] -[[package]] -name = "dirs" -version = "6.0.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c3e8aa94d75141228480295a7d0e7feb620b1a5ad9f12bc40be62411e38cce4e" -dependencies = [ - "dirs-sys", -] - -[[package]] -name = "dirs-sys" -version = "0.5.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e01a3366d27ee9890022452ee61b2b63a67e6f13f58900b651ff5665f0bb1fab" -dependencies = [ - "libc", - "option-ext", - "redox_users", - "windows-sys 0.61.2", -] - [[package]] name = "displaydoc" version = "0.2.5" @@ -2922,12 +2901,6 @@ dependencies = [ "vcpkg", ] -[[package]] -name = "option-ext" -version = "0.2.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "04744f49eae99ab78e0d5c0b603ab218f515ea8cfe5a456d7629ad883a3b6e7d" - [[package]] name = "ordered-float" version = "4.6.0" @@ -3106,7 +3079,6 @@ dependencies = [ "schemars", "serde", "serde_json", - "shellexpand", "tempfile", "thiserror", "toml", @@ -3580,17 +3552,6 @@ dependencies = [ "bitflags", ] -[[package]] -name = "redox_users" -version = "0.5.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a4e608c6638b9c18977b00b475ac1f28d14e84b27d8d42f70e0bf1e3dec127ac" -dependencies = [ - "getrandom 0.2.17", - "libredox", - "thiserror", -] - [[package]] name = "ref-cast" version = "1.0.25" @@ -4193,15 +4154,6 @@ dependencies = [ "lazy_static", ] -[[package]] -name = "shellexpand" -version = "3.1.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "32824fab5e16e6c4d86dc1ba84489390419a39f97699852b66480bb87d297ed8" -dependencies = [ - "dirs", -] - [[package]] name = "shlex" version = "1.3.0" diff --git a/pgdog-config/Cargo.toml b/pgdog-config/Cargo.toml index 48862ab2a..12e1ff052 100644 --- a/pgdog-config/Cargo.toml +++ b/pgdog-config/Cargo.toml @@ -17,7 +17,6 @@ once_cell = "*" schemars.workspace = true indexmap.workspace = true derive_more = { version = "2", features = ["display", "from_str"] } -shellexpand = "3.1.2" [dev-dependencies] tempfile = "3.23.0" diff --git a/pgdog-config/src/expand.rs b/pgdog-config/src/expand.rs index 6cb482066..d6f149381 100644 --- a/pgdog-config/src/expand.rs +++ b/pgdog-config/src/expand.rs @@ -1,28 +1,85 @@ //! Environment variable expansion in configuration files. use std::borrow::Cow; -use std::convert::Infallible; use std::env::var; use serde::de::DeserializeOwned; use crate::Error; -/// Expand `$VAR` and `${VAR}` references in a configuration file against the -/// process environment. +/// Start of a variable reference. +const OPEN: &str = "${"; + +/// Expand `${VAR}` references in a configuration file against the process +/// environment. /// -/// References to variables that aren't set are left in the document verbatim, so -/// values that merely contain a `$` (passwords, most commonly) survive -/// untouched. Write `$$` for a literal `$`, and `${VAR:-value}` to supply a -/// fallback. +/// Only the braced form is a reference: a bare `$VAR`, a `${` that's malformed +/// or unterminated, and a reference to a variable that isn't set are all literal +/// text, so values that merely contain a `$` (passwords, most commonly) survive +/// untouched. Write `$${VAR}` for a literal `${VAR}`, and `${VAR:-value}` to +/// supply a fallback. /// /// **Note:** expansion happens on the document source, before it's parsed, so a /// variable is interpolated as TOML rather than as a string. `${PASSWORD}` in /// value position needs surrounding quotes, and a value containing `"` or a /// newline changes how the rest of the document parses. pub fn expand(source: &str) -> Cow<'_, str> { - shellexpand::env_with_context(source, |name| Ok::<_, Infallible>(var(name).ok())) - .expect("lookup is infallible") + if !source.contains(OPEN) { + return Cow::Borrowed(source); + } + + let mut expanded = String::with_capacity(source.len()); + let mut rest = source; + + while let Some(start) = rest.find(OPEN) { + let body = &rest[start + OPEN.len()..]; + + // A reference is `${`, a valid name, an optional `:-fallback`, and `}`. + // Anything else is literal text: emit through the `${` and rescan right + // after it, so a stray `${` in one value can't swallow a real reference + // later in the document. + let reference = body.find('}').and_then(|end| { + let (name, fallback) = match body[..end].split_once(":-") { + Some((name, fallback)) => (name, Some(fallback)), + None => (&body[..end], None), + }; + is_name(name).then_some((name, fallback, end)) + }); + let Some((name, fallback, end)) = reference else { + expanded.push_str(&rest[..start + OPEN.len()]); + rest = body; + continue; + }; + + let stop = start + OPEN.len() + end + 1; + if rest[..start].ends_with('$') { + // `$${VAR}` escapes the reference: drop the `$` and keep the + // reference as written, whether or not the variable is set. + expanded.push_str(&rest[..start - 1]); + expanded.push_str(&rest[start..stop]); + } else { + expanded.push_str(&rest[..start]); + match var(name).ok().as_deref().or(fallback) { + Some(value) => expanded.push_str(value), + // Unset with no fallback: the reference stays as written. + None => expanded.push_str(&rest[start..stop]), + } + } + rest = &rest[stop..]; + } + + expanded.push_str(rest); + Cow::Owned(expanded) +} + +/// Is this a shell variable name, i.e. letters, digits and underscores, not +/// starting with a digit? +fn is_name(name: &str) -> bool { + let mut chars = name.chars(); + chars + .next() + .is_some_and(|first| first.is_ascii_alphabetic() || first == '_') + && chars.all(|c| c.is_ascii_alphanumeric() || c == '_') } /// Parse a TOML configuration document, expanding environment variables first. @@ -53,11 +110,59 @@ mod test { let _unset = remove_env_var("PGDOG_TEST_MISSING"); assert_eq!(expand("${PGDOG_TEST_VAR}"), "expanded"); - assert_eq!(expand("$PGDOG_TEST_VAR/db"), "expanded/db"); + assert_eq!(expand("${PGDOG_TEST_VAR}/db"), "expanded/db"); + assert_eq!( + expand("a${PGDOG_TEST_VAR}b${PGDOG_TEST_VAR}"), + "aexpandedbexpanded" + ); assert_eq!(expand("${PGDOG_TEST_MISSING}"), "${PGDOG_TEST_MISSING}"); assert_eq!(expand("${PGDOG_TEST_MISSING:-fallback}"), "fallback"); + assert_eq!(expand("${PGDOG_TEST_VAR:-fallback}"), "expanded"); + } + + #[test] + fn test_expand_leaves_unbraced_alone() { + let _set = set_env_var("PGDOG_TEST_VAR", "expanded"); + + assert_eq!(expand("$PGDOG_TEST_VAR/db"), "$PGDOG_TEST_VAR/db"); assert_eq!(expand("sup$rsecret"), "sup$rsecret"); - assert_eq!(expand("p$$w0rd"), "p$w0rd"); + assert_eq!(expand("p$$w0rd"), "p$$w0rd"); + } + + #[test] + fn test_expand_leaves_malformed_alone() { + let _set = set_env_var("PGDOG_TEST_VAR", "expanded"); + + assert_eq!(expand("${PGDOG_TEST_VAR"), "${PGDOG_TEST_VAR"); + assert_eq!(expand("${PGDOG TEST VAR}"), "${PGDOG TEST VAR}"); + assert_eq!(expand("${}"), "${}"); + assert_eq!(expand("${1VAR}"), "${1VAR}"); + } + + #[test] + fn test_expand_escape() { + let _set = set_env_var("PGDOG_TEST_VAR", "expanded"); + let _unset = remove_env_var("PGDOG_TEST_MISSING"); + + assert_eq!(expand("$${PGDOG_TEST_VAR}"), "${PGDOG_TEST_VAR}"); + // The escape doesn't depend on the variable being set. + assert_eq!(expand("$${PGDOG_TEST_MISSING}"), "${PGDOG_TEST_MISSING}"); + // Only a well-formed reference needs escaping; a `$` before anything + // else is literal. + assert_eq!(expand("a$${b"), "a$${b"); + assert_eq!(expand("p$${a b}q"), "p$${a b}q"); + } + + #[test] + fn test_expand_scans_past_stray_reference() { + let _set = set_env_var("PGDOG_TEST_VAR", "expanded"); + + // A stray `${` in one value must not swallow a real reference later + // in the document. + assert_eq!( + expand("password = \"ab${cd\"\nhost = \"${PGDOG_TEST_VAR}\""), + "password = \"ab${cd\"\nhost = \"expanded\"" + ); } #[test]