diff --git a/cmd/cli/gateway.go b/cmd/cli/gateway.go index 06ac433..1db41b0 100644 --- a/cmd/cli/gateway.go +++ b/cmd/cli/gateway.go @@ -5,8 +5,11 @@ import ( "fmt" "os" + "github.com/qubesome/cli/internal/files" "github.com/qubesome/cli/internal/gateway" + "github.com/qubesome/cli/internal/sandbox" "github.com/qubesome/cli/internal/session" + "github.com/qubesome/cli/internal/types" "github.com/urfave/cli/v3" ) @@ -23,6 +26,12 @@ 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 + logsWorkload string +) + func gatewayCommand() *cli.Command { cmd := &cli.Command{ Name: "gateway", @@ -33,6 +42,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 +51,7 @@ next launch then starts a fresh one inside the same session. Commands: []*cli.Command{ gatewayStatusCommand(), gatewayStopCommand(), + gatewayLogsCommand(), }, } return cmd @@ -53,19 +64,63 @@ 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. +// +// It is read from the record the gateway's own launch wrote rather than +// inferred from whichever profiles happen to be active. Inference answered +// wrongly exactly when it mattered: a gateway is session wide and outlives +// the profile that started it, so once that profile has stopped there is +// nothing left among the active ones to point at, and the nearest config +// is a guess that names an image, a policy and a subnet the running +// gateway need have nothing to do with. +// +// Not profileConfigOrDefault, for the same reason. Its fallbacks are right +// for a launch, which is choosing a config to act on, and wrong here, +// where the question is which config something already running came from. +// +// With no record and no gateway running, the user-level file is the +// answer: there is nothing whose provenance could be got wrong, and what a +// status then describes is the gateway this host would start. +func sessionConfig() (*types.Config, string) { + g := gateway.Current() + + if path, ok := g.RecordedConfig(); ok { + if cfg := config(path); cfg != nil { + return cfg, "" + } + + return nil, fmt.Sprintf( + "the running gateway was started from %s, which no longer reads as a config, "+ + "so the image, policy and subnet it names are unknown", path) + } + + // A gateway with no record is one started before qubesome kept one, or + // one whose record could not be written. Either way nothing here can + // say where it came from, and a nearby config would be a guess. + if sandbox.Alive(files.GatewayStatePath()) { + return nil, "the running gateway has no record of the config it was started from, " + + "so the image, policy and subnet it names are unknown" + } + + return profileConfigOrDefault(""), "" +} + func gatewayStopCommand() *cli.Command { return &cli.Command{ Name: "stop", @@ -89,3 +144,71 @@ 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 +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{ + Name: "follow", + Aliases: []string{"f"}, + 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 matching lines", + Destination: &logsLast, + }, + }, + Action: func(ctx context.Context, _ *cli.Command) error { + return gateway.ShowLogs(ctx, os.Stdout, gateway.LogOptions{ + Path: files.GatewayLogPath(), + Profile: targetProfile, + Workload: logsWorkload, + Last: logsLast, + Follow: logsFollow, + }) + }, + } +} diff --git a/cmd/cli/gateway_test.go b/cmd/cli/gateway_test.go new file mode 100644 index 0000000..9e821f3 --- /dev/null +++ b/cmd/cli/gateway_test.go @@ -0,0 +1,55 @@ +package cli + +import ( + "os" + "path/filepath" + "testing" + + "github.com/qubesome/cli/internal/gateway" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// The gateway belongs to the session and not to a profile, and it outlives +// the profile that started it, so its provenance is read from the record +// that launch wrote rather than inferred from whichever profiles happen to +// be active now. +func TestRecordedConfigReadsBackWhatWasWritten(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + g := gateway.Gateway{ConfigPath: filepath.Join(dir, "gateway-config")} + + require.NoError(t, os.WriteFile(g.ConfigPath, []byte("/home/u/dotfiles/qubesome.yaml\n"), 0o600)) + + got, ok := g.RecordedConfig() + assert.True(t, ok) + assert.Equal(t, "/home/u/dotfiles/qubesome.yaml", got) +} + +// No record is not an empty answer. A caller has to tell the two apart, +// because one means the provenance is unknown and the other would name a +// config called "". +func TestRecordedConfigWithNoRecord(t *testing.T) { + t.Parallel() + + g := gateway.Gateway{ConfigPath: filepath.Join(t.TempDir(), "gateway-config")} + + got, ok := g.RecordedConfig() + assert.False(t, ok) + assert.Empty(t, got) +} + +// A record that was created but never filled in says nothing, and must not +// read as a config whose path is empty. +func TestRecordedConfigWithAnEmptyRecord(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + g := gateway.Gateway{ConfigPath: filepath.Join(dir, "gateway-config")} + + require.NoError(t, os.WriteFile(g.ConfigPath, []byte("\n"), 0o600)) + + _, ok := g.RecordedConfig() + assert.False(t, ok) +} diff --git a/cmd/cli/host_run.go b/cmd/cli/host_run.go index c2c8007..4a7db62 100644 --- a/cmd/cli/host_run.go +++ b/cmd/cli/host_run.go @@ -3,11 +3,91 @@ package cli import ( "context" "fmt" + "os" "os/exec" + "slices" + "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. +// +// 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 +// 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 namesTheLaunchingSession(e) { + continue + } + env = append(env, e) + } + + return append(env, + "DISPLAY=:"+strconv.Itoa(int(display)), + "XAUTHORITY="+cookie, + ) +} + +// 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", @@ -37,12 +117,28 @@ 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)) - out, err := c.CombinedOutput() - fmt.Println(string(out)) + c.Env = hostRunEnv(os.Environ(), prof.Display, cookie) + + // 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 err + return nil }, } return cmd diff --git a/cmd/cli/host_run_test.go b/cmd/cli/host_run_test.go new file mode 100644 index 0000000..7558ed4 --- /dev/null +++ b/cmd/cli/host_run_test.go @@ -0,0 +1,71 @@ +package cli + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "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") +} + +// 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") +} diff --git a/cmd/cli/root.go b/cmd/cli/root.go index fcb9af6..f65c9f3 100644 --- a/cmd/cli/root.go +++ b/cmd/cli/root.go @@ -49,14 +49,24 @@ func RootCommand() *cli.Command { superviseCommand(), sessionHoldCommand(), gatewayCommand(), + tunnelCommand(), vmInitCommand(), consoleCommand(), }, } + // This runs ahead of every command, and several of them have a caller + // reading their standard output as data rather than as text for a + // person: tunnel puts an ssh connection through it, completion is + // eval'd by a shell, and clipboard writes back what was pasted. A + // warning on stdout is read as the far end talking, as shell input, or + // as part of the paste, so it goes to stderr for the same reason + // main.go sends a returned error there. It is still seen: stderr is + // where ssh shows what a ProxyCommand says. cmd.Before = func(ctx context.Context, c *cli.Command) (context.Context, error) { if strings.EqualFold(os.Getenv("XDG_SESSION_TYPE"), "wayland") { - fmt.Println("\033[33mWARN: Running qubesome in Wayland is experimental. Some features may not work as expected.\033[0m") + fmt.Fprintln(os.Stderr, + "\033[33mWARN: Running qubesome in Wayland is experimental. Some features may not work as expected.\033[0m") } return ctx, nil } @@ -98,6 +108,7 @@ func config(path string) *types.Config { if err != nil { return nil } + cfg.Source = path cfg.RootDir = filepath.Dir(path) return cfg diff --git a/cmd/cli/tunnel.go b/cmd/cli/tunnel.go new file mode 100644 index 0000000..f7ee607 --- /dev/null +++ b/cmd/cli/tunnel.go @@ -0,0 +1,60 @@ +package cli + +import ( + "context" + "fmt" + "os" + + "github.com/qubesome/cli/internal/gateway" + "github.com/urfave/cli/v3" +) + +// tunnelCommand reaches a host through the session gateway's proxy. +// +// It is hidden for the reason supervise and profile-display are: it runs +// inside a sandbox and is qubesome calling itself. What calls it is the +// ProxyCommand qubesome writes into the sandbox's ssh config, and there is +// nothing outside a sandbox for it to do, because the endpoint it needs is +// only in a workload's environment. +// +// It is here rather than left to socat because the sandbox already has the +// qubesome binary, having been given it to be wired to the gateway at all, +// and does not have socat. Doing it here also keeps the CONNECT exchange +// in something that can be tested, rather than in a config string nothing +// reads until an ssh fails. +func tunnelCommand() *cli.Command { + cmd := &cli.Command{ + Name: "tunnel", + Hidden: true, + Usage: "carries a connection to a host through the session gateway", + Description: `Not intended to be called directly. qubesome points a workload's +ssh at it, as + + ProxyCommand qubesome tunnel %h %p + +The gateway drops every port but 80, 443 and 53, so a workload reaches +anything else by asking the gateway's proxy to carry it. The endpoint to +ask is in QUBESOME_GATEWAY_PROXY, which a launch puts in the environment +of every workload it attaches to a gateway. + +The connection is carried on standard input and output, so this speaks +whatever the client and the host speak and understands none of it. A host +the gateway's policy does not allow is refused by the gateway, and the +refusal is reported here with what it said. +`, + Action: func(ctx context.Context, cmd *cli.Command) error { + args := cmd.Args().Slice() + if len(args) != 2 { + return fmt.Errorf("usage: qubesome tunnel ") + } + + proxy, err := gateway.ProxyFromEnv() + if err != nil { + return err + } + + return gateway.Tunnel(ctx, proxy, args[0], args[1], os.Stdin, os.Stdout) + }, + } + return cmd +} diff --git a/cmd/qubesome/main.go b/cmd/qubesome/main.go index c1a3f92..5234747 100644 --- a/cmd/qubesome/main.go +++ b/cmd/qubesome/main.go @@ -11,8 +11,12 @@ import ( func main() { cmd := cli.RootCommand() + // Not stdout. Some commands have a caller reading their standard + // output as data rather than as text for a person: tunnel puts an ssh + // connection through it, and a diagnostic written there would be read + // as the far end talking. if err := cmd.Run(context.Background(), os.Args); err != nil { - fmt.Println(err) + fmt.Fprintln(os.Stderr, err) os.Exit(1) } } 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/files/files.go b/internal/files/files.go index 6dba4b1..8290786 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. // @@ -282,6 +294,25 @@ func GatewayAllocPath() string { return filepath.Join(SessionDir(), "gateway-addresses.json") } +// GatewayConfigPath returns the record of which qubesome config the +// running gateway was started from. +// +// It sits beside the gateway's state file because it has the same +// lifetime, and that lifetime is the whole point of it. A gateway +// describes itself with the image, policy and subnet one config named, and +// once the profile that started it has stopped there is nothing left on +// the host to say which config that was. Inferring it from whichever +// profiles happen to be active answers wrongly exactly when it matters: +// after things have gone quiet. +// +// It is not in the home directory for the same reason. A record that +// outlives the session outlives the gateway it describes, so after a +// reboot it would name a config from a session that no longer exists, +// which is a guess wearing the clothes of a fact. +func GatewayConfigPath() string { + return filepath.Join(SessionDir(), "gateway-config") +} + // GatewayCredsPath returns the client half of the control channel's mTLS // material. // diff --git a/internal/gateway/attach.go b/internal/gateway/attach.go index 24e9fd2..21db136 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" ) @@ -52,7 +54,7 @@ func Attached(cfg *types.Config, network string) (*Attach, error) { } g := Current() - if err := g.Up(*cfg.Gateway, cfg.RootDir); err != nil { + if err := g.Up(*cfg.Gateway, cfg.RootDir, cfg.Source); err != nil { return nil, err } @@ -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/logline.go b/internal/gateway/logline.go new file mode 100644 index 0000000..f68cbec --- /dev/null +++ b/internal/gateway/logline.go @@ -0,0 +1,138 @@ +package gateway + +import ( + "strconv" + "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 what it + // cannot write plainly within it. + for i := 1; i < len(rest); i++ { + switch rest[i] { + case '\\': + // Whatever follows a backslash is part of the escape and + // cannot end the value, whether it is a quote or another + // backslash. + i++ + case '"': + token := rest[:i+1] + + // Undone rather than copied through. The escapes are + // Go's own, so a value holding a newline is written as + // one holding a backslash and an n, and passing that on + // would report a different message than was logged. + if v, err := strconv.Unquote(token); err == nil { + return v + } + + // A token slog did not write, or one this cut short. + // What is between the quotes is the best left to say. + return token[1:i] + } + } + + // No closing quote, so there is no value here to read. + return "" + } + + 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..7b5d9b5 --- /dev/null +++ b/internal/gateway/logline_test.go @@ -0,0 +1,129 @@ +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. +// slog escapes a value it quotes, so what is between the quotes is not +// the value: a message holding a newline reads as one holding an n until +// the escapes are undone. +func TestLogFieldUndoesEscapes(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + line string + want string + }{ + { + name: "a newline", + line: `time=1 level=ERROR msg="dial failed\nretry" host=example.com`, + want: "dial failed\nretry", + }, + { + name: "a tab", + line: `time=1 level=ERROR msg="one\ttwo"`, + want: "one\ttwo", + }, + { + name: "an escaped quote", + line: `time=1 level=ERROR msg="he said \"no\"" host=example.com`, + want: `he said "no"`, + }, + { + name: "a backslash", + line: `time=1 level=ERROR msg="one\\two"`, + want: `one\two`, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + assert.Equal(t, tc.want, logField(tc.line, "msg")) + }) + } +} + +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 new file mode 100644 index 0000000..0503c4f --- /dev/null +++ b/internal/gateway/logs.go @@ -0,0 +1,392 @@ +package gateway + +import ( + "bufio" + "context" + "errors" + "fmt" + "io" + "log/slog" + "os" + "strings" + "time" + + "github.com/qubesome/cli/internal/files" +) + +// followInterval is how often a follow looks for more of the log. +// +// 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 + +// 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 + + // 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. + 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) { + // Not "no gateway has been started". A gateway from an + // older qubesome runs without writing this file, so what + // can be said is that the log is not there to read. + return fmt.Errorf("there is no gateway log at %s to read", path) + } + + return fmt.Errorf("failed to open the gateway log %q: %w", path, err) + } + defer f.Close() + + selects := selector(opts.Profile, opts.Workload) + + // The tail is whatever the gateway was in the middle of writing. It + // is not printed here and not thrown away either: a follow joins it + // to the rest when the rest is written. + tail, err := writeTail(w, f, opts.Last, selects) + if err != nil { + return err + } + + if !opts.Follow { + return nil + } + + return follow(ctx, w, f, selects, tail) +} + +// writeTail writes the lines of f that selects accepts: all of them, or +// only the final few when last is set. It returns the unterminated line +// the gateway was still writing, which it does not print. +// +// 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, selects func(string) bool) (string, error) { + reader := bufio.NewReader(f) + + // A ring of the last lines wanted, so a log far larger than the + // answer is not held in memory to produce it. It grows as lines are + // read rather than being sized to last: that number is typed at a + // terminal, and reserving it before a line has been read turns a + // large enough -n into a way to bring qubesome down. + // + // next is where the oldest line sits once the ring is full, which is + // also where the next one overwrites it. Nothing is shifted along. + var ring []string + var next int + + var tail string + + for { + line, terminated, ok, err := readLine(reader) + if err != nil { + return "", fmt.Errorf("failed to read the gateway log: %w", err) + } + + if !terminated { + if ok { + tail = line + } + + break + } + + // A line too long to hold was discarded as it was read, so there + // is nothing to match a filter against or to print. + if !ok || !selects(line) { + continue + } + + if last <= 0 { + if err := writeLine(w, line); err != nil { + return "", err + } + + continue + } + + if len(ring) < last { + ring = append(ring, line) + + continue + } + + ring[next] = line + next = (next + 1) % last + } + + for i := range ring { + // next is 0 until the ring has filled, so this is the order the + // lines were read in either way. + if err := writeLine(w, ring[(next+i)%len(ring)]); err != nil { + return "", err + } + } + + return tail, nil +} + +// readLine returns the next line of r, without its newline. +// +// terminated is false when the line has no newline yet, which means the +// gateway is still writing it: what comes back is the part written so +// far, and the rest arrives on a later read. +// +// ok is false when the line was longer than maxLineLen. Such a line is +// discarded as it is read rather than returned, so a gateway writing +// without newlines cannot make a reader of its log hold the whole of it. +func readLine(r *bufio.Reader) (line string, terminated, ok bool, err error) { + var b strings.Builder + + held := true + + for { + // ReadSlice stops at the end of the buffer rather than growing + // one, so what is read in a turn is bounded whatever the writer + // is doing. + chunk, err := r.ReadSlice('\n') + + if len(chunk) > 0 { + if held && b.Len()+len(chunk) > maxLineLen { + held = false + + b.Reset() + } + if held { + b.Write(chunk) + } + } + + switch { + case err == nil: + return strings.TrimSuffix(b.String(), "\n"), true, held, nil + case errors.Is(err, bufio.ErrBufferFull): + continue + case errors.Is(err, io.EOF): + return b.String(), false, held, nil + default: + return "", false, false, err + } + } +} + +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 nil +} + +// 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, tail string) error { + ticker := time.NewTicker(followInterval) + defer ticker.Stop() + + reader := bufio.NewReader(f) + + var pending strings.Builder + pending.WriteString(tail) + + // What the log had been grown to by the time the initial read + // finished. A log shorter than this later is a different log: the + // next gateway truncated this same path and started again. + size, err := f.Seek(0, io.SeekCurrent) + if err != nil { + return fmt.Errorf("failed to find the end of the gateway log: %w", err) + } + + 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: + restarted, err := rewound(f, size) + if err != nil { + return err + } + if restarted { + // Nothing of the old log is worth carrying over, least + // of all half a line of it. + reader.Reset(f) + pending.Reset() + } + + if err := followOnce(w, reader, &pending, selects); err != nil { + return err + } + + if size, err = f.Seek(0, io.SeekCurrent); err != nil { + return fmt.Errorf("failed to find the end of the gateway log: %w", err) + } + } + } +} + +// rewound reports whether the log has been replaced by a shorter one, and +// puts f back at the start when it has. +// +// A gateway truncates the log it inherits rather than writing to a new +// path, so a follow that outlives one gateway is reading the next one's +// log through a descriptor still positioned at the end of the last one. +// Everything the new gateway said before it had said as much as the old +// one did would be stepped over. +func rewound(f *os.File, size int64) (bool, error) { + fi, err := f.Stat() + if err != nil { + return false, fmt.Errorf("failed to look at the gateway log: %w", err) + } + + if fi.Size() >= size { + return false, nil + } + + if _, err := f.Seek(0, io.SeekStart); err != nil { + return false, fmt.Errorf("failed to go back to the start of the gateway log: %w", err) + } + + return true, nil +} + +// 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 { + line, terminated, ok, err := readLine(reader) + if err != nil { + return fmt.Errorf("failed to read the gateway log: %w", err) + } + + // A read that stopped short of a newline is a line the gateway + // has not finished writing. It is held until it has. + if !terminated { + if !ok || pending.Len()+len(line) > maxLineLen { + pending.Reset() + + return nil + } + pending.WriteString(line) + + return nil + } + + if pending.Len() > 0 { + line = pending.String() + line + pending.Reset() + } + + if !ok || !selects(line) { + continue + } + if err := writeLine(w, line); err != nil { + return 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) { + // O_APPEND as well as O_TRUNC. The uplink writes to this same file + // through a descriptor of its own, and a write that is not an append + // goes to wherever this descriptor's offset has reached, which is + // behind whatever the uplink has added since. Both writers append, so + // neither lands on the other. + f, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC|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 +} + +// 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 new file mode 100644 index 0000000..1451305 --- /dev/null +++ b/internal/gateway/logs_test.go @@ -0,0 +1,432 @@ +package gateway + +import ( + "bytes" + "context" + "math" + "os" + "path/filepath" + "strings" + "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)) +} + +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) +} + +// 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) }) +} + +// -n is a number a user types, and holding a slice of that size before a +// line has been read makes the log unreadable by asking for it. +func TestShowLogsDoesNotAllocateWhatWasAskedFor(t *testing.T) { + t.Parallel() + + path := writeLog(t, "one", "two", "three") + + var buf bytes.Buffer + err := ShowLogs(t.Context(), &buf, LogOptions{Path: path, Last: math.MaxInt}) + require.NoError(t, err) + + assert.Equal(t, "one\ntwo\nthree\n", buf.String()) +} + +// The last line of a log being written has no newline yet. Printing it and +// then carrying on from the end splits one record into two, so it is held +// back and joined to the rest when the rest arrives. +func TestShowLogsHoldsALineStillBeingWritten(t *testing.T) { + t.Parallel() + + path := filepath.Join(t.TempDir(), "gateway.log") + require.NoError(t, os.WriteFile(path, []byte("whole\npart"), 0o600)) + + var buf bytes.Buffer + require.NoError(t, ShowLogs(t.Context(), &buf, LogOptions{Path: path})) + + assert.Equal(t, "whole\n", buf.String()) +} + +// A follow that outlives one gateway reads the next one's log, which +// starts again at nothing. Staying at the old offset skips everything the +// new gateway said until it has said as much as the old one did. +func TestShowLogsFollowsAcrossARestart(t *testing.T) { + t.Parallel() + + path := writeLog(t, "old one", "old two", "old three") + + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + + buf := &syncBuffer{} + + done := make(chan error, 1) + go func() { done <- ShowLogs(ctx, buf, LogOptions{Path: path, Follow: true}) }() + + require.Eventually(t, func() bool { + return strings.Contains(buf.String(), "old three") + }, 2*time.Second, 10*time.Millisecond) + + // What a new gateway does to the log it inherits. + f, err := openLog(path) + require.NoError(t, err) + _, err = f.WriteString("fresh\n") + require.NoError(t, err) + require.NoError(t, f.Close()) + + require.Eventually(t, func() bool { + return strings.Contains(buf.String(), "fresh") + }, 3*time.Second, 10*time.Millisecond) + + cancel() + require.NoError(t, <-done) +} + +// Both writers have to append. The sandbox's descriptor carries its own +// offset, so without it the uplink's lines are overwritten by whatever the +// sandbox says next. +func TestOpenLogAppends(t *testing.T) { + t.Parallel() + + path := filepath.Join(t.TempDir(), "gateway.log") + + sandboxLog, err := openLog(path) + require.NoError(t, err) + defer sandboxLog.Close() + + _, err = sandboxLog.WriteString("from the sandbox\n") + require.NoError(t, err) + + uplink, err := appendLog(path) + require.NoError(t, err) + _, err = uplink.WriteString("from the uplink\n") + require.NoError(t, err) + require.NoError(t, uplink.Close()) + + _, err = sandboxLog.WriteString("from the sandbox again\n") + require.NoError(t, err) + + got, err := os.ReadFile(path) + require.NoError(t, err) + assert.Equal(t, "from the sandbox\nfrom the uplink\nfrom the sandbox again\n", string(got)) +} 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 1c626a0..7baef75 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" @@ -93,6 +104,7 @@ type Gateway struct { StatePath string AllocPath string CredsPath string + ConfigPath string Socket string SocketDir string SecretsDir string @@ -106,6 +118,7 @@ func Current() Gateway { StatePath: files.GatewayStatePath(), AllocPath: files.GatewayAllocPath(), CredsPath: files.GatewayCredsPath(), + ConfigPath: files.GatewayConfigPath(), Socket: files.GatewaySocket(), SocketDir: files.GatewaySocketDir(), SecretsDir: files.GatewaySecretsDir(), @@ -115,10 +128,10 @@ func Current() Gateway { // Up makes sure the session's gateway is running and returns once it is // ready to police traffic. // -// cfg is the gateway block of the qubesome config and root is the directory +// cfg is the gateway block of the qubesome config, root is the directory // that config was read from, which is what the policy file path is resolved -// against. -func (g Gateway) Up(cfg types.GatewayConfig, root string) error { +// against, and source is the config file itself. +func (g Gateway) Up(cfg types.GatewayConfig, root, source string) error { if err := os.MkdirAll(g.Dir, files.DirMode); err != nil { return fmt.Errorf("failed to create the session dir %q: %w", g.Dir, err) } @@ -128,6 +141,15 @@ func (g Gateway) Up(cfg types.GatewayConfig, root string) error { return err } + // Only when this launch created the gateway. A launch that found one + // running reuses it whatever config it itself came from, so recording + // its own here would rename a gateway that has not changed. That is + // the whole difference between the config a gateway came from and the + // last config anything opened. + if started { + g.recordConfig(source) + } + if err := g.ready(); err != nil { return err } @@ -250,6 +272,51 @@ func (g Gateway) startOnce(cfg types.GatewayConfig, root string) (bool, error) { return true, nil } +// recordConfig notes which config the gateway now running was started +// from. +// +// A failure is a warning and nothing more. The gateway is up by this point +// and policing traffic, and a record qubesome could not write is a status +// command that has to say it cannot name the config. That is a worse +// report, not a broken session, and it is not worth refusing a launch the +// user asked for. +// +// An empty source writes nothing. A config that was never read from a file +// has no path to record, and an empty record would read as one that could +// not be written rather than as one that never applied. +func (g Gateway) recordConfig(source string) { + if source == "" { + return + } + + if err := os.WriteFile(g.ConfigPath, []byte(source+"\n"), files.FileMode); err != nil { + slog.Warn("failed to record which config the gateway was started from", + "path", g.ConfigPath, "config", source, "error", err) + } +} + +// RecordedConfig returns the config the running gateway was started from, +// and whether there is a record of one. +// +// It is the only thing on the host that can answer, once the profile that +// started the gateway has stopped. A caller that gets false has to say the +// provenance is unknown rather than reach for whichever config is nearest: +// a gateway describes itself with an image, a policy and a subnet, and +// naming the wrong config names three wrong things. +func (g Gateway) RecordedConfig() (string, bool) { + data, err := os.ReadFile(g.ConfigPath) + if err != nil { + return "", false + } + + path := strings.TrimSpace(string(data)) + if path == "" { + return "", false + } + + return path, true +} + // acquire takes the gateway lock and returns the file that holds it. // // LOCK_EX and not LOCK_EX|LOCK_NB, which is the opposite of the session @@ -394,8 +461,27 @@ 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. + // + // 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 err = cmd.Start() @@ -691,8 +777,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 closeLog(log) + cmd.Stdout = log + cmd.Stderr = log if err := cmd.Start(); err != nil { return nil, err @@ -1088,10 +1183,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..f071552 100644 --- a/internal/gateway/run_test.go +++ b/internal/gateway/run_test.go @@ -103,7 +103,7 @@ func TestUpDoesNotStartASecondGatewayWhenOneIsRunning(t *testing.T) { require.NoError(t, sandbox.WriteState(g.StatePath, os.Getpid())) - require.NoError(t, g.Up(unusableConfig(), t.TempDir())) + require.NoError(t, g.Up(unusableConfig(), t.TempDir(), "")) assert.Equal(t, 1, gw.reloaded()) } @@ -122,7 +122,7 @@ func TestUpAcceptsAGatewayThatCannotReload(t *testing.T) { require.NoError(t, sandbox.WriteState(g.StatePath, os.Getpid())) - assert.NoError(t, g.Up(unusableConfig(), t.TempDir())) + assert.NoError(t, g.Up(unusableConfig(), t.TempDir(), "")) } // A state file outlives the process it names, so a gateway that crashed must @@ -138,7 +138,7 @@ func TestUpStartsAGatewayWhenTheStateIsStale(t *testing.T) { state := fmt.Sprintf(`{"pid":%d,"startTime":1}`, os.Getpid()) require.NoError(t, os.WriteFile(g.StatePath, []byte(state), 0o600)) - err := g.Up(unusableConfig(), t.TempDir()) + err := g.Up(unusableConfig(), t.TempDir(), "") require.Error(t, err) assert.Contains(t, err.Error(), "gateway.yml") @@ -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") +} 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) { diff --git a/internal/gateway/stop.go b/internal/gateway/stop.go index 84afa90..a059b4a 100644 --- a/internal/gateway/stop.go +++ b/internal/gateway/stop.go @@ -3,8 +3,10 @@ package gateway import ( "errors" "fmt" + "log/slog" "os" "syscall" + "time" "github.com/qubesome/cli/internal/files" "github.com/qubesome/cli/internal/sandbox" @@ -52,6 +54,8 @@ func (g Gateway) Stop() (int, error) { return 0, fmt.Errorf("failed to remove the gateway state %q: %w", g.StatePath, err) } + g.forgetConfig() + return 0, nil } @@ -73,6 +77,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. @@ -80,5 +93,58 @@ func (g Gateway) Stop() (int, error) { return st.PID, fmt.Errorf("failed to remove the gateway state %q: %w", g.StatePath, err) } + g.forgetConfig() + return st.PID, nil } + +// forgetConfig drops the note of which config the gateway came from. +// +// It goes with the state file, because the two describe the same gateway +// and a record that outlived it would name the provenance of something +// that is no longer running. A failure is a warning: the next gateway to +// start overwrites this, so what is left behind is a stale path that only +// a status asked between the two would read, and saying so is better than +// failing a stop that otherwise worked. +func (g Gateway) forgetConfig() { + if err := os.Remove(g.ConfigPath); err != nil && !errors.Is(err, os.ErrNotExist) { + slog.Warn("failed to remove the gateway's config record", + "path", g.ConfigPath, "error", err) + } +} + +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/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") +} diff --git a/internal/gateway/tunnel.go b/internal/gateway/tunnel.go new file mode 100644 index 0000000..d617ef5 --- /dev/null +++ b/internal/gateway/tunnel.go @@ -0,0 +1,160 @@ +package gateway + +import ( + "bufio" + "context" + "errors" + "fmt" + "io" + "net" + "net/http" + "os" + "strconv" + "strings" +) + +// ProxyEnv names the variable a sandbox is told its proxy endpoint in. +// +// It is the whole endpoint and not only the address, because the port +// belongs to the gateway image rather than to qubesome. A workload could +// work the address out for itself, since it is already its default route +// and its resolver, and it could not work out the port at all. +const ProxyEnv = "QUBESOME_GATEWAY_PROXY" + +// ProxyFromEnv returns the endpoint this sandbox asks for a tunnel on. +// +// An absent variable means this workload has no gateway, which is not a +// thing a default could stand in for: there would be nothing listening at +// whatever was guessed. +func ProxyFromEnv() (string, error) { + addr := os.Getenv(ProxyEnv) + if addr == "" { + return "", fmt.Errorf("%s is not set, so this workload has no gateway to ask for a tunnel", ProxyEnv) + } + + return addr, nil +} + +// Tunnel joins in and out to host:port through the proxy at proxyAddr. +// +// The gateway drops every port but 80, 443 and 53, so a workload reaches +// anything else by asking it for a tunnel rather than by connecting out. +// This is that ask, in the shape ssh wants it: a command that speaks the +// target on its own standard input and output, so +// +// ProxyCommand qubesome tunnel %h %p +// +// is the whole of what a workload needs to reach a host it is allowed to. +// +// It returns when the target is done, not when the workload stops writing. +// A client that has said all it has to say still has an answer coming, and +// ending at its end of the copy would cut that answer off. +func Tunnel(ctx context.Context, proxyAddr, host, port string, in io.Reader, out io.Writer) error { + target, err := checkTarget(host, port) + if err != nil { + return err + } + + var d net.Dialer + + conn, err := d.DialContext(ctx, "tcp", proxyAddr) + if err != nil { + return fmt.Errorf("failed to reach the gateway proxy at %s: %w", proxyAddr, err) + } + defer conn.Close() + + // Any bytes the target has already sent are in this reader and not in + // the socket, because reading the response head is what put them + // there. Reading from the socket after this point loses them, and for + // ssh those bytes are the server's banner. + br := bufio.NewReader(conn) + + if err := connect(conn, br, target); err != nil { + return err + } + + go func() { + _, _ = io.Copy(conn, in) + + // The target learns that the workload has finished only if the + // write half is closed. Without this a server waiting for the + // end of a request waits for the tunnel to be torn down instead. + if c, ok := conn.(*net.TCPConn); ok { + _ = c.CloseWrite() + } + }() + + if _, err := io.Copy(out, br); err != nil { + return fmt.Errorf("failed while carrying %s: %w", target, err) + } + + return nil +} + +// connect asks the proxy for a tunnel to target and reports whether it +// gave one. +func connect(conn net.Conn, br *bufio.Reader, target string) error { + req := "CONNECT " + target + " HTTP/1.1\r\nHost: " + target + "\r\n\r\n" + if _, err := io.WriteString(conn, req); err != nil { + return fmt.Errorf("failed to ask the gateway for a tunnel to %s: %w", target, err) + } + + // A response to CONNECT carries no body of its own, so this reads the + // head and stops, leaving the target's own bytes in br. + resp, err := http.ReadResponse(br, &http.Request{Method: http.MethodConnect}) + if err != nil { + return fmt.Errorf("the gateway gave no usable answer for %s: %w", target, err) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + // The gateway says why in the body, and why is the whole of what + // is useful here: a refusal is its policy, not a fault. + reason, _ := io.ReadAll(io.LimitReader(resp.Body, 512)) + + said := strings.TrimSpace(string(reason)) + if said == "" { + said = resp.Status + } + + return fmt.Errorf("the gateway refused a tunnel to %s with %d: %s", target, resp.StatusCode, said) + } + + return nil +} + +// checkTarget returns host and port in the one form CONNECT accepts, and +// refuses anything that would not survive being written into a request. +// +// The target reaches here from an ssh command line, which is a workload's +// to choose, and it goes straight into a request head. A host carrying a +// line ending would end that head early and let whatever follows be read +// as a second request of the workload's own writing, so the check is on +// what the proxy would parse rather than on what a hostname may contain. +func checkTarget(host, port string) (string, error) { + if host == "" { + return "", errors.New("not a target: no host was given") + } + + // A colon is not among these. An IPv6 literal is made of them, and + // JoinHostPort below brackets one so that the target stays a host and + // a port however many colons the host holds. + if strings.ContainsAny(host, "\r\n\t ") || strings.ContainsFunc(host, isControl) { + return "", fmt.Errorf("not a target: host %q holds a character a request head cannot carry", host) + } + + n, err := strconv.Atoi(port) + if err != nil { + return "", fmt.Errorf("not a target: port %q is not a number", port) + } + if n < 1 || n > 65535 { + return "", fmt.Errorf("not a target: port %d is outside 1 to 65535", n) + } + + return net.JoinHostPort(host, strconv.Itoa(n)), nil +} + +// isControl reports whether r is a character a request head cannot carry. +func isControl(r rune) bool { + return r < 0x20 || r == 0x7f +} diff --git a/internal/gateway/tunnel_test.go b/internal/gateway/tunnel_test.go new file mode 100644 index 0000000..6e4b6e3 --- /dev/null +++ b/internal/gateway/tunnel_test.go @@ -0,0 +1,164 @@ +package gateway + +import ( + "bufio" + "bytes" + "io" + "net" + "net/http" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// fakeProxy answers one CONNECT on a loopback listener and returns the +// address to ask. handle is given the target the request named and the +// connection, positioned just after the request head. +func fakeProxy(t *testing.T, handle func(target string, c net.Conn)) string { + t.Helper() + + var lc net.ListenConfig + + ln, err := lc.Listen(t.Context(), "tcp", "127.0.0.1:0") + require.NoError(t, err) + t.Cleanup(func() { _ = ln.Close() }) + + go func() { + c, err := ln.Accept() + if err != nil { + return + } + defer c.Close() + + req, err := http.ReadRequest(bufio.NewReader(c)) + if err != nil { + return + } + + handle(req.RequestURI, c) + }() + + return ln.Addr().String() +} + +// The tunnel is a pipe once the proxy has answered, so what the workload +// writes reaches the target and what the target says reaches the workload. +// +// The server's first bytes ride in the same write as the response head +// here, which is what a real one does and is exactly what a reader that +// buffers that head will swallow if it then reads from the socket instead +// of from its own buffer. +func TestTunnelCarriesBytesBothWays(t *testing.T) { + t.Parallel() + + sent := make(chan string, 1) + addr := fakeProxy(t, func(_ string, c net.Conn) { + _, _ = io.WriteString(c, "HTTP/1.1 200 Connection Established\r\n\r\nSSH-2.0-server\r\n") + + b, _ := io.ReadAll(c) + sent <- string(b) + }) + + var out bytes.Buffer + err := Tunnel(t.Context(), addr, "github.com", "22", strings.NewReader("SSH-2.0-client\r\n"), &out) + require.NoError(t, err) + + assert.Equal(t, "SSH-2.0-server\r\n", out.String()) + assert.Equal(t, "SSH-2.0-client\r\n", <-sent) +} + +// The proxy is told which host and port to reach, in the one form it +// accepts. +func TestTunnelAsksForTheTarget(t *testing.T) { + t.Parallel() + + asked := make(chan string, 1) + addr := fakeProxy(t, func(target string, c net.Conn) { + asked <- target + _, _ = io.WriteString(c, "HTTP/1.1 200 Connection Established\r\n\r\n") + }) + + require.NoError(t, Tunnel(t.Context(), addr, "github.com", "22", strings.NewReader(""), io.Discard)) + assert.Equal(t, "github.com:22", <-asked) +} + +// A refusal is the gateway's policy answering, and it is the whole reason +// a workload cannot reach something. Saying which target was refused and +// what the proxy said is what turns it into something actionable. +func TestTunnelReportsARefusal(t *testing.T) { + t.Parallel() + + addr := fakeProxy(t, func(_ string, c net.Conn) { + _, _ = io.WriteString(c, "HTTP/1.1 403 Forbidden\r\nContent-Length: 10\r\n\r\nForbidden\n") + }) + + err := Tunnel(t.Context(), addr, "gitlab.com", "22", strings.NewReader(""), io.Discard) + require.Error(t, err) + assert.Contains(t, err.Error(), "gitlab.com:22") + assert.Contains(t, err.Error(), "403") +} + +// A host is put into a request head, so one carrying a line ending could +// write a second request of its own choosing to the proxy. The target +// comes from an ssh command line, which is a place a workload chooses, so +// it is checked before anything is dialled rather than trusted. +func TestTunnelRejectsATargetThatCouldForgeARequest(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + host string + port string + }{ + {"a newline in the host", "github.com\r\nCONNECT evil:22 HTTP/1.1", "22"}, + {"a bare newline", "github.com\nx", "22"}, + {"a space in the host", "github.com evil", "22"}, + {"an empty host", "", "22"}, + {"a port that is not a number", "github.com", "22\r\nx"}, + {"a port out of range", "github.com", "70000"}, + {"an empty port", "github.com", ""}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + // An address nothing is listening on, so a dial that should + // not happen fails as a dial and not as this. + err := Tunnel(t.Context(), "127.0.0.1:1", tc.host, tc.port, strings.NewReader(""), io.Discard) + require.Error(t, err) + assert.Contains(t, err.Error(), "not a target") + }) + } +} + +// A proxy that is not there is the gateway not being there, which is a +// different thing from one that refused. +func TestTunnelReportsAProxyItCannotReach(t *testing.T) { + t.Parallel() + + err := Tunnel(t.Context(), "127.0.0.1:1", "github.com", "22", strings.NewReader(""), io.Discard) + require.Error(t, err) + assert.Contains(t, err.Error(), "127.0.0.1:1") +} + +// The endpoint comes from the sandbox's environment, where the launch put +// it. Nothing else in the sandbox knows the port, so its absence means +// this workload has no gateway rather than that a default would do. +func TestProxyFromEnv(t *testing.T) { + t.Setenv(ProxyEnv, "10.111.0.1:3128") + + got, err := ProxyFromEnv() + require.NoError(t, err) + assert.Equal(t, "10.111.0.1:3128", got) +} + +func TestProxyFromEnvWithoutOne(t *testing.T) { + t.Setenv(ProxyEnv, "") + + _, err := ProxyFromEnv() + require.Error(t, err) + assert.Contains(t, err.Error(), ProxyEnv) +} 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") +} 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/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 ec144fa..8346885 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" ) @@ -104,6 +105,20 @@ 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 + + if err := writeSSHConfig(in); err != nil { + return err + } + } + spec, err := buildSpec(in) if err != nil { return err @@ -372,7 +387,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 +493,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..12ac782 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 @@ -110,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 @@ -354,8 +360,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"}) @@ -412,6 +427,17 @@ func workloadMounts(in input) []sandbox.Mount { ) } + // Keyed on the endpoint and not on in.Gateway, so that the file + // naming the tunnel command and the variable the command reads are + // never one without the other. + if in.GatewayProxy != "" { + mounts = append(mounts, sandbox.Mount{ + Src: filepath.Join(in.ProfileDir, sshConfigFile), + Dst: sshConfigDst, + ReadOnly: true, + }) + } + // The mime handler, the supervisor and the console are all the // qubesome binary, so a workload that is more than one of them still // shares it once. @@ -470,7 +496,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...) @@ -480,8 +506,22 @@ func workloadEnv(in input) []string { "QUBESOME_PROFILE="+profile.Name, ) - if profile.Timezone != "" { - env = append(env, "TZ="+profile.Timezone) + // 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. + 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/sshconfig.go b/internal/runners/bwrap/sshconfig.go new file mode 100644 index 0000000..cae1350 --- /dev/null +++ b/internal/runners/bwrap/sshconfig.go @@ -0,0 +1,68 @@ +package bwrap + +import ( + "fmt" + "os" + "path/filepath" + + "github.com/qubesome/cli/internal/files" +) + +const ( + // sshConfigFile is the drop-in's name in the profile directory, beside + // the other files a launch writes for a workload to read. + sshConfigFile = "ssh_gateway.conf" + + // sshConfigDst is where the drop-in lands in the sandbox. + // + // The system ssh_config an image ships already includes this + // directory, so a file here is read without the image being changed + // and without anything of the image's being covered over. The user's + // own ~/.ssh/config is read before it and so still wins, which is the + // right way round: a workload's dotfiles may have reasons of their + // own for how a host is reached. + // + // The number keeps it early among any siblings. ssh takes the first + // value it is given for a keyword rather than the last. + sshConfigDst = "/etc/ssh/ssh_config.d/10-qubesome-gateway.conf" +) + +// sshGatewayConfig returns the ssh drop-in for a workload on the gateway. +// +// The gateway drops every port but 80, 443 and 53, so ssh cannot connect +// out and has to ask for a tunnel instead. Nothing in an image knows that, +// and the endpoint to ask is not knowable until a launch has allocated an +// address, so the workload is told here rather than in its image or in +// anybody's dotfiles. +// +// The endpoint itself is deliberately absent. The command reads it from +// QUBESOME_GATEWAY_PROXY, which the same launch sets, so this file says +// only how to reach the gateway and never which one. +func sshGatewayConfig() string { + return `# Written by qubesome for a workload attached to the session gateway. +# +# The gateway drops every port but 80, 443 and 53, so ssh reaches a host +# by asking the gateway to carry the connection. The endpoint to ask is in +# QUBESOME_GATEWAY_PROXY, which the command below reads for itself. +Host * + ProxyCommand ` + files.InProfileBinary + ` tunnel %h %p +` +} + +// writeSSHConfig puts the drop-in where the sandbox mounts it from. +// +// It is rewritten on every launch rather than kept, because the file is +// qubesome's own and a stale one left by an older version would be +// mounted unchanged. +func writeSSHConfig(in input) error { + if err := os.MkdirAll(in.ProfileDir, files.DirMode); err != nil { + return fmt.Errorf("failed to ensure profile dir: %w", err) + } + + path := filepath.Join(in.ProfileDir, sshConfigFile) + if err := os.WriteFile(path, []byte(sshGatewayConfig()), files.FileMode); err != nil { + return fmt.Errorf("failed to write %s: %w", sshConfigFile, err) + } + + return nil +} diff --git a/internal/runners/bwrap/sshconfig_test.go b/internal/runners/bwrap/sshconfig_test.go new file mode 100644 index 0000000..9858554 --- /dev/null +++ b/internal/runners/bwrap/sshconfig_test.go @@ -0,0 +1,87 @@ +package bwrap + +import ( + "os" + "path/filepath" + "testing" + + "github.com/qubesome/cli/internal/files" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// The endpoint is not written into the file. It is read from the +// environment by the command the file names, so the two cannot drift into +// disagreeing about which gateway this workload has. +func TestSSHGatewayConfigNamesTheTunnel(t *testing.T) { + t.Parallel() + + got := sshGatewayConfig() + + assert.Contains(t, got, "Host *") + assert.Contains(t, got, "ProxyCommand "+files.InProfileBinary+" tunnel %h %p") + assert.NotContains(t, got, "3128") +} + +// A workload on the gateway reaches port 22 by asking for a tunnel, and +// nothing in an image says so. The drop-in is what tells its ssh, and it +// lands where the system config already includes from. +func TestSpecGivesAGatewayWorkloadAnSSHConfig(t *testing.T) { + t.Parallel() + + in := gatewayInput() + in.GatewayProxy = "10.111.0.1:3128" + + args := render(t, in) + + src := filepath.Join(in.ProfileDir, sshConfigFile) + + i := indexOfArg(args, "--ro-bind", src) + require.NotEqual(t, -1, i, "the ssh drop-in is not mounted") + assert.Equal(t, sshConfigDst, args[i+2]) +} + +// A workload with no gateway has nothing to tunnel through, and a +// ProxyCommand pointing at a proxy that is not there would break the ssh +// that works today. +func TestSpecWithoutAGatewayHasNoSSHConfig(t *testing.T) { + t.Parallel() + + args := render(t, plainInput()) + + assert.NotContains(t, args, sshConfigDst) +} + +// The launch writes the file the sandbox then mounts, so a profile +// directory that does not exist yet is not a reason for the launch to +// fail. +func TestWriteSSHConfig(t *testing.T) { + t.Parallel() + + in := gatewayInput() + in.ProfileDir = filepath.Join(t.TempDir(), "work") + + require.NoError(t, writeSSHConfig(in)) + + got, err := os.ReadFile(filepath.Join(in.ProfileDir, sshConfigFile)) + require.NoError(t, err) + assert.Equal(t, sshGatewayConfig(), string(got)) +} + +// A file left by an older launch is mounted as it is, so what is there +// afterwards has to be what this version writes. +func TestWriteSSHConfigReplacesAnOlderOne(t *testing.T) { + t.Parallel() + + in := gatewayInput() + in.ProfileDir = t.TempDir() + + path := filepath.Join(in.ProfileDir, sshConfigFile) + require.NoError(t, os.WriteFile(path, []byte("ProxyCommand socat -\n"), files.FileMode)) + + require.NoError(t, writeSSHConfig(in)) + + got, err := os.ReadFile(path) + require.NoError(t, err) + assert.Equal(t, sshGatewayConfig(), string(got)) +} 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/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 { diff --git a/internal/types/config.go b/internal/types/config.go index bd5c625..08e4121 100644 --- a/internal/types/config.go +++ b/internal/types/config.go @@ -63,6 +63,18 @@ type Config struct { Gateway *GatewayConfig `yaml:"gateway"` RootDir string + + // Source is the file this config was decoded from, with symlinks + // already resolved. RootDir is its directory, and it is the directory + // that every path in the config is resolved against, so the two are + // not interchangeable: the file is what has to be read again to get + // this same config back. + // + // It is here so that a gateway can record which config started it. A + // session's gateway outlives the profile that launched it, and once + // that profile has stopped nothing else on the host says where its + // image, policy and subnet came from. + Source string } // GatewayConfig describes the gateway that gives workloads their egress. @@ -498,6 +510,7 @@ func DecodeConfig(r io.Reader, path string) (*Config, error) { return nil, fmt.Errorf("failed to decode config %q: %w", path, err) } + cfg.Source = path cfg.RootDir = filepath.Dir(path) // To avoid names being defined twice on the profiles, the name 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 +} 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") }