From f6aea3aad6973fa8f0202cd982b76622d5cb2307 Mon Sep 17 00:00:00 2001 From: Chris Marshall Date: Mon, 24 Aug 2026 17:02:53 -0400 Subject: [PATCH 1/3] 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 ed2671ce0..dbf260db9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1584,6 +1584,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" @@ -3015,6 +3036,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" @@ -3194,6 +3221,7 @@ dependencies = [ "serde", "serde_json", "serde_with", + "shellexpand", "tempfile", "thiserror", "toml", @@ -3691,6 +3719,17 @@ dependencies = [ "bitflags 2.11.1", ] +[[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" @@ -4340,6 +4379,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 c5ba25722..7a19abaf6 100644 --- a/pgdog-config/Cargo.toml +++ b/pgdog-config/Cargo.toml @@ -18,6 +18,7 @@ schemars.workspace = true indexmap.workspace = true serde_with.workspace = true derive_more = { workspace = true, features = ["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 010a4518e..31d928ffb 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, User, 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 f76d148af..68deef384 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; @@ -33,6 +34,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 b5fb24606..91e17a516 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, Memory}; +use pgdog_config::{General, FromToml, LogFormat, Memory}; use tracing::level_filters::LevelFilter; use tracing::subscriber::Interest; use tracing::{Event, Metadata, Subscriber}; @@ -347,7 +347,7 @@ fn build_runtime(general: &General, memory: &Memory) -> std::io::Result(&config).ok()) + .and_then(|config| config::Config::from_toml(&config).ok()) .map(|config| config.general) .unwrap_or_default(); From a8584bcd46df61f2ecf78ea16db5ebb8b3032a53 Mon Sep 17 00:00:00 2001 From: Chris Marshall Date: Wed, 2 Sep 2026 11:37:00 -0400 Subject: [PATCH 2/3] 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 | 47 -------------- pgdog-config/Cargo.toml | 1 - pgdog-config/src/expand.rs | 127 +++++++++++++++++++++++++++++++++---- 3 files changed, 116 insertions(+), 59 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index dbf260db9..64401b5a8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1584,27 +1584,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" @@ -3036,12 +3015,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" @@ -3719,17 +3692,6 @@ dependencies = [ "bitflags 2.11.1", ] -[[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" @@ -4379,15 +4341,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 7a19abaf6..c5ba25722 100644 --- a/pgdog-config/Cargo.toml +++ b/pgdog-config/Cargo.toml @@ -18,7 +18,6 @@ schemars.workspace = true indexmap.workspace = true serde_with.workspace = true derive_more = { workspace = true, features = ["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] From a45b98095c0718cda9e677770ed3acb3eda761b0 Mon Sep 17 00:00:00 2001 From: Chris Marshall Date: Tue, 15 Sep 2026 13:26:34 -0400 Subject: [PATCH 3/3] feat(config): read secrets from files with ${file.PATH} MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Configuration files can now interpolate the contents of a file, so a secret mounted into the container never has to be copied into an environment variable to reach `pgdog.toml`: ```toml [admin] password = "${file./run/secrets/admin_password}" ``` Environment references move under an `env.` prefix at the same time: `${env.VAR}` where it used to be `${VAR}`. Both prefixes are required, so a plain `${VAR}` is now literal text. The two forms are otherwise identical — `${env.VAR:-value}` and `${file.PATH:-value}` both supply a fallback, and `$${env.VAR}` is still a literal `${env.VAR}`. This is a breaking change to syntax introduced in the two commits right before it and never released, so nothing in the wild depends on the unprefixed form. The namespace is what makes a second reference kind possible without guessing at the target from its shape, and it widens the set of values that pass through untouched: a `${...}` that isn't one of the two known prefixes is left alone rather than being treated as a lookup that happened to miss. File references are strict where environment references are lenient. An unset variable is left in the document verbatim, because `users.toml` is full of values that may legitimately contain a `$` and failing the load on one would be worse than leaving it be. A `file.` reference names a path the operator wrote down on purpose, so a file that can't be read is an error — `Error::FileReference`, carrying the path and the underlying `io::Error` — unless a fallback is given. A silently empty password is the failure mode worth avoiding here. Trailing newlines are trimmed from file contents. Secret managers and `echo` alike conventionally leave one behind, and since expansion runs on the document source before it is parsed, that newline would otherwise land in the middle of a TOML string and change how the rest of the document parses. Implementation notes: - `expand()` now returns `Result, Error>`; it still returns `Cow::Borrowed` when the source contains no `${`, so the common case costs no allocation. `FromToml::from_toml` propagates the error. - A `Reference` enum carries the parsed target, so the recognition step stays where the `}` is found and the substitution step just matches on what was recognised. - `is_path` is deliberately loose — non-empty, no whitespace, no `$` or `{`. It exists to decide reference-or-literal, not to validate a path; anything stricter would reject paths that the filesystem accepts, and `read_to_string` reports the real answer anyway. - Only trailing `\r` and `\n` are trimmed, not interior newlines or trailing spaces, both of which can be part of a secret. - Tests write real files with `tempfile` rather than mocking the read, which keeps the newline-trimming and missing-file cases honest. --- Cargo.lock | 1 - pgdog-config/src/error.rs | 3 + pgdog-config/src/expand.rs | 254 ++++++++++++++++++++++++++++--------- 3 files changed, 197 insertions(+), 61 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 64401b5a8..ed2671ce0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3194,7 +3194,6 @@ dependencies = [ "serde", "serde_json", "serde_with", - "shellexpand", "tempfile", "thiserror", "toml", diff --git a/pgdog-config/src/error.rs b/pgdog-config/src/error.rs index fc36f2753..33f385ba4 100644 --- a/pgdog-config/src/error.rs +++ b/pgdog-config/src/error.rs @@ -28,6 +28,9 @@ pub enum Error { #[error("parse error: {0}")] ParseError(String), + + #[error("cannot read ${{file.{0}}}: {1}")] + FileReference(String, std::io::Error), } impl Error { diff --git a/pgdog-config/src/expand.rs b/pgdog-config/src/expand.rs index d6f149381..0d4ce77f2 100644 --- a/pgdog-config/src/expand.rs +++ b/pgdog-config/src/expand.rs @@ -1,7 +1,8 @@ -//! Environment variable expansion in configuration files. +//! Environment variable and file expansion in configuration files. use std::borrow::Cow; use std::env::var; +use std::fs::read_to_string; use serde::de::DeserializeOwned; @@ -10,22 +11,44 @@ use crate::Error; /// Start of a variable reference. const OPEN: &str = "${"; -/// Expand `${VAR}` references in a configuration file against the process -/// environment. +/// Prefix of an environment variable reference. +const ENV: &str = "env."; + +/// Prefix of a file reference. +const FILE: &str = "file."; + +/// A parsed `${env.NAME}` or `${file.PATH}` reference. +enum Reference<'a> { + Env(&'a str), + File(&'a str), +} + +/// Expand `${env.VAR}` and `${file.PATH}` references in a configuration file. +/// +/// `${env.VAR}` interpolates the environment variable `VAR` from the process +/// environment. `${file.PATH}` interpolates the contents of the file at +/// `PATH`, with trailing newlines trimmed; a relative path resolves against +/// the current working directory. /// -/// 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. +/// Only the braced, prefixed form is a reference: a bare `$VAR`, an unprefixed +/// `${VAR}`, a `${` that's malformed or unterminated, and an `env.` reference +/// to a variable that isn't set are all literal text, so values that merely +/// contain a `$` (passwords, most commonly) survive untouched. Write +/// `$${env.VAR}` for a literal `${env.VAR}`, and `${env.VAR:-value}` or +/// `${file.PATH:-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 +/// reference is interpolated as TOML rather than as a string. `${env.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> { +/// +/// # Errors +/// +/// Returns [`Error::FileReference`] if a `file.` reference without a fallback +/// names a file that can't be read. +pub fn expand(source: &str) -> Result, Error> { if !source.contains(OPEN) { - return Cow::Borrowed(source); + return Ok(Cow::Borrowed(source)); } let mut expanded = String::with_capacity(source.len()); @@ -34,18 +57,26 @@ pub fn expand(source: &str) -> Cow<'_, str> { 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. + // A reference is `${`, `env.` plus a valid name or `file.` plus a + // plausible path, 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)), + let (target, fallback) = match body[..end].split_once(":-") { + Some((target, fallback)) => (target, Some(fallback)), None => (&body[..end], None), }; - is_name(name).then_some((name, fallback, end)) + let reference = if let Some(name) = target.strip_prefix(ENV) { + is_name(name).then_some(Reference::Env(name)) + } else if let Some(path) = target.strip_prefix(FILE) { + is_path(path).then_some(Reference::File(path)) + } else { + None + }?; + Some((reference, fallback, end)) }); - let Some((name, fallback, end)) = reference else { + let Some((reference, fallback, end)) = reference else { expanded.push_str(&rest[..start + OPEN.len()]); rest = body; continue; @@ -53,23 +84,34 @@ pub fn expand(source: &str) -> Cow<'_, str> { 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. + // `$${env.VAR}` escapes the reference: drop the `$` and keep the + // reference as written, whether or not it resolves. 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]), + match reference { + Reference::Env(name) => 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]), + }, + Reference::File(path) => match read_to_string(path) { + // Mounted secrets conventionally end with a newline that + // would corrupt the surrounding TOML. + Ok(contents) => expanded.push_str(contents.trim_end_matches(['\r', '\n'])), + Err(err) => match fallback { + Some(value) => expanded.push_str(value), + None => return Err(Error::FileReference(path.into(), err)), + }, + }, } } rest = &rest[stop..]; } expanded.push_str(rest); - Cow::Owned(expanded) + Ok(Cow::Owned(expanded)) } /// Is this a shell variable name, i.e. letters, digits and underscores, not @@ -82,16 +124,26 @@ fn is_name(name: &str) -> bool { && chars.all(|c| c.is_ascii_alphanumeric() || c == '_') } -/// Parse a TOML configuration document, expanding environment variables first. +/// Is this a plausible file path, i.e. non-empty with no whitespace or +/// reference syntax? Anything else is literal text, not a reference. +fn is_path(path: &str) -> bool { + !path.is_empty() + && path + .chars() + .all(|c| !c.is_whitespace() && c != '$' && c != '{') +} + +/// Parse a TOML configuration document, expanding variable references first. pub trait FromToml: DeserializeOwned { - /// Parse `source` as TOML, [`expand`]ing environment variables first. + /// Parse `source` as TOML, [`expand`]ing variable references first. /// /// # Errors /// - /// Returns [`Error::MissingField`] if the expanded document isn't valid TOML - /// or doesn't match the shape of `Self`. + /// Returns [`Error::FileReference`] if a `file.` reference can't be read, + /// or [`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); + let expanded = expand(source)?; toml::from_str(&expanded).map_err(|err| Error::config(&expanded, err)) } } @@ -100,43 +152,108 @@ impl FromToml for T {} #[cfg(test)] mod test { + use std::io::Write; + + use tempfile::NamedTempFile; + use super::*; use crate::test_utils::{remove_env_var, set_env_var}; use crate::{Config, Users}; + fn expanded(source: &str) -> String { + expand(source).unwrap().into_owned() + } + + fn secret_file(contents: &str) -> NamedTempFile { + let mut file = NamedTempFile::new().unwrap(); + file.write_all(contents.as_bytes()).unwrap(); + file + } + #[test] - fn test_expand() { + fn test_expand_env() { 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!(expanded("${env.PGDOG_TEST_VAR}"), "expanded"); + assert_eq!(expanded("${env.PGDOG_TEST_VAR}/db"), "expanded/db"); assert_eq!( - expand("a${PGDOG_TEST_VAR}b${PGDOG_TEST_VAR}"), + expanded("a${env.PGDOG_TEST_VAR}b${env.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"); + assert_eq!( + expanded("${env.PGDOG_TEST_MISSING}"), + "${env.PGDOG_TEST_MISSING}" + ); + assert_eq!(expanded("${env.PGDOG_TEST_MISSING:-fallback}"), "fallback"); + assert_eq!(expanded("${env.PGDOG_TEST_VAR:-fallback}"), "expanded"); + } + + #[test] + fn test_expand_file() { + let file = secret_file("not a real secret\n"); + let path = file.path().display(); + + assert_eq!(expanded(&format!("${{file.{path}}}")), "not a real secret"); + assert_eq!( + expanded(&format!("a${{file.{path}}}b")), + "anot a real secretb" + ); + // A fallback covers a file that can't be read, set or not. + assert_eq!( + expanded(&format!("${{file.{path}:-fallback}}")), + "not a real secret" + ); + assert_eq!( + expanded("${file./pgdog/no/such/file:-fallback}"), + "fallback" + ); + } + + #[test] + fn test_expand_file_trims_trailing_newlines() { + let trailing = secret_file("secret\r\n\n"); + assert_eq!( + expanded(&format!("${{file.{}}}", trailing.path().display())), + "secret" + ); + + // Only trailing newlines are trimmed, not interior ones or spaces. + let interior = secret_file("a\nb "); + assert_eq!( + expanded(&format!("${{file.{}}}", interior.path().display())), + "a\nb " + ); + } + + #[test] + fn test_expand_file_missing_is_error() { + let err = expand("${file./pgdog/no/such/file}").unwrap_err(); + assert!(matches!(err, Error::FileReference(..)), "{err:?}"); } #[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!(expanded("$PGDOG_TEST_VAR/db"), "$PGDOG_TEST_VAR/db"); + assert_eq!(expanded("sup$rsecret"), "sup$rsecret"); + assert_eq!(expanded("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}"); + // An unprefixed reference is literal text. + assert_eq!(expanded("${PGDOG_TEST_VAR}"), "${PGDOG_TEST_VAR}"); + assert_eq!(expanded("${env.PGDOG_TEST_VAR"), "${env.PGDOG_TEST_VAR"); + assert_eq!(expanded("${env.PGDOG TEST VAR}"), "${env.PGDOG TEST VAR}"); + assert_eq!(expanded("${env.}"), "${env.}"); + assert_eq!(expanded("${env.1VAR}"), "${env.1VAR}"); + assert_eq!(expanded("${file.}"), "${file.}"); + assert_eq!(expanded("${file.a b}"), "${file.a b}"); + assert_eq!(expanded("${file.a${b}"), "${file.a${b}"); } #[test] @@ -144,13 +261,20 @@ mod test { 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}"); + assert_eq!(expanded("$${env.PGDOG_TEST_VAR}"), "${env.PGDOG_TEST_VAR}"); + // The escape doesn't depend on the reference resolving. + assert_eq!( + expanded("$${env.PGDOG_TEST_MISSING}"), + "${env.PGDOG_TEST_MISSING}" + ); + assert_eq!( + expanded("$${file./pgdog/no/such/file}"), + "${file./pgdog/no/such/file}" + ); // 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"); + assert_eq!(expanded("a$${env.b"), "a$${env.b"); + assert_eq!(expanded("p$${env.a b}q"), "p$${env.a b}q"); } #[test] @@ -160,7 +284,7 @@ mod test { // 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}\""), + expanded("password = \"ab${cd\"\nhost = \"${env.PGDOG_TEST_VAR}\""), "password = \"ab${cd\"\nhost = \"expanded\"" ); } @@ -168,17 +292,20 @@ mod test { #[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 timeout = secret_file("1_000\n"); - let source = r#" + let source = format!( + r#" [admin] -password = "${PGDOG_TEST_PASSWORD}" +password = "${{env.PGDOG_TEST_PASSWORD}}" [general] -shutdown_timeout = ${PGDOG_TEST_SHUTDOWN_TIMEOUT} -"#; +shutdown_timeout = ${{file.{}}} +"#, + timeout.path().display() + ); - let config = Config::from_toml(source).unwrap(); + let config = Config::from_toml(&source).unwrap(); assert_eq!(config.admin.password, "not a real secret"); assert_eq!(config.general.shutdown_timeout, 1_000); } @@ -191,16 +318,23 @@ shutdown_timeout = ${PGDOG_TEST_SHUTDOWN_TIMEOUT} [[users]] name = "pgdog" database = "pgdog" -password = "${PGDOG_TEST_MISSING}" +password = "${env.PGDOG_TEST_MISSING}" "#; let users = Users::from_toml(source).unwrap(); assert_eq!( users.users[0].password.as_deref(), - Some("${PGDOG_TEST_MISSING}") + Some("${env.PGDOG_TEST_MISSING}") ); } + #[test] + fn test_from_toml_reports_missing_file() { + let err = + Users::from_toml("[[users]]\nname = \"${file./pgdog/no/such/file}\"\n").unwrap_err(); + assert!(matches!(err, Error::FileReference(..)), "{err:?}"); + } + #[test] fn test_from_toml_reports_errors() { let err = Config::from_toml("[general]\nnot_a_field = 1\n").unwrap_err();