fix: witness key redaction and vulns by Sunny6889 · Pull Request #221 · tronprotocol/tron-deployment

Sunny6889 · 2026-08-20T05:51:35Z

What does this PR do?

1. fix(render) — a witness key on its own line was never redacted.
RedactWitnessLine matches per line, and in the shape the shipped template uses

localwitness = [
  <key>
]

only the opening line starts with the key name. The line carrying the key passed through untouched into plan --diff, config diff, verify-config and the MCP drift tool. Adds RedactWitnessLines, which stays inside an unterminated array; comparison still uses raw lines so a rotated key still shows as a change.

2. chore(private-net) — the demo witness key controlled a funded account.
It derives TPL66VK2gCXNCD7EJg9pgJRfqcRazjhUZY, holding 26,568 TRX on Nile. Replaced with a pair whose address exists on neither chain. The template uses the address in three places that must agree — genesis allocation, genesis witness list, localwitness — so all three move together.

3. build — five reachable advisories in govulncheck.
GO-2026-6218, -6090, -5972, -5856, -5026. Needs go1.25.13 and x/net v0.55.0. The Makefile pinned 1.25.9 while go.mod asked for 1.25.11; both now say 1.25.13, hashes refreshed from go.dev/dl/?mode=json.

4. fix(bootstrap) — bootstrap could not run over SSH.
It calls apt-get, yum, useradd, sh, none of which are whitelisted — deliberately, since trond exec passes whichever name the caller gives. So it worked locally and failed on the first apt-get remotely, untested. Adds a provisioning mode scoped to those four commands; SetProvisioning is off the target.Target interface and bootstrap is its only caller, so no lifecycle path or trond exec can reach it.

5. fix(state) — most state writers held no lock.
Only apply and the MCP lifecycle tool took it. The other ten read the node list, worked, then wrote it all back with nothing in between, so a concurrent trond drops one of the two updates — 200 times out of 200 in a test. The lock is now taken where the read happens (nodeContext acquires in resolveNodeContext, releases in Close; network commands and MCP heal take it directly). Nothing nests an acquire: apply keeps its own lock and doesn't use nodeContext, and network upgrade only spawns children. Save also shared a fixed <state>.tmp between concurrent writers — now unique per call, 0600.

Why are these changes required?

The redaction hole leaks a live SR signing key to stdout, to JSON, and off the machine through the MCP tool. The template teaches the leaking shape.
A key published in a repository must not control anything; this one controls 26k TRX on a public testnet.
make vuln already fails on develop, so the gate cannot distinguish a new advisory from the backlog.
Bootstrap prepares remote hosts and has never worked against one.
Lost state updates are silent — the registry forgets a node and nothing errors.

RedactWitnessLine decides per line, and only the opening line of localwitness = [ <key> ] starts with the key name. The element line — the one that actually carries the key — was returned unchanged, so it reached every surface that emits config lines: plan --diff, config diff, verify-config, and the MCP drift tool, whose output is handed to a third-party model provider. This is the shape the shipped private-network template uses, so a witness deployed from it leaked its key on any of those paths. Add RedactWitnessLines, which walks a whole config at once and stays inside an unterminated localwitness array, and move the four diff surfaces onto it. Comparison still runs over the raw lines, so a rotated key is still reported as a change.

The demo key shipped in the private-network template derives TPL66VK2gCXNCD7EJg9pgJRfqcRazjhUZY, which holds funds on the public chains — 26,568 TRX on Nile and 1 TRX on mainnet at the time of writing. A key published in a repository must not control anything, and anyone following the template was pointed at an account that does. Replace it with a freshly generated pair whose address does not exist on either chain. The template uses the address in three places that have to agree or the private chain will not produce blocks — the genesis allocation, the genesis witness list, and the localwitness key — so all three move together. old TPL66VK2gCXNCD7EJg9pgJRfqcRazjhUZY new TM4yToQ1njkcFwi3ADY5x6dbdfNekU3rVi The pair was generated with tools/txgen and checked by deriving the address back from the private key.

`make vuln` reported five advisories reachable from this code: GO-2026-6218 net/url quadratic resolvePath GO-2026-6090 crypto/tls GO-2026-5972 encoding/asn1 recursion depth GO-2026-5856 crypto/tls ECH privacy leak GO-2026-5026 x/net/idna ASCII-only punycode labels The stdlib ones need go1.25.13; the last also needs x/net v0.55.0. The Makefile pinned 1.25.9 while go.mod asked for 1.25.11, so the bootstrap and the toolchain directive named different versions; both now say 1.25.13. The four tarball hashes in bootstrap-go.sh come from go.dev/dl/?mode=json, as the comment there requires. `make vuln` now reports no vulnerabilities.

The SSH whitelist deliberately dropped apt-get, yum and the shell — a wide list would hand the SSH user's authority to anyone who can run `trond exec`, which passes whichever command name the caller gives. The comment on allowedCommands says package managers "can be re-allowed scoped to that one command path if needed", but bootstrap kept calling them, so `trond bootstrap` failed on its first apt-get against any SSH target. It only ever worked locally, where LocalTarget.Exec does not consult the whitelist, and installDocker/installJDK have no test. Add that scoping: a provisioning set (apt-get, yum, useradd, sh) that only applies to a target SSHTarget.SetProvisioning has switched on, and only bootstrap calls it. SetProvisioning is not part of target.Target, so no lifecycle path and no `trond exec` can reach it. Anything outside both sets — curl and wget among them — is still refused.

