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 16 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: |
- Chain sandbox egress through a corporate HTTP proxy so outbound traffic from within the sandbox respects the host proxy settings - Forward sandbox proxy environment variables to the generated Podman config so the proxy is applied consistently to Podman-managed workloads Signed-off-by: Philippe Martin <phmartin@redhat.com>
The operator-configured corporate egress proxy was injected under the conventional HTTPS_PROXY/HTTP_PROXY/NO_PROXY names as defaults beneath sandbox spec/template environment, so a sandbox creator could redirect egress at an arbitrary proxy or disable proxying with NO_PROXY=*. Route the boundary through reserved, supervisor-only variables (OPENSHELL_UPSTREAM_HTTPS_PROXY/HTTP_PROXY/NO_PROXY) written in the Podman driver's required-variable tier. Any sandbox-supplied value under a reserved name is stripped before the operator value is applied, so the supervisor never observes a reserved proxy variable the operator did not set. The supervisor now reads only the reserved names and ignores the conventional proxy variables the sandbox controls. Add the reserved proxy variables to the supervisor-only child-environment denylist so the corporate proxy URL and any embedded credentials are not inherited by the sandbox workload, which reaches egress through the local policy proxy and never needs them. Signed-off-by: Philippe Martin <phmartin@redhat.com>
…file Proxy credentials were embedded inline in the proxy URL, so they were stored in gateway.toml and exposed in container metadata via 'podman inspect'. Reject inline 'user:pass@' credentials in https_proxy/http_proxy at startup (parsed with the url crate rather than hand-splitting), and add a proxy_auth_file option pointing at a 'user:pass' file. The driver stages that file as a per-sandbox root-only Podman secret, mounts it at a fixed path, and exports only the path in the reserved OPENSHELL_UPSTREAM_PROXY_AUTH_FILE variable, so the credential never appears in config, environment, or container metadata. Reading the file fails closed on a missing, empty, or control-character-bearing value. The supervisor reads the credential from the mounted file and builds the Proxy-Authorization: Basic header, rejecting control characters, and no longer derives credentials from URL userinfo. The auth-file path is added to the child-environment strip list, and the generated gateway.toml is written owner-only (mode 0600). Signed-off-by: Philippe Martin <phmartin@redhat.com>
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>
…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>
… URL form Escape backslashes, quotes, and control characters when gateway.sh writes proxy values into gateway.toml, so a hostile or unusual environment value cannot corrupt the config or inject extra keys. Restrict the upstream proxy URL grammar to the documented http://host:port form: a scheme-less value is no longer normalized to http:// and a missing port is no longer silently defaulted to 80. Docs, README, and CLI help now state the explicit-form requirement consistently. Also fix a test-only call of handle_tcp_connection that was missing the upstream_proxy argument added in an earlier commit. Signed-off-by: Philippe Martin <phmartin@redhat.com>
328a084 to
354b1d0
Compare
…sponse The CONNECT handshake reads the corporate proxy's response in chunks, so the read that completes the header block can also contain the first tunneled payload bytes. Those bytes were discarded, silently corrupting the start of the tunnel for server-speaks-first destinations or proxies that coalesce writes. connect_via now returns a PrefixedStream that replays any bytes received past the response terminator before reading from the socket again; writes pass through unchanged. Direct dials wrap the stream with an empty prefix so downstream relay and TLS paths keep a single stream type, and tls_connect_upstream is generalized to any AsyncRead + AsyncWrite stream. Adds regression coverage for a combined response/payload read and for prefix replay ordering. 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 Podman and supervisor scope is coherent, and the relevant Fern and architecture documentation is present. Head SHA: Thanks @feloy. I checked the newest commit's TOML escaping, explicit The fresh independent code review found these remaining blocking items:
The reserved operator-variable ownership, child-environment stripping, root-only credential secret, cleanup, and fail-closed parsing all look sound. Please also list the three new local-development environment variables in Docs:
Next state: |
…it opt-in Proxy-Authorization: Basic is base64 over the plain-TCP connection to the http:// corporate proxy, so anyone on the network path between the sandbox host and the proxy can recover the credential. Sending it is now an explicit operator decision instead of an implicit side effect of configuring proxy_auth_file. Add a proxy_auth_allow_insecure driver setting (CLI --sandbox-proxy-auth-allow-insecure, env OPENSHELL_SANDBOX_PROXY_AUTH_ALLOW_INSECURE), delivered to the supervisor as the reserved OPENSHELL_UPSTREAM_PROXY_AUTH_ALLOW_INSECURE variable. Fail-closed pairing on both sides: an auth file without the acknowledgement is rejected at gateway startup and at supervisor startup, as is the acknowledgement without an auth file or any value other than 'true'. gateway.sh writes the key only as a TOML boolean; a non-boolean value is emitted as a quoted string so the gateway rejects it at startup instead of risking injection. Documents the exposure prominently in the gateway config reference, the driver README, and the sandbox architecture doc. 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 Podman and supervisor scope is coherent, and the relevant Fern and architecture documentation is present. Head SHA: Thanks @feloy. I checked the newest commit's The fresh independent code review found these remaining blocking items:
Please also list
Next state: |
…NO_PROXY Two divergences from the documented NO_PROXY contract: - A port-qualified entry (internal.corp:8443) was silently stripped to its hostname and bypassed the proxy for every port, excluding traffic the operator never listed. Entries now keep the optional :port qualifier (also on IP and CIDR entries) and only apply to that destination port; a trailing qualifier that is not a valid port stays part of the pattern instead of widening the entry. - IP and CIDR entries only matched IP-literal hosts, so a bypass like 10.0.0.0/8 never applied to hostnames resolving into that range. NO_PROXY evaluation now sees the validated resolved addresses: an IP/CIDR entry matching through resolution authorizes a direct dial of only the addresses it contains, so a bypass scoped to an internal range cannot widen into a direct dial of addresses outside it. Hostname-level matches (loopback, wildcard, domain entries, IP-literal hosts) keep authorizing all validated addresses. proxy_for is replaced by decision(host, port, resolved) returning either the proxy endpoint or the permitted direct-dial subset, and dial_upstream restricts the direct connect to that subset. Adds regression coverage for port-scoped bypasses on domain, IP, and CIDR entries, invalid port qualifiers, resolved-address matching, and split-resolution subset dialing. 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 Podman and supervisor scope is coherent, with the relevant Fern and architecture documentation present. Head SHA: Thanks @feloy. I checked the newest commit's cleartext Basic-auth acknowledgement across driver validation, reserved-variable injection, supervisor parsing, and docs. The prior credential-transport finding is resolved: The fresh independent code review found these remaining blocking items:
Required follow-up alongside those fixes:
Docs:
Next state: |
PR Review StatusValidation: 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: Thanks @feloy. I checked the newest commit's port-qualified and resolved-address The fresh independent code review found these remaining blocking items:
Required follow-ups:
Docs:
Next state: |
The CONNECT request sent to the corporate proxy carried the destination hostname, so the proxy resolved the name itself and the addresses that had passed SSRF and allowed_ips validation were discarded. Split-horizon DNS or rebinding at the proxy could then reach internal or otherwise unapproved destinations through a tunnel the supervisor logged as validated, and IP-range policy could not be enforced at all on proxied dials. CONNECT now targets a validated resolved address by default: the proxy performs no DNS resolution and the tunnel stays bound to the answer the supervisor checked. The hostname still travels inside the tunnel (TLS SNI, application Host), so destination servers behave normally. In split-horizon networks, operators point the gateway host at the corporate resolver so internal names validate to their internal addresses. For proxies whose ACLs filter on hostnames and reject IP CONNECT targets, a new proxy_connect_by_hostname opt-in (CLI --sandbox-proxy-connect-by-hostname, env OPENSHELL_SANDBOX_PROXY_CONNECT_BY_HOSTNAME, reserved OPENSHELL_UPSTREAM_PROXY_CONNECT_BY_HOSTNAME) restores hostname CONNECT, documented as re-opening proxy-side resolution and making the proxy's ACLs the effective egress control. Fail-closed pairing on both sides: the opt-in without a proxy, or any value other than 'true', is fatal. Adds regression coverage for the IP CONNECT request line (including IPv6 bracketing and hostname non-leakage), the hostname opt-in, and the config pairing rules in driver and supervisor. 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 Podman and supervisor scope is coherent, and the relevant Fern and architecture documentation is present. Head SHA: Thanks @feloy. I checked the newest commit's validated-address CONNECT implementation. The prior SSRF binding blocker is resolved: the default proxy path now sends a locally validated IP in CONNECT while retaining the requested hostname for inner TLS SNI and certificate verification. The explicit hostname mode is operator-controlled, fail-closed, and documents the security tradeoff. The fresh independent code review found these remaining blocking items:
Required follow-ups alongside those fixes:
Docs:
Next state: |
http://[fd00::1]: passed the explicit-port check because the bracketed branch only tested for a colon after the bracket, then fell back to port 80 — violating the fail-closed http://host:port contract and potentially sending configured Basic credentials to an unintended service. Require a non-empty suffix after ]:, matching the unbracketed branch, and cover the case in the shared parser tests. Signed-off-by: Philippe Martin <phmartin@redhat.com>
…CONNECT The direct path hands TcpStream::connect the whole validated address list and it falls back across them, but the validated-IP CONNECT path attempted only the first address, so a dual-stack destination could fail through the corporate proxy even when a later validated address was reachable. connect_via_validated tries each validated address in order under one aggregate CONNECT_HANDSHAKE_TIMEOUT budget, returning the first success; when every attempt fails the error names the attempt count and carries the last failure. An empty address list is rejected up front. Adds regressions for first-fails/second-succeeds fallback (asserting both CONNECT request lines), the aggregate all-addresses failure message, and the empty-list rejection. 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>
PR Review StatusValidation: 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: Thanks @feloy. I checked the newest commits. The empty bracketed-IPv6 port is now rejected, validated-IP CONNECT retries immediate failures across addresses, both new reserved proxy variables are stripped from workload children, the gateway help and Podman README list the five settings, and the IPv6 The fresh independent code review found these remaining blocking items:
Required follow-ups alongside those fixes:
Runtime proxy behavior requires Next state: |
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.OPENSHELL_UPSTREAM_*variables in its required-variable tier, so sandbox/template environment cannot override them and the conventionalHTTPS_PROXY/NO_PROXYvariables a sandbox sets have no effect. The reserved 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, 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.Related Issue
Part of #1792
Changes
openshell-supervisor-network: newupstream_proxymodule — reserved-variable 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.openshell-core: sharedparse_upstream_proxy_url(stricthttp://host:portgrammar — explicit scheme and port required) andparse_upstream_proxy_credentialvalidators; credential errors never carry credential content. Reservedsandbox_envnames, including the newOPENSHELL_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 injects the reserved variables and stages the credential as a per-sandbox root-only secret (cleaned up with the sandbox).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.architecture/sandbox.md: corporate proxy data flow, trust boundary, CONNECT-target binding, 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 addressesTesting
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, 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, and preservation of tunneled bytes read with the CONNECT response
Unit tests for the shared URL/credential validators in
openshell-coreand for Podman config validation and container-spec env injection/stripping; TOML escaping verified against a TOML parser with hostile valuesExisting 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