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

@@ -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)
 
@@ -242,6 +250,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,

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"+
-		"Error: failed to read mnemonic: there is no terminal to ask on. "+
-		"Please set the SB_SECRET_MNEMONIC environment variable or run "+
+	require.Equal(t, "Error: failed to read mnemonic: there is no terminal to "+
+		"ask on. 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 {
-		cmd.Printf("Initialized secrets manager at: %s\n", stateDir)
+	// Refuse an existing default vault before asking for anything
+	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/passphrase_prompt_test.go

@@ -138,6 +138,134 @@ 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
+// the vault then opens 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()
+
+	_, err := vault.CreateVault(afero.NewOsFs(), stateDir, "default",
+		testMnemonicBuffer(t), passphrase)
+	require.NoError(t, err)
+
+	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)
+
+	typed := memguard.NewBufferFromBytes([]byte(newPassphrase))
+	defer typed.Destroy()
+
+	reopened := vault.NewVault(afero.NewOsFs(), stateDir, "default")
+	reopened.UnlockPassphrase = typed
+
+	_, err = reopened.GetOrDeriveLongTermKey()
+	require.NoError(t, err, "the vault does not open with the passphrase typed")
+}
+
+// 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. The binary
+// runs in a new session, which has no terminal.
+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)
+
+	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"),
+	}
+	cmd.SysProcAttr = &syscall.SysProcAttr{Setsid: true}
+
+	output, err := cmd.CombinedOutput()
+	require.NoError(t, err, "secret unlocker add passphrase: %s", output)
+
+	assert.Contains(t, string(output), "The new unlocker takes its passphrase "+
+		"from SB_UNLOCK_PASSPHRASE, since there is no terminal to ask for a "+
+		"new one on\n")
+}
+
 // 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/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/unlockers.go

@@ -440,15 +440,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()
 	}
 
@@ -459,6 +464,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/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/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/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
-// anything: creating it again would replace its keys, and its secrets could
-// no longer be decrypted. The commands that call it hold the state directory
-// lock, so no other command can create the vault between the check and the
-// writes.
+// needs a mnemonic. It refuses an invalid name or a vault that already exists,
+// as CheckNewVaultName does, before writing anything. The commands that call
+// it hold the state directory lock, so no other command can create the vault
+// between the check and the 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 {