From b1e7b6d26ebd5fd4de7806c5268f27758746b09c Mon Sep 17 00:00:00 2001 From: jochen Date: Fri, 9 Oct 2026 14:05:53 +0200 Subject: [PATCH] Read the session record strictly, and make its tests hold (hq ADR 0272) - UID, CLASS and SCOPE are read at their first occurrence: logind writes each once, and a later line saying otherwise is not the record's word. - A comment says /run/systemd/sessions is systemd's private format, and that a format change fails closed: no session, so nobody is the terminal. - Tests that hold, each proved by the mutation it guards failing: - a record naming UID=1001 for session-8.scope under user-1000.slice, asked as uid 1000 (the record's UID check alone); - session-a.b.scope with a passing record (the id's characters alone); - session-9.scope/x.scope (the scope being the slice's direct child); - a key said twice, either way round (first occurrence); - the client sends its line only after the session was read, so reading the session in the forward fails the test. --- internal/meshcli/meshcli.go | 10 +++++- internal/meshcli/meshcli_test.go | 56 +++++++++++++++++++++++++------- 2 files changed, 54 insertions(+), 12 deletions(-) diff --git a/internal/meshcli/meshcli.go b/internal/meshcli/meshcli.go index dc64264..3113b81 100644 --- a/internal/meshcli/meshcli.go +++ b/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 } } diff --git a/internal/meshcli/meshcli_test.go b/internal/meshcli/meshcli_test.go index dd0f84f..b764c83 100644 --- a/internal/meshcli/meshcli_test.go +++ b/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-1001.slice/session-4.scope\n", 1000, ""}, // another account's session + {"0::/user.slice/user-1000.slice/session-9.scope\n", 1001, ""}, // asked as another uid than the slice's + {"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 -// line is read is judged as it was when it connected. +// **The session is read at accept, with the namespaces, before the line** (review of ADR 0272): the peer is judged as +// 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 } - sessionOf = func(int32, uint32) string { order = append(order, "session"); return "session-1.scope" } + sameNamespaces = func(int32) error { note("namespaces"); return nil } + 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) } }