diff --git a/internal/catalogue/co_located_test.go b/internal/catalogue/co_located_test.go index bf098c16..3373936f 100644 --- a/internal/catalogue/co_located_test.go +++ b/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----- ` diff --git a/internal/catalogue/placement.go b/internal/catalogue/placement.go index 1f6a7e38..808151ea 100644 --- a/internal/catalogue/placement.go +++ b/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"} - systemRoots = []string{"/", "/var", "/var/lib", "/var/cache", "/var/log", "/var/tmp", "/var/spool", "/home", - "/mnt", "/media", "/srv", "/opt", "/tmp", "/storage", "/data", "/services"} + "/dev", "/bin", "/sbin", "/lib", "/lib32", "/lib64", "/var/lib/mesh", "/var/lib/mesh-host", "/var/spool", + "/var/lib/docker", "/var/lib/containers", "/opt"} + 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" diff --git a/internal/catalogue/settings.go b/internal/catalogue/settings.go index f7042491..2fab2177 100644 --- a/internal/catalogue/settings.go +++ b/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, -// because a provider serves its certificate authority's root to its consumers as one (step-ca to the route -// proxy). Each block is a BEGIN line, base64 lines of exactly 64 characters but the last, and the END line of -// 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 -} +// lineEnds are every character a reader may take as the end of a line: \n and \r, vertical tab and form feed, +// NEL, and the Unicode line and paragraph separators; and NUL, which ends a string early wherever C reads it. +const lineEnds = "\n\r\v\f\x00\u0085\u2028\u2029" -// base64Line is one line of a PEM body: 64 characters of the base64 alphabet, or for the last line at most 64, -// a whole number of groups, with at most two '=' at its end. -func base64Line(line string, last bool) bool { - if line == "" || len(line) > 64 || (!last && len(line) != 64) || len(line)%4 != 0 { +// rootSetting is the one setting that may hold lines: the certificate authority's root, which step-ca serves to +// its consumers as the setting `root` and which they write into a bundle file (the co-located path, issue 038). +const ( + 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, "=") - if len(line)-len(data) > 2 || (!last && data != line) { - return false + rest := []byte(v) + found := 0 + 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") { - return strings.NewReplacer("\n", `\n`, "\r", `\r`, "\x00", `\0`).Replace(at) + if strings.ContainsAny(at, lineEnds) { + 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: diff --git a/internal/catalogue/terminal_settings_test.go b/internal/catalogue/terminal_settings_test.go index 27a2d4ea..bc3831fe 100644 --- a/internal/catalogue/terminal_settings_test.go +++ b/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 -// a line in them that is not base64, is refused. -func TestAPEMBlockIsTheOneSettingWithLines(t *testing.T) { - m := Manifest{Module: "route-proxy"} - full := strings.Repeat("MIIB", 16) - pem := "-----BEGIN CERTIFICATE-----\n" + full + "\n" + full + "\nAbCd+/==\n-----END CERTIFICATE-----\n" - for _, ok := range []string{pem, pem + pem, strings.TrimSuffix(pem, "\n"), - "-----BEGIN CERTIFICATE-----\nMIIBeDCCAR2gAwIBAgIQfake000000000000000000000000\n-----END CERTIFICATE-----\n"} { - if err := JudgeSettings(m, []Layer{{From: "anchor", Values: map[string]any{"root": ok}}}, false); err != nil { - t.Errorf("a PEM block: %v", err) +// Lines are allowed in one setting only: step-ca's `root`, as certificates that encoding/pem decodes and +// x509.ParseCertificate parses (novox/hq issue 339). Any other module or key, another label, or a block that is +// not a certificate is refused like any other line break. +func TestOnlyTheAuthoritysRootMayHoldLines(t *testing.T) { + ca := Manifest{Module: "step-ca"} + judge := func(m Manifest, key, v string) error { + return JudgeSettings(m, []Layer{{From: "anchor", Values: map[string]any{key: v}}}, false) + } + for _, ok := range []string{servedRoot, servedRoot + servedRoot, strings.TrimSuffix(servedRoot, "\n")} { + if err := judge(ca, "root", ok); err != nil { + t.Errorf("the authority's root: %v", err) } } - for _, bad := range []string{ - pem + "PATH=/tmp\n", - "PATH=\n" + pem, - "-----BEGIN CERTIFICATE-----\n" + full + "\nPATH=\n-----END CERTIFICATE-----\n", - "-----BEGIN CERTIFICATE-----\nshort\n" + full + "\n-----END CERTIFICATE-----\n", - "-----BEGIN CERTIFICATE-----\n" + full + "\n-----END KEY-----\n", - "-----BEGIN CERTIFICATE-----\n-----END CERTIFICATE-----\n", - "-----BEGIN CERTIFICATE-----\r\n" + full + "\r\n-----END CERTIFICATE-----\r\n", + body := strings.TrimSuffix(strings.TrimPrefix(servedRoot, "-----BEGIN CERTIFICATE-----\n"), "-----END CERTIFICATE-----\n") + for name, bad := range map[string]string{ + "a line after it": servedRoot + "PATH=/tmp\n", + "a line before it": "PATH=\n" + servedRoot, + "another label": "-----BEGIN PRIVATE KEY-----\n" + body + "-----END PRIVATE KEY-----\n", + "headers": "-----BEGIN CERTIFICATE-----\nProc-Type: 4,ENCRYPTED\n\n" + body + "-----END CERTIFICATE-----\n", + "base64 that is no cert": "-----BEGIN CERTIFICATE-----\nMIIBeDCCAR2gAwIBAgIQfake000000000000000000000000\n-----END CERTIFICATE-----\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 { - t.Errorf("taken: %q", bad) + if err := judge(ca, "root", bad); err == nil { + 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) } } }