diff --git a/TODO.md b/TODO.md index 40bc2fb..a13d985 100644 --- a/TODO.md +++ b/TODO.md @@ -25,6 +25,17 @@ Bring the repo into policy compliance in one commit: # Completed Steps +- 2026-10-04: The next command that takes the state directory lock deletes + what commands 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`, in the state directory, each vault, each secret + and each version, the only directories those make them in. Before, they + stayed, encrypted keys included, until deleted by hand. 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: A crash while an unlocker is being replaced no longer leaves a current unlocker that cannot open the vault (https://git.eeqj.de/sneak/secret/issues/71). Every new unlocker gets a @@ -188,15 +199,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..f4b8197 --- /dev/null +++ b/internal/cli/leftovers_test.go @@ -0,0 +1,72 @@ +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. +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/unlockers_corrupt_test.go b/internal/cli/unlockers_corrupt_test.go index d7dd8c7..3a4134a 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 is -// removed by its directory name only as the last unlocker is. +// 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 is removed by its directory name only as the +// last unlocker is. //nolint:testpackage // white-box test of unexported internals package cli @@ -113,6 +114,27 @@ func TestUnlockerRemoveWithCorruptUnlocker(t *testing.T) { } } +// TestUnlockerRemoveWithoutMetadata asserts that a partial unlocker +// directory, one with no metadata file, is removed by its directory name +// without --force from a vault with secrets: it cannot unlock the vault, so +// removing it never removes the last unlocker. +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) + + require.NoError(t, instance.UnlockersRemove(listTestUnlockerDirOne, false, cmd)) + assertDirEntries(t, fs, unlockersDir, listTestUnlockerDirTwo) +} + // TestUnlockerRemoveWithUnreadableMetadata asserts that removing the only // unlocker of a vault with secrets by its directory name, when its // metadata file cannot be checked for or read, is refused without --force: 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..ef849f7 100644 --- a/internal/vault/lock.go +++ b/internal/vault/lock.go @@ -1,6 +1,7 @@ package vault import ( + "errors" "fmt" "os" "path/filepath" @@ -25,6 +26,9 @@ 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 deletes the temporary files and directories +// that commands killed part-way left behind, since no command still using +// them can be running. // // 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 +36,78 @@ 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) } + + removeLeftovers(fs, stateDir) + + return 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; the command goes on, and the next one tries again. +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