Skip to content

Commit 563c456

Browse files
authored
fix(node,gossip): route gossip HTTP through the no-redirect client (#93) (#140)
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).
1 parent 6c95592 commit 563c456

1 file changed

Lines changed: 102 additions & 18 deletions

File tree

‎crates/gitlawb-node/src/main.rs‎

Lines changed: 102 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -145,16 +145,8 @@ async fn main() -> Result<()> {
145145
None
146146
};
147147

148-
// Do not follow redirects: this client fans out to peer-supplied URLs
149-
// (sync trigger, profile/repo fetches). A peer answering `302 Location:
150-
// http://127.0.0.1/` would otherwise bypass the announce-time public-URL
151-
// check and turn the redirect into an SSRF.
152-
let http_client = Arc::new(
153-
reqwest::Client::builder()
154-
.timeout(std::time::Duration::from_secs(10))
155-
.redirect(reqwest::redirect::Policy::none())
156-
.build()?,
157-
);
148+
// Shared no-redirect HTTP client. See build_http_client for the SSRF rationale.
149+
let http_client = Arc::new(build_http_client()?);
158150

159151
let (ref_update_tx, _) = tokio::sync::broadcast::channel::<state::RefUpdateBroadcast>(256);
160152
let (task_event_tx, _) = tokio::sync::broadcast::channel::<state::TaskEventBroadcast>(256);
@@ -515,7 +507,12 @@ async fn gossip_task(
515507
}
516508
}
517509

518-
let client = reqwest::Client::new();
510+
// Reuse the shared no-redirect client for every gossip outbound call (the
511+
// bootstrap announce POST and the periodic peer /health ping). Peer URLs are
512+
// attacker-influenceable, so a 3xx to a private address must not be followed.
513+
// Do NOT fall back to reqwest::Client::new(): its default follows redirects
514+
// and would reintroduce the SSRF closed here (#93).
515+
let client = state.http_client.clone();
519516
let my_did = state.node_did.to_string();
520517
let my_url = state.config.public_url.clone().unwrap_or_default();
521518

@@ -590,13 +587,7 @@ async fn gossip_task(
590587
Err(_) => continue,
591588
};
592589
for peer in peers {
593-
let url = format!("{}/health", peer.http_url.trim_end_matches('/'));
594-
let ok = client
595-
.get(&url)
596-
.send()
597-
.await
598-
.map(|r| r.status().is_success())
599-
.unwrap_or(false);
590+
let ok = ping_peer_health(&client, &peer.http_url).await;
600591
let _ = state.db.mark_peer_ping(&peer.did, ok).await;
601592
}
602593
}
@@ -610,6 +601,38 @@ async fn gossip_task(
610601
}
611602
}
612603

604+
/// Build the shared node HTTP client used for every outbound fan-out (sync
605+
/// trigger, profile/repo fetches, gossip announce + peer pings).
606+
///
607+
/// No redirects: peer URLs are attacker-influenceable, so a `3xx` to a private
608+
/// address must not be followed (SSRF guard, #78/#93). Do NOT replace with
609+
/// `reqwest::Client::new()` — its default follows redirects. Kept as a named
610+
/// builder so tests bind the redirect guarantee to the real client the node
611+
/// runs, not a hand-rolled equivalent.
612+
fn build_http_client() -> reqwest::Result<reqwest::Client> {
613+
reqwest::Client::builder()
614+
.timeout(std::time::Duration::from_secs(10))
615+
.redirect(reqwest::redirect::Policy::none())
616+
.build()
617+
}
618+
619+
/// Ping a peer's `/health` endpoint and report whether it answered 2xx.
620+
///
621+
/// Takes the client by reference so callers supply the shared, no-redirect
622+
/// `state.http_client`. Peer URLs are attacker-influenceable, so a `3xx` to a
623+
/// private address must not be followed. Do NOT call this with a bare
624+
/// `reqwest::Client::new()`: its default follows redirects and would
625+
/// reintroduce the SSRF this guards against (#93).
626+
async fn ping_peer_health(client: &reqwest::Client, http_url: &str) -> bool {
627+
let url = format!("{}/health", http_url.trim_end_matches('/'));
628+
client
629+
.get(&url)
630+
.send()
631+
.await
632+
.map(|r| r.status().is_success())
633+
.unwrap_or(false)
634+
}
635+
613636
fn load_or_create_keypair(config: &Config) -> Result<Keypair> {
614637
let key_path = config.resolved_key_path();
615638

@@ -642,3 +665,64 @@ fn load_or_create_keypair(config: &Config) -> Result<Keypair> {
642665
Ok(kp)
643666
}
644667
}
668+
669+
#[cfg(test)]
670+
mod gossip_ssrf_tests {
671+
use super::ping_peer_health;
672+
673+
// Build the client exactly as production does (super::build_http_client) so
674+
// these tests bind the redirect guarantee to the real shared client the
675+
// node runs. A regression that makes build_http_client follow redirects
676+
// fails ping_peer_health_does_not_follow_redirect.
677+
fn production_http_client() -> reqwest::Client {
678+
super::build_http_client().expect("failed to build production http client")
679+
}
680+
681+
// A peer answering `/health` with a 302 toward an internal address must not
682+
// be followed: the redirect target must never be requested (#93).
683+
#[tokio::test]
684+
async fn ping_peer_health_does_not_follow_redirect() {
685+
let mut server = mockito::Server::new_async().await;
686+
let internal = server
687+
.mock("GET", "/internal-metadata")
688+
.with_status(200)
689+
.expect(0)
690+
.create_async()
691+
.await;
692+
let _health = server
693+
.mock("GET", "/health")
694+
.with_status(302)
695+
.with_header("location", &format!("{}/internal-metadata", server.url()))
696+
.create_async()
697+
.await;
698+
699+
let ok = ping_peer_health(&production_http_client(), &server.url()).await;
700+
701+
assert!(!ok, "a 302 must not count as a healthy peer");
702+
// expect(0) is enforced only at assert time; this fails if the redirect
703+
// was followed to the internal target.
704+
internal.assert_async().await;
705+
}
706+
707+
#[tokio::test]
708+
async fn ping_peer_health_reports_success_on_200() {
709+
let mut server = mockito::Server::new_async().await;
710+
let _health = server
711+
.mock("GET", "/health")
712+
.with_status(200)
713+
.create_async()
714+
.await;
715+
716+
let ok = ping_peer_health(&production_http_client(), &server.url()).await;
717+
718+
assert!(ok, "a 200 /health must count as a healthy peer");
719+
}
720+
721+
// A transport error (nothing listening) must map to unhealthy, never a
722+
// spurious healthy — the .unwrap_or(false) arm.
723+
#[tokio::test]
724+
async fn ping_peer_health_reports_unhealthy_on_connection_error() {
725+
let ok = ping_peer_health(&production_http_client(), "http://127.0.0.1:1").await;
726+
assert!(!ok, "a connection error must count as an unhealthy peer");
727+
}
728+
}

0 commit comments

Comments
 (0)