Move HMAC auth key off firmware image onto internal-flash key store - #472
Open
nateinaction wants to merge 31 commits into
Open
Move HMAC auth key off firmware image onto internal-flash key store#472nateinaction wants to merge 31 commits into
nateinaction wants to merge 31 commits into
Conversation
The command-authentication key was compiled into the firmware image as AUTH_DEFAULT_KEY, making it trivially extractable from a shipped binary (issue #220). This moves the key to a dedicated littlefs partition on internal flash (mounted at /keys), alongside the anti-replay sequence number file which previously lived on the unreliable SD-card FAT filesystem. TcSecurityDeframer now boots keyless and supports provisioning (PROVISION_KEY, bypass-allowlisted, trust-on-first-use while the store is empty) and rotation with up to 2 active keys (ADD_KEY/REMOVE_KEY, both requiring an authenticated frame). SPI validation checks the active key store instead of a hard-coded SPI 0. The key store is shared across all deframer instances (UART/LoRa/Sband): an unknown SPI triggers a reload from disk, so a rotation issued over one link propagates to the others. Also decouples Validator.cpp from the FPP-generated AuthKeyStore type via a plain ActiveSpiSlots array, keeping it (and its host-side gtest coverage) free of F Prime dependencies as documented in test/unit-tests/README.md.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe authentication flow now uses a flash-backed LittleFS key store instead of a compiled-in key. HMAC keys can be provisioned, added, removed, and selected by SPI. Ground tooling and CI supply keys at runtime. ChangesAuthentication key-store migration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CI
participant GroundPlugin
participant TcSecurityDeframer
participant KeyStore
participant PacketValidator
CI->>GroundPlugin: Set PROVES_AUTH_KEY
CI->>TcSecurityDeframer: Send PROVISION_KEY
TcSecurityDeframer->>KeyStore: Persist key by SPI
GroundPlugin->>TcSecurityDeframer: Send authenticated frame
TcSecurityDeframer->>PacketValidator: Validate SPI and sequence
Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…-uart silence integration-uart/integration-radio are both failing with zero bytes ever received from the board after flashing this branch's firmware (PR #472). The USB CDC ACM link repeatedly drops and re-enumerates, consistent with a boot-time crash loop rather than a framing/auth logic bug. Add a temporary step that halts both RP2350 cores via OpenOCD ~20s after boot and dumps CFSR/HFSR/registers/backtrace so we can see the actual fault cause.
…nostic The previous attempt used "rp2350.cm0 halt" as a single command, which isn't valid OpenOCD syntax (only arp_halt exists per-target instance); OpenOCD printed the target's command-usage listing and exited before reaching any of the reg/mdw/bt commands, so the run produced no diagnostic data. Select the target explicitly with "targets rp2350.cmN" first, then issue plain halt/reg/mdw/bt commands against it.
…ult diagnostic The previous attempt's bulk "reg" dump came back with register names but no values, and OpenOCD logged "target was in unknown state when halt was requested" for both cores -- the halt raced the initial poll cycle right after init. Also "bt" isn't a valid OpenOCD console command (that's GDB), so it aborted the remaining -c chain before the cm1 section ever ran. Force an explicit poll before and after halt, add a settle delay, and query pc/lr/sp/xpsr/r0-r3 individually instead of the bulk dump.
Diagnostics are done: the board is not crashing (CFSR/HFSR both read 0, core0 was live in Thread mode with no fault flags set). Restore integration-uart to its pre-diagnostic form.
…radio failure Three rounds of live SWD diagnostics on the integration-uart bench confirm the board is not crashing (CFSR/HFSR read 0 on both cores; core0 was live in Thread mode executing fs_open when halted). The ground side sees real USB serial disconnects instead. Leading theory: moving the sequence-number file and key-store reload onto the internal QSPI flash that also serves the running code trades the old SD-card path's interrupt-safety for the RP2350's requirement to disable interrupts system-wide during any internal flash write, now hit on the hot path (every accepted frame, and every frame with an unrecognized SPI while unprovisioned). Not yet confirmed or fixed -- see PROBLEM.md for the full writeup and candidate next steps.
…on mount keystore_partition@0x400000 sat past the declared 4MB flash0 boundary, so is_valid_range() rejected every flash op with -EINVAL before littlefs could even attempt to mount /keys -- masquerading as the interrupt-stall symptom described in PROBLEM.md. storage_partition already extended to 16MB against this same 4MB declaration on main, unnoticed only because nothing mounted it, which is strong evidence the physical chip is 16MB and the declaration was simply wrong. Awaiting hardware CI to confirm the integration-uart/integration-radio jobs go green with a mountable /keys.
CI run 30034861047 confirms the physical chip is 16MB (OpenOCD: w25q128fv/jv, 16384 KiB), so the flash0 size fix was correct and necessary. But integration-uart/integration-radio still fail with the identical USB-disconnect symptom, so the size fix alone doesn't unblock CI. Revised theory: now that /keys can actually mount, littlefs's first-ever format reaches flash_rpi_write/flash_rpi_erase for the first time, both of which hold irq_lock() for the whole erase/program call -- PROBLEM.md's original interrupt-stall theory may be correct after all, it just had nothing to act on before this fix.
Enable Zephyr console+logging (prj.conf) to see whether the remaining integration-uart/integration-radio USB-disconnect stall lines up with littlefs's first-ever format on the newly-mountable keystore partition, per the revised interrupt-stall theory in INVESTIGATION.md. Console shares cdc_acm_uart0 with the F' downlink, so this intentionally desyncs GDS for this one CI run -- capture raw boot text from a fresh power cycle before anything else opens the port, same tradeoff as the earlier fault-register diagnostic commits. check-console-disabled is temporarily disabled in the build job since it would otherwise fail on this intentional change.
Diagnostic done (CI run 30036653676): boot log shows normal USB init through ~1.16s then total silence for the rest of the 40s capture -- no more log lines and no TM-frame noise either, consistent with a full hang very early in boot. Ruled out a stale PICO_FLASH_SIZE_BYTES hard_assert in the Pico SDK's flash_range_erase (macro isn't defined in this build). Restore prj.conf/ci.yaml to their pre-diagnostic state since console can't coexist with a working GDS link.
The console-log diagnostic (reverted, see INVESTIGATION.md) showed total silence from ~1.16s after boot through the rest of a 40s capture, no firmware/config changes needed here -- halt both cores via OpenOCD and read pc/lr/sp/xpsr at t=+2s/+10s/+20s after a fresh reset to see whether either core is parked inside flash_range_erase/flash_range_program (or a bootrom routine they call), consistent with the revised interrupt-stall theory. Halt/resume only, so the rest of the job's normal (still-failing) GDS flow is undisturbed.
Diagnostic done (CI run 30043983799): cm0's pc/lr/sp/xpsr are identical at t=+2s/+10s/+20s after reset -- zero progress for 20+ seconds. pc resolves to fs_open+2, lr resolves into idle() (the early SYS_INIT/ fstab-automount context), i.e. the hang is at the first real file operation this branch performs after /keys mounts (almost certainly loadKeyStore() opening a virgin key-store file for the first time). cm1 sampled a bootrom address unchanged throughout -- it was never launched into Zephyr code, this app runs single-core. See INVESTIGATION.md "PC-sweep diagnostic" for full reasoning and the two candidate mechanisms, both of which point at replacing the littlefs /keys mount with raw flash_area_*/NVS.
The PC sweep (reverted, see INVESTIGATION.md) located a permanent cm0 freeze
at ~fs_open but could not say why. The one sampled PC (0x101864b8) is mid-way
through fs_open's opening 4-byte stmdb (fs_open starts at 0x101864b6), which
reads more like a stalled instruction fetch / fault than code cleanly blocked
at a call site -- so before committing to the littlefs->NVS rework, capture
what actually distinguishes the candidate mechanisms:
- SCB fault regs (CFSR/HFSR/DFSR/DHCSR) -> faulted / locked-up core
- QMI/XIP state (DIRECT_CSR.EN, M0_RFMT/RCMD, XIP_CTRL) -> XIP left wedged
by the erase/program XIP-exit dance (a flash-layer bug that NVS would hit
identically, making the rework useless)
- SRAM stack dump -> hand-unwind below fs_open (mount/create vs. a k_mutex
wait that the rework would actually fix)
Read-only: halt/read/resume, no firmware or config change, so the rest of the
(still-failing) GDS flow is undisturbed. Deliberately never reads a 0x10xx_xxxx
flash address over SWD -- if XIP is wedged such a read can stall the adapter;
the instruction at PC is already known from zephyr.elf. Added to
integration-uart only (both jobs fail identically). Remove once root-caused.
Run 30051402310 confirmed the hang is NOT a fault or lockup (CFSR/HFSR/DFSR=0, DHCSR S_LOCKUP=0, S_SLEEP=0) and NOT a wedged flash/XIP interface (XIP enabled, QMI_M0_RCMD=0xeb intact, DIRECT_CSR.EN=0) -- ICSR.ISRPENDING=1 with interrupts unserviced points at a spin holding irq_lock/a spinlock, which starves the USB IRQ and drops the CDC link. But the core-register and stack-dump blocks came back empty: bare reg/mdw output does not reach the CI step stdout in batch mode, only echo/capture does. Wrap those reads via a `dump` helper, and add PRIMASK/BASEPRI/FAULTMASK/CONTROL plus r4-r7 so the next run yields the stack unwind (which call chain holds the lock) and the masking mechanism.
The register/QMI/stack capture (runs 30051402310 + 30052303346) ruled out a CPU fault, a lockup, and a wedged flash/XIP interface (XIP enabled, QMI RCMD=0xeb intact) -- so the littlefs->NVS rework is NOT provably futile -- but showed the core spinning with interrupts masked via PRIMASK=1, an off-boundary PC inside fs_open, an incoherent LR (idle), and a shallow garbage stack. That is a software control-flow failure (wild jump or a panic-spin), not the clean fs mutex block the rework was pitched against. v3 attaches arm-zephyr-eabi-gdb (or gdb-multiarch/arm-none-eabi-gdb, or the Zephyr SDK gdb) to the OpenOCD gdb server for a real DWARF backtrace, disassembly around PC, and single-stepping, to decide wild-jump vs. __ASSERT/SPIN_VALIDATE panic vs. lock-spin -- which determines the fix. Read-only; skips gracefully if no gdb is on the integration runner. Uses the flight ELF from the build artifact (build-artifacts/zephyr/fprime-zephyr-deployment). INVESTIGATION.md: add the forensics section, caveat the earlier NVS-rework recommendation, and update the diagnostic-history table + branch-state note.
…spin -> dead SysTick) Reproduced on a local board via SWD (raspberrypi openocd + zephyr-sdk gdb, matching local zephyr.elf). Definitive chain, read off the hung target: wild jump to 0x20010480 (inside FileHandling::fileManager data; lr=0x1019 garbage) -> UsageFault (z_arm_usage_fault/z_arm_fault) -> non-recoverable -> z_arm_fatal_error fatal-halt loop "msr BASEPRI_MAX; b ." (PC pinned across 16 halts + 30 single-steps, primask=1 basepri=0x10, MSP/handler) -> SysTick ISR starved -> cycle_count/curr_tick frozen at ~8.1s -> rate-group k_timer_status_sync never ticks (main parked in arch_swap) -> no telemetry -> GDS "device disconnected". Board confirmed silent 38s (>30s cadence). This supersedes the littlefs-format, flash/XIP-wedge, and interrupt-stall theories and the recommended littlefs->NVS rework -- none address the actual defect. Open: the source of the corrupted pointer (stack overflow / garbage F-prime handler pointer / overrun near fileManager). The flash-size fix stays.
Two independent defects, both confirmed on hardware with single-variable A/B tests (full clean `make generate build` each side, measuring F' telemetry off the board CDC). 1. CommandDispatcher opcode-table overflow -- the CI blocker. CMD_DISPATCHER_DISPATCH_TABLE_SIZE was 350 against a deployment already at 348 commands. This branch adds PROVISION_KEY/ADD_KEY/REMOVE_KEY to TcSecurityDeframer, and there are two instances (ComCcsdsUart, ComCcsdsLora), so 3 x 2 = 6 new commands -> 354 > 350. CommandDispatcherImpl.cpp:35 FW_ASSERTs when the RedBlackTreeMap insert fails, so the 351st registration panics the board during boot (z_fatal_error(reason=4) via z_arm_svc -- a k_panic, not a CPU fault, which is why every fault-vector probe found nothing). The downlink never starts, GDS reports "device disconnected", integration-uart/radio fail. Raised to 512 for headroom. Measured: main 1328 bytes @t+1.0s; branch as shipped 0; branch + table 512 1392 bytes @t+1.01s; branch + table back to 350 again 0. 2. littlefs was never compiled in -- the feature was inert. west.yml's name-allowlist imported fatfs but not littlefs, so the module never reached zephyr_modules.txt, ZEPHYR_LITTLEFS_MODULE was undefined, and Kconfig silently dropped CONFIG_FILE_SYSTEM_LITTLEFS=y. No lfs_* symbols in the image, /keys never existed, every fs_open("/keys/...") failed -- while the build stayed clean. Added to the allowlist and pinned as an explicit project so it lands under lib/zephyr-workspace/ rather than the workspace topdir. Verified end to end on hardware: /keys mounts and formats on internal flash; PROVISION_KEY succeeds on a keyless board over the bypass-allowlisted link; the key survives a cold reboot (re-read by loadKeyStore()); the flash-stored key authenticates uplink (SET_SEQ_NUM took effect); the sequence number survives a cold reboot; erasing the partition re-formats cleanly. The PROVISION_KEY opcodes were re-verified against a fresh dictionary and match Bypasser.cpp. Also adds the SWD/GDB diagnostics used to find this (scripts/diag/) and rewrites INVESTIGATION.md around the confirmed cause -- its earlier conclusions (wild jump, stack overflow, fs-lock deadlock, flash/XIP wedge, littlefs->NVS rework) were all wrong and are marked as such. Follow-ups tracked in TODO.md, including removing these diagnostics before merge and promoting the useful ones to an ADR.
The firmware blockers are fixed and hardware-verified (ec0bdb3), so the old investigation -- which chased a boot "hang" that turned out not to exist -- has served its purpose and is superseded. Its content stays in git history. New INVESTIGATION.md is scoped to the actual remaining goal: integration tests green on the local bench AND in CI. 1. provision_key_test.py errors in pytest *setup*, so the test body never runs. start_gds loops on CdhCore.cmdDisp.CMD_NO_OP and its two-item event sequence times out; recover_from_safe_mode is autouse=True and depends on start_gds, so every test in test/int/ errors with it. The firmware side is proven -- the identical PROVISION_KEY sent outside pytest provisions the board, and the router read routed=3 bypassed=3 rejected=0, so keyless commands are dispatched and none rejected. Leading hypothesis is a GDS downlink desync from resetting the board underneath a long-lived GDS (it logged "APID 2 received sequence count: 4 (expected: 1)"), which CI should not be exposed to since it power-cycles before starting GDS. Recorded as UNCONFIRMED: only 3 bypassed packets were counted against more attempts than that, so uplink loss is not ruled out. Ordered next steps included. 2. No over-the-air recovery from a mis-provisioned key. PROVISION_KEY is refused on a non-empty store, REMOVE_KEY refuses the last key, and ADD_KEY needs an authenticated link -- so a board keyed with the wrong value is unreachable from the ground; bench recovery needed an SWD erase of keystore_partition. Five options are laid out with their security/recoverability trade-offs; this is a decision for the project owners, not a quiet patch. Also adds a TODO to decouple the autouse fixture from start_gds regardless of the cause, records the bench procedure gotchas that cost time, and repoints the stale INVESTIGATION.md section references in TODO.md at git history.
provision_key_test never reached its body. It awaited "either
KeyProvisioned or KeyProvisionFailed" via
await_event(satisfies_any([get_event_pred(a), get_event_pred(b)]))
but IntegrationTestAPI.get_event_pred only passes its argument through
when it is already an event_predicate. A satisfies_any is a predicate
and not an event_predicate, so it was used as the event-ID predicate and
its inner EventData checks were evaluated against an int -- never true.
The event did arrive; the search timed out anyway and the None result
surfaced as an AttributeError. Match over ids instead, and assert the
event is not None so a real no-response failure reports as itself.
The accompanying "every test in test/int/ errors in setup" was a halted
board dropping its USB CDC, not a GDS downlink desync. start_gds used a
bare `assert gds_working`, so that presented as 40-odd unexplained setup
errors; it now names the command, attempt count and last exception. The
redundant start_gds dependency is dropped from recover_from_safe_mode
(every test file already requests start_gds directly).
Four unrelated bench failures also fixed or marked:
- rtc_test's uplink helper fired CreateDirectory /seq and uplinked
immediately, racing the mkdir -- FileOpenError landed 30ms before
CreateDirectorySucceeded whenever /seq did not already exist. Wait for
the directory to resolve either way first. Real pre-existing race.
- Without a battery the power monitor reads 0.012V, so modeManager
auto-enters SAFE_MODE every debounce period and its sequence switches
the face load switch off; enough cycles wedge the face I2C bus for the
session. New --no-battery option drops SafeModeEntryVoltage to 0
before each test (per-test, since PRM_SET does not survive the reboots
reset_manager performs).
- drv2605 asserts a 0.3W rise in INA219 system power, which reads 0.0W
on both samples without a battery -> requires_battery.
- safe_09 asserts the boot count increments after a watchdog-driven
power cycle, which cannot happen with JP6 open -> requires_watchdog_jumper.
Markers are inert in CI, which never passes --bare-flight-controler-board.
Also drop the two Hang Forensics CI steps: the root cause is found, and
they halted the target over SWD immediately before GDS started -- the
exact failure mode above.
Bench result: 30 passed, 0 failed. Both key-store paths verified on
hardware -- a keyless board (keystore erased over SWD) provisions and
then authenticates, and an already-provisioned board reports NotEmpty.
The Start YAMCS Stack step runs `make yamcs`, which launches
tools/yamcs/proves_adapter.py. That adapter builds the same authenticated
framing the GDS plugin does, so it resolves the HMAC key at construction.
While the key was compiled into the flight image the step needed no
environment; now that it is provisioned onto the satellite and read from
PROVES_AUTH_KEY, the adapter died at startup with
ValueError: No authentication key available: pass --authentication-key
or set the PROVES_AUTH_KEY environment variable.
so YAMCS came up with no uplink path and test_noop_round_trip timed out
after 18 CMD_NO_OP attempts. Scope the secret to the step like every
other step that starts a framing process.
Audited the rest of the workflow for the same gap: the only other steps
matching `make yamcs` are Stop YAMCS Stack (`make yamcs-stop`) and
Validate YAMCS server boots with current MDB (`make yamcs-build-check`),
neither of which starts a framing process.
Run 30316568128 otherwise validated the branch end to end on hardware
from a keyless board: Provision Key, Sync Sequence Number and Run UART
Integration Tests all passed in integration-uart, and integration-radio
passed in full.
The key-store work needed a long hardware investigation; its notes and one-off debug tooling were committed along the way. None of it belongs in the reviewable change set: - TODO.md / INVESTIGATION.md / PROBLEM.md: working notes for the boot-hang and CI investigations, now resolved. - scripts/diag/: throwaway OpenOCD/GDB probes (thread walk, downlink trace, fault breakpoints, forensics) used to find the UsageFault and the /keys mount stall. Also drops the FRAM codespell exception (only TODO.md used the word), the stale AuthDefaultKey.h include path in the unit-test CMakeLists, and the branch name from the CommandDispatcher table-size comment. Implementation, tests, and documentation are unchanged; unit tests pass.
Addresses the review callouts on the key-store rotation path. The store file is shared by all three deframer instances (UART/LoRa/Sband), but the code treated it as per-instance state. - Share the key store mutex across instances. It was a per-instance member, so a PROVISION_KEY/ADD_KEY on one instance's command thread could rewrite the file while another instance's dataIn_handler read it on the com thread, at worst yielding a half-old/half-new record whose valid byte is set but whose key bytes are mixed. Replaced with a function-local static taken by every access site. - Make writeKeyStore() durable: write to a temp file, flush, rename over the target, mirroring StartupManager::persist_boot_count. Overwriting in place meant a reset mid-write (e.g. the watchdog power cycle used for command-loss recovery) could truncate the store, which reads back as BAD_SIZE and boots the board keyless with no authenticated way in. - Reload the store before the read-modify-write in PROVISION_KEY, ADD_KEY and REMOVE_KEY. An instance holding a copy that predates a rotation issued over another link would otherwise write it back, resurrecting a revoked key on flash and dropping the current one. This also makes the NotEmpty/StoreFull/LastKey/SpiNotFound rejections truthful against the store that is actually on flash. - Report PSA import failures instead of swallowing them. importKeyStore() now returns whether every valid slot imported; on failure the handlers emit ImportError and EXECUTION_ERROR rather than KeyAdded and OK, so the operator no longer believes a key is live that the board cannot use. The store stays on flash so a reboot or later reload can retry. - Reject ADD_KEY for an SPI that already has a slot, via a new DuplicateSpi status. Two slots with one SPI are unusable: findKeyIdForSpi only returns the first match, so the new key would authenticate nothing while REMOVE_KEY would clear only one of them. - Zeroize the parsed key bytes in PROVISION_KEY and ADD_KEY, matching what importHmacKey already does, covering the early-return paths. DuplicateSpi is appended to KeyStoreProvisionStatus, so existing serialized enum values are unchanged.
nateinaction
marked this pull request as ready for review
July 28, 2026 22:50
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
PROVESFlightControllerReference/Components/TcSecurityDeframer/docs/sdd.md (1)
155-178: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAlign lifecycle event documentation with handler behavior.
Import failures and duplicate-SPI rejection are implemented but not fully documented, and
ActiveKeyCountis emitted before import success is known.
PROVESFlightControllerReference/Components/TcSecurityDeframer/docs/sdd.md#L155-L178: document import and duplicate-SPI failure statuses and correct telemetry timing.docs-site/components/TcSecurityDeframer.md#L155-L178: apply the same correction to the published mirror.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@PROVESFlightControllerReference/Components/TcSecurityDeframer/docs/sdd.md` around lines 155 - 178, The lifecycle event documentation in PROVESFlightControllerReference/Components/TcSecurityDeframer/docs/sdd.md lines 155-178 must document import failure and duplicate-SPI rejection statuses, and state that ActiveKeyCount is emitted only after key import succeeds; apply the identical corrections to docs-site/components/TcSecurityDeframer.md lines 155-178. Update the relevant event and telemetry descriptions without changing unrelated entries..github/workflows/ci.yaml (1)
184-186: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFormat LittleFS before provisioning, not after. Formatting removes
/keys/authkeys.bin; both jobs then reboot into a keyless state before their authenticated phases.
.github/workflows/ci.yaml#L184-L186: moveformat_filesystembeforeProvision Key..github/workflows/ci.yaml#L449-L458: runformat_filesystembeforeprovision_key, or reprovision immediately afterward.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yaml around lines 184 - 186, Move the “Format Filesystem” step before “Provision Key” in .github/workflows/ci.yaml lines 184-186. In the second workflow site at .github/workflows/ci.yaml lines 449-458, run format_filesystem before provision_key, or reprovision the key immediately afterward, so authenticated phases retain the required key.Makefile (1)
156-160: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPrevent the insecure MCUboot sample key from becoming the default signing key.
generate, and thus the documented first-time build path, useskeys/proves.pembutcopy-secretsis required-and-optional. If that step is skipped, Make createskeys/proves.pemfromlib/zephyr-workspace/bootloader/mcuboot/root-rsa-2048.pem, MCUboot’s default sample key whose private key is public. Make won’t regenerate it anyway, so the build can sign the resulting artifact with a known signing key unless the fallback is gated or removed.🔒 Suggested replacement
keys/proves.pem: `@mkdir` -p keys - `@cp` lib/zephyr-workspace/bootloader/mcuboot/root-rsa-2048.pem keys/proves.pem + `@echo` "⚠️ keys/proves.pem is required but has not been copied from SECRETS_DIR."; \ + echo "Run 'make copy-secrets SECRETS_DIR=...' before 'make generate'."; \ + exit 1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Makefile` around lines 156 - 160, Remove the MCUboot sample-key fallback from the keys/proves.pem rule so the default generate path cannot create a signing key with a publicly known private key. Require keys/proves.pem to come from copy-secrets or another explicit provisioning step, and make generate fail clearly when it is absent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Framing/src/authenticate_plugin.py`:
- Around line 36-46: Update the key-loading logic surrounding PROVES_AUTH_KEY
and the CLI authentication-key input to normalize both sources consistently,
then validate that the result contains exactly 32 hexadecimal characters before
returning or constructing the framer. Reject malformed, non-128-bit values with
a clear startup error while preserving the existing missing-key handling and
0x-prefix removal.
In `@PROVESFlightControllerReference/Components/ProvesRouter/Bypasser.cpp`:
- Around line 47-49: The bypass entries for PROVISION_KEY in Bypasser.cpp use
fragile hardcoded opcodes. Replace the 0x2100B002, 0x2200B002, and 0x2300B002
literals with the generated command ID bindings or deployed-dictionary
definitions for the corresponding TcSecurityDeframer commands, preserving the
three radio-link mappings and ensuring future command insertions cannot shift
the allowed opcode.
In
`@PROVESFlightControllerReference/Components/TcSecurityDeframer/Authenticator.hpp`:
- Around line 61-63: Update destroyHmacKey() to return the psa_destroy_key()
status instead of discarding it, and update importKeyStore() plus its
event-handling path to check and surface release failures before clearing
m_keyIds[i]. Only clear the slot after the old key is successfully destroyed,
preserving the existing rotation behavior on success.
In
`@PROVESFlightControllerReference/Components/TcSecurityDeframer/TcSecurityDeframer.cpp`:
- Around line 213-224: Both key-install handlers must zeroize partially
populated key buffers before parse-failure returns. In TcSecurityDeframer.cpp
ranges 213-224 and 271-276, update the handlers using parseHexKey, before
KeyProvisionFailed(ParseKeyError) and KeyAddFailed(ParseKeyError) respectively,
to call mbedtls_platform_zeroize on keyBytes; retain the existing success-path
zeroization.
- Around line 80-96: Gate the SPI-invalid retry reload in the deframer around
validatePacket so loadKeyStore() runs only when the shared key-store generation
has changed, while preserving the initial-load sentinel behavior. Add the shared
keyStoreGeneration() state, increment it only after writeKeyStore() successfully
persists the store, and track the last observed generation with
TcSecurityDeframer::m_keyStoreGeneration before retrying validation.
- Around line 203-211: Gate unauthenticated PROVISION_KEY handling on a
successful key-store read, not merely activeKeyCount() == 0. Add the requested
ReadError value to KeyStoreProvisionStatus in TcSecurityDeframer.fpp, update
loadKeyStore() to record and expose non-OK/non-DOESNT_EXIST read failures, and
make the PROVISION_KEY handler reject with the existing provisioning-failure
response whenever the store is unreadable; retain bootstrap only when the store
is confirmed absent or successfully read as empty.
In
`@PROVESFlightControllerReference/Components/TcSecurityDeframer/TcSecurityDeframer.fpp`:
- Around line 17-25: Update REMOVE_KEY_cmdHandler to zeroize the revoked
AuthKeySlot.key bytes before calling writeKeyStore(), not just clear valid.
Preserve the previous slot contents and restore them if the durable write fails,
so failed persistence does not lose the active key.
In `@PROVESFlightControllerReference/test/int/common.py`:
- Around line 57-60: Update
PROVESFlightControllerReference/test/int/common.py:57-60 in the SAFE_MODE setup
helper to verify EXIT_SAFE_MODE command completion and, preferably, the
resulting mode; do not suppress failures. Update
PROVESFlightControllerReference/test/int/antenna_deployer_test.py:33-35 to rely
on this verified helper before issuing deployment commands, with no separate
unverified SAFE_MODE handling.
In `@PROVESFlightControllerReference/test/int/provision_key_test.py`:
- Around line 53-54: Update the PROVISION_KEY send in the provisioning test to
use the existing proves_send_and_assert_command retry helper instead of calling
fprime_test_api.send_command directly. Preserve the current deframer command and
arguments, and retain the history clearing before the retried attempt.
In `@PROVESFlightControllerReference/test/int/rtc_test.py`:
- Around line 113-125: Update the directory-creation wait in the test around
fprime_test_api.await_event to assert that a completion event is received before
calling uplink_file. Treat timeout or missing completion as an immediate test
failure, while preserving acceptance of both CreateDirectorySucceeded and
DirectoryCreateError events.
In `@README.md`:
- Line 155: Update the authentication-key rotation guidance in the README so it
explicitly states that ground must use the old key while authenticating ADD_KEY,
switch the framing plugin to the new key only after ADD_KEY succeeds, and then
issue REMOVE_KEY for the old SPI.
---
Outside diff comments:
In @.github/workflows/ci.yaml:
- Around line 184-186: Move the “Format Filesystem” step before “Provision Key”
in .github/workflows/ci.yaml lines 184-186. In the second workflow site at
.github/workflows/ci.yaml lines 449-458, run format_filesystem before
provision_key, or reprovision the key immediately afterward, so authenticated
phases retain the required key.
In `@Makefile`:
- Around line 156-160: Remove the MCUboot sample-key fallback from the
keys/proves.pem rule so the default generate path cannot create a signing key
with a publicly known private key. Require keys/proves.pem to come from
copy-secrets or another explicit provisioning step, and make generate fail
clearly when it is absent.
In `@PROVESFlightControllerReference/Components/TcSecurityDeframer/docs/sdd.md`:
- Around line 155-178: The lifecycle event documentation in
PROVESFlightControllerReference/Components/TcSecurityDeframer/docs/sdd.md lines
155-178 must document import failure and duplicate-SPI rejection statuses, and
state that ActiveKeyCount is emitted only after key import succeeds; apply the
identical corrections to docs-site/components/TcSecurityDeframer.md lines
155-178. Update the relevant event and telemetry descriptions without changing
unrelated entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0c767840-d4fe-40b3-a08a-59b52bed3a3c
📒 Files selected for processing (37)
.github/workflows/ci.yamlAGENTS.mdFraming/src/authenticate_plugin.pyMakefilePROVESFlightControllerReference/Components/ProvesRouter/Bypasser.cppPROVESFlightControllerReference/Components/TcSecurityDeframer/Authenticator.cppPROVESFlightControllerReference/Components/TcSecurityDeframer/Authenticator.hppPROVESFlightControllerReference/Components/TcSecurityDeframer/TcSecurityDeframer.cppPROVESFlightControllerReference/Components/TcSecurityDeframer/TcSecurityDeframer.fppPROVESFlightControllerReference/Components/TcSecurityDeframer/TcSecurityDeframer.hppPROVESFlightControllerReference/Components/TcSecurityDeframer/Types.hppPROVESFlightControllerReference/Components/TcSecurityDeframer/Validator.cppPROVESFlightControllerReference/Components/TcSecurityDeframer/Validator.hppPROVESFlightControllerReference/Components/TcSecurityDeframer/docs/sdd.mdPROVESFlightControllerReference/ReferenceDeployment/Top/ReferenceDeploymentPackets.fppiPROVESFlightControllerReference/project/config/CommandDispatcherImplCfg.hppPROVESFlightControllerReference/test/int/antenna_deployer_test.pyPROVESFlightControllerReference/test/int/common.pyPROVESFlightControllerReference/test/int/conftest.pyPROVESFlightControllerReference/test/int/drv2605_test.pyPROVESFlightControllerReference/test/int/mode_manager_test.pyPROVESFlightControllerReference/test/int/provision_key_test.pyPROVESFlightControllerReference/test/int/rtc_test.pyPROVESFlightControllerReference/test/int/tmp112_test.pyPROVESFlightControllerReference/test/int/veml6031_test.pyPROVESFlightControllerReference/test/unit-tests/CMakeLists.txtPROVESFlightControllerReference/test/unit-tests/test_TcSecurityDeframer_Authenticator.cppPROVESFlightControllerReference/test/unit-tests/test_TcSecurityDeframer_Validator.cppREADME.mdboards/bronco_space/proves_flight_control_board_v5/proves_flight_control_board_v5.dtsidocs-site/components/TcSecurityDeframer.mdprj.confpytest.iniscripts/generate_auth_default_key.hscripts/generate_auth_key_header.pytools/yamcs/proves_adapter.pywest.yml
💤 Files with no reviewable changes (3)
- scripts/generate_auth_key_header.py
- scripts/generate_auth_default_key.h
- PROVESFlightControllerReference/test/unit-tests/CMakeLists.txt
Flight side (TcSecurityDeframer): - Gate the unknown-SPI store reload on a process-wide generation counter. SPI validation runs before MAC verification, so an unauthenticated frame carrying a bogus SPI reached the reload path: a littlefs read plus a full PSA destroy/re-import of every slot, on the com thread, holding the lock shared by all three uplinks. A stream of such frames serialized UART, LoRa and Sband behind that work with no credential needed. Every writer is in this process and bumps the counter under the same lock, so the reload stays exact while costing nothing in the common case. - Refuse PROVISION_KEY/ADD_KEY/REMOVE_KEY on a store that could not be read back (new StoreUnreadable status). loadKeyStore() only resets m_keyStore on OP_OK and DOESNT_EXIST; on any other status a freshly booted instance kept the default all-invalid value, so activeKeyCount() returned 0 and the trust-on-first-use check in PROVISION_KEY passed. Since PROVISION_KEY is bypass-allowlisted, any induced read failure - a truncated record reading back as BAD_SIZE, a littlefs error, a mount not ready at configure() time - let an unauthenticated party install their own key while a valid one was still on flash. TOFU now requires proof the store is empty. The same guard keeps ADD_KEY/REMOVE_KEY from writing a stale guess over the real store. - Zeroize revoked key bytes in REMOVE_KEY before persisting. Clearing `valid` alone left the raw key in the fixed-layout record, recoverable from /keys/authkeys.bin; the slot is restored if the durable write fails. The write-failure rollbacks in PROVISION_KEY/ADD_KEY zeroize too, so an invalid slot never carries key bytes into a later successful write. - Zeroize keyBytes on the parse-failure returns. parseHexKey fills the buffer as it scans, so a key rejected part-way through left a prefix of real key bytes on the stack. - destroyHmacKey() returns the PSA status instead of discarding it, and importKeyStore() reports a failed release. A destroy that fails leaves the old key live in PSA while the slot was being treated as recycled - on REMOVE_KEY that told the operator a key was revoked when it could still authenticate frames. Ground: - authenticate_plugin.py validates the key at startup, from both --authentication-key and PROVES_AUTH_KEY: exactly 32 hex characters after normalizing an optional 0x prefix. Malformed values previously blew up in bytes.fromhex() mid-run or produced frames the board silently rejected. - README documents the ground-side key transition during rotation, since ground uses one key at a time while the board holds two. Integration tests: - provision_key_test retries PROVISION_KEY on the same Fibonacci backoff the rest of the suite uses. Every authenticated test depends on this one, so a single dropped uplink failed the whole provisioning gate. - exit_safe_mode() asserts the command completes instead of swallowing every failure. If it never lands, deployment stays inhibited and the faces stay unpowered, and the caller fails for a reason unrelated to what it tested. - rtc_test asserts the CreateDirectory outcome event arrived, instead of falling through into the uplink race the wait exists to close.
Since the key store moved to flash, importKeyStore() imports each slot's raw bytes with importHmacKeyBytes(); the hex-parsing wrapper had no callers left outside its own tests. KeyImportStatus::ParseKeyError goes with it, as importHmacKey was the only thing that produced it - the identically named KeyStoreProvisionStatus::ParseKeyError on the command handlers is unrelated and stays. The two tests covering the wrapper's parse path are dropped (parseHexKey has its own null/invalid-hex coverage) and importTestKey() now goes through parseHexKey + importHmacKeyBytes.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
PROVESFlightControllerReference/Components/TcSecurityDeframer/Authenticator.hpp (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winIntroduce a dedicated key-size constant instead of reusing
kTCSecurityTrailer.
kTCSecurityTrailernames/represents the CCSDS security trailer (MAC) size; it's being reused across these files purely because it currently equals the 128-bit HMAC key size too. Nothing ties the two together, so a future change to the MAC truncation length would silently mis-size the key-parsing/import buffers.
PROVESFlightControllerReference/Components/TcSecurityDeframer/Authenticator.hpp#L48-58: define a distinctconstexpr size_t kHmacKeySize = 16;(or similar) and use it forparseHexKey's andimportHmacKeyBytes's key-byte-array parameter sizes instead ofCcsds355_0_B_2::kTCSecurityTrailer.PROVESFlightControllerReference/Components/TcSecurityDeframer/TcSecurityDeframer.cpp#L249-249: sizekeyBytesinPROVISION_KEY_cmdHandlerwith the new key-size constant.PROVESFlightControllerReference/Components/TcSecurityDeframer/TcSecurityDeframer.cpp#L322-322: sizekeyBytesinADD_KEY_cmdHandlerwith the new key-size constant.PROVESFlightControllerReference/Components/TcSecurityDeframer/TcSecurityDeframer.cpp#L404-419: sizerevokedKeyinREMOVE_KEY_cmdHandler(and itsstatic_assert) with the new key-size constant.PROVESFlightControllerReference/test/unit-tests/test_TcSecurityDeframer_Authenticator.cpp#L19-28: sizekeyBytesinimportTestKey()with the new key-size constant.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@PROVESFlightControllerReference/Components/TcSecurityDeframer/Authenticator.hpp` at line 1, Define a dedicated kHmacKeySize constant in Authenticator.hpp and use it for the key-array parameters of parseHexKey and importHmacKeyBytes. Replace kTCSecurityTrailer with kHmacKeySize when sizing keyBytes in PROVISION_KEY_cmdHandler and ADD_KEY_cmdHandler, revokedKey and its static_assert in REMOVE_KEY_cmdHandler, and keyBytes in the test helper importTestKey().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@PROVESFlightControllerReference/Components/TcSecurityDeframer/Authenticator.cpp`:
- Around line 95-96: Update the ImportKeyBytesDirectly unit test to capture and
assert the return value from destroyHmacKey, ensuring key-destruction failures
fail the test instead of being ignored. Keep the existing destroyHmacKey
implementation unchanged.
---
Outside diff comments:
In
`@PROVESFlightControllerReference/Components/TcSecurityDeframer/Authenticator.hpp`:
- Line 1: Define a dedicated kHmacKeySize constant in Authenticator.hpp and use
it for the key-array parameters of parseHexKey and importHmacKeyBytes. Replace
kTCSecurityTrailer with kHmacKeySize when sizing keyBytes in
PROVISION_KEY_cmdHandler and ADD_KEY_cmdHandler, revokedKey and its
static_assert in REMOVE_KEY_cmdHandler, and keyBytes in the test helper
importTestKey().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b6675d45-a771-470a-9459-8b4ec7b5df74
📒 Files selected for processing (15)
Framing/src/authenticate_plugin.pyPROVESFlightControllerReference/Components/TcSecurityDeframer/Authenticator.cppPROVESFlightControllerReference/Components/TcSecurityDeframer/Authenticator.hppPROVESFlightControllerReference/Components/TcSecurityDeframer/TcSecurityDeframer.cppPROVESFlightControllerReference/Components/TcSecurityDeframer/TcSecurityDeframer.fppPROVESFlightControllerReference/Components/TcSecurityDeframer/TcSecurityDeframer.hppPROVESFlightControllerReference/Components/TcSecurityDeframer/docs/sdd.mdPROVESFlightControllerReference/test/int/common.pyPROVESFlightControllerReference/test/int/provision_key_test.pyPROVESFlightControllerReference/test/int/rtc_test.pyPROVESFlightControllerReference/test/unit-tests/test_TcSecurityDeframer_Authenticator.cppREADME.mddocs-site/components/TcSecurityDeframer.mdprj.confwest.yml
💤 Files with no reviewable changes (2)
- west.yml
- prj.conf
… store (#485) A board whose key store file does not exist - a brand-new board, or one whose /keys partition was erased or reformatted - could not be provisioned. It came up keyless (correct) and then refused PROVISION_KEY with StoreUnreadable forever, which is total, unrecoverable command loss over every link. Root cause is a host/target divergence. The gate added in b214dd8 reads if (loadStatus != OP_OK && loadStatus != DOESNT_EXIST) -> StoreUnreadable but ZephyrFile::open (lib/fprime-zephyr Os/File.cpp) discards fs_open's errno and returns OTHER_ERROR for every failure, so Os::File::DOESNT_EXIST is unreachable on flight hardware. On the POSIX host the same missing file maps ENOENT -> DOESNT_EXIST and the gate passes, which is why host unit tests never saw it. CI hardware never saw it either: every bench board was seeded under 69dcec7 (which gated only on activeKeyCount(), reading the default-empty in-memory store), and the littlefs keystore partition survives reflashing, so the cold path is never exercised. The naive fix - map -ENOENT to DOESNT_EXIST in the Zephyr shim - would reintroduce the hole b214dd8 closed. Zephyr's fs_get_mnt_point returns -ENOENT when the mount point itself is absent, identically to a missing file, so ENOENT alone would let an attacker who can keep /keys from mounting provision their own key over a board that still holds a valid one (PROVISION_KEY is bypass-allowlisted, i.e. unauthenticated and reachable over RF). Instead, stop inferring anything from the read status and probe the filesystem: * fs_stat on the store file (via FileSystem::_getPathType, not getPathType(), which folds every error into NOT_EXIST) for a positive absence answer; * fs_statvfs on the mount point (via FileSystem::getFreeSpace) as independent proof that /keys is actually mounted. Provisioning is permitted only for a demonstrably-absent file on a demonstrably live mount, or a store that read back cleanly and holds no key. An unreadable store is never provisionable, so the security property is preserved. ADD_KEY and REMOVE_KEY use the same predicate, since their gates had the same defect. The decision itself lives in Components::KeyStore in Types.hpp as pure C++ over probe results rather than over an Os status, so the divergence has nowhere to hide and the host gtest suite can cover the whole domain. A plain component test could not have caught this bug, since on the host the buggy code passes. - Types.hpp: KeyStore::MountProbe / StoreProbe / storeStateIsKnown / storeIsProvisionable (pure, header-only, no Fw:: or Os:: dependency) - TcSecurityDeframer: probeKeyStore() + the three rewritten gates - test_TcSecurityDeframer_KeyStorePolicy.cpp: exhaustive mount x store x key-count matrix, plus a test that documents the host/target divergence - provision_key_test.py: spell out why NotEmpty (and only NotEmpty) is tolerated, and add an explicit skipped test marking the cold-provision coverage gap - sdd.md: document the probe and why the read status is not a usable signal Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
PROVESFlightControllerReference/Components/TcSecurityDeframer/TcSecurityDeframer.cpp (1)
628-647: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRemove the temporary key-store file when the durable write fails.
writeKeyStore()writes the complete store, including all plaintext key bytes, to<m_keyStoreFilePath>.tmp. Ifflush()orrename()fails, that file stays on the/keyspartition and is never removed. The result is a second copy of live key material at a path that no other code manages.This also weakens the guarantee
REMOVE_KEYestablishes. Lines 449-451 zeroize a revoked key so its bytes do not remain on flash. If the rename fails during aREMOVE_KEY, the pre-removal store — including the revoked key — persists in.tmp.Delete the temporary file on every failure path.
🔒️ Proposed fix: unlink the temporary file on failure
if (status == Os::File::OP_OK && Os::FileSystem::rename(tempPath.toChar(), this->m_keyStoreFilePath.toChar()) != Os::FileSystem::OP_OK) { status = Os::File::OTHER_ERROR; } if (status != Os::File::OP_OK) { + // The temp file holds a full plaintext copy of the store. It never becomes authoritative, + // so remove it rather than leaving key material at a path nothing else manages. + (void)Os::FileSystem::removeFile(tempPath.toChar()); this->log_WARNING_HI_KeyStoreWriteFailed(static_cast<Os::FileStatus::T>(status)); } else {Confirm the removal API name on this F Prime version before applying:
#!/bin/bash # Find the file-removal entry point on Os::FileSystem. rg -nP -C4 '\b(removeFile|remove)\s*\(' --iglob '*FileSystem*.hpp'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@PROVESFlightControllerReference/Components/TcSecurityDeframer/TcSecurityDeframer.cpp` around lines 628 - 647, Update writeKeyStore’s temporary-file flow to remove tempPath whenever the write, flush, or rename operation fails, including failures after file.close(). Confirm and use the available Os::FileSystem file-removal API, while preserving the existing rename-on-success behavior and status handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@PROVESFlightControllerReference/Components/TcSecurityDeframer/TcSecurityDeframer.cpp`:
- Around line 628-647: Update writeKeyStore’s temporary-file flow to remove
tempPath whenever the write, flush, or rename operation fails, including
failures after file.close(). Confirm and use the available Os::FileSystem
file-removal API, while preserving the existing rename-on-success behavior and
status handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d8f1e392-c157-4fe1-bc8a-df80c83b42a4
📒 Files selected for processing (7)
PROVESFlightControllerReference/Components/TcSecurityDeframer/TcSecurityDeframer.cppPROVESFlightControllerReference/Components/TcSecurityDeframer/TcSecurityDeframer.hppPROVESFlightControllerReference/Components/TcSecurityDeframer/Types.hppPROVESFlightControllerReference/Components/TcSecurityDeframer/docs/sdd.mdPROVESFlightControllerReference/test/int/provision_key_test.pyPROVESFlightControllerReference/test/unit-tests/test_TcSecurityDeframer_KeyStorePolicy.cppdocs-site/components/TcSecurityDeframer.md
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
PROVESFlightControllerReference/test/int/rtc_test.py (2)
471-475:⚠️ Potential issue | 🟠 MajorAssert both
TimeBaseChangedevents.The two
await_eventresults are discarded. The test can pass when either event is missing. The F Prime GDS API leaves success checking to the caller forawait_searches. Its default search starts after existing history, so it can also miss an event emitted whileproves_send_and_assert_commandis processing the command. (fprime.jpl.nasa.gov)Replace both calls with
assert_event(..., start=0, timeout=10). The helper clears histories before dispatch, sostart=0checks the current command history and still allows late delivery.Suggested fix
- fprime_test_api.await_event(f"{rtcManager}.TimeBaseChanged", timeout=10) + fprime_test_api.assert_event( + f"{rtcManager}.TimeBaseChanged", start=0, timeout=10 + ) ... - fprime_test_api.await_event(f"{rtcManager}.TimeBaseChanged", timeout=10) + fprime_test_api.assert_event( + f"{rtcManager}.TimeBaseChanged", start=0, timeout=10 + )As per coding guidelines,
PROVESFlightControllerReference/test/int/**/*.pyintegration tests must use pytest with the fprime-gdsIntegrationTestAPIfor commands and event/telemetry assertions.#!/bin/bash set -euo pipefail rg -n -C 2 'TimeBaseChanged|await_event|assert_event' \ PROVESFlightControllerReference/test/int/rtc_test.py rg -n -C 12 'def proves_send_and_assert_command' \ PROVESFlightControllerReference/test/int/common.pyAlso applies to: 478-481
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@PROVESFlightControllerReference/test/int/rtc_test.py` around lines 471 - 475, Update both TimeBaseChanged checks in the RTC integration test to use the fprime-gds IntegrationTestAPI assert_event method with start=0 and timeout=10, and assert each returned result. Replace the existing await_event calls while preserving the current command flow and event names.Source: Coding guidelines
466-466: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the intentional fixture-only parameter.
Ruff reports
ARG001becausestart_gdsis not referenced in the function body. Keep the parameter so pytest executes the fixture. Add# noqa: ARG001with a short reason, or configure Ruff to ignore pytest fixture parameters.Suggested fix
-def test_11_proc_toggle(fprime_test_api: IntegrationTestAPI, start_gds): +def test_11_proc_toggle( + fprime_test_api: IntegrationTestAPI, start_gds # noqa: ARG001 +):As per coding guidelines,
**/*.{c,cc,cpp,h,hpp,py,fpp,yml,yaml,json}files requiremake fmtbefore every commit to execute formatting, linting, spell checking, and standard pre-commit checks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@PROVESFlightControllerReference/test/int/rtc_test.py` at line 466, Add an inline Ruff suppression with a brief explanation to the unused start_gds parameter in test_11_proc_toggle, preserving the parameter so pytest still injects and executes the fixture. Do not remove the fixture parameter or alter the test behavior.Sources: Coding guidelines, Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@PROVESFlightControllerReference/test/int/rtc_test.py`:
- Around line 471-475: Update both TimeBaseChanged checks in the RTC integration
test to use the fprime-gds IntegrationTestAPI assert_event method with start=0
and timeout=10, and assert each returned result. Replace the existing
await_event calls while preserving the current command flow and event names.
- Line 466: Add an inline Ruff suppression with a brief explanation to the
unused start_gds parameter in test_11_proc_toggle, preserving the parameter so
pytest still injects and executes the fixture. Do not remove the fixture
parameter or alter the test behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1eb2c2b8-0c5d-416d-8875-9d66e6960c99
📒 Files selected for processing (1)
PROVESFlightControllerReference/test/int/rtc_test.py
Contributor
|
Confirmed
Testing
- works with two keys provisioned
-Watchdog / power-cut mid-command storm Note:
|
ineskhou
reviewed
Aug 3, 2026
| cache-size = <64>; | ||
| lookahead-size = <32>; | ||
| block-cycles = <512>; | ||
| }; |
Contributor
There was a problem hiding this comment.
Michael got some help for the review of risks https://claude.ai/code/session_01VRCjXtQCFyoBvekU4zcCQX
it pointed out that if Zephyr fails to mount the littlefs volume at /keys, it mounts the empty one. I think this is the desired behvaour, a empty filesytem allows for recovery unlike a corrupt one. However it would allow someone to provision a first key which could be seen.
We have observed FAT filesystems failing in the sd card, but this is a littlefs. We could move the sequence number off here and back to the regular filesusyem to hammer the filesystem better. Could also think of a better wat to fall bacm on a failed filesysten
I'm highlighting this one bc of the suspected filesystem issues we've had on the last 3 sats (unconfirmed)
Collaborator
Author
There was a problem hiding this comment.
Hmmm based on my reading of this comment it sounds like we might want to move the seq num back only to experiment trying to understand FAT failure modes? This doesn't sound like a request for this PR where we want to introduce a reliable component.
Callout for mounting risk sounds OK to me. Good callout.
Let me know if I misread this.
| bool hasSpi(uint16_t spi) const; | ||
|
|
||
| //! Returns the number of valid slots in m_keyStore. Must be called with the key store lock held. | ||
| U8 activeKeyCount() const; |
Contributor
There was a problem hiding this comment.
Could be good to get this, as well as what spis exist, in case we forgot what slot we put keys in? not super important tho
Collaborator
Author
There was a problem hiding this comment.
I agree, it would be good to have an SPI list where we get fingerprints back so we can ID which keys are onboard.
Collaborator
Author
There was a problem hiding this comment.
Filed #488 to track this — a bit more involved than fits in this PR (new command + fingerprint, not just exposing existing state).
Operators provisioning a non-default SPI had no way to point make gds at it and had to fall back to invoking fprime-gds directly. SPI=<n> now forwards --spi the same way UART_DEVICE forwards --uart-device. Addresses PR #472 review comment from ineskhou.
The rotation section assumed a key was already provisioned and gave a beginner nowhere to start. Add a short walkthrough of PROVISION_KEY's trust-on-first-use bootstrap (allowed once, only while the on-flash store is empty) and point at the new make gds SPI=<n>. Addresses PR #472 review comments from ineskhou on README.md and Framing/src/authenticate_plugin.py.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Makefile`:
- Around line 413-417: Update the GDS launch logic around the UART_DEVICE
conditional so the default $(GDS_COMMAND) invocation runs only when UART_DEVICE
is unset. Preserve the UART-specific invocation and SPI arguments, ensuring make
gds UART_DEVICE=... starts exactly one GDS process.
In `@README.md`:
- Line 159: Update the README’s key-provisioning description to remove the claim
that PROVISION_KEY becomes available after every key has been removed, since
REMOVE_KEY cannot remove the last active key. Keep the first-boot empty-store
behavior intact, or document the required key-store reset operation if that
recovery path must remain supported.
- Around line 161-163: Update the README procedure so the authentication key is
configured before the Step 1 GDS launch, using PROVES_AUTH_KEY or
--authentication-key. Include the selected non-zero SPI in that same startup
command, then keep the provisioning and ground configuration steps consistent
with those values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2baf0be6-d158-4930-91f9-121453970054
📒 Files selected for processing (2)
MakefileREADME.md
Comment on lines
413
to
+417
| @if [ -n "$(UART_DEVICE)" ]; then \ | ||
| echo "Using UART_DEVICE=$(UART_DEVICE)"; \ | ||
| $(GDS_COMMAND) --uart-device $(UART_DEVICE); \ | ||
| $(GDS_COMMAND) --uart-device $(UART_DEVICE) $(if $(SPI),--spi $(SPI)); \ | ||
| fi | ||
| $(GDS_COMMAND) | ||
| $(GDS_COMMAND) $(if $(SPI),--spi $(SPI)) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/sh
set -eu
rg -n -C 4 '^(GDS_COMMAND|UART_DEVICE)|^gds:' Makefile
make -n gds UART_DEVICE=/dev/ttyTEST SPI=1Repository: Open-Source-Space-Foundation/proves-core-reference
Length of output: 874
🏁 Script executed:
#!/bin/sh
set -eu
echo "== Makefile relevant section =="
sed -n '405,425p' Makefile
echo
echo "== dry-run command extraction (Make not installed) =="
python3 - <<'PY'
# Read Make recipe lines without executing repository code.
# Shell scripts are shown with echo to demonstrate call shapes as data.
from pathlib import Path
text = Path("Makefile").read_text()
print("Makefile exists")
PYRepository: Open-Source-Space-Foundation/proves-core-reference
Length of output: 946
Make the UART and default GDS launches mutually exclusive.
When UART_DEVICE is set, the if branch runs the UART-specific $(GDS_COMMAND), then the succeeding unconditional command runs $(GDS_COMMAND) again. Use else or guard the second call so make gds UART_DEVICE=... starts exactly one GDS process.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Makefile` around lines 413 - 417, Update the GDS launch logic around the
UART_DEVICE conditional so the default $(GDS_COMMAND) invocation runs only when
UART_DEVICE is unset. Preserve the UART-specific invocation and SPI arguments,
ensuring make gds UART_DEVICE=... starts exactly one GDS process.
|
|
||
| ##### Provisioning your first key | ||
|
|
||
| A freshly flashed board boots with an empty on-flash key store — no authentication key ever ships in the image. Because the store is empty, the board allows exactly one unauthenticated command: `PROVISION_KEY`. Once any key is provisioned, `PROVISION_KEY` is refused, so this only works the first time (or again after every key has been removed). |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Remove the unreachable empty-store claim.
REMOVE_KEY refuses to remove the last active key, as stated at Line 171. Therefore, PROVISION_KEY cannot become available again “after every key has been removed” through the documented commands. Document the required key-store reset operation, or remove this clause.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` at line 159, Update the README’s key-provisioning description to
remove the claim that PROVISION_KEY becomes available after every key has been
removed, since REMOVE_KEY cannot remove the last active key. Keep the first-boot
empty-store behavior intact, or document the required key-store reset operation
if that recovery path must remain supported.
Comment on lines
+161
to
+163
| 1. Start GDS as normal (`make gds`). | ||
| 2. From the GDS command view, send `PROVISION_KEY(spi=<n>, key=<32 hex chars>)`, e.g. `PROVISION_KEY(0, 00112233445566778899aabbccddeeff)`. | ||
| 3. Tell ground to use the same key for every command after that: set `PROVES_AUTH_KEY` to the same hex string (or pass `--authentication-key`). If you provisioned a non-zero SPI, also run gds with `make gds SPI=<n>` (or pass `--spi <n>` directly) so ground's outgoing frames carry the matching SPI. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Set the authentication key before starting GDS.
The framing plugin requires --authentication-key or PROVES_AUTH_KEY at startup. Step 1 starts make gds before the user sets either value, so a fresh operator cannot start GDS or send PROVISION_KEY. Set the key before Step 1 and include SPI in the same launch when using a non-zero SPI.
Proposed procedure
-1. Start GDS as normal (`make gds`).
-2. From the GDS command view, send `PROVISION_KEY(spi=<n>, key=<32 hex chars>)`.
-3. Tell ground to use the same key for every command after that.
+1. Set `PROVES_AUTH_KEY` to the 32-hex-character key.
+2. Start GDS with `PROVES_AUTH_KEY=<key> make gds` or `make gds SPI=<n>`.
+3. From the GDS command view, send `PROVISION_KEY(spi=<n>, key=<32 hex chars>)`.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` around lines 161 - 163, Update the README procedure so the
authentication key is configured before the Step 1 GDS launch, using
PROVES_AUTH_KEY or --authentication-key. Include the selected non-zero SPI in
that same startup command, then keep the provisioning and ground configuration
steps consistent with those values.
4 tasks
This was referenced Aug 4, 2026
5 tasks
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
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In this PR the command-authentication key moves off the firmware image and onto a dedicated littlefs partition on internal flash, mounted at
/keys. The key was previously compiled into the binary asAUTH_DEFAULT_KEY, which issue #220 showed is trivially extractable from a shipped image, so baking it in defeated the point of authentication. The anti-replay sequence-number file moves to the same partition, since it previously lived on the unreliable SD-card FAT filesystem.A board now boots keyless and is provisioned afterward.
TcSecurityDeframergains three commands:PROVISION_KEYfor bootstrap (bypass-allowlisted so it works with no key on board, but only honored while the store is empty), andADD_KEY/REMOVE_KEYfor rotation (both require an authenticated frame). Up to 2 keys can be active at once so a rotation can add the new key before removing the old one. SPI validation now checks the active key store instead of a hard-coded SPI 0. The store is shared across all deframer instances (UART/LoRa/Sband): on an unknown SPI a deframer reloads the store from disk once before rejecting, so a rotation issued over one link is picked up by the others.The ground-side framing plugin no longer reads a compiled-in default key; it takes
--authentication-keyor thePROVES_AUTH_KEYenv var. CI provisions the board after flashing via a newprovision_keyintegration test before running the authenticated integration suites.Along the way,
Validator.cpphad started depending on the FPP-generatedAuthKeyStoretype (which pulls inFw::Serializable), breaking the project's pure-C++ contract for thetest-unitgtest suite. It's decoupled via a small plainActiveSpiSlotstype inTypes.hpp, withTcSecurityDeframer::activeSpiSlots()doing the conversion. New unit tests coverparseHexKey,importHmacKeyBytes/destroyHmacKey, and SPI/sequence-number validation against the new active-key-set shape.Test plan
make test-unit— all 8 suites pass, including expanded Authenticator/Validator coveragemake generate build— clean full build with noAuthDefaultKey.h, FLASH 69.82%, RAM 62.32%make check-console-disabled— OK