Hold the encrypt and decrypt result in memory until it is written (closes #143)

With --output, secret encrypt and secret decrypt emptied that file before
reading any input, so a failed command left it empty, and encrypting a file
onto itself gave the encryption of nothing.

With --output they now hold the result in memory and open --output, with the
same call as before, only once encryption or decryption has succeeded. A
failure to encrypt or decrypt leaves --output untouched, and --input can be the
same file. A failure while writing --output, such as a full disk, can leave it
empty or partly written; an error from closing it is reported. Without
--output the result still goes to stdout as it is produced.

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
 
@@ -313,6 +317,11 @@ generates a new Age key.
 
 Decrypts data using an Age key stored as a secret.
 
+With `--output`, both hold the result in memory and write it to `--output` only
+once encryption or decryption has succeeded, so a failure to encrypt or decrypt
+leaves `--output` untouched. A failure while writing `--output`, such as a full
+disk, can leave it empty or partly written.
+
 ## Storage Architecture
 
 ### Directory Structure

internal/cli/crypto.go

@@ -1,6 +1,7 @@
 package cli
 
 import (
+	"bytes"
 	"errors"
 	"fmt"
 	"io"
@@ -107,8 +108,8 @@ func (cli *Instance) storeNewEncryptionKey(
 // does not exist. The caller must destroy the returned buffer. Only storing
 // a new key takes the state directory lock, so that reading an existing key
 // works on a read-only state directory and keeps no other command waiting
-// at the passphrase prompt, and Encrypt streams its input and output
-// unlocked.
+// at the passphrase prompt, and Encrypt reads its input and writes its
+// output without the lock.
 func (cli *Instance) resolveEncryptionKey(
 	vlt *vault.Vault, secretName string,
 ) (*memguard.LockedBuffer, error) {
@@ -188,17 +189,17 @@ func (cli *Instance) Encrypt(secretName, inputFile, outputFile string) error {
 		input = file
 	}
 
-	// Set up output writer
+	// Set up output writer. With --output the result is held in memory
+	// and written only once encryption has succeeded, so a failure to
+	// encrypt leaves --output untouched and --input can be the same file.
+	// A failure while writing --output, such as a full disk, can leave it
+	// empty or partly written.
 	output := cli.cmd.OutOrStdout()
 
-	if outputFile != "" {
-		file, err := cli.fs.Create(outputFile)
-		if err != nil {
-			return fmt.Errorf("failed to create output file: %w", err)
-		}
-		defer func() { _ = file.Close() }()
+	var result bytes.Buffer
 
-		output = file
+	if outputFile != "" {
+		output = &result
 	}
 
 	// Encrypt the data
@@ -217,7 +218,11 @@ func (cli *Instance) Encrypt(secretName, inputFile, outputFile string) error {
 		return fmt.Errorf("failed to finalize encryption: %w", err)
 	}
 
-	return nil
+	if outputFile == "" {
+		return nil
+	}
+
+	return cli.writeOutputFile(outputFile, result.Bytes())
 }
 
 // Decrypt decrypts data using an age secret key stored in a secret
@@ -278,17 +283,17 @@ func (cli *Instance) Decrypt(secretName, inputFile, outputFile string) error {
 		input = file
 	}
 
-	// Set up output writer
+	// Set up output writer. With --output the result is held in memory
+	// and written only once decryption has succeeded, so a failure to
+	// decrypt leaves --output untouched and --input can be the same file.
+	// A failure while writing --output, such as a full disk, can leave it
+	// empty or partly written.
 	output := cli.cmd.OutOrStdout()
 
-	if outputFile != "" {
-		file, err := cli.fs.Create(outputFile)
-		if err != nil {
-			return fmt.Errorf("failed to create output file: %w", err)
-		}
-		defer func() { _ = file.Close() }()
+	var result bytes.Buffer
 
-		output = file
+	if outputFile != "" {
+		output = &result
 	}
 
 	// Decrypt the data
@@ -302,6 +307,32 @@ func (cli *Instance) Decrypt(secretName, inputFile, outputFile string) error {
 		return fmt.Errorf("failed to decrypt data: %w", err)
 	}
 
+	if outputFile == "" {
+		return nil
+	}
+
+	return cli.writeOutputFile(outputFile, result.Bytes())
+}
+
+// writeOutputFile creates outputFile, or empties it if it exists, and
+// writes result into it.
+func (cli *Instance) writeOutputFile(outputFile string, result []byte) error {
+	file, err := cli.fs.Create(outputFile)
+	if err != nil {
+		return fmt.Errorf("failed to create output file: %w", err)
+	}
+
+	_, err = file.Write(result)
+
+	closeErr := file.Close()
+	if err == nil {
+		err = closeErr
+	}
+
+	if err != nil {
+		return fmt.Errorf("failed to write output file: %w", err)
+	}
+
 	return nil
 }
 

internal/cli/crypto_test.go

@@ -0,0 +1,185 @@
+//nolint:testpackage // sets the unexported fields of Instance
+package cli
+
+import (
+	"errors"
+	"fmt"
+	"io"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/spf13/afero"
+	"github.com/spf13/cobra"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/secret/internal/vault"
+)
+
+// plainText is the content of plain.txt in the directory newCryptoTest makes.
+const plainText = "document"
+
+// newCryptoTest returns an Instance whose current vault, in a new state
+// directory on the real filesystem, holds the age key secret "key", and a
+// new directory holding only plain.txt and plain.txt.age, its encryption
+// with that key.
+func newCryptoTest(t *testing.T) (*Instance, string) {
+	t.Helper()
+
+	fs := afero.NewOsFs()
+	stateDir := t.TempDir()
+
+	_, err := vault.CreateVault(fs, stateDir, "default", testMnemonicBuffer(t), nil)
+	require.NoError(t, err)
+
+	cli := NewCLIInstanceWithStateDir(fs, stateDir)
+	cli.Mnemonic = testMnemonicBuffer(t)
+	cli.cmd = &cobra.Command{}
+
+	dir := t.TempDir()
+	plain := filepath.Join(dir, "plain.txt")
+	require.NoError(t, afero.WriteFile(fs, plain, []byte(plainText), 0o600))
+	require.NoError(t, cli.Encrypt("key", plain, plain+".age"))
+
+	return cli, dir
+}
+
+// TestFailedCryptoLeavesOutputUnchanged is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/143, where secret encrypt and
+// secret decrypt emptied the file named by --output before they failed.
+// A failed command must leave an existing file as it was and create none,
+// and give the same error as without --output.
+func TestFailedCryptoLeavesOutputUnchanged(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name string
+		run  func(cli *Instance, dir, output string) error
+	}{
+		// Reading a directory fails once encryption has begun
+		{"encrypt", func(cli *Instance, dir, output string) error {
+			return cli.Encrypt("key", dir, output)
+		}},
+		// plain.txt is not an age file
+		{"decrypt", func(cli *Instance, dir, output string) error {
+			return cli.Decrypt("key", filepath.Join(dir, "plain.txt"), output)
+		}},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			cli, dir := newCryptoTest(t)
+			existing := filepath.Join(dir, "existing.txt")
+			require.NoError(t, afero.WriteFile(cli.fs, existing, []byte("precious"), 0o600))
+
+			cli.cmd.SetOut(io.Discard)
+			toStdout := tt.run(cli, dir, "")
+			require.Error(t, toStdout)
+
+			require.EqualError(t, tt.run(cli, dir, existing), toStdout.Error())
+			require.EqualError(t, tt.run(cli, dir, filepath.Join(dir, "new.txt")),
+				toStdout.Error())
+
+			content, err := afero.ReadFile(cli.fs, existing)
+			require.NoError(t, err)
+			require.Equal(t, "precious", string(content))
+
+			exists, err := afero.Exists(cli.fs, filepath.Join(dir, "new.txt"))
+			require.NoError(t, err)
+			require.False(t, exists, "a failed %s created --output", tt.name)
+		})
+	}
+}
+
+// TestCryptoInPlace checks that secret encrypt and secret decrypt given
+// one file as both --input and --output replace it with the right result.
+// Before https://git.eeqj.de/sneak/secret/issues/143 they emptied the file
+// before reading it.
+func TestCryptoInPlace(t *testing.T) {
+	t.Parallel()
+
+	cli, dir := newCryptoTest(t)
+	file := filepath.Join(dir, "plain.txt")
+
+	require.NoError(t, cli.Encrypt("key", file, file))
+	require.NoError(t, cli.Decrypt("key", file, file))
+
+	content, err := afero.ReadFile(cli.fs, file)
+	require.NoError(t, err)
+	require.Equal(t, plainText, string(content))
+}
+
+// errCloseFailed is returned by the files closeFailFs creates in place of
+// a successful Close, as NFS reports a full disk only when the file is
+// closed.
+var errCloseFailed = errors.New("no space left on device")
+
+// closeFailFs creates files whose Close fails with errCloseFailed.
+type closeFailFs struct {
+	afero.Fs
+}
+
+// closeFailFile is a file created through closeFailFs.
+type closeFailFile struct {
+	afero.File
+}
+
+func (f closeFailFile) Close() error {
+	_ = f.File.Close()
+
+	return errCloseFailed
+}
+
+//nolint:ireturn // afero.File is the interface required by afero.Fs
+func (f closeFailFs) Create(name string) (afero.File, error) {
+	file, err := f.Fs.Create(name)
+	if err != nil {
+		return nil, err
+	}
+
+	return closeFailFile{File: file}, nil
+}
+
+// TestDecryptFailsWhenClosingOutputFails checks that secret decrypt fails
+// when closing --output fails, since some filesystems report a failed
+// write only then.
+func TestDecryptFailsWhenClosingOutputFails(t *testing.T) {
+	t.Parallel()
+
+	cli, dir := newCryptoTest(t)
+	cli.fs = closeFailFs{Fs: cli.fs}
+
+	err := cli.Decrypt("key", filepath.Join(dir, "plain.txt.age"),
+		filepath.Join(dir, "out.txt"))
+	require.ErrorIs(t, err, errCloseFailed)
+}
+
+// TestDecryptToDescriptorOfDeletedFile checks that secret decrypt given
+// /dev/fd/N as --output writes through that descriptor. A program may give
+// the command a temporary file it has already deleted as stdout, pass
+// /dev/stdout as --output and read the result back from that file.
+func TestDecryptToDescriptorOfDeletedFile(t *testing.T) {
+	t.Parallel()
+
+	cli, dir := newCryptoTest(t)
+
+	file, err := os.CreateTemp(t.TempDir(), "out")
+	require.NoError(t, err)
+
+	defer func() { _ = file.Close() }()
+
+	require.NoError(t, os.Remove(file.Name()))
+
+	output := fmt.Sprintf("/dev/fd/%d", file.Fd())
+	require.NoError(t, cli.Decrypt("key", filepath.Join(dir, "plain.txt.age"), output))
+
+	// On macOS the command's open of /dev/fd/N shares this descriptor's
+	// position, which its write leaves at the end.
+	_, err = file.Seek(0, io.SeekStart)
+	require.NoError(t, err)
+
+	content, err := io.ReadAll(file)
+	require.NoError(t, err)
+	require.Equal(t, plainText, string(content))
+}

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")
+}