diff --git a/internal/cli/move_cross_vault_test.go b/internal/cli/move_cross_vault_test.go new file mode 100644 index 0000000..6ee0ee6 --- /dev/null +++ b/internal/cli/move_cross_vault_test.go @@ -0,0 +1,152 @@ +//nolint:testpackage // white-box test of unexported internals +package cli + +import ( + "errors" + "path/filepath" + "testing" + + "github.com/awnumar/memguard" + "github.com/spf13/afero" + "github.com/spf13/cobra" + "github.com/stretchr/testify/require" + "sneak.berlin/go/secret/internal/vault" +) + +// errPermissionDenied is returned by renameFailFs and removeAllFailFs in +// place of a rename or removal that succeeds. +var errPermissionDenied = errors.New("permission denied") + +// renameFailFs fails every Rename of one path, as a read-only parent +// directory does. +type renameFailFs struct { + afero.Fs + + path string +} + +func (f *renameFailFs) Rename(oldname, newname string) error { + if oldname == f.path { + return errPermissionDenied + } + + return f.Fs.Rename(oldname, newname) +} + +// removeAllFailFs fails every RemoveAll of an entry of the directory dir, +// as a file in it that cannot be deleted does, and records that entry's +// path in failed. +type removeAllFailFs struct { + afero.Fs + + dir string + failed string +} + +func (f *removeAllFailFs) RemoveAll(path string) error { + if filepath.Dir(path) == f.dir { + f.failed = path + + return errPermissionDenied + } + + return f.Fs.RemoveAll(path) +} + +// createCrossVaults creates in fs the vaults "work" and "default", where +// "default" holds the secret "x" with the value "value". +func createCrossVaults(t *testing.T, fs afero.Fs) { + t.Helper() + + _, err := vault.CreateVault(fs, testStateDir, "work", + testMnemonicBuffer(t), nil) + require.NoError(t, err) + + source, err := vault.CreateVault(fs, testStateDir, "default", + testMnemonicBuffer(t), nil) + require.NoError(t, err) + + err = source.AddSecret("x", + memguard.NewBufferFromBytes([]byte("value")), false) + require.NoError(t, err) +} + +// requireValue requires that the vault vaultName in fs holds secretName +// with the value createCrossVaults stored. +func requireValue(t *testing.T, fs afero.Fs, vaultName, secretName string) { + t.Helper() + + vlt := vault.NewVault(fs, testStateDir, vaultName) + vlt.Mnemonic = testMnemonicBuffer(t) + + value, err := vlt.GetSecret(secretName) + require.NoError(t, err, vaultName+":"+secretName) + + defer value.Destroy() + + require.Equal(t, []byte("value"), value.Bytes()) +} + +// TestCrossVaultMoveFailsWhenSourceRemains is a regression test for +// https://git.eeqj.de/sneak/secret/issues/159, where a move between two +// vaults whose source could not be removed reported the secret moved and +// succeeded. It must fail, and the copy and the source must both be there. +// The state directory lock refuses the wrapped filesystem, so the test +// calls moveSecret, which MoveSecret runs once it holds the lock. +func TestCrossVaultMoveFailsWhenSourceRemains(t *testing.T) { + t.Parallel() + + base := afero.NewMemMapFs() + createCrossVaults(t, base) + + instance := NewCLIInstanceWithStateDir(&renameFailFs{ + Fs: base, + path: testStateDir + "/vaults.d/default/secrets.d/x", + }, testStateDir) + instance.Mnemonic = testMnemonicBuffer(t) + + err := instance.moveSecret(&cobra.Command{}, "default:x", "work:y", false) + require.ErrorIs(t, err, errPermissionDenied) + require.ErrorContains(t, err, "secret 'default:x' is copied with all "+ + "1 version(s) to 'work:y', but the source remains") + + requireValue(t, base, "default", "x") + requireValue(t, base, "work", "y") +} + +// TestCrossVaultMoveFailsWhenOldFilesRemain makes the removal of the source +// fail after the source has been moved out of its vault, when only the +// temporary directory it was moved into cannot be deleted. The move must +// fail, say that the secret is moved, and name that directory. +func TestCrossVaultMoveFailsWhenOldFilesRemain(t *testing.T) { + t.Parallel() + + base := afero.NewMemMapFs() + createCrossVaults(t, base) + + sourceVaultDir := testStateDir + "/vaults.d/default" + fs := &removeAllFailFs{Fs: base, dir: sourceVaultDir} + + instance := NewCLIInstanceWithStateDir(fs, testStateDir) + instance.Mnemonic = testMnemonicBuffer(t) + + err := instance.moveSecret(&cobra.Command{}, "default:x", "work:y", false) + require.ErrorIs(t, err, errPermissionDenied) + require.ErrorContains(t, err, "secret 'default:x' is moved with all "+ + "1 version(s) to 'work:y', but its old files could not all be deleted") + + // The error names the directory that holds the old files + require.NotEmpty(t, fs.failed) + require.ErrorContains(t, err, fs.failed) + + oldFiles, err := afero.DirExists(base, filepath.Join(fs.failed, "x")) + require.NoError(t, err) + require.True(t, oldFiles) + + // The source vault no longer holds the secret + remains, err := afero.DirExists(base, sourceVaultDir+"/secrets.d/x") + require.NoError(t, err) + require.False(t, remains) + + requireValue(t, base, "work", "y") +} diff --git a/internal/cli/secrets.go b/internal/cli/secrets.go index 59c64e9..4f69b52 100644 --- a/internal/cli/secrets.go +++ b/internal/cli/secrets.go @@ -1097,12 +1097,9 @@ func (cli *Instance) moveSecretCrossVault( // Delete source secret err = secret.RemoveDirAtomic(cli.fs, srcSecretDir) if err != nil { - // Copy succeeded but delete failed - warn but don't fail - cmd.Printf("Warning: copied secret but failed to remove source: %v\n", err) - cmd.Printf("Moved secret '%s:%s' to '%s:%s' (%d version(s))\n", - srcVault.Name, srcSecretName, destVault.Name, destSecretName, versionCount) - - return nil + return cli.sourceRemovalError(err, srcSecretDir, + srcVault.Name+":"+srcSecretName, destVault.Name+":"+destSecretName, + versionCount) } cmd.Printf("Moved secret '%s:%s' to '%s:%s' (%d version(s))\n", @@ -1110,3 +1107,31 @@ func (cli *Instance) moveSecretCrossVault( return nil } + +// sourceRemovalError returns the error of a move between vaults that +// copied all versionCount versions of the secret srcName to destName, and +// then failed with removeErr to remove srcSecretDir, the directory of +// srcName. The removal can fail after the source has left its vault, when +// only the temporary directory it was moved into cannot be deleted, so the +// error says the source remains only if srcSecretDir is still there. +func (cli *Instance) sourceRemovalError( + removeErr error, srcSecretDir, srcName, destName string, versionCount int, +) error { + remains, err := afero.DirExists(cli.fs, srcSecretDir) + + switch { + case err != nil: + return fmt.Errorf("secret '%s' is copied with all %d version(s) "+ + "to '%s', but removing the source failed and whether it "+ + "remains is unknown: %w", + srcName, versionCount, destName, errors.Join(removeErr, err)) + case remains: + return fmt.Errorf("secret '%s' is copied with all %d version(s) "+ + "to '%s', but the source remains: %w", + srcName, versionCount, destName, removeErr) + default: + return fmt.Errorf("secret '%s' is moved with all %d version(s) "+ + "to '%s', but its old files could not all be deleted: %w", + srcName, versionCount, destName, removeErr) + } +} diff --git a/internal/secret/atomic.go b/internal/secret/atomic.go index 5c2f6f9..f21e9ee 100644 --- a/internal/secret/atomic.go +++ b/internal/secret/atomic.go @@ -149,7 +149,9 @@ func WriteDir(fs afero.Fs, dir string, write func(dir string) error) error { // RemoveDirAtomic deletes the directory dir so that it disappears in one // rename: dir is moved into a new directory from TempDirFor, which is then -// deleted. A crash part-way leaves only that temporary directory behind. +// deleted. A crash part-way leaves only that temporary directory behind. If +// only deleting the temporary directory fails, dir is already gone, and the +// error names the temporary directory, which holds what is left of it. func RemoveDirAtomic(fs afero.Fs, dir string) error { tmp, err := TempDirFor(fs, dir) if err != nil { @@ -165,7 +167,8 @@ func RemoveDirAtomic(fs afero.Fs, dir string) error { err = fs.RemoveAll(tmp) if err != nil { - return fmt.Errorf("failed to remove %s: %w", dir, err) + return fmt.Errorf("failed to remove %s after moving %s into it: %w", + tmp, dir, err) } return nil