feat(provision): keep credentials out of the state file entirely - #1832
Open
clonea1 wants to merge 9 commits into
Open
feat(provision): keep credentials out of the state file entirely#1832clonea1 wants to merge 9 commits into
clonea1 wants to merge 9 commits into
Conversation
provision.py keys its state files by serial port -- COM3.json, COM5.json. A
port describes which USB socket you happened to use, not which board is in it.
Cycling six boards through three sockets makes a duplicate node_id
near-certain, and nothing errors: the second board silently takes the first
one's identity, and it surfaces later as two nodes claiming to be node 2 while
the link table quietly merges them.
A MAC is burned into the silicon, so a board keeps its identity regardless of
socket, order, or machine.
board_index.py what is plugged in, and who is it
board_index.py --assign give an unrecognised board the lowest free id
board_index.py --watch sit and report boards as they mount
The index only ever adds. An existing MAC's node_id is never rewritten,
because that rewrite is the exact accident this prevents. A duplicate id in
the index halts with a loud message rather than printing a tidy table over it.
Reads the MAC via esptool (authoritative, but resets the board into its
bootloader -- harmless when about to flash, disruptive otherwise), with
--passive to sniff the boot log instead and leave a running node alone.
Index is gitignored: physical hardware addresses, not secret but not useful to
anyone else, and it churns per fleet.
Co-Authored-By: claude-flow <ruv@ruv.net>
(cherry picked from commit 68e4209)
… line provision.py takes --password as an argument, so every invocation leaves the credential in shell history, process listings, terminal scrollback and any transcript of the session. Easy to do, easy to forget you did, and permanent. This reads it from a file outside the repo and never prints it. The password still reaches the child's argv -- that is provision.py's only interface -- so this is not defence against a local attacker; it stops the credential being permanently recorded somewhere it does not belong. Also fills in the two things easiest to get wrong by hand: - node_id comes from board_index.json, so identity follows the MAC rather than whichever USB socket the board landed in. - --tdm-total is REQUIRED, not optional. The firmware derives its ESP-NOW beacon period from it, and a node provisioned without it reports `fleet=1` and silently falls back to the 80 ms default. That is correct at three nodes and overruns the 50 Hz receive gate at nine -- measured on 2026-08-28, where an over-fast beacon discarded 60-90% of peer frames at random and wedged a transmit queue. Verified on hardware today: node 2 booted reporting `period=80ms (fleet=1, derived)` because nothing had ever set it. provision_conf.json is gitignored; it points at the credential file rather than containing it. Co-Authored-By: claude-flow <ruv@ruv.net> (cherry picked from commit b2bd7e8)
provision_node.py exits with the schema to create when no profile exists, which is better than shipping a template that can be half-filled and mistaken for real config. But the printed example carried a real network name and real credential paths. Uses 'thisismyssid' and shows the password file's actual contents, so a reader does not have to guess whether it holds JSON or a bare string. Co-Authored-By: claude-flow <ruv@ruv.net>
provision.py cached every provisionable attribute in a per-port JSON state file so a re-run could merge prior values. That list included 'password' and 'seed_token', so every successful run wrote the WiFi passphrase in cleartext to a file under the user's config dir -- defeating the point of keeping the credential in a file outside the repo, and leaving stale copies of retired passwords lying around after an SSID change. Secrets are now excluded from the merge list AND filtered again on write, so a credential cannot reach the disk by a route someone adds later. The cost is that a secret must be supplied on every run rather than merged from prior state; provision_node.py already does exactly that, reading them from files, and a direct provision.py call now fails loudly instead of silently reusing an old credential. Related upstream work: PR ruvnet#1760 makes the same state file owner-only, which is worth having, but still lists password and seed_token as mergeable -- so it protects a file that should not contain the secret at all. These compose: their permissions plus this exclusion. board_index.py additionally prefers esptool's 'BASE MAC' line over any MAC-like string in the output; the other lines are derived forms and picking one of those keys a board by an address it does not provision under. Co-Authored-By: claude-flow <ruv@ruv.net>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| print(" " + " ".join(shown[1:])) | ||
| if a.dry_run: | ||
| continue | ||
| r = subprocess.run(cmd, capture_output=True, text=True) |
| if a.no_auto_reset: | ||
| esp += ["--before", "no-reset"] | ||
| esp += ["write_flash", NVS_OFFSET, binp] | ||
| f = subprocess.run(esp, capture_output=True, text=True) |
Semgrep flags both calls as possible command injection. They are list-form subprocess.run with no shell=True, so argv goes straight to execve and there is no shell to inject into. The elements are a fixed flag sequence plus values from a local operator-owned config file; a hostile value can only become one bad argument to provision.py or esptool, not a second command. Suppressed with nosemgrep and the reasoning inline rather than restructuring working code around a false positive -- but stated explicitly so the next reader can check the claim instead of trusting the annotation. Co-Authored-By: claude-flow <ruv@ruv.net>
Replaces the nosemgrep suppressions from the previous commit with an actual fix. Suppressing a scanner finding in the one script that handles WiFi credentials and the OTA PSK is the wrong shape of answer, even when the reasoning is sound. The reasoning WAS sound -- these are list-form subprocess calls with no shell=True, so argv goes to execve and there is no shell to inject into. But that safety is a property of this call site, and the same values also become esptool arguments. Constraining them at the top means the safety no longer depends on every future caller remembering that. Validated: chip against the set of ESP targets, ssid against the 802.11 1-32 character limit, target_ip through ipaddress.ip_address(), edge_tier as an int in 0..2. Each exits with a specific message. The secondary benefit is the one that will actually be felt: a typo in provision_conf.json now fails immediately with "unsupported chip 'esp32c66'" rather than surfacing as a confusing esptool error partway through a nine-board run, with some boards provisioned and some not. Spot-checked that the validators reject shell metacharacters, traversal and empty values, and that every validated value is the one passed to the argv list rather than the raw config being re-read. Co-Authored-By: claude-flow <ruv@ruv.net>
ruvnet
reviewed
Sep 5, 2026
ruvnet
left a comment
ruvnet
left a comment
Owner
There was a problem hiding this comment.
Dream portfolio exact-head review (2026-09-05)
Frozen hypothesis: after provisioning or re-provisioning, password and seed_token must never enter the per-port JSON state, missing fresh secrets must fail loudly, and the supported provisioning paths must retain deterministic board identity and OTA behavior under targeted regression tests.
Reachable code now removes the two secret attributes from the merge list and filters them again on write. All six exact-head workflows pass, including CI, firmware, QEMU, security, and policy gates. However, this five-file change adds no targeted regression test for legacy state containing either secret, save/load round-trips, missing-secret failure, or file cleanup. The new wrapper also deliberately passes the WiFi password and OTA key through child-process argv; that residual is explicitly disclosed but is CWE-214 and remains observable to local process inspection.
The state-file direction is sound, and the argv limitation should not block a narrowly scoped state-migration fix, but the frozen state contract has not been executed. Add failure-injection tests that seed legacy secret-bearing state, verify scrubbed output, and exercise both direct and wrapper paths; retain the current hardware/field limitations.
INCONCLUSIVE
The suppression added earlier named `dangerous-subprocess-use-audit`. The rule reporting these two lines is `dangerous-subprocess-use-tainted-env-args`, so the annotation matched nothing and both findings were still uploaded to code scanning, which is what turns the Semgrep OSS check red. Both spellings of the id are listed: the registry reports the fully-qualified form and a local run reports the short one. The findings are false positives for command injection -- list-form subprocess.run with no shell=True, so argv goes to execve and a hostile value can only become one bad argument, never a second command. The claim that every non-literal element is range-checked was not quite true, though: `port` reached the esptool argv unvalidated. It is now constrained to the shape of a real device node, so the comment describes the code. Co-Authored-By: claude-flow <ruv@ruv.net> Claude-Session: https://claude.ai/code/session_0191PwxLFAChNxRr5sGAVTxH
Excluding secrets from MERGEABLE_ATTRS stops new state files carrying the passphrase, but does nothing about the ones already written. Those were only cleaned if the board happened to be provisioned again -- which never happens for a board that has been retired, moved, or handed to someone else, so the credential just sits there. Reading the file is the only moment we are certain one exists, so strip the secrets there, say which were removed, and rewrite the file immediately. Also correct the WiFi-trio error message, which said "no per-port state file" -- true before, and misleading in exactly the new case where the file is present but no longer carries credentials, sending the operator to look for the wrong problem. Co-Authored-By: claude-flow <ruv@ruv.net> Claude-Session: https://claude.ai/code/session_0191PwxLFAChNxRr5sGAVTxH
The review asked for the frozen contract to be executed rather than
asserted. It was worse than untested: three existing tests in
tests/test_provision_state.py still encoded the OLD contract and had
been failing on this branch since secrets left MERGEABLE_ATTRS.
CI never said so, because nothing runs firmware/esp32-csi-node/tests.
The repo's pytest calls cover archive/v1, python/ and
tests/performance, and the `Tests (3.x)` job marks every step
continue-on-error, so it cannot fail a PR at all. A change that put
the passphrase back into the state file would have gone in green.
* Update the three merge tests and the round-trip test to the new
contract, and pin that a prior secret is NOT merged back into
args -- without that, "not persisted" would be cosmetic and the
credential would keep flowing from disk into every flash.
* Add the failure-injection case: seed a legacy state file holding
password, seed_token and ota_psk, then assert the secrets are
hidden from the caller, removed from disk, named in the warning,
and that the run which relied on the cached passphrase now fails
the WiFi-trio check instead of flashing silently.
* Add a gating `Provisioning Tooling Tests` job. Stdlib unittest, no
dependencies, under a second.
Each new assertion was negative-controlled: reverting the scrub fails
four of them, and putting the secrets back into MERGEABLE_ATTRS fails
the merge test.
Co-Authored-By: claude-flow <ruv@ruv.net>
Claude-Session: https://claude.ai/code/session_0191PwxLFAChNxRr5sGAVTxH
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.
provision.py cached every provisionable attribute in a per-port JSON state file
so a re-run could merge prior values. That list included 'password' and
'seed_token', so every successful run wrote the WiFi passphrase in cleartext to
a file under the user's config dir -- defeating the point of keeping the
credential in a file outside the repo, and leaving stale copies of retired
passwords lying around after an SSID change.
Secrets are now excluded from the merge list AND filtered again on write, so a
credential cannot reach the disk by a route someone adds later. The cost is
that a secret must be supplied on every run rather than merged from prior
state; provision_node.py already does exactly that, reading them from files,
and a direct provision.py call now fails loudly instead of silently reusing an
old credential.
Related upstream work: PR #1760 makes the same state file owner-only, which is
worth having, but still lists password and seed_token as mergeable -- so it
protects a file that should not contain the secret at all. These compose: their
permissions plus this exclusion.
board_index.py additionally prefers esptool's 'BASE MAC' line over any MAC-like
string in the output; the other lines are derived forms and picking one of
those keys a board by an address it does not provision under.
Rebased onto current
main; one additive.gitignoreconflict resolved by keeping both sides. Scanned the diff for credential literals before pushing (none), and the three scripts byte-compile.