Repository navigation
feat(observability): correlate HTTP enforcement with traces and OCSF - #5214
afourniernv wants to merge 3 commits into
Conversation
Signed-off-by: Alex Fournier <afournier@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
Hey, thanks for working on this. The separation between correlation context and authorization identity is important. I've been testing Keel as an external pre-execution authorization service through OpenShell's Supervisor Middleware, and this connects directly to an issue we encountered during our v0.1.3 qualification. In that experiment, the adapter could associate I see this PR introduces For that next integration step, would the intended approach be to propagate the supervisor-generated That would let an external authorization provider join:
It would also be useful to preserve that identifier across request and response evaluation, including denials and middleware failures. I'm not suggesting that correlation alone proves exact-request binding or downstream effects. Those remain separate guarantees. Happy to help validate the eventual runtime integration against our existing sanitized test cases if helpful. |
… record Signed-off-by: Alex Fournier <afournier@nvidia.com>
Preserve the prior primitive-only review history while adopting the tested HTTP core built on merged NVIDIA#4789. Signed-off-by: Alex Fournier <afournier@nvidia.com>
afourniernv
changed the title
|
Written by Claude Code, reviewed by @krishicks A question about span volume. Every inspected request gets We see supervisor traces as a tool for latency and errors, sampled like any other service, rather than a full record of every request. OCSF already provides that record, and request rates belong in metrics. Sampling for supervisors is tracked in #5362, and supervisor metrics in #5364. In the meantime, we'd like these spans not to assume they're always exported. In particular, could you confirm that the request ID join through |
Compatibility review (advisory)New or updated breaking changes; see the full report for all findings. 1. api-behavior — No explicit risk acceptance foundSuccessful HTTP FORWARD requests without an L7 route now emit a fallback HTTP Activity record instead of the existing Allowed/Success record with the requested path and process.
2. api-behavior — No explicit risk acceptance foundx-openshell-request-id previously passed through HTTP forwarding and allowed middleware mutations; the candidate strips incoming values, replaces upstream values and rejects mutations.
3. api-behavior — No explicit risk acceptance foundInspected HTTP denials replace bounded supervisor.egress.deny spans with unbounded openshell.http.request and policy spans that can inherit workload traceparent.
Review details
Advisory review; publication gates are unchanged. |
krishicks
left a comment
krishicks
left a comment
There was a problem hiding this comment.
Written by Claude Code, reviewed by @krishicks
Reviewed at 7e8f7ae. Each point starts with what a user would see; the cause and file references follow. Line numbers refer to that commit.
Audit records
- A SIEM shows requests the supervisor blocked as allowed. A request blocked by a middleware failure (403), admission exhaustion (503), a WebSocket preflight error (502), or a credential preparation failure (502) gets one HTTP Activity record that says
Allowed, with no failure status.- Cause:
emit_primaryreclassifies the record only when the outcome isdenied(l7/correlation.rs:212-220). Theplatform_errorandupstream_erroroutcomes keep the allow record (relay.rs:900-904,relay.rs:1823-1855).
- Cause:
- Allowed plain-HTTP proxy requests no longer say which process made them. On main, every allowed FORWARD request without L7 inspection is recorded with its path, binary, PID, and cmdline. With this PR, it gets a placeholder record: "HTTP request ended before a response was selected", URL
/, no action, no process.- Cause: the correlation is created up front (
proxy.rs:5921-5926), so the empty slot is emitted at the response head. The real allow record arrives after the relay (proxy.rs:7228) and is dropped (correlation.rs:174).
- Cause: the correlation is created up front (
- Requests rejected before policy runs are reported as policy denials. An
Upgraderefusal, a malformed request target, or an invalid GraphQL or JSON-RPC body produces "HTTP request blocked after policy admission", URL/, and a span withpolicy.decision=deny. Policy never ran on these requests, so an operator investigating policy is sent to the wrong place.- Cause: these paths emit only a Network Activity record, so the placeholder record is emitted and relabelled as a denial (
relay.rs:1310-1335,relay.rs:1550-1665,relay.rs:3344-3348).
- Cause: these paths emit only a Network Activity record, so the placeholder record is emitted and relabelled as a denial (
- Some requests still produce extra HTTP Activity records that can't be joined by request ID, and one produces two records that disagree. This happens to any request with response middleware, a request-middleware denial, or a token-grant refusal. A token-grant refusal gives one record saying Denied and another saying Allowed with a 502. Denials on these paths also open a separate
supervisor.egress.denyspan next to the request span.- Cause: these paths still emit through
telemetry::emit_decisionwithoutcorrelation_uid(http_response.rs:698-1251,middleware.rs:1023,token_grant_injection.rs:344). The request ID is already available at the call sites (relay.rs:1810,relay.rs:2655,relay.rs:3138).
- Cause: these paths still emit through
- When credential resolution or MCP reinspection rejects a request, the record no longer shows which rule matched. The surviving record loses the request path, policy rule, and reason. A problem after the response headers are sent produces no correlated record at all.
- Cause:
emit()replaces the primary record with any later HTTP Activity, and silently drops one that arrives after the head is written (correlation.rs:166-176).
- Cause:
Trust and sampling
- A sandboxed workload decides which trace ID appears on supervisor audit records. If it sends a sampled
traceparent, the HTTP Activity record'strace.uidis the workload's trace ID. A workload could point its denials at an unrelated trace. Once #5362 adds a sampling ratio, it could also force its request spans to be exported regardless of that ratio.- Cause: a sampled workload context becomes the parent of
openshell.http.request(propagation.rs:169-174). #4789 documents that workload headers never supply supervisor correlation, and #2640 keeps workload context in a separate field. - Linking sampled context the way unsampled context is already linked would keep the connection to the agent's trace without either effect.
- Cause: a sampled workload context becomes the parent of
Behavior changes beyond correlation
These change what clients see. Could they move to a separate PR, or at least be listed in the PR description?
- Agent HTTP clients stop retrying, so a routine connection close becomes a failed request. When an upstream closes an idle keep-alive connection, main closes the client connection. Go, urllib3, and curl retry that automatically. With this PR the client gets a 502 and surfaces the error.
- Cause: a synthetic 502 on upstream EOF before headers (
http_response.rs:100-110).
- Cause: a synthetic 502 on upstream EOF before headers (
- Some upstream APIs that work today stop working. For example, a server that sends
HTTP/1.1 200without a reason phrase now has its response rejected, and the agent sees the connection close. The audit record still reports a 502.- Cause: new strict response-head validation that applies whether or not a request is correlated (
http_response.rs:425-440).httparseaccepts a missing reason phrase.
- Cause: new strict response-head validation that applies whether or not a request is correlated (
- Agents get a 500 response where the connection used to close. This happens on a missing credential or a stale credential generation.
- Cause:
relay.rs:1064-1080,proxy.rs:6851-6880.
- Cause:
Spans
- Traces don't show where an inspected request's time went. Only policy evaluation appears under
openshell.http.request. Middleware calls, credential preparation, and the upstream dial are not children of the request span, and remote middleware receives the connection's trace context instead of the request's. - WebSocket requests show up as a single span lasting the whole session. The root span stays open until the upgraded connection closes.
- The span marks an error but not which stage failed.
record_error_stageignores its stage argument (correlation.rs:162).
Tests
Most of the record problems above would have been caught by a test asserting exactly one HTTP Activity per request. The dispatch tests use .find() instead. These paths have no coverage:
- placeholder records and rejections before policy runs
- the content of a non-L7 FORWARD record
- token-grant refusal
- request-middleware denial
- upstream EOF before headers
- a second request on a keep-alive connection, which should get a new ID
Process
The branch conflicts with main.
|
This is what the robot drew when I asked for a before/after with this PR: 
|
|
Written by Claude Code, reviewed by @krishicks A follow-up connecting the review with my earlier comment on span volume. The compatibility review's item 3 raises the same concern: span volume. Review point 10 asks for a child span per stage, which adds spans. Nothing bounds that until #5362 adds supervisor sampling, and the #4789 deny-span limiter isn't the right tool, since #5362 removes it. Could the request spans land with or after #5362? This PR would keep the request ID, Then the after diagram would look like this: |
|
Drafted with Codex; reviewed and approved by @HarryMoss. Hi @afourniernv, I’ve read Kris’s latest review and the proposed split between the correlation core and request spans. My local #2640 connection-startup prototype is based on #5214 at Once you’ve settled the split, which revision should I track for those shared paths so I can avoid duplicating your fixes? Context: #2640 (comment) |
Summary
Connect an inspected HTTP request to its OpenShell policy span, OCSF record and response receipt. This replaces the primitive-only scope with the combined HTTP core, built on merged #4789.
Related Issue
Closes #5393. Tracking: #5210, which remains open for the remaining correlation layers. The component issue needs maintainer acceptance; this PR does not close the broader tracking issue.
Changes
metadata.correlation_uidand the protectedX-OpenShell-Request-IDresponse header.openshell.http.requestroot and policy-evaluation child. Emit one primary HTTP Activity record when response headers are accepted, with a fallback for earlier failure or cancellation.No new gateway RPC, protobuf field or Relay dependency. The receipt is not authorization or proof of complete delivery; L4 and uninspected passthrough traffic do not provide it. Middleware, credentials, revisions, MCP and terminal delivery remain separate layers, not part of this diff.
Testing
mise run pre-commit, including strict workspace and standalone clippy.mise run testrerun passed with four test threads. An initial unchanged VM cleanup fixture flaked; its isolated reruns and all 274 VM tests passed in the final run. Fullmise run ciis not claimed.Checklist
The review-history merge preserves the old draft branch without rewriting it. Its file tree is identical to the HTTP core, directly based on merged main.
Tested upstream snapshot:
638974575(merged #4789). Main has since advanced and GitHub currently reports conflicts. Ready for review does not mean merge-ready; the current code has not been retested on today's main.