fix(network): preserve chunked request boundaries - #3530
Conversation
|
Label |
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
107ce34 to
738965b
Compare
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
|
The MCP E2E failure was caused by the 8 KiB write-coalescing added in the previous revision. A small chunk on the long-lived MCP stream remained buffered until end-of-body, so the elicitation callback could not run and Commit 619c86a keeps connection-scoped read-ahead but forwards each complete validated chunk immediately. It also adds an open-stream regression that verifies the first chunk reaches the destination before the terminal chunk is sent. Verified locally:
|
|
Was this a regression? IIRC this was a request smuggling issue we fixed a while ago. Def worth getting in as a security hardening if this is ready to approve and merge. |
@johntmyers |
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Maintainer Approval NeededGator’s critical-only review of the current merge-only update found no new blocker, and all required checks are green. @johntmyers asked whether this was a regression; thanks @pimlock for clarifying that the earlier #2373 work did not cover this pipelined-request boundary case. I checked #2373’s stated scope, the current range-diff, and the three PR-owned file blobs; the five reviewed PR commits and their file contents are unchanged by the latest merge from Review: The earlier framing-amplification finding remains resolved, with no open Gator threads and no new Critical defect in the current author patch. Human maintainer approval is now required. Gator metadata
|
Monitoring CompleteMonitoring is complete because this PR has merged. Final status: The current head passed the required Branch Checks, Helm Lint, Trivy Changes, and E2E gates, and received maintainer approval before merge. I removed the active Gator metadata
|
Summary
Preserve HTTP/1.1 request boundaries with connection-scoped read-ahead buffering so a following pipelined request remains available for its own L7 policy decision. Forward each complete validated chunk promptly without imposing a cumulative framing limit.
Related Issue
No issue required: this is an obvious localized correctness fix in the chunked request relay.
Failure Scenario and Fix
A client can send an allowed chunked request and the next request in one socket write:
Previously, the chunked relay could read both requests into its local 8 KiB buffer and write the entire buffer upstream before recognizing the
0\r\n\r\nterminator. The subsequentDELETEtherefore reached upstream as overflow from the allowed request before its own policy evaluation.The inspected connection now owns the read-ahead buffer for its full keep-alive lifetime:
The upstream receives only the allowed
POST. The remainingDELETEstays in the connection buffer, enters the normal request loop, and receives a separate policy decision.There is no longer a 32 KiB aggregate framing limit. Valid requests with many tiny chunks continue to relay. Underlying reads use the persistent 8 KiB connection buffer, and each complete validated chunk is forwarded promptly. Individual chunk-size and trailer lines remain subject to the existing 16 KiB line ceiling so an unterminated line cannot grow memory without bound.
Changes
Testing
mise run pre-commitpassesmise exec -- cargo test -p openshell-supervisor-network --lib(1,341 passed, 2 ignored)mise run --no-deps --skip-deps e2e:mcp(3 scenarios passed, including elicitation defaults)OPENSHELL_E2E_DOCKER_TEST=proxy_egress_pipeline mise run e2e:docker(11 passed)Checklist