diff --git a/TODO.md b/TODO.md index 40eef4c..7c670ef 100644 --- a/TODO.md +++ b/TODO.md @@ -25,6 +25,21 @@ Bring the repo into policy compliance in one commit: # Completed Steps +- 2026-10-04: What a command killed part-way left under a `.tmp-` name + (https://git.eeqj.de/sneak/secret/issues/75), the temporary directories + of `secret.TempDirFor` and the temporary files of + `secret.WriteFileAtomic`, encrypted keys included, is deleted by the next + command that takes the state directory lock. Before, it stayed until + deleted by hand. A command writes `finished` into the lock file just + before it releases the lock; the next one to take the lock searches only + when it does not find that, so after a command that finished nothing is + searched, however many secrets and versions there are. The search looks + in the state directory, each vault, each secret and each version, the + only directories those helpers make them in. A command that only reads + takes no lock and deletes nothing. A failure to delete is warned about + and the command goes on. An unlocker directory with no metadata file was + already removed by `secret unlocker remove` given its directory name; a + test now shows it. - 2026-10-04: `script/lint-darwin` (`make lint-darwin`) runs `go vet` and `golangci-lint` in docker on the code as a macOS build compiles it (`GOOS=darwin`), with cgo off @@ -223,15 +238,10 @@ Bring the repo into policy compliance in one commit: cross-vault copies are built in a temporary directory and renamed into place, and removals rename out of the way first, so a version or secret is never half-added and never half-removed. An - interrupted command can still leave: - - from `init` or `vault create` killed after the passphrase prompt - but before the unlocker is written, a vault with no unlocker, - which `vault create` has already made the current vault; - - data under a `.tmp-` name in the state directory: a secret, - version or unlocker being added, or the secret, version, unlocker - or vault being removed, encrypted keys included. Nothing deletes - it; it must be deleted by hand - (https://git.eeqj.de/sneak/secret/issues/75). + interrupted command can still leave, from `init` or `vault create` + killed after the passphrase prompt but before the unlocker is + written, a vault with no unlocker, which `vault create` has already + made the current vault. - 2026-10-03: The checks run before changing a vault now stop with an error naming the path and cause when they cannot read what they inspect, instead of reading the failure as "nothing there": the diff --git a/internal/cli/leftovers_test.go b/internal/cli/leftovers_test.go new file mode 100644 index 0000000..04d5a1e --- /dev/null +++ b/internal/cli/leftovers_test.go @@ -0,0 +1,74 @@ +package cli_test + +import ( + "io" + "testing" + + "git.eeqj.de/sneak/secret/internal/cli" + "git.eeqj.de/sneak/secret/internal/secret" + "git.eeqj.de/sneak/secret/internal/vault" + "github.com/spf13/afero" + "github.com/spf13/cobra" + "github.com/stretchr/testify/require" +) + +// TestLeftoversRemovedByNextChangingCommand is a regression test for +// https://git.eeqj.de/sneak/secret/issues/75. It plants what a command +// killed part-way leaves in each directory where secret.TempDirFor and +// secret.WriteFileAtomic make temporary entries: a temporary directory +// holding a vault, secret, unlocker or version being added or removed, and +// a temporary file beside a file being replaced. `secret list` must leave +// them all, and the next command that takes the state directory lock, here +// `secret vault select` of the vault already current, must delete exactly +// them: a vault named like a temporary directory stays. The copy has no +// lock file yet, so that command, as after a killed one, finds no mark that +// the last holder of the lock finished. +func TestLeftoversRemovedByNextChangingCommand(t *testing.T) { + t.Parallel() + + fs := newTwoVaultFs(t) + + _, err := vault.CreateVault(fs, testStateDir, ".tmp-0", nil) + require.NoError(t, err) + require.NoError(t, vault.SelectVault(fs, testStateDir, "default")) + + before := snapshotStateDir(t, fs) + + vaultDir := testStateDir + "/vaults.d/default" + secretDir := vaultDir + "/secrets.d/x" + + versions, err := secret.ListVersions(fs, secretDir) + require.NoError(t, err) + require.Len(t, versions, 1) + + for _, dir := range []string{ + testStateDir + "/.tmp-1/default", + vaultDir + "/.tmp-2/x", + secretDir + "/.tmp-3/" + testVersion, + } { + require.NoError(t, fs.MkdirAll(dir, secret.DirPerms)) + require.NoError(t, afero.WriteFile(fs, dir+"/value.age", + []byte("encrypted"), secret.FilePerms)) + } + + for _, file := range []string{ + testStateDir + "/.currentvault.tmp-4", + vaultDir + "/.current-unlocker.tmp-5", + secretDir + "/.current.tmp-6", + secretDir + "/versions/" + versions[0] + "/.metadata.age.tmp-7", + } { + require.NoError(t, afero.WriteFile(fs, file, + []byte("partial"), secret.FilePerms)) + } + + planted := snapshotStateDir(t, fs) + c := cli.NewCLIInstanceWithStateDir(fs, testStateDir) + cmd := &cobra.Command{} + cmd.SetOut(io.Discard) + + require.NoError(t, c.ListSecrets(cmd, false, false, "")) + require.Equal(t, planted, snapshotStateDir(t, fs)) + + require.NoError(t, c.SelectVault(cmd, "default")) + require.Equal(t, before, snapshotStateDir(t, fs)) +} diff --git a/internal/cli/lock_test.go b/internal/cli/lock_test.go index ccf77a5..abe7b8b 100644 --- a/internal/cli/lock_test.go +++ b/internal/cli/lock_test.go @@ -362,7 +362,6 @@ func requireWaitsForLock( fs := afero.NewMemMapFs() olderVersion, unlockerID := setupEveryCommand(t, fs, withUnlocker) - before := stateDirModTimes(t, fs) release, err := vault.LockStateDir(fs, testStateDir) require.NoError(t, err) @@ -372,6 +371,9 @@ func requireWaitsForLock( release = sync.OnceFunc(release) defer release() + // Taken only now, since taking the lock writes the lock file. + before := stateDirModTimes(t, fs) + unlockPassphrase := memguard.NewBufferFromBytes([]byte(testPassphrase)) defer unlockPassphrase.Destroy() diff --git a/internal/cli/path_traversal_test.go b/internal/cli/path_traversal_test.go index c8816fd..090ff10 100644 --- a/internal/cli/path_traversal_test.go +++ b/internal/cli/path_traversal_test.go @@ -90,6 +90,8 @@ func newTwoVaultFs(t *testing.T) afero.Fs { // snapshotStateDir maps every file under the state directory to its // contents, and every directory, written with a trailing "/", to "". Two // snapshots are equal only if nothing in it was added, removed or changed. +// The lock file, which every command that takes the lock writes, is left +// out. func snapshotStateDir(t *testing.T, fs afero.Fs) map[string]string { t.Helper() @@ -102,6 +104,10 @@ func snapshotStateDir(t *testing.T, fs afero.Fs) map[string]string { return err } + if path == testStateDir+"/lock" { + return nil + } + if info.IsDir() { tree[path+"/"] = "" diff --git a/internal/cli/unlockers_corrupt_test.go b/internal/cli/unlockers_corrupt_test.go index 7047c9b..1ea6939 100644 --- a/internal/cli/unlockers_corrupt_test.go +++ b/internal/cli/unlockers_corrupt_test.go @@ -4,9 +4,10 @@ // by its ID. These tests give the first unlocker, which sorts before the // one the commands act on, metadata that is not JSON, and check that the // commands step past it, and that it can itself be removed by its -// directory name, which `secret unlocker list` names in its warning. A -// last test checks that an unlocker whose metadata file cannot be read -// counts as the last unlocker when it is removed by its directory name. +// directory name, which `secret unlocker list` names in its warning, as can +// one with no metadata file. A last test checks that an unlocker whose +// metadata file cannot be read counts as the last unlocker when it is +// removed by its directory name. //nolint:testpackage // white-box test of unexported internals package cli @@ -106,6 +107,34 @@ func TestUnlockerRemoveWithCorruptUnlocker(t *testing.T) { } } +// TestUnlockerRemoveWithoutMetadata asserts that a partial unlocker +// directory, one with no metadata file, removed by its directory name from +// a vault with secrets, does not count as the vault's last unlocker, since +// it cannot unlock the vault, so the question says it is not. It is +// removed once the user confirms. +func TestUnlockerRemoveWithoutMetadata(t *testing.T) { + t.Parallel() + + fs := newListTestVault(t, 2) + vaultDir := testVaultDir(listTestVaultName) + unlockersDir := filepath.Join(vaultDir, listTestUnlockersDirName) + + require.NoError(t, fs.Remove(filepath.Join( + unlockersDir, listTestUnlockerDirOne, listTestMetadataFileName))) + writeTestSecret(t, fs, vaultDir) + + instance, cmd := newTestInstance(fs) + + found, err := instance.findUnlockerToRemove(listTestUnlockerDirOne) + require.NoError(t, err) + assert.False(t, found.last) + assert.Contains(t, found.question, "not the vault's last unlocker") + + instance.terminal = strings.NewReader("y\n") + require.NoError(t, instance.UnlockersRemove(listTestUnlockerDirOne, false, cmd)) + assertDirEntries(t, fs, unlockersDir, listTestUnlockerDirTwo) +} + // TestUnlockerRemoveWithUnreadableMetadata asserts that the only unlocker // of a vault with secrets, removed by its directory name when its metadata // file cannot be checked for or read, counts as the vault's last unlocker, diff --git a/internal/secret/atomic.go b/internal/secret/atomic.go index 3870a5b..5c2f6f9 100644 --- a/internal/secret/atomic.go +++ b/internal/secret/atomic.go @@ -5,10 +5,15 @@ import ( "fmt" "os" "path/filepath" + "strings" "github.com/spf13/afero" ) +// tempNamePart is in the name of every temporary file WriteFileAtomic makes, +// ".NAME.tmp-123", and every temporary directory TempDirFor makes, ".tmp-123". +const tempNamePart = ".tmp-" + // WriteFileAtomic replaces the file at path with data so that a reader, or // a crash at any moment, finds either the old content or the new, never a // partial file. The data goes into a temporary file that afero.TempFile @@ -17,7 +22,7 @@ import ( // temporary file is removed if any step fails. func WriteFileAtomic(fs afero.Fs, path string, data []byte) error { tmp, err := afero.TempFile(fs, filepath.Dir(path), - "."+filepath.Base(path)+".tmp-*") + "."+filepath.Base(path)+tempNamePart+"*") if err != nil { return fmt.Errorf("failed to create temporary file for %s: %w", path, err) } @@ -54,7 +59,7 @@ func WriteFileAtomic(fs afero.Fs, path string, data []byte) error { // Its name leaves out target's, which may already be as long as a file name // can be. func TempDirFor(fs afero.Fs, target string) (string, error) { - dir, err := afero.TempDir(fs, filepath.Dir(filepath.Dir(target)), ".tmp-") + dir, err := afero.TempDir(fs, filepath.Dir(filepath.Dir(target)), tempNamePart) if err != nil { return "", fmt.Errorf( "failed to create temporary directory for %s: %w", target, err) @@ -63,6 +68,40 @@ func TempDirFor(fs afero.Fs, target string) (string, error) { return dir, nil } +// RemoveLeftovers deletes from dir the temporary files of WriteFileAtomic +// and the temporary directories of TempDirFor that a command killed +// part-way left there: each entry whose name starts with "." and holds +// tempNamePart. The caller must hold the state directory lock, so that no +// running command is still using one. A dir that does not exist holds none. +func RemoveLeftovers(fs afero.Fs, dir string) error { + entries, err := afero.ReadDir(fs, dir) + if errors.Is(err, os.ErrNotExist) { + return nil + } + + if err != nil { + return fmt.Errorf("failed to read %s: %w", dir, err) + } + + for _, entry := range entries { + name := entry.Name() + if !strings.HasPrefix(name, ".") || !strings.Contains(name, tempNamePart) { + continue + } + + path := filepath.Join(dir, name) + + err = fs.RemoveAll(path) + if err != nil { + return fmt.Errorf("failed to remove %s: %w", path, err) + } + + Debug("Removed what an interrupted command left", "path", path) + } + + return nil +} + // WriteDir calls write to write the files of the new directory dir into a // temporary directory from TempDirFor, which is then renamed to dir, so that // neither a failure nor a crash leaves dir half-written; on a failure the diff --git a/internal/vault/lock.go b/internal/vault/lock.go index 3639c9c..4a60777 100644 --- a/internal/vault/lock.go +++ b/internal/vault/lock.go @@ -1,6 +1,7 @@ package vault import ( + "errors" "fmt" "os" "path/filepath" @@ -14,6 +15,11 @@ import ( // lockFileName is the file in the state directory that LockStateDir locks. const lockFileName = "lock" +// finishedMark is what the lock file holds once the command that last held +// the lock has released it. A command killed while holding it leaves the +// file empty. +const finishedMark = "finished\n" + // memFsLock stands in for the lock file on the in-memory filesystem, which // has no file locks. Every in-memory filesystem in the process shares it. // @@ -25,6 +31,12 @@ var memFsLock sync.Mutex // it. While one command holds it, the next one waits here. Reads take no // lock: each file or directory a command changes is replaced in a single // rename, so a reader finds it as it was before or after, never half-made. +// Once it holds the lock, it empties the lock file, and the function it +// returns writes finishedMark there just before releasing the lock, so a +// command killed while holding the lock leaves the mark missing. Finding it +// missing, LockStateDir first deletes the temporary files and directories +// such a command may have left, since no command still using them can be +// running. After a command that finished, it searches nothing. // // On the real filesystem the lock is flock(2) on the file "lock" in // stateDir, which the kernel releases when the process dies, so a killed @@ -32,16 +44,97 @@ var memFsLock sync.Mutex // use has no file locks, so a process-wide mutex stands in for flock there. // Any other filesystem is refused rather than left unlocked. func LockStateDir(fs afero.Fs, stateDir string) (func(), error) { + var release func() + switch fs.(type) { case *afero.OsFs: - return flockStateDir(stateDir) + var err error + + release, err = flockStateDir(stateDir) + if err != nil { + return nil, err + } case *afero.MemMapFs: memFsLock.Lock() - return memFsLock.Unlock, nil + release = memFsLock.Unlock default: return nil, fmt.Errorf("%w %T", ErrNoLockForFilesystem, fs) } + + // The lock file is written in place, never replaced: a command waiting + // for flock on the old file would then take a lock nobody else checks. + lockPath := filepath.Join(stateDir, lockFileName) + + mark, err := afero.ReadFile(fs, lockPath) + if err != nil || string(mark) != finishedMark { + removeLeftovers(fs, stateDir) + } + + err = afero.WriteFile(fs, lockPath, nil, secret.FilePerms) + if err != nil { + release() + + return nil, fmt.Errorf("failed to empty lock file %s: %w", lockPath, err) + } + + return func() { + // If this fails, the next command searches when it need not. + _ = afero.WriteFile(fs, lockPath, []byte(finishedMark), secret.FilePerms) + + release() + }, nil +} + +// removeLeftovers deletes the temporary files and directories that commands +// killed part-way left in each directory where secret.WriteFileAtomic and +// secret.TempDirFor make them: the state directory, each vault, each secret +// and each version. Unlocker directories are written whole by +// secret.WriteDir and never changed after, so they hold none. A failure is +// only warned about, and the command goes on. +func removeLeftovers(fs afero.Fs, stateDir string) { + dirs := []string{stateDir} + + for _, vaultDir := range subdirs(fs, filepath.Join(stateDir, "vaults.d")) { + dirs = append(dirs, vaultDir) + + for _, secretDir := range subdirs(fs, filepath.Join(vaultDir, "secrets.d")) { + dirs = append(dirs, secretDir) + dirs = append(dirs, subdirs(fs, filepath.Join(secretDir, "versions"))...) + } + } + + for _, dir := range dirs { + err := secret.RemoveLeftovers(fs, dir) + if err != nil { + secret.Warn("Failed to remove what an interrupted command left", + "error", err) + } + } +} + +// subdirs returns the directories in dir: none if dir does not exist, and +// none, with a warning, if it cannot be read. +func subdirs(fs afero.Fs, dir string) []string { + entries, err := afero.ReadDir(fs, dir) + if err != nil { + if !errors.Is(err, os.ErrNotExist) { + secret.Warn("Failed to look for what an interrupted command left", + "directory", dir, "error", err) + } + + return nil + } + + var dirs []string + + for _, entry := range entries { + if entry.IsDir() { + dirs = append(dirs, filepath.Join(dir, entry.Name())) + } + } + + return dirs } // flockStateDir takes flock(2) on the lock file in stateDir, creating the diff --git a/internal/vault/lock_test.go b/internal/vault/lock_test.go index d6882fc..d34d909 100644 --- a/internal/vault/lock_test.go +++ b/internal/vault/lock_test.go @@ -1,9 +1,11 @@ package vault_test import ( + "path/filepath" "testing" "time" + "git.eeqj.de/sneak/secret/internal/secret" "git.eeqj.de/sneak/secret/internal/vault" "github.com/spf13/afero" "github.com/stretchr/testify/assert" @@ -121,6 +123,51 @@ func TestLockStateDirFreeAfterPanic(t *testing.T) { } } +// TestLockStateDirRemovesLeftoversOnlyAfterKill checks that taking the lock +// deletes a temporary directory a killed command left only when the last +// holder of the lock did not release it. A holder killed while it holds the +// lock leaves the lock file as it is at that moment. +func TestLockStateDirRemovesLeftoversOnlyAfterKill(t *testing.T) { + t.Parallel() + + for _, lfs := range lockFilesystems(t) { + t.Run(lfs.name, func(t *testing.T) { + t.Parallel() + + lockFile := filepath.Join(lfs.stateDir, "lock") + leftover := filepath.Join(lfs.stateDir, ".tmp-1") + + release, err := vault.LockStateDir(lfs.fs, lfs.stateDir) + require.NoError(t, err) + + whileHeld, err := afero.ReadFile(lfs.fs, lockFile) + require.NoError(t, err) + release() + + require.NoError(t, lfs.fs.MkdirAll(leftover, secret.DirPerms)) + + release, err = vault.LockStateDir(lfs.fs, lfs.stateDir) + require.NoError(t, err) + release() + + exists, err := afero.DirExists(lfs.fs, leftover) + require.NoError(t, err) + assert.True(t, exists, "searched after a holder that finished") + + require.NoError(t, afero.WriteFile(lfs.fs, lockFile, whileHeld, + secret.FilePerms)) + + release, err = vault.LockStateDir(lfs.fs, lfs.stateDir) + require.NoError(t, err) + release() + + exists, err = afero.DirExists(lfs.fs, leftover) + require.NoError(t, err) + assert.False(t, exists, "not searched after a holder that was killed") + }) + } +} + // TestLockStateDirRefusesOtherFilesystems checks that a filesystem with no // lock implementation is refused instead of being used unlocked. func TestLockStateDirRefusesOtherFilesystems(t *testing.T) {