Repository navigation
feat(bep): inject a stored key in its registered form per request (T28) - #2574
norrietaylor merged 13 commits into
Conversation
The forwarder reads a store reference's value from the store for each request and puts it in the rule's header, with its prefix, or in one basic_auth field. A deleted or disabled item, or a value or prefix with a line break, refuses the request and is audited.
…tored-key-in-the-registered-form-read
|
Important Review skippedBot user detected. To trigger a single review, invoke the
 |
|
Approved: every review thread resolved at @norrietaylor resolved the last thread, which releases the approval this review had withheld. Nothing to do. |
There was a problem hiding this comment.
arch-review: CONDITIONS · 3 conditions · the full ruling
The store injection, the per-request read and the deletion refusal conform to Gatehouse §6.10. One deviation remains. carried() now takes a sealed value from any header, and no path checks that header. Line 849 says the handle is located where the injection format says the value rides. C1 adds the check on the store path, before the Keychain read. C2 keeps the GitHub module credential in Authorization only. C3 files the two wiring follow-ups as issues, because bep serve injects no store reference until they land.
Ruled at 00a7a41 against gominimal/arch@109541a48c31 by claude-opus-5-5.
An item that does not let the proxy read it raised a system dialog and blocked the request. With prompts off, the read returns errSecInteractionNotAllowed and the request fails as store_item_disabled. A mutex serialises reads, because the prompt setting is process-wide.
|
Round 1 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
A request to the rule's upstream carries the stored key in the registered header, and a replaced key is sent next time. On macOS, a Keychain item that does not let the proxy read it now fails the request as store_item_disabled and does not wait on a system dialog. Notes from the agent: Revise round 1. The macOS host::read now holds a process-wide mutex and calls SecKeychain::disable_user_interaction() for the duration of get_generic_password. The guard turns prompts back on when it drops. The mutex is necessary because the prompt setting is process-wide. Without it, one read can turn prompts back on while another read is in progress. This box has no macOS target, so the changed code was not compiled. I checked the API against the security-framework 3.7.0 source: os::macos::keychain::SecKeychain::disable_user_interaction returns Result. Linux clippy and all seven test lines pass, but they do not compile the macOS module. A macOS lane (ci-macos) must confirm that it builds. |
There was a problem hiding this comment.
arch-review: CONDITIONS · 2 conditions · the full ruling
The forwarder injects a store value in the registered form. But it takes the sealed value from whichever header it is found in. Gatehouse §6.10 line 849 says the handle is located where the injection format says the value rides. Make inject_store() refuse a sealed value outside the registered header or basic_auth field (C1). The running proxy also admits no store reference yet, and that follow-up has no issue. File it and name it in the PR body (C2). The Keychain prompt change in the compare fails closed and conforms.
Ruled at 9680aea against gominimal/arch@109541a48c31 by claude-opus-5-5.
A store reference must sit in the header or basic_auth field its rule names, and a sign-in reference in Authorization. Otherwise the proxy refuses it as sealed_header_misplaced and does not read the store. The mixed-credential refusal text now covers two sealed headers, and the store wiring comments say it is interim.
…tored-key-in-the-registered-form-read
|
Round 2 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
A request to the rule's upstream carries the stored key in the registered header, and a replaced key is sent next time. A sealed value in any other header is now refused as sealed_header_misplaced, and the proxy does not read the store for it. These facts hold in the bep flow tests. bep serve does not wire the store yet, so a running proxy does not inject a stored key. Notes from the agent: Revise round 2. New marker SEALED_HEADER_MISPLACED, with new refusal text. This round also adds two tests: store_reference_in_unregistered_header_is_refused and sign_in_reference_outside_authorization_is_refused. The MIXED_AUTHORIZATION text now reads 'because it carries a credential in more than one header. Send one sealed credential, in one header.' No test or doc pinned the old text. The store path is not wired in production. BoxRedeemer (crates/bep/src/redeemer.rs) does not override redeem_reference, and bep serve does not call Flow::with_store. Both files are outside this task's layers. I did not open follow-up issues because this session has no GitHub credential. A person must open them and link them from the two interim comments. The full bep suite passed (178 tests) and clippy -p bep --all-targets is clean. I did not run just clippy-strict. |
There was a problem hiding this comment.
arch-review: PASS_WITH_CONDITIONS · 1 condition · the full ruling
The change now conforms to Gatehouse §6.10 line 849, step 4. The proxy reads the store only when the sealed value is in the header or basic_auth field that the rule names. Otherwise it refuses as sealed_header_misplaced. One item is open. The two interim notes in forward.rs say "a follow-up task outside T28" and link no issue. Link the issue that owns the BoxRedeemer override and the bep serve store wiring, as C1 says.
Ruled at 2af1ce7 against gominimal/arch@109541a48c31 by claude-opus-5-5.
The injection_invalid text named a line break only. The marker also covers NUL, a colon in the user field, and a bad header name or field.
|
Round 3 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
In the bep tests, a request to the rule's upstream carries the stored key in the registered header, and a replaced key goes upstream on the next request. bep serve does not yet do this, because no production redeemer returns a store reference and proxy.rs does not call with_store. Notes from the agent: This round changes one thing. The injection_invalid refusal text now holds for each of its causes, and the crlf test pins the new text. The C2 wiring (BoxRedeemer returns Redeemed::Store, proxy.rs calls with_store) is outside this task's layers. The coder has no gh credential, so it did not file the issue. A person must file it and name it in the PR body as a residual. Until then, bep serve refuses every store reference. clippy -p bep --all-targets is clean. |
There was a problem hiding this comment.
arch-review: PASS_WITH_CONDITIONS · 1 condition · the full ruling
The compare rewords the injection_invalid refusal so that it holds for each cause the marker covers, and a test pins the text. That conforms. The marker and the audit record are unchanged (§6.10 line 849). One condition stays open. The interim comments on redeem_reference and with_store defer the bep serve wiring to an unnamed follow-up task. C1 links each one to a filed issue with its flip condition.
Ruled at 4ee866a against gominimal/arch@109541a48c31 by claude-opus-5-5.
The default redeem_reference now takes the sealed value's header name and refuses one outside Authorization before redeem runs, so no sign-in token is renewed for it. The forwarder zeroizes the header a store value was injected in, whatever its name, and inject moves the value into the request in place of a copy. The interim notes link #2280 and state when they stop being true.
…tored-key-in-the-registered-form-read
|
Round 4 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
A request to the rule's upstream carries the stored key in the registered header. A replaced key is sent on the next request. The forward tests show both against a memory store. A served flow does not inject yet: bep serve does not call Flow::with_store, and no redeemer returns Redeemed::Store until the store rules land (#2280). Notes from the agent: Round 4. Redeemer::redeem_reference now takes the name of the header that carries the sealed value. Its default refuses a value outside Authorization as sealed_header_misplaced before redeem runs. Only the AdmitsStore test overrides it. The placement check after a sign-in admit stays as a second guard for an override. A new private Flow::decide_injected returns the injected header name, and forward_requests zeroizes that header after it writes the head. Flow::decide keeps its signature. inject now moves the value out of its Zeroizing buffer and does not copy it. I cannot file issues from this box. So the interim notes link #2280, the issue that redeemer.rs and main.rs give for store rules, and each note states its flip condition. The PR body must name #2280 as the residual for the BoxRedeemer override and the bep serve with_store wiring. If #2483 owns the serve wiring, a person can relink it. Two new tests: sign_in_reference_outside_authorization_is_not_redeemed and injected_header_is_named_for_zeroizing. |
There was a problem hiding this comment.
arch-review: PASS_WITH_CONDITIONS · 1 condition · the full ruling
The compare closes the open points of the ruling on 4ee866a4. A sign-in value outside Authorization is now refused before the redeemer runs, so nothing is renewed (§6.10 lines 839 and 849). The injected header is zeroized whatever its name. The interim notes link #2280 and state when they stop being true. One condition stays open as a filed residual: bep serve admits no store reference until #2280 lands (C1).
Ruled at 90885ea against gominimal/arch@109541a48c31 by claude-opus-5-5.
A sealed value in a header such as x-api-key, beside an Authorization header that does not hold it, is now mixed and refused. The upstream can honour either header.
|
Round 5 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
A request to the rule's upstream carries the stored key in the registered header, and a replaced key is sent on the next request. A sealed value in a header such as x-api-key beside the box's own Authorization header is now refused as mixed_authorization. Notes from the agent: Revise round 5. Only crates/bep/src/forward.rs changed: carried() returns Mixed when any Authorization header that does not hold the sealed value is beside it, with the new test sealed_in_other_header_beside_authorization_is_mixed. The full bep suite passes (181). The two minimal-arch interim-note threads describe work for #2280 and the bep serve wiring. The notes already link #2280 and state the flip condition, so this round changes nothing for them. just clippy-strict was not run. |
There was a problem hiding this comment.
arch-review: PASS_WITH_CONDITIONS · 1 condition · the full ruling
The compare refuses a sealed key in any header beside the box's own Authorization header. That is fail-closed, and it conforms to Gatehouse §6.10 line 849. The baseline ruling stands for the rest of the change. The bep serve path does not inject a store value yet, and #2280 owns that work. Make the change in C1, so that the PR does not close #2292 while the serve-path proof is open.
Ruled at 8a505ad against gominimal/arch@109541a48c31 by claude-opus-5-5.
8a505ad to
5d11e4b
Compare
There was a problem hiding this comment.
arch-review: PASS_WITH_CONDITIONS · 1 condition · the full ruling
This push has the same tree as 8a505ad2, so the ruling on that head stands. The injection matches Gatehouse §6.10 line 849: exactly prefix plus value, no CR or LF, and an audit without the value. One condition stays open. C1, filed as #2586, wires BoxRedeemer::redeem_reference and Flow::with_store into bep serve. Until then bep serve refuses store members, and BEP-032 holds in the forwarder only.
Ruled at 5d11e4b against gominimal/arch@109541a48c31 by claude-opus-5-5.
…tored-key-in-the-registered-form-read
A rule that names content-length, host, connection or another framing or hop-by-hop header is refused as injection_invalid. The interim notes now name #2586 and the two edits that end them.
|
Round 6 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
In the crate's tests, a request to the rule's upstream carries the stored key in the registered header, and a replaced key is sent on the next request. A rule that names a framing header such as content-length is now refused as injection_invalid. Notes from the agent: Revise round 6. inject.rs refuses framing and hop-by-hop header names (connection, content-length, host, keep-alive, proxy-authorization, proxy-connection, te, trailer, transfer-encoding, upgrade) as injection_invalid. The two interim notes in forward.rs now link #2586 and name both edits that end them. The PR body is written by the runner, not by this branch: it must say 'Part of #2292', not 'Closes #2292', because bep serve does not inject until #2586 lands. The full bep suite passed once at the end (192 passed). |
There was a problem hiding this comment.
arch-review: PASS_WITH_CONDITIONS · 1 condition · the full ruling
The compare changes the PR's own code in one commit only. The merge of main leaves the PR's lines unchanged. Commit 91c6c78e refuses a framing or hop-by-hop header as an injection target. This agrees with Gatehouse §6.10 line 849. The rule layer in crates/boxes/src/core/bep.rs enforces the full line 849 deny set, so the shorter list in inject.rs is an extra check. The interim notes now name #2586 and both flip edits. C1, the bep serve path, stays open as that residual.
Ruled at 91c6c78 against gominimal/arch@109541a48c31 by claude-opus-5-5.
errSecInteractionNotAllowed also comes from a locked keychain when prompts are off. Report it as a store refusal that names both causes.
|
Round 7 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
A request to the rule's upstream carries the stored key in the registered header, and a replaced key is sent next time. This round, a read that fails with errSecInteractionNotAllowed no longer says that the secret does not let the proxy read it. The refusal now says that the keychain is locked, or that the secret does not let the Box Egress Proxy read it without a prompt. Notes from the agent: The changed code is in the macOS-only |
There was a problem hiding this comment.
arch-review: PASS_WITH_CONDITIONS · 1 condition · the full ruling
The compare moves errSecInteractionNotAllowed out of the disabled-item codes. A locked keychain is now recorded as store_item_unreadable, and the request is still refused. Gatehouse §6.10 line 844 requires the read under the store's ACL, and this record is more exact than before. The baseline condition stays open as C1 and the filed residual #2586: bep serve does not inject a store value yet.
Ruled at fe47db2 against gominimal/arch@109541a48c31 by claude-opus-5-5.
A store reference whose store failed the read now gets its own refusal text, not the generic one. The base64 form of a basic_auth pair is now encoded straight into the zeroizing header buffer.
|
Round 8 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
In the bep flow tests, a request to the rule's upstream carries the stored key in the registered header, and a key that is replaced in the store is sent on the next request. bep serve does not inject a store value yet. That wiring is in redeemer.rs and proxy.rs, outside this task, and issue #2586 tracks it. Notes from the agent: Round 8. A store read failure now has its own refusal text. The base64 basic_auth pair is now encoded straight into the zeroizing buffer. errSecInteractionNotAllowed stays mapped to store_item_unreadable, because a locked keychain also returns it (a thread from an earlier round). The two C1 threads ask for changes in redeemer.rs and proxy.rs, which are outside this task's layers. The full bep suite passed once at the end: 192 passed. |
There was a problem hiding this comment.
arch-review: PASS_WITH_CONDITIONS · 1 condition · the full ruling
The new push conforms. The store_item_unreadable refusal now states the real cause, and the basic_auth value stays in one zeroizing buffer with the same bytes. Section 6.10 line 849 still holds for the injection form. C1 from the last ruling stays open: bep serve does not inject a store value until BoxRedeemer and proxy.rs are wired. That work is filed as #2586, so the change can merge.
Ruled at fec6f0b against gominimal/arch@109541a48c31 by claude-opus-5-5.
|
Round 9 of 10 on the runtime's review findings: the coder changed nothing. Revise round 9. I judged both open threads invalid, so this round has no code change. The head commit fec6f0b is unchanged, and all seven test lines pass on it. The C1 wiring (proxy.rs calls Flow::with_store, and BoxRedeemer overrides redeem_reference) is in crates/bep/src/proxy.rs and the redeemer. Those files are outside this task's layers, and #2586 tracks the change. Review threads the coder judged:
|
fec6f0b to
ac87b37
Compare
There was a problem hiding this comment.
arch-review: PASS_WITH_CONDITIONS · 1 condition · the full ruling
This head has the same content as fec6f0b4, so the earlier ruling stands. The injection allowlist, the read for each request and the deleted-item refusal conform to Gatehouse §6.10 lines 844 and 849. bep serve does not inject a store value yet, because BoxRedeemer does not return Redeemed::Store and proxy.rs does not call Flow::with_store. C1 covers that wiring. It is filed as #2586 and can stay open at merge.
Ruled at ac87b37 against gominimal/arch@109541a48c31 by claude-opus-5-5.
There was a problem hiding this comment.
minimal-review — approval withheld
Verdict COMMENT — APPROVE withheld: 7 unresolved threads — minimal-arch[bot] crates/bep/src/forward.rs:326, minimal-arch[bot] crates/bep/src/forward.rs:326, minimal-arch[bot] crates/bep/src/forward.rs:326, minimal-arch[bot] crates/bep/src/forward.rs:794, minimal-foundry[bot] crates/bep/src/forward.rs:68, …. No findings.
Panel: 3 of 3 personas ran (correctness, security, supply-chain).
There was a problem hiding this comment.
minimal-review — approved
Verdict APPROVE — every review thread is resolved; @norrietaylor closed the last one. The review of ac87b37ed9e0 withheld this approval on the threads open then. Posted as a comment: the App cannot approve its own pull request; the check and the label carry it.
There was a problem hiding this comment.
Babysitter approval at ac87b37: every merge gate passes at this head (arch PASS at ac87b37, zero open threads, every lane aggregator green). Squash title feat(bep): inject a stored key in its registered form per request (T28) and the squashed message pass commitlint. Queued for squash merge.
Generated by Claude Code
T28 — Inject a stored key in the registered form, read from the Keychain per request
slice: S3
kind: task
covers: BEP-032, BEP-033, BEP-045, BEP-054, BEP-065, BEP-066
size: L
est: 380 code, 420 tests
executor: agent
difficulty: needs care
model: strongest
diagnostics:
min box audit <box>shows each injection's store and identifier, and a refused one markedinjection_invalidor as a deleted item, never the valueobservability: one debug log line per injection naming the store, the identifier and the header, never the value
layers:
- inject:
- crates/bep/src/inject.rs (new): emits the registered prefix and the value as the header's value, or one
basic_authfield; refuses CR or LF in a value or prefix- crates/bep/src/store.rs: reads the value per request, writes it nowhere, refuses a deleted or disabled item
- crates/bep/src/forward.rs: injects for an admitted store reference; refuses a sign-in reference naming another instance
- crates/bep/src/lib.rs: module line
test: cargo nextest run -p bep store_reference_injects_in_registered_form
test: cargo nextest run -p bep store_value_read_per_request_never_written
test: cargo nextest run -p bep deleted_keychain_item_refuses_next_injection
test: cargo nextest run -p bep reference_to_another_sign_in_instance_is_refused
test: cargo nextest run -p bep replaced_secret_is_injected_on_next_request
test: cargo nextest run -p bep injected_value_is_exactly_prefix_plus_value
test: cargo nextest run -p bep crlf_in_value_or_prefix_refuses_injection
produces: none
consumes:
- crates/bep/src/store.rs
proof: A request to the rule's upstream carries the stored key in the registered header, and a replaced key is sent next time.
depends on:
- blocked by #2287
- blocked by #2288
- blocked by #2257
Proof
At the forwarder, a request that carries a sealed store reference reaches the rule's upstream with the stored key in the registered header (for example
authorization: Bearer <key>orx-api-key: <key>), or in one basic_auth field. Aftermin secret setreplaces the item, the next request on the same flow carries the new key. The forwarder tests in crates/bep/src/forward.rs show this. Inbep servethis does not happen yet, because BoxRedeemer admits no store reference (see notes).Notes from the agent: Design: the Redeemer trait gets a default method
redeem_referencethat returnsRedeemed::VerdictorRedeemed::Store(StoreAdmit). The forwarder calls it. Existing redeemers stay unchanged, so redeemer.rs, proxy.rs and main.rs need no edit. On an admitted store reference,Flowreads the value through a newstore::ValueReader(set withFlow::with_store) for each request. Theninject::injectremoves the header that carried the sealed value and writes the registered form.carried()now searches every header for a sealed value, not only Authorization, so a client that sends its key inx-api-keyworks. Two sealed values in one request, or a sealed Authorization beside another Authorization header, is still refused asmixed_authorization. New audit markers:store_item_deleted,store_item_disabled,store_item_unreadable(no store in the flow, or the store failed).injection_invalidalso covers a NUL byte, a ':' in the basic_auth user field, an unknown basic_auth field, and a header name that is not a token. Each injection writes one debug line with the store, the identifier, the header and the outcome, and never the value. Follow-up work outside these layers: (1) BoxRedeemer (redeemer.rs) must overrideredeem_referenceto returnRedeemed::Storefor an admitted store-handle member. Its store rules are still empty until T25. (2) proxy.rs must callFlow::with_store(Arc::new(HostStore))where it builds a Flow. Until both land, `b…[truncated]Tests
cargo nextest run -p bep store_reference_injects_in_registered_formcargo nextest run -p bep store_value_read_per_request_never_writtencargo nextest run -p bep deleted_keychain_item_refuses_next_injectioncargo nextest run -p bep reference_to_another_sign_in_instance_is_refusedcargo nextest run -p bep replaced_secret_is_injected_on_next_requestcargo nextest run -p bep injected_value_is_exactly_prefix_plus_valuecargo nextest run -p bep crlf_in_value_or_prefix_refuses_injectionVerify
min task run agent-verifypassed ([tasks.verify]).Base
The base branch moved while the coder worked; it was merged in before the tests ran.
Residuals
bep servedoes not inject a store value yet. The T28 proof holds in the forwarder only untilBoxRedeemeroverridesredeem_reference(crates/bep/src/redeemer.rs) andbep servecallsFlow::with_store(Arc::new(HostStore))(crates/bep/src/proxy.rs). BEP-032 stays open until then. The epic owner keepsCloses #2292and files the open part.Closes #2292
Generated by Claude Code