Skip to content

Commit a82cf26

Browse files
feloyfeloy
committed
fix(sandbox,podman): fail closed on invalid upstream proxy configuration
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 #2245. Signed-off-by: Philippe Martin <[email protected]>
1 parent fa0082c commit a82cf26

8 files changed

Lines changed: 420 additions & 215 deletions

File tree

‎Cargo.lock‎

Lines changed: 0 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎architecture/sandbox.md‎

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -118,14 +118,22 @@ the corporate proxy is the requested hostname. The workload child's proxy
118118
variables are unaffected — they are always rewritten to point at the local
119119
policy proxy.
120120

121+
The configuration is fail-closed: a reserved variable that is present but
122+
invalid — an unsupported or malformed proxy URL, an unreadable auth file, or a
123+
malformed credential — is fatal to supervisor startup instead of being treated
124+
as unset, so a misconfiguration can never silently degrade to direct dialing
125+
or unauthenticated proxy access. The driver validates the same rules at
126+
sandbox-create time through a validator shared with the supervisor
127+
(`openshell_core::driver_utils::parse_upstream_proxy_url`).
128+
121129
Proxy credentials are never embedded in the URL: an inline `user:pass@` is
122-
rejected at startup because it would be stored in `gateway.toml` and exposed in
123-
container metadata. Operators supply credentials via `proxy_auth_file`; the
124-
driver stages them as a root-only secret mounted at a fixed path and exports
125-
only that path in `OPENSHELL_UPSTREAM_PROXY_AUTH_FILE`. The supervisor reads the
126-
file and builds the `Proxy-Authorization: Basic` header, rejecting credentials
127-
containing control characters. The reserved proxy variables — including the
128-
auth-file path — are stripped from workload child processes.
130+
rejected because it would be stored in `gateway.toml` and exposed in container
131+
metadata. Operators supply credentials via `proxy_auth_file`; the driver
132+
stages them as a root-only secret mounted at a fixed path and exports only
133+
that path in `OPENSHELL_UPSTREAM_PROXY_AUTH_FILE`. The supervisor reads the
134+
file and builds the `Proxy-Authorization: Basic` header; an empty credential
135+
or one containing control characters is fatal. The reserved proxy variables —
136+
including the auth-file path — are stripped from workload child processes.
129137

130138
## Credentials
131139

‎crates/openshell-core/src/driver_utils.rs‎

Lines changed: 152 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,97 @@ pub const SANDBOX_TOKEN_MOUNT_PATH: &str = "/etc/openshell/auth/sandbox.jwt";
7979
/// mount so the credential never appears in container environment/metadata.
8080
pub const UPSTREAM_PROXY_AUTH_MOUNT_PATH: &str = "/etc/openshell/auth/upstream-proxy";
8181

