mirror of
https://github.com/rustfs/rustfs.git
synced 2026-08-29 00:17:11 +00:00
fix(targets): disable HTTP redirects on webhook delivery client (#4420)
fix(targets): disable HTTP redirects on webhook delivery client (backlog#974) The webhook reqwest client used the default redirect policy (follows up to 10 redirects). Even though the configured endpoint is validated against outbound-egress rules, a malicious or compromised endpoint could return a 3xx response to bounce the outbound request to an internal address (e.g. the cloud metadata service at 169.254.169.254), bypassing that validation (SSRF). - Set `.redirect(reqwest::redirect::Policy::none())` on the delivery client so redirects are never followed. - Treat a 3xx response as a delivery failure (error carries the status code) instead of silently succeeding. - Add a regression test that stands up a loopback server returning a 302 to an internal metadata address and asserts the client surfaces the 3xx rather than following it. Refs: https://github.com/rustfs/backlog/issues/974
This commit is contained in:
@@ -223,6 +223,12 @@ where
|
|||||||
fn build_http_client(args: &WebhookArgs) -> Result<Client, TargetError> {
|
fn build_http_client(args: &WebhookArgs) -> Result<Client, TargetError> {
|
||||||
let mut client_builder = Client::builder()
|
let mut client_builder = Client::builder()
|
||||||
.timeout(Duration::from_secs(30))
|
.timeout(Duration::from_secs(30))
|
||||||
|
// SSRF hardening (backlog#974): never follow HTTP redirects on webhook delivery.
|
||||||
|
// reqwest follows up to 10 redirects by default, which lets a malicious or
|
||||||
|
// compromised endpoint use a 3xx response to bounce the outbound request to an
|
||||||
|
// internal address (e.g. the cloud metadata service at 169.254.169.254),
|
||||||
|
// bypassing the outbound-endpoint validation performed on the configured URL.
|
||||||
|
.redirect(reqwest::redirect::Policy::none())
|
||||||
.user_agent(crate::get_user_agent(crate::ServiceType::Basis));
|
.user_agent(crate::get_user_agent(crate::ServiceType::Basis));
|
||||||
#[cfg(test)]
|
#[cfg(test)]
|
||||||
{
|
{
|
||||||
@@ -458,6 +464,14 @@ where
|
|||||||
);
|
);
|
||||||
self.delivery_counters.record_success();
|
self.delivery_counters.record_success();
|
||||||
Ok(())
|
Ok(())
|
||||||
|
} else if status.is_redirection() {
|
||||||
|
// SSRF hardening (backlog#974): redirects are intentionally not followed
|
||||||
|
// (see build_http_client). Treat a 3xx response as a delivery failure rather
|
||||||
|
// than silently chasing the redirect to a potentially internal target.
|
||||||
|
Err(TargetError::Request(format!(
|
||||||
|
"{} returned redirect '{}'; redirects are not followed for webhook delivery",
|
||||||
|
self.args.endpoint, status
|
||||||
|
)))
|
||||||
} else if status == StatusCode::FORBIDDEN {
|
} else if status == StatusCode::FORBIDDEN {
|
||||||
Err(TargetError::Authentication(format!(
|
Err(TargetError::Authentication(format!(
|
||||||
"{} returned '{}', please check if your auth token is correctly set",
|
"{} returned '{}', please check if your auth token is correctly set",
|
||||||
@@ -833,4 +847,46 @@ mod tests {
|
|||||||
};
|
};
|
||||||
assert!(err.to_string().contains("not allowed"));
|
assert!(err.to_string().contains("not allowed"));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// SSRF hardening regression (backlog#974): the delivery client must not follow HTTP
|
||||||
|
// redirects, otherwise a 3xx from the endpoint could bounce the outbound request to an
|
||||||
|
// internal address (e.g. the cloud metadata service) and bypass endpoint validation.
|
||||||
|
#[tokio::test]
|
||||||
|
async fn test_webhook_client_does_not_follow_redirects() {
|
||||||
|
use std::io::{Read, Write};
|
||||||
|
use std::net::TcpListener;
|
||||||
|
|
||||||
|
// Minimal HTTP server on an ephemeral loopback port.
|
||||||
|
let listener = TcpListener::bind("127.0.0.1:0").expect("bind mock server");
|
||||||
|
let addr = listener.local_addr().expect("local addr");
|
||||||
|
|
||||||
|
// Serve exactly one request with a 302 pointing at an internal metadata address.
|
||||||
|
// If the client followed the redirect it would issue a second request to that
|
||||||
|
// (unreachable) target instead of returning the 3xx status.
|
||||||
|
let handle = std::thread::spawn(move || {
|
||||||
|
if let Ok((mut stream, _)) = listener.accept() {
|
||||||
|
let mut buf = [0u8; 1024];
|
||||||
|
let _ = stream.read(&mut buf);
|
||||||
|
let response =
|
||||||
|
"HTTP/1.1 302 Found\r\nLocation: http://169.254.169.254/latest/meta-data/\r\nContent-Length: 0\r\n\r\n";
|
||||||
|
let _ = stream.write_all(response.as_bytes());
|
||||||
|
let _ = stream.flush();
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
|
let client = WebhookTarget::<serde_json::Value>::build_http_client(&base_args()).expect("build client");
|
||||||
|
|
||||||
|
let resp = client
|
||||||
|
.post(format!("http://{addr}/hook"))
|
||||||
|
.body("{}")
|
||||||
|
.send()
|
||||||
|
.await
|
||||||
|
.expect("request should complete without following the redirect");
|
||||||
|
|
||||||
|
// Redirects are disabled, so the 3xx is surfaced as-is rather than chased to the
|
||||||
|
// internal Location target.
|
||||||
|
assert_eq!(resp.status().as_u16(), 302, "webhook client must not follow redirects");
|
||||||
|
|
||||||
|
handle.join().expect("mock server thread");
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user