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>
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.
…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.
…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.
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: 2a6fd919be84afe8929264253a391bc4800ae49f
Thanks @pimlock. I checked the current non-draft head after the Linux handler-setup adjustment for the plaintext preflight regression. The test-only change preserves the no-upstream-connect assertion, and the required Branch Checks now pass. The fresh independent review found one high-severity resource-boundary defect, two medium-severity reload close-path defects, and two low-severity published-doc inaccuracies; all five are attached inline.
Review findings:
- High: unbounded upstream writes can retain every process-wide WebSocket assembly permit indefinitely.
- Medium: a policy reload after the upstream
101was forwarded can produce abrupt EOF instead of a typed1012close. - Medium: a generation check between data-frame header and payload writes can corrupt the subsequent close frame.
- Low: published Fern docs still omit the assembly budget, fragment/timeout limits,
1013exhaustion behavior, and process-lifetime/reload scope. - Low: published
ValidateConfigtimeout semantics do not match the implementation.
Docs: The relevant Fern pages and mapped skills were updated, and navigation needs no change, but the two attached documentation issues remain.
Next state: gator:in-review pending author changes. DCO, Branch Checks, and Helm Lint are passing; the required E2E gate is pending. 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.
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: 4242919a2918b140b56cad1104feec2c01c59867
Thanks @pimlock. I checked the current-head hardening update for bounded upstream forwarding, typed reload closure after 101, frame-boundary generation checks, published limits, and ValidateConfig timeout semantics. All five prior findings are resolved. The fresh independent review found two remaining protocol-correctness issues; one line-specific finding is attached inline.
Review findings:
- Medium: parsed middleware sessions can preserve WebSocket extensions that the parsed relay cannot handle. Extension negotiation currently depends only on the L7 endpoint configuration, but a retained middleware session independently forces the parsed relay on REST endpoints without credential rewriting and on plaintext forward-proxy routes without WebSocket L7 configuration. Those paths preserve
permessage-deflatewhile configuring the parsed relay with no decompression, so valid RSV1 text frames are rejected with1002. Make safe extension negotiation depend on a retained inspecting middleware session and addws://andwss://compressed-message relay coverage. - Medium: a stage that returns
SKIPis immediately ended asCANCELLATIONbefore the aggregate concurrent-preflight result is known. If another stage denies or fails closed, the skipped stream receives the wrong terminal reason. The actionable detail is attached inline.
Docs: The relevant Fern pages now cover assembly and session limits, fragment and timeout bounds, 1013, process-lifetime reload scope, and the correct ValidateConfig timeout. Navigation already includes the extensibility folder.
Next state: gator:in-review pending author changes. Required Branch Checks and E2E are still pending. 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.
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