Skip to content

fix(keyring): stop concurrent writers wiping the shared secrets file #5436

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
senamakel merged 5 commits into from
Aug 8, 2026
Merged

fix(keyring): stop concurrent writers wiping the shared secrets file #5436

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f597778
fix(keyring): stop concurrent writers wiping the shared secrets file
senamakel Aug 7, 2026
66bd6b3
fix(keyring): lock legacy migration reads
senamakel Aug 7, 2026
e566e86
fix(keyring): serialize corrupt reads and retry temp files
senamakel Aug 7, 2026
51d691c
fix(keyring): compile race-safe file writes
senamakel Aug 7, 2026
b2ec573
merge: resolve keyring store.rs/README conflicts with main
senamakel Aug 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/openhuman/security/keyring/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ OS-keychain-backed secret storage with pluggable test/debug backends, plus a Cha
| `src/openhuman/security/keyring/backend.rs` | `KeyringBackend` trait + `OsBackend` (native keychain via the `keyring` crate, service name `"openhuman"`), `FileBackend` (plaintext `dev-keychain.json`, test/debug only), and test-only `MockBackend`. |
| `src/openhuman/security/keyring/encrypted_file_backend.rs` | `EncryptedFileBackend` — all secrets in one ChaCha20-Poly1305 `secrets.enc` file keyed by an app master key; `init_master_key`/`is_master_key_available`; legacy `dev-keychain.json` migration; corrupt-file quarantine. |
| `src/openhuman/security/keyring/encrypted_store.rs` | `SecretStore` — config-field encryption (`enc2:` ChaCha20-Poly1305, legacy `enc:` XOR migration), keychain-backed master key with legacy `.secret_key` file migration, process-wide key cache, Windows ACL repair (`icacls`). |
| `src/openhuman/security/keyring/file_store.rs` | Shared secrets-file primitives for both file backends: the cross-process advisory write lock (`lock_for_write`, on a sidecar `<path>.lock`), the `0600` unique-temp-then-rename `write_atomic`, and `quarantine_corrupt`. |
| `src/openhuman/security/keyring/crypto.rs` | Shared ChaCha20-Poly1305 helpers (`chacha20_encrypt`/`chacha20_decrypt`), random-byte generation, hex encode/decode. Used by both `encrypted_store` and `encrypted_file_backend`. |
| `src/openhuman/security/keyring/error.rs` | `KeyringError` (thiserror) with variants `Os`/`InvalidUtf8`/`MigrationReadFailed`/`VerifyFailed`/`MigrationDeleteFailed`/`RandomGeneration`/`Crypto`/`Backend`, plus a log-safe `diagnostic()` that preserves the `keyring::Error` variant + `OSStatus`. |
| `src/openhuman/security/keyring/tests.rs` | Module tests (backend isolation via `force_backend_for_test`). |
Expand Down Expand Up @@ -65,6 +66,8 @@ Secret storage backend, selected once and frozen in a `OnceLock`:

Workspace dir resolves from `init_workspace`, else `OPENHUMAN_WORKSPACE`, else `~/.openhuman` (or `~/.openhuman-staging` under `OPENHUMAN_APP_ENV=staging`). **In `cfg(test)` builds only**, that rule is bypassed — see the test-isolation note below.

Both file backends keep every secret in one file, so a `set` of one key rewrites all of them. That read-modify-write cycle is guarded by `file_store::lock_for_write` — an in-process mutex is not sufficient, because a desktop core, a `medulla` TUI embedding the same core, and a `cargo test` run that inherited `OPENHUMAN_WORKSPACE` all address the same path.

## Dependencies

