Skip to content

Commit 29e8159

Browse files
committed
fix(sandbox): add JSON bodies to proxy 403/502 responses and include port in HTTP log URLs
L4 CONNECT and forward-proxy denial responses (403) and upstream failure responses (502) were returned with empty bodies, making them indistinguishable from each other when using curl -s. Add a build_json_error_response() helper that produces consistent JSON bodies with error and detail fields, matching the format already used by the L7 deny path. Also fix Url::to_display_string() in openshell-ocsf to include non-default ports in the shorthand log output. Requests to e.g. http://host:9876/path were logged as http://host/path, dropping the port number. Closes #807, closes #808
1 parent dafb799 commit 29e8159

File tree

3 files changed

+267
-13
lines changed

3 files changed

+267
-13
lines changed

crates/openshell-ocsf/src/format/shorthand.rs

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -447,6 +447,35 @@ mod tests {
447447
);
448448
}
449449

450+
#[test]
451+
fn test_http_activity_shorthand_non_default_port() {
452+
let event = OcsfEvent::HttpActivity(HttpActivityEvent {
453+
base: base(4002, "HTTP Activity", 4, "Network Activity", 3, "Get"),
454+
http_request: Some(HttpRequest::new(
455+
"GET",
456+
Url::new("http", "172.20.0.1", "/test", 9876),
457+
)),
458+
http_response: None,
459+
src_endpoint: None,
460+
dst_endpoint: None,
461+
proxy_endpoint: None,
462+
actor: Some(Actor {
463+
process: Process::new("curl", 68),
464+
}),
465+
firewall_rule: Some(FirewallRule::new("allow_host_9876", "mechanistic")),
466+
action: Some(ActionId::Allowed),
467+
disposition: None,
468+
observation_point_id: None,
469+
is_src_dst_assignment_known: None,
470+
});
471+
472+
let shorthand = event.format_shorthand();
473+
assert_eq!(
474+
shorthand,
475+
"HTTP:GET [INFO] ALLOWED curl(68) -> GET http://172.20.0.1:9876/test [policy:allow_host_9876]"
476+
);
477+
}
478+
450479
#[test]
451480
fn test_ssh_activity_shorthand() {
452481
let event = OcsfEvent::SshActivity(SshActivityEvent {

crates/openshell-ocsf/src/objects/http.rs

Lines changed: 41 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -58,12 +58,21 @@ impl Url {
5858
}
5959

6060
/// Format as a display string.
61+
///
62+
/// Includes the port when it is present and differs from the scheme default
63+
/// (443 for `https`, 80 for `http`).
6164
#[must_use]
6265
pub fn to_display_string(&self) -> String {
6366
let scheme = self.scheme.as_deref().unwrap_or("https");
6467
let hostname = self.hostname.as_deref().unwrap_or("unknown");
6568
let path = self.path.as_deref().unwrap_or("/");
66-
format!("{scheme}://{hostname}{path}")
69+
let port_suffix = match self.port {
70+
Some(443) if scheme == "https" => String::new(),
71+
Some(80) if scheme == "http" => String::new(),
72+
Some(p) => format!(":{p}"),
73+
None => String::new(),
74+
};
75+
format!("{scheme}://{hostname}{port_suffix}{path}")
6776
}
6877
}
6978

@@ -94,9 +103,39 @@ mod tests {
94103
}
95104

96105
#[test]
97-
fn test_url_display_string() {
106+
fn test_url_display_string_default_port() {
98107
let url = Url::new("https", "api.example.com", "/v1/data", 443);
99108
assert_eq!(url.to_display_string(), "https://api.example.com/v1/data");
109+
110+
let url = Url::new("http", "example.com", "/index", 80);
111+
assert_eq!(url.to_display_string(), "http://example.com/index");
112+
}
113+
114+
#[test]
115+
fn test_url_display_string_non_default_port() {
116+
let url = Url::new("http", "172.20.0.1", "/test", 9876);
117+
assert_eq!(url.to_display_string(), "http://172.20.0.1:9876/test");
118+
119+
let url = Url::new("https", "api.example.com", "/v1/data", 8443);
120+
assert_eq!(
121+
url.to_display_string(),
122+
"https://api.example.com:8443/v1/data"
123+
);
124+
125+
// HTTP on 443 is non-default — should show port
126+
let url = Url::new("http", "example.com", "/path", 443);
127+
assert_eq!(url.to_display_string(), "http://example.com:443/path");
128+
}
129+
130+
#[test]
131+
fn test_url_display_string_no_port() {
132+
let url = Url {
133+
scheme: Some("https".to_string()),
134+
hostname: Some("example.com".to_string()),
135+
path: Some("/path".to_string()),
136+
port: None,
137+
};
138+
assert_eq!(url.to_display_string(), "https://example.com/path");
100139
}
101140

102141
#[test]

crates/openshell-sandbox/src/proxy.rs

Lines changed: 197 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -463,7 +463,16 @@ async fn handle_tcp_connection(
463463
&deny_reason,
464464
"connect",
465465
);
466-
respond(&mut client, b"HTTP/1.1 403 Forbidden\r\n\r\n").await?;
466+
respond(
467+
&mut client,
468+
&build_json_error_response(
469+
403,
470+
"Forbidden",
471+
"policy_denied",
472+
&format!("CONNECT {host_lc}:{port} not permitted by policy"),
473+
),
474+
)
475+
.await?;
467476
return Ok(());
468477
}
469478

@@ -518,7 +527,16 @@ async fn handle_tcp_connection(
518527
&reason,
519528
"ssrf",
520529
);
521-
respond(&mut client, b"HTTP/1.1 403 Forbidden\r\n\r\n").await?;
530+
respond(
531+
&mut client,
532+
&build_json_error_response(
533+
403,
534+
"Forbidden",
535+
"ssrf_denied",
536+
&format!("CONNECT {host_lc}:{port} blocked: allowed_ips check failed"),
537+
),
538+
)
539+
.await?;
522540
return Ok(());
523541
}
524542
},
@@ -553,7 +571,16 @@ async fn handle_tcp_connection(
553571
&reason,
554572
"ssrf",
555573
);
556-
respond(&mut client, b"HTTP/1.1 403 Forbidden\r\n\r\n").await?;
574+
respond(
575+
&mut client,
576+
&build_json_error_response(
577+
403,
578+
"Forbidden",
579+
"ssrf_denied",
580+
&format!("CONNECT {host_lc}:{port} blocked: invalid allowed_ips in policy"),
581+
),
582+
)
583+
.await?;
557584
return Ok(());
558585
}
559586
}
@@ -594,7 +621,16 @@ async fn handle_tcp_connection(
594621
&reason,
595622
"ssrf",
596623
);
597-
respond(&mut client, b"HTTP/1.1 403 Forbidden\r\n\r\n").await?;
624+
respond(
625+
&mut client,
626+
&build_json_error_response(
627+
403,
628+
"Forbidden",
629+
"ssrf_denied",
630+
&format!("CONNECT {host_lc}:{port} blocked: internal address"),
631+
),
632+
)
633+
.await?;
598634
return Ok(());
599635
}
600636
}
@@ -2071,7 +2107,16 @@ async fn handle_forward_proxy(
20712107
reason,
20722108
"forward",
20732109
);
2074-
respond(client, b"HTTP/1.1 403 Forbidden\r\n\r\n").await?;
2110+
respond(
2111+
client,
2112+
&build_json_error_response(
2113+
403,
2114+
"Forbidden",
2115+
"policy_denied",
2116+
&format!("{method} {host_lc}:{port}{path} not permitted by policy"),
2117+
),
2118+
)
2119+
.await?;
20752120
return Ok(());
20762121
}
20772122
};
@@ -2199,7 +2244,16 @@ async fn handle_forward_proxy(
21992244
&reason,
22002245
"forward-l7-deny",
22012246
);
2202-
respond(client, b"HTTP/1.1 403 Forbidden\r\n\r\n").await?;
2247+
respond(
2248+
client,
2249+
&build_json_error_response(
2250+
403,
2251+
"Forbidden",
2252+
"policy_denied",
2253+
&format!("{method} {host_lc}:{port}{path} denied by L7 policy"),
2254+
),
2255+
)
2256+
.await?;
22032257
return Ok(());
22042258
}
22052259
}
@@ -2254,7 +2308,16 @@ async fn handle_forward_proxy(
22542308
&reason,
22552309
"ssrf",
22562310
);
2257-
respond(client, b"HTTP/1.1 403 Forbidden\r\n\r\n").await?;
2311+
respond(
2312+
client,
2313+
&build_json_error_response(
2314+
403,
2315+
"Forbidden",
2316+
"ssrf_denied",
2317+
&format!("{method} {host_lc}:{port} blocked: allowed_ips check failed"),
2318+
),
2319+
)
2320+
.await?;
22582321
return Ok(());
22592322
}
22602323
},
@@ -2292,7 +2355,18 @@ async fn handle_forward_proxy(
22922355
&reason,
22932356
"ssrf",
22942357
);
2295-
respond(client, b"HTTP/1.1 403 Forbidden\r\n\r\n").await?;
2358+
respond(
2359+
client,
2360+
&build_json_error_response(
2361+
403,
2362+
"Forbidden",
2363+
"ssrf_denied",
2364+
&format!(
2365+
"{method} {host_lc}:{port} blocked: invalid allowed_ips in policy"
2366+
),
2367+
),
2368+
)
2369+
.await?;
22962370
return Ok(());
22972371
}
22982372
}
@@ -2334,7 +2408,16 @@ async fn handle_forward_proxy(
23342408
&reason,
23352409
"ssrf",
23362410
);
2337-
respond(client, b"HTTP/1.1 403 Forbidden\r\n\r\n").await?;
2411+
respond(
2412+
client,
2413+
&build_json_error_response(
2414+
403,
2415+
"Forbidden",
2416+
"ssrf_denied",
2417+
&format!("{method} {host_lc}:{port} blocked: internal address"),
2418+
),
2419+
)
2420+
.await?;
23382421
return Ok(());
23392422
}
23402423
}
@@ -2363,7 +2446,16 @@ async fn handle_forward_proxy(
23632446
))
23642447
.build();
23652448
ocsf_emit!(event);
2366-
respond(client, b"HTTP/1.1 502 Bad Gateway\r\n\r\n").await?;
2449+
respond(
2450+
client,
2451+
&build_json_error_response(
2452+
502,
2453+
"Bad Gateway",
2454+
"upstream_unreachable",
2455+
&format!("connection to {host_lc}:{port} failed"),
2456+
),
2457+
)
2458+
.await?;
23672459
return Ok(());
23682460
}
23692461
};
@@ -2402,7 +2494,16 @@ async fn handle_forward_proxy(
24022494
error = %e,
24032495
"credential injection failed in forward proxy"
24042496
);
2405-
respond(client, b"HTTP/1.1 500 Internal Server Error\r\n\r\n").await?;
2497+
respond(
2498+
client,
2499+
&build_json_error_response(
2500+
500,
2501+
"Internal Server Error",
2502+
"credential_injection_failed",
2503+
"unresolved credential placeholder in request",
2504+
),
2505+
)
2506+
.await?;
24062507
return Ok(());
24072508
}
24082509
};
@@ -2431,6 +2532,30 @@ async fn respond(client: &mut TcpStream, bytes: &[u8]) -> Result<()> {
24312532
Ok(())
24322533
}
24332534

