diff --git a/docs/security/keychain-secretstore-migration.md b/docs/security/keychain-secretstore-migration.md new file mode 100644 index 0000000000..77a12dab98 --- /dev/null +++ b/docs/security/keychain-secretstore-migration.md @@ -0,0 +1,40 @@ +# Secret Key Keychain Migration + +## Goal + +Move the `SecretStore` master encryption key out of `.secret_key` files and into the OS keychain for real app environments. + +Accepted exception: +- unit tests and explicit debug overrides may keep using file-backed storage. + +## Current State + +- Auth/provider credentials already prefer the keychain when available. +- `SecretStore` still keeps its master key in `{openhuman_dir}/.secret_key`. +- `keyring` currently falls back to a file backend in some non-test environments. + +## Target State + +- `dev`, `staging`, and `prod` use the OS keychain for the `SecretStore` master key. +- Existing ciphertext stays on disk unchanged. +- Existing `.secret_key` files are migrated into the keychain and then deleted only after verification. +- Unit tests continue to work without depending on the host OS keychain. + +## Migration Plan + +1. Change keyring backend selection so `cfg(test)` keeps the file backend, but normal app environments default to the OS backend unless explicitly overridden. +2. Teach `SecretStore` to: + - derive a stable keychain namespace from the user/openhuman directory + - migrate an existing `.secret_key` file into the keychain + - create the key in keychain when no legacy file exists + - fall back safely when keychain is unavailable +3. Add tests for: + - legacy `.secret_key` migration + - post-migration decrypt compatibility + - unit-test file-backed behavior + +## Constraints + +- Never re-encrypt existing payloads unless the ciphertext format itself changes. +- Never delete `.secret_key` until the keychain write is verified. +- Keep an explicit override path for debugging and recovery. diff --git a/gitbooks/SUMMARY.md b/gitbooks/SUMMARY.md index 6f95481d03..5efa8770c4 100644 --- a/gitbooks/SUMMARY.md +++ b/gitbooks/SUMMARY.md @@ -33,6 +33,7 @@ * [System & Utilities](features/native-tools/system-and-utilities.md) * [Subconscious Loop](features/subconscious.md) * [Privacy & Security](features/privacy-and-security.md) + * [OS Keyring & Secret Storage](features/os-keyring-and-secret-storage.md) * [Platform & Availability](features/platform.md) * [Cloud Deploy](features/cloud-deploy.md) diff --git a/gitbooks/features/os-keyring-and-secret-storage.md b/gitbooks/features/os-keyring-and-secret-storage.md new file mode 100644 index 0000000000..ea236cd9ad --- /dev/null +++ b/gitbooks/features/os-keyring-and-secret-storage.md @@ -0,0 +1,116 @@ +--- +icon: key +--- + +# OS Keyring & Secret Storage + +OpenHuman uses the **operating system's secure credential store** to protect the secrets that must live on your device. + +On desktop builds, that means: + +* **macOS:** Keychain +* **Windows:** Credential Manager +* **Linux:** Secret Service / libsecret + +This is the root of trust for local secret material. OpenHuman does not rely on a plaintext `.env` file or a plaintext local config file for user credentials. + +*** + +## What goes into the OS keyring + +OpenHuman uses the OS keyring for two kinds of local secret material: + +### 1. Credential entries + +When a feature needs a local credential slot, OpenHuman stores it in the platform keyring rather than writing the raw secret into a normal config file. + +Examples include: + +* locally stored provider API keys +* session and bearer tokens that must remain on-device +* wallet secret material where applicable + +These entries are scoped under OpenHuman's own key namespace so they do not collide with unrelated apps. + +### 2. The master encryption key + +Some sensitive values still need to live **inside local files** because the application configuration itself is file-based. + +OpenHuman handles that by splitting storage in two: + +* the **secret value on disk** is stored as encrypted ciphertext +* the **master key used to decrypt it** lives in the OS keyring + +This means your local config and state files can contain encrypted values without the decryption key sitting beside them in plaintext. + +*** + +## What stays encrypted on disk + +When OpenHuman needs to persist sensitive application settings locally, it writes the **ciphertext** to disk and keeps the key in the OS keyring. + +That covers local secrets such as: + +* BYO API keys for supported providers +* channel and webhook secrets stored in local config +* other locally persisted secret settings required for desktop features + +The encryption format is authenticated, so OpenHuman can detect tampering instead of silently accepting modified ciphertext. + +In practice, the security model is: + +* **key in keyring** +* **ciphertext in file** +* **plaintext only in memory when needed** + +*** + +## Why this is better than plaintext config + +If your machine has a local workspace backup, sync folder, or support bundle, plaintext secrets in config files are a liability. + +Using the OS keyring as the root secret store gives OpenHuman a safer split: + +* config files can be copied without exposing raw credentials +* accidental log or file inspection is less likely to reveal secrets +* the decryption key is delegated to the platform's credential system rather than to an app-managed plaintext file + +This is not a replacement for full-disk encryption or OS account security. It is a narrower, stronger way to handle application secrets. + +*** + +## Managed integrations vs local secrets + +Not every secret follows the same path. + +### Managed integrations + +For the default managed integration flow, third-party OAuth tokens are handled by the OpenHuman backend. Your local app does **not** need to persist those provider tokens in plaintext on your machine. + +### Local BYO credentials + +When you choose a bring-your-own-key or direct-mode path, OpenHuman treats those credentials as **local secrets** and protects them using the OS keyring plus encrypted-at-rest local storage where needed. + +*** + +## Migration from older installs + +Older versions could keep local encryption material in a file-based form. + +Current desktop builds migrate that material into the OS keyring and keep the encrypted payloads on disk. The goal is to move the root secret out of ordinary files and into the platform credential store, without requiring users to re-enter every secret by hand. + +*** + +## Platform note + +This page describes **desktop** OpenHuman: the Tauri app on macOS, Windows, and Linux. + +In development and test environments, the repository may use test-specific overrides so automated runs do not depend on an interactive OS keychain. That is a developer convenience, not the end-user desktop security model. + +*** + +## See also + +* [Privacy & Security](privacy-and-security.md) +* [Third-party Integrations](integrations/README.md) +* [Local AI (optional)](model-routing/local-ai.md) diff --git a/gitbooks/features/privacy-and-security.md b/gitbooks/features/privacy-and-security.md index 2a0c7a8372..193e2498a1 100644 --- a/gitbooks/features/privacy-and-security.md +++ b/gitbooks/features/privacy-and-security.md @@ -14,7 +14,7 @@ OpenHuman is designed so that the **memory of your life lives on your machine**. **Integration tokens are held by the backend, not on your laptop.** OAuth tokens are never written to disk in plaintext on your device. The OpenHuman backend brokers each integration request, the core never speaks any third-party API directly. -**OS-level credential storage.** Sensitive tokens are stored in your platform's secure keychain, macOS Keychain, Windows Credential Manager, Linux Secret Service. +**OS-level credential storage.** Sensitive local secrets are rooted in your platform's secure keychain, macOS Keychain, Windows Credential Manager, Linux Secret Service. See [OS Keyring & Secret Storage](os-keyring-and-secret-storage.md). **No training on your data.** Your conversations, your Memory Tree, and your personal information are never used to train AI models or improve systems. @@ -70,6 +70,8 @@ Compression and locality together become the privacy architecture. **Encrypted in transit.** All communication between the application and the OpenHuman backend uses TLS. No data travels in plain text. +**Key in keyring, ciphertext on disk.** For local secrets that must be persisted in app files, OpenHuman stores encrypted ciphertext on disk and keeps the master decryption key in the OS keyring. See [OS Keyring & Secret Storage](os-keyring-and-secret-storage.md). + **Sandboxed skills.** Each skill runs in its own isolated execution environment with enforced memory and resource limits. Skills cannot access each other's data, the host system's file system, or your credentials. **Workspace-scoped tools.** The native [filesystem tools](native-tools/coder.md) operate within the workspace the user opens; they do not have ambient access to the rest of the disk. diff --git a/scripts/test-rust-e2e.sh b/scripts/test-rust-e2e.sh index f309bb5a20..f863a16839 100755 --- a/scripts/test-rust-e2e.sh +++ b/scripts/test-rust-e2e.sh @@ -35,6 +35,8 @@ ALL_E2E_SUITES=( autocomplete_memory_e2e calendar_grounding_e2e json_rpc_e2e + keyring_secretstore_fresh_e2e + keyring_secretstore_e2e linux_cef_deb_runtime_e2e live_routing_e2e memory_graph_sync_e2e diff --git a/src/openhuman/config/schema/load.rs b/src/openhuman/config/schema/load.rs index 70ec09356a..fb6966c7e6 100644 --- a/src/openhuman/config/schema/load.rs +++ b/src/openhuman/config/schema/load.rs @@ -370,12 +370,12 @@ async fn resolve_config_dirs_ignoring_env( } fn decrypt_optional_secret( - store: &crate::openhuman::security::SecretStore, + store: &crate::openhuman::keyring::SecretStore, value: &mut Option, field_name: &str, ) -> Result<()> { if let Some(raw) = value.clone() { - if crate::openhuman::security::SecretStore::is_encrypted(&raw) { + if crate::openhuman::keyring::SecretStore::is_encrypted(&raw) { *value = Some( store .decrypt(&raw) @@ -387,12 +387,12 @@ fn decrypt_optional_secret( } fn encrypt_optional_secret( - store: &crate::openhuman::security::SecretStore, + store: &crate::openhuman::keyring::SecretStore, value: &mut Option, field_name: &str, ) -> Result<()> { if let Some(raw) = value.clone() { - if !crate::openhuman::security::SecretStore::is_encrypted(&raw) { + if !crate::openhuman::keyring::SecretStore::is_encrypted(&raw) { *value = Some( store .encrypt(&raw) @@ -412,7 +412,7 @@ fn decrypt_config_secrets(config: &mut Config, openhuman_dir: &Path) -> Result<( if !config.secrets.encrypt { return Ok(()); } - let store = crate::openhuman::security::SecretStore::new(openhuman_dir, true); + let store = crate::openhuman::keyring::SecretStore::new(openhuman_dir, true); decrypt_optional_secret(&store, &mut config.api_key, "api_key")?; @@ -521,7 +521,7 @@ fn encrypt_config_secrets(config: &mut Config) -> Result<()> { .config_path .parent() .context("Config path must have a parent directory")?; - let store = crate::openhuman::security::SecretStore::new(parent_dir, true); + let store = crate::openhuman::keyring::SecretStore::new(parent_dir, true); encrypt_optional_secret(&store, &mut config.api_key, "api_key")?; diff --git a/src/openhuman/credentials/ops.rs b/src/openhuman/credentials/ops.rs index 4958066aed..11db7e50b2 100644 --- a/src/openhuman/credentials/ops.rs +++ b/src/openhuman/credentials/ops.rs @@ -10,7 +10,7 @@ use crate::openhuman::credentials::session_support::{ build_session_state, is_local_session_token, local_session_user_id, parse_fields_value, profile_name_or_default, summarize_auth_profile, LOCAL_SESSION_USER_ID, }; -use crate::openhuman::security::SecretStore; +use crate::openhuman::keyring::SecretStore; use crate::rpc::RpcOutcome; use super::{AuthService, APP_SESSION_PROVIDER, DEFAULT_AUTH_PROFILE_NAME}; diff --git a/src/openhuman/credentials/profiles.rs b/src/openhuman/credentials/profiles.rs index 4b64f56591..0434e63b02 100644 --- a/src/openhuman/credentials/profiles.rs +++ b/src/openhuman/credentials/profiles.rs @@ -1,4 +1,4 @@ -use crate::openhuman::security::SecretStore; +use crate::openhuman::keyring::SecretStore; use anyhow::{Context, Result}; use chrono::{DateTime, Utc}; use serde::{Deserialize, Serialize}; diff --git a/src/openhuman/devices/rpc.rs b/src/openhuman/devices/rpc.rs index 4e7f368587..ffb86b8f2d 100644 --- a/src/openhuman/devices/rpc.rs +++ b/src/openhuman/devices/rpc.rs @@ -22,7 +22,7 @@ use crate::openhuman::devices::tunnel_client; use crate::openhuman::devices::types::{ CreatePairingResponse, ListDevicesResponse, PairingSession, RevokeDeviceResponse, }; -use crate::openhuman::security::SecretStore; +use crate::openhuman::keyring::SecretStore; use crate::rpc::RpcOutcome; // --------------------------------------------------------------------------- diff --git a/src/openhuman/encryption/README.md b/src/openhuman/encryption/README.md index 567b121360..3c11dbb908 100644 --- a/src/openhuman/encryption/README.md +++ b/src/openhuman/encryption/README.md @@ -28,4 +28,4 @@ AES-256-GCM at-rest crypto for AI memory storage and the encrypt/decrypt RPC sur ## Tests -- This domain has no `*_tests.rs` siblings; the underlying crypto round-trips are exercised by `src/openhuman/security/secrets_tests.rs` and the credentials tests, which both cover encrypt/decrypt happy paths and tampered-ciphertext rejection. +- This domain has no `*_tests.rs` siblings; the underlying crypto round-trips are exercised by `src/openhuman/keyring/encrypted_store_tests.rs` and the credentials tests, which both cover encrypt/decrypt happy paths and tampered-ciphertext rejection. diff --git a/src/openhuman/keyring/backend.rs b/src/openhuman/keyring/backend.rs index 3a06abcb4b..bbffbb4922 100644 --- a/src/openhuman/keyring/backend.rs +++ b/src/openhuman/keyring/backend.rs @@ -8,9 +8,7 @@ //! //! - [`FileBackend`]: Stores secrets in a plain JSON file at //! `{workspace}/dev-keychain.json`. **This file is NOT encrypted** — it is a -//! development artifact only and must never be used in production. It exists -//! solely to avoid the "different binary signature → macOS Keychain permission -//! prompt on every `cargo run`" problem that plagues dev workflows. +//! test/debug artifact only and must never be used in production. //! //! Backend selection happens once at first use (see [`super::selected_backend`]). @@ -99,15 +97,13 @@ impl KeyringBackend for OsBackend { // ── FileBackend ─────────────────────────────────────────────────────────────── -/// Dev-only backend: plain JSON file at `{workspace}/dev-keychain.json`. +/// Test/debug backend: plain JSON file at `{workspace}/dev-keychain.json`. /// /// # WARNING — NOT FOR PRODUCTION /// /// Secrets stored here are **not encrypted**. This backend exists only to -/// eliminate macOS Keychain permission prompts during development (where the -/// binary signature changes on every `cargo build`). It is selected -/// automatically in debug builds and when `OPENHUMAN_APP_ENV=dev|staging`. -/// Never use it in a production deployment. +/// keep unit tests and explicit recovery/debug overrides independent from the +/// host OS keychain. Never use it in a production deployment. /// /// # Thread safety /// diff --git a/src/openhuman/keyring/encrypted_store.rs b/src/openhuman/keyring/encrypted_store.rs new file mode 100644 index 0000000000..95f4580fa2 --- /dev/null +++ b/src/openhuman/keyring/encrypted_store.rs @@ -0,0 +1,704 @@ +// Encrypted secret store — defense-in-depth for API keys and tokens. +// +// Secrets are encrypted using ChaCha20-Poly1305 AEAD with a random key stored +// in the OS keychain for normal app environments. Legacy installs may still +// have `{data_dir}/openhuman/.secret_key`; those keys are migrated into the +// keychain on first use. Unit tests retain the file path for deterministic +// isolation. The config file stores only hex-encoded ciphertext, never +// plaintext keys. +// +// Each encryption generates a fresh random 12-byte nonce, prepended to the +// ciphertext. The Poly1305 authentication tag prevents tampering. +// +// This prevents: +// - Plaintext exposure in config files +// - Casual `grep` or `git log` leaks +// - Accidental commit of raw API keys +// - Known-plaintext attacks (unlike the previous XOR cipher) +// - Ciphertext tampering (authenticated encryption) +// +// For sovereign users who prefer plaintext, `secrets.encrypt = false` disables this. +// +// Migration: values with the legacy `enc:` prefix (XOR cipher) are decrypted +// using the old algorithm for backward compatibility. New encryptions always +// produce `enc2:` (ChaCha20-Poly1305). + +use anyhow::{Context, Result}; +use chacha20poly1305::aead::{Aead, KeyInit, OsRng}; +use chacha20poly1305::{AeadCore, ChaCha20Poly1305, Key, Nonce}; +use std::collections::HashMap; +use std::fs; +use std::path::{Path, PathBuf}; +use std::sync::{Mutex, OnceLock}; +use std::time::Duration; + +/// Length of the random encryption key in bytes (256-bit, matches `ChaCha20`). +const KEY_LEN: usize = 32; + +/// ChaCha20-Poly1305 nonce length in bytes. +const NONCE_LEN: usize = 12; + +/// Keychain slot used for the SecretStore master key. +const KEYCHAIN_MASTER_KEY: &str = "secretstore.master_key"; + +/// Manages encrypted storage of secrets (API keys, tokens, etc.) +#[derive(Debug, Clone)] +pub struct SecretStore { + /// Legacy path to the on-disk key file (`{data_dir}/openhuman/.secret_key`). + /// Still used for unit tests and one-time migration into the keychain. + key_path: PathBuf, + /// Whether encryption is enabled + enabled: bool, +} + +impl SecretStore { + /// Create a new secret store rooted at the given directory. + pub fn new(openhuman_dir: &Path, enabled: bool) -> Self { + Self { + key_path: openhuman_dir.join(".secret_key"), + enabled, + } + } + + /// Encrypt a plaintext secret. Returns hex-encoded ciphertext prefixed with `enc2:`. + /// Format: `enc2:` (12 + N + 16 bytes). + /// If encryption is disabled, returns the plaintext as-is. + pub fn encrypt(&self, plaintext: &str) -> Result { + if !self.enabled || plaintext.is_empty() { + return Ok(plaintext.to_string()); + } + + let key_bytes = self.load_or_create_key()?; + let key = Key::from_slice(&key_bytes); + let cipher = ChaCha20Poly1305::new(key); + + let nonce = ChaCha20Poly1305::generate_nonce(&mut OsRng); + let ciphertext = cipher + .encrypt(&nonce, plaintext.as_bytes()) + .map_err(|e| anyhow::anyhow!("Encryption failed: {e}"))?; + + // Prepend nonce to ciphertext for storage + let mut blob = Vec::with_capacity(NONCE_LEN + ciphertext.len()); + blob.extend_from_slice(&nonce); + blob.extend_from_slice(&ciphertext); + + Ok(format!("enc2:{}", hex_encode(&blob))) + } + + /// Decrypt a secret. + /// - `enc2:` prefix → ChaCha20-Poly1305 (current format) + /// - `enc:` prefix → legacy XOR cipher (backward compatibility for migration) + /// - No prefix → returned as-is (plaintext config) + /// + /// **Warning**: Legacy `enc:` values are insecure. Use `decrypt_and_migrate` to + /// automatically upgrade them to the secure `enc2:` format. + pub fn decrypt(&self, value: &str) -> Result { + if let Some(hex_str) = value.strip_prefix("enc2:") { + self.decrypt_chacha20(hex_str) + } else if let Some(hex_str) = value.strip_prefix("enc:") { + self.decrypt_legacy_xor(hex_str) + } else { + Ok(value.to_string()) + } + } + + /// Decrypt a secret and return a migrated `enc2:` value if the input used legacy `enc:` format. + /// + /// Returns `(plaintext, Some(new_enc2_value))` if migration occurred, or + /// `(plaintext, None)` if no migration was needed. + /// + /// This allows callers to persist the upgraded value back to config. + pub fn decrypt_and_migrate(&self, value: &str) -> Result<(String, Option)> { + if let Some(hex_str) = value.strip_prefix("enc2:") { + // Already using secure format — no migration needed + let plaintext = self.decrypt_chacha20(hex_str)?; + Ok((plaintext, None)) + } else if let Some(hex_str) = value.strip_prefix("enc:") { + // Legacy XOR cipher — decrypt and re-encrypt with ChaCha20-Poly1305 + log::warn!( + "Decrypting legacy XOR-encrypted secret (enc: prefix). \ + This format is insecure and will be removed in a future release. \ + The secret will be automatically migrated to enc2: (ChaCha20-Poly1305)." + ); + let plaintext = self.decrypt_legacy_xor(hex_str)?; + let migrated = self.encrypt(&plaintext)?; + Ok((plaintext, Some(migrated))) + } else { + // Plaintext — no migration needed + Ok((value.to_string(), None)) + } + } + + /// Check if a value uses the legacy `enc:` format that should be migrated. + pub fn needs_migration(value: &str) -> bool { + value.starts_with("enc:") + } + + /// Decrypt using ChaCha20-Poly1305 (current secure format). + fn decrypt_chacha20(&self, hex_str: &str) -> Result { + let blob = + hex_decode(hex_str).context("Failed to decode encrypted secret (corrupt hex)")?; + anyhow::ensure!( + blob.len() > NONCE_LEN, + "Encrypted value too short (missing nonce)" + ); + + let (nonce_bytes, ciphertext) = blob.split_at(NONCE_LEN); + let nonce = Nonce::from_slice(nonce_bytes); + let key_bytes = self.load_or_create_key()?; + let key = Key::from_slice(&key_bytes); + let cipher = ChaCha20Poly1305::new(key); + + let plaintext_bytes = cipher + .decrypt(nonce, ciphertext) + .map_err(|_| anyhow::anyhow!("Decryption failed — wrong key or tampered data"))?; + + String::from_utf8(plaintext_bytes) + .context("Decrypted secret is not valid UTF-8 — corrupt data") + } + + /// Decrypt using legacy XOR cipher (insecure, for backward compatibility only). + fn decrypt_legacy_xor(&self, hex_str: &str) -> Result { + let ciphertext = hex_decode(hex_str) + .context("Failed to decode legacy encrypted secret (corrupt hex)")?; + let key = self.load_or_create_key()?; + let plaintext_bytes = xor_cipher(&ciphertext, &key); + String::from_utf8(plaintext_bytes) + .context("Decrypted legacy secret is not valid UTF-8 — wrong key or corrupt data") + } + + /// Check if a value is already encrypted (current or legacy format). + pub fn is_encrypted(value: &str) -> bool { + value.starts_with("enc2:") || value.starts_with("enc:") + } + + /// Check if a value uses the secure `enc2:` format. + pub fn is_secure_encrypted(value: &str) -> bool { + value.starts_with("enc2:") + } + + fn use_legacy_file_key(&self) -> bool { + cfg!(test) + } + + fn keyring_user_id(&self) -> String { + keyring_user_id_from_dir(self.key_path.parent().unwrap_or_else(|| Path::new("."))) + } + + fn load_or_create_key_from_keyring(&self) -> Result> { + let user_id = self.keyring_user_id(); + match crate::openhuman::keyring::migrate_from_file( + &user_id, + KEYCHAIN_MASTER_KEY, + &self.key_path, + ) { + Ok(crate::openhuman::keyring::MigrationOutcome::MigratedAndDeleted) => { + log::info!( + "[security] migrated legacy secret-store key into keychain from {}", + self.key_path.display() + ); + } + Ok(crate::openhuman::keyring::MigrationOutcome::AlreadyMigrated) + | Ok(crate::openhuman::keyring::MigrationOutcome::NoSourceFile) => {} + Err(error) => { + log::warn!( + "[security] failed to migrate legacy secret-store key from {}: {error}", + self.key_path.display() + ); + } + } + + let hex_key = + crate::openhuman::keyring::get_or_create_random(&user_id, KEYCHAIN_MASTER_KEY, KEY_LEN) + .map_err(|e| { + anyhow::anyhow!("Failed to load secret-store master key from keychain: {e}") + })?; + decode_key_hex(hex_key.trim()) + } + + /// Load the encryption key from keychain-backed storage, falling back to + /// the legacy file path only in unit tests. + /// + /// The decoded key is cached process-wide keyed by `key_path`, so repeated + /// callers (e.g. every `app_state_snapshot` poll) hit memory instead of + /// disk/keychain lookup. + fn load_or_create_key(&self) -> Result> { + // Normalize the path once so all callers share the same cache slot + // regardless of how `key_path` was spelled (relative vs absolute, + // symlinks, case-variants on Windows). + let cache_key_path = normalize_cache_path(&self.key_path); + + if let Some(cached) = cached_key(&cache_key_path) { + return Ok(cached); + } + + if !self.use_legacy_file_key() { + let key = self.load_or_create_key_from_keyring()?; + cache_key(&cache_key_path, &key); + return Ok(key); + } + + if self.key_path.exists() { + let read_result = read_key_file_with_retry(&self.key_path); + + // On Windows a previous bad icacls invocation may have stripped the + // inherited ACEs from %APPDATA% without granting the current user + // explicit access, leaving the file permanently unreadable. Attempt + // a one-shot ACL repair via `icacls /reset` before giving up. + #[cfg(windows)] + let read_result = if let Err(ref e) = read_result { + if is_permission_error(e) { + log::warn!( + "[security] PermissionDenied reading key file '{}'; \ + attempting icacls /reset self-repair", + self.key_path.display() + ); + repair_windows_acl(&self.key_path); + // Single retry regardless of whether repair reported success — + // icacls /reset may partially restore access even on a non-zero exit. + read_key_file_with_retry(&self.key_path) + } else { + read_result + } + } else { + read_result + }; + + let hex_key = read_result.with_context(|| { + let mut msg = format!( + "Failed to read secret key file at {}", + self.key_path.display() + ); + #[cfg(windows)] + { + msg.push_str( + "\n\nThis is often caused by incorrect file permissions on Windows. \ + Try repairing ACLs on the .openhuman directory:\n\ + icacls \"%USERPROFILE%\\.openhuman\" /reset /t /c\n\ + icacls \"%USERPROFILE%\\.openhuman\\.secret_key\" /reset /c", + ); + } + msg + })?; + let key = decode_key_hex(hex_key.trim())?; + cache_key(&cache_key_path, &key); + Ok(key) + } else { + let key = generate_random_key(); + let hex_key = hex_encode(&key); + if let Some(parent) = self.key_path.parent() { + fs::create_dir_all(parent)?; + } + + // Write key file with restrictive permissions atomically on Unix + // to avoid a TOCTOU race where the file is briefly world-readable, + // and to avoid clobbering a key another process created first. + // See: src/core/auth.rs:write_token_file for the reference pattern. + #[cfg(unix)] + { + use std::io::Write as _; + use std::os::unix::fs::OpenOptionsExt as _; + let open_result = fs::OpenOptions::new() + .write(true) + .create_new(true) + .mode(0o600) + .open(&self.key_path); + + match open_result { + Ok(mut file) => { + file.write_all(hex_key.as_bytes()) + .context("Failed to write secret key file")?; + } + Err(error) if error.kind() == std::io::ErrorKind::AlreadyExists => { + let existing_key = read_key_file_with_retry(&self.key_path) + .with_context(|| { + format!( + "Secret key file was created concurrently but could not be read at {}", + self.key_path.display() + ) + }) + .and_then(|existing_hex| decode_key_hex(existing_hex.trim()))?; + cache_key(&cache_key_path, &existing_key); + return Ok(existing_key); + } + Err(error) => { + return Err(error).context("Failed to create secret key file"); + } + } + } + #[cfg(not(unix))] + { + use std::io::Write as _; + + let open_result = fs::OpenOptions::new() + .write(true) + .create_new(true) + .open(&self.key_path); + + match open_result { + Ok(mut file) => { + file.write_all(hex_key.as_bytes()) + .context("Failed to write secret key file")?; + } + Err(error) if error.kind() == std::io::ErrorKind::AlreadyExists => { + let existing_key = read_key_file_with_retry(&self.key_path) + .with_context(|| { + format!( + "Secret key file was created concurrently but could not be read at {}", + self.key_path.display() + ) + }) + .and_then(|existing_hex| decode_key_hex(existing_hex.trim()))?; + cache_key(&cache_key_path, &existing_key); + return Ok(existing_key); + } + Err(error) => { + return Err(error).context("Failed to create secret key file"); + } + } + } + #[cfg(windows)] + { + // On Windows, use icacls to restrict permissions to current user only. + // We use USERDOMAIN\USERNAME so the account is resolved correctly on + // domain-joined and AAD-joined machines (bare USERNAME is ambiguous and + // may refer to a local account that doesn't match the signed-in user). + let username = std::env::var("USERNAME").unwrap_or_default(); + let userdomain = std::env::var("USERDOMAIN").unwrap_or_default(); + let computername = std::env::var("COMPUTERNAME").unwrap_or_default(); + let qualified_username = + qualify_windows_username(&username, &userdomain, &computername); + let Some(grant_arg) = build_windows_icacls_grant_arg(&qualified_username) else { + log::warn!( + "[security] USERNAME/USERDOMAIN environment variables are empty; \ + cannot restrict key file permissions via icacls" + ); + cache_key(&cache_key_path, &key); + return Ok(key); + }; + + let icacls_ok = match std::process::Command::new("icacls") + .arg(&self.key_path) + .args(["/inheritance:r", "/grant:r"]) + .arg(&grant_arg) + .output() + { + Ok(o) if o.status.success() => { + log::debug!("[security] key file permissions restricted via icacls"); + true + } + Ok(o) => { + log::warn!( + "[security] icacls exited {:?} for account '{}'; \ + restoring inherited ACLs so the file remains readable", + o.status.code(), + grant_arg, + ); + false + } + Err(e) => { + log::warn!( + "[security] could not run icacls: {e}; \ + restoring inherited ACLs" + ); + false + } + }; + // If the icacls grant command failed, the `/inheritance:r` flag may have + // already stripped the inherited ACEs that let the current user read the + // file. Explicitly reset to restore inheritance so the file is always + // readable — a slightly weaker ACL is preferable to a locked-out user. + if !icacls_ok { + let _ = std::process::Command::new("icacls") + .arg(&self.key_path) + .args(["/reset"]) + .output(); + } + } + + cache_key(&cache_key_path, &key); + Ok(key) + } + } +} + +fn keyring_user_id_from_dir(openhuman_dir: &Path) -> String { + if let Some(id) = openhuman_dir.file_name().and_then(|s| s.to_str()) { + if !id.is_empty() { + return id.to_string(); + } + } + + let path_str = openhuman_dir.to_string_lossy(); + let mut hash: u64 = 14695981039346656037u64; + for b in path_str.as_bytes() { + hash ^= u64::from(*b); + hash = hash.wrapping_mul(1099511628211u64); + } + format!("secretstore-path-{hash:016x}") +} + +/// Normalize a path into a stable cache key. Tries `canonicalize` first (so +/// symlinks, relative paths, and Windows case-variants all collapse to the +/// same key), falls back to `std::path::absolute` when the file does not yet +/// exist (e.g. the create branch in `load_or_create_key`), and finally to the +/// raw path so a normalization failure never breaks the cache. +fn normalize_cache_path(path: &Path) -> PathBuf { + fs::canonicalize(path) + .or_else(|_| std::path::absolute(path)) + .unwrap_or_else(|_| path.to_path_buf()) +} + +/// Process-wide cache of decoded key bytes keyed by absolute path. +/// +/// Loading the key once per process is both faster and more reliable than +/// re-reading `.secret_key` on every decrypt. On Windows the file can be +/// transiently inaccessible (AV scanners holding a handle), and re-reading +/// turned that transient failure into a perma-failure for every subsequent +/// RPC call. +fn key_cache() -> &'static Mutex>> { + static CACHE: OnceLock>>> = OnceLock::new(); + CACHE.get_or_init(|| Mutex::new(HashMap::new())) +} + +fn cached_key(path: &Path) -> Option> { + key_cache().lock().ok()?.get(path).cloned() +} + +fn cache_key(path: &Path, key: &[u8]) { + if let Ok(mut cache) = key_cache().lock() { + cache.insert(path.to_path_buf(), key.to_vec()); + } +} + +/// Clear the cached key for `path`. Test-only — production code should never +/// need to invalidate the cache, since the key file is write-once. +#[cfg(test)] +pub(super) fn clear_cached_key(path: &Path) { + let normalized = normalize_cache_path(path); + if let Ok(mut cache) = key_cache().lock() { + cache.remove(&normalized); + } +} + +/// Read the key file, retrying transient errors a handful of times. +/// +/// Windows AV scanners (Defender, etc.) routinely hold short-lived read +/// handles right after a file is created, which surfaces as +/// `ERROR_SHARING_VIOLATION` (raw OS error 32) or `PermissionDenied`. A few +/// short backoffs are enough to ride over the lock; the typical successful +/// path returns on the first attempt with zero added latency. +fn read_key_file_with_retry(path: &Path) -> std::io::Result { + use std::io::ErrorKind; + + const MAX_ATTEMPTS: u32 = 5; + let mut last_err: Option = None; + for attempt in 0..MAX_ATTEMPTS { + match fs::read_to_string(path) { + Ok(contents) => return Ok(contents), + Err(err) => { + let transient = matches!( + err.kind(), + ErrorKind::PermissionDenied | ErrorKind::Interrupted | ErrorKind::WouldBlock + ) || err.raw_os_error() == Some(32); // ERROR_SHARING_VIOLATION (Windows) + last_err = Some(err); + if !transient || attempt + 1 == MAX_ATTEMPTS { + break; + } + let backoff_ms = 10u64 << attempt; // 10, 20, 40, 80 ms + std::thread::sleep(Duration::from_millis(backoff_ms)); + } + } + } + Err(last_err.unwrap_or_else(|| std::io::Error::other("read_to_string failed"))) +} + +/// Returns `true` when an `std::io::Error` is a permanent permission/access +/// denial rather than a transient sharing violation. +#[cfg(windows)] +fn is_permission_error(e: &std::io::Error) -> bool { + matches!(e.kind(), std::io::ErrorKind::PermissionDenied) || e.raw_os_error() == Some(5) + // ERROR_ACCESS_DENIED +} + +/// Attempt to repair a locked key file by running `icacls /reset` on it. +/// +/// Attempt to repair a locked key file. +/// +/// Two-step process: +/// 1. `icacls /reset` — removes all explicit ACEs and re-enables ACL +/// inheritance from the parent directory. +/// 2. `icacls /grant:r :F` — explicit grant for the current +/// user as a belt-and-suspenders fallback for environments (e.g. CI +/// temp dirs) where the parent's inheritance chain may not include the +/// runner account. +/// +/// Returns `true` if the file is actually readable after the repair attempt, +/// regardless of which step(s) succeeded. +#[cfg(windows)] +pub(super) fn repair_windows_acl(path: &Path) -> bool { + // Step 1: restore inheritance. + match std::process::Command::new("icacls") + .arg(path) + .args(["/reset"]) + .output() + { + Ok(o) if o.status.success() => { + log::info!( + "[security] icacls /reset succeeded for '{}'; ACL inheritance restored", + path.display() + ); + } + Ok(o) => { + log::warn!( + "[security] icacls /reset exited {:?} for '{}'", + o.status.code(), + path.display() + ); + } + Err(e) => { + log::warn!( + "[security] could not run icacls /reset for '{}': {e}", + path.display() + ); + } + } + + // Step 2: explicit grant for current user — handles CI environments + // where the temp/app dir's inheritable ACEs don't include the runner. + let username = std::env::var("USERNAME").unwrap_or_default(); + let userdomain = std::env::var("USERDOMAIN").unwrap_or_default(); + let computername = std::env::var("COMPUTERNAME").unwrap_or_default(); + let qualified = qualify_windows_username(&username, &userdomain, &computername); + if let Some(grant_arg) = build_windows_icacls_grant_arg(&qualified) { + match std::process::Command::new("icacls") + .arg(path) + .args(["/grant:r"]) + .arg(&grant_arg) + .output() + { + Ok(o) if o.status.success() => { + log::debug!( + "[security] explicit grant '{grant_arg}' succeeded during repair of '{}'", + path.display() + ); + } + Ok(o) => { + log::warn!( + "[security] explicit grant '{grant_arg}' exited {:?} during repair of '{}'", + o.status.code(), + path.display() + ); + } + Err(e) => { + log::warn!( + "[security] could not run icacls /grant during repair of '{}': {e}", + path.display() + ); + } + } + } + + // Return whether the file is actually readable now — callers use this + // for logging/metrics; the retry in load_or_create_key is unconditional. + std::fs::read(path).is_ok() +} + +/// XOR cipher with repeating key. Same function for encrypt and decrypt. +fn xor_cipher(data: &[u8], key: &[u8]) -> Vec { + if key.is_empty() { + return data.to_vec(); + } + data.iter() + .enumerate() + .map(|(i, &b)| b ^ key[i % key.len()]) + .collect() +} + +/// Generate a random 256-bit key using the OS CSPRNG. +/// +/// Uses `OsRng` (via `getrandom`) directly, providing full 256-bit entropy +/// without the fixed version/variant bits that UUID v4 introduces. +fn generate_random_key() -> Vec { + ChaCha20Poly1305::generate_key(&mut OsRng).to_vec() +} + +fn decode_key_hex(hex_key: &str) -> Result> { + let key = hex_decode(hex_key).context("Secret key file is corrupt")?; + anyhow::ensure!( + key.len() == KEY_LEN, + "Secret key file has wrong length: expected {KEY_LEN} bytes, got {}", + key.len() + ); + Ok(key) +} + +/// Hex-encode bytes to a lowercase hex string. +fn hex_encode(data: &[u8]) -> String { + let mut s = String::with_capacity(data.len() * 2); + for b in data { + use std::fmt::Write; + let _ = write!(s, "{b:02x}"); + } + s +} + +/// Build the `/grant` argument for `icacls` using a normalized username. +/// Returns `None` when the username is empty or whitespace-only. +fn build_windows_icacls_grant_arg(username: &str) -> Option { + let normalized = username.trim(); + if normalized.is_empty() { + return None; + } + Some(format!("{normalized}:F")) +} + +/// Produce a domain-qualified Windows account name suitable for `icacls`. +/// +/// On domain-joined machines `USERDOMAIN` is the domain name and differs from +/// `COMPUTERNAME`. On standalone machines they are equal, so we use the bare +/// `username` in that case to avoid a redundant `DESKTOP-XYZ\alice` prefix. +/// +/// Returns an empty string when both inputs are empty (caller must treat this +/// as "cannot determine account name"). +#[cfg(windows)] +fn qualify_windows_username(username: &str, userdomain: &str, computername: &str) -> String { + let username = username.trim(); + let userdomain = userdomain.trim(); + let computername = computername.trim(); + + if username.is_empty() { + return String::new(); + } + + // If USERDOMAIN is set and differs from COMPUTERNAME the machine is + // domain/AAD-joined; use the fully-qualified form so icacls resolves the + // account unambiguously. + if !userdomain.is_empty() + && !computername.is_empty() + && !userdomain.eq_ignore_ascii_case(computername) + { + format!("{userdomain}\\{username}") + } else { + username.to_string() + } +} + +/// Hex-decode a hex string to bytes. +#[allow(clippy::manual_is_multiple_of)] +fn hex_decode(hex: &str) -> Result> { + if (hex.len() & 1) != 0 { + anyhow::bail!("Hex string has odd length"); + } + (0..hex.len()) + .step_by(2) + .map(|i| { + u8::from_str_radix(&hex[i..i + 2], 16) + .map_err(|e| anyhow::anyhow!("Invalid hex at position {i}: {e}")) + }) + .collect() +} + +#[cfg(test)] +#[path = "encrypted_store_tests.rs"] +mod tests; diff --git a/src/openhuman/security/secrets_tests.rs b/src/openhuman/keyring/encrypted_store_tests.rs similarity index 98% rename from src/openhuman/security/secrets_tests.rs rename to src/openhuman/keyring/encrypted_store_tests.rs index 28557324d9..342642c2b2 100644 --- a/src/openhuman/security/secrets_tests.rs +++ b/src/openhuman/keyring/encrypted_store_tests.rs @@ -45,6 +45,21 @@ fn disabled_store_returns_plaintext() { assert_eq!(result, "sk-secret", "Disabled store should not encrypt"); } +#[test] +fn keyring_user_id_uses_last_path_component_when_available() { + let path = std::path::Path::new("/tmp/openhuman/users/user-123"); + assert_eq!(keyring_user_id_from_dir(path), "user-123"); +} + +#[test] +fn keyring_user_id_falls_back_to_stable_hash() { + let path = std::path::Path::new("/"); + let first = keyring_user_id_from_dir(path); + let second = keyring_user_id_from_dir(path); + assert_eq!(first, second); + assert!(first.starts_with("secretstore-path-")); +} + #[test] fn is_encrypted_detects_prefix() { assert!(SecretStore::is_encrypted("enc2:aabbcc")); diff --git a/src/openhuman/keyring/mod.rs b/src/openhuman/keyring/mod.rs index a6ef70ccfa..98821b1e41 100644 --- a/src/openhuman/keyring/mod.rs +++ b/src/openhuman/keyring/mod.rs @@ -1,11 +1,11 @@ -//! OS-keychain backed secret storage with a dev-mode file backend. +//! OS-keychain backed secret storage with a test/debug file backend override. //! -//! Wraps the [`keyring`] crate (or a file-based backend in dev) to provide a +//! Wraps the [`keyring`] crate (or an explicit file backend override) to provide a //! namespaced, user-scoped interface to secret storage: //! - **macOS**: Keychain (prod) //! - **Windows**: Credential Manager (prod) //! - **Linux**: Secret Service / libsecret (prod) -//! - **Any platform — dev**: JSON file at `{workspace}/dev-keychain.json` +//! - **Tests / explicit override**: JSON file at `{workspace}/dev-keychain.json` //! //! All keys are scoped under a `user_id` parameter so multiple users can //! coexist without collision. The backend entry key format is: @@ -16,9 +16,8 @@ //! The backend is chosen **once** at first use, in this priority order: //! //! 1. `OPENHUMAN_KEYRING_BACKEND` env var: `"os"` | `"file"` | `"mock"`. -//! 2. `cfg!(debug_assertions)` → `file`. -//! 3. `OPENHUMAN_APP_ENV` == `"dev"` or `"staging"` → `file`. -//! 4. Otherwise → `os` (production). +//! 2. `cfg!(test)` → `file`. +//! 3. Otherwise → `os`. //! //! The selected backend is logged once with `[keyring] backend= ...`. //! @@ -30,16 +29,15 @@ //! always reports as available. pub mod backend; +pub mod encrypted_store; pub mod error; pub mod ops; pub mod store; -#[cfg(test)] -mod tests; - // ── Public re-exports ───────────────────────────────────────────────────────── pub use backend::KeyringBackend; +pub use encrypted_store::SecretStore; pub use error::KeyringError; pub use ops::{ delete, get, get_or_create_random, is_available, migrate_from_file, set, MigrationOutcome, diff --git a/src/openhuman/keyring/store.rs b/src/openhuman/keyring/store.rs index 9ad1211400..9f4b17d676 100644 --- a/src/openhuman/keyring/store.rs +++ b/src/openhuman/keyring/store.rs @@ -63,33 +63,14 @@ pub(super) fn build_backend() -> Box { } } - // Priority 2: debug build → file backend. - if cfg!(debug_assertions) { + // Priority 2: unit tests → file backend for deterministic isolation. + if cfg!(test) { let path = workspace_dir_for_file_backend(); - log::info!( - "[keyring] backend=file path={} (debug_assertions build)", - path.display() - ); + log::info!("[keyring] backend=file path={} (cfg(test))", path.display()); return Box::new(backend::FileBackend::new(&path)); } - // Priority 3: OPENHUMAN_APP_ENV dev/staging → file backend. - if let Ok(app_env) = std::env::var("OPENHUMAN_APP_ENV") { - match app_env.trim() { - "dev" | "staging" => { - let path = workspace_dir_for_file_backend(); - log::info!( - "[keyring] backend=file path={} (OPENHUMAN_APP_ENV={})", - path.display(), - app_env.trim() - ); - return Box::new(backend::FileBackend::new(&path)); - } - _ => {} - } - } - - // Priority 4: production OS backend. + // Priority 3: real app environments → OS backend. log::info!("[keyring] backend=os"); Box::new(backend::OsBackend) } diff --git a/src/openhuman/security/README.md b/src/openhuman/security/README.md index 4b12575478..8832da9ef7 100644 --- a/src/openhuman/security/README.md +++ b/src/openhuman/security/README.md @@ -10,7 +10,7 @@ Trust boundary for the autonomous core. Owns the autonomy / risk policy, sandbox - `pub trait Sandbox` / `pub struct NoopSandbox` — `traits.rs` — pluggable sandbox abstraction. - `pub fn create_sandbox(config: &SecurityConfig) -> Arc` — `detect.rs:1` — pick the best backend for the host. - Sandbox backends: `pub mod docker`, `pub mod bubblewrap`, `pub mod firejail`, `pub mod landlock` — domain-specific implementations of `Sandbox`. -- `pub struct SecretStore` — `secrets.rs` — XOR / OS-keychain encrypted secret persistence with round-trip helpers. +- `pub struct SecretStore` — `keyring/encrypted_store.rs` (re-exported here) — encrypted-on-disk secret codec whose master key lives in keychain-backed storage. - `pub struct AuditLogger` / `pub enum AuditEventType` / `pub struct AuditEvent` / `pub struct Actor` / `pub struct Action` / `pub struct ExecutionResult` / `pub struct SecurityContext` / `pub struct CommandExecutionLog` — `audit.rs` — append-only audit trail. - `pub struct PairingGuard` / `pub fn constant_time_eq` / `pub fn is_public_bind` — `pairing.rs` — pairing-token check before binding the RPC server publicly. - `pub fn redact(value: &str) -> String` — `core.rs:3` — uniform 4-char-prefix redaction for logs. @@ -20,7 +20,7 @@ Trust boundary for the autonomous core. Owns the autonomy / risk policy, sandbox - `src/openhuman/config/` — `SecurityConfig`, `AutonomyConfig` for policy + sandbox selection. - OS-level sandbox tools — `docker`, `bwrap`, `firejail`, `landlock` syscalls (per backend). -- Filesystem under the workspace dir for the audit log + secrets store. +- Filesystem under the workspace dir for the audit log + encrypted ciphertext payloads. ## Called by @@ -33,6 +33,6 @@ Trust boundary for the autonomous core. Owns the autonomy / risk policy, sandbox ## Tests -- Unit: `pairing_tests.rs`, `policy_tests.rs`, `secrets_tests.rs`. +- Unit: `pairing_tests.rs`, `policy_tests.rs`, `keyring/encrypted_store_tests.rs`. - `core.rs` `#[cfg(test)] mod tests` — round-trips `SecretStore` encrypt/decrypt, `redact()` cases, `PairingGuard` defaults. - Sandbox-backend smoke: each backend file has its own `#[cfg(test)]` blocks where the binary is available. diff --git a/src/openhuman/security/mod.rs b/src/openhuman/security/mod.rs index 30765a7fb9..496066496e 100644 --- a/src/openhuman/security/mod.rs +++ b/src/openhuman/security/mod.rs @@ -13,6 +13,8 @@ pub mod policy; pub mod secrets; pub mod traits; +#[allow(unused_imports)] +pub use crate::openhuman::keyring::SecretStore; #[allow(unused_imports)] pub use audit::{ get_or_create_workspace_audit_logger, AuditEvent, AuditEventType, AuditLogger, @@ -34,8 +36,6 @@ pub use policy::AutonomyLevel; pub use policy::SecurityPolicy; pub use policy::ToolOperation; #[allow(unused_imports)] -pub use secrets::SecretStore; -#[allow(unused_imports)] pub use traits::{NoopSandbox, Sandbox}; pub use schemas::{ diff --git a/src/openhuman/security/secrets.rs b/src/openhuman/security/secrets.rs index b624919631..1406295d7f 100644 --- a/src/openhuman/security/secrets.rs +++ b/src/openhuman/security/secrets.rs @@ -1,637 +1 @@ -// Encrypted secret store — defense-in-depth for API keys and tokens. -// -// Secrets are encrypted using ChaCha20-Poly1305 AEAD with a random key stored -// in `{data_dir}/openhuman/.secret_key` with restrictive file permissions (0600). The -// config file stores only hex-encoded ciphertext, never plaintext keys. -// -// Each encryption generates a fresh random 12-byte nonce, prepended to the -// ciphertext. The Poly1305 authentication tag prevents tampering. -// -// This prevents: -// - Plaintext exposure in config files -// - Casual `grep` or `git log` leaks -// - Accidental commit of raw API keys -// - Known-plaintext attacks (unlike the previous XOR cipher) -// - Ciphertext tampering (authenticated encryption) -// -// For sovereign users who prefer plaintext, `secrets.encrypt = false` disables this. -// -// Migration: values with the legacy `enc:` prefix (XOR cipher) are decrypted -// using the old algorithm for backward compatibility. New encryptions always -// produce `enc2:` (ChaCha20-Poly1305). - -use anyhow::{Context, Result}; -use chacha20poly1305::aead::{Aead, KeyInit, OsRng}; -use chacha20poly1305::{AeadCore, ChaCha20Poly1305, Key, Nonce}; -use std::collections::HashMap; -use std::fs; -use std::path::{Path, PathBuf}; -use std::sync::{Mutex, OnceLock}; -use std::time::Duration; - -/// Length of the random encryption key in bytes (256-bit, matches `ChaCha20`). -const KEY_LEN: usize = 32; - -/// ChaCha20-Poly1305 nonce length in bytes. -const NONCE_LEN: usize = 12; - -/// Manages encrypted storage of secrets (API keys, tokens, etc.) -#[derive(Debug, Clone)] -pub struct SecretStore { - /// Path to the key file (`{data_dir}/openhuman/.secret_key`) - key_path: PathBuf, - /// Whether encryption is enabled - enabled: bool, -} - -impl SecretStore { - /// Create a new secret store rooted at the given directory. - pub fn new(openhuman_dir: &Path, enabled: bool) -> Self { - Self { - key_path: openhuman_dir.join(".secret_key"), - enabled, - } - } - - /// Encrypt a plaintext secret. Returns hex-encoded ciphertext prefixed with `enc2:`. - /// Format: `enc2:` (12 + N + 16 bytes). - /// If encryption is disabled, returns the plaintext as-is. - pub fn encrypt(&self, plaintext: &str) -> Result { - if !self.enabled || plaintext.is_empty() { - return Ok(plaintext.to_string()); - } - - let key_bytes = self.load_or_create_key()?; - let key = Key::from_slice(&key_bytes); - let cipher = ChaCha20Poly1305::new(key); - - let nonce = ChaCha20Poly1305::generate_nonce(&mut OsRng); - let ciphertext = cipher - .encrypt(&nonce, plaintext.as_bytes()) - .map_err(|e| anyhow::anyhow!("Encryption failed: {e}"))?; - - // Prepend nonce to ciphertext for storage - let mut blob = Vec::with_capacity(NONCE_LEN + ciphertext.len()); - blob.extend_from_slice(&nonce); - blob.extend_from_slice(&ciphertext); - - Ok(format!("enc2:{}", hex_encode(&blob))) - } - - /// Decrypt a secret. - /// - `enc2:` prefix → ChaCha20-Poly1305 (current format) - /// - `enc:` prefix → legacy XOR cipher (backward compatibility for migration) - /// - No prefix → returned as-is (plaintext config) - /// - /// **Warning**: Legacy `enc:` values are insecure. Use `decrypt_and_migrate` to - /// automatically upgrade them to the secure `enc2:` format. - pub fn decrypt(&self, value: &str) -> Result { - if let Some(hex_str) = value.strip_prefix("enc2:") { - self.decrypt_chacha20(hex_str) - } else if let Some(hex_str) = value.strip_prefix("enc:") { - self.decrypt_legacy_xor(hex_str) - } else { - Ok(value.to_string()) - } - } - - /// Decrypt a secret and return a migrated `enc2:` value if the input used legacy `enc:` format. - /// - /// Returns `(plaintext, Some(new_enc2_value))` if migration occurred, or - /// `(plaintext, None)` if no migration was needed. - /// - /// This allows callers to persist the upgraded value back to config. - pub fn decrypt_and_migrate(&self, value: &str) -> Result<(String, Option)> { - if let Some(hex_str) = value.strip_prefix("enc2:") { - // Already using secure format — no migration needed - let plaintext = self.decrypt_chacha20(hex_str)?; - Ok((plaintext, None)) - } else if let Some(hex_str) = value.strip_prefix("enc:") { - // Legacy XOR cipher — decrypt and re-encrypt with ChaCha20-Poly1305 - log::warn!( - "Decrypting legacy XOR-encrypted secret (enc: prefix). \ - This format is insecure and will be removed in a future release. \ - The secret will be automatically migrated to enc2: (ChaCha20-Poly1305)." - ); - let plaintext = self.decrypt_legacy_xor(hex_str)?; - let migrated = self.encrypt(&plaintext)?; - Ok((plaintext, Some(migrated))) - } else { - // Plaintext — no migration needed - Ok((value.to_string(), None)) - } - } - - /// Check if a value uses the legacy `enc:` format that should be migrated. - pub fn needs_migration(value: &str) -> bool { - value.starts_with("enc:") - } - - /// Decrypt using ChaCha20-Poly1305 (current secure format). - fn decrypt_chacha20(&self, hex_str: &str) -> Result { - let blob = - hex_decode(hex_str).context("Failed to decode encrypted secret (corrupt hex)")?; - anyhow::ensure!( - blob.len() > NONCE_LEN, - "Encrypted value too short (missing nonce)" - ); - - let (nonce_bytes, ciphertext) = blob.split_at(NONCE_LEN); - let nonce = Nonce::from_slice(nonce_bytes); - let key_bytes = self.load_or_create_key()?; - let key = Key::from_slice(&key_bytes); - let cipher = ChaCha20Poly1305::new(key); - - let plaintext_bytes = cipher - .decrypt(nonce, ciphertext) - .map_err(|_| anyhow::anyhow!("Decryption failed — wrong key or tampered data"))?; - - String::from_utf8(plaintext_bytes) - .context("Decrypted secret is not valid UTF-8 — corrupt data") - } - - /// Decrypt using legacy XOR cipher (insecure, for backward compatibility only). - fn decrypt_legacy_xor(&self, hex_str: &str) -> Result { - let ciphertext = hex_decode(hex_str) - .context("Failed to decode legacy encrypted secret (corrupt hex)")?; - let key = self.load_or_create_key()?; - let plaintext_bytes = xor_cipher(&ciphertext, &key); - String::from_utf8(plaintext_bytes) - .context("Decrypted legacy secret is not valid UTF-8 — wrong key or corrupt data") - } - - /// Check if a value is already encrypted (current or legacy format). - pub fn is_encrypted(value: &str) -> bool { - value.starts_with("enc2:") || value.starts_with("enc:") - } - - /// Check if a value uses the secure `enc2:` format. - pub fn is_secure_encrypted(value: &str) -> bool { - value.starts_with("enc2:") - } - - /// Load the encryption key from disk, or create one if it doesn't exist. - /// - /// The decoded key is cached process-wide keyed by `key_path`, so repeated - /// callers (e.g. every `app_state_snapshot` poll) hit memory instead of - /// disk. This also rides over transient Windows sharing violations that - /// can occur when an AV scanner briefly locks the file — once we've read - /// it successfully, we never need to read it again for this process. - fn load_or_create_key(&self) -> Result> { - // Normalize the path once so all callers share the same cache slot - // regardless of how `key_path` was spelled (relative vs absolute, - // symlinks, case-variants on Windows). - let cache_key_path = normalize_cache_path(&self.key_path); - - if let Some(cached) = cached_key(&cache_key_path) { - return Ok(cached); - } - - if self.key_path.exists() { - let read_result = read_key_file_with_retry(&self.key_path); - - // On Windows a previous bad icacls invocation may have stripped the - // inherited ACEs from %APPDATA% without granting the current user - // explicit access, leaving the file permanently unreadable. Attempt - // a one-shot ACL repair via `icacls /reset` before giving up. - #[cfg(windows)] - let read_result = if let Err(ref e) = read_result { - if is_permission_error(e) { - log::warn!( - "[security] PermissionDenied reading key file '{}'; \ - attempting icacls /reset self-repair", - self.key_path.display() - ); - repair_windows_acl(&self.key_path); - // Single retry regardless of whether repair reported success — - // icacls /reset may partially restore access even on a non-zero exit. - read_key_file_with_retry(&self.key_path) - } else { - read_result - } - } else { - read_result - }; - - let hex_key = read_result.with_context(|| { - let mut msg = format!( - "Failed to read secret key file at {}", - self.key_path.display() - ); - #[cfg(windows)] - { - msg.push_str( - "\n\nThis is often caused by incorrect file permissions on Windows. \ - Try repairing ACLs on the .openhuman directory:\n\ - icacls \"%USERPROFILE%\\.openhuman\" /reset /t /c\n\ - icacls \"%USERPROFILE%\\.openhuman\\.secret_key\" /reset /c", - ); - } - msg - })?; - let key = decode_key_hex(hex_key.trim())?; - cache_key(&cache_key_path, &key); - Ok(key) - } else { - let key = generate_random_key(); - let hex_key = hex_encode(&key); - if let Some(parent) = self.key_path.parent() { - fs::create_dir_all(parent)?; - } - - // Write key file with restrictive permissions atomically on Unix - // to avoid a TOCTOU race where the file is briefly world-readable, - // and to avoid clobbering a key another process created first. - // See: src/core/auth.rs:write_token_file for the reference pattern. - #[cfg(unix)] - { - use std::io::Write as _; - use std::os::unix::fs::OpenOptionsExt as _; - let open_result = fs::OpenOptions::new() - .write(true) - .create_new(true) - .mode(0o600) - .open(&self.key_path); - - match open_result { - Ok(mut file) => { - file.write_all(hex_key.as_bytes()) - .context("Failed to write secret key file")?; - } - Err(error) if error.kind() == std::io::ErrorKind::AlreadyExists => { - let existing_key = read_key_file_with_retry(&self.key_path) - .with_context(|| { - format!( - "Secret key file was created concurrently but could not be read at {}", - self.key_path.display() - ) - }) - .and_then(|existing_hex| decode_key_hex(existing_hex.trim()))?; - cache_key(&cache_key_path, &existing_key); - return Ok(existing_key); - } - Err(error) => { - return Err(error).context("Failed to create secret key file"); - } - } - } - #[cfg(not(unix))] - { - use std::io::Write as _; - - let open_result = fs::OpenOptions::new() - .write(true) - .create_new(true) - .open(&self.key_path); - - match open_result { - Ok(mut file) => { - file.write_all(hex_key.as_bytes()) - .context("Failed to write secret key file")?; - } - Err(error) if error.kind() == std::io::ErrorKind::AlreadyExists => { - let existing_key = read_key_file_with_retry(&self.key_path) - .with_context(|| { - format!( - "Secret key file was created concurrently but could not be read at {}", - self.key_path.display() - ) - }) - .and_then(|existing_hex| decode_key_hex(existing_hex.trim()))?; - cache_key(&cache_key_path, &existing_key); - return Ok(existing_key); - } - Err(error) => { - return Err(error).context("Failed to create secret key file"); - } - } - } - #[cfg(windows)] - { - // On Windows, use icacls to restrict permissions to current user only. - // We use USERDOMAIN\USERNAME so the account is resolved correctly on - // domain-joined and AAD-joined machines (bare USERNAME is ambiguous and - // may refer to a local account that doesn't match the signed-in user). - let username = std::env::var("USERNAME").unwrap_or_default(); - let userdomain = std::env::var("USERDOMAIN").unwrap_or_default(); - let computername = std::env::var("COMPUTERNAME").unwrap_or_default(); - let qualified_username = - qualify_windows_username(&username, &userdomain, &computername); - let Some(grant_arg) = build_windows_icacls_grant_arg(&qualified_username) else { - log::warn!( - "[security] USERNAME/USERDOMAIN environment variables are empty; \ - cannot restrict key file permissions via icacls" - ); - cache_key(&cache_key_path, &key); - return Ok(key); - }; - - let icacls_ok = match std::process::Command::new("icacls") - .arg(&self.key_path) - .args(["/inheritance:r", "/grant:r"]) - .arg(&grant_arg) - .output() - { - Ok(o) if o.status.success() => { - log::debug!("[security] key file permissions restricted via icacls"); - true - } - Ok(o) => { - log::warn!( - "[security] icacls exited {:?} for account '{}'; \ - restoring inherited ACLs so the file remains readable", - o.status.code(), - grant_arg, - ); - false - } - Err(e) => { - log::warn!( - "[security] could not run icacls: {e}; \ - restoring inherited ACLs" - ); - false - } - }; - // If the icacls grant command failed, the `/inheritance:r` flag may have - // already stripped the inherited ACEs that let the current user read the - // file. Explicitly reset to restore inheritance so the file is always - // readable — a slightly weaker ACL is preferable to a locked-out user. - if !icacls_ok { - let _ = std::process::Command::new("icacls") - .arg(&self.key_path) - .args(["/reset"]) - .output(); - } - } - - cache_key(&cache_key_path, &key); - Ok(key) - } - } -} - -/// Normalize a path into a stable cache key. Tries `canonicalize` first (so -/// symlinks, relative paths, and Windows case-variants all collapse to the -/// same key), falls back to `std::path::absolute` when the file does not yet -/// exist (e.g. the create branch in `load_or_create_key`), and finally to the -/// raw path so a normalization failure never breaks the cache. -fn normalize_cache_path(path: &Path) -> PathBuf { - fs::canonicalize(path) - .or_else(|_| std::path::absolute(path)) - .unwrap_or_else(|_| path.to_path_buf()) -} - -/// Process-wide cache of decoded key bytes keyed by absolute path. -/// -/// Loading the key once per process is both faster and more reliable than -/// re-reading `.secret_key` on every decrypt. On Windows the file can be -/// transiently inaccessible (AV scanners holding a handle), and re-reading -/// turned that transient failure into a perma-failure for every subsequent -/// RPC call. -fn key_cache() -> &'static Mutex>> { - static CACHE: OnceLock>>> = OnceLock::new(); - CACHE.get_or_init(|| Mutex::new(HashMap::new())) -} - -fn cached_key(path: &Path) -> Option> { - key_cache().lock().ok()?.get(path).cloned() -} - -fn cache_key(path: &Path, key: &[u8]) { - if let Ok(mut cache) = key_cache().lock() { - cache.insert(path.to_path_buf(), key.to_vec()); - } -} - -/// Clear the cached key for `path`. Test-only — production code should never -/// need to invalidate the cache, since the key file is write-once. -#[cfg(test)] -pub(super) fn clear_cached_key(path: &Path) { - let normalized = normalize_cache_path(path); - if let Ok(mut cache) = key_cache().lock() { - cache.remove(&normalized); - } -} - -/// Read the key file, retrying transient errors a handful of times. -/// -/// Windows AV scanners (Defender, etc.) routinely hold short-lived read -/// handles right after a file is created, which surfaces as -/// `ERROR_SHARING_VIOLATION` (raw OS error 32) or `PermissionDenied`. A few -/// short backoffs are enough to ride over the lock; the typical successful -/// path returns on the first attempt with zero added latency. -fn read_key_file_with_retry(path: &Path) -> std::io::Result { - use std::io::ErrorKind; - - const MAX_ATTEMPTS: u32 = 5; - let mut last_err: Option = None; - for attempt in 0..MAX_ATTEMPTS { - match fs::read_to_string(path) { - Ok(contents) => return Ok(contents), - Err(err) => { - let transient = matches!( - err.kind(), - ErrorKind::PermissionDenied | ErrorKind::Interrupted | ErrorKind::WouldBlock - ) || err.raw_os_error() == Some(32); // ERROR_SHARING_VIOLATION (Windows) - last_err = Some(err); - if !transient || attempt + 1 == MAX_ATTEMPTS { - break; - } - let backoff_ms = 10u64 << attempt; // 10, 20, 40, 80 ms - std::thread::sleep(Duration::from_millis(backoff_ms)); - } - } - } - Err(last_err.unwrap_or_else(|| std::io::Error::other("read_to_string failed"))) -} - -/// Returns `true` when an `std::io::Error` is a permanent permission/access -/// denial rather than a transient sharing violation. -#[cfg(windows)] -fn is_permission_error(e: &std::io::Error) -> bool { - matches!(e.kind(), std::io::ErrorKind::PermissionDenied) || e.raw_os_error() == Some(5) - // ERROR_ACCESS_DENIED -} - -/// Attempt to repair a locked key file by running `icacls /reset` on it. -/// -/// Attempt to repair a locked key file. -/// -/// Two-step process: -/// 1. `icacls /reset` — removes all explicit ACEs and re-enables ACL -/// inheritance from the parent directory. -/// 2. `icacls /grant:r :F` — explicit grant for the current -/// user as a belt-and-suspenders fallback for environments (e.g. CI -/// temp dirs) where the parent's inheritance chain may not include the -/// runner account. -/// -/// Returns `true` if the file is actually readable after the repair attempt, -/// regardless of which step(s) succeeded. -#[cfg(windows)] -pub(super) fn repair_windows_acl(path: &Path) -> bool { - // Step 1: restore inheritance. - match std::process::Command::new("icacls") - .arg(path) - .args(["/reset"]) - .output() - { - Ok(o) if o.status.success() => { - log::info!( - "[security] icacls /reset succeeded for '{}'; ACL inheritance restored", - path.display() - ); - } - Ok(o) => { - log::warn!( - "[security] icacls /reset exited {:?} for '{}'", - o.status.code(), - path.display() - ); - } - Err(e) => { - log::warn!( - "[security] could not run icacls /reset for '{}': {e}", - path.display() - ); - } - } - - // Step 2: explicit grant for current user — handles CI environments - // where the temp/app dir's inheritable ACEs don't include the runner. - let username = std::env::var("USERNAME").unwrap_or_default(); - let userdomain = std::env::var("USERDOMAIN").unwrap_or_default(); - let computername = std::env::var("COMPUTERNAME").unwrap_or_default(); - let qualified = qualify_windows_username(&username, &userdomain, &computername); - if let Some(grant_arg) = build_windows_icacls_grant_arg(&qualified) { - match std::process::Command::new("icacls") - .arg(path) - .args(["/grant:r"]) - .arg(&grant_arg) - .output() - { - Ok(o) if o.status.success() => { - log::debug!( - "[security] explicit grant '{grant_arg}' succeeded during repair of '{}'", - path.display() - ); - } - Ok(o) => { - log::warn!( - "[security] explicit grant '{grant_arg}' exited {:?} during repair of '{}'", - o.status.code(), - path.display() - ); - } - Err(e) => { - log::warn!( - "[security] could not run icacls /grant during repair of '{}': {e}", - path.display() - ); - } - } - } - - // Return whether the file is actually readable now — callers use this - // for logging/metrics; the retry in load_or_create_key is unconditional. - std::fs::read(path).is_ok() -} - -/// XOR cipher with repeating key. Same function for encrypt and decrypt. -fn xor_cipher(data: &[u8], key: &[u8]) -> Vec { - if key.is_empty() { - return data.to_vec(); - } - data.iter() - .enumerate() - .map(|(i, &b)| b ^ key[i % key.len()]) - .collect() -} - -/// Generate a random 256-bit key using the OS CSPRNG. -/// -/// Uses `OsRng` (via `getrandom`) directly, providing full 256-bit entropy -/// without the fixed version/variant bits that UUID v4 introduces. -fn generate_random_key() -> Vec { - ChaCha20Poly1305::generate_key(&mut OsRng).to_vec() -} - -fn decode_key_hex(hex_key: &str) -> Result> { - let key = hex_decode(hex_key).context("Secret key file is corrupt")?; - anyhow::ensure!( - key.len() == KEY_LEN, - "Secret key file has wrong length: expected {KEY_LEN} bytes, got {}", - key.len() - ); - Ok(key) -} - -/// Hex-encode bytes to a lowercase hex string. -fn hex_encode(data: &[u8]) -> String { - let mut s = String::with_capacity(data.len() * 2); - for b in data { - use std::fmt::Write; - let _ = write!(s, "{b:02x}"); - } - s -} - -/// Build the `/grant` argument for `icacls` using a normalized username. -/// Returns `None` when the username is empty or whitespace-only. -fn build_windows_icacls_grant_arg(username: &str) -> Option { - let normalized = username.trim(); - if normalized.is_empty() { - return None; - } - Some(format!("{normalized}:F")) -} - -/// Produce a domain-qualified Windows account name suitable for `icacls`. -/// -/// On domain-joined machines `USERDOMAIN` is the domain name and differs from -/// `COMPUTERNAME`. On standalone machines they are equal, so we use the bare -/// `username` in that case to avoid a redundant `DESKTOP-XYZ\alice` prefix. -/// -/// Returns an empty string when both inputs are empty (caller must treat this -/// as "cannot determine account name"). -#[cfg(windows)] -fn qualify_windows_username(username: &str, userdomain: &str, computername: &str) -> String { - let username = username.trim(); - let userdomain = userdomain.trim(); - let computername = computername.trim(); - - if username.is_empty() { - return String::new(); - } - - // If USERDOMAIN is set and differs from COMPUTERNAME the machine is - // domain/AAD-joined; use the fully-qualified form so icacls resolves the - // account unambiguously. - if !userdomain.is_empty() - && !computername.is_empty() - && !userdomain.eq_ignore_ascii_case(computername) - { - format!("{userdomain}\\{username}") - } else { - username.to_string() - } -} - -/// Hex-decode a hex string to bytes. -#[allow(clippy::manual_is_multiple_of)] -fn hex_decode(hex: &str) -> Result> { - if (hex.len() & 1) != 0 { - anyhow::bail!("Hex string has odd length"); - } - (0..hex.len()) - .step_by(2) - .map(|i| { - u8::from_str_radix(&hex[i..i + 2], 16) - .map_err(|e| anyhow::anyhow!("Invalid hex at position {i}: {e}")) - }) - .collect() -} - -#[cfg(test)] -#[path = "secrets_tests.rs"] -mod tests; +pub use crate::openhuman::keyring::encrypted_store::*; diff --git a/tests/json_rpc_e2e.rs b/tests/json_rpc_e2e.rs index 9c342a1356..eddc43cda4 100644 --- a/tests/json_rpc_e2e.rs +++ b/tests/json_rpc_e2e.rs @@ -61,10 +61,14 @@ impl Drop for EnvVarGuard { /// process-global, so parallel tests would clobber each other and hit the wrong `config.toml` or /// inherited `VITE_BACKEND_URL`. static JSON_RPC_E2E_ENV_LOCK: OnceLock> = OnceLock::new(); +static JSON_RPC_E2E_KEYRING_INIT: OnceLock<()> = OnceLock::new(); static CHAT_COMPLETION_MODELS: OnceLock>> = OnceLock::new(); static CHAT_COMPLETION_REQUESTS: OnceLock>> = OnceLock::new(); fn json_rpc_e2e_env_lock() -> std::sync::MutexGuard<'static, ()> { + JSON_RPC_E2E_KEYRING_INIT.get_or_init(|| unsafe { + std::env::set_var("OPENHUMAN_KEYRING_BACKEND", "file"); + }); let mutex = JSON_RPC_E2E_ENV_LOCK.get_or_init(|| Mutex::new(())); // Recover from poison so that a panic in one test does not cascade to all others. match mutex.lock() { @@ -776,9 +780,11 @@ async fn wait_for_chat_completion_requests_len(expected_len: usize) -> Vec String { + let _keyring_backend_guard = EnvVarGuard::set("OPENHUMAN_KEYRING_BACKEND", "file"); let config = openhuman_core::openhuman::config::load_config_with_timeout() .await .expect("load config for encrypted test mnemonic"); + openhuman_core::openhuman::keyring::init_workspace(&config.workspace_dir); openhuman_core::openhuman::encryption::rpc::encrypt_secret( &config, "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about", diff --git a/tests/keyring_secretstore_e2e.rs b/tests/keyring_secretstore_e2e.rs new file mode 100644 index 0000000000..263df8c0f9 --- /dev/null +++ b/tests/keyring_secretstore_e2e.rs @@ -0,0 +1,141 @@ +use openhuman_core::openhuman::config::schema::{Config, StreamMode, TelegramConfig}; +use openhuman_core::openhuman::keyring; +use std::sync::{Mutex, OnceLock}; + +fn env_lock() -> std::sync::MutexGuard<'static, ()> { + static LOCK: OnceLock> = OnceLock::new(); + LOCK.get_or_init(|| Mutex::new(())).lock().unwrap() +} + +struct EnvGuard { + key: &'static str, + previous: Option, +} + +impl EnvGuard { + fn set(key: &'static str, value: &std::path::Path) -> Self { + let previous = std::env::var_os(key); + unsafe { + std::env::set_var(key, value); + } + Self { key, previous } + } + + fn set_str(key: &'static str, value: &'static str) -> Self { + let previous = std::env::var_os(key); + unsafe { + std::env::set_var(key, value); + } + Self { key, previous } + } +} + +impl Drop for EnvGuard { + fn drop(&mut self) { + match self.previous.take() { + Some(value) => unsafe { + std::env::set_var(self.key, value); + }, + None => unsafe { + std::env::remove_var(self.key); + }, + } + } +} + +#[tokio::test] +async fn config_secrets_roundtrip_via_keyring_backed_master_key_migration() { + let _guard = env_lock(); + let tmp = tempfile::tempdir().expect("tempdir"); + let openhuman_dir = tmp.path().join("user-123"); + let workspace_dir = openhuman_dir.join("workspace"); + std::fs::create_dir_all(&workspace_dir).expect("workspace dir"); + + let _keyring_backend = EnvGuard::set_str("OPENHUMAN_KEYRING_BACKEND", "file"); + let _workspace_override = EnvGuard::set("OPENHUMAN_WORKSPACE", &openhuman_dir); + keyring::init_workspace(&workspace_dir); + + let legacy_key_path = openhuman_dir.join(".secret_key"); + std::fs::write( + &legacy_key_path, + "000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f", + ) + .expect("legacy key file"); + + let config_path = openhuman_dir.join("config.toml"); + let config = Config { + config_path: config_path.clone(), + workspace_dir: workspace_dir.clone(), + api_key: Some("sk-direct-secret".into()), + search: openhuman_core::openhuman::config::schema::SearchConfig { + parallel: openhuman_core::openhuman::config::schema::SearchEngineCredentials { + api_key: Some("parallel-secret".into()), + ..Default::default() + }, + ..Default::default() + }, + channels_config: openhuman_core::openhuman::config::schema::ChannelsConfig { + telegram: Some(TelegramConfig { + bot_token: "tg-bot-secret".into(), + allowed_users: vec!["alice".into()], + stream_mode: StreamMode::default(), + draft_update_interval_ms: 1000, + silent_streaming: true, + mention_only: false, + }), + ..Default::default() + }, + ..Default::default() + }; + + config.save().await.expect("save config"); + + assert!( + !legacy_key_path.exists(), + "legacy .secret_key should be deleted after verified migration" + ); + + let config_toml = tokio::fs::read_to_string(&config_path) + .await + .expect("read config.toml"); + assert!( + config_toml.contains("enc2:"), + "config should store ciphertext" + ); + assert!( + !config_toml.contains("sk-direct-secret"), + "config should not contain plaintext api_key" + ); + assert!( + !config_toml.contains("parallel-secret"), + "config should not contain plaintext search api key" + ); + assert!( + !config_toml.contains("tg-bot-secret"), + "config should not contain plaintext telegram bot token" + ); + + let keyring_file = workspace_dir.join("dev-keychain.json"); + let keyring_payload = tokio::fs::read_to_string(&keyring_file) + .await + .expect("read dev-keychain"); + assert!( + keyring_payload.contains("user-123:secretstore.master_key"), + "migrated master key should live in keyring backend payload" + ); + + let loaded = Config::load_or_init().await.expect("reload config"); + assert_eq!(loaded.api_key.as_deref(), Some("sk-direct-secret")); + assert_eq!( + loaded.search.parallel.api_key.as_deref(), + Some("parallel-secret") + ); + assert_eq!( + loaded + .channels_config + .telegram + .as_ref() + .map(|cfg| cfg.bot_token.as_str()), + Some("tg-bot-secret") + ); +} diff --git a/tests/keyring_secretstore_fresh_e2e.rs b/tests/keyring_secretstore_fresh_e2e.rs new file mode 100644 index 0000000000..b3a115b018 --- /dev/null +++ b/tests/keyring_secretstore_fresh_e2e.rs @@ -0,0 +1,139 @@ +use openhuman_core::openhuman::config::schema::{Config, StreamMode, TelegramConfig}; +use openhuman_core::openhuman::keyring; +use std::sync::{Mutex, OnceLock}; + +fn env_lock() -> std::sync::MutexGuard<'static, ()> { + static LOCK: OnceLock> = OnceLock::new(); + LOCK.get_or_init(|| Mutex::new(())).lock().unwrap() +} + +struct EnvGuard { + key: &'static str, + previous: Option, +} + +impl EnvGuard { + fn set(key: &'static str, value: &std::path::Path) -> Self { + let previous = std::env::var_os(key); + unsafe { + std::env::set_var(key, value); + } + Self { key, previous } + } + + fn set_str(key: &'static str, value: &'static str) -> Self { + let previous = std::env::var_os(key); + unsafe { + std::env::set_var(key, value); + } + Self { key, previous } + } +} + +impl Drop for EnvGuard { + fn drop(&mut self) { + match self.previous.take() { + Some(value) => unsafe { + std::env::set_var(self.key, value); + }, + None => unsafe { + std::env::remove_var(self.key); + }, + } + } +} + +fn parse_keyring_payload(json: &str) -> serde_json::Value { + serde_json::from_str(json).expect("parse keyring json") +} + +#[tokio::test] +async fn config_secrets_create_master_key_in_keyring_on_fresh_install() { + let _guard = env_lock(); + let tmp = tempfile::tempdir().expect("tempdir"); + let openhuman_dir = tmp.path().join("fresh-user"); + let workspace_dir = openhuman_dir.join("workspace"); + std::fs::create_dir_all(&workspace_dir).expect("workspace dir"); + + let _keyring_backend = EnvGuard::set_str("OPENHUMAN_KEYRING_BACKEND", "file"); + let _workspace_override = EnvGuard::set("OPENHUMAN_WORKSPACE", &openhuman_dir); + keyring::init_workspace(&workspace_dir); + + let legacy_key_path = openhuman_dir.join(".secret_key"); + assert!( + !legacy_key_path.exists(), + "fresh install should not start with legacy key file" + ); + + let config_path = openhuman_dir.join("config.toml"); + let backup_path = openhuman_dir.join("config.toml.bak"); + let config = Config { + config_path: config_path.clone(), + workspace_dir: workspace_dir.clone(), + api_key: Some("sk-fresh-secret".into()), + channels_config: openhuman_core::openhuman::config::schema::ChannelsConfig { + telegram: Some(TelegramConfig { + bot_token: "fresh-tg-secret".into(), + allowed_users: vec!["bob".into()], + stream_mode: StreamMode::default(), + draft_update_interval_ms: 1000, + silent_streaming: true, + mention_only: false, + }), + ..Default::default() + }, + ..Default::default() + }; + + config.save().await.expect("first save"); + config.save().await.expect("second save"); + + assert!( + !legacy_key_path.exists(), + "fresh install should not create a legacy key file" + ); + + let keyring_file = workspace_dir.join("dev-keychain.json"); + let keyring_payload = tokio::fs::read_to_string(&keyring_file) + .await + .expect("read dev-keychain"); + let parsed = parse_keyring_payload(&keyring_payload); + let key_entry = parsed + .get("fresh-user:secretstore.master_key") + .and_then(|v| v.as_str()) + .expect("master key entry"); + assert_eq!(key_entry.len(), 64, "master key should be 32 bytes as hex"); + + let config_toml = tokio::fs::read_to_string(&config_path) + .await + .expect("read config.toml"); + let backup_toml = tokio::fs::read_to_string(&backup_path) + .await + .expect("read config.toml.bak"); + + for payload in [&config_toml, &backup_toml] { + assert!( + payload.contains("enc2:"), + "persisted config should be encrypted" + ); + assert!( + !payload.contains("sk-fresh-secret"), + "persisted config should not leak plaintext api_key" + ); + assert!( + !payload.contains("fresh-tg-secret"), + "persisted config should not leak plaintext bot token" + ); + } + + let loaded = Config::load_or_init().await.expect("reload config"); + assert_eq!(loaded.api_key.as_deref(), Some("sk-fresh-secret")); + assert_eq!( + loaded + .channels_config + .telegram + .as_ref() + .map(|cfg| cfg.bot_token.as_str()), + Some("fresh-tg-secret") + ); +}