fix(node,gossip): route gossip HTTP through the no-redirect client (#93)#140
Conversation
gossip_task built its own reqwest::Client::new(), whose default redirect policy follows up to 10 redirects, and used it for the bootstrap announce and the periodic peer /health ping. A peer host (public at announce time, per is_public_http_url) could answer /health with 302 Location: http://169.254.169.254/... and the node would follow it into cloud metadata or internal addresses. Blind SSRF: only request status feeds mark_peer_ping. Reuse the shared state.http_client (Policy::none, already on AppState), matching sync.rs and the announce fan-out hardened in #78. Extract ping_peer_health so the redirect-not-followed behavior is covered by a mockito test (302 to an internal target registered expect(0), asserted not hit).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR centralizes HTTP client construction via a new ChangesSSRF hardening of outbound gossip HTTP client
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant gossip_task
participant ping_peer_health
participant state.http_client
participant peer.health endpoint
gossip_task->>ping_peer_health: ping_peer_health(&client, peer.http_url)
ping_peer_health->>state.http_client: GET {peer.http_url}/health
state.http_client->>peer.health endpoint: HTTP request
peer.health endpoint-->>state.http_client: 200 or 302 Location
state.http_client-->>ping_peer_health: response or error
ping_peer_health-->>gossip_task: is_success() boolean
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the contribution. I do not see any actionable issues from my review.
@kevincodex1 LGTM
Closes #93.
gossip_taskbuilt its ownreqwest::Client::new(), which follows up to 10 redirects by default, and used it for the bootstrap announce and the periodic peer/healthping. A peer host (public at announce time peris_public_http_url) can answer/healthwith a302pointing at a link-local or internal address, and the node follows it. So an unauthenticated announce becomes a blind SSRF, one outbound request to an attacker-chosen internal target per gossip cycle.The fix reuses the shared
state.http_client, which is already built withredirect::Policy::none(), the same guardsync.rsand the announce fan-out got in #78. The ping is extracted intoping_peer_healthso the redirect behavior is testable, and the shared client's construction is pulled intobuild_http_clientso the tests exercise the exact client the node runs rather than a hand-rolled copy.Tests: a
302to an internal mock (registeredexpect(0)) is not followed, a200is healthy, a connection error is unhealthy. The redirect guard is load-bearing against the production builder (flippingbuild_http_clientto follow redirects makes it fail). Full crate suite green, clippy-D warningsclean.One thing not covered by a test: that gossip's call site reads
state.http_clientrather than a fresh client. Driving the real gossip loop needs a mock peer, but mockito binds to127.0.0.1and bothupsert_peerandlist_peersreject non-public hosts, so a local mock never enters the loop. That call site is guarded by a comment against reintroducingClient::new(); the redirect policy itself is test-bound viabuild_http_client.Summary by CodeRabbit