feat(run): mask fetched values by default; --no-mask turns it off

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,29 @@ 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=<value>` 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.
+
+   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
    all of the output to its own stdout and stderr, and replaces each
@@ -3102,7 +3120,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]
@@ -3306,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
@@ -3315,11 +3334,16 @@ 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.
-
-`--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
+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
+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 +3843,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

THREAT-MODEL.md

@@ -387,12 +387,14 @@ 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
-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
+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
 secret after a strange request.
 Risk that remains: real.
 
@@ -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

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 {

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

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 "<value>" 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()))
+	}
+}

internal/client/helpers_test.go

@@ -475,8 +475,11 @@ 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.
+		// TestRunMaskRule and TestRunMaskDenied test the masked default.
+		terminal: func() bool { return true },
 	}
 }
 

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