From 637f25f25b8449fd7b5bc7a968da7d92bcef1484 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 7 Mar 2023 14:02:51 +0000 Subject: [PATCH 1/5] refactor(http): move auth mod to handlers::common The `auth` mod does not contains a handler. It only contains auth error and funtion to ma the error into responses. --- src/http/axum_implementation/extractors/key.rs | 2 +- src/http/axum_implementation/handlers/announce.rs | 2 +- src/http/axum_implementation/handlers/{ => common}/auth.rs | 0 src/http/axum_implementation/handlers/common/mod.rs | 1 + src/http/axum_implementation/handlers/mod.rs | 1 - 5 files changed, 3 insertions(+), 3 deletions(-) rename src/http/axum_implementation/handlers/{ => common}/auth.rs (100%) diff --git a/src/http/axum_implementation/extractors/key.rs b/src/http/axum_implementation/extractors/key.rs index 2a3f2a991..e32c4c76a 100644 --- a/src/http/axum_implementation/extractors/key.rs +++ b/src/http/axum_implementation/extractors/key.rs @@ -7,7 +7,7 @@ use axum::extract::{FromRequestParts, Path}; use axum::http::request::Parts; use axum::response::{IntoResponse, Response}; -use crate::http::axum_implementation::handlers::auth::{self, KeyParam}; +use crate::http::axum_implementation::handlers::common::auth::{self, KeyParam}; use crate::http::axum_implementation::responses; use crate::tracker::auth::Key; diff --git a/src/http/axum_implementation/handlers/announce.rs b/src/http/axum_implementation/handlers/announce.rs index e4b5ece80..33f78814f 100644 --- a/src/http/axum_implementation/handlers/announce.rs +++ b/src/http/axum_implementation/handlers/announce.rs @@ -11,7 +11,7 @@ use super::common::peer_ip; use crate::http::axum_implementation::extractors::announce_request::ExtractRequest; use crate::http::axum_implementation::extractors::key::Extract; use crate::http::axum_implementation::extractors::remote_client_ip::RemoteClientIp; -use crate::http::axum_implementation::handlers::auth; +use crate::http::axum_implementation::handlers::common::auth; use crate::http::axum_implementation::requests::announce::{Announce, Compact, Event}; use crate::http::axum_implementation::responses::{self, announce}; use crate::http::axum_implementation::services; diff --git a/src/http/axum_implementation/handlers/auth.rs b/src/http/axum_implementation/handlers/common/auth.rs similarity index 100% rename from src/http/axum_implementation/handlers/auth.rs rename to src/http/axum_implementation/handlers/common/auth.rs diff --git a/src/http/axum_implementation/handlers/common/mod.rs b/src/http/axum_implementation/handlers/common/mod.rs index ed159a32b..41bf1369f 100644 --- a/src/http/axum_implementation/handlers/common/mod.rs +++ b/src/http/axum_implementation/handlers/common/mod.rs @@ -1 +1,2 @@ pub mod peer_ip; +pub mod auth; diff --git a/src/http/axum_implementation/handlers/mod.rs b/src/http/axum_implementation/handlers/mod.rs index 36a810d95..69b69127e 100644 --- a/src/http/axum_implementation/handlers/mod.rs +++ b/src/http/axum_implementation/handlers/mod.rs @@ -2,7 +2,6 @@ use super::responses; use crate::tracker::error::Error; pub mod announce; -pub mod auth; pub mod common; pub mod scrape; From 49bb0db5b2958bcf19cc4b3eab40f0ee4e317d0b Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 7 Mar 2023 14:33:33 +0000 Subject: [PATCH 2/5] refactor(http): rename mod and move struct --- .../extractors/{key.rs => authentication_key.rs} | 13 ++++++++++++- src/http/axum_implementation/extractors/mod.rs | 2 +- src/http/axum_implementation/handlers/announce.rs | 2 +- .../axum_implementation/handlers/common/auth.rs | 11 ----------- src/http/axum_implementation/handlers/scrape.rs | 2 +- 5 files changed, 15 insertions(+), 15 deletions(-) rename src/http/axum_implementation/extractors/{key.rs => authentication_key.rs} (92%) diff --git a/src/http/axum_implementation/extractors/key.rs b/src/http/axum_implementation/extractors/authentication_key.rs similarity index 92% rename from src/http/axum_implementation/extractors/key.rs rename to src/http/axum_implementation/extractors/authentication_key.rs index e32c4c76a..8ffc4ff12 100644 --- a/src/http/axum_implementation/extractors/key.rs +++ b/src/http/axum_implementation/extractors/authentication_key.rs @@ -6,13 +6,24 @@ use axum::extract::rejection::PathRejection; use axum::extract::{FromRequestParts, Path}; use axum::http::request::Parts; use axum::response::{IntoResponse, Response}; +use serde::Deserialize; -use crate::http::axum_implementation::handlers::common::auth::{self, KeyParam}; +use crate::http::axum_implementation::handlers::common::auth; use crate::http::axum_implementation::responses; use crate::tracker::auth::Key; pub struct Extract(pub Key); +#[derive(Deserialize)] +pub struct KeyParam(String); + +impl KeyParam { + #[must_use] + pub fn value(&self) -> String { + self.0.clone() + } +} + #[async_trait] impl FromRequestParts for Extract where diff --git a/src/http/axum_implementation/extractors/mod.rs b/src/http/axum_implementation/extractors/mod.rs index 04e9e306b..97aae63a5 100644 --- a/src/http/axum_implementation/extractors/mod.rs +++ b/src/http/axum_implementation/extractors/mod.rs @@ -1,4 +1,4 @@ pub mod announce_request; -pub mod key; +pub mod authentication_key; pub mod remote_client_ip; pub mod scrape_request; diff --git a/src/http/axum_implementation/handlers/announce.rs b/src/http/axum_implementation/handlers/announce.rs index 33f78814f..18787737f 100644 --- a/src/http/axum_implementation/handlers/announce.rs +++ b/src/http/axum_implementation/handlers/announce.rs @@ -9,7 +9,7 @@ use log::debug; use super::common::peer_ip; use crate::http::axum_implementation::extractors::announce_request::ExtractRequest; -use crate::http::axum_implementation::extractors::key::Extract; +use crate::http::axum_implementation::extractors::authentication_key::Extract; use crate::http::axum_implementation::extractors::remote_client_ip::RemoteClientIp; use crate::http::axum_implementation::handlers::common::auth; use crate::http::axum_implementation::requests::announce::{Announce, Compact, Event}; diff --git a/src/http/axum_implementation/handlers/common/auth.rs b/src/http/axum_implementation/handlers/common/auth.rs index b1b73e60e..30971725a 100644 --- a/src/http/axum_implementation/handlers/common/auth.rs +++ b/src/http/axum_implementation/handlers/common/auth.rs @@ -1,21 +1,10 @@ use std::panic::Location; -use serde::Deserialize; use thiserror::Error; use crate::http::axum_implementation::responses; use crate::tracker::auth; -#[derive(Deserialize)] -pub struct KeyParam(String); - -impl KeyParam { - #[must_use] - pub fn value(&self) -> String { - self.0.clone() - } -} - #[derive(Debug, Error)] pub enum Error { #[error("Missing authentication key param for private tracker. Error in {location}")] diff --git a/src/http/axum_implementation/handlers/scrape.rs b/src/http/axum_implementation/handlers/scrape.rs index d8d68a4c3..b65fa5592 100644 --- a/src/http/axum_implementation/handlers/scrape.rs +++ b/src/http/axum_implementation/handlers/scrape.rs @@ -5,7 +5,7 @@ use axum::response::{IntoResponse, Response}; use log::debug; use super::common::peer_ip; -use crate::http::axum_implementation::extractors::key::Extract; +use crate::http::axum_implementation::extractors::authentication_key::Extract; use crate::http::axum_implementation::extractors::remote_client_ip::RemoteClientIp; use crate::http::axum_implementation::extractors::scrape_request::ExtractRequest; use crate::http::axum_implementation::requests::scrape::Scrape; From 6ebcfcdcd78b4011a823e613362748420eead674 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 7 Mar 2023 15:36:31 +0000 Subject: [PATCH 3/5] refactor(http): push logic from Axum to App layer Some app logic was coupled to Axum and it could be potencially used with any other web library. Besides, it's easier to test. --- ...mote_client_ip.rs => client_ip_sources.rs} | 29 +--- .../axum_implementation/extractors/mod.rs | 2 +- .../axum_implementation/handlers/announce.rs | 29 ++-- .../handlers/common/peer_ip.rs | 164 ++---------------- .../axum_implementation/handlers/scrape.rs | 37 ++-- src/http/axum_implementation/services/mod.rs | 1 + .../services/peer_ip_resolver.rs | 149 ++++++++++++++++ src/tracker/error.rs | 2 + 8 files changed, 212 insertions(+), 201 deletions(-) rename src/http/axum_implementation/extractors/{remote_client_ip.rs => client_ip_sources.rs} (51%) create mode 100644 src/http/axum_implementation/services/peer_ip_resolver.rs diff --git a/src/http/axum_implementation/extractors/remote_client_ip.rs b/src/http/axum_implementation/extractors/client_ip_sources.rs similarity index 51% rename from src/http/axum_implementation/extractors/remote_client_ip.rs rename to src/http/axum_implementation/extractors/client_ip_sources.rs index 0f6789261..b41478c22 100644 --- a/src/http/axum_implementation/extractors/remote_client_ip.rs +++ b/src/http/axum_implementation/extractors/client_ip_sources.rs @@ -1,34 +1,19 @@ //! Wrapper for two Axum extractors to get the relevant information //! to resolve the remote client IP. -use std::net::{IpAddr, SocketAddr}; +use std::net::SocketAddr; use axum::async_trait; use axum::extract::{ConnectInfo, FromRequestParts}; use axum::http::request::Parts; use axum::response::Response; use axum_client_ip::RightmostXForwardedFor; -use serde::{Deserialize, Serialize}; -/// Given this request chain: -/// -/// client <-> http proxy 1 <-> http proxy 2 <-> server -/// ip: 126.0.0.1 ip: 126.0.0.2 ip: 126.0.0.3 ip: 126.0.0.4 -/// X-Forwarded-For: 126.0.0.1 X-Forwarded-For: 126.0.0.1,126.0.0.2 -/// -/// This extractor extracts these values from the HTTP headers and connection info. -/// -/// `right_most_x_forwarded_for` = 126.0.0.2 -/// `connection_info_ip` = 126.0.0.3 -/// -/// More info about inner extractors: -#[derive(Serialize, Deserialize, Debug, PartialEq, Eq, Clone)] -pub struct RemoteClientIp { - pub right_most_x_forwarded_for: Option, - pub connection_info_ip: Option, -} +use crate::http::axum_implementation::services::peer_ip_resolver::ClientIpSources; + +pub struct Extract(pub ClientIpSources); #[async_trait] -impl FromRequestParts for RemoteClientIp +impl FromRequestParts for Extract where S: Send + Sync, { @@ -45,9 +30,9 @@ where Err(_) => None, }; - Ok(RemoteClientIp { + Ok(Extract(ClientIpSources { right_most_x_forwarded_for, connection_info_ip, - }) + })) } } diff --git a/src/http/axum_implementation/extractors/mod.rs b/src/http/axum_implementation/extractors/mod.rs index 97aae63a5..557330257 100644 --- a/src/http/axum_implementation/extractors/mod.rs +++ b/src/http/axum_implementation/extractors/mod.rs @@ -1,4 +1,4 @@ pub mod announce_request; pub mod authentication_key; -pub mod remote_client_ip; +pub mod client_ip_sources; pub mod scrape_request; diff --git a/src/http/axum_implementation/handlers/announce.rs b/src/http/axum_implementation/handlers/announce.rs index 18787737f..05216ce28 100644 --- a/src/http/axum_implementation/handlers/announce.rs +++ b/src/http/axum_implementation/handlers/announce.rs @@ -7,23 +7,28 @@ use axum::extract::State; use axum::response::{IntoResponse, Response}; use log::debug; -use super::common::peer_ip; use crate::http::axum_implementation::extractors::announce_request::ExtractRequest; -use crate::http::axum_implementation::extractors::authentication_key::Extract; -use crate::http::axum_implementation::extractors::remote_client_ip::RemoteClientIp; +use crate::http::axum_implementation::extractors::authentication_key::Extract as ExtractKey; +use crate::http::axum_implementation::extractors::client_ip_sources::Extract as ExtractClientIpSources; use crate::http::axum_implementation::handlers::common::auth; use crate::http::axum_implementation::requests::announce::{Announce, Compact, Event}; use crate::http::axum_implementation::responses::{self, announce}; -use crate::http::axum_implementation::services; +use crate::http::axum_implementation::services::peer_ip_resolver::ClientIpSources; +use crate::http::axum_implementation::services::{self, peer_ip_resolver}; use crate::protocol::clock::{Current, Time}; use crate::tracker::peer::Peer; use crate::tracker::Tracker; +/* code-review: authentication, authorization and peer IP resolution could be moved + from the handler (Axum) layer into the app layer `services::announce::invoke`. + That would make the handler even simpler and the code more reusable and decoupled from Axum. +*/ + #[allow(clippy::unused_async)] pub async fn handle_without_key( State(tracker): State>, ExtractRequest(announce_request): ExtractRequest, - remote_client_ip: RemoteClientIp, + ExtractClientIpSources(client_ip_sources): ExtractClientIpSources, ) -> Response { debug!("http announce request: {:#?}", announce_request); @@ -34,15 +39,15 @@ pub async fn handle_without_key( .into_response(); } - handle(&tracker, &announce_request, &remote_client_ip).await + handle(&tracker, &announce_request, &client_ip_sources).await } #[allow(clippy::unused_async)] pub async fn handle_with_key( State(tracker): State>, ExtractRequest(announce_request): ExtractRequest, - Extract(key): Extract, - remote_client_ip: RemoteClientIp, + ExtractClientIpSources(client_ip_sources): ExtractClientIpSources, + ExtractKey(key): ExtractKey, ) -> Response { debug!("http announce request: {:#?}", announce_request); @@ -51,18 +56,18 @@ pub async fn handle_with_key( Err(error) => return responses::error::Error::from(error).into_response(), } - handle(&tracker, &announce_request, &remote_client_ip).await + handle(&tracker, &announce_request, &client_ip_sources).await } -async fn handle(tracker: &Arc, announce_request: &Announce, remote_client_ip: &RemoteClientIp) -> Response { +async fn handle(tracker: &Arc, announce_request: &Announce, client_ip_sources: &ClientIpSources) -> Response { match tracker.authorize(&announce_request.info_hash).await { Ok(_) => (), Err(error) => return responses::error::Error::from(error).into_response(), } - let peer_ip = match peer_ip::resolve(tracker.config.on_reverse_proxy, remote_client_ip) { + let peer_ip = match peer_ip_resolver::invoke(tracker.config.on_reverse_proxy, client_ip_sources) { Ok(peer_ip) => peer_ip, - Err(err) => return err, + Err(error) => return responses::error::Error::from(error).into_response(), }; let mut peer = peer_from_request(announce_request, &peer_ip); diff --git a/src/http/axum_implementation/handlers/common/peer_ip.rs b/src/http/axum_implementation/handlers/common/peer_ip.rs index 1c3b6c815..df10e5eb1 100644 --- a/src/http/axum_implementation/handlers/common/peer_ip.rs +++ b/src/http/axum_implementation/handlers/common/peer_ip.rs @@ -1,170 +1,34 @@ -//! Helper handler function to resolve the peer IP from the `RemoteClientIp` extractor. -use std::net::IpAddr; -use std::panic::Location; - -use axum::response::{IntoResponse, Response}; -use thiserror::Error; - -use crate::http::axum_implementation::extractors::remote_client_ip::RemoteClientIp; use crate::http::axum_implementation::responses; +use crate::http::axum_implementation::services::peer_ip_resolver::PeerIpResolutionError; -#[derive(Error, Debug)] -pub enum ResolutionError { - #[error( - "missing or invalid the right most X-Forwarded-For IP (mandatory on reverse proxy tracker configuration) in {location}" - )] - MissingRightMostXForwardedForIp { location: &'static Location<'static> }, - #[error("cannot get the client IP from the connection info in {location}")] - MissingClientIp { location: &'static Location<'static> }, -} - -impl From for responses::error::Error { - fn from(err: ResolutionError) -> Self { +impl From for responses::error::Error { + fn from(err: PeerIpResolutionError) -> Self { responses::error::Error { failure_reason: format!("Error resolving peer IP: {err}"), } } } -/// It resolves the peer IP. -/// -/// # Errors -/// -/// Will return an error response if the peer IP cannot be obtained according to the configuration. -/// For example, if the IP is extracted from an HTTP header which is missing in the request. -pub fn resolve(on_reverse_proxy: bool, remote_client_ip: &RemoteClientIp) -> Result { - match resolve_peer_ip(on_reverse_proxy, remote_client_ip) { - Ok(ip) => Ok(ip), - Err(error) => Err(error.into_response()), - } -} - -fn resolve_peer_ip(on_reverse_proxy: bool, remote_client_ip: &RemoteClientIp) -> Result { - if on_reverse_proxy { - resolve_peer_ip_on_reverse_proxy(remote_client_ip) - } else { - resolve_peer_ip_without_reverse_proxy(remote_client_ip) - } -} - -fn resolve_peer_ip_without_reverse_proxy(remote_client_ip: &RemoteClientIp) -> Result { - if let Some(ip) = remote_client_ip.connection_info_ip { - Ok(ip) - } else { - Err(responses::error::Error::from(ResolutionError::MissingClientIp { - location: Location::caller(), - })) - } -} - -fn resolve_peer_ip_on_reverse_proxy(remote_client_ip: &RemoteClientIp) -> Result { - if let Some(ip) = remote_client_ip.right_most_x_forwarded_for { - Ok(ip) - } else { - Err(responses::error::Error::from( - ResolutionError::MissingRightMostXForwardedForIp { - location: Location::caller(), - }, - )) - } -} - #[cfg(test)] mod tests { - use super::resolve_peer_ip; - use crate::http::axum_implementation::responses::error::Error; + use std::panic::Location; - fn assert_error_response(error: &Error, error_message: &str) { + use crate::http::axum_implementation::responses; + use crate::http::axum_implementation::services::peer_ip_resolver::PeerIpResolutionError; + + fn assert_error_response(error: &responses::error::Error, error_message: &str) { assert!( error.failure_reason.contains(error_message), "Error response does not contain message: '{error_message}'. Error: {error:?}" ); } - mod working_without_reverse_proxy { - use std::net::IpAddr; - use std::str::FromStr; - - use super::{assert_error_response, resolve_peer_ip}; - use crate::http::axum_implementation::extractors::remote_client_ip::RemoteClientIp; - - #[test] - fn it_should_get_the_peer_ip_from_the_connection_info() { - let on_reverse_proxy = false; - - let ip = resolve_peer_ip( - on_reverse_proxy, - &RemoteClientIp { - right_most_x_forwarded_for: None, - connection_info_ip: Some(IpAddr::from_str("203.0.113.195").unwrap()), - }, - ) - .unwrap(); - - assert_eq!(ip, IpAddr::from_str("203.0.113.195").unwrap()); - } - - #[test] - fn it_should_return_an_error_if_it_cannot_get_the_peer_ip_from_the_connection_info() { - let on_reverse_proxy = false; - - let response = resolve_peer_ip( - on_reverse_proxy, - &RemoteClientIp { - right_most_x_forwarded_for: None, - connection_info_ip: None, - }, - ) - .unwrap_err(); - - assert_error_response( - &response, - "Error resolving peer IP: cannot get the client IP from the connection info", - ); - } - } - - mod working_on_reverse_proxy { - use std::net::IpAddr; - use std::str::FromStr; - - use super::assert_error_response; - use crate::http::axum_implementation::extractors::remote_client_ip::RemoteClientIp; - use crate::http::axum_implementation::handlers::common::peer_ip::resolve_peer_ip; - - #[test] - fn it_should_get_the_peer_ip_from_the_right_most_ip_in_the_x_forwarded_for_header() { - let on_reverse_proxy = true; - - let ip = resolve_peer_ip( - on_reverse_proxy, - &RemoteClientIp { - right_most_x_forwarded_for: Some(IpAddr::from_str("203.0.113.195").unwrap()), - connection_info_ip: None, - }, - ) - .unwrap(); - - assert_eq!(ip, IpAddr::from_str("203.0.113.195").unwrap()); - } - - #[test] - fn it_should_return_an_error_if_it_cannot_get_the_right_most_ip_from_the_x_forwarded_for_header() { - let on_reverse_proxy = true; - - let response = resolve_peer_ip( - on_reverse_proxy, - &RemoteClientIp { - right_most_x_forwarded_for: None, - connection_info_ip: None, - }, - ) - .unwrap_err(); + #[test] + fn it_should_map_a_peer_ip_resolution_error_into_an_error_response() { + let response = responses::error::Error::from(PeerIpResolutionError::MissingRightMostXForwardedForIp { + location: Location::caller(), + }); - assert_error_response( - &response, - "Error resolving peer IP: missing or invalid the right most X-Forwarded-For IP", - ); - } + assert_error_response(&response, "Error resolving peer IP"); } } diff --git a/src/http/axum_implementation/handlers/scrape.rs b/src/http/axum_implementation/handlers/scrape.rs index b65fa5592..2027b8604 100644 --- a/src/http/axum_implementation/handlers/scrape.rs +++ b/src/http/axum_implementation/handlers/scrape.rs @@ -4,50 +4,55 @@ use axum::extract::State; use axum::response::{IntoResponse, Response}; use log::debug; -use super::common::peer_ip; -use crate::http::axum_implementation::extractors::authentication_key::Extract; -use crate::http::axum_implementation::extractors::remote_client_ip::RemoteClientIp; +use crate::http::axum_implementation::extractors::authentication_key::Extract as ExtractKey; +use crate::http::axum_implementation::extractors::client_ip_sources::Extract as ExtractClientIpSources; use crate::http::axum_implementation::extractors::scrape_request::ExtractRequest; use crate::http::axum_implementation::requests::scrape::Scrape; +use crate::http::axum_implementation::services::peer_ip_resolver::{self, ClientIpSources}; use crate::http::axum_implementation::{responses, services}; use crate::tracker::Tracker; +/* code-review: authentication, authorization and peer IP resolution could be moved + from the handler (Axum) layer into the app layer `services::announce::invoke`. + That would make the handler even simpler and the code more reusable and decoupled from Axum. +*/ + #[allow(clippy::unused_async)] pub async fn handle_without_key( State(tracker): State>, ExtractRequest(scrape_request): ExtractRequest, - remote_client_ip: RemoteClientIp, + ExtractClientIpSources(client_ip_sources): ExtractClientIpSources, ) -> Response { debug!("http scrape request: {:#?}", &scrape_request); if tracker.requires_authentication() { - return handle_fake_scrape(&tracker, &scrape_request, &remote_client_ip).await; + return handle_fake_scrape(&tracker, &scrape_request, &client_ip_sources).await; } - handle_real_scrape(&tracker, &scrape_request, &remote_client_ip).await + handle_real_scrape(&tracker, &scrape_request, &client_ip_sources).await } #[allow(clippy::unused_async)] pub async fn handle_with_key( State(tracker): State>, ExtractRequest(scrape_request): ExtractRequest, - Extract(key): Extract, - remote_client_ip: RemoteClientIp, + ExtractClientIpSources(client_ip_sources): ExtractClientIpSources, + ExtractKey(key): ExtractKey, ) -> Response { debug!("http scrape request: {:#?}", &scrape_request); match tracker.authenticate(&key).await { Ok(_) => (), - Err(_) => return handle_fake_scrape(&tracker, &scrape_request, &remote_client_ip).await, + Err(_) => return handle_fake_scrape(&tracker, &scrape_request, &client_ip_sources).await, } - handle_real_scrape(&tracker, &scrape_request, &remote_client_ip).await + handle_real_scrape(&tracker, &scrape_request, &client_ip_sources).await } -async fn handle_real_scrape(tracker: &Arc, scrape_request: &Scrape, remote_client_ip: &RemoteClientIp) -> Response { - let peer_ip = match peer_ip::resolve(tracker.config.on_reverse_proxy, remote_client_ip) { +async fn handle_real_scrape(tracker: &Arc, scrape_request: &Scrape, client_ip_sources: &ClientIpSources) -> Response { + let peer_ip = match peer_ip_resolver::invoke(tracker.config.on_reverse_proxy, client_ip_sources) { Ok(peer_ip) => peer_ip, - Err(err) => return err, + Err(error) => return responses::error::Error::from(error).into_response(), }; let scrape_data = services::scrape::invoke(tracker, &scrape_request.info_hashes, &peer_ip).await; @@ -56,10 +61,10 @@ async fn handle_real_scrape(tracker: &Arc, scrape_request: &Scrape, rem } /// When authentication fails in `private` mode the tracker returns empty swarm metadata for all the requested infohashes. -async fn handle_fake_scrape(tracker: &Arc, scrape_request: &Scrape, remote_client_ip: &RemoteClientIp) -> Response { - let peer_ip = match peer_ip::resolve(tracker.config.on_reverse_proxy, remote_client_ip) { +async fn handle_fake_scrape(tracker: &Arc, scrape_request: &Scrape, remote_client_ip: &ClientIpSources) -> Response { + let peer_ip = match peer_ip_resolver::invoke(tracker.config.on_reverse_proxy, remote_client_ip) { Ok(peer_ip) => peer_ip, - Err(err) => return err, + Err(error) => return responses::error::Error::from(error).into_response(), }; let scrape_data = services::scrape::fake_invoke(tracker, &scrape_request.info_hashes, &peer_ip).await; diff --git a/src/http/axum_implementation/services/mod.rs b/src/http/axum_implementation/services/mod.rs index 776d2dfbf..5d1acd67d 100644 --- a/src/http/axum_implementation/services/mod.rs +++ b/src/http/axum_implementation/services/mod.rs @@ -1,2 +1,3 @@ pub mod announce; +pub mod peer_ip_resolver; pub mod scrape; diff --git a/src/http/axum_implementation/services/peer_ip_resolver.rs b/src/http/axum_implementation/services/peer_ip_resolver.rs new file mode 100644 index 000000000..fae1e4ec0 --- /dev/null +++ b/src/http/axum_implementation/services/peer_ip_resolver.rs @@ -0,0 +1,149 @@ +//! Given this request chain: +//! +//! client <-> http proxy 1 <-> http proxy 2 <-> server +//! ip: 126.0.0.1 ip: 126.0.0.2 ip: 126.0.0.3 ip: 126.0.0.4 +//! X-Forwarded-For: 126.0.0.1 X-Forwarded-For: 126.0.0.1,126.0.0.2 +//! +//! This service resolves the peer IP from these values: +//! +//! `right_most_x_forwarded_for` = 126.0.0.2 +//! `connection_info_ip` = 126.0.0.3 +//! +//! Depending on the tracker configuration. +use std::net::IpAddr; +use std::panic::Location; + +use serde::{Deserialize, Serialize}; +use thiserror::Error; + +#[derive(Serialize, Deserialize, Debug, PartialEq, Eq, Clone)] +pub struct ClientIpSources { + pub right_most_x_forwarded_for: Option, + pub connection_info_ip: Option, +} + +#[derive(Error, Debug)] +pub enum PeerIpResolutionError { + #[error( + "missing or invalid the right most X-Forwarded-For IP (mandatory on reverse proxy tracker configuration) in {location}" + )] + MissingRightMostXForwardedForIp { location: &'static Location<'static> }, + #[error("cannot get the client IP from the connection info in {location}")] + MissingClientIp { location: &'static Location<'static> }, +} + +/// # Errors +/// +/// Will return an error if the peer IP cannot be obtained according to the configuration. +/// For example, if the IP is extracted from an HTTP header which is missing in the request. +pub fn invoke(on_reverse_proxy: bool, client_ip_sources: &ClientIpSources) -> Result { + if on_reverse_proxy { + resolve_peer_ip_on_reverse_proxy(client_ip_sources) + } else { + resolve_peer_ip_without_reverse_proxy(client_ip_sources) + } +} + +fn resolve_peer_ip_without_reverse_proxy(remote_client_ip: &ClientIpSources) -> Result { + if let Some(ip) = remote_client_ip.connection_info_ip { + Ok(ip) + } else { + Err(PeerIpResolutionError::MissingClientIp { + location: Location::caller(), + }) + } +} + +fn resolve_peer_ip_on_reverse_proxy(remote_client_ip: &ClientIpSources) -> Result { + if let Some(ip) = remote_client_ip.right_most_x_forwarded_for { + Ok(ip) + } else { + Err(PeerIpResolutionError::MissingRightMostXForwardedForIp { + location: Location::caller(), + }) + } +} + +#[cfg(test)] +mod tests { + use super::invoke; + + mod working_without_reverse_proxy { + use std::net::IpAddr; + use std::str::FromStr; + + use super::invoke; + use crate::http::axum_implementation::services::peer_ip_resolver::{ClientIpSources, PeerIpResolutionError}; + + #[test] + fn it_should_get_the_peer_ip_from_the_connection_info() { + let on_reverse_proxy = false; + + let ip = invoke( + on_reverse_proxy, + &ClientIpSources { + right_most_x_forwarded_for: None, + connection_info_ip: Some(IpAddr::from_str("203.0.113.195").unwrap()), + }, + ) + .unwrap(); + + assert_eq!(ip, IpAddr::from_str("203.0.113.195").unwrap()); + } + + #[test] + fn it_should_return_an_error_if_it_cannot_get_the_peer_ip_from_the_connection_info() { + let on_reverse_proxy = false; + + let error = invoke( + on_reverse_proxy, + &ClientIpSources { + right_most_x_forwarded_for: None, + connection_info_ip: None, + }, + ) + .unwrap_err(); + + assert!(matches!(error, PeerIpResolutionError::MissingClientIp { .. })); + } + } + + mod working_on_reverse_proxy { + use std::net::IpAddr; + use std::str::FromStr; + + use crate::http::axum_implementation::services::peer_ip_resolver::{invoke, ClientIpSources, PeerIpResolutionError}; + + #[test] + fn it_should_get_the_peer_ip_from_the_right_most_ip_in_the_x_forwarded_for_header() { + let on_reverse_proxy = true; + + let ip = invoke( + on_reverse_proxy, + &ClientIpSources { + right_most_x_forwarded_for: Some(IpAddr::from_str("203.0.113.195").unwrap()), + connection_info_ip: None, + }, + ) + .unwrap(); + + assert_eq!(ip, IpAddr::from_str("203.0.113.195").unwrap()); + } + + #[test] + fn it_should_return_an_error_if_it_cannot_get_the_right_most_ip_from_the_x_forwarded_for_header() { + let on_reverse_proxy = true; + + let error = invoke( + on_reverse_proxy, + &ClientIpSources { + right_most_x_forwarded_for: None, + connection_info_ip: None, + }, + ) + .unwrap_err(); + + assert!(matches!(error, PeerIpResolutionError::MissingRightMostXForwardedForIp { .. })); + } + } +} diff --git a/src/tracker/error.rs b/src/tracker/error.rs index 51bcbf3bb..080903da6 100644 --- a/src/tracker/error.rs +++ b/src/tracker/error.rs @@ -4,6 +4,7 @@ use crate::located_error::LocatedError; #[derive(thiserror::Error, Debug, Clone)] pub enum Error { + // Authentication errors #[error("The supplied key: {key:?}, is not valid: {source}")] PeerKeyNotValid { key: super::auth::Key, @@ -12,6 +13,7 @@ pub enum Error { #[error("The peer is not authenticated, {location}")] PeerNotAuthenticated { location: &'static Location<'static> }, + // Authorization errors #[error("The torrent: {info_hash}, is not whitelisted, {location}")] TorrentNotWhitelisted { info_hash: crate::protocol::info_hash::InfoHash, From fa609949d407f3ab36f114f65741ce0beaace137 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 7 Mar 2023 17:58:21 +0000 Subject: [PATCH 4/5] test(http): [#128] unit test for announce handler --- .../axum_implementation/handlers/announce.rs | 279 ++++++++++++++++-- 1 file changed, 257 insertions(+), 22 deletions(-) diff --git a/src/http/axum_implementation/handlers/announce.rs b/src/http/axum_implementation/handlers/announce.rs index 05216ce28..9a92b243d 100644 --- a/src/http/axum_implementation/handlers/announce.rs +++ b/src/http/axum_implementation/handlers/announce.rs @@ -16,13 +16,9 @@ use crate::http::axum_implementation::responses::{self, announce}; use crate::http::axum_implementation::services::peer_ip_resolver::ClientIpSources; use crate::http::axum_implementation::services::{self, peer_ip_resolver}; use crate::protocol::clock::{Current, Time}; +use crate::tracker::auth::Key; use crate::tracker::peer::Peer; -use crate::tracker::Tracker; - -/* code-review: authentication, authorization and peer IP resolution could be moved - from the handler (Axum) layer into the app layer `services::announce::invoke`. - That would make the handler even simpler and the code more reusable and decoupled from Axum. -*/ +use crate::tracker::{AnnounceData, Tracker}; #[allow(clippy::unused_async)] pub async fn handle_without_key( @@ -32,14 +28,7 @@ pub async fn handle_without_key( ) -> Response { debug!("http announce request: {:#?}", announce_request); - if tracker.requires_authentication() { - return responses::error::Error::from(auth::Error::MissingAuthKey { - location: Location::caller(), - }) - .into_response(); - } - - handle(&tracker, &announce_request, &client_ip_sources).await + handle(&tracker, &announce_request, &client_ip_sources, None).await } #[allow(clippy::unused_async)] @@ -51,29 +40,67 @@ pub async fn handle_with_key( ) -> Response { debug!("http announce request: {:#?}", announce_request); - match tracker.authenticate(&key).await { - Ok(_) => (), - Err(error) => return responses::error::Error::from(error).into_response(), - } + handle(&tracker, &announce_request, &client_ip_sources, Some(key)).await +} - handle(&tracker, &announce_request, &client_ip_sources).await +async fn handle( + tracker: &Arc, + announce_request: &Announce, + client_ip_sources: &ClientIpSources, + maybe_key: Option, +) -> Response { + let announce_data = match handle_announce(tracker, announce_request, client_ip_sources, maybe_key).await { + Ok(announce_data) => announce_data, + Err(error) => return error.into_response(), + }; + build_response(announce_request, announce_data) } -async fn handle(tracker: &Arc, announce_request: &Announce, client_ip_sources: &ClientIpSources) -> Response { +/* code-review: authentication, authorization and peer IP resolution could be moved + from the handler (Axum) layer into the app layer `services::announce::invoke`. + That would make the handler even simpler and the code more reusable and decoupled from Axum. +*/ + +async fn handle_announce( + tracker: &Arc, + announce_request: &Announce, + client_ip_sources: &ClientIpSources, + maybe_key: Option, +) -> Result { + // Authentication + if tracker.requires_authentication() { + match maybe_key { + Some(key) => match tracker.authenticate(&key).await { + Ok(_) => (), + Err(error) => return Err(responses::error::Error::from(error)), + }, + None => { + return Err(responses::error::Error::from(auth::Error::MissingAuthKey { + location: Location::caller(), + })) + } + } + } + + // Authorization match tracker.authorize(&announce_request.info_hash).await { Ok(_) => (), - Err(error) => return responses::error::Error::from(error).into_response(), + Err(error) => return Err(responses::error::Error::from(error)), } let peer_ip = match peer_ip_resolver::invoke(tracker.config.on_reverse_proxy, client_ip_sources) { Ok(peer_ip) => peer_ip, - Err(error) => return responses::error::Error::from(error).into_response(), + Err(error) => return Err(responses::error::Error::from(error)), }; let mut peer = peer_from_request(announce_request, &peer_ip); let announce_data = services::announce::invoke(tracker.clone(), announce_request.info_hash, &mut peer).await; + Ok(announce_data) +} + +fn build_response(announce_request: &Announce, announce_data: AnnounceData) -> Response { match &announce_request.compact { Some(compact) => match compact { Compact::Accepted => announce::Compact::from(announce_data).into_response(), @@ -108,3 +135,211 @@ fn map_to_aquatic_event(event: &Option) -> AnnounceEvent { None => aquatic_udp_protocol::AnnounceEvent::None, } } + +#[cfg(test)] +mod tests { + use std::sync::Arc; + + use crate::config::{ephemeral_configuration, Configuration}; + use crate::http::axum_implementation::requests::announce::Announce; + use crate::http::axum_implementation::responses; + use crate::http::axum_implementation::services::peer_ip_resolver::ClientIpSources; + use crate::protocol::info_hash::InfoHash; + use crate::tracker::mode::Mode; + use crate::tracker::statistics::Keeper; + use crate::tracker::{peer, Tracker}; + + fn private_tracker() -> Tracker { + let mut configuration = ephemeral_configuration(); + configuration.mode = Mode::Private; + tracker_factory(configuration) + } + + fn listed_tracker() -> Tracker { + let mut configuration = ephemeral_configuration(); + configuration.mode = Mode::Listed; + tracker_factory(configuration) + } + + fn tracker_on_reverse_proxy() -> Tracker { + let mut configuration = ephemeral_configuration(); + configuration.on_reverse_proxy = true; + tracker_factory(configuration) + } + + fn tracker_not_on_reverse_proxy() -> Tracker { + let mut configuration = ephemeral_configuration(); + configuration.on_reverse_proxy = false; + tracker_factory(configuration) + } + + fn tracker_factory(configuration: Configuration) -> Tracker { + // code-review: the tracker initialization is duplicated in many places. Consider make this function public. + + // Initialize stats tracker + let (stats_event_sender, stats_repository) = Keeper::new_active_instance(); + + // Initialize Torrust tracker + match Tracker::new(&Arc::new(configuration), Some(stats_event_sender), stats_repository) { + Ok(tracker) => tracker, + Err(error) => { + panic!("{}", error) + } + } + } + + fn sample_announce_request() -> Announce { + Announce { + info_hash: "3b245504cf5f11bbdbe1201cea6a6bf45aee1bc0".parse::().unwrap(), + peer_id: "-qB00000000000000001".parse::().unwrap(), + port: 17548, + downloaded: None, + uploaded: None, + left: None, + event: None, + compact: None, + } + } + + fn sample_client_ip_sources() -> ClientIpSources { + ClientIpSources { + right_most_x_forwarded_for: None, + connection_info_ip: None, + } + } + + fn assert_error_response(error: &responses::error::Error, error_message: &str) { + assert!( + error.failure_reason.contains(error_message), + "Error response does not contain message: '{error_message}'. Error: {error:?}" + ); + } + + mod with_tracker_in_private_mode { + + use std::str::FromStr; + use std::sync::Arc; + + use super::{private_tracker, sample_announce_request, sample_client_ip_sources}; + use crate::http::axum_implementation::handlers::announce::handle_announce; + use crate::http::axum_implementation::handlers::announce::tests::assert_error_response; + use crate::tracker::auth; + + #[tokio::test] + async fn it_should_fail_when_the_authentication_key_is_missing() { + let tracker = Arc::new(private_tracker()); + + let maybe_key = None; + + let response = handle_announce(&tracker, &sample_announce_request(), &sample_client_ip_sources(), maybe_key) + .await + .unwrap_err(); + + assert_error_response( + &response, + "Authentication error: Missing authentication key param for private tracker", + ); + } + + #[tokio::test] + async fn it_should_fail_when_the_authentication_key_is_invalid() { + let tracker = Arc::new(private_tracker()); + + let unregistered_key = auth::Key::from_str("YZSl4lMZupRuOpSRC3krIKR5BPB14nrJ").unwrap(); + + let maybe_key = Some(unregistered_key); + + let response = handle_announce(&tracker, &sample_announce_request(), &sample_client_ip_sources(), maybe_key) + .await + .unwrap_err(); + + assert_error_response(&response, "Authentication error: Failed to read key"); + } + } + + mod with_tracker_in_listed_mode { + + use std::sync::Arc; + + use super::{listed_tracker, sample_announce_request, sample_client_ip_sources}; + use crate::http::axum_implementation::handlers::announce::handle_announce; + use crate::http::axum_implementation::handlers::announce::tests::assert_error_response; + + #[tokio::test] + async fn it_should_fail_when_the_announced_torrent_is_not_whitelisted() { + let tracker = Arc::new(listed_tracker()); + + let announce_request = sample_announce_request(); + + let response = handle_announce(&tracker, &announce_request, &sample_client_ip_sources(), None) + .await + .unwrap_err(); + + assert_error_response( + &response, + &format!( + "Tracker error: The torrent: {}, is not whitelisted", + announce_request.info_hash + ), + ); + } + } + + mod with_tracker_on_reverse_proxy { + + use std::sync::Arc; + + use super::{sample_announce_request, tracker_on_reverse_proxy}; + use crate::http::axum_implementation::handlers::announce::handle_announce; + use crate::http::axum_implementation::handlers::announce::tests::assert_error_response; + use crate::http::axum_implementation::services::peer_ip_resolver::ClientIpSources; + + #[tokio::test] + async fn it_should_fail_when_the_right_most_x_forwarded_for_header_ip_is_not_available() { + let tracker = Arc::new(tracker_on_reverse_proxy()); + + let client_ip_sources = ClientIpSources { + right_most_x_forwarded_for: None, + connection_info_ip: None, + }; + + let response = handle_announce(&tracker, &sample_announce_request(), &client_ip_sources, None) + .await + .unwrap_err(); + + assert_error_response( + &response, + "Error resolving peer IP: missing or invalid the right most X-Forwarded-For IP", + ); + } + } + + mod with_tracker_not_on_reverse_proxy { + + use std::sync::Arc; + + use super::{sample_announce_request, tracker_not_on_reverse_proxy}; + use crate::http::axum_implementation::handlers::announce::handle_announce; + use crate::http::axum_implementation::handlers::announce::tests::assert_error_response; + use crate::http::axum_implementation::services::peer_ip_resolver::ClientIpSources; + + #[tokio::test] + async fn it_should_fail_when_the_client_ip_from_the_connection_info_is_not_available() { + let tracker = Arc::new(tracker_not_on_reverse_proxy()); + + let client_ip_sources = ClientIpSources { + right_most_x_forwarded_for: None, + connection_info_ip: None, + }; + + let response = handle_announce(&tracker, &sample_announce_request(), &client_ip_sources, None) + .await + .unwrap_err(); + + assert_error_response( + &response, + "Error resolving peer IP: cannot get the client IP from the connection info", + ); + } + } +} From 3860fc867b664e47186260996f3b434cfa57e6c8 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 7 Mar 2023 18:08:48 +0000 Subject: [PATCH 5/5] fix: format --- src/http/axum_implementation/handlers/common/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/http/axum_implementation/handlers/common/mod.rs b/src/http/axum_implementation/handlers/common/mod.rs index 41bf1369f..dc028cabf 100644 --- a/src/http/axum_implementation/handlers/common/mod.rs +++ b/src/http/axum_implementation/handlers/common/mod.rs @@ -1,2 +1,2 @@ -pub mod peer_ip; pub mod auth; +pub mod peer_ip;