Internal openhuman/core modules: **none** — the keyring module's own files only `use crate::openhuman::security::keyring::*` (self-internal). It is a leaf infrastructure module. External crates: `keyring`, `chacha20poly1305`, `serde_json`, `parking_lot`, `thiserror`, `anyhow`, `chrono`, `dirs`.
Expand All @@ -89,6 +92,8 @@ Discovered consumers (`crate::openhuman::security::keyring::*`):
- **`force_backend_for_test` panics if `BACKEND` is already initialized** — it must run before any keyring call in the same process (dedicated test binary or very top of a test).
- **`migrate_from_file` never deletes the source unless the verified write succeeds**, so failures are retryable.
- **`encrypted_file` corrupt/undecryptable files are quarantined** (renamed `secrets.enc.corrupt.<ts>`) and treated as empty rather than crashing.
- **A corrupt file is never *overwritten*.** `file` reads degrade to an empty map (so a missing token means "sign in again"), but a `set`/`delete` over an unparseable file quarantines it and returns an error. Returning empty on the write path is what turned a corrupt file into a wipe: the write that followed persisted a map holding nothing but the key being set.
- **Mutations hold a cross-process lock**, on `<secrets file>.lock` rather than the secrets file itself — `write_atomic` replaces the file by rename, so a lock on the old inode would guard nothing. Callers must hold it across the read *and* the write.
- **`SecretStore` master-key file is write-once**; on Windows it survives transient AV-scanner sharing violations via retry/backoff and attempts `icacls` ACL self-repair on permission errors. Decoded keys are cached so repeated decrypts (e.g. snapshot polls) hit memory.
- **Legacy formats:** `SecretStore` migrates `enc:` (XOR) → `enc2:` (ChaCha20-Poly1305) on decrypt; `EncryptedFileBackend` migrates plaintext `dev-keychain.json` → `secrets.enc` (renaming the legacy file `.json.migrated`).
- **Errors never carry secret values** — only namespaced keys; `diagnostic()` is safe to log and preserves the underlying `keyring::Error` variant/`OSStatus`.
110 changes: 48 additions & 62 deletions src/openhuman/security/keyring/backend.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,8 @@
use std::collections::HashMap;
use std::path::{Path, PathBuf};

use parking_lot::Mutex;

use crate::openhuman::security::keyring::error::KeyringError;
use crate::openhuman::security::keyring::file_store;

// ── Trait ─────────────────────────────────────────────────────────────────────

