ci: pin gitleaks, allowlist pre-submodule test history, and add an aggregate gate by BitHighlander · Pull Request #474 · BitHighlander/keepkey-firmware

BitHighlander · 2026-08-17T22:23:07Z

CI hardening in two independently revertable commits. Together they make the pipeline deterministic and structurally incapable of hiding a required-job failure.

Commit 1 — `52a78b525` scanner determinism

secret-scan went red across the entire repository with no change to it, taking the whole build and test graph down.

Run 32075116969 on #472: failed in 9s, everything downstream SKIPPED. Reproduced locally with CI's exact command — 12 findings, none from this release:

file	findings	era
`tests/test_ecies.py`	6	2014, inherited from Trezor
`tests/test_msg_eos_signtx.py`	4	2018
`tests/test_msg_ethereum_erc20_signtx_exchange.py`	2	2018

git rev-list 1af2ffe7de..HEAD intersected with the findings: 0.

The flagged lines are published BIP32 test vectors — WIF keys printed beside their own address and pubkey as documentation:

# address: 1CK7SJdcb8z9HuvVft3D91HLpLC6KSsGb
# pubkey:  0338d78612e990f2eea0c426b5e48a8db70b9d7ed66282b3b26511e0b1c75515a6
# privkey: L5X3rf5hJfRt9ZjQzFopvSBGkpnSotn4jKGLL6ECJxcuT2JgGh65

Root cause: GITLEAKS_VERSION resolved from releases/latest. A newer generic-api-key rule started flagging historical fixtures. Nothing here changed.

Two fixes. Allowlist ^tests/ — that path does not exist in the working tree (the python suite lived at the repo root before becoming deps/python-keepkey), but CI scans with fetch-depth: 0. Identical situation and remedy to the existing ^deps/ exemption, and scoped by path rather than regex because the findings are real key material by format. And pin gitleaks to 8.30.0.

The cost is written into the config rather than left implicit: a genuine secret under tests/ would not be caught, which is acceptable only while the directory doesn't exist — if it returns, the exemption goes in the same commit.

Negative-controlled both directions:

planted	result
fresh EC private key under `lib/firmware/`	detected (`leaks found: 1`)
same key under `tests/`	ignored (path-scoped as intended)

gitleaks detect --source . --verbose --redact → no leaks found, exit 0.

