Skip to content

Commit 562c200

Browse files
authored
test: isolate the keychain from the test suite (#214)
Rebased onto `main` @ 9d67765 (after #201, #216, #213, #212, #203, #219). #207 already fixed the root cause for the two crates it touched, so this PR is now the **audit**, the **consolidation**, and the **docs** - the three halves it did not cover. ## Root cause (recap, for context) `keyring` 4.x's `Entry::new` installs the PLATFORM-NATIVE store as the process default on its **first** call, overwriting whatever default is already set (`keyring-4.1.5/src/v1.rs`, the `SET_CREDENTIAL_STORE` latch). So installing `keyring-core`'s mock *before* the first real `Entry` is silently undone, and every "mock" write lands in the real OS keychain. On macOS each rebuild is a new binary identity, so the OS re-prompts on every single rebuild and the run blocks on the modal dialog. Confirmed against the maintainer's login keychain before touching anything - it held the tests' own hard-coded account ids: ``` driven.google.refresh_token / acct-with-token (created 20260729180400Z) driven.google.client_creds / acct-byo driven.s3.credentials / acct-s3-round-trip (created 20260729181820Z) ``` ## 1. The audit: are there any other paths? **Verified answer: no. Every test that reaches a real `keyring::Entry` is now behind the shared helper - there are no unisolated paths left.** Done empirically rather than by inspection. A temporary probe panicked at all **four** `Entry` construction sites in the workspace - `driven-crypto/src/keystore.rs:63`, `driven-drive/src/google/token_store.rs:102` and `:157`, `driven-s3/src/config.rs:221` - and `cargo test --workspace --no-fail-fast` ran every test binary on this exact tree. Exactly **13** tests reach an entry, across 5 binaries: | Crate | Tests reaching an entry | Services touched | |---|---|---| | `driven-backend` | 3 (pre-existing) | `driven.google.refresh_token`, `driven.google.client_creds` | | `driven-s3` | 4 (pre-existing) | `driven.s3.credentials` | | `driven-crypto` | 3 (new here) | `dev.maxhogan.driven` | | `driven-drive` | 2 (new here) | `driven.google.refresh_token`, `driven.google.client_creds` | | `src-tauri` | 1 (new here) | `dev.maxhogan.driven` | All 13 go through `driven_test_fixtures::keychain::isolated()`. The probe hit list and the isolated-test list match exactly, with no remainder. Also checked and clear: `driven-cli` (its integration tests only exercise `--help` / missing-argument paths, so the spawned binary never opens an entry), every integration test under `crates/*/tests` and `src-tauri/tests`, and the two new crates from this week - `driven-localfs` and `driven-rclone` reference neither `keyring` nor any of the credential-store types. ## 2. Consolidation: one helper, not three `driven_test_fixtures::keychain` (`crates/driven-test-fixtures/src/keychain.rs`) is now the single implementation. Both of #207's local copies are repointed at it and `keyring-core` is dropped from both crates' dev-deps: - `crates/driven-backend/src/lib.rs` - 60-line local helper -> one-line delegate - `crates/driven-s3/src/config.rs` - same It keeps #207's load-bearing ordering (burn the latch with a throwaway `Entry`, *then* install the mock) and adds one thing #207's version lacks: **An I/O-free precondition ahead of the proof write.** The installed default store must report `CredentialPersistence::ProcessOnly`, which by definition means its credentials cannot outlive the process; a real OS keychain reports `UntilDelete`/`UntilReboot`. So a defeated mock is caught **before anything is written**. Under #207's version the sentinel write is itself the first thing that would leak into a real keychain when the mock fails. The sentinel round trip still runs, after the precondition passes, as the functional check. Constructing the throwaway `Entry` is safe: `build` in `apple-native-keyring-store-1.0.1/src/keychain.rs` is pure struct construction with no keychain I/O, so it raises no prompt and creates nothing. **Drift protection:** `the_test_suite_is_isolated_from_the_os_keychain` in every crate that owns a keychain call site - `driven-crypto`, `driven-drive`, `driven-s3`, `driven-backend`, `src-tauri`. These *assert* rather than skip, so a future `keyring` upgrade that breaks the mechanism fails loudly instead of the suite quietly resuming real writes. All five crates now have the dev-dep wired, so isolating a new test is a one-line change with no `Cargo.toml` work. Production is untouched: no shipped code path changed, and `keyring-core` is a direct dependency only of `driven-test-fixtures`, which is `publish = false` and only ever a `[dev-dependencies]` entry. ## 3. The docs half ### README - new "macOS re-prompts for keychain access after every update" Placed in the existing macOS caveats, between the APFS locked-file section and the auto-updater caveat. Verified, not restated: - Driven's macOS build carries **no Developer ID signature**. It is only ad-hoc (linker) signed, so it has no stable [designated requirement](https://developer.apple.com/library/archive/technotes/tn2206/_index.html) and its cdhash changes with every build. (`tauri.conf.json` sets no `signingIdentity`; `release.yml` runs no `codesign`.) - macOS pins keychain ACLs to the identity of the binary that was granted access. Apple states the rule directly: ["This dialog appears if you recently updated your system software or the app, or if the app has been modified"](https://support.apple.com/guide/keychain-access/if-a-trusted-app-asks-for-keychain-access-kyca1331/mac). So "Always Allow" does not carry across an update, for any of the four services Driven uses (`dev.maxhogan.driven`, `driven.google.refresh_token`, `driven.google.client_creds`, `driven.s3.credentials`). - Denying it is safe by construction - an encrypted source whose master key cannot be read fails closed (`crypto.key_missing`) rather than uploading plaintext - but it stalls encrypted sources until the user re-authorizes. - The only fix is a Developer ID signature. Stated without overstating: no entitlement or partial workaround makes an ad-hoc-signed build's grants survive an update. **Aligned with #216, not contradicting it.** #216's `fdaBanner.unsignedNote` already covers the TCC/Full Disk Access half ("macOS ties this permission to the app's signature, and Driven is not signed yet ... remove Driven from the Full Disk Access list and add it back"). The README's FDA bullet gives the same mechanism and the same remove-and-re-add remedy, and now explicitly says the in-app banner says the same thing. The genuinely new material is the **keychain** half, which nothing documented. ### DESIGN - 3.6 gains "macOS, second cost: permission grants do not survive an update" next to the existing "no Apple Developer ID" material. It defers the TCC half to 5.3.3 rather than restating it, and documents the keychain half. - 5.3.3 (#216's FDA onboarding section, which already explains the cdhash binding for TCC) gains a short pointer noting the same binding governs keychain ACLs, with a cross-reference to 3.6. ### CONTRIBUTING A local-gates subsection: the suite is keychain-isolated, how to isolate a new test, why macOS makes it load-bearing, and cleanup commands for anyone who ran the suite while the flawed helper was on `main`. ## Bonus: coverage that was previously impossible The old module docs in `driven-crypto/src/keystore.rs` and `driven-drive/src/google/token_store.rs` claimed the mock store was unusable and steered contributors away from testing these paths. Both are corrected, and the paths they excluded are now covered for real against the in-memory store: - `Keystore` store/load/delete master key, `NotFound` on a wiped keychain, idempotent delete, per-account scoping - `KeyringTokenStore` and `ClientCredsStore` round trips, incl. the empty-secret PKCE case and per-account isolation - `KeystoreCryptoProvider` with an encrypted source that HAS a wrapped key but no master key - the only path there that actually opens the keystore, which no existing test reached (every existing one short-circuits on a missing wrapped key). This is the GA-critical fail-closed rule. ## Verification: zero keychain prompts Prompts are interactive and the machine is locked, so this is proved, not asserted. Headless throughout - no GUI automation, no app launch. 1. **Complete audit.** The 4-site panic probe above enumerated every test in the workspace that constructs a keychain entry; all 13 are isolated. 2. **The five guard tests pass**, so the mock really is the effective default store in each of those binaries - the exact thing that was silently false. 3. **mdat unchanged.** After a full `cargo test --workspace`, the S3 item's `mdat` is still `20260729181820Z` - unchanged across multiple full runs and a targeted `cargo test -p driven-s3`, whose `credentials_round_trip_through_the_keychain` stores to that exact account. Nothing wrote. 4. **Nothing created.** The two Google items were cleaned up mid-session, so runs since then started from an empty state for those services. After the final full run, `dev.maxhogan.driven`, `driven.google.refresh_token`, `driven.google.client_creds`, the helper's own `driven.test.keyring-latch` / `driven.test.keyring-sentinel` and the test round-trip service are **all absent** - despite the suite performing real store/load/delete round trips against every production service. 5. **No dialog.** Every run executed non-interactively in the background and exited on its own, so nothing blocked on a modal prompt. ## Gates (all re-run after the rebase onto 9d67765) - `SQLX_OFFLINE=true cargo test --workspace` - pass, 0 failures - `SQLX_OFFLINE=true cargo clippy --workspace --all-targets -- -D warnings` - clean - `cargo fmt --all -- --check` - clean - `git diff --check` - clean - `cargo deny check` - `advisories ok, bans ok, licenses ok, sources ok` - No em/en dashes in the diff (checked) - UI suite not run: no UI file is touched by this PR. Note: `test:` is a hidden changelog type here, so the user-facing README/DESIGN macOS caveat will not appear in the release notes. Flagging deliberately - a follow-up `docs:` commit is your call.
1 parent 7d36ef0 commit 562c200

17 files changed

Lines changed: 658 additions & 154 deletions

File tree

CONTRIBUTING.md

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,61 @@ example real-Google-Drive end-to-end tests with no credentials, or VSS /
105105
elevation tests without admin). A clean run is all-pass plus those honest skips,
106106
not a hidden failure.
107107

108+
### The test suite is isolated from your OS keychain
109+
110+
`cargo test --workspace` must never touch your real login keychain, and it
111+
does not. Any test that can reach a keychain entry - Driven stores the account
112+
master key under `dev.maxhogan.driven`, the Google secrets under
113+
`driven.google.refresh_token` / `driven.google.client_creds`, and S3 key pairs
114+
under `driven.s3.credentials` - starts with:
115+
116+
```rust
117+
let Some(_guard) = driven_test_fixtures::keychain::isolated() else {
118+
return; // could not isolate; skip rather than write to a real keychain
119+
};
120+
```
121+
122+
`isolated()` installs `keyring-core`'s in-memory mock as the process-global
123+
default credential store, proves it is the effective store before the test is
124+
allowed to store anything, and returns `None` (so the test skips honestly) if it
125+
cannot. Add that line to any new test that reaches the keychain, and add
126+
`driven-test-fixtures` to your crate's `[dev-dependencies]` if it is not there
127+
yet. Every crate that owns a keychain call site - `driven-crypto`,
128+
`driven-drive`, `driven-s3`, `driven-backend`, `src-tauri` - already has the
129+
wiring and a
130+
`the_test_suite_is_isolated_from_the_os_keychain` guard test, so if the
131+
mechanism ever breaks (a `keyring` upgrade, say) those fail loudly instead of
132+
the suite quietly starting to write for real.
133+
134+
**Why this matters more on macOS.** macOS ties a keychain ACL to the identity
135+
of the binary that was granted access, and every `cargo test` rebuild produces a
136+
binary with a new identity. So a single test that reaches the real keychain
137+
raises a modal "allow ... to access ..." prompt on *every* rebuild - clicking
138+
"Always Allow" does not help, because the next build is a different app - and
139+
the run blocks on the dialog until you answer it. (The same mechanism means
140+
released builds re-prompt users on every update; see the macOS notes in the
141+
README.) Ordering is load-bearing here: `keyring` 4.x's `Entry::new` installs
142+
the platform-native store on its *first* call and overwrites whatever default is
143+
already set, so installing the mock without first burning that latch is silently
144+
undone. `crates/driven-test-fixtures/src/keychain.rs` documents the sequence -
145+
use the helper rather than re-deriving it.
146+
147+
If you ran the suite before this isolation landed, you may have leftover
148+
test-only items in your login keychain. Remove them with:
149+
150+
```sh
151+
security delete-generic-password -s "driven.google.refresh_token" -a "acct-with-token"
152+
security delete-generic-password -s "driven.google.client_creds" -a "acct-byo"
153+
security delete-generic-password -s "driven.s3.credentials" -a "acct-s3-round-trip"
154+
security delete-generic-password -s "driven.s3.credentials" -a "acct-a"
155+
security delete-generic-password -s "driven.s3.credentials" -a "acct-b"
156+
security delete-generic-password -s "driven.s3.credentials" -a "acct-s3-delete"
157+
security delete-generic-password -s "driven.s3.credentials" -a "acct-control-chars"
158+
```
159+
160+
A `security: ... could not be found in the keychain` for any of these just means
161+
that one never leaked on your machine.
162+
108163
## Coverage gate
109164

110165
The `coverage` workflow (`.github/workflows/coverage.yml`) measures line

Cargo.lock

Lines changed: 7 additions & 3 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

README.md

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -272,6 +272,41 @@ unmounts. It works without Time Machine being set up.
272272
> would already have to be running as you, and would gain a read-only copy of
273273
> files they could already read. Installing from a `.pkg` would restore the
274274
> check to full strength; it is an improvement, not a prerequisite.
275+
#### macOS re-prompts for keychain access after every update
276+
277+
macOS ties a permission grant to the *identity* of the binary that was granted
278+
it. Driven's macOS build carries no Developer ID signature - the Mach-O is only
279+
ad-hoc (linker) signed, which gives it no stable
280+
[designated requirement](https://developer.apple.com/library/archive/technotes/tn2206/_index.html),
281+
so its code-directory hash changes with every build. A Developer ID signature
282+
would let the OS recognise version 2.4 and version 2.5 as the same app; without
283+
one, it cannot.
284+
285+
The practical consequence, once per update:
286+
287+
- **Keychain.** Every credential Driven holds lives in your login keychain: the
288+
account master key (service `dev.maxhogan.driven`), the Google refresh token
289+
(`driven.google.refresh_token`), your BYO OAuth client credentials
290+
(`driven.google.client_creds`), and any S3 key pair
291+
(`driven.s3.credentials`). "Always Allow" records the *specific* build you
292+
allowed, so after an update macOS asks again. Apple's own description of the
293+
dialog is "This dialog appears if you recently updated your system software or
294+
the app, or if the app has been modified". Click **Always Allow** once after
295+
each update and Driven picks up where it left off. If you deny it, Driven does
296+
not quietly fall back to storing your files unencrypted - an encrypted source
297+
whose master key it cannot read fails closed and reports `crypto.key_missing`
298+
in the activity log.
299+
- **Full Disk Access.** The same identity check applies to macOS's privacy
300+
layer, so a previously granted Full Disk Access can stop taking effect after
301+
an update even though Driven still appears (checked) in the list. If backups
302+
of protected folders start failing with permission errors right after an
303+
update, remove Driven from System Settings > Privacy & Security > Full Disk
304+
Access with the "-" button and re-add it with "+". Driven's in-app Full Disk
305+
Access banner says the same thing when it detects denied files.
306+
307+
Both go away for good with a Developer ID signature; there is no partial
308+
workaround that makes an ad-hoc-signed build's grants survive an update. This
309+
is tracked with the other signing work below.
275310

276311
#### macOS auto-updater caveat
277312

crates/driven-backend/Cargo.toml

Lines changed: 8 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -24,16 +24,13 @@ anyhow.workspace = true
2424
tracing.workspace = true
2525

2626
[dev-dependencies]
27-
# `keyring` 4.x is a thin wrapper over `keyring-core`, whose `mock` module is an
28-
# IN-MEMORY credential store installable as the process default. That makes the
29-
# keychain-backed paths (the factory's Drive arm, the BYO client-creds
30-
# resolution) coverable by real tests instead of being untestable on a headless
31-
# CI box with no OS keychain - the constraint `driven-drive`'s `token_store`
32-
# module documents and had to work around. Dev-only: production always uses the
33-
# real per-OS store.
34-
keyring-core = "1"
35-
# The facade production uses. The test helper needs it to burn keyring 4.x's
36-
# one-shot platform-store latch before the in-memory mock can stick.
37-
keyring.workspace = true
27+
# `driven_test_fixtures::keychain::isolated()` installs (and PROVES) an
28+
# in-memory credential store as the process default, so the keychain-backed
29+
# paths here - the factory's Drive arm, the BYO client-creds resolution - are
30+
# covered by real tests that never reach an OS keychain: neither the
31+
# developer's login keychain (which on macOS re-prompts on every rebuild) nor a
32+
# headless CI box's absent one. Dev-only: production always uses the real
33+
# per-OS store.
34+
driven-test-fixtures = { path = "../driven-test-fixtures" }
3835
# The local-folder tests need a throwaway destination directory.
3936
tempfile = "3"

crates/driven-backend/src/lib.rs

Lines changed: 25 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -420,63 +420,19 @@ pub fn resolve_account_oauth_creds(account_id: &str) -> (String, String) {
420420
#[cfg(test)]
421421
mod tests {
422422
use super::*;
423-
use std::sync::{Mutex, OnceLock};
424423

425-
/// Install `keyring-core`'s IN-MEMORY store as the process default, so the
426-
/// keychain-backed paths run for real without touching (or requiring) an OS
427-
/// keychain.
424+
/// Isolate this test from the OS keychain, so the keychain-backed paths run
425+
/// for real against an in-memory store without touching (or requiring) a
426+
/// real one. Returns `None` when isolation could not be established, in
427+
/// which case the caller MUST skip - see
428+
/// [`driven_test_fixtures::keychain`] for why writing for real is not an
429+
/// acceptable fallback (on macOS it blocks the run on a modal prompt).
428430
///
429-
/// Returns `None` when the mock could not be made the EFFECTIVE store, in
430-
/// which case the caller MUST skip - the alternative is writing test
431-
/// secrets into the developer's real login keychain, which on macOS also
432-
/// blocks the run on a modal permission prompt.
433-
///
434-
/// ## Ordering is load-bearing
435-
///
436-
/// `keyring` 4.x's `Entry::new` installs the PLATFORM-NATIVE store on its
437-
/// FIRST call, overwriting whatever default is already set (see
438-
/// `keyring-4.1.5/src/v1.rs`, the `SET_CREDENTIAL_STORE` latch). Installing
439-
/// the mock first is therefore silently undone by the first real `Entry`,
440-
/// and every "mock" write lands in the OS keychain instead. So this burns
441-
/// that latch first with a throwaway `Entry` (constructing one performs no
442-
/// credential I/O), THEN installs the mock, and finally PROVES the mock is
443-
/// in effect with a sentinel round trip before any test stores a secret.
444-
fn keychain() -> Option<std::sync::MutexGuard<'static, ()>> {
445-
static LOCK: OnceLock<Mutex<()>> = OnceLock::new();
446-
static ACTIVE: OnceLock<bool> = OnceLock::new();
447-
let guard = LOCK
448-
.get_or_init(|| Mutex::new(()))
449-
.lock()
450-
.unwrap_or_else(|e| e.into_inner());
451-
let active = *ACTIVE.get_or_init(|| {
452-
// 1. Burn keyring's one-shot platform-store latch. Its error on a
453-
// headless box (no secret service) is expected and ignored.
454-
let _ = keyring::Entry::new("driven.test.keyring-latch", "latch");
455-
// 2. Now the mock sticks.
456-
match keyring_core::mock::Store::new() {
457-
Ok(store) => keyring_core::set_default_store(store),
458-
Err(_) => return false,
459-
}
460-
// 3. Prove it, through the same `keyring` facade production uses.
461-
let sentinel = match keyring::Entry::new("driven.test.sentinel", "probe") {
462-
Ok(e) => e,
463-
Err(_) => return false,
464-
};
465-
if sentinel.set_password("mock-is-active").is_err() {
466-
return false;
467-
}
468-
let ok = sentinel.get_password().ok().as_deref() == Some("mock-is-active");
469-
let _ = sentinel.delete_credential();
470-
ok
471-
});
472-
if !active {
473-
eprintln!(
474-
"skipping the keychain test: the in-memory keyring store is not the effective \
475-
default, and this test will not write to a real OS keychain"
476-
);
477-
return None;
478-
}
479-
Some(guard)
431+
/// The returned guard also serializes these tests: the default credential
432+
/// store is process-global, so they key their entries by a unique account
433+
/// id and take turns.
434+
fn keychain() -> Option<driven_test_fixtures::keychain::KeychainGuard> {
435+
driven_test_fixtures::keychain::isolated()
480436
}
481437

482438
/// Env vars are process-global too; `env_oauth_creds` reads them, so the
@@ -492,6 +448,20 @@ mod tests {
492448
}
493449
}
494450

451+
#[test]
452+
fn the_test_suite_is_isolated_from_the_os_keychain() {
453+
// The guard for this whole crate: if the isolation mechanism ever stops
454+
// working (a `keyring` upgrade changing how the default store is
455+
// installed, say), fail HERE and loudly rather than letting the
456+
// keychain tests below silently skip - or, worse, start writing into a
457+
// real login keychain, which is exactly what happened when the mock was
458+
// installed BEFORE `keyring` had claimed the default store.
459+
assert!(
460+
driven_test_fixtures::keychain::is_isolated(),
461+
"the in-memory keyring store must be the effective default store"
462+
);
463+
}
464+
495465
#[test]
496466
fn descriptors_cover_every_kind_in_picker_order() {
497467
let d = descriptors();

crates/driven-crypto/Cargo.toml

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,3 +42,13 @@ md5 = { workspace = true }
4242
# directly via `RngCore`/`TryRngCore`, so the two `rand_core` major versions
4343
# never need to match.
4444
rand = "0.10"
45+
46+
[dev-dependencies]
47+
# `driven_test_fixtures::keychain::isolated()` binds keyring to an in-memory
48+
# mock store, so the [`Keystore`] round-trip tests exercise the real API
49+
# without touching a developer's login keychain (which on macOS re-prompts on
50+
# every rebuild) or needing one at all on headless CI. The dependency edge is a
51+
# cycle - driven-test-fixtures depends on driven-core, which depends on this
52+
# crate - which cargo permits precisely because it is dev-only and so cannot
53+
# appear in a shipped build.
54+
driven-test-fixtures = { path = "../driven-test-fixtures" }

0 commit comments

Comments
 (0)