82+
/// A validated corporate upstream-proxy address.
83+
///
84+
/// Produced by [`parse_upstream_proxy_url`], which is the single source of
85+
/// truth for what counts as a valid upstream proxy URL. Compute drivers use
86+
/// it to reject bad operator config at sandbox-create time, and the
87+
/// in-container supervisor applies the same rules to the reserved
88+
/// `OPENSHELL_UPSTREAM_*` variables so a value one side accepts is never
89+
/// rejected (or silently ignored) by the other.
90+
#[derive(Debug, Clone, PartialEq, Eq)]
91+
pub struct UpstreamProxyAddr {
92+
/// Proxy hostname, IPv4, or IPv6 address (IPv6 without brackets).
93+
pub host: String,
94+
/// Proxy TCP port (defaults to 80 when the URL has none).
95+
pub port: u16,
96+
}
97+
98+
/// Why an upstream proxy URL was rejected by [`parse_upstream_proxy_url`].
99+
///
100+
/// Kept as a typed error so each consumer (driver config validation,
101+
/// supervisor startup) can phrase the message for its own surface while
102+
/// enforcing identical semantics.
103+
#[derive(Debug, Clone, PartialEq, Eq, thiserror::Error)]
104+
pub enum UpstreamProxyUrlError {
105+
/// The value is empty or whitespace-only.
106+
#[error("proxy URL is empty")]
107+
Empty,
108+
/// The value does not parse as a URL.
109+
#[error("not a valid proxy URL: {0}")]
110+
Invalid(url::ParseError),
111+
/// The URL uses a scheme other than `http` (TLS and SOCKS proxies are
112+
/// not supported by the sandbox supervisor).
113+
#[error(
114+
"unsupported proxy scheme '{0}': only http:// forward proxies are \
115+
supported by the sandbox supervisor"
116+
)]
117+
UnsupportedScheme(String),
118+
/// The URL embeds `user:pass@` credentials, which would leak into config
119+
/// and container metadata. Credentials must come from the proxy auth file.
120+
#[error("proxy URL must not embed credentials; supply them via the proxy auth file")]
121+
InlineCredentials,
122+
/// The URL has no host component.
123+
#[error("proxy URL is missing a proxy host")]
124+
MissingHost,
125+
}
126+
127+
/// Parse and validate a corporate upstream-proxy URL.
128+
///
129+
/// A bare `host:port` (no `://`) is normalized to `http://host:port`. Only
130+
/// `http://` proxies are accepted, inline userinfo is rejected, and the port
131+
/// defaults to 80.
132+
///
133+
/// # Errors
134+
///
135+
/// Returns an [`UpstreamProxyUrlError`] describing the first rule the value
136+
/// violates.
137+
pub fn parse_upstream_proxy_url(raw: &str) -> Result<UpstreamProxyAddr, UpstreamProxyUrlError> {
138+
let trimmed = raw.trim();
139+
if trimmed.is_empty() {
140+
return Err(UpstreamProxyUrlError::Empty);
141+
}
142+
let parsed = if trimmed.contains("://") {
143+
url::Url::parse(trimmed)
144+
} else {
145+
url::Url::parse(&format!("http://{trimmed}"))
146+
}
147+
.map_err(UpstreamProxyUrlError::Invalid)?;
148+
149+
if !parsed.scheme().eq_ignore_ascii_case("http") {
150+
return Err(UpstreamProxyUrlError::UnsupportedScheme(
151+
parsed.scheme().to_string(),
152+
));
153+
}
154+
if !parsed.username().is_empty() || parsed.password().is_some() {
155+
return Err(UpstreamProxyUrlError::InlineCredentials);
156+
}
157+
let host = match parsed.host() {
158+
// `Host::Ipv6` renders without brackets, which is what socket
159+
// connect APIs expect.
160+
Some(url::Host::Ipv6(ip)) => ip.to_string(),
161+
Some(host) => host.to_string(),
162+
None => return Err(UpstreamProxyUrlError::MissingHost),
163+
};
164+
if host.is_empty() {
165+
return Err(UpstreamProxyUrlError::MissingHost);
166+
}
167+
Ok(UpstreamProxyAddr {
168+
host,
169+
port: parsed.port().unwrap_or(80),
170+
})
171+
}
172+
82173
/// Return the XDG state path for a driver's sandbox JWT token file.
83174
///
84175
/// The resulting path is `$XDG_STATE_HOME/openshell/<driver_subdir>[/<namespace>]/<sandbox_id>/sandbox.jwt`.
@@ -167,3 +258,64 @@ pub fn supervisor_image_tag(image: &str) -> Option<&str> {
167258
pub fn supervisor_image_should_refresh(image: &str) -> bool {
168259
matches!(supervisor_image_tag(image), Some("dev" | "latest"))
169260
}
261+
262+
#[cfg(test)]
263+
mod tests {
264+
use super::*;
265+
266+
#[test]
267+
fn upstream_proxy_url_accepts_http_with_port() {
268+
let addr = parse_upstream_proxy_url("http://proxy.corp.com:8080").unwrap();
269+
assert_eq!(addr.host, "proxy.corp.com");
270+
assert_eq!(addr.port, 8080);
271+
}
272+
273+
#[test]
274+
fn upstream_proxy_url_defaults_scheme_and_port() {
275+
let addr = parse_upstream_proxy_url("proxy.corp.com").unwrap();
276+
assert_eq!(addr.host, "proxy.corp.com");
277+
assert_eq!(addr.port, 80);
278+
let addr = parse_upstream_proxy_url("proxy.corp.com:3128").unwrap();
279+
assert_eq!(addr.port, 3128);
280+
}
281+
282+
#[test]
283+
fn upstream_proxy_url_ipv6_host_is_bracket_free() {
284+
let addr = parse_upstream_proxy_url("http://[fd00::1]:8080").unwrap();
285+
assert_eq!(addr.host, "fd00::1");
286+
assert_eq!(addr.port, 8080);
287+
}
288+
289+
#[test]
290+
fn upstream_proxy_url_rejects_tls_and_socks_schemes() {
291+
for url in ["https://proxy:443", "socks5://proxy:1080"] {
292+
assert!(matches!(
293+
parse_upstream_proxy_url(url),
294+
Err(UpstreamProxyUrlError::UnsupportedScheme(_))
295+
));
296+
}
297+
}
298+
299+
#[test]
300+
fn upstream_proxy_url_rejects_inline_credentials() {
301+
for url in ["http://user:pass@proxy:8080", "http://user@proxy:8080"] {
302+
assert_eq!(
303+
parse_upstream_proxy_url(url),
304+
Err(UpstreamProxyUrlError::InlineCredentials)
305+
);
306+
}
307+
}
308+
309+
#[test]
310+
fn upstream_proxy_url_rejects_empty_and_invalid() {
311+
assert_eq!(
312+
parse_upstream_proxy_url(" "),
313+
Err(UpstreamProxyUrlError::Empty)
314+
);
315+
assert!(matches!(
316+
parse_upstream_proxy_url("http://proxy:notaport"),
317+
Err(UpstreamProxyUrlError::Invalid(_))
318+
));
319+
assert!(parse_upstream_proxy_url("http://").is_err());
320+
}
321+
}

‎crates/openshell-driver-podman/Cargo.toml‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,6 @@ tracing = { workspace = true }
3434
tracing-subscriber = { workspace = true }
3535
thiserror = { workspace = true }
3636
miette = { workspace = true }
37-
url = { workspace = true }
3837

3938
[dev-dependencies]
4039
prost-types = { workspace = true }

‎crates/openshell-driver-podman/src/config.rs‎

Lines changed: 22 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -6,19 +6,6 @@ use std::net::IpAddr;
66
use std::path::PathBuf;
77
use std::str::FromStr;
88

9-
/// Parse a proxy URL, defaulting a missing scheme to `http://`.
10-
///
11-
/// A bare `host:port` (no `://`) is normalized to `http://host:port` so it
12-
/// parses the same way the in-sandbox supervisor treats a scheme-less proxy
13-
/// value. Returns the parsed [`url::Url`] for scheme/userinfo/host inspection.
14-
fn parse_proxy_url(raw: &str) -> Result<url::Url, url::ParseError> {
15-
if raw.contains("://") {
16-
url::Url::parse(raw)
17-
} else {
18-
url::Url::parse(&format!("http://{raw}"))
19-
}
20-
}
21-
229
/// Default Podman bridge network name.
2310
pub const DEFAULT_NETWORK_NAME: &str = "openshell";
2411
pub const MACOS_PODMAN_MACHINE_HOST_GATEWAY_IP: &str = "192.168.127.254";
@@ -233,49 +220,34 @@ impl PodmanComputeConfig {
233220

234221
/// Validate optional corporate proxy configuration.
235222
///
236-
/// The in-container supervisor only supports `http://` forward proxies, so
237-
/// reject other schemes at config time instead of silently ignoring them
238-
/// inside every sandbox. Credentials must be supplied through
239-
/// `proxy_auth_file`; an inline `user:pass@` in the URL is rejected because
240-
/// it would otherwise be stored in `gateway.toml` and exposed in container
241-
/// metadata.
223+
/// Shares validation semantics with the in-container supervisor through
224+
/// [`openshell_core::driver_utils::parse_upstream_proxy_url`], so a value
225+
/// accepted here can never be rejected by the supervisor at sandbox
226+
/// startup (or vice versa). The supervisor only supports `http://`
227+
/// forward proxies, so other schemes are rejected at config time instead
228+
/// of failing inside every sandbox. Credentials must be supplied through
229+
/// `proxy_auth_file`; an inline `user:pass@` in the URL is rejected
230+
/// because it would otherwise be stored in `gateway.toml` and exposed in
231+
/// container metadata.
242232
pub fn validate_proxy_config(&self) -> Result<(), crate::client::PodmanApiError> {
233+
use openshell_core::driver_utils::{UpstreamProxyUrlError, parse_upstream_proxy_url};
243234
for (field, value) in [
244235
("https_proxy", &self.https_proxy),
245236
("http_proxy", &self.http_proxy),
246237
] {
247238
let Some(url) = value else { continue };
248-
let trimmed = url.trim();
249-
if trimmed.is_empty() {
250-
return Err(crate::client::PodmanApiError::InvalidInput(format!(
251-
"{field} must not be empty when set"
252-
)));
253-
}
254-
255-
let parsed = parse_proxy_url(trimmed).map_err(|e| {
256-
crate::client::PodmanApiError::InvalidInput(format!(
257-
"{field} is not a valid proxy URL: {e}"
258-
))
239+
parse_upstream_proxy_url(url).map_err(|err| {
240+
crate::client::PodmanApiError::InvalidInput(match err {
241+
UpstreamProxyUrlError::Empty => {
242+
format!("{field} must not be empty when set")
243+
}
244+
UpstreamProxyUrlError::InlineCredentials => format!(
245+
"{field} must not embed credentials in the URL; supply them via \
246+
proxy_auth_file so they are not stored in config or container metadata"
247+
),
248+
err => format!("{field} {err}"),
249+
})
259250
})?;
260-
261-
if !parsed.scheme().eq_ignore_ascii_case("http") {
262-
return Err(crate::client::PodmanApiError::InvalidInput(format!(
263-
"{field} uses unsupported scheme '{}': only http:// forward \
264-
proxies are supported by the sandbox supervisor",
265-
parsed.scheme()
266-
)));
267-
}
268-
if !parsed.username().is_empty() || parsed.password().is_some() {
269-
return Err(crate::client::PodmanApiError::InvalidInput(format!(
270-
"{field} must not embed credentials in the URL; supply them via \
271-
proxy_auth_file so they are not stored in config or container metadata"
272-
)));
273-
}
274-
if parsed.host_str().is_none_or(str::is_empty) {
275-
return Err(crate::client::PodmanApiError::InvalidInput(format!(
276-
"{field} is missing a proxy host"
277-
)));
278-
}
279251
}
280252

281253
if let Some(path) = self.proxy_auth_file.as_deref() {
@@ -538,7 +510,7 @@ mod tests {
538510
};
539511
let err = cfg.validate_proxy_config().unwrap_err();
540512
assert!(
541-
err.to_string().contains("unsupported scheme"),
513+
err.to_string().contains("unsupported proxy scheme"),
542514
"{url}: {err}"
543515
);
544516
}

‎crates/openshell-supervisor-network/src/proxy.rs‎

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -246,8 +246,27 @@ impl ProxyHandle {
246246
// variables. Read once at startup; the workload cannot influence the
247247
// supervisor's environment, and the conventional HTTPS_PROXY/NO_PROXY
248248
// variables it does control are ignored on this path.
249+
//
250+
// This is an operator-owned security boundary, so a present-but-invalid
251+
// value (bad proxy URL, unreadable auth file, malformed credential) is
252+
// fatal to proxy startup: failing closed prevents a misconfiguration
253+
// from silently degrading to direct dialing or unauthenticated proxy
254+
// access.
249255
let upstream_proxy: Arc<Option<UpstreamProxyConfig>> =
250-
Arc::new(UpstreamProxyConfig::from_env());
256+
Arc::new(UpstreamProxyConfig::from_env().map_err(|err| {
257+
let event =
258+
openshell_ocsf::ConfigStateChangeBuilder::new(openshell_ocsf::ctx::ctx())
259+
.severity(SeverityId::High)
260+
.status(StatusId::Failure)
261+
.state(openshell_ocsf::StateId::Disabled, "invalid")
262+
.message(format!(
263+
"Upstream corporate proxy configuration invalid; \
264+
refusing to start: {err}"
265+
))
266+
.build();
267+
ocsf_emit!(event);
268+
miette::miette!("invalid upstream corporate proxy configuration: {err}")
269+
})?);
251270
if let Some(cfg) = upstream_proxy.as_ref() {
252271
let event = openshell_ocsf::ConfigStateChangeBuilder::new(openshell_ocsf::ctx::ctx())
253272
.severity(SeverityId::Informational)

0 commit comments

Comments
 (0)