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
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>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
This comment has been minimized.
This comment has been minimized.
…equest-middleware/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>
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
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
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