feat(store): fetch secrets with bws and bw

THREAT-MODEL.md

@@ -19,7 +19,7 @@ access to another machine and asks a person before each use.
 | asset | where it lives |
 |---|---|
 | SSH private keys | broker memory; encrypted credentials on the broker host's disk |
-| secret-store access (`bws` token, `bw` master password) | broker host |
+| secret-store access (`bws` token, `bw` master password and session key) | broker host |
 | secret values | the store; for a moment the broker; after approval, the one command on the agent machine |
 | the approval decision | Telegram, the broker |
 | the audit log | broker host, and hash copies in the broker's journal |
@@ -324,9 +324,25 @@ Risk that remains:
 
 **T18. The store programs leak a token.**
 Tokens and the `bw` session go in environment variables, never on the
-command line, so other users cannot see them in the process list. The
-broker passes only the variables the program needs.
-Risk that remains: low.
+command line, so other users cannot see them in the process list. The `bw`
+master password is not in a variable either: bw reads the password file
+itself. The broker passes only `HOME`, the one variable with the token
+or the session key, and for bw a fixed `PATH`, and nothing from its own
+environment, so a variable such as `NODE_OPTIONS` or `LD_PRELOAD` cannot
+change what the program does. The broker never logs the programs'
+output, and its errors hold only a fixed text and the exit code. Tests
+check all of this with made-up secrets and fake programs that record what
+they get. If a store program crashes, a core handler such as
+systemd-coredump would write its environment, with the token or the
+session key, into the journal. So the broker sets its soft `RLIMIT_CORE`
+to 1, which the programs inherit, and the example unit sets
+`LimitCORE=1`: with exactly 1 and a pipe (`|`) or file `core_pattern`
+(the systemd default), the kernel calls no core handler. A limit of 0
+does not stop one, and a socket `core_pattern` (`@`) does not look at the
+limit (DESIGN.md section 16).
+Risk that remains: low. The broker clears the token and the session key
+in its memory, but only as best effort: Go strings and the copies that the
+system call makes are not cleared.
 
 **T19. The bot token leaks in an error message.**
 Go's HTTP errors include the URL, and the URL holds the token. So no
@@ -427,8 +443,18 @@ read. The client checks its own TLS key in the same way. `nodkey keygen`
 writes a new key file with mode `0600`, never over a file or a link that
 is there. It checks the directories and the new, empty file in the same
 way before it writes the key.
+Each store program keeps its state, such as bw's copy of the vault, in
+its own home directory, with mode `0700`, owned by the broker user, below
+directories that pass the same checks. The broker refuses a `bw-data`
+directory next to bw's executable, where bw would keep its state instead.
+bw reads its password file by its path; the broker checks the file right
+before each unlock. The directories in bw's `PATH` pass the same checks,
+and the `node` in them passes the checks of a store's program, also after
+its links, so nobody else can put or change the `node` that a bw script
+starts.
 Risk that remains: root and the broker user can still change everything
-(T16). Programs and libraries that a store program loads are not checked.
+(T16); for example, they can swap the bw password file between the check
+and bw's read. The libraries that a store program loads are not checked.
 On overlayfs, nodkey sees only the mode bits of the top copy of a file; a
 lower copy of a key can be readable by others who can reach the lower
 layer (DESIGN.md section 14). Keep keys and tokens out of lower layers and
@@ -491,6 +517,28 @@ service, not a way to get a secret or an approval. The answer is a
 firewall that lets only the agent machines reach the broker port
 (DESIGN.md section 10).
 
