From c3cc26c7210cd28897d53bde679f87d82c887cc7 Mon Sep 17 00:00:00 2001 From: sneak Date: Wed, 14 Oct 2026 21:47:30 +0000 Subject: [PATCH] Put the terminal back when a signal ends the mnemonic prompt (closes #91) A SIGINT, SIGTERM or SIGHUP at the mnemonic prompt killed the tool with echo still off. The prompt now catches them with signals.Notify from before echo goes off, and reads the line in a goroutine. Once the line is read or a signal arrives, it puts the terminal settings back and stops catching; if a signal has arrived by then, it sends the tool the same signal again, so the tool is still ended by it (130 for SIGINT in a shell). A test sends SIGINT at the prompt on a pseudo-terminal. The signals package moves from internal/cli to internal/signals, since the mnemonic prompt now uses it as well. Unchanged: the Windows prompt, which uses term.ReadPassword. Model: opus-5-5 --- README.md | 17 ++-- internal/cli/age/age.go | 2 +- internal/cli/cli.go | 3 + internal/cli/prompt_test.go | 110 +++++++++++++++++++------- internal/cli/ssh/install.go | 2 +- internal/cli/ssh/to.go | 2 +- internal/mnemonic/prompt_unix.go | 75 ++++++++++++++++-- internal/{cli => }/signals/signals.go | 16 ++-- 8 files changed, 175 insertions(+), 52 deletions(-) rename internal/{cli => }/signals/signals.go (65%) diff --git a/README.md b/README.md index 381ff1b..9996d00 100644 --- a/README.md +++ b/README.md @@ -65,10 +65,12 @@ calls into `internal/`. The packages there are: and decrypts with it. - `internal/childmnemonic` derives a child mnemonic from the main one using BIP-85's own mnemonic application. +- `internal/signals` catches SIGINT, SIGTERM and SIGHUP for the commands that + clean up before they end, and for the mnemonic prompt, which puts the terminal + back before the signal ends the tool. - `internal/cli` builds the cobra command tree and runs it. Under it, - `cli/options` holds the flags every command shares, `cli/signals` catches - SIGINT, SIGTERM and SIGHUP for the commands that clean up before they end, and - `cli/ssh`, `cli/age` and `cli/mnemonic` are the command groups. + `cli/options` holds the flags every command shares, and `cli/ssh`, `cli/age` + and `cli/mnemonic` are the command groups. - `internal/bip39` is a copy of `github.com/tyler-smith/go-bip39` v1.1.0, trimmed to what keyfunc uses. @@ -328,10 +330,11 @@ passes through `ssh`'s own exit status. SIGINT, SIGTERM and SIGHUP end any command at once, at the mnemonic prompt too, with the status a shell gives a program killed by that signal (130 for SIGINT). -While `age encrypt -o` or `age decrypt -o` is writing a new file or replacing a -regular one, the signal makes it remove the unfinished file, leave a file -already at the named path as it was, and exit with status 1. That holds for a -signal that has reached `keyfunc` when its input ends; a later one leaves the +At the prompt the terminal's settings are put back as they were first, so echo +is on again. While `age encrypt -o` or `age decrypt -o` is writing a new file or +replacing a regular one, the signal makes it remove the unfinished file, leave a +file already at the named path as it was, and exit with status 1. That holds for +a signal that has reached `keyfunc` when its input ends; a later one leaves the whole file in place. Ctrl-C on a pipeline ends the input at the same moment, and on Linux `keyfunc` sees the signal first, though no system promises that. A named pipe or a device at the `-o` path, or a path that is the same file as diff --git a/internal/cli/age/age.go b/internal/cli/age/age.go index feb5137..45dbfc7 100644 --- a/internal/cli/age/age.go +++ b/internal/cli/age/age.go @@ -16,8 +16,8 @@ import ( "github.com/spf13/cobra" "sneak.berlin/go/keyfunc/internal/agekey" "sneak.berlin/go/keyfunc/internal/cli/options" - "sneak.berlin/go/keyfunc/internal/cli/signals" "sneak.berlin/go/keyfunc/internal/derive" + "sneak.berlin/go/keyfunc/internal/signals" ) // ErrInterrupted is returned when SIGINT, SIGTERM or SIGHUP has been diff --git a/internal/cli/cli.go b/internal/cli/cli.go index dabad05..03a107e 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -87,6 +87,9 @@ func init() { // still runs, and "age encrypt -o" and "age decrypt -o" while they // write a new file to rename over the named one, so the unfinished file // is removed. +// +// At the mnemonic prompt the signal is caught only to put the terminal +// back, and then ends the tool as above. func Main() int { err := Root().Execute() if err == nil { diff --git a/internal/cli/prompt_test.go b/internal/cli/prompt_test.go index 6090182..3c99dfb 100644 --- a/internal/cli/prompt_test.go +++ b/internal/cli/prompt_test.go @@ -6,26 +6,25 @@ import ( "os" "os/exec" "strings" + "syscall" "testing" "time" "github.com/creack/pty" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/term" "sneak.berlin/go/keyfunc/internal/mnemonic" ) -// TestEndOfInputAtThePromptEndsTheTool runs "age pub" with no mnemonic -// given and a terminal on all three streams, waits for the prompt, and -// types Ctrl-D. The tool has to end with status 1, say that the +// TestEndOfInputAtThePromptEndsTheTool starts "age pub" and, at the +// prompt, types Ctrl-D. The tool has to end with status 1, say that the // mnemonic is empty, and leave the terminal set as it found it, so with // echo on again. // // pty.Open returns the two ends of a new terminal: tty is the end the // tool uses as its terminal, and ptmx the end the test reads what the -// terminal shows from and types into. Reading ptmx stops at the -// deadline, so a prompt that never comes fails the test instead of -// hanging it. +// terminal shows from and types into. func TestEndOfInputAtThePromptEndsTheTool(t *testing.T) { t.Setenv(mnemonic.Variable, "") t.Setenv(mnemonic.CommandVariable, "") @@ -36,33 +35,10 @@ func TestEndOfInputAtThePromptEndsTheTool(t *testing.T) { defer func() { _ = ptmx.Close() }() defer func() { _ = tty.Close() }() - require.NoError(t, ptmx.SetReadDeadline(time.Now().Add(10*time.Second))) - before, err := term.GetState(int(tty.Fd())) require.NoError(t, err) - //nolint:gosec // this test's own binary as the tool - command := exec.CommandContext(t.Context(), os.Args[0], "age", "pub") - - command.Env = append(os.Environ(), runAsTool+"=1") - command.Stdin = tty - command.Stdout = tty - command.Stderr = tty - - require.NoError(t, command.Start()) - - var ( - shown string - char byte - ) - - terminal := bufio.NewReader(ptmx) - for !strings.HasSuffix(shown, "mnemonic: ") { - char, err = terminal.ReadByte() - require.NoError(t, err, "the tool did not prompt: %q", shown) - - shown += string(char) - } + command, terminal := prompted(t, ptmx, tty) _, err = ptmx.WriteString("\x04") // Ctrl-D require.NoError(t, err) @@ -81,3 +57,77 @@ func TestEndOfInputAtThePromptEndsTheTool(t *testing.T) { said, _ := io.ReadAll(terminal) require.Contains(t, string(said), mnemonic.ErrEmpty.Error()) } + +// TestASignalAtThePromptPutsTheTerminalBack starts "age pub" and, at +// the prompt, once the tool has turned echo off, sends it SIGINT. The +// tool has to be ended by SIGINT, which a shell reports as status 130, +// and leave the terminal set as it found it, so with echo on again. +func TestASignalAtThePromptPutsTheTerminalBack(t *testing.T) { + t.Setenv(mnemonic.Variable, "") + t.Setenv(mnemonic.CommandVariable, "") + + ptmx, tty, err := pty.Open() + require.NoError(t, err) + + defer func() { _ = ptmx.Close() }() + defer func() { _ = tty.Close() }() + + before, err := term.GetState(int(tty.Fd())) + require.NoError(t, err) + + command, _ := prompted(t, ptmx, tty) + + // The prompt is shown just before echo is turned off, and the + // signal has to come while it is off. + require.Eventually(t, func() bool { + now, err := term.GetState(int(tty.Fd())) + + return err == nil && !assert.ObjectsAreEqual(before, now) + }, 5*time.Second, 5*time.Millisecond, "the tool did not turn echo off") + + require.NoError(t, command.Process.Signal(syscall.SIGINT)) + waitForTool(t, "SIGINT at the prompt", command) + + status, ok := command.ProcessState.Sys().(syscall.WaitStatus) + require.True(t, ok) + require.True(t, status.Signaled(), "the tool was not ended by the signal") + require.Equal(t, syscall.SIGINT, status.Signal()) + + after, err := term.GetState(int(tty.Fd())) + require.NoError(t, err) + require.Equal(t, before, after, "the terminal settings were not restored") +} + +// prompted starts "age pub" as a subprocess, for a test that has set no +// mnemonic, with the terminal's tty end on all three streams, and +// returns once the tool has shown the prompt. The reader returned reads +// what the terminal shows from its ptmx end. Reading ptmx stops at a +// deadline, so a prompt that never comes fails the test instead of +// hanging it. +func prompted(t *testing.T, ptmx, tty *os.File) (*exec.Cmd, *bufio.Reader) { + t.Helper() + + require.NoError(t, ptmx.SetReadDeadline(time.Now().Add(10*time.Second))) + + //nolint:gosec // this test's own binary as the tool + command := exec.CommandContext(t.Context(), os.Args[0], "age", "pub") + + command.Env = append(os.Environ(), runAsTool+"=1") + command.Stdin = tty + command.Stdout = tty + command.Stderr = tty + + require.NoError(t, command.Start()) + + var shown string + + terminal := bufio.NewReader(ptmx) + for !strings.HasSuffix(shown, "mnemonic: ") { + char, err := terminal.ReadByte() + require.NoError(t, err, "the tool did not prompt: %q", shown) + + shown += string(char) + } + + return command, terminal +} diff --git a/internal/cli/ssh/install.go b/internal/cli/ssh/install.go index 4a217cb..7bae317 100644 --- a/internal/cli/ssh/install.go +++ b/internal/cli/ssh/install.go @@ -15,8 +15,8 @@ import ( "time" "github.com/spf13/cobra" - "sneak.berlin/go/keyfunc/internal/cli/signals" "sneak.berlin/go/keyfunc/internal/mnemonic" + "sneak.berlin/go/keyfunc/internal/signals" ) // Where the key goes on the host and what the file it arrives in is diff --git a/internal/cli/ssh/to.go b/internal/cli/ssh/to.go index 69fc806..428e8dd 100644 --- a/internal/cli/ssh/to.go +++ b/internal/cli/ssh/to.go @@ -10,8 +10,8 @@ import ( "syscall" "github.com/spf13/cobra" - "sneak.berlin/go/keyfunc/internal/cli/signals" "sneak.berlin/go/keyfunc/internal/mnemonic" + "sneak.berlin/go/keyfunc/internal/signals" ) // StatusError says the tool should end with the status ssh ended with. diff --git a/internal/mnemonic/prompt_unix.go b/internal/mnemonic/prompt_unix.go index e7701b2..ef74be6 100644 --- a/internal/mnemonic/prompt_unix.go +++ b/internal/mnemonic/prompt_unix.go @@ -8,8 +8,10 @@ import ( "fmt" "io" "os" + "os/signal" "golang.org/x/sys/unix" + "sneak.berlin/go/keyfunc/internal/signals" ) // readWithoutEcho reads one line from the terminal on standard input @@ -17,6 +19,11 @@ import ( // input (Ctrl-D). The terminal is set as term.ReadPassword sets it; // ReadPassword itself is not used here because it never sees the end // of input and goes on waiting. +// +// Meanwhile SIGINT, SIGTERM and SIGHUP are caught, as signals.Notify +// catches them. One that has arrived by the time the line is read puts +// the terminal back as it was and then ends the tool, as it would have +// ended it if it were not caught. func readWithoutEcho() (string, error) { fd := int(os.Stdin.Fd()) @@ -25,6 +32,13 @@ func readWithoutEcho() (string, error) { return "", fmt.Errorf("reading the terminal settings: %w", err) } + // Caught from before echo is turned off, so that no signal can end + // the tool with echo off. + received := make(chan os.Signal, 1) + signals.Notify(received) + + defer signal.Stop(received) + hidden := *settings hidden.Lflag &^= unix.ECHO hidden.Lflag |= unix.ICANON | unix.ISIG @@ -35,14 +49,63 @@ func readWithoutEcho() (string, error) { return "", fmt.Errorf("turning echo off: %w", err) } - defer func() { _ = unix.IoctlSetTermios(fd, ioctlWriteTermios, settings) }() + var ( + typed string + failed error + ) - // With ICANON set, a read from the terminal returns at most one - // line, so the buffer takes nothing past the newline. - typed, err := bufio.NewReader(os.Stdin).ReadString('\n') - if errors.Is(err, io.EOF) { + read := make(chan struct{}) + + go func() { + // With ICANON set, a read from the terminal returns at most one + // line, so the buffer takes nothing past the newline. + typed, failed = bufio.NewReader(os.Stdin).ReadString('\n') + + close(read) + }() + + var ending os.Signal + + select { + case <-read: + case ending = <-received: + } + + // The terminal is put back while the signals are still caught, so + // that none can end the tool with echo off. Stop returns only once + // every signal the tool has received has been handed over, so one + // that came as the line was read is in received by then. + _ = unix.IoctlSetTermios(fd, ioctlWriteTermios, settings) + + signal.Stop(received) + + if len(received) > 0 { + ending = <-received + } + + if ending != nil { + return "", endBy(ending) + } + + if errors.Is(failed, io.EOF) { return typed, nil } - return typed, err + return typed, failed +} + +// endBy sends the tool the signal again, now that nothing catches it, +// so that it ends the tool, and waits for that. It returns only if the +// signal could not be sent. +func endBy(ending os.Signal) error { + tool, err := os.FindProcess(os.Getpid()) + if err == nil { + err = tool.Signal(ending) + } + + if err != nil { + return fmt.Errorf("ending the tool by %s: %w", ending, err) + } + + select {} } diff --git a/internal/cli/signals/signals.go b/internal/signals/signals.go similarity index 65% rename from internal/cli/signals/signals.go rename to internal/signals/signals.go index 1657564..3da5c05 100644 --- a/internal/cli/signals/signals.go +++ b/internal/signals/signals.go @@ -1,5 +1,9 @@ -// Package signals catches the signals that end the tool, for the -// commands that clean up before they end. +// Package signals catches SIGINT, SIGTERM and SIGHUP, the signals that +// end the tool, for the commands that clean up before they end and for +// the mnemonic prompt, which puts the terminal back before the signal +// ends the tool. Go keeps an inherited ignore only for SIGHUP and +// SIGINT, so a SIGTERM the tool was started with set to be ignored is +// not left ignored. package signals import ( @@ -11,8 +15,8 @@ import ( // Context is signal.NotifyContext for SIGINT, SIGTERM and SIGHUP: the // context it returns is cancelled when one of them arrives, and stop -// stops catching them. It leaves out any of the three the tool was -// started with set to be ignored, as nohup does with SIGHUP, because +// stops catching them. It leaves out SIGHUP or SIGINT if the tool was +// started with it set to be ignored, as nohup does with SIGHUP, because // catching a signal turns an ignored one back on and would end a run // that was meant to survive it. func Context(parent context.Context) (context.Context, context.CancelFunc) { @@ -37,8 +41,8 @@ func Notify(c chan<- os.Signal) { } } -// caught returns those of SIGINT, SIGTERM and SIGHUP that the tool was -// not started with set to be ignored. +// caught returns SIGINT, SIGTERM and SIGHUP, less any that +// signal.Ignored reports as ignored. func caught() []os.Signal { endings := []os.Signal{syscall.SIGINT, syscall.SIGTERM, syscall.SIGHUP} kept := make([]os.Signal, 0, len(endings)) -- 2.54.0