mirror of
https://github.com/NVIDIA/OpenShell.git
synced 2026-10-30 07:33:59 +08:00
fix(driver-mxc): emit MXC network.proxy as {localhost: port}
Verified against the real wxc-exec 0.6.0-alpha via --dry-run: MXC accepts
only the {localhost: N} proxy shape (the form the design doc specifies)
and rejects {host, port} with a parse error. Schema 0.6.0-alpha can
express only a loopback port, so non-127.0.0.1 redirect addresses are now
rejected: split_policy emits an error loss (no proxy block) and the driver
refuses egress_proxy_addr values off 127.0.0.1. Per-sandbox attribution
must use per-sandbox ports until the schema widens.
Signed-off-by: Giedrius Burachas <gburachas@nvidia.com>
(cherry picked from commit edde8d5434571fd5398409204fcf6862672c0793)
Signed-off-by: Jamie King <jamiek@nvidia.com>
This commit is contained in:
committed by
Drew Newberry
parent
a81f472aef
commit
c69e9ff162
@@ -217,11 +217,24 @@ fn configured_egress_addr(config: &MxcComputeConfig) -> Result<Option<SocketAddr
|
||||
"mxc egress_proxy_addr is required when egress_proxy is enabled",
|
||||
));
|
||||
}
|
||||
raw.parse::<SocketAddr>().map(Some).map_err(|e| {
|
||||
let addr = raw.parse::<SocketAddr>().map_err(|e| {
|
||||
tonic::Status::invalid_argument(format!(
|
||||
"mxc egress_proxy_addr must be an IP:PORT socket address: {e}"
|
||||
))
|
||||
})
|
||||
})?;
|
||||
// MXC 0.6.0-alpha expresses the redirect as {"proxy": {"localhost": N}}
|
||||
// — it has no way to encode a non-loopback host. Reject early so the
|
||||
// operator gets a clear message rather than a silent policy gap.
|
||||
// Use 127.0.0.1:PORT. Future schema versions may lift this restriction.
|
||||
if addr.ip() != std::net::IpAddr::from([127, 0, 0, 1]) {
|
||||
return Err(tonic::Status::invalid_argument(format!(
|
||||
"mxc egress_proxy_addr must be 127.0.0.1:PORT — MXC 0.6.0-alpha \
|
||||
expresses the redirect as {{\"localhost\": N}} and cannot encode \
|
||||
non-loopback addresses (got {})",
|
||||
addr.ip()
|
||||
)));
|
||||
}
|
||||
Ok(Some(addr))
|
||||
}
|
||||
|
||||
impl MxcComputeBackend {
|
||||
@@ -871,6 +884,29 @@ mod lifecycle_tests {
|
||||
assert!(config.egress_proxy_addr.is_empty());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn egress_non_loopback_addr_is_rejected() {
|
||||
// MXC 0.6.0-alpha can only express {"proxy": {"localhost": N}}, so
|
||||
// non-127.0.0.1 redirect addresses must be rejected at validate time.
|
||||
let mut config = demo_config(
|
||||
"C:/work/demo",
|
||||
vec!["cmd".into(), "/c".into(), "exit 0".into()],
|
||||
);
|
||||
config.backend = MxcBackend::ProcessContainer;
|
||||
config.egress_proxy = true;
|
||||
config.egress_proxy_addr = "10.0.0.1:18080".into();
|
||||
let backend = MxcComputeBackend::new_mocked(config);
|
||||
let err = backend
|
||||
.validate_sandbox_create(&driver_sandbox("sb-nonlocal"))
|
||||
.unwrap_err();
|
||||
assert_eq!(err.code(), tonic::Code::InvalidArgument);
|
||||
assert!(
|
||||
err.message().contains("127.0.0.1"),
|
||||
"error should mention 127.0.0.1, got: {}",
|
||||
err.message()
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn egress_on_isolation_session_is_rejected() {
|
||||
let mut config = demo_config(
|
||||
@@ -1062,8 +1098,16 @@ mod lifecycle_tests {
|
||||
.unwrap()
|
||||
.is_empty()
|
||||
);
|
||||
assert_eq!(recorded["network"]["proxy"]["host"], "127.0.0.1");
|
||||
assert_eq!(recorded["network"]["proxy"]["port"], 18080);
|
||||
// MXC 0.6.0-alpha accepts only {"proxy": {"localhost": N}}.
|
||||
assert_eq!(recorded["network"]["proxy"]["localhost"], 18080);
|
||||
assert!(
|
||||
recorded["network"]["proxy"].get("host").is_none(),
|
||||
"proxy must not contain 'host' key"
|
||||
);
|
||||
assert!(
|
||||
recorded["network"]["proxy"].get("port").is_none(),
|
||||
"proxy must not contain 'port' key"
|
||||
);
|
||||
|
||||
let reg = backend.registry.lock().await;
|
||||
let entry = reg.get("sb-egress").expect("registry entry");
|
||||
|
||||
@@ -94,16 +94,19 @@ pub struct MxcProcess {
|
||||
}
|
||||
|
||||
fn network_json(network: &MxcNetwork) -> serde_json::Value {
|
||||
// MXC 0.6.0-alpha schema accepts ONLY {"proxy": {"localhost": <port>}}.
|
||||
// {"host": ..., "port": ...} and every other shape is rejected — verified
|
||||
// empirically against the real wxc-exec 0.6.0-alpha binary via --dry-run.
|
||||
// See also docs/reference/mxc-compute-driver-design.mdx §network.proxy.
|
||||
// The MxcNetwork.proxy field remains SocketAddr so callers keep full
|
||||
// precision; only the port is serialized into the localhost key.
|
||||
let mut value = serde_json::json!({
|
||||
"defaultPolicy": network.default_policy.as_str(),
|
||||
"allowedHosts": [],
|
||||
"blockedHosts": [],
|
||||
});
|
||||
if let Some(proxy) = network.proxy {
|
||||
value["proxy"] = serde_json::json!({
|
||||
"host": proxy.ip().to_string(),
|
||||
"port": proxy.port(),
|
||||
});
|
||||
value["proxy"] = serde_json::json!({ "localhost": proxy.port() });
|
||||
}
|
||||
value
|
||||
}
|
||||
@@ -755,8 +758,31 @@ mod tests {
|
||||
.unwrap()
|
||||
.is_empty()
|
||||
);
|
||||
assert_eq!(config["network"]["proxy"]["host"], "127.0.0.1");
|
||||
assert_eq!(config["network"]["proxy"]["port"], 18080);
|
||||
// MXC 0.6.0-alpha accepts only {"proxy": {"localhost": N}}.
|
||||
assert_eq!(config["network"]["proxy"]["localhost"], 18080);
|
||||
assert!(
|
||||
config["network"]["proxy"].get("host").is_none(),
|
||||
"proxy must not contain 'host' key"
|
||||
);
|
||||
assert!(
|
||||
config["network"]["proxy"].get("port").is_none(),
|
||||
"proxy must not contain 'port' key"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn network_json_emits_localhost_port_shape() {
|
||||
// MXC 0.6.0-alpha rejects {"host":...,"port":...} and accepts only
|
||||
// {"proxy": {"localhost": N}} — verified against the real binary via
|
||||
// --dry-run. This test pins the exact emitted JSON shape.
|
||||
let network = MxcNetwork {
|
||||
default_policy: "block".into(),
|
||||
proxy: Some("127.0.0.1:18080".parse().unwrap()),
|
||||
};
|
||||
let value = network_json(&network);
|
||||
assert_eq!(value["proxy"]["localhost"], 18080);
|
||||
assert!(value["proxy"].get("host").is_none());
|
||||
assert!(value["proxy"].get("port").is_none());
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
@@ -172,16 +172,33 @@ fn build_split_mxc_config(
|
||||
|
||||
// Direct egress is blocked; all outbound flows through the OpenShell proxy.
|
||||
// allowedHosts is intentionally empty — the proxy enforces the full policy.
|
||||
//
|
||||
// MXC 0.6.0-alpha schema accepts ONLY {"proxy": {"localhost": <port>}}.
|
||||
// {"host": ..., "port": ...} and every other shape is rejected — verified
|
||||
// empirically against the real wxc-exec 0.6.0-alpha binary via --dry-run.
|
||||
// See also docs/reference/mxc-compute-driver-design.mdx §network.proxy.
|
||||
if proxy_supported && proxy_addr.ip() != std::net::IpAddr::from([127, 0, 0, 1]) {
|
||||
add_loss(
|
||||
items,
|
||||
"network.proxy",
|
||||
"error",
|
||||
&format!(
|
||||
"MXC schema 0.6.0-alpha can only express a localhost port \
|
||||
({{\"localhost\": N}}); non-127.0.0.1 redirect address {} \
|
||||
is not representable.",
|
||||
proxy_addr
|
||||
),
|
||||
"per-sandbox egress attribution",
|
||||
"The redirect cannot be emitted; use a 127.0.0.1:PORT address.",
|
||||
);
|
||||
}
|
||||
let mut network = json!({
|
||||
"defaultPolicy": "block",
|
||||
"allowedHosts": [],
|
||||
"blockedHosts": [],
|
||||
});
|
||||
if proxy_supported {
|
||||
network["proxy"] = json!({
|
||||
"host": proxy_addr.ip().to_string(),
|
||||
"port": proxy_addr.port(),
|
||||
});
|
||||
if proxy_supported && proxy_addr.ip() == std::net::IpAddr::from([127, 0, 0, 1]) {
|
||||
network["proxy"] = json!({ "localhost": proxy_addr.port() });
|
||||
}
|
||||
|
||||
let mut config = json!({
|
||||
|
||||
@@ -212,8 +212,10 @@ fn all_example_policies_split_with_lossless_invariants() {
|
||||
|
||||
assert_eq!(cfg["network"]["defaultPolicy"], "block");
|
||||
assert!(str_list(&cfg["network"]["allowedHosts"]).is_empty());
|
||||
assert_eq!(cfg["network"]["proxy"]["host"], "127.0.0.1");
|
||||
assert_eq!(cfg["network"]["proxy"]["port"], 18080);
|
||||
// MXC 0.6.0-alpha accepts only {"proxy": {"localhost": N}}.
|
||||
assert_eq!(cfg["network"]["proxy"]["localhost"], 18080);
|
||||
assert!(cfg["network"]["proxy"].get("host").is_none());
|
||||
assert!(cfg["network"]["proxy"].get("port").is_none());
|
||||
assert!(
|
||||
result.loss.iter().all(|i| i.severity != "error"),
|
||||
"processcontainer split must not emit error losses for {}: {:?}",
|
||||
@@ -277,9 +279,23 @@ fn split_policy_routes_network_to_proxy() {
|
||||
let result = split_policy(&policy, &opts).expect("split_policy returns Some when addr is set");
|
||||
let cfg = &result.mxc_config;
|
||||
|
||||
// Proxy redirect is emitted.
|
||||
assert_eq!(cfg["network"]["proxy"]["host"], "127.0.0.2");
|
||||
assert_eq!(cfg["network"]["proxy"]["port"], 8080);
|
||||
// Proxy redirect is emitted. 127.0.0.2 is not the loopback 127.0.0.1 so
|
||||
// the mapper records an error loss and omits the proxy block entirely.
|
||||
// (MXC 0.6.0-alpha can only encode {"localhost": N}; non-127.0.0.1 is
|
||||
// not representable.)
|
||||
assert!(
|
||||
cfg["network"].get("proxy").is_none() || cfg["network"]["proxy"].is_null(),
|
||||
"non-127.0.0.1 redirect must NOT produce a proxy block: {:?}",
|
||||
cfg["network"].get("proxy")
|
||||
);
|
||||
let has_proxy_loss = result
|
||||
.loss
|
||||
.iter()
|
||||
.any(|i| i.path == "network.proxy" && i.severity == "error");
|
||||
assert!(
|
||||
has_proxy_loss,
|
||||
"non-127.0.0.1 redirect must produce an error loss item"
|
||||
);
|
||||
|
||||
// Direct egress is blocked; allowedHosts is empty (proxy enforces the list).
|
||||
assert_eq!(cfg["network"]["defaultPolicy"], "block");
|
||||
@@ -390,3 +406,87 @@ fn network_only_policy_has_empty_filesystem() {
|
||||
vec!["api.anthropic.com".to_owned()]
|
||||
);
|
||||
}
|
||||
|
||||
// ── New tests: proxy JSON shape and non-127.0.0.1 guard ──────────────────────
|
||||
|
||||
#[test]
|
||||
fn split_with_loopback_addr_emits_localhost_port_shape() {
|
||||
// MXC 0.6.0-alpha accepts ONLY {"proxy": {"localhost": N}}.
|
||||
// Verified against the real wxc-exec 0.6.0-alpha binary via --dry-run.
|
||||
let path = examples_root().join("sandbox-policy-quickstart/policy.yaml");
|
||||
let yaml = std::fs::read_to_string(&path).expect("read quickstart");
|
||||
let policy = parse_sandbox_policy(&yaml).expect("parse quickstart");
|
||||
|
||||
let opts = MxcMappingOptions {
|
||||
containment: "processcontainer".to_owned(),
|
||||
proxy_redirect: Some("127.0.0.1:18080".parse().unwrap()),
|
||||
..Default::default()
|
||||
};
|
||||
let result = split_policy(&policy, &opts).expect("split returns Some");
|
||||
let cfg = &result.mxc_config;
|
||||
|
||||
assert_eq!(
|
||||
cfg["network"]["proxy"]["localhost"], 18080,
|
||||
"proxy must use {{\"localhost\": N}} shape"
|
||||
);
|
||||
assert!(
|
||||
cfg["network"]["proxy"].get("host").is_none(),
|
||||
"proxy must not contain 'host' key"
|
||||
);
|
||||
assert!(
|
||||
cfg["network"]["proxy"].get("port").is_none(),
|
||||
"proxy must not contain 'port' key"
|
||||
);
|
||||
// No error losses — 127.0.0.1 is representable.
|
||||
assert!(
|
||||
result.loss.iter().all(|i| i.severity != "error"),
|
||||
"127.0.0.1 proxy must not emit error losses: {:?}",
|
||||
result
|
||||
.loss
|
||||
.iter()
|
||||
.filter(|i| i.severity == "error")
|
||||
.collect::<Vec<_>>()
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn split_with_non_loopback_addr_emits_error_loss_and_no_proxy_block() {
|
||||
// Non-127.0.0.1 redirect addresses are not representable in MXC 0.6.0-alpha.
|
||||
// The mapper must record an error loss and omit the proxy block.
|
||||
let path = examples_root().join("sandbox-policy-quickstart/policy.yaml");
|
||||
let yaml = std::fs::read_to_string(&path).expect("read quickstart");
|
||||
let policy = parse_sandbox_policy(&yaml).expect("parse quickstart");
|
||||
|
||||
let opts = MxcMappingOptions {
|
||||
containment: "processcontainer".to_owned(),
|
||||
proxy_redirect: Some("127.0.0.5:18080".parse().unwrap()),
|
||||
..Default::default()
|
||||
};
|
||||
let result = split_policy(&policy, &opts).expect("split returns Some");
|
||||
let cfg = &result.mxc_config;
|
||||
|
||||
// Proxy block must be absent.
|
||||
assert!(
|
||||
cfg["network"].get("proxy").is_none() || cfg["network"]["proxy"].is_null(),
|
||||
"non-127.0.0.1 redirect must not produce a proxy block: {:?}",
|
||||
cfg["network"].get("proxy")
|
||||
);
|
||||
|
||||
// An error loss for "network.proxy" must be present.
|
||||
let proxy_loss = result
|
||||
.loss
|
||||
.iter()
|
||||
.find(|i| i.path == "network.proxy" && i.severity == "error");
|
||||
assert!(
|
||||
proxy_loss.is_some(),
|
||||
"non-127.0.0.1 redirect must produce an error loss item on network.proxy: {:?}",
|
||||
result.loss
|
||||
);
|
||||
let loss = proxy_loss.unwrap();
|
||||
assert_eq!(loss.openshell_feature, "per-sandbox egress attribution");
|
||||
assert!(
|
||||
loss.message.contains("localhost"),
|
||||
"loss message should mention 'localhost': {}",
|
||||
loss.message
|
||||
);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user