diff --git a/src/agent-memory/config/systemd/anolisa-memory@.service b/src/agent-memory/config/systemd/anolisa-memory@.service index c752350617..5ad075ee7d 100644 --- a/src/agent-memory/config/systemd/anolisa-memory@.service +++ b/src/agent-memory/config/systemd/anolisa-memory@.service @@ -13,6 +13,13 @@ Environment=MEMORY_MOUNT_STRATEGY=auto Environment=USER_ID=%i RuntimeDirectory=anolisa/sessions/%i RuntimeDirectoryMode=0700 +# Point the server at the runtime directory the two lines above create. +# Without this it used the compiled-in default /run/anolisa/sessions, which +# the RPM's tmpfiles.d snippet creates 0700 root:root and this *user* unit +# can therefore neither traverse nor write — so the RuntimeDirectory= was +# dead config and the session log (mem_promote / mem_session_log) was lost. +# %t is $XDG_RUNTIME_DIR for a user unit. +Environment=MEMORY_SESSION_DIR=%t/anolisa/sessions/%i # user namespace + mount namespace are both unprivileged, so no # AmbientCapabilities= are required. ProtectSystem/Home are off because the @@ -45,12 +52,18 @@ RestrictNamespaces=user mnt # to our own scope. # Read-only root filesystem with explicit write paths for the memory -# store (~/.anolisa), session scratch (/run/anolisa), and cgroupfs -# (the Delegate=memory subtree below requires cgroup.subtree_control -# and memory.max writes; ReadOnlyPaths=/ would otherwise mount -# /sys/fs/cgroup read-only inside our namespace and EROFS those writes). +# store (~/.anolisa), session scratch (the per-user runtime dir created by +# RuntimeDirectory= above, plus the legacy /run/anolisa for operators who +# point MEMORY_SESSION_DIR back at it), and cgroupfs (the Delegate=memory +# subtree below requires cgroup.subtree_control and memory.max writes; +# ReadOnlyPaths=/ would otherwise mount /sys/fs/cgroup read-only inside our +# namespace and EROFS those writes). +# +# %t/anolisa has to be listed explicitly: ReadOnlyPaths=/ makes the whole +# tree read-only inside the unit's namespace, so a RuntimeDirectory= that +# systemd creates outside it is still unwritable from inside. ReadOnlyPaths=/ -ReadWritePaths=~/.anolisa /run/anolisa /sys/fs/cgroup +ReadWritePaths=~/.anolisa /run/anolisa %t/anolisa /sys/fs/cgroup # P6.4: delegate cgroup controllers so [memory.cgroup].enabled=true can # create a child cgroup under our scope and apply memory.max. Without diff --git a/src/agent-memory/src/host.rs b/src/agent-memory/src/host.rs new file mode 100644 index 0000000000..33e965ed77 --- /dev/null +++ b/src/agent-memory/src/host.rs @@ -0,0 +1,105 @@ +//! Host identity, captured before the process switches namespaces. +//! +//! `main` enters the user namespace before it constructs anything else, and +//! the default unprivileged mapping is `0 1`. From that point on +//! `geteuid()` reports 0 for *every* user on the box, so anything that has +//! to tell one host user from another — a per-user directory name, say — +//! must use the uid recorded here rather than the one the kernel reports +//! now. +//! +//! The converse matters just as much: anything that has to match filesystem +//! metadata (`st_uid`) must keep using the *current* uid, because that is +//! the namespace the metadata is reported in. Inside our own user +//! namespace a directory we own on `/tmp` shows up as uid 0, and one a +//! neighbour owns shows up as the overflow uid — comparing either of those +//! against the host uid would be wrong. + +use std::sync::OnceLock; + +/// The uid this process was launched with. Set once, before any `unshare`. +static HOST_UID: OnceLock = OnceLock::new(); + +/// Record the calling uid while it is still the host uid. +/// +/// Idempotent — the first call wins — so it is safe (and intended) to call +/// it both from `main`, before `early_enter_userns`, and from +/// `LinuxUserNsMount::enter`, before the `unshare` itself. +pub fn capture_host_uid() -> u32 { + *HOST_UID.get_or_init(|| nix::unistd::Uid::current().as_raw()) +} + +/// The uid this process was launched with, even after `unshare(CLONE_NEWUSER)`. +/// +/// When nothing was captured — a library consumer that built `MemoryService` +/// without going through `main` — the answer is recovered from +/// `/proc/self/uid_map` instead, so a namespaced process still gets a +/// per-host-user value rather than the 0 every one of its neighbours shares. +pub fn host_uid() -> u32 { + if let Some(uid) = HOST_UID.get() { + return *uid; + } + match std::fs::read_to_string("/proc/self/uid_map") { + Ok(map) => host_uid_from_uid_map(nix::unistd::Uid::current().as_raw(), &map) + .unwrap_or_else(capture_host_uid), + Err(_) => capture_host_uid(), + } +} + +/// Map `current`, a uid in this process's user namespace, back to the host +/// uid using the contents of `/proc/self/uid_map`. +/// +/// Each line is ` `. Returns `None` when `current` +/// is not covered by any mapping (an unmapped uid, reported by the kernel as +/// the overflow id), in which case there is no host uid to recover. +fn host_uid_from_uid_map(current: u32, uid_map: &str) -> Option { + uid_map.lines().find_map(|line| { + let mut fields = line.split_whitespace(); + let inside = fields.next()?.parse::().ok()?; + let outside = fields.next()?.parse::().ok()?; + let count = fields.next()?.parse::().ok()?; + let offset = current.checked_sub(inside)?; + (offset < count).then_some(outside + offset) + }) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn recovers_the_host_uid_from_a_single_id_mapping() { + // `LinuxUserNsMount::enter` writes exactly this: inside-0 is the + // launching user, and every other host uid is unmapped. + let map = " 0 1000 1\n"; + assert_eq!(host_uid_from_uid_map(0, map), Some(1000)); + assert_eq!( + host_uid_from_uid_map(1, map), + None, + "an unmapped uid has no host uid" + ); + } + + #[test] + fn is_the_identity_outside_a_user_namespace() { + let map = " 0 0 4294967295\n"; + assert_eq!(host_uid_from_uid_map(1000, map), Some(1000)); + assert_eq!(host_uid_from_uid_map(0, map), Some(0)); + } + + #[test] + fn handles_a_multi_range_map_and_ignores_junk() { + let map = "garbage\n0 1000 10\n10 2000 5\n"; + assert_eq!(host_uid_from_uid_map(0, map), Some(1000)); + assert_eq!(host_uid_from_uid_map(9, map), Some(1009)); + assert_eq!(host_uid_from_uid_map(12, map), Some(2002)); + assert_eq!(host_uid_from_uid_map(15, map), None); + } + + #[test] + fn host_uid_never_reports_the_namespace_uid_after_capture() { + // Whatever namespace we happen to be in, the captured value is the + // one callers get; this is the property the tmp-dir suffix relies on. + let captured = capture_host_uid(); + assert_eq!(host_uid(), captured); + } +} diff --git a/src/agent-memory/src/lib.rs b/src/agent-memory/src/lib.rs index b9379d1a9c..c423cde216 100644 --- a/src/agent-memory/src/lib.rs +++ b/src/agent-memory/src/lib.rs @@ -24,6 +24,7 @@ pub mod consolidation; pub mod embedding; pub mod error; pub mod git_repo; +pub mod host; pub mod index; pub mod mcp_server; pub mod mount; diff --git a/src/agent-memory/src/main.rs b/src/agent-memory/src/main.rs index 41556bf254..13438f4b4b 100644 --- a/src/agent-memory/src/main.rs +++ b/src/agent-memory/src/main.rs @@ -39,6 +39,12 @@ enum Commands { fn main() -> Result<()> { let cli = Cli::parse(); + // Before anything below can change the answer: `early_enter_userns` + // maps our uid to 0, and the per-user session fallback has to be named + // after the *host* uid or every user on the box collides on + // `/tmp/anolisa-sessions-0`. + agent_memory::host::capture_host_uid(); + tracing_subscriber::fmt() .with_env_filter(EnvFilter::from_default_env()) .with_writer(std::io::stderr) @@ -130,7 +136,7 @@ async fn run_mcp_server(config: AppConfig) -> Result<()> { let svc = Arc::new(MemoryService::new(config)?); tracing::info!("mount: {}", svc.mount.root.display()); if let Some(s) = &svc.session { - tracing::info!("session: {} ({})", s.sid(), s.root().display()); + tracing::info!("session: {} ({})", s.sid(), s.display_root().display()); } let server = MemoryMcpServer::new(Arc::clone(&svc)); diff --git a/src/agent-memory/src/mount/linux_userns.rs b/src/agent-memory/src/mount/linux_userns.rs index 5bec4f461a..898688d165 100644 --- a/src/agent-memory/src/mount/linux_userns.rs +++ b/src/agent-memory/src/mount/linux_userns.rs @@ -74,6 +74,13 @@ impl LinuxUserNsMount { // running under userland (which would silently produce wrong // ownership / permissions on every home-dir syscall). if !UNSHARED.load(Ordering::Acquire) { + // Record the launching uid *before* the unshare below makes + // `geteuid()` report the mapped one, so the per-user session + // fallback name can still tell host users apart. `main` already + // did this; it is repeated here because `enter` is also reached + // from `MemoryService::new` in library consumers. + crate::host::capture_host_uid(); + let real_uid = geteuid().as_raw(); let real_gid = getegid().as_raw(); diff --git a/src/agent-memory/src/service/mod.rs b/src/agent-memory/src/service/mod.rs index a6379f7564..885b814a47 100644 --- a/src/agent-memory/src/service/mod.rs +++ b/src/agent-memory/src/service/mod.rs @@ -1,15 +1,21 @@ -use std::path::PathBuf; +use std::os::fd::{FromRawFd, OwnedFd, RawFd}; +use std::path::{Path, PathBuf}; use std::sync::Arc; use std::sync::atomic::{AtomicUsize, Ordering}; +use nix::errno::Errno; +use nix::fcntl::{AtFlags, OFlag}; +use nix::sys::stat::{Mode, SFlag}; +use nix::unistd::UnlinkatFlags; + use crate::audit::AuditLogger; use crate::config::AppConfig; use crate::consolidation::{OwnedAuditEntry, run_consolidation_owned}; -use crate::error::Result; +use crate::error::{MemoryError, Result}; use crate::index::{IndexHandle, SearchHit}; use crate::mount::pick_strategy; use crate::ns::{MountPoint, Namespace}; -use crate::session::{EndAction, SessionId, SessionLogService}; +use crate::session::{EndAction, SessionBase, SessionId, SessionLogService}; use crate::tools::{GrepHit, GrepOptions, ListEntry, ListOptions}; /// MemoryService is the top-level entry point used by both the MCP server and @@ -45,6 +51,13 @@ impl MemoryService { /// directory is writable. Failure to start the session is logged and /// degrades gracefully (mem_promote / mem_session_log will return errors). pub fn new(config: AppConfig) -> Result { + // `pick_strategy` below may unshare into a user namespace, after + // which `geteuid()` reports the mapped uid (0 for the default + // `0 1` mapping) instead of the one we were launched + // with. Record it now so the per-user session fallback name stays + // per *host* user; `main` does the same, earlier, for the binary. + crate::host::capture_host_uid(); + let base = config.resolved_base_dir(); std::fs::create_dir_all(&base)?; @@ -413,9 +426,331 @@ impl MemoryService { } } +/// Monotonic suffix for the writability probe file so two servers probing +/// the same candidate concurrently never collide on the name. Each probe +/// reserves a whole `PROBE_ATTEMPTS` block, so a retry cannot step onto a +/// name another prober was just handed either. +static PROBE_SEQ: AtomicUsize = AtomicUsize::new(0); + +/// Session base directories to try, in preference order. +/// +/// The second element of each pair says whether the candidate is one *we* +/// picked (and therefore has to be hardened against a local user planting +/// the name first) or one the operator configured (their own choice, so it +/// is only required to be a usable directory). +/// +/// The configured directory — `/run/anolisa/sessions` by default, and the +/// value the OpenClaw plugin forwards as `MEMORY_SESSION_DIR` on every +/// spawn — is only writable by root, yet the server always runs +/// unprivileged: +/// +/// - the RPM ships `config/systemd/anolisa-memory-tmpfiles.conf`, which +/// creates `/run/anolisa` and `/run/anolisa/sessions` `0700 root:root`. +/// A non-root server can neither traverse the parent (no `x` for others) +/// nor create `` inside it. +/// - `make install`, containers and dev boxes ship no tmpfiles snippet at +/// all, and `/run` itself is `drwxr-xr-x root root`, so even the first +/// `create_dir_all` component fails with EACCES. +/// - the shipped unit is a *user* template (`anolisa-memory@.service`), and +/// MCP clients such as the OpenClaw plugin spawn `agent-memory serve` +/// directly as the logged-in user. +/// +/// Before the fallback existed, every one of those cases made +/// `start_session` fail and `MemoryService::new` degrade to `session = None` +/// behind a single `warn!` — so on a stock install *every* `mem_promote` +/// returned `SessionUnavailable`, every `mem_session_log` returned +/// `NotImplemented`, and the session-scoped consolidation path had no log +/// to read. +/// +/// Both fallbacks are per-user by construction: `$XDG_RUNTIME_DIR` is +/// already `0700` and owned by the user, and the tmp candidate is suffixed +/// with the *host* uid so two users on one host never share a base. +fn session_base_candidates(configured: &Path) -> Vec<(PathBuf, bool)> { + candidates_for_host_uid(configured, crate::host::host_uid()) +} + +/// [`session_base_candidates`] with the host uid supplied explicitly, so the +/// naming rule can be tested without entering a user namespace. +/// +/// `host_uid` has to be the uid the process was *launched* with, not the one +/// the kernel reports now. `main::early_enter_userns` runs before the +/// service is constructed and `LinuxUserNsMount::enter` installs the mapping +/// `0 1`, so `geteuid()` is 0 for every user on the box by the +/// time this is reached. Suffixing the tmp candidate with that would make +/// all of them compute `/tmp/anolisa-sessions-0` whenever the configured base +/// is unusable, `XDG_RUNTIME_DIR` is absent and `TMPDIR` is unset or shared: +/// whoever creates it first ends up owning a directory every other user's +/// namespace reports as foreign-owned, so the ownership check rejects the +/// last candidate and they lose the session anyway. +fn candidates_for_host_uid(configured: &Path, host_uid: u32) -> Vec<(PathBuf, bool)> { + let mut out: Vec<(PathBuf, bool)> = vec![(configured.to_path_buf(), false)]; + + if let Ok(xdg) = std::env::var("XDG_RUNTIME_DIR") { + let xdg = xdg.trim(); + if !xdg.is_empty() { + out.push((Path::new(xdg).join("anolisa").join("sessions"), true)); + } + } + + let tmp = std::env::var("TMPDIR") + .ok() + .map(|v| v.trim().to_string()) + .filter(|v| !v.is_empty()) + .map(PathBuf::from) + .unwrap_or_else(|| PathBuf::from("/tmp")); + out.push((tmp.join(format!("anolisa-sessions-{host_uid}")), true)); + + out +} + +/// First candidate that can be created *and* written to, else an error +/// naming every directory that was tried. +/// +/// The winner comes back as a [`SessionBase`] — an open descriptor plus the +/// path derived from it — so that validation and use are bound to the same +/// directory rather than to a pathname that can be swapped in between. +fn pick_session_base(candidates: &[(PathBuf, bool)]) -> Result { + let mut rejected: Vec = Vec::with_capacity(candidates.len()); + for (dir, ours) in candidates { + match probe_session_base(dir, *ours) { + Ok(base) => return Ok(base), + Err(e) => rejected.push(format!("{}: {e}", dir.display())), + } + } + Err(MemoryError::Other(format!( + "no writable session directory; tried {}", + rejected.join(", ") + ))) +} + +/// Resolve the session scratch base, falling back to a per-user runtime or +/// tmp directory when the configured one is not usable. A fallback is +/// reported with `warn!` so the operator can see that the configured +/// location was ignored and why. +fn resolve_session_base(config: &AppConfig) -> Result { + let configured = config.resolved_session_dir(); + let candidates = session_base_candidates(&configured); + let base = pick_session_base(&candidates)?; + if base.display_path() != configured.as_path() { + tracing::warn!( + "session dir {} is not usable by uid {}; using {} instead \ + (set MEMORY_SESSION_DIR to override)", + configured.display(), + crate::host::host_uid(), + base.display_path().display() + ); + } + Ok(base) +} + +/// How many distinct probe names one `probe_writable` call may burn before +/// it gives up on the candidate. Only a stale probe left by a killed server +/// can occupy a name (a symlink ends the attempt immediately), so a handful +/// is plenty; the whole block is reserved out of `PROBE_SEQ` at once so two +/// concurrent probers can never be handed the same name. +const PROBE_ATTEMPTS: usize = 8; + +/// Whether a session base *we* chose may be used given the uid that owns it. +/// +/// There is deliberately no root exemption. `/tmp/anolisa-sessions-0` is a +/// predictable name in a world-writable directory, so "the owner is not me +/// but I am root, so it is fine" lets any local user pre-create the base and +/// then own every session root a root server builds inside it — scratch +/// files, `log.jsonl` and the `mem_promote` source tree — including swapping +/// a known `MEMORY_SESSION_ID` entry for a symlink that the server's own +/// `create_dir_all` / chmod / metadata writes then follow. +/// +/// Both arguments are uids in the namespace the *filesystem metadata* is +/// reported in, i.e. the current one — inside our own user namespace a base +/// we own on `/tmp` shows `st_uid == 0` and one a neighbour owns shows the +/// overflow uid. Comparing against the host uid (see +/// [`candidates_for_host_uid`], which needs it for the *name*) would reject +/// every fallback we own. +fn fallback_owner_is_us(owner: u32, me: u32) -> bool { + owner == me +} + +/// Prove `base` is writable by creating and removing a scratch file in it. +/// +/// `seq_base` is the first of `PROBE_ATTEMPTS` reserved probe names. +/// +/// The name is predictable, so on a base another local user can write to — +/// which includes an operator-configured one, since a group- or +/// world-writable `MEMORY_SESSION_DIR` is explicitly allowed — it can be +/// pre-planted. `std::fs::write` follows symlinks and truncates the target, +/// so probing with it turned the writability check itself into a +/// file-clobber primitive against anything the server uid can write. +/// `O_CREAT|O_EXCL` refuses to open, let alone follow, whatever already +/// occupies the name, and `openat`/`unlinkat` against the base descriptor +/// look the name up in the directory that was actually validated rather +/// than in whatever the pathname resolves to now. +fn probe_writable(base: &SessionBase, seq_base: usize) -> Result<()> { + let pid = std::process::id(); + let flags = + OFlag::O_WRONLY | OFlag::O_CREAT | OFlag::O_EXCL | OFlag::O_NOFOLLOW | OFlag::O_CLOEXEC; + for offset in 0..PROBE_ATTEMPTS { + let name = format!(".anolisa-probe-{pid}-{}", seq_base + offset); + match nix::fcntl::openat( + Some(base.fd()), + name.as_str(), + flags, + Mode::from_bits_truncate(0o600), + ) { + Ok(raw) => { + // Close before unlinking so the name is released even on + // filesystems that defer deletion of open files. + // SAFETY: `openat` just handed us this descriptor. + drop(unsafe { OwnedFd::from_raw_fd(raw) }); + let _ = nix::unistd::unlinkat( + Some(base.fd()), + name.as_str(), + UnlinkatFlags::NoRemoveDir, + ); + return Ok(()); + } + Err(Errno::EEXIST) => { + // A symlink means somebody is actively squatting on this + // base, so refuse the candidate rather than unlink their + // file. A regular file is a stale probe from a killed + // server: not ours to delete, so take the next name. + if is_planted_symlink(base.fd(), name.as_str()) { + return Err(MemoryError::Other(format!( + "{} holds a symlink at {name}; refusing it as a session dir", + base.display_path().display() + ))); + } + } + Err(e) => { + return Err(MemoryError::Other(format!( + "cannot probe {} ({name}): {e}", + base.display_path().display() + ))); + } + } + } + + Err(MemoryError::Other(format!( + "no free .anolisa-probe-* name under {} after {PROBE_ATTEMPTS} attempts", + base.display_path().display() + ))) +} + +/// Whether `name` inside `dirfd` is a symlink. `AT_SYMLINK_NOFOLLOW`, so +/// this reports the entry itself and not whatever it points at. +fn is_planted_symlink(dirfd: RawFd, name: &str) -> bool { + nix::sys::stat::fstatat(Some(dirfd), name, AtFlags::AT_SYMLINK_NOFOLLOW) + .is_ok_and(|st| SFlag::from_bits_truncate(st.st_mode).contains(SFlag::S_IFLNK)) +} + +/// The one and only pathname resolution for a candidate: open it, and +/// validate the descriptor we get. `ours` decides whether a symlink in the +/// final component is an attack or the operator's own choice. +/// +/// The base is opened read-only because a directory cannot be opened for +/// writing; `O_DIRECTORY` is what turns "a regular file sits where the +/// session base should be" into `ENOTDIR` instead of a later surprise. +fn open_candidate(dir: &Path, ours: bool) -> Result { + if ours { + SessionBase::open_nofollow(dir) + } else { + SessionBase::open(dir) + } +} + +/// Create `dir` if needed, prove it is a writable directory, and hand back +/// the descriptor the decision was made on. +/// +/// Existence alone is not enough: a pre-existing read-only directory +/// passes `create_dir_all` and then fails on the first session, so the +/// probe does a real create-and-remove. +/// +/// For directories *we* chose (`ours`), additionally refuse a symlink, an +/// owner other than our effective uid (root included), and a base that +/// cannot be tightened to `0700`. Without that, any local user could plant +/// `/tmp/anolisa-sessions-` as a symlink — or, against a root +/// server, own the directory outright — and redirect both the probe and +/// every session root into a directory of their choosing. The +/// operator-configured directory is exempt from those three: a symlink, a +/// shared owner or a group-writable mount there is the operator's own +/// decision. It still gets the `O_EXCL` probe, because "the operator chose +/// a world-writable location" is not "the operator chose to let neighbours +/// clobber our files". +/// +/// Every check below is an `fstat`/`fchmod` on a descriptor from a single +/// `open`, and that is load-bearing. The previous shape looked at the +/// pathname twice — `symlink_metadata`, then `metadata` — and each lookup +/// resolved the name from scratch, so a local user could let the first one +/// see an ordinary directory they had pre-created under `/tmp`, rename it +/// away, and leave a symlink to a directory the server already owns. The +/// second lookup followed it, both the ownership and the mode check passed +/// against the wrong inode, and `SessionLogService` went on to build the +/// session under the redirect target. Returning the descriptor is what makes +/// the decision stick; another pathname check would only move the window. +fn probe_session_base(dir: &Path, ours: bool) -> Result { + let base = match open_candidate(dir, ours) { + Ok(base) => base, + Err(MemoryError::Io(e)) if e.kind() == std::io::ErrorKind::NotFound => { + std::fs::create_dir_all(dir)?; + // Re-open rather than re-check: the name may have been swapped + // while we were creating it, and `O_NOFOLLOW` on this second + // open is what catches that. + open_candidate(dir, ours)? + } + Err(e) => return Err(e), + }; + + let mut st = base.stat()?; + if !SFlag::from_bits_truncate(st.st_mode).contains(SFlag::S_IFDIR) { + return Err(MemoryError::Other(format!( + "{} is not a directory", + dir.display() + ))); + } + + if ours { + let me = nix::unistd::Uid::current().as_raw(); + if !fallback_owner_is_us(st.st_uid, me) { + return Err(MemoryError::Other(format!( + "{} is owned by uid {}, not {me}; refusing it as a session dir", + dir.display(), + st.st_uid + ))); + } + + // Fallbacks can land under a world-writable /tmp, and a base created + // before this hardening keeps whatever the umask gave it. Each + // `` is already forced to 0700 by `SessionLogService::start`, + // but the base should not be listable or writable by other users + // either. Both failures reject the candidate instead of being + // swallowed: a chmod that errors, or that a filesystem accepts + // without applying (some FUSE and network mounts do), would + // otherwise leave session data in a base we just promised was 0700. + // Going through `fchmod` on the descriptor means the tightening + // cannot be redirected at a different inode either. + if st.st_mode & 0o077 != 0 { + nix::sys::stat::fchmod(base.fd(), Mode::from_bits_truncate(0o700)).map_err(|e| { + MemoryError::Other(format!("cannot tighten {} to 0700: {e}", dir.display())) + })?; + st = base.stat()?; + if st.st_mode & 0o077 != 0 { + return Err(MemoryError::Other(format!( + "{} is still mode {:o} after chmod; refusing it as a session dir", + dir.display(), + st.st_mode & 0o777 + ))); + } + } + } + + probe_writable( + &base, + PROBE_SEQ.fetch_add(PROBE_ATTEMPTS, Ordering::Relaxed), + )?; + Ok(base) +} + fn start_session(config: &AppConfig, ns: &Namespace) -> Result { - let base = config.resolved_session_dir(); - std::fs::create_dir_all(&base)?; + let base = resolve_session_base(config)?; let sid = match std::env::var("MEMORY_SESSION_ID") { Ok(s) if !s.is_empty() => match SessionId::from_string(&s) { Ok(sid) => sid, @@ -432,8 +767,8 @@ fn start_session(config: &AppConfig, ns: &Namespace) -> Result Option

