README.md

@@ -271,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,

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/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() }()