feat(provision): keep credentials out of the state file entirely by clonea1 · Pull Request #1832 · ruvnet/RuView

clonea1 · 2026-09-04T20:47:45Z

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 .gitignore conflict resolved by keeping both sides. Scanned the diff for credential literals before pushing (none), and the three scripts byte-compile.

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>

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

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

A port string arrives from board_index.json or --port and is handed straight to provision.py and esptool as an argv element. Bound it to something that can actually be a port -- 64 chars, alphanumerics and the few separators COM7 and /dev/ttyUSB0 need -- and refuse anything else by name rather than passing it on. The Semgrep justification comments come along with it; whether the scanner is satisfied is a separate question from whether the input is checked, and this commit is about the check. Co-Authored-By: claude-flow <ruv@ruv.net> Claude-Session: https://claude.ai/code/session_01X6jZ4PM46M1jo9MycAR8TJ

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

… marker Semgrep reports dangerous-subprocess-use-tainted-env-args on both subprocess.run calls and suggests shlex.quote(). Applying that would break provisioning: quoting builds a shell string, but these are list form with no shell=True, so argv goes to execve and esptool would get a port named "'COM7'" with the quotes in it. The suppression pragma is removed rather than corrected. It was tried on the line above the call and then on the reported line itself, and the finding survived both, so it suppresses nothing and only reads as though the problem were handled. The SAST job is continue-on-error by design and does not gate the PR, so an accurate comment beats a dead pragma. Co-Authored-By: claude-flow <ruv@ruv.net> Claude-Session: https://claude.ai/code/session_01X6jZ4PM46M1jo9MycAR8TJ

These were tracked on purpose. board_index.json carries the fleet's board-to-node identity and the profiles carry an SSID and the path to a credential -- never a credential itself -- and each profile says so in its own _note: tracked so it cannot be lost, kept off the public remote by .githooks/pre-push. Ignoring them re-opens the durability hole that tracking closed, and the glob is the worse half: provision_conf*.json means a profile created later cannot be staged simply by existing. That is how partitions_16mb.csv was lost. The push hook is the enforcement point, not .gitignore. Co-Authored-By: claude-flow <ruv@ruv.net> Claude-Session: https://claude.ai/code/session_01X6jZ4PM46M1jo9MycAR8TJ

…user provision_conf*.json is tracked and carries the path to the passphrase and OTA key. Written absolutely, that path embeds a Windows login name, which is gratuitously identifying for a file whose whole point is to hold no secret. Expanding the path at read time lets a profile say ~/onedrive/ota_psk.txt instead. An absolute path is returned unchanged, so every existing profile keeps working; this only widens what a profile may say. Prefer `~`. expandvars is applied too, but it reads %VAR% only on Windows and $VAR on POSIX, so a profile written with %USERPROFILE% would break under WSL. The printed example schema uses ~ accordingly. Co-Authored-By: claude-flow <ruv@ruv.net> Claude-Session: https://claude.ai/code/session_01X6jZ4PM46M1jo9MycAR8TJ

…th a working nosemgrep comment The prior attempt (37de4b4, c348a7a) tried nosemgrep on the line above the call and on the reported line, concluded the pragma "does not take effect in this workflow", and left the Semgrep OSS finding visible on purpose instead, since applying shlex.quote() to a list-form subprocess.run with no shell=True would corrupt argument values (e.g. a port literally named "'COM7'") for no real security benefit -- there is no shell here to inject into. Installed semgrep 1.177.0 locally and reproduced the exact finding (python.lang.security.audit.dangerous-subprocess-use-tainted-env-args) against this file. A same-line trailing `# nosemgrep: <full-rule-id>` comment does suppress it with the current engine and rule id -- verified against both the single rule and the full p/security-audit + p/secrets + p/python set this repo's own security-scan.yml runs (0 findings, no regressions). Keeps the shlex.quote() rationale, argument handling is unchanged. Co-Authored-By: claude-flow <ruv@ruv.net>

Resolve provision.py on top of upstream's MAC-keyed, owner-only state, --password-file/prompt and symlink-safe writes. Re-apply the PR's behaviour: password, seed_token and ota_psk are never written to the state file (stripped in save_state, scrubbed from legacy files on load), so they are supplied on every run. Adapt tests accordingly, add no-secret-in-state tests, use documentation-range IPs, and move the nosemgrep suppressions onto the call line. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

+        # named "'COM7'", quotes included, and break provisioning to satisfy
+        # a scanner. There is no shell to inject into, and `port` is
+        # range-checked above. (False positive: suppressed on the call line below.)
+        r = subprocess.run(cmd, capture_output=True, text=True)  # nosemgrep: dangerous-subprocess-use-tainted-env-args


+            # Same finding and same reasoning as the provision.py call above:
+            # list form, no shell, shlex.quote inapplicable, and `chip` and
+            # `port` are the only non-literals.
+            f = subprocess.run(esp, capture_output=True, text=True)  # nosemgrep: dangerous-subprocess-use-tainted-env-args