u32 { + std::fs::metadata(p).unwrap().permissions().mode() & 0o777 + } + + #[test] + fn prefers_a_writable_configured_dir() { + let tmp = tempfile::tempdir().unwrap(); + let configured = tmp.path().join("sessions"); + let fallback = tmp.path().join("fallback"); + let picked = + pick_session_base(&[(configured.clone(), false), (fallback.clone(), true)]).unwrap(); + assert_eq!(picked.display_path(), configured); + assert!( + !fallback.exists(), + "must not touch the fallback unnecessarily" + ); + } + + #[test] + fn skips_a_configured_dir_that_cannot_be_created() { + // A regular file where the session base should be: create_dir_all can + // never succeed. This is the shape a non-root server hits on the + // shipped default /run/anolisa/sessions. + let tmp = tempfile::tempdir().unwrap(); + let blocker = tmp.path().join("blocker"); + std::fs::write(&blocker, b"").unwrap(); + let fallback = tmp.path().join("fallback"); + + let picked = + pick_session_base(&[(blocker.join("sessions"), false), (fallback.clone(), true)]) + .unwrap(); + assert_eq!(picked.display_path(), fallback); + assert!(fallback.join("x").parent().unwrap().exists()); + } + + #[test] + fn skips_an_existing_read_only_dir() { + if nix::unistd::Uid::current().is_root() { + eprintln!("skipped: root bypasses directory permissions"); + return; + } + let tmp = tempfile::tempdir().unwrap(); + let ro = tmp.path().join("ro"); + std::fs::create_dir_all(&ro).unwrap(); + std::fs::set_permissions(&ro, std::fs::Permissions::from_mode(0o500)).unwrap(); + let fallback = tmp.path().join("fallback"); + + // create_dir_all succeeds on `ro` (it already exists), so only the + // write probe can catch it. + let picked = pick_session_base(&[(ro.clone(), false), (fallback.clone(), true)]).unwrap(); + assert_eq!(picked.display_path(), fallback); + assert!( + !std::fs::read_dir(&ro) + .unwrap() + .next() + .is_some_and(|e| e.is_ok_and(|e| e + .file_name() + .to_string_lossy() + .starts_with(".anolisa-probe-"))), + "probe must not leave a file behind in a directory it rejected" + ); + } + + #[test] + fn refuses_a_symlinked_fallback_but_honours_a_symlinked_configured_dir() { + let tmp = tempfile::tempdir().unwrap(); + let real = tmp.path().join("real"); + std::fs::create_dir_all(&real).unwrap(); + let link = tmp.path().join("link"); + std::os::unix::fs::symlink(&real, &link).unwrap(); + let good = tmp.path().join("good"); + + // A directory we chose has to be a real one, or any local user could + // plant the name and redirect every session root. + let picked = pick_session_base(&[(link.clone(), true), (good.clone(), true)]).unwrap(); + assert_eq!(picked.display_path(), good); + + // The operator's own symlink is their decision, not an attack — but + // it is still resolved exactly once, into the descriptor the winner + // carries, so nothing after this point re-walks the name. + let picked = pick_session_base(&[(link.clone(), false), (good, true)]).unwrap(); + assert_eq!(picked.display_path(), link); + assert_eq!( + picked.path().canonicalize().unwrap(), + real.canonicalize().unwrap(), + "the handle must be anchored to the inode the link points at" + ); + } + + #[test] + fn creates_a_fallback_0700() { + let tmp = tempfile::tempdir().unwrap(); + let ours = tmp.path().join("fresh").join("anolisa-sessions-1000"); + let picked = pick_session_base(&[(ours.clone(), true)]).unwrap(); + assert_eq!(picked.display_path(), ours); + assert_eq!(modes(&ours), 0o700, "fallback base must not be listable"); + } + + #[test] + fn reports_every_candidate_when_none_is_usable() { + let tmp = tempfile::tempdir().unwrap(); + let blocker = tmp.path().join("blocker"); + std::fs::write(&blocker, b"").unwrap(); + let err = pick_session_base(&[(blocker.join("a"), false), (blocker.join("b"), true)]) + .unwrap_err() + .to_string(); + assert!(err.contains("no writable session directory"), "{err}"); + assert!(err.contains("blocker/a"), "{err}"); + assert!(err.contains("blocker/b"), "{err}"); + } + + #[test] + fn candidate_chain_starts_with_the_configured_dir_and_ends_in_tmp() { + let configured = Path::new("/run/anolisa/sessions"); + let candidates = session_base_candidates(configured); + assert_eq!(candidates[0].0, configured); + assert!( + !candidates[0].1, + "the configured dir is the operator's choice" + ); + let (last, ours) = candidates.last().unwrap(); + assert!(ours, "fallbacks must be hardened"); + assert!( + last.file_name() + .is_some_and(|n| n.to_string_lossy().starts_with("anolisa-sessions-")), + "tmp fallback must be uid-suffixed, got {}", + last.display() + ); + } + + // ---------- review follow-ups: hardening the probe and the fallback ---------- + + #[test] + fn fallback_owner_check_has_no_root_exemption() { + // `/tmp/anolisa-sessions-0` is a predictable name in a world-writable + // directory. Accepting an existing base just because the server + // happens to be uid 0 hands any local user control over every session + // root the root server builds inside it, so the check compares + // against the effective uid and nothing else. + assert!(fallback_owner_is_us(0, 0)); + assert!(fallback_owner_is_us(1000, 1000)); + assert!( + !fallback_owner_is_us(1000, 0), + "a root server must reject a user-owned fallback" + ); + assert!(!fallback_owner_is_us(0, 1000)); + } + + #[test] + fn rejects_a_foreign_owned_fallback_even_as_root() { + use nix::unistd::{Gid, Uid, chown}; + + if !nix::unistd::Uid::current().is_root() { + eprintln!("skipped: planting a foreign owner needs root"); + return; + } + let tmp = tempfile::tempdir().unwrap(); + let planted = tmp.path().join("anolisa-sessions-0"); + std::fs::create_dir_all(&planted).unwrap(); + // Addressed numerically, so this does not depend on `nobody` existing. + chown( + &planted, + Some(Uid::from_raw(65534)), + Some(Gid::from_raw(65534)), + ) + .unwrap(); + let good = tmp.path().join("next"); + + let picked = pick_session_base(&[(planted.clone(), true), (good.clone(), true)]).unwrap(); + + assert_eq!( + picked.display_path(), + good.as_path(), + "must fall through instead of using the foreign-owned {}", + planted.display() + ); + } + + #[test] + fn probe_refuses_to_follow_a_planted_symlink() { + // The probe name is predictable (pid + a counter), so on a base + // another local user can write to it can be pre-planted. That + // includes an operator-configured one: a group-writable + // MEMORY_SESSION_DIR is explicitly allowed. `std::fs::write` followed + // the link and truncated the target, which made the writability check + // itself a file-clobber primitive against anything the server uid can + // write; O_CREAT|O_EXCL refuses the name instead. + let tmp = tempfile::tempdir().unwrap(); + let victim = tmp.path().join("victim"); + std::fs::write(&victim, b"precious").unwrap(); + let base = tmp.path().join("base"); + std::fs::create_dir_all(&base).unwrap(); + let planted = base.join(format!(".anolisa-probe-{}-7", std::process::id())); + std::os::unix::fs::symlink(&victim, &planted).unwrap(); + + let handle = SessionBase::open(&base).unwrap(); + let err = probe_writable(&handle, 7).unwrap_err().to_string(); + assert!(err.contains("symlink"), "{err}"); + assert_eq!( + std::fs::read(&victim).unwrap(), + b"precious", + "the symlink target must survive the probe" + ); + assert!( + std::fs::symlink_metadata(&planted) + .unwrap() + .file_type() + .is_symlink(), + "a squatter's file is not ours to unlink" + ); + } + + #[test] + fn probe_steps_over_a_stale_file_and_removes_only_its_own() { + let tmp = tempfile::tempdir().unwrap(); + let base = tmp.path().join("base"); + std::fs::create_dir_all(&base).unwrap(); + let pid = std::process::id(); + let stale = base.join(format!(".anolisa-probe-{pid}-3")); + std::fs::write(&stale, b"left behind by a killed server").unwrap(); + + let handle = SessionBase::open(&base).unwrap(); + probe_writable(&handle, 3).unwrap(); + + assert_eq!( + std::fs::read(&stale).unwrap(), + b"left behind by a killed server", + "a stale probe is not ours to delete" + ); + let probes: Vec = std::fs::read_dir(&base) + .unwrap() + .filter_map(|e| e.ok()) + .map(|e| e.file_name().to_string_lossy().into_owned()) + .filter(|n| n.starts_with(".anolisa-probe-")) + .collect(); + assert_eq!( + probes, + vec![stale.file_name().unwrap().to_string_lossy().into_owned()], + "our own probe must be cleaned up" + ); + } + + #[test] + fn tightens_a_pre_existing_fallback_to_0700() { + // A base created before this hardening keeps whatever the umask gave + // it. The chmod that fixes that is no longer a `let _ =`, so a + // filesystem that refuses it rejects the candidate instead of quietly + // holding session data in a directory other users can list. + let tmp = tempfile::tempdir().unwrap(); + let ours = tmp.path().join("anolisa-sessions-1000"); + std::fs::create_dir_all(&ours).unwrap(); + std::fs::set_permissions(&ours, std::fs::Permissions::from_mode(0o755)).unwrap(); + + let picked = pick_session_base(&[(ours.clone(), true)]).unwrap(); + + assert_eq!(picked.display_path(), ours); + assert_eq!(modes(&ours), 0o700); + } + + #[test] + fn leaves_an_operator_configured_dir_mode_alone() { + // The tightening above is for directories *we* chose. An operator who + // points MEMORY_SESSION_DIR at a group-shared mount made that call + // deliberately, and the probe must not widen or narrow it. + let tmp = tempfile::tempdir().unwrap(); + let configured = tmp.path().join("shared"); + std::fs::create_dir_all(&configured).unwrap(); + std::fs::set_permissions(&configured, std::fs::Permissions::from_mode(0o770)).unwrap(); + + let picked = pick_session_base(&[(configured.clone(), false)]).unwrap(); + + assert_eq!(picked.display_path(), configured); + assert_eq!(modes(&configured), 0o770); + } + + // ---- review round 2: one descriptor for validation *and* use ---- + + #[test] + fn a_symlink_to_a_directory_we_own_is_still_refused() { + // `metadata()` would follow this and happily report a directory + // owned by us with a mode we chose, which is exactly why the check + // is a single `O_NOFOLLOW` open rather than a pair of pathname + // lookups that a rename can slip between. + let tmp = tempfile::tempdir().unwrap(); + let owned = tmp.path().join("ours"); + std::fs::create_dir_all(&owned).unwrap(); + std::fs::set_permissions(&owned, std::fs::Permissions::from_mode(0o700)).unwrap(); + let link = tmp.path().join("anolisa-sessions-1000"); + std::os::unix::fs::symlink(&owned, &link).unwrap(); + + let err = probe_session_base(&link, true).unwrap_err().to_string(); + + assert!(err.contains("symlink"), "{err}"); + assert_eq!( + std::fs::read_dir(&owned).unwrap().count(), + 0, + "the probe must not have written into the link target" + ); + } + + #[test] + fn probe_writes_into_the_validated_directory_not_the_swapped_name() { + let tmp = tempfile::tempdir().unwrap(); + let base = tmp.path().join("base"); + std::fs::create_dir_all(&base).unwrap(); + let target = tmp.path().join("target"); + std::fs::create_dir_all(&target).unwrap(); + + let handle = SessionBase::open_nofollow(&base).unwrap(); + + // The swap from the review: rename the validated directory away and + // leave a symlink to something else in its place. + let stolen = tmp.path().join("stolen"); + std::fs::rename(&base, &stolen).unwrap(); + std::os::unix::fs::symlink(&target, &base).unwrap(); + + // Occupy the probe name *in the swapped-in directory*. A probe that + // resolved the pathname would hit this symlink and refuse; anchored + // to the descriptor it never sees it. + let squat = target.join(format!(".anolisa-probe-{}-11", std::process::id())); + std::os::unix::fs::symlink(&target, &squat).unwrap(); + + probe_writable(&handle, 11).unwrap(); + + assert_eq!( + std::fs::read_dir(&target).unwrap().count(), + 1, + "only the squatter's own link may be under the redirect target" + ); + } + + #[test] + fn validation_and_use_stay_anchored_when_the_pathname_is_swapped() { + // End to end: validate, let a local user swap the pathname, then + // build the session. Two pathname lookups passed both the ownership + // and the mode check against the wrong inode here and went on to + // write the session under the redirect target; one `open` plus + // `fstat` on the descriptor cannot be redirected after the fact. + let tmp = tempfile::tempdir().unwrap(); + let base = tmp.path().join("anolisa-sessions-1000"); + std::fs::create_dir_all(&base).unwrap(); + let target = tmp.path().join("already-owned-by-the-server"); + std::fs::create_dir_all(&target).unwrap(); + + let picked = pick_session_base(&[(base.clone(), true)]).unwrap(); + + let stolen = tmp.path().join("stolen"); + std::fs::rename(&base, &stolen).unwrap(); + std::os::unix::fs::symlink(&target, &base).unwrap(); + + let session = SessionLogService::start_in( + picked, + SessionId::from_string("ses_anchored").unwrap(), + "alice", + Some("test"), + "user-alice", + None, + ) + .unwrap(); + + let real_root = session.root().canonicalize().unwrap(); + assert!( + real_root.starts_with(stolen.canonicalize().unwrap()), + "the session must stay in the validated inode, got {}", + real_root.display() + ); + assert_eq!( + std::fs::read_dir(&target).unwrap().count(), + 0, + "nothing may be written under the redirect target" + ); + assert!(session.root().join("meta.toml").exists()); + assert!(session.scratch_root().is_dir()); + assert!(session.log_path().exists()); + assert_eq!( + session.display_root(), + base.join("ses_anchored"), + "the reported path stays the one the operator recognises" + ); + } + + // ---- review round 2: the fallback name follows the host uid ---- + + #[test] + fn tmp_fallback_is_named_after_the_host_uid_not_the_namespace_uid() { + // `early_enter_userns` runs before the service is built and maps + // `0 1`, so by the time the candidate chain is computed + // `geteuid()` is 0 for every user on the box. Naming the tmp + // fallback after that makes them all compute + // `/tmp/anolisa-sessions-0`; whoever creates it first owns a + // directory every other user's namespace reports as foreign-owned, + // so the ownership check rejects the last candidate and they lose + // the session anyway. + let configured = Path::new("/run/anolisa/sessions"); + + let alice = candidates_for_host_uid(configured, 1000); + let bob = candidates_for_host_uid(configured, 1001); + + assert_eq!( + alice.last().unwrap().0.file_name().unwrap(), + "anolisa-sessions-1000" + ); + assert_eq!( + bob.last().unwrap().0.file_name().unwrap(), + "anolisa-sessions-1001" + ); + assert_ne!( + alice.last().unwrap().0, + bob.last().unwrap().0, + "two host users sharing a TMPDIR must not compute one base" + ); + } + + #[test] + fn the_live_candidate_chain_uses_the_host_identity() { + // `host_uid()` is the captured launching uid (or, if nothing was + // captured, the one recovered from `/proc/self/uid_map`), so the + // chain the real code builds never depends on which namespace we + // happen to be in when it is built. + let configured = Path::new("/run/anolisa/sessions"); + assert_eq!( + session_base_candidates(configured), + candidates_for_host_uid(configured, crate::host::host_uid()) + ); + } +} diff --git a/src/agent-memory/src/session/base.rs b/src/agent-memory/src/session/base.rs new file mode 100644 index 0000000000..ed62da763c --- /dev/null +++ b/src/agent-memory/src/session/base.rs @@ -0,0 +1,195 @@ +//! An anchored handle on the directory a session tree is built in. +//! +//! Deciding that a directory may hold sessions and then *using* it are two +//! separate pathname lookups, and a local user who can write to the parent +//! can change what the name resolves to in between. Checking the pathname +//! twice does not help: `symlink_metadata` followed by `metadata` is exactly +//! the pair a rename-then-symlink swap defeats, because each call resolves +//! the name from scratch. +//! +//! So the caller opens the directory **once** — with `O_NOFOLLOW` when it is +//! a name we picked — and validates the resulting descriptor with `fstat`. +//! This type turns that descriptor into the path every later operation uses: +//! `/proc/self/fd/` is a magic symlink that the kernel resolves to the +//! *inode* the descriptor holds rather than re-walking the original +//! pathname, so renaming the directory or replacing it with a symlink +//! afterwards cannot redirect anything built through [`SessionBase::path`]. +//! +//! The descriptor has to stay open for as long as those paths are used, +//! which is why `SessionBase` owns it and why `SessionLogService` keeps the +//! `SessionBase` alive next to the paths derived from it. + +use std::os::fd::{AsRawFd, FromRawFd, OwnedFd, RawFd}; +use std::path::{Path, PathBuf}; + +use nix::errno::Errno; +use nix::fcntl::{AtFlags, OFlag}; +use nix::sys::stat::{Mode, SFlag}; + +use crate::error::{MemoryError, Result}; + +/// A directory that was opened exactly once and is only ever used through +/// that one open file description. +#[derive(Debug)] +pub struct SessionBase { + /// The pathname the operator configured, or the fallback we chose. + /// For messages and logs only — never used to touch the filesystem + /// again, which is the whole point. + display: PathBuf, + /// `/proc/self/fd/`. Every filesystem operation goes through this. + anchored: PathBuf, + /// Keeps `anchored` resolving and keeps the descriptor number from being + /// recycled to an unrelated file. + fd: OwnedFd, +} + +impl SessionBase { + /// Open `dir` as a session base, refusing a symlink in its final + /// component. This is the gate for directories *we* chose: an attacker + /// who plants the name gets `ELOOP` instead of a redirected session. + pub fn open_nofollow(dir: &Path) -> Result { + Self::open_with(dir, OFlag::O_NOFOLLOW) + } + + /// Open `dir` as a session base, honouring a symlink in its final + /// component. This is the gate for the operator-configured directory, + /// where the link is the operator's own decision — but it is still + /// resolved exactly once, here, and everything afterwards is anchored. + pub fn open(dir: &Path) -> Result { + Self::open_with(dir, OFlag::empty()) + } + + /// Like [`Self::open`], creating `dir` first when it does not exist. + /// + /// The retry still goes through `open`, so a name that turns into + /// something else between the `ENOENT` and the `mkdir` is caught by the + /// same single resolution rather than by a second pathname check. + pub fn open_creating(dir: &Path) -> Result { + match Self::open(dir) { + Ok(base) => Ok(base), + Err(MemoryError::Io(e)) if e.kind() == std::io::ErrorKind::NotFound => { + std::fs::create_dir_all(dir)?; + Self::open(dir) + } + Err(e) => Err(e), + } + } + + fn open_with(dir: &Path, extra: OFlag) -> Result { + let flags = OFlag::O_RDONLY | OFlag::O_DIRECTORY | OFlag::O_CLOEXEC | extra; + let raw = match nix::fcntl::open(dir, flags, Mode::empty()) { + Ok(raw) => raw, + Err(Errno::ENOTDIR | Errno::ELOOP) => { + // The open already refused the candidate; this lookup only + // picks the message. A symlink surfaces as `ELOOP` under + // `O_NOFOLLOW`, and as `ENOTDIR` once `O_DIRECTORY` is in + // the mix, because the kernel then checks the *link* for + // being a directory. + let is_link = + std::fs::symlink_metadata(dir).is_ok_and(|md| md.file_type().is_symlink()); + let why = if is_link { + "is a symlink; refusing it as a session dir" + } else { + "is not a directory" + }; + return Err(MemoryError::Other(format!("{} {why}", dir.display()))); + } + Err(e) => return Err(io_error(e).into()), + }; + // SAFETY: `open` just handed us a descriptor nobody else owns. + let fd = unsafe { OwnedFd::from_raw_fd(raw) }; + Self::from_fd(dir.to_path_buf(), fd) + } + + /// Adopt an already-open directory descriptor. + fn from_fd(display: PathBuf, fd: OwnedFd) -> Result { + let anchored = PathBuf::from(format!("/proc/self/fd/{}", fd.as_raw_fd())); + + // Everything below rests on `/proc/self/fd/` resolving to this + // descriptor's inode. Check that once, here, so a host without a + // usable /proc fails with an explanation instead of turning into a + // mysterious ENOENT partway through session setup. + let direct = nix::sys::stat::fstat(fd.as_raw_fd()).map_err(io_error)?; + let through = nix::sys::stat::stat(&anchored).map_err(|e| { + MemoryError::Other(format!( + "cannot anchor {} to an open descriptor ({}: {e}); /proc must be mounted", + display.display(), + anchored.display() + )) + })?; + if (direct.st_dev, direct.st_ino) != (through.st_dev, through.st_ino) { + return Err(MemoryError::Other(format!( + "{} does not resolve to {}; cannot anchor the session dir", + anchored.display(), + display.display() + ))); + } + + Ok(Self { + display, + anchored, + fd, + }) + } + + /// The raw descriptor, for the `*at` family. + pub fn fd(&self) -> RawFd { + self.fd.as_raw_fd() + } + + /// The path every filesystem operation must use. + pub fn path(&self) -> &Path { + &self.anchored + } + + /// The pathname the operator configured or we chose. Display only. + pub fn display_path(&self) -> &Path { + &self.display + } + + /// `stat` the anchored directory — the descriptor's own inode, never a + /// re-resolved pathname. + pub fn stat(&self) -> Result { + nix::sys::stat::fstat(self.fd()).map_err(mem_io_error) + } + + /// `mkdirat` a direct child of this base, i.e. create it relative to the + /// descriptor without re-resolving the base pathname at all. + /// + /// An existing child is not an error — a session may be reopened — but + /// whatever occupies the name has to be a real directory, checked + /// without following it. A name that is a symlink would quietly send + /// every later write to its target. + pub fn mkdir_child(&self, name: &str, mode: u32) -> Result<()> { + let bits = Mode::from_bits_truncate(mode); + match nix::sys::stat::mkdirat(Some(self.fd()), name, bits) { + Ok(()) | Err(Errno::EEXIST) => {} + Err(e) => { + return Err(MemoryError::Other(format!( + "mkdirat({}, {name}): {e}", + self.display.display() + ))); + } + } + + let st = nix::sys::stat::fstatat(Some(self.fd()), name, AtFlags::AT_SYMLINK_NOFOLLOW) + .map_err(|e| { + MemoryError::Other(format!("fstatat({}, {name}): {e}", self.display.display())) + })?; + if !SFlag::from_bits_truncate(st.st_mode).contains(SFlag::S_IFDIR) { + return Err(MemoryError::Other(format!( + "{name} under {} is not a directory", + self.display.display() + ))); + } + Ok(()) + } +} + +fn io_error(e: Errno) -> std::io::Error { + std::io::Error::from_raw_os_error(e as i32) +} + +fn mem_io_error(e: Errno) -> MemoryError { + MemoryError::Io(io_error(e)) +} diff --git a/src/agent-memory/src/session/mod.rs b/src/agent-memory/src/session/mod.rs index 11ee32b8aa..fb7f9330e1 100644 --- a/src/agent-memory/src/session/mod.rs +++ b/src/agent-memory/src/session/mod.rs @@ -9,10 +9,12 @@ //! Tests set `MEMORY_SESSION_DIR` to a tempdir to avoid colliding with //! `/run/anolisa/sessions/` in the host. +pub mod base; pub mod id; pub mod paths; pub mod service; +pub use base::SessionBase; pub use id::SessionId; pub use paths::resolve_in_scratch; pub use service::{EndAction, SessionLogService}; diff --git a/src/agent-memory/src/session/service.rs b/src/agent-memory/src/session/service.rs index f2198fe51f..af0392fb60 100644 --- a/src/agent-memory/src/session/service.rs +++ b/src/agent-memory/src/session/service.rs @@ -7,6 +7,7 @@ use std::sync::Mutex; use chrono::Utc; use serde::{Deserialize, Serialize}; +use super::base::SessionBase; use super::id::SessionId; use crate::audit::AuditEntry; use crate::error::{MemoryError, Result}; @@ -42,7 +43,16 @@ pub enum EndAction { /// Per-process session scratch + log. pub struct SessionLogService { sid: SessionId, + /// Keeps the base directory's descriptor open. `root`, `scratch` and + /// `log_path` are all built on `/proc/self/fd/`, which only resolves + /// while this is alive — dropping it would dangle every path below and + /// let the descriptor number be recycled to an unrelated file. + base: SessionBase, + /// `/` — the path every filesystem operation uses. root: PathBuf, + /// `/` — the same directory by the name an operator + /// recognises. Reporting only. + display_root: PathBuf, scratch: PathBuf, log_path: PathBuf, /// Held file handle for jsonl appends — avoids repeated open/close. @@ -69,10 +79,37 @@ impl SessionLogService { mount_ns: &str, mirror_dir: Option<&Path>, ) -> Result { - let root = base_dir.as_ref().join(sid.as_str()); + Self::start_in( + SessionBase::open_creating(base_dir.as_ref())?, + sid, + owner_user_id, + agent_id, + mount_ns, + mirror_dir, + ) + } + + /// Same as [`Self::start`], but on a base that has already been opened + /// and validated. The session tree is built through that one + /// descriptor, so nothing that happens to the base's *pathname* between + /// validation and here can redirect it. + pub fn start_in( + base: SessionBase, + sid: SessionId, + owner_user_id: &str, + agent_id: Option<&str>, + mount_ns: &str, + mirror_dir: Option<&Path>, + ) -> Result { + let root = base.path().join(sid.as_str()); + let display_root = base.display_path().join(sid.as_str()); let scratch = root.join(SCRATCH_DIR); let log_path = root.join(LOG_FILE); + // `` is created with mkdirat against the base descriptor: the + // one step where a swapped pathname would matter most, and the one + // that can be done without touching the pathname at all. + base.mkdir_child(sid.as_str(), 0o700)?; std::fs::create_dir_all(&scratch)?; // Enforce 0700 on session root so only the owner can read // meta.toml (owner_user_id, agent_id, mount_ns) and log.jsonl @@ -133,7 +170,9 @@ impl SessionLogService { Ok(Self { sid, + base, root, + display_root, scratch, log_path, log_file: Mutex::new(log_file), @@ -145,10 +184,34 @@ impl SessionLogService { &self.sid } + /// The session directory as paths see it: anchored to the descriptor the + /// base was validated through, so nothing that happens to the base's + /// pathname can redirect what is read or written here. + /// + /// Valid only while this service is alive — the descriptor backing it is + /// dropped with the service. Use [`Self::display_root`] for anything + /// that has to outlive it (logs, a path shown to an operator). pub fn root(&self) -> &Path { &self.root } + /// The same directory by the pathname an operator configured or the + /// fallback message reported. Use this in logs; use [`Self::root`] for + /// anything that touches the filesystem. + pub fn display_root(&self) -> &Path { + &self.display_root + } + + /// The base directory this session lives under. + /// + /// Mostly here to be explicit about why the field exists at all: holding + /// the [`SessionBase`] is what keeps the descriptor open, and therefore + /// what keeps every path above resolving to the directory that was + /// validated rather than to whatever the pathname points at now. + pub fn base(&self) -> &SessionBase { + &self.base + } + pub fn scratch_root(&self) -> &Path { &self.scratch } diff --git a/src/agent-memory/tests/session_test.rs b/src/agent-memory/tests/session_test.rs index 200ab43742..2c85f6c831 100644 --- a/src/agent-memory/tests/session_test.rs +++ b/src/agent-memory/tests/session_test.rs @@ -85,8 +85,11 @@ fn end_discard_removes_dir() { None, ) .unwrap(); - let root = svc.root().to_path_buf(); + // `end` consumes the service, which closes the descriptor the anchored + // `root()` path is built on — assert on the operator-facing pathname. + let root = svc.display_root().to_path_buf(); assert!(root.exists()); + assert!(svc.root().exists()); svc.end(EndAction::Discard).unwrap(); assert!(!root.exists()); } @@ -130,9 +133,10 @@ fn end_keep_preserves_dir() { None, ) .unwrap(); - let root = svc.root().to_path_buf(); + let root = svc.display_root().to_path_buf(); svc.end(EndAction::Keep).unwrap(); assert!(root.exists()); + assert!(root.join("meta.toml").exists()); } // ---------- mem_promote integration ---------- @@ -218,33 +222,165 @@ fn session_log_includes_promote_and_prior_session_log_call() { ); } +// ---------- session-dir fallback ---------- +// +// This case runs in a child process, and that is not stylistic. The fallback +// chain is built from the *inherited* `XDG_RUNTIME_DIR` / `TMPDIR`, and +// `start_session` reads the inherited `MEMORY_SESSION_ID`, so exercising it +// in-process would probe — and chmod `0700` — the real per-user runtime dir, +// and, if the id it happened to inherit already had a session there, reopen +// that live session, write `scratch/draft.md` into it and let cleanup +// recursively delete the lot, unrelated scratch data included. The child +// gets a runtime dir, a tmp dir and a session id that are all this test's +// own, and the parent plants a decoy session next to it so the assertion +// covers what cleanup is allowed to reach. + +const FALLBACK_CHILD_ENV: &str = "ANOLISA_TEST_SESSION_FALLBACK_CHILD"; +const FALLBACK_CHILD_STORE: &str = "ANOLISA_TEST_STORE"; +const FALLBACK_CHILD_BLOCKER: &str = "ANOLISA_TEST_BLOCKER"; +const FALLBACK_CHILD_BASE: &str = "ANOLISA_TEST_FALLBACK_BASE"; +const FALLBACK_SID: &str = "ses_fallback_child"; +const DECOY_SID: &str = "ses_fallback_decoy"; + #[test] -fn session_log_degrades_gracefully_when_session_dir_unavailable() { - // Make the session base dir a regular file → create_dir_all fails → - // service still constructs but svc.session == None; session-dependent - // tools return NotImplemented. - let store_tmp = tempdir().unwrap(); +fn unusable_session_dir_falls_back_instead_of_losing_the_session() { + if std::env::var_os(FALLBACK_CHILD_ENV).is_some() { + fallback_child(); + return; + } + fallback_parent(); +} + +fn fallback_parent() { + use std::os::unix::fs::PermissionsExt; + + // Every directory the child can reach is one we hand it. + let runtime = tempdir().unwrap(); + let child_tmp = tempdir().unwrap(); + let store = tempdir().unwrap(); let blocker = tempdir().unwrap(); + + // Make the configured session base a regular file so `create_dir_all` can + // never succeed — the same failure a non-root server gets from the + // shipped default /run/anolisa/sessions, whose parent the RPM creates + // 0700 root:root and which make install / containers do not create at + // all (/run is drwxr-xr-x root root). let blocking_file = blocker.path().join("not-a-dir"); - std::fs::write(&blocking_file, "").unwrap(); + std::fs::write(&blocking_file, b"").unwrap(); + + // A live session already sitting in the fallback base. The child must + // neither write into it nor let its cleanup reach it. + let fallback_base = runtime.path().join("anolisa").join("sessions"); + let decoy_root = fallback_base.join(DECOY_SID); + std::fs::create_dir_all(decoy_root.join("scratch")).unwrap(); + for d in [&fallback_base, &decoy_root] { + std::fs::set_permissions(d, std::fs::Permissions::from_mode(0o700)).unwrap(); + } + let decoy_file = decoy_root.join("scratch").join("keep.md"); + std::fs::write(&decoy_file, b"someone else's session").unwrap(); + + let exe = std::env::current_exe().expect("path to this test binary"); + let out = std::process::Command::new(exe) + .arg("--exact") + .arg("unusable_session_dir_falls_back_instead_of_losing_the_session") + .arg("--nocapture") + .env(FALLBACK_CHILD_ENV, "1") + .env("XDG_RUNTIME_DIR", runtime.path()) + .env("TMPDIR", child_tmp.path()) + .env("MEMORY_SESSION_ID", FALLBACK_SID) + .env(FALLBACK_CHILD_STORE, store.path()) + .env(FALLBACK_CHILD_BLOCKER, &blocking_file) + .env(FALLBACK_CHILD_BASE, &fallback_base) + .output() + .expect("spawn the isolated child"); + + let stdout = String::from_utf8_lossy(&out.stdout); + let stderr = String::from_utf8_lossy(&out.stderr); + assert!( + out.status.success(), + "child exited with {:?}\n--- stdout ---\n{stdout}--- stderr ---\n{stderr}", + out.status + ); + assert!( + stdout.contains("fallback child ok:"), + "the child body did not run to completion\n--- stdout ---\n{stdout}--- stderr ---\n{stderr}" + ); + + // The child's cleanup was scoped to its own session id. + assert!( + !fallback_base.join(FALLBACK_SID).exists(), + "the child's own session must be gone after Discard" + ); + assert_eq!( + std::fs::read(&decoy_file).unwrap(), + b"someone else's session", + "cleanup must not reach a live session sharing the fallback base" + ); + + // The fallback it landed on was the runtime dir we gave it, so the tmp + // candidate was never needed — and, because both were test-owned, the + // real per-user directories were never probed or chmod'ed at all. + let leaked: Vec = std::fs::read_dir(child_tmp.path()) + .unwrap() + .filter_map(|e| e.ok()) + .map(|e| e.file_name().to_string_lossy().into_owned()) + .filter(|n| n.starts_with("anolisa-sessions-")) + .collect(); + assert!( + leaked.is_empty(), + "the tmp fallback must not be reached once the runtime dir works: {leaked:?}" + ); +} + +fn fallback_child() { + use std::path::Path; + + let store = std::env::var(FALLBACK_CHILD_STORE).expect("store dir"); + let blocker = std::env::var(FALLBACK_CHILD_BLOCKER).expect("blocking file"); + let fallback_base = std::env::var(FALLBACK_CHILD_BASE).expect("fallback base"); + let sid = std::env::var("MEMORY_SESSION_ID").expect("session id"); let mut cfg = AppConfig::default(); cfg.global.user_id = "carol".into(); - cfg.memory.paths.base_dir = store_tmp.path().to_string_lossy().into(); - cfg.memory.session.base_dir = blocking_file.to_string_lossy().into(); + cfg.memory.paths.base_dir = store; + cfg.memory.session.base_dir = blocker; cfg.memory.mount.strategy = agent_memory::mount::MountStrategyKind::Userland; let svc = MemoryService::new(cfg).expect("service should still build"); - assert!( - svc.session.is_none(), - "session should be None when base unwritable" - ); - let err = svc.session_log().unwrap_err(); - assert!(matches!(err, MemoryError::NotImplemented(_))); + // Before the fallback existed this degraded to `session == None` behind + // a single `warn!`, so every `mem_promote` / `mem_session_log` call + // errored on a stock install. + let session = svc + .session + .as_ref() + .expect("session must be recovered from a fallback dir, not lost"); - let err = svc.promote("x.md", "y.md").unwrap_err(); - assert!(matches!(err, MemoryError::NotImplemented(_))); + assert_eq!(session.sid().as_str(), sid, "must honour the pinned id"); + + let expected = Path::new(&fallback_base).canonicalize().unwrap().join(&sid); + assert_eq!( + session.root().canonicalize().unwrap(), + expected, + "the fallback must be the test-owned runtime dir" + ); + assert_eq!(session.display_root().canonicalize().unwrap(), expected); + + // Both session-dependent tools work end to end through the fallback. + std::fs::write(session.scratch_root().join("draft.md"), b"fallback scratch").unwrap(); + let n = svc.promote("draft.md", "notes/from-fallback.md").unwrap(); + assert!(n > 0); + assert!(svc.mount.root.join("notes/from-fallback.md").exists()); + svc.session_log().unwrap(); + + // Go through the real shutdown path so the parent can inspect exactly + // what cleanup touched. + let action = svc.config.memory.session.end_action; + svc.try_end_session(action); + + // Proof for the parent that this body really ran to completion — a child + // that silently matched no test would also exit 0. + println!("fallback child ok: {}", expected.display()); } // ---------- audit double-write ----------