aiolabs/nsecbunkerd
8
0
Fork 0
Code Issues 12 Pull requests Projects Releases Packages Wiki Activity Actions

create_new_key clobbers an existing key — silent, unrecoverable identity loss on re-pair #39

New issue
Closed
opened 2026-06-21 13:51:39 +00:00 by padreug · 1 comment
padreug commented 2026-06-21 13:51:39 +00:00
Owner
Copy link

Severity: high (silent key destruction)

src/daemon/admin/commands/create_new_key.ts unconditionally generates a fresh keypair and calls saveEncrypted(...keyName), which overwrites config.keys[keyName] on disk. There is no guard for an already-existing key name.

So invoking create_new_key with a name that already exists:

  1. generates a new keypair,
  2. overwrites the existing encrypted nsec on disk,
  3. loadNsec replaces the in-memory activeKeys[keyName] entry (the old nsec then survives only in the still-running Backend, and is gone on the next restart).

The old key is unrecoverable — no backup, no export RPC.

How it bit us

spirekeeper's pair_spire re-pairs a machine through the same spire-<machine_id> keyName and documents the assumption verbatim (pairing.py):

create_new_key is idempotent — returns the existing key if the name is taken

It isn't. A repeat /pair (re-pair) for an already-paired machine rotated the machine's spire identity (437d8fd1… → a freshly generated 679ac2a8…), orphaning everything bound to the old key — connect tokens, the published cassettes-state beacon, path-B wallet routing — with the old nsec lost. Reproduced on the dev stack 2026-06-21.

Fix

PR: make create_new_key idempotent. If a key with the name already exists, recover and return it instead of generate-and-overwrite:

  • no _nsec → decrypt the existing entry with the supplied passphrase, return its npub (the idempotent re-pair path);
  • explicit _nsec, unrecognized entry, or a passphrase that doesn't decrypt the existing key → throw rather than overwrite. Destroying a key must never be a silent side effect of "create".

Follow-ups

  • Automated regression test is currently blocked by the NDK 3.x ESM / ts-node(CJS) harness limitation (same constraint that pushed acl/index.ts to import type). Functionally verified for now; an ESM-capable test harness would let this land as a unit test.
  • Consider whether saveEncrypted itself should refuse to overwrite an existing name as defence-in-depth.
## Severity: high (silent key destruction) `src/daemon/admin/commands/create_new_key.ts` unconditionally generates a fresh keypair and calls `saveEncrypted(...keyName)`, which overwrites `config.keys[keyName]` on disk. There is **no guard for an already-existing key name**. So invoking `create_new_key` with a name that already exists: 1. generates a new keypair, 2. **overwrites the existing encrypted nsec on disk**, 3. `loadNsec` replaces the in-memory `activeKeys[keyName]` entry (the old nsec then survives only in the still-running Backend, and is gone on the next restart). The old key is **unrecoverable** — no backup, no export RPC. ## How it bit us spirekeeper's `pair_spire` re-pairs a machine through the same `spire-<machine_id>` keyName and **documents the assumption verbatim** (`pairing.py`): > create_new_key is idempotent — returns the existing key if the name is taken It isn't. A repeat `/pair` (re-pair) for an already-paired machine **rotated the machine's spire identity** (`437d8fd1…` → a freshly generated `679ac2a8…`), orphaning everything bound to the old key — connect tokens, the published `cassettes-state` beacon, path-B wallet routing — with the old nsec lost. Reproduced on the dev stack 2026-06-21. ## Fix PR: make `create_new_key` idempotent. If a key with the name already exists, recover and return it instead of generate-and-overwrite: - no `_nsec` → decrypt the existing entry with the supplied passphrase, return its npub (the idempotent re-pair path); - explicit `_nsec`, unrecognized entry, or a passphrase that doesn't decrypt the existing key → throw rather than overwrite. Destroying a key must never be a silent side effect of "create". ## Follow-ups - Automated regression test is currently blocked by the NDK 3.x ESM / ts-node(CJS) harness limitation (same constraint that pushed `acl/index.ts` to `import type`). Functionally verified for now; an ESM-capable test harness would let this land as a unit test. - Consider whether `saveEncrypted` itself should refuse to overwrite an existing name as defence-in-depth.
padreug commented 2026-06-21 14:13:23 +00:00
Author
Owner
Copy link

Fixed via PR #40 (merged to dev @ 4a0a3e7). Verified on the dev bunker: a repeat /pair for an already-paired machine now returns the existing spire key with the on-disk entry unchanged (idempotent), where before it minted a new identity and clobbered the old blob. Closing.

Fixed via PR #40 (merged to `dev` @ `4a0a3e7`). Verified on the dev bunker: a repeat `/pair` for an already-paired machine now returns the existing spire key with the on-disk entry unchanged (idempotent), where before it minted a new identity and clobbered the old blob. Closing.
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
master
dev
docs-migration-runbook
issue-9-fix-create-new-key-race
No results found.
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
aiolabs/nsecbunkerd#39
No description provided.