feat(sandbox,gateway): route sandbox egress through corporate HTTP proxy#2245
feat(sandbox,gateway): route sandbox egress through corporate HTTP proxy#2245feloy wants to merge 27 commits into
Conversation
PR Review StatusValidation: This is project-valid work for the enterprise restricted-egress gap in #1792. Maintainer triage confirmed the need for corporate proxy chaining after OpenShell policy enforcement, and the PR includes the relevant Podman configuration plus Fern and architecture documentation. Head SHA: Review findings:
Tests should cover the SSRF/DNS binding, CONNECT over-read, invalid-config fail-closed behavior, plain HTTP against a standard forward proxy, combined Podman environment precedence, and HTTPS host-gateway bypass. Runtime proxy behavior also requires Docs: The existing Fern gateway reference is the correct page and no @feloy, please push an updated commit addressing the items above. Gator will re-review the new head before starting the E2E gate. Next state: |
johntmyers
added
the
gator:in-review
ca5c041 to
7a1f8f3
Compare
PR Review StatusValidation: This remains project-valid work for the corporate restricted-egress gap in #1792. The current scope covers the supervisor proxy path and Podman operator configuration, with the relevant Fern and architecture documentation present. Head SHA: The fresh independent code review found these blocking items on the current head:
Also tighten port-qualified The prior control-character concern is not a header-injection blocker because decoded Basic credentials are Base64-encoded before insertion, though rejecting ASCII controls is still sensible input validation. Docs:
Next state: |
PR Review StatusValidation: This remains project-valid work for the corporate restricted-egress gap in #1792. The supervisor and Podman scope is coherent, and the relevant Fern and architecture documentation is present. Head SHA: The newest commit resolves the prior proxy-selection ownership finding: reserved The fresh independent code review found these remaining blocking items:
Next state: |
9f1b270 to
42291f2
Compare
PR Review StatusValidation: This remains project-valid work for the corporate restricted-egress gap in #1792. The supervisor and Podman scope is coherent, with the relevant Fern and architecture documentation present. Head SHA: The newest commit resolves the prior child-environment part of the credential finding: reserved The fresh independent code review found these remaining blocking items:
Please add regression coverage for the SSRF/DNS binding, fail-closed startup, HTTP wire form, HTTPS host-gateway bypass, CONNECT over-read, port-aware
Next state: |
PR Review StatusValidation: This remains project-valid work for the corporate restricted-egress gap in #1792. The supervisor and Podman scope is coherent, and the relevant Fern and architecture documentation is present. Head SHA: The newest commit resolves the prior credential-storage concern substantially: proxy credentials now use a root-only per-sandbox Podman secret, the reserved auth-file path is stripped from workload children, and generated gateway configuration is mode The fresh independent code review found these remaining blocking items:
Docs: Please add focused regressions for validated-IP binding, fatal invalid config/auth handling, absolute-form HTTP forwarding, CONNECT over-read, HTTPS host-gateway bypass, port-aware
Next state: |
The reserved OPENSHELL_UPSTREAM_* variables are an operator-owned egress boundary, but the supervisor treated present-but-invalid values as unset: an unsupported or malformed proxy URL was ignored with a warning, an unreadable auth file proceeded without credentials, and a malformed credential silently became unauthenticated. Any of these could quietly downgrade the corporate proxy boundary to direct dialing or unauthenticated proxy access. Make every configured-but-invalid proxy or auth setting fatal to supervisor proxy startup, emit an OCSF ConfigStateChange failure event before refusing, and share URL validation semantics between the Podman driver and the supervisor through a single validator in openshell-core (parse_upstream_proxy_url), so a value accepted at sandbox-create time can never be rejected in-container or vice versa. Inline user:pass@ URL credentials are now fatal in the supervisor too (previously warn-and-strip), matching the driver. Unset or empty variables still mean no proxy; only present-but-invalid values fail. Error paths never include credential content. Addresses the fail-closed review item on NVIDIA#2245. Signed-off-by: Philippe Martin <phmartin@redhat.com>
PR Review StatusValidation: This remains project-valid work for the corporate restricted-egress gap in #1792. The supervisor and Podman scope is coherent, and the existing Fern gateway reference is the right documentation location. Head SHA: The latest commit materially improves the fail-closed configuration path: non-empty malformed or unsupported proxy URLs, inline credentials, and unreadable or malformed auth-file contents now fail closed through shared URL validation. The operator-owned environment boundary, workload stripping, and Podman secret staging also remain sound. The fresh independent code review found these remaining blocking items:
Docs/tests: No
Next state: |
… contract The upstream proxy URL already had a single shared validator, but the credential did not: the Podman driver rejected only CR/LF/NUL while the supervisor rejected every control character, so a credential accepted at sandbox-create time (e.g. one containing a tab) could still be rejected in-container. Present-but-whitespace reserved OPENSHELL_UPSTREAM_* values were also silently treated as unset, quietly downgrading the operator's egress boundary to direct dialing. Add parse_upstream_proxy_credential to openshell-core as the single source of truth for the documented user:pass credential form (non-empty user, no control characters, trimmed) and use it in both the Podman driver's secret staging and the supervisor's Proxy-Authorization header construction. Error variants carry no payload so credential content can never leak into messages. Make a present-but-empty reserved variable fatal to supervisor proxy startup instead of meaning "unset"; only fully unset variables disable the proxy. The driver correspondingly rejects an empty no_proxy at config time so it can never inject a value the supervisor refuses. Addresses the remaining fail-closed credential/config review item on Signed-off-by: Philippe Martin <phmartin@redhat.com> NVIDIA#2245.
PR Review StatusValidation: This remains project-valid work for the corporate restricted-egress gap in #1792. The supervisor and Podman scope is coherent, and the existing Fern gateway reference is the correct documentation location. Head SHA: The latest commit improves the shared fail-closed contract: driver and supervisor now share proxy URL/credential parsing, reject normal present-but-empty reserved values, and validate malformed credentials consistently. Operator-owned routing, workload stripping, root-only credential staging, and cleanup also remain sound. The fresh independent code review found these remaining blocking items:
Also reject proxy URL paths, queries, and fragments rather than silently ignoring them. Add focused regressions for split-horizon binding, HTTPS host-gateway bypass, absolute-form HTTP forwarding, CONNECT over-read, Docs: No
Next state: |
…paths
Two configuration paths could still silently run without the proxy
boundary the operator believed was in effect.
A no_proxy bypass list configured without any https_proxy/http_proxy
was accepted by both the driver and the supervisor and simply meant
"dial everything directly". Reject it on both sides, exactly like the
existing proxy_auth_file-without-proxy rule: an operator who wrote a
bypass list assumed proxying was active, so accepting it hides a
fail-open state.
The gateway.sh dev script guarded proxy settings with [[ -n "${VAR:-}"
]], which conflates unset with explicitly-empty and dropped the latter
before the gateway's validation could see it. Use ${VAR+x} instead so
a set-but-empty variable is written into gateway.toml and rejected at
startup by validate_proxy_config rather than silently discarded.
Addresses the remaining fail-open configuration review item on NVIDIA#2245.
Signed-off-by: Philippe Martin <phmartin@redhat.com>
PR Review StatusValidation: This remains project-valid work for the corporate restricted-egress gap in #1792. The supervisor and Podman scope is coherent, and the existing Fern gateway reference is the correct documentation location. Head SHA: The newest commit resolves two prior fail-open paths: The following blocking items remain:
Docs: No Please add focused regressions for proxy/local DNS divergence, HTTPS host-gateway bypass, absolute-form HTTP forwarding, CONNECT over-read, URL-component rejection, port-aware/CIDR
Next state: |
…fragment parse_upstream_proxy_url accepted URLs like http://proxy.corp.com:8080/some/path and silently discarded everything after host:port, a lenience inherited from the original supervisor parser. A forward proxy is addressed by host:port only, so extra components indicate a misconfiguration (for example a pasted endpoint URL) and silently truncating them violates the present-but-invalid-is-fatal contract enforced everywhere else in this configuration surface. Reject a path, query, or fragment in the shared validator with a new UnexpectedComponent error. A bare trailing slash remains accepted because the url crate normalizes an absent http path to "/", making the two indistinguishable. Both the Podman driver (gateway startup) and the supervisor (sandbox startup) inherit the rule through the shared parser, keeping their semantics identical by construction. Addresses the proxy URL component review item on NVIDIA#2245. Signed-off-by: Philippe Martin <phmartin@redhat.com>
PR Review StatusValidation: This remains project-valid work for the corporate restricted-egress gap in #1792. The supervisor and Podman scope is coherent, and the existing Fern gateway reference is the correct documentation location. Head SHA: The newest commit resolves the prior proxy-URL component finding: path, query, and fragment values are now rejected through the parser shared by the driver and supervisor, with focused coverage. The fresh independent code review found these remaining blocking items:
Also escape the proxy values written into generated TOML at Docs: No
Next state: |
The http_proxy path tunneled plain-HTTP requests through the corporate proxy with CONNECT to port 80 and then sent origin-form requests down the tunnel. Conventional enterprise forward proxies expect plain HTTP as absolute-form requests sent directly over the proxy connection, and commonly refuse CONNECT to port 80, so the setting looked supported but failed against typical deployments. Tunneling also blinds the proxy to the one protocol it could inspect. Narrow the feature to TLS (CONNECT) egress only, which is the conventional and already-correct case: plain-HTTP requests now always dial the destination directly, and only client CONNECT tunnels chain through the corporate proxy. Remove the http_proxy config field, the --sandbox-http-proxy / OPENSHELL_SANDBOX_HTTP_PROXY driver surface, the reserved OPENSHELL_UPSTREAM_HTTP_PROXY variable, and the UpstreamScheme plumbing. The feature never shipped, so this is a clean removal; a stray http_proxy key in gateway.toml still fails loudly through the config's deny_unknown_fields. Removing the plain-HTTP proxy branch also removes its host-gateway special case; the architecture doc now documents the real host-gateway behavior (add driver-injected host aliases to the reserved NO_PROXY list) instead of an invariant the HTTPS path never implemented. Plain-HTTP forwarding through a corporate proxy can return later as absolute-form forwarding behind its own design review. Addresses the plain-HTTP forwarding review item on NVIDIA#2245. Signed-off-by: Philippe Martin <phmartin@redhat.com>
PR Review StatusValidation: This remains project-valid work for the corporate restricted-egress gap in #1792. The supervisor and Podman scope is coherent, and the existing Fern gateway reference is the correct documentation location. Head SHA: Thanks @feloy. I checked the newest commit's decision to remove plain-HTTP upstream proxying. That resolves the prior forwarding-scope finding: plain HTTP now explicitly dials directly, and the architecture text no longer promises an automatic HTTPS host-gateway bypass. Operators instead add unreachable host aliases to the reserved The fresh independent code review found these remaining blocking items:
Please add focused regressions for DNS/SSRF divergence, port-aware and resolved-address Docs: No @feloy, please push an updated commit; gator will re-review the new head. Next state: |
The reserved OPENSHELL_UPSTREAM_* variables are an operator-owned egress boundary, but the supervisor treated present-but-invalid values as unset: an unsupported or malformed proxy URL was ignored with a warning, an unreadable auth file proceeded without credentials, and a malformed credential silently became unauthenticated. Any of these could quietly downgrade the corporate proxy boundary to direct dialing or unauthenticated proxy access. Make every configured-but-invalid proxy or auth setting fatal to supervisor proxy startup, emit an OCSF ConfigStateChange failure event before refusing, and share URL validation semantics between the Podman driver and the supervisor through a single validator in openshell-core (parse_upstream_proxy_url), so a value accepted at sandbox-create time can never be rejected in-container or vice versa. Inline user:pass@ URL credentials are now fatal in the supervisor too (previously warn-and-strip), matching the driver. Unset or empty variables still mean no proxy; only present-but-invalid values fail. Error paths never include credential content. Addresses the fail-closed review item on NVIDIA#2245. Signed-off-by: Philippe Martin <phmartin@redhat.com>
… contract The upstream proxy URL already had a single shared validator, but the credential did not: the Podman driver rejected only CR/LF/NUL while the supervisor rejected every control character, so a credential accepted at sandbox-create time (e.g. one containing a tab) could still be rejected in-container. Present-but-whitespace reserved OPENSHELL_UPSTREAM_* values were also silently treated as unset, quietly downgrading the operator's egress boundary to direct dialing. Add parse_upstream_proxy_credential to openshell-core as the single source of truth for the documented user:pass credential form (non-empty user, no control characters, trimmed) and use it in both the Podman driver's secret staging and the supervisor's Proxy-Authorization header construction. Error variants carry no payload so credential content can never leak into messages. Make a present-but-empty reserved variable fatal to supervisor proxy startup instead of meaning "unset"; only fully unset variables disable the proxy. The driver correspondingly rejects an empty no_proxy at config time so it can never inject a value the supervisor refuses. Addresses the remaining fail-closed credential/config review item on Signed-off-by: Philippe Martin <phmartin@redhat.com> NVIDIA#2245.
…paths
Two configuration paths could still silently run without the proxy
boundary the operator believed was in effect.
A no_proxy bypass list configured without any https_proxy/http_proxy
was accepted by both the driver and the supervisor and simply meant
"dial everything directly". Reject it on both sides, exactly like the
existing proxy_auth_file-without-proxy rule: an operator who wrote a
bypass list assumed proxying was active, so accepting it hides a
fail-open state.
The gateway.sh dev script guarded proxy settings with [[ -n "${VAR:-}"
]], which conflates unset with explicitly-empty and dropped the latter
before the gateway's validation could see it. Use ${VAR+x} instead so
a set-but-empty variable is written into gateway.toml and rejected at
startup by validate_proxy_config rather than silently discarded.
Addresses the remaining fail-open configuration review item on NVIDIA#2245.
Signed-off-by: Philippe Martin <phmartin@redhat.com>
328a084 to
354b1d0
Compare
…fragment parse_upstream_proxy_url accepted URLs like http://proxy.corp.com:8080/some/path and silently discarded everything after host:port, a lenience inherited from the original supervisor parser. A forward proxy is addressed by host:port only, so extra components indicate a misconfiguration (for example a pasted endpoint URL) and silently truncating them violates the present-but-invalid-is-fatal contract enforced everywhere else in this configuration surface. Reject a path, query, or fragment in the shared validator with a new UnexpectedComponent error. A bare trailing slash remains accepted because the url crate normalizes an absent http path to "/", making the two indistinguishable. Both the Podman driver (gateway startup) and the supervisor (sandbox startup) inherit the rule through the shared parser, keeping their semantics identical by construction. Addresses the proxy URL component review item on NVIDIA#2245. Signed-off-by: Philippe Martin <phmartin@redhat.com>
The http_proxy path tunneled plain-HTTP requests through the corporate proxy with CONNECT to port 80 and then sent origin-form requests down the tunnel. Conventional enterprise forward proxies expect plain HTTP as absolute-form requests sent directly over the proxy connection, and commonly refuse CONNECT to port 80, so the setting looked supported but failed against typical deployments. Tunneling also blinds the proxy to the one protocol it could inspect. Narrow the feature to TLS (CONNECT) egress only, which is the conventional and already-correct case: plain-HTTP requests now always dial the destination directly, and only client CONNECT tunnels chain through the corporate proxy. Remove the http_proxy config field, the --sandbox-http-proxy / OPENSHELL_SANDBOX_HTTP_PROXY driver surface, the reserved OPENSHELL_UPSTREAM_HTTP_PROXY variable, and the UpstreamScheme plumbing. The feature never shipped, so this is a clean removal; a stray http_proxy key in gateway.toml still fails loudly through the config's deny_unknown_fields. Removing the plain-HTTP proxy branch also removes its host-gateway special case; the architecture doc now documents the real host-gateway behavior (add driver-injected host aliases to the reserved NO_PROXY list) instead of an invariant the HTTPS path never implemented. Plain-HTTP forwarding through a corporate proxy can return later as absolute-form forwarding behind its own design review. Addresses the plain-HTTP forwarding review item on NVIDIA#2245. Signed-off-by: Philippe Martin <phmartin@redhat.com>
…tests Follow-ups from review: - Add OPENSHELL_UPSTREAM_PROXY_AUTH_ALLOW_INSECURE and OPENSHELL_UPSTREAM_PROXY_CONNECT_BY_HOSTNAME to the supervisor-only strip list so workload child processes never inherit them, matching the documented contract for the other reserved proxy variables, and cover both in the supervisor-only variable test. - Document all five OPENSHELL_SANDBOX_* proxy variables in the mise run gateway help text, marked Podman-only and stating the auth-file/acknowledgement pairing, and complete the gateway-key list in the Podman README. - Add NO_PROXY composition coverage for bracketed IPv6 entries with port qualifiers, bare IPv6 entries, and IPv6 CIDR matching against an IPv6-literal host and against a hostname's resolved addresses, including the port-qualified CIDR form. Signed-off-by: Philippe Martin <phmartin@redhat.com>
…ed budget A proxy that accepted the first CONNECT request but never responded consumed the entire aggregate handshake timeout, so later validated addresses were never tried and the hang defeated the multi-address fallback. Each attempt is now time-boxed to its fair share of the time remaining before the shared deadline (remaining / attempts_left): a hanging attempt is cut off with enough budget left for every remaining address, while time a fast failure does not use rolls over to later attempts and the total never exceeds CONNECT_HANDSHAKE_TIMEOUT. A timed-out attempt is recorded like any other failure, and the aggregate error distinguishes all-attempted from budget-exhausted runs. Adds a first-hangs/second-succeeds regression driven through a test-visible budget parameter so it runs in about a second instead of a real 30s window. Signed-off-by: Philippe Martin <phmartin@redhat.com>
… argv The proxy settings were injected as reserved OPENSHELL_UPSTREAM_* container environment variables. The driver only wrote names the operator configured, but container runtimes layer the spec environment over ENV values baked into the sandbox image, so an image could supply NO_PROXY=*, enable hostname CONNECT, or point an unconfigured deployment at an attacker-controlled proxy whenever the operator left a field unset. The settings now travel as supervisor command-line arguments (--upstream-proxy, --upstream-no-proxy, --upstream-proxy-auth-file, --upstream-proxy-auth-allow-insecure, --upstream-proxy-connect-by-hostname) built by the driver from operator config. The driver sets the container entrypoint and command explicitly, so neither sandbox spec/template environment nor image ENV can influence argv, and an omitted flag genuinely means unconfigured — in every supervisor topology, since the supervisor no longer consults its environment for these settings at all. Credentials stay on the root-only secret mount; only the mount path appears on argv. The reserved environment names, their strip-list entries, and the env-based validation surface are removed. UpstreamProxyConfig::from_args replaces from_env, reusing the same shared fail-closed validation and pairing rules keyed by the CLI flag names. Driver tests now assert the argv contract, including that sandbox-supplied environment cannot add, remove, or redirect proxy flags; supervisor tests cover from_args mapping and its pairing rules. Signed-off-by: Philippe Martin <phmartin@redhat.com>
The argv migration left comments describing the configuration as reserved
environment variables ("reserved value", "present-but-empty variable",
"reserved upstream proxy variables"). Rephrase them as driver-supplied
arguments and operator settings so the documented trust boundary matches
the implementation. Comment-only change.
Signed-off-by: Philippe Martin <phmartin@redhat.com>
parse_target split the CONNECT authority at the first colon, so an IPv6-literal target like [2001:db8::1]:443 always failed port parsing and IPv6-literal clients could never reach policy evaluation; a regression test even locked in that failure. Parse the RFC 3986 bracketed form and return the host bracket-free, matching what DNS resolution, SSRF validation, NO_PROXY matching, and the upstream CONNECT builder expect. Unclosed brackets, a missing or empty port after the bracket, and non-numeric ports are rejected; unbracketed behavior is unchanged. Replaces the failure-locking test with success coverage for bracketed targets and adds malformed-bracket rejection cases. Signed-off-by: Philippe Martin <phmartin@redhat.com>
The per-sandbox proxy-auth credential secret is staged before the container is created and removed on cleanup, but no test proved the cleanup paths actually issue the secret removal. Add Podman-stub tests that drive create_sandbox to a container-create failure and to a start failure, and delete_sandbox for an out-of-band deletion, asserting each path issues the DELETE for the per-sandbox proxy-auth secret so a credential can never outlive the sandbox that owned it. Signed-off-by: Philippe Martin <phmartin@redhat.com>
The Fern Podman driver section enumerated its gateway.toml keys but omitted the corporate egress proxy settings. Add https_proxy, no_proxy, proxy_auth_file, proxy_auth_allow_insecure, and proxy_connect_by_hostname with a pointer to the gateway configuration reference for the full contract. No navigation change: the reference folder already includes the gateway configuration page. Signed-off-by: Philippe Martin <phmartin@redhat.com>
Existing tests exercised validated-IP CONNECT and the upstream-TLS helper independently, but not the full boundary. Add an end-to-end regression that stands up a fake corporate proxy tunneling to a fake TLS server and drives the real path: connect_via_validated CONNECTs to the validated address, the proxy splices the tunnel, and tls_connect_upstream verifies the upstream certificate against the original hostname carried in SNI. It asserts the CONNECT authority is the validated IP and never the hostname, that verification succeeds for the matching hostname, and that a mismatched hostname is rejected — proving a rebinding or split-horizon substitution behind the proxy cannot pass certificate verification. Signed-off-by: Philippe Martin <phmartin@redhat.com>
d845860 to
c8434ad
Compare
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: This remains project-valid work for the corporate restricted-egress gap in #1792. The Podman and supervisor scope is coherent, and the relevant Fern and architecture documentation is present.
Head SHA: c8434ad4a6001c2460606d63bc2cacea89d846bb
The prior wording finding on upstream_proxy.rs and proxy.rs is resolved by the latest commits. One related low-severity residue remains inline in config.rs.
Review findings:
- High — missing cross-layer E2E coverage: No E2E crosses gateway TOML → Podman configuration → OCI argv/secret mount → supervisor CONNECT. The unit tests cannot detect Podman API schema errors, Clap/argv integration failures, or secret-mount labeling failures under SELinux/AppArmor. Please add a Podman sandbox E2E using a fake forward proxy that verifies a policy-approved HTTPS request produces validated-IP CONNECT, credentials are delivered successfully, a denied destination never reaches the proxy, and deletion removes the secret. Relevant paths include
crates/openshell-driver-podman/src/container.rs,crates/openshell-supervisor-network/src/proxy.rs, andcrates/openshell-supervisor-network/src/upstream_proxy.rs. - Three additional actionable findings are anchored inline in this review.
Docs: The Fern/operator documentation is sufficient for the declared Podman-only scope; the existing reference pages are already covered by reference navigation.
Next state: gator:in-review — blocking findings remain and require an author update before E2E/pipeline monitoring.
| return Ok(None); | ||
| }; | ||
|
|
||
| let raw = tokio::fs::read_to_string(path).await.map_err(|e| { |
There was a problem hiding this comment.
gator-agent
Medium — CWE-400: read_to_string has no size bound; a huge credential file or special file such as /dev/zero can exhaust gateway memory during sandbox creation. The supervisor repeats the unbounded read at upstream_proxy.rs:445. Define a small shared maximum credential size and use bounded reads on both sides, preferably rejecting non-regular files; add oversized/special-file tests.
| // Explicit-port presence was verified above; `port()` is `None` only | ||
| // when the URL spells out the scheme default (`:80`), which the url | ||
| // crate normalizes away. | ||
| port: parsed.port().unwrap_or(80), |
There was a problem hiding this comment.
gator-agent
Medium: http://proxy:0 is accepted as valid configuration, but remote TCP port zero is not a usable proxy endpoint, so startup validation succeeds and every proxied request fails later. Reject port 0 with a typed validation error and add shared-validator and Podman-config tests.
| })?; | ||
| } | ||
|
|
||
| // The supervisor treats a present-but-empty reserved variable as a |
There was a problem hiding this comment.
gator-agent
Low: This still describes a “reserved variable,” although the transport is now driver-supplied argv. Reword it to “present-but-empty driver-supplied argument” so the documented security boundary matches the implementation.
… comment Three review findings: - CWE-400: the proxy-auth credential file was read with an unbounded read_to_string on both the driver (sandbox-create) and supervisor (startup) paths, so a huge file or a special file such as /dev/zero could exhaust memory. Add a shared bounded reader in openshell-core that rejects non-regular files, caps the size at 4 KiB, and reads at most that many bytes; the driver runs it via spawn_blocking. Covers oversized, special-file, and missing-path cases on both sides. - Reject an upstream proxy URL with port 0: it passed the explicit-port check and startup validation but is not a connectable TCP port, so every proxied dial would fail later. Add a typed ZeroPort error with shared-validator and Podman-config tests. - Reword a driver-config comment that still described a 'reserved variable' to match the argv transport. Signed-off-by: Philippe Martin <phmartin@redhat.com>
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: This remains project-valid work for the corporate restricted-egress gap in #1792. The Podman and supervisor scope is coherent, and the operator-facing Fern plus architecture documentation is present.
Head SHA: 4524ac91d7a6ca281382ad40bec7d4b1e0bed373
Thanks @feloy. I checked the latest commit. The shared 4 KiB credential-reader cap, proxy port-zero rejection, and argv wording changes resolve three parts of the prior review. The core validated-IP CONNECT, TLS/SNI composition, authentication acknowledgement, and cleanup logic otherwise look coherent.
The fresh independent code review found these remaining items:
- Medium — add a committed cross-layer Podman regression. The current tests instantiate configuration/specs or call CONNECT helpers directly, so they cannot detect failures across gateway TOML deserialization, actual Podman argv and secret-mount semantics, supervisor CLI parsing, policy denial before proxy contact, or real secret deletion. Extend the existing Podman E2E harness with an authenticated fake proxy and verify an approved TLS request traverses it, a denied request never reaches it, credentials arrive through the mounted secret, and sandbox deletion removes the Podman secret. Relevant paths include
crates/openshell-driver-podman/src/config.rs:147,container.rs:955,container.rs:1064,driver.rs:126,driver.rs:742,crates/openshell-supervisor-network/src/proxy.rs:1216, ande2e/with-podman-gateway.sh:413. - One additional actionable credential-file finding is anchored inline in this review.
Docs: The Fern/operator documentation is sufficient for the declared Podman-only scope; the existing reference pages are already covered by navigation.
Runtime proxy behavior still requires test:e2e, but the label and authorized /ok to test remain deferred while author changes are required. Please push an updated commit; gator will re-review the new head before starting the pipeline gate.
Next state: gator:in-review
| pub fn read_upstream_proxy_credential_file(path: &str) -> Result<String, String> { | ||
| use std::io::Read as _; | ||
|
|
||
| let file = std::fs::File::open(path) |
There was a problem hiding this comment.
gator-agent
Medium — CWE-400: File::open occurs before the regular-file check. On Unix, opening a FIFO read-only blocks until a writer appears, so a configured FIFO can indefinitely hang sandbox creation and synchronously hang supervisor startup. Open with O_NONBLOCK on Unix, then inspect the opened descriptor and reject non-regular files; add a mkfifo regression proving the function returns promptly.
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: This remains project-valid work for the corporate restricted-egress gap in #1792. The Podman implementation is coherent and includes the relevant Fern and architecture documentation.
Head SHA: 77af7a55533adb32b7405a199d67c777f050ffbe
The latest commit resolves the prior FIFO blocker: the credential reader now opens non-blocking before rejecting non-regular files, with focused regression coverage. The fresh independent review found one new blocking manifest regression, posted inline.
Docs: docs/reference/gateway-config.mdx and docs/reference/sandbox-compute-drivers.mdx cover the operator contract and security tradeoffs; no docs/index.yml change is needed because the existing reference folder is navigated automatically.
Runtime proxy behavior still requires the test:e2e gate after the manifest finding is fixed and the new head passes review. /ok to test is therefore not posted for this head.
@feloy, please move chrono and reqwest back into the general dependency table and push an updated commit.
Next state: gator:in-review
| ipnet = "2" | ||
| base64 = { workspace = true } | ||
|
|
||
| [target.'cfg(unix)'.dependencies] |
There was a problem hiding this comment.
gator-agent
Critical: This target table also captures the existing chrono and reqwest declarations below it. Telemetry is enabled by default and uses both crates on non-Unix targets, so those builds will lose the dependencies and fail. Keep chrono and reqwest in [dependencies], and place only nix under [target.'cfg(unix)'.dependencies].
read_upstream_proxy_credential_file opened the path with a blocking File::open before the regular-file check, so a configured FIFO with no writer would block open() indefinitely — hanging sandbox creation on the driver and supervisor startup. Open with O_NONBLOCK on Unix so the open returns immediately, then reject the non-regular file as before; O_NONBLOCK has no effect on the later read of a regular file. Adds a mkfifo regression asserting the reader returns promptly with a non-regular-file error instead of hanging. Signed-off-by: Philippe Martin <phmartin@redhat.com>
77af7a5 to
1f9fc5b
Compare
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: This remains project-valid work for the corporate restricted-egress gap in #1792. The Podman and supervisor scope is coherent, and the relevant Fern and architecture documentation is present.
Head SHA: 1f9fc5ba360905a33d52fbb045e406807314dd9b
Thanks @feloy. I checked the latest head. Moving chrono and reqwest back into the general dependency table resolves the prior cross-platform manifest regression, while the Unix-only nix dependency now contains only the FIFO fix. Opening the credential file with O_NONBLOCK before rejecting non-regular files resolves the FIFO hang, with focused regression coverage.
The fresh independent code review found no remaining actionable correctness, security, portability, or maintainability defects. The operator-owned argv boundary, fail-closed configuration, validated-IP CONNECT binding, original-host TLS verification, bounded credential reads, and Podman secret cleanup are coherent.
Docs: docs/reference/gateway-config.mdx and docs/reference/sandbox-compute-drivers.mdx cover the operator contract and security tradeoffs; no docs/index.yml change is needed because the reference folder is already navigated.
Runtime proxy behavior requires test:e2e. Gator will apply that label and use the operator-authorized /ok to test for this head if the copy-PR gate requires it, then monitor the required checks.
Review findings: No blocking findings remain.
Next state: gator:watch-pipeline
johntmyers
added
gator:watch-pipeline
|
Label |
|
/ok to test 1f9fc5b |
johntmyers
added
gator:approval-needed
|
It seems that agent-gator forgot about it, but I think it will be necessary to add e2e tests are described in #2245 (review) |
johntmyers
added
gator:in-review
The existing corporate-proxy tests construct config structs or call CONNECT helpers directly, so none of them detect a break in the wiring between layers: gateway TOML deserialization, the Podman argv and secret-mount semantics, supervisor CLI parsing, or policy denial before proxy contact. Add a Podman e2e that drives the whole chain against a fake authenticated forward proxy and asserts that an approved TLS request traverses it with a validated-IP CONNECT, a policy-denied destination is refused with 403 without ever reaching the proxy, credentials arrive through the mounted per-sandbox secret, and deleting the sandbox removes that secret. SupportContainer is a new harness fixture. Unlike ContainerHttpServer it probes readiness with a TCP connect rather than an HTTP GET, so it can host a forward proxy and TLS servers, and it exposes container logs and network IP for assertions. Signed-off-by: Philippe Martin <phmartin@redhat.com>
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: This remains project-valid work for the corporate restricted-egress gap in #1792. The Podman and supervisor implementation is coherent, with the relevant Fern and architecture documentation present.
Head SHA: 2b18a75e3f403ba1746c1f2eaad243ecbf9ee237
Thanks @feloy. I checked your note that the cross-layer E2E was still needed and reviewed the new committed test. It now exercises gateway TOML configuration, Podman argv and secret delivery, validated-IP CONNECT, policy denial before proxy contact, authenticated TLS egress, and normal secret cleanup.
The fresh independent code review found one remaining actionable E2E cleanup defect, anchored inline. On a panic after the temporary proxy config is applied, the config file is restored but the shared gateway process is not restarted, so later test binaries can inherit the live temporary proxy configuration.
Docs: docs/reference/gateway-config.mdx and docs/reference/sandbox-compute-drivers.mdx cover the operator contract; no docs/index.yml change is needed because the existing reference folder is already navigated.
test:e2e remains required. /ok to test is deferred until the cleanup defect is fixed and the new head is re-reviewed.
@feloy, please push an updated commit addressing the inline finding.
Next state: gator:in-review
| // Panic path: put the original config back so later test binaries in | ||
| // this run do not inherit the proxy settings. `ManagedGateway::drop` | ||
| // restarts the gateway from the restored file. | ||
| let _ = std::fs::write(&self.config_path, &self.original); |
There was a problem hiding this comment.
gator-agent
Medium: This panic cleanup restores only the TOML file; there is no live ManagedGateway value here whose Drop will restart the process. A panic after apply() therefore leaves the shared gateway running with the temporary proxy configuration after the support proxy is removed, which can poison later test binaries. Restore and synchronously restart the managed gateway here (or isolate this test behind its own gateway), and set restored = true only after the normal restore/restart succeeds.
The panic cleanup for the temporary corporate proxy configuration restored the gateway TOML but left the gateway process running with the temporary configuration still loaded, which could poison later test binaries in the same run. Nothing restarted it: the only ManagedGateway is the short-lived one inside restart_gateway, and its Drop only calls start, which does not reload config for an already-running gateway. Restore and synchronously stop/start the gateway in Drop, and set restored only after the normal restore and restart both succeed so a failed restart no longer suppresses the fallback. Signed-off-by: Philippe Martin <phmartin@redhat.com>
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: This remains project-valid work for the corporate restricted-egress gap in #1792. The Podman and supervisor implementation is coherent, with the relevant Fern and architecture documentation present.
Head SHA: c2c797ea7f7457ab1fc97615f503ce707da0a718
Thanks @feloy. I checked the latest commit. Restoring the original config before synchronously stopping and starting the managed gateway resolves the prior panic-path cleanup finding; the normal path also leaves restored false until the restart succeeds.
The fresh independent code review found one remaining proxy-bypass edge case, anchored inline.
One additional cross-file cleanup gap remains: proxy-auth secrets need crash-recovery reconciliation (CWE-459). The driver persists openshell-proxy-auth-* in Podman before container creation, but cleans it only on returned create errors or explicit sandbox deletion. If the gateway terminates after secret creation, the server's missing-container reconciliation prunes the database record without asking the driver to remove that secret, leaving long-lived corporate credentials behind indefinitely. Add startup/runtime secret reconciliation against active sandbox IDs and a crash-recovery regression.
Docs: docs/reference/gateway-config.mdx and docs/reference/sandbox-compute-drivers.mdx cover the operator contract; no docs/index.yml change is needed because the existing reference folder is already navigated.
test:e2e remains required. /ok to test is deferred until these findings are fixed and the new head is re-reviewed.
@feloy, please push an updated commit addressing the inline bypass and orphaned-secret reconciliation findings.
Next state: gator:in-review
| resolved: &[SocketAddr], | ||
| ) -> Option<Vec<SocketAddr>> { | ||
| let host_ip = host.trim_matches(['[', ']']).parse::<IpAddr>().ok(); | ||
| if host == "localhost" || host_ip.is_some_and(|ip| ip.is_loopback()) { |
There was a problem hiding this comment.
gator-agent
Warning (CWE-20): Do not treat the string localhost as proof that the destination is loopback. resolved can come from the sandbox's /etc/hosts; mapping localhost to an allowed non-loopback address makes this bypass the operator proxy. Derive the automatic bypass from resolved loopback IPs only—or require explicit NO_PROXY—and add a spoofed-resolution regression test.
2 participants
Summary
allowed_ipsvalidation; the hostname still travels inside the tunnel (TLS SNI, applicationHost). An explicitproxy_connect_by_hostnameopt-in exists for proxies whose ACLs filter on hostnames.HTTPS_PROXY/NO_PROXYvariables a sandbox sets have no effect. Reserved variable names are stripped from workload child processes.proxy_auth_file), never through the environment, container metadata, or the proxy URL. BecauseProxy-Authorization: Basictravels over plain TCP to thehttp://proxy, sending credentials additionally requires the explicitproxy_auth_allow_insecure = trueacknowledgement.user:pass@, unreadable or malformed credential, credentials without the insecure-auth acknowledgement, port 0, or any auxiliary setting without a proxy) is fatal at gateway startup or sandbox startup instead of silently degrading to direct or unauthenticated egress. Driver and supervisor share single URL/credential validators inopenshell-core, so a value accepted at sandbox-create time can never be rejected in-container or vice versa.O_NONBLOCK) to reject FIFOs and other blocking special files promptly.Related Issue
Part of #1792
Changes
openshell-supervisor-network: newupstream_proxymodule — supervisor-argv parsing,NO_PROXYmatching, CONNECT handshake with optionalProxy-Authorization: Basicfrom the auth file.decision(host, port, resolved)implements port-aware, resolution-awareNO_PROXY: entries take an optional:portqualifier that limits them to that destination port, and IP/CIDR entries also match hostnames through their validated resolved addresses, with the direct dial restricted to the addresses the entry contains.connect_viatakes aConnectTarget(validated IP by default, hostname on operator opt-in) and returns aPrefixedStreamthat replays any tunneled bytes received in the same read as the CONNECT response instead of discarding them.[::1]:443).openshell-core: sharedparse_upstream_proxy_url(stricthttp://host:portgrammar — explicit scheme and port required, rejects port 0, rejects bracketed IPv6 with empty port) andparse_upstream_proxy_credentialvalidators (bounded reads, non-blocking open to reject FIFOs); credential errors never carry credential content. Reservedsandbox_envnames, includingOPENSHELL_UPSTREAM_PROXY_AUTH_ALLOW_INSECUREandOPENSHELL_UPSTREAM_PROXY_CONNECT_BY_HOSTNAME, and the fixed secret mount path.openshell-driver-podman:PodmanComputeConfiggainshttps_proxy,no_proxy,proxy_auth_file,proxy_auth_allow_insecure, andproxy_connect_by_hostnamewith fail-closed validation mirrored by the supervisor; container creation delivers proxy config on the supervisor argv (not container environment) and stages the credential as a per-sandbox root-only secret (cleaned up with the sandbox).openshell-sandbox: main entry point parses proxy settings from supervisor argv and passes them to the network supervisor.openshell-supervisor-process: reserved proxy variables added to the supervisor-only strip list so workload children never inherit them.tasks/scripts/gateway.sh: dev gateway writes the proxy settings intogateway.tomlwith TOML-escaped values (quotes, backslashes, control characters cannot corrupt the config or inject keys); boolean flags are written only as genuine TOML booleans, with anything else emitted as a string so the gateway rejects it at startup. Set-but-empty values are preserved so validation rejects them instead of silently dropping them.docs/reference/gateway-config.mdx: documents the proxy fields, the strict URL grammar, theNO_PROXYsemantics, the CONNECT binding and its hostname opt-out, the cleartext-credential acknowledgement, and the fail-closed contract.docs/reference/sandbox-compute-drivers.mdx: lists corporate proxy keys in the Podman compute-driver overview.architecture/sandbox.md: corporate proxy data flow, trust boundary, CONNECT-target binding, argv transport, and fail-closed invariants.Commits
feat(sandbox,gateway): route sandbox egress through corporate HTTP proxyfix(sandbox,podman): make corporate proxy routing operator-ownedfeat(sandbox,podman): deliver corporate proxy credentials via secret filefix(sandbox,podman): fail closed on invalid upstream proxy configurationfix(sandbox,podman): finish the fail-closed upstream proxy credential contractfix(sandbox,podman): close remaining fail-open upstream proxy config pathsfix(sandbox,podman): reject upstream proxy URLs with path, query, or fragmentfix(sandbox,podman): remove plain-HTTP upstream proxy supportfix(sandbox,podman): escape generated TOML and require explicit proxy URL formfix(sandbox,podman): preserve tunneled bytes read with the CONNECT responsefix(sandbox,podman): gate cleartext proxy Basic auth behind an explicit opt-infix(sandbox,podman): honor port qualifiers and resolved addresses in NO_PROXYfix(sandbox,podman): bind proxied CONNECT tunnels to validated addressesfix(sandbox,podman): reject empty port after bracketed IPv6 proxy hostfix(sandbox,podman): fall back across validated addresses in proxied CONNECTfix(sandbox,podman): strip new reserved proxy vars and complete docs/testsfix(sandbox,podman): cap each proxied CONNECT attempt within the shared budgetfix(sandbox,podman): deliver corporate proxy config on the supervisor argvdocs(sandbox): align proxy comments with the argv transportfix(sandbox): parse bracketed IPv6 authorities in client CONNECT targetstest(podman): cover proxy-auth secret cleanup across lifecycle failuresdocs: list corporate proxy keys in the Podman compute-driver overviewtest(sandbox): cover the SSRF-to-TLS composition across the proxy tunnelfix(sandbox,podman): bound proxy-auth reads, reject port 0, fix stale commentfix(sandbox): open proxy-auth file non-blocking to reject FIFOs promptlyTesting
Unit tests for reserved-variable parsing and fail-closed rejection: empty values, bad schemes, missing scheme/port, URL components, inline credentials, malformed or unreadable auth files, credentials without the insecure-auth acknowledgement, invalid acknowledgement values, port 0, bracketed IPv6 with empty port, and auxiliary settings without a proxy
Unit tests for
NO_PROXYmatching: wildcards, domains, CIDR, loopback bypass, port-qualified entries on domain/IP/CIDR patterns, invalid port qualifiers, resolved-address matching, and split-resolution subset dialingUnit tests for the CONNECT handshake: success, auth header, non-200 rejection, malformed response, timeout, IPv6 bracketing in both target modes, validated-IP request lines with hostname non-leakage, hostname opt-in, preservation of tunneled bytes read with the CONNECT response, fallback across validated addresses, and per-attempt timeout budgeting
Unit tests for the shared URL/credential validators in
openshell-core(including bounded reads, non-blocking FIFO rejection) and for Podman config validation, container-spec argv injection/env stripping, and proxy-auth secret cleanup across lifecycle failures; TOML escaping verified against a TOML parser with hostile valuesIntegration test covering the full SSRF-to-TLS composition across the proxy tunnel
Existing proxy and SSRF test suites pass — policy evaluation is unchanged
mise run pre-commitpassesUnit tests added/updated
E2E tests added/updated (deferred pending gator re-review and
/ok to test)Checklist