Skip to content

feat(middleware): introduce improved HTTP request/response hooks for the middleware - #4359

Open
pimlock wants to merge 25 commits into
mainfrom
feat/2431-http-middleware-protocol-2/pimlock
Open

pimlock wants to merge 25 commits into
mainfrom
feat/2431-http-middleware-protocol-2/pimlock

Conversation

@pimlock

@pimlock pimlock commented Oct 9, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Introduce HTTP session hooks, streaming HTTP request and response hooks that replace the v1 HTTP hooks. The v1 hooks were too limited: they could only inspect requests whose body fit in a buffer within our payload limit. The session hooks stream the body, so middleware can inspect messages of any size.

  • New streaming API. EvaluateHttpRequestSession and EvaluateHttpResponseSession, with one stream per HTTP message. A preflight decides whether to continue, inspect the body, reject a request, or block response delivery with block_delivery. Body inspection can be buffered or streamed. The request preflight also lets middleware allow or refuse traffic OpenShell cannot inspect, such as tls: skip tunnels.
  • Opt in with a capability, no new operations. A service keeps its HTTP_REQUEST and HTTP_RESPONSE bindings and lists openshell.supervisor-middleware.http-session in its extension required_capabilities. Gateways and supervisors have rejected unmet required capabilities at Describe since v0.1.0, so older peers refuse the service instead of calling it through the v1 RPCs. No RPC, message, or enum value carries a version.
  • No fail_open. The session hooks are always fail-closed, which simplifies the UX, the API, and the code.
  • v1 hooks are deprecated. EvaluateHttpRequest, HttpResponsePreReturn, and the types only they use are marked deprecated in 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

See interactive walk-through of main parts of this PR.

Related Issue

Part of #2431 and #3307.

Changes

  • Gateways and supervisors advertise the openshell.supervisor-middleware.http-session capability. Registration rejects a manifest that lists it as supported but not required, because older peers would accept that service and call v1 HTTP hooks. extension_protocol::http_session_middleware_metadata builds the metadata.
  • The gateway rejects on_error: fail_open for session hook services, exempts session hook request middleware from the tls: skip conflict rule, and rejects policies whose overlapping selectors would mix hook versions in one chain.
  • The content guard example moves to HTTP session hooks.
  • RFC 0009: contract versioning now describes capability-gated contracts and the removal of deprecated elements in a minor release, as RFC 0014 allows.
  • Docs: session hooks are documented under Supported Operations, with shared body modes and limits, a short legacy migration note, and updated links.

Testing

  • mise run pre-commit passes
  • Unit tests added/updated
  • E2E tests added/updated: new mise run e2e:middleware-http-session suite
  • buf breaking against v0.1.2 reports no breaking change

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)

@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown

@pimlock
pimlock marked this pull request as draft October 9, 2026 01:31
@copy-pr-bot

copy-pr-bot Bot commented Oct 9, 2026

Copy link
Copy Markdown

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 pimlock changed the title feat(middleware): add HTTP middleware protocol 2 feat(middleware): introduce improved HTTP request/response hook for the middleware Oct 9, 2026
@pimlock pimlock changed the title feat(middleware): introduce improved HTTP request/response hook for the middleware feat(middleware): add v2 HTTP request and response hooks Oct 9, 2026
@pimlock pimlock changed the title feat(middleware): add v2 HTTP request and response hooks feat(middleware): introduce improved HTTP request/response hooks for the middleware Oct 10, 2026
@pimlock

pimlock commented Oct 10, 2026

Copy link
Copy Markdown
Collaborator Author

/ok to test 7f09efe

@pimlock
pimlock marked this pull request as ready for review October 10, 2026 01:45

@pimlock pimlock left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gator-agent

PR Review Status

The initial code review found three blocking issues in the new HTTP session pipeline: response completion before the terminal verdict, response delivery after policy revocation, and incomplete headers at body-stage begin. The inline comments describe the fixes and deterministic regression cases.

Action required: @pimlock, address the three inline findings and push an updated head for a focused follow-up review.

