Skip to content

fix(miner): cap pre-auth handshakes per source IP - #729

Open
illuzen wants to merge 3 commits into
mainfrom
fix/miner-preauth-slot-lockout
Open

illuzen wants to merge 3 commits into
mainfrom
fix/miner-preauth-slot-lockout

Conversation

@illuzen

@illuzen illuzen commented Nov 11, 2026

Copy link
Copy Markdown
Contributor

Problem

On the external-miner QUIC endpoint (--miner-listen-port), the pre-auth slot (unauth_slots) is acquired before the Ready { token } check, and TLS uses with_no_client_auth(). A tokenless peer can therefore complete the QUIC/TLS handshake, never send Ready, and reconnect the instant AUTH_HANDSHAKE_TIMEOUT (10s) frees its slot. The global cap of MAX_UNAUTHENTICATED_CONNECTIONS = 32 bounds how long a slot is held but not how fast one peer can re-take it: ~32/10s = 3.2 handshakes/s keeps all 32 slots full and refuses every legitimate miner. Already-authenticated miners keep their connection (the permit is dropped on success), but any miner that must (re)connect — e.g. right after a node restart — is locked out for as long as the attacker continues.

This was reported as a sustained DoS of a mining node. The companion crash issue (RUSTSEC-2026-0342, empty DATAGRAM frames in quinn-proto ≤ 0.11.16) is already fixed on main via the bump to quinn-proto 0.11.19; this PR addresses the remaining lockout.

Fix

Add a per-source-IP cap on in-flight pre-auth handshakes (MAX_UNAUTHENTICATED_CONNECTIONS_PER_IP = 4), enforced before the global permit. A peer must receive our packets to finish the handshake, so its source IP is genuine — bounding slots per IP means exhausting the global budget now requires many distinct hosts rather than one. The per-IP slot is released on the same paths as the global permit (auth success and handshake end), so authenticated miners don't count against it.

Notes / possible follow-ups

  • This is the minimal, non-breaking fix. Deeper hardening the reporter suggested — a --miner-listen-addr flag defaulting to loopback, a client certificate pinned like the server's, or a shorter Ready deadline — would move the credential check in front of the slot grab entirely, but each changes deployment or protocol behavior and is left out of this PR.

Test

  • New unit test per_ip_preauth_cap_bounds_one_ip_and_releases_on_drop covers the cap, per-IP isolation, and release-on-drop.
  • SKIP_WASM_BUILD=1 cargo clippy --locked -p quantus-node --tests clean; cargo test -p quantus-node miner_server — 13 passed.

Made with Cursor

A tokenless peer could complete the QUIC/TLS handshake, never send Ready,
and reconnect as fast as AUTH_HANDSHAKE_TIMEOUT frees its slot, keeping all
32 global pre-auth slots full and refusing every legitimate miner. Bound
in-flight pre-auth handshakes per source IP so exhausting the global budget
requires many distinct hosts rather than one.

Co-authored-by: Cursor <cursoragent@cursor.com>
@illuzen
illuzen requested a review from n13 November 11, 2026 10:37

@n13 n13 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: Request changes
Reviewer: Opus 5.5

The per-IP cap does not close the reported lockout. It keys on a source address that nobody has validated yet. The fix for that is small.

Blocking

  1. node/src/miner_server.rs:655-666: the per-IP slot is keyed on an unvalidated source address.
    endpoint.accept() yields a quinn Incoming built from a single client Initial packet. At that point remote_address_validated() is false. The code never checks it and never calls retry(). Both slots are charged right there, and they stay held for up to 10s (line 686) if the handshake never finishes. So the comment at line 80 ("a peer must receive our packets to finish the handshake, so its source IP is genuine") is false at the point where the slot is taken. Two results:

    • One host on a network that allows source spoofing sends 32 Initials every 10s from 8 forged IPs, 4 per IP. All 32 global slots stay full at about 4 KB/s. This is the same lockout the PR sets out to fix.
    • New with this PR: 4 spoofed Initials every 10s carrying a known miner's IP keep that IP at the cap. That miner is refused every time it reconnects. This also gets through the source-IP firewall allow-list that MINING.md offers as an option. Before this PR the same attack took 32 packets and hit every miner. The warn log then names the victim's IP as the offender.

    Fix: before taking either slot, do if !connecting.remote_address_validated() { connecting.retry()...; continue; }. quinn guarantees may_retry() is true in that case, and Retry is stateless. The per-IP key is then a proven address and the comment becomes true. The cost is one extra round trip per connect.

