From 01d219091ee23789a241873f400e975f60e5a540 Mon Sep 17 00:00:00 2001 From: sehkone Date: Sun, 9 Aug 2026 22:44:25 +0900 Subject: [PATCH 1/4] Pass environment values into the resolvers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In Rust 2024 `set_var`/`remove_var` are unsafe, and the shared mutex these tests took before each mutation does not discharge that contract: it serialises only the code that takes the lock, while the tokio runtime, the HTTP client, the env-filtered subscriber and the libc resolver all read the environment from threads that hold nothing. Every resolver a test steered now takes the value as a parameter, with one thin wrapper left at the composition root doing the read. The precedence, the fallbacks and the variables consulted are unchanged; what moves is where they are read. The `.env` load keeps its single production `set_var`, with the decision of which pairs to apply split off into a pure function the tests can drive. That removes the last user of `compose_project::test_env`, of `openbao_auth`'s `ENV_LOCK`, of `init::steps::test_support`'s lock and of the three anonymous `OnceLock>` guards, all of which are deleted — clippy fails on a dead static, so they cannot be left for a later tidy-up. `reinit`'s scope-check test no longer moves the whole process into a tempdir either: the working directory is process-global exactly as the environment is, and the `.` it was reproducing is a property of `compose_file_dir`, not of where the process happens to stand. Closes #806 --- src/commands/clean.rs | 46 +-- src/commands/compose_project.rs | 218 ++++++------ src/commands/container_name.rs | 19 +- src/commands/dotenv.rs | 189 +++++------ src/commands/infra.rs | 71 +++- src/commands/init/steps.rs | 10 - src/commands/init/steps/database.rs | 429 ++++++++++++++---------- src/commands/init/steps/orchestrator.rs | 41 ++- src/commands/openbao_auth.rs | 141 ++++---- src/commands/openbao_url.rs | 148 ++------ src/commands/reinit.rs | 189 ++++++++--- src/commands/rotate/db.rs | 55 +-- src/commands/status.rs | 24 +- src/db.rs | 80 ++--- src/host_port.rs | 339 +++++++++++-------- 15 files changed, 1069 insertions(+), 930 deletions(-) diff --git a/src/commands/clean.rs b/src/commands/clean.rs index 0ed18100..0ce88563 100644 --- a/src/commands/clean.rs +++ b/src/commands/clean.rs @@ -209,7 +209,6 @@ fn remove_file_if_exists(path: &Path, messages: &Messages) -> Result<()> { mod tests { use super::*; use crate::commands::compose_project::resolve_compose_project_for_dir; - use crate::commands::compose_project::test_env::{ComposeProjectEnv, env_lock}; use crate::i18n::Messages; // The project-resolution tests below cover what the retired @@ -261,8 +260,6 @@ mod tests { /// recorded identity is what drives the `_` prefix. #[test] fn resolve_compose_project_uses_the_recorded_instance() { - let _guard = env_lock(); - let _env = ComposeProjectEnv::set(None); let dir = tempfile::tempdir().unwrap(); std::fs::write( dir.path().join(".env"), @@ -270,7 +267,7 @@ mod tests { ) .unwrap(); let project = - resolve_compose_project_for_dir(dir.path(), None, &Messages::new("en").unwrap()) + resolve_compose_project_for_dir(dir.path(), None, None, &Messages::new("en").unwrap()) .unwrap(); assert_eq!(project, "real-project-name"); } @@ -280,13 +277,12 @@ mod tests { /// targets the same project a fresh `infra install` created. #[test] fn resolve_compose_project_falls_back_to_the_fixed_default() { - let _guard = env_lock(); - let _env = ComposeProjectEnv::set(None); let root = tempfile::tempdir().unwrap(); let dir = root.path().join("Bootroot.Stack"); std::fs::create_dir(&dir).unwrap(); let project = - resolve_compose_project_for_dir(&dir, None, &Messages::new("en").unwrap()).unwrap(); + resolve_compose_project_for_dir(&dir, None, None, &Messages::new("en").unwrap()) + .unwrap(); assert_eq!(project, "bootroot"); } @@ -296,13 +292,15 @@ mod tests { /// the stack comes back on another. #[test] fn resolve_compose_project_honours_env_var_over_the_recorded_instance() { - let _guard = env_lock(); - let _env = ComposeProjectEnv::set(Some("env-project")); let dir = tempfile::tempdir().unwrap(); std::fs::write(dir.path().join(".env"), "BOOTROOT_INSTANCE=recorded\n").unwrap(); - let project = - resolve_compose_project_for_dir(dir.path(), None, &Messages::new("en").unwrap()) - .unwrap(); + let project = resolve_compose_project_for_dir( + dir.path(), + None, + Some("env-project"), + &Messages::new("en").unwrap(), + ) + .unwrap(); assert_eq!(project, "env-project"); } @@ -312,13 +310,15 @@ mod tests { /// `.env` beside it. #[test] fn resolve_compose_project_handles_dot_relative_compose_dir() { - let _guard = env_lock(); - let _env = ComposeProjectEnv::set(None); let compose_dir = compose_file_dir(Path::new("docker-compose.yml")); assert_eq!(compose_dir, std::path::PathBuf::from(".")); - let project = - resolve_compose_project_for_dir(&compose_dir, None, &Messages::new("en").unwrap()) - .expect("resolve_compose_project_for_dir must succeed for `.`"); + let project = resolve_compose_project_for_dir( + &compose_dir, + None, + None, + &Messages::new("en").unwrap(), + ) + .expect("resolve_compose_project_for_dir must succeed for `.`"); assert!(!project.is_empty()); } @@ -326,12 +326,14 @@ mod tests { /// must still win when the compose dir is the helper-normalised `.`. #[test] fn resolve_compose_project_honours_env_var_for_dot_relative_compose_dir() { - let _guard = env_lock(); - let _env = ComposeProjectEnv::set(Some("explicit-env-project")); let compose_dir = compose_file_dir(Path::new("docker-compose.yml")); - let project = - resolve_compose_project_for_dir(&compose_dir, None, &Messages::new("en").unwrap()) - .unwrap(); + let project = resolve_compose_project_for_dir( + &compose_dir, + None, + Some("explicit-env-project"), + &Messages::new("en").unwrap(), + ) + .unwrap(); assert_eq!(project, "explicit-env-project"); } } diff --git a/src/commands/compose_project.rs b/src/commands/compose_project.rs index ab21a63e..91139cfb 100644 --- a/src/commands/compose_project.rs +++ b/src/commands/compose_project.rs @@ -98,7 +98,12 @@ fn recorded_instance_name(compose_dir: &Path, messages: &Messages) -> Result Option { +/// +/// The crate's single read of that variable. Every resolver below takes +/// the value as a parameter, so the commands at the composition root +/// call this once and the resolvers stay steerable without the +/// process-global environment. +pub(crate) fn compose_project_name_override() -> Option { std::env::var(COMPOSE_PROJECT_NAME_ENV) .ok() .filter(|value| !value.is_empty()) @@ -109,22 +114,24 @@ fn compose_project_name_override() -> Option { /// /// 1. `instance_name` — the `--instance-name` value, which only /// `infra install` can supply; -/// 2. `COMPOSE_PROJECT_NAME` from the invoking environment, when -/// non-empty. Used verbatim and deliberately not validated: the E2E -/// harness's per-run project names are longer than an instance name -/// may be, and they must keep working; +/// 2. `project_override` — what the invoking environment's +/// `COMPOSE_PROJECT_NAME` held, per +/// [`compose_project_name_override`]. Used verbatim and deliberately +/// not validated: the E2E harness's per-run project names are longer +/// than an instance name may be, and they must keep working; /// 3. `BOOTROOT_INSTANCE` from `/.env`; /// 4. the literal [`DEFAULT_INSTANCE_NAME`]. pub(crate) fn resolve_compose_project_for_dir( compose_dir: &Path, instance_name: Option<&str>, + project_override: Option<&str>, messages: &Messages, ) -> Result { if let Some(name) = instance_name { return Ok(name.to_string()); } - if let Some(project) = compose_project_name_override() { - return Ok(project); + if let Some(project) = project_override.filter(|value| !value.is_empty()) { + return Ok(project.to_string()); } if let Some(recorded) = recorded_instance_name(compose_dir, messages)? { return Ok(recorded); @@ -185,17 +192,42 @@ impl ComposeIdentity { } } - /// Resolves both halves of the identity for a compose directory. + /// Resolves both halves of the identity for a compose directory, + /// reading `COMPOSE_PROJECT_NAME` from the invoking environment. + /// + /// The composition root for that variable: the resolvers underneath + /// take it as a parameter. pub(crate) fn resolve_for_dir( compose_dir: &Path, instance_name: Option<&str>, messages: &Messages, + ) -> Result { + Self::resolve_for_dir_with_override( + compose_dir, + instance_name, + compose_project_name_override().as_deref(), + messages, + ) + } + + /// [`ComposeIdentity::resolve_for_dir`] with the + /// `COMPOSE_PROJECT_NAME` override supplied by the caller. + fn resolve_for_dir_with_override( + compose_dir: &Path, + instance_name: Option<&str>, + project_override: Option<&str>, + messages: &Messages, ) -> Result { if let Some(name) = instance_name { return Ok(Self::for_instance(name)); } Ok(Self { - project: resolve_compose_project_for_dir(compose_dir, None, messages)?, + project: resolve_compose_project_for_dir( + compose_dir, + None, + project_override, + messages, + )?, instance_name: resolve_recorded_instance_name(compose_dir, None, messages)?, }) } @@ -306,78 +338,8 @@ fn compose_args( args } -/// Test-only helpers for the process-global `COMPOSE_PROJECT_NAME`. -/// -/// One lock for the whole crate: the resolver is exercised from several -/// modules' tests, and a per-module mutex would let two of them mutate -/// the same process-global variable concurrently. -#[cfg(test)] -pub(crate) mod test_env { - use std::ffi::OsString; - use std::sync::{LazyLock, Mutex, MutexGuard}; - - use super::COMPOSE_PROJECT_NAME_ENV; - - static ENV_LOCK: LazyLock> = LazyLock::new(|| Mutex::new(())); - - /// Serialises tests that mutate `COMPOSE_PROJECT_NAME`. - /// - /// A panicking test poisons the mutex; recovering the guard rather - /// than propagating the poison keeps one failure from cascading into - /// every other test that touches the variable. - pub(crate) fn env_lock() -> MutexGuard<'static, ()> { - ENV_LOCK - .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner) - } - - /// RAII guard restoring a process-global variable on drop, so a - /// failing assertion cannot leave the shared environment mutated. - pub(crate) struct ScopedEnv { - key: &'static str, - prior: Option, - } - - impl ScopedEnv { - /// Sets (or clears) `key` for the duration of a test. Callers - /// must hold [`env_lock`]. - pub(crate) fn set(key: &'static str, value: Option<&str>) -> Self { - let prior = std::env::var_os(key); - match value { - // SAFETY: every call site holds `env_lock()`. - Some(value) => unsafe { std::env::set_var(key, value) }, - // SAFETY: as above. - None => unsafe { std::env::remove_var(key) }, - } - Self { key, prior } - } - } - - impl Drop for ScopedEnv { - fn drop(&mut self) { - match self.prior.take() { - // SAFETY: the caller still holds `env_lock()`. - Some(prior) => unsafe { std::env::set_var(self.key, prior) }, - // SAFETY: as above. - None => unsafe { std::env::remove_var(self.key) }, - } - } - } - - /// [`ScopedEnv`] pinned to `COMPOSE_PROJECT_NAME`, the variable most - /// of these tests scope. - pub(crate) struct ComposeProjectEnv; - - impl ComposeProjectEnv { - pub(crate) fn set(value: Option<&str>) -> ScopedEnv { - ScopedEnv::set(COMPOSE_PROJECT_NAME_ENV, value) - } - } -} - #[cfg(test)] mod tests { - use super::test_env::{ComposeProjectEnv, ScopedEnv, env_lock}; use super::*; use crate::i18n::test_messages; @@ -443,32 +405,38 @@ mod tests { #[test] fn flag_wins_over_compose_project_name() { - let _guard = env_lock(); - let _env = ComposeProjectEnv::set(Some("env-project")); let dir = tempfile::tempdir().unwrap(); write_dotenv_with_instance(dir.path(), "recorded"); - let project = - resolve_compose_project_for_dir(dir.path(), Some("flag"), &test_messages()).unwrap(); + let project = resolve_compose_project_for_dir( + dir.path(), + Some("flag"), + Some("env-project"), + &test_messages(), + ) + .unwrap(); assert_eq!(project, "flag"); } #[test] fn compose_project_name_wins_over_dotenv() { - let _guard = env_lock(); - let _env = ComposeProjectEnv::set(Some("env-project")); let dir = tempfile::tempdir().unwrap(); write_dotenv_with_instance(dir.path(), "recorded"); - let project = resolve_compose_project_for_dir(dir.path(), None, &test_messages()).unwrap(); + let project = resolve_compose_project_for_dir( + dir.path(), + None, + Some("env-project"), + &test_messages(), + ) + .unwrap(); assert_eq!(project, "env-project"); } #[test] fn dotenv_wins_over_the_default() { - let _guard = env_lock(); - let _env = ComposeProjectEnv::set(None); let dir = tempfile::tempdir().unwrap(); write_dotenv_with_instance(dir.path(), "recorded"); - let project = resolve_compose_project_for_dir(dir.path(), None, &test_messages()).unwrap(); + let project = + resolve_compose_project_for_dir(dir.path(), None, None, &test_messages()).unwrap(); assert_eq!(project, "recorded"); } @@ -477,22 +445,24 @@ mod tests { /// named. #[test] fn default_is_the_fixed_literal_regardless_of_directory_name() { - let _guard = env_lock(); - let _env = ComposeProjectEnv::set(None); let root = tempfile::tempdir().unwrap(); let dir = root.path().join("clumit-insight"); std::fs::create_dir(&dir).unwrap(); - let project = resolve_compose_project_for_dir(&dir, None, &test_messages()).unwrap(); + let project = resolve_compose_project_for_dir(&dir, None, None, &test_messages()).unwrap(); assert_eq!(project, DEFAULT_INSTANCE_NAME); } + /// `COMPOSE_PROJECT_NAME=` is how a shell clears the variable for + /// one command, so an empty value has to fall through exactly as an + /// absent one does — both at the read + /// ([`compose_project_name_override`]) and at the resolver, which + /// is reachable with a value the read never filtered. #[test] fn empty_compose_project_name_is_treated_as_unset() { - let _guard = env_lock(); - let _env = ComposeProjectEnv::set(Some("")); let dir = tempfile::tempdir().unwrap(); write_dotenv_with_instance(dir.path(), "recorded"); - let project = resolve_compose_project_for_dir(dir.path(), None, &test_messages()).unwrap(); + let project = + resolve_compose_project_for_dir(dir.path(), None, Some(""), &test_messages()).unwrap(); assert_eq!(project, "recorded"); } @@ -502,13 +472,13 @@ mod tests { #[test] fn compose_project_name_is_not_validated_as_an_instance_name() { const HARNESS_PROJECT: &str = "bootroot-e2e-ci-openbao-tls-no-delta-1234567"; - let _guard = env_lock(); - let _env = ComposeProjectEnv::set(Some(HARNESS_PROJECT)); let messages = test_messages(); assert!(HARNESS_PROJECT.len() > MAX_INSTANCE_NAME_LEN); assert!(validate_instance_name(HARNESS_PROJECT, &messages).is_err()); let dir = tempfile::tempdir().unwrap(); - let project = resolve_compose_project_for_dir(dir.path(), None, &messages).unwrap(); + let project = + resolve_compose_project_for_dir(dir.path(), None, Some(HARNESS_PROJECT), &messages) + .unwrap(); assert_eq!(project, HARNESS_PROJECT); } @@ -518,8 +488,6 @@ mod tests { /// wrong project's volumes. #[test] fn resolver_reads_the_dotenv_beside_the_given_compose_file() { - let _guard = env_lock(); - let _env = ComposeProjectEnv::set(None); let root = tempfile::tempdir().unwrap(); let here = root.path().join("here"); let there = root.path().join("there"); @@ -538,17 +506,23 @@ mod tests { /// throwaway harness project must not become the install's identity. #[test] fn recorded_identity_ignores_compose_project_name() { - let _guard = env_lock(); - let _env = ComposeProjectEnv::set(Some("bootroot-e2e-ci-reinit-42")); + const HARNESS_PROJECT: &str = "bootroot-e2e-ci-reinit-42"; + let messages = test_messages(); let dir = tempfile::tempdir().unwrap(); - let recorded = resolve_recorded_instance_name(dir.path(), None, &test_messages()).unwrap(); + // The contrast is the point: the same override that decides the + // project has no way into the recorded identity, because + // `resolve_recorded_instance_name` does not take one. + assert_eq!( + resolve_compose_project_for_dir(dir.path(), None, Some(HARNESS_PROJECT), &messages) + .unwrap(), + HARNESS_PROJECT + ); + let recorded = resolve_recorded_instance_name(dir.path(), None, &messages).unwrap(); assert_eq!(recorded, DEFAULT_INSTANCE_NAME); } #[test] fn recorded_identity_preserves_the_existing_value_without_the_flag() { - let _guard = env_lock(); - let _env = ComposeProjectEnv::set(Some("bootroot-e2e-ci-reinit-42")); let dir = tempfile::tempdir().unwrap(); write_dotenv_with_instance(dir.path(), "insight"); let recorded = resolve_recorded_instance_name(dir.path(), None, &test_messages()).unwrap(); @@ -557,8 +531,6 @@ mod tests { #[test] fn recorded_identity_takes_the_flag_over_the_existing_value() { - let _guard = env_lock(); - let _env = ComposeProjectEnv::set(None); let dir = tempfile::tempdir().unwrap(); write_dotenv_with_instance(dir.path(), "insight"); let recorded = @@ -835,17 +807,24 @@ mod tests { /// An inherited `BOOTROOT_INSTANCE` must never reach Compose: it /// would outrank the compose directory's `.env` and rename the /// containers of the install being acted on. + /// + /// What closes that is unconditional: `command()` sets the variable + /// on every invocation it builds, so whatever the invoking + /// environment exported is replaced rather than inherited. The + /// assertion is therefore on the child environment the builder + /// declares, which is the only thing the ambient value could have + /// competed with. #[test] fn recorded_instance_overrides_an_inherited_variable() { - let _guard = env_lock(); - let _project = ComposeProjectEnv::set(None); - let _inherited = ScopedEnv::set(INSTANCE_NAME_ENV_KEY, Some("other")); let dir = tempfile::tempdir().unwrap(); write_dotenv_with_instance(dir.path(), "insight"); - let identity = - ComposeIdentity::resolve_for_dir(dir.path(), None, &test_messages()).unwrap(); - // The child value is set unconditionally, so the exported - // `other` is replaced rather than inherited. + let identity = ComposeIdentity::resolve_for_dir_with_override( + dir.path(), + None, + None, + &test_messages(), + ) + .unwrap(); let command = identity .compose(&["docker-compose.yml"], None, &["up", "-d"]) .command(&[]); @@ -867,12 +846,15 @@ mod tests { #[test] fn long_compose_project_scopes_the_project_but_not_the_containers() { const HARNESS_PROJECT: &str = "bootroot-e2e-ci-openbao-tls-no-delta-1234567"; - let _guard = env_lock(); - let _env = ComposeProjectEnv::set(Some(HARNESS_PROJECT)); let dir = tempfile::tempdir().unwrap(); write_dotenv_with_instance(dir.path(), DEFAULT_INSTANCE_NAME); - let identity = - ComposeIdentity::resolve_for_dir(dir.path(), None, &test_messages()).unwrap(); + let identity = ComposeIdentity::resolve_for_dir_with_override( + dir.path(), + None, + Some(HARNESS_PROJECT), + &test_messages(), + ) + .unwrap(); assert_eq!(identity.project(), HARNESS_PROJECT); assert_eq!( identity.container(BootrootContainer::OpenBao), diff --git a/src/commands/container_name.rs b/src/commands/container_name.rs index 72ecb0b8..0b334805 100644 --- a/src/commands/container_name.rs +++ b/src/commands/container_name.rs @@ -125,7 +125,7 @@ pub(crate) fn resolve_container_name( mod tests { use super::*; use crate::commands::compose_project::{ - DEFAULT_INSTANCE_NAME, INSTANCE_NAME_ENV_KEY, test_env, + DEFAULT_INSTANCE_NAME, INSTANCE_NAME_ENV_KEY, resolve_compose_project_for_dir, }; use crate::i18n::test_messages; @@ -218,8 +218,6 @@ mod tests { #[test] fn resolve_reads_the_identity_recorded_beside_the_compose_file() { - let _guard = test_env::env_lock(); - let _env = test_env::ComposeProjectEnv::set(None); let dir = tempfile::tempdir().expect("tempdir"); std::fs::write( dir.path().join(".env"), @@ -302,14 +300,21 @@ mod tests { /// than an instance name may be. #[test] fn resolve_ignores_the_compose_project_override() { - let _guard = test_env::env_lock(); - let _env = - test_env::ComposeProjectEnv::set(Some("bootroot-e2e-ci-openbao-tls-no-delta-1234567")); + const HARNESS_PROJECT: &str = "bootroot-e2e-ci-openbao-tls-no-delta-1234567"; + let messages = test_messages(); let dir = tempfile::tempdir().expect("tempdir"); + // The same override that selects the Compose project has no + // route into the container name: `resolve_container_name` goes + // through `resolve_recorded_instance_name`, which takes none. + assert_eq!( + resolve_compose_project_for_dir(dir.path(), None, Some(HARNESS_PROJECT), &messages) + .expect("the override must select the project"), + HARNESS_PROJECT + ); let name = resolve_container_name( &dir.path().join("docker-compose.yml"), BootrootContainer::Http01, - &test_messages(), + &messages, ) .expect("resolving the container name must succeed"); assert_eq!(name, "bootroot-http01"); diff --git a/src/commands/dotenv.rs b/src/commands/dotenv.rs index ffc73505..7974e8ac 100644 --- a/src/commands/dotenv.rs +++ b/src/commands/dotenv.rs @@ -62,6 +62,25 @@ pub(crate) fn write_dotenv( Ok(()) } +/// Decides which of `entries` [`load_dotenv_into_env`] would apply, +/// given `is_set`, which answers whether a key already has a value in +/// the target environment. +/// +/// The whole decision, split off from the one mutation it feeds so it +/// can be exercised — the exclusion below above all — without a process +/// environment to steer. +fn dotenv_pairs_to_apply<'a>( + entries: &'a BTreeMap, + is_set: &dyn Fn(&str) -> bool, +) -> Vec<(&'a str, &'a str)> { + entries + .iter() + .filter(|(key, _)| key.as_str() != COMPOSE_PROJECT_NAME_ENV) + .filter(|(key, _)| !is_set(key)) + .map(|(key, value)| (key.as_str(), value.as_str())) + .collect() +} + /// Loads key-value pairs from a `.env` file into the process environment. /// /// Only sets variables that are not already present in the process @@ -85,16 +104,11 @@ pub(crate) fn load_dotenv_into_env(path: &Path, messages: &Messages) -> Result<( return Ok(()); } let map = read_dotenv(path, messages)?; - for (key, value) in &map { - if key == COMPOSE_PROJECT_NAME_ENV { - continue; - } - if std::env::var(key).is_err() { - // SAFETY: called once during single-threaded init setup, - // before any worker threads are spawned. - unsafe { - std::env::set_var(key, value); - } + for (key, value) in dotenv_pairs_to_apply(&map, &|key| std::env::var(key).is_ok()) { + // SAFETY: called once during single-threaded init setup, + // before any worker threads are spawned. + unsafe { + std::env::set_var(key, value); } } Ok(()) @@ -194,55 +208,36 @@ mod tests { assert_eq!(map.get("B").unwrap(), "keep"); } - #[test] - fn test_load_dotenv_into_env_sets_missing_vars() { - let dir = tempdir().unwrap(); - let path = dir.path().join(".env"); - let messages = test_messages(); - // Use a unique key to avoid collision with parallel tests. - let nonce = std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .expect("system time is before UNIX_EPOCH") - .as_nanos(); - let key = format!("DOTENV_TEST_{nonce}"); - std::fs::write(&path, format!("{key}=from_file\n")).unwrap(); - - // SAFETY: test-only, unique key avoids interference. - unsafe { - std::env::remove_var(&key); - } - load_dotenv_into_env(&path, &messages).unwrap(); - assert_eq!(std::env::var(&key).unwrap(), "from_file"); + /// Builds the parsed `.env` a load would be handed. + fn entries(pairs: &[(&str, &str)]) -> BTreeMap { + pairs + .iter() + .map(|(key, value)| ((*key).to_string(), (*value).to_string())) + .collect() + } - // Clean up. - unsafe { - std::env::remove_var(&key); - } + /// An environment holding exactly `set`. + fn holding(set: &'static [&'static str]) -> impl Fn(&str) -> bool { + move |key: &str| set.contains(&key) } #[test] - fn test_load_dotenv_into_env_does_not_overwrite_existing() { - let dir = tempdir().unwrap(); - let path = dir.path().join(".env"); - let messages = test_messages(); - let nonce = std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .expect("system time is before UNIX_EPOCH") - .as_nanos(); - let key = format!("DOTENV_EXIST_{nonce}"); - std::fs::write(&path, format!("{key}=from_file\n")).unwrap(); - - // SAFETY: test-only, unique key avoids interference. - unsafe { - std::env::set_var(&key, "already_set"); - } - load_dotenv_into_env(&path, &messages).unwrap(); - assert_eq!(std::env::var(&key).unwrap(), "already_set"); + fn load_dotenv_applies_keys_the_environment_does_not_hold() { + let map = entries(&[("DOTENV_A", "from_file"), ("DOTENV_B", "also_from_file")]); + assert_eq!( + dotenv_pairs_to_apply(&map, &holding(&[])), + vec![("DOTENV_A", "from_file"), ("DOTENV_B", "also_from_file")] + ); + } - // Clean up. - unsafe { - std::env::remove_var(&key); - } + #[test] + fn load_dotenv_does_not_overwrite_a_key_the_environment_already_holds() { + let map = entries(&[("DOTENV_A", "from_file"), ("DOTENV_B", "also_from_file")]); + assert_eq!( + dotenv_pairs_to_apply(&map, &holding(&["DOTENV_A"])), + vec![("DOTENV_B", "also_from_file")], + "process env > .env file, so an already-set key is left alone" + ); } /// `init` resolves the compose project once before this load and @@ -256,75 +251,41 @@ mod tests { /// `.env` key is not that. #[test] fn load_dotenv_into_env_never_promotes_compose_project_name() { - use crate::commands::compose_project::{ - COMPOSE_PROJECT_NAME_ENV, resolve_compose_project_for_dir, - test_env::{ComposeProjectEnv, env_lock}, - }; - - let _guard = env_lock(); - let _env = ComposeProjectEnv::set(None); - let dir = tempdir().unwrap(); - let path = dir.path().join(".env"); - let messages = test_messages(); - std::fs::write( - &path, - format!("{COMPOSE_PROJECT_NAME_ENV}=from-dotenv\nBOOTROOT_INSTANCE=insight\n"), - ) - .unwrap(); - - let before = resolve_compose_project_for_dir(dir.path(), None, &messages).unwrap(); - load_dotenv_into_env(&path, &messages).unwrap(); - let after = resolve_compose_project_for_dir(dir.path(), None, &messages).unwrap(); - + let map = entries(&[ + (COMPOSE_PROJECT_NAME_ENV, "from-dotenv"), + ("BOOTROOT_INSTANCE", "insight"), + ]); + let applied = dotenv_pairs_to_apply(&map, &holding(&[])); assert!( - std::env::var(COMPOSE_PROJECT_NAME_ENV).is_err(), - "a .env-authored {COMPOSE_PROJECT_NAME_ENV} must not reach the process environment" - ); - assert_eq!( - (before.as_str(), after.as_str()), - ("insight", "insight"), - "the resolved project must not change across the .env load" + !applied + .iter() + .any(|(key, _)| *key == COMPOSE_PROJECT_NAME_ENV), + "a .env-authored {COMPOSE_PROJECT_NAME_ENV} must never be applied: {applied:?}" ); } + /// The key is skipped because of what it is, not because something + /// else already holds it: an unset `COMPOSE_PROJECT_NAME` is exactly + /// the case where a promotion would change the resolved project. + #[test] + fn compose_project_name_is_excluded_even_when_the_environment_is_empty() { + let map = entries(&[(COMPOSE_PROJECT_NAME_ENV, "from-dotenv")]); + assert!(dotenv_pairs_to_apply(&map, &holding(&[])).is_empty()); + } + /// The exclusion is scoped to that one key: everything else `.env` /// carries still has to reach the process environment, which is the /// whole point of the load (`POSTGRES_PASSWORD` for the DSN builders). #[test] fn load_dotenv_into_env_still_loads_other_keys_alongside_it() { - use crate::commands::compose_project::{ - COMPOSE_PROJECT_NAME_ENV, - test_env::{ComposeProjectEnv, env_lock}, - }; - - let _guard = env_lock(); - let _env = ComposeProjectEnv::set(None); - let dir = tempdir().unwrap(); - let path = dir.path().join(".env"); - let messages = test_messages(); - let nonce = std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .expect("system time is before UNIX_EPOCH") - .as_nanos(); - let key = format!("DOTENV_ALONGSIDE_{nonce}"); - std::fs::write( - &path, - format!("{COMPOSE_PROJECT_NAME_ENV}=from-dotenv\n{key}=from_file\n"), - ) - .unwrap(); - - // SAFETY: test-only, unique key avoids interference. - unsafe { - std::env::remove_var(&key); - } - load_dotenv_into_env(&path, &messages).unwrap(); - assert_eq!(std::env::var(&key).unwrap(), "from_file"); - assert!(std::env::var(COMPOSE_PROJECT_NAME_ENV).is_err()); - - // SAFETY: as above. - unsafe { - std::env::remove_var(&key); - } + let map = entries(&[ + (COMPOSE_PROJECT_NAME_ENV, "from-dotenv"), + ("POSTGRES_PASSWORD", "from_file"), + ]); + assert_eq!( + dotenv_pairs_to_apply(&map, &holding(&[])), + vec![("POSTGRES_PASSWORD", "from_file")] + ); } #[test] diff --git a/src/commands/infra.rs b/src/commands/infra.rs index 24f56989..16a489c5 100644 --- a/src/commands/infra.rs +++ b/src/commands/infra.rs @@ -9,7 +9,8 @@ use anyhow::{Context, Result}; use bootroot::db::POSTGRES_HOST_PORT_ENV; use bootroot::host_port::{ HTTP01_ADMIN_HOST_PORT_ENV, OPENBAO_HOST_PORT_ENV, STEPCA_HOST_PORT_ENV, - resolve_http01_admin_host_port, resolve_openbao_host_port, resolve_stepca_host_port, + resolve_http01_admin_host_port_with_env, resolve_openbao_host_port_with_env, + resolve_stepca_host_port_with_env, }; use bootroot::openbao::OpenBaoClient; @@ -881,23 +882,56 @@ struct HostPorts { http01_admin: u16, } +/// The `*_HOST_PORT` variables [`HostPorts::resolve`] consults, read +/// from the invoking environment. +/// +/// Read once at the composition root and passed down, so the resolution +/// below is steered by a value rather than by process-global state. +#[derive(Debug, Clone, Default)] +struct HostPortEnv { + postgres: Option, + openbao: Option, + stepca: Option, + http01_admin: Option, +} + +impl HostPortEnv { + fn from_process_env() -> Self { + Self { + postgres: std::env::var(POSTGRES_HOST_PORT_ENV).ok(), + openbao: std::env::var(OPENBAO_HOST_PORT_ENV).ok(), + stepca: std::env::var(STEPCA_HOST_PORT_ENV).ok(), + http01_admin: std::env::var(HTTP01_ADMIN_HOST_PORT_ENV).ok(), + } + } +} + impl HostPorts { /// Resolves every core service's host-side published port for an /// `infra install` run. fn resolve(args: &InfraInstallArgs, compose_dir: &Path) -> Self { + Self::resolve_with_env(args, compose_dir, &HostPortEnv::from_process_env()) + } + + /// [`HostPorts::resolve`] with the `*_HOST_PORT` variables supplied + /// by the caller instead of read from the process environment. + fn resolve_with_env(args: &InfraInstallArgs, compose_dir: &Path, env: &HostPortEnv) -> Self { Self { - postgres: args - .postgres_host_port - .unwrap_or_else(|| bootroot::db::resolve_postgres_host_port(compose_dir)), - openbao: args - .openbao_host_port - .unwrap_or_else(|| resolve_openbao_host_port(compose_dir)), - stepca: args - .stepca_host_port - .unwrap_or_else(|| resolve_stepca_host_port(compose_dir)), - http01_admin: args - .http01_admin_host_port - .unwrap_or_else(|| resolve_http01_admin_host_port(compose_dir)), + postgres: args.postgres_host_port.unwrap_or_else(|| { + bootroot::db::resolve_postgres_host_port_with_env( + env.postgres.as_deref(), + compose_dir, + ) + }), + openbao: args.openbao_host_port.unwrap_or_else(|| { + resolve_openbao_host_port_with_env(env.openbao.as_deref(), compose_dir) + }), + stepca: args.stepca_host_port.unwrap_or_else(|| { + resolve_stepca_host_port_with_env(env.stepca.as_deref(), compose_dir) + }), + http01_admin: args.http01_admin_host_port.unwrap_or_else(|| { + resolve_http01_admin_host_port_with_env(env.http01_admin.as_deref(), compose_dir) + }), } } } @@ -2738,7 +2772,6 @@ mod tests { /// `.env` → compile-time default, per service. #[test] fn host_ports_resolve_prefers_the_flag_then_the_env_file() { - let _env = crate::commands::openbao_url::test_env::HostPortEnvGuard::new(); let dir = tempfile::tempdir().unwrap(); std::fs::write( dir.path().join(".env"), @@ -2747,8 +2780,16 @@ mod tests { .unwrap(); let mut args = install_args(dir.path().join("docker-compose.yml")); args.openbao_host_port = Some(28200); - let ports = HostPorts::resolve(&args, dir.path()); + let env = HostPortEnv { + postgres: Some("15432".to_string()), + ..HostPortEnv::default() + }; + let ports = HostPorts::resolve_with_env(&args, dir.path(), &env); assert_eq!(ports.openbao, 28200, "the flag wins"); + assert_eq!( + ports.postgres, 15432, + "the process environment outranks `.env`" + ); assert_eq!(ports.stepca, 19000, "`.env` is consulted"); assert_eq!( ports.http01_admin, diff --git a/src/commands/init/steps.rs b/src/commands/init/steps.rs index 5e19604e..bd8eb219 100644 --- a/src/commands/init/steps.rs +++ b/src/commands/init/steps.rs @@ -860,22 +860,12 @@ mod rollback_tests { #[cfg(test)] pub(super) mod test_support { use std::path::PathBuf; - use std::sync::{Mutex, MutexGuard, OnceLock}; use super::super::constants::openbao_constants::SECRET_ID_TTL; use super::super::constants::{DEFAULT_CERT_DURATION, DEFAULT_STEPCA_PROVISIONER}; use crate::cli::args::InitArgs; pub(in crate::commands::init::steps) use crate::i18n::test_messages; - static ENV_LOCK: OnceLock> = OnceLock::new(); - - pub(in crate::commands::init::steps) fn env_lock() -> MutexGuard<'static, ()> { - ENV_LOCK - .get_or_init(|| Mutex::new(())) - .lock() - .expect("env lock") - } - pub(in crate::commands::init::steps) fn default_init_args() -> InitArgs { InitArgs { openbao: crate::cli::args::OpenBaoArgs { diff --git a/src/commands/init/steps/database.rs b/src/commands/init/steps/database.rs index 811ef805..a1d689f3 100644 --- a/src/commands/init/steps/database.rs +++ b/src/commands/init/steps/database.rs @@ -1,4 +1,5 @@ use std::env; +use std::fmt; use std::path::Path; use std::time::Duration; @@ -6,7 +7,7 @@ use anyhow::{Context, Result}; use bootroot::db::{ DB_COMPOSE_HOST, DB_HOST_RUNTIME_HOST, DbDsn, build_db_dsn, check_auth_sync, check_tcp, effective_admin_dsn_for_kv, for_compose_runtime, parse_db_dsn, provision_db_sync, - resolve_postgres_host_port, validate_db_identifier, + resolve_postgres_host_port_with_env, validate_db_identifier, }; use super::super::constants::{DEFAULT_DB_NAME, DEFAULT_DB_USER, SECRET_BYTES}; @@ -16,9 +17,69 @@ use crate::cli::args::{InitArgs, InitFeature}; use crate::commands::guardrails::is_single_host_db_host; use crate::i18n::Messages; +const POSTGRES_USER_ENV: &str = "POSTGRES_USER"; +const POSTGRES_PASSWORD_ENV: &str = "POSTGRES_PASSWORD"; +const POSTGRES_DB_ENV: &str = "POSTGRES_DB"; +const POSTGRES_HOST_ENV: &str = "POSTGRES_HOST"; +const POSTGRES_PORT_ENV: &str = "POSTGRES_PORT"; +const POSTGRES_SSLMODE_ENV: &str = "POSTGRES_SSLMODE"; + +/// The `POSTGRES_*` variables the DSN builders below consult. +/// +/// Read once, at the point `init` has finished loading the compose +/// `.env` into the process environment, and passed down from there — +/// so every builder underneath is steered by a value a caller supplies +/// rather than by process-global state. +#[derive(Clone, Default)] +pub(super) struct PostgresEnv { + user: Option, + password: Option, + database: Option, + host: Option, + port: Option, + sslmode: Option, + host_port: Option, +} + +impl PostgresEnv { + /// Reads the `POSTGRES_*` variables from the process environment. + /// + /// `init` calls this after `load_dotenv_into_env` has promoted the + /// compose `.env`, which is where the `PostgreSQL` credentials + /// `infra install` generated live. + pub(super) fn from_process_env() -> Self { + Self { + user: env::var(POSTGRES_USER_ENV).ok(), + password: env::var(POSTGRES_PASSWORD_ENV).ok(), + database: env::var(POSTGRES_DB_ENV).ok(), + host: env::var(POSTGRES_HOST_ENV).ok(), + port: env::var(POSTGRES_PORT_ENV).ok(), + sslmode: env::var(POSTGRES_SSLMODE_ENV).ok(), + host_port: env::var(bootroot::db::POSTGRES_HOST_PORT_ENV).ok(), + } + } +} + +/// Hand-written so a `#[derive(Debug)]` on anything holding one cannot +/// print the `PostgreSQL` password. +impl fmt::Debug for PostgresEnv { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("PostgresEnv") + .field("user", &self.user) + .field("password", &self.password.as_ref().map(|_| "")) + .field("database", &self.database) + .field("host", &self.host) + .field("port", &self.port) + .field("sslmode", &self.sslmode) + .field("host_port", &self.host_port) + .finish() + } +} + pub(super) async fn resolve_db_dsn_for_init( args: &InitArgs, compose_dir: &Path, + postgres_env: &PostgresEnv, messages: &Messages, ) -> Result<(String, DbDsnNormalization, Option)> { // Under reinit mode the preserved `ca.json` runtime DSN is threaded @@ -42,7 +103,7 @@ pub(super) async fn resolve_db_dsn_for_init( anyhow::bail!(messages.error_db_provision_conflict()); } if args.has_feature(InitFeature::DbProvision) && !reinit_preserved_db_dsn { - let inputs = resolve_db_provision_inputs(args, compose_dir, messages)?; + let inputs = resolve_db_provision_inputs(args, compose_dir, postgres_env, messages)?; let admin = parse_db_dsn(&inputs.admin_dsn) .map_err(|_| anyhow::anyhow!(messages.error_invalid_db_dsn()))?; ensure_db_host_reachable_from_compose(&admin.host, messages)?; @@ -91,7 +152,7 @@ pub(super) async fn resolve_db_dsn_for_init( Some(admin_dsn_for_kv), )); } - let dsn = resolve_db_dsn(args, messages)?; + let dsn = resolve_db_dsn(args, postgres_env, messages)?; let parsed = parse_db_dsn(&dsn).map_err(|_| anyhow::anyhow!(messages.error_invalid_db_dsn()))?; ensure_db_host_reachable_from_compose(&parsed.host, messages)?; @@ -118,11 +179,12 @@ struct DbProvisionInputs { fn resolve_db_provision_inputs( args: &InitArgs, compose_dir: &Path, + postgres_env: &PostgresEnv, messages: &Messages, ) -> Result { let admin_dsn = if let Some(value) = &args.db_admin.admin_dsn { value.clone() - } else if let Some(value) = build_admin_dsn_from_env(compose_dir) { + } else if let Some(value) = build_admin_dsn_from_env(compose_dir, postgres_env) { value } else { prompt_text(&format!("{}: ", messages.prompt_db_admin_dsn()), messages)? @@ -130,7 +192,7 @@ fn resolve_db_provision_inputs( let default_db_name = args .db_name .clone() - .or_else(|| env::var("POSTGRES_DB").ok()) + .or_else(|| postgres_env.database.clone()) .unwrap_or_else(|| DEFAULT_DB_NAME.to_string()); let db_user = if let Some(value) = &args.db_user { value.clone() @@ -166,13 +228,9 @@ fn resolve_db_provision_inputs( }) } -fn build_admin_dsn_from_env(compose_dir: &Path) -> Option { - let Ok(user) = env::var("POSTGRES_USER") else { - return None; - }; - let Ok(password) = env::var("POSTGRES_PASSWORD") else { - return None; - }; +fn build_admin_dsn_from_env(compose_dir: &Path, postgres_env: &PostgresEnv) -> Option { + let user = postgres_env.user.as_deref()?; + let password = postgres_env.password.as_deref()?; // `provision_db_sync` connects to PostgreSQL from the host (it shells // out via the `postgres` crate, not from inside the compose network), // so the auto-derived admin DSN must be host-reachable. Default the @@ -181,47 +239,47 @@ fn build_admin_dsn_from_env(compose_dir: &Path) -> Option { // `${POSTGRES_HOST_PORT:-5433}` in `docker-compose.yml` — process env // → `compose_dir/.env` → 5433. `POSTGRES_HOST` / `POSTGRES_PORT` // remain explicit overrides for operator-supplied topologies. - let host = env::var("POSTGRES_HOST").unwrap_or_else(|_| DB_HOST_RUNTIME_HOST.to_string()); - let port = env::var("POSTGRES_PORT") - .ok() + let host = postgres_env.host.as_deref().unwrap_or(DB_HOST_RUNTIME_HOST); + let port = postgres_env + .port + .as_deref() .and_then(|value| value.parse::().ok()) - .unwrap_or_else(|| resolve_postgres_host_port(compose_dir)); + .unwrap_or_else(|| { + resolve_postgres_host_port_with_env(postgres_env.host_port.as_deref(), compose_dir) + }); // Always connect to the "postgres" database for admin operations. // POSTGRES_DB names the application database that will be created, // not the admin database used for provisioning. - let sslmode = env::var("POSTGRES_SSLMODE").ok(); Some(build_db_dsn( - &user, - &password, - &host, + user, + password, + host, port, "postgres", - sslmode.as_deref(), + postgres_env.sslmode.as_deref(), )) } -fn resolve_db_dsn(args: &InitArgs, messages: &Messages) -> Result { +fn resolve_db_dsn( + args: &InitArgs, + postgres_env: &PostgresEnv, + messages: &Messages, +) -> Result { if let Some(dsn) = &args.db_dsn { return Ok(dsn.clone()); } - if let Some(dsn) = build_dsn_from_env() { + if let Some(dsn) = build_dsn_from_env(postgres_env) { return Ok(dsn); } prompt_text(&format!("{}: ", messages.prompt_db_dsn()), messages) } -fn build_dsn_from_env() -> Option { - let Ok(user) = env::var("POSTGRES_USER") else { - return None; - }; - let Ok(password) = env::var("POSTGRES_PASSWORD") else { - return None; - }; - let Ok(db) = env::var("POSTGRES_DB") else { - return None; - }; - let host = env::var("POSTGRES_HOST").unwrap_or_else(|_| "postgres".to_string()); - let port = env::var("POSTGRES_PORT").unwrap_or_else(|_| "5432".to_string()); +fn build_dsn_from_env(postgres_env: &PostgresEnv) -> Option { + let user = postgres_env.user.as_deref()?; + let password = postgres_env.password.as_deref()?; + let db = postgres_env.database.as_deref()?; + let host = postgres_env.host.as_deref().unwrap_or(DB_COMPOSE_HOST); + let port = postgres_env.port.as_deref().unwrap_or("5432"); let dsn = format!("postgresql://{user}:{password}@{host}:{port}/{db}?sslmode=disable"); Some(dsn) } @@ -253,34 +311,57 @@ fn ensure_db_host_reachable_from_compose(host: &str, messages: &Messages) -> Res #[cfg(test)] mod tests { - use std::env; + use bootroot::db::DEFAULT_POSTGRES_HOST_PORT; - use super::super::test_support::{default_init_args, env_lock, test_messages}; + use super::super::test_support::{default_init_args, test_messages}; use super::*; + /// The `POSTGRES_*` set an `infra install`-provisioned `.env` + /// leaves behind, which is what the builders below see in practice. + fn postgres_env(pairs: &[(&str, &str)]) -> PostgresEnv { + let mut env = PostgresEnv::default(); + for (key, value) in pairs { + let slot = match *key { + POSTGRES_USER_ENV => &mut env.user, + POSTGRES_PASSWORD_ENV => &mut env.password, + POSTGRES_DB_ENV => &mut env.database, + POSTGRES_HOST_ENV => &mut env.host, + POSTGRES_PORT_ENV => &mut env.port, + POSTGRES_SSLMODE_ENV => &mut env.sslmode, + bootroot::db::POSTGRES_HOST_PORT_ENV => &mut env.host_port, + other => panic!("{other} is not a POSTGRES_* variable these builders read"), + }; + *slot = Some((*value).to_string()); + } + env + } + + /// A nonce-based fixture password. `CodeQL` flags a literal as a + /// hard-coded credential; a per-run value has no relation to a real + /// one. + fn nonce_password(prefix: &str) -> String { + let nonce = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .expect("system time is before UNIX_EPOCH") + .as_nanos(); + format!("{prefix}-{nonce}") + } + #[test] fn test_resolve_db_dsn_prefers_cli() { - let _guard = env_lock(); - // SAFETY: tests run single-threaded for this scope; vars are restored below. - unsafe { - env::set_var("POSTGRES_USER", "envuser"); - env::set_var("POSTGRES_PASSWORD", "envpass"); - env::set_var("POSTGRES_DB", "envdb"); - } + let env = postgres_env(&[ + (POSTGRES_USER_ENV, "envuser"), + (POSTGRES_PASSWORD_ENV, "envpass"), + (POSTGRES_DB_ENV, "envdb"), + ]); let mut args = default_init_args(); args.db_dsn = Some("postgresql://cliuser:clipass@localhost/db".to_string()); - let dsn = resolve_db_dsn(&args, &test_messages()).unwrap(); - unsafe { - env::remove_var("POSTGRES_USER"); - env::remove_var("POSTGRES_PASSWORD"); - env::remove_var("POSTGRES_DB"); - } + let dsn = resolve_db_dsn(&args, &env, &test_messages()).unwrap(); assert_eq!(dsn, "postgresql://cliuser:clipass@localhost/db"); } #[test] fn test_resolve_db_dsn_for_init_rejects_remote_host() { - let _guard = env_lock(); let mut args = default_init_args(); args.db_dsn = Some("postgresql://user:pass@db.internal:5432/stepca?sslmode=disable".to_string()); @@ -290,6 +371,7 @@ mod tests { .block_on(resolve_db_dsn_for_init( &args, Path::new("."), + &PostgresEnv::default(), &test_messages(), )) .expect_err("remote db host should fail single-host guardrail"); @@ -301,7 +383,6 @@ mod tests { #[test] fn test_resolve_db_dsn_for_init_normalizes_localhost_to_postgres() { - let _guard = env_lock(); let mut args = default_init_args(); args.db_dsn = Some("postgresql://user:pass@localhost:5432/stepca".to_string()); @@ -310,6 +391,7 @@ mod tests { .block_on(resolve_db_dsn_for_init( &args, Path::new("."), + &PostgresEnv::default(), &test_messages(), )) .expect("dsn should resolve"); @@ -323,7 +405,6 @@ mod tests { #[test] fn test_resolve_db_dsn_for_init_keeps_postgres_host() { - let _guard = env_lock(); let mut args = default_init_args(); args.db_dsn = Some("postgresql://user:pass@postgres:5432/stepca".to_string()); @@ -332,6 +413,7 @@ mod tests { .block_on(resolve_db_dsn_for_init( &args, Path::new("."), + &PostgresEnv::default(), &test_messages(), )) .expect("dsn should resolve"); @@ -349,7 +431,6 @@ mod tests { // (POSTGRES_HOST_PORT territory) must not leak into the stored // compose-internal DSN. Both host and port flip to the compose // pair. - let _guard = env_lock(); let mut args = default_init_args(); args.db_dsn = Some("postgresql://user:pass@127.0.0.1:5433/stepca".to_string()); @@ -358,6 +439,7 @@ mod tests { .block_on(resolve_db_dsn_for_init( &args, Path::new("."), + &PostgresEnv::default(), &test_messages(), )) .expect("dsn should resolve"); @@ -378,41 +460,44 @@ mod tests { #[test] fn test_resolve_db_dsn_uses_env() { - let _guard = env_lock(); - // SAFETY: tests run single-threaded for this scope; vars are restored below. - // CodeQL flags "secret" as a hard-coded credential, but this is a test-only - // fixture value with no relation to any real credential. Dismiss as false positive. - unsafe { - env::set_var("POSTGRES_USER", "step"); - env::set_var("POSTGRES_PASSWORD", "secret"); - env::set_var("POSTGRES_DB", "stepca"); - env::set_var("POSTGRES_HOST", "postgres"); - env::set_var("POSTGRES_PORT", "5432"); - } + // CodeQL flags "secret" as a hard-coded credential, but this is a + // test-only fixture value with no relation to any real credential. + // Dismiss as false positive. + let env = postgres_env(&[ + (POSTGRES_USER_ENV, "step"), + (POSTGRES_PASSWORD_ENV, "secret"), + (POSTGRES_DB_ENV, "stepca"), + (POSTGRES_HOST_ENV, "postgres"), + (POSTGRES_PORT_ENV, "5432"), + ]); let args = default_init_args(); - let dsn = resolve_db_dsn(&args, &test_messages()).unwrap(); - unsafe { - env::remove_var("POSTGRES_USER"); - env::remove_var("POSTGRES_PASSWORD"); - env::remove_var("POSTGRES_DB"); - env::remove_var("POSTGRES_HOST"); - env::remove_var("POSTGRES_PORT"); - } + let dsn = resolve_db_dsn(&args, &env, &test_messages()).unwrap(); assert_eq!( dsn, "postgresql://step:secret@postgres:5432/stepca?sslmode=disable" ); } + /// Without a user, a password and a database name there is nothing + /// to build a DSN from, and the resolver must fall through to the + /// prompt rather than synthesising one out of the defaults. + #[test] + fn build_dsn_from_env_needs_the_three_required_variables() { + assert!(build_dsn_from_env(&PostgresEnv::default()).is_none()); + assert!( + build_dsn_from_env(&postgres_env(&[ + (POSTGRES_USER_ENV, "step"), + (POSTGRES_PASSWORD_ENV, "secret"), + ])) + .is_none(), + "a missing POSTGRES_DB must not fall back to a default database" + ); + } + #[test] fn test_resolve_db_provision_inputs_with_args() { - let _guard = env_lock(); - let nonce = std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .expect("system time is before UNIX_EPOCH") - .as_nanos(); - let admin_password = format!("admin-{nonce}"); - let db_password = format!("step-{nonce}"); + let admin_password = nonce_password("admin"); + let db_password = nonce_password("step"); let mut args = default_init_args(); args.enable.push(InitFeature::DbProvision); args.db_admin.admin_dsn = Some(format!( @@ -422,7 +507,13 @@ mod tests { args.db_password = Some(db_password.clone()); args.db_name = Some("stepdb".to_string()); - let inputs = resolve_db_provision_inputs(&args, Path::new("."), &test_messages()).unwrap(); + let inputs = resolve_db_provision_inputs( + &args, + Path::new("."), + &PostgresEnv::default(), + &test_messages(), + ) + .unwrap(); assert_eq!( inputs.admin_dsn, format!("postgresql://admin:{admin_password}@localhost:5432/postgres?sslmode=disable") @@ -434,13 +525,8 @@ mod tests { #[test] fn test_resolve_db_provision_inputs_rejects_invalid_identifier() { - let _guard = env_lock(); - let nonce = std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .expect("system time is before UNIX_EPOCH") - .as_nanos(); - let admin_password = format!("admin-{nonce}"); - let db_password = format!("step-{nonce}"); + let admin_password = nonce_password("admin"); + let db_password = nonce_password("step"); let mut args = default_init_args(); args.enable.push(InitFeature::DbProvision); args.db_admin.admin_dsn = Some(format!( @@ -450,7 +536,13 @@ mod tests { args.db_password = Some(db_password); args.db_name = Some("stepdb".to_string()); - let err = resolve_db_provision_inputs(&args, Path::new("."), &test_messages()).unwrap_err(); + let err = resolve_db_provision_inputs( + &args, + Path::new("."), + &PostgresEnv::default(), + &test_messages(), + ) + .unwrap_err(); assert!(err.to_string().contains("Invalid DB identifier")); } @@ -461,16 +553,8 @@ mod tests { // admin role. After provision, the role's password is // `db_password`, not the value embedded in the original admin // DSN — the persisted DSN must reflect that. - // - // Nonce-based test fixtures sidestep CodeQL's - // `rust/hard-coded-cryptographic-value` rule (the values are - // generated per run and have no relation to a real credential). - let nonce = std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .expect("system time is before UNIX_EPOCH") - .as_nanos(); - let old = format!("old-{nonce}"); - let new = format!("new-{nonce}"); + let old = nonce_password("old"); + let new = nonce_password("new"); let admin_dsn = format!("postgresql://step:{old}@127.0.0.1:5433/postgres?sslmode=disable"); let resolved = effective_admin_dsn_for_kv(&admin_dsn, "step", &new).unwrap(); assert_eq!( @@ -483,13 +567,8 @@ mod tests { fn test_effective_admin_dsn_for_kv_unchanged_when_distinct_role() { // When admin and runtime are distinct roles, the admin DSN is // untouched by provisioning and should be persisted verbatim. - // Nonce-based fixtures sidestep CodeQL — see the sibling test. - let nonce = std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .expect("system time is before UNIX_EPOCH") - .as_nanos(); - let admin_pw = format!("admin-{nonce}"); - let runtime_pw = format!("runtime-{nonce}"); + let admin_pw = nonce_password("admin"); + let runtime_pw = nonce_password("runtime"); let admin_dsn = format!("postgresql://admin:{admin_pw}@127.0.0.1:5433/postgres?sslmode=disable"); let resolved = effective_admin_dsn_for_kv(&admin_dsn, "stepca", &runtime_pw).unwrap(); @@ -498,13 +577,8 @@ mod tests { #[test] fn test_resolve_db_dsn_for_init_rejects_conflict() { - let _guard = env_lock(); - let nonce = std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .expect("system time is before UNIX_EPOCH") - .as_nanos(); - let admin_password = format!("admin-{nonce}"); - let db_password = format!("step-{nonce}"); + let admin_password = nonce_password("admin"); + let db_password = nonce_password("step"); let mut args = default_init_args(); args.db_dsn = Some("postgresql://user:pass@localhost/db".to_string()); args.enable.push(InitFeature::DbProvision); @@ -520,6 +594,7 @@ mod tests { .block_on(resolve_db_dsn_for_init( &args, Path::new("."), + &PostgresEnv::default(), &test_messages(), )) .unwrap_err(); @@ -533,12 +608,7 @@ mod tests { /// the already-good `PostgreSQL` role's password is not rotated. #[test] fn test_resolve_db_dsn_for_init_accepts_preserved_dsn_in_reinit_mode() { - let _guard = env_lock(); - let nonce = std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .expect("system time is before UNIX_EPOCH") - .as_nanos(); - let preserved_password = format!("preserved-{nonce}"); + let preserved_password = nonce_password("preserved"); let mut args = default_init_args(); args.reinit_mode = true; args.db_dsn = Some(format!( @@ -561,6 +631,7 @@ mod tests { .block_on(resolve_db_dsn_for_init( &args, Path::new("."), + &PostgresEnv::default(), &test_messages(), )) .expect("preserved DSN must be accepted under reinit_mode"); @@ -585,31 +656,20 @@ mod tests { // `127.0.0.1:5433` (the new published default) rather than the // compose-internal `postgres:5432` — `provision_db_sync` runs // from the host. - let _guard = env_lock(); - let nonce = std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .expect("system time is before UNIX_EPOCH") - .as_nanos(); - let password = format!("admin-{nonce}"); + let password = nonce_password("admin"); let dir = tempfile::tempdir().expect("tempdir"); - // SAFETY: env_lock() serialises env-var-touching tests. - unsafe { - env::set_var("POSTGRES_USER", "step"); - env::set_var("POSTGRES_PASSWORD", &password); - env::remove_var("POSTGRES_HOST"); - env::remove_var("POSTGRES_PORT"); - env::remove_var("POSTGRES_HOST_PORT"); - env::remove_var("POSTGRES_SSLMODE"); - } - let dsn = build_admin_dsn_from_env(dir.path()).expect("dsn"); - unsafe { - env::remove_var("POSTGRES_USER"); - env::remove_var("POSTGRES_PASSWORD"); - } + let env = postgres_env(&[ + (POSTGRES_USER_ENV, "step"), + (POSTGRES_PASSWORD_ENV, &password), + ]); + let dsn = build_admin_dsn_from_env(dir.path(), &env).expect("dsn"); + let host = DB_HOST_RUNTIME_HOST; + let port = DEFAULT_POSTGRES_HOST_PORT; assert_eq!( dsn, - format!("postgresql://step:{password}@127.0.0.1:5433/postgres?sslmode=disable") + format!("postgresql://step:{password}@{host}:{port}/postgres?sslmode=disable") ); + assert_eq!((host, port), ("127.0.0.1", 5433)); } #[test] @@ -619,69 +679,76 @@ mod tests { // auto-derived admin DSN must honor that (Docker Compose's // `${POSTGRES_HOST_PORT:-5433}` precedence: process env → // compose_dir/.env → 5433). - let _guard = env_lock(); - let nonce = std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .expect("system time is before UNIX_EPOCH") - .as_nanos(); - let password = format!("admin-{nonce}"); + let password = nonce_password("admin"); let dir = tempfile::tempdir().expect("tempdir"); std::fs::write( dir.path().join(".env"), "POSTGRES_USER=step\nPOSTGRES_HOST_PORT=6543\n", ) .expect("write .env"); - // SAFETY: env_lock() serialises env-var-touching tests. - unsafe { - env::set_var("POSTGRES_USER", "step"); - env::set_var("POSTGRES_PASSWORD", &password); - env::remove_var("POSTGRES_HOST"); - env::remove_var("POSTGRES_PORT"); - env::remove_var("POSTGRES_HOST_PORT"); - env::remove_var("POSTGRES_SSLMODE"); - } - let dsn = build_admin_dsn_from_env(dir.path()).expect("dsn"); - unsafe { - env::remove_var("POSTGRES_USER"); - env::remove_var("POSTGRES_PASSWORD"); - } + let env = postgres_env(&[ + (POSTGRES_USER_ENV, "step"), + (POSTGRES_PASSWORD_ENV, &password), + ]); + let dsn = build_admin_dsn_from_env(dir.path(), &env).expect("dsn"); assert_eq!( dsn, format!("postgresql://step:{password}@127.0.0.1:6543/postgres?sslmode=disable") ); } + /// The process environment still outranks the compose `.env`, which + /// is the first step of the `${POSTGRES_HOST_PORT:-5433}` + /// precedence. + #[test] + fn build_admin_dsn_from_env_prefers_the_host_port_variable_over_dotenv() { + let password = nonce_password("admin"); + let dir = tempfile::tempdir().expect("tempdir"); + std::fs::write(dir.path().join(".env"), "POSTGRES_HOST_PORT=6543\n").expect("write .env"); + let env = postgres_env(&[ + (POSTGRES_USER_ENV, "step"), + (POSTGRES_PASSWORD_ENV, &password), + (bootroot::db::POSTGRES_HOST_PORT_ENV, "6544"), + ]); + let dsn = build_admin_dsn_from_env(dir.path(), &env).expect("dsn"); + assert_eq!( + dsn, + format!("postgresql://step:{password}@127.0.0.1:6544/postgres?sslmode=disable") + ); + } + #[test] fn build_admin_dsn_from_env_postgres_port_overrides_host_port() { // Explicit POSTGRES_PORT (operator-supplied topology) wins over // the resolved POSTGRES_HOST_PORT default — the env var is the // historical operator override and stays authoritative. - let _guard = env_lock(); - let nonce = std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .expect("system time is before UNIX_EPOCH") - .as_nanos(); - let password = format!("admin-{nonce}"); + let password = nonce_password("admin"); let dir = tempfile::tempdir().expect("tempdir"); std::fs::write(dir.path().join(".env"), "POSTGRES_HOST_PORT=6543\n").expect("write .env"); - // SAFETY: env_lock() serialises env-var-touching tests. - unsafe { - env::set_var("POSTGRES_USER", "step"); - env::set_var("POSTGRES_PASSWORD", &password); - env::set_var("POSTGRES_PORT", "7777"); - env::remove_var("POSTGRES_HOST"); - env::remove_var("POSTGRES_HOST_PORT"); - env::remove_var("POSTGRES_SSLMODE"); - } - let dsn = build_admin_dsn_from_env(dir.path()).expect("dsn"); - unsafe { - env::remove_var("POSTGRES_USER"); - env::remove_var("POSTGRES_PASSWORD"); - env::remove_var("POSTGRES_PORT"); - } + let env = postgres_env(&[ + (POSTGRES_USER_ENV, "step"), + (POSTGRES_PASSWORD_ENV, &password), + (POSTGRES_PORT_ENV, "7777"), + ]); + let dsn = build_admin_dsn_from_env(dir.path(), &env).expect("dsn"); assert_eq!( dsn, format!("postgresql://step:{password}@127.0.0.1:7777/postgres?sslmode=disable") ); } + + /// The password never reaches a `Debug` rendering, so a struct that + /// derives `Debug` around one cannot leak the credential + /// `infra install` generated. + #[test] + fn postgres_env_debug_redacts_the_password() { + let password = nonce_password("admin"); + let env = postgres_env(&[ + (POSTGRES_USER_ENV, "step"), + (POSTGRES_PASSWORD_ENV, &password), + ]); + let rendered = format!("{env:?}"); + assert!(!rendered.contains(&password), "{rendered}"); + assert!(rendered.contains(""), "{rendered}"); + } } diff --git a/src/commands/init/steps/orchestrator.rs b/src/commands/init/steps/orchestrator.rs index a1df25ec..2655a9be 100644 --- a/src/commands/init/steps/orchestrator.rs +++ b/src/commands/init/steps/orchestrator.rs @@ -14,7 +14,7 @@ use super::super::types::{ }; use super::InitRollback; use super::RollbackFile; -use super::database::{check_db_connectivity, resolve_db_dsn_for_init}; +use super::database::{PostgresEnv, check_db_connectivity, resolve_db_dsn_for_init}; use super::http01_admin_tls::{ build_http01_admin_tls_sans, issue_http01_admin_tls_cert, record_http01_admin_infra_cert, }; @@ -60,7 +60,7 @@ use crate::commands::init::{ OPENBAO_TLS_CERT_PATH, OPENBAO_TLS_KEY_PATH, RESPONDER_CONFIG_DIR, RESPONDER_CONFIG_NAME, }; use crate::commands::openbao_unseal::unseal_keys_path; -use crate::commands::openbao_url::effective_openbao_url; +use crate::commands::openbao_url::{OPENBAO_HOST_PORT_ENV, effective_openbao_url_with_env}; use crate::i18n::Messages; use crate::state::StateFile; @@ -73,8 +73,26 @@ use crate::state::StateFile; /// and `run_init_inner`'s own bind-intent-gated branch takes over once /// the TLS certificate has been issued. fn args_with_effective_openbao_url(args: &InitArgs) -> Cow<'_, InitArgs> { + args_with_effective_openbao_url_with_env( + args, + std::env::var(OPENBAO_HOST_PORT_ENV).ok().as_deref(), + ) +} + +/// [`args_with_effective_openbao_url`] with the `OPENBAO_HOST_PORT` +/// value supplied by the caller instead of read from the process +/// environment. +fn args_with_effective_openbao_url_with_env<'a>( + args: &'a InitArgs, + host_port_env: Option<&str>, +) -> Cow<'a, InitArgs> { let compose_dir = compose_file_dir(&args.compose.compose_file); - let url = effective_openbao_url(&args.openbao.openbao_url, &compose_dir, None); + let url = effective_openbao_url_with_env( + &args.openbao.openbao_url, + &compose_dir, + None, + host_port_env, + ); if url == args.openbao.openbao_url { return Cow::Borrowed(args); } @@ -454,8 +472,12 @@ async fn run_init_inner( // the temporary POSTGRES_PASSWORD written by `infra install`. crate::commands::dotenv::load_dotenv_into_env(&compose_dir.join(".env"), messages)?; + // Read after the `.env` load above, which is what puts the + // `PostgreSQL` credentials `infra install` generated into the + // process environment. + let postgres_env = PostgresEnv::from_process_env(); let (db_dsn, db_dsn_normalization, admin_dsn_for_kv) = - resolve_db_dsn_for_init(args, compose_dir, messages).await?; + resolve_db_dsn_for_init(args, compose_dir, &postgres_env, messages).await?; let mut secrets = resolve_init_secrets(args, messages, db_dsn)?; let db_info = parse_db_dsn(&secrets.db_dsn) .map_err(|_| anyhow::anyhow!(messages.error_invalid_db_dsn()))?; @@ -1690,26 +1712,29 @@ mod tests { /// the same host would initialise against the first one's `OpenBao`. #[test] fn init_args_follow_the_configured_openbao_host_port() { - let _env = crate::commands::openbao_url::test_env::HostPortEnvGuard::new(); let dir = tempfile::tempdir().expect("tempdir"); std::fs::write(dir.path().join(".env"), "OPENBAO_HOST_PORT=18200\n").expect("write .env"); let mut args = default_init_args(); args.compose.compose_file = dir.path().join("docker-compose.yml"); - let resolved = args_with_effective_openbao_url(&args); + let resolved = args_with_effective_openbao_url_with_env(&args, None); assert_eq!(resolved.openbao.openbao_url, "http://localhost:18200"); + let from_env = args_with_effective_openbao_url_with_env(&args, Some("18201")); + assert_eq!( + from_env.openbao.openbao_url, "http://localhost:18201", + "the process environment outranks the compose `.env`" + ); } /// Closes #731: an operator-supplied `--openbao-url` is used /// verbatim, and the unchanged case borrows instead of cloning. #[test] fn init_args_keep_an_explicit_openbao_url() { - let _env = crate::commands::openbao_url::test_env::HostPortEnvGuard::new(); let dir = tempfile::tempdir().expect("tempdir"); std::fs::write(dir.path().join(".env"), "OPENBAO_HOST_PORT=18200\n").expect("write .env"); let mut args = default_init_args(); args.compose.compose_file = dir.path().join("docker-compose.yml"); args.openbao.openbao_url = "https://openbao.internal:8200".to_string(); - let resolved = args_with_effective_openbao_url(&args); + let resolved = args_with_effective_openbao_url_with_env(&args, None); assert_eq!( resolved.openbao.openbao_url, "https://openbao.internal:8200" diff --git a/src/commands/openbao_auth.rs b/src/commands/openbao_auth.rs index 371de32e..8e721231 100644 --- a/src/commands/openbao_auth.rs +++ b/src/commands/openbao_auth.rs @@ -29,10 +29,20 @@ pub(crate) fn resolve_runtime_auth( args, allow_root_prompt, std::io::stdin().is_terminal(), + root_token_from_env().as_deref(), messages, ) } +/// Reads `OPENBAO_ROOT_TOKEN` from the invoking environment. +/// +/// The module's only read of it: [`resolve_root_token`] takes the value +/// as a parameter so the precedence around it can be exercised without +/// a process-global environment. +fn root_token_from_env() -> Option { + std::env::var(OPENBAO_ROOT_TOKEN_ENV).ok() +} + /// Resolves runtime auth, deciding whether the interactive root-token prompt is /// reachable from `stdin_is_tty`. Splitting the TTY check out lets tests assert /// the non-interactive error paths without a real terminal and without hitting @@ -41,9 +51,10 @@ fn resolve_runtime_auth_inner( args: &RuntimeAuthArgs, allow_root_prompt: bool, stdin_is_tty: bool, + env_root_token: Option<&str>, messages: &Messages, ) -> Result { - let root_token = resolve_root_token(args)?; + let root_token = resolve_root_token(args, env_root_token)?; let approle_role_id = resolve_from_value_or_file( args.approle_role_id.as_deref(), args.approle_role_id_file.as_deref(), @@ -90,7 +101,7 @@ fn resolve_runtime_auth_inner( pub(crate) fn resolve_runtime_auth_optional( args: &RuntimeAuthArgs, ) -> Result> { - let root_token = resolve_root_token(args)?; + let root_token = resolve_root_token(args, root_token_from_env().as_deref())?; let approle_role_id = resolve_from_value_or_file( args.approle_role_id.as_deref(), args.approle_role_id_file.as_deref(), @@ -124,14 +135,19 @@ pub(crate) fn resolve_runtime_auth_optional( Ok(auth) } -/// Resolves the root token from `--root-token-file`, `--root-token`, or the -/// `OPENBAO_ROOT_TOKEN` env var. Splitting the env var off the CLI flag lets -/// us distinguish an explicit `--root-token` from an env-injected value when -/// detecting conflicts with `--root-token-file`. The clap-level +/// Resolves the root token from `--root-token-file`, `--root-token`, or +/// `env_root_token` — what the invoking environment's +/// `OPENBAO_ROOT_TOKEN` held, per [`root_token_from_env`]. Splitting +/// the env var off the CLI flag lets us distinguish an explicit +/// `--root-token` from an env-injected value when detecting conflicts +/// with `--root-token-file`. The clap-level /// `conflicts_with = "root_token"` already rejects the explicit-flag combo at /// parse time; we still re-check here for callers that build /// `RuntimeAuthArgs` directly (e.g. tests). -fn resolve_root_token(args: &RuntimeAuthArgs) -> Result> { +fn resolve_root_token( + args: &RuntimeAuthArgs, + env_root_token: Option<&str>, +) -> Result> { if let Some(path) = args.root_token_file.as_deref() { if args.root_token.is_some() { anyhow::bail!("--root-token-file conflicts with --root-token; pass only one of them"); @@ -141,8 +157,8 @@ fn resolve_root_token(args: &RuntimeAuthArgs) -> Result> { if let Some(value) = &args.root_token { return Ok(Some(value.clone())); } - match std::env::var(OPENBAO_ROOT_TOKEN_ENV) { - Ok(value) if !value.is_empty() => Ok(Some(value)), + match env_root_token { + Some(value) if !value.is_empty() => Ok(Some(value.to_string())), _ => Ok(None), } } @@ -231,49 +247,11 @@ fn resolve_from_value_or_file( #[cfg(test)] mod tests { use std::path::PathBuf; - use std::sync::{LazyLock, Mutex, MutexGuard}; use tempfile::tempdir; use super::*; - static ENV_LOCK: LazyLock> = LazyLock::new(|| Mutex::new(())); - - fn env_lock() -> MutexGuard<'static, ()> { - ENV_LOCK.lock().expect("env lock not poisoned") - } - - struct ScopedRootTokenEnv { - previous: Option, - } - - impl ScopedRootTokenEnv { - fn set(value: Option<&str>) -> Self { - let previous = std::env::var(OPENBAO_ROOT_TOKEN_ENV).ok(); - // SAFETY: tests serialise via ENV_LOCK - unsafe { - match value { - Some(v) => std::env::set_var(OPENBAO_ROOT_TOKEN_ENV, v), - None => std::env::remove_var(OPENBAO_ROOT_TOKEN_ENV), - } - } - Self { previous } - } - } - - impl Drop for ScopedRootTokenEnv { - fn drop(&mut self) { - // SAFETY: tests serialise via ENV_LOCK - unsafe { - if let Some(prev) = &self.previous { - std::env::set_var(OPENBAO_ROOT_TOKEN_ENV, prev); - } else { - std::env::remove_var(OPENBAO_ROOT_TOKEN_ENV); - } - } - } - } - fn args_with(root_token: Option<&str>, root_token_file: Option<&Path>) -> RuntimeAuthArgs { RuntimeAuthArgs { auth_mode: AuthMode::Auto, @@ -299,86 +277,80 @@ mod tests { #[test] fn root_token_file_overrides_env_when_no_cli_flag() { - let _lock = env_lock(); - let _env = ScopedRootTokenEnv::set(Some("env-token")); let dir = tempdir().expect("tempdir"); let path = write_token_file(dir.path(), "root.token", "file-token\n", 0o600); let args = args_with(None, Some(&path)); - let resolved = resolve_root_token(&args).expect("resolve ok"); + let resolved = resolve_root_token(&args, Some("env-token")).expect("resolve ok"); assert_eq!(resolved.as_deref(), Some("file-token")); } #[test] fn root_token_file_conflicts_with_explicit_root_token_flag() { - let _lock = env_lock(); - let _env = ScopedRootTokenEnv::set(None); let dir = tempdir().expect("tempdir"); let path = write_token_file(dir.path(), "root.token", "file-token\n", 0o600); let args = args_with(Some("explicit"), Some(&path)); - let err = resolve_root_token(&args).expect_err("should conflict"); + let err = resolve_root_token(&args, None).expect_err("should conflict"); assert!(err.to_string().contains("--root-token-file conflicts")); } #[test] fn explicit_root_token_flag_beats_env() { - let _lock = env_lock(); - let _env = ScopedRootTokenEnv::set(Some("env-token")); let args = args_with(Some("explicit"), None); - let resolved = resolve_root_token(&args).expect("resolve ok"); + let resolved = resolve_root_token(&args, Some("env-token")).expect("resolve ok"); assert_eq!(resolved.as_deref(), Some("explicit")); } #[test] fn env_used_when_no_flag_or_file() { - let _lock = env_lock(); - let _env = ScopedRootTokenEnv::set(Some("env-token")); let args = args_with(None, None); - let resolved = resolve_root_token(&args).expect("resolve ok"); + let resolved = resolve_root_token(&args, Some("env-token")).expect("resolve ok"); assert_eq!(resolved.as_deref(), Some("env-token")); } #[test] fn no_token_when_nothing_provided() { - let _lock = env_lock(); - let _env = ScopedRootTokenEnv::set(None); let args = args_with(None, None); - let resolved = resolve_root_token(&args).expect("resolve ok"); + let resolved = resolve_root_token(&args, None).expect("resolve ok"); + assert!(resolved.is_none()); + } + + /// `OPENBAO_ROOT_TOKEN=` is how a shell clears the variable for one + /// command; an empty value must not become a token that then fails + /// authentication with an opaque 403. + #[test] + fn empty_env_token_is_treated_as_unset() { + let args = args_with(None, None); + let resolved = resolve_root_token(&args, Some("")).expect("resolve ok"); assert!(resolved.is_none()); } #[cfg(unix)] #[test] fn root_token_file_mode_0o600_accepted() { - let _lock = env_lock(); - let _env = ScopedRootTokenEnv::set(None); let dir = tempdir().expect("tempdir"); let path = write_token_file(dir.path(), "t", "tok\n", 0o600); let args = args_with(None, Some(&path)); - let resolved = resolve_root_token(&args).expect("0o600 ok"); + let resolved = resolve_root_token(&args, None).expect("0o600 ok"); assert_eq!(resolved.as_deref(), Some("tok")); } #[cfg(unix)] #[test] fn root_token_file_mode_0o640_accepted_for_group_sharing() { - let _lock = env_lock(); - let _env = ScopedRootTokenEnv::set(None); let dir = tempdir().expect("tempdir"); let path = write_token_file(dir.path(), "t", "tok\n", 0o640); let args = args_with(None, Some(&path)); - let resolved = resolve_root_token(&args).expect("0o640 ok"); + let resolved = resolve_root_token(&args, None).expect("0o640 ok"); assert_eq!(resolved.as_deref(), Some("tok")); } #[cfg(unix)] #[test] fn root_token_file_mode_0o644_rejected() { - let _lock = env_lock(); - let _env = ScopedRootTokenEnv::set(None); let dir = tempdir().expect("tempdir"); let path = write_token_file(dir.path(), "t", "tok\n", 0o644); let args = args_with(None, Some(&path)); - let err = resolve_root_token(&args).expect_err("0o644 must fail"); + let err = resolve_root_token(&args, None).expect_err("0o644 must fail"); let msg = err.to_string(); assert!(msg.contains("world-readable"), "msg = {msg}"); assert!(msg.contains("chmod 0600"), "msg = {msg}"); @@ -386,11 +358,9 @@ mod tests { #[test] fn auto_mode_non_tty_bails_actionable_without_prompting() { - let _lock = env_lock(); - let _env = ScopedRootTokenEnv::set(None); let messages = Messages::new("en").expect("messages"); let args = args_with(None, None); - let err = resolve_runtime_auth_inner(&args, true, false, &messages) + let err = resolve_runtime_auth_inner(&args, true, false, None, &messages) .expect_err("non-tty must bail instead of prompting"); let msg = err.to_string(); assert!(msg.contains("--root-token"), "msg = {msg}"); @@ -399,12 +369,10 @@ mod tests { #[test] fn root_mode_non_tty_bails_actionable_without_prompting() { - let _lock = env_lock(); - let _env = ScopedRootTokenEnv::set(None); let messages = Messages::new("en").expect("messages"); let mut args = args_with(None, None); args.auth_mode = AuthMode::Root; - let err = resolve_runtime_auth_inner(&args, true, false, &messages) + let err = resolve_runtime_auth_inner(&args, true, false, None, &messages) .expect_err("non-tty must bail instead of prompting"); assert_eq!( err.to_string(), @@ -416,14 +384,27 @@ mod tests { ); } + /// The mirror of the two tests above: with the environment holding + /// a token there is nothing to prompt for, so the non-TTY path + /// resolves instead of bailing. + #[test] + fn non_tty_resolves_from_the_env_token() { + let messages = Messages::new("en").expect("messages"); + let args = args_with(None, None); + let resolved = resolve_runtime_auth_inner(&args, true, false, Some("env-token"), &messages) + .expect("an env token needs no prompt"); + assert!(matches!( + resolved, + RuntimeAuthResolved::RootToken(token) if token == "env-token" + )); + } + #[test] fn root_token_file_empty_rejected() { - let _lock = env_lock(); - let _env = ScopedRootTokenEnv::set(None); let dir = tempdir().expect("tempdir"); let path = write_token_file(dir.path(), "t", " \n", 0o600); let args = args_with(None, Some(&path)); - let err = resolve_root_token(&args).expect_err("empty must fail"); + let err = resolve_root_token(&args, None).expect_err("empty must fail"); assert!(err.to_string().contains("empty")); } } diff --git a/src/commands/openbao_url.rs b/src/commands/openbao_url.rs index b7da8a3a..0e7e057d 100644 --- a/src/commands/openbao_url.rs +++ b/src/commands/openbao_url.rs @@ -12,7 +12,8 @@ use std::path::Path; -use bootroot::host_port::resolve_openbao_host_port; +pub(crate) use bootroot::host_port::OPENBAO_HOST_PORT_ENV; +use bootroot::host_port::resolve_openbao_host_port_with_env; use crate::commands::init::DEFAULT_OPENBAO_URL; @@ -23,195 +24,114 @@ use crate::commands::init::DEFAULT_OPENBAO_URL; /// In that case the default's port is replaced by the resolved host port — /// `host_port` when the command has a flag that carries one, else the /// process environment, else `/.env`, else 8200 (see -/// [`resolve_openbao_host_port`]). +/// [`resolve_openbao_host_port_with_env`]). pub(crate) fn effective_openbao_url( cli_url: &str, compose_dir: &Path, host_port: Option, +) -> String { + effective_openbao_url_with_env( + cli_url, + compose_dir, + host_port, + std::env::var(OPENBAO_HOST_PORT_ENV).ok().as_deref(), + ) +} + +/// [`effective_openbao_url`] with the `OPENBAO_HOST_PORT` value supplied +/// by the caller instead of read from the process environment. +pub(crate) fn effective_openbao_url_with_env( + cli_url: &str, + compose_dir: &Path, + host_port: Option, + env_value: Option<&str>, ) -> String { if cli_url != DEFAULT_OPENBAO_URL { return cli_url.to_string(); } - let port = host_port.unwrap_or_else(|| resolve_openbao_host_port(compose_dir)); + let port = + host_port.unwrap_or_else(|| resolve_openbao_host_port_with_env(env_value, compose_dir)); let Some((prefix, _)) = DEFAULT_OPENBAO_URL.rsplit_once(':') else { return cli_url.to_string(); }; format!("{prefix}:{port}") } -/// Shared test guard for the host-port environment variables. -/// -/// The variables are process-global, so a test that *sets* one and a -/// test that merely *reads* one (through the resolvers, from a `.env` -/// file) interfere across modules unless both hold the same lock. Every -/// binary-crate test that touches `OPENBAO_HOST_PORT`, -/// `STEPCA_HOST_PORT` or `HTTP01_ADMIN_HOST_PORT` — in either direction -/// — takes this guard. -#[cfg(test)] -pub(crate) mod test_env { - use std::sync::{Mutex, MutexGuard, OnceLock, PoisonError}; - - use bootroot::host_port::{ - HTTP01_ADMIN_HOST_PORT_ENV, OPENBAO_HOST_PORT_ENV, STEPCA_HOST_PORT_ENV, - }; - - const GUARDED_KEYS: [&str; 3] = [ - OPENBAO_HOST_PORT_ENV, - STEPCA_HOST_PORT_ENV, - HTTP01_ADMIN_HOST_PORT_ENV, - ]; - - /// Clears the guarded variables for the lifetime of the guard and - /// restores their pre-test values on drop. - pub(crate) struct HostPortEnvGuard { - _lock: MutexGuard<'static, ()>, - previous: Vec<(&'static str, Option)>, - } - - impl HostPortEnvGuard { - pub(crate) fn new() -> Self { - static LOCK: OnceLock> = OnceLock::new(); - let lock = LOCK - .get_or_init(|| Mutex::new(())) - .lock() - .unwrap_or_else(PoisonError::into_inner); - let previous = GUARDED_KEYS - .iter() - .map(|key| (*key, std::env::var(key).ok())) - .collect(); - for key in GUARDED_KEYS { - // SAFETY: removal is serialized by the mutex held above. - unsafe { - std::env::remove_var(key); - } - } - Self { - _lock: lock, - previous, - } - } - - pub(crate) fn set(&self, key: &str, value: &str) { - assert!( - self.previous.iter().any(|(tracked, _)| *tracked == key), - "{key} is not tracked by the guard and would leak into other tests" - ); - // SAFETY: mutation is serialized by the mutex held by `_lock`. - unsafe { - std::env::set_var(key, value); - } - } - } - - impl Drop for HostPortEnvGuard { - fn drop(&mut self) { - for (key, previous) in &self.previous { - // SAFETY: restored inside the shared mutex held by `_lock`. - unsafe { - match previous { - Some(value) => std::env::set_var(key, value), - None => std::env::remove_var(key), - } - } - } - } - } -} - #[cfg(test)] mod tests { - use bootroot::host_port::OPENBAO_HOST_PORT_ENV; - - use super::test_env::HostPortEnvGuard; use super::*; const NON_DEFAULT_URL: &str = "https://openbao.internal:8200"; - struct EnvGuard(HostPortEnvGuard); - - impl EnvGuard { - fn new() -> Self { - Self(HostPortEnvGuard::new()) - } - - fn set(&self, value: &str) { - self.0.set(OPENBAO_HOST_PORT_ENV, value); - } - } - #[test] fn default_url_takes_the_flag_supplied_port() { - let _guard = EnvGuard::new(); let dir = tempfile::tempdir().expect("tempdir"); assert_eq!( - effective_openbao_url(DEFAULT_OPENBAO_URL, dir.path(), Some(18200)), + effective_openbao_url_with_env(DEFAULT_OPENBAO_URL, dir.path(), Some(18200), None), "http://localhost:18200" ); } #[test] fn default_url_takes_the_process_env_port() { - let guard = EnvGuard::new(); let dir = tempfile::tempdir().expect("tempdir"); - guard.set("18201"); assert_eq!( - effective_openbao_url(DEFAULT_OPENBAO_URL, dir.path(), None), + effective_openbao_url_with_env(DEFAULT_OPENBAO_URL, dir.path(), None, Some("18201")), "http://localhost:18201" ); } #[test] fn default_url_takes_the_env_file_port() { - let _guard = EnvGuard::new(); let dir = tempfile::tempdir().expect("tempdir"); std::fs::write(dir.path().join(".env"), "OPENBAO_HOST_PORT=18202\n").expect("write .env"); assert_eq!( - effective_openbao_url(DEFAULT_OPENBAO_URL, dir.path(), None), + effective_openbao_url_with_env(DEFAULT_OPENBAO_URL, dir.path(), None, None), "http://localhost:18202" ); } #[test] fn flag_wins_over_process_env_and_env_file() { - let guard = EnvGuard::new(); let dir = tempfile::tempdir().expect("tempdir"); std::fs::write(dir.path().join(".env"), "OPENBAO_HOST_PORT=18203\n").expect("write .env"); - guard.set("18204"); assert_eq!( - effective_openbao_url(DEFAULT_OPENBAO_URL, dir.path(), Some(18205)), + effective_openbao_url_with_env( + DEFAULT_OPENBAO_URL, + dir.path(), + Some(18205), + Some("18204") + ), "http://localhost:18205" ); } #[test] fn default_url_unchanged_when_nothing_is_configured() { - let _guard = EnvGuard::new(); let dir = tempfile::tempdir().expect("tempdir"); assert_eq!( - effective_openbao_url(DEFAULT_OPENBAO_URL, dir.path(), None), + effective_openbao_url_with_env(DEFAULT_OPENBAO_URL, dir.path(), None, None), DEFAULT_OPENBAO_URL ); } #[test] fn non_default_url_is_never_rewritten() { - let guard = EnvGuard::new(); let dir = tempfile::tempdir().expect("tempdir"); // .env source. std::fs::write(dir.path().join(".env"), "OPENBAO_HOST_PORT=18206\n").expect("write .env"); assert_eq!( - effective_openbao_url(NON_DEFAULT_URL, dir.path(), None), + effective_openbao_url_with_env(NON_DEFAULT_URL, dir.path(), None, None), NON_DEFAULT_URL ); // Process-environment source. - guard.set("18207"); assert_eq!( - effective_openbao_url(NON_DEFAULT_URL, dir.path(), None), + effective_openbao_url_with_env(NON_DEFAULT_URL, dir.path(), None, Some("18207")), NON_DEFAULT_URL ); // Flag source. assert_eq!( - effective_openbao_url(NON_DEFAULT_URL, dir.path(), Some(18208)), + effective_openbao_url_with_env(NON_DEFAULT_URL, dir.path(), Some(18208), None), NON_DEFAULT_URL ); } diff --git a/src/commands/reinit.rs b/src/commands/reinit.rs index 97ddb8f7..acc79f0d 100644 --- a/src/commands/reinit.rs +++ b/src/commands/reinit.rs @@ -14,12 +14,16 @@ use crate::commands::clean::{ inspect_label_via_docker, remove_openbao_container_and_volumes, }; use crate::commands::compose_file::compose_file_dir; -use crate::commands::compose_project::resolve_compose_project_for_dir; +use crate::commands::compose_project::{ + compose_project_name_override, resolve_compose_project_for_dir, +}; use crate::commands::container_name::{BootrootContainer, resolve_container_name}; use crate::commands::guardrails::client_url_from_bind_addr; use crate::commands::infra::run_infra_up; use crate::commands::init::{compose_has_openbao, prompt_yes_no, run_init}; -use crate::commands::openbao_url::effective_openbao_url; +use crate::commands::openbao_url::{ + OPENBAO_HOST_PORT_ENV, effective_openbao_url, effective_openbao_url_with_env, +}; use crate::i18n::Messages; use crate::state::StateFile; @@ -100,6 +104,7 @@ pub(crate) async fn run_reinit(args: &ReinitArgs, messages: &Messages) -> Result verify_compose_managed_openbao( compose_file, &compose_dir, + compose_project_name_override().as_deref(), &openbao_container, &container_exists_via_docker, &inspect_label_via_docker, @@ -206,7 +211,12 @@ pub(crate) async fn run_reinit(args: &ReinitArgs, messages: &Messages) -> Result // `--root-token-output`, if set, is threaded into the init args // so the freshly issued root token is persisted with mode 0600 // after init succeeds. - let init_args = init_args_for_reinit(args, &snapshot, &effective_secrets_dir); + let init_args = init_args_for_reinit( + args, + &snapshot, + &effective_secrets_dir, + std::env::var(OPENBAO_HOST_PORT_ENV).ok().as_deref(), + ); run_init(&init_args, messages).await?; println!("{}", messages.reinit_completed()); @@ -324,6 +334,7 @@ pub(crate) fn reject_explicit_openbao_url(openbao_url: &str, messages: &Messages fn verify_compose_managed_openbao( compose_file: &Path, compose_dir: &Path, + project_override: Option<&str>, openbao_container: &str, container_exists: &dyn Fn(&str) -> Result, inspect: &dyn Fn(&str, &str) -> Result>, @@ -340,8 +351,11 @@ fn verify_compose_managed_openbao( // from a source independent of the container (env override or // compose-dir basename). Otherwise a mismatched container // would never trip the check. - let expected_project = - resolve_expected_compose_project_excluding_container(compose_dir, messages)?; + let expected_project = resolve_expected_compose_project_excluding_container( + compose_dir, + project_override, + messages, + )?; let container_project = inspect(openbao_container, COMPOSE_PROJECT_LABEL)?.ok_or_else(|| { anyhow::anyhow!(messages.error_reinit_container_missing_compose_label( @@ -375,7 +389,11 @@ fn verify_compose_managed_openbao( // only when the compose project can be derived from the work // directory; an unresolvable project surfaces here as an // actionable error rather than letting reinit proceed. - let _ = resolve_expected_compose_project_excluding_container(compose_dir, messages)?; + let _ = resolve_expected_compose_project_excluding_container( + compose_dir, + project_override, + messages, + )?; } Ok(()) } @@ -388,9 +406,10 @@ fn verify_compose_managed_openbao( /// "what is" side stays a real comparison rather than a tautology. fn resolve_expected_compose_project_excluding_container( compose_dir: &Path, + project_override: Option<&str>, messages: &Messages, ) -> Result { - resolve_compose_project_for_dir(compose_dir, None, messages) + resolve_compose_project_for_dir(compose_dir, None, project_override, messages) } /// Subset of `StateFile` fields preserved across a reinit. Mirrors the @@ -964,10 +983,16 @@ fn locate_provisioners(value: &serde_json::Value) -> Option<&Vec, ) -> Box { let mut openbao = args.openbao.clone(); if openbao.openbao_url == crate::commands::init::DEFAULT_OPENBAO_URL { @@ -979,10 +1004,11 @@ fn init_args_for_reinit( // silently point a destructive recovery at a dead endpoint. openbao.openbao_url = match snapshot.openbao_bind_addr.as_deref() { Some(bind) => client_url_from_bind_addr(bind), - None => effective_openbao_url( + None => effective_openbao_url_with_env( &openbao.openbao_url, &compose_file_dir(&args.compose.compose_file), None, + openbao_host_port_env, ), }; } @@ -1048,10 +1074,6 @@ mod tests { use tempfile::tempdir; use super::*; - // One crate-wide lock: the resolver these tests exercise is also - // driven from `clean` and `compose_project`, and a per-module mutex - // would let two modules mutate `COMPOSE_PROJECT_NAME` concurrently. - use crate::commands::compose_project::test_env::{ComposeProjectEnv, env_lock}; use crate::i18n::test_messages; use crate::state::{InfraCertEntry, ReloadStrategy, StateFile}; @@ -1289,6 +1311,7 @@ mod tests { let err = verify_compose_managed_openbao( &compose_file, dir.path(), + None, DEFAULT_OPENBAO_CONTAINER, &container_exists, &inspect, @@ -1314,6 +1337,7 @@ mod tests { let err = verify_compose_managed_openbao( &compose_file, dir.path(), + None, DEFAULT_OPENBAO_CONTAINER, &container_exists, &inspect, @@ -1343,14 +1367,12 @@ mod tests { Ok(None) } }; - // Make sure no stale env override decides the expected project. - // The lock guards process-wide env mutation against parallel - // tests in this module that also touch `COMPOSE_PROJECT_NAME`. - let _guard = env_lock(); - let _env = ComposeProjectEnv::set(None); + // No project override is passed, so the expected project comes + // from the work directory alone. let err = verify_compose_managed_openbao( &compose_file, &work, + None, DEFAULT_OPENBAO_CONTAINER, &container_exists, &inspect, @@ -1378,8 +1400,6 @@ mod tests { Ok(None) } }; - let _guard = env_lock(); - let _env = ComposeProjectEnv::set(None); for recorded in [None, Some("insight")] { let dir = tempdir().unwrap(); @@ -1393,6 +1413,7 @@ mod tests { let err = verify_compose_managed_openbao( &compose_file, &work, + None, DEFAULT_OPENBAO_CONTAINER, &container_exists, &inspect, @@ -1422,8 +1443,6 @@ mod tests { Ok(None) } }; - let _guard = env_lock(); - let _env = ComposeProjectEnv::set(None); let dir = tempdir().unwrap(); let work = dir.path().join("stack"); fs::create_dir_all(&work).unwrap(); @@ -1433,6 +1452,7 @@ mod tests { verify_compose_managed_openbao( &compose_file, &work, + None, DEFAULT_OPENBAO_CONTAINER, &container_exists, &inspect, @@ -1441,6 +1461,59 @@ mod tests { .expect("a container in the recorded instance's project must be accepted"); } + /// The E2E harness exports `COMPOSE_PROJECT_NAME` for a whole + /// scenario, and `run_reinit` hands what it held to the verifier. + /// The expected project has to follow it — otherwise reinit would + /// compare a running harness container against the `.env`-recorded + /// identity and refuse to proceed. + #[test] + fn verify_compose_managed_openbao_expects_the_project_override() { + const HARNESS_PROJECT: &str = "bootroot-e2e-ci-reinit-42"; + let messages = test_messages(); + let dir = tempdir().unwrap(); + let work = dir.path().join("stack"); + fs::create_dir_all(&work).unwrap(); + let compose_file = work.join("docker-compose.yml"); + fs::write(&compose_file, "services:\n openbao:\n image: openbao\n").unwrap(); + fs::write(work.join(".env"), "BOOTROOT_INSTANCE=insight\n").unwrap(); + let container_exists = |_: &str| -> Result { Ok(true) }; + let labelled = |project: &'static str| { + move |_: &str, label: &str| -> Result> { + if label == COMPOSE_PROJECT_LABEL { + Ok(Some(project.to_string())) + } else { + Ok(Some(OPENBAO_COMPOSE_SERVICE.to_string())) + } + } + }; + + verify_compose_managed_openbao( + &compose_file, + &work, + Some(HARNESS_PROJECT), + DEFAULT_OPENBAO_CONTAINER, + &container_exists, + &labelled(HARNESS_PROJECT), + &messages, + ) + .expect("a container in the overridden project must be accepted"); + + let err = verify_compose_managed_openbao( + &compose_file, + &work, + Some(HARNESS_PROJECT), + DEFAULT_OPENBAO_CONTAINER, + &container_exists, + &labelled("insight"), + &messages, + ) + .unwrap_err(); + assert!( + err.to_string().contains(HARNESS_PROJECT), + "the override must be the expected project, got: {err}" + ); + } + /// With a default install co-located, the existence question must be /// about *this* instance's container. Asking about `bootroot-openbao` /// would answer "exists" and then inspect the other install's labels. @@ -1468,6 +1541,7 @@ mod tests { verify_compose_managed_openbao( &compose_file, &work, + None, "insight-openbao", &container_exists, &inspect, @@ -1486,8 +1560,6 @@ mod tests { /// label branch runs and both inspections are about it. #[test] fn verify_compose_managed_openbao_inspects_the_instances_own_container() { - let _guard = env_lock(); - let _env = ComposeProjectEnv::set(None); let dir = tempdir().unwrap(); let work = dir.path().join("stack"); fs::create_dir_all(&work).unwrap(); @@ -1510,6 +1582,7 @@ mod tests { verify_compose_managed_openbao( &compose_file, &work, + None, "insight-openbao", &container_exists, &inspect, @@ -1539,6 +1612,7 @@ mod tests { verify_compose_managed_openbao( &compose_file, &work, + None, DEFAULT_OPENBAO_CONTAINER, &container_exists, &inspect, @@ -1567,6 +1641,7 @@ mod tests { let err = verify_compose_managed_openbao( &compose_file, &work, + None, DEFAULT_OPENBAO_CONTAINER, &container_exists, &inspect, @@ -1601,11 +1676,10 @@ mod tests { Ok(None) } }; - let _guard = env_lock(); - let _env = ComposeProjectEnv::set(None); let result = verify_compose_managed_openbao( &compose_file, &work, + None, DEFAULT_OPENBAO_CONTAINER, &container_exists, &inspect, @@ -1639,11 +1713,10 @@ mod tests { Ok(None) } }; - let _guard = env_lock(); - let _env = ComposeProjectEnv::set(None); verify_compose_managed_openbao( &compose_file, &work, + None, DEFAULT_OPENBAO_CONTAINER, &container_exists, &inspect, @@ -1654,43 +1727,45 @@ mod tests { /// Regression for #611: when `--compose-file` is the default /// relative `docker-compose.yml`, `run_reinit` derives `compose_dir` - /// via [`compose_file_dir`]. The verifier must accept that - /// `compose_dir` and complete its scope check without surfacing - /// `could not derive compose project name from `. Exercises the - /// container-absent branch (the stuck-after-`clean --openbao-only` - /// recovery path) with CWD pointed at a tempdir holding a valid - /// compose declaration. + /// via [`compose_file_dir`], which normalises a bare filename to + /// `.`. The verifier must accept that `compose_dir` and complete + /// its scope check without surfacing `could not derive compose + /// project name from `. Exercises the container-absent branch (the + /// stuck-after-`clean --openbao-only` recovery path). + /// + /// The `.` is what broke; the working directory it happens to name + /// is not. The directory is therefore given to the verifier as a + /// parameter, and the normalisation is pinned separately — moving + /// the whole process into a tempdir to reproduce it would race every + /// other test in the binary, the working directory being + /// process-global exactly as the environment is. #[test] fn verify_compose_managed_openbao_accepts_default_relative_compose_file() { - let _guard = env_lock(); - let _env = ComposeProjectEnv::set(None); + assert_eq!( + compose_file_dir(Path::new("docker-compose.yml")), + PathBuf::from("."), + "the default --compose-file must still normalise to `.`" + ); + let messages = test_messages(); + resolve_expected_compose_project_excluding_container(Path::new("."), None, &messages) + .expect("`.` must resolve to a compose project"); let dir = tempdir().unwrap(); - fs::write( - dir.path().join("docker-compose.yml"), - "services:\n openbao:\n image: openbao\n", - ) - .unwrap(); - let original_cwd = std::env::current_dir().unwrap(); - std::env::set_current_dir(dir.path()).unwrap(); - - let compose_file = PathBuf::from("docker-compose.yml"); + let compose_file = dir.path().join("docker-compose.yml"); + fs::write(&compose_file, "services:\n openbao:\n image: openbao\n").unwrap(); let compose_dir = compose_file_dir(&compose_file); - let messages = test_messages(); let container_exists = |_: &str| -> Result { Ok(false) }; let inspect = |_: &str, _: &str| -> Result> { Ok(None) }; - let result = verify_compose_managed_openbao( + verify_compose_managed_openbao( &compose_file, &compose_dir, + None, DEFAULT_OPENBAO_CONTAINER, &container_exists, &inspect, &messages, - ); - - std::env::set_current_dir(&original_cwd).unwrap(); - - result.expect("default relative --compose-file must not break the scope check"); + ) + .expect("default relative --compose-file must not break the scope check"); } #[test] @@ -2133,6 +2208,7 @@ mod tests { &reinit_args, &DeploymentIntent::default(), &reinit_args.secrets_dir.secrets_dir, + None, ); assert_eq!( init_args.root_token_output, @@ -2176,6 +2252,7 @@ mod tests { &reinit_args, &snapshot, &reinit_args.secrets_dir.secrets_dir, + None, ); assert_eq!( init_args.openbao.openbao_url, "https://192.168.1.10:8200", @@ -2188,7 +2265,6 @@ mod tests { /// CLI default's literal 8200. #[test] fn init_args_for_reinit_carries_the_resolved_host_port() { - let _env = crate::commands::openbao_url::test_env::HostPortEnvGuard::new(); let dir = tempdir().unwrap(); fs::write(dir.path().join(".env"), "OPENBAO_HOST_PORT=18200\n").unwrap(); let reinit_args = ReinitArgs { @@ -2213,6 +2289,7 @@ mod tests { &reinit_args, &DeploymentIntent::default(), &reinit_args.secrets_dir.secrets_dir, + None, ); assert_eq!(init_args.openbao.openbao_url, "http://localhost:18200"); } @@ -2223,7 +2300,6 @@ mod tests { /// destructive recovery at an endpoint that no longer serves. #[test] fn init_args_for_reinit_prefers_the_bind_addr_over_the_host_port() { - let _env = crate::commands::openbao_url::test_env::HostPortEnvGuard::new(); let dir = tempdir().unwrap(); fs::write(dir.path().join(".env"), "OPENBAO_HOST_PORT=18200\n").unwrap(); let reinit_args = ReinitArgs { @@ -2252,6 +2328,7 @@ mod tests { &reinit_args, &snapshot, &reinit_args.secrets_dir.secrets_dir, + None, ); assert_eq!( init_args.openbao.openbao_url, "https://192.168.1.10:8200", @@ -2312,6 +2389,7 @@ mod tests { &reinit_args, &DeploymentIntent::default(), &reinit_args.secrets_dir.secrets_dir, + None, ); assert_eq!( init_args.db_dsn.as_deref(), @@ -2351,6 +2429,7 @@ mod tests { &reinit_args, &DeploymentIntent::default(), &reinit_args.secrets_dir.secrets_dir, + None, ); assert!( init_args.db_dsn.is_none(), @@ -2417,6 +2496,7 @@ mod tests { &reinit_args, &DeploymentIntent::default(), &reinit_args.secrets_dir.secrets_dir, + None, ); assert_eq!( init_args.stepca_provisioner, @@ -2462,6 +2542,7 @@ mod tests { &reinit_args, &DeploymentIntent::default(), &reinit_args.secrets_dir.secrets_dir, + None, ); assert_eq!( init_args.stepca_provisioner, @@ -2628,7 +2709,7 @@ mod tests { let effective = effective_secrets_dir(&reinit_args, &snapshot); assert_eq!(effective, snapshot_secrets); - let init_args = init_args_for_reinit(&reinit_args, &snapshot, &effective); + let init_args = init_args_for_reinit(&reinit_args, &snapshot, &effective, None); assert_eq!( init_args.secrets_dir.secrets_dir, snapshot_secrets, "init args must carry the snapshotted secrets_dir so the \ diff --git a/src/commands/rotate/db.rs b/src/commands/rotate/db.rs index de9bc8ea..68d0c9d1 100644 --- a/src/commands/rotate/db.rs +++ b/src/commands/rotate/db.rs @@ -3,7 +3,9 @@ use std::path::Path; use std::time::Duration; use anyhow::{Context, Result}; -use bootroot::db::{self, effective_admin_dsn_for_kv, for_compose_runtime, for_host_runtime}; +use bootroot::db::{ + self, effective_admin_dsn_for_kv, for_compose_runtime, for_host_runtime_with_port, +}; use bootroot::openbao::OpenBaoClient; use super::helpers::{ @@ -42,7 +44,11 @@ pub(super) async fn rotate_db( } else { read_admin_dsn_from_kv(client, &ctx.kv_mount).await? }; - let admin_dsn = resolve_db_admin_dsn(args, &compose_dir, kv_admin_dsn.as_deref(), messages)?; + // The host-side port the compose stack publishes, resolved once + // here so the DSN translation below is steered by a value rather + // than by the process environment. + let host_port = db::resolve_postgres_host_port(&compose_dir); + let admin_dsn = resolve_db_admin_dsn(args, host_port, kv_admin_dsn.as_deref(), messages)?; let admin = db::parse_db_dsn(&admin_dsn).with_context(|| messages.error_invalid_db_dsn())?; ensure_single_host_db_host(&admin.host, messages)?; let db_password = match &args.password { @@ -136,9 +142,15 @@ pub(super) async fn rotate_db( Ok(()) } +/// Resolves the admin DSN `rotate db` provisions with. +/// +/// `host_port` is the host-side published `PostgreSQL` port, per +/// [`db::resolve_postgres_host_port`]: the persisted DSN is stored in +/// compose-internal form and has to be translated to something +/// reachable from the host this command runs on. fn resolve_db_admin_dsn( args: &RotateDbArgs, - compose_dir: &Path, + host_port: u16, kv_admin_dsn: Option<&str>, messages: &Messages, ) -> Result { @@ -149,11 +161,11 @@ fn resolve_db_admin_dsn( return Ok(value.clone()); } // Persisted admin DSN from `init --enable db-provision`. Translate - // through `for_host_runtime` so a value that was stored in + // through `for_host_runtime_with_port` so a value that was stored in // compose-internal form (host `postgres`) becomes reachable from // the host where `rotate db` runs. if let Some(value) = kv_admin_dsn { - return for_host_runtime(value, compose_dir) + return for_host_runtime_with_port(value, host_port) .with_context(|| messages.error_invalid_db_dsn()); } // Per issue #588 §2: do NOT fall back to `ca.json.db.dataSource`. @@ -246,16 +258,16 @@ fn parse_ca_json_dsn(contents: &str, messages: &Messages) -> Result { mod tests { use std::fs; + use bootroot::db::DEFAULT_POSTGRES_HOST_PORT; use tempfile::tempdir; - use super::super::test_support::{ScopedEnvVar, env_lock, test_messages}; + use super::super::test_support::test_messages; use super::*; use crate::cli::args::{DbAdminDsnArgs, DbTimeoutArgs}; #[test] fn resolve_db_admin_dsn_uses_cli_arg() { let messages = test_messages(); - let dir = tempdir().expect("tempdir"); let args = RotateDbArgs { admin_dsn: DbAdminDsnArgs { admin_dsn: Some("postgresql://admin:pass@127.0.0.1:15432/postgres".to_string()), @@ -263,8 +275,8 @@ mod tests { password: None, timeout: DbTimeoutArgs { timeout_secs: 30 }, }; - let resolved = - resolve_db_admin_dsn(&args, dir.path(), None, &messages).expect("resolve dsn"); + let resolved = resolve_db_admin_dsn(&args, DEFAULT_POSTGRES_HOST_PORT, None, &messages) + .expect("resolve dsn"); assert_eq!(resolved, "postgresql://admin:pass@127.0.0.1:15432/postgres"); } @@ -275,8 +287,6 @@ mod tests { // DSN; using it as an admin DSN reproduces the original // self-ALTER failure. With no flag and no KV value, fail fast // with a message naming the available recovery paths. - let _lock = env_lock(); - let _port_guard = ScopedEnvVar::set("POSTGRES_HOST_PORT", ""); let messages = test_messages(); let dir = tempdir().expect("tempdir"); // Even an existing ca.json must not be silently consumed as an @@ -292,7 +302,7 @@ mod tests { password: None, timeout: DbTimeoutArgs { timeout_secs: 30 }, }; - let err = resolve_db_admin_dsn(&args, dir.path(), None, &messages) + let err = resolve_db_admin_dsn(&args, DEFAULT_POSTGRES_HOST_PORT, None, &messages) .expect_err("must error when no admin DSN source is available"); let chained = err .chain() @@ -309,28 +319,35 @@ mod tests { fn resolve_db_admin_dsn_prefers_kv_admin() { // Closes issue #588 §2: when init persisted the admin DSN to // KV, `rotate db` must use it instead of erroring out. - let _lock = env_lock(); - let _port_guard = ScopedEnvVar::set("POSTGRES_HOST_PORT", ""); let messages = test_messages(); - let dir = tempdir().expect("tempdir"); let args = RotateDbArgs { admin_dsn: DbAdminDsnArgs { admin_dsn: None }, password: None, timeout: DbTimeoutArgs { timeout_secs: 30 }, }; let kv_admin = "postgresql://step:admin@postgres:5432/postgres?sslmode=disable"; - let resolved = resolve_db_admin_dsn(&args, dir.path(), Some(kv_admin), &messages) - .expect("resolve dsn"); + let resolved = + resolve_db_admin_dsn(&args, DEFAULT_POSTGRES_HOST_PORT, Some(kv_admin), &messages) + .expect("resolve dsn"); assert_eq!( resolved, "postgresql://step:admin@127.0.0.1:5433/postgres?sslmode=disable", "must use the persisted admin DSN, translated to host-side" ); + assert_eq!(DEFAULT_POSTGRES_HOST_PORT, 5433); + + // The port is what the translation follows, so a different one + // has to land in the DSN. + let on_another_port = + resolve_db_admin_dsn(&args, 6543, Some(kv_admin), &messages).expect("resolve dsn"); + assert_eq!( + on_another_port, + "postgresql://step:admin@127.0.0.1:6543/postgres?sslmode=disable" + ); } #[test] fn resolve_db_admin_dsn_cli_flag_overrides_kv() { let messages = test_messages(); - let dir = tempdir().expect("tempdir"); let args = RotateDbArgs { admin_dsn: DbAdminDsnArgs { admin_dsn: Some("postgresql://flag:pass@127.0.0.1:9999/postgres".to_string()), @@ -340,7 +357,7 @@ mod tests { }; let resolved = resolve_db_admin_dsn( &args, - dir.path(), + DEFAULT_POSTGRES_HOST_PORT, Some("postgresql://kv:kv@postgres:5432/postgres"), &messages, ) diff --git a/src/commands/status.rs b/src/commands/status.rs index 827af25b..c6c345c9 100644 --- a/src/commands/status.rs +++ b/src/commands/status.rs @@ -16,7 +16,7 @@ use crate::commands::init::{ APPROLE_BOOTROOT_STEPCA, PATH_AGENT_EAB, PATH_CA_TRUST, PATH_RESPONDER_HMAC, PATH_STEPCA_DB, PATH_STEPCA_PASSWORD, SECRET_ID_TTL, parse_ttl_to_secs, }; -use crate::commands::openbao_url::effective_openbao_url; +use crate::commands::openbao_url::{OPENBAO_HOST_PORT_ENV, effective_openbao_url_with_env}; use crate::i18n::Messages; use crate::state::StateFile; @@ -135,10 +135,17 @@ pub(crate) async fn run_status(args: &StatusArgs, messages: &Messages) -> Result /// another bootroot instance's `OpenBao`. An explicit `--openbao-url` /// is still honoured verbatim. fn status_openbao_url(args: &StatusArgs) -> String { - effective_openbao_url( + status_openbao_url_with_env(args, std::env::var(OPENBAO_HOST_PORT_ENV).ok().as_deref()) +} + +/// [`status_openbao_url`] with the `OPENBAO_HOST_PORT` value supplied by +/// the caller instead of read from the process environment. +fn status_openbao_url_with_env(args: &StatusArgs, host_port_env: Option<&str>) -> String { + effective_openbao_url_with_env( &args.openbao.openbao_url, &compose_file_dir(&args.compose.compose_file), None, + host_port_env, ) } @@ -406,28 +413,33 @@ mod tests { /// recorded in the compose directory's `.env`. #[test] fn status_openbao_url_follows_the_configured_host_port() { - let _env = crate::commands::openbao_url::test_env::HostPortEnvGuard::new(); let dir = tempfile::tempdir().expect("tempdir"); std::fs::write(dir.path().join(".env"), "OPENBAO_HOST_PORT=18200\n").expect("write .env"); let args = status_args( dir.path().join("docker-compose.yml"), crate::commands::init::DEFAULT_OPENBAO_URL, ); - assert_eq!(status_openbao_url(&args), "http://localhost:18200"); + assert_eq!( + status_openbao_url_with_env(&args, None), + "http://localhost:18200" + ); } /// Closes #731: an operator-supplied `--openbao-url` is used /// verbatim, whatever the configured host port is. #[test] fn status_openbao_url_honours_an_explicit_url() { - let _env = crate::commands::openbao_url::test_env::HostPortEnvGuard::new(); let dir = tempfile::tempdir().expect("tempdir"); std::fs::write(dir.path().join(".env"), "OPENBAO_HOST_PORT=18200\n").expect("write .env"); let args = status_args( dir.path().join("docker-compose.yml"), "https://openbao.internal:8200", ); - assert_eq!(status_openbao_url(&args), "https://openbao.internal:8200"); + assert_eq!( + status_openbao_url_with_env(&args, Some("18201")), + "https://openbao.internal:8200", + "an explicit --openbao-url outranks every port source" + ); } fn rfc3339(ts: OffsetDateTime) -> String { diff --git a/src/db.rs b/src/db.rs index 16145bad..21aca45e 100644 --- a/src/db.rs +++ b/src/db.rs @@ -186,6 +186,22 @@ pub fn resolve_postgres_host_port(compose_dir: &Path) -> u16 { ) } +/// [`resolve_postgres_host_port`] with the `POSTGRES_HOST_PORT` value +/// supplied by the caller instead of read from the process environment. +/// +/// Lets a caller that already holds the variable — `init`'s DSN +/// builders, and the tests steering them — drive step 1 of the +/// precedence without mutating process-global state. +#[must_use] +pub fn resolve_postgres_host_port_with_env(env_value: Option<&str>, compose_dir: &Path) -> u16 { + crate::host_port::resolve_host_port_with_env( + env_value, + compose_dir, + POSTGRES_HOST_PORT_ENV, + DEFAULT_POSTGRES_HOST_PORT, + ) +} + /// Returns the admin DSN that should be persisted to KV after running /// [`provision_db_sync`]. /// @@ -649,18 +665,21 @@ mod tests { fn host_compose_round_trip_with_env_file() { // Round-trip symmetry: host -> compose -> host returns the original // (host, port) pair, with compose_dir/.env providing - // POSTGRES_HOST_PORT. + // POSTGRES_HOST_PORT. The `None` is the process environment + // holding no value, so the file is what decides. let dir = tempfile::tempdir().expect("tempdir"); std::fs::write(dir.path().join(".env"), "POSTGRES_HOST_PORT=5433\n").expect("write .env"); - // Ensure process env does not override the file value. - let _guard = env_port_guard(); let host_dsn = "postgresql://step:pw@127.0.0.1:5433/stepca?sslmode=disable"; let compose = for_compose_runtime(host_dsn).unwrap(); assert_eq!( compose, "postgresql://step:pw@postgres:5432/stepca?sslmode=disable" ); - let round_trip = for_host_runtime(&compose, dir.path()).unwrap(); + let round_trip = for_host_runtime_with_port( + &compose, + resolve_postgres_host_port_with_env(None, dir.path()), + ) + .unwrap(); assert_eq!(round_trip, host_dsn); } @@ -668,21 +687,17 @@ mod tests { fn resolve_postgres_host_port_prefers_process_env() { let dir = tempfile::tempdir().expect("tempdir"); std::fs::write(dir.path().join(".env"), "POSTGRES_HOST_PORT=9999\n").expect("write .env"); - let _guard = env_port_guard(); - // SAFETY: `env_port_guard` serializes POSTGRES_HOST_PORT access and - // restores the value on drop. - unsafe { - std::env::set_var(POSTGRES_HOST_PORT_ENV, "7777"); - } - assert_eq!(resolve_postgres_host_port(dir.path()), 7777); + assert_eq!( + resolve_postgres_host_port_with_env(Some("7777"), dir.path()), + 7777 + ); } #[test] fn resolve_postgres_host_port_defaults_when_missing() { let dir = tempfile::tempdir().expect("tempdir"); - let _guard = env_port_guard(); assert_eq!( - resolve_postgres_host_port(dir.path()), + resolve_postgres_host_port_with_env(None, dir.path()), DEFAULT_POSTGRES_HOST_PORT ); } @@ -695,43 +710,6 @@ mod tests { "# comment\nOTHER=1\nPOSTGRES_HOST_PORT=\"5433\"\n", ) .expect("write .env"); - let _guard = env_port_guard(); - assert_eq!(resolve_postgres_host_port(dir.path()), 5433); - } - - /// Guards `POSTGRES_HOST_PORT` across tests that read/write process env. - struct EnvPortGuard { - _lock: std::sync::MutexGuard<'static, ()>, - previous: Option, - } - - impl Drop for EnvPortGuard { - fn drop(&mut self) { - // SAFETY: restored inside the shared mutex held by `_lock`. - unsafe { - match &self.previous { - Some(value) => std::env::set_var(POSTGRES_HOST_PORT_ENV, value), - None => std::env::remove_var(POSTGRES_HOST_PORT_ENV), - } - } - } - } - - fn env_port_guard() -> EnvPortGuard { - use std::sync::{Mutex, OnceLock}; - static LOCK: OnceLock> = OnceLock::new(); - let lock = LOCK - .get_or_init(|| Mutex::new(())) - .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner); - let previous = std::env::var(POSTGRES_HOST_PORT_ENV).ok(); - // SAFETY: removal is serialized by the mutex above. - unsafe { - std::env::remove_var(POSTGRES_HOST_PORT_ENV); - } - EnvPortGuard { - _lock: lock, - previous, - } + assert_eq!(resolve_postgres_host_port_with_env(None, dir.path()), 5433); } } diff --git a/src/host_port.rs b/src/host_port.rs index 7a2ae7fd..0f7878de 100644 --- a/src/host_port.rs +++ b/src/host_port.rs @@ -34,9 +34,10 @@ pub const HTTP01_ADMIN_HOST_PORT_ENV: &str = "HTTP01_ADMIN_HOST_PORT"; pub const DEFAULT_HTTP01_ADMIN_HOST_PORT: u16 = 8080; /// Resolves a host-side published port with the same precedence Docker -/// Compose uses for a `${:-}` port mapping: +/// Compose uses for a `${:-}` port mapping, given the +/// value the process environment holds for `key`: /// -/// 1. process environment `key` (non-empty), else +/// 1. `env_value` (present and non-empty), else /// 2. `key` from `compose_dir/.env`, else /// 3. `default`. /// @@ -44,12 +45,21 @@ pub const DEFAULT_HTTP01_ADMIN_HOST_PORT: u16 = 8080; /// rather than erroring — the caller is deriving an endpoint, not /// validating the environment. /// +/// The environment value is a parameter rather than a read so a caller +/// — a test, or a command that already resolved it — can steer the +/// first step without touching the process-global environment. +/// /// Crate-internal: the exported surface is the per-service resolvers /// below, which pin the `(key, default)` pair to what the compose files /// actually interpolate. #[must_use] -pub(crate) fn resolve_host_port(compose_dir: &Path, key: &str, default: u16) -> u16 { - if let Ok(value) = std::env::var(key) +pub(crate) fn resolve_host_port_with_env( + env_value: Option<&str>, + compose_dir: &Path, + key: &str, + default: u16, +) -> u16 { + if let Some(value) = env_value && !value.is_empty() && let Ok(port) = value.parse::() { @@ -63,17 +73,75 @@ pub(crate) fn resolve_host_port(compose_dir: &Path, key: &str, default: u16) -> default } +/// [`resolve_host_port_with_env`] reading `key` from the process +/// environment. +/// +/// The one place in this module that touches the environment; every +/// other resolver here is a `(key, default)` pairing on top of it. +#[must_use] +pub(crate) fn resolve_host_port(compose_dir: &Path, key: &str, default: u16) -> u16 { + resolve_host_port_with_env( + std::env::var(key).ok().as_deref(), + compose_dir, + key, + default, + ) +} + +/// One service's published host port: the environment variable that +/// overrides it and the compile-time default the compose files +/// interpolate. +/// +/// The two travel as one value so a resolver and the test that steers +/// it cannot name different halves of the pair. +#[derive(Debug, Clone, Copy)] +struct HostPortSpec { + key: &'static str, + default: u16, +} + +const OPENBAO_HOST_PORT: HostPortSpec = HostPortSpec { + key: OPENBAO_HOST_PORT_ENV, + default: DEFAULT_OPENBAO_HOST_PORT, +}; +const STEPCA_HOST_PORT: HostPortSpec = HostPortSpec { + key: STEPCA_HOST_PORT_ENV, + default: DEFAULT_STEPCA_HOST_PORT, +}; +const HTTP01_ADMIN_HOST_PORT: HostPortSpec = HostPortSpec { + key: HTTP01_ADMIN_HOST_PORT_ENV, + default: DEFAULT_HTTP01_ADMIN_HOST_PORT, +}; + +impl HostPortSpec { + /// Resolves this port, reading the override from the process + /// environment. + fn resolve(self, compose_dir: &Path) -> u16 { + resolve_host_port(compose_dir, self.key, self.default) + } + + /// [`HostPortSpec::resolve`] with the process-environment value + /// supplied by the caller. + fn resolve_with_env(self, env_value: Option<&str>, compose_dir: &Path) -> u16 { + resolve_host_port_with_env(env_value, compose_dir, self.key, self.default) + } +} + /// Resolves the host-side `OpenBao` port: process environment /// [`OPENBAO_HOST_PORT_ENV`] (non-empty), else the same key from /// `compose_dir/.env`, else [`DEFAULT_OPENBAO_HOST_PORT`]. An /// unparseable value falls through to the next source. #[must_use] pub fn resolve_openbao_host_port(compose_dir: &Path) -> u16 { - resolve_host_port( - compose_dir, - OPENBAO_HOST_PORT_ENV, - DEFAULT_OPENBAO_HOST_PORT, - ) + OPENBAO_HOST_PORT.resolve(compose_dir) +} + +/// [`resolve_openbao_host_port`] with the `OPENBAO_HOST_PORT` value +/// supplied by the caller instead of read from the process +/// environment. +#[must_use] +pub fn resolve_openbao_host_port_with_env(env_value: Option<&str>, compose_dir: &Path) -> u16 { + OPENBAO_HOST_PORT.resolve_with_env(env_value, compose_dir) } /// Resolves the host-side step-ca port: process environment @@ -82,7 +150,14 @@ pub fn resolve_openbao_host_port(compose_dir: &Path) -> u16 { /// value falls through to the next source. #[must_use] pub fn resolve_stepca_host_port(compose_dir: &Path) -> u16 { - resolve_host_port(compose_dir, STEPCA_HOST_PORT_ENV, DEFAULT_STEPCA_HOST_PORT) + STEPCA_HOST_PORT.resolve(compose_dir) +} + +/// [`resolve_stepca_host_port`] with the `STEPCA_HOST_PORT` value +/// supplied by the caller instead of read from the process environment. +#[must_use] +pub fn resolve_stepca_host_port_with_env(env_value: Option<&str>, compose_dir: &Path) -> u16 { + STEPCA_HOST_PORT.resolve_with_env(env_value, compose_dir) } /// Resolves the host-side HTTP-01 admin API port: process environment @@ -91,11 +166,15 @@ pub fn resolve_stepca_host_port(compose_dir: &Path) -> u16 { /// unparseable value falls through to the next source. #[must_use] pub fn resolve_http01_admin_host_port(compose_dir: &Path) -> u16 { - resolve_host_port( - compose_dir, - HTTP01_ADMIN_HOST_PORT_ENV, - DEFAULT_HTTP01_ADMIN_HOST_PORT, - ) + HTTP01_ADMIN_HOST_PORT.resolve(compose_dir) +} + +/// [`resolve_http01_admin_host_port`] with the +/// `HTTP01_ADMIN_HOST_PORT` value supplied by the caller instead of +/// read from the process environment. +#[must_use] +pub fn resolve_http01_admin_host_port_with_env(env_value: Option<&str>, compose_dir: &Path) -> u16 { + HTTP01_ADMIN_HOST_PORT.resolve_with_env(env_value, compose_dir) } /// Reads a single `key=value` entry out of `compose_dir/.env`. @@ -142,164 +221,163 @@ fn read_env_file_value(compose_dir: &Path, key: &str) -> Option { #[cfg(test)] mod tests { - use std::sync::{Mutex, MutexGuard, OnceLock, PoisonError}; - use super::*; - const TEST_KEYS: [&str; 3] = [ - OPENBAO_HOST_PORT_ENV, - STEPCA_HOST_PORT_ENV, - HTTP01_ADMIN_HOST_PORT_ENV, - ]; - - /// Serialises every test that touches the host-port environment - /// variables and restores their pre-test values on drop. - struct EnvGuard { - _lock: MutexGuard<'static, ()>, - previous: Vec<(&'static str, Option)>, - } - - impl EnvGuard { - fn new() -> Self { - static LOCK: OnceLock> = OnceLock::new(); - let lock = LOCK - .get_or_init(|| Mutex::new(())) - .lock() - .unwrap_or_else(PoisonError::into_inner); - let previous = TEST_KEYS - .iter() - .map(|key| (*key, std::env::var(key).ok())) - .collect(); - for key in TEST_KEYS { - // SAFETY: removal is serialized by the mutex held above. - unsafe { - std::env::remove_var(key); - } - } - Self { - _lock: lock, - previous, - } - } - - fn set(&self, key: &str, value: &str) { - assert!( - self.previous.iter().any(|(tracked, _)| *tracked == key), - "{key} is not tracked by the guard and would leak into other tests" - ); - // SAFETY: mutation is serialized by the mutex held by `_lock`. - unsafe { - std::env::set_var(key, value); - } - } - } - - impl Drop for EnvGuard { - fn drop(&mut self) { - for (key, previous) in &self.previous { - // SAFETY: restored inside the shared mutex held by `_lock`. - unsafe { - match previous { - Some(value) => std::env::set_var(key, value), - None => std::env::remove_var(key), - } - } - } - } - } - - /// One new variable, as `(env key, compile-time default, resolver)`. - type ResolverCase = (&'static str, u16, fn(&Path) -> u16); - - /// Every variable this module adds. - fn cases() -> Vec { - vec![ - ( - OPENBAO_HOST_PORT_ENV, - DEFAULT_OPENBAO_HOST_PORT, - resolve_openbao_host_port as fn(&Path) -> u16, - ), - ( - STEPCA_HOST_PORT_ENV, - DEFAULT_STEPCA_HOST_PORT, - resolve_stepca_host_port as fn(&Path) -> u16, - ), - ( - HTTP01_ADMIN_HOST_PORT_ENV, - DEFAULT_HTTP01_ADMIN_HOST_PORT, - resolve_http01_admin_host_port as fn(&Path) -> u16, - ), - ] - } + /// Every host port this module resolves. + /// + /// The per-service resolvers are one call each on top of these, so + /// driving the specs directly covers the precedence rules once for + /// all three without steering the process environment. + const SPECS: [HostPortSpec; 3] = [OPENBAO_HOST_PORT, STEPCA_HOST_PORT, HTTP01_ADMIN_HOST_PORT]; #[test] fn process_env_wins_over_env_file() { - let guard = EnvGuard::new(); - for (key, _, resolve) in cases() { + for spec in SPECS { let dir = tempfile::tempdir().expect("tempdir"); - std::fs::write(dir.path().join(".env"), format!("{key}=19999\n")).expect("write .env"); - guard.set(key, "17777"); - assert_eq!(resolve(dir.path()), 17777, "{key} must prefer process env"); + std::fs::write(dir.path().join(".env"), format!("{}=19999\n", spec.key)) + .expect("write .env"); + assert_eq!( + spec.resolve_with_env(Some("17777"), dir.path()), + 17777, + "{} must prefer the process env", + spec.key + ); } } #[test] fn env_file_used_when_process_env_unset() { - let _guard = EnvGuard::new(); - for (key, _, resolve) in cases() { + for spec in SPECS { let dir = tempfile::tempdir().expect("tempdir"); std::fs::write( dir.path().join(".env"), - format!("# comment\nOTHER=1\n{key}=\"18888\"\n"), + format!("# comment\nOTHER=1\n{}=\"18888\"\n", spec.key), ) .expect("write .env"); - assert_eq!(resolve(dir.path()), 18888, "{key} must read .env"); + assert_eq!( + spec.resolve_with_env(None, dir.path()), + 18888, + "{} must read .env", + spec.key + ); } } #[test] fn default_used_when_nothing_set() { - let _guard = EnvGuard::new(); - for (key, default, resolve) in cases() { + for spec in SPECS { let dir = tempfile::tempdir().expect("tempdir"); - assert_eq!(resolve(dir.path()), default, "{key} must fall back"); + assert_eq!( + spec.resolve_with_env(None, dir.path()), + spec.default, + "{} must fall back", + spec.key + ); } } #[test] fn unparseable_process_env_falls_through_to_env_file() { - let guard = EnvGuard::new(); - for (key, _, resolve) in cases() { + for spec in SPECS { let dir = tempfile::tempdir().expect("tempdir"); - std::fs::write(dir.path().join(".env"), format!("{key}=16666\n")).expect("write .env"); - guard.set(key, "not-a-port"); + std::fs::write(dir.path().join(".env"), format!("{}=16666\n", spec.key)) + .expect("write .env"); assert_eq!( - resolve(dir.path()), + spec.resolve_with_env(Some("not-a-port"), dir.path()), 16666, - "{key} must fall through to .env" + "{} must fall through to .env", + spec.key ); } } #[test] fn unparseable_env_file_falls_through_to_default() { - let _guard = EnvGuard::new(); - for (key, default, resolve) in cases() { + for spec in SPECS { let dir = tempfile::tempdir().expect("tempdir"); - std::fs::write(dir.path().join(".env"), format!("{key}=99999999\n")) + std::fs::write(dir.path().join(".env"), format!("{}=99999999\n", spec.key)) .expect("write .env"); - assert_eq!(resolve(dir.path()), default, "{key} must fall through"); + assert_eq!( + spec.resolve_with_env(None, dir.path()), + spec.default, + "{} must fall through", + spec.key + ); } } #[test] fn empty_process_env_falls_through_to_env_file() { - let guard = EnvGuard::new(); - for (key, _, resolve) in cases() { + for spec in SPECS { + let dir = tempfile::tempdir().expect("tempdir"); + std::fs::write(dir.path().join(".env"), format!("{}=15555\n", spec.key)) + .expect("write .env"); + assert_eq!( + spec.resolve_with_env(Some(""), dir.path()), + 15555, + "{} must ignore an empty env", + spec.key + ); + } + } + + /// The specs are what pair a key with its default; a resolver that + /// picked up the wrong one would read the wrong variable and + /// publish the wrong fallback, and every test above would still + /// pass. Pin both halves against the documented values. + #[test] + fn each_spec_pairs_its_documented_key_and_default() { + assert_eq!( + (OPENBAO_HOST_PORT.key, OPENBAO_HOST_PORT.default), + (OPENBAO_HOST_PORT_ENV, DEFAULT_OPENBAO_HOST_PORT) + ); + assert_eq!( + (STEPCA_HOST_PORT.key, STEPCA_HOST_PORT.default), + (STEPCA_HOST_PORT_ENV, DEFAULT_STEPCA_HOST_PORT) + ); + assert_eq!( + (HTTP01_ADMIN_HOST_PORT.key, HTTP01_ADMIN_HOST_PORT.default), + (HTTP01_ADMIN_HOST_PORT_ENV, DEFAULT_HTTP01_ADMIN_HOST_PORT) + ); + } + + /// Each exported resolver's parameterised sibling is what every + /// caller outside this module steers, so it has to carry the same + /// spec as the resolver it mirrors. A sibling wired to the wrong + /// key would be invisible to every test above. + #[test] + fn each_parameterised_sibling_mirrors_its_resolver() { + type Sibling = fn(Option<&str>, &Path) -> u16; + let siblings: [(HostPortSpec, Sibling); 3] = [ + (OPENBAO_HOST_PORT, resolve_openbao_host_port_with_env), + (STEPCA_HOST_PORT, resolve_stepca_host_port_with_env), + ( + HTTP01_ADMIN_HOST_PORT, + resolve_http01_admin_host_port_with_env, + ), + ]; + for (spec, sibling) in siblings { let dir = tempfile::tempdir().expect("tempdir"); - std::fs::write(dir.path().join(".env"), format!("{key}=15555\n")).expect("write .env"); - guard.set(key, ""); - assert_eq!(resolve(dir.path()), 15555, "{key} must ignore an empty env"); + std::fs::write(dir.path().join(".env"), format!("{}=18299\n", spec.key)) + .expect("write .env"); + assert_eq!( + sibling(None, dir.path()), + 18299, + "{} must be read from .env", + spec.key + ); + assert_eq!( + sibling(Some("18300"), dir.path()), + 18300, + "the supplied {} must outrank .env", + spec.key + ); + assert_eq!( + sibling(None, tempfile::tempdir().expect("tempdir").path()), + spec.default, + "{} must fall back to its own default", + spec.key + ); } } @@ -320,13 +398,12 @@ mod tests { /// default and preflight a port Compose never publishes. #[test] fn env_file_line_without_a_separator_does_not_end_the_scan() { - let _guard = EnvGuard::new(); let dir = tempfile::tempdir().expect("tempdir"); std::fs::write( dir.path().join(".env"), "MALFORMED\nOPENBAO_HOST_PORT=18200\n", ) .expect("write .env"); - assert_eq!(resolve_openbao_host_port(dir.path()), 18200); + assert_eq!(resolve_openbao_host_port_with_env(None, dir.path()), 18200); } } From 913e620146e6f6d55b3f8b4082406d4dc65cb80d Mon Sep 17 00:00:00 2001 From: sehkone Date: Sun, 9 Aug 2026 22:52:56 +0900 Subject: [PATCH 2/4] Keep the compose-file resolver test off the environment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `resolver_reads_the_dotenv_beside_the_given_compose_file` went through `ComposeIdentity::resolve`, which is the composition root that reads `COMPOSE_PROJECT_NAME`. Dropping its `ScopedEnv` left the assertion steered by whatever the invoking shell exported: with the variable set the resolver returns the override and the test fails, which is exactly the ambient dependence the conversion was meant to remove — the E2E harness exports that variable for a whole scenario. `resolve` gets the same override-taking sibling `resolve_for_dir` already has, and the test passes `None` in. Part of #806 --- src/commands/compose_project.rs | 33 +++++++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(-) diff --git a/src/commands/compose_project.rs b/src/commands/compose_project.rs index 91139cfb..031c05b2 100644 --- a/src/commands/compose_project.rs +++ b/src/commands/compose_project.rs @@ -239,7 +239,28 @@ impl ComposeIdentity { instance_name: Option<&str>, messages: &Messages, ) -> Result { - Self::resolve_for_dir(&compose_file_dir(compose_file), instance_name, messages) + Self::resolve_with_override( + compose_file, + instance_name, + compose_project_name_override().as_deref(), + messages, + ) + } + + /// [`ComposeIdentity::resolve`] with the `COMPOSE_PROJECT_NAME` + /// override supplied by the caller. + fn resolve_with_override( + compose_file: &Path, + instance_name: Option<&str>, + project_override: Option<&str>, + messages: &Messages, + ) -> Result { + Self::resolve_for_dir_with_override( + &compose_file_dir(compose_file), + instance_name, + project_override, + messages, + ) } /// The Compose project every invocation is scoped to with `-p`. @@ -496,9 +517,13 @@ mod tests { write_dotenv_with_instance(&here, "here-instance"); write_dotenv_with_instance(&there, "there-instance"); - let identity = - ComposeIdentity::resolve(&there.join("docker-compose.yml"), None, &test_messages()) - .unwrap(); + let identity = ComposeIdentity::resolve_with_override( + &there.join("docker-compose.yml"), + None, + None, + &test_messages(), + ) + .unwrap(); assert_eq!(identity.project(), "there-instance"); } From 4693bb6f9b931576f623b09baf774d797c631fae Mon Sep 17 00:00:00 2001 From: sehkone Date: Sun, 9 Aug 2026 23:02:13 +0900 Subject: [PATCH 3/4] Drop the ca.json fixture the port parameter stranded `resolve_db_admin_dsn` no longer takes a compose directory, so the ca.json the no-source test wrote could not be reached by any code path the test exercises. The file proved nothing and its comment claimed the opposite; what the signature now rules out is stated instead. Also rewords the `dotenv_pairs_to_apply` summary, which read as a typo. Part of #806 --- src/commands/dotenv.rs | 4 ++-- src/commands/rotate/db.rs | 13 ++++--------- 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/src/commands/dotenv.rs b/src/commands/dotenv.rs index 7974e8ac..f1482045 100644 --- a/src/commands/dotenv.rs +++ b/src/commands/dotenv.rs @@ -67,8 +67,8 @@ pub(crate) fn write_dotenv( /// the target environment. /// /// The whole decision, split off from the one mutation it feeds so it -/// can be exercised — the exclusion below above all — without a process -/// environment to steer. +/// can be exercised without a process environment to steer — above all +/// the [`COMPOSE_PROJECT_NAME_ENV`] exclusion documented below. fn dotenv_pairs_to_apply<'a>( entries: &'a BTreeMap, is_set: &dyn Fn(&str) -> bool, diff --git a/src/commands/rotate/db.rs b/src/commands/rotate/db.rs index 68d0c9d1..f09e81e0 100644 --- a/src/commands/rotate/db.rs +++ b/src/commands/rotate/db.rs @@ -287,16 +287,11 @@ mod tests { // DSN; using it as an admin DSN reproduces the original // self-ALTER failure. With no flag and no KV value, fail fast // with a message naming the available recovery paths. + // + // The resolver takes a port rather than a compose directory, so + // there is no longer a path by which a ca.json could be reached + // at all; what stays asserted is the error the operator sees. let messages = test_messages(); - let dir = tempdir().expect("tempdir"); - // Even an existing ca.json must not be silently consumed as an - // admin DSN — the file is left in place to confirm no fallthrough. - let ca_json = dir.path().join("ca.json"); - fs::write( - &ca_json, - r#"{"db":{"type":"postgresql","dataSource":"postgresql://stepca:runtime@postgres:5432/stepca?sslmode=disable"}}"#, - ) - .expect("write ca.json"); let args = RotateDbArgs { admin_dsn: DbAdminDsnArgs { admin_dsn: None }, password: None, From 6eecf1a41b19d7f3a97cea436b2527342a78c5d9 Mon Sep 17 00:00:00 2001 From: sehkone Date: Sun, 9 Aug 2026 23:10:58 +0900 Subject: [PATCH 4/4] Exercise the three ports the parameters only carried MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mutating the production side showed three of the new parameters were threaded but never steered anything: `status_openbao_url_with_env`, `init_args_for_reinit`'s `openbao_host_port_env`, and every field of `HostPortEnv` bar `postgres`. Each could be replaced with `None` and the whole suite still passed, which is the failure mode this work is supposed to avoid — a resolver that no longer reads the environment and no test that proves the caller supplies it instead. Each new assertion now fails when the value does not reach its resolver. Part of #806 --- src/commands/infra.rs | 35 +++++++++++++++++++++++++++++++++++ src/commands/reinit.rs | 11 +++++++++++ src/commands/status.rs | 18 ++++++++++++++++++ 3 files changed, 64 insertions(+) diff --git a/src/commands/infra.rs b/src/commands/infra.rs index 16a489c5..97bfb7ab 100644 --- a/src/commands/infra.rs +++ b/src/commands/infra.rs @@ -2798,6 +2798,41 @@ mod tests { ); } + /// Each field of [`HostPortEnv`] has to reach its own service. The + /// four resolvers differ only in the `(key, default)` pair they + /// carry, so a field wired to the wrong one would still produce a + /// plausible port and no other assertion here would notice. + #[test] + fn host_ports_resolve_routes_each_env_field_to_its_own_service() { + let dir = tempfile::tempdir().unwrap(); + // Every service also has a `.env` value, so a field that failed + // to reach its resolver would fall through to a different number + // rather than silently matching. + std::fs::write( + dir.path().join(".env"), + "POSTGRES_HOST_PORT=15400\nOPENBAO_HOST_PORT=18400\n\ + STEPCA_HOST_PORT=19400\nHTTP01_ADMIN_HOST_PORT=18500\n", + ) + .unwrap(); + let args = install_args(dir.path().join("docker-compose.yml")); + let env = HostPortEnv { + postgres: Some("15432".to_string()), + openbao: Some("18200".to_string()), + stepca: Some("19000".to_string()), + http01_admin: Some("18080".to_string()), + }; + let ports = HostPorts::resolve_with_env(&args, dir.path(), &env); + assert_eq!( + ( + ports.postgres, + ports.openbao, + ports.stepca, + ports.http01_admin + ), + (15432, 18200, 19000, 18080) + ); + } + /// Closes #731: only the flags the operator actually supplied are /// propagated to `.env` and to the compose subprocess environment. #[test] diff --git a/src/commands/reinit.rs b/src/commands/reinit.rs index acc79f0d..4a6f20e7 100644 --- a/src/commands/reinit.rs +++ b/src/commands/reinit.rs @@ -2292,6 +2292,17 @@ mod tests { None, ); assert_eq!(init_args.openbao.openbao_url, "http://localhost:18200"); + + // Step 1 of the same precedence: what the invoking environment + // held outranks the compose `.env`, so a reinit run from a shell + // scoped to one instance does not re-init another's `OpenBao`. + let from_env = init_args_for_reinit( + &reinit_args, + &DeploymentIntent::default(), + &reinit_args.secrets_dir.secrets_dir, + Some("18201"), + ); + assert_eq!(from_env.openbao.openbao_url, "http://localhost:18201"); } /// Closes #731: a snapshotted non-loopback bind intent outranks the diff --git a/src/commands/status.rs b/src/commands/status.rs index c6c345c9..dbaf7805 100644 --- a/src/commands/status.rs +++ b/src/commands/status.rs @@ -425,6 +425,24 @@ mod tests { ); } + /// The first step of the `${OPENBAO_HOST_PORT:-8200}` precedence: + /// what the invoking environment held outranks the compose `.env`, + /// so `status` reaches the instance an operator scoped the shell to + /// rather than the one the directory records. + #[test] + fn status_openbao_url_prefers_the_environment_over_the_env_file() { + let dir = tempfile::tempdir().expect("tempdir"); + std::fs::write(dir.path().join(".env"), "OPENBAO_HOST_PORT=18200\n").expect("write .env"); + let args = status_args( + dir.path().join("docker-compose.yml"), + crate::commands::init::DEFAULT_OPENBAO_URL, + ); + assert_eq!( + status_openbao_url_with_env(&args, Some("18201")), + "http://localhost:18201" + ); + } + /// Closes #731: an operator-supplied `--openbao-url` is used /// verbatim, whatever the configured host port is. #[test]