feat(firmware): add CTAP2 passkey support for 7.16 - #504
Merged
Conversation
BitHighlander
force-pushed
the
feat/passkeys-7.16
branch
2 times, most recently
from
August 21, 2026 23:54
91c19af to
9ac79b4
Compare
BitHighlander
force-pushed
the
feat/passkeys-7.16
branch
from
August 22, 2026 00:07
9ac79b4 to
95ad076
Compare
Owner
Author
Holding this one — U5 fired on something real
Two separate things need answering. 1. Version number 18 is being REUSED for a different layoutV18 already existed once. It carried the clear-sign identity block, and 7.15 reverted the flash format to V17 in This PR now defines V18 as something else entirely: // V18 stores passkey state inside V17's 996-byte reserved plaintext area.
#define V18_STORAGE_LEN (1501 + V17_ENCSEC_SIZE) // 2525So a blob stamped The cheap fix is to take 19 or 20 instead. A version number costs nothing; a collision costs a misparsed storage blob on somebody's device. 2. The bump needs its release argument written downPer the gate doc, a bump is a deliberate release act. Before U5 gets updated it wants, in the PR body or the SRS:
Not blocking on my account — I have no objection to passkeys needing storage, and putting the state in V17's reserved plaintext area is a reasonable shape. But I am not merging a storage-format bump to alpha on a red gate whose whole purpose is to stop exactly this from going in unargued. Happy to make the version-number change myself if you want it moved to 19. |
U5 (test_active_flash_format_is_v17) went red on this branch, and it was right to. The branch bumped the flash format to 18 -- a number that already means something else. 18 was the clear-sign identity block and 19 was the PIN-KDF migration. Both were ACTIVE, not merely drafted: git log -S"STORAGE_VERSION_LAST(19)" shows e109404 making 19 live and 6bebde7 reverting 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. Reusing 18 would make this firmware read those blobs as CTAP2 state. Not refuse them, not wipe them -- parse them, as something they are not. That is the failure docs/StorageVersionGate.md calls the highest-severity item in the file, and it is the reason U5 asserts a literal instead of trusting the compile-time check: STORAGE_VERSION and STORAGE_VERSION_LAST_SHIPPED are two numbers in the same header, and raising one to make a build compile is exactly the edit that must not pass silently. So: the passkey format takes 20. 18 and 19 stay in the ladder -- the enum is positional, so removing an entry renumbers everything after it -- but there is deliberately NO reader for either. A blob stamped 18 or 19 falls through to the default and the device wipes, which is the documented behaviour for an unrecognised format and is strictly better than misparsing one. The passkey format itself is unchanged: same layout, same 2525 bytes in V17's reserved plaintext area, same reader logic. Only the version number it registers under, and the V18_* -> V20_* symbol names, have moved. U5 still needs updating with the release argument for the bump (reader chain, anti-rollback story, release note) -- that is a deliberate release act and belongs to whoever cuts 7.16, not to this fix.
fix(storage): take V20, because 18 and 19 are burned
Brings in the U5 update that argues the bump -- why 20 and not 18, the reader chain, the anti-rollback wipe, and the drafted release note -- plus U5b, which asserts formats 18 and 19 have no dispatch case so a burned blob keeps falling to the wipe path.
The ladder moved to LAST(20) but STORAGE_VERSION in the header stayed 18, so
the static_assert in storage_versions.inc fired and every build failed:
storage_versions.inc:34:1: error: static_assert failed
"need to update storage_versions.inc"
Two numbers describe the same fact and both have to move. That is precisely
why U5 asserts a literal from OUTSIDE the header -- a gate that only compares
these two against each other cannot notice when a release moves them together
for the wrong reason.
STORAGE_VERSION_LAST_SHIPPED deliberately stays 17. It is the high-water mark
of what is in the FIELD, and 7.15 shipped V17; it moves in the release commit
that tags 7.16, not here. The assert only needs STORAGE_VERSION >= LAST_SHIPPED
and 20 >= 17 holds.
The V20 change renamed the implementation but not its callers, so unittests/firmware/storage.cpp still referenced storage_readV18 / storage_writeV18 / V18_STORAGE_LEN and every emulator build failed with "use of undeclared identifier". My omission, and a reminder that a rename is only done when the compiler says so on every target -- the ARM build passed because it does not compile the unit tests, so the first thing that noticed was the bitcoin-only emulator leg. Also updates the test NAMES and prose, which described the passkey format as V18: PasskeyMetadataV18RoundTrip -> ...V20..., and V18IgnoresRetiredClearsignIdentityBlock -> V20... . That second one is worth reading twice: it asserts the passkey format ignores a trailing clear-sign identity block left by an older layout, which is exactly the collision that made 18 unusable in the first place.
storage_fromFlash has no default case on purpose -- that absence is what makes -Werror=switch name any storage version we forget to handle. The V20 rename left 18 and 19 unlisted with a comment claiming they "fall through to the default", which was wrong twice over: there is no default, and the comment is not something the compiler reads. Both ARM legs failed to build. Listed explicitly, returning SUS_Invalid, which is the wipe path a blob in an unrecognised format is supposed to take. No behaviour change from what the comment intended -- only now the compiler agrees. Both ARM variants cross-compiled locally in kktech/firmware:v15 before pushing.
Picks up BitHighlander/python-keepkey#52, which fixes the two gates that failed this branch: - the structured EIP-712 suite gated on requires_message(), which only asks whether python-keepkey's OWN bindings define a message. That is a property of the pinned submodule, not the firmware, so it passed here despite this branch having no eip712_stream.c at all, and four tests failed as though the feature were broken rather than absent. Now probes the device. - test_burned_versions_have_no_reader asserted the ABSENCE of a `case StorageVersion_18:` label. storage_fromFlash has no default case on purpose, so -Werror=switch requires that label to exist -- the test and the compiler gate were in direct contradiction. Now asserts the property that matters: the labels dispatch to SUS_Invalid with no reader behind them.
Merging alpha into this branch broke the ARM link:
ld: Insufficient runtime SRAM: require 16 KiB stack/heap reserve
between _ebss and _stack
Passkeys and structured EIP-712 each fit alone; together they did not.
Measured rather than guessed: the reserve was 15,956 B against a 16,384 B
gate, so 428 B short.
Passkeys cost 1,248 B of static allocation. A PasskeyCredential is 212 B
and PasskeyStorage holds four, and the whole struct lives in RAM TWICE --
the live copy in shadow_config and the sector staging copy in flash_temp
-- so the 888 B struct lands as ~1,248 B of .bss.
Dropping MAX_DECODE_SIZE 12 KB -> 11 KB returns exactly 1,024 B, putting
the reserve at 16,980 B with a 596 B margin. Both ARM variants link with
the real gate in place, and no message static-assert fired, so nothing
outgrew the smaller buffer.
Chosen over cutting PASSKEY_MAX_DISCOVERABLE_CREDENTIALS from 4 to 2,
which would have reclaimed a comparable ~848 B. That is a permanent,
user-visible limit -- 4 discoverable credentials is already low against a
YubiKey's 25+ -- whereas the decode buffer is protocol headroom nobody
sees, and fsm.c static-asserts every registered message against it, so
outgrowing it fails the build rather than the field.
The structural headroom, for later and not tonight: canvas_buffer is
16,384 B, one byte per pixel for a MONO display. At 1bpp it needs 2,048.
That single change would give 7.16 room for everything with margin.
…ge gate Picks up BitHighlander/python-keepkey#53. Two things this branch needs: The storage-version gate no longer pins V20, so it stops reporting a failure whose only cause is which firmware branch the emulator was built from. It derives the ladder, the burned set and LAST_SHIPPED from the tree under test and is green on both the 7.15 and 7.16 lines. And the integration job now finishes: 636 passed, 32 skipped, 0 failed in 2m46s, against an emulator built from current firmware. It had been timing out at 30 minutes with zero assertions run, behind a green check.
Picks up #54, which repoints the three storage-gate sections whose tests were renamed. The catalog names the test that evidences each requirement, so a rename orphans its section and the report validation fails even though every test passes -- which is exactly what this branch was seeing.
BitHighlander
marked this pull request as ready for review
August 22, 2026 18:07
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
alpha, bump firmware to 7.16.0, and add CTAPHID CBOR/CANCEL/KEEPALIVE while retaining U2FVerification
alphaat0e494d10444b8111ec741126fbfbf3c49db83b28d4d3796094a349a34a1702cd0d9d1a73218997362690a6d751372398a165e163git diff --checkpassesRelease gates
This is intentionally a draft until: