A cross-vault secret mv that cannot remove the source fails (closes #159)

When `secret mv` copied a secret into another vault but could not remove
the source, it printed a warning and the usual "Moved" line and exited 0,
so a script could not tell that the secret was now in both vaults. It now
prints no "Moved" line and fails with an error saying that the copy holds
all versions and the source remains.

The test is in package cli and calls moveSecret, because the state
directory lock refuses the wrapped filesystem that makes the removal fail.

Judgement call: if the source is moved aside but the temporary directory
holding it cannot then be deleted, the error still says the source remains.

Model: opus-5-5

README.md

@@ -142,7 +142,11 @@ value may be over 100 MiB, whatever the limit.
 
 #### `secret add <secret-name> [--force]`
 
-Adds a secret to the current vault. Reads the secret value from stdin.
+Adds a secret to the current vault. Reads the secret value from stdin. When the
+value ends in a newline, that one newline is dropped, so
+`echo value | secret add name` stores `value`; from `printf 'a\n\n'` it stores
+`a` and one newline. To keep a file's final newline, as in a PEM key, use
+`secret import`, which stores the file's bytes unchanged.
 
 - `--force, -f`: Overwrite existing secret
 
@@ -155,13 +159,13 @@ contain `//`, or have `..` as a path segment.
 
 #### `secret get <secret-name> [--version <version>]`
 
-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.
+Writes the secret's value to stdout exactly as stored, with no newline added
+after it. 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)
 
@@ -300,7 +304,7 @@ Selects an unlocker as the current default for operations.
 #### `secret import <secret-name> --source <filename>`
 
 Imports a secret from a file and stores it in the current vault under the given
-name.
+name. The file's bytes are stored unchanged, a final newline included.
 
 ### Encryption Operations
 

internal/cli/trailing_newline_test.go

@@ -0,0 +1,47 @@
+//nolint:testpackage // white-box test of unexported internals
+package cli
+
+import (
+	"bytes"
+	"testing"
+
+	"github.com/spf13/afero"
+	"github.com/spf13/cobra"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// TestAddDropsOneTrailingNewlineImportKeepsIt pins the rule the README
+// gives: `secret add` drops one trailing newline from stdin, and
+// `secret import` stores the file's bytes unchanged.
+func TestAddDropsOneTrailingNewlineImportKeepsIt(t *testing.T) {
+	t.Parallel()
+
+	fs, vlt := newSizeTestVault(t)
+	value := []byte("a\n\n")
+
+	cmd := &cobra.Command{}
+	cmd.SetIn(bytes.NewReader(value))
+
+	cli := NewCLIInstanceWithStateDir(fs, testStateDir)
+	cli.cmd = cmd
+
+	require.NoError(t, cli.AddSecret("added", false))
+
+	sourceFile := "/test/value"
+	require.NoError(t, afero.WriteFile(fs, sourceFile, value, 0o600))
+	require.NoError(t, cli.ImportSecret(cmd, "imported", sourceFile, false))
+
+	added, err := vlt.GetSecret("added")
+	require.NoError(t, err)
+
+	defer added.Destroy()
+
+	imported, err := vlt.GetSecret("imported")
+	require.NoError(t, err)
+
+	defer imported.Destroy()
+
+	assert.Equal(t, []byte("a\n"), added.Bytes())
+	assert.Equal(t, []byte("a\n\n"), imported.Bytes())
+}

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,24 @@ 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.
+// err is returned unchanged when gpg wrote nothing to stderr.
+func withGPGStderr(err error) error {
+	var exitErr *exec.ExitError
+
+	if !errors.As(err, &exitErr) {
+		return err
+	}
+
+	stderr := strings.TrimSpace(string(exitErr.Stderr))
+	if stderr == "" {
+		return err
+	}
+
+	return fmt.Errorf("%w: %s", err, stderr)
+}
+
 // gpgEncryptDefault is the default implementation of GPG encryption
 func gpgEncryptDefault(data *memguard.LockedBuffer, keyID string) ([]byte, error) {
 	if data == nil {
@@ -437,7 +456,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 +469,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,27 @@ 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")
+}
+
+// TestGPGDecryptErrorWithoutGPGStderr checks that when gpg fails without
+// writing to stderr, the error ends with the exit status and nothing after it.
+//
+//nolint:paralleltest // installFakeGPG uses t.Setenv
+func TestGPGDecryptErrorWithoutGPGStderr(t *testing.T) {
+	installFakeGPG(t)
+
+	_, err := secret.GPGDecryptFunc([]byte("not an OpenPGP message"))
+	require.EqualError(t, err, "GPG decryption failed: exit status 1")
+}