fix(keyring): don't mint a new master key on keychain access-denied (#3311)#3338
Merged
senamakel merged 2 commits intoJun 4, 2026
Merged
Conversation
…inyhumansai#3311) Surface a master-key load failure proactively at startup by publishing KeyringConsentRequired, reusing the existing consent-gate dedup flag so the user is warned instead of secrets silently appearing empty.
…inyhumansai#3311) try_load_master_key conflated keyring::Error::NoStorageAccess with NoEntry, so a macOS app update that changes the binary's code-signing identity / keychain ACL — making the EXISTING master key unreadable — caused the code to mint a brand-new key and overwrite the keychain entry. Every secret encrypted under the old key became undecryptable: API keys silently wiped, connectors disconnected, no warning. Split the match so only a genuine NoEntry mints; the catch-all Err(e) arm fails safe (no mint, no set_password), making the fix independent of the exact error variant macOS returns on denial. Ciphertext survives and recovers on the next launch once keychain access is restored. On failure, init_master_key now logs at error and publishes the warn signal. Extract the decision behind a MasterKeyEntry trait so the NoEntry-mints vs denied-fails-safe arms are unit-tested with an injected fake (no real OS keychain, no MASTER_KEY OnceLock involvement).
Contributor
📝 WalkthroughWalkthroughThis PR prevents data loss when the OS keychain becomes unavailable by restricting new master-key generation to genuine absence ( ChangesMaster Key Availability and Error Handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
coderabbitai
Bot
added
rust-core
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/openhuman/keyring/encrypted_file_backend.rs (1)
125-164: ⚡ Quick winAdd debug/trace logs around the keychain decision branches.
This new fix path makes the key external calls and branch decisions explicit, but it only emits an
info!on successful mint. Please adddebug!/trace!logs for the existing-key load path, theNoEntrymint path, the write/readback verification, and the fail-safe refusal branch so this flow is diagnosable without reproducing it locally.Suggested diff
fn load_or_mint_master_key<E: MasterKeyEntry>(entry: &E) -> Result<[u8; KEY_LEN], String> { + log::debug!("[keyring:encrypted_file] loading master key from OS keychain"); match entry.get_password() { Ok(hex_str) => { + log::debug!("[keyring:encrypted_file] existing master key found"); let bytes = crypto::hex_decode(hex_str.trim())?; if bytes.len() != KEY_LEN { return Err(format!( @@ } Err(keyring::Error::NoEntry) => { + log::debug!( + "[keyring:encrypted_file] master key entry missing; minting a new master key" + ); let key_bytes = crypto::generate_random_bytes(KEY_LEN); let hex_value = crypto::hex_encode(&key_bytes); entry .set_password(&hex_value) .map_err(|e| format!("failed to store new master key in keychain: {e}"))?; @@ if readback.trim() != hex_value { return Err("master key write verification failed".to_string()); } + log::debug!( + "[keyring:encrypted_file] new master key stored and readback verified" + ); @@ - Err(e) => Err(format!( - "OS keychain access unavailable; refusing to mint a replacement master key so \ - existing secrets are preserved (`#3311`): {e}" - )), + Err(e) => { + log::debug!( + "[keyring:encrypted_file] master key load failed; refusing replacement mint: {e}" + ); + Err(format!( + "OS keychain access unavailable; refusing to mint a replacement master key so \ + existing secrets are preserved (`#3311`): {e}" + )) + } } }As per coding guidelines, "Use
log/tracingatdebugortracelevel for development-oriented logs at entry/exit points, branch decisions, external calls, retries/timeouts, state transitions, and error handling paths" and "Add substantial debug-level logs while implementing features or fixes in Rust usinglog/tracingcrate with stable prefixes and correlation fields (request IDs, method names, entity IDs) for grep-friendly tracing".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/keyring/encrypted_file_backend.rs` around lines 125 - 164, Add debug/trace logs in load_or_mint_master_key to make each branch observable: log a debug/trace at the start of the function and when entry.get_password() succeeds (include trimmed hex length or KEY_LEN), log a debug/trace on the Err(keyring::Error::NoEntry) path right before generating the key and after storing it (with hex_value masked or first/last chars for safety), log a debug/trace around the write/readback verification (both success and mismatch cases), and log a debug/trace in the final Err(e) refusal branch including the error string; use log::debug!/log::trace! with a stable prefix like "[keyring:encrypted_file]" and include method name load_or_mint_master_key for grepability.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/openhuman/keyring_consent/policy.rs`:
- Around line 153-160: notify_master_key_unavailable currently always sets
CONSENT_EVENT_PUBLISHED and publishes DomainEvent::KeyringConsentRequired even
when consent is cached; change it to first consult the existing consent cache
(the same check used by check_secret_access() / CONSENT_CACHE) and return early
if consent is already present or marked declined, so you do not flip
CONSENT_EVENT_PUBLISHED or publish the event; only when the cache indicates no
consent should you proceed to swap CONSENT_EVENT_PUBLISHED and call
crate::core::event_bus::publish_global(DomainEvent::KeyringConsentRequired)
within notify_master_key_unavailable.
---
Nitpick comments:
In `@src/openhuman/keyring/encrypted_file_backend.rs`:
- Around line 125-164: Add debug/trace logs in load_or_mint_master_key to make
each branch observable: log a debug/trace at the start of the function and when
entry.get_password() succeeds (include trimmed hex length or KEY_LEN), log a
debug/trace on the Err(keyring::Error::NoEntry) path right before generating the
key and after storing it (with hex_value masked or first/last chars for safety),
log a debug/trace around the write/readback verification (both success and
mismatch cases), and log a debug/trace in the final Err(e) refusal branch
including the error string; use log::debug!/log::trace! with a stable prefix
like "[keyring:encrypted_file]" and include method name load_or_mint_master_key
for grepability.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 26ce499e-5f6f-4706-ad22-337007c8fb07
📒 Files selected for processing (2)
src/openhuman/keyring/encrypted_file_backend.rssrc/openhuman/keyring_consent/policy.rs
Comment on lines
+153
to
+160
| pub fn notify_master_key_unavailable(reason: &str) { | ||
| warn!("{LOG_PREFIX} master key unavailable: {reason}"); | ||
| if !CONSENT_EVENT_PUBLISHED.swap(true, Ordering::SeqCst) { | ||
| info!("{LOG_PREFIX} publishing KeyringConsentRequired event (master key unavailable)"); | ||
| crate::core::event_bus::publish_global( | ||
| crate::core::event_bus::DomainEvent::KeyringConsentRequired, | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
Skip KeyringConsentRequired when consent is already cached.
This helper bypasses the CONSENT_CACHE check that check_secret_access() uses, so local_encrypted / declined sessions can still publish a consent-required event and flip CONSENT_EVENT_PUBLISHED to true. That breaks the DomainEvent::KeyringConsentRequired contract and can suppress the first real publish from the lazy gate later in the session.
Suggested diff
pub fn notify_master_key_unavailable(reason: &str) {
warn!("{LOG_PREFIX} master key unavailable: {reason}");
+ let cached = CONSENT_CACHE.read().clone();
+ if matches!(
+ cached.as_ref().map(|p| p.storage_mode.as_str()),
+ Some("local_encrypted" | "declined")
+ ) {
+ return;
+ }
if !CONSENT_EVENT_PUBLISHED.swap(true, Ordering::SeqCst) {
info!("{LOG_PREFIX} publishing KeyringConsentRequired event (master key unavailable)");
crate::core::event_bus::publish_global(
crate::core::event_bus::DomainEvent::KeyringConsentRequired,
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pub fn notify_master_key_unavailable(reason: &str) { | |
| warn!("{LOG_PREFIX} master key unavailable: {reason}"); | |
| if !CONSENT_EVENT_PUBLISHED.swap(true, Ordering::SeqCst) { | |
| info!("{LOG_PREFIX} publishing KeyringConsentRequired event (master key unavailable)"); | |
| crate::core::event_bus::publish_global( | |
| crate::core::event_bus::DomainEvent::KeyringConsentRequired, | |
| ); | |
| } | |
| pub fn notify_master_key_unavailable(reason: &str) { | |
| warn!("{LOG_PREFIX} master key unavailable: {reason}"); | |
| let cached = CONSENT_CACHE.read().clone(); | |
| if matches!( | |
| cached.as_ref().map(|p| p.storage_mode.as_str()), | |
| Some("local_encrypted" | "declined") | |
| ) { | |
| return; | |
| } | |
| if !CONSENT_EVENT_PUBLISHED.swap(true, Ordering::SeqCst) { | |
| info!("{LOG_PREFIX} publishing KeyringConsentRequired event (master key unavailable)"); | |
| crate::core::event_bus::publish_global( | |
| crate::core::event_bus::DomainEvent::KeyringConsentRequired, | |
| ); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/openhuman/keyring_consent/policy.rs` around lines 153 - 160,
notify_master_key_unavailable currently always sets CONSENT_EVENT_PUBLISHED and
publishes DomainEvent::KeyringConsentRequired even when consent is cached;
change it to first consult the existing consent cache (the same check used by
check_secret_access() / CONSENT_CACHE) and return early if consent is already
present or marked declined, so you do not flip CONSENT_EVENT_PUBLISHED or
publish the event; only when the cache indicates no consent should you proceed
to swap CONSENT_EVENT_PUBLISHED and call
crate::core::event_bus::publish_global(DomainEvent::KeyringConsentRequired)
within notify_master_key_unavailable.
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
2 participants
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.
Summary
NoStorageAccess, notNoEntry). The old code conflated the two and minted a replacement — orphaning every secret encrypted under the old key (silent API-key wipe + disconnected connectors, no warning).NoEntrymay now mint; every other error fails safe (no mint, noset_passwordoverwrite), so existing ciphertext survives and recovers on the next launch once access is restored.errorand publishesKeyringConsentRequiredso the frontend can warn instead of silently resetting — the "warn before reset" the issue asks for.MasterKeyEntrytrait and covered by unit tests for each error arm.Problem
Secrets (API keys, connector tokens) are encrypted in a file whose single master key lives in the OS keychain (introduced in #2660). In
try_load_master_key, the mint-new branch matched bothkeyring::Error::NoEntryandkeyring::Error::NoStorageAccess(_):NoEntrymeans "no key was ever stored" — minting is correct.NoStorageAccessmeans "a key exists but I can't read it right now" (locked keychain, or — the #3311 trigger — an app update changed the binary's code-signing identity / the keychain item's ACL trust). Minting there overwrites the only key that can decrypt the user's secrets. Every secret is then undecryptable: API keys appear wiped, connectors disconnect, and the only log line was aninfo!. This matches the report exactly — "after update", "all at once" (single master key gates everything), "no warning".This path only runs in staging/production (
OPENHUMAN_APP_ENV), so dev never hit it — consistent with it only surfacing in shipped builds.Solution
NoEntrymints. A catch-allErr(e)arm returns an error without minting or callingset_password. This is deliberately variant-independent: whatever variant macOS actually returns on a post-update denial, anything that isn'tNoEntryfails safe.init_master_keynow logs the failure aterrorand calls a newkeyring_consent::policy::notify_master_key_unavailable, which publishes the existingDomainEvent::KeyringConsentRequired(reusing the consent-gate dedup flag so it never double-publishes).load_or_mint_master_key<E: MasterKeyEntry>behind a small trait. A realkeyring::Entrycan't be exercised undercargo test(first access blocks on a GUI prompt), so the trait lets a fake inject eachkeyring::Errorvariant. The pure decision fn touches noMASTER_KEYOnceLockstate, so the tests need no global reset seam.Submission Checklist
cargo test --lib openhuman::keyringgreen (92 tests); the newload_or_mint_master_keyarms are unit-tested. The 2-line event-publish glue ininit_master_keyis not unit-covered (it drives the global event bus); CIdiff-coveris the gate.Closes #NNNin the## RelatedsectionImpact
keyring-crate error-variant audit, and the variant-independent catch-all design — not on a live end-to-end repro.Related
NoEntrywith an existing non-emptysecrets.enc, surface a "secrets locked / master key missing" state rather than minting over real data (minting still orphans data in that narrow case).AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/3311-keyring-no-mint-on-access-denied239c4c2f865972bdf0c72be9deacc460de618d99Validation Run
pnpm --filter openhuman-app format:check— no frontend files changedpnpm typecheck— no frontend files changedcargo test --lib openhuman::keyring— 92 passed, 0 failedcargo fmt --checkclean;cargo check --libclean;cargo clippy --libno new lints in changed filesapp/src-taurifiles changedValidation Blocked
command:live end-to-end repro of post-update keychain access denialerror:cannot fake a code-signing / keychain-ACL change in a local buildimpact:branch logic verified by unit tests + crate audit instead; catch-all design is variant-independentBehavior Changes
Parity Contract
NoEntry) still mints + stores a master key exactly as before; successful load path unchanged.Duplicate / Superseded PR Handling
Summary by CodeRabbit
Release Notes
Bug Fixes
New Features