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

No file locking and no atomic writes: concurrent or interrupted operations corrupt the vault #34

New Issue
Closed
opened 2026-08-09 03:38:51 +02:00 by clawbot · 3 comments
clawbot commented 2026-08-09 03:38:51 +02:00
Collaborator
Copy Link
Copy Source

From the 1.0 security survey. Confirmed still present and entirely unmitigated. This is the largest work unit on the milestone.

Threat

There is no file locking of any kind in the repo. Searching all non-test Go files for flock, syscall.Flock, O_EXCL, sync.Mutex, and sync.RWMutex returns zero hits. There are also no atomic write paths — every write is a direct afero.WriteFile to its final destination. The combination means two concurrent secret invocations, or one invocation interrupted at the wrong moment, can leave the vault in a state that is not merely stale but permanently undecryptable.

Five concrete unsynchronized paths in AddSecret (internal/vault/secrets.go:114-190):

1. Version serial allocation is a read-modify-write race. secret.GenerateVersionName (internal/secret/version.go:79-132) lists the versions directory, finds the maximum serial, and adds one. Two concurrent secret add calls on the same secret both compute e.g. 20260809.003; the second silently overwrites the first's value.age, priv.age, and metadata.age. The first secret is gone with no error reported to either caller.

2. Version.Save writes three files non-atomically (internal/secret/version.go:135-195, writing at :371, :391, :441, :482). A crash or a competing writer between those writes leaves a version directory where, for example, value.age exists but priv.age does not. That version can never be decrypted again — the per-version private key is the only thing that recovers value.age, and it is gone. This is unrecoverable data loss from a plain Ctrl-C.

3. The current-version pointer is written remove-then-write. secret.SetCurrentVersion (internal/secret/version.go:544-557):

// Remove existing file if it exists
_ = fs.Remove(currentPath)

// Write just the version name to the file
err := afero.WriteFile(fs, currentPath, []byte(version), FilePerms)

Note this is a plain file, not the symlink the README describes. Between the Remove and the WriteFile the secret has no current version at all: a concurrent secret get fails with "failed to read current version file", and a crash inside that window orphans every version of the secret permanently — the data is intact on disk but nothing points at it. SelectVault (internal/vault/management.go:296-306) uses the identical remove-then-write pattern on the global currentvault pointer, so the same crash can leave the tool with no selected vault.

4. Previous-version metadata is decrypt-modify-reencrypt-write (updateVersionMetadata, internal/vault/secrets.go:193-242, writing at :236). Concurrent adds interleave here and lose a notAfter stamp or write a torn file.

5. Directory preparation is TOCTOU. prepareSecretDir (:536-568) does DirExists and then MkdirAll, so the existence check that backs --force can be invalidated between the two.

Rollback is also non-atomic: createAndSaveVersion:734 and copyVersionsWithRollback:760,769 call RemoveAll on failure, which can itself partially fail and leave debris.

None of this needs an attacker. A cron job overlapping an interactive session, a CI fan-out, a make target running two secret calls in parallel, or a laptop suspending mid-write all reach it. The user-visible symptom of the worst case is secret get failing forever on a secret that was written successfully.

Definition of done

  • A per-vault advisory lock is taken for the whole duration of every mutating operation: AddSecret, RemoveSecret, MoveSecret, CopySecretAllVersions, SetCurrentVersion, SelectVault, and unlocker create/remove.
  • The lock is released on every exit path including error returns and panics.
  • All multi-file state transitions are atomic or, where genuinely impossible, ordered so that a crash at any point leaves a state that is still readable. In particular Version.Save must not be able to leave value.age without its priv.age.
  • Single-file writes go through one shared writeFileAtomic helper: create a temp file in the destination directory with mode 0600, write, fsync, then Rename over the target. The temp file must be cleaned up on every error path, and must never be created with default permissions and chmod'd afterward — that is a window where key material is world-readable.
  • SetCurrentVersion and SelectVault no longer remove-then-write. Rename over the existing file is atomic on POSIX; use it.
  • Tests demonstrate the failure modes are actually closed, not merely that the happy path still works. At minimum: concurrent AddSecret on the same secret does not lose a version and does not produce duplicate serials; a simulated failure between the writes in Version.Save leaves no half-written version directory; the current pointer is never observably absent.
  • make check green with -race enabled (see the script/test issue — land that first so the race detector is actually running). TODO.md updated in the same commit.

