diff --git a/README.md b/README.md index 98a53ba..ddc228a 100644 --- a/README.md +++ b/README.md @@ -288,6 +288,18 @@ girl mad pet galaxy egg matter matrix prison refuse sense ordinary nose Errors go to standard error and the exit status is 1, except for `ssh to`, which 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). +An interrupted `age encrypt -o` or `age decrypt -o` leaves no file. While it is +writing the file, the signal makes it remove the unfinished file, leave a file +already at the named path as it was, and exit with status 1. It puts the file in +place a tenth of a second after its input ends, and a signal in that time still +counts: Ctrl-C on a pipeline also ends the program feeding it, so the input can +end just before the signal arrives. While `ssh to` or `ssh install` has `ssh` or +`sftp` running, the signal ends that program instead, the tool removes its agent +socket or working files, and it exits with status 1, or for `ssh to` with +`ssh`'s own status if `ssh` reported one. + ## Entrypoints The repo adheres to the diff --git a/internal/cli/age/age.go b/internal/cli/age/age.go index 700fdf2..c5a5506 100644 --- a/internal/cli/age/age.go +++ b/internal/cli/age/age.go @@ -3,17 +3,33 @@ package age import ( + "context" + "errors" "fmt" "io" "os" "path/filepath" + "time" "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" ) +// ErrInterrupted is returned when SIGINT, SIGTERM or SIGHUP came before +// the file --output names was put in place. +var ErrInterrupted = errors.New( + "interrupted by a signal; the output file was left as it was", +) + +// signalWait is how long after the work has ended a signal still keeps +// the new file from being put in place. Ctrl-C on "producer | keyfunc +// age encrypt -o file" ends the producer as well, and the end of the +// input can reach the work a moment before the signal reaches the tool. +const signalWait = 100 * time.Millisecond + // Command returns the age command and everything under it. func Command() *cobra.Command { group := &cobra.Command{ @@ -128,8 +144,9 @@ func runDecrypt(cmd *cobra.Command, args []string) error { return through(cmd, args, key.Decrypt) } -// through opens the input and the output the arguments ask for, hands -// them to the work, and finishes the output afterwards either way. +// through opens the input the arguments ask for and hands it to the +// work, with the file --output names to write to, or the command's own +// output when it names none. func through( cmd *cobra.Command, args []string, work func(io.Writer, io.Reader) error, @@ -141,14 +158,16 @@ func through( defer closeSrc() - dst, done, err := output(cmd) + name, err := cmd.Flags().GetString("output") if err != nil { - return err + return fmt.Errorf("reading the output file: %w", err) } - err = work(dst, src) + if name == "" { + return work(cmd.OutOrStdout(), src) + } - return done(err) + return output(name, src, work) } // input returns what to read from: the named file, or the command's @@ -167,35 +186,44 @@ func input(cmd *cobra.Command, args []string) (io.Reader, func(), error) { return file, func() { _ = file.Close() }, nil } -// output returns what to write to: a new file beside the one --output -// names, or the command's own output when it names none. The second -// result finishes the write, and is given whatever the work returned: -// the new file takes the named file's place only when the work -// succeeded, so a file that is already there survives a run that -// failed. -func output(cmd *cobra.Command) (io.Writer, func(error) error, error) { - name, err := cmd.Flags().GetString("output") - if err != nil { - return nil, nil, fmt.Errorf("reading the output file: %w", err) - } - - if name == "" { - return cmd.OutOrStdout(), func(failed error) error { - return failed - }, nil - } +// output has the work write a new file beside the named one, and puts +// the new file in the named file's place only when the work succeeded, +// so a file that is already there survives a run that failed. +// +// Meanwhile SIGINT, SIGTERM and SIGHUP are caught, as signals.Context +// does. One that comes while the work runs, or within signalWait after +// it has ended, wins: the new file is removed and ErrInterrupted +// returned at once, without waiting for the work, which may be blocked +// reading its input. +func output( + name string, src io.Reader, work func(io.Writer, io.Reader) error, +) error { + interrupted, stop := signals.Context(context.Background()) + defer stop() // The file is made in the same directory so that putting it in // place is a rename and never a copy, and it is readable only by // its owner, which is the mode it keeps once renamed. file, err := os.CreateTemp(filepath.Dir(name), filepath.Base(name)+".") if err != nil { - return nil, nil, fmt.Errorf("creating a file beside %s: %w", name, err) + return fmt.Errorf("creating a file beside %s: %w", name, err) } - return file, func(failed error) error { - return finish(file, name, failed) - }, nil + worked := make(chan error, 1) + + go func() { worked <- work(file, src) }() + + select { + case failed := <-worked: + select { + case <-interrupted.Done(): + case <-time.After(signalWait): + return finish(file, name, failed) + } + case <-interrupted.Done(): + } + + return finish(file, name, ErrInterrupted) } // finish closes the new file and puts it in the named file's place, or diff --git a/internal/cli/age_test.go b/internal/cli/age_test.go index 1b37231..c40fb90 100644 --- a/internal/cli/age_test.go +++ b/internal/cli/age_test.go @@ -1,10 +1,14 @@ package cli_test import ( + "io" "os" + "os/exec" "path/filepath" "strings" + "syscall" "testing" + "time" "github.com/stretchr/testify/require" "sneak.berlin/go/keyfunc/internal/agekey" @@ -90,6 +94,140 @@ func TestARefusedDecryptionLeavesTheOutputFileAlone(t *testing.T) { require.Equal(t, "what was already there\n", string(kept)) } +func TestASignalStopsAnEncryptionAndLeavesNoFile(t *testing.T) { + t.Setenv(mnemonic.Variable, example()) + + for _, ending := range []os.Signal{ + syscall.SIGTERM, syscall.SIGINT, syscall.SIGHUP, + } { + interrupted(t, ending, "encrypt", "the start of the secret\n", false) + } +} + +func TestASignalStopsADecryptionAndLeavesNoFile(t *testing.T) { + t.Setenv(mnemonic.Variable, example()) + + // All of an encryption but its last byte, so the tool reads the + // header and then waits for the rest. + sealed := run(t, "age", "encrypt", written(t, "notes.txt", "the secret\n")) + cut := sealed[:len(sealed)-1] + + for _, ending := range []os.Signal{ + syscall.SIGTERM, syscall.SIGINT, syscall.SIGHUP, + } { + interrupted(t, ending, "decrypt", cut, false) + } +} + +func TestASignalAsTheInputEndsLeavesNoFile(t *testing.T) { + t.Setenv(mnemonic.Variable, example()) + + sealed := run(t, "age", "encrypt", written(t, "notes.txt", "the secret\n")) + + // Ctrl-C on "producer | keyfunc age encrypt -o file" ends the + // producer too, so the input ends just as the signal comes, with + // enough of it in hand for a whole encryption or decryption. Which + // of the two reaches the tool first varies, so it is tried often. + for range 25 { + interrupted(t, syscall.SIGINT, "encrypt", "the start of the secret\n", true) + interrupted(t, syscall.SIGINT, "decrypt", sealed, true) + } +} + +func TestAnEncryptionStartedUnderNohupSurvivesAHangup(t *testing.T) { + t.Setenv(mnemonic.Variable, example()) + + directory := t.TempDir() + named := filepath.Join(directory, "notes") + + // nohup starts the tool with SIGHUP ignored. A tool that caught it + // anyway would turn it back on and be ended by it. + command, producer := writing( + t, directory, "the secret\n", + "nohup", os.Args[0], "age", "encrypt", "-o", named, + ) + + require.NoError(t, command.Process.Signal(syscall.SIGHUP)) + require.NoError(t, producer.Close()) + waitForTool(t, "SIGHUP under nohup", command) + + require.Equal(t, 0, command.ProcessState.ExitCode()) + + left, err := os.ReadDir(directory) + require.NoError(t, err) + require.Len(t, left, 1) + require.Equal(t, "the secret\n", run(t, "age", "decrypt", named)) +} + +// interrupted runs "age encrypt -o" or "age decrypt -o", as the +// operation says, writing into a directory of its own, and once it has +// begun writing sends it the signal, then ends the input if endInput +// says so and otherwise leaves it open. The tool has to end with status +// 1 and leave the directory empty. A tool that went on reading would +// not end until the input did; one that did not remove the file it was +// writing would leave it there, with what it had written so far; one +// that put that file in place because the input ended would leave the +// named file. +func interrupted( + t *testing.T, signal os.Signal, operation, input string, endInput bool, +) { + t.Helper() + + name := operation + " " + signal.String() + directory := t.TempDir() + + command, producer := writing( + t, directory, input, + os.Args[0], "age", operation, "-o", filepath.Join(directory, "notes"), + ) + + require.NoError(t, command.Process.Signal(signal)) + + if endInput { + require.NoError(t, producer.Close()) + } + + waitForTool(t, name, command) + + require.Equal(t, 1, command.ProcessState.ExitCode(), name) + + left, err := os.ReadDir(directory) + require.NoError(t, err) + require.Empty(t, left, name) +} + +// writing starts argv, the tool told to write into directory, as a +// subprocess reading the input from a pipe, and returns once the tool +// has begun writing the file beside the one it was named. The pipe is +// left open for the caller to end. +func writing( + t *testing.T, directory, input string, argv ...string, +) (*exec.Cmd, io.WriteCloser) { + t.Helper() + + //nolint:gosec // this test's own binary as the tool, or nohup running it + command := exec.CommandContext(t.Context(), argv[0], argv[1:]...) + + command.Env = append(os.Environ(), runAsTool+"=1") + + producer, err := command.StdinPipe() + require.NoError(t, err) + require.NoError(t, command.Start()) + + _, err = io.WriteString(producer, input) + require.NoError(t, err) + + // The file beside the named one is made once the mnemonic has been + // read, before any input is. + require.Eventually(t, func() bool { + entries, err := os.ReadDir(directory) + + return err == nil && len(entries) > 0 + }, 5*time.Second, 5*time.Millisecond) + + return command, producer +} + // written puts the contents in a file of that name in a directory of // this test's own and returns the path to it. func written(t *testing.T, name, contents string) string { diff --git a/internal/cli/cli.go b/internal/cli/cli.go index 37ee347..fe4ecb8 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -2,13 +2,10 @@ package cli import ( - "context" "errors" "fmt" "os" - "os/signal" "runtime/debug" - "syscall" "github.com/spf13/cobra" "sneak.berlin/go/keyfunc/internal/cli/age" @@ -70,18 +67,15 @@ func Root() *cobra.Command { // ended with. ssh has already said whatever it had to say in that // case, so nothing more is printed. // -// SIGINT, SIGTERM and SIGHUP cancel the command's context instead of -// killing the process outright, so the child ssh or sftp ends and the -// deferred cleanup that removes the agent socket and the install -// working directory still runs. +// SIGINT, SIGTERM and SIGHUP end the tool at once, as they end any Go +// program, so a command waiting at the mnemonic prompt or reading what +// it encrypts or decrypts goes no further. The exceptions catch the +// signals to clean up first: "ssh to" and "ssh install" while they +// have ssh or sftp running, so the child ends and their own cleanup +// still runs, and "age encrypt -o" and "age decrypt -o" while they +// write, so the unfinished file is removed. func Main() int { - ctx, stop := signal.NotifyContext( - context.Background(), - syscall.SIGINT, syscall.SIGTERM, syscall.SIGHUP, - ) - defer stop() - - err := Root().ExecuteContext(ctx) + err := Root().Execute() if err == nil { return 0 } diff --git a/internal/cli/signals/signals.go b/internal/cli/signals/signals.go new file mode 100644 index 0000000..fdd56a0 --- /dev/null +++ b/internal/cli/signals/signals.go @@ -0,0 +1,34 @@ +// Package signals catches the signals that end the tool, for the +// commands that clean up before they end. +package signals + +import ( + "context" + "os" + "os/signal" + "syscall" +) + +// 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 +// 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) { + endings := []os.Signal{syscall.SIGINT, syscall.SIGTERM, syscall.SIGHUP} + caught := make([]os.Signal, 0, len(endings)) + + for _, ending := range endings { + if !signal.Ignored(ending) { + caught = append(caught, ending) + } + } + + // Given no signals at all, NotifyContext would catch every one. + if len(caught) == 0 { + return context.WithCancel(parent) + } + + return signal.NotifyContext(parent, caught...) +} diff --git a/internal/cli/ssh/install.go b/internal/cli/ssh/install.go index 162bd23..263ed45 100644 --- a/internal/cli/ssh/install.go +++ b/internal/cli/ssh/install.go @@ -13,6 +13,7 @@ import ( "strings" "github.com/spf13/cobra" + "sneak.berlin/go/keyfunc/internal/cli/signals" ) // Where the key goes on the host and what the file it arrives in is @@ -62,6 +63,14 @@ func install() *cobra.Command { return err } + // From here on a signal cancels the context, which + // sftp runs under, instead of ending the tool, so sftp + // ends and the working directory is still removed. + ctx, stop := signals.Context(cmd.Context()) + defer stop() + + cmd.SetContext(ctx) + return add(cmd, args[0], args[1:], line) }, } diff --git a/internal/cli/ssh/to.go b/internal/cli/ssh/to.go index c36ec88..c165796 100644 --- a/internal/cli/ssh/to.go +++ b/internal/cli/ssh/to.go @@ -10,6 +10,7 @@ import ( "syscall" "github.com/spf13/cobra" + "sneak.berlin/go/keyfunc/internal/cli/signals" ) // StatusError says the tool should end with the status ssh ended with. @@ -42,7 +43,14 @@ func to() *cobra.Command { return err } - served, err := key.Serve(cmd.Context(), comment) + // From here until the agent is taken down, a signal + // cancels the context instead of ending the tool, so + // ssh ends and the socket and its directory are still + // removed. + ctx, stop := signals.Context(cmd.Context()) + defer stop() + + served, err := key.Serve(ctx, comment) if err != nil { return err } @@ -53,7 +61,7 @@ func to() *cobra.Command { "-o", "IdentityAgent=" + served.Socket(), }, args) - return connect(cmd.Context(), argv) + return connect(ctx, argv) }, } @@ -76,7 +84,7 @@ func connect(ctx context.Context, argv []string) error { command.Stdout = os.Stdout command.Stderr = os.Stderr - // A cancelled context means a signal ended the tool. Send ssh a + // A cancelled context means a signal arrived. Send ssh a // SIGTERM rather than the default kill, so it puts the terminal // back the way it found it before it goes. command.Cancel = func() error { diff --git a/internal/cli/ssh_test.go b/internal/cli/ssh_test.go index 04749f4..4776237 100644 --- a/internal/cli/ssh_test.go +++ b/internal/cli/ssh_test.go @@ -20,13 +20,13 @@ import ( // runAsTool, set in the environment of a re-executed test binary, tells // TestMain to run the tool through Main rather than the suite, so the -// signal test can drive the real signal path in a process it can send a -// signal to. +// signal tests can drive the real signal path in a process they can +// send a signal to. const runAsTool = "KEYFUNC_TEST_RUN_AS_TOOL" // TestMain re-executes the test binary as the tool when runAsTool is -// set, and otherwise runs the suite. The signal test starts the tool -// this way, as a subprocess it can signal and watch clean up. +// set, and otherwise runs the suite. The signal tests start the tool +// this way, as a subprocess they can signal and watch end. func TestMain(m *testing.M) { if os.Getenv(runAsTool) == "1" { os.Exit(cli.Main()) @@ -209,6 +209,16 @@ fi sleep 5 ` +// stalled is a stand-in for the system sftp that notes it has started +// and then blocks, so a test can signal the tool while sftp is running +// and watch it remove its working directory. The exec keeps the shell +// from leaving a sleep behind that holds the output the tool reads sftp +// through. +const stalled = ` +touch "$KEYFUNC_TEST_STARTED" +exec sleep 5 +` + // pretended is where a stand-in writes down what it was asked to do. type pretended struct { // home stands in for the home directory on the host. @@ -544,7 +554,7 @@ func TestASignalTakesTheAgentDirectoryDown(t *testing.T) { // ssh that blocks, waits until the agent is up and ssh is running // against it, sends the tool the signal, and requires the agent socket // and its directory to be gone once the tool has ended. The subprocess -// goes through Main and its signal handling, so with that handling +// goes through Main and the command's signal handling, so with that handling // removed the signal kills the tool outright, no deferred cleanup runs, // the directory is left behind, and the check fails. func signalEndsTheTool(t *testing.T, name string, signal os.Signal) { @@ -611,6 +621,56 @@ func waitForSocket(t *testing.T, noted string) string { return socket } +func TestASignalTakesTheInstallWorkingDirectoryDown(t *testing.T) { + t.Setenv(mnemonic.Variable, example()) + + for _, ending := range []os.Signal{ + syscall.SIGTERM, syscall.SIGINT, syscall.SIGHUP, + } { + signalEndsTheInstall(t, ending.String(), ending) + } +} + +// signalEndsTheInstall runs "ssh install" as a subprocess against a +// stand-in sftp that blocks, with a temporary directory of the test's +// own, waits until sftp is running, sends the tool the signal, and +// requires the working directory the tool made there to be gone once +// the tool has ended. +func signalEndsTheInstall(t *testing.T, name string, signal os.Signal) { + t.Helper() + + temporary := t.TempDir() + started := filepath.Join(t.TempDir(), "started") + t.Setenv("KEYFUNC_TEST_STARTED", started) + standIn(t, "sftp", stalled) + + //nolint:gosec // the binary is this test's own, re-run as the tool + command := exec.CommandContext( + t.Context(), os.Args[0], subcommand, installing, host, + ) + + command.Env = append(os.Environ(), runAsTool+"=1", "TMPDIR="+temporary) + require.NoError(t, command.Start()) + + // sftp is started only once the working directory has been made. + require.Eventually(t, func() bool { + _, err := os.Stat(started) + + return err == nil + }, 5*time.Second, 5*time.Millisecond) + + working, err := os.ReadDir(temporary) + require.NoError(t, err) + require.Len(t, working, 1, name) + + require.NoError(t, command.Process.Signal(signal)) + waitForTool(t, name, command) + + left, err := os.ReadDir(temporary) + require.NoError(t, err) + require.Empty(t, left, name) +} + func TestTheMnemonicIsNotHandedToSFTP(t *testing.T) { t.Setenv(mnemonic.CommandVariable, "echo "+example()) t.Setenv(mnemonic.Variable, example())