From 45e47d4f40a1ae759c58a0d5cb1a7fa5a13ef2ae Mon Sep 17 00:00:00 2001 From: CMGS Date: Mon, 14 Sep 2026 13:05:15 +0800 Subject: [PATCH 1/6] fix: deny a tunnel whose authority names a port outside 1-65535 hostPort substituted the default port for one it could not parse, and serveConnect dialed the raw authority, so the verdict and the dial could name different ports. CONNECT example.com:99999 was judged and audited as port 443 (a rule listing 443 allowed it, the intercept path even terminated TLS for it) and only the dial failed; a path-form CONNECT with Host: example.com:+22 was judged as 443 and then dialed to port 22, which Go's dialer accepts, so a port a rule forbids was reachable with an allow verdict. An unparsable or zero port is now a deny with an audit line carrying no port; serveConnect dials the parsed host and port, and the intercept handler builds its authority from the same pair, so the verdict, the dial and the inner requests agree on one target. --- sandboxd/egress/intercept.go | 3 ++- sandboxd/egress/proxy.go | 30 ++++++++++++++-------- sandboxd/egress/proxy_test.go | 48 +++++++++++++++++++++++++++++++++++ 3 files changed, 69 insertions(+), 12 deletions(-) diff --git a/sandboxd/egress/intercept.go b/sandboxd/egress/intercept.go index 69da19ea0..dcc68add9 100644 --- a/sandboxd/egress/intercept.go +++ b/sandboxd/egress/intercept.go @@ -5,6 +5,7 @@ import ( "io" "net" "net/http" + "strconv" "strings" "sync" "sync/atomic" @@ -53,7 +54,7 @@ func (p *Proxy) serveIntercept(w http.ResponseWriter, r *http.Request, host stri _ = client.SetDeadline(time.Time{}) ln := &singleConnListener{conn: tlsConn, done: make(chan struct{})} srv := &http.Server{ - Handler: &interceptHandler{proxy: p, host: host, port: port, authority: r.Host}, + Handler: &interceptHandler{proxy: p, host: host, port: port, authority: net.JoinHostPort(host, strconv.Itoa(int(port)))}, ReadHeaderTimeout: interceptTimeout, } _ = srv.Serve(ln) diff --git a/sandboxd/egress/proxy.go b/sandboxd/egress/proxy.go index 9d73514fd..626861b38 100644 --- a/sandboxd/egress/proxy.go +++ b/sandboxd/egress/proxy.go @@ -136,7 +136,12 @@ func (p *Proxy) untrack(conn net.Conn) { // serveConnect gates an HTTPS/opaque tunnel by host and port. func (p *Proxy) serveConnect(w http.ResponseWriter, r *http.Request) { - host, port := hostPort(r.Host, 443) + host, port, ok := hostPort(r.Host, 443) + if !ok { + p.record(Event{Method: r.Method, Host: host, Decision: DecisionDeny}) + denied(w, host) + return + } decision, intercept := p.tunnelDecision(host, port) if intercept { p.serveIntercept(w, r, host, port) @@ -147,7 +152,7 @@ func (p *Proxy) serveConnect(w http.ResponseWriter, r *http.Request) { denied(w, host) return } - upstream, err := p.dial(r.Context(), "tcp", r.Host) + upstream, err := p.dial(r.Context(), "tcp", net.JoinHostPort(host, strconv.Itoa(int(port)))) if err != nil { http.Error(w, "egress: upstream unreachable", http.StatusBadGateway) return @@ -189,7 +194,12 @@ func (p *Proxy) serveForward(w http.ResponseWriter, r *http.Request) { if r.URL.Scheme == "https" { def = 443 } - host, port := hostPort(r.URL.Host, def) + host, port, ok := hostPort(r.URL.Host, def) + if !ok { + p.record(Event{Method: r.Method, Host: host, Decision: DecisionDeny}) + denied(w, host) + return + } rule, decision := p.policy.Eval(host, r.Method, port) p.relay(w, r, Event{Method: r.Method, Host: host, Port: port, Decision: decision}, rule, p.tr, nil) } @@ -284,14 +294,12 @@ func hostOnly(authority string) string { return authority } -// hostPort splits an authority into host and port; a bare host or an unparsable port takes def. -func hostPort(authority string, def uint16) (string, uint16) { - host, portText, err := net.SplitHostPort(authority) +// hostPort splits an authority; a bare host takes def, a port outside 1-65535 is refused rather than defaulted. +func hostPort(authority string, def uint16) (host string, port uint16, ok bool) { + host, text, err := net.SplitHostPort(authority) if err != nil { - return authority, def - } - if port, err := strconv.ParseUint(portText, 10, 16); err == nil { - return host, uint16(port) + return authority, def, true } - return host, def + n, err := strconv.ParseUint(text, 10, 16) + return host, uint16(n), err == nil && n != 0 } diff --git a/sandboxd/egress/proxy_test.go b/sandboxd/egress/proxy_test.go index 3f720da75..ee876808b 100644 --- a/sandboxd/egress/proxy_test.go +++ b/sandboxd/egress/proxy_test.go @@ -275,6 +275,54 @@ func TestPortRuleGatesConnectAndForward(t *testing.T) { } } +func TestUnparsablePortIsDeniedNotDefaulted(t *testing.T) { + policy := Policy{Allow: []Rule{{Host: "echo.internal", Ports: []uint16{443}}}} + events := make(chan Event, 4) + p := New("sb_1", "acme", policy, nil, nil, fixedDial(echoServer(t)), func(ev Event) { events <- ev }) + front := httptest.NewServer(p) + defer front.Close() + + conn := dialConnect(t, front.Listener.Addr().String(), "echo.internal:99999") + defer func() { _ = conn.Close() }() + if status := readStatus(t, bufio.NewReader(conn)); status != "HTTP/1.1 403 Forbidden" { + t.Errorf("CONNECT with an out-of-range port = %q, want 403 Forbidden", status) + } + if ev := recvEvent(t, events); ev.Port != 0 || ev.Decision != DecisionDeny { + t.Errorf("audit event = %+v, want deny with no port, not the 443 default", ev) + } + + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, "http://echo.internal:99999/x", nil) + if err != nil { + t.Fatalf("build request: %v", err) + } + resp, err := proxyClient(t, front.URL).Do(req) + if err != nil { + t.Fatalf("proxied GET: %v", err) + } + _ = resp.Body.Close() + if resp.StatusCode != http.StatusForbidden { + t.Errorf("forward GET with an out-of-range port = %d, want 403", resp.StatusCode) + } + if ev := recvEvent(t, events); ev.Port != 0 || ev.Decision != DecisionDeny { + t.Errorf("audit event = %+v, want deny with no port", ev) + } + + raw, err := net.Dial("tcp", front.Listener.Addr().String()) + if err != nil { + t.Fatalf("dial proxy: %v", err) + } + defer func() { _ = raw.Close() }() + if _, err := io.WriteString(raw, "CONNECT /x HTTP/1.1\r\nHost: echo.internal:+8443\r\n\r\n"); err != nil { + t.Fatalf("write path-form CONNECT: %v", err) + } + if status := readStatus(t, bufio.NewReader(raw)); status != "HTTP/1.1 403 Forbidden" { + t.Errorf("path-form CONNECT with a signed port = %q, want 403 (the dialer would have accepted +8443)", status) + } + if ev := recvEvent(t, events); ev.Port != 0 || ev.Decision != DecisionDeny { + t.Errorf("audit event = %+v, want deny with no port", ev) + } +} + type fakeSecrets map[string][2]string func (f fakeSecrets) Header(name string) (string, string, bool) { From 532b7d938a86f01bc83d64b8b21d2ed6bdcc82f9 Mon Sep 17 00:00:00 2001 From: CMGS Date: Mon, 14 Sep 2026 13:21:29 +0800 Subject: [PATCH 2/6] review: settle the door's owner, the lane wait and the ownership round The SOCKS5 door is the pool policy's call: the tenant's rules already gate every tunnel through Eval, so the tenant-side AND removed a capability without removing any reach and failed silently when a tenant policy omitted the flag. ServesSocks trusts Validate instead of re-checking the tunnel rule it guarantees. A raw SOCKS5 request for port 0 is refused like the HTTP doors refuse one. hostOnly folds into hostPort. The desktop unit's wait outlives the host's probe and write timeouts (90 s + 120 s) instead of 30 s, exits non-zero when no verdict arrives so a mis-timed build is visible, tests device-backed NICs the way silkd does, and reads the file without tripping set -e; desktopsmoke checks the unit is active and passes its probe URL as a positional argument. sockssmoke's IMAP leg asserts curl's SOCKS5 refusal code, since %{http_code} is 000 for any imap:// URL. silkd latches a relay verdict (the host never unlocks a lane, a late lock can land after the first exec) so the per-exec read happens once on the bridge lane, base_env returns a static slice instead of a Vec per spawn, the watcher moves its owned path into the frame, and an import moves to the file's use group. Deploy docs gain the daemon/image lockstep for the lane file, the strict-config rollback and the opt-in behavior change; the desktop README names the installed apps and the unit's contract; two restating comments and the logger locals in reconcile and refillOne follow the house style. Comment lines: +5 / -8. --- README.md | 2 +- docs/deploy.md | 10 ++++++++ docs/desktop.md | 16 ++++++++----- docs/egress.md | 19 ++++++++------- docs/silkd.md | 5 ++-- e2e/cmd/desktopsmoke/main.go | 8 +++---- e2e/cmd/sockssmoke/main.go | 9 ++++--- os-image/desktop/24.04/Dockerfile | 12 +++++----- os-image/desktop/README.md | 14 ++++++++--- sandboxd/egress/intercept.go | 2 +- sandboxd/egress/policy.go | 10 ++++---- sandboxd/egress/policy_test.go | 39 +++++++------------------------ sandboxd/egress/proxy.go | 9 ------- sandboxd/egress/socks.go | 2 +- sandboxd/egress/socks_test.go | 13 +++++++++++ sandboxd/engine/lane.go | 4 ++-- sandboxd/pool/egress.go | 1 - sandboxd/pool/egress_test.go | 2 +- sandboxd/pool/reconcile.go | 6 ++--- sandboxd/pool/refill.go | 8 +++---- sandboxd/pool/telemetry.go | 1 - silkd/src/exec.rs | 7 +++--- silkd/src/net.rs | 16 +++++++++++-- silkd/src/pty.rs | 4 +++- silkd/src/session.rs | 2 +- silkd/src/sysutil.rs | 12 ++++++++-- silkd/src/watch.rs | 11 ++++++--- 27 files changed, 139 insertions(+), 105 deletions(-) diff --git a/README.md b/README.md index 31cf39d02..3946485e0 100644 --- a/README.md +++ b/README.md @@ -72,7 +72,7 @@ performance) — source in `desktop`, and `android` - `scripts/` — `boot-bench.sh` (boot phase timing), `bench.sh` (the published benchmark procedure), `sandboxd-e2e.sh` (bare-metal e2e, below), plus the - `archive`/`egress`/`intercept` e2e drivers + `archive`/`egress`/`intercept`/`socks` e2e drivers ## Build & test diff --git a/docs/deploy.md b/docs/deploy.md index f3817d050..3cfc1be81 100644 --- a/docs/deploy.md +++ b/docs/deploy.md @@ -45,6 +45,16 @@ volume claim has finalized, do not roll a node back to an older sandboxd until all volume claims are gone; the older daemon cannot preserve the volume claims' admission holds and marker bookkeeping recorded in `claims.json`. +The guest images and sandboxd move together on the egress lane: sandboxd tells +a NIC-bearing guest how its traffic leaves through `/etc/silkd-lane`, and a +guest image from another release reads a different file or none, which on a +locked bridge lane leaves its execs routing into the lock. Roll the images +with the daemon. A config that uses the `socks5` or `ports` policy keys does +not load on an older sandboxd (unknown keys are rejected), so a rollback takes +the config with it. The SOCKS5 door is opt-in: a policy that relied on the +door binding for any bare host rule adds `"socks5": true` or loses the door +silently, since its config still loads. + ## Configuration sandboxd reads one JSON file (`-config`, default diff --git a/docs/desktop.md b/docs/desktop.md index 6dd0ee461..1918020b8 100644 --- a/docs/desktop.md +++ b/docs/desktop.md @@ -24,8 +24,7 @@ up a few seconds later — poll `GET /screenshot` until it returns 200. with an [egress policy](egress.md) when tasks visit the OSWorld mocked websites or the real web; the session's browsers reach the web through the relay. Thunderbird's mail policy is pinned to the SOCKS5 door, so a pool that - runs mail tasks sets `"socks5": true` in its policy, and so does the tenant - policy when the claim carries one. `net=egress` on a + runs mail tasks sets `"socks5": true` in its policy. `net=egress` on a guarded bridge works the same way: the session's proxy setup waits for the host's lane verdict in `/etc/silkd-lane` and follows it, so the locked NIC costs the desktop nothing (a CNI-backed lane is told `direct` and routes). @@ -79,8 +78,13 @@ Configure it with `SANDBOXD_ADDR`, `SANDBOXD_TOKEN`, `COCOON_TEMPLATE` needs a browser the tasks do not pkill and rebind. It takes the caller's profile and debugging port. - `guest-proxy.service` points the session's browsers and `osworld-server`'s - commands at silkd's loopback relay on a guest with no NIC, and on a guest - whose NIC the host reports as `relay` in `/etc/silkd-lane`; it waits up to - 30 s for that verdict before the session starts. A NIC the host calls - `direct`, or a host that writes nothing, leaves the unit idle. + commands at silkd's loopback relay on a guest with no device-backed NIC, and + on a guest whose NIC the host reports as `relay` in `/etc/silkd-lane`; on a + NIC-bearing guest it holds the session until that verdict lands, up to 300 s, + longer than the host's own probe and write timeouts, so a verdict that never + comes means a host that already gave the boot up or an old sandboxd that + writes none: the unit then fails visibly and the session routes directly. A + verdict that changes under a running desktop (a node moved from CNI networks + to a bridge after the claim) reaches silkd's execs at once but the session + only through a re-claim. - x86_64 only. diff --git a/docs/egress.md b/docs/egress.md index 881c633d4..ec7c5a368 100644 --- a/docs/egress.md +++ b/docs/egress.md @@ -48,14 +48,15 @@ through the same code: a rule with `methods` that does not name `CONNECT` denies it, and a rule with a `secret` opens it without injecting anything, on both ports. The one difference is `intercept`: on 3128 such a rule terminates the TLS and filters the requests inside, on 1080 there is no HTTP to filter, so -the tunnel is refused. The door is opt-in: the policy sets `"socks5": true` -and some rule of it can admit a tunnel, otherwise the host side stays unwired -and the guest's dial is refused like the HTTP one with no policy. Both sides of -a pool/tenant pair must opt in, each checked on its own, so disjoint host sets -still bind a listener that denies everything. A policy that opts in with rules -that all carry `methods` without `CONNECT`, or all `intercept`, is rejected at -load. A policy that does not opt in pays nothing for the door on the claim -path. Audit lines carry `"method":"SOCKS5"`. +the tunnel is refused. The door is opt-in: the pool policy sets +`"socks5": true`, otherwise the host side stays unwired and the guest's dial +is refused like the HTTP one with no policy. The tenant policy's rules gate +every tunnel through the door as they gate `CONNECT`, so the tenant does not +opt in separately; its own `socks5` counts only on a claim outside any +configured pool, where the tenant policy is the whole policy. A policy that +opts in with rules that all carry `methods` without `CONNECT`, or all +`intercept`, is rejected at load. A policy that does not opt in pays nothing +for the door on the claim path. Audit lines carry `"method":"SOCKS5"`. ```sh curl --socks5-hostname 127.0.0.1:1080 imaps://imap.example.com/ # allowed: {"socks5": true, "allow": [{"host": "imap.example.com"}]} @@ -163,7 +164,7 @@ file. "egress": { "allow": [{ "host": "api.github.com", "secret": "gh" }] } } ], "tenants": [ - { "name": "acme", "token": "…", "egress": { "socks5": true, "allow": [{ "host": "api.github.com" }] } } + { "name": "acme", "token": "…", "egress": { "allow": [{ "host": "api.github.com" }] } } ] } ``` diff --git a/docs/silkd.md b/docs/silkd.md index 9e3ae81dd..f27d7edc8 100644 --- a/docs/silkd.md +++ b/docs/silkd.md @@ -66,8 +66,9 @@ lane. `SILKD_NET=none|egress` overrides the probe for tests and operators. On every lane silkd also binds `127.0.0.1:3128` and relays each connection to the host's [guarded-egress](egress.md) proxy over vsock (`CID2:2049`), and `127.0.0.1:1080` to its SOCKS5 door (`CID2:2050`) for clients whose protocol -has no HTTP-proxy form; when the host wired no policy the per-connection dial -is refused, so the ports are inert. +has no HTTP-proxy form; when the host wired no policy, or the policy did not +opt into the SOCKS5 door, the per-connection dial is refused, so the ports are +inert. Where nothing routes directly — the no-network lane, or a NIC whose host verdict in `/etc/silkd-lane` reads `relay` because it is nft-locked — silkd forwards the image-baked proxy variables (`http_proxy` and friends) into every diff --git a/e2e/cmd/desktopsmoke/main.go b/e2e/cmd/desktopsmoke/main.go index a021376bc..766e63590 100644 --- a/e2e/cmd/desktopsmoke/main.go +++ b/e2e/cmd/desktopsmoke/main.go @@ -145,19 +145,19 @@ func clickAndReadCursor(ctx context.Context, sb *sandbox.Sandbox) error { // proxyReachesSession proves the session's environment carries the relay and the relay reaches an allowed origin. func proxyReachesSession(ctx context.Context, sb *sandbox.Sandbox, probe string) error { - script := `test -n "$http_proxy" && curl -sS -m 20 -o /dev/null -w '%{http_code}' ` + probe - out, err := execute(ctx, sb, []string{"sh", "-c", script}, 30) + script := `systemctl is-active guest-proxy.service >&2 || exit 1; [ -n "$http_proxy" ] || { echo http_proxy unset in the session >&2; exit 1; }; exec curl -sS -m 20 -o /dev/null -w '%{http_code}' "$1"` + out, err := execute(ctx, sb, []string{"sh", "-c", script, "sh", probe}, 30) if err != nil { return fmt.Errorf("proxy leg: %w", err) } if strings.TrimSpace(out) != "200" { return fmt.Errorf("proxy leg: fetch through the session's proxy returned %q, want 200", strings.TrimSpace(out)) } - fmt.Println(" proxy: the session's environment carries the relay and an allowed origin answers 200") + fmt.Println(" proxy: guest-proxy.service active, the session's environment carries the relay, an allowed origin answers 200") return nil } -// execute runs one command through osworld-server and returns its output; a non-zero exit is an error. +// execute runs one command through osworld-server and returns its output. func execute(ctx context.Context, sb *sandbox.Sandbox, command []string, timeout int) (string, error) { req, _ := json.Marshal(map[string]any{"command": command, "shell": false, "timeout": timeout}) body, err := harness.HTTPOverPort(ctx, sb, serverPort, "POST", "/execute", req) diff --git a/e2e/cmd/sockssmoke/main.go b/e2e/cmd/sockssmoke/main.go index 11a006a67..3a1c5d636 100644 --- a/e2e/cmd/sockssmoke/main.go +++ b/e2e/cmd/sockssmoke/main.go @@ -23,7 +23,8 @@ const ( socksProxy = "127.0.0.1:1080" httpProxy = "http://127.0.0.1:3128" - curlLoginDenied = "exit=67" + curlLoginDenied = "exit=67" + curlProxyRefused = "exit=97" ) func main() { @@ -78,8 +79,10 @@ func run(addr, token, template, echo, getOnly, imap string) error { return fmt.Errorf("IMAPS over SOCKS5 did not reach the login step: %q", strings.TrimSpace(out)) } fmt.Println(" IMAPS handshake rides the tunnel; the server refused the probe login") - if code := curl(ctx, sb, "--socks5-hostname", socksProxy, "imap://"+imap+":143/"); code != "000" { - return fmt.Errorf("IMAP on the unlisted port 143 returned %q, want a refused tunnel", code) + out, _ = sb.Exec(ctx, "sh", "-c", fmt.Sprintf( + "curl -sS -m 30 --socks5-hostname %s imap://%s:143/ 2>&1; echo exit=$?", socksProxy, imap)) + if !strings.Contains(out, curlProxyRefused) { + return fmt.Errorf("IMAP on the unlisted port 143 did not fail at the SOCKS5 handshake: %q", strings.TrimSpace(out)) } fmt.Println(" IMAP on the unlisted port refused on 1080") } diff --git a/os-image/desktop/24.04/Dockerfile b/os-image/desktop/24.04/Dockerfile index db8f39602..1eb9820a0 100644 --- a/os-image/desktop/24.04/Dockerfile +++ b/os-image/desktop/24.04/Dockerfile @@ -151,19 +151,19 @@ RUN apt-get update && apt-get install -y --no-install-recommends \ 'host='"'"'127.0.0.1'"'"'' \ 'port=1080' \ > /usr/local/share/guest-proxy.dconf \ - # a NIC alone says nothing about routing: the host writes /etc/silkd-lane once silkd answers, so a NIC-bearing guest waits for the verdict + # a NIC alone says nothing about routing: the host writes /etc/silkd-lane once silkd answers; the wait outlives the host's + # probe (90 s) and write (120 s) timeouts, so a guest still waiting belongs to a boot the host already gave up on && printf '%s\n' \ '#!/bin/sh' \ 'set -e' \ ': > /run/guest-proxy.env' \ 'nic=0' \ - 'for n in /sys/class/net/*; do case ${n##*/} in lo|sit0) ;; *) nic=1;; esac; done' \ + 'for n in /sys/class/net/*; do if [ -e "$n/device" ]; then nic=1; fi; done' \ 'if [ $nic = 1 ]; then' \ ' i=0' \ - ' while [ ! -e /etc/silkd-lane ] && [ $i -lt 300 ]; do sleep 0.1; i=$((i+1)); done' \ - ' if [ ! -e /etc/silkd-lane ]; then echo "guest-proxy: no lane verdict from the host; routing directly" >&2; exit 0; fi' \ - ' read -r lane < /etc/silkd-lane' \ - ' [ "$lane" = relay ] || exit 0' \ + ' while [ ! -e /etc/silkd-lane ] && [ $i -lt 3000 ]; do sleep 0.1; i=$((i+1)); done' \ + ' if [ ! -e /etc/silkd-lane ]; then echo "guest-proxy: no lane verdict from the host after 300 s; routing directly" >&2; exit 1; fi' \ + ' [ "$(cat /etc/silkd-lane)" = relay ] || exit 0' \ 'fi' \ 'cat /usr/local/share/guest-proxy.env > /run/guest-proxy.env' \ 'cat /usr/local/share/guest-proxy.dconf > /etc/dconf/db/local.d/01-proxy' \ diff --git a/os-image/desktop/README.md b/os-image/desktop/README.md index 4dfe522f5..5d81d9314 100644 --- a/os-image/desktop/README.md +++ b/os-image/desktop/README.md @@ -6,13 +6,15 @@ plus a GNOME session (Ubuntu session, dock, Yaru) on an Xvfb `:1` display at ([xlang-ai/osworld-server](https://github.com/xlang-ai/osworld-server), pinned commit) on guest loopback `5000`, the OSWorld Chrome CDP bridge on loopback `9222`, and the OSWorld app set: Google Chrome, LibreOffice, GIMP, -VLC. The intended pool shape is `size: 2xlarge` (8 CPU / 16G); the session -idles at ~0.5 GB anonymous memory with gnome-shell around 290 MB RSS. +VLC, Thunderbird, VS Code, Zotero, Obsidian, Shotcut, FreeCAD and WPS Office. +The intended pool shape is `size: 2xlarge` (8 CPU / 16G); the session idles +at ~0.5 GB anonymous memory with gnome-shell around 290 MB RSS. An OSWorld harness or the Sai driver claims a sandbox and reaches the guest server through the silkd port relay (`DialPort`/`ProxyPort` 5000, 9222), so the `none` lane works for local tasks; tasks that visit the OSWorld mocked -websites need the `egress` lane. +websites or the web add an egress policy, on the `none` lane or a guarded +bridge `egress` lane alike (see `docs/desktop.md`). ## Guest contract @@ -27,6 +29,12 @@ websites need the `egress` lane. `cdp-bridge` unit forwards loopback `9222` to Chrome's `1337`. - dconf defaults turn on `toolkit-accessibility`, and turn off animations, idle, screen lock, and the welcome dialog. +- `guest-proxy.service` writes the session's proxy environment, the dconf + proxy entry and the apt proxy when the guest has no device-backed NIC, or + when the host's verdict in `/etc/silkd-lane` reads `relay`; on a NIC-bearing + guest it holds the session for that verdict up to 300 s and fails visibly + without it. Thunderbird's policy pins mail to the SOCKS5 door on `1080`, so + a pool that runs mail tasks opts its policy into `socks5`. ## Build diff --git a/sandboxd/egress/intercept.go b/sandboxd/egress/intercept.go index dcc68add9..707368d26 100644 --- a/sandboxd/egress/intercept.go +++ b/sandboxd/egress/intercept.go @@ -94,7 +94,7 @@ func (h *interceptHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { out.URL.Scheme = "https" out.URL.Host = h.authority // keep the guest's Host when it names the CONNECT host: SigV4 signing breaks on a rewrite. - if !strings.EqualFold(hostOnly(r.Host), h.host) { + if inner, _, _ := hostPort(r.Host, 0); !strings.EqualFold(inner, h.host) { out.Host = h.authority } }) diff --git a/sandboxd/egress/policy.go b/sandboxd/egress/policy.go index c6360fcc5..edb73fc18 100644 --- a/sandboxd/egress/policy.go +++ b/sandboxd/egress/policy.go @@ -64,7 +64,7 @@ func (p *Policy) Intercepts() bool { return p != nil && slices.ContainsFunc(p.Allow, func(r Rule) bool { return r.Intercept }) } -// Validate rejects an empty or bare-wildcard host, a zero or repeated port, and socks5 without a tunnel rule; an empty allow-list denies everything. +// Validate rejects a policy the proxy could never honor; an empty allow-list is valid and denies everything. func (p Policy) Validate() error { for i, r := range p.Allow { switch r.Host { @@ -131,9 +131,9 @@ func (p Policy) EvalInner(host, method string, port uint16) (Rule, Decision) { return Rule{}, DecisionDeny } -// ServesSocks reports whether the SOCKS5 door is bound: opted in, with some rule that can admit a tunnel. +// ServesSocks reports whether the policy opted into the SOCKS5 door; Validate holds the rule that makes the door usable. func (p Policy) ServesSocks() bool { - return p.Socks5 && p.admitsTunnel() + return p.Socks5 } func (p Policy) admitsTunnel() bool { @@ -190,7 +190,7 @@ func (c composite) EvalInner(host, method string, port uint16) (Rule, Decision) return rule, DecisionAllow } -// ServesSocks is conservative: two host sets that never meet still bind a door that denies everything. +// ServesSocks is the pool's call: the tenant's rules already gate every tunnel through Eval. func (c composite) ServesSocks() bool { - return c.pool.ServesSocks() && c.tenant.ServesSocks() + return c.pool.ServesSocks() } diff --git a/sandboxd/egress/policy_test.go b/sandboxd/egress/policy_test.go index fe5363e59..9d6835dbd 100644 --- a/sandboxd/egress/policy_test.go +++ b/sandboxd/egress/policy_test.go @@ -98,10 +98,10 @@ func TestCompositeEvalInnerIntersectsTenant(t *testing.T) { func TestWildcardApexIsNotMatched(t *testing.T) { policy := Policy{Allow: []Rule{{Host: "*.example.com"}}} if _, d := policy.Eval("example.com", "GET", 443); d != DecisionDeny { - t.Errorf("apex example.com matched *.example.com, want deny") + t.Error("apex example.com matched *.example.com, want deny") } if _, d := policy.Eval("a.example.com", "GET", 443); d != DecisionAllow { - t.Errorf("a.example.com did not match *.example.com, want allow") + t.Error("a.example.com did not match *.example.com, want allow") } } @@ -131,37 +131,16 @@ func TestPolicyValidate(t *testing.T) { } func TestPolicyServesSocks(t *testing.T) { - tests := []struct { - name string - policy Policy - want bool - }{ - {"opted in, no rules", Policy{Socks5: true}, false}, - {"opted in, bare host", Policy{Socks5: true, Allow: []Rule{{Host: "a.internal"}}}, true}, - {"bare host without opting in", Policy{Allow: []Rule{{Host: "a.internal"}}}, false}, - {"opted in, methods without CONNECT", Policy{Socks5: true, Allow: []Rule{{Host: "a.internal", Methods: []string{"GET"}}}}, false}, - {"opted in, methods with CONNECT", Policy{Socks5: true, Allow: []Rule{{Host: "a.internal", Methods: []string{"connect"}}}}, true}, - {"opted in, intercept only", Policy{Socks5: true, Allow: []Rule{{Host: "a.internal", Intercept: true}}}, false}, - {"opted in, secret", Policy{Socks5: true, Allow: []Rule{{Host: "a.internal", Secret: "s"}}}, true}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := tt.policy.ServesSocks(); got != tt.want { - t.Errorf("ServesSocks() = %v, want %v", got, tt.want) - } - }) - } - bare := Policy{Socks5: true, Allow: []Rule{{Host: "a.internal"}}} - getOnly := Policy{Socks5: true, Allow: []Rule{{Host: "a.internal", Methods: []string{"GET"}}}} + opted := Policy{Socks5: true, Allow: []Rule{{Host: "a.internal"}}} silent := Policy{Allow: []Rule{{Host: "a.internal"}}} - if Compose(bare, getOnly).ServesSocks() { - t.Error("composite ServesSocks() = true with a GET-only tenant, want false") + if !opted.ServesSocks() || silent.ServesSocks() { + t.Errorf("ServesSocks() opted=%v silent=%v, want true/false", opted.ServesSocks(), silent.ServesSocks()) } - if Compose(bare, silent).ServesSocks() { - t.Error("composite ServesSocks() = true with a tenant that did not opt in, want false") + if !Compose(opted, silent).ServesSocks() { + t.Error("composite ServesSocks() = false with an opted-in pool and a silent tenant; the pool owns the door") } - if !Compose(bare, bare).ServesSocks() { - t.Error("composite ServesSocks() = false with two opted-in bare policies, want true") + if Compose(silent, opted).ServesSocks() { + t.Error("composite ServesSocks() = true with a silent pool; a tenant cannot open the door alone") } } diff --git a/sandboxd/egress/proxy.go b/sandboxd/egress/proxy.go index 626861b38..ecf870a9a 100644 --- a/sandboxd/egress/proxy.go +++ b/sandboxd/egress/proxy.go @@ -134,7 +134,6 @@ func (p *Proxy) untrack(conn net.Conn) { p.connMu.Unlock() } -// serveConnect gates an HTTPS/opaque tunnel by host and port. func (p *Proxy) serveConnect(w http.ResponseWriter, r *http.Request) { host, port, ok := hostPort(r.Host, 443) if !ok { @@ -286,14 +285,6 @@ func denied(w http.ResponseWriter, host string) { http.Error(w, fmt.Sprintf("egress denied: %s", host), http.StatusForbidden) } -// hostOnly strips a port from an authority, tolerating a bare host and IPv6 literals. -func hostOnly(authority string) string { - if host, _, err := net.SplitHostPort(authority); err == nil { - return host - } - return authority -} - // hostPort splits an authority; a bare host takes def, a port outside 1-65535 is refused rather than defaulted. func hostPort(authority string, def uint16) (host string, port uint16, ok bool) { host, text, err := net.SplitHostPort(authority) diff --git a/sandboxd/egress/socks.go b/sandboxd/egress/socks.go index af1131ba1..07368ed2f 100644 --- a/sandboxd/egress/socks.go +++ b/sandboxd/egress/socks.go @@ -59,7 +59,7 @@ func (p *Proxy) serveSocks(ctx context.Context, conn net.Conn) { return } decision, intercept := p.tunnelDecision(host, port) - if intercept { + if intercept || port == 0 { decision = DecisionDeny } p.record(Event{Method: methodSOCKS, Host: host, Port: port, Decision: decision}) diff --git a/sandboxd/egress/socks_test.go b/sandboxd/egress/socks_test.go index 930caffe4..b99a75ef1 100644 --- a/sandboxd/egress/socks_test.go +++ b/sandboxd/egress/socks_test.go @@ -165,6 +165,19 @@ func TestSocksPortRuleGatesTheTunnel(t *testing.T) { } } +func TestSocksPortZeroIsDeniedEvenByABareRule(t *testing.T) { + events := make(chan Event, 4) + _, addr := socksProxy(t, Policy{Allow: []Rule{{Host: "mail.internal"}}}, nil, fixedDial(echoServer(t)), events) + conn, reply := socksExchange(t, addr, socksGreeting, socksNameRequest("mail.internal", 0)) + defer func() { _ = conn.Close() }() + if reply[1] != socksDenied { + t.Fatalf("port 0 reply = %#x, want denied", reply[1]) + } + if ev := recvEvent(t, events); ev.Decision != DecisionDeny { + t.Errorf("audit event = %+v, want deny", ev) + } +} + func socksProxy(t *testing.T, policy Policy, ca *CA, dial DialFunc, events chan Event) (*Proxy, string) { t.Helper() var audit func(Event) diff --git a/sandboxd/engine/lane.go b/sandboxd/engine/lane.go index d395c3220..90ff17296 100644 --- a/sandboxd/engine/lane.go +++ b/sandboxd/engine/lane.go @@ -3,10 +3,10 @@ package engine import "context" const ( - laneFile = "/etc/silkd-lane" - LaneRelay Lane = "relay" LaneDirect Lane = "direct" + + laneFile = "/etc/silkd-lane" ) // Lane is the host's verdict on how a guest's traffic leaves: through the relay when its NIC is nft-locked, directly when the NIC routes. diff --git a/sandboxd/pool/egress.go b/sandboxd/pool/egress.go index 482ee59a5..70e2eb43a 100644 --- a/sandboxd/pool/egress.go +++ b/sandboxd/pool/egress.go @@ -94,7 +94,6 @@ func (m *Manager) lockEgressNIC(ctx context.Context, sb *types.Sandbox) error { return nil } -// markLane tells the guest how its traffic leaves; the nft lock is invisible from inside. func (m *Manager) markLane(ctx context.Context, key types.PoolKey, sock string) error { lane := m.laneOf(key) if lane == "" { diff --git a/sandboxd/pool/egress_test.go b/sandboxd/pool/egress_test.go index c2f76ad53..f49b701c4 100644 --- a/sandboxd/pool/egress_test.go +++ b/sandboxd/pool/egress_test.go @@ -79,7 +79,6 @@ func TestArmEgressBindsSocksOnlyWhenOptedIn(t *testing.T) { }{ {"opted in with a bare host rule", &egress.Policy{Socks5: true, Allow: []egress.Rule{{Host: "example.com"}}}, true}, {"bare host rule without opting in", &egress.Policy{Allow: []egress.Rule{{Host: "example.com"}}}, false}, - {"opted in with a GET-only rule", &egress.Policy{Socks5: true, Allow: []egress.Rule{{Host: "example.com", Methods: []string{"GET"}}}}, false}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -116,6 +115,7 @@ func TestLaneVerdictReachesGuestOnColdBoots(t *testing.T) { {"golden build on an unlocked egress lane", egKey, false, false, []string{"direct"}}, {"golden build on the none lane", testKey, true, false, nil}, {"cold provision on a locked egress lane", egKey, true, true, []string{"relay"}}, + {"cold provision on an unlocked egress lane", egKey, false, true, []string{"direct"}}, {"cold provision on the none lane", testKey, true, true, nil}, } for _, tt := range tests { diff --git a/sandboxd/pool/reconcile.go b/sandboxd/pool/reconcile.go index e338d6918..fd44cf4f7 100644 --- a/sandboxd/pool/reconcile.go +++ b/sandboxd/pool/reconcile.go @@ -71,17 +71,17 @@ func (m *Manager) Reconcile(ctx context.Context) error { m.mu.Unlock() // a crash mid-export leaves a *.tmp staging dir nothing in this life reuses tmps, _ := filepath.Glob(filepath.Join(m.goldensDir(), "*.tmp")) + logger := log.WithFunc("pool.Reconcile") if err := m.ckpts.SweepStaging(); err != nil { - log.WithFunc("pool.Reconcile").Error(ctx, err, "sweep checkpoint staging") + logger.Error(ctx, err, "sweep checkpoint staging") } if err := m.tpls.SweepStaging(); err != nil { - log.WithFunc("pool.Reconcile").Error(ctx, err, "sweep template staging") + logger.Error(ctx, err, "sweep template staging") } for _, tmp := range tmps { _ = os.RemoveAll(tmp) } - logger := log.WithFunc("pool.Reconcile") m.reclaimOrphanArchiveCks(ctx, claims) m.retryArchiveDeletes(ctx) removed := m.sweepStaleVMs(ctx, live, owned) diff --git a/sandboxd/pool/refill.go b/sandboxd/pool/refill.go index 523a5103c..e61b32d83 100644 --- a/sandboxd/pool/refill.go +++ b/sandboxd/pool/refill.go @@ -117,17 +117,17 @@ func (m *Manager) refillOne(ctx context.Context, p *pool, golden string) { parked := now.Before(m.atCapacityUntil) m.mu.Unlock() if err != nil { + logger := log.WithFunc("pool.refillOne") switch { case ctx.Err() != nil: case entered: - log.WithFunc("pool.refillOne").Errorf(ctx, err, - "node is at capacity; parking refill for %s", capacityBackoff) + logger.Errorf(ctx, err, "node is at capacity; parking refill for %s", capacityBackoff) case parked: default: hash := p.hash - log.WithFunc("pool.refillOne").Errorf(ctx, err, "refill %s", hash) + logger.Errorf(ctx, err, "refill %s", hash) if backedOff { - log.WithFunc("pool.refillOne").Warnf(ctx, + logger.Warnf(ctx, "refill %s failed %d times running; pausing refill for %s", hash, fails, wait.Round(time.Millisecond)) } diff --git a/sandboxd/pool/telemetry.go b/sandboxd/pool/telemetry.go index e8f8006ed..4ddccd330 100644 --- a/sandboxd/pool/telemetry.go +++ b/sandboxd/pool/telemetry.go @@ -61,7 +61,6 @@ type auditFrame struct { Secret string `json:"secret,omitempty"` //nolint:gosec // the secret's ref name, never its value } -// Counters snapshots the monotonic telemetry counters. func (m *Manager) Counters() Counters { c := &m.counters return Counters{ diff --git a/silkd/src/exec.rs b/silkd/src/exec.rs index 0c06f7c00..5a70a2c4c 100644 --- a/silkd/src/exec.rs +++ b/silkd/src/exec.rs @@ -4,7 +4,7 @@ use std::process::Stdio; use std::sync::{Arc, OnceLock}; use std::time::Duration; -use tokio::io::{AsyncWrite, AsyncWriteExt}; +use tokio::io::{AsyncRead, AsyncReadExt, AsyncWrite, AsyncWriteExt}; use tokio::process::Command; use tokio::sync::mpsc; use tokio::time::timeout; @@ -41,7 +41,7 @@ where cmd.current_dir(cwd); } cmd.env_clear(); - cmd.envs(sysutil::base_env()); + cmd.envs(sysutil::base_env().iter().copied()); cmd.envs(&req.env); if let Some(ref user) = req.user && let Err(e) = sysutil::apply_user(&mut cmd, user) @@ -167,9 +167,8 @@ async fn pump_out( async fn copy_stream(mut r: R, proc: &Proc, stderr: bool, fg: Option<&mpsc::Sender>) where - R: tokio::io::AsyncRead + Unpin, + R: AsyncRead + Unpin, { - use tokio::io::AsyncReadExt; let make: fn(Vec) -> Chunk = if stderr { Chunk::Stderr } else { Chunk::Stdout }; let mut buf = [0u8; crate::proto::READ_CHUNK]; loop { diff --git a/silkd/src/net.rs b/silkd/src/net.rs index a813e47e3..58ce1edfb 100644 --- a/silkd/src/net.rs +++ b/silkd/src/net.rs @@ -3,12 +3,13 @@ use std::fs::File; use std::io::Read; use std::sync::LazyLock; -use std::sync::atomic::{AtomicI8, Ordering}; +use std::sync::atomic::{AtomicBool, AtomicI8, Ordering}; /// File the host writes with its lane verdict, `relay` or `direct`; on the root filesystem so a guest reboot keeps it. const LANE_FILE: &str = "/etc/silkd-lane"; static LANE_OVERRIDE: AtomicI8 = AtomicI8::new(-1); +static LANE_RELAY: AtomicBool = AtomicBool::new(false); /// Reports whether the guest can reach a network; only a `device`-backed interface counts, since the kernel auto-creates virtual tunnels. pub fn has_egress() -> bool { @@ -34,7 +35,18 @@ pub fn has_egress() -> bool { /// Reports whether execs route directly: a NIC the host did not mark as relayed. pub fn routes_directly() -> bool { - has_egress() && !lane_is_relay(LANE_FILE) + if !has_egress() { + return false; + } + // relay latches: the host never unlocks a lane, but a late lock can land after the first exec. + if LANE_RELAY.load(Ordering::Relaxed) { + return false; + } + let relay = lane_is_relay(LANE_FILE); + if relay { + LANE_RELAY.store(true, Ordering::Relaxed); + } + !relay } /// Lane override for tests: set_var would race every concurrent getenv. diff --git a/silkd/src/pty.rs b/silkd/src/pty.rs index f2c406b68..cbab7f0c5 100644 --- a/silkd/src/pty.rs +++ b/silkd/src/pty.rs @@ -43,7 +43,9 @@ pub async fn open( if let Some(dir) = &req.cwd { cmd.current_dir(dir); } - cmd.env_clear().envs(sysutil::base_env()).envs(&req.env); + cmd.env_clear() + .envs(sysutil::base_env().iter().copied()) + .envs(&req.env); if let Some(user) = &req.user && let Err(e) = sysutil::apply_user(&mut cmd, user) { diff --git a/silkd/src/session.rs b/silkd/src/session.rs index b2ac70e53..4c2961986 100644 --- a/silkd/src/session.rs +++ b/silkd/src/session.rs @@ -50,7 +50,7 @@ impl Table { } let mut child = Command::new("bash") .env_clear() - .envs(sysutil::base_env()) + .envs(sysutil::base_env().iter().copied()) .stdin(Stdio::piped()) .stdout(Stdio::piped()) // null, not piped: the init line `exec 2>&1` repoints fd 2 at the stdout pipe. diff --git a/silkd/src/sysutil.rs b/silkd/src/sysutil.rs index 64b3b053a..b0d62a6c8 100644 --- a/silkd/src/sysutil.rs +++ b/silkd/src/sysutil.rs @@ -63,8 +63,16 @@ pub fn signal_pid(pid: u32, sig: i32) { } /// The environment every exec starts from; the proxy snapshot rides only where nothing routes directly. -pub fn base_env() -> Vec<(&'static str, &'static str)> { - compose_env(crate::net::routes_directly(), proxy_vars()) +pub fn base_env() -> &'static [(&'static str, &'static str)] { + static DIRECT: LazyLock> = + LazyLock::new(|| compose_env(true, proxy_vars())); + static RELAYED: LazyLock> = + LazyLock::new(|| compose_env(false, proxy_vars())); + if crate::net::routes_directly() { + &DIRECT + } else { + &RELAYED + } } /// Applies base_env's lane rule to an env-inheriting child: a routed NIC drops the proxy variables. diff --git a/silkd/src/watch.rs b/silkd/src/watch.rs index 52429804b..a7ae2151e 100644 --- a/silkd/src/watch.rs +++ b/silkd/src/watch.rs @@ -95,9 +95,14 @@ fn to_frames(res: notify::Result, out: &mut Vec) { N::Remove(_) => EventKind::Deleted, _ => return, }; - out.extend(event.paths.into_iter().map(|p| Response::Event { - kind, - path: p.to_string_lossy().into_owned(), + out.extend(event.paths.into_iter().map(|p| { + Response::Event { + kind, + path: p + .into_os_string() + .into_string() + .unwrap_or_else(|os| os.to_string_lossy().into_owned()), + } })); } From e32bfaaac13428eb7592e09a2d505a3d0ab0a701 Mon Sep 17 00:00:00 2001 From: CMGS Date: Mon, 14 Sep 2026 13:40:08 +0800 Subject: [PATCH 3/6] review: refuse a malformed authority and name the door's owner in the docs hostPort treated every split error as a bare host, so a path-form CONNECT with Host: example.com:443:8443 was judged as 443 and audited as allowed before the dial failed; only a host with no colon, or a bracketed IPv6 literal, takes the default now. Three doc sentences still said "the policy" opts into the SOCKS5 door where it is the pool policy, or the tenant's only on a claim outside any configured pool. --- docs/deploy.md | 8 +++++--- docs/egress.md | 5 +++-- docs/silkd.md | 5 +++-- sandboxd/egress/proxy.go | 6 ++++-- sandboxd/egress/proxy_test.go | 17 ++++++++++++++++- 5 files changed, 31 insertions(+), 10 deletions(-) diff --git a/docs/deploy.md b/docs/deploy.md index 3cfc1be81..2ef508b8e 100644 --- a/docs/deploy.md +++ b/docs/deploy.md @@ -51,9 +51,11 @@ guest image from another release reads a different file or none, which on a locked bridge lane leaves its execs routing into the lock. Roll the images with the daemon. A config that uses the `socks5` or `ports` policy keys does not load on an older sandboxd (unknown keys are rejected), so a rollback takes -the config with it. The SOCKS5 door is opt-in: a policy that relied on the -door binding for any bare host rule adds `"socks5": true` or loses the door -silently, since its config still loads. +the config with it. The SOCKS5 door is opt-in and the pool policy owns it: a +pool that relied on the door binding for any bare host rule adds +`"socks5": true` to its policy or loses the door silently, since its config +still loads; a tenant policy's flag counts only on a claim outside any +configured pool, where it is the whole policy. ## Configuration diff --git a/docs/egress.md b/docs/egress.md index ec7c5a368..5c7cd81c9 100644 --- a/docs/egress.md +++ b/docs/egress.md @@ -55,8 +55,9 @@ every tunnel through the door as they gate `CONNECT`, so the tenant does not opt in separately; its own `socks5` counts only on a claim outside any configured pool, where the tenant policy is the whole policy. A policy that opts in with rules that all carry `methods` without `CONNECT`, or all -`intercept`, is rejected at load. A policy that does not opt in pays nothing -for the door on the claim path. Audit lines carry `"method":"SOCKS5"`. +`intercept`, is rejected at load. A pool whose policy does not opt in pays +nothing for the door on the claim path, whatever its tenants' policies say. +Audit lines carry `"method":"SOCKS5"`. ```sh curl --socks5-hostname 127.0.0.1:1080 imaps://imap.example.com/ # allowed: {"socks5": true, "allow": [{"host": "imap.example.com"}]} diff --git a/docs/silkd.md b/docs/silkd.md index f27d7edc8..4ea54b924 100644 --- a/docs/silkd.md +++ b/docs/silkd.md @@ -66,8 +66,9 @@ lane. `SILKD_NET=none|egress` overrides the probe for tests and operators. On every lane silkd also binds `127.0.0.1:3128` and relays each connection to the host's [guarded-egress](egress.md) proxy over vsock (`CID2:2049`), and `127.0.0.1:1080` to its SOCKS5 door (`CID2:2050`) for clients whose protocol -has no HTTP-proxy form; when the host wired no policy, or the policy did not -opt into the SOCKS5 door, the per-connection dial is refused, so the ports are +has no HTTP-proxy form; when the host wired no policy, or the door-owning +policy (the pool's, or the tenant's on a claim outside any pool) did not opt +into the SOCKS5 door, the per-connection dial is refused, so the ports are inert. Where nothing routes directly — the no-network lane, or a NIC whose host verdict in `/etc/silkd-lane` reads `relay` because it is nft-locked — silkd diff --git a/sandboxd/egress/proxy.go b/sandboxd/egress/proxy.go index ecf870a9a..9f91058c6 100644 --- a/sandboxd/egress/proxy.go +++ b/sandboxd/egress/proxy.go @@ -285,11 +285,13 @@ func denied(w http.ResponseWriter, host string) { http.Error(w, fmt.Sprintf("egress denied: %s", host), http.StatusForbidden) } -// hostPort splits an authority; a bare host takes def, a port outside 1-65535 is refused rather than defaulted. +// hostPort splits an authority; a bare host takes def, a port outside 1-65535 or a malformed authority is refused rather than defaulted. func hostPort(authority string, def uint16) (host string, port uint16, ok bool) { host, text, err := net.SplitHostPort(authority) if err != nil { - return authority, def, true + bare := !strings.Contains(authority, ":") || + (strings.HasPrefix(authority, "[") && strings.HasSuffix(authority, "]")) + return authority, def, bare } n, err := strconv.ParseUint(text, 10, 16) return host, uint16(n), err == nil && n != 0 diff --git a/sandboxd/egress/proxy_test.go b/sandboxd/egress/proxy_test.go index ee876808b..0f549c302 100644 --- a/sandboxd/egress/proxy_test.go +++ b/sandboxd/egress/proxy_test.go @@ -312,7 +312,7 @@ func TestUnparsablePortIsDeniedNotDefaulted(t *testing.T) { t.Fatalf("dial proxy: %v", err) } defer func() { _ = raw.Close() }() - if _, err := io.WriteString(raw, "CONNECT /x HTTP/1.1\r\nHost: echo.internal:+8443\r\n\r\n"); err != nil { + if _, err = io.WriteString(raw, "CONNECT /x HTTP/1.1\r\nHost: echo.internal:+8443\r\n\r\n"); err != nil { t.Fatalf("write path-form CONNECT: %v", err) } if status := readStatus(t, bufio.NewReader(raw)); status != "HTTP/1.1 403 Forbidden" { @@ -321,6 +321,21 @@ func TestUnparsablePortIsDeniedNotDefaulted(t *testing.T) { if ev := recvEvent(t, events); ev.Port != 0 || ev.Decision != DecisionDeny { t.Errorf("audit event = %+v, want deny with no port", ev) } + + malformed, err := net.Dial("tcp", front.Listener.Addr().String()) + if err != nil { + t.Fatalf("dial proxy: %v", err) + } + defer func() { _ = malformed.Close() }() + if _, err = io.WriteString(malformed, "CONNECT /x HTTP/1.1\r\nHost: echo.internal:443:8443\r\n\r\n"); err != nil { + t.Fatalf("write malformed CONNECT: %v", err) + } + if status := readStatus(t, bufio.NewReader(malformed)); status != "HTTP/1.1 403 Forbidden" { + t.Errorf("path-form CONNECT with two ports = %q, want 403, not a bare-host default", status) + } + if ev := recvEvent(t, events); ev.Port != 0 || ev.Decision != DecisionDeny { + t.Errorf("audit event = %+v, want deny with no port", ev) + } } type fakeSecrets map[string][2]string From 7f5293e72aa913ec996fdb62b2673aec091931bd Mon Sep 17 00:00:00 2001 From: CMGS Date: Mon, 14 Sep 2026 13:47:17 +0800 Subject: [PATCH 4/6] review: unwrap a bracketed IPv6 authority instead of dialing it twice-bracketed A path-form CONNECT whose Host was a bracketed IPv6 literal without a port kept its brackets as the policy host, so the verdict was recorded on "[addr]" and the dial went to "[[addr]]:443" and failed after an allowed audit line; any bracketed text passed the same way. The literal is now unwrapped and parsed with netip, a valid IPv6 address becomes the bare host the rules and the dial both see, and anything else is refused. --- sandboxd/egress/proxy.go | 11 ++++++++--- sandboxd/egress/proxy_test.go | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/sandboxd/egress/proxy.go b/sandboxd/egress/proxy.go index 9f91058c6..aab35460c 100644 --- a/sandboxd/egress/proxy.go +++ b/sandboxd/egress/proxy.go @@ -8,6 +8,7 @@ import ( "maps" "net" "net/http" + "net/netip" "net/textproto" "slices" "strconv" @@ -289,9 +290,13 @@ func denied(w http.ResponseWriter, host string) { func hostPort(authority string, def uint16) (host string, port uint16, ok bool) { host, text, err := net.SplitHostPort(authority) if err != nil { - bare := !strings.Contains(authority, ":") || - (strings.HasPrefix(authority, "[") && strings.HasSuffix(authority, "]")) - return authority, def, bare + if !strings.Contains(authority, ":") { + return authority, def, true + } + literal, bracketed := strings.CutPrefix(authority, "[") + literal, closed := strings.CutSuffix(literal, "]") + addr, parseErr := netip.ParseAddr(literal) + return literal, def, bracketed && closed && parseErr == nil && addr.Is6() } n, err := strconv.ParseUint(text, 10, 16) return host, uint16(n), err == nil && n != 0 diff --git a/sandboxd/egress/proxy_test.go b/sandboxd/egress/proxy_test.go index 0f549c302..7a2c7c48b 100644 --- a/sandboxd/egress/proxy_test.go +++ b/sandboxd/egress/proxy_test.go @@ -338,6 +338,39 @@ func TestUnparsablePortIsDeniedNotDefaulted(t *testing.T) { } } +func TestBracketedIPv6AuthorityIsUnwrapped(t *testing.T) { + policy := Policy{Allow: []Rule{{Host: "2606:4700:4700::1111"}}} + events := make(chan Event, 4) + p := New("sb_1", "acme", policy, nil, nil, fixedDial(echoServer(t)), func(ev Event) { events <- ev }) + front := httptest.NewServer(p) + defer front.Close() + + for _, tc := range []struct { + authority string + status string + decision Decision + }{ + {"[2606:4700:4700::1111]", "HTTP/1.1 200 Connection Established", DecisionAllow}, + {"[2606:4700:4700::1111]:443", "HTTP/1.1 200 Connection Established", DecisionAllow}, + {"[not-an-address]", "HTTP/1.1 403 Forbidden", DecisionDeny}, + } { + conn, err := net.Dial("tcp", front.Listener.Addr().String()) + if err != nil { + t.Fatalf("dial proxy: %v", err) + } + if _, err = io.WriteString(conn, "CONNECT /x HTTP/1.1\r\nHost: "+tc.authority+"\r\n\r\n"); err != nil { + t.Fatalf("write CONNECT: %v", err) + } + if status := readStatus(t, bufio.NewReader(conn)); status != tc.status { + t.Errorf("CONNECT with Host %s = %q, want %q", tc.authority, status, tc.status) + } + _ = conn.Close() + if ev := recvEvent(t, events); ev.Decision != tc.decision || (tc.decision == DecisionAllow && (ev.Host != "2606:4700:4700::1111" || ev.Port != 443)) { + t.Errorf("audit event for %s = %+v, want %v on the bare address and port 443", tc.authority, ev, tc.decision) + } + } +} + type fakeSecrets map[string][2]string func (f fakeSecrets) Header(name string) (string, string, bool) { From 560e5650c79636d00a4165ec284d9ec266db3c2b Mon Sep 17 00:00:00 2001 From: CMGS Date: Mon, 14 Sep 2026 13:54:22 +0800 Subject: [PATCH 5/6] review: check bracket syntax before the bare-host default A bracketed authority with no colon inside, such as "[not-an-address]", reached the bare-host branch first and was allowed under a wildcard rule before the dial failed. An authority that opens a bracket must now close it around a valid IPv6 address, or it is refused; the test runs under a wildcard policy and requires a no-port deny for the malformed forms. --- sandboxd/egress/proxy.go | 8 ++++---- sandboxd/egress/proxy_test.go | 10 +++++++--- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/sandboxd/egress/proxy.go b/sandboxd/egress/proxy.go index aab35460c..dfe1f4c39 100644 --- a/sandboxd/egress/proxy.go +++ b/sandboxd/egress/proxy.go @@ -290,13 +290,13 @@ func denied(w http.ResponseWriter, host string) { func hostPort(authority string, def uint16) (host string, port uint16, ok bool) { host, text, err := net.SplitHostPort(authority) if err != nil { - if !strings.Contains(authority, ":") { - return authority, def, true - } literal, bracketed := strings.CutPrefix(authority, "[") + if !bracketed { + return authority, def, !strings.Contains(authority, ":") + } literal, closed := strings.CutSuffix(literal, "]") addr, parseErr := netip.ParseAddr(literal) - return literal, def, bracketed && closed && parseErr == nil && addr.Is6() + return literal, def, closed && parseErr == nil && addr.Is6() } n, err := strconv.ParseUint(text, 10, 16) return host, uint16(n), err == nil && n != 0 diff --git a/sandboxd/egress/proxy_test.go b/sandboxd/egress/proxy_test.go index 7a2c7c48b..731ce21ce 100644 --- a/sandboxd/egress/proxy_test.go +++ b/sandboxd/egress/proxy_test.go @@ -339,7 +339,7 @@ func TestUnparsablePortIsDeniedNotDefaulted(t *testing.T) { } func TestBracketedIPv6AuthorityIsUnwrapped(t *testing.T) { - policy := Policy{Allow: []Rule{{Host: "2606:4700:4700::1111"}}} + policy := Policy{Allow: []Rule{{Host: "*"}}} events := make(chan Event, 4) p := New("sb_1", "acme", policy, nil, nil, fixedDial(echoServer(t)), func(ev Event) { events <- ev }) front := httptest.NewServer(p) @@ -353,6 +353,7 @@ func TestBracketedIPv6AuthorityIsUnwrapped(t *testing.T) { {"[2606:4700:4700::1111]", "HTTP/1.1 200 Connection Established", DecisionAllow}, {"[2606:4700:4700::1111]:443", "HTTP/1.1 200 Connection Established", DecisionAllow}, {"[not-an-address]", "HTTP/1.1 403 Forbidden", DecisionDeny}, + {"[2606:4700:4700::1111", "HTTP/1.1 403 Forbidden", DecisionDeny}, } { conn, err := net.Dial("tcp", front.Listener.Addr().String()) if err != nil { @@ -365,8 +366,11 @@ func TestBracketedIPv6AuthorityIsUnwrapped(t *testing.T) { t.Errorf("CONNECT with Host %s = %q, want %q", tc.authority, status, tc.status) } _ = conn.Close() - if ev := recvEvent(t, events); ev.Decision != tc.decision || (tc.decision == DecisionAllow && (ev.Host != "2606:4700:4700::1111" || ev.Port != 443)) { - t.Errorf("audit event for %s = %+v, want %v on the bare address and port 443", tc.authority, ev, tc.decision) + ev := recvEvent(t, events) + allowed := ev.Decision == DecisionAllow && ev.Host == "2606:4700:4700::1111" && ev.Port == 443 + denied := ev.Decision == DecisionDeny && ev.Port == 0 + if (tc.decision == DecisionAllow) != allowed || (tc.decision == DecisionDeny) != denied { + t.Errorf("audit event for %s = %+v, want %v (allow on the bare address and 443, or deny with no port)", tc.authority, ev, tc.decision) } } } From feec461ebb11b3e25f11fd8284208b79daccdde6 Mon Sep 17 00:00:00 2001 From: CMGS Date: Mon, 14 Sep 2026 14:04:11 +0800 Subject: [PATCH 6/6] review: default only a well-formed bare host A stray bracket such as "echo.internal]" or an empty Host still reached the bare-host default and was allowed under a wildcard rule before the dial failed. The default now applies only to a nonempty authority that contains no colon and no bracket; the wildcard-policy test gains rows for the stray bracket and the empty host, each a deny with no port. --- sandboxd/egress/proxy.go | 2 +- sandboxd/egress/proxy_test.go | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/sandboxd/egress/proxy.go b/sandboxd/egress/proxy.go index dfe1f4c39..e3856b975 100644 --- a/sandboxd/egress/proxy.go +++ b/sandboxd/egress/proxy.go @@ -292,7 +292,7 @@ func hostPort(authority string, def uint16) (host string, port uint16, ok bool) if err != nil { literal, bracketed := strings.CutPrefix(authority, "[") if !bracketed { - return authority, def, !strings.Contains(authority, ":") + return authority, def, authority != "" && !strings.ContainsAny(authority, ":[]") } literal, closed := strings.CutSuffix(literal, "]") addr, parseErr := netip.ParseAddr(literal) diff --git a/sandboxd/egress/proxy_test.go b/sandboxd/egress/proxy_test.go index 731ce21ce..183ce1a3b 100644 --- a/sandboxd/egress/proxy_test.go +++ b/sandboxd/egress/proxy_test.go @@ -354,6 +354,8 @@ func TestBracketedIPv6AuthorityIsUnwrapped(t *testing.T) { {"[2606:4700:4700::1111]:443", "HTTP/1.1 200 Connection Established", DecisionAllow}, {"[not-an-address]", "HTTP/1.1 403 Forbidden", DecisionDeny}, {"[2606:4700:4700::1111", "HTTP/1.1 403 Forbidden", DecisionDeny}, + {"echo.internal]", "HTTP/1.1 403 Forbidden", DecisionDeny}, + {"", "HTTP/1.1 403 Forbidden", DecisionDeny}, } { conn, err := net.Dial("tcp", front.Listener.Addr().String()) if err != nil {