Ask for the new passphrase in unlocker add when the variable is set (closes #154)

With SB_UNLOCK_PASSPHRASE set, `secret unlocker add passphrase` gave the
new unlocker the passphrase in the variable without asking. It now asks
on the terminal for the new passphrase and uses the variable to unlock
the vault. With no controlling terminal the new unlocker takes the
variable's passphrase, and the output says so.

secret.ErrNoTerminal is exported so that the command can tell a missing
terminal from other failures to read, such as nothing typed or a
mismatched confirmation, which still fail the command.

The in-process tests that add a passphrase unlocker with the variable
set rely on the Docker test phase having no controlling terminal; at a
terminal they would ask.

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)
 
@@ -267,6 +271,12 @@ Creates a new unlocker of the specified type:
 A vault has one passphrase unlocker: adding one replaces the one the vault has,
 which is removed only once the new one is the current unlocker.
 
+`secret unlocker add passphrase` asks on the terminal for the new passphrase
+even when `SB_UNLOCK_PASSPHRASE` is set, and uses the variable to unlock the
+vault. With no controlling terminal, as under cron or in a CI job, the new
+unlocker takes its passphrase from `SB_UNLOCK_PASSPHRASE`, and the command says
+so.
+
 #### `secret unlocker remove <unlocker-id> [--force]` / `secret unlocker rm` ⚠️ 🛑
 
 **DANGER**: Permanently removes an unlocker. It first asks for confirmation,
@@ -300,7 +310,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/passphrase_prompt_test.go

@@ -138,6 +138,148 @@ func TestInitWritesNoBlankLineAfterMnemonic(t *testing.T) {
 		"stderr: %q", stderr.String())
 }
 
+// TestUnlockerAddAsksForNewPassphrase is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/154: with SB_UNLOCK_PASSPHRASE set,
+// `secret unlocker add passphrase` gave the new unlocker the passphrase in
+// the variable without asking. At a terminal it must ask for the new
+// passphrase there, twice, and unlock the vault with the variable, so that
+// `secret get` then opens the vault with the passphrase typed. pty.Open is
+// described above TestRemoveIgnoresTerminalOnStdout.
+func TestUnlockerAddAsksForNewPassphrase(t *testing.T) {
+	t.Parallel()
+
+	const newPassphrase = "new-passphrase"
+
+	stateDir := t.TempDir()
+
+	passphrase := memguard.NewBufferFromBytes([]byte(testPassphrase))
+	defer passphrase.Destroy()
+
+	vlt, err := vault.CreateVault(afero.NewOsFs(), stateDir, "default",
+		testMnemonicBuffer(t), passphrase)
+	require.NoError(t, err)
+
+	value := memguard.NewBufferFromBytes([]byte("v1"))
+	defer value.Destroy()
+
+	require.NoError(t, vlt.AddSecret("s1", value, false))
+
+	ctx, cancel := context.WithTimeout(t.Context(), commandWait)
+	defer cancel()
+
+	//nolint:gosec // G204: test executes the freshly built secret binary
+	cmd := exec.CommandContext(ctx, secretBinaryPath(t),
+		"unlocker", "add", "passphrase")
+	cmd.Env = []string{
+		secret.EnvStateDir + "=" + stateDir,
+		secret.EnvUnlockPassphrase + "=" + testPassphrase,
+		"PATH=" + os.Getenv("PATH"),
+		"HOME=" + os.Getenv("HOME"),
+	}
+
+	ptmx, tty, err := pty.Open()
+	require.NoError(t, err)
+
+	defer func() { _ = ptmx.Close() }()
+
+	deadline, _ := ctx.Deadline()
+	require.NoError(t, ptmx.SetReadDeadline(deadline))
+
+	cmd.Stderr = tty
+	// Ctty is a descriptor in the new process: 2, its stderr.
+	cmd.SysProcAttr = &syscall.SysProcAttr{Setsid: true, Setctty: true, Ctty: 2}
+
+	require.NoError(t, cmd.Start())
+
+	_ = tty.Close()
+
+	var (
+		shown []byte
+		char  byte
+	)
+
+	terminal := bufio.NewReader(ptmx)
+
+	for _, prompt := range []string{
+		"Enter passphrase for unlocker: ", "Confirm passphrase: ",
+	} {
+		for !bytes.HasSuffix(shown, []byte(prompt)) {
+			char, err = terminal.ReadByte()
+			require.NoError(t, err,
+				"secret unlocker add passphrase did not ask on the terminal: %s",
+				shown)
+
+			shown = append(shown, char)
+		}
+
+		_, err = ptmx.WriteString(newPassphrase + "\n")
+		require.NoError(t, err)
+	}
+
+	// The read ends once the command has exited and so closed the terminal.
+	rest, _ := io.ReadAll(terminal)
+
+	require.NoError(t, cmd.Wait(),
+		"secret unlocker add passphrase: %s%s", shown, rest)
+
+	// The binary, not this test process, opens the vault: the new unlocker
+	// has age's full scrypt work factor, which is slow under the race
+	// detector.
+	assert.Equal(t, "v1",
+		runWithoutTerminal(t, stateDir, newPassphrase, "get", "s1"))
+}
+
+// TestUnlockerAddWithoutTerminalSaysPassphraseIsVariable is a regression
+// test for https://git.eeqj.de/sneak/secret/issues/154: with no terminal to
+// ask on, `secret unlocker add passphrase` gives the new unlocker the
+// passphrase in SB_UNLOCK_PASSPHRASE, and its output must say so.
+func TestUnlockerAddWithoutTerminalSaysPassphraseIsVariable(t *testing.T) {
+	t.Parallel()
+
+	stateDir := t.TempDir()
+
+	passphrase := memguard.NewBufferFromBytes([]byte(testPassphrase))
+	defer passphrase.Destroy()
+
+	_, err := vault.CreateVault(afero.NewOsFs(), stateDir, "default",
+		testMnemonicBuffer(t), passphrase)
+	require.NoError(t, err)
+
+	output := runWithoutTerminal(t, stateDir, testPassphrase,
+		"unlocker", "add", "passphrase")
+
+	assert.Contains(t, output, "The new unlocker takes its passphrase "+
+		"from SB_UNLOCK_PASSPHRASE, since there is no terminal to ask for a "+
+		"new one on\n")
+}
+
+// runWithoutTerminal runs the secret binary with args and SB_UNLOCK_PASSPHRASE
+// set to passphrase, in a new session, which has no terminal, and returns
+// what it printed once it has succeeded.
+func runWithoutTerminal(
+	t *testing.T, stateDir, passphrase string, args ...string,
+) string {
+	t.Helper()
+
+	ctx, cancel := context.WithTimeout(t.Context(), commandWait)
+	defer cancel()
+
+	//nolint:gosec // G204: test executes the freshly built secret binary
+	cmd := exec.CommandContext(ctx, secretBinaryPath(t), args...)
+	cmd.Env = []string{
+		secret.EnvStateDir + "=" + stateDir,
+		secret.EnvUnlockPassphrase + "=" + passphrase,
+		"PATH=" + os.Getenv("PATH"),
+		"HOME=" + os.Getenv("HOME"),
+	}
+	cmd.SysProcAttr = &syscall.SysProcAttr{Setsid: true}
+
+	output, err := cmd.CombinedOutput()
+	require.NoError(t, err, "secret %v: %s", args, output)
+
+	return string(output)
+}
+
 // runAtTerminal runs the secret binary with args and input on stdin, in a
 // new session whose controlling terminal is a new terminal, which is also
 // its stderr: a command typed at a terminal with its input piped. pty.Open

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/cli/unlockers.go

