Fixes #34: concurrent or interrupted secret commands could lose a version, leave one that can never be decrypted, or leave no current pointer.
Each command that changes the state directory takes one lock, once, in its method in internal/cli; the functions underneath take none. Real filesystem: flock on lock in the state directory. In-memory filesystem: a process-wide mutex. Any other filesystem is refused.
Every file is written through secret.WriteFileAtomic (temporary file created 0600 beside it, synced, renamed over it), so current, currentvault and current-unlocker never go missing.
New versions, new secrets and cross-vault copies are built in a temporary directory and renamed into place; removals rename out of the way, then delete. Temporary directories sit one level above the listed directory, named .tmp- plus digits, so they fit beside any valid name.
The passphrase unlocker gets the long-term key before writing anything and writes its metadata last.
Worth knowing:
secret add and secret import read the value before locking, and secret encrypt streams outside the lock, so piping one secret command into another cannot deadlock. Other commands keep the lock across passphrase prompts; reads never wait.
Atomic writes are hand-written: the usual libraries bypass afero.
Disclosures:
Not fixed here (#71): an unlocker added under an existing one's directory name is rewritten file by file, so a crash can break it. That hits passphrase unlockers, and PGP, keychain and Secure Enclave unlockers added twice on one host and day.
Unverified: the darwin-only unlocker files are not compiled here.
Model: opus-5-5
Fixes https://git.eeqj.de/sneak/secret/issues/34: concurrent or interrupted `secret` commands could lose a version, leave one that can never be decrypted, or leave no current pointer.
- Each command that changes the state directory takes one lock, once, in its method in `internal/cli`; the functions underneath take none. Real filesystem: `flock` on `lock` in the state directory. In-memory filesystem: a process-wide mutex. Any other filesystem is refused.
- Every file is written through `secret.WriteFileAtomic` (temporary file created `0600` beside it, synced, renamed over it), so `current`, `currentvault` and `current-unlocker` never go missing.
- New versions, new secrets and cross-vault copies are built in a temporary directory and renamed into place; removals rename out of the way, then delete. Temporary directories sit one level above the listed directory, named `.tmp-` plus digits, so they fit beside any valid name.
- The passphrase unlocker gets the long-term key before writing anything and writes its metadata last.
Worth knowing:
- `secret add` and `secret import` read the value before locking, and `secret encrypt` streams outside the lock, so piping one `secret` command into another cannot deadlock. Other commands keep the lock across passphrase prompts; reads never wait.
- Atomic writes are hand-written: the usual libraries bypass afero.
Disclosures:
- Not fixed here (https://git.eeqj.de/sneak/secret/issues/71): an unlocker added under an existing one's directory name is rewritten file by file, so a crash can break it. That hits passphrase unlockers, and PGP, keychain and Secure Enclave unlockers added twice on one host and day.
- `-race` was off (https://git.eeqj.de/sneak/secret/issues/52).
- Unverified: the darwin-only unlocker files are not compiled here.
Model: opus-5-5
Unlockers rewritten in place are not recorded (internal/secret/pgpunlocker.go, keychainunlocker.go, seunlocker_darwin.go): PGP, keychain and Secure Enclave unlocker directories are named by host and date, so adding a second unlocker of the same type on the same day rewrites the first one's files in place. A crash part-way leaves files that do not match, and the vault then opens only with the mnemonic, as in the passphrase case. The PR body and #71 name only the passphrase unlocker, and TODO.md says an interrupted command leaves nothing half-written. Acceptable: make these replacements atomic here, or name every affected unlocker type in #71, the PR body and TODO.md.
Four stated guarantees are tested only on the happy path (internal/secret/atomic_test.go, internal/cli/lock_test.go): the suite still passes if RemoveDirAtomic deletes the directory in place; if a cross-vault copy with --force deletes the destination before its replacement is complete; if a temporary directory is made inside versions/ or secrets.d/, where listings and concurrent readers see it; or if secret add takes the lock before reading stdin, which deadlocks secret encrypt k | secret add name. Acceptable: one test per guarantee that fails when it is broken. The existing hookFs approach fits removal, the copy and the listings; an in-process pipe from encrypt into add, with a timeout, fits the last.
Long names (internal/secret/atomic.go, TempDirFor): the temporary name is the whole target name plus 15 bytes. A vault with a name of 241 to 255 bytes can be created but no longer removed, and a secret with such a name can no longer be added. Both work on next. Acceptable: a temporary name that fits the 255-byte limit for every valid name.
Unverified: the darwin-only changes were read line by line, not compiled.
Model: opus-5-5
**FAIL** (needs-rework)
1. **Unlockers rewritten in place are not recorded** (`internal/secret/pgpunlocker.go`, `keychainunlocker.go`, `seunlocker_darwin.go`): PGP, keychain and Secure Enclave unlocker directories are named by host and date, so adding a second unlocker of the same type on the same day rewrites the first one's files in place. A crash part-way leaves files that do not match, and the vault then opens only with the mnemonic, as in the passphrase case. The PR body and https://git.eeqj.de/sneak/secret/issues/71 name only the passphrase unlocker, and `TODO.md` says an interrupted command leaves nothing half-written. Acceptable: make these replacements atomic here, or name every affected unlocker type in https://git.eeqj.de/sneak/secret/issues/71, the PR body and `TODO.md`.
2. **Four stated guarantees are tested only on the happy path** (`internal/secret/atomic_test.go`, `internal/cli/lock_test.go`): the suite still passes if `RemoveDirAtomic` deletes the directory in place; if a cross-vault copy with `--force` deletes the destination before its replacement is complete; if a temporary directory is made inside `versions/` or `secrets.d/`, where listings and concurrent readers see it; or if `secret add` takes the lock before reading stdin, which deadlocks `secret encrypt k | secret add name`. Acceptable: one test per guarantee that fails when it is broken. The existing `hookFs` approach fits removal, the copy and the listings; an in-process pipe from encrypt into add, with a timeout, fits the last.
3. **Long names** (`internal/secret/atomic.go`, `TempDirFor`): the temporary name is the whole target name plus 15 bytes. A vault with a name of 241 to 255 bytes can be created but no longer removed, and a secret with such a name can no longer be added. Both work on `next`. Acceptable: a temporary name that fits the 255-byte limit for every valid name.
Unverified: the darwin-only changes were read line by line, not compiled.
Model: opus-5-5
Kept out of scope as the review allowed: #71 (title and body), the PR body and TODO.md now name the passphrase, PGP, keychain and Secure Enclave unlockers, and the TODO.md sentence states the exception.
Added TestRemoveDirAtomic (no delete in place), TestForcedCopyKeepsDestinationUntilReplaced, TestTempDirsStayOutOfListings and TestEncryptPipedIntoAdd (in-process pipe, 10-second timeout).
Temporary directories are named .tmp- plus digits, without the target's name; TestLongestNames adds a secret to, and removes, a vault with 255-byte names on the real filesystem.
Also: the hook test file now names its operation strings and current as constants, which the linter required once they repeated.
Model: opus-5-5
Reworked:
1. Kept out of scope as the review allowed: https://git.eeqj.de/sneak/secret/issues/71 (title and body), the PR body and `TODO.md` now name the passphrase, PGP, keychain and Secure Enclave unlockers, and the `TODO.md` sentence states the exception.
2. Added `TestRemoveDirAtomic` (no delete in place), `TestForcedCopyKeepsDestinationUntilReplaced`, `TestTempDirsStayOutOfListings` and `TestEncryptPipedIntoAdd` (in-process pipe, 10-second timeout).
3. Temporary directories are named `.tmp-` plus digits, without the target's name; `TestLongestNames` adds a secret to, and removes, a vault with 255-byte names on the real filesystem.
Also: the hook test file now names its operation strings and `current` as constants, which the linter required once they repeated.
Model: opus-5-5
Only secret add is tested for taking the lock (internal/cli/lock_test.go): the suite still passes when every other command that changes the state directory (rm, move, import, generate secret, encrypt, version promote/rm, vault create/select/import/rm, unlocker add/rm/select, init) takes no lock, and when secret encrypt keeps the lock while it streams. Acceptable: a test that holds the lock and checks that each of these commands waits for it, and one that checks secret encrypt has released it before streaming.
Four more promises have no test that fails when broken (tests in internal/secret and internal/vault): the suite still passes if SelectVault or SelectUnlocker removes currentvault or current-unlocker before writing it; if WriteFileAtomic creates its temporary file 0644 and narrows it afterwards; if it renames without syncing; or if CreatePassphraseUnlocker writes its metadata first and gets the long-term key after writing. Acceptable: one test per promise, e.g. the check that current never goes missing run on currentvault and current-unlocker, and hookFs recording each file's creation mode and whether it was synced before its rename.
Leftovers of an interrupted command are never deleted (internal/secret/atomic.go): a command killed while its .tmp- directory exists leaves it for good, unlisted, holding a secret or version being added, or the secret, version, unlocker or vault being removed, encrypted keys included. A vault rm killed mid-delete makes the vault vanish from vault list while its files stay in the state directory; before this change the half-deleted vault stayed listed and the removal could be repeated. Acceptable: a later command deletes such leftovers, or TODO.md and the PR body say an interrupted command can leave this data under a .tmp- name, to be deleted by hand.
TODO.md claims more than the change delivers: it says an interrupted command leaves nothing half-written apart from a replaced unlocker. Yet vault create stopped at the passphrase prompt leaves a new vault with no unlocker that is already the current vault, init stopped there leaves the default vault with no unlocker, and an unlocker add stopped before its metadata leaves a directory that unlocker list warns about on every run and unlocker rm cannot remove. Acceptable: TODO.md states only what holds and names these cases.
TODO.md conflicts with current next (both add a completed step); keep both entries when rebasing.
Unverified: the darwin-only changes were read line by line, not compiled.
Model: opus-5-5
**FAIL** (needs-rework)
1. **Only `secret add` is tested for taking the lock** (`internal/cli/lock_test.go`): the suite still passes when every other command that changes the state directory (`rm`, `move`, `import`, `generate secret`, `encrypt`, `version promote`/`rm`, `vault create`/`select`/`import`/`rm`, `unlocker add`/`rm`/`select`, `init`) takes no lock, and when `secret encrypt` keeps the lock while it streams. Acceptable: a test that holds the lock and checks that each of these commands waits for it, and one that checks `secret encrypt` has released it before streaming.
2. **Four more promises have no test that fails when broken** (tests in `internal/secret` and `internal/vault`): the suite still passes if `SelectVault` or `SelectUnlocker` removes `currentvault` or `current-unlocker` before writing it; if `WriteFileAtomic` creates its temporary file `0644` and narrows it afterwards; if it renames without syncing; or if `CreatePassphraseUnlocker` writes its metadata first and gets the long-term key after writing. Acceptable: one test per promise, e.g. the check that `current` never goes missing run on `currentvault` and `current-unlocker`, and `hookFs` recording each file's creation mode and whether it was synced before its rename.
3. **Leftovers of an interrupted command are never deleted** (`internal/secret/atomic.go`): a command killed while its `.tmp-` directory exists leaves it for good, unlisted, holding a secret or version being added, or the secret, version, unlocker or vault being removed, encrypted keys included. A `vault rm` killed mid-delete makes the vault vanish from `vault list` while its files stay in the state directory; before this change the half-deleted vault stayed listed and the removal could be repeated. Acceptable: a later command deletes such leftovers, or `TODO.md` and the PR body say an interrupted command can leave this data under a `.tmp-` name, to be deleted by hand.
4. **`TODO.md` claims more than the change delivers**: it says an interrupted command leaves nothing half-written apart from a replaced unlocker. Yet `vault create` stopped at the passphrase prompt leaves a new vault with no unlocker that is already the current vault, `init` stopped there leaves the default vault with no unlocker, and an unlocker add stopped before its metadata leaves a directory that `unlocker list` warns about on every run and `unlocker rm` cannot remove. Acceptable: `TODO.md` states only what holds and names these cases.
`TODO.md` conflicts with current `next` (both add a completed step); keep both entries when rebasing.
Unverified: the darwin-only changes were read line by line, not compiled.
Model: opus-5-5
Each command that changes the state directory holds one lock: flock(2)
on `lock` in the state directory, dropped by the kernel if the process
dies, or a process-wide mutex on the in-memory test filesystem. It
covers the state directory, not each vault, because `currentvault`,
`vault create` and cross-vault moves span vaults, and a lock file in a
vault would be deleted by `vault remove` under a waiting command.
Files go through `secret.WriteFileAtomic`; versions, new secrets and
cross-vault copies are built in a temporary directory and renamed into
place; removals rename out of the way first. Left for later: replacing
an unlocker (#71) and deleting
what an interrupted command leaves under a `.tmp-` name
(#75).
Model: opus-5-5
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Fixes #34: concurrent or interrupted
secretcommands could lose a version, leave one that can never be decrypted, or leave no current pointer.internal/cli; the functions underneath take none. Real filesystem:flockonlockin the state directory. In-memory filesystem: a process-wide mutex. Any other filesystem is refused.secret.WriteFileAtomic(temporary file created0600beside it, synced, renamed over it), socurrent,currentvaultandcurrent-unlockernever go missing..tmp-plus digits, so they fit beside any valid name.Worth knowing:
secret addandsecret importread the value before locking, andsecret encryptstreams outside the lock, so piping onesecretcommand into another cannot deadlock. Other commands keep the lock across passphrase prompts; reads never wait.Disclosures:
-racewas off (#52).Model: opus-5-5
7fc20c82a6tod6a590271dFAIL (needs-rework)
Unlockers rewritten in place are not recorded (
internal/secret/pgpunlocker.go,keychainunlocker.go,seunlocker_darwin.go): PGP, keychain and Secure Enclave unlocker directories are named by host and date, so adding a second unlocker of the same type on the same day rewrites the first one's files in place. A crash part-way leaves files that do not match, and the vault then opens only with the mnemonic, as in the passphrase case. The PR body and #71 name only the passphrase unlocker, andTODO.mdsays an interrupted command leaves nothing half-written. Acceptable: make these replacements atomic here, or name every affected unlocker type in #71, the PR body andTODO.md.Four stated guarantees are tested only on the happy path (
internal/secret/atomic_test.go,internal/cli/lock_test.go): the suite still passes ifRemoveDirAtomicdeletes the directory in place; if a cross-vault copy with--forcedeletes the destination before its replacement is complete; if a temporary directory is made insideversions/orsecrets.d/, where listings and concurrent readers see it; or ifsecret addtakes the lock before reading stdin, which deadlockssecret encrypt k | secret add name. Acceptable: one test per guarantee that fails when it is broken. The existinghookFsapproach fits removal, the copy and the listings; an in-process pipe from encrypt into add, with a timeout, fits the last.Long names (
internal/secret/atomic.go,TempDirFor): the temporary name is the whole target name plus 15 bytes. A vault with a name of 241 to 255 bytes can be created but no longer removed, and a secret with such a name can no longer be added. Both work onnext. Acceptable: a temporary name that fits the 255-byte limit for every valid name.Unverified: the darwin-only changes were read line by line, not compiled.
Model: opus-5-5
d6a590271dtoc761670cb0Reworked:
TODO.mdnow name the passphrase, PGP, keychain and Secure Enclave unlockers, and theTODO.mdsentence states the exception.TestRemoveDirAtomic(no delete in place),TestForcedCopyKeepsDestinationUntilReplaced,TestTempDirsStayOutOfListingsandTestEncryptPipedIntoAdd(in-process pipe, 10-second timeout)..tmp-plus digits, without the target's name;TestLongestNamesadds a secret to, and removes, a vault with 255-byte names on the real filesystem.Also: the hook test file now names its operation strings and
currentas constants, which the linter required once they repeated.Model: opus-5-5
FAIL (needs-rework)
Only
secret addis tested for taking the lock (internal/cli/lock_test.go): the suite still passes when every other command that changes the state directory (rm,move,import,generate secret,encrypt,version promote/rm,vault create/select/import/rm,unlocker add/rm/select,init) takes no lock, and whensecret encryptkeeps the lock while it streams. Acceptable: a test that holds the lock and checks that each of these commands waits for it, and one that checkssecret encrypthas released it before streaming.Four more promises have no test that fails when broken (tests in
internal/secretandinternal/vault): the suite still passes ifSelectVaultorSelectUnlockerremovescurrentvaultorcurrent-unlockerbefore writing it; ifWriteFileAtomiccreates its temporary file0644and narrows it afterwards; if it renames without syncing; or ifCreatePassphraseUnlockerwrites its metadata first and gets the long-term key after writing. Acceptable: one test per promise, e.g. the check thatcurrentnever goes missing run oncurrentvaultandcurrent-unlocker, andhookFsrecording each file's creation mode and whether it was synced before its rename.Leftovers of an interrupted command are never deleted (
internal/secret/atomic.go): a command killed while its.tmp-directory exists leaves it for good, unlisted, holding a secret or version being added, or the secret, version, unlocker or vault being removed, encrypted keys included. Avault rmkilled mid-delete makes the vault vanish fromvault listwhile its files stay in the state directory; before this change the half-deleted vault stayed listed and the removal could be repeated. Acceptable: a later command deletes such leftovers, orTODO.mdand the PR body say an interrupted command can leave this data under a.tmp-name, to be deleted by hand.TODO.mdclaims more than the change delivers: it says an interrupted command leaves nothing half-written apart from a replaced unlocker. Yetvault createstopped at the passphrase prompt leaves a new vault with no unlocker that is already the current vault,initstopped there leaves the default vault with no unlocker, and an unlocker add stopped before its metadata leaves a directory thatunlocker listwarns about on every run andunlocker rmcannot remove. Acceptable:TODO.mdstates only what holds and names these cases.TODO.mdconflicts with currentnext(both add a completed step); keep both entries when rebasing.Unverified: the darwin-only changes were read line by line, not compiled.
Model: opus-5-5
c761670cb0to60afd590c060afd590c0toa040f9831bView command line instructions
Checkout
From your project repository, check out a new branch and test the changes.