Conversation
Replace the unary HTTP request evaluator with a bidirectional streaming protocol and move SigV4 signing into the built-in middleware stage. BREAKING CHANGE: replace the unary HTTP request middleware RPC with the streaming HttpRequestPreCredentials Evaluate contract. Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
|
🌿 Preview your docs: https://nvidia-preview-pr-3450.docs.buildwithfern.com/openshell |
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
In auto mode, a bodyless request with no x-amz-content-sha256 falls through to UNSIGNED-PAYLOAD. That's not the normal SigV4 empty-body hash and is service-specific, so a GET/HEAD to a non-S3 AWS service can be re-signed with payload semantics the service rejects. Could BodyFraming::None sign Bytes(&[]) unless the client explicitly requested an unsigned payload, and add a non-S3 empty-body case?
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
pimlock
changed the title
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
This comment has been minimized.
This comment has been minimized.
pimlock
left a comment
pimlock
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
This maintainer-authored implementation is project-valid and the initial full-patch review found four blocking runtime-contract regressions.
Action required: @pimlock, please address the four inline findings and push an updated head for a focused follow-up review.
Blocking findings:
GATOR-96ff9454-01: restore the aggregate HTTP middleware chain deadlineGATOR-96ff9454-02: bound retained STREAM output in complete-body callersGATOR-96ff9454-03: preserve denial diagnostics and invocation telemetryGATOR-96ff9454-04: reliably deliver and correctly classify terminal events
Carried findings:
- None
Non-blocking suggestions:
- None
Gator metadata
- Validation: Maintainer-authored work implementing linked issue #3307 and the approved middleware protocol direction
- Docs: Fern middleware and gateway documentation are updated
- Checks: Current gate publication is green, but comprehensive required jobs have not run on this head
- E2E: Required for middleware and network-proxy behavior; dispatch waits until blocking review feedback is resolved
- Head SHA:
96ff9454396dbe362682ed3e206a40a7764d1120 - Base SHA:
fa0bfa490e42c87a74a70be6ebb40faee7fb8faa - Merge base SHA:
fa0bfa490e42c87a74a70be6ebb40faee7fb8faa - Patch ID:
540cf899eff802a35e24339758729c389fe3e5e0 - Gator payload:
9 - Review mode:
initial - Previous reviewed SHA: none
- Review budget exhausted: no
- Maintainer decision required: no
- Next state:
gator:in-review
| limits: Some(limits), | ||
| declared_input_bytes: input.declared_body_length, | ||
| }; | ||
| let opened = tokio::time::timeout(entry.timeout(), async { |
There was a problem hiding this comment.
gator-agent
Warning — GATOR-96ff9454-01 · Restore the aggregate chain deadline
Summary: An operator with a multi-stage HTTP middleware chain can have each service respond just before its per-stage timeout. Because every request and response stage gets a fresh timeout, ten 30-second stages can hold a request and a shared session permit for roughly five minutes even though the platform contract limits a middleware chain to 30 seconds. A slow or compromised registered service can exhaust the 32-session budget and deny protected traffic.
Fix: Create one MAX_MIDDLEWARE_CHAIN_TIMEOUT deadline per HTTP chain operation and cap every sequential stream-open, send, and result wait by its remaining time. Keep the separate two-minute request-body lifetime.
Verify: With paused time, make two 30-second stages answer after 29 seconds each; the chain must fail with middleware_chain_timeout by 30 seconds for both request and response preflight.
Agent context
- Location:
crates/openshell-supervisor-middleware/src/request.rs:894 - Sibling paths: response preflight, compatibility evaluation, and request body-stage exchanges restart the same per-stage budget.
- Ownership: The replaced base paths enforced one aggregate deadline.
There was a problem hiding this comment.
Resolved GATOR-96ff9454-01 in 284a896.
Change: Request/response preflight and body processing now share one aggregate chain deadline; each stage open, send, and result wait is capped by the earlier of that deadline and the stage timeout. The independent two-minute request-body lifetime remains intact.
Verification: mise run test and mise run ci pass, including all 34 middleware tests and 1,326 network tests.
| let mut units = Vec::new(); | ||
| while let Some(event) = output_rx.recv().await { | ||
| if let HttpRequestBodyOutput::Chunk(data) = event { | ||
| units.push(data); |
There was a problem hiding this comment.
gator-agent
Warning — GATOR-96ff9454-02 · Bound retained STREAM output
Summary: Body-aware GraphQL, MCP, JSON-RPC, and forward-proxy paths collect middleware output before policy re-evaluation or forwarding. A registered or compromised STREAM middleware can emit unlimited individually valid 64 KiB chunks; this collector retains them all and the compatibility evaluator then concatenates them into another allocation. One admitted request can therefore exhaust supervisor memory and disrupt enforcement for unrelated traffic.
Fix: For callers that retain a complete output, either permit only BUFFERED or enforce MAX_HTTP_REQUEST_DEFERRED_BYTES cumulatively, advertise that total-output limit, and fail closed while releasing the session on overflow.
Verify: For a one-byte request, emit valid chunks beyond the platform payload cap before Finish; the chain must terminate at the cap instead of accepting and concatenating the output.
Agent context
- Location:
crates/openshell-supervisor-middleware/src/request.rs:420 - Agent path:
evaluate_described_with_policy_admittedretains these units and concatenates them for post-transform policy evaluation. - Ownership: The prior unary replacement was bounded by the stage payload limit.
There was a problem hiding this comment.
Resolved GATOR-96ff9454-02 in 284a896.
Change: Complete-body callers now cap cumulative retained STREAM output at MAX_HTTP_REQUEST_DEFERRED_BYTES, advertise the correct total-output ceiling, and release the session with a fail-closed overflow. Inter-stage output is also re-chunked to the next stage's unit limit.
Verification: mise run test and mise run ci pass, including all middleware and network suites.
| denial.reason_code.as_deref(), | ||
| ), | ||
| denial: Some(denial), | ||
| diagnostics: Box::default(), |
There was a problem hiding this comment.
gator-agent
Warning — GATOR-96ff9454-03 · Preserve denial telemetry
Summary: When request or response middleware rejects a body with valid findings, metadata, and a reason code, the traffic is blocked but the new conversion paths discard those diagnostics and omit the rejecting invocation. Reports from earlier completed request stages can also be lost when a later stage rejects. Operators and alerting systems then cannot see the audit-safe reason for the security-relevant denial.
Fix: Collect validated rejection diagnostics before constructing the failure, append the explicit Reject or BlockDelivery invocation, and merge reports from already completed request stages into later-stage failure diagnostics.
Verify: Have BUFFERED request and response middleware return HttpReject with content_match, one finding, and metadata; the returned failure and network events must retain all three plus the denial invocation.
Agent context
- Location:
crates/openshell-supervisor-middleware/src/request.rs:1533 - Sibling paths: response body rejection also validates then discards its diagnostics; a later request-stage error drops earlier reports.
- Ownership: The prior request and response paths retained denial diagnostics and invocation state.
There was a problem hiding this comment.
Resolved GATOR-96ff9454-03 in 284a896.
Change: Request and response denials now retain normalized reason codes, findings, metadata, earlier-stage reports, and explicit Reject/BlockDelivery invocations. Later stage transport/protocol failures retain completed reports as well.
Verification: request::tests::rejection_preserves_diagnostics_and_denial_invocation, cargo test -p openshell-supervisor-middleware, mise run test, and mise run ci pass.
| protocol_error: None, | ||
| })), | ||
| }; | ||
| let _ = tokio::time::timeout(SESSION_END_TIMEOUT, self.sender.send(event)).await; |
There was a problem hiding this comment.
gator-agent
Warning — GATOR-96ff9454-04 · Deliver correctly classified terminal events
Summary: Completing an HTTP middleware stage only queues SessionEnd locally, then drops both gRPC halves together. That can cancel the HTTP/2 stream before remote middleware receives the terminal event. Body rejection is also reported as MIDDLEWARE_FAILURE instead of MIDDLEWARE_DENIAL. Remote services can miss cleanup and record ordinary denials as service failures.
Fix: Use the established WebSocket lifecycle: own the transport during end, send SessionEnd, half-close the request sender, and retain and drain the response stream within SESSION_END_TIMEOUT. Choose MIDDLEWARE_DENIAL when the failure contains an explicit denial, while preserving cancellation and protocol reasons.
Verify: A tonic service that waits for SessionEnd must observe NORMAL after success and MIDDLEWARE_DENIAL after body rejection before the supervisor call returns.
Agent context
- Location:
crates/openshell-supervisor-middleware/src/request.rs:163 - Sibling path: The response transport has the same queue-and-drop behavior.
- Ownership: The replaced response transport and current WebSocket transport half-close and drain specifically to prevent cancellation from discarding the event.
There was a problem hiding this comment.
Resolved GATOR-96ff9454-04 in 284a896.
Change: HTTP transports now send SessionEnd, half-close the request side, and drain the response stream within SESSION_END_TIMEOUT. Success, explicit denial, failure, and dropped/cancelled sessions use their corresponding terminal reasons.
Verification: whole_body_stream_can_delay_output_while_input_is_active asserts delivery of NORMAL; the complete network suite, mise run test, and mise run ci pass.
pimlock
added
the
gator:in-review
…equest-middleware/pimlock Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
pimlock
left a comment
pimlock
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Thanks @pimlock. I reviewed the new preflight-only binding commit and the author-only delta after the merge from main; the narrower feature is documented and introduces no new blocking finding, but it does not address the four open runtime-contract obligations from the initial review.
Action required: @pimlock, please address the four existing inline findings and push an updated head for another focused follow-up review.
Blocking findings:
- No new blocking findings
Carried findings:
GATOR-96ff9454-01: restore the aggregate HTTP middleware chain deadlineGATOR-96ff9454-02: bound retained STREAM output in complete-body callersGATOR-96ff9454-03: preserve denial diagnostics and invocation telemetryGATOR-96ff9454-04: reliably deliver and correctly classify terminal events
Gator metadata
- Validation: Maintainer-authored work implementing linked issue #3307 and the approved middleware protocol direction
- Docs: Fern middleware and gateway documentation cover the preflight-only behavior
- Checks: Current-head branch checks are still running; E2E and GPU E2E gate statuses are present and green
- E2E: Required for middleware and network-proxy behavior; current gate status is green, but review blockers remain
- Head SHA:
4b19df7873dfa258f332fa08dea2838041321a5d - Base SHA:
484f0768fc6a0d93e0a2be295c1679aed24e18a9 - Merge base SHA:
484f0768fc6a0d93e0a2be295c1679aed24e18a9 - Patch ID:
a2d4ff4e7ecd75c5471525e186b25972258d9222 - Gator payload:
9 - Review mode:
follow_up - Previous reviewed SHA:
96ff9454396dbe362682ed3e206a40a7764d1120 - Review budget exhausted: no
- Maintainer decision required: no
- Next state:
gator:in-review
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
pimlock
left a comment
pimlock
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Thanks @pimlock. I checked the four fixes claimed for 284a8965f against the author-only delta. The patch materially improves each area, but each original runtime-contract obligation still has a reachable residual case, so none of the four Gator threads can be resolved yet.
Action required: A maintainer must decide whether these four residual obligations remain required or explicitly waive the identified behavior. If they remain required, @pimlock should address the cases below before another review.
Blocking findings:
- No new blocking findings
Carried findings:
GATOR-96ff9454-01: the complete-body compatibility loop still creates a fresh chain deadline for each entry instead of sharing one aggregate 30-second deadline.GATOR-96ff9454-02: complete-body STREAM collection now enforces the cumulative cap, but preflight still advertisesmax_total_output_bytesas unbounded.GATOR-96ff9454-03: malformed response-stage diagnostics, trailer mutations, and similar post-exchange validation failures can still discard earlier reports and the failing invocation.GATOR-96ff9454-04: response-body failure exits still fall throughDropand reportCancellationrather thanMiddlewareFailure; terminal send and drain also use separate timeout budgets.
Gator metadata
- Validation: Maintainer-authored work implementing linked issue #3307 and the approved middleware protocol direction
- Docs: Fern middleware and gateway documentation are updated
- Checks: Current-head branch checks are still running; Helm, Trivy, E2E, and GPU E2E required statuses are green
- E2E: Required for middleware and network-proxy behavior; current gate statuses are green, but review blockers remain
- Head SHA:
284a8965f6ffdcc07d4f2bf1f0747eeb22394649 - Base SHA:
484f0768fc6a0d93e0a2be295c1679aed24e18a9 - Merge base SHA:
484f0768fc6a0d93e0a2be295c1679aed24e18a9 - Patch ID:
910d88d908d4120d9825f7e0ce478bd42b0911f1 - Gator payload:
9 - Review mode:
follow_up - Previous reviewed SHA:
4b19df7873dfa258f332fa08dea2838041321a5d - Review budget exhausted: yes — this is the third finding-bearing round
- Maintainer decision required: yes — four concrete carried obligations remain unresolved
- Next state:
gator:blocked - Blocked reason:
review_convergence_decision_required
pimlock
added
gator:blocked
2 participants
Summary
Replace the WIP HTTP middleware body protocol with the approved two-mode, fail-closed contract. Request middleware now selects bounded in-memory
BUFFEREDprocessing or independent duplexSTREAMprocessing, while response middleware initially offersBUFFEREDonly. A binding can also advertise no body modes for preflight-only header mutation or denial; onContinue, the body and framing pass through that stage unchanged. OpenShell does not retain STREAM recovery copies or spool middleware bodies to disk.This is the base protocol/runtime PR in a three-PR stack:
The existing inline proxy SigV4 implementation remains in this base PR. Git signing is intentionally excluded.
Related Issue
Part of #2431
Closes #3307
Follow-up to #2426 and #3074.
Changes
EvaluateHttp(stream HttpEvent) returns (stream HttpResult)services with explicit HTTP protocol-version and body-mode capability negotiation.BUFFEREDandSTREAM; remove ownership acknowledgements, sequence/ACK/replay state, skip semantics, and proxy-managed middleware spooling.OutputStartuntil it has consumed the full request.POST_CREDENTIALSphase and reserve its enum value for wire safety.Review hardening in
284a8965f:SessionEndevents, includingNORMAL,MIDDLEWARE_DENIAL,MIDDLEWARE_FAILURE, and cancellation paths.output_body_byteswithContent-Lengthframing and keep HTTP/1.0 requests on a fully collected path.The pre-existing SigV4 behavior for bodyless requests without
x-amz-content-sha256remains intentionally deferred to the stacked SigV4 middleware PR, where signing behavior is owned.Design artifacts
Testing
mise run pre-commitmise run testmise run cicargo test -p openshell-supervisor-middleware(34 passed)cargo test -p openshell-supervisor-network(1,326 passed, 2 ignored; LocalStack tests remain opt-in)OutputStartdenial, and SigV4/body-middleware rejection before reading the request bodymise run e2eexercised the relevant proxy paths; its aggregate run later hit shared Docker runtime contention inprovider_readiness, which passed when rerun in isolation withOPENSHELL_E2E_DOCKER_TEST=provider_readiness mise run e2e:rustOne initial
mise run testattempt hit the existing timing-sensitive plaintext MCP proxy test under maximum parallel load. The exact test passed immediately in isolation and then passed in the clean fullmise run testandmise run cireruns.Checklist