Merge #1474: Refactor remote client IP resolution code

e0162d1 refactor: the remote client IP resolution code (Jose Celano)

Pull request description:

  Refactor the remote client IP resolution code.

  - [x] Extract `ReverseProxyMode` flag.
  - [x] Extract `RemoteClientAddr` type.
  - [x] Add IP wrapper `ResolvedIp` to track IP source.
  - [x] Move all code to the `http-protocol` package.
  - [x] Other improvements.

ACKs for top commit:
  josecelano:
    ACK e0162d1

Tree-SHA512: 21721ac0fa334c747663dd239bae427bc7a1abf2bd9d8d04000ddb32265df3cb1ea9357b7e17e0d60a10094a49295e47a485b1f290e384e89a7f79a285603934

Author: josecelano

packages/http-protocol/src/v1/services/peer_ip_resolver.rs

@@ -1,4 +1,4 @@
-//! This service resolves the peer IP from the request.
+//! This service resolves the remote client address.
 //!
 //! The peer IP is used to identify the peer in the tracker. It's the peer IP
 //! that is used in the `announce` responses (peer list). And it's also used to
@@ -12,20 +12,65 @@
 //!                     X-Forwarded-For: 126.0.0.1       X-Forwarded-For: 126.0.0.1,126.0.0.2
 //! ```
 //!
-//! This service returns two options for the peer IP:
+//! This `ClientIpSources` contains two options for the peer IP:
 //!
 //! ```text
 //! right_most_x_forwarded_for = 126.0.0.2
 //! connection_info_ip         = 126.0.0.3
 //! ```
 //!
-//! Depending on the tracker configuration.
+//! Which one to use depends on the `ReverseProxyMode`.
 use std::net::{IpAddr, SocketAddr};
 use std::panic::Location;
 
 use serde::{Deserialize, Serialize};
 use thiserror::Error;
 
