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>
Review Convergence CheckpointHead SHA: Four finding-bearing review rounds have completed, so Gator has reached the required human convergence checkpoint. Thanks @pimlock. I checked the merge-main update and its conflict resolutions in the six affected files. The checkpoint-only independent review found no new Critical defect introduced by that delta. Root-cause findings:
Scope growth:
Reviewer-quality signals:
Maintainer action: accept the current scope, split the remaining extension-negotiation obligation into follow-up work, waive it, or explicitly authorize another autonomous review round. Next state: |
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Maintainer convergence updateAddressed the remaining compression obligation in
Validation:
I explicitly authorize one final focused Gator verification round on current head |
Re-check After Maintainer UpdateThanks @pimlock. I re-evaluated current head Head SHA: What I checked: the post-checkpoint author delta, the retained-session extension-negotiation decision in the REST/TLS and plaintext forward-proxy paths, and the compressed Disposition: resolved. A retained inspecting middleware session now selects managed The mixed-preflight terminal-reason attribution remains waived by your verified maintainer disposition and was not re-raised. All other durable Gator findings remain resolved. Remaining items:
Next state: I did not approve, merge, push, apply |
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Re-check After Author UpdateThanks @pimlock. I checked head Head SHA: What I checked: the author-only two-file test delta, including the bounded scenario orchestration, relay-task cleanup, and the arm64 timeout path that failed on the previous head. The bounded independent checkpoint review found no newly introduced Critical security, data-loss, or correctness defect. No production behavior changed. Disposition: resolved. All durable Gator findings remain resolved or maintainer-waived, and no blocking review findings remain. Required Branch Checks and E2E are running for this head. Helm Lint and GPU E2E are green. The existing Next state: I did not approve, merge, push, apply |
| } | ||
|
|
||
| message WebSocketPreflightDecision { | ||
| WebSocketPreflightAction action = 1; |
There was a problem hiding this comment.
long chat w/ agent:
Should an INSPECT preflight decision be able to declare which requested subprotocols the middleware supports? Preflight sees the offered protocols, but the actual selection is learned later through the notification-only session_start. If middleware supports only a subset, it currently cannot distinguish “inspect if upstream selects X” from “inspect regardless of the selection.”
Consider adding something like accepted_subprotocols to WebSocketPreflightDecision. OpenShell could validate the upstream selection before starting message inspection and apply explicitly defined behavior—such as denying or skipping—when the selected protocol is outside that set. This keeps preflight as the single decision point instead of introducing a second decision at session start.
Bascially, do we see the initial preflight req/resp as being protocol-aware?
There was a problem hiding this comment.
the callout was that the subprotocol-negotiation happens after this initial decision, which kinda makes a nested state machine
There was a problem hiding this comment.
Good question.
I think we can defer this until a concrete middleware needs it. We can still add it later, if we get some signal on it being needed. Adding it now introduces more conditions into an already complex WebSocket handshake and I'm not sure how common this subprotocol negotiation is, how often does it include multiple options, etc. (tried to look for this and it seems like it's not that common).
What could go wrong today?
- Client offers subprotocols
[a, b, c]. - Middleware only understands
b. - Middleware sees the offered protocols during preflight, but cannot make
INSPECTconditional on the final selection. - Upstream selects
a. - Middleware receives
session_startwitha, but that event is notification-only. - When messages arrive, middleware may fail to parse them and either deny or trigger its configured
on_errorbehavior.
If middleware could declare support for [b], OpenShell could narrow the upstream offer to [b]. If upstream supports b, the connection could proceed successfully.
Possible future flow
- Client offers
[a, b, c]. - Preflight returns an optional accepted-subprotocol constraint.
- We intersect all sets (client + all middleware's preflight) and offer that to the server.
- One gets selected or server rejects the upgrade.
This would be an addition to the preflight decision and we could make it backward compatible (i.e. not a one-way door).
| } | ||
|
|
||
| message WebSocketPreflightDecision { | ||
| WebSocketPreflightAction action = 1; |
There was a problem hiding this comment.
Good question.
I think we can defer this until a concrete middleware needs it. We can still add it later, if we get some signal on it being needed. Adding it now introduces more conditions into an already complex WebSocket handshake and I'm not sure how common this subprotocol negotiation is, how often does it include multiple options, etc. (tried to look for this and it seems like it's not that common).
What could go wrong today?
- Client offers subprotocols
[a, b, c]. - Middleware only understands
b. - Middleware sees the offered protocols during preflight, but cannot make
INSPECTconditional on the final selection. - Upstream selects
a. - Middleware receives
session_startwitha, but that event is notification-only. - When messages arrive, middleware may fail to parse them and either deny or trigger its configured
on_errorbehavior.
If middleware could declare support for [b], OpenShell could narrow the upstream offer to [b]. If upstream supports b, the connection could proceed successfully.
Possible future flow
- Client offers
[a, b, c]. - Preflight returns an optional accepted-subprotocol constraint.
- We intersect all sets (client + all middleware's preflight) and offer that to the server.
- One gets selected or server rejects the upgrade.
This would be an addition to the preflight decision and we could make it backward compatible (i.e. not a one-way door).
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