From 260dc2b7e12c945a2f40737e170a7d022f966e10 Mon Sep 17 00:00:00 2001 From: Tinson Lai Date: Mon, 27 Apr 2026 05:41:10 +0000 Subject: [PATCH] feat(provider): per-credential custom placeholders and passthrough opt-in MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds two new optional fields to Provider in proto/datamodel.proto: - credential_placeholders: map keyed by credential env var name. When set, the supervisor injects the literal override string into the agent's environment instead of the canonical openshell:resolve:env: placeholder, and registers the override with the secret resolver so the L7 proxy substitutes the real value on the wire by exact-match lookup. Used for SDKs that validate credential format in-process before any network call (e.g. Slack's xoxb- prefix check, AWS access-key prefixes, OAuth libraries). - passthrough_credentials: repeated string of env var names whose REAL value is injected directly into the agent process, bypassing placeholder/L7-proxy substitution entirely. Required for credentials consumed in-process (signature computation, checksum verification, non-HTTP transports) where no on-the-wire substitution is possible. Drops the "agent never sees the real value" invariant for the listed keys; treated as a per-credential security trade-off. A key MUST NOT appear in both maps; validation rejects overlaps and keys not present in `credentials`. Custom-format placeholders are substituted in HTTP header values (full-value, Bearer prefix, Basic-decoded) and query parameter values, but NOT in URL path segments — path extraction relies on the canonical env-var-name grammar. The fail-closed post-rewrite scan covers both canonical and custom-registered placeholders, so an override that ends up in a path is rejected before egress. Server changes (crates/openshell-server): - grpc/provider.rs: round-trip the two new fields on create/update/get, validate non-empty / no CR/LF/NUL / no key overlap, plumb through to GetSandboxProviderEnvironmentResponse as placeholder_overrides and passthrough_keys. - grpc/validation.rs: shared helpers for placeholder string validation and overlap detection. - grpc/policy.rs, inference.rs: respect passthrough keys when assembling resolver registrations and policy rules. Sandbox changes (crates/openshell-sandbox): - grpc_client.rs: surface placeholder_overrides and passthrough_keys on the supervisor side. - secrets.rs: register override strings with the resolver, inject overrides (or real passthrough values) into the child environment in place of canonical placeholders, with the fail-closed invariant preserved for canonical and custom forms. - lib.rs: wire the new env construction into the supervisor launch path. CLI changes (crates/openshell-cli): - main.rs, run.rs: add per-credential --placeholder KEY=VALUE and --passthrough KEY flags on provider create/update; surface readback in provider get/list output. - integration tests: cover round-trip, overlap rejection, empty/CRLF override rejection, passthrough opt-in. TUI: render override and passthrough markers in the provider detail view. Architecture docs (architecture/sandbox-providers.md): new "Custom Placeholder Formats" section covering motivation, semantics, substitution scope, validation rules, and the security trade-off vs the canonical form. Motivation: groundwork for NemoClaw#2031, whose Slack/Discord/Telegram provider integrations need SDK-format-passing placeholders (Slack's xoxb- prefix check) and in-process passthrough (Slack signing-secret HMAC, Discord interaction-signature verification) that the canonical placeholder form cannot satisfy. Fixes #894 Signed-off-by: Tinson Lai --- architecture/sandbox-providers.md | 65 ++- crates/openshell-cli/src/main.rs | 46 +++ crates/openshell-cli/src/run.rs | 91 +++++ .../tests/ensure_providers_integration.rs | 26 ++ .../tests/provider_commands_integration.rs | 24 ++ crates/openshell-sandbox/src/grpc_client.rs | 32 +- crates/openshell-sandbox/src/lib.rs | 57 +-- crates/openshell-sandbox/src/secrets.rs | 383 +++++++++++++++++- crates/openshell-server/src/grpc/policy.rs | 10 +- crates/openshell-server/src/grpc/provider.rs | 288 ++++++++++++- .../openshell-server/src/grpc/validation.rs | 46 +++ crates/openshell-server/src/inference.rs | 6 + crates/openshell-tui/src/lib.rs | 4 + proto/datamodel.proto | 21 + proto/openshell.proto | 12 + 15 files changed, 1038 insertions(+), 73 deletions(-) diff --git a/architecture/sandbox-providers.md b/architecture/sandbox-providers.md index 088bd7592..e98910017 100644 --- a/architecture/sandbox-providers.md +++ b/architecture/sandbox-providers.md @@ -35,6 +35,13 @@ Provider is defined in `proto/datamodel.proto`: - `type`: canonical provider slug (`claude`, `gitlab`, `github`, etc.) - `credentials`: `map` for secret values - `config`: `map` for non-secret settings +- `credential_placeholders`: optional `map` keyed by the same + env var name as `credentials`. When present, the supervisor injects this + literal string into the child environment instead of the canonical + `openshell:resolve:env:` placeholder. Used to satisfy SDKs that + validate credential format in-process before any network call (for + example, Slack's `xoxb-` / `xapp-` prefix check). See + [Custom Placeholder Formats](#custom-placeholder-formats) below. The gRPC surface is defined in `proto/openshell.proto`: @@ -320,10 +327,12 @@ request is forwarded upstream. Placeholders are resolved in four locations: This applies to forward-proxy HTTP requests, L7-inspected REST requests inside CONNECT tunnels, and credential-injection-only passthrough relays on TLS-terminated connections. -All rewriting fails closed: if any `openshell:resolve:env:*` placeholder is detected but -cannot be resolved, the proxy rejects the request with HTTP 500 instead of forwarding the -raw placeholder upstream. Resolved secret values are validated for prohibited control -characters (CR, LF, null byte) to prevent header injection (CWE-113). Path segment +All rewriting fails closed: if any registered placeholder string is detected in the output +but cannot be resolved, the proxy rejects the request with HTTP 500 instead of forwarding +the raw placeholder upstream. The fail-closed scan covers both canonical +`openshell:resolve:env:*` placeholders and custom-format placeholders registered via +`credential_placeholders` (see below). Resolved secret values are validated for prohibited +control characters (CR, LF, null byte) to prevent header injection (CWE-113). Path segment credentials are additionally validated to reject traversal sequences, path separators, and URI delimiters (CWE-22). @@ -331,6 +340,54 @@ The real secret value remains in supervisor memory only; it is not re-injected i child process environment. See [Credential injection](sandbox.md#credential-injection) for the full implementation details, encoding rules, and security properties. +### Custom Placeholder Formats + +The default canonical placeholder `openshell:resolve:env:` is distinctive and easy to +detect, but some SDKs validate credential format in-process before the request reaches the +network — and therefore before the proxy can substitute the real value. Slack's +`@slack/web-api` rejects bot tokens that do not start with `xoxb-`; AWS, Google, and several +OAuth libraries do similar prefix or charset checks. For those SDKs the canonical placeholder +fails validation and the request is never made. + +To handle this, a provider may declare a `credential_placeholders` entry per credential +env var. When set: + +- the supervisor injects the literal override string into the child environment instead of + the canonical placeholder; +- the supervisor's resolver registers ` -> ` so the L7 proxy can + substitute it on the wire by exact-match lookup; +- the fail-closed scan also rejects requests in which the override survives rewriting. + +The override string must be: + +- non-empty; +- free of CR, LF, and NUL bytes (HTTP framing safety); +- distinctive enough not to collide with legitimate user data on the wire — the proxy + substitutes by exact-match against the registered string, so any header value, query + parameter, or path segment that exactly equals the override is replaced. + +**Substitution scope for custom-format placeholders.** Custom-format placeholders are +substituted in: + +- HTTP header values (full-value, `Bearer ` prefix, and Basic-auth-decoded forms); +- URL query parameter values. + +They are **not** substituted in URL path segments. Path-segment extraction relies on the +canonical env-var-name grammar (`[A-Za-z_][A-Za-z0-9_]*`) to find placeholder boundaries +inside concatenated paths like Telegram's `/bot/sendMessage`; arbitrary custom shapes +do not fit that grammar. If a credential is consumed in a path-embedded position, leave its +`credential_placeholders` entry unset so the canonical placeholder applies. Custom-format +placeholders that end up in a path are caught by the fail-closed post-rewrite scan and +rejected; they never leak upstream. + +**When to use it.** Custom placeholders are a security trade-off. The canonical form +guarantees that no real-credential-shaped string appears in the agent process at all. +Custom placeholders look like real credentials but contain no real value, which is what +the SDK's format check needs to pass — and is still useless to anything that obtains it +(prompt injection, crash dumps, accidental logs) because the L7 proxy is the only path +that can substitute it. Use a custom placeholder only when an in-process SDK forces the +issue; otherwise the canonical form is preferable. + ### End-to-End Flow ```text diff --git a/crates/openshell-cli/src/main.rs b/crates/openshell-cli/src/main.rs index 9a7c41216..1b62a0c99 100644 --- a/crates/openshell-cli/src/main.rs +++ b/crates/openshell-cli/src/main.rs @@ -674,6 +674,22 @@ enum ProviderCommands { /// Provider config key/value pair. #[arg(long = "config", value_name = "KEY=VALUE")] config: Vec, + + /// Per-credential format-preserving placeholder shown to the agent + /// process instead of the canonical `openshell:resolve:env:`. + /// Use this when an SDK rejects the canonical placeholder by format + /// (for example, Slack expects `xoxb-` / `xapp-` prefixes). The L7 + /// proxy still substitutes the real value on the wire. + #[arg(long = "credential-placeholder", value_name = "KEY=VALUE")] + credential_placeholders: Vec, + + /// Mark a credential as passthrough: inject the REAL value into the + /// agent process, with no placeholder and no L7 proxy substitution. + /// Use only when the SDK consumes the credential in-process and no + /// on-the-wire substitution is possible. This drops the + /// "agent never sees the real value" invariant for the listed key. + #[arg(long = "credential-passthrough", value_name = "KEY")] + credential_passthrough: Vec, }, /// Fetch a provider by name. @@ -722,6 +738,26 @@ enum ProviderCommands { /// Provider config key/value pair. #[arg(long = "config", value_name = "KEY=VALUE")] config: Vec, + + /// Per-credential format-preserving placeholder shown to the agent + /// process. Pass `KEY=` (empty value) to remove an existing override. + #[arg(long = "credential-placeholder", value_name = "KEY=VALUE")] + credential_placeholders: Vec, + + /// Mark credentials as passthrough (inject real value into the agent + /// process). Repeats build the new full list, replacing any prior + /// passthrough configuration. Pass `--clear-credential-passthrough` + /// to clear without replacing. + #[arg( + long = "credential-passthrough", + value_name = "KEY", + conflicts_with = "clear_credential_passthrough" + )] + credential_passthrough: Vec, + + /// Clear the passthrough credential list on this provider. + #[arg(long)] + clear_credential_passthrough: bool, }, /// Delete providers by name. @@ -2564,6 +2600,8 @@ async fn main() -> Result<()> { from_existing, credentials, config, + credential_placeholders, + credential_passthrough, } => { run::provider_create( endpoint, @@ -2572,6 +2610,8 @@ async fn main() -> Result<()> { from_existing, &credentials, &config, + &credential_placeholders, + &credential_passthrough, &tls, ) .await?; @@ -2591,6 +2631,9 @@ async fn main() -> Result<()> { from_existing, credentials, config, + credential_placeholders, + credential_passthrough, + clear_credential_passthrough, } => { run::provider_update( endpoint, @@ -2598,6 +2641,9 @@ async fn main() -> Result<()> { from_existing, &credentials, &config, + &credential_placeholders, + &credential_passthrough, + clear_credential_passthrough, &tls, ) .await?; diff --git a/crates/openshell-cli/src/run.rs b/crates/openshell-cli/src/run.rs index ba25488b8..d29e8f775 100644 --- a/crates/openshell-cli/src/run.rs +++ b/crates/openshell-cli/src/run.rs @@ -3324,6 +3324,8 @@ async fn auto_create_provider( r#type: provider_type.to_string(), credentials: discovered.credentials.clone(), config: discovered.config.clone(), + credential_placeholders: std::collections::HashMap::new(), + passthrough_credentials: Vec::new(), }), }; @@ -3360,6 +3362,7 @@ async fn auto_create_provider( r#type: provider_type.to_string(), credentials: discovered.credentials.clone(), config: discovered.config.clone(), + credential_placeholders: std::collections::HashMap::new(), }), }; @@ -3420,6 +3423,51 @@ fn parse_key_value_pairs(items: &[String], flag: &str) -> Result Result> { + let mut seen = std::collections::HashSet::new(); + let mut out = Vec::with_capacity(items.len()); + for item in items { + let key = item.trim(); + if key.is_empty() { + return Err(miette::miette!( + "--credential-passthrough key cannot be empty" + )); + } + if !seen.insert(key.to_string()) { + continue; + } + out.push(key.to_string()); + } + Ok(out) +} + +fn validate_placeholder_and_passthrough_consistency( + credentials: &HashMap, + placeholders: &HashMap, + passthrough: &[String], +) -> Result<()> { + for key in placeholders.keys() { + if !credentials.contains_key(key) { + return Err(miette::miette!( + "--credential-placeholder {key} has no matching --credential" + )); + } + } + for key in passthrough { + if !credentials.contains_key(key) { + return Err(miette::miette!( + "--credential-passthrough {key} has no matching --credential" + )); + } + if placeholders.contains_key(key) { + return Err(miette::miette!( + "credential '{key}' cannot be both --credential-placeholder and --credential-passthrough" + )); + } + } + Ok(()) +} + fn parse_credential_pairs(items: &[String]) -> Result> { let mut map = HashMap::new(); @@ -3463,6 +3511,8 @@ pub async fn provider_create( from_existing: bool, credentials: &[String], config: &[String], + credential_placeholders: &[String], + credential_passthrough: &[String], tls: &TlsOptions, ) -> Result<()> { if from_existing && !credentials.is_empty() { @@ -3479,6 +3529,9 @@ pub async fn provider_create( let mut credential_map = parse_credential_pairs(credentials)?; let mut config_map = parse_key_value_pairs(config, "--config")?; + let placeholder_map = + parse_key_value_pairs(credential_placeholders, "--credential-placeholder")?; + let passthrough_list = parse_passthrough_keys(credential_passthrough)?; if from_existing { let registry = ProviderRegistry::new(); @@ -3506,6 +3559,12 @@ pub async fn provider_create( )); } + validate_placeholder_and_passthrough_consistency( + &credential_map, + &placeholder_map, + &passthrough_list, + )?; + let response = client .create_provider(CreateProviderRequest { provider: Some(Provider { @@ -3514,6 +3573,8 @@ pub async fn provider_create( r#type: provider_type, credentials: credential_map, config: config_map, + credential_placeholders: placeholder_map, + passthrough_credentials: passthrough_list, }), }) .await @@ -3640,6 +3701,9 @@ pub async fn provider_update( from_existing: bool, credentials: &[String], config: &[String], + credential_placeholders: &[String], + credential_passthrough: &[String], + clear_credential_passthrough: bool, tls: &TlsOptions, ) -> Result<()> { if from_existing && !credentials.is_empty() { @@ -3652,6 +3716,14 @@ pub async fn provider_update( let mut credential_map = parse_credential_pairs(credentials)?; let mut config_map = parse_key_value_pairs(config, "--config")?; + let placeholder_map = + parse_key_value_pairs(credential_placeholders, "--credential-placeholder")?; + let mut passthrough_list = parse_passthrough_keys(credential_passthrough)?; + if clear_credential_passthrough { + // Sentinel: a single empty-string entry tells the server to clear + // the existing passthrough list. See merge_passthrough on the server. + passthrough_list = vec![String::new()]; + } if from_existing { // Fetch the existing provider to discover its type for credential lookup. @@ -3684,6 +3756,23 @@ pub async fn provider_update( } } + // Best-effort cross-field validation: the server's authoritative check + // runs after the merge against the persisted provider, but failing fast + // here on the obvious case (the same key on both flags) gives a better + // error than waiting for the round trip. + if !placeholder_map.is_empty() && !passthrough_list.is_empty() { + for key in &passthrough_list { + if key.is_empty() { + continue; + } + if placeholder_map.contains_key(key) { + return Err(miette::miette!( + "credential '{key}' cannot be both --credential-placeholder and --credential-passthrough" + )); + } + } + } + let response = client .update_provider(UpdateProviderRequest { provider: Some(Provider { @@ -3692,6 +3781,8 @@ pub async fn provider_update( r#type: String::new(), credentials: credential_map, config: config_map, + credential_placeholders: placeholder_map, + passthrough_credentials: passthrough_list, }), }) .await diff --git a/crates/openshell-cli/tests/ensure_providers_integration.rs b/crates/openshell-cli/tests/ensure_providers_integration.rs index a3dd6826f..8353e7845 100644 --- a/crates/openshell-cli/tests/ensure_providers_integration.rs +++ b/crates/openshell-cli/tests/ensure_providers_integration.rs @@ -110,6 +110,8 @@ impl TestOpenShell { r#type: provider_type.to_string(), credentials: HashMap::new(), config: HashMap::new(), + credential_placeholders: HashMap::new(), + passthrough_credentials: Vec::new(), }, ); } @@ -273,12 +275,36 @@ impl OpenShell for TestOpenShell { } base }; + let merge_passthrough = |existing: Vec, incoming: Vec| -> Vec { + if incoming.is_empty() { + return existing; + } + let mut seen = std::collections::HashSet::new(); + let mut out = Vec::with_capacity(incoming.len()); + for k in incoming { + if k.is_empty() { + continue; + } + if seen.insert(k.clone()) { + out.push(k); + } + } + out + }; let updated = Provider { id: existing.id, name: provider.name, r#type: existing.r#type, credentials: merge(existing.credentials, provider.credentials), config: merge(existing.config, provider.config), + credential_placeholders: merge( + existing.credential_placeholders, + provider.credential_placeholders, + ), + passthrough_credentials: merge_passthrough( + existing.passthrough_credentials, + provider.passthrough_credentials, + ), }; providers.insert(updated.name.clone(), updated.clone()); Ok(Response::new(ProviderResponse { diff --git a/crates/openshell-cli/tests/provider_commands_integration.rs b/crates/openshell-cli/tests/provider_commands_integration.rs index dc6ec9d4c..b908b437e 100644 --- a/crates/openshell-cli/tests/provider_commands_integration.rs +++ b/crates/openshell-cli/tests/provider_commands_integration.rs @@ -227,12 +227,36 @@ impl OpenShell for TestOpenShell { } base }; + let merge_passthrough = |existing: Vec, incoming: Vec| -> Vec { + if incoming.is_empty() { + return existing; + } + let mut seen = std::collections::HashSet::new(); + let mut out = Vec::with_capacity(incoming.len()); + for k in incoming { + if k.is_empty() { + continue; + } + if seen.insert(k.clone()) { + out.push(k); + } + } + out + }; let updated = Provider { id: existing.id, name: provider.name, r#type: existing.r#type, credentials: merge(existing.credentials, provider.credentials), config: merge(existing.config, provider.config), + credential_placeholders: merge( + existing.credential_placeholders, + provider.credential_placeholders, + ), + passthrough_credentials: merge_passthrough( + existing.passthrough_credentials, + provider.passthrough_credentials, + ), }; providers.insert(updated.name.clone(), updated.clone()); Ok(Response::new(ProviderResponse { diff --git a/crates/openshell-sandbox/src/grpc_client.rs b/crates/openshell-sandbox/src/grpc_client.rs index 0af6476c5..0fca8abee 100644 --- a/crates/openshell-sandbox/src/grpc_client.rs +++ b/crates/openshell-sandbox/src/grpc_client.rs @@ -191,15 +191,30 @@ pub async fn sync_policy(endpoint: &str, sandbox: &str, policy: &ProtoSandboxPol sync_policy_with_client(&mut client, sandbox, policy).await } +/// Provider environment fetched from the gateway: credential values plus any +/// per-credential placeholder overrides and passthrough opt-ins. +#[derive(Debug, Default, Clone)] +pub struct ProviderEnvironment { + /// Real credential values keyed by env var name. + pub env: HashMap, + /// Optional placeholder overrides keyed by env var name. Missing entries + /// fall back to the canonical `openshell:resolve:env:` form. + pub placeholder_overrides: HashMap, + /// Env var names whose REAL credential value is injected directly into + /// the agent process, with no placeholder substitution. The supervisor + /// must NOT register passthrough keys with the secret resolver. + pub passthrough_keys: Vec, +} + /// Fetch provider environment variables for a sandbox from OpenShell server via gRPC. /// -/// Returns a map of environment variable names to values derived from provider -/// credentials configured on the sandbox. Returns an empty map if the sandbox +/// Returns the credential map, per-credential placeholder overrides, and the +/// list of passthrough credential keys. Returns empty values if the sandbox /// has no providers or the call fails. pub async fn fetch_provider_environment( endpoint: &str, sandbox_id: &str, -) -> Result> { +) -> Result { debug!(endpoint = %endpoint, sandbox_id = %sandbox_id, "Fetching provider environment"); let mut client = connect(endpoint).await?; @@ -209,9 +224,14 @@ pub async fn fetch_provider_environment( sandbox_id: sandbox_id.to_string(), }) .await - .into_diagnostic()?; - - Ok(response.into_inner().environment) + .into_diagnostic()? + .into_inner(); + + Ok(ProviderEnvironment { + env: response.environment, + placeholder_overrides: response.placeholder_overrides, + passthrough_keys: response.passthrough_keys, + }) } /// A reusable gRPC client for the OpenShell service. diff --git a/crates/openshell-sandbox/src/lib.rs b/crates/openshell-sandbox/src/lib.rs index 34ee80bb5..64c3c80e6 100644 --- a/crates/openshell-sandbox/src/lib.rs +++ b/crates/openshell-sandbox/src/lib.rs @@ -267,41 +267,48 @@ pub async fn run_sandbox( // Fetch provider environment variables from the server. // This is done after loading the policy so the sandbox can still start // even if provider env fetch fails (graceful degradation). - let provider_env = if let (Some(id), Some(endpoint)) = (&sandbox_id, &openshell_endpoint) { - match grpc_client::fetch_provider_environment(endpoint, id).await { - Ok(env) => { - ocsf_emit!( + let provider_environment = + if let (Some(id), Some(endpoint)) = (&sandbox_id, &openshell_endpoint) { + match grpc_client::fetch_provider_environment(endpoint, id).await { + Ok(env) => { + ocsf_emit!( ConfigStateChangeBuilder::new(ocsf_ctx()) .severity(SeverityId::Informational) .status(StatusId::Success) .state(StateId::Enabled, "loaded") .message(format!( - "Fetched provider environment [env_count:{}]", - env.len() + "Fetched provider environment [env_count:{} placeholder_overrides:{} passthrough:{}]", + env.env.len(), + env.placeholder_overrides.len(), + env.passthrough_keys.len(), )) .build() ); - env - } - Err(e) => { - ocsf_emit!( - ConfigStateChangeBuilder::new(ocsf_ctx()) - .severity(SeverityId::Medium) - .status(StatusId::Failure) - .state(StateId::Other, "degraded") - .message(format!( - "Failed to fetch provider environment, continuing without: {e}" - )) - .build() - ); - std::collections::HashMap::new() + env + } + Err(e) => { + ocsf_emit!( + ConfigStateChangeBuilder::new(ocsf_ctx()) + .severity(SeverityId::Medium) + .status(StatusId::Failure) + .state(StateId::Other, "degraded") + .message(format!( + "Failed to fetch provider environment, continuing without: {e}" + )) + .build() + ); + grpc_client::ProviderEnvironment::default() + } } - } - } else { - std::collections::HashMap::new() - }; + } else { + grpc_client::ProviderEnvironment::default() + }; - let (provider_env, secret_resolver) = SecretResolver::from_provider_env(provider_env); + let (provider_env, secret_resolver) = SecretResolver::from_provider_env_with_modes( + provider_environment.env, + provider_environment.placeholder_overrides, + &provider_environment.passthrough_keys, + ); let secret_resolver = secret_resolver.map(Arc::new); // Create identity cache for SHA256 TOFU when OPA is active diff --git a/crates/openshell-sandbox/src/secrets.rs b/crates/openshell-sandbox/src/secrets.rs index a27537c91..624629470 100644 --- a/crates/openshell-sandbox/src/secrets.rs +++ b/crates/openshell-sandbox/src/secrets.rs @@ -5,9 +5,13 @@ use base64::Engine as _; use std::collections::HashMap; use std::fmt; +/// Default placeholder prefix used when a credential has no override. +/// +/// The full placeholder grammar is `openshell:resolve:env:`, +/// where the env var name matches `[A-Za-z_][A-Za-z0-9_]*`. const PLACEHOLDER_PREFIX: &str = "openshell:resolve:env:"; -/// Public access to the placeholder prefix for fail-closed scanning in other modules. +/// Public access to the default placeholder prefix for fail-closed scanning in other modules. pub(crate) const PLACEHOLDER_PREFIX_PUBLIC: &str = PLACEHOLDER_PREFIX; /// Characters that are valid in an env var key name (used to extract @@ -64,26 +68,127 @@ pub(crate) struct RewriteTargetResult { #[derive(Debug, Clone, Default)] pub struct SecretResolver { by_placeholder: HashMap, + /// Whether any registered placeholder uses a non-canonical (custom) form. + /// When false, fast-path scans can short-circuit on the canonical prefix. + has_custom_placeholders: bool, } impl SecretResolver { + /// Build a resolver from credential values, using only canonical + /// placeholders. Test-only convenience wrapper around + /// [`Self::from_provider_env_with_modes`]. + #[cfg(test)] pub(crate) fn from_provider_env( provider_env: HashMap, + ) -> (HashMap, Option) { + Self::from_provider_env_with_modes(provider_env, HashMap::new(), &[]) + } + + /// Build a resolver from credential values and optional per-credential + /// placeholder overrides. Test-only convenience wrapper around + /// [`Self::from_provider_env_with_modes`] for tests that pre-date the + /// passthrough opt-in. + #[cfg(test)] + pub(crate) fn from_provider_env_with_overrides( + provider_env: HashMap, + placeholder_overrides: HashMap, + ) -> (HashMap, Option) { + Self::from_provider_env_with_modes(provider_env, placeholder_overrides, &[]) + } + + /// Build a resolver from credential values, optional per-credential + /// placeholder overrides, and an opt-in list of passthrough credentials. + /// + /// For each credential, the value placed in the child environment is: + /// - **Passthrough** (`key` appears in `passthrough_keys`): the REAL + /// credential value. The credential is NOT registered with the + /// resolver — there is no on-the-wire placeholder for it. Use this + /// only for credentials that an SDK consumes in-process and for which + /// no L7 substitution is possible. + /// - **Custom placeholder** (`placeholder_overrides` has a valid entry): + /// the override string. The resolver maps `override -> real_value` + /// for exact-match substitution at the L7 proxy. + /// - **Default**: the canonical `openshell:resolve:env:` placeholder. + /// The resolver maps it to the real value for substitution. + /// + /// Override strings that are empty or contain prohibited characters + /// (CR, LF, NUL) are silently dropped — the credential falls back to the + /// canonical placeholder. Validation happens upstream at the gateway, so + /// this is a defence-in-depth check. Passthrough takes precedence over + /// any placeholder override on the same key. + pub(crate) fn from_provider_env_with_modes( + provider_env: HashMap, + placeholder_overrides: HashMap, + passthrough_keys: &[String], ) -> (HashMap, Option) { if provider_env.is_empty() { return (HashMap::new(), None); } + let passthrough: std::collections::HashSet<&str> = + passthrough_keys.iter().map(String::as_str).collect(); + let mut child_env = HashMap::with_capacity(provider_env.len()); let mut by_placeholder = HashMap::with_capacity(provider_env.len()); + let mut has_custom_placeholders = false; for (key, value) in provider_env { - let placeholder = placeholder_for_env_key(&key); + if passthrough.contains(key.as_str()) { + // Inject the real value directly. Do NOT register with the + // resolver — passthrough credentials have no placeholder + // and must not be substituted by the L7 proxy. + child_env.insert(key, value); + continue; + } + let placeholder = match placeholder_overrides.get(&key) { + Some(override_value) if is_valid_placeholder(override_value) => { + has_custom_placeholders = true; + override_value.clone() + } + _ => placeholder_for_env_key(&key), + }; child_env.insert(key, placeholder.clone()); by_placeholder.insert(placeholder, value); } - (child_env, Some(Self { by_placeholder })) + if by_placeholder.is_empty() { + // All credentials were passthrough; no resolver needed. + return (child_env, None); + } + + ( + child_env, + Some(Self { + by_placeholder, + has_custom_placeholders, + }), + ) + } + + /// Returns true if any registered placeholder is a custom (non-canonical) + /// form. Test-only — production code reads `self.has_custom_placeholders` + /// directly inside [`Self::contains_any_placeholder`]. + #[cfg(test)] + pub(crate) fn has_custom_placeholders(&self) -> bool { + self.has_custom_placeholders + } + + /// Return true if the given haystack contains any registered placeholder + /// string, or any canonical-form placeholder (the latter so that scans + /// catch unresolved canonical placeholders even when the resolver has + /// only custom-form entries). + pub(crate) fn contains_any_placeholder(&self, haystack: &str) -> bool { + if haystack.contains(PLACEHOLDER_PREFIX) { + return true; + } + if self.has_custom_placeholders { + for placeholder in self.by_placeholder.keys() { + if !placeholder.starts_with(PLACEHOLDER_PREFIX) && haystack.contains(placeholder) { + return true; + } + } + } + false } /// Resolve a placeholder string to the real secret value. @@ -142,8 +247,8 @@ impl SecretResolver { let decoded_bytes = b64.decode(encoded.trim()).ok()?; let decoded = std::str::from_utf8(&decoded_bytes).ok()?; - // Check if the decoded string contains any placeholder - if !decoded.contains(PLACEHOLDER_PREFIX) { + // Check if the decoded string contains any registered placeholder. + if !self.contains_any_placeholder(decoded) { return None; } @@ -176,6 +281,14 @@ pub(crate) fn placeholder_for_env_key(key: &str) -> String { format!("{PLACEHOLDER_PREFIX}{key}") } +/// Reject placeholder strings that would corrupt HTTP framing or that an empty +/// string would silently disable. Mirrors the gateway-side check so that the +/// supervisor never registers an unsafe override even if the gateway is +/// outdated or misbehaves. +fn is_valid_placeholder(value: &str) -> bool { + !value.is_empty() && !value.bytes().any(|b| matches!(b, b'\r' | b'\n' | 0)) +} + // --------------------------------------------------------------------------- // Secret validation (F1 — CWE-113) // --------------------------------------------------------------------------- @@ -352,8 +465,11 @@ fn rewrite_request_line( } }; - // Only rewrite if the URI contains a placeholder - if !uri.contains(PLACEHOLDER_PREFIX) { + // Only rewrite if the URI contains a registered placeholder (canonical + // or custom). Custom placeholders only resolve in query params, but the + // fast-path enters processing so the post-scan can fail closed on a + // custom placeholder that leaks into a URL path. + if !resolver.contains_any_placeholder(uri) { return Ok(RewriteLineResult { line: line.to_string(), redacted_target: None, @@ -511,7 +627,7 @@ fn rewrite_uri_query_params( query: &str, resolver: &SecretResolver, ) -> Result, UnresolvedPlaceholderError> { - if !query.contains(PLACEHOLDER_PREFIX) { + if !resolver.contains_any_placeholder(query) { return Ok(None); } @@ -608,16 +724,17 @@ pub(crate) fn rewrite_http_header_block( output.extend_from_slice(&raw[header_end..]); // Fail-closed scan: check for any remaining unresolved placeholders - // in both raw form and percent-decoded form of the output header block. + // (canonical or custom) in both raw form and percent-decoded form of the + // output header block. let output_header = String::from_utf8_lossy(&output[..output.len().min(header_end + 256)]); - if output_header.contains(PLACEHOLDER_PREFIX) { + if resolver.contains_any_placeholder(&output_header) { return Err(UnresolvedPlaceholderError { location: "header" }); } // Also check percent-decoded form of the request line (F5 — encoded placeholder bypass) let rewritten_rl = output_header.split("\r\n").next().unwrap_or(""); let decoded_rl = percent_decode(rewritten_rl); - if decoded_rl.contains(PLACEHOLDER_PREFIX) { + if resolver.contains_any_placeholder(&decoded_rl) { return Err(UnresolvedPlaceholderError { location: "path" }); } @@ -646,10 +763,12 @@ pub(crate) fn rewrite_target_for_eval( target: &str, resolver: &SecretResolver, ) -> Result { - if !target.contains(PLACEHOLDER_PREFIX) { - // Also check percent-decoded form + if !resolver.contains_any_placeholder(target) { + // Also check percent-decoded form for canonical placeholders + // (encoded-bypass guard); custom placeholders contain no special + // ASCII so percent-encoding does not alter them. let decoded = percent_decode(target); - if decoded.contains(PLACEHOLDER_PREFIX) { + if resolver.contains_any_placeholder(&decoded) { return Err(UnresolvedPlaceholderError { location: "path" }); } return Ok(RewriteTargetResult { @@ -687,6 +806,13 @@ pub(crate) fn rewrite_target_for_eval( None => redacted_path, }; + // Fail-closed scan: catch any registered placeholder that survived + // rewriting (e.g. a custom-format placeholder that landed in a path + // segment, where only canonical placeholders are substituted). + if resolver.contains_any_placeholder(&resolved) { + return Err(UnresolvedPlaceholderError { location: "path" }); + } + Ok(RewriteTargetResult { resolved, redacted }) } @@ -1473,4 +1599,233 @@ mod tests { assert_eq!(result.resolved, "/bottok123/method?key=key456"); assert_eq!(result.redacted, "/bot[CREDENTIAL]/method?key=[CREDENTIAL]"); } + + // === Custom placeholder overrides (issue #894) === + + #[test] + fn custom_placeholder_overrides_replace_canonical_form() { + let (child_env, resolver) = SecretResolver::from_provider_env_with_overrides( + [( + "SLACK_BOT_TOKEN".to_string(), + "xoxb-real-secret".to_string(), + )] + .into_iter() + .collect(), + [( + "SLACK_BOT_TOKEN".to_string(), + "xoxb-OPENSHELL-PLACEHOLDER".to_string(), + )] + .into_iter() + .collect(), + ); + let resolver = resolver.expect("resolver"); + + // Child sees the format-preserving override, not the canonical placeholder. + assert_eq!( + child_env.get("SLACK_BOT_TOKEN"), + Some(&"xoxb-OPENSHELL-PLACEHOLDER".to_string()) + ); + assert_eq!( + resolver.resolve_placeholder("xoxb-OPENSHELL-PLACEHOLDER"), + Some("xoxb-real-secret") + ); + // Canonical form is NOT registered when an override is present. + assert_eq!( + resolver.resolve_placeholder("openshell:resolve:env:SLACK_BOT_TOKEN"), + None + ); + assert!(resolver.has_custom_placeholders()); + } + + #[test] + fn custom_placeholder_falls_back_to_canonical_for_credentials_without_override() { + let (child_env, resolver) = SecretResolver::from_provider_env_with_overrides( + [ + ("SLACK_BOT_TOKEN".to_string(), "xoxb-real".to_string()), + ("ANTHROPIC_API_KEY".to_string(), "sk-real".to_string()), + ] + .into_iter() + .collect(), + std::iter::once(("SLACK_BOT_TOKEN".to_string(), "xoxb-OPENSHELL".to_string())) + .collect(), + ); + let resolver = resolver.expect("resolver"); + + assert_eq!( + child_env.get("SLACK_BOT_TOKEN"), + Some(&"xoxb-OPENSHELL".to_string()) + ); + assert_eq!( + child_env.get("ANTHROPIC_API_KEY"), + Some(&"openshell:resolve:env:ANTHROPIC_API_KEY".to_string()) + ); + assert_eq!( + resolver.resolve_placeholder("xoxb-OPENSHELL"), + Some("xoxb-real") + ); + assert_eq!( + resolver.resolve_placeholder("openshell:resolve:env:ANTHROPIC_API_KEY"), + Some("sk-real") + ); + } + + #[test] + fn empty_or_unsafe_overrides_fall_back_to_canonical() { + let (child_env, resolver) = SecretResolver::from_provider_env_with_overrides( + [ + ("EMPTY_OVERRIDE".to_string(), "v1".to_string()), + ("CRLF_OVERRIDE".to_string(), "v2".to_string()), + ("NUL_OVERRIDE".to_string(), "v3".to_string()), + ] + .into_iter() + .collect(), + [ + ("EMPTY_OVERRIDE".to_string(), String::new()), + ("CRLF_OVERRIDE".to_string(), "bad\r\nvalue".to_string()), + ("NUL_OVERRIDE".to_string(), "bad\0value".to_string()), + ] + .into_iter() + .collect(), + ); + let resolver = resolver.expect("resolver"); + + for key in ["EMPTY_OVERRIDE", "CRLF_OVERRIDE", "NUL_OVERRIDE"] { + assert_eq!( + child_env.get(key), + Some(&format!("openshell:resolve:env:{key}")), + "{key} should fall back to canonical placeholder" + ); + } + assert!(!resolver.has_custom_placeholders()); + } + + #[test] + fn custom_placeholder_substitutes_in_bearer_authorization_header() { + let (child_env, resolver) = SecretResolver::from_provider_env_with_overrides( + std::iter::once(( + "SLACK_BOT_TOKEN".to_string(), + "xoxb-real-secret".to_string(), + )) + .collect(), + std::iter::once(( + "SLACK_BOT_TOKEN".to_string(), + "xoxb-OPENSHELL-PLACEHOLDER".to_string(), + )) + .collect(), + ); + let resolver = resolver.expect("resolver"); + let placeholder = child_env.get("SLACK_BOT_TOKEN").unwrap(); + assert_eq!(placeholder, "xoxb-OPENSHELL-PLACEHOLDER"); + + let raw = format!( + "POST /api/chat.postMessage HTTP/1.1\r\nAuthorization: Bearer {placeholder}\r\nContent-Length: 0\r\n\r\n" + ); + let result = rewrite_http_header_block(raw.as_bytes(), Some(&resolver)) + .expect("rewrite should succeed"); + let rewritten = String::from_utf8(result.rewritten).expect("utf8"); + + assert!(rewritten.contains("Authorization: Bearer xoxb-real-secret")); + assert!(!rewritten.contains("xoxb-OPENSHELL-PLACEHOLDER")); + } + + #[test] + fn custom_placeholder_substitutes_in_query_param() { + let (child_env, resolver) = SecretResolver::from_provider_env_with_overrides( + std::iter::once(("API_KEY".to_string(), "real-api-key".to_string())).collect(), + std::iter::once(("API_KEY".to_string(), "MOCK-API-KEY-XYZ".to_string())).collect(), + ); + let resolver = resolver.expect("resolver"); + let placeholder = child_env.get("API_KEY").unwrap(); + + let target = format!("/v1/list?key={placeholder}&format=json"); + let result = rewrite_target_for_eval(&target, &resolver).expect("rewrite should succeed"); + + assert_eq!(result.resolved, "/v1/list?key=real-api-key&format=json"); + assert_eq!(result.redacted, "/v1/list?key=[CREDENTIAL]&format=json"); + } + + #[test] + fn custom_placeholder_in_path_segment_fails_closed() { + // Custom-format placeholders are NOT substituted in URL path segments + // (path-segment extraction relies on the canonical env-var-name grammar). + // A custom placeholder that ends up in a path must be rejected by the + // post-rewrite scan rather than silently leaking upstream. + let (child_env, resolver) = SecretResolver::from_provider_env_with_overrides( + std::iter::once(("BOT_TOKEN".to_string(), "real-token".to_string())).collect(), + std::iter::once(("BOT_TOKEN".to_string(), "MOCK-BOT-TOKEN".to_string())).collect(), + ); + let resolver = resolver.expect("resolver"); + let placeholder = child_env.get("BOT_TOKEN").unwrap(); + + let target = format!("/bot{placeholder}/sendMessage"); + let err = rewrite_target_for_eval(&target, &resolver) + .expect_err("must fail closed when custom placeholder lands in path"); + assert_eq!(err.location, "path"); + } + + #[test] + fn mixed_canonical_and_custom_placeholders_round_trip() { + let (child_env, resolver) = SecretResolver::from_provider_env_with_overrides( + [ + ("ANTHROPIC_API_KEY".to_string(), "sk-anthropic".to_string()), + ("SLACK_BOT_TOKEN".to_string(), "xoxb-slack".to_string()), + ] + .into_iter() + .collect(), + std::iter::once(("SLACK_BOT_TOKEN".to_string(), "xoxb-OPENSHELL".to_string())) + .collect(), + ); + let resolver = resolver.expect("resolver"); + let anthropic_ph = child_env.get("ANTHROPIC_API_KEY").unwrap().clone(); + let slack_ph = child_env.get("SLACK_BOT_TOKEN").unwrap().clone(); + + let raw = format!( + "POST /v1 HTTP/1.1\r\nx-api-key: {anthropic_ph}\r\nAuthorization: Bearer {slack_ph}\r\n\r\n" + ); + let result = rewrite_http_header_block(raw.as_bytes(), Some(&resolver)) + .expect("rewrite should succeed"); + let rewritten = String::from_utf8(result.rewritten).expect("utf8"); + + assert!(rewritten.contains("x-api-key: sk-anthropic")); + assert!(rewritten.contains("Authorization: Bearer xoxb-slack")); + assert!(!rewritten.contains("openshell:resolve:env:")); + assert!(!rewritten.contains("xoxb-OPENSHELL")); + } + + #[test] + fn unresolved_custom_placeholder_in_header_fails_closed() { + // Register a custom placeholder, then send a request whose header + // contains a different (unregistered) custom-shaped string. Exact + // match fails, so the registered string never appears in output — + // request passes through. But a header carrying the *registered* + // placeholder unresolved (e.g. via a bug in rewriting) must be + // caught by the post-scan. + let (child_env, resolver) = SecretResolver::from_provider_env_with_overrides( + std::iter::once(("BOT_TOKEN".to_string(), "real".to_string())).collect(), + std::iter::once(("BOT_TOKEN".to_string(), "MOCK-OPENSHELL-BOT".to_string())).collect(), + ); + let resolver = resolver.expect("resolver"); + let placeholder = child_env.get("BOT_TOKEN").unwrap(); + + // Embed the registered placeholder in a header position rewrite_header_value + // does not currently substitute (e.g. inside an `X-Notes:` free-form field + // followed by other text). The exact-match lookup fails (value is not + // exactly the placeholder), Bearer split fails (no whitespace separator + // before the placeholder), so the header passes through. The post-scan + // must then reject because the registered placeholder still appears. + let raw = format!("POST / HTTP/1.1\r\nX-Notes: token-is-{placeholder}-keep-secret\r\n\r\n"); + let err = rewrite_http_header_block(raw.as_bytes(), Some(&resolver)) + .expect_err("post-scan must reject unresolved custom placeholder"); + assert_eq!(err.location, "header"); + } + + #[test] + fn no_overrides_does_not_set_custom_placeholders_flag() { + let (_, resolver) = SecretResolver::from_provider_env_with_overrides( + std::iter::once(("KEY".to_string(), "val".to_string())).collect(), + HashMap::new(), + ); + let resolver = resolver.expect("resolver"); + assert!(!resolver.has_custom_placeholders()); + } } diff --git a/crates/openshell-server/src/grpc/policy.rs b/crates/openshell-server/src/grpc/policy.rs index 8ef8cb5c7..a49261797 100644 --- a/crates/openshell-server/src/grpc/policy.rs +++ b/crates/openshell-server/src/grpc/policy.rs @@ -449,19 +449,23 @@ pub(super) async fn handle_get_sandbox_provider_environment( .spec .ok_or_else(|| Status::internal("sandbox has no spec"))?; - let environment = + let resolved = super::provider::resolve_provider_environment(state.store.as_ref(), &spec.providers) .await?; info!( sandbox_id = %sandbox_id, provider_count = spec.providers.len(), - env_count = environment.len(), + env_count = resolved.env.len(), + placeholder_override_count = resolved.placeholder_overrides.len(), + passthrough_count = resolved.passthrough_keys.len(), "GetSandboxProviderEnvironment request completed successfully" ); Ok(Response::new(GetSandboxProviderEnvironmentResponse { - environment, + environment: resolved.env, + placeholder_overrides: resolved.placeholder_overrides, + passthrough_keys: resolved.passthrough_keys, })) } diff --git a/crates/openshell-server/src/grpc/provider.rs b/crates/openshell-server/src/grpc/provider.rs index bbf8f93cc..dc7e404a8 100644 --- a/crates/openshell-server/src/grpc/provider.rs +++ b/crates/openshell-server/src/grpc/provider.rs @@ -132,6 +132,14 @@ pub(super) async fn update_provider_record( r#type: existing.r#type, credentials: merge_map(existing.credentials, provider.credentials), config: merge_map(existing.config, provider.config), + credential_placeholders: merge_map( + existing.credential_placeholders, + provider.credential_placeholders, + ), + passthrough_credentials: merge_passthrough( + existing.passthrough_credentials, + provider.passthrough_credentials, + ), }; validate_provider_fields(&updated)?; @@ -177,25 +185,69 @@ fn merge_map( existing } +/// Merge an incoming passthrough credential list into the existing one. +/// +/// `repeated string` has no field presence, so we use a sentinel: +/// - If `incoming` is empty, the merge is a no-op (preserve existing). +/// - Otherwise, the incoming list replaces the existing list verbatim. +/// - To clear the list explicitly, callers send a single empty-string entry, +/// which is filtered out and produces an empty result. +/// +/// Duplicate entries are de-duplicated while preserving first-occurrence order. +fn merge_passthrough(existing: Vec, incoming: Vec) -> Vec { + if incoming.is_empty() { + return existing; + } + let mut seen = std::collections::HashSet::new(); + let mut out = Vec::with_capacity(incoming.len()); + for key in incoming { + if key.is_empty() { + continue; + } + if seen.insert(key.clone()) { + out.push(key); + } + } + out +} + // --------------------------------------------------------------------------- // Provider environment resolution // --------------------------------------------------------------------------- +/// Resolved provider environment for sandbox injection. +#[derive(Debug, Default)] +pub(super) struct ResolvedProviderEnvironment { + /// Real credential values keyed by env var name. + pub env: std::collections::HashMap, + /// Optional placeholder overrides keyed by env var name. Missing entries + /// fall back to the canonical `openshell:resolve:env:` form. + pub placeholder_overrides: std::collections::HashMap, + /// Env var names whose REAL credential value must be injected directly + /// into the agent process, bypassing placeholder substitution. The + /// supervisor must NOT register passthrough keys with the secret + /// resolver — there is no on-the-wire placeholder for them. + pub passthrough_keys: Vec, +} + /// Resolve provider credentials into environment variables. /// /// For each provider name in the list, fetches the provider from the store and -/// collects credential key-value pairs. Returns a map of environment variables -/// to inject into the sandbox. When duplicate keys appear across providers, the -/// first provider's value wins. +/// collects credential key-value pairs along with any placeholder overrides. +/// Returns both maps for sandbox injection. When duplicate keys appear across +/// providers, the first provider's value (and override) wins. pub(super) async fn resolve_provider_environment( store: &Store, provider_names: &[String], -) -> Result, Status> { +) -> Result { if provider_names.is_empty() { - return Ok(std::collections::HashMap::new()); + return Ok(ResolvedProviderEnvironment::default()); } let mut env = std::collections::HashMap::new(); + let mut placeholder_overrides = std::collections::HashMap::new(); + let mut passthrough_keys = Vec::new(); + let mut passthrough_seen = std::collections::HashSet::new(); for name in provider_names { let provider = store @@ -204,20 +256,68 @@ pub(super) async fn resolve_provider_environment( .map_err(|e| Status::internal(format!("failed to fetch provider '{name}': {e}")))? .ok_or_else(|| Status::failed_precondition(format!("provider '{name}' not found")))?; + let passthrough_set: std::collections::HashSet<&String> = + provider.passthrough_credentials.iter().collect(); + for (key, value) in &provider.credentials { - if is_valid_env_key(key) { - env.entry(key.clone()).or_insert_with(|| value.clone()); - } else { + if !is_valid_env_key(key) { warn!( provider_name = %name, key = %key, "skipping credential with invalid env var key" ); + continue; + } + // First provider's value wins. + if env.contains_key(key) { + continue; + } + env.insert(key.clone(), value.clone()); + + if passthrough_set.contains(key) { + // Passthrough credentials bypass placeholder substitution + // entirely. A key cannot be both passthrough and have a + // placeholder override (validate_provider_fields rejects it + // at write time); double-check here as defence in depth. + if provider.credential_placeholders.contains_key(key) { + warn!( + provider_name = %name, + key = %key, + "credential is both passthrough and has placeholder override; treating as passthrough" + ); + } + if passthrough_seen.insert(key.clone()) { + passthrough_keys.push(key.clone()); + } + continue; + } + + if let Some(placeholder) = provider.credential_placeholders.get(key) { + if is_valid_placeholder(placeholder) { + placeholder_overrides.insert(key.clone(), placeholder.clone()); + } else { + warn!( + provider_name = %name, + key = %key, + "skipping placeholder override with prohibited characters" + ); + } } } } - Ok(env) + Ok(ResolvedProviderEnvironment { + env, + placeholder_overrides, + passthrough_keys, + }) +} + +/// Reject placeholder strings that would corrupt HTTP framing or be empty. +/// CRLF and NUL bytes are forbidden; the supervisor's placeholder scanner +/// also relies on the value being a non-empty stable string. +fn is_valid_placeholder(value: &str) -> bool { + !value.is_empty() && !value.bytes().any(|b| matches!(b, b'\r' | b'\n' | 0)) } pub(super) fn is_valid_env_key(key: &str) -> bool { @@ -373,6 +473,8 @@ mod tests { ] .into_iter() .collect(), + credential_placeholders: HashMap::new(), + passthrough_credentials: Vec::new(), } } @@ -414,6 +516,8 @@ mod tests { .collect(), config: std::iter::once(("endpoint".to_string(), "https://gitlab.com".to_string())) .collect(), + credential_placeholders: HashMap::new(), + passthrough_credentials: Vec::new(), }, ) .await @@ -478,6 +582,8 @@ mod tests { r#type: String::new(), credentials: HashMap::new(), config: HashMap::new(), + credential_placeholders: HashMap::new(), + passthrough_credentials: Vec::new(), }, ) .await @@ -498,6 +604,8 @@ mod tests { r#type: String::new(), credentials: HashMap::new(), config: HashMap::new(), + credential_placeholders: HashMap::new(), + passthrough_credentials: Vec::new(), }, ) .await @@ -522,6 +630,8 @@ mod tests { r#type: String::new(), credentials: HashMap::new(), config: HashMap::new(), + credential_placeholders: HashMap::new(), + passthrough_credentials: Vec::new(), }, ) .await @@ -565,6 +675,8 @@ mod tests { r#type: String::new(), credentials: std::iter::once(("SECONDARY".to_string(), String::new())).collect(), config: std::iter::once(("region".to_string(), String::new())).collect(), + credential_placeholders: HashMap::new(), + passthrough_credentials: Vec::new(), }, ) .await @@ -612,6 +724,8 @@ mod tests { r#type: String::new(), credentials: HashMap::new(), config: HashMap::new(), + credential_placeholders: HashMap::new(), + passthrough_credentials: Vec::new(), }, ) .await @@ -637,6 +751,8 @@ mod tests { r#type: "openai".to_string(), credentials: HashMap::new(), config: HashMap::new(), + credential_placeholders: HashMap::new(), + passthrough_credentials: Vec::new(), }, ) .await @@ -664,6 +780,8 @@ mod tests { r#type: String::new(), credentials: std::iter::once((oversized_key, "value".to_string())).collect(), config: HashMap::new(), + credential_placeholders: HashMap::new(), + passthrough_credentials: Vec::new(), }, ) .await @@ -676,7 +794,8 @@ mod tests { async fn resolve_provider_env_empty_list_returns_empty() { let store = Store::connect("sqlite::memory:").await.unwrap(); let result = resolve_provider_environment(&store, &[]).await.unwrap(); - assert!(result.is_empty()); + assert!(result.env.is_empty()); + assert!(result.placeholder_overrides.is_empty()); } #[tokio::test] @@ -697,15 +816,23 @@ mod tests { "https://api.anthropic.com".to_string(), )) .collect(), + credential_placeholders: HashMap::new(), + passthrough_credentials: Vec::new(), }; create_provider_record(&store, provider).await.unwrap(); let result = resolve_provider_environment(&store, &["claude-local".to_string()]) .await .unwrap(); - assert_eq!(result.get("ANTHROPIC_API_KEY"), Some(&"sk-abc".to_string())); - assert_eq!(result.get("CLAUDE_API_KEY"), Some(&"sk-abc".to_string())); - assert!(!result.contains_key("endpoint")); + assert_eq!( + result.env.get("ANTHROPIC_API_KEY"), + Some(&"sk-abc".to_string()) + ); + assert_eq!( + result.env.get("CLAUDE_API_KEY"), + Some(&"sk-abc".to_string()) + ); + assert!(!result.env.contains_key("endpoint")); } #[tokio::test] @@ -733,15 +860,17 @@ mod tests { .into_iter() .collect(), config: HashMap::new(), + credential_placeholders: HashMap::new(), + passthrough_credentials: Vec::new(), }; create_provider_record(&store, provider).await.unwrap(); let result = resolve_provider_environment(&store, &["test-provider".to_string()]) .await .unwrap(); - assert_eq!(result.get("VALID_KEY"), Some(&"value".to_string())); - assert!(!result.contains_key("nested.api_key")); - assert!(!result.contains_key("bad-key")); + assert_eq!(result.env.get("VALID_KEY"), Some(&"value".to_string())); + assert!(!result.env.contains_key("nested.api_key")); + assert!(!result.env.contains_key("bad-key")); } #[tokio::test] @@ -759,6 +888,8 @@ mod tests { )) .collect(), config: HashMap::new(), + credential_placeholders: HashMap::new(), + passthrough_credentials: Vec::new(), }, ) .await @@ -772,6 +903,8 @@ mod tests { credentials: std::iter::once(("GITLAB_TOKEN".to_string(), "glpat-xyz".to_string())) .collect(), config: HashMap::new(), + credential_placeholders: HashMap::new(), + passthrough_credentials: Vec::new(), }, ) .await @@ -783,8 +916,14 @@ mod tests { ) .await .unwrap(); - assert_eq!(result.get("ANTHROPIC_API_KEY"), Some(&"sk-abc".to_string())); - assert_eq!(result.get("GITLAB_TOKEN"), Some(&"glpat-xyz".to_string())); + assert_eq!( + result.env.get("ANTHROPIC_API_KEY"), + Some(&"sk-abc".to_string()) + ); + assert_eq!( + result.env.get("GITLAB_TOKEN"), + Some(&"glpat-xyz".to_string()) + ); } #[tokio::test] @@ -799,6 +938,8 @@ mod tests { credentials: std::iter::once(("SHARED_KEY".to_string(), "first-value".to_string())) .collect(), config: HashMap::new(), + credential_placeholders: HashMap::new(), + passthrough_credentials: Vec::new(), }, ) .await @@ -815,6 +956,8 @@ mod tests { )) .collect(), config: HashMap::new(), + credential_placeholders: HashMap::new(), + passthrough_credentials: Vec::new(), }, ) .await @@ -826,7 +969,105 @@ mod tests { ) .await .unwrap(); - assert_eq!(result.get("SHARED_KEY"), Some(&"first-value".to_string())); + assert_eq!( + result.env.get("SHARED_KEY"), + Some(&"first-value".to_string()) + ); + } + + #[tokio::test] + async fn resolve_provider_env_returns_placeholder_overrides() { + let store = Store::connect("sqlite::memory:").await.unwrap(); + create_provider_record( + &store, + Provider { + id: String::new(), + name: "slack-local".to_string(), + r#type: "slack".to_string(), + credentials: [ + ("SLACK_BOT_TOKEN".to_string(), "xoxb-real".to_string()), + ("SLACK_APP_TOKEN".to_string(), "xapp-real".to_string()), + ] + .into_iter() + .collect(), + config: HashMap::new(), + credential_placeholders: [ + ( + "SLACK_BOT_TOKEN".to_string(), + "xoxb-OPENSHELL-PLACEHOLDER".to_string(), + ), + ( + "SLACK_APP_TOKEN".to_string(), + "xapp-OPENSHELL-PLACEHOLDER".to_string(), + ), + ] + .into_iter() + .collect(), + passthrough_credentials: Vec::new(), + }, + ) + .await + .unwrap(); + + let result = resolve_provider_environment(&store, &["slack-local".to_string()]) + .await + .unwrap(); + + assert_eq!( + result.placeholder_overrides.get("SLACK_BOT_TOKEN"), + Some(&"xoxb-OPENSHELL-PLACEHOLDER".to_string()) + ); + assert_eq!( + result.placeholder_overrides.get("SLACK_APP_TOKEN"), + Some(&"xapp-OPENSHELL-PLACEHOLDER".to_string()) + ); + } + + #[tokio::test] + async fn resolve_provider_env_drops_unsafe_placeholder_overrides() { + let store = Store::connect("sqlite::memory:").await.unwrap(); + create_provider_record( + &store, + Provider { + id: String::new(), + name: "broken-provider".to_string(), + r#type: "test".to_string(), + credentials: [ + ("OK_KEY".to_string(), "v1".to_string()), + ("CRLF_KEY".to_string(), "v2".to_string()), + ("EMPTY_KEY".to_string(), "v3".to_string()), + ] + .into_iter() + .collect(), + config: HashMap::new(), + credential_placeholders: [ + ("OK_KEY".to_string(), "ok-PLACEHOLDER".to_string()), + ("CRLF_KEY".to_string(), "bad\r\ninjected".to_string()), + ("EMPTY_KEY".to_string(), String::new()), + ] + .into_iter() + .collect(), + passthrough_credentials: Vec::new(), + }, + ) + .await + .unwrap(); + + let result = resolve_provider_environment(&store, &["broken-provider".to_string()]) + .await + .unwrap(); + + // Safe override is preserved. + assert_eq!( + result.placeholder_overrides.get("OK_KEY"), + Some(&"ok-PLACEHOLDER".to_string()) + ); + // Unsafe overrides are dropped — the credential falls back to the + // canonical placeholder downstream. + assert!(!result.placeholder_overrides.contains_key("CRLF_KEY")); + assert!(!result.placeholder_overrides.contains_key("EMPTY_KEY")); + // All credentials are still present in env. + assert_eq!(result.env.len(), 3); } #[tokio::test] @@ -847,6 +1088,8 @@ mod tests { )) .collect(), config: HashMap::new(), + credential_placeholders: HashMap::new(), + passthrough_credentials: Vec::new(), }, ) .await @@ -876,7 +1119,10 @@ mod tests { .await .unwrap(); - assert_eq!(env.get("ANTHROPIC_API_KEY"), Some(&"sk-test".to_string())); + assert_eq!( + env.env.get("ANTHROPIC_API_KEY"), + Some(&"sk-test".to_string()) + ); } #[tokio::test] @@ -906,7 +1152,7 @@ mod tests { .await .unwrap(); - assert!(env.is_empty()); + assert!(env.env.is_empty()); } #[tokio::test] diff --git a/crates/openshell-server/src/grpc/validation.rs b/crates/openshell-server/src/grpc/validation.rs index 1517c0577..cc3d788fa 100644 --- a/crates/openshell-server/src/grpc/validation.rs +++ b/crates/openshell-server/src/grpc/validation.rs @@ -267,6 +267,36 @@ pub(super) fn validate_provider_fields(provider: &Provider) -> Result<(), Status MAX_MAP_VALUE_LEN, "provider.config", )?; + validate_string_map( + &provider.credential_placeholders, + MAX_PROVIDER_CREDENTIALS_ENTRIES, + MAX_MAP_KEY_LEN, + MAX_MAP_VALUE_LEN, + "provider.credential_placeholders", + )?; + if provider.passthrough_credentials.len() > MAX_PROVIDER_CREDENTIALS_ENTRIES { + return Err(Status::invalid_argument(format!( + "provider.passthrough_credentials has too many entries ({} > {MAX_PROVIDER_CREDENTIALS_ENTRIES})", + provider.passthrough_credentials.len() + ))); + } + for key in &provider.passthrough_credentials { + if key.is_empty() { + return Err(Status::invalid_argument( + "provider.passthrough_credentials contains an empty key", + )); + } + if key.len() > MAX_MAP_KEY_LEN { + return Err(Status::invalid_argument(format!( + "provider.passthrough_credentials key exceeds {MAX_MAP_KEY_LEN} byte limit" + ))); + } + if provider.credential_placeholders.contains_key(key) { + return Err(Status::invalid_argument(format!( + "provider credential '{key}' is in both credential_placeholders and passthrough_credentials; pick one" + ))); + } + } Ok(()) } @@ -626,6 +656,8 @@ mod tests { credentials: one_credential(), config: std::iter::once(("endpoint".to_string(), "https://example.com".to_string())) .collect(), + credential_placeholders: HashMap::new(), + passthrough_credentials: Vec::new(), }; assert!(validate_provider_fields(&provider).is_ok()); } @@ -638,6 +670,8 @@ mod tests { r#type: "claude".to_string(), credentials: one_credential(), config: HashMap::new(), + credential_placeholders: HashMap::new(), + passthrough_credentials: Vec::new(), }; let err = validate_provider_fields(&provider).unwrap_err(); assert_eq!(err.code(), Code::InvalidArgument); @@ -652,6 +686,8 @@ mod tests { r#type: "x".repeat(MAX_PROVIDER_TYPE_LEN + 1), credentials: one_credential(), config: HashMap::new(), + credential_placeholders: HashMap::new(), + passthrough_credentials: Vec::new(), }; let err = validate_provider_fields(&provider).unwrap_err(); assert_eq!(err.code(), Code::InvalidArgument); @@ -669,6 +705,8 @@ mod tests { r#type: "claude".to_string(), credentials: creds, config: HashMap::new(), + credential_placeholders: HashMap::new(), + passthrough_credentials: Vec::new(), }; let err = validate_provider_fields(&provider).unwrap_err(); assert_eq!(err.code(), Code::InvalidArgument); @@ -686,6 +724,8 @@ mod tests { r#type: "claude".to_string(), credentials: one_credential(), config, + credential_placeholders: HashMap::new(), + passthrough_credentials: Vec::new(), }; let err = validate_provider_fields(&provider).unwrap_err(); assert_eq!(err.code(), Code::InvalidArgument); @@ -700,6 +740,8 @@ mod tests { r#type: "claude".to_string(), credentials: one_credential(), config: HashMap::new(), + credential_placeholders: HashMap::new(), + passthrough_credentials: Vec::new(), }; assert!(validate_provider_fields(&provider).is_ok()); } @@ -714,6 +756,8 @@ mod tests { r#type: "claude".to_string(), credentials: creds, config: HashMap::new(), + credential_placeholders: HashMap::new(), + passthrough_credentials: Vec::new(), }; let err = validate_provider_fields(&provider).unwrap_err(); assert_eq!(err.code(), Code::InvalidArgument); @@ -730,6 +774,8 @@ mod tests { r#type: "claude".to_string(), credentials: one_credential(), config, + credential_placeholders: HashMap::new(), + passthrough_credentials: Vec::new(), }; let err = validate_provider_fields(&provider).unwrap_err(); assert_eq!(err.code(), Code::InvalidArgument); diff --git a/crates/openshell-server/src/inference.rs b/crates/openshell-server/src/inference.rs index 79f303aeb..e779cb708 100644 --- a/crates/openshell-server/src/inference.rs +++ b/crates/openshell-server/src/inference.rs @@ -500,6 +500,8 @@ mod tests { r#type: provider_type.to_string(), credentials: std::iter::once((key_name.to_string(), key_value.to_string())).collect(), config: std::collections::HashMap::new(), + credential_placeholders: std::collections::HashMap::new(), + passthrough_credentials: Vec::new(), } } @@ -662,6 +664,8 @@ mod tests { "https://station.example.com/v1".to_string(), )) .collect(), + credential_placeholders: std::collections::HashMap::new(), + passthrough_credentials: Vec::new(), }; store .put_message(&provider) @@ -733,6 +737,8 @@ mod tests { credentials: std::iter::once(("OPENAI_API_KEY".to_string(), "sk-rotated".to_string())) .collect(), config: provider.config, + credential_placeholders: provider.credential_placeholders, + passthrough_credentials: provider.passthrough_credentials, }; store .put_message(&rotated_provider) diff --git a/crates/openshell-tui/src/lib.rs b/crates/openshell-tui/src/lib.rs index affdbc224..846be8015 100644 --- a/crates/openshell-tui/src/lib.rs +++ b/crates/openshell-tui/src/lib.rs @@ -1563,6 +1563,8 @@ fn spawn_create_provider(app: &App, tx: mpsc::UnboundedSender) { r#type: ptype.clone(), credentials: credentials.clone(), config: Default::default(), + credential_placeholders: Default::default(), + passthrough_credentials: Default::default(), }), }; @@ -1642,6 +1644,8 @@ fn spawn_update_provider(app: &App, tx: mpsc::UnboundedSender) { r#type: ptype, credentials, config: Default::default(), + credential_placeholders: Default::default(), + passthrough_credentials: Default::default(), }), }; diff --git a/proto/datamodel.proto b/proto/datamodel.proto index f84d1e352..e9826e7e0 100644 --- a/proto/datamodel.proto +++ b/proto/datamodel.proto @@ -15,4 +15,25 @@ message Provider { map credentials = 4; // Non-secret provider configuration. map config = 5; + // Optional per-credential placeholder strings, keyed by the same env var + // name as `credentials`. When set, this literal string (instead of the + // canonical `openshell:resolve:env:`) is what the agent process sees + // in its environment, while the L7 proxy still substitutes the real value + // on the wire. Used for SDKs that validate credential format in-process + // before any network call (for example, Slack's `xoxb-` prefix check). + // Missing entries fall back to the canonical placeholder. + map credential_placeholders = 6; + // Optional per-credential passthrough opt-in: env var names whose REAL + // credential value is injected directly into the agent process, with no + // placeholder and no L7 proxy substitution. Use this only for credentials + // that an SDK consumes in-process (signature computation, checksum + // verification, non-HTTP transports) where neither the canonical + // placeholder nor a custom-format placeholder can be substituted on the + // wire. This drops the "agent never sees the real value" invariant for + // the listed keys; treat it as a per-credential security trade-off and + // prefer canonical or custom placeholders whenever the SDK can tolerate + // them. A key MUST NOT appear in both `credential_placeholders` and + // `passthrough_credentials`. Keys not present in `credentials` are + // ignored. + repeated string passthrough_credentials = 7; } diff --git a/proto/openshell.proto b/proto/openshell.proto index 2434f1a80..c0c8f7b3e 100644 --- a/proto/openshell.proto +++ b/proto/openshell.proto @@ -578,6 +578,18 @@ message GetSandboxProviderEnvironmentRequest { message GetSandboxProviderEnvironmentResponse { // Provider credential environment variables. map environment = 1; + // Optional per-credential placeholder overrides keyed by env var name. + // When present, the supervisor injects this literal value into the child + // environment instead of the canonical `openshell:resolve:env:` + // placeholder. The L7 proxy registers it and substitutes the real value + // on the wire. + map placeholder_overrides = 2; + // Optional list of env var names whose REAL credential value is injected + // directly into the agent process, bypassing the placeholder/L7-proxy + // substitution path entirely. The supervisor must NOT register passthrough + // keys with the secret resolver. Used for credentials that an SDK consumes + // in-process and for which no on-the-wire substitution is possible. + repeated string passthrough_keys = 3; } // ---------------------------------------------------------------------------