2535+
/// Build an HTTP error response with a JSON body.
2536+
///
2537+
/// Returns bytes ready to write to the client socket. The body is a JSON
2538+
/// object with `error` and `detail` fields, matching the format used by the
2539+
/// L7 deny path in `l7/rest.rs`.
2540+
fn build_json_error_response(status: u16, status_text: &str, error: &str, detail: &str) -> Vec<u8> {
2541+
let body = serde_json::json!({
2542+
"error": error,
2543+
"detail": detail,
2544+
});
2545+
let body_str = body.to_string();
2546+
format!(
2547+
"HTTP/1.1 {status} {status_text}\r\n\
2548+
Content-Type: application/json\r\n\
2549+
Content-Length: {}\r\n\
2550+
Connection: close\r\n\
2551+
\r\n\
2552+
{}",
2553+
body_str.len(),
2554+
body_str,
2555+
)
2556+
.into_bytes()
2557+
}
2558+
24342559
/// Check if a miette error represents a benign connection close.
24352560
///
24362561
/// TLS handshake EOF, missing `close_notify`, connection resets, and broken
@@ -3292,4 +3417,65 @@ mod tests {
32923417
let result = implicit_allowed_ips_for_ip_host("*.example.com");
32933418
assert!(result.is_empty());
32943419
}
3420+
3421+
// -- build_json_error_response --
3422+
3423+
#[test]
3424+
fn test_json_error_response_403() {
3425+
let resp = build_json_error_response(
3426+
403,
3427+
"Forbidden",
3428+
"policy_denied",
3429+
"CONNECT api.example.com:443 not permitted by policy",
3430+
);
3431+
let resp_str = String::from_utf8(resp).unwrap();
3432+
3433+
assert!(resp_str.starts_with("HTTP/1.1 403 Forbidden\r\n"));
3434+
assert!(resp_str.contains("Content-Type: application/json\r\n"));
3435+
assert!(resp_str.contains("Connection: close\r\n"));
3436+
3437+
// Extract body after \r\n\r\n
3438+
let body_start = resp_str.find("\r\n\r\n").unwrap() + 4;
3439+
let body: serde_json::Value = serde_json::from_str(&resp_str[body_start..]).unwrap();
3440+
assert_eq!(body["error"], "policy_denied");
3441+
assert_eq!(
3442+
body["detail"],
3443+
"CONNECT api.example.com:443 not permitted by policy"
3444+
);
3445+
}
3446+
3447+
#[test]
3448+
fn test_json_error_response_502() {
3449+
let resp = build_json_error_response(
3450+
502,
3451+
"Bad Gateway",
3452+
"upstream_unreachable",
3453+
"connection to api.example.com:443 failed",
3454+
);
3455+
let resp_str = String::from_utf8(resp).unwrap();
3456+
3457+
assert!(resp_str.starts_with("HTTP/1.1 502 Bad Gateway\r\n"));
3458+
3459+
let body_start = resp_str.find("\r\n\r\n").unwrap() + 4;
3460+
let body: serde_json::Value = serde_json::from_str(&resp_str[body_start..]).unwrap();
3461+
assert_eq!(body["error"], "upstream_unreachable");
3462+
assert_eq!(body["detail"], "connection to api.example.com:443 failed");
3463+
}
3464+
3465+
#[test]
3466+
fn test_json_error_response_content_length_matches() {
3467+
let resp = build_json_error_response(403, "Forbidden", "test", "detail");
3468+
let resp_str = String::from_utf8(resp).unwrap();
3469+
3470+
// Extract Content-Length value
3471+
let cl_line = resp_str
3472+
.lines()
3473+
.find(|l| l.starts_with("Content-Length:"))
3474+
.unwrap();
3475+
let cl: usize = cl_line.split(": ").nth(1).unwrap().trim().parse().unwrap();
3476+
3477+
// Verify body length matches
3478+
let body_start = resp_str.find("\r\n\r\n").unwrap() + 4;
3479+
assert_eq!(resp_str[body_start..].len(), cl);
3480+
}
32953481
}

0 commit comments

Comments
 (0)