Implementation requirements

  • The afero.Fs abstraction is the hard part and must be handled deliberately. afero has no locking primitive, and afero.MemMapFs backs the test suite. The lock needs a real-filesystem implementation with a no-op or in-process-mutex equivalent for the memory filesystem. Do not silently degrade to no locking when the filesystem is not a real one — make the substitution explicit and documented, or the tests will pass while production is unprotected.
  • Similarly, Rename-based atomic writes must be verified to behave on the afero backend used in tests, not just on the real filesystem.
  • Consider whether the lock belongs at the vault directory level (<vaultDir>/.lock) or the state-directory level. Vault-level allows concurrent work in different vaults; state-level is simpler and safer. Pick one, write down the reasoning in the commit message, and be consistent.
  • Locking must not introduce a deadlock between nested calls — several of the listed operations call each other. Either make the lock reentrant or restructure so it is taken exactly once at the outermost entry point.
  • A stale lock file from a killed process must not wedge the tool forever. flock on an open descriptor releases automatically on process death; a hand-rolled lockfile-with-PID does not. Prefer the former.
  • Temp files introduced by this change are new key-material-bearing artifacts. They must be 0600 from creation, inside the destination directory (not /tmp, which may be a different filesystem and would make Rename non-atomic), and removed on failure.

Notes

Sequence this after the script/test fix, so -race is on while this is being developed, and after the lint branch lands, since it rewrites internal/vault/secrets.go and internal/secret/version.go heavily.

Also worth noting: the survey found the repo currently creates no temp files anywhere, and file permissions are correct throughout (DirPerms = 0o700, FilePerms = 0o600 in internal/secret/constants.go). The reason there are no temp files is precisely that writes are not atomic. This change introduces them, so the permission discipline above is a new requirement, not an existing one being preserved.

