Skip to content

fix(security): gate the wallet transport and make the no-mint rule structural - #452

Draft
MichaelTaylor3d wants to merge 1 commit into
mainfrom
loop/batch-seckey
Draft

fix(security): gate the wallet transport and make the no-mint rule structural#452
MichaelTaylor3d wants to merge 1 commit into
mainfrom
loop/batch-seckey

Conversation

@MichaelTaylor3d

@MichaelTaylor3d MichaelTaylor3d commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

DRAFT — the orchestrator gates. This batch expects the full security gate given the custody content.

Six key-custody, pairing and authorization defects. One branch, one squashed commit.

Closes #345 · Closes #343 · Closes #257 · Closes #255 · Closes #346 · Closes #355

Per-ticket detail, including the revert-proofs, is commented on each ticket. This body carries the
blast radius, what each fix makes structurally impossible versus merely checked, and the gaps.


What each fix makes IMPOSSIBLE, versus merely checked

# Structurally impossible now Still only checked
257 Building a router into WalletBackend::dispatch without an authorization gate — build_router takes Arc<dyn WalletCallGate> as a required parameter and there is one handler behind POST /:method. An allow-everything policy is #[cfg(test)]-only; the crate ships only DenyAll. Which methods the gate answers "yes" for — that is wallet_authz's table, unchanged.
345 Reaching the mint or the legacy delete from a failed read — LegacySeed::ConfirmedAbsent is constructed in exactly one place, from Presence::Absent. The delete needs LegacySeed::Found(_) in the pattern. Nothing material.
255 The overlay silently outliving its reason (a test fails once the contract adopts the name) and the overlay narrowing the contract's set. That setUpstream is master tier at all — that is a dig-node-side overlay until the contract adopts it (below).
346 Rendering a clip without its mark — render_untrusted returns ONE String containing the marker; there is no companion bool to drop, which was the dig-app CRITICAL. Refusing over-long input at ingest removes the ingest truncation entirely. That every future display site calls the helper. Three call sites converted; a fourth would need review.
343 A protection summary omitting the derived-key caveat — the caveat is appended in ONE place (seed_protection_summary is split out), so an added tier arm cannot forget it. That the derived key is protected. It is not. See the deliberate non-fix below.
355 A privilege branch that asserts nothing — both branches of all three tests now assert, and the new pure-rule test is privilege-independent. See the verification gap below.

Blast radius checked

.gitnexus is stale (the registered index points at the primary checkout, not this worktree),
and impact on a stale index returns a false-safe impactedCount: 0. So this was done by grep +
direct read
, and that is stated rather than implied.

Symbols whose signature or contract changed, and every call site found and updated:

  • dig_wallet::sage::transport::{build_router, build_cors_router, serve_mtls, serve_http, serve_dual}
    — all gained a gate parameter. Callers: wallet_mtls::spawn (production), and 6 in-crate tests.
    serve_dual has no production call site (loopback_bind_guard.rs says so). Breaking for
    dig-wallet's public API
    → minor bump on a 0.x.
  • wallet_mtls::spawn — one call site, server.rs:2188.
  • machine_key::read_legacy — private, one caller (mint_or_migrate, new). Return type changed
    Option<_>LegacySeed.
  • machine_key::protection_summary — public, one consumer (dig-node-core/src/lib.rs:4460,
    dign stdout). Output string CHANGED (caveat appended); no parser consumes it.
  • control::requires_master_token — behaviour widened by one method. Consumers:
    wallet_authz::master_tier_control_equivalent, server.rs control gate. The existing lockstep
    test caught the widening immediately, which is the check working.
  • pairing::request — now refuses client_name > 64 chars. A behaviour change on an OPEN
    method
    ; a client sending a longer label gets INVALID_PARAMS instead of a silent clip.
  • pair::format_list — display only.

New dependency: unicode-width = "0.2" on dig-node-service, already in Cargo.lock
transitively. Needed to charge the display budget on rendered width rather than code points.

Evidence

