feat(run): --mask hides fetched values in the command's output

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,33 @@ 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 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
@@ -3073,7 +3102,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 +3317,13 @@ 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. 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`
 are wrong usage (64), with the one line above: an exit with 0 that runs nothing would look like a
@@ -3783,7 +3819,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:

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

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: <reason> (exit <code>)`: 77 when the request was denied or

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.
 }

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

internal/client/mask.go

@@ -0,0 +1,137 @@
+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. 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
+	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) {
+	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
+	}
+	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]
+}

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