Blocking findings:

  • GATOR-7f09efe2-01: Preserve detectable failure until the response middleware accepts the terminal result.
  • GATOR-7f09efe2-02: Check the current policy generation before every response write.
  • GATOR-7f09efe2-03: Include all preflight mutations in each body stage's begin head.

Carried findings: None.

Non-blocking suggestions:

  • Split size-expanding content-guard scanner output into chunks within the negotiated limit, including the final tail. A 64 KiB chunk of a expanded to [REDACTED] currently exceeds the output chunk limit and fails closed.
Gator metadata
  • Validation: Focused implementation of the streaming middleware work in #2431 and #3307; repository-admin author and no duplicate identified.
  • Docs: Fern HTTP Session Hooks page, migration guidance, related pages, and navigation updated.
  • Checks: Current-head Branch Checks, Helm Lint, Trivy Changes, and DCO pass; no merge conflict.
  • E2E: test:e2e is required for proxy, policy, and credential behavior. The existing E2E jobs were skipped without the label; runtime test dispatch remains after review fixes. GPU and Windows labels are not indicated by this patch.
  • Head SHA: 7f09efe20318880bd2d884c4aadb4a56f902d48d
  • Base SHA: 0ccc6b9a053fdaf47700e91c1b8ed858f2afed4f
  • Merge base SHA: 0ccc6b9a053fdaf47700e91c1b8ed858f2afed4f
  • Patch ID: 611a1c2a9287a1c7583b112c37822874f207e666
  • Gator payload: 11
  • Review mode: initial
  • Previous reviewed SHA: none
  • Review budget exhausted: no
  • Maintainer decision required: no
  • Next state: gator:in-review
  • Validation method: Static code review; no local tests run.

Comment thread crates/openshell-supervisor-middleware/src/http_session/pipeline.rs Outdated
@pimlock pimlock added the gator:in-review Gator is reviewing or awaiting PR review feedback label Oct 10, 2026
@pimlock

pimlock commented Oct 10, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the gator review of 7f09efe. The new head is 1a4a2bb, with one commit per finding:

Finding Commit
GATOR-7f09efe2-01: completion before the terminal verdict c2684e8. Also fixes the same gap on request uploads.
GATOR-7f09efe2-02: writes after policy revocation 0a492e2
GATOR-7f09efe2-03: incomplete Begin head a491a29
Non-blocking: expanded content guard output 1a4a2bb. STREAM output is split within the offered max_chunk_bytes. In the regression test, eight 秘密 terms expand to eight [REDACTED] markers under a 16-byte limit.

Each regression test fails on 7f09efe and passes now.

Verification:

  • mise run pre-commit passes.
  • buf breaking against v0.1.2 reports no breaking changes. The proto changes are comments only.
  • Unit tests pass for openshell-supervisor-middleware (129), openshell-supervisor-network (1561), and the content guard example (22).
  • mise run e2e:middleware-http-session passes.

@pimlock pimlock added the test:e2e Requires end-to-end coverage label Oct 10, 2026
@github-actions

Copy link
Copy Markdown

Label test:e2e applied for 1a4a2bb. Open the existing run and click Re-run all jobs to execute with the label set. The run will execute the standard E2E suite after building the required gateway, sandbox, and supervisor images once. The matching required CI gate status on this PR will flip green automatically once the run finishes.

@pimlock pimlock left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gator-agent

PR Review Status

Thanks @pimlock. I checked your fixes for terminal completion, policy revocation, and the body-stage Begin head, including the request-upload completion path and the new regression cases. All three findings are fixed, and their Gator threads are resolved. The focused follow-up also checked the content-guard chunk splitting and found no new blocking issues.

Action required: a maintainer must open the current-head Branch E2E Checks run and select Re-run all jobs now that test:e2e is applied. Its runtime jobs were skipped before the label was set; Gator cannot advance to pipeline monitoring until they actually start.

Blocking findings: None.

Carried findings: None; GATOR-7f09efe2-01, GATOR-7f09efe2-02, and GATOR-7f09efe2-03 are fixed.

