fix: repair master after the 09-07 batch merge - #115
Kartikey1306 wants to merge 6 commits into
Conversation
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
srpatcha
left a comment
srpatcha
left a comment
There was a problem hiding this comment.
Review — eBoot#115 "fix: repair master after the 09-07 batch merge"
head: 4d8696c author: Kartikey1306 ci: pass (25 checks green, 0 red; only Create GitHub Release skipped)
Verdict: This repairs a master that does not configure, does not compile, does not
link and has no passing test suite. I reproduced the broken baseline and the repaired one
independently: clean origin/master fails to build; this head configures, builds with no
errors and passes 31/31 under ctest --no-tests=error. Every blob-identity claim in the
PR body checks out against git rev-parse. This should land before the other four open
eBoot PRs — three of them are blocked behind exactly the breakage it fixes.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Medium | tests/unit/test_fw_update.c:156-181, tests/unit/test_fw_transport.c |
A behavioural arbitration is being settled inside a repair PR, with no ADR and no design text backing it. #104 (verify signature at install, before anti-rollback) and #103 (anti-rollback counter check) were both merged and contradict each other; this PR picks #104's ordering and rewrites #103's fixtures to sign their images. The call is right and the reasoning in the body is sound — a counter is only trustworthy after the prefix binding it is authenticated. But the master design does not actually say so for the update path: §8.1 orders "Verify Manifest → Verify Image → Check Version Policy" for boot, while §15's update flow is "Download → Verify → Install" and never places the anti-rollback counter check at all. So the repo now has an ordering rule that exists only in a PR body. .ai/architect.md asks that structural decisions be recorded, and this is one. |
Nothing to change in the code. Record the decision: an ADR under the §38 series (the natural slot is ADR-011, eOTA firmware/update contract) stating that anti-rollback evaluation happens strictly after signature verification in the install path. I have appended an architecture proposal for §15.1 to .ai/autoreview/proposals/2026-09.md so the design stops being silent on it. |
| 2 | Low | core/sha512.c (eos_sha512_update) |
ctx->bitlen[0] += (uint64_t)copy >> 61; is always zero. copy is clamped to 128 - ctx->buffer_len, so copy <= 128 and copy >> 61 == 0 on every iteration. The real 128-bit carry is already handled correctly by the old_low comparison two lines above. The line reads as overflow handling and does nothing, which is the kind of thing a future reader preserves because they assume it matters. Not a defect — the digest is correct, confirmed by KAT below. |
Delete the line, or replace it with a comment noting that copy is block-bounded so the low word alone cannot overflow within one iteration. Since this file is a byte-exact restoration of a previously-reviewed blob, it is fine to leave for a follow-up rather than perturb the restoration. |
| 3 | Low | tests/vectors/fw_update_test_sigs.h:1-8, tools/gen_fw_update_test_sigs.py |
The committed vectors carry a "do not edit / regenerate with …" header, but nothing verifies that the committed bytes are what the committed script produces. If someone edits the header by hand, or changes a field inside the signed prefix in build_image() without regenerating, the failure surfaces as an opaque EOS_ERR_SIGNATURE in an unrelated-looking test rather than as "your vectors are stale". The suites do fail closed, which is the important half. |
Add a Python test that runs tools/gen_fw_update_test_sigs.py and asserts its stdout equals tests/vectors/fw_update_test_sigs.h. requirements.txt:4 already pins cryptography>=41.0, and tests/unit/test_eos_sign_payload_offset.py:34 already has the "ImportError must fail the job" idiom to copy. |
Three findings, all minor, on a 21-file 522-line repair. That is the honest count — I went
looking for more and did not find it.
Verification performed for this review
Detached scratch worktrees under .ai/autoreview/state/verify/. The user's eBoot
checkout was not touched; nothing was committed to a tracked branch or pushed.
| Check | Result |
|---|---|
cmake -B build/host -DEBLDR_BUILD_TESTS=ON -DCMAKE_BUILD_TYPE=Debug on this head |
PASS |
cmake --build build/host --parallel 4 on this head |
PASS — no errors |
ctest --test-dir build/host --output-on-failure --no-tests=error |
PASS — 100%, 31/31, 11.04s |
Same three steps on clean origin/master |
FAIL — configure aborts on the duplicate eboot_test_fdt_loader; with that removed, compile fails core/sha512.c ×5 + core/ed25519_verify.c:303. Confirms the stated baseline. |
test_ed25519 specifically (covers the restored SHA-512) |
PASS, 10.40s — RFC 8032 vectors accepted, test_sha512_known_answers, test_sha512_streaming_matches_one_shot |
Blob claim core/sha512.c = d9aa57c |
CONFIRMED — git rev-parse FETCH_HEAD:core/sha512.c → d9aa57ccc57dd28aad380693b0c579e4a2b100be |
Blob claim CMakeLists.txt = f8fe6eb |
CONFIRMED |
Blob claim include/eos_boot_log.h = 964ebb8 |
CONFIRMED |
Provenance of d9aa57c (was it really pre-#55?) |
CONFIRMED — that blob was core/sha512.c at abd4dab (#93, 2026-09-03); effa9a2 (#55, 2026-09-07) replaced it |
#111 hunk equivalence claim |
CONFIRMED — the deletion hunk is identical; the files differ because #115 also fills in EBLDR_UNIT_TESTS. The body says "hunk", which is the accurate word. |
pytest tests/ ("78 passed") |
NOT RUN — No module named pytest on this host. Unverified. |
On the SHA-512 "rewrite". core/sha512.c shows as 211+/111− and the first instinct is
that someone hand-wrote a hash primitive into the TCB, which .ai/security.md §Cryptography
would treat harshly. It is not: the file is byte-identical to the blob that was in the tree
from #93 until #55 overwrote it with a version predating the bitlen[2]/buffer_len
context. I read it anyway. Init constants are the FIPS 180-4 SHA-512 IVs; the 128-bit length
is accumulated correctly with the carry into bitlen[0]; final() handles the i > 112
case (no room for the 16-byte length) by transforming a padded block first, and the i == 112
boundary lands exactly right; buffer_len can never exceed 127 on entry to final(), so the
0x80 write cannot overrun buffer[128]; the context is zeroized on completion. Together
with the passing KAT, streaming-vs-one-shot equivalence test, and RFC 8032 vectors — which
would fail on any deviation, since Ed25519 derives its challenge scalar from this hash — the
restoration is sound.
Was anything weakened? No, and I checked specifically because the diff deletes 40 lines
from tests/unit/test_ed25519.c and one line each from test_tlv_auth.c and others.
master declared k_low_order[8][32] and messages[] twice (test_ed25519.c:106 and
:266), which is itself a compile error; the PR removes the first, unused copy and keeps the
one that sits with k_non_canonical[]. All fourteen TEST() cases survive and all fourteen
are still invoked from main() — including test_ed25519_low_order_keys_rejected,
test_ed25519_non_canonical_encodings_rejected and
test_ed25519_low_order_R_with_a_valid_key_is_not_a_forgery, the three that .ai/security.md's
"reject invalid and low-order public keys explicitly" rule depends on. The tests_run = <literal>
deletions are the opposite of a weakening: they replace a hand-maintained count with
tests_run++ inside the TEST() macro, so a suite can no longer silently report 6/6
while running four.
The change also tightens three things worth naming: ci.yml:253 adds fuzz-build to
CI Gate's needs (the gate previously did not wait for it); eosim-sanity.yml pins
shell: bash on a job whose Windows legs ran bash syntax under PowerShell, where the
|| { exit 1 } guard parsed as an unexecuted script block and could never fail; and
tests/CMakeLists.txt:226 restores foreach(TEST_NAME ${EBLDR_UNIT_TESTS}) with fourteen
missing list(APPEND ...) entries filled in, which puts eleven previously-uncovered suites
back under Valgrind. All three are fail-closed repairs of checks that could not fail.
On the test signing key. tools/gen_fw_update_test_sigs.py embeds the RFC 8032 §7.1
TEST 1 secret scalar. That is a published constant in a public IETF document, not a secret,
and the script says so. Only the public half and the resulting signatures reach
tests/vectors/fw_update_test_sigs.h. Per .ai/security.md's "test keys must be
structurally incapable of signing a release artifact": the script is invoked by nothing in
.github/workflows/release.yml, which builds and signs through a separate path, and its
output is a C header consumed only by tests/. No exposure.
Architecture conformance
Conforms. §21: eBoot is Tier 1 Foundation; every file touched is inside the owning repo.
§5.1 dependency direction is preserved and in one place restored — the CMakeLists.txt
change adds core/boot_log.c, core/secure_boot.c and core/fdt_loader.c back to
eboot_core, which is the correct tier for them under .ai/architect.md's target shape
(core/ = shared boot logic). No edge points up a tier; nothing outside eBoot is referenced.
§5.1's "eBoot keeps the trusted computing base minimal and auditable" is directly served:
before this PR the secure-boot implementation was not compiled into the core library at
all, so eos_secure_boot, eos_secure_boot_lock_debug and eos_boot_log_append were
undefined symbols — a TCB that does not link is not auditable. §8.1's boot ordering is
unchanged. The include/eos_boot_log.h edit looks at first like a removed public function,
but master declared eos_boot_log_read() twice (:61 and :72) and never declared
eos_boot_log_get_head(), which core/boot_log.c:57 defines; the PR replaces the redundant
declaration with the missing one. No public API is lost, so brief item 8 is not engaged.
The pattern behind this PR
Worth recording beyond this review: git log -- core/sha512.c shows five separate
"restore the build / repair merge artifacts" commits between 2026-08-27 and 2026-09-11
(9fbfc91, 45519d7, ffda321, effa9a2, a8743dd). The proximate cause named here —
twenty PRs merged in ninety minutes, each replayed onto a master its author never saw —
is a merge-process defect, not a code defect, and no amount of repair PRs will stop it
recurring. A required status check on master plus a merge queue that rebases and re-runs
before landing would. That is outside this PR's scope and is carried into the run summary.
Proposed changes
Merge order (each of the next three is blocked until this lands):
1. #115 (this PR)
2. #111 -> its tests/CMakeLists.txt hunk becomes empty; close it or let it rebase to nothing
3. #109 -> rebase; resolve test_jump_app_bounds.c:main() by DROPPING `tests_run = 3;`
4. #112, #116, #117 -> rebase and get their first real CI
Follow-ups, none blocking:
- delete the dead `copy >> 61` term in core/sha512.c
- add a test regenerating tests/vectors/fw_update_test_sigs.h and diffing it
- ADR-011 addendum: anti-rollback is evaluated after signature verification
Not checked
pytest tests/— NOT RUN.pytestis not installed on this host, so the body's
"78 passed" is unverified here, as istest_imgpack.py's--version=Xargparse fix and
the four newNO_TEST_MACROclassifications intest_suite_bookkeeping.py. The reasoning
for the argparse change is correct on its face (3.10's negative-number matcher treats
-1.0.0as an unknown option), but I did not run it on 3.10 to confirm.- Cross-compile (STM32F4, Cortex-M4) and the fuzz harness build — not run locally. CI
reports both green on this head and I am taking that at face value; I did not read the logs. - ASan/UBSan — not run. The body claims 31/31 under
-DEBLDR_SANITIZE=ON; I built Debug
without sanitizers. Unverified. - The
#104 vs #103negative control. The body says flipping one signature byte makes
test_write_streams_tlv_then_finalize_rejects_below_floorfail withEOS_ERR_SIGNATURE.
I did not reproduce that mutation, so the claim that these tests would catch a signature
regression is inferred from readingbuild_image(), not observed. - The gcr.io billing explanation for the
scorecard-actionbump. The version change is
benign either way, but I did not verify the stated cause. - I did not audit the other ~18 PRs from the 09-07 batch for damage this PR did not find.
The claim "this is everything that broke" is the author's; my evidence only supports
"everything that breaks configure, compile, link andcteston the host is now fixed."
Automated architecture review of 4d8696cbecbd — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.
srpatcha
left a comment
srpatcha
left a comment
There was a problem hiding this comment.
Thank you for this — it is a careful repair and the description made it easy to check. What I verified locally on your branch (gcc, Ninja, Debug): configure OK, build clean apart from the intentional #warning in keystore.c, ctest 31/31, the 31 valgrind_* targets 31/31, pytest 78/78. I also confirmed the three "blob-identical" claims by hash, reproduced the master configure failure and the sha512.c/ed25519_verify.c:303 compile failures on #111's branch, and confirmed tools/gen_fw_update_test_sigs.py regenerates tests/vectors/fw_update_test_sigs.h byte-for-byte.
One non-blocking note: tests/unit/test_fw_update.c:174-181 — since the C fixture and the Python generator each build the signed prefix independently, a future edit to either silently breaks the other with EOS_ERR_SIGNATURE. A short comment above build_image() naming the regenerate command would help.
For the maintainers: #116 is stacked on this branch, and #111 is a strict subset of the tests/CMakeLists.txt hunk here. Approving — this should land first.
The changelog said eleven suites had no Valgrind run. Eleven were missing from EBLDR_UNIT_TESTS, but four of those were named in the hand-written foreach and did run; seven had no run at all. Say which. ADR-020: its design-document citation now says where it comes from (the architecture review of embeddedos-org#115), and the sentence about the two PRs' bases is replaced with what the history shows -- embeddedos-org#103's commits predate embeddedos-org#104's merge, and embeddedos-org#104 was written without embeddedos-org#103's check in place.
4d8696c to
976ab0c
Compare
|
Thanks for the verification and the approval. Follow-ups are on the branch now, each as its own commit:
Current head: Release build clean, |
srpatcha
left a comment
srpatcha
left a comment
There was a problem hiding this comment.
Review — eBoot#115 "fix: repair master after the 09-07 batch merge"
head: a276016 author: Kartikey1306 ci: pass (26 green, 0 red; only Create GitHub Release skipped)
Verdict: Follow-up review. All three findings from the previous report are answered —
two fixed on this branch, one moved to #123. The new commits are clean and I verified the
one that matters (the fixture pin) by breaking it on purpose. Two residuals, both Low, both
stated below rather than dropped. Nothing here blocks the merge.
Status of the previous findings (eBoot-115-4d8696cb.md)
| # | Prev. severity | Status | Evidence |
|---|---|---|---|
| 1 | Medium — install-path ordering settled with no ADR | Resolved | 049e2fe adds docs/adr/ADR-020-install-path-verifies-signature-before-anti-rollback.md and docs/adr/README.md. The recorded rule matches the code: core/fw_update.c:259 calls eos_image_verify_signature(), :270/:277 read and check the counter after it. The numbering claim checks out — eos/docs/adr/ holds exactly ADR-012…ADR-019, so 020 reuses nothing. |
| 2 | Low — dead copy >> 61 carry term in core/sha512.c |
Resolved elsewhere, correctly | Deliberately excluded so core/sha512.c stays blob d9aa57c; carried as #123, which is open and in this review batch. Keeping a byte-exact restoration byte-exact was the right call. |
| 3 | Low — nothing checks the committed vectors against their generator | Addressed, with a residual | 8028bcc + a276016 add tests/unit/test_fw_update_test_sigs.py. See finding 1 below for the part that is still open. |
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Low | tools/gen_fw_update_test_sigs.py:25-27, tests/unit/test_fw_update.c:142, tests/unit/test_fw_transport.c:347 |
The pin test closes the hand-edited header hole, not the drifted prefix hole. test_committed_header_is_the_generator_output proves tests/vectors/fw_update_test_sigs.h == generator stdout, and I confirmed it fails on a one-byte edit. But the generator states outright that "the field values are copied from the tests" — the signed prefix is still built twice, in Python and in C. Change a field in build_image(), regenerate, and both halves agree with each other while disagreeing with the C test: the pin stays green and the failure is still the opaque EOS_ERR_SIGNATURE finding 3 was about. The new comments at both call sites are a real mitigation and may well be the proportionate answer for a test fixture; this is recorded so the remaining gap is on paper, not so it must be closed now. |
If it is worth closing later: have the C suite emit its prefix bytes under a test-only flag and assert the generator's signed_prefix() equals them, so the two copies are compared rather than described. Not worth a refactor on its own. |
| 2 | Low | docs/adr/ADR-020-...md:4, CHANGELOG.md |
The ADR is status: Proposed. docs/adr/README.md defines Proposed as "not yet ratified … " and reserves "binding on new code" for Accepted, while the CHANGELOG entry says the ordering is "settled". The code is already merged and behaving this way, so the record is describing shipped behaviour with a status that says it is not yet binding. |
Nothing for the author to change unilaterally — ratification belongs to the deciders named in the header (Architecture Council, eBoot maintainers). Flag it for them so the ADR does not sit at Proposed indefinitely while the code it describes is in master. |
Two findings, both Low, on 173 lines of genuinely new work. I went looking for a defect in
the new commits and did not find one.
Verification performed for this review
Detached worktree at .ai/autoreview/state/scratch/eBoot-115 off refs/pull/115/head. The
user's eBoot checkout (on fix/ed25519-low-order-keys) was not touched and is still clean;
nothing was committed or pushed.
| Check | Result |
|---|---|
cmake -B build/host -DEBLDR_BUILD_TESTS=ON -DCMAKE_BUILD_TYPE=Release |
PASS |
cmake --build build/host --parallel 4 |
PASS — no errors |
ctest --test-dir build/host --output-on-failure --no-tests=error |
PASS — 31/31, 2.52s |
pytest tests/ -q with EOS_REQUIRE_SIGNING_TESTS=1 |
PASS — 80 passed, 1.93s. (The PR comment says 79; that was measured before a276016 added the second pin test. The number is now 80.) |
Negative control on the new pin test — flipped one hex byte in tests/vectors/fw_update_test_sigs.h |
FAILS as designed — test_committed_header_is_the_generator_output fails with the regenerate command and a unified diff. The header was restored; git diff --stat clean afterwards. This is the check the previous review listed as NOT RUN. |
| Is the pin test actually reachable in CI? | YES — .github/workflows/ci.yml:48 exports EOS_REQUIRE_SIGNING_TESTS=1 before :78 runs python3 -m pytest tests/, so a missing cryptography is a hard error there and a silent skip is not possible. |
| ADR-020 ordering claim vs. the code | CONFIRMED — signature at core/fw_update.c:259, counter read at :270, eos_rollback_verify() at :277. |
| ADR numbering claim ("eos holds 012–019") | CONFIRMED — eos/docs/adr/ contains ADR-012 … ADR-019 and nothing else. |
| CHANGELOG correction ("seven suites had no Valgrind run at all") | CONFIRMED for the seven — test_eos_sign_boot_path, test_fdt_loader, test_fw_decrypt, test_fw_update_sig, test_jump_app_bounds, test_qemu_arm64_timer, test_secure_boot_policy are all absent from the 20-name hand-written foreach at tests/CMakeLists.txt:226 in 221325c. Correcting one's own PR body downward is the right instinct. |
On the rebase. 4d8696cb is not an ancestor of this head — the branch was rebased onto
master after #114 landed, so a raw 4d8696cb..a276016b diff also shows #114's governance
files (AGENTS.md, docs/wiki/*, linked-issue.yml, the PR template). Those are not this
PR's changes. The actual new work is CHANGELOG.md, docs/adr/ (2 files), the two build_*
comments, and tests/unit/test_fw_update_test_sigs.py — which is exactly what the author's
comment claims. No quiet additions rode in on the rebase.
Was anything weakened in the new commits? No. The diff is additive apart from the rebase:
no test disabled, no assertion removed, no lint loosened, no permission widened. The new
pytest module fails closed under EOS_REQUIRE_SIGNING_TESTS and only degrades to
importorskip on a developer machine, which is the same idiom
tests/unit/test_eos_sign_payload_offset.py:33 already established.
Architecture conformance
Conforms, unchanged from the previous review. §21: eBoot is Tier 1 Foundation and every
touched file is inside the owning repo. §5.1 dependency direction is untouched — the new
files are a markdown ADR, a changelog entry, two C comments and a test-only Python module;
none of them creates an edge of any kind, let alone one pointing up a tier. ADR-020 makes the
install-path ordering conform explicitly to the §8.1 principle (authenticate, then apply
version policy) that §15 leaves unstated for the update path — which is what the proposal
appended for §15.1 in proposals/2026-09.md asks the master design to say. §38's ADR series
is the right home for it.
Proposed changes
None blocking. Merge order from the previous review still stands:
1. #115 (this PR) — then #111, #109, #112, #116, #117 rebase onto it
Follow-ups, none blocking:
- ratify ADR-020 (Proposed -> Accepted) or say why not
- if ever worth it, compare the generator's signed prefix to the C suite's
rather than documenting that they must match by hand
Not checked
- Cross-compile (STM32F4, Cortex-M4), fuzz-harness build, CodeQL, cppcheck/clang-tidy —
NOT RUN locally. CI reports all green on this head; I am taking that at face value and
did not read the job logs. - ASan/UBSan — NOT RUN. I built Release without sanitizers. The body's
-DEBLDR_SANITIZE=ON
claim is unverified here. - Valgrind — NOT RUN. The restored
EBLDR_UNIT_TESTSlist is what puts the eleven suites
back under Valgrind; I verified the list, by reading it and by confirming the seven named
suites were absent from the old hand-written one. I did not execute the Valgrind tests, so
"these eleven now get a Valgrind run" is Observed from CMake, not Verified by running it. - The rest of the 09-07 batch. Unchanged from last time: my evidence supports "everything
that breaks configure, compile, link,ctestandpyteston the host is fixed", not the
stronger "this is everything that broke". - ADR-020's account of why #103 and #104 disagreed (authorship dates, who saw what) is
read from the ADR; I did not re-derive the commit chronology.
Automated architecture review of a276016bd3c3 — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.
|
Both findings accepted as recorded, and nothing pushed for them — deliberately, and here is why for each. Finding 1 (the drifted-prefix hole). Agreed on the shape of the gap: the pin test proves Finding 2 (ADR-020 Corrected a stale number of mine. The earlier comment said Not re-run here, stated rather than omitted: Valgrind, ASan/UBSan, the cross-compile legs — same as your list; CI is green on this head and I have not read those logs either. |
Twenty PRs were merged into master in ninety minutes on 09-07, each on the base it was written against. Master has not configured, compiled, linked or passed its tests since. Every nightly and every eos simulation run (which builds eBoot master) has been red for the same reason. Configure: - tests/CMakeLists.txt registered eboot_test_fdt_loader twice. embeddedos-org#84 added it, and embeddedos-org#85 -- stacked on embeddedos-org#84 -- re-added it at a different anchor when it was replayed onto a master that already had embeddedos-org#84. Same hunk as embeddedos-org#111. Compile (eboot_core): - embeddedos-org#55 restored a core/sha512.c from 02b7dac that keeps its byte count in ctx->count; the eos_sha512_ctx_t master ships has bitlen[2]/buffer_len (embeddedos-org#69/embeddedos-org#93). Back to the pre-embeddedos-org#55 file, blob-identical (d9aa57c). - embeddedos-org#55's source-list "correction" dropped core/boot_log.c, core/secure_boot.c and core/fdt_loader.c, which embeddedos-org#72/embeddedos-org#84 had added after embeddedos-org#55 was written. CMakeLists.txt is blob-identical to pre-embeddedos-org#55 again (f8fe6eb). - embeddedos-org#55 replaced the eos_boot_log_get_head() declaration with a second copy of eos_boot_log_read(); embeddedos-org#91 had already fixed the prototype it meant to fix. Header restored (964ebb8). - embeddedos-org#94 and embeddedos-org#105 each repaired the Ed25519 verifier and each added an identical static scalarbase(); both merged. One copy removed. - The same pair each added k_low_order[]/messages[] to test_ed25519.c. The embeddedos-org#105 copy is removed; embeddedos-org#94's stays because it also carries k_non_canonical[]. Tests that stopped passing because two merged PRs disagree on behaviour: - embeddedos-org#104 verifies the image signature at install unconditionally, before the anti-rollback check embeddedos-org#103 added, so embeddedos-org#103's unsigned images are refused as EOS_ERR_SIGNATURE before they can be refused as EOS_ERR_ANTI_ROLLBACK, and test_fw_transport's XMODEM install can no longer finalize. Both suites now stream genuinely signed images. eBoot has no Ed25519 signer in C, so tools/gen_fw_update_test_sigs.py signs the exact header prefixes those suites build under the RFC 8032 section 7.1 TEST 1 key and emits tests/vectors/fw_update_test_sigs.h; the suites serve that key from a simulated OTP slot 0. Negative control: one flipped signature byte fails test_write_streams_tlv_then_finalize_rejects_below_floor with EOS_ERR_SIGNATURE. - embeddedos-org#103's step 5b reads the TLV counter through the HAL slot containing the image; test_secure_boot_policy (embeddedos-org#82) declared no slots, so eos_secure_boot() returned EOS_SBOOT_ERR_BAD_HEADER two steps before the one under test. The fixture now places its image in slot A. Guards from embeddedos-org#95 that later merges walked back, never run until now because the C configure step failed first: - embeddedos-org#103 replayed the hand-written Valgrind foreach over the derived one. Restored foreach(TEST_NAME ${EBLDR_UNIT_TESTS}); eleven registered suites had no list(APPEND ...) and so no Valgrind run. - Seven suites assign tests_run = <literal> and their TEST() does not count; four suites have no TEST() macro at all. Counted, and classified. - embeddedos-org#101 added fuzz-build after embeddedos-org#90's gate; the gate did not wait for it. CI plumbing: - eosim-sanity.yml: the install-validate job is written in bash but ran under PowerShell on the Windows legs (no shell:), where SITE_PACKAGES=$(...) is an unknown command and `|| { exit 1 }` is an unexecuted script block. - scorecard.yml: ossf/[email protected] pulls gcr.io, which now demands GCP billing. v2.4.3 pulls ghcr.io; eos already pins it and is green. Verified locally (macOS, clang): Release build clean, 31/31 ctest; the same under -DEBLDR_SANITIZE=ON (ASan+UBSan); 78/78 pytest. Not fixed here, reported separately: core/keystore.c's compiled-in default_dev_key is described as the RFC 8032 TEST 1 public key but differs from byte 21 on and is not a point on the curve, so nothing can verify against it on any board without OTP. With embeddedos-org#104 that makes firmware update refuse every image on such boards.
… -1.0.0 as an option test_out_of_range_version_is_rejected[-1.0.0] passed locally (Python 3.14) and failed in CI (ubuntu-22.04, Python 3.10) with argparse's own "expected one argument": the older negative-number matcher does not accept -1.0.0, so the token was read as an unknown option and imgpack.py's range check -- the thing under test -- never ran. The joined form is unambiguous on every interpreter and the test now reaches the tool's message.
tests/vectors/fw_update_test_sigs.h is the output of tools/gen_fw_update_test_sigs.py, committed because eBoot has no Ed25519 signer in C. Nothing checked that the two agree: a generator edit without a regeneration leaves test_fw_update and test_fw_transport verifying against stale signatures, failing with EOS_ERR_SIGNATURE and nothing to say why. tests/unit/test_fw_update_test_sigs.py runs the generator with the test's own interpreter and compares its stdout to the committed header byte for byte, so a line-ending change counts too. It follows the same dependency rule as test_eos_sign_payload_offset.py: with EOS_REQUIRE_SIGNING_TESTS set (the CI workflow sets it before the pytest step) a missing cryptography module fails the job instead of skipping. Negative control: one flipped hex byte in the header fails the test with a unified diff naming the line. build_image() and build_container() now carry a comment naming the coupling: the signed prefix is assembled both there and in the generator, and changing any field in it means changing the generator's copy and regenerating the header.
…epair embeddedos-org#103 and embeddedos-org#104 were both merged and disagree on whether the anti-rollback counter or the signature is checked first in eos_fw_update_finalize(). The master design orders boot as verify image, then version policy (section 8.1), but its update flow (section 15) never places the anti-rollback check, so the order the install path uses existed only in a PR body. ADR-020 records it: the signature over the signed header prefix is verified first, the TLV counter is read only after the prefix that binds it is authenticated, and an image that fails verification is refused as EOS_ERR_SIGNATURE without its counter being consulted. docs/adr/README.md is added in the shape of the eos repository's index; 020 avoids reusing 001 through 019. CHANGELOG.md gains the Unreleased entries for the repair: the configure, compile and link breakage after the 09-07 batch merge, the settled check ordering with the suites streaming signed images, the re-derived Valgrind list, fuzz-build in the CI gate, counted tests_run, the EoSim Windows legs running under bash, and the Scorecard action on its ghcr.io-hosted release.
The changelog said eleven suites had no Valgrind run. Eleven were missing from EBLDR_UNIT_TESTS, but four of those were named in the hand-written foreach and did run; seven had no run at all. Say which. ADR-020: its design-document citation now says where it comes from (the architecture review of embeddedos-org#115), and the sentence about the two PRs' bases is replaced with what the history shows -- embeddedos-org#103's commits predate embeddedos-org#104's merge, and embeddedos-org#104 was written without embeddedos-org#103's check in place.
The regeneration test's failure path -- the unified diff under the regenerate command -- ran only when the header was stale, so a green run never executed it and the coverage report said so. The report is now a helper the match asserts with, and a second test drives the helper with two byte strings that differ in one byte and checks the command and both sides of the changed line appear.
a276016 to
e152d8e
Compare
…warded Finding 1 from the review of 88cb92e. _forwarded_definitions() matched add_compile_definitions(EBLDR_X) with the closing paren required to follow the name, so of the three ordinary ways to forward a compile definition it saw one. The reviewer showed the other two carrying a name nothing reads straight past it: add_compile_definitions(EBLDR_DEAD_A=1) -> 4 passed target_compile_definitions(eboot_core PRIVATE EBLDR_DEAD_B) -> 4 passed add_compile_definitions(EBLDR_DEAD_C) -> 1 failed The =1 form is what someone writes when they want the macro to have a value; target_compile_definitions is what they write when they want it scoped to one library. Neither is exotic, and this guard exists only for the future -- the tree forwards exactly one EBLDR_ name today and it is read. The collector now takes the whole argument list of add_ or target_compile_definitions(...), every EBLDR_ token in it, with anything after = stripped. Five shapes are pinned as collected, including a multi-line call and a mixed list, and a second test appends each shape to the real CMakeLists.txt text with a dead name and requires the guard to fail on it -- which is exactly how a dead switch would come back. All three of the reviewer's probes now fail the guard. The secondary note is taken too: test_documented_options_exist checked documented <= declared; the reverse direction is now asserted as well, with EBLDR_BOARD exempted because the README covers it in prose above the table rather than in it. Clean today, as the reviewer enumerated. Negative control: with the old one-shape collector restored and the new tests kept, 5 of 11 fail. At this head (restacked onto embeddedos-org#115's e152d8e, itself on master 682d005): pytest tests/ 91 passed, ctest 31/31.
…warded Finding 1 from the review of 88cb92e. _forwarded_definitions() matched add_compile_definitions(EBLDR_X) with the closing paren required to follow the name, so of the three ordinary ways to forward a compile definition it saw one. The reviewer showed the other two carrying a name nothing reads straight past it: add_compile_definitions(EBLDR_DEAD_A=1) -> 4 passed target_compile_definitions(eboot_core PRIVATE EBLDR_DEAD_B) -> 4 passed add_compile_definitions(EBLDR_DEAD_C) -> 1 failed The =1 form is what someone writes when they want the macro to have a value; target_compile_definitions is what they write when they want it scoped to one library. Neither is exotic, and this guard exists only for the future -- the tree forwards exactly one EBLDR_ name today and it is read. The collector now takes the whole argument list of add_ or target_compile_definitions(...), every EBLDR_ token in it, with anything after = stripped. Five shapes are pinned as collected, including a multi-line call and a mixed list, and a second test appends each shape to the real CMakeLists.txt text with a dead name and requires the guard to fail on it -- which is exactly how a dead switch would come back. All three of the reviewer's probes now fail the guard. The secondary note is taken too: test_documented_options_exist checked documented <= declared; the reverse direction is now asserted as well, with EBLDR_BOARD exempted because the README covers it in prose above the table rather than in it. Clean today, as the reviewer enumerated. Negative control: with the old one-shape collector restored and the new tests kept, 5 of 11 fail. At this head (restacked onto embeddedos-org#115's e152d8e, itself on master 682d005): pytest tests/ 93 passed, ctest 31/31.
srpatcha
left a comment
srpatcha
left a comment
There was a problem hiding this comment.
Review — eBoot#115 "fix: repair master after the 09-07 batch merge"
head: e152d8e author: Kartikey1306 ci: pass (26 green, 0 red; only Create GitHub Release skipped)
Verdict: Third look. This head is a pure rebase of the previously reviewed
a276016b onto the new master — no new commits, and the only content delta is the
tests/CMakeLists.txt hunk that dropped out because #111 landed, exactly as the last
review predicted. Both outstanding findings are Low and remain open by the author's
deliberate choice; one of the two stated reasons for deferring no longer holds. Build,
ctest and pytest re-verified green on this head. Nothing here blocks the merge.
Status of the previous findings (eBoot-115-a276016b.md)
| # | Prev. severity | Status | Evidence |
|---|---|---|---|
| 1 | Low — the pin test closes the hand-edited-header hole, not the drifted-prefix hole | Untouched, accepted, deferred | No change on the branch; tools/gen_fw_update_test_sigs.py:25-27 still states the field values are copied from the tests. The author accepted the analysis and the proposed close in the 2026-09-14T20:21Z comment and queued it as a follow-up rather than a change to this PR. Recorded as open, not dropped. |
| 2 | Low — ADR-020 is status: Proposed while CHANGELOG.md calls the ordering "settled" |
Untouched; the deferral reason is now stale | docs/adr/ADR-020-…md:4 is still status: Proposed; CHANGELOG.md:17 still reads "Install-path verification order settled". See finding 1 below. |
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Low | CHANGELOG.md:17 |
The stated reason for not pushing the one-word correction has been overtaken by this head. The author's 2026-09-14T20:21Z comment says the fix is held back because "#116 and #122 are stacked on this branch and a push here re-stacks both and stales the review ranges stated in their bodies". That re-stack has now happened anyway: a276016b is no longer reachable, git merge-base pr/115 pr/116 and git merge-base pr/115 pr/122 both return e152d8ed, and both stacked PRs already carry new heads in this same review batch. The cost the deferral was avoiding has already been paid, so the word can ride the next push at zero additional cost. Still Low — this is a changelog adjective, and the underlying ADR ratification is the deciders' call, not the author's. |
In the next push to this branch (or in the squash), change "settled" to a word that matches docs/adr/README.md's definition of Proposed — e.g. "recorded" or "proposed". Leave status: alone; ratification belongs to the named deciders. |
One finding, Low, on a rebase that introduced no new source. I re-read the new head's
delta specifically looking for anything that rode in on the rebase and found none — see
below.
Verification performed for this review
Detached worktree at .ai/autoreview/state/scratch/eBoot-115 off refs/pull/115/head.
The user's eBoot checkout (on fix/ed25519-low-order-keys) was not touched; nothing was
committed or pushed.
| Check | Result |
|---|---|
cmake -B build/host -DEBLDR_BUILD_TESTS=ON -DCMAKE_BUILD_TYPE=Release |
PASS |
cmake --build build/host --parallel 4 |
PASS — no errors |
ctest --test-dir build/host --output-on-failure --no-tests=error |
PASS — 31/31, 2.49s |
pytest tests/ -q with EOS_REQUIRE_SIGNING_TESTS=1 |
PASS — 82 passed, 2.07s. Up from 80 at the last review; the two extra are tests/unit/test_sign_image.py cases that arrived on the new base from #117, not from this branch. |
| Is this head a rebase or new work? | REBASE, confirmed. origin/master..pr/115 lists the same six commit subjects in the same order as a276016b's range. Per-commit git patch-id --stable over both ranges differs on exactly one commit (b7d7cac → 35280b5). |
| What changed in that one commit | CONFIRMED benign. Diffing (221325c..a276016b) against (682d005..e152d8ed) yields only the removal of the five-line eboot_test_fdt_loader deletion hunk in tests/CMakeLists.txt plus the resulting @@ line-number shifts and a blob-index line. #111 (682d005) removed the duplicate on master, so the hunk is now redundant. Totals move 191 → 186 deletions; insertions are unchanged at 702. |
| Duplicate target actually gone at this head | CONFIRMED — grep -c 'add_executable(eboot_test_fdt_loader' tests/CMakeLists.txt = 1. The configure failure the original PR fixed cannot recur from this file. |
| Did anything else ride in on the rebase? | NO. The 25 files the PR touches are unchanged from the reviewed set apart from the hunk above. The a276016b..e152d8ed raw range additionally shows build.yml, nightly.yml, release.yml, weekly.yml, tools/sign_image.py and tests/unit/test_sign_image.py — those are #117 (f22896d) arriving on the new base, not this PR's changes. |
| Deferral-reason check for finding 1 | CONFIRMED stale — git merge-base --is-ancestor e152d8ed pr/116 and … pr/122 both true. |
| Residual 1 still present | CONFIRMED — tools/gen_fw_update_test_sigs.py unchanged on this head. |
| Residual 2 still present | CONFIRMED — ADR-020 frontmatter status: Proposed; CHANGELOG.md:17 "settled". |
Was anything weakened? No. There is no new source on this branch since the last
review, and the one hunk that disappeared disappeared because master already performs
the same removal. No test disabled, no assertion removed, no lint loosened, no permission
widened, and the test count went up (31 ctest unchanged, pytest 80 → 82).
On the author's self-corrections. Two numbers in the PR comments are now stale in the
harmless direction: "pytest 80 passed" was measured at a276016b and is 82 at this head.
That is the base moving, not a claim the author got wrong, and no action is needed. It is
noted only so the next reader does not treat the mismatch as a discrepancy.
Architecture conformance
Conforms; unchanged from the previous two reviews and re-checked rather than recalled.
§21: eBoot is Tier 1 Foundation and every touched file is inside the owning repo. §5.1
dependency direction is untouched — a rebase that removes a CMake deletion hunk creates no
edge of any kind. The core/ placement of boot_log.c, secure_boot.c and fdt_loader.c
restored by the original commit still matches .ai/architect.md's target shape for eBoot
(core/ = shared boot logic), and §5.1's "eBoot keeps the trusted computing base minimal
and auditable" is still served by having those objects actually link. §8.1's boot ordering
is unchanged. ADR-020 continues to record the install-path ordering that §15 leaves
unstated; the corresponding master-design gap is already filed as the 2026-09-13 proposal
"The update path does not say where anti-rollback is evaluated" in
.ai/autoreview/proposals/2026-09.md, so no new proposal is appended for this head.
Proposed changes
None blocking. Merge order from the previous reviews holds, with #111/#112/#117 now landed:
1. #115 (this PR) — #116 and #122 are stacked on it and rebase for free
Follow-ups, neither blocking:
- CHANGELOG.md:17 "settled" -> "recorded"/"proposed" on the next push to this branch
- ratify ADR-020 (Proposed -> Accepted) or say why not [deciders, not the author]
- if ever worth it, compare the generator's signed prefix against the C suite's bytes
rather than documenting by hand that they must match
Not checked
- Cross-compile (STM32F4, Cortex-M4), fuzz-harness build, CodeQL, cppcheck/clang-tidy —
NOT RUN locally. CI reports all green on this head; taken at face value, job logs not read. - ASan/UBSan — NOT RUN. Built Release without sanitizers on this head.
- Valgrind — NOT RUN. The restored
EBLDR_UNIT_TESTSlist is Observed from CMake, as in
the previous review; not executed here. - Negative control on the pin test — NOT RE-RUN. It was executed and passed at
a276016b;tests/unit/test_fw_update_test_sigs.pyand the vectors are byte-identical at
this head, so the earlier result carries, but I did not re-break the header this time. - The rest of the 09-07 batch. Unchanged: the evidence supports "everything that breaks
configure, compile, link,ctestandpyteston the host is fixed", not "this is
everything that broke". - CI on this exact head vs. the reported checks. I read
checks.txtfrom the bundle; I
did not re-query GitHub or open any job log.
Automated architecture review of e152d8edfce4 — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.
…warded Finding 1 from the review of 88cb92e. _forwarded_definitions() matched add_compile_definitions(EBLDR_X) with the closing paren required to follow the name, so of the three ordinary ways to forward a compile definition it saw one. The reviewer showed the other two carrying a name nothing reads straight past it: add_compile_definitions(EBLDR_DEAD_A=1) -> 4 passed target_compile_definitions(eboot_core PRIVATE EBLDR_DEAD_B) -> 4 passed add_compile_definitions(EBLDR_DEAD_C) -> 1 failed The =1 form is what someone writes when they want the macro to have a value; target_compile_definitions is what they write when they want it scoped to one library. Neither is exotic, and this guard exists only for the future -- the tree forwards exactly one EBLDR_ name today and it is read. The collector now takes the whole argument list of add_ or target_compile_definitions(...), every EBLDR_ token in it, with anything after = stripped. Five shapes are pinned as collected, including a multi-line call and a mixed list, and a second test appends each shape to the real CMakeLists.txt text with a dead name and requires the guard to fail on it -- which is exactly how a dead switch would come back. All three of the reviewer's probes now fail the guard. The secondary note is taken too: test_documented_options_exist checked documented <= declared; the reverse direction is now asserted as well, with EBLDR_BOARD exempted because the README covers it in prose above the table rather than in it. Clean today, as the reviewer enumerated. Negative control: with the old one-shape collector restored and the new tests kept, 5 of 11 fail. At this head (restacked onto embeddedos-org#115's e152d8e, itself on master 682d005): pytest tests/ 93 passed, ctest 31/31.
|
Answering the 06:41Z re-review at The deferral reason you quoted was stale, and you were right to say so. "#116 and #122 are stacked on this" was the reason on 09-14; the restack at 12:05 IST paid that cost anyway. The reason today is a different number: twelve open PRs now sit on ADR-020 ratification is the deciders' call, as you say; nothing from me there. Body's "Relationship to open PRs" was updated at 12:05 IST to record the rebase and #111/#112/#117 with SHAs. Nothing pushed; nothing to re-approve. |
|
Status for the merger, 2026-09-17: green on all checks, rebased onto |
|
@srpatcha — re-approval request, with the facts that make it a two-minute read rather than a re-review. Your approval at The repair commit you approved got smaller, not different. Then What is new since your approval: 5 follow-up commits, +187/−1 across 7 files, each answering a finding from your automated review — What this unblocks: 12 open PRs are stacked on this head (#116, #122, #123, #125, #127, #129, #131, #133, #135, #138, #139, #142 — each drops to its own diff once this lands), plus Mohammed18-19's #109, whose only red is inherited from master. Nothing further will be pushed to this branch. Merge order: this first. |
|
Fuck you 🍆
…On Thu, Sep 17, 2026, 10:23 Kartikeydheer Srivastava < ***@***.***> wrote:
*Kartikey1306* left a comment (embeddedos-org/eBoot#115)
<#115 (comment)>
@srpatcha <https://github.com/srpatcha> — re-approval request, with the
facts that make it a two-minute read rather than a re-review.
*Your approval at 4d8696c (2026-09-14 05:16Z) was dismissed by follow-up
pushes answering the automated review.* The branch has not moved since
e152d8e (2026-09-15 06:30Z).
*The repair commit you approved got smaller, not different.* Then a8743dd
(20 files, +515/−190), now 35280b5 (20 files, +515/−185): the only change
is five lines of tests/CMakeLists.txt the rebase dropped because #111
<#111> — merged to master on
09-14 — removed the same duplicate test_fdt_loader target this repair
removed. Same 20 files, same additions; what remains is a strict subset of
what you verified with gcc/Ninja/Debug.
*What is new since your approval: 5 follow-up commits, +187/−1 across 7
files*, each answering a finding from your automated review — 2b30fd7 (
--version=X so argparse never reads -1.0.0 as an option), a95400f
(fixture header pinned to its generator), e152d8e (exercise the
stale-header report, not only the match), edfbaed/b01e9ce (ADR-020 for
the install-path check ordering; the Valgrind gap stated exactly). Compare:
4d8696c...e152d8e
<4d8696c...e152d8e>
— the automated review's final verdict on this head: *"This head is a
pure rebase of the previously reviewed"* content.
*What this unblocks:* 12 open PRs are stacked on this head (#116
<#116>, #122
<#122>, #123
<#123>, #125
<#125>, #127
<#127>, #129
<#129>, #131
<#131>, #133
<#133>, #135
<#135>, #138
<#138>, #139
<#139>, #142
<#142> — each drops to its
own diff once this lands), plus Mohammed18-19's #109
<#109>, whose only red is
inherited from master.
Nothing further will be pushed to this branch. Merge order: this first.
—
Reply to this email directly, view it on GitHub
<#115?email_source=notifications&email_token=BFUJGLRRL4EUWXTLZBNXWLD5POUQRA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKNZRGIYDGMJQGQ2KM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-5712031044>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BFUJGLRTLZ75SDH36NWZUE35POUQRAVCNFSNUABGKJSXA33TNF2G64TZHMYTCOJRGA4TONJQGE5US43TOVSTWNJUGIZTIOBTGMYDFILWAI>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/BFUJGLXWKTD6MVDWMPMYS6L5POUQRA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKNZRGIYDGMJQGQ2KM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJKTGN5XXIZLSL5UW64Y>
and Android
<https://github.com/notifications/mobile/android/BFUJGLUKKLEWCEICC7CIDRL5POUQRA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKNZRGIYDGMJQGQ2KM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLTGN5XXIZLSL5QW4ZDSN5UWI>.
Download it today!
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
This branch has not been deployed
4 participants
For the merger (2026-09-17):
master682d005failsCI — eBoot(34898627047):core/sha512.cuses acountmembereos_sha512_ctx_tno longer has (6 sites) andcore/ed25519_verify.c:303redefinesscalarbase, so every PR branched from it fails the six build-side checks — currently #109 (@Mohammed18-19, 34471772092). This PR's head passesCI — eBoot(34937012090), is 0 behindmaster, and nothing needs to land before it; it is the base of #116 → #122 → #123 → #125 and of #127/#129/#131/#133/#138, all green on top of it. Merging it takesmastergreen and lets #109 go green on a rebase (merge result built and tested: 31/31).Problem
eBoot master has not configured, compiled, linked or passed its tests since the 09-07 batch merge (twenty PRs in ninety minutes, each replayed onto a master its author never saw). Every
CI — eBoot,eBoot Build & Test,CodeQLand nightly run on master is red, and becauseeos/.github/workflows/eos-simulation.ymland eos's nightlyUpstream driftbuild eBoot atmaster, eos is red on those jobs too, regardless of what an eos PR contains.Verified against run 34177770756 (
CI — eBootonb429354) and 34177770734 (eBoot Build & Test), and reproduced locally on master before every fix below.What broke, and which merge broke it
add_executable cannot create target "eboot_test_fdt_loader"11f2f3f'eos_sha512_ctx_t' has no member named 'count'×5core/sha512.cfrom02b7dacthat predates thebitlen[2]/buffer_lencontext (#69/#93)d9aa57c)redefinition of 'scalarbase'static scalarbase(); both mergedredefinition of 'k_low_order'/'messages'intest_ed25519.ck_non_canonical[]_eos_boot_log_append,_eos_secure_boot,_eos_secure_boot_lock_debugundefinedcore/boot_log.c,core/secure_boot.c,core/fdt_loader.c(added by #72/#84 after #55 was written)CMakeLists.txtblob-identical to pre-#55 (f8fe6eb)eos_boot_log_get_headundeclared intest_boot_log.ceos_boot_log_read(); #91 had already fixed the prototype #55 meant to fix964ebb8)Three suites then compiled and failed, because two merged PRs disagree on behaviour:
EOS_ERR_SIGNATUREbefore they can be refused asEOS_ERR_ANTI_ROLLBACK, andtest_fw_transport's XMODEM install can no longer finalize. fix(fw-update): verify the image signature unconditionally at install #104's ordering is right (a counter is only trustworthy after the prefix that binds it is authenticated), so the tests now stream genuinely signed images. eBoot has no Ed25519 signer in C, sotools/gen_fw_update_test_sigs.pysigns the exact header prefixes those suites build under the RFC 8032 §7.1 TEST 1 key and emitstests/vectors/fw_update_test_sigs.h; the suites serve that key from a simulated OTP slot 0, the patheos_keystore_init()takes on a provisioned board. Negative control (verified): one flipped signature byte failstest_write_streams_tlv_then_finalize_rejects_below_floorwithEOS_ERR_SIGNATURE.test_secure_boot_policy(fix(secure-boot): a debug lock that failed must not report a successful boot #82) declared no slots, soeos_secure_boot()returnedEOS_SBOOT_ERR_BAD_HEADER(−3) two steps before the one under test. The fixture now places its image in slot A.Guards from #95 that later merges walked back, invisible until now because configure failed first (observed locally:
pytest tests/on master-with-the-C-fixes was 5 failed / 73 passed):foreachover the derived one. Restoredforeach(TEST_NAME ${EBLDR_UNIT_TESTS}); eleven registered suites had nolist(APPEND ...)and so no Valgrind run.tests_run = <literal>and theirTEST()does not count; four suites have noTEST()macro. Counted, and classified inNO_TEST_MACROwith a reason each.fuzz-buildafter ci: add one job branch protection can require #90's gate;CI Gatedid not wait for it.One more, found on this PR's first run (run 34591438682):
test_imgpack.py::test_out_of_range_version_is_rejected[-1.0.0]passes on Python 3.13+ and fails on ubuntu-22.04's 3.10, because the older argparse negative-number matcher reads-1.0.0as an unknown option and rejects it beforeimgpack.py's own range check runs. The test now passes--version=X, which every interpreter reads as a value; verified the tool's message is what the test then sees.CI plumbing on the same red runs:
eosim-sanity.yml: theinstall-validatejob is written in bash ($(...),|| { ... }, a heredoc,/tmp) but ran under PowerShell on the Windows legs, whereSITE_PACKAGES=$(...)is an unknown command — and where the|| { exit 1 }guard in "Verify installation" parses as an unexecuted script block and could never fail.defaults.run.shell: bashon the job. (Run 34561593562.)scorecard.yml:ossf/[email protected]pullsgcr.io/openssf/scorecard-action, and gcr.io now refuses with "This API method requires billing to be enabled".v2.4.3pulls from ghcr.io; eos already pins it and its Scorecard job is green. (Run 34177770753.)Test plan
Verified, macOS / clang:
Not verified here: the STM32F4 / Cortex-M4 cross build (no arm-none-eabi toolchain on this machine) and the fuzz harness build (no libFuzzer runtime). Both failed on master for the compile errors above, which are gone; CI on this PR is the check.
Relationship to open PRs
682d005. Thetests/CMakeLists.txthunk here was byte-identical to it, so that half of this PR is now on master and drops out of this diff on merge. Verified rather than assumed: this branch merged onto682d005leaves exactly onetest_fdt_loaderregistration, configures with 0 errors, builds, and passes ctest 31/31. Rebased onto682d005at 12:05 IST 2026-09-15 (heada276016→e152d8e, six commits replayed with--onto, the CMake hunk dropped out as expected — onetest_fdt_loaderregistration in the tree). Verified on the new head before the push: configure 0 errors, build OK, ctest 31/31, pytest 55 passed / 3 skipped. This corrects the previous "Not rebased" statement here: the hold was for the active merge window overnight; nothing has merged since 02:54 IST and the PR is the base of the stack, so it goes first and the dependents follow in the same window.50c66af. Touches only workflow files this PR does not; no overlap.f22896d. No overlap with this PR.682d005still does not compile without this PR:core/sha512.ccount(×6) and thescalarbaseredefinition are unchanged by the three merges above.Reported, not fixed here
core/keystore.c's compiled-indefault_dev_keyis described (in a#warningand in comments, including one in my owntest_secure_boot_policy.c) as the RFC 8032 TEST 1 public key. It is not: it diverges from byte 21 on and does not decode to a point on the curve (verified by decoding y and checking x² against the curve equation). Nothing can ever verify against it, and since #104 that means firmware update refuses every image on a board without OTP. That is a defect in its own right, unrelated to the batch merge, and is why the tests here provision the key through simulated OTP rather than rely on the fallback. Separate PR to follow.Review follow-ups (2026-09-14)
Rebased onto the governance commit and the review findings applied, each as its own commit:
tests/unit/test_fw_update_test_sigs.pyrunstools/gen_fw_update_test_sigs.pyand asserts its output equals the committedtests/vectors/fw_update_test_sigs.hbyte for byte (finding 3; approver's ask). Verified it fails on a one-byte edit of the header, with a unified diff naming the regenerate command. Comments abovebuild_image()/build_container()name the coupling and the command.docs/adr/ADR-020-install-path-verifies-signature-before-anti-rollback.md(+docs/adr/README.md) records the ordering this PR settles: signature verification over the signed prefix precedes anti-rollback evaluation ineos_fw_update_finalize()(finding 1).test_imgpack.py:--version=Xform (the 3.10 argparse difference).copy >> 61term (finding 2) is fix(sha512): drop a carry term that is always zero #123, kept separate socore/sha512.chere stays blobd9aa57c.One correction to the text above: "eleven registered suites had no
list(APPEND …)and so no Valgrind run" overstated it — eleven were missing fromEBLDR_UNIT_TESTS, but four of them were in the hand-writtenforeachand did run; seven (test_eos_sign_boot_path,test_fdt_loader,test_fw_decrypt,test_fw_update_sig,test_jump_app_bounds,test_qemu_arm64_timer,test_secure_boot_policy) had no Valgrind run at all. The CHANGELOG says it that way.Verified on the current head: Release build clean,
ctest31/31,pytest tests/79 passed (78 + the new test), also withEOS_REQUIRE_SIGNING_TESTS=1.Closing issue
Fixes #118