diff --git a/.github/workflows/test-reusable.yml b/.github/workflows/test-reusable.yml index 471cea134..8e08c14ae 100644 --- a/.github/workflows/test-reusable.yml +++ b/.github/workflows/test-reusable.yml @@ -99,6 +99,36 @@ jobs: - name: Test core crate (openhuman) run: cargo test -p openhuman + rust-core-tests-windows: + if: inputs.run_rust_core + name: Rust Core Tests (Windows — secrets ACL) + runs-on: windows-latest + timeout-minutes: 20 + env: + CARGO_INCREMENTAL: '0' + SCCACHE_GHA_ENABLED: 'true' + RUSTC_WRAPPER: sccache + steps: + - name: Checkout code + uses: actions/checkout@v5 + with: + ref: ${{ inputs.ref }} + fetch-depth: 1 + submodules: recursive + - name: Cache Rust build artifacts + uses: Swatinem/rust-cache@v2 + with: + workspaces: . -> target + cache-on-failure: true + key: core-windows + - name: Install sccache + uses: mozilla-actions/sccache-action@v0.0.9 + - name: Run Windows-specific secrets tests + # Runs the full security::secrets suite including all #[cfg(windows)] + # tests: self-repair ACL path (OPENHUMAN-TAURI-GN), domain-qualified + # icacls username, is_permission_error, repair_windows_acl. + run: cargo test -p openhuman -- security::secrets --nocapture + rust-tauri-tests: if: inputs.run_rust_tauri name: Rust Tauri Shell Tests diff --git a/src/openhuman/security/secrets.rs b/src/openhuman/security/secrets.rs index b710224e5..c4f61cf1b 100644 --- a/src/openhuman/security/secrets.rs +++ b/src/openhuman/security/secrets.rs @@ -188,7 +188,32 @@ impl SecretStore { } if self.key_path.exists() { - let hex_key = read_key_file_with_retry(&self.key_path).with_context(|| { + let read_result = read_key_file_with_retry(&self.key_path); + + // On Windows a previous bad icacls invocation may have stripped the + // inherited ACEs from %APPDATA% without granting the current user + // explicit access, leaving the file permanently unreadable. Attempt + // a one-shot ACL repair via `icacls /reset` before giving up. + #[cfg(windows)] + let read_result = if let Err(ref e) = read_result { + if is_permission_error(e) { + log::warn!( + "[security] PermissionDenied reading key file '{}'; \ + attempting icacls /reset self-repair", + self.key_path.display() + ); + repair_windows_acl(&self.key_path); + // Single retry regardless of whether repair reported success — + // icacls /reset may partially restore access even on a non-zero exit. + read_key_file_with_retry(&self.key_path) + } else { + read_result + } + } else { + read_result + }; + + let hex_key = read_result.with_context(|| { let mut msg = format!( "Failed to read secret key file at {}", self.key_path.display() @@ -229,35 +254,60 @@ impl SecretStore { } #[cfg(windows)] { - // On Windows, use icacls to restrict permissions to current user only + // On Windows, use icacls to restrict permissions to current user only. + // We use USERDOMAIN\USERNAME so the account is resolved correctly on + // domain-joined and AAD-joined machines (bare USERNAME is ambiguous and + // may refer to a local account that doesn't match the signed-in user). let username = std::env::var("USERNAME").unwrap_or_default(); - let Some(grant_arg) = build_windows_icacls_grant_arg(&username) else { + let userdomain = std::env::var("USERDOMAIN").unwrap_or_default(); + let computername = std::env::var("COMPUTERNAME").unwrap_or_default(); + let qualified_username = + qualify_windows_username(&username, &userdomain, &computername); + let Some(grant_arg) = build_windows_icacls_grant_arg(&qualified_username) else { log::warn!( - "USERNAME environment variable is empty; \ + "[security] USERNAME/USERDOMAIN environment variables are empty; \ cannot restrict key file permissions via icacls" ); cache_key(&cache_key_path, &key); return Ok(key); }; - match std::process::Command::new("icacls") + let icacls_ok = match std::process::Command::new("icacls") .arg(&self.key_path) .args(["/inheritance:r", "/grant:r"]) - .arg(grant_arg) + .arg(&grant_arg) .output() { - Ok(o) if !o.status.success() => { + Ok(o) if o.status.success() => { + log::debug!("[security] key file permissions restricted via icacls"); + true + } + Ok(o) => { log::warn!( - "Failed to set key file permissions via icacls (exit code {:?})", - o.status.code() + "[security] icacls exited {:?} for account '{}'; \ + restoring inherited ACLs so the file remains readable", + o.status.code(), + grant_arg, ); + false } Err(e) => { - log::warn!("Could not set key file permissions: {e}"); - } - _ => { - log::debug!("Key file permissions restricted via icacls"); + log::warn!( + "[security] could not run icacls: {e}; \ + restoring inherited ACLs" + ); + false } + }; + // If the icacls grant command failed, the `/inheritance:r` flag may have + // already stripped the inherited ACEs that let the current user read the + // file. Explicitly reset to restore inheritance so the file is always + // readable — a slightly weaker ACL is preferable to a locked-out user. + if !icacls_ok { + let _ = std::process::Command::new("icacls") + .arg(&self.key_path) + .args(["/reset"]) + .output(); } } @@ -342,6 +392,97 @@ fn read_key_file_with_retry(path: &Path) -> std::io::Result { Err(last_err.unwrap_or_else(|| std::io::Error::other("read_to_string failed"))) } +/// Returns `true` when an `std::io::Error` is a permanent permission/access +/// denial rather than a transient sharing violation. +#[cfg(windows)] +fn is_permission_error(e: &std::io::Error) -> bool { + matches!(e.kind(), std::io::ErrorKind::PermissionDenied) || e.raw_os_error() == Some(5) + // ERROR_ACCESS_DENIED +} + +/// Attempt to repair a locked key file by running `icacls /reset` on it. +/// +/// Attempt to repair a locked key file. +/// +/// Two-step process: +/// 1. `icacls /reset` — removes all explicit ACEs and re-enables ACL +/// inheritance from the parent directory. +/// 2. `icacls /grant:r :F` — explicit grant for the current +/// user as a belt-and-suspenders fallback for environments (e.g. CI +/// temp dirs) where the parent's inheritance chain may not include the +/// runner account. +/// +/// Returns `true` if the file is actually readable after the repair attempt, +/// regardless of which step(s) succeeded. +#[cfg(windows)] +pub(super) fn repair_windows_acl(path: &Path) -> bool { + // Step 1: restore inheritance. + match std::process::Command::new("icacls") + .arg(path) + .args(["/reset"]) + .output() + { + Ok(o) if o.status.success() => { + log::info!( + "[security] icacls /reset succeeded for '{}'; ACL inheritance restored", + path.display() + ); + } + Ok(o) => { + log::warn!( + "[security] icacls /reset exited {:?} for '{}'", + o.status.code(), + path.display() + ); + } + Err(e) => { + log::warn!( + "[security] could not run icacls /reset for '{}': {e}", + path.display() + ); + } + } + + // Step 2: explicit grant for current user — handles CI environments + // where the temp/app dir's inheritable ACEs don't include the runner. + let username = std::env::var("USERNAME").unwrap_or_default(); + let userdomain = std::env::var("USERDOMAIN").unwrap_or_default(); + let computername = std::env::var("COMPUTERNAME").unwrap_or_default(); + let qualified = qualify_windows_username(&username, &userdomain, &computername); + if let Some(grant_arg) = build_windows_icacls_grant_arg(&qualified) { + match std::process::Command::new("icacls") + .arg(path) + .args(["/grant:r"]) + .arg(&grant_arg) + .output() + { + Ok(o) if o.status.success() => { + log::debug!( + "[security] explicit grant '{grant_arg}' succeeded during repair of '{}'", + path.display() + ); + } + Ok(o) => { + log::warn!( + "[security] explicit grant '{grant_arg}' exited {:?} during repair of '{}'", + o.status.code(), + path.display() + ); + } + Err(e) => { + log::warn!( + "[security] could not run icacls /grant during repair of '{}': {e}", + path.display() + ); + } + } + } + + // Return whether the file is actually readable now — callers use this + // for logging/metrics; the retry in load_or_create_key is unconditional. + std::fs::read(path).is_ok() +} + /// XOR cipher with repeating key. Same function for encrypt and decrypt. fn xor_cipher(data: &[u8], key: &[u8]) -> Vec { if key.is_empty() { @@ -381,6 +522,37 @@ fn build_windows_icacls_grant_arg(username: &str) -> Option { Some(format!("{normalized}:F")) } +/// Produce a domain-qualified Windows account name suitable for `icacls`. +/// +/// On domain-joined machines `USERDOMAIN` is the domain name and differs from +/// `COMPUTERNAME`. On standalone machines they are equal, so we use the bare +/// `username` in that case to avoid a redundant `DESKTOP-XYZ\alice` prefix. +/// +/// Returns an empty string when both inputs are empty (caller must treat this +/// as "cannot determine account name"). +#[cfg(windows)] +fn qualify_windows_username(username: &str, userdomain: &str, computername: &str) -> String { + let username = username.trim(); + let userdomain = userdomain.trim(); + let computername = computername.trim(); + + if username.is_empty() { + return String::new(); + } + + // If USERDOMAIN is set and differs from COMPUTERNAME the machine is + // domain/AAD-joined; use the fully-qualified form so icacls resolves the + // account unambiguously. + if !userdomain.is_empty() + && !computername.is_empty() + && !userdomain.eq_ignore_ascii_case(computername) + { + format!("{userdomain}\\{username}") + } else { + username.to_string() + } +} + /// Hex-decode a hex string to bytes. #[allow(clippy::manual_is_multiple_of)] fn hex_decode(hex: &str) -> Result> { diff --git a/src/openhuman/security/secrets_tests.rs b/src/openhuman/security/secrets_tests.rs index 9400ef2b3..a50d83be5 100644 --- a/src/openhuman/security/secrets_tests.rs +++ b/src/openhuman/security/secrets_tests.rs @@ -497,6 +497,253 @@ fn windows_icacls_grant_arg_preserves_valid_characters() { ); } +// ── qualify_windows_username ───────────────────────────────── + +#[cfg(windows)] +#[test] +fn qualify_windows_username_local_account() { + // USERDOMAIN == COMPUTERNAME → standalone machine → plain username + assert_eq!( + qualify_windows_username("alice", "DESKTOP-ABC", "DESKTOP-ABC"), + "alice" + ); +} + +#[cfg(windows)] +#[test] +fn qualify_windows_username_domain_joined() { + // USERDOMAIN != COMPUTERNAME → domain-joined → prefix with domain + assert_eq!( + qualify_windows_username("alice", "CORP", "DESKTOP-ABC"), + "CORP\\alice" + ); +} + +#[cfg(windows)] +#[test] +fn qualify_windows_username_case_insensitive_comparison() { + // Case-insensitive: "desktop-abc" == "DESKTOP-ABC" → local account + assert_eq!( + qualify_windows_username("bob", "desktop-abc", "DESKTOP-ABC"), + "bob" + ); +} + +#[cfg(windows)] +#[test] +fn qualify_windows_username_empty_computername() { + // COMPUTERNAME is unset — fall back to plain username to avoid prefixing + // with a potentially meaningless domain string + assert_eq!(qualify_windows_username("alice", "CORP", ""), "alice"); +} + +#[cfg(windows)] +#[test] +fn qualify_windows_username_empty_userdomain() { + // USERDOMAIN is unset — use plain username + assert_eq!( + qualify_windows_username("alice", "", "DESKTOP-ABC"), + "alice" + ); +} + +#[cfg(windows)] +#[test] +fn qualify_windows_username_empty_username_returns_empty() { + assert_eq!(qualify_windows_username("", "CORP", "DESKTOP-ABC"), ""); +} + +#[cfg(windows)] +#[test] +fn qualify_windows_username_whitespace_trimmed() { + assert_eq!( + qualify_windows_username(" alice ", " CORP ", " DESKTOP-XYZ "), + "CORP\\alice" + ); +} + +// ── Windows self-repair path ───────────────────────────────── + +/// Simulate a locked key file on non-Windows: write the file, remove all +/// read permissions, verify the store recovers after `chmod` restores them. +/// On Windows the equivalent is tested by is_permission_error / repair_windows_acl. +#[cfg(unix)] +#[test] +fn locked_key_file_fails_gracefully_on_unix() { + use std::os::unix::fs::PermissionsExt; + + let tmp = TempDir::new().unwrap(); + let store = SecretStore::new(tmp.path(), true); + + // Trigger key creation so the file exists on disk. + let encrypted = store.encrypt("original-secret").unwrap(); + assert!(store.key_path.exists()); + + // Lock the file before clearing the cache, so the next decrypt must read + // from disk and encounter the PermissionDenied error. + fs::set_permissions(&store.key_path, fs::Permissions::from_mode(0o000)).unwrap(); + + // Clear the cache so the decrypt path actually hits the disk. + super::clear_cached_key(&store.key_path); + + // Linux CI containers commonly run as root, which bypasses file permission + // checks — chmod 0o000 has no effect and the file stays readable. Only + // assert the graceful-failure behaviour when the lock actually took hold; + // otherwise the test would fail vacuously on root runners. + let file_is_locked = fs::read_to_string(&store.key_path).is_err(); + if file_is_locked { + let result = store.decrypt(&encrypted); + assert!( + result.is_err(), + "decrypt must fail gracefully when key file is locked and cache is empty" + ); + } + + // Restore permissions so TempDir cleanup can remove the file. + fs::set_permissions(&store.key_path, fs::Permissions::from_mode(0o600)).unwrap(); +} + +/// End-to-end test for the Windows self-repair path. +/// +/// Recreates the exact bad state that caused OPENHUMAN-TAURI-GN: +/// 1. Key file created, ACL corrupted with `icacls /inheritance:r` + no valid grant +/// (simulated here with an explicit `Everyone:DENY` which is even stricter). +/// 2. In-memory cache cleared so the next call must actually read from disk. +/// 3. `decrypt` is called — the self-repair path must run `icacls /reset`, +/// restore inherited ACLs, re-read the file, and return the correct plaintext. +/// +/// The lock step may be a no-op when the test process runs as SYSTEM/Administrator +/// (elevated tokens bypass DENY ACEs). In that case the test skips the +/// "verify locked" assertion and still validates that repair_windows_acl + decrypt +/// complete without panicking or returning an unexpected error. +/// +/// Run on Windows CI via the `rust-core-tests-windows` job in test-reusable.yml. +#[cfg(windows)] +#[test] +fn self_repair_recovers_from_locked_key_file() { + let tmp = TempDir::new().unwrap(); + let store = SecretStore::new(tmp.path(), true); + + // Step 1: create the key file and produce a ciphertext to decrypt later. + let encrypted = store + .encrypt("secret-to-survive-acl-lockout") + .expect("initial encrypt must succeed"); + assert!( + store.key_path.exists(), + "key file must exist after first encrypt" + ); + + // Step 2: clear the in-memory cache so the next decrypt reads from disk. + super::clear_cached_key(&store.key_path); + + // Step 3: corrupt the ACL — strip inheritance AND add an explicit DENY for + // Everyone. This is a strict superset of the production failure mode (where + // /inheritance:r ran but the /grant target was unresolvable, leaving no ACE). + let lock_status = std::process::Command::new("icacls") + .arg(&store.key_path) + .args(["/inheritance:r", "/deny"]) + .arg("Everyone:F") + .status() + .expect("icacls must be available on Windows"); + assert!( + lock_status.success(), + "icacls lock step must succeed — test setup invalid" + ); + + // Step 4: check whether the lock actually made the file unreadable. + // Elevated (SYSTEM/admin) tokens bypass DENY ACEs, so on those runners + // the file stays readable and we skip the self-repair assertion — but we + // still validate repair_windows_acl completes cleanly (no panic). + let file_is_locked = fs::read_to_string(&store.key_path).is_err(); + + if file_is_locked { + // Full E2E path: self-repair must restore access and return plaintext. + let decrypted = store + .decrypt(&encrypted) + .expect("self-repair must restore access and return correct plaintext"); + assert_eq!( + decrypted, "secret-to-survive-acl-lockout", + "decrypted value must match original" + ); + // Verify the repair is durable: clear the in-memory cache and decrypt a + // second time from disk. If the ACL is truly fixed, this succeeds on the + // first read attempt without triggering the repair path again. (A direct + // fs::read_to_string assertion here is flaky — Windows Defender / the + // Security Center can briefly re-acquire the file handle right after an + // icacls operation, causing intermittent PermissionDenied. Going through + // load_or_create_key means the retry backoff in read_key_file_with_retry + // absorbs that transient window, which is exactly what production code does.) + super::clear_cached_key(&store.key_path); + let decrypted2 = store + .decrypt(&encrypted) + .expect("ACL fix must be durable: second from-disk decrypt must succeed"); + assert_eq!( + decrypted2, "secret-to-survive-acl-lockout", + "second decrypt must return the same plaintext" + ); + } else { + // Elevated runner: lock was bypassed. Verify repair_windows_acl runs + // cleanly on an already-accessible file (icacls /reset is idempotent). + let repaired = super::repair_windows_acl(&store.key_path); + assert!( + repaired, + "repair_windows_acl must succeed on an accessible file" + ); + let decrypted = store + .decrypt(&encrypted) + .expect("decrypt must succeed when file is accessible"); + assert_eq!(decrypted, "secret-to-survive-acl-lockout"); + } +} + +/// Verify that the self-repair path does NOT trigger for non-permission errors +/// (e.g. corrupt/truncated file) — we should get a clear error, not a silent +/// retry that produces garbage. +#[cfg(windows)] +#[test] +fn self_repair_does_not_trigger_for_corrupt_file() { + let tmp = TempDir::new().unwrap(); + let store = SecretStore::new(tmp.path(), true); + + // Write a corrupt (non-hex) key file directly — simulates on-disk corruption. + fs::create_dir_all(tmp.path()).unwrap(); + fs::write(&store.key_path, "this-is-not-valid-hex!!!").unwrap(); + super::clear_cached_key(&store.key_path); + + let err = store.encrypt("anything").unwrap_err(); + let msg = format!("{err:?}"); + // Must surface a hex/corrupt error, not attempt a repair loop. + assert!( + msg.contains("corrupt") || msg.contains("hex") || msg.contains("Invalid"), + "corrupt file must surface a clear decode error, got: {msg}" + ); +} + +#[cfg(windows)] +#[test] +fn is_permission_error_matches_access_denied() { + use std::io::{Error, ErrorKind}; + let perm_err = Error::from(ErrorKind::PermissionDenied); + assert!(is_permission_error(&perm_err)); +} + +#[cfg(windows)] +#[test] +fn is_permission_error_ignores_not_found() { + use std::io::{Error, ErrorKind}; + let not_found = Error::from(ErrorKind::NotFound); + assert!(!is_permission_error(¬_found)); +} + +#[cfg(windows)] +#[test] +fn is_permission_error_matches_raw_os_error_5() { + use std::io::Error; + // raw OS error 5 = ERROR_ACCESS_DENIED + let err = Error::from_raw_os_error(5); + assert!(is_permission_error(&err)); +} + #[test] fn generate_random_key_correct_length() { let key = generate_random_key();