Deuxfleurs/garage
48
56
Fork 73
Code Issues 100 Pull requests 13 Releases 30 Wiki Activity

web_server.rs: Log X-Forwarded-For IP #504

Merged
lx merged 3 commits from jpds/garage:web_server-log-x-forwarded-for into main 2023-03-06 12:33:07 +00:00
Conversation 3 Commits 3 Files changed 4 +82 -10
4 changed files with 82 additions and 10 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

13
src/api/generic_server.rs
View file

@ -19,6 +19,7 @@ use opentelemetry::{
};
use garage_util::error::Error as GarageError;
use garage_util::forwarded_headers;
use garage_util::metrics::{gen_trace_id, RecordDuration};
pub(crate) trait ApiEndpoint: Send + Sync + 'static {
@ -126,15 +127,9 @@ impl<A: ApiHandler> ApiServer<A> {
) -> Result<Response<Body>, GarageError> {
let uri = req.uri().clone();
let has_forwarded_for_header = req.headers().contains_key("x-forwarded-for");
if has_forwarded_for_header {
let forwarded_for_ip_addr = &req
.headers()
.get("x-forwarded-for")
.expect("Could not parse X-Forwarded-For header")
.to_str()
.unwrap_or_default();
if let Ok(forwarded_for_ip_addr) =
forwarded_headers::handle_forwarded_for_headers(&req.headers())
lx commented 2023-03-06 10:18:42 +00:00
Outdated
Review
Copy link

You don't need lines 130-131, just use the second if (the if let statement) with the else branch of the first one.

You don't need lines 130-131, just use the second if (the `if let` statement) with the else branch of the first one.
{
info!(
jpds marked this conversation as resolved Outdated
lx commented 2023-03-01 19:03:54 +00:00
Outdated
Review
Copy link

With handle_forwarded_for_headers returning a Result (see below), lines 130-133 should be rewritten as follows:

if let Ok(forwarded_for_ip_addr) = forwarded_headers::handle_forwarded_for_headers(&req.headers()) {

  info!(...)
With `handle_forwarded_for_headers` returning a `Result` (see below), lines 130-133 should be rewritten as follows: ```rust if let Ok(forwarded_for_ip_addr) = forwarded_headers::handle_forwarded_for_headers(&req.headers()) { info!(...) ```
"{} (via {}) {} {}",
forwarded_for_ip_addr,

63
src/util/forwarded_headers.rs Normal file
View file

@ -0,0 +1,63 @@
use http::{HeaderMap, HeaderValue};
use std::net::IpAddr;
use std::str::FromStr;
jpds marked this conversation as resolved Outdated
lx commented 2023-03-01 19:02:39 +00:00
Outdated
Review
Copy link

This function should return a Result<String, Error>, which is Ok if the value exists and passes all the checks, and Err otherwise.

This function should return a `Result<String, Error>`, which is `Ok` if the value exists and passes all the checks, and `Err` otherwise.
use crate::error::{Error, OkOrMessage};
pub fn handle_forwarded_for_headers(headers: &HeaderMap<HeaderValue>) -> Result<String, Error> {
let forwarded_for_header = headers
.get("x-forwarded-for")
.ok_or_message("X-Forwarded-For header not provided")?;
let forwarded_for_ip_str = forwarded_for_header
.to_str()
.ok_or_message("Error parsing X-Forwarded-For header")?;
let client_ip = IpAddr::from_str(&forwarded_for_ip_str)
.ok_or_message("Valid IP address not found in X-Forwarded-For header")?;
Ok(client_ip.to_string())
}
#[cfg(test)]
mod test {
use super::*;
#[test]
fn test_handle_forwarded_for_headers_ipv4_client() {
let mut test_headers = HeaderMap::new();
test_headers.insert("X-Forwarded-For", "192.0.2.100".parse().unwrap());
if let Ok(forwarded_ip) = handle_forwarded_for_headers(&test_headers) {
assert_eq!(forwarded_ip, "192.0.2.100");
}
}
#[test]
fn test_handle_forwarded_for_headers_ipv6_client() {
let mut test_headers = HeaderMap::new();
test_headers.insert("X-Forwarded-For", "2001:db8::f00d:cafe".parse().unwrap());
if let Ok(forwarded_ip) = handle_forwarded_for_headers(&test_headers) {
assert_eq!(forwarded_ip, "2001:db8::f00d:cafe");
}
}
#[test]
fn test_handle_forwarded_for_headers_invalid_ip() {
let mut test_headers = HeaderMap::new();
test_headers.insert("X-Forwarded-For", "www.example.com".parse().unwrap());
let result = handle_forwarded_for_headers(&test_headers);
assert!(result.is_err());
}
#[test]
fn test_handle_forwarded_for_headers_missing() {
let mut test_headers = HeaderMap::new();
test_headers.insert("Host", "www.deuxfleurs.fr".parse().unwrap());
let result = handle_forwarded_for_headers(&test_headers);
assert!(result.is_err());
}
}

1
src/util/lib.rs
View file

@ -11,6 +11,7 @@ pub mod data;
pub mod encode;
pub mod error;
pub mod formater;
pub mod forwarded_headers;
pub mod metrics;
pub mod migrate;
pub mod persister;

13
src/web/web_server.rs
View file

@ -29,6 +29,7 @@ use garage_model::garage::Garage;
use garage_table::*;
use garage_util::error::Error as GarageError;
use garage_util::forwarded_headers;
use garage_util::metrics::{gen_trace_id, RecordDuration};
struct WebMetrics {
@ -104,7 +105,19 @@ impl WebServer {
req: Request<Body>,
addr: SocketAddr,
) -> Result<Response<Body>, Infallible> {
if let Ok(forwarded_for_ip_addr) =
forwarded_headers::handle_forwarded_for_headers(&req.headers())
lx commented 2023-03-06 10:18:55 +00:00
Outdated
Review
Copy link

Same here

Same here
{
info!(
jpds marked this conversation as resolved Outdated
lx commented 2023-03-01 19:04:35 +00:00
Outdated
Review
Copy link

Same as above, lines 108-111 should be rewritten as:

if let Ok(forwarded_for_ip_addr) = forwarded_headers::handle_forwarded_for_headers(&req.headers()) {

  info!(...)
Same as above, lines 108-111 should be rewritten as: ```rust if let Ok(forwarded_for_ip_addr) = forwarded_headers::handle_forwarded_for_headers(&req.headers()) { info!(...) ```
"{} (via {}) {} {}",
forwarded_for_ip_addr,
addr,
req.method(),
req.uri()
);
} else {
info!("{} {} {}", addr, req.method(), req.uri());
}
// Lots of instrumentation
let tracer = opentelemetry::global::tracer("garage");