diff --git a/README.md b/README.md index 63d0120..03869cb 100644 --- a/README.md +++ b/README.md @@ -267,6 +267,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..8543424 100644 --- a/internal/cli/passphrase_prompt_test.go +++ b/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 diff --git a/internal/cli/unlockers.go b/internal/cli/unlockers.go index 2e1e3de..0d3078b 100644 --- a/internal/cli/unlockers.go +++ b/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") 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() }()