Gator metadata
  • Validation: Streaming supervisor middleware work in #2431 and #3307; project validity established in the initial review.
  • Docs: Fern delivery guarantees and preflight-head ordering match the fixes; existing navigation remains appropriate.
  • Checks: Branch Checks are running; Helm Lint and Trivy Changes pass, DCO passes, and the PR has no merge conflict. Maintainer approval is still missing.
  • E2E: test:e2e applied; current-head runtime jobs were skipped and require Re-run all jobs. GPU and Windows labels are not indicated.
  • Head SHA: 1a4a2bb694bc60b0bcbc478057ef67594de5f540
  • Base SHA: 0ccc6b9a053fdaf47700e91c1b8ed858f2afed4f
  • Merge base SHA: 0ccc6b9a053fdaf47700e91c1b8ed858f2afed4f
  • Patch ID: 128cfed5631373ce935945cbce6c4849e2591b9f
  • Gator payload: 11
  • Review mode: follow_up
  • Previous reviewed SHA: 7f09efe20318880bd2d884c4aadb4a56f902d48d
  • Review budget exhausted: no
  • Maintainer decision required: no
  • Next state: gator:blocked
  • Blocked reason: test_dispatch_required
  • Validation method: Static independent review; no local tests run. Author-reported passing tests were considered alongside inspected regression code.

@pimlock pimlock added gator:blocked Gator is blocked by process or repository gates and removed gator:in-review Gator is reviewing or awaiting PR review feedback labels Oct 10, 2026
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>
A body stage captured the head right after its own preflight, so its
begin event lacked the preflight mutations of later stages even though
the delivered head carried them. Keep the final preflight head on the
pipeline and begin every stage on it plus earlier stages' late
mutations, as the HttpBegin contract states.

Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
A STREAM response stage can keep emitting output after it consumed the
upstream body, and the session response writer checked the policy
generation only when it committed the head. Output and the terminating
chunk then reached the client under revoked authorization, before the
final check. Check the generation before every write instead, and treat
the response as committed only once a byte was written.

Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
With a declared output length, a STREAM stage could emit every byte and
then reject at input_end. The relay had already written a complete
Content-Length message, so the sandbox saw a successful response, and
on uploads the upstream received a complete request it could act on or
answer.

Hold back what would complete the message until every stage returns its
terminal result: the final byte of a declared length, or the whole head
of a declared empty body. The chunked terminator already waited. An
empty response body that is rejected still gets the canonical 403.

Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
…imit

Redacting a term shorter than the marker makes STREAM output longer than
its input, so one output chunk could exceed the max_chunk_bytes the
preflight offered and fail the message closed. Split every output chunk
within that limit.

Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
@pimlock
pimlock force-pushed the feat/2431-http-middleware-protocol-2/pimlock branch from 1a4a2bb to f814a43 Compare October 13, 2026 15:44
… sandbox

Address load-test findings on HTTP session hooks:

- Give HTTP session hooks their own admission budgets: 64 slots for
  preflight and BUFFERED bodies, and 256 for STREAM bodies so long-lived
  server-sent events cannot starve short messages. A message waits for a
  slot within the chain deadline instead of failing at once, releases it
  when no stage inspects the body, and frees its BUFFERED slot when the
  last BUFFERED stage ends. Uninspectable decisions use the same budget.
- After an early middleware response (preflight rejection or failure,
  503, mid-stream failure, or 408), shut down writing and discard what
  the sandbox still sends, within time and byte bounds, so the sandbox
  reads the response instead of a connection reset.
- Fill originating_process (binary and ancestors) in request and
  response preflights, as the uninspectable preflight already did.
- Return a platform-owned failure_code from a closed set, and the failed
  stage's policy entry name, in 403, 502, and 503 failure bodies.
- Document raising gRPC message limits for 4 MiB BUFFERED bodies, and
  raise them in the content guard example.

Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Address the proto review of the HTTP session hooks contract:

- Split the shared HttpEvent/HttpResult into per-RPC messages
  (HttpRequestSessionEvent/Result and HttpResponseSessionEvent/Result)
  with direction-specific preflights, so a request stream cannot carry a
  response subject and the reverse. Body events and results stay shared.
