Repository navigation
T110 — Make bep --replace-key keep the old key until the new key exists - #2801
norrietaylor merged 17 commits into
Conversation
`replace` now generates the new key under a second label beside the held key. It moves the old key aside, moves the new key to the role's label, and then removes the old key. A failed generation or a failed move keeps the old key in use, and a failed move removes the new key. `bep --replace-key <role>` no longer tells the operator to run the same command again when no new key was made. It names the cause, says that the old key is still in use, and leaves the identity file as it was.
|
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: PASS · the full ruling
The change conforms to Gatehouse §6.10 line 839. A replacement still runs only on the operator command, and a start still never makes a key again. The old key now stays under the role's label until the new key takes it. A failed generation or a failed move keeps the old key in use. The design does not give an order for the steps of a replacement, so the new order is the coder's to choose. The macOS lane must compile the keychain relabel. Nobody compiled it on this Linux box.
Ruled at 521b8cb against gominimal/arch@8640f85e193d by claude-opus-5-5.
…eplace-key-keep-the-old-key-until-the
A keychain move out of the new slot that finds no item is now an error, so replace keeps the old key in use instead of reporting success. Narrow the replace doc to what holds when the old key is not removed, and split the Restore error text into sentences.
…eplace-key-keep-the-old-key-until-the
|
Round 1 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
When bep --replace-key cannot make a new key, the old key stays in use and the message names the cause, not the same command again. A new key that is missing when it must move in is now also a failure that keeps the old key, not a false success. Notes from the agent: Revise round 1. All three findings hold. The new test a_missing_new_key_keeps_the_old_key uses a new MemoryStore toggle, lose_new_keys. The keychain branch of relabel is macOS-only, so this Linux box did not compile it. Only the memory store mirror ran here. On Linux, missing_new is gated with cfg(any(test, target_os = "macos")) to avoid dead code. |
There was a problem hiding this comment.
arch-review: PASS · the full ruling
The ruling on 521b8cbd stands for this head. The new commit makes replace fail when the new key is not in its slot, so the old key stays in use. Section 6.10 line 839 says that a key is never regenerated silently. This change closes a path where the command reported a replacement that did not occur. The two merges bring only main. The macOS keychain branch was not compiled here, so the macOS lane must compile it.
Ruled at d8317d8 against gominimal/arch@8640f85e193d by claude-opus-5-5.
replace_outcome moves a key left in the .old slot back to the role's label when that label is empty, before the cleanup removes it. A failed generation then keeps the old key in use. When the store does not remove the old key after a replacement, bep --replace-key now says so, with the command that removes it.
|
Round 2 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
When bep --replace-key cannot make a new key, the old key stays in use, also after an earlier replacement stopped part way. The message names the cause and does not tell the person to run the same command again. When the new key is in use but the store did not remove the old key, the command now says so. Notes from the agent: Revise round 2. keys.rs: new replace_outcome returns Replaced { key, old_kept }, and replace stays as a wrapper that returns only the key. This keeps the callers in publish.rs, which is outside the layers, unchanged. replace_outcome first moves a key in the .old slot back to the role's label when find reports that label empty. main.rs: replace_key uses replace_outcome and prints this on stderr when old_kept is set: "The old key was not removed: . The proxy does not use it. Run |
…eplace-key-keep-the-old-key-until-the
There was a problem hiding this comment.
arch-review: CONDITIONS · 2 conditions · the full ruling
The new restore in replace_outcome moves any key in Slot::Old back when the role's label is empty. Slot::Old can hold a key that an earlier replacement replaced and did not remove. If that new key is later lost, the next --replace-key brings the replaced key back. On a failed generation the proxy uses the replaced key again. Gatehouse §6.10 line 839 says that members sealed to a replaced key are dead. C1 limits the restore to the state that an interrupted replacement leaves. C2 makes the test build that state.
Ruled at 91c1f6e against gominimal/arch@8640f85e193d by claude-opus-5-5.
A failed move back of a key that an earlier replacement moved aside is now ReplaceError::Recover, and bep --replace-key says the proxy has no key for the role. A replacement that keeps the old key publishes the new key and exits non-zero.
|
Round 3 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
When bep --replace-key cannot make a new key, the old key stays under the role's label and the identity file does not change. The command prints "The key was not replaced, because no new key was made: . The old key is still in use." and exits non-zero. It does not tell the person to run the same command again. In this round, a replacement that keeps the old key also exits non-zero. A key from a stopped replacement that does not move back is reported as no key in use. Notes from the agent: Revise round 3. A new ReplaceError::Recover variant reports a failed move back of a key that an earlier replacement moved aside. Its message says that the proxy has no key for the role now. A run of the command tries the move back again, so the message tells the person to run it again. replace_key now exits non-zero when the old key stays under the .old label. It still publishes the new key first, because the store already uses the new key. New tests: an_interrupted_replacement_not_put_back_is_reported (keys.rs), replace_key_exits_non_zero_when_the_old_key_stays and replace_key_names_an_old_key_not_put_back (main.rs). I did not run just clippy-strict. |
There was a problem hiding this comment.
arch-review: PASS · the full ruling
This head closes the two items that were open on 91c1f6eb. A failed move back of an old key is now ReplaceError::Recover, and the command says that the proxy has no key for the role. A replacement that keeps the old key now exits non-zero. Gatehouse §6.10 line 839 says that a replacement is never a silent overlap failure. The change now conforms to that line and to BEP-060.
Ruled at 8f1a355 against gominimal/arch@8640f85e193d by claude-opus-5-5.
There was a problem hiding this comment.
minimal-review — approval withheld
Verdict COMMENT — APPROVE withheld: 7 unresolved threads — minimal-foundry[bot] crates/bep/src/keys.rs:989, minimal-foundry[bot] crates/bep/src/keys.rs, minimal-foundry[bot] crates/bep/src/keys.rs:614, minimal-foundry[bot] crates/bep/src/main.rs:285, minimal-foundry[bot] crates/bep/src/keys.rs, …. No findings.
Panel: 3 of 3 personas ran (correctness, security, supply-chain).
There was a problem hiding this comment.
Reviewed at 521b8cb against T110's plan block.
What holds:
- A failed generation keeps the old key, and
generate_incleans up its slot. - #2790's custody rule still refuses an exportable new key in a release build, through
keep_generatedingenerate_in. - A failed move puts the old key back.
- The identity file is untouched on failure.
- The plan's three tests exist and pass.
just test-one tests -p beppasses all 238.
Two key-loss paths and one message:
crates/bep/src/keys.rs:559-561with:569-574: a replacement cut off between its two moves loses the old key on the next run.- After
relabel(role, Held, Old)and beforerelabel(role, New, Held), no key holds the role's label. A kill, crash or power loss there leaves the old key at.oldand the new key at.new. - The next
bepstart refuses the role as lost. The only way out isbep --replace-key <role>, and its first step removes.newand.old. That deletes the old key, which was still recoverable, before it generates a third. - So the old key does not stay until a new key holds the role, which BEP-060 and the plan promise.
- After
crates/bep/src/keys.rs:921, with:574:relabelmapserrSecItemNotFoundtoOkfor every move.- That is right for Held→Old, where the role may hold nothing. It is wrong for New→Held, where
replacehas just generated the key. - If two
--replace-keyruns overlap, B's cleanup deletes A's.newand.old. A's New→Held then "succeeds" on nothing, and A prints success and writes an identity for a key no store holds. Values sealed to it never open.
- That is right for Held→Old, where the role may hold nothing. It is wrong for New→Held, where
crates/bep/src/main.rs:246, pinned at:3324: the custody refusal contradicts itself. In a release build where the keychain makes the new key exportable, a key was made and then refused and removed. The user reads "…because no new key was made: the key store failed for the sealing key: the keychain made the key exportable." Say what happened, with each fact in its own sentence.
Not a finding, for the macOS lane: whether SecItemUpdate can relabel a key the legacy keychain refuses to a rebuilt binary is unproven. That is the case --replace-key exists for.
|
/revise Three fixes from review at 521b8cb. Keep the plan's message for a failed generation, the identity-file behaviour, and the release custody check in
Run |
|
That Nothing to type again: the command gets a rocket when its round starts, or a refusal that names the reason. No round was counted for it yet. |
Move the old key back under the role's label only when Slot::New also holds a key, through the new KeyStore::holds_in. A replaced key left in Slot::Old after a finished replacement never comes back. A new key the keychain made exportable is now KeyError::Exportable, and bep --replace-key says that a key was made and refused, not that none was made.
|
Round 4 of 10 on a review requesting changes, pushed as
Verify Review threads the coder judged:
When bep --replace-key cannot make a new key, the old key stays in use and the identity file is unchanged. The message names the cause, not the same command again. When a release build refuses a new key that the keychain made exportable, the message says that, in separate sentences, and the old key stays in use. Notes from the agent: Review findings 1 and 2 were already fixed on the branch at 8f1a355, after the reviewed commit 521b8cb. replace_outcome moves Slot::Old back before its cleanup, and relabel from an empty Slot::New is an error in both stores. This round: (1) the recovery now runs only when the role's label is empty and Slot::New also holds a key (new KeyStore::holds_in). If holds_in fails, the replacement stops with ReplaceError::Recover and does not remove Slot::Old. (2) Finding 3: keep_generated returns a new KeyError::Exportable, and replace_outcome maps it to ReplaceError::Exportable. bep --replace-key prints "The key was not replaced. The keychain made the new key exportable. This build does not use an exportable key. The old key is still in use." This message gives no next step, because no command fixes it. The warn line for this case is "refused the new proxy key and kept the old key". The first-start refusal of an exportable key also changes. It is now "The proxy did not start: the keychain made the new sealing key exportable. This build does not use an exportable key." a_release_build_refuses_an_exportable_key_at_both_sites pins it. The macOS Keychain::holds_in is not compiled or run on this Linux box. It searches the slot's label with load_refs, and it reads a denied search as a held key so that recovery is not skipped. The macOS lane owes that evidence. |
There was a problem hiding this comment.
arch-review: CONDITIONS · 1 condition · the full ruling
The new recovery arm is correct. It does not put back a replaced key, and it stops when the store does not answer. The Exportable split is also correct. One report is false. After a ReplaceError::Restore, the next bep --replace-key removes the old key from Slot::Old. If generation then fails, the command prints "The old key is still in use." But the store holds no key for the role. Gatehouse §6.10 line 839 says the keys persist and are never regenerated silently, so the record must say what the store holds. Make the change in C1.
Ruled at a015b28 against gominimal/arch@8640f85e193d by claude-opus-5-5.
…eplace-key-keep-the-old-key-until-the
With no identity file that names a key for the role, replace_outcome and load_with now keep a key alone in the Old slot and stop, as they do for an unread identity. A failed cleanup for a role whose label held no key now says the proxy has no key, not that an old key is in use.
…eplace-key-keep-the-old-key-until-the
|
Round 8 of 10 on a /revise note, pushed as
Verify Review threads the coder judged:
When bep --replace-key cannot make a new key, the old key stays in use and the message names the cause, not the same command again. Also, a key set aside alone that no identity file names now survives both bep --replace-key and a start. A failed cleanup for a role with no key held says that the proxy has no key, not that an old key is in use. Notes from the agent: Round 8. Item 1: recover_interrupted returns a new Interrupted::Unnamed for PublishedKid::Absent when the Old slot holds a lone key. replace_outcome maps it to the new ReplaceError::Unnamed and removes nothing. load_with treats it as Unread, so the role is a lost key and the set-aside key stays. The new main.rs text names no command, because no command resolves this state. Test: an_old_key_left_alone_with_no_identity_is_kept (replace_outcome and load_with). Item 2: the cleanup and the move-aside failures return the new ReplaceError::RemoveNoKey when the label held no key on entry. Its main.rs text says the proxy has no key and names |
There was a problem hiding this comment.
minimal-review — approval withheld
Verdict COMMENT — APPROVE withheld: 9 unresolved threads — minimal-arch[bot] crates/bep/src/keys.rs:770, minimal-arch[bot] crates/bep/src/keys.rs:1822, minimal-arch[bot] crates/bep/src/keys.rs, minimal-arch[bot] crates/bep/src/keys.rs:817, minimal-arch[bot] crates/bep/src/keys.rs, …. No findings.
Panel: 3 of 3 personas ran (correctness, security, supply-chain).
There was a problem hiding this comment.
arch-review: CONDITIONS · 1 condition · the full ruling
The RemoveNoKey part of 2417b928 conforms. The Unnamed refusal does not. With no identity file and a lone set-aside key, the start refuses and says to run bep --replace-key <role>. That command now refuses every time, and no command leaves the state. Gatehouse §6.10 line 839 makes replacement an explicit command, and it says that members sealed to the old key are dead after a replacement. C1 brings back the ac4a047f behaviour: the start keeps the key aside, and the explicit replacement removes it.
Ruled at 767d394 against gominimal/arch@8640f85e193d by claude-opus-5-5.
|
/revise The owner ruled for arch C1 at 767d394. This replaces item 1 of my round-8 note. The rule:
Change nothing else. |
…eplace-key-keep-the-old-key-until-the
A start still keeps a lone set-aside key that no identity file names, and refuses. An explicit bep --replace-key now removes that key and makes a new one, because the operator asked for a replacement. This removes ReplaceError::Unnamed, its message and its test.
|
Round 9 of 10 on a /revise note, pushed as
Verify Review threads the coder judged:
When bep --replace-key cannot make a new key, the old key stays in use and the message names the cause, not the same command again. This round, a start still refuses and keeps a lone set-aside key that no identity file names, and an explicit bep --replace-key removes that key and makes a new one. Notes from the agent: Round 9 applies the owner's C1 ruling. In recover_interrupted, the PublishedKid::Absent arm returns Ok(false) again. Interrupted::Unnamed, ReplaceError::Unnamed, its arm in replace_failure and the test replace_key_names_a_key_set_aside_that_no_identity_names are removed. The test an_old_key_left_alone_with_no_identity_is_kept is renamed to an_old_key_left_alone_with_no_identity_is_kept_until_a_replacement. Its start half is unchanged. Its replacement half asserts a new key under Slot::Held and empty Slot::Old and Slot::New. The old_key_left_alone fixture leaves the memory store in a mode that loses every new key. So the test needed a new test-only MemoryStore helper, keep_new_keys, in keys.rs, to turn that mode off before the replacement. That helper is the only addition beyond the directive's list. ReplaceError::RemoveNoKey, its message and its tests are kept. cargo clippy -p bep --all-targets is clean. I did not run just clippy-strict. |
There was a problem hiding this comment.
arch-review: PASS · the full ruling
The change conforms to Gatehouse §6.10 line 839. A restart keeps the sealing key and regenerates nothing. Only bep --replace-key removes a key set aside with no identity file, and the command logs the new fingerprint. An identity file that bep cannot read still stops the replacement. The doc comment on replace_outcome at keys.rs lines 723–724 still states the old behaviour. Update it, but it is not a design condition.
Ruled at a0e0845 against gominimal/arch@8640f85e193d by claude-opus-5-5.
There was a problem hiding this comment.
Approving at a0e0845. Round 9 applies the owner's ruling on the arch C1 of 767d394:
recover_interruptedreturnsOk(false)forPublishedKid::Absent. A start keeps a lone set-aside key and refuses. An explicitbep --replace-keyremoves the key and makes a new one.Unnamed, its message and its test are gone.an_old_key_left_alone_with_no_identity_is_kept_until_a_replacementpins both halves.
Earlier rounds still hold:
- The old key moves back before the new key goes.
- A key the identity names comes back.
UnreadandCheckstop without removing anything.- Each no-key case (
NoKey,MoveNoKey,RemoveNoKey, andExportablewithheld: false) has its own message.
The rest of the diff comes from the merge of main.
|
/revise One doc fix only, from thread PRRT_kwDOPdfcxM6yTmB1. In Since round 9 that is the opposite of the code. Replace that sentence with: "When no identity file names a key for the role, a start keeps that key aside and refuses, and an explicit replacement removes it and makes a new key." Change no code and no test. |
An explicit replacement removes a set-aside key that no identity file names and makes a new key. A start keeps it aside and refuses.
|
Round 10 of 10 on a /revise note, pushed as
Verify Review threads the coder judged:
When bep --replace-key cannot make a new key, the old key stays in use and the message names the cause, not the same command again. This round also makes the replace_outcome doc comment agree with the code for a set-aside key that no identity file names. Notes from the agent: Revise round 10 changes only the doc comment, as the note asks. No code or test changed. |
There was a problem hiding this comment.
arch-review: PASS · the full ruling
This push changes one doc comment on replace_outcome in crates/bep/src/keys.rs. It changes no code. The new sentence matches what recover_interrupted and load_with do for a set-aside key that no identity file names. A start refuses, and only an explicit replacement removes the key. That keeps Gatehouse §6.10 line 839 and BEP-060. The PASS on a0e08456 stands for everything else.
Ruled at 1a64f23 against gominimal/arch@8640f85e193d 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 1a64f235d888 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.
T110 — Make bep --replace-key keep the old key until the new key exists
slice: S15
kind: task
covers: BEP-060
size: M
est: 140 code, 180 tests
executor: agent
difficulty: needs care
model: standard
diagnostics: when no new key can be made,
bep --replace-key <role>prints "The key was not replaced, because no new key was made: . The old key is still in use." and exits non-zero; the text "Runbep --replace-key <role>again." goes, pinned by replace_key_failure_keeps_the_old_key_in_useobservability: the warn line "replaced a proxy key" names the new key's fingerprint, as today; a failed generation writes one warn line naming the role and the cause, and that the old key is kept
layers:
- keys:
- crates/bep/src/keys.rs:
replacegenerates the new key beside the held one under a second label, then removes the old key and moves the new key to the role's label; when the generation fails, the old key stays andreplacereturns an error that says so; the memory store and the keychain store both do this, and a failed move of the label removes the new key and keeps the old onetest: cargo nextest run -p bep a_failed_generation_keeps_the_old_key
test: cargo nextest run -p bep refused_key_names_the_replacement_command
- command:
- crates/bep/src/main.rs:
replace_keyprints the text above for a failed generation, and the identity file is left as it wastest: cargo nextest run -p bep replace_key_failure_keeps_the_old_key_in_use
produces: none
consumes: none
proof: When bep --replace-key cannot make a new key, the old key stays in use and the message names the cause, not the same command again.
depends on:
- blocked by #2259
- blocked by #2785
Proof
When
bep --replace-key <role>cannot make a new key, the store keeps the old key and the proxy still starts with it. The command prints "The key was not replaced, because no new key was made: . The old key is still in use.", exits non-zero, and leaves the identity file as it was. It no longer tells the operator to run the same command again.Notes from the agent: Design:
KeyStorehas a newSlot(Held, New, Old) and the required methodsgenerate_in,remove_inandrelabel.generateandremoveare now default methods on the Held slot.replacefirst removes leftovers in New and Old. It then generates in New, moves Held to Old, moves New to Held and removes Old. A failed move of the new key removes the new key and moves the old key back. The task says the old key is removed before the label move. That order cannot keep the old key on a failed move, so the old key moves to a third label instead and is removed last. A newReplaceError::Movecovers a failed move. A newReplaceError::Restorecovers the case where the old key also does not move back. For that casebep --replace-keyprints that the proxy has no key for the role and tells the operator to run the command again, because then the old key is really gone. The keychain store (macOS only) usesSecItemUpdateonkSecAttrLabelforrelabel. It declaresSecItemUpdate,kSecClassandkSecClassKeyin the local extern block. This box is Linux, so the keychain code was NOT compiled or run here.just cion macOS or the macOS lane must compile it. Tests: the old testsa_failed_rep 8000 lacement_refuses_the_next_startanda_failed_generation_reports_the_old_key_removedasserted the old behaviour (old key deleted). They are nowa_failed_move_keeps_the_old_keyanda_failed_generation_keeps_the_old_key. A new test,a_replacement_leaves_only_the_new_key, was added.a_lost_key_with_no_identity_file_refuses_the_startnow removes the key directly, because a failed rep…[truncated]Tests
cargo nextest run -p bep a_failed_generation_keeps_the_old_keycargo nextest run -p bep refused_key_names_the_replacement_commandcargo nextest run -p bep replace_key_failure_keeps_the_old_key_in_useVerify
min task run agent-verifypassed ([tasks.verify]).Closes #2784