From 388e63179d1a65d2ddc81fe55b73d932c3675959 Mon Sep 17 00:00:00 2001 From: highlander Date: Fri, 21 Aug 2026 23:55:41 -0500 Subject: [PATCH 1/6] fix(ci): a crashed emulator now fails in seconds instead of hanging 30 minutes The integration job has ended "cancelled" at exactly 30 minutes on every master run for at least six merges, while a green check named "Integration Tests" sat next to it. Four defects stacked. THE HANG. The emulator segfaulted mid-suite -- the service log reads "Application Version 7.10.0 / Segmentation fault (core dumped)" -- and transport_udp.py never called settimeout(), so _raw_read() blocked in recv() until something outside killed the process. 243 of 662 tests ran in 6 seconds, then 29 minutes of nothing. The next file in collection order is test_msg_ethereum_erc20_uniswap_liquidity.py, which matches the known Uniswap liquidity defect, so the crash is probably reproducible and this change is what will let anyone see it. Now raises IOError naming the device, the port and the timeout. Verified against a socket that is BOUND but never answers -- a crashed emulator whose container still holds the port, which is the case ICMP does not cover: 3.0s and a named error, where before it blocked indefinitely. KK_UDP_TIMEOUT overrides; 0 disables for interactive debugging. THE FALSE GREEN. mikepenz/action-junit-report was given check_name, which makes it publish a SEPARATE check run through the Checks API. Its require_tests default is 'false', so the absent junit.xml a killed pytest leaves behind reported conclusion:success -- created already-completed, so started_at == completed_at, the zero duration. Now annotate_only with require_tests and fail_on_failure on, so it annotates and never mints a verdict of its own. CANCELLED IS NOT A FAILURE. A job-level timeout ends the job "cancelled", which reads as an infrastructure blip; the "Fail on test failure" step correctly evaluated to failure and was overridden. pytest now carries a 10-minute STEP timeout, so a hang is reported as what it is, with the job backstop lowered 30 -> 14. CYCLE TIME. Added a concurrency group with cancel-in-progress so a new push supersedes the old run rather than both burning a runner. NOT FIXED HERE, and it is the reason none of this was caught: master has NO branch protection at all -- `gh api .../branches/master/protection` returns 404 and rulesets is []. A required check whose conclusion is "cancelled" would have blocked every one of these merges. --- .github/workflows/ci.yml | 25 ++++++++++++++++++++++--- keepkeylib/transport_udp.py | 29 ++++++++++++++++++++++++++++- 2 files changed, 50 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4831275f..0d266fe2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -17,6 +17,12 @@ on: pull_request: branches: [master, develop, reconcile/upstream-sync] +# One run per ref: a new push supersedes the old instead of both burning a +# runner to completion. +concurrency: + group: ci-${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} + cancel-in-progress: true + jobs: # ═══════════════════════════════════════════════════════════ # STAGE 1: GATE @@ -62,7 +68,7 @@ jobs: integration: needs: [lint] runs-on: ubuntu-latest - timeout-minutes: 30 + timeout-minutes: 14 services: kkemu: @@ -101,11 +107,16 @@ jobs: sleep 1 done + # Step-level timeout, deliberately: a JOB-level timeout ends the job as + # "cancelled", which reads as an infra blip. A step timeout is a FAILURE. - name: Run integration tests + timeout-minutes: 10 env: KK_TRANSPORT_MAIN: "127.0.0.1:11044" KK_TRANSPORT_DEBUG: "127.0.0.1:11045" PYTHONPATH: "${{ github.workspace }}/keepkeylib:${{ github.workspace }}" + # A crashed emulator now raises instead of blocking in recv() forever. + KK_UDP_TIMEOUT: "45" run: | cd tests pytest -v --junitxml=junit.xml 2>&1 | tee pytest-output.txt @@ -161,12 +172,20 @@ jobs: echo "---" >> "$GITHUB_STEP_SUMMARY" echo "*KeepKey python-keepkey CI*" >> "$GITHUB_STEP_SUMMARY" - - name: Upload test results + # NO check_name. With one, this action publishes a SEPARATE check run + # via the Checks API, and its require_tests default of 'false' means an + # absent junit.xml -- which is exactly what a killed pytest leaves behind + # -- reports conclusion:success with zero duration. That green check sat + # on top of a job timing out at 30 minutes for at least six merges. + # annotate_only keeps the inline annotations without minting a check. + - name: Annotate test results uses: mikepenz/action-junit-report@v4 if: always() with: report_paths: tests/junit.xml - check_name: Integration Tests + annotate_only: true + require_tests: true + fail_on_failure: true - name: Fail on test failure if: always() diff --git a/keepkeylib/transport_udp.py b/keepkeylib/transport_udp.py index 05767de7..1dbdf672 100644 --- a/keepkeylib/transport_udp.py +++ b/keepkeylib/transport_udp.py @@ -2,10 +2,23 @@ '''SocketTransport implements TCP socket interface for Transport.''' +import os import socket from select import select from .transport import Transport +# A dead emulator must surface as an ERROR, not as an infinite wait. +# +# The socket had no timeout, so when the emulator segfaulted mid-suite, +# recv() blocked in a syscall until something outside killed the process -- +# in CI that was a 30-minute job timeout reported as "cancelled", which reads +# as an infrastructure blip rather than the device crash it actually was. It +# hid a real segfault for at least six merges. +# +# Generous by default because a confirm screen legitimately waits on a human; +# override for unattended runs with KK_UDP_TIMEOUT (seconds, 0 disables). +DEFAULT_TIMEOUT = float(os.environ.get('KK_UDP_TIMEOUT', '60')) + class FakeRead(object): # Let's pretend we have a file-like interface def __init__(self, func): @@ -31,6 +44,8 @@ def __init__(self, device, *args, **kwargs): def _open(self): self.socket = socket.socket(socket.AF_INET, socket.SOCK_DGRAM) self.socket.connect(self.device) + if DEFAULT_TIMEOUT > 0: + self.socket.settimeout(DEFAULT_TIMEOUT) def _close(self): self.socket.close() @@ -57,7 +72,19 @@ def _read(self): def _raw_read(self, length): while len(self.buffer) < length: - data = self.socket.recv(64) + try: + data = self.socket.recv(64) + except socket.timeout: + # Name the cause. "timed out" alone sends people looking at the + # test; the device is what stopped answering. + raise IOError( + 'No response from the emulator at %s:%d after %gs -- it is ' + 'not running, has crashed, or is wedged on a confirm screen ' + 'nothing acknowledged. Set KK_UDP_TIMEOUT to change or 0 to ' + 'disable.' % (self.device[0], self.device[1], + DEFAULT_TIMEOUT)) + if not data: + raise IOError('Emulator closed the connection') self.buffer += data[1:] ret = self.buffer[:length] From 1e3ff0504c3e5c6f29ded5e74721ce8fb1a5d1ed Mon Sep 17 00:00:00 2001 From: highlander Date: Sat, 22 Aug 2026 00:03:29 -0500 Subject: [PATCH 2/6] fix(ci): the emulator image is five months stale, and a test was killing Docker Two more defects behind the same 30-minute wall, both found by tracing the crash rather than by reading the workflow. THE SEGFAULT IS A STALE IMAGE, NOT A FIRMWARE BUG. CI's service container is `kktech/kkemu:latest`, a FLOATING tag whose current image was built 2026-03-12 and reports firmware 7.10.0 -- six minor versions behind the suite that runs against it. 7.10.0's zxliquidtx.c formats the Uniswap deadline with ctime(); the test vectors carry a JavaScript MILLISECOND timestamp, which as time_t is ~year 53234, and on the image's Alpine 3.8 musl that segfaults. Reproduced inside the image directly. Current firmware does not call ctime at all -- it snprintf's PRIu64 -- and all three tests PASS against a locally built 7.15.0. So the tests were right the whole time. Worse, 80 tests gate on requires_firmware("7.15.0") and have been SILENTLY SKIPPING against that image, and it predates -DKK_CLEARSIGN_TEST_ROOT=ON entirely. Added a version gate that runs before pytest and fails closed if the emulator is older than the suite. "It answered a ping" is not "it is the right firmware", and a floating tag cannot tell you which you have. A TEST WAS KILLING THE DOCKER DAEMON. test_msg_session_trust_lifetime's _power_cycle() finds "the process bound to udp/11044" with lsof and kills it. When the emulator runs in a container that process is the port forwarder -- docker-proxy or dockerd on Linux, com.docker.backend on macOS -- in a different pid namespace from kkemu, which never appears in the host namespace at all. Killing it does not reboot anything: it removes the port forward, and every later test blocks forever on a socket that will never answer. It took Docker Desktop down three separate times on this machine tonight while we were building firmware, which is how it was found. _emulator_process() now refuses to return any pid whose basename is not kkemu, so _power_cycle takes its documented skip instead. No coverage is deleted and the uniswap tests are untouched -- they are correct. Measured healthy suite runtime: 83.64s for 656 tests, 4 failed, 627 passed, 31 skipped. The job budget was 30 minutes. pytest now bounded at 8 minutes, job backstop 15. --- .github/workflows/ci.yml | 45 ++++++++++++++++++++++-- tests/test_msg_session_trust_lifetime.py | 19 ++++++++++ 2 files changed, 62 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0d266fe2..037e9711 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -68,10 +68,14 @@ jobs: integration: needs: [lint] runs-on: ubuntu-latest - timeout-minutes: 14 + timeout-minutes: 15 services: kkemu: + # kktech/kkemu:latest on Docker Hub is firmware 7.10.0, built + # 2026-03-12 -- five months and six minor versions behind the suite + # that runs against it. Pin a digest once a current image is published; + # until then the version gate below is what fails closed. image: kktech/kkemu:latest ports: - 11044:11044/udp @@ -107,10 +111,47 @@ jobs: sleep 1 done + # "The emulator answered a ping" is not "the emulator is the right + # firmware". CI ran a 7.16-era suite against a 7.10.0 image for five + # months: 80 tests gate on requires_firmware("7.15.0") and silently + # SKIPPED, while one unskipped test drove a code path that segfaults in + # 7.10.0 and is already fixed in 7.15 -- which reads as a product failure + # but is only a stale image. A floating tag cannot tell you that. This + # can, and it fails closed. + - name: Assert the emulator is not older than the suite + timeout-minutes: 2 + env: + KK_TRANSPORT_MAIN: "127.0.0.1:11044" + KK_TRANSPORT_DEBUG: "127.0.0.1:11045" + KK_MIN_FW: "7.15.0" + KK_UDP_TIMEOUT: "20" + working-directory: tests + run: | + python - <<'PY' + import os, sys + sys.path.insert(0, '..') + import config + from keepkeylib.client import KeepKeyDebuglinkClient + c = KeepKeyDebuglinkClient(config.TRANSPORT(*config.TRANSPORT_ARGS, + **config.TRANSPORT_KWARGS)) + c.set_debuglink(config.DEBUG_TRANSPORT(*config.DEBUG_TRANSPORT_ARGS, + **config.DEBUG_TRANSPORT_KWARGS)) + c.init_device() + f = c.features + got = (f.major_version, f.minor_version, f.patch_version) + floor = tuple(int(x) for x in os.environ['KK_MIN_FW'].split('.')) + print('emulator firmware %d.%d.%d, floor %s' % + (got + (os.environ['KK_MIN_FW'],))) + if got < floor: + sys.exit('FATAL: the emulator image predates the tests that run ' + 'against it. Republish kktech/kkemu from current ' + 'firmware and pin the new digest above.') + PY + # Step-level timeout, deliberately: a JOB-level timeout ends the job as # "cancelled", which reads as an infra blip. A step timeout is a FAILURE. - name: Run integration tests - timeout-minutes: 10 + timeout-minutes: 8 env: KK_TRANSPORT_MAIN: "127.0.0.1:11044" KK_TRANSPORT_DEBUG: "127.0.0.1:11045" diff --git a/tests/test_msg_session_trust_lifetime.py b/tests/test_msg_session_trust_lifetime.py index 7861ec33..76e5caa0 100644 --- a/tests/test_msg_session_trust_lifetime.py +++ b/tests/test_msg_session_trust_lifetime.py @@ -91,6 +91,11 @@ def probe_blob(): )) +# Names `ps -o comm=` reports for the emulator binary. Anything else bound to +# the port is not ours to kill -- see the guard in _emulator_process(). +_EMULATOR_EXE_NAMES = ('kkemu',) + + def _emulator_process(port): """(pid, exe, cwd) of the process BOUND to udp/port, or None. @@ -124,6 +129,20 @@ def _emulator_process(port): exe = subprocess.run(['ps', '-o', 'comm=', '-p', str(pid)], stdout=subprocess.PIPE, stderr=subprocess.PIPE, universal_newlines=True).stdout.strip() + if os.path.basename(exe) not in _EMULATOR_EXE_NAMES: + # Whatever holds this port, it is not the firmware. Whenever the + # emulator runs in a container the bound process is the Docker + # port forwarder -- docker-proxy or dockerd on Linux, + # com.docker.backend on macOS -- in a different pid namespace + # from kkemu. Killing it does not reboot anything: it removes + # the port forward, and every later test in the run then blocks + # forever on a socket that will never answer again. Measured + # here: it took the whole Docker daemon down mid-suite. + # + # Fall through to "not found" so _power_cycle() takes its + # documented skip, which the report renders as WITHHELD rather + # than as a pass. + continue cwd_out = subprocess.run( ['lsof', '-a', '-p', str(pid), '-d', 'cwd', '-Fn'], stdout=subprocess.PIPE, From 073f2eaae85e8e665e5031515f292fb4d1ea74eb Mon Sep 17 00:00:00 2001 From: highlander Date: Sat, 22 Aug 2026 00:09:15 -0500 Subject: [PATCH 3/6] fix(ci): build the emulator from source instead of pulling a stale tag The job pulled kktech/kkemu:latest -- a FLOATING tag whose image was built 2026-03-12 and reports firmware 7.10.0, five months and six minor versions behind the suite running against it. That one fact caused every symptom: 80 tests gating on requires_firmware("7.15.0") skipped in silence, and one unskipped test drove a ctime() path that segfaults on that image and does not exist in current firmware. Publishing a fresher image would only reset the clock and wait for the same failure. Building from source removes the class -- the emulator under test is, by construction, the firmware the tests were written against, and there is nothing to publish, pin, or remember to refresh. python-keepkey is a submodule OF the firmware repo, so the job now checks out BitHighlander/keepkey-firmware@alpha alongside it and overlays THIS checkout of python-keepkey over the pinned one -- otherwise it would test whatever revision firmware happens to pin rather than the PR under review. The version gate from the previous commit stays. It is now a belt-and- braces check rather than the only defence, and it still earns its place: it catches the day someone points this at a branch that has regressed. Cost: one emulator build per run, bounded at 20 minutes. Measured healthy suite runtime is 83.64s, so the build dominates -- and that is the right trade against a job that spent 30 minutes producing no signal at all. --- .github/workflows/ci.yml | 65 +++++++++++++++++++++++++++++----------- 1 file changed, 48 insertions(+), 17 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 037e9711..f2ec2d56 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -70,28 +70,59 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 15 - services: - kkemu: - # kktech/kkemu:latest on Docker Hub is firmware 7.10.0, built - # 2026-03-12 -- five months and six minor versions behind the suite - # that runs against it. Pin a digest once a current image is published; - # until then the version gate below is what fails closed. - image: kktech/kkemu:latest - ports: - - 11044:11044/udp - - 11045:11045/udp - - 5000:5000 + # NO published emulator image. This job BUILDS one from current firmware. + # + # It used to pull kktech/kkemu:latest -- a floating tag whose image was + # five months and six minor versions stale. That single fact caused every + # symptom we chased: 80 tests gating on requires_firmware("7.15.0") skipped + # silently, and one unskipped test drove a ctime() path that segfaults on + # the old image and does not exist in current firmware. + # + # Publishing a fresher image would only reset that clock. Building from + # source removes the class: the emulator under test is, by construction, + # the firmware the tests were written against. steps: - uses: actions/checkout@v4 with: submodules: recursive + path: python-keepkey + + # python-keepkey is a SUBMODULE of the firmware repo, so the firmware is + # where the emulator lives. alpha is the fork's integration branch. + - name: Checkout firmware + uses: actions/checkout@v4 + with: + repository: BitHighlander/keepkey-firmware + ref: alpha + submodules: recursive + path: keepkey-firmware + + # Test THIS checkout of python-keepkey, not the one the firmware pins. + - name: Overlay this python-keepkey onto the firmware tree + run: | + rm -rf keepkey-firmware/deps/python-keepkey + cp -a python-keepkey keepkey-firmware/deps/python-keepkey + + - name: Build the emulator + timeout-minutes: 20 + working-directory: keepkey-firmware + run: | + docker build -t kkemu-ci -f scripts/emulator/Dockerfile . + + - name: Start the emulator + run: | + docker run -d --name kkemu \ + -p 11044:11044/udp -p 11045:11045/udp -p 5000:5000 kkemu-ci + sleep 3 + docker logs kkemu | head -5 - uses: actions/setup-python@v5 with: python-version: '3.11' - name: Install dependencies + working-directory: python-keepkey run: | pip install --upgrade pip pip install "protobuf>=3.20,<4" @@ -125,7 +156,7 @@ jobs: KK_TRANSPORT_DEBUG: "127.0.0.1:11045" KK_MIN_FW: "7.15.0" KK_UDP_TIMEOUT: "20" - working-directory: tests + working-directory: python-keepkey/tests run: | python - <<'PY' import os, sys @@ -155,18 +186,18 @@ jobs: env: KK_TRANSPORT_MAIN: "127.0.0.1:11044" KK_TRANSPORT_DEBUG: "127.0.0.1:11045" - PYTHONPATH: "${{ github.workspace }}/keepkeylib:${{ github.workspace }}" + PYTHONPATH: "${{ github.workspace }}/python-keepkey/keepkeylib:${{ github.workspace }}/python-keepkey" # A crashed emulator now raises instead of blocking in recv() forever. KK_UDP_TIMEOUT: "45" run: | - cd tests + cd python-keepkey/tests pytest -v --junitxml=junit.xml 2>&1 | tee pytest-output.txt echo "${PIPESTATUS[0]}" > status - name: Test summary if: always() run: | - XML="tests/junit.xml" + XML="python-keepkey/tests/junit.xml" echo "## 🔑 KeepKey python-keepkey — Integration Tests" >> "$GITHUB_STEP_SUMMARY" echo "" >> "$GITHUB_STEP_SUMMARY" @@ -223,7 +254,7 @@ jobs: uses: mikepenz/action-junit-report@v4 if: always() with: - report_paths: tests/junit.xml + report_paths: python-keepkey/tests/junit.xml annotate_only: true require_tests: true fail_on_failure: true @@ -231,5 +262,5 @@ jobs: - name: Fail on test failure if: always() run: | - STATUS=$(cat tests/status 2>/dev/null || echo "1") + STATUS=$(cat python-keepkey/tests/status 2>/dev/null || echo "1") [ "$STATUS" = "0" ] || exit 1 From 39450f372bd4eb33196a36dd77e516fe0276618a Mon Sep 17 00:00:00 2001 From: highlander Date: Sat, 22 Aug 2026 00:16:31 -0500 Subject: [PATCH 4/6] fix(ci): do not recurse trezor-firmware's vendor tree `submodules: recursive` on the firmware checkout tries to clone trezor-firmware's micropython vendor tree, whose lib/lwip lives on git.savannah.gnu.org. That host serves DUMB HTTP and cannot satisfy the shallow clone actions/checkout asks for: fatal: dumb http transport does not support shallow capabilities fatal: Failed to recurse into submodule path 'deps/crypto/trezor-firmware' Nothing in the emulator build needs micropython. The firmware repo's own CI inits exactly the paths it needs, non-recursively, for this same reason -- so do that here. deps/python-keepkey is supplied by the overlay step instead, which is the point of the overlay: test THIS checkout, not whatever revision firmware pins. --- .github/workflows/ci.yml | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f2ec2d56..be0f91a1 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -95,9 +95,22 @@ jobs: with: repository: BitHighlander/keepkey-firmware ref: alpha - submodules: recursive path: keepkey-firmware + # NOT `submodules: recursive`. trezor-firmware carries a micropython + # vendor tree whose lib/lwip lives on git.savannah.gnu.org, which serves + # dumb HTTP and cannot do the shallow clone actions/checkout requests -- + # it fails the whole job. The firmware repo's own CI inits exactly these + # paths, non-recursively, for the same reason. + - name: Init the submodules the emulator build needs + working-directory: keepkey-firmware + run: | + git submodule update --init --depth 1 deps/crypto/trezor-firmware + git submodule update --init --depth 1 deps/device-protocol + git submodule update --init --depth 1 deps/googletest + git submodule update --init --depth 1 deps/qrenc/QR-Code-generator + git submodule update --init --depth 1 deps/sca-hardening/SecAESSTM32 + # Test THIS checkout of python-keepkey, not the one the firmware pins. - name: Overlay this python-keepkey onto the firmware tree run: | From e2941641af0dbc2ff55cd2071e162da59bbcf51a Mon Sep 17 00:00:00 2001 From: highlander Date: Sat, 22 Aug 2026 00:22:26 -0500 Subject: [PATCH 5/6] fix(tests): Failure_UnexpectedMessage lives in types_pb2, and run from the firmware tree Two failures the emulator-from-source build finally exposed. Both were always there; the job never got far enough to show them. requires_structured_eip712() referenced _proto.Failure_UnexpectedMessage. messages_pb2 has no such attribute -- the FailureType enum is generated into types_pb2 -- so the helper raised AttributeError and took all four structured EIP-712 tests down with it. My error, from the commit that added the helper. Worth recording alongside it: I claimed in that commit that requires_message() "only asks whether python-keepkey's own bindings define a message". That is wrong. It scans the modules AND then probes the device, skipping on Failure code 1. I stopped reading at the module scan. The helper is still the better gate -- it names the capability instead of a message and does not depend on serialising an empty probe -- but it is an improvement, not a fix for something broken. The storage-version-gate tests assert against lib/firmware/storage.c, which they locate by walking UP from the test directory. Run from a standalone python-keepkey checkout there is no firmware above them and five tests failed claiming the sources were missing. pytest now runs from the OVERLAID copy inside the firmware tree, where they resolve -- which is also the copy the emulator was built from, so the tests and the device now come from one tree rather than two. --- .github/workflows/ci.yml | 17 +++++++++++------ tests/common.py | 3 ++- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index be0f91a1..8ed986c8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -169,7 +169,7 @@ jobs: KK_TRANSPORT_DEBUG: "127.0.0.1:11045" KK_MIN_FW: "7.15.0" KK_UDP_TIMEOUT: "20" - working-directory: python-keepkey/tests + working-directory: keepkey-firmware/deps/python-keepkey/tests run: | python - <<'PY' import os, sys @@ -199,18 +199,23 @@ jobs: env: KK_TRANSPORT_MAIN: "127.0.0.1:11044" KK_TRANSPORT_DEBUG: "127.0.0.1:11045" - PYTHONPATH: "${{ github.workspace }}/python-keepkey/keepkeylib:${{ github.workspace }}/python-keepkey" + PYTHONPATH: "${{ github.workspace }}/keepkey-firmware/deps/python-keepkey" # A crashed emulator now raises instead of blocking in recv() forever. KK_UDP_TIMEOUT: "45" run: | - cd python-keepkey/tests + # From the OVERLAID copy, not the standalone checkout: the + # storage-version-gate tests assert against lib/firmware/storage.c, + # which they find by walking UP. Run them as a sibling of the + # firmware and they resolve; run them standalone and they fail + # claiming the sources are missing. + cd keepkey-firmware/deps/python-keepkey/tests pytest -v --junitxml=junit.xml 2>&1 | tee pytest-output.txt echo "${PIPESTATUS[0]}" > status - name: Test summary if: always() run: | - XML="python-keepkey/tests/junit.xml" + XML="keepkey-firmware/deps/python-keepkey/tests/junit.xml" echo "## 🔑 KeepKey python-keepkey — Integration Tests" >> "$GITHUB_STEP_SUMMARY" echo "" >> "$GITHUB_STEP_SUMMARY" @@ -267,7 +272,7 @@ jobs: uses: mikepenz/action-junit-report@v4 if: always() with: - report_paths: python-keepkey/tests/junit.xml + report_paths: keepkey-firmware/deps/python-keepkey/tests/junit.xml annotate_only: true require_tests: true fail_on_failure: true @@ -275,5 +280,5 @@ jobs: - name: Fail on test failure if: always() run: | - STATUS=$(cat python-keepkey/tests/status 2>/dev/null || echo "1") + STATUS=$(cat keepkey-firmware/deps/python-keepkey/tests/status 2>/dev/null || echo "1") [ "$STATUS" = "0" ] || exit 1 diff --git a/tests/common.py b/tests/common.py index 73dac785..f0b0e65f 100644 --- a/tests/common.py +++ b/tests/common.py @@ -156,6 +156,7 @@ def requires_structured_eip712(self): """ from keepkeylib import messages_ethereum_pb2 as _eth from keepkeylib import messages_pb2 as _proto + from keepkeylib import types_pb2 as _types probe = _eth.EthereumSignTypedData() for n in (0x8000002C, 0x8000003C, 0x80000000, 0, 0): @@ -166,7 +167,7 @@ def requires_structured_eip712(self): resp = self.client.call_raw(probe) if isinstance(resp, _proto.Failure): self.client.init_device() - if resp.code == _proto.Failure_UnexpectedMessage: + if resp.code == _types.Failure_UnexpectedMessage: self.skipTest( "Firmware does not implement structured EIP-712 " "(EthereumSignTypedData is not handled)") From 594e366ac6a6e13c570d992d846745c41afb2229 Mon Sep 17 00:00:00 2001 From: highlander Date: Sat, 22 Aug 2026 00:56:20 -0500 Subject: [PATCH 6/6] fix(tests): derive the storage version from the tree, not from one branch python-keepkey is ONE submodule shared by every firmware branch, and CI now builds the emulator from whichever branch is under test. So a test pinned to one branch's version reports a failure whose only cause is which branch you are on. Two did: test_active_flash_format_is_v20 assertEqual(20, version) test_burned_versions_are_dispatched... "case StorageVersion_18:" Both true on the passkeys branch, both FALSE on the 7.15 line, where STORAGE_VERSION is 17 and nothing is burned. A third, at the reboot test, was invisible only because CI has no emulator -- and pk-fix already carried a local patch flipping its 17 to 20, so the rot was being papered over branch by branch. A test that reads a source file has to assert properties of what it read. The ladder, the burned set, LAST_SHIPPED and which versions have readers are now all derived per tree. Burnedness cannot be inferred from storage.c alone: deleting the reader for a SHIPPED version would silently reclassify it as burned and the suite would bless the wipe. So two independent files are cross-checked -- storage_versions.inc DECLARES burned, storage.c DEMONSTRATES it (returns SUS_Invalid, no reader) -- and set equality between them is asserted. test_no_shipped_version_is_burned is the anchor: burned intersected with [1..LAST_SHIPPED] must be empty, so the declaration can never authorise wiping a format that reached hardware. One number is still written down, STORAGE_VERSION_LAST_SHIPPED_FLOOR = 17, and it is a FLOOR rather than an equality on purpose. 7.15 shipping V17 is finished history and cannot become false, so it survives 7.16 raising the constant. assertEqual(17, last_shipped) was the wrong shape: it goes false the day 7.16 ships, so it rots and gets "fixed" by whoever it inconveniences -- and lowering LAST_SHIPPED is the highest-severity item in docs/StorageVersionGate.md, with both operands of its static assert living in the same header where one commit reaches both. Verified on BOTH trees from one file: 10 passed / 5 skipped against the 7.15 line, 15 passed against the 7.16 line. Not vacuous: 10 mutations injected into throwaway copies, 9 fail loudly; the one that passes is a complete deliberate bump (header + ladder + case + reader), which is exactly what should pass. --- tests/test_storage_version_gate.py | 650 +++++++++++++++++++++++------ 1 file changed, 520 insertions(+), 130 deletions(-) diff --git a/tests/test_storage_version_gate.py b/tests/test_storage_version_gate.py index 866e1dbe..16820394 100644 --- a/tests/test_storage_version_gate.py +++ b/tests/test_storage_version_gate.py @@ -49,6 +49,54 @@ # TestStorageVersionGateSource needs no device at all: it reads the firmware # sources and asserts the gate's own invariants. Those tests run everywhere, # including CI, so this section is never completely dark. +# +# --------------------------------------------------------------------------- +# Why the source tests name no version number +# --------------------------------------------------------------------------- +# +# They used to. test_active_flash_format_is_v20 asserted STORAGE_VERSION == 20 +# and test_burned_versions_are_dispatched_to_the_wipe_path asserted the literal +# string "case StorageVersion_18:", because 7.16 writes V20 and burns 18/19. +# Both are true on the passkeys branch and both are FALSE on the 7.15 line, +# where STORAGE_VERSION is 17 and nothing is burned. python-keepkey is one +# submodule shared by every firmware branch, and CI now builds the emulator +# from whichever branch is under test, so a test pinned to one branch's version +# reports a failure whose only cause is which branch you are on. +# +# A test that reads a source file has to assert properties of what it read. +# What follows is derived, per tree: +# +# STORAGE_VERSION, STORAGE_VERSION_LAST_SHIPPED, include/.../storage.h +# STORAGE_VERSION_BTC_ONLY_BASE +# the version ladder lib/firmware/storage_versions.inc +# which versions are BURNED lib/firmware/storage_versions.inc +# which versions have a reader / hit the wipe path lib/firmware/storage.c +# +# The only version number still written down is +# STORAGE_VERSION_LAST_SHIPPED_FLOOR, and it is a FLOOR, not an equality -- see +# its comment for why that distinction is the whole argument. (The V16 numbers +# in the emulator section are a different thing: they describe the format 7.14.x +# shipped, which is finished history and cannot change. The flash offsets are +# unchanged by the V20 bump -- V20 keeps V17's layout and puts passkey state in +# its reserved area at +501 -- so the migration test reads the same way on both +# lines.) +# +# Decoupling is not the same as weakening. The property docs/StorageVersionGate +# .md exists to protect -- "a bump is a deliberate release act, never an +# accident" -- is enforced harder than before, because it no longer rests on +# somebody also editing a constant in this file. A bare `#define +# STORAGE_VERSION 18` now has to survive: +# +# * the ladder must be contiguous 1..N and END at STORAGE_VERSION, so the +# bump forces an append to storage_versions.inc; +# * every ladder version must be dispatched in storage_fromFlash, so the bump +# forces a case label; +# * the version this firmware WRITES must have a reader, so the bump forces a +# reader behind that label. +# +# Three files have to move together, and every one of them is a file that has +# to move anyway for the firmware to be correct. The old constant was the only +# artifact in the set that did not. from __future__ import print_function @@ -102,9 +150,30 @@ FLAG_AUTHDATA_INITIALIZED = 1 << 18 FLAG_AUTHDATA_ENCRYPTED = 1 << 19 -# include/keepkey/firmware/storage.h +# include/keepkey/firmware/storage.h. Cross-checked against the header by +# test_version_never_drops_below_a_shipped_release -- the emulator tests below +# stamp wallets into this band by hand, so a drift between the two would make +# them exercise a band the firmware does not use. STORAGE_VERSION_BTC_ONLY_BASE = 10000 +# The lowest value STORAGE_VERSION_LAST_SHIPPED may ever hold. 7.15 shipped +# storage V17; that is a fact about the past and cannot become false, so this +# is a RATCHET and not a version pin. Raise it when a later release actually +# ships (the same commit that raises the constant in storage.h); there is no +# branch on which it needs lowering, and lowering it is the edit this exists to +# stop. +# +# The distinction matters. `assertEqual(17, last_shipped)` is wrong the day +# 7.16 ships and wrong on any branch that has already bumped it, so it rots and +# gets "fixed" by whoever the failure inconveniences. `>= 17` is wrong only if +# somebody deletes history. It still catches the edit docs/StorageVersionGate.md +# calls the single highest-severity review item in the file: the static assert +# is STORAGE_VERSION >= STORAGE_VERSION_LAST_SHIPPED, so the way to make a +# LOWERED storage version compile is to lower LAST_SHIPPED to match it, and +# both numbers live in the same header where one commit reaches both. An +# independent witness is the only thing that sees it. +STORAGE_VERSION_LAST_SHIPPED_FLOOR = 17 + MNEMONIC_ALL = " ".join(["all"] * 12) LABEL = "storagegate" PIN = "1234" @@ -116,7 +185,20 @@ # --------------------------------------------------------------------------- def _repo_root(): - """Directory of the firmware checkout this python-keepkey lives under.""" + """Directory of the firmware checkout this python-keepkey lives under. + + KK_FIRMWARE_ROOT wins, so the gate can be pointed at a tree this clone is + not nested inside. That is not a convenience: these tests now derive every + version number from the tree, and the only way to show they hold on BOTH + release lines is to run one checkout of them against two firmware trees. + Unset -- which is how CI runs, from deps/python-keepkey -- the walk up is + unchanged. + """ + env = os.environ.get("KK_FIRMWARE_ROOT") + if env: + assert os.path.isfile(os.path.join(env, "lib", "firmware", "storage.c")), ( + "KK_FIRMWARE_ROOT=%s has no lib/firmware/storage.c" % env) + return env d = _HERE for _ in range(8): if os.path.isfile(os.path.join(d, "lib", "firmware", "storage.c")): @@ -152,6 +234,156 @@ def _define(text, name): return int(m.group(1)) +def _strip_c_comments(text): + """Comments are prose and must never be mistaken for code. + + Both files this module parses argue their case in long comments that name + the very identifiers being searched for -- the burned arm in storage.c says + "there is deliberately NO reader" a few words from where a reader would be + written. Classification runs on the stripped text so a rewording can never + change a verdict. + """ + text = re.sub(r"/\*.*?\*/", " ", text, flags=re.S) + return re.sub(r"//[^\n]*", " ", text) + + +# -- lib/firmware/storage_versions.inc -------------------------------------- + +_LADDER_ENTRY = re.compile( + r"STORAGE_VERSION_(?:ENTRY|LAST)\s*\(\s*(\d+)\s*\)") +_LADDER_LAST = re.compile(r"STORAGE_VERSION_LAST\s*\(\s*(\d+)\s*\)") +_ENTRY_LINE = re.compile(r"^\s*STORAGE_VERSION_ENTRY\s*\(\s*(\d+)\s*\)\s*$") +_BURNED_WORD = re.compile(r"\bBURNED\b") + + +def _ladder(inc): + """Every version in storage_versions.inc, in file order. + + The x-macro definitions at the top of the file take a parameter named X, + not a digit, so they do not match. + """ + return [int(m) for m in _LADDER_ENTRY.findall(_strip_c_comments(inc))] + + +def _ladder_last(inc): + """The single STORAGE_VERSION_LAST(N) entry: the version this build writes.""" + last = _LADDER_LAST.findall(_strip_c_comments(inc)) + assert len(last) == 1, ( + "storage_versions.inc must have exactly one STORAGE_VERSION_LAST entry, " + "found %s" % last) + return int(last[0]) + + +def _burned_declared(inc): + """Versions storage_versions.inc annotates as BURNED. + + THE DECLARATION SITE. A burned version is one that a pre-release build + wrote with a layout that was later abandoned, so devices carrying it exist + and no reader may ever be written for it -- parsing such a blob as the + current format is worse than refusing it, because nothing announces the + misparse. That is a fact about history, not about code, so it cannot be + inferred from the code: it has to be stated somewhere and read from there. + + The convention is a comment containing the word BURNED, immediately above + the entries it applies to: + + STORAGE_VERSION_ENTRY(17) + /* 18 and 19 are BURNED. */ + STORAGE_VERSION_ENTRY(18) + STORAGE_VERSION_ENTRY(19) + STORAGE_VERSION_LAST(20) + + The run ends at the first line that is not a bare STORAGE_VERSION_ENTRY -- + a blank line, another comment, or the STORAGE_VERSION_LAST line, which by + definition is the version being written and so can never be burned. + + Numbers inside the comment text are deliberately NOT scraped: that prose + mentions the commit that reverted the format and the version it reverted + TO, and reading V17 out of it would declare a shipped version burned. + Position is the annotation; the words are for humans. + + An unannotated version that turns out to be dispatched to the wipe path is + a mismatch, not a silent pass -- see + test_burned_versions_agree_between_the_ladder_and_the_dispatch. + """ + burned = set() + lines = inc.splitlines() + i = 0 + while i < len(lines): + if "/*" not in lines[i]: + i += 1 + continue + block = [] + while i < len(lines): + block.append(lines[i]) + if "*/" in lines[i]: + break + i += 1 + i += 1 + if not _BURNED_WORD.search("\n".join(block)): + continue + while i < len(lines): + m = _ENTRY_LINE.match(lines[i]) + if not m: + break + burned.add(int(m.group(1))) + i += 1 + return burned + + +# -- lib/firmware/storage.c -------------------------------------------------- + +_CASE_LABEL = re.compile(r"case\s+StorageVersion_(\w+)\s*:") +_READER_CALL = re.compile(r"\bstorage_read\w*\s*\(") +_WIPE_RETURN = re.compile(r"return\s+SUS_Invalid\b") + + +def _from_flash_arms(c): + """Map every StorageVersion_X label in storage_fromFlash to its arm text. + + Consecutive labels share one arm: `case 2: case 3: ... case 10:` is a + single body reached by nine versions, and each of them must be credited + with what that body does. So labels accumulate until one is followed by + something other than whitespace and comments, and the whole group is + assigned that text. + + Keys are the label suffixes as written -- "17", "BTC_ONLY", "NONE" -- so + the non-numeric arms stay visible to the tests that care about them. + """ + i = c.index("StorageUpdateStatus storage_fromFlash") + body = c[i:c.index("\n}", i)] + assert "case StorageVersion_NONE" in body, ( + "storage_fromFlash body was cut short before the end of its switch; " + "the parse below would under-report every arm") + + labels = list(_CASE_LABEL.finditer(body)) + assert labels, "no case StorageVersion_* labels in storage_fromFlash" + + arms = {} + group = [] + for idx, m in enumerate(labels): + group.append(m.group(1)) + end = labels[idx + 1].start() if idx + 1 < len(labels) else len(body) + own = body[m.end():end] + if _strip_c_comments(own).strip(): + for name in group: + arms[name] = own + group = [] + for name in group: # labels trailing the last statement: no body at all + arms[name] = "" + return arms + + +def _reads(arm): + """Does this arm call a storage_readVxx reader?""" + return bool(_READER_CALL.search(_strip_c_comments(arm))) + + +def _wipes(arm): + """Does this arm return SUS_Invalid -- the reset-and-commit path?""" + return bool(_WIPE_RETURN.search(_strip_c_comments(arm))) + + # --------------------------------------------------------------------------- # Emulator process management # --------------------------------------------------------------------------- @@ -389,7 +621,14 @@ def _capture(client): class TestStorageVersionGateSource(unittest.TestCase): """No device needed. These are the checks that survive a CI runner which - cannot restart an emulator, so the section is never entirely unmeasured.""" + cannot restart an emulator, so the section is never entirely unmeasured. + + Every number these tests compare against is read out of the tree they are + run in, so one copy of this file states the same invariants on the 7.15 + line (STORAGE_VERSION 17, nothing burned) and on 7.16 (20, with 18 and 19 + burned). See the note at the top of the module for why that is a + strengthening rather than a relaxation. + """ def setUp(self): self.h = _read_source("include/keepkey/firmware/storage.h") @@ -398,107 +637,42 @@ def setUp(self): self.version = _define(self.h, "STORAGE_VERSION") self.last_shipped = _define(self.h, "STORAGE_VERSION_LAST_SHIPPED") - def test_active_flash_format_is_v20(self): - """7.16 writes V20. The bump is argued here, which is the point of the - test: it asserts a LITERAL so that raising a header constant cannot - quietly satisfy it. - - The compile-time assert in storage.c compares STORAGE_VERSION against - STORAGE_VERSION_LAST_SHIPPED -- two numbers in the same header, both - editable in one commit -- so raising LAST_SHIPPED to make a build - compile is the edit docs/StorageVersionGate.md calls the highest - severity review item in the file. An independent witness is the only - thing that catches it. - - WHY 20 AND NOT 18. 18 was the clear-sign identity block and 19 the - PIN-KDF migration. Both were ACTIVE, not merely drafted: e109404ee made - 19 live and 6bebde7b2 reverted the format to V17 for 7.15. Any device - that ran an alpha build in that window carries a blob stamped 18 or 19 - whose layout has nothing to do with passkeys, and reading one as CTAP2 - state would misparse it rather than refuse it. 20 is unburned. - - THE READER CHAIN. V17 blobs are read by storage_readV17 and restamped - to STORAGE_VERSION; V20 blobs by storage_readV20. There is deliberately - NO reader for 18 or 19: they remain in the ladder because the enum is - positional and removing an entry renumbers everything after it, but a - blob stamped with either falls through to the default and the device - wipes. That is the documented behaviour for an unrecognised format and - is strictly better than misparsing one. - - ANTI-ROLLBACK. Once a device writes V20, installing 7.15 -- which knows - only up to V17 -- maps the blob to StorageVersion_NONE and storage_init - resets it. The device wipes. That is normal downgrade behaviour and is - stated here so it is a known consequence rather than a field report. - A signed UPGRADE never wipes; only going backwards does. - - RELEASE NOTE. "7.16 changes the on-device storage format to hold - passkey credentials. Upgrading preserves your wallet. Downgrading to - 7.15 or earlier will ERASE it -- back up your recovery phrase before - downgrading." - """ - self.assertEqual( - 20, self.version, - "STORAGE_VERSION is %d, not the V20 format 7.16 introduces. A bump " - "is a deliberate release act (docs/StorageVersionGate.md): confirm " - "the reader chain, the anti-rollback story, and the release notes, " - "then update this test." % self.version) - # LAST_SHIPPED stays at 17 until 7.16 actually SHIPS in a signed - # release. It is the high-water mark of what is IN THE FIELD, not of - # what is in the tree -- and storage.h says two lines above the - # constant that raising it to make a build compile "is the exact edit - # that turns every upgrade in the field into a silent wipe". The - # compile-time assert only needs STORAGE_VERSION >= LAST_SHIPPED, and - # 20 >= 17 holds, so nothing requires the raise. - self.assertEqual( - 17, self.last_shipped, - "STORAGE_VERSION_LAST_SHIPPED is %d. It tracks the last SIGNED " - "release (7.15 = V17) and moves in the release commit that tags " - "7.16, not when a format lands in the tree." % self.last_shipped) + self.ladder = _ladder(self.inc) + self.burned = _burned_declared(self.inc) + self.arms = _from_flash_arms(self.c) - def test_burned_versions_are_dispatched_to_the_wipe_path(self): - """18 and 19 must never be PARSED by 7.16. + # -- helpers ------------------------------------------------------------ - They were real formats in alpha builds before the 7.15 revert, so - devices carrying them exist. A reader for either would parse a - clear-sign identity block or a PIN-KDF blob as passkey state. + def _arm(self, version): + arm = self.arms.get(str(version)) + self.assertIsNotNone( + arm, + "storage_fromFlash has no `case StorageVersion_%d:` -- see " + "test_every_ladder_version_is_dispatched" % version) + return arm - This used to assert the absence of a `case StorageVersion_18:` label, - on the theory that falling to the default is what sends them to the - wipe path. That was wrong twice over: storage_fromFlash has NO default - case -- deliberately, so -Werror=switch names any version we forget -- - so an unlisted version does not fall anywhere, it fails the ARM build. + # -- the ladder --------------------------------------------------------- - So the labels must exist. What must NOT exist is a reader behind them. - Assert the real property: 18 and 19 are dispatched, and what they - dispatch to is SUS_Invalid rather than any storage_readVxx call. - """ - for burned in (18, 19): - label = "case StorageVersion_%d:" % burned - self.assertIn( - label, self.c, - "%s must be listed; storage_fromFlash has no default case, so " - "an unlisted version breaks the -Werror=switch build" % label) - - # The two labels must sit together and return SUS_Invalid before any - # other case begins. Slice from the first burned label to the next - # `case ` that is not one of the burned ones. - i = self.c.index("case StorageVersion_18:") - rest = self.c[i:] - j = len(rest) - for m in re.finditer(r"\n\s*case StorageVersion_(\w+):", rest): - if m.group(1) not in ("18", "19"): - j = m.start() - break - arm = rest[:j] + def test_version_ladder_is_contiguous_and_ends_at_storage_version(self): + """storage_versions.inc may only ever be APPENDED to. - self.assertIn( - "SUS_Invalid", arm, - "the burned versions must return SUS_Invalid (the wipe path); " - "arm was:\n%s" % arm) - self.assertNotIn( - "storage_read", arm, - "a reader behind a burned version would misparse blobs written by " - "pre-revert alpha builds; arm was:\n%s" % arm) + The enum is emitted in .inc order after StorageVersion_NONE = 0, so a + contiguous 1..N list is what makes StorageVersion_N == N. Deleting or + renumbering an entry silently drops a version from version_from_int() + and wipes every device carrying it. + + Ending AT StorageVersion is the half that makes a bare header bump + loud: raise STORAGE_VERSION without appending here and the two numbers + disagree. + """ + self.assertTrue(self.ladder, "no version entries parsed from the ladder") + self.assertEqual(list(range(1, len(self.ladder) + 1)), self.ladder, + "storage_versions.inc is not contiguous from 1") + self.assertEqual( + self.version, _ladder_last(self.inc), + "STORAGE_VERSION is %d but the ladder ends at %d. A version this " + "firmware writes and cannot enumerate is not recognised on the next " + "boot -- it wipes itself." % (self.version, _ladder_last(self.inc))) def test_version_never_drops_below_a_shipped_release(self): """Lowering STORAGE_VERSION wipes every device upgrading FROM a shipped @@ -507,41 +681,247 @@ def test_version_never_drops_below_a_shipped_release(self): stay under the bitcoin-only band, or a multi-chain wallet would be stamped into the band that multi-chain firmware refuses to load.""" self.assertGreaterEqual(self.version, self.last_shipped) - self.assertLess(self.version, STORAGE_VERSION_BTC_ONLY_BASE) - - def test_version_ladder_is_contiguous_and_ends_at_storage_version(self): - """storage_versions.inc may only ever be APPENDED to. - - The enum is emitted in .inc order after StorageVersion_NONE = 0, so a - contiguous 1..N list is what makes StorageVersion_N == N. Deleting or - renumbering an entry silently drops a version from version_from_int() - and wipes every device carrying it. + band = _define(self.h, "STORAGE_VERSION_BTC_ONLY_BASE") + self.assertEqual( + STORAGE_VERSION_BTC_ONLY_BASE, band, + "the header moved the bitcoin-only band to %d; the emulator tests " + "in this file stamp wallets into %d by hand and would be measuring " + "a band the firmware no longer uses" + % (band, STORAGE_VERSION_BTC_ONLY_BASE)) + self.assertLess(self.version, band) + + def test_last_shipped_never_moves_backwards(self): + """STORAGE_VERSION_LAST_SHIPPED is a high-water mark of the FIELD. + + It records the newest format any signed release ever wrote, so it can + only rise, and only in the commit that ships. The compile-time assert + in storage.c is STORAGE_VERSION >= STORAGE_VERSION_LAST_SHIPPED, and + both operands live in the same header -- so the way to make a LOWERED + storage version build is to lower this to match, which is exactly the + edit that turns every upgrade in the field into a silent wipe. + docs/StorageVersionGate.md calls that the highest-severity review item + in the file. + + A floor asserted from outside the header is the independent witness. + It is not a version pin: it stays true when 7.16 raises the constant to + 20, and it is only ever raised, never corrected. """ - entries = [int(m) for m in re.findall( - r"STORAGE_VERSION_(?:ENTRY|LAST)\s*\(\s*(\d+)\s*\)", self.inc)] - self.assertTrue(entries, "no version entries parsed from the ladder") - self.assertEqual(list(range(1, len(entries) + 1)), entries, - "storage_versions.inc is not contiguous from 1") - last = re.findall(r"STORAGE_VERSION_LAST\s*\(\s*(\d+)\s*\)", self.inc) - self.assertEqual([str(self.version)], last) + self.assertGreaterEqual( + self.last_shipped, STORAGE_VERSION_LAST_SHIPPED_FLOOR, + "STORAGE_VERSION_LAST_SHIPPED is %d, below the %d that 7.15 shipped. " + "Either a signed release is being un-remembered to make a lowered " + "STORAGE_VERSION compile, or the ratchet in this file is wrong -- " + "and only one of those two has ever happened." + % (self.last_shipped, STORAGE_VERSION_LAST_SHIPPED_FLOOR)) - def test_every_ladder_version_has_a_reader(self): + # -- the dispatch ------------------------------------------------------- + + def test_every_ladder_version_is_dispatched(self): """Every version in the ladder needs a case in storage_fromFlash(). This is the failure the static asserts do NOT cover. They pin the enum - to its own numbering; they say nothing about the switch. Drop a case - and control falls out of the switch to `return SUS_Invalid` -- which - storage_init() answers with storage_reset(). Every device carrying that - version is wiped on upgrade, and the build stays green. + to its own numbering; they say nothing about the switch. + + The switch has no default case, deliberately, so that -Werror=switch + names any version we forget -- which means on ARM this is also a build + failure. It is asserted anyway because the emulator and the unit tests + are built by other toolchains and other flag sets, and because the + message here says which device gets wiped, where the compiler says + which enumerator is unhandled. + """ + missing = [v for v in self.ladder if str(v) not in self.arms] + self.assertEqual( + [], missing, + "storage_fromFlash has no case for version(s) %s -- a device " + "carrying one is wiped at boot" % missing) + + def test_an_unrecognised_version_reaches_the_wipe_path(self): + """version_from_int() maps anything off the ladder to + StorageVersion_NONE, and that arm must return SUS_Invalid. + + This is the mechanism the downgrade half of the policy rests on: a + device that has run newer firmware carries a stamp older firmware + cannot read, and it must reset rather than load a blob it will + misparse. The emulator test test_unrecognised_version_wipes_on_boot + proves the behaviour end to end; this proves the arm still exists on a + runner with no emulator. """ - body = self.c.split("StorageUpdateStatus storage_fromFlash", 1) - self.assertEqual(2, len(body), "storage_fromFlash not found") - cases = set(int(m) for m in re.findall( - r"case\s+StorageVersion_(\d+)\s*:", body[1])) - missing = sorted(set(range(1, self.version + 1)) - cases) - self.assertEqual([], missing, - "storage_fromFlash has no case for version(s) %s -- a " - "device carrying one is wiped at boot" % missing) + arm = self.arms.get("NONE") + self.assertIsNotNone(arm, "storage_fromFlash has no StorageVersion_NONE case") + self.assertTrue( + _wipes(arm), + "StorageVersion_NONE no longer returns SUS_Invalid. An unknown " + "storage version would be accepted, and an attacker could roll back " + "to an older signed image with a known extraction bug and keep the " + "seed. Arm was:\n%s" % arm) + self.assertFalse( + _reads(arm), + "a reader behind StorageVersion_NONE parses a blob whose format is " + "by definition unknown. Arm was:\n%s" % arm) + + def test_every_dispatched_version_either_reads_or_refuses(self): + """An arm reads a blob or it refuses one. Never both, never neither. + + Neither means control reached a case that falls out of the switch -- + storage_fromFlash ends in `return SUS_Invalid`, so the device wipes, + and nothing in the source says that was meant. + + Both means the classification below cannot say what the arm is for, and + an arm that reads before refusing has already parsed the blob. If a + real reader ever needs an error return, this assertion is where that + design gets argued rather than assumed -- which is the point of the + gate. + """ + for version in self.ladder: + arm = self._arm(version) + reads, wipes = _reads(arm), _wipes(arm) + self.assertNotEqual( + reads, wipes, + "version %d %s. Arm was:\n%s" + % (version, + "both reads a blob and returns SUS_Invalid" if reads else + "neither reads a blob nor returns SUS_Invalid, so it falls " + "out of the switch and wipes without saying so", + arm)) + + def test_every_shipped_version_has_a_reader(self): + """THE upgrade-never-wipes property, for every device in the field. + + An upgrading device arrives carrying the format written by the release + it is leaving. STORAGE_VERSION_LAST_SHIPPED is the newest of those, so + 1..LAST_SHIPPED is the set of formats that exist on real hardware, and + every one of them must be read rather than refused. Lose a reader here + and every wallet carrying that version is erased at boot with no + prompt, while the build stays green. + + This is the test that carries the section on a release line with + nothing burned, and it is the reason a burned version can never be one + that shipped -- see test_no_shipped_version_is_burned. + """ + for version in range(1, self.last_shipped + 1): + arm = self._arm(version) + self.assertTrue( + _reads(arm), + "version %d has SHIPPED (STORAGE_VERSION_LAST_SHIPPED is %d) " + "but storage_fromFlash does not read it. Every device carrying " + "it is wiped on upgrade. Arm was:\n%s" + % (version, self.last_shipped, arm)) + self.assertFalse( + _wipes(arm), + "version %d has SHIPPED but its arm returns SUS_Invalid, which " + "is storage_reset() + storage_commit() at boot. Arm was:\n%s" + % (version, arm)) + + def test_the_version_this_firmware_writes_can_be_read_back(self): + """A device commits STORAGE_VERSION and reboots into the same firmware. + + If the arm for the version it just wrote does not read, storage_init() + resets on the very next boot -- the wallet does not survive a power + cycle of the build that created it. The emulator test + test_reboot_preserves_the_wallet proves this on a running device; here + it also makes a header bump carry a reader with it, because there is no + version so new that the firmware writing it may refuse to read it. + """ + arm = self._arm(self.version) + self.assertTrue( + _reads(arm), + "STORAGE_VERSION is %d and storage_fromFlash does not read version " + "%d. This firmware cannot load the blob it writes. Arm was:\n%s" + % (self.version, self.version, arm)) + self.assertNotIn( + self.version, self.burned, + "storage_versions.inc declares version %d BURNED and storage.h " + "writes it. A burned version is one no reader may exist for." + % self.version) + + # -- burned versions ---------------------------------------------------- + + def test_burned_versions_agree_between_the_ladder_and_the_dispatch(self): + """Two files, one answer. + + storage_versions.inc DECLARES which versions are burned; storage.c + DEMONSTRATES it by dispatching them to SUS_Invalid with no reader. + Neither file can be the only witness: + + * derived from storage.c alone, deleting the reader for a shipped + version would silently reclassify it as burned and the suite would + approve of it; + * declared in the .inc alone, a reader wired in behind a burned label + would parse a blob written by a build whose layout was abandoned, + and the declaration would sit there saying otherwise. + + Requiring the two to match catches both, and matching costs an edit in + two files -- which is what "a deliberate act" means here. On a line + with no burned versions both sides are empty and this test says so. + """ + dispatched = set( + v for v in self.ladder + if not _reads(self._arm(v)) and _wipes(self._arm(v))) + self.assertEqual( + sorted(self.burned), sorted(dispatched), + "storage_versions.inc declares %s BURNED; storage_fromFlash sends " + "%s to the wipe path. Whichever is right, the other is a lie about " + "what happens to a device carrying one of these blobs." + % (sorted(self.burned) or "nothing", sorted(dispatched) or "nothing")) + + def test_burned_versions_are_dispatched_to_the_wipe_path(self): + """A burned version must be listed, must refuse, and must have no reader. + + Burned means: a pre-release build wrote this format, devices carrying + it exist, and the number was then reused for something else -- so the + blob's bytes mean one thing and the stamp claims another. Refusing it + wipes, which is the documented behaviour for a format we do not + recognise and strictly better than misparsing one. + + LISTED, not defaulted. storage_fromFlash has no default case on + purpose, so an unlisted version fails the -Werror=switch build rather + than falling anywhere. + """ + if not self.burned: + self.skipTest( + "no version is declared BURNED in storage_versions.inc on this " + "line -- STORAGE_VERSION is %d and the whole ladder has " + "readers. Nothing to measure here; the upgrade path is carried " + "by test_every_shipped_version_has_a_reader." % self.version) + for version in sorted(self.burned): + self.assertIn( + version, self.ladder, + "version %d is declared BURNED but is not in the ladder. The " + "entry has to stay: the enum is positional, so removing one " + "renumbers every version after it." % version) + arm = self._arm(version) + self.assertTrue( + _wipes(arm), + "burned version %d does not return SUS_Invalid. Arm was:\n%s" + % (version, arm)) + self.assertFalse( + _reads(arm), + "a reader behind burned version %d would parse a blob written " + "by a build whose layout has nothing to do with the current " + "format, and would do it silently. Arm was:\n%s" % (version, arm)) + + def test_no_shipped_version_is_burned(self): + """Burning a version that SHIPPED wipes every device carrying it. + + This is what keeps the burned set from being a loophole. Burnedness is + declared, and a declaration can be written for any number -- so the one + thing it may never cover is a format that reached real hardware. + STORAGE_VERSION_LAST_SHIPPED is where the firmware records how far that + reaches, and STORAGE_VERSION_LAST_SHIPPED_FLOOR keeps that record from + being quietly walked back. + + A version may only be burned if it lives strictly above the last + shipped release: written by an alpha, never by anything signed. + """ + shipped_and_burned = sorted( + v for v in self.burned if v <= self.last_shipped) + self.assertEqual( + [], shipped_and_burned, + "version(s) %s are declared BURNED but are at or below " + "STORAGE_VERSION_LAST_SHIPPED (%d), so signed firmware wrote them " + "and devices in the field carry them. Burning one erases those " + "wallets at boot." + % (shipped_and_burned, self.last_shipped)) # --------------------------------------------------------------------------- @@ -629,8 +1009,18 @@ def test_reboot_preserves_the_wallet(self): fingerprint and the ciphertext all round-tripped together. """ addr, off = self._create_wallet() - self.assertEqual(17, self.emu.read_u32(off, OFF_VERSION), - "this build committed a storage version other than 17") + # The stamp in flash must be the version the header declares. This is + # not a tautology and it is not a version pin either: the emulator was + # built from _ROOT, so the two sides are the WRITER and the DECLARATION, + # and a writer that stamps anything else produces blobs the next boot + # does not recognise. Reading 17 or 20 out of this file instead would + # only record which branch the author was standing on. + declared = _define(_read_source("include/keepkey/firmware/storage.h"), + "STORAGE_VERSION") + self.assertEqual( + declared, self.emu.read_u32(off, OFF_VERSION), + "the firmware committed a storage version other than the %d its " + "header declares" % declared) before = self.emu.image() self.emu.boot()