fix(api): route slot image reads through the rootful podman seam - #1937
fix(api): route slot image reads through the rootful podman seam#1937thinmintdev wants to merge 5 commits into
Conversation
`ContainerProvider.image_present` / `running_image` / `running_argv` shelled out to a bare `podman` as the unprivileged `hal0` service user, which has no subuid ranges, so they read hal0-api's own ROOTLESS image store. Slots run ROOTFUL podman (Quadlet units under /etc/containers/systemd/, root's store), so that store by construction never holds a slot image. On every standard install `GET /api/slots` therefore reported `image_status: "missing"` for every running, healthy slot, `actual_image` was always null, and the #663 image-drift detector could never fire. The `hal0-podman-ro` seam and its `is_hal0_service_user()` gate already existed (cbc8e94 wired them into /api/system-info) but exposed only the argument-free `images` verb, so the three call sites that need a *specific* image ref or container name could not use it. This adds three argument-taking read verbs, with the argument validated on the ROOT side of the privilege boundary before exec — the same posture hal0-systemctl's slot/agent verbs have had since P3-perms: * `image-exists <ref>` — ref matched against the OCI reference grammar (optional host[:port]/, path segments with single ._- separators, optional :tag, optional @sha256:<64 hex>) and length-capped at 512. A validated ref cannot contain whitespace, a shell metacharacter, a leading '-', '..', or a second argv word. * `container-image <slot-token>` / `container-argv <slot-token>` — these take the slot's bare instance token (the hal0-systemctl validate_slot_id charset), NOT a container name: `hal0-slot-<token>` is assembled root-side, so the caller can only ever address a hal0 slot container. Every podman subcommand, flag and --format string remains a literal in the wrapper; no shell is evaluated; rm/run/build/exec/pull are still unreachable. Two side-effect-free validator probes (`check-image-ref`, `check-slot-token`) exist so the regexes can be exercised without podman, root, or a provisioned box. A documented exit-code contract (0 = podman answered, 64 = rejected, 65 = no podman) lets the Python side distinguish "the seam did not answer" from "the answer is negative" — the named-object reads deliberately do NOT silently fall back to the rootless store, because a rootless answer about a named image is not a stale answer, it is an answer about a different object. podman_introspect mirrors both regexes so the unprivileged side fails fast instead of burning a sudo round-trip; a shell test asserts the mirrors and the wrapper agree. podman-ro also gains a `help` probe in preflight_seams and seam_check, since a silently-missing grant is no longer cosmetic. Refs #1889
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a06f74e7e0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # false reject on a legitimate ref; a false reject is precisely the failure | ||
| # mode #1889 is about (it degrades to image_status="missing" again). | ||
| _REF_HOST='[A-Za-z0-9]+([.-][A-Za-z0-9]+)*(:[0-9]{1,5})?' | ||
| _REF_PATH='[A-Za-z0-9]+([._-][A-Za-z0-9]+)*(/[A-Za-z0-9]+([._-][A-Za-z0-9]+)*)*' |
There was a problem hiding this comment.
The repository-component regex rejects valid container image names containing a double underscore or a run of hyphens, such as registry.example/team/model__gpu:v1 or team/model--gpu:v1; the standard distribution-reference grammar permits __ and one-or-more - as separators. On an installed box, these references are rejected before the rootful seam runs, so image_exists() returns None and ContainerProvider.image_present() falls back to the unrelated rootless store, reporting a rootful image as missing and recreating #1889 for valid custom images. The Python mirror and tests should be updated with the wrapper regex.
Useful? React with 👍 / 👎.
| run_podman image inspect --format '{{.Id}}' -- "$1" | ||
| if (( PODMAN_RC == 0 )); then echo present; else echo missing; fi |
There was a problem hiding this comment.
When rootful Podman returns any nonzero status—not only when the image is absent, but also for storage corruption, locking, permission, or runtime failures—this branch prints missing and lets the wrapper exit 0. image_exists() consequently treats an operational failure as an authoritative negative, so the slot API reports a locally present image as missing instead of degrading to an unknown/fallback result. Only the documented not-found outcome should produce missing; other Podman failures need to propagate as a nonzero seam result.
Useful? React with 👍 / 👎.
| seam_ref = podman_introspect.container_image(token) | ||
| if seam_ref: | ||
| return seam_ref |
There was a problem hiding this comment.
For a slot declared with a valid shorthand or tagless image such as alpine:latest or ghcr.io/team/model, Podman commonly returns a canonical ImageName such as docker.io/library/alpine:latest or one with the implicit :latest tag. Returning that value directly makes the existing literal _image_mismatch() comparison mark an unchanged running slot as drifted now that this commit finally makes running_image() reachable on production boxes. Normalize equivalent references before exposing the value to the drift comparison, or compare immutable image identity instead.
Useful? React with 👍 / 👎.
Three P2 findings, all real — each one would have re-created #1889 for a subset of slots rather than fixing it. 1. The image-ref regex rejected valid repository names. The distribution reference grammar's separator set is `"." | "_" | "__" | "-"+`, so `team/model__gpu` and `team/model--gpu` are legal; the regex accepted only single `.`/`_`/`-`. A ref this wrongly rejects never reaches the rootful seam, so image_present falls back to the unrelated rootless store and the image reports missing — exactly the bug being fixed. Both the wrapper and the Python mirror now use the grammar's separator set (with `__` ordered first in the Python copy, whose `re` is leftmost-first rather than POSIX leftmost-longest). 2. An operational podman failure was reported as a negative answer. `podman inspect` collapses "not found" and store-corruption/lock/permission failures into a single rc 125, so the wrapper printed `missing` and exited 0 for both, and image_exists() treated a broken store as authoritative. The presence probes now use `podman image exists` / `podman container exists`, whose contract is rc 0 = yes, rc 1 = no, anything else = error, and the wrapper grew rc 66 for "podman ran but failed" — which the Python side already reads as "the seam did not answer", degrading rather than lying. The container verbs do both podman calls inside one sudo hop. 3. Drift comparison would have cried wolf on every correctly running slot. podman reports a canonical `docker.io/library/alpine:latest` while hal0 profiles declare the shorthand an operator types, and `_image_mismatch` was a literal string compare. That was harmless only because running_image returned None on every deployed box; this branch makes it reachable, which would have turned an inert detector into a lying one. Both sides are now canonicalised (implicit `docker.io/`+`library/` registry, implicit `:latest`) before comparison, splitting the tag on the last colon after the last slash so a registry port is not amputated. Digest-vs-tag is compared on the repository alone: resolving whether a tag and a digest name the same image needs a registry round-trip this hot path will never make, and guessing "drifted" is the cry-wolf failure #663 forbids. Refs #1889
Codex review triage — all three P2s accepted and fixed in 62b7d78Each finding was real, and each would have re-created #1889 for a subset of slots rather than fixing it. None were dismissed. 1. 2. podman failure reported as a negative — CONFIRMED, fixed. 3. Drift would cry wolf on every correctly running slot — CONFIRMED, fixed. Good catch on the second-order effect: VerificationNew coverage: the three previously-rejected refs, rc 66 → Still not for automerge — SUDOERS + INSTALLER change, operator review required. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62b7d7867d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| has_registry = bool(slash) and ("." in first or ":" in first or first == "localhost") | ||
| if not has_registry: | ||
| name = f"docker.io/library/{name}" if not slash else f"docker.io/{name}" |
There was a problem hiding this comment.
When a profile explicitly declares a single-component Docker Hub path such as docker.io/alpine, this branch treats the registry-qualified name as complete and produces docker.io/alpine:latest; Podman normalizes that reference to docker.io/library/alpine:latest, so _image_mismatch() reports drift for an unchanged container. Add the library/ namespace when the registry is Docker Hub and the repository portion has only one component.
Useful? React with 👍 / 👎.
| seam_answer = podman_introspect.image_exists(image) | ||
| if seam_answer is not None: |
There was a problem hiding this comment.
When the API is running as hal0 and the rootful seam fails, image_exists() returns None, causing this method to fall through to the unrelated rootless store and usually return False. Thus a missing grant, validation rejection, or operational Podman failure still becomes image_status="missing" for a potentially present rootful image. Fresh evidence is that the new wrapper now correctly maps operational failures to rc 66/None, but this fallback immediately collapses that distinction; only dev/CI calls outside the service account should use the rootless fallback.
Useful? React with 👍 / 👎.
…issing" Second Codex round, two more P2s, both real. 1. The rootless fallback undid the rc-66 distinction the previous commit introduced. On a provisioned box the rootless store definitionally cannot hold a slot image, so falling through to it after a seam failure turned "podman is broken" / "the grant is missing" / "the ref was rejected" back into an authoritative-looking `image_status: "missing"`. The fallback is now gated on `is_hal0_service_user()` — it runs only off the service account, where the operator's own store IS the store slots use — and the seam-failed-as-hal0 path logs a warning so an operator gets a diagnosable signal instead of a silent wrong answer. `running_image` / `running_argv` are gated the same way; there the answer was already None either way, so this only drops a wasted spawn from the status hot path and removes the chance of answering about a same-named container that is not this slot's. `image_present` still has to answer False there: its contract is a bool and `image_status` has no "unknown" member. Surfacing a distinct unknown state is an API-schema change, deliberately left out of scope. 2. `canonical_image_ref` missed Docker Hub's implicit `library/` namespace for an explicitly-qualified `docker.io/alpine`, which podman reports as `docker.io/library/alpine` — so an unchanged slot declared that way would have been flagged as drifted. The namespace rule now applies to the repository portion however the registry got there, after the registry rule rather than inside it. Refs #1889
Codex round 2 — both P2s accepted and fixed in 6d6c4de4. Rootless fallback collapsed the rc-66 distinction — CONFIRMED, fixed. This was the sharpest finding on the PR: the previous commit added rc 66 precisely so an operational podman failure would not read as "missing", and then the fallback in The fallback is now gated on One honest limitation, called out rather than papered over: 5. Docker Hub's implicit VerificationNew coverage: Still not for automerge — SUDOERS + INSTALLER change, operator review required. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d6c4decff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # rootless one and #1889 comes straight back for that image. `model__gpu` and | ||
| # `model--gpu` are both legal repository names. | ||
| _REF_SEP='(__|[._]|-+)' | ||
| _REF_HOST="[A-Za-z0-9]+(([.]|-+)[A-Za-z0-9]+)*(:[0-9]{1,5})?" |
There was a problem hiding this comment.
Fresh evidence beyond the separator fix is that this host expression still rejects valid bracketed IPv6 registries, such as [2001:db8::1]:5000/team/model:v1. Because image_pin is honored verbatim, an installed service can launch such an image successfully, but image_exists() rejects the same reference before invoking the rootful seam; image_present() then reports it as missing. Extend both the wrapper regex and its Python mirror to accept bracketed IPv6 hosts.
Useful? React with 👍 / 👎.
| if bool(running_digest) != bool(declared_digest): | ||
| return _split_image_tag(running_repo)[0] != _split_image_tag(declared_repo)[0] | ||
|
|
||
| return True |
There was a problem hiding this comment.
When both references are digest-pinned, this unconditional return reports drift unless their complete strings match. Valid equivalent forms such as ghcr.io/team/model:v1@sha256:<digest> and ghcr.io/team/model@sha256:<same-digest> therefore produce image_mismatch=true, even though the repository and immutable digest identify the same image. The both-digest branch should compare the repository with any tag removed and then compare the digest values.
Useful? React with 👍 / 👎.
| # cosmetic — probe it. `help` prints usage and touches nothing. | ||
| SeamSpec( | ||
| "hal0-podman-ro", | ||
| probe=("help",), |
There was a problem hiding this comment.
On a box where the best-effort wrapper refresh fails and leaves the pre-#1889 wrapper installed, this probe still exits successfully because that old wrapper already implements help, even though it rejects all three new verbs. hal0 doctor therefore reports the seam as healthy while image_status remains missing and actual_image remains unavailable. Probe a release-specific, side-effect-free capability such as check-slot-token <known-token>, or validate that the help output advertises the new verbs.
Useful? React with 👍 / 👎.
…ests Third Codex round, three P2s, all real. 1. The seam probe could not tell a current wrapper from a stale one. `help` exists on the PRE-#1889 wrapper too, so a box whose best-effort wrapper refresh failed would keep the one-verb wrapper, probe green, and report the seam healthy from `hal0 doctor` and from install.sh's post-install assertion — while every new verb was rejected and image_status stayed "missing". That is exactly the undiagnosable-green failure #1465 exists to prevent. Both probe inventories now use `check-slot-token hal0probe`, which is release-specific (the old wrapper answers rc 64) and still side-effect-free: it validates the token and prints the container name it would build, touching neither podman nor the filesystem. preflight's probe runner now splits its hardcoded probe line into argv words, since a verb-only probe cannot make this distinction. 2. Bracketed IPv6 registry literals were rejected. `[2001:db8::1]:5000/team/ model:v1` is a legal reference host, and a rejection here is the #1889 failure mode again — the ref never reaches the rootful seam and the image reports missing. The bracket body is hex-and-colons only, so it still cannot carry a path, whitespace, a metacharacter or a second word. 3. Two digest-pinned refs compared as whole strings. A digest IS the immutable image id, so `repo:v1@sha256:D` and `repo@sha256:D` name the same image and the tag text alongside is noise; the old code reported drift for them. When both sides carry a digest the comparison is now repository-without-tag plus digest. The one-sided case is unchanged (repository only — resolving a tag against a digest needs a registry round-trip this hot path will never make). Refs #1889
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Codex round 3 — all three P2s accepted and fixed in 8290fdf6. The probe could not tell a current wrapper from a stale one — CONFIRMED, fixed. The best finding of the three, and a self-inflicted one: I added a Both inventories now probe 7. Bracketed IPv6 registries rejected — CONFIRMED, fixed. 8. Two digest-pinned refs compared as whole strings — CONFIRMED, fixed. A digest is the immutable image id, so VerificationPrior rounds' CI on 6d6c4de was fully green (python 3.12, γ-suite, ui, sunset, CodeQL ×4) before these commits landed. Still not for automerge — SUDOERS + INSTALLER change, operator review required. |
Independent security review — APPROVE (no blocking findings)Adversarial review of the privilege boundary only. I did not write this code and re-ran every claim rather than reading the PR body for them. No validation bypass found. 4 non-blocking findings, all hardening/plumbing, none of which lets a caller escape the intended object or reach a non-read podman subcommand. Note this does not clear the PR's own operator gate — it is a sudo-seam change and still needs the operator's sign-off, not automerge. Design claims verified1. Root-side validation. ~120 hostile inputs beyond the suite's set, run against the real
Arity is enforced per verb ( No shell interpolation anywhere: every podman call is a quoted exec array, No ReDoS. 2. Container verbs cannot address an arbitrary container. Confirmed. The token charset admits 3. Sudoers byte-unchanged. 4. Exit-code contract + fallback gate. Wrapper side is right: 5. Parity test really runs the wrapper. Mutated 6. Codex round-3 #6 — probe is release-specific. Confirmed against the real old wrapper from 7. Revert-and-confirm-red. 8. No hostile ref reaches a non-read subcommand. The only podman invocations in the file are Suites: Findings (non-blocking)1. [low] Root-side regex semantics are caller-influenced by locale, and the test hides it. The Python mirror uses I brute-forced all 255 single bytes under both locales and the accepted set is identical and ASCII-alnum-only in each, so nothing structural leaks: no metacharacter, no space, no Suggested: 2. [low] rc 64 / 65 / 66 all collapse to 3. [nit] 4. [nit] TOCTOU between Assembly of |
Security review hardening on #1889 (APPROVE, 0 blocking). Finding 1 [low] — the wrapper's `[[ =~ ]]` validators were locale-sensitive. bash bracket expressions use the current locale's collation, so under a UTF-8 locale `[A-Za-z0-9]` collates accented letters INTO the range: with LC_ALL=en_US.UTF-8, `check-image-ref alpiné` returned rc 0 while the Python mirror rejected it — the wrapper LOOSER than its mirror, the dangerous direction of a parity break. It was reachable in production because sudo's default `env_keep` passes LANG/LC_* straight through from the calling process; the test suite could not see it because its `_ENV` strips locale vars entirely. `export LC_ALL=C` now runs immediately after `set -euo pipefail`, before any validator can, so every character class means exactly the ASCII bytes it spells in any caller's environment. Reproduced first: `env -i PATH=… LC_ALL=en_US.UTF-8 hal0-podman-ro check-image-ref alpiné` → rc 0 before, rc 64 after. Finding 3 [nit] — `container_read` used a bare `podman inspect`, which also resolves images, volumes, networks and pods, so a name collision could return a different object's fields. Now `podman container inspect`: type-safety by construction, even though the name is already pinned to the hal0-slot- prefix. Tests: 7 new cases, red before the fix. Unicode refs and tokens must be rejected under a UTF-8 locale; the whole ref corpus must reach an IDENTICAL verdict in both locales AND still agree with the Python mirror under UTF-8; a structural assertion pins `export LC_ALL=C` ahead of the first validator (the locale cases prove nothing on a box where en_US.UTF-8 is not generated, since bash silently falls back to C there); and every `run_podman … inspect` must name its object type. Refs #1889
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Security review hardening — both findings addressed in
|
1 participant
Important
SUDOERS + INSTALLER CHANGE — operator review required. This PR widens a root sudo seam (
hal0-podman-ro) from zero caller-supplied argv to three verbs that each accept one validated positional operand. Please review the validation regexes and the argument doctrine in the wrapper header before merging. Do not automerge.Refs #1889.
The bug
ContainerProvider.image_present/running_image/running_argvshelled out to a barepodmanas the unprivilegedhal0service user, which has no subuid ranges at all, so they read hal0-api's own rootless image store. Slots run rootful podman (Quadlet units under/etc/containers/systemd/, root's store), so that store by construction never holds a slot image.On every standard install (reproduced on ct151, ct150 and ct105/rc.5 — pre-existing, not an rc.6 regression):
GET /api/slotsreportedimage_status: "missing"for every running, healthy slotGET /api/slots/{name}/pull/statusread "missing" for a present imageactual_imagewas alwaysnull, so the Cleanup: retire /proc actual_backend; deterministic image-tag mismatch #663 image-drift detector could never fireThe
hal0-podman-roseam and itsis_hal0_service_user()gate already existed —cbc8e94dwired them into/api/system-info, which answers correctly — but the wrapper exposed only the argument-freeimagesverb, and its header doctrine forbade caller args reaching podman's argv. The three call sites that need a specific image ref or container name therefore could not use it.The privilege boundary
The doctrine is refined, not abandoned, and lands exactly where
hal0-systemctl's slot/agent verbs have had it since P3-perms: a caller-supplied value may reach podman's argv only as a single positional operand validated on the root side against a closed regex, for a verb whose podman subcommand, flags and--formatstring are literals in the wrapper.image-exists <ref>^([A-Za-z0-9]+([.-][A-Za-z0-9]+)*(:[0-9]{1,5})?/)?[A-Za-z0-9]+([._-][A-Za-z0-9]+)*(/…)*(:tag)?(@sha256:[0-9a-f]{64})?$, length ≤ 512container-image <slot-token>^[A-Za-z0-9_-]{1,64}$— byte-identical tohal0-systemctl'svalidate_slot_idcontainer-argv <slot-token>Deliberate design points:
hal0-slot-<token>itself, root-side. The caller can therefore only ever address a hal0 slot container — never an arbitrary one, and never a podman option.-(no flag smuggling),.., or a second argv word. Each verb rejects a second argv word outright.eval, no word splitting, no wildcards, no caller-supplied--format.rm/run/build/exec/pullremain unreachable.image_status: "missing"again).hal0 ALL=(root) NOPASSWD: /usr/lib/hal0/bin/hal0-podman-ro). A bare command path in sudoers already permits any argv; the wrapper is the control surface. Enumerating verbs in sudoers instead would be a second, silently-drifting copy of the verb list, and sudoers wildcards are a known footgun. Only the explanatory comment changed.Exit-code contract
0= podman answered (empty stdout on a container verb is a real "no such container");64= wrapper rejected the argument or verb;65= podman absent. This is what lets the Python side tell "the seam did not answer" from "the answer is negative". The named-object reads deliberately do not silently fall back to the rootless store: a rootless answer about a named image is not a stale answer, it is an answer about a different object — that conflation is #1889. The fallback survives only for the dev/CI case where the process is not the hal0 service user (no grant exists there, and the operator's own store is the right one).Files changed
installer/wrappers/hal0-podman-rocheck-image-ref,check-slot-token), 2 validators, exit-code contract, rewritten argument doctrinepackaging/sudoers/hal0-podman-roinstaller/install.shinstaller/lib/preflight.shhal0-podman-rogains ahelpprobe — the grant is now load-bearing for slot status, so presence-only checking was no longer enoughsrc/hal0/system/seam_check.py#1465inventory)src/hal0/providers/podman_introspect.pyimage_exists/container_image/container_argv(tri-state),is_valid_image_ref/is_valid_slot_tokenmirrorssrc/hal0/providers/container.py_decode_argv_jsonhelpertests/installer/test_podman_ro_validation.pytests/providers/test_container_podman_ro_routing.pytests/providers/test_podman_introspect.pyTest evidence
The wrapper tests run the real bash wrapper (no root, no sudo, no podman, no provisioned box — the
hal0-systemctldrop-in suite's posture) over ~40 malicious argv shapes: command chaining (;/&&/||/|), substitution ($(…)/backticks), flag smuggling (--rm,-v/:/host,--format={{.Config}}, bare-), traversal (../../../etc/shadow,foo/../../etc/passwd), malformed digests, control bytes and RTL-override unicode, plus 12 legitimate refs that must not be rejected. They also assert parity between the wrapper and the Python mirrors — a mirror that drifts looser turns a fast rejection into an opaquerc 64; one that drifts stricter silently re-creates #1889.The issue's "test to promote" is
test_slot_view_reports_present_for_a_running_slot, which drives the realContainerProviderthroughslot_view's TTL cache helper — the exact path that producesimage_status. The seam-routing tests booby-trapsubprocess.runso a test cannot pass by silently falling through to the rootless read.Not covered / follow-ups
tests/apisuite was not run locally (it exceeds a 10-minute local budget); the relevant slot/system-info files were. CI covers the rest.shellcheckis not installed on this box, sotests/installer/test_platform_gate_hardening.pyskipped its shellcheck arm as it does on main.bash -npasses and is asserted by a test.image_status: "present"for a running slot on ct151) needs a deploy and is left to RC validation.🤖 Generated with Claude Code