From a83743383e77a2b5590c100a8069d45614a46acd Mon Sep 17 00:00:00 2001 From: sneak Date: Sun, 4 Oct 2026 21:39:24 +0000 Subject: [PATCH] Give each failure one error value (closes #113) internal/cli's copies of vault.ErrSecretNotFound, ErrVaultNotFound, ErrVersionNotFound and ErrSecretExists are removed; the commands wrap the vault errors. Every error of secret.ReadPassphrase wraps ErrPassphraseNotRead, so its callers no longer add those words. ResolveGPGKeyFingerprint returns ErrGPGKeyNotFound for a key the keyring lacks, recognised by gpg's status line. storeInKeychain returns errNilDataBuffer. bip85's ErrPasswordTooShort and ErrEncodedTooShort go with their unreachable checks. Tests that matched these errors' text use errors.Is. Model: opus-5-5 --- TODO.md | 21 ++++++++ internal/cli/create_vault_test.go | 2 +- internal/cli/crypto.go | 3 +- internal/cli/errors_test.go | 62 ++++++++++++++++++++++++ internal/cli/integration_test.go | 20 +++----- internal/cli/move_test.go | 23 +++++---- internal/cli/path_traversal_test.go | 14 +----- internal/cli/secrets.go | 15 +++--- internal/cli/unlockers.go | 2 +- internal/cli/unlockers_add_test.go | 3 +- internal/cli/vault.go | 6 +-- internal/cli/version.go | 9 ++-- internal/cli/version_test.go | 4 +- internal/secret/crypto.go | 11 +++-- internal/secret/keychainunlocker_cgo.go | 2 +- internal/secret/keychainunlocker_test.go | 3 +- internal/secret/passphraseunlocker.go | 8 +-- internal/secret/pgpunlocker.go | 18 ++++++- pkg/bip85/bip85.go | 30 ++---------- 19 files changed, 159 insertions(+), 97 deletions(-) create mode 100644 internal/cli/errors_test.go diff --git a/TODO.md b/TODO.md index cdbff2a..a407d50 100644 --- a/TODO.md +++ b/TODO.md @@ -18,6 +18,27 @@ https://git.eeqj.de/sneak/secret/milestone/12 # Completed Steps +- 2026-10-04: A failure returns the same error value whichever command hits + it (https://git.eeqj.de/sneak/secret/issues/113). `internal/cli` no longer + keeps its own copies of `vault.ErrSecretNotFound`, `ErrVaultNotFound`, + `ErrVersionNotFound` and `ErrSecretExists`: `secret mv`, `rm`, `decrypt`, + `vault import`, `vault remove` and `version list`, `promote` and `rm` wrap + the `vault` errors. Messages are unchanged, except that `secret decrypt` + of a missing secret says "not found", as `secret get` does, not "does not + exist". Every error of `secret.ReadPassphrase` wraps + `secret.ErrPassphraseNotRead`, which supplies the words "failed to read + passphrase" that its callers used to add themselves; so two passphrases + that differ now give only "passphrases do not match", the words now follow + "failed to read mnemonic:" and "failed to read passphrase confirmation:", + and a terminal read error no longer repeats them. A GPG key the keyring + does not hold gives `secret.ErrGPGKeyNotFound`, found by gpg's status line + for "No public key"; before, the message repeated "failed to resolve GPG + key fingerprint" and ended in gpg's exit status. The keychain unlocker + returns `errNilDataBuffer` for nil data; this and its test build only on + macOS with cgo and were only read. `bip85.ErrPasswordTooShort` and + `ErrEncodedTooShort` are removed with their checks: 64 bytes of entropy + always give 86 Base64 or 80 Base85 characters, the most a password length + may ask for. Tests that matched these errors' text use `errors.Is`. - 2026-10-04: Tests check which error a failure returns with `errors.Is`, not by matching words of its message (https://git.eeqj.de/sneak/secret/issues/49). Every exported error that diff --git a/internal/cli/create_vault_test.go b/internal/cli/create_vault_test.go index d590a3c..40f3111 100644 --- a/internal/cli/create_vault_test.go +++ b/internal/cli/create_vault_test.go @@ -188,7 +188,7 @@ func TestStopAtPassphrasePromptLeavesNothing(t *testing.T) { err := tt.run(c) - require.ErrorContains(t, err, "failed to read passphrase") + require.ErrorIs(t, err, secret.ErrPassphraseNotRead) require.Equal(t, before, snapshotStateDir(t, tt.fs)) }) } diff --git a/internal/cli/crypto.go b/internal/cli/crypto.go index b420ac3..cecc73f 100644 --- a/internal/cli/crypto.go +++ b/internal/cli/crypto.go @@ -17,7 +17,6 @@ import ( var ( errNotAgeSecretKey = errors.New( "does not contain a valid age secret key") - errSecretDoesNotExist = errors.New("does not exist") ) // newCryptoCmd builds an encrypt/decrypt command with input/output flags @@ -245,7 +244,7 @@ func (cli *Instance) Decrypt(secretName, inputFile, outputFile string) error { } if !exists { - return fmt.Errorf("secret '%s' %w", secretName, errSecretDoesNotExist) + return fmt.Errorf("secret '%s' %w", secretName, vault.ErrSecretNotFound) } // Get the age secret key from the secret diff --git a/internal/cli/errors_test.go b/internal/cli/errors_test.go new file mode 100644 index 0000000..4e9bb6a --- /dev/null +++ b/internal/cli/errors_test.go @@ -0,0 +1,62 @@ +package cli_test + +import ( + "testing" + + "git.eeqj.de/sneak/secret/internal/cli" + "git.eeqj.de/sneak/secret/internal/vault" + "github.com/spf13/cobra" +) + +// TestMissingSecretOrVaultErrors checks that a command that finds no such +// secret or vault returns the vault package's error for it, as `secret get` +// does, and leaves the vaults unchanged. "default" is the current vault, and +// both vaults hold the secret "x". +func TestMissingSecretOrVaultErrors(t *testing.T) { + t.Parallel() + + before := snapshotStateDir(t, newTwoVaultFs(t)) + + tests := []struct { + command string + want error + run func(c *cli.Instance) error + }{ + { + "rm --force nosuch", vault.ErrSecretNotFound, + func(c *cli.Instance) error { + return c.RemoveSecret(&cobra.Command{}, "nosuch", true) + }, + }, + { + "version rm --force nosuch", vault.ErrSecretNotFound, + func(c *cli.Instance) error { + return c.RemoveVersion(&cobra.Command{}, "nosuch", "20260101.001", true) + }, + }, + { + "mv --force work:nosuch default", vault.ErrSecretNotFound, + func(c *cli.Instance) error { + return c.MoveSecret(&cobra.Command{}, "work:nosuch", "default", true) + }, + }, + { + "decrypt nosuch", vault.ErrSecretNotFound, + func(c *cli.Instance) error { return c.Decrypt("nosuch", "", "") }, + }, + { + "vault rm --force nosuch", vault.ErrVaultNotFound, + func(c *cli.Instance) error { + return c.RemoveVault(&cobra.Command{}, "nosuch", true) + }, + }, + } + + for _, tt := range tests { + t.Run(tt.command, func(t *testing.T) { + t.Parallel() + + requireRejectedAndUnchanged(t, before, tt.want, tt.run) + }) + } +} diff --git a/internal/cli/integration_test.go b/internal/cli/integration_test.go index 17d580c..1c861f1 100644 --- a/internal/cli/integration_test.go +++ b/internal/cli/integration_test.go @@ -1216,9 +1216,8 @@ func test12bMoveSecret(t *testing.T, testMnemonic string, runSecret func(...stri // Test error cases // Try to move non-existent secret - output, err = runSecret("move", "test/nonexistent", "test/destination") - require.Error(t, err, "move non-existent should fail") - assert.Contains(t, output, "not found", "should indicate source not found") + _, err = runSecret("move", "test/nonexistent", "test/destination") + require.ErrorIs(t, err, vault.ErrSecretNotFound, "move non-existent should fail") // Try to move to existing destination _, err = runSecretWithStdin("dest-value", map[string]string{ @@ -1226,9 +1225,8 @@ func test12bMoveSecret(t *testing.T, testMnemonic string, runSecret func(...stri }, "add", "test/existing-dest") require.NoError(t, err, "add test/existing-dest should succeed") - output, err = runSecret("move", "test/renamed", "test/existing-dest") - require.Error(t, err, "move to existing destination should fail") - assert.Contains(t, output, "already exists", "should indicate destination exists") + _, err = runSecret("move", "test/renamed", "test/existing-dest") + require.ErrorIs(t, err, vault.ErrSecretExists, "move to existing destination should fail") // Verify the source wasn't removed since move failed getOutput, err = runSecretWithEnv(map[string]string{ @@ -1933,12 +1931,11 @@ func test23ErrorHandling(t *testing.T, tempDir, secretPath, testMnemonic string, // Import to non-existent vault with test passphrase testPassphrase := "test-passphrase-123" // Define testPassphrase locally - output, err := runSecretWithEnv(map[string]string{ + _, err = runSecretWithEnv(map[string]string{ secret.EnvMnemonic: testMnemonic, secret.EnvUnlockPassphrase: testPassphrase, }, "vault", "import", "nonexistent") - require.Error(t, err, "import to non-existent vault should fail") - assert.Contains(t, output, "does not exist", "should indicate vault doesn't exist") + require.ErrorIs(t, err, vault.ErrVaultNotFound, "import to non-existent vault should fail") // Get specific version that doesn't exist _, err = runSecretWithEnv(map[string]string{ @@ -1947,11 +1944,10 @@ func test23ErrorHandling(t *testing.T, tempDir, secretPath, testMnemonic string, require.ErrorIs(t, err, vault.ErrVersionNotFound, "get non-existent version should fail") // Promote non-existent version - output, err = runSecretWithEnv(map[string]string{ + _, err = runSecretWithEnv(map[string]string{ secret.EnvMnemonic: testMnemonic, }, "version", "promote", "database/password", "99999999.999") - require.Error(t, err, "promote non-existent version should fail") - assert.Contains(t, output, "not found", "should indicate version not found") + require.ErrorIs(t, err, vault.ErrVersionNotFound, "promote non-existent version should fail") } func test24EnvironmentVariables(t *testing.T, tempDir, secretPath, testMnemonic, testPassphrase string) { diff --git a/internal/cli/move_test.go b/internal/cli/move_test.go index 1075946..434b1d7 100644 --- a/internal/cli/move_test.go +++ b/internal/cli/move_test.go @@ -45,15 +45,6 @@ func TestRejectedMoveWithinVaultLeavesStateUnchanged(t *testing.T) { {`mv --force work:x ""`, workX, "", true, ontoItself}, // "work" is a vault name, so the destination is work:x. {"mv --force work:x work", workX, "work", true, ontoItself}, - { - "mv work:nosuch work:y", "work:nosuch", "work:y", false, - "secret 'nosuch' not found", - }, - // Only an existing vault is used. - { - "mv --force nosuch:x nosuch:y", "nosuch:x", "nosuch:y", true, - "vault 'nosuch' does not exist", - }, } for _, tt := range tests { @@ -70,6 +61,20 @@ func TestRejectedMoveWithinVaultLeavesStateUnchanged(t *testing.T) { }) } + // A missing secret, and a missing vault: only an existing vault is used. + for source, want := range map[string]error{ + "work:nosuch": vault.ErrSecretNotFound, + "nosuch:x": vault.ErrVaultNotFound, + } { + t.Run("mv --force "+source+" work:y", func(t *testing.T) { + t.Parallel() + + requireRejectedAndUnchanged(t, before, want, func(c *cli.Instance) error { + return c.MoveSecret(&cobra.Command{}, source, "work:y", true) + }) + }) + } + // Each of these spells "work" a second way. The spelling is not a valid // vault name, so the move is not taken for a move between two vaults, // which would delete the destination, here the source. diff --git a/internal/cli/path_traversal_test.go b/internal/cli/path_traversal_test.go index d69bac5..f193af3 100644 --- a/internal/cli/path_traversal_test.go +++ b/internal/cli/path_traversal_test.go @@ -297,18 +297,8 @@ func TestInvalidVersionLeavesVaultsUnchanged(t *testing.T) { for _, tt := range commands { for _, version := range []string{"", ".", "..", "../../..", "a/b"} { t.Run(fmt.Sprintf("%s %q", tt.command, version), func(t *testing.T) { - fs := newFsFromSnapshot(t, before) - - err := tt.run(cli.NewCLIInstanceWithStateDir(fs, testStateDir), version) - - require.Equal(t, before, snapshotStateDir(t, fs)) - - // Compared as text: `version rm` and `version promote` return - // internal/cli's own error of this text, which errors.Is does - // not match to vault.ErrVersionNotFound. - want := fmt.Errorf("version '%s' %w '%s'", - version, vault.ErrVersionNotFound, "x") - require.EqualError(t, err, want.Error()) + requireRejectedAndUnchanged(t, before, vault.ErrVersionNotFound, + func(c *cli.Instance) error { return tt.run(c, version) }) }) } } diff --git a/internal/cli/secrets.go b/internal/cli/secrets.go index a5fe1b1..a996b49 100644 --- a/internal/cli/secrets.go +++ b/internal/cli/secrets.go @@ -35,10 +35,6 @@ var ( errSecretTooLarge = errors.New("secret too large: exceeds 100MB limit") errSecretFileTooLarge = errors.New( "secret file too large: exceeds 100MB limit") - errSecretNotFound = errors.New("not found") - errSecretExistsNoForce = errors.New( - "already exists (use --force to overwrite)") - errVaultDoesNotExist = errors.New("does not exist") errCrossVaultSourceUnqualified = errors.New( "source must specify vault (e.g., vault:secret) for cross-vault move") errMoveOntoItself = errors.New("cannot be moved onto itself") @@ -776,7 +772,7 @@ func (cli *Instance) findSecretToRemove( if !exists { return secretToRemove{}, - fmt.Errorf("secret '%s' %w", secretName, errSecretNotFound) + fmt.Errorf("secret '%s' %w", secretName, vault.ErrSecretNotFound) } // A secret without a versions directory has no versions, and can @@ -907,7 +903,7 @@ func (cli *Instance) existingVault(name string) (*vault.Vault, error) { } if !slices.Contains(vaults, name) { - return nil, fmt.Errorf("vault '%s' %w", name, errVaultDoesNotExist) + return nil, fmt.Errorf("vault '%s' %w", name, vault.ErrVaultNotFound) } return vault.NewVault(cli.fs, cli.stateDir, name), nil @@ -938,7 +934,7 @@ func (cli *Instance) moveSecretWithinVault( } if !exists { - return fmt.Errorf("secret '%s' %w", source, errSecretNotFound) + return fmt.Errorf("secret '%s' %w", source, vault.ErrSecretNotFound) } destEncoded := strings.ReplaceAll(dest, "/", "%") @@ -963,7 +959,8 @@ func (cli *Instance) moveSecretWithinVault( if exists { if !force { - return fmt.Errorf("secret '%s' %w", dest, errSecretExistsNoForce) + return fmt.Errorf("secret '%s' %w (use --force to overwrite)", + dest, vault.ErrSecretExists) } err = secret.RemoveDirAtomic(cli.fs, destDir) @@ -1028,7 +1025,7 @@ func (cli *Instance) moveSecretCrossVault( exists, err := afero.DirExists(cli.fs, srcSecretDir) if err != nil || !exists { return fmt.Errorf("secret '%s' %w in vault '%s'", - srcSecretName, errSecretNotFound, srcVault.Name) + srcSecretName, vault.ErrSecretNotFound, srcVault.Name) } // The source is removed after the copy, so a destination that is the diff --git a/internal/cli/unlockers.go b/internal/cli/unlockers.go index 1c07dd5..0fd64fd 100644 --- a/internal/cli/unlockers.go +++ b/internal/cli/unlockers.go @@ -474,7 +474,7 @@ func (cli *Instance) addPassphraseUnlocker(cmd *cobra.Command) error { // Use secure passphrase input with confirmation passphraseBuffer, err = readSecurePassphrase("Enter passphrase for unlocker: ") if err != nil { - return fmt.Errorf("failed to read passphrase: %w", err) + return err } defer passphraseBuffer.Destroy() } diff --git a/internal/cli/unlockers_add_test.go b/internal/cli/unlockers_add_test.go index da070d6..bce3208 100644 --- a/internal/cli/unlockers_add_test.go +++ b/internal/cli/unlockers_add_test.go @@ -5,6 +5,7 @@ import ( "path/filepath" "testing" + "git.eeqj.de/sneak/secret/internal/secret" "git.eeqj.de/sneak/secret/internal/vault" "github.com/awnumar/memguard" "github.com/spf13/afero" @@ -98,7 +99,7 @@ func TestAddPGPUnlockerUnknownKey(t *testing.T) { err := instance.addPGPUnlocker(cmd) - require.ErrorContains(t, err, "failed to resolve GPG key fingerprint") + require.ErrorIs(t, err, secret.ErrGPGKeyNotFound) assertDirEntries(t, base, filepath.Join(testVaultDir(listTestVaultName), listTestUnlockersDirName), listTestUnlockerDirOne) diff --git a/internal/cli/vault.go b/internal/cli/vault.go index e3a45be..1ab2ee0 100644 --- a/internal/cli/vault.go +++ b/internal/cli/vault.go @@ -250,7 +250,7 @@ func (cli *Instance) resolvePassphrase() (*memguard.LockedBuffer, func(), error) // Use secure passphrase input with confirmation passphraseBuffer, err := readSecurePassphrase("Enter passphrase for unlocker: ") if err != nil { - return nil, nil, fmt.Errorf("failed to read passphrase: %w", err) + return nil, nil, err } return passphraseBuffer, passphraseBuffer.Destroy, nil @@ -353,7 +353,7 @@ func (cli *Instance) vaultImportPreflight( if !exists { return "", "", "", fmt.Errorf("vault '%s' %w", - vaultName, errVaultDoesNotExist) + vaultName, vault.ErrVaultNotFound) } // Check if vault already has a public key @@ -644,7 +644,7 @@ func (cli *Instance) findVaultToRemove(name string) (vaultToRemove, error) { if !slices.Contains(vaults, name) { return vaultToRemove{}, - fmt.Errorf("vault '%s' %w", name, errVaultDoesNotExist) + fmt.Errorf("vault '%s' %w", name, vault.ErrVaultNotFound) } if len(vaults) == 1 { diff --git a/internal/cli/version.go b/internal/cli/version.go index 6a5e44f..ee2cd4c 100644 --- a/internal/cli/version.go +++ b/internal/cli/version.go @@ -23,7 +23,6 @@ const ( // Sentinel errors for version operations var ( - errVersionNotFound = errors.New("not found for secret") errCannotRemoveCurrentVersion = errors.New("promote another version first") ) @@ -156,7 +155,7 @@ func (cli *Instance) ListVersions(cmd *cobra.Command, secretName string) error { if !exists { secret.Debug("Secret not found", "secret_name", secretName) - return fmt.Errorf("secret '%s' %w", secretName, errSecretNotFound) + return fmt.Errorf("secret '%s' %w", secretName, vault.ErrSecretNotFound) } // List all versions @@ -289,7 +288,7 @@ func (cli *Instance) PromoteVersion( if !exists { return fmt.Errorf("version '%s' %w '%s'", - version, errVersionNotFound, secretName) + version, vault.ErrVersionNotFound, secretName) } // Update the current symlink using the proper function @@ -374,7 +373,7 @@ func (cli *Instance) findVersionToRemove( if !exists { return versionToRemove{}, - fmt.Errorf("secret '%s' %w", secretName, errSecretNotFound) + fmt.Errorf("secret '%s' %w", secretName, vault.ErrSecretNotFound) } // Check if version exists @@ -386,7 +385,7 @@ func (cli *Instance) findVersionToRemove( if !exists { return versionToRemove{}, fmt.Errorf("version '%s' %w '%s'", - version, errVersionNotFound, secretName) + version, vault.ErrVersionNotFound, secretName) } // Get current version diff --git a/internal/cli/version_test.go b/internal/cli/version_test.go index 0afa5c2..c8d5923 100644 --- a/internal/cli/version_test.go +++ b/internal/cli/version_test.go @@ -171,7 +171,7 @@ func TestListVersionsNonExistentSecret(t *testing.T) { // Try to list versions of non-existent secret err := cli.ListVersions(cmd, "nonexistent/secret") - require.ErrorIs(t, err, errSecretNotFound) + require.ErrorIs(t, err, vault.ErrSecretNotFound) } func TestPromoteVersionCommand(t *testing.T) { @@ -265,7 +265,7 @@ func TestPromoteNonExistentVersion(t *testing.T) { // Try to promote non-existent version err = cli.PromoteVersion(cmd, "test/secret", "20991231.999") - require.ErrorIs(t, err, errVersionNotFound) + require.ErrorIs(t, err, vault.ErrVersionNotFound) } func TestGetSecretWithVersion(t *testing.T) { diff --git a/internal/secret/crypto.go b/internal/secret/crypto.go index f59dcda..5f33278 100644 --- a/internal/secret/crypto.go +++ b/internal/secret/crypto.go @@ -166,19 +166,20 @@ func DecryptWithPassphrase( // ReadPassphrase reads a passphrase securely from the terminal without echoing // This version is for unlocking and doesn't require confirmation -// Returns a LockedBuffer containing the passphrase for secure memory handling +// Returns a LockedBuffer containing the passphrase for secure memory handling. +// Every error it returns wraps ErrPassphraseNotRead. func ReadPassphrase(prompt string) (*memguard.LockedBuffer, error) { // Check if stdin is a terminal if !term.IsTerminal(syscall.Stdin) { // Not a terminal - never read passphrases from piped input // for security reasons - return nil, errStdinNotTerminal + return nil, fmt.Errorf("%w: %w", ErrPassphraseNotRead, errStdinNotTerminal) } // stdin is a terminal, check if stderr is also a terminal for // interactive prompting if !term.IsTerminal(syscall.Stderr) { - return nil, errStderrNotTerminal + return nil, fmt.Errorf("%w: %w", ErrPassphraseNotRead, errStderrNotTerminal) } // Both stdin and stderr are terminals - use secure password reading @@ -186,14 +187,14 @@ func ReadPassphrase(prompt string) (*memguard.LockedBuffer, error) { passphrase, err := term.ReadPassword(syscall.Stdin) if err != nil { - return nil, fmt.Errorf("failed to read passphrase: %w", err) + return nil, fmt.Errorf("%w: %w", ErrPassphraseNotRead, err) } // Print newline to stderr since ReadPassword doesn't echo fmt.Fprintln(os.Stderr) if len(passphrase) == 0 { - return nil, errEmptyPassphrase + return nil, fmt.Errorf("%w: %w", ErrPassphraseNotRead, errEmptyPassphrase) } // Create a secure buffer and copy the passphrase diff --git a/internal/secret/keychainunlocker_cgo.go b/internal/secret/keychainunlocker_cgo.go index 86bdfde..2751b19 100644 --- a/internal/secret/keychainunlocker_cgo.go +++ b/internal/secret/keychainunlocker_cgo.go @@ -15,7 +15,7 @@ import ( // storeInKeychain stores data in the macOS keychain using keybase/go-keychain func storeInKeychain(itemName string, data *memguard.LockedBuffer) error { if data == nil { - return fmt.Errorf("data buffer is nil") + return errNilDataBuffer } if err := validateKeychainItemName(itemName); err != nil { return fmt.Errorf("invalid keychain item name: %w", err) diff --git a/internal/secret/keychainunlocker_test.go b/internal/secret/keychainunlocker_test.go index 918e244..10551cc 100644 --- a/internal/secret/keychainunlocker_test.go +++ b/internal/secret/keychainunlocker_test.go @@ -130,8 +130,7 @@ func TestKeychainNilData(t *testing.T) { // Test storing nil data err := storeInKeychain("test-item", nil) - assert.Error(t, err, "Expected error when storing nil data") - assert.Contains(t, err.Error(), "data buffer is nil") + require.ErrorIs(t, err, errNilDataBuffer) } func TestKeychainLargeData(t *testing.T) { diff --git a/internal/secret/passphraseunlocker.go b/internal/secret/passphraseunlocker.go index 2ae7bfb..05f59ff 100644 --- a/internal/secret/passphraseunlocker.go +++ b/internal/secret/passphraseunlocker.go @@ -11,9 +11,9 @@ import ( "github.com/spf13/afero" ) -// ErrPassphraseNotRead is wrapped in the error of a passphrase unlocker -// that could not read its passphrase from the terminal, for example because -// there is none. The unlocker itself was not tried. +// ErrPassphraseNotRead is wrapped in every error of ReadPassphrase: there +// is no terminal to read the passphrase from, reading it failed, or it was +// empty. A passphrase unlocker that fails with it was not tried. var ErrPassphraseNotRead = errors.New("failed to read passphrase") // PassphraseUnlocker represents a passphrase-protected unlocker @@ -155,7 +155,7 @@ func (p *PassphraseUnlocker) getPassphrase() (*memguard.LockedBuffer, error) { if err != nil { Debug("Failed to read passphrase", "error", err, "unlocker_id", p.GetID()) - return nil, fmt.Errorf("%w: %w", ErrPassphraseNotRead, err) + return nil, err } return secureBuffer, nil diff --git a/internal/secret/pgpunlocker.go b/internal/secret/pgpunlocker.go index 5484646..eee25ad 100644 --- a/internal/secret/pgpunlocker.go +++ b/internal/secret/pgpunlocker.go @@ -18,6 +18,10 @@ import ( "github.com/spf13/afero" ) +// gpgNoPublicKeyStatus is the status line gpg writes when it has no key for +// the ID it was asked to list: 9 is gpg's error code for "No public key". +const gpgNoPublicKeyStatus = "[GNUPG:] ERROR keylist.getkey 9\n" + var ( errGPGKeyIDEmpty = errors.New("GPG key ID cannot be empty") errInvalidGPGKeyID = errors.New("invalid GPG key ID format") @@ -25,6 +29,10 @@ var ( errNilDataBuffer = errors.New("data buffer is nil") ) +// ErrGPGKeyNotFound is returned by ResolveGPGKeyFingerprint for a key ID +// that matches no key in the GPG keyring. +var ErrGPGKeyNotFound = errors.New("GPG key not found") + // Variables to allow overriding in tests var ( // GPGEncryptFunc is the function used for GPG encryption @@ -367,14 +375,20 @@ func ResolveGPGKeyFingerprint(keyID string) (string, error) { return "", fmt.Errorf("invalid GPG key ID: %w", err) } - // Use GPG to get the full fingerprint for the key + // Use GPG to get the full fingerprint for the key. --status-fd 1 adds + // gpg's status lines to the output. cmd := exec.CommandContext( //nolint:gosec // G204: keyID validated above context.Background(), - "gpg", "--list-keys", "--with-colons", "--fingerprint", keyID, + "gpg", "--status-fd", "1", + "--list-keys", "--with-colons", "--fingerprint", keyID, ) output, err := cmd.Output() if err != nil { + if strings.Contains(string(output), gpgNoPublicKeyStatus) { + return "", fmt.Errorf("%w: %s", ErrGPGKeyNotFound, keyID) + } + return "", fmt.Errorf("failed to resolve GPG key fingerprint: %w", err) } diff --git a/pkg/bip85/bip85.go b/pkg/bip85/bip85.go index c5ff617..af5ad53 100644 --- a/pkg/bip85/bip85.go +++ b/pkg/bip85/bip85.go @@ -59,16 +59,6 @@ var ( // ErrInvalidBase85PwdLen is returned when the Base85 password length // is out of range. ErrInvalidBase85PwdLen = errors.New("pwdLen must be between 10 and 80") - // ErrPasswordTooShort is returned when the derived material is - // shorter than the requested password length. It carries only the - // middle of the message, which the caller composes as - // "derived password length is shorter than requested length ", - // so the emitted text is unchanged. - ErrPasswordTooShort = errors.New("is shorter than requested length") - // ErrEncodedTooShort is returned when the encoded material is shorter - // than the requested password length. Composed as - // "encoded length is less than requested length ". - ErrEncodedTooShort = errors.New("is less than requested length") ) // Version bytes for extended keys @@ -381,14 +371,8 @@ func DeriveBase64Password( // Remove any padding encodedStr = strings.TrimRight(encodedStr, "=") - // Slice to the desired password length - if len(encodedStr) < int(pwdLen) { - return "", fmt.Errorf( - "derived password length %d %w %d", - len(encodedStr), ErrPasswordTooShort, pwdLen, - ) - } - + // Slice to the desired password length: 64 bytes of entropy leave 86 + // characters, the most pwdLen allows return encodedStr[:pwdLen], nil } @@ -411,14 +395,8 @@ func DeriveBase85Password( // Base85 encode all 64 bytes of entropy using the RFC1924 character set encoded := encodeBase85WithRFC1924Charset(entropy) - // Slice to the desired password length - if len(encoded) < int(pwdLen) { - return "", fmt.Errorf( - "encoded length %d %w %d", - len(encoded), ErrEncodedTooShort, pwdLen, - ) - } - + // Slice to the desired password length: 64 bytes of entropy give 80 + // characters, the most pwdLen allows return encoded[:pwdLen], nil }