Repository navigation
fix(ocsf): report https scheme for TLS-terminated requests - #4684
ericcurtin wants to merge 2 commits into
Conversation
Closes NVIDIA#4562 Signed-off-by: Eric Curtin <eric.curtin@docker.com>
|
@krishicks @matthewgrossman could you please accept #4562 so the issue check can pass? @pimlock PTAL, and |
EmilienM
left a comment
EmilienM
left a comment
There was a problem hiding this comment.
Another iteration needed. The ocsf_url helper is a nice way to fix the inspected paths, but the passthrough relay still passes "http" to middleware and to the response-middleware OCSF event on TLS traffic (see inline on relay.rs). So #4562's "all L7 relay emission paths" isn't fully covered yet. Two small nits on the test and docs as well.
| .http_request(HttpRequest::new( | ||
| &req.action, | ||
| OcsfUrl::new("http", &ctx.host, &redacted_target, ctx.port), | ||
| ctx.ocsf_url(&redacted_target), |
There was a problem hiding this comment.
This event now says https, but further down the same function still hardcodes "http": apply_request(..., "http", ...) at L3611 and response_relay(&req_with_auth, ctx, "http") at L3685. This path carries TLS-terminated traffic for endpoints without L7 rules (proxy.rs:3089 sets request_default_port = Some(443) before it gets here). The second literal ends up in HttpRequestTarget.scheme, and http_response_middleware_invocation_events builds its OCSF URL from it (rest/http_response.rs:947). So with response middleware on a TLS endpoint, one request logs https on the request events and http on the response-middleware event, and the middleware itself gets http as input.
Suggest ctx.request_scheme() at both, the same way #4397 did for the inspected path (L756), plus a passthrough case in the regression test that checks the response-middleware event.
There was a problem hiding this comment.
Fixed, both call sites use ctx.request_scheme(). Added a passthrough middleware test.
| let ctx = L7EvalContext { | ||
| host: "api.example.test".into(), | ||
| port, | ||
| request_default_port: Some(port), |
There was a problem hiding this comment.
Nit: port and request_default_port always match here, so this would still pass if the scheme came from ctx.port. A case where they differ (TLS on 8443 -> https with :8443 in the display, plaintext on 443 -> http) would pin down the transport-based behavior.
There was a problem hiding this comment.
Done. Cases now cover TLS on 8443 and plaintext on 443.
|
|
||
| ```text | ||
| OCSF HTTP:POST [INFO] ALLOWED POST http://httpbin.org:443/anything [policy:httpbin engine:middleware] [failed:false transformed:true] | ||
| OCSF HTTP:POST [INFO] ALLOWED POST https://httpbin.org/anything [policy:httpbin engine:middleware] [failed:false transformed:true] |
There was a problem hiding this comment.
Nit: a few samples for TLS traffic to api.github.com still show http://: lines 37 and 125 here, and docs/observability/accessing-logs.mdx:23. Worth switching them to https:// in the same pass.
There was a problem hiding this comment.
Done, switched to https://.
Signed-off-by: Eric Curtin <eric.curtin@docker.com>
This branch has not been deployed
2 participants
Summary
L7 OCSF HTTP events recorded
httpfor TLS-terminated requests. Use the transport scheme.Related Issue
Closes #4562
Changes
L7EvalContext::ocsf_url, built fromrequest_scheme."http"URL sites with it.Testing
cargo test,cargo clippy -D warningsandcargo fmtpass foropenshell-supervisor-network.Checklist