suite result
dig-node-service --lib 664 passed / 0 failed (662 before)
dig-node-core --lib machine_key 24 passed / 0 failed (20 before — the ticket's stated baseline)
dig-wallet --lib sage::transport 12 passed / 0 failed

Full suites on the FINAL tree, each run alone:

crate result
dig-node-core --lib 1027 passed / 0 failed
dig-node-service --lib 664 passed / 0 failed
dig-wallet --lib 736 passed / 0 failed / 1 ignored

Counts read from the test result: line, not from ok, and every filtered run matched a non-zero
number of tests.

One caveat worth stating: a --workspace run launched while a previous cargo process still held the
shared target/ reported error: test failed, -p dig-node-core. Re-run alone it is 1027/0. The
per-crate numbers above are the ones taken without a concurrent cargo, which is the discipline
dig-node#345 asks for.

Revert-proofs — each fix reverted alone, from committed state, via file copies (never
git checkout <path>):

mutation result
neuter the transport gate 9 passed; 3 failed — all three #257 tests
Err(_) => Ok(LegacySeed::ConfirmedAbsent) (the ticket's own .ok() exploit) 22 passed; 2 failed
delete the master-tier overlay 662 passed; 2 failed
drop render_untrusted from the pending prompt 3 passed; 2 failed

A FALSE GREEN I shipped, found, and fixed — please check this one hardest

My first a_client_name_cannot_forge_an_extra_line_in_the_approval_prompt passed with the fix
reverted.
It asserted a line count and the absence of the forged quotation, and {:?} quoting
alone satisfies both — Debug renders a newline as the two characters \n, which neither adds a
line nor leaves a quote unescaped. It pinned the QUOTING and said nothing about neutralisation.

The discriminator Debug cannot supply is the visible U+FFFD, so the test now asserts that and
asserts no escaped-but-surviving \n. Separately, the clip test read only the TOKEN list while the
mutation hit the PENDING list — two different format! arms — so it now exercises both.

It was found only by running the revert-proof. Both were re-measured after the fix.

Two things NOT done, deliberately, and why

  1. sec: the derived BLS peer key is at rest UNSEALED, so sealing the machine seed does not make peer_id copy-resistant #343: node.key is not sealed. It is written by dig_tls::NodeCert::load_or_generate in
    another repo and read back by path by a third crate (dig_peer_protocol::load_ssl_cert), so
    sealing it is a release-first cascade across dig-tls + dig-peer-protocol, not a single-writer
    change this lane can make. peer_id must also not change while fixing it. The ticket's second
    option — state the gap plainly — is what landed, in protection_summary and SPEC §18.25a
    (including the recovery hazard: any sealing MUST use the SAME device key, or a device-key loss
    bricks the identity instead of being recoverable).
  2. SECURITY: control.config.setUpstream is ordinary-tier, persists an attacker-chosen RPC upstream, and survives pairing.revoke #255: the contract still classifies setUpstream as ordinary. The durable fix is in
    dig-node-control-interface, another lane's repo. The overlay here is a single declared,
    strictly-widening bridge with a test that FAILS once the contract adopts the name, so it cannot
    quietly become a second opinion. Filed as
    sec: control.config.setUpstream belongs on the master tier -- its effect outlives the token AND names a principal dig-node-control-interface#40 — the one issue this lane
    opens (six closed, one opened). Adopting it there is what DELETES the overlay here.

§2.4b deferred: this PR does not sweep dig-*/chia-* deps to latest. The batch is
security-scoped across three crates and a dep cascade would dwarf it; flagging rather than silently
skipping.

Verification gaps, stated

§908

Untouched. Nothing here signs on the user's behalf, and no fix moves a user spend key toward the
node. #257 restricts what the node's wallet transport will serve; it grants nothing.

…ructural

Six key-custody, pairing and authorization defects, one branch.

#257 — the Sage-parity mTLS wallet transport dispatched the whole surface,
custody and master-tier included, on possession of the shared client cert alone.
`build_router` now REQUIRES an `Arc<dyn WalletCallGate>`, there is one handler
behind `POST /:method`, and it authorizes before the body is interpreted. The
only gate the transport crate ships is `DenyAll`. `dig-node-service` supplies
`NodeWalletGate`, delegating to the same `wallet_authz::authorize` the HTTP and
`/ws` planes call, so a third plane cannot form its own opinion.

#255 — `control.config.setUpstream` was ordinary-tier, persisted an
attacker-chosen RPC upstream, and survived `pairing.revoke`. Moved to master
tier via a single declared, strictly-widening, self-retiring overlay rather than
a restated string match; the refined rule (outlives the token AND confers
authority on a principal) is applied to the whole control surface, with
`cache.setCap` and `log.setLevel` judged and deliberately left ordinary. The
upstream is now validated as a well-formed URL with no userinfo, cleartext
confined to loopback.

#345 — the no-mint rule was carried by a `?` in `read_legacy`; swapping it for
`.ok()` minted over a present-but-locked seed and then deleted it. `read_legacy`
now returns a three-valued `LegacySeed` whose `ConfirmedAbsent` is constructed
only from a determined absence, and `load_or_create` matches with both arms
named.

#346 — `client_name` reached the operator's approval prompt verbatim with an
unmarked truncation. Over-long names are now REFUSED at ingest; the render goes
through the new `untrusted_text`, which marks clips in-band, charges the budget
on rendered width, and neutralises control, Cf and bidi characters visibly. The
stored value stays byte-verbatim.

#343 — the derived BLS peer key is at rest UNSEALED, so sealing the seed does not
make `peer_id` copy-resistant. Sealing it is a dig-tls/dig-peer-protocol cascade,
so the CLAIM is corrected instead: every protection summary now names the
uncovered derived key, in one place.

#355 — the token-squat guard asserted nothing under root and printed ok. Both
root-skipped guards now assert the complementary observable — ownership, which
only root can manipulate — so every branch can fail.

SPEC.md gains §7.12's structural-gate obligation and §18.25a-d.

Closes #345
Closes #343
Closes #257
Closes #255
Closes #346
Closes #355

Co-Authored-By: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment