bug: Handle private networks as trusted proxies (#53)

* bug: Handle private networks as trusted proxies

Local private networks should be automatically considered proxies and
skipped for metadata lookup.

Closes #51

Author: jrconlin

channelserver/src/main.rs

@@ -165,22 +165,39 @@ fn main() {
     };
     let msettings = settings.clone();
     let mut trusted_list: Vec<ipnet::IpNet> = Vec::new();
+    // Add the known private networks to the trusted proxy list
+    trusted_list.push("10.0.0.0/8".parse().unwrap());
+    trusted_list.push("172.16.0.0/12".parse().unwrap());
+    trusted_list.push("192.168.0.0/16".parse().unwrap());
+
     // Add the list of trusted proxies.
     if settings.trusted_proxy_list.len() > 0 {
         for mut proxy in settings.trusted_proxy_list.split(",") {
             proxy = proxy.trim();
             if proxy.len() > 0 {
-                let addr: ipnet::IpNet = proxy.parse().unwrap();
-                trusted_list.push(addr);
+                // ipnet::IpNet only wants CIDRs. Normally that's not a problem, but the
+                // user may specify a single address. In that case, force the single
+                // into a CIDR by giving it a single address scope.
+                let mut fixed = proxy.to_owned();
+                if !proxy.contains("/") {
+                    fixed = format!("{}/32", proxy);
+                    debug!(log.log, "Fixing single address {}", fixed);
+                }
+                match fixed.parse::<ipnet::IpNet>() {
+                    Ok(addr) => trusted_list.push(addr),
+                    Err(err) => {
+                        error!(logger.log, r#"Ignoring unparsable IP address "{}"#, proxy);
+                    }
+                };
             }
         }
     }
+    debug!(logger.log, "Trusted Proxies: {:?}", trusted_list);
     // check that the maxmind db is where it should be.
     if !Path::new(&settings.mmdb_loc).exists() {
-        slog_error!(
+        error!(
             logger.log,
-            "Cannot find geoip database: {}",
-            settings.mmdb_loc
+            "Cannot find geoip database: {}", settings.mmdb_loc
         );
         return;
     };

channelserver/src/meta.rs

@@ -127,24 +127,16 @@ fn get_ua(
     None
 }
 
-fn is_trusted_proxy(proxy_list: &[IpNet], host: &str) -> Result<bool, HandlerError> {
-    // Return if an address is NOT part of the allow list
-    let test_addr: IpAddr = match host.parse() {
-        Ok(addr) => addr,
-        Err(e) => return Err(HandlerErrorKind::BadRemoteAddrError(format!("{:?}", e)).into()),
-    };
-    for proxy_range in proxy_list {
-        if proxy_range.contains(&test_addr) {
-            return Ok(true);
-        }
-    }
-    Ok(false)
+fn is_trusted_proxy(proxy_list: &[IpNet], host: &IpAddr) -> bool {
+    // Return if an address is part of the allow list
+    proxy_list.iter().any(|range| range.contains(host))
 }
 
 fn get_remote(
     peer: &Option<SocketAddr>,
     headers: &http::HeaderMap,
     proxy_list: &[IpNet],
+    log: &logging::MozLogger,
 ) -> Result<String, HandlerError> {
     // Actix determines the connection_info.remote() from the first entry in the
     // Forwarded then X-Fowarded-For, Forwarded-For, then peer name. The problem is that any
@@ -156,31 +148,36 @@ fn get_remote(
     if peer.is_none() {
         return Err(HandlerErrorKind::BadRemoteAddrError("Peer is unspecified".to_owned()).into());
     }
-    let peer_ip = peer.unwrap().ip().to_string();
+    let peer_ip = peer.unwrap().ip();
     // if the peer is not a known proxy, ignore the X-Forwarded-For headers
-    if !is_trusted_proxy(proxy_list, &peer_ip)? {
-        return Ok(peer_ip);
+    if !is_trusted_proxy(proxy_list, &peer_ip) {
+        return Ok(peer_ip.to_string());
     }
 
     // The peer is a known proxy, so take rightmost X-Forwarded-For that is not a trusted proxy.
     match headers.get(HeaderName::from_lowercase("x-forwarded-for".as_bytes()).unwrap()) {
         Some(header) => {
             match header.to_str() {
                 Ok(hstr) => {
+                    info!(log.log, "Remote IP List: {:?}", hstr);
                     // successive proxies are appeneded to this header.
                     let mut host_list: Vec<&str> = hstr.split(',').collect();
                     host_list.reverse();
                     for host_str in host_list {
-                        let host = host_str.trim().to_owned();
-                        if !is_trusted_proxy(proxy_list, &host)? {
-                            return Ok(host.to_owned());
+                        match host_str.trim().parse::<IpAddr>() {
+                            Ok(addr) => {
+                                if !addr.is_loopback() &&
+                                   !is_trusted_proxy(proxy_list, &addr) {
+                                    return Ok(addr.to_string());
+                                }
+                            },
+                            Err(err) => {
+                                return Err(HandlerErrorKind::BadRemoteAddrError("Bad IP Specified".to_owned()).into());
+                            }
                         }
-                    }
-                    Err(HandlerErrorKind::BadRemoteAddrError(format!(
-                        "Could not find remote IP in X-Forwarded-For"
-                    ))
-                    .into())
-                }
+                    };
+                    Err(HandlerErrorKind::BadRemoteAddrError("Only proxies specified".to_owned()).into())
+                },
                 Err(err) => Err(HandlerErrorKind::BadRemoteAddrError(format!(
                     "Unknown address in X-Forwarded-For: {:?}",
                     err
@@ -302,6 +299,7 @@ impl From<HttpRequest<WsChannelSessionState>> for SenderData {
             &req.peer_addr(),
             &req.headers(),
             &req.state().trusted_proxy_list,
+            &log,
         ) {
             Ok(addr) => Some(addr),
             Err(err) => {
@@ -485,30 +483,31 @@ mod test {
 
         let true_remote: SocketAddr = "1.2.3.4:0".parse().unwrap();
         let proxy_server: SocketAddr = "192.168.0.4:0".parse().unwrap();
+        let log = logging::MozLogger::new_human();
 
         bad_headers.insert(
             http::header::HeaderName::from_lowercase("x-forwarded-for".as_bytes()).unwrap(),
             "".parse().unwrap(),
         );
 
         // Proxy only, no XFF header
-        let remote = get_remote(&Some(proxy_server), &empty_headers, &proxy_list);
+        let remote = get_remote(&Some(proxy_server), &empty_headers, &proxy_list, &log);
         assert!(remote.is_err());
 
         // Proxy only, bad XFF header
-        let remote = get_remote(&Some(proxy_server), &bad_headers, &proxy_list);
+        let remote = get_remote(&Some(proxy_server), &bad_headers, &proxy_list, &log);
         assert!(remote.is_err());
 
         // Proxy only, crap XFF header
         bad_headers.insert(
             http::header::HeaderName::from_lowercase("x-forwarded-for".as_bytes()).unwrap(),
             "invalid".parse().unwrap(),
         );
-        let remote = get_remote(&Some(proxy_server), &bad_headers, &proxy_list);
+        let remote = get_remote(&Some(proxy_server), &bad_headers, &proxy_list, &log);
         assert!(remote.is_err());
 
         // Peer only, no header
-        let remote = get_remote(&Some(true_remote), &empty_headers, &proxy_list);
+        let remote = get_remote(&Some(true_remote), &empty_headers, &proxy_list, &log);
         assert_eq!(remote.unwrap(), "1.2.3.4".to_owned());
 
         headers.insert(
@@ -517,7 +516,7 @@ mod test {
         );
 
         // Peer proxy, fetch from XFF header
-        let remote = get_remote(&Some(proxy_server), &headers, &proxy_list);
+        let remote = get_remote(&Some(proxy_server), &headers, &proxy_list, &log);
         assert_eq!(remote.unwrap(), "1.2.3.4".to_owned());
 
         // Peer proxy, ensure right most XFF client fetched
@@ -526,7 +525,7 @@ mod test {
             "1.2.3.4, 2.3.4.5".parse().unwrap(),
         );
 
-        let remote = get_remote(&Some(proxy_server), &headers, &proxy_list);
+        let remote = get_remote(&Some(proxy_server), &headers, &proxy_list, &log);
         assert_eq!(remote.unwrap(), "2.3.4.5".to_owned());
 
         // Peer proxy, ensure right most non-proxy XFF client fetched
@@ -535,7 +534,7 @@ mod test {
             "1.2.3.4, 2.3.4.5, 192.168.0.10".parse().unwrap(),
         );
 
-        let remote = get_remote(&Some(proxy_server), &headers, &proxy_list);
+        let remote = get_remote(&Some(proxy_server), &headers, &proxy_list, &log);
         assert_eq!(remote.unwrap(), "2.3.4.5".to_owned());
     }
 }