diff --git a/crates/nuncio-cli/src/runner.rs b/crates/nuncio-cli/src/runner.rs index 5bb57d5..edaabf7 100644 --- a/crates/nuncio-cli/src/runner.rs +++ b/crates/nuncio-cli/src/runner.rs @@ -1,11 +1,13 @@ //! Headless engine runner executing CLI commands against core services. use nuncio_core::EventBus; -use nuncio_store::vault::{SecretManager, GRPC_TOKEN_ACCOUNT}; +use nuncio_store::vault::{ + get_or_create_key_bytes_bounded, BoundedVaultError, SecretManager, GRPC_TOKEN_ACCOUNT, + KEYRING_READ_TIMEOUT, +}; use nuncio_store::{DatabaseEngine, DatabaseError}; use serde_json::json; use std::sync::Arc; -use std::time::Duration; use thiserror::Error; use crate::args::{ @@ -28,12 +30,6 @@ use crate::output::{format_json, format_json_error, format_json_error_with_info} /// nothing can answer without an interactive desktop session. pub const GRPC_TOKEN_ENV: &str = "NUNCIO_GRPC_TOKEN"; -/// Upper bound on how long a keyring read may block before the CLI gives up and -/// reports an actionable error. Generous enough that a human can answer a -/// keychain consent prompt, short enough that an unattended run always -/// terminates instead of stalling forever. -const KEYRING_READ_TIMEOUT: Duration = Duration::from_secs(20); - /// Normalizes a raw environment value into a usable token override. /// /// Surrounding whitespace is stripped (shell heredocs and `env` files routinely @@ -446,15 +442,10 @@ impl HeadlessRunner { /// Resolves the gRPC bearer token, preferring an explicit override over the /// OS keyring. /// - /// The keyring read runs on a detached OS thread behind a bounded timeout - /// rather than inline. On macOS a keychain item whose ACL does not trust - /// this binary makes the read block on a GUI consent prompt that never - /// arrives in a headless session, and that call cannot be cancelled — so - /// the timeout must be able to abandon the thread. A plain - /// `std::thread::spawn` is used instead of `spawn_blocking` precisely - /// because Tokio joins its blocking pool during runtime shutdown, which - /// would reintroduce the hang at process exit; a detached thread is reaped - /// when the process ends. + /// The keyring read is bounded by [`get_or_create_key_bytes_bounded`], so a + /// keychain that never answers ends in an error rather than an unbounded + /// stall (see that helper for why the read is abandoned rather than + /// cancelled). /// /// The returned error never carries token material — only the vault's own /// failure description, which is keyed by account name, not secret value. @@ -463,32 +454,25 @@ impl HeadlessRunner { return Ok(token.clone()); } - let secrets = Arc::clone(&self.secrets); - let (tx, rx) = tokio::sync::oneshot::channel(); - std::thread::spawn(move || { - let resolved = secrets - .get_or_create_key_bytes(GRPC_TOKEN_ACCOUNT, 32) - .map(hex::encode) - .map_err(|e| e.to_string()); - // A closed receiver means the timeout already fired; nothing to do. - let _ = tx.send(resolved); - }); - - match tokio::time::timeout(KEYRING_READ_TIMEOUT, rx).await { - Ok(Ok(Ok(token))) => Ok(token), - Ok(Ok(Err(e))) => Err(format!("failed to read gRPC bearer token from vault: {e}")), - Ok(Err(_)) => { - Err("gRPC bearer token lookup ended without producing a result".to_string()) - } - Err(_) => Err(format!( + match get_or_create_key_bytes_bounded( + Arc::clone(&self.secrets), + GRPC_TOKEN_ACCOUNT, + 32, + KEYRING_READ_TIMEOUT, + ) + .await + { + Ok(bytes) => Ok(hex::encode(bytes)), + Err(BoundedVaultError::TimedOut { timeout, .. }) => Err(format!( "timed out after {}s reading the gRPC bearer token from the OS keyring: the \ keyring is most likely waiting on an interactive access-consent prompt that \ cannot be answered in this session (the token belongs to nunciod, so another \ binary reading it needs consent). Run this command from a logged-in desktop \ session, or set {GRPC_TOKEN_ENV} to the daemon's bearer token to bypass the \ keyring entirely", - KEYRING_READ_TIMEOUT.as_secs() + timeout.as_secs() )), + Err(e) => Err(format!("failed to read gRPC bearer token from vault: {e}")), } } diff --git a/crates/nuncio-store/src/vault.rs b/crates/nuncio-store/src/vault.rs index a3fb2a7..0840a4c 100644 --- a/crates/nuncio-store/src/vault.rs +++ b/crates/nuncio-store/src/vault.rs @@ -1,6 +1,7 @@ use aes_gcm::aead::{rand_core::RngCore, OsRng}; use std::collections::HashMap; use std::sync::{Arc, Mutex}; +use std::time::Duration; use thiserror::Error; use zeroize::{Zeroize, ZeroizeOnDrop}; @@ -241,6 +242,87 @@ impl SecretManager { } } +/// Upper bound on how long a keyring read may block before the caller gives up +/// and reports an actionable error. +/// +/// Generous enough that a human at a logged-in desktop can answer a keychain +/// consent prompt, short enough that an unattended process always terminates +/// instead of stalling forever. +pub const KEYRING_READ_TIMEOUT: Duration = Duration::from_secs(20); + +/// Why a bounded key-material read did not produce a key. +/// +/// No variant ever carries key material: the only value interpolated into these +/// messages is the vault *account name*, which names a purpose (for example +/// `grpc-bearer-token`), never a secret. +#[derive(Debug, Error)] +pub enum BoundedVaultError { + /// The underlying vault read completed but failed. + #[error(transparent)] + Vault(#[from] VaultError), + /// The read did not finish within the allotted bound. + #[error( + "timed out after {}s reading key material '{account}' from the OS keyring: the keyring \ + did not answer, which usually means it is locked (screen-lock or a never-unlocked login \ + session) or is waiting on an access-consent prompt that no one can answer in this \ + session (a keychain whose ACL no longer trusts this binary -- after a restore, \ + migration, or rebuild -- prompts every time)", + .timeout.as_secs() + )] + TimedOut { + /// Vault account name whose read timed out. + account: String, + /// Bound that elapsed. + timeout: Duration, + }, + /// The reader ended without reporting either success or failure. + #[error("the '{account}' keyring read ended without producing a result")] + Lost { + /// Vault account name whose read was lost. + account: String, + }, +} + +/// [`SecretManager::get_or_create_key_bytes`], bounded so it always terminates. +/// +/// The read runs on a detached OS thread rather than inline, because a platform +/// keyring read is an uncancellable blocking call: a locked keychain, or one +/// whose ACL no longer trusts the calling binary, blocks on a consent prompt +/// that never arrives in a headless or screen-locked session. Bounding it +/// therefore means abandoning the thread, not cancelling the call. +/// +/// A plain `std::thread::spawn` is used instead of `spawn_blocking` precisely +/// because Tokio joins its blocking pool during runtime shutdown, which would +/// reintroduce the same hang at process exit; a detached thread is reaped when +/// the process ends. +pub async fn get_or_create_key_bytes_bounded( + secrets: Arc, + account: &str, + len: usize, + timeout: Duration, +) -> Result, BoundedVaultError> { + let owned_account = account.to_string(); + let thread_account = owned_account.clone(); + let (tx, rx) = tokio::sync::oneshot::channel(); + std::thread::spawn(move || { + let resolved = secrets.get_or_create_key_bytes(&thread_account, len); + // A closed receiver means the bound already elapsed; nothing to do. + let _ = tx.send(resolved); + }); + + match tokio::time::timeout(timeout, rx).await { + Ok(Ok(Ok(bytes))) => Ok(bytes), + Ok(Ok(Err(e))) => Err(BoundedVaultError::Vault(e)), + Ok(Err(_)) => Err(BoundedVaultError::Lost { + account: owned_account, + }), + Err(_) => Err(BoundedVaultError::TimedOut { + account: owned_account, + timeout, + }), + } +} + #[cfg(test)] mod tests { use super::*; @@ -397,4 +479,117 @@ mod tests { // headless CI never depends on a real credential store being available. let _ = manager; } + + /// Vault provider whose reads never return, standing in for a locked + /// keychain or one waiting on an access-consent prompt nobody can answer. + struct BlockingVault; + + impl SecretVault for BlockingVault { + fn get_secret(&self, _key: &str) -> Result { + loop { + std::thread::park(); + } + } + fn set_secret(&self, _key: &str, _secret: &str) -> Result<(), VaultError> { + Ok(()) + } + fn delete_secret(&self, _key: &str) -> Result<(), VaultError> { + Ok(()) + } + } + + #[tokio::test] + async fn bounded_read_gives_up_on_a_keyring_that_never_answers() { + let secrets = Arc::new(SecretManager::new(BlockingVault)); + + let started = std::time::Instant::now(); + let err = get_or_create_key_bytes_bounded( + secrets, + GRPC_TOKEN_ACCOUNT, + 32, + Duration::from_millis(150), + ) + .await + .expect_err("a read that never returns must not succeed"); + + assert!( + matches!(err, BoundedVaultError::TimedOut { .. }), + "expected a timeout, got: {err}" + ); + assert!( + started.elapsed() < Duration::from_secs(5), + "the bound must actually terminate the read" + ); + let message = err.to_string(); + assert!( + message.contains(GRPC_TOKEN_ACCOUNT), + "the error must name which key material could not be read: {message}" + ); + assert!( + message.contains("locked") && message.contains("consent"), + "the error must suggest the likely cause: {message}" + ); + } + + #[tokio::test] + async fn bounded_read_mints_and_reloads_key_material_like_the_unbounded_one() { + let secrets = Arc::new(SecretManager::mock()); + + let minted = get_or_create_key_bytes_bounded( + Arc::clone(&secrets), + GRPC_TOKEN_ACCOUNT, + 32, + KEYRING_READ_TIMEOUT, + ) + .await + .expect("mock vault mints key material"); + assert_eq!(minted.len(), 32); + + let reloaded = get_or_create_key_bytes_bounded( + Arc::clone(&secrets), + GRPC_TOKEN_ACCOUNT, + 32, + KEYRING_READ_TIMEOUT, + ) + .await + .expect("mock vault reloads the same key material"); + assert_eq!(minted, reloaded, "a second read must return the same key"); + assert_eq!( + reloaded, + secrets + .get_or_create_key_bytes(GRPC_TOKEN_ACCOUNT, 32) + .expect("unbounded read agrees"), + "bounding a read must not change what it returns" + ); + } + + #[tokio::test] + async fn bounded_read_surfaces_a_vault_failure_verbatim() { + struct FailingVault; + impl SecretVault for FailingVault { + fn get_secret(&self, _key: &str) -> Result { + Err(VaultError::StorageFailed("keyring is locked".into())) + } + fn set_secret(&self, _key: &str, _secret: &str) -> Result<(), VaultError> { + Ok(()) + } + fn delete_secret(&self, _key: &str) -> Result<(), VaultError> { + Ok(()) + } + } + + let err = get_or_create_key_bytes_bounded( + Arc::new(SecretManager::new(FailingVault)), + WEBHOOK_SIGNING_KEY_ACCOUNT, + 32, + KEYRING_READ_TIMEOUT, + ) + .await + .expect_err("a failing vault must fail closed"); + + assert!( + matches!(err, BoundedVaultError::Vault(_)), + "a real vault failure must not be reported as a timeout: {err}" + ); + } } diff --git a/crates/nunciod/src/lib.rs b/crates/nunciod/src/lib.rs index a87fa7e..39e53ea 100644 --- a/crates/nunciod/src/lib.rs +++ b/crates/nunciod/src/lib.rs @@ -12,6 +12,7 @@ pub mod orchestrator; pub mod outbox; pub mod pagination; pub mod rpc_trace; +pub mod secrets; pub mod send; pub mod sync; pub mod sync_dispatcher; diff --git a/crates/nunciod/src/main.rs b/crates/nunciod/src/main.rs index 134a805..74d8426 100644 --- a/crates/nunciod/src/main.rs +++ b/crates/nunciod/src/main.rs @@ -263,11 +263,14 @@ async fn main() -> Result<(), Box> { // `SecretManager` to write account password credentials to the OS // keyring (never to SQLite) -- see `nunciod::grpc`'s security comment on // why EVERY mounted service shares this one `BearerAuthInterceptor`. + // The read is bounded: a keyring that never answers must end in this + // logged startup failure, not in a daemon that hangs here forever. let grpc_secrets = account_secrets.clone(); - let grpc_token_bytes = grpc_secrets - .get_or_create_key_bytes(nuncio_store::vault::GRPC_TOKEN_ACCOUNT, 32) - .map_err(|e| format!("failed to provision gRPC bearer token from vault: {e}"))?; - let grpc_token = hex::encode(grpc_token_bytes); + let grpc_token = nunciod::secrets::provision_grpc_token( + grpc_secrets.clone(), + nuncio_store::vault::KEYRING_READ_TIMEOUT, + ) + .await?; let grpc_addr = nunciod::grpc::grpc_addr_from_env(); tracing::info!( "nunciod gRPC (nuncio.v1.System, nuncio.v1.Accounts, nuncio.v1.Mail, nuncio.v1.Filters, \ diff --git a/crates/nunciod/src/outbox.rs b/crates/nunciod/src/outbox.rs index 77fcaaf..595bd8d 100644 --- a/crates/nunciod/src/outbox.rs +++ b/crates/nunciod/src/outbox.rs @@ -40,7 +40,7 @@ use nuncio_mail::{ RemoteMutationSpec, SmtpTransportEngine, }; use nuncio_store::db::{DatabaseEngine, DatabaseError}; -use nuncio_store::vault::{SecretManager, VaultError, WEBHOOK_SIGNING_KEY_ACCOUNT}; +use nuncio_store::vault::{SecretManager, VaultError}; use std::sync::Arc; use std::time::Duration; use thiserror::Error; @@ -618,23 +618,22 @@ impl ProductionExecutionEnv { /// the vault is unavailable, so a `CALL WEBHOOK` is never dispatched /// unsigned -- and, being lazy, that failure is isolated to webhook /// dispatch and never disables the other mutation paths. - fn webhook_dispatcher(&self) -> Result<&WebhookDispatcher, WebhookError> { + /// + /// The vault read is bounded, so a keyring that never answers fails this + /// one dispatch honestly instead of parking the outbox worker forever. + async fn webhook_dispatcher(&self) -> Result<&WebhookDispatcher, WebhookError> { if let Some(dispatcher) = self.webhook.get() { return Ok(dispatcher); } - let key_bytes = self - .secrets - .get_or_create_key_bytes(WEBHOOK_SIGNING_KEY_ACCOUNT, 32) - .map_err(|e| { - WebhookError::SigningError(format!( - "failed to provision webhook signing key from the vault: {e}" - )) - })?; + let key = crate::secrets::provision_webhook_signing_key( + Arc::clone(&self.secrets), + nuncio_store::vault::KEYRING_READ_TIMEOUT, + ) + .await + .map_err(WebhookError::SigningError)?; // A concurrent caller may win the race to set it; either way the stored // dispatcher is authoritative. - let _ = self - .webhook - .set(WebhookDispatcher::new(hex::encode(key_bytes))); + let _ = self.webhook.set(WebhookDispatcher::new(key)); self.webhook.get().ok_or_else(|| { WebhookError::SigningError("webhook dispatcher unavailable after provisioning".into()) }) @@ -682,7 +681,8 @@ impl RemoteExecutionEnv for ProductionExecutionEnv { ) -> Result { // Production keeps the secure default egress policy (private/loopback // targets are rejected). - self.webhook_dispatcher()? + self.webhook_dispatcher() + .await? .dispatch_with_options( url, rule_id, diff --git a/crates/nunciod/src/secrets.rs b/crates/nunciod/src/secrets.rs new file mode 100644 index 0000000..5fbb300 --- /dev/null +++ b/crates/nunciod/src/secrets.rs @@ -0,0 +1,61 @@ +//! Bounded provisioning of the daemon's own key material. +//! +//! `nunciod` normally *mints* the keys it uses, so the OS keyring hands them +//! back without ceremony. That is a habit, not a guarantee: a keychain locked +//! by screen-lock, a restored or migrated keychain whose ACL no longer trusts +//! this binary, a re-signed or rebuilt `nunciod`, or a Linux Secret Service +//! with no unlocked collection all turn a keyring read into a blocking wait on +//! something nobody can answer. Unbounded, that wait becomes a daemon that +//! hangs at startup with no error and no log line. +//! +//! Every read here therefore goes through +//! [`nuncio_store::vault::get_or_create_key_bytes_bounded`], so the worst case +//! is an honest, logged startup failure instead of a silent stall. Errors name +//! the key material's purpose and the likely cause; they never carry the key. + +use nuncio_store::vault::{ + get_or_create_key_bytes_bounded, SecretManager, GRPC_TOKEN_ACCOUNT, WEBHOOK_SIGNING_KEY_ACCOUNT, +}; +use std::sync::Arc; +use std::time::Duration; + +/// Byte length of the daemon's symmetric key material (256 bits). +const KEY_LEN: usize = 32; + +/// Provision the loopback gRPC bearer token, minting it on first run and +/// loading it back on every subsequent one. +/// +/// Fails closed: the daemon must never serve its API with a token it could not +/// confirm, and must never invent one. +pub async fn provision_grpc_token( + secrets: Arc, + timeout: Duration, +) -> Result { + get_or_create_key_bytes_bounded(secrets, GRPC_TOKEN_ACCOUNT, KEY_LEN, timeout) + .await + .map(hex::encode) + .map_err(|e| { + format!( + "failed to provision the gRPC bearer token that nunciod needs to authenticate \ + its loopback API: {e}" + ) + }) +} + +/// Provision the daemon-wide HMAC key that signs outbound filter webhooks. +/// +/// Fails closed: a `CALL WEBHOOK` is never dispatched unsigned. +pub async fn provision_webhook_signing_key( + secrets: Arc, + timeout: Duration, +) -> Result { + get_or_create_key_bytes_bounded(secrets, WEBHOOK_SIGNING_KEY_ACCOUNT, KEY_LEN, timeout) + .await + .map(hex::encode) + .map_err(|e| { + format!( + "failed to provision the webhook signing key that nunciod needs to sign outbound \ + filter webhooks: {e}" + ) + }) +} diff --git a/crates/nunciod/tests/daemon_keyring_bound_test.rs b/crates/nunciod/tests/daemon_keyring_bound_test.rs new file mode 100644 index 0000000..dfd53a9 --- /dev/null +++ b/crates/nunciod/tests/daemon_keyring_bound_test.rs @@ -0,0 +1,151 @@ +//! The daemon's own keyring reads must always terminate. +//! +//! `nunciod` provisions its bearer token and webhook signing key at startup and +//! on first webhook dispatch. A keyring that never answers -- a locked keychain, +//! an ACL that no longer trusts this binary, a Secret Service with no unlocked +//! collection -- used to leave the daemon parked on that read with no error and +//! no log line. These tests pin the bounded behaviour: an honest error, named +//! key material, and never the key itself. + +#![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] + +use nuncio_store::vault::{SecretManager, SecretVault, VaultError}; +use nunciod::secrets::{provision_grpc_token, provision_webhook_signing_key}; +use std::sync::Arc; +use std::time::{Duration, Instant}; + +/// Sentinel standing in for real key material: if any of it ever reaches an +/// error message, the assertions below catch it. +const SENTINEL_KEY_HEX: &str = "53454e54494e454c6b65796d6174657269616c4141424243434444454546464747"; + +/// Vault whose reads never return, standing in for a locked keychain or one +/// waiting on an access-consent prompt nobody can answer. +struct BlockingVault; + +impl SecretVault for BlockingVault { + fn get_secret(&self, _key: &str) -> Result { + loop { + std::thread::park(); + } + } + fn set_secret(&self, _key: &str, _secret: &str) -> Result<(), VaultError> { + Ok(()) + } + fn delete_secret(&self, _key: &str) -> Result<(), VaultError> { + Ok(()) + } +} + +fn blocked_secrets() -> Arc { + Arc::new(SecretManager::new(BlockingVault)) +} + +#[tokio::test] +async fn blocked_bearer_token_read_fails_instead_of_hanging_startup() { + let started = Instant::now(); + let err = provision_grpc_token(blocked_secrets(), Duration::from_millis(150)) + .await + .expect_err("a keyring that never answers must not yield a token"); + + assert!( + started.elapsed() < Duration::from_secs(5), + "startup must give up on the read, not wait on it" + ); + assert!( + err.contains("gRPC bearer token"), + "the error must name which key material could not be read: {err}" + ); + assert!( + err.contains("locked") || err.contains("consent"), + "the error must suggest the likely cause: {err}" + ); + assert!( + !err.contains(SENTINEL_KEY_HEX), + "an error must never carry key material: {err}" + ); +} + +#[tokio::test] +async fn blocked_webhook_key_read_fails_instead_of_hanging_the_outbox() { + let started = Instant::now(); + let err = provision_webhook_signing_key(blocked_secrets(), Duration::from_millis(150)) + .await + .expect_err("a keyring that never answers must not yield a signing key"); + + assert!( + started.elapsed() < Duration::from_secs(5), + "the outbox must give up on the read, not wait on it" + ); + assert!( + err.contains("webhook signing key"), + "the error must name which key material could not be read: {err}" + ); + assert!( + err.contains("locked") || err.contains("consent"), + "the error must suggest the likely cause: {err}" + ); + assert!( + !err.contains(SENTINEL_KEY_HEX), + "an error must never carry key material: {err}" + ); +} + +#[tokio::test] +async fn a_working_keyring_still_mints_and_reloads_the_same_key_material() { + let secrets = Arc::new(SecretManager::mock()); + let timeout = Duration::from_secs(5); + + let token = provision_grpc_token(Arc::clone(&secrets), timeout) + .await + .expect("the mock vault mints a token"); + // 32 bytes, hex-encoded. + assert_eq!(token.len(), 64); + assert_eq!( + token, + provision_grpc_token(Arc::clone(&secrets), timeout) + .await + .expect("the token reloads across reads"), + "bounding the read must not change the token the daemon serves with" + ); + + let webhook_key = provision_webhook_signing_key(Arc::clone(&secrets), timeout) + .await + .expect("the mock vault mints a signing key"); + assert_eq!(webhook_key.len(), 64); + assert_ne!( + token, webhook_key, + "each purpose must get its own key material" + ); +} + +#[tokio::test] +async fn a_failing_keyring_reports_the_vault_failure_not_a_timeout() { + struct FailingVault; + impl SecretVault for FailingVault { + fn get_secret(&self, _key: &str) -> Result { + Err(VaultError::StorageFailed("no unlocked collection".into())) + } + fn set_secret(&self, _key: &str, _secret: &str) -> Result<(), VaultError> { + Ok(()) + } + fn delete_secret(&self, _key: &str) -> Result<(), VaultError> { + Ok(()) + } + } + + let err = provision_grpc_token( + Arc::new(SecretManager::new(FailingVault)), + Duration::from_secs(5), + ) + .await + .expect_err("a failing vault must fail closed"); + + assert!( + err.contains("no unlocked collection"), + "a real vault failure must surface verbatim: {err}" + ); + assert!( + !err.contains("timed out"), + "a real vault failure must not be reported as a timeout: {err}" + ); +}