From 8bb3d949d9f83c9956c3dc0b212c29393af24c4a Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Thu, 15 Oct 2026 01:35:42 +0200 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 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. Untested: a signal that comes just as the line is read. 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))