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 }