diff --git a/Cargo.lock b/Cargo.lock index 461762e4..2eba9957 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3031,7 +3031,7 @@ dependencies = [ [[package]] name = "dig-node-service" -version = "0.191.0" +version = "0.195.0" dependencies = [ "async-trait", "axum", @@ -3080,6 +3080,7 @@ dependencies = [ "tower-http", "tracing", "tracing-subscriber", + "unicode-width 0.2.2", "url", "windows-service", "windows-sys 0.61.2", diff --git a/Cargo.toml b/Cargo.toml index 173dc22a..37dc3503 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -32,7 +32,7 @@ edition = "2021" # the ROOT manifest (`[workspace.package].version`), so it MUST be set here for a # release to fire (§3.6). The library crates (dig-node-core/dig-runtime/dig-wallet) # keep their own independent versions — only the released binary tracks the workspace version. -version = "0.191.0" +version = "0.195.0" # Release hardening, matching digstore: keep integer-overflow checks ON in release. # The node parses untrusted serialized input and does offset/length arithmetic over diff --git a/SPEC.md b/SPEC.md index a131dfbb..581d1c87 100644 --- a/SPEC.md +++ b/SPEC.md @@ -2229,6 +2229,23 @@ These methods are NEVER relayed upstream — a signing request must never leave authorized call is served locally (or, until the wallet surface is served on a given transport, returns a catalogued error — it is never proxied to the public gateway). +**The gate binds EVERY transport into the wallet handler set, and the binding MUST be structural +(dig-node#257).** The tier is a property of the CAPABILITY, never of the transport a caller reached +it through. The Sage-parity mTLS listener authenticates with a shared client certificate whose DER +must equal the server's own; that authenticates the transport and MUST NOT be treated as +authorizing a capability. A node MUST NOT serve any route into the wallet handler set without first +obtaining an authorization decision for the requested method name — including for a method name the +node does not implement, so a future method cannot arrive ungated. + +This MUST be enforced by construction rather than by a per-route check: the router is built with an +authorization gate as a REQUIRED parameter, there is exactly one handler behind the method route, +and the only gate the transport crate itself offers denies everything. A per-route test set cannot +see a route nobody wrote a test for, which is how this listener came to serve custody, spends and +master-tier peer mutations on certificate possession alone. + +A refused call MUST be answered `401` and MUST NOT reach the handler, so a refused spend has not +been built or broadcast. The refusal MUST be distinguishable from "no such method". + **Retired namespaces (`wallet.*`, `auth.*`) MUST be refused outright.** Node-side USER custody and its unlock-auth gate were removed by dig_ecosystem#1701, superseded by the #1500 ratification: the node holds no user spend key. No method exists under either prefix, and a node MUST classify the whole prefix as @@ -6179,6 +6196,92 @@ this host cannot open, the node MUST NOT make a recovery promise: per dig-keysto §17.5b the envelope records a hardware *class* and carries no device identity, so the same error is returned for a blob copied off its machine (recoverable) and for the original machine with its trusted component wiped (permanent). The node MAY state that condition; it MUST NOT resolve it. +### 18.25a. What sealing the seed does NOT cover: the derived peer key (dig-node#343) + +Sealing the seed (§18.25) protects the ability to **re-derive** the node's identity. It does NOT +protect the **derived** material, and a surface MUST NOT imply that it does. + +`dig_tls::NodeCert::load_or_generate` persists the derived BLS/TLS leaf key **UNSEALED** at +`/peer-net/identity/node.key`, because the dig-gossip pool listener loads it from disk +BY PATH (`dig_peer_protocol::load_ssl_cert`) and holds no key material of its own at that point. + +Since `peer_id = SHA-256(TLS SPKI DER)`, possession of that one file IS possession of the node's +network identity for every purpose the network cares about — dialing as it, serving as it, and any +authorization keyed on it — **without touching either half of the sealed pair**. The +partial-exfiltration boundary §16.4 and §18.25 describe therefore holds for the SEED and does not +extend to the key peers authenticate. + +- Any surface reporting the machine key's protection tier **MUST name this gap in the same + sentence**, so a reader cannot take a copy-resistance claim about the seed as a claim about the + node's network identity. The node satisfies this by appending a fixed caveat to every protection + summary, in one place, so a later tier cannot be added without it. +- The node MUST NOT change `peer_id` in the course of closing this gap. A changed `peer_id` + silently orphans every peer holding the old one, which is a worse outcome than the exposure. +- Sealing `node.key` is the preferred end state and MUST use the **same device key** as the seed. + A separate device key would make the derived key unrecoverable after a device-key loss without + also making the seed unrecoverable, and dig-keystore `SPEC.md` §17.5b establishes that + `HardwareUnwrapFailed` cannot distinguish a copied blob from a wiped device — so an + independently-sealed derived key turns a recoverable state into a bricked node identity. +- Until then the gap is DOCUMENTED, not implied. An honest stated gap is correctable; an unstated + one is a shipped claim that is false about the artifact at risk. + +### 18.25b. Master tier is authority that outlives the token (dig-node#255) + +`dig-node-control-interface` states the rule as *"the effect outlives the token that invoked it"*. +That is necessary and not sufficient, and a node MUST apply the refined rule: a capability is +**master tier** when its effect both **outlives the token** AND **confers authority on a +principal** — installs someone the node will thereafter believe, obey, or speak to. + +- `control.chiaPeers.add` / `.remove` — installs a peer believed WITHOUT corroboration. Master. +- `control.config.setUpstream` — persists a caller-chosen third party that every method this node + does not implement is FORWARDED to, read on next start, and untouched by `pairing.revoke`. + Master. The node ships with no upstream precisely so an unimplemented method answers a truthful + local `-32601`; pointing it at an attacker-controlled URL makes that surface answerable by the + attacker, and the escalation delegates — after the call the caller no longer needs the token. +- `control.cache.setCap`, `control.log.setLevel` — persist and survive revocation, but move a + local resource budget or local verbosity. They name no principal and confer no authority. + ORDINARY, deliberately: promoting them would break paired clients for nothing gained. + +A node MUST resolve the tier by CALLING the contract's predicate, never by restating it as a string +match. Where the node enforces master tier AHEAD of the contract, the additional names MUST be a +single declared list that only ever WIDENS the contract's set, and a test MUST fail once the +contract adopts a name from it, so the two statements of one rule cannot drift. + +**A persisted upstream MUST be a well-formed `http(s)` URL.** The node MUST reject a value with no +scheme it speaks, an empty host, whitespace in the host, or userinfo (`user@host` reads as one host +and resolves to another). Cleartext `http://` MUST be confined to loopback. + +### 18.25c. Attacker-supplied text in an operator prompt (dig-node#346) + +`pairing.request` is OPEN and unauthenticated, and the `client_name` it carries is composed into +the sentence an operator reads before granting a control token. It is therefore an input to a +privileged decision and MUST be treated as hostile. + +- **The node MUST NOT silently truncate it.** An unmarked truncation is a forgery the node + performs: padding with budget-consuming characters that render as nothing makes the node itself + produce a short, trusted-looking name. The node MUST either REFUSE an over-long value at ingest — + which is what `pairing.request` does — or mark the clip **IN-BAND**, as part of the rendered + string, never as a separate flag a caller can drop. +- **The display budget MUST be charged on RENDERED WIDTH**, and Unicode `Cf` format characters, + zero-width characters and bidi overrides MUST be neutralised — not merely `is_control()`. A + neutralised character MUST render VISIBLY; deleting it lets an attacker choose what the operator + sees just as effectively as inserting one. +- **Only the RENDERING is neutralised.** The stored `client_name` MUST stay byte-verbatim, because + a value that is ever compared or used as an identity must not be quietly rewritten. +- **The value MUST NOT be able to add a line to the prompt**, which is line-oriented, and its slot + MUST be quoted such that an embedded quote cannot terminate it and let the remainder read as the + node's own words. + +### 18.25d. A privilege-gated test MUST assert in every privilege branch (dig-node#355) + +A security test that skips its assertions under `root` reports `ok` while proving nothing, and CI +containers commonly run as root — so the guard may never have executed. A silently-skipping test is +worse than a missing one, because it is counted as coverage. + +Where a discriminator (a Unix mode bit) is genuinely not meaningful under `root`, the test MUST +assert the COMPLEMENTARY observable that still is — ownership, which only `root` can manipulate — +rather than skipping. Every branch must be able to fail. + ## 18.26. Coin reservations — the two phases, and who owns the truth (dig_ecosystem#3127) A **coin reservation** records that a coin is already committed to a spend that has not settled, so a diff --git a/crates/dig-node-core/src/lib.rs b/crates/dig-node-core/src/lib.rs index b0a7f1e3..6db96031 100644 --- a/crates/dig-node-core/src/lib.rs +++ b/crates/dig-node-core/src/lib.rs @@ -4463,9 +4463,10 @@ impl Node { // NOTE: this copies the seed out of its `Zeroizing` wrapper, because // `Node::identity_seed` is a plain `Option<[u8; 32]>` that seam 7's // `KeyManager::identity_seed_for_peer` returns by value. Narrowing that type - // is a seam-wide change, tracked separately -- it is not made cheaper by - // doing it here, and doing it here would widen this diff across the peer - // seam. The copy lives as long as the node does either way. + // is a seam-wide change, tracked at + // https://github.com/DIG-Network/dig-node/issues/345 -- it is not made + // cheaper by doing it here, and doing it here would widen this diff across + // the peer seam. The copy lives as long as the node does either way. Some(*seed) } Err(e) => { diff --git a/crates/dig-node-core/src/seams/key_mgmt/machine_key.rs b/crates/dig-node-core/src/seams/key_mgmt/machine_key.rs index 498fcf90..019c4209 100644 --- a/crates/dig-node-core/src/seams/key_mgmt/machine_key.rs +++ b/crates/dig-node-core/src/seams/key_mgmt/machine_key.rs @@ -379,21 +379,55 @@ impl MachineKeyStore { ) -> Result, MachineKeyError> { let seed_key = BackendKey::new(SEED_RECORD); // `presence`, never `Path::exists`. `exists()` reports a locked, permission-denied or - // otherwise unreadable path as ABSENT, and the very next thing this function does with an - // "absent" answer is MINT — overwriting both halves of a real identity that was merely - // unreadable for a moment. Before this seed was sealed that misread cost a recoverable - // duplicate; now both halves would be gone, so the read has to refuse instead. - if self.seed_presence()? == Presence::Present { - return self.unseal_stored(&seed_key); + // otherwise unreadable path as ABSENT, and the one branch below that mints would then + // overwrite both halves of a real identity that was merely unreadable for a moment. + // Before this seed was sealed that misread cost a recoverable duplicate; now both halves + // would be gone, so the read refuses instead. + // + // Written as a MATCH with both arms named rather than as an early `return`: the mint is + // reachable from exactly one arm, and a reader can see which one without tracing control + // flow (dig-node#345). + match self.seed_presence()? { + Presence::Present => self.unseal_stored(&seed_key), + Presence::Absent => self.mint_or_migrate(&seed_key, legacy_dir), } - let seed = match legacy_dir.map(Self::read_legacy).transpose()?.flatten() { - Some(seed) => seed, - None => Zeroizing::new(random_bytes::<32>()), + } + + /// No sealed blob exists: adopt a legacy plaintext seed if there is one, else mint a fresh one. + /// + /// # The no-mint rule, stated as an arm rather than carried by an operator + /// + /// This is the only code path in the module that can create a new identity, and the only one + /// that deletes the legacy plaintext. It may run ONLY on + /// [`LegacySeed::ConfirmedAbsent`] — a value [`Self::read_legacy`] constructs in a single + /// place, from a [`Presence::Absent`] answer, and never from a failed read. + /// + /// The previous shape was safe only because `read_legacy` happened to use `?` on its + /// `fs::read`. Swap that one operator for `.ok()` and an unreadable-but-present legacy seed + /// reads as absent, so this function mints over it and then deletes it — irreversible key + /// loss, produced by a character. A rule that depends on which operator someone typed is one + /// refactor away from being gone, so it is a named variant now and + /// `an_unreadable_legacy_seed_is_never_minted_over_or_deleted` fails if that relaxation is + /// ever made. + fn mint_or_migrate( + &self, + seed_key: &BackendKey, + legacy_dir: Option<&Path>, + ) -> Result, MachineKeyError> { + let legacy = match legacy_dir { + Some(dir) => Self::read_legacy(dir)?, + None => LegacySeed::ConfirmedAbsent, }; - let settled = self.seal_new(&seed_key, &seed)?; - if let Some(dir) = legacy_dir { - // Only reached once `seal_new` has proven the sealed copy reads back from storage: - // removing the one plaintext copy before that would destroy the node's identity. + let seed = match &legacy { + LegacySeed::Found(seed) => seed.clone(), + LegacySeed::ConfirmedAbsent => Zeroizing::new(random_bytes::<32>()), + }; + let settled = self.seal_new(seed_key, &seed)?; + if let (LegacySeed::Found(_), Some(dir)) = (&legacy, legacy_dir) { + // Two conditions, both required, and both now visible in the pattern: the legacy seed + // was CONFIRMED READ (never merely "not seen"), and `seal_new` has proven the sealed + // copy reads back from storage. Removing the one plaintext copy without either would + // destroy the node's identity. let _ = std::fs::remove_file(dir.join(LEGACY_SEED_FILE)); } Ok(settled) @@ -421,7 +455,30 @@ impl MachineKeyStore { Ok(self.backend.blob_tier(&BackendKey::new(SEED_RECORD))?) } - /// One honest sentence about the stored seed's protection, fit for a log line or status field. + /// One honest sentence about the stored SEED's protection, fit for a log line or status field. + /// + /// # What sealing does NOT buy, and why that must be said here (dig-node#343) + /// + /// Every sentence below describes the stored SEED. It does not describe + /// `/peer-net/identity/node.key` — the BLS/TLS key DERIVED from that seed, which + /// `dig_tls::NodeCert::load_or_generate` persists UNSEALED because the dig-gossip pool + /// listener loads it from disk by path (`dig_peer_protocol::load_ssl_cert`). + /// + /// That derived key is the artifact peers actually authenticate: `peer_id` is + /// `SHA-256(TLS SPKI DER)`, so whoever reads that one file has this node's network identity + /// for every purpose the network cares about — dialing as it, serving as it, and any + /// authorization keyed on it — WITHOUT touching either half of the sealed pair. + /// + /// So a summary that said only "sealed to this host" would be true of the seed and false of + /// the thing a reader assumes it means. [`DERIVED_KEY_CAVEAT`] is therefore appended to every + /// variant, including the failure one, and + /// `the_protection_summary_never_claims_copy_resistance_without_naming_the_derived_key` + /// fails if a variant is added without it. + /// + /// Sealing the derived key is the better fix and is NOT done here: the key is written by + /// `dig-tls`, in another repo, and is read back by path by a third crate, so it is a + /// release-first cascade rather than a change this module can make. An honest documented gap + /// beats an unstated one in the meantime. /// /// On a host with no provider this says the key is protected by file permissions and names the /// reason the tier degraded — it never implies hardware backing the key does not have. On a @@ -431,6 +488,14 @@ impl MachineKeyStore { /// original machine with its trusted component wiped (permanent). Any reassurance would be a /// guess, and the wrong guess is the irreversible one. pub fn protection_summary(&self) -> String { + format!("{}. {DERIVED_KEY_CAVEAT}", self.seed_protection_summary()) + } + + /// The seed-only half of [`Self::protection_summary`], without the derived-key caveat. + /// + /// Split out so the caveat is appended in ONE place rather than in each arm: a variant added + /// later cannot forget it, because there is nowhere to forget it. + fn seed_protection_summary(&self) -> String { match self.protection() { // The blob names a hardware CLASS and carries no device identity, so "it is wrapped" // is NOT "this host can open it". Only a host bound to the same class may speak in the @@ -569,8 +634,14 @@ impl MachineKeyStore { } } - /// The legacy plaintext seed, if `dir` holds one. - fn read_legacy(dir: &Path) -> Result>, MachineKeyError> { + /// The legacy plaintext seed at `dir`, three-valued by construction (dig-node#345). + /// + /// The two safe answers are VALUES; every unsafe answer is an `Err`. In particular there is + /// no way to obtain [`LegacySeed::ConfirmedAbsent`] from a failed read: it is produced in + /// exactly one place, from a [`Presence::Absent`] verdict, which is what makes + /// "never mint over a seed that is merely unreadable" a property of the type rather than of + /// the error operator on the next line. + fn read_legacy(dir: &Path) -> Result { let path = dir.join(LEGACY_SEED_FILE); // Same refusal as the mint decision: an unreadable legacy path reported as absent would // mint a NEW identity while the real one sat right there, unreadable for a moment. @@ -579,12 +650,42 @@ impl MachineKeyStore { source, })?; match found { - Presence::Absent => Ok(None), - Presence::Present => exactly_32(LEGACY_SEED_FILE, &std::fs::read(&path)?).map(Some), + Presence::Absent => Ok(LegacySeed::ConfirmedAbsent), + Presence::Present => match std::fs::read(&path) { + Ok(bytes) => exactly_32(LEGACY_SEED_FILE, &bytes).map(LegacySeed::Found), + // NAMED, so the rule is legible at the point it is enforced: a legacy seed that + // is PRESENT but unreadable is a refusal, never an absence. An on-access scanner, + // a roaming-profile sync or a permission blip all land here, and every one of + // them resolves by itself — whereas treating any of them as "no seed" mints over + // the real identity and then deletes it. + Err(source) => Err(MachineKeyError::ExistenceUndeterminable { path, source }), + }, } } } +/// What the legacy plaintext seed read established. Three-valued: the two safe answers are +/// variants and everything else is an `Err` (dig-node#345). +enum LegacySeed { + /// A legacy plaintext seed was READ and validated. Migrating it is safe, and it is the only + /// answer that permits deleting the plaintext copy afterwards. + Found(Zeroizing<[u8; 32]>), + /// The legacy path was determined to be ABSENT. The ONLY answer that permits minting. + /// + /// Constructed in exactly one place, from [`Presence::Absent`]. A read failure can never + /// produce it, which is the whole point of the variant. + ConfirmedAbsent, +} + +/// The sentence appended to every protection summary, naming what sealing the seed does NOT cover +/// (dig-node#343). +/// +/// It is a constant rather than prose repeated per arm so the claim cannot drift between the +/// variants, and so a test can assert its presence rather than matching on wording. +pub const DERIVED_KEY_CAVEAT: &str = "The DERIVED peer key at peer-net/identity/node.key is NOT \ + sealed \u{2014} it is stored readable because the peer listener loads it by path, and \ + possession of that one file is possession of this node's peer_id"; + /// Narrow a stored record to the 32 bytes a seed must be. fn exactly_32(record: &'static str, bytes: &[u8]) -> Result, MachineKeyError> { <[u8; 32]>::try_from(bytes) @@ -795,6 +896,172 @@ mod tests { haystack.windows(needle.len()).any(|w| w == needle) } + /// **Proves (dig-node#345):** a legacy plaintext seed that is PRESENT but unreadable is never + /// minted over, and is never deleted. + /// + /// This is the destructive case the module exists to prevent, and until now nothing tested + /// it: `read_legacy` was safe only because its `fs::read` happened to carry a `?`. Replace + /// that operator with `.ok()` — a one-character relaxation a refactor could make in good + /// faith — and the unreadable seed reads as ABSENT, so `load_or_create` mints a new identity, + /// seals it, and then removes the real seed. Both halves gone, from a character. + /// + /// The fixture is a DIRECTORY at the legacy seed path. It is the one shape that makes + /// `presence` answer `Present` while `fs::read` fails, on every platform, without needing + /// permissions the runner may or may not have — which matters because a `chmod`-based fixture + /// is exactly the kind that silently stops discriminating under root (dig-node#355). + #[test] + fn an_unreadable_legacy_seed_is_never_minted_over_or_deleted() { + let root = tempfile::tempdir().expect("tempdir"); + let dir = identity_dir(&root); + std::fs::create_dir_all(&dir).expect("identity dir"); + let legacy_dir = root.path().join("legacy"); + std::fs::create_dir_all(&legacy_dir).expect("legacy dir"); + + // Present to `presence`, unreadable to `fs::read`, on every platform. + let legacy_path = legacy_dir.join(LEGACY_SEED_FILE); + std::fs::create_dir(&legacy_path).expect("the unreadable fixture"); + assert_eq!( + presence(&legacy_path).expect("presence"), + Presence::Present, + "the fixture must LOOK present, or this test proves nothing" + ); + assert!( + std::fs::read(&legacy_path).is_err(), + "the fixture must be unreadable, or this test proves nothing" + ); + + let store = software_store(&dir); + let err = store + .load_or_create(Some(&legacy_dir)) + .expect_err("an unreadable legacy seed must refuse, never mint"); + + assert!( + matches!(err, MachineKeyError::ExistenceUndeterminable { .. }), + "the refusal must name the undetermined read, got: {err}" + ); + assert!( + legacy_path.exists(), + "the legacy seed must NOT be deleted when it could not be read" + ); + assert!( + !store.seed_blob_path().exists(), + "nothing may be sealed over an identity we could not read" + ); + } + + /// The control for the test above: a legacy seed that IS readable is migrated, and only then + /// is the plaintext removed. + /// + /// Without this, an implementation that refused every legacy path would pass the refusal test + /// while breaking migration entirely — the failure mode a one-sided assertion cannot see. + #[test] + fn a_readable_legacy_seed_is_adopted_and_then_removed() { + let root = tempfile::tempdir().expect("tempdir"); + let dir = identity_dir(&root); + std::fs::create_dir_all(&dir).expect("identity dir"); + let legacy_dir = root.path().join("legacy"); + std::fs::create_dir_all(&legacy_dir).expect("legacy dir"); + + let legacy_path = legacy_dir.join(LEGACY_SEED_FILE); + let planted = [7u8; 32]; + std::fs::write(&legacy_path, planted).expect("plant the legacy seed"); + + let store = software_store(&dir); + let settled = store + .load_or_create(Some(&legacy_dir)) + .expect("a readable legacy seed migrates"); + + assert_eq!( + settled.as_slice(), + &planted, + "the legacy identity must be ADOPTED, not replaced" + ); + assert!( + !legacy_path.exists(), + "the plaintext copy is removed once the sealed copy reads back" + ); + } + + /// **Proves:** `ConfirmedAbsent` is the ONLY answer that reaches the mint, and it is produced + /// only from a determined absence. + /// + /// Asserted directly on `read_legacy` because the destructive consequence is two calls away + /// from the read, and a test that only observes the consequence cannot say which of the two + /// decisions was wrong. + #[test] + fn read_legacy_reports_confirmed_absence_only_for_a_determined_absence() { + let root = tempfile::tempdir().expect("tempdir"); + let empty = root.path().join("empty"); + std::fs::create_dir_all(&empty).expect("dir"); + assert!( + matches!( + MachineKeyStore::read_legacy(&empty), + Ok(LegacySeed::ConfirmedAbsent) + ), + "a determined absence is the one value that permits a mint" + ); + + let blocked = root.path().join("blocked"); + std::fs::create_dir_all(&blocked).expect("dir"); + std::fs::create_dir(blocked.join(LEGACY_SEED_FILE)).expect("unreadable fixture"); + assert!( + matches!( + MachineKeyStore::read_legacy(&blocked), + Err(MachineKeyError::ExistenceUndeterminable { .. }) + ), + "an unreadable legacy seed must be an Err, never ConfirmedAbsent" + ); + } + + /// **Proves (dig-node#343):** no protection summary ever claims copy-resistance without also + /// naming the derived peer key that is NOT covered. + /// + /// Sealing the seed builds a real partial-exfiltration boundary — recovering it needs BOTH + /// `machine-identity.dks` and the sibling `device.dks`. But the key peers actually + /// authenticate is the DERIVED one at `peer-net/identity/node.key`, stored unsealed one + /// directory away, and `peer_id = SHA-256(SPKI DER)`. So "sealed to this host; it does not + /// open on another machine" is true of the seed and false of the artifact a reader assumes it + /// means — a shipped surface asserting a protection that does not cover the thing at risk. + /// + /// Asserted over BOTH tiers and the error path, because the caveat is only load-bearing if it + /// cannot be lost by adding one more arm. + #[test] + fn the_protection_summary_never_claims_copy_resistance_without_naming_the_derived_key() { + let root = tempfile::tempdir().expect("tempdir"); + + let software_dir = identity_dir(&root); + let software = software_store(&software_dir); + software.load_or_create(None).expect("mint + seal"); + let software_summary = software.protection_summary(); + assert!( + software_summary.contains(DERIVED_KEY_CAVEAT), + "the software-tier summary must name the uncovered derived key: {software_summary}" + ); + + let hardware_dir = root.path().join("hw"); + let hardware = hardware_store(&hardware_dir, 1); + hardware.load_or_create(None).expect("mint + seal"); + let hardware_summary = hardware.protection_summary(); + assert!( + hardware_summary.contains("does not open on another machine"), + "the hardware-tier claim under test must actually be made: {hardware_summary}" + ); + assert!( + hardware_summary.contains(DERIVED_KEY_CAVEAT), + "the copy-resistance claim must be scoped to the SEED: {hardware_summary}" + ); + + // The error path too: a store with nothing sealed yet still reports a tier, and the + // caveat is exactly as true there. + let empty_dir = root.path().join("empty"); + let empty_summary = software_store(&empty_dir).protection_summary(); + assert!( + empty_summary.contains(DERIVED_KEY_CAVEAT), + "even an unknown-protection summary must not imply the derived key is covered: \ + {empty_summary}" + ); + } + /// **Proves:** `DIG_IDENTITY_DIR` still selects the identity directory. /// /// **Catches:** dropping the override while reproducing digstore's private `identity_dir`. diff --git a/crates/dig-node-service/Cargo.toml b/crates/dig-node-service/Cargo.toml index e187aa76..726c28fe 100644 --- a/crates/dig-node-service/Cargo.toml +++ b/crates/dig-node-service/Cargo.toml @@ -203,6 +203,9 @@ tokio = { version = "1", features = ["rt-multi-thread", "macros", "signal", "tim tower-http = { version = "0.6", features = ["cors"] } serde = { version = "1", features = ["derive"] } serde_json = "1" +# Charging an operator-facing budget on RENDERED WIDTH rather than code points +# (`untrusted_text`, dig-node#346) needs the real Unicode width tables. +unicode-width = "0.2" # Structured logging. The node's engine library (`dig_node_core`) and its transitive # P2P/TLS stack emit `tracing` events; WITHOUT a subscriber installed here every one is diff --git a/crates/dig-node-service/src/config.rs b/crates/dig-node-service/src/config.rs index dba0e770..5ba4e023 100644 --- a/crates/dig-node-service/src/config.rs +++ b/crates/dig-node-service/src/config.rs @@ -575,6 +575,83 @@ pub fn normalize_upstream(raw: &str) -> String { } } +/// Reject an upstream that is not a well-formed, honestly-readable URL (dig-node#255 item 3). +/// +/// The only pre-existing check was for control characters, and it exists for an unrelated reason +/// (#526/B2 — a control character would be baked verbatim into a root-owned systemd unit line). +/// So every other malformed value persisted silently and became the passthrough target for every +/// method this node does not implement. +/// +/// Three rules, each closing something an operator reading `control.config.get` back could be +/// fooled by: +/// +/// 1. **A scheme, and only the two we speak.** [`normalize_upstream`] prepends `https://` to a +/// bare host, so anything still lacking a scheme here carries one we do not speak. +/// 2. **A non-empty host with no whitespace.** An empty host means "use the default" and must be +/// said that way rather than smuggled in as `https://`. +/// 3. **No userinfo (`@`).** `https://rpc.dig.net@evil.example` READS as the legitimate host to a +/// person and RESOLVES to the attacker's. That is the same forgery class as an unmarked +/// truncation: the display is honest about the bytes and dishonest about the meaning. +/// +/// Plain `http://` is permitted ONLY for loopback, so a developer's local upstream keeps working +/// while a remote cleartext upstream — which any on-path party could answer — does not. +pub fn validate_upstream(normalized: &str) -> Result<(), String> { + let rest = if let Some(r) = normalized.strip_prefix("https://") { + r + } else if let Some(r) = normalized.strip_prefix("http://") { + let host = host_of(r); + if !is_loopback_host(host) { + return Err(format!( + "an http:// upstream is only allowed for loopback (got host {host:?}); any \ + on-path party can answer cleartext, and this node forwards every unimplemented \ + method to it" + )); + } + r + } else { + return Err("upstream must be an http:// or https:// URL".to_string()); + }; + + let host = host_of(rest); + if host.is_empty() { + return Err( + "upstream has no host; pass an empty string to restore the default".to_string(), + ); + } + if host.chars().any(char::is_whitespace) { + return Err(format!("upstream host {host:?} contains whitespace")); + } + if rest.contains('@') { + return Err( + "upstream must not carry userinfo (`user@host`): such a URL reads as one host and \ + resolves to another" + .to_string(), + ); + } + Ok(()) +} + +/// The authority portion of a URL remainder (everything before the first `/`, `?` or `#`), with +/// any `host:port` left intact. +fn host_of(rest: &str) -> &str { + let end = rest.find(['/', '?', '#']).unwrap_or(rest.len()); + &rest[..end] +} + +/// Whether an authority names the local machine, for the cleartext carve-out above. +fn is_loopback_host(authority: &str) -> bool { + let host = authority + .rsplit_once(':') + .map(|(h, _)| h) + .unwrap_or(authority) + .trim_start_matches('[') + .trim_end_matches(']'); + host.eq_ignore_ascii_case("localhost") + || host == "127.0.0.1" + || host == "::1" + || host.eq_ignore_ascii_case("dig.local") +} + /// Split a normalised upstream into `(host, port)`, defaulting the port from the scheme. /// /// Pure, and deliberately tolerant: anything it cannot parse yields `None`, which the caller @@ -718,6 +795,67 @@ mod tests { use super::*; + /// **Proves (dig-node#255 item 3):** a userinfo URL is refused. + /// + /// `https://rpc.dig.net@evil.example` READS as the legitimate host to an operator checking + /// `control.config.get` and RESOLVES to the attacker's. Only the control-character check + /// existed before, and it exists for an unrelated reason, so this value persisted silently and + /// became the passthrough target for every unimplemented method. + #[test] + fn a_userinfo_upstream_is_refused_while_the_host_it_impersonates_is_accepted() { + assert!( + validate_upstream("https://rpc.dig.net@evil.example").is_err(), + "a userinfo URL reads as one host and resolves to another" + ); + assert!( + validate_upstream("https://rpc.dig.net").is_ok(), + "the host it impersonates must still be accepted, or this proves nothing" + ); + } + + /// **Proves:** cleartext is confined to loopback. + /// + /// A remote `http://` upstream can be answered by any on-path party, and this node forwards + /// every method it does not implement to whatever answers. A developer's local upstream keeps + /// working, which is the control that stops the rule collapsing into "refuse all http". + #[test] + fn cleartext_upstream_is_loopback_only() { + for ok in [ + "http://localhost:9779", + "http://127.0.0.1:8080", + "http://dig.local", + ] { + assert!(validate_upstream(ok).is_ok(), "{ok} is local cleartext"); + } + for bad in ["http://rpc.dig.net", "http://evil.example:80"] { + assert!(validate_upstream(bad).is_err(), "{bad} is remote cleartext"); + } + } + + /// **Proves:** a scheme we do not speak, and a scheme-less-after-normalisation value, are + /// refused rather than persisted. + #[test] + fn only_http_and_https_upstreams_are_accepted() { + for bad in [ + "ftp://rpc.dig.net", + "file:///etc/passwd", + "javascript:alert(1)", + "https://", + "https:// rpc.dig.net", + ] { + assert!(validate_upstream(bad).is_err(), "{bad} must be refused"); + } + } + + /// **Proves:** normalisation and validation compose the way the control method uses them — a + /// bare host becomes a valid `https://` URL rather than being refused for lacking a scheme. + #[test] + fn a_bare_host_normalises_into_an_accepted_https_upstream() { + let normalized = normalize_upstream("rpc.dig.net/"); + assert_eq!(normalized, "https://rpc.dig.net"); + assert!(validate_upstream(&normalized).is_ok()); + } + #[test] fn normalize_upstream_trims_and_strips_trailing_slash() { assert_eq!( diff --git a/crates/dig-node-service/src/control.rs b/crates/dig-node-service/src/control.rs index 49c51a95..b718b904 100644 --- a/crates/dig-node-service/src/control.rs +++ b/crates/dig-node-service/src/control.rs @@ -354,6 +354,52 @@ pub const KNOWN_UNPUBLISHED_CONTROL_METHODS: &[&str] = &["control.peers.ping"]; /// the latter widened silently: a newly served-but-unpublished method inherited the exemption the /// moment it was added to [`CONTROL_METHODS`], and no lockstep test could see it (a method the /// contract does not know is absent from both sides of every comparison they make). +/// Capabilities dig-node enforces at MASTER tier AHEAD of the published contract (dig-node#255). +/// +/// # Why an overlay exists at all, given #254 removed the last one +/// +/// The tier of a capability belongs in `dig-node-control-interface`, and restating it here as a +/// string match is the byte-drift bug that produced this whole family. This list is therefore NOT +/// a second opinion about tiers — it is a strictly-stricter, self-retiring bridge for a capability +/// the contract has not classified yet, and +/// [`locally_master_tier_methods_are_still_unclassified_by_the_contract`] FAILS the moment the +/// contract adopts one, so it cannot quietly outlive its reason. +/// +/// # The one member, and the rule that puts it here +/// +/// The contract's rule is *"master tier means the effect outlives the token that invoked it"*. +/// `control.config.setUpstream` persists a CALLER-CHOSEN URL that `Config::from_env` reads on the +/// next start as the RPC passthrough target, and `pairing.revoke` — the designated remedy for a +/// compromised paired app — does not touch it. So the escalation delegates and is not revocable: +/// after the call the attacker no longer needs the token. +/// +/// The reach is wider than `chiaPeers.add`, which is already master tier for the same shape. The +/// upstream is where every method this node does NOT implement is forwarded, and dig-node ships +/// with it EMPTY precisely so an unimplemented method answers a truthful local `-32601` rather +/// than something a third party made up. Pointing it at an attacker-controlled URL makes that +/// whole surface answerable by the attacker. +/// +/// # What is NOT here, judged rather than left unexamined (#255 item 2) +/// +/// `control.cache.setCap` and `control.log.setLevel` also persist, and they also survive a +/// revocation — so "outlives the token" alone would sweep them in. It is not the whole rule. The +/// discriminating question is whether the surviving effect confers AUTHORITY: whether it installs +/// a principal the node will thereafter believe, obey, or speak to. +/// +/// - `chiaPeers.add` installs a peer believed WITHOUT corroboration — a principal. Master. +/// - `config.setUpstream` installs a third party every unimplemented method is forwarded to — a +/// principal. Master. +/// - `cache.setCap` moves a local resource budget. It names nobody, is plainly visible on +/// `control.cache.get`, and is reset through the same ordinary-tier door it was set through. +/// Ordinary. (Promoting it would also break the cache-size control dig-app drives with a paired +/// token, which is a real cost for no authority gained.) +/// - `log.setLevel` changes local verbosity in a restricted-ACL log dir. It names nobody and +/// confers nothing. Ordinary. +/// +/// Stating the refinement as a rule — *outlives the token AND confers authority on a principal* — +/// is what lets the NEXT method be judged instead of matched against these four by analogy. +pub const LOCALLY_MASTER_TIER_CONTROL_METHODS: &[&str] = &["control.config.setUpstream"]; + pub fn requires_master_token(method: &str) -> bool { requires_master_token_given(method, KNOWN_UNPUBLISHED_CONTROL_METHODS) } @@ -366,6 +412,13 @@ pub fn requires_master_token(method: &str) -> bool { /// this rule from the one it replaces, because both answer "master" there. Injecting the list is /// what makes the difference observable. fn requires_master_token_given(method: &str, exempt: &[&str]) -> bool { + // The overlay is applied FIRST and only ever widens the master set, so this predicate can + // never be looser than the contract's — a contract that later promotes the same method + // changes nothing here, and one that never does still cannot leave the capability reachable + // by a paired token. + if LOCALLY_MASTER_TIER_CONTROL_METHODS.contains(&method) { + return true; + } match ControlMethod::from_name(method) { Some(published) => published.requires_master_token(), None => !exempt.contains(&method), @@ -1095,6 +1148,17 @@ fn config_set_upstream(ctx: &ControlCtx, id: Value, params: &Value) -> Value { ); } let normalized = crate::config::normalize_upstream(upstream); + // An EMPTY normalized value is the documented "restore the default" request and is the one + // value that must not be validated as a URL. + if !normalized.is_empty() { + if let Err(why) = crate::config::validate_upstream(&normalized) { + return control_error( + id, + ErrorCode::InvalidParams, + format!("control.config.setUpstream: {why}"), + ); + } + } match set_upstream_override(&ctx.config_path, &normalized) { Ok(()) => control_ok( id, @@ -5344,6 +5408,10 @@ mod tests { "control.pairing.revoke", "control.chiaPeers.add", "control.chiaPeers.remove", + // Master tier HERE ahead of the contract (dig-node#255): it persists a caller-chosen + // third party the node forwards every unimplemented method to, and it survives + // `pairing.revoke`. See LOCALLY_MASTER_TIER_CONTROL_METHODS. + "control.config.setUpstream", ] .into_iter() .collect(); @@ -5360,9 +5428,18 @@ mod tests { .map(|m| m.name()) .filter(|n| CONTROL_METHODS.contains(n)) .collect(); + let overlay: BTreeSet<&str> = LOCALLY_MASTER_TIER_CONTROL_METHODS + .iter() + .copied() + .collect(); assert_eq!( - actual, contract, - "this node's master tier disagrees with dig-node-control-interface" + actual, + contract.union(&overlay).copied().collect::>(), + "this node's master tier disagrees with dig-node-control-interface plus the declared local overlay" + ); + assert!( + contract.is_subset(&actual), + "the overlay may only WIDEN the contract's master set, never narrow it" ); } @@ -5656,6 +5733,78 @@ mod tests { let _ = std::fs::remove_file(&file); } + /// **Proves (dig-node#255):** a PAIRED token cannot reach `control.config.setUpstream`, while + /// the ordinary-tier methods it legitimately drives stay reachable. + /// + /// The escalation this closes delegates and is NOT revocable: the value persists into + /// `config.json`, `Config::from_env` reads it on the next start as the RPC passthrough target, + /// and `pairing.revoke` — the operator's designated remedy — does not touch it. So the + /// attacker stops needing the token the moment the call returns. + /// + /// The ordinary-tier half is the control. Without it, an implementation that answered "master" + /// for EVERY `control.*` method would pass the first assertion while breaking every paired + /// client, and the test could not tell the two apart. + #[test] + fn a_paired_token_cannot_set_the_rpc_upstream_but_still_drives_ordinary_config() { + assert!( + requires_master_token("control.config.setUpstream"), + "setUpstream persists a caller-chosen third party the node forwards to, and survives \ + pairing.revoke" + ); + + for ordinary in [ + "control.config.get", + "control.cache.setCap", + "control.log.setLevel", + "control.cache.get", + "control.status", + ] { + assert!( + !requires_master_token(ordinary), + "{ordinary} confers no authority over a principal and must stay paired-reachable" + ); + } + } + + /// **Proves:** the local master-tier overlay is a BRIDGE, not a second opinion — every member + /// is a capability the published contract has not classified yet. + /// + /// This is what makes the overlay self-retiring. When `dig-node-control-interface` promotes + /// `control.config.setUpstream`, this test fails and the entry must be deleted, so the repo + /// cannot end up with two disagreeing statements of the same tier — which is the byte-drift + /// bug that produced this whole family (#254 item 1). + #[test] + fn locally_master_tier_methods_are_still_unclassified_by_the_contract() { + for method in LOCALLY_MASTER_TIER_CONTROL_METHODS { + let published = ControlMethod::from_name(method) + .unwrap_or_else(|| panic!("{method} must be a published control method")); + assert!( + !published.requires_master_token(), + "the contract now puts {method} on the master tier; DELETE it from \ + LOCALLY_MASTER_TIER_CONTROL_METHODS so there is one rule rather than two" + ); + } + } + + /// **Proves:** the overlay can only WIDEN the master set — it never demotes a capability the + /// contract already protects. + /// + /// Asserted over the whole published method list rather than over the overlay, because the + /// failure being excluded is one the overlay's own contents cannot exhibit. + #[test] + fn the_overlay_never_demotes_a_contract_master_method() { + for method in CONTROL_METHODS { + if let Some(published) = ControlMethod::from_name(method) { + if published.requires_master_token() { + assert!( + requires_master_token(method), + "{method} is master tier in the contract and must remain so here" + ); + } + } + } + } + #[test] fn load_or_create_token_persists_and_is_stable() { let dir = std::env::temp_dir().join(format!( @@ -5672,12 +5821,23 @@ mod tests { let _ = std::fs::remove_dir_all(&dir); } - /// SECURITY (#501 residual): a pre-existing control-token file that is NOT owned by a - /// trusted principal (here forced group/other-readable, so not owner-only) MUST be DELETED - /// and REGENERATED — never returned — so a planted/squatted token can never become the - /// trusted one (which would hand an attacker full local node control). Unix-gated: it - /// relies on mode bits (CI runs on Linux). Skipped when running as root, where a - /// root-owned file is legitimately trusted regardless of mode. + /// SECURITY (#501 residual, dig-node#355): a pre-existing control-token file that is NOT + /// owned by a trusted principal MUST be DELETED and REGENERATED — never returned — so a + /// planted/squatted token can never become the trusted one (which would hand an attacker + /// full local node control). + /// + /// # Why this test has two branches instead of one branch and a hole + /// + /// It used to put EVERY assertion inside `if !running_as_root`, so under root it built the + /// fixture, called the function, checked nothing, and printed `ok`. CI containers commonly + /// run as root, so the one test standing between a planted token and full local node control + /// had plausibly never executed an assertion. + /// + /// Under root the MODE is genuinely not the discriminator — `unix_token_owner_is_trusted` + /// trusts any root-owned file, whatever its bits — but OWNERSHIP still is. So the root branch + /// chowns a second fixture to a foreign uid (which only root can do) and asserts THAT one is + /// regenerated. Both branches therefore exercise the guard, and deleting the guard fails this + /// test whichever uid runs it. #[cfg(unix)] #[test] fn foreign_owned_token_file_is_regenerated_not_trusted() { @@ -5698,7 +5858,44 @@ mod tests { .map(|m| m.uid() == 0) .unwrap_or(false); let got = load_or_create_token_at(&path).unwrap(); - if !running_as_root { + + if running_as_root { + // The carve-out, stated as an assertion rather than as a skip: a root-owned file is + // written by a trusted principal, so it is kept as-is even at 0644. + assert_eq!( + got, planted, + "a root-owned token is trusted regardless of mode; it must not be regenerated" + ); + + // The real second case. Only root can hand a file to another uid, which is exactly + // the planting an unprivileged attacker would have to achieve — and it is what the + // guard exists to refuse. + let foreign = dir.join("foreign").join(CONTROL_TOKEN_FILE); + std::fs::create_dir_all(foreign.parent().unwrap()).unwrap(); + std::fs::write(&foreign, &planted).unwrap(); + std::fs::set_permissions(&foreign, std::fs::Permissions::from_mode(0o600)).unwrap(); + const FOREIGN_UID: u32 = 65534; // `nobody` on every mainstream distro + std::os::unix::fs::chown(&foreign, Some(FOREIGN_UID), None).expect( + "root must be able to chown the fixture; without it this branch proves nothing", + ); + assert_eq!( + std::fs::metadata(&foreign).unwrap().uid(), + FOREIGN_UID, + "the fixture must actually be foreign-owned before the assertion below means anything" + ); + + let from_foreign = load_or_create_token_at(&foreign).unwrap(); + assert_ne!( + from_foreign, planted, + "a foreign-uid token must be regenerated, not returned — deleting the trust check \ + makes this equal the planted value" + ); + assert_eq!( + from_foreign.len(), + 64, + "the regenerated token is a fresh 64-hex value" + ); + } else { assert_ne!( got, planted, "an untrusted (group-readable) token must be regenerated, not returned" @@ -5718,6 +5915,46 @@ mod tests { let _ = std::fs::remove_dir_all(&dir); } + /// The trust RULE itself, asserted independently of whichever uid the suite happens to run + /// as (dig-node#355). + /// + /// [`crate::state::unix_token_owner_is_trusted`] is pure, so it can be interrogated for the + /// foreign-owner case on a root runner and the root case on an unprivileged one. A + /// filesystem test can only ever see the cases its runner's privileges permit; this one sees + /// all of them, and it fails the moment the rule is relaxed. + #[cfg(unix)] + #[test] + fn the_unix_token_trust_rule_refuses_a_foreign_owner_at_every_mode() { + use crate::state::unix_token_owner_is_trusted; + + for mode in [0o600, 0o644, 0o666, 0o400] { + assert!( + !unix_token_owner_is_trusted(4242, mode, 1000), + "a token owned by uid 4242 must never be trusted by uid 1000 (mode {mode:o})" + ); + assert!( + !unix_token_owner_is_trusted(4242, mode, 0), + "a foreign-owned token must not become trusted merely because WE are root \ + (mode {mode:o})" + ); + assert!( + unix_token_owner_is_trusted(0, mode, 1000), + "a root-owned token is written by a trusted principal (mode {mode:o})" + ); + } + + assert!( + unix_token_owner_is_trusted(1000, 0o600, 1000), + "our own owner-only token is trusted" + ); + for loose in [0o601, 0o640, 0o604, 0o660] { + assert!( + !unix_token_owner_is_trusted(1000, loose, 1000), + "our own token must be owner-only; {loose:o} lets another local user read it" + ); + } + } + /// A trusted (owner-only `0600`, current-user-owned) pre-existing token is loaded AS-IS — /// never regenerated — so a legit token stays stable across runs (#501 residual). #[cfg(unix)] @@ -5930,7 +6167,15 @@ mod tests { std::fs::write(&path, "a".repeat(64)).unwrap(); std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o000)).unwrap(); let running_as_root = std::fs::read_to_string(&path).is_ok(); - if !running_as_root { + if running_as_root { + // Root reads through mode `000`, so `PermissionDenied` is unreachable here. Assert + // the complementary observable instead of skipping (dig-node#355): the token is + // returned, and in particular the classifier does NOT invent the misleading + // `NotFound` for a file that is plainly present. + let got = read_token_readonly_at(&path) + .expect("root reads through mode 000; the reader must return the token"); + assert_eq!(got, "a".repeat(64), "the token is returned verbatim"); + } else { let err = read_token_readonly_at(&path).unwrap_err(); assert_eq!( err.kind(), diff --git a/crates/dig-node-service/src/lib.rs b/crates/dig-node-service/src/lib.rs index ba78a5cc..68755a5e 100644 --- a/crates/dig-node-service/src/lib.rs +++ b/crates/dig-node-service/src/lib.rs @@ -133,6 +133,10 @@ pub mod state; /// reloadable rustls config (fail-soft when no CA/leaf yet) and drive dig-cert's leaf /// renewal so the running listener hot-reloads a rotated leaf. See [`tls`]. pub mod tls; +/// Rendering ATTACKER-SUPPLIED text into an operator-facing sentence (dig-node#346): the clip mark +/// is in-band, the budget is charged on rendered width, and only the display is neutralised — the +/// stored value stays byte-verbatim. See [`untrusted_text`]. +pub mod untrusted_text; /// The beacon (`dig-updater`) RPC proxy (#515): `control.updater.*` reads the DIG auto-update /// beacon's world-readable status and shells its elevation-gated CLI for channel/pause/resume/ /// check-now — never a second implementation of the beacon's own trust logic. See [`updater`]. diff --git a/crates/dig-node-service/src/pair.rs b/crates/dig-node-service/src/pair.rs index ef94f806..ba2b0a79 100644 --- a/crates/dig-node-service/src/pair.rs +++ b/crates/dig-node-service/src/pair.rs @@ -18,6 +18,8 @@ use serde_json::{json, Value}; +use crate::untrusted_text::render_untrusted; + use crate::cli::Outcome; use crate::config::Config; use crate::control_client::call_control; @@ -48,11 +50,14 @@ pub fn run(config: &Config, action: PairAction) -> std::io::Result { "control.pairing.approve", json!({ "pairing_id": pairing_id }), )?; - let name = result["client_name"].as_str().unwrap_or("controller"); + let name = render_untrusted( + result["client_name"].as_str().unwrap_or("controller"), + CLIENT_NAME_COLUMNS, + ); let tid = result["token_id"].as_str().unwrap_or(""); Ok(Outcome::new( format!( - "dig-node: approved pairing for \"{name}\" — issued controller token {tid}.\n\ + "dig-node: approved pairing for {name:?} — issued controller token {tid}.\n\ The extension's poll will now receive its scoped token. Revoke anytime with \ `dig-node pair revoke {tid}`." ), @@ -76,7 +81,19 @@ pub fn run(config: &Config, action: PairAction) -> std::io::Result { } } +/// The display budget, in terminal columns, for an attacker-supplied `client_name`. +/// +/// It matches `pairing::MAX_CLIENT_NAME`, so a name this node ACCEPTED renders unmarked and the +/// clip marker only ever appears on a value that arrived from somewhere else (a hand-edited +/// paired-token store, or a future ingest path). The marker therefore carries information. +const CLIENT_NAME_COLUMNS: usize = 64; + /// Render `control.pairing.list` as an operator-friendly summary. +/// +/// Every `client_name` here is ATTACKER-SUPPLIED, because `pairing.request` is open and +/// unauthenticated, and it is composed into the sentence that gates a control-token grant. It +/// therefore goes through [`render_untrusted`] rather than into the format string directly +/// (dig-node#346). fn format_list(result: &Value) -> String { let mut out = String::new(); let pending = result["pending"].as_array().cloned().unwrap_or_default(); @@ -88,10 +105,13 @@ fn format_list(result: &Value) -> String { ); for p in &pending { out.push_str(&format!( - " • {} code {} \"{}\"\n approve: dig-node pair approve {}\n", + " • {} code {} {:?}\n approve: dig-node pair approve {}\n", p["pairing_id"].as_str().unwrap_or("?"), p["pairing_code"].as_str().unwrap_or("??????"), - p["client_name"].as_str().unwrap_or("controller"), + render_untrusted( + p["client_name"].as_str().unwrap_or("controller"), + CLIENT_NAME_COLUMNS, + ), p["pairing_id"].as_str().unwrap_or("?"), )); } @@ -103,9 +123,12 @@ fn format_list(result: &Value) -> String { out.push_str("Issued controller tokens (revoke with `dig-node pair revoke `):\n"); for t in &tokens { out.push_str(&format!( - " • {} \"{}\"\n", + " • {} {:?}\n", t["id"].as_str().unwrap_or("?"), - t["client_name"].as_str().unwrap_or("controller"), + render_untrusted( + t["client_name"].as_str().unwrap_or("controller"), + CLIENT_NAME_COLUMNS, + ), )); } } @@ -116,6 +139,125 @@ fn format_list(result: &Value) -> String { mod tests { use super::*; + /// **Proves (dig-node#346):** an attacker-supplied `client_name` cannot forge a line of the + /// operator's approval prompt. + /// + /// The prompt is line-oriented and the value is QUOTED rather than escaped, so a newline plus + /// the prompt's own bullet shape prints a second, entirely attacker-written pending request in + /// the node's voice — and the operator's only evidence about who is asking is this string. + /// + /// The assertion is on the LINE COUNT, not on the absence of a substring: a renderer that + /// escaped the quotes but kept the newline would still have added a line, and a substring + /// check would not see it. + #[test] + fn a_client_name_cannot_forge_an_extra_line_in_the_approval_prompt() { + let forged = "Sage\n \u{2022} deadbeef code 000000 \"DIG Chrome Extension\""; + let out = format_list(&json!({ + "pending": [{ + "pairing_id": "aabb", + "pairing_code": "123456", + "client_name": forged, + }], + "tokens": [], + })); + + // The property is LINES, not bullets. A renderer that escaped the quotes but let the + // newline through would still have added a line, and a substring check would not see it. + // + // The expected count is taken from the SAME render with a benign name rather than counted + // by hand: hand-counting is how this assertion was wrong the first time, and a baseline + // cannot drift when the prompt's own layout changes. + let benign = format_list(&json!({ + "pending": [{ + "pairing_id": "aabb", + "pairing_code": "123456", + "client_name": "Sage", + }], + "tokens": [], + })); + assert_eq!( + out.lines().count(), + benign.lines().count(), + "an attacker-supplied name must not change how many lines the prompt has:\n{out}" + ); + // The slot is `{:?}`-quoted, so an embedded quote is escaped rather than terminating the + // slot and letting the remainder read as the prompt's own words. + assert!( + !out.contains("\"DIG Chrome Extension\""), + "the forged inner quotation must not survive unescaped:\n{out}" + ); + + // The two assertions above are satisfied by the `{:?}` QUOTING ALONE — measured, by + // deleting `render_untrusted` from this call site and watching them both still pass. Debug + // renders a newline as the two characters `\n`, which neither adds a line nor leaves the + // quote unescaped, so they pin the quoting and say nothing about neutralisation. + // + // The replacement character is the discriminator Debug cannot supply: only + // `render_untrusted` maps a forbidden character to a VISIBLE U+FFFD. Dropping the call + // fails here. + assert!( + out.contains(crate::untrusted_text::REPLACEMENT), + "the newline must be NEUTRALISED to a visible replacement, not merely escaped:\n{out}" + ); + assert!( + !out.contains("\\n"), + "an escaped-but-surviving newline means the value was quoted rather than \ + neutralised:\n{out}" + ); + } + + /// **Proves:** a clip in the operator prompt is MARKED, so the operator can tell a short name + /// from a name the node made short. + /// + /// The paired-token store is on disk and is not bounded by `pairing::MAX_CLIENT_NAME`, so a + /// value longer than the display budget can genuinely reach this renderer. + #[test] + fn an_over_budget_client_name_is_clipped_with_a_visible_mark() { + let long = "x".repeat(400); + + // BOTH call sites, because they are separate `format!` arms: a test that read only one + // could not see the other losing its neutralisation. + let tokens = format_list(&json!({ + "pending": [], + "tokens": [{ "id": "tok1", "client_name": long }], + })); + assert!( + tokens.contains(crate::untrusted_text::CLIP_MARK), + "a clipped token name must say the node clipped it:\n{tokens}" + ); + + let pending = format_list(&json!({ + "pending": [{ + "pairing_id": "aabb", + "pairing_code": "123456", + "client_name": long, + }], + "tokens": [], + })); + assert!( + pending.contains(crate::untrusted_text::CLIP_MARK), + "a clipped pending name must say the node clipped it:\n{pending}" + ); + } + + /// **Proves:** a legitimate name renders EXACTLY, unmarked and unmangled. + /// + /// Without this control every assertion above is satisfied by a renderer that mangles + /// everything, which would make the clip marker meaningless and the prompt unreadable. + #[test] + fn an_ordinary_client_name_renders_unchanged() { + let out = format_list(&json!({ + "pending": [{ + "pairing_id": "aabb", + "pairing_code": "123456", + "client_name": "DIG Chrome Extension", + }], + "tokens": [], + })); + assert!(out.contains("\"DIG Chrome Extension\""), "{out}"); + assert!(!out.contains(crate::untrusted_text::CLIP_MARK), "{out}"); + } + #[test] fn format_list_reports_nothing_when_empty() { let s = format_list(&json!({ "pending": [], "tokens": [] })); diff --git a/crates/dig-node-service/src/pairing.rs b/crates/dig-node-service/src/pairing.rs index 89c1f1c1..f98f2c66 100644 --- a/crates/dig-node-service/src/pairing.rs +++ b/crates/dig-node-service/src/pairing.rs @@ -76,7 +76,16 @@ const PAIRING_TTL_MS: u64 = 5 * 60 * 1000; /// pending entries are dropped past this. const MAX_PENDING: usize = 32; -/// The longest `client_name` retained (defensive — it is echoed to the operator). +/// The longest `client_name` this node will ACCEPT, in characters. +/// +/// It is a REFUSAL bound, not a clip (dig-node#346). Silently truncating an attacker-supplied +/// label is a forgery the node performs on the attacker's behalf: pad a hostile name with +/// characters that spend the budget while rendering as nothing, and the node's own clip produces +/// a short, trusted-looking name for the operator to approve. Refusing an over-long request is +/// visible to the caller and invents nothing. +/// +/// The stored value stays BYTE-VERBATIM; neutralisation happens at render time +/// ([`crate::untrusted_text::render_untrusted`]), because only the display is a lie surface. const MAX_CLIENT_NAME: usize = 64; /// Current unix time in milliseconds (0 on a clock error — only affects TTL math). @@ -128,9 +137,16 @@ pub fn request(pending: &Mutex, id: Value, params: &Value) -> V .map(str::trim) .filter(|s| !s.is_empty()) .unwrap_or("unknown controller") - .chars() - .take(MAX_CLIENT_NAME) - .collect(); + .to_string(); + if client_name.chars().count() > MAX_CLIENT_NAME { + return control_error( + id, + ErrorCode::InvalidParams, + format!( + "client_name must be at most {MAX_CLIENT_NAME} characters; this request is refused rather than shortened, because a name the node shortened is a name the node partly wrote" + ), + ); + } // Fail CLOSED: the pairing id + code gate the consent step, so if the OS CSPRNG is // unavailable refuse the request rather than mint guessable pairing material (§7.3). @@ -445,6 +461,60 @@ pub fn is_paired_token(path: &Path, presented: &str) -> bool { mod tests { use super::*; + /// **Proves (dig-node#346):** an over-long `client_name` is REFUSED, not shortened. + /// + /// The ingest used to `.take(64)`, which is an unmarked truncation on the OPEN, + /// unauthenticated `pairing.request` — so an attacker could pad a hostile label with budget + /// -consuming characters and have the NODE produce a short, trusted-looking name for the + /// operator to approve. Refusing is the only answer that invents nothing. + /// + /// The at-bound case is asserted alongside, because a bound tested only from above cannot + /// distinguish "refuses over-long" from "refuses everything". + #[test] + fn an_over_long_client_name_is_refused_rather_than_silently_shortened() { + let pending = Mutex::new(PendingPairings::default()); + + let at_bound = "n".repeat(MAX_CLIENT_NAME); + let ok = request(&pending, json!(1), &json!({ "client_name": at_bound })); + assert!( + ok.get("result").is_some(), + "a name exactly at the bound must be accepted: {ok}" + ); + + let too_long = "n".repeat(MAX_CLIENT_NAME + 1); + let refused = request(&pending, json!(2), &json!({ "client_name": too_long })); + assert_eq!( + refused["error"]["data"]["code"], + json!(ErrorCode::InvalidParams.name()), + "an over-long name must be refused: {refused}" + ); + assert!( + refused["error"]["message"] + .as_str() + .unwrap() + .contains("refused rather than shortened"), + "the refusal must say why it is a refusal: {refused}" + ); + } + + /// **Proves:** the accepted `client_name` is stored BYTE-VERBATIM. + /// + /// Neutralisation belongs at the render, never at the store: a value that is ever compared or + /// used as an identity must not be quietly rewritten, and rewriting at ingest would also make + /// the stored value disagree with what the requester believes it sent. + #[test] + fn an_accepted_client_name_is_stored_verbatim() { + let pending = Mutex::new(PendingPairings::default()); + // Contains characters the RENDERER must neutralise; the STORE must not. + let raw = "app\u{200b}\u{202e}name"; + let resp = request(&pending, json!(1), &json!({ "client_name": raw })); + let pairing_id = resp["result"]["pairing_id"].as_str().unwrap().to_string(); + + let g = pending.lock().unwrap(); + let stored = &g.map.get(&pairing_id).expect("pending entry").client_name; + assert_eq!(stored, raw, "the stored label must be byte-verbatim"); + } + /// A unique temp STATE dir (#501: the paired-token store now lives in the state /// dir, not beside a `config.json`). Returns `(state_dir, state_dir)` so both /// tuple bindings point at the dir a test seeds + cleans. diff --git a/crates/dig-node-service/src/server.rs b/crates/dig-node-service/src/server.rs index c8ce20bc..e2aec190 100644 --- a/crates/dig-node-service/src/server.rs +++ b/crates/dig-node-service/src/server.rs @@ -2185,10 +2185,18 @@ where // `POST /{method}` surface + the `/ws` transport, which is what the extension uses — but it is // no longer SILENT: `crate::wallet_mtls` logs it and publishes it on `control.status`. The // listener stops when the process exits with the rest of the node. + // + // The listener is GATED by the same `wallet_authz` policy the HTTP and `/ws` planes use + // (dig-node#257): the shared client certificate authenticates the transport, it does not + // authorize a capability. crate::wallet_mtls::spawn( DEFAULT_MTLS_PORT, wallet_backend.clone(), wallet_cert.clone(), + std::sync::Arc::new(crate::wallet_mtls::NodeWalletGate::new( + state.control_token.clone(), + &state.state_dir, + )), ); let app = router(state); diff --git a/crates/dig-node-service/src/state.rs b/crates/dig-node-service/src/state.rs index 12dcfb2f..a8c59448 100644 --- a/crates/dig-node-service/src/state.rs +++ b/crates/dig-node-service/src/state.rs @@ -1479,7 +1479,30 @@ mod tests { let running_as_root = std::fs::metadata(&path) .map(|m| m.uid() == 0) .unwrap_or(false); - if !running_as_root { + if running_as_root { + // Under root the MODE is not the discriminator, so asserting nothing here would make + // the whole branch a silent pass on the CI runners that are root (dig-node#355). + // Assert the carve-out itself, then the observable that IS still discriminating: + // ownership. Only root can hand the fixture to another uid, which is precisely the + // planting the guard refuses. + assert!( + token_file_is_trusted(&path, false), + "a root-owned token is trusted regardless of mode" + ); + const FOREIGN_UID: u32 = 65534; // `nobody` + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)).unwrap(); + std::os::unix::fs::chown(&path, Some(FOREIGN_UID), None) + .expect("root must be able to chown; without it this branch proves nothing"); + assert_eq!( + std::fs::metadata(&path).unwrap().uid(), + FOREIGN_UID, + "the fixture must actually be foreign-owned" + ); + assert!( + !token_file_is_trusted(&path, false), + "a foreign-uid token is never trusted, even by root" + ); + } else { assert!( !token_file_is_trusted(&path, false), "a group/other-readable token is not trusted" diff --git a/crates/dig-node-service/src/untrusted_text.rs b/crates/dig-node-service/src/untrusted_text.rs new file mode 100644 index 00000000..c441ad0a --- /dev/null +++ b/crates/dig-node-service/src/untrusted_text.rs @@ -0,0 +1,223 @@ +//! Rendering attacker-supplied text into an operator-facing sentence (dig-node#346). +//! +//! # The problem this exists to solve +//! +//! `pairing.request` is an OPEN, unauthenticated method, and the `client_name` it carries is +//! composed into the sentence an operator reads before granting a control token. The operator's +//! only evidence about who is asking is that string, so it is the input to a privileged decision. +//! +//! Three attacks follow from composing it verbatim, all of them demonstrated on the dig-app +//! equivalent (dig-app PR #265): +//! +//! 1. **An UNMARKED truncation is a forgery the app performs.** Pad a hostile name with +//! characters that consume the length budget while rendering as nothing, and the software's own +//! clip produces a short, trusted-looking name. Nobody typed the lie; the renderer wrote it. +//! 2. **Zero-width and format characters survive `trim`.** U+200B-200F, U+061C, U+2060 and U+FEFF +//! are neither `char::is_control` nor whitespace, so they pass every obvious filter while +//! spending budget. +//! 3. **Composition.** The prompt is line-oriented and the value is quoted rather than escaped, so +//! a newline or a bidi override lets the value forge additional lines in the node's own voice. +//! +//! # The three rules, and why each is structural rather than advisory +//! +//! - **The clip mark is IN-BAND.** [`render_untrusted`] returns ONE `String` that already contains +//! its marker. There is no companion `bool` a caller can forget to read - the dropped `bool` +//! *was* the dig-app CRITICAL. +//! - **The budget is charged on RENDERED WIDTH**, after the invisible characters are made visible, +//! so a name cannot buy silence with characters that occupy no columns. +//! - **Only the RENDERING is neutralised.** The stored `client_name` stays byte-verbatim, because +//! anything that is ever compared or used as an identity must not be quietly rewritten. + +use unicode_width::UnicodeWidthChar; + +/// The in-band marker appended when a value was clipped. +/// +/// It names the actor: the operator must be able to tell "this name is short" from "we made this +/// name short". +pub const CLIP_MARK: &str = "[clipped by dig-node]"; + +/// The replacement for a character that must not reach the terminal. +/// +/// A visible placeholder rather than a deletion: silently dropping a character lets an attacker +/// choose what the operator sees just as effectively as inserting one, and leaves no trace that +/// anything was removed. +pub const REPLACEMENT: char = '\u{fffd}'; + +/// Whether `c` may never be rendered into an operator-facing sentence. +/// +/// Covers three families, all of which the naive `char::is_control` check misses at least part of: +/// +/// - **Control characters**, including the newline and carriage return that let a value forge its +/// own line, and the tab that lets it forge a column. +/// - **Unicode `Cf` FORMAT characters** - the zero-width space/joiner family, the word joiner and +/// the byte-order mark. Enumerated by range rather than by an `is_*` predicate the standard +/// library does not offer. +/// - **Bidirectional overrides and isolates**, which reorder the text AROUND them and so can +/// rewrite the quoting the prompt relies on. +fn is_forbidden(c: char) -> bool { + if c.is_control() { + return true; + } + matches!(c as u32, + 0x00AD // SOFT HYPHEN + | 0x061C // ARABIC LETTER MARK + | 0x180E // MONGOLIAN VOWEL SEPARATOR + | 0x200B..=0x200F // zero-width space/joiner/non-joiner, LRM, RLM + | 0x202A..=0x202E // bidi embeddings + overrides + | 0x2060..=0x2064 // word joiner + invisible operators + | 0x2066..=0x206F // bidi isolates + deprecated format chars + | 0xFEFF // zero-width no-break space / BOM + | 0xFFF9..=0xFFFB // interlinear annotation + | 0x1D173..=0x1D17A // musical format controls + | 0xE0000..=0xE007F // tag characters + ) +} + +/// Render attacker-supplied `raw` for an operator, within `width_budget` display columns. +/// +/// Forbidden characters ([`is_forbidden`]) become [`REPLACEMENT`]; the remainder is charged +/// against the budget by its **display width**, and a value that does not fit is clipped with +/// [`CLIP_MARK`] appended IN-BAND. A value that fits is returned unmarked, so the marker's +/// presence is itself evidence. +/// +/// The budget is the width of the VALUE; the marker is additional, because a marker that had to +/// fit inside the budget could be clipped away by a long enough name - which is the failure it +/// exists to prevent. +pub fn render_untrusted(raw: &str, width_budget: usize) -> String { + let mut out = String::new(); + let mut used = 0usize; + let mut clipped = false; + + for c in raw.chars() { + let rendered = if is_forbidden(c) { REPLACEMENT } else { c }; + // A width of `None` means a non-printable the width tables cannot place; charge it as one + // column so it can never be free. + let w = rendered.width().unwrap_or(1).max(1); + if used + w > width_budget { + clipped = true; + break; + } + out.push(rendered); + used += w; + } + + if clipped { + out.push_str(CLIP_MARK); + } + out +} + +#[cfg(test)] +mod tests { + use super::*; + + /// **Proves:** a clip is ALWAYS marked, and the mark is part of the returned string. + /// + /// This is the dig-app CRITICAL restated as a type fact: there is no second return value to + /// drop, so a caller cannot render a clipped value as if it were whole. + #[test] + fn a_clipped_value_carries_its_mark_in_band() { + let long = "a".repeat(200); + let out = render_untrusted(&long, 16); + assert!(out.ends_with(CLIP_MARK), "the clip must be marked: {out}"); + assert_eq!( + out.chars().filter(|c| *c == 'a').count(), + 16, + "the budget is charged on the value, not on the marker" + ); + } + + /// **Proves:** a value that FITS is returned unmarked - so the marker means something. + /// + /// Without this control, an implementation that appended the mark unconditionally would pass + /// the test above while telling the operator every name had been tampered with. + #[test] + fn a_value_that_fits_is_not_marked() { + let out = render_untrusted("DIG Chrome Extension", 64); + assert_eq!(out, "DIG Chrome Extension"); + assert!(!out.contains(CLIP_MARK)); + } + + /// **Proves attack 1:** invisible padding cannot buy a short, trusted-looking render. + /// + /// The attack is to spend the budget on characters that occupy zero columns so the honest + /// suffix is clipped away and the app itself prints a clean name. Every padding character + /// here becomes a visible replacement and is charged a column, so the result is BOTH visibly + /// mangled and marked as clipped. + #[test] + fn zero_width_padding_cannot_hide_a_clip() { + let mut hostile = String::new(); + for _ in 0..60 { + hostile.push('\u{200b}'); // zero-width space: not control, not whitespace + } + hostile.push_str("Sage Wallet"); + + let out = render_untrusted(&hostile, 64); + + assert!( + !out.contains('\u{200b}'), + "a zero-width character must never reach the terminal: {out:?}" + ); + assert_eq!( + out.chars().filter(|c| *c == REPLACEMENT).count(), + 60, + "each invisible character must render visibly and cost a column" + ); + assert!( + !out.starts_with("Sage Wallet"), + "the hostile prefix must remain visible rather than being padded out of view" + ); + } + + /// **Proves attack 2/3:** a newline cannot forge a second line, and a bidi override cannot + /// reorder the sentence around the value. + /// + /// The forged line is the whole attack - the prompt's own format is + /// `- code ""`, so a value containing a newline plus that shape prints a + /// second, entirely attacker-written pending request in the node's voice. + #[test] + fn newlines_and_bidi_overrides_cannot_forge_a_line() { + let forged = "ok\n code 000000 \"Trusted App\""; + let out = render_untrusted(forged, 200); + assert!(!out.contains('\n'), "no newline may survive: {out:?}"); + assert!( + out.starts_with(&format!("ok{REPLACEMENT}")), + "the newline must render as a visible replacement: {out:?}" + ); + + for override_char in ['\u{202e}', '\u{2066}', '\u{202d}'] { + let out = render_untrusted(&format!("a{override_char}b"), 64); + assert!( + !out.contains(override_char), + "bidi control {override_char:?} must not survive: {out:?}" + ); + } + } + + /// **Proves:** a wide (double-column) character is charged two columns, so a CJK name cannot + /// overflow the operator's line by rendering at twice its code-point count. + #[test] + fn wide_characters_are_charged_their_rendered_width() { + // Each of these occupies two terminal columns. + let wide = "\u{4f60}\u{597d}\u{4e16}\u{754c}"; // 4 chars, 8 columns + assert_eq!( + render_untrusted(wide, 8), + wide, + "8 columns fits a budget of 8" + ); + + let out = render_untrusted(wide, 7); + assert!(out.ends_with(CLIP_MARK), "7 columns cannot hold 8: {out}"); + assert_eq!( + out.chars().filter(|c| !CLIP_MARK.contains(*c)).count(), + 3, + "only three wide characters fit in seven columns" + ); + } + + /// **Proves:** an empty value renders empty and unmarked - the renderer invents nothing. + #[test] + fn an_empty_value_renders_empty() { + assert_eq!(render_untrusted("", 64), ""); + } +} diff --git a/crates/dig-node-service/src/wallet_mtls.rs b/crates/dig-node-service/src/wallet_mtls.rs index cde15265..b2b898a6 100644 --- a/crates/dig-node-service/src/wallet_mtls.rs +++ b/crates/dig-node-service/src/wallet_mtls.rs @@ -16,7 +16,41 @@ use std::sync::{Arc, RwLock}; use serde_json::{json, Value}; use dig_wallet::sage::rpc::WalletBackend; -use dig_wallet::sage::transport::{serve_mtls, SharedCert}; +use dig_wallet::sage::transport::{serve_mtls, SharedCert, WalletCallGate}; + +/// The node's authorization policy for the Sage-parity wallet transport (dig-node#257). +/// +/// The transport crate owns the OBLIGATION to ask ([`WalletCallGate`]); this owns the ANSWER, +/// because tier is a property of the capability and only `dig-node-service` holds the published +/// contract that states it. Delegating to [`wallet_authz::authorize`] — the same function the +/// HTTP and `/ws` planes call — is what stops a third plane drifting into its own opinion. +/// +/// Possession of the shared wallet certificate authenticates the TRANSPORT. It is not a +/// capability tier, and it must not act as one: the cert is generated per run today, but the +/// moment it is persisted so a client can read it (which is the whole point of the parity +/// surface) an ungated listener becomes a fully open custody plane. +pub struct NodeWalletGate { + master: String, + paired_tokens_path: std::path::PathBuf, +} + +impl NodeWalletGate { + /// Build the gate from the node's master control token and its paired-token store. + pub fn new(master: String, state_dir: &std::path::Path) -> Self { + Self { + master, + paired_tokens_path: crate::pairing::paired_tokens_path(state_dir), + } + } +} + +impl WalletCallGate for NodeWalletGate { + fn authorize(&self, method: &str, presented: Option<&str>) -> bool { + crate::wallet_authz::authorize(method, presented, &self.master, |tok| { + crate::pairing::is_paired_token(&self.paired_tokens_path, tok) + }) + } +} /// What the last bring-up attempt did. `NotStarted` is the pre-serve state — a node that /// never reached the serve path (the in-process browser runtime, a CLI subcommand). @@ -87,12 +121,17 @@ fn set_state(next: ListenerState) { /// Never fatal: a failure is logged at WARN and published on `control.status` so the /// operator learns about the contention from the node itself rather than from an opaque /// TLS `handshake_failure` in whatever else wanted the port. -pub fn spawn(port: u16, backend: Arc, cert: SharedCert) { +pub fn spawn( + port: u16, + backend: Arc, + cert: SharedCert, + gate: Arc, +) { let Some(listener) = bind_and_record(port) else { return; }; tokio::spawn(async move { - if let Err(e) = serve_mtls(backend, listener, &cert).await { + if let Err(e) = serve_mtls(backend, listener, &cert, gate).await { tracing::warn!(error = %e, "wallet mTLS listener exited"); } }); diff --git a/crates/dig-node-service/tests/server.rs b/crates/dig-node-service/tests/server.rs index a68dbec3..105253dc 100644 --- a/crates/dig-node-service/tests/server.rs +++ b/crates/dig-node-service/tests/server.rs @@ -2518,7 +2518,12 @@ async fn control_method_with_wrong_token_is_rejected() { // revoke immediately un-authorizes it. /// A control MUTATION probe (`control.config.setUpstream`) for the pairing test — -/// reusable across the un-paired / paired / revoked assertions. +/// reusable across the un-paired / revoked assertions. +/// +/// This method is MASTER tier (`LOCALLY_MASTER_TIER_CONTROL_METHODS`, dig-node#255): the upstream +/// it persists is the third party every unimplemented method is forwarded to, and `pairing.revoke` +/// does not take it back. So it proves a token is REJECTED; it can never prove a PAIRED token is +/// live — use [`paired_tier_mutation`] for that. async fn setupstream_mutation(addr: &SocketAddr, token: Option<&str>) -> Value { post_rpc( addr, @@ -2529,6 +2534,25 @@ async fn setupstream_mutation(addr: &SocketAddr, token: Option<&str>) -> Value { .await } +/// A control MUTATION probe on the ORDINARY tier (`control.cache.setCap`), for asserting that a +/// PAIRED token is genuinely live. +/// +/// `cache.setCap` is the method dig-app already drives with a paired token, and +/// [`requires_master_token`]'s stated rule places it on the ordinary tier deliberately: it moves a +/// local resource budget, names no principal, and confers no authority that survives the token. +/// It therefore mutates for a paired token and is refused without one — exactly the discrimination +/// a liveness precondition needs. `cap_bytes` is echoed from the request, so the assertion reads +/// the call's own answer rather than process-global cache state a sibling test also writes. +async fn paired_tier_mutation(addr: &SocketAddr, token: Option<&str>) -> Value { + post_rpc( + addr, + json!({ "jsonrpc": "2.0", "id": 1, "method": "control.cache.setCap", + "params": { "cap_bytes": 128 * 1024 * 1024u64 } }), + token, + ) + .await +} + /// OPEN `pairing.poll` for the given id. async fn poll_pairing(addr: &SocketAddr, pairing_id: &str) -> Value { post_rpc( @@ -2545,9 +2569,15 @@ async fn pairing_flow_grants_then_revokes_a_scoped_control_token() { let (upstream, _calls) = start_mock_upstream().await; let (addr, master, _hold) = start_companion_full(&upstream).await; - // A control MUTATION with no token is rejected (the extension can't read the file). + // A control MUTATION with no token is rejected (the extension can't read the file) — on the + // ordinary tier too, so the rejection is about the missing token and not about the tier. let denied = setupstream_mutation(&addr, None).await; assert_eq!(denied["error"]["data"]["code"], json!("UNAUTHORIZED")); + let denied_ordinary = paired_tier_mutation(&addr, None).await; + assert_eq!( + denied_ordinary["error"]["data"]["code"], + json!("UNAUTHORIZED") + ); // 1. OPEN pairing.request → a pairing_id + a compare-codes value. let req = post_rpc( @@ -2583,9 +2613,9 @@ async fn pairing_flow_grants_then_revokes_a_scoped_control_token() { let scoped = approved["result"]["token"].as_str().unwrap().to_string(); assert_eq!(scoped.len(), 64); - // 5. The scoped token AUTHORIZES a control mutation. - let ok = setupstream_mutation(&addr, Some(&scoped)).await; - assert_eq!(ok["result"]["upstream"], json!("https://paired.example")); + // 5. The scoped token AUTHORIZES an ordinary-tier control mutation. + let ok = paired_tier_mutation(&addr, Some(&scoped)).await; + assert_eq!(ok["result"]["cap_bytes"], json!(128 * 1024 * 1024u64)); // 6. But the scoped token CANNOT administer pairings (master-only). let admin = post_rpc( @@ -2606,7 +2636,7 @@ async fn pairing_flow_grants_then_revokes_a_scoped_control_token() { .await; assert_eq!(revoke["result"]["revoked"], json!(true)); - let after_revoke = setupstream_mutation(&addr, Some(&scoped)).await; + let after_revoke = paired_tier_mutation(&addr, Some(&scoped)).await; assert_eq!(after_revoke["error"]["data"]["code"], json!("UNAUTHORIZED")); } @@ -2650,11 +2680,11 @@ async fn a_paired_token_cannot_grant_itself_a_trusted_chia_peer() { .unwrap() .to_string(); - // The scoped token is genuinely live: it drives an ordinary control mutation. - let live = setupstream_mutation(&addr, Some(&scoped)).await; + // The scoped token is genuinely live: it drives an ordinary-tier control mutation. + let live = paired_tier_mutation(&addr, Some(&scoped)).await; assert_eq!( - live["result"]["upstream"], - json!("https://paired.example"), + live["result"]["cap_bytes"], + json!(128 * 1024 * 1024u64), "the token must be VALID, or the refusals below prove nothing" ); diff --git a/crates/dig-wallet/src/sage/transport.rs b/crates/dig-wallet/src/sage/transport.rs index f01dafc8..c0653df9 100644 --- a/crates/dig-wallet/src/sage/transport.rs +++ b/crates/dig-wallet/src/sage/transport.rs @@ -42,6 +42,58 @@ use tower_http::cors::{Any, CorsLayer}; use super::events::SyncEvent; use super::rpc::WalletBackend; +/// The header a node-class client presents its control/paired token in, byte-identical to the +/// control plane's `X-Dig-Control-Token`. +/// +/// It is declared HERE, in the crate that owns the transport, because the transport is what must +/// read it; `dig-node-service` asserts the two spellings agree rather than each guessing. +pub const WALLET_TOKEN_HEADER: &str = "x-dig-control-token"; + +/// The authorization decision every route into [`WalletBackend::dispatch`] MUST obtain first. +/// +/// # Why this is a constructor parameter rather than a middleware someone remembers to add +/// +/// The Sage-parity mTLS listener used to authenticate with [`SharedCertVerifier`] alone and then +/// dispatch the entire wallet surface — custody, spends and master-tier peer mutations — on +/// possession of the shared cert (dig-node#257). The two token-bearing planes in +/// `dig-node-service` were gated; this third one was not, because nothing in the type system +/// asked it to be. A per-route test could not see the hole either, since the hole was a route +/// nobody had written a test for. +/// +/// [`build_router`] therefore takes a gate and there is exactly ONE handler behind `POST +/// /{method}`, so a fourth transport cannot reach `dispatch` without answering this question. The +/// POLICY still lives in `dig-node-service` (`wallet_authz`), which is the only place that knows +/// the tier of a capability; this crate owns only the obligation to ask. +pub trait WalletCallGate: Send + Sync + 'static { + /// Whether `method` may run for a caller presenting `presented`. + /// + /// `presented` is the raw token from [`WALLET_TOKEN_HEADER`], or `None` when the caller sent + /// no token at all. An implementation MUST treat `None` as unauthenticated rather than as a + /// local-possession grant — reaching the loopback socket is not a capability tier. + fn authorize(&self, method: &str, presented: Option<&str>) -> bool; +} + +/// A gate that authorizes NOTHING. +/// +/// This is the correct default for a transport whose policy owner has not been wired yet: a node +/// that cannot decide serves no wallet method. It exists so "no gate available" is expressible as +/// a refusal instead of as an omission. +#[derive(Debug, Clone, Copy, Default)] +pub struct DenyAll; + +impl WalletCallGate for DenyAll { + fn authorize(&self, _method: &str, _presented: Option<&str>) -> bool { + false + } +} + +/// The shared axum state: the handler set plus the gate that guards it. +#[derive(Clone)] +struct TransportState { + backend: Arc, + gate: Arc, +} + /// The default loopback port for the Sage-parity wallet mTLS listener (design C.4). /// /// **This is deliberately NOT Sage's own RPC port.** Sage defaults its RPC to `9257` @@ -163,26 +215,55 @@ pub fn build_server_config(cert: &SharedCert) -> Result>, + State(state): State, Path(method): Path, + headers: axum::http::HeaderMap, body: Bytes, ) -> Response { + let presented = headers + .get(WALLET_TOKEN_HEADER) + .and_then(|v| v.to_str().ok()); + if !state.gate.authorize(&method, presented) { + return plain_response( + StatusCode::UNAUTHORIZED, + "this wallet method requires the local control token (X-Dig-Control-Token) or a \ + paired controller token (see `dig-node pair`); presenting the shared wallet \ + certificate authenticates the transport, it does not authorize the capability" + .to_string(), + ); + } let body_str = String::from_utf8_lossy(&body); - let (status, out) = backend.dispatch(&method, &body_str).await; + let (status, out) = state.backend.dispatch(&method, &body_str).await; let code = StatusCode::from_u16(status).unwrap_or(StatusCode::INTERNAL_SERVER_ERROR); - let content_type = if status == 200 { - "application/json" + if status == 200 { + let mut resp = Response::new(axum::body::Body::from(out)); + *resp.status_mut() = code; + resp.headers_mut().insert( + header::CONTENT_TYPE, + HeaderValue::from_static("application/json"), + ); + resp } else { - "text/plain; charset=utf-8" - }; - let mut resp = Response::new(axum::body::Body::from(out)); + plain_response(code, out) + } +} + +/// A `text/plain` response with `code` — Sage's error shape, and the shape a refusal takes. +fn plain_response(code: StatusCode, body: String) -> Response { + let mut resp = Response::new(axum::body::Body::from(body)); *resp.status_mut() = code; - resp.headers_mut() - .insert(header::CONTENT_TYPE, HeaderValue::from_static(content_type)); + resp.headers_mut().insert( + header::CONTENT_TYPE, + HeaderValue::from_static("text/plain; charset=utf-8"), + ); resp } @@ -209,10 +290,14 @@ fn event_type_tag(event: &SyncEvent) -> &'static str { /// subscriber (missed events dropped from the broadcast channel) simply skips the gap rather /// than erroring the stream — `get_sync_status` polling remains the authoritative source of /// truth regardless. +/// +/// `GET /events` does NOT reach [`WalletBackend::dispatch`] and carries no method name, so the +/// per-method gate has nothing to answer about it; it streams the same sync notifications the +/// open read plane already publishes. async fn handle_events( - State(backend): State>, + State(state): State, ) -> Sse>> { - let rx = backend.events().subscribe(); + let rx = state.backend.events().subscribe(); let stream = BroadcastStream::new(rx).filter_map(|item| { let event = item.ok()?; let tag = event_type_tag(&event); @@ -226,21 +311,26 @@ async fn handle_events( } /// Build the shared `Router` (`POST /{method}` + `GET /events`) both transports dispatch. -pub fn build_router(backend: Arc) -> Router { +/// +/// `gate` is REQUIRED: there is no router without an authorization policy (dig-node#257). +pub fn build_router(backend: Arc, gate: Arc) -> Router { Router::new() .route("/:method", post(handle)) .route("/events", get(handle_events)) - .with_state(backend) + .with_state(TransportState { backend, gate }) } /// The browser mirror's router: the shared routes + permissive CORS (loopback only, so a /// wildcard origin is safe; the extension origin is `chrome-extension://…`). -pub fn build_cors_router(backend: Arc) -> Router { +/// +/// CORS widens who may SPEAK to the surface; it does not widen what they may do. The same gate +/// applies, which is why a browser client must present the same token a node-class client does. +pub fn build_cors_router(backend: Arc, gate: Arc) -> Router { let cors = CorsLayer::new() .allow_origin(Any) .allow_methods([Method::POST, Method::OPTIONS]) .allow_headers(Any); - build_router(backend).layer(cors) + build_router(backend, gate).layer(cors) } /// Serve the wallet mTLS listener (Sage byte-parity) on a pre-bound std listener. @@ -248,11 +338,12 @@ pub async fn serve_mtls( backend: Arc, listener: std::net::TcpListener, cert: &SharedCert, + gate: Arc, ) -> std::io::Result<()> { let config = build_server_config(cert).map_err(|e| std::io::Error::other(e.to_string()))?; let rustls_config = RustlsConfig::from_config(Arc::new(config)); axum_server::from_tcp_rustls(listener, rustls_config) - .serve(build_router(backend).into_make_service()) + .serve(build_router(backend, gate).into_make_service()) .await } @@ -260,26 +351,29 @@ pub async fn serve_mtls( pub async fn serve_http( backend: Arc, listener: tokio::net::TcpListener, + gate: Arc, ) -> std::io::Result<()> { - axum::serve(listener, build_cors_router(backend)).await + axum::serve(listener, build_cors_router(backend, gate)).await } /// Bring up BOTH transports on loopback (design C.3): the wallet mTLS listener and the -/// plain-HTTP+CORS browser mirror, each dispatching the shared handler set. Returns once -/// either listener exits. Both bind `127.0.0.1` only. +/// plain-HTTP+CORS browser mirror, each dispatching the shared handler set behind the SAME gate. +/// Returns once either listener exits. Both bind `127.0.0.1` only. pub async fn serve_dual( backend: Arc, mtls_port: u16, http_port: u16, cert: SharedCert, + gate: Arc, ) -> std::io::Result<()> { let mtls_listener = std::net::TcpListener::bind(("127.0.0.1", mtls_port))?; let http_listener = tokio::net::TcpListener::bind(("127.0.0.1", http_port)).await?; let mtls = { let backend = backend.clone(); - tokio::spawn(async move { serve_mtls(backend, mtls_listener, &cert).await }) + let gate = gate.clone(); + tokio::spawn(async move { serve_mtls(backend, mtls_listener, &cert, gate).await }) }; - let http = tokio::spawn(async move { serve_http(backend, http_listener).await }); + let http = tokio::spawn(async move { serve_http(backend, http_listener, gate).await }); tokio::select! { r = mtls => r.map_err(|e| std::io::Error::other(e.to_string()))?, r = http => r.map_err(|e| std::io::Error::other(e.to_string()))?, @@ -326,6 +420,63 @@ mod tests { ); } + /// A test gate that authorizes everything, so the pre-existing transport tests keep + /// exercising the dispatch path they were written for. + /// + /// It is deliberately test-only: the production crate offers [`DenyAll`] and nothing else, so + /// an allow-everything policy cannot be reached by a shipping call site. + struct AllowAll; + impl WalletCallGate for AllowAll { + fn authorize(&self, _method: &str, _presented: Option<&str>) -> bool { + true + } + } + + fn allow_all() -> Arc { + Arc::new(AllowAll) + } + + /// One question the gate was asked: the method name, and the token presented with it. + type GateQuestion = (String, Option); + + /// The shared log of every question a [`RecordingGate`] answered. + type GateLog = Arc>>; + + /// A gate that records every question it was asked and answers with a fixed verdict. + #[derive(Clone)] + struct RecordingGate { + verdict: bool, + asked: GateLog, + } + + impl RecordingGate { + fn new(verdict: bool) -> Self { + Self { + verdict, + asked: Arc::new(std::sync::Mutex::new(Vec::new())), + } + } + + fn asked_methods(&self) -> Vec { + self.asked + .lock() + .unwrap() + .iter() + .map(|(m, _)| m.clone()) + .collect() + } + } + + impl WalletCallGate for RecordingGate { + fn authorize(&self, method: &str, presented: Option<&str>) -> bool { + self.asked + .lock() + .unwrap() + .push((method.to_string(), presented.map(str::to_string))); + self.verdict + } + } + async fn test_backend() -> Arc { let db = WalletDb::open_in_memory().await.unwrap(); db.set_initial_sync_complete(true).await.unwrap(); @@ -354,8 +505,8 @@ mod tests { let backend = test_backend().await; // The two transports differ only by the CORS layer; the dispatched body must be // byte-identical (acceptance #3, structural proof). - let base = build_router(backend.clone()); - let cors = build_cors_router(backend.clone()); + let base = build_router(backend.clone(), allow_all()); + let cors = build_cors_router(backend.clone(), allow_all()); let (s1, b1) = oneshot_body(base, "get_version").await; let (s2, b2) = oneshot_body(cors, "get_version").await; let direct = backend.dispatch("get_version", "{}").await; @@ -367,7 +518,8 @@ mod tests { #[tokio::test] async fn error_body_is_plain_text_with_mapped_status() { let backend = test_backend().await; - let (status, body) = oneshot_body(build_router(backend), "get_secret_key").await; + let (status, body) = + oneshot_body(build_router(backend, allow_all()), "get_secret_key").await; assert_eq!(status, 404); assert!(body.contains("unsupported")); } @@ -412,7 +564,7 @@ mod tests { let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); let addr = listener.local_addr().unwrap(); - tokio::spawn(serve_http(backend, listener)); + tokio::spawn(serve_http(backend, listener, allow_all())); // Retry-connect so the test never races the server's first accept. let mut stream = None; @@ -447,7 +599,7 @@ mod tests { #[tokio::test] async fn events_sse_streams_a_published_sync_event() { let backend = test_backend().await; - let router = build_cors_router(backend.clone()); + let router = build_cors_router(backend.clone(), allow_all()); let req = axum::http::Request::builder() .method("GET") @@ -495,7 +647,118 @@ mod tests { #[tokio::test] async fn events_route_coexists_with_method_dispatch_route() { let backend = test_backend().await; - let (status, _) = oneshot_body(build_router(backend), "events").await; + let (status, _) = oneshot_body(build_router(backend, allow_all()), "events").await; assert_eq!(status, 405); } + + /// **Proves (dig-node#257):** NO route into [`WalletBackend::dispatch`] runs without the gate + /// having answered first — for a wallet read, a custody mutation, a master-tier peer + /// mutation, a retired custody name, and a method name that does not exist at all. + /// + /// The unknown name is the load-bearing case. A per-route or per-method-list gate is + /// satisfied by enumerating the methods someone thought of, which is exactly how the mTLS + /// plane came to serve the whole surface ungated: the hole was a route nobody had listed. + /// Asking the gate about a name the backend has never heard of proves the question is asked + /// by the ROUTER, not by a table. + #[tokio::test] + async fn every_route_into_dispatch_consults_the_gate_including_unknown_methods() { + const PROBES: &[&str] = &[ + "get_version", + "get_sync_status", + "send_xch", + "sign_coin_spends", + "submit_transaction", + "add_peer", + "wallet.unlock", + "a_method_that_has_never_existed", + ]; + + let backend = test_backend().await; + let gate = RecordingGate::new(false); + let router = build_router(backend.clone(), Arc::new(gate.clone())); + + for method in PROBES { + let (status, body) = oneshot_body(router.clone(), method).await; + assert_eq!( + status, 401, + "`{method}` was served by a denying gate; the transport authorizes on cert \ + possession alone" + ); + assert!( + body.contains("control token"), + "the refusal must name the credential it wants, got: {body}" + ); + } + + assert_eq!( + gate.asked_methods(), + PROBES.iter().map(|m| m.to_string()).collect::>(), + "the gate must be consulted once per call, for every method name" + ); + } + + /// **Proves:** a denied call never reaches the handler set — the refusal body is not the + /// dispatch output, so the method did not run and merely had its answer suppressed. + /// + /// Asserting the status alone would pass against an implementation that dispatched first and + /// rewrote the status afterwards, which for `submit_transaction` would have already + /// broadcast. + #[tokio::test] + async fn a_denied_call_does_not_reach_dispatch() { + let backend = test_backend().await; + let dispatched = backend.dispatch("get_version", "{}").await.1; + assert!( + dispatched.contains(env!("CARGO_PKG_VERSION")), + "control: dispatch really does answer get_version" + ); + + let router = build_router(backend.clone(), Arc::new(DenyAll)); + let (status, body) = oneshot_body(router, "get_version").await; + + assert_eq!(status, 401); + assert_ne!( + body, dispatched, + "the denied call still produced the dispatch body" + ); + } + + /// **Proves:** the gate receives the token verbatim from [`WALLET_TOKEN_HEADER`], and `None` + /// when the caller sends nothing — so a policy that distinguishes master from paired from + /// absent can actually express that distinction on this transport. + #[tokio::test] + async fn the_presented_token_reaches_the_gate_verbatim() { + let backend = test_backend().await; + let gate = RecordingGate::new(true); + let router = build_router(backend, Arc::new(gate.clone())); + + let req = axum::http::Request::builder() + .method("POST") + .uri("/get_version") + .header("content-type", "application/json") + .header(WALLET_TOKEN_HEADER, "tok-abc") + .body(axum::body::Body::from("{}")) + .unwrap(); + router.clone().oneshot(req).await.unwrap(); + let (_, _) = oneshot_body(router, "get_version").await; + + let asked = gate.asked.lock().unwrap().clone(); + assert_eq!( + asked, + vec![ + ("get_version".to_string(), Some("tok-abc".to_string())), + ("get_version".to_string(), None), + ], + "the header token must reach the gate unchanged, and its absence must read as None" + ); + } + + /// **Proves:** [`DenyAll`] is the only policy this crate ships. A transport whose policy + /// owner has not been wired serves nothing rather than everything. + #[test] + fn deny_all_refuses_every_method_with_and_without_a_token() { + for method in ["get_version", "send_xch", ""] { + assert!(!DenyAll.authorize(method, None)); + assert!(!DenyAll.authorize(method, Some("any-token"))); + } + } }