- Add phase to both preflights, as the WebSocket preflight has.
- Add an explicit cannot_inspect result, valid wherever reject is, and
  drop the FAILED_PRECONDITION convention; every gRPC status now maps to
  middleware_unavailable.
- Offer no body mode for a request whose Content-Encoding is not
  identity (ENCODED), as for responses: middleware would only see the
  encoded bytes.
- Send declared_body_bytes, and apply OVER_LIMIT, only to stages before
  the first one that inspects the body, which may replace it.
- Remove the queue limits from HttpBodyLimits; they had no defined
  meaning for a service.
- Clarify comments: BUFFERED fails over its limit instead of falling
  back, finish requires output_start, and unavailable_body_modes is empty
  for a zero payload limit.
- Document that policies stored before a service changed hook version
  must be re-applied.

Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
@pimlock

pimlock commented Oct 14, 2026 •

Copy link
Copy Markdown
Collaborator Author

Thanks @matthewgrossman for the contract review. The fixes are in e8feab4, the new head.

# Finding Change
1 Compressed request bodies A request whose Content-Encoding isn't identity gets no body mode (ENCODED), as responses already did. The content guard returns cannot_inspect for it.
2 Mixing hook versions Not in this PR. Migrating openshell/regex to session hooks will follow in the same release. Wrapping v1 entries as BUFFERED stages would mean mapping the v1 response modes and honoring per-entry on_error inside a pipeline that always fails closed. Migration step 4 now says to re-apply stored policies after a service changes hook version.
3 One message pair for two RPCs Split into HttpRequestSessionEvent/Result and HttpResponseSessionEvent/Result. The request preflight's subject is a request or uninspectable traffic. The response preflight carries only the response head. The body messages stay shared.
4 No phase Both preflights carry phase.
5 Undefined queue limits Removed. idle_timeout is now field 3.
6 declared_body_bytes Only stages before the first one that inspects the body receive it, and OVER_LIMIT follows the same rule. The example's empty-body skip is now correct.
7 FAILED_PRECONDITION as the signal Replaced by an explicit cannot_inspect result, valid wherever reject is. Every gRPC status now maps to middleware_unavailable.
8 Comment nits Fixed. BUFFERED fails over its limit instead of falling back, finish requires output_start, and unavailable_body_modes is empty for a zero payload limit.

e62d4a7, pushed just before, addresses separate load-test findings:

Finding Change
Session cap rejects instead of queueing HTTP session hooks have their own budgets, separate from WebSocket: 64 slots for preflight and BUFFERED bodies, and 256 for STREAM bodies (more than twice the 96-stream relay cap), so open SSE responses can't starve short requests. A message waits for a slot until the 30 s chain deadline. It releases the slot after continue_without_body or a rejection, and frees its BUFFERED slot when the last BUFFERED stage ends.
Rejected uploads get an empty reply After any early middleware response (preflight rejection or failure, 503, mid-stream failure, or 408), the relay shuts down writing and discards input until the client closes, goes idle for 2 s, or reaches 5 s or 8 MiB. The forward proxy does the same.
Session preflights omit originating_process Request, response, and uninspectable preflights carry the binary and its ancestors. pid stays 0 because the relay context doesn't track it. v1 also sends no process (RFC 0009 says so on purpose), so v1 and WebSocket are unchanged.
4 MiB cap vs gRPC defaults The session hooks page says to raise gRPC receive and send limits to 4.5 MiB, and the content guard example does.
Failure reason in the sandbox response 403, 502, and 503 failure bodies carry failure_code from a closed, platform-owned set (middleware_cannot_inspect, middleware_timeout, stream_idle_timeout, middleware_unavailable, middleware_lifecycle_violation, body_over_limit, header_mutation_*, middleware_hook_versions_mixed), plus middleware when one stage failed. Service text never reaches the sandbox.

The main L7 response path built its middleware request context by hand
with originating_process set to None, so HTTP session hook response
preflights lacked the process that request preflights already carry.
Build it with middleware_request_context, as the other paths do.

Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
@github-actions

Copy link
Copy Markdown

