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..031c05b2 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)?, }) } @@ -207,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`. @@ -306,78 +359,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 +426,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 +466,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 +493,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 +509,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"); @@ -528,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"); } @@ -538,17 +531,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 +556,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 +832,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 +871,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..f1482045 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 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, +) -> 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..97bfb7ab 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, @@ -2757,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/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..4a6f20e7 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,8 +2289,20 @@ mod tests { &reinit_args, &DeploymentIntent::default(), &reinit_args.secrets_dir.secrets_dir, + 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 @@ -2223,7 +2311,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 +2339,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 +2400,7 @@ mod tests { &reinit_args, &DeploymentIntent::default(), &reinit_args.secrets_dir.secrets_dir, + None, ); assert_eq!( init_args.db_dsn.as_deref(), @@ -2351,6 +2440,7 @@ mod tests { &reinit_args, &DeploymentIntent::default(), &reinit_args.secrets_dir.secrets_dir, + None, ); assert!( init_args.db_dsn.is_none(), @@ -2417,6 +2507,7 @@ mod tests { &reinit_args, &DeploymentIntent::default(), &reinit_args.secrets_dir.secrets_dir, + None, ); assert_eq!( init_args.stepca_provisioner, @@ -2462,6 +2553,7 @@ mod tests { &reinit_args, &DeploymentIntent::default(), &reinit_args.secrets_dir.secrets_dir, + None, ); assert_eq!( init_args.stepca_provisioner, @@ -2628,7 +2720,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..f09e81e0 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,24 +287,17 @@ 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", ""); + // + // 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, 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 +314,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 +352,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..dbaf7805 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,51 @@ 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" + ); + } + + /// 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] 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); } }