chore(release): guard against credentials in published firmware - #148
Conversation
Every sdkconfig.credentials value is compiled into .bin and .elf as a plain string. tools/release_guard.sh refuses artifacts that contain any such value or a secret-shaped string (tskey-…, PEM keys, literal auth tokens); CONTRIBUTING documents that only credential-free `-public` builds may be attached to a release.
![graphify-labs[bot]](https://avatars.githubusercontent.com/in/4356270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 3 advisory finding(s) below merit a look before merge.
Graphify review — findings
Adds tools/release_guard.sh, a scanner that refuses (exit 1) to bless a firmware artifact carrying secrets: it extracts every value from any sdkconfig.credentials found in firmware/ or machn/ and greps each .bin/.elf for it, plus flags secret-shaped strings (Tailscale tskey-… keys, PEM private keys, literal Bearer/Basic tokens) even when no credentials file is present, naming the offending pattern without printing the value. Values matching tracked Kconfig default strings (e.g. the documented admin password) are treated as public and won't trip the guard, and empty/boolean/short values are skipped to avoid false positives. Documents the release process in CONTRIBUTING.md: build without credentials using a -public-suffixed PROJECT_VER, then run the guard on every artifact before gh release upload.
Worth a look
- Empty values array under
set -ucauses unbound-variable crash —tools/release_guard.sh:46· Escalate · high- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Short credential values are silently exempted from the release guard —
tools/release_guard.sh:37· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- 'clean' printed per-artifact using global fail flag —
tools/release_guard.sh:61· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 14 functions depend on the 14 functions this change touches.
Health — grade A; no new coupling hotspots.
Verification — 14 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 14 function(s) in the blast radius were not formally verified this run
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
… per-file verdicts Review round: - images compiled with a credentials file carry ML-BUILD-WITH-CREDENTIALS (CMake compile definition + one boot log line in ml_app.c), so the guard refuses them on any machine, whatever credentials file it has or lacks - no empty-array expansion under set -u (bash 3.2) - Kconfig \" and \\ unescaped before matching; short values matched as whole NUL-terminated C strings instead of being skipped - PEM check requires a base64 body (bare header is an mbedTLS constant) - per-artifact clean/LEAK verdict; values de-duplicated
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 3 advisory finding(s) below merit a look before merge.
Graphify review — findings
Adds tools/release_guard.sh, a pre-upload check that refuses any firmware artifact carrying secrets, scanning each .bin/.elf for the private-build brand, the actual values from any sdkconfig.credentials in the tree (short ones matched only as whole NUL-delimited C strings), and secret-shaped patterns like tskey-…, PEM keys, and Bearer/Basic tokens, while whitelisting public Kconfig defaults; exits 1 on any leak with the pattern named but the value never printed. Brands credentials-bearing builds so the guard works on any machine: both firmware and machn CMake now define ML_BUILD_WITH_CREDENTIALS when sdkconfig.credentials exists, and ml_app_start logs the ML-BUILD-WITH-CREDENTIALS marker (referenced to survive --gc-sections) so the verdict doesn't depend on which credentials file the checking machine holds. Documents the release-publishing workflow in CONTRIBUTING.md: build without credentials using a -public version suffix, run the guard on every artifact, and never attach a credentials-bearing build to a release.
Worth a look
- Firmware credentials brand definition is set after ESP-IDF processes components —
firmware/CMakeLists.txt:29· Escalate · high- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Short credential detection path uses a malformed sed regex —
tools/release_guard.sh:38· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- PEM detection requires base64 body immediately after header, missing keys with newline/whitespace-only wrapping represented differently in binary —
tools/release_guard.sh:72· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 60 functions depend on the 46 functions this change touches.
Health — this change adds coupling hotspots:
- new:
dcs_support_init()— 2 callers, 30 callees - new:
ml_app_start()— 1 callers, 11 callees - new:
fleet_ota_checkin()— 1 callers, 8 callees - new:
supervisor_task()— 0 callers, 8 callees
Verification — 60 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 60 function(s) in the blast radius were not formally verified this run
· 4 more finding(s) on lines outside this diff (see the check run).
There was a problem hiding this comment.
I reviewed the new commit (which addresses the five issues from my earlier review) and the automated hunt found no new bugs this run. Because tools/release_guard.sh is the last line of defense against leaking credentials in a public firmware release, a human look is still worthwhile.
What was reviewed: the build-brand plumbing (firmware/CMakeLists.txt, machn/CMakeLists.txt, ml_app.c) now genuinely defines and logs ML_BUILD_WITH_CREDENTIALS, so layer 1 of the guard is no longer dead code; a candidate where a secret matching a different key's Kconfig default silently passes was investigated and judged pre-existing/low-risk; a while read last-line-without-newline gap was also investigated and ruled out. One thing worth a human's eyes: layers 2 (short-value match) and 3 (PEM detection) now rely on grep -P/-z, which stock BSD grep (e.g. macOS's default /usr/bin/grep) doesn't support — worth confirming the guard is actually invoked where GNU grep is available.
Extended reasoning...
Overview
This diff is the fix-up commit (114b5a3) on top of the original release-guard PR (169b6d3). It reworks tools/release_guard.sh to (a) actually define/consume the ML_BUILD_WITH_CREDENTIALS build brand in firmware/CMakeLists.txt, machn/CMakeLists.txt, and components/microlink/src/ml_app.c, (b) switch the credentials-values collection from a bash array to a newline-delimited string to avoid set -u/bash<4.4 unbound-array issues, (c) unescape Kconfig \"/\\ sequences before substring-matching against the artifact, and (d) reset the per-artifact leak flag so batch runs report an accurate clean/LEAK verdict per file instead of latching failure across the whole run.
Security risks
The change is squarely about preventing secret leakage in shipped firmware, so correctness of the detection logic is the whole point. I re-verified, by reading the diff directly, that each of my four prior inline findings (dead build-brand, bash-3.2 unbound array, missing Kconfig unescape, sticky per-file fail flag) is addressed by concrete code changes in this commit, and the fifth (short values silently skipped) is now handled via a NUL-delimited match instead of being dropped. I did not find a way to independently re-break any of those four. The one new observation I have — grep -P and grep -z are GNU extensions not present in BSD/macOS grep — is a portability risk that could silently degrade layers 2/3 of the guard (via the A && B short-circuit not tripping set -e) rather than a logic bug in the matching itself, and I have not confirmed what OS the release process actually runs on.
Level of scrutiny
This remains a security-sensitive, non-trivial bash script that is the sole automated gate before a firmware image reaches a public GitHub release, and a prior review round on this exact PR surfaced multiple real correctness issues. Given that history, a human review of the fix-up commit is warranted even though this run's automated hunt came back clean, rather than approving outright.
Other factors
No CODEOWNERS/ANTOWNERS restrict these paths, and there's no CI wiring that runs release_guard.sh automatically (it's a manual pre-upload step per CONTRIBUTING.md), so there is no automated test coverage forcing platform parity — reinforcing that a human check on the actual release-runner environment is worthwhile.
…ect() - short values matched as whole NUL-delimited C strings via tr|grep -xF, PEM check via tr|grep -E — works with BSD grep (macOS) - idf_build_set_property(COMPILE_DEFINITIONS) placed between include(project.cmake) and project(); re-verified: private build carries the brand, credential-free build does not
|
Round 2 (
|
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 2 advisory finding(s) below merit a look before merge.
Graphify review — findings
Adds tools/release_guard.sh, which refuses (exit 1) any firmware artifact that would leak secrets, checking three independent layers: the ML-BUILD-WITH-CREDENTIALS build brand, every value found in any sdkconfig.credentials in the tree (skipping public Kconfig defaults, matching short values only as whole C strings), and secret-shaped strings like Tailscale keys, PEM private keys, and literal HTTP auth tokens — never printing the offending value. Brands credentials-bearing builds by defining ML_BUILD_WITH_CREDENTIALS in both firmware and machn CMake whenever a credentials file is present, and logging ML-BUILD-WITH-CREDENTIALS from ml_app_start so the marker survives --gc-sections and the guard's verdict doesn't depend on which credentials file the checking machine happens to have. Documents the release workflow in CONTRIBUTING.md: build public artifacts in a clean checkout with a -public version suffix and run the guard on every file before gh release upload.
Worth a look
- Lowercase HTTP auth schemes bypass secret scan —
tools/release_guard.sh:80· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Private-build marker can be compiled out with INFO logs —
components/microlink/src/ml_app.c:1105· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 60 functions depend on the 46 functions this change touches.
Health — this change adds coupling hotspots:
- new:
dcs_support_init()— 2 callers, 30 callees - new:
ml_app_start()— 1 callers, 11 callees - new:
fleet_ota_checkin()— 1 callers, 8 callees - new:
supervisor_task()— 0 callers, 8 callees
Verification — 60 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 60 function(s) in the blast radius were not formally verified this run
· 4 more finding(s) on lines outside this diff (see the check run).
An ESP_LOGI format string is compiled out below LOG_LOCAL_LEVEL, which would silently remove the private-build brand and defeat the guard's machine-independent layer (review). The brand is now a __attribute__((used)) const object, referenced by an ESP_LOGW.
There was a problem hiding this comment.
Graphify reviewed this change.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify review — findings
Adds a release guard that refuses to publish firmware artifacts carrying secrets: tools/release_guard.sh scans each .bin/.elf for a private-build brand, any value from an in-tree sdkconfig.credentials (short values matched only as a whole NUL-terminated C string, Kconfig-unescaped, public Kconfig defaults excluded), and secret-shaped strings (tskey-…, PEM keys, HTTP auth tokens), exiting 1 on any leak without printing the value. Brands every credentials-bearing build so the verdict is machine-independent: the firmware and machn CMakeLists.txt define ML_BUILD_WITH_CREDENTIALS when a credentials file is present, and ml_app_start emits the ML-BUILD-WITH-CREDENTIALS / ML-BUILD-PUBLIC marker into a used static (not just a log format string, so it survives log-level stripping). Documents the publishing workflow in CONTRIBUTING.md — build public releases in a clean checkout with a -public version suffix and run the guard on every artifact before uploading.
No blocking issues surfaced. 15 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 60 functions depend on the 46 functions this change touches.
Health — this change adds coupling hotspots:
- new:
dcs_support_init()— 2 callers, 30 callees - new:
ml_app_start()— 1 callers, 11 callees - new:
fleet_ota_checkin()— 1 callers, 8 callees - new:
supervisor_task()— 0 callers, 8 callees
Verification — 60 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 60 function(s) in the blast radius were not formally verified this run
· 4 more finding(s) on lines outside this diff (see the check run).
There was a problem hiding this comment.
I reviewed the latest commits and didn't find any new bugs — the human look is still worthwhile given this is security-sensitive release-safety tooling that has already needed several rounds of correctness fixes.
What was reviewed:
- The five issues raised in earlier rounds (bash 3.2 unbound-array crash on an empty
values, missing Kconfig\"/\\unescaping, short (<6 char) credential values being skipped, per-artifact "clean" reporting being lost after the first leak, and the private-build brand being emitted only via a log call that's compiled out below a given log level) all appear fixed intools/release_guard.shandcomponents/microlink/src/ml_app.c. - Checked the public-defaults allowlist (
tools/release_guard.sh) matching by bare value rather than by which CONFIG key it came from — this is a pre-existing, already-flagged risk rather than a new one. - Checked the short-value NUL-exact-match path against GNU ld string-merging/suffix concerns — the whole-line match against NUL-split strings avoids the accidental-substring case.
Extended reasoning...
Overview
This run's diff (9d3b0e4..HEAD) is unchanged in scope from prior reviews: it adds tools/release_guard.sh, wires ML_BUILD_WITH_CREDENTIALS into firmware/CMakeLists.txt and machn/CMakeLists.txt, embeds a build-brand string in components/microlink/src/ml_app.c, and documents the publishing workflow in CONTRIBUTING.md. Since my last review (which flagged the brand string being emitted only through an ESP_LOGI/ESP_LOGW call that can be compiled out below a configured log level), one new commit (689546e) landed that moves the brand into a static const char[] __attribute__((used)) object referenced by, but independent of, the log call — directly addressing that finding. The two prior commits (114b5a3, b24bc22) had already addressed the bash 3.2 unbound-array issue (values is now a plain string, not an array, so no ${values[@]} expansion under set -u), the Kconfig escape handling (kconfig_unescape), the short-value skip (now matched via NUL-exact cstrings comparison instead of being dropped), and the per-artifact clean/leak reporting (each loop iteration now resets a local leak flag rather than relying on a single sticky fail).
Security risks
This is exactly the kind of code where injection/exposure risks matter: a bash script that greps binaries for secret material and gates a publish step. The residual risk I can see (the public-defaults allowlist matching by bare value across any CONFIG key, not the specific key the secret came from) is a pre-existing, already-raised concern rather than something new, and is a narrow theoretical case requiring a real secret to collide byte-for-byte with an unrelated public default.
Level of scrutiny
Given the security-sensitive purpose (preventing credential leakage into public releases) and the history of needing three rounds of substantive fixes to get the bash correctness right, this warrants continued human attention even though this run found no new issues — subtle shell/quoting/linker-behavior bugs in this class of script are easy to miss and have already surfaced multiple times in this same PR.
Other factors
No CLAUDE.md applies. The PR's own verification notes (guard run against real v1.3 public/private artifacts) provide some confidence, but I have not independently built firmware to confirm the __attribute__((used)) object survives --gc-sections under every log-level configuration, so I'm not treating that as fully proven — hence deferring rather than approving.
…rd re-establishment, coord deadline (#156) Consolidates #149, #152, #151, #155 and #153 (closed as superseded; their review threads and bot findings were addressed there and the fixes are included here). 12 linear commits, 409+/29−. Every commit was in the build that passed the 3.7 h soak below. ## What is fixed | area | commit(s) | defect | |---|---|---| | USB tether TX | `de335e8` `1b5fe82` | `tinyusb_net_send_sync(…, 0)` reclaimed the packet before the equal-priority TinyUSB task ran the deferred send — ~95 % of device→host frames silently dropped (regression from #135). Now a 20 ms wait; TIMEOUT-after-send duplicate accepted and documented. | | USB tether ring | `3a8a527` | 16-slot ring lost ~1 safety frame/min on a hub-chain host (burst-induced). 64 slots (96 KB PSRAM), 400 ms lifetime. | | Tether link state | `f9ec4cb` | netif never went link-down when the host detached/suspended, so a dead tether stayed the preferred uplink. esp_tinyusb events now drive it; device netif gets its own MAC. | | WiFi failover crash | `9823785` | first time the failover path ever ran it aborted: `dcs_wifi_set_enabled` reaches flash from the supervisor's PSRAM stack. Toggle moved to an internal-stack worker. | | WireGuard re-establishment | `0440ad2` `5b86d21` `e020c86` `cae7e94` `54149eb` `d7a335f` | **root cause of the multi-minute/indefinite bond drops**: on direct regain with no keypair left, the live endpoint stayed 0.0.0.0 (relay-only) and a safety peer fell into the bulk-peer "one-shot, wait for them to initiate" — the machine host's wireguard-go never initiates without data. Safety peers now connect with paced retries; initiations get a second leg to the peer's authenticated direct disco source while DERP-only; disco pings fan out to it. Per-peer WG diagnostics in `/admin/api/peers`. | | Coord partial frame | `8cf5104` | `coord_recv` bounded by a retry count × 2 s socket timeout (≈10 min); now a 10 s wall-clock deadline, `ETIMEDOUT` so callers reconnect (#127). 88-check host test. | ## Evidence - Deterministic WG repro (block direct+DERP, reboot remote mid-outage, unblock direct only, no host help): **v1.3-14 never recovers (>300 s); this branch bonds in 22 s.** - Soak 2026-09-19 12:06–15:47 PDT, 3.7 h, this code + #148/#150/#154, two Ethernet remotes + one USB remote bonded to a ROS 2 machine on the tether host, DUT E-stop cycled every 5 min (42/42, stop ≤0.5 s, re-arm ≤0.75 s): **zero bond drops / rebonds / resets, 100 % bonded samples on all three** (12 638 × 1 Hz each); reply age p99.9 361–391 ms; USB ring 566 916 frames, 0 expired, 0 dropped; machine-side 194 207 requests, 194 170 answered (the rest late DERP-mirror copies, correctly rejected). One 1.07 s receive-side excursion on an Ethernet remote (W5500 RX class, tracked separately). - Baseline for the same class of USB remote on v1.3: 252 ring drops + 55 expiries per 4 h, and 2–7 min bond outages. ## Not in this PR #148 (release guard), #150 (name scrub), #154 (lockstep mismatch diagnostics — SIL comparator, reviewed separately). DERP-mirror latency (1.3 s median) and the W5500 RX stall are follow-ups.
2 participants
Why
Every value in the git-ignored
sdkconfig.credentialsis compiled into both the.binand the.elfas a plain string. Nothing in the repo prevented such a build from being attached to a GitHub release.What
tools/release_guard.sh <artifact>…— exit 1 if any artifact (a) carries the private-build brand, (b) contains a value of a local credentials file (public Kconfig defaults skipped; Kconfig escapes undone; short values matched as whole C strings), or (c) contains a secret-shaped string (tskey-…, PEM key with body, literalBearer/Basictokens). Never prints the value, only which key leaked.sdkconfig.credentialsis present, CMake definesML_BUILD_WITH_CREDENTIALSandml_app.cembedsML-BUILD-WITH-CREDENTIALSvia a boot log line — so the verdict does not depend on which credentials file the checking machine has.<tag>-public, and must pass the guard before upload.Verification
v1.3-publicartifacts now attached to the v1.3 release:clean×6 (bins, full-flash images, ELFs), re-verified on the copies downloaded back from GitHub.tskey-pattern.Review round in 114b5a3 (bash 3.2 empty-array, Kconfig unescape, short values, per-file verdicts, brand). 107+/33− over 5 files.