Compatibility review (advisory)

New or updated breaking changes; see the full report for all findings.

1. rust-sdk — No explicit risk acceptance found

SupervisorMiddleware gained EvaluateHttpRequestSession and EvaluateHttpResponseSession, so tonic’s generated server trait now requires two additional stream types and two additional methods from every implementer.

  • Impact: Rust middleware and mock servers implementing openshell_sdk::raw::proto::middleware::v1::supervisor_middleware_server::SupervisorMiddleware no longer compile until they add all four generated items, even when they continue using only v1 hooks.
  • Action: For v0.1.5, place the session RPCs on a new protobuf service or otherwise preserve the old generated server trait, and defer any required trait expansion to a minor release with Rust-specific migration guidance.
  • PR: feat(middleware): introduce improved HTTP request/response hooks for the middleware #4359
Review details
  • Execution: complete
  • Candidate: PR #4359 (6231d339e147c55e5ef41f012a6e50b3c03ad318)
  • Baseline: 6546691d5e493343c7877573252e835d28bf9e23 (6546691d5e493343c7877573252e835d28bf9e23)
  • Reviewed: rust-sdk, python-sdk, go-sdk, typescript-sdk, api-behavior, cli, configuration, policy, helm, state

Advisory review; publication gates are unchanged.

Branch check run

…te docs

Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>

@matthewgrossman matthewgrossman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second-round review of the proto contract and the HTTP session hook pipeline (through 30b0f46).

