Repository navigation
feat(security): secrets and credentials on the storage backend #7183
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.
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
Changes from all commits
cc258d0
cd699f3
90a624e
2a3f39f
b9b1866
bef7dca
f33c38a
95cd6bd
b56dadb
bc547f2
57c700a
da2b516
5d7b0af
0fed911
3a5a523
397ccd6
ebc5bb1
f5344c8
9d8e496
ef59bd4
5e1212a
bcbc3ee
7382cd7
621445c
c8002d3
e3d78e1
cff1d30
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -89,6 +89,12 @@ path = "../../tests/storage_approvals_e2e.rs" | |
| name = "storage_domains_e2e" | ||
| path = "../../tests/storage_domains_e2e.rs" | ||
|
|
||
| [[test]] | ||
| # Its own binary: it installs a storage backend into the process-wide slot and | ||
| # sets the keyring master-key environment. | ||
| name = "storage_secrets_e2e" | ||
| path = "../../tests/storage_secrets_e2e.rs" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add the registered integration-test source file The referenced [RULE] missing-test-target-source · |
||
|
|
||
| [[test]] | ||
| name = "embedded_server_shutdown_e2e" | ||
| path = "../../tests/embedded_server_shutdown_e2e.rs" | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -152,7 +152,7 @@ tinyagents-registry = { path = "../../vendor/tinyagents/crates/tinyagents-regist | |||||
| tinyagents-session = { path = "../../vendor/tinyagents/crates/tinyagents-session", features = ["storage-drivers"] } | ||||||
| # The storage ports and the URL-to-backend facade (`storage` domain). No driver | ||||||
| # by default: the `storage-*` features below add them. | ||||||
| tinystoragedrivers = { path = "../../vendor/tinyagents/vendor/tinystoragedrivers/crates/tinystoragedrivers", default-features = false, features = ["blocking"] } | ||||||
| tinystoragedrivers = { path = "../../vendor/tinyagents/vendor/tinystoragedrivers/crates/tinystoragedrivers", default-features = false, features = ["blocking", "secrets"] } | ||||||
|
senamakel marked this conversation as resolved.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove the undeclared dependency feature Cargo rejects features that are not declared by the selected package. No
Suggested change
[RULE] undeclared-feature · |
||||||
| tinyagents-runtime = { path = "../../vendor/tinyagents/crates/tinyagents-runtime" } | ||||||
| tinyagents-orchestration = { path = "../../vendor/tinyagents/crates/tinyagents-orchestration" } | ||||||
| tinyagents-tasks = { path = "../../vendor/tinyagents/crates/tinyagents-tasks" } | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -334,6 +334,44 @@ impl HttpCredentialsStore { | |
| } | ||
|
|
||
| fn read_persisted(&self) -> Result<PersistedHttpCredentials> { | ||
| if let Some(secrets) = crate::storage::secrets::current()? { | ||
|
senamakel marked this conversation as resolved.
senamakel marked this conversation as resolved.
|
||
| return match crate::storage::secrets::get_blocking(&secrets, STORAGE_SECRET_NAME)? { | ||
| Some(bytes) => serde_json::from_slice(&bytes) | ||
| .context("http-credentials record on the storage backend is not valid JSON"), | ||
| None => { | ||
| // First use of the backend: adopt the on-disk store so | ||
|
senamakel marked this conversation as resolved.
|
||
| // existing credentials stay visible and the next upsert | ||
| // cannot replace them. The file is left in place. | ||
| let legacy = self.read_file()?; | ||
| if self.path.exists() && !legacy.credentials.is_empty() { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Keep the legacy credential store synchronized with the backend Residual of the earlier "Remove or synchronize the legacy credential store" finding: the legacy file is read once for adoption and then left in place, and writes after that go only to storage, so the file permanently freezes at its pre-backend content. That is now the intended design (the e2e test asserts the files are byte-identical after backend writes) and the README documents it, so the earlier "remove or synchronize" demand is withdrawn as a bug and reduced to a caution: a host that later uninstalls the storage backend (the e2e test exercises [RULE] legacy-sync · |
||
| let json = serde_json::to_vec(&legacy) | ||
| .context("failed to serialize migrated http-credentials store")?; | ||
| // A failed adoption keeps the legacy credentials | ||
| // readable; the next read retries it. | ||
| match crate::storage::secrets::set_blocking( | ||
| &secrets, | ||
| STORAGE_SECRET_NAME, | ||
| &json, | ||
| ) { | ||
| Ok(()) => log::info!( | ||
| target: "credentials", | ||
| "[credentials] migrated http-credentials file to storage secret" | ||
| ), | ||
| Err(_) => log::warn!( | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Surface why the http-credentials migration failed Still stands from the earlier revision: the [RULE] silent-error · |
||
| target: "credentials", | ||
| "[credentials] http-credentials migration to storage failed; will retry" | ||
| ), | ||
| } | ||
| } | ||
| Ok(legacy) | ||
| } | ||
| }; | ||
| } | ||
| self.read_file() | ||
| } | ||
|
|
||
| /// Reads the on-disk `http-credentials.json` store (empty when absent). | ||
| fn read_file(&self) -> Result<PersistedHttpCredentials> { | ||
| if !self.path.exists() { | ||
| return Ok(PersistedHttpCredentials::default()); | ||
| } | ||
|
|
@@ -355,6 +393,12 @@ impl HttpCredentialsStore { | |
| } | ||
|
|
||
| fn write_persisted(&self, persisted: &PersistedHttpCredentials) -> Result<()> { | ||
| if let Some(secrets) = crate::storage::secrets::current()? { | ||
| let json = serde_json::to_vec(persisted) | ||
| .context("failed to serialize http-credentials store")?; | ||
| crate::storage::secrets::set_blocking(&secrets, STORAGE_SECRET_NAME, &json)?; | ||
|
senamakel marked this conversation as resolved.
|
||
| return Ok(()); | ||
| } | ||
| if let Some(parent) = self.path.parent() { | ||
| fs::create_dir_all(parent).with_context(|| { | ||
| format!( | ||
|
|
@@ -388,6 +432,11 @@ impl HttpCredentialsStore { | |
| } | ||
| } | ||
|
|
||
| /// The secret the store lives in on a storage backend | ||
| /// ([`crate::storage::secrets`]): the same JSON as the file, encrypted as one | ||
| /// secret in the acting agent's scope. | ||
| const STORAGE_SECRET_NAME: &str = "file:http-credentials.json"; | ||
|
|
||
| fn parse_scheme(raw: &str) -> Option<HttpCredentialScheme> { | ||
| match raw.trim().to_ascii_lowercase().as_str() { | ||
| "bearer" => Some(HttpCredentialScheme::Bearer), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,11 @@ use super::{ | |
| PERSIST_RETRY_BASE_MS, PROFILES_FILENAME, | ||
| }; | ||
|
|
||
| /// The secret the profiles live in on a storage backend | ||
| /// ([`crate::storage::secrets`]): the same JSON as `auth-profiles.json`, | ||
| /// encrypted as one secret in the acting agent's scope. | ||
| const STORAGE_SECRET_NAME: &str = "file:auth-profiles.json"; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a storage URL is enabled on the standard non-SaaS app/CLI/TUI launch, the root context has no AGENTS.md reference: AGENTS.md:L22-L22 Useful? React with 👍 / 👎. |
||
|
|
||
| impl AuthProfilesStore { | ||
| pub(super) fn save_locked(&self, data: &AuthProfilesData) -> Result<()> { | ||
| let mut persisted = PersistedAuthProfiles { | ||
|
|
@@ -114,6 +119,56 @@ impl AuthProfilesStore { | |
| } | ||
|
|
||
| pub(super) fn read_persisted_locked(&self) -> Result<PersistedAuthProfiles> { | ||
| if let Some(secrets) = crate::storage::secrets::current()? { | ||
|
senamakel marked this conversation as resolved.
|
||
| let Some(bytes) = crate::storage::secrets::get_blocking(&secrets, STORAGE_SECRET_NAME)? | ||
|
senamakel marked this conversation as resolved.
|
||
| else { | ||
|
senamakel marked this conversation as resolved.
|
||
| // First use of the backend: adopt the profiles the on-disk | ||
| // store holds, so they do not vanish from reads and the next | ||
| // save cannot replace them with an empty set. The file is left | ||
| // in place (never deleted by this path). | ||
| let legacy = self.read_file_locked()?; | ||
| if self.path.exists() && !legacy.profiles.is_empty() { | ||
|
senamakel marked this conversation as resolved.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do not resurrect profiles from the legacy file The adoption path still re-uploads the entire on-disk store whenever the storage record is absent and the file is non-empty. The file is intentionally left in place (the diff and README say so), so any later absence of the storage record — a deletion of the secret, a scope reset — makes the next read restore profiles that were removed while on the backend. Adoption should be one-shot (e.g. tombstone recorded on the backend once adopted, or the legacy file retired after a successful migration) rather than retried on every missing-record read. [RULE] legacy-store-resurrection · |
||
| let json = serde_json::to_vec(&legacy) | ||
| .context("Failed to serialize migrated auth profiles")?; | ||
| // A failed adoption keeps the legacy profiles readable; | ||
| // the next read retries it. | ||
| match crate::storage::secrets::set_blocking( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security & Privacy | 🛡️ Detected with Advanced Tier | 🟠 Major | 🏗️ Heavy lift 🧩 Analysis chain🏁 Script executed: set -eu
printf '%s\n' '--- changed files ---'
git diff --stat c76d5c2b1a692a47f621ecf468bb9c37d72f940d bcbc3ee4e4f75f916573ac98814ede98e26b0dc7
printf '%s\n' '--- relevant diff ---'
git diff --unified=35 c76d5c2b1a692a47f621ecf468bb9c37d72f940d bcbc3ee4e4f75f916573ac98814ede98e26b0dc7 -- crates/openhuman-core/src/security/credentials/profiles/persistence.rs crates/openhuman-core/src/security/credentials/http_creds.rs crates/openhuman-core/src/storage
printf '%s\n' '--- storage and credential bindings ---'
rg -n -F --glob '*.rs' -- 'pub fn current' crates/openhuman-core/src crates 2>/dev/null || true
rg -n -F --glob '*.rs' -- 'set_blocking' crates/openhuman-core/src/security crates/openhuman-core/src/storage 2>/dev/null || true
rg -n -F --glob '*.rs' -- 'STORAGE_SECRET_NAME' crates/openhuman-core/src/security crates/openhuman-core/src/storage 2>/dev/null || true
rg -n -F --glob '*.rs' -- 'state_dir' crates/openhuman-core/src/security crates/openhuman-core/src/storage 2>/dev/null || true
rg -n -F --glob '*.rs' -- 'auth-profiles.json' crates/openhuman-core/src/security crates/openhuman-core/src/storage 2>/dev/null || true
rg -n -F --glob '*.rs' -- 'http' crates/openhuman-core/src/security/credentials/http_creds.rs | head -80 || true
printf '%s\n' '--- relevant source blocks ---'
sed -n '1,240p' crates/openhuman-core/src/security/credentials/profiles/persistence.rs
sed -n '250,410p' crates/openhuman-core/src/security/credentials/http_creds.rs
find crates/openhuman-core/src -path '*storage*' -type f -maxdepth 8 -printRepository: tinyhumansai/openhuman Length of output: 42729 🏁 Script executed: set -eu
printf '%s\n' '--- storage secrets implementation ---'
nl -ba crates/openhuman-core/src/storage/secrets.rs | sed -n '1,260p'
printf '%s\n' '--- storage documentation ---'
nl -ba crates/openhuman-core/src/storage/README.md | sed -n '1,180p'
printf '%s\n' '--- credential store declarations and constructors ---'
rg -n -F --glob '*.rs' -- 'struct AuthProfilesStore' crates/openhuman-core/src/security/credentials
rg -n -F --glob '*.rs' -- 'AuthProfilesStore::new' crates/openhuman-core/src
rg -n -F --glob '*.rs' -- 'struct HttpCredentialsStore' crates/openhuman-core/src/security/credentials
rg -n -F --glob '*.rs' -- 'HttpCredentialsStore::new' crates/openhuman-core/src
rg -n -F --glob '*.rs' -- 'PROFILES_FILENAME' crates/openhuman-core/src/security/credentials
rg -n -F --glob '*.rs' -- 'STORE_FILENAME' crates/openhuman-core/src/security/credentials/http_creds.rs
printf '%s\n' '--- enclosing credential definitions and path construction ---'
nl -ba crates/openhuman-core/src/security/credentials/profiles/mod.rs | sed -n '1,300p'
nl -ba crates/openhuman-core/src/security/credentials/http_creds.rs | sed -n '1,180p'
printf '%s\n' '--- config/storage scope references ---'
rg -n -i --glob '*.rs' --glob '*.md' 'agent.?scope|per.?agent|multi.?agent|workspace.*state|state.*dir|OPENHUMAN_STORAGE_URL|storage.*scope' crates/openhuman-core/src crates/openhuman-core/README.md docs README.md 2>/dev/null || trueRepository: tinyhumansai/openhuman Length of output: 42239 🏁 Script executed: set -eu
printf '%s\n' '--- storage secrets implementation ---'
nl -ba crates/openhuman-core/src/storage/secrets.rs | sed -n '1,260p'
printf '%s\n' '--- storage documentation ---'
nl -ba crates/openhuman-core/src/storage/README.md | sed -n '1,180p'
printf '%s\n' '--- credential store declarations and constructors ---'
rg -n -F --glob '*.rs' -- 'struct AuthProfilesStore' crates/openhuman-core/src/security/credentials
rg -n -F --glob '*.rs' -- 'AuthProfilesStore::new' crates/openhuman-core/src
rg -n -F --glob '*.rs' -- 'struct HttpCredentialsStore' crates/openhuman-core/src/security/credentials
rg -n -F --glob '*.rs' -- 'HttpCredentialsStore::new' crates/openhuman-core/src
rg -n -F --glob '*.rs' -- 'PROFILES_FILENAME' crates/openhuman-core/src/security/credentials
rg -n -F --glob '*.rs' -- 'STORE_FILENAME' crates/openhuman-core/src/security/credentials/http_creds.rs
printf '%s\n' '--- enclosing credential definitions and path construction ---'
nl -ba crates/openhuman-core/src/security/credentials/profiles/mod.rs | sed -n '1,320p'
nl -ba crates/openhuman-core/src/security/credentials/http_creds.rs | sed -n '1,180p'
printf '%s\n' '--- config/storage scope references ---'
rg -n -i --glob '*.rs' --glob '*.md' 'agent.?scope|per.?agent|multi.?agent|workspace.*state|state.*dir|OPENHUMAN_STORAGE_URL|storage.*scope' crates/openhuman-core/src crates/openhuman-core/README.md docs README.md 2>/dev/null || trueRepository: tinyhumansai/openhuman Length of output: 41803 Sensitive Data Exposure Reachability: Internal Bind legacy credential files to the acting storage scope before adoption.
📍 Affects 2 files
🤖 Prompt for AI Agents |
||
| &secrets, | ||
| STORAGE_SECRET_NAME, | ||
| &json, | ||
| ) { | ||
| Ok(()) => tracing::info!( | ||
| "[credentials] migrated auth profiles file to storage secret" | ||
| ), | ||
| Err(_) => tracing::warn!( | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| "[credentials] auth profiles migration to storage failed; will retry" | ||
| ), | ||
| } | ||
| } | ||
| return Ok(legacy); | ||
| }; | ||
| // No quarantine on a storage backend: an unparseable record is an | ||
| // error, so a later write can never replace profiles it could not | ||
| // read. | ||
| let mut persisted: PersistedAuthProfiles = serde_json::from_slice(&bytes) | ||
| .context("auth profile record on the storage backend is not valid JSON")?; | ||
| if persisted.schema_version == 0 { | ||
| persisted.schema_version = CURRENT_SCHEMA_VERSION; | ||
| } | ||
| if persisted.schema_version > CURRENT_SCHEMA_VERSION { | ||
| anyhow::bail!( | ||
| "Unsupported auth profile schema version {} (max supported: {})", | ||
| persisted.schema_version, | ||
| CURRENT_SCHEMA_VERSION | ||
| ); | ||
| } | ||
| return Ok(persisted); | ||
| } | ||
| self.read_file_locked() | ||
| } | ||
|
|
||
| /// Reads the on-disk `auth-profiles.json` store (empty when absent). | ||
| fn read_file_locked(&self) -> Result<PersistedAuthProfiles> { | ||
| if !self.path.exists() { | ||
| return Ok(PersistedAuthProfiles::default()); | ||
| } | ||
|
|
@@ -163,6 +218,12 @@ impl AuthProfilesStore { | |
| } | ||
|
|
||
| pub(super) fn write_persisted_locked(&self, persisted: &PersistedAuthProfiles) -> Result<()> { | ||
| if let Some(secrets) = crate::storage::secrets::current()? { | ||
| let json = | ||
| serde_json::to_vec(persisted).context("Failed to serialize auth profiles")?; | ||
| crate::storage::secrets::set_blocking(&secrets, STORAGE_SECRET_NAME, &json)?; | ||
| return Ok(()); | ||
| } | ||
| if let Some(parent) = self.path.parent() { | ||
| fs::create_dir_all(parent).with_context(|| { | ||
| format!( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,6 +45,11 @@ type MasterKeyInit = Result<Option<[u8; KEY_LEN]>, String>; | |
| /// Process-wide master-key outcome, set once by [`init_master_key`]. | ||
| static MASTER_KEY: OnceLock<MasterKeyInit> = OnceLock::new(); | ||
|
|
||
| /// Set when [`init_master_key`] found the OS keychain unable to provide the | ||
| /// key, so storage secrets reuse that outcome instead of retrying the | ||
| /// keychain (and its prompt) on their first operation. | ||
| static KEYCHAIN_UNAVAILABLE: OnceLock<()> = OnceLock::new(); | ||
|
|
||
| // ── Public API for core startup ────────────────────────────────────────────── | ||
|
|
||
| /// Initialize the keyring subsystem: set the workspace directory and load | ||
|
|
@@ -109,12 +114,53 @@ pub fn init_master_key() -> Result<(), String> { | |
| // Surface the denied state to the frontend instead of silently | ||
| // resetting — this is the "warn before reset" the issue asks for. | ||
| crate::security::keyring_consent::policy::notify_master_key_unavailable(&e); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reuse the recorded keychain failure for storage keys Still stands in substance: [RULE] error-swallow · |
||
| let _ = KEYCHAIN_UNAVAILABLE.set(()); | ||
| Ok(None) | ||
| } | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| /// The master key that encrypts secrets on a configured storage backend | ||
| /// ([`crate::storage::secrets`]): the key [`init_master_key`] loaded when | ||
| /// there is one, otherwise the same resolution run once for storage — | ||
| /// [`MASTER_KEY_ENV`] / [`MASTER_KEY_FILE_ENV`] first, then the OS keychain. | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// When no source can provide the key. Storage secrets then fail closed: | ||
| /// they are never written unencrypted or under a freshly minted key that | ||
| /// would orphan the ones already stored. | ||
| pub(crate) fn storage_master_key() -> Result<[u8; KEY_LEN], String> { | ||
| // Only a loaded key is cached: a failure (locked keychain, denied prompt) | ||
| // is retried on the next call so secrets recover once access is restored. | ||
| static STORAGE_MASTER_KEY: OnceLock<[u8; KEY_LEN]> = OnceLock::new(); | ||
| if let Some(Ok(Some(key))) = MASTER_KEY.get() { | ||
| return Ok(*key); | ||
| } | ||
| // `init_master_key` already tried the keychain this session and it | ||
| // failed: reuse that outcome, do not prompt again. (`Ok(None)` alone also | ||
| // means "backend needs no key / init skipped", which must still load.) | ||
| if KEYCHAIN_UNAVAILABLE.get().is_some() { | ||
| return Err("OS keychain master key unavailable this session".into()); | ||
| } | ||
| if let Some(key) = STORAGE_MASTER_KEY.get() { | ||
| return Ok(*key); | ||
| } | ||
| match try_load_master_key() { | ||
|
senamakel marked this conversation as resolved.
|
||
| Ok((key, source)) => { | ||
| log::info!("[keyring:storage] master key loaded from {source}"); | ||
| Ok(*STORAGE_MASTER_KEY.get_or_init(|| key)) | ||
| } | ||
| Err(MasterKeyError::Configured(_) | MasterKeyError::Keychain(_)) => { | ||
| // Fixed message: the underlying error can carry a path taken | ||
| // from `MASTER_KEY_FILE_ENV`. | ||
| log::error!("[keyring:storage] master key unavailable"); | ||
| Err("master key unavailable".into()) | ||
| } | ||
| } | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| /// Runs `init` at most once per `cell` and reports its outcome on every call. | ||
| /// | ||
| /// A configuration error is stored in the cell rather than leaving it empty | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add the registered integration test source
The manifest now declares
storage_secrets_e2e, but repository search finds notests/storage_secrets_e2e.rsoutside this manifest entry. Cargo will fail when it tries to load this integration-test target, so add the source file at that path or remove this target until the test exists.[RULE] missing-source-file ·