Only apply and the MCP lifecycle tool took the state lock. Every other writer — start, stop, restart, upgrade, rollback, heal, remove and the three network commands — read the node list, worked for as long as the operation took, then wrote the whole list back, with no lock in between. Two trond processes therefore both start from the same list and the second save drops whatever the first added. A test that runs the unlocked cycle twice concurrently loses an update 200 times out of 200, so this is what happens rather than a narrow race. Take the lock where the read happens: - nodeContext acquires it in resolveNodeContext and releases it in Close, which covers the eight commands built on it. apply does not use nodeContext and keeps its own lock, and `network upgrade` only spawns children, so nothing nests an acquire inside another. - the three network commands and the MCP heal tool take it directly, matching what the lifecycle tool already did. Save also wrote through a fixed "<state>.tmp". Concurrent writers share that name, overwrite each other's half-written file and then rename whatever is there, which defeats the point of writing to a temp file at all. Use a unique name per call and chmod it to 0600, since CreateTemp makes the file 0600 already but that is not part of its contract.

warku123

Thanks for picking these up — the direction looks right, and the parts I could verify offline checked out (Go 1.25.13 SHA256s match go.dev byte-for-byte, x/net v0.55.0 exists, the demo key rotation leaves zero residual references, and I found no lock re-entrancy deadlock: heal reuses the same nodeContext, apply takes no lock, network upgrade spawns subprocesses).

Two items I'd like to see addressed before merge, left as inline comments: the MCP live-conf resource still returns the witness key unredacted, and the state lock is now held by read-only / unbounded commands (logs -f, wait, ...), which can wedge every other trond process on the host. A few minor nits I'll follow up with separately.

warku123

One more question on the redaction logic — inline below.

warku123 · 2026-08-20T06:41:09Z

+			out[i] = lineIndent(line) + redactedWitnessAssignment
+			// An assignment that opens an array without closing it on
+			// the same line continues on the lines that follow.
+			if !strings.Contains(trimmed, "]") {


Question on this heuristic: strings.Contains(trimmed, "]") is comment-blind, so two shapes slip through:

localwitness = [ # comment with ] — the ] inside the comment makes us skip array mode, and the real key on the following lines leaks;

an element line that itself ends with ] followed by more elements mis-parses the state.

The shipped templates don't produce either shape, so nothing is on fire today — but for a redaction function I'd rather not rely on line-shape heuristics at all. Since the repo already depends on github.com/gurkankaymak/hocon (used by internal/dbfork), would it be simpler to parse the config, extract the actual localwitness array values, and replace those literal strings with <REDACTED> in the raw text? That's formatting-agnostic (single-line, multi-line, weird comments all immune), with the current line scan kept as a fallback when parsing fails.

Alternatively, stripping trailing comments before the ] check (and adding test rows for both shapes) would close the gap too. Curious what you think — was the hand-rolled scan a deliberate choice to avoid the parser here?

warku123

One more minor, anchored on the line that activates the provisioning path.

Four things the review caught. The MCP conf resource handed out the live config with no redaction. It is the one surface whose full text leaves the machine by design — the client, and from there a model provider — and a witness node's config carries its signing key. Redact at the resource, not in the shared helper: the drift tool reads the same helper and needs the raw text, since a redacted side would report every witness node as drifted. RedactWitnessLines decided array membership from line shapes, and two shapes defeated it. A `]` inside the comment on the opening line ended the array before it began and let the key through; an element that closed the array left the scanner inside it, so every following line came out as <REDACTED> and the diff said nothing. Find the values by parsing instead — the config parser is already a dependency for dbfork — and replace those literal strings wherever they appear, which no formatting can dodge. The scan stays as the fallback for text that does not parse, hardened to strip comments before the bracket checks. Every nodeContext command was holding the exclusive state lock until Close, including ones that never write: logs, wait, exec, files, health, diagnose, verify-config. A `trond logs -f` left running would block every other trond process on the host, and Acquire had no deadline, so the wait was indefinite and unexplained. Split the resolve path — readers take nothing, writers keep the lock across load-modify-save — and bound the wait at 30s with an error that says which process to look for. Enabling provisioning mode made a discarded error live: ensureServiceUser ignored useradd's result, which was invisible while the whitelist refused the call. Check it, tolerating only "already exists" so bootstrap stays re-runnable.

Little-Peony added 5 commits August 20, 2026 13:39

warku123 reviewed Aug 20, 2026

View reviewed changes

Comment thread internal/mcp/tools_drift.go

Comment thread cmd/resolve.go Outdated

warku123 reviewed Aug 20, 2026

View reviewed changes

Comment thread cmd/bootstrap.go

warku123 approved these changes Aug 20, 2026

View reviewed changes

kuny0707 merged commit 3d32d1e into tronprotocol:develop Aug 20, 2026
11 checks passed

Sunny6889 deleted the fix/witness-key-redaction-and-vulns branch August 20, 2026 07:15

Navigation Menu

Uh oh!

fix: witness key redaction and vulns - #221

fix: witness key redaction and vulns#221
kuny0707 merged 6 commits into
tronprotocol:developfrom
Sunny6889:fix/witness-key-redaction-and-vulns

Sunny6889 commented Aug 20, 2026 •

edited

Loading

Uh oh!

warku123 left a comment

Uh oh!

Uh oh!

Uh oh!

warku123 left a comment

Uh oh!

warku123 Aug 20, 2026

Uh oh!

Sunny6889 Aug 20, 2026

Uh oh!

warku123 left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

Sunny6889 commented Aug 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

warku123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

warku123 left a comment

Choose a reason for hiding this comment

Uh oh!

warku123 Aug 20, 2026

Choose a reason for hiding this comment

Uh oh!

Sunny6889 Aug 20, 2026

Choose a reason for hiding this comment

Uh oh!

warku123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Sunny6889 commented Aug 20, 2026 •

edited

Loading