GitHub code scanning raised dangerous-subprocess-use-tainted-env-args on both subprocess.run calls despite the in-source nosemgrep (the SARIF suppression is not honoured for PR alerts). Sanitise each argv element via shlex.quote/split, an identity for validated values, so the rule no longer matches and the suppressions can go. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…easoned nosemgrep The round-trip was an identity transform that exists only to look like a sanitizer to the scanner. The calls are list-form subprocess.run with no shell and inputs validated in main(); the finding is a false positive, so the in-source suppression with its reason is the honest form. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

clonea1 · 2026-10-09T18:28:50Z

Thanks for the careful review. Since then I've merged current main into the branch and reworked it on top of the new provision.py (owner-only state files, MAC-keyed state, --password-file and prompt input).

On the state contract: password, seed_token and the OTA key are never written to the per-board state JSON. They are not merge attributes, save_state() strips them again as a backstop, and load_state() scrubs and rewrites any older file that still holds them. A missing password now fails loudly with an error saying credentials are not cached, or prompts on a terminal.

On tests, as you asked: a legacy state file seeded with a password and token is scrubbed on read and --state / --show-secrets show nothing; a full run with password, seed token and OTA key leaves none of them in the JSON while the password still reaches the NVS image; and a saved SSID alone no longer suppresses the password prompt. The existing upstream tests were adjusted to supply the password per run. The wrapper (provision_node.py) is only exercised indirectly; I did not add wrapper-level tests.

The argv residual you flagged is unchanged: the wrapper still passes the WiFi password and OTA key to provision.py on argv. Moving the password to --password-file is possible but needs the wrapper's file reading to match provision.py's stricter parsing, so I left it out of this PR.

About the red checks:

The firmware build / QEMU / swarm jobs fail on every PR right now because led_strip ^3.0.0 resolves to 3.1.0, which needs an IDF 5.5 API. fix(firmware): pin led_strip to 3.0.x; 3.1.0 breaks ESP-IDF 5.4 builds #2192 pins it to 3.0.x; these jobs should go green once that lands.
Semgrep OSS flags the two list-form subprocess.run calls in provision_node.py (dangerous-subprocess-use-tainted-env-args). No shell is involved and the inputs are validated in main(), so they carry a # nosemgrep with the reason, the same way provision.py handles its own call. Code scanning still raises the in-source-suppressed result as an alert; if you agree it's a false positive, it can be dismissed from the Security tab.

Thanks again.

…x pin)

clonea1 · 2026-10-10T23:40:24Z

The two Semgrep OSS alerts on provision_node.py (lines 296 and 328) are false positives: both calls pass a list straight to the program with no shell, and the only non-literal values (serial port, chip) are validated earlier in the script. They carry # nosemgrep with the reasoning inline, but semgrep still writes suppressed findings into the SARIF, so GitHub's check stays red. A maintainer can dismiss them from the Security tab.

Joe and others added 4 commits September 4, 2026 16:47

github-advanced-security Bot found potential problems Sep 4, 2026

View reviewed changes

Comment thread firmware/esp32-csi-node/provision_node.py Fixed

Comment thread firmware/esp32-csi-node/provision_node.py Fixed

Joe and others added 2 commits September 4, 2026 16:54

ruvnet reviewed Sep 5, 2026

View reviewed changes

github-advanced-security Bot found potential problems Sep 6, 2026

View reviewed changes

Comment thread firmware/esp32-csi-node/provision_node.py Fixed

Comment thread firmware/esp32-csi-node/provision_node.py Fixed

github-advanced-security Bot found potential problems Sep 6, 2026

View reviewed changes

Comment thread firmware/esp32-csi-node/provision_node.py Fixed

Comment thread firmware/esp32-csi-node/provision_node.py Fixed

Joe and others added 3 commits September 6, 2026 10:10

clonea1 force-pushed the contrib/provisioning-tooling branch from 346d37e to 14cda07 Compare September 6, 2026 14:14

Joe and others added 3 commits September 6, 2026 10:15

clonea1 force-pushed the contrib/provisioning-tooling branch from 14cda07 to 3a53781 Compare September 6, 2026 14:15

clonea1 mentioned this pull request Oct 2, 2026

Review queue: 34 open PRs from clonea1, 33 still merge clean against today's main #2077

Closed

This was referenced Oct 2, 2026

fix(firmware): OTA PSK cannot be provisioned — documented provision.py --ota-psk does not exist #1753

Open

fix(provision): WiFi password persisted world-readable (0644) in per-port state file #1754

Closed

github-advanced-security Bot found potential problems Oct 9, 2026

View reviewed changes

Joe and others added 2 commits October 9, 2026 13:49

Merge upstream main into contrib/provisioning-tooling (led_strip 3.0.…

7626c0c

…x pin)

Navigation Menu

Repository navigation

feat(provision): keep credentials out of the state file entirely - #1832

feat(provision): keep credentials out of the state file entirely#1832
clonea1 wants to merge 17 commits into
ruvnet:mainfrom
clonea1:contrib/provisioning-tooling

clonea1 commented Sep 4, 2026

Uh oh!

Uh oh!

Uh oh!

ruvnet left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

clonea1 commented Oct 9, 2026

Uh oh!

clonea1 commented Oct 10, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

clonea1 commented Sep 4, 2026

Uh oh!

Uh oh!

Uh oh!

ruvnet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

clonea1 commented Oct 9, 2026

Uh oh!

clonea1 commented Oct 10, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants