From 7d4bcb31780095a205fa41042c262aba49ea65c7 Mon Sep 17 00:00:00 2001 From: Geno Roupsky <185580+groupsky@users.noreply.github.com> Date: Wed, 11 Nov 2026 13:36:20 +0200 Subject: [PATCH 1/6] chore: start work on #170 From be5e3cda754a92e8d4f1ff4699dc5f6b96ba5e21 Mon Sep 17 00:00:00 2001 From: Geno Roupsky <185580+groupsky@users.noreply.github.com> Date: Wed, 11 Nov 2026 13:39:25 +0200 Subject: [PATCH 2/6] feat(run): mask by default; --no-mask runs the command with execve On a terminal (stdout and stderr both), the default stays execve, so interactive commands keep the terminal. Refs #170. --- internal/client/child_linux_test.go | 93 ++++++++++++++++++++++++-- internal/client/helpers_test.go | 4 +- internal/client/request_test.go | 13 +++- internal/client/run.go | 59 ++++++++++++---- internal/client/runmask.go | 7 ++ internal/client/runmask_test.go | 48 +++++++++++++ internal/client/terminal_linux.go | 14 ++++ internal/client/terminal_linux_test.go | 59 ++++++++++++++++ internal/client/terminal_other.go | 7 ++ 9 files changed, 281 insertions(+), 23 deletions(-) create mode 100644 internal/client/terminal_linux.go create mode 100644 internal/client/terminal_linux_test.go create mode 100644 internal/client/terminal_other.go diff --git a/internal/client/child_linux_test.go b/internal/client/child_linux_test.go index e8777d2..13eba80 100644 --- a/internal/client/child_linux_test.go +++ b/internal/client/child_linux_test.go @@ -86,6 +86,7 @@ func childRun(args []string) int { exec: checkedExec, notDumpable: notDumpable, masked: runMasked, + terminal: onTerminal, } return r.main(args, os.Stderr) //nolint:forbidigo // the child is nodkey run, which writes to stderr. } @@ -114,6 +115,8 @@ func dumpable() int { // SHA-256 of its value in hex is SUM. It prints "env ok VAR", or "env // wrong VAR", never the value. // - "dumpable": print "dumpable N", the dumpable flag of the command. +// - "pid": print "pid N", the process ID of the command. +// - "print VAR": print "" of VAR to stdout and to stderr. // - "exit N": exit with N, when every step before passed. func helper(args []string) int { stdout := os.Stdout //nolint:forbidigo // the helper is the command, which writes to stdout. @@ -134,6 +137,13 @@ func helper(args []string) int { } _, _ = fmt.Fprintf(stdout, "env %s %s\n", result, name) args = args[3:] + case args[0] == "pid": + _, _ = fmt.Fprintf(stdout, "pid %d\n", os.Getpid()) + args = args[1:] + case args[0] == "print" && len(args) >= 2: + _, _ = fmt.Fprintf(stdout, "<%s>\n", os.Getenv(args[1])) + _, _ = fmt.Fprintf(os.Stderr, "<%s>\n", os.Getenv(args[1])) //nolint:forbidigo // the helper is the command. + args = args[2:] case args[0] == "dumpable": _, _ = fmt.Fprintf(stdout, "dumpable %d\n", dumpable()) args = args[1:] @@ -214,8 +224,8 @@ func runChild(t *testing.T, cmd *exec.Cmd) (stdout, stderr string, code int) { return out.String(), errOut.String(), code } -// TestRunInAChild: the person approves, the command replaces nodkey run -// with the value in its environment, in place of an old variable of the +// TestRunInAChild: the person approves, the command runs (masked, by +// default, as the output is a pipe) with the value in its environment, in place of an old variable of the // same name, and nodkey run exits with the command's exit code. nodkey // prints only the short code, and nothing after the command started. The // value never reaches nodkey's output. @@ -254,7 +264,7 @@ func TestRunKeepsSIGPIPE(t *testing.T) { cat := needProgram(t, "cat") tb := newTestBroker(t) tb.approver.Auto(audit.AnswerApproveOnce) - stdout, stderr, code := runChild(t, tb.child(t, nil, "--secret", "GH_TOKEN=github-token", "--", cat, + stdout, stderr, code := runChild(t, tb.child(t, nil, "--secret", "GH_TOKEN=github-token", "--no-mask", "--", cat, "/proc/self/status")) mustNotLeak(t, tb.values["github-token"], stdout, stderr) if code != 0 { @@ -317,7 +327,7 @@ func TestRunInheritedSIGPIPE(t *testing.T) { } tb := newTestBroker(t) tb.approver.Auto(audit.AnswerApproveOnce) - inner := tb.child(t, nil, "--secret", "GH_TOKEN=github-token", "--", cat, "/proc/self/status") + inner := tb.child(t, nil, "--secret", "GH_TOKEN=github-token", "--no-mask", "--", cat, "/proc/self/status") cmd := exec.CommandContext(t.Context(), sh, append([]string{"-c", `trap '' PIPE; exec "$0" "$@"`}, inner.Args...)...) //nolint:gosec // G204: the test runs its own binary again, through sh. cmd.Env = inner.Env stdout, stderr, code := runChild(t, cmd) @@ -430,7 +440,7 @@ func TestRunCannotStart(t *testing.T) { } tb := newTestBroker(t) tb.approver.Auto(audit.AnswerApproveOnce) - stdout, stderr, code := runChild(t, tb.child(t, nil, "--secret", "GH_TOKEN=github-token", "--", bad)) + stdout, stderr, code := runChild(t, tb.child(t, nil, "--secret", "GH_TOKEN=github-token", "--no-mask", "--", bad)) mustNotLeak(t, tb.values["github-token"], stdout, stderr) last := lastLine(stderr) if code != exitCannotRun || !strings.HasPrefix(last, "nodkey: cannot run ") || !strings.HasSuffix(last, "(exit 126)") { @@ -507,7 +517,8 @@ func TestRunNotDumpable(t *testing.T) { tb := newTestBroker(t) // The helper exits with 5: the race detector pauses for a second when // a program exits with 0. - cmd := tb.child(t, nil, "--secret", "GH_TOKEN=github-token", "--", self(t), helperArg, "dumpable", "exit", "5") + cmd := tb.child(t, nil, "--secret", "GH_TOKEN=github-token", "--no-mask", "--", self(t), helperArg, "dumpable", + "exit", "5") var stdout, stderr bytes.Buffer cmd.Stdout, cmd.Stderr = &stdout, &stderr if err := cmd.Start(); err != nil { @@ -582,3 +593,73 @@ func TestRunProgramLinkChanges(t *testing.T) { t.Errorf("the command printed %q; want B, from the new target of the link", got) } } + +// TestRunMasksByDefault: without --mask or --no-mask, and with stdout and +// stderr as pipes, the command runs as a child of nodkey run, and each +// value in its stdout and stderr becomes ***. nodkey run exits with the +// command's exit code. +func TestRunMasksByDefault(t *testing.T) { + t.Parallel() + tb := newTestBroker(t) + tb.approver.Auto(audit.AnswerApproveOnce) + cmd := tb.child(t, nil, "--secret", "GH_TOKEN=github-token", "--", self(t), helperArg, "print", "GH_TOKEN", + "pid", "exit", "7") + var stdout, stderr bytes.Buffer + cmd.Stdout, cmd.Stderr = &stdout, &stderr + if err := cmd.Start(); err != nil { + t.Fatal(err) + } + err := cmd.Wait() + var exit *exec.ExitError + if !errors.As(err, &exit) || exit.ExitCode() != 7 { + t.Fatalf("%v; stderr %q; want the exit code 7 of the command", err, safe(stderr.String())) + } + mustNotLeak(t, tb.values["github-token"], stdout.String(), stderr.String()) + want := fmt.Sprintf("<***>\npid %d\n", cmd.Process.Pid) + if out := stdout.String(); !strings.HasPrefix(out, "<***>\npid ") || out == want { + t.Errorf("stdout %q; want the value masked, and the command in a child process", safe(out)) + } + if !strings.HasSuffix(stderr.String(), "\n<***>\n") { + t.Errorf("stderr %q; want the value masked", safe(stderr.String())) + } +} + +// TestRunNoMaskExecs: with --no-mask, the command replaces nodkey run +// (execve): it has the same process ID, as before --mask existed. +func TestRunNoMaskExecs(t *testing.T) { + t.Parallel() + tb := newTestBroker(t) + tb.approver.Auto(audit.AnswerApproveOnce) + value := tb.values["github-token"] + cmd := tb.child(t, nil, "--secret", "GH_TOKEN=github-token", "--no-mask", "--", self(t), helperArg, + "env", "GH_TOKEN", sha256Hex(value), "pid", "exit", "7") + var stdout, stderr bytes.Buffer + cmd.Stdout, cmd.Stderr = &stdout, &stderr + if err := cmd.Start(); err != nil { + t.Fatal(err) + } + err := cmd.Wait() + var exit *exec.ExitError + if !errors.As(err, &exit) || exit.ExitCode() != 7 { + t.Fatalf("%v; stderr %q; want the exit code 7 of the command", err, safe(stderr.String())) + } + mustNotLeak(t, value, stdout.String(), stderr.String()) + if want := fmt.Sprintf("env ok GH_TOKEN\npid %d\n", cmd.Process.Pid); stdout.String() != want { + t.Errorf("stdout %q, want %q: the command must replace nodkey run", safe(stdout.String()), want) + } +} + +// TestRunMaskedSignal: by default (masked), when the command dies of +// SIGTERM, nodkey run dies of SIGTERM too, so a shell sees it. +func TestRunMaskedSignal(t *testing.T) { + t.Parallel() + sh := needProgram(t, "sh") + tb := newTestBroker(t) + tb.approver.Auto(audit.AnswerApproveOnce) + cmd := tb.child(t, nil, "--secret", "GH_TOKEN=github-token", "--", sh, "-c", "kill -TERM $$") + var stdout, stderr bytes.Buffer + cmd.Stdout, cmd.Stderr = &stdout, &stderr + if err := cmd.Run(); !diedOf(err, syscall.SIGTERM) { + t.Errorf("%v; stderr %q; want nodkey run to die of SIGTERM", err, safe(stderr.String())) + } +} diff --git a/internal/client/helpers_test.go b/internal/client/helpers_test.go index 687700c..8fca7d8 100644 --- a/internal/client/helpers_test.go +++ b/internal/client/helpers_test.go @@ -475,8 +475,10 @@ func testRunner(b *Broker, env []string, fx *fakeExec) runner { // 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") + panic("this test must not run a masked command") }, + // As on a terminal: without --mask, the command replaces nodkey. + terminal: func() bool { return true }, } } diff --git a/internal/client/request_test.go b/internal/client/request_test.go index e30a354..9647667 100644 --- a/internal/client/request_test.go +++ b/internal/client/request_test.go @@ -47,6 +47,12 @@ func TestRunUsage(t *testing.T) { {"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"}, + {"a value for --no-mask", []string{"--secret", "A=b", "--no-mask=1", "--", "gh"}, "--no-mask takes no value"}, + {"two no-masks", []string{"--no-mask", "--secret", "A=b", "-no-mask", "--", "gh"}, "--no-mask is given twice"}, + {"mask and no-mask", []string{"--mask", "--no-mask", "--secret", "A=b", "--", "gh"}, + "give --mask or --no-mask, not both"}, + {"no-mask and mask", []string{"--no-mask", "--secret", "A=b", "--mask", "--", "gh"}, + "give --mask or --no-mask, not both"}, {"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`}, @@ -140,8 +146,11 @@ 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.mask || o.noMask { + t.Error("parseArgs without --mask or --no-mask: one is on") + } + if o, err := parseArgs([]string{"--secret", "A=a", "-no-mask", "--", "gh"}); err != nil || !o.noMask || o.mask { + t.Errorf("--no-mask: %+v, %v", o, err) } if o, err := parseArgs([]string{"--secret", "A=a", "-mask", "--", "gh", "--mask"}); err != nil || !o.mask || !slices.Equal(o.command, []string{"gh", "--mask"}) { diff --git a/internal/client/run.go b/internal/client/run.go index cc54491..4131ce5 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] [--mask] -- COMMAND [ARGS...] +const runUsage = `Usage: nodkey run [--secret VAR=NAME]... [--grant-token FILE] [--reason TEXT] [--mask | --no-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,10 +56,17 @@ 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. + --mask mask the output also when stdout and stderr are both + a terminal (see below). + --no-mask do not mask: COMMAND replaces nodkey, and its output + is not checked. + +By default, nodkey replaces 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. When stdout and stderr of nodkey are both a terminal, +COMMAND replaces nodkey and owns the terminal, and its output is not +masked: a person reads it there, not an agent. 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 @@ -77,8 +84,11 @@ const reasonVar = "NODKEY_REASON" // Run runs nodkey run with args, the arguments after "run", and writes its // own messages to stderr. version is the version of nodkey: it goes to the // broker, and Run prints a warning when the broker runs another version. -// When the person approves, the command replaces nodkey (execve), with the -// values in its environment, and Run does not return. Otherwise Run +// When the person approves, the command runs with the values in its +// environment: by default as a child, with its output masked (runMasked), +// and Run returns its exit code. With --no-mask, or when stdout and stderr +// are both terminals and --mask is not given, the command replaces nodkey +// (execve), and Run does not return. Otherwise Run // returns the exit code of DESIGN.md section 15, and the last line on // stderr is "nodkey: (exit )". // @@ -92,7 +102,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, masked: runMasked, version: version} + notDumpable: notDumpable, masked: runMasked, terminal: onTerminal, version: version} return r.main(args, stderr) } @@ -101,7 +111,8 @@ 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, masked: runMasked, version: version} + exec: syscall.Exec, notDumpable: notDumpable, masked: runMasked, terminal: onTerminal, + version: version} return r.main(args, stderr) } @@ -124,6 +135,9 @@ type runner struct { // 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 + // terminal reports whether stdout and stderr are both terminals: then + // the default is exec, not masked. onTerminal. + terminal func() bool // version is the version of nodkey (warnVersions); "" sends none. version string } @@ -223,8 +237,10 @@ 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) + // The output is masked by default; on a terminal, the command owns it. + mask := o.mask || (!o.noMask && !r.terminal()) var values [][]byte - if err == nil && o.mask { + if err == nil && mask { for _, s := range ans.Secrets { values = append(values, bytes.Clone(s.Value)) } @@ -244,7 +260,7 @@ func (r runner) run(ctx context.Context, args []string, w io.Writer, beforeExec if ctx.Err() != nil { return cancelled(ctx, stderr) } - if o.mask { + if mask { return r.masked(path, o.command, envv, values, stderr) } if err := r.exec(path, o.command, envv); err != nil { @@ -364,13 +380,15 @@ type runOptions struct { reasonSet bool // grantToken is the file of --grant-token ("" for none). grantToken string - // mask is --mask: the command's output is masked. - mask bool + // mask is --mask: the command's output is masked, also on a terminal. + mask bool + // noMask is --no-mask: the command replaces nodkey (execve). + noMask bool command []string } // parseArgs reads the arguments of nodkey run: the options, "--", and the -// command. Options are --secret VAR=NAME, which may repeat, --mask, and --reason +// command. Options are --secret VAR=NAME, which may repeat, --mask, --no-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. @@ -404,9 +422,22 @@ func parseArgs(args []string) (*runOptions, error) { return nil, errors.New("--mask takes no value") case o.mask: return nil, errors.New("--mask is given twice") + case o.noMask: + return nil, errors.New("give --mask or --no-mask, not both") } o.mask = true continue + case "no-mask": + switch { + case hasValue: + return nil, errors.New("--no-mask takes no value") + case o.noMask: + return nil, errors.New("--no-mask is given twice") + case o.mask: + return nil, errors.New("give --mask or --no-mask, not both") + } + o.noMask = 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 index 2357229..069f3f9 100644 --- a/internal/client/runmask.go +++ b/internal/client/runmask.go @@ -181,3 +181,10 @@ func copyMasked(r *os.File, w io.Writer, values [][]byte) error { } } } + +// onTerminal reports whether both stdout and stderr of nodkey are +// terminals. Then nodkey run without --mask or --no-mask replaces itself +// with the command, so the command owns the terminal (DESIGN.md section 5). +func onTerminal() bool { + return isTerminal(1) && isTerminal(2) +} diff --git a/internal/client/runmask_test.go b/internal/client/runmask_test.go index 69bd37e..7dabf6d 100644 --- a/internal/client/runmask_test.go +++ b/internal/client/runmask_test.go @@ -183,3 +183,51 @@ func TestMaskedCommandClosedOutput(t *testing.T) { t.Errorf("exit %d, signal %d, %v; want death by SIGPIPE", code, sig, err) } } + +// TestRunMaskRule: which way the command runs, for each option and for +// stdout and stderr on a terminal or not. Without an option, the output +// is masked unless both are terminals; --mask always masks; --no-mask +// never does. +func TestRunMaskRule(t *testing.T) { + t.Parallel() + tests := []struct { + name string + option string + terminal bool + masked bool + }{ + {"default, pipe", "", false, true}, + {"default, terminal", "", true, false}, + {"--mask, pipe", "--mask", false, true}, + {"--mask, terminal", "--mask", true, true}, + {"--no-mask, pipe", "--no-mask", false, false}, + {"--no-mask, terminal", "--no-mask", true, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(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) + r.terminal = func() bool { return tt.terminal } + ranMasked := false + r.masked = func(string, []string, []string, [][]byte, io.Writer) int { + ranMasked = true + return 7 + } + args := []string{"--secret", "GH_TOKEN=github-token"} + if tt.option != "" { + args = append(args, tt.option) + } + code, stderr := runIn(t.Context(), r, append(args, "--", self(t))...) + execed := len(fx.ran()) == 1 + switch { + case tt.masked && (!ranMasked || execed || code != 7): + t.Errorf("masked %v, exec %v, code %d; want masked (stderr %q)", ranMasked, execed, code, safe(stderr)) + case !tt.masked && (ranMasked || !execed || code != 0): + t.Errorf("masked %v, exec %v, code %d; want exec (stderr %q)", ranMasked, execed, code, safe(stderr)) + } + }) + } +} diff --git a/internal/client/terminal_linux.go b/internal/client/terminal_linux.go new file mode 100644 index 0000000..10e84e2 --- /dev/null +++ b/internal/client/terminal_linux.go @@ -0,0 +1,14 @@ +package client + +import ( + "syscall" + "unsafe" +) + +// isTerminal reports whether fd is a terminal: the TCGETS ioctl, as +// isatty does, works only on a terminal. +func isTerminal(fd uintptr) bool { + var t syscall.Termios + _, _, errno := syscall.Syscall(syscall.SYS_IOCTL, fd, syscall.TCGETS, uintptr(unsafe.Pointer(&t))) //nolint:gosec // G103: the ioctl needs the address. + return errno == 0 +} diff --git a/internal/client/terminal_linux_test.go b/internal/client/terminal_linux_test.go new file mode 100644 index 0000000..0564dae --- /dev/null +++ b/internal/client/terminal_linux_test.go @@ -0,0 +1,59 @@ +package client + +import ( + "os" + "strconv" + "syscall" + "testing" + "unsafe" +) + +// TestIsTerminal: a pseudo-terminal is a terminal; a pipe, a file and +// /dev/null are not. +func TestIsTerminal(t *testing.T) { + t.Parallel() + ptmx, err := os.OpenFile("/dev/ptmx", os.O_RDWR|syscall.O_NOCTTY, 0) + if err != nil { + if os.Getenv("CI") != "" { + t.Fatalf("no pseudo-terminal: %v", err) + } + t.Skipf("no pseudo-terminal: %v", err) + } + defer func() { _ = ptmx.Close() }() + var unlock int32 + if _, _, e := syscall.Syscall(syscall.SYS_IOCTL, ptmx.Fd(), syscall.TIOCSPTLCK, uintptr(unsafe.Pointer(&unlock))); e != 0 { //nolint:gosec // G103: the ioctl needs the address. + t.Fatalf("unlockpt: %v", e) + } + var n uint32 + if _, _, e := syscall.Syscall(syscall.SYS_IOCTL, ptmx.Fd(), syscall.TIOCGPTN, uintptr(unsafe.Pointer(&n))); e != 0 { //nolint:gosec // G103: the same. + t.Fatalf("ptsname: %v", e) + } + pts, err := os.OpenFile("/dev/pts/"+strconv.FormatUint(uint64(n), 10), os.O_RDWR|syscall.O_NOCTTY, 0) + if err != nil { + t.Fatal(err) + } + defer func() { _ = pts.Close() }() + if !isTerminal(pts.Fd()) { + t.Error("a pseudo-terminal is not a terminal") + } + r, w, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + defer func() { _ = r.Close(); _ = w.Close() }() + f, err := os.Create(t.TempDir() + "/out") + if err != nil { + t.Fatal(err) + } + defer func() { _ = f.Close() }() + null, err := os.Open(os.DevNull) + if err != nil { + t.Fatal(err) + } + defer func() { _ = null.Close() }() + for name, fd := range map[string]uintptr{"a pipe": w.Fd(), "a file": f.Fd(), os.DevNull: null.Fd()} { + if isTerminal(fd) { + t.Errorf("%s is a terminal", name) + } + } +} diff --git a/internal/client/terminal_other.go b/internal/client/terminal_other.go new file mode 100644 index 0000000..9e6f9e9 --- /dev/null +++ b/internal/client/terminal_other.go @@ -0,0 +1,7 @@ +//go:build !linux + +package client + +// isTerminal reports false here: version 1 runs on Linux only (DESIGN.md +// section 3), so nodkey run masks the output on other systems. +func isTerminal(uintptr) bool { return false } From 2de7140a04004503c1a7bccb082cad1b91152e58 Mon Sep 17 00:00:00 2001 From: Geno Roupsky <185580+groupsky@users.noreply.github.com> Date: Wed, 11 Nov 2026 13:40:03 +0200 Subject: [PATCH 3/6] docs: mask is the default of nodkey run; --no-mask; the rule for terminals Refs #170. --- DESIGN.md | 47 ++++++++++++++++++++++++++++++++--------------- THREAT-MODEL.md | 20 +++++++++++++------- docs/setup.md | 15 ++++++++++----- 3 files changed, 55 insertions(+), 27 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index 92c1dbd..6dcddd2 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -68,8 +68,10 @@ what it protects against, and what it does not. holds plain host keys, and the broker signs only with plain keys. - Keys in hardware (TPM or YubiKey) through an upstream agent. - 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). + base64 or split between stdout and stderr (`nodkey run` masks it only + as it is, section 5). +- A pseudo-terminal for a masked command. On a terminal, `nodkey run` + does not mask by default (section 5). - More than one approver, and Telegram and ntfy on one broker at once. Each of these can become a GitHub issue later. @@ -256,13 +258,25 @@ nodkey run --secret GH_TOKEN=github-token \ in the order of the request and with the same names, each 1 byte to 64 KiB and without a NUL byte. Otherwise the command does not run, and the client exits with 69. Then the client adds `GH_TOKEN=` to - its own environment, in place of a variable of the same name, and - replaces itself with the command (`execve`): the path of step 1, with - the arguments as the agent gave them. So no nodkey process stays in - 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 + its own environment, in place of a variable of the same name, and runs + the command: the path of step 1, with the arguments as the agent gave + them. The command's exit code is the exit code of `nodkey run`. How it + runs the command depends on the options and on the output: + - By default, the client masks the output (below), unless its stdout + and stderr are both a terminal. An agent reads the output of a + command through a pipe or a file, so its output is masked. On a + terminal, a person reads the output, and an editor, `ssh`, a prompt + or colors need the command to own the terminal; so there the client + replaces itself, as with `--no-mask`. The client checks each with + the `TCGETS` ioctl, as `isatty` does; `/dev/null` is not a terminal. + - With `--no-mask`, the client replaces itself with the command + (`execve`), as before masking was the default. So no nodkey process + stays in memory with the values. The client never ignores + `SIGPIPE`: an ignored signal stays ignored after `execve`, so the + command would run with it ignored. + - With `--mask`, the client masks the output, also on a terminal. + + To 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 @@ -3102,7 +3116,7 @@ other users must not be able to change it. ``` nodkey broker [--config FILE] -nodkey run [--secret VAR=NAME]... [--grant-token FILE] [--reason TEXT] [--mask] -- COMMAND [ARGS...] +nodkey run [--secret VAR=NAME]... [--grant-token FILE] [--reason TEXT] [--mask | --no-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] @@ -3317,9 +3331,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 +Masking (section 5, step 7): by default, unless stdout and stderr are +both a terminal, the command runs as a child of nodkey, and each fetched +value in its stdout and stderr becomes `***`. `--mask` masks also on a +terminal. `--no-mask` never masks: the command replaces nodkey +(`execve`). Each takes no value and may be given once; both together are +wrong usage (64). When masked, 126 also covers a command that 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. @@ -3819,8 +3836,8 @@ 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. With - `--mask`, nodkey stays not dumpable while the command runs, as it holds + does not change `RLIMIT_CORE`, which the command would inherit. When it + masks the output, 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 diff --git a/THREAT-MODEL.md b/THREAT-MODEL.md index 7f6ff13..f3e0b75 100644 --- a/THREAT-MODEL.md +++ b/THREAT-MODEL.md @@ -387,9 +387,11 @@ item 2. Risk that remains: real; outside nodkey. **T14. The agent keeps a secret after approval.** See section 5, item 1. -`nodkey run` itself keeps no value: it replaces itself with the command -(`execve`), so the value is only in the environment of the command. -Before it asks, it marks itself not dumpable, so it writes no core dump +With `--no-mask` (and by default on a terminal), `nodkey run` itself +keeps no value: it replaces itself with the command (`execve`), so the +value is only in the environment of the command. When it masks the +output (the default), it holds the values while the command runs, to +mask them, and stays not dumpable. Before it asks, it marks itself not dumpable, 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 then. The command itself is dumpable again. Use short-lived or narrow secrets, and rotate a @@ -433,13 +435,17 @@ 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. With -`nodkey run --mask`, nodkey replaces each fetched value in the command's +claimed text stays a claim. The command itself may print a secret. By +default, `nodkey run` 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 +between stdout and stderr, or written to a file, is not masked. The +output is not filtered with `--no-mask`, nor by default when stdout and +stderr of `nodkey run` are both a terminal: then a person reads it, and +the command owns the terminal. An agent that runs its commands in a +pseudo-terminal of its own gets the values unmasked there; `--mask` +masks also on a terminal. 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 f62cf4e..1d8c89a 100644 --- a/docs/setup.md +++ b/docs/setup.md @@ -1683,11 +1683,16 @@ 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. +The command itself may print a value, for example in a debug line. So, +by default, 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. The command's stdout and stderr are then pipes, not a terminal. + +When stdout and stderr of `nodkey run` are both a terminal, a person +reads the output, so nodkey does not mask it: the command owns the +terminal, and editors, `ssh` and prompts work. `--mask` masks also on a +terminal. `--no-mask` turns masking off, for a value that is also a +common word, for example: then normal output would show `***`. `nodkey run` exits with the exit code of the command. When nodkey itself stops, its last line on stderr is From def464b9bd3e40e09f19bfec21a0477e5e177b5c Mon Sep 17 00:00:00 2001 From: Geno Roupsky <185580+groupsky@users.noreply.github.com> Date: Wed, 11 Nov 2026 13:45:03 +0200 Subject: [PATCH 4/6] fix: security review of #170: deny test of the masked default, docs and comments Refs #170. --- DESIGN.md | 11 +++++++++-- cmd/nodkey/run.go | 3 ++- internal/client/helpers_test.go | 1 + internal/client/mask.go | 2 +- internal/client/run.go | 4 ++-- internal/client/runmask.go | 3 ++- internal/client/runmask_test.go | 16 ++++++++++++++++ 7 files changed, 33 insertions(+), 7 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index 6dcddd2..dd53d06 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -276,6 +276,10 @@ nodkey run --secret GH_TOKEN=github-token \ command would run with it ignored. - With `--mask`, the client masks the output, also on a terminal. + Masking changes the bytes of normal output where a value appears. A + short value, such as `1` or `true`, can break JSON or other output + that a program reads; use `--no-mask` for such a value. + To 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 @@ -3320,7 +3324,8 @@ tries again. More cases: is not a program. `nodkey run` looks for the command before it asks anyone. So 126 and -127 come without a request, unless `execve` itself fails. +127 come without a request, unless `execve` itself fails, or the command +cannot be started as a child (masked). `SIGINT` (Ctrl-C) or `SIGTERM` while nodkey waits: it closes the connection, so the broker cancels the request, and prints the last line @@ -3329,7 +3334,9 @@ ends itself with the same signal, as a program that does not catch it. A shell then shows 130 or 143. A shell stops a loop or a script only when 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. +phone. Once the command starts, the signals are the command's. When the +output is masked, nodkey stays and passes some of them on (section 5, +step 7). Masking (section 5, step 7): by default, unless stdout and stderr are both a terminal, the command runs as a child of nodkey, and each fetched diff --git a/cmd/nodkey/run.go b/cmd/nodkey/run.go index 5d60f0a..42c659f 100644 --- a/cmd/nodkey/run.go +++ b/cmd/nodkey/run.go @@ -7,7 +7,8 @@ import ( ) // runRun runs nodkey run (package client). When the person approves, the -// command replaces nodkey, and runRun does not return. It must never +// command runs as a child with its output masked, or, with --no-mask or +// on a terminal, replaces nodkey, and runRun does not return. It must never // ignore SIGPIPE, as keygen does: an ignored signal stays ignored after // execve, so the command would run with it ignored. func runRun(args []string, stderr io.Writer) int { diff --git a/internal/client/helpers_test.go b/internal/client/helpers_test.go index 8fca7d8..d308d8c 100644 --- a/internal/client/helpers_test.go +++ b/internal/client/helpers_test.go @@ -478,6 +478,7 @@ func testRunner(b *Broker, env []string, fx *fakeExec) runner { panic("this test must not run a masked command") }, // As on a terminal: without --mask, the command replaces nodkey. + // TestRunMaskRule and TestRunMaskDenied test the masked default. terminal: func() bool { return true }, } } diff --git a/internal/client/mask.go b/internal/client/mask.go index 14d1a51..e2a16f8 100644 --- a/internal/client/mask.go +++ b/internal/client/mask.go @@ -7,7 +7,7 @@ import ( ) // maskText replaces each fetched value in the output of the command -// (nodkey run --mask, DESIGN.md section 5). +// (nodkey run, DESIGN.md section 5). const maskText = "***" // maskWriter writes to w what is written to it, with each value replaced diff --git a/internal/client/run.go b/internal/client/run.go index 4131ce5..f12d8da 100644 --- a/internal/client/run.go +++ b/internal/client/run.go @@ -132,8 +132,8 @@ 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 runs the command with its output masked, with the values to + // mask, and returns the exit code: runMasked. masked func(path string, argv, envv []string, values [][]byte, stderr io.Writer) int // terminal reports whether stdout and stderr are both terminals: then // the default is exec, not masked. onTerminal. diff --git a/internal/client/runmask.go b/internal/client/runmask.go index 069f3f9..929d6ba 100644 --- a/internal/client/runmask.go +++ b/internal/client/runmask.go @@ -11,7 +11,8 @@ import ( "syscall" ) -// runMasked runs the command for nodkey run --mask, and returns the exit +// runMasked runs the command for nodkey run when it masks the output +// (the default), 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 SIGINT, diff --git a/internal/client/runmask_test.go b/internal/client/runmask_test.go index 7dabf6d..7f87e8c 100644 --- a/internal/client/runmask_test.go +++ b/internal/client/runmask_test.go @@ -231,3 +231,19 @@ func TestRunMaskRule(t *testing.T) { }) } } + +// TestRunMaskDenied: by default, with the output not on a terminal, a +// denied request runs nothing: neither masked nor exec. (The masked stub +// of testRunner panics.) +func TestRunMaskDenied(t *testing.T) { + t.Parallel() + tb := newTestBroker(t) + tb.approver.Auto(audit.AnswerDeny) + fx := &fakeExec{} + r := testRunner(tb.client(t), []string{"PATH=/usr/bin"}, fx) + r.terminal = func() bool { return false } + code, stderr := runIn(t.Context(), r, "--secret", "GH_TOKEN=github-token", "--", self(t)) + if code != exitDenied || len(fx.ran()) != 0 { + t.Errorf("exit code %d, exec %d times; want 77 and nothing run (stderr %q)", code, len(fx.ran()), safe(stderr)) + } +} From af2edc8dce0f935e666f05ee456de1a0a89cbd46 Mon Sep 17 00:00:00 2001 From: Geno Roupsky <185580+groupsky@users.noreply.github.com> Date: Wed, 11 Nov 2026 13:45:55 +0200 Subject: [PATCH 5/6] docs: wrap a long line in T14 --- THREAT-MODEL.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/THREAT-MODEL.md b/THREAT-MODEL.md index f3e0b75..e08e9f5 100644 --- a/THREAT-MODEL.md +++ b/THREAT-MODEL.md @@ -391,8 +391,8 @@ With `--no-mask` (and by default on a terminal), `nodkey run` itself keeps no value: it replaces itself with the command (`execve`), so the value is only in the environment of the command. When it masks the output (the default), it holds the values while the command runs, to -mask them, and stays not dumpable. Before it asks, it marks itself not dumpable, so it writes no core dump -while it holds the values, also when the agent sets `GOTRACEBACK`, and +mask them, and stays not dumpable. Before it asks, it marks itself not +dumpable, 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 then. The command itself is dumpable again. Use short-lived or narrow secrets, and rotate a secret after a strange request. From 046f56a240e7f7ced3ccd57f23ba46bb38eb434e Mon Sep 17 00:00:00 2001 From: Geno Roupsky <185580+groupsky@users.noreply.github.com> Date: Wed, 11 Nov 2026 13:46:43 +0200 Subject: [PATCH 6/6] docs: wrap T14 and the comment of runMasked --- THREAT-MODEL.md | 6 +++--- internal/client/runmask.go | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/THREAT-MODEL.md b/THREAT-MODEL.md index e08e9f5..a9a67c6 100644 --- a/THREAT-MODEL.md +++ b/THREAT-MODEL.md @@ -392,9 +392,9 @@ keeps no value: it replaces itself with the command (`execve`), so the value is only in the environment of the command. When it masks the output (the default), it holds the values while the command runs, to mask them, and stays not dumpable. Before it asks, it marks itself not -dumpable, 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 then. The command -itself is dumpable again. Use short-lived or narrow secrets, and rotate a +dumpable, 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 then. The command itself is dumpable again. Use short-lived or narrow secrets, and rotate a secret after a strange request. Risk that remains: real. diff --git a/internal/client/runmask.go b/internal/client/runmask.go index 929d6ba..ddd6fd8 100644 --- a/internal/client/runmask.go +++ b/internal/client/runmask.go @@ -12,8 +12,8 @@ import ( ) // runMasked runs the command for nodkey run when it masks the output -// (the default), and returns the exit -// code of nodkey run. The command is a child of nodkey, not a replacement: +// (the default), 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 SIGINT, // SIGTERM, SIGHUP or SIGKILL, nodkey ends itself with the same signal, so