From a12b5908aa378ed9e09e9ed6eeb32c64bbfc3ba0 Mon Sep 17 00:00:00 2001 From: jochen Date: Sat, 10 Oct 2026 21:38:17 +0200 Subject: [PATCH] mounts: name a malformed entry of sources by its position, never by its name A password holding '=' and a space or comma splits the entry, and the tail read as name=... was said as "the source ", carrying part of the secret into the health answer and the operator's conditions (review of mesh-catalog#190, issue 386). --- modules/mounts/README.md | 4 ++-- modules/mounts/cmd/mounts/config.go | 25 +++++++++++++++---------- modules/mounts/cmd/mounts/write_test.go | 9 +++++++-- 3 files changed, 24 insertions(+), 14 deletions(-) diff --git a/modules/mounts/README.md b/modules/mounts/README.md index 0d321d10..65cac223 100644 --- a/modules/mounts/README.md +++ b/modules/mounts/README.md @@ -70,8 +70,8 @@ Both are preferences whose default, `none`, names nothing (ADR 0262 allows no em 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 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. + written. An entry that cannot be read whole is said as malformed by its position alone ("entry 2 of sources + is malformed: …"), never with its name or anything else 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 3a7587cb..b6922e4d 100644 --- a/modules/mounts/cmd/mounts/config.go +++ b/modules/mounts/cmd/mounts/config.go @@ -163,24 +163,24 @@ func ParseSources(v string) ([]Source, []string) { var out []Source var problems []string seen := map[string]bool{} - // **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. + // **Nothing an entry holds is said back until the whole entry has read**: 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, whose + // tail may itself look like name=…. A malformed entry is named by its position alone. for i, e := range entries(v) { name, rest, ok := strings.Cut(e, "=") 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)) + problems = append(problems, malformed(i, "it is not name=@, its name lower-case letters, digits and dashes")) continue } at := strings.LastIndex(rest, "@") if at < 0 { - problems = append(problems, fmt.Sprintf("the source %s names no mount point (…@)", name)) + problems = append(problems, malformed(i, "it names no mount point (…@)")) continue } where, pointRO := rest[:at], rest[at+1:] point, ro := readOnly(pointRO) 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)) + problems = append(problems, malformed(i, "its mount point is not an absolute, plainly written path outside the system's own directories")) continue } s := Source{Name: name, Mountpoint: point, ReadOnly: ro} @@ -188,19 +188,19 @@ func ParseSources(v string) ([]Source, []string) { case strings.HasPrefix(where, "smb://"): user, host, share, problem := parseSMB(strings.TrimPrefix(where, "smb://")) if problem != "" { - problems = append(problems, fmt.Sprintf("the source %s: %s", name, problem)) + problems = append(problems, malformed(i, problem)) continue } s.Kind, s.User, s.Host, s.Share = "smb", user, host, share case strings.HasPrefix(where, "disk:"): uuid := strings.TrimPrefix(where, "disk:") if !uuidRe.MatchString(uuid) { - problems = append(problems, fmt.Sprintf("the source %s is not disk:", name)) + problems = append(problems, malformed(i, "it is not disk:")) continue } s.Kind, s.UUID = "disk", uuid default: - problems = append(problems, fmt.Sprintf("the source %s is neither smb://[@]/ nor disk:", name)) + problems = append(problems, malformed(i, "it is neither smb://[@]/ nor disk:")) continue } if seen[name] || seen["@"+point] { @@ -213,6 +213,11 @@ func ParseSources(v string) ([]Source, []string) { return out, problems } +// malformed is a problem with the i-th entry of sources, by its position alone: nothing it holds is said. +func malformed(i int, why string) string { + return fmt.Sprintf("entry %d of sources is malformed: %s", i+1, why) +} + // ParseConfig reads the rendered file: `key=value` lines. func ParseConfig(text string) Config { kv := map[string]string{} @@ -301,7 +306,7 @@ func parseSMB(body string) (user, host, share, problem string) { body = body[at+1:] 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" + return "", "", "", "it 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" } diff --git a/modules/mounts/cmd/mounts/write_test.go b/modules/mounts/cmd/mounts/write_test.go index 5c08b4c6..ad9dd82c 100644 --- a/modules/mounts/cmd/mounts/write_test.go +++ b/modules/mounts/cmd/mounts/write_test.go @@ -490,10 +490,15 @@ func TestAnSMBSourcesUserIsInItsEntryAndAPasswordThereIsRefusedUnsaid(t *testing "e=SMB://root:" + secret + "@nas/Media@/mnt/e", "e=cifs://root:" + secret + "@nas/Media@/mnt/e", "SECRET" + "PW=smb://nas/Media@/mnt/e", + // A password holding '=' and a space, or '=' and a comma: the tail reads like name=… and is still not said. + "e=smb://root:sec RETPW=x@nas/Media@/mnt/e", + "e=smb://root:sec,RETPW=x@nas/Media@/mnt/e", + "e=smb://root:SEC RETPW=smb://nas/Media@/mnt/e", } { _, problems := ParseSources(v) said := strings.Join(problems, " ") - if len(problems) == 0 || strings.Contains(said, "RETPW") || strings.Contains(said, "SEC") { + if len(problems) == 0 || strings.Contains(strings.ToUpper(said), "RETPW") || strings.Contains(said, "SEC") || + strings.Contains(said, "sec") && !strings.Contains(said, "secret smb-password") { t.Errorf("%q says %q", strings.ReplaceAll(v, "RETPW", ""), said) } // And nothing of it reaches the status or the health answer. @@ -502,7 +507,7 @@ func TestAnSMBSourcesUserIsInItsEntryAndAPasswordThereIsRefusedUnsaid(t *testing 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") { + if strings.Contains(strings.ToUpper(string(raw)), "RETPW") || strings.Contains(strings.ToUpper(f.everything("mounts.conf")), "RETPW") { t.Errorf("%q leaked into the status or the health: %s", strings.ReplaceAll(v, "RETPW", ""), raw) } }