From b942fa51dd82591d848e00a56ad8dbd4f4a7b5ae Mon Sep 17 00:00:00 2001 From: John Myers <9696606+johntmyers@users.noreply.github.com> Date: Tue, 31 Mar 2026 11:58:01 -0700 Subject: [PATCH 1/3] feat(sandbox): extend L7 credential injection to query params, Basic auth, and URL paths Closes #689 Extend SecretResolver to resolve openshell:resolve:env:* placeholders in URL query parameters, Basic auth tokens, and URL path segments. Absorbs working code from PR #631 for query param and Basic auth support. Adds path rewriting for Telegram-style APIs (/bot{TOKEN}/method). Changes all placeholder rewriting to fail-closed: unresolved placeholders cause HTTP 500 instead of forwarding raw placeholder strings. Validates resolved secret values for CRLF/null injection (CWE-113). Validates path credentials for traversal sequences (CWE-22). Rewrites request targets before OPA L7 policy evaluation. OPA receives a redacted path with [CREDENTIAL] markers. Real secrets appear only in the upstream connection. All log statements use redacted targets. --- Cargo.lock | 1 + architecture/sandbox-providers.md | 33 +- architecture/sandbox.md | 151 ++- crates/openshell-sandbox/Cargo.toml | 3 + crates/openshell-sandbox/src/l7/relay.rs | 73 +- crates/openshell-sandbox/src/l7/rest.rs | 32 +- crates/openshell-sandbox/src/proxy.rs | 39 +- crates/openshell-sandbox/src/secrets.rs | 1270 +++++++++++++++++++++- docs/sandboxes/manage-providers.md | 41 + 9 files changed, 1564 insertions(+), 79 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9d8247e5d..07eb73bbf 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2935,6 +2935,7 @@ name = "openshell-sandbox" version = "0.0.0" dependencies = [ "anyhow", + "base64 0.22.1", "bytes", "clap", "hex", diff --git a/architecture/sandbox-providers.md b/architecture/sandbox-providers.md index 16b7948bc..fe5d48a97 100644 --- a/architecture/sandbox-providers.md +++ b/architecture/sandbox-providers.md @@ -305,18 +305,31 @@ start from `env_clear()`, so the handshake secret is not present there. ### Proxy-Time Secret Resolution -When a sandboxed tool uses one of these placeholder env vars to populate an outbound HTTP -header (for example `Authorization: Bearer openshell:resolve:env:ANTHROPIC_API_KEY`), the -sandbox proxy rewrites the placeholder to the real secret value immediately before the -request is forwarded upstream. - -This applies to: - -- forward-proxy HTTP requests, and -- L7-inspected REST requests inside CONNECT tunnels. +When a sandboxed tool uses one of these placeholder env vars in an outbound HTTP request, +the sandbox proxy rewrites the placeholder to the real secret value immediately before the +request is forwarded upstream. Placeholders are resolved in four locations: + +- **HTTP header values** — exact match (`x-api-key: openshell:resolve:env:KEY`), prefixed + match (`Authorization: Bearer openshell:resolve:env:KEY`), and Base64-decoded Basic auth + tokens (`Authorization: Basic `) +- **URL query parameters** — for APIs that authenticate via query string + (e.g., `?key=openshell:resolve:env:YOUTUBE_API_KEY`) +- **URL path segments** — for APIs that embed tokens in the URL path + (e.g., `/bot/sendMessage` for Telegram Bot API) + +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 +credentials are additionally validated to reject traversal sequences, path separators, and +URI delimiters (CWE-22). The real secret value remains in supervisor memory only; it is not re-injected into the -child process environment. +child process environment. See [Credential injection](sandbox.md#credential-injection) for +the full implementation details, encoding rules, and security properties. ### End-to-End Flow diff --git a/architecture/sandbox.md b/architecture/sandbox.md index 333cef5ea..c870708dd 100644 --- a/architecture/sandbox.md +++ b/architecture/sandbox.md @@ -33,6 +33,7 @@ All paths are relative to `crates/openshell-sandbox/src/`. | `l7/relay.rs` | Protocol-aware bidirectional relay with per-request OPA evaluation, credential-injection-only passthrough relay | | `l7/rest.rs` | HTTP/1.1 request/response parsing, body framing (Content-Length, chunked), deny response generation | | `l7/provider.rs` | `L7Provider` trait and `L7Request`/`BodyLength` types | +| `secrets.rs` | `SecretResolver` credential placeholder system — placeholder generation, multi-location rewriting (headers, query params, path segments, Basic auth), fail-closed scanning, secret validation, percent-encoding | ## Startup and Orchestration @@ -824,11 +825,13 @@ When `Router::proxy_with_candidates()` returns an error, `router_error_to_http() | `RouterError` variant | HTTP status | Response body | |----------------------|-------------|---------------| -| `RouteNotFound(hint)` | `400` | `no route configured for route '{hint}'` | -| `NoCompatibleRoute(protocol)` | `400` | `no compatible route for source protocol '{protocol}'` | -| `Unauthorized(msg)` | `401` | `{msg}` | -| `UpstreamUnavailable(msg)` | `503` | `{msg}` | -| `UpstreamProtocol(msg)` / `Internal(msg)` | `502` | `{msg}` | +| `RouteNotFound(_)` | `400` | `no inference route configured` | +| `NoCompatibleRoute(_)` | `400` | `no compatible inference route available` | +| `Unauthorized(_)` | `401` | `unauthorized` | +| `UpstreamUnavailable(_)` | `503` | `inference service unavailable` | +| `UpstreamProtocol(_)` / `Internal(_)` | `502` | `inference service error` | + +Response messages are generic — internal details (upstream URLs, hostnames, TLS errors, route hints) are never exposed to the sandboxed process. Full error context is logged server-side at `warn` level. ### Inference routing context @@ -1027,20 +1030,131 @@ TLS termination is automatic. The proxy peeks the first bytes of every CONNECT t System CA bundles are searched at well-known paths: `/etc/ssl/certs/ca-certificates.crt` (Debian/Ubuntu), `/etc/pki/tls/certs/ca-bundle.crt` (RHEL), `/etc/ssl/ca-bundle.pem` (openSUSE), `/etc/ssl/cert.pem` (Alpine/macOS). -### Credential-injection-only relay +### Credential injection + +**Files:** `crates/openshell-sandbox/src/secrets.rs`, `crates/openshell-sandbox/src/l7/relay.rs`, `crates/openshell-sandbox/src/l7/rest.rs`, `crates/openshell-sandbox/src/proxy.rs` + +The sandbox proxy resolves `openshell:resolve:env:*` credential placeholders in outbound HTTP requests. The `SecretResolver` holds a supervisor-only map from placeholder strings to real secret values, constructed at startup from the provider environment. Child processes only see placeholder values in their environment; the proxy rewrites them to real secrets immediately before forwarding upstream. + +#### `SecretResolver` + +```rust +pub(crate) struct SecretResolver { + by_placeholder: HashMap, +} +``` + +`SecretResolver::from_provider_env()` splits the provider environment into two maps: a child-visible map with placeholder values (`openshell:resolve:env:ANTHROPIC_API_KEY`) and a supervisor-only resolver map (`{"openshell:resolve:env:ANTHROPIC_API_KEY": "sk-real-key"}`). The placeholder grammar is `openshell:resolve:env:[A-Za-z_][A-Za-z0-9_]*`. + +#### Credential placement locations + +The resolver rewrites placeholders in four locations within HTTP requests: + +| Location | Example | Encoding | Implementation | +|----------|---------|----------|----------------| +| Header value (exact) | `x-api-key: openshell:resolve:env:KEY` | None (raw replacement) | `rewrite_header_value()` | +| Header value (prefixed) | `Authorization: Bearer openshell:resolve:env:KEY` | None (prefix preserved) | `rewrite_header_value()` | +| Basic auth token | `Authorization: Basic ` | Base64 decode → resolve → re-encode | `rewrite_basic_auth_token()` | +| URL query parameter | `?key=openshell:resolve:env:KEY` | Percent-decode → resolve → percent-encode (RFC 3986 unreserved) | `rewrite_uri_query_params()` | +| URL path segment | `/bot/sendMessage` | Percent-decode → resolve → validate → percent-encode (RFC 3986 pchar) | `rewrite_uri_path()` → `rewrite_path_segment()` | + +**Header values**: Direct match replaces the entire value. Prefixed match (e.g., `Bearer `) splits on whitespace, resolves the placeholder portion, and reassembles. Basic auth match detects `Authorization: Basic `, decodes the Base64 content, resolves any placeholders in the decoded `user:password` string, and re-encodes. + +**Query parameters**: Each `key=value` pair is checked. Values are percent-decoded before resolution and percent-encoded after (RFC 3986 Section 2.3 unreserved characters preserved: `ALPHA / DIGIT / "-" / "." / "_" / "~"`). + +**Path segments**: Handles substring matching for APIs that embed tokens within path segments (e.g., Telegram's `/bot{TOKEN}/sendMessage`). Each segment is percent-decoded, scanned for placeholder boundaries using the env var key grammar (`[A-Za-z_][A-Za-z0-9_]*`), resolved, validated for path safety, and percent-encoded per RFC 3986 Section 3.3 pchar rules (`unreserved / sub-delims / ":" / "@"`). + +#### Path credential validation (CWE-22) + +Resolved credential values destined for URL path segments are validated by `validate_credential_for_path()` before insertion. The following values are rejected: + +| Pattern | Rejection reason | +|---------|-----------------| +| `../`, `..\\`, `..` | Path traversal sequence | +| `/`, `\` | Path separator | +| `\0`, `\r`, `\n` | Control character | +| `?`, `#` | URI delimiter | + +Rejection causes the request to fail closed (HTTP 500). + +#### Secret value validation (CWE-113) + +All resolved credential values are validated at the `resolve_placeholder()` level for prohibited control characters: CR (`\r`), LF (`\n`), and null byte (`\0`). This prevents HTTP header injection via malicious credential values. The validation applies to all placement locations automatically — header values, query parameters, and path segments all pass through `resolve_placeholder()`. + +#### Fail-closed behavior + +All placeholder rewriting fails closed. If any `openshell:resolve:env:*` placeholder is detected in the request but cannot be resolved, the proxy rejects the request with HTTP 500 instead of forwarding the raw placeholder to the upstream. The fail-closed mechanism operates at two levels: + +1. **Per-location**: Each rewrite function (`rewrite_uri_query_params`, `rewrite_path_segment`, `rewrite_header_line`) returns an `UnresolvedPlaceholderError` when a placeholder is detected but the resolver has no mapping for it. + +2. **Final scan**: After all rewriting completes, `rewrite_http_header_block()` scans the output for any remaining `openshell:resolve:env:` tokens. It also checks the percent-decoded form of the request line to catch encoded placeholder bypass attempts (e.g., `openshell%3Aresolve%3Aenv%3AUNKNOWN`). + +```rust +pub(crate) struct UnresolvedPlaceholderError { + pub location: &'static str, // "header", "query_param", "path" +} +``` + +#### Rewrite-before-OPA with redaction + +When L7 inspection is active, credential placeholders in the request target (path + query) are resolved BEFORE OPA L7 policy evaluation. This is implemented in `relay_with_inspection()` and `relay_passthrough_with_credentials()` in `l7/relay.rs`: + +1. `rewrite_target_for_eval()` resolves the request target, producing two strings: + - **Resolved**: real secrets inserted — used only for the upstream connection + - **Redacted**: `[CREDENTIAL]` markers in place of secrets — used for OPA input and logs + +2. OPA `evaluate_l7_request()` receives the redacted path in `request.path`, so policy rules never see real credential values. + +3. All log statements (`L7_REQUEST`, `HTTP_REQUEST`) use the redacted target. Real credential values never appear in logs. + +4. The resolved path (with real secrets) goes only to the upstream via `relay_http_request_with_resolver()`. + +```rust +pub(crate) struct RewriteTargetResult { + pub resolved: String, // for upstream forwarding only + pub redacted: String, // for OPA + logs +} +``` + +If credential resolution fails on the request target, the relay returns HTTP 500 and closes the connection. + +#### Credential-injection-only relay **File:** `crates/openshell-sandbox/src/l7/relay.rs` (`relay_passthrough_with_credentials()`) -When TLS is auto-terminated but no L7 policy (`protocol` + `access`/`rules`) is configured on the endpoint, the proxy enters a passthrough mode that still provides value: it parses HTTP requests minimally to rewrite credential placeholders (via `SecretResolver`) and logs each request for observability. This relay: +When TLS is auto-terminated but no L7 policy (`protocol` + `access`/`rules`) is configured on the endpoint, the proxy enters a passthrough mode that still provides credential injection and observability. This relay: 1. Reads each HTTP request from the client via `RestProvider::parse_request()` -2. Logs the request method, path, host, and port at `info!()` level (tagged `"HTTP relay (credential injection)"`) -3. Forwards the request to upstream via `relay_http_request_with_resolver()`, which rewrites headers containing `openshell:resolve:env:*` placeholders with actual provider credential values -4. Relays the upstream response back to the client -5. Loops for HTTP keep-alive; exits on client close or non-reusable response +2. Resolves and redacts the request target via `rewrite_target_for_eval()` (for log safety) +3. Logs the request method, redacted path, host, and port at `info!()` level (tagged `HTTP_REQUEST`) +4. Forwards the request to upstream via `relay_http_request_with_resolver()`, which rewrites all credential placeholders in headers, query parameters, path segments, and Basic auth tokens +5. Relays the upstream response back to the client +6. Loops for HTTP keep-alive; exits on client close or non-reusable response This enables credential injection on all HTTPS endpoints automatically, without requiring the policy author to add `protocol: rest` and `access: full` just to get credentials injected. +#### Known limitation: host-binding + +The resolver resolves all placeholders regardless of destination host. If an agent has OPA-allowed access to an attacker-controlled host, it could construct a URL containing a placeholder and exfiltrate the resolved credential value to that host. OPA host restrictions are the defense — only endpoints explicitly allowed by policy receive traffic. Per-credential host binding (restricting which credentials resolve for which destination hosts) is not implemented. + +#### Data flow + +```mermaid +sequenceDiagram + participant A as Agent Process + participant P as Proxy (SecretResolver) + participant O as OPA Engine + participant U as Upstream API + + A->>P: GET /bot/send?key= HTTP/1.1
Authorization: Bearer + P->>P: rewrite_target_for_eval(target)
→ resolved: /bot{secret}/send?key={secret}
→ redacted: /bot[CREDENTIAL]/send?key=[CREDENTIAL] + P->>O: evaluate_l7_request(redacted path) + O-->>P: allow + P->>P: rewrite_http_header_block(headers)
→ resolve header placeholders
→ resolve query param placeholders
→ resolve path segment placeholders
→ fail-closed scan + P->>U: GET /bot{secret}/send?key={secret} HTTP/1.1
Authorization: Bearer {secret} + Note over P: Logs use redacted path only +``` + ### REST protocol provider **File:** `crates/openshell-sandbox/src/l7/rest.rs` @@ -1060,11 +1174,12 @@ Implements `L7Provider` for HTTP/1.1: `relay_with_inspection()` in `crates/openshell-sandbox/src/l7/relay.rs` is the main relay loop: 1. Parse one HTTP request from client via the provider -2. Build L7 input JSON with `request.method`, `request.path`, `request.query_params`, plus the CONNECT-level context (host, port, binary, ancestors, cmdline) -3. Evaluate `data.openshell.sandbox.allow_request` and `data.openshell.sandbox.request_deny_reason` -4. Log the L7 decision (tagged `L7_REQUEST`) -5. If allowed (or audit mode): relay request to upstream and response back to client, then loop -6. If denied in enforce mode: send 403 and close the connection +2. Resolve credential placeholders in the request target via `rewrite_target_for_eval()`. OPA receives the redacted path (`[CREDENTIAL]` markers); the resolved path goes only to upstream. If resolution fails, return HTTP 500 and close the connection. +3. Build L7 input JSON with `request.method`, the **redacted** `request.path`, `request.query_params`, plus the CONNECT-level context (host, port, binary, ancestors, cmdline) +4. Evaluate `data.openshell.sandbox.allow_request` and `data.openshell.sandbox.request_deny_reason` +5. Log the L7 decision (tagged `L7_REQUEST`) using the redacted target — real credential values never appear in logs +6. If allowed (or audit mode): relay request to upstream via `relay_http_request_with_resolver()` (which rewrites all remaining credential placeholders in headers, query parameters, path segments, and Basic auth tokens) and relay the response back to client, then loop +7. If denied in enforce mode: send 403 (using redacted target in the response body) and close the connection ## Process Identity @@ -1317,6 +1432,10 @@ The sandbox uses `miette` for error reporting and `thiserror` for typed errors. | Log push gRPC stream breaks | Push loop exits, flushes remaining batch | | Proxy accept error | Log + break accept loop | | Benign connection close (EOF, reset, pipe) | Debug level (not visible to user by default) | +| Credential injection: unresolved placeholder detected | HTTP 500, connection closed (fail-closed) | +| Credential injection: resolved value contains CR/LF/null | Placeholder treated as unresolvable, fail-closed | +| Credential injection: path credential contains traversal/separator | HTTP 500, connection closed (fail-closed) | +| Credential injection: percent-encoded placeholder bypass attempt | HTTP 500, connection closed (fail-closed) | | L7 parse error | Close the connection | | SSH server failure | Async task error logged, main process unaffected | | Process timeout | Kill process, return exit code 124 | diff --git a/crates/openshell-sandbox/Cargo.toml b/crates/openshell-sandbox/Cargo.toml index 8a0639a7d..26da57efd 100644 --- a/crates/openshell-sandbox/Cargo.toml +++ b/crates/openshell-sandbox/Cargo.toml @@ -52,6 +52,9 @@ webpki-roots = { workspace = true } # HTTP bytes = { workspace = true } +# Encoding +base64 = { workspace = true } + # IP network / CIDR parsing ipnet = "2" diff --git a/crates/openshell-sandbox/src/l7/relay.rs b/crates/openshell-sandbox/src/l7/relay.rs index e0ad2a18c..49caea64d 100644 --- a/crates/openshell-sandbox/src/l7/relay.rs +++ b/crates/openshell-sandbox/src/l7/relay.rs @@ -9,10 +9,10 @@ use crate::l7::provider::L7Provider; use crate::l7::{EnforcementMode, L7EndpointConfig, L7Protocol, L7RequestInfo}; -use crate::secrets::SecretResolver; +use crate::secrets::{self, SecretResolver}; use miette::{IntoDiagnostic, Result, miette}; use std::sync::{Arc, Mutex}; -use tokio::io::{AsyncRead, AsyncWrite}; +use tokio::io::{AsyncRead, AsyncWrite, AsyncWriteExt}; use tracing::{debug, info, warn}; /// Context for L7 request policy evaluation. @@ -105,13 +105,36 @@ where } }; + // Rewrite credential placeholders in the request target BEFORE OPA + // evaluation. OPA sees the redacted path; the resolved path goes only + // to the upstream write. + let (eval_target, redacted_target) = if let Some(ref resolver) = ctx.secret_resolver { + match secrets::rewrite_target_for_eval(&req.target, resolver) { + Ok(result) => (result.resolved, result.redacted), + Err(e) => { + warn!( + host = %ctx.host, + port = ctx.port, + error = %e, + "credential resolution failed in request target, rejecting" + ); + let response = b"HTTP/1.1 500 Internal Server Error\r\nContent-Length: 0\r\nConnection: close\r\n\r\n"; + client.write_all(response).await.into_diagnostic()?; + client.flush().await.into_diagnostic()?; + return Ok(()); + } + } + } else { + (req.target.clone(), req.target.clone()) + }; + let request_info = L7RequestInfo { action: req.action.clone(), - target: req.target.clone(), + target: redacted_target.clone(), query_params: req.query_params.clone(), }; - // Evaluate L7 policy via Rego + // Evaluate L7 policy via Rego (using redacted target) let (allowed, reason) = evaluate_l7_request(engine, ctx, &request_info)?; let decision_str = match (allowed, config.enforcement) { @@ -120,20 +143,23 @@ where (false, EnforcementMode::Enforce) => "deny", }; - // Log every L7 decision + // Log every L7 decision (using redacted target — never log real secrets) info!( dst_host = %ctx.host, dst_port = ctx.port, policy = %ctx.policy_name, l7_protocol = "rest", l7_action = %request_info.action, - l7_target = %request_info.target, + l7_target = %redacted_target, l7_query_params = ?request_info.query_params, l7_decision = decision_str, l7_deny_reason = %reason, "L7_REQUEST", ); + // Store the resolved target for the deny response redaction + let _ = &eval_target; + if allowed || config.enforcement == EnforcementMode::Audit { // Forward request to upstream and relay response let reusable = crate::l7::rest::relay_http_request_with_resolver( @@ -152,9 +178,15 @@ where return Ok(()); } } else { - // Enforce mode: deny with 403 and close connection + // Enforce mode: deny with 403 and close connection (use redacted target) crate::l7::rest::RestProvider - .deny(&req, &ctx.policy_name, &reason, client) + .deny_with_redacted_target( + &req, + &ctx.policy_name, + &reason, + client, + Some(&redacted_target), + ) .await?; return Ok(()); } @@ -266,13 +298,34 @@ where request_count += 1; - // Log for observability. + // Resolve and redact the target for logging. + let redacted_target = if let Some(ref res) = ctx.secret_resolver { + match secrets::rewrite_target_for_eval(&req.target, res) { + Ok(result) => result.redacted, + Err(e) => { + warn!( + host = %ctx.host, + port = ctx.port, + error = %e, + "credential resolution failed in request target, rejecting" + ); + let response = b"HTTP/1.1 500 Internal Server Error\r\nContent-Length: 0\r\nConnection: close\r\n\r\n"; + client.write_all(response).await.into_diagnostic()?; + client.flush().await.into_diagnostic()?; + return Ok(()); + } + } + } else { + req.target.clone() + }; + + // Log for observability (using redacted target — never log real secrets). let has_creds = resolver.is_some(); info!( host = %ctx.host, port = ctx.port, method = %req.action, - path = %req.target, + path = %redacted_target, credentials_injected = has_creds, request_num = request_count, "HTTP_REQUEST", diff --git a/crates/openshell-sandbox/src/l7/rest.rs b/crates/openshell-sandbox/src/l7/rest.rs index da453ce16..6ac87901e 100644 --- a/crates/openshell-sandbox/src/l7/rest.rs +++ b/crates/openshell-sandbox/src/l7/rest.rs @@ -47,7 +47,21 @@ impl L7Provider for RestProvider { reason: &str, client: &mut C, ) -> Result<()> { - send_deny_response(req, policy_name, reason, client).await + send_deny_response(req, policy_name, reason, client, None).await + } +} + +impl RestProvider { + /// Deny with a redacted target for the response body. + pub(crate) async fn deny_with_redacted_target( + &self, + req: &L7Request, + policy_name: &str, + reason: &str, + client: &mut C, + redacted_target: Option<&str>, + ) -> Result<()> { + send_deny_response(req, policy_name, reason, client, redacted_target).await } } @@ -247,10 +261,11 @@ where .position(|w| w == b"\r\n\r\n") .map_or(req.raw_header.len(), |p| p + 4); - let rewritten_header = rewrite_http_header_block(&req.raw_header[..header_end], resolver); + let rewrite_result = rewrite_http_header_block(&req.raw_header[..header_end], resolver) + .map_err(|e| miette!("credential injection failed: {e}"))?; upstream - .write_all(&rewritten_header) + .write_all(&rewrite_result.rewritten) .await .into_diagnostic()?; @@ -278,16 +293,21 @@ where } /// Send a 403 Forbidden JSON deny response. +/// +/// When `redacted_target` is provided, it is used instead of `req.target` +/// in the response body to avoid leaking resolved credential values. async fn send_deny_response( req: &L7Request, policy_name: &str, reason: &str, client: &mut C, + redacted_target: Option<&str>, ) -> Result<()> { + let target = redacted_target.unwrap_or(&req.target); let body = serde_json::json!({ "error": "policy_denied", "policy": policy_name, - "rule": format!("{} {}", req.action, req.target), + "rule": format!("{} {}", req.action, target), "detail": reason }); let body_bytes = body.to_string(); @@ -1371,8 +1391,8 @@ mod tests { ); let raw = b"GET /v1/messages HTTP/1.1\r\nAuthorization: Bearer openshell:resolve:env:ANTHROPIC_API_KEY\r\nHost: example.com\r\n\r\n"; - let rewritten = rewrite_http_header_block(raw, resolver.as_ref()); - let rewritten = String::from_utf8(rewritten).expect("utf8"); + let result = rewrite_http_header_block(raw, resolver.as_ref()).expect("should succeed"); + let rewritten = String::from_utf8(result.rewritten).expect("utf8"); assert!(rewritten.contains("Authorization: Bearer sk-test\r\n")); assert!(!rewritten.contains("openshell:resolve:env:ANTHROPIC_API_KEY")); diff --git a/crates/openshell-sandbox/src/proxy.rs b/crates/openshell-sandbox/src/proxy.rs index 49fe5e07b..ce980a45c 100644 --- a/crates/openshell-sandbox/src/proxy.rs +++ b/crates/openshell-sandbox/src/proxy.rs @@ -1617,7 +1617,7 @@ fn rewrite_forward_request( used: usize, path: &str, secret_resolver: Option<&SecretResolver>, -) -> Vec { +) -> Result, crate::secrets::UnresolvedPlaceholderError> { let header_end = raw[..used] .windows(4) .position(|w| w == b"\r\n\r\n") @@ -1698,7 +1698,15 @@ fn rewrite_forward_request( output.extend_from_slice(&raw[header_end..used]); } - output + // Fail-closed: scan for any remaining unresolved placeholders + if secret_resolver.is_some() { + let output_str = String::from_utf8_lossy(&output); + if output_str.contains(crate::secrets::PLACEHOLDER_PREFIX_PUBLIC) { + return Err(crate::secrets::UnresolvedPlaceholderError { location: "header" }); + } + } + + Ok(output) } /// Handle a plain HTTP forward proxy request (non-CONNECT). @@ -2040,7 +2048,19 @@ async fn handle_forward_proxy( ); // 9. Rewrite request and forward to upstream - let rewritten = rewrite_forward_request(buf, used, &path, secret_resolver.as_deref()); + let rewritten = match rewrite_forward_request(buf, used, &path, secret_resolver.as_deref()) { + Ok(bytes) => bytes, + Err(e) => { + warn!( + dst_host = %host_lc, + dst_port = port, + error = %e, + "credential injection failed in forward proxy" + ); + respond(client, b"HTTP/1.1 500 Internal Server Error\r\n\r\n").await?; + return Ok(()); + } + }; upstream.write_all(&rewritten).await.into_diagnostic()?; // 8. Relay remaining traffic bidirectionally (supports streaming) @@ -2740,7 +2760,7 @@ mod tests { fn test_rewrite_get_request() { let raw = b"GET http://10.0.0.1:8000/api HTTP/1.1\r\nHost: 10.0.0.1:8000\r\nAccept: */*\r\n\r\n"; - let result = rewrite_forward_request(raw, raw.len(), "/api", None); + let result = rewrite_forward_request(raw, raw.len(), "/api", None).expect("should succeed"); let result_str = String::from_utf8_lossy(&result); assert!(result_str.starts_with("GET /api HTTP/1.1\r\n")); assert!(result_str.contains("Host: 10.0.0.1:8000")); @@ -2751,7 +2771,7 @@ mod tests { #[test] fn test_rewrite_strips_proxy_headers() { let raw = b"GET http://host/p HTTP/1.1\r\nHost: host\r\nProxy-Authorization: Basic abc\r\nProxy-Connection: keep-alive\r\nAccept: */*\r\n\r\n"; - let result = rewrite_forward_request(raw, raw.len(), "/p", None); + let result = rewrite_forward_request(raw, raw.len(), "/p", None).expect("should succeed"); let result_str = String::from_utf8_lossy(&result); assert!( !result_str @@ -2765,7 +2785,7 @@ mod tests { #[test] fn test_rewrite_replaces_connection_header() { let raw = b"GET http://host/p HTTP/1.1\r\nHost: host\r\nConnection: keep-alive\r\n\r\n"; - let result = rewrite_forward_request(raw, raw.len(), "/p", None); + let result = rewrite_forward_request(raw, raw.len(), "/p", None).expect("should succeed"); let result_str = String::from_utf8_lossy(&result); assert!(result_str.contains("Connection: close")); assert!(!result_str.contains("keep-alive")); @@ -2774,7 +2794,7 @@ mod tests { #[test] fn test_rewrite_preserves_body_overflow() { let raw = b"POST http://host/api HTTP/1.1\r\nHost: host\r\nContent-Length: 13\r\n\r\n{\"key\":\"val\"}"; - let result = rewrite_forward_request(raw, raw.len(), "/api", None); + let result = rewrite_forward_request(raw, raw.len(), "/api", None).expect("should succeed"); let result_str = String::from_utf8_lossy(&result); assert!(result_str.contains("{\"key\":\"val\"}")); assert!(result_str.contains("POST /api HTTP/1.1")); @@ -2783,7 +2803,7 @@ mod tests { #[test] fn test_rewrite_preserves_existing_via() { let raw = b"GET http://host/p HTTP/1.1\r\nHost: host\r\nVia: 1.0 upstream\r\n\r\n"; - let result = rewrite_forward_request(raw, raw.len(), "/p", None); + let result = rewrite_forward_request(raw, raw.len(), "/p", None).expect("should succeed"); let result_str = String::from_utf8_lossy(&result); assert!(result_str.contains("Via: 1.0 upstream")); // Should not add a second Via header @@ -2798,7 +2818,8 @@ mod tests { .collect(), ); let raw = b"GET http://host/p HTTP/1.1\r\nHost: host\r\nAuthorization: Bearer openshell:resolve:env:ANTHROPIC_API_KEY\r\n\r\n"; - let result = rewrite_forward_request(raw, raw.len(), "/p", resolver.as_ref()); + let result = rewrite_forward_request(raw, raw.len(), "/p", resolver.as_ref()) + .expect("should succeed"); let result_str = String::from_utf8_lossy(&result); assert!(result_str.contains("Authorization: Bearer sk-test")); assert!(!result_str.contains("openshell:resolve:env:ANTHROPIC_API_KEY")); diff --git a/crates/openshell-sandbox/src/secrets.rs b/crates/openshell-sandbox/src/secrets.rs index 4ee1ee846..88b84831e 100644 --- a/crates/openshell-sandbox/src/secrets.rs +++ b/crates/openshell-sandbox/src/secrets.rs @@ -1,10 +1,66 @@ // SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 +use base64::Engine as _; use std::collections::HashMap; +use std::fmt; const PLACEHOLDER_PREFIX: &str = "openshell:resolve:env:"; +/// Public access to the 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 +/// placeholder boundaries within concatenated strings like path segments). +fn is_env_key_char(b: u8) -> bool { + b.is_ascii_alphanumeric() || b == b'_' +} + +// --------------------------------------------------------------------------- +// Error and result types +// --------------------------------------------------------------------------- + +/// Error returned when a placeholder cannot be resolved or a resolved secret +/// contains prohibited characters. +#[derive(Debug)] +pub(crate) struct UnresolvedPlaceholderError { + pub location: &'static str, // "header", "query_param", "path" +} + +impl fmt::Display for UnresolvedPlaceholderError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!( + f, + "unresolved credential placeholder in {}: detected openshell:resolve:env:* token that could not be resolved", + self.location + ) + } +} + +/// Result of rewriting an HTTP header block with credential resolution. +#[derive(Debug)] +pub(crate) struct RewriteResult { + /// The rewritten HTTP bytes (headers + body overflow). + pub rewritten: Vec, + /// A redacted version of the request target for logging. + /// Contains `[CREDENTIAL]` in place of resolved credential values. + /// `None` if the target was not modified. + pub redacted_target: Option, +} + +/// Result of rewriting a request target for OPA evaluation. +#[derive(Debug)] +pub(crate) struct RewriteTargetResult { + /// The resolved target (real secrets) — for upstream forwarding only. + pub resolved: String, + /// The redacted target (`[CREDENTIAL]` in place of secrets) — for OPA + logs. + pub redacted: String, +} + +// --------------------------------------------------------------------------- +// SecretResolver +// --------------------------------------------------------------------------- + #[derive(Debug, Clone, Default)] pub(crate) struct SecretResolver { by_placeholder: HashMap, @@ -30,45 +86,513 @@ impl SecretResolver { (child_env, Some(Self { by_placeholder })) } + /// Resolve a placeholder string to the real secret value. + /// + /// Returns `None` if the placeholder is unknown or the resolved value + /// contains prohibited control characters (CRLF, null byte). pub(crate) fn resolve_placeholder(&self, value: &str) -> Option<&str> { - self.by_placeholder.get(value).map(String::as_str) + let secret = self.by_placeholder.get(value).map(String::as_str)?; + match validate_resolved_secret(secret) { + Ok(s) => Some(s), + Err(reason) => { + tracing::warn!( + location = "resolve_placeholder", + reason, + "credential resolution rejected: resolved value contains prohibited characters" + ); + None + } + } } pub(crate) fn rewrite_header_value(&self, value: &str) -> Option { + // Direct placeholder match: `x-api-key: openshell:resolve:env:KEY` if let Some(secret) = self.resolve_placeholder(value.trim()) { return Some(secret.to_string()); } let trimmed = value.trim(); + + // Basic auth decoding: `Basic ` where the decoded content + // contains a placeholder (e.g. `user:openshell:resolve:env:PASS`). + if let Some(encoded) = trimmed + .strip_prefix("Basic ") + .or_else(|| trimmed.strip_prefix("basic ")) + .map(str::trim) + { + if let Some(rewritten) = self.rewrite_basic_auth_token(encoded) { + return Some(format!("Basic {rewritten}")); + } + } + + // Prefixed placeholder: `Bearer openshell:resolve:env:KEY` let split_at = trimmed.find(char::is_whitespace)?; let prefix = &trimmed[..split_at]; let candidate = trimmed[split_at..].trim(); let secret = self.resolve_placeholder(candidate)?; Some(format!("{prefix} {secret}")) } + + /// Decode a Base64-encoded Basic auth token, resolve any placeholders in + /// the decoded `username:password` string, and re-encode. + /// + /// Returns `None` if decoding fails or no placeholders are found. + fn rewrite_basic_auth_token(&self, encoded: &str) -> Option { + let b64 = base64::engine::general_purpose::STANDARD; + 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) { + return None; + } + + // Rewrite all placeholder occurrences in the decoded string + let mut rewritten = decoded.to_string(); + for (placeholder, secret) in &self.by_placeholder { + if rewritten.contains(placeholder.as_str()) { + // Validate the resolved secret for control characters + if validate_resolved_secret(secret).is_err() { + tracing::warn!( + location = "basic_auth", + "credential resolution rejected: resolved value contains prohibited characters" + ); + return None; + } + rewritten = rewritten.replace(placeholder.as_str(), secret); + } + } + + // Only return if we actually changed something + if rewritten == decoded { + return None; + } + + Some(b64.encode(rewritten.as_bytes())) + } } pub(crate) fn placeholder_for_env_key(key: &str) -> String { format!("{PLACEHOLDER_PREFIX}{key}") } -pub(crate) fn rewrite_http_header_block(raw: &[u8], resolver: Option<&SecretResolver>) -> Vec { +// --------------------------------------------------------------------------- +// Secret validation (F1 — CWE-113) +// --------------------------------------------------------------------------- + +/// Validate that a resolved secret value does not contain characters that +/// could enable HTTP header injection or request splitting. +fn validate_resolved_secret(value: &str) -> Result<&str, &'static str> { + if value + .bytes() + .any(|b| b == b'\r' || b == b'\n' || b == b'\0') + { + return Err("resolved secret contains prohibited control characters (CR, LF, or NUL)"); + } + Ok(value) +} + +// --------------------------------------------------------------------------- +// Percent encoding/decoding (RFC 3986) +// --------------------------------------------------------------------------- + +/// Percent-encode a string for safe use in URL query parameter values. +/// +/// Encodes all characters except unreserved characters (RFC 3986 Section 2.3): +/// ALPHA / DIGIT / "-" / "." / "_" / "~" +fn percent_encode_query(input: &str) -> String { + let mut encoded = String::with_capacity(input.len()); + for byte in input.bytes() { + match byte { + b'A'..=b'Z' | b'a'..=b'z' | b'0'..=b'9' | b'-' | b'.' | b'_' | b'~' => { + encoded.push(byte as char); + } + _ => { + use fmt::Write; + let _ = write!(encoded, "%{byte:02X}"); + } + } + } + encoded +} + +/// Percent-encode a string for safe use in URL path segments. +/// +/// RFC 3986 §3.3: pchar = unreserved / pct-encoded / sub-delims / ":" / "@" +/// sub-delims = "!" / "$" / "&" / "'" / "(" / ")" / "*" / "+" / "," / ";" / "=" +/// +/// Must encode: `/`, `?`, `#`, space, and other non-pchar characters. +fn percent_encode_path_segment(input: &str) -> String { + let mut encoded = String::with_capacity(input.len()); + for byte in input.bytes() { + match byte { + // unreserved + b'A'..=b'Z' | b'a'..=b'z' | b'0'..=b'9' | b'-' | b'.' | b'_' | b'~' => { + encoded.push(byte as char); + } + // sub-delims + ":" + "@" + b'!' | b'$' | b'&' | b'\'' | b'(' | b')' | b'*' | b'+' | b',' | b';' | b'=' | b':' + | b'@' => { + encoded.push(byte as char); + } + _ => { + use fmt::Write; + let _ = write!(encoded, "%{byte:02X}"); + } + } + } + encoded +} + +/// Percent-decode a URL-encoded string. +fn percent_decode(input: &str) -> String { + let mut decoded = Vec::with_capacity(input.len()); + let mut bytes = input.bytes(); + while let Some(b) = bytes.next() { + if b == b'%' { + let hi = bytes.next(); + let lo = bytes.next(); + if let (Some(h), Some(l)) = (hi, lo) { + let hex = [h, l]; + if let Ok(s) = std::str::from_utf8(&hex) { + if let Ok(val) = u8::from_str_radix(s, 16) { + decoded.push(val); + continue; + } + } + // Invalid percent encoding — preserve verbatim + decoded.push(b'%'); + decoded.push(h); + decoded.push(l); + } else { + decoded.push(b'%'); + if let Some(h) = hi { + decoded.push(h); + } + } + } else { + decoded.push(b); + } + } + String::from_utf8_lossy(&decoded).into_owned() +} + +// --------------------------------------------------------------------------- +// Path credential validation (F3 — CWE-22) +// --------------------------------------------------------------------------- + +/// Validate that a resolved credential value is safe for use in a URL path segment. +/// +/// Operates on the raw (decoded) credential value before percent-encoding. +/// Rejects values that could enable path traversal, request splitting, or +/// URI structure breakage. +fn validate_credential_for_path(value: &str) -> Result<(), String> { + if value.contains("../") || value.contains("..\\") || value == ".." { + return Err("credential contains path traversal sequence".into()); + } + if value.contains('\0') || value.contains('\r') || value.contains('\n') { + return Err("credential contains control character".into()); + } + if value.contains('/') || value.contains('\\') { + return Err("credential contains path separator".into()); + } + if value.contains('?') || value.contains('#') { + return Err("credential contains URI delimiter".into()); + } + Ok(()) +} + +// --------------------------------------------------------------------------- +// URI rewriting +// --------------------------------------------------------------------------- + +/// Result of rewriting the request line. +struct RewriteLineResult { + /// The rewritten request line. + line: String, + /// Redacted target for logging (if any rewriting occurred). + redacted_target: Option, +} + +/// Rewrite credential placeholders in the request line's URL. +/// +/// Given a request line like `GET /bot{TOKEN}/path?key={APIKEY} HTTP/1.1`, +/// resolves placeholders in both path segments and query parameter values. +fn rewrite_request_line( + line: &str, + resolver: &SecretResolver, +) -> Result { + // Request line format: METHOD SP REQUEST-URI SP HTTP-VERSION + let mut parts = line.splitn(3, ' '); + let method = match parts.next() { + Some(m) => m, + None => { + return Ok(RewriteLineResult { + line: line.to_string(), + redacted_target: None, + }); + } + }; + let uri = match parts.next() { + Some(u) => u, + None => { + return Ok(RewriteLineResult { + line: line.to_string(), + redacted_target: None, + }); + } + }; + let version = match parts.next() { + Some(v) => v, + None => { + return Ok(RewriteLineResult { + line: line.to_string(), + redacted_target: None, + }); + } + }; + + // Only rewrite if the URI contains a placeholder + if !uri.contains(PLACEHOLDER_PREFIX) { + return Ok(RewriteLineResult { + line: line.to_string(), + redacted_target: None, + }); + } + + // Split URI into path and query + let (path, query) = match uri.split_once('?') { + Some((p, q)) => (p, Some(q)), + None => (uri, None), + }; + + // Rewrite path segments + let (resolved_path, redacted_path) = match rewrite_uri_path(path, resolver)? { + Some((resolved, redacted)) => (resolved, redacted), + None => (path.to_string(), path.to_string()), + }; + + // Rewrite query params + let (resolved_query, redacted_query) = match query { + Some(q) => match rewrite_uri_query_params(q, resolver)? { + Some((resolved, redacted)) => (Some(resolved), Some(redacted)), + None => (Some(q.to_string()), Some(q.to_string())), + }, + None => (None, None), + }; + + // Reassemble + let resolved_uri = match &resolved_query { + Some(q) => format!("{resolved_path}?{q}"), + None => resolved_path.clone(), + }; + let redacted_uri = match &redacted_query { + Some(q) => format!("{redacted_path}?{q}"), + None => redacted_path, + }; + + Ok(RewriteLineResult { + line: format!("{method} {resolved_uri} {version}"), + redacted_target: Some(redacted_uri), + }) +} + +/// Rewrite placeholders in URL path segments. +/// +/// Handles substring matching for cases like Telegram's `/bot{TOKEN}/method` +/// where the placeholder is concatenated with literal text in a segment. +/// +/// Returns `Some((resolved_path, redacted_path))` if any placeholders were found, +/// `None` if no placeholders exist in the path. +fn rewrite_uri_path( + path: &str, + resolver: &SecretResolver, +) -> Result, UnresolvedPlaceholderError> { + if !path.contains(PLACEHOLDER_PREFIX) { + return Ok(None); + } + + let segments: Vec<&str> = path.split('/').collect(); + let mut resolved_segments = Vec::with_capacity(segments.len()); + let mut redacted_segments = Vec::with_capacity(segments.len()); + let mut any_rewritten = false; + + for segment in &segments { + let decoded = percent_decode(segment); + if !decoded.contains(PLACEHOLDER_PREFIX) { + resolved_segments.push(segment.to_string()); + redacted_segments.push(segment.to_string()); + continue; + } + + let (resolved, redacted) = rewrite_path_segment(&decoded, resolver)?; + // Percent-encode the resolved segment for path context + resolved_segments.push(percent_encode_path_segment(&resolved)); + redacted_segments.push(redacted); + any_rewritten = true; + } + + if !any_rewritten { + return Ok(None); + } + + Ok(Some(( + resolved_segments.join("/"), + redacted_segments.join("/"), + ))) +} + +/// Rewrite placeholders within a single path segment (already percent-decoded). +/// +/// Uses the placeholder grammar `openshell:resolve:env:[A-Za-z_][A-Za-z0-9_]*` +/// to determine placeholder boundaries within concatenated text. +fn rewrite_path_segment( + segment: &str, + resolver: &SecretResolver, +) -> Result<(String, String), UnresolvedPlaceholderError> { + let mut resolved = String::with_capacity(segment.len()); + let mut redacted = String::with_capacity(segment.len()); + let mut pos = 0; + let bytes = segment.as_bytes(); + + while pos < bytes.len() { + if let Some(start) = segment[pos..].find(PLACEHOLDER_PREFIX) { + let abs_start = pos + start; + // Copy literal prefix before the placeholder + resolved.push_str(&segment[pos..abs_start]); + redacted.push_str(&segment[pos..abs_start]); + + // Extract the key name using the env var grammar: [A-Za-z_][A-Za-z0-9_]* + let key_start = abs_start + PLACEHOLDER_PREFIX.len(); + let key_end = segment[key_start..] + .bytes() + .position(|b| !is_env_key_char(b)) + .map_or(segment.len(), |p| key_start + p); + + if key_end == key_start { + // Empty key — not a valid placeholder, copy literally + resolved.push_str(&segment[abs_start..abs_start + PLACEHOLDER_PREFIX.len()]); + redacted.push_str(&segment[abs_start..abs_start + PLACEHOLDER_PREFIX.len()]); + pos = abs_start + PLACEHOLDER_PREFIX.len(); + continue; + } + + let full_placeholder = &segment[abs_start..key_end]; + if let Some(secret) = resolver.resolve_placeholder(full_placeholder) { + validate_credential_for_path(secret).map_err(|reason| { + tracing::warn!( + location = "path", + %reason, + "credential resolution rejected: resolved value unsafe for path" + ); + UnresolvedPlaceholderError { location: "path" } + })?; + resolved.push_str(secret); + redacted.push_str("[CREDENTIAL]"); + } else { + return Err(UnresolvedPlaceholderError { location: "path" }); + } + pos = key_end; + } else { + // No more placeholders in remainder + resolved.push_str(&segment[pos..]); + redacted.push_str(&segment[pos..]); + break; + } + } + + Ok((resolved, redacted)) +} + +/// Rewrite placeholders in query parameter values. +/// +/// Returns `Some((resolved_query, redacted_query))` if any placeholders were found. +fn rewrite_uri_query_params( + query: &str, + resolver: &SecretResolver, +) -> Result, UnresolvedPlaceholderError> { + if !query.contains(PLACEHOLDER_PREFIX) { + return Ok(None); + } + + let mut resolved_params = Vec::new(); + let mut redacted_params = Vec::new(); + let mut any_rewritten = false; + + for param in query.split('&') { + if let Some((key, value)) = param.split_once('=') { + let decoded_value = percent_decode(value); + if let Some(secret) = resolver.resolve_placeholder(&decoded_value) { + resolved_params.push(format!("{key}={}", percent_encode_query(secret))); + redacted_params.push(format!("{key}=[CREDENTIAL]")); + any_rewritten = true; + } else if decoded_value.contains(PLACEHOLDER_PREFIX) { + // Placeholder detected but not resolved + return Err(UnresolvedPlaceholderError { + location: "query_param", + }); + } else { + resolved_params.push(param.to_string()); + redacted_params.push(param.to_string()); + } + } else { + resolved_params.push(param.to_string()); + redacted_params.push(param.to_string()); + } + } + + if !any_rewritten { + return Ok(None); + } + + Ok(Some((resolved_params.join("&"), redacted_params.join("&")))) +} + +// --------------------------------------------------------------------------- +// Public rewrite API +// --------------------------------------------------------------------------- + +/// Rewrite credential placeholders in an HTTP header block. +/// +/// Resolves `openshell:resolve:env:*` placeholders in the request line +/// (path segments and query parameter values), header values (including +/// Basic auth tokens), and returns a `RewriteResult` with the rewritten +/// bytes and a redacted target for logging. +/// +/// Returns `Err` if any placeholder is detected but cannot be resolved +/// (fail-closed behavior). +pub(crate) fn rewrite_http_header_block( + raw: &[u8], + resolver: Option<&SecretResolver>, +) -> Result { let Some(resolver) = resolver else { - return raw.to_vec(); + return Ok(RewriteResult { + rewritten: raw.to_vec(), + redacted_target: None, + }); }; let Some(header_end) = raw.windows(4).position(|w| w == b"\r\n\r\n").map(|p| p + 4) else { - return raw.to_vec(); + return Ok(RewriteResult { + rewritten: raw.to_vec(), + redacted_target: None, + }); }; let header_str = String::from_utf8_lossy(&raw[..header_end]); let mut lines = header_str.split("\r\n"); let Some(request_line) = lines.next() else { - return raw.to_vec(); + return Ok(RewriteResult { + rewritten: raw.to_vec(), + redacted_target: None, + }); }; + // Rewrite request line (path + query params) + let rl_result = rewrite_request_line(request_line, resolver)?; + let mut output = Vec::with_capacity(raw.len()); - output.extend_from_slice(request_line.as_bytes()); + output.extend_from_slice(rl_result.line.as_bytes()); output.extend_from_slice(b"\r\n"); for line in lines { @@ -82,7 +606,25 @@ pub(crate) fn rewrite_http_header_block(raw: &[u8], resolver: Option<&SecretReso output.extend_from_slice(b"\r\n"); output.extend_from_slice(&raw[header_end..]); - output + + // Fail-closed scan: check for any remaining unresolved placeholders + // 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) { + 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) { + return Err(UnresolvedPlaceholderError { location: "path" }); + } + + Ok(RewriteResult { + rewritten: output, + redacted_target: rl_result.redacted_target, + }) } pub(crate) fn rewrite_header_line(line: &str, resolver: &SecretResolver) -> String { @@ -96,10 +638,68 @@ pub(crate) fn rewrite_header_line(line: &str, resolver: &SecretResolver) -> Stri } } +/// Resolve placeholders in a request target (path + query) for OPA evaluation. +/// +/// Returns the resolved target (real secrets, for upstream) and a redacted +/// version (`[CREDENTIAL]` in place of secrets, for OPA input and logs). +pub(crate) fn rewrite_target_for_eval( + target: &str, + resolver: &SecretResolver, +) -> Result { + if !target.contains(PLACEHOLDER_PREFIX) { + // Also check percent-decoded form + let decoded = percent_decode(target); + if decoded.contains(PLACEHOLDER_PREFIX) { + return Err(UnresolvedPlaceholderError { location: "path" }); + } + return Ok(RewriteTargetResult { + resolved: target.to_string(), + redacted: target.to_string(), + }); + } + + let (path, query) = match target.split_once('?') { + Some((p, q)) => (p, Some(q)), + None => (target, None), + }; + + // Rewrite path + let (resolved_path, redacted_path) = match rewrite_uri_path(path, resolver)? { + Some((resolved, redacted)) => (resolved, redacted), + None => (path.to_string(), path.to_string()), + }; + + // Rewrite query + let (resolved_query, redacted_query) = match query { + Some(q) => match rewrite_uri_query_params(q, resolver)? { + Some((resolved, redacted)) => (Some(resolved), Some(redacted)), + None => (Some(q.to_string()), Some(q.to_string())), + }, + None => (None, None), + }; + + let resolved = match &resolved_query { + Some(q) => format!("{resolved_path}?{q}"), + None => resolved_path, + }; + let redacted = match &redacted_query { + Some(q) => format!("{redacted_path}?{q}"), + None => redacted_path, + }; + + Ok(RewriteTargetResult { resolved, redacted }) +} + +// --------------------------------------------------------------------------- +// Tests +// --------------------------------------------------------------------------- + #[cfg(test)] mod tests { use super::*; + // === Existing tests (preserved) === + #[test] fn provider_env_is_replaced_with_placeholders() { let (child_env, resolver) = SecretResolver::from_provider_env( @@ -163,17 +763,13 @@ mod tests { ); let raw = b"POST /v1 HTTP/1.1\r\nAuthorization: Bearer openshell:resolve:env:CUSTOM_TOKEN\r\nContent-Length: 5\r\n\r\nhello"; - let rewritten = rewrite_http_header_block(raw, resolver.as_ref()); - let rewritten = String::from_utf8(rewritten).expect("utf8"); + let result = rewrite_http_header_block(raw, resolver.as_ref()).expect("should succeed"); + let rewritten = String::from_utf8(result.rewritten).expect("utf8"); assert!(rewritten.contains("Authorization: Bearer secret-token\r\n")); assert!(rewritten.ends_with("\r\n\r\nhello")); } - /// Simulates the full round-trip: provider env → child placeholders → - /// HTTP headers → rewrite. This is the exact flow that occurs when a - /// sandbox child process reads placeholder env vars, constructs an HTTP - /// request, and the proxy rewrites headers before forwarding upstream. #[test] fn full_round_trip_child_env_to_rewritten_headers() { let provider_env: HashMap = [ @@ -191,13 +787,11 @@ mod tests { let (child_env, resolver) = SecretResolver::from_provider_env(provider_env); - // Child process reads placeholders from the environment let auth_value = child_env.get("ANTHROPIC_API_KEY").unwrap(); let token_value = child_env.get("CUSTOM_SERVICE_TOKEN").unwrap(); assert!(auth_value.starts_with(PLACEHOLDER_PREFIX)); assert!(token_value.starts_with(PLACEHOLDER_PREFIX)); - // Child constructs an HTTP request using those placeholders let raw = format!( "GET /v1/messages HTTP/1.1\r\n\ Host: api.example.com\r\n\ @@ -206,11 +800,10 @@ mod tests { Content-Length: 0\r\n\r\n" ); - // Proxy rewrites headers - let rewritten = rewrite_http_header_block(raw.as_bytes(), resolver.as_ref()); - let rewritten = String::from_utf8(rewritten).expect("utf8"); + let result = + rewrite_http_header_block(raw.as_bytes(), resolver.as_ref()).expect("should succeed"); + let rewritten = String::from_utf8(result.rewritten).expect("utf8"); - // Real secrets must appear in the rewritten headers assert!( rewritten.contains("Authorization: Bearer sk-real-key-12345\r\n"), "Expected rewritten Authorization header, got: {rewritten}" @@ -219,14 +812,10 @@ mod tests { rewritten.contains("x-api-key: tok-real-svc-67890\r\n"), "Expected rewritten x-api-key header, got: {rewritten}" ); - - // Placeholders must not appear assert!( !rewritten.contains("openshell:resolve:env:"), "Placeholder leaked into rewritten request: {rewritten}" ); - - // Request line and non-secret headers must be preserved assert!(rewritten.starts_with("GET /v1/messages HTTP/1.1\r\n")); assert!(rewritten.contains("Host: api.example.com\r\n")); assert!(rewritten.contains("Content-Length: 0\r\n")); @@ -241,9 +830,8 @@ mod tests { ); let raw = b"GET / HTTP/1.1\r\nHost: example.com\r\nAccept: application/json\r\nContent-Type: text/plain\r\n\r\n"; - let rewritten = rewrite_http_header_block(raw, resolver.as_ref()); - // The output should be byte-identical since no placeholders are present - assert_eq!(raw.as_slice(), rewritten.as_slice()); + let result = rewrite_http_header_block(raw, resolver.as_ref()).expect("should succeed"); + assert_eq!(raw.as_slice(), result.rewritten.as_slice()); } #[test] @@ -256,7 +844,633 @@ mod tests { #[test] fn rewrite_with_no_resolver_returns_original() { let raw = b"GET / HTTP/1.1\r\nAuthorization: Bearer my-token\r\n\r\n"; - let rewritten = rewrite_http_header_block(raw, None); - assert_eq!(raw.as_slice(), rewritten.as_slice()); + let result = rewrite_http_header_block(raw, None).expect("should succeed"); + assert_eq!(raw.as_slice(), result.rewritten.as_slice()); + } + + // === Secret validation tests (F1 — CWE-113) === + + #[test] + fn resolve_placeholder_rejects_crlf() { + let (_, resolver) = SecretResolver::from_provider_env( + [("BAD_KEY".to_string(), "value\r\nEvil: header".to_string())] + .into_iter() + .collect(), + ); + let resolver = resolver.expect("resolver"); + assert!( + resolver + .resolve_placeholder("openshell:resolve:env:BAD_KEY") + .is_none() + ); + } + + #[test] + fn resolve_placeholder_rejects_null() { + let (_, resolver) = SecretResolver::from_provider_env( + [("BAD_KEY".to_string(), "value\0rest".to_string())] + .into_iter() + .collect(), + ); + let resolver = resolver.expect("resolver"); + assert!( + resolver + .resolve_placeholder("openshell:resolve:env:BAD_KEY") + .is_none() + ); + } + + #[test] + fn resolve_placeholder_accepts_normal_values() { + let (_, resolver) = SecretResolver::from_provider_env( + [("KEY".to_string(), "sk-abc123_DEF.456~xyz".to_string())] + .into_iter() + .collect(), + ); + let resolver = resolver.expect("resolver"); + assert_eq!( + resolver.resolve_placeholder("openshell:resolve:env:KEY"), + Some("sk-abc123_DEF.456~xyz") + ); + } + + // === Query parameter rewriting tests (absorbed from PR #631) === + + #[test] + fn rewrites_query_param_placeholder_in_request_line() { + let (child_env, resolver) = SecretResolver::from_provider_env( + [("YOUTUBE_API_KEY".to_string(), "AIzaSy-secret".to_string())] + .into_iter() + .collect(), + ); + let placeholder = child_env.get("YOUTUBE_API_KEY").unwrap(); + + let raw = format!( + "GET /youtube/v3/search?part=snippet&key={placeholder} HTTP/1.1\r\n\ + Host: www.googleapis.com\r\n\r\n" + ); + let result = + rewrite_http_header_block(raw.as_bytes(), resolver.as_ref()).expect("should succeed"); + let rewritten = String::from_utf8(result.rewritten).expect("utf8"); + + assert!( + rewritten + .starts_with("GET /youtube/v3/search?part=snippet&key=AIzaSy-secret HTTP/1.1\r\n"), + "Expected query param rewritten, got: {rewritten}" + ); + assert!(!rewritten.contains("openshell:resolve:env:")); + } + + #[test] + fn rewrites_query_param_with_special_chars_percent_encoded() { + let (child_env, resolver) = SecretResolver::from_provider_env( + [( + "API_KEY".to_string(), + "key with spaces&symbols=yes".to_string(), + )] + .into_iter() + .collect(), + ); + let placeholder = child_env.get("API_KEY").unwrap(); + + let raw = format!("GET /api?token={placeholder} HTTP/1.1\r\nHost: x\r\n\r\n"); + let result = + rewrite_http_header_block(raw.as_bytes(), resolver.as_ref()).expect("should succeed"); + let rewritten = String::from_utf8(result.rewritten).expect("utf8"); + + assert!( + rewritten.contains("token=key%20with%20spaces%26symbols%3Dyes"), + "Expected percent-encoded secret, got: {rewritten}" + ); + } + + #[test] + fn rewrites_query_param_only_placeholder_first_param() { + let (child_env, resolver) = SecretResolver::from_provider_env( + [("KEY".to_string(), "secret123".to_string())] + .into_iter() + .collect(), + ); + let placeholder = child_env.get("KEY").unwrap(); + + let raw = format!("GET /api?key={placeholder}&format=json HTTP/1.1\r\nHost: x\r\n\r\n"); + let result = + rewrite_http_header_block(raw.as_bytes(), resolver.as_ref()).expect("should succeed"); + let rewritten = String::from_utf8(result.rewritten).expect("utf8"); + + assert!( + rewritten.starts_with("GET /api?key=secret123&format=json HTTP/1.1"), + "Expected first param rewritten, got: {rewritten}" + ); + } + + #[test] + fn no_query_param_rewrite_without_placeholder() { + let (_, resolver) = SecretResolver::from_provider_env( + [("KEY".to_string(), "secret".to_string())] + .into_iter() + .collect(), + ); + + let raw = b"GET /api?key=normalvalue HTTP/1.1\r\nHost: x\r\n\r\n"; + let result = rewrite_http_header_block(raw, resolver.as_ref()).expect("should succeed"); + assert_eq!(raw.as_slice(), result.rewritten.as_slice()); + } + + // === Basic Authorization header encoding tests (absorbed from PR #631) === + + #[test] + fn rewrites_basic_auth_placeholder_in_decoded_token() { + let b64 = base64::engine::general_purpose::STANDARD; + + let (child_env, resolver) = SecretResolver::from_provider_env( + [("DB_PASSWORD".to_string(), "s3cret!".to_string())] + .into_iter() + .collect(), + ); + let resolver = resolver.expect("resolver"); + let placeholder = child_env.get("DB_PASSWORD").unwrap(); + + let credentials = format!("admin:{placeholder}"); + let encoded = b64.encode(credentials.as_bytes()); + + let header_line = format!("Authorization: Basic {encoded}"); + let rewritten = rewrite_header_line(&header_line, &resolver); + + let rewritten_token = rewritten.strip_prefix("Authorization: Basic ").unwrap(); + let decoded = b64.decode(rewritten_token).unwrap(); + let decoded_str = std::str::from_utf8(&decoded).unwrap(); + + assert_eq!(decoded_str, "admin:s3cret!"); + assert!(!rewritten.contains("openshell:resolve:env:")); + } + + #[test] + fn basic_auth_without_placeholder_unchanged() { + let (_, resolver) = SecretResolver::from_provider_env( + [("KEY".to_string(), "secret".to_string())] + .into_iter() + .collect(), + ); + let resolver = resolver.expect("resolver"); + + let b64 = base64::engine::general_purpose::STANDARD; + let encoded = b64.encode(b"user:password"); + let header_line = format!("Authorization: Basic {encoded}"); + + let rewritten = rewrite_header_line(&header_line, &resolver); + assert_eq!( + rewritten, header_line, + "Should not modify non-placeholder Basic auth" + ); + } + + #[test] + fn basic_auth_full_round_trip_header_block() { + let b64 = base64::engine::general_purpose::STANDARD; + + let (child_env, resolver) = SecretResolver::from_provider_env( + [("REGISTRY_PASS".to_string(), "hunter2".to_string())] + .into_iter() + .collect(), + ); + let placeholder = child_env.get("REGISTRY_PASS").unwrap(); + let credentials = format!("deploy:{placeholder}"); + let encoded = b64.encode(credentials.as_bytes()); + + let raw = format!( + "GET /v2/_catalog HTTP/1.1\r\n\ + Host: registry.example.com\r\n\ + Authorization: Basic {encoded}\r\n\ + Accept: application/json\r\n\r\n" + ); + + let result = + rewrite_http_header_block(raw.as_bytes(), resolver.as_ref()).expect("should succeed"); + let rewritten = String::from_utf8(result.rewritten).expect("utf8"); + + let auth_line = rewritten + .lines() + .find(|l| l.starts_with("Authorization:")) + .unwrap(); + let token = auth_line.strip_prefix("Authorization: Basic ").unwrap(); + let decoded = b64.decode(token).unwrap(); + assert_eq!(std::str::from_utf8(&decoded).unwrap(), "deploy:hunter2"); + + assert!(rewritten.contains("Host: registry.example.com\r\n")); + assert!(rewritten.contains("Accept: application/json\r\n")); + assert!(!rewritten.contains("openshell:resolve:env:")); + } + + // === Percent encoding tests (absorbed from PR #631) === + + #[test] + fn percent_encode_preserves_unreserved() { + assert_eq!(percent_encode_query("abc123-._~"), "abc123-._~"); + } + + #[test] + fn percent_encode_encodes_special_chars() { + assert_eq!(percent_encode_query("a b"), "a%20b"); + assert_eq!(percent_encode_query("key=val&x"), "key%3Dval%26x"); + } + + #[test] + fn percent_decode_round_trips() { + let original = "hello world & more=stuff"; + let encoded = percent_encode_query(original); + let decoded = percent_decode(&encoded); + assert_eq!(decoded, original); + } + + // === URL path rewriting tests === + + #[test] + fn rewrite_path_single_segment_placeholder() { + let (child_env, resolver) = SecretResolver::from_provider_env( + [("TOKEN".to_string(), "abc123".to_string())] + .into_iter() + .collect(), + ); + let resolver = resolver.expect("resolver"); + let placeholder = child_env.get("TOKEN").unwrap(); + + let raw = format!("GET /api/{placeholder}/data HTTP/1.1\r\nHost: x\r\n\r\n"); + let result = + rewrite_http_header_block(raw.as_bytes(), Some(&resolver)).expect("should succeed"); + let rewritten = String::from_utf8(result.rewritten).expect("utf8"); + + assert!( + rewritten.starts_with("GET /api/abc123/data HTTP/1.1"), + "Expected path rewritten, got: {rewritten}" + ); + assert_eq!( + result.redacted_target.as_deref(), + Some("/api/[CREDENTIAL]/data") + ); + } + + #[test] + fn rewrite_path_telegram_style_concatenated() { + let (child_env, resolver) = SecretResolver::from_provider_env( + [( + "TELEGRAM_TOKEN".to_string(), + "123456:ABC-DEF1234ghIkl-zyx57W2v1u123ew11".to_string(), + )] + .into_iter() + .collect(), + ); + let resolver = resolver.expect("resolver"); + let placeholder = child_env.get("TELEGRAM_TOKEN").unwrap(); + + let raw = format!( + "POST /bot{placeholder}/sendMessage HTTP/1.1\r\nHost: api.telegram.org\r\n\r\n" + ); + let result = + rewrite_http_header_block(raw.as_bytes(), Some(&resolver)).expect("should succeed"); + let rewritten = String::from_utf8(result.rewritten).expect("utf8"); + + assert!( + rewritten.starts_with( + "POST /bot123456:ABC-DEF1234ghIkl-zyx57W2v1u123ew11/sendMessage HTTP/1.1" + ), + "Expected Telegram-style path rewritten, got: {rewritten}" + ); + assert_eq!( + result.redacted_target.as_deref(), + Some("/bot[CREDENTIAL]/sendMessage") + ); + } + + #[test] + fn rewrite_path_multiple_placeholders_in_separate_segments() { + let (child_env, resolver) = SecretResolver::from_provider_env( + [ + ("ORG_ID".to_string(), "org-123".to_string()), + ("API_KEY".to_string(), "key-456".to_string()), + ] + .into_iter() + .collect(), + ); + let resolver = resolver.expect("resolver"); + let org_ph = child_env.get("ORG_ID").unwrap(); + let key_ph = child_env.get("API_KEY").unwrap(); + + let raw = format!("GET /orgs/{org_ph}/keys/{key_ph} HTTP/1.1\r\nHost: x\r\n\r\n"); + let result = + rewrite_http_header_block(raw.as_bytes(), Some(&resolver)).expect("should succeed"); + let rewritten = String::from_utf8(result.rewritten).expect("utf8"); + + assert!( + rewritten.starts_with("GET /orgs/org-123/keys/key-456 HTTP/1.1"), + "Expected both path segments rewritten, got: {rewritten}" + ); + } + + #[test] + fn rewrite_path_no_placeholders_unchanged() { + let (_, resolver) = SecretResolver::from_provider_env( + [("KEY".to_string(), "secret".to_string())] + .into_iter() + .collect(), + ); + + let raw = b"GET /v1/chat/completions HTTP/1.1\r\nHost: x\r\n\r\n"; + let result = rewrite_http_header_block(raw, resolver.as_ref()).expect("should succeed"); + assert_eq!(raw.as_slice(), result.rewritten.as_slice()); + assert!(result.redacted_target.is_none()); + } + + #[test] + fn rewrite_path_preserves_query_params() { + let (child_env, resolver) = SecretResolver::from_provider_env( + [("TOKEN".to_string(), "tok123".to_string())] + .into_iter() + .collect(), + ); + let resolver = resolver.expect("resolver"); + let placeholder = child_env.get("TOKEN").unwrap(); + + let raw = format!("GET /bot{placeholder}/method?format=json HTTP/1.1\r\nHost: x\r\n\r\n"); + let result = + rewrite_http_header_block(raw.as_bytes(), Some(&resolver)).expect("should succeed"); + let rewritten = String::from_utf8(result.rewritten).expect("utf8"); + + assert!( + rewritten.starts_with("GET /bottok123/method?format=json HTTP/1.1"), + "Expected path rewritten and query preserved, got: {rewritten}" + ); + } + + #[test] + fn rewrite_path_credential_traversal_rejected() { + let (child_env, resolver) = SecretResolver::from_provider_env( + [("BAD".to_string(), "../admin".to_string())] + .into_iter() + .collect(), + ); + let resolver = resolver.expect("resolver"); + let placeholder = child_env.get("BAD").unwrap(); + + let raw = format!("GET /api/{placeholder}/data HTTP/1.1\r\nHost: x\r\n\r\n"); + let result = rewrite_http_header_block(raw.as_bytes(), Some(&resolver)); + assert!( + result.is_err(), + "Path traversal credential should be rejected" + ); + } + + #[test] + fn rewrite_path_credential_backslash_rejected() { + let (child_env, resolver) = SecretResolver::from_provider_env( + [("BAD".to_string(), "foo\\bar".to_string())] + .into_iter() + .collect(), + ); + let resolver = resolver.expect("resolver"); + let placeholder = child_env.get("BAD").unwrap(); + + let raw = format!("GET /api/{placeholder} HTTP/1.1\r\nHost: x\r\n\r\n"); + let result = rewrite_http_header_block(raw.as_bytes(), Some(&resolver)); + assert!( + result.is_err(), + "Backslash in credential should be rejected" + ); + } + + #[test] + fn rewrite_path_credential_slash_rejected() { + let (child_env, resolver) = SecretResolver::from_provider_env( + [("BAD".to_string(), "foo/bar".to_string())] + .into_iter() + .collect(), + ); + let resolver = resolver.expect("resolver"); + let placeholder = child_env.get("BAD").unwrap(); + + let raw = format!("GET /api/{placeholder} HTTP/1.1\r\nHost: x\r\n\r\n"); + let result = rewrite_http_header_block(raw.as_bytes(), Some(&resolver)); + assert!( + result.is_err(), + "Slash in path credential should be rejected" + ); + } + + #[test] + fn rewrite_path_credential_null_rejected() { + let (child_env, resolver) = SecretResolver::from_provider_env( + [("BAD".to_string(), "foo\0bar".to_string())] + .into_iter() + .collect(), + ); + let resolver = resolver.expect("resolver"); + let placeholder = child_env.get("BAD").unwrap(); + + let raw = format!("GET /api/{placeholder} HTTP/1.1\r\nHost: x\r\n\r\n"); + // The null byte in the credential is caught by resolve_placeholder's + // validate_resolved_secret, which returns None. This triggers the + // unresolved placeholder path in rewrite_path_segment → fail-closed. + let result = rewrite_http_header_block(raw.as_bytes(), Some(&resolver)); + assert!( + result.is_err(), + "Null byte in credential should be rejected" + ); + } + + #[test] + fn rewrite_path_percent_encodes_special_chars() { + let (child_env, resolver) = SecretResolver::from_provider_env( + [("TOKEN".to_string(), "hello world".to_string())] + .into_iter() + .collect(), + ); + let resolver = resolver.expect("resolver"); + let placeholder = child_env.get("TOKEN").unwrap(); + + // Space in the credential should trigger path validation rejection + // since space is safe to encode but the credential also doesn't + // contain path-unsafe chars. Actually, space IS allowed (just encoded). + // Let's test with a safe value that just needs encoding. + let raw = format!("GET /api/{placeholder}/data HTTP/1.1\r\nHost: x\r\n\r\n"); + let result = + rewrite_http_header_block(raw.as_bytes(), Some(&resolver)).expect("should succeed"); + let rewritten = String::from_utf8(result.rewritten).expect("utf8"); + + assert!( + rewritten.contains("/api/hello%20world/data"), + "Expected percent-encoded path segment, got: {rewritten}" + ); + } + + // === Fail-closed tests === + + #[test] + fn unresolved_header_placeholder_returns_error() { + let (_, resolver) = SecretResolver::from_provider_env( + [("KEY".to_string(), "secret".to_string())] + .into_iter() + .collect(), + ); + + let raw = b"GET / HTTP/1.1\r\nx-api-key: openshell:resolve:env:UNKNOWN_KEY\r\n\r\n"; + let result = rewrite_http_header_block(raw, resolver.as_ref()); + assert!(result.is_err(), "Unresolved header placeholder should fail"); + } + + #[test] + fn unresolved_query_param_returns_error() { + let (_, resolver) = SecretResolver::from_provider_env( + [("KEY".to_string(), "secret".to_string())] + .into_iter() + .collect(), + ); + + let raw = b"GET /api?token=openshell:resolve:env:UNKNOWN HTTP/1.1\r\nHost: x\r\n\r\n"; + let result = rewrite_http_header_block(raw, resolver.as_ref()); + assert!( + result.is_err(), + "Unresolved query param placeholder should fail" + ); + } + + #[test] + fn unresolved_path_placeholder_returns_error() { + let (_, resolver) = SecretResolver::from_provider_env( + [("KEY".to_string(), "secret".to_string())] + .into_iter() + .collect(), + ); + + let raw = b"GET /api/openshell:resolve:env:UNKNOWN/data HTTP/1.1\r\nHost: x\r\n\r\n"; + let result = rewrite_http_header_block(raw, resolver.as_ref()); + assert!(result.is_err(), "Unresolved path placeholder should fail"); + } + + #[test] + fn percent_encoded_placeholder_in_path_caught() { + let (_, resolver) = SecretResolver::from_provider_env( + [("KEY".to_string(), "secret".to_string())] + .into_iter() + .collect(), + ); + + // Percent-encode "openshell:resolve:env:UNKNOWN" in the path + let encoded_placeholder = "openshell%3Aresolve%3Aenv%3AUNKNOWN"; + let raw = format!("GET /api/{encoded_placeholder}/data HTTP/1.1\r\nHost: x\r\n\r\n"); + let result = rewrite_http_header_block(raw.as_bytes(), resolver.as_ref()); + assert!( + result.is_err(), + "Percent-encoded placeholder should be caught by fail-closed scan" + ); + } + + #[test] + fn all_resolved_succeeds() { + let (child_env, resolver) = SecretResolver::from_provider_env( + [ + ("KEY1".to_string(), "secret1".to_string()), + ("KEY2".to_string(), "secret2".to_string()), + ] + .into_iter() + .collect(), + ); + let ph1 = child_env.get("KEY1").unwrap(); + let ph2 = child_env.get("KEY2").unwrap(); + + let raw = format!( + "GET /api/{ph1}?token={ph2} HTTP/1.1\r\n\ + x-auth: {ph1}\r\n\r\n" + ); + let result = + rewrite_http_header_block(raw.as_bytes(), resolver.as_ref()).expect("should succeed"); + let rewritten = String::from_utf8(result.rewritten).expect("utf8"); + + assert!(!rewritten.contains("openshell:resolve:env:")); + assert!(rewritten.contains("secret1")); + assert!(rewritten.contains("secret2")); + } + + #[test] + fn no_resolver_passes_through_without_scanning() { + // Even if placeholders are present, None resolver means no scanning + let raw = b"GET /api/openshell:resolve:env:KEY HTTP/1.1\r\nHost: x\r\n\r\n"; + let result = rewrite_http_header_block(raw, None).expect("should succeed"); + assert_eq!(raw.as_slice(), result.rewritten.as_slice()); + } + + // === Redaction tests === + + #[test] + fn redacted_target_replaces_path_secrets_with_credential_marker() { + let (child_env, resolver) = SecretResolver::from_provider_env( + [("TOKEN".to_string(), "real-secret".to_string())] + .into_iter() + .collect(), + ); + let resolver = resolver.expect("resolver"); + let placeholder = child_env.get("TOKEN").unwrap(); + + let result = rewrite_target_for_eval(&format!("/bot{placeholder}/sendMessage"), &resolver) + .expect("should succeed"); + + assert_eq!(result.redacted, "/bot[CREDENTIAL]/sendMessage"); + assert!(result.resolved.contains("real-secret")); + assert!(!result.redacted.contains("real-secret")); + } + + #[test] + fn redacted_target_replaces_query_secrets_with_credential_marker() { + let (child_env, resolver) = SecretResolver::from_provider_env( + [("KEY".to_string(), "secret123".to_string())] + .into_iter() + .collect(), + ); + let resolver = resolver.expect("resolver"); + let placeholder = child_env.get("KEY").unwrap(); + + let result = + rewrite_target_for_eval(&format!("/api?key={placeholder}&format=json"), &resolver) + .expect("should succeed"); + + assert_eq!(result.redacted, "/api?key=[CREDENTIAL]&format=json"); + assert!(result.resolved.contains("secret123")); + assert!(!result.redacted.contains("secret123")); + } + + #[test] + fn redacted_target_preserves_non_secret_segments() { + let (_, resolver) = SecretResolver::from_provider_env( + [("KEY".to_string(), "secret".to_string())] + .into_iter() + .collect(), + ); + let resolver = resolver.expect("resolver"); + + let result = rewrite_target_for_eval("/v1/chat/completions?format=json", &resolver) + .expect("should succeed"); + + assert_eq!(result.resolved, "/v1/chat/completions?format=json"); + assert_eq!(result.redacted, "/v1/chat/completions?format=json"); + } + + #[test] + fn rewrite_target_for_eval_roundtrip() { + let (child_env, resolver) = SecretResolver::from_provider_env( + [ + ("TOKEN".to_string(), "tok123".to_string()), + ("KEY".to_string(), "key456".to_string()), + ] + .into_iter() + .collect(), + ); + let resolver = resolver.expect("resolver"); + let tok_ph = child_env.get("TOKEN").unwrap(); + let key_ph = child_env.get("KEY").unwrap(); + + let target = format!("/bot{tok_ph}/method?key={key_ph}"); + let result = rewrite_target_for_eval(&target, &resolver).expect("should succeed"); + + assert_eq!(result.resolved, "/bottok123/method?key=key456"); + assert_eq!(result.redacted, "/bot[CREDENTIAL]/method?key=[CREDENTIAL]"); } } diff --git a/docs/sandboxes/manage-providers.md b/docs/sandboxes/manage-providers.md index bcab48307..717d54c4a 100644 --- a/docs/sandboxes/manage-providers.md +++ b/docs/sandboxes/manage-providers.md @@ -124,6 +124,47 @@ $ openshell sandbox create -- claude This detects `claude` as a known tool, finds your `ANTHROPIC_API_KEY`, creates a provider, attaches it to the sandbox, and launches Claude Code. +## How Credential Injection Works + +The agent process inside the sandbox never sees real credential values. At startup, the proxy replaces each credential with an opaque placeholder token in the agent's environment. When the agent sends an HTTP request containing a placeholder, the proxy resolves it to the real credential before forwarding upstream. + +This resolution requires the proxy to see plaintext HTTP. Endpoints must use `protocol: rest` in the policy (which auto-terminates TLS) or explicit `tls: terminate`. Endpoints without TLS termination pass traffic through as an opaque stream, and credential placeholders are forwarded unresolved. + +### Supported injection locations + +The proxy resolves credential placeholders in the following parts of an HTTP request: + +| Location | How the agent uses it | Example | +|---|---|---| +| Header value | Agent reads `$API_KEY` from env and places it in a header. | `Authorization: Bearer ` | +| Header value (Basic auth) | Agent base64-encodes `user:` in an `Authorization: Basic` header. The proxy decodes, resolves, and re-encodes. | `Authorization: Basic dXNlcjpvcGVuc2hlbGw6...` | +| Query parameter value | Agent places the placeholder in a URL query parameter. | `GET /api?key=` | +| URL path segment | Agent builds a URL with the placeholder in the path. Supports concatenated patterns. | `POST /bot/sendMessage` | + +The proxy does not modify request bodies, cookies, or response content. + +### Fail-closed behavior + +If the proxy detects a credential placeholder in a request but cannot resolve it, it rejects the request with HTTP 500 instead of forwarding the raw placeholder to the upstream server. This prevents accidental credential leakage in server logs or error responses. + +### Example: Telegram Bot API (path-based credential) + +Create a provider with the Telegram bot token: + +```console +$ openshell provider create --name telegram --type generic --credential TELEGRAM_BOT_TOKEN=123456:ABC-DEF +``` + +The agent reads `TELEGRAM_BOT_TOKEN` from its environment and builds a request like `POST /bot/sendMessage`. The proxy resolves the placeholder in the URL path and forwards `POST /bot123456:ABC-DEF/sendMessage` to the upstream. + +### Example: Google API (query parameter credential) + +```console +$ openshell provider create --name google --type generic --credential YOUTUBE_API_KEY=AIzaSy-secret +``` + +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. + ## Supported Provider Types The following provider types are supported. From 44deb5a96d92ae7afa807c3306da545e2c5884f2 Mon Sep 17 00:00:00 2001 From: John Myers <9696606+johntmyers@users.noreply.github.com> Date: Tue, 31 Mar 2026 12:10:51 -0700 Subject: [PATCH 2/3] fix(docs): remove base64 example that triggers secret scanning alert --- docs/sandboxes/manage-providers.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/sandboxes/manage-providers.md b/docs/sandboxes/manage-providers.md index 717d54c4a..6d35766bf 100644 --- a/docs/sandboxes/manage-providers.md +++ b/docs/sandboxes/manage-providers.md @@ -137,7 +137,7 @@ The proxy resolves credential placeholders in the following parts of an HTTP req | Location | How the agent uses it | Example | |---|---|---| | Header value | Agent reads `$API_KEY` from env and places it in a header. | `Authorization: Bearer ` | -| Header value (Basic auth) | Agent base64-encodes `user:` in an `Authorization: Basic` header. The proxy decodes, resolves, and re-encodes. | `Authorization: Basic dXNlcjpvcGVuc2hlbGw6...` | +| Header value (Basic auth) | Agent base64-encodes `user:` in an `Authorization: Basic` header. The proxy decodes, resolves, and re-encodes. | `Authorization: Basic ` | | Query parameter value | Agent places the placeholder in a URL query parameter. | `GET /api?key=` | | URL path segment | Agent builds a URL with the placeholder in the path. Supports concatenated patterns. | `POST /bot/sendMessage` | From 000ab31a97305ad6c8b068415fbd4d566ba438e4 Mon Sep 17 00:00:00 2001 From: John Myers <9696606+johntmyers@users.noreply.github.com> Date: Tue, 31 Mar 2026 12:41:45 -0700 Subject: [PATCH 3/3] test(sandbox): add relay integration tests for all credential injection methods Add 9 integration tests that exercise credential injection through the full relay_http_request_with_resolver pipeline with simulated upstream: - Bearer header injection - Exact header value injection (x-api-key) - Basic auth decode/resolve/re-encode - Query parameter injection - URL path injection (Telegram-style concatenated) - URL path injection (standalone segment) - Combined path + header injection - Fail-closed: unresolved header placeholder - Fail-closed: unresolved path placeholder --- crates/openshell-sandbox/src/l7/rest.rs | 373 ++++++++++++++++++++++++ 1 file changed, 373 insertions(+) diff --git a/crates/openshell-sandbox/src/l7/rest.rs b/crates/openshell-sandbox/src/l7/rest.rs index 6ac87901e..ec5494c9d 100644 --- a/crates/openshell-sandbox/src/l7/rest.rs +++ b/crates/openshell-sandbox/src/l7/rest.rs @@ -762,6 +762,7 @@ fn is_benign_close(err: &std::io::Error) -> bool { mod tests { use super::*; use crate::secrets::SecretResolver; + use base64::Engine as _; #[test] fn parse_content_length() { @@ -1572,4 +1573,376 @@ mod tests { "Real secret should NOT appear without resolver, got: {forwarded}" ); } + + // ========================================================================= + // Credential injection integration tests + // + // Each test exercises a different injection location through the full + // relay_http_request_with_resolver pipeline: child builds an HTTP request + // with a placeholder, the relay rewrites it, and we verify what upstream + // receives. + // ========================================================================= + + /// Helper: run a request through the relay and capture what upstream receives. + async fn relay_and_capture( + raw_header: Vec, + body_length: BodyLength, + resolver: Option<&SecretResolver>, + ) -> Result { + let (mut proxy_to_upstream, mut upstream_side) = tokio::io::duplex(8192); + let (mut _app_side, mut proxy_to_client) = tokio::io::duplex(8192); + + // Parse the request line to extract action and target for L7Request + let header_str = String::from_utf8_lossy(&raw_header); + let first_line = header_str.lines().next().unwrap_or(""); + let parts: Vec<&str> = first_line.splitn(3, ' ').collect(); + let action = parts.first().unwrap_or(&"GET").to_string(); + let target = parts.get(1).unwrap_or(&"/").to_string(); + + let req = L7Request { + action, + target, + query_params: HashMap::new(), + raw_header, + body_length, + }; + + let content_len = match body_length { + BodyLength::ContentLength(n) => n, + _ => 0, + }; + + let upstream_task = tokio::spawn(async move { + let mut buf = vec![0u8; 8192]; + let mut total = 0; + loop { + let n = upstream_side.read(&mut buf[total..]).await.unwrap(); + if n == 0 { + break; + } + total += n; + if let Some(hdr_end) = buf[..total].windows(4).position(|w| w == b"\r\n\r\n") { + if total >= hdr_end + 4 + content_len as usize { + break; + } + } + } + upstream_side + .write_all(b"HTTP/1.1 200 OK\r\nContent-Length: 2\r\n\r\nok") + .await + .unwrap(); + upstream_side.flush().await.unwrap(); + String::from_utf8_lossy(&buf[..total]).to_string() + }); + + let relay = tokio::time::timeout( + std::time::Duration::from_secs(5), + relay_http_request_with_resolver( + &req, + &mut proxy_to_client, + &mut proxy_to_upstream, + resolver, + ), + ) + .await + .map_err(|_| miette!("relay timed out"))?; + relay?; + + let forwarded = upstream_task + .await + .map_err(|e| miette!("upstream task failed: {e}"))?; + Ok(forwarded) + } + + #[tokio::test] + async fn relay_injects_bearer_header_credential() { + let (child_env, resolver) = SecretResolver::from_provider_env( + [("API_KEY".to_string(), "sk-real-secret-key".to_string())] + .into_iter() + .collect(), + ); + let placeholder = child_env.get("API_KEY").unwrap(); + + let raw = format!( + "POST /v1/chat HTTP/1.1\r\n\ + Host: api.example.com\r\n\ + Authorization: Bearer {placeholder}\r\n\ + Content-Length: 2\r\n\r\n{{}}" + ); + + let forwarded = relay_and_capture( + raw.into_bytes(), + BodyLength::ContentLength(2), + resolver.as_ref(), + ) + .await + .expect("relay should succeed"); + + assert!( + forwarded.contains("Authorization: Bearer sk-real-secret-key\r\n"), + "Upstream should see real Bearer token, got: {forwarded}" + ); + assert!( + !forwarded.contains("openshell:resolve:env:"), + "Placeholder leaked to upstream: {forwarded}" + ); + } + + #[tokio::test] + async fn relay_injects_exact_header_credential() { + let (child_env, resolver) = SecretResolver::from_provider_env( + [("CUSTOM_TOKEN".to_string(), "tok-12345".to_string())] + .into_iter() + .collect(), + ); + let placeholder = child_env.get("CUSTOM_TOKEN").unwrap(); + + let raw = format!( + "GET /api/data HTTP/1.1\r\n\ + Host: api.example.com\r\n\ + x-api-key: {placeholder}\r\n\ + Content-Length: 0\r\n\r\n" + ); + + let forwarded = relay_and_capture( + raw.into_bytes(), + BodyLength::ContentLength(0), + resolver.as_ref(), + ) + .await + .expect("relay should succeed"); + + assert!( + forwarded.contains("x-api-key: tok-12345\r\n"), + "Upstream should see real x-api-key, got: {forwarded}" + ); + assert!(!forwarded.contains("openshell:resolve:env:")); + } + + #[tokio::test] + async fn relay_injects_basic_auth_credential() { + let b64 = base64::engine::general_purpose::STANDARD; + + let (child_env, resolver) = SecretResolver::from_provider_env( + [("REGISTRY_PASS".to_string(), "hunter2".to_string())] + .into_iter() + .collect(), + ); + let placeholder = child_env.get("REGISTRY_PASS").unwrap(); + let encoded = b64.encode(format!("deploy:{placeholder}").as_bytes()); + + let raw = format!( + "GET /v2/_catalog HTTP/1.1\r\n\ + Host: registry.example.com\r\n\ + Authorization: Basic {encoded}\r\n\ + Content-Length: 0\r\n\r\n" + ); + + let forwarded = relay_and_capture( + raw.into_bytes(), + BodyLength::ContentLength(0), + resolver.as_ref(), + ) + .await + .expect("relay should succeed"); + + // Extract and decode the Basic auth token from what upstream received + let auth_line = forwarded + .lines() + .find(|l| l.starts_with("Authorization: Basic")) + .expect("upstream should have Authorization header"); + let token = auth_line + .strip_prefix("Authorization: Basic ") + .unwrap() + .trim(); + let decoded = b64.decode(token).expect("valid base64"); + let decoded_str = std::str::from_utf8(&decoded).expect("valid utf8"); + + assert_eq!( + decoded_str, "deploy:hunter2", + "Decoded Basic auth should contain real password" + ); + assert!(!forwarded.contains("openshell:resolve:env:")); + } + + #[tokio::test] + async fn relay_injects_query_param_credential() { + let (child_env, resolver) = SecretResolver::from_provider_env( + [("YOUTUBE_KEY".to_string(), "AIzaSy-secret".to_string())] + .into_iter() + .collect(), + ); + let placeholder = child_env.get("YOUTUBE_KEY").unwrap(); + + let raw = format!( + "GET /v3/search?part=snippet&key={placeholder} HTTP/1.1\r\n\ + Host: www.googleapis.com\r\n\ + Content-Length: 0\r\n\r\n" + ); + + let forwarded = relay_and_capture( + raw.into_bytes(), + BodyLength::ContentLength(0), + resolver.as_ref(), + ) + .await + .expect("relay should succeed"); + + assert!( + forwarded.contains("key=AIzaSy-secret"), + "Upstream should see real API key in query param, got: {forwarded}" + ); + assert!( + forwarded.contains("part=snippet"), + "Non-secret query params should be preserved, got: {forwarded}" + ); + assert!(!forwarded.contains("openshell:resolve:env:")); + } + + #[tokio::test] + async fn relay_injects_url_path_credential_telegram_style() { + let (child_env, resolver) = SecretResolver::from_provider_env( + [( + "TELEGRAM_TOKEN".to_string(), + "123456:ABC-DEF1234ghIkl".to_string(), + )] + .into_iter() + .collect(), + ); + let placeholder = child_env.get("TELEGRAM_TOKEN").unwrap(); + + let raw = format!( + "POST /bot{placeholder}/sendMessage HTTP/1.1\r\n\ + Host: api.telegram.org\r\n\ + Content-Length: 2\r\n\r\n{{}}" + ); + + let forwarded = relay_and_capture( + raw.into_bytes(), + BodyLength::ContentLength(2), + resolver.as_ref(), + ) + .await + .expect("relay should succeed"); + + assert!( + forwarded.contains("POST /bot123456:ABC-DEF1234ghIkl/sendMessage HTTP/1.1"), + "Upstream should see real token in URL path, got: {forwarded}" + ); + assert!(!forwarded.contains("openshell:resolve:env:")); + } + + #[tokio::test] + async fn relay_injects_url_path_credential_standalone_segment() { + let (child_env, resolver) = SecretResolver::from_provider_env( + [("ORG_TOKEN".to_string(), "org-abc-789".to_string())] + .into_iter() + .collect(), + ); + let placeholder = child_env.get("ORG_TOKEN").unwrap(); + + let raw = format!( + "GET /api/{placeholder}/resources HTTP/1.1\r\n\ + Host: api.example.com\r\n\ + Content-Length: 0\r\n\r\n" + ); + + let forwarded = relay_and_capture( + raw.into_bytes(), + BodyLength::ContentLength(0), + resolver.as_ref(), + ) + .await + .expect("relay should succeed"); + + assert!( + forwarded.contains("GET /api/org-abc-789/resources HTTP/1.1"), + "Upstream should see real token in path segment, got: {forwarded}" + ); + assert!(!forwarded.contains("openshell:resolve:env:")); + } + + #[tokio::test] + async fn relay_injects_combined_path_and_header_credentials() { + let (child_env, resolver) = SecretResolver::from_provider_env( + [ + ("PATH_TOKEN".to_string(), "tok-path-123".to_string()), + ("HEADER_KEY".to_string(), "sk-header-456".to_string()), + ] + .into_iter() + .collect(), + ); + let path_ph = child_env.get("PATH_TOKEN").unwrap(); + let header_ph = child_env.get("HEADER_KEY").unwrap(); + + let raw = format!( + "POST /bot{path_ph}/send HTTP/1.1\r\n\ + Host: api.example.com\r\n\ + x-api-key: {header_ph}\r\n\ + Content-Length: 2\r\n\r\n{{}}" + ); + + let forwarded = relay_and_capture( + raw.into_bytes(), + BodyLength::ContentLength(2), + resolver.as_ref(), + ) + .await + .expect("relay should succeed"); + + assert!( + forwarded.contains("/bottok-path-123/send"), + "Upstream should see real token in path, got: {forwarded}" + ); + assert!( + forwarded.contains("x-api-key: sk-header-456\r\n"), + "Upstream should see real token in header, got: {forwarded}" + ); + assert!(!forwarded.contains("openshell:resolve:env:")); + } + + #[tokio::test] + async fn relay_fail_closed_rejects_unresolved_placeholder() { + // Create a resolver that knows about KEY1 but not UNKNOWN_KEY + let (child_env, resolver) = SecretResolver::from_provider_env( + [("KEY1".to_string(), "secret1".to_string())] + .into_iter() + .collect(), + ); + let _ = child_env; + + // The request references a placeholder that the resolver doesn't know + let raw = b"GET /api HTTP/1.1\r\n\ + Host: example.com\r\n\ + x-api-key: openshell:resolve:env:UNKNOWN_KEY\r\n\ + Content-Length: 0\r\n\r\n" + .to_vec(); + + let result = relay_and_capture(raw, BodyLength::ContentLength(0), resolver.as_ref()).await; + + assert!( + result.is_err(), + "Relay should fail when placeholder cannot be resolved" + ); + } + + #[tokio::test] + async fn relay_fail_closed_rejects_unresolved_path_placeholder() { + let (_, resolver) = SecretResolver::from_provider_env( + [("KEY1".to_string(), "secret1".to_string())] + .into_iter() + .collect(), + ); + + let raw = + b"GET /api/openshell:resolve:env:UNKNOWN_KEY/data HTTP/1.1\r\nHost: x\r\nContent-Length: 0\r\n\r\n" + .to_vec(); + + let result = relay_and_capture(raw, BodyLength::ContentLength(0), resolver.as_ref()).await; + + assert!( + result.is_err(), + "Relay should fail when path placeholder cannot be resolved" + ); + } }