Expand Down Expand Up @@ -105,23 +104,25 @@ impl KeyringBackend for OsBackend {
/// keep unit tests and explicit recovery/debug overrides independent from the
/// host OS keychain. Never use it in a production deployment.
///
/// # Thread safety
/// # Concurrency
///
/// The `mutex` field serializes in-process read-modify-write operations on
/// `set` and `delete`. Cross-process safety relies on the atomic rename in
/// `write_map`.
/// Every secret lives in this one file, so a `set` or `delete` is a read →
/// modify → write cycle over *all* of them. That cycle is guarded by the
/// cross-process advisory lock in [`file_store::lock_for_write`] — an in-process
/// mutex is not enough, because a desktop core, a `medulla` TUI embedding the
/// same core, and a `cargo test` run that inherited `OPENHUMAN_WORKSPACE` all
/// address the same path. Unguarded, the later writer's map (read before the
/// earlier writer landed) silently discards the earlier one's secret; when that
/// secret is the app session, the symptom is being signed out for no reason.
pub struct FileBackend {
path: PathBuf,
/// In-process lock covering the read→modify→write cycle in mutating ops.
mutex: Mutex<()>,
}

impl FileBackend {
/// Create a `FileBackend` that reads/writes `{workspace_dir}/dev-keychain.json`.
pub fn new(workspace_dir: &Path) -> Self {
Self {
path: workspace_dir.join("dev-keychain.json"),
mutex: Mutex::new(()),
}
}

Expand All @@ -130,7 +131,18 @@ impl FileBackend {
&self.path
}

fn read_map(&self) -> Result<HashMap<String, String>, KeyringError> {
/// Read the whole map.
///
/// `for_write` decides what an unparseable file means, and the two answers
/// are not interchangeable:
///
/// - Reading (`false`): degrade to empty. The caller sees "no such secret",
/// which for a session token means signing in again — recoverable, and
/// better than failing every unrelated lookup.
/// - Writing (`true`): quarantine the bytes and fail. Returning empty here
/// is what turned a corrupt file into a *wipe*, because the write that
/// followed persisted a map holding nothing but the key being set.
fn read_map(&self, for_write: bool) -> Result<HashMap<String, String>, KeyringError> {
if !self.path.exists() {
return Ok(HashMap::new());
}
Expand All @@ -141,82 +153,56 @@ impl FileBackend {
if bytes.is_empty() {
return Ok(HashMap::new());
}
serde_json::from_slice::<HashMap<String, String>>(&bytes)
.map_err(|e| {
// Treat a corrupt file as empty so we degrade gracefully.
match serde_json::from_slice::<HashMap<String, String>>(&bytes) {
Ok(map) => Ok(map),
Err(e) if for_write => {
file_store::quarantine_corrupt(&self.path, "json");
Err(KeyringError::Backend(format!(
"dev-keychain.json at {} could not be parsed ({e}); it was moved aside \
rather than overwritten",
self.path.display()
)))
}
Comment on lines +156 to +165

@coderabbitai coderabbitai Bot Aug 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

The error text claims the file was moved even when the quarantine fails.

quarantine_corrupt returns None when the rename fails, and that result is discarded. The error message then states "it was moved aside rather than overwritten" while the corrupt file is still in place. A user who follows the message looks for a .corrupt. file that does not exist.

Branch on the returned Option and report the actual outcome.

🛠️ Proposed fix 
             Err(e) if for_write => {
-                file_store::quarantine_corrupt(&self.path, "json");
-                Err(KeyringError::Backend(format!(
-                    "dev-keychain.json at {} could not be parsed ({e}); it was moved aside \
-                     rather than overwritten",
-                    self.path.display()
-                )))
+                let outcome = match file_store::quarantine_corrupt(&self.path, "json") {
+                    Some(target) => format!("it was moved to {}", target.display()),
+                    None => "it could not be moved aside and is still in place".to_string(),
+                };
+                Err(KeyringError::Backend(format!(
+                    "dev-keychain.json at {} could not be parsed ({e}); {outcome} \
+                     rather than overwritten",
+                    self.path.display()
+                )))
             }

The existing test asserts on the substring "moved aside", so update file_backend_set_refuses_to_overwrite_an_unparseable_file in src/openhuman/security/keyring/tests.rs to match the new wording.

🤖 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/security/keyring/backend.rs` around lines 156 - 165, Update the
parse-error branch in the keyring backend to inspect the Option returned by
quarantine_corrupt and report whether the file was moved aside or remained in
place when quarantine fails. Preserve the existing error context, and update
file_backend_set_refuses_to_overwrite_an_unparseable_file to assert the revised
wording.

Err(e) => {
log::warn!(
"[keyring] dev-keychain.json at {} is corrupt ({e}); treating as empty",
self.path.display()
);
// Return empty map by converting to a no-source variant.
drop(e);
KeyringError::VerifyFailed {
key: "<parse>".to_string(),
}
})
.or_else(|_| Ok(HashMap::new()))
Ok(HashMap::new())
}
}
}

fn write_map(&self, map: &HashMap<String, String>) -> Result<(), KeyringError> {
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,
})?;
}

// Serialize the map to pretty JSON. Propagate serialization failure so
// callers are not silently fed empty data on a write error.
// Propagate serialization failure so callers are not silently fed empty
// data on a write error.
let json = serde_json::to_vec_pretty(map).map_err(|e| {
KeyringError::Backend(format!("failed to serialize dev keychain map: {e}"))
})?;

// Atomic write: temp file + rename.
let tmp_path = self.path.with_extension("tmp");
std::fs::write(&tmp_path, &json).map_err(|e| KeyringError::MigrationDeleteFailed {
path: tmp_path.display().to_string(),
source: e,
})?;

// Set mode 0600 on Unix before moving into place.
#[cfg(unix)]
{
use std::os::unix::fs::PermissionsExt;
let perms = std::fs::Permissions::from_mode(0o600);
if let Err(e) = std::fs::set_permissions(&tmp_path, perms) {
log::warn!("[keyring] could not set 0600 on dev-keychain.json tmp file: {e}");
}
}

std::fs::rename(&tmp_path, &self.path).map_err(|e| {
KeyringError::MigrationDeleteFailed {
path: self.path.display().to_string(),
source: e,
}
})?;

Ok(())
file_store::write_atomic(&self.path, &json)
}
}

impl KeyringBackend for FileBackend {
fn get(&self, namespaced_key: &str) -> Result<Option<String>, KeyringError> {
let map = self.read_map()?;
// No lock: `write_atomic` publishes by rename, so a reader sees either
// the whole previous file or the whole next one, never a mix.
let map = self.read_map(false)?;
Ok(map.get(namespaced_key).cloned())
}

fn set(&self, namespaced_key: &str, value: &str) -> Result<(), KeyringError> {
// Hold the in-process lock for the full read→modify→write cycle.
let _guard = self.mutex.lock();
let mut map = self.read_map()?;
// Held across the read as well as the write: taking it around the write
// alone would still let a stale map overwrite a concurrent one.
let _guard = file_store::lock_for_write(&self.path)?;
let mut map = self.read_map(true)?;
map.insert(namespaced_key.to_string(), value.to_string());
self.write_map(&map)
}

fn delete(&self, namespaced_key: &str) -> Result<(), KeyringError> {
// Hold the in-process lock for the full read→modify→write cycle.
let _guard = self.mutex.lock();
let mut map = self.read_map()?;
let _guard = file_store::lock_for_write(&self.path)?;
let mut map = self.read_map(true)?;
if map.remove(namespaced_key).is_some() {
self.write_map(&map)?;
}
Expand Down
69 changes: 22 additions & 47 deletions src/openhuman/security/keyring/encrypted_file_backend.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,10 @@ use std::collections::HashMap;
use std::path::{Path, PathBuf};
use std::sync::OnceLock;

use parking_lot::Mutex;

use crate::openhuman::security::keyring::backend::KeyringBackend;
use crate::openhuman::security::keyring::crypto::{self, KEY_LEN};
use crate::openhuman::security::keyring::error::KeyringError;
use crate::openhuman::security::keyring::file_store;
use crate::openhuman::security::keyring::store::BackendKind;

const KEYCHAIN_SERVICE: &str = "openhuman";
Expand Down Expand Up @@ -174,18 +173,24 @@ fn master_key() -> Option<&'static [u8; KEY_LEN]> {

// ── Backend ──────────────────────────────────────────────────────────────────

/// Every secret in one ChaCha20-Poly1305 file.
///
/// Mutations are a read → decrypt → modify → encrypt → write cycle over the
/// whole set, guarded by the cross-process advisory lock in
/// [`file_store::lock_for_write`]. An in-process mutex would not do: more than
/// one process routinely addresses the same workspace (a desktop core and a
/// `medulla` TUI embedding the same core), and the later writer's snapshot —
/// read before the earlier writer landed — silently drops the earlier secret.
pub struct EncryptedFileBackend {
path: PathBuf,
workspace_dir: PathBuf,
mutex: Mutex<()>,
}

impl EncryptedFileBackend {
pub fn new(workspace_dir: &Path) -> Self {
Self {
path: workspace_dir.join(SECRETS_FILENAME),
workspace_dir: workspace_dir.to_path_buf(),
mutex: Mutex::new(()),
}
}

Expand Down Expand Up @@ -230,42 +235,13 @@ impl EncryptedFileBackend {
key: &[u8; KEY_LEN],
map: &HashMap<String, String>,
) -> Result<(), KeyringError> {
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,
})?;
}

let json = serde_json::to_vec(map)
.map_err(|e| KeyringError::Backend(format!("failed to serialize secrets: {e}")))?;

let blob = crypto::chacha20_encrypt(key, &json)
.map_err(|e| KeyringError::Backend(format!("encryption failed: {e}")))?;

let tmp_path = self.path.with_extension("enc.tmp");
std::fs::write(&tmp_path, &blob).map_err(|e| KeyringError::MigrationDeleteFailed {
path: tmp_path.display().to_string(),
source: e,
})?;

#[cfg(unix)]
{
use std::os::unix::fs::PermissionsExt;
let perms = std::fs::Permissions::from_mode(0o600);
if let Err(e) = std::fs::set_permissions(&tmp_path, perms) {
log::warn!("[keyring:encrypted_file] could not set 0600 on temp file: {e}");
}
}

std::fs::rename(&tmp_path, &self.path).map_err(|e| {
KeyringError::MigrationDeleteFailed {
path: self.path.display().to_string(),
source: e,
}
})?;

Ok(())
file_store::write_atomic(&self.path, &blob)
}

fn migrate_legacy_dev_keychain(
Expand Down Expand Up @@ -320,17 +296,10 @@ impl EncryptedFileBackend {
Ok(map)
}

/// Move an undecryptable / unparseable secrets file aside so the next call
/// starts fresh without destroying the bytes.
fn handle_corruption(&self) {
let ts = chrono::Utc::now().format("%Y%m%d%H%M%S");
let corrupt_path = self.path.with_extension(format!("enc.corrupt.{ts}"));
if let Err(e) = std::fs::rename(&self.path, &corrupt_path) {
log::error!("[keyring:encrypted_file] could not rename corrupt file: {e}");
} else {
log::warn!(
"[keyring:encrypted_file] corrupt file renamed to {}",
corrupt_path.display()
);
}
file_store::quarantine_corrupt(&self.path, "enc");
}
}

Expand All @@ -339,7 +308,11 @@ impl KeyringBackend for EncryptedFileBackend {
let Some(key) = master_key() else {
return Ok(None);
};
let _guard = self.mutex.lock();
// `read_map` can mutate the filesystem: it migrates a missing file and
// quarantines corrupt ciphertext. Hold the same lock as writers for
// either case so a delayed quarantine cannot rename a replacement a
// concurrent `set` just published.
let _guard = file_store::lock_for_write(&self.path)?;

@chatgpt-codex-connector chatgpt-codex-connector Bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Serialize legacy migration with plaintext writers

When a production desktop using EncryptedFileBackend and a dev/file-backed Medulla process share a workspace during legacy migration, this locks secrets.enc.lock while FileBackend::set locks dev-keychain.json.lock, so they do not serialize. The migration can read the old plaintext map, the plaintext writer can publish a new entry, and then the migration can rename that newer file to .migrated while writing its stale snapshot to secrets.enc, making the new credential disappear from both active stores. The fresh evidence beyond the earlier same-backend migration race is the distinct lock path used by FileBackend; migration must also coordinate on the legacy file's lock or use one workspace-wide lock.

Useful? React with 👍 / 👎.

let map = self.read_map(key)?;
Ok(map.get(namespaced_key).cloned())
}
Expand All @@ -350,7 +323,9 @@ impl KeyringBackend for EncryptedFileBackend {
"master key unavailable — cannot store secrets".to_string(),
));
};
let _guard = self.mutex.lock();
// Held across the read as well as the write: taking it around the write
// alone would still let a stale map overwrite a concurrent one.
let _guard = file_store::lock_for_write(&self.path)?;
let mut map = self.read_map(key)?;
map.insert(namespaced_key.to_string(), value.to_string());
self.write_map(key, &map)
Expand All @@ -360,7 +335,7 @@ impl KeyringBackend for EncryptedFileBackend {
let Some(key) = master_key() else {
return Ok(());
};
let _guard = self.mutex.lock();
let _guard = file_store::lock_for_write(&self.path)?;
let mut map = self.read_map(key)?;
if map.remove(namespaced_key).is_some() {
self.write_map(key, &map)?;
Expand Down
Loading
Loading