From 42f49b443e436818639f6e6c6422d753053f7fed Mon Sep 17 00:00:00 2001 From: sneak Date: Wed, 14 Oct 2026 21:50:40 +0000 Subject: [PATCH] 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 | 6 ++ internal/cli/passphrase_prompt_test.go | 128 +++++++++++++++++++++++++ internal/cli/unlockers.go | 29 ++++-- internal/secret/crypto.go | 7 +- 4 files changed, 159 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 3eaa1c4..9646b6a 100644 --- a/README.md +++ b/README.md @@ -261,6 +261,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 [--force]` / `secret unlocker rm` ⚠️ 🛑 **DANGER**: Permanently removes an unlocker. It first asks for confirmation, diff --git a/internal/cli/passphrase_prompt_test.go b/internal/cli/passphrase_prompt_test.go index a0ae81b..8ff3caf 100644 --- a/internal/cli/passphrase_prompt_test.go +++ b/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 diff --git a/internal/cli/unlockers.go b/internal/cli/unlockers.go index 87b3d7e..f3db2f5 100644 --- a/internal/cli/unlockers.go +++ b/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") diff --git a/internal/secret/crypto.go b/internal/secret/crypto.go index 8bbd208..8b0beee 100644 --- a/internal/secret/crypto.go +++ b/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() }()