feat(keyring): OS keychain for secrets with dev-mode file backend#2513
Merged
Conversation
…ychain migration
Phase 1 — Dev FileBackend (NEW):
- Add `src/openhuman/keyring/backend.rs` with `KeyringBackend` trait, `OsBackend`
(wraps `keyring` crate), `FileBackend` (plain JSON at `{workspace}/dev-keychain.json`),
and `MockBackend` (test-only in-memory map).
- Backend selected once at startup via priority: `OPENHUMAN_KEYRING_BACKEND` env var >
`cfg!(debug_assertions)` > `OPENHUMAN_APP_ENV=dev|staging` > OS keychain.
- Logged once at init: `[keyring] backend=file path=...` or `[keyring] backend=os`.
- `dev-keychain.json` is NOT encrypted (dev artifact); added to `.gitignore`.
- FileBackend: atomic write (tmp+rename), Unix mode 0600, JSON map format.
- Rewrite `mod.rs` to route `get`/`set`/`delete`/`is_available` through the
selected backend. `is_available()` always returns true for file/mock backends.
- `init_workspace(path)` registers workspace dir for FileBackend path resolution;
falls back to `OPENHUMAN_WORKSPACE` env var or home-dir defaults.
- Remove now-unused `entry.rs`.
Phase 2 — Auth profiles keychain migration (credentials/profiles.rs):
- `AuthProfilesStore` probes keychain availability at construction and caches it.
- On load: keychain entries take priority; legacy enc2:/enc: JSON fields are
decrypted and promoted to keychain on first read (idempotent); JSON secret
fields are cleared and file rewritten.
- On save: secrets stored in keychain (JSON gets no secret fields); falls back
to ChaCha20-Poly1305 encrypted JSON on keychain error.
- Profile deletion removes keychain entries (idempotent).
Phase 3 — Wallet mnemonic keychain migration (wallet/ops.rs):
- Encrypted mnemonic ciphertext moves from `wallet-state.json` to keychain at
`"wallet.mnemonic"` (user-id-scoped).
- On load: keychain first; if missing and JSON has `encrypted_mnemonic`, migrates
to keychain and rewrites JSON without that field.
- Password unlock (Argon2id+AES-GCM) continues to work post-migration.
Tests:
- 9 new FileBackend tests: round-trip, overwrite, user isolation, multi-key,
delete-nonexistent, migrate-from-file, Unix mode 0600, env-var selection,
get_or_create_random idempotency.
- OS backend tests skip when Secret Service unavailable (Linux headless/CI).
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a pluggable keyring module (OS/file/mock backends) and integrates OS keychain storage and migration for auth-profile secrets and the wallet mnemonic, with fallbacks to encrypted JSON and comprehensive unit tests. ChangesKeyring-backed secret storage for profiles and wallet
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Member
Author
|
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
…to drop libdbus-1-dev build-time dep
coderabbitai
Bot
added
feature
![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: 5
🧹 Nitpick comments (2)
src/openhuman/credentials/profiles.rs (1)
529-533: ⚡ Quick winLog the actual keychain error for diagnostics.
The error
_eis captured but not included in the log message, which loses diagnostic information when investigating keychain I/O issues.Suggested fix
- Err(_e) => { + Err(e) => { // Keychain I/O error — fall through to JSON decrypt path. log::warn!( - "[auth] keychain error for profile_id={id}; falling back to JSON" + "[auth] keychain error for profile_id={id}: {e}; falling back to JSON" );🤖 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/credentials/profiles.rs` around lines 529 - 533, Replace the ignored error variable in the keychain Err arm so the actual error is included in the log; change Err(_e) to Err(e) and add the error to the log::warn! call in the same block (e.g., include {e:?} or a formatted error) so the keychain I/O error is emitted alongside the existing message in the profile handling code that contains the Err(_) branch.src/openhuman/keyring/mod.rs (1)
35-36: ⚡ Quick winAlign test-module wiring with repository Rust test pattern.
Prefer a sibling
*_test.rsfile wired via explicit#[path = "..._test.rs"]rather than plainmod tests;.As per coding guidelines, "When extracting Rust tests out of an implementation file, prefer a sibling
*_test.rsfile wired in with#[cfg(test)] #[path = \"..._test.rs\"] mod tests;."🤖 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/mod.rs` around lines 35 - 36, Replace the bare test module declaration in mod.rs ("#[cfg(test)] mod tests;") with an explicit path-wired module using the repository test pattern: change it to #[cfg(test)] #[path = "mod_test.rs"] mod tests; and add the sibling mod_test.rs file containing the tests (ensuring the test functions remain inside the tests module). This updates the test wiring for the module declared in mod.rs to the preferred `*_test.rs` sibling convention.
🤖 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/backend.rs`:
- Around line 200-210: The FileBackend's set and delete perform unsynchronized
read-modify-write (see methods set, delete, and helpers read_map/write_map)
which makes them race-prone; protect the full read-modify-write critical section
by introducing a shared in-process Mutex (e.g., a Mutex<Field> on the
FileBackend struct) and/or an OS file lock and acquire it at the start of
set/delete (and any other mutating paths referenced around lines 126/153) before
calling read_map/modify/write_map, releasing after write completes so concurrent
callers are serialized and lost updates are prevented.
- Around line 161-165: The code currently uses
serde_json::to_vec_pretty(map).unwrap_or_default() which can silently drop data
on serialization failure; replace the unwrap_or_default with proper error
propagation/handling (e.g., use serde_json::to_vec_pretty(map)? or .map_err(|e|
/* convert to your crate error */ ) to return an Err) so that the failing call
to to_vec_pretty returns a visible error instead of producing an empty payload;
update the surrounding function (the caller that assigns to `json` in
backend.rs) to propagate or handle that Result accordingly and include context
in the error message.
In `@src/openhuman/keyring/mod.rs`:
- Around line 64-400: The mod.rs currently contains heavyweight operational
logic; extract runtime code into focused sibling modules (e.g. create ops.rs and
store.rs) and keep mod.rs export-only. Move functions and types such as
build_backend, backend, workspace_dir_for_file_backend, workspace_dir init logic
(init_workspace / WORKSPACE_DIR), MigrationOutcome, get, set, delete,
is_available, get_or_create_random, migrate_from_file, namespaced_key, and
hex_encode into appropriate new modules (e.g. store.rs for backend selection and
workspace handling; ops.rs for
get/set/delete/migrate/is_available/get_or_create_random), keeping their
signatures and visibility unchanged; ensure the static BACKEND / WORKSPACE_DIR
state is moved or referenced safely (preserve initialization semantics) and any
helper functions remain accessible. In mod.rs replace the moved definitions with
pub mod declarations and pub use re-exports (e.g. pub mod store; pub use
store::{get,set,delete,MigrationOutcome, ...}) so external callers keep the same
API and adjust any internal references/imports to use the new module paths.
- Around line 412-417: The function force_backend_for_test currently swallows
the OnceLock::set failure which can silently leave a stale BACKEND; change it to
fail loudly by checking the result of BACKEND.set(b) and panicking if it returns
Err. Specifically, update the body of force_backend_for_test to call
BACKEND.set(b) and either .expect(...) or assert on is_ok() with a clear message
(e.g., "force_backend_for_test called after BACKEND already initialized") so
tests cannot continue with a silently unchanged backend.
In `@src/openhuman/keyring/tests.rs`:
- Around line 125-152: The test file_backend_migrate_from_file_happy_path is
reimplementing migration logic instead of invoking the production function;
replace the manual steps with a direct call to super::migrate_from_file(...)
(using the same temp source file and FileBackend instance) and assert the
returned MigrationOutcome and resulting backend state (e.g.,
MigrationOutcome::Migrated and fb.get(...) == Some("migrated_value")). Do the
same change for the other migration-style tests (the ones around the second
block indicated) so they call migrate_from_file and assert the correct
MigrationOutcome variants and error/cleanup behavior (e.g., file removed or
preserved) rather than duplicating the migration implementation.
---
Nitpick comments:
In `@src/openhuman/credentials/profiles.rs`:
- Around line 529-533: Replace the ignored error variable in the keychain Err
arm so the actual error is included in the log; change Err(_e) to Err(e) and add
the error to the log::warn! call in the same block (e.g., include {e:?} or a
formatted error) so the keychain I/O error is emitted alongside the existing
message in the profile handling code that contains the Err(_) branch.
In `@src/openhuman/keyring/mod.rs`:
- Around line 35-36: Replace the bare test module declaration in mod.rs
("#[cfg(test)] mod tests;") with an explicit path-wired module using the
repository test pattern: change it to #[cfg(test)] #[path = "mod_test.rs"] mod
tests; and add the sibling mod_test.rs file containing the tests (ensuring the
test functions remain inside the tests module). This updates the test wiring for
the module declared in mod.rs to the preferred `*_test.rs` sibling convention.
🪄 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: 4edb5e72-def3-4a09-aabf-4d3268fe274c
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockapp/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
.gitignoreCargo.tomlsrc/openhuman/credentials/profiles.rssrc/openhuman/keyring/backend.rssrc/openhuman/keyring/error.rssrc/openhuman/keyring/mod.rssrc/openhuman/keyring/tests.rssrc/openhuman/mod.rssrc/openhuman/wallet/ops.rs
# Conflicts: # Cargo.toml
…s and silent errors - Fix 1: propagate serde_json serialisation failure in FileBackend::write_map instead of silently returning empty data via unwrap_or_default; add KeyringError::Backend variant to carry the message. - Fix 2: add parking_lot::Mutex to FileBackend covering the full read→modify→write cycle in set() and delete() for in-process safety. - Fix 3: split heavy mod.rs into ops.rs (get/set/delete/migrate/helpers) and store.rs (BACKEND/WORKSPACE_DIR OnceLocks, backend selection); mod.rs is now re-exports only. - Fix 4: force_backend_for_test panics loudly if BACKEND is already initialised instead of silently ignoring the set() failure. - Fix 5: rewrite migration tests to call the production migrate_from_file logic (via run_migrate helper driving FileBackend directly) and assert on MigrationOutcome variants + post-conditions (file removed, value stored).
…IN=1 Default cargo test runs were blocking on the macOS Keychain GUI permission prompt because os_keychain_available() actually probed the real backend. Now opt-in only: set OPENHUMAN_TEST_OS_KEYCHAIN=1 to exercise the OS path, otherwise tests use the FileBackend path and finish in milliseconds.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/openhuman/keyring/backend.rs (1)
166-199:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMap
FileBackendwrite-path I/O failures to non-migration errors.Line 166 and Line 180/Line 195 currently classify normal file-backend write failures as migration errors (
MigrationReadFailed/MigrationDeleteFailed), which makes runtime diagnostics misleading forset/deletepaths.💡 Proposed fix
- if let Some(parent) = self.path.parent() { - std::fs::create_dir_all(parent).map_err(|e| KeyringError::MigrationReadFailed { - path: parent.display().to_string(), - source: e, - })?; - } + if let Some(parent) = self.path.parent() { + std::fs::create_dir_all(parent).map_err(|e| { + KeyringError::Backend(format!( + "failed to create keyring parent directory {}: {e}", + parent.display() + )) + })?; + } @@ - std::fs::write(&tmp_path, &json).map_err(|e| KeyringError::MigrationDeleteFailed { - path: tmp_path.display().to_string(), - source: e, - })?; + std::fs::write(&tmp_path, &json).map_err(|e| { + KeyringError::Backend(format!( + "failed to write keyring temp file {}: {e}", + tmp_path.display() + )) + })?; @@ - std::fs::rename(&tmp_path, &self.path).map_err(|e| { - KeyringError::MigrationDeleteFailed { - path: self.path.display().to_string(), - source: e, - } - })?; + std::fs::rename(&tmp_path, &self.path).map_err(|e| { + KeyringError::Backend(format!( + "failed to atomically replace keyring file {}: {e}", + self.path.display() + )) + })?;🤖 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/backend.rs` around lines 166 - 199, The write-path I/O errors in the FileBackend (the create_dir_all(parent) error mapping, the std::fs::write(&tmp_path, &json) mapping, and the std::fs::rename(&tmp_path, &self.path) mapping) are incorrectly converted to migration-specific variants (MigrationReadFailed / MigrationDeleteFailed); update those map_err closures to return non-migration errors (e.g. KeyringError::Backend with a concise message including the operation and path and the source error) so normal set/delete failures are classified correctly; specifically change the map_err on create_dir_all(parent) and the map_err closures around std::fs::write and std::fs::rename to produce KeyringError::Backend (or the appropriate generic write/IO KeyringError variant used elsewhere) with path/tmp_path/self.path and include the original source error.
🧹 Nitpick comments (1)
Cargo.toml (1)
118-118: Validatekeyring3.x version/features and security posture.
- crates.io latest is 4.0.1;
version = "3"intentionally stays on 3.x (won’t automatically pick up 4.x).- Features
apple-native,windows-native,linux-nativeare present for keyring 3.x.- No security advisories were reported for
keyringin the queried vulnerability feed.🤖 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 `@Cargo.toml` at line 118, The Cargo.toml dependency for keyring is pinned to "3" but crates.io has 4.0.1; update the dependency by changing keyring = { version = "4.0.1", features = ["apple-native", "windows-native", "linux-native"] } and verify the feature names remain valid in keyring 4.0.1; if you intentionally must stay on 3.x instead, replace version = "3" with an explicit pinned 3.x release (e.g., "3.2.0") and add a brief comment above the dependency explaining why we are not upgrading.
🤖 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/ops.rs`:
- Around line 118-135: Add a guard at the start of get_or_create_random to
reject len_bytes == 0 before allocating or generating bytes: check the len_bytes
parameter, log a debug/error and return an appropriate KeyringError (e.g. an
InvalidInput/InvalidLength variant or create one) instead of proceeding to
allocate the zero-length vector and persisting an empty secret; place this check
before the vec![0u8; len_bytes] allocation and before OsRng.fill_bytes to
prevent creating empty secrets.
In `@src/openhuman/keyring/tests.rs`:
- Around line 140-141: The test silently swallows backend read errors by calling
fb.get(&nk).unwrap_or(None) which treats failures as None; replace those
unwrap_or(None) usages (both the occurrence checking for
MigrationOutcome::AlreadyMigrated and the later one at lines 161-162) with
proper error propagation from FileBackend::get (e.g., use the ? operator or
map_err to return the Err up the test's Result) so that backend read failures
are surfaced instead of treated as missing values.
---
Outside diff comments:
In `@src/openhuman/keyring/backend.rs`:
- Around line 166-199: The write-path I/O errors in the FileBackend (the
create_dir_all(parent) error mapping, the std::fs::write(&tmp_path, &json)
mapping, and the std::fs::rename(&tmp_path, &self.path) mapping) are incorrectly
converted to migration-specific variants (MigrationReadFailed /
MigrationDeleteFailed); update those map_err closures to return non-migration
errors (e.g. KeyringError::Backend with a concise message including the
operation and path and the source error) so normal set/delete failures are
classified correctly; specifically change the map_err on create_dir_all(parent)
and the map_err closures around std::fs::write and std::fs::rename to produce
KeyringError::Backend (or the appropriate generic write/IO KeyringError variant
used elsewhere) with path/tmp_path/self.path and include the original source
error.
---
Nitpick comments:
In `@Cargo.toml`:
- Line 118: The Cargo.toml dependency for keyring is pinned to "3" but crates.io
has 4.0.1; update the dependency by changing keyring = { version = "4.0.1",
features = ["apple-native", "windows-native", "linux-native"] } and verify the
feature names remain valid in keyring 4.0.1; if you intentionally must stay on
3.x instead, replace version = "3" with an explicit pinned 3.x release (e.g.,
"3.2.0") and add a brief comment above the dependency explaining why we are not
upgrading.
🪄 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: 40d35401-f175-42e5-a8ba-16d5b10b3c4f
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockapp/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.tomlsrc/openhuman/keyring/backend.rssrc/openhuman/keyring/error.rssrc/openhuman/keyring/mod.rssrc/openhuman/keyring/ops.rssrc/openhuman/keyring/store.rssrc/openhuman/keyring/tests.rs
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/openhuman/keyring/tests.rs (1)
5-8:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the module header to match the new opt-in OS test behavior.
The header says OS backend tests run unconditionally on macOS/Windows, but the current gate requires
OPENHUMAN_TEST_OS_KEYCHAIN=1. Please align the header text to avoid confusion.🤖 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/tests.rs` around lines 5 - 8, Update the module doc comment in the tests module to reflect the new opt-in behavior: change the statement that OS backend tests run unconditionally on macOS/Windows to note they only run when OPENHUMAN_TEST_OS_KEYCHAIN=1 is set (and on Linux they remain skipped if the Secret Service daemon is unavailable); keep the note about test keys being prefixed with `__openhuman_test__` for cleanup and ensure the module header (the top-level doc comment in src/openhuman/keyring/tests.rs) mentions the OPENHUMAN_TEST_OS_KEYCHAIN env var requirement.
🤖 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.
Outside diff comments:
In `@src/openhuman/keyring/tests.rs`:
- Around line 5-8: Update the module doc comment in the tests module to reflect
the new opt-in behavior: change the statement that OS backend tests run
unconditionally on macOS/Windows to note they only run when
OPENHUMAN_TEST_OS_KEYCHAIN=1 is set (and on Linux they remain skipped if the
Secret Service daemon is unavailable); keep the note about test keys being
prefixed with `__openhuman_test__` for cleanup and ensure the module header (the
top-level doc comment in src/openhuman/keyring/tests.rs) mentions the
OPENHUMAN_TEST_OS_KEYCHAIN env var requirement.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 837d80a3-0ab7-44cb-b025-4742373d3a42
📒 Files selected for processing (1)
src/openhuman/keyring/tests.rs
…ad errors in tests Two CodeRabbit follow-ups on PR tinyhumansai#2513: - get_or_create_random now rejects len_bytes == 0 (would otherwise persist an empty 'secret'). - run_migrate test helper uses ? instead of unwrap_or(None), so backend read failures surface as test errors instead of silently being treated as 'no entry'.
Two profile-store tests still asserted the legacy enc2: on-disk format that the keychain migration replaced: - store_roundtrip_with_encryption: dropped 'raw must contain enc2:'. Under the keychain model secret fields are omitted from the JSON entirely; remaining no-plaintext assertions still prove no leak. - load_drops_profiles_whose_decryption_fails_under_rotated_key: delete the freshly-written keychain entry before planting a corrupt enc2: blob in the JSON, so load falls back to the legacy decrypt path and exercises the drop-on-failure behavior the test is named for.
This was referenced May 23, 2026
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
1 participant
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
Problem
Sensitive material lived next to its ciphertext on disk: `~/.openhuman/.secret_key` (0600) was the ChaCha20 root that decrypted every OAuth token and API key in `auth-profiles.json`. Anyone with file read on the user's home directory could exfiltrate all provider credentials in one step. The wallet mnemonic ciphertext sat in a JSON state file. No OS-level secret management was in play despite all three desktop platforms shipping mature keychain APIs.
Solution
Adopt the `keyring` crate (`apple-native`, `windows-native`, `sync-secret-service`, `crypto-rust` features) and route all secret persistence through a new `src/openhuman/keyring/` module with a backend trait, OS / File / Mock implementations, and these functions:
```rust
pub fn get(user_id, key) -> Result<Option>;
pub fn set(user_id, key, value) -> Result<()>;
pub fn delete(user_id, key) -> Result<()>;
pub fn is_available() -> bool;
pub fn get_or_create_random(user_id, key, len_bytes) -> Result;
pub fn migrate_from_file(user_id, key, path) -> Result;
```
Backend selection precedence: `OPENHUMAN_KEYRING_BACKEND` env override > `cfg!(debug_assertions)` > `OPENHUMAN_APP_ENV=dev|staging` > OS keychain. The dev FileBackend is a plain JSON map (it's a dev artifact, not a production secret store) at `{workspace}/dev-keychain.json`, gitignored, file mode 0600 on Unix.
Callers updated:
Tradeoffs / decisions:
Submission Checklist
Impact
Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
Validation Run
Validation Blocked
Behavior Changes
Parity Contract
Duplicate / Superseded PR Handling
Summary by CodeRabbit
New Features
Public APIs
Tests
Chores