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>
The trailer mutation rules added in 9f7a04a landed on HttpInputChunk.data and inside the HttpDenial comment. Move them onto HttpBufferedResult.trailer_mutations and HttpFinish.trailer_mutations, and restore the HttpDenial comment. Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
…e can inspect bodies HTTP session hooks offer no body mode for a response with a content coding other than identity. The sandbox controls Accept-Encoding, and common clients (requests, httpx, fetch) send gzip by default, so most compressed responses reached response stages uninspectable: a guard either blocked them with cannot_inspect or let them through unread. When the response chain has an HTTP session hook stage that may select a body mode, the relay now replaces every Accept-Encoding field with identity. It does so as the last header step on both the buffered and the streamed request path, after request-stage mutations and before credential injection, so a request stage cannot undo it. Request middleware still sees the sandbox's value. v1 hooks and bindings with max_payload_bytes 0 are unchanged. An encoded response now means the upstream ignored identity. The e2e upstream gains a /compressible route that gzips when asked, and the workload asks for gzip as common clients do. Without the change the guard answers 403 cannot_inspect; with it, the response is redacted. Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
A response stage that needs the request's headers, such as a guard that judges a response against what was asked, had to keep per-request state keyed by request_id and route both streams to the same replica. HttpResponsePreflightHead.request_headers now carries the request headers as the request middleware chain forwarded them: after every request stage's mutations, including a streamed body's late mutations, and before OpenShell injects credentials or replaces Accept-Encoding. They have the omissions and limits of the request preflight's headers, and a value that contains a credential placeholder reads [REDACTED], as the response target's query already does. The relay hands response middleware a request that may already carry token grants, under header names the request-header sanitizer does not know, so the head is recorded before injection: by HttpMiddlewareExchange when request middleware allows the request, and before prepare_inspected_request on the inspected relay path. A live body applies its late mutations to that record when its head commits, before the upstream can answer. A response chain with no recorded head fails closed with middleware_cannot_inspect, as do request headers over the head limits. The response preflight's context comment now says request_id matches the request's preflights. Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
HTTP session hook heads already omit Authorization, Cookie, and other credential headers, but a credential placeholder in a custom header, such as X-Api-Key: openshell:resolve:env:API_KEY, reached request stages as-is and response stages as [REDACTED]. Whether a stage saw a placeholder depended on which header carried it. A request header whose value carries a credential placeholder, including one inside a Basic credential, is now a credential header for HTTP session hooks. The request preflight, begin, and the response preflight's request_headers omit every value of that name, and the pipeline rejects a stage mutation that writes or removes it with header_mutation_protected_header, so a stage cannot replace or drop a credential reference it cannot see. The [REDACTED] sentinel is gone. Like Envoy's ext_proc forwarding rules, OpenShell drops headers rather than masking their values; a typed list of withheld names can be added later if a stage needs to know they were present. v1 HTTP hooks keep their released behavior and still receive these headers. Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Resolve conflicts: - e2e/rust/Cargo.lock: take main's lockfile and add the flate2 dev-dependency entries at the versions this branch already pinned. Adapt to main: - Main's #4397 and #4684 report the inspected transport's scheme to middleware and OCSF. apply_middleware_chain_with_request_id_and_delivery, which the relays use for HTTP session hooks, still passed "https", so a plaintext request reached request stages as https. It now passes ctx.request_scheme(), and the scheme test covers that entry point too. - The request-body client timeout event uses ctx.ocsf_url() instead of a fixed http URL. Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
The hook let middleware allow connections OpenShell cannot inspect (tls: skip, h2c, unsupported tunnels, raw TCP, SQL passthrough) by metadata alone. No published middleware uses it, and the mixed-port cases it served are better handled by selector changes, so remove it before it ships. Uninspectable traffic goes back to the chain-level on_error rule: a fail_closed entry denies the connection, and an all-fail_open chain relays it with a bypass finding. That fail_open exception is no longer deprecated. The tls: skip overlap rule returns to shared policy validation, so supervisors enforce it again alongside the gateway. The tls: skip gate still runs before 200 Connection Established, so a denial is a plain 403. The proto service, operation, phase, and enums are deleted rather than reserved because they were never released. Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Middleware selectors match hostnames on every port, but a pattern such as api.example.com:443 parsed as a hostname that never matches, so the entry was silently never selected. Reject ':' in include and exclude patterns at gateway and supervisor validation instead. IPv6 literals are still accepted. Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
This comment was marked as outdated.
This comment was marked as outdated.
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Rejecting every ':' outside a complete IPv6 literal also rejected IPv6 globs such as 2001:4860:*, which selectors have matched since they shipped and network endpoints still accept. Treat a pattern containing ':' as IPv6 text instead, and reject only the shapes of a host:port authority: bracketed literals, characters IPv6 text cannot contain, dots before the last ':', a single ':' followed by digits, and invalid literals without wildcards. Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
|
Re: compatibility review, finding 1 (IPv6 selector globs) Fixed in 97f049c. Middleware selector patterns that contain The check still rejects port-shaped patterns, which never matched any host:
Gateway validation and the supervisor's local-file policy path share this check. Tests cover the accepted and rejected forms, and the schema and middleware configuration docs now say to write IPv6 addresses without brackets. |
matthewgrossman
left a comment
matthewgrossman
left a comment
There was a problem hiding this comment.
Review of head 97f049c from a multi-agent review panel (security, correctness, API/UX, and scope reviewers, plus a verification and triage step). I checked the two findings that change behavior (v1 response context and the IPv6 selector check) against the code by hand. The reviewers read the code but didn't build it or run tests.
Summary: no blocking issues. The security reviewer found nothing. Correctness covered the stage pipeline, buffered and stream modes, upload hold-back, response reuse, and selector and hook-rule validation, and found no defects beyond the smaller items below.
Worth fixing in this PR
- v1 response hooks now receive
originating_process, which changes v1 behavior (l7/middleware.rs:286). - Hex-only patterns like
cafe:*pass the new selector check as IPv6 globs (openshell-policy/src/middleware.rs:208). - Unreachable
RequestTimeoutbranches return a generic error instead of a 408 (l7/relay.rs:2808). - Docs and skill:
request_timeoutis missing from the logging docs, the skill's troubleshooting rows use internal reasons instead of the responsefailure_code, and thehost:portupgrade note isn't in the published docs.
Follow-up (fine as a separate issue)
- The plain-HTTP forward proxy still caps hooked uploads at 4 MiB (
proxy.rs:1828).
Considered and not raised: splitting the PR, including moving the host:port selector change into its own PR. The description presents it deliberately, with an upgrade note, so that's your call.
Details are in the inline comments.
| workspace: ctx.workspace.clone(), | ||
| originating_process: None, | ||
| }, | ||
| request_context: middleware_request_context(ctx, &self.request_id), |
There was a problem hiding this comment.
v1 response hooks now receive originating_process
response_relay used to build RequestContext { originating_process: None }. It now calls middleware_request_context, which fills in the binary path and ancestors. That context reaches the v1 HttpResponsePreflightInput (rest/http_response.rs:424), so v1 response services receive process data they didn't before. http_response_middleware_relay changed the same way. The v1 request path still sends None (openshell-supervisor-middleware lib.rs:2127), so v1 request and response hooks now see different context.
The description says v1 hooks keep working unchanged. Either keep None for v1 response chains and fill it in only for session-hook chains, or call out the v1 change in the PR and docs.
There was a problem hiding this comment.
Went the other way and filled it in everywhere: v1 request hooks and WebSocket preflights now get originating_process too, so all hooks see the same caller. This also closes #4269. A deep ancestor chain is trimmed (farthest first) to stay within the 4 KiB context limit instead of failing closed. The PR description and docs call out the v1 change. 25dc2a5
| selectors match hosts on every port and cannot include one" | ||
| .into()); | ||
| } | ||
| if !pattern.chars().all(|ch| { |
There was a problem hiding this comment.
Hex-only word:* selectors pass as IPv6 patterns
A pattern like cafe:* or beef:* contains only hex digits, : and *. It passes the character check, isn't caught by the port_suffix or . checks, and skips the Ipv6Addr parse because it contains a glob character. It is therefore accepted as an IPv6 pattern, although it was most likely meant as host:port, the case this validation exists to reject. Such an entry still never matches anything, so it's harmless, but it's the silent no-match the new rule aims to prevent.
Possible fix: require at least two : characters, or a leading ::, for glob patterns, and add tests for cafe:* and similar inputs.
There was a problem hiding this comment.
Fixed by dropping IPv6 globs from middleware selectors. : is now accepted only in a complete IPv6 address, so cafe:*, beef:443, and 2001:db8:* are all rejected, with tests. We accept the break: middleware attachment is done by domain name in practice, and a glob heuristic can't tell cafe:* from a mistyped host:port. The upgrade note covers it. 702284c
|
|
||
| <ParamField path="endpoints.include" type="string[]" required={true}> | ||
| Destination hosts to run the middleware for. Uses the same host patterns as network policy endpoints. | ||
| Destination hosts to run the middleware for. Uses the same host patterns as network policy endpoints. A pattern matches a host on every port and cannot include a port. Write IPv6 addresses and patterns without brackets, such as `2001:db8:*`. |
There was a problem hiding this comment.
Docs: add the host:port upgrade note here
The PR description has a good upgrade note about stored policies with host:port selectors now failing validation, but the published docs only state the new rule. Consider a sentence below endpoints.include: "Policies written for earlier releases that include a port, such as api.example.com:443, now fail validation; remove the port. Such entries never matched traffic."
There was a problem hiding this comment.
Added under endpoints.include, also covering IPv6 globs, which are now rejected. 702284c
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>
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