diff --git a/README.md b/README.md index c5d4c83..24dd19a 100644 --- a/README.md +++ b/README.md @@ -70,6 +70,24 @@ make build ## Commands Reference +### Confirmation Before Removal + +`secret rm`, `secret version rm`, `secret vault remove` and +`secret unlocker remove` destroy data that exists nowhere else. On a terminal +each one first asks `[y/N]`, naming exactly what it is about to remove, and +goes ahead only on `y` or `yes`; any other answer, a bare Enter included, +cancels and removes nothing. The question is asked only after the command's +checks have passed, and before it changes anything. + +Whether to ask is decided by stdin, where the answer is read from, so +`secret rm foo | tee log` still asks. When stdin is not a terminal, as in a +script or a CI job, nobody is there to answer: the command fails at once, +removes nothing, and says to pass `--force`. + +`--force` (`-f`) removes without asking, whatever the command removes: a vault +that holds secrets and the last unlocker of a vault included. Scripts that +remove things pass `--force`. + ### Initialization #### `secret init` @@ -100,13 +118,13 @@ Switches to the specified vault for subsequent operations. #### `secret vault remove [--force]` / `secret vault rm` ⚠️ 🛑 -**DANGER**: Permanently removes a vault and all its secrets. Like Unix `rm`, -this command does not ask for confirmation. +**DANGER**: Permanently removes a vault and all its secrets. It first asks +for confirmation, naming the vault and how many secrets it holds (see +[Confirmation Before Removal](#confirmation-before-removal)). The last vault +cannot be removed. Removing the current vault makes another vault the current +one. -Requires --force if the vault contains secrets. With --force, will -automatically switch to another vault if removing the current one. - -- `--force, -f`: Force removal even if vault contains secrets +- `--force, -f`: Remove without asking, also a vault that contains secrets - **NO RECOVERY**: All secrets in the vault will be permanently deleted ### Secret Management @@ -132,9 +150,12 @@ Retrieves and outputs a secret value to stdout. Lists all secrets in the current vault. Optional filter for substring matching. -#### `secret remove ` / `secret rm` ⚠️ 🛑 +#### `secret remove [--force]` / `secret rm` ⚠️ 🛑 -**DANGER**: Permanently removes a secret and ALL its versions. Like Unix `rm`, this command does not ask for confirmation. +**DANGER**: Permanently removes a secret and ALL its versions. It first asks +for confirmation, naming the secret, its vault and how many versions it has +(see [Confirmation Before Removal](#confirmation-before-removal)). +- `--force, -f`: Remove without asking - **NO RECOVERY**: Once removed, the secret cannot be recovered - **ALL VERSIONS DELETED**: Every version of the secret will be permanently deleted @@ -158,10 +179,12 @@ Lists all versions of a secret showing creation time, status, and validity perio Promotes a specific version to current by updating the symlink. Does not modify any timestamps, allowing for rollback scenarios. -#### `secret version remove ` / `secret version rm` ⚠️ 🛑 +#### `secret version remove [--force]` / `secret version rm` ⚠️ 🛑 -**DANGER**: Permanently removes a specific version of a secret. Like Unix -`rm`, this command does not ask for confirmation. +**DANGER**: Permanently removes a specific version of a secret. It first asks +for confirmation, naming the version, the secret and its vault (see +[Confirmation Before Removal](#confirmation-before-removal)). +- `--force, -f`: Remove without asking - **NO RECOVERY**: Once removed, this version cannot be recovered - Cannot remove the current version (must promote another version first) @@ -202,12 +225,15 @@ has, which is removed only once the new one is the current unlocker. #### `secret unlocker remove [--force]` / `secret unlocker rm` ⚠️ 🛑 -**DANGER**: Permanently removes an unlocker. Like Unix `rm`, this command -does not ask for confirmation. Cannot remove the last unlocker if the vault -has secrets unless --force is used. An unlocker directory that -`secret unlocker list` skips with a warning, because its metadata cannot be -read or parsed, is removed by the directory name the warning gives. -- `--force, -f`: Force removal of last unlocker even if vault has secrets +**DANGER**: Permanently removes an unlocker. It first asks for confirmation, +naming the unlocker and its vault and saying whether it is the vault's last +unlocker; for the last one it says how many secrets the vault holds and warns +that the vault then opens only with its mnemonic (see +[Confirmation Before Removal](#confirmation-before-removal)). An unlocker +directory that `secret unlocker list` skips with a warning, because its +metadata cannot be read or parsed, is removed by the directory name the +warning gives. +- `--force, -f`: Remove without asking, even the last unlocker - **CRITICAL WARNING**: Without unlockers and without your mnemonic phrase, vault data will be PERMANENTLY INACCESSIBLE - **NO RECOVERY**: Removing all unlockers without having your mnemonic means @@ -377,7 +403,7 @@ secret list secret get database/prod/password secret get services/api/key -# Remove a secret ⚠️ 🛑 (NO CONFIRMATION - PERMANENT!) +# Remove a secret ⚠️ 🛑 (asks first - PERMANENT!) secret remove ssh/servers/web01 ``` @@ -400,7 +426,7 @@ echo "personal-email-pass" | secret add email/password # List all vaults secret vault list -# Remove a vault ⚠️ 🛑 (NO CONFIRMATION - PERMANENT!) +# Remove a vault ⚠️ 🛑 (--force: NO CONFIRMATION - PERMANENT!) secret vault remove personal --force ``` @@ -418,7 +444,7 @@ secret unlocker list # Select a specific unlocker secret unlocker select -# Remove an unlocker ⚠️ 🛑 (NO CONFIRMATION!) +# Remove an unlocker ⚠️ 🛑 (asks first!) secret unlocker remove ``` @@ -431,7 +457,7 @@ secret version list database/prod/password # Promote an older version to current secret version promote database/prod/password 20231215.001 -# Remove an old version ⚠️ 🛑 (NO CONFIRMATION - PERMANENT!) +# Remove an old version ⚠️ 🛑 (asks first - PERMANENT!) secret version remove database/prod/password 20231214.001 ``` diff --git a/TODO.md b/TODO.md index 40bc2fb..401fbca 100644 --- a/TODO.md +++ b/TODO.md @@ -25,6 +25,20 @@ Bring the repo into policy compliance in one commit: # Completed Steps +- 2026-10-04: `secret rm`, `secret version rm`, `secret vault remove` and + `secret unlocker remove` ask `[y/N]` before removing anything + (https://git.eeqj.de/sneak/secret/issues/39), naming what they remove: the + secret, its vault and its version count; the version, secret and vault; the + vault and its secret count; the unlocker, its vault and whether it is the + last, and for the last the vault's secret count and that the vault then + opens only with its mnemonic. Only `y` or `yes` goes ahead. Without + `--force`, a command whose stdin is not a terminal fails at once. `--force` + (now also on `rm` and `version rm`) removes without asking; it replaces the + old refusals to remove a vault with secrets or the last unlocker of one + without `--force`, which the question now covers. The checks run, and the + question is asked, before the state directory lock is taken; under the + lock the checks run again, and if they would ask a different question, + nothing is removed. `secret rm` fails when it cannot count the versions. - 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 @@ -304,8 +318,6 @@ Bring the repo into policy compliance in one commit: - High priority: - Secure temporary file handling and cleanup. - Initialize a default unlock key at vault creation. - - Confirmation prompts for destructive operations (keys rm, vault - deletion). - Add secret rm and vault deletion commands. - Medium priority: - Standardize error messages; stop leaking internals. diff --git a/go.mod b/go.mod index 594d650..7452231 100644 --- a/go.mod +++ b/go.mod @@ -9,6 +9,7 @@ require ( github.com/btcsuite/btcd/btcec/v2 v2.1.3 github.com/btcsuite/btcd/btcutil v1.1.6 github.com/btcsuite/btcutil v0.0.0-20190425235716-9e5f4b9a998d + github.com/creack/pty v1.1.24 github.com/keybase/go-keychain v0.0.0-20230307172405-3e4884637dd1 github.com/oklog/ulid/v2 v2.1.1 github.com/spf13/afero v1.14.0 diff --git a/go.sum b/go.sum index 66b0089..4f68997 100644 --- a/go.sum +++ b/go.sum @@ -35,6 +35,8 @@ github.com/btcsuite/snappy-go v1.0.0/go.mod h1:8woku9dyThutzjeg+3xrA5iCpBRH8XEEg github.com/btcsuite/websocket v0.0.0-20150119174127-31079b680792/go.mod h1:ghJtEyQwv5/p4Mg4C0fgbePVuGr935/5ddU9Z3TmDRY= github.com/btcsuite/winsvc v1.0.0/go.mod h1:jsenWakMcC0zFBFurPLEAyrnc/teJEM1O46fmI40EZs= github.com/cpuguy83/go-md2man/v2 v2.0.6/go.mod h1:oOW0eioCTA6cOiMLiUPZOpcVxMig6NIQQ7OS05n1F4g= +github.com/creack/pty v1.1.24 h1:bJrF4RRfyJnbTJqzRLHzcGaZK1NeM5kTC9jGgovnR1s= +github.com/creack/pty v1.1.24/go.mod h1:08sCNb52WyoAwi2QDyzUCTgcvVFhUzewun7wtTfvcwE= github.com/davecgh/go-spew v0.0.0-20171005155431-ecdeabc65495/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= diff --git a/internal/cli/cli.go b/internal/cli/cli.go index 890c7f5..4c653df 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -3,6 +3,7 @@ package cli import ( "fmt" + "io" "os" "git.eeqj.de/sneak/secret/internal/secret" @@ -21,6 +22,10 @@ type Instance struct { // none. Mnemonic *memguard.LockedBuffer UnlockPassphrase *memguard.LockedBuffer + // terminal, when set, stands in for the terminal that confirm reads + // the user's answer from; only tests set it. When it is nil, confirm + // reads stdin, and only when stdin is a terminal. + terminal io.Reader } // NewCLIInstance creates a new CLI instance with the real filesystem diff --git a/internal/cli/confirm.go b/internal/cli/confirm.go new file mode 100644 index 0000000..3ccd2b8 --- /dev/null +++ b/internal/cli/confirm.go @@ -0,0 +1,108 @@ +package cli + +import ( + "bufio" + "errors" + "fmt" + "io" + "os" + "strings" + + "git.eeqj.de/sneak/secret/internal/vault" + "github.com/spf13/cobra" + "golang.org/x/term" +) + +// Sentinel errors for asking the user to confirm a removal +var ( + errNoTerminal = errors.New("stdin is not a terminal, so there is " + + "nobody to ask for confirmation; pass --force to remove without asking") + errNotConfirmed = errors.New("cancelled; nothing was removed") + errChangedWhileAsking = errors.New("what was to be removed changed " + + "while waiting for the answer; nothing was removed") +) + +// askThenLock asks the user to confirm a removal, unless force is set, and +// then takes the state directory lock and returns the function that +// releases it. find makes the command's checks, keeps what it found for +// the caller to remove, and returns the question that names it. find runs +// before the question, which is asked without the lock so that no other +// command waits while the user answers, and runs again once the lock is +// taken. That run is the last, so the caller removes what find found under +// the lock. If its question then differs from the one the user answered, +// something changed in between, and askThenLock fails. +func (cli *Instance) askThenLock( + cmd *cobra.Command, force bool, find func() (string, error), +) (func(), error) { + asked := "" + + if !force { + question, err := find() + if err != nil { + return nil, err + } + + err = cli.confirm(cmd, question) + if err != nil { + return nil, err + } + + asked = question + } + + release, err := vault.LockStateDir(cli.fs, cli.stateDir) + if err != nil { + return nil, err + } + + question, err := find() + if err == nil && !force && question != asked { + err = errChangedWhileAsking + } + + if err != nil { + release() + + return nil, err + } + + return release, nil +} + +// confirm asks question and returns nil only when the user answers y or +// yes; any other answer, a bare Enter included, cancels. When stdin is not +// a terminal it asks nothing and fails at once: nobody is there to answer, +// and waiting for an answer would hang a script. Stdin decides, not +// stdout, because the answer is read from stdin: `secret rm foo | tee log` +// still asks. The question goes to stderr. +func (cli *Instance) confirm(cmd *cobra.Command, question string) error { + answers := cli.terminal + if answers == nil { + answers = cmd.InOrStdin() + + if !isTerminal(answers) { + return errNoTerminal + } + } + + _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "%s [y/N] ", question) + + answer, err := bufio.NewReader(answers).ReadString('\n') + if err != nil && !errors.Is(err, io.EOF) { + return fmt.Errorf("failed to read the answer: %w", err) + } + + switch strings.ToLower(strings.TrimSpace(answer)) { + case "y", "yes": + return nil + default: + return errNotConfirmed + } +} + +// isTerminal reports whether r is a terminal. +func isTerminal(r io.Reader) bool { + file, ok := r.(*os.File) + + return ok && term.IsTerminal(int(file.Fd())) +} diff --git a/internal/cli/confirm_test.go b/internal/cli/confirm_test.go new file mode 100644 index 0000000..8569e70 --- /dev/null +++ b/internal/cli/confirm_test.go @@ -0,0 +1,410 @@ +// Confirmation Tests +// +// `secret rm`, `secret version rm`, `secret vault remove` and +// `secret unlocker remove` ask the user to confirm on a terminal, naming +// what they are about to remove, and remove it only on y or yes. --force +// skips the question. Without --force, a command whose stdin is not a +// terminal fails at once, since nobody is there to answer. +// +// The tests answer through Instance.terminal, which stands in for a +// terminal. Without it, whether stdin is a terminal decides; the tests in +// integration_test.go that run `secret rm` on a pseudo-terminal cover that. + +//nolint:testpackage // sets the unexported terminal field of Instance +package cli + +import ( + "bufio" + "bytes" + "fmt" + "io" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "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/assert" + "github.com/stretchr/testify/require" +) + +const ( + // confirmTestSecret is the secret the tests remove, or remove a + // version of, in the vault "work". + confirmTestSecret = "test/secret" + + // lastUnlockerRemoval names the case that removes the only unlocker. + lastUnlockerRemoval = "unlocker rm, the last one" +) + +// removal is one removal command, set up on its own state directory. +type removal struct { + fs afero.Fs + run func(cli *Instance, cmd *cobra.Command, force bool) error + // removed is the directory the command removes. + removed string + // question is the question the command asks. + question string +} + +// newConfirmTestVaults returns an in-memory state directory with the +// vaults "other" and "work", the current one. "work" holds two versions of +// confirmTestSecret and the given number of PGP unlockers. It returns the +// directory of "work" and the older version. +func newConfirmTestVaults( + t *testing.T, unlockers int, +) (*afero.MemMapFs, string, string) { + t.Helper() + + fs := &afero.MemMapFs{} + mnemonic := testMnemonicBuffer(t) + + _, err := vault.CreateVault(fs, testStateDir, "other", mnemonic) + require.NoError(t, err) + + vlt, err := vault.CreateVault(fs, testStateDir, "work", mnemonic) + require.NoError(t, err) + + addTestSecret(t, vlt, []byte("older"), false) + addTestSecret(t, vlt, []byte("newer"), true) + + vaultDir, err := vlt.GetDirectory() + require.NoError(t, err) + + versions, err := secret.ListVersions(fs, + filepath.Join(vaultDir, "secrets.d", "test%secret")) + require.NoError(t, err) + require.Len(t, versions, 2) + + for i := range unlockers { + writePGPUnlocker(t, fs, filepath.Join(vaultDir, "unlockers.d"), + fmt.Sprintf("pgp-%d", i), + time.Date(2026, time.October, 4, 12, i, 0, 0, time.UTC), + listTestGPGKeyID+string(rune('A'+i))) + } + + // ListVersions lists the newest version first. + return fs, vaultDir, versions[1] +} + +// newRemoval sets up the removal the command names. +func newRemoval(t *testing.T, command string) removal { + t.Helper() + + unlockers := 2 + if command == lastUnlockerRemoval { + unlockers = 1 + } + + fs, workDir, older := newConfirmTestVaults(t, unlockers) + unlockerID := "pgp-" + listTestGPGKeyID + "A" + + removeFirstUnlocker := func(cli *Instance, cmd *cobra.Command, force bool) error { + return cli.UnlockersRemove(unlockerID, force, cmd) + } + + switch command { + case "rm": + return removal{ + fs: fs, + run: func(cli *Instance, cmd *cobra.Command, force bool) error { + return cli.RemoveSecret(cmd, confirmTestSecret, force) + }, + removed: filepath.Join(workDir, "secrets.d", "test%secret"), + question: "Permanently remove secret 'test/secret' and its 2 " + + "version(s) from vault 'work'?", + } + case "version rm": + return removal{ + fs: fs, + run: func(cli *Instance, cmd *cobra.Command, force bool) error { + return cli.RemoveVersion(cmd, confirmTestSecret, older, force) + }, + removed: filepath.Join( + workDir, "secrets.d", "test%secret", "versions", older), + question: "Permanently remove version " + older + + " of secret 'test/secret' from vault 'work'?", + } + case "vault rm": + return removal{ + fs: fs, + run: func(cli *Instance, cmd *cobra.Command, force bool) error { + return cli.RemoveVault(cmd, "work", force) + }, + removed: workDir, + question: "Permanently remove vault 'work' and its 1 secret(s)?", + } + case "unlocker rm": + return removal{ + fs: fs, + run: removeFirstUnlocker, + removed: filepath.Join(workDir, "unlockers.d", "pgp-0"), + question: "Permanently remove unlocker '" + unlockerID + + "' from vault 'work'? It is not the vault's last unlocker.", + } + case lastUnlockerRemoval: + return removal{ + fs: fs, + run: removeFirstUnlocker, + removed: filepath.Join(workDir, "unlockers.d", "pgp-0"), + question: "Permanently remove unlocker '" + unlockerID + + "', the last unlocker of vault 'work', which holds 1 " + + "secret(s)? Without an unlocker the vault opens only " + + "with its mnemonic.", + } + } + + t.Fatalf("no removal %q", command) + + return removal{} +} + +// removalCommands lists the commands newRemoval sets up. +func removalCommands() []string { + return []string{ + "rm", "version rm", "vault rm", "unlocker rm", lastUnlockerRemoval, + } +} + +// newConfirmTestCommand returns a command whose output is discarded and +// whose stderr, where the question goes, is the returned buffer. +func newConfirmTestCommand() (*cobra.Command, *bytes.Buffer) { + var stderr bytes.Buffer + + cmd := &cobra.Command{} + cmd.SetOut(io.Discard) + cmd.SetErr(&stderr) + + return cmd, &stderr +} + +// requireExists asserts whether the directory dir exists. +func requireExists(t *testing.T, fs afero.Fs, dir string, want bool) { + t.Helper() + + exists, err := afero.DirExists(fs, dir) + require.NoError(t, err) + require.Equal(t, want, exists, dir) +} + +// TestConfirmAnswers checks which answers confirm accepts: y or yes, in +// any case, around which spaces do not matter. +func TestConfirmAnswers(t *testing.T) { + t.Parallel() + + for answer, want := range map[string]error{ + "y\n": nil, + "Y\n": nil, + "yes\n": nil, + " YES \n": nil, + "y": nil, + "\n": errNotConfirmed, + "": errNotConfirmed, + "n\n": errNotConfirmed, + "yy\n": errNotConfirmed, + "no\ny\n": errNotConfirmed, + } { + t.Run(fmt.Sprintf("%q", answer), func(t *testing.T) { + t.Parallel() + + cli := &Instance{terminal: strings.NewReader(answer)} + cmd, stderr := newConfirmTestCommand() + + err := cli.confirm(cmd, "Remove it?") + + require.ErrorIs(t, err, want) + assert.Equal(t, "Remove it? [y/N] ", stderr.String()) + }) + } +} + +// TestRemovalAnsweredYesRemoves checks that each removal asks its question +// and removes what it names when the user answers y. +func TestRemovalAnsweredYesRemoves(t *testing.T) { + t.Parallel() + + for _, command := range removalCommands() { + t.Run(command, func(t *testing.T) { + t.Parallel() + + r := newRemoval(t, command) + requireExists(t, r.fs, r.removed, true) + + cli := NewCLIInstanceWithStateDir(r.fs, testStateDir) + cli.terminal = strings.NewReader("y\n") + cmd, stderr := newConfirmTestCommand() + + require.NoError(t, r.run(cli, cmd, false)) + + assert.Equal(t, r.question+" [y/N] ", stderr.String()) + requireExists(t, r.fs, r.removed, false) + }) + } +} + +// TestRemovalDeclinedLeavesEverything checks that each removal changes +// nothing when the user answers anything but y or yes, a bare Enter +// included. +func TestRemovalDeclinedLeavesEverything(t *testing.T) { + t.Parallel() + + for _, command := range removalCommands() { + for _, answer := range []string{"\n", "n\n", ""} { + t.Run(fmt.Sprintf("%s %q", command, answer), func(t *testing.T) { + t.Parallel() + + r := newRemoval(t, command) + before := stateDirModTimes(t, r.fs) + + cli := NewCLIInstanceWithStateDir(r.fs, testStateDir) + cli.terminal = strings.NewReader(answer) + cmd, stderr := newConfirmTestCommand() + + err := r.run(cli, cmd, false) + + require.ErrorIs(t, err, errNotConfirmed) + assert.Equal(t, r.question+" [y/N] ", stderr.String()) + assert.Equal(t, before, stateDirModTimes(t, r.fs)) + }) + } + } +} + +// TestRemovalForcedAsksNothing checks that each removal with --force +// removes what it would have named without asking, and without reading +// its input, which is not a terminal. +func TestRemovalForcedAsksNothing(t *testing.T) { + t.Parallel() + + for _, command := range removalCommands() { + t.Run(command, func(t *testing.T) { + t.Parallel() + + r := newRemoval(t, command) + + input := strings.NewReader("n\n") + cli := NewCLIInstanceWithStateDir(r.fs, testStateDir) + cmd, stderr := newConfirmTestCommand() + cmd.SetIn(input) + + require.NoError(t, r.run(cli, cmd, true)) + + assert.Empty(t, stderr.String(), "asked with --force") + assert.Equal(t, 2, input.Len(), "read its input with --force") + requireExists(t, r.fs, r.removed, false) + }) + } +} + +// TestRemovalWithoutTerminalFailsAtOnce checks that each removal without +// --force, whose input is not a terminal, fails at once telling the user +// to pass --force, and changes nothing. The input is a pipe that nobody +// writes to or closes, so reading it would block for good. +func TestRemovalWithoutTerminalFailsAtOnce(t *testing.T) { + t.Parallel() + + for _, command := range removalCommands() { + t.Run(command, func(t *testing.T) { + t.Parallel() + + r := newRemoval(t, command) + before := stateDirModTimes(t, r.fs) + + input, inputWriter, err := os.Pipe() + require.NoError(t, err) + + t.Cleanup(func() { + _ = inputWriter.Close() + _ = input.Close() + }) + + cli := NewCLIInstanceWithStateDir(r.fs, testStateDir) + cmd, stderr := newConfirmTestCommand() + cmd.SetIn(input) + + done := make(chan error, 1) + + go func() { done <- r.run(cli, cmd, false) }() + + select { + case err := <-done: + require.ErrorIs(t, err, errNoTerminal) + assert.Contains(t, err.Error(), "pass --force") + case <-time.After(lockWait): + // Closing the pipe ends the read, and frees the lock if + // the command holds it. + _ = inputWriter.Close() + + t.Fatal("waited for an answer on input that is not a terminal") + } + + assert.Empty(t, stderr.String(), "asked without a terminal") + assert.Equal(t, before, stateDirModTimes(t, r.fs)) + }) + } +} + +// TestRemovalAsksWithoutHoldingLock checks that while `secret rm` waits +// for its answer, another command can take the state directory lock and +// change the secret, and that the removal then removes nothing, since the +// secret is no longer what the question named. +func TestRemovalAsksWithoutHoldingLock(t *testing.T) { + t.Parallel() + + r := newRemoval(t, "rm") + + answers, answerWriter := io.Pipe() + questions, questionWriter := io.Pipe() + + // Closing the answers ends the read if the test fails while the + // command waits for one. + t.Cleanup(func() { _ = answerWriter.Close() }) + + rm := NewCLIInstanceWithStateDir(r.fs, testStateDir) + rm.terminal = answers + cmd := &cobra.Command{} + cmd.SetOut(io.Discard) + cmd.SetErr(questionWriter) + + done := make(chan error, 1) + + go func() { done <- r.run(rm, cmd, false) }() + + question, err := bufio.NewReader(questions).ReadString(']') + require.NoError(t, err) + require.Equal(t, r.question+" [y/N]", question) + + // Adds a third version while rm waits for its answer. + add := NewCLIInstanceWithStateDir(r.fs, testStateDir) + add.Mnemonic = testMnemonicBuffer(t) + add.cmd = &cobra.Command{} + add.cmd.SetIn(strings.NewReader("newest")) + add.cmd.SetOut(io.Discard) + + added := make(chan error, 1) + + go func() { added <- add.AddSecret(confirmTestSecret, true) }() + + select { + case err := <-added: + require.NoError(t, err) + case <-time.After(lockWait): + t.Fatal("secret add waited for the lock while secret rm asked") + } + + _, err = answerWriter.Write([]byte("y\n")) + require.NoError(t, err) + + select { + case err := <-done: + require.ErrorIs(t, err, errChangedWhileAsking) + case <-time.After(lockWait): + t.Fatal("secret rm did not finish once answered") + } + + requireExists(t, r.fs, r.removed, true) +} diff --git a/internal/cli/integration_test.go b/internal/cli/integration_test.go index c38fd76..a77ae0e 100644 --- a/internal/cli/integration_test.go +++ b/internal/cli/integration_test.go @@ -2,10 +2,13 @@ package cli_test import ( + "bufio" + "bytes" "context" "encoding/json" "errors" "fmt" + "io" "os" "os/exec" "path/filepath" @@ -16,7 +19,11 @@ import ( "git.eeqj.de/sneak/secret/internal/cli" "git.eeqj.de/sneak/secret/internal/secret" + "git.eeqj.de/sneak/secret/internal/vault" "git.eeqj.de/sneak/secret/pkg/agehd" + "github.com/awnumar/memguard" + "github.com/creack/pty" + "github.com/spf13/afero" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -2543,3 +2550,145 @@ func copyFile(src, dst string) error { return nil } + +// secretRmCommand makes a state directory whose vault "default" holds the +// secret "x", and returns `secret rm x` on the built binary against it, and +// the directory of "x". The vault has no unlocker, so making it derives no +// key from a passphrase. +func secretRmCommand(ctx context.Context, t *testing.T) (*exec.Cmd, string) { + t.Helper() + + stateDir := t.TempDir() + + mnemonic := memguard.NewBufferFromBytes([]byte(testMnemonic)) + defer mnemonic.Destroy() + + vlt, err := vault.CreateVault(afero.NewOsFs(), stateDir, "default", mnemonic) + require.NoError(t, err) + + value := memguard.NewBufferFromBytes([]byte("value")) + defer value.Destroy() + + require.NoError(t, vlt.AddSecret("x", value, false)) + + //nolint:gosec // G204: test executes the freshly built secret binary + cmd := exec.CommandContext(ctx, secretBinaryPath(t), "rm", "x") + cmd.Env = []string{ + secret.EnvStateDir + "=" + stateDir, + "PATH=" + os.Getenv("PATH"), + "HOME=" + os.Getenv("HOME"), + } + + return cmd, filepath.Join(stateDir, "vaults.d", "default", "secrets.d", "x") +} + +// TestRemoveWithoutTerminalFailsAtOnce runs `secret rm` without --force, +// with a stdin that is not a terminal and never delivers anything, as in a +// script or a CI job. It must fail at once, telling the user to pass +// --force, instead of waiting for an answer, and remove nothing. +func TestRemoveWithoutTerminalFailsAtOnce(t *testing.T) { + t.Parallel() + + // Nobody writes to or closes the pipe, so reading it would block for good. + stdin, stdinWriter, err := os.Pipe() + require.NoError(t, err) + + defer func() { + _ = stdinWriter.Close() + _ = stdin.Close() + }() + + ctx, cancel := context.WithTimeout(t.Context(), time.Minute) + defer cancel() + + cmd, secretDir := secretRmCommand(ctx, t) + cmd.Stdin = stdin + + output, err := cmd.CombinedOutput() + + require.NoError(t, ctx.Err(), "secret rm waited for an answer") + require.Error(t, err) + assert.Contains(t, string(output), "pass --force") + assert.DirExists(t, secretDir) +} + +// The next two tests run `secret rm` with a terminal on stdin or on stdout +// and stderr, not both: whether it asks must depend on stdin alone, where +// the answer is read from. pty.Open returns the two ends of a new terminal: +// tty is the end a program uses as its terminal, and ptmx the end the test +// reads what the terminal shows from and types into. + +// TestRemoveIgnoresTerminalOnStdout runs `echo y | secret rm x` at a +// terminal. stdin is a pipe, so nobody can answer there, and the command +// must fail as in a script, removing nothing. +func TestRemoveIgnoresTerminalOnStdout(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithTimeout(t.Context(), time.Minute) + defer cancel() + + cmd, secretDir := secretRmCommand(ctx, t) + + ptmx, tty, err := pty.Open() + require.NoError(t, err) + + defer func() { _ = ptmx.Close() }() + + cmd.Stdin = strings.NewReader("y\n") + cmd.Stdout = tty + cmd.Stderr = tty + + require.NoError(t, cmd.Start()) + + _ = tty.Close() + + // The read ends once secret rm has exited and so closed the terminal. + shown, _ := io.ReadAll(ptmx) + + require.Error(t, cmd.Wait()) + assert.Contains(t, string(shown), "pass --force") + assert.DirExists(t, secretDir) +} + +// TestRemoveAsksAtTerminalOnStdin runs `secret rm x | cat` at a terminal. +// It must ask on the terminal, and remove the secret when y is typed there. +func TestRemoveAsksAtTerminalOnStdin(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithTimeout(t.Context(), time.Minute) + defer cancel() + + cmd, secretDir := secretRmCommand(ctx, t) + + ptmx, tty, err := pty.Open() + require.NoError(t, err) + + defer func() { _ = ptmx.Close() }() + + cmd.Stdin = tty + // Not a file, so exec.Cmd connects stdout through a pipe. + cmd.Stdout = io.Discard + cmd.Stderr = tty + + require.NoError(t, cmd.Start()) + + _ = tty.Close() + + var ( + shown []byte + char byte + ) + + terminal := bufio.NewReader(ptmx) + for !bytes.HasSuffix(shown, []byte("[y/N] ")) { + char, err = terminal.ReadByte() + require.NoError(t, err, "secret rm ended without asking: %s", shown) + + shown = append(shown, char) + } + + _, err = ptmx.WriteString("y\n") + require.NoError(t, err) + require.NoError(t, cmd.Wait()) + assert.NoDirExists(t, secretDir) +} diff --git a/internal/cli/lock_test.go b/internal/cli/lock_test.go index f3bd0a2..ccf77a5 100644 --- a/internal/cli/lock_test.go +++ b/internal/cli/lock_test.go @@ -241,8 +241,10 @@ func TestFailedCommandReleasesLock(t *testing.T) { fs := afero.NewMemMapFs() cli := NewCLIInstanceWithStateDir(fs, testStateDir) - // Fails once it holds the lock: there is no current vault - err := cli.RemoveSecret(&cobra.Command{}, "missing", false) + // Fails once it holds the lock: there is no current vault. Without + // --force it would fail before taking the lock, on the check it makes + // before asking. + err := cli.RemoveSecret(&cobra.Command{}, "missing", true) require.Error(t, err) select { @@ -431,8 +433,8 @@ func TestChangingCommandsWaitForLock(t *testing.T) { {"encrypt", false, func(cli *Instance, _, _ string) error { return cli.Encrypt("key", testInput, "") }}, - {"rm", false, func(cli *Instance, _, _ string) error { - return cli.RemoveSecret(cli.cmd, "test/secret", false) + {"rm --force", false, func(cli *Instance, _, _ string) error { + return cli.RemoveSecret(cli.cmd, "test/secret", true) }}, {"move", false, func(cli *Instance, _, _ string) error { return cli.MoveSecret(cli.cmd, "test/secret", "moved", false) @@ -440,8 +442,8 @@ func TestChangingCommandsWaitForLock(t *testing.T) { {"version promote", false, func(cli *Instance, olderVersion, _ string) error { return cli.PromoteVersion(cli.cmd, "test/secret", olderVersion) }}, - {"version rm", false, func(cli *Instance, olderVersion, _ string) error { - return cli.RemoveVersion(cli.cmd, "test/secret", olderVersion) + {"version rm --force", false, func(cli *Instance, olderVersion, _ string) error { + return cli.RemoveVersion(cli.cmd, "test/secret", olderVersion, true) }}, {"vault create", false, func(cli *Instance, _, _ string) error { return cli.CreateVault(cli.cmd, "created") @@ -452,13 +454,13 @@ func TestChangingCommandsWaitForLock(t *testing.T) { {"vault import", false, func(cli *Instance, _, _ string) error { return cli.VaultImport(cli.cmd, "other") }}, - {"vault rm", false, func(cli *Instance, _, _ string) error { - return cli.RemoveVault(cli.cmd, "other", false) + {"vault rm --force", false, func(cli *Instance, _, _ string) error { + return cli.RemoveVault(cli.cmd, "other", true) }}, {"unlocker add", false, func(cli *Instance, _, _ string) error { return cli.UnlockersAdd("passphrase", cli.cmd) }}, - {"unlocker rm", true, func(cli *Instance, _, unlockerID string) error { + {"unlocker rm --force", true, func(cli *Instance, _, unlockerID string) error { return cli.UnlockersRemove(unlockerID, true, cli.cmd) }}, {"unlocker select", true, func(cli *Instance, _, unlockerID string) error { diff --git a/internal/cli/path_traversal_test.go b/internal/cli/path_traversal_test.go index eb1fb39..c8816fd 100644 --- a/internal/cli/path_traversal_test.go +++ b/internal/cli/path_traversal_test.go @@ -170,8 +170,8 @@ func requireRejectedAndUnchanged( // TestInvalidSecretNameLeavesVaultsUnchanged is a regression test for // https://git.eeqj.de/sneak/secret/issues/33, where `secret rm ..` deleted // the whole vault, and `secret rm .` or `secret rm ""` every secret in it. -// Moves and imports use --force, so that only the name check stands in -// the way. +// Removals, moves and imports use --force, so that only the name check +// stands in the way. // //nolint:paralleltest // the cases share cmd func TestInvalidSecretNameLeavesVaultsUnchanged(t *testing.T) { @@ -191,17 +191,17 @@ func TestInvalidSecretNameLeavesVaultsUnchanged(t *testing.T) { rejected string // the secret name the command must reject run func(c *cli.Instance) error }{ - {"rm ..", "..", func(c *cli.Instance) error { - return c.RemoveSecret(cmd, "..", false) + {"rm --force ..", "..", func(c *cli.Instance) error { + return c.RemoveSecret(cmd, "..", true) }}, - {"rm .", ".", func(c *cli.Instance) error { - return c.RemoveSecret(cmd, ".", false) + {"rm --force .", ".", func(c *cli.Instance) error { + return c.RemoveSecret(cmd, ".", true) }}, - {`rm ""`, "", func(c *cli.Instance) error { - return c.RemoveSecret(cmd, "", false) + {`rm --force ""`, "", func(c *cli.Instance) error { + return c.RemoveSecret(cmd, "", true) }}, - {"rm ../../etc", "../../etc", func(c *cli.Instance) error { - return c.RemoveSecret(cmd, "../../etc", false) + {"rm --force ../../etc", "../../etc", func(c *cli.Instance) error { + return c.RemoveSecret(cmd, "../../etc", true) }}, {"mv --force .. x", "..", func(c *cli.Instance) error { return c.MoveSecret(cmd, "..", "x", true) @@ -244,8 +244,8 @@ func TestInvalidSecretNameLeavesVaultsUnchanged(t *testing.T) { {"version promote ..", "..", func(c *cli.Instance) error { return c.PromoteVersion(cmd, "..", testVersion) }}, - {"version rm ..", "..", func(c *cli.Instance) error { - return c.RemoveVersion(cmd, "..", testVersion) + {"version rm --force ..", "..", func(c *cli.Instance) error { + return c.RemoveVersion(cmd, "..", testVersion, true) }}, {"encrypt ..", "..", func(c *cli.Instance) error { return c.Encrypt("..", "", "") @@ -279,8 +279,8 @@ func TestInvalidVersionLeavesVaultsUnchanged(t *testing.T) { command string run func(c *cli.Instance, version string) error }{ - {"version rm x", func(c *cli.Instance, version string) error { - return c.RemoveVersion(cmd, "x", version) + {"version rm --force x", func(c *cli.Instance, version string) error { + return c.RemoveVersion(cmd, "x", version, true) }}, {"version promote x", func(c *cli.Instance, version string) error { return c.PromoteVersion(cmd, "x", version) @@ -361,9 +361,9 @@ func TestInvalidVaultNameLeavesStateUnchanged(t *testing.T) { } } -// TestRemoveVersionRemovesOnlyThatVersion checks that `secret version rm` -// with a version that is not the current one removes that version and -// changes nothing else. +// TestRemoveVersionRemovesOnlyThatVersion checks that +// `secret version rm --force` with a version that is not the current one +// removes that version and changes nothing else. func TestRemoveVersionRemovesOnlyThatVersion(t *testing.T) { t.Parallel() @@ -389,7 +389,7 @@ func TestRemoveVersionRemovesOnlyThatVersion(t *testing.T) { require.Contains(t, before, oldDir) c := cli.NewCLIInstanceWithStateDir(fs, testStateDir) - err = c.RemoveVersion(&cobra.Command{}, "x", versions[1]) + err = c.RemoveVersion(&cobra.Command{}, "x", versions[1], true) require.NoError(t, err) // Expected: the state as before without everything under oldDir. diff --git a/internal/cli/secrets.go b/internal/cli/secrets.go index 492280d..a5fe1b1 100644 --- a/internal/cli/secrets.go +++ b/internal/cli/secrets.go @@ -205,19 +205,25 @@ func newRemoveCmd() *cobra.Command { Aliases: []string{"rm"}, Short: "Remove a secret from the vault", Long: `Remove a secret and all its versions from the current ` + - `vault. This action is permanent and cannot be undone.`, + `vault. This action is permanent and cannot be undone. ` + + `Asks for confirmation first; when stdin is not a terminal, ` + + `fails unless --force is given.`, Args: cobra.ExactArgs(1), ValidArgsFunction: getSecretNamesCompletionFunc(cli.fs, cli.stateDir), RunE: func(cmd *cobra.Command, args []string) error { + force, _ := cmd.Flags().GetBool("force") + cli, err := NewCLIInstance() if err != nil { return fmt.Errorf("failed to initialize CLI: %w", err) } - return cli.RemoveSecret(cmd, args[0], false) + return cli.RemoveSecret(cmd, args[0], force) }, } + cmd.Flags().BoolP("force", "f", false, "Remove without asking for confirmation") + return cmd } @@ -699,29 +705,64 @@ func (cli *Instance) ImportSecret( return nil } -// RemoveSecret removes a secret from the vault -func (cli *Instance) RemoveSecret(cmd *cobra.Command, secretName string, _ bool) error { +// RemoveSecret removes a secret and all its versions from the current +// vault, after asking the user to confirm unless force is set. +func (cli *Instance) RemoveSecret( + cmd *cobra.Command, secretName string, force bool, +) error { err := vault.ValidateSecretName(secretName) if err != nil { return err } - release, err := vault.LockStateDir(cli.fs, cli.stateDir) + var found secretToRemove + + release, err := cli.askThenLock(cmd, force, func() (string, error) { + var err error + + found, err = cli.findSecretToRemove(secretName) + + return found.question, err + }) if err != nil { return err } defer release() - // Get current vault - currentVlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir) + err = secret.RemoveDirAtomic(cli.fs, found.dir) if err != nil { - return err + return fmt.Errorf("failed to remove secret: %w", err) + } + + cmd.Printf("Removed secret '%s' (%d version(s) deleted)\n", + secretName, found.versions) + + return nil +} + +// secretToRemove is what removing a secret removes, as findSecretToRemove +// found it. +type secretToRemove struct { + // dir is the secret's directory, which holds all its versions. + dir string + versions int + // question names what is removed, for the user to confirm. + question string +} + +// findSecretToRemove checks that the secret exists in the current vault +// and counts its versions. +func (cli *Instance) findSecretToRemove( + secretName string, +) (secretToRemove, error) { + currentVlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir) + if err != nil { + return secretToRemove{}, err } - // Check if secret exists vaultDir, err := currentVlt.GetDirectory() if err != nil { - return err + return secretToRemove{}, err } encodedName := strings.ReplaceAll(secretName, "/", "%") @@ -729,32 +770,30 @@ func (cli *Instance) RemoveSecret(cmd *cobra.Command, secretName string, _ bool) exists, err := afero.DirExists(cli.fs, secretDir) if err != nil { - return fmt.Errorf("failed to check if secret exists: %w", err) + return secretToRemove{}, + fmt.Errorf("failed to check if secret exists: %w", err) } if !exists { - return fmt.Errorf("secret '%s' %w", secretName, errSecretNotFound) + return secretToRemove{}, + fmt.Errorf("secret '%s' %w", secretName, errSecretNotFound) } - // Count versions for information - versionsDir := filepath.Join(secretDir, "versions") - versionCount := 0 - - entries, err := afero.ReadDir(cli.fs, versionsDir) - if err == nil { - versionCount = len(entries) + // A secret without a versions directory has no versions, and can + // still be removed. + versions, err := afero.ReadDir(cli.fs, filepath.Join(secretDir, "versions")) + if err != nil && !errors.Is(err, os.ErrNotExist) { + return secretToRemove{}, fmt.Errorf( + "failed to count the versions of secret '%s': %w", secretName, err) } - // Remove the secret directory - err = secret.RemoveDirAtomic(cli.fs, secretDir) - if err != nil { - return fmt.Errorf("failed to remove secret: %w", err) - } - - cmd.Printf("Removed secret '%s' (%d version(s) deleted)\n", - secretName, versionCount) - - return nil + return secretToRemove{ + dir: secretDir, + versions: len(versions), + question: fmt.Sprintf("Permanently remove secret '%s' and its %d "+ + "version(s) from vault '%s'?", + secretName, len(versions), currentVlt.GetName()), + }, nil } // MoveSecret moves or renames a secret (within or across vaults), holding diff --git a/internal/cli/unlockers.go b/internal/cli/unlockers.go index 7857328..bcb417c 100644 --- a/internal/cli/unlockers.go +++ b/internal/cli/unlockers.go @@ -47,7 +47,6 @@ var ( errGPGKeyAlreadyUnlocker = errors.New( "is already added as an unlocker") errUnsupportedUnlockerType = errors.New("unsupported unlocker type") - errLastUnlocker = errors.New("refusing to remove last unlocker") ) // UnlockerInfo represents unlocker information for display @@ -267,10 +266,11 @@ func newUnlockerRemoveCmd() *cobra.Command { Use: "remove ", Aliases: []string{"rm"}, Short: "Remove an unlocker", - Long: `Remove an unlocker from the current vault. Cannot remove ` + - `the last unlocker if the vault has secrets unless --force is ` + - `used. Warning: Without unlockers and without your mnemonic, ` + - `vault data will be permanently inaccessible.`, + Long: `Remove an unlocker from the current vault. Asks for ` + + `confirmation first, saying whether it is the vault's last ` + + `unlocker; when stdin is not a terminal, fails unless --force ` + + `is given. Warning: Without unlockers and without your ` + + `mnemonic, vault data will be permanently inaccessible.`, Args: cobra.ExactArgs(1), ValidArgsFunction: getUnlockerIDsCompletionFunc(cli.fs, cli.stateDir), RunE: func(cmd *cobra.Command, args []string) error { @@ -286,7 +286,7 @@ func newUnlockerRemoveCmd() *cobra.Command { } cmd.Flags().BoolP("force", "f", false, - "Force removal of last unlocker even if vault has secrets") + "Remove without asking for confirmation, even the last unlocker") return cmd } @@ -726,55 +726,91 @@ func (cli *Instance) addPGPUnlocker(cmd *cobra.Command) error { return nil } -// UnlockersRemove removes an unlocker, holding the state directory lock -// while removeUnlocker runs +// UnlockersRemove removes an unlocker from the current vault, after asking +// the user to confirm unless force is set. func (cli *Instance) UnlockersRemove( unlockerID string, force bool, cmd *cobra.Command, ) error { - release, err := vault.LockStateDir(cli.fs, cli.stateDir) + var found unlockerToRemove + + release, err := cli.askThenLock(cmd, force, func() (string, error) { + var err error + + found, err = cli.findUnlockerToRemove(unlockerID) + + return found.question, err + }) if err != nil { return err } defer release() - return cli.removeUnlocker(unlockerID, force, cmd) + return cli.removeUnlocker(unlockerID, found, cmd) } -// removeUnlocker removes an unlocker with safety checks -func (cli *Instance) removeUnlocker( - unlockerID string, force bool, cmd *cobra.Command, -) error { - // Get current vault +// unlockerToRemove is what removing an unlocker removes, as +// findUnlockerToRemove found it. +type unlockerToRemove struct { + vlt *vault.Vault + // last is set when the unlocker counts as the vault's last one, and + // secrets is then the number of secrets in the vault. + last bool + secrets int + // question names what is removed, for the user to confirm. + question string +} + +// findUnlockerToRemove checks that the current vault has the unlocker and +// finds whether it is the vault's last one. +func (cli *Instance) findUnlockerToRemove( + unlockerID string, +) (unlockerToRemove, error) { vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir) if err != nil { - return err + return unlockerToRemove{}, err + } + + exists, err := vlt.HasUnlocker(unlockerID) + if err != nil { + return unlockerToRemove{}, err + } + + if !exists { + return unlockerToRemove{}, fmt.Errorf("unlocker with ID %s %w", + unlockerID, vault.ErrUnlockerNotFound) } // Get list of unlockers. It leaves out a directory whose metadata file // is missing or cannot be checked for, read or parsed. unlockers, err := vlt.ListUnlockers() if err != nil { - return fmt.Errorf("failed to list unlockers: %w", err) + return unlockerToRemove{}, + fmt.Errorf("failed to list unlockers: %w", err) } vaultDir, err := vlt.GetDirectory() if err != nil { - return fmt.Errorf("failed to get vault directory: %w", err) + return unlockerToRemove{}, + fmt.Errorf("failed to get vault directory: %w", err) } unlockersDir := filepath.Join(vaultDir, "unlockers.d") - // Check if we're removing the last unlocker - removingLast := false + found := unlockerToRemove{ + vlt: vlt, + question: fmt.Sprintf("Permanently remove unlocker '%s' from vault "+ + "'%s'? It is not the vault's last unlocker.", + unlockerID, vlt.GetName()), + } if len(unlockers) == 1 { lastID, err := findUnlockerIDByMetadata( cli.fs, unlockersDir, unlockers[0], true) if err != nil { - return err + return unlockerToRemove{}, err } - removingLast = lastID == unlockerID + found.last = lastID == unlockerID } // unlockerID may instead name a directory left out of the list. If its @@ -783,34 +819,36 @@ func (cli *Instance) removeUnlocker( // for or read, the unlocker may be the only working one, so removing it // counts as removing the last unlocker. if metadataUnreadable(cli.fs, filepath.Join(unlockersDir, unlockerID)) { - removingLast = true + found.last = true } - if removingLast { - // Check if vault has secrets - numSecrets, err := vlt.NumSecrets() + if found.last { + found.secrets, err = vlt.NumSecrets() if err != nil { - return fmt.Errorf("failed to count secrets: %w", err) + return unlockerToRemove{}, + fmt.Errorf("failed to count secrets: %w", err) } - if numSecrets > 0 && !force { - cmd.Println("ERROR: Cannot remove the last unlocker when the " + - "vault contains secrets.") - cmd.Println("WARNING: Without unlockers, you MUST have your " + - "mnemonic phrase to decrypt the vault.") - cmd.Println("If you want to proceed anyway, use --force") - - return errLastUnlocker - } - - if numSecrets > 0 && force { - cmd.Println("WARNING: Removing the last unlocker. You MUST " + - "have your mnemonic phrase to access this vault again!") - } + found.question = fmt.Sprintf("Permanently remove unlocker '%s', "+ + "the last unlocker of vault '%s', which holds %d secret(s)? "+ + "Without an unlocker the vault opens only with its mnemonic.", + unlockerID, vlt.GetName(), found.secrets) } - // Remove the unlocker - err = vlt.RemoveUnlocker(unlockerID) + return found, nil +} + +// removeUnlocker removes the unlocker that findUnlockerToRemove found. The +// caller holds the state directory lock. +func (cli *Instance) removeUnlocker( + unlockerID string, found unlockerToRemove, cmd *cobra.Command, +) error { + if found.last && found.secrets > 0 { + cmd.Println("WARNING: Removing the last unlocker. You MUST " + + "have your mnemonic phrase to access this vault again!") + } + + err := found.vlt.RemoveUnlocker(unlockerID) if err != nil { return err } diff --git a/internal/cli/unlockers_corrupt_test.go b/internal/cli/unlockers_corrupt_test.go index d7dd8c7..7047c9b 100644 --- a/internal/cli/unlockers_corrupt_test.go +++ b/internal/cli/unlockers_corrupt_test.go @@ -5,14 +5,15 @@ // 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. +// 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 import ( "path/filepath" + "strings" "testing" "git.eeqj.de/sneak/secret/internal/vault" @@ -56,37 +57,27 @@ func TestUnlockerSelectSkipsCorruptUnlocker(t *testing.T) { } // TestUnlockerRemoveWithCorruptUnlocker asserts that the second unlocker -// can be removed, unless the vault holds secrets: the corrupt unlocker -// cannot unlock the vault, so the second is its last. The corrupt one can -// be removed by its directory name without --force even then. +// counts as the vault's last one, since the corrupt unlocker cannot unlock +// the vault, and that the corrupt one, removed by its directory name, does +// not. Either is removed once the user confirms. func TestUnlockerRemoveWithCorruptUnlocker(t *testing.T) { t.Parallel() tests := []struct { name string unlockerID string - withSecret bool - wantErr error + wantLast bool wantEntries []string }{ { name: "the other unlocker", unlockerID: "pgp-" + listTestGPGKeyID + "B", + wantLast: true, wantEntries: []string{listTestUnlockerDirOne}, }, - { - name: "the other unlocker, the last one, with secrets", - unlockerID: "pgp-" + listTestGPGKeyID + "B", - withSecret: true, - wantErr: errLastUnlocker, - wantEntries: []string{ - listTestUnlockerDirOne, listTestUnlockerDirTwo, - }, - }, { name: "the corrupt unlocker by its directory name", unlockerID: listTestUnlockerDirOne, - withSecret: true, wantEntries: []string{listTestUnlockerDirTwo}, }, } @@ -96,14 +87,16 @@ func TestUnlockerRemoveWithCorruptUnlocker(t *testing.T) { t.Parallel() fs := newCorruptUnlockerVault(t) - if tt.withSecret { - writeTestSecret(t, fs, testVaultDir(listTestVaultName)) - } + writeTestSecret(t, fs, testVaultDir(listTestVaultName)) instance, cmd := newTestInstance(fs) - err := instance.UnlockersRemove(tt.unlockerID, false, cmd) - require.ErrorIs(t, err, tt.wantErr) + found, err := instance.findUnlockerToRemove(tt.unlockerID) + require.NoError(t, err) + assert.Equal(t, tt.wantLast, found.last) + + instance.terminal = strings.NewReader("y\n") + require.NoError(t, instance.UnlockersRemove(tt.unlockerID, false, cmd)) assertDirEntries(t, fs, filepath.Join(testVaultDir(listTestVaultName), @@ -113,13 +106,14 @@ func TestUnlockerRemoveWithCorruptUnlocker(t *testing.T) { } } -// 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: -// listing leaves it out, but it may still be the vault's only working -// unlocker. With --force it is removed. The state directory lock refuses -// the failing filesystem, so the test calls removeUnlocker, which -// UnlockersRemove runs once it holds the lock. +// 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, +// so the question warns that it is: listing leaves it out, but it may +// still be the vault's only working unlocker. It is then removed. The +// state directory lock refuses the failing filesystem, so the test calls +// findUnlockerToRemove and removeUnlocker, which UnlockersRemove runs to +// make its checks and, once it holds the lock, to remove the unlocker. func TestUnlockerRemoveWithUnreadableMetadata(t *testing.T) { t.Parallel() @@ -155,12 +149,13 @@ func TestUnlockerRemoveWithUnreadableMetadata(t *testing.T) { instance, cmd := newTestInstance(tt.wrap(base)) - err := instance.removeUnlocker(listTestUnlockerDirOne, false, cmd) - require.ErrorIs(t, err, errLastUnlocker) - assertDirEntries(t, base, unlockersDir, listTestUnlockerDirOne) + found, err := instance.findUnlockerToRemove(listTestUnlockerDirOne) + require.NoError(t, err) + assert.True(t, found.last) + assert.Contains(t, found.question, "the last unlocker") require.NoError(t, - instance.removeUnlocker(listTestUnlockerDirOne, true, cmd)) + instance.removeUnlocker(listTestUnlockerDirOne, found, cmd)) assertDirEntries(t, base, unlockersDir) }) } diff --git a/internal/cli/unreadable_dir_test.go b/internal/cli/unreadable_dir_test.go index eda4486..fb8be6f 100644 --- a/internal/cli/unreadable_dir_test.go +++ b/internal/cli/unreadable_dir_test.go @@ -2,15 +2,18 @@ // // The checks that guard adding a PGP unlocker (is this key already an // unlocker?), removing the last unlocker and removing a vault (does the -// vault hold secrets?), and importing a mnemonic (does the vault already -// have a long-term key?) each look at the vault on disk before acting. +// vault hold secrets?), removing a secret (how many versions does it +// have?), and importing a mnemonic (does the vault already have a +// long-term key?) each look at the vault on disk before acting. // When that look fails they must refuse to act, not read the failure as // "nothing there" and go ahead. // // The tests make the look fail with a wrapper around the in-memory // filesystem, which the state directory lock refuses. So they call the -// function each command runs once it holds the lock, such as removeVault -// for RemoveVault. +// function each command runs once it holds the lock, such as addPGPUnlocker +// for UnlockersAdd, or, for a removal, the function that makes its checks, +// such as findVaultToRemove for RemoveVault, which runs again under the +// lock before anything is removed, with --force or without. //nolint:testpackage // white-box test of unexported internals package cli @@ -285,10 +288,9 @@ func TestRemoveLastUnlockerAbortsWhenSecretsUnreadable(t *testing.T) { base := newListTestVault(t, 1) writeTestSecret(t, base, vaultDir) - instance, cmd := newTestInstance(&statFailFs{Fs: base, path: path}) + instance, _ := newTestInstance(&statFailFs{Fs: base, path: path}) - err := instance.removeUnlocker( - "pgp-"+listTestGPGKeyID+"A", false, cmd) + _, err := instance.findUnlockerToRemove("pgp-" + listTestGPGKeyID + "A") require.ErrorIs(t, err, errStatFailed) assertDirEntries(t, base, unlockersDir, listTestUnlockerDirOne) @@ -332,9 +334,9 @@ func TestRemoveVaultAbortsWhenSecretsDirUnreadable(t *testing.T) { base := newListTestVault(t, 1) writeTestSecret(t, base, vaultDir) - instance, cmd := newTestInstance(tt.failFs(base)) + instance, _ := newTestInstance(tt.failFs(base)) - err := instance.removeVault(cmd, unreadableTestOtherVault, false) + _, err := instance.findVaultToRemove(unreadableTestOtherVault) require.ErrorIs(t, err, tt.wantErr) @@ -345,6 +347,31 @@ func TestRemoveVaultAbortsWhenSecretsDirUnreadable(t *testing.T) { } } +// TestRemoveSecretAbortsWhenVersionsUnreadable asserts that a secret is +// kept when its versions directory exists but cannot be listed, so that +// the question cannot say how many versions would be removed. +func TestRemoveSecretAbortsWhenVersionsUnreadable(t *testing.T) { + t.Parallel() + + secretDir := filepath.Join(testVaultDir(listTestVaultName), + unreadableTestSecretsDirName, unreadableTestSecretName) + versionsDir := filepath.Join(secretDir, "versions") + + base := newListTestVault(t, 1) + writeTestSecret(t, base, testVaultDir(listTestVaultName)) + require.NoError(t, base.MkdirAll(versionsDir, listTestDirPerm)) + + instance, _ := newTestInstance(&openFailFs{Fs: base, path: versionsDir}) + + _, err := instance.findSecretToRemove(unreadableTestSecretName) + + require.ErrorIs(t, err, errOpenFailed) + + exists, err := afero.DirExists(base, secretDir) + require.NoError(t, err) + assert.True(t, exists, "the secret must not be removed") +} + // TestVaultImportAbortsWhenPubKeyUnreadable asserts that a mnemonic import // stops when whether the vault already has a long-term key cannot be // determined. diff --git a/internal/cli/vault.go b/internal/cli/vault.go index e425e5d..4cd699c 100644 --- a/internal/cli/vault.go +++ b/internal/cli/vault.go @@ -31,8 +31,6 @@ var ( errPassphraseEnvNotSet = errors.New( "SB_UNLOCK_PASSPHRASE environment variable not set") errCannotRemoveLastVault = errors.New("cannot remove the last vault") - errVaultContainsSecrets = errors.New( - "contains secrets; use --force to remove") ) func newVaultCmd() *cobra.Command { @@ -156,9 +154,12 @@ func newVaultRemoveCmd() *cobra.Command { Use: "remove ", Aliases: []string{"rm"}, Short: "Remove a vault", - Long: `Remove a vault. Requires --force if the vault contains ` + - `secrets. Will automatically switch to another vault if ` + - `removing the currently selected one.`, + Long: `Remove a vault and all its secrets. Asks for ` + + `confirmation first, naming how many secrets the vault ` + + `holds; when stdin is not a terminal, fails unless --force ` + + `is given. Will automatically switch to another vault if ` + + `removing the currently selected one. The last vault ` + + `cannot be removed.`, Args: cobra.ExactArgs(1), ValidArgsFunction: getVaultNamesCompletionFunc(cli.fs, cli.stateDir), RunE: func(cmd *cobra.Command, args []string) error { @@ -173,7 +174,8 @@ func newVaultRemoveCmd() *cobra.Command { }, } - cmd.Flags().BoolP("force", "f", false, "Force removal even if vault contains secrets") + cmd.Flags().BoolP("force", "f", false, + "Remove without asking for confirmation, even a vault that contains secrets") return cmd } @@ -537,27 +539,27 @@ func (cli *Instance) importMnemonic(cmd *cobra.Command, vaultName string) error return nil } -// vaultHasSecrets reports whether the vault directory contains any secrets -func (cli *Instance) vaultHasSecrets(vaultDir string) (bool, error) { +// countVaultSecrets returns the number of secrets in the vault directory +func (cli *Instance) countVaultSecrets(vaultDir string) (int, error) { secretsDir := filepath.Join(vaultDir, "secrets.d") exists, err := afero.DirExists(cli.fs, secretsDir) if err != nil { - return false, fmt.Errorf("failed to check secrets directory %s: %w", + return 0, fmt.Errorf("failed to check secrets directory %s: %w", secretsDir, err) } if !exists { - return false, nil + return 0, nil } entries, err := afero.ReadDir(cli.fs, secretsDir) if err != nil { - return false, fmt.Errorf("failed to read secrets directory %s: %w", + return 0, fmt.Errorf("failed to read secrets directory %s: %w", secretsDir, err) } - return len(entries) > 0, nil + return len(entries), nil } // switchAwayFromVault selects another vault as current before removal @@ -586,88 +588,107 @@ func (cli *Instance) switchAwayFromVault( return nil } -// RemoveVault removes a vault, holding the state directory lock while -// removeVault runs +// RemoveVault removes a vault and all its secrets, after asking the user +// to confirm unless force is set. func (cli *Instance) RemoveVault(cmd *cobra.Command, name string, force bool) error { err := vault.ValidateVaultName(name) if err != nil { return err } - release, err := vault.LockStateDir(cli.fs, cli.stateDir) + var found vaultToRemove + + release, err := cli.askThenLock(cmd, force, func() (string, error) { + var err error + + found, err = cli.findVaultToRemove(name) + + return found.question, err + }) if err != nil { return err } defer release() - return cli.removeVault(cmd, name, force) -} - -// removeVault removes a vault with safety checks -func (cli *Instance) removeVault(cmd *cobra.Command, name string, force bool) error { - // Get list of all vaults - vaults, err := vault.ListVaults(cli.fs, cli.stateDir) - if err != nil { - return fmt.Errorf("failed to list vaults: %w", err) - } - - // Check if vault exists - if !slices.Contains(vaults, name) { - return fmt.Errorf("vault '%s' %w", name, errVaultDoesNotExist) - } - - // Don't allow removing the last vault - if len(vaults) == 1 { - return errCannotRemoveLastVault - } - - // Check if this is the current vault - currentVault, err := vault.GetCurrentVault(cli.fs, cli.stateDir) - if err != nil { - return fmt.Errorf("failed to get current vault: %w", err) - } - - isCurrentVault := currentVault.GetName() == name - - // Load the vault to check for secrets - vlt := vault.NewVault(cli.fs, cli.stateDir, name) - - vaultDir, err := vlt.GetDirectory() - if err != nil { - return fmt.Errorf("failed to get vault directory: %w", err) - } - - // Check if vault has secrets - hasSecrets, err := cli.vaultHasSecrets(vaultDir) - if err != nil { - return err - } - - // Require --force if vault has secrets - if hasSecrets && !force { - return fmt.Errorf("vault '%s' %w", name, errVaultContainsSecrets) - } - // If removing current vault, switch to another vault first - if isCurrentVault { - err = cli.switchAwayFromVault(cmd, vaults, name) + if found.isCurrent { + err = cli.switchAwayFromVault(cmd, found.vaults, name) if err != nil { return err } } // Remove the vault directory - err = secret.RemoveDirAtomic(cli.fs, vaultDir) + err = secret.RemoveDirAtomic(cli.fs, found.dir) if err != nil { return fmt.Errorf("failed to remove vault directory: %w", err) } cmd.Printf("Removed vault '%s'\n", name) - if hasSecrets { + if found.secrets > 0 { cmd.Printf("Warning: Vault contained secrets that have been " + "permanently deleted\n") } return nil } + +// vaultToRemove is what removing a vault removes, as findVaultToRemove +// found it. +type vaultToRemove struct { + // dir is the vault's directory, which holds all its secrets. + dir string + secrets int + // vaults lists every vault, this one included, and isCurrent is set + // when this one is the current vault. + vaults []string + isCurrent bool + // question names what is removed, for the user to confirm. + question string +} + +// findVaultToRemove checks that the vault exists and is not the last one, +// and counts its secrets. +func (cli *Instance) findVaultToRemove(name string) (vaultToRemove, error) { + vaults, err := vault.ListVaults(cli.fs, cli.stateDir) + if err != nil { + return vaultToRemove{}, fmt.Errorf("failed to list vaults: %w", err) + } + + if !slices.Contains(vaults, name) { + return vaultToRemove{}, + fmt.Errorf("vault '%s' %w", name, errVaultDoesNotExist) + } + + if len(vaults) == 1 { + return vaultToRemove{}, errCannotRemoveLastVault + } + + currentVault, err := vault.GetCurrentVault(cli.fs, cli.stateDir) + if err != nil { + return vaultToRemove{}, + fmt.Errorf("failed to get current vault: %w", err) + } + + vaultDir, err := vault.NewVault(cli.fs, cli.stateDir, name).GetDirectory() + if err != nil { + return vaultToRemove{}, + fmt.Errorf("failed to get vault directory: %w", err) + } + + secrets, err := cli.countVaultSecrets(vaultDir) + if err != nil { + return vaultToRemove{}, err + } + + return vaultToRemove{ + dir: vaultDir, + secrets: secrets, + vaults: vaults, + isCurrent: currentVault.GetName() == name, + question: fmt.Sprintf( + "Permanently remove vault '%s' and its %d secret(s)?", + name, secrets), + }, nil +} diff --git a/internal/cli/version.go b/internal/cli/version.go index a471f0d..6a5e44f 100644 --- a/internal/cli/version.go +++ b/internal/cli/version.go @@ -89,7 +89,8 @@ func VersionCommands(cli *Instance) *cobra.Command { Aliases: []string{"rm"}, Short: "Remove a specific version of a secret", Long: "Remove a specific version of a secret. Cannot remove the " + - "current version.", + "current version. Asks for confirmation first; when stdin " + + "is not a terminal, fails unless --force is given.", Args: cobra.ExactArgs(2), //nolint:mnd // secret-name and version args ValidArgsFunction: func( cmd *cobra.Command, args []string, toComplete string, @@ -102,10 +103,15 @@ func VersionCommands(cli *Instance) *cobra.Command { return nil, cobra.ShellCompDirectiveNoFileComp }, RunE: func(cmd *cobra.Command, args []string) error { - return cli.RemoveVersion(cmd, args[0], args[1]) + force, _ := cmd.Flags().GetBool("force") + + return cli.RemoveVersion(cmd, args[0], args[1], force) }, } + removeCmd.Flags().BoolP("force", "f", false, + "Remove without asking for confirmation") + versionCmd.AddCommand(listCmd, promoteCmd, removeCmd) return versionCmd @@ -297,30 +303,62 @@ func (cli *Instance) PromoteVersion( return nil } -// RemoveVersion removes a specific version of a secret +// RemoveVersion removes a specific version of a secret, after asking the +// user to confirm unless force is set. func (cli *Instance) RemoveVersion( - cmd *cobra.Command, secretName string, version string, + cmd *cobra.Command, secretName string, version string, force bool, ) error { err := vault.ValidateSecretName(secretName) if err != nil { return err } - release, err := vault.LockStateDir(cli.fs, cli.stateDir) + var found versionToRemove + + release, err := cli.askThenLock(cmd, force, func() (string, error) { + var err error + + found, err = cli.findVersionToRemove(secretName, version) + + return found.question, err + }) if err != nil { return err } defer release() - // Get current vault + err = secret.RemoveDirAtomic(cli.fs, found.dir) + if err != nil { + return fmt.Errorf("failed to remove version: %w", err) + } + + cmd.Printf("Removed version %s of secret '%s'\n", version, secretName) + + return nil +} + +// versionToRemove is what removing a version removes, as +// findVersionToRemove found it. +type versionToRemove struct { + // dir is the version's directory. + dir string + // question names what is removed, for the user to confirm. + question string +} + +// findVersionToRemove checks that the version exists in the secret in the +// current vault and is not its current version. +func (cli *Instance) findVersionToRemove( + secretName, version string, +) (versionToRemove, error) { vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir) if err != nil { - return err + return versionToRemove{}, err } vaultDir, err := vlt.GetDirectory() if err != nil { - return err + return versionToRemove{}, err } // Get the encoded secret name @@ -330,45 +368,44 @@ func (cli *Instance) RemoveVersion( // Check if secret exists exists, err := afero.DirExists(cli.fs, secretDir) if err != nil { - return fmt.Errorf("failed to check if secret exists: %w", err) + return versionToRemove{}, + fmt.Errorf("failed to check if secret exists: %w", err) } if !exists { - return fmt.Errorf("secret '%s' %w", secretName, errSecretNotFound) + return versionToRemove{}, + fmt.Errorf("secret '%s' %w", secretName, errSecretNotFound) } // Check if version exists exists, err = secret.VersionExists(cli.fs, secretDir, version) if err != nil { - return fmt.Errorf("failed to check if version exists: %w", err) + return versionToRemove{}, + fmt.Errorf("failed to check if version exists: %w", err) } if !exists { - return fmt.Errorf("version '%s' %w '%s'", + return versionToRemove{}, fmt.Errorf("version '%s' %w '%s'", version, errVersionNotFound, secretName) } // Get current version currentVersion, err := secret.GetCurrentVersion(cli.fs, secretDir) if err != nil { - return fmt.Errorf("failed to get current version: %w", err) + return versionToRemove{}, + fmt.Errorf("failed to get current version: %w", err) } // Don't allow removing the current version if version == currentVersion { - return fmt.Errorf("cannot remove the current version '%s'; %w", + return versionToRemove{}, fmt.Errorf( + "cannot remove the current version '%s'; %w", version, errCannotRemoveCurrentVersion) } - // Remove the version directory - versionDir := filepath.Join(secretDir, "versions", version) - - err = secret.RemoveDirAtomic(cli.fs, versionDir) - if err != nil { - return fmt.Errorf("failed to remove version: %w", err) - } - - cmd.Printf("Removed version %s of secret '%s'\n", version, secretName) - - return nil + return versionToRemove{ + dir: filepath.Join(secretDir, "versions", version), + question: fmt.Sprintf("Permanently remove version %s of secret "+ + "'%s' from vault '%s'?", version, secretName, vlt.GetName()), + }, nil } diff --git a/internal/vault/unlockers.go b/internal/vault/unlockers.go index b609ddc..1ca933a 100644 --- a/internal/vault/unlockers.go +++ b/internal/vault/unlockers.go @@ -274,6 +274,24 @@ func (v *Vault) readUnlockerMetadataOrWarn( return metadata, true } +// HasUnlocker reports whether RemoveUnlocker finds something to remove by +// the ID unlockerID: an unlocker with that ID, or an unlocker directory of +// that name that ListUnlockers skips. +func (v *Vault) HasUnlocker(unlockerID string) (bool, error) { + vaultDir, err := v.GetDirectory() + if err != nil { + return false, err + } + + _, unlockerDir, err := v.findUnlockerByID( + filepath.Join(vaultDir, "unlockers.d"), unlockerID) + if err != nil { + return false, err + } + + return unlockerDir != "", nil +} + // RemoveUnlocker removes an unlocker from this vault. An unlocker // directory that ListUnlockers skips is removed by its directory name; its // type is unknown, so only the directory is removed.