diff --git a/TODO.md b/TODO.md index 281cbac..307e98b 100644 --- a/TODO.md +++ b/TODO.md @@ -25,6 +25,13 @@ Bring the repo into policy compliance in one commit: # Completed Steps +- 2026-10-03: The checks run before changing a vault now stop with an + error naming the path and cause when they cannot read what they + inspect, instead of reading the failure as "nothing there": the + duplicate check before `unlocker add pgp` (an unreadable + `unlockers.d`), the secret count that guards removing the last + unlocker and removing a vault, and the existing long-term key check + before `vault import`. - 2026-10-02: A plain `docker build .` builds again: the size tests skip a case that needs more locked memory than the process can lock, and run every case under `script/cibuild`. The image stamps the diff --git a/internal/cli/unlockers.go b/internal/cli/unlockers.go index 593f462..561873e 100644 --- a/internal/cli/unlockers.go +++ b/internal/cli/unlockers.go @@ -49,7 +49,6 @@ var ( "is already added as an unlocker") errUnsupportedUnlockerType = errors.New("unsupported unlocker type") errLastUnlocker = errors.New("refusing to remove last unlocker") - errUnlockerExists = errors.New("unlocker already exists") ) // UnlockerInfo represents unlocker information for display @@ -691,8 +690,15 @@ func (cli *Instance) addPGPUnlocker(cmd *cobra.Command) error { // Check if this GPG key is already added expectedID := "pgp-" + fingerprint - err = cli.checkUnlockerExists(vlt, expectedID) + exists, err := cli.checkUnlockerExists(vlt, expectedID) if err != nil { + return fmt.Errorf( + "could not check whether GPG key %s is already an unlocker: %w", + gpgKeyID, err, + ) + } + + if exists { return fmt.Errorf("GPG key %s %w", gpgKeyID, errGPGKeyAlreadyUnlocker) } @@ -772,44 +778,38 @@ func (cli *Instance) UnlockerSelect(unlockerID string) error { return vlt.SelectUnlocker(unlockerID) } -// checkUnlockerExists checks if an unlocker with the given ID exists -func (cli *Instance) checkUnlockerExists(vlt *vault.Vault, unlockerID string) error { - // Get the list of unlockers and check if any match the ID - unlockers, err := vlt.ListUnlockers() - if err != nil { - secret.Warn("Could not list unlockers during duplicate check", "error", err) - - return nil // If we can't list unlockers, assume it doesn't exist - } - - // Get vault directory to construct unlocker instances +// checkUnlockerExists reports whether the vault already has an unlocker +// with the given ID. It returns an error, and no answer, when unlockers.d +// cannot be read; the caller must then not create the unlocker. +func (cli *Instance) checkUnlockerExists( + vlt *vault.Vault, unlockerID string, +) (bool, error) { vaultDir, err := vlt.GetDirectory() if err != nil { - secret.Warn("Could not get vault directory during duplicate check", - "error", err) - - return nil + return false, fmt.Errorf("failed to get vault directory: %w", err) } - // Check each unlocker's ID unlockersDir := filepath.Join(vaultDir, "unlockers.d") + unlockers, err := vlt.ListUnlockers() + if err != nil { + return false, fmt.Errorf( + "failed to list unlockers in %s: %w", unlockersDir, err, + ) + } + for _, metadata := range unlockers { // Construct the unlocker matching this metadata to get its ID id, err := findUnlockerIDByMetadata(cli.fs, unlockersDir, metadata, true) if err != nil { - secret.Warn( - "Could not read unlockers directory during duplicate check, "+ - "skipping unlocker", - "unlockers_dir", unlockersDir, "error", err) - - continue + // Unlike `unlocker list`, never skip here: a skipped entry may be the duplicate. + return false, err } if id != "" && id == unlockerID { - return errUnlockerExists + return true, nil } } - return nil + return false, nil } diff --git a/internal/cli/unreadable_dir_test.go b/internal/cli/unreadable_dir_test.go new file mode 100644 index 0000000..d1dc600 --- /dev/null +++ b/internal/cli/unreadable_dir_test.go @@ -0,0 +1,262 @@ +// Unreadable Directory Tests +// +// 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. +// When that look fails they must refuse to act, not read the failure as +// "nothing there" and go ahead. + +//nolint:testpackage // white-box test of unexported internals +package cli + +import ( + "context" + "errors" + "io" + "os" + "os/exec" + "path/filepath" + "testing" + "time" + + "git.eeqj.de/sneak/secret/internal/secret" + "github.com/spf13/afero" + "github.com/spf13/cobra" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const ( + // unreadableTestGPGUserID is the user ID of the throwaway GPG key the + // PGP unlocker tests generate, and the --keyid they pass. + unreadableTestGPGUserID = "unlocker-test@example.com" + + // unreadableTestSecretName is the secret stored in the vaults the + // removal tests remove from. + unreadableTestSecretName = "api-key" + + // unreadableTestOtherVault is a second vault for the vault removal + // test, since the last vault can never be removed. + unreadableTestOtherVault = "work" + + // unreadableTestSecretsDirName is the directory holding a vault's + // secrets, and unreadableTestCurrentFileName the per-secret file + // naming its current version. + unreadableTestSecretsDirName = "secrets.d" + unreadableTestCurrentFileName = "current" +) + +// errStatFailed is returned by statFailFs in place of a successful stat. +var errStatFailed = errors.New("input/output error") + +// statFailFs fails every Stat of one path, as an I/O or permission error +// on that path would. +type statFailFs struct { + afero.Fs + + path string +} + +func (f *statFailFs) Stat(name string) (os.FileInfo, error) { + if name == f.path { + return nil, errStatFailed + } + + return f.Fs.Stat(name) +} + +// testVaultDir returns the directory of the named vault in the synthetic +// state directory built by newListTestVault. +func testVaultDir(vaultName string) string { + return filepath.Join(listTestStateDir, "vaults.d", vaultName) +} + +// newTestInstance returns a CLI instance on fs whose output is discarded. +func newTestInstance(fs afero.Fs) (*Instance, *cobra.Command) { + cmd := &cobra.Command{} + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + + return &Instance{fs: fs, stateDir: listTestStateDir, cmd: cmd}, cmd +} + +// assertDirEntries asserts that dir holds exactly the named entries. +func assertDirEntries(t *testing.T, fs afero.Fs, dir string, want ...string) { + t.Helper() + + entries, err := afero.ReadDir(fs, dir) + require.NoError(t, err) + + names := make([]string, 0, len(entries)) + for _, entry := range entries { + names = append(names, entry.Name()) + } + + assert.ElementsMatch(t, want, names) +} + +// newTestGPGKey points GNUPGHOME at a fresh directory, generates a GPG key +// without a passphrase there, and returns the key's fingerprint. +func newTestGPGKey(t *testing.T) string { + t.Helper() + + t.Setenv("GNUPGHOME", t.TempDir()) + + t.Cleanup(func() { + // Stop the gpg-agent that key generation starts. t.Context is + // already canceled when cleanup runs. + ctx := context.WithoutCancel(t.Context()) + _ = exec.CommandContext(ctx, "gpgconf", "--kill", "gpg-agent").Run() + }) + + output, err := exec.CommandContext(t.Context(), "gpg", "--batch", + "--pinentry-mode", "loopback", "--passphrase", "", + "--quick-gen-key", unreadableTestGPGUserID, "ed25519", "sign", "never", + ).CombinedOutput() + require.NoError(t, err, "generating the test GPG key: %s", output) + + fingerprint, err := secret.ResolveGPGKeyFingerprint(unreadableTestGPGUserID) + require.NoError(t, err) + + return fingerprint +} + +// addTestPGPUnlocker runs `secret unlocker add pgp` for the test key +// against fs. +func addTestPGPUnlocker(fs afero.Fs) error { + instance, cmd := newTestInstance(fs) + cmd.Flags().String("keyid", unreadableTestGPGUserID, "") + + return instance.addPGPUnlocker(cmd) +} + +// TestAddPGPUnlockerDuplicateCheck asserts that adding a PGP unlocker +// fails, and creates no unlocker directory, when unlockers.d cannot be +// read for the duplicate check; and, as the control case, that a readable +// unlockers.d holding the same key is still refused as a duplicate. +// +//nolint:paralleltest // t.Setenv (GNUPGHOME) forbids parallel tests +func TestAddPGPUnlockerDuplicateCheck(t *testing.T) { + fingerprint := newTestGPGKey(t) + unlockersDir := filepath.Join( + testVaultDir(listTestVaultName), listTestUnlockersDirName) + + tests := []struct { + name string + openBudget int + }{ + // The vault's own enumeration of unlockers.d fails. + {name: "listing fails", openBudget: 0}, + // The enumeration succeeds; the rescan that resolves IDs fails. + {name: "rescan fails", openBudget: 1}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + base := newListTestVault(t, 1) + fs := &unlockersDirFailFs{Fs: base, openBudget: tt.openBudget} + + err := addTestPGPUnlocker(fs) + + require.ErrorIs(t, err, errUnlockersDirUnreadable) + require.NotErrorIs(t, err, errGPGKeyAlreadyUnlocker) + assert.Contains(t, err.Error(), unlockersDir, + "the error must name the directory it could not read") + assertDirEntries(t, base, unlockersDir, listTestUnlockerDirOne) + }) + } + + t.Run("duplicate refused", func(t *testing.T) { + base := newListTestVault(t, 1) + writePGPUnlocker(t, base, unlockersDir, listTestUnlockerDirTwo, + time.Date(2026, time.August, 10, 12, 30, 0, 0, time.UTC), + fingerprint) + + err := addTestPGPUnlocker(base) + + require.ErrorIs(t, err, errGPGKeyAlreadyUnlocker) + assertDirEntries(t, base, unlockersDir, + listTestUnlockerDirOne, listTestUnlockerDirTwo) + }) +} + +// writeTestSecret stores a secret with a current-version pointer, which is +// what makes it count as a secret, in the given vault directory. +func writeTestSecret(t *testing.T, fs afero.Fs, vaultDir string) { + t.Helper() + + secretDir := filepath.Join( + vaultDir, unreadableTestSecretsDirName, unreadableTestSecretName) + require.NoError(t, fs.MkdirAll(secretDir, listTestDirPerm)) + require.NoError(t, afero.WriteFile(fs, + filepath.Join(secretDir, unreadableTestCurrentFileName), + []byte("20260809.001"), listTestFilePerm)) +} + +// TestRemoveLastUnlockerAbortsWhenSecretsUnreadable asserts that the last +// unlocker is kept when the secrets it protects cannot be counted. +func TestRemoveLastUnlockerAbortsWhenSecretsUnreadable(t *testing.T) { + t.Parallel() + + vaultDir := testVaultDir(listTestVaultName) + unlockersDir := filepath.Join(vaultDir, listTestUnlockersDirName) + secretsDir := filepath.Join(vaultDir, unreadableTestSecretsDirName) + + for _, path := range []string{ + secretsDir, + filepath.Join(secretsDir, unreadableTestSecretName, + unreadableTestCurrentFileName), + } { + t.Run(filepath.Base(path), func(t *testing.T) { + t.Parallel() + + base := newListTestVault(t, 1) + writeTestSecret(t, base, vaultDir) + instance, cmd := newTestInstance(&statFailFs{Fs: base, path: path}) + + err := instance.UnlockersRemove( + "pgp-"+listTestGPGKeyID+"A", false, cmd) + + require.ErrorIs(t, err, errStatFailed) + assertDirEntries(t, base, unlockersDir, listTestUnlockerDirOne) + }) + } +} + +// TestRemoveVaultAbortsWhenSecretsDirUnreadable asserts that a vault is +// kept when whether it holds secrets cannot be determined. +func TestRemoveVaultAbortsWhenSecretsDirUnreadable(t *testing.T) { + t.Parallel() + + base := newListTestVault(t, 1) + vaultDir := testVaultDir(unreadableTestOtherVault) + writeTestSecret(t, base, vaultDir) + instance, cmd := newTestInstance(&statFailFs{ + Fs: base, path: filepath.Join(vaultDir, unreadableTestSecretsDirName), + }) + + err := instance.RemoveVault(cmd, unreadableTestOtherVault, false) + + require.ErrorIs(t, err, errStatFailed) + + exists, err := afero.DirExists(base, vaultDir) + require.NoError(t, err) + assert.True(t, exists, "the vault must not be removed") +} + +// TestVaultImportAbortsWhenPubKeyUnreadable asserts that a mnemonic import +// stops when whether the vault already has a long-term key cannot be +// determined. +func TestVaultImportAbortsWhenPubKeyUnreadable(t *testing.T) { + t.Parallel() + + base := newListTestVault(t, 1) + instance, cmd := newTestInstance(&statFailFs{ + Fs: base, path: filepath.Join(testVaultDir(listTestVaultName), "pub.age"), + }) + + err := instance.VaultImport(cmd, listTestVaultName) + + require.ErrorIs(t, err, errStatFailed) +} diff --git a/internal/cli/vault.go b/internal/cli/vault.go index 63781d4..b8533b6 100644 --- a/internal/cli/vault.go +++ b/internal/cli/vault.go @@ -388,8 +388,12 @@ func (cli *Instance) vaultImportPreflight( // Check if vault already has a public key pubKeyPath := vaultDir + "/pub.age" - _, err = cli.fs.Stat(pubKeyPath) - if err == nil { + exists, err = afero.Exists(cli.fs, pubKeyPath) + if err != nil { + return "", "", "", fmt.Errorf("failed to check %s: %w", pubKeyPath, err) + } + + if exists { return "", "", "", fmt.Errorf("vault '%s' %w", vaultName, errVaultHasLongTermKey) } @@ -536,17 +540,26 @@ func (cli *Instance) VaultImport(cmd *cobra.Command, vaultName string) error { } // vaultHasSecrets reports whether the vault directory contains any secrets -func (cli *Instance) vaultHasSecrets(vaultDir string) bool { +func (cli *Instance) vaultHasSecrets(vaultDir string) (bool, error) { secretsDir := filepath.Join(vaultDir, "secrets.d") - exists, _ := afero.DirExists(cli.fs, secretsDir) + exists, err := afero.DirExists(cli.fs, secretsDir) + if err != nil { + return false, fmt.Errorf("failed to check secrets directory %s: %w", + secretsDir, err) + } + if !exists { - return false + return false, nil } entries, err := afero.ReadDir(cli.fs, secretsDir) + if err != nil { + return false, fmt.Errorf("failed to read secrets directory %s: %w", + secretsDir, err) + } - return err == nil && len(entries) > 0 + return len(entries) > 0, nil } // switchAwayFromVault selects another vault as current before removal @@ -610,7 +623,10 @@ func (cli *Instance) RemoveVault(cmd *cobra.Command, name string, force bool) er } // Check if vault has secrets - hasSecrets := cli.vaultHasSecrets(vaultDir) + hasSecrets, err := cli.vaultHasSecrets(vaultDir) + if err != nil { + return err + } // Require --force if vault has secrets if hasSecrets && !force { diff --git a/internal/vault/vault.go b/internal/vault/vault.go index d28f36b..bc8b34b 100644 --- a/internal/vault/vault.go +++ b/internal/vault/vault.go @@ -138,7 +138,12 @@ func (v *Vault) NumSecrets() (int, error) { secretsDir := filepath.Join(vaultDir, "secrets.d") - exists, _ := afero.DirExists(v.fs, secretsDir) + exists, err := afero.DirExists(v.fs, secretsDir) + if err != nil { + return 0, fmt.Errorf("failed to check secrets directory %s: %w", + secretsDir, err) + } + if !exists { return 0, nil } @@ -162,7 +167,7 @@ func (v *Vault) NumSecrets() (int, error) { exists, err := afero.Exists(v.fs, currentFile) if err != nil { - continue // Skip directories we can't read + return 0, fmt.Errorf("failed to check %s: %w", currentFile, err) } if exists {