From 43f66bf36995d3221f248cea14a99e08f1ae714c Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Mon, 5 Oct 2026 01:08:01 +0200 Subject: [PATCH] Give each failure one error value (closes #113) internal/cli drops its copies of vault.ErrSecretNotFound, ErrVaultNotFound, ErrVersionNotFound and ErrSecretExists and of the secret package's keychain and Secure Enclave errors, and its second error for an unknown unlocker type, an invalid mnemonic, a length below 1, an unsupported secret type and an oversized secret. vault.ErrNilValueBuffer becomes secret.ErrNilValueBuffer. Every error of secret.ReadPassphrase wraps ErrPassphraseNotRead. ResolveGPGKeyFingerprint returns ErrGPGKeyNotFound for a key the keyring lacks. storeInKeychain returns errNilDataBuffer. bip85's ErrPasswordTooShort and ErrEncodedTooShort go with their unreachable checks, as does the macOS check in macOS-only code. Tests that matched these errors' text use errors.Is. Model: opus-5-5 --- TODO.md | 37 +++++++++++++ internal/cli/create_vault_test.go | 2 +- internal/cli/crypto.go | 3 +- internal/cli/errors_test.go | 62 ++++++++++++++++++++++ internal/cli/generate.go | 13 ++--- internal/cli/input_errors_test.go | 67 ++++++++++++++++++++++++ internal/cli/integration_test.go | 20 +++---- internal/cli/move_test.go | 36 +++++++++---- internal/cli/path_traversal_test.go | 14 +---- internal/cli/secrets.go | 23 +++----- internal/cli/secrets_size_test.go | 2 +- internal/cli/unlockers.go | 17 +----- internal/cli/unlockers_add_test.go | 3 +- internal/cli/vault.go | 9 ++-- internal/cli/version.go | 9 ++-- internal/cli/version_test.go | 4 +- internal/secret/crypto.go | 11 ++-- internal/secret/keychainunlocker.go | 18 ------- internal/secret/keychainunlocker_cgo.go | 2 +- internal/secret/keychainunlocker_test.go | 3 +- internal/secret/passphraseunlocker.go | 8 +-- internal/secret/pgpunlocker.go | 18 ++++++- internal/secret/seunlocker_darwin.go | 5 -- internal/secret/version.go | 10 ++-- internal/vault/errors.go | 3 -- internal/vault/errors_test.go | 2 +- internal/vault/secrets.go | 2 +- pkg/bip85/bip85.go | 30 ++--------- 28 files changed, 271 insertions(+), 162 deletions(-) create mode 100644 internal/cli/errors_test.go create mode 100644 internal/cli/input_errors_test.go diff --git a/TODO.md b/TODO.md index cdbff2a..f431534 100644 --- a/TODO.md +++ b/TODO.md @@ -18,6 +18,43 @@ 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. `errUnsupportedUnlockerType` is removed: `secret + unlocker add` gives `errInvalidUnlockerType` for an unknown type, whichever + check rejects it. Off macOS, adding a keychain or Secure Enclave unlocker + returns the `secret` package's error for it, not an `internal/cli` copy; on + macOS, the check that the system is macOS is gone, as it could never fail. + `secret vault import` gives `errInvalidMnemonicPhrase` for an invalid + mnemonic, as `init` and `vault create` do. `secret generate secret` gives + `errLengthTooSmall` for a length below 1 wherever it is checked, and + `errUnsupportedSecretType` for `--type mnemonic` too. `secret import` of a + file over 100MB wraps `errSecretTooLarge`, as `secret add` returns it. + `vault.ErrNilValueBuffer` is replaced by `secret.ErrNilValueBuffer`, which + `secret` already returned under another name. Messages are unchanged, + except that `secret decrypt` of a missing secret says "not found", as + `secret get` does, not "does not exist"; `vault import` of an invalid + mnemonic says "invalid BIP39 mnemonic phrase"; `--type mnemonic` says + "unsupported type: mnemonic (use 'secret generate mnemonic' instead)"; and + a file too large to import says + `failed to read secret from file : secret too large: exceeds 100MB limit`. + 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/generate.go b/internal/cli/generate.go index c608d2c..0906f51 100644 --- a/internal/cli/generate.go +++ b/internal/cli/generate.go @@ -20,11 +20,7 @@ const ( // Sentinel errors for secret generation var ( - errLengthTooSmall = errors.New("length must be at least 1") - errLengthNotPositive = errors.New("length must be positive") - errMnemonicTypeNotSupported = errors.New( - "mnemonic type not supported for secret generation, " + - "use 'secret generate mnemonic' instead") + errLengthTooSmall = errors.New("length must be at least 1") errUnsupportedSecretType = errors.New("unsupported type") ) @@ -148,7 +144,8 @@ func (cli *Instance) GenerateSecret( case "alnum": secretValue, err = generateRandomAlnum(length) case "mnemonic": - return errMnemonicTypeNotSupported + return fmt.Errorf("%w: mnemonic (use 'secret generate mnemonic' instead)", + errUnsupportedSecretType) default: return fmt.Errorf("%w: %s (supported: base58, alnum)", errUnsupportedSecretType, secretType) @@ -204,8 +201,8 @@ func generateRandomAlnum(length int) (string, error) { // generateRandomString generates a random string of the specified length // using the given character set func generateRandomString(length int, charset string) (string, error) { - if length <= 0 { - return "", errLengthNotPositive + if length < 1 { + return "", errLengthTooSmall } result := make([]byte, length) diff --git a/internal/cli/input_errors_test.go b/internal/cli/input_errors_test.go new file mode 100644 index 0000000..a1567ce --- /dev/null +++ b/internal/cli/input_errors_test.go @@ -0,0 +1,67 @@ +//nolint:testpackage // white-box test of unexported internals +package cli + +import ( + "testing" + + "git.eeqj.de/sneak/secret/internal/vault" + "github.com/awnumar/memguard" + "github.com/spf13/afero" + "github.com/stretchr/testify/require" +) + +// TestInvalidMnemonicError checks that every command that takes a mnemonic +// returns errInvalidMnemonicPhrase for one that is not valid BIP39. The vault +// "other" has no long-term key, as vault import needs. +func TestInvalidMnemonicError(t *testing.T) { + t.Parallel() + + tests := []struct { + command string + run func(c *Instance) error + }{ + {"secret init", func(c *Instance) error { return c.Init(c.cmd) }}, + {"secret vault create work", func(c *Instance) error { + return c.CreateVault(c.cmd, "work") + }}, + {"secret vault import other", func(c *Instance) error { + return c.VaultImport(c.cmd, "other") + }}, + } + + for _, tt := range tests { + t.Run(tt.command, func(t *testing.T) { + t.Parallel() + + fs := afero.NewMemMapFs() + _, err := vault.CreateVault(fs, listTestStateDir, "other", nil, nil) + require.NoError(t, err) + + instance, _ := newTestInstance(fs) + instance.Mnemonic = memguard.NewBufferFromBytes([]byte("not a mnemonic")) + t.Cleanup(instance.Mnemonic.Destroy) + + require.ErrorIs(t, tt.run(instance), errInvalidMnemonicPhrase) + }) + } +} + +// TestGenerateSecretErrors checks that `secret generate secret` gives one +// error for a length below 1 and one for a type it cannot generate. +func TestGenerateSecretErrors(t *testing.T) { + t.Parallel() + + instance, cmd := newTestInstance(afero.NewMemMapFs()) + + err := instance.GenerateSecret(cmd, "x", 0, "base58", false) + require.ErrorIs(t, err, errLengthTooSmall) + + _, err = generateRandomString(0, "ab") + require.ErrorIs(t, err, errLengthTooSmall) + + err = instance.GenerateSecret(cmd, "x", defaultSecretLength, "mnemonic", false) + require.ErrorIs(t, err, errUnsupportedSecretType) + + err = instance.GenerateSecret(cmd, "x", defaultSecretLength, "hex", false) + require.ErrorIs(t, err, errUnsupportedSecretType) +} 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..440bbf2 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,33 @@ func TestRejectedMoveWithinVaultLeavesStateUnchanged(t *testing.T) { }) } + missing := []struct { + command string + source, dest string + force bool + want error + }{ + { + "mv work:nosuch work:y", "work:nosuch", "work:y", false, + vault.ErrSecretNotFound, + }, + // Only an existing vault is used. + { + "mv --force nosuch:x nosuch:y", "nosuch:x", "nosuch:y", true, + vault.ErrVaultNotFound, + }, + } + + for _, tt := range missing { + t.Run(tt.command, func(t *testing.T) { + t.Parallel() + + requireRejectedAndUnchanged(t, before, tt.want, func(c *cli.Instance) error { + return c.MoveSecret(&cobra.Command{}, tt.source, tt.dest, tt.force) + }) + }) + } + // 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..cd598cd 100644 --- a/internal/cli/secrets.go +++ b/internal/cli/secrets.go @@ -32,13 +32,7 @@ const ( // Sentinel errors for secret operations 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") + errSecretTooLarge = errors.New("secret too large: exceeds 100MB limit") errCrossVaultSourceUnqualified = errors.New( "source must specify vault (e.g., vault:secret) for cross-vault move") errMoveOntoItself = errors.New("cannot be moved onto itself") @@ -673,10 +667,6 @@ func (cli *Instance) ImportSecret( buffers, totalSize, err := readSecretFromReader(file) if err != nil { - if errors.Is(err, errSecretTooLarge) { - return errSecretFileTooLarge - } - return fmt.Errorf("failed to read secret from file %s: %w", sourceFile, err) } defer destroyBuffers(buffers) @@ -776,7 +766,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 +897,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 +928,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 +953,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 +1019,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/secrets_size_test.go b/internal/cli/secrets_size_test.go index 5c041f4..33c42f1 100644 --- a/internal/cli/secrets_size_test.go +++ b/internal/cli/secrets_size_test.go @@ -289,7 +289,7 @@ func TestImportSecretVariousSizes(t *testing.T) { { name: "101MB file - should fail", size: 101 * 1024 * 1024, - wantErr: errSecretFileTooLarge, + wantErr: errSecretTooLarge, }, } diff --git a/internal/cli/unlockers.go b/internal/cli/unlockers.go index 1c07dd5..5373ba2 100644 --- a/internal/cli/unlockers.go +++ b/internal/cli/unlockers.go @@ -39,15 +39,10 @@ var ( errInvalidUnlockerType = errors.New("invalid unlocker type") errKeyIDOnlyForPGP = errors.New( "--keyid flag is only valid for PGP unlockers") - errKeychainMacOSOnly = errors.New( - "keychain unlockers are only supported on macOS") - errSecureEnclaveMacOSOnly = errors.New( - "secure enclave unlockers are only supported on macOS") // errGPGKeyAlreadyUnlocker carries only the message tail; the caller // composes "GPG key is already added as an unlocker". errGPGKeyAlreadyUnlocker = errors.New( "is already added as an unlocker") - errUnsupportedUnlockerType = errors.New("unsupported unlocker type") ) // UnlockerInfo represents unlocker information for display @@ -439,7 +434,7 @@ func (cli *Instance) UnlockersAdd(unlockerType string, cmd *cobra.Command) error } return fmt.Errorf("%w: %s (supported: %s)", - errUnsupportedUnlockerType, unlockerType, supportedTypes) + errInvalidUnlockerType, unlockerType, supportedTypes) } } @@ -474,7 +469,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() } @@ -494,10 +489,6 @@ func (cli *Instance) addPassphraseUnlocker(cmd *cobra.Command) error { // addKeychainUnlocker creates a macOS Keychain unlocker in the current vault func (cli *Instance) addKeychainUnlocker(cmd *cobra.Command) error { - if runtime.GOOS != platformDarwin { - return errKeychainMacOSOnly - } - keychainUnlocker, err := secret.CreateKeychainUnlocker( cli.fs, cli.stateDir, cli.Mnemonic, cli.UnlockPassphrase) if err != nil { @@ -525,10 +516,6 @@ func (cli *Instance) addKeychainUnlocker(cmd *cobra.Command) error { // addSecureEnclaveUnlocker creates a Secure Enclave unlocker in the // current vault func (cli *Instance) addSecureEnclaveUnlocker(cmd *cobra.Command) error { - if runtime.GOOS != platformDarwin { - return errSecureEnclaveMacOSOnly - } - seUnlocker, err := secret.CreateSecureEnclaveUnlocker( cli.fs, cli.stateDir, cli.Mnemonic, cli.UnlockPassphrase) if err != nil { 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..4d5b7b4 100644 --- a/internal/cli/vault.go +++ b/internal/cli/vault.go @@ -23,7 +23,6 @@ import ( var ( errMnemonicEmpty = errors.New("mnemonic cannot be empty") errInvalidMnemonicPhrase = errors.New("invalid BIP39 mnemonic phrase") - errInvalidMnemonic = errors.New("invalid BIP39 mnemonic") errVaultHasLongTermKey = errors.New( "already has a long-term key configured") errMnemonicEnvNotSet = errors.New( @@ -250,7 +249,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 +352,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 @@ -381,7 +380,7 @@ func (cli *Instance) vaultImportPreflight( secret.Debug("Validating BIP39 mnemonic", "word_count", len(mnemonicWords)) if !bip39.IsMnemonicValid(mnemonic) { - return "", "", "", errInvalidMnemonic + return "", "", "", errInvalidMnemonicPhrase } return vaultDir, pubKeyPath, mnemonic, nil @@ -644,7 +643,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.go b/internal/secret/keychainunlocker.go index 2a62102..8a85d63 100644 --- a/internal/secret/keychainunlocker.go +++ b/internal/secret/keychainunlocker.go @@ -11,7 +11,6 @@ import ( "os" "path/filepath" "regexp" - "runtime" "time" "filippo.io/age" @@ -39,8 +38,6 @@ const ( var keychainItemNameRegex = regexp.MustCompile(`^[A-Za-z0-9._-]+$`) var ( - errNotMacOS = errors.New( - "keychain unlockers are only supported on macOS") errKeychainItemNameEmpty = errors.New("keychain item name cannot be empty") errInvalidKeychainItemName = errors.New("invalid keychain item name format") errUnsupportedCurrentUnlocker = errors.New( @@ -394,12 +391,6 @@ func deriveLongTermPrivateKey( func CreateKeychainUnlocker( fs afero.Fs, stateDir string, mnemonic, passphrase *memguard.LockedBuffer, ) (*KeychainUnlocker, error) { - // Check if we're on macOS - err := checkMacOSAvailable() - if err != nil { - return nil, err - } - // Get current vault using the GetCurrentVault function from the same package vault, err := GetCurrentVault(fs, stateDir) if err != nil { @@ -555,15 +546,6 @@ func writeKeychainUnlocker( }, nil } -// checkMacOSAvailable verifies that we're running on macOS -func checkMacOSAvailable() error { - if runtime.GOOS != "darwin" { - return fmt.Errorf("%w, current OS: %s", errNotMacOS, runtime.GOOS) - } - - return nil -} - // validateKeychainItemName validates that a keychain item name is safe for // command execution func validateKeychainItemName(itemName string) error { 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/internal/secret/seunlocker_darwin.go b/internal/secret/seunlocker_darwin.go index f90ed30..601923e 100644 --- a/internal/secret/seunlocker_darwin.go +++ b/internal/secret/seunlocker_darwin.go @@ -216,11 +216,6 @@ func CreateSecureEnclaveUnlocker( stateDir string, mnemonic, passphrase *memguard.LockedBuffer, ) (*SecureEnclaveUnlocker, error) { - err := checkMacOSAvailable() - if err != nil { - return nil, err - } - vault, err := GetCurrentVault(fs, stateDir) if err != nil { return nil, fmt.Errorf("failed to get current vault: %w", err) diff --git a/internal/secret/version.go b/internal/secret/version.go index fe9fd0a..16e4159 100644 --- a/internal/secret/version.go +++ b/internal/secret/version.go @@ -22,10 +22,10 @@ const ( maxVersionsPerDay = 999 ) -var ( - errMaxVersionsPerDay = errors.New("exceeded maximum versions per day (999)") - errNilValueBuffer = errors.New("value buffer is nil") -) +var errMaxVersionsPerDay = errors.New("exceeded maximum versions per day (999)") + +// ErrNilValueBuffer is returned when a secret's value is given as nil. +var ErrNilValueBuffer = errors.New("value buffer is nil") // VersionMetadata contains information about a secret version type VersionMetadata struct { @@ -138,7 +138,7 @@ func GenerateVersionName(fs afero.Fs, secretDir string) (string, error) { // process dies part-way. func (sv *Version) Save(value *memguard.LockedBuffer) error { if value == nil { - return errNilValueBuffer + return ErrNilValueBuffer } DebugWith("Saving secret version", diff --git a/internal/vault/errors.go b/internal/vault/errors.go index 32805d1..6efa625 100644 --- a/internal/vault/errors.go +++ b/internal/vault/errors.go @@ -36,9 +36,6 @@ var ( // it unlocks. Composed as "vault needs a mnemonic for an unlocker". ErrUnlockerWithoutMnemonic = errors.New("needs a mnemonic for an unlocker") - // ErrNilValueBuffer indicates a nil value buffer was supplied. - ErrNilValueBuffer = errors.New("value buffer is nil") - // ErrInvalidSecretName indicates a secret name that breaks the naming // rule: only ASCII letters, digits, '.', '-', '_' and '/'; not empty; // no leading '.' or '/', no trailing '/', no '//', no '..' path segment. diff --git a/internal/vault/errors_test.go b/internal/vault/errors_test.go index b8c2b44..0dfd3bd 100644 --- a/internal/vault/errors_test.go +++ b/internal/vault/errors_test.go @@ -60,7 +60,7 @@ func TestVaultErrors(t *testing.T) { }, vault.ErrVaultNotFound}, {"add a nil value", func(vlt *vault.Vault) error { return vlt.AddSecret(missingName, nil, false) - }, vault.ErrNilValueBuffer}, + }, secret.ErrNilValueBuffer}, {"get a missing secret", func(vlt *vault.Vault) error { _, err := vlt.GetSecret(missingName) diff --git a/internal/vault/secrets.go b/internal/vault/secrets.go index 3faeb47..8cfe14e 100644 --- a/internal/vault/secrets.go +++ b/internal/vault/secrets.go @@ -130,7 +130,7 @@ func ValidateSecretName(name string) error { // AddSecret adds a secret to this vault func (v *Vault) AddSecret(name string, value *memguard.LockedBuffer, force bool) error { if value == nil { - return ErrNilValueBuffer + return secret.ErrNilValueBuffer } secret.DebugWith("Adding secret to vault", 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 }