test(sec-2391): prove one anomalous frame cannot tear down a peer link - #64
Conversation
…ost-#63 main Co-Authored-By: Claude <noreply@anthropic.com>
DigLink's inbound loop removes three former one-frame kill switches (unmatched correlation id, unknown opcode, undecodable frame), but nothing in this crate proved it, so a future change could silently restore one. Drives each anomaly over a real TLS websocket and asserts a subsequent correlated request is still served, plus a negative control proving that assertion fails against a genuinely torn-down link. Refs DIG-Network/dig_ecosystem#2391 Co-Authored-By: Claude <noreply@anthropic.com>
Pins each hostile frame to the decoder so a fixture that quietly stopped being hostile cannot leave the tolerance tests green while exercising nothing. Test-only change, so patch. Co-Authored-By: Claude <noreply@anthropic.com>
MichaelTaylor3d
left a comment
There was a problem hiding this comment.
Correctness gate: PASS
Head reviewed: 967f095569bb40384d33602c6282589b78b4ac84.
Scope claim verified
git diff origin/main -- src/ is EMPTY, including src/relay/ (dig-relay vendors that wire byte-identically — unmoved). Diff is +356/-2 across Cargo.toml, Cargo.lock, and two test files. tests/common/wss_full_node.rs is purely additive — the new block is appended after the pre-existing acceptors, every prior byte unchanged. Version 0.23.1 in both Cargo.toml and Cargo.lock; cargo metadata --locked exits 0. Patch is the right bump for a test-only change.
The vacuity question — adjudicated by measurement, not by report
The survival assertion is structural, not hoped-for. connect_to blocks on request_infallible(RequestPeers::new()) and only returns Ok after a correlated RespondPeers arrives over the same link (src/service/gossip_handle.rs:840-844), and add_to_new_table is called even for an empty peer list precisely so the CON-001 hook records the exchange (:846-853). Since the server emits the hostile frame before reading RequestPeers, and the stream is ordered, the client cannot reach Ok without having read and survived the hostile frame. Delivery is structural.
I did not take the claimed mutation on report — I ran a stronger one. The implementer's proposed mutation edits the harness (close the socket after injecting), which mostly re-proves what the negative control already proves. The decisive question is whether these tests catch a production-side reintroduction of the kill switch in dig_peer_protocol::DigLink. So I vendored dig-peer-protocol 0.5.0, patched it in via [patch.crates-io], and reverted the tolerance at each of the three points in read_inbound:
Mutation (in DigLink::read_inbound) |
Result |
|---|---|
from_bytes_owned → None returns Err instead of continue |
malformed_frame_does_not_drop_the_link RED |
unmatched correlation id returns Err instead of falling through |
unmatched_correlation_id_does_not_drop_the_link RED |
opcode 223 returns Err |
unknown_opcode_does_not_drop_the_link RED |
| baseline (unmutated) | all 5 green |
Each case turns red only for its own mutation — no cross-talk, and the negative control plus the fixture guard stayed green throughout. That is materially stronger evidence than the PR claims for itself.
The subtle claim — both halves verified
UnknownOpcodereally is routine now, not a decode failure: the fixture guard'sDigMessage::from_bytes(...).expect(...)passes, so it decodes.MalformedFramereally does reach thefrom_bytes_owned → Noneskip path: mutating exactly that branch and nothing else turned exactly that test red. Measured, not reasoned.
Negative control and fixture guard are real
torn_down_link_fails_the_survival_assertiongenuinely errors:dial failed: LinkError(WebSocket(Protocol(SendAfterClosing))). Not vacuous.- Fixture guard: I mutated
MalformedFrameto declare a truthful 2-byte length; the guard went RED with its intended message. Real. (One gap in its opcode half — non-gating comment inline.)
Flakiness
Low risk. Five tests complete in 8-10s under default parallelism against a 20s per-dial budget — roughly 10x headroom on loopback TLS. Ran three times (serial + two parallel), zero intermittency. The failure signature when a kill switch exists is a budget timeout, so the bound is doing real work rather than masking.
Readable-code (§2.5)
Above bar. The module doc states what is proven, why the shape was chosen, and what is deliberately not covered (the full-channel case, with the flake reasoning) — that last section is exactly the honesty this contract asks for. HostileFrame as data rather than three harnesses is the right abstraction; dial_outcome returning Result<(), String> so the control can state how it failed is a good call. Hand-encoding the wire bytes is justified in a comment (a correct encoder cannot emit MalformedFrame).
One non-gating comment inline; I am resolving it myself so it does not block merge.
Co-authored-by: MichaelTaylor3d <5665004+MichaelTaylor3d@users.noreply.github.com>
Co-authored-by: MichaelTaylor3d <5665004+MichaelTaylor3d@users.noreply.github.com>
Co-authored-by: MichaelTaylor3d <5665004+MichaelTaylor3d@users.noreply.github.com>
The guard listed three DIG opcodes by hand, so it covered 3 of the 23 assigned and could not catch its own drift. ALL_DIG_OPCODES is the canonical set already used for the same purpose in src/connection/inbound_limits.rs. Co-Authored-By: Claude <noreply@anthropic.com>
DO NOT MERGE — draft; the orchestrator gates and merges this.
Closes the dig-gossip half of DIG-Network/dig_ecosystem#2391.
What this is
A re-derivation of #60 on top of post-#63
main, not a rebase. #60's central premise — thatthe crate is written against
chia_protocol::Messagein ~80 places and needs a translationseam — stopped being true when #63 migrated the crate onto
dig_peer_protocol::DigLinkwithraw
u8opcodes.src/connection/link_adapter.rsis therefore dropped, not ported. Whatsurvives is the valuable part: the acceptance test.
DigLink's inbound loop already removes three former one-frame kill switches, and a pre-mergesecurity audit confirmed it. Nothing in this crate proved it, so a future change could silently
restore one. This is that proof.
Blast radius checked
tests/only.git diff origin/main -- src/is EMPTY — includingsrc/relay/, whichdig-relay vendors byte-identically. The only shared file touched is
tests/common/wss_full_node.rs, which gains a new hostile-node acceptor and leaves everyexisting acceptor untouched; its three consumers (
con_001,dsc_004,dsc_005) were re-rungreen. No
src/change proved necessary — the ported tests pass onmainas-is, which is theexpected result and is itself the finding.
Tests
unmatched_correlation_id_does_not_drop_the_linkunknown_opcode_does_not_drop_the_linkmalformed_frame_does_not_drop_the_linktorn_down_link_fails_the_survival_assertionhostile_fixtures_still_have_the_property_they_are_named_forTwo design properties are preserved deliberately from #60:
as silently. It asserts a subsequent correlated request is still served:
connect_toreturns a peer id only after its
RequestPeersgets aRespondPeersover the same link,sent after the hostile frame. The stream is ordered, so the reply cannot have been read
without the hostile frame having been read and survived first.
would pass here while the real wire stayed broken.
Non-vacuity evidence
Reverting the tolerance by hand (server closes the websocket immediately after injecting the
hostile frame — exactly what a restored kill switch looks like) turns all three red, and the
control stays green:
Unmutated, with the injected byte counts printed by the server so delivery is observable:
Deliberately not covered
DigLinkalso drops rather than queues an inbound frame when the application channel isfull. Driving that here means racing a burst against a service task that drains the channel
continuously — no deterministic outcome, so it is left to a
dig-peer-protocolunit test wherethe channel can be held without a race. A flaky test is worse than an absent one.
#2391 also asks for a sweep across dig-relay / dig-nat / dig-peer / dig-app IPC. That sweep
is out of scope for this PR — other lanes own those repos.
Version
0.23.0→0.23.1(patch: test-only, no behaviour change).Cargo.toml+Cargo.lockbothbumped;
cargo metadata --lockedexits 0.The fixture guard: 1 -> 3 -> 23
The
hostile_fixtures_still_have_the_property_they_are_named_forguard exists so the hostilefixtures cannot silently stop exercising what their names claim. It took three passes to actually
do that, and the first two both passed review:
is_dig_messageis a bare== 220, plusRespondPeers)eaae16d(Copilot)DIG_MESSAGE,STORE_MELTED,HOLDINGS_ANNOUNCE)458831d(final)dig_peer_protocol::ALL_DIG_OPCODES)A guard that checks one member of the set it claims to cover is vacuous by construction, and
nobody noticed until someone counted. The canonical set was already in the crate for exactly this
purpose (
src/connection/inbound_limits.rs:330callsALL_DIG_OPCODES"the canonical" list), sothe fix is a reuse, not an invention.
Proven by mutation, not by inspection: setting
UNALLOCATED_DIG_OPCODEto200- an assigned DIGopcode that both earlier revisions accepted - now fails with
opcode 200 is no longer unallocated; restoring223returns all 5 tests to green.