fix(sign_image): --genkey writes the key the build reads, not a header nothing includes - #142
Open
Kartikey1306 wants to merge 23 commits into
Open
Kartikey1306 wants to merge 23 commits into
Kartikey1306 wants to merge 23 commits into
Conversation
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/scorecard-action@v2.4.0 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.
…e curve core/keystore.c's default_dev_key claims, in a #warning and in comments, to be the RFC 8032 section 7.1 TEST 1 public key. From v0.1.0 it agreed with the RFC for 21 bytes and then did not, and the bytes it held do not decode to a point on edwards25519. No signature could ever verify against it, on any board that fell back to it; after embeddedos-org#104 made signature verification unconditional at install, that meant firmware update refused every image on every board without OTP. Nothing noticed because no test ever asked the fallback key to verify anything. The array is now the RFC's 32 bytes. test_keystore.c asks the compiled-in anchor to verify the RFC's own TEST 1 signature over the empty message, and then shows the accept discriminates (wrong message, flipped bit). On the old bytes the first verify returns EOS_ERR_SIGNATURE. Also corrects the note in test_secure_boot_policy.c that repeated the claim.
… and record the behaviour change The unit test that checks the compiled-in development trust anchor carried its own hand-typed copy of the RFC 8032 TEST 1 public key, the third such copy in the tree. tests/vectors/fw_update_test_sigs.h already holds that key as eos_test_sig_pubkey: it is derived from the RFC secret by tools/gen_fw_update_test_sigs.py and pinned to the generator by tests/unit/test_fw_update_test_sigs.py. The test now includes the fixture and compares against it, so the anchor is checked against a second, independently derived copy of the key instead of bytes that could have been mistyped the same way. The signature constant and the four assertions are unchanged; on the old anchor bytes the memcmp still fails. The CHANGELOG gains a Security entry for the anchor fix and states the consequence plainly: a board with no OTP and no EBLDR_PRODUCTION_KEY moves from refusing every image, as it has since embeddedos-org#104, to accepting images signed with the public RFC test key. That is what the #warning in core/keystore.c has always said a development build does, and the key must never reach a device; embeddedos-org#120 tracks making that structural.
… production trust anchor core/keystore.c falls back to a compiled-in public key when the board has no OTP -- and no board under boards/ implements otp_read, so on every shipped board the compiled-in key is the trust anchor. EBLDR_PRODUCTION_KEY had no CMake option, nothing set it, and its #else branch declared an extern that nothing defined: following the #warning's own instruction produced a link error. Every artifact ever built therefore carried the RFC 8032 test key, whose secret is published. Now -DEBLDR_PRODUCTION_KEY=<64 hex characters> is checked (exactly 64 hex, and not the development key), turned into build/generated/production_key.c defining ebldr_production_key[] (declared in include/eos_production_key.h), compiled into eboot_core, and selects the production branch of keystore.c. A Release build of a real board that sets no key refuses to configure and says how to proceed: give a key, or pass -DEBLDR_ALLOW_DEV_KEY=ON for a bring-up or CI build that will never reach a device. Host builds and Debug builds are not gated, so development is unchanged apart from the existing #warning. Verified on this host: Release + stm32f4 with no key -> FATAL_ERROR with the gate message as the first error; the same with EBLDR_ALLOW_DEV_KEY=ON -> no gate message; the development key, and abc, as the production key -> refused; the RFC 8032 TEST 2 public key -> eboot_core builds without the keystore #warning, nm shows ebldr_production_key defined and default_dev_key absent.
…embeds the development key
Every board configure in release.yml -- the stm32f4/stm32h7/nrf52 matrix,
rpi4, riscv64_virt, both esp32 and both esp32c3 lines, x86_64_efi -- now
passes -DEBLDR_PRODUCTION_KEY="${{ secrets.EBLDR_PRODUCTION_KEY_HEX }}".
With the secret unset the value is empty and CMake refuses to configure, so
a release cannot be cut on the development key until a maintainer provides
the anchor. That is the intended shape.
After each firmware build a step scans every .elf, .bin, .a and .o under
build/ for the development key's 32 bytes and fails the job on a hit,
printing the file. Verified the snippet against a file containing the bytes
(hit, exit 1) and a clean one (exit 0).
ci.yml's ARM cross-compile is a Release build of stm32f4 and would now trip
the gate; it passes -DEBLDR_ALLOW_DEV_KEY=ON with a comment saying why: it
checks that the tree cross-compiles and nothing it produces reaches a device.
…roduction keystore tests/unit/test_keystore_production.c compiles core/keystore.c with EBLDR_PRODUCTION_KEY and a fixture key (RFC 8032 TEST 2, generated by the same cmake/ProductionKey.cmake function the real build uses), so the production branch -- the one a release is built from, which had never been compiled -- is built and run on every host build. It asserts the generated symbol carries the configured bytes, that a board without OTP gets that key as its active anchor from the compiled-in source, and that the key is not the development key. Flipping one fixture byte fails it; configuring the fixture as the development key is refused by CMake. tests/unit/test_production_key_gate.py runs real configures: Release + board with no key fails with the gate message as the first error; the opt-out clears it; Debug and host builds are not gated; the development key (either case) and four malformed values are refused; a real key generates the anchor source with exactly those bytes. tests/unit/test_release_workflow_production_key.py parses the workflows: all 8 board configures in release.yml carry the secret and none the opt-out; all 6 firmware jobs scan after building and before collecting; the bytes the scan names are the bytes core/keystore.c and cmake/ProductionKey.cmake name; and no workflow anywhere cross-compiles a Release board without a key or the opt-out. Each property was checked by mutation: dropping the flag from one esp32c3 line, changing one scan hex digit, and removing ci.yml's opt-out each fail the suite.
docs/key_lifecycle.md described a header scheme (eos_signing_key.h, EOS_BUILD_PRODUCTION/STAGING) that does not exist in the tree and a development key that was not the one compiled in. Sections 3.1, 7.2 and 7.4 now describe what core/keystore.c, cmake/ProductionKey.cmake and release.yml actually do, name the RFC 8032 TEST 1 pair as the development key, and quote the gate's message.
…hes run on a green tree The check that no workflow cross-compiles a Release board without a key or the opt-out only ever took its skip branches on the real workflows, so a green run never executed the branch that reports a violation and the coverage report said so. The check is now a helper returning what it would flag, and a second test feeds it a synthetic workflow: one Release + real-board job with neither flag is reported, and the keyed, opted-out, Debug, host, no-board and not-a-job cases are not. That is the mutation check from the review, kept as a test.
…ifier would Two findings from the review at 6a1d21f, both in the gate rather than the mechanism, both closed before this merges because this PR is what makes the anchor load-bearing. The gate matched the literal string "Release" and nothing else. Real configures of stm32f4 with RelWithDebInfo, MinSizeRel, "release", "RELEASE" and no CMAKE_BUILD_TYPE at all every one configured successfully and printed the development anchor as a STATUS line. MinSizeRel is the ordinary build type for a bootloader. Only Debug is not release-shaped -- every other value is optimised, an unset value on a cross build still gets -Os from this file, and a multi-config generator has no build type at configure time -- so the gate now asks "is this Debug?" and refuses everything else. The error names the build type it saw, or says there was none. EBLDR_PRODUCTION_KEY was checked for length and hex-ness and for not being the development key, and never for being a point on the curve -- the exact defect the development key had until embeddedos-org#116, now reachable through one mistyped hex digit in a release secret: green build, green artifact scan, a status line saying "production key", and a fleet whose bootloader refuses every image it is ever offered. tools/check_production_key.py applies core/ed25519_verify.c's own acceptance rule in pure Python with no dependencies: decode per RFC 8032 5.1.3, [L]P == identity, P != identity, not the development key. cmake/ProductionKey.cmake runs it at configure when python3 is found and warns, naming what was not checked, when it is not. release.yml runs it on the secret in the validate job every firmware job needs, so a bad secret fails the release before a board is configured. docs/key_lifecycle.md no longer says "never"; it says what the gate covers and what it cannot see (a Debug build flashed to a device, an explicit EBLDR_ALLOW_DEV_KEY=ON, a fork that removes the gate). Verified by execution: configure, real board, no key, each of Release / RelWithDebInfo / MinSizeRel / release / RELEASE / unset -> refused by the gate Debug, debug -> configures MinSizeRel + EBLDR_ALLOW_DEV_KEY=ON -> configures host build, no board -> configures, no gate key = the pre-embeddedos-org#116 off-curve bytes -> refused, "no point" key = the order-2 point ec ff..7f -> refused, "low order" key = RFC 8032 TEST 2 -> accepted tools/check_production_key.py against 15 vectors, each refusal checked for its stated reason -- two of my first vectors were 66 characters and were being refused for length, which the reason check caught. negative controls: gate reverted to the literal "Release" -> 3 tests fail; the curve check's result ignored -> 3 tests fail. Restored. pytest 121 passed; host build rc=0; ctest 32/32. Not done here, and stated: the six copies of the artifact scan in release.yml (review finding 3, Low) are unchanged -- lifting them into one tool is a follow-up, not a merge blocker.
The previous commit made the trust-anchor gate refuse every build of a real board that is not Debug, including one with no CMAKE_BUILD_TYPE. build.yml's "Cross-compile STM32F4" job is exactly that: -DEBLDR_BOARD=stm32f4 with no build type and no opt-out, so the gate refused it and the job went red on this PR. The gate was right and the reply on this PR was wrong -- it said no CI leg was affected, having looked for lines that set CMAKE_BUILD_TYPE and not for jobs that set none. The job is a compile check whose output reaches no device, the same shape as ci.yml's ARM leg, which already passes -DEBLDR_ALLOW_DEV_KEY=ON. It now does too, with the comment that travels with every copy of that flag: never use it in a workflow that publishes an artifact. Verified: configure of stm32f4 with no build type and the opt-out passes the gate and reports the development anchor; the gate and release-workflow test modules still pass. Every other board-configuring job was audited by grep for EBLDR_BOARD= and carries Debug, the opt-out, or a production key.
…e of six copies Finding 3 from the review of 6a1d21f, the one f011dc5 deferred. The scan for the development trust anchor was six inline copies in release.yml, each reading .elf .bin .a .o, while the Collect artifacts step in the same jobs shipped .hex .uf2 and .efi too. Two lists kept by hand beside each other, only one of them pinned by a test. It was more than a list drift. Intel HEX is ASCII, and a UF2 file is 512-byte blocks with headers, so grepping either for the key's raw bytes finds nothing even when the key is in the image. Extending the suffix list without decoding would have made the scan open those files and report them clean, which is worse than not opening them. tools/check_no_dev_anchor.py is the one copy. It decodes Intel HEX (record types 00/01/02/04; anything else is an error, not a skip) and UF2 (magic, payload size, address per block) to the image they encode, merges records and blocks into contiguous runs so a key straddling two of them is one search, and reads the raw formats directly. A file with a scanned suffix that cannot be decoded is reported as a failure, because a file the scanner could not read is a file it did not check. release.yml's six scan steps each call it. is_release_shaped() in the workflow test, which still matched the literal "Release", now mirrors the gate as 302fc5f left it: any optimised type, case-insensitive, or none named at all. tests/unit/test_release_workflow_production_key.py: - every scan step is exactly one call to the tool - the tool names the key core/keystore.c compiles in - the tool's suffix set covers every -name glob in every Collect artifacts step, parsed from the YAML, so the two cannot drift apart - a key placed to straddle a 16-byte HEX record and a 256-byte UF2 block is found in .hex and .uf2 -- and the same test first asserts a raw search of those files returns False, so the decoding is demonstrated to be load-bearing rather than assumed - a clean tree passes; an undecodable .hex fails Negative controls, reverting one piece and keeping the tests: scanner grepping .hex/.uf2 raw instead of decoding -> 2 of 9 fail scanner suffix set missing .hex -> 3 of 9 fail one inline scan restored in release.yml -> 1 of 9 fail Measured at this head: pytest tests/ 126 passed; ctest 32/32; all 17 workflows parse and release.yml's six scan steps and every firmware job's needs: are asserted from the parsed structure.
… about 302fc5f wires tools/check_production_key.py into the configure-time gate through execute_process, and when python3 is not found it emits a WARNING and continues: the key is compiled in unchecked. The review's finding 2 names why that is the wrong branch -- the gate is "the only control for anyone building a device image outside this workflow: a vendor, a downstream fork, a board bring-up that becomes a product" -- and a warning scrolls past. .ai/security.md: a verification step that cannot run must fail, not pass. Now FATAL_ERROR, saying what to install. Development builds pass no EBLDR_PRODUCTION_KEY and never reach this branch, confirmed below. production key, -DCMAKE_DISABLE_FIND_PACKAGE_Python3=TRUE -> rc 1, "python3 was not found", no CMake Warning, nothing generated production key, python3 findable -> rc 0 Debug board build, python3 unfindable -> rc 0 Negative control: with the WARNING branch restored, the new test fails (1 failed). pytest tests/ 126 passed at this head.
…which way a bad key fails The three findings from the review of 8d09efb, none blocking, all taken. 1. (Medium) No pre-merge job compiled the production-key branch on any target. After f011dc5, EBLDR_PRODUCTION_KEY appeared only in release.yml; both jobs that configure a real board pre-merge passed -DEBLDR_ALLOW_DEV_KEY=ON, so build/generated/production_key.c and the #ifdef EBLDR_PRODUCTION_KEY half of core/keystore.c were first cross-compiled when a tag was pushed, inside the workflow that publishes. A link or section-placement fault in the generated TU would have surfaced during a release. build.yml's Cross-compile STM32F4 leg now passes the RFC 8032 TEST 2 public key with -DCMAKE_BUILD_TYPE=Release instead of the opt-out -- a genuine curve point the configure-time check accepts, the same fixture the host tests use, and one whose secret is published, which is fine for a job whose output reaches no device. ci.yml's ARM leg still compiles the dev-key branch, so both halves of keystore.c are cross-compiled on every PR. A new step proves it rather than assuming it: generated/production_key.c exists, its object was built, and the object carries the fixture key and not the dev key. Locally, with the host compiler standing in for arm-none-eabi: configure rc 0, generated TU present, eboot_core builds with 0 errors, production_key.c.o: fixture key present, dev key absent. 2. (Low) tools/check_no_dev_anchor.py indexed rec[0..3] before checking the record had them, so a `:00` line escaped as an IndexError traceback -- still exit 1, so it failed closed, but by accident, and the file-annotated "could not be decoded" line the tool promises was lost. Now a length check before unpacking, and the record's own byte count is checked against its actual length; IndexError and struct.error are caught with the rest. Separately, p.suffix was compared case-sensitively, so a .BIN with the dev key in it was skipped and the tree reported clean. Case-folded. 3. (Low) tools/check_production_key.py said "a point of low order" for every point outside the prime-order subgroup. Two very different things land there. A genuine low-order point (order 2, 4, 8) makes every signature verify -- fails open, an attack vector. A point of order 2L, 4L or 8L is where a mistyped hex digit lands about half the time; the verifier refuses it and every image is rejected -- fails closed, a typo. The message pointed a release engineer at the first when the cause was the second, and no test covered the mixed case, which is why the wording was never challenged. [8]P == identity now tells them apart and each gets its own sentence. Reviewer's 0300...00 and the dev key with its last digit changed both now report "not in the prime-order subgroup -- a mistyped hex digit usually lands here"; the order-2/4/8 vectors still report low order. Negative controls, one piece reverted at a time, tests kept: scanner at 8d09efb -> 2 of 11 fail (truncated record, .BIN) checker at 8d09efb -> 3 of 19 fail (0300..00, mistyped dev key, class-distinction test) At this head: pytest tests/ 131 passed; ctest 32/32; 17 workflows parse; build.yml's configure asserted from non-comment lines of the parsed step (the opt-out flag is named in the comment explaining why it is gone, which is exactly the kind of grep false-match to check for). NOT RUN: the ARM cross-compile itself (no arm-none-eabi-gcc here). The step that proves the branch was compiled is the one CI runs; what was run locally is the same configure and build with the host compiler.
The step added in 4a98951 to prove the production-key branch was cross-compiled failed on its first run: ::error::generated/production_key.c was not compiled The build log two steps above it says otherwise: [ 70%] Building C object CMakeFiles/eboot_core.dir/generated/production_key.c.obj CMake names objects .obj, not .o, when CMAKE_SYSTEM_NAME=Generic, and the step searched for .o. So the finding-1 concern is in fact answered -- the TU is compiled into eboot_core and linked into eboot_firmware.elf on the cross toolchain -- and the step that was meant to show it had a wrong glob. It failed closed on its own bug rather than passing vacuously, which is the right failure, but a red check that is wrong about why it is red helps nobody. Matches both namings now. The find was run against a fixture tree with each extension in turn and returns the object for both. build.yml parses; the workflow test suite (11) passes at this head. Local reproduction of the .obj naming is NOT possible here (the host generator produces .o); the evidence is the CI log line above.
…ot catch The one finding from the review of 4d4e702, plus the docs line agreed on the thread. RAW_SUFFIXES in tools/check_no_dev_anchor.py listed .o and not .obj. 4d4e702 had just fixed the proof step in build.yml for exactly this -- CMake names objects .obj under CMAKE_SYSTEM_NAME=Generic, which is every cross target -- and the scanner's own list did not get the same correction. So on the builds whose objects matter most, the entry that exists to scan object files matched nothing. Reproduced: a .obj holding the dev key under build/ was reported clean. Added .obj, with the reason in the comment above the set, and a test that puts the key in both a .o and a .obj and requires both to be found. docs/key_lifecycle.md gains a provisioning row for the thing the checks cannot do. Of all 960 single-hex-digit typos of a key, 495 go off the curve and 413 land outside the prime-order subgroup -- both refused -- but 52 (about one in twenty) land on a different valid key inside the subgroup, indistinguishable from the real one by construction. A device built on it refuses every image, with a green build. The only control is comparing the secret to the .pub the generator wrote before it enters the secret store, and the mixed-order refusal message now ends with that instruction. Negative control: with the scanner at 4d4e702 and the new test kept, 1 of 12 fails (the .obj case). At this head: pytest tests/ 132 passed; ctest 32/32; line endings unchanged in every edited file.
…ne, not one key's count The provisioning row added in 1cd1265 said "about one in twenty (52 of 960, measured)" for the share of single-hex-digit typos that land on a different valid key. The review of a50df28 is right that this is one key's count stated as the general rate, and that the general rate is worse: a mutated encoding is a curve point with probability about 1/2 and in the prime-order subgroup with probability 1/8, so about 1/16 for any key, by construction. Re-measured at this head with the branch's own checker: 52/960 for the development key, 64/960 for RFC 8032 TEST 2, 75/960 for TEST 3 -- the spread around 60 that 1/16 predicts. The row now gives the derivable rate and cites all three as measurements of specific keys, which is what a number in a security document needs to be: reproducible without the key it was measured on. One sentence in one Markdown file; nothing else changes.
…r nothing includes Finding 2 (High, P1) from the review of embeddedos-org#116 at ef5b70d. Closes embeddedos-org#141. docs/quickstart.md told a developer that `sign_image.py --genkey` writes keys/public_key.h. It did: a header defining ebldr_default_pubkey[32]. That symbol has zero consumers -- core/image_verify.c's own comment records that it "was never defined anywhere, so this did not link", and the verifier reads the anchor from the keystore instead, whose production slot is ebldr_production_key[], generated at configure time from -DEBLDR_PRODUCTION_KEY=<64 hex>. So the documented procedure generated a key, wrote a header, built without error, and shipped firmware whose trust anchor was still the RFC 8032 test key. The review checked that embeddedos-org#122 does not close this, and it does not: after the stack landed the repo would have documented two key paths, one of which did nothing. Both emitters -- --genkey and --extract-pubkey -- now write the one artefact the build consumes: public_key.hex, 64 lowercase hex characters, the exact value EBLDR_PRODUCTION_KEY takes, and print the cmake invocation that uses it with a pointer to docs/key_lifecycle.md and the instruction to compare the value against the .pub before storing it. The header is not written under any name: a second mechanism for the same thing is how this drifted in the first place. docs/quickstart.md's signing section now shows the configure step with -DEBLDR_PRODUCTION_KEY=$(cat keys/public_key.hex), says what happens without it, and names the lifecycle document. Tests, tests/unit/test_sign_image.py: - --genkey writes no public_key.h; writes public_key.hex; 64 hex chars; equal to the raw key in public.pem; stdout names the flag - the written value passes tools/check_production_key.py, the same check the configure-time gate runs, so the documented path cannot produce a key the build then refuses - --extract-pubkey writes the same hex, and no dead symbol - quickstart names public_key.hex and the flag, and not the header Negative controls, one file reverted at a time, tests kept: tools/sign_image.py at 18fab20 -> 3 of 20 fail docs/quickstart.md at 18fab20 -> 1 of 1 fails End to end, the documented procedure at this head: --genkey, then cmake -DEBLDR_BOARD=stm32f4 -DCMAKE_BUILD_TYPE=Release -DEBLDR_PRODUCTION_KEY=$(cat keys/public_key.hex): configure rc 0, and build/generated/production_key.c carries exactly the generated key. At this head, on embeddedos-org#122 at 18fab20: pytest tests/ 138 passed.
This was referenced Sep 15, 2026
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Kartikey1306
added a commit
to Kartikey1306/eBoot
that referenced
this pull request
Sep 15, 2026
…er had
Same class as the quickstart defect this branch fixes, in the second
document. docs/key_lifecycle.md section 2.2 "Alternative -- using
eBootloader tooling" showed:
python3 tools/sign_image.py --genkey \
--key-out keys/production_key.pem \
--pub-header include/eos_signing_key.h
sign_image.py has never defined --key-out or --pub-header; its options
are --genkey, --extract-pubkey and --output. A reader following the
lifecycle document -- the one written for production provisioning --
got an argparse error, or, reading the intent, went looking for a
header to embed, which is the dead path 0c7c8f5 removes. Replaced with
the real invocation and the configure flag that consumes its output.
The doc test now covers both documents, and a second test reads every
sign_image.py invocation shown in quickstart.md, key_lifecycle.md and
README.md and requires each --flag to be one the tool defines, so a
documented flag that does not exist fails the suite the same way a
documented header nothing reads does.
Negative control: key_lifecycle.md at 0c7c8f5 with the tests kept ->
2 of 24 fail (the phantom flags, and the header-free assertion).
pytest tests/ 140 passed at this head.
Found by a sibling session working the same review finding in
parallel; its patch was not applied, this is the one piece of it embeddedos-org#142
lacked.
…er had
Same class as the quickstart defect this branch fixes, in the second
document. docs/key_lifecycle.md section 2.2 "Alternative -- using
eBootloader tooling" showed:
python3 tools/sign_image.py --genkey \
--key-out keys/production_key.pem \
--pub-header include/eos_signing_key.h
sign_image.py has never defined --key-out or --pub-header; its options
are --genkey, --extract-pubkey and --output. A reader following the
lifecycle document -- the one written for production provisioning --
got an argparse error, or, reading the intent, went looking for a
header to embed, which is the dead path 0c7c8f5 removes. Replaced with
the real invocation and the configure flag that consumes its output.
The doc test now covers both documents, and a second test reads every
sign_image.py invocation shown in quickstart.md, key_lifecycle.md and
README.md -- continuation lines included -- and requires each --flag to
be one the tool defines, so a documented flag that does not exist
fails the suite the same way a documented header nothing reads does.
The first version of that guard could not fail: its pattern's [^\n]*
swallowed the line-ending backslash, so it never read a continuation
line and saw only --genkey. Caught because the negative control
reported 1 failure where 2 were claimed. Fixed to consume
backslash-newline pairs explicitly; the same control now reports 2.
Negative control: key_lifecycle.md at 0c7c8f5 with the tests kept ->
2 of 24 fail (the phantom flags, and the header-free assertion).
pytest tests/ 142 passed at this head.
Found by a sibling session working the same review finding in
parallel; its patch was not applied, this is the one piece of it embeddedos-org#142
lacked.
Kartikey1306
force-pushed
the
fix/genkey-emits-the-key-the-build-reads
branch
from
September 15, 2026 07:01
277e019 to
180a74f
Compare
Contributor
Author
|
Ready for review — this PR has never had one, and it is the tip of the eBoot stack. Head
Verified at
Not run here: the ARM and release legs, and the production-key gate's Merge order: #115 → #116 → #122 → this. The quickstart text this rewrites references what #122 creates, which is why the earlier review asked for it to sit on top of #122 rather than on master. Closes #141. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #122 (→ #116 → #115); review
18fab20..180a74ffor this change alone — two commits, four files. (Corrected from18fab20..0c7c8f5: the second commit fixes the same class indocs/key_lifecycle.md, which showed--key-outand--pub-header, two flags the tool has never had, and adds a guard that reads every documentedsign_image.pyinvocation and requires each flag to exist.) Merge order #115 → #116 → #122 → this; the quickstart text this rewrites references what #122 creates, which is why the review asked for it on top of #122.Closes #141.
The defect
docs/quickstart.mdtells a developer thatsign_image.py --genkeywriteskeys/public_key.h. It did — a header definingstatic const uint8_t ebldr_default_pubkey[32]. That symbol has zero consumers:core/image_verify.c's own comment records it "was never defined anywhere, so this did not link"; the verifier reads the anchor from the keystore, whose production slot isebldr_production_key[], generated at configure time from-DEBLDR_PRODUCTION_KEY=<64 hex>. So the documented procedure generated a key, wrote a header, built without error, and shipped firmware whose trust anchor was still the RFC 8032 test key — whose private half is printed in an IETF document. The review of #116 (finding 2, High) found it and checked that #122 does not close it: after the stack landed the repo would document two key paths, one of which does nothing.The fix
Both emitters —
--genkeyand--extract-pubkey— now write the one artefact the build consumes:public_key.hex, 64 lowercase hex characters, the exact valueEBLDR_PRODUCTION_KEYtakes. The tool prints thecmakeinvocation that uses it, points atdocs/key_lifecycle.md, and says to compare the value against the.pubbefore storing it (the one control for the ~1/16 typo case #122 documents). The header is not written under any name — a second mechanism for the same thing is how this drifted in the first place.docs/quickstart.md's signing section now shows the configure step with-DEBLDR_PRODUCTION_KEY=$(cat keys/public_key.hex), says what happens without it, and names the lifecycle document.Tests
test_genkey_writes_the_hex_the_build_consumes_and_no_dead_headerpublic_key.h;public_key.hexis 64 hex chars equal to the raw key inpublic.pem; stdout names the flagtest_genkey_output_passes_the_configure_time_key_checktools/check_production_key.py— the same check the gate runs — so the documented path cannot produce a key the build then refusestest_extract_pubkey_writes_hex_tootest_quickstart_documents_the_key_path_the_build_readspublic_key.hexand the flag; not the headerNegative controls, one file reverted at a time, tests kept
End to end, the documented procedure at
0c7c8f5tools/sign_image.pyis CRLF and stays CRLF (445 → 427 lines). Two corrections visible in the branch history: the phantom-flag guard's first version could not fail (its pattern never read a continuation line — caught because the negative control reported 1 failure where 2 were claimed; fixed in the same commit before this body was updated), and the first push of this commit had flattened it to LF — a 445-line diff for a 68-line change — caught on the line-ending check and amended before this PR was opened.