Merge pull request 'Read the session record strictly, and make its tests hold (hq ADR 0272)' (#72) from fix/272-the-session-record-read-strictly into main

internal/meshcli/meshcli.go

@@ -116,9 +116,17 @@ func userSessionIn(cgroup string, uid uint32) string {
 	if err != nil {
 		return ""
 	}
+	// **The record is systemd's private format**, not an interface it promises: `KEY=value` lines logind writes for
+	// itself. Only UID, CLASS and SCOPE are read, each at its first occurrence — logind writes each once, and a later
+	// line saying otherwise is not the record's word. A systemd that changes the format, renames a key or stops
+	// writing the file fails closed here: no session is read, so nobody is the terminal until this reads it again.
 	record := map[string]string{}
 	for _, line := range strings.Split(string(raw), "\n") {
-		if k, v, ok := strings.Cut(line, "="); ok {
+		k, v, ok := strings.Cut(line, "=")
+		if !ok {
+			continue
+		}
+		if _, said := record[k]; !said {
 			record[k] = v
 		}
 	}

internal/meshcli/meshcli_test.go

@@ -10,6 +10,7 @@ import (
 	"path/filepath"
 	"sort"
 	"strings"
+	"sync"
 	"testing"
 	"time"
 
@@ -233,6 +234,9 @@ func TestTheLoginSessionAskingIsReadFromTheKernel(t *testing.T) {
 		{"0::/system.slice/node-tools.service\n", ""},
 		{"0::/user.slice/user-1001.slice/session-3.scope\n", ""}, // another account's session
 		{"12:pids:/user.slice\n0::/user.slice/user-1000.slice/session-9.scope/sub\n", ""},
+		// Below a session scope, in a scope of its own: only the check that the scope is the account slice's direct
+		// child refuses it.
+		{"0::/user.slice/user-1000.slice/session-9.scope/x.scope\n", ""},
 		{"", ""},
 	} {
 		if got := loginSessionIn(c.cgroup, 1000); got != c.session {
@@ -269,8 +273,14 @@ func TestALoginSessionIsAUsersByItsOwnRecord(t *testing.T) {
 	}
 	write("9", "UID=1000\nUSER=op\nCLASS=user\nSCOPE=session-9.scope\nACTIVE=1\n")
 	write("3", "UID=1000\nCLASS=greeter\nSCOPE=session-3.scope\n")
-	write("4", "UID=1001\nCLASS=user\nSCOPE=session-4.scope\n")
 	write("5", "UID=1000\nCLASS=user\nSCOPE=session-6.scope\n")
+	// The account the record names is not the slice's: only the record's UID check refuses it.
+	write("8", "UID=1001\nCLASS=user\nSCOPE=session-8.scope\n")
+	// A name with a dot, held as a record that would pass: only the check of the id's characters refuses it.
+	write("a.b", "UID=1000\nCLASS=user\nSCOPE=session-a.b.scope\n")
+	// A key said twice: the first is the record's word (systemd writes each once); a later one is not read.
+	write("11", "UID=1001\nCLASS=user\nSCOPE=session-11.scope\nUID=1000\n")
+	write("12", "UID=1000\nCLASS=user\nSCOPE=session-12.scope\nCLASS=greeter\nUID=1001\n")
 	for _, c := range []struct {
 		cgroup  string
 		uid     uint32
@@ -278,12 +288,15 @@ func TestALoginSessionIsAUsersByItsOwnRecord(t *testing.T) {
 	}{
 		{"0::/user.slice/user-1000.slice/session-9.scope\n", 1000, "session-9.scope"},
 		{"0::/user.slice/user-1000.slice/x.scope\n", 1000, ""},
-		{"0::/user.slice/user-1000.slice/session-9.scope\n", 1001, ""}, // a different uid than the scope's slice
+		{"0::/user.slice/user-1000.slice/session-9.scope\n", 1001, ""}, // asked as another uid than the slice's
-		{"0::/user.slice/user-1001.slice/session-4.scope\n", 1000, ""}, // another account's session
+		{"0::/user.slice/user-1000.slice/session-8.scope\n", 1000, ""}, // the record names uid 1001
 		{"0::/user.slice/user-1000.slice/session-3.scope\n", 1000, ""}, // a greeter's
 		{"0::/user.slice/user-1000.slice/session-5.scope\n", 1000, ""}, // a record naming another scope
 		{"0::/user.slice/user-1000.slice/session-7.scope\n", 1000, ""}, // no record at all
-		{"0::/user.slice/user-1000.slice/session-../9.scope\n", 1000, ""},
+		{"0::/user.slice/user-1000.slice/session-a.b.scope\n", 1000, ""},
+		{"0::/user.slice/user-1000.slice/session-...scope\n", 1000, ""},
+		{"0::/user.slice/user-1000.slice/session-11.scope\n", 1000, ""},                 // first UID is 1001
+		{"0::/user.slice/user-1000.slice/session-12.scope\n", 1000, "session-12.scope"}, // first UID and CLASS stand
 	} {
 		if got := userSessionIn(c.cgroup, c.uid); got != c.session {
 			t.Errorf("%q as uid %d read as %q, want %q", c.cgroup, c.uid, got, c.session)
@@ -291,24 +304,45 @@ func TestALoginSessionIsAUsersByItsOwnRecord(t *testing.T) {
 	}
 }
 
-// The session is read at accept, with the namespaces, and before the line: a peer that execs or exits while its
+// **The session is read at accept, with the namespaces, before the line** (review of ADR 0272): the peer is judged as
-// line is read is judged as it was when it connected.
+// it was when it connected. The client sends its line only once the session has been read; were the session read
+// after the line — in the forward, say — the line would never be sent and the test fails.
 func TestTheSessionIsReadBeforeTheLine(t *testing.T) {
+	var mu sync.Mutex
 	var order []string
+	note := func(s string) { mu.Lock(); order = append(order, s); mu.Unlock() }
+	read := make(chan struct{}, 1)
 	beforeNS, beforeSession := sameNamespaces, sessionOf
-	sameNamespaces = func(int32) error { order = append(order, "namespaces"); return nil }
+	sameNamespaces = func(int32) error { note("namespaces"); return nil }
-	sessionOf = func(int32, uint32) string { order = append(order, "session"); return "session-1.scope" }
+	sessionOf = func(int32, uint32) string { note("session"); read <- struct{}{}; return "session-1.scope" }
 	t.Cleanup(func() { sameNamespaces, sessionOf = beforeNS, beforeSession })
 	var got Asked
 	socket := serving(t, func(_ context.Context, asked Asked) cli.Answer {
-		order = append(order, "forward")
+		note("forward")
 		got = asked
 		return cli.Answer{}
 	})
-	if _, err := cli.Ask(context.Background(), socket, []string{"status"}); err != nil {
+	conn, err := net.Dial("unix", socket)
+	if err != nil {
 		t.Fatal(err)
 	}
-	if strings.Join(order, ",") != "namespaces,session,forward" || got.Session != "session-1.scope" {
+	defer conn.Close()
+	select {
+	case <-read:
+	case <-time.After(3 * time.Second):
+		t.Fatal("the session was not read before the line was sent")
+	}
+	note("line sent")
+	if err := cli.WriteRequest(conn, cli.Request{Line: []string{"status"}}); err != nil {
+		t.Fatal(err)
+	}
+	_ = conn.SetDeadline(time.Now().Add(5 * time.Second))
+	if _, err := cli.ReadAnswer(conn); err != nil {
+		t.Fatal(err)
+	}
+	mu.Lock()
+	defer mu.Unlock()
+	if strings.Join(order, ",") != "namespaces,session,line sent,forward" || got.Session != "session-1.scope" {
 		t.Fatalf("read in the order %v, sent %q", order, got.Session)
 	}
 }