Repository navigation
feat(bep): verify store handles against the client's registered key and the current rule - #2467
Conversation
Add the store-handle wire format (an ES256 compact JWS), the registry of client keys, and the BEP-064 check. The proxy reads the rule for each request, so an edited [secret-store-rules] rule decides the next request. A sign-in reference has the GitHub module as its rule. The redemption decision makes the handle check after the revocation check, and refuses a bad handle as store_handle_invalid. Add a Kani harness for it and raise the bep harness count in scripts/kani.sh.
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
|
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 · 4 conditions · the full ruling
The check is in the right place and fails closed. But the handle does not use the design's store-handle wire format. Gatehouse §6.10 line 842 gives typ "gatehouse-store-handle+jws" and a payload with sub, node_id, iat and jti. It also requires the BEP to check sub and node_id against the sealed value's context after the signature. Without that check, a handle copied from one box can be re-sealed for another box and pass, because the envelope proves no origin. C1 and C2 fix the format and add the check. C3 moves kind to the member, and C4 amends BEP-063 and BEP-064 to match the design.
Outside the diff
docs/specs/24-spec-box-egress-proxy/24-spec-box-egress-proxy.md:481: C4 · Add the context case to BEP-064 and its property: refuse when the handle'ssubornode_iddiffers from the sealed value's context. Amend BEP-063 (line 464) to listsub,node_id,iatandjti.
Ruled at d398872 against gominimal/arch@109541a48c31 by claude-opus-5-5.
Add handle::read_opened, which reads the handle from the sealed member's own jws. A handle of another kind than the member, or a value opened for another module, has no rule and is refused as store_handle_invalid. A sign-in reference whose store is not the module's id now has no rule. The debug line logs the unverified store and id with Debug escaping.
…ned-handles-against-the-client-s-regis
|
Round 1 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
In crates/bep, a handle with a bad signature, a handle past its expiry, or a handle wider than its rule is refused as store_handle_invalid. An edited [secret-store-rules] rule decides the next request. The handle that the decision judges is now read from the opened member's own jws, with handle::read_opened. Notes from the agent: Revise round 1. Added handle::read_opened. It reads the handle from Opened.member.jws, and it gives no rule when the handle kind is not the member kind or when the value was opened for another module. decide stays a pure function over facts. The binding is in how the shell builds Redemption.handle, and the Redemption.handle doc says so. The bound set that comes from the handle's upstream stays as it is. Spec BEP-032's design note says that the handle's upstream is the bound set, and the handle is sealed inside the member. Deferred: cargo nextest run -p bep kani_redeem_refuses_invalid_store_handle (lane: kani). The harness is from round 0 and is unchanged, and this round did not run it. just clippy-strict did not run. |
There was a problem hiding this comment.
Own review of dcfed82 (opening commit d398872, round 1 commit 3ff6ee8, base merge) against the T7 plan block (BEP-064).
- Layers:
crates/bep/src/handle.rs(new),redeem.rs,lib.rs,scripts/kani.sh. Nothing outside the task's layers; no Cargo change (ES256 rides p256's default feature). - Wire format and registry: compact JWS
header.payload.signature, ES256 overheader.payload,kidas the RFC 7638 thumbprint;ClientKeysregisters SEC1 keys andverifyrefuses a tampered or foreign signature (testa_tampered_or_foreign_signature_does_not_verify). - BEP-064 check:
Handle::checkis pure and ordered Signature, Expired, NoRule, OutsideRule, InjectMismatch;deciderefuses asstore_handle_invalidafter Revoked and before OffPort, and the oraclefailures() plusadmits_iff_every_check_passescarry the new check.prop_store_handle_verified_against_current_rule(256 cases) and the Kani harness count (bep 10) are in place; the Kani line is the deferred one the plan allows. - Rule read per request:
readandread_openedtake aRulessource and keep no copy;edited_store_rule_applies_to_next_requestshows narrowed, reformed, removed and restored rules deciding the next request.sign_in_reference_is_judged_against_the_modulejudges a reference against the module's authorities with no injection. - Round 1:
read_openedreads the handle from the opened member's ownjws, so a handle of another kind or another module has no rule (the HIGH binding finding); a sign-in reference whosestoreis notmodule.idhas no rule; the debug line logs the unverifiedstoreandidwith Debug escaping and never the signature. - Open: arch-review's four conditions on d398872 (the design's
typand payload fieldssub/node_id/iat/jti, aContextMismatchcheck binding the handle to the opened value's box id and host, the kind taken from the sealed member rather than the JWS payload, and the BEP-063/BEP-064 spec amendment) are not yet carried; they are the next round. The plan's diagnostics line (the audit marker inmin box audit) waits on the proxy shell wiring, as the body says.
Retitled to feat(bep): verify store handles against the client's registered key and the current rule, which lints.
Generated by Claude Code
There was a problem hiding this comment.
arch-review: CONDITIONS · 6 conditions · the full ruling
The four BEP-064 checks are correct, and the compare binds the handle to the opened member. But the handle is still not in the format of Gatehouse §6.10 line 842. The typ is min-handle, and the payload has no sub, node_id, iat or jti. So the proxy does not compare the handle's box and node with the sealed context, and a handle can be sealed into another box's value. Make the changes in C1 to C4, amend BEP-064 in the spec (C5), and file the two residuals (C6). The escalation asks the design owner what td and env hold on an un-enrolled host.
Ruled at dcfed82 against gominimal/arch@109541a48c31 by claude-opus-5-5.
Follow the Gatehouse store-handle wire format: typ gatehouse-store-handle+jws, and sub, node_id, iat an 10000 d jti in the payload. The sealed member gives the kind, not the payload. A handle whose sub or node_id differs from the opened value refuses as context_mismatch. read is private, so only read_opened builds a handle, from the sealed member.
|
Round 2 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
The proxy refuses a handle as store_handle_invalid when its signature does not verify under a registered client key, when its sub or node_id is not the box and Box Host of the sealed value, when it is past expiry, or when it is wider than its rule or its inject differs. An edited [secret-store-rules] rule decides the next request, and the unit tests in crates/bep/src/handle.rs and redeem.rs show each case. Notes from the agent: Revise round 2. Deferred: cargo nextest run -p bep kani_redeem_refuses_invalid_store_handle (lane: kani). The harness now also models the new Handle::bound fact, and I did not run it. A full run of cargo nextest run -p bep passed (97 tests). seal.rs is outside this task's layers and MemberKind has only SignIn, so read_opened reads sign-in references only. A later task that adds a store member kind must add one arm to the exhaustive match in read_opened. The store path of the check is covered through the private read in the unit tests. |
There was a problem hiding this comment.
minimal-review — approval withheld
Verdict COMMENT — APPROVE withheld: 15 unresolved threads — minimal-foundry[bot] crates/bep/src/handle.rs:502, minimal-foundry[bot] crates/bep/src/handle.rs:530, minimal-arch[bot] crates/bep/src/handle.rs, minimal-arch[bot] crates/bep/src/handle.rs:83, minimal-arch[bot] crates/bep/src/handle.rs, …. No findings.
Panel: 3 of 3 personas ran (correctness, security, supply-chain).
There was a problem hiding this comment.
arch-review: PASS · the full ruling
The compare meets the store-handle wire format of Gatehouse §6.10 line 842. The typ is gatehouse-store-handle+jws. The payload carries sub, node_id, iat and jti. Handle::check now verifies the signature, then sub and node_id, then exp, then the current rule. That is the order the design gives. Every failure refuses as store_handle_invalid. BEP-064 conforms, and this ruling has no conditions.
Ruled at 38f9925 against gominimal/arch@109541a48c31 by claude-opus-5-5.
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 38f9925c87e7 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.
|
/revise arch-review's ruling on dcfed82 (review 5593020858, run 118942596082) carried two conditions that 38f9925 did not touch; the PASS on 38f9925 read only the code. Carry them in substance: C5 — amend spec 24. In C6 — list the residuals in the PR body. Add a No code change is asked. Reply on the C5 threads if any open, else none. Generated by Claude Code |
…ned-handles-against-the-client-s-regis
BEP-063 names the handle's typ and the sub, node_id, iat and jti claims. BEP-064 and its property refuse a handle whose sub or node_id differs from the sealed value's box or host, as Gatehouse §6.10 does.
Spec lint
Advisory. None of this blocks the merge; the review decides. Advisory, and never a merge gate. Linter: |
There was a problem hiding this comment.
Own review of bcad687 (round 3 commit, docs only, plus a base merge) against arch-review's condition C5 on dcfed82.
- BEP-063 now names the
typgatehouse-store-handle+jwsand the payload fieldssub,node_id,iatandjtibeside the store, the identifier, the upstream authorities, the injection form and the expiry. BEP-064 and itsproperty:line add the case "itssubornode_iddiffers from the sealed value's box or host". Both match Gatehouse §6.10 line 842 and the code at 38f9925. No code changed. - The PR body carries a Residuals section naming #2468, #2469 and gominimal/arch#139 (condition C6).
- The earlier review of dcfed82 stands for the code. The squash subject stays the PR title, which lints.
Generated by Claude Code
|
Round 3 of 10 on a /revise note, pushed as
Verify Outside the task's
Converted to a draft: something above needs a person before review. A handle with a bad signature, past expiry, wider than its rule, or bound to another box or Box Host is refused, and an edited rule decides the next request. Spec 24 now records the handle's typ and its sub, node_id, iat and jti claims, and BEP-064 names the box and host binding. Notes from the agent: Revise round 3. C5: the spec 24 file is outside this task's layers. The /revise note asked for the change, so I made it. BEP-063 now names the JWS typ gatehouse-store-handle+jws and the sub, node_id, iat and jti claims. BEP-064 and its property line add the sub or node_id binding to the sealed value's box and host. No code changed. Vale is not installed on this box, so just lint-prose did not run. C6: I cannot edit the pull request body. Please add this to the body. ## Residuals: #2468 (the audit record and min box audit carry store_handle_invalid with store, identifier and jti once the shell calls decide). #2469 (seal::MemberKind::Store, outside this task's layers, T27 #2288). gominimal/arch#139 (what td and env hold un-enrolled; kind stays on the member). Deferred: cargo nextest run -p bep kani_redeem_refuses_invalid_store_handle (lane: kani), not run. The directive listed no review threads. |
There was a problem hiding this comment.
arch-review: PASS · the full ruling
The compare since the PASS on 38f9925 is a clean merge of main and one spec commit. The bep code is unchanged. The spec commit adds the sub and node_id binding to BEP-063 and BEP-064. That text now follows Gatehouse §6.10 line 842 and the code at handle.rs line 512. BEP-064 conforms, and there are no conditions.
Ruled at bcad687 against gominimal/arch@109541a48c31 by claude-opus-5-5.
|
Round 4 of 10 on the runtime's review findings, pushed as
Verify Outside the task's
Converted to a draft: something above needs a person before review. Review threads the coder judged:
A handle with a bad signature, past expiry, or wider than its rule is refused as store_handle_invalid, and an edited rule decides the next request. The debug line for a handle whose upstream does not parse now names unparsed_upstream, not signature. Notes from the agent: Revise round 4: one finding addressed in crates/bep/src/handle.rs. The refusal is unchanged. Only the debug outcome changed, through a new pure function outcome() with the unit test unparsed_upstream_is_logged_apart_from_the_signature (1 passed). Deferred: cargo nextest run -p bep kani_redeem_refuses_invalid_store_handle (lane: kani), not run on this box. just clippy-strict was not run. |
There was a problem hiding this comment.
arch-review: PASS · the full ruling
The new commit changes only the outcome text of the handle debug line. A verified handle with an upstream that does not parse now logs unparsed_upstream. The refusal does not change: Handle::check still refuses that handle, and the audit marker stays store_handle_invalid (Gatehouse §6.10 line 842). The PASS on bcad6877 stands for this head.
Ruled at 9f51ebc against gominimal/arch@109541a48c31 by claude-opus-5-5.
There was a problem hiding this comment.
Own review of 9f51ebc (round 4 commit) against the LOW it answers: outcome() names unparsed_upstream when the signature verifies but an upstream authority does not parse, so the debug line no longer reads signature for a parse failure; the refusal path is unchanged and a unit test pins the three outcomes. Sound.
One lane is red on this head: commitlint fails on this commit's body (a line over 100 characters, body-max-line-length). The squash message carries every commit body, so that line must be rewrapped before the merge; a /revise asks for it. The earlier reviews stand.
Generated by Claude Code
|
/revise the Reword that one commit's message so every body line is 100 characters or fewer (wrap the sentence), and push the branch with the rewritten commit; the subject Generated by Claude Code |
|
Round 5 of 10 on a /revise note: the coder changed nothing. The runtime cannot rewrite a pushed commit. The commitlint failure is real: the body of 9f51ebc has one line of 144 characters, and the limit is 100. The fix is to reword that commit and force-push the branch. The coder must not rewrite the commits on the branch or force-push, so I made no change. A new commit on top does not help, because commitlint checks every commit in the pull request. A person with push rights can fix it. Run |
A handle that verifies but names an upstream that does not parse is still refused, but its debug line now says unparsed_upstream, not signature. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J3Guwr3mQPqSeGMcjzVjTd
9f51ebc to
926919d
Compare
There was a problem hiding this comment.
arch-review: PASS · the full ruling
This head has the same tree as 9f51ebc0, which had a PASS. Only the commit signature changed. BEP-064 conforms to Gatehouse §6.10 line 842, as before. The Kani harness is deferred and still owes evidence.
Ruled at 926919d against gominimal/arch@109541a48c31 by claude-opus-5-5.
decide now refuses an authority outside the handle's own upstream as outside_bound_set, so a bound set wider than the checked handle cannot widen where the value goes. Adds a test for the mismatch.
|
Round 6 of 10 on the runtime's review findings, pushed as
Verify Outside the task's
Converted to a draft: something above needs a person before review. Review threads the coder judged:
The proxy refuses a handle with a bad signature, a handle past its expiry, or a handle wider than its rule, and audits it store_handle_invalid. The proxy reads the rule on each request, so an edited rule decides the next request. A request to an authority outside the handle's own upstream is now refused too, even when the bound set the shell passes is wider. Notes from the agent: Revise round 6. decide refuses a connection authority that is not in the handle's upstream, as outside_bound_set, beside the bound-set check (check 9). New test bound_set_wider_than_handle_upstream_is_refused covers the mismatch. The test helper reached now puts the authority in the handle's upstream, and in its rule's, only when the upstream lacks it, so the handle's verdict does not change. The BEP-064 property now sends through reached. The Kani harnesses for BEP-021 and BEP-025 read the handle's upstream too. No harness was added, so scripts/kani.sh is unchanged. Deferred (lane: kani): cargo nextest run -p bep kani_redeem_refuses_invalid_store_handle. I did not run it. The full bep suite passed once at the end: 99 passed. I did not run just clippy-strict. |
There was a problem hiding this comment.
arch-review: PASS · the full ruling
This push adds one check. decide now refuses an authority outside the handle's own upstream, so a wider bound set cannot widen where a value goes. Gatehouse §6.10 line 842 says the registration gives the authorities the value may be sent to. The handle's upstream must be a subset of that registration. So the new check enforces the design's bound and fails closed. The tests and the Kani harnesses carry the same check, and no gate is relaxed. The ruling on the earlier head stands for all other parts of the change.
Ruled at 77a5787 against gominimal/arch@109541a48c31 by claude-opus-5-5.
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 77a57878ffb3 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.
Own review of 77a5787 (round 6 commit) against the MEDIUM security finding it answers.
decidenow refuses asOutsideBoundSetwhen the authority is outside the handle's ownupstream, even ifbound_setandcurrent_setadmit it, so a shell that fillsbound_setwider than the checked handle cannot widen where the value goes. The module doc's check 9 says so.- The
failures()oracle,admits_iff_every_check_passes, the BEP-064 proptest (throughreached(), which keeps the handle and its rule consistent with the authority so only the new clause moves) and both Kani harnesses carry the same clause.bound_set_wider_than_handle_upstream_is_refusedpins the mismatch and the admit on the handle's own authority. Sound. - Nothing else changed;
handle.rsis as reviewed at 38f9925. The commit message lints. The squash subject stays the PR title.
Generated by Claude Code
There was a problem hiding this comment.
Babysitter approval for T7 (closes #2270) at 77a5787.
All twelve gates pass on this head: own review, arch review PASS, the runtime's review concluded with every thread resolved, zero open threads, every lane aggregator green (ci-success, ci-linux-native-success, ci-linux-kvm-success, ci-macos-success, ci-shell-installer-success, ci-kani-success), CodeRabbit and license/cla green, not a draft, no needs-human, no implement in flight. The squash subject is a Conventional Commits feat(bep) subject under the length limit. Residuals are filed as #2468, #2469 and gominimal/arch#139.
Enqueuing a squash merge.
Generated by Claude Code
T7 — Verify signed handles against the client's registered key and the current rule
slice: S0
kind: task
covers: BEP-064
size: L
est: 300 code, 240 tests
executor: agent
difficulty: needs care
model: standard
diagnostics: a refused handle is audited
store_handle_invalid, andmin box audit <box>shows the marker with the store and identifierobservability: one debug log line per handle checked with the store, the identifier and the outcome, never the signature
layers:
- handle:
- crates/bep/src/handle.rs (new): the store-handle wire format, the client-key registry, and the BEP-064 check against the rule read per request
- crates/bep/src/redeem.rs: the handle check stands where the module checks read a host set, mode and breadth
- crates/bep/src/lib.rs: module line
- scripts/kani.sh: the added harness's count
test: cargo nextest run -p bep prop_store_handle_verified_against_current_rule
test: cargo nextest run -p bep edited_store_rule_applies_to_next_request
test: cargo nextest run -p bep sign_in_reference_is_judged_against_the_module
test (deferred: kani): cargo nextest run -p bep kani_redeem_refuses_invalid_store_handle
produces:
- crates/bep/src/handle.rs
consumes: none
proof: A handle with a bad signature, past expiry, or wider than its rule is refused, and an edited rule decides the next request.
depends on:
- blocked by #2268
Proof
In crates/bep, redeem::decide refuses a handle as store_handle_invalid in four cases: its signature does not verify under a registered client key, it is past its exp, its upstream is not a subset of the current rule, or its inject differs from the rule. The tests show this with real ES256 handles. handle::read reads the rule again for each request, so a rule that is narrowed, has a new form, or is removed refuses the next request, and a restored rule admits it again. A sign-in reference is judged against the GitHub module.
Notes from the agent: Deferred (lane: kani): kani_redeem_refuses_invalid_store_handle is written in redeem.rs kani_proofs, and the bep count in scripts/kani.sh is now 10. I did not run it. Design: Redemption gains a third type parameter I (the injection form, default handle::Inject) and a handle: Handle<H, I> field. The check runs after Revoked and before OffPort. For a store handle the shell must give the handle's upstream as bound_set and the rule's authorities as current_set (see the redeem.rs module doc). No shell calls decide yet, so the audit record and min box audit do not show store_handle_invalid until the proxy shell is wired. Check::StoreHandleInvalid.marker() and handle::Read::identifier (":") are the values that wiring must put in the record. handle::read writes one tracing::debug line per handle with store, id and outcome, and never the signature. seal::MemberKind has no Store variant (seal.rs is outside the layers), so the handle kind comes from the JWS payload. ES256 uses p256's default ecdsa feature, which was already on, so no Cargo.toml change. Whole-crate run: cargo nextest run -p bep, 96 passed. That run found that the shared valid_handle fixture expired inside other tests' clock range, and I fixed it before the commit. I deleted the generated crates/bep/proptest-regressions/ directory and did not commit it.
Tests
cargo nextest run -p bep prop_store_handle_verified_against_current_rulecargo nextest run -p bep edited_store_rule_applies_to_next_requestcargo nextest run -p bep sign_in_reference_is_judged_against_the_moduleVerify
min task run agent-verifypassed ([tasks.verify]).Deferred
The plan marked these test lines deferred: the executor lacks the lane, so nothing here ran them and they still owe evidence.
cargo nextest run -p bep kani_redeem_refuses_invalid_store_handleCloses #2270
Residuals
min box audit <box>carrystore_handle_invalidwith the store, the identifier andjtionce the proxy shell (T10 T10 — Accept only box connections and terminate TLS for the module's hosts over HTTP/1.1 #2271 / T11 T11 — Validate the upstream before substituting, pass unsealed requests through, and stream responses #2272) callsdecidewith a handle built throughread_opened.seal::MemberKind::Store, so a store handle can be sealed into a member and read throughread_opened;crates/bep/src/seal.rsis outside this task's layers (T27 T27 — Mint store handles and keep the client's key registered across proxy restarts #2288 is the home).tdandenvhold in an un-enrolled store handle, and thatkindstays on the sealed member, not in the JWS payload (minimal-arch's escalation on dcfed82).Generated by Claude Code