commit 2d7a2a46391fc0204e6c6b4bd862058f2e0b50a5 parent 1603e367223a3ebe99aab994b9dd10384975569d Author: triesap <tyson@radroots.org> Date: Fri, 28 Aug 2026 00:40:28 +0000 refactor(secrets): await credential operations Diffstat:
14 files changed, 404 insertions(+), 225 deletions(-)
diff --git a/AGENTS.md b/AGENTS.md @@ -126,7 +126,10 @@ substitute. - The native host owns its Tokio runtime for exactly one application-core lifetime. Runtime close is explicit, idempotent, and cancellation-resumable; observer work and the bounded keyring worker must finish before terminal - close is reported. Authoritative locks fail closed on poison. + close is reported. `SecretStore` is an object-safe asynchronous application + port. Every caller awaits it, Tokio workers await one-shot results, and only + the dedicated credential thread may drive the blocking platform adapter. + Authoritative locks fail closed on poison. - Services-hardening changes use the approved target-state contracts. Do not add compatibility aliases, dual reads, dual writes, or fallback behavior for prototype surfaces removed by the clean-slate refactor. diff --git a/README.md b/README.md @@ -82,8 +82,10 @@ relay URL, destination, DNS, connection, and bounded-fetch behavior. The native FFI host owns one runtime per application core and closes it idempotently. Cancelling a close never reopens command admission, and a later close call resumes the same shutdown. Operating-system keyring calls run -through a bounded supervised worker rather than directly on an async runtime -worker. +through an object-safe asynchronous application port and a bounded supervised +worker rather than directly on an async runtime worker. Callers await one-shot +responses; the dedicated operating-system thread alone drives the blocking +platform adapter. ## Project documentation diff --git a/core/compatibility/harvestcircle-storage-api-v1.txt b/core/compatibility/harvestcircle-storage-api-v1.txt @@ -54,10 +54,10 @@ impl core::fmt::Debug for harvestcircle_storage::HarvestCircleStorageContract pub fn harvestcircle_storage::HarvestCircleStorageContract::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result pub struct harvestcircle_storage::OsKeyringSecretStore impl harvestcircle_application::secrets::SecretStore for harvestcircle_storage::OsKeyringSecretStore -pub fn harvestcircle_storage::OsKeyringSecretStore::contains(&self, harvestcircle_domain::key::PublicKey) -> core::result::Result<bool, harvestcircle_domain::error::SafeError> -pub fn harvestcircle_storage::OsKeyringSecretStore::delete(&self, harvestcircle_domain::key::PublicKey) -> core::result::Result<(), harvestcircle_domain::error::SafeError> -pub fn harvestcircle_storage::OsKeyringSecretStore::load(&self, harvestcircle_domain::key::PublicKey) -> core::result::Result<harvestcircle_domain::key::SecretKeyInput, harvestcircle_domain::error::SafeError> -pub fn harvestcircle_storage::OsKeyringSecretStore::put(&self, harvestcircle_domain::key::PublicKey, harvestcircle_domain::key::SecretKeyInput) -> core::result::Result<(), harvestcircle_domain::error::SafeError> +pub fn harvestcircle_storage::OsKeyringSecretStore::contains(&self, harvestcircle_domain::key::PublicKey) -> harvestcircle_application::ports::BoxFuture<'_, core::result::Result<bool, harvestcircle_domain::error::SafeError>> +pub fn harvestcircle_storage::OsKeyringSecretStore::delete(&self, harvestcircle_domain::key::PublicKey) -> harvestcircle_application::ports::BoxFuture<'_, core::result::Result<(), harvestcircle_domain::error::SafeError>> +pub fn harvestcircle_storage::OsKeyringSecretStore::load(&self, harvestcircle_domain::key::PublicKey) -> harvestcircle_application::ports::BoxFuture<'_, core::result::Result<harvestcircle_domain::key::SecretKeyInput, harvestcircle_domain::error::SafeError>> +pub fn harvestcircle_storage::OsKeyringSecretStore::put(&self, harvestcircle_domain::key::PublicKey, harvestcircle_domain::key::SecretKeyInput) -> harvestcircle_application::ports::BoxFuture<'_, core::result::Result<(), harvestcircle_domain::error::SafeError>> pub struct harvestcircle_storage::VerifiedHarvestCircleBackup impl core::fmt::Debug for harvestcircle_storage::VerifiedHarvestCircleBackup pub fn harvestcircle_storage::VerifiedHarvestCircleBackup::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result diff --git a/core/crates/harvestcircle_application/src/identities.rs b/core/crates/harvestcircle_application/src/identities.rs @@ -167,11 +167,11 @@ impl AppCore { if let Some(existing) = &previous && (local_keyring_binding(existing)?.availability() != SignerAvailability::CredentialMissing - || secrets.contains(public_key)?) + || secrets.contains(public_key).await?) { return Err(identity_exists()); } - if previous.is_none() && secrets.contains(public_key)? { + if previous.is_none() && secrets.contains(public_key).await? { return Err(identity_exists()); } let identity = if let Some(existing) = &previous { @@ -262,7 +262,7 @@ impl AppCore { }; } } - secrets.put(identity.public_key(), secret)?; + secrets.put(identity.public_key(), secret).await?; operations .advance_durable_operation( request_id, @@ -372,7 +372,7 @@ impl AppCore { } let identity = ®istry[index]; let local_keyring = local_keyring_binding(identity)?; - match secrets.delete(public_key) { + match secrets.delete(public_key).await { Ok(()) => {} Err(error) if error.code() == SafeErrorCode::CredentialMissing @@ -468,7 +468,7 @@ impl AppCore { { self.sign_out()?; } - match secrets.delete(public_key) { + match secrets.delete(public_key).await { Ok(()) => {} Err(error) if error.code() == SafeErrorCode::CredentialMissing @@ -605,7 +605,7 @@ impl AppCore { if let Some(existing) = identities.find_identity(public_key).await? { if local_keyring_binding(&existing)?.availability() != SignerAvailability::CredentialMissing - || secrets.contains(public_key)? + || secrets.contains(public_key).await? { return Err(identity_exists()); } @@ -630,7 +630,7 @@ impl AppCore { })?; return Ok(ImportIdentityReceipt { identity: repaired }); } - if secrets.contains(public_key)? { + if secrets.contains(public_key).await? { return Err(identity_exists()); } let identity = NostrIdentity::new( @@ -677,7 +677,7 @@ impl AppCore { let operation = journal .begin_operation(kind, public_key, clock.now()) .await?; - if let Err(error) = secrets.put(public_key, secret) { + if let Err(error) = secrets.put(public_key, secret).await { let _ = journal.finalize_operation(operation).await; return Err(error); } @@ -771,7 +771,7 @@ async fn compensate_identity_write( identities.remove_identity(public_key).await }; let selection_rollback = app_state.save_selected_identity(previous_selection).await; - let credential_rollback = secrets.delete(public_key); + let credential_rollback = secrets.delete(public_key).await; if metadata_rollback.is_err() || selection_rollback.is_err() || credential_rollback.is_err() { let _ = journal .update_operation( @@ -1255,7 +1255,7 @@ mod tests { .expect("generate"); let public_key = receipt.identity().public_key(); assert_eq!(public_key.to_hex().len(), 64); - assert!(secrets.contains(public_key).expect("credential")); + assert!(secrets.contains(public_key).await.expect("credential")); assert_eq!( identities .load_selected_identity() @@ -1293,7 +1293,7 @@ mod tests { .await .expect("import"); let public_key = receipt.identity().public_key(); - assert!(secrets.contains(public_key).expect("credential")); + assert!(secrets.contains(public_key).await.expect("credential")); assert_eq!(core.snapshot().selected_identity(), Some(public_key)); assert_eq!(core.snapshot().session(), SessionState::SignedOut); assert!(!format!("{:?}", core.snapshot()).contains(input)); @@ -1423,7 +1423,7 @@ mod tests { .availability(), SignerAvailability::Available ); - assert!(secrets.contains(public_key).expect("credential")); + assert!(secrets.contains(public_key).await.expect("credential")); assert_eq!(core.snapshot().identities().len(), 1); } @@ -1656,7 +1656,7 @@ mod tests { .expect("remove"); assert_eq!(removed.identities().len(), 1); assert_eq!(removed.selected_identity(), Some(second)); - assert!(!secrets.contains(first).expect("credential removed")); + assert!(!secrets.contains(first).await.expect("credential removed")); assert_eq!(removed.session(), SessionState::SignedOut); } @@ -1710,7 +1710,10 @@ mod tests { .import(SecretKeyInput::parse(SECRET.to_owned()).expect("secret")) .expect("key material"); let (public_key, _npub, secret) = material.into_parts(); - secrets.put(public_key, secret).expect("orphan credential"); + secrets + .put(public_key, secret) + .await + .expect("orphan credential"); assert_eq!( core.import_secret_key( @@ -1935,7 +1938,7 @@ mod tests { .save_selected_identity(Some(public_key)) .await .expect("selection"); - secrets.put(public_key, secret).expect("credential"); + secrets.put(public_key, secret).await.expect("credential"); core.apply_transition(StateTransition::BootstrapRegistry { identities: vec![identity], selected: Some(public_key), diff --git a/core/crates/harvestcircle_application/src/recovery.rs b/core/crates/harvestcircle_application/src/recovery.rs @@ -90,8 +90,8 @@ async fn recover_durable_removal( let identity = operation.identity(); let mut phase = operation.phase(); if phase == DurableOperationPhase::IntentRecorded { - if secrets.contains(identity)? { - secrets.delete(identity)?; + if secrets.contains(identity).await? { + secrets.delete(identity).await?; } operations .advance_durable_operation( @@ -160,8 +160,8 @@ async fn recover_durable_addition( let identity = operation.identity(); match operation.phase() { DurableOperationPhase::IntentRecorded => { - if secrets.contains(identity)? { - secrets.delete(identity)?; + if secrets.contains(identity).await? { + secrets.delete(identity).await?; } operations .finalize_durable_operation( @@ -285,8 +285,8 @@ async fn compensate_durable_addition( operations: &(impl DurableOperationRepository + ?Sized), clock: &(impl Clock + ?Sized), ) -> Result<(), SafeError> { - if secrets.contains(operation.identity())? { - secrets.delete(operation.identity())?; + if secrets.contains(operation.identity()).await? { + secrets.delete(operation.identity()).await?; } if let Some(availability) = operation.prior().binding_availability() { if let Some(previous) = identities.find_identity(operation.identity()).await? { @@ -346,7 +346,7 @@ async fn recover_removal( ) -> Result<(), SafeError> { let public_key = operation.subject(); if operation.phase() == IdentityOperationPhase::IntentRecorded { - match secrets.delete(public_key) { + match secrets.delete(public_key).await { Ok(()) => {} Err(error) if error.code() == harvestcircle_domain::SafeErrorCode::CredentialMissing => {} @@ -401,7 +401,7 @@ async fn recover_addition( IdentityOperationPhase::CredentialWritten | IdentityOperationPhase::CompensationPending if !has_metadata => { - match secrets.delete(operation.subject()) { + match secrets.delete(operation.subject()).await { Ok(()) => {} Err(error) if error.code() == harvestcircle_domain::SafeErrorCode::CredentialMissing => {} @@ -671,7 +671,7 @@ pub(crate) mod tests { ] { let (core, identities, secrets, _journal, public_key) = seeded().await; if phase != DurableOperationPhase::IntentRecorded { - secrets.delete(public_key).expect("delete credential"); + secrets.delete(public_key).await.expect("delete credential"); } if matches!( phase, @@ -695,7 +695,7 @@ pub(crate) mod tests { } let (core, identities, secrets, _journal, public_key) = seeded().await; - secrets.delete(public_key).expect("delete credential"); + secrets.delete(public_key).await.expect("delete credential"); identities .remove_identity(public_key) .await @@ -757,7 +757,7 @@ pub(crate) mod tests { .expect("remove metadata"); } if !retain_secret { - secrets.delete(public_key).expect("delete credential"); + secrets.delete(public_key).await.expect("delete credential"); } let recovered = run_durable( &core, @@ -798,7 +798,7 @@ pub(crate) mod tests { .remove_identity(public_key) .await .expect("remove metadata"); - secrets.delete(public_key).expect("delete credential"); + secrets.delete(public_key).await.expect("delete credential"); let recovered = run_durable( &core, &identities, @@ -819,7 +819,7 @@ pub(crate) mod tests { for credential_present in [true, false] { let (core, identities, secrets, journal, public_key) = seeded().await; if !credential_present { - secrets.delete(public_key).expect("delete credential"); + secrets.delete(public_key).await.expect("delete credential"); identities .save_selected_identity(None) .await @@ -854,7 +854,7 @@ pub(crate) mod tests { .expect("remove metadata"); } if !credential_present { - secrets.delete(public_key).expect("delete credential"); + secrets.delete(public_key).await.expect("delete credential"); } let id = journal .begin_operation(kind, public_key, FixedClock.now()) @@ -895,6 +895,7 @@ pub(crate) mod tests { ) .expect("secret"), ) + .await .expect("store credential"); secrets.fail_next(SecretStoreOperation::Delete); let id = journal diff --git a/core/crates/harvestcircle_application/src/secrets.rs b/core/crates/harvestcircle_application/src/secrets.rs @@ -4,31 +4,37 @@ use std::sync::{Mutex, MutexGuard}; use harvestcircle_domain::{PublicKey, SafeError, SafeErrorCode, SafeMessage, SecretKeyInput}; use secrecy::{ExposeSecret, SecretString}; +use crate::BoxFuture; + pub trait SecretStore: Send + Sync { /// Stores a credential under its canonical public key without overwriting. /// /// # Errors /// /// Returns a safe duplicate or keyring error without exposing the credential. - fn put(&self, public_key: PublicKey, secret: SecretKeyInput) -> Result<(), SafeError>; + fn put( + &self, + public_key: PublicKey, + secret: SecretKeyInput, + ) -> BoxFuture<'_, Result<(), SafeError>>; /// Loads a credential into a non-cloneable redacted boundary value. /// /// # Errors /// /// Returns a safe missing-credential or keyring error. - fn load(&self, public_key: PublicKey) -> Result<SecretKeyInput, SafeError>; + fn load(&self, public_key: PublicKey) -> BoxFuture<'_, Result<SecretKeyInput, SafeError>>; /// Reports whether a credential exists without exposing it. /// /// # Errors /// /// Returns a safe keyring error when availability cannot be determined. - fn contains(&self, public_key: PublicKey) -> Result<bool, SafeError>; + fn contains(&self, public_key: PublicKey) -> BoxFuture<'_, Result<bool, SafeError>>; /// Deletes a credential without affecting public identity metadata. /// /// # Errors /// /// Returns a safe missing-credential or keyring error. - fn delete(&self, public_key: PublicKey) -> Result<(), SafeError>; + fn delete(&self, public_key: PublicKey) -> BoxFuture<'_, Result<(), SafeError>>; } #[derive(Default)] @@ -108,32 +114,44 @@ impl FailureSecretStore { } impl SecretStore for FailureSecretStore { - fn put(&self, public_key: PublicKey, secret: SecretKeyInput) -> Result<(), SafeError> { - if self.record_and_should_fail(SecretStoreOperation::Put, public_key) { - return Err(keyring_unavailable()); - } - self.inner.put(public_key, secret) + fn put( + &self, + public_key: PublicKey, + secret: SecretKeyInput, + ) -> BoxFuture<'_, Result<(), SafeError>> { + Box::pin(async move { + if self.record_and_should_fail(SecretStoreOperation::Put, public_key) { + return Err(keyring_unavailable()); + } + self.inner.put(public_key, secret).await + }) } - fn load(&self, public_key: PublicKey) -> Result<SecretKeyInput, SafeError> { - if self.record_and_should_fail(SecretStoreOperation::Load, public_key) { - return Err(keyring_unavailable()); - } - self.inner.load(public_key) + fn load(&self, public_key: PublicKey) -> BoxFuture<'_, Result<SecretKeyInput, SafeError>> { + Box::pin(async move { + if self.record_and_should_fail(SecretStoreOperation::Load, public_key) { + return Err(keyring_unavailable()); + } + self.inner.load(public_key).await + }) } - fn contains(&self, public_key: PublicKey) -> Result<bool, SafeError> { - if self.record_and_should_fail(SecretStoreOperation::Contains, public_key) { - return Err(keyring_unavailable()); - } - self.inner.contains(public_key) + fn contains(&self, public_key: PublicKey) -> BoxFuture<'_, Result<bool, SafeError>> { + Box::pin(async move { + if self.record_and_should_fail(SecretStoreOperation::Contains, public_key) { + return Err(keyring_unavailable()); + } + self.inner.contains(public_key).await + }) } - fn delete(&self, public_key: PublicKey) -> Result<(), SafeError> { - if self.record_and_should_fail(SecretStoreOperation::Delete, public_key) { - return Err(keyring_unavailable()); - } - self.inner.delete(public_key) + fn delete(&self, public_key: PublicKey) -> BoxFuture<'_, Result<(), SafeError>> { + Box::pin(async move { + if self.record_and_should_fail(SecretStoreOperation::Delete, public_key) { + return Err(keyring_unavailable()); + } + self.inner.delete(public_key).await + }) } } @@ -144,33 +162,44 @@ impl InMemorySecretStore { } impl SecretStore for InMemorySecretStore { - fn put(&self, public_key: PublicKey, secret: SecretKeyInput) -> Result<(), SafeError> { - let mut credentials = self.credentials()?; - if credentials.contains_key(&public_key) { - return Err(credential_exists()); - } - let value = secret.with_exposed_secret(ToOwned::to_owned); - credentials.insert(public_key, SecretString::from(value)); - Ok(()) + fn put( + &self, + public_key: PublicKey, + secret: SecretKeyInput, + ) -> BoxFuture<'_, Result<(), SafeError>> { + Box::pin(async move { + let mut credentials = self.credentials()?; + if credentials.contains_key(&public_key) { + return Err(credential_exists()); + } + let value = secret.with_exposed_secret(ToOwned::to_owned); + credentials.insert(public_key, SecretString::from(value)); + Ok(()) + }) } - fn load(&self, public_key: PublicKey) -> Result<SecretKeyInput, SafeError> { - let credentials = self.credentials()?; - let secret = credentials - .get(&public_key) - .ok_or_else(credential_missing)?; - SecretKeyInput::parse(secret.expose_secret().to_owned()).map_err(|_| credential_missing()) + fn load(&self, public_key: PublicKey) -> BoxFuture<'_, Result<SecretKeyInput, SafeError>> { + Box::pin(async move { + let credentials = self.credentials()?; + let secret = credentials + .get(&public_key) + .ok_or_else(credential_missing)?; + SecretKeyInput::parse(secret.expose_secret().to_owned()) + .map_err(|_| credential_missing()) + }) } - fn contains(&self, public_key: PublicKey) -> Result<bool, SafeError> { - Ok(self.credentials()?.contains_key(&public_key)) + fn contains(&self, public_key: PublicKey) -> BoxFuture<'_, Result<bool, SafeError>> { + Box::pin(async move { Ok(self.credentials()?.contains_key(&public_key)) }) } - fn delete(&self, public_key: PublicKey) -> Result<(), SafeError> { - self.credentials()? - .remove(&public_key) - .map(|_| ()) - .ok_or_else(credential_missing) + fn delete(&self, public_key: PublicKey) -> BoxFuture<'_, Result<(), SafeError>> { + Box::pin(async move { + self.credentials()? + .remove(&public_key) + .map(|_| ()) + .ok_or_else(credential_missing) + }) } } @@ -203,29 +232,30 @@ mod tests { const SECRET: &str = "7e7e9c42a91bfef19fa7ea99d52d8afdb67d893a8fefba1f5cb9793f2107f6d7"; - #[test] - fn secret_store_puts_loads_checks_and_deletes_redacted_credentials() { + #[tokio::test] + async fn secret_store_puts_loads_checks_and_deletes_redacted_credentials() { let store = InMemorySecretStore::default(); let public_key = PublicKey::from_bytes([7; 32]).expect("valid public key"); - assert!(!store.contains(public_key).expect("contains")); + assert!(!store.contains(public_key).await.expect("contains")); store .put( public_key, SecretKeyInput::parse(SECRET.to_owned()).expect("secret"), ) + .await .expect("put"); - assert!(store.contains(public_key).expect("contains")); - let loaded = store.load(public_key).expect("load"); + assert!(store.contains(public_key).await.expect("contains")); + let loaded = store.load(public_key).await.expect("load"); assert_eq!(loaded.with_exposed_secret(str::len), 64); - store.delete(public_key).expect("delete"); - assert!(!store.contains(public_key).expect("contains")); + store.delete(public_key).await.expect("delete"); + assert!(!store.contains(public_key).await.expect("contains")); } - #[test] - fn secret_store_rejects_duplicates_and_reports_missing_credentials() { + #[tokio::test] + async fn secret_store_rejects_duplicates_and_reports_missing_credentials() { let store = InMemorySecretStore::default(); let public_key = PublicKey::from_bytes([7; 32]).expect("valid public key"); - let Err(missing) = store.load(public_key) else { + let Err(missing) = store.load(public_key).await else { panic!("missing credential was returned"); }; assert_eq!(missing.code(), SafeErrorCode::CredentialMissing); @@ -234,21 +264,23 @@ mod tests { public_key, SecretKeyInput::parse(SECRET.to_owned()).expect("secret"), ) + .await .expect("put"); let duplicate = store .put( public_key, SecretKeyInput::parse(SECRET.to_owned()).expect("secret"), ) + .await .expect_err("duplicate"); assert_eq!(duplicate.code(), SafeErrorCode::IdentityAlreadyExists); - store.delete(public_key).expect("delete"); - let missing = store.delete(public_key).expect_err("missing delete"); + store.delete(public_key).await.expect("delete"); + let missing = store.delete(public_key).await.expect_err("missing delete"); assert_eq!(missing.code(), SafeErrorCode::CredentialMissing); } - #[test] - fn failure_secret_store_injects_each_boundary_without_mutating_state() { + #[tokio::test] + async fn failure_secret_store_injects_each_boundary_without_mutating_state() { let store = FailureSecretStore::default(); let public_key = PublicKey::from_bytes([7; 32]).expect("valid public key"); store.fail_next(SecretStoreOperation::Put); @@ -257,15 +289,17 @@ mod tests { public_key, SecretKeyInput::parse(SECRET.to_owned()).expect("secret"), ) + .await .expect_err("put failure"); assert_eq!(error.code(), SafeErrorCode::KeyringUnavailable); - assert!(!store.contains(public_key).expect("not written")); + assert!(!store.contains(public_key).await.expect("not written")); store .put( public_key, SecretKeyInput::parse(SECRET.to_owned()).expect("secret"), ) + .await .expect("put"); for operation in [ SecretStoreOperation::Load, @@ -274,19 +308,24 @@ mod tests { ] { store.fail_next(operation); let error = match operation { - SecretStoreOperation::Load => store.load(public_key).map(|_| ()), - SecretStoreOperation::Contains => store.contains(public_key).map(|_| ()), - SecretStoreOperation::Delete => store.delete(public_key), + SecretStoreOperation::Load => store.load(public_key).await.map(|_| ()), + SecretStoreOperation::Contains => store.contains(public_key).await.map(|_| ()), + SecretStoreOperation::Delete => store.delete(public_key).await, SecretStoreOperation::Put => unreachable!("put tested separately"), } .expect_err("injected failure"); assert_eq!(error.code(), SafeErrorCode::KeyringUnavailable); } - assert!(store.contains(public_key).expect("credential retained")); + assert!( + store + .contains(public_key) + .await + .expect("credential retained") + ); } - #[test] - fn failure_secret_store_call_log_contains_only_public_identity() { + #[tokio::test] + async fn failure_secret_store_call_log_contains_only_public_identity() { let store = FailureSecretStore::default(); let public_key = PublicKey::from_bytes([7; 32]).expect("valid public key"); store @@ -294,6 +333,7 @@ mod tests { public_key, SecretKeyInput::parse(SECRET.to_owned()).expect("secret"), ) + .await .expect("put"); let calls = store.calls(); assert_eq!(calls[0].operation(), SecretStoreOperation::Put); diff --git a/core/crates/harvestcircle_application/src/session.rs b/core/crates/harvestcircle_application/src/session.rs @@ -38,7 +38,7 @@ impl AppCore { .ok_or_else(identity_not_found)?; self.apply_transition(StateTransition::BeginActivation(public_key))?; let prepared = async { - let credential = secrets.load(public_key)?; + let credential = secrets.load(public_key).await?; let imported = self.key_material().import(credential)?; let (derived_public_key, _npub, canonical_secret) = imported.into_parts(); drop(canonical_secret); @@ -202,7 +202,10 @@ mod tests { assert_eq!(registered, active.identity()); assert_eq!(registered.last_used_at(), Some(FixedClock.now())); - secrets.delete(second).expect("remove second credential"); + secrets + .delete(second) + .await + .expect("remove second credential"); let error = core .activate_identity( second, @@ -223,6 +226,7 @@ mod tests { second, input("7e7e9c42a91bfef19fa7ea99d52d8afdb67d893a8fefba1f5cb9793f2107f6d7"), ) + .await .expect("mismatched credential"); let invalid = core .activate_identity( @@ -287,6 +291,11 @@ mod tests { assert!(signed_out.active_identity().is_none()); assert_eq!(signed_out.identities().len(), 1); assert_eq!(signed_out.selected_identity(), Some(public_key)); - assert!(secrets.contains(public_key).expect("credential retained")); + assert!( + secrets + .contains(public_key) + .await + .expect("credential retained") + ); } } diff --git a/core/crates/harvestcircle_ffi/src/keyring_worker.rs b/core/crates/harvestcircle_ffi/src/keyring_worker.rs @@ -1,9 +1,9 @@ use std::sync::{Arc, Mutex}; use std::thread::JoinHandle; -use harvestcircle_application::SecretStore; +use harvestcircle_application::{BoxFuture, SecretStore}; use harvestcircle_domain::{PublicKey, SafeError, SafeErrorCode, SafeMessage, SecretKeyInput}; -use tokio::sync::watch; +use tokio::sync::{oneshot, watch}; const KEYRING_QUEUE_CAPACITY: usize = 8; @@ -11,20 +11,14 @@ enum Request { Put( PublicKey, SecretKeyInput, - std::sync::mpsc::SyncSender<Result<(), SafeError>>, + oneshot::Sender<Result<(), SafeError>>, ), Load( PublicKey, - std::sync::mpsc::SyncSender<Result<SecretKeyInput, SafeError>>, - ), - Contains( - PublicKey, - std::sync::mpsc::SyncSender<Result<bool, SafeError>>, - ), - Delete( - PublicKey, - std::sync::mpsc::SyncSender<Result<(), SafeError>>, + oneshot::Sender<Result<SecretKeyInput, SafeError>>, ), + Contains(PublicKey, oneshot::Sender<Result<bool, SafeError>>), + Delete(PublicKey, oneshot::Sender<Result<(), SafeError>>), Close, } @@ -38,22 +32,31 @@ impl BoundedKeyringWorker { pub(crate) fn new(store: impl SecretStore + 'static) -> Result<Arc<Self>, SafeError> { let (sender, receiver) = std::sync::mpsc::sync_channel(KEYRING_QUEUE_CAPACITY); let (completion_sender, completion_receiver) = watch::channel(false); + let runtime = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .map_err(|_| worker_unavailable())?; let thread = std::thread::Builder::new() .name("harvestcircle-keyring-worker".to_owned()) .spawn(move || { while let Ok(request) = receiver.recv() { match request { Request::Put(public_key, secret, response) => { - let _ = response.send(store.put(public_key, secret)); + let _ = response.send( + runtime.block_on(async { store.put(public_key, secret).await }), + ); } Request::Load(public_key, response) => { - let _ = response.send(store.load(public_key)); + let _ = response + .send(runtime.block_on(async { store.load(public_key).await })); } Request::Contains(public_key, response) => { - let _ = response.send(store.contains(public_key)); + let _ = response + .send(runtime.block_on(async { store.contains(public_key).await })); } Request::Delete(public_key, response) => { - let _ = response.send(store.delete(public_key)); + let _ = response + .send(runtime.block_on(async { store.delete(public_key).await })); } Request::Close => break, } @@ -68,20 +71,21 @@ impl BoundedKeyringWorker { })) } - fn submit<T>( + async fn submit<T>( &self, - request: impl FnOnce(std::sync::mpsc::SyncSender<T>) -> Request, + request: impl FnOnce(oneshot::Sender<T>) -> Request, ) -> Result<T, SafeError> { - let (response_sender, response_receiver) = std::sync::mpsc::sync_channel(1); - let sender_guard = self.sender.lock().map_err(|_| worker_unavailable())?; - let Some(sender) = sender_guard.as_ref() else { - return Err(worker_unavailable()); - }; - sender - .try_send(request(response_sender)) - .map_err(|_| worker_unavailable())?; - drop(sender_guard); - response_receiver.recv().map_err(|_| worker_unavailable()) + let (response_sender, response_receiver) = oneshot::channel(); + { + let sender_guard = self.sender.lock().map_err(|_| worker_unavailable())?; + let Some(sender) = sender_guard.as_ref() else { + return Err(worker_unavailable()); + }; + sender + .try_send(request(response_sender)) + .map_err(|_| worker_unavailable())?; + } + response_receiver.await.map_err(|_| worker_unavailable()) } pub(crate) async fn close(&self) -> Result<(), SafeError> { @@ -118,20 +122,36 @@ fn signal_close(sender: std::sync::mpsc::SyncSender<Request>) { } impl SecretStore for BoundedKeyringWorker { - fn put(&self, public_key: PublicKey, secret: SecretKeyInput) -> Result<(), SafeError> { - self.submit(|response| Request::Put(public_key, secret, response))? + fn put( + &self, + public_key: PublicKey, + secret: SecretKeyInput, + ) -> BoxFuture<'_, Result<(), SafeError>> { + Box::pin(async move { + self.submit(|response| Request::Put(public_key, secret, response)) + .await? + }) } - fn load(&self, public_key: PublicKey) -> Result<SecretKeyInput, SafeError> { - self.submit(|response| Request::Load(public_key, response))? + fn load(&self, public_key: PublicKey) -> BoxFuture<'_, Result<SecretKeyInput, SafeError>> { + Box::pin(async move { + self.submit(|response| Request::Load(public_key, response)) + .await? + }) } - fn contains(&self, public_key: PublicKey) -> Result<bool, SafeError> { - self.submit(|response| Request::Contains(public_key, response))? + fn contains(&self, public_key: PublicKey) -> BoxFuture<'_, Result<bool, SafeError>> { + Box::pin(async move { + self.submit(|response| Request::Contains(public_key, response)) + .await? + }) } - fn delete(&self, public_key: PublicKey) -> Result<(), SafeError> { - self.submit(|response| Request::Delete(public_key, response))? + fn delete(&self, public_key: PublicKey) -> BoxFuture<'_, Result<(), SafeError>> { + Box::pin(async move { + self.submit(|response| Request::Delete(public_key, response)) + .await? + }) } } @@ -154,8 +174,11 @@ const fn worker_unavailable() -> SafeError { #[cfg(test)] mod tests { - use harvestcircle_application::{InMemorySecretStore, SecretStore}; - use harvestcircle_domain::{PublicKey, SecretKeyInput}; + use std::time::Duration; + + use harvestcircle_application::{BoxFuture, InMemorySecretStore, SecretStore}; + use harvestcircle_domain::{PublicKey, SafeError, SecretKeyInput}; + use tokio::sync::oneshot; use super::{BoundedKeyringWorker, Request, signal_close}; @@ -164,6 +187,35 @@ mod tests { .expect("public key") } + struct SlowContainsStore { + inner: InMemorySecretStore, + } + + impl SecretStore for SlowContainsStore { + fn put( + &self, + public_key: PublicKey, + secret: SecretKeyInput, + ) -> BoxFuture<'_, Result<(), SafeError>> { + self.inner.put(public_key, secret) + } + + fn load(&self, public_key: PublicKey) -> BoxFuture<'_, Result<SecretKeyInput, SafeError>> { + self.inner.load(public_key) + } + + fn contains(&self, public_key: PublicKey) -> BoxFuture<'_, Result<bool, SafeError>> { + Box::pin(async move { + std::thread::sleep(Duration::from_millis(50)); + self.inner.contains(public_key).await + }) + } + + fn delete(&self, public_key: PublicKey) -> BoxFuture<'_, Result<(), SafeError>> { + self.inner.delete(public_key) + } + } + #[tokio::test] async fn worker_round_trips_without_exposing_secret_material() { let worker = BoundedKeyringWorker::new(InMemorySecretStore::default()).expect("worker"); @@ -171,19 +223,38 @@ mod tests { "0000000000000000000000000000000000000000000000000000000000000001".to_owned(), ) .expect("secret"); - worker.put(public_key(), secret).expect("put"); - assert!(worker.contains(public_key()).expect("contains")); - let loaded = worker.load(public_key()).expect("load"); + worker.put(public_key(), secret).await.expect("put"); + assert!(worker.contains(public_key()).await.expect("contains")); + let loaded = worker.load(public_key()).await.expect("load"); assert_eq!(loaded.with_exposed_secret(str::len), 64); - worker.delete(public_key()).expect("delete"); + worker.delete(public_key()).await.expect("delete"); + worker.close().await.expect("close"); + assert!(worker.contains(public_key()).await.is_err()); + } + + #[tokio::test(flavor = "current_thread")] + async fn response_waiting_never_blocks_the_tokio_runtime_thread() { + let worker = BoundedKeyringWorker::new(SlowContainsStore { + inner: InMemorySecretStore::default(), + }) + .expect("worker"); + let response = worker.contains(public_key()); + tokio::pin!(response); + + tokio::select! { + biased; + result = &mut response => panic!("slow keyring response completed before runtime progress: {result:?}"), + () = tokio::task::yield_now() => {} + } + + assert!(!response.await.expect("contains")); worker.close().await.expect("close"); - assert!(worker.contains(public_key()).is_err()); } #[test] fn close_signal_never_blocks_on_a_full_bounded_queue() { let (sender, receiver) = std::sync::mpsc::sync_channel(1); - let (response, _response_receiver) = std::sync::mpsc::sync_channel(1); + let (response, _response_receiver) = oneshot::channel(); assert!( sender .try_send(Request::Contains(public_key(), response)) diff --git a/core/crates/harvestcircle_runtime/src/runtime_actor.rs b/core/crates/harvestcircle_runtime/src/runtime_actor.rs @@ -1478,8 +1478,8 @@ const fn observer_registration_failed() -> SafeError { mod tests { use std::future::Future; use std::num::NonZeroUsize; + use std::sync::Arc; use std::sync::atomic::{AtomicBool, Ordering}; - use std::sync::{Arc, Condvar, Mutex}; use std::task::{Context, Poll, Wake, Waker}; use std::thread::{self, Thread}; use std::time::{Duration, Instant}; @@ -1576,8 +1576,8 @@ mod tests { inner: InMemorySecretStore, block_next_put: AtomicBool, put_started: AtomicBool, - released: Mutex<bool>, - release_signal: Condvar, + released: AtomicBool, + release_signal: tokio::sync::Notify, } impl BlockingSecretStore { @@ -1586,8 +1586,8 @@ mod tests { inner: InMemorySecretStore::default(), block_next_put: AtomicBool::new(true), put_started: AtomicBool::new(false), - released: Mutex::new(false), - release_signal: Condvar::new(), + released: AtomicBool::new(false), + release_signal: tokio::sync::Notify::new(), } } @@ -1598,40 +1598,41 @@ mod tests { } fn release(&self) { - *self - .released - .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner) = true; - self.release_signal.notify_all(); + self.released.store(true, Ordering::Release); + self.release_signal.notify_waiters(); } } impl SecretStore for BlockingSecretStore { - fn put(&self, public_key: PublicKey, secret: SecretKeyInput) -> Result<(), SafeError> { - if self.block_next_put.swap(false, Ordering::AcqRel) { - self.put_started.store(true, Ordering::Release); - let released = self - .released - .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner); - drop( - self.release_signal - .wait_while(released, |released| !*released) - .unwrap_or_else(std::sync::PoisonError::into_inner), - ); - } - self.inner.put(public_key, secret) + fn put( + &self, + public_key: PublicKey, + secret: SecretKeyInput, + ) -> BoxFuture<'_, Result<(), SafeError>> { + Box::pin(async move { + if self.block_next_put.swap(false, Ordering::AcqRel) { + self.put_started.store(true, Ordering::Release); + loop { + let notified = self.release_signal.notified(); + if self.released.load(Ordering::Acquire) { + break; + } + notified.await; + } + } + self.inner.put(public_key, secret).await + }) } - fn load(&self, public_key: PublicKey) -> Result<SecretKeyInput, SafeError> { + fn load(&self, public_key: PublicKey) -> BoxFuture<'_, Result<SecretKeyInput, SafeError>> { self.inner.load(public_key) } - fn contains(&self, public_key: PublicKey) -> Result<bool, SafeError> { + fn contains(&self, public_key: PublicKey) -> BoxFuture<'_, Result<bool, SafeError>> { self.inner.contains(public_key) } - fn delete(&self, public_key: PublicKey) -> Result<(), SafeError> { + fn delete(&self, public_key: PublicKey) -> BoxFuture<'_, Result<(), SafeError>> { self.inner.delete(public_key) } } @@ -1758,7 +1759,7 @@ mod tests { assert_eq!(foreground.identity().public_key(), public_key); assert_eq!(foreground.signer_binding().identity(), public_key); assert_eq!(foreground.generation(), actor.session_generation()); - assert!(secrets.contains(public_key).expect("credential")); + assert!(secrets.contains(public_key).await.expect("credential")); let signed_out = actor.sign_out().await.expect("sign out"); assert_eq!(signed_out.session(), SessionState::SignedOut); @@ -1772,7 +1773,12 @@ mod tests { .await .expect("remove"); assert!(removed.identities().is_empty()); - assert!(!secrets.contains(public_key).expect("credential removed")); + assert!( + !secrets + .contains(public_key) + .await + .expect("credential removed") + ); } #[tokio::test(flavor = "multi_thread")] @@ -1869,6 +1875,7 @@ mod tests { assert!( !secrets .contains(stage.view().identity().public_key()) + .await .expect("keyring") ); assert!(actor.sign_out().await.is_err()); @@ -1903,7 +1910,7 @@ mod tests { assert_eq!(recovery.with_exposed_secret(str::len), 63); assert!(handle.take_recovery_nsec().is_err()); assert_eq!(actor.snapshot(), initial); - assert!(!secrets.contains(public_key).expect("not committed")); + assert!(!secrets.contains(public_key).await.expect("not committed")); let committed = actor .acknowledge_generated_key_stage_test(handle.id()) @@ -1911,7 +1918,12 @@ mod tests { .expect("acknowledge"); assert_eq!(committed.identities().len(), 1); assert_eq!(committed.selected_identity(), Some(public_key)); - assert!(secrets.contains(public_key).expect("credential committed")); + assert!( + secrets + .contains(public_key) + .await + .expect("credential committed") + ); assert!( actor .acknowledge_generated_key_stage_test(handle.id()) diff --git a/core/crates/harvestcircle_runtime/tests/local_relay_e2e.rs b/core/crates/harvestcircle_runtime/tests/local_relay_e2e.rs @@ -112,7 +112,12 @@ async fn local_relay_e2e_imports_activates_refreshes_and_caches_profile() { .await .expect("import identity"); let public_key = imported.identity().public_key(); - assert!(secrets.contains(public_key).expect("credential exists")); + assert!( + secrets + .contains(public_key) + .await + .expect("credential exists") + ); adapter .activate_identity(public_key, &secrets, &FixedClock) .await diff --git a/core/crates/harvestcircle_storage/src/os_keyring.rs b/core/crates/harvestcircle_storage/src/os_keyring.rs @@ -1,6 +1,6 @@ use std::sync::{Mutex, MutexGuard}; -use harvestcircle_application::SecretStore; +use harvestcircle_application::{BoxFuture, SecretStore}; use harvestcircle_domain::{PublicKey, SafeError, SafeErrorCode, SafeMessage, SecretKeyInput}; use harvestcircle_product::KEYRING_SERVICE; use keyring::{Entry, Error as KeyringError}; @@ -26,47 +26,59 @@ impl OsKeyringSecretStore { } impl SecretStore for OsKeyringSecretStore { - fn put(&self, public_key: PublicKey, secret: SecretKeyInput) -> Result<(), SafeError> { - let _operation = self.operation()?; - let entry = Self::entry(public_key)?; - match entry.get_password() { - Ok(password) => { - drop(Zeroizing::new(password)); - return Err(credential_exists()); + fn put( + &self, + public_key: PublicKey, + secret: SecretKeyInput, + ) -> BoxFuture<'_, Result<(), SafeError>> { + Box::pin(async move { + let _operation = self.operation()?; + let entry = Self::entry(public_key)?; + match entry.get_password() { + Ok(password) => { + drop(Zeroizing::new(password)); + return Err(credential_exists()); + } + Err(KeyringError::NoEntry) => {} + Err(_) => return Err(keyring_unavailable()), } - Err(KeyringError::NoEntry) => {} - Err(_) => return Err(keyring_unavailable()), - } - secret - .with_exposed_secret(|value| entry.set_password(value)) - .map_err(|_| keyring_unavailable()) + secret + .with_exposed_secret(|value| entry.set_password(value)) + .map_err(|_| keyring_unavailable()) + }) } - fn load(&self, public_key: PublicKey) -> Result<SecretKeyInput, SafeError> { - let _operation = self.operation()?; - let password = Self::entry(public_key)? - .get_password() - .map_err(|error| map_read_error(&error))?; - SecretKeyInput::parse(password) + fn load(&self, public_key: PublicKey) -> BoxFuture<'_, Result<SecretKeyInput, SafeError>> { + Box::pin(async move { + let _operation = self.operation()?; + let password = Self::entry(public_key)? + .get_password() + .map_err(|error| map_read_error(&error))?; + SecretKeyInput::parse(password) + }) } - fn contains(&self, public_key: PublicKey) -> Result<bool, SafeError> { - let _operation = self.operation()?; - match Self::entry(public_key)?.get_password() { - Ok(password) => { - drop(Zeroizing::new(password)); - Ok(true) + fn contains(&self, public_key: PublicKey) -> BoxFuture<'_, Result<bool, SafeError>> { + Box::pin(async move { + let _operation = self.operation()?; + match Self::entry(public_key)?.get_password() { + Ok(password) => { + drop(Zeroizing::new(password)); + Ok(true) + } + Err(KeyringError::NoEntry) => Ok(false), + Err(_) => Err(keyring_unavailable()), } - Err(KeyringError::NoEntry) => Ok(false), - Err(_) => Err(keyring_unavailable()), - } + }) } - fn delete(&self, public_key: PublicKey) -> Result<(), SafeError> { - let _operation = self.operation()?; - Self::entry(public_key)? - .delete_credential() - .map_err(|error| map_read_error(&error)) + fn delete(&self, public_key: PublicKey) -> BoxFuture<'_, Result<(), SafeError>> { + Box::pin(async move { + let _operation = self.operation()?; + Self::entry(public_key)? + .delete_credential() + .map_err(|error| map_read_error(&error)) + }) } } @@ -117,8 +129,8 @@ mod tests { ); } - #[test] - fn poisoned_operation_lock_fails_closed_before_keyring_access() { + #[tokio::test] + async fn poisoned_operation_lock_fails_closed_before_keyring_access() { let store = OsKeyringSecretStore::default(); let panic = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { let _operation = store.operation_lock.lock().expect("operation lock"); @@ -129,27 +141,31 @@ mod tests { let public_key = PublicKey::from_hex("7e7e9c42a91bfef19fa7ea99d52d8afdb67d893a8fefba1f5cb9793f2107f6d7") .expect("valid public key"); - let error = store.contains(public_key).expect_err("poison must reject"); + let error = store + .contains(public_key) + .await + .expect_err("poison must reject"); assert_eq!(error.code(), SafeErrorCode::KeyringUnavailable); } - #[test] + #[tokio::test] #[ignore = "mutates the current user's operating-system credential store"] - fn real_keyring_smoke_round_trips_and_deletes() { + async fn real_keyring_smoke_round_trips_and_deletes() { let store = OsKeyringSecretStore::default(); let public_key = PublicKey::from_hex("7e7e9c42a91bfef19fa7ea99d52d8afdb67d893a8fefba1f5cb9793f2107f6d7") .expect("valid public key"); - let _ = store.delete(public_key); + let _ = store.delete(public_key).await; store .put( public_key, SecretKeyInput::parse("11".repeat(32)).expect("secret"), ) + .await .expect("keyring put"); - assert!(store.contains(public_key).expect("keyring contains")); - let loaded = store.load(public_key).expect("keyring load"); + assert!(store.contains(public_key).await.expect("keyring contains")); + let loaded = store.load(public_key).await.expect("keyring load"); assert_eq!(loaded.with_exposed_secret(str::len), 64); - store.delete(public_key).expect("keyring delete"); + store.delete(public_key).await.expect("keyring delete"); } } diff --git a/core/crates/harvestcircle_storage/tests/package_boundary.rs b/core/crates/harvestcircle_storage/tests/package_boundary.rs @@ -61,6 +61,8 @@ fn storage_package_keeps_one_sqlite_authority_and_a_sealed_public_surface() { "pub fn harvestcircle_storage::verify_harvestcircle_backup", "impl harvestcircle_application::ports::DurableOperationRepository for harvestcircle_storage::Database", "harvestcircle_application::ports::BoxFuture", + "pub fn harvestcircle_storage::OsKeyringSecretStore::contains(&self, harvestcircle_domain::key::PublicKey) -> harvestcircle_application::ports::BoxFuture", + "pub fn harvestcircle_storage::OsKeyringSecretStore::put(&self, harvestcircle_domain::key::PublicKey, harvestcircle_domain::key::SecretKeyInput) -> harvestcircle_application::ports::BoxFuture", "pub fn harvestcircle_storage::harvestcircle_migration_catalog()", "pub fn harvestcircle_storage::harvestcircle_schema_catalog()", ] { diff --git a/core/crates/harvestcircle_test_bridge/src/lib.rs b/core/crates/harvestcircle_test_bridge/src/lib.rs @@ -326,8 +326,8 @@ impl HarvestCircleTestBridge { .snapshot() .selected_identity() .ok_or_else(request_unavailable)?; - let secret = self.secrets.load(selected)?; self.runtime.block_on(async { + let secret = self.secrets.load(selected).await?; let keys = secret .with_exposed_secret(Keys::parse) .map_err(|_| invalid_secret())?; diff --git a/tools/xtask/src/lib.rs b/tools/xtask/src/lib.rs @@ -315,6 +315,16 @@ fn native_runtime_boundary(root: &Path, findings: &mut Vec<String>) { "pub(crate) struct BoundedKeyringWorker", "keyring worker", ), + ( + &keyring, + "use tokio::sync::{oneshot, watch}", + "keyring worker", + ), + ( + &keyring, + "response_receiver.await", + "keyring worker", + ), ] { if !source.contains(required) { findings.push(format!( @@ -322,6 +332,11 @@ fn native_runtime_boundary(root: &Path, findings: &mut Vec<String>) { )); } } + if keyring.contains("response_receiver.recv") { + findings.push( + "harvestcircle_ffi: keyring response blocks a Tokio runtime thread".to_owned(), + ); + } } fn namespace_audit(root: &Path, inventory: &Inventory, findings: &mut Vec<String>) {