From the 1.0 security survey. Confirmed still present and entirely unmitigated. This is the largest work unit on the milestone. ## Threat There is **no file locking of any kind in the repo**. Searching all non-test Go files for `flock`, `syscall.Flock`, `O_EXCL`, `sync.Mutex`, and `sync.RWMutex` returns zero hits. There are also **no atomic write paths** — every write is a direct `afero.WriteFile` to its final destination. The combination means two concurrent `secret` invocations, or one invocation interrupted at the wrong moment, can leave the vault in a state that is not merely stale but permanently undecryptable. Five concrete unsynchronized paths in `AddSecret` (`internal/vault/secrets.go:114-190`): **1. Version serial allocation is a read-modify-write race.** `secret.GenerateVersionName` (`internal/secret/version.go:79-132`) lists the versions directory, finds the maximum serial, and adds one. Two concurrent `secret add` calls on the same secret both compute e.g. `20260809.003`; the second silently overwrites the first's `value.age`, `priv.age`, and `metadata.age`. The first secret is gone with no error reported to either caller. **2. `Version.Save` writes three files non-atomically** (`internal/secret/version.go:135-195`, writing at `:371`, `:391`, `:441`, `:482`). A crash or a competing writer between those writes leaves a version directory where, for example, `value.age` exists but `priv.age` does not. **That version can never be decrypted again** — the per-version private key is the only thing that recovers `value.age`, and it is gone. This is unrecoverable data loss from a plain Ctrl-C. **3. The current-version pointer is written remove-then-write.** `secret.SetCurrentVersion` (`internal/secret/version.go:544-557`): ```go // Remove existing file if it exists _ = fs.Remove(currentPath) // Write just the version name to the file err := afero.WriteFile(fs, currentPath, []byte(version), FilePerms) ``` Note this is a plain file, not the symlink the README describes. Between the `Remove` and the `WriteFile` the secret has **no current version at all**: a concurrent `secret get` fails with "failed to read current version file", and a crash inside that window orphans every version of the secret permanently — the data is intact on disk but nothing points at it. `SelectVault` (`internal/vault/management.go:296-306`) uses the identical remove-then-write pattern on the global `currentvault` pointer, so the same crash can leave the tool with no selected vault. **4. Previous-version metadata is decrypt-modify-reencrypt-write** (`updateVersionMetadata`, `internal/vault/secrets.go:193-242`, writing at `:236`). Concurrent adds interleave here and lose a `notAfter` stamp or write a torn file. **5. Directory preparation is TOCTOU.** `prepareSecretDir` (`:536-568`) does `DirExists` and then `MkdirAll`, so the existence check that backs `--force` can be invalidated between the two. Rollback is also non-atomic: `createAndSaveVersion:734` and `copyVersionsWithRollback:760,769` call `RemoveAll` on failure, which can itself partially fail and leave debris. None of this needs an attacker. A cron job overlapping an interactive session, a CI fan-out, a `make` target running two `secret` calls in parallel, or a laptop suspending mid-write all reach it. The user-visible symptom of the worst case is `secret get` failing forever on a secret that was written successfully. ## Definition of done - A per-vault advisory lock is taken for the whole duration of every mutating operation: `AddSecret`, `RemoveSecret`, `MoveSecret`, `CopySecretAllVersions`, `SetCurrentVersion`, `SelectVault`, and unlocker create/remove. - The lock is released on every exit path including error returns and panics. - All multi-file state transitions are atomic or, where genuinely impossible, ordered so that a crash at any point leaves a state that is still readable. In particular `Version.Save` must not be able to leave `value.age` without its `priv.age`. - Single-file writes go through one shared `writeFileAtomic` helper: create a temp file in the destination directory with mode `0600`, write, `fsync`, then `Rename` over the target. The temp file must be cleaned up on every error path, and must never be created with default permissions and chmod'd afterward — that is a window where key material is world-readable. - `SetCurrentVersion` and `SelectVault` no longer remove-then-write. `Rename` over the existing file is atomic on POSIX; use it. - Tests demonstrate the failure modes are actually closed, not merely that the happy path still works. At minimum: concurrent `AddSecret` on the same secret does not lose a version and does not produce duplicate serials; a simulated failure between the writes in `Version.Save` leaves no half-written version directory; the current pointer is never observably absent. - `make check` green with `-race` enabled (see the `script/test` issue — land that first so the race detector is actually running). `TODO.md` updated in the same commit. ## Implementation requirements - **The `afero.Fs` abstraction is the hard part and must be handled deliberately.** `afero` has no locking primitive, and `afero.MemMapFs` backs the test suite. The lock needs a real-filesystem implementation with a no-op or in-process-mutex equivalent for the memory filesystem. Do not silently degrade to no locking when the filesystem is not a real one — make the substitution explicit and documented, or the tests will pass while production is unprotected. - Similarly, `Rename`-based atomic writes must be verified to behave on the afero backend used in tests, not just on the real filesystem. - Consider whether the lock belongs at the vault directory level (`<vaultDir>/.lock`) or the state-directory level. Vault-level allows concurrent work in different vaults; state-level is simpler and safer. Pick one, write down the reasoning in the commit message, and be consistent. - Locking must not introduce a deadlock between nested calls — several of the listed operations call each other. Either make the lock reentrant or restructure so it is taken exactly once at the outermost entry point. - A stale lock file from a killed process must not wedge the tool forever. `flock` on an open descriptor releases automatically on process death; a hand-rolled lockfile-with-PID does not. Prefer the former. - Temp files introduced by this change are new key-material-bearing artifacts. They must be `0600` from creation, inside the destination directory (not `/tmp`, which may be a different filesystem and would make `Rename` non-atomic), and removed on failure. ## Notes Sequence this **after** the `script/test` fix, so `-race` is on while this is being developed, and after the lint branch lands, since it rewrites `internal/vault/secrets.go` and `internal/secret/version.go` heavily. Also worth noting: the survey found the repo currently creates no temp files anywhere, and file permissions are correct throughout (`DirPerms = 0o700`, `FilePerms = 0o600` in `internal/secret/constants.go`). The reason there are no temp files is precisely that writes are not atomic. This change introduces them, so the permission discipline above is a new requirement, not an existing one being preserved.
clawbot added this to the 1.0.0 milestone 2026-08-09 03:38:51 +02:00
clawbot added the critical label 2026-10-03 14:20:41 +02:00
clawbot commented 2026-10-03 14:20:41 +02:00
Author
Collaborator
Copy Link
Copy Source

Labelled critical: two secret commands writing at once, or one interrupted by Ctrl-C mid-write, can silently overwrite a stored secret or leave it permanently undecryptable for the vault owner.

Model: opus-5-5

Labelled critical: two `secret` commands writing at once, or one interrupted by Ctrl-C mid-write, can silently overwrite a stored secret or leave it permanently undecryptable for the vault owner. Model: opus-5-5
clawbot commented 2026-10-03 14:41:04 +02:00
Author
Collaborator
Copy Link
Copy Source

