mirror of
https://github.com/NVIDIA/OpenShell.git
synced 2026-10-17 05:31:16 +08:00
docker-gpu-support
9
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 John T. Myers
|
c960d480fd |
fix(sandbox): canonicalize HTTP request-targets before L7 policy evaluation (#878)
* fix(sandbox): canonicalize HTTP request-targets before L7 policy evaluation
The L7 REST proxy evaluated OPA path rules against the raw request-target
and forwarded the raw header block byte-for-byte to the upstream. The
upstream normalized `..`, `%2e%2e`, and `//` before dispatching, so a
compromised agent inside the sandbox could bypass any path-based allow
rule by prefixing the blocked path with an allowed one (e.g.
`GET /public/../secret` matches `/public/**` at the proxy but the
upstream serves `/secret`). The inference-routing detection had the same
normalization gap via `normalize_inference_path`, which only stripped
scheme+authority and preserved dot-segments verbatim.
- Add `crates/openshell-sandbox/src/l7/path.rs` with
`canonicalize_request_target`. Percent-decodes (with uppercase hex
re-emission), resolves `.` / `..` segments per RFC 3986 5.2.4,
collapses doubled slashes, strips trailing `;params`, rejects
fragments / raw non-ASCII / control bytes / encoded slashes (unless
explicitly enabled per-endpoint), and returns a `rewritten` flag so
callers know when to rewrite the outbound request line.
- Wire the canonicalizer into `rest.rs::parse_http_request`. The
canonical path is the `target` on `L7Request` that OPA sees, and
when the canonical form differs from the raw input the request line
is rebuilt in `raw_header` so the upstream dispatches on the exact
bytes the policy evaluated.
- Canonicalize the forward (plain HTTP proxy) path at
`proxy.rs`'s second L7 evaluation site. A non-canonical request-target
is rejected with 400 Bad Request and an OCSF `NetworkActivity` Fail
event rather than silently passed through.
- Replace the old `normalize_inference_path` body with a call to the
canonicalizer. On canonicalization error the raw path is returned
(so inference detection simply misses and the normal forward path
handles and rejects).
- Document the invariant in `sandbox-policy.rego`: `input.request.path`
is pre-canonicalized so rules must not attempt defensive matching
against `..` or `%2e%2e`.
- Architecture doc (`architecture/sandbox.md`) lists the new module.
- 24 new unit tests cover dot segments, percent-encoded dot segments,
double-slash collapse, encoded-slash reject/opt-in, null/control byte
rejection, legitimate percent-encoded bytes round-tripping, mixed-case
percent normalization, fragment rejection, absolute-form stripping,
empty / length-bounded / non-ASCII handling, and regression payloads
for the specific bypasses above.
Tracks OS-99.
* fix(sandbox): close forward-proxy parser-differential and wire allow_encoded_slash
Addresses two review findings on the L7 path canonicalization change.
1. `handle_forward_proxy` previously evaluated OPA on the canonical path
but wrote the raw path to the upstream via `rewrite_forward_request`,
leaving the parser-differential open for plain-HTTP forward-proxy
traffic even though it was closed for the L7 tunnel path. `path` is
now reassigned to the canonical form inside the L7 block before the
outbound write, so the bytes dispatched to the upstream match what
OPA evaluated. Covered by a new regression test against
`rewrite_forward_request`.
2. `allow_encoded_slash` is now wired through the YAML policy schema,
the proto `NetworkEndpoint` message, the Rego endpoint config, and
the `L7EndpointConfig` → `RestProvider` canonicalization options.
Operators can opt in per-endpoint for upstreams like GitLab that
embed `%2F` in namespaced resource paths; the default remains
strict. The `RestProvider` gains a constructor
`RestProvider::with_options` so `relay_rest` constructs a provider
with per-endpoint options while `relay_passthrough_with_credentials`
retains strict defaults.
Integration tests:
- `parse_http_request_canonicalizes_target_and_rewrites_raw_header`
drives a non-canonical request through the parser and asserts both
`L7Request.target` (what OPA evaluates) and `raw_header` (what the
upstream receives) are canonical.
- `parse_http_request_canonicalization_preserves_query_string`,
`parse_http_request_leaves_canonical_input_byte_for_byte`,
`parse_http_request_rejects_traversal_above_root`,
`parse_http_request_preserves_http_10_version_on_rewrite`.
- `parse_http_request_accepts_encoded_slash_when_endpoint_opts_in` /
`_rejects_encoded_slash_by_default` verify the
`allow_encoded_slash` gate.
- `test_rewrite_forward_request_uses_canonical_path_on_the_wire`
asserts the fix to the forward-proxy flow.
- `parse_l7_config_allow_encoded_slash_{defaults_false,opt_in}` verify
the YAML/Rego wiring.
|
||
|
John T. Myers
|
28e1ff7b40 |
feat(policy): add deny rules to network policy schema (#822)
* feat(policy): add deny rules to network policy schema Closes #565 Add L7 deny rules that block specific requests even when allowed by access presets or explicit allow rules. Deny rules mirror the full capability set of allow rules (method, path, query params, SQL command) and take precedence -- if a request matches any deny rule, it is blocked regardless of allow rules. This enables the "allow everything except these specific operations" pattern without enumerating every allowed endpoint. For example, granting read-write access to GitHub while blocking PR approvals, branch protection changes, and ruleset modifications. * fix(policy): deny query matching fails closed, mirror allow-side validation Addresses PR review findings P1 and P3: P1: Deny-side query matching now uses fail-closed semantics. If ANY value for a query key matches the deny matcher, the deny fires. The previous implementation reused allow-side "all values must match" logic which allowed ?force=true&force=false to bypass a deny on force=true. P3: Deny-side query validation now mirrors the full allow-side checks: empty any lists, non-string matcher values, glob+any mutual exclusion, glob type checks, and glob syntax warnings are all validated. |
||
|
John T. MyersandJohn Myers
|
9e721df9dd |
fix(sandbox): resolve symlinked binary paths in network policy matching (#774)
* fix(sandbox): resolve symlinked binary paths in network policy matching Policy binary paths specified as symlinks (e.g., /usr/bin/python3) were silently denied because the kernel reports the canonical path via /proc/<pid>/exe (e.g., /usr/bin/python3.11). The strict string equality in Rego never matched. Expand policy binary paths by resolving symlinks through the container filesystem (/proc/<pid>/root/) after the entrypoint starts. The OPA data now contains both the original and resolved paths, so Rego's existing strict equality check naturally matches either. - Add resolve_binary_in_container() helper for Linux symlink resolution - Add from_proto_with_pid() and reload_from_proto_with_pid() to OpaEngine - Trigger one-shot OPA rebuild after entrypoint_pid is stored - Thread entrypoint_pid through run_policy_poll_loop for hot-reloads - Improve deny reason with symlink debugging hint - Add 18 new tests including hot-reload and Linux symlink e2e tests Closes #770 * fix(sandbox): skip procfs-dependent tests when /proc/<pid>/root/ is inaccessible The Linux-specific symlink resolution tests depend on /proc/<pid>/root/ being readable, which requires CAP_SYS_PTRACE or permissive ptrace scope. This is unavailable in CI containers, rootless containers, and hardened hosts. Add a procfs_root_accessible() guard that skips these tests gracefully instead of failing. * fix(sandbox): add explicit logging when symlink resolution fails and improve deny messages When /proc/<pid>/root/ is inaccessible (restricted ptrace, rootless containers, hardened hosts), resolve_binary_in_container now logs a per-binary warning with the specific error, the path it tried, and actionable guidance (use canonical path or grant CAP_SYS_PTRACE). Previously this was completely silent. The Rego deny reason for binary mismatches now leads with 'SYMLINK HINT' and includes a concrete fix command ('readlink -f' inside the sandbox) plus what to look for in logs if automatic resolution isn't working. * fix(sandbox): use read_link instead of canonicalize for symlink resolution std::fs::canonicalize resolves /proc/<pid>/root itself (a kernel pseudo-symlink to /) which strips the prefix needed for path extraction. This caused resolution to silently fail in all environments, not just CI. Replace with an iterative read_link loop that walks the symlink chain within the container namespace without resolving the /proc mount point. Add normalize_path helper for relative symlink targets containing .. components. Update procfs_root_accessible test guard to actually probe the full resolution path instead of just checking path existence. * fix(sandbox): defer symlink resolution until container filesystem is ready The one-shot resolve ran immediately after ProcessHandle::spawn, before the child's mount namespace and /proc/<pid>/root/ were populated. This caused symlink_metadata to fail with ENOENT on every binary, and the poll loop never retried because it only reloads when the policy hash changes on the server. Replace the synchronous resolve with an async task that probes /proc/<pid>/root/ with retries (10 attempts, 500ms apart, 5s total). The child's mount namespace is typically ready within a few hundred ms. Also inline error values into warning message strings so they appear in default log output (not just as structured tracing fields that may be elided), and add debug-level logs before each symlink_metadata call to aid diagnosis. --------- Co-authored-by: John Myers <johntmyers@users.noreply.github.com> |
||
|
John T. MyersandJohn Myers
|
e8950e624c |
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 <johntmyers@users.noreply.github.com> |
||
|
John T. Myers
|
256f7fc884 |
fix(sandbox,server): fix chunk merge duplicates and OPA variable collision with overlapping policies (#571)
* 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.
|
||
|
John T. Myers
|
f37b69b5e5 |
feat(sandbox): auto-detect TLS and terminate unconditionally for credential injection (#544)
* 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. |
||
|
John T. Myersandjohntmyers
|
241e95dc39 |
feat(policy): support host wildcards and multi-port endpoints (#366)
* feat(policy): support host wildcards and multi-port endpoints Add glob-style host wildcards to endpoints[].host using OPA's glob.match with "." as delimiter — *.example.com matches a single DNS label, **.example.com matches across labels. Validation rejects bare * and requires *. prefix; warns on broad patterns like *.com. Add repeated uint32 ports field to NetworkEndpoint for multi-port support. Backwards compatible: existing port scalar is normalized to ports array. Both the proto-to-JSON and YAML-to-JSON conversion paths emit a ports array; Rego always references endpoint.ports[_]. Fix OpaEngine::reload() to route through the full preprocessing pipeline instead of bypassing L7 validation and port normalization. Closes #359 * fix(policy): reject configs with both port and ports set --------- Co-authored-by: johntmyers <johntmyers@users.noreply.github.com> |
||
|
John T. Myers
|
647b7947f6 |
fix: security hardening from aardvark/codex scanner findings (#352)
* fix(sandbox): prevent overread request smuggling in L7 REST parser The HTTP/1.1 parser used a 1024-byte read buffer that could capture bytes from a pipelined second request. Those overflow bytes were forwarded upstream as body overflow without L7 policy evaluation, enabling request smuggling that bypasses per-request method/path enforcement. Replace multi-byte read with byte-at-a-time read_u8 that stops exactly at the CRLFCRLF header terminator. Add regression test proving two pipelined requests are parsed independently. Refs: #350 * fix(server): harden sandbox TLS secret volume permissions to 0400 Kubernetes secret volumes default to 0644, allowing the unprivileged sandbox user to read the mTLS client private key via the Landlock baseline /etc read set. A compromised sandbox could use the key to impersonate the control-plane client. Set defaultMode to 256 (octal 0400, owner-read only) on both the default and custom pod template paths. The supervisor reads TLS materials as root before forking, so this does not affect normal operation. Refs: #350 * fix(sandbox): stop using cmdline paths for binary policy matching /proc/pid/cmdline is fully attacker-controlled (argv[0] can be set to any string via execve) and had no integrity verification, unlike exec.path and ancestors which are kernel-managed and get TOFU/SHA256 checks. The Rego policy used cmdline_paths as a grant-access signal, allowing any sandboxed process to claim the identity of an allowed binary and bypass network restrictions. Remove the cmdline exact-match rule and exclude cmdline_paths from the glob-match rule. Only exec.path and exec.ancestors (from /proc/pid/exe) are now used for binary identity. cmdline_paths remain in the OPA input for deny-reason diagnostics only. Refs: #350 * fix(sandbox): reject symlink and non-dir read_write paths before chown The supervisor runs as root and calls chown on each read_write path. Since chown follows symlinks, a malicious container image could place a symlink (e.g. /sandbox -> /etc/shadow) to trick the supervisor into transferring ownership of arbitrary files to the sandbox user. Add symlink_metadata (lstat) check before chown to reject symlinks and non-directory entries. The TOCTOU window is not exploitable because no untrusted child process has been forked yet at this point. Refs: #350 * fix(server): redact provider credentials in gRPC CRUD responses Provider CRUD RPCs (create, get, list, update) returned full Provider objects including plaintext credentials (API keys, secrets). Any authenticated client -- including sandbox workloads running untrusted code -- could read credentials for all providers. Add redact_provider_credentials helper that clears the credentials map before returning. Internal server paths (inference routing, sandbox env injection) read from the store directly and are unaffected. Update tests to verify redaction and assert persistence via direct store reads. Refs: #350 * fix(sandbox): enforce non-root fallback when process user unset drop_privileges silently returned Ok(()) when both run_as_user and run_as_group were None, even when running as root. In local/dev mode policies are loaded from disk without passing through the server-side ensure_sandbox_process_identity normalization, so child processes could retain root and all capabilities (SYS_ADMIN, NET_ADMIN). When running as root with no process identity configured, fall back to sandbox:sandbox instead of no-oping. Non-root runtimes are unaffected. Refs: #350 * fix(sandbox): deny forward proxy for L7-configured endpoints The forward proxy path only performed L4 (endpoint) and allowed_ips checks. If an endpoint had L7 rules (method/path restrictions), a sandboxed process could bypass them by using HTTP_PROXY with plain http:// requests instead of CONNECT tunneling, since L7 inspection only runs in the CONNECT path. Add a guard in handle_forward_proxy that queries the endpoint's L7 config and returns 403 if any L7 rules are present, forcing traffic through the CONNECT path where per-request inspection happens. Refs: #350 * test(e2e): add regression test for forward proxy L7 bypass Verifies that the forward proxy path (plain http:// via HTTP_PROXY) returns 403 for endpoints with L7 rules configured, preventing sandboxed processes from bypassing per-request method/path enforcement by avoiding the CONNECT tunnel. Refs: #350 * fix: update tests for cmdline path and TLS volume changes Update OPA tests to verify cmdline_paths no longer grant access (regression tests for the security fix). Fix formatting in TLS volume test. Refs: #350 * test(e2e): update provider e2e tests for credential redaction Provider CRUD gRPC responses no longer include credential values. Update three e2e tests to assert credentials are empty in responses. Provider functionality is verified by existing e2e tests that check env var injection into sandboxes (which read from the store directly). Refs: #350 * chore: reformat for Rust 1.94 assert! macro style * fix(sandbox): make drop_privileges tests root-aware for CI CI runs as root but has no 'sandbox' user. The security fix correctly errors when running as root with no process identity and no fallback user available -- this is the intended behavior (refuse to run as root). Update tests to expect that error in root-without-sandbox-user environments instead of unconditionally asserting Ok. Refs: #350 * fix(sandbox): allow non-directory read_write entries like /dev/null The symlink guard incorrectly rejected all non-directory entries. Character devices like /dev/null are legitimate read_write paths used in sandbox policies. Only reject symlinks, which are the actual attack vector for the chown privilege escalation. Refs: #350 |
||
|
Drew Newberry
|
fbd93a4632 | refactor: rename navigator- crate prefix to openshell- (#277) |