fix(pairing): give a user-run client the client half of the pairing handshake - #498
Conversation
Anchor commit for dig-node#403. Bumps the workspace version to 0.243.0 and opens the branch so the lane's state survives a session cap. Refs #403
…en ladder (#403) An ordinary OS user could not drive control.* against a dig-node running as a root system service: control_client::call_control had exactly one token source, the 0600 root:root master token (#501). The server side of the #280 handshake was complete; nothing implemented the client half. - paired_client: the token LADDER as a pure function over the two read outcomes (master when readable -> per-user paired token -> the master read's own remedy, verbatim), plus the per-user 0600 store, the refusal bound on client_name, and the poll bound taken from the server's expires_ms. - pair connect [--client-name NAME]: request over the OPEN method, print the compare-codes value, poll to a terminal state, persist on approval. No file mode is widened anywhere. The paired token cannot administer pairings and carries no chain authority. Co-Authored-By: Claude <noreply@anthropic.com>
|
IN PROGRESS — not the verdict. Independent correctness gate, fresh context, head Confirmed so far (reading only; suite running in my own worktree):
Two findings already established (details + |
# Conflicts: # Cargo.lock # Cargo.toml
loop-security — IN PROGRESS, NOT THE VERDICTAudited head: I verified the lane's claim rather than accepting it: CLEARED so farTrust direction — clean. Tier gate — fail-closed, and this PR does not weaken it. Ladder cannot fail open at the server. CORS is not relied on.
FINDING 1 — the per-user token store is created world-readable, then chmod'd (MEDIUM, non-blocking on its own but see the verdict)
The crate's own established discipline for exactly this class of file is three lines away in Why the copied shape is newly exploitable rather than pre-existing. Fix is small and matches the crate: Still open (next)Blast radius of a stolen/held paired token (the exact non-master |
MichaelTaylor3d
left a comment
There was a problem hiding this comment.
CHANGES-REQUIRED
Head reviewed: a7b60df07a86665e6680b8546237f74bf82c27e3 (resolved from the remote myself). I began at 8453bda; the head moved under me when main was merged in. I verified that is immaterial: git diff --stat origin/main...a7b60df is 9 files / 525 insertions / 6 deletions, identical to 8453bda, and paired_client.rs, pair.rs, control_client.rs and pairing.rs hash byte-identical across the two. Every finding below is anchored to a7b60df.
What I verified as CORRECT — do not re-derive these
- The ladder, all four combinations.
paired_client.rs:439-445.Ok(t) => Ok(t)returns beforepaired()is ever named, so rung 1 structurally cannot consult the store. The thunk claim is real, not decorative:a_readable_master_token_wins_without_consulting_the_paired_store(paired_client.rs:564) flips aCellinside the closure and asserts it stayedfalse— an implementation that read the file every time would pass amaster-winsassertion and fail this one. - The remedy survives unchanged in kind and text.
paired_client.rs:547asserts both; the kind matters becausecli::ExitCode::from_io_errormaps it. (Its CONTENT is finding 2 below — that is a different problem from degradation.) - The poll terminates, and the units are right.
expires_msis an ABSOLUTE epoch-ms timestamp (pairing.rs:167,created + PAIRING_TTL_MS; returned at:196), sonext_poll_step'snow_ms >= expires_msis dimensionally sound. This was the one that could have been silently fatal — had the server returned a remaining-duration, the firstpendingpoll would have compared ~1.7e12 against 300000 and reportedExpiredimmediately, and the verb would never have worked once. It does not.expiredandunknownare both terminal atpair.rs:159, and a missingexpires_msdegrades to0, i.e. fails CLOSED. client_nameis refused, not truncated, and mirrors rather than copies.validate_client_name(paired_client.rs:453) readscrate::pairing::MAX_CLIENT_NAMEdirectly; the wholepairing.rschange isconst->pub const. The test pins BOTH sides of the bound and additionally asserts the refusal does not echo a shortened form back.call_open, notcall_control, for both handshake steps.pair.rs:118andpair.rs:136;control_client.rs:77-83passestoken: None. No bootstrap deadlock.- Nothing widened a permission. The nine changed files do not include
packaging/, the systemd unit, orstate.rs. No chmod of any machine-wide path;restrict_fileis untouched. - No regression for a user who can read the master token — rung 1 is the pre-#403 path, byte-for-byte.
Test evidence I observed personally
Own worktree at C:/tmp/worktrees/gate-403-review, not the implementer's.
- Baseline
cargo test -p dig-node-service --lib->759 passed; 0 failed; 0 ignored; 0 filtered out. The zero-filtered count matters: a filter matching nothing also exits 0. - Revert-proof REPRODUCED. Replacing the fallback arm of
select_tokenwithErr(master_err) => Err(master_err)->758 passed; 1 failed, the single failure beingpaired_client::tests::a_denied_master_read_falls_back_to_the_paired_token. Exactly one test, and it is the one at the decision. Worktree restored from a copy afterwards;git statusclean apart from my own scratch files. (Worth recording: cargo's own exit was 101 while the harness line readexited with code 0— the count, not the status, is what I gated on.)
Findings
| # | severity | where |
|---|---|---|
| 1 | blocking | crates/dig-node-service/src/pair.rs:144,155,165 — corrupted whitespace in three user-facing strings |
| 2 | blocking | crates/dig-node-service/src/control.rs:553 — rung-3 remedy does not name dign pair connect |
| 3 | non-blocking | paired_client.rs:389 — store lands in a PID-keyed temp dir when $HOME is unwritable |
| 4 | non-blocking | paired_client.rs:419 — write-then-chmod window |
| 5 | non-blocking | control_client.rs:123 — master-tier verbs now return a worse error for a paired user |
Findings 3-5 are posted as notes and I will resolve them myself so they do not bar merge. Findings 1 and 2 stay open as the gating threads.
Neither blocking finding is a design objection. The shape is right, the security argument in the PR body holds up, and the tests are genuinely load-bearing rather than decorative — which is not the usual outcome here. What fails is coherence: the verb ships with three of its own user-facing sentences visibly broken, and the message that is supposed to route a user TO the verb still does not mention it.
(Recorded per contract: a same-identity --request-changes is HTTP 422 here, so this verdict is a comment review. The open inline threads on findings 1 and 2 are what bar the merge under required_conversation_resolution.)
loop-security — IN PROGRESS, NOT THE VERDICT (2/2)Head audited: FINDING 2 — the per-user token store can resolve into a WORLD-WRITABLE directory, and the writer follows a symlink into it (HIGH, BLOCKING, INTRODUCED-HERE)
So when the invoking user's
The module doc at Concrete exploit — deterministic, no race. Mallory is any unprivileged local account.
What that token is worth. The paired tier is everything in Second primitive from the same defect. Point the symlink at a file Alice owns — When the fallback actually fires (I am stating this as a precondition, not claiming it is the default): an account with no usable home — precisely the "dig-app Agent on a server" persona Why INTRODUCED-HERE and not pre-existing. The shape is not new — Suggested fix (small, and it closes FINDING 1 in the same move). Replace FINDING 3 — revocation works, but a stale store then masks the remedy (LOW, non-blocking, INTRODUCED-HERE)Revocation itself is clean: The residue: after a revoke the user's |
|
Correction to my verdict comment — line numbers, not substance. Several
Verdict is unchanged: CHANGES-REQUIRED, gating on the two open threads ( |
|
Two of my own corrected anchors were still off by three: the |
loop-security VERDICT: CHANGES-REQUIREDHead audited: 1. The reachability answer, enumeratedWho can reach the node's privileged
Verdict on the PR body's hypothesis. On the authorization plane it survives refutation: the set of principals is unchanged, and nothing gains MASTER tier. The change admits a new client program into an existing, operator-gated flow. I attacked the ladder, the trust direction and both wallet planes and could not construct a master-tier gain. Where it fails: the set is not exactly the intended one, because the new credential store adds P4 and P5. Neither reaches master tier — 2. FindingsBLOCKINGF2 — the token store can resolve into a world-writable directory and the writer follows a symlink into it. HIGH. INTRODUCED-HERE. Unix only.
F1 — the store is created world-readable, then chmod'd. MEDIUM. INTRODUCED-HERE.
One edit closes both: NON-BLOCKING
3. Areas checked and clear
4. What gatesF2 and F1 only. They share one three-line fix in I ran no build and no test — this verdict rests entirely on reading the diff and the surrounding code at For the orchestrator: this PR is DRAFT and was |
…create it exclusively Four gate findings from dig-node#498. B1: `paired_token_path` resolved through `state::legacy_state_dir`, whose chain runs `resolve_cache_dir` -> `private_fallback_dir` = `temp_dir()/DigNode-<PID>/cache` when the canonical dir is unwritable. On such a host the bearer token was written into a 1777 directory under a /proc-enumerable name. It now resolves from `dig_node_core::platform_user_base()` directly, so the temp fallback is structurally unreachable, and REFUSES when no per-user base exists rather than degrading to the cwd. B2: the store was `fs::write` + a later chmod, so it existed at the process umask (0644) for a window, and `write` follows symlinks -- a planted link disclosed the token or clobbered an arbitrary file as the victim, with no race. It is now created with `create_new` at mode 0600. B3: three user-facing strings in `pair connect` carried a ~26-space run from a lost line continuation, including the verb's SUCCESS message and both terminal failure paths. A fourth (the waiting banner) leaked its source indentation. All four are lifted into named functions so a guard test can assert the signature is gone. B4: the rung-3 remedy named only sudo verbs the unprivileged reader who sees it cannot run. It now names `dign pair connect` first, keeping the operator half and the platform-correctness that landed in #458. Co-Authored-By: Claude <noreply@anthropic.com>
fix lane — progress (head
|
| finding | state at e9783db |
|---|---|
| security F2 — store resolves into a world-writable dir, writer follows a symlink (HIGH) | fixed — paired_client.rs:53-99 resolves from platform_user_base() and REFUSES an unresolvable base; paired_client.rs:145-160 unlinks then create_new(true).mode(0o600). Tests the_store_resolves_from_the_per_user_base_and_never_the_temp_fallback (:355), an_unresolvable_per_user_base_refuses_rather_than_writing_to_the_cwd (:388), the_store_is_created_exclusively_and_never_follows_a_planted_symlink (:423) |
| security F1 — created world-readable then chmod'd (MEDIUM) | fixed by the same edit — ensure_dir_restricted + mode at open(2) time, no post-hoc chmod (paired_client.rs:142-160) |
review thread a — corrupted 26-space runs in three user-facing literals (pair.rs:144/155/165) |
fixed — the four strings are lifted to waiting_banner/paired_message/EXPIRED_BEFORE_APPROVAL/terminal_status_message (pair.rs:172-201) and asserted by the_user_facing_pair_strings_have_no_lost_line_continuation (pair.rs:275) |
review thread b — rung 3 remedy does not name dign pair connect (control_client.rs:59) |
fixed — remedy_for_unreadable_token unix branch now leads with the unprivileged step (control.rs:553-556), operator verbs kept, windows branch unchanged; asserted one-sidedly at control.rs:6417-6427 |
Next action: git merge origin/main (Cargo.toml → the pre-assigned 0.250.0, Cargo.lock from main then cargo update -w --offline), then cargo test -p dig-node-service --lib in the background and check the test COUNT, then reply + resolve both threads.
No production code changed by this lane so far.
Resolves the two conflicts from the concurrent release train: - Cargo.toml: only `[workspace.package].version` conflicted (0.243.0 on the branch vs 0.245.0 on main). Resolved to the pre-assigned 0.250.0 so the version gate reads against the merged base rather than a stale one; every other hunk of main's manifest is kept verbatim. - Cargo.lock: took origin/main's file wholesale, then `cargo update -w --offline`, which relocked exactly one package (dig-node-service 0.245.0 -> 0.250.0). No dependency line moved. No source conflicts: control.rs, control_cli.rs, SPEC.md and the dig-wallet sage files auto-merged. Co-Authored-By: Claude <noreply@anthropic.com>
…ce run
The guard against a lost `\` line continuation trimmed the start of every
line before checking, so it could only see the corruption in ONE of its two
forms. A multi-line literal without the trailing backslash emits the source's
indentation as a NEWLINE plus nine spaces, not as an interior run, and
`"a\n b"` prints just as raggedly as `"a b"`.
Proved by reverting only `paired_message`'s continuation: the old assertion
PASSED on the reverted string, so it was not load-bearing against that shape.
The tightened test fails on it ("prose must not be indented"), and passes once
the continuation is restored.
The banner keeps its one legitimate indent -- exactly four spaces on the
copy-pasteable `sudo dign pair approve <id>` line -- and is now the only string
allowed any, checked against that exact width rather than waved through.
No production code changed.
Co-Authored-By: Claude <noreply@anthropic.com>
The previous fix commit left `the_store_resolves_from_the_per_user_base_and_ never_the_temp_fallback` unformatted, which the fmt gate fails on. Whitespace only; no assertion changed. Co-Authored-By: Claude <noreply@anthropic.com>
…0.250.0 Main moved to 0.246.0 while this branch was being fixed. Same two conflicts as the previous merge and the same resolution: Cargo.toml keeps only the assigned 0.250.0 (every other hunk of main verbatim), Cargo.lock taken from origin/main then `cargo update -w --offline`, which relocked one package. No source conflicts; SPEC.md auto-merged. Co-Authored-By: Claude <noreply@anthropic.com>
fix lane — DONE, ready for the security re-gatehead Every blocking finding, verified against the tree rather than against a comment
The one real defect this lane found: the whitespace guard was half-blindProving thread a's test load-bearing by reverting only its fix showed it PASSED on the reverted string. A lost Tightened at Also committed at MergeMerged Re-gate scopeProduction code changed by this lane: none. The two commits are a test-only tightening ( PR remains DRAFT and unmerged. |
|
orchestrator bc2a88 / loop-security scoped re-gate started on 5e5bcb5 at 2026-09-02T17:31Z |
# Conflicts: # Cargo.lock # Cargo.toml
loop-security — scoped re-gate STARTEDIN PROGRESS — not the verdict. Head resolved from the remote myself: Scope of this round (scoped re-gate, not a full re-audit):
Working in my own detached worktree at |
loop-security — IN PROGRESS, not the verdict (1/2)Head audited: Item 3 — the merge commit is clean. CONFIRMED.
Zero source lines. The merge introduces no code. Version, re-read from Item 2 — review threads. CONFIRMED RESOLVED, and the fixes are real.GraphQL (not the UI): I checked the two BLOCKING ones as fixes rather than as resolutions:
Nit, not a finding: Item 4 — path traversal on the store path. CLEAR.
The token is also never printed: on success |
loop-security — IN PROGRESS, not the verdict (2/2): F2 REVERT-PROOF, executedHead: The problem with revert-proofing F2 on this host, and how I got around it
So I ran the primitive on a real Linux kernel instead (WSL2 Ubuntu, kernel 6.6.87, rustc 1.98.0), on a native ext4 path ( The guard is load-bearing and the fix is real. Under the reverted body the bearer token is written straight THROUGH the attacker's symlink into the victim file, and the store path is still a symlink afterwards. Under the fixed body the victim is untouched, the store is a regular file the invoking user created, and it is The reverted run also confirms the original finding's second primitive in passing:
F1 — created world-readable then chmodded. FIXED, and structurally so.There is no F2 — the temp-dir resolution half. FIXED structurally; Windows-side revert-proof still running.
|
loop-security — IN PROGRESS, not the verdict (3/3): scope item 4, what is NEWHead: Note which test is NOT in those 11: CLEARED
Bonus the fix earned that was not asked forRemoving NON-BLOCKING observations (follow-up tickets, not merge gates)N1 — the whitespace guard is scoped to an enumeration, and the identical defect survives one module over.
This is PRE-EXISTING — byte-identical at main N2 — This does not weaken the fix: N3 — N4 — the refusal predicate rejects only |
loop-security — VERDICT: PASSHead audited: Primary checkout was read-only (git objects only). All builds and probes ran in my own detached worktree The blocking set — every item verified against the tree, not against the fix table
Review threads: F2 revert-proof — executed, both halves, with countsThe crate's own symlink test is (a) The symlink / exclusive-create half — WSL2 Ubuntu, kernel 6.6.87, rustc 1.98.0, native ext4 Under the reverted body the bearer token lands inside the victim file, through the attacker's symlink. The reverted line's (b) The refusal half — on this host, in-crate.
(c) The resolver half is fixed in the CODE but its guard is vacuous — see N1, which I proved rather than argued. What is NEW on the pairing/store path — cleared
NON-BLOCKING — follow-up tickets, explicitly NOT gatingN1 — This does not make the code vulnerable, which is why it does not gate. The fix is structural: N2 — the same lost-continuation defect thread a gated on survives one module over, in a user-facing string. N3 — N4 — the refusal predicate rejects only the empty path and For the orchestrator — merge mechanics, not security
A rebase moves the head, and this verdict is against |
|
orchestrator
Chain log: |
Co-Authored-By: Claude <noreply@anthropic.com>
Closes #403
The gap
control_client::call_controlhad exactly ONE token source:control::load_token_readonly(), themaster control token. On a
.debinstall #501 deliberately made that file0600 root:rootinside a0700 root:rootdirectory, so an ordinary OS user driving the CLI against a root system service wasdenied on every token-gated
control.*verb. The server side of the #280 handshake was alreadycomplete (
pairing.rs: OPENpairing.request, master-gatedcontrol.pairing.approve, OPENpairing.poll), andpair.rsimplemented the OPERATOR side only. Nothing implemented the clienthalf, and no client anywhere stored or presented a paired token.
What landed
dign pair connect [--client-name NAME]— the client half, and the onlypairverb needing notoken. Requests over the OPEN method (
call_open, nevercall_control), prints the compare-codesvalue plus the operator's exact command, polls to a terminal state bounded by the server's own
expires_ms, and on approval persists the scoped token to the invoking user's own state dir(
<user_state_dir>/client-token,0600on Unix).The token ladder in
call_control— master when readable -> this user's paired token -> themaster read's error VERBATIM (kind and remedy text both preserved; that message landed in #458 and
is platform-correct).
--client-nameis bounded bypairing::MAX_CLIENT_NAMEitself (madepubso the client cannotdrift from the server) and an over-long name is REFUSED, never shortened.
No permission is widened
Not one mode changes.
build-deb.sh, the systemd unit hardening,/var/lib/dig-nodeand the mastertoken are untouched; no group is added and nothing is chmod'd. The paired token is strictly less
powerful than the master token: it cannot drive
control.pairing.*and cannot drivecontrol.chiaPeers.add/.remove, so pairing a CLI user confers neither pairing administration norchain authority over the wallet replica. Revocation is unchanged.
Why the ladder is a pure function
select_token(master: io::Result<String>, paired: impl FnOnce() -> Option<String>). Theunprivileged case cannot be reproduced by a test process that is privileged, and a unit test cannot
drop privileges — so the read OUTCOMES are arguments, the pattern #458 established here.
pairedisa THUNK rather than an
Optionspecifically so that "rung 1 never consults the store" isOBSERVABLE: with an
Optionthe file has already been read before the decision, and no test couldtell a correct implementation from one that reads it every time.
Blast radius
gitnexus's index is stale for this repo and returns a false-safeimpactedCount: 0, so this wasmeasured by grep + direct read and is stated as such.
call_controlhas 14 call sites across 5files (
control_cli.rs,control_client.rs,entrypoint.rs,pair.rs,peers.rs); its signatureand error contract are unchanged, and the change is purely additive at rung 2 — a caller that could
authenticate before still authenticates the same way, by the same token, first.
Windows does not regress. The installer grants the interactive user read on the master token, so
rung 1 succeeds there and the ladder never engages.
restrict_fileis already a no-op on Windows(the parent dir's inheritable ACL governs), and the
0600assertion is#[cfg(unix)].Evidence
cargo test -p dig-node-service --lib-> 759 passed, 0 failed, 0 filtered out.cargo clippy -p dig-node-service --lib --all-targets-> clean, exit 0.Revert-proof, run for real: reverting ONLY the fallback arm of
select_token(toErr(e) => Err(e)) makesa_denied_master_read_falls_back_to_the_paired_tokenFAIL with thePermissionDenied remedy as its panic payload — the test is load-bearing on the decision, not on
plumbing below it. The other seven still pass, which is the point: the fixture varies one rung.
An earlier run also caught a real defect via an existing guard —
no_help_text_exposes_an_internal_ticket_numberfailed on(#403).in the new clap help. Fixed in8453bda. (Note for reviewers: cargo reported exit 0 on that failing run; the test COUNT is thesignal here, not the exit status.)
Tests added:
a_denied_master_read_falls_back_to_the_paired_token,with_no_paired_token_the_master_remedy_is_returned_unchanged,a_readable_master_token_wins_without_consulting_the_paired_store,the_per_user_store_round_trips_and_is_owner_only,a_blank_store_is_not_a_token,an_over_long_client_name_is_refused_not_truncated(pinned from both sides: at-bound passes, oneover fails),
the_default_client_name_is_within_the_bound,polling_waits_until_the_servers_deadline_and_then_stops(time pinned to an explicitNOW, not thewall clock), plus two
pair connectparse assertions in the existing entrypoint pair test.Docs + deps
SPEC.mdgains §7.11a (the client verb, the four MUSTs of the flow, the ladder with itsrung-1-must-not-consult rule, and what pairing does NOT grant), and §7.3a now states that the
client-side token is NOT in the machine-wide state dir.
README.mddoes not documentdign pair, sonothing there to update.
§2.4b: all 13 external
dig-*/chia-*declarations indig-node-servicewere checked againstindex.crates.ioand every caret range already resolves to the latest published version(
dig-node-control-interface0.30.0,dig-chainsource-interface0.3.2,dig-rpc-protocol0.10.2,dig-constants0.13.0, …); thechia-*set is uniformly on the 0.36 line. No drift to fix.Version already at 0.243.0 from the lane-opening commit; not re-bumped.