From c4e9c98de686125ec918903d19cd52423e79ac3b Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Sat, 31 Oct 2026 12:27:22 -0700 Subject: [PATCH] refactor: insecure mode cleanup --- .github/workflows/ci.yml | 2 +- AGENTS.md | 19 +++--- native/suidhelper/Cargo.toml | 10 +--- native/suidhelper/README.md | 8 +-- native/suidhelper/build.rs | 8 +-- native/suidhelper/src/config.rs | 2 +- native/suidhelper/src/insecure_mode.rs | 67 ++++++++++++++++++++++ native/suidhelper/src/lib.rs | 2 +- native/suidhelper/src/main.rs | 7 --- native/suidhelper/src/security_gate.rs | 44 -------------- native/suidhelper/tests/config_seam.rs | 6 +- native/suidhelper/tests/e2e/argv.rs | 2 +- native/suidhelper/tests/e2e/chroot_jail.rs | 2 +- native/suidhelper/tests/e2e/config.rs | 2 +- native/suidhelper/tests/e2e/jailer.rs | 2 +- native/suidhelper/tests/e2e/support.rs | 2 +- native/suidhelper/tests/e2e/thin_dump.rs | 2 +- native/suidhelper/tests/e2e/version.rs | 2 +- native/suidhelper/tests/security_gate.rs | 50 ---------------- 19 files changed, 100 insertions(+), 139 deletions(-) create mode 100644 native/suidhelper/src/insecure_mode.rs delete mode 100644 native/suidhelper/src/security_gate.rs delete mode 100644 native/suidhelper/tests/security_gate.rs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2492964e..5442ad17 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -302,7 +302,7 @@ jobs: - name: Gated tests as root + coverage run: | sudo -E env "PATH=$PATH" \ - cargo llvm-cov nextest --features insecure_test_seams \ + cargo llvm-cov nextest --features insecure_mode \ -E 'test(/_as_root$/)' --lcov --output-path lcov-privileged.info - name: Upload coverage to Codecov diff --git a/AGENTS.md b/AGENTS.md index 69bfc5b5..8f54be44 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -47,8 +47,8 @@ run in CI's `integration` job, which provisions the GitHub-hosted runner via `lib/hyper/redist/` — **pure cores**: units algebra, strict number parsing, EWMA/rate controls, `/proc` parsers, hashing. No processes, no I/O. These are where property tests pay off most. -- `native/suidhelper/` — the privileged Rust helper. Source in `src/`, tests in - `tests/` (see Rust rules below). +- `native/suidhelper/` — the privileged Rust helper. Source in `src/`, tests + inline in `src/` or in `tests/` (see Rust rules below). - Generated, do not hand-edit: `lib/hyper/firecracker/api/{operations,schemas}` (regen `mix firecracker.gen`) and `lib/hyper/grpc/v1/hyper.pb.ex` (regen `mix grpc.gen`). Both are gitignored and rebuilt by a Mix compiler. @@ -157,13 +157,14 @@ If a case needs its own assertions, it is a separate test, not a row. ### Rust specifics (`native/suidhelper/`) -- **Tests live in `tests/`, never inline `#[cfg(test)]` in `src/`.** The crate - is split into a lib (`src/lib.rs`, crate `hyper_suidhelper`) and a thin bin - (`src/main.rs`) precisely so integration tests can reach the internals. - **Exception: `src/linux/`.** Tests for these utilities are colocated with - the code, in a `#[cfg(test)] mod tests` block at the bottom of the file - under test. -- Each test file must be registered as a `[[test]]` target in `Cargo.toml` +- **We are migrating tests inline.** New tests for a module go in a + `#[cfg(test)] mod tests` block at the bottom of the file under test, not in + `tests/`. Many existing tests still live in `tests/` (`src/linux/` and + `src/insecure_mode.rs` are already migrated); when you substantially change a + module whose tests are still in `tests/`, move them inline as part of the + change. `tests/` remains the home for tests that drive the built binary or + span modules (`tests/e2e/`, `tests/config_seam.rs`). +- Each file in `tests/` must be registered as a `[[test]]` target in `Cargo.toml` (`tests/` subdirectories are not auto-discovered) — copy an existing `[[test]]` block when adding one. - `proptest` is the dev-dependency; use `proptest!{ #[test] fn ... }` with diff --git a/native/suidhelper/Cargo.toml b/native/suidhelper/Cargo.toml index 92b12922..7167ccf4 100644 --- a/native/suidhelper/Cargo.toml +++ b/native/suidhelper/Cargo.toml @@ -13,9 +13,9 @@ license = "MIT" repository = "https://github.com/harmont-dev/hyper" [features] -# TEST ONLY. Compiles the security_gate insecure arms. Never enable for a real +# TEST ONLY. Compiles the insecure_mode insecure arms. Never enable for a real # (setuid) build — build.rs refuses to compile a release binary with this on. -insecure_test_seams = [] +insecure_mode = [] [[bin]] name = "hyper-suidhelper" @@ -24,7 +24,7 @@ path = "src/main.rs" [[bin]] name = "fake-jailer" path = "tests/e2e/fake_jailer.rs" -required-features = ["insecure_test_seams"] +required-features = ["insecure_mode"] # Integration tests mirror the `src/` module tree under `tests/`. Cargo only # auto-discovers test files at the top level of `tests/`, so each nested file @@ -33,10 +33,6 @@ required-features = ["insecure_test_seams"] name = "safe_path" path = "tests/util/safe_path.rs" -[[test]] -name = "security_gate" -path = "tests/security_gate.rs" - [[test]] name = "config_seam" path = "tests/config_seam.rs" diff --git a/native/suidhelper/README.md b/native/suidhelper/README.md index 15161a87..67c734a5 100644 --- a/native/suidhelper/README.md +++ b/native/suidhelper/README.md @@ -80,18 +80,18 @@ elsewhere via dm-thin), so there is no read-write attach. - `cargo nextest run` — pure layers (parser matrix, confinement primitives, gate defaults). No root, no config, no Docker. -- `cargo nextest run --features insecure_test_seams` — adds the config-seam and +- `cargo nextest run --features insecure_mode` — adds the config-seam and subprocess suites. Cases needing root self-skip with a loud `SKIP` line. - As root (`sudo -E env "PATH=$PATH" cargo nextest run --features - insecure_test_seams --test e2e_config --test e2e_argv`) — runs every case, + insecure_mode --test e2e_config --test e2e_argv`) — runs every case, including `sys-test ok` and the fake-bin argv-capture assertions. CI does this in the `suidhelper-privileged` job. ### The insecure test seam -`security_gate` is the single decision point for INSECURE TEST MODE. The insecure +`insecure_mode` is the single decision point for INSECURE TEST MODE. The insecure code path (the `HYPER_SETUIDHELPER_CONFIG_PATH` config override) activates only -when BOTH gates are open: the `insecure_test_seams` Cargo feature (compile-time) +when BOTH gates are open: the `insecure_mode` Cargo feature (compile-time) AND `HYPER_SETUIDHELPER_IS_INSECURE_MODE=1` (runtime). Release builds never enable the feature — `build.rs` refuses to compile a release binary with it — so the installed setuid binary always takes the secure arm regardless of the diff --git a/native/suidhelper/build.rs b/native/suidhelper/build.rs index de04496b..2a9f2d63 100644 --- a/native/suidhelper/build.rs +++ b/native/suidhelper/build.rs @@ -1,19 +1,19 @@ // SPDX-License-Identifier: MIT -//! Build guard: keep the `insecure_test_seams` feature out of any release +//! Build guard: keep the `insecure_mode` feature out of any release //! artifact. Tests build in the `debug` profile, so they are unaffected. fn main() { // Re-run only when the relevant inputs change. println!("cargo:rerun-if-changed=build.rs"); - if std::env::var_os("CARGO_FEATURE_INSECURE_TEST_SEAMS").is_some() { + if std::env::var_os("CARGO_FEATURE_INSECURE_MODE").is_some() { println!( - "cargo:warning=hyper-suidhelper built with `insecure_test_seams` \ + "cargo:warning=hyper-suidhelper built with `insecure_mode` \ — TEST ONLY. Never install this binary setuid." ); if std::env::var("PROFILE").as_deref() == Ok("release") { panic!( - "refusing to build a RELEASE binary with `insecure_test_seams`: \ + "refusing to build a RELEASE binary with `insecure_mode`: \ it would ship a setuid security bypass" ); } diff --git a/native/suidhelper/src/config.rs b/native/suidhelper/src/config.rs index 493280f6..a47e0292 100644 --- a/native/suidhelper/src/config.rs +++ b/native/suidhelper/src/config.rs @@ -84,7 +84,7 @@ pub fn validate_uid_gid_range(r: &UidGidRange) -> Result<(), LoadingError> { /// Only in INSECURE TEST MODE (both gates open) may an env var redirect it — the /// secure arm is always the hardcoded path, so a release build cannot be steered. fn config_path() -> PathBuf { - crate::security_gate::split( + crate::insecure_mode::select( || PathBuf::from(CONFIG_PATHSTR), || { std::env::var(INSECURE_CONFIG_PATH_ENV) diff --git a/native/suidhelper/src/insecure_mode.rs b/native/suidhelper/src/insecure_mode.rs new file mode 100644 index 00000000..0edc915a --- /dev/null +++ b/native/suidhelper/src/insecure_mode.rs @@ -0,0 +1,67 @@ +// SPDX-License-Identifier: MIT +//! Utility for deciding whether the program is running in insecure mode. +//! +//! Insecure mode is enabled by both building with the `insecure_mode` flag, as well as +//! setting `$HYPER_SETUIDHELPER_IS_INSECURE_MODE`. +//! +//! Insecure mode is designed to help make testing easier. The only public function you should use +//! is [`select`]. Callers control the semantics of what "insecure" means. +use std::sync::LazyLock; + +const INSECURE_MODE_ENV: &str = "HYPER_SETUIDHELPER_IS_INSECURE_MODE"; + +static INSECURE: LazyLock = LazyLock::new(|| { + let insecure = + cfg!(feature = "insecure_mode") && std::env::var(INSECURE_MODE_ENV).as_deref() == Ok("1"); + + if insecure { + eprintln!( + "hyper-suidhelper: WARNING: running in INSECURE TEST MODE \ + ({INSECURE_MODE_ENV}=1); never do this on a real host" + ); + } + + insecure +}); + +/// Run `secure` in production; run `insecure` in testing. +pub fn select(secure: impl FnOnce() -> T, insecure: impl FnOnce() -> T) -> T { + if cfg!(feature = "insecure_mode") && *INSECURE { + insecure() + } else { + secure() + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[cfg(not(feature = "insecure_mode"))] + #[test] + fn secure_arm_always_taken_without_feature() { + std::env::set_var(INSECURE_MODE_ENV, "1"); + assert_eq!(select(|| "secure", || "insecure"), "secure"); + } + + #[cfg(feature = "insecure_mode")] + #[test] + fn secure_arm_when_env_absent_even_with_feature() { + std::env::remove_var(INSECURE_MODE_ENV); + assert_eq!(select(|| "secure", || "insecure"), "secure"); + } + + #[cfg(feature = "insecure_mode")] + #[test] + fn insecure_arm_when_feature_and_env() { + std::env::set_var(INSECURE_MODE_ENV, "1"); + assert_eq!(select(|| "secure", || "insecure"), "insecure"); + } + + #[cfg(feature = "insecure_mode")] + #[test] + fn secure_arm_when_env_not_exactly_one() { + std::env::set_var(INSECURE_MODE_ENV, "true"); + assert_eq!(select(|| "secure", || "insecure"), "secure"); + } +} diff --git a/native/suidhelper/src/lib.rs b/native/suidhelper/src/lib.rs index 3f56f65f..ec1af487 100644 --- a/native/suidhelper/src/lib.rs +++ b/native/suidhelper/src/lib.rs @@ -10,8 +10,8 @@ pub mod checksum; pub mod config; pub mod fs; +pub mod insecure_mode; pub mod linux; pub mod process; -pub mod security_gate; pub mod tools; pub mod util; diff --git a/native/suidhelper/src/main.rs b/native/suidhelper/src/main.rs index 1ecf6367..300d1dfb 100644 --- a/native/suidhelper/src/main.rs +++ b/native/suidhelper/src/main.rs @@ -89,13 +89,6 @@ impl SysTest { } fn run(mut token: Unprivileged) { - // The gate must resolve before clap parsing: some subcommands' value_parsers - // (losetup attach's backing-file gate) consult the config while clap parses - // argv, and the config consults this gate. Resolving the gate first ensures - // that first config load — wherever it happens — sees the right gate state. - // In release this is a no-op: the gate stays secure. - hyper_suidhelper::security_gate::init(); - let command = Cli::parse().command; // `version` is a pure self-report: it touches neither config nor privileges, diff --git a/native/suidhelper/src/security_gate.rs b/native/suidhelper/src/security_gate.rs deleted file mode 100644 index 4b361856..00000000 --- a/native/suidhelper/src/security_gate.rs +++ /dev/null @@ -1,44 +0,0 @@ -// SPDX-License-Identifier: MIT -//! Central security gate: the single place that decides whether this process is -//! running in INSECURE TEST MODE, resolved once at startup. -//! -//! Two independent gates, both required to open the insecure path: -//! 1. compile-time — the `insecure_test_seams` Cargo feature. Never enabled in -//! release; `build.rs` refuses a release build with it on. -//! 2. runtime — `HYPER_SETUIDHELPER_IS_INSECURE_MODE=1` in the environment. -//! -//! Seams never test these directly. They call [`split`], which pairs every -//! insecure branch with its production default and can only run the insecure arm -//! when BOTH gates are open. The default — uninitialized, or any release build — -//! is always the secure arm, so a forgotten `init` fails safe. - -use std::sync::atomic::{AtomicBool, Ordering}; - -const INSECURE_MODE_ENV: &str = "HYPER_SETUIDHELPER_IS_INSECURE_MODE"; - -/// Resolved once by [`init`]; defaults to secure so a missing `init` is safe. -static INSECURE: AtomicBool = AtomicBool::new(false); - -/// Resolve the gate from the compile feature and the environment. Call once, as -/// the very first thing in `main`, before any seam (e.g. the config load) runs. -/// Idempotent. -pub fn init() { - let insecure = cfg!(feature = "insecure_test_seams") - && std::env::var(INSECURE_MODE_ENV).as_deref() == Ok("1"); - if insecure { - eprintln!( - "hyper-suidhelper: WARNING: running in INSECURE TEST MODE \ - ({INSECURE_MODE_ENV}=1); never do this on a real host" - ); - } - INSECURE.store(insecure, Ordering::Relaxed); -} - -/// Run `secure` in production; run `insecure` in testing. -pub fn split(secure: impl FnOnce() -> T, insecure: impl FnOnce() -> T) -> T { - if cfg!(feature = "insecure_test_seams") && INSECURE.load(Ordering::Relaxed) { - insecure() - } else { - secure() - } -} diff --git a/native/suidhelper/tests/config_seam.rs b/native/suidhelper/tests/config_seam.rs index af876776..4e2e98b5 100644 --- a/native/suidhelper/tests/config_seam.rs +++ b/native/suidhelper/tests/config_seam.rs @@ -1,22 +1,20 @@ -//! The config-path seam routes through security_gate::split. With both gates +//! The config-path seam routes through insecure_mode::select. With both gates //! open, safe_load reads HYPER_SETUIDHELPER_CONFIG_PATH instead of the hardcoded //! /etc/hyper/config.toml — but with every production check (SafePath lexical //! gate, SafeFile ownership/type) still enforced. A `..` override is rejected by //! the real SafePath gate as LooseComponents; the default path is lexically //! clean, so that error proves the override (not the default) was used. No root //! needed and no security check is bypassed. -#![cfg(feature = "insecure_test_seams")] +#![cfg(feature = "insecure_mode")] use hyper_suidhelper::config::{Config, LoadingError}; use hyper_suidhelper::fs::path::{ComponentError, ValidationError}; -use hyper_suidhelper::security_gate; #[test] fn override_path_is_consulted_through_the_real_lexical_gate() { let loose = "/tmp/hyper-suidhelper/../escape.toml"; std::env::set_var("HYPER_SETUIDHELPER_IS_INSECURE_MODE", "1"); std::env::set_var("HYPER_SETUIDHELPER_CONFIG_PATH", loose); - security_gate::init(); let err = Config::safe_load().expect_err("override must be consulted"); assert!( diff --git a/native/suidhelper/tests/e2e/argv.rs b/native/suidhelper/tests/e2e/argv.rs index c73875ea..27727f72 100644 --- a/native/suidhelper/tests/e2e/argv.rs +++ b/native/suidhelper/tests/e2e/argv.rs @@ -2,7 +2,7 @@ //! the one thing the design deliberately hides from the caller. We point the //! tool's config path at a root-owned fake that writes its argv+env to a file as //! JSON, then assert on the reconstructed command line. -#![cfg(feature = "insecure_test_seams")] +#![cfg(feature = "insecure_mode")] use predicates::str::contains; use std::fs; diff --git a/native/suidhelper/tests/e2e/chroot_jail.rs b/native/suidhelper/tests/e2e/chroot_jail.rs index c3a617a8..6fb3a648 100644 --- a/native/suidhelper/tests/e2e/chroot_jail.rs +++ b/native/suidhelper/tests/e2e/chroot_jail.rs @@ -8,7 +8,7 @@ //! likewise privileged `mknod` — no unit-level substitute exists; it is proven //! only here, root-gated, with the insecure config seam supplying a `[network]` //! table. -#![cfg(feature = "insecure_test_seams")] +#![cfg(feature = "insecure_mode")] use std::fs; use std::os::unix::fs::{FileTypeExt, MetadataExt, PermissionsExt}; diff --git a/native/suidhelper/tests/e2e/config.rs b/native/suidhelper/tests/e2e/config.rs index 98b0fc57..7f56c638 100644 --- a/native/suidhelper/tests/e2e/config.rs +++ b/native/suidhelper/tests/e2e/config.rs @@ -1,7 +1,7 @@ //! L5: the binary's startup config contract, observed end to end (exit code + //! stderr + stdout JSON). Uses the config-path seam to point at a per-test //! tempfile instead of /etc/hyper, so tests never touch the real host. -#![cfg(feature = "insecure_test_seams")] +#![cfg(feature = "insecure_mode")] use hyper_suidhelper::config::{BinError, Config}; use hyper_suidhelper::process::binary; diff --git a/native/suidhelper/tests/e2e/jailer.rs b/native/suidhelper/tests/e2e/jailer.rs index 2651921c..46370848 100644 --- a/native/suidhelper/tests/e2e/jailer.rs +++ b/native/suidhelper/tests/e2e/jailer.rs @@ -3,7 +3,7 @@ //! propagates. We point the config's `jailer` at a root-owned recorder that //! writes its argv and its `/proc/self/environ` to files, then exits with a known //! code. Root-only: `become_root_permanently` requires we are already root. -#![cfg(feature = "insecure_test_seams")] +#![cfg(feature = "insecure_mode")] use std::fs; use std::os::unix::fs::PermissionsExt; diff --git a/native/suidhelper/tests/e2e/support.rs b/native/suidhelper/tests/e2e/support.rs index 7cac4608..8c3f50bf 100644 --- a/native/suidhelper/tests/e2e/support.rs +++ b/native/suidhelper/tests/e2e/support.rs @@ -22,7 +22,7 @@ pub fn is_root() -> bool { nix::unistd::geteuid().is_root() } -/// Run the helper with the security gate open and the config path redirected +/// Run the helper with insecure mode open and the config path redirected /// to a per-test file, so tests never touch the real host's /etc/hyper. pub fn run(config: &Path, args: &[&str]) -> Assert { let mut cmd = Command::new(BIN); diff --git a/native/suidhelper/tests/e2e/thin_dump.rs b/native/suidhelper/tests/e2e/thin_dump.rs index 21147488..640720a5 100644 --- a/native/suidhelper/tests/e2e/thin_dump.rs +++ b/native/suidhelper/tests/e2e/thin_dump.rs @@ -3,7 +3,7 @@ //! operands at parse time, translate the XML to the ranges JSON, and surface //! both tool failure and a wrong dev_id as errors — an empty result must never //! mean "wrong dump". -#![cfg(feature = "insecure_test_seams")] +#![cfg(feature = "insecure_mode")] use predicates::str::contains; use std::fs; diff --git a/native/suidhelper/tests/e2e/version.rs b/native/suidhelper/tests/e2e/version.rs index 7e565b34..5e257414 100644 --- a/native/suidhelper/tests/e2e/version.rs +++ b/native/suidhelper/tests/e2e/version.rs @@ -6,7 +6,7 @@ //! Contract under test: exit 0 with a single JSON object on stdout where //! `version` == the crate version and `checksum_blake3` is 32 bytes of //! lowercase hex (all-zero on an unstamped dev build -- shape, not value). -#![cfg(feature = "insecure_test_seams")] +#![cfg(feature = "insecure_mode")] use std::path::Path; diff --git a/native/suidhelper/tests/security_gate.rs b/native/suidhelper/tests/security_gate.rs deleted file mode 100644 index 6b94e8d6..00000000 --- a/native/suidhelper/tests/security_gate.rs +++ /dev/null @@ -1,50 +0,0 @@ -//! Gate-resolution contract for `security_gate`. nextest runs each test in its -//! own process, so the env mutations and the process-global `INSECURE` flag do -//! not race across tests. - -use hyper_suidhelper::security_gate; - -const ENV: &str = "HYPER_SETUIDHELPER_IS_INSECURE_MODE"; - -// Without the feature compiled in, the insecure arm is unreachable no matter -// what the environment says — this is the production guarantee. -#[cfg(not(feature = "insecure_test_seams"))] -#[test] -fn secure_arm_always_taken_without_feature() { - std::env::set_var(ENV, "1"); - security_gate::init(); - assert_eq!(security_gate::split(|| "secure", || "insecure"), "secure"); -} - -// With the feature, the env var is still required: feature alone is not enough. -#[cfg(feature = "insecure_test_seams")] -#[test] -fn secure_arm_when_env_absent_even_with_feature() { - std::env::remove_var(ENV); - security_gate::init(); - assert_eq!(security_gate::split(|| "secure", || "insecure"), "secure"); -} - -// Both gates open → insecure arm. -#[cfg(feature = "insecure_test_seams")] -#[test] -fn insecure_arm_when_feature_and_env() { - std::env::set_var(ENV, "1"); - security_gate::init(); - assert_eq!(security_gate::split(|| "secure", || "insecure"), "insecure"); -} - -// A wrong env value is not opt-in. -#[cfg(feature = "insecure_test_seams")] -#[test] -fn secure_arm_when_env_not_exactly_one() { - std::env::set_var(ENV, "true"); - security_gate::init(); - assert_eq!(security_gate::split(|| "secure", || "insecure"), "secure"); -} - -// Default (init never called) is secure. -#[test] -fn secure_arm_before_init() { - assert_eq!(security_gate::split(|| "secure", || "insecure"), "secure"); -}