diff --git a/README.md b/README.md index 3f9ff56..72908e6 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: it removes +the unfinished file it was writing, leaves a file already at the named path as +it was, and exits 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..a1ae4bd 100644 --- a/internal/cli/age/age.go +++ b/internal/cli/age/age.go @@ -3,10 +3,14 @@ package age import ( + "errors" "fmt" "io" "os" + "os/signal" "path/filepath" + "syscall" + "time" "github.com/spf13/cobra" "sneak.berlin/go/keyfunc/internal/agekey" @@ -14,6 +18,18 @@ import ( "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,45 @@ 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) - } +// 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. 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 { + signals := make(chan os.Signal, 1) + signal.Notify(signals, syscall.SIGINT, syscall.SIGTERM, syscall.SIGHUP) - if name == "" { - return cmd.OutOrStdout(), func(failed error) error { - return failed - }, nil - } + defer signal.Stop(signals) // 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 <-signals: + case <-time.After(signalWait): + return finish(file, name, failed) + } + case <-signals: + } + + 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..5793e76 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,102 @@ 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) + } +} + +// interrupted runs "age encrypt -o" or "age decrypt -o", as the +// operation says, as a subprocess writing into a directory of its own +// and reading the input from a pipe. It waits until the tool has begun +// writing the file beside the one it was named, and 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() + + //nolint:gosec // the binary is this test's own, re-run as the tool + command := exec.CommandContext( + t.Context(), os.Args[0], "age", operation, + "-o", filepath.Join(directory, "notes"), + ) + + 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) + + 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) +} + // 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 e49c853..631acae 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/ssh/install.go b/internal/cli/ssh/install.go index 162bd23..5891d39 100644 --- a/internal/cli/ssh/install.go +++ b/internal/cli/ssh/install.go @@ -8,9 +8,11 @@ import ( "fmt" "os" "os/exec" + "os/signal" "path/filepath" "slices" "strings" + "syscall" "github.com/spf13/cobra" ) @@ -62,6 +64,17 @@ 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 := signal.NotifyContext( + cmd.Context(), + syscall.SIGINT, syscall.SIGTERM, syscall.SIGHUP, + ) + 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 6569c5c..b5ecaa9 100644 --- a/internal/cli/ssh/to.go +++ b/internal/cli/ssh/to.go @@ -6,6 +6,7 @@ import ( "fmt" "os" "os/exec" + "os/signal" "slices" "syscall" @@ -42,7 +43,17 @@ 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 := signal.NotifyContext( + cmd.Context(), + syscall.SIGINT, syscall.SIGTERM, syscall.SIGHUP, + ) + defer stop() + + served, err := key.Serve(ctx, comment) if err != nil { return err } @@ -53,7 +64,7 @@ func to() *cobra.Command { "-o", "IdentityAgent=" + served.Socket(), }, args) - return connect(cmd.Context(), argv) + return connect(ctx, argv) }, } @@ -76,7 +87,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())