diff --git a/README.md b/README.md index 0bb262a..72f4964 100644 --- a/README.md +++ b/README.md @@ -241,6 +241,14 @@ 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 +that `secret unlocker select` accepts, in the order `secret unlocker list` shows +them, the current one, and says which; when there is none, the vault is left +without a current unlocker. `secret unlocker select` refuses an unlocker whose +type is not `passphrase` or `pgp`. When `current-unlocker` names an unlocker +that does not exist and the vault has another that `secret unlocker select` +accepts, opening the vault fails with an error that says to select one 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 cfc97da..b608afe 100644 --- a/TODO.md +++ b/TODO.md @@ -18,6 +18,19 @@ 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 that + `secret unlocker select` accepts, in the order `secret unlocker list` shows, + current before removing it, and says so; when there is none, it 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 another that `secret unlocker select` accepts, the error + wraps `vault.ErrUnlockerNotFound` and says to run `secret unlocker select`, + without the advice to use the mnemonic. - 2026-10-14: A `secret add --force` that cannot unlock the vault, given a wrong passphrase or mnemonic, leaves the secret as it was (https://git.eeqj.de/sneak/secret/issues/136). The new version used to be diff --git a/internal/cli/unlock_failure_test.go b/internal/cli/unlock_failure_test.go index d8ea8c7..817c9e1 100644 --- a/internal/cli/unlock_failure_test.go +++ b/internal/cli/unlock_failure_test.go @@ -4,9 +4,11 @@ // 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 that `secret unlocker select` accepts, 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 +224,69 @@ 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")) +} + +// TestMissingCurrentUnlockerNamesMnemonic checks the error of `secret get` +// when the unlocker that current-unlocker names does not exist and the +// vault's only other unlocker is the directory "odd", whose metadata names a +// type that `secret unlocker select` refuses: the error ends with the advice +// that the mnemonic still opens the vault, not with advice to select another +// unlocker. The test then follows that advice. +func TestMissingCurrentUnlockerNamesMnemonic(t *testing.T) { + t.Parallel() + + fs := newTwoVaultFs(t) + unlockerDir, c := newUnlockTestCLI(t, fs) + + require.NoError(t, fs.RemoveAll(unlockerDir)) + require.NoError(t, afero.WriteFile(fs, + filepath.Join(unlockTestVaultDir, "unlockers.d", "odd", + "unlocker-metadata.json"), + []byte(`{"type": "odd"}`), secret.FilePerms)) + + err := c.GetSecret(discardCmd(), "x") + + var cause *os.PathError + + require.ErrorAs(t, err, &cause) + require.NotErrorIs(t, err, vault.ErrUnlockerNotFound) + require.EqualError(t, err, "failed to unlock vault: "+ + "failed to get long-term key: failed to get current unlocker: "+ + "failed to read unlocker metadata: "+cause.Error()+mnemonicAdvice) + + c.Mnemonic = testMnemonicBuffer(t) + require.NoError(t, c.UnlockersAdd("passphrase", discardCmd())) + + c.Mnemonic = nil + 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..d354ce2 100644 --- a/internal/cli/unlockers.go +++ b/internal/cli/unlockers.go @@ -222,8 +222,11 @@ 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 that ` + + `'secret unlocker select' accepts. 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 +615,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..f27911e --- /dev/null +++ b/internal/cli/unlockers_remove_test.go @@ -0,0 +1,94 @@ +//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/secret" + "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 removes +// current-unlocker. +// +// The second case also gives the vault the unlocker directory "odd", whose +// metadata names a type that `secret unlocker select` refuses, and which +// sorts before the passphrase unlocker: neither removal makes it current. +// +//nolint:paralleltest // t.Setenv (GNUPGHOME) forbids parallel tests +func TestUnlockerRemoveCurrent(t *testing.T) { + tests := []struct { + name string + withOdd bool + }{ + {name: "two unlockers", withOdd: false}, + {name: "and the directory odd", withOdd: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(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) + + if tt.withOdd { + require.NoError(t, afero.WriteFile(fs, + filepath.Join(testVaultDir(listTestVaultName), "unlockers.d", + "odd", "unlocker-metadata.json"), + []byte(`{"type": "odd"}`), secret.FilePerms)) + } + + 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 passphrase 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..62fdeba 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" @@ -19,6 +21,7 @@ import ( // Unlocker metadata type strings. const ( unlockerTypePassphrase = "passphrase" + unlockerTypePGP = "pgp" ) // GetCurrentUnlocker returns the current unlocker for this vault @@ -52,6 +55,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), @@ -74,7 +82,7 @@ func (v *Vault) GetCurrentUnlocker() (secret.Unlocker, error) { passphraseUnlocker := secret.NewPassphraseUnlocker(v.fs, unlockerDir, metadata) passphraseUnlocker.Passphrase = v.UnlockPassphrase unlocker = passphraseUnlocker - case "pgp": + case unlockerTypePGP: secret.Debug("Creating PGP unlocker instance", "unlocker_type", metadata.Type) unlocker = secret.NewPGPUnlocker(v.fs, unlockerDir, metadata) @@ -119,6 +127,37 @@ 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 another unlocker +// that SelectUnlocker accepts. When it 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 + } + + otherID, err := v.firstOtherUnlockerID(filepath.Base(unlockerDir)) + if err != nil { + return err + } + + if otherID == "" { + 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 @@ -159,7 +198,7 @@ func (v *Vault) findUnlockerByID( switch metadata.Type { case unlockerTypePassphrase: tempUnlocker = secret.NewPassphraseUnlocker(v.fs, unlockerDirPath, metadata) - case "pgp": + case unlockerTypePGP: tempUnlocker = secret.NewPGPUnlocker(v.fs, unlockerDirPath, metadata) default: continue @@ -282,10 +321,15 @@ 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 unlocker +// firstOtherUnlockerID returns is made current before the removal; with +// none, 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 +338,92 @@ 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 unlocker firstOtherUnlockerID returns for +// unlockerID the current unlocker and returns its ID. It returns "" when +// there is no such unlocker. +func (v *Vault) selectOtherUnlocker(unlockerID string) (string, error) { + otherID, err := v.firstOtherUnlockerID(unlockerID) + if err != nil { + return "", err + } + + if otherID == "" { + return "", nil + } + + err = v.SelectUnlocker(otherID) + if err != nil { + return "", err + } + + return otherID, nil +} + +// firstOtherUnlockerID returns the ID of the vault's first unlocker other +// than unlockerID, in ID order, the order `secret unlocker list` shows, that +// SelectUnlocker accepts, or "" when there is none. SelectUnlocker refuses +// an unlocker whose metadata names a type other than passphrase or pgp, +// which ListUnlockers still returns. +func (v *Vault) firstOtherUnlockerID(unlockerID string) (string, error) { + unlockers, err := v.ListUnlockers() + if err != nil { + return "", err + } + + for _, id := range slices.Sorted(maps.Keys(unlockers)) { + unlockerType := unlockers[id].Type + + if id != unlockerID && (unlockerType == unlockerTypePassphrase || + unlockerType == unlockerTypePGP) { + 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 }