Repository navigation
feat(bep): revoke or replace a registered client key and bound the registry (T63) - #2571
norrietaylor merged 18 commits into
Conversation
The control socket takes revoke_client_key by kid and replace_client_key, which registers a new key and revokes every other one. A removal leaves the registry and the key file before the reply, so it survives a restart. The registry holds at most a set count of keys and can drop a key that nobody registered within an idle period.
When the keychain refuses the client's signing key, local_client_key sends the replacement with replace_client_key before the keychain keeps it. The proxy revokes every other client key, so a store handle signed under the refused key no longer verifies. tests.rs only learns the two new control socket requests and the new reply field.
|
Important Review skippedBot user detected. To trigger a single review, invoke the
 |
The inventory of daemon connections failed on the two new sites T63 added. Both reach the proxy control socket, not the daemon, so they are ungated.
|
Round 1 of 10 on a failed check suite, pushed as
Verify Outside the task's
Converted to a draft: something above needs a person before review. A store handle signed under a key the client revoked or replaced is refused by the proxy, also after a restart, and the registry never holds more keys than its bound. The minimal test suite is green again on this branch. Notes from the agent: The failed check was tests::every_daemon_connection_is_classified in crates/minimal/src/tests.rs. The earlier round added two ControlClient::connect sites in mint.rs: local_client_key_at and the test a_replaced_client_key_is_revoked_at_the_proxy. Both connect to the proxy control socket, not to the daemon. This round records them as ungated in the inventory, with a comment. crates/minimal/src/tests.rs is outside the layers: lines, but the earlier round already touched it and the minimal test target compiles it. ci-linux-native-success is the lane aggregator and failed only because the tests job failed. The directive listed no review threads. |
|
Approved: no findings at Nothing to do. Verdict APPROVE, posted as a comment: the App cannot review its own pull request; the check and the label carry it. Clobber: none seen. |
…replace-a-registered-client-key-at-the
There was a problem hiding this comment.
arch-review: PASS_WITH_CONDITIONS · 2 conditions · the full ruling
The change conforms to Gatehouse §6.10 lines 839 and 842. Revocation and the bound use the owner-only control socket, and a replacement fails closed. Two items are open. The spec does not record that a key replacement ends the store references of running boxes, and its verify line names a test that no longer proves the replacement (C1). The PR body lists two undone items without issues (C2). File the issues and link them from the PR body.
Ruled at 9a7da10 against gominimal/arch@109541a48c31 by claude-opus-5-5.
A replacement revokes every other key because one user keeps one client key. The proxy binary uses the default bound, with no setting.
|
Round 2 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 store handle signed under a key that the client revoked or replaced, and a restart does not change this. The registry never holds more keys than its bound. The docs in crates/bep/src/control.rs now state that the registry serves one client for each user, and that the proxy binary uses the fixed default bound. Notes from the agent: This round changes only docs in crates/bep/src/control.rs. Thread 1: the keychain item has a fixed service and account, so each user has one client key. I kept the revoke-all behavior and documented the single-client assumption. I also corrected the DEFAULT_MAX_KEYS doc, which said 'many clients'. Thread 2: a setting for the bound needs crates/bep/src/main.rs, which is outside this task's layers. I documented that the binary uses KeyBound::default, a fixed count and no idle period. A later task must add a flag in main.rs to make the idle bound reachable in production. |
There was a problem hiding this comment.
arch-review: PASS_WITH_CONDITIONS · 1 condition · the full ruling
The new ops conform. They run on the owner-only control socket, and a revoked key goes through the audited store_handle_invalid path that §6.10 line 842 requires. The commit since the last ruling changes only doc comments, and its claims are true. One question is open. A key replacement now revokes the old key, so running boxes lose their store references. §6.10 line 839 does not list that as a reason to re-create a box. C1 records this behaviour and the question in spec 24. The escalation asks the design owner to decide.
Ruled at 6427cb2 against gominimal/arch@109541a48c31 by claude-opus-5-5.
|
/revise Round 3 of #2571, on two review findings at 6427cb2. No round is in flight. The epic owner accepted on 2026-10-25 that a replacement revokes every other client key. Keep that design. The spec work is #2578, and the design question is gominimal/arch#144. Do not change the revoke-all behaviour or the refusal codes.
Keep to Generated by Claude Code |
…replace-a-registered-client-key-at-the
A replacement now registers the new key, then stores it, then revokes the others. A failed keychain write leaves the old keys registered. A failed revocation is logged at warn. Control::bind_with_key_bound applies the bound to the restore, so a larger bound keeps every key the file holds. It replaces with_key_bound, which restored under the default bound first.
…replace-a-registered-client-key-at-the
|
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. Review threads the coder judged:
A store handle signed under a key that the client revoked or replaced is refused by the proxy, after a restart too. The registry never holds more keys than its bound. A replacement that the keychain does not keep now leaves every old key registered. An embedder's bound now applies when the proxy restores the key file. Notes from the agent: Commit 4017c63. The revoke-all behaviour and the refusal codes are unchanged. Control::with_key_bound is removed and replaced by Control::bind_with_key_bound(path, registry, boxes, bound), which Control::bind calls with KeyBound::default(). Nothing in the tree called with_key_bound. The module doc now names the new function. The directive asks me to reply on the threads and resolve them. The runtime posts the replies from the thread verdicts. I resolve no thread, because the reviewer decides that. cargo clippy -p bep -p minimal --all-targets gave no warnings in either crate. I did not run just clippy-strict. |
|
Revise round 1 ran against |
|
/revise Round 4 of #2571, on the red
One commit, Conventional Commits subject, body lines at most 72 characters. Generated by Claude Code |
…replace-a-registered-client-key-at-the
Round 3 added two ControlClient::connect sites in mint.rs: a second connect in local_client_key_at, and one in the test a_replacement_the_store_does_not_keep_revokes_no_key. Both reach the Box Egress Proxy control socket, not the daemon, so list them as ungated in every_daemon_connection_is_classified.
A known key inside the bound now updates its time in memory only. The file takes the time at the next write, at most a minute later.
A creation that waits on the lock reads the store again and uses a key that another creation kept, so a revoke never removes the stored key.
…replace-a-registered-client-key-at-the # Conflicts: # crates/bep/src/control.rs # crates/minimal/src/tests.rs
|
Round 6 of 10 on the runtime's review findings, pushed as Resolved conflicts in:
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 store handle signed under a key that the client revoked or replaced, and it refuses it after a restart too. The registry never holds more keys than its bound. Two creations that both get a keychain refusal now replace the key one after the other, so the revoke never removes the key that the keychain keeps. A known key that registers again does not rewrite and fsync the key file each time. Notes from the agent: Round 6. bep: KeyFile::register keeps a known key's new time in memory only, when the last register write is less than a refresh interval old and the bound drops nothing. The interval is a minute, or a quarter of the idle period when that is shorter. A restart can lose up to that much of a key's registration time. New test: a_key_removal_survives_a_restart_without_a_write_for_each_registration. minimal: a lock file |
|
The runtime will not revise this pull request again on its own: the diff is +1147/-136 (1283 lines), past 3x the plan's estimate of 380 lines (est: 180 code, 200 tests, as #2543 reads now), at round 6 (the last trigger was the runtime's review findings). A person decides now. Remove |
|
/revise Round 7 of #2571, on the red
One commit, Conventional Commits subject, every body line at most 72 characters. Generated by Claude Code |
…replace-a-registered-client-key-at-the
The new mint.rs test for two creations connects to the proxy control socket, not the daemon. List it as ungated in the connection inventory of every_daemon_connection_is_classified, in sorted order.
|
Round 1 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 store handle signed under a key the client revoked or replaced is refused by the proxy, after a restart too, and the registry never holds more keys than its bound. This round makes the connection inventory test pass again, so the tests job can go green at the new head. Notes from the agent: Revise round: I added the inventory line "mint.rs::a_replaced_client_key_is_revoked_at_the_proxy_once_for_two_creations = ungated" to every_daemon_connection_is_classified. I did not put it at the place the directive gave. The test compares the inventory against a sorted list, and "_after_a_failed_revoke" sorts before "_once_for_two_creations". At the directive's place the test failed with a pure ordering diff. So the line goes after the _after_a_failed_revoke entry. The test function has two ControlClient::connect calls, but the inventory records one entry per function, and the test passes with one line. Nothing else changed. One commit. I did not run just ci or clippy-strict. |
|
The runtime will not revise this pull request again on its own: the diff is +1148/-136 (1284 lines), past 3x the plan's estimate of 380 lines (est: 180 code, 200 tests, as #2543 reads now), at round 1 (the last trigger was a /revise note). A person decides now. Remove |
241990b to
f165d2e
Compare
There was a problem hiding this comment.
arch-review: PASS_WITH_CONDITIONS · 2 conditions · the full ruling
The compare adds a lock on the client key replacement and fewer key file writes. Neither change moves a check across the control socket boundary. Neither breaks first-use registration (Gatehouse §6.10 line 842). The baseline conditions C1 and C2 stay open as filed issues #2578, #2579 and #2580. The re-creation question stays with gominimal/arch#144. You can merge with these open.
Ruled at f165d2e against gominimal/arch@109541a48c31 by claude-opus-5-5.
There was a problem hiding this comment.
Babysitter approval at f165d2e: every merge gate passes at this head (arch PASS at f165d2e, zero open threads, every lane aggregator green). Squash title feat(bep): revoke or replace a registered client key and bound the registry (T63) and the squashed message pass commitlint. Queued for squash merge.
Generated by Claude Code
T63 — Revoke or replace a registered client key at the proxy and bound the registry
slice: S9
kind: task
covers: BEP-063
size: M
est: 180 code, 200 tests
executor: agent
difficulty: needs care
model: standard
diagnostics: the proxy log records each key removal with its identifier, and a store handle under a revoked key is refused with a reason naming the key
observability: one info line per removal; one warn line when the bound drops a key
layers:
- control:
- crates/bep/src/control.rs: a revoke request from the socket's owner removes a key from the registry and the key file; the registry drops keys past a configured count or idle period
- crates/minimal/src/mint.rs:
local_client_keyrevokes the key it replaces after a refusaltest: cargo nextest run -p bep a_revoked_client_key_no_longer_verifies_a_store_handle
test: cargo nextest run -p bep a_key_removal_survives_a_restart
test: cargo nextest run -p minimal a_replaced_client_key_is_revoked_at_the_proxy
produces: none
consumes: none
proof: A store handle signed under a key the client revoked or replaced is refused by the proxy, after a restart included, and the registry never holds more keys than its bound.
depends on:
- blocked by T27
- blocked by T59
- assumes OQ3
Proof
The proxy now refuses a store handle signed under a client key that was revoked by kid or replaced after a keychain refusal. The refusal holds after a proxy restart, because the key file is rewritten before the reply. The registry holds at most KeyBound::max_keys keys (16 by default) and drops the key registered least recently. With an idle period set, it also drops a key that nobody registered within that period.
Notes from the agent: Design: the client cannot read the key that the keychain refused, so it cannot name that key. local_client_key therefore sends a new control op, replace_client_key. This op registers the new key and revokes every other registered key. It runs before the keychain stores the new key, so a proxy that is down or refuses leaves the old key in place and the next creation tries again. A side effect: on the refusal path only, local_client_key now needs the proxy to be running. create.rs already connects to the proxy before it calls local_client_key, so the behaviour of box creation does not change. Revoking one key by kid is the other new op, revoke_client_key. The key file format changed to ' '. Old lines with no time still load. Each change rewrites the file and renames it into place, and the old append path is gone.
Out of scope, touched: crates/minimal/src/tests.rs. Its two fake control servers match on Request exhaustively and build a Reply literal, so the crate did not compile without the two new variants and the new
revokedfield. The change only adds those arms and the field.Not done, outside the layers: (1) main.rs passes no KeyBound, so
bep serveuses the default (16 keys, no idle period). Control::with_key_bound exists for a flag or setting in a later task. The idle default is None because the root validity, which sets how long a handle lives, is an open question (OQ3 / gominimal/arch#85). (2) The diagnostics line asks for a handle refusal reason that names the key. Verification is in …[truncated]Tests
cargo nextest run -p bep a_revoked_client_key_no_longer_verifies_a_store_handlecargo nextest run -p bep a_key_removal_survives_a_restartcargo nextest run -p minimal a_replaced_client_key_is_revoked_at_the_proxyVerify
min task run agent-verifyexited 101 under rustc 1.98.1, which the repository does not pin (1.97.0 in rust-toolchain.toml); the PR is not a draft on that alone — CI, which honours the pin, is the gate.Layer exception
The epic owner's rule accepts out-of-layer changes a task needs. The babysitter accepted this one on 2026-10-25:
crates/minimal/src/tests.rs: its two fake control servers matchRequestexhaustively and build aReplyliteral, so the crate does not compile without the two new arms and the newrevokedfield.Residuals
On 2026-10-25 the epic owner accepted that
replace_client_keyrevokes every other client key. Running boxes that hold references under the replaced key need re-creation.-p minimaltest.bep servepasses aKeyBoundfrom a flag or setting.unknown_keynames itskidin the log line.userCanceled) also starts the replacement. The epic owner accepted the behaviour for this PR on 2026-10-25. T71 — Amend BEP-063 for client-key revocation, replacement and the registry bound #2578 carries the spec requirement and the[NEEDS CLARIFICATION]entry.Out of scope
These files are outside the task's
layers:, so this PR is a draft:crates/minimal/src/tests.rsCloses #2543
Generated by Claude Code