diff --git a/README.md b/README.md index 50e6515..ec1d8d9 100644 --- a/README.md +++ b/README.md @@ -250,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 [--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() }()