@@ -441,15 +441,20 @@ func (cli *Instance) addPassphraseUnlocker(cmd *cobra.Command) error {
 	// long-term key
 	vlt.Mnemonic, vlt.UnlockPassphrase = cli.Mnemonic, cli.UnlockPassphrase
 
-	// The new unlocker gets the passphrase from the environment, which also
-	// unlocks the current passphrase unlocker, else the one entered here
-	passphraseBuffer := cli.UnlockPassphrase
-	if passphraseBuffer == nil {
-		// Use secure passphrase input with confirmation
-		passphraseBuffer, err = readSecurePassphrase("Enter passphrase for unlocker: ")
-		if err != nil {
-			return err
-		}
+	// The new passphrase is asked for even when SB_UNLOCK_PASSPHRASE is
+	// set, and the variable unlocks the vault. With no terminal to ask on,
+	// the new unlocker takes the passphrase in the variable.
+	fromVariable := false
+
+	passphraseBuffer, err := readSecurePassphrase("Enter passphrase for unlocker: ")
+
+	switch {
+	case errors.Is(err, secret.ErrNoTerminal) && cli.UnlockPassphrase != nil:
+		passphraseBuffer = cli.UnlockPassphrase
+		fromVariable = true
+	case err != nil:
+		return err
+	default:
 		defer passphraseBuffer.Destroy()
 	}
 
@@ -460,6 +465,12 @@ func (cli *Instance) addPassphraseUnlocker(cmd *cobra.Command) error {
 
 	cmd.Printf("Created passphrase unlocker: %s\n", passphraseUnlocker.GetID())
 
+	if fromVariable {
+		cmd.Printf("The new unlocker takes its passphrase from %s, since "+
+			"there is no terminal to ask for a new one on\n",
+			secret.EnvUnlockPassphrase)
+	}
+
 	// CreatePassphraseUnlocker has already made it the current unlocker
 	cmd.Printf("Automatically selected as current unlocker\n")
 

internal/secret/crypto.go

@@ -15,10 +15,13 @@ import (
 
 var (
 	errNilPassphraseBuffer = errors.New("passphrase buffer is nil")
-	errNoTerminal          = errors.New("there is no terminal to ask on")
 	errNothingEntered      = errors.New("nothing was entered")
 )
 
+// ErrNoTerminal is wrapped in the error of ReadPassphrase and ReadMnemonic
+// when the process has no controlling terminal to ask on.
+var ErrNoTerminal = errors.New("there is no terminal to ask on")
+
 // ErrMnemonicNotRead is wrapped in every error of ReadMnemonic: there is no
 // terminal to read the mnemonic from, reading it failed, or it was empty.
 var ErrMnemonicNotRead = errors.New("failed to read mnemonic")
@@ -203,7 +206,7 @@ func readFromTerminal(
 
 		return nil, fmt.Errorf(
 			"%w: %w. Please set the %s environment variable or run interactively",
-			notRead, errNoTerminal, envVar)
+			notRead, ErrNoTerminal, envVar)
 	}
 	defer func() { _ = terminal.Close() }()
 

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