From 8446a904761e49b7f6df425abe57f953bf062ec9 Mon Sep 17 00:00:00 2001 From: Tinson Lai Date: Fri, 1 May 2026 16:24:54 +0000 Subject: [PATCH] feat(provider): selective credential passthrough MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add an opt-in passthrough mechanism for provider credentials that bypass the canonical openshell:resolve:env:* placeholder and L7 proxy substitution. Provider gains a repeated string passthrough_credentials field listing credential keys whose real value should be injected directly into the agent process environment. Required for SDKs that validate credential format in-process (Slack xoxb-*/xapp-*), credentials consumed by in-process crypto (HMAC signing secrets, signature verification), and transports the proxy cannot rewrite (WebSocket payloads after HTTP 101 upgrade). Passthrough drops the "agent never sees the real secret" invariant for the listed keys; documented as a per-credential security trade-off. Server: validation rejects keys not in credentials, invalid env var names, duplicates, and over-cap. resolve_provider_environment returns a ProviderEnvironment struct with the merged env map and the union of passthrough keys; passthrough flag follows the winning value on duplicate keys across providers. Update merge auto-prunes orphaned passthrough entries when their credential is deleted in the same call (proto3 cannot distinguish "field unset" from "set to empty list"), preserving the "recover via update" path. Explicit non-empty incoming list still rejects orphans as a caller bug. Sandbox: SecretResolver::from_provider_env_with_passthrough handles the new mode — passthrough keys go to child env as real values, resolver does not learn them; non-passthrough keys still get the canonical placeholder. CLI: provider create/update accept --passthrough KEY (repeatable); provider get annotates passthrough keys, provider list adds a PASSTHROUGH column. Refs #894 (selective passthrough lands; custom-format placeholders deferred to a follow-up). Signed-off-by: Tinson Lai --- architecture/sandbox-providers.md | 89 +++++ crates/openshell-cli/src/main.rs | 22 ++ crates/openshell-cli/src/run.rs | 89 ++++- .../tests/ensure_providers_integration.rs | 6 + .../tests/provider_commands_integration.rs | 12 + crates/openshell-sandbox/src/grpc_client.rs | 25 +- crates/openshell-sandbox/src/lib.rs | 20 +- crates/openshell-sandbox/src/secrets.rs | 139 ++++++- crates/openshell-server/src/grpc/policy.rs | 8 +- crates/openshell-server/src/grpc/provider.rs | 356 ++++++++++++++++-- .../openshell-server/src/grpc/validation.rs | 87 +++++ crates/openshell-server/src/inference.rs | 3 + crates/openshell-tui/src/lib.rs | 2 + docs/sandboxes/manage-providers.mdx | 24 ++ e2e/python/test_sandbox_providers.py | 77 ++++ proto/datamodel.proto | 14 + proto/openshell.proto | 5 + 17 files changed, 929 insertions(+), 49 deletions(-) diff --git a/architecture/sandbox-providers.md b/architecture/sandbox-providers.md index 088bd7592..5f4784b18 100644 --- a/architecture/sandbox-providers.md +++ b/architecture/sandbox-providers.md @@ -35,6 +35,9 @@ 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 +- `passthrough_credentials`: `repeated string` listing credential keys whose + real value is injected directly into the agent process, bypassing the + canonical placeholder substitution (see [Selective Passthrough](#selective-passthrough)). The gRPC surface is defined in `proto/openshell.proto`: @@ -363,6 +366,92 @@ CLI: openshell sandbox create -- claude +-- Proxy rewrites outbound auth header placeholders -> real secrets ``` +## Selective Passthrough + +The canonical placeholder model breaks for credentials that: + +- are validated **in-process** by an SDK before any network call (Slack + `@slack/web-api` checks the `xoxb-` prefix; `@slack/socket-mode` checks + `xapp-`; OAuth libraries parse JWT structure; AWS SDKs validate access-key + format), +- are sent over **transports the L7 proxy cannot rewrite** (WebSocket payloads + after HTTP `101 Switching Protocols` upgrade — for example Discord's + IDENTIFY frame), or +- are consumed by **in-process crypto** that never reaches the wire (Slack + signing-secret HMAC, Discord interaction-signature verification). + +For these cases the operator can opt specific credential keys into +**passthrough**: the supervisor injects the **real value** into the agent's +environment instead of the canonical +`openshell:resolve:env:` placeholder, and the resolver does not learn +that key. The L7 proxy therefore performs no substitution for it. + +### Configuration + +Each entry in `Provider.passthrough_credentials` must: + +- be a valid environment-variable name (matches `^[A-Za-z_][A-Za-z0-9_]*$`), +- be present as a key in `Provider.credentials`, +- appear at most once. + +The gateway rejects updates that violate any of these conditions. + +**Update merge semantics.** Proto3 cannot distinguish "field unset" from +"field set to empty list" for repeated strings, so the gateway uses a +two-mode merge: + +- **Empty incoming list** (caller is not declaring passthrough) — preserve + the existing list, but auto-prune any entries whose credential was deleted + in the same update. Without this prune, marking a credential passthrough + and then deleting that credential would lock the provider in an invalid + state. +- **Non-empty incoming list** — replace the stored list verbatim. Strict + validation still applies, so an explicit entry naming a non-existent + credential is rejected (treated as a caller bug, not a state-rescue + request). + +**Clearing the list.** Because proto3 cannot signal "set to empty" for +repeated strings, there is no direct update that wipes the passthrough +list while leaving credentials intact. Available paths: + +- delete the underlying credentials (auto-prune removes the corresponding + passthrough entries), +- replace the list with a non-empty subset (send the entries you want to + keep), or +- recreate the provider. + +### Resolution and merge across providers + +`resolve_provider_environment()` collects credentials and the union of +passthrough keys across all providers attached to a sandbox. When two providers +declare the same credential key, the first provider's value wins (existing +semantics) and the passthrough flag follows the **winning** value: a +passthrough opt-in declared on the second provider has no effect. + +### Supervisor injection + +`SecretResolver::from_provider_env_with_passthrough()` consumes both the env +map and the passthrough key list. For each credential: + +- key in passthrough list → child env receives the real value; resolver does + not register a placeholder entry; +- otherwise → child env receives the canonical placeholder and the resolver + maps it to the real value for on-the-wire substitution. + +If every key is passthrough the resolver is `None` and no L7 substitution +runs. + +### Security trade-off + +Passthrough drops the "agent never sees the real secret" invariant for the +listed keys. The real value is at rest in `/proc//environ` and any +descendant process inherits it. Use sparingly: + +- prefer canonical placeholders whenever the consumer permits it; +- only opt in keys whose consumer demonstrably fails with the placeholder + (in-process format check, opaque transport, in-process crypto); +- document each opt-in in the provider's deployment notes. + ## Persistence and Validation The gateway enforces: diff --git a/crates/openshell-cli/src/main.rs b/crates/openshell-cli/src/main.rs index ccad7a099..2f7aeee4f 100644 --- a/crates/openshell-cli/src/main.rs +++ b/crates/openshell-cli/src/main.rs @@ -709,6 +709,15 @@ enum ProviderCommands { /// Provider config key/value pair. #[arg(long = "config", value_name = "KEY=VALUE")] config: Vec, + + /// Credential key whose real value is injected directly into the + /// agent process, bypassing the canonical placeholder substitution + /// and L7 proxy rewriting. Each key must also appear in `--credential` + /// (or be discovered via `--from-existing`). Drops the "agent never + /// sees the real secret" invariant for that key — see provider docs + /// for the security trade-off. + #[arg(long = "passthrough", value_name = "KEY")] + passthrough: Vec, }, /// Fetch a provider by name. @@ -757,6 +766,15 @@ enum ProviderCommands { /// Provider config key/value pair. #[arg(long = "config", value_name = "KEY=VALUE")] config: Vec, + + /// Credential key whose real value is injected directly into the + /// agent process, bypassing the canonical placeholder substitution + /// and L7 proxy rewriting. Replaces the existing passthrough list + /// when non-empty. Each key must also be present in the merged + /// credentials. Drops the "agent never sees the real secret" + /// invariant for that key. + #[arg(long = "passthrough", value_name = "KEY")] + passthrough: Vec, }, /// Delete providers by name. @@ -2742,6 +2760,7 @@ async fn main() -> Result<()> { from_existing, credentials, config, + passthrough, } => { run::provider_create( endpoint, @@ -2750,6 +2769,7 @@ async fn main() -> Result<()> { from_existing, &credentials, &config, + &passthrough, &tls, ) .await?; @@ -2769,6 +2789,7 @@ async fn main() -> Result<()> { from_existing, credentials, config, + passthrough, } => { run::provider_update( endpoint, @@ -2776,6 +2797,7 @@ async fn main() -> Result<()> { from_existing, &credentials, &config, + &passthrough, &tls, ) .await?; diff --git a/crates/openshell-cli/src/run.rs b/crates/openshell-cli/src/run.rs index eaadf7908..62027282a 100644 --- a/crates/openshell-cli/src/run.rs +++ b/crates/openshell-cli/src/run.rs @@ -3576,6 +3576,7 @@ async fn auto_create_provider( r#type: provider_type.to_string(), credentials: discovered.credentials.clone(), config: discovered.config.clone(), + passthrough_credentials: Vec::new(), }), }; @@ -3616,6 +3617,7 @@ async fn auto_create_provider( r#type: provider_type.to_string(), credentials: discovered.credentials.clone(), config: discovered.config.clone(), + passthrough_credentials: Vec::new(), }), }; @@ -3676,6 +3678,50 @@ fn parse_key_value_pairs(items: &[String], flag: &str) -> Result Result> { + let mut seen = 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!("--passthrough key cannot be empty")); + } + if !seen.insert(key.to_string()) { + return Err(miette::miette!( + "--passthrough key '{key}' was supplied more than once" + )); + } + out.push(key.to_string()); + } + Ok(out) +} + +/// Reject any passthrough key that is not present in `credential_map`. The +/// caller is responsible for assembling the full local credential set before +/// invoking — typically explicit `--credential` entries merged with anything +/// `--from-existing` discovered. +fn ensure_passthrough_in_credentials( + passthrough: &[String], + credential_map: &HashMap, +) -> Result<()> { + for key in passthrough { + if !credential_map.contains_key(key) { + return Err(miette::miette!( + "--passthrough '{key}' is not present in resolved credentials" + )); + } + } + Ok(()) +} + fn parse_credential_pairs(items: &[String]) -> Result> { let mut map = HashMap::new(); @@ -3712,6 +3758,7 @@ fn parse_credential_pairs(items: &[String]) -> Result> { Ok(map) } +#[allow(clippy::too_many_arguments)] // user-facing CLI command pub async fn provider_create( server: &str, name: &str, @@ -3719,6 +3766,7 @@ pub async fn provider_create( from_existing: bool, credentials: &[String], config: &[String], + passthrough: &[String], tls: &TlsOptions, ) -> Result<()> { if from_existing && !credentials.is_empty() { @@ -3735,6 +3783,7 @@ pub async fn provider_create( let mut credential_map = parse_credential_pairs(credentials)?; let mut config_map = parse_key_value_pairs(config, "--config")?; + let passthrough_credentials = parse_passthrough_keys(passthrough)?; if from_existing { let registry = ProviderRegistry::new(); @@ -3762,6 +3811,11 @@ pub async fn provider_create( )); } + // The full credential set is known locally on create (--credential plus + // anything --from-existing discovered), so cross-check passthrough keys + // here for fast feedback. The server re-validates regardless. + ensure_passthrough_in_credentials(&passthrough_credentials, &credential_map)?; + let response = client .create_provider(CreateProviderRequest { provider: Some(Provider { @@ -3774,6 +3828,7 @@ pub async fn provider_create( r#type: provider_type, credentials: credential_map, config: config_map, + passthrough_credentials, }), }) .await @@ -3806,8 +3861,25 @@ pub async fn provider_get(server: &str, name: &str, tls: &TlsOptions) -> Result< .provider .ok_or_else(|| miette::miette!("provider missing from response"))?; - let credential_keys = provider.credentials.keys().cloned().collect::>(); - let config_keys = provider.config.keys().cloned().collect::>(); + let passthrough: HashSet<&str> = provider + .passthrough_credentials + .iter() + .map(String::as_str) + .collect(); + let mut credential_keys = provider + .credentials + .keys() + .map(|k| { + if passthrough.contains(k.as_str()) { + format!("{k} (passthrough)") + } else { + k.clone() + } + }) + .collect::>(); + credential_keys.sort(); + let mut config_keys = provider.config.keys().cloned().collect::>(); + config_keys.sort(); println!("{}", "Provider:".cyan().bold()); println!(); @@ -3878,19 +3950,21 @@ pub async fn provider_list( .max(4); println!( - "{: Result<()> { if from_existing && !credentials.is_empty() { @@ -3916,6 +3991,11 @@ pub async fn provider_update( let mut credential_map = parse_credential_pairs(credentials)?; let mut config_map = parse_key_value_pairs(config, "--config")?; + // On update the local --credential set may legitimately be empty (the + // operator is only changing the passthrough list against existing stored + // credentials), so the cross-check against credentials is delegated to + // server-side validation, which sees the post-merge state. + let passthrough_credentials = parse_passthrough_keys(passthrough)?; if from_existing { // Fetch the existing provider to discover its type for credential lookup. @@ -3960,6 +4040,7 @@ pub async fn provider_update( r#type: String::new(), credentials: credential_map, config: config_map, + passthrough_credentials, }), }) .await diff --git a/crates/openshell-cli/tests/ensure_providers_integration.rs b/crates/openshell-cli/tests/ensure_providers_integration.rs index a5a485735..fc2e19712 100644 --- a/crates/openshell-cli/tests/ensure_providers_integration.rs +++ b/crates/openshell-cli/tests/ensure_providers_integration.rs @@ -115,6 +115,7 @@ impl TestOpenShell { r#type: provider_type.to_string(), credentials: HashMap::new(), config: HashMap::new(), + passthrough_credentials: Vec::new(), }, ); } @@ -293,6 +294,11 @@ impl OpenShell for TestOpenShell { r#type: existing.r#type, credentials: merge(existing.credentials, provider.credentials), config: merge(existing.config, provider.config), + passthrough_credentials: if provider.passthrough_credentials.is_empty() { + existing.passthrough_credentials + } else { + provider.passthrough_credentials + }, }; let updated_name = updated.object_name().to_string(); providers.insert(updated_name, updated.clone()); diff --git a/crates/openshell-cli/tests/provider_commands_integration.rs b/crates/openshell-cli/tests/provider_commands_integration.rs index d151e5a1c..778a43ba6 100644 --- a/crates/openshell-cli/tests/provider_commands_integration.rs +++ b/crates/openshell-cli/tests/provider_commands_integration.rs @@ -243,6 +243,11 @@ impl OpenShell for TestOpenShell { r#type: existing.r#type, credentials: merge(existing.credentials, provider.credentials), config: merge(existing.config, provider.config), + passthrough_credentials: if provider.passthrough_credentials.is_empty() { + existing.passthrough_credentials + } else { + provider.passthrough_credentials + }, }; let updated_name = updated.object_name().to_string(); providers.insert(updated_name, updated.clone()); @@ -501,6 +506,7 @@ async fn provider_cli_run_functions_support_full_crud_flow() { false, &["API_KEY=abc".to_string()], &["profile=dev".to_string()], + &[], &ts.tls, ) .await @@ -519,6 +525,7 @@ async fn provider_cli_run_functions_support_full_crud_flow() { false, &["API_KEY=rotated".to_string()], &["profile=prod".to_string()], + &[], &ts.tls, ) .await @@ -540,6 +547,7 @@ async fn provider_create_rejects_key_only_credentials_without_local_env_value() false, &["INVALID_PAIR".to_string()], &[], + &[], &ts.tls, ) .await @@ -564,6 +572,7 @@ async fn provider_create_supports_generic_type_and_env_lookup_credentials() { false, &["NAV_GENERIC_TEST_KEY".to_string()], &[], + &[], &ts.tls, ) .await @@ -598,6 +607,7 @@ async fn provider_create_rejects_combined_from_existing_and_credentials() { true, &["API_KEY=abc".to_string()], &[], + &[], &ts.tls, ) .await @@ -622,6 +632,7 @@ async fn provider_create_rejects_empty_env_var_for_key_only_credential() { false, &["NAV_EMPTY_ENV_KEY".to_string()], &[], + &[], &ts.tls, ) .await @@ -646,6 +657,7 @@ async fn provider_create_supports_nvidia_type_with_nvidia_api_key() { false, &["NVIDIA_API_KEY".to_string()], &[], + &[], &ts.tls, ) .await diff --git a/crates/openshell-sandbox/src/grpc_client.rs b/crates/openshell-sandbox/src/grpc_client.rs index 44f372355..e3f510a54 100644 --- a/crates/openshell-sandbox/src/grpc_client.rs +++ b/crates/openshell-sandbox/src/grpc_client.rs @@ -238,13 +238,24 @@ pub async fn sync_policy(endpoint: &str, sandbox: &str, policy: &ProtoSandboxPol /// 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 -/// has no providers or the call fails. +/// Resolved provider environment for the sandbox. +#[derive(Debug, Default, Clone)] +pub struct ProviderEnvironment { + /// Credential environment variables (canonical and passthrough mixed). + pub environment: HashMap, + /// Subset of `environment` keys whose value is the real credential and + /// must be injected directly into the agent process — bypassing the + /// canonical placeholder substitution and L7 proxy rewriting. + pub passthrough_keys: Vec, +} + +/// Returns the env map and passthrough keys derived from provider credentials +/// configured on the sandbox. Returns an empty result 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?; @@ -256,7 +267,11 @@ pub async fn fetch_provider_environment( .await .into_diagnostic()?; - Ok(response.into_inner().environment) + let inner = response.into_inner(); + Ok(ProviderEnvironment { + environment: inner.environment, + passthrough_keys: inner.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 fbd7460e2..9f047df7d 100644 --- a/crates/openshell-sandbox/src/lib.rs +++ b/crates/openshell-sandbox/src/lib.rs @@ -269,21 +269,22 @@ 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) { + let resolved = if let (Some(id), Some(endpoint)) = (&sandbox_id, &openshell_endpoint) { match grpc_client::fetch_provider_environment(endpoint, id).await { - Ok(env) => { + Ok(resolved) => { 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:{} passthrough_count:{}]", + resolved.environment.len(), + resolved.passthrough_keys.len() )) .build() ); - env + resolved } Err(e) => { ocsf_emit!( @@ -296,14 +297,17 @@ pub async fn run_sandbox( )) .build() ); - std::collections::HashMap::new() + grpc_client::ProviderEnvironment::default() } } } else { - std::collections::HashMap::new() + 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_passthrough( + resolved.environment, + &resolved.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 63e253e50..c58f93cc8 100644 --- a/crates/openshell-sandbox/src/secrets.rs +++ b/crates/openshell-sandbox/src/secrets.rs @@ -70,23 +70,65 @@ pub struct SecretResolver { } impl SecretResolver { + /// Test helper: build a `SecretResolver` from the provider env map with no + /// passthrough keys. Equivalent to + /// [`Self::from_provider_env_with_passthrough`] with an empty passthrough + /// list. Production callers must use `from_provider_env_with_passthrough` + /// directly so the supervisor honours operator-configured passthrough. + #[cfg(test)] pub(crate) fn from_provider_env( provider_env: HashMap, + ) -> (HashMap, Option) { + Self::from_provider_env_with_passthrough(provider_env, &[]) + } + + /// Build a `SecretResolver` and the corresponding child-process environment + /// from the provider env map and the set of passthrough keys. + /// + /// For each entry in `provider_env`: + /// - If the key appears in `passthrough_keys`, the child environment receives + /// the **real** value directly. The resolver does not learn it; the L7 + /// proxy will not rewrite anything for that key. This drops the "agent + /// never sees the real secret" invariant for that key — required for + /// credentials consumed by SDKs that validate format in-process or by + /// transports the proxy cannot rewrite. + /// - Otherwise the child environment receives the canonical placeholder + /// (`openshell:resolve:env:KEY`) and the resolver maps that placeholder + /// back to the real value for on-the-wire substitution. + /// + /// Returns `(child_env, resolver)`. The resolver is `None` only when there + /// are no canonical placeholders to register (every key was passthrough or + /// `provider_env` was empty). + pub(crate) fn from_provider_env_with_passthrough( + provider_env: 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()); for (key, value) in provider_env { - let placeholder = placeholder_for_env_key(&key); - child_env.insert(key, placeholder.clone()); - by_placeholder.insert(placeholder, value); + if passthrough.contains(key.as_str()) { + child_env.insert(key, value); + } else { + let placeholder = placeholder_for_env_key(&key); + child_env.insert(key, placeholder.clone()); + by_placeholder.insert(placeholder, value); + } } - (child_env, Some(Self { by_placeholder })) + let resolver = if by_placeholder.is_empty() { + None + } else { + Some(Self { by_placeholder }) + }; + (child_env, resolver) } /// Resolve a placeholder string to the real secret value. @@ -716,6 +758,95 @@ pub fn rewrite_target_for_eval( mod tests { use super::*; + // === Passthrough credential tests === + + #[test] + fn passthrough_key_receives_real_value_in_child_env() { + let provider_env: HashMap = [ + ("SLACK_BOT_TOKEN".to_string(), "xoxb-real".to_string()), + ("CLAUDE_API_KEY".to_string(), "sk-real".to_string()), + ] + .into_iter() + .collect(); + let (child_env, resolver) = SecretResolver::from_provider_env_with_passthrough( + provider_env, + &["SLACK_BOT_TOKEN".to_string()], + ); + + // Passthrough key: real value in child env. + assert_eq!( + child_env.get("SLACK_BOT_TOKEN"), + Some(&"xoxb-real".to_string()) + ); + // Non-passthrough key: canonical placeholder in child env. + assert_eq!( + child_env.get("CLAUDE_API_KEY"), + Some(&"openshell:resolve:env:CLAUDE_API_KEY".to_string()) + ); + // Resolver only knows about the canonical placeholder. + let resolver = resolver.expect("resolver should exist for non-passthrough key"); + assert_eq!( + resolver.resolve_placeholder("openshell:resolve:env:CLAUDE_API_KEY"), + Some("sk-real") + ); + // Resolver must not learn the real value of the passthrough key. + assert!( + resolver + .resolve_placeholder("openshell:resolve:env:SLACK_BOT_TOKEN") + .is_none() + ); + } + + #[test] + fn all_passthrough_produces_no_resolver() { + let provider_env: HashMap = [ + ("SLACK_BOT_TOKEN".to_string(), "xoxb-real".to_string()), + ("SLACK_SIGNING_SECRET".to_string(), "sig-real".to_string()), + ] + .into_iter() + .collect(); + let (child_env, resolver) = SecretResolver::from_provider_env_with_passthrough( + provider_env, + &[ + "SLACK_BOT_TOKEN".to_string(), + "SLACK_SIGNING_SECRET".to_string(), + ], + ); + + assert_eq!( + child_env.get("SLACK_BOT_TOKEN"), + Some(&"xoxb-real".to_string()) + ); + assert_eq!( + child_env.get("SLACK_SIGNING_SECRET"), + Some(&"sig-real".to_string()) + ); + assert!( + resolver.is_none(), + "resolver should be None when every key is passthrough" + ); + } + + #[test] + fn passthrough_entry_for_unrelated_key_is_silently_ignored() { + // A passthrough key not present in provider_env is a no-op (the server + // already validates this on Provider creation; the supervisor must not + // crash if it ever receives a stale list). + let provider_env: HashMap = [("API_KEY".to_string(), "real".to_string())] + .into_iter() + .collect(); + let (child_env, resolver) = SecretResolver::from_provider_env_with_passthrough( + provider_env, + &["UNRELATED_KEY".to_string()], + ); + + assert_eq!( + child_env.get("API_KEY"), + Some(&"openshell:resolve:env:API_KEY".to_string()) + ); + assert!(resolver.is_some()); + } + // === Existing tests (preserved) === #[test] diff --git a/crates/openshell-server/src/grpc/policy.rs b/crates/openshell-server/src/grpc/policy.rs index ff5625278..df9cc427a 100644 --- a/crates/openshell-server/src/grpc/policy.rs +++ b/crates/openshell-server/src/grpc/policy.rs @@ -490,19 +490,21 @@ 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.environment.len(), + passthrough_count = resolved.passthrough_keys.len(), "GetSandboxProviderEnvironment request completed successfully" ); Ok(Response::new(GetSandboxProviderEnvironmentResponse { - environment, + environment: resolved.environment, + passthrough_keys: resolved.passthrough_keys, })) } diff --git a/crates/openshell-server/src/grpc/provider.rs b/crates/openshell-server/src/grpc/provider.rs index a9b18b7eb..a7895b636 100644 --- a/crates/openshell-server/src/grpc/provider.rs +++ b/crates/openshell-server/src/grpc/provider.rs @@ -150,11 +150,30 @@ pub(super) async fn update_provider_record( )); } + let merged_credentials = merge_map(existing.credentials, provider.credentials); + let merged_config = merge_map(existing.config, provider.config); + // Passthrough merge with orphan pruning: when the caller does not declare a + // new passthrough list (empty incoming = preserve), drop any entries whose + // credential was deleted in this same update. Without this, deleting a + // credential that was marked passthrough leaves the provider in a state + // that fails validation, and the operator cannot recover without + // recreating the provider. When the caller declares a non-empty list, keep + // strict validation — an explicit orphan is a caller bug and must surface. + let merged_passthrough = if provider.passthrough_credentials.is_empty() { + existing + .passthrough_credentials + .into_iter() + .filter(|key| merged_credentials.contains_key(key)) + .collect() + } else { + provider.passthrough_credentials + }; let updated = Provider { metadata: existing.metadata, r#type: existing.r#type, - credentials: merge_map(existing.credentials, provider.credentials), - config: merge_map(existing.config, provider.config), + credentials: merged_credentials, + config: merged_config, + passthrough_credentials: merged_passthrough, }; // Ensure metadata is valid (defense in depth - existing.metadata should always be valid) @@ -207,21 +226,38 @@ fn merge_map( // Provider environment resolution // --------------------------------------------------------------------------- +/// Resolved provider environment for a sandbox. +#[derive(Debug)] +pub(super) struct ProviderEnvironment { + /// Credential environment variables (canonical and passthrough mixed). + pub environment: std::collections::HashMap, + /// Subset of `environment` keys whose value is the real credential and + /// must be injected directly into the agent process — bypassing the + /// canonical placeholder substitution and L7 proxy rewriting. + 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 and the passthrough opt-in list. +/// Returns a [`ProviderEnvironment`] describing the env map and which keys are +/// passthrough. When duplicate keys appear across providers, the first +/// provider's value wins; a key is treated as passthrough if any provider that +/// owns the winning value lists it as passthrough. 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(ProviderEnvironment { + environment: std::collections::HashMap::new(), + passthrough_keys: Vec::new(), + }); } let mut env = std::collections::HashMap::new(); + let mut passthrough = std::collections::HashSet::new(); for name in provider_names { let provider = store @@ -230,20 +266,37 @@ 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 provider_passthrough: std::collections::HashSet<&str> = provider + .passthrough_credentials + .iter() + .map(String::as_str) + .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; + } + if !env.contains_key(key) { + env.insert(key.clone(), value.clone()); + if provider_passthrough.contains(key.as_str()) { + passthrough.insert(key.clone()); + } } } } - Ok(env) + let mut passthrough_keys: Vec = passthrough.into_iter().collect(); + passthrough_keys.sort(); + + Ok(ProviderEnvironment { + environment: env, + passthrough_keys, + }) } pub(super) fn is_valid_env_key(key: &str) -> bool { @@ -392,6 +445,7 @@ mod tests { ] .into_iter() .collect(), + passthrough_credentials: Vec::new(), } } @@ -437,6 +491,7 @@ mod tests { .collect(), config: std::iter::once(("endpoint".to_string(), "https://gitlab.com".to_string())) .collect(), + passthrough_credentials: Vec::new(), }, ) .await @@ -505,6 +560,7 @@ mod tests { r#type: String::new(), credentials: HashMap::new(), config: HashMap::new(), + passthrough_credentials: Vec::new(), }, ) .await @@ -529,6 +585,7 @@ mod tests { r#type: String::new(), credentials: HashMap::new(), config: HashMap::new(), + passthrough_credentials: Vec::new(), }, ) .await @@ -557,6 +614,7 @@ mod tests { r#type: String::new(), credentials: HashMap::new(), config: HashMap::new(), + passthrough_credentials: Vec::new(), }, ) .await @@ -604,6 +662,7 @@ 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(), + passthrough_credentials: Vec::new(), }, ) .await @@ -655,6 +714,7 @@ mod tests { r#type: String::new(), credentials: HashMap::new(), config: HashMap::new(), + passthrough_credentials: Vec::new(), }, ) .await @@ -684,6 +744,7 @@ mod tests { r#type: "openai".to_string(), credentials: HashMap::new(), config: HashMap::new(), + passthrough_credentials: Vec::new(), }, ) .await @@ -715,6 +776,7 @@ mod tests { r#type: String::new(), credentials: std::iter::once((oversized_key, "value".to_string())).collect(), config: HashMap::new(), + passthrough_credentials: Vec::new(), }, ) .await @@ -723,11 +785,121 @@ mod tests { assert_eq!(err.code(), Code::InvalidArgument); } + #[tokio::test] + async fn update_provider_prunes_passthrough_for_deleted_credential() { + let store = Store::connect("sqlite::memory:?cache=shared") + .await + .unwrap(); + + create_provider_record( + &store, + Provider { + metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { + id: String::new(), + name: "slack".to_string(), + created_at_ms: 0, + labels: HashMap::new(), + }), + r#type: "slack".to_string(), + credentials: [ + ("SLACK_BOT_TOKEN".to_string(), "xoxb".to_string()), + ("SLACK_SIGNING_SECRET".to_string(), "sig".to_string()), + ] + .into_iter() + .collect(), + config: HashMap::new(), + passthrough_credentials: vec![ + "SLACK_BOT_TOKEN".to_string(), + "SLACK_SIGNING_SECRET".to_string(), + ], + }, + ) + .await + .unwrap(); + + // Caller deletes the bot token without touching the passthrough list. + // The orphaned passthrough entry must be pruned, not block the update. + let updated = update_provider_record( + &store, + Provider { + metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { + id: String::new(), + name: "slack".to_string(), + created_at_ms: 0, + labels: HashMap::new(), + }), + r#type: String::new(), + credentials: std::iter::once(("SLACK_BOT_TOKEN".to_string(), String::new())) + .collect(), + config: HashMap::new(), + passthrough_credentials: Vec::new(), + }, + ) + .await + .unwrap(); + + assert!(!updated.credentials.contains_key("SLACK_BOT_TOKEN")); + assert!(updated.credentials.contains_key("SLACK_SIGNING_SECRET")); + assert_eq!( + updated.passthrough_credentials, + vec!["SLACK_SIGNING_SECRET".to_string()] + ); + } + + #[tokio::test] + async fn update_provider_rejects_explicit_passthrough_orphan() { + // When the caller declares a non-empty passthrough list, an entry that + // names a non-existent credential is a caller bug and must surface. + let store = Store::connect("sqlite::memory:?cache=shared") + .await + .unwrap(); + + create_provider_record( + &store, + Provider { + metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { + id: String::new(), + name: "slack-explicit".to_string(), + created_at_ms: 0, + labels: HashMap::new(), + }), + r#type: "slack".to_string(), + credentials: std::iter::once(("API_KEY".to_string(), "v".to_string())).collect(), + config: HashMap::new(), + passthrough_credentials: Vec::new(), + }, + ) + .await + .unwrap(); + + let err = update_provider_record( + &store, + Provider { + metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { + id: String::new(), + name: "slack-explicit".to_string(), + created_at_ms: 0, + labels: HashMap::new(), + }), + r#type: String::new(), + credentials: HashMap::new(), + config: HashMap::new(), + passthrough_credentials: vec!["UNKNOWN_KEY".to_string()], + }, + ) + .await + .unwrap_err(); + + assert_eq!(err.code(), Code::InvalidArgument); + assert!(err.message().contains("not present in credentials")); + } + #[tokio::test] 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.environment.is_empty()); + assert!(result.passthrough_keys.is_empty()); } #[tokio::test] @@ -752,15 +924,23 @@ mod tests { "https://api.anthropic.com".to_string(), )) .collect(), + 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.environment.get("ANTHROPIC_API_KEY"), + Some(&"sk-abc".to_string()) + ); + assert_eq!( + result.environment.get("CLAUDE_API_KEY"), + Some(&"sk-abc".to_string()) + ); + assert!(!result.environment.contains_key("endpoint")); + assert!(result.passthrough_keys.is_empty()); } #[tokio::test] @@ -792,15 +972,19 @@ mod tests { .into_iter() .collect(), config: 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.environment.get("VALID_KEY"), + Some(&"value".to_string()) + ); + assert!(!result.environment.contains_key("nested.api_key")); + assert!(!result.environment.contains_key("bad-key")); } #[tokio::test] @@ -822,6 +1006,7 @@ mod tests { )) .collect(), config: HashMap::new(), + passthrough_credentials: Vec::new(), }, ) .await @@ -839,6 +1024,7 @@ mod tests { credentials: std::iter::once(("GITLAB_TOKEN".to_string(), "glpat-xyz".to_string())) .collect(), config: HashMap::new(), + passthrough_credentials: Vec::new(), }, ) .await @@ -850,8 +1036,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.environment.get("ANTHROPIC_API_KEY"), + Some(&"sk-abc".to_string()) + ); + assert_eq!( + result.environment.get("GITLAB_TOKEN"), + Some(&"glpat-xyz".to_string()) + ); } #[tokio::test] @@ -870,6 +1062,7 @@ mod tests { credentials: std::iter::once(("SHARED_KEY".to_string(), "first-value".to_string())) .collect(), config: HashMap::new(), + passthrough_credentials: Vec::new(), }, ) .await @@ -890,6 +1083,7 @@ mod tests { )) .collect(), config: HashMap::new(), + passthrough_credentials: Vec::new(), }, ) .await @@ -901,7 +1095,114 @@ mod tests { ) .await .unwrap(); - assert_eq!(result.get("SHARED_KEY"), Some(&"first-value".to_string())); + assert_eq!( + result.environment.get("SHARED_KEY"), + Some(&"first-value".to_string()) + ); + } + + #[tokio::test] + async fn resolve_provider_env_propagates_passthrough_keys() { + let store = Store::connect("sqlite::memory:").await.unwrap(); + create_provider_record( + &store, + Provider { + metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { + id: String::new(), + name: "slack".to_string(), + created_at_ms: 0, + labels: HashMap::new(), + }), + r#type: "slack".to_string(), + credentials: [ + ("SLACK_BOT_TOKEN".to_string(), "xoxb-real".to_string()), + ("SLACK_SIGNING_SECRET".to_string(), "sig-real".to_string()), + ] + .into_iter() + .collect(), + config: HashMap::new(), + passthrough_credentials: vec![ + "SLACK_BOT_TOKEN".to_string(), + "SLACK_SIGNING_SECRET".to_string(), + ], + }, + ) + .await + .unwrap(); + + let result = resolve_provider_environment(&store, &["slack".to_string()]) + .await + .unwrap(); + let mut keys = result.passthrough_keys.clone(); + keys.sort(); + assert_eq!( + keys, + vec![ + "SLACK_BOT_TOKEN".to_string(), + "SLACK_SIGNING_SECRET".to_string(), + ] + ); + assert_eq!( + result.environment.get("SLACK_BOT_TOKEN"), + Some(&"xoxb-real".to_string()) + ); + } + + #[tokio::test] + async fn resolve_provider_env_skips_passthrough_when_first_provider_wins_canonical() { + // When two providers declare the same key but only the second marks it + // as passthrough, the first provider's value wins (existing semantics) + // and the key stays canonical. + let store = Store::connect("sqlite::memory:").await.unwrap(); + create_provider_record( + &store, + Provider { + metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { + id: String::new(), + name: "first".to_string(), + created_at_ms: 0, + labels: HashMap::new(), + }), + r#type: "claude".to_string(), + credentials: std::iter::once(("API_KEY".to_string(), "first-value".to_string())) + .collect(), + config: HashMap::new(), + passthrough_credentials: Vec::new(), + }, + ) + .await + .unwrap(); + create_provider_record( + &store, + Provider { + metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { + id: String::new(), + name: "second".to_string(), + created_at_ms: 0, + labels: HashMap::new(), + }), + r#type: "anthropic".to_string(), + credentials: std::iter::once(("API_KEY".to_string(), "second-value".to_string())) + .collect(), + config: HashMap::new(), + passthrough_credentials: vec!["API_KEY".to_string()], + }, + ) + .await + .unwrap(); + + let result = + resolve_provider_environment(&store, &["first".to_string(), "second".to_string()]) + .await + .unwrap(); + assert_eq!( + result.environment.get("API_KEY"), + Some(&"first-value".to_string()) + ); + assert!( + result.passthrough_keys.is_empty(), + "passthrough must follow the winning value, not the losing provider's opt-in" + ); } #[tokio::test] @@ -926,6 +1227,7 @@ mod tests { )) .collect(), config: HashMap::new(), + passthrough_credentials: Vec::new(), }, ) .await @@ -954,11 +1256,14 @@ mod tests { .unwrap() .unwrap(); let spec = loaded.spec.unwrap(); - let env = resolve_provider_environment(&store, &spec.providers) + let resolved = resolve_provider_environment(&store, &spec.providers) .await .unwrap(); - assert_eq!(env.get("ANTHROPIC_API_KEY"), Some(&"sk-test".to_string())); + assert_eq!( + resolved.environment.get("ANTHROPIC_API_KEY"), + Some(&"sk-test".to_string()) + ); } #[tokio::test] @@ -987,11 +1292,12 @@ mod tests { .unwrap() .unwrap(); let spec = loaded.spec.unwrap(); - let env = resolve_provider_environment(&store, &spec.providers) + let resolved = resolve_provider_environment(&store, &spec.providers) .await .unwrap(); - assert!(env.is_empty()); + assert!(resolved.environment.is_empty()); + assert!(resolved.passthrough_keys.is_empty()); } #[tokio::test] diff --git a/crates/openshell-server/src/grpc/validation.rs b/crates/openshell-server/src/grpc/validation.rs index 160b7e031..8c7b1c7b1 100644 --- a/crates/openshell-server/src/grpc/validation.rs +++ b/crates/openshell-server/src/grpc/validation.rs @@ -267,6 +267,44 @@ pub(super) fn validate_provider_fields(provider: &Provider) -> Result<(), Status MAX_MAP_VALUE_LEN, "provider.config", )?; + validate_passthrough_credentials(provider)?; + Ok(()) +} + +/// Validate the `passthrough_credentials` field: each entry must be a valid +/// environment variable name, must reference a key present in `credentials`, +/// and the list must not contain duplicates. +fn validate_passthrough_credentials(provider: &Provider) -> Result<(), Status> { + if provider.passthrough_credentials.len() > MAX_PROVIDER_CREDENTIALS_ENTRIES { + return Err(Status::invalid_argument(format!( + "provider.passthrough_credentials exceeds maximum entries ({} > {MAX_PROVIDER_CREDENTIALS_ENTRIES})", + provider.passthrough_credentials.len() + ))); + } + let mut seen = std::collections::HashSet::with_capacity(provider.passthrough_credentials.len()); + for key in &provider.passthrough_credentials { + if key.len() > MAX_MAP_KEY_LEN { + return Err(Status::invalid_argument(format!( + "provider.passthrough_credentials key exceeds maximum length ({} > {MAX_MAP_KEY_LEN})", + key.len() + ))); + } + if !super::provider::is_valid_env_key(key) { + return Err(Status::invalid_argument(format!( + "provider.passthrough_credentials contains invalid env var name: '{key}'" + ))); + } + if !provider.credentials.contains_key(key) { + return Err(Status::invalid_argument(format!( + "provider.passthrough_credentials key '{key}' is not present in credentials" + ))); + } + if !seen.insert(key.as_str()) { + return Err(Status::invalid_argument(format!( + "provider.passthrough_credentials contains duplicate entry: '{key}'" + ))); + } + } Ok(()) } @@ -878,6 +916,7 @@ mod tests { r#type: provider_type.to_string(), credentials, config, + passthrough_credentials: Vec::new(), } } @@ -961,6 +1000,54 @@ mod tests { assert!(err.message().contains("key")); } + #[test] + fn validate_provider_fields_accepts_passthrough_subset_of_credentials() { + let creds: HashMap = [ + ("API_KEY".to_string(), "v1".to_string()), + ("SIGNING_SECRET".to_string(), "v2".to_string()), + ] + .into_iter() + .collect(); + let mut provider = make_test_provider("ok", "claude", creds, HashMap::new()); + provider.passthrough_credentials = vec!["SIGNING_SECRET".to_string()]; + assert!(validate_provider_fields(&provider).is_ok()); + } + + #[test] + fn validate_provider_fields_accepts_empty_passthrough() { + let provider = make_test_provider("ok", "claude", one_credential(), HashMap::new()); + assert!(validate_provider_fields(&provider).is_ok()); + } + + #[test] + fn validate_provider_fields_rejects_passthrough_key_absent_from_credentials() { + let mut provider = make_test_provider("ok", "claude", one_credential(), HashMap::new()); + provider.passthrough_credentials = vec!["MISSING".to_string()]; + let err = validate_provider_fields(&provider).unwrap_err(); + assert_eq!(err.code(), Code::InvalidArgument); + assert!(err.message().contains("not present in credentials")); + } + + #[test] + fn validate_provider_fields_rejects_invalid_passthrough_env_var_name() { + let mut creds = one_credential(); + creds.insert("bad-key".to_string(), "v".to_string()); + let mut provider = make_test_provider("ok", "claude", creds, HashMap::new()); + provider.passthrough_credentials = vec!["bad-key".to_string()]; + let err = validate_provider_fields(&provider).unwrap_err(); + assert_eq!(err.code(), Code::InvalidArgument); + assert!(err.message().contains("invalid env var name")); + } + + #[test] + fn validate_provider_fields_rejects_duplicate_passthrough_entries() { + let mut provider = make_test_provider("ok", "claude", one_credential(), HashMap::new()); + provider.passthrough_credentials = vec!["KEY".to_string(), "KEY".to_string()]; + let err = validate_provider_fields(&provider).unwrap_err(); + assert_eq!(err.code(), Code::InvalidArgument); + assert!(err.message().contains("duplicate")); + } + #[test] fn validate_provider_fields_rejects_oversized_config_value() { let mut config = HashMap::new(); diff --git a/crates/openshell-server/src/inference.rs b/crates/openshell-server/src/inference.rs index b52700f0d..7bfab276b 100644 --- a/crates/openshell-server/src/inference.rs +++ b/crates/openshell-server/src/inference.rs @@ -511,6 +511,7 @@ 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(), + passthrough_credentials: Vec::new(), } } @@ -675,6 +676,7 @@ mod tests { "https://station.example.com/v1".to_string(), )) .collect(), + passthrough_credentials: Vec::new(), }; store .put_message(&provider) @@ -749,6 +751,7 @@ mod tests { credentials: std::iter::once(("OPENAI_API_KEY".to_string(), "sk-rotated".to_string())) .collect(), config: provider.config.clone(), + passthrough_credentials: provider.passthrough_credentials.clone(), }; store .put_message(&rotated_provider) diff --git a/crates/openshell-tui/src/lib.rs b/crates/openshell-tui/src/lib.rs index 8571ebbe1..fd97d6713 100644 --- a/crates/openshell-tui/src/lib.rs +++ b/crates/openshell-tui/src/lib.rs @@ -1569,6 +1569,7 @@ fn spawn_create_provider(app: &App, tx: mpsc::UnboundedSender) { r#type: ptype.clone(), credentials: credentials.clone(), config: HashMap::default(), + passthrough_credentials: Vec::new(), }), }; @@ -1659,6 +1660,7 @@ fn spawn_update_provider(app: &App, tx: mpsc::UnboundedSender) { r#type: ptype, credentials, config: HashMap::default(), + passthrough_credentials: Vec::new(), }), }; diff --git a/docs/sandboxes/manage-providers.mdx b/docs/sandboxes/manage-providers.mdx index fbfc4d380..2b886e2de 100644 --- a/docs/sandboxes/manage-providers.mdx +++ b/docs/sandboxes/manage-providers.mdx @@ -147,6 +147,30 @@ openshell provider create --name google --type generic --credential YOUTUBE_API_ The agent sends `GET /youtube/v3/search?part=snippet&key=`. The proxy resolves the placeholder in the query parameter value and percent-encodes the result before forwarding. +## Selective Passthrough + +Some SDKs reject the placeholder before any HTTP request is made — for example Slack's `@slack/web-api` checks the `xoxb-` prefix on bot tokens, and OAuth libraries parse JWT structure in process. Other credentials never reach the wire at all (Slack signing-secret HMAC, Discord interaction-signature verification) or flow through transports the proxy cannot rewrite (WebSocket payloads after HTTP `101 Switching Protocols`). + +For these cases, mark a credential key as **passthrough**: the supervisor injects the real value directly into the agent process instead of a placeholder. + +```shell +openshell provider create \ + --name slack \ + --type generic \ + --credential SLACK_BOT_TOKEN=xoxb-real-value \ + --credential SLACK_SIGNING_SECRET=real-signing-secret \ + --passthrough SLACK_BOT_TOKEN \ + --passthrough SLACK_SIGNING_SECRET +``` + +`--passthrough` is repeatable. Each key must also appear in `--credential`. The same flag is available on `openshell provider update`. + + +Passthrough drops the "agent never sees the real secret" invariant for the listed keys. The real value sits in `/proc//environ` and any descendant process inherits it. Use it only when the consumer demonstrably fails with the canonical placeholder — prefer the placeholder model whenever the consumer permits it. + + +`openshell provider get ` annotates passthrough keys in its output, and `openshell provider list` shows a `PASSTHROUGH` column with the per-provider count. + ## Supported Provider Types The following provider types are supported. diff --git a/e2e/python/test_sandbox_providers.py b/e2e/python/test_sandbox_providers.py index 0e9349dc0..895c61b2b 100644 --- a/e2e/python/test_sandbox_providers.py +++ b/e2e/python/test_sandbox_providers.py @@ -58,6 +58,7 @@ def provider( name: str, provider_type: str, credentials: dict[str, str], + passthrough_credentials: list[str] | None = None, ) -> Iterator[str]: """Create a provider for the duration of the block, then delete it.""" _delete_provider(stub, name) @@ -67,6 +68,7 @@ def provider( metadata=datamodel_pb2.ObjectMeta(name=name), type=provider_type, credentials=credentials, + passthrough_credentials=passthrough_credentials or [], ) ) ) @@ -183,6 +185,81 @@ def read_nvidia_key() -> str: assert result.stdout.strip() == "openshell:resolve:env:NVIDIA_API_KEY" +# =========================================================================== +# Tests: selective passthrough +# =========================================================================== + + +def test_passthrough_credential_visible_as_real_value( + sandbox: Callable[..., Sandbox], + sandbox_client: SandboxClient, +) -> None: + """A credential listed in passthrough_credentials is injected as the real + value, not as the canonical openshell:resolve:env:* placeholder.""" + real_value = "xoxb-e2e-passthrough-real-value-for-slack" + with provider( + sandbox_client._stub, + name="e2e-test-passthrough-real-value", + provider_type="generic", + credentials={"SLACK_BOT_TOKEN": real_value}, + passthrough_credentials=["SLACK_BOT_TOKEN"], + ) as provider_name: + spec = datamodel_pb2.SandboxSpec( + policy=_default_policy(), + providers=[provider_name], + ) + + def read_slack_token() -> str: + import os + + return os.environ.get("SLACK_BOT_TOKEN", "NOT_SET") + + with sandbox(spec=spec, delete_on_exit=True) as sb: + result = sb.exec_python(read_slack_token) + assert result.exit_code == 0, result.stderr + value = result.stdout.strip() + assert value == real_value + assert value != "openshell:resolve:env:SLACK_BOT_TOKEN" + + +def test_passthrough_and_canonical_credentials_coexist( + sandbox: Callable[..., Sandbox], + sandbox_client: SandboxClient, +) -> None: + """A provider with one passthrough and one canonical credential injects + each in its own mode: real value for the passthrough key, canonical + placeholder for the other.""" + real_signing_secret = "e2e-passthrough-real-signing-secret" + with provider( + sandbox_client._stub, + name="e2e-test-passthrough-mixed", + provider_type="generic", + credentials={ + "SLACK_SIGNING_SECRET": real_signing_secret, + "SLACK_API_KEY": "irrelevant-canonical-value", + }, + passthrough_credentials=["SLACK_SIGNING_SECRET"], + ) as provider_name: + spec = datamodel_pb2.SandboxSpec( + policy=_default_policy(), + providers=[provider_name], + ) + + def read_both_envs() -> str: + import os + + signing = os.environ.get("SLACK_SIGNING_SECRET", "NOT_SET") + api = os.environ.get("SLACK_API_KEY", "NOT_SET") + return f"{signing}|{api}" + + with sandbox(spec=spec, delete_on_exit=True) as sb: + result = sb.exec_python(read_both_envs) + assert result.exit_code == 0, result.stderr + signing, api = result.stdout.strip().split("|", 1) + assert signing == real_signing_secret + assert api == "openshell:resolve:env:SLACK_API_KEY" + + # =========================================================================== # Tests: security & edge cases # =========================================================================== diff --git a/proto/datamodel.proto b/proto/datamodel.proto index 534b043ae..b61816304 100644 --- a/proto/datamodel.proto +++ b/proto/datamodel.proto @@ -34,4 +34,18 @@ message Provider { map credentials = 3; // Non-secret provider configuration. map config = 4; + // Subset of `credentials` keys whose real values should be injected directly + // into the agent process environment instead of the canonical + // `openshell:resolve:env:` placeholder. The L7 proxy does not rewrite + // these on egress because the real value is already on the client side. + // + // Use sparingly: this drops the "agent never sees the real secret" invariant + // for the listed keys. Required for SDKs that validate credential format + // in-process (Slack `xoxb-*`/`xapp-*`), credentials consumed in in-process + // crypto (HMAC signing secrets, signature verification), and transports the + // proxy cannot rewrite (WebSocket payloads after HTTP 101 upgrade). + // + // Each entry must be a valid environment variable name and present in + // `credentials`. + repeated string passthrough_credentials = 5; } diff --git a/proto/openshell.proto b/proto/openshell.proto index 75490f338..17afcaebf 100644 --- a/proto/openshell.proto +++ b/proto/openshell.proto @@ -577,6 +577,11 @@ message GetSandboxProviderEnvironmentRequest { message GetSandboxProviderEnvironmentResponse { // Provider credential environment variables. map environment = 1; + // Subset of keys in `environment` whose value is the real credential and + // must be injected directly into the agent process — bypassing the + // canonical placeholder substitution and L7 proxy rewriting. Sourced from + // each provider's `passthrough_credentials` list. + repeated string passthrough_keys = 2; } // ---------------------------------------------------------------------------