test: isolate the keychain from the test suite - #214
Merged
Conversation
pmaxhogan
force-pushed
the
test/keychain-isolation
branch
from
July 29, 2026 19:45
0e30a3d to
89298a5
Compare
Contributor
Coverage
Gate: passed - no coverage regression (epsilon 0.1 pp). |
`cargo test --workspace` was writing into the developer's real macOS login keychain, which produced a fresh "allow <binary> to access <service>" prompt on every rebuild (each rebuild is a new binary identity) and could block the run on a modal dialog. Root cause: `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 the "install keyring-core's mock as the default store" helper added in #200 was silently undone by the first real `Entry`, and every "mock" write landed in the OS keychain. Confirmed empirically: the login keychain contained `driven.google.refresh_token`/`acct-with-token` and `driven.google.client_creds`/`acct-byo` - the literal hard-coded ids of the driven-backend tests. A workspace-wide probe (panic at every `Entry::new` call site, full `cargo test --workspace`) found exactly three offending tests, all in `crates/driven-backend/src/lib.rs`. No other crate's tests reach a real entry. The fix, as one shared mechanism rather than a per-crate copy: `driven_test_fixtures::keychain::isolated()` burns the latch first, installs the mock, then PROVES it is the effective store before any secret is stored - first with an I/O-free precondition (the default store must report `CredentialPersistence::ProcessOnly`, so a defeated mock is caught before anything could leak), then with a sentinel round trip through the same `keyring` facade production uses. It returns `None` (caller skips) rather than ever writing for real. This adopts and generalises the sequence from #207. Also: - Guard tests (`the_test_suite_is_isolated_from_the_os_keychain`) in every crate that owns a keychain call site, so a future `keyring` upgrade that breaks the mechanism fails loudly instead of silently writing for real. - Real round-trip coverage of `Keystore`, `KeyringTokenStore`, `ClientCredsStore` and the encrypted-source-with-no-master-key fail-closed path, all previously untestable. Corrects the module docs in driven-crypto and driven-drive that claimed the mock store was unusable. - Documents the shipped-side cost of the same macOS mechanism: because Driven carries no Developer ID signature (only an ad-hoc/linker signature, so no stable designated requirement and a per-build cdhash), keychain ACLs and TCC grants are pinned to that exact build, so users are re-prompted for keychain access on every update and a granted Full Disk Access can lapse. README macOS caveats + DESIGN 3.6. Verified: full `cargo test --workspace` from an empty login keychain completes non-interactively with zero failures, and none of the six Driven keychain services exists afterwards - despite the suite round-tripping real store/load/delete against all three production services.
… with the FDA banner
pmaxhogan
force-pushed
the
test/keychain-isolation
branch
from
July 29, 2026 23:47
89298a5 to
9379811
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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)
keyring4.x'sEntry::newinstalls the PLATFORM-NATIVE store as the processdefault on its first call, overwriting whatever default is already set
(
keyring-4.1.5/src/v1.rs, theSET_CREDENTIAL_STORElatch). So installingkeyring-core's mock before the first realEntryis silently undone, andevery "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:
1. The audit: are there any other paths?
Verified answer: no. Every test that reaches a real
keyring::Entryis nowbehind the shared helper - there are no unisolated paths left.
Done empirically rather than by inspection. A temporary probe panicked at all
four
Entryconstruction sites in the workspace -driven-crypto/src/keystore.rs:63,driven-drive/src/google/token_store.rs:102and
:157,driven-s3/src/config.rs:221- andcargo test --workspace --no-fail-fastran every test binary on this exacttree. Exactly 13 tests reach an entry, across 5 binaries:
driven-backenddriven.google.refresh_token,driven.google.client_credsdriven-s3driven.s3.credentialsdriven-cryptodev.maxhogan.drivendriven-drivedriven.google.refresh_token,driven.google.client_credssrc-tauridev.maxhogan.drivenAll 13 go through
driven_test_fixtures::keychain::isolated(). The probe hitlist 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/*/testsandsrc-tauri/tests, and the twonew crates from this week -
driven-localfsanddriven-rclonereferenceneither
keyringnor 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-coreis dropped from both crates' dev-deps:crates/driven-backend/src/lib.rs- 60-line local helper -> one-line delegatecrates/driven-s3/src/config.rs- sameIt 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 definitionmeans its credentials cannot outlive the process; a real OS keychain reports
UntilDelete/UntilReboot. So a defeated mock is caught before anything iswritten. 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
Entryis safe:buildinapple-native-keyring-store-1.0.1/src/keychain.rsis pure struct constructionwith no keychain I/O, so it raises no prompt and creates nothing.
Drift protection:
the_test_suite_is_isolated_from_the_os_keychainin everycrate that owns a keychain call site -
driven-crypto,driven-drive,driven-s3,driven-backend,src-tauri. These assert rather than skip, so afuture
keyringupgrade that breaks the mechanism fails loudly instead of thesuite 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.tomlwork.Production is untouched: no shipped code path changed, and
keyring-coreis adirect dependency only of
driven-test-fixtures, which ispublish = falseandonly 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:
(linker) signed, so it has no stable
designated requirement
and its cdhash changes with every build. (
tauri.conf.jsonsets nosigningIdentity;release.ymlruns nocodesign.)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".
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).cannot be read fails closed (
crypto.key_missing) rather than uploadingplaintext - but it stalls encrypted sources until the user re-authorizes.
entitlement or partial workaround makes an ad-hoc-signed build's grants
survive an update.
Aligned with #216, not contradicting it. #216's
fdaBanner.unsignedNotealready 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
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.
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.rsanddriven-drive/src/google/token_store.rsclaimed the mock store was unusable andsteered contributors away from testing these paths. Both are corrected, and the
paths they excluded are now covered for real against the in-memory store:
Keystorestore/load/delete master key,NotFoundon a wiped keychain,idempotent delete, per-account scoping
KeyringTokenStoreandClientCredsStoreround trips, incl. the empty-secretPKCE case and per-account isolation
KeystoreCryptoProviderwith an encrypted source that HAS a wrapped key butno 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.
workspace that constructs a keychain entry; all 13 are isolated.
store in each of those binaries - the exact thing that was silently false.
cargo test --workspace, the S3 item'smdatis still20260729181820Z- unchanged across multiple full runs and atargeted
cargo test -p driven-s3, whosecredentials_round_trip_through_the_keychainstores to that exact account.Nothing wrote.
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 owndriven.test.keyring-latch/driven.test.keyring-sentineland the test round-trip service are allabsent - despite the suite performing real store/load/delete round trips
against every production service.
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 failuresSQLX_OFFLINE=true cargo clippy --workspace --all-targets -- -D warnings- cleancargo fmt --all -- --check- cleangit diff --check- cleancargo deny check-advisories ok, bans ok, licenses ok, sources okNote:
test:is a hidden changelog type here, so the user-facing README/DESIGNmacOS caveat will not appear in the release notes. Flagging deliberately - a
follow-up
docs:commit is your call.