From 934a83fdac5138e6ee01740882dc0c108e11d0c1 Mon Sep 17 00:00:00 2001 From: Afonso Jorge Ramos Date: Tue, 11 Aug 2026 16:59:11 +0200 Subject: [PATCH 1/3] fix(secrets): enable the real keyring backends on macOS and Windows keyring 3.x ships no `default` feature contents, and every platform arm reads `not(feature = "...-native")` -> `pub use mock as default`. Naming no features therefore selected the in-memory mock on macOS and Windows, whose writes succeed and whose master key dies with the process, while `Backend::new` still reported `BackendKind::Keyring`. The consequence is that every offline track's content key becomes undecryptable on the next launch, so playback treats it as a cache miss and silently re-streams what the user had already downloaded. Linux is left off the list because qbz-credentials already enables `async-secret-service` and Cargo unifies features; naming `sync-secret-service` here would trip keyring's own compile_error for enabling both halves of a store. Proven with a probe wrapping in one process and unwrapping in another, on the same machine, changing only this line: before: backend = Keyring, UNWRAP FAILED: cipher error: aead::Error after: backend = Keyring, UNWRAPPED OK --- crates/Cargo.lock | 19 ++++++++++++++++++- crates/qbz-secrets/Cargo.toml | 26 +++++++++++++++++--------- 2 files changed, 35 insertions(+), 10 deletions(-) diff --git a/crates/Cargo.lock b/crates/Cargo.lock index b6aa3284d..638e15a25 100644 --- a/crates/Cargo.lock +++ b/crates/Cargo.lock @@ -3931,9 +3931,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", ] @@ -7292,7 +7296,7 @@ dependencies = [ "openssl-probe", "rustls-pki-types", "schannel", - "security-framework", + "security-framework 3.7.0", ] [[package]] @@ -7415,6 +7419,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..245446fca 100644 --- a/crates/qbz-secrets/Cargo.toml +++ b/crates/qbz-secrets/Cargo.toml @@ -32,13 +32,21 @@ 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`. +# OS keyring backends: secret-service (Linux), Keychain (macOS), Credential +# Manager (Windows). **Desktop only, deliberately** — see the target gate below. +# +# 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 is absent from this list because it is already covered: qbz-credentials +# enables `async-secret-service` and Cargo unifies features across the graph. +# Naming `sync-secret-service` here would be a build failure rather than belt +# and braces — keyring's lib.rs has a `compile_error!` for enabling both the +# sync and async halves of a store at once. [target.'cfg(not(any(target_os = "android", target_os = "ios")))'.dependencies] -keyring = "3" +keyring = { version = "3", features = ["apple-native", "windows-native"] } From 99ba4aa48e40236837c7ff777cea9ce3c668e941 Mon Sep 17 00:00:00 2001 From: Afonso Jorge Ramos Date: Tue, 11 Aug 2026 17:08:53 +0200 Subject: [PATCH 2/3] test(secrets): prove a wrapped secret survives a process restart MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The property this crate exists for is the one no in-process test could show. A single process wraps and unwraps happily against a master key that never left memory, which is exactly what keyring's mock hands back — so the whole suite stayed green while every offline download lost its content key on the next launch. The test re-executes the test binary: wrap in one child, unwrap in another. It asserts persistence and not which backend provided it, so the KDF fallback passes it too and a runner without secret-service is a pass rather than a flake, while the mock fails everywhere. The existing job is Linux-only, which is where these crates are least likely to break — both bugs it now guards shipped on macOS while Linux was fine. Adds a narrow macOS job for the two platform-sensitive crates: no ALSA/JACK/D-Bus, no UI graph, a couple of minutes on a 10x runner. --- .github/workflows/test-crates.yml | 45 +++++++ .../qbz-secrets/tests/survives_a_restart.rs | 116 ++++++++++++++++++ 2 files changed, 161 insertions(+) create mode 100644 crates/qbz-secrets/tests/survives_a_restart.rs diff --git a/.github/workflows/test-crates.yml b/.github/workflows/test-crates.yml index b2f4ffcb7..8c22eb6d8 100644 --- a/.github/workflows/test-crates.yml +++ b/.github/workflows/test-crates.yml @@ -103,3 +103,48 @@ jobs: if [ -n "$hits" ]; then echo "::error::qbzd graph resolves Slint crates:"; echo "$hits"; exit 1 fi + + # The job above is Linux-only, which is where the platform-sensitive crates + # are *least* likely to break: the two bugs this job exists for both shipped + # on macOS while Linux was fine. + # + # - qbz-secrets picks a keyring backend per platform, and picked an + # in-memory mock on macOS and Windows whose writes silently succeed. + # - qbz-offline-cache resolves bundle paths under an app directory that + # only moves on macOS-derived platforms (iOS reassigns the data + # container; the desktop cache root is stable). + # + # Deliberately narrow: two crates, no ALSA/JACK/D-Bus, no UI graph, so this + # stays a couple of minutes on a runner billed at 10x Linux. Add + # `windows-latest` to the matrix if the same class of bug turns up there — + # the tests are already platform-agnostic. + platform-sensitive: + name: cargo test (secrets + offline cache, ${{ matrix.os }}) + strategy: + fail-fast: false + matrix: + os: [macos-14] + runs-on: ${{ matrix.os }} + timeout-minutes: 15 + steps: + - name: Checkout + uses: actions/checkout@v7 + + - name: Setup Rust + uses: dtolnay/rust-toolchain@stable + + - name: Rust cache + uses: Swatinem/rust-cache@v2 + with: + workspaces: crates -> target + shared-key: test-crates-${{ matrix.os }} + cache-on-failure: true + save-if: ${{ github.ref == 'refs/heads/pre-release' }} + + - name: cargo test + env: + CARGO_TERM_COLOR: always + CARGO_BUILD_JOBS: "3" + run: | + cargo test --manifest-path crates/Cargo.toml \ + -p qbz-secrets -p qbz-offline-cache --no-fail-fast 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..3b1adf610 --- /dev/null +++ b/crates/qbz-secrets/tests/survives_a_restart.rs @@ -0,0 +1,116 @@ +//! 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(not(any(target_os = "android", target_os = "ios")))] +fn forget(service: &str) { + if let Ok(entry) = keyring::Entry::new(service, "master-key-v1") { + let _ = entry.delete_credential(); + } +} + +#[cfg(any(target_os = "android", target_os = "ios"))] +fn forget(_service: &str) {} From 7814d4d2d5b7a69a387f40e7c9fccc8d3b7ae506 Mon Sep 17 00:00:00 2001 From: Afonso Jorge Ramos Date: Wed, 12 Aug 2026 03:21:26 +0200 Subject: [PATCH 3/3] fix(secrets): keep the unit tests off the OS keyring Asking for real keyring backends made the four round-trip tests write a genuine login-Keychain item, and macOS binds an item's ACL to the exact binary that created it. So every run after the first, with any source edit in between, was a new binary against an existing item, and the OS blocked all four tests on an authorization dialog that a headless run can never answer. Reproduced: four tests hanging past 60s, where a fresh keychain passes in 0.39s. The comment claiming the sandboxed tempdir guaranteed a fresh state was the thing that hid it. try_open_keyring ignores storage_dir entirely and looks the entry up by service name alone, so the tempdir bought nothing. Backend::kdf_only pins them to the path they were always documented as taking. What they assert, wrap/unwrap round-trips and tamper detection, is identical on either backend, so nothing is lost. survives_a_restart.rs still exercises the real backend, which is where it belongs: persistence across processes is what that test is about. Also names Linux's backend here instead of inheriting it. qbz-credentials enables async-secret-service and Cargo unifies features, which holds for every shipping build but makes this crate's correctness depend on a sibling: built on its own, cargo tree showed keyring with no secret-service, i.e. the same silent mock this PR exists to remove. Verified: secret-service v4.0.0 on Linux, security-framework on macOS, and still zero keyring on iOS and Android. --- crates/qbz-secrets/Cargo.toml | 24 ++++++++++++++++++------ crates/qbz-secrets/src/backend.rs | 18 ++++++++++++++++++ crates/qbz-secrets/src/lib.rs | 23 +++++++++++++++-------- 3 files changed, 51 insertions(+), 14 deletions(-) diff --git a/crates/qbz-secrets/Cargo.toml b/crates/qbz-secrets/Cargo.toml index 245446fca..7611b6e8a 100644 --- a/crates/qbz-secrets/Cargo.toml +++ b/crates/qbz-secrets/Cargo.toml @@ -43,10 +43,22 @@ tempfile = "3" # the next. `Backend::new` still reports `BackendKind::Keyring`, so the logs # say nothing is wrong. # -# Linux is absent from this list because it is already covered: qbz-credentials -# enables `async-secret-service` and Cargo unifies features across the graph. -# Naming `sync-secret-service` here would be a build failure rather than belt -# and braces — keyring's lib.rs has a `compile_error!` for enabling both the -# sync and async halves of a store at once. +# Linux names its backend here too, rather than relying on qbz-credentials +# enabling `async-secret-service` and Cargo unifying features across the graph. +# That does hold for any build containing qbz-credentials, which today is every +# shipping one, but it makes this crate's correctness depend on a sibling: build +# qbz-secrets on its own and `cargo tree` shows keyring with no secret-service, +# which is the silent mock described above. A crate meant to be reusable should +# not be one dependency-graph change away from storing secrets in a hashmap. +# +# `async-secret-service` and not `sync-secret-service`: keyring's lib.rs has a +# `compile_error!` for enabling both halves of a store, and qbz-credentials +# already picked the async one. `crypto-rust` is not optional, secret-service +# does not build without a crypto backend selected. [target.'cfg(not(any(target_os = "android", target_os = "ios")))'.dependencies] -keyring = { version = "3", features = ["apple-native", "windows-native"] } +keyring = { version = "3", features = [ + "apple-native", + "windows-native", + "async-secret-service", + "crypto-rust", +] } diff --git a/crates/qbz-secrets/src/backend.rs b/crates/qbz-secrets/src/backend.rs index 9a66198aa..e4f721fb3 100644 --- a/crates/qbz-secrets/src/backend.rs +++ b/crates/qbz-secrets/src/backend.rs @@ -87,6 +87,24 @@ 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. + pub 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 } diff --git a/crates/qbz-secrets/src/lib.rs b/crates/qbz-secrets/src/lib.rs index 80c2838a3..655ac0195 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]