diff --git a/.gitignore b/.gitignore index 26b5cfc0..0c8e31a7 100644 --- a/.gitignore +++ b/.gitignore @@ -16,3 +16,4 @@ modules/mesh-watcher/mesh-watcher modules/dnsmasq/dnsmasq modules/mesh-delivery/mesh-delivery modules/mesh-issues/cmd/mesh-issues/mesh-issues +modules/mounts/cmd/mounts/mounts diff --git a/modules/mounts/README.md b/modules/mounts/README.md index 76fad833..0d321d10 100644 --- a/modules/mounts/README.md +++ b/modules/mounts/README.md @@ -66,11 +66,12 @@ Both are preferences whose default, `none`, names nothing (ADR 0262 allows no em For example `library=/srv/library downloads=/srv/downloads:ro`. - `sources`: space-separated occasional sources, each optionally `:ro`: `name=smb://[@]/@` or `name=disk:@`. - For example `games=smb://root@192.0.2.76/Share@/mnt/games`. An SMB source's username is in its entry: it - is not a secret (novox/hq ADR 0283). It is percent-decoded, so a device that needs a domain takes it as + For example `games=smb://root@192.0.2.76/Share@/mnt/games`; the host is a name or an IPv4 address. An SMB + source's username is in its entry: it is not a secret (novox/hq ADR 0283). It is percent-decoded, so a device that needs a domain takes it as `WORKGROUP%5Cuser` (for `WORKGROUP\user`), and an `@` in it is written `%40`; it holds no space, comma or `=`. A password never goes in the entry: `user:password@` is refused, saying so and never what was - written. An SMB entry that cannot be read is said without what it holds. + written. An entry that cannot be read is said by its source's name, or by its position when even the name + cannot be read, and never with what it holds. A mount point is an absolute, plainly written path, not a directory of the system's own (`/etc`, `/usr`, `/run`, …). An entry that cannot be read is said, and only that entry is left out. diff --git a/modules/mounts/cmd/mounts/config.go b/modules/mounts/cmd/mounts/config.go index 1e8b1f13..3a7587cb 100644 --- a/modules/mounts/cmd/mounts/config.go +++ b/modules/mounts/cmd/mounts/config.go @@ -15,6 +15,7 @@ import ( "path/filepath" "regexp" "strings" + "unicode" ) // Where the module keeps its things. ConfigFile is placed by the mesh; the rest by the module's code. @@ -162,18 +163,13 @@ func ParseSources(v string) ([]Source, []string) { var out []Source var problems []string seen := map[string]bool{} - for _, e := range entries(v) { + // **Nothing an entry holds is said back but a name that reads as one**: an entry may carry a password + // where the user goes, in any scheme, and a password with a space or a comma splits it in two. An entry + // is named by its position otherwise. + for i, e := range entries(v) { name, rest, ok := strings.Cut(e, "=") - if !ok { - if strings.Contains(e, "://") { - problems = append(problems, "an entry of sources is not name=@") - } else { - problems = append(problems, fmt.Sprintf("the source %q is not name=@", e)) - } - continue - } - if !nameRe.MatchString(name) { - problems = append(problems, fmt.Sprintf("the source name %q is not lower-case letters, digits and dashes", name)) + if !ok || !nameRe.MatchString(name) { + problems = append(problems, fmt.Sprintf("entry %d of sources is not name=@, its name lower-case letters, digits and dashes", i+1)) continue } at := strings.LastIndex(rest, "@") @@ -183,14 +179,13 @@ func ParseSources(v string) ([]Source, []string) { } where, pointRO := rest[:at], rest[at+1:] point, ro := readOnly(pointRO) - if err := CheckMountpoint(point); err != nil { - problems = append(problems, fmt.Sprintf("the source %s: %v", name, err)) + if CheckMountpoint(point) != nil { + problems = append(problems, fmt.Sprintf("the source %s: its mount point is not an absolute, plainly written path outside the system's own directories", name)) continue } s := Source{Name: name, Mountpoint: point, ReadOnly: ro} switch { case strings.HasPrefix(where, "smb://"): - // Nothing of an SMB entry is said back: it may carry a password where the user goes. user, host, share, problem := parseSMB(strings.TrimPrefix(where, "smb://")) if problem != "" { problems = append(problems, fmt.Sprintf("the source %s: %s", name, problem)) @@ -200,16 +195,16 @@ func ParseSources(v string) ([]Source, []string) { case strings.HasPrefix(where, "disk:"): uuid := strings.TrimPrefix(where, "disk:") if !uuidRe.MatchString(uuid) { - problems = append(problems, fmt.Sprintf("the source %s: %q is not disk:", name, where)) + problems = append(problems, fmt.Sprintf("the source %s is not disk:", name)) continue } s.Kind, s.UUID = "disk", uuid default: - problems = append(problems, fmt.Sprintf("the source %s: %q is neither smb:/// nor disk:", name, where)) + problems = append(problems, fmt.Sprintf("the source %s is neither smb://[@]/ nor disk:", name)) continue } if seen[name] || seen["@"+point] { - problems = append(problems, fmt.Sprintf("the source %s or its mount point %s is named twice", name, point)) + problems = append(problems, fmt.Sprintf("the source %s or its mount point is named twice", name)) continue } seen[name], seen["@"+point] = true, true @@ -255,7 +250,7 @@ func ParseConfig(text string) Config { kept := c.Sources[:0] for _, s := range c.Sources { if other, dup := points[s.Mountpoint]; dup { - c.Problems = append(c.Problems, fmt.Sprintf("the source %s mounts at %s, where the share %s already does", s.Name, s.Mountpoint, other)) + c.Problems = append(c.Problems, fmt.Sprintf("the source %s mounts where the share %s already does", s.Name, other)) continue } kept = append(kept, s) @@ -300,16 +295,19 @@ var userRe = regexp.MustCompile(`^[^\s=,]{1,64}$`) // percent-decoded, so `DOMAIN%5Cuser` or a user holding `@` can be written; a password in its place // (`user:pass@`) is refused, and no problem says anything of what was written. func parseSMB(body string) (user, host, share, problem string) { - const form = "is not smb://[@]/" + const form = "is not smb://[@]/, the host a name or an IPv4 address" if at := strings.LastIndex(body, "@"); at >= 0 { raw := body[:at] body = body[at+1:] - if strings.Contains(raw, ":") { + switch { + case strings.Contains(raw, ":"): return "", "", "", "its entry carries a password in the URL; a password is given as the secret smb-password-, never in a setting" + case strings.Contains(raw, "@"): + return "", "", "", "its user holds an @; write it %40" } decoded, err := url.PathUnescape(raw) - if err != nil || !userRe.MatchString(decoded) { - return "", "", "", "its user is not a username without spaces, commas or = (percent-encode a \\ or an @ in it)" + if err != nil || !userRe.MatchString(decoded) || strings.IndexFunc(decoded, unicode.IsControl) >= 0 { + return "", "", "", "its user is not a username without spaces, commas, = or control characters (percent-encode a \\ or an @ in it)" } user = decoded } diff --git a/modules/mounts/cmd/mounts/write_test.go b/modules/mounts/cmd/mounts/write_test.go index 24a3002a..5c08b4c6 100644 --- a/modules/mounts/cmd/mounts/write_test.go +++ b/modules/mounts/cmd/mounts/write_test.go @@ -468,6 +468,10 @@ func TestAnSMBSourcesUserIsInItsEntryAndAPasswordThereIsRefusedUnsaid(t *testing "a user with a space, encoded": "e=smb://a%20" + testPassword + "@nas/Media@/mnt/e", "a bad percent escape": "e=smb://%zz" + testPassword + "@nas/Media@/mnt/e", "no mount point, with a password": "smb://root:" + testPassword + "@nas/Media", + "another scheme, upper case": "e=SMB://root:" + testPassword + "@nas/Media@/mnt/e", + "a cifs URL": "e=cifs://root:" + testPassword + "@nas/Media@/mnt/e", + "two @ before the host": "e=smb://me@example@nas/Media@/mnt/e" + testPassword, + "a control character in the user": "e=smb://me%07" + testPassword + "@nas/Media@/mnt/e", } { got, problems := ParseSources(entry) said := strings.Join(problems, " ") @@ -478,6 +482,30 @@ func TestAnSMBSourcesUserIsInItsEntryAndAPasswordThereIsRefusedUnsaid(t *testing if _, problems := ParseSources("e=smb://root:" + testPassword + "@nas/Media@/mnt/e"); !strings.Contains(problems[0], "smb-password-") { t.Errorf("a password in the URL is said with where it goes: %q", problems) } + // A password with a space or a comma splits the entry: neither half, nor a name that is not one, is said. + const secret = "SECRETPW" + for _, v := range []string{ + "e=smb://root:SEC RETPW@nas/Media@/mnt/e", + "e=smb://root:SEC,RETPW@nas/Media@/mnt/e", + "e=SMB://root:" + secret + "@nas/Media@/mnt/e", + "e=cifs://root:" + secret + "@nas/Media@/mnt/e", + "SECRET" + "PW=smb://nas/Media@/mnt/e", + } { + _, problems := ParseSources(v) + said := strings.Join(problems, " ") + if len(problems) == 0 || strings.Contains(said, "RETPW") || strings.Contains(said, "SEC") { + t.Errorf("%q says %q", strings.ReplaceAll(v, "RETPW", ""), said) + } + // And nothing of it reaches the status or the health answer. + f := newSystem(t) + f.write(ConfigFile, "sources="+v+"\npasswords=/var/lib/mesh/mounts\nstate=/var/lib/mesh/mounts\n") + now := time.Now() + f.applier(now).Look(context.Background()) + raw, _ := json.Marshal(f.machine().Health(now)) + if strings.Contains(string(raw), "RETPW") || strings.Contains(f.everything("mounts.conf"), "RETPW") { + t.Errorf("%q leaked into the status or the health: %s", strings.ReplaceAll(v, "RETPW", ""), raw) + } + } } func TestTheHealthGivesAShareFiveMinutesAndAnAbsentSourceNone(t *testing.T) {