sneak/secret
1
0
Fork 0
Code Issues 2 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

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

New Issue
Closed
opened 2026-08-09 04:25:11 +02:00 by clawbot · 1 comment
clawbot commented 2026-08-09 04:25:11 +02:00
Collaborator
Copy Link
Copy Source

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
clawbot commented 2026-10-03 14:25:42 +02:00
Author
Collaborator
Copy Link
Copy Source

Built in #64. The duplicate check before adding a PGP unlocker now stops with an error when unlockers.d cannot be read. The same now holds for the "does the vault hold secrets" check before removing the last unlocker or a vault, and for the long-term key check before vault import.

Audit findings left out of that PR:

  • The duplicate check still gets each unlocker's ID through the listing helper (findUnlockerIDByMetadata), which skips an entry whose metadata it cannot read. The check has read every metadata file strictly just before, so this only matters if a file becomes unreadable between the two reads. Closing the gap means giving the check its own strict scan instead of reusing the listing helper, which this issue asked to leave alone.
  • secret move work:api-key default: when the vault list cannot be read, default is taken as a secret name, not a vault name, so the move becomes a rename inside work (MoveSecret in internal/cli/secrets.go).
  • Adding a new version to an existing secret: when the current version cannot be read, the old version never gets its notAfter time (the GetCurrentVersion call in internal/vault/secrets.go).
  • Some display code also reads a failure as "none": version counts, the current-vault marker, info totals. It decides nothing, so it was left as is.

Model: opus-5-5

Built in https://git.eeqj.de/sneak/secret/pulls/64. The duplicate check before adding a PGP unlocker now stops with an error when `unlockers.d` cannot be read. The same now holds for the "does the vault hold secrets" check before removing the last unlocker or a vault, and for the long-term key check before `vault import`. Audit findings left out of that PR: - The duplicate check still gets each unlocker's ID through the listing helper (`findUnlockerIDByMetadata`), which skips an entry whose metadata it cannot read. The check has read every metadata file strictly just before, so this only matters if a file becomes unreadable between the two reads. Closing the gap means giving the check its own strict scan instead of reusing the listing helper, which this issue asked to leave alone. - `secret move work:api-key default`: when the vault list cannot be read, `default` is taken as a secret name, not a vault name, so the move becomes a rename inside `work` (`MoveSecret` in `internal/cli/secrets.go`). - Adding a new version to an existing secret: when the current version cannot be read, the old version never gets its `notAfter` time (the `GetCurrentVersion` call in `internal/vault/secrets.go`). - Some display code also reads a failure as "none": version counts, the current-vault marker, `info` totals. It decides nothing, so it was left as is. Model: opus-5-5
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
critical
Extremely important: data loss, a security hole, production down, or basic use impossible
 merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No labels
Milestone
No items
No Milestone 1.0.0
Projects
Clear projects
No projects
Assignees
Clear assignees
clawbot sneak (Jeffrey Paul)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/secret#51