fix(sandbox): handle per-path Landlock errors instead of abandoning entire ruleset (#677)

* fix(sandbox): handle per-path Landlock errors instead of abandoning entire ruleset

A single missing path (e.g., /app in containers without that directory)
caused PathFd::new() to propagate an error out of the entire Landlock
setup closure. Under BestEffort mode, this silently disabled all
filesystem restrictions for the sandbox.

Changes:
- Extract try_open_path() and classify_path_error() helpers that handle
  PathFd failures per-path instead of per-ruleset
- BestEffort mode: skip inaccessible paths with a warning, apply
  remaining rules
- HardRequirement mode: fail immediately on any inaccessible path
- Add zero-rule safety check to prevent applying an empty ruleset that
  would block all filesystem access
- Pre-filter system-injected baseline paths (e.g., /app) in enrichment
  functions so missing paths never reach Landlock
- Add unit tests for try_open_path, classify_path_error, and error
  classification for ENOENT, EACCES, ELOOP, ENAMETOOLONG, ENOTDIR
- Update user-facing docs and architecture docs with Landlock behavior
  tables, baseline path filtering, and compatibility mode semantics
- Fix stale ABI::V1 references in docs (code uses ABI::V2)

Closes #664

* fix(sandbox): use debug log for NotFound in Landlock best-effort mode

NotFound errors for stale baseline paths (e.g. /app persisted in the
server-stored policy but absent in this container) are expected in
best-effort mode. Downgrade from warn! to debug! so the message does
not leak into SSH exec stdout (the pre_exec hook inherits the tracing
subscriber whose writer targets fd 1).

Genuine errors (permission denied, symlink loops, etc.) remain at warn!
for operator visibility.

Also move custom_image e2e marker from /opt to /etc (a Landlock baseline
read-only path) since the security fix now properly enforces filesystem
restrictions.

---------

Co-authored-by: John Myers <johntmyers@users.noreply.github.com>

Author: johntmyers

architecture/sandbox.md

@@ -431,15 +431,21 @@ Landlock restricts the child process's filesystem access to an explicit allowlis
 1. Build path lists from `filesystem.read_only` and `filesystem.read_write`
 2. If `include_workdir` is true, add the working directory to `read_write`
 3. If both lists are empty, skip Landlock entirely (no-op)
-4. Create a Landlock ruleset targeting ABI V1:
+4. Create a Landlock ruleset targeting ABI V2:
    - Read-only paths receive `AccessFs::from_read(abi)` rights
    - Read-write paths receive `AccessFs::from_all(abi)` rights
-5. Call `ruleset.restrict_self()` -- this applies to the calling process and all descendants
+5. For each path, attempt `PathFd::new()`. If it fails:
+   - `BestEffort`: Log a warning with the error classification (not found, permission denied, symlink loop, etc.) and skip the path. Continue building the ruleset from remaining valid paths.
+   - `HardRequirement`: Return a fatal error, aborting the sandbox.
+6. If all paths failed (zero rules applied), return an error rather than calling `restrict_self()` on an empty ruleset (which would block all filesystem access)
+7. Call `ruleset.restrict_self()` -- this applies to the calling process and all descendants
 
-Error behavior depends on `LandlockCompatibility`:
+Kernel-level error behavior (e.g., Landlock ABI unavailable) depends on `LandlockCompatibility`:
 - `BestEffort`: Log a warning and continue without filesystem isolation
 - `HardRequirement`: Return a fatal error, aborting the sandbox
 
+**Baseline path filtering**: System-injected baseline paths (e.g., `/app`) are pre-filtered by `enrich_proto_baseline_paths()` / `enrich_sandbox_baseline_paths()` using `Path::exists()` before they reach Landlock. User-specified paths are not pre-filtered -- they are evaluated at Landlock apply time so misconfigurations surface as warnings or errors.
+
 ### Seccomp syscall filtering
 
 **File:** `crates/openshell-sandbox/src/sandbox/linux/seccomp.rs`

architecture/security-policy.md

@@ -320,7 +320,7 @@ Controls which filesystem paths the sandboxed process can access. Enforced via L
 | `read_only`       | `string[]` | `[]`    | Paths accessible in read-only mode                             |
 | `read_write`      | `string[]` | `[]`    | Paths accessible in read-write mode                            |
 
-**Enforcement mapping**: Each path becomes a Landlock `PathBeneath` rule. Read-only paths receive `AccessFs::from_read(ABI::V1)` permissions. Read-write paths receive `AccessFs::from_all(ABI::V1)` permissions (read, write, execute, create, delete, rename). All other paths are denied by the Landlock ruleset.
+**Enforcement mapping**: Each path becomes a Landlock `PathBeneath` rule. Read-only paths receive `AccessFs::from_read(ABI::V2)` permissions. Read-write paths receive `AccessFs::from_all(ABI::V2)` permissions (read, write, execute, create, delete, rename). All other paths are denied by the Landlock ruleset.
 
 **Filesystem preparation**: Before the child process spawns, the supervisor creates any `read_write` directories that do not exist and sets their ownership to `process.run_as_user`:`process.run_as_group` via `chown()`. See `crates/openshell-sandbox/src/lib.rs` -- `prepare_filesystem()`.
 
@@ -358,10 +358,16 @@ Controls Landlock LSM compatibility behavior. **Static field** -- immutable afte
 
 | Value              | Behavior                                                                                                                    |
 | ------------------ | --------------------------------------------------------------------------------------------------------------------------- |
-| `best_effort`      | If Landlock is unavailable (older kernel, unprivileged container), log a warning and continue without filesystem sandboxing |
-| `hard_requirement` | If Landlock is unavailable, abort sandbox startup with an error                                                             |
+| `best_effort`      | If Landlock is unavailable (older kernel, unprivileged container), log a warning and continue without filesystem sandboxing. Individual inaccessible paths (missing, permission denied, symlink loops) are skipped with a warning while remaining rules are still applied. If all paths fail, the sandbox continues without Landlock rather than applying an empty ruleset that would block all access. |
+| `hard_requirement` | If Landlock is unavailable or any configured path cannot be opened, abort sandbox startup with an error.                    |
 
-See `crates/openshell-sandbox/src/sandbox/linux/landlock.rs` -- `compat_level()`.
+**Per-path error handling**: `PathFd::new()` (which wraps `open(path, O_PATH | O_CLOEXEC)`) can fail for several reasons beyond path non-existence: `EACCES` (permission denied), `ELOOP` (symlink loop), `ENAMETOOLONG`, `ENOTDIR`. Each failure is classified with a human-readable reason in logs. In `best_effort` mode, the path is skipped and ruleset construction continues. In `hard_requirement` mode, the error is fatal.
+
+**Baseline path filtering**: The enrichment functions (`enrich_proto_baseline_paths`, `enrich_sandbox_baseline_paths`) pre-filter system-injected baseline paths (e.g., `/app`) by checking `Path::exists()` before adding them to the policy. This prevents missing baseline paths from reaching Landlock at all. User-specified paths are not pre-filtered — they are evaluated at Landlock apply time so that misconfigurations surface as warnings (`best_effort`) or errors (`hard_requirement`).
+
+**Zero-rule safety check**: If all paths in the ruleset fail to open, `apply()` returns an error rather than calling `restrict_self()` on an empty ruleset. An empty Landlock ruleset with `restrict_self()` would block all filesystem access — the inverse of the intended degradation behavior. This error is caught by the outer `BestEffort` handler, which logs a warning and continues without Landlock.
+
+See `crates/openshell-sandbox/src/sandbox/linux/landlock.rs` -- `compat_level()`, `try_open_path()`, `classify_path_fd_error()`, `classify_io_error()`.
 
 ```yaml
 landlock:

crates/openshell-sandbox/src/lib.rs

@@ -899,12 +899,31 @@ fn enrich_proto_baseline_paths(proto: &mut openshell_core::proto::SandboxPolicy)
     let mut modified = false;
     for &path in PROXY_BASELINE_READ_ONLY {
         if !fs.read_only.iter().any(|p| p.as_str() == path) {
+            // Baseline paths are system-injected, not user-specified. Skip
+            // paths that do not exist in this container image to avoid noisy
+            // warnings from Landlock and, more critically, to prevent a single
+            // missing baseline path from abandoning the entire Landlock
+            // ruleset under best-effort mode (see issue #664).
+            if !std::path::Path::new(path).exists() {
+                debug!(
+                    path,
+                    "Baseline read-only path does not exist, skipping enrichment"
+                );
+                continue;
+            }
             fs.read_only.push(path.to_string());
             modified = true;
         }
     }
     for &path in PROXY_BASELINE_READ_WRITE {
         if !fs.read_write.iter().any(|p| p.as_str() == path) {
+            if !std::path::Path::new(path).exists() {
+                debug!(
+                    path,
+                    "Baseline read-write path does not exist, skipping enrichment"
+                );
+                continue;
+            }
             fs.read_write.push(path.to_string());
             modified = true;
         }
@@ -929,13 +948,29 @@ fn enrich_sandbox_baseline_paths(policy: &mut SandboxPolicy) {
     for &path in PROXY_BASELINE_READ_ONLY {
         let p = std::path::PathBuf::from(path);
         if !policy.filesystem.read_only.contains(&p) {
+            // Baseline paths are system-injected — skip non-existent paths to
+            // avoid Landlock ruleset abandonment (issue #664).
+            if !p.exists() {
+                debug!(
+                    path,
+                    "Baseline read-only path does not exist, skipping enrichment"
+                );
+                continue;
+            }
             policy.filesystem.read_only.push(p);
             modified = true;
         }
     }
     for &path in PROXY_BASELINE_READ_WRITE {
         let p = std::path::PathBuf::from(path);
         if !policy.filesystem.read_write.contains(&p) {
+            if !p.exists() {
+                debug!(
+                    path,
+                    "Baseline read-write path does not exist, skipping enrichment"
+                );
+                continue;
+            }
             policy.filesystem.read_write.push(p);
             modified = true;
         }

crates/openshell-sandbox/src/sandbox/linux/landlock.rs

@@ -5,11 +5,11 @@
 
 use crate::policy::{LandlockCompatibility, SandboxPolicy};
 use landlock::{
-    ABI, Access, AccessFs, CompatLevel, Compatible, PathBeneath, PathFd, Ruleset, RulesetAttr,
-    RulesetCreatedAttr,
+    ABI, Access, AccessFs, CompatLevel, Compatible, PathBeneath, PathFd, PathFdError, Ruleset,
+    RulesetAttr, RulesetCreatedAttr,
 };
 use miette::{IntoDiagnostic, Result};
-use std::path::PathBuf;
+use std::path::{Path, PathBuf};
 use tracing::{debug, info, warn};
 
 pub fn apply(policy: &SandboxPolicy, workdir: Option<&str>) -> Result<()> {
@@ -29,6 +29,7 @@ pub fn apply(policy: &SandboxPolicy, workdir: Option<&str>) -> Result<()> {
         return Ok(());
     }
 
+    let total_paths = read_only.len() + read_write.len();
     let abi = ABI::V2;
     info!(
         abi = ?abi,
@@ -38,47 +39,61 @@ pub fn apply(policy: &SandboxPolicy, workdir: Option<&str>) -> Result<()> {
         "Applying Landlock filesystem sandbox"
     );
 
+    let compatibility = &policy.landlock.compatibility;
+
     let result: Result<()> = (|| {
         let access_all = AccessFs::from_all(abi);
         let access_read = AccessFs::from_read(abi);
 
         let mut ruleset = Ruleset::default();
         ruleset = ruleset
-            .set_compatibility(compat_level(&policy.landlock.compatibility))
+            .set_compatibility(compat_level(compatibility))
             .handle_access(access_all)
             .into_diagnostic()?;
 
         let mut ruleset = ruleset.create().into_diagnostic()?;
+        let mut rules_applied: usize = 0;
+
+        for path in &read_only {
+            if let Some(path_fd) = try_open_path(path, compatibility)? {
+                debug!(path = %path.display(), "Landlock allow read-only");
+                ruleset = ruleset
+                    .add_rule(PathBeneath::new(path_fd, access_read))
+                    .into_diagnostic()?;
+                rules_applied += 1;
+            }
+        }
 
-        for path in read_only {
-            debug!(path = %path.display(), "Landlock allow read-only");
-            ruleset = ruleset
-                .add_rule(PathBeneath::new(
-                    PathFd::new(path).into_diagnostic()?,
-                    access_read,
-                ))
-                .into_diagnostic()?;
+        for path in &read_write {
+            if let Some(path_fd) = try_open_path(path, compatibility)? {
+                debug!(path = %path.display(), "Landlock allow read-write");
+                ruleset = ruleset
+                    .add_rule(PathBeneath::new(path_fd, access_all))
+                    .into_diagnostic()?;
+                rules_applied += 1;
+            }
         }
 
-        for path in read_write {
-            debug!(path = %path.display(), "Landlock allow read-write");
-            ruleset = ruleset
-                .add_rule(PathBeneath::new(
-                    PathFd::new(path).into_diagnostic()?,
-                    access_all,
-                ))
-                .into_diagnostic()?;
+        if rules_applied == 0 {
+            return Err(miette::miette!(
+                "Landlock ruleset has zero valid paths — all {} path(s) failed to open. \
+                 Refusing to apply an empty ruleset that would block all filesystem access.",
+                total_paths,
+            ));
         }
 
+        let skipped = total_paths - rules_applied;
+        info!(
+            rules_applied,
+            skipped, "Landlock ruleset built successfully"
+        );
+
         ruleset.restrict_self().into_diagnostic()?;
         Ok(())
     })();
 
     if let Err(err) = result {
-        if matches!(
-            policy.landlock.compatibility,
-            LandlockCompatibility::BestEffort
-        ) {
+        if matches!(compatibility, LandlockCompatibility::BestEffort) {
             warn!(
                 error = %err,
                 "Landlock filesystem sandbox is UNAVAILABLE — running WITHOUT filesystem restrictions. \
@@ -92,9 +107,177 @@ pub fn apply(policy: &SandboxPolicy, workdir: Option<&str>) -> Result<()> {
     Ok(())
 }
 
+/// Attempt to open a path for Landlock rule creation.
+///
+/// In `BestEffort` mode, inaccessible paths (missing, permission denied, symlink
+/// loops, etc.) are skipped with a warning and `Ok(None)` is returned so the
+/// caller can continue building the ruleset from the remaining valid paths.
+///
+/// In `HardRequirement` mode, any failure is fatal — the caller propagates the
+/// error, which ultimately aborts sandbox startup.
+fn try_open_path(path: &Path, compatibility: &LandlockCompatibility) -> Result<Option<PathFd>> {
+    match PathFd::new(path) {
+        Ok(fd) => Ok(Some(fd)),
+        Err(err) => {
+            let reason = classify_path_fd_error(&err);
+            let is_not_found = matches!(
+                &err,
+                PathFdError::OpenCall { source, .. }
+                    if source.kind() == std::io::ErrorKind::NotFound
+            );
+            match compatibility {
+                LandlockCompatibility::BestEffort => {
+                    // NotFound is expected for stale baseline paths (e.g.
+                    // /app baked into the server-stored policy but absent
+                    // in this container image).  Log at debug! to avoid
+                    // polluting SSH exec stdout — the pre_exec hook
+                    // inherits the tracing subscriber whose writer targets
+                    // fd 1 (the pipe/PTY).
+                    //
+                    // Other errors (permission denied, symlink loops, etc.)
+                    // are genuinely unexpected and logged at warn!.
+                    if is_not_found {
+                        debug!(
+                            path = %path.display(),
+                            reason,
+                            "Skipping non-existent Landlock path (best-effort mode)"
+                        );
+                    } else {
+                        warn!(
+                            path = %path.display(),
+                            error = %err,
+                            reason,
+                            "Skipping inaccessible Landlock path (best-effort mode)"
+                        );
+                    }
+                    Ok(None)
+                }
+                LandlockCompatibility::HardRequirement => Err(miette::miette!(
+                    "Landlock path unavailable in hard_requirement mode: {} ({}): {}",
+                    path.display(),
+                    reason,
+                    err,
+                )),
+            }
+        }
+    }
+}
+
+/// Classify a [`PathFdError`] into a human-readable reason.
+///
+/// `PathFd::new()` wraps `open(path, O_PATH | O_CLOEXEC)` which can fail for
+/// several reasons beyond simple non-existence. The `PathFdError::OpenCall`
+/// variant wraps the underlying `std::io::Error`.
+fn classify_path_fd_error(err: &PathFdError) -> &'static str {
+    match err {
+        PathFdError::OpenCall { source, .. } => classify_io_error(source),
+        // PathFdError is #[non_exhaustive], handle future variants gracefully.
+        _ => "unexpected error",
+    }
+}
+
+/// Classify a `std::io::Error` into a human-readable reason string.
+fn classify_io_error(err: &std::io::Error) -> &'static str {
+    match err.kind() {
+        std::io::ErrorKind::NotFound => "path does not exist",
+        std::io::ErrorKind::PermissionDenied => "permission denied",
+        _ => match err.raw_os_error() {
+            Some(40) => "too many symlink levels",           // ELOOP
+            Some(36) => "path name too long",                // ENAMETOOLONG
+            Some(20) => "path component is not a directory", // ENOTDIR
+            _ => "unexpected error",
+        },
+    }
+}
+
 fn compat_level(level: &LandlockCompatibility) -> CompatLevel {
     match level {
         LandlockCompatibility::BestEffort => CompatLevel::BestEffort,
         LandlockCompatibility::HardRequirement => CompatLevel::HardRequirement,
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn try_open_path_best_effort_returns_none_for_missing_path() {
+        let result = try_open_path(
+            &PathBuf::from("/nonexistent/openshell/test/path"),
+            &LandlockCompatibility::BestEffort,
+        );
+        assert!(result.is_ok());
+        assert!(result.unwrap().is_none());
+    }
+
+    #[test]
+    fn try_open_path_hard_requirement_errors_for_missing_path() {
+        let result = try_open_path(
+            &PathBuf::from("/nonexistent/openshell/test/path"),
+            &LandlockCompatibility::HardRequirement,
+        );
+        assert!(result.is_err());
+        let err_msg = result.unwrap_err().to_string();
+        assert!(
+            err_msg.contains("hard_requirement"),
+            "error should mention hard_requirement mode: {err_msg}"
+        );
+        assert!(
+            err_msg.contains("does not exist"),
+            "error should include the classified reason: {err_msg}"
+        );
+    }
+
+    #[test]
+    fn try_open_path_succeeds_for_existing_path() {
+        let dir = tempfile::tempdir().unwrap();
+        let result = try_open_path(dir.path(), &LandlockCompatibility::BestEffort);
+        assert!(result.is_ok());
+        assert!(result.unwrap().is_some());
+    }
+
+    #[test]
+    fn classify_not_found() {
+        let err = std::io::Error::from_raw_os_error(libc::ENOENT);
+        assert_eq!(classify_io_error(&err), "path does not exist");
+    }
+
+    #[test]
+    fn classify_permission_denied() {
+        let err = std::io::Error::from_raw_os_error(libc::EACCES);
+        assert_eq!(classify_io_error(&err), "permission denied");
+    }
+
+    #[test]
+    fn classify_symlink_loop() {
+        let err = std::io::Error::from_raw_os_error(libc::ELOOP);
+        assert_eq!(classify_io_error(&err), "too many symlink levels");
+    }
+
+    #[test]
+    fn classify_name_too_long() {
+        let err = std::io::Error::from_raw_os_error(libc::ENAMETOOLONG);
+        assert_eq!(classify_io_error(&err), "path name too long");
+    }
+
+    #[test]
+    fn classify_not_a_directory() {
+        let err = std::io::Error::from_raw_os_error(libc::ENOTDIR);
+        assert_eq!(classify_io_error(&err), "path component is not a directory");
+    }
+
+    #[test]
+    fn classify_unknown_error() {
+        let err = std::io::Error::from_raw_os_error(libc::EIO);
+        assert_eq!(classify_io_error(&err), "unexpected error");
+    }
+
+    #[test]
+    fn classify_path_fd_error_extracts_io_error() {
+        // Use PathFd::new on a non-existent path to get a real PathFdError
+        // (the OpenCall variant is #[non_exhaustive] and can't be constructed directly).
+        let err = PathFd::new("/nonexistent/openshell/classify/test").unwrap_err();
+        assert_eq!(classify_path_fd_error(&err), "path does not exist");
+    }
+}