CreatePGPUnlocker leaves a partial unlocker directory on disk when fingerprint resolution fails #48

New Issue

clawbot · 2026-08-09T03:44:00+02:00

clawbot commented

2026-08-09 03:44:00 +02:00

From the 1.0 security survey. Small, but it produces exactly the corrupt-unlocker state that #42 is about handling gracefully — better to not create it.

Problem

CreatePGPUnlocker (internal/secret/pgpunlocker.go:260) writes its on-disk state across several steps, and the last validation happens after most of the writing is done:

:249-304 create the unlocker directory, write pub.txt, and write longterm.age.
:324 then re-resolves the GPG fingerprint, feeding writePGPUnlockerMetadata at :341.

If that resolution fails — the key was removed from the keyring between the pre-check and here, the agent died, a smartcard was unplugged — the function errors out having already written an unlocker directory with no metadata file. That is precisely the shape that broke secret unlocker list in issue #1, and the same shape that currently makes PGPUnlocker.GetID() panic (#42).

The pre-flight validation is good and already exists: internal/cli/unlockers.go:671-674 calls ResolveGPGKeyFingerprint before CreatePGPUnlocker, and preparePGPUnlockerDir calls checkGPGAvailable(). So the window is narrow. It is not zero, and the consequence is a vault the user cannot list.

Definition of done

CreatePGPUnlocker either completes fully or leaves no trace. Any failure after the unlocker directory is created removes it before returning.
Cleanup runs on every error path, including a failure of the cleanup itself, which must not mask the original error — report the original, mention the cleanup failure.
Ordering is reconsidered so that everything fallible and cheap happens before anything is written: resolve the fingerprint once, up front, and reuse the result rather than re-resolving at :324.
A test simulates a failure at the late step and asserts no unlocker directory remains.
The same pattern is checked in the other unlocker creation paths — passphrase, keychain, Secure Enclave — and fixed if present, or explicitly confirmed absent in the PR description.
make check green. TODO.md updated in the same commit.

Implementation requirements

Cleanup by RemoveAll on a directory this function created is safe; make sure it cannot ever run against a directory that already existed before the call, which would delete a working unlocker. Track whether this invocation created it.
This overlaps #34 (atomic writes). If #34 lands first and introduces a general atomic-write or transactional-directory helper, use it rather than hand-rolling cleanup here. If this lands first, keep the fix local and note it for the #34 implementer to fold in.
Resolving the fingerprint once instead of twice is a behavior change in the failure timing. Verify no caller depends on the current late-resolution ordering.

From the 1.0 security survey. Small, but it produces exactly the corrupt-unlocker state that #42 is about handling gracefully — better to not create it. ## Problem `CreatePGPUnlocker` (`internal/secret/pgpunlocker.go:260`) writes its on-disk state across several steps, and the last validation happens after most of the writing is done: - `:249-304` create the unlocker directory, write `pub.txt`, and write `longterm.age`. - `:324` then re-resolves the GPG fingerprint, feeding `writePGPUnlockerMetadata` at `:341`. If that resolution fails — the key was removed from the keyring between the pre-check and here, the agent died, a smartcard was unplugged — the function errors out having **already written an unlocker directory with no metadata file**. That is precisely the shape that broke `secret unlocker list` in issue #1, and the same shape that currently makes `PGPUnlocker.GetID()` panic (#42). The pre-flight validation is good and already exists: `internal/cli/unlockers.go:671-674` calls `ResolveGPGKeyFingerprint` before `CreatePGPUnlocker`, and `preparePGPUnlockerDir` calls `checkGPGAvailable()`. So the window is narrow. It is not zero, and the consequence is a vault the user cannot list. ## Definition of done - `CreatePGPUnlocker` either completes fully or leaves no trace. Any failure after the unlocker directory is created removes it before returning. - Cleanup runs on every error path, including a failure of the cleanup itself, which must not mask the original error — report the original, mention the cleanup failure. - Ordering is reconsidered so that everything fallible and cheap happens before anything is written: resolve the fingerprint once, up front, and reuse the result rather than re-resolving at `:324`. - A test simulates a failure at the late step and asserts no unlocker directory remains. - The same pattern is checked in the other unlocker creation paths — passphrase, keychain, Secure Enclave — and fixed if present, or explicitly confirmed absent in the PR description. - `make check` green. `TODO.md` updated in the same commit. ## Implementation requirements - Cleanup by `RemoveAll` on a directory this function created is safe; make sure it cannot ever run against a directory that already existed before the call, which would delete a working unlocker. Track whether this invocation created it. - This overlaps #34 (atomic writes). If #34 lands first and introduces a general atomic-write or transactional-directory helper, use it rather than hand-rolling cleanup here. If this lands first, keep the fix local and note it for the #34 implementer to fold in. - Resolving the fingerprint once instead of twice is a behavior change in the failure timing. Verify no caller depends on the current late-resolution ordering.

clawbot added this to the 1.0.0 milestone 2026-08-09 03:44:00 +02:00

clawbot referenced this issue

2026-08-09 03:46:49 +02:00

Darwin-only code is never compiled, linted, or tested by CI, including both hardware-backed unlockers #50

clawbot referenced this issue

2026-08-09 04:25:11 +02:00

checkUnlockerExists fails open: an unenumerable unlockers.d reports "no duplicate" and permits a duplicate unlocker #51

Sign in to join this conversation.