diff --git a/README.md b/README.md index df13040a..31cf39d0 100644 --- a/README.md +++ b/README.md @@ -56,8 +56,8 @@ performance) — source in - `e2e/` — in-process full-stack tests (real pool/engine/relay/SDK, fake cocoon+guest) plus bare-metal acceptance drivers under `cmd/`: `demo`, `smoke`, `meshsmoke`, `crossnode`, `coldproof`, `egresssmoke`, - `interceptsmoke`, `volumesmoke`, `lifecycle` (idle→hibernate→archive), - `androidsmoke`, `browsersmoke`, `desktopsmoke`, and the `pullbench`/`pushbench`/`rpcbench` + `interceptsmoke`, `sockssmoke`, `volumesmoke`, `lifecycle` (idle→hibernate→archive), + `androidsmoke`, `browsersmoke`, `desktopsmoke`, and the `pullbench`/`pushbench`/`rpcbench`/`qaab` perf drivers - `boot/kernel/` — kernel version pin (`VERSION` + matching tarball `SHA256`, bump both together) + config fragment (amd64: over `x86_64_defconfig` + diff --git a/docs/desktop.md b/docs/desktop.md index 639e7f12..b2842ba4 100644 --- a/docs/desktop.md +++ b/docs/desktop.md @@ -20,8 +20,12 @@ up a few seconds later — poll `GET /screenshot` until it returns 200. ## Claim shape -- **Lane**: `net=none` for local tasks (os, office, file work); `net=egress` - when tasks visit the OSWorld mocked websites or the real web. +- **Lane**: `net=none` — for local tasks (os, office, file work) as it is, and + 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. `net=egress` on a guarded bridge gives the desktop no web: the NIC is + locked and the session's proxy setup does not follow the lock marker (a + CNI-backed lane still routes directly). - **Size**: `2xlarge` (8 CPU / 16G) — the t3.xlarge class the OSWorld AWS image runs on; the idle session is ~0.5 GB anonymous memory with gnome-shell around 290 MB RSS, and the headroom is for the apps. @@ -73,7 +77,9 @@ Configure it with `SANDBOXD_ADDR`, `SANDBOXD_TOKEN`, `COCOON_TEMPLATE` profile and debugging port. - On a guest with no NIC, `guest-proxy.service` points the session's browsers and `osworld-server`'s commands at silkd's loopback relay; a guest with a - NIC routes directly and the unit does nothing. + NIC routes directly and the unit does nothing. The unit tests NIC presence + only, so it does not follow the locked-NIC marker silkd honors on the bridge + egress lane; run desktop pools on the none lane. - No Thunderbird or VS Code yet (snap-only on 24.04 / vendor repo); tasks targeting them are out of scope for this flavor version. - x86_64 only. diff --git a/e2e/cmd/browsersmoke/main.go b/e2e/cmd/browsersmoke/main.go index f1ea03d2..be53fab5 100644 --- a/e2e/cmd/browsersmoke/main.go +++ b/e2e/cmd/browsersmoke/main.go @@ -18,8 +18,7 @@ import ( const ( cdpPort = 9222 - // Chromium starts after silkd, so the claim returns before CDP is up; - // a cold first launch on a loaded node can take tens of seconds. + // Chromium starts after silkd, so the claim returns before CDP is up; a cold first launch takes tens of seconds. cdpWait = 3 * time.Minute ) diff --git a/e2e/cmd/desktopsmoke/main.go b/e2e/cmd/desktopsmoke/main.go index 95d1aea2..b3c07173 100644 --- a/e2e/cmd/desktopsmoke/main.go +++ b/e2e/cmd/desktopsmoke/main.go @@ -19,8 +19,7 @@ import ( const ( serverPort = 5000 - // The GNOME session and osworld-server start after silkd; a cold first - // launch on a loaded node can take tens of seconds. + // the GNOME session and osworld-server start after silkd; a cold first launch on a loaded node takes tens of seconds. serverWait = 3 * time.Minute clickX = 300 clickY = 300 diff --git a/e2e/cmd/egresssmoke/main.go b/e2e/cmd/egresssmoke/main.go index 5975686d..edb5a5ab 100644 --- a/e2e/cmd/egresssmoke/main.go +++ b/e2e/cmd/egresssmoke/main.go @@ -70,8 +70,7 @@ func run(addr, token, template, wantToken, netShape, reach, nicAddr, echo string defer func() { _ = sb.Close() }() fmt.Printf(" claimed %s-lane sandbox %s\n", netShape, sb.ID) - // The image does not configure the NIC, so bring it up statically to give the - // guest a real route toward -reach; a blocked direct egress is then the lock. + // the image leaves the NIC unconfigured; a static address gives the guest a route toward -reach, so a blocked direct egress is the lock. if nicAddr != "" { out, cfgErr := sb.Exec(ctx, "sh", "-c", fmt.Sprintf( "nic=$(ip -o link | awk -F': ' '$2 ~ /^(eth|en)/{sub(/@.*/,\"\",$2); print $2; exit}'); "+ @@ -94,9 +93,7 @@ func run(addr, token, template, wantToken, netShape, reach, nicAddr, echo string } fmt.Println(" direct egress blocked") - // The injection origin is a public echo reached through the proxy: post-#26 - // the SSRF guard refuses private/loopback destinations even when allow-listed - // (issue #27, Option A), so a private origin cannot exercise injection. + // the SSRF guard refuses private and loopback destinations even when allow-listed, so injection needs a public echo. seen, err := sb.Exec(ctx, "sh", "-c", fmt.Sprintf("curl -s -x %s http://%s/get", proxy, echo)) if err != nil { return fmt.Errorf("proxied exec: %w", err) diff --git a/e2e/cmd/interceptsmoke/main.go b/e2e/cmd/interceptsmoke/main.go index 46f4a405..4c20fc22 100644 --- a/e2e/cmd/interceptsmoke/main.go +++ b/e2e/cmd/interceptsmoke/main.go @@ -73,7 +73,7 @@ func run(addr, token, template, echo, secret, issuer string) error { fmt.Printf(" guest's TLS leaf issued by this node's intermediate (%s)\n", strings.TrimSpace(gotIssuer)) } - blocked, _ := sb.Exec(ctx, "sh", "-c", fmt.Sprintf("curl -s -m 3 https://%s/ >/dev/null 2>&1 && echo REACHED || echo BLOCKED", echo)) + blocked, _ := sb.Exec(ctx, "sh", "-c", fmt.Sprintf("curl -s -m 3 --noproxy '*' https://%s/ >/dev/null 2>&1 && echo REACHED || echo BLOCKED", echo)) if strings.TrimSpace(blocked) != "BLOCKED" { return fmt.Errorf("guest reached the origin without the proxy: %q", strings.TrimSpace(blocked)) } diff --git a/e2e/cmd/sockssmoke/main.go b/e2e/cmd/sockssmoke/main.go index 7b722937..5d12d06b 100644 --- a/e2e/cmd/sockssmoke/main.go +++ b/e2e/cmd/sockssmoke/main.go @@ -1,8 +1,8 @@ // Command sockssmoke proves the SOCKS5 egress channel from inside a claimed // sandbox: an allowed host tunnels over 127.0.0.1:1080, a GET-only host is // reachable on the HTTP proxy but refused on SOCKS5, an unlisted host is -// refused, IMAPS rides the tunnel, and a pool without a policy leaves the -// host door unwired so the guest's dial is refused. +// refused, IMAPS rides the tunnel, and no tunnel opens from a pool without a +// policy. package main import ( @@ -84,9 +84,9 @@ func run(addr, token, template, echo, getOnly, imap string) error { } defer func() { _ = bare.Close() }() if code := curl(ctx, bare, "--socks5-hostname", socksProxy, "http://"+echo+"/get"); code != "000" { - return fmt.Errorf("policy-less pool served SOCKS5 with %q, want the host door unwired and the dial refused", code) + return fmt.Errorf("policy-less pool opened a SOCKS5 tunnel with %q, want none", code) } - fmt.Println(" pool without a policy: host door unwired, guest dial refused") + fmt.Println(" pool without a policy: no SOCKS5 tunnel opens") return nil } diff --git a/sandboxd/config/config_test.go b/sandboxd/config/config_test.go index f3473075..a4cf391a 100644 --- a/sandboxd/config/config_test.go +++ b/sandboxd/config/config_test.go @@ -5,6 +5,7 @@ import ( "os" "path/filepath" "slices" + "strconv" "strings" "testing" @@ -80,9 +81,11 @@ func TestAutoRefillConcurrency(t *testing.T) { {cpus: 384, want: 256}, {cpus: 768, want: 256}, } { - if got := autoRefillConcurrency(tt.cpus); got != tt.want { - t.Errorf("autoRefillConcurrency(%d) = %d, want %d", tt.cpus, got, tt.want) - } + t.Run(strconv.Itoa(tt.cpus)+" cpus", func(t *testing.T) { + if got := autoRefillConcurrency(tt.cpus); got != tt.want { + t.Errorf("autoRefillConcurrency(%d) = %d, want %d", tt.cpus, got, tt.want) + } + }) } } diff --git a/sandboxd/egress/policy.go b/sandboxd/egress/policy.go index c0bd492e..923e6e06 100644 --- a/sandboxd/egress/policy.go +++ b/sandboxd/egress/policy.go @@ -112,7 +112,7 @@ func (p Policy) EvalInner(host, method string) (Rule, Decision) { return Rule{}, DecisionDeny } -// Tunnels reports whether some rule can admit an opaque tunnel; false leaves the SOCKS5 listener unbound, and a composite answers true even when its two host sets never meet. +// Tunnels reports whether some rule can admit an opaque tunnel; a composite answers true even when its two host sets never meet. func (p Policy) Tunnels() bool { return slices.ContainsFunc(p.Allow, func(r Rule) bool { return !r.Intercept && r.matchMethod(http.MethodConnect) }) } diff --git a/sandboxd/egress/proxy.go b/sandboxd/egress/proxy.go index f898ea33..d451fbef 100644 --- a/sandboxd/egress/proxy.go +++ b/sandboxd/egress/proxy.go @@ -28,6 +28,9 @@ var hopHeaders = []string{ // DialFunc opens the upstream connection for a permitted request. type DialFunc func(ctx context.Context, network, addr string) (net.Conn, error) +// AuditFunc receives every egress decision the proxy takes. +type AuditFunc func(Event) + // Secrets resolves a rule's Secret name to the header it injects. type Secrets interface { Header(name string) (header, value string, ok bool) @@ -50,7 +53,7 @@ type Proxy struct { policy Evaluator secrets Secrets ca *CA - audit func(Event) + audit AuditFunc dial DialFunc tr *http.Transport mitmTr *http.Transport @@ -66,7 +69,7 @@ type Proxy struct { } // New builds a Proxy for one sandbox; secrets, ca, and audit may be nil, dial must not. -func New(sandbox, tenant string, policy Evaluator, secrets Secrets, ca *CA, dial DialFunc, audit func(Event)) *Proxy { +func New(sandbox, tenant string, policy Evaluator, secrets Secrets, ca *CA, dial DialFunc, audit AuditFunc) *Proxy { p := &Proxy{ sandbox: sandbox, tenant: tenant, diff --git a/sandboxd/egress/socks.go b/sandboxd/egress/socks.go index 2855abb6..adc07b56 100644 --- a/sandboxd/egress/socks.go +++ b/sandboxd/egress/socks.go @@ -28,18 +28,20 @@ const ( socksBadCmd = 0x07 socksBadAtyp = 0x08 - socksTimeout = 30 * time.Second + socksTimeout = 30 * time.Second + socksAcceptBackoff = 50 * time.Millisecond ) // ServeSOCKS serves SOCKS5 on ln until it closes; a tunnel takes the decision of an un-intercepted CONNECT. -func (p *Proxy) ServeSOCKS(ctx context.Context, ln net.Listener) error { +func (p *Proxy) ServeSOCKS(ctx context.Context, ln net.Listener) { for { conn, err := ln.Accept() + if errors.Is(err, net.ErrClosed) { + return + } if err != nil { - if errors.Is(err, net.ErrClosed) { - return nil - } - return err + time.Sleep(socksAcceptBackoff) + continue } go p.serveSocks(ctx, conn) } diff --git a/sandboxd/egress/socks_test.go b/sandboxd/egress/socks_test.go index 0251b500..c2f08c9b 100644 --- a/sandboxd/egress/socks_test.go +++ b/sandboxd/egress/socks_test.go @@ -157,7 +157,7 @@ func socksProxy(t *testing.T, policy Policy, ca *CA, dial DialFunc, events chan _ = ln.Close() p.Close() }) - go func() { _ = p.ServeSOCKS(t.Context(), ln) }() + go p.ServeSOCKS(t.Context(), ln) return p, ln.Addr().String() } diff --git a/sandboxd/engine/engine.go b/sandboxd/engine/engine.go index 53c9c6bc..5f1b8dda 100644 --- a/sandboxd/engine/engine.go +++ b/sandboxd/engine/engine.go @@ -479,7 +479,6 @@ func parseSemver(s string) ([3]int, bool) { return out, true } -// readBufLine takes one line from r, capped at max bytes. func readBufLine(r *bufio.Reader, max int) (string, error) { line, err := r.ReadSlice('\n') if err != nil { diff --git a/sandboxd/pool/egress.go b/sandboxd/pool/egress.go index 5fce0b83..52a8940c 100644 --- a/sandboxd/pool/egress.go +++ b/sandboxd/pool/egress.go @@ -157,7 +157,7 @@ func (m *Manager) armEgressProxy(ctx context.Context, sb *types.Sandbox) error { m.mu.Unlock() go func() { _ = el.srv.Serve(el.ln) }() if el.socks != nil { - go func() { _ = proxy.ServeSOCKS(evCtx, el.socks) }() + go proxy.ServeSOCKS(evCtx, el.socks) } return nil } diff --git a/sandboxd/pool/pool.go b/sandboxd/pool/pool.go index 7bd0daf7..57c9af00 100644 --- a/sandboxd/pool/pool.go +++ b/sandboxd/pool/pool.go @@ -44,9 +44,9 @@ const ( coldProbeTimeout = 90 * time.Second // a clone answers in ~1s even saturated, so a silent one is replaced, not waited on warmProbeTimeout = 5 * time.Second - // One list; a wrong answer only costs an extra sweep. + // one list; a wrong answer only costs an extra sweep. removeVerifyTimeout = 15 * time.Second - // A full node clears only when VMs go away; retrying sooner buys nothing. + // a full node clears only when VMs go away; retrying sooner buys nothing. capacityBackoff = buildRetryDelay vsockPollInterval = 100 * time.Millisecond defaultTTL = 5 * time.Minute @@ -55,14 +55,14 @@ const ( recommitMaxBackoff = 5 * time.Second // one failed boot is ordinary; only an unbroken run predicts the next failure refillFailStreak = 8 - // Doubles per failure past the streak. + // doubles per failure past the streak. refillBackoffBase = 250 * time.Millisecond refillBackoffMax = buildRetryDelay // fallbacks when a Manager is built from a Config that skipped config.Load's defaulting defaultMaxFork = 16 defaultRefill = 4 - // A heal pulls a whole guest memory image, so a few saturate disk and NIC. + // a heal pulls a whole guest memory image, so a few saturate disk and NIC. maxConcurrentHeals = 4 vmPrefix = "sbx-" @@ -92,7 +92,7 @@ var ( ErrNoEgressFork = errors.New("egress-lane sandboxes cannot fork, checkpoint, or promote: a resumed guest egresses before its fresh tap can be locked") ErrVolumeCapture = errors.New("sandboxes with volumes cannot hibernate, fork, checkpoint, or promote") ErrVolumeBusy = errors.New("volume is held by another claim") - // Replaying a journal takes a writable mount, so readers stay out. + // replaying a journal takes a writable mount, so readers stay out. ErrVolumeNeedsRecovery = errors.New("volume needs recovery by a writable claim") ErrQuota = errors.New("node claim quota reached") @@ -691,7 +691,6 @@ func logSweepResult(ctx context.Context, logger *log.Fields, err error, okMsg, f } } -// benignSweepErr reports whether err is the expected outcome of a housekeeping sweep. func benignSweepErr(err error) bool { return errors.Is(err, ErrUnknownSandbox) || errors.Is(err, errWokeMeanwhile) || errors.Is(err, ErrNoEgressHibernate) diff --git a/sandboxd/pool/reconcile.go b/sandboxd/pool/reconcile.go index 7fff4ed9..dfe3e669 100644 --- a/sandboxd/pool/reconcile.go +++ b/sandboxd/pool/reconcile.go @@ -50,7 +50,7 @@ func (m *Manager) Reconcile(ctx context.Context) error { // running with either flag set means a crashed wake or an intent the engine never saw sb.HibernateSnap, sb.PendingSnap = "", "" case ok && sb.HibernateSnap != "": - // Hibernated: the VM is stopped by design and wakes on demand. + // hibernated: the VM is stopped by design and wakes on demand. sb.PendingSnap = "" case ok && sb.PendingSnap != "" && (snapsErr != nil || slices.Contains(snaps, sb.PendingSnap)): // the journaled intent names the wake image, so adopt it when the snapshot is there @@ -139,7 +139,7 @@ func (m *Manager) removeStaleVM(ctx context.Context, name string, rec types.VMRe // a canceled ctx must not skip the busy check guarding the forced remove switch outcome, err := m.eng.ReconcileStaleCreate(context.WithoutCancel(ctx), name); { case err != nil: - // Verb missing (cocoon < v0.5.8) or failed: keep the old sweep. + // verb missing (cocoon < v0.5.8) or failed: keep the old sweep. logger.Warnf(ctx, "reconcile stale create %s: %v; removing", name, err) case outcome == engine.StaleCreateCollected, outcome == engine.StaleCreateNotFound: return true @@ -176,6 +176,9 @@ func (m *Manager) resyncEgress(ctx context.Context, live map[string]types.VMReco if err == nil && !locked[tap] { err = netfilter.Lock(tap) } + if err == nil && !locked[tap] { + err = m.markLockedNIC(ctx, sb.Key, sb.VsockSocket) + } if err != nil { logger.Errorf(ctx, err, "ensure egress lock %s; quarantining", sb.ID) quarantine = append(quarantine, sb) @@ -186,7 +189,7 @@ func (m *Manager) resyncEgress(ctx context.Context, live map[string]types.VMReco logger.Errorf(ctx, proxyErr, "arm egress proxy %s", sb.ID) } } - // Quarantine before the sweep so a failed remove's still-running tap is kept. + // quarantine before the sweep so a failed remove's still-running tap is kept. keep := make(map[string]bool, len(live)) for _, sb := range quarantine { if m.quarantineClaim(ctx, sb) { @@ -195,7 +198,7 @@ func (m *Manager) resyncEgress(ctx context.Context, live map[string]types.VMReco keep[sb.TAP] = true } } - // Keep every still-running VM's tap; sweep only a confirmed-gone VM's table. + // keep every still-running VM's tap; sweep only a confirmed-gone VM's table. for name, rec := range live { if tap := rec.TapDevice(); tap != "" && !removed[name] { keep[tap] = true @@ -206,7 +209,7 @@ func (m *Manager) resyncEgress(ctx context.Context, live map[string]types.VMReco } } -// A failed remove stays out of service and queued until teardown succeeds. +// quarantineClaim parks a claim whose teardown failed: out of service and queued until a remove succeeds. func (m *Manager) quarantineClaim(ctx context.Context, sb *types.Sandbox) bool { td := m.quiesceVolumes(ctx, sb) gone := m.removeOrRetry(ctx, sb.VMName, sb.ID, "", td) diff --git a/sdk/langchain/cocoonsandbox_langchain/toolkit.py b/sdk/langchain/cocoonsandbox_langchain/toolkit.py index bf9d1e81..8c279f14 100644 --- a/sdk/langchain/cocoonsandbox_langchain/toolkit.py +++ b/sdk/langchain/cocoonsandbox_langchain/toolkit.py @@ -48,7 +48,7 @@ def __init__( net: str = "", ttl_seconds: int = 0, from_checkpoint: str = "", - ): + ) -> None: self._client = Client(addr, api_token=api_token, timeout=CALL_TIMEOUT) self._template = template self._net = net @@ -61,7 +61,7 @@ def __init__( def __enter__(self) -> CocoonToolkit: return self - def __exit__(self, *exc) -> None: + def __exit__(self, *exc: object) -> None: self.close() def get_tools(self) -> list[StructuredTool]: diff --git a/sdk/python/cocoonsandbox/client.py b/sdk/python/cocoonsandbox/client.py index 943f5d51..bedc4f23 100644 --- a/sdk/python/cocoonsandbox/client.py +++ b/sdk/python/cocoonsandbox/client.py @@ -26,7 +26,7 @@ class Client: """Talks to one sandboxd node (and, transparently, its cluster).""" - def __init__(self, addr: str, api_token: str = "", timeout: float = 120.0): + def __init__(self, addr: str, api_token: str = "", timeout: float = 120.0) -> None: self.addr = addr.split(",")[0].strip() self.api_token = api_token self.timeout = timeout @@ -143,7 +143,7 @@ def post(peer): owner, reply = _redirect_fallback(addr, redirect, post, verb) return self._handle_from(owner, reply) - def _peers(self) -> list: + def _peers(self) -> list[str]: # /v1/peers is tenant-accessible; /v1/info is operator-only, so a tenant cannot read peers from it. try: return ( @@ -171,7 +171,7 @@ def _post_json(self, addr: str, path: str, body: dict, verb: str) -> dict: return self._request(addr, "POST", path, body, verb) def _request( - self, addr: str, method: str, path: str, body, verb: str, bearer: str = "", timeout: float = 0.0 + self, addr: str, method: str, path: str, body: dict | None, verb: str, bearer: str = "", timeout: float = 0.0 ) -> dict: """Issues one control-plane request. bearer overrides the api token — sandbox-scoped verbs (release, hibernate) authenticate with the diff --git a/sdk/python/cocoonsandbox/sandbox.py b/sdk/python/cocoonsandbox/sandbox.py index 0f357330..f6dcf450 100644 --- a/sdk/python/cocoonsandbox/sandbox.py +++ b/sdk/python/cocoonsandbox/sandbox.py @@ -34,7 +34,7 @@ def __init__( from_checkpoint: str = "", template_digest: str = "", volumes: list[dict] | None = None, - ): + ) -> None: self._client = client self.id = id self.token = token @@ -426,7 +426,7 @@ def _drain_proc( class Session(_Closeable): """A persistent shell inside the sandbox, addressed by id.""" - def __init__(self, sandbox: Sandbox, id: str): + def __init__(self, sandbox: Sandbox, id: str) -> None: self._sandbox = sandbox self.id = id @@ -441,7 +441,7 @@ def close(self) -> None: class Watcher(_Closeable): """A live filesystem event stream; iterate for {kind, path} events.""" - def __init__(self, conn: Conn): + def __init__(self, conn: Conn) -> None: self._conn = conn self.error: Exception | None = None @@ -463,7 +463,7 @@ def close(self) -> None: class Pty(_Closeable): """An interactive shell under a guest pty; read/write are raw bytes.""" - def __init__(self, sandbox: Sandbox, conn: Conn, pid: int): + def __init__(self, sandbox: Sandbox, conn: Conn, pid: int) -> None: self._sandbox = sandbox self._conn = conn self.pid = pid @@ -492,7 +492,7 @@ class Lsp(_Closeable): """A language server in the sandbox, spoken to over the relay. silkd is a broker: it pipes JSON-RPC bytes; the caller frames and correlates.""" - def __init__(self, sandbox: Sandbox, server_id: str): + def __init__(self, sandbox: Sandbox, server_id: str) -> None: self._sandbox = sandbox self.server_id = server_id @@ -513,7 +513,7 @@ def stop(self) -> None: class PortConn(_Closeable): """A byte stream to a guest port, relayed over the silkd connection.""" - def __init__(self, conn: Conn): + def __init__(self, conn: Conn) -> None: self._conn = conn self._eof = False diff --git a/silkd/src/find.rs b/silkd/src/find.rs index 104a2039..72cd703b 100644 --- a/silkd/src/find.rs +++ b/silkd/src/find.rs @@ -18,7 +18,7 @@ pub const FIND_MAX_FILE: u64 = 8 * 1024 * 1024; /// Match frames in flight between the walking thread and the writer. const MATCH_QUEUE: usize = 256; -/// Bytes of match content in flight; 256 size-bound single-line frames would pin gigabytes. +/// Bytes of match content in flight: above the largest single match, so a reserve never blocks forever, and far below the gigabytes 256 size-bound frames would pin. const MATCH_QUEUE_BYTES: usize = 2 * FIND_MAX_FILE as usize; struct MatchBudget { diff --git a/silkd/src/server.rs b/silkd/src/server.rs index 46ca0e1b..4971018e 100644 --- a/silkd/src/server.rs +++ b/silkd/src/server.rs @@ -96,12 +96,12 @@ impl State { proto::write_frame(&mut writer, &Response::SessionCreated { id }).await } Err(e) => { - proto::error_frame( - &mut writer, - ErrorKind::Internal, - format!("session create: {e}"), - ) - .await + let kind = if e.kind() == std::io::ErrorKind::InvalidInput { + ErrorKind::BadRequest + } else { + ErrorKind::Internal + }; + proto::error_frame(&mut writer, kind, format!("session create: {e}")).await } } } diff --git a/silkd/src/session.rs b/silkd/src/session.rs index b0f2ecfc..b2ac70e5 100644 --- a/silkd/src/session.rs +++ b/silkd/src/session.rs @@ -42,6 +42,12 @@ impl Table { Some(s) if !s.is_empty() => s, _ => format!("sess-{}", sysutil::tmp_suffix()), }; + if let Some(name) = env.keys().find(|k| !is_env_name(k)) { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + format!("environment name {name:?} is not a shell identifier"), + )); + } let mut child = Command::new("bash") .env_clear() .envs(sysutil::base_env()) @@ -82,7 +88,7 @@ impl Table { shell_quote_into(&mut init, k); init.push('='); shell_quote_into(&mut init, v); - init.push('\n'); + init.push_str(" || exit 1\n"); } { let mut io = session.io.lock().await; @@ -287,10 +293,28 @@ fn shell_quote_into(out: &mut String, s: &str) { out.push('\''); } +fn is_env_name(name: &str) -> bool { + let mut chars = name.chars(); + chars + .next() + .is_some_and(|c| c == '_' || c.is_ascii_alphabetic()) + && chars.all(|c| c == '_' || c.is_ascii_alphanumeric()) +} + #[cfg(test)] mod tests { use super::*; + #[test] + fn is_env_name_accepts_only_shell_identifiers() { + for ok in ["A", "_x", "HTTP_PROXY", "a1"] { + assert!(is_env_name(ok), "{ok:?}"); + } + for bad in ["", "1a", "A B", "a-b", "a=b", "é"] { + assert!(!is_env_name(bad), "{bad:?}"); + } + } + #[test] fn shell_quote_into_pins_exact_output() { let cases = [ diff --git a/silkd/tests/session_e2e.rs b/silkd/tests/session_e2e.rs index 5ec58fe1..b2969826 100644 --- a/silkd/tests/session_e2e.rs +++ b/silkd/tests/session_e2e.rs @@ -72,9 +72,9 @@ async fn session_applies_cwd_and_env_at_create() { } #[tokio::test] -async fn session_create_with_hostile_env_key_still_answers() { +async fn session_create_rejects_an_env_name_bash_cannot_export() { let state = Arc::new(State::new()); - let created = tokio::time::timeout( + let f = tokio::time::timeout( Duration::from_secs(8), one( &state, @@ -83,10 +83,33 @@ async fn session_create_with_hostile_env_key_still_answers() { ) .await .expect("session_create hung on a hostile env key"); - assert_eq!(type_of(&created[0]), "session_created", "{created:?}"); - let id = created[0]["id"].as_str().unwrap(); - let got = sh(&state, id, &["sh", "-c", "echo $GOOD"]).await; - assert_eq!(stdout_body(&got).trim(), "ok", "{got:?}"); + assert_eq!(type_of(&f[0]), "error", "{f:?}"); + assert_eq!(f[0]["kind"], "bad_request", "{f:?}"); + assert!( + state.sessions.list().is_empty(), + "{:?}", + state.sessions.list() + ); +} + +#[tokio::test] +async fn session_create_fails_when_bash_refuses_an_export() { + let state = Arc::new(State::new()); + let f = tokio::time::timeout( + Duration::from_secs(8), + one( + &state, + &json!({"op":"session_create","env":{"UID":"1"}}).to_string(), + ), + ) + .await + .expect("session_create hung on a readonly variable"); + assert_eq!(type_of(&f[0]), "error", "{f:?}"); + assert!( + state.sessions.list().is_empty(), + "{:?}", + state.sessions.list() + ); } #[tokio::test]