+**T29. A store program hangs, floods, or answers wrong or old data.**
+The broker gives each run of a store program 30 seconds and caps its
+output: 1 MiB on stdout and 64 KiB on stderr. Over a limit, or when the
+request ends, it kills the program's whole process group, and the request
+is denied. If the broker itself ends during a run, the kernel kills the
+program. The broker reads only one strict JSON object, and denies
+anything else: an answer without the asked ID or for another item, and a
+missing or empty value, or one with a NUL byte. bw keeps a copy of the
+vault, so the broker runs `bw sync --force` before each fetch, and denies
+the request if the sync fails. Without `--force`, bw would compare the
+server's time with the broker's clock, and with that clock ahead it would
+never download again. An item in the trash or in the archive is refused,
+because `bw get item` still returns such an item by its ID. If bw is
+logged out, every request for that store is denied until a person logs in
+again (DESIGN.md sections 11 and 16). With a pipe or file
+`core_pattern`, a crashed program writes no core dump and starts no core
+handler (T18).
+Risk that remains: a process that leaves the program's process group (for
+example with `setsid`) is not killed; systemd stops it when the service
+stops. Such a process keeps the token or the session key in its
+environment.
+
 ## 7. Assumptions
 
 - The broker host is patched, has no agent on it, and only the owner has

internal/config/files.go

@@ -322,6 +322,18 @@ func checkAuditLogFile(fi fs.FileInfo, self uint32) error {
 	return nil
 }
 
+// CheckProgram makes the checks of a store's program (see CheckFiles) on
+// the program at path. It follows symbolic links. Every directory on the
+// way, also each one that holds a link, must pass the checks of
+// ReadPrivateFile. The program must be a regular file owned by root or by
+// the user that runs nodkey, and group and others must not be able to
+// write to it. The store package checks with it the other programs that
+// get a secret, such as the node that a bw script starts.
+func CheckProgram(path string) error {
+	_, err := checkProgram(path)
+	return err
+}
+
 // checkProgram checks a program that the broker runs and gives a token to.
 // It follows symbolic links. Every directory on the way must pass the
 // checks of ReadPrivateFile. The program must be a regular file owned by

internal/config/files_linux_test.go

@@ -13,6 +13,43 @@ import (
 	"time"
 )
 
+// TestCheckProgram checks CheckProgram, the check of a store's program,
+// which the store package also uses for the node that a bw script starts.
+// CheckFiles tests the same check in more cases.
+func TestCheckProgram(t *testing.T) {
+	dir := privateTempDir(t)
+	bin := mkdir(t, dir, "bin", 0o755)
+	open := mkdir(t, dir, "open", 0o777)
+	good := writeFile(t, bin, "node", "", 0o755)
+	for _, path := range []string{good, symlink(t, good, filepath.Join(bin, "node-link"))} {
+		if err := CheckProgram(path); err != nil {
+			t.Errorf("CheckProgram(%s): %v", path, err)
+		}
+	}
+	writable := writeFile(t, bin, "node-0777", "", 0o777)
+	inOpen := writeFile(t, open, "node", "", 0o755)
+	hop := symlink(t, good, filepath.Join(open, "hop"))
+	tests := []struct{ name, path, want string }{
+		{"others can write it", writable, writable + ": group or others can write to it (mode 0777)"},
+		{"a link to a file that others can write", symlink(t, writable, filepath.Join(bin, "link-0777")),
+			writable + ": group or others can write to it (mode 0777)"},
+		{"a link into a directory that others can write", symlink(t, inOpen, filepath.Join(bin, "link-open")),
+			open + ": group or others can write to this directory (mode 0777)"},
+		{"a link held in a directory that others can write", symlink(t, hop, filepath.Join(bin, "link-hop")),
+			open + ": group or others can write to this directory (mode 0777)"},
+		{"a directory", mkdir(t, bin, "node-dir", 0o755), "is not a regular file"},
+		{"missing", filepath.Join(bin, "missing"), "no such file or directory"},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			checkError(t, CheckProgram(tc.path), tc.want, "")
+		})
+	}
+	if err := CheckProgram(filepath.Join(bin, "missing")); !errors.Is(err, os.ErrNotExist) {
+		t.Errorf("CheckProgram(missing) = %v, want an error that is fs.ErrNotExist", err)
+	}
+}
+
 // TestCheckPrivateFileWithACL sets a real POSIX ACL on a file, like the one
 // systemd puts on a credential, and checks that the group bits it shows in
 // the mode do not count as group access.

