fix(control): #1888 node endpoints are changeable from a running dashboard, behind a typed confirmation and a reachability probe - #1920
Conversation
…s, behind a typed confirmation and a probe The remote Monero and Tari node endpoints leave the never-committable security perimeter for the confirm-gated tier, on the operator's 2026-09-06 ruling. On an appliance the old refusal was not a defence, it was a dead end: there is no host shell, so "edit config.json and run apply" left the setting unchangeable for the life of the machine (#786/#1821). Four keys move: MONERO_NODE_HOST, MONERO_RPC_PORT, MONERO_ZMQ_PORT and TARI_GRPC_ADDRESS. MONERO_NODE_USERNAME and MONERO_NODE_PASSWORD do NOT — those are secrets, not address identity — and neither do the binds. Allowlisting alone would have shipped a SILENT FREE-COMMIT: 43-'s default-deny regex is the union of the editable and confirm sets, so a listed key only gets PAST default-deny, while the typed-confirm branch fires on describe_change's flag column. 39-:152 was an msg-only arm (INFO) for the three Monero keys and TARI_GRPC_ADDRESS had no arm at all, so both halves are here. The compensating control is the reachability probe: before a node-endpoint commit is accepted, the approval gate runs preflight_remote_nodes on the STAGED config, host-side, and refuses an endpoint that does not answer. It fires only when an endpoint key actually changed and only after the typed confirmation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y7txeRNDQmj5b4Wsx33mCF
…lity probe A probe nothing invokes is decoration. tests/stack/test-control-add-only-ssrf.sh is the only harness that drives the real gate — run_sourced cannot, because it leaves $0 as the caller and the gate shells out to "$0" apply --dry-run --porcelain — so the three cases go through the actual request spool. One variable moves per case, on a remote-Tari baseline: no token (refused, asks for the confirmation), token + an endpoint nothing answers on (refused BY THE PROBE), token + a port that does answer (committed). Case 2 is the teeth: without the probe that same commit applies. Case 3 is what stops a probe that refuses everything from reading as a pass. The listener binds a kernel-chosen port, because two lanes may run this suite at once and a fixed port would collide, and the fixture carries its own control — the test dials its own port before trusting any verdict built on it. The file passes 400 lines, so it earns a docs/dev/file-budget.tsv ceiling. Only that ONE row is added: --generate reprints the whole budget, which here would have LOWERED fourteen other files' ceilings and reddened every un-rebased branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y7txeRNDQmj5b4Wsx33mCF
a60edf9 to
9714cda
Compare
Rebased onto the current
|
| Gate | Result | Its own control |
|---|---|---|
make lint-pithead-parity |
PASS — "the committed artifact is exactly what 54 slice(s) build" | --self-test ran first: refuses an empty slice, a slice with no trailing newline, a directory named *.sh |
make lint-file-budget |
PASS — every over-target file has a ceiling, none grew, the tsv is monotonic | self-test fired, including the empty-candidate-scan and zero-tracked-files enumeration guards |
shfmt -i 4 -d at the gate's pin (3.13.1) |
clean on all five shell surfaces | flags are the gate's own, not mine |
A first attempt at parity was a NO-OP and is not being reported as evidence: ./scripts/build-pithead.sh
returned rc 126 (the file is 0664, not executable), so the build never ran and the unchanged sha proved
nothing. The row above is make lint-pithead-parity, which invokes it as bash … and self-tests first.
NOT re-run at this head: the full stack suite, the dashboard suite and patch-coverage — all three were
green at the pre-rebase head (tests/stack/run.sh 3444/0; make test-dashboard 2574 pass, 97.59%;
test-patch-coverage 5 lines, 0 missing, and it NAMED the file so it was not the vacuous shape). The
rebase is patch-identical, so those results carry on the diff; they do not carry on the merged tree,
which is what CI at this head measures. Tier 3 remains the stated gap — the appliance round trip needs
the KVM battery.
This PR still needs a non-author PASS. Asking for one at 9714cda4.
PR 1920 — head 9714cda — PASS (non-author)Head re-derived, not taken from the prompt: My main line of attack (item 1) failed on a fact I checked last and should have checked first. It is 1. The perimeter / SSRF — ruling IMPLEMENTED; the primitive is real but confers nothingImplemented, not asserted. The widening is carried by four coordinated edits, not one allowlist What I went after. It is also an open/closed oracle, two ways. Set Why this is not a finding. The in-scope adversary here is a compromised dashboard — the repo I checked the obvious remedy before recommending it, and am not recommending it: 2. identity / password stay NEVER-APPROVE — VERIFIED at the code pathChecked at head against all three bash lists and both Python maps, with firing controls (my first
3. The drift test — NOT a tautology on re-derivationThe two sides come from genuinely different files and different derivations:
Neither side is sourced from the thing it tests. The stated failure — a confirm key with no probe — I probed the one escape I could construct: add the key to the bash confirm set only, leaving the Residual, worth stating: the derivation is keyed on the 4. Head named — 9714cdaRe-derived independently at the top of this file. Follow-ups (none merge-blocking)
What I did not run, and whyBurst window 3 is CI-only while the bench is claimed by I did not re-run the gates the author reports at this head (parity, file-budget, shfmt) and did not |
…tors cannot use lint-operator-strings refuses an issue/PR number in a describe_change msg=; operator-facing text drops the reference and comments keep it. Line-neutral: 39-describe-change.sh stays at 428/428. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y7txeRNDQmj5b4Wsx33mCF
⛔ The PASS above is VOID — the head moved to
|
|
Non-author review of the hunk I was asked for ( What I checked and found correct
Finding: the probe can pass without probing, and the gate cannot tell
but So a commit that changes only I want to be accurate about severity rather than inflate it: on a local or off chain the committed
Either fix is small, and I do not have a preference strong enough to override the author's:
The test for whichever you pick needs to be a fixture that would go green today: assert a What I did not doRan nothing. No pytest, no I also did not review |
The delta re-pass PASSed, CI is green — and the second reviewer's finding is RIGHT about the code and WRONG about the reach. Head does not move.Two things landed on this PR at once. Taking them in the order that decides the merge. 1. The dashboard lane's finding (issuecomment-5558071985) — confirmed at source, then refuted at the reachI confirmed their mechanism by reading both sites myself, not by taking their word.
Their conclusion does not survive, and the half that dies is the reach. For the silent-skip branch to be entered, a node-endpoint key's rendered value must CHANGE while
On a non-remote chain all four endpoint keys are constants. The one variable in them is The Their severity read is separately correct and I verified it independently: So: mechanism accurate, conclusion not. I would rather say which half died than restate the What is actually wrong here, stated properlyThe gate is correct today by an accident of the renderer, not by construction. Its contract — That is worth fixing. It is not worth fixing by adding a runtime refusal for a case nothing can Where my confidence ends, plainly: the refutation above is a STATIC derivation — the renderer's Why the head does not move for itpull 1920 carries two non-author verdicts naming 2. The delta re-pass — PASS, and its condition is now met
The verdict below was conditional on three checks that had never reported on this branch —
The reviewer's new finding, which I confirmed with a firing controlThey report I did not check it their way. I enumerated all thirteen targets of the Filed rather than fixed — MERGE-READY at PR 1920 — head
|
| head | Lint per-surface |
cause, read from the log |
|---|---|---|
a60edf98 (pre-rebase) |
failure | operator strings: issue/PR number in a … describe_change msg= — 9793: flag=CONFIRM msg="… #1888: confirm-gated …", make: *** [Makefile:147: lint-operator-strings] Error 1 |
9714cda4 (the PASSed head) |
failure | byte-identical cause, same line 9793, same Makefile:147 |
ea2548b2 (head) |
success | — |
Your reading is confirmed on both counts: the rebase did not introduce it, and the PR was red from its first push. The mechanism is confirmed too — the battery recorded at 9714cda4 lists lint-pithead-parity, lint-file-budget and shfmt, and make lint (Makefile:66) reaches ten targets that job does not, lint-operator-strings among them. A hand-picked subset cannot be reported as "the gates".
CI at ea2548b2 — 19 green, 0 failures, 3 still running
total_count=22. completed/success × 17, completed/skipped × 2 (both CVE-sweep legs), in_progress × 3:
Shell tests (shellcheck + pithead suite)— this ismake lint-sh(shfmt at the pin) and the pithead suite, i.e. §2's open question andtest-config.sh's describe_change rows.Dashboard tests (pytest + coverage)—test_env_key_perimeter.py,test_control_service.py, the patch-coverage gate.Dashboard image (Docker test stage)
This is the part that needs saying plainly: all three were cancelled at BOTH prior heads (concurrency-cancelled by the next push). ea2548b2 is the first head on this PR where they will run to completion. So it is not merely that CI is unfinished here — the shell suite, the dashboard suite and the image stage have never reported on this branch at all. The PASS below is conditional on them.
Findings
Non-blocking, and new — lint-pithead-parity is not in CI at all.
grep -rl 'pithead-parity' .github/workflows/ -> 0 files. Firing control on the same sweep: grep -rl 'lint-file-budget' .github/workflows/ -> 1 file, so the needle and the root both work. Lint per-surface (ci.yml:513) runs ten targets and parity is not one; Shell tests runs lint-sh and the suites; no workflow runs make lint whole.
So the gate that proves the shipped artifact is what the sources build is local-only, on a repo that commits both. This cycle is the demonstration: your ./scripts/build-pithead.sh returned rc 126 without running, the unchanged sha read as parity, and nothing downstream would have caught it. Worth a one-line addition to the Lint per-surface step. Closest existing issues are #1900 (stop committing the artifact) and #1463 (semantic cross-slice ordering); neither is this. I did not file it — it is outside the delta and outside my paths.
Carried unchanged from the earlier verdict, and correctly kept out of this commit: the bare-TCP Tari probe leg vs the docs/dashboard.md claim, the host-vs-bridge namespace for p2pool, and endpoint_re's trailing empty alternative. Their own PR from the tip is the right call.
What I did not run, and why
The burst-window CI-only rule is in force — fleet.sh claims shows a live claim by the appliance lane on the bench box, 1-minute load 2.86 throughout. So no make lint-pithead-parity, no make test*, no shellcheck/shfmt, no docker. §3 is a direct bash invocation of one text-concatenation script against an exported tree, which adds no measurable load and is not a suite run; load was 2.86 before and after. Everything else here is git, gh and sed.
I did not re-run the earlier verdict's own work, and Tier 3 (appliance round trip / KVM battery) remains a stated gap that this pass does not clear.
Verdict
PASS on the delta at ea2548b2d0bf77c47f6354f53af7ca0992b04535, non-author. The commit does exactly one thing, does it line-neutrally at a zero-headroom ceiling, keeps the artifact in parity with its sources under an instrument I made fail on demand, and does not damage the operator's sentence.
Conditional, and the condition is real, not ceremonial: the three checks named in §5 have never reported on this branch. PASS becomes unconditional when they land green; if any reds, this verdict is void and the head must move again. Base is develop -> MERGE-READY for the seat, not for me.
Follow-up (1) is now WIDER than I filed it, and #1889 is the reason — re-derived at source todayI recorded three non-blocking follow-ups on this PR and called (1) the one worth fixing before the RC: That still holds. What is new is that the SAME paragraph's mechanism half goes stale in the other direction the moment #1889's branch merges, and that branch's diff never touches this file. Re-derived, not relayedAt
This paragraph as written says "a TCP connect on each port, and for Monero's ZMQ port a protocol greeting". After #1889 that understates Monero's RPC port, which becomes a protocol check too — while the Tari conclusion stays overstated. One sentence, wrong in two directions at once, and only after a merge that does not touch the file it is in. Why I am NOT fixing it in this PRThe sentence exists only on this branch — The replacement text, so the follow-up is mechanical rather than a re-derivation
ORDER MATTERS: that text is correct only AFTER #1889 lands. Applied while #1889 is still open it would claim a Monero RPC protocol check this tree does not make. If the two merge in the other order, the Monero clause has to be held back and only the Tari sentence added. Follow-ups (2) — the probe running in the host namespace against bridge-networked p2pool — and (3) — |
Follow-up (2) is filed as #1946, with the false pass measured rather than arguedAuthor note. The head does not move — this is bookkeeping on a recorded follow-up, not a change to this PR. The reviewer's non-blocking follow-up (2) — the probe runs in the host namespace while p2pool is bridge-networked, so a Driving the real Two things I added to the reviewer's statement:
Scope of the measurement, stated so nobody reads it wider than it is: the Tari It is filed rather than fixed here because Follow-up (3) — |
Closes #1888
What changed
The remote Monero and Tari node endpoints leave the never-committable security perimeter for the
confirm-gated tier, on the operator's 2026-09-06 ruling. Four keys move:
MONERO_NODE_HOST,MONERO_RPC_PORT,MONERO_ZMQ_PORT,TARI_GRPC_ADDRESSMONERO_NODE_USERNAME,MONERO_NODE_PASSWORD(RPC login secrets, not address identity), and every bind (MONERO_RPC_BIND,MONERO_ZMQ_BIND,TARI_GRPC_BIND— a listen address is not an endpoint)MONERO_ZMQ_PORTis the fourth key and the grant did not name it. I judge it in, deliberately: itsits in the same block as the other three, the probe already checks it, and shipping host + rpc_port
committable while the ZMQ port stayed host-only would leave an operator able to repoint half a
remote Monero node — an incoherent half-move.
The threat model, and what answers it
It was already written, at the site —
dashboard/tests/service/test_env_key_perimeter.py, the entrythis PR removes:
That is still true and still the risk. What changed is that refusing them was not a defence on an
appliance, it was a dead end: there is no host shell there, so "edit
config.jsonand run./pithead apply" made the setting unchangeable for the life of the machine (#786/#1821). Theoperator ruled the trade.
What is traded for the perimeter entry is not the typed token — that is UX friction, as
42-control-policy-and-host-checks.shsays in its own words, since a compromised dashboard that canset a field can also fill a confirm box. It is the host-side reachability probe: before a
node-endpoint commit is accepted, the approval gate runs
preflight_remote_nodeson the stagedconfig, host-side, and refuses an endpoint that does not answer. A dashboard cannot silently park a
chain on a node that is not there. The probe fires only when an endpoint key actually changed (an
unrelated commit is never blocked by a node that happens to be down) and only after the typed
confirmation (an unconfirmed attempt never pays the dial timeouts).
Unchanged, and stated because the issue asks: the control channel's own authentication, the approval
identity and password (
pithead-media-config's never-approve set), and the audit log. Nothing hereloosens those.
Why both halves had to ship together
Allowlisting alone would have been a silent free-commit of four security-perimeter keys, the
exact opposite of the ruling:
43-:92builds its default-deny regex from the union of the editable and confirm sets, solisting a key only gets it past default-deny.
describe_change.39-:152was an existing msg-only arm (so:INFO) for the three Monero keys, andTARI_GRPC_ADDRESShad no arm at all.So
42-+39-are one change, not two.Files, and the shared/serialized ones
lib/pithead/42-control-policy-and-host-checks.sh— the four keys joinCONTROL_DASHBOARD_CONFIRM_KEYS;a new
CONTROL_NODE_ENDPOINT_KEYSnames the probe subset once rather than adding a fourthhand-kept copy of the key names; the editable-set prose no longer claims node endpoints are host-only.
lib/pithead/39-describe-change.sh— the arm becomesCONFIRMand folds inTARI_GRPC_ADDRESS;the message names the chain, old → new, and what the machine will now trust. 428/428, line-neutral.
lib/pithead/43-control-approval-and-preview.sh— the probe, plus_control_host_remedy(): all fiverefusals that named
./pithead applynow say, on an appliance, what an appliance operator canactually do (issue item 3). 375 → 399 against the 400 target — one line of headroom, so the next
edit there should extract rather than add.
dashboard/mining_dashboard/service/control_service.py— the mirroredCONFIRM_ENV_KEY_PATHSentries, and
_confirm_paths(cfg)now offers a chain's endpoint only while that chain is on aremote node. 403 → 413 against a 414 ceiling.
dashboard/tests/service/test_env_key_perimeter.py,tests/stack/test-config.sh,tests/stack/test-control-add-only-ssrf.sh— tests, below.pithead— regenerated; parity gate green.SECURITY.md(disclosed,_shared.pathsclass 2 by operator ruling, DECISIONS item 15): nodeendpoints move from the never-committable enumeration to the confirm-gated one, with the probe.
The doc had to move with the code or the repo would document a perimeter it no longer has. Not a
rewrite — two sentences.
docs/(disclosed,_shared.paths):docs/dashboard.md(the confirm-gated field list, theperimeter paragraph, and a new paragraph on the probe and the appliance remedy),
docs/configuration.md(both endpoint rows),
docs/appliance.md(a remote node's address is not a one-time answer).Grants used, all per-item and expiring on this merge:
39-and42-fromcli(
answers/cli-20260906T071418Z.md,answers/cli-20260906T034940Z.md),tests/stack/test-config.shfrom
tests(answers/tests-20260906T071420Z.md). No.lane-overridewas needed or used.What was RUN
Tier 1,
tests/stack/test-config.sh(in place, at the existing CONFIRM-tier rows; the six newlines are paid for line-for-line inside the file, which is at its 753 ceiling):
MONERO_NODE_HOSTand
TARI_GRPC_ADDRESSareCONFIRM, the preview names old → new, and — the row that makes the setable to say no — a remote node's RPC password is not confirm-gated.
Tier 1 black-box,
tests/stack/test-control-add-only-ssrf.sh(mine outright; the only harnessthat drives the real gate, since
run_sourcedcannot — it leaves$0as the caller, and the gateshells out to
"$0" apply --dry-run --porcelain). Three cases on a remote-Tari baseline, onevariable each: no token → refused, asks for the confirmation; token + an endpoint nothing answers on
→ refused by the probe; token + a port that does answer → committed. Case 2 is the teeth —
without the probe that same commit applies — and case 3 is what stops a probe that refuses
everything from reading as a pass. The listener binds a kernel-chosen port (two lanes may run this
suite concurrently) and the fixture carries its own control: the test dials its own port first.
Dashboard tests — two new ones in
test_env_key_perimeter.py(no budget row): the endpoint keysare confirm-gated in both copies and on the probe list, and
_confirm_pathsoffers a chain'sendpoint only while that chain is remote (checked on a mixed config, so a rule keyed on "any
chain is remote" would not pass).
Controls, all fired. Four seeded mutations, each proved applied by a sha256 move and each
reverted to a byte-identical file:
TARI_GRPC_ADDRESSfrom the builtpithead'sCONTROL_NODE_ENDPOINT_KEYStest_node_endpoint_keys_are_confirm_gated_and_probedMONERO_NODE_HOSTfromCONFIRM_ENV_KEY_PATHS_confirm_pathstest_confirm_pathsignore the config_confirm_pathstestMONERO_NODE_HOSTback intoNEVER_COMMITTABLE_ENV_KEYSA first version of the drift test was a tautology and is fixed. It iterated the endpoint list and
asserted each key was confirm-gated — structurally blind to the failure its own comment claimed to
guard, which is a confirm key with no probe behind it. It now derives "is a node endpoint" from the
config-path shape (
.remote.inCONFIRM_ENV_KEY_PATHS) and compares sets, so the source isindependent of the list being checked.
Gates:
make lint-file-budget,make lint-pithead-parity,make lint-docs-voice,make lint-py,make lint-md— all green, each with its own self-test firing.shfmt -i 4 -dclean on all threeslices.
tests/stack/run.shin full: 3444 passed, 0 failed (a run of the same tree before theblack-box block gave 3435 — the +9 is exactly that block's assertions).
make test-dashboard:2574 passed, 97.59% total.
make test-patch-coverage: 5 changed lines, 0 missing, 100% — and itnames the file it measured, so not the vacuous-pass shape (#1000).
make test-frontend: 579 pass,0 fail.
lint-file-budgetalso produced its OTHER answer here: it FAILED on the test file at 448lines with no ceiling before the row was added, and passed after. The lane guard was shown live in this worktree by a near-miss
control: a one-byte change to
lib/pithead/40-apply-and-render.sh, staged alone, was refused.What was NOT run, and what is assumed
it and it needs the KVM battery, which is the appliance lane's and is inside the image freeze.
The appliance-facing half that is proven here is tier 1: the refusal text on
PITHEAD_APPLIANCE=1. The wording itself is a judgement call, not a measurement.make test-frontendandmake test-patch-coverage— see the checks on this PR.covered at unit level by Node changes are accepted without reaching the node: probe remote monerod and Tari endpoints before the wizard and the config view let the user proceed #1889's
reachable AND greeting -> rc 0, not re-derived here; case 3above exercises the accept path on the Tari leg only.
describe_changenow flagsthese keys
CONFIRM, and40-apply-and-render.sh:172treatsCONFIRMlikeDESTon the host —so a node-endpoint change from a shell now prints as a warning rather than an info row. The y/N
confirmation it folds into already existed for every change.
For the dashboard lane
Their build requirement was to gate the Tari endpoint row on
TARI_MODE != off. I have gated it onthe config's own
tari.mode, offering the row only when the chain isremote— which isstrictly narrower (it also hides the row on a
localchain, where the endpoint is stack-derived andediting it would do nothing) and needs no
.envvariable, so it does not depend on #1905 landing anddoes not need an edit when
offarrives with #1855. The Python hunk iscontrol_service.pyandtest_env_key_perimeter.pyonly; nothing in42-,43-or their open PRs (#1880/#1894/#1905).Follow-up filed, not fixed here
#1897 —
monero.remotehas no auth keys, so the probe is unauthenticated and a node requiring RPCauth is refused as
auth, whiledocs/configuration.mdtells operators to setmonero.node_usernamefor a remote node. Milestonedv2 - appliance; not a blocker for this.🤖 Generated with Claude Code
https://claude.ai/code/session_01Y7txeRNDQmj5b4Wsx33mCF