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 2a8d576..b0e30af 100644 --- a/TODO.md +++ b/TODO.md @@ -25,6 +25,13 @@ 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 it never counts as removing the last unlocker. - 2026-10-04: The `Makefile` no longer sets `DOCKER_HOST`, so its docker targets use the local docker daemon, or whatever `DOCKER_HOST` the environment sets. `make build` calls the new `script/build`, which @@ -105,7 +112,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..e94daa1 100644 --- a/internal/cli/unlockers.go +++ b/internal/cli/unlockers.go @@ -744,14 +744,33 @@ func (cli *Instance) removeUnlocker( return err } - // Get list of unlockers + // Get list of unlockers. It leaves out a directory whose metadata + // cannot be read or parsed: that unlocker does not work, so removing it + // by its directory name never removes the last unlocker. unlockers, err := vlt.ListUnlockers() if err != nil { return fmt.Errorf("failed to list unlockers: %w", err) } // Check if we're removing the last unlocker + removingLast := false + if len(unlockers) == 1 { + vaultDir, err := vlt.GetDirectory() + if err != nil { + return fmt.Errorf("failed to get vault directory: %w", err) + } + + lastID, err := findUnlockerIDByMetadata(cli.fs, + filepath.Join(vaultDir, "unlockers.d"), unlockers[0], true) + if err != nil { + return err + } + + removingLast = lastID == unlockerID + } + + if removingLast { // Check if vault has secrets numSecrets, err := vlt.NumSecrets() if err != nil { diff --git a/internal/cli/unlockers_corrupt_test.go b/internal/cli/unlockers_corrupt_test.go new file mode 100644 index 0000000..65a23fc --- /dev/null +++ b/internal/cli/unlockers_corrupt_test.go @@ -0,0 +1,112 @@ +// 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. + +//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...) + }) + } +} 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) }