internal/store/answer.go

@@ -0,0 +1,246 @@
+package store
+
+import (
+	"bytes"
+	"fmt"
+)
+
+// The answers that nodkey reads from bws and bw. DESIGN.md section 16
+// lists the fields.
+
+// Limits of the keys that a store keeps in memory.
+const (
+	// maxTokenLen is the longest bws access token. A token has about 100
+	// characters.
+	maxTokenLen = 4096
+	// maxSessionLen is the longest bw session key. A session key is 64
+	// random bytes in base64: 88 characters.
+	maxSessionLen = 1024
+	// maxStatusLen is the longest status in the answer of bw status.
+	maxStatusLen = 64
+	// maxIDLen is the longest "id" in an answer that nodkey compares with
+	// the asked ID. A UUID has 36 characters.
+	maxIDLen = 64
+)
+
+// The states of bw that bw status reports.
+const (
+	bwUnauthenticated = "unauthenticated" // logged out
+	bwLocked          = "locked"          // logged in, but the session key does not work
+	bwUnlocked        = "unlocked"        // logged in, and the session key works
+)
+
+// errOtherItem is the error of an answer without an "id", or with the ID
+// of another item than the one asked for.
+var errOtherItem = fmt.Errorf("%w: it has no id, or the id of another item", ErrBadAnswer)
+
+// parseBWSSecret reads the answer of "bws secret get ID --output json":
+// one JSON object. It reads only the fields "id" and "value". "id" must be
+// the asked ID id, in upper or lower case. A value that is missing, null or
+// empty is ErrNoValue, and one with a NUL byte is ErrBadValue (see
+// keepValue). The other fields, such as "key" and "note", are skipped;
+// "note" can hold secrets too.
+func parseBWSSecret(data []byte, id string) (Value, error) {
+	var value []byte
+	sameID := false
+	err := readAnswer(data, func(r *jsonReader, key string) error {
+		switch key {
+		case "id":
+			return readID(r, id, &sameID)
+		case "value":
+			switch r.peek() {
+			case 'n':
+				return r.null()
+			case '"':
+				b, err := r.secret()
+				value = b
+				return err
+			}
+			return ErrBadAnswer
+		}
+		return r.skip()
+	})
+	if err == nil && !sameID {
+		err = errOtherItem
+	}
+	return keepValue(value, err)
+}
+
+// parseBWItem reads the answer of "bw get item ID": one JSON object. It
+// reads only the fields "id", "deletedDate", "archivedDate" and
+// "login.password". "id" must be the asked ID id, in upper or lower case.
+// An item in the trash (a deletedDate that is not null) is ErrTrashed, and
+// an archived item (an archivedDate that is not null) is ErrArchived: bw
+// still returns such items by their ID, but its own search skips them. A
+// normal item has these two fields as null, or not at all. A password
+// with a NUL byte is ErrBadValue (see keepValue). A password that is
+// missing, null or empty is ErrNoValue; so is an item that is not
+// a login, which has no login field. The other fields are skipped;
+// "notes", "fields", "passwordHistory" and "login.totp" can hold secrets
+// too.
+func parseBWItem(data []byte, id string) (Value, error) {
+	var password []byte
+	sameID, trashed, archived := false, false, false
+	err := readAnswer(data, func(r *jsonReader, key string) error {
+		switch key {
+		case "id":
+			return readID(r, id, &sameID)
+		case "deletedDate":
+			return readDate(r, &trashed)
+		case "archivedDate":
+			return readDate(r, &archived)
+		case "login":
+			if r.peek() == 'n' {
+				return r.null()
+			}
+			return r.object(func(r *jsonReader, key string) error {
+				if key != "password" {
+					return r.skip()
+				}
+				switch r.peek() {
+				case 'n':
+					return r.null()
+				case '"':
+					b, err := r.secret()
+					password = b
+					return err
+				}
+				return ErrBadAnswer
+			})
+		}
+		return r.skip()
+	})
+	switch {
+	case err != nil:
+	case !sameID:
+		err = errOtherItem
+	case trashed:
+		err = ErrTrashed
+	case archived:
+		err = ErrArchived
+	}
+	return keepValue(password, err)
+}
+
+// readID reads the field "id" of an answer. It sets *same when the ID is
+// want, in upper or lower case.
+func readID(r *jsonReader, want string, same *bool) error {
+	got, err := r.text(maxIDLen)
+	if err != nil {
+		return err
+	}
+	*same = sameUUID(got, want)
+	return nil
+}
+
+// sameUUID reports whether a and b are the same text, where only the
+// ASCII letters may differ in case. strings.EqualFold also takes other
+// letters, such as the Kelvin sign for "k".
+func sameUUID(a, b string) bool {
+	if len(a) != len(b) {
+		return false
+	}
+	for i := 0; i < len(a); i++ {
+		if lower(a[i]) != lower(b[i]) {
+			return false
+		}
+	}
+	return true
+}
+
+// lower returns c in lower case, if it is an ASCII letter.
+func lower(c byte) byte {
+	if 'A' <= c && c <= 'Z' {
+		return c + 'a' - 'A'
+	}
+	return c
+}
+
+// readDate reads a date of a bw item, such as "deletedDate": null, or a
+// string, which sets *set.
+func readDate(r *jsonReader, set *bool) error {
+	switch r.peek() {
+	case 'n':
+		return r.null()
+	case '"':
+		*set = true
+		return r.skip()
+	}
+	return ErrBadAnswer
+}
+
+// keepValue returns b as a Value when err is nil and b is a value that
+// nodkey can hand out, and clears b otherwise. An empty value is
+// ErrNoValue. A value with a NUL byte is ErrBadValue: a command's
+// environment cannot hold it, because execve ends each variable at a NUL.
+func keepValue(b []byte, err error) (Value, error) {
+	switch {
+	case err != nil:
+	case len(b) == 0:
+		err = ErrNoValue
+	case bytes.IndexByte(b, 0) >= 0:
+		err = ErrBadValue
+	}
+	if err != nil {
+		clear(b)
+		return Value{}, err
+	}
+	return newValue(b), nil
+}
+
+// parseBWStatus reads the answer of "bw status": one JSON object. It
+// reads only the field "status": unauthenticated, locked or unlocked. The
+// other fields, such as "userEmail", are skipped.
+func parseBWStatus(data []byte) (string, error) {
+	status := ""
+	err := readAnswer(data, func(r *jsonReader, key string) error {
+		if key != "status" {
+			return r.skip()
+		}
+		s, err := r.text(maxStatusLen)
+		status = s
+		return err
+	})
+	if err != nil {
+		return "", err
+	}
+	switch status {
+	case bwUnauthenticated, bwLocked, bwUnlocked:
+		return status, nil
+	}
+	return "", ErrBadAnswer
+}
+
+// parseKey reads a bws access token from its file, or a bw session key
+// from the answer of "bw unlock --raw". Whitespace at the start and the
+// end is dropped, such as the newline at the end of a file. What is left
+// must be 1 to limit printable ASCII characters without spaces; a token
+// and a session key have no others. It returns a new Value; the caller
+// clears data. An error never shows the key.
+func parseKey(data []byte, limit int) (Value, bool) {
+	start, end := 0, len(data)
+	for start < end && isSpace(data[start]) {
+		start++
+	}
+	for end > start && isSpace(data[end-1]) {
+		end--
+	}
+	if end == start || end-start > limit {
+		return Value{}, false
+	}
+	for _, c := range data[start:end] {
+		if c <= ' ' || c > '~' {
+			return Value{}, false
+		}
+	}
+	return newValue(append([]byte(nil), data[start:end]...)), true
+}
+
+// isSpace reports whether c is ASCII whitespace.
+func isSpace(c byte) bool {
+	switch c {
+	case ' ', '\t', '\n', '\r', '\v', '\f':
+		return true
+	}
+	return false
+}