internal/apply/process.go

@@ -203,7 +203,7 @@ func unitFor(r *declaration.Process) string {
 		fmt.Fprintf(&b, "EnvironmentFile=%s\n", file)
 	}
 	for _, key := range sortedKeys(r.Env) {
-		fmt.Fprintf(&b, "Environment=%s=%s\n", key, r.Env[key])
+		fmt.Fprintf(&b, "Environment=%s\n", unitValue(key, r.Env[key]))
 	}
 	if r.User != "" {
 		fmt.Fprintf(&b, "User=%s\n", r.User)
@@ -264,3 +264,28 @@ func calendarFor(cron string) string {
 	}
 	return fmt.Sprintf("%s*-%s-%s %s:%s:00", day, month, dom, hour, minute)
 }
+
+// unitValue renders one environment assignment so a unit file means what the declaration said.
+//
+// **Three things a unit file does to a value that nothing else does**, and a module's environment
+// routinely contains all three — a generated password is arbitrary bytes.
+//
+//   - `%` begins a specifier. `%H` is the hostname, `%i` the instance. A password containing one
+//     is silently replaced by something else, and the failure is an authentication error nobody
+//     can explain by looking at the declaration.
+//   - whitespace separates assignments. `Environment=K=a b` sets K to "a" and then tries to read
+//     "b" as another assignment.
+//   - a newline ends the line. What follows it is read as a unit DIRECTIVE, so a value carrying
+//     one could write ExecStart= and have the machine run something nobody declared.
+//
+// Quoted, with quotes and backslashes escaped and percent doubled. A container needs none of this
+// because `--env` is passed through literally, which is why this had to be found here rather than
+// noticed in both.
+func unitValue(key, value string) string {
+	escaped := strings.NewReplacer(
+		`\`, `\\`,
+		`"`, `\"`,
+		"%", "%%",
+	).Replace(value)
+	return `"` + key + "=" + escaped + `"`
+}

internal/apply/process_test.go

@@ -130,3 +130,36 @@ func TestAProcessThatStaysUpIsStillRestartedWhenItExits(t *testing.T) {
 		t.Fatalf("a process that should stay up is declared a step:\n%s", unit)
 	}
 }
+
+// **A unit file reinterprets a value in three ways nothing else does**, and a module's environment
+// routinely contains all three — a generated password is arbitrary bytes.
+func TestAnEnvironmentValueMeansWhatTheDeclarationSaid(t *testing.T) {
+	// A percent begins a specifier: %H is the hostname. A password containing one would be
+	// silently replaced, failing as an authentication error nobody can explain from the
+	// declaration.
+	percent := aProcess()
+	percent.Env = map[string]string{"PASSWORD": "a%Hb"}
+	if !strings.Contains(unitFor(percent), "%%H") {
+		t.Fatalf("a percent was left as a systemd specifier:\n%s", unitFor(percent))
+	}
+
+	// Whitespace separates assignments: unquoted, K=a b sets K to "a" and reads "b" as another.
+	spaced := aProcess()
+	spaced.Env = map[string]string{"GREETING": "hello there"}
+	if !strings.Contains(unitFor(spaced), `"GREETING=hello there"`) {
+		t.Fatalf("a value with a space was not quoted:\n%s", unitFor(spaced))
+	}
+
+	// A quote would end the quoting early, and what follows would be read as unit syntax.
+	quoted := aProcess()
+	quoted.Env = map[string]string{"TOKEN": `a"b`}
+	line := ""
+	for _, l := range strings.Split(unitFor(quoted), "\n") {
+		if strings.HasPrefix(l, "Environment=") {
+			line = l
+		}
+	}
+	if !strings.Contains(line, `\"`) {
+		t.Fatalf("a quote was not escaped, so the value ends early: %s", line)
+	}
+}

internal/declaration/declaration.go

@@ -502,6 +502,24 @@ func (d *Process) validate(where string, _ bool) []string {
 			break
 		}
 	}
+	// **A newline cannot be represented in a unit's environment, so it is refused rather than
+	// mangled.** Everything else a unit file reinterprets — a percent specifier, whitespace
+	// splitting assignments, a quote ending one early — can be escaped. A newline cannot: it ends
+	// the line, and what follows is read as a unit DIRECTIVE. A value carrying one could write
+	// ExecStart= and have the machine run something nobody declared.
+	//
+	// Refused here, near whoever wrote it, rather than at the far end of a declaration.
+	for key, value := range d.Env {
+		if strings.ContainsAny(value, "\n\r") {
+			problems = append(problems, fmt.Sprintf(
+				"%s: the value of %s contains a line break, which cannot be written into a unit's "+
+					"environment — what followed it would be read as a unit directive", where, key))
+		}
+		if key == "" {
+			problems = append(problems, where+": an environment value with no name")
+		}
+	}
+
 	if d.Schedule != "" {
 		if d.RunOnce {
 			problems = append(problems, where+

internal/declaration/process_test.go

@@ -116,3 +116,45 @@ func TestEachModeOnItsOwnIsAccepted(t *testing.T) {
 		t.Fatalf("a scheduled process was refused: %v", problems)
 	}
 }
+
+// **The one that cannot be escaped, only refused.**
+//
+// Everything else a unit file reinterprets can be escaped: a percent specifier doubled, whitespace
+// quoted, a quote backslashed. A newline cannot — it ends the line, and what follows is read as a
+// unit DIRECTIVE. A value carrying one could write ExecStart= and have the machine run something
+// nobody declared.
+//
+// So it is refused here, near whoever wrote it, rather than rendered into a unit at the far end of
+// a declaration.
+func TestAnEnvironmentValueCannotCarryALineBreak(t *testing.T) {
+	for _, bad := range []string{
+		"safe\nExecStart=/usr/bin/whatever",
+		"carriage\rreturn",
+	} {
+		p := aProcess()
+		p.Env = map[string]string{"X": bad}
+		problems := p.validate("a process", false)
+		if len(problems) == 0 {
+			t.Fatalf("a value containing %q was accepted", bad)
+		}
+		var said bool
+		for _, problem := range problems {
+			if strings.Contains(problem, "line break") {
+				said = true
+			}
+		}
+		if !said {
+			t.Fatalf("refused for some other reason, which would stop being true: %v", problems)
+		}
+	}
+}
+
+// And ordinary awkward values are accepted, because escaping is what handles those — refusing them
+// too would make a module unable to hold a generated password.
+func TestOrdinaryAwkwardValuesAreAccepted(t *testing.T) {
+	p := aProcess()
+	p.Env = map[string]string{"PASSWORD": `a%H b"c\d`}
+	if problems := p.validate("a process", false); len(problems) != 0 {
+		t.Fatalf("a password containing the characters passwords contain was refused: %v", problems)
+	}
+}