diff --git a/crates/Cargo.lock b/crates/Cargo.lock index 2381facfa..f4425bd9d 100644 --- a/crates/Cargo.lock +++ b/crates/Cargo.lock @@ -2690,9 +2690,13 @@ version = "3.6.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "eebcc3aff044e5944a8fbaf69eb277d11986064cba30c468730e8b9909fb551c" dependencies = [ + "byteorder", "dbus-secret-service", "log", "secret-service", + "security-framework 2.11.1", + "security-framework 3.7.0", + "windows-sys 0.60.2", "zbus 4.4.0", "zeroize", ] @@ -5351,7 +5355,7 @@ dependencies = [ "openssl-probe", "rustls-pki-types", "schannel", - "security-framework", + "security-framework 3.7.0", ] [[package]] @@ -5442,6 +5446,19 @@ dependencies = [ "zbus 4.4.0", ] +[[package]] +name = "security-framework" +version = "2.11.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "897b2245f0b511c87893af39b033e5ca9cce68824c4d7e7630b5a1d339658d02" +dependencies = [ + "bitflags 2.13.0", + "core-foundation 0.9.4", + "core-foundation-sys", + "libc", + "security-framework-sys", +] + [[package]] name = "security-framework" version = "3.7.0" diff --git a/crates/qbz-secrets/Cargo.toml b/crates/qbz-secrets/Cargo.toml index 377ef764c..2cd7f9f2d 100644 --- a/crates/qbz-secrets/Cargo.toml +++ b/crates/qbz-secrets/Cargo.toml @@ -32,13 +32,32 @@ log = { workspace = true } [dev-dependencies] tempfile = "3" -# OS keyring backends: libsecret/secret-service (Linux), Keychain (macOS), -# DPAPI (Windows). **Desktop only, deliberately.** keyring 3.x compiles no real -# backend for Android, and none for iOS without its `apple-native` feature — it -# routes both to an in-memory `mock` whose writes *succeed*. Depending on it -# there would not fail loudly; it would hand back a master key that never -# leaves the process, so every blob wrapped in one run is undecryptable in the -# next. Mobile takes the KDF path, which is derived from a persisted install id -# and is stable across launches. See `backend.rs`. -[target.'cfg(not(any(target_os = "android", target_os = "ios")))'.dependencies] -keyring = "3" +# OS keyring backends: Secret Service (Linux), Keychain (macOS), Credential +# Manager (Windows). These are the only targets allowed to attempt a keyring. +# +# Every backend is opt-in. keyring 3.x ships no `default` feature contents at +# all, and each platform arm reads `not(feature = "…-native")` → `pub use mock +# as default`. That mock is an in-memory store whose writes *succeed*, so +# depending on it does not fail loudly: it hands back a master key that never +# outlives the process, and every blob wrapped in one run is undecryptable in +# the next. `Backend::new` still reports `BackendKind::Keyring`, so the logs +# say nothing is wrong. +# +# Linux names its backend here rather than relying on qbz-credentials and Cargo +# feature unification. macOS and Windows opt into only their native stores. +# Mobile and every other target omit `keyring` entirely and take the persisted +# KDF path in `backend.rs`; an unsupported target must never reach the mock. +# +# `async-secret-service` and not `sync-secret-service`: keyring rejects enabling +# both. `crypto-rust` is required by the Secret Service backend. +[target.'cfg(target_os = "linux")'.dependencies] +keyring = { version = "3", default-features = false, features = [ + "async-secret-service", + "crypto-rust", +] } + +[target.'cfg(target_os = "macos")'.dependencies] +keyring = { version = "3", default-features = false, features = ["apple-native"] } + +[target.'cfg(target_os = "windows")'.dependencies] +keyring = { version = "3", default-features = false, features = ["windows-native"] } diff --git a/crates/qbz-secrets/src/backend.rs b/crates/qbz-secrets/src/backend.rs index 9a66198aa..dacc4724c 100644 --- a/crates/qbz-secrets/src/backend.rs +++ b/crates/qbz-secrets/src/backend.rs @@ -5,15 +5,13 @@ //! AES-256-GCM wraps. Failure (for any reason) = we fall back to HKDF //! over device identifiers. //! -//! **Mobile skips the keyring entirely and goes straight to the KDF path.** -//! Not a preference — a correctness requirement. `keyring` 3.x compiles no real -//! backend for Android, and none for iOS without its `apple-native` feature; -//! both resolve to an in-memory `mock`. The mock's writes *succeed*, so the -//! attempt below would not fail into the fallback: it would report success and -//! hand back a key that dies with the process, silently making every blob -//! wrapped in one run undecryptable in the next. The KDF path derives from a -//! persisted install id and is stable across launches, which is what a phone -//! needs. `keyring` is a desktop-only dependency for the same reason. +//! **Only Linux, macOS and Windows consult the keyring.** Every other target +//! goes straight to the KDF path. This is a correctness requirement: without a +//! native feature, `keyring` 3.x resolves to an in-memory `mock` whose writes +//! *succeed*. Such a call would not fail into the fallback; it would hand back a +//! key that dies with the process, silently making every blob wrapped in one run +//! undecryptable in the next. The KDF path derives from a persisted install id +//! and is stable across launches. //! //! The backend discriminator is baked into every wrapped blob so a blob //! produced by one backend can't be silently decrypted by another (and @@ -24,7 +22,7 @@ use std::path::Path; use hkdf::Hkdf; // Only the keyring path mints a random master key; the KDF path derives one. -#[cfg(not(any(target_os = "android", target_os = "ios")))] +#[cfg(any(target_os = "linux", target_os = "macos", target_os = "windows"))] use rand::RngCore; use sha2::Sha256; @@ -33,7 +31,7 @@ use crate::error::SecretError; use crate::install_id; const MASTER_KEY_LEN: usize = 32; -#[cfg(not(any(target_os = "android", target_os = "ios")))] +#[cfg(any(target_os = "linux", target_os = "macos", target_os = "windows"))] const KEYRING_ENTRY_NAME: &str = "master-key-v1"; const HKDF_INFO: &[u8] = b"qbz-secrets master-key derivation v1"; @@ -60,9 +58,9 @@ pub struct Backend { impl Backend { pub fn new(service_name: &str, storage_dir: &Path) -> Result { - // Try keyring first — desktop only; see the module comment for why - // mobile must not, and why a mock's *success* is the dangerous case. - #[cfg(not(any(target_os = "android", target_os = "ios")))] + // Try keyring only where a real backend is selected at compile time; + // see the module comment for why a mock's success is dangerous. + #[cfg(any(target_os = "linux", target_os = "macos", target_os = "windows"))] match try_open_keyring(service_name) { Ok(master_key) => { log::info!("[qbz-secrets] Using OS keyring backend"); @@ -87,6 +85,25 @@ impl Backend { }) } + /// The KDF path only, never consulting the OS keyring. + /// + /// This exists for tests. Reaching the real Keychain from a unit test is + /// not a stronger test, it is a flaky one: macOS binds a keychain item's + /// ACL to the exact binary that created it, so the *second* `cargo test` + /// after any source edit is a new binary against an existing item, and the + /// OS blocks the process on an authorization dialog that a headless or CI + /// run can never answer. What the tests are about, wrap/unwrap round-trips + /// and tamper detection, is identical on either backend, so there is + /// nothing to be gained by rolling that dice. + #[cfg(test)] + pub(crate) fn kdf_only(service_name: &str, storage_dir: &Path) -> Result { + let master_key = derive_fallback_key(service_name, storage_dir)?; + Ok(Self { + kind: BackendKind::KdfFallback, + master_key, + }) + } + pub fn kind(&self) -> BackendKind { self.kind } @@ -109,7 +126,7 @@ impl Backend { } /// Read (or create and store) the master key from the OS keyring. -#[cfg(not(any(target_os = "android", target_os = "ios")))] +#[cfg(any(target_os = "linux", target_os = "macos", target_os = "windows"))] fn try_open_keyring(service_name: &str) -> Result<[u8; MASTER_KEY_LEN], SecretError> { use base64::engine::general_purpose::STANDARD as B64; use base64::Engine; diff --git a/crates/qbz-secrets/src/lib.rs b/crates/qbz-secrets/src/lib.rs index 80c2838a3..9bb09c1c7 100644 --- a/crates/qbz-secrets/src/lib.rs +++ b/crates/qbz-secrets/src/lib.rs @@ -140,14 +140,21 @@ mod tests { fn test_vault() -> (SecretBox, TempDir) { let dir = TempDir::new().expect("tempdir"); - // Use a service name that's extremely unlikely to collide with a - // real keyring entry on the dev machine. The KDF fallback path is - // exercised by design because the sandboxed tempdir + nonexistent - // entry guarantees a fresh state; the keyring may be reachable - // but we accept either backend — the round-trip still holds. - let vault = SecretBox::open("qbz-secrets-test-harness", dir.path()) - .expect("open vault"); - (vault, dir) + // Pinned to the KDF path rather than letting `open` pick. The old + // comment here claimed the sandboxed tempdir guaranteed a fresh state + // and so exercised the fallback by design; that was never true, because + // `try_open_keyring` ignores `storage_dir` entirely and looks the entry + // up by service name alone. Once this crate asks for real keyring + // backends, these four tests wrote a genuine login-Keychain item, and + // every subsequent run built a new test binary that macOS then blocked + // on an authorization dialog: four tests hanging forever, on every dev + // machine, from the second run onwards. + // + // `survives_a_restart.rs` is where the real backend belongs, because + // persistence across processes is what it is actually testing. + let backend = + Backend::kdf_only("qbz-secrets-test-harness", dir.path()).expect("kdf backend"); + (SecretBox::from_backend(backend), dir) } #[test] diff --git a/crates/qbz-secrets/tests/survives_a_restart.rs b/crates/qbz-secrets/tests/survives_a_restart.rs new file mode 100644 index 000000000..7b554aa08 --- /dev/null +++ b/crates/qbz-secrets/tests/survives_a_restart.rs @@ -0,0 +1,118 @@ +//! The one property this crate exists for, and the one no in-process test can +//! demonstrate: a secret wrapped by one run of the app can be unwrapped by the +//! next. +//! +//! Everything else here passes whether or not that holds. A single process can +//! wrap and unwrap against a master key that never left memory and never +//! reached a store, which is exactly what keyring's `mock` backend hands back +//! when no platform feature is enabled — silently, while `Backend::new` reports +//! `BackendKind::Keyring`. That shipped on macOS and Windows and cost every +//! offline download its content key on the next launch. +//! +//! So this test crosses a process boundary. It re-executes the test binary, +//! wraps in the child, then re-executes again and unwraps in a second child. +//! +//! Deliberately **backend-agnostic**: it asserts persistence, not which store +//! provided it. The KDF fallback satisfies it too (its install id is on disk), +//! so a runner with no secret-service is a pass rather than a flake — while the +//! mock, which satisfies neither, fails everywhere. + +use std::path::Path; +use std::process::Command; + +/// Set on the children. Its value is the half of the exchange to perform. +const ROLE: &str = "QBZ_SECRETS_RESTART_ROLE"; +const SERVICE: &str = "QBZ_SECRETS_RESTART_SERVICE"; +const DIR: &str = "QBZ_SECRETS_RESTART_DIR"; + +const SECRET: &[u8; 16] = b"0123456789abcdef"; + +#[test] +fn a_wrapped_secret_survives_a_process_restart() { + if let Ok(role) = std::env::var(ROLE) { + run_child(&role); + return; + } + + let dir = tempfile::tempdir().expect("tempdir"); + // Unique per run so concurrent jobs — and a developer's real qbz entry — + // never collide. Cleaned up at the end. + let service = format!( + "qbz-secrets-test-{}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .expect("clock") + .as_nanos() + ); + + let wrapped = spawn_self("wrap", &service, dir.path()); + assert!( + wrapped.status.success(), + "the first run could not wrap a secret:\n{}", + String::from_utf8_lossy(&wrapped.stderr) + ); + + let unwrapped = spawn_self("unwrap", &service, dir.path()); + let stderr = String::from_utf8_lossy(&unwrapped.stderr).into_owned(); + + forget(&service); + + assert!( + unwrapped.status.success(), + "a secret wrapped by one process could not be unwrapped by the next — \ + the master key did not outlive the process that made it:\n{stderr}" + ); +} + +/// Re-execute this test binary with `ROLE` set, so the child runs the body +/// above and takes the branch at the top. +fn spawn_self(role: &str, service: &str, dir: &Path) -> std::process::Output { + Command::new(std::env::current_exe().expect("current_exe")) + .args([ + "--exact", + "a_wrapped_secret_survives_a_process_restart", + "--nocapture", + ]) + .env(ROLE, role) + .env(SERVICE, service) + .env(DIR, dir) + .output() + .expect("re-exec the test binary") +} + +fn run_child(role: &str) { + let service = std::env::var(SERVICE).expect(SERVICE); + let dir = std::env::var(DIR).expect(DIR); + let blob = Path::new(&dir).join("wrapped.bin"); + + let vault = qbz_secrets::SecretBox::open(&service, Path::new(&dir)) + .unwrap_or_else(|e| panic!("open the vault: {e}")); + eprintln!("child role={role} backend={:?}", vault.backend_kind()); + + match role { + "wrap" => { + let wrapped = vault.wrap(SECRET).unwrap_or_else(|e| panic!("wrap: {e}")); + std::fs::write(&blob, wrapped).expect("write the wrapped blob"); + } + _ => { + let wrapped = std::fs::read(&blob).expect("read the wrapped blob"); + let plain = vault + .unwrap(&wrapped) + .unwrap_or_else(|e| panic!("unwrap: {e}")); + assert_eq!(plain, SECRET, "the secret came back changed"); + } + } +} + +/// Drop the entry this run created. Best-effort: the KDF fallback never made +/// one, and a store that refuses the delete is not this test's business. +#[cfg(any(target_os = "linux", target_os = "macos", target_os = "windows"))] +fn forget(service: &str) { + if let Ok(entry) = keyring::Entry::new(service, "master-key-v1") { + let _ = entry.delete_credential(); + } +} + +#[cfg(not(any(target_os = "linux", target_os = "macos", target_os = "windows")))] +fn forget(_service: &str) {}