diff --git a/internal/cli/move_cross_vault_test.go b/internal/cli/move_cross_vault_test.go index 6ee0ee6..97ffee7 100644 --- a/internal/cli/move_cross_vault_test.go +++ b/internal/cli/move_cross_vault_test.go @@ -4,12 +4,15 @@ package cli import ( "errors" "path/filepath" + "sync" "testing" + "time" "github.com/awnumar/memguard" "github.com/spf13/afero" "github.com/spf13/cobra" "github.com/stretchr/testify/require" + "sneak.berlin/go/secret/internal/secret" "sneak.berlin/go/secret/internal/vault" ) @@ -92,7 +95,8 @@ func requireValue(t *testing.T, fs afero.Fs, vaultName, secretName string) { // 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. +// calls copySecretAndRemoveSource, which a move between vaults runs once it +// holds the lock. func TestCrossVaultMoveFailsWhenSourceRemains(t *testing.T) { t.Parallel() @@ -103,9 +107,12 @@ func TestCrossVaultMoveFailsWhenSourceRemains(t *testing.T) { 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) + source := vault.NewVault(instance.fs, testStateDir, "default") + source.Mnemonic = testMnemonicBuffer(t) + + err := instance.copySecretAndRemoveSource(&cobra.Command{}, source, "x", + vault.NewVault(instance.fs, testStateDir, "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") @@ -128,9 +135,12 @@ func TestCrossVaultMoveFailsWhenOldFilesRemain(t *testing.T) { fs := &removeAllFailFs{Fs: base, dir: sourceVaultDir} instance := NewCLIInstanceWithStateDir(fs, testStateDir) - instance.Mnemonic = testMnemonicBuffer(t) - err := instance.moveSecret(&cobra.Command{}, "default:x", "work:y", false) + source := vault.NewVault(fs, testStateDir, "default") + source.Mnemonic = testMnemonicBuffer(t) + + err := instance.copySecretAndRemoveSource(&cobra.Command{}, source, "x", + vault.NewVault(fs, testStateDir, "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") @@ -150,3 +160,138 @@ func TestCrossVaultMoveFailsWhenOldFilesRemain(t *testing.T) { requireValue(t, base, "work", "y") } + +// TestCrossVaultMoveUnlocksBeforeLocking is a regression test for +// https://git.eeqj.de/sneak/secret/issues/189: a move between two vaults +// unlocked them while holding the state directory lock, and unlocking a +// vault can ask for its passphrase, so every other command that changes +// anything waited while the user answered. Here each vault has a passphrase +// unlocker whose passphrase is the vault's name, and the move is given the +// passphrase of only one of them. Run while the test holds the lock, it +// must fail to unlock the other vault without waiting for the lock. +// +//nolint:paralleltest // times commands against the in-memory lock all tests share +func TestCrossVaultMoveUnlocksBeforeLocking(t *testing.T) { + fs := afero.NewMemMapFs() + createCrossVaults(t, fs) + + vaults, err := vault.ListVaults(fs, testStateDir) + require.NoError(t, err) + + for _, name := range vaults { + vlt := vault.NewVault(fs, testStateDir, name) + vlt.Mnemonic = testMnemonicBuffer(t) + + passphrase := memguard.NewBufferFromBytes([]byte(name)) + _, err = vlt.CreatePassphraseUnlocker(passphrase) + passphrase.Destroy() + require.NoError(t, err) + } + + // The move is from "default" to "work" + for _, tc := range []struct { + // failing is the vault that the move cannot unlock, given the + // passphrase of the vault named given + failing, given string + }{ + {"source", "work"}, + {"destination", "default"}, + } { + t.Run(tc.failing, func(t *testing.T) { + passphrase := memguard.NewBufferFromBytes([]byte(tc.given)) + defer passphrase.Destroy() + + instance := NewCLIInstanceWithStateDir(fs, testStateDir) + instance.UnlockPassphrase = passphrase + + release, err := vault.LockStateDir(fs, testStateDir) + require.NoError(t, err) + // Also frees a waiting move if the test fails, so that it + // releases the lock the other tests use + defer release() + + done := make(chan error, 1) + + go func() { + done <- instance.MoveSecret(&cobra.Command{}, + "default:x", "work:y", false) + }() + + select { + case err := <-done: + require.ErrorIs(t, err, secret.ErrWrongPassphrase) + require.ErrorContains(t, err, + "failed to unlock "+tc.failing+" vault") + case <-time.After(lockWait): + t.Fatal("waited for the lock before unlocking the " + + tc.failing + " vault") + } + }) + } +} + +// TestCrossVaultMoveChecksAgainOnceLocked starts a move between two vaults, +// given the mnemonic, while the test holds the state directory lock. Once +// the move waits for the lock, the test removes what another command could +// have removed in the meantime, then releases the lock. The move must fail +// with the error that says what is missing. +// +//nolint:paralleltest // waitingForLock sees any test's command waiting for the lock +func TestCrossVaultMoveChecksAgainOnceLocked(t *testing.T) { + for _, tc := range []struct { + name string + // removed is the directory, under the state directory, that the + // test removes + removed string + want error + }{ + {"source", "/vaults.d/default/secrets.d/x", vault.ErrSecretNotFound}, + {"destination vault", "/vaults.d/work", vault.ErrVaultNotFound}, + } { + t.Run(tc.name, func(t *testing.T) { + fs := afero.NewMemMapFs() + createCrossVaults(t, fs) + + release, err := vault.LockStateDir(fs, testStateDir) + require.NoError(t, err) + + // Released at most once, and also if the test fails while + // holding it, so that later tests can take it + release = sync.OnceFunc(release) + defer release() + + instance := NewCLIInstanceWithStateDir(fs, testStateDir) + instance.Mnemonic = testMnemonicBuffer(t) + + done := make(chan error, 1) + + go func() { + done <- instance.MoveSecret(&cobra.Command{}, + "default:x", "work:y", false) + }() + + timeout := time.After(lockWait) + + for !waitingForLock() { + select { + case err := <-done: + t.Fatalf("finished while the lock was held, with error %v", + err) + case <-timeout: + t.Fatal("never waited for the lock") + case <-time.After(time.Millisecond): + } + } + + require.NoError(t, fs.RemoveAll(testStateDir+tc.removed)) + release() + + select { + case err := <-done: + require.ErrorIs(t, err, tc.want) + case <-time.After(lockWait): + t.Fatal("did not finish once the lock was released") + } + }) + } +} diff --git a/internal/cli/secrets.go b/internal/cli/secrets.go index 4f69b52..81a8939 100644 --- a/internal/cli/secrets.go +++ b/internal/cli/secrets.go @@ -816,24 +816,12 @@ func (cli *Instance) findSecretToRemove( }, nil } -// MoveSecret moves or renames a secret (within or across vaults), holding -// the state directory lock. +// MoveSecret moves or renames a secret (within or across vaults). It +// leaves taking the state directory lock to moveSecretWithinVault and +// moveSecretCrossVault, since a move between vaults unlocks both vaults +// before it takes the lock. func (cli *Instance) MoveSecret( cmd *cobra.Command, source, dest string, force bool, -) error { - release, err := vault.LockStateDir(cli.fs, cli.stateDir) - if err != nil { - return err - } - defer release() - - return cli.moveSecret(cmd, source, dest, force) -} - -// moveSecret does the work of MoveSecret. The caller holds the state -// directory lock. -func (cli *Instance) moveSecret( - cmd *cobra.Command, source, dest string, force bool, ) error { // Parse source and destination srcVaultName, srcSecretName, srcQualified := ParseVaultSecretRef(source) @@ -933,8 +921,9 @@ func (cli *Instance) existingVault(name string) (*vault.Vault, error) { return vault.NewVault(cli.fs, cli.stateDir, name), nil } -// moveSecretWithinVault renames a secret within the vault vlt. Its caller, -// MoveSecret, has already checked both secret names. +// moveSecretWithinVault renames a secret within the vault vlt, holding the +// state directory lock. Its caller, MoveSecret, has already checked both +// secret names. func (cli *Instance) moveSecretWithinVault( cmd *cobra.Command, vlt *vault.Vault, source, dest string, force bool, ) error { @@ -944,6 +933,12 @@ func (cli *Instance) moveSecretWithinVault( return fmt.Errorf("secret '%s' %w", source, errMoveOntoItself) } + release, err := vault.LockStateDir(cli.fs, cli.stateDir) + if err != nil { + return err + } + defer release() + vaultDir, err := vlt.GetDirectory() if err != nil { return err @@ -1030,50 +1025,22 @@ func (cli *Instance) sameDirectory(dir, other string) (bool, error) { // moveSecretCrossVault handles moving between two different vaults. Its // caller, MoveSecret, has already checked both secret names and that both -// vaults exist. +// vaults exist. Copying needs the long-term keys of both vaults, and getting +// one can ask for a passphrase, so it gets them before it takes the state +// directory lock, and no other command waits while the user answers. func (cli *Instance) moveSecretCrossVault( cmd *cobra.Command, srcVault *vault.Vault, srcSecretName string, destVault *vault.Vault, destSecretName string, force bool, ) error { - srcVaultDir, err := srcVault.GetDirectory() - if err != nil { - return fmt.Errorf("failed to get source vault directory: %w", err) - } - - // Verify source secret exists - srcStorageName := strings.ReplaceAll(srcSecretName, "/", "%") - srcSecretDir := filepath.Join(srcVaultDir, "secrets.d", srcStorageName) - - exists, err := afero.DirExists(cli.fs, srcSecretDir) - if err != nil || !exists { - return fmt.Errorf("secret '%s' %w in vault '%s'", - srcSecretName, vault.ErrSecretNotFound, srcVault.Name) - } - - // The source is removed after the copy, so a destination that is the - // source under another name would be lost with it. - destVaultDir, err := destVault.GetDirectory() - if err != nil { - return fmt.Errorf("failed to get destination vault directory: %w", err) - } - - destStorageName := strings.ReplaceAll(destSecretName, "/", "%") - destSecretDir := filepath.Join(destVaultDir, "secrets.d", destStorageName) - - same, err := cli.sameDirectory(srcSecretDir, destSecretDir) + // Refuse a move that cannot be made before asking for anything + _, err := cli.checkCrossVaultMove( + srcVault, srcSecretName, destVault, destSecretName) if err != nil { return err } - if same { - return fmt.Errorf("secret '%s:%s' %w: '%s:%s' is the same secret on "+ - "this filesystem", srcVault.Name, srcSecretName, errMoveOntoItself, - destVault.Name, destSecretName) - } - - // Copying needs the long-term keys of both vaults srcVault.Mnemonic, srcVault.UnlockPassphrase = cli.Mnemonic, cli.UnlockPassphrase destVault.Mnemonic, destVault.UnlockPassphrase = cli.Mnemonic, cli.UnlockPassphrase @@ -1083,6 +1050,39 @@ func (cli *Instance) moveSecretCrossVault( return fmt.Errorf("failed to unlock destination vault '%s': %w", destVault.Name, err) } + _, err = srcVault.GetOrDeriveLongTermKey() + if err != nil { + return fmt.Errorf("failed to unlock source vault '%s': %w", + srcVault.Name, err) + } + + release, err := vault.LockStateDir(cli.fs, cli.stateDir) + if err != nil { + return err + } + defer release() + + return cli.copySecretAndRemoveSource( + cmd, srcVault, srcSecretName, destVault, destSecretName, force) +} + +// copySecretAndRemoveSource copies the secret srcSecretName of srcVault, +// with all its versions, to destSecretName in destVault, and removes the +// source. moveSecretCrossVault calls it holding the state directory lock. +// Another command may have changed either vault since moveSecretCrossVault +// checked them, so it checks them again first. +func (cli *Instance) copySecretAndRemoveSource( + cmd *cobra.Command, + srcVault *vault.Vault, srcSecretName string, + destVault *vault.Vault, destSecretName string, + force bool, +) error { + srcSecretDir, err := cli.checkCrossVaultMove( + srcVault, srcSecretName, destVault, destSecretName) + if err != nil { + return err + } + // Count versions for user feedback versions, _ := secret.ListVersions(cli.fs, srcSecretDir) versionCount := len(versions) @@ -1108,6 +1108,57 @@ func (cli *Instance) moveSecretCrossVault( return nil } +// checkCrossVaultMove checks that the secret srcSecretName exists in +// srcVault, that destVault exists, and that destSecretName in destVault is +// not the source under another name. It returns the source's directory. +func (cli *Instance) checkCrossVaultMove( + srcVault *vault.Vault, srcSecretName string, + destVault *vault.Vault, destSecretName string, +) (string, error) { + srcVaultDir, err := srcVault.GetDirectory() + if err != nil { + return "", fmt.Errorf("failed to get source vault directory: %w", err) + } + + // Verify source secret exists + srcStorageName := strings.ReplaceAll(srcSecretName, "/", "%") + srcSecretDir := filepath.Join(srcVaultDir, "secrets.d", srcStorageName) + + exists, err := afero.DirExists(cli.fs, srcSecretDir) + if err != nil || !exists { + return "", fmt.Errorf("secret '%s' %w in vault '%s'", + srcSecretName, vault.ErrSecretNotFound, srcVault.Name) + } + + _, err = cli.existingVault(destVault.Name) + if err != nil { + return "", err + } + + // The source is removed after the copy, so a destination that is the + // source under another name would be lost with it. + destVaultDir, err := destVault.GetDirectory() + if err != nil { + return "", fmt.Errorf("failed to get destination vault directory: %w", err) + } + + destStorageName := strings.ReplaceAll(destSecretName, "/", "%") + destSecretDir := filepath.Join(destVaultDir, "secrets.d", destStorageName) + + same, err := cli.sameDirectory(srcSecretDir, destSecretDir) + if err != nil { + return "", err + } + + if same { + return "", fmt.Errorf("secret '%s:%s' %w: '%s:%s' is the same secret "+ + "on this filesystem", srcVault.Name, srcSecretName, errMoveOntoItself, + destVault.Name, destSecretName) + } + + return srcSecretDir, 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