Skip to content

Commit f2cb58c

Browse files
test(chroot_jail): cover the chroot-jail subsystem (#29)
1 parent b40879d commit f2cb58c

12 files changed

Lines changed: 737 additions & 54 deletions

File tree

.github/workflows/ci.yml

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -229,16 +229,18 @@ jobs:
229229
with:
230230
tool: nextest
231231

232-
# The feature-gated E2E suites self-skip without root. Run ONLY these two as
233-
# root (via sudo -E so the runner's cargo/rustup home and PATH are kept) so
234-
# the root-only cases -- sys-test ok, fake-bin argv capture -- actually run.
235-
# The rest of the suite (owner-axis tests assume a non-root uid) stays in the
236-
# non-root `rust` job above.
237-
- name: Gated E2E suites as root
232+
# Root-gated cases are tagged by name: every test that needs root ends in
233+
# `_as_root` and self-skips when run unprivileged. We select them by that
234+
# suffix (via sudo -E so the runner's cargo/rustup home and PATH are kept)
235+
# so the root-only paths -- sys-test ok, fake-bin argv capture, mknod/chown
236+
# jail build -- actually run. A new root test is picked up automatically by
237+
# following the naming convention; no test-binary enumeration to maintain.
238+
# The rest of the suite (owner-axis tests assume a non-root uid) stays in
239+
# the non-root `rust` job above.
240+
- name: Gated tests as root
238241
run: |
239242
sudo -E env "PATH=$PATH" \
240-
cargo nextest run --features insecure_test_seams \
241-
--test e2e_config --test e2e_argv
243+
cargo nextest run --features insecure_test_seams -E 'test(/_as_root$/)'
242244
243245
# Uploads the triggering event payload so the workflow_run publisher
244246
# (test-results.yml) can map results back to the originating PR -- required

AGENTS.md

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -132,13 +132,25 @@ Coverage is a side effect of good tests, never the target.
132132

133133
## Other conventions
134134

135-
- **Comments earn their place.** No section-divider banners (`# --- foo ---`),
136-
no comment that just restates what the next line does. A comment explains a
137-
non-obvious *why* a reader cannot recover from the code — a workaround, an
138-
invariant, a deliberate trade-off. Prefer self-documenting code: a named
139-
function, a `Unit.*` quantity instead of a bare `1024 * 1024`, a descriptive
140-
variable — over a comment narrating the mechanics. If you reach for a comment
141-
to explain *what*, rename the thing instead.
135+
- **NEVER write section-divider banners** — comments like `// --- foo ---`,
136+
`# === bar ===`, `// ---- Tests ----`, or any comment whose job is to label a
137+
region of a file. They are a code smell: reaching for one is a signal that
138+
the file is doing too many things at once. When you feel the urge, do NOT
139+
write the banner — make the underlying decision instead:
140+
1. **Split it out.** If the regions are genuinely distinct
141+
responsibilities, they belong in separate modules/files (or, for tests,
142+
separate test files or submodules). Extract them.
143+
2. **Or drop the comment entirely.** If the code is already cohesive, the
144+
banner adds nothing a reader can't see from the names — delete it. A
145+
blank line is sufficient separation.
146+
There is no third option where the banner stays.
147+
- **Comments earn their place.** No comment that just restates what the next
148+
line does. A comment explains a non-obvious *why* a reader cannot recover
149+
from the code — a workaround, an invariant, a deliberate trade-off. Prefer
150+
self-documenting code: a named function, a `Unit.*` quantity instead of a
151+
bare `1024 * 1024`, a descriptive variable — over a comment narrating the
152+
mechanics. If you reach for a comment to explain *what*, rename the thing
153+
instead.
142154
- Don't hand-roll magic numbers for sizes/durations/bandwidth: use the
143155
`Unit.*` types (`Unit.Information.mib(8)`, not `8 * 1024 * 1024`) and
144156
`use Unit.Operators` for unit-aware arithmetic.

native/suidhelper/Cargo.toml

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,14 @@ path = "tests/tools/dmsetup_parsers.rs"
4646
name = "util_confinement"
4747
path = "tests/util/confinement.rs"
4848

49+
[[test]]
50+
name = "tools_chroot_jail_remove"
51+
path = "tests/tools/chroot_jail_remove.rs"
52+
53+
[[test]]
54+
name = "util_chroot_jail"
55+
path = "tests/util/chroot_jail.rs"
56+
4957
[[test]]
5058
name = "e2e_config"
5159
path = "tests/e2e/config.rs"
@@ -54,6 +62,10 @@ path = "tests/e2e/config.rs"
5462
name = "e2e_argv"
5563
path = "tests/e2e/argv.rs"
5664

65+
[[test]]
66+
name = "e2e_chroot_jail"
67+
path = "tests/e2e/chroot_jail.rs"
68+
5769
[dependencies]
5870
clap = { version = "4", features = ["derive"] }
5971
nix = { version = "0.29", features = ["user", "fs", "dir"] }

native/suidhelper/src/tools/chroot_jail/mod.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
//! `chroot-jail`: per-VM chroot/jail lifecycle.
33
44
mod prepare;
5-
mod remove;
5+
pub mod remove;
66

77
pub use prepare::PrepareArgs;
88
pub use remove::RemoveArgs;

native/suidhelper/src/tools/chroot_jail/remove.rs

Lines changed: 13 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -74,8 +74,8 @@ impl IsTool for Remove {
7474
type RunT = Result<(), Error>;
7575

7676
fn run_privileged(&self) -> Self::RunT {
77-
remove_chroot(&self.args.chroot)?;
78-
remove_cgroup(&self.args.cgroup)?;
77+
remove_chroot_under(&Config::get().jail_base(), &self.args.chroot)?;
78+
remove_cgroup_under(Path::new(CGROUP_BASE), &self.args.cgroup)?;
7979
Ok(())
8080
}
8181

@@ -85,18 +85,17 @@ impl IsTool for Remove {
8585
}
8686
}
8787

88-
/// Recursively remove the per-VM chroot `<JAIL_BASE>/<exec>/<id>`, fd-relative
89-
/// after an `O_NOFOLLOW` walk from `JAIL_BASE`. Idempotent on a missing target.
90-
fn remove_chroot(chroot: &Path) -> Result<(), Error> {
91-
let jail_base = Config::get().jail_base();
88+
/// Recursively remove the per-VM chroot `<jail_base>/<exec>/<id>`, fd-relative
89+
/// after an `O_NOFOLLOW` walk from `jail_base`. Idempotent on a missing target.
90+
/// `--chroot` must be exactly `<exec>/<id>` (one parent component, one leaf).
91+
pub fn remove_chroot_under(jail_base: &Path, chroot: &Path) -> Result<(), Error> {
9292
let path: LexicalPath = chroot.to_path_buf().try_into().map_err(Error::ChrootPath)?;
93-
let (parents, leaf) = path.relative_to(&jail_base).map_err(Error::ChrootPath)?;
94-
// Exactly <exec>/<id>: one parent component, one leaf.
93+
let (parents, leaf) = path.relative_to(jail_base).map_err(Error::ChrootPath)?;
9594
if parents.len() != 1 {
9695
return Err(Error::ChrootDepth(chroot.to_path_buf()));
9796
}
9897

99-
let Some(parent) = walk(jail_base, &parents)? else {
98+
let Some(parent) = walk(jail_base.to_path_buf(), &parents)? else {
10099
return Ok(()); // an ancestor is already gone
101100
};
102101
match parent.remove_dir_all(&leaf) {
@@ -107,17 +106,16 @@ fn remove_chroot(chroot: &Path) -> Result<(), Error> {
107106
}
108107

109108
/// Remove the (empty) per-VM cgroup leaf, fd-relative after an `O_NOFOLLOW` walk
110-
/// from `/sys/fs/cgroup`. Idempotent on `ENOENT`/`ENOTEMPTY`.
111-
fn remove_cgroup(cgroup: &Path) -> Result<(), Error> {
112-
let base = PathBuf::from(CGROUP_BASE);
109+
/// from `base`. `--cgroup` must be at least two components below `base` (one or
110+
/// more parents, plus the leaf). Idempotent on `ENOENT`/`ENOTEMPTY`.
111+
pub fn remove_cgroup_under(base: &Path, cgroup: &Path) -> Result<(), Error> {
113112
let path: LexicalPath = cgroup.to_path_buf().try_into().map_err(Error::CgroupPath)?;
114-
let (parents, leaf) = path.relative_to(&base).map_err(Error::CgroupPath)?;
115-
// At least two components below the base: one or more parents, plus the leaf.
113+
let (parents, leaf) = path.relative_to(base).map_err(Error::CgroupPath)?;
116114
if parents.is_empty() {
117115
return Err(Error::CgroupDepth(cgroup.to_path_buf()));
118116
}
119117

120-
let Some(parent) = walk(base, &parents)? else {
118+
let Some(parent) = walk(base.to_path_buf(), &parents)? else {
121119
return Ok(()); // an ancestor is already gone
122120
};
123121
match parent.rmdir(&leaf) {

native/suidhelper/src/tools/mod.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
//! privilege boundary.
55
66
mod blockdev;
7-
mod chroot_jail;
7+
pub mod chroot_jail;
88
mod dmsetup;
99
mod losetup;
1010

native/suidhelper/src/util/chroot_jail.rs

Lines changed: 27 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ pub enum Error {
3434
source: io::Error,
3535
},
3636
#[error("kernel source must be under {}: {path}", .base.display())]
37-
OutsideBase { base: &'static Path, path: PathBuf },
37+
OutsideBase { base: PathBuf, path: PathBuf },
3838
#[error("open kernel source {path}: {source}")]
3939
OpenSrc {
4040
path: PathBuf,
@@ -103,41 +103,53 @@ impl<K> ChrootJail<K, Unset> {
103103
}
104104

105105
impl ChrootJail<Kernel, Rootfs> {
106-
/// Realize the jail on disk: confined walk to the chroot dir, then stage the
107-
/// kernel and create the rootfs node relative to that fd.
106+
/// Realize the jail on disk under the configured `JAIL_BASE`/`HYPER_BASE`.
108107
pub fn build(self) -> Result<(), Error> {
109-
let chroot = open_chroot(&self.chroot)?;
110-
stage_kernel(&chroot, &self.kernel.0, self.uid, self.gid)?;
108+
let cfg = Config::get();
109+
self.build_under(&cfg.jail_base(), cfg.hyper_base())
110+
}
111+
112+
/// Realize the jail relative to explicit base directories: confined walk to
113+
/// the chroot dir, then stage the kernel and create the rootfs node relative
114+
/// to that fd. The public [`build`](Self::build) wires `Config` into this.
115+
pub fn build_under(self, jail_base: &Path, hyper_base: &Path) -> Result<(), Error> {
116+
let chroot = open_chroot_under(jail_base, &self.chroot)?;
117+
stage_kernel_under(&chroot, &self.kernel.0, hyper_base, self.uid, self.gid)?;
111118
make_rootfs(&chroot, &self.rootfs.0, self.uid, self.gid)?;
112119
Ok(())
113120
}
114121
}
115122

116-
/// Open the chroot directory by walking it from `JAIL_BASE` with `O_NOFOLLOW`, so
123+
/// Open the chroot directory by walking it from `jail_base` with `O_NOFOLLOW`, so
117124
/// a symlinked component cannot redirect outside the jail.
118-
fn open_chroot(chroot: &Path) -> Result<SafeDir, Error> {
119-
let jail_base = Config::get().jail_base();
125+
pub fn open_chroot_under(jail_base: &Path, chroot: &Path) -> Result<SafeDir, Error> {
120126
let path: SafePath<IsAbsolute, StrictComponents> = chroot.to_path_buf().try_into()?;
121-
let (parents, leaf) = path.relative_to(&jail_base)?;
122-
let anchor: SafePath<IsAbsolute, StrictComponents> = jail_base.try_into()?;
127+
let (parents, leaf) = path.relative_to(jail_base)?;
128+
let anchor: SafePath<IsAbsolute, StrictComponents> = jail_base.to_path_buf().try_into()?;
123129

124130
let mut components = parents;
125131
components.push(leaf);
126132
Ok(SafeDir::open(&anchor)?.descend(&components)?)
127133
}
128134

129135
/// Stage host file `src` into `chroot` as `vmlinux`: confine the source under
130-
/// `HYPER_BASE`, hard-link it (copy across filesystems), then chown to `uid:gid`.
131-
fn stage_kernel(chroot: &SafeDir, src: &Path, uid: u32, gid: u32) -> Result<(), Error> {
136+
/// `hyper_base` (after canonicalization), hard-link it (copy across filesystems),
137+
/// then chown to `uid:gid`.
138+
pub fn stage_kernel_under(
139+
chroot: &SafeDir,
140+
src: &Path,
141+
hyper_base: &Path,
142+
uid: u32,
143+
gid: u32,
144+
) -> Result<(), Error> {
132145
let kernel = Path::new(KERNEL_NAME);
133146
let src_canon = std::fs::canonicalize(src).map_err(|source| Error::Source {
134147
path: src.to_path_buf(),
135148
source,
136149
})?;
137-
let base = Config::get().hyper_base();
138-
if !src_canon.starts_with(base) {
150+
if !src_canon.starts_with(hyper_base) {
139151
return Err(Error::OutsideBase {
140-
base,
152+
base: hyper_base.to_path_buf(),
141153
path: src_canon,
142154
});
143155
}

native/suidhelper/tests/e2e/argv.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ fn recorded_argv(record: &Path) -> Vec<String> {
5454
}
5555

5656
#[test]
57-
fn dmsetup_create_snapshot_reconstructs_canonical_table() {
57+
fn dmsetup_create_snapshot_reconstructs_canonical_table_as_root() {
5858
if !is_root() {
5959
eprintln!("SKIP dmsetup_create: needs root to acquire + own the fake bin");
6060
return;
@@ -100,7 +100,7 @@ fn dmsetup_create_snapshot_reconstructs_canonical_table() {
100100
}
101101

102102
#[test]
103-
fn dmsetup_remove_retry_toggle() {
103+
fn dmsetup_remove_retry_toggle_as_root() {
104104
if !is_root() {
105105
eprintln!("SKIP dmsetup_remove: needs root");
106106
return;
@@ -126,7 +126,7 @@ fn dmsetup_remove_retry_toggle() {
126126
}
127127

128128
#[test]
129-
fn dmsetup_message_create_thin() {
129+
fn dmsetup_message_create_thin_as_root() {
130130
if !is_root() {
131131
eprintln!("SKIP dmsetup_message: needs root");
132132
return;
@@ -157,7 +157,7 @@ fn dmsetup_message_create_thin() {
157157
}
158158

159159
#[test]
160-
fn blockdev_getsz_argv_and_parse() {
160+
fn blockdev_getsz_argv_and_parse_as_root() {
161161
if !is_root() {
162162
eprintln!("SKIP blockdev: needs root");
163163
return;

0 commit comments

Comments
 (0)