+/// Resolves the client's real address considering proxy headers. Port is also
+/// included when available.
+///
+/// # Errors
+///
+/// This function returns an error if the IP address cannot be resolved.
+pub fn resolve_remote_client_addr(
+    reverse_proxy_mode: &ReverseProxyMode,
+    client_ip_sources: &ClientIpSources,
+) -> Result<RemoteClientAddr, PeerIpResolutionError> {
+    let ip = match reverse_proxy_mode {
+        ReverseProxyMode::Enabled => ResolvedIp::FromXForwardedFor(client_ip_sources.try_client_ip_from_proxy_header()?),
+        ReverseProxyMode::Disabled => ResolvedIp::FromSocketAddr(client_ip_sources.try_client_ip_from_connection_info()?),
+    };
+
+    let port = client_ip_sources.client_port_from_connection_info();
+
+    Ok(RemoteClientAddr::new(ip, port))
+}
+
+/// This struct indicates whether the tracker is running on reverse proxy mode.
+#[derive(Serialize, Deserialize, Debug, PartialEq, Eq, Clone, Copy)]
+pub enum ReverseProxyMode {
+    Enabled,
+    Disabled,
+}
+
+impl From<ReverseProxyMode> for bool {
+    fn from(reverse_proxy_mode: ReverseProxyMode) -> Self {
+        match reverse_proxy_mode {
+            ReverseProxyMode::Enabled => true,
+            ReverseProxyMode::Disabled => false,
+        }
+    }
+}
+
+impl From<bool> for ReverseProxyMode {
+    fn from(reverse_proxy_mode: bool) -> Self {
+        if reverse_proxy_mode {
+            ReverseProxyMode::Enabled
+        } else {
+            ReverseProxyMode::Disabled
+        }
+    }
+}
 /// This struct contains the sources from which the peer IP can be obtained.
 #[derive(Serialize, Deserialize, Debug, PartialEq, Eq, Clone)]
 pub struct ClientIpSources {
@@ -36,6 +81,36 @@ pub struct ClientIpSources {
     pub connection_info_socket_address: Option<SocketAddr>,
 }
 
+impl ClientIpSources {
+    fn try_client_ip_from_connection_info(&self) -> Result<IpAddr, PeerIpResolutionError> {
+        if let Some(socket_addr) = self.connection_info_socket_address {
+            Ok(socket_addr.ip())
+        } else {
+            Err(PeerIpResolutionError::MissingClientIp {
+                location: Location::caller(),
+            })
+        }
+    }
+
+    fn try_client_ip_from_proxy_header(&self) -> Result<IpAddr, PeerIpResolutionError> {
+        if let Some(ip) = self.right_most_x_forwarded_for {
+            Ok(ip)
+        } else {
+            Err(PeerIpResolutionError::MissingRightMostXForwardedForIp {
+                location: Location::caller(),
+            })
+        }
+    }
+
+    fn client_port_from_connection_info(&self) -> Option<u16> {
+        if self.connection_info_socket_address.is_some() {
+            self.connection_info_socket_address.map(|socket_addr| socket_addr.port())
+        } else {
+            None
+        }
+    }
+}
+
 /// The error that can occur when resolving the peer IP.
 #[derive(Error, Debug, Clone)]
 pub enum PeerIpResolutionError {
@@ -54,120 +129,79 @@ pub enum PeerIpResolutionError {
     MissingClientIp { location: &'static Location<'static> },
 }
 
-/// Resolves the peer IP from the request.
-///
-/// Given the sources from which the peer IP can be obtained, this function
-/// resolves the peer IP according to the tracker configuration.
-///
-/// With the tracker running on reverse proxy mode:
-///
-/// ```rust
-/// use std::net::IpAddr;
-/// use std::str::FromStr;
-///
-/// use bittorrent_http_tracker_protocol::v1::services::peer_ip_resolver::{invoke, ClientIpSources, PeerIpResolutionError};
-///
-/// 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_socket_address: None,
-///     },
-/// )
-/// .unwrap();
-///
-/// assert_eq!(ip, IpAddr::from_str("203.0.113.195").unwrap());
-/// ```
-///
-/// With the tracker non running on reverse proxy mode:
-///
-/// ```rust
-/// use std::net::{IpAddr,Ipv4Addr,SocketAddr};
-/// use std::str::FromStr;
-///
-/// use bittorrent_http_tracker_protocol::v1::services::peer_ip_resolver::{invoke, ClientIpSources, PeerIpResolutionError};
-///
-/// let on_reverse_proxy = false;
-///
-/// let ip = invoke(
-///     on_reverse_proxy,
-///     &ClientIpSources {
-///         right_most_x_forwarded_for: None,
-///         connection_info_socket_address: Some(SocketAddr::new(IpAddr::V4(Ipv4Addr::new(203, 0, 113, 195)), 8080))
-///     },
-/// )
-/// .unwrap();
-///
-/// assert_eq!(ip, IpAddr::from_str("203.0.113.195").unwrap());
-/// ```
-///
-/// # 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<IpAddr, PeerIpResolutionError> {
-    if on_reverse_proxy {
-        resolve_peer_ip_on_reverse_proxy(client_ip_sources)
-    } else {
-        resolve_peer_ip_without_reverse_proxy(client_ip_sources)
-    }
+#[derive(Serialize, Deserialize, Debug, PartialEq, Eq, Clone, Copy)]
+pub struct RemoteClientAddr {
+    ip: ResolvedIp,
+    port: Option<u16>,
 }
 
-fn resolve_peer_ip_without_reverse_proxy(remote_client_ip: &ClientIpSources) -> Result<IpAddr, PeerIpResolutionError> {
-    if let Some(socket_addr) = remote_client_ip.connection_info_socket_address {
-        Ok(socket_addr.ip())
-    } else {
-        Err(PeerIpResolutionError::MissingClientIp {
-            location: Location::caller(),
-        })
+impl RemoteClientAddr {
+    #[must_use]
+    pub fn new(ip: ResolvedIp, port: Option<u16>) -> Self {
+        Self { ip, port }
+    }
+
+    #[must_use]
+    pub fn ip(&self) -> IpAddr {
+        match self.ip {
+            ResolvedIp::FromSocketAddr(ip) | ResolvedIp::FromXForwardedFor(ip) => ip,
+        }
     }
-}
 
-fn resolve_peer_ip_on_reverse_proxy(remote_client_ip: &ClientIpSources) -> Result<IpAddr, PeerIpResolutionError> {
-    if let Some(ip) = remote_client_ip.right_most_x_forwarded_for {
-        Ok(ip)
-    } else {
-        Err(PeerIpResolutionError::MissingRightMostXForwardedForIp {
-            location: Location::caller(),
-        })
+    #[must_use]
+    pub fn port(&self) -> Option<u16> {
+        self.port
     }
 }
 
+/// This enum indicates the source of the resolved IP address.
+#[derive(Serialize, Deserialize, Debug, PartialEq, Eq, Clone, Copy)]
+pub enum ResolvedIp {
+    FromXForwardedFor(IpAddr),
+    FromSocketAddr(IpAddr),
+}
+
 #[cfg(test)]
 mod tests {
-    use super::invoke;
+    use super::resolve_remote_client_addr;
 
     mod working_without_reverse_proxy {
         use std::net::{IpAddr, Ipv4Addr, SocketAddr};
         use std::str::FromStr;
 
-        use super::invoke;
-        use crate::v1::services::peer_ip_resolver::{ClientIpSources, PeerIpResolutionError};
+        use super::resolve_remote_client_addr;
+        use crate::v1::services::peer_ip_resolver::{
+            ClientIpSources, PeerIpResolutionError, RemoteClientAddr, ResolvedIp, ReverseProxyMode,
+        };
 
         #[test]
-        fn it_should_get_the_peer_ip_from_the_connection_info() {
-            let on_reverse_proxy = false;
+        fn it_should_get_the_remote_client_address_from_the_connection_info() {
+            let reverse_proxy_mode = ReverseProxyMode::Disabled;
 
-            let ip = invoke(
-                on_reverse_proxy,
+            let ip = resolve_remote_client_addr(
+                &reverse_proxy_mode,
                 &ClientIpSources {
                     right_most_x_forwarded_for: None,
                     connection_info_socket_address: Some(SocketAddr::new(IpAddr::V4(Ipv4Addr::new(203, 0, 113, 195)), 8080)),
                 },
             )
             .unwrap();
 
-            assert_eq!(ip, IpAddr::from_str("203.0.113.195").unwrap());
+            assert_eq!(
+                ip,
+                RemoteClientAddr::new(
+                    ResolvedIp::FromSocketAddr(IpAddr::from_str("203.0.113.195").unwrap()),
+                    Some(8080)
+                )
+            );
         }
 
         #[test]
-        fn it_should_return_an_error_if_it_cannot_get_the_peer_ip_from_the_connection_info() {
-            let on_reverse_proxy = false;
+        fn it_should_return_an_error_if_it_cannot_get_the_remote_client_ip_from_the_connection_info() {
+            let reverse_proxy_mode = ReverseProxyMode::Disabled;
 
-            let error = invoke(
-                on_reverse_proxy,
+            let error = resolve_remote_client_addr(
+                &reverse_proxy_mode,
                 &ClientIpSources {
                     right_most_x_forwarded_for: None,
                     connection_info_socket_address: None,
@@ -179,34 +213,42 @@ mod tests {
         }
     }
 
-    mod working_on_reverse_proxy {
+    mod working_on_reverse_proxy_mode {
         use std::net::IpAddr;
         use std::str::FromStr;
 
-        use crate::v1::services::peer_ip_resolver::{invoke, ClientIpSources, PeerIpResolutionError};
+        use crate::v1::services::peer_ip_resolver::{
+            resolve_remote_client_addr, ClientIpSources, PeerIpResolutionError, RemoteClientAddr, ResolvedIp, ReverseProxyMode,
+        };
 
         #[test]
-        fn it_should_get_the_peer_ip_from_the_right_most_ip_in_the_x_forwarded_for_header() {
-            let on_reverse_proxy = true;
+        fn it_should_get_the_remote_client_ip_from_the_right_most_ip_in_the_x_forwarded_for_header() {
+            let reverse_proxy_mode = ReverseProxyMode::Enabled;
 
-            let ip = invoke(
-                on_reverse_proxy,
+            let ip = resolve_remote_client_addr(
+                &reverse_proxy_mode,
                 &ClientIpSources {
                     right_most_x_forwarded_for: Some(IpAddr::from_str("203.0.113.195").unwrap()),
                     connection_info_socket_address: None,
                 },
             )
             .unwrap();
 
-            assert_eq!(ip, IpAddr::from_str("203.0.113.195").unwrap());
+            assert_eq!(
+                ip,
+                RemoteClientAddr::new(
+                    ResolvedIp::FromXForwardedFor(IpAddr::from_str("203.0.113.195").unwrap()),
+                    None
+                )
+            );
         }
 
         #[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 reverse_proxy_mode = ReverseProxyMode::Enabled;
 
-            let error = invoke(
-                on_reverse_proxy,
+            let error = resolve_remote_client_addr(
+                &reverse_proxy_mode,
                 &ClientIpSources {
                     right_most_x_forwarded_for: None,
                     connection_info_socket_address: None,

packages/http-tracker-core/src/event/mod.rs

@@ -1,13 +1,12 @@
 use std::net::{IpAddr, SocketAddr};
 
+use bittorrent_http_tracker_protocol::v1::services::peer_ip_resolver::RemoteClientAddr;
 use bittorrent_primitives::info_hash::InfoHash;
 use torrust_tracker_metrics::label::{LabelSet, LabelValue};
 use torrust_tracker_metrics::label_name;
 use torrust_tracker_primitives::peer::PeerAnnouncement;
 use torrust_tracker_primitives::service_binding::ServiceBinding;
 
-use crate::services::RemoteClientAddr;
-
 pub mod sender;
 
 /// A HTTP core event.
@@ -64,12 +63,12 @@ pub struct ClientConnectionContext {
 impl ClientConnectionContext {
     #[must_use]
     pub fn ip_addr(&self) -> IpAddr {
-        self.remote_client_addr.ip
+        self.remote_client_addr.ip()
     }
 
     #[must_use]
     pub fn port(&self) -> Option<u16> {
-        self.remote_client_addr.port
+        self.remote_client_addr.port()
     }
 }
 
@@ -100,11 +99,11 @@ impl From<ConnectionContext> for LabelSet {
 #[cfg(test)]
 pub mod test {
 
+    use bittorrent_http_tracker_protocol::v1::services::peer_ip_resolver::{RemoteClientAddr, ResolvedIp};
     use torrust_tracker_primitives::peer::Peer;
     use torrust_tracker_primitives::service_binding::Protocol;
 
     use super::Event;
-    use crate::services::RemoteClientAddr;
     use crate::tests::sample_info_hash;
 
     #[must_use]
@@ -153,7 +152,7 @@ pub mod test {
 
         let event1 = Event::TcpAnnounce {
             connection: ConnectionContext::new(
-                RemoteClientAddr::new(remote_client_ip, Some(8080)),
+                RemoteClientAddr::new(ResolvedIp::FromSocketAddr(remote_client_ip), Some(8080)),
                 ServiceBinding::new(Protocol::HTTP, SocketAddr::new(IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)), 7070)).unwrap(),
             ),
             info_hash,
@@ -162,7 +161,10 @@ pub mod test {
 
         let event2 = Event::TcpAnnounce {
             connection: ConnectionContext::new(
-                RemoteClientAddr::new(IpAddr::V4(Ipv4Addr::new(127, 0, 0, 2)), Some(8080)),
+                RemoteClientAddr::new(
+                    ResolvedIp::FromSocketAddr(IpAddr::V4(Ipv4Addr::new(127, 0, 0, 2))),
+                    Some(8080),
+                ),
                 ServiceBinding::new(Protocol::HTTP, SocketAddr::new(IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)), 7070)).unwrap(),
             ),
             info_hash,