mounts: say nothing an entry of sources holds when it cannot be read

A password typed into an entry by mistake, in another scheme or split by a
space or a comma, was quoted back in the problem and so reached the status
and the health answer. An entry is now named by its source's name or its
position only; a user with a control character or a second @ is refused
(review of mesh-catalog#190, issue 386).

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://[<user>@]<host>/<share>@<mountpoint>` or `name=disk:<filesystem uuid>@<mountpoint>`.
-  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.

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=<where>@<mountpoint>")
-			} else {
-				problems = append(problems, fmt.Sprintf("the source %q is not name=<where>@<mountpoint>", 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=<where>@<mountpoint>, 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:<filesystem uuid>", name, where))
+				problems = append(problems, fmt.Sprintf("the source %s is not disk:<filesystem uuid>", name))
 				continue
 			}
 			s.Kind, s.UUID = "disk", uuid
 		default:
-			problems = append(problems, fmt.Sprintf("the source %s: %q is neither smb://<host>/<share> nor disk:<uuid>", name, where))
+			problems = append(problems, fmt.Sprintf("the source %s is neither smb://[<user>@]<host>/<share> nor disk:<uuid>", 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://[<user>@]<host>/<share>"
+	const form = "is not smb://[<user>@]<host>/<share>, 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-<source>, 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
 	}

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-<source>") {
 		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", "<pw>"), 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", "<pw>"), raw)
+		}
+	}
 }
 
 func TestTheHealthGivesAShareFiveMinutesAndAnAbsentSourceNone(t *testing.T) {