From 4c47b233bb32afbba9badc9640ffa074b44f3835 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Thu, 15 Oct 2026 10:49:06 +0200 Subject: [PATCH] A secret mv between vaults unlocks both vaults before taking the lock (closes #189) A move between two vaults unlocked them while holding the state directory lock, and unlocking can ask for a passphrase, so every other changing command waited at that prompt. moveSecretCrossVault now checks the move, refusing an existing destination secret without --force, unlocks both vaults, and only then takes the lock. Under the lock, it checks that the destination vault still has the long-term key it unlocked, and copySecretAndRemoveSource checks the move again before it copies. A move within a vault takes the lock in moveSecretWithinVault, so MoveSecret takes none itself. The error for a vault created again with another key, shared with secret unlocker add, now ends "nothing was changed". Model: opus-5-5 --- README.md | 9 +- internal/cli/move_cross_vault_test.go | 173 ++++++++++++++++++++++++- internal/cli/secrets.go | 176 ++++++++++++++++++-------- internal/cli/unlockers.go | 31 +++-- 4 files changed, 318 insertions(+), 71 deletions(-) diff --git a/README.md b/README.md index 118fe02..a716bb6 100644 --- a/README.md +++ b/README.md @@ -440,10 +440,11 @@ in a CI job, the command fails and names the variable to set. A command that changes anything locks the state directory, through its file `lock`, and another such command waits until the lock is released. -`secret init`, `secret vault create`, `secret unlocker add`, and `secret add`, -`secret import` and `secret generate secret` with `--force`, ask for the -mnemonic or passphrase before they take the lock, so a prompt of theirs left -open in one terminal holds up no command in another. +`secret init`, `secret vault create`, `secret unlocker add`, `secret mv` from +one vault to another, and `secret add`, `secret import` and +`secret generate secret` with `--force`, ask for the mnemonic or passphrase +before they take the lock, so a prompt of theirs left open in one terminal holds +up no command in another. ## Security Features diff --git a/internal/cli/move_cross_vault_test.go b/internal/cli/move_cross_vault_test.go index 6ee0ee6..038147d 100644 --- a/internal/cli/move_cross_vault_test.go +++ b/internal/cli/move_cross_vault_test.go @@ -5,11 +5,13 @@ import ( "errors" "path/filepath" "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 +94,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 +106,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 +134,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 +159,157 @@ 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 runs a move between vaults, given +// the mnemonic, as TestCommandsCheckAgainOnceLocked runs the commands in its +// table: once the move waits for the state directory lock, the test removes +// the source secret or the destination vault, or creates the destination +// vault again from another mnemonic, and the move must then fail and change +// nothing. +// +//nolint:paralleltest // waitingForLock sees any test's command waiting for the lock +func TestCrossVaultMoveChecksAgainOnceLocked(t *testing.T) { + mnemonic := testMnemonicBuffer(t) + + otherMnemonic := memguard.NewBufferFromBytes([]byte("legal winner thank " + + "year wave sausage worth useful legal winner thank yellow")) + t.Cleanup(otherMnemonic.Destroy) + + move := func(cli *Instance) error { + cli.Mnemonic = mnemonic + + return cli.MoveSecret(cli.cmd, "work:test/secret", "other:moved", false) + } + + t.Run("source removed", func(t *testing.T) { + requireChecksAgainOnceLocked(t, move, func(fs afero.Fs) error { + return fs.RemoveAll(testStateDir + "/vaults.d/work/secrets.d/test%secret") + }, vault.ErrSecretNotFound) + }) + + t.Run("destination vault removed", func(t *testing.T) { + requireChecksAgainOnceLocked(t, move, func(fs afero.Fs) error { + return fs.RemoveAll(testStateDir + "/vaults.d/other") + }, vault.ErrVaultNotFound) + }) + + t.Run("destination vault created again from another mnemonic", func(t *testing.T) { + // Set by the move, which has finished once + // requireChecksAgainOnceLocked returns + var moveErr error + + requireChecksAgainOnceLocked(t, func(cli *Instance) error { + moveErr = move(cli) + + return moveErr + }, func(fs afero.Fs) error { + err := fs.RemoveAll(testStateDir + "/vaults.d/other") + if err != nil { + return err + } + + _, err = vault.CreateVault(fs, testStateDir, "other", otherMnemonic, nil) + + return err + }, errVaultCreatedAgain) + + // secret unlocker add fails with the same error, so its message + // must fit a move too + require.EqualError(t, moveErr, "vault 'other' was removed and "+ + "created again with another key while it was being unlocked; "+ + "nothing was changed") + }) +} + +// TestCrossVaultMoveRefusesExistingSecretBeforeUnlocking checks that a move +// between vaults onto a secret that exists, without --force, is refused +// before either vault is unlocked, since unlocking can ask for a +// passphrase. The vaults have no unlockers and the move is given no +// mnemonic, so unlocking either vault fails. +func TestCrossVaultMoveRefusesExistingSecretBeforeUnlocking(t *testing.T) { + t.Parallel() + + fs := afero.NewMemMapFs() + createCrossVaults(t, fs) + + work := vault.NewVault(fs, testStateDir, "work") + work.Mnemonic = testMnemonicBuffer(t) + + err := work.AddSecret("y", + memguard.NewBufferFromBytes([]byte("other")), false) + require.NoError(t, err) + + instance := NewCLIInstanceWithStateDir(fs, testStateDir) + + err = instance.MoveSecret(&cobra.Command{}, "default:x", "work:y", false) + require.ErrorIs(t, err, vault.ErrSecretExists) +} diff --git a/internal/cli/secrets.go b/internal/cli/secrets.go index f72a1af..7c8b18f 100644 --- a/internal/cli/secrets.go +++ b/internal/cli/secrets.go @@ -875,24 +875,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) @@ -992,8 +980,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 { @@ -1003,6 +992,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 @@ -1089,59 +1084,75 @@ 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. The copy needs the source vault's long-term key and is +// encrypted to the destination vault's long-term public key. The +// destination vault is unlocked to check that the copy will open with what +// the user gave, and, once the state directory lock is held, that it still +// has the key unlocked. Either unlock can ask for a passphrase, so both +// happen before the lock is taken, 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) + // Before unlocking, check that the source secret exists, that the + // destination is not the source under another name, and, without + // --force, that the destination secret does not exist + _, err := cli.checkCrossVaultMove( + srcVault, srcSecretName, destVault, destSecretName, force) 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 // Unlock destination vault (will fail if neither mnemonic nor unlocker available) - _, err = destVault.GetOrDeriveLongTermKey() + destIdentity, err := destVault.GetOrDeriveLongTermKey() if err != nil { 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() + + err = cli.checkLongTermKey(destVault, destIdentity) + if err != nil { + return err + } + + 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, force) + if err != nil { + return err + } + // Count versions for user feedback versions, _ := secret.ListVersions(cli.fs, srcSecretDir) versionCount := len(versions) @@ -1167,6 +1178,65 @@ func (cli *Instance) moveSecretCrossVault( return nil } +// checkCrossVaultMove checks that the secret srcSecretName exists in +// srcVault, that destSecretName in destVault is not the source under +// another name, and, unless force is set, that destSecretName does not +// exist in destVault. It returns the source's directory. +func (cli *Instance) checkCrossVaultMove( + srcVault *vault.Vault, srcSecretName string, + destVault *vault.Vault, destSecretName string, + force bool, +) (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) + } + + // 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) + } + + exists, err = afero.DirExists(cli.fs, destSecretDir) + if err != nil { + return "", fmt.Errorf("failed to check if destination secret exists: %w", err) + } + + if exists && !force { + return "", fmt.Errorf( + "secret '%s' %w in vault '%s' (use --force to overwrite)", + destSecretName, vault.ErrSecretExists, destVault.Name) + } + + 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 diff --git a/internal/cli/unlockers.go b/internal/cli/unlockers.go index 1bda264..5ee06c7 100644 --- a/internal/cli/unlockers.go +++ b/internal/cli/unlockers.go @@ -15,6 +15,7 @@ import ( "strings" "time" + "filippo.io/age" "github.com/spf13/afero" "github.com/spf13/cobra" "sneak.berlin/go/secret/internal/secret" @@ -42,7 +43,7 @@ var ( // errVaultCreatedAgain carries only the message tail; the caller // composes "vault '' was removed and created again ...". errVaultCreatedAgain = errors.New("was removed and created again " + - "with another key while it was being unlocked; nothing was added") + "with another key while it was being unlocked; nothing was changed") ) // UnlockerInfo represents unlocker information for display @@ -442,29 +443,41 @@ func (cli *Instance) unlockThenLock(vlt *vault.Vault) (func(), error) { return nil, err } - _, err = cli.existingVault(vlt.GetName()) + err = cli.checkLongTermKey(vlt, ltIdentity) if err != nil { release() return nil, err } + return release, nil +} + +// checkLongTermKey checks, for the vault vlt unlocked as ltIdentity before +// the state directory lock was taken, that vlt still exists and that its +// long-term public key is still that of ltIdentity. Another command may have +// removed the vault in the meantime, or removed it and created it again with +// another long-term key. The caller holds the lock. +func (cli *Instance) checkLongTermKey( + vlt *vault.Vault, ltIdentity *age.X25519Identity, +) error { + _, err := cli.existingVault(vlt.GetName()) + if err != nil { + return err + } + vaultDir, _ := vlt.GetDirectory() ltPubKey, err := afero.ReadFile(cli.fs, filepath.Join(vaultDir, "pub.age")) if err != nil { - release() - - return nil, fmt.Errorf("failed to read long-term public key: %w", err) + return fmt.Errorf("failed to read long-term public key: %w", err) } if string(ltPubKey) != ltIdentity.Recipient().String() { - release() - - return nil, fmt.Errorf("vault '%s' %w", vlt.GetName(), errVaultCreatedAgain) + return fmt.Errorf("vault '%s' %w", vlt.GetName(), errVaultCreatedAgain) } - return release, nil + return nil } // autoSelectUnlocker selects the newly created unlocker as current,