fix(p2p): send X-P2P-Key from every gossip sender, so enforcing it later is not an outage - #8203
Merged
Merged
Conversation
/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>
github-actions
Bot
added
BCOS-L1
Contributor
|
| Metric | Value |
|---|---|
| Trust Score | 49/100 |
| Certificate ID | BCOS-6f0136c5 |
| Tier | L1 (not met) |
What does this mean?
The BCOS (Beacon Certified Open Source) engine scans for:
- SPDX license header compliance
- Known CVE vulnerabilities (OSV database)
- Static analysis findings (Semgrep)
- SBOM completeness
- Dependency freshness
- Test infrastructure evidence
- Review attestation tier
BCOS v2 Engine - Free & Open Source (MIT) - Elyan Labs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Groundwork so that #8190 can land without taking the network down.
The gap
/p2p/gossiphas two senders innode/rustchain_p2p_gossip.py:request_full_sync()at line ~1538 sendsheaders={"X-P2P-Key": P2P_SECRET}_send_to_peer()at line ~894, which is whatbroadcast()fans out through, sends no headers at allThe receiving endpoint does not require the header today, so nothing fails and the inconsistency is invisible.
It stops being invisible the moment anyone enforces on the receiver. #8190 proposes precisely that, adding
_require_p2p_read_auth()to/p2p/gossip, and adds the header to zero senders. Merged as written it would 401 every broadcast message and stop inter-node gossip fleet-wide. The finding behind that PR is correct; only the sender half is missing.This change
Send the header from
_send_to_peer()too. Inert today, because nothing checks it. What it buys is that enforcement becomes a config change rather than an outage: once every node in the fleet is running a build that sends the key, the receiver can require it.The test
CI cannot catch the original problem. It needs two live nodes talking to each other, and nothing in the suite does that. So the invariant is pinned statically instead: the test walks the AST for
requests.post(...)calls whose URL contains/p2p/gossipand asserts each one passesX-P2P-Key. A sender added later without the header fails in CI rather than in production.Reverting the source change fails the test, so it is checking the thing it claims to check.
Verification
Full CI suite: 3729 passed, 46 skipped, 2 xfailed, 0 failed.
Not included
Enforcement on the receiver. That is #8190's change to make, and it should land only after the fleet is running a build that sends the key, ideally behind a flag so the cutover is reversible without a redeploy.