From 0ac1fbd21b50f3aeaf7160223bf6ea5f6061a144 Mon Sep 17 00:00:00 2001 From: LateNightHackathon <256481314+latenighthackathon@users.noreply.github.com> Date: Sun, 29 Mar 2026 18:24:08 -0500 Subject: [PATCH 01/20] fix(l7): reject duplicate Content-Length headers to prevent request smuggling (CWE-444) (#663) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(l7): reject duplicate Content-Length headers to prevent request smuggling Both parse_body_length() in rest.rs and try_parse_http_request() in inference.rs silently accepted multiple Content-Length headers, overwriting with the last value seen. Per RFC 7230 Section 3.3.3, a message with multiple Content-Length headers with differing values must be rejected to prevent HTTP request smuggling (CWE-444). An attacker could send conflicting Content-Length values causing the proxy and downstream server to disagree on message boundaries. Fix: - rest.rs: detect duplicate CL headers with differing values and return an error before forwarding - inference.rs: add ParseResult::Invalid variant; detect duplicate CL headers and return Invalid with a descriptive reason - proxy.rs: handle ParseResult::Invalid by sending HTTP 400 and denying the connection Closes #637 Signed-off-by: latenighthackathon * fix(l7): address review feedback on Content-Length smuggling defense - inference.rs: reject unparseable Content-Length values instead of silently defaulting to 0 via unwrap_or(0) - rest.rs: reject unparseable Content-Length values so a valid+invalid duplicate pair cannot bypass the differing-values check - rest.rs: fix Transfer-Encoding substring match (.contains("chunked") → split/trim exact match) to align with inference.rs and prevent false positives on values like "chunkedx" - proxy.rs: log parsing details server-side via tracing::warn and return generic "Bad Request" body instead of leaking internal parsing reasons to sandboxed code - Add tests for all new rejection paths in inference.rs and rest.rs Signed-off-by: latenighthackathon * style(l7): apply cargo fmt formatting Signed-off-by: latenighthackathon --------- Signed-off-by: latenighthackathon Co-authored-by: latenighthackathon --- crates/openshell-sandbox/src/l7/inference.rs | 58 +++++++++++++++- crates/openshell-sandbox/src/l7/rest.rs | 71 ++++++++++++++++++-- crates/openshell-sandbox/src/proxy.rs | 6 ++ 3 files changed, 129 insertions(+), 6 deletions(-) diff --git a/crates/openshell-sandbox/src/l7/inference.rs b/crates/openshell-sandbox/src/l7/inference.rs index 59dafdaba..140213f07 100644 --- a/crates/openshell-sandbox/src/l7/inference.rs +++ b/crates/openshell-sandbox/src/l7/inference.rs @@ -96,6 +96,8 @@ pub enum ParseResult { Complete(ParsedHttpRequest, usize), /// Headers are incomplete — caller should read more data. Incomplete, + /// The request is malformed and must be rejected (e.g., duplicate Content-Length). + Invalid(String), } /// Try to parse an HTTP/1.1 request from raw bytes. @@ -125,6 +127,7 @@ pub fn try_parse_http_request(buf: &[u8]) -> ParseResult { let mut headers = Vec::new(); let mut content_length: usize = 0; + let mut has_content_length = false; let mut is_chunked = false; for line in lines { if line.is_empty() { @@ -134,7 +137,21 @@ pub fn try_parse_http_request(buf: &[u8]) -> ParseResult { let name = name.trim().to_string(); let value = value.trim().to_string(); if name.eq_ignore_ascii_case("content-length") { - content_length = value.parse().unwrap_or(0); + let new_len: usize = match value.parse() { + Ok(v) => v, + Err(_) => { + return ParseResult::Invalid(format!( + "invalid Content-Length value: {value}" + )); + } + }; + if has_content_length && new_len != content_length { + return ParseResult::Invalid(format!( + "duplicate Content-Length headers with differing values ({content_length} vs {new_len})" + )); + } + content_length = new_len; + has_content_length = true; } if name.eq_ignore_ascii_case("transfer-encoding") && value @@ -552,4 +569,43 @@ mod tests { }; assert_eq!(parsed.body.len(), 100); } + + // ---- SEC: Content-Length validation ---- + + #[test] + fn reject_differing_duplicate_content_length() { + let request = b"POST /v1/chat/completions HTTP/1.1\r\nHost: x\r\nContent-Length: 0\r\nContent-Length: 50\r\n\r\n"; + assert!(matches!( + try_parse_http_request(request), + ParseResult::Invalid(reason) if reason.contains("differing values") + )); + } + + #[test] + fn accept_identical_duplicate_content_length() { + let request = b"POST /v1/chat/completions HTTP/1.1\r\nHost: x\r\nContent-Length: 5\r\nContent-Length: 5\r\n\r\nhello"; + let ParseResult::Complete(parsed, _) = try_parse_http_request(request) else { + panic!("expected Complete for identical duplicate CL"); + }; + assert_eq!(parsed.body, b"hello"); + } + + #[test] + fn reject_non_numeric_content_length() { + let request = + b"POST /v1/chat/completions HTTP/1.1\r\nHost: x\r\nContent-Length: abc\r\n\r\n"; + assert!(matches!( + try_parse_http_request(request), + ParseResult::Invalid(reason) if reason.contains("invalid Content-Length") + )); + } + + #[test] + fn reject_two_non_numeric_content_lengths() { + let request = b"POST /v1/chat/completions HTTP/1.1\r\nHost: x\r\nContent-Length: abc\r\nContent-Length: def\r\n\r\n"; + assert!(matches!( + try_parse_http_request(request), + ParseResult::Invalid(_) + )); + } } diff --git a/crates/openshell-sandbox/src/l7/rest.rs b/crates/openshell-sandbox/src/l7/rest.rs index ebb349578..f47f01bdc 100644 --- a/crates/openshell-sandbox/src/l7/rest.rs +++ b/crates/openshell-sandbox/src/l7/rest.rs @@ -242,14 +242,22 @@ fn parse_body_length(headers: &str) -> Result { let lower = line.to_ascii_lowercase(); if lower.starts_with("transfer-encoding:") { let val = lower.split_once(':').map_or("", |(_, v)| v.trim()); - if val.contains("chunked") { + if val.split(',').any(|enc| enc.trim() == "chunked") { has_te_chunked = true; } } - if lower.starts_with("content-length:") - && let Some(val) = lower.split_once(':').map(|(_, v)| v.trim()) - && let Ok(len) = val.parse::() - { + if lower.starts_with("content-length:") { + let val = lower.split_once(':').map_or("", |(_, v)| v.trim()); + let len: u64 = val + .parse() + .map_err(|_| miette!("Request contains invalid Content-Length value"))?; + if let Some(prev) = cl_value { + if prev != len { + return Err(miette!( + "Request contains multiple Content-Length headers with differing values ({prev} vs {len})" + )); + } + } cl_value = Some(len); } } @@ -702,6 +710,59 @@ mod tests { ); } + /// SEC: Reject differing duplicate Content-Length headers. + #[test] + fn reject_differing_duplicate_content_length() { + let headers = + "POST /api HTTP/1.1\r\nHost: x\r\nContent-Length: 0\r\nContent-Length: 50\r\n\r\n"; + assert!( + parse_body_length(headers).is_err(), + "Must reject differing duplicate Content-Length" + ); + } + + /// SEC: Accept identical duplicate Content-Length headers. + #[test] + fn accept_identical_duplicate_content_length() { + let headers = + "POST /api HTTP/1.1\r\nHost: x\r\nContent-Length: 42\r\nContent-Length: 42\r\n\r\n"; + match parse_body_length(headers).unwrap() { + BodyLength::ContentLength(42) => {} + other => panic!("Expected ContentLength(42), got {other:?}"), + } + } + + /// SEC: Reject non-numeric Content-Length values. + #[test] + fn reject_non_numeric_content_length() { + let headers = "POST /api HTTP/1.1\r\nHost: x\r\nContent-Length: abc\r\n\r\n"; + assert!( + parse_body_length(headers).is_err(), + "Must reject non-numeric Content-Length" + ); + } + + /// SEC: Reject when second Content-Length is non-numeric (bypass test). + #[test] + fn reject_valid_then_invalid_content_length() { + let headers = + "POST /api HTTP/1.1\r\nHost: x\r\nContent-Length: 42\r\nContent-Length: abc\r\n\r\n"; + assert!( + parse_body_length(headers).is_err(), + "Must reject when any Content-Length is non-numeric" + ); + } + + /// SEC: Transfer-Encoding substring match must not match partial tokens. + #[test] + fn te_substring_not_chunked() { + let headers = "POST /api HTTP/1.1\r\nHost: x\r\nTransfer-Encoding: chunkedx\r\n\r\n"; + match parse_body_length(headers).unwrap() { + BodyLength::None => {} + other => panic!("Expected None for non-matching TE, got {other:?}"), + } + } + /// SEC-009: Bare LF in headers enables header injection. #[tokio::test] async fn reject_bare_lf_in_headers() { diff --git a/crates/openshell-sandbox/src/proxy.rs b/crates/openshell-sandbox/src/proxy.rs index 1f38a2cca..6f3eaf9bd 100644 --- a/crates/openshell-sandbox/src/proxy.rs +++ b/crates/openshell-sandbox/src/proxy.rs @@ -943,6 +943,12 @@ async fn handle_inference_interception( buf.resize((buf.len() * 2).min(MAX_INFERENCE_BUF), 0); } } + ParseResult::Invalid(reason) => { + warn!(reason = %reason, "rejecting malformed inference request"); + let response = format_http_response(400, &[], b"Bad Request"); + write_all(&mut tls_client, &response).await?; + return Ok(InferenceOutcome::Denied { reason }); + } } } From 94fbb643b3da25e92afee1bd34c529969b5009b1 Mon Sep 17 00:00:00 2001 From: LateNightHackathon <256481314+latenighthackathon@users.noreply.github.com> Date: Mon, 30 Mar 2026 11:24:38 -0500 Subject: [PATCH 02/20] fix(proxy): add L7 inspection to forward proxy path (#666) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * refactor(l7): export evaluate_l7_request for cross-module use Make evaluate_l7_request() public so the forward proxy path can evaluate individual requests against L7 policy without going through the full relay_with_inspection() loop. Signed-off-by: latenighthackathon * fix(proxy): add L7 inspection to forward proxy path The forward proxy previously rejected all requests to endpoints with L7 rules (blanket 403), forcing clients through the CONNECT tunnel. This meant policies like read-only (allow GET, block POST) had no effect on plain http:// requests through the forward proxy. Replace the blanket rejection with actual L7 evaluation: - Query L7 config for the endpoint (same as before) - Clone the OPA engine and evaluate the request method/path - Allow if L7 policy permits, deny with 403 if enforcement is enforce - Audit mode: log but allow (matching CONNECT path behavior) - Fail-closed: deny on evaluation errors The forward proxy uses Connection: close (one request per connection), so a single evaluation suffices — no relay loop needed. Update e2e tests to validate the new behavior: - GET /allowed → 200 (L7 policy allows) - POST /allowed → 403 (L7 policy denies, enforcement: enforce) Update security-policy.md to reflect the new forward proxy L7 behavior. Closes #643 Signed-off-by: latenighthackathon * style(proxy): apply cargo fmt formatting Signed-off-by: latenighthackathon --------- Signed-off-by: latenighthackathon Co-authored-by: latenighthackathon --- architecture/security-policy.md | 2 +- crates/openshell-sandbox/src/l7/relay.rs | 2 +- crates/openshell-sandbox/src/proxy.rs | 95 ++++++++++++++++++----- e2e/rust/tests/forward_proxy_l7_bypass.rs | 76 ++++++++++++++---- 4 files changed, 141 insertions(+), 34 deletions(-) diff --git a/architecture/security-policy.md b/architecture/security-policy.md index 44898d70b..8c41e5b91 100644 --- a/architecture/security-policy.md +++ b/architecture/security-policy.md @@ -716,7 +716,7 @@ If any condition fails, the proxy returns `403 Forbidden`. 7. Rewrites the request: absolute-form → origin-form (`GET /path HTTP/1.1`), strips hop-by-hop headers, adds `Via: 1.1 openshell-sandbox` and `Connection: close` 8. Forwards the rewritten request, then relays bidirectionally using `tokio::io::copy_bidirectional` (supports chunked transfer, SSE streams, and other long-lived responses with no idle timeout) -**V1 simplifications**: Forward proxy v1 injects `Connection: close` (no keep-alive) and does not perform L7 inspection on the forwarded traffic. Every forward proxy connection handles exactly one request-response exchange. +**V1 simplifications**: Forward proxy v1 injects `Connection: close` (no keep-alive). Every forward proxy connection handles exactly one request-response exchange. When an endpoint has L7 rules configured, the forward proxy evaluates the single request's method and path against L7 policy before forwarding. **Implementation**: See `crates/openshell-sandbox/src/proxy.rs` -- `handle_forward_proxy()`, `parse_proxy_uri()`, `rewrite_forward_request()`. diff --git a/crates/openshell-sandbox/src/l7/relay.rs b/crates/openshell-sandbox/src/l7/relay.rs index 940e7f94c..88f9ce8a6 100644 --- a/crates/openshell-sandbox/src/l7/relay.rs +++ b/crates/openshell-sandbox/src/l7/relay.rs @@ -180,7 +180,7 @@ fn is_benign_connection_error(err: &miette::Report) -> bool { /// Evaluate an L7 request against the OPA engine. /// /// Returns `(allowed, deny_reason)`. -fn evaluate_l7_request( +pub fn evaluate_l7_request( engine: &Mutex, ctx: &L7EvalContext, request: &L7RequestInfo, diff --git a/crates/openshell-sandbox/src/proxy.rs b/crates/openshell-sandbox/src/proxy.rs index 6f3eaf9bd..088ec46a6 100644 --- a/crates/openshell-sandbox/src/proxy.rs +++ b/crates/openshell-sandbox/src/proxy.rs @@ -1803,10 +1803,62 @@ async fn handle_forward_proxy( }; let policy_str = matched_policy.as_deref().unwrap_or("-"); - // 4b. Reject if the endpoint has L7 config — the forward proxy path does - // not perform per-request method/path inspection, so L7-configured - // endpoints must go through the CONNECT tunnel where inspection happens. - if query_l7_config(&opa_engine, &decision, &host_lc, port).is_some() { + // 4b. If the endpoint has L7 config, evaluate the request against + // L7 policy. The forward proxy handles exactly one request per + // connection (Connection: close), so a single evaluation suffices. + if let Some(l7_config) = query_l7_config(&opa_engine, &decision, &host_lc, port) { + let tunnel_engine = opa_engine.clone_engine_for_tunnel().unwrap_or_else(|e| { + warn!( + error = %e, + "Failed to clone OPA engine for forward L7" + ); + regorus::Engine::new() + }); + let engine_mutex = std::sync::Mutex::new(tunnel_engine); + + let l7_ctx = crate::l7::relay::L7EvalContext { + host: host_lc.clone(), + port, + policy_name: matched_policy.clone().unwrap_or_default(), + binary_path: decision + .binary + .as_ref() + .map(|p| p.to_string_lossy().into_owned()) + .unwrap_or_default(), + ancestors: decision + .ancestors + .iter() + .map(|p| p.to_string_lossy().into_owned()) + .collect(), + cmdline_paths: decision + .cmdline_paths + .iter() + .map(|p| p.to_string_lossy().into_owned()) + .collect(), + secret_resolver: secret_resolver.clone(), + }; + + let request_info = crate::l7::L7RequestInfo { + action: method.to_string(), + target: path.clone(), + }; + + let (allowed, reason) = + crate::l7::relay::evaluate_l7_request(&engine_mutex, &l7_ctx, &request_info) + .unwrap_or_else(|e| { + warn!( + error = %e, + "L7 eval failed, denying request" + ); + (false, format!("L7 evaluation error: {e}")) + }); + + let decision_str = match (allowed, l7_config.enforcement) { + (true, _) => "allow", + (false, crate::l7::EnforcementMode::Audit) => "audit", + (false, crate::l7::EnforcementMode::Enforce) => "deny", + }; + info!( dst_host = %host_lc, dst_port = port, @@ -1814,21 +1866,28 @@ async fn handle_forward_proxy( path = %path, binary = %binary_str, policy = %policy_str, - action = "deny", - reason = "endpoint has L7 rules; use CONNECT", - "FORWARD", + l7_protocol = "rest", + l7_decision = decision_str, + l7_deny_reason = %reason, + "FORWARD_L7", ); - emit_denial_simple( - denial_tx, - &host_lc, - port, - &binary_str, - &decision, - "endpoint has L7 rules configured; forward proxy bypasses L7 inspection — use CONNECT", - "forward-l7-bypass", - ); - respond(client, b"HTTP/1.1 403 Forbidden\r\n\r\n").await?; - return Ok(()); + + let effectively_denied = + !allowed && l7_config.enforcement == crate::l7::EnforcementMode::Enforce; + + if effectively_denied { + emit_denial_simple( + denial_tx, + &host_lc, + port, + &binary_str, + &decision, + &reason, + "forward-l7-deny", + ); + respond(client, b"HTTP/1.1 403 Forbidden\r\n\r\n").await?; + return Ok(()); + } } // 5. DNS resolution + SSRF defence (mirrors the CONNECT path logic). diff --git a/e2e/rust/tests/forward_proxy_l7_bypass.rs b/e2e/rust/tests/forward_proxy_l7_bypass.rs index fb75176cc..89ecf2cd4 100644 --- a/e2e/rust/tests/forward_proxy_l7_bypass.rs +++ b/e2e/rust/tests/forward_proxy_l7_bypass.rs @@ -1,10 +1,10 @@ // SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 -//! Regression test: the forward proxy path must reject requests to endpoints -//! that have L7 rules configured. Before the fix, plain `http://` requests -//! (which use the forward proxy, not CONNECT) bypassed per-request method/path -//! enforcement entirely. +//! Regression tests: the forward proxy path must evaluate L7 rules for +//! endpoints that have them configured. Allowed requests (e.g. GET on a +//! read-only endpoint) should succeed; denied requests (e.g. POST) should +//! receive 403. #![cfg(feature = "e2e")] @@ -145,6 +145,7 @@ network_policies: - host: host.openshell.internal port: {port} protocol: rest + enforcement: enforce allowed_ips: - "172.0.0.0/8" rules: @@ -164,24 +165,21 @@ network_policies: Ok(file) } -/// The forward proxy path (plain http:// via HTTP_PROXY) must return 403 for -/// endpoints with L7 rules, forcing clients through the CONNECT tunnel where -/// per-request method/path inspection actually happens. +/// GET /allowed should succeed — the L7 policy explicitly allows it. #[tokio::test] -async fn forward_proxy_rejects_l7_configured_endpoint() { +async fn forward_proxy_allows_l7_permitted_request() { let server = DockerServer::start() .await .expect("start docker test server"); - let policy = write_policy_with_l7_rules(server.port).expect("write custom policy"); + let policy = + write_policy_with_l7_rules(server.port) + .expect("write custom policy"); let policy_path = policy .path() .to_str() .expect("temp policy path should be utf-8") .to_string(); - // Python script that tries a plain http:// request (forward proxy path). - // HTTP_PROXY is set automatically by the sandbox, so urllib will use the - // forward proxy for http:// URLs (not CONNECT). let script = format!( r#" import urllib.request, urllib.error, json, sys @@ -208,10 +206,60 @@ except Exception as e: .await .expect("sandbox create"); - // The forward proxy should return 403 because the endpoint has L7 rules. + // L7 policy allows GET /allowed — should succeed. + assert!( + guard.create_output.contains("\"status\": 200"), + "expected 200 for L7-allowed GET, got:\n{}", + guard.create_output + ); +} + +/// POST /allowed should be denied — the L7 policy only allows GET. +#[tokio::test] +async fn forward_proxy_denies_l7_blocked_request() { + let server = DockerServer::start() + .await + .expect("start docker test server"); + let policy = + write_policy_with_l7_rules(server.port) + .expect("write custom policy"); + let policy_path = policy + .path() + .to_str() + .expect("temp policy path should be utf-8") + .to_string(); + + let script = format!( + r#" +import urllib.request, urllib.error, json, sys +url = "http://host.openshell.internal:{port}/allowed" +req = urllib.request.Request(url, data=b"test", method="POST") +try: + resp = urllib.request.urlopen(req, timeout=15) + print(json.dumps({{"status": resp.status, "error": None}})) +except urllib.error.HTTPError as e: + print(json.dumps({{"status": e.code, "error": str(e)}})) +except Exception as e: + print(json.dumps({{"status": -1, "error": str(e)}})) +"#, + port = server.port, + ); + + let guard = SandboxGuard::create(&[ + "--policy", + &policy_path, + "--", + "python3", + "-c", + &script, + ]) + .await + .expect("sandbox create"); + + // L7 policy denies POST — should return 403. assert!( guard.create_output.contains("\"status\": 403"), - "expected 403 from forward proxy for L7-configured endpoint, got:\n{}", + "expected 403 for L7-denied POST, got:\n{}", guard.create_output ); } From a69ef060349bb5c59ee0f40a8b95d17913e05641 Mon Sep 17 00:00:00 2001 From: "John T. Myers" <9696606+johntmyers@users.noreply.github.com> Date: Mon, 30 Mar 2026 09:43:42 -0700 Subject: [PATCH 03/20] fix(ci): skip docs preview deploy for fork PRs (#679) Fork PRs receive a read-only GITHUB_TOKEN that cannot push to gh-pages, causing the deploy step to fail with a 403. Skip the deploy for fork PRs while still running the docs build to validate changes compile cleanly. --- .github/workflows/docs-preview-pr.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/docs-preview-pr.yml b/.github/workflows/docs-preview-pr.yml index ccade7411..6c0672ba2 100644 --- a/.github/workflows/docs-preview-pr.yml +++ b/.github/workflows/docs-preview-pr.yml @@ -49,6 +49,7 @@ jobs: find _build -name .buildinfo -exec rm {} \; - name: Deploy preview + if: github.event.pull_request.head.repo.full_name == github.repository uses: rossjrw/pr-preview-action@v1 with: source-dir: ./_build/docs/ From c1dd81e5d4f8891a945810b6313b50ac34e53046 Mon Sep 17 00:00:00 2001 From: Drew Newberry Date: Mon, 30 Mar 2026 10:00:04 -0700 Subject: [PATCH 04/20] docs(rfc): add RFC process with draft/review/accepted lifecycle (#678) --- CONTRIBUTING.md | 5 ++ rfc/0000-template.md | 49 +++++++++++++++ rfc/README.md | 140 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 194 insertions(+) create mode 100644 rfc/0000-template.md create mode 100644 rfc/README.md diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index c25d30b92..1ebf71df2 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -186,9 +186,14 @@ These are the primary `mise` tasks for day-to-day development: | `tasks/` | `mise` task definitions and build scripts | | `deploy/` | Dockerfiles, Helm chart, Kubernetes manifests | | `architecture/` | Architecture docs and plans | +| `rfc/` | Request for Comments proposals | | `docs/` | User-facing documentation (Sphinx/MyST) | | `.agents/` | Agent skills and persona definitions | +## RFCs + +For cross-cutting architectural decisions, API contract changes, or process proposals that need broad consensus, use the RFC process. RFCs live in `rfc/` — copy the template, fill it in, and open a PR for discussion. See [rfc/README.md](rfc/README.md) for the full lifecycle and guidelines on when to write an RFC versus a spike issue or architecture doc. + ## Documentation If your change affects user-facing behavior (new flags, changed defaults, new features, bug fixes that contradict existing docs), update the relevant pages under `docs/` in the same PR. diff --git a/rfc/0000-template.md b/rfc/0000-template.md new file mode 100644 index 000000000..ec66ca967 --- /dev/null +++ b/rfc/0000-template.md @@ -0,0 +1,49 @@ +--- +authors: + - "@your-github-username" +state: draft +links: + - (related PRs, discussions, or issues) +--- + +# RFC NNNN - Your Title Here + + + +## Summary + +One paragraph explanation of the feature or change. + +## Motivation + +Why are we doing this? What problem does it solve? What use cases does it support? + +## Non-goals + +What is explicitly out of scope for this RFC? + +## Proposal + +What are we actually proposing? Explain the design in enough detail that someone familiar with OpenShell can understand and implement it. + +## Implementation plan + +How do we get from here to there? Consider rollout strategy, backwards compatibility, and migration. + +## Risks + +Why should we *not* do this? What are the costs? + +## Alternatives + +What other designs have been considered? What is the impact of not doing this? + +## Prior art + +Does this feature exist in other projects? What can we learn from them? + +## Open questions + +What parts of the design are still TBD? diff --git a/rfc/README.md b/rfc/README.md new file mode 100644 index 000000000..b409cad00 --- /dev/null +++ b/rfc/README.md @@ -0,0 +1,140 @@ +# OpenShell RFCs + +Substantial changes to OpenShell should be proposed in writing before implementation begins. An RFC provides a consistent way to propose an idea, collect feedback from the community, build consensus, and document the decision for future contributors. Not every change needs an RFC — bug fixes, small features, and routine maintenance go through normal pull requests. RFCs are for the changes that are cross-cutting, potentially controversial, or significant enough that stakeholders should weigh in before code is written. + +## Start with a GitHub Discussion + +Before writing an RFC, consider opening a [GitHub Discussion](https://github.com/NVIDIA/OpenShell/discussions) to gauge interest and get early feedback. This helps: + +- Validate that the problem is worth solving +- Surface potential concerns early +- Build consensus before investing in a detailed proposal +- Identify the right reviewers and stakeholders + +If the discussion shows sufficient interest and the idea has merit, then it's time to write an RFC to detail the plan and technical approach. + +## RFCs vs other artifacts + +OpenShell has several places where design information lives. Use this guide to pick the right one: + +| Artifact | Purpose | When to use | +|----------|---------|-------------| +| **GitHub Discussion** | Gauge interest in a rough idea | You have a thought but aren't sure it's worth a proposal yet | +| **Spike issue** (`create-spike`) | Investigate implementation feasibility for a scoped change | You need to explore the codebase and produce a buildable issue for a specific component or feature | +| **RFC** | Propose a cross-cutting decision that needs broad consensus | Architectural changes, API contracts, process changes, or anything that spans multiple components or teams | +| **Architecture doc** (`architecture/`) | Document how things work today | Living reference material — updated as the system evolves | + +The key distinction: **spikes investigate whether and how something can be done; RFCs propose that we should do it and seek agreement on the approach.** A spike may precede an RFC (to gather data) or follow one (to flesh out implementation details). When an RFC reaches `implemented`, its relevant content should be folded into the appropriate `architecture/` docs so the living reference stays current. + +## When to use an RFC + +The following are examples of when an RFC is appropriate: + +- An architectural or design decision for the platform +- Change to an API or command-line tool +- Change to an internal API or tool +- Add or change a company or team process +- A design for testing + +RFCs don't only apply to technical ideas but overall project ideas and processes as well. If you have an idea to improve the way something is being done, you have the power to make your voice heard. + +## When NOT to use an RFC + +Not everything needs an RFC. Skip the RFC process for: + +- Bug fixes +- Small feature additions scoped to a single component (use a spike instead) +- Documentation changes +- Dependency updates +- Refactoring that doesn't change public interfaces + +If a change doesn't require cross-team consensus, a spike issue is the right vehicle. + +## RFC metadata and state + +At the start of every RFC document, we include a brief amount of metadata in YAML front matter: + +```yaml +--- +authors: + - "@username" +state: draft +links: + - https://github.com/NVIDIA/OpenShell/pull/123 + - https://github.com/NVIDIA/OpenShell/discussions/456 +--- +``` + +We track the following metadata: + +- **authors**: The authors (and therefore owners) of an RFC. Listed as GitHub usernames. +- **state**: Must be one of the states discussed below. +- **links**: Related PRs, discussions, or issues. Add entries as the RFC progresses. +- **superseded_by**: *(optional)* For RFCs in the `superseded` state, the RFC number that replaces this one (e.g., `0005`). + +An RFC can be in one of the following states: + +| State | Description | +|-------|-------------| +| `draft` | The RFC is being written and is not yet ready for formal review. | +| `review` | Under active discussion in a pull request. | +| `accepted` | The proposal has been accepted and is ready for implementation. | +| `rejected` | The proposal was reviewed and declined. | +| `implemented` | The idea has been entirely implemented. Changes would be infrequent. | +| `superseded` | Replaced by a newer RFC. The `superseded_by` field should reference the replacement. | + +## RFC lifecycle + +### 1. Reserve an RFC number + +Look at the existing RFCs in this directory and choose the next available number. If two authors happen to pick the same number on separate branches, the conflict is resolved during PR review — the later PR simply picks the next available number. + +### 2. Create your RFC + +Copy `0000-template.md` to `NNNN-my-feature.md` where `NNNN` is your RFC number (zero-padded to 4 digits) and `my-feature` is a short descriptive name. + +Fill in the metadata and start writing. The state should be `draft` while you're iterating. + +### 3. Open a pull request + +When you're ready for feedback, update the state to `review` and open a pull request. Add the PR link to the `pr` field in the metadata. + +The PR is where discussion happens. Anyone subscribed to the repo will get a notification and can read your RFC and provide feedback. + +### 4. Iterate and build consensus + +The comments you choose to accept are up to you as the owner of the RFC, but you should remain empathetic in how you engage. For those giving feedback, be sure that all feedback is constructive. + +RFCs rarely go through this process unchanged. Make edits as new commits to the PR and leave comments explaining your changes. + +### 5. Merge the pull request + +After there has been time for folks to comment, the RFC author requests merge and a maintainer approves and merges. The state should be updated from `review` to `accepted`. If the proposal is declined, the state should be set to `rejected`. The timing is left to the author's discretion. As a guideline, a few business days seems reasonable, but circumstances may dictate a different timeline. + +In general, RFCs shouldn't be merged if no one else has read or commented on it. If no one is reading your RFC, it's time to explicitly ask someone to give it a read! + +### 6. Implementation + +Once an RFC has been entirely implemented, its state should be moved to `implemented`. This represents ideas that have been fully developed. While discussion on implemented RFCs is permitted, changes would be expected to be infrequent. + +## Diagrams + +When an RFC needs diagrams to illustrate architecture, data flow, or component interactions, use [Mermaid](https://mermaid.js.org/) diagrams embedded directly in the Markdown. Mermaid renders natively on GitHub and keeps diagrams version-controlled alongside the text. + +````markdown +```mermaid +graph LR + A[Client] --> B[Gateway] + B --> C[Sandbox] +``` +```` + +Prefer Mermaid over external image files whenever possible. If a diagram is too complex for Mermaid (e.g., detailed UI mockups), commit the image to the same directory as the RFC and reference it with a relative path. + +## Making changes to an RFC + +After your RFC has been merged, there is always opportunity to make changes. Open a pull request with the change you would like to make. If you are not the original author, be sure to @ the original authors to make sure they see and approve of the changes. + +## RFC postponement + +Some RFCs are marked `rejected` when the proposal is declined or when we want neither to think about evaluating the proposal nor about implementing it until some time in the future. Rejected RFCs may be revisited when the time is right. From 0832f11a6639ad6d6c04ef3d40e892e15339a072 Mon Sep 17 00:00:00 2001 From: "John T. Myers" <9696606+johntmyers@users.noreply.github.com> Date: Mon, 30 Mar 2026 11:58:45 -0700 Subject: [PATCH 05/20] fix(e2e): add uv-managed python binary glob to forward proxy L7 test (#686) The base sandbox image installs Python via uv at /sandbox/.uv/python/*/bin/python*, but the proxy resolves binary identity via /proc/PID/exe (the real path, not the symlink). The test policy only listed /usr/bin/python* and /usr/local/bin/python*, so OPA denied the connection at L4 before L7 evaluation could run. Co-authored-by: John Myers --- e2e/rust/tests/forward_proxy_l7_bypass.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/e2e/rust/tests/forward_proxy_l7_bypass.rs b/e2e/rust/tests/forward_proxy_l7_bypass.rs index 89ecf2cd4..c3ae584b0 100644 --- a/e2e/rust/tests/forward_proxy_l7_bypass.rs +++ b/e2e/rust/tests/forward_proxy_l7_bypass.rs @@ -156,6 +156,7 @@ network_policies: - path: /usr/bin/curl - path: /usr/bin/python* - path: /usr/local/bin/python* + - path: /sandbox/.uv/python/*/bin/python* "# ); file.write_all(policy.as_bytes()) From 38655a65eca9489b717cee8c371845467f3be350 Mon Sep 17 00:00:00 2001 From: LateNightHackathon <256481314+latenighthackathon@users.noreply.github.com> Date: Mon, 30 Mar 2026 14:12:29 -0500 Subject: [PATCH 06/20] fix(l7): reject requests with both CL and TE headers in inference parser (CWE-444) (#671) The CL/TE desynchronisation guard added in #663 for the REST path was not applied to the inference request parser. A request containing both Content-Length and Transfer-Encoding headers could be interpreted differently by the proxy and the upstream server, enabling HTTP request smuggling (CWE-444, RFC 7230 Section 3.3.3). Add the same rejection check and tests mirroring the REST parser coverage, including TE substring validation. Signed-off-by: latenighthackathon Co-authored-by: latenighthackathon --- crates/openshell-sandbox/src/l7/inference.rs | 57 ++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/crates/openshell-sandbox/src/l7/inference.rs b/crates/openshell-sandbox/src/l7/inference.rs index 140213f07..5136c8783 100644 --- a/crates/openshell-sandbox/src/l7/inference.rs +++ b/crates/openshell-sandbox/src/l7/inference.rs @@ -164,6 +164,12 @@ pub fn try_parse_http_request(buf: &[u8]) -> ParseResult { } } + if is_chunked && has_content_length { + return ParseResult::Invalid( + "Request contains both Transfer-Encoding and Content-Length headers".to_string(), + ); + } + let (body, consumed) = if is_chunked { let Some((decoded_body, consumed)) = parse_chunked_body(buf, body_start) else { return ParseResult::Incomplete; @@ -570,6 +576,24 @@ mod tests { assert_eq!(parsed.body.len(), 100); } + /// SEC: Transfer-Encoding substring match must not match partial tokens. + #[test] + fn te_substring_not_chunked() { + let body = r#"{"model":"m","messages":[]}"#; + let request = format!( + "POST /v1/chat/completions HTTP/1.1\r\n\ + Host: x\r\n\ + Transfer-Encoding: chunkedx\r\n\ + Content-Length: {}\r\n\ + \r\n{body}", + body.len(), + ); + let ParseResult::Complete(parsed, _) = try_parse_http_request(request.as_bytes()) else { + panic!("expected Complete for non-matching TE with valid CL"); + }; + assert_eq!(parsed.body.len(), body.len()); + } + // ---- SEC: Content-Length validation ---- #[test] @@ -608,4 +632,37 @@ mod tests { ParseResult::Invalid(_) )); } + + // ---- SEC-009: CL/TE desynchronisation ---- + + /// Reject requests with both Content-Length and Transfer-Encoding to + /// prevent CL/TE request smuggling (RFC 7230 Section 3.3.3). + #[test] + fn reject_dual_content_length_and_transfer_encoding() { + let request = b"POST /v1/chat/completions HTTP/1.1\r\nHost: x\r\nContent-Length: 5\r\nTransfer-Encoding: chunked\r\n\r\n"; + assert!( + matches!( + try_parse_http_request(request), + ParseResult::Invalid(reason) + if reason.contains("Transfer-Encoding") + && reason.contains("Content-Length") + ), + "Must reject request with both CL and TE" + ); + } + + /// Same rejection regardless of header order. + #[test] + fn reject_dual_transfer_encoding_and_content_length() { + let request = b"POST /v1/chat/completions HTTP/1.1\r\nHost: x\r\nTransfer-Encoding: chunked\r\nContent-Length: 5\r\n\r\n"; + assert!( + matches!( + try_parse_http_request(request), + ParseResult::Invalid(reason) + if reason.contains("Transfer-Encoding") + && reason.contains("Content-Length") + ), + "Must reject request with both TE and CL" + ); + } } From 758c62d18dccf7a3a92bae31ac62fe94d5bf7434 Mon Sep 17 00:00:00 2001 From: "John T. Myers" <9696606+johntmyers@users.noreply.github.com> Date: Mon, 30 Mar 2026 13:19:19 -0700 Subject: [PATCH 07/20] fix(sandbox): handle per-path Landlock errors instead of abandoning entire ruleset (#677) * fix(sandbox): handle per-path Landlock errors instead of abandoning entire ruleset A single missing path (e.g., /app in containers without that directory) caused PathFd::new() to propagate an error out of the entire Landlock setup closure. Under BestEffort mode, this silently disabled all filesystem restrictions for the sandbox. Changes: - Extract try_open_path() and classify_path_error() helpers that handle PathFd failures per-path instead of per-ruleset - BestEffort mode: skip inaccessible paths with a warning, apply remaining rules - HardRequirement mode: fail immediately on any inaccessible path - Add zero-rule safety check to prevent applying an empty ruleset that would block all filesystem access - Pre-filter system-injected baseline paths (e.g., /app) in enrichment functions so missing paths never reach Landlock - Add unit tests for try_open_path, classify_path_error, and error classification for ENOENT, EACCES, ELOOP, ENAMETOOLONG, ENOTDIR - Update user-facing docs and architecture docs with Landlock behavior tables, baseline path filtering, and compatibility mode semantics - Fix stale ABI::V1 references in docs (code uses ABI::V2) Closes #664 * fix(sandbox): use debug log for NotFound in Landlock best-effort mode NotFound errors for stale baseline paths (e.g. /app persisted in the server-stored policy but absent in this container) are expected in best-effort mode. Downgrade from warn! to debug! so the message does not leak into SSH exec stdout (the pre_exec hook inherits the tracing subscriber whose writer targets fd 1). Genuine errors (permission denied, symlink loops, etc.) remain at warn! for operator visibility. Also move custom_image e2e marker from /opt to /etc (a Landlock baseline read-only path) since the security fix now properly enforces filesystem restrictions. --------- Co-authored-by: John Myers --- architecture/sandbox.md | 12 +- architecture/security-policy.md | 14 +- crates/openshell-sandbox/src/lib.rs | 35 +++ .../src/sandbox/linux/landlock.rs | 231 ++++++++++++++++-- docs/reference/policy-schema.md | 15 +- docs/sandboxes/policies.md | 15 +- e2e/rust/tests/custom_image.rs | 6 +- 7 files changed, 293 insertions(+), 35 deletions(-) diff --git a/architecture/sandbox.md b/architecture/sandbox.md index 1117d0f71..bfc71ba31 100644 --- a/architecture/sandbox.md +++ b/architecture/sandbox.md @@ -431,15 +431,21 @@ Landlock restricts the child process's filesystem access to an explicit allowlis 1. Build path lists from `filesystem.read_only` and `filesystem.read_write` 2. If `include_workdir` is true, add the working directory to `read_write` 3. If both lists are empty, skip Landlock entirely (no-op) -4. Create a Landlock ruleset targeting ABI V1: +4. Create a Landlock ruleset targeting ABI V2: - Read-only paths receive `AccessFs::from_read(abi)` rights - Read-write paths receive `AccessFs::from_all(abi)` rights -5. Call `ruleset.restrict_self()` -- this applies to the calling process and all descendants +5. For each path, attempt `PathFd::new()`. If it fails: + - `BestEffort`: Log a warning with the error classification (not found, permission denied, symlink loop, etc.) and skip the path. Continue building the ruleset from remaining valid paths. + - `HardRequirement`: Return a fatal error, aborting the sandbox. +6. If all paths failed (zero rules applied), return an error rather than calling `restrict_self()` on an empty ruleset (which would block all filesystem access) +7. Call `ruleset.restrict_self()` -- this applies to the calling process and all descendants -Error behavior depends on `LandlockCompatibility`: +Kernel-level error behavior (e.g., Landlock ABI unavailable) depends on `LandlockCompatibility`: - `BestEffort`: Log a warning and continue without filesystem isolation - `HardRequirement`: Return a fatal error, aborting the sandbox +**Baseline path filtering**: System-injected baseline paths (e.g., `/app`) are pre-filtered by `enrich_proto_baseline_paths()` / `enrich_sandbox_baseline_paths()` using `Path::exists()` before they reach Landlock. User-specified paths are not pre-filtered -- they are evaluated at Landlock apply time so misconfigurations surface as warnings or errors. + ### Seccomp syscall filtering **File:** `crates/openshell-sandbox/src/sandbox/linux/seccomp.rs` diff --git a/architecture/security-policy.md b/architecture/security-policy.md index 8c41e5b91..8b7b61d21 100644 --- a/architecture/security-policy.md +++ b/architecture/security-policy.md @@ -320,7 +320,7 @@ Controls which filesystem paths the sandboxed process can access. Enforced via L | `read_only` | `string[]` | `[]` | Paths accessible in read-only mode | | `read_write` | `string[]` | `[]` | Paths accessible in read-write mode | -**Enforcement mapping**: Each path becomes a Landlock `PathBeneath` rule. Read-only paths receive `AccessFs::from_read(ABI::V1)` permissions. Read-write paths receive `AccessFs::from_all(ABI::V1)` permissions (read, write, execute, create, delete, rename). All other paths are denied by the Landlock ruleset. +**Enforcement mapping**: Each path becomes a Landlock `PathBeneath` rule. Read-only paths receive `AccessFs::from_read(ABI::V2)` permissions. Read-write paths receive `AccessFs::from_all(ABI::V2)` permissions (read, write, execute, create, delete, rename). All other paths are denied by the Landlock ruleset. **Filesystem preparation**: Before the child process spawns, the supervisor creates any `read_write` directories that do not exist and sets their ownership to `process.run_as_user`:`process.run_as_group` via `chown()`. See `crates/openshell-sandbox/src/lib.rs` -- `prepare_filesystem()`. @@ -358,10 +358,16 @@ Controls Landlock LSM compatibility behavior. **Static field** -- immutable afte | Value | Behavior | | ------------------ | --------------------------------------------------------------------------------------------------------------------------- | -| `best_effort` | If Landlock is unavailable (older kernel, unprivileged container), log a warning and continue without filesystem sandboxing | -| `hard_requirement` | If Landlock is unavailable, abort sandbox startup with an error | +| `best_effort` | If Landlock is unavailable (older kernel, unprivileged container), log a warning and continue without filesystem sandboxing. Individual inaccessible paths (missing, permission denied, symlink loops) are skipped with a warning while remaining rules are still applied. If all paths fail, the sandbox continues without Landlock rather than applying an empty ruleset that would block all access. | +| `hard_requirement` | If Landlock is unavailable or any configured path cannot be opened, abort sandbox startup with an error. | -See `crates/openshell-sandbox/src/sandbox/linux/landlock.rs` -- `compat_level()`. +**Per-path error handling**: `PathFd::new()` (which wraps `open(path, O_PATH | O_CLOEXEC)`) can fail for several reasons beyond path non-existence: `EACCES` (permission denied), `ELOOP` (symlink loop), `ENAMETOOLONG`, `ENOTDIR`. Each failure is classified with a human-readable reason in logs. In `best_effort` mode, the path is skipped and ruleset construction continues. In `hard_requirement` mode, the error is fatal. + +**Baseline path filtering**: The enrichment functions (`enrich_proto_baseline_paths`, `enrich_sandbox_baseline_paths`) pre-filter system-injected baseline paths (e.g., `/app`) by checking `Path::exists()` before adding them to the policy. This prevents missing baseline paths from reaching Landlock at all. User-specified paths are not pre-filtered — they are evaluated at Landlock apply time so that misconfigurations surface as warnings (`best_effort`) or errors (`hard_requirement`). + +**Zero-rule safety check**: If all paths in the ruleset fail to open, `apply()` returns an error rather than calling `restrict_self()` on an empty ruleset. An empty Landlock ruleset with `restrict_self()` would block all filesystem access — the inverse of the intended degradation behavior. This error is caught by the outer `BestEffort` handler, which logs a warning and continues without Landlock. + +See `crates/openshell-sandbox/src/sandbox/linux/landlock.rs` -- `compat_level()`, `try_open_path()`, `classify_path_fd_error()`, `classify_io_error()`. ```yaml landlock: diff --git a/crates/openshell-sandbox/src/lib.rs b/crates/openshell-sandbox/src/lib.rs index 493e4d237..297d7fc38 100644 --- a/crates/openshell-sandbox/src/lib.rs +++ b/crates/openshell-sandbox/src/lib.rs @@ -899,12 +899,31 @@ fn enrich_proto_baseline_paths(proto: &mut openshell_core::proto::SandboxPolicy) let mut modified = false; for &path in PROXY_BASELINE_READ_ONLY { if !fs.read_only.iter().any(|p| p.as_str() == path) { + // Baseline paths are system-injected, not user-specified. Skip + // paths that do not exist in this container image to avoid noisy + // warnings from Landlock and, more critically, to prevent a single + // missing baseline path from abandoning the entire Landlock + // ruleset under best-effort mode (see issue #664). + if !std::path::Path::new(path).exists() { + debug!( + path, + "Baseline read-only path does not exist, skipping enrichment" + ); + continue; + } fs.read_only.push(path.to_string()); modified = true; } } for &path in PROXY_BASELINE_READ_WRITE { if !fs.read_write.iter().any(|p| p.as_str() == path) { + if !std::path::Path::new(path).exists() { + debug!( + path, + "Baseline read-write path does not exist, skipping enrichment" + ); + continue; + } fs.read_write.push(path.to_string()); modified = true; } @@ -929,6 +948,15 @@ fn enrich_sandbox_baseline_paths(policy: &mut SandboxPolicy) { for &path in PROXY_BASELINE_READ_ONLY { let p = std::path::PathBuf::from(path); if !policy.filesystem.read_only.contains(&p) { + // Baseline paths are system-injected — skip non-existent paths to + // avoid Landlock ruleset abandonment (issue #664). + if !p.exists() { + debug!( + path, + "Baseline read-only path does not exist, skipping enrichment" + ); + continue; + } policy.filesystem.read_only.push(p); modified = true; } @@ -936,6 +964,13 @@ fn enrich_sandbox_baseline_paths(policy: &mut SandboxPolicy) { for &path in PROXY_BASELINE_READ_WRITE { let p = std::path::PathBuf::from(path); if !policy.filesystem.read_write.contains(&p) { + if !p.exists() { + debug!( + path, + "Baseline read-write path does not exist, skipping enrichment" + ); + continue; + } policy.filesystem.read_write.push(p); modified = true; } diff --git a/crates/openshell-sandbox/src/sandbox/linux/landlock.rs b/crates/openshell-sandbox/src/sandbox/linux/landlock.rs index e276840dd..abb91fd4f 100644 --- a/crates/openshell-sandbox/src/sandbox/linux/landlock.rs +++ b/crates/openshell-sandbox/src/sandbox/linux/landlock.rs @@ -5,11 +5,11 @@ use crate::policy::{LandlockCompatibility, SandboxPolicy}; use landlock::{ - ABI, Access, AccessFs, CompatLevel, Compatible, PathBeneath, PathFd, Ruleset, RulesetAttr, - RulesetCreatedAttr, + ABI, Access, AccessFs, CompatLevel, Compatible, PathBeneath, PathFd, PathFdError, Ruleset, + RulesetAttr, RulesetCreatedAttr, }; use miette::{IntoDiagnostic, Result}; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use tracing::{debug, info, warn}; pub fn apply(policy: &SandboxPolicy, workdir: Option<&str>) -> Result<()> { @@ -29,6 +29,7 @@ pub fn apply(policy: &SandboxPolicy, workdir: Option<&str>) -> Result<()> { return Ok(()); } + let total_paths = read_only.len() + read_write.len(); let abi = ABI::V2; info!( abi = ?abi, @@ -38,47 +39,61 @@ pub fn apply(policy: &SandboxPolicy, workdir: Option<&str>) -> Result<()> { "Applying Landlock filesystem sandbox" ); + let compatibility = &policy.landlock.compatibility; + let result: Result<()> = (|| { let access_all = AccessFs::from_all(abi); let access_read = AccessFs::from_read(abi); let mut ruleset = Ruleset::default(); ruleset = ruleset - .set_compatibility(compat_level(&policy.landlock.compatibility)) + .set_compatibility(compat_level(compatibility)) .handle_access(access_all) .into_diagnostic()?; let mut ruleset = ruleset.create().into_diagnostic()?; + let mut rules_applied: usize = 0; + + for path in &read_only { + if let Some(path_fd) = try_open_path(path, compatibility)? { + debug!(path = %path.display(), "Landlock allow read-only"); + ruleset = ruleset + .add_rule(PathBeneath::new(path_fd, access_read)) + .into_diagnostic()?; + rules_applied += 1; + } + } - for path in read_only { - debug!(path = %path.display(), "Landlock allow read-only"); - ruleset = ruleset - .add_rule(PathBeneath::new( - PathFd::new(path).into_diagnostic()?, - access_read, - )) - .into_diagnostic()?; + for path in &read_write { + if let Some(path_fd) = try_open_path(path, compatibility)? { + debug!(path = %path.display(), "Landlock allow read-write"); + ruleset = ruleset + .add_rule(PathBeneath::new(path_fd, access_all)) + .into_diagnostic()?; + rules_applied += 1; + } } - for path in read_write { - debug!(path = %path.display(), "Landlock allow read-write"); - ruleset = ruleset - .add_rule(PathBeneath::new( - PathFd::new(path).into_diagnostic()?, - access_all, - )) - .into_diagnostic()?; + if rules_applied == 0 { + return Err(miette::miette!( + "Landlock ruleset has zero valid paths — all {} path(s) failed to open. \ + Refusing to apply an empty ruleset that would block all filesystem access.", + total_paths, + )); } + let skipped = total_paths - rules_applied; + info!( + rules_applied, + skipped, "Landlock ruleset built successfully" + ); + ruleset.restrict_self().into_diagnostic()?; Ok(()) })(); if let Err(err) = result { - if matches!( - policy.landlock.compatibility, - LandlockCompatibility::BestEffort - ) { + if matches!(compatibility, LandlockCompatibility::BestEffort) { warn!( error = %err, "Landlock filesystem sandbox is UNAVAILABLE — running WITHOUT filesystem restrictions. \ @@ -92,9 +107,177 @@ pub fn apply(policy: &SandboxPolicy, workdir: Option<&str>) -> Result<()> { Ok(()) } +/// Attempt to open a path for Landlock rule creation. +/// +/// In `BestEffort` mode, inaccessible paths (missing, permission denied, symlink +/// loops, etc.) are skipped with a warning and `Ok(None)` is returned so the +/// caller can continue building the ruleset from the remaining valid paths. +/// +/// In `HardRequirement` mode, any failure is fatal — the caller propagates the +/// error, which ultimately aborts sandbox startup. +fn try_open_path(path: &Path, compatibility: &LandlockCompatibility) -> Result> { + match PathFd::new(path) { + Ok(fd) => Ok(Some(fd)), + Err(err) => { + let reason = classify_path_fd_error(&err); + let is_not_found = matches!( + &err, + PathFdError::OpenCall { source, .. } + if source.kind() == std::io::ErrorKind::NotFound + ); + match compatibility { + LandlockCompatibility::BestEffort => { + // NotFound is expected for stale baseline paths (e.g. + // /app baked into the server-stored policy but absent + // in this container image). Log at debug! to avoid + // polluting SSH exec stdout — the pre_exec hook + // inherits the tracing subscriber whose writer targets + // fd 1 (the pipe/PTY). + // + // Other errors (permission denied, symlink loops, etc.) + // are genuinely unexpected and logged at warn!. + if is_not_found { + debug!( + path = %path.display(), + reason, + "Skipping non-existent Landlock path (best-effort mode)" + ); + } else { + warn!( + path = %path.display(), + error = %err, + reason, + "Skipping inaccessible Landlock path (best-effort mode)" + ); + } + Ok(None) + } + LandlockCompatibility::HardRequirement => Err(miette::miette!( + "Landlock path unavailable in hard_requirement mode: {} ({}): {}", + path.display(), + reason, + err, + )), + } + } + } +} + +/// Classify a [`PathFdError`] into a human-readable reason. +/// +/// `PathFd::new()` wraps `open(path, O_PATH | O_CLOEXEC)` which can fail for +/// several reasons beyond simple non-existence. The `PathFdError::OpenCall` +/// variant wraps the underlying `std::io::Error`. +fn classify_path_fd_error(err: &PathFdError) -> &'static str { + match err { + PathFdError::OpenCall { source, .. } => classify_io_error(source), + // PathFdError is #[non_exhaustive], handle future variants gracefully. + _ => "unexpected error", + } +} + +/// Classify a `std::io::Error` into a human-readable reason string. +fn classify_io_error(err: &std::io::Error) -> &'static str { + match err.kind() { + std::io::ErrorKind::NotFound => "path does not exist", + std::io::ErrorKind::PermissionDenied => "permission denied", + _ => match err.raw_os_error() { + Some(40) => "too many symlink levels", // ELOOP + Some(36) => "path name too long", // ENAMETOOLONG + Some(20) => "path component is not a directory", // ENOTDIR + _ => "unexpected error", + }, + } +} + fn compat_level(level: &LandlockCompatibility) -> CompatLevel { match level { LandlockCompatibility::BestEffort => CompatLevel::BestEffort, LandlockCompatibility::HardRequirement => CompatLevel::HardRequirement, } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn try_open_path_best_effort_returns_none_for_missing_path() { + let result = try_open_path( + &PathBuf::from("/nonexistent/openshell/test/path"), + &LandlockCompatibility::BestEffort, + ); + assert!(result.is_ok()); + assert!(result.unwrap().is_none()); + } + + #[test] + fn try_open_path_hard_requirement_errors_for_missing_path() { + let result = try_open_path( + &PathBuf::from("/nonexistent/openshell/test/path"), + &LandlockCompatibility::HardRequirement, + ); + assert!(result.is_err()); + let err_msg = result.unwrap_err().to_string(); + assert!( + err_msg.contains("hard_requirement"), + "error should mention hard_requirement mode: {err_msg}" + ); + assert!( + err_msg.contains("does not exist"), + "error should include the classified reason: {err_msg}" + ); + } + + #[test] + fn try_open_path_succeeds_for_existing_path() { + let dir = tempfile::tempdir().unwrap(); + let result = try_open_path(dir.path(), &LandlockCompatibility::BestEffort); + assert!(result.is_ok()); + assert!(result.unwrap().is_some()); + } + + #[test] + fn classify_not_found() { + let err = std::io::Error::from_raw_os_error(libc::ENOENT); + assert_eq!(classify_io_error(&err), "path does not exist"); + } + + #[test] + fn classify_permission_denied() { + let err = std::io::Error::from_raw_os_error(libc::EACCES); + assert_eq!(classify_io_error(&err), "permission denied"); + } + + #[test] + fn classify_symlink_loop() { + let err = std::io::Error::from_raw_os_error(libc::ELOOP); + assert_eq!(classify_io_error(&err), "too many symlink levels"); + } + + #[test] + fn classify_name_too_long() { + let err = std::io::Error::from_raw_os_error(libc::ENAMETOOLONG); + assert_eq!(classify_io_error(&err), "path name too long"); + } + + #[test] + fn classify_not_a_directory() { + let err = std::io::Error::from_raw_os_error(libc::ENOTDIR); + assert_eq!(classify_io_error(&err), "path component is not a directory"); + } + + #[test] + fn classify_unknown_error() { + let err = std::io::Error::from_raw_os_error(libc::EIO); + assert_eq!(classify_io_error(&err), "unexpected error"); + } + + #[test] + fn classify_path_fd_error_extracts_io_error() { + // Use PathFd::new on a non-existent path to get a real PathFdError + // (the OpenCall variant is #[non_exhaustive] and can't be constructed directly). + let err = PathFd::new("/nonexistent/openshell/classify/test").unwrap_err(); + assert_eq!(classify_path_fd_error(&err), "path does not exist"); + } +} diff --git a/docs/reference/policy-schema.md b/docs/reference/policy-schema.md index cb37d0bae..6916e8d0c 100644 --- a/docs/reference/policy-schema.md +++ b/docs/reference/policy-schema.md @@ -105,7 +105,20 @@ Configures [Landlock LSM](https://docs.kernel.org/security/landlock.html) enforc | Field | Type | Required | Values | Description | |---|---|---|---|---| -| `compatibility` | string | No | `best_effort`, `hard_requirement` | How OpenShell handles kernel ABI differences. `best_effort` uses the highest Landlock ABI the host kernel supports. `hard_requirement` fails if the required ABI is unavailable. | +| `compatibility` | string | No | `best_effort`, `hard_requirement` | How OpenShell handles Landlock failures. See behavior table below. | + +**Compatibility modes:** + +| Value | Kernel ABI unavailable | Individual path inaccessible | All paths inaccessible | +|---|---|---|---| +| `best_effort` | Warns and continues without Landlock. | Skips the path, applies remaining rules. | Warns and continues without Landlock (refuses to apply an empty ruleset). | +| `hard_requirement` | Aborts sandbox startup. | Aborts sandbox startup. | Aborts sandbox startup. | + +`best_effort` (the default) is appropriate for most deployments. It handles missing paths gracefully -- for example, `/app` may not exist in every container image but is included in the baseline path set for containers that do have it. Individual missing paths are skipped while the remaining filesystem rules are still enforced. + +`hard_requirement` is for environments where any gap in filesystem isolation is unacceptable. If a listed path cannot be opened for any reason (missing, permission denied, symlink loop), sandbox startup fails immediately rather than running with reduced protection. + +When a path is skipped under `best_effort`, the sandbox logs a warning that includes the path, the specific error, and a human-readable reason (for example, "path does not exist" or "permission denied"). Example: diff --git a/docs/sandboxes/policies.md b/docs/sandboxes/policies.md index 565a7a4c9..fa5ed5d8b 100644 --- a/docs/sandboxes/policies.md +++ b/docs/sandboxes/policies.md @@ -70,10 +70,23 @@ Dynamic sections can be updated on a running sandbox with `openshell policy set` | Section | Type | Description | |---|---|---| | `filesystem_policy` | Static | Controls which directories the agent can access on disk. Paths are split into `read_only` and `read_write` lists. Any path not listed in either list is inaccessible. Set `include_workdir: true` to automatically add the agent's working directory to `read_write`. [Landlock LSM](https://docs.kernel.org/security/landlock.html) enforces these restrictions at the kernel level. | -| `landlock` | Static | Configures Landlock LSM enforcement behavior. Set `compatibility` to `best_effort` (use the highest ABI the host kernel supports) or `hard_requirement` (fail if the required ABI is unavailable). | +| `landlock` | Static | Configures Landlock LSM enforcement behavior. Set `compatibility` to `best_effort` (skip individual inaccessible paths while applying remaining rules) or `hard_requirement` (fail if any path is inaccessible or the required kernel ABI is unavailable). See the [Policy Schema Reference](../reference/policy-schema.md#landlock) for the full behavior table. | | `process` | Static | Sets the OS-level identity for the agent process. `run_as_user` and `run_as_group` default to `sandbox`. Root (`root` or `0`) is rejected. The agent also runs with seccomp filters that block dangerous system calls. | | `network_policies` | Dynamic | Controls network access for ordinary outbound traffic from the sandbox. Each block has a name, a list of endpoints (host, port, protocol, and optional rules), and a list of binaries allowed to use those endpoints.
Every outbound connection except `https://inference.local` goes through the proxy, which queries the {doc}`policy engine <../about/architecture>` with the destination and calling binary. A connection is allowed only when both match an entry in the same policy block.
For endpoints with `protocol: rest`, the proxy auto-detects TLS and terminates it so each HTTP request is checked against that endpoint's `rules` (method and path).
Endpoints without `protocol` allow the TCP stream through without inspecting payloads.
If no endpoint matches, the connection is denied. Configure managed inference separately through {doc}`../inference/configure`. | +## Baseline Filesystem Paths + +When a sandbox runs in proxy mode (the default), OpenShell automatically adds baseline filesystem paths required for the sandbox child process to function: `/usr`, `/lib`, `/etc`, `/var/log` (read-only) and `/sandbox`, `/tmp` (read-write). Paths like `/app` are included in the baseline set but are only added if they exist in the container image. + +This filtering prevents a missing baseline path from degrading Landlock enforcement. Without it, a single missing path could cause the entire Landlock ruleset to fail, leaving the sandbox with no filesystem restrictions at all. + +User-specified paths in your policy YAML are not pre-filtered. If you list a path that does not exist: + +- In `best_effort` mode, the path is skipped with a warning and remaining rules are still applied. +- In `hard_requirement` mode, sandbox startup fails immediately. + +This distinction means baseline system paths degrade gracefully while user-specified paths surface configuration errors. + ## Apply a Custom Policy Pass a policy YAML file when creating the sandbox: diff --git a/e2e/rust/tests/custom_image.rs b/e2e/rust/tests/custom_image.rs index 14fc3f47a..10cf99091 100644 --- a/e2e/rust/tests/custom_image.rs +++ b/e2e/rust/tests/custom_image.rs @@ -26,7 +26,9 @@ RUN groupadd -g 1000 sandbox && \ useradd -m -u 1000 -g sandbox sandbox # Write a marker file so we can verify this is our custom image. -RUN echo "custom-image-e2e-marker" > /opt/marker.txt +# Place under /etc (Landlock baseline read-only path) so the sandbox +# can read it when filesystem restrictions are properly enforced. +RUN echo "custom-image-e2e-marker" > /etc/marker.txt CMD ["sleep", "infinity"] "#; @@ -53,7 +55,7 @@ async fn sandbox_from_custom_dockerfile() { dockerfile_str, "--", "cat", - "/opt/marker.txt", + "/etc/marker.txt", ]) .await .expect("sandbox create from Dockerfile"); From 8c4b17221c76127e5c893559b80ae52759faab0d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vinicius=20Corr=C3=AAa?= Date: Mon, 30 Mar 2026 17:50:25 -0300 Subject: [PATCH 08/20] Missed input parameter (#645) Missed "--from openclaw" input parameter. Without this input sandbox container didn't install "openclaw": 'Stop with: openshell forward stop 18789 eager-dragonfly '/bin/bash: line 1: openclaw-start: command not found --- examples/openclaw.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/examples/openclaw.md b/examples/openclaw.md index c1ccd2cd4..f098e139e 100644 --- a/examples/openclaw.md +++ b/examples/openclaw.md @@ -3,7 +3,7 @@ ## Quick start ```sh -openshell sandbox create --forward 18789 -- openclaw-start +openshell sandbox create --forward 18789 --from openclaw -- openclaw-start ``` `openclaw-start` is a helper script pre-installed in the sandbox that runs the @@ -25,7 +25,7 @@ Note: you will need use the auth token present in the bootstrapping process to c ### Create the sandbox ```sh -openshell sandbox create --forward 18789 +openshell sandbox create --forward 18789 --from openclaw ``` Inside the sandbox, run the onboarding wizard and start the gateway: From e8950e624c7e98e60e624d25e7d57d0003a8b61c Mon Sep 17 00:00:00 2001 From: "John T. Myers" <9696606+johntmyers@users.noreply.github.com> Date: Mon, 30 Mar 2026 13:53:12 -0700 Subject: [PATCH 09/20] feat(sandbox): add L7 query parameter matchers (#617) * feat(sandbox): add L7 query parameter matchers Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com> * fix(sandbox): decode + as space in query params and validate glob syntax Three improvements from PR #617 review: 1. Decode + as space in query string values per the application/x-www-form-urlencoded convention. This matches Python's urllib.parse, JavaScript's URLSearchParams, Go's url.ParseQuery, and most HTTP frameworks. Literal + should be sent as %2B. 2. Add glob pattern syntax validation (warnings) for query matchers. Checks for unclosed brackets and braces in glob/any patterns. These are warnings (not errors) because OPA's glob.match is forgiving, but they surface likely typos during policy loading. 3. Add missing test cases: empty query values, keys without values, unicode after percent-decoding, empty query strings, and literal + via %2B encoding. * fix(sandbox): add missing query_params field in forward proxy L7 request info * style(sandbox): fix formatting in proxy L7 query param parsing --------- Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com> Co-authored-by: John Myers --- architecture/sandbox.md | 6 +- architecture/security-policy.md | 8 +- crates/openshell-policy/src/lib.rs | 94 ++++- .../data/sandbox-policy.rego | 50 +++ crates/openshell-sandbox/src/l7/mod.rs | 351 +++++++++++++++++- crates/openshell-sandbox/src/l7/provider.rs | 3 + crates/openshell-sandbox/src/l7/relay.rs | 3 + crates/openshell-sandbox/src/l7/rest.rs | 202 +++++++++- .../src/mechanistic_mapper.rs | 1 + crates/openshell-sandbox/src/opa.rs | 209 ++++++++++- crates/openshell-sandbox/src/proxy.rs | 5 +- docs/reference/policy-schema.md | 6 + docs/sandboxes/policies.md | 26 ++ e2e/python/test_sandbox_policy.py | 246 ++++++++++++ proto/sandbox.proto | 12 + 15 files changed, 1202 insertions(+), 20 deletions(-) diff --git a/architecture/sandbox.md b/architecture/sandbox.md index bfc71ba31..333cef5ea 100644 --- a/architecture/sandbox.md +++ b/architecture/sandbox.md @@ -968,7 +968,7 @@ flowchart LR | `EnforcementMode` | `Audit`, `Enforce` | What to do on L7 deny (log-only vs block) | | `L7EndpointConfig` | `{ protocol, tls, enforcement }` | Per-endpoint L7 configuration | | `L7Decision` | `{ allowed, reason, matched_rule }` | Result of L7 evaluation | -| `L7RequestInfo` | `{ action, target }` | HTTP method + path for policy evaluation | +| `L7RequestInfo` | `{ action, target, query_params }` | HTTP method, path, and decoded query multimap for policy evaluation | ### Access presets @@ -1047,7 +1047,7 @@ This enables credential injection on all HTTPS endpoints automatically, without Implements `L7Provider` for HTTP/1.1: -- **`parse_request()`**: Reads up to 16 KiB of headers, parses the request line (method, path), determines body framing from `Content-Length` or `Transfer-Encoding: chunked` headers. Returns `L7Request` with raw header bytes (may include overflow body bytes). +- **`parse_request()`**: Reads up to 16 KiB of headers, parses the request line (method, path), decodes query parameters into a multimap, determines body framing from `Content-Length` or `Transfer-Encoding: chunked` headers. Returns `L7Request` with raw header bytes (may include overflow body bytes). - **`relay()`**: Forwards request headers and body to upstream (handling Content-Length, chunked, and no-body cases), then reads and relays the full response back to the client. @@ -1060,7 +1060,7 @@ 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`, plus the CONNECT-level context (host, port, binary, ancestors, cmdline) +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 diff --git a/architecture/security-policy.md b/architecture/security-policy.md index 8b7b61d21..555ba67a5 100644 --- a/architecture/security-policy.md +++ b/architecture/security-policy.md @@ -467,9 +467,14 @@ rules: - allow: method: GET path: "/repos/**" + query: + per_page: "1*" - allow: method: POST path: "/repos/*/issues" + query: + labels: + any: ["bug*", "p1*"] ``` #### `L7Allow` @@ -479,8 +484,9 @@ rules: | `method` | `string` | HTTP method: `GET`, `HEAD`, `POST`, `PUT`, `DELETE`, `PATCH`, `OPTIONS`, or `*` (any). Case-insensitive matching. | | `path` | `string` | URL path glob pattern: `**` matches everything, otherwise `glob.match` with `/` delimiter. | | `command` | `string` | SQL command: `SELECT`, `INSERT`, `UPDATE`, `DELETE`, or `*` (any). Case-insensitive matching. For `protocol: sql` endpoints. | +| `query` | `map` | Optional REST query rules keyed by decoded query param name. Value is either a glob string (for example, `tag: "foo-*"`) or `{ any: ["foo-*", "bar-*"] }`. | -Method and command fields use `*` as wildcard for "any". Path patterns use `**` for "match everything" and standard glob patterns with `/` as a delimiter otherwise. See `sandbox-policy.rego` -- `method_matches()`, `path_matches()`, `command_matches()`. +Method and command fields use `*` as wildcard for "any". Path patterns use `**` for "match everything" and standard glob patterns with `/` as a delimiter otherwise. Query matching is case-sensitive and evaluates decoded values; when duplicate keys are present in the request, every value for that key must match the configured matcher. See `sandbox-policy.rego` -- `method_matches()`, `path_matches()`, `command_matches()`, `query_params_match()`. #### Access Presets diff --git a/crates/openshell-policy/src/lib.rs b/crates/openshell-policy/src/lib.rs index f1c15539e..7adb4dfda 100644 --- a/crates/openshell-policy/src/lib.rs +++ b/crates/openshell-policy/src/lib.rs @@ -15,8 +15,8 @@ use std::path::Path; use miette::{IntoDiagnostic, Result, WrapErr}; use openshell_core::proto::{ - FilesystemPolicy, L7Allow, L7Rule, LandlockPolicy, NetworkBinary, NetworkEndpoint, - NetworkPolicyRule, ProcessPolicy, SandboxPolicy, + FilesystemPolicy, L7Allow, L7QueryMatcher, L7Rule, LandlockPolicy, NetworkBinary, + NetworkEndpoint, NetworkPolicyRule, ProcessPolicy, SandboxPolicy, }; use serde::{Deserialize, Serialize}; @@ -120,6 +120,22 @@ struct L7AllowDef { path: String, #[serde(default, skip_serializing_if = "String::is_empty")] command: String, + #[serde(default, skip_serializing_if = "BTreeMap::is_empty")] + query: BTreeMap, +} + +#[derive(Debug, Serialize, Deserialize)] +#[serde(untagged)] +enum QueryMatcherDef { + Glob(String), + Any(QueryAnyDef), +} + +#[derive(Debug, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +struct QueryAnyDef { + #[serde(default, skip_serializing_if = "Vec::is_empty")] + any: Vec, } #[derive(Debug, Serialize, Deserialize)] @@ -176,6 +192,23 @@ fn to_proto(raw: PolicyFile) -> SandboxPolicy { method: r.allow.method, path: r.allow.path, command: r.allow.command, + query: r + .allow + .query + .into_iter() + .map(|(key, matcher)| { + let proto = match matcher { + QueryMatcherDef::Glob(glob) => { + L7QueryMatcher { glob, any: vec![] } + } + QueryMatcherDef::Any(any) => L7QueryMatcher { + glob: String::new(), + any: any.any, + }, + }; + (key, proto) + }) + .collect(), }), }) .collect(), @@ -275,6 +308,20 @@ fn from_proto(policy: &SandboxPolicy) -> PolicyFile { method: a.method, path: a.path, command: a.command, + query: a + .query + .into_iter() + .map(|(key, matcher)| { + let yaml_matcher = if !matcher.any.is_empty() { + QueryMatcherDef::Any(QueryAnyDef { + any: matcher.any, + }) + } else { + QueryMatcherDef::Glob(matcher.glob) + }; + (key, yaml_matcher) + }) + .collect(), }, } }) @@ -754,6 +801,49 @@ network_policies: assert_eq!(rule.binaries[0].path, "/usr/bin/curl"); } + #[test] + fn parse_l7_query_matchers_and_round_trip() { + let yaml = r#" +version: 1 +network_policies: + query_test: + name: query_test + endpoints: + - host: api.example.com + port: 8080 + protocol: rest + rules: + - allow: + method: GET + path: /download + query: + slug: "my-*" + tag: + any: ["foo-*", "bar-*"] + binaries: + - path: /usr/bin/curl +"#; + let proto = parse_sandbox_policy(yaml).expect("parse failed"); + let allow = proto.network_policies["query_test"].endpoints[0].rules[0] + .allow + .as_ref() + .expect("allow"); + assert_eq!(allow.query["slug"].glob, "my-*"); + assert_eq!(allow.query["slug"].any, Vec::::new()); + assert_eq!(allow.query["tag"].any, vec!["foo-*", "bar-*"]); + assert!(allow.query["tag"].glob.is_empty()); + + let yaml_out = serialize_sandbox_policy(&proto).expect("serialize failed"); + let proto_round_trip = parse_sandbox_policy(&yaml_out).expect("re-parse failed"); + let allow_round_trip = proto_round_trip.network_policies["query_test"].endpoints[0].rules + [0] + .allow + .as_ref() + .expect("allow"); + assert_eq!(allow_round_trip.query["slug"].glob, "my-*"); + assert_eq!(allow_round_trip.query["tag"].any, vec!["foo-*", "bar-*"]); + } + #[test] fn parse_rejects_unknown_fields() { let yaml = "version: 1\nbogus_field: true\n"; diff --git a/crates/openshell-sandbox/data/sandbox-policy.rego b/crates/openshell-sandbox/data/sandbox-policy.rego index 1544dfe55..0a7a33888 100644 --- a/crates/openshell-sandbox/data/sandbox-policy.rego +++ b/crates/openshell-sandbox/data/sandbox-policy.rego @@ -208,6 +208,7 @@ request_allowed_for_endpoint(request, endpoint) if { rule.allow.method method_matches(request.method, rule.allow.method) path_matches(request.path, rule.allow.path) + query_params_match(request, rule) } # --- L7 rule matching: SQL command --- @@ -235,6 +236,55 @@ path_matches(actual, pattern) if { glob.match(pattern, ["/"], actual) } +# Query matching: +# - If no query rules are configured, allow any query params. +# - For configured keys, all request values for that key must match. +# - Matcher shape supports either `glob` or `any`. +query_params_match(request, rule) if { + query_rules := object.get(rule.allow, "query", {}) + not query_mismatch(request, query_rules) +} + +query_mismatch(request, query_rules) if { + some key + matcher := query_rules[key] + not query_key_matches(request, key, matcher) +} + +query_key_matches(request, key, matcher) if { + request_query := object.get(request, "query_params", {}) + values := object.get(request_query, key, null) + values != null + count(values) > 0 + not query_value_mismatch(values, matcher) +} + +query_value_mismatch(values, matcher) if { + some i + value := values[i] + not query_value_matches(value, matcher) +} + +query_value_matches(value, matcher) if { + is_string(matcher) + glob.match(matcher, [], value) +} + +query_value_matches(value, matcher) if { + is_object(matcher) + glob_pattern := object.get(matcher, "glob", "") + glob_pattern != "" + glob.match(glob_pattern, [], value) +} + +query_value_matches(value, matcher) if { + is_object(matcher) + any_patterns := object.get(matcher, "any", []) + count(any_patterns) > 0 + some i + glob.match(any_patterns[i], [], value) +} + # SQL command matching: "*" matches any; otherwise case-insensitive. command_matches(_, "*") if true diff --git a/crates/openshell-sandbox/src/l7/mod.rs b/crates/openshell-sandbox/src/l7/mod.rs index 09e547885..880b6fd9e 100644 --- a/crates/openshell-sandbox/src/l7/mod.rs +++ b/crates/openshell-sandbox/src/l7/mod.rs @@ -76,6 +76,8 @@ pub struct L7RequestInfo { pub action: String, /// Target: URL path for REST, or empty for SQL. pub target: String, + /// Decoded query parameter multimap for REST requests. + pub query_params: std::collections::HashMap>, } /// Parse an L7 endpoint config from a regorus Value (returned by Rego query). @@ -144,6 +146,49 @@ fn get_object_str(val: ®orus::Value, key: &str) -> Option { } } +/// Check a glob pattern for obvious syntax issues. +/// +/// Returns `Some(warning_message)` if the pattern looks malformed. +/// OPA's `glob.match` is forgiving, so these are warnings (not errors) +/// to surface likely typos without blocking policy loading. +fn check_glob_syntax(pattern: &str) -> Option { + let mut bracket_depth: i32 = 0; + for c in pattern.chars() { + match c { + '[' => bracket_depth += 1, + ']' => { + if bracket_depth == 0 { + return Some(format!("glob pattern '{pattern}' has unmatched ']'")); + } + bracket_depth -= 1; + } + _ => {} + } + } + if bracket_depth > 0 { + return Some(format!("glob pattern '{pattern}' has unclosed '['")); + } + + let mut brace_depth: i32 = 0; + for c in pattern.chars() { + match c { + '{' => brace_depth += 1, + '}' => { + if brace_depth == 0 { + return Some(format!("glob pattern '{pattern}' has unmatched '}}'")); + } + brace_depth -= 1; + } + _ => {} + } + } + if brace_depth > 0 { + return Some(format!("glob pattern '{pattern}' has unclosed '{{'")); + } + + None +} + /// Validate L7 policy configuration in the loaded OPA data. /// /// Returns a list of errors and warnings. Errors should prevent sandbox startup; @@ -279,7 +324,7 @@ pub fn validate_l7_policies(data_json: &serde_json::Value) -> (Vec, Vec< "GET", "HEAD", "POST", "PUT", "DELETE", "PATCH", "OPTIONS", "*", ]; if let Some(rules) = ep.get("rules").and_then(|v| v.as_array()) { - for rule in rules { + for (rule_idx, rule) in rules.iter().enumerate() { if let Some(method) = rule .get("allow") .and_then(|a| a.get("method")) @@ -291,6 +336,110 @@ pub fn validate_l7_policies(data_json: &serde_json::Value) -> (Vec, Vec< "{loc}: Unknown HTTP method '{method}'. Standard methods: GET, HEAD, POST, PUT, DELETE, PATCH, OPTIONS." )); } + + let Some(query) = rule + .get("allow") + .and_then(|a| a.get("query")) + .filter(|v| !v.is_null()) + else { + continue; + }; + + let Some(query_obj) = query.as_object() else { + errors.push(format!( + "{loc}.rules[{rule_idx}].allow.query: expected map of query matchers" + )); + continue; + }; + + for (param, matcher) in query_obj { + if let Some(glob_str) = matcher.as_str() { + if let Some(warning) = check_glob_syntax(glob_str) { + warnings.push(format!( + "{loc}.rules[{rule_idx}].allow.query.{param}: {warning}" + )); + } + continue; + } + + let Some(matcher_obj) = matcher.as_object() else { + errors.push(format!( + "{loc}.rules[{rule_idx}].allow.query.{param}: expected string glob or object with `any`" + )); + continue; + }; + + let has_any = matcher_obj.get("any").is_some(); + let has_glob = matcher_obj.get("glob").is_some(); + let has_unknown = matcher_obj.keys().any(|k| k != "any" && k != "glob"); + if has_unknown { + errors.push(format!( + "{loc}.rules[{rule_idx}].allow.query.{param}: unknown matcher keys; only `glob` or `any` are supported" + )); + continue; + } + + if has_glob && has_any { + errors.push(format!( + "{loc}.rules[{rule_idx}].allow.query.{param}: matcher cannot specify both `glob` and `any`" + )); + continue; + } + + if !has_glob && !has_any { + errors.push(format!( + "{loc}.rules[{rule_idx}].allow.query.{param}: object matcher requires `glob` string or non-empty `any` list" + )); + continue; + } + + if has_glob { + match matcher_obj.get("glob").and_then(|v| v.as_str()) { + None => { + errors.push(format!( + "{loc}.rules[{rule_idx}].allow.query.{param}.glob: expected glob string" + )); + } + Some(g) => { + if let Some(warning) = check_glob_syntax(g) { + warnings.push(format!( + "{loc}.rules[{rule_idx}].allow.query.{param}.glob: {warning}" + )); + } + } + } + continue; + } + + let any = matcher_obj.get("any").and_then(|v| v.as_array()); + let Some(any) = any else { + errors.push(format!( + "{loc}.rules[{rule_idx}].allow.query.{param}.any: expected array of glob strings" + )); + continue; + }; + + if any.is_empty() { + errors.push(format!( + "{loc}.rules[{rule_idx}].allow.query.{param}.any: list must not be empty" + )); + continue; + } + + if any.iter().any(|v| v.as_str().is_none()) { + errors.push(format!( + "{loc}.rules[{rule_idx}].allow.query.{param}.any: all values must be strings" + )); + } + + for item in any.iter().filter_map(|v| v.as_str()) { + if let Some(warning) = check_glob_syntax(item) { + warnings.push(format!( + "{loc}.rules[{rule_idx}].allow.query.{param}.any: {warning}" + )); + } + } + } } } } @@ -780,4 +929,204 @@ mod tests { "should have no tls warnings with auto-detect: {warnings:?}" ); } + + #[test] + fn validate_query_any_requires_non_empty_array() { + let data = serde_json::json!({ + "network_policies": { + "test": { + "endpoints": [{ + "host": "api.example.com", + "port": 8080, + "protocol": "rest", + "rules": [{ + "allow": { + "method": "GET", + "path": "/download", + "query": { + "tag": { "any": [] } + } + } + }] + }], + "binaries": [] + } + } + }); + let (errors, _warnings) = validate_l7_policies(&data); + assert!( + errors.iter().any(|e| e.contains("allow.query.tag.any")), + "expected query any validation error, got: {errors:?}" + ); + } + + #[test] + fn validate_query_object_rejects_unknown_keys() { + let data = serde_json::json!({ + "network_policies": { + "test": { + "endpoints": [{ + "host": "api.example.com", + "port": 8080, + "protocol": "rest", + "rules": [{ + "allow": { + "method": "GET", + "path": "/download", + "query": { + "tag": { "mode": "foo-*" } + } + } + }] + }], + "binaries": [] + } + } + }); + let (errors, _warnings) = validate_l7_policies(&data); + assert!( + errors.iter().any(|e| e.contains("unknown matcher keys")), + "expected unknown query matcher key error, got: {errors:?}" + ); + } + + #[test] + fn validate_query_glob_warns_on_unclosed_bracket() { + let data = serde_json::json!({ + "network_policies": { + "test": { + "endpoints": [{ + "host": "api.example.com", + "port": 8080, + "protocol": "rest", + "rules": [{ + "allow": { + "method": "GET", + "path": "/download", + "query": { + "tag": "[unclosed" + } + } + }] + }], + "binaries": [] + } + } + }); + let (errors, warnings) = validate_l7_policies(&data); + assert!( + errors.is_empty(), + "malformed glob should warn, not error: {errors:?}" + ); + assert!( + warnings + .iter() + .any(|w| w.contains("unclosed '['") && w.contains("allow.query.tag")), + "expected glob syntax warning, got: {warnings:?}" + ); + } + + #[test] + fn validate_query_glob_warns_on_unclosed_brace() { + let data = serde_json::json!({ + "network_policies": { + "test": { + "endpoints": [{ + "host": "api.example.com", + "port": 8080, + "protocol": "rest", + "rules": [{ + "allow": { + "method": "GET", + "path": "/download", + "query": { + "format": { "glob": "{json,xml" } + } + } + }] + }], + "binaries": [] + } + } + }); + let (errors, warnings) = validate_l7_policies(&data); + assert!( + errors.is_empty(), + "malformed glob should warn, not error: {errors:?}" + ); + assert!( + warnings + .iter() + .any(|w| w.contains("unclosed '{'") && w.contains("allow.query.format.glob")), + "expected glob syntax warning, got: {warnings:?}" + ); + } + + #[test] + fn validate_query_any_warns_on_malformed_glob_item() { + let data = serde_json::json!({ + "network_policies": { + "test": { + "endpoints": [{ + "host": "api.example.com", + "port": 8080, + "protocol": "rest", + "rules": [{ + "allow": { + "method": "GET", + "path": "/download", + "query": { + "tag": { "any": ["valid-*", "[bad"] } + } + } + }] + }], + "binaries": [] + } + } + }); + let (errors, warnings) = validate_l7_policies(&data); + assert!( + errors.is_empty(), + "malformed glob in any should warn, not error: {errors:?}" + ); + assert!( + warnings + .iter() + .any(|w| w.contains("unclosed '['") && w.contains("allow.query.tag.any")), + "expected glob syntax warning for any item, got: {warnings:?}" + ); + } + + #[test] + fn validate_query_string_and_any_matchers_are_accepted() { + let data = serde_json::json!({ + "network_policies": { + "test": { + "endpoints": [{ + "host": "api.example.com", + "port": 8080, + "protocol": "rest", + "rules": [{ + "allow": { + "method": "GET", + "path": "/download", + "query": { + "slug": "my-*", + "tag": { "any": ["foo-*", "bar-*"] }, + "owner": { "glob": "org-*" } + } + } + }] + }], + "binaries": [] + } + } + }); + let (errors, _warnings) = validate_l7_policies(&data); + assert!( + errors.is_empty(), + "valid query matcher shapes should not error: {errors:?}" + ); + } } diff --git a/crates/openshell-sandbox/src/l7/provider.rs b/crates/openshell-sandbox/src/l7/provider.rs index a9bf8bf5f..df0dfb292 100644 --- a/crates/openshell-sandbox/src/l7/provider.rs +++ b/crates/openshell-sandbox/src/l7/provider.rs @@ -10,6 +10,7 @@ //! works for both plaintext TCP and TLS-terminated connections. use miette::Result; +use std::collections::HashMap; use std::future::Future; use tokio::io::{AsyncRead, AsyncWrite}; @@ -31,6 +32,8 @@ pub struct L7Request { pub action: String, /// Target: URL path for REST, empty for SQL. pub target: String, + /// Decoded query parameter multimap for REST requests. + pub query_params: HashMap>, /// Raw request header bytes (request line + headers for HTTP, message for SQL). /// May include overflow body bytes read during header parsing. pub raw_header: Vec, diff --git a/crates/openshell-sandbox/src/l7/relay.rs b/crates/openshell-sandbox/src/l7/relay.rs index 88f9ce8a6..e0ad2a18c 100644 --- a/crates/openshell-sandbox/src/l7/relay.rs +++ b/crates/openshell-sandbox/src/l7/relay.rs @@ -108,6 +108,7 @@ where let request_info = L7RequestInfo { action: req.action.clone(), target: req.target.clone(), + query_params: req.query_params.clone(), }; // Evaluate L7 policy via Rego @@ -127,6 +128,7 @@ where l7_protocol = "rest", l7_action = %request_info.action, l7_target = %request_info.target, + l7_query_params = ?request_info.query_params, l7_decision = decision_str, l7_deny_reason = %reason, "L7_REQUEST", @@ -198,6 +200,7 @@ pub fn evaluate_l7_request( "request": { "method": request.action, "path": request.target, + "query_params": request.query_params.clone(), } }); diff --git a/crates/openshell-sandbox/src/l7/rest.rs b/crates/openshell-sandbox/src/l7/rest.rs index f47f01bdc..da453ce16 100644 --- a/crates/openshell-sandbox/src/l7/rest.rs +++ b/crates/openshell-sandbox/src/l7/rest.rs @@ -10,6 +10,7 @@ use crate::l7::provider::{BodyLength, L7Provider, L7Request}; use crate::secrets::rewrite_http_header_block; use miette::{IntoDiagnostic, Result, miette}; +use std::collections::HashMap; use tokio::io::{AsyncRead, AsyncReadExt, AsyncWrite, AsyncWriteExt}; use tracing::debug; @@ -116,7 +117,7 @@ async fn parse_http_request(client: &mut C) -> Result(client: &mut C) -> Result Result<(String, HashMap>)> { + match target.split_once('?') { + Some((path, query)) => Ok((path.to_string(), parse_query_params(query)?)), + None => Ok((target.to_string(), HashMap::new())), + } +} + +fn parse_query_params(query: &str) -> Result>> { + let mut params: HashMap> = HashMap::new(); + if query.is_empty() { + return Ok(params); + } + + for pair in query.split('&') { + if pair.is_empty() { + continue; + } + + let (raw_key, raw_value) = match pair.split_once('=') { + Some((key, value)) => (key, value), + None => (pair, ""), + }; + let key = decode_query_component(raw_key)?; + let value = decode_query_component(raw_value)?; + params.entry(key).or_default().push(value); + } + + Ok(params) +} + +/// Decode a single query string component (key or value). +/// +/// Handles both RFC 3986 percent-encoding (`%20` → space) and the +/// `application/x-www-form-urlencoded` convention (`+` → space). +/// Decoding `+` as space matches the behavior of Python's `urllib.parse`, +/// JavaScript's `URLSearchParams`, Go's `url.ParseQuery`, and most HTTP +/// frameworks. Callers that need a literal `+` should send `%2B`. +fn decode_query_component(input: &str) -> Result { + let bytes = input.as_bytes(); + let mut decoded = Vec::with_capacity(bytes.len()); + let mut i = 0; + + while i < bytes.len() { + if bytes[i] == b'+' { + decoded.push(b' '); + i += 1; + continue; + } + + if bytes[i] != b'%' { + decoded.push(bytes[i]); + i += 1; + continue; + } + + if i + 2 >= bytes.len() { + return Err(miette!("Invalid percent-encoding in query component")); + } + + let hi = decode_hex_nibble(bytes[i + 1]) + .ok_or_else(|| miette!("Invalid percent-encoding in query component"))?; + let lo = decode_hex_nibble(bytes[i + 2]) + .ok_or_else(|| miette!("Invalid percent-encoding in query component"))?; + decoded.push((hi << 4) | lo); + i += 3; + } + + String::from_utf8(decoded).map_err(|_| miette!("Query component is not valid UTF-8")) +} + +fn decode_hex_nibble(byte: u8) -> Option { + match byte { + b'0'..=b'9' => Some(byte - b'0'), + b'a'..=b'f' => Some(byte - b'a' + 10), + b'A'..=b'F' => Some(byte - b'A' + 10), + _ => None, + } +} + /// Forward an allowed HTTP request to upstream and relay the response back. /// /// Returns `true` if the upstream connection is reusable, `false` if consumed. @@ -689,6 +771,92 @@ mod tests { } } + #[test] + fn parse_target_query_parses_duplicate_values() { + let (path, query) = parse_target_query("/download?tag=a&tag=b").expect("parse"); + assert_eq!(path, "/download"); + assert_eq!( + query.get("tag").cloned(), + Some(vec!["a".into(), "b".into()]) + ); + } + + #[test] + fn parse_target_query_decodes_percent_and_plus() { + let (path, query) = parse_target_query("/download?slug=my%2Fskill&name=Foo+Bar").unwrap(); + assert_eq!(path, "/download"); + assert_eq!( + query.get("slug").cloned(), + Some(vec!["my/skill".to_string()]) + ); + // `+` is decoded as space per application/x-www-form-urlencoded. + // Literal `+` should be sent as `%2B`. + assert_eq!( + query.get("name").cloned(), + Some(vec!["Foo Bar".to_string()]) + ); + } + + #[test] + fn parse_target_query_literal_plus_via_percent_encoding() { + let (_path, query) = parse_target_query("/search?q=a%2Bb").unwrap(); + assert_eq!( + query.get("q").cloned(), + Some(vec!["a+b".to_string()]), + "%2B should decode to literal +" + ); + } + + #[test] + fn parse_target_query_empty_value() { + let (_path, query) = parse_target_query("/api?tag=").unwrap(); + assert_eq!( + query.get("tag").cloned(), + Some(vec!["".to_string()]), + "key with empty value should produce empty string" + ); + } + + #[test] + fn parse_target_query_key_without_value() { + let (_path, query) = parse_target_query("/api?verbose").unwrap(); + assert_eq!( + query.get("verbose").cloned(), + Some(vec!["".to_string()]), + "key without = should produce empty string value" + ); + } + + #[test] + fn parse_target_query_unicode_after_decoding() { + // "café" = c a f %C3%A9 + let (_path, query) = parse_target_query("/search?q=caf%C3%A9").unwrap(); + assert_eq!( + query.get("q").cloned(), + Some(vec!["café".to_string()]), + "percent-encoded UTF-8 should decode correctly" + ); + } + + #[test] + fn parse_target_query_empty_query_string() { + let (path, query) = parse_target_query("/api?").unwrap(); + assert_eq!(path, "/api"); + assert!( + query.is_empty(), + "empty query after ? should produce empty map" + ); + } + + #[test] + fn parse_target_query_rejects_malformed_percent_encoding() { + let err = parse_target_query("/download?slug=bad%2").expect_err("expected parse error"); + assert!( + err.to_string().contains("percent-encoding"), + "unexpected error: {err}" + ); + } + /// SEC-009: Reject requests with both Content-Length and Transfer-Encoding /// to prevent CL/TE request smuggling (RFC 7230 Section 3.3.3). #[test] @@ -807,6 +975,32 @@ mod tests { assert!(result.is_err(), "Must reject unsupported HTTP version"); } + #[tokio::test] + async fn parse_http_request_splits_path_and_query_params() { + let (mut client, mut writer) = tokio::io::duplex(4096); + tokio::spawn(async move { + writer + .write_all( + b"GET /download?slug=my%2Fskill&tag=foo&tag=bar HTTP/1.1\r\nHost: x\r\n\r\n", + ) + .await + .unwrap(); + }); + let req = parse_http_request(&mut client) + .await + .expect("request should parse") + .expect("request should exist"); + assert_eq!(req.target, "/download"); + assert_eq!( + req.query_params.get("slug").cloned(), + Some(vec!["my/skill".to_string()]) + ); + assert_eq!( + req.query_params.get("tag").cloned(), + Some(vec!["foo".to_string(), "bar".to_string()]) + ); + } + /// Regression test: two pipelined requests in a single write must be /// parsed independently. Before the fix, the 1024-byte `read()` buffer /// could capture bytes from the second request, which were forwarded @@ -831,6 +1025,7 @@ mod tests { .expect("expected first request"); assert_eq!(first.action, "GET"); assert_eq!(first.target, "/allowed"); + assert!(first.query_params.is_empty()); assert_eq!( first.raw_header, b"GET /allowed HTTP/1.1\r\nHost: example.com\r\n\r\n", "raw_header must contain only the first request's headers" @@ -842,6 +1037,7 @@ mod tests { .expect("expected second request"); assert_eq!(second.action, "POST"); assert_eq!(second.target, "/blocked"); + assert!(second.query_params.is_empty()); } #[test] @@ -1194,7 +1390,7 @@ mod tests { /// to the upstream API, causing 401 Unauthorized errors. #[tokio::test] async fn relay_request_with_resolver_rewrites_credential_placeholders() { - let provider_env: std::collections::HashMap = [( + let provider_env: HashMap = [( "NVIDIA_API_KEY".to_string(), "nvapi-real-secret-key".to_string(), )] @@ -1210,6 +1406,7 @@ mod tests { let req = L7Request { action: "POST".to_string(), target: "/v1/chat/completions".to_string(), + query_params: HashMap::new(), raw_header: format!( "POST /v1/chat/completions HTTP/1.1\r\n\ Host: integrate.api.nvidia.com\r\n\ @@ -1293,6 +1490,7 @@ mod tests { let req = L7Request { action: "POST".to_string(), target: "/v1/chat/completions".to_string(), + query_params: HashMap::new(), raw_header: format!( "POST /v1/chat/completions HTTP/1.1\r\n\ Host: integrate.api.nvidia.com\r\n\ diff --git a/crates/openshell-sandbox/src/mechanistic_mapper.rs b/crates/openshell-sandbox/src/mechanistic_mapper.rs index e5ae64977..4fe90d084 100644 --- a/crates/openshell-sandbox/src/mechanistic_mapper.rs +++ b/crates/openshell-sandbox/src/mechanistic_mapper.rs @@ -337,6 +337,7 @@ fn build_l7_rules(samples: &HashMap<(String, String), u32>) -> Vec { method: method.clone(), path: generalised, command: String::new(), + query: HashMap::new(), }), }); } diff --git a/crates/openshell-sandbox/src/opa.rs b/crates/openshell-sandbox/src/opa.rs index cd2931b35..f1df12ff4 100644 --- a/crates/openshell-sandbox/src/opa.rs +++ b/crates/openshell-sandbox/src/opa.rs @@ -667,13 +667,35 @@ fn proto_to_opa_data_json(proto: &ProtoSandboxPolicy) -> String { .iter() .map(|r| { let a = r.allow.as_ref(); - serde_json::json!({ - "allow": { - "method": a.map_or("", |a| &a.method), - "path": a.map_or("", |a| &a.path), - "command": a.map_or("", |a| &a.command), - } - }) + let mut allow = serde_json::json!({ + "method": a.map_or("", |a| &a.method), + "path": a.map_or("", |a| &a.path), + "command": a.map_or("", |a| &a.command), + }); + let query: serde_json::Map = a + .map(|allow| { + allow + .query + .iter() + .map(|(key, matcher)| { + let mut matcher_json = serde_json::json!({}); + if !matcher.glob.is_empty() { + matcher_json["glob"] = + matcher.glob.clone().into(); + } + if !matcher.any.is_empty() { + matcher_json["any"] = + matcher.any.clone().into(); + } + (key.clone(), matcher_json) + }) + .collect() + }) + .unwrap_or_default(); + if !query.is_empty() { + allow["query"] = query.into(); + } + serde_json::json!({ "allow": allow }) }) .collect(); ep["rules"] = rules.into(); @@ -714,8 +736,9 @@ mod tests { use super::*; use openshell_core::proto::{ - FilesystemPolicy as ProtoFs, NetworkBinary, NetworkEndpoint, NetworkPolicyRule, - ProcessPolicy as ProtoProc, SandboxPolicy as ProtoSandboxPolicy, + FilesystemPolicy as ProtoFs, L7Allow, L7QueryMatcher, L7Rule, NetworkBinary, + NetworkEndpoint, NetworkPolicyRule, ProcessPolicy as ProtoProc, + SandboxPolicy as ProtoSandboxPolicy, }; const TEST_POLICY: &str = include_str!("../data/sandbox-policy.rego"); @@ -1337,6 +1360,27 @@ network_policies: access: full binaries: - { path: /usr/bin/curl } + query_api: + name: query_api + endpoints: + - host: api.query.com + port: 8080 + protocol: rest + enforcement: enforce + rules: + - allow: + method: GET + path: "/download" + query: + tag: "foo-*" + - allow: + method: GET + path: "/search" + query: + tag: + any: ["foo-*", "bar-*"] + binaries: + - { path: /usr/bin/curl } l4_only: name: l4_only endpoints: @@ -1359,6 +1403,16 @@ process: } fn l7_input(host: &str, port: u16, method: &str, path: &str) -> serde_json::Value { + l7_input_with_query(host, port, method, path, serde_json::json!({})) + } + + fn l7_input_with_query( + host: &str, + port: u16, + method: &str, + path: &str, + query_params: serde_json::Value, + ) -> serde_json::Value { serde_json::json!({ "network": { "host": host, "port": port }, "exec": { @@ -1368,7 +1422,8 @@ process: }, "request": { "method": method, - "path": path + "path": path, + "query_params": query_params } }) } @@ -1472,6 +1527,140 @@ process: assert!(eval_l7(&engine, &input)); } + #[test] + fn l7_query_glob_allows_matching_duplicate_values() { + let engine = l7_engine(); + let input = l7_input_with_query( + "api.query.com", + 8080, + "GET", + "/download", + serde_json::json!({ + "tag": ["foo-a", "foo-b"], + "extra": ["ignored"], + }), + ); + assert!(eval_l7(&engine, &input)); + } + + #[test] + fn l7_query_glob_denies_on_mismatched_duplicate_value() { + let engine = l7_engine(); + let input = l7_input_with_query( + "api.query.com", + 8080, + "GET", + "/download", + serde_json::json!({ + "tag": ["foo-a", "evil"], + }), + ); + assert!(!eval_l7(&engine, &input)); + } + + #[test] + fn l7_query_any_allows_if_every_value_matches_any_pattern() { + let engine = l7_engine(); + let input = l7_input_with_query( + "api.query.com", + 8080, + "GET", + "/search", + serde_json::json!({ + "tag": ["foo-a", "bar-b"], + }), + ); + assert!(eval_l7(&engine, &input)); + } + + #[test] + fn l7_query_missing_required_key_denied() { + let engine = l7_engine(); + let input = l7_input_with_query( + "api.query.com", + 8080, + "GET", + "/download", + serde_json::json!({}), + ); + assert!(!eval_l7(&engine, &input)); + } + + #[test] + fn l7_query_rules_from_proto_are_enforced() { + let mut query = std::collections::HashMap::new(); + query.insert( + "tag".to_string(), + L7QueryMatcher { + glob: "foo-*".to_string(), + any: vec![], + }, + ); + + let mut network_policies = std::collections::HashMap::new(); + network_policies.insert( + "query_proto".to_string(), + NetworkPolicyRule { + name: "query_proto".to_string(), + endpoints: vec![NetworkEndpoint { + host: "api.proto.com".to_string(), + port: 8080, + protocol: "rest".to_string(), + enforcement: "enforce".to_string(), + rules: vec![L7Rule { + allow: Some(L7Allow { + method: "GET".to_string(), + path: "/download".to_string(), + command: String::new(), + query, + }), + }], + ..Default::default() + }], + binaries: vec![NetworkBinary { + path: "/usr/bin/curl".to_string(), + ..Default::default() + }], + }, + ); + + let proto = ProtoSandboxPolicy { + version: 1, + filesystem: Some(ProtoFs { + include_workdir: true, + read_only: vec![], + read_write: vec![], + }), + landlock: Some(openshell_core::proto::LandlockPolicy { + compatibility: "best_effort".to_string(), + }), + process: Some(ProtoProc { + run_as_user: "sandbox".to_string(), + run_as_group: "sandbox".to_string(), + }), + network_policies, + }; + + let engine = OpaEngine::from_proto(&proto).expect("engine from proto"); + let allow_input = l7_input_with_query( + "api.proto.com", + 8080, + "GET", + "/download", + serde_json::json!({ "tag": ["foo-a"] }), + ); + assert!(eval_l7(&engine, &allow_input)); + + let deny_input = l7_input_with_query( + "api.proto.com", + 8080, + "GET", + "/download", + serde_json::json!({ "tag": ["evil"] }), + ); + assert!(!eval_l7(&engine, &deny_input)); + } + #[test] fn l7_no_request_on_l4_only_endpoint() { // L4-only endpoint should not match L7 allow_request diff --git a/crates/openshell-sandbox/src/proxy.rs b/crates/openshell-sandbox/src/proxy.rs index 088ec46a6..008877257 100644 --- a/crates/openshell-sandbox/src/proxy.rs +++ b/crates/openshell-sandbox/src/proxy.rs @@ -1838,9 +1838,12 @@ async fn handle_forward_proxy( secret_resolver: secret_resolver.clone(), }; + let (target_path, query_params) = crate::l7::rest::parse_target_query(&path) + .unwrap_or_else(|_| (path.clone(), std::collections::HashMap::new())); let request_info = crate::l7::L7RequestInfo { action: method.to_string(), - target: path.clone(), + target: target_path, + query_params, }; let (allowed, reason) = diff --git a/docs/reference/policy-schema.md b/docs/reference/policy-schema.md index 6916e8d0c..7ad317f36 100644 --- a/docs/reference/policy-schema.md +++ b/docs/reference/policy-schema.md @@ -196,6 +196,7 @@ Used when `access` is not set. Each rule explicitly allows a method and path com |---|---|---|---| | `allow.method` | string | Yes | HTTP method to allow (for example, `GET`, `POST`). | | `allow.path` | string | Yes | URL path pattern. Supports `*` and `**` glob syntax. | +| `allow.query` | map | No | Query parameter matchers keyed by decoded param name. Matcher value can be a glob string (`tag: "foo-*"`) or an object with `any` (`tag: { any: ["foo-*", "bar-*"] }`). | Example with rules: @@ -204,9 +205,14 @@ rules: - allow: method: GET path: /**/info/refs* + query: + service: "git-*" - allow: method: POST path: /**/git-upload-pack + query: + tag: + any: ["v1.*", "v2.*"] ``` ### Binary Object diff --git a/docs/sandboxes/policies.md b/docs/sandboxes/policies.md index fa5ed5d8b..3ec33af9e 100644 --- a/docs/sandboxes/policies.md +++ b/docs/sandboxes/policies.md @@ -269,6 +269,32 @@ Endpoints with `protocol: rest` enable HTTP request inspection. The proxy auto-d ::::: +### Query parameter matching + +REST rules can also constrain query parameter values: + +```yaml + download_api: + name: download_api + endpoints: + - host: api.example.com + port: 443 + protocol: rest + enforcement: enforce + rules: + - allow: + method: GET + path: "/api/v1/download" + query: + slug: "skill-*" + version: + any: ["1.*", "2.*"] + binaries: + - { path: /usr/bin/curl } +``` + +`query` matchers are case-sensitive and run on decoded values. If a request has duplicate keys (for example, `tag=a&tag=b`), every value for that key must match the configured glob(s). + ## Next Steps Explore related topics: diff --git a/e2e/python/test_sandbox_policy.py b/e2e/python/test_sandbox_policy.py index 1615bef6c..6a4bf5ed2 100644 --- a/e2e/python/test_sandbox_policy.py +++ b/e2e/python/test_sandbox_policy.py @@ -271,6 +271,90 @@ def fn(proxy_host, proxy_port, target_url): return fn +def _proxy_connect_then_http_with_server(): + """Return a closure that starts a local HTTP server and sends CONNECT+HTTP.""" + + def fn(proxy_host, proxy_port, target_host, target_port, method="GET", path="/"): + import json as _json + import socket + import threading + import time + from http.server import BaseHTTPRequestHandler, HTTPServer + + class Handler(BaseHTTPRequestHandler): + def do_GET(self): + self.send_response(200) + body = b"connect-server-ok" + self.send_header("Content-Length", str(len(body))) + self.end_headers() + self.wfile.write(body) + + def do_POST(self): + self.send_response(200) + body = b"connect-server-ok" + self.send_header("Content-Length", str(len(body))) + self.end_headers() + self.wfile.write(body) + + def log_message(self, *args): + pass + + srv = HTTPServer(("0.0.0.0", int(target_port)), Handler) + threading.Thread(target=srv.handle_request, daemon=True).start() + time.sleep(0.5) + + conn = socket.create_connection((proxy_host, int(proxy_port)), timeout=10) + try: + conn.sendall( + f"CONNECT {target_host}:{target_port} HTTP/1.1\r\nHost: {target_host}\r\n\r\n".encode() + ) + connect_resp = conn.recv(256).decode("latin1") + if "200" not in connect_resp: + return _json.dumps( + {"connect_status": connect_resp.strip(), "http_status": 0} + ) + + request = ( + f"{method} {path} HTTP/1.1\r\nHost: {target_host}\r\nConnection: close\r\n\r\n" + ) + conn.sendall(request.encode()) + + data = b"" + conn.settimeout(5) + try: + while True: + chunk = conn.recv(4096) + if not chunk: + break + data += chunk + except socket.timeout: + pass + + response = data.decode("latin1", errors="replace") + status_line = response.split("\r\n")[0] if response else "" + status_code = ( + int(status_line.split()[1]) if len(status_line.split()) >= 2 else 0 + ) + + header_end = response.find("\r\n\r\n") + headers_raw = response[:header_end] if header_end > 0 else "" + body = response[header_end + 4 :] if header_end > 0 else "" + + return _json.dumps( + { + "connect_status": connect_resp.strip(), + "http_status": status_code, + "headers": headers_raw, + "body": body, + } + ) + finally: + conn.close() + srv.server_close() + + return fn + + def test_policy_applies_to_exec_commands( sandbox: Callable[..., Sandbox], ) -> None: @@ -796,6 +880,8 @@ def test_ssrf_loopback_blocked_even_with_allowed_ips( # L7-T6: L7 deny response is valid JSON with expected fields # L7-T7: L7 request logging includes structured fields # L7-T8: Port 443 + protocol=rest without tls=terminate warns (L7 not evaluated) +# L7-T9: Query matcher glob/any allows and denies as expected +# L7-T10: Rule without query matcher allows any query params # ============================================================================= @@ -1102,6 +1188,166 @@ def test_l7_tls_log_fields( assert "l7_decision" in log +def test_l7_query_matchers_enforced( + sandbox: Callable[..., Sandbox], +) -> None: + """L7-T9: Query matcher glob/any allows and denies as expected.""" + policy = _base_policy( + network_policies={ + "query_api": sandbox_pb2.NetworkPolicyRule( + name="query_api", + endpoints=[ + sandbox_pb2.NetworkEndpoint( + host=_SANDBOX_IP, + port=_FORWARD_PROXY_PORT, + protocol="rest", + enforcement="enforce", + allowed_ips=["10.200.0.0/24"], + rules=[ + sandbox_pb2.L7Rule( + allow=sandbox_pb2.L7Allow( + method="GET", + path="/download", + query={ + "tag": sandbox_pb2.L7QueryMatcher(glob="foo-*"), + }, + ), + ), + sandbox_pb2.L7Rule( + allow=sandbox_pb2.L7Allow( + method="GET", + path="/search", + query={ + "tag": sandbox_pb2.L7QueryMatcher( + any=["foo-*", "bar-*"] + ), + }, + ), + ), + ], + ), + ], + binaries=[sandbox_pb2.NetworkBinary(path="/**")], + ), + }, + ) + spec = datamodel_pb2.SandboxSpec(policy=policy) + with sandbox(spec=spec, delete_on_exit=True) as sb: + allowed = sb.exec_python( + _proxy_connect_then_http_with_server(), + args=( + _PROXY_HOST, + _PROXY_PORT, + _SANDBOX_IP, + _FORWARD_PROXY_PORT, + "GET", + "/download?tag=foo-a&tag=foo-b", + ), + ) + assert allowed.exit_code == 0, allowed.stderr + allowed_resp = json.loads(allowed.stdout) + assert "200" in allowed_resp["connect_status"] + assert allowed_resp["http_status"] == 200 + assert "connect-server-ok" in allowed_resp["body"] + + denied = sb.exec_python( + _proxy_connect_then_http_with_server(), + args=( + _PROXY_HOST, + _PROXY_PORT, + _SANDBOX_IP, + _FORWARD_PROXY_PORT, + "GET", + "/download?tag=foo-a&tag=evil", + ), + ) + assert denied.exit_code == 0, denied.stderr + denied_resp = json.loads(denied.stdout) + assert denied_resp["http_status"] == 403 + assert "policy_denied" in denied_resp["body"] + + any_allowed = sb.exec_python( + _proxy_connect_then_http_with_server(), + args=( + _PROXY_HOST, + _PROXY_PORT, + _SANDBOX_IP, + _FORWARD_PROXY_PORT, + "GET", + "/search?tag=foo-a&tag=bar-b", + ), + ) + assert any_allowed.exit_code == 0, any_allowed.stderr + any_resp = json.loads(any_allowed.stdout) + assert any_resp["http_status"] == 200 + assert "connect-server-ok" in any_resp["body"] + + missing_required = sb.exec_python( + _proxy_connect_then_http_with_server(), + args=( + _PROXY_HOST, + _PROXY_PORT, + _SANDBOX_IP, + _FORWARD_PROXY_PORT, + "GET", + "/download?slug=skill-1", + ), + ) + assert missing_required.exit_code == 0, missing_required.stderr + missing_resp = json.loads(missing_required.stdout) + assert missing_resp["http_status"] == 403 + assert "policy_denied" in missing_resp["body"] + + +def test_l7_rule_without_query_matcher_allows_any_query_params( + sandbox: Callable[..., Sandbox], +) -> None: + """L7-T10: Rule without query matcher allows any query params.""" + policy = _base_policy( + network_policies={ + "query_optional": sandbox_pb2.NetworkPolicyRule( + name="query_optional", + endpoints=[ + sandbox_pb2.NetworkEndpoint( + host=_SANDBOX_IP, + port=_FORWARD_PROXY_PORT, + protocol="rest", + enforcement="enforce", + allowed_ips=["10.200.0.0/24"], + rules=[ + sandbox_pb2.L7Rule( + allow=sandbox_pb2.L7Allow( + method="GET", + path="/download", + ), + ), + ], + ), + ], + binaries=[sandbox_pb2.NetworkBinary(path="/**")], + ), + }, + ) + spec = datamodel_pb2.SandboxSpec(policy=policy) + with sandbox(spec=spec, delete_on_exit=True) as sb: + result = sb.exec_python( + _proxy_connect_then_http_with_server(), + args=( + _PROXY_HOST, + _PROXY_PORT, + _SANDBOX_IP, + _FORWARD_PROXY_PORT, + "GET", + "/download?tag=anything&slug=any-value", + ), + ) + assert result.exit_code == 0, result.stderr + resp = json.loads(result.stdout) + assert "200" in resp["connect_status"] + assert resp["http_status"] == 200 + assert "connect-server-ok" in resp["body"] + + # ============================================================================= # Live policy update + log streaming tests # diff --git a/proto/sandbox.proto b/proto/sandbox.proto index a96ca33fd..61948a527 100644 --- a/proto/sandbox.proto +++ b/proto/sandbox.proto @@ -100,6 +100,18 @@ message L7Allow { string path = 2; // SQL command (SQL): SELECT, INSERT, etc. or "*" for any. string command = 3; + // Query parameter matcher map (REST). + // Key is the decoded query parameter name (case-sensitive). + // Value supports either a single glob (`glob`) or a list (`any`). + map query = 4; +} + +// Query value matcher for one query parameter key. +message L7QueryMatcher { + // Single glob pattern. + string glob = 1; + // Any-of glob patterns. + repeated string any = 2; } // A binary identity for network policy matching. From 0815f82958002bfdef89e0c0cda1cf95ac847f80 Mon Sep 17 00:00:00 2001 From: Rafael Marcelino Koike Date: Mon, 30 Mar 2026 17:29:00 -0400 Subject: [PATCH 10/20] perf(sandbox): streaming SHA256 and spawn_blocking for identity resolution (#555) * perf(sandbox): streaming SHA256, spawn_blocking for identity resolution Key changes: - Replace full file read + SHA256 with streaming 64KB-buffered hash (saves 124MB allocation for node binary) - Wrap evaluate_opa_tcp in spawn_blocking to prevent blocking tokio runtime during heavy /proc I/O and SHA256 computation - Add file-based perf logging for profiling proxy latency phases Profiling data (node binary, 124MB): - Cold TOFU: ~890ms (read+hash), warm: 0ms (cache hit) - evaluate_opa_tcp: cold=1002ms, warm=11ms - OPA evaluation: 1ms - DNS+TCP connect: 166-437ms Made-with: Cursor Signed-off-by: Rafael Koike * refactor(sandbox): replace perf_log with tracing::debug Replace the custom file-based perf_log() helper with standard tracing::debug!() macros as requested in PR review. This removes the custom log file writes to /var/log/openshell-perf.log and routes all performance timing through the tracing framework at DEBUG level, consistent with the rest of the codebase. Made-with: Cursor * refactor(sandbox): reduce tracing to 6 key diagnostic logs Address PR review feedback: 1. Remove ~20 inner-phase timing logs, keeping only the 6 that tell the full diagnostic story: - evaluate_opa_tcp TOTAL (proxy.rs) - dns_resolve_and_tcp_connect (proxy.rs) - file_sha256 (procfs.rs) - verify_or_cache CACHE HIT / CACHE MISS / TOTAL cold (identity.rs) 2. Restore intent-describing comments that were replaced by timing logs: - "TOFU verify the immediate binary" (proxy.rs) - "Walk the process tree upward to collect ancestor binaries" (proxy.rs) - "Collect cmdline paths for script-based binary detection." (proxy.rs) - "First: scan descendants of the entrypoint process" (procfs.rs) - "Fallback: scan all of /proc in case the process isn't in the tree" (procfs.rs) - "Skip PIDs we already checked" (procfs.rs) 3. Preserve file path in file_sha256 read errors instead of losing context via into_diagnostic(). 4. Tests: 293 passed, 1 pre-existing failure (drop_privileges), 1 ignored. Made-with: Cursor * style(sandbox): apply rustfmt formatting to debug macros --------- Signed-off-by: Rafael Koike Co-authored-by: John Myers --- crates/openshell-sandbox/src/identity.rs | 19 +++++++ crates/openshell-sandbox/src/procfs.rs | 34 ++++++++++-- crates/openshell-sandbox/src/proxy.rs | 71 +++++++++++++++++------- 3 files changed, 98 insertions(+), 26 deletions(-) diff --git a/crates/openshell-sandbox/src/identity.rs b/crates/openshell-sandbox/src/identity.rs index d27976ba7..49809f95b 100644 --- a/crates/openshell-sandbox/src/identity.rs +++ b/crates/openshell-sandbox/src/identity.rs @@ -16,6 +16,7 @@ use std::fs::Metadata; use std::os::unix::fs::MetadataExt; use std::path::{Path, PathBuf}; use std::sync::Mutex; +use tracing::debug; #[derive(Clone)] struct FileFingerprint { @@ -100,6 +101,7 @@ impl BinaryIdentityCache { where F: FnMut(&Path) -> Result, { + let start = std::time::Instant::now(); let metadata = std::fs::metadata(path) .map_err(|error| miette::miette!("Failed to stat {}: {error}", path.display()))?; let fingerprint = FileFingerprint::from_metadata(&metadata); @@ -114,9 +116,20 @@ impl BinaryIdentityCache { if let Some(cached_binary) = &cached && cached_binary.fingerprint == fingerprint { + debug!( + " verify_or_cache: {}ms CACHE HIT path={}", + start.elapsed().as_millis(), + path.display() + ); return Ok(cached_binary.hash.clone()); } + debug!( + " verify_or_cache: CACHE MISS size={} path={}", + metadata.len(), + path.display() + ); + let current_hash = hash_file(path)?; let mut hashes = self @@ -143,6 +156,12 @@ impl BinaryIdentityCache { }, ); + debug!( + " verify_or_cache TOTAL (cold): {}ms path={}", + start.elapsed().as_millis(), + path.display() + ); + Ok(current_hash) } } diff --git a/crates/openshell-sandbox/src/procfs.rs b/crates/openshell-sandbox/src/procfs.rs index ece16c82a..785a9489e 100644 --- a/crates/openshell-sandbox/src/procfs.rs +++ b/crates/openshell-sandbox/src/procfs.rs @@ -6,10 +6,11 @@ //! Provides functions to resolve binary paths and compute file hashes //! for process-identity binding in the OPA proxy policy engine. -use miette::{IntoDiagnostic, Result}; +use miette::Result; use std::path::Path; #[cfg(target_os = "linux")] use std::path::PathBuf; +use tracing::debug; /// Read the binary path of a process via `/proc/{pid}/exe` symlink. /// @@ -229,8 +230,9 @@ fn parse_proc_net_tcp(pid: u32, peer_port: u16) -> Result { fn find_pid_by_socket_inode(inode: u64, entrypoint_pid: u32) -> Result { let target = format!("socket:[{inode}]"); - // First: scan descendants of the entrypoint process (targeted, most likely to succeed) + // First: scan descendants of the entrypoint process let descendants = collect_descendant_pids(entrypoint_pid); + for &pid in &descendants { if let Some(found) = check_pid_fds(pid, &target) { return Ok(found); @@ -238,7 +240,6 @@ fn find_pid_by_socket_inode(inode: u64, entrypoint_pid: u32) -> Result { } // Fallback: scan all of /proc in case the process isn't in the tree - // (e.g., if /proc//task//children wasn't available) if let Ok(proc_dir) = std::fs::read_dir("/proc") { for entry in proc_dir.flatten() { let name = entry.file_name(); @@ -318,9 +319,32 @@ fn collect_descendant_pids(root_pid: u32) -> Vec { /// same hash, or the request is denied. pub fn file_sha256(path: &Path) -> Result { use sha2::{Digest, Sha256}; + use std::io::Read; + + let start = std::time::Instant::now(); + let mut file = std::fs::File::open(path) + .map_err(|e| miette::miette!("Failed to open {}: {e}", path.display()))?; + let mut hasher = Sha256::new(); + let mut buf = [0u8; 65536]; + let mut total_read = 0u64; + loop { + let n = file + .read(&mut buf) + .map_err(|e| miette::miette!("Failed to read {}: {e}", path.display()))?; + if n == 0 { + break; + } + total_read += n as u64; + hasher.update(&buf[..n]); + } - let bytes = std::fs::read(path).into_diagnostic()?; - let hash = Sha256::digest(&bytes); + let hash = hasher.finalize(); + debug!( + " file_sha256: {}ms size={} path={}", + start.elapsed().as_millis(), + total_read, + path.display() + ); Ok(hex::encode(hash)) } diff --git a/crates/openshell-sandbox/src/proxy.rs b/crates/openshell-sandbox/src/proxy.rs index 008877257..49fe5e07b 100644 --- a/crates/openshell-sandbox/src/proxy.rs +++ b/crates/openshell-sandbox/src/proxy.rs @@ -336,15 +336,25 @@ async fn handle_tcp_connection( let peer_addr = client.peer_addr().into_diagnostic()?; let local_addr = client.local_addr().into_diagnostic()?; - // Evaluate OPA policy with process-identity binding - let decision = evaluate_opa_tcp( - peer_addr, - &opa_engine, - &identity_cache, - &entrypoint_pid, - &host_lc, - port, - ); + // Evaluate OPA policy with process-identity binding. + // Wrapped in spawn_blocking because identity resolution does heavy sync I/O: + // /proc scanning + SHA256 hashing of binaries (e.g. node at 124MB). + let opa_clone = opa_engine.clone(); + let cache_clone = identity_cache.clone(); + let pid_clone = entrypoint_pid.clone(); + let host_clone = host_lc.clone(); + let decision = tokio::task::spawn_blocking(move || { + evaluate_opa_tcp( + peer_addr, + &opa_clone, + &cache_clone, + &pid_clone, + &host_clone, + port, + ) + }) + .await + .map_err(|e| miette::miette!("identity resolution task panicked: {e}"))?; // Extract action string and matched policy for logging let (matched_policy, deny_reason) = match &decision.action { @@ -426,6 +436,7 @@ async fn handle_tcp_connection( } // Defense-in-depth: resolve DNS and reject connections to internal IPs. + let dns_connect_start = std::time::Instant::now(); let mut upstream = if !raw_allowed_ips.is_empty() { // allowed_ips mode: validate resolved IPs against CIDR allowlist. // Loopback and link-local are still always blocked. @@ -502,6 +513,11 @@ async fn handle_tcp_connection( } }; + debug!( + "handle_tcp_connection dns_resolve_and_tcp_connect: {}ms host={host_lc}", + dns_connect_start.elapsed().as_millis() + ); + respond(&mut client, b"HTTP/1.1 200 Connection Established\r\n\r\n").await?; // Check if endpoint has L7 config for protocol-aware inspection @@ -736,7 +752,9 @@ fn evaluate_opa_tcp( ); } + let total_start = std::time::Instant::now(); let peer_port = peer_addr.port(); + let (bin_path, binary_pid) = match crate::procfs::resolve_tcp_peer_identity(pid, peer_port) { Ok(r) => r, Err(e) => { @@ -767,7 +785,6 @@ fn evaluate_opa_tcp( // Walk the process tree upward to collect ancestor binaries let ancestors = crate::procfs::collect_ancestor_binaries(binary_pid, pid); - // TOFU verify each ancestor binary for ancestor in &ancestors { if let Err(e) = identity_cache.verify_or_cache(ancestor) { return deny( @@ -784,7 +801,6 @@ fn evaluate_opa_tcp( } // Collect cmdline paths for script-based binary detection. - // Excludes exe paths already captured in bin_path/ancestors to avoid duplicates. let mut exclude = ancestors.clone(); exclude.push(bin_path.clone()); let cmdline_paths = crate::procfs::collect_cmdline_paths(binary_pid, pid, &exclude); @@ -798,7 +814,7 @@ fn evaluate_opa_tcp( cmdline_paths: cmdline_paths.clone(), }; - match engine.evaluate_network_action(&input) { + let result = match engine.evaluate_network_action(&input) { Ok(action) => ConnectDecision { action, binary: Some(bin_path), @@ -813,7 +829,12 @@ fn evaluate_opa_tcp( ancestors, cmdline_paths, ), - } + }; + debug!( + "evaluate_opa_tcp TOTAL: {}ms host={host} port={port}", + total_start.elapsed().as_millis() + ); + result } /// Non-Linux stub: OPA identity binding requires /proc. @@ -1728,14 +1749,22 @@ async fn handle_forward_proxy( let peer_addr = client.peer_addr().into_diagnostic()?; let local_addr = client.local_addr().into_diagnostic()?; - let decision = evaluate_opa_tcp( - peer_addr, - &opa_engine, - &identity_cache, - &entrypoint_pid, - &host_lc, - port, - ); + let opa_clone = opa_engine.clone(); + let cache_clone = identity_cache.clone(); + let pid_clone = entrypoint_pid.clone(); + let host_clone = host_lc.clone(); + let decision = tokio::task::spawn_blocking(move || { + evaluate_opa_tcp( + peer_addr, + &opa_clone, + &cache_clone, + &pid_clone, + &host_clone, + port, + ) + }) + .await + .map_err(|e| miette::miette!("identity resolution task panicked: {e}"))?; // Build log context let binary_str = decision From 36329a1059f5fe0c182c1656c4f73b9b190a2ec6 Mon Sep 17 00:00:00 2001 From: Peter Andreas Entschev Date: Tue, 31 Mar 2026 00:02:30 +0200 Subject: [PATCH 11/20] feat(inference): allow setting custom inference timeout (#672) * feat(inference): add timeout * feat(inference): fix dynamic timeout change * feat(inference): update docs * feat(inference): fix formatting --- architecture/inference-routing.md | 23 ++++++++++------- crates/openshell-cli/src/main.rs | 13 +++++++++- crates/openshell-cli/src/run.rs | 21 ++++++++++++++-- crates/openshell-router/src/backend.rs | 3 ++- crates/openshell-router/src/config.rs | 8 ++++++ crates/openshell-router/src/lib.rs | 3 --- crates/openshell-router/src/mock.rs | 1 + .../tests/backend_integration.rs | 6 +++++ crates/openshell-sandbox/src/lib.rs | 22 ++++++++++++++++ .../tests/system_inference.rs | 3 +++ crates/openshell-server/src/inference.rs | 25 +++++++++++++++++-- docs/inference/configure.md | 23 +++++++++++++++-- proto/inference.proto | 10 ++++++++ 13 files changed, 141 insertions(+), 20 deletions(-) diff --git a/architecture/inference-routing.md b/architecture/inference-routing.md index 0d3a95afb..9d45d7cd9 100644 --- a/architecture/inference-routing.md +++ b/architecture/inference-routing.md @@ -92,10 +92,10 @@ File: `proto/inference.proto` Key messages: -- `SetClusterInferenceRequest` -- `provider_name` + `model_id` + optional `no_verify` override, with verification enabled by default -- `SetClusterInferenceResponse` -- `provider_name` + `model_id` + `version` +- `SetClusterInferenceRequest` -- `provider_name` + `model_id` + `timeout_secs` + optional `no_verify` override, with verification enabled by default +- `SetClusterInferenceResponse` -- `provider_name` + `model_id` + `timeout_secs` + `version` - `GetInferenceBundleResponse` -- `repeated ResolvedRoute routes` + `revision` + `generated_at_ms` -- `ResolvedRoute` -- `name`, `base_url`, `protocols`, `api_key`, `model_id`, `provider_type` +- `ResolvedRoute` -- `name`, `base_url`, `protocols`, `api_key`, `model_id`, `provider_type`, `timeout_secs` ## Data Plane (Sandbox) @@ -106,7 +106,7 @@ Files: - `crates/openshell-sandbox/src/lib.rs` -- inference context initialization, route refresh - `crates/openshell-sandbox/src/grpc_client.rs` -- `fetch_inference_bundle()` -In cluster mode, the sandbox starts a background refresh loop as soon as the inference context is created. The loop polls the gateway every 5 seconds by default (`OPENSHELL_ROUTE_REFRESH_INTERVAL_SECS` override) and uses the bundle revision hash to skip no-op cache writes. +In cluster mode, the sandbox starts a background refresh loop as soon as the inference context is created. The loop polls the gateway every 5 seconds by default (`OPENSHELL_ROUTE_REFRESH_INTERVAL_SECS` override) and uses the bundle revision hash to skip no-op cache writes. The revision hash covers all route fields including `timeout_secs`, so any configuration change (provider, model, or timeout) triggers a cache update on the next poll. ### Interception flow @@ -143,7 +143,7 @@ If no pattern matches, the proxy returns `403 Forbidden` with `{"error": "connec ### Route cache - `InferenceContext` holds a `Router`, the pattern list, and an `Arc>>` route cache. -- In cluster mode, `spawn_route_refresh()` polls `GetInferenceBundle` every 30 seconds (`ROUTE_REFRESH_INTERVAL_SECS`). On failure, stale routes are kept. +- In cluster mode, `spawn_route_refresh()` polls `GetInferenceBundle` every 5 seconds (`OPENSHELL_ROUTE_REFRESH_INTERVAL_SECS`). On failure, stale routes are kept. - In file mode (`--inference-routes`), routes load once at startup from YAML. No refresh task is spawned. - In cluster mode, an empty initial bundle still enables the inference context so the refresh task can pick up later configuration. @@ -209,9 +209,11 @@ File: `crates/openshell-router/src/mock.rs` Routes with `mock://` scheme endpoints return canned responses without making HTTP requests. Mock responses are protocol-aware (OpenAI chat completion, OpenAI completion, Anthropic messages, or generic JSON). Mock routes include an `x-openshell-mock: true` response header. -### HTTP client +### Per-request timeout -The router uses a `reqwest::Client` with a 60-second timeout. Timeouts and connection failures map to `RouterError::UpstreamUnavailable`. +Each `ResolvedRoute` carries a `timeout` field (`Duration`). The `reqwest::Client` has no global timeout; instead, each outgoing request applies `.timeout(route.timeout)` on the request builder. When `timeout_secs` is `0` in the proto message, the default of 60 seconds is used (defined as `DEFAULT_ROUTE_TIMEOUT` in `config.rs`). Timeouts and connection failures map to `RouterError::UpstreamUnavailable`. + +Timeout changes propagate dynamically to running sandboxes. The bundle revision hash includes `timeout_secs`, so when the timeout is updated via `openshell inference update --timeout`, the refresh loop detects the revision change and updates the route cache within one polling interval (5 seconds by default). ## Standalone Route File @@ -297,13 +299,16 @@ The system route is stored as a separate `InferenceRoute` record in the gateway Cluster inference commands: -- `openshell inference set --provider --model ` -- configures user-facing cluster inference -- `openshell inference set --system --provider --model ` -- configures system inference +- `openshell inference set --provider --model [--timeout ]` -- configures user-facing cluster inference +- `openshell inference set --system --provider --model [--timeout ]` -- configures system inference +- `openshell inference update [--provider ] [--model ] [--timeout ]` -- updates individual fields without resetting others - `openshell inference get` -- displays both user and system inference configuration - `openshell inference get --system` -- displays only the system inference configuration The `--provider` flag references a provider record name (not a provider type). The provider must already exist in the cluster and have a supported inference type (`openai`, `anthropic`, or `nvidia`). +The `--timeout` flag sets the per-request timeout in seconds for upstream inference calls. When omitted or set to `0`, the default of 60 seconds applies. Timeout changes propagate to running sandboxes within the route refresh interval (5 seconds by default). + Inference writes verify by default. `--no-verify` is the explicit opt-out for endpoints that are not up yet. ## Provider Discovery diff --git a/crates/openshell-cli/src/main.rs b/crates/openshell-cli/src/main.rs index 5de31c79c..df37410b6 100644 --- a/crates/openshell-cli/src/main.rs +++ b/crates/openshell-cli/src/main.rs @@ -937,6 +937,10 @@ enum InferenceCommands { /// Skip endpoint verification before saving the route. #[arg(long)] no_verify: bool, + + /// Request timeout in seconds for inference calls (0 = default 60s). + #[arg(long, default_value_t = 0)] + timeout: u64, }, /// Update gateway-level inference configuration (partial update). @@ -957,6 +961,10 @@ enum InferenceCommands { /// Skip endpoint verification before saving the route. #[arg(long)] no_verify: bool, + + /// Request timeout in seconds for inference calls (0 = default 60s, unchanged if omitted). + #[arg(long)] + timeout: Option, }, /// Get gateway-level inference provider and model. @@ -2026,10 +2034,11 @@ async fn main() -> Result<()> { model, system, no_verify, + timeout, } => { let route_name = if system { "sandbox-system" } else { "" }; run::gateway_inference_set( - endpoint, &provider, &model, route_name, no_verify, &tls, + endpoint, &provider, &model, route_name, no_verify, timeout, &tls, ) .await?; } @@ -2038,6 +2047,7 @@ async fn main() -> Result<()> { model, system, no_verify, + timeout, } => { let route_name = if system { "sandbox-system" } else { "" }; run::gateway_inference_update( @@ -2046,6 +2056,7 @@ async fn main() -> Result<()> { model.as_deref(), route_name, no_verify, + timeout, &tls, ) .await?; diff --git a/crates/openshell-cli/src/run.rs b/crates/openshell-cli/src/run.rs index e32eec2a4..bab819137 100644 --- a/crates/openshell-cli/src/run.rs +++ b/crates/openshell-cli/src/run.rs @@ -3481,6 +3481,7 @@ pub async fn gateway_inference_set( model_id: &str, route_name: &str, no_verify: bool, + timeout_secs: u64, tls: &TlsOptions, ) -> Result<()> { let progress = if std::io::stdout().is_terminal() { @@ -3504,6 +3505,7 @@ pub async fn gateway_inference_set( route_name: route_name.to_string(), verify: false, no_verify, + timeout_secs, }) .await; @@ -3525,6 +3527,7 @@ pub async fn gateway_inference_set( println!(" {} {}", "Provider:".dimmed(), configured.provider_name); println!(" {} {}", "Model:".dimmed(), configured.model_id); println!(" {} {}", "Version:".dimmed(), configured.version); + print_timeout(configured.timeout_secs); if configured.validation_performed { println!(" {}", "Validated Endpoints:".dimmed()); for endpoint in configured.validated_endpoints { @@ -3540,11 +3543,12 @@ pub async fn gateway_inference_update( model_id: Option<&str>, route_name: &str, no_verify: bool, + timeout_secs: Option, tls: &TlsOptions, ) -> Result<()> { - if provider_name.is_none() && model_id.is_none() { + if provider_name.is_none() && model_id.is_none() && timeout_secs.is_none() { return Err(miette::miette!( - "at least one of --provider or --model must be specified" + "at least one of --provider, --model, or --timeout must be specified" )); } @@ -3561,6 +3565,7 @@ pub async fn gateway_inference_update( let provider = provider_name.unwrap_or(¤t.provider_name); let model = model_id.unwrap_or(¤t.model_id); + let timeout = timeout_secs.unwrap_or(current.timeout_secs); let progress = if std::io::stdout().is_terminal() { let spinner = ProgressBar::new_spinner(); @@ -3582,6 +3587,7 @@ pub async fn gateway_inference_update( route_name: route_name.to_string(), verify: false, no_verify, + timeout_secs: timeout, }) .await; @@ -3603,6 +3609,7 @@ pub async fn gateway_inference_update( println!(" {} {}", "Provider:".dimmed(), configured.provider_name); println!(" {} {}", "Model:".dimmed(), configured.model_id); println!(" {} {}", "Version:".dimmed(), configured.version); + print_timeout(configured.timeout_secs); if configured.validation_performed { println!(" {}", "Validated Endpoints:".dimmed()); for endpoint in configured.validated_endpoints { @@ -3639,6 +3646,7 @@ pub async fn gateway_inference_get( println!(" {} {}", "Provider:".dimmed(), configured.provider_name); println!(" {} {}", "Model:".dimmed(), configured.model_id); println!(" {} {}", "Version:".dimmed(), configured.version); + print_timeout(configured.timeout_secs); } else { // Show both routes by default. print_inference_route(&mut client, "Gateway inference", "").await; @@ -3666,6 +3674,7 @@ async fn print_inference_route( println!(" {} {}", "Provider:".dimmed(), configured.provider_name); println!(" {} {}", "Model:".dimmed(), configured.model_id); println!(" {} {}", "Version:".dimmed(), configured.version); + print_timeout(configured.timeout_secs); } Err(e) if e.code() == Code::NotFound => { println!("{}", format!("{label}:").cyan().bold()); @@ -3680,6 +3689,14 @@ async fn print_inference_route( } } +fn print_timeout(timeout_secs: u64) { + if timeout_secs == 0 { + println!(" {} {}s (default)", "Timeout:".dimmed(), 60); + } else { + println!(" {} {}s", "Timeout:".dimmed(), timeout_secs); + } +} + fn format_inference_status(status: Status) -> miette::Report { let message = status.message().trim(); diff --git a/crates/openshell-router/src/backend.rs b/crates/openshell-router/src/backend.rs index d82ea082c..d1d7092c0 100644 --- a/crates/openshell-router/src/backend.rs +++ b/crates/openshell-router/src/backend.rs @@ -149,7 +149,7 @@ async fn send_backend_request( } Err(_) => body, }; - builder = builder.body(body); + builder = builder.body(body).timeout(route.timeout); builder.send().await.map_err(|e| { if e.is_timeout() { @@ -468,6 +468,7 @@ mod tests { protocols: protocols.iter().map(|p| (*p).to_string()).collect(), auth, default_headers: vec![("anthropic-version".to_string(), "2023-06-01".to_string())], + timeout: crate::config::DEFAULT_ROUTE_TIMEOUT, } } diff --git a/crates/openshell-router/src/config.rs b/crates/openshell-router/src/config.rs index d9c081d60..52c22da9f 100644 --- a/crates/openshell-router/src/config.rs +++ b/crates/openshell-router/src/config.rs @@ -3,11 +3,14 @@ use serde::Deserialize; use std::path::Path; +use std::time::Duration; pub use openshell_core::inference::AuthHeader; use crate::RouterError; +pub const DEFAULT_ROUTE_TIMEOUT: Duration = Duration::from_secs(60); + #[derive(Debug, Clone, Deserialize)] pub struct RouterConfig { pub routes: Vec, @@ -45,6 +48,8 @@ pub struct ResolvedRoute { pub auth: AuthHeader, /// Extra headers injected on every request (e.g. `anthropic-version`). pub default_headers: Vec<(String, String)>, + /// Per-request timeout for proxied inference calls. + pub timeout: Duration, } impl std::fmt::Debug for ResolvedRoute { @@ -57,6 +62,7 @@ impl std::fmt::Debug for ResolvedRoute { .field("protocols", &self.protocols) .field("auth", &self.auth) .field("default_headers", &self.default_headers) + .field("timeout", &self.timeout) .finish() } } @@ -129,6 +135,7 @@ impl RouteConfig { protocols, auth, default_headers, + timeout: DEFAULT_ROUTE_TIMEOUT, }) } } @@ -256,6 +263,7 @@ routes: protocols: vec!["openai_chat_completions".to_string()], auth: AuthHeader::Bearer, default_headers: Vec::new(), + timeout: DEFAULT_ROUTE_TIMEOUT, }; let debug_output = format!("{route:?}"); assert!( diff --git a/crates/openshell-router/src/lib.rs b/crates/openshell-router/src/lib.rs index a5712d9a0..7deed6fc4 100644 --- a/crates/openshell-router/src/lib.rs +++ b/crates/openshell-router/src/lib.rs @@ -5,8 +5,6 @@ mod backend; pub mod config; mod mock; -use std::time::Duration; - pub use backend::{ ProxyResponse, StreamingProxyResponse, ValidatedEndpoint, ValidationFailure, ValidationFailureKind, verify_backend_endpoint, @@ -39,7 +37,6 @@ pub struct Router { impl Router { pub fn new() -> Result { let client = reqwest::Client::builder() - .timeout(Duration::from_secs(60)) .build() .map_err(|e| RouterError::Internal(format!("failed to build HTTP client: {e}")))?; Ok(Self { diff --git a/crates/openshell-router/src/mock.rs b/crates/openshell-router/src/mock.rs index 9b6accb60..a17ce486f 100644 --- a/crates/openshell-router/src/mock.rs +++ b/crates/openshell-router/src/mock.rs @@ -131,6 +131,7 @@ mod tests { protocols: protocols.iter().map(ToString::to_string).collect(), auth: crate::config::AuthHeader::Bearer, default_headers: Vec::new(), + timeout: crate::config::DEFAULT_ROUTE_TIMEOUT, } } diff --git a/crates/openshell-router/tests/backend_integration.rs b/crates/openshell-router/tests/backend_integration.rs index 4861bd6d0..571964aa8 100644 --- a/crates/openshell-router/tests/backend_integration.rs +++ b/crates/openshell-router/tests/backend_integration.rs @@ -15,6 +15,7 @@ fn mock_candidates(base_url: &str) -> Vec { protocols: vec!["openai_chat_completions".to_string()], auth: AuthHeader::Bearer, default_headers: Vec::new(), + timeout: openshell_router::config::DEFAULT_ROUTE_TIMEOUT, }] } @@ -117,6 +118,7 @@ async fn proxy_no_compatible_route_returns_error() { protocols: vec!["anthropic_messages".to_string()], auth: AuthHeader::Custom("x-api-key"), default_headers: Vec::new(), + timeout: openshell_router::config::DEFAULT_ROUTE_TIMEOUT, }]; let err = router @@ -178,6 +180,7 @@ async fn proxy_mock_route_returns_canned_response() { protocols: vec!["openai_chat_completions".to_string()], auth: AuthHeader::Bearer, default_headers: Vec::new(), + timeout: openshell_router::config::DEFAULT_ROUTE_TIMEOUT, }]; let body = serde_json::to_vec(&serde_json::json!({ @@ -312,6 +315,7 @@ async fn proxy_uses_x_api_key_for_anthropic_route() { protocols: vec!["anthropic_messages".to_string()], auth: AuthHeader::Custom("x-api-key"), default_headers: vec![("anthropic-version".to_string(), "2023-06-01".to_string())], + timeout: openshell_router::config::DEFAULT_ROUTE_TIMEOUT, }]; let body = serde_json::to_vec(&serde_json::json!({ @@ -370,6 +374,7 @@ async fn proxy_anthropic_does_not_send_bearer_auth() { protocols: vec!["anthropic_messages".to_string()], auth: AuthHeader::Custom("x-api-key"), default_headers: vec![("anthropic-version".to_string(), "2023-06-01".to_string())], + timeout: openshell_router::config::DEFAULT_ROUTE_TIMEOUT, }]; let response = router @@ -414,6 +419,7 @@ async fn proxy_forwards_client_anthropic_version_header() { protocols: vec!["anthropic_messages".to_string()], auth: AuthHeader::Custom("x-api-key"), default_headers: vec![("anthropic-version".to_string(), "2023-06-01".to_string())], + timeout: openshell_router::config::DEFAULT_ROUTE_TIMEOUT, }]; let body = serde_json::to_vec(&serde_json::json!({ diff --git a/crates/openshell-sandbox/src/lib.rs b/crates/openshell-sandbox/src/lib.rs index 297d7fc38..149632446 100644 --- a/crates/openshell-sandbox/src/lib.rs +++ b/crates/openshell-sandbox/src/lib.rs @@ -801,6 +801,11 @@ pub(crate) fn bundle_to_resolved_routes( .map(|r| { let (auth, default_headers) = openshell_core::inference::auth_for_provider_type(&r.provider_type); + let timeout = if r.timeout_secs == 0 { + openshell_router::config::DEFAULT_ROUTE_TIMEOUT + } else { + Duration::from_secs(r.timeout_secs) + }; openshell_router::config::ResolvedRoute { name: r.name.clone(), endpoint: r.base_url.clone(), @@ -809,6 +814,7 @@ pub(crate) fn bundle_to_resolved_routes( protocols: r.protocols.clone(), auth, default_headers, + timeout, } }) .collect() @@ -1517,6 +1523,7 @@ mod tests { "openai_responses".to_string(), ], provider_type: "openai".to_string(), + timeout_secs: 0, }, openshell_core::proto::ResolvedRoute { name: "local".to_string(), @@ -1525,6 +1532,7 @@ mod tests { model_id: "llama-3".to_string(), protocols: vec!["openai_chat_completions".to_string()], provider_type: String::new(), + timeout_secs: 120, }, ], revision: "abc123".to_string(), @@ -1545,11 +1553,21 @@ mod tests { routes[0].protocols, vec!["openai_chat_completions", "openai_responses"] ); + assert_eq!( + routes[0].timeout, + openshell_router::config::DEFAULT_ROUTE_TIMEOUT, + "timeout_secs=0 should map to default" + ); assert_eq!(routes[1].endpoint, "http://vllm:8000/v1"); assert_eq!( routes[1].auth, openshell_core::inference::AuthHeader::Bearer ); + assert_eq!( + routes[1].timeout, + Duration::from_secs(120), + "timeout_secs=120 should map to 120s" + ); } #[test] @@ -1574,6 +1592,7 @@ mod tests { model_id: "model".to_string(), protocols: vec!["openai_chat_completions".to_string()], provider_type: "openai".to_string(), + timeout_secs: 0, }], revision: "rev".to_string(), generated_at_ms: 0, @@ -1594,6 +1613,7 @@ mod tests { protocols: vec!["openai_chat_completions".to_string()], auth: openshell_core::inference::AuthHeader::Bearer, default_headers: vec![], + timeout: openshell_router::config::DEFAULT_ROUTE_TIMEOUT, }, openshell_router::config::ResolvedRoute { name: "sandbox-system".to_string(), @@ -1603,6 +1623,7 @@ mod tests { protocols: vec!["anthropic_messages".to_string()], auth: openshell_core::inference::AuthHeader::Custom("x-api-key"), default_headers: vec![], + timeout: openshell_router::config::DEFAULT_ROUTE_TIMEOUT, }, ]; @@ -1891,6 +1912,7 @@ filesystem_policy: auth: openshell_core::inference::AuthHeader::Bearer, protocols: vec!["openai_chat_completions".to_string()], default_headers: vec![], + timeout: openshell_router::config::DEFAULT_ROUTE_TIMEOUT, }]; let cache = Arc::new(RwLock::new(routes)); diff --git a/crates/openshell-sandbox/tests/system_inference.rs b/crates/openshell-sandbox/tests/system_inference.rs index 3f6a471e5..5d581fbe2 100644 --- a/crates/openshell-sandbox/tests/system_inference.rs +++ b/crates/openshell-sandbox/tests/system_inference.rs @@ -20,6 +20,7 @@ fn make_system_route() -> ResolvedRoute { protocols: vec!["openai_chat_completions".to_string()], auth: AuthHeader::Bearer, default_headers: Vec::new(), + timeout: openshell_router::config::DEFAULT_ROUTE_TIMEOUT, } } @@ -32,6 +33,7 @@ fn make_user_route() -> ResolvedRoute { protocols: vec!["openai_chat_completions".to_string()], auth: AuthHeader::Bearer, default_headers: Vec::new(), + timeout: openshell_router::config::DEFAULT_ROUTE_TIMEOUT, } } @@ -124,6 +126,7 @@ async fn system_inference_with_anthropic_protocol() { protocols: vec!["anthropic_messages".to_string()], auth: AuthHeader::Custom("x-api-key"), default_headers: vec![("anthropic-version".to_string(), "2023-06-01".to_string())], + timeout: openshell_router::config::DEFAULT_ROUTE_TIMEOUT, }; let ctx = InferenceContext::new(patterns, router, vec![], vec![system_route]); diff --git a/crates/openshell-server/src/inference.rs b/crates/openshell-server/src/inference.rs index bbabaf70b..0fb29bde5 100644 --- a/crates/openshell-server/src/inference.rs +++ b/crates/openshell-server/src/inference.rs @@ -86,6 +86,7 @@ impl Inference for InferenceService { route_name, &req.provider_name, &req.model_id, + req.timeout_secs, verify, ) .await?; @@ -103,6 +104,7 @@ impl Inference for InferenceService { route_name: route_name.to_string(), validation_performed: !route.validation.is_empty(), validated_endpoints: route.validation, + timeout_secs: config.timeout_secs, })) } @@ -140,6 +142,7 @@ impl Inference for InferenceService { model_id: config.model_id.clone(), version: route.version, route_name: route_name.to_string(), + timeout_secs: config.timeout_secs, })) } } @@ -149,6 +152,7 @@ async fn upsert_cluster_inference_route( route_name: &str, provider_name: &str, model_id: &str, + timeout_secs: u64, verify: bool, ) -> Result { if provider_name.trim().is_empty() { @@ -173,7 +177,7 @@ async fn upsert_cluster_inference_route( Vec::new() }; - let config = build_cluster_inference_config(&provider, model_id); + let config = build_cluster_inference_config(&provider, model_id, timeout_secs); let existing = store .get_message_by_name::(route_name) @@ -204,10 +208,15 @@ async fn upsert_cluster_inference_route( Ok(UpsertedInferenceRoute { route, validation }) } -fn build_cluster_inference_config(provider: &Provider, model_id: &str) -> ClusterInferenceConfig { +fn build_cluster_inference_config( + provider: &Provider, + model_id: &str, + timeout_secs: u64, +) -> ClusterInferenceConfig { ClusterInferenceConfig { provider_name: provider.name.clone(), model_id: model_id.to_string(), + timeout_secs, } } @@ -267,6 +276,7 @@ fn resolve_provider_route(provider: &Provider) -> Result Result Date: Mon, 30 Mar 2026 16:08:26 -0700 Subject: [PATCH 12/20] fix(sandbox): track PTY state per SSH channel to fix terminal resize (#687) Replace flat pty_master/input_sender/pty_request fields in SshHandler with a HashMap so each channel tracks its own PTY resources independently. This fixes window_change_request resizing the wrong PTY when multiple channels are open simultaneously. Also fixes ioctl UB in set_winsize (pass &winsize not winsize by value) and adds warn! logging for unknown channels across all handlers. Resolves #543 --- crates/openshell-sandbox/src/ssh.rs | 198 +++++++++++++++++++++++++--- 1 file changed, 177 insertions(+), 21 deletions(-) diff --git a/crates/openshell-sandbox/src/ssh.rs b/crates/openshell-sandbox/src/ssh.rs index 10eab8c45..e3add8874 100644 --- a/crates/openshell-sandbox/src/ssh.rs +++ b/crates/openshell-sandbox/src/ssh.rs @@ -263,6 +263,19 @@ fn hmac_sha256(key: &[u8], data: &[u8]) -> String { hex::encode(result) } +/// Per-channel state for tracking PTY resources and I/O senders. +/// +/// Each SSH channel gets its own PTY master (if a PTY was requested) and input +/// sender. This allows `window_change_request` to resize the correct PTY when +/// multiple channels are open simultaneously (e.g. parallel shells, shell + +/// sftp, etc.). +#[derive(Default)] +struct ChannelState { + input_sender: Option>>, + pty_master: Option, + pty_request: Option, +} + struct SshHandler { policy: SandboxPolicy, workdir: Option, @@ -270,9 +283,7 @@ struct SshHandler { proxy_url: Option, ca_file_paths: Option>, provider_env: HashMap, - input_sender: Option>>, - pty_master: Option, - pty_request: Option, + channels: HashMap, } impl SshHandler { @@ -291,9 +302,7 @@ impl SshHandler { proxy_url, ca_file_paths, provider_env, - input_sender: None, - pty_master: None, - pty_request: None, + channels: HashMap::new(), } } } @@ -315,12 +324,27 @@ impl russh::server::Handler for SshHandler { async fn channel_open_session( &mut self, - _channel: russh::Channel, + channel: russh::Channel, _session: &mut Session, ) -> Result { + self.channels.insert(channel.id(), ChannelState::default()); Ok(true) } + /// Clean up per-channel state when the channel is closed. + /// + /// This is the final cleanup and subsumes `channel_eof` — if `channel_close` + /// fires without a preceding `channel_eof`, all resources (pty_master File, + /// input_sender) are dropped here. + async fn channel_close( + &mut self, + channel: ChannelId, + _session: &mut Session, + ) -> Result<(), Self::Error> { + self.channels.remove(&channel); + Ok(()) + } + async fn channel_open_direct_tcpip( &mut self, channel: russh::Channel, @@ -388,7 +412,11 @@ impl russh::server::Handler for SshHandler { _modes: &[(russh::Pty, u32)], session: &mut Session, ) -> Result<(), Self::Error> { - self.pty_request = Some(PtyRequest { + let state = self + .channels + .get_mut(&channel) + .ok_or_else(|| anyhow::anyhow!("pty_request on unknown channel {channel:?}"))?; + state.pty_request = Some(PtyRequest { term: term.to_string(), col_width, row_height, @@ -401,21 +429,27 @@ impl russh::server::Handler for SshHandler { async fn window_change_request( &mut self, - _channel: ChannelId, + channel: ChannelId, col_width: u32, row_height: u32, pixel_width: u32, pixel_height: u32, _session: &mut Session, ) -> Result<(), Self::Error> { - if let Some(master) = self.pty_master.as_ref() { + let Some(state) = self.channels.get(&channel) else { + warn!("window_change_request on unknown channel {channel:?}"); + return Ok(()); + }; + if let Some(master) = state.pty_master.as_ref() { let winsize = Winsize { ws_row: to_u16(row_height.max(1)), ws_col: to_u16(col_width.max(1)), ws_xpixel: to_u16(pixel_width), ws_ypixel: to_u16(pixel_height), }; - let _ = unsafe_pty::set_winsize(master.as_raw_fd(), winsize); + if let Err(e) = unsafe_pty::set_winsize(master.as_raw_fd(), winsize) { + warn!("failed to resize PTY for channel {channel:?}: {e}"); + } } Ok(()) } @@ -474,7 +508,10 @@ impl russh::server::Handler for SshHandler { self.ca_file_paths.clone(), &self.provider_env, )?; - self.input_sender = Some(input_sender); + let state = self.channels.get_mut(&channel).ok_or_else(|| { + anyhow::anyhow!("subsystem_request on unknown channel {channel:?}") + })?; + state.input_sender = Some(input_sender); } else { warn!(subsystem = name, "unsupported subsystem requested"); session.channel_failure(channel)?; @@ -499,11 +536,15 @@ impl russh::server::Handler for SshHandler { async fn data( &mut self, - _channel: ChannelId, + channel: ChannelId, data: &[u8], _session: &mut Session, ) -> Result<(), Self::Error> { - if let Some(sender) = self.input_sender.as_ref() { + let Some(state) = self.channels.get(&channel) else { + warn!("data on unknown channel {channel:?}"); + return Ok(()); + }; + if let Some(sender) = state.input_sender.as_ref() { let _ = sender.send(data.to_vec()); } Ok(()) @@ -511,14 +552,18 @@ impl russh::server::Handler for SshHandler { async fn channel_eof( &mut self, - _channel: ChannelId, + channel: ChannelId, _session: &mut Session, ) -> Result<(), Self::Error> { // Drop the input sender so the stdin writer thread sees a // disconnected channel and closes the child's stdin pipe. This // is essential for commands like `cat | tar xf -` which need // stdin EOF to know the input stream is complete. - self.input_sender.take(); + if let Some(state) = self.channels.get_mut(&channel) { + state.input_sender.take(); + } else { + warn!("channel_eof on unknown channel {channel:?}"); + } Ok(()) } } @@ -530,7 +575,11 @@ impl SshHandler { handle: Handle, command: Option, ) -> anyhow::Result<()> { - if let Some(pty) = self.pty_request.take() { + let state = self + .channels + .get_mut(&channel) + .ok_or_else(|| anyhow::anyhow!("start_shell on unknown channel {channel:?}"))?; + if let Some(pty) = state.pty_request.take() { // PTY was requested — allocate a real PTY (interactive shell or // exec that explicitly asked for a terminal). let (pty_master, input_sender) = spawn_pty_shell( @@ -545,8 +594,8 @@ impl SshHandler { self.ca_file_paths.clone(), &self.provider_env, )?; - self.pty_master = Some(pty_master); - self.input_sender = Some(input_sender); + state.pty_master = Some(pty_master); + state.input_sender = Some(input_sender); } else { // No PTY requested — use plain pipes so stdout/stderr are // separate and output has clean LF line endings. This is the @@ -562,7 +611,7 @@ impl SshHandler { self.ca_file_paths.clone(), &self.provider_env, )?; - self.input_sender = Some(input_sender); + state.input_sender = Some(input_sender); } Ok(()) } @@ -999,7 +1048,7 @@ mod unsafe_pty { #[allow(unsafe_code)] pub fn set_winsize(fd: RawFd, winsize: Winsize) -> std::io::Result<()> { - let rc = unsafe { libc::ioctl(fd, libc::TIOCSWINSZ, winsize) }; + let rc = unsafe { libc::ioctl(fd, libc::TIOCSWINSZ, &winsize) }; if rc != 0 { return Err(std::io::Error::last_os_error()); } @@ -1404,4 +1453,111 @@ mod tests { assert!(!is_loopback_host("not-an-ip")); assert!(!is_loopback_host("[]")); } + + // ----------------------------------------------------------------------- + // Per-channel PTY state tests (#543) + // ----------------------------------------------------------------------- + + #[test] + fn set_winsize_applies_to_correct_pty() { + // Verify that set_winsize applies to a specific PTY master FD, + // which is the mechanism that per-channel tracking relies on. + // With the old single-pty_master design, a window_change_request + // for channel N would resize whatever PTY was stored last — + // potentially belonging to a different channel. + let pty_a = openpty(None, None).expect("openpty a"); + let pty_b = openpty(None, None).expect("openpty b"); + let master_a = std::fs::File::from(pty_a.master); + let master_b = std::fs::File::from(pty_b.master); + let fd_a = master_a.as_raw_fd(); + let fd_b = master_b.as_raw_fd(); + assert_ne!(fd_a, fd_b, "two PTYs must have distinct FDs"); + + // Close the slave ends to avoid leaking FDs in the test. + drop(std::fs::File::from(pty_a.slave)); + drop(std::fs::File::from(pty_b.slave)); + + // Resize only PTY B. + let winsize_b = Winsize { + ws_row: 50, + ws_col: 120, + ws_xpixel: 0, + ws_ypixel: 0, + }; + unsafe_pty::set_winsize(fd_b, winsize_b).expect("set_winsize on PTY B"); + + // Resize PTY A to a different size. + let winsize_a = Winsize { + ws_row: 24, + ws_col: 80, + ws_xpixel: 0, + ws_ypixel: 0, + }; + unsafe_pty::set_winsize(fd_a, winsize_a).expect("set_winsize on PTY A"); + + // Read back sizes via ioctl to verify independence. + let mut actual_a: libc::winsize = unsafe { std::mem::zeroed() }; + let mut actual_b: libc::winsize = unsafe { std::mem::zeroed() }; + #[allow(unsafe_code)] + unsafe { + libc::ioctl(fd_a, libc::TIOCGWINSZ, &mut actual_a); + libc::ioctl(fd_b, libc::TIOCGWINSZ, &mut actual_b); + } + + assert_eq!(actual_a.ws_row, 24, "PTY A should be 24 rows"); + assert_eq!(actual_a.ws_col, 80, "PTY A should be 80 cols"); + assert_eq!(actual_b.ws_row, 50, "PTY B should be 50 rows"); + assert_eq!(actual_b.ws_col, 120, "PTY B should be 120 cols"); + } + + #[test] + fn channel_state_independent_input_senders() { + // Verify that each channel gets its own input sender so that + // data() and channel_eof() affect only the targeted channel. + let (tx_a, rx_a) = mpsc::channel::>(); + let (tx_b, rx_b) = mpsc::channel::>(); + + let mut state_a = ChannelState { + input_sender: Some(tx_a), + ..Default::default() + }; + let state_b = ChannelState { + input_sender: Some(tx_b), + ..Default::default() + }; + + // Send data to channel A only. + state_a + .input_sender + .as_ref() + .unwrap() + .send(b"hello-a".to_vec()) + .unwrap(); + // Send data to channel B only. + state_b + .input_sender + .as_ref() + .unwrap() + .send(b"hello-b".to_vec()) + .unwrap(); + + assert_eq!(rx_a.recv().unwrap(), b"hello-a"); + assert_eq!(rx_b.recv().unwrap(), b"hello-b"); + + // EOF on channel A (drop sender) should not affect channel B. + state_a.input_sender.take(); + assert!( + rx_a.recv().is_err(), + "channel A sender dropped, recv should fail" + ); + + // Channel B should still be functional. + state_b + .input_sender + .as_ref() + .unwrap() + .send(b"still-alive".to_vec()) + .unwrap(); + assert_eq!(rx_b.recv().unwrap(), b"still-alive"); + } } From 047de66b22e6ef228479c4348e48a1296049849b Mon Sep 17 00:00:00 2001 From: Evan Lezar Date: Tue, 31 Mar 2026 17:23:52 +0200 Subject: [PATCH 13/20] feat(bootstrap,cli): switch GPU injection to CDI where supported (#495) * feat(bootstrap): switch GPU injection to CDI where supported Use an explicit CDI device request (driver="cdi", device_ids=["nvidia.com/gpu=all"]) when the Docker daemon reports CDI spec directories via GET /info (SystemInfo.CDISpecDirs). This makes device injection declarative and decouples spec generation from consumption. When the daemon reports no CDI spec directories, fall back to the legacy NVIDIA device request (driver="nvidia", count=-1) which relies on the NVIDIA Container Runtime hook. Failure modes for both paths are equivalent: a missing or stale NVIDIA Container Toolkit installation will cause container start to fail. CDI spec generation is out of scope for this change; specs are expected to be pre-generated out-of-band, for example by the NVIDIA Container Toolkit. --------- Signed-off-by: Evan Lezar Co-authored-by: Piotr Mlocek --- README.md | 2 +- architecture/gateway-single-node.md | 14 ++- crates/openshell-bootstrap/src/docker.rs | 120 +++++++++++++++++++---- crates/openshell-bootstrap/src/lib.rs | 39 ++++++-- crates/openshell-cli/src/bootstrap.rs | 6 +- crates/openshell-cli/src/main.rs | 15 ++- crates/openshell-cli/src/run.rs | 2 +- docs/sandboxes/manage-gateways.md | 2 +- e2e/rust/tests/cli_smoke.rs | 17 ++-- 9 files changed, 175 insertions(+), 42 deletions(-) diff --git a/README.md b/README.md index ccf8bbdef..5549fa7b4 100644 --- a/README.md +++ b/README.md @@ -128,7 +128,7 @@ OpenShell can pass host GPUs into sandboxes for local inference, fine-tuning, or openshell sandbox create --gpu --from [gpu-enabled-sandbox] -- claude ``` -The CLI auto-bootstraps a GPU-enabled gateway on first use. GPU intent is also inferred automatically for community images with `gpu` in the name. +The CLI auto-bootstraps a GPU-enabled gateway on first use, auto-selecting CDI when available and otherwise falling back to Docker's NVIDIA GPU request path (`--gpus all`). GPU intent is also inferred automatically for community images with `gpu` in the name. **Requirements:** NVIDIA drivers and the [NVIDIA Container Toolkit](https://docs.nvidia.com/datacenter/cloud-native/container-toolkit/latest/install-guide.html) must be installed on the host. The sandbox image itself must include the appropriate GPU drivers and libraries for your workload — the default `base` image does not. See the [BYOC example](https://github.com/NVIDIA/OpenShell/tree/main/examples/bring-your-own-container) for building a custom sandbox image with GPU support. diff --git a/architecture/gateway-single-node.md b/architecture/gateway-single-node.md index 57aebd3a5..c417775a5 100644 --- a/architecture/gateway-single-node.md +++ b/architecture/gateway-single-node.md @@ -296,8 +296,10 @@ When environment variables are set, the entrypoint modifies the HelmChart manife GPU support is part of the single-node gateway bootstrap path rather than a separate architecture. -- `openshell gateway start --gpu` threads a boolean deploy option through `crates/openshell-cli`, `crates/openshell-bootstrap`, and `crates/openshell-bootstrap/src/docker.rs`. -- When enabled, the cluster container is created with Docker `DeviceRequests`, which is the API equivalent of `docker run --gpus all`. +- `openshell gateway start --gpu` threads GPU device options through `crates/openshell-cli`, `crates/openshell-bootstrap`, and `crates/openshell-bootstrap/src/docker.rs`. +- When enabled, the cluster container is created with Docker `DeviceRequests`. The injection mechanism is selected based on whether CDI is enabled on the daemon (`SystemInfo.CDISpecDirs` via `GET /info`): + - **CDI enabled** (daemon reports non-empty `CDISpecDirs`): CDI device injection — `driver="cdi"` with `nvidia.com/gpu=all`. Specs are expected to be pre-generated on the host (e.g. automatically by the `nvidia-cdi-refresh.service` or manually via `nvidia-ctk generate`). + - **CDI not enabled**: `--gpus all` device request — `driver="nvidia"`, `count=-1`, which relies on the NVIDIA Container Runtime hook. - `deploy/docker/Dockerfile.images` installs NVIDIA Container Toolkit packages in a dedicated Ubuntu stage and copies the runtime binaries, config, and `libnvidia-container` shared libraries into the final Ubuntu-based cluster image. - `deploy/docker/cluster-entrypoint.sh` checks `GPU_ENABLED=true` and copies GPU-only manifests from `/opt/openshell/gpu-manifests/` into k3s's manifests directory. - `deploy/kube/gpu-manifests/nvidia-device-plugin-helmchart.yaml` installs the NVIDIA device plugin chart, currently pinned to `0.18.2`. NFD and GFD are disabled; the device plugin's default `nodeAffinity` (which requires `feature.node.kubernetes.io/pci-10de.present=true` or `nvidia.com/gpu.present=true` from NFD/GFD) is overridden to empty so the DaemonSet schedules on the single-node cluster without requiring those labels. @@ -308,12 +310,16 @@ The runtime chain is: ```text Host GPU drivers & NVIDIA Container Toolkit - └─ Docker: --gpus all (DeviceRequests in bollard API) + └─ Docker: DeviceRequests (CDI when enabled, --gpus all otherwise) └─ k3s/containerd: nvidia-container-runtime on PATH -> auto-detected └─ k8s: nvidia-device-plugin DaemonSet advertises nvidia.com/gpu └─ Pods: request nvidia.com/gpu in resource limits ``` +### `--gpu` flag + +The `--gpu` flag on `gateway start` enables GPU passthrough. OpenShell auto-selects CDI when enabled on the daemon and falls back to Docker's NVIDIA GPU request path (`--gpus all`) otherwise. + The expected smoke test is a plain pod requesting `nvidia.com/gpu: 1` with `runtimeClassName: nvidia` and running `nvidia-smi`. ## Remote Image Transfer @@ -381,7 +387,7 @@ When `openshell sandbox create` cannot connect to a gateway (connection refused, 1. `should_attempt_bootstrap()` in `crates/openshell-cli/src/bootstrap.rs` checks the error type. It returns `true` for connectivity errors and missing default TLS materials, but `false` for TLS handshake/auth errors. 2. If running in a terminal, the user is prompted to confirm. 3. `run_bootstrap()` deploys a gateway named `"openshell"`, sets it as active, and returns fresh `TlsOptions` pointing to the newly-written mTLS certs. -4. When `sandbox create` requests GPU explicitly (`--gpu`) or infers it from an image whose final name component contains `gpu` (such as `nvidia-gpu`), the bootstrap path enables gateway GPU support before retrying sandbox creation. +4. When `sandbox create` requests GPU explicitly (`--gpu`) or infers it from an image whose final name component contains `gpu` (such as `nvidia-gpu`), the bootstrap path enables gateway GPU support before retrying sandbox creation, using the same CDI-or-fallback selection as `gateway start --gpu`. ## Container Environment Variables diff --git a/crates/openshell-bootstrap/src/docker.rs b/crates/openshell-bootstrap/src/docker.rs index 9c365bfe8..cc63aacce 100644 --- a/crates/openshell-bootstrap/src/docker.rs +++ b/crates/openshell-bootstrap/src/docker.rs @@ -22,6 +22,29 @@ use std::collections::HashMap; const REGISTRY_NAMESPACE_DEFAULT: &str = "openshell"; +/// Resolve the raw GPU device-ID list, replacing the `"auto"` sentinel with a +/// concrete device ID based on whether CDI is enabled on the daemon. +/// +/// | Input | Output | +/// |--------------|--------------------------------------------------------------| +/// | `[]` | `[]` — no GPU | +/// | `["legacy"]` | `["legacy"]` — pass through to the non-CDI fallback path | +/// | `["auto"]` | `["nvidia.com/gpu=all"]` if CDI enabled, else `["legacy"]` | +/// | `[cdi-ids…]` | unchanged | +pub(crate) fn resolve_gpu_device_ids(gpu: &[String], cdi_enabled: bool) -> Vec { + match gpu { + [] => vec![], + [v] if v == "auto" => { + if cdi_enabled { + vec!["nvidia.com/gpu=all".to_string()] + } else { + vec!["legacy".to_string()] + } + } + other => other.to_vec(), + } +} + const REGISTRY_MODE_EXTERNAL: &str = "external"; fn env_non_empty(key: &str) -> Option { @@ -454,7 +477,7 @@ pub async fn ensure_container( disable_gateway_auth: bool, registry_username: Option<&str>, registry_token: Option<&str>, - gpu: bool, + device_ids: &[String], ) -> Result<()> { let container_name = container_name(name); @@ -542,21 +565,35 @@ pub async fn ensure_container( ..Default::default() }; - // When GPU support is requested, add NVIDIA device requests. - // This is the programmatic equivalent of `docker run --gpus all`. - // The NVIDIA Container Toolkit runtime hook injects /dev/nvidia* devices - // and GPU driver libraries from the host into the container. - if gpu { - host_config.device_requests = Some(vec![DeviceRequest { - driver: Some("nvidia".to_string()), - count: Some(-1), // all GPUs - capabilities: Some(vec![vec![ - "gpu".to_string(), - "utility".to_string(), - "compute".to_string(), - ]]), - ..Default::default() - }]); + // Inject GPU devices into the container based on the resolved device ID list. + // + // The list is pre-resolved by `resolve_gpu_device_ids` before reaching here: + // [] — no GPU passthrough + // ["legacy"] — internal non-CDI fallback path: `driver="nvidia"`, + // `count=-1`; relies on the NVIDIA Container Runtime hook + // [cdi-ids…] — CDI DeviceRequest (driver="cdi") with the given device IDs; + // Docker resolves them against the host CDI spec at /etc/cdi/ + match device_ids { + [] => {} + [id] if id == "legacy" => { + host_config.device_requests = Some(vec![DeviceRequest { + driver: Some("nvidia".to_string()), + count: Some(-1), // all GPUs + capabilities: Some(vec![vec![ + "gpu".to_string(), + "utility".to_string(), + "compute".to_string(), + ]]), + ..Default::default() + }]); + } + ids => { + host_config.device_requests = Some(vec![DeviceRequest { + driver: Some("cdi".to_string()), + device_ids: Some(ids.to_vec()), + ..Default::default() + }]); + } } let mut cmd = vec![ @@ -671,7 +708,7 @@ pub async fn ensure_container( // GPU support: tell the entrypoint to deploy the NVIDIA device plugin // HelmChart CR so k8s workloads can request nvidia.com/gpu resources. - if gpu { + if !device_ids.is_empty() { env_vars.push("GPU_ENABLED=true".to_string()); } @@ -1195,4 +1232,53 @@ mod tests { "should return a reasonable number of sockets" ); } + + // --- resolve_gpu_device_ids --- + + #[test] + fn resolve_gpu_empty_returns_empty() { + assert_eq!(resolve_gpu_device_ids(&[], true), Vec::::new()); + assert_eq!(resolve_gpu_device_ids(&[], false), Vec::::new()); + } + + #[test] + fn resolve_gpu_auto_cdi_enabled() { + assert_eq!( + resolve_gpu_device_ids(&["auto".to_string()], true), + vec!["nvidia.com/gpu=all"], + ); + } + + #[test] + fn resolve_gpu_auto_cdi_disabled() { + assert_eq!( + resolve_gpu_device_ids(&["auto".to_string()], false), + vec!["legacy"], + ); + } + + #[test] + fn resolve_gpu_legacy_passthrough() { + assert_eq!( + resolve_gpu_device_ids(&["legacy".to_string()], true), + vec!["legacy"], + ); + assert_eq!( + resolve_gpu_device_ids(&["legacy".to_string()], false), + vec!["legacy"], + ); + } + + #[test] + fn resolve_gpu_cdi_ids_passthrough() { + let ids = vec!["nvidia.com/gpu=all".to_string()]; + assert_eq!(resolve_gpu_device_ids(&ids, true), ids); + assert_eq!(resolve_gpu_device_ids(&ids, false), ids); + + let multi = vec![ + "nvidia.com/gpu=0".to_string(), + "nvidia.com/gpu=1".to_string(), + ]; + assert_eq!(resolve_gpu_device_ids(&multi, true), multi); + } } diff --git a/crates/openshell-bootstrap/src/lib.rs b/crates/openshell-bootstrap/src/lib.rs index 938986757..7dcabe052 100644 --- a/crates/openshell-bootstrap/src/lib.rs +++ b/crates/openshell-bootstrap/src/lib.rs @@ -31,7 +31,8 @@ use crate::constants::{ }; use crate::docker::{ check_existing_gateway, check_port_conflicts, destroy_gateway_resources, ensure_container, - ensure_image, ensure_network, ensure_volume, start_container, stop_container, + ensure_image, ensure_network, ensure_volume, resolve_gpu_device_ids, start_container, + stop_container, }; use crate::metadata::{ create_gateway_metadata, create_gateway_metadata_with_host, local_gateway_host, @@ -111,10 +112,13 @@ pub struct DeployOptions { /// bootstrap pull and inside the k3s cluster at runtime. Only needed /// for private registries. pub registry_token: Option, - /// Enable NVIDIA GPU passthrough. When true, the Docker container is - /// created with GPU device requests (`--gpus all`) and the NVIDIA - /// k8s-device-plugin is deployed inside the k3s cluster. - pub gpu: bool, + /// GPU device IDs to inject into the gateway container. + /// + /// - `[]` — no GPU passthrough (default) + /// - `["legacy"]` — internal non-CDI fallback path (`driver="nvidia"`, `count=-1`) + /// - `["auto"]` — resolved at deploy time: CDI if enabled on the daemon, else the non-CDI fallback + /// - `[cdi-ids…]` — CDI DeviceRequest with the given device IDs + pub gpu: Vec, /// When true, destroy any existing gateway resources before deploying. /// When false, an existing gateway is left as-is and deployment is /// skipped (the caller is responsible for prompting the user first). @@ -133,7 +137,7 @@ impl DeployOptions { disable_gateway_auth: false, registry_username: None, registry_token: None, - gpu: false, + gpu: vec![], recreate: false, } } @@ -187,9 +191,13 @@ impl DeployOptions { self } - /// Enable NVIDIA GPU passthrough for the cluster container. + /// Set GPU device IDs for the cluster container. + /// + /// Pass `vec!["auto"]` to auto-select between CDI and the non-CDI fallback + /// based on daemon capabilities at deploy time. The `legacy` sentinel is an + /// internal implementation detail for the fallback path. #[must_use] - pub fn with_gpu(mut self, gpu: bool) -> Self { + pub fn with_gpu(mut self, gpu: Vec) -> Self { self.gpu = gpu; self } @@ -288,6 +296,18 @@ where (preflight.docker, None) }; + // CDI is considered enabled when the daemon reports at least one CDI spec + // directory via `GET /info` (`SystemInfo.CDISpecDirs`). An empty list or + // missing field means CDI is not configured and we fall back to the legacy + // NVIDIA `DeviceRequest` (driver="nvidia"). Detection is best-effort — + // failure to query daemon info is non-fatal. + let cdi_supported = target_docker + .info() + .await + .ok() + .and_then(|info| info.cdi_spec_dirs) + .is_some_and(|dirs| !dirs.is_empty()); + // If an existing gateway is found, either tear it down (when recreate is // requested) or bail out so the caller can prompt the user / reuse it. if let Some(existing) = check_existing_gateway(&target_docker, &name).await? { @@ -405,6 +425,7 @@ where // leaving an orphaned volume in a corrupted state that blocks retries. // See: https://github.com/NVIDIA/OpenShell/issues/463 let deploy_result: Result = async { + let device_ids = resolve_gpu_device_ids(&gpu, cdi_supported); ensure_container( &target_docker, &name, @@ -416,7 +437,7 @@ where disable_gateway_auth, registry_username.as_deref(), registry_token.as_deref(), - gpu, + &device_ids, ) .await?; start_container(&target_docker, &name).await?; diff --git a/crates/openshell-cli/src/bootstrap.rs b/crates/openshell-cli/src/bootstrap.rs index e976061fa..ea6410b91 100644 --- a/crates/openshell-cli/src/bootstrap.rs +++ b/crates/openshell-cli/src/bootstrap.rs @@ -178,7 +178,11 @@ pub async fn run_bootstrap( { options = options.with_gateway_host(host); } - options = options.with_gpu(gpu); + options = options.with_gpu(if gpu { + vec!["auto".to_string()] + } else { + vec![] + }); let handle = deploy_gateway_with_panel(options, &gateway_name, location).await?; let server = handle.gateway_endpoint().to_string(); diff --git a/crates/openshell-cli/src/main.rs b/crates/openshell-cli/src/main.rs index df37410b6..4f4d49695 100644 --- a/crates/openshell-cli/src/main.rs +++ b/crates/openshell-cli/src/main.rs @@ -807,6 +807,10 @@ enum GatewayCommands { /// NVIDIA k8s-device-plugin so Kubernetes workloads can request /// `nvidia.com/gpu` resources. Requires NVIDIA drivers and the /// NVIDIA Container Toolkit on the host. + /// + /// When enabled, OpenShell auto-selects CDI when the Docker daemon has + /// CDI enabled and falls back to Docker's NVIDIA GPU request path + /// (`--gpus all`) otherwise. #[arg(long)] gpu: bool, }, @@ -1112,8 +1116,10 @@ enum SandboxCommands { /// Request GPU resources for the sandbox. /// /// When no gateway is running, auto-bootstrap starts a GPU-enabled - /// gateway. GPU intent is also inferred automatically for known - /// GPU-designated image names such as `nvidia-gpu`. + /// gateway using the same automatic injection selection as + /// `openshell gateway start --gpu`. GPU intent is also inferred + /// automatically for known GPU-designated image names such as + /// `nvidia-gpu`. #[arg(long)] gpu: bool, @@ -1570,6 +1576,11 @@ async fn main() -> Result<()> { registry_token, gpu, } => { + let gpu = if gpu { + vec!["auto".to_string()] + } else { + vec![] + }; run::gateway_admin_deploy( &name, remote.as_deref(), diff --git a/crates/openshell-cli/src/run.rs b/crates/openshell-cli/src/run.rs index bab819137..67eafc886 100644 --- a/crates/openshell-cli/src/run.rs +++ b/crates/openshell-cli/src/run.rs @@ -1355,7 +1355,7 @@ pub async fn gateway_admin_deploy( disable_gateway_auth: bool, registry_username: Option<&str>, registry_token: Option<&str>, - gpu: bool, + gpu: Vec, ) -> Result<()> { let location = if remote.is_some() { "remote" } else { "local" }; diff --git a/docs/sandboxes/manage-gateways.md b/docs/sandboxes/manage-gateways.md index 2f3dba7a9..d4d9ccf55 100644 --- a/docs/sandboxes/manage-gateways.md +++ b/docs/sandboxes/manage-gateways.md @@ -168,7 +168,7 @@ $ openshell gateway info --name my-remote-cluster | Flag | Purpose | |---|---| -| `--gpu` | Enable NVIDIA GPU passthrough. Requires NVIDIA drivers and the Container Toolkit on the host. | +| `--gpu` | Enable NVIDIA GPU passthrough. Requires NVIDIA drivers and the Container Toolkit on the host. OpenShell auto-selects CDI when enabled on the daemon and falls back to Docker's NVIDIA GPU request path (`--gpus all`) otherwise. | | `--plaintext` | Listen on HTTP instead of mTLS. Use behind a TLS-terminating reverse proxy. | | `--disable-gateway-auth` | Skip mTLS client certificate checks. Use when a reverse proxy cannot forward client certs. | | `--registry-username` | Username for registry authentication. Defaults to `__token__` when `--registry-token` is set. Only needed for private registries. Also configurable with `OPENSHELL_REGISTRY_USERNAME`. | diff --git a/e2e/rust/tests/cli_smoke.rs b/e2e/rust/tests/cli_smoke.rs index 35b2801c9..0abc24b43 100644 --- a/e2e/rust/tests/cli_smoke.rs +++ b/e2e/rust/tests/cli_smoke.rs @@ -122,17 +122,22 @@ async fn sandbox_connect_help_shows_editor_flag() { ); } -/// `openshell gateway start --help` must show `--recreate`. +/// `openshell gateway start --help` must show key flags. #[tokio::test] -async fn gateway_start_help_shows_recreate() { +async fn gateway_start_help_shows_key_flags() { let (output, code) = run_isolated(&["gateway", "start", "--help"]).await; assert_eq!(code, 0, "openshell gateway start --help should exit 0"); let clean = strip_ansi(&output); - assert!( - clean.contains("--recreate"), - "expected '--recreate' in gateway start --help:\n{clean}" - ); + for flag in [ + "--gpu", + "--recreate", + ] { + assert!( + clean.contains(flag), + "expected '{flag}' in gateway start --help:\n{clean}" + ); + } } // ------------------------------------------------------------------- From 122bc74948a89f02dd6c042bacd96ba18c0e60da Mon Sep 17 00:00:00 2001 From: Evan Lezar Date: Tue, 31 Mar 2026 17:35:39 +0200 Subject: [PATCH 14/20] feat(sandbox): switch device plugin to CDI injection mode (#503) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(sandbox): switch device plugin to CDI injection mode Configure the NVIDIA device plugin to use deviceListStrategy=cdi-cri so that GPU devices are injected via direct CDI device requests in the CRI. Sandbox pods now only require the nvidia.com/gpu resource request — runtimeClassName is no longer set on GPU pods. Signed-off-by: Evan Lezar --- .../skills/debug-openshell-cluster/SKILL.md | 44 +++++++++- architecture/gateway-single-node.md | 8 +- crates/openshell-server/src/sandbox/mod.rs | 87 +++++++++++++------ deploy/docker/Dockerfile.images | 16 ++-- .../nvidia-device-plugin-helmchart.yaml | 9 ++ 5 files changed, 122 insertions(+), 42 deletions(-) diff --git a/.agents/skills/debug-openshell-cluster/SKILL.md b/.agents/skills/debug-openshell-cluster/SKILL.md index 5af8895cf..4ef851a7e 100644 --- a/.agents/skills/debug-openshell-cluster/SKILL.md +++ b/.agents/skills/debug-openshell-cluster/SKILL.md @@ -257,7 +257,43 @@ Look for: - `OOMKilled` — memory limits too low - `FailedMount` — volume issues -### Step 8: Check DNS Resolution +### Step 8: Check GPU Device Plugin and CDI (GPU gateways only) + +Skip this step for non-GPU gateways. + +The NVIDIA device plugin DaemonSet must be running and healthy before GPU sandboxes can be created. It uses CDI injection (`deviceListStrategy: cdi-cri`) to inject GPU devices into sandbox pods — no `runtimeClassName` is set on sandbox pods. + +```bash +# DaemonSet status — numberReady must be >= 1 +openshell doctor exec -- kubectl get daemonset -n nvidia-device-plugin + +# Device plugin pod logs — look for "CDI" lines confirming CDI mode is active +openshell doctor exec -- kubectl logs -n nvidia-device-plugin -l app.kubernetes.io/name=nvidia-device-plugin --tail=50 + +# List CDI devices registered by the device plugin (requires nvidia-ctk in the cluster image). +# Device plugin CDI entries use the vendor string "k8s.device-plugin.nvidia.com" so entries +# will be prefixed "k8s.device-plugin.nvidia.com/gpu=". If the list is empty, CDI spec +# generation has not completed yet. +openshell doctor exec -- nvidia-ctk cdi list + +# Verify CDI spec files were generated on the node +openshell doctor exec -- ls /var/run/cdi/ + +# Helm install job logs for the device plugin chart +openshell doctor exec -- kubectl -n kube-system logs -l job-name=helm-install-nvidia-device-plugin --tail=100 + +# Confirm a GPU sandbox pod has no runtimeClassName (CDI injection, not runtime class) +openshell doctor exec -- kubectl get pod -n openshell -o jsonpath='{range .items[*]}{.metadata.name}{" runtimeClassName="}{.spec.runtimeClassName}{"\n"}{end}' +``` + +Common issues: + +- **DaemonSet 0/N ready**: The device plugin chart may still be deploying (k3s Helm controller can take 1–2 min) or the pod is crashing. Check pod logs. +- **`nvidia-ctk cdi list` returns no `k8s.device-plugin.nvidia.com/gpu=` entries**: CDI spec generation has not completed. The device plugin may still be starting or the `cdi-cri` strategy isn't active. Verify `deviceListStrategy: cdi-cri` is in the rendered Helm values. +- **No CDI spec files at `/var/run/cdi/`**: Same as above — device plugin hasn't written CDI specs yet. +- **`HEALTHCHECK_GPU_DEVICE_PLUGIN_NOT_READY` in health check logs**: Device plugin has no ready pods. Check DaemonSet events and pod logs. + +### Step 9: Check DNS Resolution DNS misconfiguration is a common root cause, especially on remote/Linux hosts: @@ -317,6 +353,7 @@ If DNS is broken, all image pulls from the distribution registry will fail, as w | gRPC `UNIMPLEMENTED` for newer RPCs in push mode | Helm values still point at older pulled images instead of the pushed refs | Verify rendered `openshell-helmchart.yaml` uses the expected push refs (`server`, `sandbox`, `pki-job`) and not `:latest` | | Sandbox pods crash with `/opt/openshell/bin/openshell-sandbox: no such file or directory` | Supervisor binary missing from cluster image | The cluster image was built/published without the `supervisor-builder` target in `deploy/docker/Dockerfile.images`. Rebuild with `mise run docker:build:cluster` and recreate gateway. Bootstrap auto-detects via `HEALTHCHECK_MISSING_SUPERVISOR` marker | | `HEALTHCHECK_MISSING_SUPERVISOR` in health check logs | `/opt/openshell/bin/openshell-sandbox` not found in gateway container | Rebuild cluster image: `mise run docker:build:cluster`, then `openshell gateway destroy && openshell gateway start` | +| `nvidia-ctk cdi list` returns no `k8s.device-plugin.nvidia.com/gpu=` entries | CDI specs not yet generated by device plugin | Device plugin may still be starting; wait and retry, or check pod logs (Step 8) | ## Full Diagnostic Dump @@ -370,4 +407,9 @@ openshell doctor exec -- ls -la /opt/openshell/bin/openshell-sandbox echo "=== DNS Configuration ===" openshell doctor exec -- cat /etc/rancher/k3s/resolv.conf + +# GPU gateways only +echo "=== GPU Device Plugin ===" +openshell doctor exec -- kubectl get daemonset -n nvidia-device-plugin +openshell doctor exec -- nvidia-ctk cdi list ``` diff --git a/architecture/gateway-single-node.md b/architecture/gateway-single-node.md index c417775a5..0921d3aaa 100644 --- a/architecture/gateway-single-node.md +++ b/architecture/gateway-single-node.md @@ -302,7 +302,7 @@ GPU support is part of the single-node gateway bootstrap path rather than a sepa - **CDI not enabled**: `--gpus all` device request — `driver="nvidia"`, `count=-1`, which relies on the NVIDIA Container Runtime hook. - `deploy/docker/Dockerfile.images` installs NVIDIA Container Toolkit packages in a dedicated Ubuntu stage and copies the runtime binaries, config, and `libnvidia-container` shared libraries into the final Ubuntu-based cluster image. - `deploy/docker/cluster-entrypoint.sh` checks `GPU_ENABLED=true` and copies GPU-only manifests from `/opt/openshell/gpu-manifests/` into k3s's manifests directory. -- `deploy/kube/gpu-manifests/nvidia-device-plugin-helmchart.yaml` installs the NVIDIA device plugin chart, currently pinned to `0.18.2`. NFD and GFD are disabled; the device plugin's default `nodeAffinity` (which requires `feature.node.kubernetes.io/pci-10de.present=true` or `nvidia.com/gpu.present=true` from NFD/GFD) is overridden to empty so the DaemonSet schedules on the single-node cluster without requiring those labels. +- `deploy/kube/gpu-manifests/nvidia-device-plugin-helmchart.yaml` installs the NVIDIA device plugin chart, currently pinned to `0.18.2`. NFD and GFD are disabled; the device plugin's default `nodeAffinity` (which requires `feature.node.kubernetes.io/pci-10de.present=true` or `nvidia.com/gpu.present=true` from NFD/GFD) is overridden to empty so the DaemonSet schedules on the single-node cluster without requiring those labels. The chart is configured with `deviceListStrategy: cdi-cri` so the device plugin injects devices via direct CDI device requests in the CRI. - k3s auto-detects `nvidia-container-runtime` on `PATH`, registers the `nvidia` containerd runtime, and creates the `nvidia` `RuntimeClass` automatically. - The OpenShell Helm chart grants the gateway service account cluster-scoped read access to `node.k8s.io/runtimeclasses` and core `nodes` so GPU sandbox admission can verify both the `nvidia` `RuntimeClass` and allocatable GPU capacity before creating a sandbox. @@ -313,14 +313,16 @@ Host GPU drivers & NVIDIA Container Toolkit └─ Docker: DeviceRequests (CDI when enabled, --gpus all otherwise) └─ k3s/containerd: nvidia-container-runtime on PATH -> auto-detected └─ k8s: nvidia-device-plugin DaemonSet advertises nvidia.com/gpu - └─ Pods: request nvidia.com/gpu in resource limits + └─ Pods: request nvidia.com/gpu in resource limits (CDI injection — no runtimeClassName needed) ``` ### `--gpu` flag The `--gpu` flag on `gateway start` enables GPU passthrough. OpenShell auto-selects CDI when enabled on the daemon and falls back to Docker's NVIDIA GPU request path (`--gpus all`) otherwise. -The expected smoke test is a plain pod requesting `nvidia.com/gpu: 1` with `runtimeClassName: nvidia` and running `nvidia-smi`. +Device injection uses CDI (`deviceListStrategy: cdi-cri`): the device plugin injects devices via direct CDI device requests in the CRI. Sandbox pods only need `nvidia.com/gpu: 1` in their resource limits, and GPU pods do not set `runtimeClassName`. + +The expected smoke test is a plain pod requesting `nvidia.com/gpu: 1` without `runtimeClassName` and running `nvidia-smi`. ## Remote Image Transfer diff --git a/crates/openshell-server/src/sandbox/mod.rs b/crates/openshell-server/src/sandbox/mod.rs index e10b33d0c..3dca66493 100644 --- a/crates/openshell-server/src/sandbox/mod.rs +++ b/crates/openshell-server/src/sandbox/mod.rs @@ -31,7 +31,6 @@ pub const SANDBOX_KIND: &str = "Sandbox"; const SANDBOX_ID_LABEL: &str = "openshell.ai/sandbox-id"; const SANDBOX_MANAGED_LABEL: &str = "openshell.ai/managed-by"; const SANDBOX_MANAGED_VALUE: &str = "openshell"; -const GPU_RUNTIME_CLASS_NAME: &str = "nvidia"; const GPU_RESOURCE_NAME: &str = "nvidia.com/gpu"; const GPU_RESOURCE_QUANTITY: &str = "1"; @@ -127,25 +126,6 @@ impl SandboxClient { } pub async fn validate_gpu_support(&self) -> Result<(), tonic::Status> { - let runtime_classes: Api = Api::all_with( - self.client.clone(), - &ApiResource::from_gvk(&GroupVersionKind::gvk("node.k8s.io", "v1", "RuntimeClass")), - ); - - let runtime_class_exists = runtime_classes - .get_opt(GPU_RUNTIME_CLASS_NAME) - .await - .map_err(|err| { - tonic::Status::internal(format!("check GPU runtime class failed: {err}")) - })? - .is_some(); - - if !runtime_class_exists { - return Err(tonic::Status::failed_precondition( - "GPU sandbox requested, but the active gateway is not GPU-enabled. To start a gateway with GPU support run: `openshell gateway start --gpu`", - )); - } - let nodes: Api = Api::all(self.client.clone()); let node_list = nodes.list(&ListParams::default()).await.map_err(|err| { tonic::Status::internal(format!("check GPU node capacity failed: {err}")) @@ -869,12 +849,7 @@ fn sandbox_template_to_k8s( } let mut spec = serde_json::Map::new(); - if gpu { - spec.insert( - "runtimeClassName".to_string(), - serde_json::json!(GPU_RUNTIME_CLASS_NAME), - ); - } else if !template.runtime_class_name.is_empty() { + if !template.runtime_class_name.is_empty() { spec.insert( "runtimeClassName".to_string(), serde_json::json!(template.runtime_class_name), @@ -1660,7 +1635,7 @@ mod tests { assert_eq!( pod_template["spec"]["runtimeClassName"], - serde_json::json!(GPU_RUNTIME_CLASS_NAME) + serde_json::Value::Null ); assert_eq!( pod_template["spec"]["containers"][0]["resources"]["limits"][GPU_RESOURCE_NAME], @@ -1668,6 +1643,64 @@ mod tests { ); } + #[test] + fn gpu_sandbox_uses_template_runtime_class_name_when_set() { + let template = SandboxTemplate { + runtime_class_name: "kata-containers".to_string(), + ..SandboxTemplate::default() + }; + + let pod_template = sandbox_template_to_k8s( + &template, + true, + "openshell/sandbox:latest", + "", + "sandbox-id", + "sandbox-name", + "https://gateway.example.com", + "0.0.0.0:2222", + "secret", + 300, + &std::collections::HashMap::new(), + "", + "", + ); + + assert_eq!( + pod_template["spec"]["runtimeClassName"], + serde_json::json!("kata-containers") + ); + } + + #[test] + fn non_gpu_sandbox_uses_template_runtime_class_name_when_set() { + let template = SandboxTemplate { + runtime_class_name: "kata-containers".to_string(), + ..SandboxTemplate::default() + }; + + let pod_template = sandbox_template_to_k8s( + &template, + false, + "openshell/sandbox:latest", + "", + "sandbox-id", + "sandbox-name", + "https://gateway.example.com", + "0.0.0.0:2222", + "secret", + 300, + &std::collections::HashMap::new(), + "", + "", + ); + + assert_eq!( + pod_template["spec"]["runtimeClassName"], + serde_json::json!("kata-containers") + ); + } + #[test] fn gpu_sandbox_preserves_existing_resource_limits() { let template = SandboxTemplate { diff --git a/deploy/docker/Dockerfile.images b/deploy/docker/Dockerfile.images index 9cc50085c..6bda277b4 100644 --- a/deploy/docker/Dockerfile.images +++ b/deploy/docker/Dockerfile.images @@ -201,7 +201,7 @@ RUN apt-get update && apt-get install -y --no-install-recommends curl ca-certifi chmod +x /tmp/helm && \ rm -rf /var/lib/apt/lists/* -FROM ubuntu:24.04 AS nvidia-toolkit +FROM ubuntu:24.04 AS nvidia-container-toolkit ARG NVIDIA_CONTAINER_TOOLKIT_VERSION RUN apt-get update && apt-get install -y --no-install-recommends \ @@ -213,10 +213,7 @@ RUN apt-get update && apt-get install -y --no-install-recommends \ | tee /etc/apt/sources.list.d/nvidia-container-toolkit.list && \ apt-get update && \ apt-get install -y --no-install-recommends \ - "nvidia-container-toolkit=${NVIDIA_CONTAINER_TOOLKIT_VERSION}" \ "nvidia-container-toolkit-base=${NVIDIA_CONTAINER_TOOLKIT_VERSION}" \ - "libnvidia-container-tools=${NVIDIA_CONTAINER_TOOLKIT_VERSION}" \ - "libnvidia-container1=${NVIDIA_CONTAINER_TOOLKIT_VERSION}" && \ rm -rf /var/lib/apt/lists/* # --------------------------------------------------------------------------- @@ -240,13 +237,10 @@ COPY --from=k3s /usr/share/zoneinfo/ /usr/share/zoneinfo/ ENV PATH="/var/lib/rancher/k3s/data/cni:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/bin/aux" \ CRI_CONFIG_FILE="/var/lib/rancher/k3s/agent/etc/crictl.yaml" -COPY --from=nvidia-toolkit /usr/bin/nvidia-cdi-hook /usr/bin/ -COPY --from=nvidia-toolkit /usr/bin/nvidia-container-runtime /usr/bin/ -COPY --from=nvidia-toolkit /usr/bin/nvidia-container-runtime-hook /usr/bin/ -COPY --from=nvidia-toolkit /usr/bin/nvidia-container-cli /usr/bin/ -COPY --from=nvidia-toolkit /usr/bin/nvidia-ctk /usr/bin/ -COPY --from=nvidia-toolkit /etc/nvidia-container-runtime /etc/nvidia-container-runtime -COPY --from=nvidia-toolkit /usr/lib/*-linux-gnu/libnvidia-container*.so* /usr/lib/ +COPY --from=nvidia-container-toolkit /usr/bin/nvidia-cdi-hook /usr/bin/ +COPY --from=nvidia-container-toolkit /usr/bin/nvidia-container-runtime /usr/bin/ +COPY --from=nvidia-container-toolkit /usr/bin/nvidia-ctk /usr/bin/ +COPY --from=nvidia-container-toolkit /etc/nvidia-container-runtime /etc/nvidia-container-runtime COPY --from=supervisor-builder /build/out/openshell-sandbox /opt/openshell/bin/openshell-sandbox RUN mkdir -p /var/lib/rancher/k3s/server/manifests \ diff --git a/deploy/kube/gpu-manifests/nvidia-device-plugin-helmchart.yaml b/deploy/kube/gpu-manifests/nvidia-device-plugin-helmchart.yaml index 088562ac9..1cb0ca70a 100644 --- a/deploy/kube/gpu-manifests/nvidia-device-plugin-helmchart.yaml +++ b/deploy/kube/gpu-manifests/nvidia-device-plugin-helmchart.yaml @@ -12,6 +12,10 @@ # (which requires nvidia.com/gpu.present=true) is overridden to empty # so it schedules on any node without requiring NFD/GFD labels. # +# CDI injection mode: the device plugin uses deviceListStrategy=cdi-cri so that +# devices are injected via CDI hooks before container start. Sandbox pods only +# need the nvidia.com/gpu resource request — no runtimeClassName is required. +# # k3s auto-detects nvidia-container-runtime on PATH and registers the "nvidia" # RuntimeClass automatically, so no manual RuntimeClass manifest is needed. @@ -28,6 +32,11 @@ spec: createNamespace: true valuesContent: |- runtimeClassName: nvidia + deviceListStrategy: cdi-cri + deviceIDStrategy: index + cdi: + nvidiaHookPath: /usr/bin/nvidia-cdi-hook + nvidiaDriverRoot: "/" gfd: enabled: false nfd: From 0eebbc840fa645a9414803a77774a71396275e89 Mon Sep 17 00:00:00 2001 From: Piotr Mlocek Date: Tue, 31 Mar 2026 08:55:31 -0700 Subject: [PATCH 15/20] fix(docker): restore apt cleanup chaining in cluster image (#702) --- deploy/docker/Dockerfile.images | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/deploy/docker/Dockerfile.images b/deploy/docker/Dockerfile.images index 6bda277b4..79c0d3e2a 100644 --- a/deploy/docker/Dockerfile.images +++ b/deploy/docker/Dockerfile.images @@ -213,7 +213,7 @@ RUN apt-get update && apt-get install -y --no-install-recommends \ | tee /etc/apt/sources.list.d/nvidia-container-toolkit.list && \ apt-get update && \ apt-get install -y --no-install-recommends \ - "nvidia-container-toolkit-base=${NVIDIA_CONTAINER_TOOLKIT_VERSION}" \ + "nvidia-container-toolkit-base=${NVIDIA_CONTAINER_TOOLKIT_VERSION}" && \ rm -rf /var/lib/apt/lists/* # --------------------------------------------------------------------------- From 2538bead50269794f7b000f895e0b8703e7b7907 Mon Sep 17 00:00:00 2001 From: Drew Newberry Date: Tue, 31 Mar 2026 09:35:31 -0700 Subject: [PATCH 16/20] fix(cluster): pass resolv-conf as kubelet arg and pin k3s image digest (#701) --- architecture/gateway-single-node.md | 2 +- deploy/docker/Dockerfile.images | 11 ++++++++++- deploy/docker/cluster-entrypoint.sh | 8 +++++--- 3 files changed, 16 insertions(+), 5 deletions(-) diff --git a/architecture/gateway-single-node.md b/architecture/gateway-single-node.md index 0921d3aaa..f46a50d2d 100644 --- a/architecture/gateway-single-node.md +++ b/architecture/gateway-single-node.md @@ -260,7 +260,7 @@ On Docker custom networks, `/etc/resolv.conf` contains `127.0.0.11` (Docker's in 2. Getting the container's `eth0` IP as a routable address. 3. Adding DNAT rules in PREROUTING to forward DNS from pod namespaces through to Docker's DNS. 4. Writing a custom resolv.conf pointing to the container IP. -5. Passing `--resolv-conf=/etc/rancher/k3s/resolv.conf` to k3s. +5. Passing `--kubelet-arg=resolv-conf=/etc/rancher/k3s/resolv.conf` to k3s. Falls back to `8.8.8.8` / `8.8.4.4` if iptables detection fails. diff --git a/deploy/docker/Dockerfile.images b/deploy/docker/Dockerfile.images index 79c0d3e2a..af17b9b0a 100644 --- a/deploy/docker/Dockerfile.images +++ b/deploy/docker/Dockerfile.images @@ -12,7 +12,11 @@ # supervisor-builder Release openshell-sandbox binary # supervisor-output Minimal stage exporting only the supervisor binary +# Pin by tag AND manifest-list digest to prevent silent upstream republishes +# from breaking the build. Update both when bumping k3s versions. +# To refresh: docker buildx imagetools inspect rancher/k3s: | head -3 ARG K3S_VERSION=v1.35.2-k3s1 +ARG K3S_DIGEST=sha256:c3184157c3048112bab0c3e17405991da486cb3413511eba23f7650efd70776b ARG K9S_VERSION=v0.50.18 ARG HELM_VERSION=v3.17.3 ARG NVIDIA_CONTAINER_TOOLKIT_VERSION=1.18.2-1 @@ -181,7 +185,7 @@ CMD ["--port", "8080"] # --------------------------------------------------------------------------- # Cluster asset stages # --------------------------------------------------------------------------- -FROM rancher/k3s:${K3S_VERSION} AS k3s +FROM rancher/k3s:${K3S_VERSION}@${K3S_DIGEST} AS k3s FROM ubuntu:24.04 AS k9s ARG K9S_VERSION @@ -262,6 +266,11 @@ COPY deploy/kube/manifests/*.yaml /opt/openshell/manifests/ COPY deploy/kube/gpu-manifests/*.yaml /opt/openshell/gpu-manifests/ ENTRYPOINT ["/usr/local/bin/cluster-entrypoint.sh"] +# Default to "server" so bare `docker run ` works without requiring +# the caller to pass a subcommand. The openshell CLI already passes +# ["server", "--disable=traefik", ...] as CMD; this default only affects +# manual `docker run` invocations that omit a command. +CMD ["server"] HEALTHCHECK --interval=5s --timeout=5s --start-period=20s --retries=60 \ CMD ["/usr/local/bin/cluster-healthcheck.sh"] diff --git a/deploy/docker/cluster-entrypoint.sh b/deploy/docker/cluster-entrypoint.sh index 2fea6fa61..d4717d88e 100644 --- a/deploy/docker/cluster-entrypoint.sh +++ b/deploy/docker/cluster-entrypoint.sh @@ -18,7 +18,7 @@ # embedded DNS resolver at 127.0.0.11. Docker's DNS listens on random high # ports (visible in the DOCKER_OUTPUT iptables chain), so we parse those ports # and set up DNAT rules to forward DNS traffic from k3s pods. We then point -# k3s's --resolv-conf at the container's routable eth0 IP. +# k3s's resolv-conf kubelet arg at the container's routable eth0 IP. # # Per k3s docs: "Manually specified resolver configuration files are not # subject to viability checks." @@ -562,6 +562,8 @@ fi # routing to settle first. wait_for_default_route -# Execute k3s with explicit resolv-conf. +# Execute k3s with explicit resolv-conf passed as a kubelet arg. +# k3s v1.35.2+ no longer accepts --resolv-conf as a top-level server flag; +# it must be passed via --kubelet-arg instead. # shellcheck disable=SC2086 -exec /bin/k3s "$@" --resolv-conf="$RESOLV_CONF" $EXTRA_KUBELET_ARGS +exec /bin/k3s "$@" --kubelet-arg=resolv-conf="$RESOLV_CONF" $EXTRA_KUBELET_ARGS From c567390dba4f3afc06443270f380a8faed01fa8c Mon Sep 17 00:00:00 2001 From: OpenClaw Agent Date: Tue, 31 Mar 2026 15:40:09 +0000 Subject: [PATCH 17/20] fix(cli): add Copilot variant to CliProviderType enum The provider backend (registry, discovery, policy) already supported copilot, but the CLI CliProviderType enum was missing the Copilot variant. This caused 'provider create --type copilot' to be rejected and 'sandbox create -- copilot' to fail at the CLI layer. --- crates/openshell-cli/src/main.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/openshell-cli/src/main.rs b/crates/openshell-cli/src/main.rs index 4f4d49695..029f0c27d 100644 --- a/crates/openshell-cli/src/main.rs +++ b/crates/openshell-cli/src/main.rs @@ -597,6 +597,7 @@ enum CliProviderType { Claude, Opencode, Codex, + Copilot, Generic, Openai, Anthropic, @@ -627,6 +628,7 @@ impl CliProviderType { Self::Claude => "claude", Self::Opencode => "opencode", Self::Codex => "codex", + Self::Copilot => "copilot", Self::Generic => "generic", Self::Openai => "openai", Self::Anthropic => "anthropic", From 4b8361cf1a731ec4b2362caa8734307ca8cdda63 Mon Sep 17 00:00:00 2001 From: Hector Flores Date: Thu, 26 Mar 2026 17:47:17 +0000 Subject: [PATCH 18/20] =?UTF-8?q?feat(sandbox):=20L7=20credential=20inject?= =?UTF-8?q?ion=20=E2=80=94=20query=20param=20rewriting=20and=20Basic=20aut?= =?UTF-8?q?h=20encoding?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add two credential injection capabilities to the L7 proxy's SecretResolver: 1. Query parameter rewriting: resolve placeholder tokens in URL query parameter values (e.g. ?api_key=openshell:resolve:env:KEY) with proper percent-encoding of the resolved secret. 2. Basic Authorization header encoding: decode base64 Basic auth tokens, resolve placeholder tokens in the decoded username:password string, and re-encode before forwarding upstream. Both features operate within the existing rewrite_http_header_block flow and require no changes to the network policy file spec or proto schema. Closes #630 --- crates/openshell-sandbox/Cargo.toml | 3 + crates/openshell-sandbox/src/secrets.rs | 344 +++++++++++++++++++++++- 2 files changed, 346 insertions(+), 1 deletion(-) 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/secrets.rs b/crates/openshell-sandbox/src/secrets.rs index 4ee1ee846..efd917f27 100644 --- a/crates/openshell-sandbox/src/secrets.rs +++ b/crates/openshell-sandbox/src/secrets.rs @@ -1,6 +1,7 @@ // 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; const PLACEHOLDER_PREFIX: &str = "openshell:resolve:env:"; @@ -35,17 +36,59 @@ impl SecretResolver { } 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`). + // Decode, rewrite placeholders in the decoded string, re-encode. + if let Some(encoded) = 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()) { + 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 { @@ -68,7 +111,7 @@ pub(crate) fn rewrite_http_header_block(raw: &[u8], resolver: Option<&SecretReso }; let mut output = Vec::with_capacity(raw.len()); - output.extend_from_slice(request_line.as_bytes()); + output.extend_from_slice(rewrite_request_line(request_line, resolver).as_bytes()); output.extend_from_slice(b"\r\n"); for line in lines { @@ -96,6 +139,117 @@ pub(crate) fn rewrite_header_line(line: &str, resolver: &SecretResolver) -> Stri } } +/// Rewrite credential placeholders in the request line's URL query parameters. +/// +/// Given a request line like `GET /api?key=openshell:resolve:env:API_KEY HTTP/1.1`, +/// resolves placeholders in query parameter values and percent-encodes the +/// resolved secret. Handles URLs with multiple query parameters and preserves +/// parameters that don't contain placeholders. +fn rewrite_request_line(line: &str, resolver: &SecretResolver) -> String { + // 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 line.to_string(), + }; + let uri = match parts.next() { + Some(u) => u, + None => return line.to_string(), + }; + let version = match parts.next() { + Some(v) => v, + None => return line.to_string(), + }; + + // Only rewrite if the URI contains a placeholder + if !uri.contains(PLACEHOLDER_PREFIX) { + return line.to_string(); + } + + let rewritten_uri = rewrite_uri_query_params(uri, resolver); + format!("{method} {rewritten_uri} {version}") +} + +/// Rewrite placeholders in query parameter values of a URI. +/// +/// Splits the URI at `?`, parses key=value pairs from the query string, +/// resolves any placeholder values, and percent-encodes the resolved secrets. +/// Parameters without placeholders are preserved verbatim. +fn rewrite_uri_query_params(uri: &str, resolver: &SecretResolver) -> String { + let Some((path, query)) = uri.split_once('?') else { + return uri.to_string(); + }; + + let mut rewritten_params = Vec::new(); + for param in query.split('&') { + if let Some((key, value)) = param.split_once('=') { + // Percent-decode the value before checking for placeholder + let decoded_value = percent_decode(value); + if let Some(secret) = resolver.resolve_placeholder(&decoded_value) { + rewritten_params.push(format!("{key}={}", percent_encode(secret))); + } else { + rewritten_params.push(param.to_string()); + } + } else { + rewritten_params.push(param.to_string()); + } + } + + format!("{path}?{}", rewritten_params.join("&")) +} + +/// 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(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); + } + _ => { + encoded.push_str(&format!("%{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() +} + #[cfg(test)] mod tests { use super::*; @@ -259,4 +413,192 @@ mod tests { let rewritten = rewrite_http_header_block(raw, None); assert_eq!(raw.as_slice(), rewritten.as_slice()); } + + // --- Query parameter rewriting tests --- + + #[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 rewritten = rewrite_http_header_block(raw.as_bytes(), resolver.as_ref()); + let rewritten = String::from_utf8(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 rewritten = rewrite_http_header_block(raw.as_bytes(), resolver.as_ref()); + let rewritten = String::from_utf8(rewritten).expect("utf8"); + + // Secret should be percent-encoded + 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 rewritten = rewrite_http_header_block(raw.as_bytes(), resolver.as_ref()); + let rewritten = String::from_utf8(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 rewritten = rewrite_http_header_block(raw, resolver.as_ref()); + assert_eq!(raw.as_slice(), rewritten.as_slice()); + } + + // --- Basic Authorization header encoding tests --- + + #[test] + fn rewrites_basic_auth_placeholder_in_decoded_token() { + use base64::Engine as _; + 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(); + + // Simulate: agent constructs Basic auth with placeholder password + 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); + + // Decode the rewritten token to verify + 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"); + + // Normal Basic auth token without any placeholder + use base64::Engine as _; + 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() { + use base64::Engine as _; + 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 rewritten = rewrite_http_header_block(raw.as_bytes(), resolver.as_ref()); + let rewritten = String::from_utf8(rewritten).expect("utf8"); + + // Extract and decode the rewritten Basic token + 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"); + + // Other headers preserved + 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 --- + + #[test] + fn percent_encode_preserves_unreserved() { + assert_eq!(percent_encode("abc123-._~"), "abc123-._~"); + } + + #[test] + fn percent_encode_encodes_special_chars() { + assert_eq!(percent_encode("a b"), "a%20b"); + assert_eq!(percent_encode("key=val&x"), "key%3Dval%26x"); + } + + #[test] + fn percent_decode_round_trips() { + let original = "hello world & more=stuff"; + let encoded = percent_encode(original); + let decoded = percent_decode(&encoded); + assert_eq!(decoded, original); + } } From 546490dc02f9197a61f984c5a7888569b4b51007 Mon Sep 17 00:00:00 2001 From: Hector Flores Date: Sat, 28 Mar 2026 06:14:07 -0500 Subject: [PATCH 19/20] ci: add fork release workflow for CLI binary and gateway image --- .github/workflows/release-fork.yml | 136 +++++++++++++++++++++++++++++ crates/openshell-core/build.rs | 18 ++-- 2 files changed, 145 insertions(+), 9 deletions(-) create mode 100644 .github/workflows/release-fork.yml diff --git a/.github/workflows/release-fork.yml b/.github/workflows/release-fork.yml new file mode 100644 index 000000000..9f0d83884 --- /dev/null +++ b/.github/workflows/release-fork.yml @@ -0,0 +1,136 @@ +name: Release Fork + +on: + push: + branches: [feat/credential-injection-query-param-basic-auth] + workflow_dispatch: + +concurrency: + group: release-fork-${{ github.ref }} + cancel-in-progress: true + +permissions: + contents: write + packages: write + +env: + CARGO_TERM_COLOR: always + +jobs: + build-cli: + name: Build CLI (linux-amd64) + runs-on: ubuntu-latest + timeout-minutes: 30 + steps: + - uses: actions/checkout@v4 + with: + fetch-depth: 0 + + - name: Install Rust stable + uses: dtolnay/rust-toolchain@stable + + - name: Install protoc + uses: arduino/setup-protoc@v3 + with: + version: "29.x" + repo-token: ${{ secrets.GITHUB_TOKEN }} + + - name: Cache cargo registry and build + uses: actions/cache@v4 + with: + path: | + ~/.cargo/registry + ~/.cargo/git + target + key: cargo-cli-${{ runner.os }}-${{ hashFiles('**/Cargo.lock') }} + restore-keys: cargo-cli-${{ runner.os }}- + + - name: Build openshell CLI (release) + run: cargo build --release -p openshell-cli + + - name: Package binary + run: | + mkdir -p dist + cp target/release/openshell dist/ + cd dist + tar czf openshell-linux-amd64.tar.gz openshell + sha256sum openshell-linux-amd64.tar.gz > openshell-linux-amd64.tar.gz.sha256 + + - name: Upload artifact + uses: actions/upload-artifact@v4 + with: + name: openshell-linux-amd64 + path: | + dist/openshell-linux-amd64.tar.gz + dist/openshell-linux-amd64.tar.gz.sha256 + + build-gateway: + name: Build gateway Docker image + runs-on: ubuntu-latest + timeout-minutes: 45 + steps: + - uses: actions/checkout@v4 + with: + fetch-depth: 0 + + - name: Set up Docker Buildx + uses: docker/setup-buildx-action@v3 + + - name: Log in to GHCR + uses: docker/login-action@v3 + with: + registry: ghcr.io + username: ${{ github.actor }} + password: ${{ secrets.GITHUB_TOKEN }} + + - name: Build and push gateway image + uses: docker/build-push-action@v6 + with: + context: . + file: deploy/docker/Dockerfile.images + target: gateway + platforms: linux/amd64 + push: true + tags: | + ghcr.io/htekdev/openshell-gateway:latest + ghcr.io/htekdev/openshell-gateway:${{ github.sha }} + cache-from: type=gha + cache-to: type=gha,mode=max + + release: + name: Create GitHub Release + needs: [build-cli] + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - name: Download CLI artifact + uses: actions/download-artifact@v4 + with: + name: openshell-linux-amd64 + path: dist/ + + - name: Create or update release + uses: softprops/action-gh-release@v2 + with: + tag_name: fork-latest + name: "Fork Release (credential injection)" + body: | + Pre-built OpenShell fork with L7 credential injection including + query-param rewriting and Basic auth encoding. + + Branch: `feat/credential-injection-query-param-basic-auth` + Commit: ${{ github.sha }} + + **Changes:** Extends the L7 proxy to inject API credentials at the + network layer for arbitrary REST endpoints, with support for query + parameter injection and HTTP Basic authentication encoding. + + **Gateway image:** `ghcr.io/htekdev/openshell-gateway:latest` + draft: false + prerelease: true + make_latest: false + files: | + dist/openshell-linux-amd64.tar.gz + dist/openshell-linux-amd64.tar.gz.sha256 + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} diff --git a/crates/openshell-core/build.rs b/crates/openshell-core/build.rs index f44cdc75f..e604f066e 100644 --- a/crates/openshell-core/build.rs +++ b/crates/openshell-core/build.rs @@ -17,15 +17,15 @@ fn main() -> Result<(), Box> { } // --- Protobuf compilation --- - // Use bundled protoc from protobuf-src. The system protoc (from apt-get) - // does not bundle the well-known type includes (google/protobuf/struct.proto - // etc.), so we must use protobuf-src which ships both the binary and the - // include tree. - // SAFETY: This is run at build time in a single-threaded build script context. - // No other threads are reading environment variables concurrently. - #[allow(unsafe_code)] - unsafe { - env::set_var("PROTOC", protobuf_src::protoc()); + // Prefer PROTOC env var (e.g., from mise, setup-protoc action, or system + // install) when available. Fall back to bundled protoc from protobuf-src. + if env::var("PROTOC").is_err() { + // SAFETY: This is run at build time in a single-threaded build script context. + // No other threads are reading environment variables concurrently. + #[allow(unsafe_code)] + unsafe { + env::set_var("PROTOC", protobuf_src::protoc()); + } } let proto_files = [ From 0b2302bf17349818ef40c4a24f7cb83055adeefe Mon Sep 17 00:00:00 2001 From: OpenClaw Agent Date: Tue, 31 Mar 2026 17:47:06 +0000 Subject: [PATCH 20/20] chore: update Cargo.lock after merge --- .github/workflows/docs-preview-pr.yml | 1 - Cargo.lock | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/docs-preview-pr.yml b/.github/workflows/docs-preview-pr.yml index 6c0672ba2..ccade7411 100644 --- a/.github/workflows/docs-preview-pr.yml +++ b/.github/workflows/docs-preview-pr.yml @@ -49,7 +49,6 @@ jobs: find _build -name .buildinfo -exec rm {} \; - name: Deploy preview - if: github.event.pull_request.head.repo.full_name == github.repository uses: rossjrw/pr-preview-action@v1 with: source-dir: ./_build/docs/ 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",