Include gpg's stderr in the error when gpg fails (closes #146)

When gpg failed while decrypting, encrypting or looking up a key
fingerprint, the error gave only gpg's exit status, so the user could
not tell why. cmd.Output keeps gpg's stderr in the *exec.ExitError it
returns; the three errors now end with that stderr, trimmed.

gpg's stderr can run to more than one line, and the error then does too.

The regression test gives gpg data that is not an OpenPGP message
instead of one encrypted to a missing key: gpg reports both the same
way, and this case starts no gpg-agent the test would have to stop.

Model: opus-5-5

README.md

@@ -90,7 +90,8 @@ remove things pass `--force`.
 #### `secret init`
 
 Initializes the secret manager with a default vault. Prompts for a BIP39
-mnemonic phrase and creates the initial directory structure.
+mnemonic phrase and creates the initial directory structure. When the vault
+`default` already exists, it fails before asking for the mnemonic or passphrase.
 
 **Environment Variables:**
 
@@ -105,7 +106,8 @@ Lists all available vaults. The current vault is marked.
 
 #### `secret vault create <name>`
 
-Creates a new vault with the specified name.
+Creates a new vault with the specified name. When a vault of that name already
+exists, it fails before asking for the mnemonic or passphrase.
 
 **Vault Name Format:** only lowercase ASCII letters, digits, `.`, `-` and `_`
 are allowed, and a name must not be empty, `.` or `..`.
@@ -142,7 +144,13 @@ contain `//`, or have `..` as a path segment.
 
 #### `secret get <secret-name> [--version <version>]`
 
-Retrieves and outputs a secret value to stdout.
+Retrieves and outputs a secret value to stdout. A failed write of the value,
+such as to a file on a full disk, prints the error and exits 1, with two
+exceptions. If the program reading from a pipe has already exited, as in
+`secret get s | true`, the broken-pipe signal (SIGPIPE) ends `secret` with no
+message and the shell reports exit status 141. If the shell closed stdout before
+`secret` started (`secret get s >&-`), the value is written to `/dev/null` and
+`secret get` exits 0.
 
 - `--version, -v`: Get a specific version (default: current)
 

internal/cli/create_vault_test.go

