Let only the authority's root hold lines, refuse every line end, and keep places off the runtime's data and any .ssh

The review of hq issue 339 found the PEM exception too wide (any module, any
key, any label, anything base64), \v, \f, NEL and the Unicode separators
still let a value end a line in some readers, and the spool, /opt, the
container runtimes' data and an account's .ssh still placeable. Lines are now
taken only in step-ca's root setting, as certificates encoding/pem decodes and
x509 parses; every line end is refused; and those paths are the machine's own.
The test certificate is a real one, made for the tests with its key thrown away.

internal/catalogue/co_located_test.go

@@ -17,9 +17,16 @@ import (
 //
 // 04-ISSUES/038 was the first of them (the port). These are the rest.
 
-// A root certificate, in the shape a certificate authority serves one.
+// A root certificate, as a certificate authority serves one: a real, self-signed one made for these tests (its key
+// thrown away), because the one setting that may hold lines must parse as a certificate (novox/hq issue 339).
 const servedRoot = `-----BEGIN CERTIFICATE-----
-MIIBeDCCAR2gAwIBAgIQfake000000000000000000000000
+MIIBPjCB8aADAgECAhRZG3p93hUUB5bz00uxhYo/nJnTQjAFBgMrZXAwFDESMBAG
+A1UEAwwJdGVzdCByb290MCAXDTI2MTAwODIyMjE0NloYDzIxMjYwOTE0MjIyMTQ2
+WjAUMRIwEAYDVQQDDAl0ZXN0IHJvb3QwKjAFBgMrZXADIQA0pt/ld+W0MXwBhPfO
+cuAt56kIW6Qcn+4vqWpuvHTiqaNTMFEwHQYDVR0OBBYEFIjgaLk4OVGIOeZQiqnN
+p9vhciFjMB8GA1UdIwQYMBaAFIjgaLk4OVGIOeZQiqnNp9vhciFjMA8GA1UdEwEB
+/wQFMAMBAf8wBQYDK2VwA0EAl9uWeSM2XAV8u0reyV3BLRxNVik+4FCRO1QKPs2k
+IlB1rK9oAOYManH+VFuBMI/JJ31ajSti81q4E0CSw8r5DQ==
 -----END CERTIFICATE-----
 `
 

internal/catalogue/placement.go

@@ -54,18 +54,28 @@ var ownerShape = regexp.MustCompile(`^[0-9]+:[0-9]+$`)
 // before anything is kept or composed, and the node-engine refuses them again where it applies.
 //
 // systemTrees are refused at and below: the machine's system, the kernel's, the boot loader's, root's home,
-// what lives only while the machine runs, and the node-engine's and the mesh's own state. systemRoots are
+// what lives only while the machine runs, the spool (cron's tables are there), the container runtimes' data
+// (every container's filesystem), /opt, and the node-engine's and the mesh's own state. Any home's .ssh is
+// refused as well, wherever the home is. systemRoots are
 // refused at, and wherever a path holds one (an ancestor of /var/lib holds it): each is the parent of every
 // module's or every person's directories, and owning it is owning all of them.
 var (
 	systemTrees = []string{"/etc", "/usr", "/boot", "/root", "/run", "/var/run", "/var/lock", "/proc", "/sys",
-		"/dev", "/bin", "/sbin", "/lib", "/lib32", "/lib64", "/var/lib/mesh", "/var/lib/mesh-host"}
+		"/dev", "/bin", "/sbin", "/lib", "/lib32", "/lib64", "/var/lib/mesh", "/var/lib/mesh-host", "/var/spool",
-	systemRoots = []string{"/", "/var", "/var/lib", "/var/cache", "/var/log", "/var/tmp", "/var/spool", "/home",
+		"/var/lib/docker", "/var/lib/containers", "/opt"}
-		"/mnt", "/media", "/srv", "/opt", "/tmp", "/storage", "/data", "/services"}
+	systemRoots = []string{"/", "/var", "/var/lib", "/var/cache", "/var/log", "/var/tmp", "/home",
+		"/mnt", "/media", "/srv", "/tmp", "/storage", "/data", "/services"}
 )
 
 // systemPath says why a clean absolute path is the machine's own and never a placement's or an access's, or "".
 func systemPath(path string) string {
+	// Any home's keys, wherever the home is: a .ssh directory is its account's, and the keys and the list of who
+	// may log in as it are in there.
+	for _, part := range strings.Split(path, "/") {
+		if part == ".ssh" {
+			return path + " is an account's .ssh, which holds its keys and who may log in as it"
+		}
+	}
 	for _, tree := range systemTrees {
 		if path == tree || strings.HasPrefix(path, tree+"/") {
 			return path + " is in " + tree + ", the machine's own or the mesh's state"

internal/catalogue/settings.go

@@ -1,10 +1,14 @@
 package catalogue
 
 import (
+	"bytes"
+	"crypto/x509"
 	"encoding/json"
+	"encoding/pem"
 	"fmt"
 	"regexp"
 	"sort"
+	"strconv"
 	"strings"
 )
 
@@ -209,18 +213,25 @@ func deepCopy(in map[string]any) map[string]any {
 	return out
 }
 
-// settingsHoldOneLine refuses a line break, a carriage return or a NUL in any string of any setting, for every
+// settingsHoldOneLine refuses a line break, a carriage return, any other line end (lineEnds) or a NUL in any
+// string of any setting, for every
 // module, at any depth, keys as well as values, in an object or a list (novox/hq issue 339). A value is
 // substituted into env and configuration files the node-engine writes as root — an app's env file, a logind
 // drop-in — and a line break there is a line of the caller's own: a directive, an assignment, a section. A NUL
 // ends a string early wherever C reads it. Judged where a layer is kept and again where it is composed, so a
 // layer that holds one, however it got into the store, is said with its key. What must hold lines is a file
-// of the module's own, never a setting.
+// of the module's own, never a setting — with one exception, the certificate authority's root as certificates
+// (certificates), because that is how step-ca serves it.
 func settingsHoldOneLine(module string, layers []Layer) error {
 	for _, layer := range layers {
 		for _, key := range sortedKeysAny(layer.Values) {
+			if module == rootModule && key == rootSetting {
+				if text, ok := layer.Values[key].(string); ok && certificates(text) {
+					continue
+				}
+			}
 			if at := lineBreakIn(layer.Values[key], key); at != "" {
-				return fmt.Errorf("%s: the setting %s in %q holds a line break, a carriage return or a NUL, which a "+
+				return fmt.Errorf("%s: the setting %s in %q holds a line break, a carriage return, another line end or a NUL, which a "+
 					"file it is written into would read as a line of its own; a setting is one line (novox/hq "+
 					"issue 339)", module, at, layer.From)
 			}
@@ -229,66 +240,51 @@ func settingsHoldOneLine(module string, layers []Layer) error {
 	return nil
 }
 
-// pemBlocks says whether a value is PEM blocks and nothing else: the one value with lines a setting may hold,
+// lineEnds are every character a reader may take as the end of a line: \n and \r, vertical tab and form feed,
-// because a provider serves its certificate authority's root to its consumers as one (step-ca to the route
+// NEL, and the Unicode line and paragraph separators; and NUL, which ends a string early wherever C reads it.
-// proxy). Each block is a BEGIN line, base64 lines of exactly 64 characters but the last, and the END line of
+const lineEnds = "\n\r\v\f\x00\u0085\u2028\u2029"
-// the same label; the last line is a whole number of base64 groups, padded at its end alone. So no line of it
-// is a path, an option, a section or an assignment of a name a program reads — a line break with anything
-// else around it is refused.
-func pemBlocks(v string) bool {
-	lines := strings.Split(strings.TrimSuffix(v, "\n"), "\n")
-	if len(lines) < 3 {
-		return false
-	}
-	for i := 0; i < len(lines); {
-		label, ok := strings.CutPrefix(lines[i], "-----BEGIN ")
-		if !ok || !strings.HasSuffix(label, "-----") {
-			return false
-		}
-		label = strings.TrimSuffix(label, "-----")
-		if label == "" || strings.Trim(label, "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789 ") != "" {
-			return false
-		}
-		i++
-		body := 0
-		for i < len(lines) && !strings.HasPrefix(lines[i], "-----END ") {
-			body++
-			i++
-		}
-		if body == 0 || i == len(lines) || lines[i] != "-----END "+label+"-----" {
-			return false
-		}
-		for j, line := range lines[i-body : i] {
-			if !base64Line(line, j == body-1) {
-				return false
-			}
-		}
-		i++
-	}
-	return true
-}
 
-// base64Line is one line of a PEM body: 64 characters of the base64 alphabet, or for the last line at most 64,
+// rootSetting is the one setting that may hold lines: the certificate authority's root, which step-ca serves to
-// a whole number of groups, with at most two '=' at its end.
+// its consumers as the setting `root` and which they write into a bundle file (the co-located path, issue 038).
-func base64Line(line string, last bool) bool {
+const (
-	if line == "" || len(line) > 64 || (!last && len(line) != 64) || len(line)%4 != 0 {
+	rootModule  = "step-ca"
+	rootSetting = "root"
+)
+
+// certificates says whether a value is one or more PEM CERTIFICATE blocks and nothing else: each decodes with
+// encoding/pem, carries no headers, and parses with x509.ParseCertificate; nothing but a final line break
+// surrounds them, and every line ends with \n alone.
+func certificates(v string) bool {
+	if strings.ContainsAny(v, "\r\v\f\x00\u0085\u2028\u2029") {
 		return false
 	}
-	data := strings.TrimRight(line, "=")
+	rest := []byte(v)
-	if len(line)-len(data) > 2 || (!last && data != line) {
+	found := 0
-		return false
+	for len(rest) > 0 {
+		if !bytes.HasPrefix(rest, []byte("-----BEGIN ")) {
+			return false
+		}
+		block, after := pem.Decode(rest)
+		if block == nil || block.Type != "CERTIFICATE" || len(block.Headers) > 0 {
+			return false
+		}
+		if _, err := x509.ParseCertificate(block.Bytes); err != nil {
+			return false
+		}
+		found++
+		rest = after
 	}
-	return strings.Trim(data, "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/") == ""
+	return found > 0
 }
 
 // lineBreakIn is where the first string under v holding \n, \r or NUL is, or "".
 func lineBreakIn(v any, at string) string {
-	if strings.ContainsAny(at, "\n\r\x00") {
+	if strings.ContainsAny(at, lineEnds) {
-		return strings.NewReplacer("\n", `\n`, "\r", `\r`, "\x00", `\0`).Replace(at)
+		return strconv.Quote(at)
 	}
 	switch t := v.(type) {
 	case string:
-		if strings.ContainsAny(t, "\n\r\x00") && !pemBlocks(t) {
+		if strings.ContainsAny(t, lineEnds) {
 			return at
 		}
 	case map[string]any:

internal/catalogue/terminal_settings_test.go

@@ -55,29 +55,75 @@ func TestASettingHoldsOneLine(t *testing.T) {
 	}
 }
 
-// PEM blocks alone may hold lines: a provider serves its authority's root as a setting. Anything around them, or
+// Lines are allowed in one setting only: step-ca's `root`, as certificates that encoding/pem decodes and
-// a line in them that is not base64, is refused.
+// x509.ParseCertificate parses (novox/hq issue 339). Any other module or key, another label, or a block that is
-func TestAPEMBlockIsTheOneSettingWithLines(t *testing.T) {
+// not a certificate is refused like any other line break.
-	m := Manifest{Module: "route-proxy"}
+func TestOnlyTheAuthoritysRootMayHoldLines(t *testing.T) {
-	full := strings.Repeat("MIIB", 16)
+	ca := Manifest{Module: "step-ca"}
-	pem := "-----BEGIN CERTIFICATE-----\n" + full + "\n" + full + "\nAbCd+/==\n-----END CERTIFICATE-----\n"
+	judge := func(m Manifest, key, v string) error {
-	for _, ok := range []string{pem, pem + pem, strings.TrimSuffix(pem, "\n"),
+		return JudgeSettings(m, []Layer{{From: "anchor", Values: map[string]any{key: v}}}, false)
-		"-----BEGIN CERTIFICATE-----\nMIIBeDCCAR2gAwIBAgIQfake000000000000000000000000\n-----END CERTIFICATE-----\n"} {
+	}
-		if err := JudgeSettings(m, []Layer{{From: "anchor", Values: map[string]any{"root": ok}}}, false); err != nil {
+	for _, ok := range []string{servedRoot, servedRoot + servedRoot, strings.TrimSuffix(servedRoot, "\n")} {
-			t.Errorf("a PEM block: %v", err)
+		if err := judge(ca, "root", ok); err != nil {
+			t.Errorf("the authority's root: %v", err)
 		}
 	}
-	for _, bad := range []string{
+	body := strings.TrimSuffix(strings.TrimPrefix(servedRoot, "-----BEGIN CERTIFICATE-----\n"), "-----END CERTIFICATE-----\n")
-		pem + "PATH=/tmp\n",
+	for name, bad := range map[string]string{
-		"PATH=\n" + pem,
+		"a line after it":        servedRoot + "PATH=/tmp\n",
-		"-----BEGIN CERTIFICATE-----\n" + full + "\nPATH=\n-----END CERTIFICATE-----\n",
+		"a line before it":       "PATH=\n" + servedRoot,
-		"-----BEGIN CERTIFICATE-----\nshort\n" + full + "\n-----END CERTIFICATE-----\n",
+		"another label":          "-----BEGIN PRIVATE KEY-----\n" + body + "-----END PRIVATE KEY-----\n",
-		"-----BEGIN CERTIFICATE-----\n" + full + "\n-----END KEY-----\n",
+		"headers":                "-----BEGIN CERTIFICATE-----\nProc-Type: 4,ENCRYPTED\n\n" + body + "-----END CERTIFICATE-----\n",
-		"-----BEGIN CERTIFICATE-----\n-----END CERTIFICATE-----\n",
+		"base64 that is no cert": "-----BEGIN CERTIFICATE-----\nMIIBeDCCAR2gAwIBAgIQfake000000000000000000000000\n-----END CERTIFICATE-----\n",
-		"-----BEGIN CERTIFICATE-----\r\n" + full + "\r\n-----END CERTIFICATE-----\r\n",
+		"carriage returns":       strings.ReplaceAll(servedRoot, "\n", "\r\n"),
 	} {
-		if err := JudgeSettings(m, []Layer{{From: "anchor", Values: map[string]any{"root": bad}}}, false); err == nil {
+		if err := judge(ca, "root", bad); err == nil {
-			t.Errorf("taken: %q", bad)
+			t.Errorf("%s was taken", name)
+		}
+	}
+	if err := judge(ca, "other", servedRoot); err == nil {
+		t.Error("a certificate in another key of the authority was taken")
+	}
+	if err := judge(Manifest{Module: "mailu"}, "root", servedRoot); err == nil {
+		t.Error("a certificate in another module's setting was taken")
+	}
+	if err := JudgeSettings(ca, []Layer{{From: "anchor", Values: map[string]any{"root": []any{servedRoot}}}}, false); err == nil {
+		t.Error("a certificate below the top of the setting was taken")
+	}
+}
+
+// Every character a reader takes as the end of a line is refused, not only \n and \r: vertical tab, form feed,
+// NEL and the Unicode line and paragraph separators (novox/hq issue 339).
+func TestEveryLineEndIsRefused(t *testing.T) {
+	m := Manifest{Module: "mailu"}
+	for _, v := range []string{"a\vb", "a\fb", "a\u0085b", "a\u2028b", "a\u2029b"} {
+		if err := JudgeSettings(m, []Layer{{From: "home", Values: map[string]any{"v": v}}}, false); err == nil ||
+			!strings.Contains(err.Error(), "line break") {
+			t.Errorf("%q: %v", v, err)
+		}
+	}
+}
+
+// The review's additions: the spool, the container runtimes' data, /opt, and any home's .ssh.
+func TestTheRuntimesDataAndAnyHomesSSHAreTheMachinesOwn(t *testing.T) {
+	m := Manifest{Module: "notes", Resources: []map[string]any{{"id": "data", "type": "directory"}},
+		Accesses: []Access{{ID: "media"}}}
+	for _, path := range []string{"/var/spool", "/var/spool/cron", "/var/lib/docker", "/var/lib/docker/volumes",
+		"/var/lib/containers/storage", "/opt", "/opt/app", "/home/alice/.ssh", "/home/alice/.ssh/keys",
+		"/srv/backup/.ssh", "/root/.ssh"} {
+		layers := []Layer{{From: "laptop", Values: map[string]any{PlacesSetting: map[string]any{"data": path}}}}
+		if _, err := Places(m, layers); err == nil || !strings.Contains(err.Error(), "issue 339") {
+			t.Errorf("a place at %s: %v", path, err)
+		}
+		layers = []Layer{{From: "laptop", Values: map[string]any{AccessesSetting: map[string]any{"media": path}}}}
+		if _, err := AccessPlaces(m, layers); err == nil || !strings.Contains(err.Error(), "issue 339") {
+			t.Errorf("an access at %s: %v", path, err)
+		}
+	}
+	for _, path := range []string{"/var/lib/dockerish", "/home/alice/ssh", "/home/alice/.sshd-notes", "/storage/media"} {
+		layers := []Layer{{From: "laptop", Values: map[string]any{PlacesSetting: map[string]any{"data": path}}}}
+		if _, err := Places(m, layers); err != nil {
+			t.Errorf("a place at %s: %v", path, err)
 		}
 	}
 }