Skip to content

Commit 2bb7e01

Browse files
committed
fix(supervisor-network): keep FORWARD denials in the connect trace
At debug level, FORWARD credential-binding and request middleware denials opened root deny spans, because the saved connect span exists there but is not entered. Deny spans also charged the per-destination limiter even when they were disabled, and a fail-closed WebSocket middleware finding did not record the denial its HTTP counterparts do. Those denials now pass or run under the saved connect span, the limiter is consulted only when INFO spans are enabled, and the fail-closed WebSocket finding sets action Denied. Signed-off-by: Kris Hicks <khicks@nvidia.com>
1 parent 8626d51 commit 2bb7e01

5 files changed

Lines changed: 102 additions & 21 deletions

File tree

‎crates/openshell-supervisor-network/src/l7/middleware.rs‎

Lines changed: 41 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -388,8 +388,13 @@ fn emit_websocket_invocations(
388388
.build();
389389
egress_emit!(event);
390390
if invocation.failed {
391-
let event = DetectionFindingBuilder::new(openshell_ocsf::ctx::ctx())
392-
.severity(if invocation.outcome == Outcome::FailClosed {
391+
let fail_closed = invocation.outcome == Outcome::FailClosed;
392+
let mut finding = DetectionFindingBuilder::new(openshell_ocsf::ctx::ctx());
393+
if fail_closed {
394+
finding = finding.action(ActionId::Denied);
395+
}
396+
let event = finding
397+
.severity(if fail_closed {
393398
SeverityId::High
394399
} else {
395400
SeverityId::Medium
@@ -1052,6 +1057,40 @@ mod trace_tests {
10521057
}
10531058
}
10541059

1060+
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
1061+
async fn websocket_fail_closed_findings_record_the_denial() {
1062+
use openshell_supervisor_middleware::{
1063+
WebSocketInvocation, WebSocketInvocationOutcome as Outcome,
1064+
};
1065+
for (outcome, action_id) in [(Outcome::FailClosed, Some(2)), (Outcome::FailOpen, None)] {
1066+
let (events, _) = exported("warn", || {
1067+
emit_websocket_invocations(
1068+
&L7EvalContext::default(),
1069+
&[WebSocketInvocation {
1070+
config_name: "guard".to_string(),
1071+
implementation: "test/guard".to_string(),
1072+
outcome,
1073+
sequence: None,
1074+
original_size: 0,
1075+
replacement_size: None,
1076+
transformed: false,
1077+
failed: true,
1078+
stage_disabled: false,
1079+
reason_code: None,
1080+
}],
1081+
);
1082+
})
1083+
.await;
1084+
let finding = events
1085+
.iter()
1086+
.find(|event| {
1087+
event["finding_info"]["uid"] == "openshell.middleware.websocket_failure"
1088+
})
1089+
.unwrap();
1090+
assert_eq!(finding["action_id"].as_u64(), action_id, "{outcome:?}");
1091+
}
1092+
}
1093+
10551094
#[test]
10561095
fn refusal_findings_record_the_denial() {
10571096
for reason in [

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

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -247,15 +247,21 @@ fn build_credential_endpoint_mismatch_event(
247247
.build()
248248
}
249249

250-
fn emit_credential_endpoint_mismatch(method: &str, host: &str, port: u16, policy_name: &str) {
250+
fn emit_credential_endpoint_mismatch(
251+
method: &str,
252+
host: &str,
253+
port: u16,
254+
policy_name: &str,
255+
connect_span: &tracing::Span,
256+
) {
251257
let event = build_credential_endpoint_mismatch_event(method, host, port, policy_name);
252258
let finding = crate::l7::build_credential_endpoint_mismatch_finding(
253259
policy_name,
254260
host,
255261
None,
256262
"Provider credential endpoint binding mismatch; request denied",
257263
);
258-
crate::telemetry::emit_decision([event, finding], None);
264+
crate::telemetry::emit_decision([event, finding], Some(connect_span));
259265
}
260266

261267
/// Hostnames injected by compute drivers as `/etc/hosts` aliases for the host
@@ -6490,7 +6496,12 @@ async fn handle_forward_proxy(
64906496
exchange: &middleware_exchange,
64916497
l7_reevaluation,
64926498
};
6493-
forward_request_bytes = match pipeline.apply(request, client).await? {
6499+
// Middleware denials join the connect trace, like other FORWARD denials.
6500+
forward_request_bytes = match pipeline
6501+
.apply(request, client)
6502+
.instrument(connect_span.clone())
6503+
.await?
6504+
{
64946505
crate::l7::middleware::MiddlewareApplyResult::Allowed(request) => request.raw_header,
64956506
crate::l7::middleware::MiddlewareApplyResult::Denied { denial, .. } => {
64966507
emit_activity_simple(activity_tx, true, "middleware");
@@ -6671,13 +6682,21 @@ async fn handle_forward_proxy(
66716682
.await;
66726683
}
66736684
if e.is_placement_mismatch() {
6674-
crate::l7::relay::reject_credential_resolution(client, &l7_ctx, method, &e).await?;
6685+
crate::l7::relay::reject_credential_resolution(client, &l7_ctx, method, &e)
6686+
.instrument(connect_span.clone())
6687+
.await?;
66756688
client.shutdown().await.into_diagnostic()?;
66766689
} else if e.is_endpoint_mismatch() {
66776690
if let Some(observer) = endpoint_observer.as_ref() {
66786691
observer.observe_credential_failure(true);
66796692
}
6680-
emit_credential_endpoint_mismatch(method, &host_lc, port, policy_str);
6693+
emit_credential_endpoint_mismatch(
6694+
method,
6695+
&host_lc,
6696+
port,
6697+
policy_str,
6698+
&connect_span,
6699+
);
66816700
respond(
66826701
client,
66836702
&build_json_error_response(

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

Lines changed: 17 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,13 @@ use opentelemetry_proto::tonic::common::v1::any_value::Value;
1010
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
1111
async fn paired_credential_denial_shares_one_deny_span() {
1212
let (events, received) = exported("warn", || {
13-
emit_credential_endpoint_mismatch("POST", "blocked.example", 443, "credential-policy");
13+
emit_credential_endpoint_mismatch(
14+
"POST",
15+
"blocked.example",
16+
443,
17+
"credential-policy",
18+
&tracing::Span::none(),
19+
);
1420
})
1521
.await;
1622
assert_eq!(events.len(), 2);
@@ -24,16 +30,16 @@ async fn repeated_credential_denials_keep_decision_spans_separate() {
2430
let (events, received) = exported(level, || {
2531
let connect =
2632
egress::connect_span(&EgressIntent::connect("blocked.example".to_string(), 443));
27-
connect.in_scope(|| {
28-
for _ in 0..2 {
29-
emit_credential_endpoint_mismatch(
30-
"POST",
31-
"blocked.example",
32-
443,
33-
"credential-policy",
34-
);
35-
}
36-
});
33+
// The FORWARD path passes its saved connect span without entering it.
34+
for _ in 0..2 {
35+
emit_credential_endpoint_mismatch(
36+
"POST",
37+
"blocked.example",
38+
443,
39+
"credential-policy",
40+
&connect,
41+
);
42+
}
3743
let allowed = NetworkActivityBuilder::new(openshell_ocsf::ctx::ctx())
3844
.action(ActionId::Allowed)
3945
.dst_endpoint(Endpoint::from_domain("allowed.example", 443))

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

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -35,8 +35,9 @@ fn as_http(event: &OcsfEvent) -> Option<&HttpActivityEvent> {
3535

3636
/// Emit all records of one synchronous decision under one deny span. Only HTTP
3737
/// Activity carries `trace`, so only a denied decision with an HTTP record gets
38-
/// a span; it is anchored on the denied HTTP record when there is one. A saved
39-
/// `parent` keeps the span in the connect trace after authorization exits.
38+
/// a span. The span describes the denied HTTP record, or the first HTTP record
39+
/// when only a companion finding is denied. A saved `parent` keeps the span in
40+
/// the connect trace after authorization exits.
4041
#[allow(clippy::disallowed_macros)]
4142
pub fn emit_decision(events: impl IntoIterator<Item = OcsfEvent>, parent: Option<&tracing::Span>) {
4243
let events: Vec<_> = events.into_iter().collect();
@@ -62,8 +63,12 @@ pub fn emit_decision(events: impl IntoIterator<Item = OcsfEvent>, parent: Option
6263
}
6364
}
6465

65-
/// Returns `None` when the destination has used up its deny spans.
66+
/// Returns `None` when deny spans are disabled or the destination has used up
67+
/// its deny spans. Disabled spans spend no tokens.
6668
fn deny_span(event: &HttpActivityEvent, parent: Option<&tracing::Span>) -> Option<tracing::Span> {
69+
if !tracing::enabled!(kind: tracing::metadata::Kind::SPAN, tracing::Level::INFO) {
70+
return None;
71+
}
6772
let endpoint = event.dst_endpoint.as_ref();
6873
let address =
6974
endpoint.and_then(|endpoint| endpoint.domain.as_deref().or(endpoint.ip.as_deref()));

‎crates/openshell-supervisor-network/src/telemetry/tests.rs‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -478,3 +478,15 @@ async fn repeated_denials_to_one_destination_export_bounded_spans() {
478478
assert_eq!(traced, denies, "limited records carry no trace.uid");
479479
assert_eq!(events.len(), super::DENY_SPAN_BURST as usize + 6);
480480
}
481+
482+
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
483+
async fn disabled_deny_spans_spend_no_tokens() {
484+
let _lock = tracing_test_lock().await;
485+
super::reset_deny_span_limiter();
486+
let subscriber =
487+
tracing_subscriber::registry().with(tracing_subscriber::filter::LevelFilter::WARN);
488+
tracing::subscriber::with_default(subscriber, || {
489+
super::egress_emit!(http_deny("quiet.example", "denied"));
490+
});
491+
assert!(super::LIMITER.lock().unwrap().buckets.is_empty());
492+
}

0 commit comments

Comments
 (0)