Merge pull request 'mounts: the SMB login is typed as one line at the desk's prompt (issue 379)' (#179) from fix/mounts-one-line-smb-login into main

modules/mounts/README.md

@@ -73,8 +73,22 @@ A mount point is an absolute, plainly written path, not a directory of the syste
 ## The secret
 
 `smb-credentials` is the module's own secret, issued by an outside party (the device's owner), so the mesh
-never makes one in its place. The operator gives it with
-`secret accept <node> mounts smb-credentials --from <file>`; the file holds one section per SMB source:
+never makes one in its place. It holds one login per SMB source, named by the source's name in brackets.
+
+The operator gives it at the desk with `secret ask <node> mounts smb-credentials` (ADR 0277): a one-line
+prompt opens that shows nothing of what is typed, and the login is typed as one line, the keys
+space-separated after the name:
+
+```
+[games] username=… password=…
+```
+
+`password=` is last and takes everything after it to the end of the line, so a password may hold spaces
+and `=`; `domain=`, when the device wants one, comes before it. The line is read trimmed, so a password
+does not end with a space; after `password=` it is kept as typed, so it may begin with one. Several
+sources are several such lines, which the prompt cannot take at once: a file can, given with
+`secret accept <node> mounts smb-credentials --from <file>`, holding
+one line per source, or one section per source with one key a line:
 
 ```
 [games]
@@ -83,8 +97,10 @@ password=…
 domain=…        (optional)
 ```
 
-A source whose section is missing is not mounted, and is said with that remedy. No tool answers a
-password: the options, fstab lines and mountinfo the tools read are redacted.
+Both forms give the same login, and a file may mix them. A line the module cannot read is said with the
+form it expected, never with what was typed. A source whose login is missing is not mounted, and is said
+with that remedy. No tool answers a password: the options, fstab lines and mountinfo the tools read are
+redacted.
 
 ## The processes and the bus
 

modules/mounts/cmd/mounts/apply.go

@@ -303,7 +303,8 @@ func (a *Applier) Look(ctx context.Context) Status {
 			c, ok := creds[s.Name]
 			if !ok || cfg.State == "" {
 				notSet[s.Name] = ShareStatus{Name: s.Name, Kind: "smb", Occasional: true, Mountpoint: s.Mountpoint, State: StateFailed,
-					Why: fmt.Sprintf("its login is not given: put a [%s] section in the module's secret with `secret accept <node> mounts smb-credentials --from <file>`", s.Name)}
+					Why: fmt.Sprintf("its login is not given: at the desk, `secret ask <node> mounts smb-credentials` and type one line `[%s] username=… password=…`; "+
+						"or put a [%s] section in a file and `secret accept <node> mounts smb-credentials --from <file>`", s.Name, s.Name)}
 				continue
 			}
 			file := filepath.Join(cfg.State, "smb-"+s.Name+".credentials")

modules/mounts/cmd/mounts/config.go

@@ -270,54 +270,98 @@ type SMBCredential struct {
 	Domain   string
 }
 
-// ParseSMBCredentials reads the secret `smb-credentials`: one section per source,
+// SMBOneLineForm is the form of a login the operator types at the desk's one-line prompt (`secret ask`,
+// novox/hq ADR 0277), said in every refusal so the operator knows what to type next.
+const SMBOneLineForm = "[<source>] username=… password=…"
+
+// ParseSMBCredentials reads the secret `smb-credentials`: one login per SMB source, named by its section.
+// A login is one line, as typed at the desk's prompt (ADR 0277), with the source's name in brackets and
+// the keys after it, space-separated:
+//
+//	[name] username=… password=…
+//
+// `password=` is last and takes everything after it to the end of the line, so a password may hold spaces
+// and `=`; `domain=`, when given, comes before it. Or a login is a section of a file, as `secret accept
+// --from <file>` takes it, one key a line:
 //
 //	[name]
 //	username=…
 //	password=…
 //	domain=…        (optional)
 //
