From f704d87fd445e4d61db00bc302e78660fc99f02f Mon Sep 17 00:00:00 2001 From: kartikey1306 Date: Thu, 3 Sep 2026 15:06:59 +0530 Subject: [PATCH 1/3] =?UTF-8?q?fix:=20repair=20master=20=E2=80=94=20the=20?= =?UTF-8?q?ABI=20asserts=20and=20the=20Ed25519=20verifier=20both=20merged?= =?UTF-8?q?=20broken?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit master (22d8f8b) does not compile. Two independent double-merges, both the same shape: two PRs fixing adjacent things landed on stale bases, each was green on its own branch, and the result was never rebuilt. 1. include/eos_image.h — #93 replaced reserved[30] with tlv_len (2) + tlv_hash[28], preserving every offset. #87 merged afterwards carrying asserts written against the older struct: error: no member named 'reserved' in 'eos_image_header_t' (x2) #93 already asserts tlv_len at 62 and tlv_hash at 64, so the offset assert was a duplicate; the width assert had no replacement and is restored as two asserts covering both halves of the same 30-byte span. No offset moves and the wire format is unchanged. 2. core/ed25519_verify.c — #86 and #57 both landed a subgroup guard, so the file carried two byte-identical point_is_identity() definitions: error: redefinition of 'point_is_identity' Only #57's public_key_is_valid_subgroup() is wired to the call site, so #86's key_has_prime_order() was dead. Kept the live function, folded #86's fuller rationale onto it, deleted the duplicate. 3. tests/unit/test_ed25519.c — collateral from the same merge. Two copies of test_ed25519_identity_key_forgery_rejected, main() calling it twice and two tests not at all, and test_ed25519_low_order_R_with_a_valid_key_is_not_a_forgery referencing k_low_order[] and messages[] that the merge had dropped. While restoring the corpus, corrected it (review finding on #86): the array claimed to hold "the eight low-order point encodings" and held five. Every order here was computed rather than copied — decode y, recover x, add the point to itself until it reaches the identity — giving 1, 2, 4, 4, 8, 8, 8, 8. Missing before: y=0 with the sign bit set, and both sign-flipped order-8 encodings. D9FF..FF was in the array and is not a low-order point at all — no x satisfies the curve equation for that y — so it moves to a separate k_non_canonical[], with EDFF..FF7F (y=p) and EEFF..FF7F (y=p+1). tests_run was assigned a literal (11) in main() and never incremented, which is how the duplicate call and the two unregistered tests went unnoticed. The TEST macro now increments it, so the total cannot drift. Verified: cmake -DEBLDR_BUILD_TESTS=ON on master FAILS to build, 3 errors same with this commit builds clean ctest 21/21 PASS ctest -DEBLDR_SANITIZE=ON (ASan+UBSan) 21/21 PASS pytest tests/ 24 passed, 1 skipped test_ed25519 14/14 PASS (was 11 claimed, 12 run) discrimination, with `public_key_is_valid_subgroup` disabled: test_ed25519_low_order_keys_rejected FAILS, as it must test_ed25519_non_canonical_... still PASSES — those are refused by unpackneg() on canonicality, a different mechanism, which is the reason they are held in a separate array rather than counted among the eight. --- core/ed25519_verify.c | 56 ++++------------- include/eos_image.h | 16 +++-- tests/unit/test_ed25519.c | 126 ++++++++++++++++++++++++++++---------- 3 files changed, 116 insertions(+), 82 deletions(-) diff --git a/core/ed25519_verify.c b/core/ed25519_verify.c index d36cc7c..34aca77 100644 --- a/core/ed25519_verify.c +++ b/core/ed25519_verify.c @@ -289,6 +289,16 @@ static int point_is_identity(gf p[4]) return diff == 0; } +static void scalarbase(gf r[4], const uint8_t *s) +{ + gf q[4]; + fe_copy16(q[0], BX); + fe_copy16(q[1], BY); + fe_copy16(q[2], gf1); + fe_mul(q[3], BX, BY); + scalarmult(r, q, s); +} + /* Reject a public key outside the prime-order subgroup. * * Decoding a point is not enough. Ed25519 has eight points of low order, and @@ -304,51 +314,9 @@ static int point_is_identity(gf p[4]) * so there is no separate constant to transcribe wrongly: a mistyped L would * reject valid keys, and only in the field. * - * A arrives negated from unpackneg(). [L](-A) = -[L]A and the identity is its - * own negation, so neither condition is affected by the sign. - * - * Formulation taken from eBoot#57 by @muhammadburhandevv-hub, which reached - * this before I did and states both conditions in one expression. + * The key arrives negated from unpackneg(). [L](-A) = -[L]A and the identity + * is its own negation, so neither condition is affected by the sign. */ -static int key_has_prime_order(gf A[4]) -{ - uint8_t order_l[32]; - gf q[4], multiple[4]; - int i; - - for (i = 0; i < 32; i++) - order_l[i] = (uint8_t)ORDER_L[i]; - for (i = 0; i < 4; i++) - fe_copy16(q[i], A[i]); - - scalarmult(multiple, q, order_l); - return point_is_identity(multiple) && !point_is_identity(A); -} - -static void scalarbase(gf r[4], const uint8_t *s) -{ - gf q[4]; - fe_copy16(q[0], BX); - fe_copy16(q[1], BY); - fe_copy16(q[2], gf1); - fe_mul(q[3], BX, BY); - scalarmult(r, q, s); -} - -static int point_is_identity(gf p[4]) -{ - uint8_t encoded[32]; - point_pack(encoded, p); - - uint8_t diff = (uint8_t)(encoded[0] ^ 1U); - for (int i = 1; i < 32; i++) - diff |= encoded[i]; - return diff == 0; -} - -/* Public keys must be non-identity points in Ed25519's prime-order subgroup. - * Merely decoding a point is insufficient: an identity or torsion key can - * make the verification equation true without knowledge of a private key. */ static int public_key_is_valid_subgroup(gf public_key[4]) { uint8_t order_l[32]; diff --git a/include/eos_image.h b/include/eos_image.h index 62744d0..24cbb16 100644 --- a/include/eos_image.h +++ b/include/eos_image.h @@ -108,7 +108,8 @@ EOS_IMG_STATIC_ASSERT(offsetof(eos_image_header_t, tlv_hash) + /* Every remaining field, pinned. * - * Four of the fourteen fields were asserted. Transposing two adjacent + * Three of the thirteen field offsets were asserted (the fourth pre-existing + * assert is sizeof, which is not a field). Transposing two adjacent * same-width fields moves neither sizeof nor any of those four offsets, so it * compiled clean: with load_addr and entry_addr swapped, all four existing * asserts still passed and the bootloader would load an image at its entry @@ -132,15 +133,18 @@ EOS_IMG_STATIC_ASSERT(offsetof(eos_image_header_t, flags) == 24, "flags must stay at offset 24"); EOS_IMG_STATIC_ASSERT(offsetof(eos_image_header_t, sig_len) == 61, "sig_len must stay at offset 61"); -EOS_IMG_STATIC_ASSERT(offsetof(eos_image_header_t, reserved) == 62, - "reserved[] must stay at offset 62"); +/* tlv_len and tlv_hash are asserted above, where #93 introduced them; the + * 30 bytes they occupy are the ones this block used to pin as reserved[]. */ /* Field widths. An offset assert cannot see a field growing into padding that - * happens to keep every later offset -- reserved[] absorbs exactly that. */ + * happens to keep every later offset -- the 30 bytes at 62 absorb exactly + * that, which is why both halves of that span carry a width assert. */ EOS_IMG_STATIC_ASSERT(sizeof(((eos_image_header_t *)0)->hash) == 32, "hash[] is 32 bytes on the wire"); -EOS_IMG_STATIC_ASSERT(sizeof(((eos_image_header_t *)0)->reserved) == 30, - "reserved[] is 30 bytes on the wire"); +EOS_IMG_STATIC_ASSERT(sizeof(((eos_image_header_t *)0)->tlv_len) == 2, + "tlv_len is 2 bytes on the wire"); +EOS_IMG_STATIC_ASSERT(sizeof(((eos_image_header_t *)0)->tlv_hash) == 28, + "tlv_hash is 28 bytes on the wire"); EOS_IMG_STATIC_ASSERT(sizeof(((eos_image_header_t *)0)->signature) == 64, "signature[] is 64 bytes on the wire"); diff --git a/tests/unit/test_ed25519.c b/tests/unit/test_ed25519.c index f78012d..9ea837c 100644 --- a/tests/unit/test_ed25519.c +++ b/tests/unit/test_ed25519.c @@ -30,6 +30,7 @@ static int tests_passed = 0; static void name(void); \ static void run_##name(void) { \ printf(" %-50s ", #name); \ + tests_run++; \ name(); \ tests_passed++; \ printf("[PASS]\n"); \ @@ -211,29 +212,105 @@ TEST(test_ed25519_identity_key_forgery_rejected) msg, sizeof(msg) - 1) != EOS_OK); } +/* The eight canonical low-order point encodings. + * + * Every order was computed rather than copied: decoding each y, recovering x, + * and repeatedly adding the point until it reached the identity gives + * 1, 2, 4, 4, 8, 8, 8, 8 for the entries below in order. An earlier revision + * of this array held only five of them -- it omitted y=0 with the sign bit + * set and both sign-flipped order-8 encodings -- while its comment claimed to + * hold "the eight". [L](-A) = -[L]A, so the guard rejects a sign variant + * whether or not it is listed; the reason to list them is that this is the + * regression record for a secure-boot bypass, and a claimed class has to be + * the class it claims. */ +static const uint8_t k_low_order[8][32] = { + /* order 1: the identity, y = 1 */ + {0x01}, + /* order 2: y = -1 */ + {0xEC,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF, + 0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0x7F}, + /* order 4: y = 0, sign bit clear */ + {0x00}, + /* order 4: y = 0, sign bit set -- the encoding the earlier array missed */ + {0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00, + 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x80}, + /* order 8 */ + {0x26,0xE8,0x95,0x8F,0xC2,0xB2,0x27,0xB0,0x45,0xC3,0xF4,0x89,0xF2,0xEF,0x98,0xF0, + 0xD5,0xDF,0xAC,0x05,0xD3,0xC6,0x33,0x39,0xB1,0x38,0x02,0x88,0x6D,0x53,0xFC,0x05}, + /* order 8 */ + {0xC7,0x17,0x6A,0x70,0x3D,0x4D,0xD8,0x4F,0xBA,0x3C,0x0B,0x76,0x0D,0x10,0x67,0x0F, + 0x2A,0x20,0x53,0xFA,0x2C,0x39,0xCC,0xC6,0x4E,0xC7,0xFD,0x77,0x92,0xAC,0x03,0x7A}, + /* order 8: sign flip of the first order-8 entry -- also missing before */ + {0x26,0xE8,0x95,0x8F,0xC2,0xB2,0x27,0xB0,0x45,0xC3,0xF4,0x89,0xF2,0xEF,0x98,0xF0, + 0xD5,0xDF,0xAC,0x05,0xD3,0xC6,0x33,0x39,0xB1,0x38,0x02,0x88,0x6D,0x53,0xFC,0x85}, + /* order 8: sign flip of the second -- also missing before */ + {0xC7,0x17,0x6A,0x70,0x3D,0x4D,0xD8,0x4F,0xBA,0x3C,0x0B,0x76,0x0D,0x10,0x67,0x0F, + 0x2A,0x20,0x53,0xFA,0x2C,0x39,0xCC,0xC6,0x4E,0xC7,0xFD,0x77,0x92,0xAC,0x03,0xFA}, +}; + +/* Not low-order points, and refused earlier and by a different mechanism: + * unpackneg() rejects them on canonicality or because no x exists. Kept + * separate so the array above means what its name says -- an earlier revision + * spent one of its eight slots on D9FF..FF, which does not decode at all. */ +static const uint8_t k_non_canonical[3][32] = { + /* y = p: reduces to 0, decodes as an order-4 point but is not canonical */ + {0xED,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF, + 0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0x7F}, + /* y = p + 1: reduces to the identity, likewise not canonical */ + {0xEE,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF, + 0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0x7F}, + /* no x satisfies the curve equation for this y: unpackneg() fails */ + {0xD9,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF, + 0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF}, +}; + +/* A low-order key forges for roughly one message in n, where n is its order, + * so a single fixed message would let a real bypass pass this suite. */ +static const char *const messages[] = { + "untrusted firmware", "v1.0.0", "", "a", "boot", "eos", "1234", "payload", +}; + TEST(test_ed25519_low_order_keys_rejected) { /* zero_pubkey covers one encoding; Ed25519 has eight low-order points and * the family is what matters. A subgroup test alone is not enough either: * the identity has order 1, which divides L, so [L]identity = identity and - * it passes. Both checks are required. */ - static const uint8_t low_order[4][32] = { - {0}, - {1}, - {0x26,0xe8,0x95,0x8f,0xc2,0xb2,0x27,0xb0,0x45,0xc3,0xf4,0x89,0xf2,0xef,0x98,0xf0, - 0xd5,0xdf,0xac,0x05,0xd3,0xc6,0x33,0x39,0xb1,0x38,0x02,0x88,0x6d,0x53,0xfc,0x05}, - {0xc7,0x17,0x6a,0x70,0x3d,0x4d,0xd8,0x4f,0xba,0x3c,0x0b,0x76,0x0d,0x10,0x67,0x0f, - 0x2a,0x20,0x53,0xfa,0x2c,0x39,0xcc,0xc6,0x4e,0xc7,0xfd,0x77,0x92,0xac,0x03,0x7a}, - }; - const uint8_t msg[] = "untrusted firmware"; + * it passes. Both checks are required. + * + * The sweep is every low-order encoding as the key against every one as R, + * over eight messages, because a low-order key of order n forges for + * roughly one message in n -- a single fixed message would let a genuine + * bypass through this test. Measured against 13a7a02, the last commit + * before the subgroup check: 16 of the 64 (key, R) pairs were accepted by + * at least one message. Here: none. */ + for (size_t k = 0; k < sizeof(k_low_order) / sizeof(k_low_order[0]); k++) { + for (size_t r = 0; r < sizeof(k_low_order) / sizeof(k_low_order[0]); r++) { + for (size_t m = 0; m < sizeof(messages) / sizeof(messages[0]); m++) { + uint8_t sig[64]; + memset(sig, 0, sizeof(sig)); + memcpy(sig, k_low_order[r], 32); + ASSERT(eos_ed25519_verify(sig, k_low_order[k], + (const uint8_t *)messages[m], + strlen(messages[m])) != EOS_OK); + } + } + } +} - for (int k = 0; k < 4; k++) { - for (int r = 0; r < 4; r++) { +TEST(test_ed25519_non_canonical_encodings_rejected) +{ + /* These are refused before the subgroup check ever runs -- unpackneg() + * rejects them on canonicality, or because no x satisfies the curve + * equation. Pinned separately so that nobody deletes that path on the + * grounds that the subgroup test now covers it. It does not. */ + for (size_t k = 0; k < sizeof(k_non_canonical) / sizeof(k_non_canonical[0]); k++) { + for (size_t m = 0; m < sizeof(messages) / sizeof(messages[0]); m++) { uint8_t sig[64]; memset(sig, 0, sizeof(sig)); - memcpy(sig, low_order[r], 32); - ASSERT(eos_ed25519_verify(sig, low_order[k], - msg, sizeof(msg) - 1) != EOS_OK); + memcpy(sig, k_non_canonical[k], 32); + ASSERT(eos_ed25519_verify(sig, k_non_canonical[k], + (const uint8_t *)messages[m], + strlen(messages[m])) != EOS_OK); } } } @@ -260,21 +337,6 @@ TEST(test_ed25519_zero_signature_rejected) ASSERT(eos_ed25519_verify(sig, pk, msg, 1) != EOS_OK); } -TEST(test_ed25519_identity_key_forgery_rejected) -{ - /* The identity point has compressed encoding 01 00...00. With both the - * public key and R set to the identity and S set to zero, the verification - * equation is true for every message unless low-order keys are rejected. */ - uint8_t identity_pub[32] = {1}; - uint8_t identity_sig[64] = {1}; - const uint8_t msg[] = "untrusted firmware"; - - ASSERT(eos_ed25519_verify(identity_sig, identity_pub, - msg, sizeof(msg) - 1) != EOS_OK); -} - -/* ---- SHA-512, the hash Ed25519 is defined over (FIPS 180-4) ---- */ - TEST(test_ed25519_low_order_R_with_a_valid_key_is_not_a_forgery) { /* The subgroup check guards the public key, not R, and that is @@ -367,13 +429,13 @@ int main(void) run_test_ed25519_null_args(); run_test_ed25519_identity_key_forgery_rejected(); run_test_ed25519_low_order_keys_rejected(); + run_test_ed25519_non_canonical_encodings_rejected(); + run_test_ed25519_low_order_R_with_a_valid_key_is_not_a_forgery(); run_test_ed25519_zero_pubkey_rejected(); run_test_ed25519_zero_signature_rejected(); - run_test_ed25519_identity_key_forgery_rejected(); run_test_sha512_known_answers(); run_test_sha512_streaming_matches_one_shot(); - tests_run = 11; printf("\n%d/%d tests passed\n", tests_passed, tests_run); return (tests_passed == tests_run) ? 0 : 1; } From 1bd6a220e34a98c666e09fff5f5cbc880c35142d Mon Sep 17 00:00:00 2001 From: kartikey1306 Date: Thu, 3 Sep 2026 16:13:34 +0530 Subject: [PATCH 2/3] ci: gate what the repository actually reports, and close the sibling gate that failed open Answers the review on #90. Finding 1 (Medium) -- `CI Gate` gates three jobs in ci.yml. This repository has 16 workflow files and a PR head reports 26 checks across five runs, so the maintainer action in the body -- require one name -- would have left CodeQL, every EoSim platform leg, all three Cross-Platform legs and a second host build unrequired, and `master` could still go red from any of them. The rot-guard had the same boundary: WORKFLOW was hardcoded to ci.yml, so a job added to build.yml or codeql.yml was neither covered nor noticed. Adds two tests over every workflow with a `pull_request` trigger: REQUIRED_CHECKS names what a maintainer must actually require, NO_GATE excuses the rest with a reason about the workflow itself, and a third test asserts each gated workflow really has a job displaying under the name given. Finding 2 (Medium) -- `Simulation Gate` had the exact fail-open shape this PR removes from ci.yml: `needs: [simulate, cross-platform]`, then it printed both results and branched on `simulate` alone before printing "All simulation checks passed". A red or skipped `cross-platform` -- three OS legs -- passed it. Ported the `toJSON(needs)` + `jq` body, which is dependency-list-agnostic and cannot fall out of step with `needs:`. And made it a rule rather than a one-off: test_no_aggregating_gate_ignores_part_of_its_needs walks every gate in every pull-request workflow and fails if a declared dependency is never compared. Two refinements were needed to make it mean something: - printing a result is not testing it. The first version grepped for `needs.X.result` anywhere in the script, which Simulation Gate satisfied with its echo line. Only a line that compares counts. - book-build.yml's `summary` writes a step summary and claims no verdict. It is reporting, not gating, so the check applies only to jobs that either `exit 1` or assert that everything passed. Finding 3 (Low) -- this branch had replaced master's `pip3 install -r requirements.txt pytest-cov` with a hand-maintained list to add pyyaml, which regressed the guard in tests/unit/test_requirements.py: these jobs run pytest over tests/ but never install from requirements.txt ... ['ci.yml:test'] Restored the requirements.txt install; pyyaml was already declared there, so the hand list was not needed at all. That also removes the collision with #88, which edits the same line. Verified: pytest tests/ 47 passed ctest 21/21 PASS yaml.safe_load of ci.yml, simulation-test.yml, book-build.yml all parse discrimination, both ways: - reverting Simulation Gate to its fail-open body gives simulation-test.yml:sanity-gate declares needs ['simulate', 'cross-platform'] but never tests ['cross-platform'] - with the toJSON(needs) body in place, 9 passed Refs #90 --- .github/workflows/ci.yml | 47 ++++- .github/workflows/simulation-test.yml | 30 ++- tests/unit/test_ci_gate.py | 256 ++++++++++++++++++++++++++ 3 files changed, 316 insertions(+), 17 deletions(-) create mode 100644 tests/unit/test_ci_gate.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1cfbad9..14ceb27 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -36,13 +36,7 @@ jobs: cmake ninja-build gcc-12 g++-12 \ gcc-arm-none-eabi binutils-arm-none-eabi \ lcov gcovr python3-pip - # Install from requirements.txt rather than a hand-maintained list. - # tests/unit/test_sign_image.py guards itself with - # pytest.importorskip("cryptography"), so a dependency that is - # declared nowhere does not fail the job -- it skips the 14 cases - # that pin the signed header format and the run still goes green. - # pytest-cov stays separate: it is a CI-only coverage plugin, not a - # dependency of anything in the repository. + # pyyaml: tests/unit/test_ci_gate.py parses .github/workflows/ci.yml pip3 install -r requirements.txt pytest-cov - name: Configure (host) @@ -182,3 +176,42 @@ jobs: generate_release_notes: true draft: false prerelease: false + + # ── Required check ──────────────────────────────────────────────────────── + # One job that succeeds only if every other job in this workflow did, so + # branch protection has a single stable name to require. `required_status_ + # checks` is null on master here, the same gap eos#92 and ebuild#87 track: + # nothing builds the merge result before it becomes master. + # + # `release` is deliberately outside the gate: it only runs on tags, so on a + # pull request it is skipped, and a required check that is skipped never + # reports -- the pull request would wait for a status that never arrives. + # + # `if: always()` for the same reason in reverse: without it the gate is + # skipped whenever an earlier job fails, so a real failure would present as a + # pull request that hangs rather than one that goes red. + # + # A non-success result of any kind fails the gate, `skipped` included. A job + # that did not run did not verify anything, and treating that as a pass is + # the fail-open shape #38, #59 and #82 removed from the boot path. + ci-gate: + name: CI Gate + runs-on: ubuntu-22.04 + needs: [test, build-arm, static-analysis] + if: always() + steps: + - name: Every job in this workflow must have succeeded + env: + RESULTS: ${{ toJSON(needs) }} + run: | + printf '%s\n' "$RESULTS" + bad=$(printf '%s' "$RESULTS" | jq -r ' + to_entries[] + | select(.value.result != "success") + | " \(.key): \(.value.result)"') + if [ -n "$bad" ]; then + echo "::error::CI Gate failed. These jobs did not succeed:" + printf '%s\n' "$bad" + exit 1 + fi + echo "All jobs succeeded." diff --git a/.github/workflows/simulation-test.yml b/.github/workflows/simulation-test.yml index 0706e8b..f3c1be0 100644 --- a/.github/workflows/simulation-test.yml +++ b/.github/workflows/simulation-test.yml @@ -95,15 +95,25 @@ jobs: needs: [simulate, cross-platform] runs-on: ubuntu-latest steps: - - name: Results + # Iterates toJSON(needs) rather than naming each dependency. The + # previous body printed both results and branched on `simulate` alone, + # so a red or skipped `cross-platform` -- three OS legs -- passed the + # gate and it still printed "All simulation checks passed". That is the + # fail-open shape ci.yml's gate was written to remove, in a job whose + # name a maintainer would plausibly require. This form cannot fall out + # of step with `needs:`. + - name: Every job in this workflow must have succeeded + env: + RESULTS: ${{ toJSON(needs) }} run: | - echo "════════════════════════════════════════════" - echo " EoSim Simulation Sanity Results" - echo "════════════════════════════════════════════" - echo "Simulation (11 platforms): ${{ needs.simulate.result }}" - echo "Cross-Platform (Win/Lin/Mac): ${{ needs.cross-platform.result }}" - echo "════════════════════════════════════════════" - if [ "${{ needs.simulate.result }}" != "success" ]; then - echo "❌ Simulation failed"; exit 1 + printf '%s\n' "$RESULTS" + bad=$(printf '%s' "$RESULTS" | jq -r ' + to_entries[] + | select(.value.result != "success") + | " \(.key): \(.value.result)"') + if [ -n "$bad" ]; then + echo "::error::Gate failed. These jobs did not succeed:" + printf '%s\n' "$bad" + exit 1 fi - echo "✅ All simulation checks passed (EoSim steps skipped — no published release)" + echo "All jobs succeeded." diff --git a/tests/unit/test_ci_gate.py b/tests/unit/test_ci_gate.py new file mode 100644 index 0000000..037d160 --- /dev/null +++ b/tests/unit/test_ci_gate.py @@ -0,0 +1,256 @@ +"""The CI workflow must expose one job that summarises all the others. + +`required_status_checks` is null on this repository's `master`. Branch protection can only +require a check by name, and the names this workflow produces are not usable +for that directly: `release` is skipped on every pull request, so requiring it would leave +every pull request waiting for a status that never arrives. + +So `ci-gate` exists to be the one name to require. These tests keep it honest +-- specifically, they fail if someone adds a job to the workflow and does not +wire it into the gate, which would otherwise silently create a job that the +required check does not cover. +""" + +import re + +import yaml +import pytest +from pathlib import Path + + +WORKFLOWS_DIR = Path(__file__).resolve().parents[2] / ".github" / "workflows" +WORKFLOW = WORKFLOWS_DIR / "ci.yml" + +#: Every workflow that produces a pull-request status, and the display name a +#: maintainer must require for it. `CI Gate` covers ci.yml and nothing else -- +#: cross-workflow `needs` is not something GitHub offers -- so the required +#: set is this mapping, not one name. checks.txt on a PR head here lists 26 +#: checks across five runs; ci.yml produces five of them. +#: +#: A workflow may sit in NO_GATE only with a reason about the workflow itself. +REQUIRED_CHECKS = { + "ci.yml": "CI Gate", +} + +NO_GATE = { + "auto-assign.yml": + "assigns a reviewer; it verifies nothing, so requiring it would block " + "merges on a housekeeping step", + "claude-code-review.yml": + "posts advisory review comments and never fails on content", + "codeql.yml": + "already reports a single stable name, `CodeQL`, which should be " + "required directly rather than wrapped in a gate", + "book-build.yml": + "builds documentation; a docs failure should not block a code merge", + "build.yml": + "produces `Host Build & Tests` and the cross-compile legs under " + "stable names that can be required directly; wrapping them adds a " + "layer without adding coverage", + "simulation-test.yml": + "its `Simulation Gate` job tests only needs.simulate.result and then " + "prints 'All simulation checks passed', so a red or skipped " + "cross-platform leg passes it. Requiring it today would assert more " + "than it checks -- fixed below rather than excused", +} + + +def _load(path): + return yaml.safe_load(path.read_text(encoding="utf-8")) + + +def _runs_on_pull_request(doc): + # PyYAML parses a bare `on:` key as the boolean True. + triggers = doc.get("on", doc.get(True, {})) + if isinstance(triggers, (dict, list)): + return "pull_request" in triggers + return triggers == "pull_request" + + +def _pr_workflows(): + found = {} + for path in sorted(WORKFLOWS_DIR.glob("*.yml")): + doc = _load(path) + if isinstance(doc, dict) and _runs_on_pull_request(doc): + found[path.name] = doc + assert found, f"no pull-request workflows found under {WORKFLOWS_DIR}" + return found + +# The name branch protection is pointed at. Changing it silently un-requires +# the check, so it is pinned here rather than merely read. +GATE_ID = "ci-gate" +GATE_NAME = "CI Gate" + + +@pytest.fixture(scope="module") +def workflow(): + assert WORKFLOW.is_file(), f"{WORKFLOW} does not exist" + return yaml.safe_load(WORKFLOW.read_text(encoding="utf-8")) + + +@pytest.fixture(scope="module") +def jobs(workflow): + return workflow["jobs"] + + +def _only_runs_on_tags(job): + """Is this job gated to tag builds, and therefore skipped on every PR?""" + condition = str(job.get("if", "")) + return "refs/tags" in condition + + +def test_gate_job_exists(jobs): + assert GATE_ID in jobs, ( + f"no {GATE_ID!r} job; branch protection has no single name to require" + ) + + +def test_gate_display_name_is_pinned(jobs): + assert jobs[GATE_ID]["name"] == GATE_NAME, ( + "the gate's display name is what branch protection matches on; " + "renaming it un-requires the check without failing anything" + ) + + +def test_gate_runs_even_when_an_earlier_job_fails(jobs): + condition = str(jobs[GATE_ID].get("if", "")).strip() + assert condition == "always()", ( + "the gate needs `if: always()`. Without it the gate is skipped when an " + "earlier job fails, and a skipped required check never reports -- the " + "pull request waits for a status that never arrives instead of showing " + "a failure" + ) + + +def test_gate_covers_every_job_that_runs_on_a_pull_request(jobs): + expected = { + name for name, job in jobs.items() + if name != GATE_ID and not _only_runs_on_tags(job) + } + declared = set(jobs[GATE_ID].get("needs", [])) + + missing = expected - declared + assert not missing, ( + f"these jobs run on pull requests but the gate does not wait for them: " + f"{sorted(missing)}. A job outside the gate is a job the required " + f"check does not cover." + ) + + unknown = declared - set(jobs) + assert not unknown, f"the gate needs jobs that do not exist: {sorted(unknown)}" + + +def test_jobs_left_out_of_the_gate_are_genuinely_tag_only(jobs): + """Excluding a job from the gate must be justified, not just convenient.""" + declared = set(jobs[GATE_ID].get("needs", [])) + for name, job in jobs.items(): + if name == GATE_ID or name in declared: + continue + assert _only_runs_on_tags(job), ( + f"job {name!r} is not in the gate and is not tag-only; either add " + f"it to `needs` or give it an `if:` that explains why it cannot run " + f"on a pull request" + ) + + +def test_every_pull_request_workflow_is_accounted_for(): + """A new workflow must be gated or explicitly excused, not silently added. + + `CI Gate` summarises ci.yml only. Requiring that one name -- which is what + this PR's description asked a maintainer to do -- leaves every other + workflow's checks unrequired, and the rot-guard above cannot see them + either. This is the test that notices. + """ + unaccounted = [ + name for name in _pr_workflows() + if name not in REQUIRED_CHECKS and name not in NO_GATE + ] + assert not unaccounted, ( + f"these workflows produce pull-request checks but are neither gated " + f"nor excused: {sorted(unaccounted)}. Add a gate job and list it in " + f"REQUIRED_CHECKS, or add it to NO_GATE with the reason." + ) + + +def test_gated_workflows_really_have_their_gate(): + workflows = _pr_workflows() + for filename, display in REQUIRED_CHECKS.items(): + assert filename in workflows, ( + f"{filename} is in REQUIRED_CHECKS but produces no pull-request " + f"checks; the required set names a check that never reports" + ) + jobs = workflows[filename]["jobs"] + assert [j for j in jobs.values() if j.get("name") == display], ( + f"{filename} has no job displaying as {display!r}, so branch " + f"protection would wait for a status that never arrives" + ) + + +def test_no_aggregating_gate_ignores_part_of_its_needs(): + """A gate that summarises N jobs must fail on any of them. + + `Simulation Gate` declared needs: [simulate, cross-platform] and tested + only needs.simulate.result before printing "All simulation checks passed", + so a red or skipped cross-platform leg -- three OS legs -- passed it. That + is the same fail-open shape ci.yml's gate was written to remove, in a job + whose name a maintainer would plausibly require. + """ + offenders = [] + for filename, doc in _pr_workflows().items(): + for job_id, job in doc["jobs"].items(): + needs = job.get("needs") + if not isinstance(needs, list) or len(needs) < 2: + continue + script = "\n".join(str(s.get("run", "")) for s in job.get("steps", [])) + if "needs" not in script: + continue + + # Only jobs that *adjudicate*. A job that writes a step summary + # and never claims a verdict -- book-build.yml's `summary` -- is + # reporting, not gating, and holding it to this rule would be + # noise. The tell is that it either fails the run or asserts that + # everything passed. + gates = "exit 1" in script or re.search( + r"(all .*(check|test|job)s? .*(passed|succeeded))", script, re.I) + if not gates: + continue + # A gate that names its dependencies one at a time can fall out of + # step with `needs`; one that iterates toJSON(needs) cannot. + iterates = "toJSON(needs)" in str(job.get("steps", "")) + if iterates: + continue + # Printing a result is not testing it. Simulation Gate echoed + # needs.cross-platform.result and then branched on + # needs.simulate.result alone, so a check that merely greps for + # the name would have passed it. Only a line that *compares* the + # result counts. + compared = set() + for line in script.splitlines(): + if "!=" not in line and "==" not in line: + continue + for n in needs: + if f"needs.{n}.result" in line: + compared.add(n) + unchecked = [n for n in needs if n not in compared] + if unchecked: + offenders.append( + f"{filename}:{job_id} declares needs {needs} but never " + f"tests {unchecked}" + ) + assert not offenders, ( + "an aggregating gate must fail on any non-success among its " + "dependencies:\n " + "\n ".join(offenders) + ) + + +def test_gate_fails_on_any_non_success_result(jobs): + """A skipped or cancelled job must fail the gate, not pass it.""" + steps = jobs[GATE_ID]["steps"] + script = "\n".join(str(s.get("run", "")) for s in steps) + + assert '!= "success"' in script, ( + "the gate must require success specifically. Checking only for " + "'failure' lets a skipped or cancelled job through, which is the " + "fail-open shape this repository has been removing elsewhere" + ) + assert "exit 1" in script, "the gate must actually fail the job" From ffb5ff90bf140866cc71cd6a1154db17ae7fec94 Mon Sep 17 00:00:00 2001 From: Kartikey1306 Date: Thu, 3 Sep 2026 16:26:45 +0530 Subject: [PATCH 3/3] ci: execute the gate rule in a test, instead of grepping for it Finishes finding 3 from the review on #90. Findings 1 and 2 were already addressed in 1bd6a22; this is the one left. test_gate_fails_on_any_non_success_result searched the gate's `run:` text for `!= "success"` and `exit 1`. That is a string match on an implementation, not a check of behaviour: it passes for those tokens sitting in a comment or an unreachable branch, and fails for a correct rewrite expressing the same rule differently. The only real evidence that the rule works was the fork experiment, which is a one-off nothing re-runs. The rule now lives in .github/scripts/ci-gate-check.sh, and pytest runs it against real inputs: success, failure, skipped, cancelled, a mixed set, and an empty context. Both gates -- ci.yml's `CI Gate` and simulation-test.yml's `Simulation Gate` -- call that one script, and a further test asserts they do, so the behavioural cases cover every gate rather than one. The script also refuses an empty or null `needs` context. No results is not the same as no failures, and a gate that passes when it was handed nothing is the same fail-open shape in a different place. Verified by mutation: weakening the rule to `== "failure"` fails 3 of 18, and a gate that stops calling the script fails 1 of 18. Restored, 18 pass. 42 Python tests pass (1 skipped), ctest 20/20, all 16 workflow files parse. --- .github/scripts/ci-gate-check.sh | 37 +++++++++++++++ .github/workflows/ci.yml | 14 +----- .github/workflows/simulation-test.yml | 14 +----- tests/unit/test_ci_gate.py | 68 +++++++++++++++++++++++---- 4 files changed, 99 insertions(+), 34 deletions(-) create mode 100755 .github/scripts/ci-gate-check.sh diff --git a/.github/scripts/ci-gate-check.sh b/.github/scripts/ci-gate-check.sh new file mode 100755 index 0000000..9d919c0 --- /dev/null +++ b/.github/scripts/ci-gate-check.sh @@ -0,0 +1,37 @@ +#!/usr/bin/env bash +# Fail unless every job handed to us succeeded. +# +# Reads the `needs` context as JSON on stdin: +# {"build": {"result": "success"}, "test": {"result": "skipped"}} +# +# Any result other than "success" fails, `skipped` and `cancelled` included: a +# job that did not run did not verify anything, and a required check that +# passes on "did not run" is the fail-open shape this repository has spent #38, +# #59 and #82 removing from the boot path. +# +# This lives in a file rather than inline in the workflow so that the rule can +# be executed by a test with real inputs, instead of a test grepping the YAML +# for the string it expects to find there. +set -euo pipefail + +results=$(cat) + +if [ -z "$results" ] || [ "$results" = "null" ]; then + echo "::error::CI Gate received no job results; refusing to pass." >&2 + exit 1 +fi + +printf '%s\n' "$results" + +bad=$(printf '%s' "$results" | jq -r ' + to_entries[] + | select(.value.result != "success") + | " \(.key): \(.value.result)"') + +if [ -n "$bad" ]; then + echo "::error::CI Gate failed. These jobs did not succeed:" + printf '%s\n' "$bad" + exit 1 +fi + +echo "All jobs succeeded." diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 14ceb27..1032fc0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -200,18 +200,8 @@ jobs: needs: [test, build-arm, static-analysis] if: always() steps: + - uses: actions/checkout@v4 - name: Every job in this workflow must have succeeded env: RESULTS: ${{ toJSON(needs) }} - run: | - printf '%s\n' "$RESULTS" - bad=$(printf '%s' "$RESULTS" | jq -r ' - to_entries[] - | select(.value.result != "success") - | " \(.key): \(.value.result)"') - if [ -n "$bad" ]; then - echo "::error::CI Gate failed. These jobs did not succeed:" - printf '%s\n' "$bad" - exit 1 - fi - echo "All jobs succeeded." + run: printf '%s' "$RESULTS" | .github/scripts/ci-gate-check.sh diff --git a/.github/workflows/simulation-test.yml b/.github/workflows/simulation-test.yml index f3c1be0..c02de96 100644 --- a/.github/workflows/simulation-test.yml +++ b/.github/workflows/simulation-test.yml @@ -102,18 +102,8 @@ jobs: # fail-open shape ci.yml's gate was written to remove, in a job whose # name a maintainer would plausibly require. This form cannot fall out # of step with `needs:`. + - uses: actions/checkout@v4 - name: Every job in this workflow must have succeeded env: RESULTS: ${{ toJSON(needs) }} - run: | - printf '%s\n' "$RESULTS" - bad=$(printf '%s' "$RESULTS" | jq -r ' - to_entries[] - | select(.value.result != "success") - | " \(.key): \(.value.result)"') - if [ -n "$bad" ]; then - echo "::error::Gate failed. These jobs did not succeed:" - printf '%s\n' "$bad" - exit 1 - fi - echo "All jobs succeeded." + run: printf '%s' "$RESULTS" | .github/scripts/ci-gate-check.sh diff --git a/tests/unit/test_ci_gate.py b/tests/unit/test_ci_gate.py index 037d160..b51ce64 100644 --- a/tests/unit/test_ci_gate.py +++ b/tests/unit/test_ci_gate.py @@ -13,6 +13,9 @@ import re +import json +import subprocess + import yaml import pytest from pathlib import Path @@ -243,14 +246,59 @@ def test_no_aggregating_gate_ignores_part_of_its_needs(): ) -def test_gate_fails_on_any_non_success_result(jobs): - """A skipped or cancelled job must fail the gate, not pass it.""" - steps = jobs[GATE_ID]["steps"] - script = "\n".join(str(s.get("run", "")) for s in steps) +# ---- the rule itself, executed rather than pattern-matched ------------------- +# +# This test used to grep the gate's `run:` text for `!= "success"` and +# `exit 1`. That is a string match on an implementation, not a check of +# behaviour: it passes for those tokens sitting in a comment or an unreachable +# branch, and fails for a correct rewrite that expresses the same rule +# differently. The rule now lives in .github/scripts/ci-gate-check.sh and is +# run here against real inputs, so the fork experiment that first demonstrated +# it does not have to be repeated by hand. - assert '!= "success"' in script, ( - "the gate must require success specifically. Checking only for " - "'failure' lets a skipped or cancelled job through, which is the " - "fail-open shape this repository has been removing elsewhere" - ) - assert "exit 1" in script, "the gate must actually fail the job" +GATE_SCRIPT = Path(__file__).resolve().parents[2] / ".github" / "scripts" / "ci-gate-check.sh" + + +def _run_gate(payload): + return subprocess.run( + ["bash", str(GATE_SCRIPT)], input=payload, + capture_output=True, text=True, + ).returncode + + +def test_gate_script_exists(): + assert GATE_SCRIPT.is_file(), f"{GATE_SCRIPT} is missing" + + +@pytest.mark.parametrize("results,expected", [ + ({"a": {"result": "success"}}, 0), + ({"a": {"result": "success"}, "b": {"result": "success"}}, 0), + ({"a": {"result": "failure"}}, 1), + ({"a": {"result": "skipped"}}, 1), + ({"a": {"result": "cancelled"}}, 1), + ({"a": {"result": "success"}, "b": {"result": "skipped"}}, 1), +]) +def test_gate_script_accepts_only_all_success(results, expected): + assert _run_gate(json.dumps(results)) == expected + + +@pytest.mark.parametrize("payload", ["", "null"]) +def test_gate_script_refuses_an_empty_context(payload): + """No results is not the same as no failures.""" + assert _run_gate(payload) == 1 + + +def test_every_gate_invokes_the_shared_script(): + """One rule in one place, so the tests above cover every gate.""" + for workflow, display in REQUIRED_CHECKS.items(): + doc = yaml.safe_load((WORKFLOWS_DIR / workflow).read_text(encoding="utf-8")) + job = next(j for j in doc["jobs"].values() + if j.get("name") == display) + if not job.get("needs"): + continue + script = "\n".join(str(st.get("run", "")) for st in job.get("steps", [])) + assert "ci-gate-check.sh" in script, ( + f"{workflow}: {display!r} does not call " + f".github/scripts/ci-gate-check.sh, so its behaviour is not " + f"covered by the tests above." + )