From e11e9136b0c59598b37ca22ef6b74d977bd17ba4 Mon Sep 17 00:00:00 2001 From: sehkone Date: Mon, 10 Aug 2026 09:38:04 +0900 Subject: [PATCH 1/3] Convert the rotate tests to the injected docker executable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The PATH-faking tests under src/commands/rotate/ substituted a fake docker by writing a script into a tempdir and prepending that directory to the process PATH. In Rust 2024 env::set_var is unsafe, and the shared Mutex those tests held serialised only the code that took it — a tokio runtime, an HTTP client and an env-filtered tracing subscriber read the environment from threads that hold nothing. Every one of them now hands production the fake through the docker seam the tree already carries, so nothing process-global is touched: approle sets it on the context make_ctx returns, helpers and stepca_password replace their Path::new("docker") literal, and infra_cert's three end-to-end tests drive a new run_rotate_with_exec. run_rotate is that function with DOCKER_BIN supplied, unchanged in signature and behaviour, so no rotate flow issues a different docker command than before. The self-contained fake infra_cert kept privately is promoted into rotate::test_support with both of its defects fixed. Its log encoding is framed rather than delimited — an argument count followed by that many NUL-terminated fields, appended per invocation — so an assertion can tell ["a b"] from ["a", "b"], an empty argument survives the round trip, and a multi-call flow keeps every call in order instead of only the last. It also quotes a log path containing an apostrophe rather than asserting one cannot appear, since tempdir() may legitimately hand one back. container_signal_addresses_the_named_container is removed rather than converted: converted it would assert the same argv against the same function as container_signal_runs_the_supplied_executable, differing only in the fake being removed here. test_support keeps everything the init tree still imports. Only TEST_DOCKER_EXIT_ENV goes, retired by the baked-in exit code, and with it the eleven await_holding_lock allows whose stated justification the departing lock took with it. Closes #808 --- src/commands/rotate.rs | 240 ++++++++++++++++++++++++- src/commands/rotate/approle.rs | 190 ++++++-------------- src/commands/rotate/helpers.rs | 48 ++--- src/commands/rotate/infra_cert.rs | 156 +++++----------- src/commands/rotate/stepca_password.rs | 36 ++-- 5 files changed, 365 insertions(+), 305 deletions(-) diff --git a/src/commands/rotate.rs b/src/commands/rotate.rs index 132a43ac..38a27f9e 100644 --- a/src/commands/rotate.rs +++ b/src/commands/rotate.rs @@ -128,14 +128,28 @@ pub(super) struct RotateContext { pub(super) state_file: PathBuf, /// The `docker` executable every spawn in this rotation runs. /// - /// Set once in [`run_rotate`] and only read afterwards, so a test - /// that builds a context can point the whole tree at a fake without - /// touching `PATH`. + /// Set once in [`run_rotate_with_exec`] and only read afterwards, so + /// a test that builds a context — or drives that entry point — can + /// point the whole tree at a fake without touching `PATH`. pub(super) docker: PathBuf, } -#[allow(clippy::too_many_lines)] pub(crate) async fn run_rotate(args: &RotateArgs, messages: &Messages) -> Result { + run_rotate_with_exec(args, Path::new(DOCKER_BIN), messages).await +} + +/// [`run_rotate`] with the `docker` executable supplied by the caller. +/// +/// Mirrors the `_with_exec` pairs in [`crate::commands::infra`]: the +/// wrapper above is this function with [`DOCKER_BIN`], and a test +/// drives the whole entry point — state-file handling and strategy +/// normalisation included — against a fake it names by path. +#[allow(clippy::too_many_lines)] +async fn run_rotate_with_exec( + args: &RotateArgs, + docker: &Path, + messages: &Messages, +) -> Result { let state_path = args .state_file .clone() @@ -173,7 +187,7 @@ pub(crate) async fn run_rotate(args: &RotateArgs, messages: &Messages) -> Result paths, state_dir, state_file: state_path, - docker: PathBuf::from(DOCKER_BIN), + docker: docker.to_path_buf(), }; // InfraCert operates on local files and Docker only — it must not @@ -282,7 +296,6 @@ pub(super) mod test_support { static ENV_LOCK: LazyLock> = LazyLock::new(|| Mutex::new(())); pub(in crate::commands) const TEST_DOCKER_ARGS_ENV: &str = "BOOTROOT_TEST_DOCKER_ARGS"; - pub(super) const TEST_DOCKER_EXIT_ENV: &str = "BOOTROOT_TEST_DOCKER_EXIT"; pub(in crate::commands) struct ScopedEnvVar { key: &'static str, @@ -347,4 +360,219 @@ exit 0 } env::join_paths(paths).expect("PATH components should be valid") } + + /// Writes a fake `docker` at `path` that appends one record per + /// invocation to `args_log` and reads nothing from its environment. + /// + /// The log path is baked into the script text as it is written, so a + /// test handing this executable to production through the `docker` + /// seam gets its argv back without setting a single variable on this + /// process — which is the point, since the test does not construct + /// the `Command` that runs the fake. + pub(super) fn write_self_contained_fake_docker(path: &Path, args_log: &Path) { + write_self_contained_fake_docker_exiting(path, args_log, 0); + } + + /// [`write_self_contained_fake_docker`] whose fake exits `exit_code` + /// after logging, so a test can steer the failure path of a docker + /// call it does not spawn itself. + /// + /// Each invocation appends its argument count and then exactly that + /// many arguments, every field NUL-terminated: `docker restart c` + /// appends `2\0restart\0c\0` and `docker a '' b` appends + /// `3\0a\0\0b\0`. The record is framed by its count rather than + /// delimited by a byte, so an empty argument stays an empty field + /// and two invocations can never merge into one. + /// [`decode_fake_docker_log`] reads it back. + pub(super) fn write_self_contained_fake_docker_exiting( + path: &Path, + args_log: &Path, + exit_code: u8, + ) { + use std::os::unix::fs::PermissionsExt; + + let log = shell_single_quote(&args_log.display().to_string()); + let script = format!( + "#!/bin/sh\nset -eu\nprintf '%s\\0' \"$#\" \"$@\" >> {log}\nexit {exit_code}\n" + ); + fs::write(path, script).expect("fake docker script should be written"); + fs::set_permissions(path, fs::Permissions::from_mode(0o700)) + .expect("fake docker script should be executable"); + } + + /// Quotes `value` as a single POSIX shell word. + /// + /// A tempdir path may legally contain `'`, which ends the quoted + /// word; the usual `'\''` dance closes, escapes and reopens it. + fn shell_single_quote(value: &str) -> String { + format!("'{}'", value.replace('\'', r"'\''")) + } + + /// Decodes a log written by [`write_self_contained_fake_docker`], + /// returning one argument vector per invocation in call order. + /// + /// # Panics + /// + /// Panics if the log is unreadable or is not the framing the fake + /// writes — a missing count, or a record the file ends inside. + pub(super) fn decode_fake_docker_log(args_log: &Path) -> Vec> { + let bytes = fs::read(args_log).expect("fake docker log should be readable"); + if bytes.is_empty() { + return Vec::new(); + } + // Every field is NUL-terminated, so dropping the final + // terminator leaves the fields themselves — including an empty + // argument, which a terminator-less split would swallow. + let body = bytes + .strip_suffix(&[0]) + .expect("the fake terminates every field it writes"); + let mut fields = body.split(|byte| *byte == 0); + let mut invocations = Vec::new(); + while let Some(count_field) = fields.next() { + let count: usize = std::str::from_utf8(count_field) + .expect("argument count must be UTF-8") + .parse() + .expect("argument count must be a decimal number"); + let argv: Vec = fields + .by_ref() + .take(count) + .map(|field| String::from_utf8_lossy(field).into_owned()) + .collect(); + assert_eq!(argv.len(), count, "the log ends inside a record"); + invocations.push(argv); + } + invocations + } +} + +#[cfg(test)] +mod tests { + use std::fs; + use std::path::Path; + use std::process::Command; + + use tempfile::tempdir; + + use super::test_support::{ + decode_fake_docker_log, write_self_contained_fake_docker, + write_self_contained_fake_docker_exiting, + }; + + /// Runs the fake once with `args`, the way production would. + fn run_fake(fake: &Path, args: &[&str]) { + let status = Command::new(fake) + .args(args) + .status() + .expect("the fake docker must be spawnable"); + assert!(status.success(), "the fake docker must exit 0"); + } + + /// A space-joined encoding cannot tell one argument holding a space + /// from two arguments, which is exactly what the `--user :` + /// assertion in `stepca_password` rests on. + #[test] + fn the_fake_docker_log_keeps_argument_boundaries() { + let dir = tempdir().expect("tempdir"); + let split_log = dir.path().join("split.log"); + let joined_log = dir.path().join("joined.log"); + let split = dir.path().join("split-docker"); + let joined = dir.path().join("joined-docker"); + write_self_contained_fake_docker(&split, &split_log); + write_self_contained_fake_docker(&joined, &joined_log); + + run_fake(&split, &["--user", "1000:1000"]); + run_fake(&joined, &["--user 1000:1000"]); + + assert_eq!( + decode_fake_docker_log(&split_log), + [["--user", "1000:1000"]] + ); + assert_eq!(decode_fake_docker_log(&joined_log), [["--user 1000:1000"]]); + assert_ne!( + decode_fake_docker_log(&split_log), + decode_fake_docker_log(&joined_log) + ); + } + + /// A multi-call flow must decode as its own invocations, in order, + /// rather than as the last call or one merged record. + #[test] + fn the_fake_docker_log_keeps_every_invocation_in_order() { + let dir = tempdir().expect("tempdir"); + let args_log = dir.path().join("docker_args.log"); + let fake = dir.path().join("fake-docker"); + write_self_contained_fake_docker(&fake, &args_log); + + run_fake(&fake, &["run", "first"]); + run_fake(&fake, &["kill", "-s", "SIGHUP", "c"]); + run_fake(&fake, &["restart", "c"]); + + assert_eq!( + decode_fake_docker_log(&args_log), + vec![ + vec!["run", "first"], + vec!["kill", "-s", "SIGHUP", "c"], + vec!["restart", "c"], + ] + ); + } + + /// The pair a doubled-NUL record terminator collapses: `["a", "", + /// "b"]` once and `["a"]` then `["b"]` encode to the same bytes + /// under that scheme, and must not under this one. + #[test] + fn the_fake_docker_log_frames_empty_arguments() { + let dir = tempdir().expect("tempdir"); + let single_log = dir.path().join("single.log"); + let pair_log = dir.path().join("pair.log"); + let single = dir.path().join("single-docker"); + let pair = dir.path().join("pair-docker"); + write_self_contained_fake_docker(&single, &single_log); + write_self_contained_fake_docker(&pair, &pair_log); + + run_fake(&single, &["a", "", "b"]); + run_fake(&pair, &["a"]); + run_fake(&pair, &["b"]); + + assert_eq!(decode_fake_docker_log(&single_log), [["a", "", "b"]]); + assert_eq!(decode_fake_docker_log(&pair_log), [["a"], ["b"]]); + assert_ne!( + decode_fake_docker_log(&single_log), + decode_fake_docker_log(&pair_log) + ); + } + + /// `tempfile::tempdir()` can legitimately hand back a path holding + /// an apostrophe, so the helper quotes rather than rejects one. + #[test] + fn the_fake_docker_handles_a_quoted_log_path() { + let dir = tempdir().expect("tempdir"); + let awkward = dir.path().join("it's a dir"); + fs::create_dir(&awkward).expect("create awkward dir"); + let args_log = awkward.join("docker args.log"); + let fake = dir.path().join("fake-docker"); + write_self_contained_fake_docker(&fake, &args_log); + + run_fake(&fake, &["restart", "c"]); + + assert_eq!(decode_fake_docker_log(&args_log), [["restart", "c"]]); + } + + /// The baked-in exit code is what replaces the environment variable + /// the shared `PATH` fake reads. + #[test] + fn the_fake_docker_reports_the_baked_in_exit_code() { + let dir = tempdir().expect("tempdir"); + let args_log = dir.path().join("docker_args.log"); + let fake = dir.path().join("fake-docker"); + write_self_contained_fake_docker_exiting(&fake, &args_log, 7); + + let status = Command::new(&fake) + .arg("run") + .status() + .expect("the fake docker must be spawnable"); + + assert_eq!(status.code(), Some(7)); + assert_eq!(decode_fake_docker_log(&args_log), [["run"]]); + } } diff --git a/src/commands/rotate/approle.rs b/src/commands/rotate/approle.rs index bfb0b63f..0538eef9 100644 --- a/src/commands/rotate/approle.rs +++ b/src/commands/rotate/approle.rs @@ -790,8 +790,7 @@ mod tests { use wiremock::{Mock, MockServer, ResponseTemplate}; use super::super::test_support::{ - ScopedEnvVar, TEST_DOCKER_ARGS_ENV, env_lock, path_with_prepend, test_messages, - write_fake_docker_script, + decode_fake_docker_log, test_messages, write_self_contained_fake_docker, }; use super::*; use crate::commands::compose_project::DOCKER_BIN; @@ -802,7 +801,11 @@ mod tests { /// Builds a rotate context whose compose directory records a /// non-default identity, so every by-name docker call these tests /// observe has to be `insight-*` rather than `bootroot-*`. - fn make_ctx(dir: &std::path::Path) -> RotateContext { + /// + /// `docker` is the executable every spawn in the rotation runs, so a + /// test that wants the argv — or wants the real `docker` kept out of + /// the run — points this at a fake instead of touching `PATH`. + fn make_ctx(dir: &std::path::Path, docker: &Path) -> RotateContext { fs::write(dir.join(".env"), "BOOTROOT_INSTANCE=insight\n").expect("write .env"); RotateContext { openbao_url: String::new(), @@ -827,7 +830,7 @@ mod tests { paths: super::super::StatePaths::new(dir.join("secrets")), state_dir: dir.to_path_buf(), state_file: dir.join("state.json"), - docker: PathBuf::from(DOCKER_BIN), + docker: docker.to_path_buf(), } } @@ -905,19 +908,12 @@ mod tests { }))) } - // The env-var lock must be held across the `.await` to prevent - // parallel tests from seeing a corrupted PATH. - #[allow(clippy::await_holding_lock)] #[tokio::test] async fn rotate_infra_writes_secret_id_restarts_agent_and_verifies_login() { let dir = tempdir().expect("tempdir"); - let bin_dir = dir.path().join("bin"); - fs::create_dir(&bin_dir).expect("create bin dir"); - write_fake_docker_script(&bin_dir.join("docker")); + let fake_docker = dir.path().join("fake-docker"); let args_log = dir.path().join("docker_args.log"); - let _lock = env_lock(); - let _path = ScopedEnvVar::set("PATH", path_with_prepend(&bin_dir)); - let _args = ScopedEnvVar::set(TEST_DOCKER_ARGS_ENV, args_log.as_os_str()); + write_self_contained_fake_docker(&fake_docker, &args_log); let server = MockServer::start().await; mount_secret_id_mock(APPROLE_BOOTROOT_STEPCA) @@ -926,7 +922,7 @@ mod tests { .await; mount_login_mock().expect(1).mount(&server).await; - let mut ctx = make_ctx(dir.path()); + let mut ctx = make_ctx(dir.path(), &fake_docker); let stepca_dir = ctx .paths .secrets_dir() @@ -967,24 +963,18 @@ mod tests { & 0o777; assert_eq!(mode, 0o600); } - let logged = fs::read_to_string(&args_log).expect("read docker args"); - let args: Vec<&str> = logged.lines().collect(); - assert_eq!(args, vec!["restart", "insight-openbao-agent-stepca"]); + assert_eq!( + decode_fake_docker_log(&args_log), + [["restart", "insight-openbao-agent-stepca"]] + ); } - // The env-var lock must be held across the `.await` to prevent - // parallel tests from seeing a corrupted PATH. - #[allow(clippy::await_holding_lock)] #[tokio::test] async fn rotate_infra_backfills_missing_role_id_file() { let dir = tempdir().expect("tempdir"); - let bin_dir = dir.path().join("bin"); - fs::create_dir(&bin_dir).expect("create bin dir"); - write_fake_docker_script(&bin_dir.join("docker")); + let fake_docker = dir.path().join("fake-docker"); let args_log = dir.path().join("docker_args.log"); - let _lock = env_lock(); - let _path = ScopedEnvVar::set("PATH", path_with_prepend(&bin_dir)); - let _args = ScopedEnvVar::set(TEST_DOCKER_ARGS_ENV, args_log.as_os_str()); + write_self_contained_fake_docker(&fake_docker, &args_log); let server = MockServer::start().await; Mock::given(method("GET")) @@ -1002,7 +992,7 @@ mod tests { .await; mount_login_mock().mount(&server).await; - let mut ctx = make_ctx(dir.path()); + let mut ctx = make_ctx(dir.path(), &fake_docker); let mut client = OpenBaoClient::new(&server.uri()).expect("client"); client.set_token("scoped-token".to_string()); let messages = test_messages(); @@ -1026,24 +1016,18 @@ mod tests { let role_id = fs::read_to_string(responder_dir.join(OPENBAO_AGENT_ROLE_ID_NAME)) .expect("role_id backfilled"); assert_eq!(role_id, "responder-role-id"); - let logged = fs::read_to_string(&args_log).expect("read docker args"); - let args: Vec<&str> = logged.lines().collect(); - assert_eq!(args, vec!["restart", "insight-openbao-agent-responder"]); + assert_eq!( + decode_fake_docker_log(&args_log), + [["restart", "insight-openbao-agent-responder"]] + ); } - // The env-var lock must be held across the `.await` to prevent - // parallel tests from seeing a corrupted PATH. - #[allow(clippy::await_holding_lock)] #[tokio::test] async fn rotate_infra_permission_denied_hints_at_infra_credential() { let dir = tempdir().expect("tempdir"); - let bin_dir = dir.path().join("bin"); - fs::create_dir(&bin_dir).expect("create bin dir"); - write_fake_docker_script(&bin_dir.join("docker")); + let fake_docker = dir.path().join("fake-docker"); let args_log = dir.path().join("docker_args.log"); - let _lock = env_lock(); - let _path = ScopedEnvVar::set("PATH", path_with_prepend(&bin_dir)); - let _args = ScopedEnvVar::set(TEST_DOCKER_ARGS_ENV, args_log.as_os_str()); + write_self_contained_fake_docker(&fake_docker, &args_log); let server = MockServer::start().await; Mock::given(method("POST")) @@ -1056,7 +1040,7 @@ mod tests { .mount(&server) .await; - let mut ctx = make_ctx(dir.path()); + let mut ctx = make_ctx(dir.path(), &fake_docker); let stepca_dir = ctx .paths .secrets_dir() @@ -1109,7 +1093,7 @@ mod tests { let server = MockServer::start().await; mount_provisioning_mocks(&server).await; - let mut ctx = make_ctx(dir.path()); + let mut ctx = make_ctx(dir.path(), Path::new(DOCKER_BIN)); let mut client = OpenBaoClient::new(&server.uri()).expect("client"); client.set_token("root-token".to_string()); let messages = test_messages(); @@ -1174,7 +1158,7 @@ mod tests { let server = MockServer::start().await; mount_provisioning_mocks(&server).await; - let mut ctx = make_ctx(dir.path()); + let mut ctx = make_ctx(dir.path(), Path::new(DOCKER_BIN)); let label = AppRoleLabel::InfraRotate.to_string(); ctx.state .approles @@ -1423,7 +1407,7 @@ mod tests { #[tokio::test] async fn rotate_all_services_empty_registry_is_noop_success() { let dir = tempdir().expect("tempdir"); - let ctx = make_ctx(dir.path()); + let ctx = make_ctx(dir.path(), Path::new(DOCKER_BIN)); // No OpenBao requests may happen; an unroutable URL makes any // accidental call fail loudly. let mut client = OpenBaoClient::new("http://127.0.0.1:1").expect("client"); @@ -1434,19 +1418,12 @@ mod tests { .expect("an empty service registry must be a no-op success"); } - // The env-var lock must be held across the `.await` to prevent - // parallel tests from seeing a corrupted PATH. - #[allow(clippy::await_holding_lock)] #[tokio::test] async fn rotate_all_services_rotates_local_and_remote_targets() { let dir = tempdir().expect("tempdir"); - let bin_dir = dir.path().join("bin"); - fs::create_dir(&bin_dir).expect("create bin dir"); - write_fake_docker_script(&bin_dir.join("docker")); + let fake_docker = dir.path().join("fake-docker"); let args_log = dir.path().join("docker_args.log"); - let _lock = env_lock(); - let _path = ScopedEnvVar::set("PATH", path_with_prepend(&bin_dir)); - let _args = ScopedEnvVar::set(TEST_DOCKER_ARGS_ENV, args_log.as_os_str()); + write_self_contained_fake_docker(&fake_docker, &args_log); let server = MockServer::start().await; mount_secret_id_mock(&service_role_name("alpha")) @@ -1467,7 +1444,7 @@ mod tests { .mount(&server) .await; - let mut ctx = make_ctx(dir.path()); + let mut ctx = make_ctx(dir.path(), &fake_docker); insert_local_service(&mut ctx, dir.path(), "alpha"); ctx.state.services.insert( "beta".to_string(), @@ -1495,19 +1472,12 @@ mod tests { ); } - // The env-var lock must be held across the `.await` to prevent - // parallel tests from seeing a corrupted PATH. - #[allow(clippy::await_holding_lock)] #[tokio::test] async fn rotate_all_services_continues_after_per_target_failure() { let dir = tempdir().expect("tempdir"); - let bin_dir = dir.path().join("bin"); - fs::create_dir(&bin_dir).expect("create bin dir"); - write_fake_docker_script(&bin_dir.join("docker")); + let fake_docker = dir.path().join("fake-docker"); let args_log = dir.path().join("docker_args.log"); - let _lock = env_lock(); - let _path = ScopedEnvVar::set("PATH", path_with_prepend(&bin_dir)); - let _args = ScopedEnvVar::set(TEST_DOCKER_ARGS_ENV, args_log.as_os_str()); + write_self_contained_fake_docker(&fake_docker, &args_log); let server = MockServer::start().await; Mock::given(method("POST")) @@ -1527,7 +1497,7 @@ mod tests { .await; mount_login_mock().expect(1).mount(&server).await; - let mut ctx = make_ctx(dir.path()); + let mut ctx = make_ctx(dir.path(), &fake_docker); insert_local_service(&mut ctx, dir.path(), "alpha"); insert_local_service(&mut ctx, dir.path(), "beta"); @@ -1620,19 +1590,12 @@ mod tests { .respond_with(response) } - // The env-var lock must be held across the `.await` to prevent - // parallel tests from seeing a corrupted PATH. - #[allow(clippy::await_holding_lock)] #[tokio::test] async fn self_mint_replaces_credential_file_after_successful_run() { let dir = tempdir().expect("tempdir"); - let bin_dir = dir.path().join("bin"); - fs::create_dir(&bin_dir).expect("create bin dir"); - write_fake_docker_script(&bin_dir.join("docker")); + let fake_docker = dir.path().join("fake-docker"); let args_log = dir.path().join("docker_args.log"); - let _lock = env_lock(); - let _path = ScopedEnvVar::set("PATH", path_with_prepend(&bin_dir)); - let _args = ScopedEnvVar::set(TEST_DOCKER_ARGS_ENV, args_log.as_os_str()); + write_self_contained_fake_docker(&fake_docker, &args_log); let server = MockServer::start().await; mount_secret_id_mock(&service_role_name("alpha")) @@ -1652,7 +1615,7 @@ mod tests { .mount(&server) .await; - let mut ctx = make_ctx(dir.path()); + let mut ctx = make_ctx(dir.path(), &fake_docker); insert_local_service(&mut ctx, dir.path(), "alpha"); let credential_path = dir.path().join("rotate-cred").join("secret_id"); fs::create_dir_all(credential_path.parent().expect("parent")).expect("create cred dir"); @@ -1699,19 +1662,12 @@ mod tests { ); } - // The env-var lock must be held across the `.await` to prevent - // parallel tests from seeing a corrupted PATH. - #[allow(clippy::await_holding_lock)] #[tokio::test] async fn self_mint_skipped_with_warning_when_auth_not_file_based() { let dir = tempdir().expect("tempdir"); - let bin_dir = dir.path().join("bin"); - fs::create_dir(&bin_dir).expect("create bin dir"); - write_fake_docker_script(&bin_dir.join("docker")); + let fake_docker = dir.path().join("fake-docker"); let args_log = dir.path().join("docker_args.log"); - let _lock = env_lock(); - let _path = ScopedEnvVar::set("PATH", path_with_prepend(&bin_dir)); - let _args = ScopedEnvVar::set(TEST_DOCKER_ARGS_ENV, args_log.as_os_str()); + write_self_contained_fake_docker(&fake_docker, &args_log); let server = MockServer::start().await; mount_secret_id_mock(&service_role_name("alpha")) @@ -1726,7 +1682,7 @@ mod tests { .mount(&server) .await; - let mut ctx = make_ctx(dir.path()); + let mut ctx = make_ctx(dir.path(), &fake_docker); insert_local_service(&mut ctx, dir.path(), "alpha"); let mut client = OpenBaoClient::new(&server.uri()).expect("client"); client.set_token("runtime-rotate-token".to_string()); @@ -1753,19 +1709,12 @@ mod tests { ); } - // The env-var lock must be held across the `.await` to prevent - // parallel tests from seeing a corrupted PATH. - #[allow(clippy::await_holding_lock)] #[tokio::test] async fn no_self_mint_under_root_auth() { let dir = tempdir().expect("tempdir"); - let bin_dir = dir.path().join("bin"); - fs::create_dir(&bin_dir).expect("create bin dir"); - write_fake_docker_script(&bin_dir.join("docker")); + let fake_docker = dir.path().join("fake-docker"); let args_log = dir.path().join("docker_args.log"); - let _lock = env_lock(); - let _path = ScopedEnvVar::set("PATH", path_with_prepend(&bin_dir)); - let _args = ScopedEnvVar::set(TEST_DOCKER_ARGS_ENV, args_log.as_os_str()); + write_self_contained_fake_docker(&fake_docker, &args_log); let server = MockServer::start().await; mount_secret_id_mock(&service_role_name("alpha")) @@ -1778,7 +1727,7 @@ mod tests { .mount(&server) .await; - let mut ctx = make_ctx(dir.path()); + let mut ctx = make_ctx(dir.path(), &fake_docker); insert_local_service(&mut ctx, dir.path(), "alpha"); let mut client = OpenBaoClient::new(&server.uri()).expect("client"); client.set_token("root-token".to_string()); @@ -1826,7 +1775,7 @@ mod tests { .mount(&server) .await; - let mut ctx = make_ctx(dir.path()); + let mut ctx = make_ctx(dir.path(), Path::new(DOCKER_BIN)); insert_local_service(&mut ctx, dir.path(), "alpha"); let credential_path = dir.path().join("rotate-cred").join("secret_id"); fs::create_dir_all(credential_path.parent().expect("parent")).expect("create cred dir"); @@ -1872,19 +1821,12 @@ mod tests { // fails its login verification must not replace the working file — // the old secret_id stays valid until TTL, so the next run // self-heals. - // The env-var lock must be held across the `.await` to prevent - // parallel tests from seeing a corrupted PATH. - #[allow(clippy::await_holding_lock)] #[tokio::test] async fn self_mint_verify_failure_keeps_old_credential_file() { let dir = tempdir().expect("tempdir"); - let bin_dir = dir.path().join("bin"); - fs::create_dir(&bin_dir).expect("create bin dir"); - write_fake_docker_script(&bin_dir.join("docker")); + let fake_docker = dir.path().join("fake-docker"); let args_log = dir.path().join("docker_args.log"); - let _lock = env_lock(); - let _path = ScopedEnvVar::set("PATH", path_with_prepend(&bin_dir)); - let _args = ScopedEnvVar::set(TEST_DOCKER_ARGS_ENV, args_log.as_os_str()); + write_self_contained_fake_docker(&fake_docker, &args_log); let server = MockServer::start().await; mount_secret_id_mock(&service_role_name("alpha")) @@ -1904,7 +1846,7 @@ mod tests { .mount(&server) .await; - let mut ctx = make_ctx(dir.path()); + let mut ctx = make_ctx(dir.path(), &fake_docker); insert_local_service(&mut ctx, dir.path(), "alpha"); let credential_path = dir.path().join("rotate-cred").join("secret_id"); fs::create_dir_all(credential_path.parent().expect("parent")).expect("create cred dir"); @@ -1951,19 +1893,12 @@ mod tests { ); } - // The env-var lock must be held across the `.await` to prevent - // parallel tests from seeing a corrupted PATH. - #[allow(clippy::await_holding_lock)] #[tokio::test] async fn self_mint_applies_recorded_cidr_binding() { let dir = tempdir().expect("tempdir"); - let bin_dir = dir.path().join("bin"); - fs::create_dir(&bin_dir).expect("create bin dir"); - write_fake_docker_script(&bin_dir.join("docker")); + let fake_docker = dir.path().join("fake-docker"); let args_log = dir.path().join("docker_args.log"); - let _lock = env_lock(); - let _path = ScopedEnvVar::set("PATH", path_with_prepend(&bin_dir)); - let _args = ScopedEnvVar::set(TEST_DOCKER_ARGS_ENV, args_log.as_os_str()); + write_self_contained_fake_docker(&fake_docker, &args_log); let server = MockServer::start().await; mount_secret_id_mock(&service_role_name("alpha")) @@ -1994,7 +1929,7 @@ mod tests { .mount(&server) .await; - let mut ctx = make_ctx(dir.path()); + let mut ctx = make_ctx(dir.path(), &fake_docker); insert_local_service(&mut ctx, dir.path(), "alpha"); ctx.state.rotate_bound_cidrs.insert( AppRoleLabel::RuntimeRotate.to_string(), @@ -2030,19 +1965,12 @@ mod tests { ); } - // The env-var lock must be held across the `.await` to prevent - // parallel tests from seeing a corrupted PATH. - #[allow(clippy::await_holding_lock)] #[tokio::test] async fn infra_invocation_self_mints_the_infra_rotate_credential() { let dir = tempdir().expect("tempdir"); - let bin_dir = dir.path().join("bin"); - fs::create_dir(&bin_dir).expect("create bin dir"); - write_fake_docker_script(&bin_dir.join("docker")); + let fake_docker = dir.path().join("fake-docker"); let args_log = dir.path().join("docker_args.log"); - let _lock = env_lock(); - let _path = ScopedEnvVar::set("PATH", path_with_prepend(&bin_dir)); - let _args = ScopedEnvVar::set(TEST_DOCKER_ARGS_ENV, args_log.as_os_str()); + write_self_contained_fake_docker(&fake_docker, &args_log); let server = MockServer::start().await; mount_secret_id_mock(APPROLE_BOOTROOT_STEPCA) @@ -2062,7 +1990,7 @@ mod tests { .mount(&server) .await; - let mut ctx = make_ctx(dir.path()); + let mut ctx = make_ctx(dir.path(), &fake_docker); let stepca_dir = ctx .paths .secrets_dir() @@ -2113,7 +2041,7 @@ mod tests { #[tokio::test] async fn rotate_bound_cidrs_rejected_without_root_auth() { let dir = tempdir().expect("tempdir"); - let mut ctx = make_ctx(dir.path()); + let mut ctx = make_ctx(dir.path(), Path::new(DOCKER_BIN)); // No OpenBao requests may happen; an unroutable URL makes any // accidental call fail loudly. let mut client = OpenBaoClient::new("http://127.0.0.1:1").expect("client"); @@ -2179,7 +2107,7 @@ mod tests { .mount(&server) .await; - let mut ctx = make_ctx(dir.path()); + let mut ctx = make_ctx(dir.path(), Path::new(DOCKER_BIN)); let mut client = OpenBaoClient::new(&server.uri()).expect("client"); client.set_token("root-token".to_string()); let messages = test_messages(); @@ -2249,7 +2177,7 @@ mod tests { .mount(&server) .await; - let mut ctx = make_ctx(dir.path()); + let mut ctx = make_ctx(dir.path(), Path::new(DOCKER_BIN)); let label = AppRoleLabel::InfraRotate.to_string(); ctx.state .approles @@ -2316,7 +2244,7 @@ mod tests { .mount(&server) .await; - let mut ctx = make_ctx(dir.path()); + let mut ctx = make_ctx(dir.path(), Path::new(DOCKER_BIN)); let label = AppRoleLabel::InfraRotate.to_string(); ctx.state .approles @@ -2395,7 +2323,7 @@ mod tests { .mount(&server) .await; - let mut ctx = make_ctx(dir.path()); + let mut ctx = make_ctx(dir.path(), Path::new(DOCKER_BIN)); ctx.state.rotate_secret_id_ttl = Some("48h".to_string()); let mut client = OpenBaoClient::new(&server.uri()).expect("client"); client.set_token("root-token".to_string()); @@ -2408,7 +2336,7 @@ mod tests { #[tokio::test] async fn clear_rotate_bound_cidrs_rejected_without_root_auth() { let dir = tempdir().expect("tempdir"); - let mut ctx = make_ctx(dir.path()); + let mut ctx = make_ctx(dir.path(), Path::new(DOCKER_BIN)); // No OpenBao requests may happen; an unroutable URL makes any // accidental call fail loudly. let mut client = OpenBaoClient::new("http://127.0.0.1:1").expect("client"); @@ -2445,7 +2373,7 @@ mod tests { .mount(&server) .await; - let mut ctx = make_ctx(dir.path()); + let mut ctx = make_ctx(dir.path(), Path::new(DOCKER_BIN)); insert_local_service(&mut ctx, dir.path(), "alpha"); let mut client = OpenBaoClient::new(&server.uri()).expect("client"); diff --git a/src/commands/rotate/helpers.rs b/src/commands/rotate/helpers.rs index 4239be49..4a872ccc 100644 --- a/src/commands/rotate/helpers.rs +++ b/src/commands/rotate/helpers.rs @@ -204,23 +204,18 @@ mod tests { use tempfile::tempdir; use super::super::test_support::{ - ScopedEnvVar, TEST_DOCKER_ARGS_ENV, env_lock, path_with_prepend, test_messages, - write_fake_docker_script, + decode_fake_docker_log, test_messages, write_self_contained_fake_docker, }; use super::*; use crate::commands::compose_project::DEFAULT_INSTANCE_NAME; - /// Runs `restart_openbao_agent` against a fake `docker` on `PATH` - /// and returns the argument vector it was given. - fn restart_args(instance: &str, container: BootrootContainer) -> Vec { + /// Runs `restart_openbao_agent` against a fake `docker` handed in + /// through the executable seam and returns the invocations it saw. + fn restart_invocations(instance: &str, container: BootrootContainer) -> Vec> { let dir = tempdir().expect("tempdir"); - let bin_dir = dir.path().join("bin"); - std::fs::create_dir(&bin_dir).expect("create bin dir"); - write_fake_docker_script(&bin_dir.join("docker")); + let fake = dir.path().join("fake-docker"); let args_log = dir.path().join("docker_args.log"); - let _lock = env_lock(); - let _path = ScopedEnvVar::set("PATH", path_with_prepend(&bin_dir)); - let _args = ScopedEnvVar::set(TEST_DOCKER_ARGS_ENV, args_log.as_os_str()); + write_self_contained_fake_docker(&fake, &args_log); std::fs::write( dir.path().join(".env"), @@ -230,16 +225,12 @@ mod tests { restart_openbao_agent( &dir.path().join("docker-compose.yml"), container, - Path::new("docker"), + &fake, &test_messages(), ) .expect("restarting the sidecar must succeed"); - std::fs::read_to_string(&args_log) - .expect("read docker args") - .lines() - .map(str::to_string) - .collect() + decode_fake_docker_log(&args_log) } /// The sidecars bypass Compose's project scoping, so `rotate db` and @@ -250,8 +241,8 @@ mod tests { #[test] fn restarting_the_stepca_agent_names_the_recorded_instance() { assert_eq!( - restart_args("insight", BootrootContainer::OpenBaoAgentStepCa), - vec!["restart", "insight-openbao-agent-stepca"] + restart_invocations("insight", BootrootContainer::OpenBaoAgentStepCa), + [["restart", "insight-openbao-agent-stepca"]] ); } @@ -260,8 +251,8 @@ mod tests { #[test] fn restarting_the_responder_agent_names_the_recorded_instance() { assert_eq!( - restart_args("insight", BootrootContainer::OpenBaoAgentResponder), - vec!["restart", "insight-openbao-agent-responder"] + restart_invocations("insight", BootrootContainer::OpenBaoAgentResponder), + [["restart", "insight-openbao-agent-responder"]] ); } @@ -271,18 +262,14 @@ mod tests { #[test] fn restarting_a_sidecar_without_a_recorded_identity_keeps_the_default_name() { let dir = tempdir().expect("tempdir"); - let bin_dir = dir.path().join("bin"); - std::fs::create_dir(&bin_dir).expect("create bin dir"); - write_fake_docker_script(&bin_dir.join("docker")); + let fake = dir.path().join("fake-docker"); let args_log = dir.path().join("docker_args.log"); - let _lock = env_lock(); - let _path = ScopedEnvVar::set("PATH", path_with_prepend(&bin_dir)); - let _args = ScopedEnvVar::set(TEST_DOCKER_ARGS_ENV, args_log.as_os_str()); + write_self_contained_fake_docker(&fake, &args_log); restart_openbao_agent( &dir.path().join("docker-compose.yml"), BootrootContainer::OpenBaoAgentStepCa, - Path::new("docker"), + &fake, &test_messages(), ) .expect("restarting the sidecar must succeed"); @@ -291,10 +278,9 @@ mod tests { // bare default-instance sidecar name in `src/` is what // `container_name`'s single-declaration guard forbids. let expected = BootrootContainer::OpenBaoAgentStepCa.name(DEFAULT_INSTANCE_NAME); - let logged = std::fs::read_to_string(&args_log).expect("read docker args"); assert_eq!( - logged.lines().collect::>(), - vec!["restart", expected.as_str()] + decode_fake_docker_log(&args_log), + [["restart", expected.as_str()]] ); } diff --git a/src/commands/rotate/infra_cert.rs b/src/commands/rotate/infra_cert.rs index 5e4cc70b..c47c4313 100644 --- a/src/commands/rotate/infra_cert.rs +++ b/src/commands/rotate/infra_cert.rs @@ -440,8 +440,7 @@ mod tests { use std::fs; use super::super::test_support::{ - ScopedEnvVar, TEST_DOCKER_ARGS_ENV, env_lock, path_with_prepend, test_messages, - write_fake_docker_script, + decode_fake_docker_log, test_messages, write_self_contained_fake_docker, }; use super::*; use crate::cli::args::{ @@ -455,41 +454,12 @@ mod tests { OPENBAO_INFRA_CERT_KEY, OPENBAO_TLS_CERT_PATH, OPENBAO_TLS_DEFAULT_RENEW_BEFORE, OPENBAO_TLS_KEY_PATH, }; - use crate::commands::rotate::run_rotate; + use crate::commands::rotate::{run_rotate, run_rotate_with_exec}; use crate::state::StateFile; /// The `OpenBao` container name a default install renders. const DEFAULT_OPENBAO_CONTAINER: &str = "bootroot-openbao"; - /// Writes a fake `docker` that appends one line per invocation to - /// `args_log` and reads nothing from its environment. - /// - /// The log path is baked into the script text as it is written, so a - /// test handing this executable through the docker seam gets its - /// argv back without setting a single variable on this process. - /// `init::steps::test_support` carries a twin for the `init` tree. - /// The one module both trees can reach, `rotate::test_support`, - /// holds the `PATH`-based fake every unconverted test still depends - /// on and stays untouched until those conversions land; the `init` - /// twin is scoped to its own tree and is not visible here. The - /// duplication is what that costs, and it goes away when the - /// conversions are free to rework the shared harness. - fn write_self_contained_fake_docker(path: &Path, args_log: &Path) { - use std::os::unix::fs::PermissionsExt; - - let log = args_log.display().to_string(); - assert!( - !log.contains('\''), - "the log path is interpolated into a single-quoted shell word" - ); - let script = format!( - "#!/bin/sh\nset -eu\n{{ printf '%s ' \"$@\"; printf '\\n'; }} >> '{log}'\nexit 0\n" - ); - fs::write(path, script).expect("fake docker script should be written"); - fs::set_permissions(path, fs::Permissions::from_mode(0o700)) - .expect("fake docker script should be executable"); - } - fn make_openbao_infra_entry(compose_dir: &std::path::Path) -> InfraCertEntry { InfraCertEntry { cert_path: compose_dir.join(OPENBAO_TLS_CERT_PATH), @@ -585,10 +555,9 @@ mod tests { let dir = tempfile::tempdir().expect("tempdir"); let messages = test_messages(); - let bin_dir = dir.path().join("bin"); - fs::create_dir(&bin_dir).expect("bin dir"); - let docker_path = bin_dir.join("docker"); - write_fake_docker_script(&docker_path); + let fake_docker = dir.path().join("fake-docker"); + let args_log = dir.path().join("docker_args.log"); + write_self_contained_fake_docker(&fake_docker, &args_log); let compose_dir = dir.path().join("compose"); let secrets_dir = dir.path().join("secrets"); @@ -665,13 +634,7 @@ mod tests { show_secrets: false, }; - let args_log = dir.path().join("docker_args.log"); - - let _lock = env_lock(); - let _path = ScopedEnvVar::set("PATH", path_with_prepend(&bin_dir)); - let _log = ScopedEnvVar::set(TEST_DOCKER_ARGS_ENV, &args_log); - - rt.block_on(run_rotate(&args, &messages)) + rt.block_on(run_rotate_with_exec(&args, &fake_docker, &messages)) .expect("run_rotate(InfraCert) must succeed and verify the swap"); // The updated state must have been written back to the @@ -701,16 +664,22 @@ mod tests { "state must not be written to hardcoded state.json" ); - // The fake docker log is truncated on every call, so its final - // contents are the last docker invocation — the reload signal. - let log = fs::read_to_string(&args_log).unwrap_or_default(); + // The fake keeps every invocation, so the reload signal is + // pinned as one whole argv rather than as bytes anywhere in the + // log, and the restart check now covers the entire run. + let invocations = decode_fake_docker_log(&args_log); assert!( - log.contains("kill") && log.contains("SIGHUP"), - "final docker call must be kill -s SIGHUP, got: {log}" + invocations.contains(&vec![ + "kill".to_string(), + "-s".to_string(), + "SIGHUP".to_string(), + DEFAULT_OPENBAO_CONTAINER.to_string(), + ]), + "the reload must be kill -s SIGHUP against the openbao container, got: {invocations:?}" ); assert!( - !log.contains("restart"), - "openbao entry must never trigger docker restart, got: {log}" + !invocations.iter().flatten().any(|arg| arg == "restart"), + "openbao entry must never trigger docker restart, got: {invocations:?}" ); } @@ -723,10 +692,9 @@ mod tests { let dir = tempfile::tempdir().expect("tempdir"); let messages = test_messages(); - let bin_dir = dir.path().join("bin"); - fs::create_dir(&bin_dir).expect("bin dir"); - let docker_path = bin_dir.join("docker"); - write_fake_docker_script(&docker_path); + let fake_docker = dir.path().join("fake-docker"); + let args_log = dir.path().join("docker_args.log"); + write_self_contained_fake_docker(&fake_docker, &args_log); let compose_dir = dir.path().join("compose"); let secrets_dir = dir.path().join("secrets"); @@ -793,15 +761,9 @@ mod tests { show_secrets: false, }; - let args_log = dir.path().join("docker_args.log"); - - let _lock = env_lock(); - let _path = ScopedEnvVar::set("PATH", path_with_prepend(&bin_dir)); - let _log = ScopedEnvVar::set(TEST_DOCKER_ARGS_ENV, &args_log); - let rt = tokio::runtime::Runtime::new().expect("tokio runtime"); let err = rt - .block_on(run_rotate(&args, &messages)) + .block_on(run_rotate_with_exec(&args, &fake_docker, &messages)) .expect_err("unreachable probe target must fail the command"); let chain = format!("{err:#}"); assert!( @@ -815,10 +777,10 @@ mod tests { // The reload signal (`kill`) must have run before the probe, so // the container was never restarted. - let log = fs::read_to_string(&args_log).unwrap_or_default(); + let invocations = decode_fake_docker_log(&args_log); assert!( - !log.contains("restart"), - "openbao entry must never trigger docker restart, got: {log}" + !invocations.iter().flatten().any(|arg| arg == "restart"), + "openbao entry must never trigger docker restart, got: {invocations:?}" ); } @@ -852,10 +814,9 @@ mod tests { let dir = tempfile::tempdir().expect("tempdir"); let messages = test_messages(); - let bin_dir = dir.path().join("bin"); - fs::create_dir(&bin_dir).expect("bin dir"); - let docker_path = bin_dir.join("docker"); - write_fake_docker_script(&docker_path); + let fake_docker = dir.path().join("fake-docker"); + let args_log = dir.path().join("docker_args.log"); + write_self_contained_fake_docker(&fake_docker, &args_log); let compose_dir = dir.path().join("compose"); let secrets_dir = dir.path().join("secrets"); @@ -920,14 +881,8 @@ mod tests { show_secrets: false, }; - let args_log = dir.path().join("docker_args.log"); - - let _lock = env_lock(); - let _path = ScopedEnvVar::set("PATH", path_with_prepend(&bin_dir)); - let _log = ScopedEnvVar::set(TEST_DOCKER_ARGS_ENV, &args_log); - let rt = tokio::runtime::Runtime::new().expect("tokio runtime"); - rt.block_on(run_rotate(&args, &messages)) + rt.block_on(run_rotate_with_exec(&args, &fake_docker, &messages)) .expect("run_rotate(InfraCert) must succeed for http01 admin entry"); let reloaded = StateFile::load(&state_file).expect("state file must be readable"); @@ -942,10 +897,15 @@ mod tests { // Verify the fake docker received a `kill -s SIGHUP` command // (the ContainerSignal reload strategy). - let log = fs::read_to_string(&args_log).unwrap_or_default(); + let invocations = decode_fake_docker_log(&args_log); assert!( - log.contains("kill") && log.contains("SIGHUP"), - "docker must have been called with kill -s SIGHUP, got: {log}" + invocations.contains(&vec![ + "kill".to_string(), + "-s".to_string(), + "SIGHUP".to_string(), + RESPONDER_SERVICE_NAME.to_string(), + ]), + "docker must have been called with kill -s SIGHUP, got: {invocations:?}" ); // The HTTP-01 admin entry gets no post-reload verification, so no // openbao_url is consulted and no probe runs. @@ -995,43 +955,14 @@ mod tests { ); } - /// The signal has to reach the instance's own responder container, - /// not a co-located install's. - #[test] - fn container_signal_addresses_the_named_container() { - let dir = tempfile::tempdir().expect("tempdir"); - let bin_dir = dir.path().join("bin"); - fs::create_dir(&bin_dir).expect("bin dir"); - write_fake_docker_script(&bin_dir.join("docker")); - let args_log = dir.path().join("docker_args.log"); - - let _lock = env_lock(); - let _path = ScopedEnvVar::set("PATH", path_with_prepend(&bin_dir)); - let _log = ScopedEnvVar::set(TEST_DOCKER_ARGS_ENV, &args_log); - - execute_reload_strategy( - &ReloadStrategy::ContainerSignal { - container_name: "insight-http01".to_string(), - signal: "SIGHUP".to_string(), - }, - Path::new("docker"), - ) - .expect("signalling must succeed against the fake docker"); - - let logged = fs::read_to_string(&args_log).expect("read docker args"); - let logged_args: Vec<&str> = logged.lines().collect(); - assert_eq!(logged_args, vec!["kill", "-s", "SIGHUP", "insight-http01"]); - } - /// The seam at the signal spawn: `execute_reload_strategy` kills the /// container with whichever program its caller named, which in the - /// flow is `ctx.docker`. + /// flow is `ctx.docker`. The signal therefore has to reach the + /// instance's own responder container, not a co-located install's. /// /// The test mutates nothing process-global — no `PATH` edit, no /// variable set, no lock — because the fake carries its own - /// argv-log path in its script text. That is the property the seam - /// exists to make possible, and the model the conversion of this - /// file's remaining `PATH` fakes follows. + /// argv-log path in its script text. #[test] fn container_signal_runs_the_supplied_executable() { let dir = tempfile::tempdir().expect("tempdir"); @@ -1048,10 +979,9 @@ mod tests { ) .expect("signalling must succeed against the supplied executable"); - let logged = fs::read_to_string(&args_log).expect("read docker args"); assert_eq!( - logged.lines().collect::>(), - vec!["kill -s SIGHUP insight-http01 "], + decode_fake_docker_log(&args_log), + [["kill", "-s", "SIGHUP", "insight-http01"]], "the supplied executable must have received the unchanged argv" ); } diff --git a/src/commands/rotate/stepca_password.rs b/src/commands/rotate/stepca_password.rs index b9711fc7..bbe7b861 100644 --- a/src/commands/rotate/stepca_password.rs +++ b/src/commands/rotate/stepca_password.rs @@ -168,22 +168,18 @@ mod tests { use tempfile::tempdir; - use super::super::test_support::*; + use super::super::test_support::{ + decode_fake_docker_log, test_messages, write_self_contained_fake_docker, + write_self_contained_fake_docker_exiting, + }; use super::*; #[test] fn change_stepca_passphrase_invokes_docker_with_force_and_expected_paths() { - let _lock = env_lock(); let temp = tempdir().expect("tempdir"); - let bin_dir = temp.path().join("bin"); - fs::create_dir_all(&bin_dir).expect("bin dir"); - let docker_path = bin_dir.join("docker"); - write_fake_docker_script(&docker_path); - + let docker_path = temp.path().join("fake-docker"); let args_log_path = temp.path().join("docker-args.log"); - let _path_guard = ScopedEnvVar::set("PATH", path_with_prepend(&bin_dir)); - let _args_guard = ScopedEnvVar::set(TEST_DOCKER_ARGS_ENV, args_log_path.as_os_str()); - let _exit_guard = ScopedEnvVar::set(TEST_DOCKER_EXIT_ENV, "0"); + write_self_contained_fake_docker(&docker_path, &args_log_path); let secrets_dir = temp.path().join("secrets"); fs::create_dir_all(secrets_dir.join("secrets")).expect("create secrets key dir"); @@ -199,13 +195,12 @@ mod tests { ¤t_password, &new_password, &key_path, - Path::new("docker"), + &docker_path, &test_messages(), ) .expect("change passphrase should succeed"); - let logged_args = fs::read_to_string(&args_log_path).expect("read logged args"); - let args: Vec<&str> = logged_args.lines().collect(); + let invocations = decode_fake_docker_log(&args_log_path); let mount_root = fs::canonicalize(&secrets_dir).expect("canonicalize secrets dir"); let expected_mount = format!("{}:/home/step", mount_root.display()); // The container must run as the secrets-directory owner, not root, @@ -230,7 +225,7 @@ mod tests { "/home/step/password.txt.new", "-f", ]; - assert_eq!(args, expected); + assert_eq!(invocations, vec![expected]); } #[test] @@ -259,17 +254,10 @@ mod tests { #[test] fn change_stepca_passphrase_surfaces_docker_failure_status() { - let _lock = env_lock(); let temp = tempdir().expect("tempdir"); - let bin_dir = temp.path().join("bin"); - fs::create_dir_all(&bin_dir).expect("bin dir"); - let docker_path = bin_dir.join("docker"); - write_fake_docker_script(&docker_path); - + let docker_path = temp.path().join("fake-docker"); let args_log_path = temp.path().join("docker-args.log"); - let _path_guard = ScopedEnvVar::set("PATH", path_with_prepend(&bin_dir)); - let _args_guard = ScopedEnvVar::set(TEST_DOCKER_ARGS_ENV, args_log_path.as_os_str()); - let _exit_guard = ScopedEnvVar::set(TEST_DOCKER_EXIT_ENV, "7"); + write_self_contained_fake_docker_exiting(&docker_path, &args_log_path, 7); let secrets_dir = temp.path().join("secrets"); fs::create_dir_all(secrets_dir.join("secrets")).expect("create secrets key dir"); @@ -285,7 +273,7 @@ mod tests { ¤t_password, &new_password, &key_path, - Path::new("docker"), + &docker_path, &test_messages(), ) .expect_err("docker failure should bubble up"); From 10ce4269552d04a9313b1a02dbe1b8c5f10d802e Mon Sep 17 00:00:00 2001 From: sehkone Date: Mon, 10 Aug 2026 09:44:03 +0900 Subject: [PATCH 2/3] Pin the fake docker log's argument-less record An invocation with no arguments encodes as the bare record `0\0`, which contributes no fields of its own. That makes it the one record whose count is the only thing separating it from the next, so a decoder that scanned for a boundary rather than counting would merge it into whatever followed. Nothing in the rotate flows issues a bare `docker`, so the framing's degenerate case went unexercised. Part of #808 --- src/commands/rotate.rs | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/src/commands/rotate.rs b/src/commands/rotate.rs index 38a27f9e..f22b930f 100644 --- a/src/commands/rotate.rs +++ b/src/commands/rotate.rs @@ -542,6 +542,28 @@ mod tests { ); } + /// An argument-less invocation is the degenerate record, `0\0`, and + /// the count is the only thing that separates it from the next one: + /// it contributes no fields of its own, so a decoder that scanned + /// for a boundary instead of counting would swallow the record after + /// it. + #[test] + fn the_fake_docker_log_frames_an_argument_less_invocation() { + let dir = tempdir().expect("tempdir"); + let args_log = dir.path().join("docker_args.log"); + let fake = dir.path().join("fake-docker"); + write_self_contained_fake_docker(&fake, &args_log); + + run_fake(&fake, &[]); + run_fake(&fake, &["restart", "c"]); + run_fake(&fake, &[]); + + assert_eq!( + decode_fake_docker_log(&args_log), + vec![vec![], vec!["restart".to_string(), "c".to_string()], vec![]] + ); + } + /// `tempfile::tempdir()` can legitimately hand back a path holding /// an apostrophe, so the helper quotes rather than rejects one. #[test] From 394f9732a7e430236f83a68f5fc8d478b50864dd Mon Sep 17 00:00:00 2001 From: sehkone Date: Mon, 10 Aug 2026 10:21:25 +0900 Subject: [PATCH 3/3] Embed the fake docker's log path byte for byte The promoted helper interpolated the argv-log path into the script text through `Display`, which replaces any byte that is not valid UTF-8. A Unix path is an arbitrary NUL-free byte sequence, so a tempdir rooted below a non-UTF-8 `TMPDIR` left the fake redirecting to a different, usually nonexistent, path -- it would exit before recording argv, and an assertion on that log would read a file nothing ever wrote. This is the same class of legal temp path the apostrophe handling already covers. Assemble the script as bytes and quote the path's own bytes, so the escaping stays byte for byte and no conversion sits in the way. Part of #808 --- src/commands/rotate.rs | 83 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 76 insertions(+), 7 deletions(-) diff --git a/src/commands/rotate.rs b/src/commands/rotate.rs index f22b930f..eefc7d99 100644 --- a/src/commands/rotate.rs +++ b/src/commands/rotate.rs @@ -389,23 +389,39 @@ exit 0 args_log: &Path, exit_code: u8, ) { + use std::os::unix::ffi::OsStrExt; use std::os::unix::fs::PermissionsExt; - let log = shell_single_quote(&args_log.display().to_string()); - let script = format!( - "#!/bin/sh\nset -eu\nprintf '%s\\0' \"$#\" \"$@\" >> {log}\nexit {exit_code}\n" - ); + // The script is assembled as bytes, not as a `String`: a Unix + // path is an arbitrary NUL-free byte sequence, and rendering + // `args_log` through `Display` would replace any byte that is + // not valid UTF-8, pointing the fake at a different path that + // nothing would ever create. + let mut script = b"#!/bin/sh\nset -eu\nprintf '%s\\0' \"$#\" \"$@\" >> ".to_vec(); + script.extend_from_slice(&shell_single_quote(args_log.as_os_str().as_bytes())); + script.extend_from_slice(format!("\nexit {exit_code}\n").as_bytes()); fs::write(path, script).expect("fake docker script should be written"); fs::set_permissions(path, fs::Permissions::from_mode(0o700)) .expect("fake docker script should be executable"); } - /// Quotes `value` as a single POSIX shell word. + /// Quotes `value` as a single POSIX shell word, byte for byte. /// /// A tempdir path may legally contain `'`, which ends the quoted /// word; the usual `'\''` dance closes, escapes and reopens it. - fn shell_single_quote(value: &str) -> String { - format!("'{}'", value.replace('\'', r"'\''")) + /// Every other byte is copied through unchanged, so a path that is + /// not UTF-8 reaches the script intact. + fn shell_single_quote(value: &[u8]) -> Vec { + let mut quoted = vec![b'\'']; + for byte in value { + if *byte == b'\'' { + quoted.extend_from_slice(br"'\''"); + } else { + quoted.push(*byte); + } + } + quoted.push(b'\''); + quoted } /// Decodes a log written by [`write_self_contained_fake_docker`], @@ -580,6 +596,59 @@ mod tests { assert_eq!(decode_fake_docker_log(&args_log), [["restart", "c"]]); } + /// The redirect the fake is written with must hold the log path's + /// own bytes. Rendering the path through `Display` instead replaces + /// every byte that is not valid UTF-8 with `U+FFFD`, which silently + /// aims the fake at a path nothing creates. + /// + /// This asserts on the script text rather than on running it, so it + /// holds on filesystems that would refuse to create the name. + #[test] + fn the_fake_docker_script_embeds_the_log_path_verbatim() { + use std::ffi::OsStr; + use std::os::unix::ffi::OsStrExt; + + let dir = tempdir().expect("tempdir"); + let args_log = dir.path().join(OsStr::from_bytes(b"non\xffutf8.log")); + let fake = dir.path().join("fake-docker"); + write_self_contained_fake_docker(&fake, &args_log); + + let script = fs::read(&fake).expect("the fake docker script should be readable"); + let expected = args_log.as_os_str().as_bytes(); + assert!( + script + .windows(expected.len()) + .any(|window| window == expected), + "the script must redirect to the log path's own bytes" + ); + } + + /// A Unix path is an arbitrary NUL-free byte sequence, so a tempdir + /// rooted below one that is not UTF-8 — which `TMPDIR` can be — must + /// still yield a fake that logs where the test reads. + /// + /// The name is only creatable where the filesystem takes it: APFS + /// and other UTF-8-enforcing filesystems reject it with `EILSEQ`, + /// and on those the property is unobservable rather than broken. + #[test] + fn the_fake_docker_handles_a_non_utf8_log_path() { + use std::ffi::OsStr; + use std::os::unix::ffi::OsStrExt; + + let dir = tempdir().expect("tempdir"); + let awkward = dir.path().join(OsStr::from_bytes(b"non\xffutf8")); + if fs::create_dir(&awkward).is_err() { + return; + } + let args_log = awkward.join("docker_args.log"); + let fake = dir.path().join("fake-docker"); + write_self_contained_fake_docker(&fake, &args_log); + + run_fake(&fake, &["restart", "c"]); + + assert_eq!(decode_fake_docker_log(&args_log), [["restart", "c"]]); + } + /// The baked-in exit code is what replaces the environment variable /// the shared `PATH` fake reads. #[test]