The first-round items look addressed: request ENCODED (decided from the original head, so a Connection: content-encoding trick can't hide it), separate per-RPC event/result messages with block_delivery for responses, phase on both preflights, queue limits removed, declared_body_bytes/OVER_LIMIT scoped to stages before the first inspecting stage, explicit HttpCannotInspect (handled in classify_result and the uninspectable path), and the comment fixes. The rule that a chain can't mix hook versions is still a design choice, but it's documented now.

Remaining items are inline, plus one I couldn't anchor to a line in the diff:

  • Request Content-Encoding is still writable by middleware (nit). is_request_protected doesn't cover content-encoding, while the response side protects it. Not a bypass, because body-mode offers come from the original head, but a stage could relabel a body it replaced (e.g. add Content-Encoding: gzip over plaintext). Consider protecting it on requests too, or documenting the asymmetry next to the HttpPreflightResult.header_mutations comment, which mentions only response coding headers.

deadline: Instant,
) -> Option<Pipeline> {
if pipeline.streams() {
let Some(stream) = self.reserve_http_stream(deadline).await else {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Waiting for a stream slot blocks preflight slots (low–medium, availability). admit_body keeps the message's preflight slot (slot, from the 64-slot budget) while it waits, up to the chain deadline, for a stream slot (256 total).

Scenario: 256 long server-sent-event streams are open (streamed LLM responses count). Then up to 64 more STREAM-selecting messages can hold every preflight slot while they wait. Short BUFFERED messages and uninspectable-connection decisions then can't start their preflight, and each fails with 503 after the chain deadline. That is the starvation the split budget was meant to prevent. Impact is limited to the one sandbox, since each sandbox runs its own supervisor.

Suggestion: drop the preflight slot before waiting (keep it only if a BUFFERED stage needs it), or try for the stream slot without waiting.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in cd0c390. A message takes a free stream slot right away when one is open. Otherwise it releases its preflight slot before waiting, and takes one back after the stream slot only if a BUFFERED stage runs. No message waits for a stream slot while holding a preflight slot, so the two waits cannot deadlock. The regression test holds every stream slot, parks 64 STREAM messages and one mixed message, and checks that all preflight slots stay free and a BUFFERED message is admitted at once. On 30b0f46 the waiters held all 64 preflight slots.

Comment thread proto/supervisor_middleware.proto Outdated
google.protobuf.Duration idle_timeout = 3;
}

// Preflight decision. To deny, return HttpReject instead.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this comment is out of date after the per-RPC split. On the response stream the denial is block_delivery, and cannot_inspect is also a valid preflight result. Maybe: "To deny, return reject (requests) or block_delivery (responses); to fail closed because the stage cannot inspect the message, return cannot_inspect."

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in b9a53d1 with your wording.

Comment thread proto/supervisor_middleware.proto Outdated
// so OpenShell fails the exchange closed and returns the platform-owned
// failure_code middleware_cannot_inspect
// to the sandbox. For uninspectable traffic, OpenShell refuses the connection.
message HttpCannotInspect {}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: HttpCannotInspect is empty, so a service can't attach an internal reason or findings for operators the way HttpReject can. Adding MiddlewareDiagnostics diagnostics = 1; now costs nothing. It would be logged only and never sent to the sandbox, same as reject's reason. Adding it later is also non-breaking, so this isn't blocking.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in b9a53d1: HttpCannotInspect.diagnostics. OpenShell validates it and records findings, metadata, and reason_code the same way as for reject. The sandbox still sees only failure_code: middleware_cannot_inspect.

participant Supervisor
participant Middleware
participant Service as External service
Return `continue_without_body` to allow the connection or `reject` to refuse it. No body mode or header mutation is allowed. Failures and timeouts refuse the connection too.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this lists only continue_without_body and reject. Per the proto (and classify_result), cannot_inspect is also valid here and refuses the connection. Worth mentioning so services know which result to use when they can't decide.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in de68986: cannot_inspect is now listed, and it refuses the connection as a failure.

Services without the session capability use `EvaluateHttpRequest` and `HttpResponsePreReturn.Evaluate`. These RPCs are deprecated for removal in 0.2.0. Legacy requests use a single buffered evaluation. Legacy responses select `HEADERS_ONLY`, `WHOLE_BODY_BYTES`, or `STREAM_BYTES` and use `block_delivery` to stop delivery.

A service that inspects the body also receives trailers after the body. A preflight can declare new trailer names, and the service can then add them.
To migrate, require the session capability, implement the session RPCs, and use `fail_closed`. Replace request evaluation and `WHOLE_BODY_BYTES` with BUFFERED, `STREAM_BYTES` with STREAM, and `HEADERS_ONLY` with `continue_without_body`. Keep `block_delivery` for responses. Re-apply affected policies after changing the service, and separate selectors from services still using legacy hooks, including `openshell/regex`.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Re-apply affected policies after changing the service" isn't enough on its own. The gateway reads each service's Describe result only once, at startup (connect_services_* sets the OnceCell), and policy validation reuses that cached manifest. Until the gateway restarts, it still sees the service as v1, so re-applied policies are checked against stale data: fail_open isn't rejected, mixed-version overlaps aren't caught, and the tls: skip exemption isn't granted. Suggest saying "restart the gateway, then re-apply affected policies", or having the gateway re-read Describe when it validates policies.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Fixed in de68986: the migration step now says the gateway reads Describe only at startup, so restart the gateway, then re-apply affected policies. Re-reading manifests during policy validation would change registry behaviour for every service, so I left it out of this PR.

…m slot

A message whose stages selected STREAM kept its preflight slot while it
waited, up to the chain deadline, for one of the stream slots. With every
stream slot taken by open server-sent-event responses, waiting messages
could hold all preflight slots and starve BUFFERED messages and
uninspectable-connection decisions.

Take a free stream slot immediately when one is available. Otherwise
release the preflight slot before waiting, and take one again afterwards
only if a BUFFERED stage runs. No message holds a preflight slot while it
waits for a stream slot, so the two waits cannot deadlock.

Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
…tages

Response preflight already treats Content-Encoding as read-only. On
requests a stage could still write or remove it. That was not a bypass,
because body-mode offers come from the original head, but a stage could
label a body it replaced as encoded, or drop the label from an encoded
body.

Session hook request heads now protect Content-Encoding. Legacy request
hooks keep their released rules.

Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
HttpCannotInspect was empty, so a stage could not attach findings or a
reason code for operators the way HttpReject can. Add diagnostics to it.
OpenShell validates and records them like a rejection's, and the sandbox
still sees only failure_code middleware_cannot_inspect.

Also update the HttpPreflightResult comment for the per-RPC split:
requests deny with reject, responses with block_delivery, and either
can return cannot_inspect.

Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
…gateway restarts

Uninspectable preflights can also return cannot_inspect, which refuses
the connection as a failure.

The gateway reads each service's Describe manifest only at startup, so
re-applying policies after a service changes hook version validated them
against the stale manifest. Tell operators to restart the gateway first.

Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
@pimlock

pimlock commented Oct 16, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks @matthewgrossman for the second round. The fixes are in de68986, the new head, with one commit per item:

Finding Commit
Waiting for a stream slot holds a preflight slot cd0c390. A message releases its preflight slot before waiting and takes one back only if a BUFFERED stage runs.
Request Content-Encoding is writable 82eba61. Session hook request heads now protect it, as responses do. Legacy request hooks keep their released rules.
Stale HttpPreflightResult comment b9a53d1
HttpCannotInspect has no diagnostics b9a53d1. Recorded like reject; the sandbox still sees only middleware_cannot_inspect.
Uninspectable docs omit cannot_inspect de68986
Re-applying policies isn't enough after a hook version change de68986. The docs now say to restart the gateway first, because it reads Describe only at startup.

Verification:

  • The new regression tests fail on 30b0f46 and pass now.
  • Each commit passes mise run pre-commit.
  • Unit tests pass for openshell-supervisor-middleware (138), openshell-supervisor-network (1580), and the content guard example (23).
  • mise run e2e:middleware-http-session passes.

@pimlock

pimlock commented Oct 16, 2026

Copy link
Copy Markdown
Collaborator Author

Accepting the rust-sdk compatibility finding for this release. Adding EvaluateHttpRequestSession and EvaluateHttpResponseSession to SupervisorMiddleware means Rust implementers of the generated server trait add two methods; returning Status::unimplemented is enough for services that stay on v1 hooks. The session RPCs belong on the same service as Describe and the other hooks, and v1 HTTP hooks are removed in 0.2.0, so a separate service would only move the change.

@github-actions

Copy link
Copy Markdown

Compatibility review (advisory)

New or updated breaking changes; see the full report for all findings.

1. rust-sdk — Risk explicitly accepted

SupervisorMiddleware gained the required EvaluateHttpRequestSession and EvaluateHttpResponseSession RPCs; the baseline generated server trait did not require them.

  • Impact: Rust middleware and mock-server implementations reached through openshell_sdk::raw::proto no longer compile until they add two associated stream types and two methods, while client callers are unaffected.

  • Action: Migration guidance and explicit author acceptance document the intentional change, but v0.1.5 must still preserve the old generated trait or move the required expansion to a minor train.

  • PR: feat(middleware): introduce improved HTTP request/response hooks for the middleware #4359

  • Acceptance: comment

Review details
  • Execution: complete
  • Candidate: PR #4359 (7d1f124e9bcdbf397e85debf5cbad50e47ac96a6)
  • Baseline: 6546691d5e493343c7877573252e835d28bf9e23 (6546691d5e493343c7877573252e835d28bf9e23)
  • Reviewed: rust-sdk, python-sdk, go-sdk, typescript-sdk, api-behavior, cli, configuration, policy, helm, state

Advisory review; publication gates are unchanged.

Branch check run

Response session hooks stripped ETag, Content-MD5, Digest,
Content-Digest, Repr-Digest, Signature, and Signature-Input whenever a
stage replaced or streamed the body, and stages could only remove them.
A STREAM stage that passed the body through unchanged still lost the
upstream's validators, and no stage could supply correct new values.
Requests never stripped them.

Session hooks now leave these headers alone in both directions, as
Envoy's external processor does. A stage that changes the body removes
them or writes new values in buffered_result or output_start; a STREAM
stage can read all input before output_start to compute one. Legacy
response hooks keep their released stripping and remove-only rules.

The content guard example removes them when it replaces a BUFFERED
body, and at output_start when it streams in redact mode.

Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
… choices

The UninspectableTraffic comment and both preflights'
permitted_body_modes comments still said a stage that finds no suitable
mode continues or denies. It can also return cannot_inspect.

Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
…sion

Supervisors also cache each service's Describe manifest and rebuild
their registry only when the set of service registrations changes.
Restarting the gateway and re-applying policies leaves running
sandboxes on the old manifest.

Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
@pimlock

pimlock commented Oct 16, 2026

Copy link
Copy Markdown
Collaborator Author

Follow-ups on the same themes as the last review round. The new head is 9fc4db2.

Commit Change
75e504e Session hook stages now own integrity headers (ETag, Content-MD5, Digest, Content-Digest, Repr-Digest, Signature, Signature-Input). OpenShell no longer strips them when a body changes, and stages may write them in either direction. A STREAM stage that passes the body through keeps the upstream's validators. A stage that changes the body removes them or writes new values in buffered_result or output_start; a STREAM stage can read all input first if it needs the full body. This matches how Envoy's ext_proc leaves validators to the external processor. Legacy response hooks keep their released stripping and remove-only rules. The content guard example removes them when it redacts.
363e12b The UninspectableTraffic and permitted_body_modes comments now list cannot_inspect.
9fc4db2 Supervisors also cache Describe manifests and rebuild only when service registrations change, so the migration step now says to recreate sandboxes that use the service as well.

Verification: unit tests for openshell-supervisor-middleware, openshell-supervisor-network, and the content guard example pass, as do mise run pre-commit and mise run e2e:middleware-http-session.

Resolve conflicts:
- extension_protocol.rs: keep both the compute isolation backend
  capability from main and the HTTP session middleware capability.
- storage_proto.rs: keep 105 hand-classified RPCs (main's 103 plus the
  two session RPCs) and main's isolation and sandbox protocol terms.

Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Since #4016, host e2e runs omit the Docker driver's grpc_endpoint and let
the driver discover the gateway. The HTTP session hook test required the
key and panicked. Fall back to loopback, which the host-networked
supervisor shares with the gateway, and read the key only from the
Docker driver table.

Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
The cannot_inspect result replaced a FAILED_PRECONDITION status
convention after review, which asked only for an explicit, typed signal
whose effect is the reason. A separate result went further: it added a
third terminal result to every phase and a failure response (502 on
responses, platform-owned failure_code) for what is a decision the
stage makes.

Remove HttpCannotInspect and the cannot_inspect result field from both
session result messages. They are unreleased, so they are deleted, not
reserved. A stage that cannot inspect a message rejects it, or blocks
delivery, with a reason_code that says so. The content guard example
uses reason code cannot_inspect. The platform failure_code
middleware_cannot_inspect remains for messages OpenShell cannot show to
a stage.

Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
@pimlock

pimlock commented Oct 16, 2026

Copy link
Copy Markdown
Collaborator Author

One more change, 5bd4e8b (new head): I removed the cannot_inspect result.

@matthewgrossman, your first-round finding asked for an explicit, typed signal in place of the FAILED_PRECONDITION status, and noted its only effect was the reason. A separate result went further than that. It added a third terminal result to every phase, and it gave the sandbox a failure response (502 on responses, platform-owned failure_code) for what is the stage's own decision. A stage that cannot inspect a message now uses reject or block_delivery with a reason_code that says so. The content guard uses reason_code: cannot_inspect. It is still an explicit result rather than a status. HttpCannotInspect and field 7 are deleted rather than reserved, since they were never released. This also drops the diagnostics added in b9a53d1; HttpReject already carries them. The middleware_cannot_inspect failure code stays for messages OpenShell itself cannot show to a stage.

Also in this push: 889d94a merges main, and 4199f43 fixes the HTTP session hook e2e test, which needed the Docker driver's grpc_endpoint. Since #4016, host runs omit it.

Verification: unit tests for openshell-supervisor-middleware (138), openshell-supervisor-network (1587), and the content guard example (23) pass, as do mise run pre-commit and mise run e2e:middleware-http-session.

Resolve conflicts:
- e2e-docker-test.yml: keep both new suites in the default matrix,
  rust-docker-push from main and middleware-http-session.

Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gator:blocked Gator is blocked by process or repository gates test:e2e Requires end-to-end coverage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants