From 604e3fe4ba47238baf80278cd1fe9a123efdb5cb Mon Sep 17 00:00:00 2001 From: Henry Park Date: Sun, 26 Jul 2026 14:55:26 -0700 Subject: [PATCH 01/14] feat(secrets): stable placeholder registry + JIT sessions The sandbox container must never see real secret material, even transiently. It now holds only an inert `icsbx_`-prefixed placeholder token, stable per (tenant, user, provider) and separate from the session store, so holding one grants nothing on its own. InMemoryCredentialBroker gains JIT minting (mint_on_first_use): a CredentialSession is minted only at actual first use of an (invocation, binding) pair, not staged up front, and bound to the placeholder so the egress proxy can find it. CredentialSessionLease guarantees the session is revoked exactly once regardless of how the dispatch ends -- success/error via explicit revoke(), timeout/panic via Drop during future-cancellation or unwind -- because a missed revoke path is a silent standing grant. W6 (egress proxy consumer) and W8 (obligation chokepoint) are not built yet; this lands the placeholder/session primitives unwired, per plan, for them to call into later. Co-Authored-By: Claude Opus 5 (1M context) --- crates/ironclaw_secrets/Cargo.toml | 2 +- crates/ironclaw_secrets/src/lib.rs | 17 + crates/ironclaw_secrets/src/placeholder.rs | 748 +++++++++++++++++++++ 3 files changed, 766 insertions(+), 1 deletion(-) create mode 100644 crates/ironclaw_secrets/src/placeholder.rs diff --git a/crates/ironclaw_secrets/Cargo.toml b/crates/ironclaw_secrets/Cargo.toml index 8b378e9ac08..82d7a8abb08 100644 --- a/crates/ironclaw_secrets/Cargo.toml +++ b/crates/ironclaw_secrets/Cargo.toml @@ -21,7 +21,7 @@ serde_json = "1" sha2 = "0.11" subtle = "2" thiserror = "2" -tokio = { version = "1", features = ["macros", "rt", "sync"] } +tokio = { version = "1", features = ["macros", "rt", "sync", "time"] } tracing = "0.1" url = "2" uuid = { version = "1", features = ["v4", "serde"] } diff --git a/crates/ironclaw_secrets/src/lib.rs b/crates/ironclaw_secrets/src/lib.rs index 9cadc24ce80..0535e62c087 100644 --- a/crates/ironclaw_secrets/src/lib.rs +++ b/crates/ironclaw_secrets/src/lib.rs @@ -11,8 +11,13 @@ mod crypto; pub mod keychain; mod legacy_store; +mod placeholder; mod secret_store; +pub use placeholder::{ + CREDENTIAL_PLACEHOLDER_PREFIX, CredentialPlaceholderOwner, CredentialPlaceholderRegistry, + CredentialPlaceholderToken, CredentialSessionLease, +}; pub use secret_store::{CredentialBroker, SecretStore}; use std::collections::HashMap; @@ -465,6 +470,8 @@ pub enum CredentialBrokerError { CredentialExtensionMismatch { account_id: CredentialAccountId }, #[error("credential account {account_id} is not allowed for requested target")] CredentialPolicyMismatch { account_id: CredentialAccountId }, + #[error("credential placeholder token {value} is invalid: {reason}")] + InvalidPlaceholderToken { value: String, reason: String }, } impl CredentialBrokerError { @@ -482,6 +489,7 @@ impl CredentialBrokerError { Self::CredentialRevoked { .. } => "CredentialRevoked", Self::CredentialExtensionMismatch { .. } => "CredentialPolicyMismatch", Self::CredentialPolicyMismatch { .. } => "CredentialPolicyMismatch", + Self::InvalidPlaceholderToken { .. } => "MissingCredential", } } @@ -561,6 +569,15 @@ pub trait CredentialSessionStore: Send + Sync { pub struct InMemoryCredentialBroker { accounts: Mutex>, sessions: Mutex>, + /// Secondary index for JIT minting: `(invocation, capability, account)` -> + /// the session already minted for it, so re-use within the same dispatch + /// does not mint a second session. See [`placeholder::JitMintKey`] and + /// [`InMemoryCredentialBroker::mint_on_first_use`]. + jit_minted: Mutex>, + /// Secondary index the egress proxy (W6) will use: placeholder token -> + /// the session currently bound to it. See + /// [`InMemoryCredentialBroker::find_session_by_placeholder`]. + sessions_by_placeholder: Mutex>, } #[derive(Debug, Clone)] diff --git a/crates/ironclaw_secrets/src/placeholder.rs b/crates/ironclaw_secrets/src/placeholder.rs new file mode 100644 index 00000000000..cfbbac27e3d --- /dev/null +++ b/crates/ironclaw_secrets/src/placeholder.rs @@ -0,0 +1,748 @@ +//! Stable credential placeholder registry and JIT session wiring. +//! +//! The container running a sandboxed invocation never sees real secret +//! material. Instead it is handed a **placeholder token** — an inert string +//! that identifies "the credential for this tenant/user/provider" without +//! granting anything on its own. The egress proxy (W6, not built yet) swaps +//! the placeholder for a live [`CredentialSession`] at request time, host +//! side only. +//! +//! This module owns two host-side responsibilities: +//! +//! 1. [`CredentialPlaceholderRegistry`] — mints and remembers one stable +//! placeholder per `(tenant, user, provider)`, and resolves a placeholder +//! back to its owner. The registry never grants access; it is pure +//! identity bookkeeping, kept deliberately separate from +//! [`InMemoryCredentialBroker`]'s session store so that holding a +//! placeholder can never be conflated with holding a session. +//! 2. [`CredentialSessionLease`] — the JIT (just-in-time) minting handle +//! returned by [`InMemoryCredentialBroker::mint_on_first_use`]. A session +//! is minted only when a binding is actually used, and the lease +//! guarantees the session is revoked when the lease is dropped — success, +//! error, timeout (future dropped by `tokio::time::timeout`), or panic +//! (drop still runs during unwind) — so a missed revoke path can never +//! leave a standing grant. +use std::collections::HashMap; +use std::fmt; +use std::sync::{Arc, Mutex}; + +use ironclaw_host_api::{CapabilityId, ExtensionId, InvocationId, TenantId, UserId}; +use uuid::Uuid; + +use crate::{ + CredentialAccountId, CredentialBrokerError, CredentialSessionId, CredentialSessionRequest, + InMemoryCredentialBroker, +}; + +/// Fixed prefix for every placeholder token. +/// +/// A companion leak-detector pattern (owned elsewhere, see the sandbox +/// credential firewall design doc) recognizes this prefix so a placeholder +/// that somehow escapes the container is flagged the same way a real secret +/// would be — even though, unlike a real secret, holding one grants nothing. +pub const CREDENTIAL_PLACEHOLDER_PREFIX: &str = "icsbx_"; + +/// Opaque, stable placeholder token for a `(tenant, user, provider)` triple. +/// +/// Deliberately **not** bearer-like: unlike [`CredentialSessionId`], a +/// placeholder is inert on its own (`Display` shows it in full, and that is +/// intentional — it is designed to sit in a container's environment/config +/// where a human or log line may see it, because it can't be used to reach a +/// real credential without a live session behind it). +#[derive(Clone, PartialEq, Eq, Hash)] +pub struct CredentialPlaceholderToken(String); + +impl CredentialPlaceholderToken { + fn generate() -> Self { + Self(format!( + "{CREDENTIAL_PLACEHOLDER_PREFIX}{}", + Uuid::new_v4().simple() + )) + } + + /// Parses a placeholder token received from the sandbox side (e.g. off an + /// outbound request), rejecting anything that does not carry the fixed + /// prefix. + pub fn parse(value: impl Into) -> Result { + let value = value.into(); + if !value.starts_with(CREDENTIAL_PLACEHOLDER_PREFIX) { + return Err(CredentialBrokerError::InvalidPlaceholderToken { + value, + reason: format!("must start with '{CREDENTIAL_PLACEHOLDER_PREFIX}'"), + }); + } + Ok(Self(value)) + } + + pub fn as_str(&self) -> &str { + &self.0 + } +} + +impl fmt::Debug for CredentialPlaceholderToken { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter + .debug_tuple("CredentialPlaceholderToken") + .field(&self.0) + .finish() + } +} + +impl fmt::Display for CredentialPlaceholderToken { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter.write_str(&self.0) + } +} + +/// The `(tenant, user, provider)` triple a placeholder token identifies. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct CredentialPlaceholderOwner { + pub tenant_id: TenantId, + pub user_id: UserId, + pub provider_id: ExtensionId, +} + +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +struct CredentialPlaceholderOwnerKey { + tenant_id: TenantId, + user_id: UserId, + provider_id: ExtensionId, +} + +/// Host-side registry mapping `(tenant, user, provider)` to a stable +/// placeholder token, and back. +/// +/// Stability is structural, not incidental: a container recycle/removal +/// touches nothing here (the registry lives at process/host lifetime, not +/// container lifetime), so the same triple always yields the same token. +/// Holding a registry-issued token grants nothing — see +/// [`InMemoryCredentialBroker::mint_on_first_use`] for the piece that does. +#[derive(Debug, Default)] +pub struct CredentialPlaceholderRegistry { + by_owner: Mutex>, + by_token: Mutex>, +} + +impl CredentialPlaceholderRegistry { + pub fn new() -> Self { + Self::default() + } + + /// Returns the stable placeholder for `(tenant, user, provider)`, seeding + /// one on first request. Idempotent: repeated calls for the same triple + /// — including calls made after a simulated container recycle, since + /// nothing here is tied to a container's lifetime — return the same + /// token. + pub fn get_or_create( + &self, + tenant_id: &TenantId, + user_id: &UserId, + provider_id: &ExtensionId, + ) -> Result { + let key = CredentialPlaceholderOwnerKey { + tenant_id: tenant_id.clone(), + user_id: user_id.clone(), + provider_id: provider_id.clone(), + }; + let mut by_owner = + self.by_owner + .lock() + .map_err(|error| CredentialBrokerError::BrokerUnavailable { + reason: error.to_string(), + })?; + if let Some(existing) = by_owner.get(&key) { + return Ok(existing.clone()); + } + let token = CredentialPlaceholderToken::generate(); + by_owner.insert(key, token.clone()); + drop(by_owner); + self.by_token + .lock() + .map_err(|error| CredentialBrokerError::BrokerUnavailable { + reason: error.to_string(), + })? + .insert( + token.clone(), + CredentialPlaceholderOwner { + tenant_id: tenant_id.clone(), + user_id: user_id.clone(), + provider_id: provider_id.clone(), + }, + ); + Ok(token) + } + + /// Reverse lookup used by the egress proxy: given a placeholder token, + /// find the `(tenant, user, provider)` it identifies. + /// + /// Backed by an exact-match `HashMap` (no prefix/substring scan), so this + /// is O(1) and structurally cannot cross-match another user's token. + pub fn resolve( + &self, + token: &CredentialPlaceholderToken, + ) -> Result, CredentialBrokerError> { + Ok(self + .by_token + .lock() + .map_err(|error| CredentialBrokerError::BrokerUnavailable { + reason: error.to_string(), + })? + .get(token) + .cloned()) + } +} + +/// Identity of a `(invocation, binding)` pair used to key JIT session +/// minting, so re-use within the same dispatch does not mint a second +/// session for the same binding. +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub(crate) struct JitMintKey { + invocation_id: InvocationId, + capability_id: CapabilityId, + account_id: CredentialAccountId, +} + +/// RAII handle for a JIT-minted [`CredentialSession`](crate::CredentialSession). +/// +/// The session behind this lease is revoked exactly once, no matter how the +/// lease is dropped: +/// - **success** / **error**: call [`CredentialSessionLease::revoke`] explicitly. +/// - **timeout**: if the future holding the lease is dropped by +/// `tokio::time::timeout` (or any other cancellation), `Drop` revokes it. +/// - **panic**: `Drop` still runs during unwind, so a panic mid-dispatch +/// revokes it too. +/// +/// A missed revoke path would leave a standing grant, so this type has no +/// safe way to leak the session past its own lifetime: there is no `mem::forget`-safe +/// accessor, and the only way to keep the session alive is to hold the lease. +pub struct CredentialSessionLease { + broker: Arc, + session_id: CredentialSessionId, + revoked: bool, +} + +impl CredentialSessionLease { + /// The id of the session this lease guards. Read-only: it cannot be used + /// to extend the session's life past the lease being dropped. + pub fn session_id(&self) -> CredentialSessionId { + self.session_id + } + + /// Explicitly revokes the session now. Intended for the success and error + /// exit paths of a dispatch, where the caller can revoke synchronously + /// rather than waiting for `Drop`. + pub fn revoke(mut self) { + self.revoke_inner(); + } + + fn revoke_inner(&mut self) { + if !self.revoked { + self.revoked = true; + self.broker.revoke_session(self.session_id); + } + } +} + +impl Drop for CredentialSessionLease { + fn drop(&mut self) { + self.revoke_inner(); + } +} + +impl InMemoryCredentialBroker { + /// Mints a [`CredentialSession`](crate::CredentialSession) the first time + /// a `(invocation, binding)` pair is actually used, associates it with + /// `placeholder` so the proxy can find it by placeholder, and returns a + /// lease that revokes the session on drop. + /// + /// If the same `(invocation, capability, account)` binding is minted + /// again while its previous session is still live, the existing session + /// is reused rather than minting a second one — staging every possible + /// binding up front would itself be a standing grant, so callers must + /// only call this at actual first use. + pub fn mint_on_first_use( + self: &Arc, + placeholder: &CredentialPlaceholderToken, + request: CredentialSessionRequest, + ) -> Result { + let key = JitMintKey { + invocation_id: request.invocation_id, + capability_id: request.capability_id.clone(), + account_id: request.account_id.clone(), + }; + + if let Some(session_id) = self.jit_minted_session_id(&key)? + && self + .validate_session(session_id, chrono::Utc::now()) + .is_ok() + { + self.bind_placeholder_to_session(placeholder.clone(), session_id)?; + return Ok(CredentialSessionLease { + broker: self.clone(), + session_id, + revoked: false, + }); + } + + let session = self.create_session(request)?; + let session_id = session.correlation_id(); + self.record_jit_mint(key, session_id)?; + self.bind_placeholder_to_session(placeholder.clone(), session_id)?; + Ok(CredentialSessionLease { + broker: self.clone(), + session_id, + revoked: false, + }) + } + + /// Finds the live session currently bound to `placeholder`, if any. + /// + /// Returns `Ok(None)` — never an error — when the placeholder has no + /// live session: an unminted, expired, use-exhausted, or already-revoked + /// binding all "grant nothing" the same way. The returned session is + /// additionally checked against `scope` so a placeholder can never + /// resolve to a session outside the caller's own tenant/user scope. + pub fn find_session_by_placeholder( + &self, + placeholder: &CredentialPlaceholderToken, + scope: &ironclaw_host_api::ResourceScope, + ) -> Result, CredentialBrokerError> { + let Some(session_id) = self.placeholder_session_id(placeholder)? else { + return Ok(None); + }; + match self.validate_session(session_id, chrono::Utc::now()) { + Ok(session) if session.scope() == scope => Ok(Some(session)), + Ok(_) => Ok(None), + Err(CredentialBrokerError::UnknownSession { .. }) => Ok(None), + Err(CredentialBrokerError::SessionExpired { .. }) => Ok(None), + Err(CredentialBrokerError::SessionUseLimitExceeded { .. }) => Ok(None), + Err(other) => Err(other), + } + } + + /// Explicitly revokes a session, e.g. from + /// [`CredentialSessionLease`]'s success/error/drop paths. Idempotent: + /// revoking an already-unknown session is a no-op, since "no session" and + /// "revoked session" both mean the placeholder grants nothing. + pub fn revoke_session(&self, session_id: CredentialSessionId) { + if let Ok(mut sessions) = self.sessions.lock() { + sessions.remove(&session_id); + } + } + + fn jit_minted_session_id( + &self, + key: &JitMintKey, + ) -> Result, CredentialBrokerError> { + Ok(self + .jit_minted + .lock() + .map_err(|error| CredentialBrokerError::BrokerUnavailable { + reason: error.to_string(), + })? + .get(key) + .copied()) + } + + fn record_jit_mint( + &self, + key: JitMintKey, + session_id: CredentialSessionId, + ) -> Result<(), CredentialBrokerError> { + self.jit_minted + .lock() + .map_err(|error| CredentialBrokerError::BrokerUnavailable { + reason: error.to_string(), + })? + .insert(key, session_id); + Ok(()) + } + + fn bind_placeholder_to_session( + &self, + placeholder: CredentialPlaceholderToken, + session_id: CredentialSessionId, + ) -> Result<(), CredentialBrokerError> { + self.sessions_by_placeholder + .lock() + .map_err(|error| CredentialBrokerError::BrokerUnavailable { + reason: error.to_string(), + })? + .insert(placeholder, session_id); + Ok(()) + } + + fn placeholder_session_id( + &self, + placeholder: &CredentialPlaceholderToken, + ) -> Result, CredentialBrokerError> { + Ok(self + .sessions_by_placeholder + .lock() + .map_err(|error| CredentialBrokerError::BrokerUnavailable { + reason: error.to_string(), + })? + .get(placeholder) + .copied()) + } +} + +#[cfg(test)] +mod tests { + use std::panic::AssertUnwindSafe; + use std::sync::Arc; + use std::time::Duration; + + use chrono::Utc; + use ironclaw_host_api::{ + CapabilityId, ExtensionId, InvocationId, NetworkMethod, ProjectId, ResourceScope, + SecretHandle, TenantId, UserId, + }; + + use crate::{ + CredentialAccount, CredentialAccountId, CredentialAccountStatus, CredentialBrokerError, + CredentialPathPolicy, CredentialSessionRequest, CredentialTargetPolicy, + InMemoryCredentialBroker, RedactedJson, + }; + + use super::{CredentialPlaceholderRegistry, CredentialPlaceholderToken}; + + fn scope(tenant: &str, user: &str) -> ResourceScope { + ResourceScope { + tenant_id: TenantId::new(tenant).unwrap(), + user_id: UserId::new(user).unwrap(), + agent_id: None, + project_id: Some(ProjectId::new("project-a").unwrap()), + mission_id: None, + thread_id: None, + invocation_id: InvocationId::new(), + } + } + + fn account( + scope: ResourceScope, + id: CredentialAccountId, + handle: SecretHandle, + ) -> CredentialAccount { + CredentialAccount { + scope, + id, + provider_or_extension_id: ExtensionId::new("google").unwrap(), + label: "Prod".to_string(), + status: CredentialAccountStatus::Active, + secret_handles: vec![handle], + allowed_targets: vec![CredentialTargetPolicy { + scheme: "https".to_string(), + host: "api.example.com".to_string(), + port: Some(443), + path: CredentialPathPolicy::Prefix("/v1/".to_string()), + methods: vec![NetworkMethod::Get], + }], + redacted_metadata: RedactedJson::new(serde_json::json!({})), + updated_at: Utc::now(), + } + } + + fn request( + scope: ResourceScope, + account_id: CredentialAccountId, + url: &str, + ) -> CredentialSessionRequest { + CredentialSessionRequest { + invocation_id: scope.invocation_id, + scope, + capability_id: CapabilityId::new("google.drive").unwrap(), + extension_id: ExtensionId::new("google").unwrap(), + account_id, + method: NetworkMethod::Get, + url: url.to_string(), + expires_at: None, + max_uses: None, + } + } + + #[test] + fn placeholder_is_stable_across_repeated_lookups_and_recycle() { + let registry = CredentialPlaceholderRegistry::new(); + let tenant = TenantId::new("tenant-a").unwrap(); + let user = UserId::new("user-a").unwrap(); + let provider = ExtensionId::new("google").unwrap(); + + let first = registry.get_or_create(&tenant, &user, &provider).unwrap(); + let second = registry.get_or_create(&tenant, &user, &provider).unwrap(); + assert_eq!(first, second); + + // Simulate a container recycle: nothing about the registry is tied to + // a container, so a "new container" asking for the same triple again + // gets the identical token. + drop(first); + let after_recycle = registry.get_or_create(&tenant, &user, &provider).unwrap(); + assert_eq!(second, after_recycle); + } + + #[test] + fn placeholders_are_distinct_and_isolated_per_user() { + let registry = CredentialPlaceholderRegistry::new(); + let tenant = TenantId::new("tenant-a").unwrap(); + let provider = ExtensionId::new("google").unwrap(); + let user_a = UserId::new("user-a").unwrap(); + let user_b = UserId::new("user-b").unwrap(); + + let token_a = registry.get_or_create(&tenant, &user_a, &provider).unwrap(); + let token_b = registry.get_or_create(&tenant, &user_b, &provider).unwrap(); + assert_ne!(token_a, token_b); + + let owner_a = registry.resolve(&token_a).unwrap().unwrap(); + let owner_b = registry.resolve(&token_b).unwrap().unwrap(); + assert_eq!(owner_a.user_id, user_a); + assert_eq!(owner_b.user_id, user_b); + assert_ne!(owner_a.user_id, owner_b.user_id); + + // User A's placeholder must never resolve to user B's owner triple. + assert_ne!( + registry.resolve(&token_a).unwrap(), + registry.resolve(&token_b).unwrap() + ); + } + + #[test] + fn placeholder_token_requires_fixed_prefix() { + assert!(CredentialPlaceholderToken::parse("icsbx_abc").is_ok()); + let err = CredentialPlaceholderToken::parse("not_a_placeholder").unwrap_err(); + assert!(matches!( + err, + CredentialBrokerError::InvalidPlaceholderToken { .. } + )); + } + + #[test] + fn placeholder_with_no_live_session_grants_nothing() { + let broker = Arc::new(InMemoryCredentialBroker::new()); + let registry = CredentialPlaceholderRegistry::new(); + let tenant = TenantId::new("tenant-a").unwrap(); + let user = UserId::new("user-a").unwrap(); + let provider = ExtensionId::new("google").unwrap(); + let token = registry.get_or_create(&tenant, &user, &provider).unwrap(); + + let found = broker + .find_session_by_placeholder(&token, &scope("tenant-a", "user-a")) + .unwrap(); + assert!(found.is_none()); + } + + #[test] + fn jit_mint_binds_session_to_placeholder_and_enforces_target_policy() { + let broker = Arc::new(InMemoryCredentialBroker::new()); + let registry = CredentialPlaceholderRegistry::new(); + let tenant = TenantId::new("tenant-a").unwrap(); + let user = UserId::new("user-a").unwrap(); + let provider = ExtensionId::new("google").unwrap(); + let token = registry.get_or_create(&tenant, &user, &provider).unwrap(); + + let caller_scope = scope("tenant-a", "user-a"); + let account_id = CredentialAccountId::new("google_prod").unwrap(); + broker + .put_account(account( + caller_scope.clone(), + account_id.clone(), + SecretHandle::new("google_key").unwrap(), + )) + .unwrap(); + + // Out-of-policy request (wrong path) must be rejected, not minted. + let rejected = broker.mint_on_first_use( + &token, + request( + caller_scope.clone(), + account_id.clone(), + "https://api.example.com/v2/x", + ), + ); + assert!(matches!( + rejected, + Err(CredentialBrokerError::CredentialPolicyMismatch { .. }) + )); + assert!( + broker + .find_session_by_placeholder(&token, &caller_scope) + .unwrap() + .is_none() + ); + + // In-policy request mints and binds. + let lease = broker + .mint_on_first_use( + &token, + request( + caller_scope.clone(), + account_id, + "https://api.example.com/v1/x", + ), + ) + .unwrap(); + let bound = broker + .find_session_by_placeholder(&token, &caller_scope) + .unwrap() + .expect("session bound to placeholder after JIT mint"); + assert_eq!(bound.correlation_id(), lease.session_id()); + lease.revoke(); + } + + #[test] + fn placeholder_session_lookup_does_not_cross_user_scope() { + let broker = Arc::new(InMemoryCredentialBroker::new()); + let registry = CredentialPlaceholderRegistry::new(); + let tenant = TenantId::new("tenant-a").unwrap(); + let provider = ExtensionId::new("google").unwrap(); + let user_a = UserId::new("user-a").unwrap(); + let token_a = registry.get_or_create(&tenant, &user_a, &provider).unwrap(); + + let scope_a = scope("tenant-a", "user-a"); + let account_id = CredentialAccountId::new("google_prod").unwrap(); + broker + .put_account(account( + scope_a.clone(), + account_id.clone(), + SecretHandle::new("google_key").unwrap(), + )) + .unwrap(); + let lease = broker + .mint_on_first_use( + &token_a, + request(scope_a, account_id, "https://api.example.com/v1/x"), + ) + .unwrap(); + + // User B's scope must never see user A's session through the placeholder. + let other_scope = scope("tenant-a", "user-b"); + let found = broker + .find_session_by_placeholder(&token_a, &other_scope) + .unwrap(); + assert!(found.is_none()); + lease.revoke(); + } + + #[test] + fn lease_revokes_on_explicit_success_call() { + let broker = Arc::new(InMemoryCredentialBroker::new()); + let registry = CredentialPlaceholderRegistry::new(); + let (token, scope_a, account_id) = seeded(&broker, ®istry, "user-success"); + + let lease = broker + .mint_on_first_use( + &token, + request(scope_a, account_id, "https://api.example.com/v1/x"), + ) + .unwrap(); + let session_id = lease.session_id(); + lease.revoke(); // success path calls this explicitly + assert!(matches!( + broker.validate_session(session_id, Utc::now()), + Err(CredentialBrokerError::UnknownSession { .. }) + )); + } + + #[test] + fn lease_revokes_on_explicit_error_call() { + let broker = Arc::new(InMemoryCredentialBroker::new()); + let registry = CredentialPlaceholderRegistry::new(); + let (token, scope_a, account_id) = seeded(&broker, ®istry, "user-error"); + + let lease = broker + .mint_on_first_use( + &token, + request(scope_a, account_id, "https://api.example.com/v1/x"), + ) + .unwrap(); + let session_id = lease.session_id(); + + let dispatch_result: Result<(), &'static str> = Err("upstream 500"); + if dispatch_result.is_err() { + lease.revoke(); // error path calls this explicitly too + } + assert!(matches!( + broker.validate_session(session_id, Utc::now()), + Err(CredentialBrokerError::UnknownSession { .. }) + )); + } + + #[tokio::test] + async fn lease_revokes_on_timeout_via_drop() { + let broker = Arc::new(InMemoryCredentialBroker::new()); + let registry = CredentialPlaceholderRegistry::new(); + let (token, scope_a, account_id) = seeded(&broker, ®istry, "user-timeout"); + + let lease = broker + .mint_on_first_use( + &token, + request(scope_a, account_id, "https://api.example.com/v1/x"), + ) + .unwrap(); + let session_id = lease.session_id(); + + let outcome = tokio::time::timeout(Duration::from_millis(5), async move { + let _lease = lease; // held across the (never-completing) sleep + tokio::time::sleep(Duration::from_secs(60)).await; + }) + .await; + + assert!(outcome.is_err(), "expected the dispatch to time out"); + assert!(matches!( + broker.validate_session(session_id, Utc::now()), + Err(CredentialBrokerError::UnknownSession { .. }) + )); + } + + #[test] + fn lease_revokes_on_panic_via_drop() { + let broker = Arc::new(InMemoryCredentialBroker::new()); + let registry = CredentialPlaceholderRegistry::new(); + let (token, scope_a, account_id) = seeded(&broker, ®istry, "user-panic"); + + let lease = broker + .mint_on_first_use( + &token, + request(scope_a, account_id, "https://api.example.com/v1/x"), + ) + .unwrap(); + let session_id = lease.session_id(); + + let result = std::panic::catch_unwind(AssertUnwindSafe(|| { + let _lease = lease; // dropped during unwind + panic!("simulated panic mid-dispatch"); + })); + + assert!(result.is_err()); + assert!(matches!( + broker.validate_session(session_id, Utc::now()), + Err(CredentialBrokerError::UnknownSession { .. }) + )); + } + + fn seeded( + broker: &Arc, + registry: &CredentialPlaceholderRegistry, + user: &str, + ) -> ( + CredentialPlaceholderToken, + ResourceScope, + CredentialAccountId, + ) { + let tenant = TenantId::new("tenant-a").unwrap(); + let user_id = UserId::new(user).unwrap(); + let provider = ExtensionId::new("google").unwrap(); + let token = registry + .get_or_create(&tenant, &user_id, &provider) + .unwrap(); + let caller_scope = scope("tenant-a", user); + let account_id = CredentialAccountId::new("google_prod").unwrap(); + broker + .put_account(account( + caller_scope.clone(), + account_id.clone(), + SecretHandle::new("google_key").unwrap(), + )) + .unwrap(); + (token, caller_scope, account_id) + } +} From bdbcadbc04925eaa264bccb5f5e566f51dbb5ac9 Mon Sep 17 00:00:00 2001 From: Henry Park Date: Sun, 26 Jul 2026 14:57:49 -0700 Subject: [PATCH 02/14] feat(safety): detect sandbox credential placeholders The upcoming credential firewall injects inert icsbx_-prefixed placeholders into the sandbox in place of real secrets; the egress proxy swaps them for the real credential at request time. Placeholders are stable and inert but must never cross the trust boundary into model output, logs, or transcripts. Sandbox exec output already runs through the leak detector, so only the pattern was missing. Co-Authored-By: Claude Opus 5 (1M context) --- crates/ironclaw_safety/src/leak_detector.rs | 66 +++++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/crates/ironclaw_safety/src/leak_detector.rs b/crates/ironclaw_safety/src/leak_detector.rs index 44ce3c00bf5..4263ad5c93c 100644 --- a/crates/ironclaw_safety/src/leak_detector.rs +++ b/crates/ironclaw_safety/src/leak_detector.rs @@ -607,6 +607,19 @@ fn default_patterns() -> Vec { severity: LeakSeverity::Critical, action: LeakAction::Block, }, + // Sandbox credential placeholder (icsbx_). The credential + // firewall injects these inert placeholders into the sandbox in place + // of real secrets; the egress proxy swaps them for the real credential + // at request time. A placeholder must never cross the trust boundary + // into model output, logs, or transcripts, so it is treated like any + // other secret. Word boundaries keep this from matching `icsbx_` as a + // substring of a longer identifier. + LeakPattern { + name: "sandbox_credential_placeholder".to_string(), + regex: Regex::new(r"\bicsbx_[A-Za-z0-9]{16,}\b").unwrap(), // safety: hardcoded literal + severity: LeakSeverity::Critical, + action: LeakAction::Block, + }, // High entropy hex (potential secrets, warn only) // Uses word boundary since look-around isn't supported in the regex crate. // This catches standalone 64-char hex strings (like SHA256 hashes used as secrets). @@ -1181,6 +1194,59 @@ mod tests { ); } + #[test] + fn test_detect_sandbox_credential_placeholder() { + let detector = LeakDetector::new(); + let content = "found in ~/.git-credentials: icsbx_7f3a9b2c1d4e5f60"; + let result = detector.scan(content); + assert!(result.should_block, "sandbox placeholder not detected"); + assert!( + result + .matches + .iter() + .any(|m| m.pattern_name == "sandbox_credential_placeholder") + ); + } + + #[test] + fn test_sandbox_credential_placeholder_short_suffix_passes() { + let detector = LeakDetector::new(); + let content = "icsbx_ab"; + let result = detector.scan(content); + assert!( + !result + .matches + .iter() + .any(|m| m.pattern_name == "sandbox_credential_placeholder"), + "short suffix should not match placeholder pattern" + ); + } + + #[test] + fn test_sandbox_credential_placeholder_substring_of_longer_word_passes() { + let detector = LeakDetector::new(); + // "icsbx_" embedded inside a longer identifier, not at a word boundary. + let content = "myicsbx_7f3a9b2c1d4e5f60prefix"; + let result = detector.scan(content); + assert!( + !result + .matches + .iter() + .any(|m| m.pattern_name == "sandbox_credential_placeholder"), + "icsbx_ substring inside a longer word should not match" + ); + } + + #[test] + fn test_scan_and_clean_blocks_sandbox_credential_placeholder() { + let detector = LeakDetector::new(); + let content = "icsbx_7f3a9b2c1d4e5f60"; + assert!( + detector.scan_and_clean(content).is_err(), + "scan_and_clean should block sandbox credential placeholder" + ); + } + /// Adversarial tests for leak detector regex patterns and masking. /// See . mod adversarial { From 3afe3e7bb6b9775fd025f861b283029bf7161339 Mon Sep 17 00:00:00 2001 From: Henry Park Date: Sun, 26 Jul 2026 18:32:40 -0700 Subject: [PATCH 03/14] fix(secrets): refcount JIT leases and multi-bind placeholder sessions Round-1 code review found two correctness gaps in the JIT credential- session skeleton: mint_on_first_use's cache-hit reuse handed out a second lease over an already-live session with no reference counting, so the first caller to revoke/drop its lease killed the session out from under a second caller still holding it; and sessions_by_placeholder was a single-valued map, so binding a second session to a stable placeholder (a second account under the same provider, or an overlapping invocation) silently clobbered the first binding instead of tracking both. Fixes: - Add a per-session lease refcount; a session is only actually revoked once its last outstanding lease releases it. - Make sessions_by_placeholder multi-valued (HashSet per token) and prune it, plus jit_minted and lease_refcounts, from revoke_session so a long-lived process doesn't accumulate stale entries. - revoke_session recovers from a poisoned lock instead of silently no-op'ing, matching the "a missed revoke can never leave a standing grant" invariant this module documents. - Add TryFrom/AsRef/into_inner() to CredentialPlaceholderToken and rename CredentialPlaceholderOwner::provider_id to provider_or_extension_id, matching this crate's existing newtype/naming conventions. - Pin the icsbx_ prefix shared between ironclaw_secrets and the ironclaw_safety leak-detector pattern with a dev-dependency-only regression test instead of a real crate dependency, so ironclaw_safety stays a dependency-light substrate (ruled via thermo-nuclear review). - Tighten cross-reference comments (W6 -> W6-EGRESS-PROXY, name the concrete sibling leak pattern) per local-patterns review. Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.lock | 1 + crates/ironclaw_safety/Cargo.toml | 6 + crates/ironclaw_safety/src/leak_detector.rs | 44 ++++ crates/ironclaw_secrets/src/lib.rs | 34 ++- crates/ironclaw_secrets/src/placeholder.rs | 276 +++++++++++++++++--- 5 files changed, 312 insertions(+), 49 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e7624d8c897..a969bcc8f1e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4621,6 +4621,7 @@ version = "0.2.2" dependencies = [ "aho-corasick", "criterion", + "ironclaw_secrets", "regex", "serde_json", "thiserror 2.0.18", diff --git a/crates/ironclaw_safety/Cargo.toml b/crates/ironclaw_safety/Cargo.toml index 473f6d34e4b..e696ad1735f 100644 --- a/crates/ironclaw_safety/Cargo.toml +++ b/crates/ironclaw_safety/Cargo.toml @@ -26,6 +26,12 @@ urlencoding = "2" [dev-dependencies] criterion = "0.8" +# Test-only: pins the sandbox_credential_placeholder leak pattern against +# ironclaw_secrets::CREDENTIAL_PLACEHOLDER_PREFIX so the two can't silently +# drift apart. Deliberately not a normal dependency — ironclaw_safety stays a +# dependency-light substrate; see leak_detector.rs's +# sandbox_credential_placeholder_prefix_matches_registry test. +ironclaw_secrets = { path = "../ironclaw_secrets" } [[bench]] name = "safety_check" diff --git a/crates/ironclaw_safety/src/leak_detector.rs b/crates/ironclaw_safety/src/leak_detector.rs index 4263ad5c93c..9569407d189 100644 --- a/crates/ironclaw_safety/src/leak_detector.rs +++ b/crates/ironclaw_safety/src/leak_detector.rs @@ -614,6 +614,15 @@ fn default_patterns() -> Vec { // into model output, logs, or transcripts, so it is treated like any // other secret. Word boundaries keep this from matching `icsbx_` as a // substring of a longer identifier. + // + // The `icsbx_` literal here must stay in sync with + // `ironclaw_secrets::placeholder::CREDENTIAL_PLACEHOLDER_PREFIX` + // (crates/ironclaw_secrets/src/placeholder.rs), which is the actual + // owner of this prefix. `ironclaw_safety` deliberately does not take + // `ironclaw_secrets` as a normal dependency just to share one string + // constant — see `sandbox_credential_placeholder_prefix_matches_registry` + // below, a dev-dependency-only regression test that fails loudly if + // the two ever drift apart. LeakPattern { name: "sandbox_credential_placeholder".to_string(), regex: Regex::new(r"\bicsbx_[A-Za-z0-9]{16,}\b").unwrap(), // safety: hardcoded literal @@ -1247,6 +1256,41 @@ mod tests { ); } + #[test] + fn sandbox_credential_placeholder_prefix_matches_registry() { + // `ironclaw_safety` deliberately does not take `ironclaw_secrets` as a + // normal dependency just to share the "icsbx_" prefix constant (it + // stays a dependency-light substrate). This dev-dependency-only test + // is the regression net instead: if the prefix is ever rotated in + // `ironclaw_secrets::placeholder::CREDENTIAL_PLACEHOLDER_PREFIX` + // without updating the hardcoded regex literal above, this fails + // loudly instead of the leak detector silently going stale. + assert_eq!( + ironclaw_secrets::CREDENTIAL_PLACEHOLDER_PREFIX, + "icsbx_", + "leak_detector's sandbox_credential_placeholder regex hardcodes 'icsbx_'; \ + update both if this constant ever changes" + ); + + // Shape a registry-issued token actually has: the fixed prefix plus a + // UUIDv4 `simple()` suffix (32 lowercase hex chars, no dashes) — see + // `CredentialPlaceholderToken::generate()` in ironclaw_secrets. + let token = format!( + "{}{}", + ironclaw_secrets::CREDENTIAL_PLACEHOLDER_PREFIX, + "0123456789abcdef0123456789abcdef" + ); + let detector = LeakDetector::new(); + let result = detector.scan(&format!("leaked token: {token}")); + assert!( + result + .matches + .iter() + .any(|m| m.pattern_name == "sandbox_credential_placeholder"), + "a realistically-shaped registry-issued placeholder token must be caught" + ); + } + /// Adversarial tests for leak detector regex patterns and masking. /// See . mod adversarial { diff --git a/crates/ironclaw_secrets/src/lib.rs b/crates/ironclaw_secrets/src/lib.rs index 0535e62c087..747ef3ebca6 100644 --- a/crates/ironclaw_secrets/src/lib.rs +++ b/crates/ironclaw_secrets/src/lib.rs @@ -20,7 +20,7 @@ pub use placeholder::{ }; pub use secret_store::{CredentialBroker, SecretStore}; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::fmt; use std::sync::Mutex; @@ -572,12 +572,34 @@ pub struct InMemoryCredentialBroker { /// Secondary index for JIT minting: `(invocation, capability, account)` -> /// the session already minted for it, so re-use within the same dispatch /// does not mint a second session. See [`placeholder::JitMintKey`] and - /// [`InMemoryCredentialBroker::mint_on_first_use`]. + /// [`InMemoryCredentialBroker::mint_on_first_use`]. Pruned (by session id) + /// on [`InMemoryCredentialBroker::revoke_session`] so a long-lived process + /// does not accumulate one entry per invocation forever. jit_minted: Mutex>, - /// Secondary index the egress proxy (W6) will use: placeholder token -> - /// the session currently bound to it. See - /// [`InMemoryCredentialBroker::find_session_by_placeholder`]. - sessions_by_placeholder: Mutex>, + /// Secondary index the egress proxy (W6-EGRESS-PROXY, not built yet) will + /// use: placeholder token -> the live session(s) currently bound to it. + /// Multi-valued because a placeholder is stable per `(tenant, user, + /// provider)` while sessions are minted per invocation/account, so more + /// than one live session can legitimately be bound to the same + /// placeholder at once (e.g. two overlapping invocations, or two + /// accounts under one provider) — a single-slot map would silently drop + /// one binding when the other was written. See + /// [`InMemoryCredentialBroker::find_session_by_placeholder`]. Choosing + /// among multiple scope-matching candidates by request target (e.g. + /// which of two same-provider accounts a specific outbound URL should + /// use) is the egress proxy's job, not this registry's — it is not built + /// yet, so `find_session_by_placeholder` returns the first scope-match it + /// finds. Pruned (by session id) on `revoke_session`. + sessions_by_placeholder: + Mutex>>, + /// Outstanding-lease counter per JIT-minted session id. `mint_on_first_use` + /// can hand out more than one [`CredentialSessionLease`] for the *same* + /// session (its cache-hit path reuses a still-live session rather than + /// minting a second one), so revoking one lease must not revoke the + /// session out from under another lease still holding it — the session is + /// only actually revoked when the last outstanding lease releases it. See + /// [`InMemoryCredentialBroker::release_lease`]. + lease_refcounts: Mutex>, } #[derive(Debug, Clone)] diff --git a/crates/ironclaw_secrets/src/placeholder.rs b/crates/ironclaw_secrets/src/placeholder.rs index cfbbac27e3d..3b326a457f9 100644 --- a/crates/ironclaw_secrets/src/placeholder.rs +++ b/crates/ironclaw_secrets/src/placeholder.rs @@ -3,9 +3,9 @@ //! The container running a sandboxed invocation never sees real secret //! material. Instead it is handed a **placeholder token** — an inert string //! that identifies "the credential for this tenant/user/provider" without -//! granting anything on its own. The egress proxy (W6, not built yet) swaps -//! the placeholder for a live [`CredentialSession`] at request time, host -//! side only. +//! granting anything on its own. The egress proxy (W6-EGRESS-PROXY, not built +//! yet) swaps the placeholder for a live [`CredentialSession`] at request +//! time, host side only. //! //! This module owns two host-side responsibilities: //! @@ -24,7 +24,7 @@ //! leave a standing grant. use std::collections::HashMap; use std::fmt; -use std::sync::{Arc, Mutex}; +use std::sync::{Arc, Mutex, MutexGuard, PoisonError}; use ironclaw_host_api::{CapabilityId, ExtensionId, InvocationId, TenantId, UserId}; use uuid::Uuid; @@ -36,10 +36,17 @@ use crate::{ /// Fixed prefix for every placeholder token. /// -/// A companion leak-detector pattern (owned elsewhere, see the sandbox -/// credential firewall design doc) recognizes this prefix so a placeholder -/// that somehow escapes the container is flagged the same way a real secret -/// would be — even though, unlike a real secret, holding one grants nothing. +/// The `sandbox_credential_placeholder` leak-detector pattern in +/// `ironclaw_safety::leak_detector` (owned there, not here — see that +/// crate's `default_patterns()`) independently recognizes this same prefix, +/// so a placeholder that somehow escapes the container is flagged the same +/// way a real secret would be, even though, unlike a real secret, holding one +/// grants nothing. `ironclaw_safety` deliberately does not depend on this +/// crate (it stays a dependency-light substrate), so the two patterns are +/// pinned together by a regression test instead of a shared dependency: see +/// `sandbox_credential_placeholder_prefix_matches_registry` in +/// `ironclaw_safety/src/leak_detector.rs`. If this prefix ever changes, +/// update the regex there too. pub const CREDENTIAL_PLACEHOLDER_PREFIX: &str = "icsbx_"; /// Opaque, stable placeholder token for a `(tenant, user, provider)` triple. @@ -60,23 +67,46 @@ impl CredentialPlaceholderToken { )) } - /// Parses a placeholder token received from the sandbox side (e.g. off an - /// outbound request), rejecting anything that does not carry the fixed - /// prefix. - pub fn parse(value: impl Into) -> Result { - let value = value.into(); + fn validate(value: &str) -> Result<(), CredentialBrokerError> { if !value.starts_with(CREDENTIAL_PLACEHOLDER_PREFIX) { return Err(CredentialBrokerError::InvalidPlaceholderToken { - value, + value: value.to_string(), reason: format!("must start with '{CREDENTIAL_PLACEHOLDER_PREFIX}'"), }); } + Ok(()) + } + + /// Parses a placeholder token received from the sandbox side (e.g. off an + /// outbound request), rejecting anything that does not carry the fixed + /// prefix. + pub fn parse(value: impl Into) -> Result { + let value = value.into(); + Self::validate(&value)?; Ok(Self(value)) } pub fn as_str(&self) -> &str { &self.0 } + + pub fn into_inner(self) -> String { + self.0 + } +} + +impl TryFrom for CredentialPlaceholderToken { + type Error = CredentialBrokerError; + + fn try_from(value: String) -> Result { + Self::parse(value) + } +} + +impl AsRef for CredentialPlaceholderToken { + fn as_ref(&self) -> &str { + &self.0 + } } impl fmt::Debug for CredentialPlaceholderToken { @@ -99,14 +129,14 @@ impl fmt::Display for CredentialPlaceholderToken { pub struct CredentialPlaceholderOwner { pub tenant_id: TenantId, pub user_id: UserId, - pub provider_id: ExtensionId, + pub provider_or_extension_id: ExtensionId, } #[derive(Debug, Clone, PartialEq, Eq, Hash)] struct CredentialPlaceholderOwnerKey { tenant_id: TenantId, user_id: UserId, - provider_id: ExtensionId, + provider_or_extension_id: ExtensionId, } /// Host-side registry mapping `(tenant, user, provider)` to a stable @@ -142,7 +172,7 @@ impl CredentialPlaceholderRegistry { let key = CredentialPlaceholderOwnerKey { tenant_id: tenant_id.clone(), user_id: user_id.clone(), - provider_id: provider_id.clone(), + provider_or_extension_id: provider_id.clone(), }; let mut by_owner = self.by_owner @@ -166,7 +196,7 @@ impl CredentialPlaceholderRegistry { CredentialPlaceholderOwner { tenant_id: tenant_id.clone(), user_id: user_id.clone(), - provider_id: provider_id.clone(), + provider_or_extension_id: provider_id.clone(), }, ); Ok(token) @@ -238,7 +268,7 @@ impl CredentialSessionLease { fn revoke_inner(&mut self) { if !self.revoked { self.revoked = true; - self.broker.revoke_session(self.session_id); + self.broker.release_lease(self.session_id); } } } @@ -259,7 +289,13 @@ impl InMemoryCredentialBroker { /// again while its previous session is still live, the existing session /// is reused rather than minting a second one — staging every possible /// binding up front would itself be a standing grant, so callers must - /// only call this at actual first use. + /// only call this at actual first use. Because that reuse can hand out + /// more than one [`CredentialSessionLease`] for the identical session, + /// each lease only *releases* its own reference on drop/`revoke()`; the + /// session itself is revoked once the last outstanding lease releases it + /// (see [`InMemoryCredentialBroker::release_lease`]), so an earlier + /// caller finishing first can never pull the session out from under a + /// later caller still using it. pub fn mint_on_first_use( self: &Arc, placeholder: &CredentialPlaceholderToken, @@ -277,6 +313,7 @@ impl InMemoryCredentialBroker { .is_ok() { self.bind_placeholder_to_session(placeholder.clone(), session_id)?; + self.acquire_lease_refcount(session_id)?; return Ok(CredentialSessionLease { broker: self.clone(), session_id, @@ -288,6 +325,7 @@ impl InMemoryCredentialBroker { let session_id = session.correlation_id(); self.record_jit_mint(key, session_id)?; self.bind_placeholder_to_session(placeholder.clone(), session_id)?; + self.acquire_lease_refcount(session_id)?; Ok(CredentialSessionLease { broker: self.clone(), session_id, @@ -295,38 +333,97 @@ impl InMemoryCredentialBroker { }) } - /// Finds the live session currently bound to `placeholder`, if any. + /// Finds a live session currently bound to `placeholder`, if any. /// /// Returns `Ok(None)` — never an error — when the placeholder has no /// live session: an unminted, expired, use-exhausted, or already-revoked - /// binding all "grant nothing" the same way. The returned session is - /// additionally checked against `scope` so a placeholder can never - /// resolve to a session outside the caller's own tenant/user scope. + /// binding all "grant nothing" the same way. Every candidate session + /// bound to the placeholder is additionally checked against `scope` so a + /// placeholder can never resolve to a session outside the caller's own + /// tenant/user/invocation scope. + /// + /// More than one live session can be bound to the same placeholder at + /// once (e.g. two accounts under one provider, or two overlapping + /// invocations); this returns the first one whose scope matches. Picking + /// the *correct* one by request target when several match is the egress + /// proxy's job (not built yet), so a caller that needs to disambiguate + /// further must do so itself once it has the candidate. pub fn find_session_by_placeholder( &self, placeholder: &CredentialPlaceholderToken, scope: &ironclaw_host_api::ResourceScope, ) -> Result, CredentialBrokerError> { - let Some(session_id) = self.placeholder_session_id(placeholder)? else { - return Ok(None); - }; - match self.validate_session(session_id, chrono::Utc::now()) { - Ok(session) if session.scope() == scope => Ok(Some(session)), - Ok(_) => Ok(None), - Err(CredentialBrokerError::UnknownSession { .. }) => Ok(None), - Err(CredentialBrokerError::SessionExpired { .. }) => Ok(None), - Err(CredentialBrokerError::SessionUseLimitExceeded { .. }) => Ok(None), - Err(other) => Err(other), + for session_id in self.placeholder_session_ids(placeholder)? { + match self.validate_session(session_id, chrono::Utc::now()) { + Ok(session) if session.scope() == scope => return Ok(Some(session)), + Ok(_) => continue, + Err(CredentialBrokerError::UnknownSession { .. }) => continue, + Err(CredentialBrokerError::SessionExpired { .. }) => continue, + Err(CredentialBrokerError::SessionUseLimitExceeded { .. }) => continue, + Err(other) => return Err(other), + } } + Ok(None) } /// Explicitly revokes a session, e.g. from - /// [`CredentialSessionLease`]'s success/error/drop paths. Idempotent: - /// revoking an already-unknown session is a no-op, since "no session" and - /// "revoked session" both mean the placeholder grants nothing. + /// [`CredentialSessionLease`]'s success/error/drop paths (by way of + /// [`InMemoryCredentialBroker::release_lease`]). Idempotent: revoking an + /// already-unknown session is a no-op, since "no session" and "revoked + /// session" both mean the placeholder grants nothing. + /// + /// Also prunes this session out of the `jit_minted` and + /// `sessions_by_placeholder` secondary indices so a long-lived process + /// does not accumulate stale entries for every invocation/binding that + /// has ever existed. pub fn revoke_session(&self, session_id: CredentialSessionId) { - if let Ok(mut sessions) = self.sessions.lock() { - sessions.remove(&session_id); + lock_or_recover(&self.sessions).remove(&session_id); + lock_or_recover(&self.jit_minted) + .retain(|_, minted_session_id| *minted_session_id != session_id); + lock_or_recover(&self.sessions_by_placeholder).retain(|_, session_ids| { + session_ids.remove(&session_id); + !session_ids.is_empty() + }); + lock_or_recover(&self.lease_refcounts).remove(&session_id); + } + + /// Registers one outstanding lease reference for `session_id`. See + /// [`InMemoryCredentialBroker::release_lease`] for the matching release + /// half of this pair. + fn acquire_lease_refcount( + &self, + session_id: CredentialSessionId, + ) -> Result<(), CredentialBrokerError> { + *lock_or_recover(&self.lease_refcounts) + .entry(session_id) + .or_insert(0) += 1; + Ok(()) + } + + /// Releases one outstanding lease reference for `session_id`, revoking + /// the underlying session only once the last outstanding reference is + /// released. See [`CredentialSessionLease::revoke_inner`] — every path + /// that drops or explicitly revokes a lease calls this instead of + /// `revoke_session` directly, so concurrent callers reusing the same + /// JIT-minted session (`mint_on_first_use`'s cache-hit path) cannot have + /// their still-live session revoked out from under them by an earlier + /// caller finishing first. + pub(crate) fn release_lease(&self, session_id: CredentialSessionId) { + let should_revoke = { + let mut counts = lock_or_recover(&self.lease_refcounts); + match counts.get_mut(&session_id) { + Some(count) if *count > 1 => { + *count -= 1; + false + } + _ => { + counts.remove(&session_id); + true + } + } + }; + if should_revoke { + self.revoke_session(session_id); } } @@ -368,14 +465,16 @@ impl InMemoryCredentialBroker { .map_err(|error| CredentialBrokerError::BrokerUnavailable { reason: error.to_string(), })? - .insert(placeholder, session_id); + .entry(placeholder) + .or_default() + .insert(session_id); Ok(()) } - fn placeholder_session_id( + fn placeholder_session_ids( &self, placeholder: &CredentialPlaceholderToken, - ) -> Result, CredentialBrokerError> { + ) -> Result, CredentialBrokerError> { Ok(self .sessions_by_placeholder .lock() @@ -383,10 +482,19 @@ impl InMemoryCredentialBroker { reason: error.to_string(), })? .get(placeholder) - .copied()) + .map(|session_ids| session_ids.iter().copied().collect()) + .unwrap_or_default()) } } +/// Locks `mutex`, recovering the inner guard on poison rather than silently +/// no-op'ing. A poisoned lock here must never be treated as "nothing to +/// clean up" — that would let a revoke path fail open and leave a standing +/// grant, contradicting this module's core invariant. +fn lock_or_recover(mutex: &Mutex) -> MutexGuard<'_, T> { + mutex.lock().unwrap_or_else(PoisonError::into_inner) +} + #[cfg(test)] mod tests { use std::panic::AssertUnwindSafe; @@ -719,6 +827,88 @@ mod tests { )); } + #[test] + fn reused_session_survives_the_first_of_two_leases_revoking() { + // Two mint_on_first_use calls for the identical (invocation, + // capability, account) binding — the documented cache-hit reuse path + // — must hand out two independent leases over the *same* session. + // Revoking the first-acquired lease must not revoke the session out + // from under the second lease still holding it; only releasing both + // should actually revoke it. + let broker = Arc::new(InMemoryCredentialBroker::new()); + let registry = CredentialPlaceholderRegistry::new(); + let (token, scope_a, account_id) = seeded(&broker, ®istry, "user-refcount"); + + let first_lease = broker + .mint_on_first_use( + &token, + request( + scope_a.clone(), + account_id.clone(), + "https://api.example.com/v1/x", + ), + ) + .unwrap(); + let second_lease = broker + .mint_on_first_use( + &token, + request(scope_a, account_id, "https://api.example.com/v1/x"), + ) + .unwrap(); + assert_eq!( + first_lease.session_id(), + second_lease.session_id(), + "cache-hit reuse must return the same underlying session" + ); + let session_id = first_lease.session_id(); + + first_lease.revoke(); + assert!( + broker.validate_session(session_id, Utc::now()).is_ok(), + "the second lease is still outstanding; revoking the first must not kill the shared session" + ); + + second_lease.revoke(); + assert!(matches!( + broker.validate_session(session_id, Utc::now()), + Err(CredentialBrokerError::UnknownSession { .. }) + )); + } + + #[test] + fn revoke_prunes_secondary_indices_so_placeholder_no_longer_resolves() { + // Revoking a session must not just make validate_session fail — it + // must also stop the placeholder from resolving to it at all, and + // must not leave stale bookkeeping (jit_minted / sessions_by_placeholder) + // behind for the life of the process. + let broker = Arc::new(InMemoryCredentialBroker::new()); + let registry = CredentialPlaceholderRegistry::new(); + let (token, scope_a, account_id) = seeded(&broker, ®istry, "user-prune"); + + let lease = broker + .mint_on_first_use( + &token, + request(scope_a.clone(), account_id, "https://api.example.com/v1/x"), + ) + .unwrap(); + assert!( + broker + .find_session_by_placeholder(&token, &scope_a) + .unwrap() + .is_some() + ); + + lease.revoke(); + + assert!( + broker + .find_session_by_placeholder(&token, &scope_a) + .unwrap() + .is_none(), + "placeholder must not resolve to a revoked session" + ); + } + fn seeded( broker: &Arc, registry: &CredentialPlaceholderRegistry, From 4bcc1ae28129738e96833d38f8d772bc6cd4c6f1 Mon Sep 17 00:00:00 2001 From: Henry Park Date: Sun, 26 Jul 2026 18:48:47 -0700 Subject: [PATCH 04/14] fix(secrets): close JIT lease reuse race and validate placeholder charset MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 2 review (performance/tests dimensions round 1 missed): - mint_on_first_use's cache-hit path validated a jit_minted session and only incremented its lease refcount afterward, as separate lock acquisitions. A concurrent release_lease on the last outstanding lease could revoke the session in that window, handing back a lease for an already-dead session. Fixed by incrementing the refcount first (only succeeds while a live entry exists) and validating after, backing out the reference if invalid — serializes the join against release_lease on the same lease_refcounts lock instead of racing across mutexes. - CredentialPlaceholderToken::validate only checked the prefix, unlike this crate's own validate_credential_id convention; a malformed suffix (e.g. carrying control characters) could reach a container's env/logs once this token round-trips through the not-yet-built egress proxy. Now enforces the same shape generate() produces. - Dropped acquire_lease_refcount's vestigial Result (it can't fail) and deduplicated mint_on_first_use's bind+lease-construction tail. Co-Authored-By: Claude Opus 5 (1M context) --- crates/ironclaw_secrets/src/lib.rs | 8 + crates/ironclaw_secrets/src/placeholder.rs | 309 ++++++++++++++++++--- 2 files changed, 275 insertions(+), 42 deletions(-) diff --git a/crates/ironclaw_secrets/src/lib.rs b/crates/ironclaw_secrets/src/lib.rs index 747ef3ebca6..b3609a23a1b 100644 --- a/crates/ironclaw_secrets/src/lib.rs +++ b/crates/ironclaw_secrets/src/lib.rs @@ -1243,6 +1243,14 @@ mod tests { .stable_reason(), "BackendMisconfigured" ); + assert_eq!( + CredentialBrokerError::InvalidPlaceholderToken { + value: "bad".to_string(), + reason: "must start with 'icsbx_'".to_string(), + } + .stable_reason(), + "MissingCredential" + ); } #[test] diff --git a/crates/ironclaw_secrets/src/placeholder.rs b/crates/ironclaw_secrets/src/placeholder.rs index 3b326a457f9..da27d8a0b0e 100644 --- a/crates/ironclaw_secrets/src/placeholder.rs +++ b/crates/ironclaw_secrets/src/placeholder.rs @@ -67,12 +67,40 @@ impl CredentialPlaceholderToken { )) } + /// Minimum length of the suffix after [`CREDENTIAL_PLACEHOLDER_PREFIX`], + /// matching the shortest suffix the `sandbox_credential_placeholder` leak + /// pattern (`ironclaw_safety::leak_detector`) will recognize and the + /// shape [`CredentialPlaceholderToken::generate`] actually produces (a + /// 32-character UUIDv4 `simple()` suffix). + const MIN_SUFFIX_LEN: usize = 16; + fn validate(value: &str) -> Result<(), CredentialBrokerError> { - if !value.starts_with(CREDENTIAL_PLACEHOLDER_PREFIX) { + let Some(suffix) = value.strip_prefix(CREDENTIAL_PLACEHOLDER_PREFIX) else { return Err(CredentialBrokerError::InvalidPlaceholderToken { value: value.to_string(), reason: format!("must start with '{CREDENTIAL_PLACEHOLDER_PREFIX}'"), }); + }; + // Mirror `validate_credential_id`'s charset discipline (this crate's + // established convention for opaque id newtypes) rather than + // accepting an arbitrary suffix: this token is designed to sit in a + // container's environment/config and a human or log line may see it + // (see the type doc comment), so malformed input — including control + // characters or shell metacharacters an attacker-controlled sandbox + // side might send back through `parse` — must fail closed here + // instead of propagating downstream. + if suffix.len() < Self::MIN_SUFFIX_LEN + || !suffix + .chars() + .all(|character| character.is_ascii_alphanumeric()) + { + return Err(CredentialBrokerError::InvalidPlaceholderToken { + value: value.to_string(), + reason: format!( + "must be '{CREDENTIAL_PLACEHOLDER_PREFIX}' followed by at least {} ASCII alphanumeric characters", + Self::MIN_SUFFIX_LEN + ), + }); } Ok(()) } @@ -308,24 +336,30 @@ impl InMemoryCredentialBroker { }; if let Some(session_id) = self.jit_minted_session_id(&key)? - && self - .validate_session(session_id, chrono::Utc::now()) - .is_ok() + && self.try_join_live_lease(session_id, chrono::Utc::now())? { - self.bind_placeholder_to_session(placeholder.clone(), session_id)?; - self.acquire_lease_refcount(session_id)?; - return Ok(CredentialSessionLease { - broker: self.clone(), - session_id, - revoked: false, - }); + return self.finish_lease(placeholder, session_id); } let session = self.create_session(request)?; let session_id = session.correlation_id(); self.record_jit_mint(key, session_id)?; + // This session_id was just minted and is not yet published to + // `jit_minted`/`sessions_by_placeholder`, so no concurrent caller can + // reference it yet: registering the first lease reference here can't + // race a concurrent `release_lease` the way the reuse path above can. + self.acquire_lease_refcount(session_id); + self.finish_lease(placeholder, session_id) + } + + /// Common tail shared by both `mint_on_first_use` branches: binds + /// `session_id` to `placeholder` and hands back the RAII lease. + fn finish_lease( + self: &Arc, + placeholder: &CredentialPlaceholderToken, + session_id: CredentialSessionId, + ) -> Result { self.bind_placeholder_to_session(placeholder.clone(), session_id)?; - self.acquire_lease_refcount(session_id)?; Ok(CredentialSessionLease { broker: self.clone(), session_id, @@ -333,6 +367,60 @@ impl InMemoryCredentialBroker { }) } + /// Attempts to join an existing lease on `session_id`, returning whether + /// it succeeded. + /// + /// The refcount increment and the liveness check are deliberately ordered + /// increment-then-validate, not validate-then-increment: incrementing + /// first only succeeds if `lease_refcounts` still has a live entry for + /// `session_id`, and [`InMemoryCredentialBroker::release_lease`] only + /// removes that entry (making the session eligible for revoke) while + /// holding the very same `lease_refcounts` lock. So once this call has + /// incremented the count, a concurrent `release_lease` on the + /// last-outstanding prior lease cannot have already revoked the session + /// — the two operations serialize on `lease_refcounts` instead of racing + /// across separate lock acquisitions. Validating the *other* way + /// (validate, then increment) would leave a window where a concurrent + /// revoke could land in between, handing back a lease for an + /// already-dead session. + /// + /// If the count was joined but the session turns out to be expired or + /// use-exhausted, the just-acquired reference is released again (via + /// [`InMemoryCredentialBroker::release_lease`], which revokes if that + /// makes this the last reference) so this method never leaves a + /// dangling refcount behind on its own failure path. + fn try_join_live_lease( + &self, + session_id: CredentialSessionId, + now: chrono::DateTime, + ) -> Result { + let joined = { + let mut counts = lock_or_recover(&self.lease_refcounts); + match counts.get_mut(&session_id) { + Some(count) => { + *count += 1; + true + } + None => false, + } + }; + if !joined { + return Ok(false); + } + match self.validate_session(session_id, now) { + Ok(_) => Ok(true), + Err(error) => { + self.release_lease(session_id); + match error { + CredentialBrokerError::UnknownSession { .. } + | CredentialBrokerError::SessionExpired { .. } + | CredentialBrokerError::SessionUseLimitExceeded { .. } => Ok(false), + other => Err(other), + } + } + } + } + /// Finds a live session currently bound to `placeholder`, if any. /// /// Returns `Ok(None)` — never an error — when the placeholder has no @@ -387,17 +475,18 @@ impl InMemoryCredentialBroker { lock_or_recover(&self.lease_refcounts).remove(&session_id); } - /// Registers one outstanding lease reference for `session_id`. See - /// [`InMemoryCredentialBroker::release_lease`] for the matching release - /// half of this pair. - fn acquire_lease_refcount( - &self, - session_id: CredentialSessionId, - ) -> Result<(), CredentialBrokerError> { + /// Registers one outstanding lease reference for a freshly-minted + /// `session_id`. See [`InMemoryCredentialBroker::release_lease`] for the + /// matching release half of this pair, and + /// [`InMemoryCredentialBroker::try_join_live_lease`] for the + /// increment-then-validate variant used when *reusing* an + /// already-published session id (this fresh-mint variant can't fail — + /// `lock_or_recover` never leaves poison unresolved — so unlike + /// `try_join_live_lease` it returns no `Result`). + fn acquire_lease_refcount(&self, session_id: CredentialSessionId) { *lock_or_recover(&self.lease_refcounts) .entry(session_id) .or_insert(0) += 1; - Ok(()) } /// Releases one outstanding lease reference for `session_id`, revoking @@ -515,7 +604,7 @@ mod tests { use super::{CredentialPlaceholderRegistry, CredentialPlaceholderToken}; - fn scope(tenant: &str, user: &str) -> ResourceScope { + fn sample_scope(tenant: &str, user: &str) -> ResourceScope { ResourceScope { tenant_id: TenantId::new(tenant).unwrap(), user_id: UserId::new(user).unwrap(), @@ -527,7 +616,7 @@ mod tests { } } - fn account( + fn sample_account( scope: ResourceScope, id: CredentialAccountId, handle: SecretHandle, @@ -551,7 +640,7 @@ mod tests { } } - fn request( + fn session_request( scope: ResourceScope, account_id: CredentialAccountId, url: &str, @@ -615,7 +704,9 @@ mod tests { #[test] fn placeholder_token_requires_fixed_prefix() { - assert!(CredentialPlaceholderToken::parse("icsbx_abc").is_ok()); + assert!( + CredentialPlaceholderToken::parse("icsbx_0123456789abcdef0123456789abcdef").is_ok() + ); let err = CredentialPlaceholderToken::parse("not_a_placeholder").unwrap_err(); assert!(matches!( err, @@ -623,6 +714,20 @@ mod tests { )); } + #[test] + fn placeholder_token_parse_rejects_malformed_suffix() { + // Empty string: no prefix at all. + assert!(CredentialPlaceholderToken::parse("").is_err()); + // Bare prefix with no suffix carries no identifying material. + assert!(CredentialPlaceholderToken::parse("icsbx_").is_err()); + // Suffix shorter than the registry ever produces. + assert!(CredentialPlaceholderToken::parse("icsbx_ab").is_err()); + // Non-alphanumeric suffix characters (control chars / shell + // metacharacters) must fail closed rather than being accepted and + // potentially propagated into a log line or env var downstream. + assert!(CredentialPlaceholderToken::parse("icsbx_0123456789abcdef\n; rm -rf /").is_err()); + } + #[test] fn placeholder_with_no_live_session_grants_nothing() { let broker = Arc::new(InMemoryCredentialBroker::new()); @@ -633,7 +738,7 @@ mod tests { let token = registry.get_or_create(&tenant, &user, &provider).unwrap(); let found = broker - .find_session_by_placeholder(&token, &scope("tenant-a", "user-a")) + .find_session_by_placeholder(&token, &sample_scope("tenant-a", "user-a")) .unwrap(); assert!(found.is_none()); } @@ -647,10 +752,10 @@ mod tests { let provider = ExtensionId::new("google").unwrap(); let token = registry.get_or_create(&tenant, &user, &provider).unwrap(); - let caller_scope = scope("tenant-a", "user-a"); + let caller_scope = sample_scope("tenant-a", "user-a"); let account_id = CredentialAccountId::new("google_prod").unwrap(); broker - .put_account(account( + .put_account(sample_account( caller_scope.clone(), account_id.clone(), SecretHandle::new("google_key").unwrap(), @@ -660,7 +765,7 @@ mod tests { // Out-of-policy request (wrong path) must be rejected, not minted. let rejected = broker.mint_on_first_use( &token, - request( + session_request( caller_scope.clone(), account_id.clone(), "https://api.example.com/v2/x", @@ -681,7 +786,7 @@ mod tests { let lease = broker .mint_on_first_use( &token, - request( + session_request( caller_scope.clone(), account_id, "https://api.example.com/v1/x", @@ -705,10 +810,10 @@ mod tests { let user_a = UserId::new("user-a").unwrap(); let token_a = registry.get_or_create(&tenant, &user_a, &provider).unwrap(); - let scope_a = scope("tenant-a", "user-a"); + let scope_a = sample_scope("tenant-a", "user-a"); let account_id = CredentialAccountId::new("google_prod").unwrap(); broker - .put_account(account( + .put_account(sample_account( scope_a.clone(), account_id.clone(), SecretHandle::new("google_key").unwrap(), @@ -717,12 +822,12 @@ mod tests { let lease = broker .mint_on_first_use( &token_a, - request(scope_a, account_id, "https://api.example.com/v1/x"), + session_request(scope_a, account_id, "https://api.example.com/v1/x"), ) .unwrap(); // User B's scope must never see user A's session through the placeholder. - let other_scope = scope("tenant-a", "user-b"); + let other_scope = sample_scope("tenant-a", "user-b"); let found = broker .find_session_by_placeholder(&token_a, &other_scope) .unwrap(); @@ -739,7 +844,7 @@ mod tests { let lease = broker .mint_on_first_use( &token, - request(scope_a, account_id, "https://api.example.com/v1/x"), + session_request(scope_a, account_id, "https://api.example.com/v1/x"), ) .unwrap(); let session_id = lease.session_id(); @@ -759,7 +864,7 @@ mod tests { let lease = broker .mint_on_first_use( &token, - request(scope_a, account_id, "https://api.example.com/v1/x"), + session_request(scope_a, account_id, "https://api.example.com/v1/x"), ) .unwrap(); let session_id = lease.session_id(); @@ -783,7 +888,7 @@ mod tests { let lease = broker .mint_on_first_use( &token, - request(scope_a, account_id, "https://api.example.com/v1/x"), + session_request(scope_a, account_id, "https://api.example.com/v1/x"), ) .unwrap(); let session_id = lease.session_id(); @@ -810,7 +915,7 @@ mod tests { let lease = broker .mint_on_first_use( &token, - request(scope_a, account_id, "https://api.example.com/v1/x"), + session_request(scope_a, account_id, "https://api.example.com/v1/x"), ) .unwrap(); let session_id = lease.session_id(); @@ -842,7 +947,7 @@ mod tests { let first_lease = broker .mint_on_first_use( &token, - request( + session_request( scope_a.clone(), account_id.clone(), "https://api.example.com/v1/x", @@ -852,7 +957,7 @@ mod tests { let second_lease = broker .mint_on_first_use( &token, - request(scope_a, account_id, "https://api.example.com/v1/x"), + session_request(scope_a, account_id, "https://api.example.com/v1/x"), ) .unwrap(); assert_eq!( @@ -888,7 +993,7 @@ mod tests { let lease = broker .mint_on_first_use( &token, - request(scope_a.clone(), account_id, "https://api.example.com/v1/x"), + session_request(scope_a.clone(), account_id, "https://api.example.com/v1/x"), ) .unwrap(); assert!( @@ -909,6 +1014,126 @@ mod tests { ); } + #[test] + fn find_session_by_placeholder_iterates_multiple_distinct_sessions_for_same_placeholder() { + // A placeholder can legitimately have more than one *distinct* + // session bound to it at once (e.g. two different accounts under + // one provider) — not just the same session reused, which is all + // the refcount tests above exercise. This pins that the + // scope-matching loop in `find_session_by_placeholder` actually + // walks past a non-matching candidate to find the right one. + let broker = Arc::new(InMemoryCredentialBroker::new()); + let registry = CredentialPlaceholderRegistry::new(); + let tenant = TenantId::new("tenant-a").unwrap(); + let provider = ExtensionId::new("google").unwrap(); + let user_a = UserId::new("user-multi-a").unwrap(); + let token = registry.get_or_create(&tenant, &user_a, &provider).unwrap(); + + let scope_a = sample_scope("tenant-a", "user-multi-a"); + let scope_b = sample_scope("tenant-a", "user-multi-b"); + let account_a = CredentialAccountId::new("google_account_a").unwrap(); + let account_b = CredentialAccountId::new("google_account_b").unwrap(); + broker + .put_account(sample_account( + scope_a.clone(), + account_a.clone(), + SecretHandle::new("key_a").unwrap(), + )) + .unwrap(); + broker + .put_account(sample_account( + scope_b.clone(), + account_b.clone(), + SecretHandle::new("key_b").unwrap(), + )) + .unwrap(); + + // Two distinct (invocation, capability, account) bindings mint two + // distinct sessions, both bound to the same placeholder token. + let lease_a = broker + .mint_on_first_use( + &token, + session_request(scope_a.clone(), account_a, "https://api.example.com/v1/x"), + ) + .unwrap(); + let lease_b = broker + .mint_on_first_use( + &token, + session_request(scope_b.clone(), account_b, "https://api.example.com/v1/x"), + ) + .unwrap(); + assert_ne!( + lease_a.session_id(), + lease_b.session_id(), + "two different accounts must mint two distinct sessions" + ); + + // Regardless of HashSet iteration order, querying with a specific + // scope must find that scope's session, not the other one bound to + // the same placeholder. + let found_b = broker + .find_session_by_placeholder(&token, &scope_b) + .unwrap() + .expect("scope_b's session must be found among the placeholder's bound sessions"); + assert_eq!(found_b.correlation_id(), lease_b.session_id()); + + let found_a = broker + .find_session_by_placeholder(&token, &scope_a) + .unwrap() + .expect("scope_a's session must be found among the placeholder's bound sessions"); + assert_eq!(found_a.correlation_id(), lease_a.session_id()); + + lease_a.revoke(); + lease_b.revoke(); + } + + #[test] + fn mint_on_first_use_remints_when_prior_jit_session_is_expired() { + // The jit_minted cache remembers a session id per (invocation, + // capability, account) key so re-use doesn't mint twice — but only + // while that session is still live. This pins the fallthrough + // branch: a cached key whose session has since expired must not be + // handed back; a fresh session must be minted instead. + let broker = Arc::new(InMemoryCredentialBroker::new()); + let registry = CredentialPlaceholderRegistry::new(); + let (token, scope_a, account_id) = seeded(&broker, ®istry, "user-remint"); + + let mut first_request = session_request( + scope_a.clone(), + account_id.clone(), + "https://api.example.com/v1/x", + ); + first_request.expires_at = Some(Utc::now() - chrono::Duration::seconds(1)); + let first_lease = broker.mint_on_first_use(&token, first_request).unwrap(); + let first_session_id = first_lease.session_id(); + assert!( + broker + .validate_session(first_session_id, Utc::now()) + .is_err(), + "sanity: the first session must already be expired" + ); + + // The jit_minted cache entry still points at the now-expired + // session; a second call for the identical key must not hand back a + // lease for the dead session — it must remint. + let second_request = session_request(scope_a, account_id, "https://api.example.com/v1/x"); + let second_lease = broker.mint_on_first_use(&token, second_request).unwrap(); + assert_ne!( + first_session_id, + second_lease.session_id(), + "an expired jit-cached session must not be reused" + ); + assert!( + broker + .validate_session(second_lease.session_id(), Utc::now()) + .is_ok(), + "the reminted session must be live" + ); + + first_lease.revoke(); + second_lease.revoke(); + } + fn seeded( broker: &Arc, registry: &CredentialPlaceholderRegistry, @@ -924,10 +1149,10 @@ mod tests { let token = registry .get_or_create(&tenant, &user_id, &provider) .unwrap(); - let caller_scope = scope("tenant-a", user); + let caller_scope = sample_scope("tenant-a", user); let account_id = CredentialAccountId::new("google_prod").unwrap(); broker - .put_account(account( + .put_account(sample_account( caller_scope.clone(), account_id.clone(), SecretHandle::new("google_key").unwrap(), From c5c58d076d2f9919daa9fbe649148b13e80ce31f Mon Sep 17 00:00:00 2001 From: Henry Park Date: Sun, 26 Jul 2026 19:15:02 -0700 Subject: [PATCH 05/14] fix(secrets): stop echoing raw placeholder tokens and cap parsed length MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit InvalidPlaceholderToken interpolated the raw, untrusted `value` verbatim into its Display impl, so a rejected sandbox-supplied token — which can carry control characters, ANSI escapes, or a real secret pasted into the wrong slot — propagated into every log line derived from the error. `reason` already carries all actionable information, so drop `value` entirely rather than sanitizing it (per the repo's boundary-mapping guideline for credential-adjacent errors, which wins over matching the sibling `InvalidAccountId` convention here since this value is attacker-controlled). Also cap the parsed token length: validation only enforced a minimum suffix length, so an arbitrarily long `icsbx_...` string handed back from the sandbox was accepted and then hashed/cloned/inserted into maps - unbounded work driven by untrusted input. Registry-issued tokens are always exactly 32 alphanumeric characters (a UUIDv4 `simple()` suffix), and nothing needs a variable-length suffix, so validation now requires that exact length instead of "at least 16". The length constant is exposed publicly (CREDENTIAL_PLACEHOLDER_SUFFIX_LEN) so the leak detector's cross-crate pinning test can assert against it instead of a bare literal (see the companion ironclaw_safety commit). Co-Authored-By: Claude Opus 5 (1M context) --- crates/ironclaw_secrets/src/lib.rs | 9 ++-- crates/ironclaw_secrets/src/placeholder.rs | 58 +++++++++++++++++----- 2 files changed, 49 insertions(+), 18 deletions(-) diff --git a/crates/ironclaw_secrets/src/lib.rs b/crates/ironclaw_secrets/src/lib.rs index b3609a23a1b..0d25e534025 100644 --- a/crates/ironclaw_secrets/src/lib.rs +++ b/crates/ironclaw_secrets/src/lib.rs @@ -15,8 +15,8 @@ mod placeholder; mod secret_store; pub use placeholder::{ - CREDENTIAL_PLACEHOLDER_PREFIX, CredentialPlaceholderOwner, CredentialPlaceholderRegistry, - CredentialPlaceholderToken, CredentialSessionLease, + CREDENTIAL_PLACEHOLDER_PREFIX, CREDENTIAL_PLACEHOLDER_SUFFIX_LEN, CredentialPlaceholderOwner, + CredentialPlaceholderRegistry, CredentialPlaceholderToken, CredentialSessionLease, }; pub use secret_store::{CredentialBroker, SecretStore}; @@ -470,8 +470,8 @@ pub enum CredentialBrokerError { CredentialExtensionMismatch { account_id: CredentialAccountId }, #[error("credential account {account_id} is not allowed for requested target")] CredentialPolicyMismatch { account_id: CredentialAccountId }, - #[error("credential placeholder token {value} is invalid: {reason}")] - InvalidPlaceholderToken { value: String, reason: String }, + #[error("credential placeholder token is invalid: {reason}")] + InvalidPlaceholderToken { reason: String }, } impl CredentialBrokerError { @@ -1245,7 +1245,6 @@ mod tests { ); assert_eq!( CredentialBrokerError::InvalidPlaceholderToken { - value: "bad".to_string(), reason: "must start with 'icsbx_'".to_string(), } .stable_reason(), diff --git a/crates/ironclaw_secrets/src/placeholder.rs b/crates/ironclaw_secrets/src/placeholder.rs index da27d8a0b0e..efbfaa891e1 100644 --- a/crates/ironclaw_secrets/src/placeholder.rs +++ b/crates/ironclaw_secrets/src/placeholder.rs @@ -49,6 +49,26 @@ use crate::{ /// update the regex there too. pub const CREDENTIAL_PLACEHOLDER_PREFIX: &str = "icsbx_"; +/// Required length of the suffix after [`CREDENTIAL_PLACEHOLDER_PREFIX`]. +/// +/// Registry-issued tokens are always exactly this long — a UUIDv4 +/// `simple()` string (32 lowercase hex characters, see +/// [`CredentialPlaceholderToken::generate`]) — and nothing in this crate +/// legitimately needs a shorter or longer suffix. [`CredentialPlaceholderToken::parse`] +/// enforces this length exactly (not just a minimum) so that a sandbox +/// caller cannot hand back an arbitrarily long `icsbx_...` string and drive +/// unbounded hashing/cloning/map-insertion work on the host from unvalidated +/// input. +/// +/// Exposed publicly so `ironclaw_safety::leak_detector`'s +/// `sandbox_credential_placeholder` pattern — which independently matches +/// `icsbx_` plus 16+ alphanumeric characters — can pin its own minimum +/// against a token this crate's public API would actually accept, rather +/// than a hardcoded literal drifting silently out of sync. See +/// `sandbox_credential_placeholder_prefix_matches_registry` in +/// `ironclaw_safety/src/leak_detector.rs`. +pub const CREDENTIAL_PLACEHOLDER_SUFFIX_LEN: usize = 32; + /// Opaque, stable placeholder token for a `(tenant, user, provider)` triple. /// /// Deliberately **not** bearer-like: unlike [`CredentialSessionId`], a @@ -67,17 +87,9 @@ impl CredentialPlaceholderToken { )) } - /// Minimum length of the suffix after [`CREDENTIAL_PLACEHOLDER_PREFIX`], - /// matching the shortest suffix the `sandbox_credential_placeholder` leak - /// pattern (`ironclaw_safety::leak_detector`) will recognize and the - /// shape [`CredentialPlaceholderToken::generate`] actually produces (a - /// 32-character UUIDv4 `simple()` suffix). - const MIN_SUFFIX_LEN: usize = 16; - fn validate(value: &str) -> Result<(), CredentialBrokerError> { let Some(suffix) = value.strip_prefix(CREDENTIAL_PLACEHOLDER_PREFIX) else { return Err(CredentialBrokerError::InvalidPlaceholderToken { - value: value.to_string(), reason: format!("must start with '{CREDENTIAL_PLACEHOLDER_PREFIX}'"), }); }; @@ -89,16 +101,23 @@ impl CredentialPlaceholderToken { // characters or shell metacharacters an attacker-controlled sandbox // side might send back through `parse` — must fail closed here // instead of propagating downstream. - if suffix.len() < Self::MIN_SUFFIX_LEN + // + // The length check is an exact match, not just a minimum: registry- + // issued tokens are always exactly `CREDENTIAL_PLACEHOLDER_SUFFIX_LEN` + // characters (see that constant's doc comment), and nothing here + // legitimately needs a variable-length suffix. Accepting an + // arbitrarily long `icsbx_...` string from the sandbox would let + // untrusted input drive unbounded hashing/cloning/map-insertion work + // on the host. + if suffix.len() != CREDENTIAL_PLACEHOLDER_SUFFIX_LEN || !suffix .chars() .all(|character| character.is_ascii_alphanumeric()) { return Err(CredentialBrokerError::InvalidPlaceholderToken { - value: value.to_string(), reason: format!( - "must be '{CREDENTIAL_PLACEHOLDER_PREFIX}' followed by at least {} ASCII alphanumeric characters", - Self::MIN_SUFFIX_LEN + "must be '{CREDENTIAL_PLACEHOLDER_PREFIX}' followed by exactly {} ASCII alphanumeric characters", + CREDENTIAL_PLACEHOLDER_SUFFIX_LEN ), }); } @@ -602,7 +621,10 @@ mod tests { InMemoryCredentialBroker, RedactedJson, }; - use super::{CredentialPlaceholderRegistry, CredentialPlaceholderToken}; + use super::{ + CREDENTIAL_PLACEHOLDER_SUFFIX_LEN, CredentialPlaceholderRegistry, + CredentialPlaceholderToken, + }; fn sample_scope(tenant: &str, user: &str) -> ResourceScope { ResourceScope { @@ -726,6 +748,16 @@ mod tests { // metacharacters) must fail closed rather than being accepted and // potentially propagated into a log line or env var downstream. assert!(CredentialPlaceholderToken::parse("icsbx_0123456789abcdef\n; rm -rf /").is_err()); + // Suffix longer than the registry ever produces: an arbitrarily long + // `icsbx_...` string from the sandbox must be rejected, not accepted + // and driven through hashing/cloning/map-insertion downstream. + assert!( + CredentialPlaceholderToken::parse(format!( + "icsbx_{}", + "a".repeat(CREDENTIAL_PLACEHOLDER_SUFFIX_LEN + 1) + )) + .is_err() + ); } #[test] From a36423cabaee7e9f15c9743d51e5498ec2995782 Mon Sep 17 00:00:00 2001 From: Henry Park Date: Sun, 26 Jul 2026 19:15:18 -0700 Subject: [PATCH 06/14] fix(safety): close word-boundary bypass in placeholder leak pattern MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sandbox_credential_placeholder regex was \bicsbx_[A-Za-z0-9]{16,}\b. `_` is a word character, so \b does not fire next to it: a single leading or trailing byte (`_icsbx_...`, `icsbx_..._x`) defeated the one pattern standing between a leaked placeholder and model output/logs. Drop the word boundaries entirely — `icsbx_` plus 16+ alphanumerics is a distinctive shape with no realistic false-positive risk, and over-matching here fails safe. Extends the existing test rather than adding a parallel file: added leading-underscore, trailing-underscore, and leading-letter cases, and flipped the prior "substring of a longer word is NOT flagged" test to assert it IS flagged now, since that flip is the deliberate fail-safe tradeoff. Also pins the length half of the sandbox_credential_placeholder / ironclaw_secrets contract, not just the prefix: the cross-crate test now asserts CREDENTIAL_PLACEHOLDER_SUFFIX_LEN stays at or above the regex's own 16-char floor, and additionally constructs a minimum-shaped token through the registry's public parse() API and asserts the detector actually flags it - pinning behavior instead of a literal. Co-Authored-By: Claude Opus 5 (1M context) --- crates/ironclaw_safety/src/leak_detector.rs | 107 ++++++++++++++++++-- 1 file changed, 100 insertions(+), 7 deletions(-) diff --git a/crates/ironclaw_safety/src/leak_detector.rs b/crates/ironclaw_safety/src/leak_detector.rs index 9569407d189..076d98a4985 100644 --- a/crates/ironclaw_safety/src/leak_detector.rs +++ b/crates/ironclaw_safety/src/leak_detector.rs @@ -612,8 +612,17 @@ fn default_patterns() -> Vec { // of real secrets; the egress proxy swaps them for the real credential // at request time. A placeholder must never cross the trust boundary // into model output, logs, or transcripts, so it is treated like any - // other secret. Word boundaries keep this from matching `icsbx_` as a - // substring of a longer identifier. + // other secret. + // + // Deliberately NO `\b` word boundaries here: `_` is a word character, + // so a boundary assertion does not fire next to it, meaning a single + // leading or trailing character (`_icsbx_...`, `icsbx_..._x`) would + // otherwise slip past the one pattern standing between a placeholder + // and model output/logs. `icsbx_` plus 16+ alphanumerics is a + // distinctive shape that does not occur naturally, so a bare + // substring match carries no realistic false-positive risk — and + // over-matching here fails *safe*, whereas under-matching would not. + // Do not "helpfully" restore the word boundaries. // // The `icsbx_` literal here must stay in sync with // `ironclaw_secrets::placeholder::CREDENTIAL_PLACEHOLDER_PREFIX` @@ -625,7 +634,7 @@ fn default_patterns() -> Vec { // the two ever drift apart. LeakPattern { name: "sandbox_credential_placeholder".to_string(), - regex: Regex::new(r"\bicsbx_[A-Za-z0-9]{16,}\b").unwrap(), // safety: hardcoded literal + regex: Regex::new(r"icsbx_[A-Za-z0-9]{16,}").unwrap(), // safety: hardcoded literal severity: LeakSeverity::Critical, action: LeakAction::Block, }, @@ -1232,17 +1241,67 @@ mod tests { } #[test] - fn test_sandbox_credential_placeholder_substring_of_longer_word_passes() { + fn test_sandbox_credential_placeholder_substring_of_longer_word_is_flagged() { + // Deliberately flipped from "should not match" to "should match": + // the pattern has no `\b` word boundaries (see the comment on the + // pattern definition), so a single leading/trailing character next + // to `icsbx_` no longer defeats detection. Over-matching here is the + // intended fail-safe behavior — a leaked placeholder embedded in a + // longer identifier must still be caught. let detector = LeakDetector::new(); - // "icsbx_" embedded inside a longer identifier, not at a word boundary. let content = "myicsbx_7f3a9b2c1d4e5f60prefix"; let result = detector.scan(content); assert!( - !result + result + .matches + .iter() + .any(|m| m.pattern_name == "sandbox_credential_placeholder"), + "icsbx_ substring inside a longer word must still be flagged (fail-safe over-match)" + ); + } + + #[test] + fn test_sandbox_credential_placeholder_leading_underscore_is_flagged() { + // A single leading `_` used to defeat the old `\bicsbx_...\b` + // pattern outright, since `_` is a word character and `\b` does not + // fire next to it. + let detector = LeakDetector::new(); + let content = "_icsbx_0123456789abcdef0123456789abcdef"; + let result = detector.scan(content); + assert!( + result + .matches + .iter() + .any(|m| m.pattern_name == "sandbox_credential_placeholder"), + "leading underscore must not defeat placeholder detection" + ); + } + + #[test] + fn test_sandbox_credential_placeholder_trailing_underscore_is_flagged() { + let detector = LeakDetector::new(); + let content = "icsbx_0123456789abcdef0123456789abcdef_x"; + let result = detector.scan(content); + assert!( + result .matches .iter() .any(|m| m.pattern_name == "sandbox_credential_placeholder"), - "icsbx_ substring inside a longer word should not match" + "trailing underscore must not defeat placeholder detection" + ); + } + + #[test] + fn test_sandbox_credential_placeholder_leading_letter_is_flagged() { + let detector = LeakDetector::new(); + let content = "xicsbx_0123456789abcdef0123456789abcdef"; + let result = detector.scan(content); + assert!( + result + .matches + .iter() + .any(|m| m.pattern_name == "sandbox_credential_placeholder"), + "leading letter must not defeat placeholder detection" ); } @@ -1272,6 +1331,40 @@ mod tests { update both if this constant ever changes" ); + // Pin the length half of the shared contract too, not just the + // prefix: the regex requires 16+ alphanumeric characters after the + // prefix (`{16,}`), so the registry's own required suffix length must + // never drop below that floor, or shorter-but-valid placeholders + // would silently stop matching. + const { + assert!( + ironclaw_secrets::CREDENTIAL_PLACEHOLDER_SUFFIX_LEN >= 16, + "leak_detector's sandbox_credential_placeholder regex requires 16+ alphanumeric \ + characters after the prefix; the registry's required suffix length must stay at \ + or above that floor" + ); + } + + // Better than asserting a bare number: construct a minimum-shaped + // token through the registry's own public API (not just a literal + // matching today's expected length) and assert the detector actually + // flags it. This pins behavior, not a number. + let minimum_shaped_token = ironclaw_secrets::CredentialPlaceholderToken::parse(format!( + "{}{}", + ironclaw_secrets::CREDENTIAL_PLACEHOLDER_PREFIX, + "a".repeat(ironclaw_secrets::CREDENTIAL_PLACEHOLDER_SUFFIX_LEN) + )) + .expect("a suffix of exactly CREDENTIAL_PLACEHOLDER_SUFFIX_LEN alphanumeric characters must be accepted by the registry's own public API"); + let detector = LeakDetector::new(); + let result = detector.scan(&format!("leaked token: {minimum_shaped_token}")); + assert!( + result + .matches + .iter() + .any(|m| m.pattern_name == "sandbox_credential_placeholder"), + "a minimum-shaped, registry-accepted placeholder token must be caught by the leak detector" + ); + // Shape a registry-issued token actually has: the fixed prefix plus a // UUIDv4 `simple()` suffix (32 lowercase hex chars, no dashes) — see // `CredentialPlaceholderToken::generate()` in ironclaw_secrets. From d24b0528b16e05c97e39e1b586857f40a082e1ec Mon Sep 17 00:00:00 2001 From: Henry Park Date: Sun, 26 Jul 2026 19:30:08 -0700 Subject: [PATCH 07/14] fix(secrets): close concurrent-first-use race in JIT session minting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit mint_on_first_use looked up and recorded a JIT mint through jit_minted in two separate lock/unlock cycles, so two threads racing on the identical (invocation, capability, account) binding could both observe "nothing minted yet" and each mint their own session — defeating the one-session-per-binding guarantee and multiplying a max_uses: Some(1) budget. Hold jit_minted once across the whole lookup-or-mint sequence instead, and drop it before binding the result to a placeholder (that part only needs "at most one session per binding", not "at most one bind call"). This is the first nested-lock pattern in this module, so it documents an explicit lock-ordering rule (jit_minted outermost) and splits release_lease/revoke_session into jit_minted-touching and jit_minted-free variants: try_join_live_lease's failure path runs while mint_on_first_use already holds jit_minted, and its old cleanup route (via revoke_session) would have re-locked jit_minted on the same thread and deadlocked on the non-reentrant std::sync::Mutex. Adds a Barrier-aligned 32-thread regression test asserting every racing lease resolves to the same session id. Co-Authored-By: Claude Opus 5 (1M context) --- crates/ironclaw_secrets/src/placeholder.rs | 212 ++++++++++++++++----- 1 file changed, 164 insertions(+), 48 deletions(-) diff --git a/crates/ironclaw_secrets/src/placeholder.rs b/crates/ironclaw_secrets/src/placeholder.rs index efbfaa891e1..8bf1813ea09 100644 --- a/crates/ironclaw_secrets/src/placeholder.rs +++ b/crates/ironclaw_secrets/src/placeholder.rs @@ -343,6 +343,39 @@ impl InMemoryCredentialBroker { /// (see [`InMemoryCredentialBroker::release_lease`]), so an earlier /// caller finishing first can never pull the session out from under a /// later caller still using it. + /// + /// **Concurrency:** the lookup-or-mint sequence below (checking + /// `jit_minted` for an existing live session, and minting+publishing a + /// new one if there isn't one) runs under a single, held-once lock on + /// `jit_minted`. Two threads racing on an identical `JitMintKey` used to + /// be able to both observe "nothing minted yet" — `jit_minted_session_id` + /// and `record_jit_mint` each locked/read-or-wrote/unlocked `jit_minted` + /// *separately* — and both mint their own session, defeating the + /// one-session-per-binding guarantee this module's doc comment asserts. + /// Holding one coarse lock across the whole sequence closes that window; + /// see `mint_on_first_use_is_race_free_under_concurrent_first_use` below. + /// Binding the result to a placeholder (`finish_lease` / + /// `bind_placeholder_to_session`) happens *after* the lock is dropped — + /// the invariant being protected is "at most one session per binding", + /// not "at most one placeholder-bind call", so that part doesn't need to + /// be in the critical section. + /// + /// **Lock ordering invariant:** `jit_minted` is always the outermost lock + /// whenever held together with `accounts` / `sessions` / + /// `lease_refcounts` — no path may acquire `jit_minted` while already + /// holding one of those. This is the first place in this module that + /// holds `jit_minted` across calls into other locking helpers + /// (`try_join_live_lease`, `create_session`, `acquire_lease_refcount`), + /// so it is also the first place a self-deadlock is possible: on + /// `std::sync::Mutex` (non-reentrant), a helper that itself tried to lock + /// `jit_minted` while this method still held it would hang forever. + /// `try_join_live_lease`'s failure path used to reach `revoke_session` + /// (via `release_lease`), which prunes `jit_minted` — exactly that + /// hazard — so it now calls `release_lease_ignoring_jit_minted` instead, + /// which does the same cleanup minus the `jit_minted` prune (safe here + /// because this method is about to overwrite that same key's entry under + /// the lock it already holds). `create_session` and + /// `acquire_lease_refcount` never touch `jit_minted` at all. pub fn mint_on_first_use( self: &Arc, placeholder: &CredentialPlaceholderToken, @@ -354,20 +387,26 @@ impl InMemoryCredentialBroker { account_id: request.account_id.clone(), }; - if let Some(session_id) = self.jit_minted_session_id(&key)? + let mut jit_minted = lock_or_recover(&self.jit_minted); + + if let Some(&session_id) = jit_minted.get(&key) && self.try_join_live_lease(session_id, chrono::Utc::now())? { + drop(jit_minted); return self.finish_lease(placeholder, session_id); } let session = self.create_session(request)?; let session_id = session.correlation_id(); - self.record_jit_mint(key, session_id)?; + jit_minted.insert(key, session_id); // This session_id was just minted and is not yet published to - // `jit_minted`/`sessions_by_placeholder`, so no concurrent caller can - // reference it yet: registering the first lease reference here can't - // race a concurrent `release_lease` the way the reuse path above can. + // `sessions_by_placeholder`, and its `jit_minted` slot was just + // (re)written under the lock we are still holding, so no concurrent + // caller can reference it yet: registering the first lease reference + // here can't race a concurrent `release_lease` the way the reuse + // path above can. self.acquire_lease_refcount(session_id); + drop(jit_minted); self.finish_lease(placeholder, session_id) } @@ -405,9 +444,20 @@ impl InMemoryCredentialBroker { /// /// If the count was joined but the session turns out to be expired or /// use-exhausted, the just-acquired reference is released again (via - /// [`InMemoryCredentialBroker::release_lease`], which revokes if that - /// makes this the last reference) so this method never leaves a - /// dangling refcount behind on its own failure path. + /// [`InMemoryCredentialBroker::release_lease_ignoring_jit_minted`], which + /// revokes if that makes this the last reference) so this method never + /// leaves a dangling refcount behind on its own failure path. + /// + /// The release on the failure path deliberately skips pruning + /// `jit_minted` (unlike the general [`InMemoryCredentialBroker::release_lease`]): + /// this method is only ever called from + /// [`InMemoryCredentialBroker::mint_on_first_use`] while that method + /// already holds the `jit_minted` lock, so re-locking it here (as plain + /// `release_lease` would, via `revoke_session`) would deadlock on the + /// non-reentrant `std::sync::Mutex`. Skipping it doesn't leave anything + /// stale: `mint_on_first_use` overwrites this exact key's `jit_minted` + /// entry with the freshly minted session id immediately afterward, under + /// the same lock. fn try_join_live_lease( &self, session_id: CredentialSessionId, @@ -429,7 +479,7 @@ impl InMemoryCredentialBroker { match self.validate_session(session_id, now) { Ok(_) => Ok(true), Err(error) => { - self.release_lease(session_id); + self.release_lease_ignoring_jit_minted(session_id); match error { CredentialBrokerError::UnknownSession { .. } | CredentialBrokerError::SessionExpired { .. } @@ -484,9 +534,20 @@ impl InMemoryCredentialBroker { /// does not accumulate stale entries for every invocation/binding that /// has ever existed. pub fn revoke_session(&self, session_id: CredentialSessionId) { - lock_or_recover(&self.sessions).remove(&session_id); lock_or_recover(&self.jit_minted) .retain(|_, minted_session_id| *minted_session_id != session_id); + self.revoke_session_ignoring_jit_minted(session_id); + } + + /// Same cleanup as [`InMemoryCredentialBroker::revoke_session`] minus the + /// `jit_minted` prune. Split out so + /// [`InMemoryCredentialBroker::release_lease_ignoring_jit_minted`] (and, + /// through it, `try_join_live_lease`'s failure path) can revoke a stale + /// session without acquiring `jit_minted` a second time on a thread that + /// may already hold it — see the lock-ordering note on + /// [`InMemoryCredentialBroker::mint_on_first_use`]. + fn revoke_session_ignoring_jit_minted(&self, session_id: CredentialSessionId) { + lock_or_recover(&self.sessions).remove(&session_id); lock_or_recover(&self.sessions_by_placeholder).retain(|_, session_ids| { session_ids.remove(&session_id); !session_ids.is_empty() @@ -517,50 +578,41 @@ impl InMemoryCredentialBroker { /// their still-live session revoked out from under them by an earlier /// caller finishing first. pub(crate) fn release_lease(&self, session_id: CredentialSessionId) { - let should_revoke = { - let mut counts = lock_or_recover(&self.lease_refcounts); - match counts.get_mut(&session_id) { - Some(count) if *count > 1 => { - *count -= 1; - false - } - _ => { - counts.remove(&session_id); - true - } - } - }; - if should_revoke { + if Self::release_lease_refcount(&self.lease_refcounts, session_id) { self.revoke_session(session_id); } } - fn jit_minted_session_id( - &self, - key: &JitMintKey, - ) -> Result, CredentialBrokerError> { - Ok(self - .jit_minted - .lock() - .map_err(|error| CredentialBrokerError::BrokerUnavailable { - reason: error.to_string(), - })? - .get(key) - .copied()) + /// Same as [`InMemoryCredentialBroker::release_lease`], but revokes + /// through [`InMemoryCredentialBroker::revoke_session_ignoring_jit_minted`] + /// instead of [`InMemoryCredentialBroker::revoke_session`] — i.e. it never + /// acquires `jit_minted`. Used only by `try_join_live_lease`'s failure + /// path, which always runs while `mint_on_first_use` already holds + /// `jit_minted`; see the lock-ordering note there. + fn release_lease_ignoring_jit_minted(&self, session_id: CredentialSessionId) { + if Self::release_lease_refcount(&self.lease_refcounts, session_id) { + self.revoke_session_ignoring_jit_minted(session_id); + } } - fn record_jit_mint( - &self, - key: JitMintKey, + /// Decrements (or removes) the outstanding-lease refcount entry for + /// `session_id`, returning whether that made this the last reference + /// (i.e. whether the caller should now actually revoke the session). + fn release_lease_refcount( + lease_refcounts: &Mutex>, session_id: CredentialSessionId, - ) -> Result<(), CredentialBrokerError> { - self.jit_minted - .lock() - .map_err(|error| CredentialBrokerError::BrokerUnavailable { - reason: error.to_string(), - })? - .insert(key, session_id); - Ok(()) + ) -> bool { + let mut counts = lock_or_recover(lease_refcounts); + match counts.get_mut(&session_id) { + Some(count) if *count > 1 => { + *count -= 1; + false + } + _ => { + counts.remove(&session_id); + true + } + } } fn bind_placeholder_to_session( @@ -623,7 +675,7 @@ mod tests { use super::{ CREDENTIAL_PLACEHOLDER_SUFFIX_LEN, CredentialPlaceholderRegistry, - CredentialPlaceholderToken, + CredentialPlaceholderToken, CredentialSessionLease, }; fn sample_scope(tenant: &str, user: &str) -> ResourceScope { @@ -1166,6 +1218,70 @@ mod tests { second_lease.revoke(); } + #[test] + fn mint_on_first_use_is_race_free_under_concurrent_first_use() { + // Regression test for a confirmed race: `jit_minted_session_id` + // (read) and `record_jit_mint` (write) used to lock/unlock + // `jit_minted` *separately*, so two threads racing on the identical + // `JitMintKey` could both observe "nothing minted yet" and both mint + // their own session — defeating the one-session-per-binding + // guarantee this module's doc comment asserts, and multiplying a + // `max_uses: Some(1)` budget. The fix holds one lock across the + // whole lookup-or-mint sequence in `mint_on_first_use`. + // + // This is only probabilistic at *catching* a reintroduced race — a + // `Barrier`-aligned start makes the race window likely to be hit, + // not guaranteed. But it has zero false-fail risk once the fix is + // in place: the lock enforces exclusion structurally, so a passing + // run here is a real guarantee, not a lucky one. + const THREAD_COUNT: usize = 32; + + let broker = Arc::new(InMemoryCredentialBroker::new()); + let registry = CredentialPlaceholderRegistry::new(); + let (token, scope_a, account_id) = seeded(&broker, ®istry, "user-race"); + let barrier = Arc::new(std::sync::Barrier::new(THREAD_COUNT)); + + let handles: Vec<_> = (0..THREAD_COUNT) + .map(|_| { + let broker = Arc::clone(&broker); + let token = token.clone(); + let scope_a = scope_a.clone(); + let account_id = account_id.clone(); + let barrier = Arc::clone(&barrier); + std::thread::spawn(move || { + let request = + session_request(scope_a, account_id, "https://api.example.com/v1/x"); + barrier.wait(); + broker + .mint_on_first_use(&token, request) + .expect("mint_on_first_use must succeed for every racing thread") + }) + }) + .collect(); + + let leases: Vec = handles + .into_iter() + .map(|handle| { + handle + .join() + .expect("mint_on_first_use thread must not panic") + }) + .collect(); + + let distinct_session_ids: std::collections::HashSet<_> = + leases.iter().map(|lease| lease.session_id()).collect(); + assert_eq!( + distinct_session_ids.len(), + 1, + "every thread racing on the identical (invocation, capability, account) binding \ + must be handed a lease for the exact same session, not one each" + ); + + for lease in leases { + lease.revoke(); + } + } + fn seeded( broker: &Arc, registry: &CredentialPlaceholderRegistry, From 7b398003c76a7a168a87e4cedab51b27bcd1e0ef Mon Sep 17 00:00:00 2001 From: Henry Park Date: Sun, 26 Jul 2026 19:30:16 -0700 Subject: [PATCH 08/14] fix(secrets): cap credential session expiry at creation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CredentialSessionRequest.expires_at was a plain Option, and create_session stored it unchanged, so None meant a genuinely unbounded session. Combined with revocation that depends on Drop running, a lease held or leaked forever was a standing grant for the process lifetime. Default a None expiry to CREDENTIAL_SESSION_DEFAULT_TTL_SECONDS out and clamp anything longer than CREDENTIAL_SESSION_MAX_TTL_SECONDS (30 minutes each, per the project's already-decided design: explicit revoke stays primary, this is only the backstop). Enforcement is the existing lazy check in validate_session; no new enforcement path. Deliberately no background sweep task: proactive eviction would be standing infrastructure for a path with zero live callers today, and this crate owns no runtime to host it on — deferred to whichever later PR gives this store an owning runtime (noted in the code). create_session has no callers outside this crate's own tests today (egress proxy not built yet), so the blast radius is contained here. Co-Authored-By: Claude Opus 5 (1M context) --- crates/ironclaw_secrets/src/lib.rs | 123 ++++++++++++++++++++++++++++- 1 file changed, 119 insertions(+), 4 deletions(-) diff --git a/crates/ironclaw_secrets/src/lib.rs b/crates/ironclaw_secrets/src/lib.rs index 0d25e534025..362368fe7f2 100644 --- a/crates/ironclaw_secrets/src/lib.rs +++ b/crates/ironclaw_secrets/src/lib.rs @@ -43,6 +43,22 @@ use uuid::Uuid; const CREDENTIAL_ID_MAX_LEN: usize = 128; const DEFAULT_SECRET_LEASE_TTL_SECONDS: i64 = 300; +/// Default lifetime handed to a [`CredentialSession`] whose +/// [`CredentialSessionRequest::expires_at`] is `None`. A caller that asks for +/// "no expiry" still gets a bounded session — see +/// [`CREDENTIAL_SESSION_MAX_TTL_SECONDS`] for why unbounded is never actually +/// granted. +const CREDENTIAL_SESSION_DEFAULT_TTL_SECONDS: i64 = 30 * 60; + +/// Hard cap on how far in the future any [`CredentialSession::expires_at`] +/// may be set, regardless of what the caller requests. Explicit revocation +/// (via [`CredentialSessionLease`]/[`InMemoryCredentialBroker::revoke_session`]) +/// is the primary way a session's life ends; this is only the backstop for a +/// lease held or leaked past the caller's own error/timeout/panic handling — +/// see `create_session`'s doc comment for why a background sweep task is not +/// the fix here. +const CREDENTIAL_SESSION_MAX_TTL_SECONDS: i64 = 30 * 60; + /// Opaque identifier for a one-shot secret lease. #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)] #[serde(transparent)] @@ -648,6 +664,32 @@ impl InMemoryCredentialBroker { Ok(()) } + /// Mints a [`CredentialSession`] for `request`, capping its expiry so a + /// session can never be genuinely unbounded. + /// + /// `request.expires_at` and `request.max_uses` are both `Option`, and a + /// caller (in practice, `mint_on_first_use`'s upstream capability + /// dispatch) can legitimately pass `expires_at: None` meaning "I have no + /// opinion" rather than "never expire". Combined with revocation that + /// depends on `Drop` running (see [`CredentialSessionLease`]), a lease + /// held or leaked forever — a caller that never awaits its future to + /// completion or cancellation, for instance — would otherwise be a + /// standing grant for the process lifetime. So `expires_at` is always + /// defaulted-and-capped here: `None` becomes + /// [`CREDENTIAL_SESSION_DEFAULT_TTL_SECONDS`] out, and anything longer + /// than [`CREDENTIAL_SESSION_MAX_TTL_SECONDS`] (including an explicit + /// request for longer) is clamped down to it. Enforcement of the + /// resulting expiry is the existing lazy check in + /// `ensure_credential_session_record_usable` / `validate_session` — this + /// only ever narrows the stored value, it adds no new enforcement path. + /// + /// Deliberately **not** paired with a background sweep task: proactive + /// eviction of expired-but-unrevoked sessions would be standing + /// infrastructure (a spawn point, shutdown handling, interval tuning) for + /// a path with zero live callers today, and this crate owns no runtime to + /// host it on. Explicit revoke stays the primary mechanism; this cap is + /// only the backstop. Proactive eviction is deferred to whichever later + /// PR gives this store an owning runtime. pub fn create_session( &self, request: CredentialSessionRequest, @@ -705,6 +747,14 @@ impl InMemoryCredentialBroker { account_id: request.account_id, }); } + let now = chrono::Utc::now(); + let default_expires_at = + now + chrono::Duration::seconds(CREDENTIAL_SESSION_DEFAULT_TTL_SECONDS); + let max_expires_at = now + chrono::Duration::seconds(CREDENTIAL_SESSION_MAX_TTL_SECONDS); + let expires_at = request + .expires_at + .unwrap_or(default_expires_at) + .min(max_expires_at); let session = CredentialSession { scope: request.scope, invocation_id: request.invocation_id, @@ -713,7 +763,7 @@ impl InMemoryCredentialBroker { account_id: account.id.clone(), secret_handles: account.secret_handles.clone(), allowed_targets: account.allowed_targets.clone(), - expires_at: request.expires_at, + expires_at: Some(expires_at), max_uses: request.max_uses, correlation_id: CredentialSessionId::new(), }; @@ -1094,9 +1144,11 @@ mod tests { use serde_json::json; use crate::{ - CREDENTIAL_ID_MAX_LEN, CredentialAccount, CredentialAccountId, CredentialAccountStatus, - CredentialBrokerError, CredentialPathPolicy, CredentialSessionId, CredentialSessionRequest, - CredentialTargetPolicy, InMemoryCredentialBroker, RedactedJson, SecretStoreError, + CREDENTIAL_ID_MAX_LEN, CREDENTIAL_SESSION_DEFAULT_TTL_SECONDS, + CREDENTIAL_SESSION_MAX_TTL_SECONDS, CredentialAccount, CredentialAccountId, + CredentialAccountStatus, CredentialBrokerError, CredentialPathPolicy, CredentialSessionId, + CredentialSessionRequest, CredentialTargetPolicy, InMemoryCredentialBroker, RedactedJson, + SecretStoreError, }; #[test] @@ -1297,6 +1349,69 @@ mod tests { assert!(debug.contains("CredentialSessionId([REDACTED])")); } + #[test] + fn create_session_defaults_and_caps_expires_at() { + // A request with no opinion on expiry (`None`) must not come back + // genuinely unbounded — it gets the default TTL. A request asking + // for longer than the hard cap must be clamped down to the cap + // rather than honored as requested. Both defend the same invariant: + // a leaked/held-forever lease can never be a standing grant for the + // process lifetime (see `create_session`'s doc comment). + let broker = InMemoryCredentialBroker::new(); + let scope = sample_scope("tenant-a", "user-a"); + let account_id = CredentialAccountId::new("openai_prod").unwrap(); + broker + .put_account(sample_account( + scope.clone(), + account_id.clone(), + SecretHandle::new("openai_key").unwrap(), + )) + .unwrap(); + + let before = Utc::now(); + let defaulted = broker + .create_session(CredentialSessionRequest { + expires_at: None, + ..session_request( + scope.clone(), + account_id.clone(), + "https://api.example.com/v1/models", + ) + }) + .unwrap(); + let after = Utc::now(); + let defaulted_expiry = defaulted + .expires_at() + .expect("None must be defaulted, not left unbounded"); + assert!( + defaulted_expiry + >= before + chrono::Duration::seconds(CREDENTIAL_SESSION_DEFAULT_TTL_SECONDS) + && defaulted_expiry + <= after + chrono::Duration::seconds(CREDENTIAL_SESSION_DEFAULT_TTL_SECONDS), + "expires_at: None must default to ~{CREDENTIAL_SESSION_DEFAULT_TTL_SECONDS}s out, got {defaulted_expiry}" + ); + + let requested_far_future = Utc::now() + chrono::Duration::days(365); + let capped = broker + .create_session(CredentialSessionRequest { + expires_at: Some(requested_far_future), + ..session_request(scope, account_id, "https://api.example.com/v1/models") + }) + .unwrap(); + let capped_expiry = capped + .expires_at() + .expect("capped session must still carry an expiry"); + assert!( + capped_expiry < requested_far_future, + "a request for a 1-year expiry must be clamped down to the cap, got {capped_expiry}" + ); + assert!( + capped_expiry + <= Utc::now() + chrono::Duration::seconds(CREDENTIAL_SESSION_MAX_TTL_SECONDS), + "clamped expiry must not exceed the {CREDENTIAL_SESSION_MAX_TTL_SECONDS}s cap, got {capped_expiry}" + ); + } + #[test] fn credential_session_validation_enforces_expiry_and_use_limits() { let broker = InMemoryCredentialBroker::new(); From 4ff93e99f4a356dcbcb0dd9d1af5c5fd6815f4c5 Mon Sep 17 00:00:00 2001 From: Henry Park Date: Sun, 26 Jul 2026 19:44:30 -0700 Subject: [PATCH 09/14] fix(secrets): close standing-grant leak when placeholder bind fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit finish_lease constructed the CredentialSessionLease only after the fallible bind_placeholder_to_session call, so a bind failure propagated `?` with the session already refcounted by the caller (mint or reuse path) but no lease ever built to drop and release that reference — an unrevocable standing grant surviving to expiry. Construct the lease first so any early return still drops it through the normal path, and switch bind_placeholder_to_session (and the sibling read, placeholder_session_ids) to lock_or_recover so a poisoned sessions_by_placeholder mutex is recovered instead of failing closed with a dangling reference. Co-Authored-By: Claude Opus 5 (1M context) --- crates/ironclaw_secrets/src/placeholder.rs | 131 ++++++++++++++++----- 1 file changed, 101 insertions(+), 30 deletions(-) diff --git a/crates/ironclaw_secrets/src/placeholder.rs b/crates/ironclaw_secrets/src/placeholder.rs index 8bf1813ea09..edc082b2480 100644 --- a/crates/ironclaw_secrets/src/placeholder.rs +++ b/crates/ironclaw_secrets/src/placeholder.rs @@ -393,36 +393,51 @@ impl InMemoryCredentialBroker { && self.try_join_live_lease(session_id, chrono::Utc::now())? { drop(jit_minted); - return self.finish_lease(placeholder, session_id); + return Ok(self.finish_lease(placeholder, session_id)); } let session = self.create_session(request)?; let session_id = session.correlation_id(); jit_minted.insert(key, session_id); - // This session_id was just minted and is not yet published to - // `sessions_by_placeholder`, and its `jit_minted` slot was just - // (re)written under the lock we are still holding, so no concurrent - // caller can reference it yet: registering the first lease reference - // here can't race a concurrent `release_lease` the way the reuse - // path above can. + // This session_id's `jit_minted` slot was just (re)written under the + // lock we are still holding — not yet published to + // `sessions_by_placeholder`, but already published to `jit_minted` — + // so no concurrent caller can observe "nothing minted yet" for this + // key: registering the first lease reference here can't race a + // concurrent `release_lease` the way the reuse path above can. self.acquire_lease_refcount(session_id); drop(jit_minted); - self.finish_lease(placeholder, session_id) + Ok(self.finish_lease(placeholder, session_id)) } - /// Common tail shared by both `mint_on_first_use` branches: binds - /// `session_id` to `placeholder` and hands back the RAII lease. + /// Common tail shared by both `mint_on_first_use` branches: hands back + /// the RAII lease for `session_id`, binding it to `placeholder` along the + /// way. + /// + /// The lease is constructed *before* the placeholder bind, not after: + /// both callers have already taken a reference on `session_id` + /// (`acquire_lease_refcount` on the mint path, the increment inside + /// `try_join_live_lease` on the reuse path) before reaching this method. + /// If the bind step returned early, an already-referenced session with + /// no lease ever constructed to drop and release that reference would be + /// exactly the standing-grant leak this module's header rules out. + /// Building the lease first means any early return still drops it and + /// releases the reference through the normal `Drop` path. (Today + /// `bind_placeholder_to_session` uses [`lock_or_recover`] and cannot + /// itself fail, but this ordering is the correct shape regardless — it + /// does not depend on that method staying infallible.) fn finish_lease( self: &Arc, placeholder: &CredentialPlaceholderToken, session_id: CredentialSessionId, - ) -> Result { - self.bind_placeholder_to_session(placeholder.clone(), session_id)?; - Ok(CredentialSessionLease { + ) -> CredentialSessionLease { + let lease = CredentialSessionLease { broker: self.clone(), session_id, revoked: false, - }) + }; + self.bind_placeholder_to_session(placeholder.clone(), session_id); + lease } /// Attempts to join an existing lease on `session_id`, returning whether @@ -510,7 +525,7 @@ impl InMemoryCredentialBroker { placeholder: &CredentialPlaceholderToken, scope: &ironclaw_host_api::ResourceScope, ) -> Result, CredentialBrokerError> { - for session_id in self.placeholder_session_ids(placeholder)? { + for session_id in self.placeholder_session_ids(placeholder) { match self.validate_session(session_id, chrono::Utc::now()) { Ok(session) if session.scope() == scope => return Ok(Some(session)), Ok(_) => continue, @@ -619,31 +634,21 @@ impl InMemoryCredentialBroker { &self, placeholder: CredentialPlaceholderToken, session_id: CredentialSessionId, - ) -> Result<(), CredentialBrokerError> { - self.sessions_by_placeholder - .lock() - .map_err(|error| CredentialBrokerError::BrokerUnavailable { - reason: error.to_string(), - })? + ) { + lock_or_recover(&self.sessions_by_placeholder) .entry(placeholder) .or_default() .insert(session_id); - Ok(()) } fn placeholder_session_ids( &self, placeholder: &CredentialPlaceholderToken, - ) -> Result, CredentialBrokerError> { - Ok(self - .sessions_by_placeholder - .lock() - .map_err(|error| CredentialBrokerError::BrokerUnavailable { - reason: error.to_string(), - })? + ) -> Vec { + lock_or_recover(&self.sessions_by_placeholder) .get(placeholder) .map(|session_ids| session_ids.iter().copied().collect()) - .unwrap_or_default()) + .unwrap_or_default() } } @@ -1218,6 +1223,72 @@ mod tests { second_lease.revoke(); } + #[test] + fn finish_lease_recovers_from_poisoned_placeholder_index_without_leaking_session() { + // Regression test for the critical leak: `bind_placeholder_to_session` + // used to be the one index write in this module that didn't use + // `lock_or_recover`, so a poisoned `sessions_by_placeholder` mutex + // made it return `Err` — and `finish_lease` used to construct the + // `CredentialSessionLease` only *after* that fallible bind, so the + // `?` propagated with the session already refcounted by the caller + // (`acquire_lease_refcount`) but no lease ever constructed to drop + // and release that reference: a standing grant nobody could revoke. + // + // Both halves of the fix are exercised here: `bind_placeholder_to_session` + // now uses `lock_or_recover` (so a poisoned lock is recovered instead + // of failing the whole mint), and `finish_lease` constructs the lease + // before the bind regardless, so the ordering invariant holds even if + // the bind step becomes fallible again in the future. Poisoning the + // real mutex and confirming the returned lease still fully revokes + // proves there is no leaked live session and no dangling refcount. + let broker = Arc::new(InMemoryCredentialBroker::new()); + let registry = CredentialPlaceholderRegistry::new(); + let (token, scope_a, account_id) = seeded(&broker, ®istry, "user-poison"); + + // Poison `sessions_by_placeholder` by panicking while holding its lock. + let poison_broker = Arc::clone(&broker); + let _ = std::panic::catch_unwind(AssertUnwindSafe(move || { + let _guard = poison_broker.sessions_by_placeholder.lock().unwrap(); + panic!("deliberately poison sessions_by_placeholder for regression test"); + })); + + let lease = broker + .mint_on_first_use( + &token, + session_request(scope_a.clone(), account_id, "https://api.example.com/v1/x"), + ) + .expect( + "mint_on_first_use must recover from a poisoned placeholder index, not fail closed \ + with an already-referenced, un-leased session", + ); + let session_id = lease.session_id(); + + assert!( + broker + .find_session_by_placeholder(&token, &scope_a) + .unwrap() + .is_some(), + "recovery from the poisoned lock must still actually bind the session" + ); + + // Revoking the lease must fully release it: no standing grant left behind. + lease.revoke(); + assert!( + matches!( + broker.validate_session(session_id, Utc::now()), + Err(CredentialBrokerError::UnknownSession { .. }) + ), + "the session must be genuinely revocable, not stranded past lease drop" + ); + assert!( + broker + .find_session_by_placeholder(&token, &scope_a) + .unwrap() + .is_none(), + "no dangling sessions_by_placeholder entry may survive revoke" + ); + } + #[test] fn mint_on_first_use_is_race_free_under_concurrent_first_use() { // Regression test for a confirmed race: `jit_minted_session_id` From 10f65fab1bf14715cf8081abdd90cdbdfc5af150 Mon Sep 17 00:00:00 2001 From: Henry Park Date: Sun, 26 Jul 2026 19:45:43 -0700 Subject: [PATCH 10/14] fix(secrets): collapse registry to one lock, tighten docs and length check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CredentialPlaceholderRegistry::get_or_create wrote by_owner and by_token under two separate mutexes, so a concurrent get_or_create for the same triple could take the by_owner early-return before by_token caught up, and resolve() would answer None for a token already handed out. Collapse both maps into one Mutex so every token resolves the instant get_or_create returns. Also: soften CredentialSessionLease's doc comment, which overclaimed "no safe way to leak the session" — std::mem::forget is safe, skips Drop, and strands the refcount; note the 30-minute expiry cap as the actual backstop. Tighten a stale comment in mint_on_first_use (the jit_minted-held-across-the-sequence fix already closed the double-mint window; the comment now says so precisely instead of leaving room to misread it as still open). And measure the placeholder suffix length in chars, matching the error message's "characters" wording, instead of bytes. Co-Authored-By: Claude Opus 5 (1M context) --- crates/ironclaw_secrets/src/placeholder.rs | 104 ++++++++++++++++----- 1 file changed, 79 insertions(+), 25 deletions(-) diff --git a/crates/ironclaw_secrets/src/placeholder.rs b/crates/ironclaw_secrets/src/placeholder.rs index edc082b2480..2ec5dc42faf 100644 --- a/crates/ironclaw_secrets/src/placeholder.rs +++ b/crates/ironclaw_secrets/src/placeholder.rs @@ -109,7 +109,13 @@ impl CredentialPlaceholderToken { // arbitrarily long `icsbx_...` string from the sandbox would let // untrusted input drive unbounded hashing/cloning/map-insertion work // on the host. - if suffix.len() != CREDENTIAL_PLACEHOLDER_SUFFIX_LEN + // Counted in `char`s, not bytes: the error message below promises + // "characters", and while the charset check just below rejects any + // non-ASCII input anyway (so `.len()` and `.chars().count()` agree on + // every value this function ever accepts), the length check runs + // first and should measure what it claims to measure regardless of + // charset-check ordering. + if suffix.chars().count() != CREDENTIAL_PLACEHOLDER_SUFFIX_LEN || !suffix .chars() .all(|character| character.is_ascii_alphanumeric()) @@ -186,6 +192,22 @@ struct CredentialPlaceholderOwnerKey { provider_or_extension_id: ExtensionId, } +/// Both maps `CredentialPlaceholderRegistry` keeps, under one lock. +/// +/// `by_owner` and `by_token` are two views of the identical bookkeeping (one +/// triple <-> one token), not two independent pieces of state — so they are +/// kept in a single `Mutex` rather than one each. Two separate locks would +/// let a concurrent `get_or_create` publish into `by_owner`, drop that guard, +/// and be observed by another caller's early return *before* the matching +/// `by_token` entry existed, so `resolve()` could answer `None` for a token +/// `get_or_create` had already handed out. A single lock over both maps +/// makes every publish atomic from an outside observer's perspective. +#[derive(Debug, Default)] +struct RegistryState { + by_owner: HashMap, + by_token: HashMap, +} + /// Host-side registry mapping `(tenant, user, provider)` to a stable /// placeholder token, and back. /// @@ -196,8 +218,7 @@ struct CredentialPlaceholderOwnerKey { /// [`InMemoryCredentialBroker::mint_on_first_use`] for the piece that does. #[derive(Debug, Default)] pub struct CredentialPlaceholderRegistry { - by_owner: Mutex>, - by_token: Mutex>, + state: Mutex, } impl CredentialPlaceholderRegistry { @@ -221,31 +242,25 @@ impl CredentialPlaceholderRegistry { user_id: user_id.clone(), provider_or_extension_id: provider_id.clone(), }; - let mut by_owner = - self.by_owner + let mut state = + self.state .lock() .map_err(|error| CredentialBrokerError::BrokerUnavailable { reason: error.to_string(), })?; - if let Some(existing) = by_owner.get(&key) { + if let Some(existing) = state.by_owner.get(&key) { return Ok(existing.clone()); } let token = CredentialPlaceholderToken::generate(); - by_owner.insert(key, token.clone()); - drop(by_owner); - self.by_token - .lock() - .map_err(|error| CredentialBrokerError::BrokerUnavailable { - reason: error.to_string(), - })? - .insert( - token.clone(), - CredentialPlaceholderOwner { - tenant_id: tenant_id.clone(), - user_id: user_id.clone(), - provider_or_extension_id: provider_id.clone(), - }, - ); + state.by_owner.insert(key, token.clone()); + state.by_token.insert( + token.clone(), + CredentialPlaceholderOwner { + tenant_id: tenant_id.clone(), + user_id: user_id.clone(), + provider_or_extension_id: provider_id.clone(), + }, + ); Ok(token) } @@ -254,16 +269,21 @@ impl CredentialPlaceholderRegistry { /// /// Backed by an exact-match `HashMap` (no prefix/substring scan), so this /// is O(1) and structurally cannot cross-match another user's token. + /// Because `by_owner` and `by_token` share one lock with `get_or_create`, + /// a token returned by `get_or_create` always resolves immediately — there + /// is no window where a freshly-minted token exists in one map but not + /// the other. pub fn resolve( &self, token: &CredentialPlaceholderToken, ) -> Result, CredentialBrokerError> { Ok(self - .by_token + .state .lock() .map_err(|error| CredentialBrokerError::BrokerUnavailable { reason: error.to_string(), })? + .by_token .get(token) .cloned()) } @@ -289,9 +309,17 @@ pub(crate) struct JitMintKey { /// - **panic**: `Drop` still runs during unwind, so a panic mid-dispatch /// revokes it too. /// -/// A missed revoke path would leave a standing grant, so this type has no -/// safe way to leak the session past its own lifetime: there is no `mem::forget`-safe -/// accessor, and the only way to keep the session alive is to hold the lease. +/// A missed revoke path would leave a standing grant, so dropping or +/// explicitly revoking the lease is the only way this type releases its +/// reference — there is no accessor that extends the session's life past the +/// lease being dropped. That said, this is intent enforced by this type's +/// API shape, not an unconditional guarantee: `std::mem::forget(lease)` is +/// safe Rust, skips `Drop`, and would strand the reference exactly the way a +/// forgotten `MutexGuard` would strand a lock. The backstop for that case +/// (and for any other way a lease's `Drop` fails to run) is the session +/// expiry cap `create_session` applies at mint time — 30 minutes by +/// default — so even a stranded reference cannot hold the session live past +/// that ceiling. pub struct CredentialSessionLease { broker: Arc, session_id: CredentialSessionId, @@ -1289,6 +1317,32 @@ mod tests { ); } + #[test] + fn registry_get_or_create_token_always_resolves_immediately() { + // Regression test: `get_or_create` used to publish into `by_owner`, + // drop that guard, and only then lock `by_token` separately. A + // concurrent call for the same triple in that window would take the + // early-return `by_owner` hit and hand out a token `resolve()` still + // answered `None` for — the registry's stated contract ("resolves a + // placeholder back to its owner") was only *eventually* true. Both + // maps now live behind one `Mutex`, so every token + // `get_or_create` returns already resolves by the time the call + // returns, with no window at all. + let registry = CredentialPlaceholderRegistry::new(); + let tenant = TenantId::new("tenant-a").unwrap(); + let user = UserId::new("user-resolve-atomic").unwrap(); + let provider = ExtensionId::new("google").unwrap(); + + let token = registry.get_or_create(&tenant, &user, &provider).unwrap(); + let owner = registry + .resolve(&token) + .unwrap() + .expect("a token returned by get_or_create must resolve immediately"); + assert_eq!(owner.tenant_id, tenant); + assert_eq!(owner.user_id, user); + assert_eq!(owner.provider_or_extension_id, provider); + } + #[test] fn mint_on_first_use_is_race_free_under_concurrent_first_use() { // Regression test for a confirmed race: `jit_minted_session_id` From a266ce3b2f3f4b1608d1ae9724c6d03200e0efc3 Mon Sep 17 00:00:00 2001 From: Henry Park Date: Sun, 26 Jul 2026 20:02:07 -0700 Subject: [PATCH 11/14] test(safety): pin placeholder redaction preserves diagnostic context redact_all_secrets tested detection-adjacent secrets but never an icsbx_ sandbox credential placeholder, and never confirmed that redaction masks only the token value while surrounding context (path, status code) survives. A redaction that nuked the whole string would pass a detection-only check while destroying the diagnostic value of sandbox output. The unwrap() in default_patterns() flagged in review is unchanged: it matches the same `// safety: hardcoded literal` convention as the 21 sibling regex patterns in this function, and the "No panics in production code" CI job (scripts/check_no_panics.py, which explicitly exempts any line containing `// safety:`) passed on this PR. Co-Authored-By: Claude Opus 5 (1M context) --- crates/ironclaw_safety/src/leak_detector.rs | 35 +++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/crates/ironclaw_safety/src/leak_detector.rs b/crates/ironclaw_safety/src/leak_detector.rs index 076d98a4985..1c889ecc33e 100644 --- a/crates/ironclaw_safety/src/leak_detector.rs +++ b/crates/ironclaw_safety/src/leak_detector.rs @@ -849,6 +849,41 @@ mod tests { assert!(redacted.contains("[REDACTED]")); } + #[test] + fn redact_all_secrets_masks_sandbox_credential_placeholder_without_dropping_context() { + // Detection of `icsbx_` placeholders is covered elsewhere; this pins + // that *redaction* actually removes the token value from + // model-visible output while the surrounding diagnostic context + // (path, status code) survives — a redaction that nuked the whole + // string would "pass" a detection-only test while destroying the + // output's diagnostic value. + let detector = LeakDetector::new(); + // Realistic shape: registry-generated placeholders are `icsbx_` plus + // exactly 32 lowercase hex characters (a simple-form UUID). + let token = "icsbx_0123456789abcdef0123456789abcdef"; + let content = format!("auth failed at /workspace/config using {token} (HTTP 401)"); + + let (redacted, changed) = detector.redact_all_secrets(&content); + + assert!( + changed, + "a placeholder was present, so redaction must report a change" + ); + assert!( + !redacted.contains(token), + "placeholder token must be redacted: {redacted}" + ); + assert!( + redacted.contains("/workspace/config"), + "path must survive: {redacted}" + ); + assert!( + redacted.contains("HTTP 401"), + "status code must survive: {redacted}" + ); + assert!(redacted.contains("[REDACTED]")); + } + #[test] fn redact_all_secrets_leaves_clean_text_untouched() { let detector = LeakDetector::new(); From 6623ab7e022bd2cdce5ae0cdf520602f870a8d4f Mon Sep 17 00:00:00 2001 From: Henry Park Date: Sun, 26 Jul 2026 20:21:41 -0700 Subject: [PATCH 12/14] fix(secrets): collapse session-lifecycle mutexes into one lock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit sessions, jit_minted, sessions_by_placeholder, and lease_refcounts were four separately-lockable HashMaps on InMemoryCredentialBroker, even though jit_minted and sessions_by_placeholder are just secondary indices over the same session records. That fragmentation forced mint_on_first_use to document a lock-ordering rule (jit_minted always outermost) and keep two "ignoring_jit_minted" twin methods purely to dodge self-deadlock on the non-reentrant std::sync::Mutex when a failure path needed to revoke while jit_minted was already held. Collapse all four into one Mutex, and fold the outstanding-lease count into CredentialSessionRecord as lease_count, replacing the parallel HashMap. mint_on_first_use now holds session_state locked across the whole lookup-or-mint-and-publish sequence as plain field access, so there is no second acquisition left to deadlock against. The twins (release_lease_ignoring_jit_minted, revoke_session_ignoring_jit_minted) and the lock-ordering doc comment are deleted outright, not refactored: the hazard they existed to dodge no longer exists. accounts stays a separate Mutex — create_session (via the new build_session split) already drops it before touching session state, so nesting it inside an already-held session_state lock cannot deadlock and cannot widen its own critical section. lock_or_recover moves from placeholder.rs to lib.rs (crate root) since it is now needed by inherent methods on InMemoryCredentialBroker defined there, and a private item in a child module is not visible to its parent. All previously-pinned properties (race-free concurrent first use, increment-then-validate ordering, revoke on all four lease exit paths, reused-session survival, secondary-index pruning, no standing grant on error, poisoned-lock recovery, cross-user isolation) still pass unchanged; the poisoned-lock regression test is updated to poison the single merged session_state mutex instead of the now-gone sessions_by_placeholder field. Co-Authored-By: Claude Opus 5 (1M context) --- crates/ironclaw_secrets/src/lib.rs | 187 ++++++---- crates/ironclaw_secrets/src/placeholder.rs | 407 +++++++++------------ 2 files changed, 273 insertions(+), 321 deletions(-) diff --git a/crates/ironclaw_secrets/src/lib.rs b/crates/ironclaw_secrets/src/lib.rs index 362368fe7f2..f5769f7384e 100644 --- a/crates/ironclaw_secrets/src/lib.rs +++ b/crates/ironclaw_secrets/src/lib.rs @@ -22,7 +22,7 @@ pub use secret_store::{CredentialBroker, SecretStore}; use std::collections::{HashMap, HashSet}; use std::fmt; -use std::sync::Mutex; +use std::sync::{Mutex, MutexGuard, PoisonError}; use async_trait::async_trait; pub use crypto::{ @@ -581,17 +581,33 @@ pub trait CredentialSessionStore: Send + Sync { ) -> Result; } +/// All session-lifecycle bookkeeping `InMemoryCredentialBroker` keeps beyond +/// account storage, collapsed under one lock. +/// +/// `sessions`, `jit_minted`, and `sessions_by_placeholder` used to be three +/// (really four, counting the outstanding-lease counter) separate `Mutex`es +/// on the broker. They are not independent state — `jit_minted` and +/// `sessions_by_placeholder` are both secondary indices over the same +/// `sessions` entries — so five separately-lockable pieces of one logical +/// session record was over-fragmentation: every mint/revoke sequence had to +/// hand-synchronize removals across maps and reason about which lock was +/// allowed to be held while acquiring which other one (see the +/// `CredentialPlaceholderRegistry` / `RegistryState` pattern just above, +/// which this mirrors). A single lock over all three makes every +/// mint/reuse/revoke sequence atomic from an outside observer's perspective, +/// and removes the need for lock-ordering rules or "ignoring" twin methods +/// that used to exist purely to dodge self-deadlock across separate +/// acquisitions of the same logical state. #[derive(Debug, Default)] -pub struct InMemoryCredentialBroker { - accounts: Mutex>, - sessions: Mutex>, +struct SessionState { + sessions: HashMap, /// Secondary index for JIT minting: `(invocation, capability, account)` -> /// the session already minted for it, so re-use within the same dispatch /// does not mint a second session. See [`placeholder::JitMintKey`] and /// [`InMemoryCredentialBroker::mint_on_first_use`]. Pruned (by session id) /// on [`InMemoryCredentialBroker::revoke_session`] so a long-lived process /// does not accumulate one entry per invocation forever. - jit_minted: Mutex>, + jit_minted: HashMap, /// Secondary index the egress proxy (W6-EGRESS-PROXY, not built yet) will /// use: placeholder token -> the live session(s) currently bound to it. /// Multi-valued because a placeholder is stable per `(tenant, user, @@ -606,22 +622,49 @@ pub struct InMemoryCredentialBroker { /// use) is the egress proxy's job, not this registry's — it is not built /// yet, so `find_session_by_placeholder` returns the first scope-match it /// finds. Pruned (by session id) on `revoke_session`. - sessions_by_placeholder: - Mutex>>, - /// Outstanding-lease counter per JIT-minted session id. `mint_on_first_use` - /// can hand out more than one [`CredentialSessionLease`] for the *same* - /// session (its cache-hit path reuses a still-live session rather than - /// minting a second one), so revoking one lease must not revoke the - /// session out from under another lease still holding it — the session is - /// only actually revoked when the last outstanding lease releases it. See - /// [`InMemoryCredentialBroker::release_lease`]. - lease_refcounts: Mutex>, + sessions_by_placeholder: HashMap>, +} + +#[derive(Debug, Default)] +pub struct InMemoryCredentialBroker { + accounts: Mutex>, + /// See [`SessionState`] for why this is one lock rather than several. + /// `accounts` stays a separate `Mutex`: `create_session` (by way of + /// `build_session`) always drops the accounts guard before touching + /// session state, so it is cleanly separable and merging it would only + /// widen the critical section for no benefit. + session_state: Mutex, } #[derive(Debug, Clone)] struct CredentialSessionRecord { session: CredentialSession, uses: u64, + /// Outstanding-lease count for this session. `mint_on_first_use` can hand + /// out more than one [`crate::CredentialSessionLease`] for the *same* + /// session (its cache-hit path reuses a still-live session rather than + /// minting a second one), so revoking one lease must not revoke the + /// session out from under another lease still holding it — the session is + /// only actually revoked when the last outstanding lease releases it. See + /// [`InMemoryCredentialBroker::release_lease`]. Zero for sessions created + /// through [`InMemoryCredentialBroker::create_session`] / + /// [`CredentialSessionStore::issue_session`] directly (no lease tracking + /// applies outside the JIT-minting path). + lease_count: usize, +} + +/// Locks `mutex`, recovering the inner guard on poison rather than silently +/// no-op'ing. A poisoned lock here must never be treated as "nothing to +/// clean up" — that would let a revoke path fail open and leave a standing +/// grant, contradicting this crate's core invariant for session state. +/// +/// Defined here (crate root) rather than in `placeholder.rs` so both that +/// module's `session_state`-locking helpers and this module's own methods +/// (`create_session`, `validate_session`, `consume_session_use`, the +/// `CredentialSessionStore` impl) can use it: a private item defined in a +/// child module is not visible to its parent, only to its own descendants. +fn lock_or_recover(mutex: &Mutex) -> MutexGuard<'_, T> { + mutex.lock().unwrap_or_else(PoisonError::into_inner) } fn ensure_credential_session_record_usable( @@ -693,6 +736,32 @@ impl InMemoryCredentialBroker { pub fn create_session( &self, request: CredentialSessionRequest, + ) -> Result { + let session = self.build_session(request)?; + lock_or_recover(&self.session_state).sessions.insert( + session.correlation_id, + CredentialSessionRecord { + session: session.clone(), + uses: 0, + lease_count: 0, + }, + ); + Ok(session) + } + + /// Validates `request` against its account and constructs the resulting + /// [`CredentialSession`], without publishing it anywhere. + /// + /// Split out of [`InMemoryCredentialBroker::create_session`] so + /// [`InMemoryCredentialBroker::mint_on_first_use`] (in `placeholder.rs`) + /// can hold `session_state` locked across the whole lookup-or-mint + /// sequence while still calling into this account-validation step: this + /// method only ever locks `accounts`, dropping that guard before + /// returning, so nesting it inside an already-held `session_state` lock + /// cannot deadlock and cannot widen `accounts`'s critical section. + fn build_session( + &self, + request: CredentialSessionRequest, ) -> Result { if request.invocation_id != request.scope.invocation_id { return Err(CredentialBrokerError::CredentialInvocationMismatch { @@ -755,7 +824,7 @@ impl InMemoryCredentialBroker { .expires_at .unwrap_or(default_expires_at) .min(max_expires_at); - let session = CredentialSession { + Ok(CredentialSession { scope: request.scope, invocation_id: request.invocation_id, capability_id: request.capability_id, @@ -766,21 +835,7 @@ impl InMemoryCredentialBroker { expires_at: Some(expires_at), max_uses: request.max_uses, correlation_id: CredentialSessionId::new(), - }; - drop(accounts); - self.sessions - .lock() - .map_err(|error| CredentialBrokerError::BrokerUnavailable { - reason: error.to_string(), - })? - .insert( - session.correlation_id, - CredentialSessionRecord { - session: session.clone(), - uses: 0, - }, - ); - Ok(session) + }) } pub fn validate_session( @@ -788,13 +843,9 @@ impl InMemoryCredentialBroker { session_id: CredentialSessionId, now: Timestamp, ) -> Result { - let mut sessions = - self.sessions - .lock() - .map_err(|error| CredentialBrokerError::BrokerUnavailable { - reason: error.to_string(), - })?; - let record = sessions + let mut state = lock_or_recover(&self.session_state); + let record = state + .sessions .get_mut(&session_id) .ok_or(CredentialBrokerError::UnknownSession { session_id })?; ensure_credential_session_record_usable(record, session_id, now)?; @@ -806,13 +857,9 @@ impl InMemoryCredentialBroker { session_id: CredentialSessionId, now: Timestamp, ) -> Result { - let mut sessions = - self.sessions - .lock() - .map_err(|error| CredentialBrokerError::BrokerUnavailable { - reason: error.to_string(), - })?; - let record = sessions + let mut state = lock_or_recover(&self.session_state); + let record = state + .sessions .get_mut(&session_id) .ok_or(CredentialBrokerError::UnknownSession { session_id })?; ensure_credential_session_record_usable(record, session_id, now)?; @@ -871,18 +918,14 @@ impl CredentialSessionStore for InMemoryCredentialBroker { &self, session: CredentialSession, ) -> Result { - self.sessions - .lock() - .map_err(|error| CredentialBrokerError::BrokerUnavailable { - reason: error.to_string(), - })? - .insert( - session.correlation_id, - CredentialSessionRecord { - session: session.clone(), - uses: 0, - }, - ); + lock_or_recover(&self.session_state).sessions.insert( + session.correlation_id, + CredentialSessionRecord { + session: session.clone(), + uses: 0, + lease_count: 0, + }, + ); Ok(session) } @@ -891,13 +934,9 @@ impl CredentialSessionStore for InMemoryCredentialBroker { scope: &ResourceScope, session_id: CredentialSessionId, ) -> Result, CredentialBrokerError> { - let sessions = - self.sessions - .lock() - .map_err(|error| CredentialBrokerError::BrokerUnavailable { - reason: error.to_string(), - })?; - Ok(sessions + let state = lock_or_recover(&self.session_state); + Ok(state + .sessions .get(&session_id) .filter(|record| record.session.scope == *scope) .map(|record| record.session.clone())) @@ -909,13 +948,9 @@ impl CredentialSessionStore for InMemoryCredentialBroker { session_id: CredentialSessionId, now: Timestamp, ) -> Result { - let sessions = - self.sessions - .lock() - .map_err(|error| CredentialBrokerError::BrokerUnavailable { - reason: error.to_string(), - })?; - let record = sessions + let state = lock_or_recover(&self.session_state); + let record = state + .sessions .get(&session_id) .filter(|record| record.session.scope == *scope) .ok_or(CredentialBrokerError::UnknownSession { session_id })?; @@ -929,13 +964,9 @@ impl CredentialSessionStore for InMemoryCredentialBroker { session_id: CredentialSessionId, now: Timestamp, ) -> Result { - let mut sessions = - self.sessions - .lock() - .map_err(|error| CredentialBrokerError::BrokerUnavailable { - reason: error.to_string(), - })?; - let record = sessions + let mut state = lock_or_recover(&self.session_state); + let record = state + .sessions .get_mut(&session_id) .filter(|record| record.session.scope == *scope) .ok_or(CredentialBrokerError::UnknownSession { session_id })?; diff --git a/crates/ironclaw_secrets/src/placeholder.rs b/crates/ironclaw_secrets/src/placeholder.rs index 2ec5dc42faf..e6db73bba3b 100644 --- a/crates/ironclaw_secrets/src/placeholder.rs +++ b/crates/ironclaw_secrets/src/placeholder.rs @@ -24,14 +24,14 @@ //! leave a standing grant. use std::collections::HashMap; use std::fmt; -use std::sync::{Arc, Mutex, MutexGuard, PoisonError}; +use std::sync::{Arc, Mutex}; use ironclaw_host_api::{CapabilityId, ExtensionId, InvocationId, TenantId, UserId}; use uuid::Uuid; use crate::{ - CredentialAccountId, CredentialBrokerError, CredentialSessionId, CredentialSessionRequest, - InMemoryCredentialBroker, + CredentialAccountId, CredentialBrokerError, CredentialSessionId, CredentialSessionRecord, + CredentialSessionRequest, InMemoryCredentialBroker, SessionState, lock_or_recover, }; /// Fixed prefix for every placeholder token. @@ -375,35 +375,26 @@ impl InMemoryCredentialBroker { /// **Concurrency:** the lookup-or-mint sequence below (checking /// `jit_minted` for an existing live session, and minting+publishing a /// new one if there isn't one) runs under a single, held-once lock on - /// `jit_minted`. Two threads racing on an identical `JitMintKey` used to - /// be able to both observe "nothing minted yet" — `jit_minted_session_id` - /// and `record_jit_mint` each locked/read-or-wrote/unlocked `jit_minted` - /// *separately* — and both mint their own session, defeating the - /// one-session-per-binding guarantee this module's doc comment asserts. - /// Holding one coarse lock across the whole sequence closes that window; - /// see `mint_on_first_use_is_race_free_under_concurrent_first_use` below. - /// Binding the result to a placeholder (`finish_lease` / - /// `bind_placeholder_to_session`) happens *after* the lock is dropped — - /// the invariant being protected is "at most one session per binding", - /// not "at most one placeholder-bind call", so that part doesn't need to - /// be in the critical section. + /// `session_state`. Two threads racing on an identical `JitMintKey` used + /// to be able to both observe "nothing minted yet" when `jit_minted` was + /// checked and updated through separate lock acquisitions, and both mint + /// their own session, defeating the one-session-per-binding guarantee + /// this module's doc comment asserts. Holding one coarse lock across the + /// whole sequence closes that window; see + /// `mint_on_first_use_is_race_free_under_concurrent_first_use` below. /// - /// **Lock ordering invariant:** `jit_minted` is always the outermost lock - /// whenever held together with `accounts` / `sessions` / - /// `lease_refcounts` — no path may acquire `jit_minted` while already - /// holding one of those. This is the first place in this module that - /// holds `jit_minted` across calls into other locking helpers - /// (`try_join_live_lease`, `create_session`, `acquire_lease_refcount`), - /// so it is also the first place a self-deadlock is possible: on - /// `std::sync::Mutex` (non-reentrant), a helper that itself tried to lock - /// `jit_minted` while this method still held it would hang forever. - /// `try_join_live_lease`'s failure path used to reach `revoke_session` - /// (via `release_lease`), which prunes `jit_minted` — exactly that - /// hazard — so it now calls `release_lease_ignoring_jit_minted` instead, - /// which does the same cleanup minus the `jit_minted` prune (safe here - /// because this method is about to overwrite that same key's entry under - /// the lock it already holds). `create_session` and - /// `acquire_lease_refcount` never touch `jit_minted` at all. + /// `sessions`, `jit_minted`, and `sessions_by_placeholder` (and the + /// outstanding-lease count, folded into each session's record) all live + /// behind that same `session_state` lock now, so the mint-then-publish + /// sequence below — inserting the session, recording it in `jit_minted`, + /// and binding it to `placeholder` — is a single atomic critical section + /// with no fallible step in the middle: there is no window where the + /// session is referenced by one index but not yet by the others, and no + /// second lock acquisition for any helper to self-deadlock against. + /// `accounts` (via [`InMemoryCredentialBroker::build_session`]) stays a + /// separate lock and is only ever acquired while already holding + /// `session_state`, never the other way around, so nesting it here + /// cannot deadlock either. pub fn mint_on_first_use( self: &Arc, placeholder: &CredentialPlaceholderToken, @@ -415,122 +406,36 @@ impl InMemoryCredentialBroker { account_id: request.account_id.clone(), }; - let mut jit_minted = lock_or_recover(&self.jit_minted); + let mut state = lock_or_recover(&self.session_state); - if let Some(&session_id) = jit_minted.get(&key) - && self.try_join_live_lease(session_id, chrono::Utc::now())? + let session_id = if let Some(&session_id) = state.jit_minted.get(&key) + && try_join_live_lease(&mut state, session_id, chrono::Utc::now())? { - drop(jit_minted); - return Ok(self.finish_lease(placeholder, session_id)); - } - - let session = self.create_session(request)?; - let session_id = session.correlation_id(); - jit_minted.insert(key, session_id); - // This session_id's `jit_minted` slot was just (re)written under the - // lock we are still holding — not yet published to - // `sessions_by_placeholder`, but already published to `jit_minted` — - // so no concurrent caller can observe "nothing minted yet" for this - // key: registering the first lease reference here can't race a - // concurrent `release_lease` the way the reuse path above can. - self.acquire_lease_refcount(session_id); - drop(jit_minted); - Ok(self.finish_lease(placeholder, session_id)) - } + session_id + } else { + let session = self.build_session(request)?; + let session_id = session.correlation_id(); + state.sessions.insert( + session_id, + CredentialSessionRecord { + session, + uses: 0, + // The first (and, at this point, only) outstanding lease + // reference for a freshly-minted session. + lease_count: 1, + }, + ); + state.jit_minted.insert(key, session_id); + session_id + }; + bind_placeholder_to_session(&mut state, placeholder.clone(), session_id); + drop(state); - /// Common tail shared by both `mint_on_first_use` branches: hands back - /// the RAII lease for `session_id`, binding it to `placeholder` along the - /// way. - /// - /// The lease is constructed *before* the placeholder bind, not after: - /// both callers have already taken a reference on `session_id` - /// (`acquire_lease_refcount` on the mint path, the increment inside - /// `try_join_live_lease` on the reuse path) before reaching this method. - /// If the bind step returned early, an already-referenced session with - /// no lease ever constructed to drop and release that reference would be - /// exactly the standing-grant leak this module's header rules out. - /// Building the lease first means any early return still drops it and - /// releases the reference through the normal `Drop` path. (Today - /// `bind_placeholder_to_session` uses [`lock_or_recover`] and cannot - /// itself fail, but this ordering is the correct shape regardless — it - /// does not depend on that method staying infallible.) - fn finish_lease( - self: &Arc, - placeholder: &CredentialPlaceholderToken, - session_id: CredentialSessionId, - ) -> CredentialSessionLease { - let lease = CredentialSessionLease { + Ok(CredentialSessionLease { broker: self.clone(), session_id, revoked: false, - }; - self.bind_placeholder_to_session(placeholder.clone(), session_id); - lease - } - - /// Attempts to join an existing lease on `session_id`, returning whether - /// it succeeded. - /// - /// The refcount increment and the liveness check are deliberately ordered - /// increment-then-validate, not validate-then-increment: incrementing - /// first only succeeds if `lease_refcounts` still has a live entry for - /// `session_id`, and [`InMemoryCredentialBroker::release_lease`] only - /// removes that entry (making the session eligible for revoke) while - /// holding the very same `lease_refcounts` lock. So once this call has - /// incremented the count, a concurrent `release_lease` on the - /// last-outstanding prior lease cannot have already revoked the session - /// — the two operations serialize on `lease_refcounts` instead of racing - /// across separate lock acquisitions. Validating the *other* way - /// (validate, then increment) would leave a window where a concurrent - /// revoke could land in between, handing back a lease for an - /// already-dead session. - /// - /// If the count was joined but the session turns out to be expired or - /// use-exhausted, the just-acquired reference is released again (via - /// [`InMemoryCredentialBroker::release_lease_ignoring_jit_minted`], which - /// revokes if that makes this the last reference) so this method never - /// leaves a dangling refcount behind on its own failure path. - /// - /// The release on the failure path deliberately skips pruning - /// `jit_minted` (unlike the general [`InMemoryCredentialBroker::release_lease`]): - /// this method is only ever called from - /// [`InMemoryCredentialBroker::mint_on_first_use`] while that method - /// already holds the `jit_minted` lock, so re-locking it here (as plain - /// `release_lease` would, via `revoke_session`) would deadlock on the - /// non-reentrant `std::sync::Mutex`. Skipping it doesn't leave anything - /// stale: `mint_on_first_use` overwrites this exact key's `jit_minted` - /// entry with the freshly minted session id immediately afterward, under - /// the same lock. - fn try_join_live_lease( - &self, - session_id: CredentialSessionId, - now: chrono::DateTime, - ) -> Result { - let joined = { - let mut counts = lock_or_recover(&self.lease_refcounts); - match counts.get_mut(&session_id) { - Some(count) => { - *count += 1; - true - } - None => false, - } - }; - if !joined { - return Ok(false); - } - match self.validate_session(session_id, now) { - Ok(_) => Ok(true), - Err(error) => { - self.release_lease_ignoring_jit_minted(session_id); - match error { - CredentialBrokerError::UnknownSession { .. } - | CredentialBrokerError::SessionExpired { .. } - | CredentialBrokerError::SessionUseLimitExceeded { .. } => Ok(false), - other => Err(other), - } - } - } + }) } /// Finds a live session currently bound to `placeholder`, if any. @@ -553,13 +458,28 @@ impl InMemoryCredentialBroker { placeholder: &CredentialPlaceholderToken, scope: &ironclaw_host_api::ResourceScope, ) -> Result, CredentialBrokerError> { - for session_id in self.placeholder_session_ids(placeholder) { - match self.validate_session(session_id, chrono::Utc::now()) { - Ok(session) if session.scope() == scope => return Ok(Some(session)), - Ok(_) => continue, - Err(CredentialBrokerError::UnknownSession { .. }) => continue, - Err(CredentialBrokerError::SessionExpired { .. }) => continue, - Err(CredentialBrokerError::SessionUseLimitExceeded { .. }) => continue, + let now = chrono::Utc::now(); + let state = lock_or_recover(&self.session_state); + let Some(session_ids) = state.sessions_by_placeholder.get(placeholder) else { + return Ok(None); + }; + for session_id in session_ids { + // A session_id can appear in `sessions_by_placeholder` for a + // session that has since been fully revoked and removed from + // `sessions` (the two live behind the same lock, but revoke + // prunes both together, not atomically-with-this-read — so a + // missing record here just means "grants nothing", same as an + // expired or use-exhausted one). + let Some(record) = state.sessions.get(session_id) else { + continue; + }; + match crate::ensure_credential_session_record_usable(record, *session_id, now) { + Ok(()) if record.session.scope() == scope => { + return Ok(Some(record.session.clone())); + } + Ok(()) => continue, + Err(CredentialBrokerError::SessionExpired { .. }) + | Err(CredentialBrokerError::SessionUseLimitExceeded { .. }) => continue, Err(other) => return Err(other), } } @@ -572,44 +492,14 @@ impl InMemoryCredentialBroker { /// already-unknown session is a no-op, since "no session" and "revoked /// session" both mean the placeholder grants nothing. /// - /// Also prunes this session out of the `jit_minted` and - /// `sessions_by_placeholder` secondary indices so a long-lived process - /// does not accumulate stale entries for every invocation/binding that - /// has ever existed. + /// Unconditional: removes the session and prunes the `jit_minted` and + /// `sessions_by_placeholder` secondary indices regardless of any + /// outstanding lease count, so a long-lived process does not accumulate + /// stale entries for every invocation/binding that has ever existed. See + /// [`InMemoryCredentialBroker::release_lease`] for the refcount-aware + /// variant every lease actually calls. pub fn revoke_session(&self, session_id: CredentialSessionId) { - lock_or_recover(&self.jit_minted) - .retain(|_, minted_session_id| *minted_session_id != session_id); - self.revoke_session_ignoring_jit_minted(session_id); - } - - /// Same cleanup as [`InMemoryCredentialBroker::revoke_session`] minus the - /// `jit_minted` prune. Split out so - /// [`InMemoryCredentialBroker::release_lease_ignoring_jit_minted`] (and, - /// through it, `try_join_live_lease`'s failure path) can revoke a stale - /// session without acquiring `jit_minted` a second time on a thread that - /// may already hold it — see the lock-ordering note on - /// [`InMemoryCredentialBroker::mint_on_first_use`]. - fn revoke_session_ignoring_jit_minted(&self, session_id: CredentialSessionId) { - lock_or_recover(&self.sessions).remove(&session_id); - lock_or_recover(&self.sessions_by_placeholder).retain(|_, session_ids| { - session_ids.remove(&session_id); - !session_ids.is_empty() - }); - lock_or_recover(&self.lease_refcounts).remove(&session_id); - } - - /// Registers one outstanding lease reference for a freshly-minted - /// `session_id`. See [`InMemoryCredentialBroker::release_lease`] for the - /// matching release half of this pair, and - /// [`InMemoryCredentialBroker::try_join_live_lease`] for the - /// increment-then-validate variant used when *reusing* an - /// already-published session id (this fresh-mint variant can't fail — - /// `lock_or_recover` never leaves poison unresolved — so unlike - /// `try_join_live_lease` it returns no `Result`). - fn acquire_lease_refcount(&self, session_id: CredentialSessionId) { - *lock_or_recover(&self.lease_refcounts) - .entry(session_id) - .or_insert(0) += 1; + remove_session_and_indices(&mut lock_or_recover(&self.session_state), session_id); } /// Releases one outstanding lease reference for `session_id`, revoking @@ -621,71 +511,102 @@ impl InMemoryCredentialBroker { /// their still-live session revoked out from under them by an earlier /// caller finishing first. pub(crate) fn release_lease(&self, session_id: CredentialSessionId) { - if Self::release_lease_refcount(&self.lease_refcounts, session_id) { - self.revoke_session(session_id); - } - } - - /// Same as [`InMemoryCredentialBroker::release_lease`], but revokes - /// through [`InMemoryCredentialBroker::revoke_session_ignoring_jit_minted`] - /// instead of [`InMemoryCredentialBroker::revoke_session`] — i.e. it never - /// acquires `jit_minted`. Used only by `try_join_live_lease`'s failure - /// path, which always runs while `mint_on_first_use` already holds - /// `jit_minted`; see the lock-ordering note there. - fn release_lease_ignoring_jit_minted(&self, session_id: CredentialSessionId) { - if Self::release_lease_refcount(&self.lease_refcounts, session_id) { - self.revoke_session_ignoring_jit_minted(session_id); - } + release_lease_in_state(&mut lock_or_recover(&self.session_state), session_id); } +} - /// Decrements (or removes) the outstanding-lease refcount entry for - /// `session_id`, returning whether that made this the last reference - /// (i.e. whether the caller should now actually revoke the session). - fn release_lease_refcount( - lease_refcounts: &Mutex>, - session_id: CredentialSessionId, - ) -> bool { - let mut counts = lock_or_recover(lease_refcounts); - match counts.get_mut(&session_id) { - Some(count) if *count > 1 => { - *count -= 1; - false - } - _ => { - counts.remove(&session_id); - true +/// Attempts to join an existing lease on `session_id`, returning whether it +/// succeeded. Always called with `state` already locked by the caller +/// ([`InMemoryCredentialBroker::mint_on_first_use`]) — `sessions`, +/// `jit_minted`, `sessions_by_placeholder`, and each record's lease count all +/// live behind that one lock now, so this can inspect and mutate all of them +/// as plain field access with no risk of a second acquisition to +/// self-deadlock against. +/// +/// The lease-count increment and the liveness check are deliberately ordered +/// increment-then-validate, not validate-then-increment: incrementing first +/// only succeeds if `state.sessions` still has a live entry for +/// `session_id`, and [`release_lease_in_state`] only removes that entry +/// (making the session eligible for revoke) under the very same lock this +/// function always runs under. So once this call has incremented the count, +/// a concurrent release of the last-outstanding prior lease cannot have +/// already removed the session from under it — the two operations serialize +/// on the single `session_state` lock instead of racing across separate +/// acquisitions. Validating the *other* way (validate, then increment) would +/// leave a window where a concurrent revoke could land in between, handing +/// back a lease for an already-dead session. +/// +/// If the count was joined but the session turns out to be expired or +/// use-exhausted, the just-acquired reference is released again (via +/// [`release_lease_in_state`], which removes the session and its secondary +/// indices if that was the last reference) so this function never leaves a +/// dangling lease count behind on its own failure path. +fn try_join_live_lease( + state: &mut SessionState, + session_id: CredentialSessionId, + now: chrono::DateTime, +) -> Result { + let Some(record) = state.sessions.get_mut(&session_id) else { + return Ok(false); + }; + record.lease_count += 1; + match crate::ensure_credential_session_record_usable(record, session_id, now) { + Ok(()) => Ok(true), + Err(error) => { + release_lease_in_state(state, session_id); + match error { + CredentialBrokerError::UnknownSession { .. } + | CredentialBrokerError::SessionExpired { .. } + | CredentialBrokerError::SessionUseLimitExceeded { .. } => Ok(false), + other => Err(other), } } } +} - fn bind_placeholder_to_session( - &self, - placeholder: CredentialPlaceholderToken, - session_id: CredentialSessionId, - ) { - lock_or_recover(&self.sessions_by_placeholder) - .entry(placeholder) - .or_default() - .insert(session_id); +/// Decrements the outstanding lease count for `session_id`; if that was the +/// last reference, removes the session and its secondary index entries (see +/// [`remove_session_and_indices`]). A missing record is treated the same as +/// "last reference already gone" — nothing left to decrement. +fn release_lease_in_state(state: &mut SessionState, session_id: CredentialSessionId) { + let is_last_reference = match state.sessions.get_mut(&session_id) { + Some(record) if record.lease_count > 1 => { + record.lease_count -= 1; + false + } + _ => true, + }; + if is_last_reference { + remove_session_and_indices(state, session_id); } +} - fn placeholder_session_ids( - &self, - placeholder: &CredentialPlaceholderToken, - ) -> Vec { - lock_or_recover(&self.sessions_by_placeholder) - .get(placeholder) - .map(|session_ids| session_ids.iter().copied().collect()) - .unwrap_or_default() - } +/// Removes `session_id` from `sessions` and prunes it out of both secondary +/// indices (`jit_minted`, `sessions_by_placeholder`) unconditionally, +/// regardless of lease count. The single shared tail for +/// [`InMemoryCredentialBroker::revoke_session`] (unconditional) and +/// [`release_lease_in_state`] (only once the last lease reference is gone). +fn remove_session_and_indices(state: &mut SessionState, session_id: CredentialSessionId) { + state.sessions.remove(&session_id); + state + .jit_minted + .retain(|_, minted_session_id| *minted_session_id != session_id); + state.sessions_by_placeholder.retain(|_, session_ids| { + session_ids.remove(&session_id); + !session_ids.is_empty() + }); } -/// Locks `mutex`, recovering the inner guard on poison rather than silently -/// no-op'ing. A poisoned lock here must never be treated as "nothing to -/// clean up" — that would let a revoke path fail open and leave a standing -/// grant, contradicting this module's core invariant. -fn lock_or_recover(mutex: &Mutex) -> MutexGuard<'_, T> { - mutex.lock().unwrap_or_else(PoisonError::into_inner) +fn bind_placeholder_to_session( + state: &mut SessionState, + placeholder: CredentialPlaceholderToken, + session_id: CredentialSessionId, +) { + state + .sessions_by_placeholder + .entry(placeholder) + .or_default() + .insert(session_id); } #[cfg(test)] @@ -1273,11 +1194,11 @@ mod tests { let registry = CredentialPlaceholderRegistry::new(); let (token, scope_a, account_id) = seeded(&broker, ®istry, "user-poison"); - // Poison `sessions_by_placeholder` by panicking while holding its lock. + // Poison `session_state` by panicking while holding its lock. let poison_broker = Arc::clone(&broker); let _ = std::panic::catch_unwind(AssertUnwindSafe(move || { - let _guard = poison_broker.sessions_by_placeholder.lock().unwrap(); - panic!("deliberately poison sessions_by_placeholder for regression test"); + let _guard = poison_broker.session_state.lock().unwrap(); + panic!("deliberately poison session_state for regression test"); })); let lease = broker From 38947f288312d5d67ee27c086994efb5a14d543f Mon Sep 17 00:00:00 2001 From: Henry Park Date: Sun, 26 Jul 2026 20:22:41 -0700 Subject: [PATCH 13/14] refactor(secrets): split placeholder tests out, merge redundant case placeholder.rs's #[cfg(test)] mod was ~745 lines, pushing the file to 1436 lines and past the repo's 1000-line convention despite the production body itself being small. Move it to a sibling placeholder_tests.rs via #[path], zero behavior change: super:: references still resolve since the module tree is identical, only the file that backs it moves. Also merge lease_revokes_on_explicit_success_call and lease_revokes_on_explicit_error_call into one lease_revokes_on_explicit_call: both called the identical CredentialSessionLease::revoke API and asserted the identical postcondition, differing only in a narrative `if dispatch_result.is_err()` wrapper that touches no broker code path. The timeout (cancellation-drop) and panic (unwind-drop) tests stay separate since those exercise genuinely different Rust mechanisms. Add the missing lookup-coverage case: a stale (expired or use-exhausted) session bound to a placeholder under the same scope as a later, still-valid one must not prevent the valid one from being found by find_session_by_placeholder. The existing multi-session test only covered scope-mismatch skipping. Co-Authored-By: Claude Opus 5 (1M context) --- crates/ironclaw_secrets/src/placeholder.rs | 747 +--------------- .../ironclaw_secrets/src/placeholder_tests.rs | 798 ++++++++++++++++++ 2 files changed, 800 insertions(+), 745 deletions(-) create mode 100644 crates/ironclaw_secrets/src/placeholder_tests.rs diff --git a/crates/ironclaw_secrets/src/placeholder.rs b/crates/ironclaw_secrets/src/placeholder.rs index e6db73bba3b..190e6c25222 100644 --- a/crates/ironclaw_secrets/src/placeholder.rs +++ b/crates/ironclaw_secrets/src/placeholder.rs @@ -610,748 +610,5 @@ fn bind_placeholder_to_session( } #[cfg(test)] -mod tests { - use std::panic::AssertUnwindSafe; - use std::sync::Arc; - use std::time::Duration; - - use chrono::Utc; - use ironclaw_host_api::{ - CapabilityId, ExtensionId, InvocationId, NetworkMethod, ProjectId, ResourceScope, - SecretHandle, TenantId, UserId, - }; - - use crate::{ - CredentialAccount, CredentialAccountId, CredentialAccountStatus, CredentialBrokerError, - CredentialPathPolicy, CredentialSessionRequest, CredentialTargetPolicy, - InMemoryCredentialBroker, RedactedJson, - }; - - use super::{ - CREDENTIAL_PLACEHOLDER_SUFFIX_LEN, CredentialPlaceholderRegistry, - CredentialPlaceholderToken, CredentialSessionLease, - }; - - fn sample_scope(tenant: &str, user: &str) -> ResourceScope { - ResourceScope { - tenant_id: TenantId::new(tenant).unwrap(), - user_id: UserId::new(user).unwrap(), - agent_id: None, - project_id: Some(ProjectId::new("project-a").unwrap()), - mission_id: None, - thread_id: None, - invocation_id: InvocationId::new(), - } - } - - fn sample_account( - scope: ResourceScope, - id: CredentialAccountId, - handle: SecretHandle, - ) -> CredentialAccount { - CredentialAccount { - scope, - id, - provider_or_extension_id: ExtensionId::new("google").unwrap(), - label: "Prod".to_string(), - status: CredentialAccountStatus::Active, - secret_handles: vec![handle], - allowed_targets: vec![CredentialTargetPolicy { - scheme: "https".to_string(), - host: "api.example.com".to_string(), - port: Some(443), - path: CredentialPathPolicy::Prefix("/v1/".to_string()), - methods: vec![NetworkMethod::Get], - }], - redacted_metadata: RedactedJson::new(serde_json::json!({})), - updated_at: Utc::now(), - } - } - - fn session_request( - scope: ResourceScope, - account_id: CredentialAccountId, - url: &str, - ) -> CredentialSessionRequest { - CredentialSessionRequest { - invocation_id: scope.invocation_id, - scope, - capability_id: CapabilityId::new("google.drive").unwrap(), - extension_id: ExtensionId::new("google").unwrap(), - account_id, - method: NetworkMethod::Get, - url: url.to_string(), - expires_at: None, - max_uses: None, - } - } - - #[test] - fn placeholder_is_stable_across_repeated_lookups_and_recycle() { - let registry = CredentialPlaceholderRegistry::new(); - let tenant = TenantId::new("tenant-a").unwrap(); - let user = UserId::new("user-a").unwrap(); - let provider = ExtensionId::new("google").unwrap(); - - let first = registry.get_or_create(&tenant, &user, &provider).unwrap(); - let second = registry.get_or_create(&tenant, &user, &provider).unwrap(); - assert_eq!(first, second); - - // Simulate a container recycle: nothing about the registry is tied to - // a container, so a "new container" asking for the same triple again - // gets the identical token. - drop(first); - let after_recycle = registry.get_or_create(&tenant, &user, &provider).unwrap(); - assert_eq!(second, after_recycle); - } - - #[test] - fn placeholders_are_distinct_and_isolated_per_user() { - let registry = CredentialPlaceholderRegistry::new(); - let tenant = TenantId::new("tenant-a").unwrap(); - let provider = ExtensionId::new("google").unwrap(); - let user_a = UserId::new("user-a").unwrap(); - let user_b = UserId::new("user-b").unwrap(); - - let token_a = registry.get_or_create(&tenant, &user_a, &provider).unwrap(); - let token_b = registry.get_or_create(&tenant, &user_b, &provider).unwrap(); - assert_ne!(token_a, token_b); - - let owner_a = registry.resolve(&token_a).unwrap().unwrap(); - let owner_b = registry.resolve(&token_b).unwrap().unwrap(); - assert_eq!(owner_a.user_id, user_a); - assert_eq!(owner_b.user_id, user_b); - assert_ne!(owner_a.user_id, owner_b.user_id); - - // User A's placeholder must never resolve to user B's owner triple. - assert_ne!( - registry.resolve(&token_a).unwrap(), - registry.resolve(&token_b).unwrap() - ); - } - - #[test] - fn placeholder_token_requires_fixed_prefix() { - assert!( - CredentialPlaceholderToken::parse("icsbx_0123456789abcdef0123456789abcdef").is_ok() - ); - let err = CredentialPlaceholderToken::parse("not_a_placeholder").unwrap_err(); - assert!(matches!( - err, - CredentialBrokerError::InvalidPlaceholderToken { .. } - )); - } - - #[test] - fn placeholder_token_parse_rejects_malformed_suffix() { - // Empty string: no prefix at all. - assert!(CredentialPlaceholderToken::parse("").is_err()); - // Bare prefix with no suffix carries no identifying material. - assert!(CredentialPlaceholderToken::parse("icsbx_").is_err()); - // Suffix shorter than the registry ever produces. - assert!(CredentialPlaceholderToken::parse("icsbx_ab").is_err()); - // Non-alphanumeric suffix characters (control chars / shell - // metacharacters) must fail closed rather than being accepted and - // potentially propagated into a log line or env var downstream. - assert!(CredentialPlaceholderToken::parse("icsbx_0123456789abcdef\n; rm -rf /").is_err()); - // Suffix longer than the registry ever produces: an arbitrarily long - // `icsbx_...` string from the sandbox must be rejected, not accepted - // and driven through hashing/cloning/map-insertion downstream. - assert!( - CredentialPlaceholderToken::parse(format!( - "icsbx_{}", - "a".repeat(CREDENTIAL_PLACEHOLDER_SUFFIX_LEN + 1) - )) - .is_err() - ); - } - - #[test] - fn placeholder_with_no_live_session_grants_nothing() { - let broker = Arc::new(InMemoryCredentialBroker::new()); - let registry = CredentialPlaceholderRegistry::new(); - let tenant = TenantId::new("tenant-a").unwrap(); - let user = UserId::new("user-a").unwrap(); - let provider = ExtensionId::new("google").unwrap(); - let token = registry.get_or_create(&tenant, &user, &provider).unwrap(); - - let found = broker - .find_session_by_placeholder(&token, &sample_scope("tenant-a", "user-a")) - .unwrap(); - assert!(found.is_none()); - } - - #[test] - fn jit_mint_binds_session_to_placeholder_and_enforces_target_policy() { - let broker = Arc::new(InMemoryCredentialBroker::new()); - let registry = CredentialPlaceholderRegistry::new(); - let tenant = TenantId::new("tenant-a").unwrap(); - let user = UserId::new("user-a").unwrap(); - let provider = ExtensionId::new("google").unwrap(); - let token = registry.get_or_create(&tenant, &user, &provider).unwrap(); - - let caller_scope = sample_scope("tenant-a", "user-a"); - let account_id = CredentialAccountId::new("google_prod").unwrap(); - broker - .put_account(sample_account( - caller_scope.clone(), - account_id.clone(), - SecretHandle::new("google_key").unwrap(), - )) - .unwrap(); - - // Out-of-policy request (wrong path) must be rejected, not minted. - let rejected = broker.mint_on_first_use( - &token, - session_request( - caller_scope.clone(), - account_id.clone(), - "https://api.example.com/v2/x", - ), - ); - assert!(matches!( - rejected, - Err(CredentialBrokerError::CredentialPolicyMismatch { .. }) - )); - assert!( - broker - .find_session_by_placeholder(&token, &caller_scope) - .unwrap() - .is_none() - ); - - // In-policy request mints and binds. - let lease = broker - .mint_on_first_use( - &token, - session_request( - caller_scope.clone(), - account_id, - "https://api.example.com/v1/x", - ), - ) - .unwrap(); - let bound = broker - .find_session_by_placeholder(&token, &caller_scope) - .unwrap() - .expect("session bound to placeholder after JIT mint"); - assert_eq!(bound.correlation_id(), lease.session_id()); - lease.revoke(); - } - - #[test] - fn placeholder_session_lookup_does_not_cross_user_scope() { - let broker = Arc::new(InMemoryCredentialBroker::new()); - let registry = CredentialPlaceholderRegistry::new(); - let tenant = TenantId::new("tenant-a").unwrap(); - let provider = ExtensionId::new("google").unwrap(); - let user_a = UserId::new("user-a").unwrap(); - let token_a = registry.get_or_create(&tenant, &user_a, &provider).unwrap(); - - let scope_a = sample_scope("tenant-a", "user-a"); - let account_id = CredentialAccountId::new("google_prod").unwrap(); - broker - .put_account(sample_account( - scope_a.clone(), - account_id.clone(), - SecretHandle::new("google_key").unwrap(), - )) - .unwrap(); - let lease = broker - .mint_on_first_use( - &token_a, - session_request(scope_a, account_id, "https://api.example.com/v1/x"), - ) - .unwrap(); - - // User B's scope must never see user A's session through the placeholder. - let other_scope = sample_scope("tenant-a", "user-b"); - let found = broker - .find_session_by_placeholder(&token_a, &other_scope) - .unwrap(); - assert!(found.is_none()); - lease.revoke(); - } - - #[test] - fn lease_revokes_on_explicit_success_call() { - let broker = Arc::new(InMemoryCredentialBroker::new()); - let registry = CredentialPlaceholderRegistry::new(); - let (token, scope_a, account_id) = seeded(&broker, ®istry, "user-success"); - - let lease = broker - .mint_on_first_use( - &token, - session_request(scope_a, account_id, "https://api.example.com/v1/x"), - ) - .unwrap(); - let session_id = lease.session_id(); - lease.revoke(); // success path calls this explicitly - assert!(matches!( - broker.validate_session(session_id, Utc::now()), - Err(CredentialBrokerError::UnknownSession { .. }) - )); - } - - #[test] - fn lease_revokes_on_explicit_error_call() { - let broker = Arc::new(InMemoryCredentialBroker::new()); - let registry = CredentialPlaceholderRegistry::new(); - let (token, scope_a, account_id) = seeded(&broker, ®istry, "user-error"); - - let lease = broker - .mint_on_first_use( - &token, - session_request(scope_a, account_id, "https://api.example.com/v1/x"), - ) - .unwrap(); - let session_id = lease.session_id(); - - let dispatch_result: Result<(), &'static str> = Err("upstream 500"); - if dispatch_result.is_err() { - lease.revoke(); // error path calls this explicitly too - } - assert!(matches!( - broker.validate_session(session_id, Utc::now()), - Err(CredentialBrokerError::UnknownSession { .. }) - )); - } - - #[tokio::test] - async fn lease_revokes_on_timeout_via_drop() { - let broker = Arc::new(InMemoryCredentialBroker::new()); - let registry = CredentialPlaceholderRegistry::new(); - let (token, scope_a, account_id) = seeded(&broker, ®istry, "user-timeout"); - - let lease = broker - .mint_on_first_use( - &token, - session_request(scope_a, account_id, "https://api.example.com/v1/x"), - ) - .unwrap(); - let session_id = lease.session_id(); - - let outcome = tokio::time::timeout(Duration::from_millis(5), async move { - let _lease = lease; // held across the (never-completing) sleep - tokio::time::sleep(Duration::from_secs(60)).await; - }) - .await; - - assert!(outcome.is_err(), "expected the dispatch to time out"); - assert!(matches!( - broker.validate_session(session_id, Utc::now()), - Err(CredentialBrokerError::UnknownSession { .. }) - )); - } - - #[test] - fn lease_revokes_on_panic_via_drop() { - let broker = Arc::new(InMemoryCredentialBroker::new()); - let registry = CredentialPlaceholderRegistry::new(); - let (token, scope_a, account_id) = seeded(&broker, ®istry, "user-panic"); - - let lease = broker - .mint_on_first_use( - &token, - session_request(scope_a, account_id, "https://api.example.com/v1/x"), - ) - .unwrap(); - let session_id = lease.session_id(); - - let result = std::panic::catch_unwind(AssertUnwindSafe(|| { - let _lease = lease; // dropped during unwind - panic!("simulated panic mid-dispatch"); - })); - - assert!(result.is_err()); - assert!(matches!( - broker.validate_session(session_id, Utc::now()), - Err(CredentialBrokerError::UnknownSession { .. }) - )); - } - - #[test] - fn reused_session_survives_the_first_of_two_leases_revoking() { - // Two mint_on_first_use calls for the identical (invocation, - // capability, account) binding — the documented cache-hit reuse path - // — must hand out two independent leases over the *same* session. - // Revoking the first-acquired lease must not revoke the session out - // from under the second lease still holding it; only releasing both - // should actually revoke it. - let broker = Arc::new(InMemoryCredentialBroker::new()); - let registry = CredentialPlaceholderRegistry::new(); - let (token, scope_a, account_id) = seeded(&broker, ®istry, "user-refcount"); - - let first_lease = broker - .mint_on_first_use( - &token, - session_request( - scope_a.clone(), - account_id.clone(), - "https://api.example.com/v1/x", - ), - ) - .unwrap(); - let second_lease = broker - .mint_on_first_use( - &token, - session_request(scope_a, account_id, "https://api.example.com/v1/x"), - ) - .unwrap(); - assert_eq!( - first_lease.session_id(), - second_lease.session_id(), - "cache-hit reuse must return the same underlying session" - ); - let session_id = first_lease.session_id(); - - first_lease.revoke(); - assert!( - broker.validate_session(session_id, Utc::now()).is_ok(), - "the second lease is still outstanding; revoking the first must not kill the shared session" - ); - - second_lease.revoke(); - assert!(matches!( - broker.validate_session(session_id, Utc::now()), - Err(CredentialBrokerError::UnknownSession { .. }) - )); - } - - #[test] - fn revoke_prunes_secondary_indices_so_placeholder_no_longer_resolves() { - // Revoking a session must not just make validate_session fail — it - // must also stop the placeholder from resolving to it at all, and - // must not leave stale bookkeeping (jit_minted / sessions_by_placeholder) - // behind for the life of the process. - let broker = Arc::new(InMemoryCredentialBroker::new()); - let registry = CredentialPlaceholderRegistry::new(); - let (token, scope_a, account_id) = seeded(&broker, ®istry, "user-prune"); - - let lease = broker - .mint_on_first_use( - &token, - session_request(scope_a.clone(), account_id, "https://api.example.com/v1/x"), - ) - .unwrap(); - assert!( - broker - .find_session_by_placeholder(&token, &scope_a) - .unwrap() - .is_some() - ); - - lease.revoke(); - - assert!( - broker - .find_session_by_placeholder(&token, &scope_a) - .unwrap() - .is_none(), - "placeholder must not resolve to a revoked session" - ); - } - - #[test] - fn find_session_by_placeholder_iterates_multiple_distinct_sessions_for_same_placeholder() { - // A placeholder can legitimately have more than one *distinct* - // session bound to it at once (e.g. two different accounts under - // one provider) — not just the same session reused, which is all - // the refcount tests above exercise. This pins that the - // scope-matching loop in `find_session_by_placeholder` actually - // walks past a non-matching candidate to find the right one. - let broker = Arc::new(InMemoryCredentialBroker::new()); - let registry = CredentialPlaceholderRegistry::new(); - let tenant = TenantId::new("tenant-a").unwrap(); - let provider = ExtensionId::new("google").unwrap(); - let user_a = UserId::new("user-multi-a").unwrap(); - let token = registry.get_or_create(&tenant, &user_a, &provider).unwrap(); - - let scope_a = sample_scope("tenant-a", "user-multi-a"); - let scope_b = sample_scope("tenant-a", "user-multi-b"); - let account_a = CredentialAccountId::new("google_account_a").unwrap(); - let account_b = CredentialAccountId::new("google_account_b").unwrap(); - broker - .put_account(sample_account( - scope_a.clone(), - account_a.clone(), - SecretHandle::new("key_a").unwrap(), - )) - .unwrap(); - broker - .put_account(sample_account( - scope_b.clone(), - account_b.clone(), - SecretHandle::new("key_b").unwrap(), - )) - .unwrap(); - - // Two distinct (invocation, capability, account) bindings mint two - // distinct sessions, both bound to the same placeholder token. - let lease_a = broker - .mint_on_first_use( - &token, - session_request(scope_a.clone(), account_a, "https://api.example.com/v1/x"), - ) - .unwrap(); - let lease_b = broker - .mint_on_first_use( - &token, - session_request(scope_b.clone(), account_b, "https://api.example.com/v1/x"), - ) - .unwrap(); - assert_ne!( - lease_a.session_id(), - lease_b.session_id(), - "two different accounts must mint two distinct sessions" - ); - - // Regardless of HashSet iteration order, querying with a specific - // scope must find that scope's session, not the other one bound to - // the same placeholder. - let found_b = broker - .find_session_by_placeholder(&token, &scope_b) - .unwrap() - .expect("scope_b's session must be found among the placeholder's bound sessions"); - assert_eq!(found_b.correlation_id(), lease_b.session_id()); - - let found_a = broker - .find_session_by_placeholder(&token, &scope_a) - .unwrap() - .expect("scope_a's session must be found among the placeholder's bound sessions"); - assert_eq!(found_a.correlation_id(), lease_a.session_id()); - - lease_a.revoke(); - lease_b.revoke(); - } - - #[test] - fn mint_on_first_use_remints_when_prior_jit_session_is_expired() { - // The jit_minted cache remembers a session id per (invocation, - // capability, account) key so re-use doesn't mint twice — but only - // while that session is still live. This pins the fallthrough - // branch: a cached key whose session has since expired must not be - // handed back; a fresh session must be minted instead. - let broker = Arc::new(InMemoryCredentialBroker::new()); - let registry = CredentialPlaceholderRegistry::new(); - let (token, scope_a, account_id) = seeded(&broker, ®istry, "user-remint"); - - let mut first_request = session_request( - scope_a.clone(), - account_id.clone(), - "https://api.example.com/v1/x", - ); - first_request.expires_at = Some(Utc::now() - chrono::Duration::seconds(1)); - let first_lease = broker.mint_on_first_use(&token, first_request).unwrap(); - let first_session_id = first_lease.session_id(); - assert!( - broker - .validate_session(first_session_id, Utc::now()) - .is_err(), - "sanity: the first session must already be expired" - ); - - // The jit_minted cache entry still points at the now-expired - // session; a second call for the identical key must not hand back a - // lease for the dead session — it must remint. - let second_request = session_request(scope_a, account_id, "https://api.example.com/v1/x"); - let second_lease = broker.mint_on_first_use(&token, second_request).unwrap(); - assert_ne!( - first_session_id, - second_lease.session_id(), - "an expired jit-cached session must not be reused" - ); - assert!( - broker - .validate_session(second_lease.session_id(), Utc::now()) - .is_ok(), - "the reminted session must be live" - ); - - first_lease.revoke(); - second_lease.revoke(); - } - - #[test] - fn finish_lease_recovers_from_poisoned_placeholder_index_without_leaking_session() { - // Regression test for the critical leak: `bind_placeholder_to_session` - // used to be the one index write in this module that didn't use - // `lock_or_recover`, so a poisoned `sessions_by_placeholder` mutex - // made it return `Err` — and `finish_lease` used to construct the - // `CredentialSessionLease` only *after* that fallible bind, so the - // `?` propagated with the session already refcounted by the caller - // (`acquire_lease_refcount`) but no lease ever constructed to drop - // and release that reference: a standing grant nobody could revoke. - // - // Both halves of the fix are exercised here: `bind_placeholder_to_session` - // now uses `lock_or_recover` (so a poisoned lock is recovered instead - // of failing the whole mint), and `finish_lease` constructs the lease - // before the bind regardless, so the ordering invariant holds even if - // the bind step becomes fallible again in the future. Poisoning the - // real mutex and confirming the returned lease still fully revokes - // proves there is no leaked live session and no dangling refcount. - let broker = Arc::new(InMemoryCredentialBroker::new()); - let registry = CredentialPlaceholderRegistry::new(); - let (token, scope_a, account_id) = seeded(&broker, ®istry, "user-poison"); - - // Poison `session_state` by panicking while holding its lock. - let poison_broker = Arc::clone(&broker); - let _ = std::panic::catch_unwind(AssertUnwindSafe(move || { - let _guard = poison_broker.session_state.lock().unwrap(); - panic!("deliberately poison session_state for regression test"); - })); - - let lease = broker - .mint_on_first_use( - &token, - session_request(scope_a.clone(), account_id, "https://api.example.com/v1/x"), - ) - .expect( - "mint_on_first_use must recover from a poisoned placeholder index, not fail closed \ - with an already-referenced, un-leased session", - ); - let session_id = lease.session_id(); - - assert!( - broker - .find_session_by_placeholder(&token, &scope_a) - .unwrap() - .is_some(), - "recovery from the poisoned lock must still actually bind the session" - ); - - // Revoking the lease must fully release it: no standing grant left behind. - lease.revoke(); - assert!( - matches!( - broker.validate_session(session_id, Utc::now()), - Err(CredentialBrokerError::UnknownSession { .. }) - ), - "the session must be genuinely revocable, not stranded past lease drop" - ); - assert!( - broker - .find_session_by_placeholder(&token, &scope_a) - .unwrap() - .is_none(), - "no dangling sessions_by_placeholder entry may survive revoke" - ); - } - - #[test] - fn registry_get_or_create_token_always_resolves_immediately() { - // Regression test: `get_or_create` used to publish into `by_owner`, - // drop that guard, and only then lock `by_token` separately. A - // concurrent call for the same triple in that window would take the - // early-return `by_owner` hit and hand out a token `resolve()` still - // answered `None` for — the registry's stated contract ("resolves a - // placeholder back to its owner") was only *eventually* true. Both - // maps now live behind one `Mutex`, so every token - // `get_or_create` returns already resolves by the time the call - // returns, with no window at all. - let registry = CredentialPlaceholderRegistry::new(); - let tenant = TenantId::new("tenant-a").unwrap(); - let user = UserId::new("user-resolve-atomic").unwrap(); - let provider = ExtensionId::new("google").unwrap(); - - let token = registry.get_or_create(&tenant, &user, &provider).unwrap(); - let owner = registry - .resolve(&token) - .unwrap() - .expect("a token returned by get_or_create must resolve immediately"); - assert_eq!(owner.tenant_id, tenant); - assert_eq!(owner.user_id, user); - assert_eq!(owner.provider_or_extension_id, provider); - } - - #[test] - fn mint_on_first_use_is_race_free_under_concurrent_first_use() { - // Regression test for a confirmed race: `jit_minted_session_id` - // (read) and `record_jit_mint` (write) used to lock/unlock - // `jit_minted` *separately*, so two threads racing on the identical - // `JitMintKey` could both observe "nothing minted yet" and both mint - // their own session — defeating the one-session-per-binding - // guarantee this module's doc comment asserts, and multiplying a - // `max_uses: Some(1)` budget. The fix holds one lock across the - // whole lookup-or-mint sequence in `mint_on_first_use`. - // - // This is only probabilistic at *catching* a reintroduced race — a - // `Barrier`-aligned start makes the race window likely to be hit, - // not guaranteed. But it has zero false-fail risk once the fix is - // in place: the lock enforces exclusion structurally, so a passing - // run here is a real guarantee, not a lucky one. - const THREAD_COUNT: usize = 32; - - let broker = Arc::new(InMemoryCredentialBroker::new()); - let registry = CredentialPlaceholderRegistry::new(); - let (token, scope_a, account_id) = seeded(&broker, ®istry, "user-race"); - let barrier = Arc::new(std::sync::Barrier::new(THREAD_COUNT)); - - let handles: Vec<_> = (0..THREAD_COUNT) - .map(|_| { - let broker = Arc::clone(&broker); - let token = token.clone(); - let scope_a = scope_a.clone(); - let account_id = account_id.clone(); - let barrier = Arc::clone(&barrier); - std::thread::spawn(move || { - let request = - session_request(scope_a, account_id, "https://api.example.com/v1/x"); - barrier.wait(); - broker - .mint_on_first_use(&token, request) - .expect("mint_on_first_use must succeed for every racing thread") - }) - }) - .collect(); - - let leases: Vec = handles - .into_iter() - .map(|handle| { - handle - .join() - .expect("mint_on_first_use thread must not panic") - }) - .collect(); - - let distinct_session_ids: std::collections::HashSet<_> = - leases.iter().map(|lease| lease.session_id()).collect(); - assert_eq!( - distinct_session_ids.len(), - 1, - "every thread racing on the identical (invocation, capability, account) binding \ - must be handed a lease for the exact same session, not one each" - ); - - for lease in leases { - lease.revoke(); - } - } - - fn seeded( - broker: &Arc, - registry: &CredentialPlaceholderRegistry, - user: &str, - ) -> ( - CredentialPlaceholderToken, - ResourceScope, - CredentialAccountId, - ) { - let tenant = TenantId::new("tenant-a").unwrap(); - let user_id = UserId::new(user).unwrap(); - let provider = ExtensionId::new("google").unwrap(); - let token = registry - .get_or_create(&tenant, &user_id, &provider) - .unwrap(); - let caller_scope = sample_scope("tenant-a", user); - let account_id = CredentialAccountId::new("google_prod").unwrap(); - broker - .put_account(sample_account( - caller_scope.clone(), - account_id.clone(), - SecretHandle::new("google_key").unwrap(), - )) - .unwrap(); - (token, caller_scope, account_id) - } -} +#[path = "placeholder_tests.rs"] +mod tests; diff --git a/crates/ironclaw_secrets/src/placeholder_tests.rs b/crates/ironclaw_secrets/src/placeholder_tests.rs new file mode 100644 index 00000000000..df70e35afe2 --- /dev/null +++ b/crates/ironclaw_secrets/src/placeholder_tests.rs @@ -0,0 +1,798 @@ +use std::panic::AssertUnwindSafe; +use std::sync::Arc; +use std::time::Duration; + +use chrono::Utc; +use ironclaw_host_api::{ + CapabilityId, ExtensionId, InvocationId, NetworkMethod, ProjectId, ResourceScope, SecretHandle, + TenantId, UserId, +}; + +use crate::{ + CredentialAccount, CredentialAccountId, CredentialAccountStatus, CredentialBrokerError, + CredentialPathPolicy, CredentialSessionRequest, CredentialTargetPolicy, + InMemoryCredentialBroker, RedactedJson, +}; + +use super::{ + CREDENTIAL_PLACEHOLDER_SUFFIX_LEN, CredentialPlaceholderRegistry, CredentialPlaceholderToken, + CredentialSessionLease, +}; + +fn sample_scope(tenant: &str, user: &str) -> ResourceScope { + ResourceScope { + tenant_id: TenantId::new(tenant).unwrap(), + user_id: UserId::new(user).unwrap(), + agent_id: None, + project_id: Some(ProjectId::new("project-a").unwrap()), + mission_id: None, + thread_id: None, + invocation_id: InvocationId::new(), + } +} + +fn sample_account( + scope: ResourceScope, + id: CredentialAccountId, + handle: SecretHandle, +) -> CredentialAccount { + CredentialAccount { + scope, + id, + provider_or_extension_id: ExtensionId::new("google").unwrap(), + label: "Prod".to_string(), + status: CredentialAccountStatus::Active, + secret_handles: vec![handle], + allowed_targets: vec![CredentialTargetPolicy { + scheme: "https".to_string(), + host: "api.example.com".to_string(), + port: Some(443), + path: CredentialPathPolicy::Prefix("/v1/".to_string()), + methods: vec![NetworkMethod::Get], + }], + redacted_metadata: RedactedJson::new(serde_json::json!({})), + updated_at: Utc::now(), + } +} + +fn session_request( + scope: ResourceScope, + account_id: CredentialAccountId, + url: &str, +) -> CredentialSessionRequest { + CredentialSessionRequest { + invocation_id: scope.invocation_id, + scope, + capability_id: CapabilityId::new("google.drive").unwrap(), + extension_id: ExtensionId::new("google").unwrap(), + account_id, + method: NetworkMethod::Get, + url: url.to_string(), + expires_at: None, + max_uses: None, + } +} + +#[test] +fn placeholder_is_stable_across_repeated_lookups_and_recycle() { + let registry = CredentialPlaceholderRegistry::new(); + let tenant = TenantId::new("tenant-a").unwrap(); + let user = UserId::new("user-a").unwrap(); + let provider = ExtensionId::new("google").unwrap(); + + let first = registry.get_or_create(&tenant, &user, &provider).unwrap(); + let second = registry.get_or_create(&tenant, &user, &provider).unwrap(); + assert_eq!(first, second); + + // Simulate a container recycle: nothing about the registry is tied to + // a container, so a "new container" asking for the same triple again + // gets the identical token. + drop(first); + let after_recycle = registry.get_or_create(&tenant, &user, &provider).unwrap(); + assert_eq!(second, after_recycle); +} + +#[test] +fn placeholders_are_distinct_and_isolated_per_user() { + let registry = CredentialPlaceholderRegistry::new(); + let tenant = TenantId::new("tenant-a").unwrap(); + let provider = ExtensionId::new("google").unwrap(); + let user_a = UserId::new("user-a").unwrap(); + let user_b = UserId::new("user-b").unwrap(); + + let token_a = registry.get_or_create(&tenant, &user_a, &provider).unwrap(); + let token_b = registry.get_or_create(&tenant, &user_b, &provider).unwrap(); + assert_ne!(token_a, token_b); + + let owner_a = registry.resolve(&token_a).unwrap().unwrap(); + let owner_b = registry.resolve(&token_b).unwrap().unwrap(); + assert_eq!(owner_a.user_id, user_a); + assert_eq!(owner_b.user_id, user_b); + assert_ne!(owner_a.user_id, owner_b.user_id); + + // User A's placeholder must never resolve to user B's owner triple. + assert_ne!( + registry.resolve(&token_a).unwrap(), + registry.resolve(&token_b).unwrap() + ); +} + +#[test] +fn placeholder_token_requires_fixed_prefix() { + assert!(CredentialPlaceholderToken::parse("icsbx_0123456789abcdef0123456789abcdef").is_ok()); + let err = CredentialPlaceholderToken::parse("not_a_placeholder").unwrap_err(); + assert!(matches!( + err, + CredentialBrokerError::InvalidPlaceholderToken { .. } + )); +} + +#[test] +fn placeholder_token_parse_rejects_malformed_suffix() { + // Empty string: no prefix at all. + assert!(CredentialPlaceholderToken::parse("").is_err()); + // Bare prefix with no suffix carries no identifying material. + assert!(CredentialPlaceholderToken::parse("icsbx_").is_err()); + // Suffix shorter than the registry ever produces. + assert!(CredentialPlaceholderToken::parse("icsbx_ab").is_err()); + // Non-alphanumeric suffix characters (control chars / shell + // metacharacters) must fail closed rather than being accepted and + // potentially propagated into a log line or env var downstream. + assert!(CredentialPlaceholderToken::parse("icsbx_0123456789abcdef\n; rm -rf /").is_err()); + // Suffix longer than the registry ever produces: an arbitrarily long + // `icsbx_...` string from the sandbox must be rejected, not accepted + // and driven through hashing/cloning/map-insertion downstream. + assert!( + CredentialPlaceholderToken::parse(format!( + "icsbx_{}", + "a".repeat(CREDENTIAL_PLACEHOLDER_SUFFIX_LEN + 1) + )) + .is_err() + ); +} + +#[test] +fn placeholder_with_no_live_session_grants_nothing() { + let broker = Arc::new(InMemoryCredentialBroker::new()); + let registry = CredentialPlaceholderRegistry::new(); + let tenant = TenantId::new("tenant-a").unwrap(); + let user = UserId::new("user-a").unwrap(); + let provider = ExtensionId::new("google").unwrap(); + let token = registry.get_or_create(&tenant, &user, &provider).unwrap(); + + let found = broker + .find_session_by_placeholder(&token, &sample_scope("tenant-a", "user-a")) + .unwrap(); + assert!(found.is_none()); +} + +#[test] +fn jit_mint_binds_session_to_placeholder_and_enforces_target_policy() { + let broker = Arc::new(InMemoryCredentialBroker::new()); + let registry = CredentialPlaceholderRegistry::new(); + let tenant = TenantId::new("tenant-a").unwrap(); + let user = UserId::new("user-a").unwrap(); + let provider = ExtensionId::new("google").unwrap(); + let token = registry.get_or_create(&tenant, &user, &provider).unwrap(); + + let caller_scope = sample_scope("tenant-a", "user-a"); + let account_id = CredentialAccountId::new("google_prod").unwrap(); + broker + .put_account(sample_account( + caller_scope.clone(), + account_id.clone(), + SecretHandle::new("google_key").unwrap(), + )) + .unwrap(); + + // Out-of-policy request (wrong path) must be rejected, not minted. + let rejected = broker.mint_on_first_use( + &token, + session_request( + caller_scope.clone(), + account_id.clone(), + "https://api.example.com/v2/x", + ), + ); + assert!(matches!( + rejected, + Err(CredentialBrokerError::CredentialPolicyMismatch { .. }) + )); + assert!( + broker + .find_session_by_placeholder(&token, &caller_scope) + .unwrap() + .is_none() + ); + + // In-policy request mints and binds. + let lease = broker + .mint_on_first_use( + &token, + session_request( + caller_scope.clone(), + account_id, + "https://api.example.com/v1/x", + ), + ) + .unwrap(); + let bound = broker + .find_session_by_placeholder(&token, &caller_scope) + .unwrap() + .expect("session bound to placeholder after JIT mint"); + assert_eq!(bound.correlation_id(), lease.session_id()); + lease.revoke(); +} + +#[test] +fn placeholder_session_lookup_does_not_cross_user_scope() { + let broker = Arc::new(InMemoryCredentialBroker::new()); + let registry = CredentialPlaceholderRegistry::new(); + let tenant = TenantId::new("tenant-a").unwrap(); + let provider = ExtensionId::new("google").unwrap(); + let user_a = UserId::new("user-a").unwrap(); + let token_a = registry.get_or_create(&tenant, &user_a, &provider).unwrap(); + + let scope_a = sample_scope("tenant-a", "user-a"); + let account_id = CredentialAccountId::new("google_prod").unwrap(); + broker + .put_account(sample_account( + scope_a.clone(), + account_id.clone(), + SecretHandle::new("google_key").unwrap(), + )) + .unwrap(); + let lease = broker + .mint_on_first_use( + &token_a, + session_request(scope_a, account_id, "https://api.example.com/v1/x"), + ) + .unwrap(); + + // User B's scope must never see user A's session through the placeholder. + let other_scope = sample_scope("tenant-a", "user-b"); + let found = broker + .find_session_by_placeholder(&token_a, &other_scope) + .unwrap(); + assert!(found.is_none()); + lease.revoke(); +} + +#[test] +fn lease_revokes_on_explicit_call() { + // Stands in for both the success-path and error-path explicit-revoke + // call sites: both call the identical `CredentialSessionLease::revoke` + // API and assert the identical postcondition (session gone). The only + // difference between them was a narrative `if dispatch_result.is_err()` + // wrapper around the call — no broker code path branches on it, so a + // single test covers both without overlap. Cancellation-drop (timeout) + // and unwind-drop (panic) exercise genuinely different Rust + // mechanisms and stay as separate tests below. + let broker = Arc::new(InMemoryCredentialBroker::new()); + let registry = CredentialPlaceholderRegistry::new(); + let (token, scope_a, account_id) = seeded(&broker, ®istry, "user-explicit-revoke"); + + let lease = broker + .mint_on_first_use( + &token, + session_request(scope_a, account_id, "https://api.example.com/v1/x"), + ) + .unwrap(); + let session_id = lease.session_id(); + lease.revoke(); // success and error paths both call this explicitly + assert!(matches!( + broker.validate_session(session_id, Utc::now()), + Err(CredentialBrokerError::UnknownSession { .. }) + )); +} + +#[tokio::test] +async fn lease_revokes_on_timeout_via_drop() { + let broker = Arc::new(InMemoryCredentialBroker::new()); + let registry = CredentialPlaceholderRegistry::new(); + let (token, scope_a, account_id) = seeded(&broker, ®istry, "user-timeout"); + + let lease = broker + .mint_on_first_use( + &token, + session_request(scope_a, account_id, "https://api.example.com/v1/x"), + ) + .unwrap(); + let session_id = lease.session_id(); + + let outcome = tokio::time::timeout(Duration::from_millis(5), async move { + let _lease = lease; // held across the (never-completing) sleep + tokio::time::sleep(Duration::from_secs(60)).await; + }) + .await; + + assert!(outcome.is_err(), "expected the dispatch to time out"); + assert!(matches!( + broker.validate_session(session_id, Utc::now()), + Err(CredentialBrokerError::UnknownSession { .. }) + )); +} + +#[test] +fn lease_revokes_on_panic_via_drop() { + let broker = Arc::new(InMemoryCredentialBroker::new()); + let registry = CredentialPlaceholderRegistry::new(); + let (token, scope_a, account_id) = seeded(&broker, ®istry, "user-panic"); + + let lease = broker + .mint_on_first_use( + &token, + session_request(scope_a, account_id, "https://api.example.com/v1/x"), + ) + .unwrap(); + let session_id = lease.session_id(); + + let result = std::panic::catch_unwind(AssertUnwindSafe(|| { + let _lease = lease; // dropped during unwind + panic!("simulated panic mid-dispatch"); + })); + + assert!(result.is_err()); + assert!(matches!( + broker.validate_session(session_id, Utc::now()), + Err(CredentialBrokerError::UnknownSession { .. }) + )); +} + +#[test] +fn reused_session_survives_the_first_of_two_leases_revoking() { + // Two mint_on_first_use calls for the identical (invocation, + // capability, account) binding — the documented cache-hit reuse path + // — must hand out two independent leases over the *same* session. + // Revoking the first-acquired lease must not revoke the session out + // from under the second lease still holding it; only releasing both + // should actually revoke it. + let broker = Arc::new(InMemoryCredentialBroker::new()); + let registry = CredentialPlaceholderRegistry::new(); + let (token, scope_a, account_id) = seeded(&broker, ®istry, "user-refcount"); + + let first_lease = broker + .mint_on_first_use( + &token, + session_request( + scope_a.clone(), + account_id.clone(), + "https://api.example.com/v1/x", + ), + ) + .unwrap(); + let second_lease = broker + .mint_on_first_use( + &token, + session_request(scope_a, account_id, "https://api.example.com/v1/x"), + ) + .unwrap(); + assert_eq!( + first_lease.session_id(), + second_lease.session_id(), + "cache-hit reuse must return the same underlying session" + ); + let session_id = first_lease.session_id(); + + first_lease.revoke(); + assert!( + broker.validate_session(session_id, Utc::now()).is_ok(), + "the second lease is still outstanding; revoking the first must not kill the shared session" + ); + + second_lease.revoke(); + assert!(matches!( + broker.validate_session(session_id, Utc::now()), + Err(CredentialBrokerError::UnknownSession { .. }) + )); +} + +#[test] +fn revoke_prunes_secondary_indices_so_placeholder_no_longer_resolves() { + // Revoking a session must not just make validate_session fail — it + // must also stop the placeholder from resolving to it at all, and + // must not leave stale bookkeeping (jit_minted / sessions_by_placeholder) + // behind for the life of the process. + let broker = Arc::new(InMemoryCredentialBroker::new()); + let registry = CredentialPlaceholderRegistry::new(); + let (token, scope_a, account_id) = seeded(&broker, ®istry, "user-prune"); + + let lease = broker + .mint_on_first_use( + &token, + session_request(scope_a.clone(), account_id, "https://api.example.com/v1/x"), + ) + .unwrap(); + assert!( + broker + .find_session_by_placeholder(&token, &scope_a) + .unwrap() + .is_some() + ); + + lease.revoke(); + + assert!( + broker + .find_session_by_placeholder(&token, &scope_a) + .unwrap() + .is_none(), + "placeholder must not resolve to a revoked session" + ); +} + +#[test] +fn find_session_by_placeholder_iterates_multiple_distinct_sessions_for_same_placeholder() { + // A placeholder can legitimately have more than one *distinct* + // session bound to it at once (e.g. two different accounts under + // one provider) — not just the same session reused, which is all + // the refcount tests above exercise. This pins that the + // scope-matching loop in `find_session_by_placeholder` actually + // walks past a non-matching candidate to find the right one. + let broker = Arc::new(InMemoryCredentialBroker::new()); + let registry = CredentialPlaceholderRegistry::new(); + let tenant = TenantId::new("tenant-a").unwrap(); + let provider = ExtensionId::new("google").unwrap(); + let user_a = UserId::new("user-multi-a").unwrap(); + let token = registry.get_or_create(&tenant, &user_a, &provider).unwrap(); + + let scope_a = sample_scope("tenant-a", "user-multi-a"); + let scope_b = sample_scope("tenant-a", "user-multi-b"); + let account_a = CredentialAccountId::new("google_account_a").unwrap(); + let account_b = CredentialAccountId::new("google_account_b").unwrap(); + broker + .put_account(sample_account( + scope_a.clone(), + account_a.clone(), + SecretHandle::new("key_a").unwrap(), + )) + .unwrap(); + broker + .put_account(sample_account( + scope_b.clone(), + account_b.clone(), + SecretHandle::new("key_b").unwrap(), + )) + .unwrap(); + + // Two distinct (invocation, capability, account) bindings mint two + // distinct sessions, both bound to the same placeholder token. + let lease_a = broker + .mint_on_first_use( + &token, + session_request(scope_a.clone(), account_a, "https://api.example.com/v1/x"), + ) + .unwrap(); + let lease_b = broker + .mint_on_first_use( + &token, + session_request(scope_b.clone(), account_b, "https://api.example.com/v1/x"), + ) + .unwrap(); + assert_ne!( + lease_a.session_id(), + lease_b.session_id(), + "two different accounts must mint two distinct sessions" + ); + + // Regardless of HashSet iteration order, querying with a specific + // scope must find that scope's session, not the other one bound to + // the same placeholder. + let found_b = broker + .find_session_by_placeholder(&token, &scope_b) + .unwrap() + .expect("scope_b's session must be found among the placeholder's bound sessions"); + assert_eq!(found_b.correlation_id(), lease_b.session_id()); + + let found_a = broker + .find_session_by_placeholder(&token, &scope_a) + .unwrap() + .expect("scope_a's session must be found among the placeholder's bound sessions"); + assert_eq!(found_a.correlation_id(), lease_a.session_id()); + + lease_a.revoke(); + lease_b.revoke(); +} + +#[test] +fn find_session_by_placeholder_skips_expired_and_exhausted_candidates_to_find_a_later_valid_one() { + // Complements the test above: that one pins scope-mismatch skipping. + // This pins that a *stale* candidate bound to the placeholder under + // the *same* scope — expired, or use-exhausted — cannot prevent a + // later, still-valid candidate bound to the identical placeholder + // and scope from being found. `sessions_by_placeholder` is a + // `HashSet`, so iteration order is unspecified; the loop in + // `find_session_by_placeholder` must skip every non-matching + // candidate regardless of where the valid one lands in that order. + let broker = Arc::new(InMemoryCredentialBroker::new()); + let registry = CredentialPlaceholderRegistry::new(); + let tenant = TenantId::new("tenant-a").unwrap(); + let provider = ExtensionId::new("google").unwrap(); + let user = UserId::new("user-stale-candidates").unwrap(); + let token = registry.get_or_create(&tenant, &user, &provider).unwrap(); + let scope = sample_scope("tenant-a", "user-stale-candidates"); + + let expired_account = CredentialAccountId::new("google_expired").unwrap(); + let exhausted_account = CredentialAccountId::new("google_exhausted").unwrap(); + let valid_account = CredentialAccountId::new("google_valid").unwrap(); + for (account_id, key) in [ + (&expired_account, "key_expired"), + (&exhausted_account, "key_exhausted"), + (&valid_account, "key_valid"), + ] { + broker + .put_account(sample_account( + scope.clone(), + account_id.clone(), + SecretHandle::new(key).unwrap(), + )) + .unwrap(); + } + + let mut expired_request = session_request( + scope.clone(), + expired_account, + "https://api.example.com/v1/x", + ); + expired_request.expires_at = Some(Utc::now() - chrono::Duration::seconds(1)); + let expired_lease = broker.mint_on_first_use(&token, expired_request).unwrap(); + + let mut exhausted_request = session_request( + scope.clone(), + exhausted_account, + "https://api.example.com/v1/x", + ); + exhausted_request.max_uses = Some(1); + let exhausted_lease = broker.mint_on_first_use(&token, exhausted_request).unwrap(); + broker + .consume_session_use(exhausted_lease.session_id(), Utc::now()) + .unwrap(); + + let valid_request = + session_request(scope.clone(), valid_account, "https://api.example.com/v1/x"); + let valid_lease = broker.mint_on_first_use(&token, valid_request).unwrap(); + + let found = broker + .find_session_by_placeholder(&token, &scope) + .unwrap() + .expect( + "a valid session must be found even with expired/exhausted candidates \ + also bound to the same placeholder and scope", + ); + assert_eq!(found.correlation_id(), valid_lease.session_id()); + + expired_lease.revoke(); + exhausted_lease.revoke(); + valid_lease.revoke(); +} + +#[test] +fn mint_on_first_use_remints_when_prior_jit_session_is_expired() { + // The jit_minted cache remembers a session id per (invocation, + // capability, account) key so re-use doesn't mint twice — but only + // while that session is still live. This pins the fallthrough + // branch: a cached key whose session has since expired must not be + // handed back; a fresh session must be minted instead. + let broker = Arc::new(InMemoryCredentialBroker::new()); + let registry = CredentialPlaceholderRegistry::new(); + let (token, scope_a, account_id) = seeded(&broker, ®istry, "user-remint"); + + let mut first_request = session_request( + scope_a.clone(), + account_id.clone(), + "https://api.example.com/v1/x", + ); + first_request.expires_at = Some(Utc::now() - chrono::Duration::seconds(1)); + let first_lease = broker.mint_on_first_use(&token, first_request).unwrap(); + let first_session_id = first_lease.session_id(); + assert!( + broker + .validate_session(first_session_id, Utc::now()) + .is_err(), + "sanity: the first session must already be expired" + ); + + // The jit_minted cache entry still points at the now-expired + // session; a second call for the identical key must not hand back a + // lease for the dead session — it must remint. + let second_request = session_request(scope_a, account_id, "https://api.example.com/v1/x"); + let second_lease = broker.mint_on_first_use(&token, second_request).unwrap(); + assert_ne!( + first_session_id, + second_lease.session_id(), + "an expired jit-cached session must not be reused" + ); + assert!( + broker + .validate_session(second_lease.session_id(), Utc::now()) + .is_ok(), + "the reminted session must be live" + ); + + first_lease.revoke(); + second_lease.revoke(); +} + +#[test] +fn finish_lease_recovers_from_poisoned_placeholder_index_without_leaking_session() { + // Regression test for the critical leak: `bind_placeholder_to_session` + // used to be the one index write in this module that didn't use + // `lock_or_recover`, so a poisoned `sessions_by_placeholder` mutex + // made it return `Err` — and `finish_lease` used to construct the + // `CredentialSessionLease` only *after* that fallible bind, so the + // `?` propagated with the session already refcounted by the caller + // but no lease ever constructed to drop and release that reference: a + // standing grant nobody could revoke. + // + // Since the session-lifecycle collapse (`sessions`, `jit_minted`, and + // `sessions_by_placeholder` now share one `session_state` mutex), + // there is no longer a separate index write that can fail after the + // session is already referenced: the whole mint-then-publish sequence + // in `mint_on_first_use` runs under one `lock_or_recover` guard and + // commits atomically. Poisoning that single mutex and confirming the + // returned lease still mints, binds, and fully revokes proves the + // same property this test always pinned, now trivially rather than + // by careful ordering. + let broker = Arc::new(InMemoryCredentialBroker::new()); + let registry = CredentialPlaceholderRegistry::new(); + let (token, scope_a, account_id) = seeded(&broker, ®istry, "user-poison"); + + // Poison `session_state` by panicking while holding its lock. + let poison_broker = Arc::clone(&broker); + let _ = std::panic::catch_unwind(AssertUnwindSafe(move || { + let _guard = poison_broker.session_state.lock().unwrap(); + panic!("deliberately poison session_state for regression test"); + })); + + let lease = broker + .mint_on_first_use( + &token, + session_request(scope_a.clone(), account_id, "https://api.example.com/v1/x"), + ) + .expect( + "mint_on_first_use must recover from a poisoned session_state lock, not fail \ + closed with an already-referenced, un-leased session", + ); + let session_id = lease.session_id(); + + assert!( + broker + .find_session_by_placeholder(&token, &scope_a) + .unwrap() + .is_some(), + "recovery from the poisoned lock must still actually bind the session" + ); + + // Revoking the lease must fully release it: no standing grant left behind. + lease.revoke(); + assert!( + matches!( + broker.validate_session(session_id, Utc::now()), + Err(CredentialBrokerError::UnknownSession { .. }) + ), + "the session must be genuinely revocable, not stranded past lease drop" + ); + assert!( + broker + .find_session_by_placeholder(&token, &scope_a) + .unwrap() + .is_none(), + "no dangling sessions_by_placeholder entry may survive revoke" + ); +} + +#[test] +fn registry_get_or_create_token_always_resolves_immediately() { + // Regression test: `get_or_create` used to publish into `by_owner`, + // drop that guard, and only then lock `by_token` separately. A + // concurrent call for the same triple in that window would take the + // early-return `by_owner` hit and hand out a token `resolve()` still + // answered `None` for — the registry's stated contract ("resolves a + // placeholder back to its owner") was only *eventually* true. Both + // maps now live behind one `Mutex`, so every token + // `get_or_create` returns already resolves by the time the call + // returns, with no window at all. + let registry = CredentialPlaceholderRegistry::new(); + let tenant = TenantId::new("tenant-a").unwrap(); + let user = UserId::new("user-resolve-atomic").unwrap(); + let provider = ExtensionId::new("google").unwrap(); + + let token = registry.get_or_create(&tenant, &user, &provider).unwrap(); + let owner = registry + .resolve(&token) + .unwrap() + .expect("a token returned by get_or_create must resolve immediately"); + assert_eq!(owner.tenant_id, tenant); + assert_eq!(owner.user_id, user); + assert_eq!(owner.provider_or_extension_id, provider); +} + +#[test] +fn mint_on_first_use_is_race_free_under_concurrent_first_use() { + // Regression test for a confirmed race: `jit_minted_session_id` + // (read) and `record_jit_mint` (write) used to lock/unlock + // `jit_minted` *separately*, so two threads racing on the identical + // `JitMintKey` could both observe "nothing minted yet" and both mint + // their own session — defeating the one-session-per-binding + // guarantee this module's doc comment asserts, and multiplying a + // `max_uses: Some(1)` budget. The fix holds one lock across the + // whole lookup-or-mint sequence in `mint_on_first_use`. + // + // This is only probabilistic at *catching* a reintroduced race — a + // `Barrier`-aligned start makes the race window likely to be hit, + // not guaranteed. But it has zero false-fail risk once the fix is + // in place: the lock enforces exclusion structurally, so a passing + // run here is a real guarantee, not a lucky one. + const THREAD_COUNT: usize = 32; + + let broker = Arc::new(InMemoryCredentialBroker::new()); + let registry = CredentialPlaceholderRegistry::new(); + let (token, scope_a, account_id) = seeded(&broker, ®istry, "user-race"); + let barrier = Arc::new(std::sync::Barrier::new(THREAD_COUNT)); + + let handles: Vec<_> = (0..THREAD_COUNT) + .map(|_| { + let broker = Arc::clone(&broker); + let token = token.clone(); + let scope_a = scope_a.clone(); + let account_id = account_id.clone(); + let barrier = Arc::clone(&barrier); + std::thread::spawn(move || { + let request = session_request(scope_a, account_id, "https://api.example.com/v1/x"); + barrier.wait(); + broker + .mint_on_first_use(&token, request) + .expect("mint_on_first_use must succeed for every racing thread") + }) + }) + .collect(); + + let leases: Vec = handles + .into_iter() + .map(|handle| { + handle + .join() + .expect("mint_on_first_use thread must not panic") + }) + .collect(); + + let distinct_session_ids: std::collections::HashSet<_> = + leases.iter().map(|lease| lease.session_id()).collect(); + assert_eq!( + distinct_session_ids.len(), + 1, + "every thread racing on the identical (invocation, capability, account) binding \ + must be handed a lease for the exact same session, not one each" + ); + + for lease in leases { + lease.revoke(); + } +} + +fn seeded( + broker: &Arc, + registry: &CredentialPlaceholderRegistry, + user: &str, +) -> ( + CredentialPlaceholderToken, + ResourceScope, + CredentialAccountId, +) { + let tenant = TenantId::new("tenant-a").unwrap(); + let user_id = UserId::new(user).unwrap(); + let provider = ExtensionId::new("google").unwrap(); + let token = registry + .get_or_create(&tenant, &user_id, &provider) + .unwrap(); + let caller_scope = sample_scope("tenant-a", user); + let account_id = CredentialAccountId::new("google_prod").unwrap(); + broker + .put_account(sample_account( + caller_scope.clone(), + account_id.clone(), + SecretHandle::new("google_key").unwrap(), + )) + .unwrap(); + (token, caller_scope, account_id) +} From 5d552254ceac9b91ef71f88ec844d7a5eb600313 Mon Sep 17 00:00:00 2001 From: Henry Park Date: Sun, 26 Jul 2026 20:37:04 -0700 Subject: [PATCH 14/14] fix(secrets): move placeholder tests to conform to test-path convention placeholder_tests.rs didn't match scripts/check_no_panics.py's is_test_only_path() exemption (src/**/tests.rs or src/**/tests/*.rs), so its test-fixture .unwrap() calls were scanned as production code, failing "No panics in production code" CI with 13 violations. Move it to placeholder/tests.rs, which matches the tests.rs filename exemption, instead of loosening the shared repo-wide panic scanner. Co-Authored-By: Claude Opus 5 (1M context) --- crates/ironclaw_secrets/src/placeholder.rs | 2 +- .../src/{placeholder_tests.rs => placeholder/tests.rs} | 0 2 files changed, 1 insertion(+), 1 deletion(-) rename crates/ironclaw_secrets/src/{placeholder_tests.rs => placeholder/tests.rs} (100%) diff --git a/crates/ironclaw_secrets/src/placeholder.rs b/crates/ironclaw_secrets/src/placeholder.rs index 190e6c25222..5bd725b0edd 100644 --- a/crates/ironclaw_secrets/src/placeholder.rs +++ b/crates/ironclaw_secrets/src/placeholder.rs @@ -610,5 +610,5 @@ fn bind_placeholder_to_session( } #[cfg(test)] -#[path = "placeholder_tests.rs"] +#[path = "placeholder/tests.rs"] mod tests; diff --git a/crates/ironclaw_secrets/src/placeholder_tests.rs b/crates/ironclaw_secrets/src/placeholder/tests.rs similarity index 100% rename from crates/ironclaw_secrets/src/placeholder_tests.rs rename to crates/ironclaw_secrets/src/placeholder/tests.rs