Plan:

  • Lock: one for the state directory (a file lock in it), not one per vault: currentvault, the derivation-index scan in vault create and cross-vault moves span vaults, and a lock file inside a vault would be deleted by vault remove while another command waits on it. Each changing command takes it once, at the top of its method in internal/cli, and releases it with defer; the vault and secret functions underneath take none, so nested calls cannot deadlock. Real filesystem: flock on an open descriptor, released by the kernel if the process dies. In-memory filesystem: one process-wide mutex instead; any other filesystem is refused. Changing commands wait for each other, also across a passphrase prompt; reads never wait.
  • Files: one secret.WriteFileAtomic (temporary file created 0600 in the destination directory, written, synced, renamed over the target, removed on any failure) for every file written into the state directory, so current, currentvault and current-unlocker are never removed and rewritten.
  • Directories: a new version, a new secret and a cross-vault move's destination are built in a temporary directory and renamed into place, so each is complete or absent; the two rollback blocks go away. Removals rename into a temporary directory first, then delete it. Temporary directories sit one level above the directory that gets listed, so crash leftovers are never read. The passphrase unlocker writes unlocker-metadata.json last, like the other types.
  • Tests: concurrent secret add of one secret on both filesystems (all succeed, one version per call, all values distinct); a test filesystem that checks before every change that the version directory is absent or complete during Version.Save and that current exists during SetCurrentVersion; a failure part-way leaves no version and no temporary directory; the lock is free after an error and after a panic.
  • Not here: re-adding a passphrase unlocker rewrites its files in place, so a crash can break it (the mnemonic still works); that needs a layout change and will be filed separately. -race stays off (#52).

Model: opus-5-5

Plan: - **Lock**: one for the state directory (a file `lock` in it), not one per vault: `currentvault`, the derivation-index scan in `vault create` and cross-vault moves span vaults, and a lock file inside a vault would be deleted by `vault remove` while another command waits on it. Each changing command takes it once, at the top of its method in `internal/cli`, and releases it with `defer`; the vault and secret functions underneath take none, so nested calls cannot deadlock. Real filesystem: `flock` on an open descriptor, released by the kernel if the process dies. In-memory filesystem: one process-wide mutex instead; any other filesystem is refused. Changing commands wait for each other, also across a passphrase prompt; reads never wait. - **Files**: one `secret.WriteFileAtomic` (temporary file created `0600` in the destination directory, written, synced, renamed over the target, removed on any failure) for every file written into the state directory, so `current`, `currentvault` and `current-unlocker` are never removed and rewritten. - **Directories**: a new version, a new secret and a cross-vault move's destination are built in a temporary directory and renamed into place, so each is complete or absent; the two rollback blocks go away. Removals rename into a temporary directory first, then delete it. Temporary directories sit one level above the directory that gets listed, so crash leftovers are never read. The passphrase unlocker writes `unlocker-metadata.json` last, like the other types. - **Tests**: concurrent `secret add` of one secret on both filesystems (all succeed, one version per call, all values distinct); a test filesystem that checks before every change that the version directory is absent or complete during `Version.Save` and that `current` exists during `SetCurrentVersion`; a failure part-way leaves no version and no temporary directory; the lock is free after an error and after a panic. - **Not here**: re-adding a passphrase unlocker rewrites its files in place, so a crash can break it (the mnemonic still works); that needs a layout change and will be filed separately. `-race` stays off (https://git.eeqj.de/sneak/secret/issues/52). Model: opus-5-5
clawbot commented 2026-10-03 15:03:22 +02:00
Author
Collaborator
Copy Link
Copy Source

Built in #69. Two departures from the plan: secret add and secret import take the lock only after reading the value, and secret encrypt only while it finds or stores its key, so a pipe between two secret commands cannot deadlock; and the passphrase unlocker now gets the long-term key before it writes anything. Replacing an existing passphrase unlocker is filed as #71.

Model: opus-5-5

Built in https://git.eeqj.de/sneak/secret/pulls/69. Two departures from the plan: `secret add` and `secret import` take the lock only after reading the value, and `secret encrypt` only while it finds or stores its key, so a pipe between two `secret` commands cannot deadlock; and the passphrase unlocker now gets the long-term key before it writes anything. Replacing an existing passphrase unlocker is filed as https://git.eeqj.de/sneak/secret/issues/71. 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 critical
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#34