feat(middleware): inspect WebSocket text messages - #2477
Conversation
|
🌿 Preview your docs: https://nvidia-preview-pr-2477.docs.buildwithfern.com/openshell |
This comment was marked as resolved.
This comment was marked as resolved.
pimlock
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: This PR remains project-valid because it implements the maintainer-authored, review-ready PR1 plan in #2428.
Head SHA: d81890743a80e74962ab4fdf8a51b22e068f2477
Thanks @pimlock. I checked your update describing the plaintext-upgrade path, bounded admission, typed termination, stream lifetime, fail-open observability, docs, skills, and expanded coverage. The plaintext path, pre-buffer admission, typed close handling, stream-deadline removal, mapped skill updates, and much of the added coverage are present. The independent re-review found three high-severity and four medium-severity issues that remain; they are attached inline.
Review findings:
- High: incomplete WebSocket messages can pin all shared middleware admission indefinitely.
- High: middleware-only parsed WebSockets do not observe policy-generation reloads.
- High: the operator docs claim the built-in regex covers WebSockets, but its manifest and implementation do not.
- Medium: persistent middleware streams have no independent session bound; fully disabled fail-open sessions still require admission; one path performs token-grant work before preflight; and policy denial is reported as middleware denial.
Docs: Fern pages and all three mapped skills were updated, but the built-in WebSocket coverage claim must be corrected or implemented.
Next state: gator:in-review pending author changes. No local tests were run as part of this code-only review.
This comment was marked as resolved.
This comment was marked as resolved.
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
ef26419 to
83121b1
Compare
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
|
/ok to test dea07e4 |
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
|
/ok to test ce9d4cb |
|
Label |
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
BlockedHead SHA: Gator is blocked because this pull request remains a draft, so the independent code review must wait until it is ready for review. The prior reviewer authentication failure did not produce a valid review disposition, and the earlier draft-blocker disposition was for a different head. DCO is passing. The required Branch Checks, Helm Lint, and E2E gates are pending. The existing Next action: mark the pull request ready for review. Gator will then run one fresh independent review for the current head. I did not approve, merge, push, apply |
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
…e-introspection/pimlock Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
|
/ok to test 63d86ff |
pimlock
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: This PR remains project-valid because it implements the maintainer-authored, review-ready PR1 plan in #2428 for forward WebSocket text-message middleware.
Head SHA: 63d86ffd14168b94b7f3c2c77f65b2eefd021514
Thanks @pimlock. I checked the current non-draft head after your updates addressing the earlier plaintext upgrade, admission, lifecycle, policy-reload, regex, token-grant ordering, denial telemetry, docs, and coverage findings. Those earlier fixes are present, but the fresh independent review found two high-severity resource-boundary gaps and two medium-severity lifecycle/compatibility issues; all four are attached inline.
Review findings:
- High: the persistent-session limit resets with each replaceable middleware registry generation.
- High: parsed text assembly is aggregate-bounded only when an active middleware session supplies admission.
- Medium: disabling a stream drops it without the documented best-effort session-end event.
- Medium: the gateway TOML field rename rejects existing
max_body_bytesconfigurations.
Docs: The relevant Fern pages and mapped skills were updated, and no navigation change is needed. The statement in architecture/sandbox-limits.md that every parsed text assembly acquires shared work before buffering must be corrected with the resource-boundary fix.
Next state: gator:in-review pending author changes. No local tests were run as part of this code-only review.
| )) | ||
| .into()); | ||
| } | ||
| let admission = if let Some(session) = options.middleware_session.as_mut() { |
There was a problem hiding this comment.
gator-agent
High (CWE-770): Assembly admission is acquired only when a middleware session exists, but parsed WebSocket text is also buffered for native policy, credential rewriting, compression, and sessions whose fail-open middleware stages were disabled. An attacker can open many such connections and pin up to 4 MiB per in-progress message for the assembly timeout, leaving aggregate supervisor memory unbounded. Acquire a process-wide assembly permit before buffering every parsed text message, independent of middleware presence, and hold it through inspection/rewrite. Cover non-middleware and post-disable parsed relays in the saturation tests.
There was a problem hiding this comment.
Proposed disposition: defer aggregate admission for every parsed WebSocket text assembly to a focused hardening follow-up rather than expand PR1 further.
The current per-connection protections remain: a 4 MiB complete and decompressed message ceiling, 4,096 fragment ceiling, 30 second assembly idle timeout, and 2 minute absolute assembly timeout. Active middleware sessions also acquire shared work admission before buffering. Commit b1ee2ddf corrects architecture/sandbox-limits.md so it no longer claims that every parsed relay path has aggregate admission.
The rationale is:
- The aggregate gap predates this PR. The base implementation already buffered up to 1 MiB per parsed connection without process-wide assembly admission. This PR raises the shared payload ceiling to 4 MiB but also adds the idle and total assembly deadlines.
- The intended deployment is primarily one agent process or tightly related process tree per sandbox. Exhaustion by a rogue client is therefore mainly sandbox self-denial of service: it can terminate its own supervisor and workload, rather than violating the functional correctness of another sandbox.
- There can still be host pressure if the runtime does not enforce sandbox memory, process, socket, and file-descriptor limits. Those runtime and node boundaries remain the multi-sandbox protection layer.
- External middleware must enforce its own tenant and global limits regardless of any per-sandbox cap. Even a 32-session sandbox limit does not protect a service receiving traffic from many thousands of sandboxes.
- A complete fix belongs to the network supervisor, independent of middleware presence and registry generation. It needs an explicit choice between concurrent-assembly and byte-weighted admission, bounded wait versus immediate shedding, terminal close behavior, telemetry, operator narrowing, and fairness. Coverage must include native policy, credential rewriting, compression, and post-disable fail-open relays.
Given the current single-workload sandbox model and existing per-message and time bounds, I consider this defense-in-depth hardening rather than a prerequisite for the forward text middleware contract. If sandbox runtime resource limits are not reliably enforced, this should be promoted because it then becomes indirect host protection rather than only sandbox self-denial of service.
…e-introspection/pimlock Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
pimlock
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: This PR remains project-valid because it implements the maintainer-authored, review-ready PR1 plan in #2428 for forward WebSocket text-message middleware.
Head SHA: b1ee2ddf5a3caed72ea1bf5752b801054390eab9
Thanks @pimlock. I checked your current-head updates and proposed disposition. The process-lifetime admission preservation, legacy max_body_bytes alias, and bounded best-effort session-end fix are present and coherent. The architecture document now discloses the aggregate assembly gap, but the independent review does not consider deferral safe because untrusted sandbox traffic can still consume unbounded aggregate supervisor memory and impair the sandbox security control.
Review findings:
- High: policy reloads can race payload assembly or middleware evaluation and allow an old-generation message to reach credentials or upstream.
- High: parsed text assembly remains aggregate-unbounded whenever no active middleware session supplies admission.
- Medium: plaintext
ws://preflight occurs only after the upstream TCP connection is established. - Medium: pre-upgrade session error paths can report
CANCELLATIONinstead of the typed terminal cause. - Medium: HTTP-only attachments can be rejected by WebSocket preflight envelope validation before binding selection.
- Low: the Fern middleware page overstates aggregate admission coverage.
All six findings are attached inline.
Docs: The relevant Fern pages are present and already navigated. The shared-capacity statement needs correction, and the before-upstream-contact contract must match the plaintext implementation.
Next state: gator:in-review pending author changes. No local tests were run as part of this code-only review. I did not approve, merge, push, apply test:e2e, or post /ok to test.
| miette!("websocket middleware message missing work admission"), | ||
| ) | ||
| })?; | ||
| let outcome = session |
There was a problem hiding this comment.
gator-agent
High (CWE-367): Generation is checked only after reading the frame header. A reload while admission, payload assembly, or this middleware RPC is awaiting can still let the old-generation message undergo credential resolution and reach upstream. Recheck the guard after admission/assembly and after middleware evaluation, immediately before credential resolution or upstream writes; terminate with close 1012 and POLICY_RELOAD. Add regressions that reload during a partial single-frame payload and a delayed middleware response, asserting no stale message or credential reaches upstream.
There was a problem hiding this comment.
Fixed in 6055dc5. The parsed relay now revalidates generation after assembly admission, middleware admission, every payload assembly, native inspection, middleware evaluation, credential rewriting, and between frame-header and payload writes. Upgrade handoff also checks generation before and after session start. Regression coverage includes reload during a partial single-frame payload, fragmented input, and a delayed middleware response carrying a credential placeholder.
| )) | ||
| .into()); | ||
| } | ||
| let admission = if let Some(session) = options.middleware_session.as_mut() { |
There was a problem hiding this comment.
gator-agent
High (CWE-770 / OWASP LLM06): Aggregate admission is conditional on an active middleware session. Native-policy, credential-rewrite, compression-only, no-middleware, and fully disabled fail-open relays can each buffer 4 MiB across an unbounded number of concurrent connections. Per-message, fragment, and timeout bounds do not provide an aggregate memory bound. Add a process-wide network-supervisor assembly budget, preserved across reloads, acquired before payload buffering with bounded wait/load shedding. Test many concurrent incomplete messages without active middleware. I do not consider deferral safe: the sandbox workload is untrusted, and cgroups limit host impact but do not preserve availability of the sandbox security control.
There was a problem hiding this comment.
Implemented in 6055dc5. This is a network-owned process-lifetime budget, independent of middleware: 32 active text assemblies plus 64 unbuffered waiters, shared by old and new policy generations and used by every parsed relay. Admission happens before payload allocation/read and the permit is retained through decompression, inspection, middleware, rewriting, and forwarding. Queue exhaustion closes with 1013 and emits payload-free telemetry. Tests cover saturation without middleware, bounded waiting/load shedding, recovery, and reload sharing.
| }; | ||
| } | ||
|
|
||
| let mut middleware_session = if let Some(chain) = websocket_chain.as_deref() { |
There was a problem hiding this comment.
gator-agent
Medium (CWE-863): Plaintext ws:// middleware preflight occurs after TcpStream::connect at line 4848. A preflight DENY therefore remains externally observable by the destination, contrary to the protobuf contract that preflight occurs before upstream contact. Move the direct dial after HTTP admission and WebSocket preflight. Add a listener regression asserting that a denied upgrade causes no upstream accept.
There was a problem hiding this comment.
Fixed in 6055dc5. The plaintext forward path now dials upstream only after authorization, HTTP middleware, WebSocket preflight, token-grant resolution, credential rewriting, and generation validation, then validates generation again after the awaited connect. A Linux handler regression verifies that a denied WebSocket preflight never reaches the upstream listener.
| .await?; | ||
| return Ok(()); | ||
| } | ||
| preflight.session |
There was a problem hiding this comment.
gator-agent
Medium: After retaining the preflight session, later guarded relay/setup operations use ? or early returns. Those paths drop WebSocketSession, whose Drop implementation reports CANCELLATION; a generation change should report POLICY_RELOAD, while upstream rejection should use its typed reason. Introduce reason-aware finalization around the pre-upgrade session. Cover reload after successful preflight but before the upgrade write. The plaintext path around proxy.rs:5070 has the same classification problem.
There was a problem hiding this comment.
Fixed in 6055dc5. Pre-upgrade sessions now use reason-aware finalization: stale generations report POLICY_RELOAD, ordinary relay/connect rejection reports UPSTREAM_REJECTED, and local setup cancellation is explicit. Generation is checked around session start as well as before the upgrade handoff. Regression coverage verifies that reload after successful preflight ends the opened stream with POLICY_RELOAD rather than the Drop fallback.
| if entries.is_empty() { | ||
| return Ok(empty_preflight_result()); | ||
| } | ||
| validate_preflight_input(&input)?; |
There was a problem hiding this comment.
gator-agent
Medium: Preflight input limits are validated before operation binding selection. Consequently, a valid upgrade with more than 32 offered subprotocols is rejected with 502 even when every matched attachment is HTTP-only and should produce binding_not_selected coverage while allowing the connection. Describe/select the WebSocket chain first and validate the preflight envelope only when a WebSocket stage participates. Add an HTTP-only attachment regression with 33 valid subprotocols.
There was a problem hiding this comment.
Fixed in 6055dc5. WebSocket bindings are now described and selected before the preflight envelope is validated. If no WebSocket stage participates, OpenShell returns coverage-only success without applying limits to an envelope it will never send. The regression uses an HTTP-only fail-closed attachment with 33 requested subprotocols and verifies allowed/no-session binding_not_selected coverage.
| 3. Complete client-to-upstream text messages in sequence order. OpenShell reassembles fragmented messages and decompresses negotiated `permessage-deflate` messages before evaluation. | ||
| 4. A best-effort session-end event when the stage stream remains writable. OpenShell attempts at most one terminal event for each opened stream, including streams opened during a preflight that rejects the upgrade before session start. | ||
|
|
||
| Allowed replacements are re-framed, re-compressed when required, and forwarded. Binary messages, control frames, and upstream-to-client traffic remain uninspected. Binary messages pass through under both `on_error` modes. For each active selected stage, OpenShell emits an informational `unsupported_message_type` coverage event and advances the session-global sequence; the next text message can therefore reach the stage with a valid sequence gap. OpenShell reserves shared middleware capacity before buffering HTTP bodies or WebSocket text messages; at most 32 evaluations run and 64 additional unbuffered callers wait for capacity. When both bounds are full, OpenShell sheds an HTTP request with `503 Service Unavailable` before reading its body. Persistent middleware streams use a separate process-wide budget of 32 sessions. WebSocket session admission does not wait: if the budget is full, OpenShell applies each selected config's `on_error` behavior before opening a stream. |
There was a problem hiding this comment.
gator-agent
Low: This says shared middleware capacity is reserved before buffering WebSocket text messages generally. The implementation only does so for active middleware sessions; architecture/sandbox-limits.md correctly documents the uncovered parsed paths. Qualify this statement or implement aggregate admission for every parsed text assembly.
There was a problem hiding this comment.
Fixed in 6055dc5. The documentation now distinguishes the network-owned assembly budget used by every parsed WebSocket text message from the additional middleware work budget used only by active middleware sessions. The sandbox limits inventory also documents the 32 active/64 waiter assembly bound, 1013 load shedding, reload-stable ownership, and payload-free exhaustion telemetry.
…e-introspection/pimlock Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
pimlock
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: This PR remains project-valid because it implements the maintainer-authored, review-ready PR1 plan in #2428 for forward WebSocket text-message middleware.
Head SHA: 6055dc5ce9d7c694f70b29b3ed14fd1bc6e104ea
Thanks @pimlock. I checked your current-head updates covering the process-wide assembly budget, typed pre-upgrade finalization, published-doc distinction, plaintext preflight ordering, binding selection, and generation rechecks. The independent review found those six earlier concerns resolved and found no blocking code or security defect.
Review findings:
- Blocking CI:
proxy::tests::plaintext_websocket_preflight_denial_does_not_connect_upstreamtimes out withElapsed(())atcrates/openshell-supervisor-network/src/proxy.rs:5714in both amd64 and arm64 Rust jobs. The required Branch Checks are therefore failing and need a code/test fix or a demonstrated infrastructure disposition. - Low docs gap: the published Fern page does not state the process-wide assembly limits, reload scope, timeout/fragment bounds, or
1013overload behavior. The actionable detail is attached inline.
Docs: The relevant Fern pages are present and already navigated, but this operator-visible capacity and close-code behavior remains incomplete.
Next state: gator:in-review pending the CI failure and documentation follow-up. Core E2E is also currently failing in two Podman-rootless jobs. No local tests were run as part of this code-only review. I did not approve, merge, push, apply test:e2e, or post /ok to test.
| 3. Complete client-to-upstream text messages in sequence order. OpenShell reassembles fragmented messages and decompresses negotiated `permessage-deflate` messages before evaluation. | ||
| 4. A best-effort session-end event when the stage stream remains writable. OpenShell attempts at most one terminal event for each opened stream, including streams opened during a preflight that rejects the upgrade before session start. | ||
|
|
||
| Allowed replacements are re-framed, re-compressed when required, and forwarded. Binary messages, control frames, and upstream-to-client traffic remain uninspected. Binary messages pass through under both `on_error` modes. For each active selected stage, OpenShell emits an informational `unsupported_message_type` coverage event and advances the session-global sequence; the next text message can therefore reach the stage with a valid sequence gap. The network supervisor reserves process-wide assembly capacity before buffering every parsed WebSocket text message. Active middleware sessions additionally reserve shared middleware capacity before buffering WebSocket text, and HTTP middleware reserves the same capacity before buffering request bodies; at most 32 evaluations run and 64 additional unbuffered callers wait for capacity. When both middleware bounds are full, OpenShell sheds an HTTP request with `503 Service Unavailable` before reading its body. Persistent middleware streams use a separate process-wide budget of 32 sessions. WebSocket session admission does not wait: if the budget is full, OpenShell applies each selected config's `on_error` behavior before opening a stream. |
There was a problem hiding this comment.
gator-agent
Low — operator documentation: The published docs distinguish the network-owned assembly budget from middleware admission, but omit its operator-visible limits and failure behavior. The implementation permits 32 active assemblies and 64 unbuffered waiters, closes overflow with 1013, limits messages to 4,096 fragments with 30-second idle and 120-second total deadlines, and preserves the budget across reloads. Those details currently appear only under architecture/, while the Fern close-code list also omits 1013. Please document them here or link to a published Fern reference.
Summary
Tip
Visual overview of this change: preview, source.
Note: this reads from github without auth, so it may be rate limited, especially when on VPN.
This pull request adds supervisor middleware for WebSocket traffic.
OpenShell asks selected middleware for a session decision before it contacts the upstream service. A middleware stage can inspect the session, skip it, or deny the upgrade. A skipped stage uses no persistent stream and receives no messages for that session.
After the upstream service accepts the upgrade, middleware can inspect complete text messages from the client. It can allow, replace, or deny each message. OpenShell runs middleware before it adds managed credentials.
This first version does not inspect messages from the upstream service, binary messages, or control frames. Binary messages pass without inspection and produce coverage telemetry. Inspection of messages from the upstream service remains future work.
This preflight lifecycle also provides a base for other streaming protocols, such as streaming HTTP.
Related Issue
Part of #2428
Changes
Attachment and preflight
endpoints.includeandendpoints.excludehost selectors to attach middleware.WEBSOCKET_MESSAGE/PRE_CREDENTIALS.INSPECT,SKIP, andDENYpreflight decisions.SKIPas a successful decision. It does not useon_error.DENYbefore upstream contact. It applies in both error modes.binding_not_selectedcoverage for traffic after the upgrade.The host selector is less precise than a request selector. Future work can add more selection inputs after we have clear use cases.
Message processing
EvaluateWebSocketstream for each inspecting stage.openshell/regexmiddleware for HTTP bodies and client WebSocket text messages.Resource limits
OpenShell records middleware limits in
architecture/sandbox-limits.md.Each sandbox supervisor owns its middleware registry and its local budgets. One sandbox cannot directly consume another sandbox's permits. The limits still protect host memory, CPU time, relay tasks, gRPC streams, channel buffers, and shared middleware services.
Initial limits:
These are permissive alpha defaults. Production data can change them. Future configuration can let operators reduce selected limits below platform ceilings.
When both work budgets are full, OpenShell returns
503 Service Unavailablebefore it buffers an HTTP body. WebSocket overload keeps its protocol-specific failure behavior.Compression
OpenShell does not disable all WebSocket compression.
For parsed modes that manage WebSocket extensions, OpenShell supports
permessage-deflateonly when client-to-server context takeover is disabled withclient_no_context_takeover. OpenShell removes unsupported extension offers.OpenShell decompresses and recompresses each inspected client message with a fresh DEFLATE context. Each client message is independently decodable. This mode also keeps memory accounting and message replacement bounded.
Server-to-client context takeover can remain enabled because this version does not inspect or replace messages in that direction.
Failure handling and telemetry
on_erroronly after a selected binding fails.1002,1007,1008,1009, and1012for the defined terminal cases.Testing
mise run pre-commitmise run testmise run cimise run e2e:websocket-conformancemise run e2e:dockerChecklist