fix(core): carry the image's declared Env into the OCI bundle - #256
Merged
Conversation
`izba exec <sandbox> -- cat` failed with crun's `not found in $PATH` on
some sandboxes while absolute paths worked. The reported axis (ubuntu vs
alpine) is a red herring: both images ship a byte-identical `PATH` in
identically-shaped OCI configs. The real axis is CACHE VINTAGE.
`ImageStore::is_cached` keys only on `rootfs.erofs`, so an image dir
written by a pre-crun izba -- layers present, `config.json` absent --
counts as fully cached. `load_config` then returns `Ok(None)`, which
`sandbox::start` accepted silently, and `generate_spec`'s
`unwrap_or_default()` turned into an empty image env. The bundle's
`process.env` shipped with no `PATH` at all. crun `clearenv()`s, seeds an
exec from exactly that env, then resolves the binary via
`getenv("PATH")` -- NULL -- so every bare command failed.
izba's design comment at `exec.rs:283` is correct and is preserved: crun
does apply the image's `PATH`, and izba must not guess a default. The
bundle simply had nothing to apply.
- add `ImageStore::is_complete` (rootfs AND config.json) and gate
`ensure_image`'s local-tag fast path on it, so a config-less entry
falls through to the registry path where the cheap self-heal lives
instead of short-circuiting past it.
- `sandbox::start` now fails loudly and actionably via
`require_image_config` instead of silently building an env-less
container. A config that is PRESENT but declares no `Env` stays legal
(`oci-archive:`, `FROM scratch`) -- "we don't know the image's env" is
a defect, "the image declares none" is the truth about that image.
- test fixtures now seed COMPLETE cache entries, the shape `ensure_image`
guarantees; the fixture image declares an implausible `PATH` so a
propagation assertion cannot pass by matching a guessed default.
- integration: `exec_collect_env` lets a test send an EMPTY env like the
real CLI does. The suite injected `PATH=STD_PATH` into every exec and
would have passed even with an env-less container.
- integration: first non-Alpine sandbox in the suite (`ubuntu:24.04`) --
bare `cat`/`sh`/`ls` plus `printenv PATH` compared against the image's
own declared PATH read back from the image store, never a constant.
- ssh: guard that the restricted login shell injects no `PATH` either; it
reaches the workload through the same `crun exec` and the same
container-definition env, so it had the identical exposure.
Refs #222
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T8RtbNTsnQ47n5iH1C8guP
The 14 `daemon::server::tests` that boot a sandbox go through the same `start` path and so hit the new "incomplete cache entry" refusal. Move the fixture publish from `sandbox::tests` into `testutil::test_paths` itself, so every fixture data root holds the COMPLETE entry shape that `ensure_image` guarantees, and drop the now-redundant local shadow. These tests were green locally and red in CI: they runtime-skip when the sandbox denies `bind`, so a sandboxed `cargo test` run silently passes over them. Verified this time by disabling the fixture and watching `start_then_stop_via_mock_driver` fail unsandboxed with the exact CI error, then restoring it. Refs #222 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T8RtbNTsnQ47n5iH1C8guP
…ctive Two P1 review findings, both verified against the code before changing anything. 1. Gating the local-tag fast path on `is_complete` opened an image SUBSTITUTION hole: `image_ref` there is a local tag, so falling through handed that bare name to registry resolution, letting a remote repository of the same name stand in for the locally tagged image -- and breaking the local image outright when no such repository exists. Tag precedence is a trust property and outranks cache repair, so the gate goes back to `is_cached`. A locally built image has no registry to heal from anyway; the config-less case is caught loudly at start. The test that pinned the fall-through is replaced by one pinning precedence (offline, so returning the tagged digest also proves no network substitution was attempted). 2. The refusal told users to `izba rm` and re-create the sandbox. `rm` removes the entire sandbox dir -- rw.img and ephemeral volumes -- so the prescribed fix destroyed data to repair an IMAGE CACHE entry. The message now repairs the cache in place (re-pull via a throwaway create, or `izba build` for a local image) and explicitly warns against rm-ing the sandbox. Asserted by the test, which no longer accepts "mentions izba rm" as evidence of being actionable. Refs #222 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T8RtbNTsnQ47n5iH1C8guP
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Closes #222
The reported axis is a red herring
izba exec <sandbox> -- cat /etc/os-releasefailed with crun'snot found in $PATHwhile absolute paths worked. The issue framed this as ubuntu-vs-alpine. It is not: I pulled both raw config blobs andubuntu:24.04andalpine:3.20declare a byte-identicalPATHin identically-shaped OCI configs (both OCI index -> OCI manifest -> OCI config media types).The real axis is cache vintage. Any image whose cache entry predates crun support breaks this way, on any distro.
Root cause, hop by hop
ImageStore::is_cachedkeys only onrootfs.erofs— it never checksconfig.json.ensure_imageonly runs on the registry path — the local-tag fast pathreturns before it, andizba starton an existing sandbox never callsensure_imageat all.load_config->Ok(None).sandbox::startaccepted that silently (the old comment said so verbatim).generate_spec'scfg.and_then(..).unwrap_or_default()-> empty image env, sooci/config.json'sprocess.envshipped with noPATH.clearenv()s, seeds an exec from exactly that env (merge_env = true), then resolves the binary withgetenv("PATH")— NULL. Hence the error.izba's design comment at
exec.rs:283is correct and is preserved. crun does apply the image'sPATH; izba must not guess a default. The bundle simply had nothing to apply. No defaultPATHis injected anywhere in this PR.Changes
ImageStore::is_complete(rootfs and config.json).ensure_image's local-tag fast path now gates on it, so a config-less entry falls through to the registry path where the cheap self-heal already lives, instead of short-circuiting past it.require_image_config—sandbox::startfails loudly and actionably instead of silently building an env-less container. The distinction that matters: config absent = "we don't know the image's env" (a defect); config present but declaring noEnv= "the image declares none" (oci-archive:,FROM scratch) and is passed through untouched.ensure_imageguarantees. The fixture image declares a deliberately implausiblePATHso a propagation assertion cannot pass by coincidentally matching a guessed default.exec_collect_env— the integration suite injectedPATH=STD_PATHinto every exec and no call site could override it, so it would have passed even if the guest delivered an env-less container. Tests can now send an empty env, like the real CLI does.Acceptance criteria
cat/sh/lssucceed on ubuntu:24.04non_alpine_image_bare_commands_resolve_via_the_images_declared_path(KVM)start_propagates_the_images_declared_path_into_the_bundle) and in-VM (printenv PATH)exec.rs:890still passesubuntu:24.04, the first non-Alpine-family sandbox rootfs in the suiteizba ssh/ restricted login shell checkedcrun exec, same container-definition env — identical exposure, fixed by the same host-side change, now guarded byssh_session_crun_argv_never_injects_a_pathVerification
All six workspace gates green locally:
cargo test --workspace(1382 in izba-core alone, 0 failed),clippy --workspace --all-targets -D warnings,fmt --check, izba-init musl (static-pie confirmed), windows-gnucheck+clippy.The host-side propagation test was verified non-vacuous by sabotage: with the image config forced to
Noneit fails withprocess.envcontaining only the six trust-CA vars and noPATH— a verbatim reproduction of #222.The new integration test is KVM-gated and
e2e.ymldoes not run on PRs, so I am dispatching it manually on this branch; local artifacts here predate the crun/kernel work and would fail for unrelated reasons.Note for review
startnow refuses a sandbox whose image cache entry has noconfig.json. That is deliberate — such a sandbox is already broken (every bare command fails) — but it does turn a silent degradation into a hard error at start, so it is the judgement call most worth a second opinion. The alternative considered was self-healing at start via a manifest fetch, rejected because it would makeizba startnetwork-dependent.🤖 Generated with Claude Code
Greptile Summary
This revision carries the image’s declared environment into generated OCI bundles and rejects legacy cache entries whose image configuration is unavailable.
Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
is_completeto distinguish rootfs-only legacy entries from cache entries containing runtime configuration.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Start sandbox] --> B[Load cached image config] B -->|Present| C[Generate OCI bundle] C --> D[Copy image Env into process.env] D --> E[crun exec resolves bare commands using image PATH] B -->|Missing| F[Fail startup with cache-repair guidance]Reviews (3): Last reviewed commit: "fix(core): keep local-tag precedence and..." | Re-trigger Greptile
Context used (3)