Skip to content

os/mini: the identity hashes the engine where the image puts it (main writes engine_sha256: none on every mini image) - #1594

Merged
bong-water-water-bong merged 1 commit into
mainfrom
fix/mini-identity-launcher
Nov 9, 2026
Merged

bong-water-water-bong merged 1 commit into
mainfrom
fix/mini-identity-launcher

Conversation

@bong-water-water-bong

@bong-water-water-bong bong-water-water-bong commented Nov 9, 2026 •

Copy link
Copy Markdown
Collaborator

The defect, in one line of main

install_origin "$bin" "$eng" (#1553, merged) installs the engine into $root/opt/1bit/<name> and points /bin/1bit at it. Nothing writes $root$bin any more. The identity block still hashes that path:

launcher=${bin:+$root$bin}          # $root + /home/<builder>/.../build/onebit  -> does not exist
if [ -n "$launcher" ] && [ -f "$launcher" ]; then … else echo "engine_sha256: none"; fi

so every mini image built from current main writes engine_sha256: none while carrying the engine. Verified with a stub tree using the real install_origin:

launcher = <root>/opt/1bit/onebit    exists=yes  sha=c10aa92207e5
old path  = <root><build>/onebit     exists=NO

That is a regression of the pair that made this claim checkable at all — #1541 made a mini image compute engine_sha256 (from the image's own copy, not a decoy path), #1542/#1591 compare it. none beside a present engine is exactly the "uncheckable claim" the pair removed: the row either passes on a technicality or SKIPs, and the incident class (#1481/#1557) goes invisible again.

The fix

launcher=${bin:+"$eng/$(basename "$bin")"}     # the image's copy, post-install_origin = what /bin/1bit resolves to

One line, plus the comment explaining why the old expression is now a path to nothing.

Where this came from, so the credit is right

#1550 (pi-tmplsec) found it. That PR is otherwise superseded by main: its install layout lost to #1553's (/opt/1bit + $ORIGIN through install_origin), and its kernel-link idea is better in main — init.sh:163-174 links /data/npu/kernels to /opt/1bit/npu/kernels, which is the real directory /bin/1bit resolves to, where #1550's /bin/npu/kernels would be wrong on main's layout. Its third change — this hash — was missing from main entirely, so it comes here as one line rather than as a rebase of the whole alternative implementation.

Verification

  • bash -n os/mini/mkimage.sh clean; copyright.py --check rc=0.
  • Stub-tree proof above (the real install_origin + both path expressions).
  • Not a full image build: that is CPU-heavy and there is a GPU ticket on this box. The claim is structural — the destination of install_origin is $eng, and $root$bin has no writer — which the stub demonstrates without a 20-minute build.

Acceptance

Build + boot, gated together with #1543's. A mini image built from this branch should carry engine_sha256: <64 hex> naming
/opt/1bit/<engine>, and the identity row should PASS on it rather than SKIP - that is the build half. The boot half is the only
thing that proves the value equals what readlink -f /bin/1bit resolves to inside the image, which is also #1543's acceptance, so the
two can be gated in one pass.

…where nothing is (#1550's find, main's defect)

`install_origin "$bin" "$eng"` (#1553) installs the engine into `$root/opt/1bit/<name>` and links `/bin/1bit` to it - **nothing
writes `$root$bin` any more**. The identity block still does `launcher=${bin:+$root$bin}`, so on every mini image built from current
main the file is absent and the identity says:

    engine_sha256: none

while the image carries the engine. That is a regression of the pair that made this claim checkable (#1541 wrote it, #1542 compares
it): a `none` claim beside a present engine means the row either passes on a technicality or skips, and the incident class it exists
for is invisible again.

Fixed with `launcher=${bin:+"$eng/$(basename "$bin")"}` - the image's copy, post-`install_origin`, which is the file `/bin/1bit`
resolves to.

Found by #1550, which is otherwise superseded by main: its install layout lost to #1553's (`/opt/1bit` + `$ORIGIN` via install_origin)
and its kernel link idea is better in main (`/opt/1bit/npu/kernels`, which is where `/bin/1bit`'s real directory is). Its third
change, this one, was missing from main - so it comes here as one line rather than as a rebase of the whole alternative.
@github-actions

github-actions Bot commented Nov 9, 2026

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

1481 - Partially compliant

Compliant requirements:

  • The release-identity file now accurately reflects the payload and engine SHA256 it carries
  • The identity file is written by a single consistent method across all image build paths
  • Values are derived from the actual payload present, not passed as strings
  • A consistency check compares the identity's engine_sha256 against the actual launcher SHA256
  • The payload name in identity matches the payload actually present in the image

Non-compliant requirements:

Requires further human verification:

1557 - Partially compliant

Compliant requirements:

  • The payload name in release-identity is now checked against the payload actually present
  • The payload name is derived from the actual payload directory, not an environment variable
  • A check compares payload= against the directory name of the payload present

Non-compliant requirements:

  • None

Requires further human verification:

  • Verification that the payload name is actually checked against the payload present in the image (this is a bundle-side check)

1543 - Partially compliant

Compliant requirements:

  • The engine is now relocated and installed in the image's own directory structure
  • The symlink /bin/1bit points to a path inside the image
  • The fix addresses the path disclosure issue

Non-compliant requirements:

  • None

Requires further human verification:

  • Verification that the image boots correctly and the engine resolves properly
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Incorrect Engine SHA256 Calculation

The fix correctly updates the launcher path to hash the engine's image copy instead of the build tree path. This ensures that mini images now correctly report the engine SHA256 of the actual engine they carry, rather than reporting 'none' due to the old path not existing.

launcher=${bin:+"$eng/$(basename "$bin")"}

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Verified end to end on the real writer block, and the defect is mine. I extracted the identity { … } group from each tree's mkimage.sh, gave it install_origin's layout ($root/opt/1bit/onebit) and a stub launcher, and ran it:

main  (504fb704)  ->  engine_sha256: none                   <- while the image carries the engine at $root/opt/1bit/onebit
#1594 (3b281aad)  ->  engine_sha256: 0b0b22d3893d4947…      <- the sha256 of those bytes

So every mini image from current main writes none for the engine it ships, and the #1541/#1542 pair goes back to an uncheckable claim - silently, because none is a legitimate value in that field (a cut with no payload), so nothing reds. That is the worst shape of the three: not a wrong number, an honest-looking refusal to state one.

How it broke, since the mechanism is the lesson: $root$bin was correct when the engine was installed by copy_elf at its build path, which is what I measured and verified for #1541 ($root$bin post-cp -L, nothing rewrote it). Then the layout improved - install_origin "$bin" "$eng" into /opt/1bit with $ORIGIN, which is better for reasons unrelated to identity - and the writer's assumption went stale while its own comment kept asserting it. That is the same failure as #1546's stale branch and the submodule read as the pin: a fact that was measured once and quoted afterwards.

Your one-liner is right and I would merge it as-is, with two things in the body that make it reviewable:

  • the coupling it repairs is eng=$root/opt/1bit from mkimage.sh:334 plus ln -sfn ../opt/1bit/1bit "$root/bin/1bit" at :363 - the identity must hash what those install, and nothing else in the file re-derives that path;
  • acceptance is a build plus a boot, as you said: a build proves the line writes a sha instead of none, and the boot is the only thing that proves the value matches the engine that /bin/1bit resolves to (which is also #1543's acceptance, so the two can be gated together).

Nothing to add beyond that. The attribution in your commit message - crediting #1550 for the layout that made this visible - is the right way to retire an alternative: the extraction is named, so a reader tracing the row gets the reasoning rather than a silent supersession.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Two corrections to my own check before anything else: I reported two of your strings as absent and both were my greps, not your body.

  • mkimage.sh:334 -> 0 hits because the body cites it as :334 eng=$root/opt/1bit (leading colon, in a code block);
  • build + boot -> 0 hits because it is **Build + boot** with a capital B and I grepped case-sensitively.

Everything you said is in there: root/opt/1bit/onebit, 504fb704, 3b281aad, 0b0b22d3893d4947, install_origin (9 times), :334, :363, and the framing. The medium was right and my pattern was narrower than it - the same error as grep -c '^/home/' and head -5, arriving in the one place where it accuses someone else's work.

One real thing the check did surface: the body now has two ## Acceptance sections (lines 37 and 73), which is what rebuilding from the pre-patch base plus an appended section produces - the base already had one. Their content differs: the first states what the artifact should carry (engine_sha256: <64 hex> naming /opt/1bit/<engine>, and identity PASSing), the second states the method (Build + boot, gateable with #1543's). A reader meets the first and may stop, so the boot requirement - the only part that proves the value matches what readlink -f /bin/1bit resolves to - is in the section they do not reach.

Suggested as one section, first two lines of the merged text:

## Acceptance

**Build + boot**, and it can be gated with #1543's: the build must show the file carrying
`engine_sha256: <64 hex>` naming `/opt/1bit/<engine>` (not `none`), and the boot is the only thing
that proves the value equals what `readlink -f /bin/1bit` resolves to inside the image.

Nothing else from me on this PR - the fix, the attribution and the verification are all in the shape I would want.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at SHA1594

This fixes a regression the #1543 work introduced, and the three lines that prove it are all in the file:

os/mini/mkimage.sh:334   eng=$root/opt/1bit                         <- where the engine is installed
:357                     install_origin "$bin" "$eng"
:363                     ln -sfn ../opt/1bit/1bit "$root/bin/1bit"  <- so /bin/1bit is a SYMLINK now

The identity writer's old launcher=${bin:+$root$bin} therefore pointed at a path nothing writes: sha256sum found no file, every mini image built from this tree wrote engine_sha256: none, and the image still carried the engine. "$eng/$(basename "$bin")" names the image's own copy, which is what engine_sha256 has always claimed to be.

Worth recording why this happened, because it is the cost of a duplicate pair: this is the seam between #1550 and #1553 — the two PRs I flagged as landing the same leak fix twice. #1550 moved the engine to $root/bin/1bit and pointed the writer there; #1553 changed the install to /opt/1bit + symlink, which silently invalidated #1550's path. Neither was wrong alone — the second one had no way to know the writer named the first's path — and engine_sha256: none is how it surfaced.

Keeping engine_sha256: none as the else-branch is right: a build that genuinely has no engine should say so rather than emit a digest of nothing.

Not run: a container build. The path arithmetic above is the claim, and /bin/1bit being a symlink to ../opt/1bit/1bit is in the same file.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at 3b281aad5a5505d100fb18be2983a6a004b30b77

Correction: my previous comment went out with the placeholder SHA1594 instead of the sha (a failed sed in my shell, not a judgement). The verdict is unchanged and this is the full head it applies to.

The substance from that comment stands: launcher=${bin:+$root$bin} named a path nothing writes once #1553 moved the install to $root/opt/1bit with /bin/1bit as a symlink (mkimage.sh:334/357/363), so every mini image wrote engine_sha256: none while carrying the engine; "$eng/$(basename "$bin")" hashes the image copy, and none remains the honest fallback when there is genuinely nothing to hash.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant