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

New Issue

clawbot · 2026-08-09T03:38:51+02:00

clawbot commented

2026-08-09 03:38:51 +02:00

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 referenced this issue

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

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

clawbot referenced this issue

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

README missing four required sections; TODO.md is stale and describes completed work as pending #46

clawbot referenced this issue

2026-08-09 06:57:13 +02:00

script/test silently passes on flaky failures, and runs without -race or -timeout #32

clawbot referenced this issue

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