-// Its values are never answered by any tool; a problem names the section and the key, never the value.
+// The two forms give the same login, and a file may mix them. A line is read trimmed, so a password does
+// not end with a space; after `password=` it is kept as typed, so it may begin with one. The values are
+// never answered by any tool; a problem names the line or the section, never a value nor a key the
+// operator typed.
 func ParseSMBCredentials(text string) (map[string]SMBCredential, []string) {
 	out := map[string]SMBCredential{}
 	var problems []string
+	// section is the open section of the file form, whose key=value lines follow; "" is none. A one-line
+	// login closes it, so a key on a later line cannot land in a login given whole.
 	section := ""
 	for i, raw := range strings.Split(text, "\n") {
 		line := strings.TrimSpace(raw)
 		if line == "" || strings.HasPrefix(line, "#") || strings.HasPrefix(line, ";") {
 			continue
 		}
-		if strings.HasPrefix(line, "[") && strings.HasSuffix(line, "]") {
-			section = strings.TrimSpace(line[1 : len(line)-1])
-			if _, dup := out[section]; dup {
-				problems = append(problems, fmt.Sprintf("the section [%s] is given twice; the first is kept", section))
-				section = ""
+		if !strings.HasPrefix(line, "[") {
+			if section == "" {
+				problems = append(problems, fmt.Sprintf("line %d is outside a [section]: a login is one line as %s, or a [section] with username= and password= lines", i+1, SMBOneLineForm))
 				continue
 			}
-			out[section] = SMBCredential{}
+			k, v, ok := strings.Cut(line, "=")
+			if !ok {
+				problems = append(problems, fmt.Sprintf("line %d in [%s] is not key=value", i+1, section))
+				continue
+			}
+			c := out[section]
+			if !c.read(k, v) {
+				problems = append(problems, fmt.Sprintf("[%s] has a key this module does not read; it reads username, password and domain", section))
+			}
+			out[section] = c
 			continue
 		}
-		k, v, ok := strings.Cut(line, "=")
-		if !ok || section == "" {
-			problems = append(problems, fmt.Sprintf("line %d is not key=value inside a [section]", i+1))
+		// `[` opens a section: the file form when nothing follows the `]` on the line, the one-line form when
+		// the keys do. The name is the first `]`'s, and a source's name holds no `]`, so nothing typed after it
+		// can open or close another section.
+		section = ""
+		name, rest, closed := strings.Cut(line[1:], "]")
+		name = strings.TrimSpace(name)
+		if !closed || !nameRe.MatchString(name) {
+			problems = append(problems, fmt.Sprintf("line %d opens no source: the name in brackets is lower-case letters, digits and dashes, as %s", i+1, SMBOneLineForm))
 			continue
 		}
-		c := out[section]
-		switch strings.ToLower(strings.TrimSpace(k)) {
-		case "username", "user":
-			c.Username = strings.TrimSpace(v)
-		case "password", "pass":
-			c.Password = strings.TrimRight(v, "\r")
-		case "domain":
-			c.Domain = strings.TrimSpace(v)
-		default:
-			problems = append(problems, fmt.Sprintf("[%s] has a key %q this module does not read", section, strings.TrimSpace(k)))
+		if _, dup := out[name]; dup {
+			problems = append(problems, fmt.Sprintf("the section [%s] is given twice; the first is kept", name))
+			continue
 		}
-		out[section] = c
+		if strings.TrimSpace(rest) == "" {
+			out[name] = SMBCredential{}
+			section = name
+			continue
+		}
+		// From here the line may be a password typed alone that happens to start like a name, so until it
+		// reads as a login nothing of it is said: not the name, not what follows it.
+		if rest[0] != ' ' && rest[0] != '\t' {
+			problems = append(problems, fmt.Sprintf("line %d: the name in brackets is not followed by a space; the one-line form is %s", i+1, SMBOneLineForm))
+			continue
+		}
+		c, ok := readOneLineLogin(rest)
+		if !ok {
+			problems = append(problems, fmt.Sprintf("line %d: what follows the name in brackets is not key=value pairs; the one-line form is %s, password last", i+1, SMBOneLineForm))
+			continue
+		}
+		if c.unknownKey {
+			problems = append(problems, fmt.Sprintf("[%s] has a key this module does not read; it reads username, password and domain", name))
+		}
+		out[name] = c.SMBCredential
 	}
+	// A login that cannot be used is not answered at all: apply says the source as not set up, rather than
+	// writing a credentials file with a hole in it.
 	for name, c := range out {
 		if c.Username == "" {
-			problems = append(problems, fmt.Sprintf("[%s] gives no username", name))
+			problems = append(problems, fmt.Sprintf("[%s] gives no username; a login is %s", name, SMBOneLineForm))
+			delete(out, name)
 		}
 		if strings.ContainsAny(c.Username+c.Password+c.Domain, "\n") {
 			problems = append(problems, fmt.Sprintf("[%s] holds a line break", name))
@@ -327,6 +371,61 @@ func ParseSMBCredentials(text string) (map[string]SMBCredential, []string) {
 	return out, problems
 }
 
+// read puts one key's value in the login and says whether the key is one this module reads. The key is
+// matched lower-cased and trimmed; a username or domain is trimmed, a password is kept as given.
+func (c *SMBCredential) read(k, v string) bool {
+	switch strings.ToLower(strings.TrimSpace(k)) {
+	case "username", "user":
+		c.Username = strings.TrimSpace(v)
+	case "password", "pass":
+		c.Password = v
+	case "domain":
+		c.Domain = strings.TrimSpace(v)
+	default:
+		return false
+	}
+	return true
+}
+
+// oneLineLogin is a login read from the one-line form, and whether a key in it is one this module does
+// not read (said by the caller; the key itself is never said).
+type oneLineLogin struct {
+	SMBCredential
+	unknownKey bool
+}
+
+// readOneLineLogin reads what follows `[name]` on a line: space-separated key=value pairs, where
+// `password=` takes everything after it to the end of the line and so is last. It is not ok when a
+// word is not key=value: that word is never said, as it may be the password typed without its key.
+func readOneLineLogin(rest string) (oneLineLogin, bool) {
+	var c oneLineLogin
+	for {
+		rest = strings.TrimLeft(rest, " \t")
+		if rest == "" {
+			return c, true
+		}
+		word := rest
+		if sp := strings.IndexAny(rest, " \t"); sp >= 0 {
+			word = rest[:sp]
+		}
+		k, v, ok := strings.Cut(word, "=")
+		key := strings.ToLower(strings.TrimSpace(k))
+		if !ok || key == "" {
+			return oneLineLogin{}, false
+		}
+		switch key {
+		case "password", "pass":
+			// To the end of the line: a password may hold spaces and `=`.
+			c.read(key, rest[len(k)+1:])
+			return c, true
+		}
+		if !c.read(key, v) {
+			c.unknownKey = true
+		}
+		rest = rest[len(word):]
+	}
+}
+
 // CredentialsFile is a source's credentials in mount.cifs's format.
 func (c SMBCredential) CredentialsFile() string {
 	s := "username=" + c.Username + "\npassword=" + c.Password + "\n"

modules/mounts/cmd/mounts/write_test.go

@@ -587,3 +587,88 @@ func (f *fakeSystem) setUnit(unit string, state map[string]string) {
 	defer f.mu.Unlock()
 	f.units[unit] = state
 }
+
+// The login as typed at the desk's one-line prompt (novox/hq ADR 0277) is the login the file form gives
+// (issue 379): the password is last and runs to the end of the line, with its spaces and its `=`.
+func TestAOneLineLoginIsTheLoginTheFileFormGives(t *testing.T) {
+	pw := "a b=c " + testPassword + "=="
+	file := "[recalbox]\nusername=root\npassword=" + pw + "\ndomain=WORKGROUP\n"
+	line := "[recalbox] username=root domain=WORKGROUP password=" + pw + "\n"
+	fromFile, fileProblems := ParseSMBCredentials(file)
+	fromLine, lineProblems := ParseSMBCredentials(line)
+	want := SMBCredential{Username: "root", Password: pw, Domain: "WORKGROUP"}
+	if len(fileProblems) != 0 || len(lineProblems) != 0 || fromFile["recalbox"] != want || fromLine["recalbox"] != want {
+		t.Fatalf("file %+v %q, line %+v %q", fromFile, fileProblems, fromLine, lineProblems)
+	}
+	// After `password=` it is kept as typed: a leading space is the password's, in both forms.
+	fromFile, _ = ParseSMBCredentials("[recalbox]\nusername=root\npassword= " + pw + "\n")
+	fromLine, _ = ParseSMBCredentials("[recalbox] username=root password= " + pw + "\n")
+	if fromFile["recalbox"].Password != " "+pw || fromLine["recalbox"] != fromFile["recalbox"] {
+		t.Errorf("a leading space: file %+v, line %+v", fromFile, fromLine)
+	}
+	fromLine, _ = ParseSMBCredentials(line)
+	if fromLine["recalbox"].CredentialsFile() != "username=root\npassword="+pw+"\ndomain=WORKGROUP\n" {
+		t.Errorf("credentials file %q", strings.ReplaceAll(fromLine["recalbox"].CredentialsFile(), pw, "<pw>"))
+	}
+	// Several sources are several lines, in either form, mixed; a `\r` from a terminal is not the password's.
+	mixed := "[recalbox] username=root password=" + pw + "\r\n[nas]\nusername=u\npassword=p\n[usb] user=x pass=y\n"
+	got, problems := ParseSMBCredentials(mixed)
+	if len(problems) != 0 || len(got) != 3 || got["recalbox"].Password != pw || got["nas"].Password != "p" || got["usb"] != (SMBCredential{Username: "x", Password: "y"}) {
+		t.Errorf("mixed %v %q", len(got), problems)
+	}
+	// A one-line login closes the file-form section open before it: a key on the next line lands in no
+	// login, neither the one given whole nor the one that was open.
+	got, problems = ParseSMBCredentials("[nas]\nusername=u\npassword=p\n[recalbox] username=root password=x\nusername=other\n")
+	if got["recalbox"].Username != "root" || got["nas"].Username != "u" || len(problems) != 1 || !strings.Contains(problems[0], "outside a [section]") {
+		t.Errorf("a stray line after a one-line login: %+v %q", got, problems)
+	}
+}
+
+// A line the module cannot read is refused with the form it expected, and nothing typed is said back:
+// not the password, not a word that may be the password typed without its key.
+func TestAMalformedLoginIsRefusedWithTheExpectedFormAndSaysNothingTyped(t *testing.T) {
+	cases := map[string]string{
+		"the password alone, as the prompt invites":  testPassword + "\n",
+		"a line without a section":                   "username=root password=" + testPassword + "\n",
+		"a word that is not key=value":               "[recalbox] root " + testPassword + "\n",
+		"a value without a key":                      "[recalbox] =" + testPassword + "\n",
+		"no space after the name":                    "[recalbox]username=root password=" + testPassword + "\n",
+		"a name that is not a source's":              "[Recal box] username=root password=" + testPassword + "\n",
+		"a bracket never closed":                     "[recalbox username=root password=" + testPassword + "\n",
+		"a password alone that looks like a name":    "[abc-1]" + testPassword + "\n",
+		"no username, line form":                     "[recalbox] password=" + testPassword + "\n",
+		"no username, file form":                     "[recalbox]\npassword=" + testPassword + "\n",
+		"a key this module does not read, file form": "[recalbox]\nusername=root\n" + testPassword + "=1\npassword=x\n",
+		"a key this module does not read, line form": "[recalbox] username=root " + testPassword + "=1 password=x\n",
+		"a section given twice, the second one-line": "[recalbox]\nusername=root\npassword=x\n[recalbox] username=u password=" + testPassword + "\n",
+	}
+	for what, text := range cases {
+		got, problems := ParseSMBCredentials(text)
+		said := strings.Join(problems, " ")
+		if len(problems) == 0 {
+			t.Errorf("%s: not refused: %+v", what, got)
+			continue
+		}
+		if strings.Contains(said, testPassword) || strings.Contains(said, "root") || strings.Contains(said, "abc-1") {
+			t.Errorf("%s: a refusal says what was typed: %q", what, said)
+		}
+		if strings.Contains(what, "does not read") {
+			if !strings.Contains(said, "it reads username, password and domain") || got["recalbox"].Password != "x" {
+				t.Errorf("%s: %q %+v", what, said, got)
+			}
+			continue
+		}
+		if strings.Contains(what, "twice") {
+			if got["recalbox"].Username != "root" || got["recalbox"].Password != "x" {
+				t.Errorf("%s: the first is not kept: %+v", what, got)
+			}
+			continue
+		}
+		if !strings.Contains(said, SMBOneLineForm) {
+			t.Errorf("%s: the refusal does not say the expected form: %q", what, said)
+		}
+		if _, kept := got["recalbox"]; kept {
+			t.Errorf("%s: a refused login is answered: %+v", what, got)
+		}
+	}
+}