diff --git a/engine/src/policies/signature/auth_tests.rs b/engine/src/policies/signature/auth_tests.rs index 122a0a7..c1bf328 100644 --- a/engine/src/policies/signature/auth_tests.rs +++ b/engine/src/policies/signature/auth_tests.rs @@ -1,12 +1,79 @@ //! Tests for the shared per-image registry-auth resolver (JEF-339, JEF-352). //! -//! The env-mutating cases rely on nextest's per-test process isolation, so each test owns a -//! clean process env; the `unsafe { set_var }` blocks mirror the pattern the rest of the -//! crate's env tests use. +//! [`RegistryAuth::from_env`] reads three **process-global** env vars +//! (`PROTECTOR_REGISTRY_USERNAME` / `PROTECTOR_REGISTRY_PASSWORD` / +//! `PROTECTOR_REGISTRY_AUTH_FILE`). Rust's default test harness runs tests as parallel +//! threads within one process, so a `set_var`/`remove_var` in one test is visible to any +//! sibling that reads the same var mid-flight — a data race that made these cases flaky +//! (JEF-412). Every case that touches those vars therefore goes through [`EnvGuard`], which +//! serializes them on a shared lock and restores the prior values on drop (even on panic), +//! so no test can observe another's env mutation. + +use std::sync::{Mutex, MutexGuard}; use super::RegistryAuth; use sigstore::registry::Auth; +/// The three process-global env vars [`RegistryAuth::from_env`] reads. [`EnvGuard`] snapshots +/// and restores exactly these, and holds the lock for the whole set → read → assert window. +const CRED_ENV_VARS: [&str; 3] = [ + "PROTECTOR_REGISTRY_USERNAME", + "PROTECTOR_REGISTRY_PASSWORD", + "PROTECTOR_REGISTRY_AUTH_FILE", +]; + +/// Serializes every test that mutates the credential env vars. Without this, `cargo test` runs +/// these cases as concurrent threads in one process and their `set_var`/`remove_var` calls race. +static ENV_LOCK: Mutex<()> = Mutex::new(()); + +/// RAII guard that gives a test exclusive, restore-on-drop access to the credential env vars. +/// +/// On construction it takes [`ENV_LOCK`] (so only one env-mutating test runs at a time), +/// snapshots the current value of each [`CRED_ENV_VARS`] entry, then clears them all so the test +/// starts from a known-clean env. On drop it restores the snapshot — even if the test panics — +/// so a failing assertion can never leak creds into a sibling. +struct EnvGuard { + _lock: MutexGuard<'static, ()>, + saved: [(&'static str, Option); 3], +} + +impl EnvGuard { + /// Acquire the lock, snapshot the credential env vars, and clear them to a known-clean state. + fn acquire() -> Self { + // A prior test that panicked mid-window would poison the lock; recover the guard so one + // legitimate assertion failure doesn't cascade into spurious failures for later tests. + let lock = ENV_LOCK.lock().unwrap_or_else(|p| p.into_inner()); + let saved = CRED_ENV_VARS.map(|k| (k, std::env::var(k).ok())); + // SAFETY: the lock guarantees no sibling test reads/writes these vars concurrently. + unsafe { + for (k, _) in &saved { + std::env::remove_var(k); + } + } + Self { _lock: lock, saved } + } + + /// Set one credential env var within the guarded window. + fn set>(&self, key: &str, value: V) { + // SAFETY: the guard holds the lock, so no sibling test races this mutation. + unsafe { std::env::set_var(key, value) }; + } +} + +impl Drop for EnvGuard { + fn drop(&mut self) { + // SAFETY: still holding the lock; restore each var to its pre-test value. + unsafe { + for (key, prior) in &self.saved { + match prior { + Some(v) => std::env::set_var(key, v), + None => std::env::remove_var(key), + } + } + } + } +} + /// Assert `auth` is `Basic(user, pass)`; panics with the actual variant otherwise. #[track_caller] fn assert_basic(auth: Auth, user: &str, pass: &str) { @@ -32,16 +99,6 @@ fn write_config(tag: &str, contents: &str) -> std::path::PathBuf { path } -/// Clear every credential env var so a test starts from a known-clean process env. -/// SAFETY: nextest runs each test in its own process, so this mutation is isolated. -fn clear_cred_env() { - unsafe { - std::env::remove_var("PROTECTOR_REGISTRY_USERNAME"); - std::env::remove_var("PROTECTOR_REGISTRY_PASSWORD"); - std::env::remove_var("PROTECTOR_REGISTRY_AUTH_FILE"); - } -} - /// A multi-registry k8s `.dockerconfigjson`: ghcr.io (base64 `auth`), Docker Hub keyed by the /// legacy `https://index.docker.io/v1/` URL, and a private `host:port` registry. /// base64("thejefflarson:ghp_token") = dGhlamVmZmxhcnNvbjpnaHBfdG9rZW4= @@ -58,8 +115,8 @@ const MULTI_REGISTRY: &str = r#"{"auths":{ #[test] fn resolves_per_image_across_multiple_registries() { let path = write_config("multi", MULTI_REGISTRY); - clear_cred_env(); - unsafe { std::env::set_var("PROTECTOR_REGISTRY_AUTH_FILE", &path) }; + let env = EnvGuard::acquire(); + env.set("PROTECTOR_REGISTRY_AUTH_FILE", &path); let auth = RegistryAuth::from_env(); assert_basic( @@ -86,8 +143,8 @@ fn resolves_per_image_across_multiple_registries() { #[test] fn docker_io_variants_match_the_index_v1_config_key() { let path = write_config("dockerio", MULTI_REGISTRY); - clear_cred_env(); - unsafe { std::env::set_var("PROTECTOR_REGISTRY_AUTH_FILE", &path) }; + let env = EnvGuard::acquire(); + env.set("PROTECTOR_REGISTRY_AUTH_FILE", &path); let auth = RegistryAuth::from_env(); for image in [ @@ -112,8 +169,8 @@ fn ghcr_private_image_still_resolves_basic_from_the_auth_file() { "ghcr", r#"{"auths":{"ghcr.io":{"auth":"dGhlamVmZmxhcnNvbjpnaHBfdG9rZW4="}}}"#, ); - clear_cred_env(); - unsafe { std::env::set_var("PROTECTOR_REGISTRY_AUTH_FILE", &path) }; + let env = EnvGuard::acquire(); + env.set("PROTECTOR_REGISTRY_AUTH_FILE", &path); let auth = RegistryAuth::from_env(); assert_basic( @@ -132,8 +189,8 @@ fn explicit_username_password_wins_over_auth_field() { "explicit", r#"{"auths":{"ghcr.io":{"username":"bot","password":"pw","auth":"aWdub3JlZDppZ25vcmVk"}}}"#, ); - clear_cred_env(); - unsafe { std::env::set_var("PROTECTOR_REGISTRY_AUTH_FILE", &path) }; + let env = EnvGuard::acquire(); + env.set("PROTECTOR_REGISTRY_AUTH_FILE", &path); let auth = RegistryAuth::from_env(); assert_basic(auth.for_image("ghcr.io/org/app"), "bot", "pw"); @@ -146,12 +203,10 @@ fn explicit_username_password_wins_over_auth_field() { #[test] fn env_override_applies_across_all_registries() { let path = write_config("envoverride", MULTI_REGISTRY); - clear_cred_env(); - unsafe { - std::env::set_var("PROTECTOR_REGISTRY_USERNAME", "envuser"); - std::env::set_var("PROTECTOR_REGISTRY_PASSWORD", "envpass"); - std::env::set_var("PROTECTOR_REGISTRY_AUTH_FILE", &path); - } + let env = EnvGuard::acquire(); + env.set("PROTECTOR_REGISTRY_USERNAME", "envuser"); + env.set("PROTECTOR_REGISTRY_PASSWORD", "envpass"); + env.set("PROTECTOR_REGISTRY_AUTH_FILE", &path); let auth = RegistryAuth::from_env(); // ghcr.io has a file entry, yet the env override still wins… @@ -165,7 +220,7 @@ fn env_override_applies_across_all_registries() { /// With no credentials of any kind, every image resolves to Anonymous — the safe default. #[test] fn anonymous_without_any_credentials() { - clear_cred_env(); + let _env = EnvGuard::acquire(); let auth = RegistryAuth::from_env(); assert!(matches!(auth.for_image("ghcr.io/org/app"), Auth::Anonymous)); assert!(matches!( @@ -177,13 +232,11 @@ fn anonymous_without_any_credentials() { /// A missing/unparseable auth file never errors — it degrades to Anonymous per image. #[test] fn missing_auth_file_degrades_to_anonymous() { - clear_cred_env(); - unsafe { - std::env::set_var( - "PROTECTOR_REGISTRY_AUTH_FILE", - "/nonexistent/protector/registry/config.json", - ) - }; + let env = EnvGuard::acquire(); + env.set( + "PROTECTOR_REGISTRY_AUTH_FILE", + "/nonexistent/protector/registry/config.json", + ); let auth = RegistryAuth::from_env(); assert!(matches!(auth.for_image("ghcr.io/org/app"), Auth::Anonymous)); }