From a76263a54d08922528f7c3783225fb05c5d0baa2 Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Wed, 9 Sep 2026 17:10:31 +0100 Subject: [PATCH 01/16] gateway: add a hidden gateway command with status and stop A launch reuses whatever gateway it finds running and never asks which image it came from, so an edit to the gateway block reaches nothing until the running one is gone. There was no way to make that happen, and the way it was being done was to read the pid out of sandbox-gateway.json, kill it and remove the file by hand. stop is that by name. It signals the pid in the record, which is the sandbox's own init and not the bwrap that started it: killing the outer bwrap does not signal the sandbox nested below it, while the init is pid 1 of its own pid namespace and a SIGKILL from an ancestor namespace takes the whole namespace with it. That is the pid running.stop already names when it takes down a gateway that failed to come up. It reads the record through sandbox.Alive rather than looking at the pid, so a record left behind by a gateway that crashed reads as no gateway and nothing signals a number the kernel has since given to something else. It takes the same lock startOnce takes, because a launch between its check and its record would otherwise be left naming a process this has already killed. The session's namespace holder stays up. Only the gateway is reused across launches, so the holder is not what has to go, and taking it down would make the next launch rebuild a user namespace that was never the problem. The next launch starts a fresh gateway inside the same session. Being asked to stop a gateway that is not running is not a failure. It is the state the caller wanted, so it is reported and the exit status stays zero. A record left behind is removed on the way, because reap would have removed it had the launch that started the gateway still been there. status reports only what qubesome already holds: the configured image, the policy path resolved against the directory the config was read from, the subnet, the liveness of the holder and the gateway, readiness through the Ready call that already exists, and the addresses handed out. No new RPC, no proto change, and nothing that needs a newer gateway than the one that is running. Its readiness question is bounded at five seconds and not by the client's ten minutes. That deadline is sized for a launch, where Ready may be waiting on an image still being unpacked and waiting is the whole job. A status is there to report, so a gateway that does not answer becomes a line rather than a command that does not return. doctor bounds the same call for the same reason. Neither subcommand takes a profile, since there is one gateway per session. The config is still what names the image, the policy and the subnet, and it is reached the way doctor reaches one, through a running profile or the user-level file. A status that finds neither says so rather than printing three blank lines, and Inspect is handed the whole config rather than its gateway block so that a config which did not load and a config with no gateway block stay different answers. Reporting the second for the first is the bug doctor's session checks were fixed for. It is hidden, but not for the reason supervise and session-hold are. Nothing here runs inside a sandbox. It is hidden because qubesome manages the gateway itself, so the ordinary way to get one is to run a workload and there is nothing for a user to do here. Signed-off-by: Paulo Gomes --- cmd/cli/gateway.go | 91 +++++++++ cmd/cli/root.go | 1 + internal/gateway/status.go | 326 ++++++++++++++++++++++++++++++++ internal/gateway/status_test.go | 268 ++++++++++++++++++++++++++ internal/gateway/stop.go | 84 ++++++++ internal/gateway/stop_test.go | 141 ++++++++++++++ 6 files changed, 911 insertions(+) create mode 100644 cmd/cli/gateway.go create mode 100644 internal/gateway/status.go create mode 100644 internal/gateway/status_test.go create mode 100644 internal/gateway/stop.go create mode 100644 internal/gateway/stop_test.go diff --git a/cmd/cli/gateway.go b/cmd/cli/gateway.go new file mode 100644 index 0000000..06ac433 --- /dev/null +++ b/cmd/cli/gateway.go @@ -0,0 +1,91 @@ +package cli + +import ( + "context" + "fmt" + "os" + + "github.com/qubesome/cli/internal/gateway" + "github.com/qubesome/cli/internal/session" + "github.com/urfave/cli/v3" +) + +// gatewayCommand reports on the session's gateway and takes it down. +// +// It is hidden, but not for the reason supervise and session-hold are. +// Nothing here runs inside a sandbox. It is hidden because qubesome manages +// the gateway itself: a launch starts one, reuses the one it finds and asks +// it to re-read its policy, so the ordinary way to get a gateway is to run a +// workload and there is nothing for a user to do here. What is left is an +// operator's business, and the one thing a launch will not do is replace a +// running gateway with one from a different image. +// +// Neither subcommand takes a profile. There is one gateway per session and +// not one per profile, because the policy it applies is keyed by workload +// across every profile. +func gatewayCommand() *cli.Command { + cmd := &cli.Command{ + Name: "gateway", + Hidden: true, + Usage: "inspects and stops the session gateway", + Description: `qubesome starts, reuses and reloads the session gateway on its own, +so this is for the cases where that is not enough: + +qubesome gateway status - Report what qubesome knows about the session's gateway +qubesome gateway stop - Stop the session's gateway, leaving the session itself up + +A running gateway is reused whatever image it came from, so a change to +the gateway block of the config reaches nothing until it is stopped. The +next launch then starts a fresh one inside the same session. +`, + Commands: []*cli.Command{ + gatewayStatusCommand(), + gatewayStopCommand(), + }, + } + return cmd +} + +func gatewayStatusCommand() *cli.Command { + return &cli.Command{ + Name: "status", + Usage: "reports what qubesome knows about the session gateway", + Action: func(ctx context.Context, cmd *cli.Command) error { + g := gateway.Current() + + // The same route doctor takes to a config, and it may find + // none. The gateway is per session, so there is no profile to + // name here, and without a running profile or a user-level + // file there is nothing that says which image, policy or + // subnet a gateway was meant to have. Inspect reports that it + // could not tell rather than leaving the lines blank. + status := g.Inspect(session.Current(), profileConfigOrDefault(""), g.StatusReady) + + return status.Write(os.Stdout) + }, + } +} + +func gatewayStopCommand() *cli.Command { + return &cli.Command{ + Name: "stop", + Usage: "stops the session gateway, leaving the session itself up", + Action: func(ctx context.Context, cmd *cli.Command) error { + pid, err := gateway.Current().Stop() + if err != nil { + return err + } + + if pid == 0 { + fmt.Fprintln(os.Stdout, "no gateway is running for this session") + return nil + } + + fmt.Fprintf(os.Stdout, + "stopped the session gateway, pid %d. The session is still up, "+ + "so the next launch starts a fresh gateway inside it.\n", pid) + + return nil + }, + } +} diff --git a/cmd/cli/root.go b/cmd/cli/root.go index 66f728e..fcb9af6 100644 --- a/cmd/cli/root.go +++ b/cmd/cli/root.go @@ -48,6 +48,7 @@ func RootCommand() *cli.Command { gpuCommand(), superviseCommand(), sessionHoldCommand(), + gatewayCommand(), vmInitCommand(), consoleCommand(), }, diff --git a/internal/gateway/status.go b/internal/gateway/status.go new file mode 100644 index 0000000..e8da0f9 --- /dev/null +++ b/internal/gateway/status.go @@ -0,0 +1,326 @@ +package gateway + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "io" + "os" + "time" + + "github.com/qubesome/cli/internal/sandbox" + "github.com/qubesome/cli/internal/session" + "github.com/qubesome/cli/internal/types" +) + +// statusReadyTimeout bounds the readiness question a status asks. +// +// It is not the client's own ten minutes. That deadline is sized for a +// launch, where Ready may be waiting on an image that is still being +// unpacked and where waiting is the whole job. A status is there to report, +// so a gateway that does not answer in a few seconds has to become a line in +// the report rather than a command that does not return. doctor bounds the +// same call for the same reason. +const statusReadyTimeout = 5 * time.Second + +// Status is what qubesome knows about the session's gateway. +// +// Every field comes from something qubesome already holds: the config that +// names a gateway, the records a launch writes, and the readiness call the +// gateway has always served. Nothing here asks the gateway anything new, so +// a status needs no newer gateway than the one that is running. +type Status struct { + // ConfigProblem says why Image, Policy and Subnet are empty. It is + // empty when they are not. A status that cannot find a config says so, + // rather than printing three blank lines that read as a gateway + // configured with nothing. + ConfigProblem string + + Image string + Policy string + Subnet string + + // PolicyProblem says why the policy file path could not be resolved + // against the directory the config was read from. + PolicyProblem string + + // HolderPath and StatePath are the records that were read. They are + // reported because they are the files an operator would otherwise have + // to go looking for. + HolderPath string + StatePath string + + HolderRunning bool + HolderPID int + + Running bool + PID int + + // ReadyErr is why a running gateway did not report itself ready. It is + // empty both when it did and when there was no gateway to ask. + ReadyErr string + + // GatewayAddr is the address the gateway holds on every veth. Allocated + // is how many workload addresses have been handed out since it started, + // and LastAddr is the highest of them. + GatewayAddr string + Allocated uint64 + LastAddr string + + // AddrProblem says why the addresses are not in the report. + AddrProblem string +} + +// Inspect gathers what is known about the session's gateway. +// +// cfg is the whole qubesome config rather than its gateway block, because a +// config that did not load and a config with no gateway block are different +// answers and a nil block cannot tell them apart. A bare status reaches a +// config only through a running profile or the user-level file, so it may +// well have neither, and reporting "no gateway is configured" for "no config +// was read" is how doctor once said there was no gateway on a host whose +// config configures one. +// +// ready is called only when a gateway is recorded as running. A readiness +// failure whose whole cause is that nothing is listening repeats the line +// above it. +func (g Gateway) Inspect(s session.Session, cfg *types.Config, ready func() error) Status { + st := Status{ + HolderPath: s.StatePath, + StatePath: g.StatePath, + } + + // sandbox.Alive and not a look at the pid. It compares the recorded + // start time too, so a record left behind by a gateway that crashed + // reads as not running rather than as whatever process has since been + // given its number. + st.HolderRunning = sandbox.Alive(s.StatePath) + if st.HolderRunning { + if rec, err := sandbox.ReadState(s.StatePath); err == nil { + st.HolderPID = rec.PID + } + } + + st.Running = sandbox.Alive(g.StatePath) + if st.Running { + if rec, err := sandbox.ReadState(g.StatePath); err == nil { + st.PID = rec.PID + } + } + + g.inspectConfig(&st, cfg) + + if st.Running && ready != nil { + if err := ready(); err != nil { + st.ReadyErr = err.Error() + } + } + + return st +} + +// inspectConfig fills in what the config says a gateway should be, and the +// addresses handed out of the subnet it names. +func (g Gateway) inspectConfig(st *Status, cfg *types.Config) { + if cfg == nil { + st.ConfigProblem = "no qubesome config was loaded, so the image, policy and subnet it names are unknown" + return + } + + if cfg.Gateway == nil { + st.ConfigProblem = "no gateway is configured, so workloads run with no egress" + return + } + + st.Image = cfg.Gateway.Image + st.Subnet = cfg.Gateway.Subnet + + policy, err := cfg.Gateway.ConfigPath(cfg.RootDir) + if err != nil { + st.PolicyProblem = err.Error() + } else { + st.Policy = policy + } + + g.inspectAddrs(st, cfg.Gateway) +} + +// inspectAddrs reports the gateway's own address and how much of the subnet +// has been handed out. +// +// The count is read straight from the record rather than through readAlloc, +// which refuses a record naming a different subnet from the one asked for. +// That refusal is right for a launch, which must not hand out an address the +// running gateway knows nothing about. A status has to be able to report the +// mismatch instead of failing on it, since a config edited under a running +// gateway is one of the things it exists to show. +func (g Gateway) inspectAddrs(st *Status, cfg *types.GatewayConfig) { + subnet, err := cfg.SubnetPrefix() + if err != nil { + st.AddrProblem = err.Error() + return + } + + addr, err := GatewayAddr(subnet) + if err != nil { + st.AddrProblem = err.Error() + return + } + st.GatewayAddr = addr.String() + + a, err := readAllocFile(g.AllocPath) + if err != nil { + st.AddrProblem = err.Error() + return + } + + if a.Subnet != "" && a.Subnet != subnet.String() { + st.AddrProblem = fmt.Sprintf( + "the running gateway hands addresses out of %s and the config now asks for %s", + a.Subnet, subnet) + return + } + + st.Allocated = a.Allocated + if a.Allocated == 0 { + return + } + + // The first workload takes the address above the gateway's, and the + // count only ever goes up, so the highest handed out is the one that + // many places past it. + last, err := addrAt(subnet, a.Allocated+1) + if err != nil { + st.AddrProblem = err.Error() + return + } + st.LastAddr = last.String() +} + +// readAllocFile returns the address record, or an empty one when a gateway +// has handed nothing out yet. +func readAllocFile(path string) (allocation, error) { + data, err := os.ReadFile(path) + if errors.Is(err, os.ErrNotExist) { + return allocation{}, nil + } + if err != nil { + return allocation{}, fmt.Errorf("failed to read the gateway addresses %q: %w", path, err) + } + + var a allocation + if err := json.Unmarshal(data, &a); err != nil { + return allocation{}, fmt.Errorf("failed to parse the gateway addresses %q: %w", path, err) + } + + return a, nil +} + +// StatusReady asks the running gateway whether its resolver, proxy and +// netfilter ruleset are up, under a deadline a status command can wait out. +func (g Gateway) StatusReady() error { + ctx, cancel := context.WithTimeout(context.Background(), statusReadyTimeout) + defer cancel() + + c, err := g.Client() + if err != nil { + return err + } + + return c.Ready(ctx) +} + +// statusLine is one labelled fact. +type statusLine struct { + label string + text string +} + +// Write renders the status, one labelled line per fact. +func (s Status) Write(w io.Writer) error { + for _, l := range s.lines() { + if _, err := fmt.Fprintf(w, "%-10s %s\n", l.label, l.text); err != nil { + return err + } + } + + return nil +} + +// lines is the report in the order it is read in: what a gateway was asked +// to be, then whether there is one, then what it is doing. +func (s Status) lines() []statusLine { + out := make([]statusLine, 0, 7) + + if s.ConfigProblem != "" { + out = append(out, statusLine{"config", s.ConfigProblem}) + } else { + policy := s.Policy + if s.PolicyProblem != "" { + policy = "cannot be resolved: " + s.PolicyProblem + } + + out = append(out, + statusLine{"image", s.Image}, + statusLine{"policy", policy}, + statusLine{"subnet", s.Subnet}, + ) + } + + out = append(out, + statusLine{"holder", alive(s.HolderRunning, s.HolderPID, s.HolderPath)}, + statusLine{"gateway", alive(s.Running, s.PID, s.StatePath)}, + ) + + if s.Running { + out = append(out, statusLine{"readiness", s.readiness()}) + } + + if line, ok := s.addresses(); ok { + out = append(out, statusLine{"addresses", line}) + } + + return out +} + +// alive words the liveness of one recorded process. The record is named in +// both cases, because it is the file that answers the question and the one +// an operator reaches for next. +func alive(running bool, pid int, path string) string { + if !running { + return fmt.Sprintf("not running (no live record in %s)", path) + } + + return fmt.Sprintf("running, pid %d (%s)", pid, path) +} + +func (s Status) readiness() string { + if s.ReadyErr != "" { + return "not ready: " + s.ReadyErr + } + + return "the resolver, proxy and ruleset are up" +} + +// addresses words what the subnet has been used for, and reports whether +// there is anything to say. There is nothing without a subnet to say it of. +func (s Status) addresses() (string, bool) { + if s.AddrProblem != "" { + return s.AddrProblem, true + } + + if s.GatewayAddr == "" { + return "", false + } + + // The count belongs to the gateway that is running. A record left by an + // earlier one is removed by the next launch, so reporting it against no + // gateway would name addresses nothing holds. + if !s.Running || s.Allocated == 0 { + return fmt.Sprintf("%s is the gateway's own", s.GatewayAddr), true + } + + return fmt.Sprintf("%s is the gateway's own, %d handed out up to %s", + s.GatewayAddr, s.Allocated, s.LastAddr), true +} diff --git a/internal/gateway/status_test.go b/internal/gateway/status_test.go new file mode 100644 index 0000000..164b41b --- /dev/null +++ b/internal/gateway/status_test.go @@ -0,0 +1,268 @@ +// A real gateway cannot be started here, for the reasons run_test.go gives, +// so what a status says about one that is genuinely up is not covered. What +// is covered is everything the report is built from: the config it reads, +// the records it reads them beside, a record left behind by a gateway that +// crashed, the addresses handed out of the subnet, and when the gateway is +// asked whether it is ready at all. +package gateway + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/qubesome/cli/internal/sandbox" + "github.com/qubesome/cli/internal/session" + "github.com/qubesome/cli/internal/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestStatusWithoutAConfig(t *testing.T) { + t.Parallel() + + g := newSessionGateway(t) + + st := g.Inspect(newTestSession(t), nil, failingReady(t)) + + assert.Contains(t, st.ConfigProblem, "no qubesome config was loaded") + assert.Empty(t, st.Image) + assert.Empty(t, st.Subnet) + + out := render(t, st) + assert.Contains(t, out, "config no qubesome config was loaded") + assert.NotContains(t, out, "image ") + assert.NotContains(t, out, "subnet ") + assert.NotContains(t, out, "addresses ") +} + +// A config with no gateway block is a supported configuration and not a +// config that failed to load. Saying the first for the second is the bug +// doctor's session checks were fixed for. +func TestStatusWithoutAGatewayBlock(t *testing.T) { + t.Parallel() + + g := newSessionGateway(t) + + st := g.Inspect(newTestSession(t), &types.Config{}, failingReady(t)) + + assert.Contains(t, st.ConfigProblem, "no gateway is configured") + assert.NotContains(t, st.ConfigProblem, "no qubesome config was loaded") +} + +func TestStatusReportsTheConfiguredGateway(t *testing.T) { + t.Parallel() + + g := newSessionGateway(t) + + st := g.Inspect(newTestSession(t), testConfig(unusableConfig()), failingReady(t)) + + require.Empty(t, st.ConfigProblem) + assert.Equal(t, "ghcr.io/qubesome/gateway:latest", st.Image) + assert.Equal(t, testSubnet, st.Subnet) + + // A path in a qubesome config is rooted at the config tree, so the + // leading separator names that tree and not the root of the disk. + assert.Equal(t, "/config/root/gateway.yml", st.Policy) + assert.Empty(t, st.PolicyProblem) + + out := render(t, st) + assert.Contains(t, out, "image ghcr.io/qubesome/gateway:latest") + assert.Contains(t, out, "policy /config/root/gateway.yml") + assert.Contains(t, out, "subnet "+testSubnet) +} + +func TestStatusReportsAPolicyPathThatLeavesTheConfigTree(t *testing.T) { + t.Parallel() + + g := newSessionGateway(t) + + cfg := unusableConfig() + cfg.Config = "../gateway.yml" + + st := g.Inspect(newTestSession(t), testConfig(cfg), failingReady(t)) + + assert.Empty(t, st.Policy) + assert.NotEmpty(t, st.PolicyProblem) + assert.Contains(t, render(t, st), "policy cannot be resolved:") +} + +func TestStatusReportsNothingRunning(t *testing.T) { + t.Parallel() + + g := newSessionGateway(t) + s := newTestSession(t) + + st := g.Inspect(s, testConfig(unusableConfig()), failingReady(t)) + + assert.False(t, st.HolderRunning) + assert.False(t, st.Running) + + out := render(t, st) + assert.Contains(t, out, "holder not running (no live record in "+s.StatePath+")") + assert.Contains(t, out, "gateway not running (no live record in "+g.StatePath+")") + + // Readiness only says something once there is a gateway to ask. + assert.NotContains(t, out, "readiness") +} + +// A record outlives the process it names, and the pid in it may since have +// been given to something else. Alive compares the recorded start time, +// which is what makes such a record read as no gateway at all. +func TestStatusReadsAStaleRecordAsNotRunning(t *testing.T) { + t.Parallel() + + g := newSessionGateway(t) + + state := fmt.Sprintf(`{"pid":%d,"startTime":1}`, os.Getpid()) + require.NoError(t, os.WriteFile(g.StatePath, []byte(state), 0o600)) + + st := g.Inspect(newTestSession(t), testConfig(unusableConfig()), failingReady(t)) + + assert.False(t, st.Running) + assert.Zero(t, st.PID) + assert.Empty(t, st.ReadyErr) + assert.Contains(t, render(t, st), "gateway not running") +} + +func TestStatusReportsARunningHolderAndGateway(t *testing.T) { + t.Parallel() + + g := newSessionGateway(t) + s := newTestSession(t) + + require.NoError(t, sandbox.WriteState(s.StatePath, os.Getpid())) + require.NoError(t, sandbox.WriteState(g.StatePath, os.Getpid())) + + st := g.Inspect(s, testConfig(unusableConfig()), func() error { return nil }) + + assert.True(t, st.HolderRunning) + assert.Equal(t, os.Getpid(), st.HolderPID) + assert.True(t, st.Running) + assert.Equal(t, os.Getpid(), st.PID) + + out := render(t, st) + assert.Contains(t, out, fmt.Sprintf("holder running, pid %d", os.Getpid())) + assert.Contains(t, out, fmt.Sprintf("gateway running, pid %d", os.Getpid())) + assert.Contains(t, out, "readiness the resolver, proxy and ruleset are up") +} + +// A gateway that does not answer is a status worth reporting rather than +// something to wait out, so the failure becomes a line like any other. +func TestStatusReportsAGatewayThatIsNotReady(t *testing.T) { + t.Parallel() + + g := newSessionGateway(t) + require.NoError(t, sandbox.WriteState(g.StatePath, os.Getpid())) + + st := g.Inspect(newTestSession(t), testConfig(unusableConfig()), + func() error { return errors.New("context deadline exceeded") }) + + assert.Equal(t, "context deadline exceeded", st.ReadyErr) + assert.Contains(t, render(t, st), "readiness not ready: context deadline exceeded") +} + +func TestStatusReportsTheAddressesHandedOut(t *testing.T) { + t.Parallel() + + g := newSessionGateway(t) + require.NoError(t, sandbox.WriteState(g.StatePath, os.Getpid())) + + require.NoError(t, os.WriteFile(g.AllocPath, + []byte(fmt.Sprintf(`{"subnet":%q,"allocated":3}`, testSubnet)), 0o600)) + + st := g.Inspect(newTestSession(t), testConfig(unusableConfig()), func() error { return nil }) + + assert.Equal(t, "10.111.0.1", st.GatewayAddr) + assert.Equal(t, uint64(3), st.Allocated) + assert.Equal(t, "10.111.0.4", st.LastAddr) + assert.Contains(t, render(t, st), "addresses 10.111.0.1 is the gateway's own, 3 handed out up to 10.111.0.4") +} + +// The count belongs to the gateway that is running, and the next launch +// clears it, so it says nothing while there is no gateway. +func TestStatusReportsOnlyTheGatewayAddressWithNoGatewayRunning(t *testing.T) { + t.Parallel() + + g := newSessionGateway(t) + + require.NoError(t, os.WriteFile(g.AllocPath, + []byte(fmt.Sprintf(`{"subnet":%q,"allocated":3}`, testSubnet)), 0o600)) + + st := g.Inspect(newTestSession(t), testConfig(unusableConfig()), failingReady(t)) + + assert.Contains(t, render(t, st), "addresses 10.111.0.1 is the gateway's own\n") +} + +// A subnet changed under a running gateway is one of the things a status is +// for, so it is reported rather than refused the way a launch refuses it. +func TestStatusReportsASubnetTheRecordDoesNotMatch(t *testing.T) { + t.Parallel() + + g := newSessionGateway(t) + require.NoError(t, os.WriteFile(g.AllocPath, []byte(`{"subnet":"10.112.0.0/24","allocated":1}`), 0o600)) + + st := g.Inspect(newTestSession(t), testConfig(unusableConfig()), failingReady(t)) + + assert.Contains(t, st.AddrProblem, "hands addresses out of 10.112.0.0/24") + assert.Contains(t, render(t, st), "addresses the running gateway hands addresses out of 10.112.0.0/24") +} + +func TestStatusReportsAnUnusableSubnet(t *testing.T) { + t.Parallel() + + g := newSessionGateway(t) + + cfg := unusableConfig() + cfg.Subnet = "10.111.0.0" + + st := g.Inspect(newTestSession(t), testConfig(cfg), failingReady(t)) + + assert.Empty(t, st.GatewayAddr) + assert.Contains(t, st.AddrProblem, "invalid gateway subnet") +} + +// newTestSession returns a session whose files are in the test's own +// directory, so nothing here reads the user's session. +func newTestSession(t *testing.T) session.Session { + t.Helper() + + dir := t.TempDir() + + return session.Session{ + Dir: dir, + LockPath: filepath.Join(dir, "lock"), + StatePath: filepath.Join(dir, "holder.json"), + } +} + +// testRoot stands in for the directory a qubesome config was read from, +// which is what a policy path in it is resolved against. +const testRoot = "/config/root" + +func testConfig(gw types.GatewayConfig) *types.Config { + return &types.Config{RootDir: testRoot, Gateway: &gw} +} + +// failingReady is the readiness probe for a test that expects no gateway to +// be asked. +func failingReady(t *testing.T) func() error { + t.Helper() + + return func() error { + t.Error("the gateway was asked whether it is ready with none running") + return nil + } +} + +func render(t *testing.T, st Status) string { + t.Helper() + + var b strings.Builder + require.NoError(t, st.Write(&b)) + + return b.String() +} diff --git a/internal/gateway/stop.go b/internal/gateway/stop.go new file mode 100644 index 0000000..84afa90 --- /dev/null +++ b/internal/gateway/stop.go @@ -0,0 +1,84 @@ +package gateway + +import ( + "errors" + "fmt" + "os" + "syscall" + + "github.com/qubesome/cli/internal/files" + "github.com/qubesome/cli/internal/sandbox" +) + +// Stop takes the session's gateway down and returns the pid it stopped, or +// zero when none was running. +// +// The session's namespace holder is left alone. A launch reuses whatever +// gateway it finds running, so a change to the gateway block reaches nothing +// until the running one is gone, and that is all this is for. Taking the +// holder down as well would make the next launch rebuild a user namespace +// that was never the problem, and every sandbox of the session nests inside +// that one. +// +// A session with no gateway in it is not an error to ask for. It is the +// state the caller wanted, and saying so is the answer rather than a +// failure. +func (g Gateway) Stop() (int, error) { + // The lock file lives in the session directory, which a host that has + // never started a session does not have. + if err := os.MkdirAll(g.Dir, files.DirMode); err != nil { + return 0, fmt.Errorf("failed to create the session dir %q: %w", g.Dir, err) + } + + // The same lock startOnce takes, and for the same reason. A launch that + // is between its own check and the record it writes would otherwise + // have its gateway killed and its record kept, leaving the session + // naming a process that is already gone. + lock, err := acquire(g.LockPath) + if err != nil { + return 0, err + } + defer lock.Close() + + // sandbox.Alive and not a look at the pid. It compares the recorded + // start time as well, so a record left behind by a gateway that crashed + // reads as not running, and nothing here signals a pid the kernel has + // since given to something else. + if !sandbox.Alive(g.StatePath) { + // Removing a record that already reads as not running changes no + // answer. It is removed because reap would have removed it had the + // launch that started the gateway still been there to do it. + if err := os.Remove(g.StatePath); err != nil && !errors.Is(err, os.ErrNotExist) { + return 0, fmt.Errorf("failed to remove the gateway state %q: %w", g.StatePath, err) + } + + return 0, nil + } + + st, err := sandbox.ReadState(g.StatePath) + if err != nil { + return 0, err + } + + // The recorded pid is the sandbox's own init and not the bwrap that + // started it. Killing the outer bwrap does not signal the sandbox + // nested below it. The init is pid 1 of its own pid namespace, and a + // SIGKILL from an ancestor namespace takes the whole namespace with it, + // which is the pid running.stop names when it takes down a gateway that + // failed to come up. + // + // The uplink is not named here. pasta is not recorded anywhere, and the + // tap it created lives in the network namespace that has just gone. + if err := syscall.Kill(st.PID, syscall.SIGKILL); err != nil && !errors.Is(err, syscall.ESRCH) { + return 0, fmt.Errorf("failed to stop the gateway sandbox pid %d: %w", st.PID, err) + } + + // A qubesome run at a terminal exits long before the gateway it started + // does, so the goroutine that would have cleared this record has + // usually gone with it. + if err := os.Remove(g.StatePath); err != nil && !errors.Is(err, os.ErrNotExist) { + return st.PID, fmt.Errorf("failed to remove the gateway state %q: %w", g.StatePath, err) + } + + return st.PID, nil +} diff --git a/internal/gateway/stop_test.go b/internal/gateway/stop_test.go new file mode 100644 index 0000000..4c94377 --- /dev/null +++ b/internal/gateway/stop_test.go @@ -0,0 +1,141 @@ +// Stopping a real gateway cannot be tested here, for the reasons +// run_test.go gives: no sandbox starts in the development container, so +// there is never one to take down. What is covered is what Stop decides: +// which pid it signals, that it signals it at all, and the two cases where +// there is nothing to signal. +package gateway + +import ( + "context" + "fmt" + "os" + "path/filepath" + "testing" + "time" + + "github.com/qubesome/cli/internal/sandbox" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/sys/execabs" +) + +// A session with no gateway in it is the state the caller asked for. +func TestStopWithNoGatewayRunning(t *testing.T) { + t.Parallel() + + pid, err := newSessionGateway(t).Stop() + + require.NoError(t, err) + assert.Zero(t, pid) +} + +// A host that has never started a session has no session directory, and +// asking it to stop a gateway is still not an error. +func TestStopWithoutASessionDir(t *testing.T) { + t.Parallel() + + g := newSessionGateway(t) + g.Dir = filepath.Join(g.Dir, "session") + g.LockPath = filepath.Join(g.Dir, "gateway.lock") + g.StatePath = filepath.Join(g.Dir, "sandbox-gateway.json") + + pid, err := g.Stop() + + require.NoError(t, err) + assert.Zero(t, pid) +} + +// A record left behind by a gateway that crashed names a pid the kernel may +// since have given to something else. It reads as no gateway, so nothing is +// signalled, and the record goes because reap would have removed it. +func TestStopRemovesARecordLeftBehind(t *testing.T) { + t.Parallel() + + g := newSessionGateway(t) + + state := fmt.Sprintf(`{"pid":%d,"startTime":1}`, os.Getpid()) + require.NoError(t, os.WriteFile(g.StatePath, []byte(state), 0o600)) + + pid, err := g.Stop() + + require.NoError(t, err) + assert.Zero(t, pid) + assert.NoFileExists(t, g.StatePath) +} + +// The pid in the record is the one that is signalled, and the record it came +// from does not outlive it. +func TestStopKillsTheRecordedProcess(t *testing.T) { + t.Parallel() + + g := newSessionGateway(t) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + cmd := execabs.CommandContext(ctx, "sleep", "60") + require.NoError(t, cmd.Start()) + + require.NoError(t, sandbox.WriteState(g.StatePath, cmd.Process.Pid)) + + pid, err := g.Stop() + + require.NoError(t, err) + assert.Equal(t, cmd.Process.Pid, pid) + assert.NoFileExists(t, g.StatePath) + + err = cmd.Wait() + require.Error(t, err) + assert.Contains(t, err.Error(), "killed") +} + +// Nothing is signalled twice. The second call finds a record naming a +// process that is gone, which is the crashed gateway case again. +func TestStopIsRepeatable(t *testing.T) { + t.Parallel() + + g := newSessionGateway(t) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + cmd := execabs.CommandContext(ctx, "sleep", "60") + require.NoError(t, cmd.Start()) + + require.NoError(t, sandbox.WriteState(g.StatePath, cmd.Process.Pid)) + + _, err := g.Stop() + require.NoError(t, err) + _ = cmd.Wait() + + pid, err := g.Stop() + require.NoError(t, err) + assert.Zero(t, pid) +} + +// The lock Stop takes is the one a launch takes to start a gateway, so it is +// released by the time Stop returns and a launch is not left waiting on it. +func TestStopReleasesTheGatewayLock(t *testing.T) { + t.Parallel() + + g := newSessionGateway(t) + + _, err := g.Stop() + require.NoError(t, err) + + done := make(chan struct{}) + go func() { + defer close(done) + + lock, err := acquire(g.LockPath) + if err == nil { + lock.Close() + } + }() + + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("the gateway lock was still held after Stop returned") + } +} From 291f22d9dab639647522de07289c5a2687458480 Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Thu, 10 Sep 2026 08:26:46 +0100 Subject: [PATCH 02/16] profiles, bwrap: carry the host timezone with TZ, not a mount A profile stopped starting after a bubblewrap upgrade: bwrap: Can't mount on symlink destination /etc/localtime 0.12.0 rewrote sandbox setup to resolve paths with openat2(RESOLVE_IN_ROOT), fixing GHSA-pxhw-h44j-8pfx, and added with it an unconditional refusal to mount on a destination that is a symlink. An image ships /etc/localtime as one, so the bind qubesome has always asked for is now fatal, for a workload as much as for a profile. Up to 0.11.x ensure_file() tested the destination with stat(), which follows links, so the mount landed on the image's link target instead and nobody had to think about it. There is no opting out: --not-a-security-boundary only reaches BIND_FAIL_OPEN, which is applied after the die. So nothing is mounted on /etc/localtime any more. internal/util/tz resolves the host's zone file, shares it under the name every image knows it by, and TZ points the sandbox at it. That is the mechanism the workload runner already used for profile.Timezone, and it carries the zone name as well as the offsets, which following a link into the image never did: a host on Europe/London used to reach a sandbox still calling itself Etc/UTC. A host that copied its zone file into place rather than linking to one has no name to carry, and keeps its offsets under one of qubesome's own. The cost is that /etc/localtime inside a sandbox is now the image's own, so anything reading it while ignoring TZ sees the image's zone. Every C library reads TZ ahead of that file, and so do the runtimes that resolve a zone themselves rather than through one. profile.Timezone now also reaches the profile sandbox, so the window manager's own clock follows it rather than the host. Assisted-by: Claude Opus 4.8 Signed-off-by: Paulo Gomes Entire-Checkpoint: 01M253CGA46V3B5FSJJDY0E4AN --- internal/profiles/profiles.go | 39 ++++- internal/profiles/profiles_test.go | 28 +++- internal/runners/bwrap/run.go | 29 +--- internal/runners/bwrap/run_test.go | 69 +++++++- internal/runners/bwrap/spec.go | 33 +++- .../runners/bwrap/testdata/granted.golden | 3 - .../runners/bwrap/testdata/hostnet.golden | 3 - internal/runners/bwrap/testdata/plain.golden | 3 - .../runners/bwrap/testdata/supervised.golden | 3 - internal/util/tz/tz.go | 104 ++++++++++++ internal/util/tz/tz_test.go | 151 ++++++++++++++++++ 11 files changed, 405 insertions(+), 60 deletions(-) create mode 100644 internal/util/tz/tz.go create mode 100644 internal/util/tz/tz_test.go diff --git a/internal/profiles/profiles.go b/internal/profiles/profiles.go index 6eea33f..a093467 100644 --- a/internal/profiles/profiles.go +++ b/internal/profiles/profiles.go @@ -34,6 +34,7 @@ import ( "github.com/qubesome/cli/internal/util/gpu" "github.com/qubesome/cli/internal/util/mtls" "github.com/qubesome/cli/internal/util/resolution" + "github.com/qubesome/cli/internal/util/tz" "github.com/qubesome/cli/internal/util/xauth" "github.com/qubesome/cli/internal/util/xkb" "github.com/qubesome/cli/pkg/inception" @@ -571,8 +572,8 @@ func createMagicCookie(profile *types.Profile) error { // and the desktop files are mounted, so the environment and the mount // list cannot disagree. bwrap applies --setenv in order, so these are the // values that survive. -func sandboxEnv(bundle images.Bundle, ca, cert, key []byte) []string { - const extra = 6 +func sandboxEnv(bundle images.Bundle, ca, cert, key []byte, timezone string) []string { + const extra = 7 // The compositor decides the keymap for everything in the profile, so // the host's layout is carried in here rather than anywhere nearer @@ -591,6 +592,13 @@ func sandboxEnv(bundle images.Bundle, ca, cert, key []byte) []string { env = append(env, bundle.Env...) env = append(env, keymap...) + // An empty value is not the same as no value here: a C library reads + // TZ="" as UTC, so a host with no timezone to give has to leave the + // image's alone rather than say nothing in a way that means UTC. + if timezone != "" { + env = append(env, "TZ="+timezone) + } + return append(env, "HOME="+profileHome, "USER="+profileUser, @@ -736,8 +744,23 @@ func createNewDisplay(bundle images.Bundle, ca, cert, key []byte, profile *types return nil, err } + // The host's timezone is shared as a zone file under its own name, + // and TZ below is what points the sandbox at it. It is deliberately + // not mounted on /etc/localtime: an image ships that as a symlink, + // and bubblewrap 0.12.0 refuses to mount on one. Releases before it + // followed the link and mounted on its target, which is why sharing + // /etc/localtime worked until 0.12.0 landed. + zone := tz.Host() + + // A profile that names a timezone means it for everything inside, + // the window manager's own clock included, whatever the host is set + // to. + timezone := profile.Timezone + if timezone == "" { + timezone = zone.TZ + } + mounts := []sandbox.Mount{ - {Src: "/etc/localtime", Dst: "/etc/localtime", ReadOnly: true}, {Src: x11Dir, Dst: "/tmp/.X11-unix"}, {Src: socket, Dst: "/tmp/qube.sock", ReadOnly: true}, {Src: server, Dst: profileHome + "/.Xserver"}, @@ -745,6 +768,14 @@ func createNewDisplay(bundle images.Bundle, ca, cert, key []byte, profile *types {Src: binPath, Dst: files.InProfileBinary, ReadOnly: true}, } + if zone.HostPath != "" { + mounts = append(mounts, sandbox.Mount{ + Src: zone.HostPath, + Dst: zone.SandboxPath, + ReadOnly: true, + }) + } + for _, p := range profile.Paths { p = env.Expand(p) @@ -821,7 +852,7 @@ func createNewDisplay(bundle images.Bundle, ca, cert, key []byte, profile *types } } - senv := sandboxEnv(bundle, ca, cert, key) + senv := sandboxEnv(bundle, ca, cert, key, timezone) // The profile runs its own compositor, so it needs nothing from the // host session beyond the display socket mounted above. The session diff --git a/internal/profiles/profiles_test.go b/internal/profiles/profiles_test.go index 284c91c..c477e85 100644 --- a/internal/profiles/profiles_test.go +++ b/internal/profiles/profiles_test.go @@ -4,6 +4,7 @@ import ( "os" "os/exec" "path/filepath" + "slices" "strings" "testing" @@ -57,7 +58,7 @@ func TestSandboxEnvNamesTheProfileUser(t *testing.T) { t.Setenv("DISPLAY", ":0") bundle := images.Bundle{Env: []string{"PATH=/usr/bin", "HOME=/root"}} - senv := sandboxEnv(bundle, []byte("ca"), []byte("cert"), []byte("key")) + senv := sandboxEnv(bundle, []byte("ca"), []byte("cert"), []byte("key"), "") assert.Equal(t, []string{"PATH=/usr/bin", "HOME=/root"}, senv[:2], "the image environment must come first") @@ -72,12 +73,35 @@ func TestSandboxEnvNamesTheProfileUser(t *testing.T) { func TestSandboxEnvWithAnEmptyImageEnvironment(t *testing.T) { t.Setenv("DISPLAY", ":0") - senv := sandboxEnv(images.Bundle{}, nil, nil, nil) + senv := sandboxEnv(images.Bundle{}, nil, nil, nil, "") assert.Equal(t, "/home/xorg-user", lastEnv(senv, "HOME")) assert.Equal(t, "xorg-user", lastEnv(senv, "USER")) } +// The profile sandbox used to read the host timezone out of the +// /etc/localtime shared with it. Nothing is mounted there any more, so +// TZ is what carries it to the window manager's own clock. +func TestSandboxEnvCarriesTheTimezone(t *testing.T) { + t.Setenv("DISPLAY", ":0") + + senv := sandboxEnv(images.Bundle{}, nil, nil, nil, "Europe/London") + + assert.Equal(t, "Europe/London", lastEnv(senv, "TZ")) +} + +// An empty TZ is not the same as no TZ: a C library reads one as UTC, +// so a host with no timezone to give must leave the image's alone. +func TestSandboxEnvWithoutATimezone(t *testing.T) { + t.Setenv("DISPLAY", ":0") + + senv := sandboxEnv(images.Bundle{}, nil, nil, nil, "") + + assert.False(t, slices.ContainsFunc(senv, func(e string) bool { + return strings.HasPrefix(e, "TZ=") + })) +} + // lastEnv returns the value of the last assignment to name, which is the // one bwrap keeps. func lastEnv(env []string, name string) string { diff --git a/internal/runners/bwrap/run.go b/internal/runners/bwrap/run.go index ec144fa..4678aee 100644 --- a/internal/runners/bwrap/run.go +++ b/internal/runners/bwrap/run.go @@ -22,6 +22,7 @@ import ( "github.com/qubesome/cli/internal/util/dbus" "github.com/qubesome/cli/internal/util/env" "github.com/qubesome/cli/internal/util/gpu" + "github.com/qubesome/cli/internal/util/tz" "golang.org/x/sys/execabs" ) @@ -372,7 +373,7 @@ func resolve(ew types.EffectiveWorkload, gw bool) (input, error) { ShmDir: shmDir, CookiePath: cookiePath, SocketPath: socketPath, - Localtime: localtime(), + Zone: tz.Host(), USBDevices: usbDevices, Paths: mappedPaths(wl.HostAccess.Paths), } @@ -478,32 +479,6 @@ func resolveMime(in *input) error { return nil } -// localtime returns /etc/localtime and, when it is a symlink, the file it -// points at. -// -// The link on its own resolves to nothing inside the sandbox, so both are -// shared. -func localtime() []string { - const file = "/etc/localtime" - - if _, err := os.Stat(file); err != nil { - return nil - } - - paths := make([]string, 0, 2) - paths = append(paths, file) - - target, err := os.Readlink(file) - if err != nil { - return paths - } - if !filepath.IsAbs(target) { - target = filepath.Join(filepath.Dir(file), target) - } - - return append(paths, target) -} - // mappedPaths expands the workload's mapped directories and creates the // host side of each. // diff --git a/internal/runners/bwrap/run_test.go b/internal/runners/bwrap/run_test.go index 6f69dbf..1e937e9 100644 --- a/internal/runners/bwrap/run_test.go +++ b/internal/runners/bwrap/run_test.go @@ -13,6 +13,7 @@ import ( "github.com/qubesome/cli/internal/sandbox" "github.com/qubesome/cli/internal/types" "github.com/qubesome/cli/internal/util/gpu" + "github.com/qubesome/cli/internal/util/tz" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -69,7 +70,11 @@ func plainInput() input { ShmDir: "/run/user/1000/qubesome/work/shm/chrome", CookiePath: "/run/user/1000/qubesome/work/.Xclient-cookie", SocketPath: "/run/user/1000/qubesome/work/qube.sock", - Localtime: []string{"/etc/localtime", "/usr/share/zoneinfo/Europe/London"}, + Zone: tz.Zone{ + HostPath: "/usr/share/zoneinfo/Europe/London", + SandboxPath: "/usr/share/zoneinfo/Europe/London", + TZ: "Europe/London", + }, } } @@ -266,15 +271,65 @@ func TestSpecIsolatedRunUser(t *testing.T) { indexOfArg(args, "--ro-bind", filepath.Join(in.ProfileDir, "machine-id"))) } -// /etc/localtime is usually a symlink, and the link alone resolves to -// nothing inside the sandbox. -func TestSpecSharesLocaltimeAndItsTarget(t *testing.T) { +// An image ships /etc/localtime as a symlink, and bubblewrap 0.12.0 +// refuses to mount on one. The host's zone reaches the sandbox as a +// zone file under its own name plus TZ, so nothing mounts there at all. +func TestSpecNeverMountsOnLocaltime(t *testing.T) { t.Parallel() - args := render(t, plainInput()) + assert.NotContains(t, render(t, plainInput()), "/etc/localtime") +} + +func TestSpecSharesTheHostZoneAndNamesIt(t *testing.T) { + t.Parallel() + + in := plainInput() + in.Workload.Profile.Timezone = "" + in.Zone = tz.Zone{ + HostPath: "/etc/zoneinfo/Europe/London", + SandboxPath: "/usr/share/zoneinfo/Europe/London", + TZ: "Europe/London", + } + + args := render(t, in) + + i := indexOfArg(args, "--ro-bind", "/etc/zoneinfo/Europe/London") + require.NotEqual(t, -1, i) + assert.Equal(t, "/usr/share/zoneinfo/Europe/London", args[i+2]) + + j := indexOfArg(args, "--setenv", "TZ") + require.NotEqual(t, -1, j) + assert.Equal(t, "Europe/London", args[j+2]) +} + +// A profile that names a timezone means it, whatever the host is set to. +func TestSpecPrefersTheProfileTimezone(t *testing.T) { + t.Parallel() + + in := plainInput() + in.Workload.Profile.Timezone = "America/New_York" + + args := render(t, in) + + i := indexOfArg(args, "--setenv", "TZ") + require.NotEqual(t, -1, i) + assert.Equal(t, "America/New_York", args[i+2]) + assert.Equal(t, 1, countArg(args, "--setenv", "TZ")) +} + +// A host with no timezone to give leaves the sandbox on the image's own, +// which is what the container runner did before it. +func TestSpecWithoutATimezone(t *testing.T) { + t.Parallel() + + in := plainInput() + in.Workload.Profile.Timezone = "" + in.Zone = tz.Zone{} + + args := render(t, in) - assert.NotEqual(t, -1, indexOfArg(args, "--ro-bind", "/etc/localtime")) - assert.NotEqual(t, -1, indexOfArg(args, "--ro-bind", "/usr/share/zoneinfo/Europe/London")) + assert.Equal(t, -1, indexOfArg(args, "--setenv", "TZ")) + assert.NotContains(t, args, "/usr/share/zoneinfo/Europe/London") } // There is no uplink in this stage, so a workload with anything short of diff --git a/internal/runners/bwrap/spec.go b/internal/runners/bwrap/spec.go index d8a2abe..61189f8 100644 --- a/internal/runners/bwrap/spec.go +++ b/internal/runners/bwrap/spec.go @@ -21,6 +21,7 @@ import ( "github.com/qubesome/cli/internal/sandbox" "github.com/qubesome/cli/internal/types" "github.com/qubesome/cli/internal/util/gpu" + "github.com/qubesome/cli/internal/util/tz" ) // runUserDir is the runtime directory a workload sees. Every workload runs @@ -88,10 +89,10 @@ type input struct { // workload does not handle mime types. HomeDir string - // Localtime is /etc/localtime and, when that is a symlink, the file it - // points at. Both are needed: the link alone resolves to nothing - // inside the sandbox. - Localtime []string + // Zone is the host's timezone. It is the zero value when the host + // has none to give, and it is ignored when the profile names a + // timezone of its own. + Zone tz.Zone // VideoDevices are the /dev/video* nodes found on the host. VideoDevices []string @@ -354,8 +355,17 @@ func workloadMounts(in input) []sandbox.Mount { var mounts []sandbox.Mount - for _, p := range in.Localtime { - mounts = append(mounts, sandbox.Mount{Src: p, Dst: p, ReadOnly: true}) + // The zone file lands under its own name rather than on + // /etc/localtime, which an image ships as a symlink and bubblewrap + // 0.12.0 refuses to mount on. TZ below is what points the sandbox at + // it, so an image whose timezone database already holds the name + // only gains the host's copy of the same file. + if in.Zone.HostPath != "" { + mounts = append(mounts, sandbox.Mount{ + Src: in.Zone.HostPath, + Dst: in.Zone.SandboxPath, + ReadOnly: true, + }) } mounts = append(mounts, sandbox.Mount{Src: in.ShmDir, Dst: "/dev/shm"}) @@ -480,8 +490,15 @@ func workloadEnv(in input) []string { "QUBESOME_PROFILE="+profile.Name, ) - if profile.Timezone != "" { - env = append(env, "TZ="+profile.Timezone) + // A profile that names a timezone means it, whatever the host is set + // to. Otherwise the workload follows the host, which it used to do + // by reading the /etc/localtime shared with it. + timezone := profile.Timezone + if timezone == "" { + timezone = in.Zone.TZ + } + if timezone != "" { + env = append(env, "TZ="+timezone) } env = append(env, in.HostEnv...) diff --git a/internal/runners/bwrap/testdata/granted.golden b/internal/runners/bwrap/testdata/granted.golden index e06bd0b..2d19da2 100644 --- a/internal/runners/bwrap/testdata/granted.golden +++ b/internal/runners/bwrap/testdata/granted.golden @@ -59,9 +59,6 @@ kali-vpn-pentest /dev/hidraw9 /dev/hidraw9 --ro-bind -/etc/localtime -/etc/localtime ---ro-bind /usr/share/zoneinfo/Europe/London /usr/share/zoneinfo/Europe/London --bind diff --git a/internal/runners/bwrap/testdata/hostnet.golden b/internal/runners/bwrap/testdata/hostnet.golden index 9ffd706..efc8f3c 100644 --- a/internal/runners/bwrap/testdata/hostnet.golden +++ b/internal/runners/bwrap/testdata/hostnet.golden @@ -32,9 +32,6 @@ chrome-work /dev/dri /dev/dri --ro-bind -/etc/localtime -/etc/localtime ---ro-bind /usr/share/zoneinfo/Europe/London /usr/share/zoneinfo/Europe/London --bind diff --git a/internal/runners/bwrap/testdata/plain.golden b/internal/runners/bwrap/testdata/plain.golden index fb1feb8..34967a2 100644 --- a/internal/runners/bwrap/testdata/plain.golden +++ b/internal/runners/bwrap/testdata/plain.golden @@ -33,9 +33,6 @@ chrome-work /dev/dri /dev/dri --ro-bind -/etc/localtime -/etc/localtime ---ro-bind /usr/share/zoneinfo/Europe/London /usr/share/zoneinfo/Europe/London --bind diff --git a/internal/runners/bwrap/testdata/supervised.golden b/internal/runners/bwrap/testdata/supervised.golden index dd82ae5..de396ae 100644 --- a/internal/runners/bwrap/testdata/supervised.golden +++ b/internal/runners/bwrap/testdata/supervised.golden @@ -33,9 +33,6 @@ chrome-work /dev/dri /dev/dri --ro-bind -/etc/localtime -/etc/localtime ---ro-bind /usr/share/zoneinfo/Europe/London /usr/share/zoneinfo/Europe/London --bind diff --git a/internal/util/tz/tz.go b/internal/util/tz/tz.go new file mode 100644 index 0000000..13ef274 --- /dev/null +++ b/internal/util/tz/tz.go @@ -0,0 +1,104 @@ +// Package tz carries the host's timezone into a sandbox. +package tz + +import ( + "log/slog" + "path" + "path/filepath" + "regexp" + "strings" +) + +// hostLocaltime is where a host records the zone it is set to. +const hostLocaltime = "/etc/localtime" + +// sandboxZoneinfo is the directory a C library searches for the zone TZ +// names, and it is the same one on every distribution whatever the host +// keeps its own database under. +const sandboxZoneinfo = "/usr/share/zoneinfo" + +// unnamedZone is where a host zone file that names no zone is shared. No +// timezone database ships a file called localtime, so this destination +// cannot land on top of a real zone. +const unnamedZone = sandboxZoneinfo + "/localtime" + +// namePattern is an IANA zone name: slash separated components of the +// characters tzdata uses in its file names, such as Europe/London, +// America/Argentina/Buenos_Aires, Etc/GMT+1 or plain UTC. +// +// A path that does not match names no zone. Passing one to TZ would be +// worse than not naming a zone at all, because a C library that cannot +// read TZ as a zone name reads it as a POSIX rule instead, and every +// rule it fails to parse leaves the sandbox on UTC without saying so. +var namePattern = regexp.MustCompile(`^[A-Za-z0-9_+-]+(?:/[A-Za-z0-9_+-]+)*$`) + +// Zone is the host's timezone in the form a sandbox needs it. +type Zone struct { + // HostPath is the zone file on the host, with every symlink already + // resolved. It is empty when the host has no timezone to give, which + // is the only field a caller needs to test. + HostPath string + + // SandboxPath is where HostPath is shared inside the sandbox. It is + // never /etc/localtime: an image almost always ships that as a + // symlink, and bubblewrap 0.12.0 refuses to mount on one. Releases + // before it followed the link and mounted on its target, which is + // why sharing /etc/localtime worked until 0.12.0 landed. + SandboxPath string + + // TZ is the value of the TZ environment variable, naming the zone + // that SandboxPath holds. + // + // This is what actually gives the sandbox the host's time, since + // /etc/localtime inside it still comes from the image. Every C + // library reads TZ ahead of /etc/localtime, and so do the runtimes + // that resolve a zone themselves rather than through one. + TZ string +} + +// Host returns the host's timezone, and the zero Zone when the host has +// none to give. +func Host() Zone { + return zoneOf(hostLocaltime) +} + +func zoneOf(localtime string) Zone { + // The link is followed here rather than shared as it is, so that a + // host naming its zone through one of the database's own aliases, + // GB for Europe/London, shares the file under the name every image + // has it under. + file, err := filepath.EvalSymlinks(localtime) + if err != nil { + slog.Debug("no host timezone to share", "path", localtime, "error", err) + return Zone{} + } + + name, ok := nameOf(file) + if !ok { + // A host that copied its zone file into place instead of + // linking to one has a timezone but no name for it. TZ takes + // the file directly, which loses only the name: the offsets and + // abbreviations all come out of the file either way. + return Zone{HostPath: file, SandboxPath: unnamedZone, TZ: ":" + unnamedZone} + } + + return Zone{HostPath: file, SandboxPath: path.Join(sandboxZoneinfo, name), TZ: name} +} + +// nameOf reads the zone name out of a resolved zone file's path, which +// is whatever follows the timezone database directory. +func nameOf(file string) (string, bool) { + const database = "/zoneinfo/" + + i := strings.LastIndex(file, database) + if i < 0 { + return "", false + } + + name := file[i+len(database):] + if !namePattern.MatchString(name) { + return "", false + } + + return name, true +} diff --git a/internal/util/tz/tz_test.go b/internal/util/tz/tz_test.go new file mode 100644 index 0000000..61d965e --- /dev/null +++ b/internal/util/tz/tz_test.go @@ -0,0 +1,151 @@ +package tz + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestZoneOf(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + // zoneFile is a regular file created under the root. Empty + // creates none, which leaves a link dangling. + zoneFile string + // link is what etc/localtime points at. Empty makes it a + // regular file instead. + link string + // wantHost is relative to the root, and empty means the host + // has no timezone to give. + wantHost string + wantSandbox string + wantTZ string + }{ + { + name: "a named zone", + zoneFile: "usr/share/zoneinfo/Europe/London", + link: "../usr/share/zoneinfo/Europe/London", + wantHost: "usr/share/zoneinfo/Europe/London", + wantSandbox: "/usr/share/zoneinfo/Europe/London", + wantTZ: "Europe/London", + }, + { + name: "a name of more than two components", + zoneFile: "usr/share/zoneinfo/America/Argentina/Buenos_Aires", + link: "../usr/share/zoneinfo/America/Argentina/Buenos_Aires", + wantHost: "usr/share/zoneinfo/America/Argentina/Buenos_Aires", + wantSandbox: "/usr/share/zoneinfo/America/Argentina/Buenos_Aires", + wantTZ: "America/Argentina/Buenos_Aires", + }, + { + name: "a name of one component", + zoneFile: "usr/share/zoneinfo/UTC", + link: "../usr/share/zoneinfo/UTC", + wantHost: "usr/share/zoneinfo/UTC", + wantSandbox: "/usr/share/zoneinfo/UTC", + wantTZ: "UTC", + }, + { + name: "a database the host does not keep under /usr/share", + zoneFile: "etc/zoneinfo/Europe/London", + link: "zoneinfo/Europe/London", + wantHost: "etc/zoneinfo/Europe/London", + wantSandbox: "/usr/share/zoneinfo/Europe/London", + wantTZ: "Europe/London", + }, + { + name: "a zone file copied into place, which names nothing", + wantHost: "etc/localtime", + wantSandbox: "/usr/share/zoneinfo/localtime", + wantTZ: ":/usr/share/zoneinfo/localtime", + }, + { + name: "a name no timezone database would have written", + zoneFile: "usr/share/zoneinfo/Europe/Lon don", + link: "../usr/share/zoneinfo/Europe/Lon don", + wantHost: "usr/share/zoneinfo/Europe/Lon don", + wantSandbox: "/usr/share/zoneinfo/localtime", + wantTZ: ":/usr/share/zoneinfo/localtime", + }, + { + name: "a link to a zone that is not there", + link: "../usr/share/zoneinfo/Europe/London", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + root := t.TempDir() + if tc.zoneFile != "" { + writeFile(t, filepath.Join(root, tc.zoneFile)) + } + + localtime := filepath.Join(root, "etc", "localtime") + require.NoError(t, os.MkdirAll(filepath.Dir(localtime), 0o755)) + if tc.link == "" { + writeFile(t, localtime) + } else { + require.NoError(t, os.Symlink(tc.link, localtime)) + } + + want := Zone{SandboxPath: tc.wantSandbox, TZ: tc.wantTZ} + if tc.wantHost != "" { + want.HostPath = resolved(t, filepath.Join(root, tc.wantHost)) + } + + assert.Equal(t, want, zoneOf(localtime)) + }) + } +} + +// A host that names a zone by one of the database's own aliases still +// shares a file every image has under the name it has there. +func TestZoneOfAnAlias(t *testing.T) { + t.Parallel() + + root := t.TempDir() + london := filepath.Join(root, "usr/share/zoneinfo/Europe/London") + writeFile(t, london) + require.NoError(t, os.Symlink("Europe/London", filepath.Join(root, "usr/share/zoneinfo/GB"))) + + localtime := filepath.Join(root, "etc", "localtime") + require.NoError(t, os.MkdirAll(filepath.Dir(localtime), 0o755)) + require.NoError(t, os.Symlink("../usr/share/zoneinfo/GB", localtime)) + + assert.Equal(t, Zone{ + HostPath: resolved(t, london), + SandboxPath: "/usr/share/zoneinfo/Europe/London", + TZ: "Europe/London", + }, zoneOf(localtime)) +} + +func TestZoneOfAHostWithoutOne(t *testing.T) { + t.Parallel() + + assert.Equal(t, Zone{}, zoneOf(filepath.Join(t.TempDir(), "etc", "localtime"))) +} + +func writeFile(t *testing.T, path string) { + t.Helper() + + require.NoError(t, os.MkdirAll(filepath.Dir(path), 0o755)) + require.NoError(t, os.WriteFile(path, []byte("TZif"), 0o600)) +} + +// resolved is what the zone file's path is once the temporary directory's +// own symlinks are gone, which is what zoneOf reports. +func resolved(t *testing.T, path string) string { + t.Helper() + + out, err := filepath.EvalSymlinks(path) + require.NoError(t, err) + + return out +} From 2ed144dee9b4b4f8accdd06b38e2ba7a4f935936 Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Thu, 10 Sep 2026 11:12:04 +0100 Subject: [PATCH 03/16] host-run: authorize the command against the profile's display qubesome host-run built the command's environment as exactly one entry, DISPLAY, so it reached the profile's display with no cookie and no HOME to find one under. The display is served by an Xwayland started with -auth, which refuses a connection that arrives without one: Authorization required, but no authorization protocol specified The socket was never the problem. A profile bind-mounts the host's /tmp/.X11-unix read-write, so Xwayland's socket has always been on the host, under bwrap as much as under the container runners before it. Only the cookie was missing, and it has been missing since the command was added: nothing qubesome did ever authorized it. A profile whose dotfiles loosened access control from the inside, with an xhost +local: or +si:localuser:, would have papered over that, which is the likely reason it used to work. The client cookie is the one passed rather than the server's. Both carry the same value, but the client copy is written with the wildcard family, so it matches whatever the host currently calls itself, while the server copy names the hostname captured when the profile started. The host environment is now inherited rather than replaced. The command runs on the host with the user's own privileges, so withholding HOME and PATH from it isolates nothing and only stops it finding its own configuration. WAYLAND_DISPLAY is dropped on the way through, because a toolkit that finds one ignores DISPLAY, and the window would open on the host desktop instead of in the profile. Assisted-by: Claude Opus 5 Signed-off-by: Paulo Gomes Entire-Checkpoint: 01M25CV60KZCHXZCG7YG7VR27N --- cmd/cli/host_run.go | 48 +++++++++++++++++++++++++++++++++++++++- cmd/cli/host_run_test.go | 39 ++++++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 1 deletion(-) create mode 100644 cmd/cli/host_run_test.go diff --git a/cmd/cli/host_run.go b/cmd/cli/host_run.go index c2c8007..0931245 100644 --- a/cmd/cli/host_run.go +++ b/cmd/cli/host_run.go @@ -3,11 +3,52 @@ package cli import ( "context" "fmt" + "os" "os/exec" + "strconv" + "strings" + "github.com/qubesome/cli/internal/files" "github.com/urfave/cli/v3" ) +// hostRunEnv returns the environment for a command the host runs on a +// profile's display. +// +// The host environment is inherited rather than replaced. The command runs +// on the host as the user, with the user's own privileges, so withholding +// anything from it isolates nothing. Starting from an empty environment +// only takes away HOME and PATH, without which most host applications +// cannot find their own configuration, or a shell to run. +// +// DISPLAY and XAUTHORITY then point it at the profile instead of the host +// session. The cookie is the one the profile's workloads authenticate with, +// and the profile's X server refuses a connection that arrives without it, +// saying only that no authorization protocol was specified. +// +// WAYLAND_DISPLAY is dropped because a toolkit that finds one connects to +// it and ignores DISPLAY, which would open the window on the host desktop +// rather than in the profile. The profile's own window manager is started +// without it for the same reason. +// +// The two entries are appended rather than substituted for the inherited +// ones. os/exec keeps the last value of a repeated key, so these are the +// values the command reads. +func hostRunEnv(base []string, display uint8, cookie string) []string { + env := make([]string, 0, len(base)+2) + for _, e := range base { + if strings.HasPrefix(e, "WAYLAND_DISPLAY=") { + continue + } + env = append(env, e) + } + + return append(env, + "DISPLAY=:"+strconv.Itoa(int(display)), + "XAUTHORITY="+cookie, + ) +} + func hostRunCommand() *cli.Command { cmd := &cli.Command{ Name: "host-run", @@ -37,8 +78,13 @@ qubesome host-run -profile firefox - Run firefox on the host and d return err } + cookie, err := files.ClientCookiePath(prof.Name) + if err != nil { + return err + } + c := exec.Command(commandName, cmd.Args().Slice()...) //nolint - c.Env = append(c.Env, fmt.Sprintf("DISPLAY=:%d", prof.Display)) + c.Env = hostRunEnv(os.Environ(), prof.Display, cookie) out, err := c.CombinedOutput() fmt.Println(string(out)) diff --git a/cmd/cli/host_run_test.go b/cmd/cli/host_run_test.go new file mode 100644 index 0000000..7a6ee7f --- /dev/null +++ b/cmd/cli/host_run_test.go @@ -0,0 +1,39 @@ +package cli + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestHostRunEnv(t *testing.T) { + t.Parallel() + + got := hostRunEnv([]string{ + "HOME=/home/user", + "PATH=/usr/bin", + "DISPLAY=:0", + "XAUTHORITY=/home/user/.Xauthority", + }, 21, "/home/user/.qubesome/run/work/.Xclient-cookie") + + require.Equal(t, []string{ + "HOME=/home/user", + "PATH=/usr/bin", + "DISPLAY=:0", + "XAUTHORITY=/home/user/.Xauthority", + "DISPLAY=:21", + "XAUTHORITY=/home/user/.qubesome/run/work/.Xclient-cookie", + }, got) +} + +func TestHostRunEnvDropsWaylandDisplay(t *testing.T) { + t.Parallel() + + got := hostRunEnv([]string{ + "HOME=/home/user", + "WAYLAND_DISPLAY=wayland-0", + }, 21, "/cookie") + + require.NotContains(t, got, "WAYLAND_DISPLAY=wayland-0") + require.Contains(t, got, "HOME=/home/user") +} From e5096fdb62226a35cf67e823efd428f53cdbe196 Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Thu, 10 Sep 2026 11:12:26 +0100 Subject: [PATCH 04/16] host-run: return while the command runs host-run waited on the command through CombinedOutput, so it held the terminal it was typed at until the application exited, and showed nothing until then. Launching an application into a profile is what the command is for, so that wait was its whole cost. It now starts the command and returns, as launching a workload does. Its stdout and stderr stay pointed at the terminal, because a command that cannot reach the profile's display says why there, and its stdin is left closed rather than pointed at a terminal the shell has taken back. The exit status now reports the launch rather than the application: a failure after it starts arrives as output, once the prompt is already back. Assisted-by: Claude Opus 5 Signed-off-by: Paulo Gomes Entire-Checkpoint: 01M25CVVJAM0PW6AGGC9FAFSGB --- cmd/cli/host_run.go | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/cmd/cli/host_run.go b/cmd/cli/host_run.go index 0931245..8d56fa4 100644 --- a/cmd/cli/host_run.go +++ b/cmd/cli/host_run.go @@ -85,10 +85,21 @@ qubesome host-run -profile firefox - Run firefox on the host and d c := exec.Command(commandName, cmd.Args().Slice()...) //nolint c.Env = hostRunEnv(os.Environ(), prof.Display, cookie) - out, err := c.CombinedOutput() - fmt.Println(string(out)) - return err + // This returns while the command keeps running, as launching + // a workload does, so the terminal it was typed at is free + // again. Its stdin is left closed rather than pointed at that + // terminal, which the shell has taken back. Its output still + // goes there, because a command that fails to reach the + // profile's display says why on it. + c.Stdout = os.Stdout + c.Stderr = os.Stderr + + if err := c.Start(); err != nil { + return fmt.Errorf("failed to start %q: %w", commandName, err) + } + + return nil }, } return cmd From a9b94977a96e0cb8842ddcb54fa531788f580b86 Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Thu, 10 Sep 2026 15:15:19 +0100 Subject: [PATCH 05/16] gateway: keep the gateway's log where it can be read The gateway's stdout and stderr were this process's own. It is started by whichever qubesome run found none already running, and put in a session of its own so that a Ctrl-C at that terminal does not take the session's egress away with it, so what it says went to a terminal nobody is necessarily still watching, interleaved with the output of the workload that happened to start it. The audit line behind a refused connection is the one thing needed when a workload cannot reach something, and there was nowhere to go and read it. The gateway sandbox and its uplink now write to a log in the session directory instead. The sandbox's launch truncates it and the uplink, which is started once that sandbox is up, appends, so one file describes the gateway that is running, in order. It is 0600: it holds every name a workload asked for and every host it was allowed or refused, which is a record of what the user was doing. Truncating on launch is why there is no rotation. The log belongs to one gateway rather than to the session, and a gateway is only ever replaced by the first one having gone. `qubesome gateway logs` reads it back, with -n for the last lines and -f to keep printing what is added. A follow is a poll, since the sandbox writes to the file through a descriptor and nothing notifies a reader of it, and it copies in bounded steps so a chatty gateway cannot make the reader hold the whole log. A session that never started a gateway has no log, and saying so is the answer to what was asked rather than a failure to answer it. Assisted-by: Claude Opus 5 Signed-off-by: Paulo Gomes Entire-Checkpoint: 01M25TRK55EZ9D3Q8SB0701QFB --- cmd/cli/gateway.go | 54 ++++++++++ internal/files/files.go | 12 +++ internal/gateway/logs.go | 177 ++++++++++++++++++++++++++++++ internal/gateway/logs_test.go | 196 ++++++++++++++++++++++++++++++++++ internal/gateway/run.go | 29 ++++- 5 files changed, 464 insertions(+), 4 deletions(-) create mode 100644 internal/gateway/logs.go create mode 100644 internal/gateway/logs_test.go diff --git a/cmd/cli/gateway.go b/cmd/cli/gateway.go index 06ac433..a0a83f3 100644 --- a/cmd/cli/gateway.go +++ b/cmd/cli/gateway.go @@ -5,6 +5,7 @@ import ( "fmt" "os" + "github.com/qubesome/cli/internal/files" "github.com/qubesome/cli/internal/gateway" "github.com/qubesome/cli/internal/session" "github.com/urfave/cli/v3" @@ -23,6 +24,11 @@ import ( // Neither subcommand takes a profile. There is one gateway per session and // not one per profile, because the policy it applies is keyed by workload // across every profile. +var ( + logsFollow bool + logsLast int +) + func gatewayCommand() *cli.Command { cmd := &cli.Command{ Name: "gateway", @@ -33,6 +39,7 @@ so this is for the cases where that is not enough: qubesome gateway status - Report what qubesome knows about the session's gateway qubesome gateway stop - Stop the session's gateway, leaving the session itself up +qubesome gateway logs - Show what the session's gateway has said A running gateway is reused whatever image it came from, so a change to the gateway block of the config reaches nothing until it is stopped. The @@ -41,6 +48,7 @@ next launch then starts a fresh one inside the same session. Commands: []*cli.Command{ gatewayStatusCommand(), gatewayStopCommand(), + gatewayLogsCommand(), }, } return cmd @@ -89,3 +97,49 @@ func gatewayStopCommand() *cli.Command { }, } } + +// gatewayLogsCommand shows what the gateway has said. +// +// The gateway is started by whichever qubesome run found none running, and +// it is put in a session of its own so that a Ctrl-C at that terminal does +// not take the session's egress with it. Its output has nowhere to go that +// anybody is still watching, so it is written to a file, and this is how it +// is read back. It is the record of which host a workload was allowed or +// refused, which is the one thing needed when a workload cannot reach +// something it should. +func gatewayLogsCommand() *cli.Command { + return &cli.Command{ + Name: "logs", + Usage: "show the session gateway's logs", + Description: `Examples: + +qubesome gateway logs - Print the log of the gateway this session is running +qubesome gateway logs -n 50 - Print its last 50 lines +qubesome gateway logs -f - Print it and keep printing what is added + +The log covers the gateway that is running. Starting a gateway begins it +afresh, so there is nothing here for a session that has not started one. +`, + Flags: []cli.Flag{ + &cli.BoolFlag{ + Name: "follow", + Aliases: []string{"f"}, + Usage: "keep printing what is added to the log", + Destination: &logsFollow, + }, + &cli.IntFlag{ + Name: "lines", + Aliases: []string{"n"}, + Usage: "print only this many of the log's last lines", + Destination: &logsLast, + }, + }, + Action: func(ctx context.Context, _ *cli.Command) error { + return gateway.ShowLogs(ctx, os.Stdout, gateway.LogOptions{ + Path: files.GatewayLogPath(), + Last: logsLast, + Follow: logsFollow, + }) + }, + } +} diff --git a/internal/files/files.go b/internal/files/files.go index 6dba4b1..5c26134 100644 --- a/internal/files/files.go +++ b/internal/files/files.go @@ -245,6 +245,18 @@ func GatewayStatePath() string { return filepath.Join(SessionDir(), "sandbox-gateway.json") } +// GatewayLogPath returns where the session's gateway writes what it says. +// +// The gateway is started by whichever launch found none running, and it is +// put in a session of its own so that a Ctrl-C at that terminal does not +// take the session's egress with it. Its stdout is therefore a terminal +// nobody is necessarily still watching, and the audit line behind a denied +// connection is the one thing a user needs when a workload cannot reach +// something. It goes here instead, and qubesome gateway logs reads it back. +func GatewayLogPath() string { + return filepath.Join(SessionDir(), "gateway.log") +} + // GatewaySocketDir returns the host directory the gateway's control socket // is created in. // diff --git a/internal/gateway/logs.go b/internal/gateway/logs.go new file mode 100644 index 0000000..54444bd --- /dev/null +++ b/internal/gateway/logs.go @@ -0,0 +1,177 @@ +package gateway + +import ( + "bytes" + "context" + "errors" + "fmt" + "io" + "os" + "time" + + "github.com/qubesome/cli/internal/files" +) + +// followInterval is how often a follow looks for more of the log. +// +// The gateway writes through a pipe to a file and nothing notifies a reader +// of it, so this is a poll. It is short enough that a decision shows up +// while the workload that caused it is still on screen, and long enough +// that watching an idle gateway is not a busy loop. +const followInterval = 200 * time.Millisecond + +// followChunk bounds a single read while following, so one write cannot +// make the reader hold the whole of a chatty log in memory. +const followChunk = 32 * 1024 + +// LogOptions selects what ShowLogs prints. +type LogOptions struct { + // Path is the log to read. Empty means the session's own gateway log. + Path string + + // Last is how many of the log's final lines to print. Zero prints all + // of it. + Last int + + // Follow keeps printing what is appended, until the context is done. + Follow bool +} + +func (o LogOptions) path() string { + if o.Path != "" { + return o.Path + } + + return files.GatewayLogPath() +} + +// ShowLogs writes the session gateway's log to w. +// +// The gateway is started by whichever qubesome run found none already +// running, and it is put in a session of its own so that a Ctrl-C at that +// terminal does not take the session's egress away with it. Its output +// therefore has nowhere to go that anybody could still be looking at, which +// is why it is written to a file and read back here. +func ShowLogs(ctx context.Context, w io.Writer, opts LogOptions) error { + path := opts.path() + + f, err := os.Open(path) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + return fmt.Errorf("there is no gateway log at %s: this session has not started a gateway", path) + } + + return fmt.Errorf("failed to open the gateway log %q: %w", path, err) + } + defer f.Close() + + if err := writeTail(w, f, opts.Last); err != nil { + return err + } + + if !opts.Follow { + return nil + } + + return follow(ctx, w, f) +} + +// writeTail copies the log to w, from the start or from the last lines of +// it. The file is left positioned at its end either way, which is where a +// follow carries on from. +func writeTail(w io.Writer, f *os.File, last int) error { + if last <= 0 { + if _, err := io.Copy(w, f); err != nil { + return fmt.Errorf("failed to read the gateway log: %w", err) + } + + return nil + } + + // Read the whole file to find where its last lines begin. A gateway log + // is bounded by the life of one gateway rather than of the session, so + // this is a file a terminal was going to be shown anyway. Seeking + // backwards in chunks would be the answer if that stopped being true. + body, err := io.ReadAll(f) + if err != nil { + return fmt.Errorf("failed to read the gateway log: %w", err) + } + + if _, err := w.Write(tail(body, last)); err != nil { + return fmt.Errorf("failed to write the gateway log: %w", err) + } + + return nil +} + +// tail returns the last n lines of body. +func tail(body []byte, n int) []byte { + // A trailing newline ends the last line rather than starting another, + // so it is not one of the separators being counted back through. + end := len(bytes.TrimSuffix(body, []byte("\n"))) + + for range n { + i := bytes.LastIndexByte(body[:end], '\n') + if i < 0 { + return body + } + end = i + } + + return body[end+1:] +} + +// follow prints what is appended to f until ctx is done. +func follow(ctx context.Context, w io.Writer, f *os.File) error { + ticker := time.NewTicker(followInterval) + defer ticker.Stop() + + for { + select { + case <-ctx.Done(): + // The only way a follow ends is by being asked to stop, so + // that is not an error to report. + return nil + case <-ticker.C: + // Copied in bounded steps rather than to EOF in one call, so a + // gateway writing faster than this reads cannot keep it here. + if _, err := io.CopyN(w, f, followChunk); err != nil && !errors.Is(err, io.EOF) { + return fmt.Errorf("failed to read the gateway log: %w", err) + } + } + } +} + +// appendLog opens the gateway's log to add to what is already there. +// +// It is what the uplink writes through. The uplink is started once the +// gateway sandbox is up, so the log it joins is the one that sandbox's +// launch has already begun, and truncating here would throw away the lines +// explaining a gateway that had trouble coming up. +func appendLog(path string) (*os.File, error) { + f, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_APPEND, files.FileMode) + if err != nil { + return nil, fmt.Errorf("failed to open the gateway log %q: %w", path, err) + } + + return f, nil +} + +// openLog opens the gateway's log for the sandbox to write to. +// +// It is truncated rather than appended to. The log is the record of the +// gateway that is running, and one gateway replaces another only by the +// first having gone, so carrying the old one's lines forward would only +// make it harder to tell which of them explains what is happening now. +// +// 0600 because a gateway log holds every name a workload asked for and +// every host it was allowed or refused, which is a record of what the user +// was doing. +func openLog(path string) (*os.File, error) { + f, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, files.FileMode) + if err != nil { + return nil, fmt.Errorf("failed to open the gateway log %q: %w", path, err) + } + + return f, nil +} diff --git a/internal/gateway/logs_test.go b/internal/gateway/logs_test.go new file mode 100644 index 0000000..ebedc9e --- /dev/null +++ b/internal/gateway/logs_test.go @@ -0,0 +1,196 @@ +package gateway + +import ( + "bytes" + "context" + "os" + "path/filepath" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func writeLog(t *testing.T, lines ...string) string { + t.Helper() + + path := filepath.Join(t.TempDir(), "gateway.log") + var body []byte + for _, l := range lines { + body = append(body, l...) + body = append(body, '\n') + } + require.NoError(t, os.WriteFile(path, body, 0o600)) + + return path +} + +func TestShowLogs(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + lines []string + last int + want string + }{ + { + name: "the whole log", + lines: []string{"one", "two", "three"}, + want: "one\ntwo\nthree\n", + }, + { + name: "the last lines", + lines: []string{"one", "two", "three"}, + last: 2, + want: "two\nthree\n", + }, + { + name: "more lines than the log holds", + lines: []string{"one"}, + last: 10, + want: "one\n", + }, + { + name: "an empty log", + lines: nil, + want: "", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + var out bytes.Buffer + require.NoError(t, ShowLogs(t.Context(), &out, LogOptions{ + Path: writeLog(t, tc.lines...), + Last: tc.last, + })) + assert.Equal(t, tc.want, out.String()) + }) + } +} + +// A session with no gateway has no log, and saying so is not a failure of +// the command. It is the answer to what was asked. +func TestShowLogsWithoutALog(t *testing.T) { + t.Parallel() + + var out bytes.Buffer + err := ShowLogs(t.Context(), &out, LogOptions{ + Path: filepath.Join(t.TempDir(), "absent.log"), + }) + + require.Error(t, err) + assert.Contains(t, err.Error(), "no gateway log") + assert.Empty(t, out.String()) +} + +// syncBuffer is a bytes.Buffer the follower writes to while the test reads. +type syncBuffer struct { + mu sync.Mutex + buf bytes.Buffer +} + +func (b *syncBuffer) Write(p []byte) (int, error) { + b.mu.Lock() + defer b.mu.Unlock() + + return b.buf.Write(p) +} + +func (b *syncBuffer) String() string { + b.mu.Lock() + defer b.mu.Unlock() + + return b.buf.String() +} + +func TestShowLogsFollows(t *testing.T) { + t.Parallel() + + path := writeLog(t, "first") + + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + + out := &syncBuffer{} + done := make(chan error, 1) + go func() { + done <- ShowLogs(ctx, out, LogOptions{Path: path, Follow: true}) + }() + + require.EventuallyWithT(t, func(c *assert.CollectT) { + assert.Equal(c, "first\n", out.String()) + }, time.Second, 10*time.Millisecond, "what the log already held must be printed") + + f, err := os.OpenFile(path, os.O_APPEND|os.O_WRONLY, 0o600) + require.NoError(t, err) + _, err = f.WriteString("second\n") + require.NoError(t, err) + require.NoError(t, f.Close()) + + require.EventuallyWithT(t, func(c *assert.CollectT) { + assert.Equal(c, "first\nsecond\n", out.String()) + }, time.Second, 10*time.Millisecond, "a line appended while following must be printed") + + cancel() + require.NoError(t, <-done, "a cancelled follow is how the command ends, not a failure") +} + +// The log describes the gateway that is running, so starting one begins the +// log afresh rather than adding to what the last one said. +func TestOpenLogTruncates(t *testing.T) { + t.Parallel() + + path := writeLog(t, "what the last gateway said") + + f, err := openLog(path) + require.NoError(t, err) + defer f.Close() + + _, err = f.WriteString("what this one says\n") + require.NoError(t, err) + + body, err := os.ReadFile(path) + require.NoError(t, err) + assert.Equal(t, "what this one says\n", string(body)) +} + +func TestOpenLogIsPrivate(t *testing.T) { + t.Parallel() + + path := filepath.Join(t.TempDir(), "gateway.log") + + f, err := openLog(path) + require.NoError(t, err) + require.NoError(t, f.Close()) + + st, err := os.Stat(path) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o600), st.Mode().Perm(), + "the log carries the names a workload asked for, so it is the user's alone") +} + +// The uplink joins the log the gateway sandbox's launch already began, +// rather than starting it over and dropping what explains a gateway that +// had trouble coming up. +func TestAppendLogKeepsWhatIsThere(t *testing.T) { + t.Parallel() + + path := writeLog(t, "the gateway is starting") + + f, err := appendLog(path) + require.NoError(t, err) + defer f.Close() + + _, err = f.WriteString("the uplink is up\n") + require.NoError(t, err) + + body, err := os.ReadFile(path) + require.NoError(t, err) + assert.Equal(t, "the gateway is starting\nthe uplink is up\n", string(body)) +} diff --git a/internal/gateway/run.go b/internal/gateway/run.go index 1c626a0..c51e3fd 100644 --- a/internal/gateway/run.go +++ b/internal/gateway/run.go @@ -394,8 +394,20 @@ func (g Gateway) launch(bundle images.Bundle, spec sandbox.Spec) error { // session whose gateway ended at the first Ctrl-C would take the egress // of every workload still running with it. cmd.SysProcAttr = &syscall.SysProcAttr{Setsid: true} - cmd.Stdout = os.Stdout - cmd.Stderr = os.Stderr + + // Not this process's stdout. The gateway outlives the launch, so what + // it says would go to a terminal that is not necessarily still there, + // interleaved with the output of the workload that happened to start + // it. qubesome gateway logs reads this back. + log, err := openLog(files.GatewayLogPath()) + if err != nil { + return err + } + // The sandbox has its own copy once it is started, and a launch that + // never got that far has nothing to write here either. + defer log.Close() + cmd.Stdout = log + cmd.Stderr = log err = cmd.Start() @@ -691,8 +703,17 @@ func (h helper) start() (*execabs.Cmd, error) { // Ctrl-C at the terminal that started a workload is not a request to // take it away. cmd.SysProcAttr = &syscall.SysProcAttr{Setsid: true} - cmd.Stdout = os.Stdout - cmd.Stderr = os.Stderr + + // The gateway's log, which the sandbox's launch has already begun by + // the time an uplink is put in its namespace. It goes there for the + // reason the gateway's own output does: it outlives the launch. + log, err := appendLog(files.GatewayLogPath()) + if err != nil { + return nil, err + } + defer log.Close() + cmd.Stdout = log + cmd.Stderr = log if err := cmd.Start(); err != nil { return nil, err From b68f9443ad00ac5d4f49463e716b805540b5c9aa Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Thu, 10 Sep 2026 20:40:47 +0100 Subject: [PATCH 06/16] host-run: leave the launching session's variables behind An application launched with host-run opened on the profile's default workspace rather than the one being looked at. host-run used to give the command DISPLAY and nothing else. It now hands over the whole host environment, which is right for HOME and PATH and wrong for the handful of variables that name the session the command was typed or bound in, because the session it is about to appear in is a different one. DESKTOP_STARTUP_ID is the one that moved the window. A window manager that spawns through startup notification records the workspace it spawned from against the id it exports, and the application passes that id to the next window it opens as _NET_STARTUP_ID. The profile's window manager then reads an id for a launch it never saw and places the window by whatever that resolves to. It is single use as well: it belongs to the launch that created it and to no later one, which is why awesome unsets it itself when it spawns without a context of its own. XDG_ACTIVATION_TOKEN is the Wayland spelling of the same thing and goes with it. WAYLAND_DISPLAY was already being dropped, for a reason that is the same one told differently, so the three are now one list with the reason written once. Assisted-by: Claude Opus 5 Signed-off-by: Paulo Gomes Entire-Checkpoint: 01M26DCGXTE5NCSVHSXA82MPNR --- cmd/cli/host_run.go | 49 ++++++++++++++++++++++++++++++++++++---- cmd/cli/host_run_test.go | 32 ++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 5 deletions(-) diff --git a/cmd/cli/host_run.go b/cmd/cli/host_run.go index 8d56fa4..4a7db62 100644 --- a/cmd/cli/host_run.go +++ b/cmd/cli/host_run.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "os/exec" + "slices" "strconv" "strings" @@ -26,10 +27,9 @@ import ( // and the profile's X server refuses a connection that arrives without it, // saying only that no authorization protocol was specified. // -// WAYLAND_DISPLAY is dropped because a toolkit that finds one connects to -// it and ignores DISPLAY, which would open the window on the host desktop -// rather than in the profile. The profile's own window manager is started -// without it for the same reason. +// What is dropped is the set of variables naming the session the command +// was launched from, which is not the session it is about to appear in. +// See launchingSession. // // The two entries are appended rather than substituted for the inherited // ones. os/exec keeps the last value of a repeated key, so these are the @@ -37,7 +37,7 @@ import ( func hostRunEnv(base []string, display uint8, cookie string) []string { env := make([]string, 0, len(base)+2) for _, e := range base { - if strings.HasPrefix(e, "WAYLAND_DISPLAY=") { + if namesTheLaunchingSession(e) { continue } env = append(env, e) @@ -49,6 +49,45 @@ func hostRunEnv(base []string, display uint8, cookie string) []string { ) } +// launchingSession are the variables describing the display session the +// command was typed or bound in, rather than the profile it is being sent +// to. Each one is a handle on the host session, and the command is about +// to connect to a different display server, so each is either ignored +// there or acted on as if it meant something. +// +// WAYLAND_DISPLAY is a path to the host compositor. A toolkit that finds +// one connects to it and ignores DISPLAY, opening the window on the host +// desktop rather than in the profile. The profile's own window manager is +// started without it for the same reason. +// +// DESKTOP_STARTUP_ID is an X11 startup notification handed out by +// whatever launched qubesome. A window manager that spawns through +// startup notification records the workspace it spawned from against that +// id, and the application exports it to the next window it opens as +// _NET_STARTUP_ID. The profile's window manager then reads an id for a +// launch it never saw, and places the window by what that resolves to +// rather than on the workspace being looked at. It is also single use: it +// belongs to the launch that created it and to no later one. +// +// XDG_ACTIVATION_TOKEN is the Wayland spelling of the same thing, with +// the same two problems. +var launchingSession = []string{ + "WAYLAND_DISPLAY", + "DESKTOP_STARTUP_ID", + "XDG_ACTIVATION_TOKEN", +} + +// namesTheLaunchingSession reports whether an environment entry is one of +// launchingSession. +func namesTheLaunchingSession(entry string) bool { + name, _, ok := strings.Cut(entry, "=") + if !ok { + return false + } + + return slices.Contains(launchingSession, name) +} + func hostRunCommand() *cli.Command { cmd := &cli.Command{ Name: "host-run", diff --git a/cmd/cli/host_run_test.go b/cmd/cli/host_run_test.go index 7a6ee7f..7558ed4 100644 --- a/cmd/cli/host_run_test.go +++ b/cmd/cli/host_run_test.go @@ -3,6 +3,7 @@ package cli import ( "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -37,3 +38,34 @@ func TestHostRunEnvDropsWaylandDisplay(t *testing.T) { require.NotContains(t, got, "WAYLAND_DISPLAY=wayland-0") require.Contains(t, got, "HOME=/home/user") } + +// A window manager that spawns through startup notification records the +// workspace it spawned from against the id it exports. Carrying that id +// into another display server hands the profile's window manager a +// sequence it never started, and the window lands wherever that resolves +// to instead of where the user is looking. +func TestHostRunEnvDropsTheLaunchingSession(t *testing.T) { + t.Parallel() + + got := hostRunEnv([]string{ + "HOME=/home/user", + "PATH=/usr/bin", + "DESKTOP_STARTUP_ID=host/awesome/1-2-3_TIME12345", + "XDG_ACTIVATION_TOKEN=abcdef", + "WAYLAND_DISPLAY=wayland-0", + }, 21, "/cookie") + + for _, unwanted := range []string{ + "DESKTOP_STARTUP_ID", + "XDG_ACTIVATION_TOKEN", + "WAYLAND_DISPLAY", + } { + for _, e := range got { + assert.NotContains(t, e, unwanted+"=", + "%s names the session the command was launched from", unwanted) + } + } + + assert.Contains(t, got, "HOME=/home/user", "the host environment is otherwise kept") + assert.Contains(t, got, "PATH=/usr/bin") +} From b441a6648604d819a09397b5191e269b953778e9 Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Thu, 10 Sep 2026 20:42:01 +0100 Subject: [PATCH 07/16] xkb: prefer the running layout on an X11 session A profile came up on a layout its user does not type on, again. Asking localectl first was right for the host it was written for and wrong for the one it broke. The two tools disagree more often than they look like they should, and which of them to believe depends on where the question is asked, not on a preference between them. On X11 the running answer wins. setxkbmap asks the X server the user is typing on, which includes a layout applied by hand after login, and that is the layout the profile should come up on. localectl reports what was configured, which on a host whose layout is set at runtime is a layout nobody is using. On Wayland the configured answer wins, which is what the previous behaviour was reaching for. There setxkbmap reaches Xwayland, which carries its own default rather than the compositor's keymap, so it answers confidently with nobody's layout. An unset session type is treated as X11: every X11 session sets it, and a session that sets nothing is not a Wayland one. XKB_DEFAULT_ still wins over both. The two tests that asserted the old order called firstOf directly with a fixed order, under names describing a policy that is now session dependent. What replaces them drives the decision itself, on both sessions, and covers the fallback in both directions. Assisted-by: Claude Opus 5 Signed-off-by: Paulo Gomes Entire-Checkpoint: 01M26DERTN4KQDGNQD5BQZFBA4 --- internal/util/xkb/xkb.go | 53 +++++++++++++++++++--------- internal/util/xkb/xkb_test.go | 65 ++++++++++++++++++++++++----------- 2 files changed, 80 insertions(+), 38 deletions(-) diff --git a/internal/util/xkb/xkb.go b/internal/util/xkb/xkb.go index eda1cf8..4fbfcf4 100644 --- a/internal/util/xkb/xkb.go +++ b/internal/util/xkb/xkb.go @@ -45,21 +45,36 @@ func Defaults() []string { return env } - // localectl is asked first, on any session. It reports the configured - // layout, which is the one the user chose. setxkbmap reports what the - // running X server happens to hold, and the two disagree more often - // than they look like they should. - // - // Both cases were seen on real hosts. On a Wayland session setxkbmap - // asks Xwayland, which carries its own default rather than the - // compositor's keymap. On an X11 host whose localectl said gb with a - // microsoftpro model, setxkbmap still answered us and pc105, and the - // profile faithfully reproduced a layout its user does not type on. - // Preferring the configured answer is right in both. - // - // A session that really does want the live value sets XKB_DEFAULT_ - // above, which still wins over both. - return firstOf(fromLocalectl(localectlQuery), fromSetxkbmap(setxkbmapQuery)) + return preferred(os.Getenv("XDG_SESSION_TYPE"), setxkbmapQuery, localectlQuery) +} + +// preferred returns the keymap of the session, asking the two tools in the +// order that session makes reliable and falling back to the other. +// +// The two disagree more often than they look like they should, and which +// one is right depends on where it is asked. +// +// On X11 the live answer wins. setxkbmap asks the running X server, which +// is the keyboard the user is typing on, including a layout applied by +// hand after login. localectl reports what was configured, which on a host +// whose layout is set at runtime is a layout its user does not type on. +// +// On Wayland the configured answer wins. There setxkbmap reaches Xwayland, +// which carries its own default rather than the compositor's keymap, so it +// answers confidently with nobody's layout. localectl reports what the +// compositor built its keymap from. +// +// An unset session type is treated as X11. Every X11 session sets it, and +// a session that sets nothing is not a Wayland one. +// +// A host that wants neither answer sets XKB_DEFAULT_ itself, which wins +// over both. +func preferred(session string, live, configured query) []string { + if strings.EqualFold(session, "wayland") { + return firstOf(fromLocalectl(configured), fromSetxkbmap(live)) + } + + return firstOf(fromSetxkbmap(live), fromLocalectl(configured)) } func firstOf(sources ...[]string) []string { @@ -102,6 +117,10 @@ func fromEnv() []string { return nil } +// query reads a tool's output. It is what makes the two sources a seam a +// test can drive, and it is the same shape for both. +type query func() ([]byte, error) + func setxkbmapQuery() ([]byte, error) { //nolint:gosec // G204: the binary is a fixed path and the argument is a literal. return execabs.Command(files.SetxkbmapBinary, "-query").Output() @@ -125,7 +144,7 @@ var localectlFields = map[string]string{ // fromLocalectl reads the configured layout, which is what a Wayland // compositor builds its keymap from and what Xwayland does not report. -func fromLocalectl(q func() ([]byte, error)) []string { +func fromLocalectl(q query) []string { out, err := q() if err != nil { slog.Debug("cannot read the configured keyboard layout", "error", err) @@ -150,7 +169,7 @@ func fromLocalectl(q func() ([]byte, error)) []string { // fromSetxkbmap parses setxkbmap -query, which prints one "key: value" // per line and omits nothing, printing an empty value for a component // that is not set. -func fromSetxkbmap(q func() ([]byte, error)) []string { +func fromSetxkbmap(q query) []string { out, err := q() if err != nil { slog.Debug("cannot read the host keyboard layout", "error", err) diff --git a/internal/util/xkb/xkb_test.go b/internal/util/xkb/xkb_test.go index 3da9388..a669b0d 100644 --- a/internal/util/xkb/xkb_test.go +++ b/internal/util/xkb/xkb_test.go @@ -145,28 +145,51 @@ func TestFirstOfTakesTheFirstThatAnswered(t *testing.T) { require.Empty(t, firstOf(nil, nil)) } -// A real X11 host reported gb and microsoftpro from localectl while -// setxkbmap answered us and pc105. The configured layout is the one its -// user types on, so it is the one preferred, on any session. -func TestDefaultsPrefersTheConfiguredLayout(t *testing.T) { - got := firstOf( - fromLocalectl(func() ([]byte, error) { - return []byte(" X11 Layout: gb\n X11 Model: microsoftpro\n"), nil - }), - fromSetxkbmap(func() ([]byte, error) { - return []byte("layout: us\nmodel: pc105\n"), nil - }), - ) - - require.Equal(t, []string{"XKB_DEFAULT_MODEL=microsoftpro", "XKB_DEFAULT_LAYOUT=gb"}, got) +// Which of the two answers to believe depends on the session, because the +// two tools are reliable on opposite ones. On X11 setxkbmap reports the +// keymap the user is typing on, including one applied by hand after login. +// On Wayland it reports Xwayland's own default, which is nobody's layout. +func TestPreferredSource(t *testing.T) { + t.Parallel() + + const live = "rules: evdev\nmodel: pc105\nlayout: gb\n" + const configured = "X11 Layout: us\nX11 Model: pc104\n" + + tests := []struct { + name string + session string + want string + }{ + {"x11 prefers the running layout", "x11", "XKB_DEFAULT_LAYOUT=gb"}, + {"a session that says nothing is treated as x11", "", "XKB_DEFAULT_LAYOUT=gb"}, + {"wayland prefers the configured layout", "wayland", "XKB_DEFAULT_LAYOUT=us"}, + {"wayland is matched whatever its case", "Wayland", "XKB_DEFAULT_LAYOUT=us"}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + got := preferred(tc.session, + func() ([]byte, error) { return []byte(live), nil }, + func() ([]byte, error) { return []byte(configured), nil }, + ) + + require.Contains(t, got, tc.want) + }) + } } -// A host with no localectl still gets the running layout. -func TestDefaultsFallsBackToTheRunningLayout(t *testing.T) { - got := firstOf( - fromLocalectl(func() ([]byte, error) { return nil, errors.New("not found") }), - fromSetxkbmap(func() ([]byte, error) { return []byte("layout: us\n"), nil }), - ) +// Whichever is preferred, the other still answers when the first cannot. +func TestPreferredFallsBack(t *testing.T) { + t.Parallel() + + const configured = "X11 Layout: us\n" + fails := func() ([]byte, error) { return nil, errors.New("not found") } + + got := preferred("x11", fails, func() ([]byte, error) { return []byte(configured), nil }) + require.Contains(t, got, "XKB_DEFAULT_LAYOUT=us") - require.Equal(t, []string{"XKB_DEFAULT_LAYOUT=us"}, got) + got = preferred("wayland", func() ([]byte, error) { return []byte("layout: gb\n"), nil }, fails) + require.Contains(t, got, "XKB_DEFAULT_LAYOUT=gb") } From 7286676fc6e0a97e8df041ee71dce05fce9488b8 Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Thu, 10 Sep 2026 20:42:59 +0100 Subject: [PATCH 08/16] profiles: name the profile's display for the window manager The window manager was given the profile's cookie and had WAYLAND_DISPLAY taken away from it, and was then left to whatever DISPLAY xwayland-run exports. The profile container's own DISPLAY is the host's, because that is what the compositor presents through, and the whole of /tmp/.X11-unix is mounted so that its socket is reachable. Anything under the window manager that inherited that would be talking to the host session rather than to the profile. It is named here instead, beside the two that were already being set, so nothing in that subtree can reach the host's X server by inheriting a path to it. Workloads were never exposed this way: each mounts only its own socket rather than the directory. Assisted-by: Claude Opus 5 Signed-off-by: Paulo Gomes Entire-Checkpoint: 01M26DGJC18YYEP4XZ3G07QR7R --- internal/profiles/display.go | 8 ++++++++ internal/profiles/display_test.go | 34 +++++++++++++++++++++++++++++-- 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/internal/profiles/display.go b/internal/profiles/display.go index d1ef6b9..ca5223d 100644 --- a/internal/profiles/display.go +++ b/internal/profiles/display.go @@ -136,8 +136,16 @@ func xwaylandArgs(p DisplayParams) ([]string, error) { // The window manager, and everything it launches, must not inherit a // path to the compositor. A client that reaches the Wayland socket // bypasses Xwayland and the isolation set above. + // + // DISPLAY names this profile's server rather than being left to what + // xwayland-run exports. The profile container's own DISPLAY is the + // host's, because that is what the compositor presents through, and + // the whole of /tmp/.X11-unix is mounted so that its socket is + // reachable. Anything under here that inherited that would be talking + // to the host session instead of to the profile. args = append(args, "--", "env", "-u", "WAYLAND_DISPLAY", + "DISPLAY=:"+strconv.Itoa(int(p.Display)), "XDG_RUNTIME_DIR="+appRuntimeDir, "XAUTHORITY="+clientAuthFile) diff --git a/internal/profiles/display_test.go b/internal/profiles/display_test.go index 3e30a2b..82059be 100644 --- a/internal/profiles/display_test.go +++ b/internal/profiles/display_test.go @@ -6,6 +6,7 @@ import ( "net" "os" "path/filepath" + "slices" "testing" "time" @@ -80,7 +81,7 @@ func TestXwaylandArgs(t *testing.T) { "-tst", "-nolisten", "tcp", "--", - "env", "-u", "WAYLAND_DISPLAY", + "env", "-u", "WAYLAND_DISPLAY", "DISPLAY=:11", "XDG_RUNTIME_DIR=/run/user/1000", "XAUTHORITY=/home/xorg-user/.Xauthority", "dbus-run-session", "awesome", @@ -106,7 +107,7 @@ func TestXwaylandArgs(t *testing.T) { "-nolisten", "tcp", "-verbose", "9", "--", - "env", "-u", "WAYLAND_DISPLAY", + "env", "-u", "WAYLAND_DISPLAY", "DISPLAY=:11", "XDG_RUNTIME_DIR=/run/user/1000", "XAUTHORITY=/home/xorg-user/.Xauthority", "dbus-run-session", "awesome", @@ -314,3 +315,32 @@ func TestCompositorStatusDistinguishesACleanExitFromStillRunning(t *testing.T) { require.Len(t, exit, 1) }) } + +// The profile container reaches the host X server: its DISPLAY names the +// host session and the whole of /tmp/.X11-unix is mounted, which is how +// the compositor presents the profile at all. The window manager must not +// inherit that. It is given the profile's own display by name rather than +// left to whatever xwayland-run happens to export, for the same reason it +// is given the profile's cookie and has WAYLAND_DISPLAY taken away. +func TestXwaylandArgsNamesTheProfileDisplay(t *testing.T) { + t.Parallel() + + got, err := xwaylandArgs(DisplayParams{ + Display: 11, + Geometry: "1920x1080", + AuthFile: "/home/xorg-user/.Xserver", + WindowManager: "exec awesome", + }) + require.NoError(t, err) + + // After the -- separator, so it is the window manager's environment + // and not an argument to Xwayland. + sep := slices.Index(got, "--") + require.NotEqual(t, -1, sep, "the window manager must be separated from the server arguments") + require.Contains(t, got[sep:], "DISPLAY=:11") + + wm := slices.Index(got, "awesome") + require.NotEqual(t, -1, wm, "the window manager must still be run") + require.Less(t, slices.Index(got, "DISPLAY=:11"), wm, + "the display has to be set before the command, or env takes it as an argument") +} From 2214fdc167bf7b3798eb11c35cd14d1744c05284 Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Thu, 10 Sep 2026 20:44:14 +0100 Subject: [PATCH 09/16] gateway: narrow the logs to one profile or one workload A session gateway serves every workload of every profile, so its log is every decision made for all of them interleaved. Finding why one workload could not reach something meant reading past everything else. -profile and -workload narrow it. The gateway knows a workload only by the name qubesome registered, which is the workload's own name and its profile's joined by a dash, and either half may hold a dash of its own, so the pair cannot be split back into two names with any certainty. Naming both is therefore the exact question and naming one alone is a prefix or a suffix of it, which is spelled out in the command's help rather than left to surprise someone. -n now counts the lines it shows rather than the lines in the file. Asking for one workload's last twenty otherwise gives however few of its lines happen to fall in the file's last twenty. Reading the log is line oriented rather than a byte copy, which is what a filter needs. A follow holds a line back until it has a newline on it: a line is what carries the workload a decision was about, so half of one cannot be matched, and printing it unmatched would show another workload's log to someone who asked not to see it. Assisted-by: Claude Opus 5 Signed-off-by: Paulo Gomes Entire-Checkpoint: 01M26DJVEWJG0V1SSAY0QJD1CM --- cmd/cli/gateway.go | 41 ++++++-- internal/gateway/logline.go | 125 +++++++++++++++++++++++ internal/gateway/logline_test.go | 87 ++++++++++++++++ internal/gateway/logs.go | 170 ++++++++++++++++++++++--------- internal/gateway/logs_test.go | 126 +++++++++++++++++++++++ 5 files changed, 490 insertions(+), 59 deletions(-) create mode 100644 internal/gateway/logline.go create mode 100644 internal/gateway/logline_test.go diff --git a/cmd/cli/gateway.go b/cmd/cli/gateway.go index a0a83f3..4ff1958 100644 --- a/cmd/cli/gateway.go +++ b/cmd/cli/gateway.go @@ -25,8 +25,9 @@ import ( // not one per profile, because the policy it applies is keyed by workload // across every profile. var ( - logsFollow bool - logsLast int + logsFollow bool + logsLast int + logsWorkload string ) func gatewayCommand() *cli.Command { @@ -113,12 +114,22 @@ func gatewayLogsCommand() *cli.Command { Usage: "show the session gateway's logs", Description: `Examples: -qubesome gateway logs - Print the log of the gateway this session is running -qubesome gateway logs -n 50 - Print its last 50 lines -qubesome gateway logs -f - Print it and keep printing what is added +qubesome gateway logs - Print the log of the gateway this session is running +qubesome gateway logs -n 50 - Print its last 50 lines +qubesome gateway logs -f - Print it and keep printing what is added +qubesome gateway logs -profile work - Only the lines about that profile's workloads +qubesome gateway logs -workload chrome - Only the lines about that workload, in any profile +qubesome gateway logs -profile work -workload chrome + - Only the lines about that one workload The log covers the gateway that is running. Starting a gateway begins it afresh, so there is nothing here for a session that has not started one. + +The gateway knows a workload as its name and its profile's joined by a +dash, and either half may hold a dash of its own, so naming only one of +the two matches the other loosely. Naming both is exact. A line about no +workload, such as the gateway's own startup, is not shown when either +filter is given. `, Flags: []cli.Flag{ &cli.BoolFlag{ @@ -127,18 +138,30 @@ afresh, so there is nothing here for a session that has not started one. Usage: "keep printing what is added to the log", Destination: &logsFollow, }, + &cli.StringFlag{ + Name: "profile", + Usage: "only the lines about the workloads of this profile", + Destination: &targetProfile, + }, + &cli.StringFlag{ + Name: "workload", + Usage: "only the lines about this workload", + Destination: &logsWorkload, + }, &cli.IntFlag{ Name: "lines", Aliases: []string{"n"}, - Usage: "print only this many of the log's last lines", + Usage: "print only this many of the log's last matching lines", Destination: &logsLast, }, }, Action: func(ctx context.Context, _ *cli.Command) error { return gateway.ShowLogs(ctx, os.Stdout, gateway.LogOptions{ - Path: files.GatewayLogPath(), - Last: logsLast, - Follow: logsFollow, + Path: files.GatewayLogPath(), + Profile: targetProfile, + Workload: logsWorkload, + Last: logsLast, + Follow: logsFollow, }) }, } diff --git a/internal/gateway/logline.go b/internal/gateway/logline.go new file mode 100644 index 0000000..29284a1 --- /dev/null +++ b/internal/gateway/logline.go @@ -0,0 +1,125 @@ +package gateway + +import "strings" + +// The gateway writes its log with slog's text handler, so a line is a +// sequence of key=value pairs and a value holding a space is quoted. This +// reads that, tolerantly: a line it cannot make sense of yields nothing +// rather than an error, because the gateway's log format is the gateway's +// to change and a reader of it should degrade to showing the line as it +// is rather than refusing to show anything. +// +// The keys qubesome reads are the ones the gateway's audit line carries. +const ( + // fieldWorkload is the name qubesome registered the workload under. + fieldWorkload = "workload" + + // fieldLevel is the slog level, which is how a line reporting a + // malfunction is told from one reporting a decision. + fieldLevel = "level" + + // fieldAction is the proxy's verdict: deny, splice or inject. + fieldAction = "action" + + // fieldHost is the host a decision was about. + fieldHost = "host" +) + +// logField returns the value of key in a log line, and "" when the line +// does not carry it. +func logField(line, key string) string { + // Anchored on a delimiter so that a key is not found inside another + // one: "load" must not match "workload=". + for i := 0; i+len(key)+1 <= len(line); i++ { + if i > 0 && line[i-1] != ' ' { + continue + } + if !strings.HasPrefix(line[i:], key+"=") { + continue + } + + return logValue(line[i+len(key)+1:]) + } + + return "" +} + +// logValue reads one value from the start of rest, which is either quoted +// or runs to the next space. +func logValue(rest string) string { + if strings.HasPrefix(rest, `"`) { + // A quoted value ends at the next quote that is not escaped. + // slog quotes a value holding a space, and escapes a quote + // within it. + var b strings.Builder + for i := 1; i < len(rest); i++ { + switch rest[i] { + case '\\': + if i+1 < len(rest) { + i++ + b.WriteByte(rest[i]) + } + case '"': + return b.String() + default: + b.WriteByte(rest[i]) + } + } + + return b.String() + } + + if i := strings.IndexByte(rest, ' '); i >= 0 { + return rest[:i] + } + + return rest +} + +// selector returns the test for whether a log line is about the workload +// being asked about. +// +// qubesome registers a workload with the gateway under its own name and +// its profile's, joined by a dash, and either half may hold a dash of its +// own. The pair therefore cannot be split back into two names with any +// certainty, so what is matched depends on how much was asked for: +// +// - both named: the registered name is exactly the two joined, which is +// the only unambiguous question of the three. +// - a profile alone: the registered name ends with it. +// - a workload alone: the registered name begins with it. +// +// A workload called "a-b" in profile "c" and one called "a" in profile +// "b-c" register the same name, and nothing here can tell them apart. +// Naming both halves is what avoids the question. +// +// A line naming no workload at all, which is what the gateway's own +// startup and shutdown lines look like, is not about the workload being +// asked about, so a filter drops it. Asking for no filter keeps +// everything. +func selector(profile, workload string) func(line string) bool { + if profile == "" && workload == "" { + return func(string) bool { return true } + } + + switch { + case profile != "" && workload != "": + want := workload + "-" + profile + + return func(line string) bool { return logField(line, fieldWorkload) == want } + + case profile != "": + suffix := "-" + profile + + return func(line string) bool { + return strings.HasSuffix(logField(line, fieldWorkload), suffix) + } + + default: + prefix := workload + "-" + + return func(line string) bool { + return strings.HasPrefix(logField(line, fieldWorkload), prefix) + } + } +} diff --git a/internal/gateway/logline_test.go b/internal/gateway/logline_test.go new file mode 100644 index 0000000..6908f45 --- /dev/null +++ b/internal/gateway/logline_test.go @@ -0,0 +1,87 @@ +package gateway + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +const decision = `time=2026-09-10T14:22:32.892Z level=INFO msg="proxy decision" ` + + `plane=proxy workload=cli-llm-work host=api.anthropic.com action=splice ` + + `reason="tunnelling TLS untouched"` + +func TestLogField(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + line string + key string + want string + }{ + {"a bare value", decision, "workload", "cli-llm-work"}, + {"the first field", decision, "time", "2026-09-10T14:22:32.892Z"}, + {"a quoted value", decision, "msg", "proxy decision"}, + {"the last field", decision, "reason", "tunnelling TLS untouched"}, + {"a key that is absent", decision, "profile", ""}, + {"a key that is only a suffix of another", decision, "load", ""}, + {"an empty line", "", "workload", ""}, + {"a value at the end with no newline", "level=INFO", "level", "INFO"}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + assert.Equal(t, tc.want, logField(tc.line, tc.key)) + }) + } +} + +// The gateway knows a workload by the name qubesome registered, which is +// the workload's own name and its profile's joined by a dash. Both halves +// may hold a dash themselves, so the pair cannot be split back apart with +// any certainty. Naming both is therefore the exact question, and naming +// one alone is a prefix or a suffix of it. +func TestSelects(t *testing.T) { + t.Parallel() + + line := func(workload string) string { + return `level=INFO msg="proxy decision" workload=` + workload + ` action=splice` + } + + tests := []struct { + name string + profile string + workload string + line string + want bool + }{ + {"no filter takes everything", "", "", line("cli-llm-work"), true}, + {"no filter takes a line naming no workload", "", "", "msg=\"starting gateway\"", true}, + + {"both, matching", "work", "cli-llm", line("cli-llm-work"), true}, + {"both, wrong profile", "personal", "cli-llm", line("cli-llm-work"), false}, + {"both, wrong workload", "work", "chrome", line("cli-llm-work"), false}, + {"both, matching only as a prefix", "work", "cli", line("cli-llm-work"), false}, + + {"profile only", "work", "", line("cli-llm-work"), true}, + {"profile only, another profile", "personal", "", line("cli-llm-work"), false}, + {"profile only, the whole name", "cli-llm-work", "", line("cli-llm-work"), false}, + + {"workload only", "", "cli-llm", line("cli-llm-work"), true}, + {"workload only, another workload", "", "chrome", line("cli-llm-work"), false}, + {"workload only, the whole name", "", "cli-llm-work", line("cli-llm-work"), false}, + + {"a filtered line naming no workload is not about it", "work", "", "msg=\"starting gateway\"", false}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + sel := selector(tc.profile, tc.workload) + assert.Equal(t, tc.want, sel(tc.line)) + }) + } +} diff --git a/internal/gateway/logs.go b/internal/gateway/logs.go index 54444bd..426d031 100644 --- a/internal/gateway/logs.go +++ b/internal/gateway/logs.go @@ -1,12 +1,13 @@ package gateway import ( - "bytes" + "bufio" "context" "errors" "fmt" "io" "os" + "strings" "time" "github.com/qubesome/cli/internal/files" @@ -14,23 +15,31 @@ import ( // followInterval is how often a follow looks for more of the log. // -// The gateway writes through a pipe to a file and nothing notifies a reader -// of it, so this is a poll. It is short enough that a decision shows up -// while the workload that caused it is still on screen, and long enough -// that watching an idle gateway is not a busy loop. +// The gateway writes through a descriptor to a file and nothing notifies a +// reader of it, so this is a poll. It is short enough that a decision +// shows up while the workload that caused it is still on screen, and long +// enough that watching an idle gateway is not a busy loop. const followInterval = 200 * time.Millisecond -// followChunk bounds a single read while following, so one write cannot -// make the reader hold the whole of a chatty log in memory. -const followChunk = 32 * 1024 +// maxLineLen bounds one log line, in the initial read and while +// following. A gateway writing without newlines cannot make a reader of +// its log hold the whole of it in memory. +const maxLineLen = 1 << 20 // LogOptions selects what ShowLogs prints. type LogOptions struct { // Path is the log to read. Empty means the session's own gateway log. Path string - // Last is how many of the log's final lines to print. Zero prints all - // of it. + // Profile and Workload narrow the log to the lines about one + // workload, one profile's workloads, or one workload wherever it + // runs. See selector for what each combination matches, and why + // naming both is the only exact question of the three. + Profile string + Workload string + + // Last is how many of the log's final matching lines to print. Zero + // prints all of them. Last int // Follow keeps printing what is appended, until the context is done. @@ -50,8 +59,8 @@ func (o LogOptions) path() string { // The gateway is started by whichever qubesome run found none already // running, and it is put in a session of its own so that a Ctrl-C at that // terminal does not take the session's egress away with it. Its output -// therefore has nowhere to go that anybody could still be looking at, which -// is why it is written to a file and read back here. +// therefore has nowhere to go that anybody could still be looking at, +// which is why it is written to a file and read back here. func ShowLogs(ctx context.Context, w io.Writer, opts LogOptions) error { path := opts.path() @@ -65,7 +74,9 @@ func ShowLogs(ctx context.Context, w io.Writer, opts LogOptions) error { } defer f.Close() - if err := writeTail(w, f, opts.Last); err != nil { + selects := selector(opts.Profile, opts.Workload) + + if err := writeTail(w, f, opts.Last, selects); err != nil { return err } @@ -73,59 +84,81 @@ func ShowLogs(ctx context.Context, w io.Writer, opts LogOptions) error { return nil } - return follow(ctx, w, f) + return follow(ctx, w, f, selects) } -// writeTail copies the log to w, from the start or from the last lines of -// it. The file is left positioned at its end either way, which is where a -// follow carries on from. -func writeTail(w io.Writer, f *os.File, last int) error { - if last <= 0 { - if _, err := io.Copy(w, f); err != nil { - return fmt.Errorf("failed to read the gateway log: %w", err) +// writeTail writes the lines of f that selects accepts: all of them, or +// only the final few when last is set. +// +// The file is left positioned at its end either way, which is where a +// follow carries on from: the scanner stops having consumed everything up +// to EOF. +func writeTail(w io.Writer, f *os.File, last int, selects func(string) bool) error { + // A ring of the last lines wanted, so a log far larger than the + // answer is not held in memory to produce it. Zero means every line + // is written as it is read and nothing is held at all. + var ring []string + if last > 0 { + ring = make([]string, 0, last) + } + + scanner := bufio.NewScanner(f) + scanner.Buffer(make([]byte, 0, bufio.MaxScanTokenSize), maxLineLen) + + for scanner.Scan() { + line := scanner.Text() + if !selects(line) { + continue } - return nil - } + if last <= 0 { + if err := writeLine(w, line); err != nil { + return err + } - // Read the whole file to find where its last lines begin. A gateway log - // is bounded by the life of one gateway rather than of the session, so - // this is a file a terminal was going to be shown anyway. Seeking - // backwards in chunks would be the answer if that stopped being true. - body, err := io.ReadAll(f) - if err != nil { + continue + } + + if len(ring) == last { + ring = append(ring[:0], ring[1:]...) + } + ring = append(ring, line) + } + if err := scanner.Err(); err != nil { return fmt.Errorf("failed to read the gateway log: %w", err) } - if _, err := w.Write(tail(body, last)); err != nil { - return fmt.Errorf("failed to write the gateway log: %w", err) + for _, line := range ring { + if err := writeLine(w, line); err != nil { + return err + } } return nil } -// tail returns the last n lines of body. -func tail(body []byte, n int) []byte { - // A trailing newline ends the last line rather than starting another, - // so it is not one of the separators being counted back through. - end := len(bytes.TrimSuffix(body, []byte("\n"))) - - for range n { - i := bytes.LastIndexByte(body[:end], '\n') - if i < 0 { - return body - } - end = i +func writeLine(w io.Writer, line string) error { + if _, err := io.WriteString(w, line+"\n"); err != nil { + return fmt.Errorf("failed to write the gateway log: %w", err) } - return body[end+1:] + return nil } -// follow prints what is appended to f until ctx is done. -func follow(ctx context.Context, w io.Writer, f *os.File) error { +// follow writes the lines appended to f that selects accepts, until ctx is +// done. +// +// Only whole lines are written. A line is what carries the workload a +// decision was about, so half of one cannot be matched against a filter, +// and printing it unmatched would show another workload's log to someone +// who asked not to see it. +func follow(ctx context.Context, w io.Writer, f *os.File, selects func(string) bool) error { ticker := time.NewTicker(followInterval) defer ticker.Stop() + reader := bufio.NewReader(f) + var pending strings.Builder + for { select { case <-ctx.Done(): @@ -133,11 +166,48 @@ func follow(ctx context.Context, w io.Writer, f *os.File) error { // that is not an error to report. return nil case <-ticker.C: - // Copied in bounded steps rather than to EOF in one call, so a - // gateway writing faster than this reads cannot keep it here. - if _, err := io.CopyN(w, f, followChunk); err != nil && !errors.Is(err, io.EOF) { - return fmt.Errorf("failed to read the gateway log: %w", err) + if err := followOnce(w, reader, &pending, selects); err != nil { + return err + } + } + } +} + +// followOnce drains what the reader can give without blocking, writing +// every whole line it completes. What is left over is kept in pending for +// the next tick, which is how a line still being written is not printed +// halfway. +func followOnce(w io.Writer, reader *bufio.Reader, pending *strings.Builder, selects func(string) bool) error { + for { + chunk, err := reader.ReadString('\n') + + // A read that stopped short of a newline is a line the gateway + // has not finished writing. It is held until it has. + if errors.Is(err, io.EOF) { + if pending.Len()+len(chunk) > maxLineLen { + pending.Reset() + + return nil } + pending.WriteString(chunk) + + return nil + } + if err != nil { + return fmt.Errorf("failed to read the gateway log: %w", err) + } + + line := strings.TrimSuffix(chunk, "\n") + if pending.Len() > 0 { + line = pending.String() + line + pending.Reset() + } + + if !selects(line) { + continue + } + if err := writeLine(w, line); err != nil { + return err } } } diff --git a/internal/gateway/logs_test.go b/internal/gateway/logs_test.go index ebedc9e..a81ac5e 100644 --- a/internal/gateway/logs_test.go +++ b/internal/gateway/logs_test.go @@ -194,3 +194,129 @@ func TestAppendLogKeepsWhatIsThere(t *testing.T) { require.NoError(t, err) assert.Equal(t, "the gateway is starting\nthe uplink is up\n", string(body)) } + +func TestShowLogsFilters(t *testing.T) { + t.Parallel() + + lines := []string{ + `msg="starting gateway"`, + `msg="proxy decision" workload=chrome-work host=a.example action=splice`, + `msg="proxy decision" workload=cli-llm-work host=b.example action=deny`, + `msg="proxy decision" workload=cli-llm-personal host=c.example action=splice`, + `msg="proxy decision" workload=chrome-personal host=d.example action=splice`, + } + + tests := []struct { + name string + profile string + workload string + want []string + }{ + { + name: "no filter shows the whole log", + want: lines, + }, + { + name: "a profile shows every workload in it", + profile: "work", + want: []string{lines[1], lines[2]}, + }, + { + name: "a workload shows it in every profile", + workload: "cli-llm", + want: []string{lines[2], lines[3]}, + }, + { + name: "both name one workload exactly", + profile: "work", + workload: "cli-llm", + want: []string{lines[2]}, + }, + { + name: "a pair that ran nothing shows nothing", + profile: "work", + workload: "obsidian", + want: nil, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + var out bytes.Buffer + require.NoError(t, ShowLogs(t.Context(), &out, LogOptions{ + Path: writeLog(t, lines...), + Profile: tc.profile, + Workload: tc.workload, + })) + + var want string + for _, l := range tc.want { + want += l + "\n" + } + assert.Equal(t, want, out.String()) + }) + } +} + +// The last lines of what was asked for, not the last lines of the file +// with the filter applied afterwards. Otherwise asking for one workload's +// last twenty lines shows however many of its lines happen to fall in the +// file's last twenty. +func TestShowLogsCountsTheLinesItShows(t *testing.T) { + t.Parallel() + + lines := []string{ + `workload=cli-llm-work host=first action=splice`, + `workload=chrome-work host=noise action=splice`, + `workload=chrome-work host=noise action=splice`, + `workload=chrome-work host=noise action=splice`, + `workload=cli-llm-work host=last action=splice`, + } + + var out bytes.Buffer + require.NoError(t, ShowLogs(t.Context(), &out, LogOptions{ + Path: writeLog(t, lines...), + Workload: "cli-llm", + Last: 2, + })) + + assert.Equal(t, lines[0]+"\n"+lines[4]+"\n", out.String()) +} + +func TestShowLogsFollowsWithAFilter(t *testing.T) { + t.Parallel() + + path := writeLog(t, `workload=cli-llm-work host=first action=splice`) + + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + + out := &syncBuffer{} + done := make(chan error, 1) + go func() { + done <- ShowLogs(ctx, out, LogOptions{Path: path, Workload: "cli-llm", Follow: true}) + }() + + require.EventuallyWithT(t, func(c *assert.CollectT) { + assert.Contains(c, out.String(), "host=first") + }, time.Second, 10*time.Millisecond) + + f, err := os.OpenFile(path, os.O_APPEND|os.O_WRONLY, 0o600) + require.NoError(t, err) + _, err = f.WriteString("workload=chrome-work host=ignored action=splice\n" + + "workload=cli-llm-work host=second action=splice\n") + require.NoError(t, err) + require.NoError(t, f.Close()) + + require.EventuallyWithT(t, func(c *assert.CollectT) { + assert.Contains(c, out.String(), "host=second") + }, time.Second, 10*time.Millisecond) + + assert.NotContains(t, out.String(), "host=ignored", + "a followed line for another workload must not be printed") + + cancel() + require.NoError(t, <-done) +} From cdfdd3ef7efcf17f0c483f2eec172aca07d96901 Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Thu, 10 Sep 2026 20:45:13 +0100 Subject: [PATCH 10/16] doctor: report what the gateway has been doing with egress doctor said whether the gateway was running and whether it was ready, and nothing about the thing it exists to do. A workload that cannot reach something got a clean bill of health and no hint of where to look. The new check separates two findings that are not the same. A refused connection is the policy working, so it is reported at ok, with the count and the hosts named, because "why can I not reach this" is what brings someone to doctor and the answer is a host name. An error is something the gateway tried to do and could not, so it warns and names the most recent one, which is where a splice that failed to dial or an original destination it could not recover now shows up. The log is summarised in internal/gateway rather than read here, so the one place that already understands the gateway's log format is the only place that parses it. It parses tolerantly: the format belongs to a component on its own release cycle, so a line that cannot be read contributes nothing rather than failing the check. The readiness check's advice was to read the gateway's own output, which until recently went to a terminal that was usually gone. It names the command now. Assisted-by: Claude Opus 5 Signed-off-by: Paulo Gomes Entire-Checkpoint: 01M26DMN1PHXHQET4H4S6H5R4Y --- internal/doctor/env.go | 19 +++++ internal/doctor/environment_test.go | 9 +++ internal/doctor/session.go | 78 +++++++++++++++++++-- internal/doctor/session_test.go | 94 +++++++++++++++++++++++++ internal/gateway/summary.go | 104 ++++++++++++++++++++++++++++ internal/gateway/summary_test.go | 73 +++++++++++++++++++ 6 files changed, 371 insertions(+), 6 deletions(-) create mode 100644 internal/gateway/summary.go create mode 100644 internal/gateway/summary_test.go diff --git a/internal/doctor/env.go b/internal/doctor/env.go index 936c0b6..c6f47d1 100644 --- a/internal/doctor/env.go +++ b/internal/doctor/env.go @@ -7,6 +7,7 @@ import ( "strconv" "time" + "github.com/qubesome/cli/internal/files" "github.com/qubesome/cli/internal/gateway" "github.com/qubesome/cli/internal/images" "github.com/qubesome/cli/internal/runners/util/usb" @@ -85,6 +86,15 @@ type Env interface { // would call a gateway ready while a workload started against it // would still have egress with no rules on it. GatewayReady() error + + // GatewayLog reports what the session gateway's log says has + // happened: how many connections it classified, how many it refused + // and to where, and whether anything failed. + // + // A summary rather than the log itself, because the log's shape is + // the gateway's own and reading it belongs in the one place that + // already understands it. + GatewayLog() (gateway.LogSummary, error) } // OSEnv is the real host. @@ -175,6 +185,15 @@ func (e *OSEnv) GatewayReady() error { return c.Ready(ctx) } +// GatewayLog reads the log the session's gateway writes. +// +// Nothing is asked of the gateway for this. It is the record the launch +// left behind, so it answers for a gateway that has stopped talking as +// well as for one that is well. +func (e *OSEnv) GatewayLog() (gateway.LogSummary, error) { + return gateway.Summarise(files.GatewayLogPath()) +} + func contextWithTimeout(d time.Duration) (context.Context, context.CancelFunc) { if d <= 0 { return context.WithCancel(context.Background()) diff --git a/internal/doctor/environment_test.go b/internal/doctor/environment_test.go index 24c7bc8..edc863e 100644 --- a/internal/doctor/environment_test.go +++ b/internal/doctor/environment_test.go @@ -8,6 +8,7 @@ import ( "time" "github.com/qubesome/cli/internal/files" + "github.com/qubesome/cli/internal/gateway" "github.com/stretchr/testify/require" ) @@ -29,6 +30,8 @@ type fakeEnv struct { // error is a ready gateway, which is also the zero value, so only a // test that wants a failure has to set it. gatewayReady error + log gateway.LogSummary + logErr error } type fakeOutput struct { @@ -128,6 +131,12 @@ func (f *fakeEnv) GatewayReady() error { return f.gatewayReady } +// GatewayLog answers with a canned summary, so a test can drive a check +// on what a gateway log said without writing one. +func (f *fakeEnv) GatewayLog() (gateway.LogSummary, error) { + return f.log, f.logErr +} + type fakeFileInfo struct { name string isDir bool diff --git a/internal/doctor/session.go b/internal/doctor/session.go index 033231a..d85e6e6 100644 --- a/internal/doctor/session.go +++ b/internal/doctor/session.go @@ -2,6 +2,7 @@ package doctor import ( "fmt" + "strings" "github.com/qubesome/cli/internal/files" "github.com/qubesome/cli/internal/types" @@ -33,10 +34,10 @@ func Session(env Env, cfg *types.Config) []Check { checks := []Check{holder, gateway} if gateway.Status == OK { - // Readiness only says something once there is a gateway to ask. - // Piling a second failure on top of the same cause would bury - // the one worth reading. - checks = append(checks, checkGatewayReady(env)) + // Readiness and egress only say something once there is a + // gateway to ask about. Piling a second failure on top of the + // same cause would bury the one worth reading. + checks = append(checks, checkGatewayReady(env), checkGatewayEgress(env)) } return checks @@ -165,8 +166,8 @@ func checkGatewayReady(env Env) Check { Name: name, Status: Fail, Detail: fmt.Sprintf("the gateway is running but did not report itself ready: %s", firstLine(err.Error())), - Fix: "Its resolver, proxy or netfilter ruleset did not come up. Check the gateway's own " + - "output, and the policy file the gateway block names.", + Fix: "Its resolver, proxy or netfilter ruleset did not come up. Read `qubesome gateway " + + "logs` for what it said, and check the policy file the gateway block names.", } } @@ -176,3 +177,68 @@ func checkGatewayReady(env Env) Check { Detail: "the gateway reports its resolver, proxy and ruleset are up", } } + +// checkGatewayEgress reports what the gateway has been doing with the +// connections its workloads made. +// +// A refused connection and a failed one are not the same finding. A +// denial is the policy doing what it says, so it is reported without +// being called a fault, and the hosts are named because "why can I not +// reach this" is what brings someone here. An error is something the +// gateway tried to do and could not, which is worth a warning. +func checkGatewayEgress(env Env) Check { + const name = "gateway egress" + + summary, err := env.GatewayLog() + if err != nil { + return Check{ + Name: name, + Status: Warn, + Detail: fmt.Sprintf("the gateway is running but what it has said cannot be read: %s", + firstLine(err.Error())), + Fix: "A gateway started before qubesome kept a log has none. Restart the session to " + + "get one, or read the terminal the gateway was started from.", + } + } + + if summary.Errors > 0 { + return Check{ + Name: name, + Status: Warn, + Detail: fmt.Sprintf("the gateway reported %s, most recently %q", + plural(summary.Errors, "error"), summary.LastError), + Fix: "Read `qubesome gateway logs` for the whole of it. `-workload` and `-profile` " + + "narrow it to one workload.", + } + } + + if summary.Denied == 0 { + return Check{ + Name: name, + Status: OK, + Detail: fmt.Sprintf("the gateway classified %s and refused none", + plural(summary.Decisions, "connection")), + } + } + + return Check{ + Name: name, + Status: OK, + Detail: fmt.Sprintf("the gateway classified %s and refused %d, to %s", + plural(summary.Decisions, "connection"), + summary.Denied, strings.Join(summary.DeniedHosts, ", ")), + Fix: "This is the policy being applied, and is only a problem if one of those hosts was " + + "meant to be reachable. A workload reaches a host named under egress.allowed or " + + "dns.allowed for it, and the gateway mirrors one onto the other when only one is set.", + } +} + +// plural renders a count with its noun, so a report reads as a sentence +// rather than as a field. +func plural(n int, noun string) string { + if n == 1 { + return "1 " + noun + } + + return fmt.Sprintf("%d %ss", n, noun) +} diff --git a/internal/doctor/session_test.go b/internal/doctor/session_test.go index 45f8ca0..0adb0f6 100644 --- a/internal/doctor/session_test.go +++ b/internal/doctor/session_test.go @@ -5,6 +5,7 @@ import ( "testing" "github.com/qubesome/cli/internal/files" + "github.com/qubesome/cli/internal/gateway" "github.com/qubesome/cli/internal/types" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -171,3 +172,96 @@ func TestRunReportsTheSessionWithoutAProfile(t *testing.T) { checks := sectionByPrefix(t, report, "Session") assert.Equal(t, OK, checkByName(t, checks, "session gateway").Status) } + +// A denial is the policy working, so it is reported without being called +// a fault. It is also the answer to why a workload cannot reach +// something, which is what brings someone to doctor in the first place, +// so the hosts are named. +func TestSessionReportsDeniedHosts(t *testing.T) { + t.Parallel() + + env := runningSession() + env.log = gateway.LogSummary{ + Decisions: 9, + Denied: 2, + DeniedHosts: []string{"ads.example.com", "telemetry.example.com"}, + } + + check := egressCheck(t, Session(env, gatewayConfig())) + + assert.Equal(t, OK, check.Status, "a policy refusing a host is not a broken gateway") + assert.Contains(t, check.Detail, "2") + assert.Contains(t, check.Detail, "ads.example.com") + assert.Contains(t, check.Detail, "telemetry.example.com") +} + +// An error is not a denial. Something the gateway tried to do did not +// work, and that is worth warning about. +func TestSessionWarnsOnGatewayErrors(t *testing.T) { + t.Parallel() + + env := runningSession() + env.log = gateway.LogSummary{ + Decisions: 4, + Errors: 3, + LastError: "splice dial failed", + } + + check := egressCheck(t, Session(env, gatewayConfig())) + + assert.Equal(t, Warn, check.Status) + assert.Contains(t, check.Detail, "splice dial failed") + assert.Contains(t, check.Fix, "qubesome gateway logs") +} + +func TestSessionEgressWithAQuietGateway(t *testing.T) { + t.Parallel() + + env := runningSession() + env.log = gateway.LogSummary{Decisions: 12} + + check := egressCheck(t, Session(env, gatewayConfig())) + + assert.Equal(t, OK, check.Status) + assert.Contains(t, check.Detail, "12") +} + +// A gateway from before qubesome kept its log has none to read. That is +// not a fault of the session, and it must not be reported as one. +func TestSessionEgressWithoutALog(t *testing.T) { + t.Parallel() + + env := runningSession() + env.logErr = errors.New("no gateway log at /run/session/gateway.log") + + check := egressCheck(t, Session(env, gatewayConfig())) + + assert.Equal(t, Warn, check.Status) + assert.Contains(t, check.Detail, "no gateway log") +} + +// A session with no gateway running has no egress to diagnose, and +// stacking a second answer on the same cause buries the one worth reading. +func TestSessionEgressIsNotAskedWithoutAGateway(t *testing.T) { + t.Parallel() + + env := &fakeEnv{alive: map[string]bool{files.SessionStatePath(): true}} + + for _, c := range Session(env, gatewayConfig()) { + assert.NotEqual(t, "gateway egress", c.Name, + "there is no gateway, so there is nothing to say about its egress") + } +} + +func egressCheck(t *testing.T, checks []Check) Check { + t.Helper() + + for _, c := range checks { + if c.Name == "gateway egress" { + return c + } + } + t.Fatalf("no gateway egress check in %v", checks) + + return Check{} +} diff --git a/internal/gateway/summary.go b/internal/gateway/summary.go new file mode 100644 index 0000000..3b1c19a --- /dev/null +++ b/internal/gateway/summary.go @@ -0,0 +1,104 @@ +package gateway + +import ( + "bufio" + "errors" + "fmt" + "os" + "slices" + "strings" +) + +// errNoLog reports a session with no gateway log to read. It is a distinct +// error because a caller diagnosing a host has to tell "the gateway said +// nothing worth reporting" from "there was nothing to read". +var errNoLog = errors.New("no gateway log") + +// maxDeniedHosts bounds how many denied hosts a summary names. A policy +// that denies a great deal is working, and a diagnosis of it should say so +// in a line rather than reproduce the log. +const maxDeniedHosts = 5 + +// LogSummary is what a gateway log says has happened. +// +// It is deliberately small. It exists so that qubesome doctor can say +// whether the gateway has been refusing connections or failing to make +// them, without reading the log for the user or growing an opinion about +// what a policy ought to allow. +type LogSummary struct { + // Decisions is how many connections the proxy classified. + Decisions int + + // Denied is how many of those it refused. A denial is the policy + // working, so this is not a count of faults. It is the answer to why + // a workload could not reach something. + Denied int + + // DeniedHosts names the distinct hosts that were denied, in the order + // they were first refused, at most maxDeniedHosts of them. + DeniedHosts []string + + // Errors is how many lines the gateway logged at error level. Unlike + // a denial, each of these is something that did not work. + Errors int + + // LastError is the message of the most recent of them. + LastError string +} + +// Summarise reads the gateway log at path and reports what it says. +// +// A line it cannot parse contributes nothing rather than failing the read. +// The log's shape belongs to the gateway, which is a separate component on +// its own release cycle, so a summary of it degrades to saying less rather +// than to refusing to say anything. +func Summarise(path string) (LogSummary, error) { + f, err := os.Open(path) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + return LogSummary{}, fmt.Errorf("%w at %s", errNoLog, path) + } + + return LogSummary{}, fmt.Errorf("failed to open the gateway log %q: %w", path, err) + } + defer f.Close() + + var s LogSummary + + scanner := bufio.NewScanner(f) + scanner.Buffer(make([]byte, 0, bufio.MaxScanTokenSize), maxLineLen) + + for scanner.Scan() { + s.read(scanner.Text()) + } + if err := scanner.Err(); err != nil { + return LogSummary{}, fmt.Errorf("failed to read the gateway log %q: %w", path, err) + } + + return s, nil +} + +// read folds one log line into the summary. +func (s *LogSummary) read(line string) { + if strings.EqualFold(logField(line, fieldLevel), "error") { + s.Errors++ + s.LastError = logField(line, "msg") + } + + action := logField(line, fieldAction) + if action == "" { + return + } + s.Decisions++ + + if action != "deny" { + return + } + s.Denied++ + + host := logField(line, fieldHost) + if host == "" || len(s.DeniedHosts) == maxDeniedHosts || slices.Contains(s.DeniedHosts, host) { + return + } + s.DeniedHosts = append(s.DeniedHosts, host) +} diff --git a/internal/gateway/summary_test.go b/internal/gateway/summary_test.go new file mode 100644 index 0000000..4830059 --- /dev/null +++ b/internal/gateway/summary_test.go @@ -0,0 +1,73 @@ +package gateway + +import ( + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestSummarise(t *testing.T) { + t.Parallel() + + path := writeLog(t, + `level=INFO msg="starting gateway"`, + `level=INFO msg="proxy decision" workload=cli-llm-work host=api.anthropic.com action=splice`, + `level=INFO msg="proxy decision" workload=chrome-work host=ads.example.com action=deny reason="policy denied host"`, + `level=INFO msg="proxy decision" workload=chrome-work host=eu.ads.example.com action=deny reason="policy denied host"`, + `level=INFO msg="proxy decision" workload=chrome-work host=ads.example.com action=deny reason="policy denied host"`, + `level=WARN msg="splice dial failed" workload=chrome-work host=slow.example`, + `level=ERROR msg="original destination lookup failed" plane=proxy`, + `level=INFO msg="proxy decision" workload=cli-llm-work host=api.github.com action=inject`, + ) + + got, err := Summarise(path) + require.NoError(t, err) + + assert.Equal(t, 5, got.Decisions, "every proxy decision counts, whatever its verdict") + assert.Equal(t, 3, got.Denied) + assert.Equal(t, []string{"ads.example.com", "eu.ads.example.com"}, got.DeniedHosts, + "a host denied twice is named once") + assert.Equal(t, 1, got.Errors, "a warning is not an error") + assert.Equal(t, "original destination lookup failed", got.LastError) +} + +func TestSummariseAQuietLog(t *testing.T) { + t.Parallel() + + got, err := Summarise(writeLog(t, `level=INFO msg="starting gateway"`)) + require.NoError(t, err) + + assert.Zero(t, got.Decisions) + assert.Zero(t, got.Denied) + assert.Empty(t, got.DeniedHosts) + assert.Zero(t, got.Errors) +} + +func TestSummariseWithoutALog(t *testing.T) { + t.Parallel() + + _, err := Summarise(filepath.Join(t.TempDir(), "absent.log")) + require.Error(t, err) + assert.ErrorIs(t, err, errNoLog) +} + +// A gateway that denies a great many hosts must not turn a diagnosis into +// a list of them. +func TestSummariseCapsTheHostsItNames(t *testing.T) { + t.Parallel() + + lines := make([]string, 0, maxDeniedHosts*2) + for i := range maxDeniedHosts * 2 { + lines = append(lines, + `level=INFO msg="proxy decision" workload=w-p action=deny host=h`+ + string(rune('a'+i))+`.example`) + } + + got, err := Summarise(writeLog(t, lines...)) + require.NoError(t, err) + + assert.Equal(t, maxDeniedHosts*2, got.Denied, "the count is of every denial") + assert.Len(t, got.DeniedHosts, maxDeniedHosts, "the names are capped") +} From 7ec3031e91f04334eb972a42d6f25d7c273bfcab Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Thu, 10 Sep 2026 21:13:37 +0100 Subject: [PATCH 11/16] gateway: do not report a config the running gateway may not have gateway status read its config through profileConfigOrDefault(""), which takes an active profile's config only when exactly one profile is active and otherwise falls back to the user-level file. The gateway is session wide: it was started by whichever launch found none running, from that profile's config, and may have come from any of the active ones. With more than one active, status could therefore name an image, a policy and a subnet the running gateway has nothing to do with, and say nothing to suggest it was guessing. Raised in review on #226. Several active profiles are usually not an ambiguity at all. One qubesome config commonly defines several profiles, so the run dir holds several symlinks to one file, and resolving them leaves a single config that is the answer whichever profile started the gateway. That is now what is looked for. It is only profiles started from genuinely different files that leave nothing here able to say which the gateway came from, and status now says that instead of picking one. ConfigProblem already exists for exactly this shape of answer, and reporting that it cannot be told is the behaviour the command was written around: it reports a reason rather than leaving the lines blank. What this does not do is say what the running gateway is actually running, as opposed to what a config names. Only the gateway's own launch knows that, and nothing records it. The allocation record already carries the subnet the running gateway hands addresses out of, and is already compared against the config, so it is where the image and the policy path would go if that becomes worth having. Assisted-by: Claude Opus 5 Signed-off-by: Paulo Gomes Entire-Checkpoint: 01M26F8MJDW9GE2ZX2Z8XKNECR --- cmd/cli/gateway.go | 81 +++++++++++++++++++++++++++++++++++++---- cmd/cli/gateway_test.go | 60 ++++++++++++++++++++++++++++++ 2 files changed, 134 insertions(+), 7 deletions(-) create mode 100644 cmd/cli/gateway_test.go diff --git a/cmd/cli/gateway.go b/cmd/cli/gateway.go index 4ff1958..7d6cc2f 100644 --- a/cmd/cli/gateway.go +++ b/cmd/cli/gateway.go @@ -4,10 +4,12 @@ import ( "context" "fmt" "os" + "path/filepath" "github.com/qubesome/cli/internal/files" "github.com/qubesome/cli/internal/gateway" "github.com/qubesome/cli/internal/session" + "github.com/qubesome/cli/internal/types" "github.com/urfave/cli/v3" ) @@ -62,19 +64,84 @@ func gatewayStatusCommand() *cli.Command { Action: func(ctx context.Context, cmd *cli.Command) error { g := gateway.Current() - // The same route doctor takes to a config, and it may find - // none. The gateway is per session, so there is no profile to - // name here, and without a running profile or a user-level - // file there is nothing that says which image, policy or - // subnet a gateway was meant to have. Inspect reports that it - // could not tell rather than leaving the lines blank. - status := g.Inspect(session.Current(), profileConfigOrDefault(""), g.StatusReady) + cfg, problem := sessionConfig() + + // Inspect reports that it could not tell rather than leaving + // the lines blank, so a config that could not be identified + // is passed on as none with the reason it could not. + status := g.Inspect(session.Current(), cfg, g.StatusReady) + if problem != "" { + status.ConfigProblem = problem + } return status.Write(os.Stdout) }, } } +// sessionConfig returns the config describing this session's gateway, and +// why it could not be told when it cannot. +// +// Not profileConfigOrDefault. That falls back to the user-level config as +// soon as more than one profile is active, and the gateway is session +// wide: it was started by whichever launch found none running, from that +// profile's config, and may have come from any of them. Reporting the +// user-level file's gateway block for it would name an image, a policy and +// a subnet that the running gateway need not have anything to do with. +// +// Several active profiles are usually not an ambiguity at all, because one +// qubesome config commonly defines several profiles and they were all +// started from the same file. It is only profiles started from different +// files that leave nothing here able to say which one the gateway came +// from, and then saying so is the answer. +func sessionConfig() (*types.Config, string) { + active := activeConfigs() + + resolved := make([]string, 0, len(active)) + for _, path := range active { + // The run dir holds a symlink per active profile, so two profiles + // sharing a config are two links to one file. + target, err := filepath.EvalSymlinks(path) + if err != nil { + continue + } + resolved = append(resolved, target) + } + + if path, ok := sessionConfigPath(resolved); ok { + if cfg := config(path); cfg != nil && len(cfg.Profiles) > 0 { + return cfg, "" + } + } + + if len(resolved) > 1 { + return nil, fmt.Sprintf( + "%d profiles are active and were started from different configs, so which of them the "+ + "running gateway came from cannot be told, and the image, policy and subnet it "+ + "names are unknown", len(resolved)) + } + + // No profile running, or one whose config no longer reads. The + // user-level file is the only thing left that describes a gateway. + return profileConfigOrDefault(""), "" +} + +// sessionConfigPath returns the one config file every active profile was +// started from, and whether there was one. +func sessionConfigPath(active []string) (string, bool) { + if len(active) == 0 { + return "", false + } + + for _, path := range active[1:] { + if path != active[0] { + return "", false + } + } + + return active[0], true +} + func gatewayStopCommand() *cli.Command { return &cli.Command{ Name: "stop", diff --git a/cmd/cli/gateway_test.go b/cmd/cli/gateway_test.go new file mode 100644 index 0000000..1dee4f7 --- /dev/null +++ b/cmd/cli/gateway_test.go @@ -0,0 +1,60 @@ +package cli + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +// The gateway belongs to the session and not to a profile, so its status +// has no profile to be told which config to read. Every active profile +// naming the same file is the ordinary case, since one qubesome config +// usually defines several profiles, and that file is the answer. Two +// profiles started from different files is the case that has no answer. +func TestSessionConfigPath(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + active []string + want string + wantOne bool + }{ + { + name: "nothing active", + active: nil, + }, + { + name: "one profile", + active: []string{"/home/u/dotfiles/qubesome.yaml"}, + want: "/home/u/dotfiles/qubesome.yaml", + wantOne: true, + }, + { + name: "several profiles from one config", + active: []string{ + "/home/u/dotfiles/qubesome.yaml", + "/home/u/dotfiles/qubesome.yaml", + }, + want: "/home/u/dotfiles/qubesome.yaml", + wantOne: true, + }, + { + name: "profiles from different configs", + active: []string{ + "/home/u/dotfiles/qubesome.yaml", + "/home/u/work/qubesome.yaml", + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + got, ok := sessionConfigPath(tc.active) + assert.Equal(t, tc.wantOne, ok) + assert.Equal(t, tc.want, got) + }) + } +} From c13d506de9cbe1b10d899542bc3d9e19eac09d87 Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Thu, 10 Sep 2026 21:23:24 +0100 Subject: [PATCH 12/16] bwrap: tell a workload where to ask the gateway for a tunnel The gateway drops every port but 80, 443 and 53, so a workload reaches anything else by asking it for a tunnel instead of connecting out. Asking means naming an endpoint, and nothing in the sandbox said what it was. The gateway's own address a workload could have worked out for itself: it is already its default route and the nameserver in its resolv.conf. The port it could not, because that one belongs to the gateway image. So what it is told is the pair, in QUBESOME_GATEWAY_PROXY, ready to be used as it stands. An ssh client reaches a host it is allowed to reach with ProxyCommand socat - PROXY:$QUBESOME_GATEWAY_PROXY:%h:%p and nothing there names a port that is the gateway's business to choose. The port is a const beside the gateway image's other paths, and carries the same caveat they do: it belongs to the image, so the two have to be changed together. Telling a workload the whole endpoint rather than only the address is what keeps that caveat out of anybody's dotfiles. A workload with no gateway is told nothing rather than told an empty value, which would read as an endpoint that exists and is nothing. Assisted-by: Claude Opus 5 Signed-off-by: Paulo Gomes Entire-Checkpoint: 01M26FTJHYYKTQB58PQ5NEECRV --- internal/gateway/attach.go | 21 +++++++++++ internal/gateway/proxyaddr_test.go | 48 ++++++++++++++++++++++++ internal/gateway/run.go | 11 ++++++ internal/runners/bwrap/proxyenv_test.go | 50 +++++++++++++++++++++++++ internal/runners/bwrap/run.go | 10 +++++ internal/runners/bwrap/spec.go | 14 ++++++- 6 files changed, 153 insertions(+), 1 deletion(-) create mode 100644 internal/gateway/proxyaddr_test.go create mode 100644 internal/runners/bwrap/proxyenv_test.go diff --git a/internal/gateway/attach.go b/internal/gateway/attach.go index 24e9fd2..a6b898e 100644 --- a/internal/gateway/attach.go +++ b/internal/gateway/attach.go @@ -3,7 +3,9 @@ package gateway import ( "context" "log/slog" + "net" "net/netip" + "strconv" "github.com/qubesome/cli/internal/types" ) @@ -67,6 +69,25 @@ func Attached(cfg *types.Config, network string) (*Attach, error) { return &Attach{gateway: g, config: *cfg.Gateway, Addr: addr}, nil } +// ProxyAddr returns the endpoint a workload asks for a tunnel on. +// +// The gateway's own address is already the workload's default route and +// its resolver, so a workload could find it for itself. The port it could +// not, so what it is told is the pair, ready to be used as it is. +func (a *Attach) ProxyAddr() (string, error) { + subnet, err := a.config.SubnetPrefix() + if err != nil { + return "", err + } + + addr, err := GatewayAddr(subnet) + if err != nil { + return "", err + } + + return net.JoinHostPort(addr.String(), strconv.Itoa(inProxyPort)), nil +} + // Wire gives the sandbox at pid a link to the gateway, addressed at both // ends and with a resolver pointed at it. // diff --git a/internal/gateway/proxyaddr_test.go b/internal/gateway/proxyaddr_test.go new file mode 100644 index 0000000..2aaa2ef --- /dev/null +++ b/internal/gateway/proxyaddr_test.go @@ -0,0 +1,48 @@ +package gateway + +import ( + "testing" + + "github.com/qubesome/cli/internal/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// A workload reaches the proxy at the gateway's own address, which is the +// first host address of the subnet and is already its default route and +// its resolver. What it cannot work out for itself is the port, so the +// whole endpoint is what it is told. +func TestAttachProxyAddr(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + subnet string + want string + }{ + {"the usual subnet", "10.111.0.0/24", "10.111.0.1:3128"}, + {"another range", "192.168.44.0/24", "192.168.44.1:3128"}, + {"a small one", "10.9.9.8/30", "10.9.9.9:3128"}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + a := &Attach{config: types.GatewayConfig{Subnet: tc.subnet}} + + got, err := a.ProxyAddr() + require.NoError(t, err) + assert.Equal(t, tc.want, got) + }) + } +} + +func TestAttachProxyAddrWithoutASubnet(t *testing.T) { + t.Parallel() + + a := &Attach{config: types.GatewayConfig{Subnet: "not a subnet"}} + + _, err := a.ProxyAddr() + require.Error(t, err) +} diff --git a/internal/gateway/run.go b/internal/gateway/run.go index c51e3fd..74d19d6 100644 --- a/internal/gateway/run.go +++ b/internal/gateway/run.go @@ -56,6 +56,17 @@ const ( // for the whole session, so unlike a workload's it carries no profile. gatewayHostname = "qubesome-gateway" + // inProxyPort is the port the gateway image's proxy serves its + // plaintext listener on, which is also where it accepts CONNECT. + // + // It is here with the image's other constants, and carries the same + // caveat: it belongs to the gateway and not to qubesome, so the two + // have to be changed together. A workload is told the whole endpoint + // rather than only the address for exactly that reason, so that a + // port which is the gateway's business does not end up written into + // anybody's dotfiles. + inProxyPort = 3128 + // pastaCommand is the uplink binary in the gateway image, where the // passt package puts it. pastaCommand = "/usr/bin/pasta" diff --git a/internal/runners/bwrap/proxyenv_test.go b/internal/runners/bwrap/proxyenv_test.go new file mode 100644 index 0000000..e23d257 --- /dev/null +++ b/internal/runners/bwrap/proxyenv_test.go @@ -0,0 +1,50 @@ +package bwrap + +import ( + "testing" + + "github.com/qubesome/cli/internal/images" + "github.com/qubesome/cli/internal/types" + "github.com/stretchr/testify/assert" +) + +// A workload that has a gateway is told where to ask it for a tunnel. One +// without a gateway has nothing to be told, and an empty variable would +// read as an endpoint of nothing at all. +func TestWorkloadEnvNamesTheGatewayProxy(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + proxy string + want bool + }{ + {"attached to a gateway", "10.111.0.1:3128", true}, + {"no gateway", "", false}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + env := workloadEnv(input{ + Workload: types.EffectiveWorkload{ + Name: "w-p", + Profile: &types.Profile{Name: "p"}, + Workload: types.Workload{}, + }, + Bundle: images.Bundle{}, + GatewayProxy: tc.proxy, + }) + + if tc.want { + assert.Contains(t, env, "QUBESOME_GATEWAY_PROXY="+tc.proxy) + + return + } + for _, e := range env { + assert.NotContains(t, e, "QUBESOME_GATEWAY_PROXY") + } + }) + } +} diff --git a/internal/runners/bwrap/run.go b/internal/runners/bwrap/run.go index 4678aee..fa3ddcb 100644 --- a/internal/runners/bwrap/run.go +++ b/internal/runners/bwrap/run.go @@ -105,6 +105,16 @@ func Run(ew types.EffectiveWorkload, cfg *types.Config) error { return err } + // The endpoint the workload asks for a tunnel on, which is known here + // because the address was allocated before the sandbox was built. + if att != nil { + proxy, err := att.ProxyAddr() + if err != nil { + return err + } + in.GatewayProxy = proxy + } + spec, err := buildSpec(in) if err != nil { return err diff --git a/internal/runners/bwrap/spec.go b/internal/runners/bwrap/spec.go index 61189f8..6c3c1aa 100644 --- a/internal/runners/bwrap/spec.go +++ b/internal/runners/bwrap/spec.go @@ -111,6 +111,11 @@ type input struct { // created on the host. Paths []sandbox.Mount + // GatewayProxy is where the workload asks the gateway for a tunnel to + // a host it may reach on a port the transparent path does not carry. + // Empty for a workload with no gateway, which has nowhere to ask. + GatewayProxy string + // HostEnv holds the host variables a workload on the host dbus reads. // The container runners named them and let the runtime copy the // values across. bwrap clears the environment instead, so the values @@ -480,7 +485,7 @@ func workloadEnv(in input) []string { wl := in.Workload.Workload profile := in.Workload.Profile - const extra = 8 + const extra = 9 env := make([]string, 0, len(in.Bundle.Env)+len(in.HostEnv)+extra) env = append(env, in.Bundle.Env...) @@ -490,6 +495,13 @@ func workloadEnv(in input) []string { "QUBESOME_PROFILE="+profile.Name, ) + // Only when there is one. An empty value would read as an endpoint + // that is there and is nothing, and a workload with no gateway has + // nowhere to ask for a tunnel at all. + if in.GatewayProxy != "" { + env = append(env, "QUBESOME_GATEWAY_PROXY="+in.GatewayProxy) + } + // A profile that names a timezone means it, whatever the host is set // to. Otherwise the workload follows the host, which it used to do // by reading the /etc/localtime shared with it. From 5105c19c19c62a852721699c46b00ec55bd4a873 Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Thu, 10 Sep 2026 23:12:11 +0100 Subject: [PATCH 13/16] gateway: say when the log will not close CodeQL: a writable handle from OpenFile closed without handling the error, where a failure can mean data that never reached the disk. Nothing here writes through this descriptor. It is opened, handed to the sandbox and to the uplink, which get their own from Start onwards, and qubesome's copy is closed as being of no further use. So unlike the write in replace there is no last part of one arriving at close, and nothing to lose by not looking. Discarding the error is still the wrong shape. A close that fails says the filesystem holding the log is unwell, and the log is the file a gateway that goes wrong explains itself in, so it is worth a line. It is a warning rather than an error because the gateway is already running by then, and a log qubesome could not close is no reason to take one down. Assisted-by: Claude Opus 5 Signed-off-by: Paulo Gomes Entire-Checkpoint: 01M26P1R6XJFJ64N08Y768H75Q --- internal/gateway/logs.go | 19 +++++++++++++++++++ internal/gateway/logs_test.go | 15 +++++++++++++++ internal/gateway/run.go | 11 +++++++++-- 3 files changed, 43 insertions(+), 2 deletions(-) diff --git a/internal/gateway/logs.go b/internal/gateway/logs.go index 426d031..4901191 100644 --- a/internal/gateway/logs.go +++ b/internal/gateway/logs.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "io" + "log/slog" "os" "strings" "time" @@ -245,3 +246,21 @@ func openLog(path string) (*os.File, error) { return f, nil } + +// closeLog closes qubesome's own copy of the gateway log. +// +// The sandbox and the uplink are handed their own descriptors for it, so +// this one is closed as soon as they are started and closing it loses +// nothing: nothing here writes through it, so there is no buffered tail +// of a write to fail to reach the disk. +// +// It is still not discarded. A close that fails says the filesystem the +// log sits on is unwell, and that is worth knowing about the file a +// gateway explains itself in. It is a warning and not an error because +// the gateway it belongs to is already running by this point, and a log +// qubesome could not close is no reason to take one down. +func closeLog(f *os.File) { + if err := f.Close(); err != nil { + slog.Warn("failed to close the gateway log", "path", f.Name(), "error", err) + } +} diff --git a/internal/gateway/logs_test.go b/internal/gateway/logs_test.go index a81ac5e..a83db07 100644 --- a/internal/gateway/logs_test.go +++ b/internal/gateway/logs_test.go @@ -320,3 +320,18 @@ func TestShowLogsFollowsWithAFilter(t *testing.T) { cancel() require.NoError(t, <-done) } + +// A close that fails is reported and does not stop the caller. The +// gateway it belongs to is already running by then, and a log qubesome +// could not close is no reason to take one down. +func TestCloseLogSurvivesAFailure(t *testing.T) { + t.Parallel() + + f, err := openLog(filepath.Join(t.TempDir(), "gateway.log")) + require.NoError(t, err) + + require.NoError(t, f.Close()) + + // The second close is the failure, since the descriptor is gone. + assert.NotPanics(t, func() { closeLog(f) }) +} diff --git a/internal/gateway/run.go b/internal/gateway/run.go index 74d19d6..82c04f2 100644 --- a/internal/gateway/run.go +++ b/internal/gateway/run.go @@ -416,7 +416,14 @@ func (g Gateway) launch(bundle images.Bundle, spec sandbox.Spec) error { } // The sandbox has its own copy once it is started, and a launch that // never got that far has nothing to write here either. - defer log.Close() + // + // The error is reported rather than deferred away. Nothing here ever + // writes through this descriptor, so unlike the write in replace + // there is no last part of one that reaches the filesystem at close + // and nothing to lose. What a failure here does say is that the + // filesystem holding the log is unwell, and the log is where a + // gateway that goes wrong explains itself, so it is worth a line. + defer closeLog(log) cmd.Stdout = log cmd.Stderr = log @@ -722,7 +729,7 @@ func (h helper) start() (*execabs.Cmd, error) { if err != nil { return nil, err } - defer log.Close() + defer closeLog(log) cmd.Stdout = log cmd.Stderr = log From 7505821b1895e4e8431c51a2119aa87d2097c934 Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Thu, 10 Sep 2026 23:18:03 +0100 Subject: [PATCH 14/16] gateway: wait for a stopped gateway to go before forgetting it Stop signalled the sandbox and removed its record in the same breath. A delivered signal is not a sandbox that has gone, so the next launch could take the lock, find no record, and start a replacement while the old pid namespace, its outer bwrap and its uplink were still coming down. Raised in review on #226. It now waits for the record to stop naming something running, and only then removes it. The record has to outlive the wait, because it is what the next launch decides by. Waiting is a poll, since there is nothing to wait on: a gateway is not the child of whatever stops it, so it cannot be reaped here and the kernel will not report its going. Five seconds bounds it, which is a ceiling on the kernel finishing rather than an estimate: SIGKILL to pid 1 of a pid namespace takes the namespace with it and is not something the process can put off. sandbox.Exited is new and is not the negation of Alive. A process killed by something that is not its parent stays in the table as a zombie, with its /proc entry and its start time intact, so Alive reads it as running when it is only waiting to be collected. The parent that would collect it is usually a qubesome run that exited at its terminal long ago, so a wait on Alive would have waited out the whole grace every time. It reads the state character from the same stat line the start time already comes from. Assisted-by: Claude Opus 5 Signed-off-by: Paulo Gomes Entire-Checkpoint: 01M26PCGB8A1QDXH7RYXWA20XB --- internal/gateway/stop.go | 46 ++++++++++++++++ internal/gateway/stop_test.go | 96 +++++++++++++++++++++++++++++++++ internal/sandbox/exited_test.go | 75 ++++++++++++++++++++++++++ internal/sandbox/state.go | 55 ++++++++++++++++++- 4 files changed, 271 insertions(+), 1 deletion(-) create mode 100644 internal/sandbox/exited_test.go diff --git a/internal/gateway/stop.go b/internal/gateway/stop.go index 84afa90..b12ea54 100644 --- a/internal/gateway/stop.go +++ b/internal/gateway/stop.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "syscall" + "time" "github.com/qubesome/cli/internal/files" "github.com/qubesome/cli/internal/sandbox" @@ -73,6 +74,15 @@ func (g Gateway) Stop() (int, error) { return 0, fmt.Errorf("failed to stop the gateway sandbox pid %d: %w", st.PID, err) } + // A delivered signal is not a sandbox that has gone. The record has to + // outlive the wait, because the next launch takes this same lock and + // decides by what it finds: removing the record first would let it see + // nothing, and start a replacement while the old pid namespace, its + // outer bwrap and its uplink were still coming down. + if err := waitGone(g.StatePath, stopGrace, stopPoll); err != nil { + return st.PID, err + } + // A qubesome run at a terminal exits long before the gateway it started // does, so the goroutine that would have cleared this record has // usually gone with it. @@ -82,3 +92,39 @@ func (g Gateway) Stop() (int, error) { return st.PID, nil } + +const ( + // stopGrace bounds the wait for a signalled gateway to go. SIGKILL to + // pid 1 of a pid namespace takes the namespace with it and is not + // something the process can put off, so this is a ceiling on the + // kernel finishing rather than an estimate of anything. + stopGrace = 5 * time.Second + + // stopPoll is how often the record is checked while waiting. There is + // nothing to wait on: the gateway is not this process's child, so it + // cannot be reaped here and its going away is not something the + // kernel will report. + stopPoll = 20 * time.Millisecond +) + +// waitGone blocks until the sandbox recorded at path has finished. +// +// sandbox.Exited and not the negation of sandbox.Alive, because a process +// killed by something that is not its parent stays in the table until +// somebody reaps it. Waiting for it to leave /proc would mean waiting for +// a parent that has usually exited long ago. +func waitGone(path string, grace, poll time.Duration) error { + deadline := time.Now().Add(grace) + + for { + if sandbox.Exited(path) { + return nil + } + + if time.Now().After(deadline) { + return fmt.Errorf("timed out after %s waiting for the gateway sandbox to stop", grace) + } + + time.Sleep(poll) + } +} diff --git a/internal/gateway/stop_test.go b/internal/gateway/stop_test.go index 4c94377..facb5dd 100644 --- a/internal/gateway/stop_test.go +++ b/internal/gateway/stop_test.go @@ -139,3 +139,99 @@ func TestStopReleasesTheGatewayLock(t *testing.T) { t.Fatal("the gateway lock was still held after Stop returned") } } + +// A successful kill is a signal delivered and not a sandbox gone. Stop +// waits, because the next launch takes the same lock and would otherwise +// find no record and start a replacement while the old namespace and its +// uplink were still coming down. +func TestWaitGone(t *testing.T) { + t.Parallel() + + t.Run("returns once the process is gone", func(t *testing.T) { + t.Parallel() + + cmd := execabs.Command("sleep", "60") + require.NoError(t, cmd.Start()) + + path := statePathFor(t, cmd.Process.Pid) + require.NoError(t, cmd.Process.Kill()) + + require.NoError(t, waitGone(path, 5*time.Second, time.Millisecond)) + + _ = cmd.Wait() + }) + + // The one that matters. A gateway is not the child of whatever stops + // it, so nothing reaps it here and it sits as a zombie until its real + // parent, or init, collects it. Waiting for it to leave the process + // table would mean waiting out the whole grace every time. + t.Run("does not wait for a zombie to be reaped", func(t *testing.T) { + t.Parallel() + + cmd := execabs.Command("sleep", "60") + require.NoError(t, cmd.Start()) + + path := statePathFor(t, cmd.Process.Pid) + require.NoError(t, cmd.Process.Kill()) + + // Long enough that a wait on reaping would fail the assertion + // rather than pass it slowly. + start := time.Now() + require.NoError(t, waitGone(path, 30*time.Second, time.Millisecond)) + assert.Less(t, time.Since(start), 5*time.Second) + + _ = cmd.Wait() + }) + + t.Run("gives up on a process that will not go", func(t *testing.T) { + t.Parallel() + + cmd := execabs.Command("sleep", "60") + require.NoError(t, cmd.Start()) + t.Cleanup(func() { _ = cmd.Process.Kill(); _ = cmd.Wait() }) + + err := waitGone(statePathFor(t, cmd.Process.Pid), 50*time.Millisecond, time.Millisecond) + + require.Error(t, err) + assert.Contains(t, err.Error(), "waiting for the gateway sandbox to stop") + }) + + t.Run("a record that names nothing is already gone", func(t *testing.T) { + t.Parallel() + + require.NoError(t, waitGone(filepath.Join(t.TempDir(), "absent.json"), time.Second, time.Millisecond)) + }) +} + +func TestStopWaitsForTheSandboxToGo(t *testing.T) { + t.Parallel() + + g := newSessionGateway(t) + + cmd := execabs.Command("sleep", "60") + require.NoError(t, cmd.Start()) + pid := cmd.Process.Pid + + require.NoError(t, sandbox.WriteState(g.StatePath, pid)) + + got, err := g.Stop() + require.NoError(t, err) + assert.Equal(t, pid, got) + assert.NoFileExists(t, g.StatePath) + + assert.True(t, sandbox.Exited(statePathFor(t, pid)), + "Stop returned while the sandbox was still running") + + _ = cmd.Wait() +} + +// statePathFor writes a record naming pid, so a test can ask about a +// process whose own record Stop has already removed. +func statePathFor(t *testing.T, pid int) string { + t.Helper() + + path := filepath.Join(t.TempDir(), "sandbox.json") + require.NoError(t, sandbox.WriteState(path, pid)) + + return path +} diff --git a/internal/sandbox/exited_test.go b/internal/sandbox/exited_test.go new file mode 100644 index 0000000..81bd017 --- /dev/null +++ b/internal/sandbox/exited_test.go @@ -0,0 +1,75 @@ +package sandbox + +import ( + "os/exec" + "path/filepath" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func recordOf(t *testing.T, pid int) string { + t.Helper() + + path := filepath.Join(t.TempDir(), "sandbox.json") + require.NoError(t, WriteState(path, pid)) + + return path +} + +func TestExited(t *testing.T) { + t.Parallel() + + t.Run("a running process has not", func(t *testing.T) { + t.Parallel() + + cmd := exec.CommandContext(t.Context(), "sleep", "60") + require.NoError(t, cmd.Start()) + t.Cleanup(func() { _ = cmd.Process.Kill(); _, _ = cmd.Process.Wait() }) + + assert.False(t, Exited(recordOf(t, cmd.Process.Pid))) + }) + + // A process killed by something that is not its parent is left for + // whoever reaps it. Until then its /proc entry is there and its start + // time still matches, so a liveness check alone reads it as running + // when it is only waiting to be collected. + t.Run("a zombie has", func(t *testing.T) { + t.Parallel() + + cmd := exec.CommandContext(t.Context(), "sleep", "60") + require.NoError(t, cmd.Start()) + pid := cmd.Process.Pid + path := recordOf(t, pid) + + require.NoError(t, cmd.Process.Kill()) + require.Eventually(t, func() bool { return Exited(path) }, 5*time.Second, 10*time.Millisecond, + "a killed process nothing has reaped must read as exited") + + // Still unreaped, so Alive is what it was: the two disagree, and + // that disagreement is the whole point. + assert.True(t, Alive(path), "the record still names a process /proc knows about") + + _, _ = cmd.Process.Wait() + }) + + t.Run("a process that is gone has", func(t *testing.T) { + t.Parallel() + + cmd := exec.CommandContext(t.Context(), "true") + require.NoError(t, cmd.Start()) + pid := cmd.Process.Pid + path := recordOf(t, pid) + _ = cmd.Wait() + + assert.True(t, Exited(path)) + }) + + t.Run("no record at all has", func(t *testing.T) { + t.Parallel() + + assert.True(t, Exited(filepath.Join(t.TempDir(), "absent.json"))) + }) +} diff --git a/internal/sandbox/state.go b/internal/sandbox/state.go index b31dc87..9cc8c48 100644 --- a/internal/sandbox/state.go +++ b/internal/sandbox/state.go @@ -87,9 +87,62 @@ func Alive(path string) bool { return st == s.StartTime } +// Exited reports whether the sandbox recorded at path has finished. +// +// It is not the negation of Alive, and the difference is a process that +// has been killed and not yet reaped. A zombie keeps its /proc entry and +// its start time still matches, so Alive reads it as running when it is +// only waiting to be collected. Nothing is being served by then, and +// something waiting for a sandbox to go would otherwise wait on a parent +// that may never come: the process that started a gateway is usually a +// qubesome run that exited at its terminal long ago. +// +// No record, or one naming a process /proc no longer has, is exited too. +// Both mean the same thing to a caller waiting for one to be gone. +func Exited(path string) bool { + s, err := ReadState(path) + if err != nil { + return true + } + + data, err := os.ReadFile(procStat(s.PID)) + if err != nil { + return true + } + + st, err := parseStartTime(string(data)) + if err != nil || st != s.StartTime { + // A start time that no longer matches is a different process + // wearing the same pid, so the recorded one has gone. + return true + } + + return parseZombie(string(data)) +} + +// parseZombie reports whether a stat line describes a process that has +// exited and is waiting to be reaped. +// +// Field 3 is the state character, and is the first after the command name +// for the reason parseStartTime counts from there. +func parseZombie(line string) bool { + i := strings.LastIndex(line, ")") + if i < 0 { + return false + } + + fields := strings.Fields(line[i+1:]) + + return len(fields) > 0 && fields[0] == "Z" +} + +func procStat(pid int) string { + return "/proc/" + strconv.Itoa(pid) + "/stat" +} + // startTime returns the start time of a process in clock ticks since boot. func startTime(pid int) (uint64, error) { - path := "/proc/" + strconv.Itoa(pid) + "/stat" + path := procStat(pid) data, err := os.ReadFile(path) if err != nil { From 134ae3f2c1d380180e8fdaf0cdec2e96faa5d060 Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Thu, 10 Sep 2026 23:18:23 +0100 Subject: [PATCH 15/16] gateway: describe the address record, not a gateway that is not running The subnet mismatch reported "the running gateway hands addresses out of X", which is false in the state it is most likely to be read in. The record outlives the gateway that wrote it, and gateway stop leaves exactly that behind: nothing running, and a note of the range the last one handed addresses out of. The test covering it had no gateway running either. Raised in review on #226. It now describes the record, and says what to do about it, which is the same remedy a launch is given when it refuses the same mismatch. Assisted-by: Claude Opus 5 Signed-off-by: Paulo Gomes Entire-Checkpoint: 01M26PD3GTGSJB8AR4615X32WN --- internal/gateway/status.go | 8 +++++++- internal/gateway/status_test.go | 18 ++++++++++++++---- 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/internal/gateway/status.go b/internal/gateway/status.go index e8da0f9..e8a76e9 100644 --- a/internal/gateway/status.go +++ b/internal/gateway/status.go @@ -175,9 +175,15 @@ func (g Gateway) inspectAddrs(st *Status, cfg *types.GatewayConfig) { return } + // What this describes is the record, not a gateway. The record + // outlives the gateway that wrote it, and gateway stop leaves exactly + // that behind: nothing running, and a note of the range the last one + // handed addresses out of. A status naming a running gateway would be + // false in the state it is most likely to be read in. if a.Subnet != "" && a.Subnet != subnet.String() { st.AddrProblem = fmt.Sprintf( - "the running gateway hands addresses out of %s and the config now asks for %s", + "this session has handed addresses out of %s and the config now asks for %s, "+ + "so the session has to be restarted before the new range is used", a.Subnet, subnet) return } diff --git a/internal/gateway/status_test.go b/internal/gateway/status_test.go index 164b41b..39b753a 100644 --- a/internal/gateway/status_test.go +++ b/internal/gateway/status_test.go @@ -197,8 +197,15 @@ func TestStatusReportsOnlyTheGatewayAddressWithNoGatewayRunning(t *testing.T) { assert.Contains(t, render(t, st), "addresses 10.111.0.1 is the gateway's own\n") } -// A subnet changed under a running gateway is one of the things a status is -// for, so it is reported rather than refused the way a launch refuses it. +// A subnet changed under the record is one of the things a status is for, +// so it is reported rather than refused the way a launch refuses it. +// +// What is reported describes the record and not a gateway. The record +// outlives the gateway that wrote it, and gateway stop leaves exactly +// that behind: no gateway running and a record of the addresses the last +// one handed out. Saying a running gateway hands addresses out of +// anything is false in the state this is most likely to be read in, and +// this test is in it, since nothing is running here. func TestStatusReportsASubnetTheRecordDoesNotMatch(t *testing.T) { t.Parallel() @@ -207,8 +214,11 @@ func TestStatusReportsASubnetTheRecordDoesNotMatch(t *testing.T) { st := g.Inspect(newTestSession(t), testConfig(unusableConfig()), failingReady(t)) - assert.Contains(t, st.AddrProblem, "hands addresses out of 10.112.0.0/24") - assert.Contains(t, render(t, st), "addresses the running gateway hands addresses out of 10.112.0.0/24") + require.False(t, st.Running, "the state this describes is one with no gateway in it") + assert.NotContains(t, st.AddrProblem, "running gateway", + "there is no running gateway to be handing anything out") + assert.Contains(t, st.AddrProblem, "10.112.0.0/24") + assert.Contains(t, render(t, st), "addresses this session has handed addresses out of 10.112.0.0/24") } func TestStatusReportsAnUnusableSubnet(t *testing.T) { From e13856c158bb624620ddae976a69a1387e0269e2 Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Thu, 10 Sep 2026 23:21:42 +0100 Subject: [PATCH 16/16] gateway: say the subnet mismatch the same way in both places A launch and a status report the same mismatch, and said it differently. The status one was reworded to describe the record rather than a gateway, because a status is read in the state gateway stop leaves: nothing running, and a note of the range the last one used. This one opens the same way now, so the two are recognisably about the same thing. What it keeps is the claim the status message had to drop. Allocate is only ever reached through Attached, after Up, so a gateway is running by the time this is read, and its holding the first address of the old range is the reason the remedy is a restart rather than an edit. Dropping that here would have made the message consistent and less useful. The message had no test. It has one now, since it is the whole of what a user is told when a subnet changes under a session that has already handed addresses out. Assisted-by: Claude Opus 5 Signed-off-by: Paulo Gomes Entire-Checkpoint: 01M26PK5W39Z73BV7D6PJNJTNG --- internal/gateway/run.go | 11 +++++++++-- internal/gateway/run_test.go | 25 +++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/internal/gateway/run.go b/internal/gateway/run.go index 82c04f2..d996f16 100644 --- a/internal/gateway/run.go +++ b/internal/gateway/run.go @@ -1127,10 +1127,17 @@ func (g Gateway) readAlloc(subnet netip.Prefix) (allocation, error) { return allocation{}, fmt.Errorf("failed to parse the gateway addresses %q: %w", g.AllocPath, err) } + // The opening clause is the one the status message uses, so the two + // describe the same thing in the same words. This one keeps the claim + // about a running gateway that the status message drops: Allocate is + // only reached after Up, so by here there is one, and it is the + // reason the remedy is a restart rather than an edit. A status is + // read in the state gateway stop leaves, where there is not. if a.Subnet != subnet.String() { return allocation{}, fmt.Errorf( - "this session's gateway hands addresses out of %s and the config now asks for %s: "+ - "the running gateway holds the first address of the old range, so the session has to be restarted", + "this session has handed addresses out of %s and the config now asks for %s: "+ + "the gateway running in it holds the first address of the old range, "+ + "so the session has to be restarted before the new range is used", a.Subnet, subnet) } diff --git a/internal/gateway/run_test.go b/internal/gateway/run_test.go index c9cc0f6..7a3fbcc 100644 --- a/internal/gateway/run_test.go +++ b/internal/gateway/run_test.go @@ -467,3 +467,28 @@ func prefix(t *testing.T, s string) netip.Prefix { return p } + +// A subnet changed under a session that has already handed addresses out +// is refused, because the count belongs to the old range and a gateway in +// the session holds its first address. +// +// The message opens the way the status one does, so the two describe the +// same thing in the same words. It keeps the claim about a running +// gateway that the status message drops: Allocate is only ever reached +// after Up, so by here there is one, and it is why a restart is the +// remedy rather than an edit. +func TestAllocateRefusesAChangedSubnet(t *testing.T) { + t.Parallel() + + g := newSessionGateway(t) + + _, err := g.Allocate(prefix(t, testSubnet)) + require.NoError(t, err) + + _, err = g.Allocate(prefix(t, "10.112.0.0/24")) + + require.Error(t, err) + assert.Contains(t, err.Error(), "this session has handed addresses out of 10.111.0.0/24") + assert.Contains(t, err.Error(), "the config now asks for 10.112.0.0/24") + assert.Contains(t, err.Error(), "the session has to be restarted") +}