Commit 2 — `4e8d74667` the aggregate gate (closes #471)

A skipped job is not a red check. When a Stage-1 gate fails, GitHub marks the downstream graph skipped, the summary shows green ticks on whatever finished, and the absence of signal is indistinguishable from success.

Three occurrences on this release line: gitleaks on develop, lint-format timing out inside its apt.llvm.org install (#471), and gitleaks again (commit 1 above).

ci-gate needs every required job, runs if: always(), and fails unless each reports exactly success. failure, cancelled and skipped are all not-success — for a required job, they are. publish-emulator is excluded as workflow_dispatch-only.

Verified against the shapes that actually occurred:

scenario	result
all success	PASS
one skipped	FAIL
one cancelled	FAIL
one failure	FAIL
`python-integration-tests` cancelled — today's branch	FAIL
`lint-format` cancelled + 2 skipped — the #471 run	FAIL

Point branch protection at CI gate rather than the individual jobs. It is the only check whose green means the entire graph ran and passed.

Consequence, stated up front

release/7.14.2 will report RED after this merges, because python-integration-tests is killed at its 30-minute timeout on every run (#466). That has been the branch's true state all along — this PR only stops it being hidden. #466 becomes the unambiguous next blocker to a genuinely green graph.

This PR's own ci-gate will therefore also be red, for exactly that reason. That is the gate working, not failing.

secret-scan went red across the whole repository with no change to it. The install step resolved GITLEAKS_VERSION from releases/latest, and a newer generic-api-key rule began flagging published BIP32 test vectors in history: 12 findings across tests/test_ecies.py (2014, inherited from Trezor), tests/test_msg_eos_signtx.py and tests/test_msg_ethereum_erc20_signtx_exchange.py (2018). Zero of them are in any commit in 1af2ffe..HEAD. The flagged lines are WIF private keys printed next to their own address and public key as documentation. They are upstream test fixtures, published for a decade, and secure nothing. Two changes, because the failure had two causes. Allowlist ^tests/. That path does not exist in the working tree -- the python suite lived at the repository root before it became deps/python-keepkey -- but CI scans with fetch-depth 0, so gitleaks still reads those commits. Same situation and same remedy as the existing ^deps/ exemption. Scoped by path, not by regex: the findings are real key material by format, so a pattern exemption would suppress genuine secrets elsewhere in the tree. The cost is recorded in the config rather than left implicit: a real secret committed under tests/ would not be caught, which is acceptable only while the directory does not exist. Negative-controlled both ways -- a freshly generated EC private key planted under lib/firmware/ is still detected, the same key under tests/ is ignored. Pin gitleaks to 8.30.0. Tracking releases/latest means an upstream ruleset change can turn this gate red with no change here, and because every build job declares `needs: [.., secret-scan]`, the entire build and test graph is SKIPPED rather than failed -- silence that looks identical to success. That is the third instance of this class on this release (see #471 for lint-format, and the earlier gitleaks breakage). Partially addresses #424. Verified: gitleaks detect --source . --verbose --redact reports "no leaks found" and exits 0. Refs #424 #471

… jobs Every build and test job declares `needs: [lint-format, static-analysis, check-submodules, secret-scan]`. When one of those gate jobs fails, GitHub marks the whole downstream graph SKIPPED rather than failed -- and a skipped job is not a red check. The run summary shows green ticks on whatever finished, which reads as healthy unless someone opens the job list and notices that the ARM build, the unit tests and both python suites produced nothing. That has happened three times on this release line: gitleaks failing on develop, lint-format timing out inside its apt.llvm.org install (#471), and gitleaks again after an unpinned upstream bump (the commit before this one). Each time the absence of signal was indistinguishable from success. ci-gate needs every required job, runs `if: always()` so it executes even when they skip, and fails unless each reports exactly `success`. failure, cancelled and skipped are all not-success, because for a required job they are. publish-emulator is deliberately excluded: it is workflow_dispatch-only and is legitimately skipped on every push and pull_request. Point branch protection at this job rather than the individual ones. It is the only check whose green means "the entire graph ran and passed". Verified against the two shapes that actually occurred, plus the obvious ones: all success -> PASS one skipped -> FAIL one cancelled -> FAIL one failure -> FAIL python-integration-tests cancelled-> FAIL (today's release/7.14.2) lint-format cancelled + 2 skipped -> FAIL (the #471 run) Consequence, stated up front: release/7.14.2 will now report RED until #466 is fixed, because python-integration-tests is killed at its 30-minute timeout on every run. That is the honest state of the branch and has been all along; this job only stops it being hidden. Closes #471

…riant The integration suite is red, the quickest route to green is to stop showing a screen, and the commit that does it looks like a test fix. Writing the rule down is the cheapest defence against that, because the pressure arrives disguised as housekeeping and the result would look like progress. Tests adapt to newly required disclosure. Firmware disclosure is never removed to satisfy a stale test. If a test fails because the device now shows more, the test is wrong. The extra THORChain screens disclose affiliate-fee bytes that were previously signed invisibly. Also replaces the known-red section, which was wrong in two ways. It said the integration stall was pre-existing at 4406197 -- a controlled run (same test files, only firmware varying) showed 1af2ffe passes and head hangs, so this release introduced it. And it could not enumerate the failures at all, because the job was killed before producing JUnit XML. With #477 the suite completes in 134s and the list is finite: 22 failed, 362 passed, 47 skipped, every failure a stale pinned expectation and none a firmware defect. Recorded as a table with an owner per row, since that is now the concrete work list for the repin. Adds the two measurement traps that cost real time here: do not run the suite sequentially against one emulator, and treat the PINNED submodule commit as the test source of truth rather than the working tree -- reading master's copy of test__sign_transformERC20 produced a confident and wrong "this breaks nothing". Refs #466 #474 #477

The emulator test script detected the firmware version with: FW_VERSION=$(sed -n '/^project/,/)/p' CMakeLists.txt \ | grep -oP '\d+\.\d+\.\d+' || echo "7.14.0") `grep -oP` is a GNU extension. This container's grep is BusyBox, which has no -P, so that command ALWAYS failed and the fallback silently supplied 7.14.0 -- on the 7.14.2 release branch. CI has been reporting "Detected FW_VERSION=7.14.0 from CMakeLists.txt" while CMakeLists says 7.14.2. Everything downstream keys off that number. SECTIONS entries are selected by ver_ge(fw_version, min_fw), so at 7.14.0 every test gated to 7.14.1 or later was excluded from BOTH the screenshot filter and validate_junit. That is the root cause of the gap #487 set out to fix: the suites this release changed captured no screens because the report system did not believe those tests existed yet. It also means the per-test screenshot audit added in #487 passed vacuously -- with no 7.14.2 sections active, nothing declared screens, so nothing could be missing. The audit was correct; it was being asked about the wrong release. Two changes: - read VERSION with sed alone, portable to BusyBox - FAIL if it cannot be read, rather than defaulting. A wrong version here is invisible and silently narrows what CI checks -- exactly the failure mode ci-gate (#474) and the screenshot audit were added to prevent. Same class as those two: the check ran, went green, and was measuring something other than what its name implied.

…at all The alpha<-develop merge left .gitleaks.toml with BOTH allowlist forms — alpha's three `[[allowlists]]` and develop's one `[allowlist]`. gitleaks refuses that outright: FTL Failed to load config error="[allowlist] is deprecated, it cannot be used alongside [[allowlists]]" secret-scan is a stage-1 gate, so its failure SKIPPED the entire build graph: build-emulator, build-arm-firmware, unit-tests, python-integration-tests, python-dylib-tests and generate-test-report all reported "skipped", and the run looked like one red job rather than a release with no evidence behind it. This is the silent-skip class #474 added the aggregate `CI gate` for, and that gate did its job — it failed and named all seven. Fix is presentational only: develop's `[allowlist]` becomes a fourth `[[allowlists]]` entry. No path, regex or rule changed, and neither side's exemptions are dropped — alpha's three are narrow (path AND regex, for the AES test vectors, ed25519 type names and the rc21 doc pin) and develop's is broad path scoping (U2F attestation artifacts, deps/, docs/, tests/) with its rationale intact. Verified locally with gitleaks 8.30.0, CI's exact command: gitleaks detect --source . --verbose --redact -> 7586 commits scanned, no leaks found, exit 0 And the negative control develop's own comment claims, re-run because a merge is exactly when an exemption silently widens — a fresh prime256v1 key planted in two places, JSON report: private-key -> lib/firmware/NEGCTRL_planted.pem CAUGHT (same key under deps/) exempt, as documented Neither gate in tools/ could have caught this. merge_symbol_gate.py reads C symbols; merge_direction_gate.py only flags a file taken verbatim from one side, and this file was genuinely merged — the union was syntactically valid TOML and semantically invalid to the tool that consumes it. Config files merged from both sides need the consuming tool run against them, not a diff read.

Five findings from the Copilot review on #474, all valid. * The mirror was not reaching the buildx builds (medium). `Pull base image` re-tags the GHCR mirror to the Docker Hub name so `FROM` resolves locally. That works for `docker build`, which reads the daemon image store, but buildx uses the docker-container driver and resolves FROM against a registry. Run 33046944618 confirms it: the buildx step logged `[auth] kktech/firmware:pull token for registry-1.docker.io`, so the python-keepkey images were still pulling from Docker Hub and the mirror bought nothing there. The resolved reference is now exported as RESOLVED_BASE and passed as a build-arg, with the Dockerfile taking `ARG BASE_IMAGE` ahead of its FROM. The default keeps plain `docker build` and local use unchanged. * Compose failure aborted before report extraction (high). `run:` steps execute under `bash -e`, so `docker compose up ...; PY_RC=$?` never reached the `$?` assignment on failure -- the step died and none of the extraction or evidence gating ran, losing the reports on exactly the runs where they matter. Now `|| PY_RC=$?`, which is exempt from -e. * A red unit suite suppressed the integration suite (medium). Consequence of merging what used to be two jobs: `make xunit` failing aborted the step and skipped everything after it. Both suites now always run; their exit codes are captured and the verdict is applied once, at the end, after every artifact is captured. * Mirror pull errors were being swallowed (low). Dropped `2>/dev/null`. The fallback keeps a mirror problem non-fatal, but hiding stderr made auth vs missing tag vs transient network indistinguishable. * Healthcheck comment stated a wrong bound (low). With timeout 3s, interval 1s and retries 40 the ceiling is roughly start_period + retries * (timeout + interval), not retries * interval. The comment claimed 40s.

…sues Suppressed finding from the round-2 Copilot review on #474 -- flagged as "previously missed, in code that hasn't changed since the last review", and present on develop rather than introduced here. cppcheck runs with --error-exitcode=1, so it returns non-zero whenever it finds anything. `run:` steps execute under `bash -e`, so `cppcheck ...; CPPCHECK_RC=$?` terminated the step at the cppcheck line on exactly the runs that have findings. The `cat cppcheck_report.txt` that emits the ::warning file= annotations, the per-severity summary written to GITHUB_STEP_SUMMARY, and the explanatory ::error:: all never executed -- leaving a bare red step with no indication of what cppcheck actually found. Captured with `|| CPPCHECK_RC=$?` instead, which is exempt from -e. Third instance of this pattern in the file; the other two were fixed in 050d600.

Suppressed finding from the round-3 Copilot review on #474 (ci.yml:385, severity medium). The unit-test container copies its XML with `cp -r unittests/*.xml ... 2>/dev/null`. If make xunit stops emitting XML, or the glob stops matching, that error is discarded and the job carries on to generate a report showing "0 tests" with nothing objecting. That is the same silent-evidence-loss mode the python side hit earlier on this branch, where a failed docker cp let a run go green having discarded 437 test results. Gated the same way: missing XML is an infrastructure failure distinct from a test failure, and it now fails at the point of loss rather than being discovered in a PDF.

Two suppressed low-severity findings from the round-4 Copilot review on #474, both in code this PR touched but did not introduce. * ci.yml:148 -- `grep -v '.pb.'` treats the dots as wildcards, so it would exclude any <char>pb<char> path from the formatting check, not only generated protobuf output. Switched to `grep -vF`. Checked against the current tree first: of the 197 files the check scans, the filter currently excludes zero either way, so this is latent rather than active -- but a future foo.pb.c would diverge. * mirror-base-image.yml:45 -- the comment read as though two references were pushed (bare name and tag). Only one tagged reference is pushed. Reworded to say what the script does.

From the round-5 Copilot review on #474 (ci.yml:148). The finding made two claims; one holds and one does not. Holds: lib/transport/src has never existed in this repo. The transport sources sit directly in lib/transport/. Because find's stderr went to /dev/null the bad path contributed nothing and said nothing. Dropped, and stderr is no longer suppressed so a future bad path is loud rather than quietly narrowing coverage. Does not hold: the claim that `-name '*.c' -o -name '*.h'` skips .c files via -o short-circuiting. GNU find applies the implicit -print to the whole expression. Verified under GNU findutils 4.9.0 -- the current expression yields 77 .c and 120 .h. Parenthesised anyway so intent is explicit; the resulting file set is byte-identical at 77/120. The recommendation to add lib/transport to the check is NOT taken, because it would turn CI red. That directory holds the vendored nanopb runtime, and pb_decode.c does not satisfy this repo's .clang-format (verified with clang-format 20.1.8). Reformatting vendored upstream sources to house style would make future nanopb updates needlessly painful. Recorded as a comment so the exclusion reads as deliberate rather than accidental.

From the round-5 Copilot review on #474. Filed as low severity; the blast radius is larger than that once ci.yml consumes the mirror. The workflow mirrored any dispatched image reference and derived the destination from the input via ${SOURCE##*/}, which keeps only the last path segment. So dispatching anyone/firmware:v15 resolved to ghcr.io/<owner>/firmware:v15 -- the exact reference ci.yml now pulls and builds firmware from. Dispatch requires write access, but "a writer can typo" and "a writer can silently replace the base image every firmware build trusts" are different problems. Two changes: * The source must match ^kktech/firmware:[A-Za-z0-9._-]+$. Anything else is refused with an explanatory error. * The destination repository name is now fixed rather than derived from input; only the validated tag is taken from the dispatch. Verified against kktech/firmware:v15 and :v16 (accepted) and attacker/firmware:v15, ghcr.io/evil/firmware:v15, kktech/other:v15, and a shell-metacharacter tag (all refused).

Suppressed high-severity finding from the round-6 Copilot review on #474, and a real regression introduced by this branch. Taking static-analysis out of the build gate was deliberate: a cppcheck finding does not mean the firmware is wrong to compile, and as the slowest Stage 1 job it was serialising the pipeline behind itself. But publish-emulator used to inherit that gate transitively, through build jobs that each depended on static-analysis. Rewriting its needs list onto the merged build-and-test dropped the dependency altogether, so a workflow_dispatch publish could push kktech/kkemu images to DockerHub from a commit with cppcheck red -- the overall run would be red, but the push would still complete. static-analysis is now listed explicitly on publish-emulator. The publish path is manual and already slow, so the gate costs nothing there, and the speed trade was only ever meant to apply to compiling.

…otoc cache Four suppressed findings from the round-7 Copilot review on #474. * GHCR login was fatal, defeating the fallback it exists to enable (medium, both build jobs). The mirror is optional by design -- ci.yml warns and falls back to Docker Hub when the pull fails -- but docker/login-action failing aborts the job before that fallback can run. A GHCR outage, a revoked package grant or a token hiccup would therefore take CI down despite the fallback. Marked continue-on-error; a failed login just means the mirror pull is unauthenticated, which takes the fallback path. * build-arm-firmware set up Buildx and exported RESOLVED_BASE, using neither (low). Introduced here: the pull-with-fallback step was applied to both build jobs identically, and buildx came along with it. That job runs the toolchain via docker run and needs only the daemon-local tag. Removed, along with the buildx-specific commentary that did not apply to it. * protoc download skipped on existence rather than content (low). An actions/cache entry can restore empty or truncated; -f would skip the download and hand unzip a bad file with no retry. Uses -s. The buildx/RESOLVED_BASE machinery remains in build-and-test, which does need it: buildx resolves FROM against a registry rather than the daemon.

@v4

Suppressed findings from the round-8 Copilot review on #474. The workflow mixed @v4 and @v7 across steps -- in places within the same job -- which is pre-existing rather than introduced here. Both versions were demonstrably working side by side, so this is consistency rather than a fix, but there is no reason for one workflow to pin two majors of the same action. All five remaining @v4 uploads moved to @v7. download-artifact stays at @v8, which is its own current major.

perf(ci): cut pipeline wall clock from ~7:20 to ~3:00

Three conflicts, plus one semantic clash the auto-merge hid. .github/workflows/ci.yml -- took develop's pipeline (PR #474 cut it from ~7:20 to ~2:00 and folded check-submodules, build-emulator, unit-tests and python-integration-tests into build-and-test) and re-applied what this branch adds on top: - permissions: contents: read at the workflow level. develop has no permissions block, so code-scanning rule actions/missing-workflow-permissions had one alert open per job. - generate-test-report restored as its OWN job rather than folded into build-and-test. develop's fold is correct for develop's report script, which only needs directories that job already has. This branch's generate-test-report.py also consumes test-reports/dylib-junit.xml and the ARM manifest -- produced by other jobs, on other runners -- and fail()s hard when either is missing. Folded, the report could never succeed. - the report's provenance env (both source SHAs, run URL, both PR URLs). develop passes only KK_BUILD_LABEL, which this branch's script does not read; the fields it does read are release gates for 7.14.2. - the report artifact is a directory again. The script emits test-report/test-report.pdf beside test-report-manifest.json and test-report.pdf.sha256; develop uploaded a bare test-report.pdf, which would have found no files. - release-evidence-gate, rewired to develop's folded job names, so a release still has one required check that every evidence-producing job succeeded. .gitleaks.toml -- union. Kept develop's header and its docs-SHA allowlist, which is broader than and subsumes this branch's python-keepkey-pin rule, plus this branch's U2F/vendored-deps allowlist and the note recording why no first-party test tree is exempted. scripts/emulator/python-keepkey.Dockerfile -- took develop's parameterised deps-stage version wholesale. scripts/emulator/Dockerfile did NOT conflict: only this branch had changed it (base -> digest), so git kept the digest silently. That defeats develop's GHCR mirror, which tags the mirrored image as kktech/firmware:v15 -- a digest FROM ignores that tag and pulls from Docker Hub on the heaviest job in the run. Parameterised it the way python-keepkey.Dockerfile already is, defaulting to the digest so a release build still names an immutable base, with CI passing the resolved mirror. kktech/firmware:v15 resolves to exactly that digest today. Verified: emulator image builds both with the default and with an explicit --build-arg; 153/153 unit tests pass; gitleaks 8.30.1 finds no leaks with the merged config; clang-format clean.

BitHighlander mentioned this pull request Aug 17, 2026

fix(confirm): stop review() reporting success when the host took the screen away #475

Merged

BitHighlander changed the title ~~ci(secret-scan): allowlist pre-submodule test history, and pin gitleaks~~ ci: pin gitleaks, allowlist pre-submodule test history, and add an aggregate gate Aug 17, 2026

BitHighlander mentioned this pull request Aug 17, 2026

python-integration-tests deadlocks: this release adds confirm screens the pinned tests never ack #466

Open

BitHighlander merged commit fc19dda into release/7.14.2 Aug 17, 2026
8 of 9 checks passed

BitHighlander deleted the fix/secret-scan-historical-tests-and-pin branch August 17, 2026 22:57

This was referenced Aug 17, 2026

ci(integration): bound every test, so a deadlock fails by name instead of by timeout #477

Merged

docs(release): make "disclosure never yields to a stale test" an invariant #478

Merged

BitHighlander mentioned this pull request Aug 20, 2026

ci: fail when a test declares screens it never captures #487

Merged

BitHighlander mentioned this pull request Aug 20, 2026

docs(release): gate status after the rc30 hardware round #490

Merged

BitHighlander pushed a commit that referenced this pull request Aug 27, 2026

Merge pull request #474 from keepkey/perf/ci-pipeline-speed

fc1e937

perf(ci): cut pipeline wall clock from ~7:20 to ~3:00

Navigation Menu

ci: pin gitleaks, allowlist pre-submodule test history, and add an aggregate gate - #474

ci: pin gitleaks, allowlist pre-submodule test history, and add an aggregate gate#474
BitHighlander merged 2 commits into
release/7.14.2from
fix/secret-scan-historical-tests-and-pin

BitHighlander commented Aug 17, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

BitHighlander commented Aug 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Commit 1 — 52a78b525 scanner determinism

Commit 2 — 4e8d74667 the aggregate gate (closes #471)

Consequence, stated up front

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

BitHighlander commented Aug 17, 2026 •

edited

Loading

Commit 1 — `52a78b525` scanner determinism

Commit 2 — `4e8d74667` the aggregate gate (closes #471)