Non-blocking

  1. :83: even with genuine addresses, 8 IPv4 addresses (one /29) hold all 32 global slots. Each slot can also be held for about 20s, not 10s. The handshake timeout (line 686) and the Ready timeout (line 760) run one after the other while both guards are held. So "many distinct hosts" means 8. The PR raises the cost of the DoS but does not close it. Consider one shared deadline for handshake plus Ready, and the loopback-default bind listed as a follow-up.
  2. :83: miners that share one source IP (a NAT farm, a Docker port proxy, several GPU processes on 127.0.0.1) now share 4 pre-auth slots. The quantus-miner reconnect backoff has no jitter and only resets after auth (crates/miner-service/src/quic.rs), so refused miners retry together and can collide again. The pre-auth window is short, so this needs a near-simultaneous reconnect. MINING.md still says "Multiple miners can connect simultaneously" and does not mention the new limit.
  3. :1087: the new test only covers try_acquire_ip_slot on a local map. Nothing covers acceptor_task. If drop(ip_guard) moved after serve_authenticated_miner(...).await, the per-IP slot would be held for the whole session and no test would fail.
  4. :661: each refused Initial writes a warn! line, at a rate the sender controls. The global branch at line 672 already does this, so it is not new. But one IP now reaches it after 4 slots instead of 32. A counter with a periodic summary line would bound it.
  5. :625: if let Some(n) = counts.get_mut(&self.ip) silently skips a state that cannot happen. Log an error (or debug_assert!) instead of a silent no-op.
  6. :660 and :671: the IP guard and the semaphore permit are always taken and released together. That gives two near-identical refusal blocks and paired drops at 695-696 and 717-718. One PreAuthSlot guard holding both would remove the copy and make it impossible to release only one. The new field initializer is also copied into three struct literals (152, 997, 1082).
  7. :74-82, :111-112, :657-659: the same rationale is written three times, with em-dashes and long sentences. One short doc on the constant is enough.
  8. Nits: remote_ip (655) and remote (685) read the same address twice, so hoist one remote. Checking the global semaphore first would skip the map insert and remove when the global budget is full.

Checked and fine

The per-IP slot is released on every path: global-refuse continue, auth success, auth failure, handshake error, timeout, and panic. The count cannot underflow. The map stays at 33 entries or fewer. The std Mutex is never held across an .await. IPv6 /64 keying does not matter today because the endpoint binds 0.0.0.0.

Tests were not run locally, because a full node build exceeds the time limit. CI Format, Clippy and the Linux test matrix pass. The macOS test job was still pending.

Verdict: Request changes. The per-IP cap needs QUIC address validation (Retry) before it charges a slot. Without it, the cap does not stop the lockout it targets.

@n13 n13 removed the bot-review label Nov 11, 2026
…th slots

Address review: the per-IP cap keyed on the unvalidated address of a
single Initial, so spoofed Initials could still fill the budget or pin a
real miner's IP at the cap. Send a stateless QUIC Retry first and only
charge a slot once the peer has proven it receives at that address.

Also: fold the global permit and per-IP count into one PreAuthSlot guard,
take the global budget before the per-IP map, bound the whole pre-auth path
under a single AUTH_HANDSHAKE_TIMEOUT deadline (one 10s hold instead of two
back-to-back), debug_assert on the impossible release path, trim duplicated
docs, and document the pre-auth limits and Retry round-trip in MINING.md.

Co-authored-by: Cursor <cursoragent@cursor.com>
@illuzen

illuzen commented Nov 11, 2026

Copy link
Copy Markdown
Contributor Author

Thanks — the blocking point was right, and the recommended fix is in e611d8e6.

1 (blocking) — unvalidated source address. Fixed. The accept loop now checks connecting.remote_address_validated() and, when false, sends a stateless retry() and continues before touching either budget. No pre-auth slot is charged until the peer has proven it can receive at the claimed address, so forged Initials can neither fill the global budget nor pin a real miner's IP at the per-IP cap (and the warn log can no longer name a spoofed victim). may_retry() is guaranteed true while unvalidated; the Err branch refuses defensively via into_incoming().refuse(). The comment that asserted the address was genuine is gone and the constant's doc now says "Retry-validated".