@@ -222,12 +222,51 @@ func TestMnemonicNotReadNamesOnlyMnemonic(t *testing.T) {
 	output, err := cmd.CombinedOutput()
 	require.Error(t, err)
 
-	require.Equal(t, "Initialized secrets manager at: "+stateDir+"\n"+
+	require.Equal(t, "Error: failed to read mnemonic: there is no terminal to "+
-		"Error: failed to read mnemonic: there is no terminal to ask on. "+
+		"ask on. Please set the SB_SECRET_MNEMONIC environment variable or run "+
-		"Please set the SB_SECRET_MNEMONIC environment variable or run "+
 		"interactively\n", string(output))
 }
 
+// TestCreateExistingVaultAsksForNothing is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/151: `secret init` and `secret
+// vault create` asked for the mnemonic and the unlocker passphrase before
+// refusing a vault that already exists, and `secret init` printed
+// "Initialized secrets manager at" first. Each runs here as the built binary,
+// in a new session, which has no terminal, and with neither
+// SB_SECRET_MNEMONIC nor SB_UNLOCK_PASSPHRASE set, so asking for either
+// fails. Each must fail saying only that the vault exists.
+func TestCreateExistingVaultAsksForNothing(t *testing.T) {
+	t.Parallel()
+
+	stateDir := t.TempDir()
+	_, err := vault.CreateVault(afero.NewOsFs(), stateDir, "default", nil, nil)
+	require.NoError(t, err)
+
+	// run runs the built binary with args, which must fail, and returns what
+	// it printed
+	run := func(args ...string) string {
+		//nolint:gosec // G204: test executes the freshly built secret binary
+		cmd := exec.CommandContext(t.Context(), secretBinaryPath(t), args...)
+		cmd.Env = []string{
+			secret.EnvStateDir + "=" + stateDir,
+			"PATH=" + os.Getenv("PATH"),
+			"HOME=" + os.Getenv("HOME"),
+		}
+		cmd.SysProcAttr = &syscall.SysProcAttr{Setsid: true}
+
+		output, err := cmd.CombinedOutput()
+		require.Error(t, err, "secret %v", args)
+
+		return string(output)
+	}
+
+	require.Equal(t,
+		"Error: failed to create default vault: vault default already exists\n",
+		run("init"))
+	require.Equal(t, "Error: vault default already exists\n",
+		run("vault", "create", "default"))
+}
+
 // TestStopDuringCreateLeavesWholeVaultOrNone is a regression test for
 // https://git.eeqj.de/sneak/secret/issues/105: `secret init` or `secret vault
 // create` killed after the passphrase prompt but before the unlocker was

internal/cli/init.go

@@ -17,6 +17,9 @@ import (
 // errPassphraseMismatch is returned when passphrase confirmation fails
 var errPassphraseMismatch = errors.New("passphrases do not match")
 
+// defaultVaultName is the name of the vault `secret init` creates
+const defaultVaultName = "default"
+
 // NewInitCmd creates the init command
 func NewInitCmd() *cobra.Command {
 	return &cobra.Command{
@@ -92,8 +95,10 @@ func (cli *Instance) initialize(cmd *cobra.Command) error {
 		return fmt.Errorf("failed to create state directory: %w", err)
 	}
 
-	if cmd != nil {
+	// Refuse an existing default vault before asking for anything
-		cmd.Printf("Initialized secrets manager at: %s\n", stateDir)
+	err = vault.CheckNewVaultName(cli.fs, cli.stateDir, defaultVaultName)
+	if err != nil {
+		return fmt.Errorf("failed to create default vault: %w", err)
 	}
 
 	// Prompt for mnemonic
@@ -133,7 +138,7 @@ func (cli *Instance) initialize(cmd *cobra.Command) error {
 	// Create the default vault with its passphrase unlocker
 	secret.Debug("Creating default vault")
 
-	vlt, err := vault.CreateVault(cli.fs, cli.stateDir, "default",
+	vlt, err := vault.CreateVault(cli.fs, cli.stateDir, defaultVaultName,
 		mnemonic, passphraseBuffer)
 	if err != nil {
 		secret.Debug("Failed to create default vault", "error", err)
@@ -152,6 +157,7 @@ func (cli *Instance) initialize(cmd *cobra.Command) error {
 	}
 
 	if cmd != nil {
+		cmd.Printf("Initialized secrets manager at: %s\n", stateDir)
 		cmd.Printf("\nDefault vault created and configured\n")
 		cmd.Printf("Long-term public key: %s\n", ltIdentity.Recipient().String())
 		cmd.Printf("Unlocker ID: %s\n", unlocker.GetID())

internal/cli/secrets.go

@@ -443,7 +443,10 @@ func (cli *Instance) GetSecret(cmd *cobra.Command, secretName string) error {
 	defer value.Destroy()
 
 	// Write the value straight from locked memory, with no trailing newline
-	_, _ = cmd.OutOrStdout().Write(value.Bytes())
+	_, err = cmd.OutOrStdout().Write(value.Bytes())
+	if err != nil {
+		return fmt.Errorf("failed to write secret value: %w", err)
+	}
 
 	return nil
 }
@@ -478,7 +481,10 @@ func (cli *Instance) GetSecretWithVersion(
 	secret.Debug("Got secret value", "valueLength", value.Size())
 
 	// Write the value straight from locked memory, with no trailing newline
-	_, _ = cmd.OutOrStdout().Write(value.Bytes())
+	_, err = cmd.OutOrStdout().Write(value.Bytes())
+	if err != nil {
+		return fmt.Errorf("failed to write secret value: %w", err)
+	}
 
 	secret.Debug("Printed value to stdout")
 

internal/cli/vault.go

@@ -267,6 +267,12 @@ func (cli *Instance) CreateVault(cmd *cobra.Command, name string) error {
 	}
 	defer release()
 
+	// Refuse an invalid name or an existing vault before asking for anything
+	err = vault.CheckNewVaultName(cli.fs, cli.stateDir, name)
+	if err != nil {
+		return err
+	}
+
 	mnemonic, cleanupMnemonic, err := cli.promptMnemonic()
 	if err != nil {
 		return err

internal/cli/version_test.go

@@ -8,6 +8,7 @@
 // - TestPromoteNonExistentVersion: Tests error handling for invalid promotion
 // - TestGetSecretWithVersion: Tests `secret get --version` flag functionality
 // - TestGetSecretWritesBinaryValue: Tests `secret get` output of binary values
+// - TestGetSecretReturnsWriteError: Tests `secret get` when stdout fails
 // - TestVersionCommandStructure: Tests command structure and help text
 // - TestListVersionsEmptyOutput: Tests edge case with no versions
 //
@@ -20,6 +21,7 @@ package cli
 
 import (
 	"bytes"
+	"os"
 	"path/filepath"
 	"strings"
 	"testing"
@@ -360,6 +362,45 @@ func TestGetSecretWritesBinaryValue(t *testing.T) {
 	assert.Equal(t, value, buf.Bytes())
 }
 
+// TestGetSecretReturnsWriteError checks that `secret get`, with and without
+// a version, returns the error of a failed write of the value.
+func TestGetSecretReturnsWriteError(t *testing.T) {
+	t.Parallel()
+
+	fs := afero.NewMemMapFs()
+	cli := NewCLIInstanceWithStateDir(fs, testStateDir)
+	cli.Mnemonic = testMnemonicBuffer(t)
+
+	setupTestVault(t, fs)
+
+	vlt, err := vault.GetCurrentVault(fs, testStateDir)
+	require.NoError(t, err)
+
+	addTestSecret(t, vlt, []byte("value"), false)
+
+	vaultDir, err := vlt.GetDirectory()
+	require.NoError(t, err)
+
+	versions, err := secret.ListVersions(fs,
+		filepath.Join(vaultDir, "secrets.d", "test%secret"))
+	require.NoError(t, err)
+	require.Len(t, versions, 1)
+
+	// Stdout is a file closed before the value is written to it
+	out, err := os.Create(filepath.Join(t.TempDir(), "stdout"))
+	require.NoError(t, err)
+	require.NoError(t, out.Close())
+
+	cmd := newRootCmd()
+	cmd.SetOut(out)
+
+	err = cli.GetSecret(cmd, "test/secret")
+	require.ErrorIs(t, err, os.ErrClosed)
+
+	err = cli.GetSecretWithVersion(cmd, "test/secret", versions[0])
+	require.ErrorIs(t, err, os.ErrClosed)
+}
+
 //nolint:paralleltest // reads process environment to determine the state dir
 func TestVersionCommandStructure(t *testing.T) {
 	// Test that version commands are properly structured

internal/secret/pgpunlocker.go

@@ -388,7 +388,8 @@ func ResolveGPGKeyFingerprint(keyID string) (string, error) {
 			return "", fmt.Errorf("%w: %s", ErrGPGKeyNotFound, keyID)
 		}
 
-		return "", fmt.Errorf("failed to resolve GPG key fingerprint: %w", err)
+		return "", fmt.Errorf(
+			"failed to resolve GPG key fingerprint: %w", withGPGStderr(err))
 	}
 
 	// Parse the output to extract the fingerprint
@@ -418,6 +419,18 @@ func checkGPGAvailable() error {
 	return nil
 }
 
+// withGPGStderr appends gpg's stderr, which says why gpg failed, to err
+// from cmd.Output. cmd.Output keeps that stderr in the *exec.ExitError.
+func withGPGStderr(err error) error {
+	var exitErr *exec.ExitError
+
+	if errors.As(err, &exitErr) {
+		return fmt.Errorf("%w: %s", err, strings.TrimSpace(string(exitErr.Stderr)))
+	}
+
+	return err
+}
+
 // gpgEncryptDefault is the default implementation of GPG encryption
 func gpgEncryptDefault(data *memguard.LockedBuffer, keyID string) ([]byte, error) {
 	if data == nil {
@@ -437,7 +450,7 @@ func gpgEncryptDefault(data *memguard.LockedBuffer, keyID string) ([]byte, error
 
 	output, err := cmd.Output()
 	if err != nil {
-		return nil, fmt.Errorf("GPG encryption failed: %w", err)
+		return nil, fmt.Errorf("GPG encryption failed: %w", withGPGStderr(err))
 	}
 
 	return output, nil
@@ -450,7 +463,7 @@ func gpgDecryptDefault(encryptedData []byte) (*memguard.LockedBuffer, error) {
 
 	output, err := cmd.Output()
 	if err != nil {
-		return nil, fmt.Errorf("GPG decryption failed: %w", err)
+		return nil, fmt.Errorf("GPG decryption failed: %w", withGPGStderr(err))
 	}
 
 	// Create a secure buffer for the decrypted data

internal/secret/pgpunlocker_test.go

@@ -102,3 +102,16 @@ func TestPGPUnlockerAddedTwiceKeepsFirst(t *testing.T) {
 	assert.NotEqual(t, first.GetDirectory(), second.GetDirectory())
 	assert.Equal(t, firstFiles, dirFiles(t, fs, first.GetDirectory()))
 }
+
+// TestGPGDecryptErrorHasGPGReason runs the real gpg on data that is not an
+// OpenPGP message and checks that the error carries the reason gpg writes
+// to stderr.
+func TestGPGDecryptErrorHasGPGReason(t *testing.T) {
+	// The empty GNUPGHOME keeps gpg away from the user's own keyring;
+	// LC_ALL=C keeps gpg's message in English.
+	t.Setenv("GNUPGHOME", t.TempDir())
+	t.Setenv("LC_ALL", "C")
+
+	_, err := secret.GPGDecryptFunc([]byte("not an OpenPGP message"))
+	require.ErrorContains(t, err, "gpg: no valid OpenPGP data found")
+}

internal/vault/management.go

@@ -212,16 +212,40 @@ func processMnemonicForVault(
 	return ltIdentity, derivationIndex, publicKeyHash, familyHash, nil
 }
 
+// CheckNewVaultName returns an error when name cannot be the name of a new
+// vault: one wrapping ErrInvalidVaultName when it is not a valid vault name,
+// and one wrapping ErrVaultExists when that vault already exists. Creating an
+// existing vault again would replace its keys, and its secrets could no
+// longer be decrypted. CreateVault checks this before writing anything; the
+// commands that create a vault also check it before they ask for the
+// mnemonic or passphrase, holding the state directory lock.
+func CheckNewVaultName(fs afero.Fs, stateDir string, name string) error {
+	err := ValidateVaultName(name)
+	if err != nil {
+		return err
+	}
+
+	exists, err := afero.DirExists(fs, filepath.Join(stateDir, "vaults.d", name))
+	if err != nil {
+		return fmt.Errorf("failed to check if vault exists: %w", err)
+	}
+
+	if exists {
+		return fmt.Errorf("vault %s %w", name, ErrVaultExists)
+	}
+
+	return nil
+}
+
 // CreateVault creates a new vault and selects it as the current vault. When
 // mnemonic is not nil, the vault's long-term key is derived from it, and the
 // returned vault has it as its Mnemonic; when it is nil, the vault has no
 // long-term key until one is imported. When passphrase is not nil, the vault
 // gets a passphrase unlocker protected by it, as its current unlocker; that
-// needs a mnemonic. It refuses a vault that already exists before writing
+// needs a mnemonic. It refuses an invalid name or a vault that already exists,
-// anything: creating it again would replace its keys, and its secrets could
+// as CheckNewVaultName does, before writing anything. The commands that call
-// no longer be decrypted. The commands that call it hold the state directory
+// it hold the state directory lock, so no other command can create the vault
-// lock, so no other command can create the vault between the check and the
+// between the check and the writes.
-// writes.
 //
 // The vault is written whole into a temporary directory, which is renamed
 // into vaults.d only once complete, and only then selected: a crash at any
@@ -233,30 +257,17 @@ func CreateVault(
 ) (*Vault, error) {
 	secret.Debug("Creating new vault", "name", name, "state_dir", stateDir)
 
-	err := ValidateVaultName(name)
+	err := CheckNewVaultName(fs, stateDir, name)
 	if err != nil {
-		secret.Debug("Invalid vault name provided", "vault_name", name)
-
 		return nil, err
 	}
 
-	secret.Debug("Vault name validation passed", "vault_name", name)
-
-	vaultDir := filepath.Join(stateDir, "vaults.d", name)
-
-	exists, err := afero.DirExists(fs, vaultDir)
-	if err != nil {
-		return nil, fmt.Errorf("failed to check if vault exists: %w", err)
-	}
-
-	if exists {
-		return nil, fmt.Errorf("vault %s %w", name, ErrVaultExists)
-	}
-
 	if passphrase != nil && mnemonic == nil {
 		return nil, fmt.Errorf("vault %s %w", name, ErrUnlockerWithoutMnemonic)
 	}
 
+	vaultDir := filepath.Join(stateDir, "vaults.d", name)
+
 	secret.Debug("Creating vault directory structure", "vault_dir", vaultDir)
 
 	err = secret.WriteDir(fs, vaultDir, func(dir string) error {