Repository navigation
Conversation
Add optional trace.uid enrichment through a plain sampled-context extractor, with explicit correlation taking precedence on all ten event builders. Register the extractor at gateway and supervisor startup and keep an INFO span active around network and HTTP deny emissions at the default log level. Integrate the Authentication event and current network APIs while retaining sanitization, policy decisions, credential handling and conditional DEBUG parenting. Document per-decision traces and export/retention limits. Signed-off-by: Harry Turner <harry.github@loopfeed.com>
Remove the top-level trace object and Trace profile from the existing 1.1/1.3 projection path, keeping this compatibility contract separate from native trace correlation. Test trace removal for both legacy targets and native OTLP body preservation, including source-event immutability. Document the legacy trace contract without redesigning downgrade behavior or claiming full schema conformance. Signed-off-by: Harry Turner <harry.github@loopfeed.com>
|
All contributors have signed the DCO ✍️ ✅ |
|
I have read the DCO document and I hereby sign the DCO. |
|
recheck |
Merge NVIDIA/OpenShell main at d223943. Keep both resource_details and the Trace schemas in the vendored schema README, correct its object count and preserve the refresh instructions. Retain both published correlation commits and the tested source tree 247a27f. Signed-off-by: Harry Turner <harry.github@loopfeed.com>
|
The README conflict is resolved, and the updated branch passes the local checks. Could a maintainer please vet and approve the CI workflows? Fresh Linux/Collector/Tempo validation remains pending. |
rhuss
left a comment
rhuss
left a comment
There was a problem hiding this comment.
cc-review Summary
@HarryMoss, please have your agent assess each of these comments and check it against the code before acting on it. Some may turn out not to apply, and I would rather you confirm or refute them than take them at face value.
This review comes from an agent-assisted run with several reviewers (cc-review: seven prompt reviewers plus Codex and CodeRabbit, then a skeptic/calibration pass). The findings and the scenarios in them come from the agents reading the code. Nothing was executed or reproduced, and I have not verified each scenario myself.
What Went Well
crates/openshell-otel/src/correlation.rscurrent_ocsf_trace_correlationonly correlates when the span context is both valid and sampled.crates/openshell-ocsf/src/trace.rskeepsopenshell-ocsffree of OpenTelemetry through aOnceLockextractor, andenrichnever overrides an explicit trace.crates/openshell-ocsf/src/events/base_event.rsset_traceadds thetraceprofile only once.crates/openshell-ocsf/src/format/downgrade.rsstrips both thetracefield and the profile for 1.1 and 1.3, with tests for each target.crates/openshell-supervisor-network/src/telemetry/tests.rsexportedchecks real OTLP/gRPC exports instead of only the JSON shape.
Findings
| Severity | File | Description | Source |
|---|---|---|---|
| Important | crates/openshell-supervisor-network/src/telemetry.rs:32 |
Paired deny event and finding get different trace IDs | gator |
| Minor | crates/openshell-server/src/cli.rs:669 |
Gateway trace.uid is caller-propagated but documented as trusted |
security |
| Minor | crates/openshell-supervisor-network/src/l7/middleware.rs:466 |
Capacity-exhausted finding marked deny when allowed | correctness |
| Minor | crates/openshell-supervisor-network/src/l7/middleware.rs:487 |
Load shedding traced as policy deny | goal-alignment |
| Minor | crates/openshell-supervisor-network/src/l7/middleware.rs:1004 |
denied: predicates untested |
test-quality |
| Minor | crates/openshell-supervisor-network/src/proxy.rs:1261 |
Transparent middleware deny not parented on connect span (see below) | correctness |
| Minor | crates/openshell-supervisor-network/src/proxy.rs:5677 |
Non-canonical rejection gets no deny span | codex |
| Minor | crates/openshell-supervisor-network/src/proxy.rs:5956 |
CONNECT/FORWARD deny-span wiring untested | test-quality |
| Minor | crates/openshell-supervisor-network/src/proxy.rs:6072 |
Audit-mode L7 failure traced as enforced deny | codex |
| Minor | crates/openshell-supervisor-network/src/proxy.rs:6660 |
parent: is a no-op for non-deny events |
architecture |
| Minor | crates/openshell-supervisor-network/src/proxy/trace_tests.rs:80 |
Debug deny test lacks count and uid checks | test-quality |
| Minor | crates/openshell-supervisor-network/src/telemetry.rs:11 |
Local ocsf_emit! shadows the upstream macro |
architecture |
| Minor | crates/openshell-supervisor-network/src/telemetry.rs:41 |
Deny classification split between event and flag | architecture |
| Minor | crates/openshell-supervisor-network/src/telemetry.rs:55 |
Deny-span policy/host attributes never asserted | test-quality |
| Minor | crates/openshell-supervisor-network/src/telemetry.rs:64 |
One exported root span per deny, no rate gate | production |
| Minor | crates/openshell-ocsf/src/builders/network.rs:121 |
.trace() setter on 10 builders used only by tests |
architecture |
| Minor | crates/openshell-otel/src/lib.rs:49 |
Supervisor filter moved into shared public API | architecture |
| Minor | docs/observability/ocsf-json-export.mdx:281 |
Docs mention the internal builder API | architecture |
Outside the diff: crates/openshell-supervisor-network/src/proxy.rs:1261. The transparent-TCP fail-closed middleware deny calls emit_middleware_uninspectable without the live connect_span, unlike the sibling transparent denies, so at debug level this deny is not linked to the connect/authorize trace even though the docs say it should be. An optional parent parameter, or wrapping the call in connect_span.in_scope(...), would fix it.
Observation: the supervisor.egress.deny span and its openshell.policy.* keys live in telemetry.rs, separate from the supervisor.egress.* spans in proxy/egress.rs, and the attribute values come from string-keyed evidence lookups. Shared constants would keep the two in sync.
Review Details
- Findings posted: 18
- Findings reviewed and not posted: 0
- Gate outcome: FAIL (one Important finding)
- Participating reviewers: architecture, correctness, goal-alignment, production, security, test-quality, gator, codex (CodeRabbit ran with no findings)
| pub fn emit(event: OcsfEvent, parent: Option<&tracing::Span>, denied: bool) { | ||
| let (action, endpoint, policy) = match &event { | ||
| OcsfEvent::NetworkActivity(e) => { | ||
| (e.action, e.dst_endpoint.as_ref(), e.firewall_rule.as_ref()) | ||
| } | ||
| OcsfEvent::HttpActivity(e) => (e.action, e.dst_endpoint.as_ref(), e.firewall_rule.as_ref()), | ||
| OcsfEvent::DetectionFinding(e) => (e.action, None, None), | ||
| _ => (None, None, None), | ||
| }; | ||
| if !denied && action != Some(ActionId::Denied) { | ||
| openshell_ocsf::ocsf_emit!(event); | ||
| return; | ||
| } | ||
|
|
||
| // Finding evidence supplies decision fields, not a trace parent. | ||
| let evidence = |key: &str| match &event { | ||
| OcsfEvent::DetectionFinding(e) => e.evidences.as_ref().and_then(|items| { | ||
| items | ||
| .iter() | ||
| .find_map(|item| item.data.as_ref()?.get(key)?.as_str()) | ||
| }), | ||
| _ => None, | ||
| }; | ||
| let policy = policy | ||
| .map(|rule| rule.name.as_str()) | ||
| .or_else(|| evidence("policy")); | ||
| let address = endpoint | ||
| .and_then(|endpoint| endpoint.domain.as_deref().or(endpoint.ip.as_deref())) | ||
| .or_else(|| evidence("host")); | ||
| let port = endpoint.and_then(|endpoint| endpoint.port); | ||
| let reason = event.base().status_detail.as_deref(); | ||
| let parent = parent.cloned().unwrap_or_else(tracing::Span::current); | ||
| let span = tracing::info_span!( | ||
| parent: &parent, | ||
| "supervisor.egress.deny", | ||
| openshell.policy.decision = "deny", | ||
| openshell.policy.name = tracing::field::Empty, | ||
| openshell.policy.reason = tracing::field::Empty, | ||
| server.address = tracing::field::Empty, | ||
| server.port = tracing::field::Empty, | ||
| ocsf.class_uid = event.class_uid(), | ||
| ); | ||
| if let Some(policy) = policy { | ||
| span.record("openshell.policy.name", policy); | ||
| } | ||
| if let Some(reason) = reason { | ||
| span.record("openshell.policy.reason", reason); | ||
| } | ||
| if let Some(address) = address { | ||
| span.record("server.address", address); | ||
| } | ||
| if let Some(port) = port { | ||
| span.record("server.port", i64::from(port)); | ||
| } | ||
| span.in_scope(|| openshell_ocsf::ocsf_emit!(event)); | ||
| } |
There was a problem hiding this comment.
Important: emit opens a new supervisor.egress.deny span for every OCSF event it handles. A denial is dual-emitted (domain event plus DetectionFinding), so the two records of one incident end up with different trace.uid values in separate root traces.
Why this matters: At the default level there is no active parent, so each emission starts its own trace. Affected sites include l7/rest.rs:1537 and :1551, proxy.rs:2948 together with l7/mod.rs:447 (the finding has no parent even at debug level), l7/relay.rs:290-291, l7/websocket.rs:1388-1389, the loop at l7/middleware.rs:1004 and the finding loops in l7/rest/http_response.rs. Pivoting from the finding's trace.uid then lands on a one-span trace that has nothing to do with the network event's trace, so the correlation is useless there. It also doubles span volume per denial.
Suggested fix: Open one deny span per decision and emit all events of that decision inside it, for example a telemetry::deny_scope(parent, &primary_event, || { ... }) helper, and let emit reuse an already active deny span instead of nesting a new one. A test that emits a network deny plus its finding and asserts one exported deny span and equal trace.uid values would keep this from regressing.
Source: gator agent (also flagged by: production agent, correctness agent)
| openshell_ocsf::register_trace_correlation_extractor( | ||
| openshell_otel::current_ocsf_trace_correlation, | ||
| ); |
There was a problem hiding this comment.
Minor: Registering current_ocsf_trace_correlation in the gateway stamps gateway audit events with the trace ID of the request span, and that span is parented on the caller's inbound traceparent (multiplex.rs:129-136) before authentication runs. On the gateway, trace.uid is therefore chosen by the caller, including on Authentication failure events.
Why this matters: Honoring inbound traceparent on the gateway is intentional (CI trace continuity from the CLI), so this is not a request to drop propagation. The problem is the contract: TraceCorrelation and correlation.rs describe the value as trusted, and #2640 says caller-controlled headers never go into trace. An unauthenticated client could then place its failed logons in another trace, or send flags=00 to keep them out of correlation. This scenario is derived from code reading and was not reproduced.
Suggested fix: Document that gateway trace.uid is caller-propagated (a correlation hint, not proof) and drop the "trusted" wording in trace.rs, base_event.rs and correlation.rs. Optionally, leave correlation off for events emitted before authentication succeeds.
Source: security agent (also flagged by: gator agent, goal-alignment agent)
| .message("Persistent middleware session admission was refused at process capacity") | ||
| .build(); | ||
| ocsf_emit!(event); | ||
| ocsf_emit!(denied: true, event); |
There was a problem hiding this comment.
Minor: emit_middleware_session_capacity_exhausted always passes denied: true, but session_capacity_exhausted() returns allowed: !fail_closed. With an all-FailOpen chain the WebSocket upgrade is allowed and still gets a deny span and deny correlation.
Why this matters: Telemetry then reports a denial for traffic that went through. Enforcement is unaffected.
Suggested fix: Pass the decision in, e.g. emit_middleware_session_capacity_exhausted(ctx, !outcome.allowed) with ocsf_emit!(denied: denied, event).
Source: correctness agent
|
|
||
| fn emit_middleware_admission_exhausted(ctx: &L7EvalContext) { | ||
| ocsf_emit!(middleware_admission_exhausted_event(ctx)); | ||
| ocsf_emit!(denied: true, middleware_admission_exhausted_event(ctx)); |
There was a problem hiding this comment.
Minor: Load-shedding findings (admission_exhausted here, session_capacity_exhausted above) open INFO deny spans with openshell.policy.decision=deny, although no policy made that decision.
Why this matters: The request is refused, so a deny label is defensible. But it adds one exported span per shed request right when the process is saturated, and the "policy" attribute is misleading.
Suggested fix: Either drop denied: true for capacity refusals or document that they are deliberately traced as denies.
Source: goal-alignment agent
| ) { | ||
| for event in middleware_events(ctx, req, outcome) { | ||
| ocsf_emit!(event); | ||
| ocsf_emit!(denied: !outcome.allowed && matches!(event, openshell_ocsf::OcsfEvent::DetectionFinding(_)), event); |
There was a problem hiding this comment.
Minor: The new denied: predicates for the middleware chain and WebSocket finding emissions (here and at lines 182, 267, 410, 430) have no tests for either branch.
Why this matters: Inverting any of these predicates keeps the suite green.
Suggested fix: Add a table-driven exported("warn", ...) test over emit_middleware_events and emit_websocket_preflight_events with an allowed and a denied outcome, asserting which events carry trace.uid and how many deny spans are exported.
Source: test-quality agent
| }), | ||
| _ => None, | ||
| }; | ||
| let policy = policy |
There was a problem hiding this comment.
Minor: No test asserts the values of openshell.policy.name and server.address on the deny span, including the fallback to evidence("policy")/evidence("host").
Why this matters: assert_deny_resolves only checks that the decision and class attributes exist, so replacing the policy expression with None keeps the suite green.
Suggested fix: In the middleware and relay trace tests, assert openshell.policy.name == "middleware-policy" / "rest-policy" and server.address == "blocked.example".
Source: test-quality agent
| let port = endpoint.and_then(|endpoint| endpoint.port); | ||
| let reason = event.base().status_detail.as_deref(); | ||
| let parent = parent.cloned().unwrap_or_else(tracing::Span::current); | ||
| let span = tracing::info_span!( |
There was a problem hiding this comment.
Minor: Each denied event exports its own INFO root span at the default level, without rate limiting or aggregation.
Why this matters: This matches the stated design and scales like the audit records themselves, but a workload retrying a blocked host in a loop produces one trace per attempt and can fill the batch exporter queue.
Suggested fix: Consider a per-destination rate gate before creating the span, or link the existing sampler settings (OTEL_TRACES_SAMPLER) from the deny-trace section of the docs.
Source: production agent
| impl<'a, EndpointState> NetworkActivityBuilder<'a, EndpointState> { | ||
| /// Set trace correlation explicitly instead of using the active span. | ||
| #[must_use] | ||
| pub fn trace(mut self, trace: crate::TraceCorrelation) -> Self { |
There was a problem hiding this comment.
Minor: All ten builders gain a public .trace(TraceCorrelation) setter with typestate plumbing, but only tests call it; production correlation goes through the registered extractor.
Why this matters: That is about 130 near-identical lines of public API, and TraceCorrelation.uid is a public String whose 32-hex invariant is only documented. The PR description lists the setters as intended, so treat this as a design question.
Suggested fix: Either let tests use BaseEventData::set_trace and drop the setters, or keep one validating constructor with a private field.
Source: architecture agent
| /// Export supervisor spans at INFO by default, or DEBUG and TRACE when enabled. | ||
| /// Dependency spans remain at INFO. | ||
| #[must_use] | ||
| pub fn supervisor_span_filter(log_level: &str) -> tracing_subscriber::EnvFilter { |
There was a problem hiding this comment.
Minor: supervisor_span_filter is now public API of the shared openshell-otel crate, while openshell-supervisor/src/main.rs keeps an otlp_span_filter wrapper that only forwards to it. The PR description does not mention the new API.
Why this matters: A crate shared by the gateway and drivers now exports one binary's span policy, and the wrapper duplicates doc and test.
Suggested fix: Inline the call in main.rs and drop the wrapper, or keep the filter in the supervisor and give the tests a generic helper.
Source: architecture agent (also flagged by: goal-alignment agent)
| with 32 lowercase hexadecimal characters. Search for it in your configured | ||
| Jaeger or Tempo backend to inspect the supervisor's handling of the deny. | ||
| Without an active sampled context, automatic correlation adds neither the | ||
| trace object nor its profile, including when OTLP export is disabled. Explicit |
There was a problem hiding this comment.
Minor: "Explicit builder correlation takes precedence" describes an internal Rust builder API that operators cannot use.
Why this matters: The docs guidelines ask to leave out internals that do not affect users.
Suggested fix: Delete the sentence or move it to the openshell-ocsf README.
Source: architecture agent
This branch has not been deployed
2 participants
Summary
OCSF events emitted inside a valid, sampled OpenTelemetry span include
trace.uid, so an operator can follow an audit record to its corresponding trace. Network and HTTP deny emissions create an INFO span at the normal supervisor log level. Correlation is per deny decision; one connection attempt can produce multiple traces.Related Issue
Closes #2640
Changes
TraceCorrelationvalue, explicit setters on all ten event builders, and automatic correlation for ordinary and routed emissions. Explicit correlation takes precedence.openshell-ocsfhas no OpenTelemetry dependency.traceparentheaders are not adopted. Allowed traffic stays at DEBUG.trace.uid; absence of a valid sampled context leaves automatic correlation absent.Integration status
The two original signed-off commits remain in this branch's history: native correlation at
dde47f447c0658007fa686859214de4524b9baf7and isolated legacy trace removal atfdbcfa89bfe66a05ce6fdb00b2a6c9f1de7efc18.Merged upstream main
d22394370d2d8b27859ffa772cac3aaaa79feb2cin signed-off commit04fb2373d5c43667a9aaa224c8eeb0eb7a2da8c9, preserving both published commits. The resulting source tree is247a27fca5a2cabe26399f15ebb132ce39193421. The README conflict is resolved: it retains main'sresource_detailsschema and all Trace objects, lists 26 objects and includes every object in the schema refresh command. All other files match Git's automatic merge. No additional Rust implementation fix was needed.Testing
Fresh local validation on merged tree
247a27fca5a2cabe26399f15ebb132ce39193421used macOS arm64, Rust 1.95.0, default package features and repository profiles, the existing build cache, two jobs, incremental compilation disabled and locked dependencies.-D warningsfor OCSF, OTel, supervisor-network, supervisor and server.The previous October 20 tests on tree
bdd07ed1f5be2d69af12122e387a9a4e209cf0acremain historical evidence. They are not added to these fresh merged-tree totals. The 21 ignored gateway tests require PostgreSQL/disposable databases or are database benchmarks; the other ignored tests are the two network tests and two OCSF doc examples declared in the repository.Independent review of the original revision
Both independent reviews cover
3fae097a5d4355eeff77746f3ea90e89228455e3, rather than the adapted integration tree.@kvnloo reviewed that commit on Linux x86_64 with Rust 1.95.0 and reported no blocking finding within his review boundary: 220 OCSF/OTel tests passed, two doc examples were ignored, and 59 targeted supervisor-network deny tests passed, including eight new correlation checks. His DEBUG regression covers staged L4 parenting. Review and evidence.
@rhuss built that revision's gateway and supervisor and checked Podman with an OTel Collector and Tempo, JSON enabled and the default supervisor log level. DNS refusal, transparent TCP denial, HTTP POST denial and HTTPS POST denial each produced an audit trace ID resolving to the corresponding supervisor deny span with sandbox identity. Allowed controls had no trace; the 1.3 projection omitted trace/profile. Deployed check.
These results remain attributed to their tested revision. They are not formal approval of the adapted tree or evidence of its fresh deployment.
Related 4288 integration
#4288 was rechecked on 21 October 2026, open and unmerged at
a79eaabb56c3e0f4ad665d86524c92f625eaf495. Its current scope is native OCSF 1.8 conformance and validation. Its earlier schema-driven downgrade implementation andDowngradeOutcomeassumptions are superseded. Broader downgrade removal in #4284 remains outside this PR.The existing compatibility experiment already uses this exact #4288 revision, its unchanged production Rust functions, the Trace schemas on all ten classes and a regenerated embedded catalog. Its 196 library tests passed, including one test with 20 native cases: ten builders with and without trace. The overlay snapshot is
c7b3373d6f0e9d0239de8c277b3acd796158913e; it is a schema experiment rather than a compiled full rebase of this implementation. If #4288 lands first, retain its native conformance fixes, account for its builder APIs, retain Trace coverage and regenerate the embedded catalog. Do not switch toDowngradeOutcomeon that basis.Kevin's separate older experiment remains pinned to
78621e8bb1fb9af4c91f9cf8dd5c3ebffc62b963: three isolated checks and one 54-case schema-overlay matrix. Its overlapping counts are not added to the current experiment or implementation totals. Pinned evidence.Coverage limits and pending validation
Acknowledgements
These acknowledgements do not add co-author, sign-off or approval claims.
Checklist