fix(secrets): integrate real desktop keyring backends (#697)

Preserve Afonso’s restart regression test while selecting native stores only on Linux, macOS and Windows. Unsupported targets take the persisted KDF path instead of keyring’s successful in-memory mock; the stale workflow change is deliberately excluded.

cx

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"

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"] }

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<Self, SecretError> {
-        // 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<Self, SecretError> {
+        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;

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]

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) {}