Repository navigation
Conversation
|
🌿 Preview your docs: https://nvidia-preview-pr-4359.docs.buildwithfern.com/openshell |
|
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. |
pimlock
changed the title
pimlock
changed the title
pimlock
changed the title
|
/ok to test 7f09efe |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
pimlock
added
the
gator:in-review
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
pimlock
added
gator:blocked
Add EvaluateHttp, a streaming HTTP middleware protocol with BUFFERED and STREAM body modes, next to the released HTTP protocol 1, which keeps its engines unchanged and is deprecated for removal in 0.2.0. - One EvaluateHttp stream per HTTP message and stage, or per uninspectable connection. A preflight selects continue, inspect, or reject; late header mutations apply before the head commits. - A service implements one HTTP protocol, chosen per binding with http_protocol_version, and a protocol 2 service requires the openshell.supervisor-middleware.http-v2 capability. The gateway delivers each service's protocol to supervisors. - Protocol 2 is always fail-closed. The gateway rejects fail_open on protocol 2 services and policies whose overlapping selectors mix protocols; a mixed chain fails closed at runtime. - Protocol 2 middleware decides about uninspectable traffic (tls: skip, h2c, unsupported tunnels, raw TCP, SQL passthrough) at a preflight, and is exempt from the static tls: skip conflict rule. - Request bodies stream to the upstream unless a later step needs the whole body; responses stream with chunked framing or buffer before commit. - Migrate the content guard example to protocol 2, add a Docker e2e suite, and document the protocol. Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Replace the http-v2 capability and per-binding protocol fields with new binding operations, and stop delivering the protocol to supervisors. - HTTP_REQUEST_V2 and HTTP_RESPONSE_V2 select HTTP protocol 2. Released gateways and supervisors already reject unknown operation/phase pairs at Describe, so the capability, extension_metadata_with_requirements, and MiddlewareBinding.http_protocol_version are removed. - Drop supported_http_body_modes. The preflight offers every mode the message is eligible for, and the stage picks one or continues or rejects. A protocol 2 binding with a zero payload limit is offered no body modes. - Move the tls: skip conflict rule out of the shared policy validator into the gateway's safety validation, which knows each service's protocol. This removes the delivered SupervisorMiddlewareService protocol field, the process-wide protocol 2 set in openshell-policy, and the supervisor's undescribed-service tracking. An undescribed entry follows on_error, which the gateway requires to be fail_closed for protocol 2 services. Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Replace EvaluateHttp with EvaluateHttpRequestV2 and EvaluateHttpResponseV2, one RPC per binding operation, as for HTTP protocol 1 and WebSocket. Both keep the shared HttpEvent and HttpResult messages, so the stage pipeline is unchanged; OpenShell picks the RPC from the preflight subject. Traffic OpenShell cannot inspect stays on the request RPC. Only services that bind HTTP_REQUEST_V2 decide about it, so only they are exempt from the gateway's tls: skip conflict rule; a response-only protocol 2 service keeps the rule and its fail-closed entry denies such traffic at runtime. Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Rename "HTTP protocol 1/2" to v1 and v2 HTTP hooks across code, proto comments, docs, skills, and the example. The docs page moves to extensibility/supervisor-middleware/http-hooks-v2, the protocol2 modules become http_v2, HttpProtocol becomes HttpHookVersion, and the mixed-chain reason becomes middleware_hook_versions_mixed. Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Rename the streaming HTTP hook RPCs to EvaluateHttpRequestSession and EvaluateHttpResponseSession, and drop the unreleased HTTP_REQUEST_V2 and HTTP_RESPONSE_V2 operations. A service keeps its HTTP_REQUEST and HTTP_RESPONSE bindings and selects HTTP session hooks for all of them by listing openshell.supervisor-middleware.http-session in its extension required_capabilities. Released gateways and supervisors already reject unmet required capabilities at Describe, so they never call such a service through the v1 RPCs. Registration rejects a manifest that supports the capability without requiring it. Mark the v1 HTTP hook RPCs, the HttpResponsePreReturn service, and the types only they use deprecated in the protobuf contract. Rename the docs page, module, and e2e suite to HTTP session hooks, and update RFC 0009 so capability-gated contracts and removing deprecated elements in a minor release match the release stability policy. Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
v1 HTTP request hooks and WebSocket preflights now receive RequestContext.originating_process like v1 response hooks and HTTP session hooks, so every hook sees the same caller. A deep ancestor chain is trimmed, farthest ancestor first, to keep the context within its 4 KiB limit instead of failing the evaluation. Closes #4269 Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
IPv6 globs such as cafe:* cannot be told apart from a mistyped host:port, so the heuristic that accepted them let some port patterns through as IPv6 text that never matches. Middleware selectors now accept ':' only in a complete IPv6 address. Docs add the upgrade note for policies with a port or an IPv6 wildcard. Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
These paths read the whole body before middleware runs, so a request timeout should not occur. If it does, send the same 408 request_timeout as the REST path instead of failing the relay with a generic error. Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
List request_timeout among the sandbox error codes, and name both the OCSF reason and the failure_code a sandbox sees in the troubleshooting skill. Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Compatibility review (advisory)New or updated breaking changes; see the full report for all findings. 1. policy — Risk explicitly acceptedMiddleware endpoints.include/exclude previously accepted IPv6 globs such as
Review details
Advisory review; publication gates are unchanged. |
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
BUFFERED becomes WHOLE_BODY and STREAM becomes DUPLEX, before the HTTP session hooks ship. WHOLE_BODY matches the v1 WHOLE_BODY_BYTES name, and DUPLEX leaves room for a later PER_CHUNK mode with one result per input chunk, like Envoy ext_proc STREAMED, without two modes both called streaming. Renames the proto enum values, HttpWholeBodyMode, HttpDuplexMode, HttpWholeBody, HttpWholeBodyResult, the whole_body and whole_body_result fields, and max_whole_body_bytes, along with the Rust names, the content-guard body_mode values, tests, and docs. Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
The capability that selects the streaming HTTP RPCs is now openshell.supervisor-middleware.http-hooks. "Session" read as a cookie-style HTTP session and clashed with the WebSocket session RPC. Docs, comments, and errors call the new contract "HTTP hooks" and the deprecated contract "legacy HTTP hooks" instead of "v1". HttpHookVersion and ChainHttpHookVersion rename V1 to Legacy, with the audit label legacy_http_hook; the new contract's label is http_hook. Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Finish the http-hooks rename so no HTTP hook name still says "session":
- Service HttpSessionMiddleware is now HttpHooks; Http{Request,Response}
Session{Event,Result,Preflight} messages are now Http*Hook*.
- http_session modules, the e2e test, and the mise task and CI suite are
now http_hooks / middleware-http-hooks.
- HttpHookVersion and ChainHttpHookVersion rename Session to Current;
HeaderAuthority renames Session{Request,Response} to Hook*.
- HttpRequestSession and HttpResponsePipelineSession are now
HttpRequestPipeline and HttpResponsePipeline, carried in a `pipeline`
field; MAX_CONCURRENT_HTTP_MIDDLEWARE_SESSIONS is now ..._MESSAGES.
Shared stream names (MiddlewareSessionEnd, session_capacity_exhausted)
and WebSocket session names are unchanged.
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
pimlock
removed
the
gator:blocked
"Messages" and "streams" read as gRPC messages and streams. The two
budgets are buffering slots (preflight and WHOLE_BODY) and streaming
slots (DUPLEX or deferred stages), as the docs already call them:
- MAX_CONCURRENT_HTTP_MIDDLEWARE_MESSAGES -> MAX_HTTP_HOOK_BUFFERING_SLOTS
- MAX_CONCURRENT_HTTP_MIDDLEWARE_STREAMS -> MAX_HTTP_HOOK_STREAMING_SLOTS
- http_{hooks,stream}_admission -> http_{buffering,streaming}_admission,
with matching reserve helpers and debug fields.
The streaming budget comment now says deferred stages also take a slot,
matching may_stream(). The docs limits row counts HTTP requests and
responses instead of messages.
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
matthewgrossman
left a comment
matthewgrossman
left a comment
There was a problem hiding this comment.
Follow-up review of 1c982fa, from the same multi-agent review panel as the round on 97f049c. The panel inferred your responses from your replies and commits, and then had the original reviewers re-check each claimed fix.
All six fixes confirmed:
- IPv6-glob/
host:portselectors are rejected, with tests and an upgrade note (702284c). - The JSON-RPC and GraphQL
RequestTimeoutpaths return 408 (7714bdb). request_timeoutis documented, and the skill pairs each OCSF reason with itsfailure_code(0732227).originating_processis filled consistently for every hook (25dc2a5).
Still open as a follow-up: forward-proxy streaming for hooked uploads, tracked in #5086.
No new findings in the PR's own changes. Most of this range is merged-in main, so the reviewers focused on the PR's own commits (the http_session → http_hooks rename and the body-mode renames). They didn't build or run tests.
Contract-level comments are on #5087, as the PR description asks; any agreed changes would land here.
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
State how zero-limit HTTP hooks bindings interact with the operator payload limit, align the HTTP/1.0 DUPLEX rule with the proto, document the 403 for a DUPLEX request denied after upstream output starts, and make failure_code wording consistent across the docs. Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
…X request denial Pin that a zero-limit HTTP hooks binding is excluded from the operator payload limit comparison, and that a DUPLEX request denied after its output started upstream still gets the canonical 403 and an unterminated upstream body. Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
…ware-protocol-2/pimlock Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com> # Conflicts: # crates/openshell-supervisor-network/src/l7/middleware.rs # crates/openshell-supervisor-network/src/proxy.rs
Compatibility review (advisory)New or updated breaking changes; see the full report for all findings. 1. policy — Risk explicitly accepted
Review details
Advisory review; publication gates are unchanged. |
Rename HttpHooks.EvaluateRequest and EvaluateResponse to EvaluateHttpRequestHook and EvaluateHttpResponseHook, and document in supervisor_middleware.proto that RPC names must be unique across the package's services, with the naming pattern for future hook services. Python and Go servers name handler methods after the RPC alone, so one object implementing several services needs distinct names. Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
|
Pushed bc90d94: renamed the HTTP hook RPCs so their names are unique across the middleware services.
Why: a middleware implements Rule, documented at the top of Wire behavior is otherwise unchanged. The rename also updates the tonic trait methods in |
matthewgrossman
left a comment
matthewgrossman
left a comment
There was a problem hiding this comment.
Follow-up review of bc90d94 from the same multi-agent review panel, covering only your changes since 1c982fa (the RPC rename, the zero-limit and late-DUPLEX tests and docs, and the merge-conflict resolutions). The rename is applied consistently and the conflict resolutions keep the hook logic the same. Two small suggestions are inline. The forward-proxy follow-up stays tracked in #5086.
| ); | ||
| // The upstream request started with a chunked body, which the denial | ||
| // leaves unterminated. | ||
| assert_eq!(capture.requests.len(), 1); |
There was a problem hiding this comment.
Nit: the test doesn't check that the upstream body is left unterminated
The comment above says the denial leaves the chunked upstream body unterminated, but the assertions only check the transfer-encoding: chunked header. Consider also asserting that the captured upstream request doesn't end with the terminating chunk, for example assert!(!capture.requests[0].ends_with("0\r\n\r\n")), so a regression that completes the upstream body would fail this test.
There was a problem hiding this comment.
Fixed in 8689c3e: the test now also asserts the captured upstream request does not end with the terminating chunk (0\r\n\r\n).
| )); | ||
| if invocation.outcome == HttpStageOutcome::Deny { | ||
| events.push( | ||
| openshell_ocsf::DetectionFindingBuilder::new(ocsf_ctx()) |
There was a problem hiding this comment.
Nit: this finding is missing .action(ActionId::Denied)
The legacy openshell.middleware.http_response_blocked finding in l7/rest/http_response.rs now sets .action(openshell_ocsf::ActionId::Denied) (added on main in #4789). This HTTP hooks version of the same finding doesn't, so the two hook versions emit different OCSF for the same decision. Consider adding it here.
There was a problem hiding this comment.
Fixed in 8689c3e: the HTTP hooks http_response_blocked finding now sets ActionId::Denied, with a unit test. The same gap existed on the hooks after-commit http_response_failure finding, so it now sets Denied when a stage stopped delivery, matching the legacy http_response_middleware_failure_event. Upstream failures after commit stay without an action, as in the legacy path.
Set ActionId::Denied on the HTTP hooks http_response_blocked finding and on the after-commit delivery failure when a stage stopped delivery, so both hook versions emit the same OCSF for the same decision. Assert that a DUPLEX request denied after upstream start leaves the chunked upstream body unterminated. Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Summary
Introduce HTTP hooks: streaming HTTP request and response hooks that replace the legacy HTTP hooks.
Why
The legacy hooks were built for small, buffered checks. The use cases we want next, such as signing a Git push or a large AWS request, need the whole body regardless of size and need to change the body and its headers together.
EvaluateHttpRequestis unary: the whole body in one message, at most 4 MiB, under one RPC deadline. A larger request fails withrequest_body_over_capacity. HTTP hooks can stream a body with no total size or time cap.Content-Lengthor a SigV4 payload hash. An HTTP hook stage can send header mutations after it has read the complete body.HttpResponsePreReturnis lockstep: each body unit gets exactly one result, and a service can't hold bytes for a later unit. In DUPLEX mode, input and output chunks are independent, so a service can redact across chunk boundaries or re-frame a stream.Why not extend the legacy hooks. A unary RPC can't become a stream without breaking every deployed service. Lockstep is a semantic guarantee, not a field, so relaxing it on the same RPC would change behavior that
buf breakingcan't detect. Older peers would also ignore new fields and call an extended service with legacy semantics. A separate service gated by a required capability makes older peers refuse it, and the legacy hooks keep working unchanged until they are removed.What this PR adds
HttpHooks.EvaluateRequestandHttpHooks.EvaluateResponse, a new service served on the same endpoint asSupervisorMiddleware, with one stream per HTTP message. A preflight decides whether to continue, inspect the body, reject a request, or block response delivery withblock_delivery. A stage inspects the body in one of two modes: WHOLE_BODY, one complete body and one result, or DUPLEX, independent input and output streams.HTTP_REQUESTandHTTP_RESPONSEbindings and listsopenshell.supervisor-middleware.http-hooksin its extensionrequired_capabilities. Gateways and supervisors have rejected unmet required capabilities atDescribesince v0.1.0, so older peers refuse the service instead of calling it through the legacy RPCs. No RPC, message, or enum value carries a version.fail_open. HTTP hooks are always fail-closed, which simplifies the UX, the API, and the code.EvaluateHttpRequest,HttpResponsePreReturn, and the types only they use are markeddeprecatedin the protobuf contract and are up for removal in the next version bump (0.2.0). Migration notes are in the new docs page. Both hook versions work side by side until then.Tip
The interactive walkthrough covers the motivation, the new API, version selection, and migration in more detail.
Related Issue
Part of #2431 and #3307.
Closes #4269.
Changes
openshell.supervisor-middleware.http-hookscapability. They reject a manifest that supports it without requiring it, because older peers would call that service through the legacy hooks.on_error: fail_open, and one chain can't mix hook versions.host:portselector never matched anything, so the entry was silently skipped. It now fails validation.RequestContext, including legacy and WebSocket hooks, now receivesoriginating_process.Release Notes
HTTP middleware can now use the new HTTP hooks, which stream request and response bodies of any size. A stage inspects the whole body (
WHOLE_BODY, up to 4 MiB) or streams it (DUPLEX, with no total size or time limit), and can change headers after reading the body. A service opts in by requiring theopenshell.supervisor-middleware.http-hookscapability. HTTP hook services are always fail-closed.The legacy HTTP hooks (
EvaluateHttpRequestandHttpResponsePreReturn) are deprecated and will be removed in 0.2.0.Upgrade note: middleware selectors can no longer contain a port. Such a selector never matched, so remove the port.
Testing
mise run pre-commitpassesmise run e2e:middleware-http-hookssuitebuf breakingagainst v0.1.2 reports no breaking changeValidation. Two prototypes on an earlier head of this PR exercise the API: Git commit signing streams a push larger than 4 MiB through an external signer that keeps the key outside the sandbox, and SigV4 signing signs the final request after HTTP hook stages transform its body.
Checklist