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

New Issue

clawbot · 2026-08-09T04:25:11+02:00

clawbot commented

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

Surfaced during the re-review of PR #29 (#29 (comment)). Pre-existing on main, not a regression from that PR — the reviewer confirmed main fails open in the same three places, which is why it did not block the merge.

Problem

internal/cli/unlockers.go:786, :795, :799-807. checkUnlockerExists is the duplicate-detection gate run before creating a new unlocker. At three separate points it treats a failure to enumerate unlockers.d as "no duplicate found" and returns success, rather than as an error.

So whenever the directory cannot be read — a transient I/O error, a permissions problem, a partially-created unlocker directory of the kind #48 describes, a filesystem hiccup — the gate silently reports the coast is clear and creation proceeds. The user ends up with two unlockers where the tool's own invariant says there should be one.

The failure direction is what makes this worth fixing rather than tolerating. A check that fails closed refuses to create an unlocker and shows an error the user can act on. This one fails open: it creates state that the duplicate check exists specifically to prevent, and does so silently. In a tool where unlockers are the access-control layer for a vault's long-term key, "we could not verify, so we allowed it" is the wrong default.

Related but distinct from #48, which is about CreatePGPUnlocker leaving a partial directory behind on failure. The two compound: #48 creates the unreadable directory, and this issue then fails to notice it when deciding whether a duplicate exists.

Definition of done

Failure to enumerate unlockers.d becomes an error that aborts unlocker creation, rather than being reported as "no duplicate".
All three sites in checkUnlockerExists are fixed, not just the first one found.
The error names the directory and the underlying cause, so the user can tell an I/O problem from a permissions problem.
The distinction is preserved between "enumerated successfully, found no duplicate" (proceed) and "could not enumerate" (abort). Those must not collapse back into a single boolean — that collapse is the bug.
A test proves that an unreadable unlockers.d causes unlocker creation to fail rather than succeed. Assert on the resulting state, not just the return value: no new unlocker directory should exist afterward.
Audit the neighbouring code for the same fail-open shape. Any other place that treats an enumeration or stat failure as a negative result belongs in this fix, or in a comment on this issue if it turns out to be larger than expected.
make check green. TODO.md updated in the same commit.

Implementation requirements

Sequence after PR #29, which rewrote this file, and coordinate with #48 — if #48 lands first and stops partial unlocker directories being created, the trigger for this becomes rarer but the fail-open remains and still needs fixing.
Do not weaken the change to preserve convenience. If aborting on an unreadable directory makes some existing test fail because that test relies on a half-built fixture, fix the fixture, not the check.
Note that #29 changed findUnlockerIDByMetadata to skip unreadable entries with a warning, which is correct for listing. This issue is the opposite case: for a safety gate, skipping is not acceptable, because skipping is what produces the fail-open. Make sure the fix here does not get "harmonized" with the listing behavior — the two call sites genuinely want different things, and that difference should be commented so a future reader does not unify them.

Surfaced during the re-review of PR #29 (https://git.eeqj.de/sneak/secret/pulls/29#issuecomment-46388). **Pre-existing on `main`, not a regression from that PR** — the reviewer confirmed `main` fails open in the same three places, which is why it did not block the merge. ## Problem `internal/cli/unlockers.go:786`, `:795`, `:799-807`. `checkUnlockerExists` is the duplicate-detection gate run before creating a new unlocker. At three separate points it treats a failure to enumerate `unlockers.d` as **"no duplicate found"** and returns success, rather than as an error. So whenever the directory cannot be read — a transient I/O error, a permissions problem, a partially-created unlocker directory of the kind #48 describes, a filesystem hiccup — the gate silently reports the coast is clear and creation proceeds. The user ends up with two unlockers where the tool's own invariant says there should be one. The failure direction is what makes this worth fixing rather than tolerating. A check that fails *closed* refuses to create an unlocker and shows an error the user can act on. This one fails *open*: it creates state that the duplicate check exists specifically to prevent, and does so silently. In a tool where unlockers are the access-control layer for a vault's long-term key, "we could not verify, so we allowed it" is the wrong default. Related but distinct from #48, which is about `CreatePGPUnlocker` leaving a partial directory behind on failure. The two compound: #48 creates the unreadable directory, and this issue then fails to notice it when deciding whether a duplicate exists. ## Definition of done - Failure to enumerate `unlockers.d` becomes an error that **aborts unlocker creation**, rather than being reported as "no duplicate". - All three sites in `checkUnlockerExists` are fixed, not just the first one found. - The error names the directory and the underlying cause, so the user can tell an I/O problem from a permissions problem. - The distinction is preserved between "enumerated successfully, found no duplicate" (proceed) and "could not enumerate" (abort). Those must not collapse back into a single boolean — that collapse is the bug. - A test proves that an unreadable `unlockers.d` causes unlocker creation to fail rather than succeed. Assert on the resulting state, not just the return value: no new unlocker directory should exist afterward. - Audit the neighbouring code for the same fail-open shape. Any other place that treats an enumeration or stat failure as a negative result belongs in this fix, or in a comment on this issue if it turns out to be larger than expected. - `make check` green. `TODO.md` updated in the same commit. ## Implementation requirements - Sequence **after** PR #29, which rewrote this file, and coordinate with #48 — if #48 lands first and stops partial unlocker directories being created, the trigger for this becomes rarer but the fail-open remains and still needs fixing. - Do not weaken the change to preserve convenience. If aborting on an unreadable directory makes some existing test fail because that test relies on a half-built fixture, fix the fixture, not the check. - Note that #29 changed `findUnlockerIDByMetadata` to skip unreadable entries with a warning, which is correct for *listing*. This issue is the opposite case: for a **safety gate**, skipping is not acceptable, because skipping is what produces the fail-open. Make sure the fix here does not get "harmonized" with the listing behavior — the two call sites genuinely want different things, and that difference should be commented so a future reader does not unify them.

clawbot added this to the 1.0.0 milestone 2026-08-09 04:25:11 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/secret#51