diff --git a/README.md b/README.md index 0bb262a..83d86e8 100644 --- a/README.md +++ b/README.md @@ -241,6 +241,12 @@ 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. +Removing the current unlocker makes the first of the vault's other unlockers, in +the order `secret unlocker list` shows them, the current one, and says which; +removing the last unlocker leaves the vault without a current unlocker. When +`current-unlocker` names an unlocker that does not exist and the vault has +others, opening the vault fails with an error that says to select one of them +with `secret unlocker select`. - `--force, -f`: Remove without asking, even the last unlocker - **CRITICAL WARNING**: Without unlockers and without your mnemonic phrase, diff --git a/TODO.md b/TODO.md index 54fcc47..f133cdf 100644 --- a/TODO.md +++ b/TODO.md @@ -18,6 +18,18 @@ https://git.eeqj.de/sneak/secret/milestone/12 # Completed Steps +- 2026-10-14: Removing the current unlocker no longer leaves the vault unable to + open through the unlockers it still has + (https://git.eeqj.de/sneak/secret/issues/137). `current-unlocker` went on + naming the removed unlocker, so every command that opened the vault failed, + with advice to use the mnemonic. `secret unlocker remove` of the current + unlocker now makes the first of the vault's other unlockers, in the order + `secret unlocker list` shows, current before removing it, and says so; + removing the last unlocker also removes `current-unlocker`. + `vault.RemoveUnlocker` returns the ID of the unlocker it made current. When + `current-unlocker` names an unlocker that does not exist and the vault has + others, the error wraps `vault.ErrUnlockerNotFound` and says to run + `secret unlocker select`, without the advice to use the mnemonic. - 2026-10-12: A mnemonic typed with extra spaces, tabs or line breaks gives the same long-term key as its words separated by single spaces (https://git.eeqj.de/sneak/secret/issues/135). The key was derived from the diff --git a/internal/cli/unlock_failure_test.go b/internal/cli/unlock_failure_test.go index d8ea8c7..5f816a4 100644 --- a/internal/cli/unlock_failure_test.go +++ b/internal/cli/unlock_failure_test.go @@ -4,9 +4,10 @@ // file the unlocker needs is missing or the passphrase is wrong, the error // keeps its cause and ends by saying that the mnemonic still opens that // vault, but only for a vault that the mnemonic does open, and not when the -// passphrase could not be read at all. When a secret's current file is -// missing, the error says how to make a version current again. Each test -// that pins such advice also follows it. +// passphrase could not be read at all. When the current unlocker does not +// exist and the vault has another, the error says to select one instead. +// When a secret's current file is missing, the error says how to make a +// version current again. Each test that pins such advice also follows it. package cli_test @@ -222,6 +223,34 @@ func TestMoveUnlockFailureNamesVault(t *testing.T) { assert.Equal(t, "value", getSecret(t, c, "y")) } +// TestMissingCurrentUnlockerNamesSelect checks the error of `secret get` +// when current-unlocker names an unlocker that does not exist and the vault +// has another: it names the missing unlocker and says to select another +// one, without the advice to use the mnemonic. The test then follows that +// advice. +func TestMissingCurrentUnlockerNamesSelect(t *testing.T) { + t.Parallel() + + fs := newTwoVaultFs(t) + unlockerDir, c := newUnlockTestCLI(t, fs) + + require.NoError(t, afero.WriteFile(fs, + filepath.Join(unlockTestVaultDir, "current-unlocker"), + []byte("missing"), secret.FilePerms)) + + err := c.GetSecret(discardCmd(), "x") + + require.ErrorIs(t, err, vault.ErrUnlockerNotFound) + require.EqualError(t, err, "failed to unlock vault: "+ + "failed to get long-term key: failed to get current unlocker: "+ + "unlocker with ID missing not found; the vault 'default' has other "+ + "unlockers: run 'secret unlocker select' with the ID of one of them "+ + "to make it the current unlocker") + + require.NoError(t, c.UnlockerSelect(filepath.Base(unlockerDir))) + assert.Equal(t, "value", getSecret(t, c, "x")) +} + // TestPassphraseNotReadNamesNoMnemonic runs `secret get x` on the built // binary without SB_UNLOCK_PASSPHRASE and with a stdin that is not a // terminal, so the passphrase cannot be read. The unlocker was not tried, diff --git a/internal/cli/unlockers.go b/internal/cli/unlockers.go index 6792a0f..b4595a9 100644 --- a/internal/cli/unlockers.go +++ b/internal/cli/unlockers.go @@ -222,8 +222,10 @@ func newUnlockerRemoveCmd() *cobra.Command { 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.`, + `is given. Removing the current unlocker makes another of the ` + + `vault's unlockers current, if it has one. 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 { @@ -612,13 +614,17 @@ func (cli *Instance) removeUnlocker( "have your mnemonic phrase to access this vault again!") } - err := found.vlt.RemoveUnlocker(unlockerID) + newCurrentID, err := found.vlt.RemoveUnlocker(unlockerID) if err != nil { return err } cmd.Printf("Removed unlocker '%s'\n", unlockerID) + if newCurrentID != "" { + cmd.Printf("Switched current unlocker to '%s'\n", newCurrentID) + } + return nil } diff --git a/internal/cli/unlockers_remove_test.go b/internal/cli/unlockers_remove_test.go new file mode 100644 index 0000000..d26e809 --- /dev/null +++ b/internal/cli/unlockers_remove_test.go @@ -0,0 +1,68 @@ +//nolint:testpackage // white-box test of unexported internals +package cli + +import ( + "bytes" + "path/filepath" + "testing" + + "github.com/awnumar/memguard" + "github.com/spf13/afero" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sneak.berlin/go/secret/internal/vault" +) + +// TestUnlockerRemoveCurrent is a regression test for +// https://git.eeqj.de/sneak/secret/issues/137. A vault has a passphrase +// unlocker and a PGP unlocker, the current one. Removing the PGP unlocker +// makes the passphrase unlocker current and says so, and the vault then +// opens through it. Removing the passphrase unlocker too, the vault's last, +// removes current-unlocker. +// +//nolint:paralleltest // t.Setenv (GNUPGHOME) forbids parallel tests +func TestUnlockerRemoveCurrent(t *testing.T) { + newTestGPGKey(t) + + passphrase := memguard.NewBufferFromBytes([]byte(testPassphrase)) + t.Cleanup(passphrase.Destroy) + + fs := afero.NewMemMapFs() + vlt, err := vault.CreateVault(fs, listTestStateDir, listTestVaultName, + testMnemonicBuffer(t), nil) + require.NoError(t, err) + + passphraseUnlocker, err := vlt.CreatePassphraseUnlocker(passphrase) + require.NoError(t, err) + + instance, cmd := newTestInstance(fs) + instance.UnlockPassphrase = passphrase + + cmd.Flags().String("keyid", unreadableTestGPGUserID, "") + require.NoError(t, instance.UnlockersAdd(unlockerTypePGP, cmd)) + + pgpUnlocker, err := vlt.GetCurrentUnlocker() + require.NoError(t, err) + require.Equal(t, unlockerTypePGP, pgpUnlocker.GetType()) + + var output bytes.Buffer + + cmd.SetOut(&output) + require.NoError(t, instance.UnlockersRemove(pgpUnlocker.GetID(), true, cmd)) + assert.Contains(t, output.String(), + "Switched current unlocker to '"+passphraseUnlocker.GetID()+"'") + + reopened := vault.NewVault(fs, listTestStateDir, listTestVaultName) + reopened.UnlockPassphrase = passphrase + + _, err = reopened.GetOrDeriveLongTermKey() + require.NoError(t, err) + + require.NoError(t, + instance.UnlockersRemove(passphraseUnlocker.GetID(), true, cmd)) + + exists, err := afero.Exists(fs, + filepath.Join(testVaultDir(listTestVaultName), "current-unlocker")) + require.NoError(t, err) + assert.False(t, exists, "current-unlocker is left after the last unlocker") +} diff --git a/internal/vault/errors_test.go b/internal/vault/errors_test.go index d0e74c7..bc84129 100644 --- a/internal/vault/errors_test.go +++ b/internal/vault/errors_test.go @@ -81,7 +81,9 @@ func TestVaultErrors(t *testing.T) { return vlt.CopySecretAllVersions(vlt, versionless, "copy", false) }, vault.ErrNoVersions}, {"remove a missing unlocker", func(vlt *vault.Vault) error { - return vlt.RemoveUnlocker(missingName) + _, err := vlt.RemoveUnlocker(missingName) + + return err }, vault.ErrUnlockerNotFound}, {"select a missing unlocker", func(vlt *vault.Vault) error { return vlt.SelectUnlocker(missingName) diff --git a/internal/vault/unlockers.go b/internal/vault/unlockers.go index 28d251c..e351cf4 100644 --- a/internal/vault/unlockers.go +++ b/internal/vault/unlockers.go @@ -5,8 +5,10 @@ import ( "errors" "fmt" "log/slog" + "maps" "os" "path/filepath" + "slices" "strings" "time" @@ -52,6 +54,11 @@ func (v *Vault) GetCurrentUnlocker() (secret.Unlocker, error) { return nil, err } + err = v.checkCurrentUnlockerExists(unlockerDir) + if err != nil { + return nil, err + } + secret.DebugWith("Resolved unlocker directory", slog.String("unlocker_dir", unlockerDir), slog.String("vault_name", v.Name), @@ -119,6 +126,36 @@ func (v *Vault) resolveUnlockerDirectory(currentUnlockerPath string) (string, er return absolutePath, nil } +// checkCurrentUnlockerExists fails with ErrUnlockerNotFound, saying to +// select another unlocker, when unlockerDir, the unlocker directory that +// current-unlocker names, does not exist and the vault has other unlockers. +// When the vault has none, it returns nil: reading the missing unlocker then +// fails, with the advice that the mnemonic still opens the vault. +func (v *Vault) checkCurrentUnlockerExists(unlockerDir string) error { + exists, err := afero.DirExists(v.fs, unlockerDir) + if err != nil { + return fmt.Errorf("failed to check for the current unlocker: %w", err) + } + + if exists { + return nil + } + + unlockers, err := v.ListUnlockers() + if err != nil { + return err + } + + if len(unlockers) == 0 { + return nil + } + + return fmt.Errorf("unlocker with ID %s %w; the vault '%s' has other "+ + "unlockers: run %s with the ID of one of them to make it the current "+ + "unlocker", filepath.Base(unlockerDir), ErrUnlockerNotFound, v.Name, + v.withVaultSelect("'secret unlocker select'")) +} + // findUnlockerByID finds an unlocker by its ID and returns the unlocker // instance and its directory path. A directory that ListUnlockers skips is // skipped here too, with the same warning. Such a directory has no ID: if @@ -282,10 +319,16 @@ func (v *Vault) HasUnlocker(unlockerID string) (bool, error) { // 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. -func (v *Vault) RemoveUnlocker(unlockerID string) error { +// +// It returns the ID of the unlocker it made current, or "" when it made none +// current. If the removed unlocker is the current one, the vault's first +// other unlocker in ID order, the order `secret unlocker list` shows, is +// made current before the removal; with no other, current-unlocker is +// removed after it. +func (v *Vault) RemoveUnlocker(unlockerID string) (string, error) { vaultDir, err := v.GetDirectory() if err != nil { - return err + return "", err } // Find the unlocker directory and create the unlocker instance @@ -294,19 +337,73 @@ func (v *Vault) RemoveUnlocker(unlockerID string) error { // Find the unlocker by ID unlocker, unlockerDir, err := v.findUnlockerByID(unlockersDir, unlockerID) if err != nil { - return err + return "", err } if unlockerDir == "" { - return fmt.Errorf("unlocker with ID %s %w", unlockerID, ErrUnlockerNotFound) + return "", fmt.Errorf("unlocker with ID %s %w", unlockerID, ErrUnlockerNotFound) + } + + currentUnlockerPath := filepath.Join(vaultDir, "current-unlocker") + + currentDir, err := v.resolveUnlockerDirectory(currentUnlockerPath) + if err != nil && !errors.Is(err, os.ErrNotExist) { + return "", err + } + + isCurrent := currentDir == unlockerDir + newCurrentID := "" + + if isCurrent { + newCurrentID, err = v.selectOtherUnlocker(unlockerID) + if err != nil { + return "", err + } } if unlocker == nil { - return secret.RemoveDirAtomic(v.fs, unlockerDir) + err = secret.RemoveDirAtomic(v.fs, unlockerDir) + } else { + err = unlocker.Remove() } - // Use the unlocker's Remove method - return unlocker.Remove() + if err != nil { + return "", err + } + + if isCurrent && newCurrentID == "" { + err = v.fs.Remove(currentUnlockerPath) + if err != nil { + return "", fmt.Errorf("failed to remove current-unlocker file: %w", err) + } + } + + return newCurrentID, nil +} + +// selectOtherUnlocker makes the vault's first unlocker in ID order other +// than unlockerID the current unlocker and returns its ID. It returns "" +// when the vault has no other unlocker. +func (v *Vault) selectOtherUnlocker(unlockerID string) (string, error) { + unlockers, err := v.ListUnlockers() + if err != nil { + return "", err + } + + for _, id := range slices.Sorted(maps.Keys(unlockers)) { + if id == unlockerID { + continue + } + + err = v.SelectUnlocker(id) + if err != nil { + return "", err + } + + return id, nil + } + + return "", nil } // SelectUnlocker selects an unlocker as current for this vault diff --git a/internal/vault/vault.go b/internal/vault/vault.go index 3aa45f6..d289bef 100644 --- a/internal/vault/vault.go +++ b/internal/vault/vault.go @@ -291,9 +291,11 @@ func (v *Vault) unlockLongTermKey( // without a mnemonic records none, and without its metadata the key cannot // be derived. It is not added when the passphrase could not be read: the // unlocker was not tried, and adding one would need a passphrase read the -// same way. +// same way. Nor is it added when the current unlocker does not exist and the +// vault has others: the error already says to select one of them. func (v *Vault) withMnemonicAdvice(err error) error { - if errors.Is(err, secret.ErrPassphraseNotRead) { + if errors.Is(err, secret.ErrPassphraseNotRead) || + errors.Is(err, ErrUnlockerNotFound) { return err } @@ -304,15 +306,21 @@ func (v *Vault) withMnemonicAdvice(err error) error { return err } - // 'secret unlocker add' acts on the current vault only. - steps := "'secret unlocker add passphrase'" - - current, currentErr := GetCurrentVault(v.fs, v.stateDir) - if currentErr != nil || current.Name != v.Name { - steps = fmt.Sprintf("'secret vault select %s', then %s", v.Name, steps) - } - return fmt.Errorf("%w; the vault '%s' still opens with its mnemonic: run "+ "%s with %s set to the mnemonic to give it a new unlocker", - err, v.Name, steps, secret.EnvMnemonic) + err, v.Name, v.withVaultSelect("'secret unlocker add passphrase'"), + secret.EnvMnemonic) +} + +// withVaultSelect returns command, a quoted 'secret unlocker' command, for +// advice about this vault. Such a command acts on the current vault only, +// so when this vault is not the current one, the command that selects it +// comes first. +func (v *Vault) withVaultSelect(command string) string { + current, err := GetCurrentVault(v.fs, v.stateDir) + if err != nil || current.Name != v.Name { + return fmt.Sprintf("'secret vault select %s', then %s", v.Name, command) + } + + return command }