2 — 8 IPs / ~20s hold. Partially addressed. Both phases now share one deadline: timeout_at(deadline, connecting) followed by authenticate_miner_connection(.., deadline) using timeout_at, so a slot is held for at most one AUTH_HANDSHAKE_TIMEOUT (10s), not two back-to-back. The 8-genuine-hosts ceiling is inherent to a per-IP cap and this PR raises the cost rather than closing it; the loopback-default bind / pinned client cert stay as follow-ups since they change deployment and protocol behavior.

3 — shared-IP miners. The limit is now documented in MINING.md (pre-auth only, 32 global / 4 per IP / 10s, authenticated miners exempt, one extra RTT for Retry, NAT'd miners reconnecting simultaneously should retry). Didn't touch the miner's backoff here since that lives in the miner repo.

4 — test coverage. Replaced the map-only test with two that exercise the real try_acquire_preauth_slot against a MinerServer: per-IP cap, that a per-IP refusal does not leak the global permit it briefly took (available_permits asserted), release-on-drop of both counts, and global-budget exhaustion/reuse. I did not add an acceptor_task test: it needs a live endpoint plus a client doing a real handshake, which the suite has no harness for. The single PreAuthSlot guard makes "release only one" impossible, but the early drop(slot) before serve_authenticated_miner is still an explicit call — flagging that honestly as the remaining gap.

5 — warn-log rate. Spoofed Initials now get a silent Retry and never reach the warn, so the attacker-controlled rate is gone for the unvalidated case. A validated peer that holds its cap still logs once per refused connect, bounded by its round-trip rate; left as warn! so operators see a real lockout.

6 — debug_assert! on the impossible None release path.
7 — one PreAuthSlot holding the OwnedSemaphorePermit and the per-IP count; one drop(slot) per path, two refusal arms collapsed into a match on PreAuthReject.
8 — rationale trimmed to a two-line doc on the constant.
9 — remote hoisted and read once; global semaphore is now taken before the per-IP map so a full budget skips the insert.

Local: clippy --locked -p quantus-node --tests clean; cargo test -p quantus-node miner_server — 14 passed.

@illuzen
illuzen requested a review from n13 November 11, 2026 15:33

@n13 n13 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: Approve
Reviewer: GPT-6.1 Sol

Reviewed head e611d8e6b6594a4abffa44fe2add347f2c16986e against base aae53e3693ab244c86cf3f10bb39e39500f5c6c2, including both changed files, the earlier review, and Quinn's locked-version address-validation implementation. No blocking findings.

The accept loop now validates the source address before reserving either budget. The combined guard releases both counts on rejection, handshake failure, timeout, and successful authentication; successful sessions release their slot before entering the long-lived handler. QUIC and Ready authentication share one deadline. The documented NAT limit matches the implementation.

Validation:

  • scripts/fmt.sh --all -- --check, Taplo formatting, and git diff --check: passed.
  • A temporary standalone harness compiled the unchanged node/src/miner_server.rs with the locked Quinn versions, a Tokio re-export, and a SHA-256 shim for the otherwise unrelated Substrate dependency. All 14 existing module tests passed. Three additional real QUIC tests also passed: eight concurrent authenticated sessions from one IP; four stalled pre-auth sessions rejecting a fifth and releasing slots after timeout; repeated wrong-token rejection followed by successful authentication. Total: 17 passed.
  • timeout 60 env SKIP_WASM_BUILD=1 CARGO_TARGET_DIR=.review-target cargo test --locked --offline -p quantus-node miner_server stopped during dependency compilation at the requested limit; the full node suite was not completed. GitHub CI remains in progress.

Non-blocking follow-up: add the real QUIC accept-loop cases to the repository suite so Retry ordering and release before long-lived sessions remain covered.

@n13 n13 removed the bot-review label Nov 11, 2026
The previous CI run failed during ubuntu apt-get (Temporary failure
resolving azure.archive.ubuntu.com) before Format ran. No code change;
local fmt --check -p quantus-node is clean.

Co-authored-by: Cursor <cursoragent@cursor.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants