fix: require X-P2P-Key auth on /p2p/gossip POST endpoint (fixes #8177) [fj4WqyCCw3C5ShR1RfB7MoBPTpkRrBFYP1uT35g3MvT] - #8190
Conversation
…rrent mempool corruption (fixes Scottcjn#8176) [fj4WqyCCw3C5ShR1RfB7MoBPTpkRrBFYP1uT35g3MvT]
…cjn#8177) [fj4WqyCCw3C5ShR1RfB7MoBPTpkRrBFYP1uT35g3MvT]
FlintLeng
left a comment
There was a problem hiding this comment.
PR Review: P2P Gossip POST Requires X-P2P-Key Authentication
Reviewed on: 2026-08-07
Summary
Fixes P2P write-path authentication gap: the /p2p/gossip POST endpoint (which feeds CRDT updates into the mempool) had no X-P2P-Key check, only per-IP rate limiting. All other P2P endpoints (GET /p2p/state, /p2p/attestation_state, /p2p/peers) already required authentication. This was a write-path authentication bypass.
Security Analysis ✅
The vulnerability is correctly scoped. A forged CRDT gossip message that reaches apply_gossip() could inject invalid state entries into the mempool. The PR body correctly identifies the impact: unauthenticated write access to the distributed state — the most dangerous class of P2P vulnerability.
Fix is minimal and correct:
auth_error = _require_p2p_read_auth()
if auth_error:
return auth_errorReuse of the existing _require_p2p_read_auth() is the right call — same auth logic for the same secret, consistent across all P2P endpoints.
Note: The PR body references _require_p2p_read_auth() but the endpoint is the write path (POST). Worth verifying that the auth function checks the same X-P2P-Key header as the GET endpoints — the naming suggests it, but confirming prevents a read/write auth gap.
Additional Change: Duplicate #8181 Fix
utxo_db.py also includes the BEGIN IMMEDIATE + rollback fix for mempool_clear_expired() — this is the same fix as PR #8181, which I reviewed on 2026-08-04. The fix is correct and already validated. Merging #8190 will effectively close #8181's fix scope as well.
Minor Note
The PR title references "15 deletions" — that includes the utxo_db.py refactor (flattening the else: block). The deletions are all from the refactor, not from the security fix. No code is removed for security reasons.
Wallet: RTC019e78d600fb3131c29d7ba80aba8fe644be426e
✅ LGTM — clean, targeted auth fix for the P2P write path.
|
Holding this one, and I want to be precise about why, because the finding behind it is correct. You are right that The problem is the other sender. def _send_to_peer(self, peer_url: str, msg: GossipMessage):
resp = requests.post(
f"{peer_url}/p2p/gossip",
json=msg.to_dict(),
timeout=10,
verify=TLS_VERIFY
)No CI does not catch it: this needs two nodes talking to each other, and nothing in the suite exercises that. The branch is green and mergeable, which is exactly why I checked the call path by hand. What would make this mergeable:
Worth flagging separately: this branch is also stacked under #8191 and #8192, so each of those carries this change plus its own. If this one needs a revision, they inherit it. Splitting them onto independent branches off current The gap you found is real and worth fixing. It just needs the sender side to land in the same change. |
/p2p/gossip has two senders. request_full_sync() sends the shared secret; _send_to_peer(), which is the broadcast fan-out path, sent nothing. The receiving endpoint does not require the header today, so the gap is invisible. It stops being invisible the moment anyone enforces on the receiver. #8190 proposes exactly that and adds the header to no sender, so merging it as written would 401 every broadcast message and take inter-node gossip down fleet-wide. CI cannot catch that: it needs two live nodes talking to each other. Sending the header now is inert, since nothing checks it yet, and it turns enforcement from an outage into a config change. The test pins the invariant by walking the AST for requests.post calls targeting /p2p/gossip and asserting each one passes X-P2P-Key, so a future sender added without it fails in CI rather than in production. Signed-off-by: Scott <scottbphone12@gmail.com> Co-authored-by: Scott <scottbphone12@gmail.com>
|
Follow-up: the sender half is now on #8203 adds That removes the outage risk from your change. Rebase onto current One thing still worth deciding in your PR: the rollout order. Even with every sender fixed, a receiver that starts enforcing before all peers are running the new build will cut off the stragglers. Gating enforcement on an env flag, defaulting permissive, makes the cutover an operator decision and reversible without a redeploy. Thanks for finding the gap. |
Fix #8177: /p2p/gossip POST endpoint has no authentication
Problem
All P2P GET endpoints (
/p2p/state,/p2p/attestation_state,/p2p/peers) requireX-P2P-Keyvia_require_p2p_read_auth(). However, the/p2p/gossipPOST endpoint — which is the write endpoint that feeds CRDT updates — has no authentication. Only per-IP rate limiting is applied.Impact: Any network-accessible attacker can POST gossip messages to the node without knowing the P2P secret, injecting forged CRDT updates.
Fix
Add the same
_require_p2p_read_auth()check that all other P2P endpoints use, placed after the rate limit check but before any content-length validation or payload processing.Changes
receive_gossip()innode/rustchain_p2p_gossip.pyTesting
get_state(),get_attestation_state(), andget_peers()