Repository navigation
feat(bep): validate the upstream, pass unsealed requests through, and stream replies - #2481
Conversation
The proxy now refuses a CONNECT whose upstream chain or hostname does not validate against the host trust store. It answers 502 and audits the refusal `upstream_tls_invalid`. It takes the interception root out of the upstream roots, so a chain under that root never validates (BEP-055). Each parsed request inside a tunnel goes to a decision. A sealed value goes to the redeemer, and its credential is substituted only on a leg that validated: the forwarder requires a proof that only a validated stream gives. An unsealed request goes upstream unchanged and is audited, marked `foreign_credential` when it carries its own Authorization header (BEP-030). The response half writes and flushes each chunk as it arrives (BEP-N01). The default redeemer refuses every sealed value, because the proxy has no proxy key or box registry yet.
|
Important Review skippedBot user detected. To trigger a single review, invoke the
|
|
Reviewed: 1 finding, none blocking at Address the comments on the diff. Clobber: none seen. |
There was a problem hiding this comment.
arch-review: CONDITIONS · 2 conditions · the full ruling
The forwarder validates the upstream before it substitutes a credential, and it streams responses. Both conform. The passthrough egress check does not use the box's declared egress. Unwired::egress gives every module host to every box, and Gatehouse §6.10 line 823 says the box's declared egress must admit the host. C1 makes the unwired default deny. This PR also closes #2272, but main.rs names #2272 as the flip for the NoSigningKey interim. C2 restates that flip and links the issue that wires the real redeemer.
Ruled at 64b47c6 against gominimal/arch@109541a48c31 by claude-opus-5-5.
Proxy::new skipped the removal of the interception root from the upstream roots when RootCertStore::add failed. It now returns the error, so the upstream leg never trusts that root (BEP-055).
|
Round 1 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
An upstream that fails validation receives nothing, an unsealed request passes unchanged, and responses stream. The proxy now also refuses to start when it cannot remove the interception root from the upstream roots, so the upstream leg never trusts that root. Notes from the agent: Revise round 1. Proxy::new now propagates the RootCertStore::add error with |
There was a problem hiding this comment.
APPROVE withheld: 5 unresolved threads — minimal-foundry[bot] crates/bep/src/proxy.rs, minimal-foundry[bot] crates/bep/src/proxy.rs:318, minimal-arch[bot] crates/bep/src/forward.rs:216, minimal-arch[bot] crates/bep/src/proxy.rs:325, minimal-arch[bot] crates/bep/src/forward.rs:23.
minimal-review — advisory review
Verdict COMMENT — No findings.
Panel: 2 of 3 personas ran (correctness, security); did not complete: supply-chain.
There was a problem hiding this comment.
arch-review: CONDITIONS · 4 conditions · the full ruling
The upstream validation (BEP-055) and the response streaming (BEP-N01) conform. The new commit fails closed on an unparsable interception root. The unsealed passthrough (BEP-030) is not egress-checked. Unwired gives the admitted set as every box's egress, and §6.10 line 823 says the box's declared egress MUST admit the host. Make Unwired refuse (C1), and prove the passthrough with a declared egress and a refusal case (C2). This PR closes #2272, but main.rs still names #2272 as the lift for the NoSigningKey interim. Point it at a new issue (C3). C4 covers sealed values in a named injection header.
Ruled at 168c6e8 against gominimal/arch@109541a48c31 by claude-opus-5-5.
There was a problem hiding this comment.
Babysitter review of T11 at 168c6e8 (round 1 over 64b47c6). Not an approval yet.
Verified line by line against spec 24: validate is the only maker of Validated, and Flow::new and substitute need the ValidatedUpstream proof only it gives, so no path forwards or substitutes on a leg that did not validate (BEP-055); the interception root is taken out of the upstream roots and an unparsable root now fails Proxy::new closed, which settles the two runtime threads at 64b47c6; an unsealed request goes upstream byte for byte and is audited, foreign_credential when it carries its own Authorization (BEP-030); relay_responses writes and flushes each chunk as it arrives (BEP-N01). The credential is zeroized in the substituted header, the serialized head and the verdict's Debug. Both commits lint.
The arch ruling's four conditions stand and go to the next round: Unwired must declare no egress, so a proxy that does not know a box's declared egress refuses a passthrough (§6.10 line 823); the passthrough test needs a redeemer with the declared egress and an outside_declared_egress case that sends nothing upstream; main.rs must stop naming #2272 as the lift of the NoSigningKey interim, since this PR closes it (the wiring of the real redeemer and declared egress is now #2483); and carried must say that a store member in a named header is #2292's.
Low, for the same round or none: the UpstreamError doc says Invalid covers an upstream that ends the handshake, but an EOF there is an io error and so Unreachable; a request with one sealed and one foreign Authorization header is treated as sealed and the foreign header rides upstream beside the substituted credential; and the Redeemer trait doc should say the sealed path applies none of decide_plain's checks, so a redeemer carries every BEP-025 check itself.
Generated by Claude Code
|
/revise Carry the four arch-review conditions at 168c6e8 (ruling 5605208954), and three low notes from the babysitter review. C1 (arch, threads 4341137473, 4341137488, 4341060874, 4341060885). C2 (arch, thread 4341137495). C3 (arch, thread 4341060893). This PR closes #2272, but the C4 (arch, thread 4341137501). Low, from the babysitter review (5605287359), take or leave: the Reply on each arch thread with the commit that carries it. Generated by Claude Code |
Unwired gave the admitted set as the egress of every box, so an unsealed request never met the declared-egress check (Gatehouse §6.10 step 3). Unwired now declares no egress, and Proxy::new installs it. A proxy with no redeemer refuses every unsealed request as outside_declared_egress. Admission::authorities had no other user and is gone. The passthrough and streaming tests now use a test redeemer that declares the GitHub host set. A new test proves that the default proxy refuses an unsealed request and sends nothing upstream. The docs now point the interim at #2483, and say that carried searches only Authorization (#2292 owns other headers), that a second foreign Authorization rides beside the substituted credential, that a redeemer carries every BEP-025 check, and that a handshake EOF is Unreachable.
|
Round 2 of 10 on a /revise note, pushed as
Verify Review threads the coder judged:
An upstream that fails validation receives no byte and the refusal is audited upstream_tls_invalid. An unsealed request to a host in the box's declared egress passes upstream unchanged and is audited, marked foreign_credential when it carries its own Authorization. An unsealed request outside the declared egress gets a 403, is audited outside_declared_egress, and the upstream reads nothing. Responses stream chunk by chunk to the box. Notes from the agent: C3 is done only in part. The forward.rs module doc and the Proxy::new doc now name #2483. The NoSigningKey doc (main.rs lines 196-207) and its warn! (line 268) still name #2272. crates/bep/src/main.rs is not in this task's layers, so I did not change it. A follow-up, or a person, must change those three lines. proxy_offers_http11_only_on_both_legs in proxy.rs passed only through the relaxed gate too. Its harness now installs a test redeemer (DeclaresGithub) that declares the GitHub host set. Admission::authorities was added by this PR and had no other user after C1, so I removed it. Low notes: the UpstreamError::Invalid doc now says a handshake EOF is Unreachable. Redeemer::redeem now says that a redeemer carries every BEP-025 check. For two Authorization headers, I documented on carried that a foreign one goes upstream beside the substituted credential. I did not add a refusal for it. |
There was a problem hiding this comment.
arch-review: PASS_WITH_CONDITIONS · 1 condition · the full ruling
The forwarder conforms to Gatehouse §6.10. The upstream validates before any request goes on it, passthrough is egress-checked, and replies stream. The compare fixes the open egress check: Unwired now declares no egress, so the default proxy refuses an unsealed request. One stale interim remains. crates/bep/src/main.rs still says #2272 lifts the bep serve interim, and this PR closes #2272. C1 points that text at #2483.
Outside the diff
crates/bep/src/main.rs:203: C1 · This comment says thatproxy::relayforwards with no per-request decision, and that #2272 (T11) wires the decision in. This PR is T11 and closes #2272. Say thatUnwiredrefuses every request until #2483 wires the real redeemer and each box's declared egress. Make the same change in thewarn!text at line 266.
Ruled at fd36c95 against gominimal/arch@109541a48c31 by claude-opus-5-5.
Every request now goes through decide_plain before the redeemer sees it. A TLS handshake failure that is not a certificate error no longer tells the box that the certificate did not validate.
|
Round 3 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
An upstream that fails validation receives nothing and is audited upstream_tls_invalid. An unsealed request inside the box's declared egress passes unchanged and is audited. Responses stream chunk by chunk. A sealed request outside the declared egress or with a wrong Host now never reaches the redeemer. Notes from the agent: The five test: lines were run together in one nextest invocation, with the two new tests sealed_request_outside_egress_or_host_is_not_redeemed and only_certificate_errors_are_invalid. The 8 tests passed. Then the whole bep suite passed once. A TLS handshake failure that is not a certificate error (an alert, an incompatible peer) is now UpstreamError::Handshake. It keeps the audit marker upstream_tls_invalid, because the leg did not validate (BEP-055). Its 502 body now says 'the TLS handshake with the server failed', and not that the certificate did not validate. The span records upstream_tls=handshake_failed for it. |
There was a problem hiding this comment.
arch-review: PASS · the full ruling
The compare puts the egress check and the Host pin before redemption. Gatehouse §6.10 line 823 gives that order. A non-certificate TLS failure now has its own error. It still refuses and audits upstream_tls_invalid, so the upstream leg stays closed. The wiring of the real redeemer stays open in #2483, and Unwired refuses everything until then.
Ruled at 4b810f5 against gominimal/arch@109541a48c31 by claude-opus-5-5.
Send the refusal before the upstream shutdown, and take an already-sent refusal when the upstream read ends, so the box sees the 403 and not a silent close. Refuse a redeemed credential that holds CR, LF or NUL, and audit it credential_not_header_safe, before it goes into a header.
|
Round 4 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
An upstream that fails validation receives nothing, an unsealed request passes unchanged, and responses stream. A refused request now gets its 403 even when the upstream closes first. A redeemed credential with CR, LF or NUL never goes into a header. Notes from the agent: Revise round 4. These test lines ran in one nextest invocation with the five task tests: credential_with_line_break_is_not_substituted and refusal_is_answered_after_upstream_closes. All seven passed. A final cargo nextest run -p bep passed 120 of 120. This round adds a new audit marker, credential_not_header_safe (forward::CREDENTIAL_NOT_HEADER_SAFE). The spec does not name it. If the spec must list every marker, a spec follow-up must add it. just clippy-strict was not run. |
There was a problem hiding this comment.
arch-review: CONDITIONS · 1 condition · the full ruling
The new header-safety refusal in Flow::decide writes an inexact audit record. The redeemer already returned the identifier and the mapping, but the record writes none and module_unmapped. Gatehouse §6.10 line 823 says every refuse records the mapped resource and permission. It uses module_unmapped only when the module cannot map the request. C1 passes Some(identifier) and mapping to the record and adds a test through Flow::decide. The relay change, which answers the 403 after the upstream closes, conforms.
Ruled at d827178 against gominimal/arch@109541a48c31 by claude-opus-5-5.
The upstream can honour either Authorization header, so a request with a sealed value and a second Authorization header is refused and audited mixed_authorization. The header-safety refusal now records the mapped identifier and mapping, and substitute fails when its header is missing.
There was a problem hiding this comment.
arch-review: PASS · the full ruling
The compare conforms to Gatehouse §6.10. A sealed value beside a second Authorization header is now refused with mixed_authorization, after the egress check and before a redeem. A refusal after the module admits now records the identifier and the mapping, as line 823 requires. substitute now fails closed when its header is missing. No conditions remain. The interims stay filed as #2483 and #2292.
Ruled at 5fc57d6 against gominimal/arch@109541a48c31 by claude-opus-5-5.
|
Round 5 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
An upstream that fails validation receives nothing, an unsealed request passes unchanged and is audited, and responses stream to the box chunk by chunk. A sealed request that also carries another Authorization header is now refused, and nothing is substituted into it. Notes from the agent: Round 5. Two new audit markers: mixed_authorization (a sealed value beside another Authorization header, which includes two sealed headers) and sealed_header_missing (substitute finds no header at its index, not reachable today). The spec does not name these markers. A refused mixed request gets a new 403 body: 'The Box Egress Proxy refused the request to , because it has more than one Authorization header. Send the sealed credential in one Authorization header.' New tests: unsafe_credential_refusal_records_the_mapping, sealed_beside_other_authorization_is_refused, substitution_without_its_header_fails. All 14 forward tests pass. The clippy run for bep was clean. just clippy-strict was not run. |
|
Round 6 of 10 on the runtime's review findings: the coder changed nothing. Revise round 6: no code change. The one open thread asks for a PR body correction. The correct statement is this: Unwired, which Proxy::new installs, declares an empty egress for every box, so it refuses every passthrough with outside_declared_egress. It does not give the admitted set as egress. The finding's behaviour claim does not hold: bep serve installs NoSigningKey (main.rs), so Proxy::server_config fails and every admitted CONNECT is answered 502 before any request is parsed. bep serve forwarded nothing before this PR and forwards nothing now. Not changed, outside layers: the NoSigningKey doc and the startup warn! in crates/bep/src/main.rs still name #2272 as the issue that lifts the interim. #2483 is the issue that wires the real redeemer and each box's declared egress. Review threads the coder judged:
|
There was a problem hiding this comment.
Babysitter approval of T11 at 5fc57d6.
Six rounds, each verified against its diff: the upstream leg validates against the host trust store before any request or substitution, with the interception root taken out and an unparsable root failing closed (BEP-055); the declared egress and the Host pin are checked for every request before a redeemer sees a sealed value; Unwired declares no egress, so the default proxy refuses every passthrough until #2483 wires the real redeemer; an unsealed request inside a declared egress goes upstream byte for byte and is audited (BEP-030); responses stream chunk by chunk (BEP-N01); a refusal reaches the box as a 403 even when the upstream closes first; a credential with a line break, a sealed value beside another Authorization header, and a non-certificate TLS failure are each refused and audited. Arch-review passed at this head, the runtime's review concluded with every thread resolved and each resolution re-verified, every lane aggregator is green, and the squash subject lints. The NoSigningKey text in main.rs that still names #2272 is #2483's.
Generated by Claude Code
T11 — Validate the upstream before substituting, pass unsealed requests through, and stream responses
slice: S0
kind: task
covers: BEP-030, BEP-055, BEP-N01
size: L
est: 400 code, 300 tests
executor: agent
difficulty: needs care
model: strongest
diagnostics: a refused upstream is audited
upstream_tls_invalidorupstream_not_tls, and a passthrough carrying its own credentialforeign_credential, each shown bymin box audit <box>observability: the connection span gains the upstream's validation outcome and the substitution decision, never the credential
layers:
- forward:
- crates/bep/src/forward.rs (new): upstream TLS validated against the host store before forwarding (BEP-055), substitution on admit, passthrough (BEP-030), chunked relay (BEP-N01)
- crat 8000 es/bep/src/proxy.rs: hands each parsed request to the decision and the forwarder
- crates/bep/src/lib.rs: module line
test: cargo nextest run -p bep unsealed_request_passes_through_and_is_audited
test: cargo nextest run -p bep prop_upstream_tls_validated_before_substitution
test: cargo nextest run -p bep upstream_tls_failure_is_refused_and_audited
test: cargo nextest run -p bep plaintext_request_to_credentialed_host_is_refused
test: cargo nextest run -p bep streamed_response_is_not_buffered
produces:
- crates/bep/src/forward.rs
consumes:
- crates/bep/src/proxy.rs
- crates/bep/src/upstream.rs
proof: An upstream that fails validation receives nothing, an unsealed request passes unchanged, and responses stream.
depends on:
- blocked by #2261
- blocked by #2271
Proof
A CONNECT to a credentialed host whose upstream certificate does not validate against the host trust store gets a 502. The upstream receives no request byte, and
min box audit <box>shows the refusal markedupstream_tls_invalid. A chain under the interception root is refused even when the host store holds that root. A request with no sealed value reaches the upstream byte-for-byte in its canonical form, and the audit shows an admit, markedforeign_credentialwhen the request carries its own Authorization header. The box receives the first chunk of a response while the upstream still holds the rest.Notes from the agent: The proxy has no proxy key, box registry or sign-in resolution yet. So the default redeemer (
forward::Unwired) refuses every sealed value with theundecryptablemarker.Proxy::with_redeemertakes the real decision when a later task wires it. Substitution itself is implemented and tested with a stub redeemer, and it needs aValidatedUpstreamproof that only a validated stream gives. The proxy also does not know a box's declared egress yet, soUnwireddeclares an empty egress for every box, anddecide_plainrefuses every passthrough asoutside_declared_egressuntil #2483 wires the real redeemer and each box's declared egress. Sealed values are found only inAuthorizationheaders (Bearer, token, Basic user or password, bare value), not in URLs or bodies. A refused request inside a tunnel gets a 403 at once when no earlier request went upstream. Otherwise the 403 follows after the upstream closes its side. The response relay copies bytes as received. Alt-Svc stripping on responses (BEP-076) is not part of this task.LeafIssuergained a defaultroot()method so the proxy can take the interception root out of the upstream roots, andmain.rsneeds no change. The test helpers inproxy.rstests becamepub(crate)soforward.rstests reuse them. One full run ofcargo nextest run -p beppassed (114 tests). A mutation check, which kept the interception root in the upstream roots, made the property test fail on the `InterceptionRoo…[truncated]Tests
cargo nextest run -p bep unsealed_request_passes_through_and_is_auditedcargo nextest run -p bep prop_upstream_tls_validated_before_substitutioncargo nextest run -p bep upstream_tls_failure_is_refused_and_auditedcargo nextest run -p bep plaintext_request_to_credentialed_host_is_refusedcargo nextest run -p bep streamed_response_is_not_bufferedVerify
min task run agent-verifypassed ([tasks.verify]).Closes #2272
Generated by Claude Code