* 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 <johntmyers@users.noreply.github.com>
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 <latenighthackathon@users.noreply.github.com>
Co-authored-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
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 <johntmyers@users.noreply.github.com>
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.
* 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 <latenighthackathon@users.noreply.github.com>
* 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 <latenighthackathon@users.noreply.github.com>
* style(proxy): apply cargo fmt formatting
Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
---------
Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
Co-authored-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
* 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 <latenighthackathon@users.noreply.github.com>
* 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 <latenighthackathon@users.noreply.github.com>
* style(l7): apply cargo fmt formatting
Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
---------
Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
Co-authored-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
Three issues prevent k3s from starting on kernels where the nf_tables
xt extension bridge (nft_compat) is unavailable:
1. kube-router's network policy controller uses the xt_comment iptables
extension and panics on startup with "Extension comment revision 0
not supported, missing kernel module?" Pass --disable-network-policy
to k3s so the controller never runs. The NSSH1 HMAC handshake remains
the primary sandbox SSH isolation boundary, so this does not weaken
the effective security posture.
2. flannel and kube-proxy also fail to insert rules via the nf_tables
iptables backend on the same kernels. Add an xt_comment probe at
cluster-entrypoint startup; if the probe fails, switch to
iptables-legacy via update-alternatives before any other netfilter
work so that flannel, kube-proxy, and the DNS proxy all use a
consistent backend.
3. The br_netfilter kernel module must be loaded on the host for
iptables rules to apply to pod bridge traffic. Without it, ClusterIP
DNAT (including kube-dns at 10.43.0.10) is never applied to pod
packets, causing silent DNS timeouts deep in the health-check loop.
Add an early check that fails fast with an actionable error message
if the module is not present, instructing the user to run
`sudo modprobe br_netfilter` on the host.
Signed-off-by: Evan Lezar <elezar@nvidia.com>
* fix(sandbox): emit warning when Landlock filesystem sandbox degrades silently
BestEffort Landlock previously swallowed failures at debug level, making
sandbox bypass invisible to operators at default log levels. Upgrade the
degradation log to warn with an actionable message pointing to the
hard_requirement setting. Add info-level startup log showing the
requested ABI and path counts so operators always know what Landlock
protections are active.
Closes#584
* fix(sandbox): revert unintended ABI bump from V2 to V5
Signed-off-by: John Myers <johntmyers@users.noreply.github.com>
---------
Signed-off-by: John Myers <johntmyers@users.noreply.github.com>
Co-authored-by: John Myers <johntmyers@users.noreply.github.com>
* fix(router): use max_completion_tokens for OpenAI GPT-5+ validation probe
OpenAI GPT-5 models reject the legacy max_tokens parameter and require
max_completion_tokens. The inference validation probe now sends
max_completion_tokens as the primary parameter, with an automatic
fallback to max_tokens when the backend returns HTTP 400 (for
legacy/self-hosted backends that only support the older parameter).
Closes#517
Signed-off-by: Maxime Grenu <maxime.grenu@gmail.com>
* style(router): fix cargo fmt import order and line length
---------
Signed-off-by: Maxime Grenu <maxime.grenu@gmail.com>
Co-authored-by: John Myers <johntmyers@users.noreply.github.com>
relay_passthrough_with_credentials called relay_http_request_with_resolver
(which internally relays the upstream response back to the client) and then
immediately called relay_response_to_client a second time. The second call
blocked forever waiting for a response that would never arrive, deadlocking
every CONNECT tunnel after its first request/response pair.
This caused npm install (and any HTTP/1.1 keep-alive client) to hang
indefinitely when routed through the sandbox proxy without L7 rules.
The L7-inspection path (relay_rest) was not affected — it correctly makes
a single call to relay_http_request_with_resolver.
* fix(sandbox,server): fix chunk merge duplicates and OPA variable collision with overlapping policies
Two related bugs triggered when a draft rule approval creates a second
policy entry for the same host:port:
1. merge_chunk_into_policy looked up existing rules by chunk.rule_name
(auto-generated as allow_{host}_{port}), which never matched the
user's original rule name. Now scans all network_policies entries
for a host:port endpoint match before falling back to insertion,
and merges allowed_ips into the existing endpoint.
2. The Rego allow_request rule and _matching_endpoint_configs
comprehension used 'some ep; ep := policy.endpoints[_]' which
caused regorus to error with 'duplicated definition of local
variable ep' when multiple policies covered the same host:port.
Refactored to isolate endpoint iteration inside helper functions
(_policy_allows_l7, _policy_endpoint_configs) so variables are
scoped per-policy evaluation.
Refs: #567
* test(e2e): add overlapping policy tests and update FWD-2 for implicit allowed_ips
- Update FWD-2 (test_forward_proxy_denied_without_allowed_ips ->
test_forward_proxy_allows_private_ip_host_without_allowed_ips):
literal IP host no longer requires explicit allowed_ips, expects 200.
- Add OVL-1: overlapping L4 policies for same host:port must not crash
OPA and should allow forward proxy connections.
- Add OVL-2: overlapping L7 policies for same host:port must not crash
OPA and should allow CONNECT tunnel establishment.
Refs: #567
* style: apply cargo fmt formatting
* test(e2e): update SSRF-3 and SSRF-6 for implicit allowed_ips behavior
SSRF-6: Private IP with literal IP host now gets implicit allowed_ips
from PR #570, so CONNECT returns 200 instead of 403.
SSRF-3: Loopback is still blocked but via the always-blocked path
(implicit allowed_ips is synthesized, then resolve_and_check_allowed_ips
catches it). Log message says 'always-blocked' instead of 'internal
address'.
* fix(e2e): use negative assertion for SSRF-6 when nothing listens on target port
When the SSRF check passes but nothing listens on the target port,
recv() returns empty bytes. Use 'assert 403 not in' (matching SSRF-4
pattern) instead of 'assert 200 in'.
* fix(e2e): update provider tests for redacted credential values
PR #569 changed credential redaction from clearing the map to
replacing values with 'REDACTED'. Update e2e assertions to expect
credential keys with REDACTED values instead of an empty map.
When a policy endpoint specifies a literal IP address as the host
(e.g. host: 192.168.86.157), the user has explicitly declared intent
to allow that destination. The SSRF guard requiring allowed_ips was
redundant for this case and forced users to duplicate the IP.
Synthesize an implicit allowed_ips entry when the host parses as an IP
address, so the existing allowlist-validation path is used instead of
the blanket internal-IP rejection. Loopback and link-local addresses
remain blocked by resolve_and_check_allowed_ips.
Applies to both the CONNECT and FORWARD proxy paths.
Refs: #567
redact_provider_credentials used HashMap::clear() which removed both
keys and values, causing provider list/get to report 0 credential keys.
Replace with value-only redaction so key names and counts are preserved
while secret values remain hidden.
Refs: #350
Pass CLUSTER_GPU=1 inline in e2e:python:gpu's depends so that the
cluster is bootstrapped with --gpu when GPU e2e tests are run.
Add --gpu flag handling to cluster-bootstrap.sh and default
OPENSHELL_E2E_GPU_IMAGE to an empty string so the server resolves
the default sandbox image when no override is provided.
Signed-off-by: Evan Lezar <elezar@nvidia.com>
* feat(sandbox): auto-detect TLS and terminate unconditionally for credential injection
Closes#533
The proxy now auto-detects TLS by peeking the first bytes of each
connection. When TLS is detected, it terminates unconditionally —
enabling credential injection and optional L7 inspection without
requiring explicit 'tls: terminate' in the policy.
Local image builds via mise run cluster / mise run docker:build:*
were missing the dev-settings feature flag, causing e2e tests to fail
with 'unknown setting key dummy_bool'. CI sets EXTRA_CARGO_FEATURES
explicitly but the local path did not.
Default EXTRA_CARGO_FEATURES to openshell-core/dev-settings so local
builds match CI. The variable can still be overridden by the caller.
* fix(cli): clear stale last-used sandbox on deletion
When a sandbox is deleted, the locally stored last-used record now gets
cleared if it matches the deleted sandbox name. This prevents subsequent
commands from falling back to a sandbox that no longer exists, which
previously caused confusing gRPC errors.
Adds clear_last_sandbox_if_matches() to openshell-bootstrap and calls
it from sandbox_delete() after each successful deletion.
Closes#172
Signed-off-by: Serge Panev <spanev@nvidia.com>
* style: fix rustfmt import ordering and line wrapping
Signed-off-by: Serge Panev <spanev@nvidia.com>
* fix(cli): pass gateway to sandbox_delete in finalize_sandbox_create_session
Signed-off-by: John Myers <johntmyers@users.noreply.github.com>
---------
Signed-off-by: Serge Panev <spanev@nvidia.com>
Signed-off-by: John Myers <johntmyers@users.noreply.github.com>
Co-authored-by: John Myers <johntmyers@users.noreply.github.com>
* fix(l7): reject ambiguous HTTP framing in REST proxy (SEC-009)
Harden the L7 REST proxy HTTP parser against request smuggling:
- Reject requests containing both Content-Length and Transfer-Encoding
headers per RFC 7230 Section 3.3.3 (CL/TE ambiguity)
- Replace String::from_utf8_lossy with strict UTF-8 validation to
prevent interpretation gaps with upstream servers
- Reject bare LF line endings (require CRLF per HTTP spec)
- Validate HTTP version string (HTTP/1.0 or HTTP/1.1 only)
* fix(server): harden shell_escape and command construction (SEC-002)
Harden the gRPC exec handler against command injection via the
structured-to-shell-string conversion:
- Reject null bytes and newlines/carriage returns in shell_escape()
- Add input validation in exec_sandbox: reject control characters in
command args, env values, and workdir
- Enforce size limits: max 1024 args, 32 KiB per arg/value, 4 KiB
workdir, 256 KiB total assembled command string
- Change shell_escape and build_remote_exec_command to return Result
so callers must handle validation failures
* fix(server): add command validation at SSH transport boundary (SEC-003)
Add defense-in-depth validation in run_exec_with_russh before sending
the command to the sandbox SSH server:
- Reject null bytes in command string at transport boundary
- Enforce max command length (256 KiB) at transport boundary
- Enhance stream_exec_over_ssh logging with command length, stdin
length, and truncated command preview for audit trail
* fix(sandbox): validate port range and extend loopback check in SSH (SEC-007)
Harden the SSH direct-tcpip channel handler:
- Validate port_to_connect <= 65535 before u32-to-u16 cast to prevent
port truncation (e.g., 65537 becoming port 1)
- Replace string-literal loopback check with is_loopback_host() that
covers the full 127.0.0.0/8 range, IPv4-mapped IPv6 (::ffff:127.x),
bracketed IPv6, and case-insensitive localhost
- Remove #[allow(clippy::cast_possible_truncation)] since the cast is
now proven safe by the preceding range check
* fix(sandbox): sanitize inference error messages returned to sandbox (SEC-008)
Replace verbatim internal error strings in router_error_to_http with
generic messages to prevent information leakage to sandboxed code.
Upstream URLs, internal hostnames, TLS details, and file paths are no
longer exposed. Full error context is still logged server-side at warn
level by the caller for debugging.
* fix(server): block internal IPs in SSH proxy target validation (SEC-006)
Add IP validation in start_single_use_ssh_proxy to prevent SSRF if a
sandbox status record were poisoned:
- Resolve DNS before connecting and validate the resolved IP
- Block loopback (127.0.0.0/8) and link-local (169.254.0.0/16, covers
cloud metadata endpoint) addresses
- Block IPv4-mapped IPv6 variants of the same ranges
- Connect to the validated SocketAddr directly to prevent TOCTOU
- Add debug logging of resolved target IP for audit
* fix(sandbox): add resource limits to chunked body parser (SEC-010)
Harden parse_chunked_body in the inference interception path:
- Replace all unchecked +2 additions with checked_add for consistent
overflow safety across all target architectures
- Add MAX_CHUNKED_BODY (10 MiB) to cap decoded body size
- Add MAX_CHUNK_COUNT (4096) to prevent CPU exhaustion via tiny chunks
- Early-reject chunk sizes larger than remaining buffer space
* fix(cli): double-escape command for SSH path, validate host and name (SEC-004)
Harden doctor_exec against command injection in the SSH remote path:
- Apply shell_escape to inner_cmd in the SSH path so it survives the
double shell interpretation (SSH remote shell + sh -lc). This also
fixes a correctness bug where multi-word commands were silently
broken in the SSH path.
- Add validate_gateway_name to reject shell metacharacters in gateway
names before use in container_name
- Add validate_ssh_host to reject metacharacters in remote_host loaded
from metadata.json
* fix(sandbox): add CIDR breadth warning and control-plane port blocklist (SEC-005)
Defense-in-depth for the allowed_ips feature:
- Log a warning when a CIDR entry has a prefix length < /16, as overly
broad ranges may unintentionally expose control-plane services
- Block K8s API (6443), etcd (2379/2380), and kubelet (10250/10255)
ports unconditionally in resolve_and_check_allowed_ips, even when
the resolved IP matches an allowed_ips entry
* test(e2e): update assertion for sanitized inference error message (SEC-008)
The SEC-008 fix changed the error message from 'no compatible route
for source protocol ...' to 'no compatible inference route available'.
Update the E2E assertion substring to match.
Disables GPU Feature Discovery and Node Feature Discovery DaemonSets and
overrides the device plugin's default nodeAffinity to null so it schedules
unconditionally on the single-node gateway without requiring NFD/GFD labels.
Setting affinity to an empty map ({}) does not override the chart defaults
because Helm deep-merges user values with chart defaults. Using null explicitly
removes the key, causing the chart template to skip the affinity block entirely.
Docker COPY preserves build-host file permissions. On hosts with a
restrictive umask (e.g. 0027), the migrations directory is copied as
750 root:root, making it unreadable by the openshell user at runtime
and causing the server to crash with a permission denied error on
startup.
Using --chmod=755 ensures the directory is always world-readable
regardless of the build host umask.
Signed-off-by: Evan Lezar <elezar@nvidia.com>
The secrets context is not available in step-level if expressions in
GitHub Actions — only in with and env blocks. This caused the workflow
file to be invalid, failing every run with:
Unrecognized named-value: 'secrets'
Fix by exporting the secret to a job-level env var and checking that
in the step condition instead.
Co-authored-by: John Myers <johntmyers@users.noreply.github.com>
The ORG_READ_TOKEN (read:org PAT) was being used for all API calls,
including closing PRs and posting comments, which it lacks permissions
for. Split into two steps:
1. Org membership check — uses ORG_READ_TOKEN exclusively
2. VOUCHED.td check + close — uses default GITHUB_TOKEN (has repo write)
Step 2 is skipped entirely if step 1 confirms org membership.
Co-authored-by: John Myers <johntmyers@users.noreply.github.com>
The GITHUB_TOKEN cannot determine org membership — it lacks read:org
scope and this is not configurable via the permissions block. Both
author_association and orgs.checkMembershipForUser return NONE/404 for
org members (even public ones) when called with the repo-scoped token.
Use an ORG_READ_TOKEN secret (fine-grained PAT with read:org) when
available, falling back to GITHUB_TOKEN. Also restores the
checkCollaborator fallback since the PAT can resolve that too.
Setup required: create a fine-grained PAT with Organization > Members >
Read permission, then add it as a repo secret named ORG_READ_TOKEN.
Co-authored-by: John Myers <johntmyers@users.noreply.github.com>
The trigger-wheel-publish job was downloading the wheel artifact onto a
persistent self-hosted runner just to glob the filenames. Since
download-artifact does not clean the destination directory, stale .whl
files from every previous run accumulated in release/ and were all sent
to GitLab as WHEEL_FILENAMES.
Confirmed on the runner: /home/ubuntu/actions-runner/_work/OpenShell/
OpenShell/release/ contained 120+ wheels spanning versions 0.0.5
through 0.0.10.
Fix: capture wheel filenames as a job output in build-python-wheels and
pass them directly to trigger-wheel-publish. The trigger job no longer
downloads the artifact at all — it only needs the filenames to construct
GitHub release asset URLs for GitLab/Kitmaker.
John Myers