feat(box): add privileged plumbing for DinD - #646
Conversation
b928ed3 to
c581579
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesPrivileged container specification
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CapabilitySet
participant create_oci_spec
participant build_linux_spec
CapabilitySet->>create_oci_spec: is_privileged()
create_oci_spec->>build_linux_spec: privileged flag
build_linux_spec-->>create_oci_spec: readonly and masked paths
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
c581579 to
8712206
Compare
ae4a493 to
542c814
Compare
📦 BoxLite review — couldn't completepowered by BoxLite |
542c814 to
75a0811
Compare
49babff to
c8c4a10
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/guest/src/container/spec.rs`:
- Around line 170-173: Update the container spec flow around `privileged` and
`build_linux_spec` to pass privilege state into mount generation, and when
enabled create an isolated cgroup namespace/hierarchy before mounting it
read-write; keep the guest-wide hierarchy inaccessible. Also revise the adjacent
comment to describe `is_privileged()` accurately rather than implying
CAP_SYS_ADMIN alone is sufficient.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 005eff7d-cd06-4cd6-af93-7bbe65fc3ba8
📒 Files selected for processing (2)
src/guest/src/container/capabilities.rssrc/guest/src/container/spec.rs
c4b5ed3 to
38402cb
Compare
DorianZheng
left a comment
There was a problem hiding this comment.
the wiring is small, but is_privileged() turns an existing capability flag into a sandbox switch, and a few things outside the diff now hang off it.
guest version gate not bumped — MIN_CAPABILITY_GUEST_VERSION (src/boxlite/src/litebox/init/tasks/guest_init.rs:23) is still (0, 9, 8), but add=["ALL"] now means two different things depending on which guest you land on. guest rootfs images are cached per version and reused — the constant's own doc says exactly that — so a host built after this change can boot an old cached 0.9.8 guest, pass the version check, get 41 caps with /proc/sys still read-only, and dockerd dies on ip_forward with nothing pointing at the cause.
no privileged flag anywhere — ContainerCapabilities is add/drop only at every layer (cli.rs:473, advanced_options.rs:574, the proto). so you can't ask for a writable /proc/sys without also taking all 41 caps, and you can't take all 41 without the wider sandbox. docs/reference/cli/README.md:625 still describes --cap-add as capabilities only. and config.json is rebuilt on every start (lifecycle.rs:180 → start.rs:126), so a box created with --cap-add ALL before this change gets a writable /proc/sys on its next restart after upgrade — no flag touched, no code changed on their side.
no opt-in gate — the repo already has one for this shape of change: ExperimentalFeature::{CustomKernel, NestedVirtualization} behind BOXLITE_EXPERIMENTAL (experimental.rs:16-24). nested virt only hands the workload a /dev/kvm node inside its own VM and is gated. this opens the guest's sysctl tree and isn't.
the capability table is now load-bearing — CAPABILITIES_BY_NUMBER is [Capability; 41], and supported_capabilities() does .take(cap_last_cap + 1) with no bound check (capabilities.rs:232-236). on a future kernel with cap_last_cap > 40 it silently truncates: 41 ⊆ 41, so the box is labelled fully privileged while genuinely missing the newest caps — and ensure_supported rejects those same caps with "is not supported by the guest kernel". it used to be an input-validation aid; it now decides a sandbox.
design doc — docs/architecture/container-capabilities.md:49 (status: accepted) says downstream code "cannot reinterpret the policy", which is what create_oci_spec now does. its user-facing advice to prefer drop=["ALL"] plus the minimum additions is also now the recipe that turns DinD off.
description is stale — it says --cap-add SYS_ADMIN counts as privileged (the code deliberately rejects that), that a writable cgroup2 mount at /sys/fs/cgroup is added (still commented out at spec.rs:644), and that readonlyPaths/maskedPaths are cleared (one entry is dropped, masked paths are untouched). the commit message gets all three right. it also still says "remains draft" but the PR isn't.
unrelated hunk — src/deps/libkrun-sys/build.rs:476 is formatting only.
38402cb to
d49e642
Compare
|
All nine addressed. The shape changed as a result, so it is easier to describe what the PR does now than to answer point by point. Privileged now clears both lists, matching I tried to make them enforceable instead. A user namespace is the mechanism that would do it, and the uid/gid mappings are already built here — only the libcontainer routes a user namespace through the systemd cgroup manager, and #9 stands and I could not fix it. It is the whole #1, #2 — #3, #4 — both local lists are gone. Nothing consults them now, and as you noted the copy had already drifted from what #5, #6, #7 — the tautological assert is gone. The gap you named in #6 is the one that mattered: Verified end to end after the rework: Docker 29.6.2 in a box on this build pulls images and runs both single containers and a two-service compose project with working service DNS. |
d49e642 to
b5d605a
Compare
|
Correction to my previous comment: I wrote that nothing filters There is a seccomp filter — The container spec is the part with no seccomp: So the conclusion holds, but the reasoning I gave for it was sloppy about which layer is protected. The commit message now states it precisely. |
b5d605a to
d1f9396
Compare
a6509dd to
85847d4
Compare
85847d4 to
d06bd67
Compare
cbce52d to
c4d0e63
Compare
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
apps/api, apps/runner, apps/libs (generated clients), and sdks/** now ship in boxlite-ai#1156 instead. This PR is left with the guest + core runtime + CLI + REST contract — the actual privileged/ DinD feature — so review stays scoped to the security-relevant parts of the change instead of being split across 84 files of mixed risk. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ResolvedSecurityPolicy::from_resolved no longer validates a canonical privileged shape, so a test asserting that mismatched atomic options and capabilities still resolve is testing a trivial pass-through, not guest behavior. resolved_policy_consumes_atomic_security_options and all_capabilities_without_privileged_keep_proc_sys_readonly already cover the same constructor with both all-true and all-false inputs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
cgroup_namespace and allow_all_devices are unnecessary for DinD: the
guest never enforced a restrictive device-cgroup default, and dockerd
tolerates running without a private cgroup namespace view. Only
unconfined_paths (masked/readonly path relief, for network sysctls)
and writable_sysfs (/sys stays writable, for dockerd's own cgroup
writes) are load-bearing.
ResolvedContainerSecurityConfig / ContainerAdvancedConfig /
ResolvedSecurityPolicy drop from 4 fields to 2. service.proto
renumbers ContainerAdvancedOptions to match (internal wire message,
no external consumer pinned to the old field numbers).
Before
resolve_container_security (Core · advanced_options.rs:789)
-> ResolvedContainerSecurityConfig{cgroup_namespace, writable_sysfs, allow_all_devices, unconfined_paths}
-> create_oci_spec (Guest · spec.rs:153)
|- build_default_namespaces(cgroup_namespace) — adds a cgroup namespace
|- build_linux_spec(..., allow_all_devices, ...) — builds an allow-all device-cgroup rule
`- build_standard_mounts(..., writable_sysfs)
After
resolve_container_security (Core · advanced_options.rs:789)
-> ResolvedContainerSecurityConfig{unconfined_paths, writable_sysfs}
-> create_oci_spec (Guest · spec.rs:153)
|- build_default_namespaces() — fixed list, no cgroup namespace
|- build_linux_spec(..., unconfined_paths) — no device-cgroup rule
`- build_standard_mounts(..., writable_sysfs)
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
architecture/README.md, container-capabilities.md, cli/README.md, and rust/README.md described privileged mode with a dead link and with the cgroup namespace / allow-all device rule this PR's own last commit removed. Reverting to main here rather than patching them in place: the accurate design record already lives outside the repo, and this PR's diff should stay code, not docs that immediately went stale. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
create_inner's own normalize_privileged() call always ran on data
sanitize_local_options had already normalized moments earlier in the
same call: RuntimeImpl is never re-exported outside the crate, and
core.rs's only construction site wraps it in LocalRuntime immediately,
so every reachable caller goes through sanitize_local_options first.
Replaced the redundant call with a debug_assert documenting that
invariant.
get_or_create_rejects_privileged_upgrade called RuntimeImpl directly
with a hand-built AdvancedBoxOptions{privileged: true, ..} literal,
bypassing that pipeline the way no real caller does; updated it to use
set_privileged, which is what actually produces the normalized shape.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ies verbatim
Host resolve_container_security() now sends literal masked_paths,
readonly_paths, and /sys mount options instead of two booleans; the guest
assigns them as-is with no reinterpretation. Matches how Docker, Podman, and
Kata Containers hand the enforcing side a finished OCI shape rather than a
flag to re-derive. capabilities stays as add/drop deltas — the guest is the
only side that knows its own kernel's capability ceiling.
Before: host sends {capabilities, unconfined_paths: bool, writable_sysfs: bool}
→ guest capabilities.rs::from_resolved(bool, bool)
→ spec.rs::build_linux_spec(.., unconfined_paths: bool) branches on it
→ spec.rs::build_standard_mounts(.., writable_sysfs: bool) branches on it
After: host resolve_container_security() resolves literal values
→ ContainerAdvancedOptions{masked_paths, readonly_paths, sys_mount_options}
→ guest capabilities.rs::from_resolved(Vec<String>, Vec<String>, Vec<String>)
→ spec.rs::build_linux_spec(.., masked_paths, readonly_paths) assigns verbatim
→ spec.rs::build_standard_mounts(.., sys_mount_options) assigns verbatim
Moved the privileged-vs-hardened test coverage to where the decision is now
made (advanced_options.rs); guest-side tests now assert pass-through fidelity.
…ollow-up PR Keeps boxlite-ai#646 to the mechanism itself — proto, host resolve, guest apply — same split rationale boxlite-ai#1156 already used to separate control-plane/runner/SDK consumers from boxlite-ai#646. The Rust API (AdvancedBoxOptions.privileged, set_privileged) stays; only the CLI --privileged flag and the self-hosted REST DTO/OpenAPI exposure move out, since neither adds mechanism, just a caller. Before: CLI/REST call AdvancedBoxOptions.privileged directly, bundled with the guest/core mechanism in one PR. After: CLI/REST removed here, follow-up PR re-adds them on top of this mechanism-only branch (rust API entry point unchanged, so the follow-up is pure plumbing with nothing left to test beyond wiring).
advanced_options.rs hand-copied oci-spec's default masked/readonly path lists rather than calling the crate's own public functions for them, even though the host crate already depends on oci-spec (runtime feature, on by default). Two independent copies of the same 10+5-entry list, one of which could silently drift on an oci-spec bump. Before: default_masked_paths() -> literal 10-entry Vec<String> default_readonly_paths() -> literal 5-entry Vec<String> After: default_masked_paths() -> oci_spec::runtime::get_default_maskedpaths() default_readonly_paths() -> oci_spec::runtime::get_default_readonly_paths()
4d27ef9 to
1eb8b2a
Compare
…ed REST Adds the CLI --privileged flag and the self-hosted REST/OpenAPI surface for the AdvancedBoxOptions.privileged mechanism feat/dind-privileged-plumbing (boxlite-ai#646) adds. Pure consumer: no new mechanism, just wiring a caller onto the Rust API's set_privileged/AdvancedBoxOptions.privileged that already exists on this branch. Before: --privileged / the REST body field don't exist; only the Rust API (AdvancedBoxOptions.privileged, set_privileged) can request privileged mode. After: cli.rs/commands::create::run parse --privileged and call set_privileged(true) -> AdvancedBoxOptions.privileged rest/types.rs deserializes the same field from the REST request body openapi/box.openapi.yaml documents both the request field and the capabilities.privileged_enabled response flag
Add Docker-style
advanced.privilegedacross the Rust runtime, REST API, runner, CLI, and SDKs.privileged=trueis normalized tocapabilities.add=["ALL"]with an empty drop list and removes only/proc/sysfrom the guest readonly paths; capability add/drop remains a separate shape.Test plan:
cargo fmt --all -- --checkcargo check -p boxlite-guest --target aarch64-unknown-linux-musl --testscap_add=ALLbehaviorapps/e2e/cases/test_privileged_options.pyon the labeled Linux E2E runnerThe Linux E2E workflow remains label-gated because it requires nested KVM. Privileged mode does not add a cgroup mount or clear masked paths.