From b9d2797840d1e74faa744cd84c6355501beef303 Mon Sep 17 00:00:00 2001 From: sneak Date: Sun, 4 Oct 2026 09:06:39 +0000 Subject: [PATCH] Skip corrupt unlocker metadata in unlocker select and remove (closes #72) findUnlockerByID failed on the first unlocker directory whose metadata could not be checked, read or parsed, so `secret unlocker select` and `remove` failed when one sorted before the unlocker asked for. It now skips such a directory with the warning ListUnlockers gives, through the code both now share. A skipped directory is removed by its directory name, with RemoveDirAtomic, and cannot be selected. Removing one whose metadata file is missing or corrupt never counts as removing the last unlocker; removing one whose metadata file cannot be checked for or read does, since it may be the only working unlocker. Model: opus-5-5 --- README.md | 4 +- TODO.md | 12 +- internal/cli/unlockers.go | 45 ++++++- internal/cli/unlockers_corrupt_test.go | 167 +++++++++++++++++++++++++ internal/vault/unlockers.go | 147 +++++++++++----------- 5 files changed, 302 insertions(+), 73 deletions(-) create mode 100644 internal/cli/unlockers_corrupt_test.go diff --git a/README.md b/README.md index 2c2bfaa..29d497d 100644 --- a/README.md +++ b/README.md @@ -198,7 +198,9 @@ Creates a new unlocker of the specified type: **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. +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 - **CRITICAL WARNING**: Without unlockers and without your mnemonic phrase, vault data will be PERMANENTLY INACCESSIBLE diff --git a/TODO.md b/TODO.md index 37a4c1c..1e88c5e 100644 --- a/TODO.md +++ b/TODO.md @@ -25,6 +25,16 @@ Bring the repo into policy compliance in one commit: # Completed Steps +- 2026-10-04: `secret unlocker select` and `secret unlocker remove` + skip, with the warning `unlocker list` gives, an unlocker directory + whose metadata file cannot be checked for, read or parsed, instead of + failing when it sorts before the unlocker asked for. Such a directory, + or one without a metadata file, is removed by its directory name, the + name the warning gives; only the directory is removed, since its type + is unknown. Removing one whose metadata file is missing or corrupt + never counts as removing the last unlocker. Removing one whose metadata + file cannot be checked for or read always does, since it may be the + only working unlocker, so in a vault with secrets it needs `--force`. - 2026-10-04: A failed command prints its error once, without the usage text after it (https://git.eeqj.de/sneak/secret/issues/41). Usage is still printed for a command called wrongly: wrong number of arguments, @@ -121,7 +131,7 @@ Bring the repo into policy compliance in one commit: which `vault create` has already made the current vault; - from an unlocker add stopped before its metadata is written, a directory that `unlocker list` warns about and `unlocker rm` - cannot remove; + removes only by its directory name; - data under a `.tmp-` name in the state directory: a secret or version being added, or the secret, version, unlocker or vault being removed, encrypted keys included. Nothing deletes it; it diff --git a/internal/cli/unlockers.go b/internal/cli/unlockers.go index 4f38cce..181431c 100644 --- a/internal/cli/unlockers.go +++ b/internal/cli/unlockers.go @@ -744,14 +744,43 @@ func (cli *Instance) removeUnlocker( return err } - // Get list of unlockers + // 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) } + vaultDir, err := vlt.GetDirectory() + if err != nil { + return 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 + if len(unlockers) == 1 { + lastID, err := findUnlockerIDByMetadata( + cli.fs, unlockersDir, unlockers[0], true) + if err != nil { + return err + } + + removingLast = lastID == unlockerID + } + + // unlockerID may instead name a directory left out of the list. If its + // metadata file is missing or corrupt it is not a working unlocker, so + // removing it never removes the last one. If the file cannot be checked + // 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 + } + + if removingLast { // Check if vault has secrets numSecrets, err := vlt.NumSecrets() if err != nil { @@ -785,6 +814,20 @@ func (cli *Instance) removeUnlocker( return nil } +// metadataUnreadable reports whether checking for or reading the metadata +// file in the unlocker directory unlockerDir fails. A missing file is not +// a failure. +func metadataUnreadable(fs afero.Fs, unlockerDir string) bool { + metadataPath := filepath.Join(unlockerDir, "unlocker-metadata.json") + + exists, err := afero.Exists(fs, metadataPath) + if err == nil && exists { + _, err = afero.ReadFile(fs, metadataPath) + } + + return err != nil +} + // UnlockerSelect selects an unlocker as current func (cli *Instance) UnlockerSelect(unlockerID string) error { release, err := vault.LockStateDir(cli.fs, cli.stateDir) diff --git a/internal/cli/unlockers_corrupt_test.go b/internal/cli/unlockers_corrupt_test.go new file mode 100644 index 0000000..d7dd8c7 --- /dev/null +++ b/internal/cli/unlockers_corrupt_test.go @@ -0,0 +1,167 @@ +// Corrupt Unlocker Tests +// +// `secret unlocker select` and `secret unlocker remove` find an unlocker +// by its ID. These tests give the first unlocker, which sorts before the +// 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. + +//nolint:testpackage // white-box test of unexported internals +package cli + +import ( + "path/filepath" + "testing" + + "git.eeqj.de/sneak/secret/internal/vault" + "github.com/spf13/afero" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// newCorruptUnlockerVault returns the two-unlocker test vault with the +// metadata of the first unlocker replaced by text that is not JSON. +func newCorruptUnlockerVault(t *testing.T) *afero.MemMapFs { + t.Helper() + + fs := newListTestVault(t, 2) + require.NoError(t, afero.WriteFile(fs, + filepath.Join(testVaultDir(listTestVaultName), listTestUnlockersDirName, + listTestUnlockerDirOne, listTestMetadataFileName), + []byte("not json"), listTestFilePerm)) + + return fs +} + +// TestUnlockerSelectSkipsCorruptUnlocker asserts that the second unlocker +// can be selected, and that the corrupt one, having no type to be used as, +// cannot be selected by its directory name. +func TestUnlockerSelectSkipsCorruptUnlocker(t *testing.T) { + t.Parallel() + + fs := newCorruptUnlockerVault(t) + instance, _ := newTestInstance(fs) + + require.NoError(t, instance.UnlockerSelect("pgp-"+listTestGPGKeyID+"B")) + + current, err := afero.ReadFile(fs, + filepath.Join(testVaultDir(listTestVaultName), "current-unlocker")) + require.NoError(t, err) + assert.Equal(t, listTestUnlockerDirTwo, string(current)) + + err = instance.UnlockerSelect(listTestUnlockerDirOne) + require.ErrorIs(t, err, vault.ErrUnlockerNotFound) +} + +// 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. +func TestUnlockerRemoveWithCorruptUnlocker(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + unlockerID string + withSecret bool + wantErr error + wantEntries []string + }{ + { + name: "the other unlocker", + unlockerID: "pgp-" + listTestGPGKeyID + "B", + 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}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + fs := newCorruptUnlockerVault(t) + if tt.withSecret { + writeTestSecret(t, fs, testVaultDir(listTestVaultName)) + } + + instance, cmd := newTestInstance(fs) + + err := instance.UnlockersRemove(tt.unlockerID, false, cmd) + require.ErrorIs(t, err, tt.wantErr) + + assertDirEntries(t, fs, + filepath.Join(testVaultDir(listTestVaultName), + listTestUnlockersDirName), + tt.wantEntries...) + }) + } +} + +// 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. +func TestUnlockerRemoveWithUnreadableMetadata(t *testing.T) { + t.Parallel() + + vaultDir := testVaultDir(listTestVaultName) + unlockersDir := filepath.Join(vaultDir, listTestUnlockersDirName) + failingPath := filepath.Join(unlockersDir, listTestUnlockerDirOne, + listTestMetadataFileName) + + tests := []struct { + name string + wrap func(base afero.Fs) afero.Fs + }{ + { + name: "checking for the file fails", + wrap: func(base afero.Fs) afero.Fs { + return &metadataStatFailFs{Fs: base, uncheckablePath: failingPath} + }, + }, + { + name: "reading the file fails", + wrap: func(base afero.Fs) afero.Fs { + return &metadataReadFailFs{Fs: base, unreadablePath: failingPath} + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + base := newListTestVault(t, 1) + writeTestSecret(t, base, vaultDir) + + instance, cmd := newTestInstance(tt.wrap(base)) + + err := instance.removeUnlocker(listTestUnlockerDirOne, false, cmd) + require.ErrorIs(t, err, errLastUnlocker) + assertDirEntries(t, base, unlockersDir, listTestUnlockerDirOne) + + require.NoError(t, + instance.removeUnlocker(listTestUnlockerDirOne, true, cmd)) + assertDirEntries(t, base, unlockersDir) + }) + } +} diff --git a/internal/vault/unlockers.go b/internal/vault/unlockers.go index a52f603..105c046 100644 --- a/internal/vault/unlockers.go +++ b/internal/vault/unlockers.go @@ -126,7 +126,11 @@ func (v *Vault) resolveUnlockerDirectory(currentUnlockerPath string) (string, er } // findUnlockerByID finds an unlocker by its ID and returns the unlocker -// instance and its directory path +// 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 +// no unlocker has the ID unlockerID but such a directory is named +// unlockerID, that directory is returned with a nil unlocker, so that +// RemoveUnlocker can remove it. // //nolint:ireturn // returns one of several concrete unlocker implementations func (v *Vault) findUnlockerByID( @@ -137,42 +141,24 @@ func (v *Vault) findUnlockerByID( return nil, "", fmt.Errorf("failed to read unlockers directory: %w", err) } + skippedDirPath := "" + for _, file := range files { if !file.IsDir() { continue } - // Read metadata file - metadataPath := filepath.Join(unlockersDir, file.Name(), "unlocker-metadata.json") + unlockerDirPath := filepath.Join(unlockersDir, file.Name()) - exists, err := afero.Exists(v.fs, metadataPath) - if err != nil { - return nil, "", fmt.Errorf( - "failed to check if metadata exists for unlocker %s: %w", - file.Name(), err) - } + metadata, ok := v.readUnlockerMetadataOrWarn(unlockersDir, file.Name()) + if !ok { + if file.Name() == unlockerID { + skippedDirPath = unlockerDirPath + } - if !exists { - // Skip directories without metadata - they might not be unlockers continue } - metadataBytes, err := afero.ReadFile(v.fs, metadataPath) - if err != nil { - return nil, "", fmt.Errorf( - "failed to read metadata for unlocker %s: %w", file.Name(), err) - } - - var metadata UnlockerMetadata - - err = json.Unmarshal(metadataBytes, &metadata) - if err != nil { - return nil, "", fmt.Errorf( - "failed to parse metadata for unlocker %s: %w", file.Name(), err) - } - - unlockerDirPath := filepath.Join(unlockersDir, file.Name()) - // Create the appropriate unlocker instance var tempUnlocker secret.Unlocker @@ -195,7 +181,7 @@ func (v *Vault) findUnlockerByID( } } - return nil, "", nil + return nil, skippedDirPath, nil } // ListUnlockers returns a list of available unlockers for this vault @@ -226,44 +212,12 @@ func (v *Vault) ListUnlockers() ([]UnlockerMetadata, error) { var unlockers []UnlockerMetadata for _, file := range files { - if file.IsDir() { - // Read metadata file - metadataPath := filepath.Join(unlockersDir, file.Name(), - "unlocker-metadata.json") - - exists, err := afero.Exists(v.fs, metadataPath) - if err != nil { - secret.Warn("Skipping unlocker directory whose metadata file cannot be checked", - "directory", file.Name(), "error", err) - - continue - } - - if !exists { - secret.Warn("Skipping unlocker directory with missing metadata file", - "directory", file.Name()) - - continue - } - - metadataBytes, err := afero.ReadFile(v.fs, metadataPath) - if err != nil { - secret.Warn("Skipping unlocker directory with unreadable metadata file", - "directory", file.Name(), "error", err) - - continue - } - - var metadata UnlockerMetadata - - err = json.Unmarshal(metadataBytes, &metadata) - if err != nil { - secret.Warn("Skipping unlocker directory with corrupt metadata file", - "directory", file.Name(), "error", err) - - continue - } + if !file.IsDir() { + continue + } + metadata, ok := v.readUnlockerMetadataOrWarn(unlockersDir, file.Name()) + if ok { unlockers = append(unlockers, metadata) } } @@ -271,7 +225,54 @@ func (v *Vault) ListUnlockers() ([]UnlockerMetadata, error) { return unlockers, nil } -// RemoveUnlocker removes an unlocker from this vault +// readUnlockerMetadataOrWarn reads the metadata of the unlocker directory +// name in unlockersDir. If the metadata file cannot be checked for, is +// missing, or cannot be read or parsed, it warns, naming the directory, +// and returns false: the caller skips that directory. +func (v *Vault) readUnlockerMetadataOrWarn( + unlockersDir, name string, +) (UnlockerMetadata, bool) { + metadataPath := filepath.Join(unlockersDir, name, "unlocker-metadata.json") + + var metadata UnlockerMetadata + + exists, err := afero.Exists(v.fs, metadataPath) + if err != nil { + secret.Warn("Skipping unlocker directory whose metadata file cannot be checked", + "directory", name, "error", err) + + return metadata, false + } + + if !exists { + secret.Warn("Skipping unlocker directory with missing metadata file", + "directory", name) + + return metadata, false + } + + metadataBytes, err := afero.ReadFile(v.fs, metadataPath) + if err != nil { + secret.Warn("Skipping unlocker directory with unreadable metadata file", + "directory", name, "error", err) + + return metadata, false + } + + err = json.Unmarshal(metadataBytes, &metadata) + if err != nil { + secret.Warn("Skipping unlocker directory with corrupt metadata file", + "directory", name, "error", err) + + return metadata, false + } + + return metadata, true +} + +// 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 { vaultDir, err := v.GetDirectory() if err != nil { @@ -282,15 +283,19 @@ func (v *Vault) RemoveUnlocker(unlockerID string) error { unlockersDir := filepath.Join(vaultDir, "unlockers.d") // Find the unlocker by ID - unlocker, _, err := v.findUnlockerByID(unlockersDir, unlockerID) + unlocker, unlockerDir, err := v.findUnlockerByID(unlockersDir, unlockerID) if err != nil { return err } - if unlocker == nil { + if unlockerDir == "" { return fmt.Errorf("unlocker with ID %s %w", unlockerID, ErrUnlockerNotFound) } + if unlocker == nil { + return secret.RemoveDirAtomic(v.fs, unlockerDir) + } + // Use the unlocker's Remove method return unlocker.Remove() } @@ -306,12 +311,14 @@ func (v *Vault) SelectUnlocker(unlockerID string) error { unlockersDir := filepath.Join(vaultDir, "unlockers.d") // Find the unlocker by ID - _, targetUnlockerDir, err := v.findUnlockerByID(unlockersDir, unlockerID) + unlocker, targetUnlockerDir, err := v.findUnlockerByID(unlockersDir, unlockerID) if err != nil { return err } - if targetUnlockerDir == "" { + // A directory found without an unlocker is one ListUnlockers skips; it + // cannot be selected. + if unlocker == nil { return fmt.Errorf("unlocker with ID %s %w", unlockerID, ErrUnlockerNotFound) }