From 3b6e987b90fc4864e34a1719a82e96fe1fe938e8 Mon Sep 17 00:00:00 2001 From: Geno Roupsky <185580+groupsky@users.noreply.github.com> Date: Wed, 11 Nov 2026 03:35:53 +0200 Subject: [PATCH 1/6] feat(run): start --mask (refs #18) From 8247b8d52f02dafcb82a57c3dd74dd6b2f2c4171 Mon Sep 17 00:00:00 2001 From: Geno Roupsky <185580+groupsky@users.noreply.github.com> Date: Wed, 11 Nov 2026 03:37:24 +0200 Subject: [PATCH 2/6] feat(run): a writer that masks values, also split across writes (refs #18) --- internal/client/mask.go | 128 +++++++++++++++++++++++++++++++++++ internal/client/mask_test.go | 126 ++++++++++++++++++++++++++++++++++ 2 files changed, 254 insertions(+) create mode 100644 internal/client/mask.go create mode 100644 internal/client/mask_test.go diff --git a/internal/client/mask.go b/internal/client/mask.go new file mode 100644 index 0000000..6dc69da --- /dev/null +++ b/internal/client/mask.go @@ -0,0 +1,128 @@ +package client + +import ( + "bytes" + "io" + "sort" +) + +// maskText replaces each fetched value in the output of the command +// (nodkey run --mask, DESIGN.md section 5). +const maskText = "***" + +// maskWriter writes to w what is written to it, with each value replaced +// by maskText. A value may be split across writes: maskWriter holds back +// the end of the data while it may still be the start of a value, and +// writes it with the next write, or with Close. At one place, the longest +// value that matches wins. maskWriter never writes a part of a value. +// It is not safe for use by more than one goroutine. +type maskWriter struct { + w io.Writer + values [][]byte // longest first; none is empty + first [256]bool + buf []byte // data that may still be the start of a value +} + +// newMaskWriter returns a maskWriter for the values. Empty values are +// left out. It keeps its own copy of the values; clear wipes it. +func newMaskWriter(w io.Writer, values [][]byte) *maskWriter { + m := &maskWriter{w: w} + for _, v := range values { + if len(v) == 0 { + continue + } + c := bytes.Clone(v) + m.values = append(m.values, c) + m.first[c[0]] = true + } + sort.SliceStable(m.values, func(i, j int) bool { return len(m.values[i]) > len(m.values[j]) }) + return m +} + +// Write masks p and writes all of it that cannot be the start of a value. +// It reports len(p) when the writes to w succeed. +func (m *maskWriter) Write(p []byte) (int, error) { + m.buf = append(m.buf, p...) + if err := m.flush(false); err != nil { + return 0, err + } + return len(p), nil +} + +// Close writes the data that is held back, masked. It does not close w. +func (m *maskWriter) Close() error { + return m.flush(true) +} + +// flush writes m.buf to w, masked. Unless final, it keeps the end of +// m.buf that is the start of a longer value. +func (m *maskWriter) flush(final bool) error { + var out []byte + b := m.buf + i := 0 + for i < len(b) { + // Copy the bytes that cannot start a value as they are. + j := i + for j < len(b) && !m.first[b[j]] { + j++ + } + out = append(out, b[i:j]...) + if i = j; i == len(b) { + break + } + rest := b[i:] + if !final && m.mayGrow(rest) { + break + } + if n := m.match(rest); n > 0 { + out = append(out, maskText...) + i += n + continue + } + out = append(out, b[i]) + i++ + } + // Keep the held bytes at the start of buf; wipe the rest of it. + held := copy(m.buf, b[i:]) + clear(m.buf[held:]) + m.buf = m.buf[:held] + if len(out) == 0 { + return nil + } + _, err := m.w.Write(out) + clear(out) + return err +} + +// mayGrow reports whether rest is a start of a value longer than rest, +// so that more data may make it a value. +func (m *maskWriter) mayGrow(rest []byte) bool { + for _, v := range m.values { + if len(v) <= len(rest) { + return false // the rest are shorter still + } + if bytes.HasPrefix(v, rest) { + return true + } + } + return false +} + +// match returns the length of the longest value at the start of rest, or 0. +func (m *maskWriter) match(rest []byte) int { + for _, v := range m.values { + if bytes.HasPrefix(rest, v) { + return len(v) + } + } + return 0 +} + +// clear wipes the copies of the values and the held data. +func (m *maskWriter) clear() { + for _, v := range m.values { + clear(v) + } + clear(m.buf) + m.buf = m.buf[:0] +} diff --git a/internal/client/mask_test.go b/internal/client/mask_test.go new file mode 100644 index 0000000..db32836 --- /dev/null +++ b/internal/client/mask_test.go @@ -0,0 +1,126 @@ +package client + +import ( + "bytes" + "crypto/rand" + "errors" + "testing" +) + +// maskAll writes the chunks to a new maskWriter for values, closes it, and +// returns what it wrote. +func maskAll(t *testing.T, values []string, chunks ...[]byte) string { + t.Helper() + var out bytes.Buffer + vs := make([][]byte, len(values)) + for i, v := range values { + vs[i] = []byte(v) + } + m := newMaskWriter(&out, vs) + for _, c := range chunks { + n, err := m.Write(c) + if err != nil || n != len(c) { + t.Fatalf("Write = %d, %v; want %d, nil", n, err, len(c)) + } + } + if err := m.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + return out.String() +} + +func TestMaskWriter(t *testing.T) { + const canary = "CANARY-7f3a9c" + tests := []struct { + name string + values []string + chunks []string + want string + }{ + {"one value", []string{canary}, []string{"token=" + canary + "\n"}, "token=***\n"}, + {"every time", []string{canary}, []string{canary + canary + " " + canary}, "****** ***"}, + {"no value", []string{canary}, []string{"plain text\n"}, "plain text\n"}, + {"two values", []string{"alpha", "beta"}, []string{"alpha beta gamma"}, "*** *** gamma"}, + {"longest wins", []string{"abc", "abcdef"}, []string{"xabcdefx abcx"}, "x***x ***x"}, + {"split in two", []string{canary}, []string{"a CAN", "ARY-7f3a9c b"}, "a *** b"}, + {"split byte by byte", []string{canary}, split("<" + canary + ">"), "<***>"}, + {"a start that is not a value", []string{canary}, []string{"CANARY-7", "x CANARY"}, "CANARY-7x CANARY"}, + {"a start at the end", []string{canary}, []string{"end CANARY-7f3a9"}, "end CANARY-7f3a9"}, + {"a value inside a false start", []string{"aab"}, []string{"a", "a", "aab"}, "aa***"}, + {"overlap", []string{"aba"}, []string{"ababa"}, "***ba"}, + {"a longer value split, a shorter one whole", []string{"ab", "abcd"}, []string{"ab", "cd ab", "c"}, "*** ***c"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + chunks := make([][]byte, len(tt.chunks)) + for i, c := range tt.chunks { + chunks[i] = []byte(c) + } + if got := maskAll(t, tt.values, chunks...); got != tt.want { + t.Errorf("got %q, want %q", got, tt.want) + } + }) + } +} + +func split(s string) []string { + out := make([]string, len(s)) + for i := range s { + out[i] = s[i : i+1] + } + return out +} + +// TestMaskWriterBinary checks binary output with a binary value, split at +// every place: the value never comes out, and every other byte does. +func TestMaskWriterBinary(t *testing.T) { + value := make([]byte, 40) + _, _ = rand.Read(value) + value[0] = 0xff // also a byte that is not valid UTF-8 + noise := make([]byte, 300) + _, _ = rand.Read(noise) + noise = bytes.ReplaceAll(noise, value[:1], []byte{0}) // no false start + var in []byte + in = append(in, noise[:100]...) + in = append(in, value...) + in = append(in, noise[100:200]...) + in = append(in, value...) + in = append(in, noise[200:]...) + want := string(noise[:100]) + maskText + string(noise[100:200]) + maskText + string(noise[200:]) + for cut := 0; cut <= len(in); cut++ { + got := maskAll(t, []string{string(value)}, in[:cut], in[cut:]) + if got != want { + t.Fatalf("cut at %d: the output is not the input with the value masked", cut) + } + if bytes.Contains([]byte(got), value) { + t.Fatalf("cut at %d: the value is in the output", cut) + } + } +} + +// TestMaskWriterHoldsBack checks that a start of a value is not written +// before it is known not to be the value. +func TestMaskWriterHoldsBack(t *testing.T) { + var out bytes.Buffer + m := newMaskWriter(&out, [][]byte{[]byte("secret"), nil}) + _, _ = m.Write([]byte("a secr")) + if got := out.String(); got != "a " { + t.Fatalf("after a start of the value, the output is %q, want %q", got, "a ") + } + _, _ = m.Write([]byte("et b")) + if got := out.String(); got != "a *** b" { + t.Fatalf("got %q, want %q", got, "a *** b") + } + m.clear() +} + +type failWriter struct{} + +func (failWriter) Write([]byte) (int, error) { return 0, errors.New("broken pipe") } + +func TestMaskWriterError(t *testing.T) { + m := newMaskWriter(failWriter{}, [][]byte{[]byte("secret")}) + if _, err := m.Write([]byte("x")); err == nil { + t.Fatal("Write to a broken writer: no error") + } +} From a90ec36610b3ad2c4d3e2617333d35346cca7897 Mon Sep 17 00:00:00 2001 From: Geno Roupsky <185580+groupsky@users.noreply.github.com> Date: Wed, 11 Nov 2026 03:40:51 +0200 Subject: [PATCH 3/6] feat(run): --mask runs the command as a child and masks its output (refs #18) --- internal/client/child_linux_test.go | 1 + internal/client/helpers_test.go | 4 + internal/client/request_test.go | 9 ++ internal/client/run.go | 41 +++++++- internal/client/runmask.go | 97 ++++++++++++++++++ internal/client/runmask_test.go | 149 ++++++++++++++++++++++++++++ 6 files changed, 296 insertions(+), 5 deletions(-) create mode 100644 internal/client/runmask.go create mode 100644 internal/client/runmask_test.go diff --git a/internal/client/child_linux_test.go b/internal/client/child_linux_test.go index d28aa42..e8777d2 100644 --- a/internal/client/child_linux_test.go +++ b/internal/client/child_linux_test.go @@ -85,6 +85,7 @@ func childRun(args []string) int { environ: os.Environ, exec: checkedExec, notDumpable: notDumpable, + masked: runMasked, } return r.main(args, os.Stderr) //nolint:forbidigo // the child is nodkey run, which writes to stderr. } diff --git a/internal/client/helpers_test.go b/internal/client/helpers_test.go index 004e438..687700c 100644 --- a/internal/client/helpers_test.go +++ b/internal/client/helpers_test.go @@ -13,6 +13,7 @@ import ( "encoding/pem" "errors" "fmt" + "io" "math/big" "net" "net/http" @@ -473,6 +474,9 @@ func testRunner(b *Broker, env []string, fx *fakeExec) runner { // The test process must stay dumpable; TestRunNotDumpable checks // the real one in a child process. notDumpable: func() error { return nil }, + masked: func(string, []string, []string, [][]byte, io.Writer) int { + panic("this test must not run a command with --mask") + }, } } diff --git a/internal/client/request_test.go b/internal/client/request_test.go index de26d03..e30a354 100644 --- a/internal/client/request_test.go +++ b/internal/client/request_test.go @@ -45,6 +45,8 @@ func TestRunUsage(t *testing.T) { "the variable GH_TOKEN is given twice"}, {"too many", append(thirtyThree, "--", "gh"), "at most 32 --secret options"}, {"two reasons", []string{"--reason", "a", "--reason=b", "--secret", "A=b", "--", "gh"}, "--reason is given twice"}, + {"a value for --mask", []string{"--secret", "A=b", "--mask=yes", "--", "gh"}, "--mask takes no value"}, + {"two masks", []string{"--mask", "--secret", "A=b", "-mask", "--", "gh"}, "--mask is given twice"}, {"unknown option", []string{"--secret", "A=b", "--bogus", "--", "gh"}, `unknown option "--bogus"`}, {"one dash", []string{"-", "--", "gh"}, `unknown option "-"`}, {"a line break", []string{"--secret", "A\nB=c", "--", "gh"}, `the variable name "A\nB" must match`}, @@ -138,6 +140,13 @@ func TestParseArgs(t *testing.T) { !slices.Equal(o.command, []string{"gh", "--secret", "--", "x"}) { t.Errorf("parseArgs = %+v", o) } + if o.mask { + t.Error("parseArgs without --mask: mask is on") + } + if o, err := parseArgs([]string{"--secret", "A=a", "-mask", "--", "gh", "--mask"}); err != nil || !o.mask || + !slices.Equal(o.command, []string{"gh", "--mask"}) { + t.Errorf("--mask: %+v, %v", o, err) + } o, err = parseArgs([]string{"--reason", "--", "--secret", "A=a", "--", "gh"}) if err != nil || o.reason != "--" || len(o.secrets) != 1 || !slices.Equal(o.command, []string{"gh"}) { t.Errorf("a reason that is \"--\": %+v, %v", o, err) diff --git a/internal/client/run.go b/internal/client/run.go index daa9a91..72f2a60 100644 --- a/internal/client/run.go +++ b/internal/client/run.go @@ -38,7 +38,7 @@ const ( ) // runUsage is the help of nodkey run. -const runUsage = `Usage: nodkey run [--secret VAR=NAME]... [--grant-token FILE] [--reason TEXT] -- COMMAND [ARGS...] +const runUsage = `Usage: nodkey run [--secret VAR=NAME]... [--grant-token FILE] [--reason TEXT] [--mask] -- COMMAND [ARGS...] Ask the broker for secrets, and wait until a person approves the request on the phone. Then run COMMAND, with the value of the secret NAME in the @@ -56,6 +56,10 @@ environment variable VAR. nodkey never prints a value. saved, COMMAND does not run (exit 77). --reason TEXT why COMMAND needs the secrets. Without it, the reason is the environment variable NODKEY_REASON. + --mask replace each value in the stdout and stderr of COMMAND + with ***. COMMAND then runs as a child of nodkey, and + its stdout and stderr are pipes, not a terminal. This + catches only a value as it is, not in another form. The client config is ~/.config/nodkey/client.toml. nodkey run exits with the exit code of COMMAND, or with 64 (wrong usage), 69 (the broker cannot @@ -88,7 +92,7 @@ const reasonVar = "NODKEY_REASON" // any Go program. func Run(args []string, stderr io.Writer, version string) int { r := runner{load: Load, configPath: ConfigPath, environ: os.Environ, exec: syscall.Exec, - notDumpable: notDumpable, version: version} + notDumpable: notDumpable, masked: runMasked, version: version} return r.main(args, stderr) } @@ -97,7 +101,7 @@ func Run(args []string, stderr io.Writer, version string) int { // never reads ~/.config/nodkey/client.toml. func RunWithConfig(args []string, stderr io.Writer, version, path string) int { r := runner{load: Load, configPath: func() (string, error) { return path, nil }, environ: os.Environ, - exec: syscall.Exec, notDumpable: notDumpable, version: version} + exec: syscall.Exec, notDumpable: notDumpable, masked: runMasked, version: version} return r.main(args, stderr) } @@ -117,6 +121,9 @@ type runner struct { // values: notDumpable. Tests in the test process replace it, so the // test process stays as it is. notDumpable func() error + // masked runs the command for --mask, with the values to mask, and + // returns the exit code: runMasked. + masked func(path string, argv, envv []string, values [][]byte, stderr io.Writer) int // version is the version of nodkey (warnVersions); "" sends none. version string } @@ -216,6 +223,12 @@ func (r runner) run(ctx context.Context, args []string, w io.Writer, beforeExec return fail(stderr, exitUnavailable, err.Error()) } envv, err := commandEnv(env, o.secrets, ans) + var values [][]byte + if err == nil && o.mask { + for _, s := range ans.Secrets { + values = append(values, bytes.Clone(s.Value)) + } + } ans.Clear() // best effort: envv holds the values as Go strings if err != nil { // The person approved, and the broker sent a wrong answer: as for @@ -226,6 +239,13 @@ func (r runner) run(ctx context.Context, args []string, w io.Writer, beforeExec if ctx.Err() != nil { return cancelled(ctx, stderr) } + if o.mask { + code := r.masked(path, o.command, envv, values, stderr) + for _, v := range values { + clear(v) + } + return code + } if err := r.exec(path, o.command, envv); err != nil { return fail(stderr, exitCannotRun, fmt.Sprintf("cannot run %q: %v", path, err)) } @@ -343,11 +363,13 @@ type runOptions struct { reasonSet bool // grantToken is the file of --grant-token ("" for none). grantToken string - command []string + // mask is --mask: the command's output is masked. + mask bool + command []string } // parseArgs reads the arguments of nodkey run: the options, "--", and the -// command. Options are --secret VAR=NAME, which may repeat, and --reason +// command. Options are --secret VAR=NAME, which may repeat, --mask, and --reason // TEXT, each also with one dash and with "=" before the value, as the flag // package reads them. The command must come after "--", so that its own // options are never read as options of nodkey run. @@ -375,6 +397,15 @@ func parseArgs(args []string) (*runOptions, error) { name, value, hasValue := splitOption(arg) switch name { case "secret", "reason", "grant-token": + case "mask": + switch { + case hasValue: + return nil, errors.New("--mask takes no value") + case o.mask: + return nil, errors.New("--mask is given twice") + } + o.mask = true + continue case "h", "help": // With other arguments, help would exit with 0 and run // nothing, which looks like a command that ran. diff --git a/internal/client/runmask.go b/internal/client/runmask.go new file mode 100644 index 0000000..49cb277 --- /dev/null +++ b/internal/client/runmask.go @@ -0,0 +1,97 @@ +package client + +import ( + "errors" + "fmt" + "io" + "os" + "os/exec" + "os/signal" + "syscall" +) + +// runMasked runs the command for nodkey run --mask, and returns the exit +// code of nodkey run. The command is a child of nodkey, not a replacement: +// its stdout and stderr go through a maskWriter each, to nodkey's stdout +// and stderr (DESIGN.md section 5). When the command dies of a signal, +// nodkey ends itself with the same signal, so that a shell sees it. +func runMasked(path string, argv, envv []string, values [][]byte, stderr io.Writer) int { + code, sig, err := maskedCommand(path, argv, envv, values, os.Stdout, stderr) //nolint:forbidigo // the command's own stdout. + if err != nil { + return fail(stderr, exitCannotRun, err.Error()) + } + if sig != 0 { + raise(sig) + } + return code +} + +// maskedCommand starts the command with stdin as nodkey's, and its stdout +// and stderr masked to stdout and stderr. It waits until the command ended +// and its output is all written. It returns the exit code of the command, +// with the signal that ended it (and 128 and its number as the code), or +// an error when the command could not start. +// +// While the command runs, nodkey passes on SIGTERM and SIGHUP to it. It +// catches SIGINT and SIGQUIT and does nothing with them: a terminal sends +// them to the command too, as the command is in the same process group. +// A signal that nodkey ignores stays ignored, also in the command. +func maskedCommand(path string, argv, envv []string, values [][]byte, stdout, stderr io.Writer) (int, syscall.Signal, error) { + outMask := newMaskWriter(stdout, values) + errMask := newMaskWriter(stderr, values) + defer outMask.clear() + defer errMask.clear() + cmd := &exec.Cmd{ + Path: path, + Args: argv, + Env: envv, + Stdin: os.Stdin, // the command reads nodkey's stdin, as after execve + Stdout: outMask, + Stderr: errMask, + } + sigs := make(chan os.Signal, 4) + var caught []os.Signal + for _, s := range []os.Signal{syscall.SIGTERM, syscall.SIGHUP, os.Interrupt, syscall.SIGQUIT} { + if !signal.Ignored(s) { + caught = append(caught, s) + } + } + signal.Notify(sigs, caught...) + defer signal.Stop(sigs) + if err := cmd.Start(); err != nil { + return 0, 0, fmt.Errorf("cannot run %q: %w", path, err) + } + done := make(chan struct{}) + go func() { + for { + select { + case s := <-sigs: + if s == syscall.SIGTERM || s == syscall.SIGHUP { + _ = cmd.Process.Signal(s) + } + case <-done: + return + } + } + }() + err := cmd.Wait() + close(done) + // Write what is held back. A write that fails cannot be reported to + // anyone: the command's own exit code stays. + _ = outMask.Close() + _ = errMask.Close() + st := cmd.ProcessState + if st == nil { + return 0, 0, fmt.Errorf("cannot run %q: %w", path, err) + } + if ws, ok := st.Sys().(syscall.WaitStatus); ok && ws.Signaled() { + return 128 + int(ws.Signal()), ws.Signal(), nil + } + var ee *exec.ExitError + if err != nil && !errors.As(err, &ee) && st.ExitCode() == 0 { + // The command exited with 0, and its output could not be written + // (such as a closed stdout): not a success. + return 1, 0, nil + } + return st.ExitCode(), 0, nil +} diff --git a/internal/client/runmask_test.go b/internal/client/runmask_test.go new file mode 100644 index 0000000..3d34a65 --- /dev/null +++ b/internal/client/runmask_test.go @@ -0,0 +1,149 @@ +package client + +import ( + "bytes" + "io" + "os" + "path/filepath" + "slices" + "strings" + "syscall" + "testing" + + "github.com/code6x/nodkey/internal/audit" +) + +// TestRunMask: with --mask, nodkey run gives the command and the values +// to masked, and does not replace itself. It returns masked's code. +func TestRunMask(t *testing.T) { + t.Parallel() + tb := newTestBroker(t) + tb.approver.Auto(audit.AnswerApproveOnce) + fx := &fakeExec{} + r := testRunner(tb.client(t), []string{"PATH=/usr/bin"}, fx) + var gotValues [][]byte + var gotArgv []string + r.masked = func(path string, argv, envv []string, values [][]byte, stderr io.Writer) int { + gotArgv = slices.Clone(argv) + for _, v := range values { + gotValues = append(gotValues, bytes.Clone(v)) + } + return 7 + } + cmd := self(t) + code, stderr := runIn(t.Context(), r, "--secret", "GH_TOKEN=github-token", "--secret", "NPM_TOKEN=npm-token", + "--mask", "--", cmd, "x") + if code != 7 { + t.Fatalf("exit code %d, want 7 (the command's); stderr %q", code, safe(stderr)) + } + if len(fx.ran()) != 0 { + t.Error("with --mask, nodkey replaced itself with the command") + } + if !slices.Equal(gotArgv, []string{cmd, "x"}) { + t.Errorf("masked got the arguments %q", gotArgv) + } + if len(gotValues) != 2 || string(gotValues[0]) != tb.values["github-token"] || + string(gotValues[1]) != tb.values["npm-token"] { + t.Errorf("masked got %d values, or not the fetched ones", len(gotValues)) + } +} + +// masked runs maskedCommand with values and a shell script, and returns +// the exit code, the signal, stdout and stderr. +func masked(t *testing.T, values []string, script string, args ...string) (int, syscall.Signal, string, string) { + t.Helper() + var stdout, stderr bytes.Buffer + vs := make([][]byte, len(values)) + env := []string{"PATH=/usr/bin:/bin"} + for i, v := range values { + vs[i] = []byte(v) + env = append(env, "V"+itoa(i)+"="+v) + } + argv := append([]string{"sh", "-c", script, "sh"}, args...) + code, sig, err := maskedCommand("/bin/sh", argv, env, vs, &stdout, &stderr) + if err != nil { + t.Fatalf("maskedCommand: %v", err) + } + return code, sig, stdout.String(), stderr.String() +} + +func TestMaskedCommand(t *testing.T) { + t.Parallel() + canary := madeUp("canary") + other := madeUp("other") + code, sig, out, errOut := masked(t, []string{canary, other}, + `printf 'value=%s\n' "$V0"; printf '%s%s' "$V1" "$V0" >&2; exit 3`) + if code != 3 || sig != 0 { + t.Errorf("exit %d, signal %d; want 3, 0", code, sig) + } + if out != "value=***\n" || errOut != "******" { + t.Errorf("stdout %q, stderr %q; want %q and %q", safe(out), safe(errOut), "value=***\n", "******") + } + mustNotLeak(t, canary, out, errOut) + mustNotLeak(t, other, out, errOut) +} + +// TestMaskedCommandSplit: the command writes the value in two writes, with +// a pause between them, so nodkey reads it in two parts. +func TestMaskedCommandSplit(t *testing.T) { + t.Parallel() + canary := madeUp("canary") + half := len(canary) / 2 + _, _, out, _ := masked(t, []string{canary}, `printf '<%s' "$1"; sleep 0.2; printf '%s>' "$2"`, + canary[:half], canary[half:]) + if out != "<***>" { + t.Errorf("stdout %q, want %q", safe(out), "<***>") + } + mustNotLeak(t, canary, out) +} + +// TestMaskedCommandBinary: binary output, with bytes that are not text and +// a value with such bytes, comes out as it is, with the value masked. +func TestMaskedCommandBinary(t *testing.T) { + t.Parallel() + value := "\xff\xfe\x01secret\x80" + code, _, out, _ := masked(t, []string{value}, `printf '\000\377\200'; printf '%s' "$V0"; printf '\000\n\377'`) + if code != 0 { + t.Fatalf("exit %d", code) + } + if want := "\x00\xff\x80***\x00\n\xff"; out != want { + t.Errorf("stdout %q, want %q", out, want) + } +} + +// TestMaskedCommandSignal: a command that dies of a signal gives 128 and +// its number, and the signal, which nodkey then raises. +func TestMaskedCommandSignal(t *testing.T) { + t.Parallel() + code, sig, _, _ := masked(t, []string{"x"}, `kill -TERM $$`) + if code != 128+int(syscall.SIGTERM) || sig != syscall.SIGTERM { + t.Errorf("exit %d, signal %d; want %d, %d", code, sig, 128+int(syscall.SIGTERM), syscall.SIGTERM) + } +} + +// TestMaskedCommandCannotStart: a file that cannot be run gives an error +// with its path, and nodkey run exits with 126. +func TestMaskedCommandCannotStart(t *testing.T) { + t.Parallel() + path := filepath.Join(t.TempDir(), "not-a-program") + if err := os.WriteFile(path, []byte("data"), 0o600); err != nil { + t.Fatal(err) + } + var stderr bytes.Buffer + code := runMasked(path, []string{path}, nil, [][]byte{[]byte("x")}, &stderr) + if code != exitCannotRun || !strings.Contains(stderr.String(), "cannot run") || + !strings.Contains(stderr.String(), "(exit 126)") { + t.Errorf("exit %d, stderr %q", code, stderr.String()) + } +} + +// TestMaskedCommandOutputFails: a command that exits with 0 while its +// output cannot be written does not count as a success. +func TestMaskedCommandOutputFails(t *testing.T) { + t.Parallel() + code, _, err := maskedCommand("/bin/sh", []string{"sh", "-c", "echo hello"}, nil, [][]byte{[]byte("x")}, + failWriter{}, io.Discard) + if err != nil || code == 0 { + t.Errorf("exit %d, %v; want a code that is not 0", code, err) + } +} From ae308d5c9ae78e50442cf6980293d31da0394b28 Mon Sep 17 00:00:00 2001 From: Geno Roupsky <185580+groupsky@users.noreply.github.com> Date: Wed, 11 Nov 2026 03:41:30 +0200 Subject: [PATCH 4/6] docs: nodkey run --mask in the design, the threat model and the setup guide (refs #18) --- DESIGN.md | 36 +++++++++++++++++++++++++++++++++--- THREAT-MODEL.md | 9 +++++++-- docs/setup.md | 6 ++++++ 3 files changed, 46 insertions(+), 5 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index a121e47..60eab54 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -67,7 +67,9 @@ what it protects against, and what it does not. - SSH certificates, for hosts and for users. The broker's `known_hosts` holds plain host keys, and the broker signs only with plain keys. - Keys in hardware (TPM or YubiKey) through an upstream agent. -- Filtering secrets out of the command's output. +- Masking a value in another form in the command's output, such as in + base64 or split between stdout and stderr (`nodkey run --mask` masks it + only as it is, section 5). - More than one approver, and Telegram and ntfy on one broker at once. Each of these can become a GitHub issue later. @@ -260,6 +262,26 @@ nodkey run --secret GH_TOKEN=github-token \ memory with the values. The command's exit code is the exit code of `nodkey run`. The client never ignores `SIGPIPE`: an ignored signal stays ignored after `execve`, so the command would run with it ignored. + With `--mask`, the client does not replace itself. It starts the + command as its child, with the same environment and stdin, and the + command's stdout and stderr are pipes to the client. The client writes + all of the output to its own stdout and stderr, and replaces each + fetched value in it with `***`, also when a value comes in more than + one read. It holds back only the end of the output that may still be + the start of a value, and writes it when the next read shows that it is + not a value, or when the command ends. Where two values match at one + place, the longest wins. It masks a value only exactly as it is: not in + base64, not split between stdout and stderr, and not in another + encoding. It waits until the command has ended and the pipes are closed + (a process that the command left behind may keep them open), then exits + with the command's exit code. A command that exits with 0 when its + output cannot be written gives 1. When the command dies of a signal, + the client ends itself with the same signal. While the command runs, + the client stays not dumpable and holds the values. It passes `SIGTERM` + and `SIGHUP` on to the command, and catches `SIGINT` and `SIGQUIT` + without passing them on, because a terminal sends them to the command + too. The command sees pipes, not a terminal, so some programs print + without colors or buffer their output. 8. On deny, timeout or error, the client prints one line to stderr, without any value, and exits: with 77 when the broker denies the request, and with 69 when it cannot reach the broker or gets another answer @@ -3073,7 +3095,7 @@ other users must not be able to change it. ``` nodkey broker [--config FILE] -nodkey run [--secret VAR=NAME]... [--grant-token FILE] [--reason TEXT] -- COMMAND [ARGS...] +nodkey run [--secret VAR=NAME]... [--grant-token FILE] [--reason TEXT] [--mask] -- COMMAND [ARGS...] nodkey ssh-agent [--socket PATH] nodkey ssh-grant --key NAME --user USER --host NAME... --for DURATION [--reason TEXT] nodkey preapprove (--secret NAME... | --key NAME --user USER --host NAME...) --until TIME --uses N [--from TIME] [--reason TEXT] @@ -3288,6 +3310,12 @@ a command dies of `SIGINT`; with a plain exit code, one Ctrl-C would end only one `nodkey run`, and the next one would send its request to the phone. Once the command starts, the signals are the command's. +`--mask` (section 5, step 7): the command runs as a child of nodkey, and +each fetched value in its stdout and stderr becomes `***`. `--mask` takes +no value and may be given once. With it, 126 also covers a command that +cannot be started as a child, and a command that dies of a signal ends +nodkey with the same signal. + `nodkey run -h` or `nodkey run --help` prints the usage and exits with 0, but only as the only argument. With other arguments, `-h` and `--help` are wrong usage (64), with the one line above: an exit with 0 that runs nothing would look like a @@ -3783,7 +3811,9 @@ name would not work for the whole update. Use `HTTPS_PROXY`. sends its request, so it writes no core dump while it holds the values, also when the agent sets `GOTRACEBACK`, and other processes of the user cannot read its memory. `execve` makes the command dumpable again. It - does not change `RLIMIT_CORE`, which the command would inherit. + does not change `RLIMIT_CORE`, which the command would inherit. With + `--mask`, nodkey stays not dumpable while the command runs, as it holds + the values to mask. - Time limits use Go's monotonic clock, so setting the wall clock does not move them. The monotonic clock stops while the host sleeps (suspend), so a request and a grant also end when the wall clock reaches their end: diff --git a/THREAT-MODEL.md b/THREAT-MODEL.md index 2f78783..7f6ff13 100644 --- a/THREAT-MODEL.md +++ b/THREAT-MODEL.md @@ -433,8 +433,13 @@ common forms only. A secret in another form is shown as it is, for example a plain word (`htpasswd -b users.txt dev SECRET`) or an option of a program the broker does not know. A hostile agent can always get a secret past masking. The -claimed text stays a claim. The command itself may print a secret; version -1 does not filter the command's output. The deny list of variable names +claimed text stays a claim. The command itself may print a secret. With +`nodkey run --mask`, nodkey replaces each fetched value in the command's +stdout and stderr with `***`, also when the value comes in more than one +read (DESIGN.md section 5). This stops a mistake, not a hostile agent or +command: a value in another form, such as in base64, reversed, split +between stdout and stderr, or written to a file, is not masked. Without +`--mask`, the output is not filtered. The deny list of variable names covers common programs only: a setting of another program can still show its value, such as `UNZIP` for unzip. The agent also sets the rest of the command's environment, which the message does not show, so a diff --git a/docs/setup.md b/docs/setup.md index 7694635..f62cf4e 100644 --- a/docs/setup.md +++ b/docs/setup.md @@ -1683,6 +1683,12 @@ Tell the agent about it, for example in its instructions for a project: names are refused, because programs print or run their values ([DESIGN.md section 5](../DESIGN.md#variable-names-that-the-broker-refuses)). +The command itself may print a value, for example in a debug line. Add +`--mask`, and nodkey replaces each value in the command's stdout and +stderr with `***`. It masks a value only as it is, not in another form +such as base64. With `--mask`, the command's stdout and stderr are pipes, +not a terminal. + `nodkey run` exits with the exit code of the command. When nodkey itself stops, its last line on stderr is `nodkey: (exit )`: 77 when the request was denied or From aba0d22a40414be40f86c5c9aaa036454f28db82 Mon Sep 17 00:00:00 2001 From: Geno Roupsky <185580+groupsky@users.noreply.github.com> Date: Wed, 11 Nov 2026 03:45:50 +0200 Subject: [PATCH 5/6] fix(run): --mask lets signals end nodkey after the command, survives a closed output, wipes its copies (refs #18) --- DESIGN.md | 36 ++++++---- internal/client/mask.go | 13 +++- internal/client/run.go | 11 +-- internal/client/runmask.go | 117 ++++++++++++++++++++++++-------- internal/client/runmask_test.go | 36 ++++++++++ 5 files changed, 162 insertions(+), 51 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index 60eab54..92c1dbd 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -270,18 +270,25 @@ nodkey run --secret GH_TOKEN=github-token \ one read. It holds back only the end of the output that may still be the start of a value, and writes it when the next read shows that it is not a value, or when the command ends. Where two values match at one - place, the longest wins. It masks a value only exactly as it is: not in - base64, not split between stdout and stderr, and not in another - encoding. It waits until the command has ended and the pipes are closed - (a process that the command left behind may keep them open), then exits - with the command's exit code. A command that exits with 0 when its - output cannot be written gives 1. When the command dies of a signal, - the client ends itself with the same signal. While the command runs, - the client stays not dumpable and holds the values. It passes `SIGTERM` - and `SIGHUP` on to the command, and catches `SIGINT` and `SIGQUIT` - without passing them on, because a terminal sends them to the command - too. The command sees pipes, not a terminal, so some programs print - without colors or buffer their output. + place, the longest wins. It masks only whole values, exactly as they + are: not in base64, not split between stdout and stderr, and not in + another encoding. A start of a value at the very end of the output, and + the rest of a value that overlaps a value before it, are shown as they + are. It waits until the command has ended and the pipes are closed (a + process that the command left behind may keep them open), then exits + with the command's exit code. When stdout or stderr of the client is + closed, the client closes the pipe from the command, so the command + gets `SIGPIPE` on its next write; a command that still exits with 0 + gives 1. When the command dies of `SIGINT`, `SIGTERM`, `SIGHUP` or + `SIGKILL`, the client ends itself with the same signal. For another + signal, it exits with 128 and the number of the signal, such as 139 + for `SIGSEGV`. While the command runs, the client stays not dumpable + and holds the values. It passes `SIGTERM` and `SIGHUP` on to the + command, and catches `SIGINT` and `SIGQUIT` without passing them on, + because a terminal sends them to the command too. Once the command has + ended, these signals end the client again. The command sees pipes, not + a terminal, so some programs print without colors or buffer their + output. 8. On deny, timeout or error, the client prints one line to stderr, without any value, and exits: with 77 when the broker denies the request, and with 69 when it cannot reach the broker or gets another answer @@ -3313,8 +3320,9 @@ phone. Once the command starts, the signals are the command's. `--mask` (section 5, step 7): the command runs as a child of nodkey, and each fetched value in its stdout and stderr becomes `***`. `--mask` takes no value and may be given once. With it, 126 also covers a command that -cannot be started as a child, and a command that dies of a signal ends -nodkey with the same signal. +cannot be started as a child. A command that dies of `SIGINT`, `SIGTERM`, +`SIGHUP` or `SIGKILL` ends nodkey with the same signal; another signal +gives 128 and its number. `nodkey run -h` or `nodkey run --help` prints the usage and exits with 0, but only as the only argument. With other arguments, `-h` and `--help` diff --git a/internal/client/mask.go b/internal/client/mask.go index 6dc69da..14d1a51 100644 --- a/internal/client/mask.go +++ b/internal/client/mask.go @@ -14,8 +14,10 @@ const maskText = "***" // by maskText. A value may be split across writes: maskWriter holds back // the end of the data while it may still be the start of a value, and // writes it with the next write, or with Close. At one place, the longest -// value that matches wins. maskWriter never writes a part of a value. -// It is not safe for use by more than one goroutine. +// value that matches wins. Only whole values are masked: a start of a +// value at the end of the output, or the rest of a value that overlaps a +// value before it (values "ab" and "bcd", output "abcd" gives "***cd"), +// is written as it is. It is not safe for use by more than one goroutine. type maskWriter struct { w io.Writer values [][]byte // longest first; none is empty @@ -42,6 +44,13 @@ func newMaskWriter(w io.Writer, values [][]byte) *maskWriter { // Write masks p and writes all of it that cannot be the start of a value. // It reports len(p) when the writes to w succeed. func (m *maskWriter) Write(p []byte) (int, error) { + if len(m.buf)+len(p) > cap(m.buf) { + // Wipe the old array, as it may hold a start of a value. + grown := make([]byte, len(m.buf), 2*cap(m.buf)+len(p)) + copy(grown, m.buf) + clear(m.buf) + m.buf = grown + } m.buf = append(m.buf, p...) if err := m.flush(false); err != nil { return 0, err diff --git a/internal/client/run.go b/internal/client/run.go index 72f2a60..cc54491 100644 --- a/internal/client/run.go +++ b/internal/client/run.go @@ -229,6 +229,11 @@ func (r runner) run(ctx context.Context, args []string, w io.Writer, beforeExec values = append(values, bytes.Clone(s.Value)) } } + defer func() { + for _, v := range values { + clear(v) + } + }() ans.Clear() // best effort: envv holds the values as Go strings if err != nil { // The person approved, and the broker sent a wrong answer: as for @@ -240,11 +245,7 @@ func (r runner) run(ctx context.Context, args []string, w io.Writer, beforeExec return cancelled(ctx, stderr) } if o.mask { - code := r.masked(path, o.command, envv, values, stderr) - for _, v := range values { - clear(v) - } - return code + return r.masked(path, o.command, envv, values, stderr) } if err := r.exec(path, o.command, envv); err != nil { return fail(stderr, exitCannotRun, fmt.Sprintf("cannot run %q: %v", path, err)) diff --git a/internal/client/runmask.go b/internal/client/runmask.go index 49cb277..04fc7a2 100644 --- a/internal/client/runmask.go +++ b/internal/client/runmask.go @@ -7,20 +7,24 @@ import ( "os" "os/exec" "os/signal" + "sync" "syscall" ) // runMasked runs the command for nodkey run --mask, and returns the exit // code of nodkey run. The command is a child of nodkey, not a replacement: // its stdout and stderr go through a maskWriter each, to nodkey's stdout -// and stderr (DESIGN.md section 5). When the command dies of a signal, -// nodkey ends itself with the same signal, so that a shell sees it. +// and stderr (DESIGN.md section 5). When the command dies of SIGINT, +// SIGTERM, SIGHUP or SIGKILL, nodkey ends itself with the same signal, so +// that a shell sees it. For another signal, it exits with 128 and its +// number: Go would print a trace for SIGSEGV or SIGQUIT, not die of it. func runMasked(path string, argv, envv []string, values [][]byte, stderr io.Writer) int { code, sig, err := maskedCommand(path, argv, envv, values, os.Stdout, stderr) //nolint:forbidigo // the command's own stdout. if err != nil { return fail(stderr, exitCannotRun, err.Error()) } - if sig != 0 { + switch sig { + case syscall.SIGINT, syscall.SIGTERM, syscall.SIGHUP, syscall.SIGKILL: raise(sig) } return code @@ -35,33 +39,58 @@ func runMasked(path string, argv, envv []string, values [][]byte, stderr io.Writ // While the command runs, nodkey passes on SIGTERM and SIGHUP to it. It // catches SIGINT and SIGQUIT and does nothing with them: a terminal sends // them to the command too, as the command is in the same process group. -// A signal that nodkey ignores stays ignored, also in the command. +// A signal that nodkey ignores stays ignored, also in the command. Once +// the command has ended, these signals end nodkey again, also while a +// process that the command left behind keeps the pipes open. +// +// nodkey catches SIGPIPE too, so a write to a closed stdout or stderr +// fails and does not end nodkey. Then nodkey closes the pipe from the +// command, so the command gets SIGPIPE or EPIPE on its next write. func maskedCommand(path string, argv, envv []string, values [][]byte, stdout, stderr io.Writer) (int, syscall.Signal, error) { - outMask := newMaskWriter(stdout, values) - errMask := newMaskWriter(stderr, values) - defer outMask.clear() - defer errMask.clear() - cmd := &exec.Cmd{ - Path: path, - Args: argv, - Env: envv, - Stdin: os.Stdin, // the command reads nodkey's stdin, as after execve - Stdout: outMask, - Stderr: errMask, - } sigs := make(chan os.Signal, 4) var caught []os.Signal - for _, s := range []os.Signal{syscall.SIGTERM, syscall.SIGHUP, os.Interrupt, syscall.SIGQUIT} { + for _, s := range []os.Signal{syscall.SIGTERM, syscall.SIGHUP, os.Interrupt, syscall.SIGQUIT, syscall.SIGPIPE} { if !signal.Ignored(s) { caught = append(caught, s) } } signal.Notify(sigs, caught...) - defer signal.Stop(sigs) - if err := cmd.Start(); err != nil { + stopSignals := sync.OnceFunc(func() { signal.Stop(sigs) }) + defer stopSignals() + + outR, outW, err := os.Pipe() + if err != nil { + return 0, 0, fmt.Errorf("cannot run %q: cannot make a pipe: %w", path, err) + } + errR, errW, err := os.Pipe() + if err != nil { + _ = outR.Close() + _ = outW.Close() + return 0, 0, fmt.Errorf("cannot run %q: cannot make a pipe: %w", path, err) + } + cmd := &exec.Cmd{ + Path: path, + Args: argv, + Env: envv, + Stdin: os.Stdin, // the command reads nodkey's stdin, as after execve + Stdout: outW, + Stderr: errW, + } + err = cmd.Start() + // The command has its own copies of the write ends now. + _ = outW.Close() + _ = errW.Close() + if err != nil { + _ = outR.Close() + _ = errR.Close() return 0, 0, fmt.Errorf("cannot run %q: %w", path, err) } - done := make(chan struct{}) + var wg sync.WaitGroup + var outErr, errErr error + wg.Go(func() { outErr = copyMasked(outR, stdout, values) }) + wg.Go(func() { errErr = copyMasked(errR, stderr, values) }) + + exited := make(chan struct{}) go func() { for { select { @@ -69,29 +98,57 @@ func maskedCommand(path string, argv, envv []string, values [][]byte, stdout, st if s == syscall.SIGTERM || s == syscall.SIGHUP { _ = cmd.Process.Signal(s) } - case <-done: + case <-exited: return } } }() - err := cmd.Wait() - close(done) - // Write what is held back. A write that fails cannot be reported to - // anyone: the command's own exit code stays. - _ = outMask.Close() - _ = errMask.Close() + waitErr := cmd.Wait() // the pipes are files: Wait does not wait for the output + close(exited) + stopSignals() + wg.Wait() st := cmd.ProcessState if st == nil { - return 0, 0, fmt.Errorf("cannot run %q: %w", path, err) + return 0, 0, fmt.Errorf("cannot run %q: %w", path, waitErr) } if ws, ok := st.Sys().(syscall.WaitStatus); ok && ws.Signaled() { return 128 + int(ws.Signal()), ws.Signal(), nil } - var ee *exec.ExitError - if err != nil && !errors.As(err, &ee) && st.ExitCode() == 0 { + if st.ExitCode() == 0 && (outErr != nil || errErr != nil) { // The command exited with 0, and its output could not be written // (such as a closed stdout): not a success. return 1, 0, nil } + var ee *exec.ExitError + if waitErr != nil && !errors.As(waitErr, &ee) { + return 0, 0, fmt.Errorf("cannot run %q: %w", path, waitErr) + } return st.ExitCode(), 0, nil } + +// copyMasked copies r to w through a maskWriter, until r ends, and closes +// r. When a write to w fails, it closes r at once, so the command gets +// SIGPIPE or EPIPE, and returns the error. It wipes its buffer and the +// held data. +func copyMasked(r *os.File, w io.Writer, values [][]byte) error { + defer func() { _ = r.Close() }() + m := newMaskWriter(w, values) + defer m.clear() + buf := make([]byte, 32*1024) + defer clear(buf) + for { + n, err := r.Read(buf) + if n > 0 { + if _, werr := m.Write(buf[:n]); werr != nil { + return werr + } + } + if errors.Is(err, io.EOF) { + return m.Close() + } + if err != nil { + _ = m.Close() + return err + } + } +} diff --git a/internal/client/runmask_test.go b/internal/client/runmask_test.go index 3d34a65..69bd37e 100644 --- a/internal/client/runmask_test.go +++ b/internal/client/runmask_test.go @@ -147,3 +147,39 @@ func TestMaskedCommandOutputFails(t *testing.T) { t.Errorf("exit %d, %v; want a code that is not 0", code, err) } } + +// TestMaskedCommandOtherSignal: for a signal that Go would not die of, +// such as SIGSEGV, nodkey run exits with 128 and its number, and does not +// raise it (that would end this test process). +func TestMaskedCommandOtherSignal(t *testing.T) { + t.Parallel() + var stderr bytes.Buffer + code := runMasked("/bin/sh", []string{"sh", "-c", "kill -SEGV $$"}, nil, [][]byte{[]byte("x")}, &stderr) + if code != 128+int(syscall.SIGSEGV) { + t.Errorf("exit %d, want %d; stderr %q", code, 128+int(syscall.SIGSEGV), stderr.String()) + } +} + +// TestMaskedCommandLeftBehind: a process that the command left behind +// writes a value after the command ended. nodkey waits for it, and masks +// it. +func TestMaskedCommandLeftBehind(t *testing.T) { + t.Parallel() + canary := madeUp("canary") + code, _, out, _ := masked(t, []string{canary}, `(sleep 0.3; printf '<%s>' "$V0") & exit 5`) + if code != 5 || out != "<***>" { + t.Errorf("exit %d, stdout %q; want 5 and %q", code, safe(out), "<***>") + } +} + +// TestMaskedCommandClosedOutput: when the output cannot be written, the +// command gets SIGPIPE on its next write, so a command that writes +// without end does not run on. +func TestMaskedCommandClosedOutput(t *testing.T) { + t.Parallel() + code, sig, err := maskedCommand("/bin/sh", []string{"sh", "-c", "while :; do echo x; done"}, nil, + [][]byte{[]byte("y")}, failWriter{}, io.Discard) + if err != nil || sig != syscall.SIGPIPE || code != 128+int(syscall.SIGPIPE) { + t.Errorf("exit %d, signal %d, %v; want death by SIGPIPE", code, sig, err) + } +} From 469a5bab05a25b313ffcf1614a18aef133136f8f Mon Sep 17 00:00:00 2001 From: Geno Roupsky <185580+groupsky@users.noreply.github.com> Date: Wed, 11 Nov 2026 03:47:53 +0200 Subject: [PATCH 6/6] fix(run): --mask keeps SIGPIPE caught until the output is written, and keeps a late SIGTERM (refs #18) --- internal/client/runmask.go | 41 ++++++++++++++++++++++++++++++++------ 1 file changed, 35 insertions(+), 6 deletions(-) diff --git a/internal/client/runmask.go b/internal/client/runmask.go index 04fc7a2..2357229 100644 --- a/internal/client/runmask.go +++ b/internal/client/runmask.go @@ -49,7 +49,7 @@ func runMasked(path string, argv, envv []string, values [][]byte, stderr io.Writ func maskedCommand(path string, argv, envv []string, values [][]byte, stdout, stderr io.Writer) (int, syscall.Signal, error) { sigs := make(chan os.Signal, 4) var caught []os.Signal - for _, s := range []os.Signal{syscall.SIGTERM, syscall.SIGHUP, os.Interrupt, syscall.SIGQUIT, syscall.SIGPIPE} { + for _, s := range []os.Signal{syscall.SIGTERM, syscall.SIGHUP, os.Interrupt, syscall.SIGQUIT} { if !signal.Ignored(s) { caught = append(caught, s) } @@ -57,6 +57,13 @@ func maskedCommand(path string, argv, envv []string, values [][]byte, stdout, st signal.Notify(sigs, caught...) stopSignals := sync.OnceFunc(func() { signal.Stop(sigs) }) defer stopSignals() + // SIGPIPE stays caught until all of the output is written, also after + // the command ended. + pipes := make(chan os.Signal, 1) + if !signal.Ignored(syscall.SIGPIPE) { + signal.Notify(pipes, syscall.SIGPIPE) + defer signal.Stop(pipes) + } outR, outW, err := os.Pipe() if err != nil { @@ -90,23 +97,45 @@ func maskedCommand(path string, argv, envv []string, values [][]byte, stdout, st wg.Go(func() { outErr = copyMasked(outR, stdout, values) }) wg.Go(func() { errErr = copyMasked(errR, stderr, values) }) - exited := make(chan struct{}) + // late is a SIGTERM or SIGHUP that came when the command had just + // ended, so it could not be passed on. It then ends nodkey. + var late syscall.Signal + exited, forwarded := make(chan struct{}), make(chan struct{}) + pass := func(s os.Signal) { + if s != syscall.SIGTERM && s != syscall.SIGHUP { + return + } + if err := cmd.Process.Signal(s); err != nil { + late = s.(syscall.Signal) //nolint:forcetypeassert // SIGTERM and SIGHUP are syscall.Signal. + } + } go func() { + defer close(forwarded) for { select { case s := <-sigs: - if s == syscall.SIGTERM || s == syscall.SIGHUP { - _ = cmd.Process.Signal(s) - } + pass(s) case <-exited: return } } }() waitErr := cmd.Wait() // the pipes are files: Wait does not wait for the output - close(exited) stopSignals() + close(exited) + <-forwarded + for drained := false; !drained; { + select { + case s := <-sigs: + pass(s) + default: + drained = true + } + } wg.Wait() + if late != 0 { + return 128 + int(late), late, nil + } st := cmd.ProcessState if st == nil { return 0, 0, fmt.Errorf("cannot run %q: %w", path, waitErr)