diff --git a/README.md b/README.md index 72f4964..828f923 100644 --- a/README.md +++ b/README.md @@ -380,6 +380,11 @@ which every command except `secret vault import` offers when the variable is not set, is the safer default; `secret vault import` has no prompt and needs both variables. +The prompt asks on the terminal the command runs in, not on stdin, so it also +asks when stdin is a pipe, as in `echo value | secret add name --force` or +`cat file | secret encrypt key`. With no controlling terminal, as under cron or +in a CI job, the command fails and names the variable to set. + ## Security Features ### Encryption diff --git a/TODO.md b/TODO.md index b608afe..0614317 100644 --- a/TODO.md +++ b/TODO.md @@ -18,6 +18,16 @@ https://git.eeqj.de/sneak/secret/milestone/12 # Completed Steps +- 2026-10-14: The passphrase and mnemonic prompts ask on the controlling + terminal (`/dev/tty`) instead of stdin, so `echo v | secret add s1 --force`, + `cat file | secret encrypt key` and `cat file.age | secret decrypt key` ask + for the passphrase at a terminal instead of failing + (https://git.eeqj.de/sneak/secret/issues/144). Only a process with no + controlling terminal gets the error that names the variable to set. On SIGINT + or SIGTERM, `Entry()` restores the settings of that terminal, not of stdin, + since that is where the prompt turns echo off. `secret init` and + `secret vault create` no longer write a newline to stderr after the mnemonic + is typed, since the prompt ends that line on the terminal. - 2026-10-14: Removing the current unlocker no longer leaves the vault unable to open through the unlockers it still has (https://git.eeqj.de/sneak/secret/issues/137). `current-unlocker` went on diff --git a/internal/cli/create_vault_test.go b/internal/cli/create_vault_test.go index 17838d7..403fd31 100644 --- a/internal/cli/create_vault_test.go +++ b/internal/cli/create_vault_test.go @@ -8,6 +8,7 @@ import ( "os/exec" "slices" "strings" + "syscall" "testing" "github.com/awnumar/memguard" @@ -197,10 +198,10 @@ func TestStopAtPassphrasePromptLeavesNothing(t *testing.T) { // TestMnemonicNotReadNamesOnlyMnemonic is a regression test for // https://git.eeqj.de/sneak/secret/issues/115: `secret init` without -// SB_SECRET_MNEMONIC and with a stdin that is not a terminal said "failed to -// read mnemonic: failed to read passphrase: ...". The error must wrap +// SB_SECRET_MNEMONIC and with no terminal said "failed to read mnemonic: +// failed to read passphrase: ...". The error must wrap // secret.ErrMnemonicNotRead and name the mnemonic only. The message is -// pinned on the built binary, whose stdin is surely not a terminal. +// pinned on the built binary, run in a new session, which has no terminal. func TestMnemonicNotReadNamesOnlyMnemonic(t *testing.T) { t.Parallel() @@ -216,14 +217,15 @@ func TestMnemonicNotReadNamesOnlyMnemonic(t *testing.T) { "PATH=" + os.Getenv("PATH"), "HOME=" + os.Getenv("HOME"), } + cmd.SysProcAttr = &syscall.SysProcAttr{Setsid: true} output, err := cmd.CombinedOutput() require.Error(t, err) require.Equal(t, "Initialized secrets manager at: "+stateDir+"\n"+ - "Error: failed to read mnemonic: stdin is not a terminal (piped input "+ - "or script). Please set the SB_SECRET_MNEMONIC environment variable "+ - "or run interactively\n", string(output)) + "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)) } // TestStopDuringCreateLeavesWholeVaultOrNone is a regression test for diff --git a/internal/cli/init.go b/internal/cli/init.go index 83b88a7..c48f758 100644 --- a/internal/cli/init.go +++ b/internal/cli/init.go @@ -5,7 +5,6 @@ import ( "fmt" "log" "log/slog" - "os" "strings" "github.com/awnumar/memguard" @@ -57,13 +56,11 @@ func (cli *Instance) promptMnemonic() (*memguard.LockedBuffer, func(), error) { // Read mnemonic securely without echo mnemonicBuffer, err := secret.ReadMnemonic("Enter your BIP39 mnemonic phrase: ") if err != nil { - secret.Debug("Failed to read mnemonic from stdin", "error", err) + secret.Debug("Failed to read mnemonic", "error", err) return nil, nil, err } - fmt.Fprintln(os.Stderr) // Add newline after hidden input - return mnemonicBuffer, mnemonicBuffer.Destroy, nil } diff --git a/internal/cli/passphrase_prompt_test.go b/internal/cli/passphrase_prompt_test.go new file mode 100644 index 0000000..a0ae81b --- /dev/null +++ b/internal/cli/passphrase_prompt_test.go @@ -0,0 +1,204 @@ +package cli_test + +import ( + "bufio" + "bytes" + "context" + "io" + "os" + "os/exec" + "strings" + "syscall" + "testing" + + "filippo.io/age" + "github.com/awnumar/memguard" + "github.com/creack/pty" + "github.com/spf13/afero" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sneak.berlin/go/secret/internal/secret" + "sneak.berlin/go/secret/internal/vault" +) + +// TestPassphraseAskedAtTerminalWithPipedStdin is a regression test for +// https://git.eeqj.de/sneak/secret/issues/144: run at a terminal with stdin +// a pipe, as in `echo v2 | secret add s1 --force`, a command that needed +// the passphrase failed instead of asking for it. Each command here must ask +// on the terminal and succeed once the passphrase is typed there. +func TestPassphraseAskedAtTerminalWithPipedStdin(t *testing.T) { + t.Parallel() + + 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)) + + identity, err := age.GenerateX25519Identity() + require.NoError(t, err) + + key := secret.IdentityToLockedBuffer(identity) + defer key.Destroy() + + require.NoError(t, vlt.AddSecret("mykey", key, false)) + + runAtTerminal(t, stateDir, "v2\n", "add", "s1", "--force") + + vlt.UnlockPassphrase = passphrase + + added, err := vlt.GetSecret("s1") + require.NoError(t, err) + + defer added.Destroy() + + assert.Equal(t, "v2", added.String()) + + encrypted := runAtTerminal(t, stateDir, "a document\n", "encrypt", "mykey") + decrypted := runAtTerminal(t, stateDir, encrypted, "decrypt", "mykey") + assert.Equal(t, "a document\n", decrypted) +} + +// TestInitWritesNoBlankLineAfterMnemonic runs `secret init 2>log` at a +// terminal: the terminal is its controlling terminal and none of stdin, +// stdout and stderr. The mnemonic is typed on the terminal and the prompt +// ends its line there, so stderr must not get a newline for it as well, a +// blank line in the log. pty.Open is described above +// TestRemoveIgnoresTerminalOnStdout. +func TestInitWritesNoBlankLineAfterMnemonic(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithTimeout(t.Context(), commandWait) + defer cancel() + + stateDir := t.TempDir() + + //nolint:gosec // G204: test executes the freshly built secret binary + cmd := exec.CommandContext(ctx, secretBinaryPath(t), "init") + 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)) + + var stderr strings.Builder + + cmd.Stderr = &stderr + cmd.ExtraFiles = []*os.File{tty} + // Ctty is a descriptor in the new process: 3, the first of ExtraFiles. + cmd.SysProcAttr = &syscall.SysProcAttr{Setsid: true, Setctty: true, Ctty: 3} + + require.NoError(t, cmd.Start()) + + _ = tty.Close() + + var ( + shown []byte + char byte + ) + + terminal := bufio.NewReader(ptmx) + for !bytes.HasSuffix(shown, []byte("Enter your BIP39 mnemonic phrase: ")) { + char, err = terminal.ReadByte() + require.NoError(t, err, "secret init did not ask on the terminal: %s", + shown) + + shown = append(shown, char) + } + + _, err = ptmx.WriteString(testMnemonic + "\n") + require.NoError(t, err) + + // The read ends once the command has exited and so closed the terminal. + rest, _ := io.ReadAll(terminal) + + err = cmd.Wait() + require.NoError(t, err, "secret init: %s%s%s", shown, rest, stderr.String()) + + assert.True(t, strings.HasPrefix(stderr.String(), + "Initialized secrets manager at: "+stateDir+"\n\n"+ + "Default vault created and configured\n"), + "stderr: %q", stderr.String()) +} + +// 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 +// is described above TestRemoveIgnoresTerminalOnStdout. runAtTerminal waits +// for the passphrase prompt on the terminal, types the passphrase there, +// and returns what the command wrote to stdout once it has succeeded. +func runAtTerminal(t *testing.T, stateDir, input 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, + "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)) + + var stdout strings.Builder + + cmd.Stdin = strings.NewReader(input) + cmd.Stdout = &stdout + 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 !bytes.HasSuffix(shown, []byte("Enter unlock passphrase: ")) { + char, err = terminal.ReadByte() + require.NoError(t, err, "secret %v did not ask on the terminal: %s", + args, shown) + + shown = append(shown, char) + } + + _, err = ptmx.WriteString(testPassphrase + "\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 %v: %s%s", args, shown, rest) + + return stdout.String() +} diff --git a/internal/cli/root.go b/internal/cli/root.go index 8a7929f..2921c8c 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -15,16 +15,29 @@ import ( // but exit with the code. func Entry() int { // On SIGINT or SIGTERM memguard runs this function, wipes every buffer - // and exits with status 1. The passphrase prompt turns terminal echo - // off until the read finishes, so a signal there would leave echo off. - // Only a process in the terminal's foreground process group may reset - // it: one in the background that tries is stopped instead of exiting. - terminalState, terminalErr := term.GetState(unix.Stdin) + // and exits with status 1. The passphrase prompt turns echo off on the + // controlling terminal until the read finishes, so a signal there would + // leave echo off. Only a process in the terminal's foreground process + // group may reset it: one in the background that tries is stopped + // instead of exiting. + terminal, terminalErr := os.Open(secret.TerminalPath) + + var terminalState *term.State + + if terminalErr == nil { + defer func() { _ = terminal.Close() }() + + terminalState, terminalErr = term.GetState(int(terminal.Fd())) + } memguard.CatchSignal(func(os.Signal) { - foreground, err := unix.IoctlGetInt(unix.Stdin, unix.TIOCGPGRP) - if terminalErr == nil && err == nil && foreground == unix.Getpgrp() { - _ = term.Restore(unix.Stdin, terminalState) + if terminalErr != nil { + return + } + + foreground, err := unix.IoctlGetInt(int(terminal.Fd()), unix.TIOCGPGRP) + if err == nil && foreground == unix.Getpgrp() { + _ = term.Restore(int(terminal.Fd()), terminalState) } }, os.Interrupt, unix.SIGTERM) diff --git a/internal/cli/unlock_failure_test.go b/internal/cli/unlock_failure_test.go index 817c9e1..7bc9574 100644 --- a/internal/cli/unlock_failure_test.go +++ b/internal/cli/unlock_failure_test.go @@ -18,6 +18,7 @@ import ( "os" "os/exec" "path/filepath" + "syscall" "testing" "filippo.io/age" @@ -288,7 +289,7 @@ func TestMissingCurrentUnlockerNamesMnemonic(t *testing.T) { } // TestPassphraseNotReadNamesNoMnemonic runs `secret get x` on the built -// binary without SB_UNLOCK_PASSPHRASE and with a stdin that is not a +// binary without SB_UNLOCK_PASSPHRASE and in a new session, which has no // terminal, so the passphrase cannot be read. The unlocker was not tried, // and adding one would need a passphrase read the same way, so the error // is the cause alone, without the advice to use the mnemonic. @@ -319,15 +320,16 @@ func TestPassphraseNotReadNamesNoMnemonic(t *testing.T) { "PATH=" + os.Getenv("PATH"), "HOME=" + os.Getenv("HOME"), } + cmd.SysProcAttr = &syscall.SysProcAttr{Setsid: true} output, err := cmd.CombinedOutput() require.Error(t, err) assert.Equal(t, "Error: failed to unlock vault: "+ "failed to get long-term key: failed to get unlocker identity: "+ - "failed to read passphrase: stdin is not a terminal (piped input or "+ - "script). Please set the SB_UNLOCK_PASSPHRASE environment variable or "+ - "run interactively\n", string(output)) + "failed to read passphrase: there is no terminal to ask on. Please "+ + "set the SB_UNLOCK_PASSPHRASE environment variable or run "+ + "interactively\n", string(output)) } // TestCryptoUnlockFailureNamesMnemonic checks that `secret encrypt` and diff --git a/internal/secret/constants.go b/internal/secret/constants.go index 6d68bfb..f8eaf9d 100644 --- a/internal/secret/constants.go +++ b/internal/secret/constants.go @@ -17,6 +17,10 @@ const ( // EnvGPGKeyID is the environment variable for providing the GPG key ID EnvGPGKeyID = "SB_GPG_KEY_ID" + // TerminalPath opens the controlling terminal of the process that opens + // it, where the passphrase and mnemonic prompts ask. + TerminalPath = "/dev/tty" + // UnlockerTimeFormat is the layout of the time, in UTC, in the name of a // new unlocker's directory. It runs to the nanosecond, so that every new // unlocker, even one added right after another, gets a directory of its diff --git a/internal/secret/crypto.go b/internal/secret/crypto.go index 28e3d2f..8bbd208 100644 --- a/internal/secret/crypto.go +++ b/internal/secret/crypto.go @@ -6,7 +6,6 @@ import ( "fmt" "io" "os" - "syscall" "unsafe" "filippo.io/age" @@ -16,11 +15,8 @@ import ( var ( errNilPassphraseBuffer = errors.New("passphrase buffer is nil") - errStdinNotTerminal = errors.New( - "stdin is not a terminal (piped input or script)") - errStderrNotTerminal = errors.New( - "stderr is not a terminal (running in non-interactive mode)") - errNothingEntered = errors.New("nothing was entered") + errNoTerminal = errors.New("there is no terminal to ask on") + errNothingEntered = errors.New("nothing was entered") ) // ErrMnemonicNotRead is wrapped in every error of ReadMnemonic: there is no @@ -193,38 +189,33 @@ func ReadMnemonic(prompt string) (*memguard.LockedBuffer, error) { return readFromTerminal(prompt, ErrMnemonicNotRead, EnvMnemonic) } -// readFromTerminal reads input from the terminal without echoing it. Every -// error it returns wraps notRead; without a terminal, the error says to set -// envVar instead. +// readFromTerminal asks on the controlling terminal and reads the answer +// there without echoing it, whatever stdin is: in +// `echo value | secret add name --force`, stdin is a pipe. Every error it +// returns wraps notRead; without a terminal, the error says to set envVar +// instead. func readFromTerminal( prompt string, notRead error, envVar string, ) (*memguard.LockedBuffer, error) { - // Check if stdin is a terminal - if !term.IsTerminal(syscall.Stdin) { - // Not a terminal - never read secrets from piped input - // for security reasons + terminal, err := os.OpenFile(TerminalPath, os.O_RDWR, 0) + if err != nil { + Debug("Failed to open the controlling terminal", "error", err) + return nil, fmt.Errorf( "%w: %w. Please set the %s environment variable or run interactively", - notRead, errStdinNotTerminal, envVar) + notRead, errNoTerminal, envVar) } + defer func() { _ = terminal.Close() }() - // stdin is a terminal, check if stderr is also a terminal for - // interactive prompting - if !term.IsTerminal(syscall.Stderr) { - return nil, fmt.Errorf("%w: %w. Please set the %s environment variable", - notRead, errStderrNotTerminal, envVar) - } + _, _ = fmt.Fprint(terminal, prompt) - // Both stdin and stderr are terminals - use secure password reading - fmt.Fprint(os.Stderr, prompt) // Write prompt to stderr, not stdout - - input, err := term.ReadPassword(syscall.Stdin) + input, err := term.ReadPassword(int(terminal.Fd())) if err != nil { return nil, fmt.Errorf("%w: %w", notRead, err) } - // Print newline to stderr since ReadPassword doesn't echo - fmt.Fprintln(os.Stderr) + // The newline typed after the input, which ReadPassword did not echo + _, _ = fmt.Fprintln(terminal) if len(input) == 0 { return nil, fmt.Errorf("%w: %w", notRead, errNothingEntered)