From 4253bee7f4c6923c96d8820c506ad6a7af7e428f Mon Sep 17 00:00:00 2001 From: Evan Spearman Date: Tue, 28 Jul 2026 20:51:01 +0000 Subject: [PATCH 1/2] feat: always set certain important variables automatically --- .minimal/minimal.toml | 3 + crates/common/src/lib.rs | 34 ++++- crates/minimal/src/lib.rs | 18 +++ crates/minimald/src/net/switch.rs | 2 +- crates/minimald/src/session.rs | 112 ++++++++++++++- crates/minimald/src/session_host.rs | 135 +++++++++++++----- crates/sandbox2/src/lib.rs | 36 ++++- crates/sessions/example_project/minimal.toml | 8 +- crates/sessions/src/core/compose.rs | 138 ++++++++++++++----- crates/sessions/src/core/primitives.rs | 36 +++++ crates/sessions/src/daemon/composer.rs | 9 +- 11 files changed, 440 insertions(+), 91 deletions(-) diff --git a/.minimal/minimal.toml b/.minimal/minimal.toml index 6e5379c94..0bb158b64 100644 --- a/.minimal/minimal.toml +++ b/.minimal/minimal.toml @@ -65,6 +65,9 @@ path = "usr/share/microvm-rootfs/rootfs.img" type = "oci-image" packages = ["libkrun"] +[session] +packages = ["just", "protobuf"] + [tasks.vim] profile = "demo" state_key = "" diff --git a/crates/common/src/lib.rs b/crates/common/src/lib.rs index b11f155a7..736577eb0 100644 --- a/crates/common/src/lib.rs +++ b/crates/common/src/lib.rs @@ -208,18 +208,40 @@ pub fn hardlink_dir_contents(src: &Path, dst: &Path) -> Result<(), HardlinkError } else if metadata.is_file() { match fs::hard_link(&path, &dst_path) { Ok(()) => Ok(()), - Err(e) => { - if e.kind() == std::io::ErrorKind::AlreadyExists { + Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => { + warn!( + "Not linking {} => {}, already exists", + path.display(), + dst_path.display() + ); + Ok(()) + } + // The cache and destination can live on different filesystems + // (e.g. a per-VM `/state` volume vs. the rootfs holding + // `/home`), where hardlinks are impossible (`EXDEV`). Fall back + // to a copy so materialization still succeeds — slower and no + // longer deduplicated, but correct. + Err(e) if e.raw_os_error() == Some(libc::EXDEV) => { + // Every file in a cross-device tree hits EXDEV, so warn only + // on the first — a per-file log would flood with thousands + // of identical lines. Once-per-process is enough: the cause + // is a fixed filesystem-layout fact, not a per-file + // condition. + use std::sync::atomic::{AtomicBool, Ordering}; + static WARNED: AtomicBool = AtomicBool::new(false); + if !WARNED.swap(true, Ordering::Relaxed) { warn!( - "Not linking {} => {}, already exists", + "Copying instead of hardlinking: cache and \ + destination are on different filesystems; further \ + cross-device copies this run are silent (first: {} \ + => {})", path.display(), dst_path.display() ); - Ok(()) - } else { - Err(e) } + fs::copy(&path, &dst_path).map(|_| ()) } + Err(e) => Err(e), } .map_err(|e| HardlinkError::HardlinkFailed(path.to_path_buf(), dst_path, e))?; } else if metadata.is_symlink() { diff --git a/crates/minimal/src/lib.rs b/crates/minimal/src/lib.rs index 6e167500b..2463a59e0 100644 --- a/crates/minimal/src/lib.rs +++ b/crates/minimal/src/lib.rs @@ -1873,9 +1873,27 @@ async fn attach_to_session( let [strict, known_hosts_file] = host_key_opts(&sock.with_file_name(paths::KNOWN_HOSTS_FILE)); let mut ssh = std::process::Command::new("ssh"); + // Pin the shell ssh uses to run the ProxyCommand. ssh launches a + // ProxyCommand via `$SHELL -c` and execs `$SHELL` with no PATH lookup, so a + // caller whose `$SHELL` is a bare name (`fish`) or points at a shell absent + // from this context fails with ": No such file or directory" and the + // transport dies at "banner exchange … Broken pipe". Our ProxyCommand is a + // full-path `min proxy …` that needs nothing but a POSIX `sh`, so force the + // always-present `/bin/sh` rather than inherit the user's interactive shell. + ssh.env("SHELL", "/bin/sh"); ssh.env("MINIMAL_SESSION_ID", id.to_string()).args([ "-o", "SendEnv=MINIMAL_SESSION_ID", + // Forward the user's locale and timezone into the session, mirroring a + // conventional `SendEnv LANG LC_* TZ`. The daemon accepts only these + // (its `AcceptEnv` allowlist) and folds them in below any loadout. + // `TERM` needs no `SendEnv`: ssh always carries it in the PTY request. + "-o", + "SendEnv=LANG", + "-o", + "SendEnv=LC_*", + "-o", + "SendEnv=TZ", "-o", &format!("ProxyCommand={proxy_cmd}"), "-o", diff --git a/crates/minimald/src/net/switch.rs b/crates/minimald/src/net/switch.rs index 129595326..19080308b 100644 --- a/crates/minimald/src/net/switch.rs +++ b/crates/minimald/src/net/switch.rs @@ -601,7 +601,7 @@ fn blocked_syn(frame: &[u8], allowed: &HashSet) -> Option<(u16, SocketAddrV return None; } let (syn, ack) = (pkt.tcp_flags & 0x02 != 0, pkt.tcp_flags & 0x10 != 0); - if !(syn && !ack) || allowed.contains(&pkt.dst.port()) { + if !syn || ack || allowed.contains(&pkt.dst.port()) { return None; } Some((pkt.dst.port(), pkt.src)) diff --git a/crates/minimald/src/session.rs b/crates/minimald/src/session.rs index 216d0ed0f..f41705366 100644 --- a/crates/minimald/src/session.rs +++ b/crates/minimald/src/session.rs @@ -108,6 +108,21 @@ pub(crate) fn registry_name(record: &Record) -> String { } } +/// The server-side `AcceptEnv` allowlist: locale and timezone vars a client is +/// permitted to forward from its shell into the session (OpenSSH's default +/// `AcceptEnv LANG LC_*`, plus `TZ`). Everything else the client set on the +/// channel — e.g. `MINIMAL_SESSION_ID`, `TRACEPARENT` — is control plumbing and +/// must not leak into the shell environment, so it is filtered out here. +fn inherited_session_env( + channel_env: &std::collections::BTreeMap, +) -> Vec<(String, String)> { + channel_env + .iter() + .filter(|(k, _)| k.as_str() == "LANG" || k.as_str() == "TZ" || k.starts_with("LC_")) + .map(|(k, v)| (k.clone(), v.clone())) + .collect() +} + /// An error that occurred when attaching to a running session/its-shell. #[derive(Debug)] pub enum AttachError { @@ -1093,6 +1108,31 @@ impl Session { None => return Err(AttachError::NoPty), }); + // Capture the environment this attach contributes to the shell it may + // mint: the locale/timezone vars the client forwarded (folded as + // defaults below the composition) and the per-connection facts folded + // above it. Currently the only connection fact is `TERM`, from the + // client's PTY request. + // + // `SSH_TTY` and `SSH_CONNECTION`/`SSH_CLIENT` are intentionally omitted: + // the session sandbox has no host `/dev/pts` and the transport is a + // local Unix socket (no peer IP/port), so any value would name something + // that doesn't exist in-session and would only mislead audit logs, + // source-IP checks, or `$SSH_TTY` consumers. + let attach_env = { + let inherited = inherited_session_env(&config.env_vars); + let mut connection = Vec::new(); + if let Some(pty) = config.pty.as_ref() + && !pty.term.is_empty() + { + connection.push(("TERM".to_string(), pty.term.clone())); + } + session_host::AttachEnv { + inherited, + connection, + } + }; + // A session that was created but never had its loadout configured has // nothing in flight, so attaching to it shouldn't be an error: set it // up now, with an empty contribution, and carry on into the attach. @@ -1175,7 +1215,7 @@ impl Session { }; match host { None => { - self.mint_session_host(session_hnd, conn_username, channel, sz) + self.mint_session_host(session_hnd, conn_username, channel, sz, attach_env) .await } Some((h, _)) => { @@ -1183,7 +1223,7 @@ impl Session { Ok(()) => Ok(()), Err((channel, sz)) => { // session host is dead - self.mint_session_host(session_hnd, conn_username, channel, sz) + self.mint_session_host(session_hnd, conn_username, channel, sz, attach_env) .await } } @@ -1197,6 +1237,7 @@ impl Session { conn_username: String, channel: Channel, sz: WinSize, + attach_env: session_host::AttachEnv, ) -> Result<(), AttachError> { let record = self.record.record().await.unwrap(); let paths = self.paths().await; @@ -1206,7 +1247,9 @@ impl Session { let control = SessionControl::new(self.manager.clone(), record.id); // Spawn/setup the session in a closure that reports progress down the terminal. - let launcher = self.session_launcher(session_hnd, &record).await?; + let launcher = self + .session_launcher(session_hnd, &record, attach_env) + .await?; let progress = ChannelProgress::new(channel, self.tracker.clone(), (sz.cols, sz.rows)); let (channel, spawned) = progress .run(Box::pin(session_host::Host::spawn( @@ -1253,6 +1296,7 @@ impl Session { &mut self, session: SessionHandle, record: &Record, + attach_env: session_host::AttachEnv, ) -> Result { // R2.1: reject a policy that is incompatible with the network mode // (e.g. egress on a non-`OwnIp` PTask) before launching the host. @@ -1269,6 +1313,7 @@ impl Session { .context(true) .await .map_err(AttachError::ContextCreationFailed)?, + attach_env, network_mode, net_switch: Arc::clone(&self.net_switch), ingress, @@ -1287,6 +1332,7 @@ impl Session { &mut self, _session: SessionHandle, record: &Record, + _attach_env: session_host::AttachEnv, ) -> Result { // Mirror the production R2.1 gate so test launches reject a // policy/network-mode mismatch the same way production does. @@ -1722,6 +1768,66 @@ mod tests { use crate::test_harness::{TestClient, TestServer, create_configured_session}; + /// The `AcceptEnv` allowlist keeps locale + timezone vars and drops + /// everything else — critically the control-plane vars, which must never + /// reach the shell environment. + #[test] + fn inherited_session_env_keeps_only_locale_and_tz() { + let env: std::collections::BTreeMap = [ + ("LANG", "en_US.UTF-8"), + ("LC_CTYPE", "en_US.UTF-8"), + ("LC_ALL", "C"), + ("TZ", "America/New_York"), + ("MINIMAL_SESSION_ID", "00000000-0000-0000-0000-000000000000"), + ("TRACEPARENT", "00-abc-def-01"), + ("PATH", "/evil/bin"), + ("PS1", "# "), + ] + .into_iter() + .map(|(k, v)| (k.to_string(), v.to_string())) + .collect(); + + let kept: std::collections::BTreeMap = + super::inherited_session_env(&env).into_iter().collect(); + + assert_eq!(kept.get("LANG").map(String::as_str), Some("en_US.UTF-8")); + assert_eq!( + kept.get("LC_CTYPE").map(String::as_str), + Some("en_US.UTF-8") + ); + assert_eq!(kept.get("LC_ALL").map(String::as_str), Some("C")); + assert_eq!(kept.get("TZ").map(String::as_str), Some("America/New_York")); + // Control-plane routing/tracing vars and everything else must be dropped. + assert!(!kept.contains_key("MINIMAL_SESSION_ID")); + assert!(!kept.contains_key("TRACEPARENT")); + assert!(!kept.contains_key("PATH")); + assert!(!kept.contains_key("PS1")); + assert_eq!(kept.len(), 4, "only LANG, LC_*, and TZ should survive"); + } + + /// A `LC_`-*prefixed* var is accepted, but a bare `LC` (or one that merely + /// contains `LC_`) is not — the filter is a prefix match, not a substring. + #[test] + fn inherited_session_env_prefix_not_substring() { + let env: std::collections::BTreeMap = [ + ("LC_MESSAGES", "C"), + ("LC", "nope"), + ("MYLC_VAR", "nope"), + ("XLANG", "nope"), + ] + .into_iter() + .map(|(k, v)| (k.to_string(), v.to_string())) + .collect(); + + let kept: std::collections::BTreeMap = + super::inherited_session_env(&env).into_iter().collect(); + + assert_eq!( + kept.keys().cloned().collect::>(), + vec!["LC_MESSAGES"] + ); + } + /// Reads the session record for `id`, or `None` once it has been deleted. async fn record_exists(client: &mut TestClient, id: SessionId) -> bool { client diff --git a/crates/minimald/src/session_host.rs b/crates/minimald/src/session_host.rs index 293038f57..43fa7a496 100644 --- a/crates/minimald/src/session_host.rs +++ b/crates/minimald/src/session_host.rs @@ -10,8 +10,6 @@ use russh::Channel; use russh::server::Msg; #[cfg(not(test))] use sandbox2::Network; -#[cfg(not(test))] -use std::collections::HashMap; use std::future::Future; use std::io; use std::io::{Read, Write}; @@ -167,7 +165,7 @@ impl Pty { /// from?" back to the loadout / project / package that contributed /// it. /// -/// Baseline items (the launcher-defaults `PS1`, `base`, `coreutils`, +/// Baseline packages (the launcher-defaults `base`, `coreutils`, /// `socat`) log with `source = "launcher-baseline"` so they can be /// distinguished from composition contributions. Patches and hooks /// still log even though the launcher can't act on them yet — an @@ -181,7 +179,6 @@ impl Pty { fn log_session_contents( session_name: &str, baseline_packages: &[&str], - baseline_var_names: &[&str], composition: Option<&sessions::core::compose::Composition>, ) { for p in baseline_packages { @@ -193,15 +190,6 @@ fn log_session_contents( "session content", ); } - for k in baseline_var_names { - tracing::info!( - session = session_name, - domain = "var", - name = k, - source = "launcher-baseline", - "session content", - ); - } let Some(comp) = composition else { return; }; @@ -831,20 +819,57 @@ impl SessionProcess for SandboxProcess { #[cfg(not(test))] const BASELINE_PACKAGES: &[&str] = &["base", "coreutils", "socat"]; -/// Env vars every session sandbox gets unconditionally, regardless of -/// the client's contribution. `PS1` is here so the shell prompt is -/// styled the same whether a composition sets it or not. -#[cfg(not(test))] -const BASELINE_VARS: &[(&str, &str)] = &[( - "PS1", - r"\[\033[01;32m\]\u@\h\[\033[00m\]:\[\033[01;34m\]\w\[\033[00m\]\$ ", -)]; +/// Environment folded into a session shell at the launching attach, over and +/// above the composition. Both halves are captured from the SSH channel that +/// mints the shell (see [`crate::session::Session::attach`]); a re-attach to an +/// already-running shell does not revisit them. +/// +/// The two halves sit on opposite sides of the composition in precedence: +/// `inherited` are defaults the composition may override, `connection` are +/// authoritative facts that override the composition. +/// +/// The fields are read only by the real [`SandboxLauncher`] (`cfg(not(test))`); +/// the mock launcher ignores them, so tolerate them being unread under `test`. +#[derive(Debug, Default, Clone)] +#[cfg_attr(test, allow(dead_code))] +pub(crate) struct AttachEnv { + /// Locale/timezone vars the client forwarded from its shell (`LANG`, + /// `LC_*`, `TZ`) — OpenSSH's `AcceptEnv` set. Applied as defaults *below* + /// the composition, so a loadout's explicit locale still wins. + pub(crate) inherited: Vec<(String, String)>, + /// Per-connection facts — currently just `TERM` from the PTY request. + /// Applied *above* the composition, the way sshd sets `TERM` + /// authoritatively regardless of shell dotfiles. (`SSH_TTY` and + /// `SSH_CONNECTION`/`SSH_CLIENT` are deliberately not set: the session + /// sandbox has no host `/dev/pts` and the Unix-socket transport has no peer + /// address, so any value would name something that doesn't exist in-session.) + pub(crate) connection: Vec<(String, String)>, +} + +/// Layers a session shell's environment by precedence, lowest first: the +/// client-forwarded `inherited` locale/timezone, then the `composition` vars +/// (which may override the inherited defaults), then the `connection` facts +/// (which override everything — sshd-style). Later inserts win on a shared key. +fn layer_session_env( + inherited: Vec<(String, String)>, + composition: Vec<(String, String)>, + connection: Vec<(String, String)>, +) -> std::collections::HashMap { + let mut env = std::collections::HashMap::new(); + for (k, v) in inherited.into_iter().chain(composition).chain(connection) { + env.insert(k, v); + } + env +} /// The real [`SessionLauncher`]: evaluates a minimal context into a graph, /// builds a sandboxed `/bin/bash`, and wires it to a freshly opened PTY. #[cfg(not(test))] pub(crate) struct SandboxLauncher { pub(crate) ctx: mctx::Context, + /// Env captured from the SSH channel that mints this shell; see + /// [`AttachEnv`]. + pub(crate) attach_env: AttachEnv, pub(crate) network_mode: NetworkMode, /// Shared per-host gvproxy switch. Used only for /// [`NetworkMode::OwnIp`] launches. @@ -931,6 +956,7 @@ impl SessionLauncher for SandboxLauncher { // the sandbox env below. let session_name = name.clone(); let composition = self.composition; + let attach_env = self.attach_env; let session = self.session; // `graph_from_all_packages` is CPU-heavy (nickel evaluation, // graph construction) — run it on the blocking pool so it @@ -996,8 +1022,11 @@ impl SessionLauncher for SandboxLauncher { // contribution the composer collected. Packages: baseline set // (required for a usable interactive shell) unioned with // everything the composition asks for, dedup-preserving-order - // so the base packages install first. Env vars: baseline - // `PS1` first, composition vars overwrite on the same key. + // so the base packages install first. Env vars: purely the + // composition's, since the launcher no longer forces any + // baseline var — sandbox2 sets the session defaults (`PS1`, + // `PATH`, `HOME`, `LANG`, …) which these composed vars then + // override on a shared key. // // Baseline is intentionally minimal: `base` for the shell, // `coreutils` for `ls`/`cat`/etc, and `socat` for the @@ -1025,10 +1054,6 @@ impl SessionLauncher for SandboxLauncher { BASELINE_PACKAGES.iter().map(|s| (*s).to_string()).collect(); let mut package_set: std::collections::HashSet = BASELINE_PACKAGES.iter().map(|s| (*s).to_string()).collect(); - let mut env_vars: HashMap = BASELINE_VARS - .iter() - .map(|(k, v)| ((*k).to_string(), (*v).to_string())) - .collect(); if let Some(comp) = &composition { for p in comp.packages() { let name = p.package(); @@ -1036,22 +1061,29 @@ impl SessionLauncher for SandboxLauncher { packages.push(name.to_string()); } } - for v in comp.vars() { - let var = v.var(); - env_vars.insert(var.name().to_string(), var.value().to_string()); - } } + // Env vars, layered by precedence (see [`layer_session_env`]): the + // client-forwarded locale/timezone sit below the composition, and the + // per-connection facts (`TERM`) sit above it. + let composition_vars: Vec<(String, String)> = composition + .as_ref() + .map(|c| { + c.vars() + .iter() + .map(|v| (v.var().name().to_string(), v.var().value().to_string())) + .collect() + }) + .unwrap_or_default(); + let env_vars = layer_session_env( + attach_env.inherited, + composition_vars, + attach_env.connection, + ); // Log every item that will (or would) end up in the session, // tagged with its provenance. Patches and lifecycle hooks are // included even though the launcher can't act on them yet — // an operator inspecting logs should see the intent. - let baseline_var_names: Vec<&str> = BASELINE_VARS.iter().map(|(k, _)| *k).collect(); - log_session_contents( - &name, - BASELINE_PACKAGES, - &baseline_var_names, - composition.as_deref(), - ); + log_session_contents(&name, BASELINE_PACKAGES, composition.as_deref()); // Build the env + container and spawn the process. Any failure here (env // build, container build, spawn) leaves no process to reap; the phase-1 @@ -1704,6 +1736,33 @@ mod tests { ypixel: 0, }; + /// Precedence: client-forwarded `inherited` sits below the composition, + /// which sits below the per-connection `connection` facts. Later layers win + /// on a shared key; non-colliding keys from every layer survive. + #[test] + fn layer_session_env_precedence() { + let sv = |k: &str, v: &str| (k.to_string(), v.to_string()); + let env = layer_session_env( + // inherited: LANG is overridden by composition; TZ survives. + vec![sv("LANG", "de_DE.UTF-8"), sv("TZ", "Europe/Berlin")], + // composition: beats inherited LANG; its TERM is overridden by the + // connection; EDITOR survives. + vec![ + sv("LANG", "fr_FR.UTF-8"), + sv("TERM", "dumb"), + sv("EDITOR", "hx"), + ], + // connection: authoritative TERM. + vec![sv("TERM", "xterm-256color")], + ); + + assert_eq!(env.get("LANG").map(String::as_str), Some("fr_FR.UTF-8")); // composition > inherited + assert_eq!(env.get("TZ").map(String::as_str), Some("Europe/Berlin")); // inherited-only survives + assert_eq!(env.get("TERM").map(String::as_str), Some("xterm-256color")); // connection > composition + assert_eq!(env.get("EDITOR").map(String::as_str), Some("hx")); // composition-only survives + assert_eq!(env.len(), 4); + } + #[test] fn open_and_get_fds() { let pty = Pty::open(DEFAULT_SIZE).expect("failed to open pty"); diff --git a/crates/sandbox2/src/lib.rs b/crates/sandbox2/src/lib.rs index e570d3d88..d865dfce2 100644 --- a/crates/sandbox2/src/lib.rs +++ b/crates/sandbox2/src/lib.rs @@ -379,6 +379,23 @@ impl Container { command.env("XDG_CONFIG_HOME", "/home/.config"); command.env("XDG_DATA_HOME", "/home/.local/share"); command.env("PATH", "/usr/bin:/bin:/usr/sbin:/sbin:/home/.local/bin"); // adds /home/.local/bin + // A styled default shell prompt for interactive sessions. Set as a + // plain default here (not forced) so a user's composition var can + // override it: the composed `env_vars` are applied further down and + // win on key collision. + command.env( + "PS1", + r"\[\033[01;32m\]\u@\h\[\033[00m\]:\[\033[01;34m\]\w\[\033[00m\]\$ ", + ); + // Login-shell identity, mirroring what sshd/pam would set from + // `/etc/passwd`. `USER`/`LOGNAME` track the configured username; + // `SHELL` points at the session shell (the `bash` package installs + // to `/usr/bin/bash`). All plain defaults, so composition vars win. + if let Some(user) = &sandbox.config.username { + command.env("USER", user); + command.env("LOGNAME", user); + } + command.env("SHELL", "/usr/bin/bash"); } else { // Both build and BoundWd layouts command.env("XDG_STATE_HOME", "/state/state"); @@ -413,8 +430,19 @@ impl Container { command.env("PYTHONHASHSEED", "0"); } - command.env("LANG", "en_US.utf8"); - command.env("LC_ALL", "en_US.utf8"); + // Locale. Sessions get a safe, always-present `C.UTF-8` floor: it's + // built into glibc so it never triggers "cannot set locale" warnings + // the way `en_US.utf8` does when that locale isn't generated in the + // rootfs, and setting only `LANG` (the lowest-precedence locale knob, + // no `LC_ALL`) lets a session's composed `env_vars` or a client's + // forwarded `LANG`/`LC_*` override it. Build/task sandboxes keep the + // fixed `en_US.utf8` + `LC_ALL` they always had, for output stability. + if let WdSetup::Session { .. } = &sandbox.config.wd { + command.env("LANG", "C.UTF-8"); + } else { + command.env("LANG", "en_US.utf8"); + command.env("LC_ALL", "en_US.utf8"); + } command.env("IS_SANDBOX", "1"); if let WdSetup::BoundDir { .. } = sandbox.config.wd { // Quality-of-life wiring for task sandboxes @@ -1313,9 +1341,9 @@ fn userns_restriction_from( euid_is_root: bool, apparmor_label: Option<&str>, ) -> Option { - if !max_user_namespaces + if max_user_namespaces .and_then(|s| s.trim().parse::().ok()) - .is_some_and(|n| n > 0) + .is_none_or(|n| n == 0) { return Some(UsernsRestriction::Disabled); } diff --git a/crates/sessions/example_project/minimal.toml b/crates/sessions/example_project/minimal.toml index a45a08c97..352aaf0ff 100644 --- a/crates/sessions/example_project/minimal.toml +++ b/crates/sessions/example_project/minimal.toml @@ -56,8 +56,10 @@ repo = "https://github.com/gominimal/pkgs" branch = "unstable" # Reused from the workspace's own `minimal.toml` — guaranteed to -# resolve as long as that pin does. -locked_commit = "75884ceb9d227e7c97642ccdfff01fecbb8380a1" +# resolve as long as that pin does. Keep this in sync with +# `.minimal/minimal.toml`; a drifted pin that upstream has since +# rebased away fails at checkout with "invalid reference". +locked_commit = "c854d6b1e0fdc67efd84fc664251079c0719987d" [stack] use = "shell" @@ -73,7 +75,7 @@ use = "shell" # the package's build attrs and routes them through the same # `PatchPolicy`/`VarsPolicy` gate every other contribution goes # through. -packages = ["mermaid-ascii", "go", "claude-code"] +packages = ["mermaid-ascii", "go"] [session.vars] # Hardcoded literal — auto-approves at the gate because diff --git a/crates/sessions/src/core/compose.rs b/crates/sessions/src/core/compose.rs index 7c4ab259a..61af38f57 100644 --- a/crates/sessions/src/core/compose.rs +++ b/crates/sessions/src/core/compose.rs @@ -24,7 +24,7 @@ use crate::core::source::{ use crate::wire::policy::{WirePatchVerdict, WireVarVerdict}; use crate::wire::primitives::{ PendingId, WirePendingPatch, WirePendingVar, WireProvenancedHook, WireSessionPatch, - WireSessionVar, WireVarSpec, + WireSessionVar, }; /// Errors produced while a [`Composable`] materializes its @@ -562,6 +562,19 @@ pub fn default_env() -> StoredEnv { Box::new(|name| std::env::var(name)) } +/// Env lookup for the *daemon-side* composer, which must never resolve +/// an inherited var against the daemon's own process environment — the +/// daemon's launch shell is not the user's shell. Every lookup reports +/// the name as present-but-empty: an `Inherit` var resolves without +/// erroring, its placeholder value is discarded, and +/// [`contribution_to_pending`] ships the preserved spec so the client +/// re-resolves against the *user's* env. (`Specified` vars never call +/// the lookup, so their literals are untouched.) +#[must_use] +pub fn deferring_env() -> StoredEnv { + Box::new(|_name| Ok(String::new())) +} + /// Anything that can contribute primitives (vars, patches, packages, /// lifecycle hooks) to a composer during session construction. /// @@ -933,15 +946,14 @@ impl PendingVar { wire: WirePendingVar, env: &dyn Fn(&str) -> Result, ) -> Result { - // Preserve the `carries_user_data` bit the daemon computed. - // The daemon always ships pending vars as `Specified` (with - // the already-resolved value), so `ResolvedVar::resolve_with` - // alone would always return `carries_user_data = false` and - // the policy gate would silently skip every daemon-derived - // var. We OR the daemon's bit on top: if the daemon resolved - // an Inherit against its own env, that value crossed a trust - // boundary and the client should still gate it. - let daemon_says_carries_user_data = wire.carries_user_data; + // Resolve the daemon-shipped spec against the *user's* env + // (`env` is the client's `std::env::var`). For an inherited + // var the daemon shipped `Inherit`/`InheritWithDefault` — never + // its own value — so this is the single, authoritative + // resolution, and `resolved.carries_user_data()` correctly + // reflects whether the value came from the user's environment. + // `Specified` specs (hardcoded project literals) resolve + // verbatim with `carries_user_data = false`, as before. let resolved = ResolvedVar::resolve_with(wire.name, wire.spec.into(), env).map_err( |err| match err { VarError::ResolutionFailure { name, source } => { @@ -957,16 +969,6 @@ impl PendingVar { }, }, )?; - // OR the two bits: user data flowed in if EITHER the daemon - // pulled from its env or the client's own `resolve_with` - // pulled from the client env. In the production path today - // the daemon always ships `Specified`, so the client's bit is - // always false and the daemon's is authoritative; the OR - // keeps `WirePendingVar` correct for direct-construction - // tests too. - let carries_user_data = daemon_says_carries_user_data || resolved.carries_user_data(); - let (name, value) = resolved.into_parts(); - let resolved = ResolvedVar::from_env_value_or_literal(name, value, carries_user_data); Ok(Self { id: wire.id, var: ProvenancedVar::new(resolved, wire.source.into()), @@ -1758,22 +1760,20 @@ pub(crate) fn contribution_to_pending( let mut wire_vars: Vec = Vec::with_capacity(vars.len()); for (i, pv) in vars.into_iter().enumerate() { let id = PendingId::new(u32::try_from(i).expect("pending var index fits in u32")); - // Items reach this transform already resolved (the composer's - // input is `ResolvedVar`); ship as a `Specified` spec so the - // client treats the value verbatim instead of re-resolving - // against its env. Carry the `carries_user_data` bit - // separately so the client's policy gate knows whether the - // resolved value pulled from an environment (daemon-side - // env, but still a host env — the client policy applies - // uniformly to any env-derived value) or was a hardcoded - // literal / fallback default. + // Ship the var's *original* spec, not the composer's resolved + // value: an inherited var (`Inherit`/`InheritWithDefault`) must + // be resolved by the client against the *user's* env, never the + // daemon's. Only `Specified` (a hardcoded literal) carries a + // real value here; the daemon composer resolves inherited vars + // against a deferring env (no host lookup), so its `value` for + // them is a discardable placeholder. `carries_user_data` is + // recomputed by the client after it resolves, so the bit shipped + // here is advisory only. let carries_user_data = pv.var().carries_user_data(); wire_vars.push(WirePendingVar { id, name: pv.var().name().to_string(), - spec: WireVarSpec::Specified { - value: pv.var().value().to_string(), - }, + spec: pv.var().spec().clone().into(), source: pv.source().clone().into(), carries_user_data, }); @@ -1864,6 +1864,80 @@ mod tests { ) } + /// The core of the daemon-resolves-inherited-vars fix: a project + /// `Inherit` var composed daemon-side must be shipped to the client + /// as an `Inherit` *spec*, never as a `Specified` carrying whatever + /// value the daemon's own environment held — and the client then + /// resolves it against the *user's* env. + #[test] + fn daemon_ships_inherit_spec_and_client_resolves_from_user_env() { + // Daemon-side resolution uses `deferring_env`, so no host lookup + // happens and the placeholder value is a discardable "". + let env = deferring_env(); + let daemon = ResolvedVar::resolve_with("LANG".into(), VarValue::Inherit, &env).unwrap(); + let transform = contribution_to_pending( + vec![ProvenancedVar::new(daemon, project_source())], + vec![], + vec![], + ); + let wire = &transform.wire.vars[0]; + // Shipped as the spec, not the daemon's baked-in value. + assert_eq!(wire.spec, crate::wire::primitives::WireVarSpec::Inherit); + + // The client resolves against the USER's env — not the daemon's. + let user_env = |name: &str| { + if name == "LANG" { + Ok("en_US.UTF-8".to_string()) + } else { + Err(std::env::VarError::NotPresent) + } + }; + let pending = PendingVar::from_wire(wire.clone(), &user_env).unwrap(); + assert_eq!(pending.provenanced().var().value(), "en_US.UTF-8"); + assert!(pending.provenanced().var().carries_user_data()); + } + + /// `InheritWithDefault` ships its default in the spec so the client + /// falls back correctly when the user's env is unset (and marks it + /// not-user-data), yet uses the user's value when present. + #[test] + fn daemon_ships_inherit_with_default_and_client_resolves_both_ways() { + let env = deferring_env(); + let daemon = + ResolvedVar::resolve_with("TZ".into(), VarValue::inherit_with_default("UTC"), &env) + .unwrap(); + let transform = contribution_to_pending( + vec![ProvenancedVar::new(daemon, project_source())], + vec![], + vec![], + ); + let wire = &transform.wire.vars[0]; + assert_eq!( + wire.spec, + crate::wire::primitives::WireVarSpec::InheritWithDefault { + default: "UTC".into() + } + ); + + // User env unset → default, not user data. + let miss = |_: &str| Err(std::env::VarError::NotPresent); + let pending = PendingVar::from_wire(wire.clone(), &miss).unwrap(); + assert_eq!(pending.provenanced().var().value(), "UTC"); + assert!(!pending.provenanced().var().carries_user_data()); + + // User env set → the user's value, marked as user data. + let hit = |name: &str| { + if name == "TZ" { + Ok("America/New_York".to_string()) + } else { + Err(std::env::VarError::NotPresent) + } + }; + let pending = PendingVar::from_wire(wire.clone(), &hit).unwrap(); + assert_eq!(pending.provenanced().var().value(), "America/New_York"); + assert!(pending.provenanced().var().carries_user_data()); + } + type VarsPolicyMutator = Box; struct ScriptedHook { diff --git a/crates/sessions/src/core/primitives.rs b/crates/sessions/src/core/primitives.rs index f9bb1385b..e0af137ba 100644 --- a/crates/sessions/src/core/primitives.rs +++ b/crates/sessions/src/core/primitives.rs @@ -554,6 +554,15 @@ pub struct ResolvedVar { /// - [`VarValue::InheritWithDefault`] with env-hit → `true` /// - [`VarValue::InheritWithDefault`] falling back to default → `false` carries_user_data: bool, + /// The original, pre-resolution spec. Preserved so a daemon-side + /// composer can ship an inherited var to the client *as an + /// `Inherit`/`InheritWithDefault` spec* (see + /// `contribution_to_pending`) rather than baking in its own + /// resolved value — the client must always resolve inherited vars + /// against the *user's* env. Terminal constructors (already- + /// resolved values that never get re-shipped) record it as a + /// [`VarValue::Specified`] of the resolved value. + spec: VarValue, } impl ResolvedVar { @@ -578,6 +587,14 @@ impl ResolvedVar { self.carries_user_data } + /// The original, pre-resolution spec. `contribution_to_pending` + /// uses this to hand an inherited var back to the client for + /// user-env resolution instead of shipping the daemon's value. + #[must_use] + pub fn spec(&self) -> &VarValue { + &self.spec + } + /// Construct a [`ResolvedVar`] whose value came directly from a /// host env lookup, so `carries_user_data` is true. Used by /// callers that already ran their own env lookup and need to @@ -588,6 +605,7 @@ impl ResolvedVar { pub fn from_env_value(name: String, value: String) -> Self { Self { name, + spec: VarValue::specified(value.clone()), value, carries_user_data: true, } @@ -600,6 +618,7 @@ impl ResolvedVar { pub fn from_literal(name: String, value: String) -> Self { Self { name, + spec: VarValue::specified(value.clone()), value, carries_user_data: false, } @@ -614,6 +633,7 @@ impl ResolvedVar { pub fn from_env_value_or_literal(name: String, value: String, carries_user_data: bool) -> Self { Self { name, + spec: VarValue::specified(value.clone()), value, carries_user_data, } @@ -650,6 +670,10 @@ impl ResolvedVar { where F: FnOnce(&str) -> Result, { + // Retain the pre-resolution spec so a daemon-side composer can + // forward it verbatim for the client to resolve against the + // user's env (see `contribution_to_pending`). + let spec = value.clone(); let (resolved_value, carries_user_data) = match value { VarValue::Specified { value } => (value, false), VarValue::Inherit => { @@ -674,6 +698,7 @@ impl ResolvedVar { name, value: resolved_value, carries_user_data, + spec, }) } @@ -708,6 +733,7 @@ impl From for ResolvedVar { fn from(v: crate::wire::primitives::WireResolvedVar) -> Self { Self { name: v.name, + spec: VarValue::specified(v.value.clone()), value: v.value, carries_user_data: v.carries_user_data, } @@ -726,6 +752,16 @@ impl From for VarValue { } } +impl From for crate::wire::primitives::WireVarSpec { + fn from(spec: VarValue) -> Self { + match spec { + VarValue::Specified { value } => Self::Specified { value }, + VarValue::Inherit => Self::Inherit, + VarValue::InheritWithDefault { default } => Self::InheritWithDefault { default }, + } + } +} + // ===================================================================== // FileSet // ===================================================================== diff --git a/crates/sessions/src/daemon/composer.rs b/crates/sessions/src/daemon/composer.rs index 7d68c0b35..430a38729 100644 --- a/crates/sessions/src/daemon/composer.rs +++ b/crates/sessions/src/daemon/composer.rs @@ -7,7 +7,7 @@ use std::collections::BTreeMap; use crate::SessionId; use crate::core::compose::{ Composable, ComposeError, ComposeOptions, Composition, Contribution, Error, SessionPatch, - SessionVar, StoredEnv, contribution_to_pending, default_env, + SessionVar, StoredEnv, contribution_to_pending, deferring_env, }; use crate::core::primitives::ResolvedPatch; use crate::core::source::{ @@ -80,14 +80,15 @@ const _: fn() = || { impl SessionComposer { /// Construct a composer seeded with the client's wire - /// contribution and the default env lookup - /// ([`std::env::var`]). + /// contribution and the daemon-side [`deferring_env`] lookup — + /// inherited vars are never resolved against the daemon's own + /// environment; the client resolves them against the user's env. #[must_use] pub fn new(client: WireContribution) -> Self { Self { client, contribution: Contribution::new(), - env: default_env(), + env: deferring_env(), } } From 7ad863b43fde86130effe0e831bd8dd9537c6ee2 Mon Sep 17 00:00:00 2001 From: Evan Spearman Date: Wed, 29 Jul 2026 02:37:38 +0000 Subject: [PATCH 2/2] fix: fixed failing tests in diagnostics module --- crates/common/src/lib.rs | 2 +- crates/diagnostics/src/net.rs | 43 +++++++++++++++++++++++++++++++---- crates/minimald/src/diag.rs | 17 +++++++++++--- 3 files changed, 54 insertions(+), 8 deletions(-) diff --git a/crates/common/src/lib.rs b/crates/common/src/lib.rs index 736577eb0..4f49e22b7 100644 --- a/crates/common/src/lib.rs +++ b/crates/common/src/lib.rs @@ -221,7 +221,7 @@ pub fn hardlink_dir_contents(src: &Path, dst: &Path) -> Result<(), HardlinkError // `/home`), where hardlinks are impossible (`EXDEV`). Fall back // to a copy so materialization still succeeds — slower and no // longer deduplicated, but correct. - Err(e) if e.raw_os_error() == Some(libc::EXDEV) => { + Err(e) if e.kind() == std::io::ErrorKind::CrossesDevices => { // Every file in a cross-device tree hits EXDEV, so warn only // on the first — a per-file log would flood with thousands // of identical lines. Once-per-process is enough: the cause diff --git a/crates/diagnostics/src/net.rs b/crates/diagnostics/src/net.rs index 9deb18748..66a233aae 100644 --- a/crates/diagnostics/src/net.rs +++ b/crates/diagnostics/src/net.rs @@ -62,6 +62,18 @@ pub async fn listening_sockets( /// The `/proc/net` socket tables, in the order a reader wants them. pub const PROC_NET_SOCKET_TABLES: &[&str] = &["tcp", "tcp6", "udp", "udp6", "unix"]; +/// The `/proc/net` table that stands in for `ip addr`/`ifconfig` when a host +/// ships no net tools: the interface list (`/proc/net/dev`). It carries names +/// and stats but no addresses or MACs — the picture a process has with nothing +/// installed, and the same table the in-microVM guest collector captures. +pub const PROC_NET_INTERFACE_TABLES: &[&str] = &["dev"]; + +/// The `/proc/net` tables that stand in for `ip route`/`netstat -rn` on a +/// stripped host: the IPv4 routing table plus the fib trie, which also carries +/// the local address picture `ip addr` gives where tools exist. Hex-encoded; +/// the dev team decodes. +pub const PROC_NET_ROUTE_TABLES: &[&str] = &["route", "fib_trie"]; + /// `/net/.txt`: the named raw `/proc/net` tables, concatenated /// verbatim behind a `=== /proc/net/ ===` banner each. /// @@ -99,8 +111,20 @@ pub async fn interfaces( } else { &[("ifconfig", &["-a"])] }; - let (banner, out) = first_available(attempts).await?; - let text = format!("{banner}\n{}", mask_macs(&out)); + let text = match first_available(attempts).await { + Ok((banner, out)) => format!("{banner}\n{}", mask_macs(&out)), + // A stripped host (no `ip`/`ifconfig`) still gets the interface picture + // `/proc/net` carries, mirroring `listening_sockets`. No MACs live in + // `/proc/net/dev`, so the mask pass is a no-op — kept to honor the + // `Redaction::Keys` label uniformly across both paths. + #[cfg(target_os = "linux")] + Err(cmd_err) => format!( + "(commands unavailable: {cmd_err})\n{}", + mask_macs(&proc_net_text(PROC_NET_INTERFACE_TABLES).await) + ), + #[cfg(not(target_os = "linux"))] + Err(cmd_err) => return Err(cmd_err.into()), + }; w.add_bytes( &format!("{dest}/net/interfaces.txt"), text.as_bytes(), @@ -121,10 +145,21 @@ pub async fn routes( } else { &[("netstat", &["-rn"])] }; - let (banner, out) = first_available(attempts).await?; + let text = match first_available(attempts).await { + Ok((banner, out)) => format!("{banner}\n{out}"), + // On a stripped host fall back to the raw routing tables, mirroring + // `listening_sockets`; `fib_trie` also carries the local address picture. + #[cfg(target_os = "linux")] + Err(cmd_err) => format!( + "(commands unavailable: {cmd_err})\n{}", + proc_net_text(PROC_NET_ROUTE_TABLES).await + ), + #[cfg(not(target_os = "linux"))] + Err(cmd_err) => return Err(cmd_err.into()), + }; w.add_bytes( &format!("{dest}/net/routes.txt"), - format!("{banner}\n{out}").as_bytes(), + text.as_bytes(), Redaction::None, ) .await diff --git a/crates/minimald/src/diag.rs b/crates/minimald/src/diag.rs index 29ab7d9f1..e535f7910 100644 --- a/crates/minimald/src/diag.rs +++ b/crates/minimald/src/diag.rs @@ -322,14 +322,25 @@ async fn build_bundle( collect_step!( w, "net.interfaces", - diagnostics::net::proc_net_tables(&mut w, "", "interfaces", &["dev"]) + diagnostics::net::proc_net_tables( + &mut w, + "", + "interfaces", + diagnostics::net::PROC_NET_INTERFACE_TABLES + ) ); // Routing *and* addresses: `fib_trie` carries the local address picture the - // host's `ip addr` gives on the other side of the switch. + // host's `ip addr` gives on the other side of the switch. Same tables the + // host collector falls back to, so the two captures stay comparable. collect_step!( w, "net.routes", - diagnostics::net::proc_net_tables(&mut w, "", "routes", &["route", "fib_trie"]) + diagnostics::net::proc_net_tables( + &mut w, + "", + "routes", + diagnostics::net::PROC_NET_ROUTE_TABLES + ) ); if s.in_microvm().await { collect_step!(w, "net.gvproxy", gvproxy_probe(&mut w));