Merge pull request #56 from actix/master

Fix audit issue logging by default peer address (#1485)

CHANGES.md

@@ -5,8 +5,14 @@
 ### Changed
 
 * Resources and Scopes can now access non-overridden data types set on App (or containing scopes) when setting their own data. [#1486]
+
+* Fix audit issue logging by default peer address [#1485]
+
 * Bump minimum supported Rust version to 1.40
 
+[#1485]: https://github.com/actix/actix-web/pull/1485
+
+
 ## [3.0.0-alpha.2] - 2020-05-08
 
 ### Changed
@@ -21,6 +27,7 @@
 [#1452]: https://github.com/actix/actix-web/pull/1452
 [#1486]: https://github.com/actix/actix-web/pull/1486
 
+
 ## [3.0.0-alpha.1] - 2020-03-11
 
 ### Added

src/info.rs

@@ -12,8 +12,8 @@ const X_FORWARDED_PROTO: &[u8] = b"x-forwarded-proto";
 pub struct ConnectionInfo {
     scheme: String,
     host: String,
-    remote: Option<String>,
+    realip_remote_addr: Option<String>,
-    peer: Option<String>,
+    remote_addr: Option<String>,
 }
 
 impl ConnectionInfo {
@@ -29,8 +29,7 @@ impl ConnectionInfo {
     fn new(req: &RequestHead, cfg: &AppConfig) -> ConnectionInfo {
         let mut host = None;
         let mut scheme = None;
-        let mut remote = None;
+        let mut realip_remote_addr = None;
-        let mut peer = None;
 
         // load forwarded header
         for hdr in req.headers.get_all(&header::FORWARDED) {
@@ -42,8 +41,8 @@ impl ConnectionInfo {
                             if let Some(val) = items.next() {
                                 match &name.to_lowercase() as &str {
                                     "for" => {
-                                        if remote.is_none() {
+                                        if realip_remote_addr.is_none() {
-                                            remote = Some(val.trim());
+                                            realip_remote_addr = Some(val.trim());
                                         }
                                     }
                                     "proto" => {
@@ -106,27 +105,25 @@ impl ConnectionInfo {
             }
         }
 
-        // remote addr
+        // get remote_addraddr from socketaddr
-        if remote.is_none() {
+        let remote_addr = req.peer_addr.map(|addr| format!("{}", addr));
+
+        if realip_remote_addr.is_none() {
             if let Some(h) = req
                 .headers
                 .get(&HeaderName::from_lowercase(X_FORWARDED_FOR).unwrap())
             {
                 if let Ok(h) = h.to_str() {
-                    remote = h.split(',').next().map(|v| v.trim());
+                    realip_remote_addr = h.split(',').next().map(|v| v.trim());
                 }
             }
-            if remote.is_none() {
-                // get peeraddr from socketaddr
-                peer = req.peer_addr.map(|addr| format!("{}", addr));
-            }
         }
 
         ConnectionInfo {
-            peer,
+            remote_addr,
             scheme: scheme.unwrap_or("http").to_owned(),
             host: host.unwrap_or("localhost").to_owned(),
-            remote: remote.map(|s| s.to_owned()),
+            realip_remote_addr: realip_remote_addr.map(|s| s.to_owned()),
         }
     }
 
@@ -155,13 +152,23 @@ impl ConnectionInfo {
         &self.host
     }
 
-    /// Remote socket addr of client initiated HTTP request.
+    /// remote_addr address of the request.
+    ///
+    /// Get remote_addr address from socket address
+    pub fn remote_addr(&self) -> Option<&str> {
+        if let Some(ref remote_addr) = self.remote_addr {
+            Some(remote_addr)
+        } else {
+            None
+        }
+    }
+    /// Real ip remote addr of client initiated HTTP request.
     ///
     /// The addr is resolved through the following headers, in this order:
     ///
     /// - Forwarded
     /// - X-Forwarded-For
-    /// - peer name of opened socket
+    /// - remote_addr name of opened socket
     ///
     /// # Security
     /// Do not use this function for security purposes, unless you can ensure the Forwarded and
@@ -169,11 +176,11 @@ impl ConnectionInfo {
     /// address explicitly, use
     /// [`HttpRequest::peer_addr()`](../web/struct.HttpRequest.html#method.peer_addr) instead.
     #[inline]
-    pub fn remote(&self) -> Option<&str> {
+    pub fn realip_remote_addr(&self) -> Option<&str> {
-        if let Some(ref r) = self.remote {
+        if let Some(ref r) = self.realip_remote_addr {
             Some(r)
-        } else if let Some(ref peer) = self.peer {
+        } else if let Some(ref remote_addr) = self.remote_addr {
-            Some(peer)
+            Some(remote_addr)
         } else {
             None
         }
@@ -202,7 +209,7 @@ mod tests {
         let info = req.connection_info();
         assert_eq!(info.scheme(), "https");
         assert_eq!(info.host(), "rust-lang.org");
-        assert_eq!(info.remote(), Some("192.0.2.60"));
+        assert_eq!(info.realip_remote_addr(), Some("192.0.2.60"));
 
         let req = TestRequest::default()
             .header(header::HOST, "rust-lang.org")
@@ -211,20 +218,20 @@ mod tests {
         let info = req.connection_info();
         assert_eq!(info.scheme(), "http");
         assert_eq!(info.host(), "rust-lang.org");
-        assert_eq!(info.remote(), None);
+        assert_eq!(info.realip_remote_addr(), None);
 
         let req = TestRequest::default()
             .header(X_FORWARDED_FOR, "192.0.2.60")
             .to_http_request();
         let info = req.connection_info();
-        assert_eq!(info.remote(), Some("192.0.2.60"));
+        assert_eq!(info.realip_remote_addr(), Some("192.0.2.60"));
 
         let req = TestRequest::default()
             .header(X_FORWARDED_HOST, "192.0.2.60")
             .to_http_request();
         let info = req.connection_info();
         assert_eq!(info.host(), "192.0.2.60");
-        assert_eq!(info.remote(), None);
+        assert_eq!(info.realip_remote_addr(), None);
 
         let req = TestRequest::default()
             .header(X_FORWARDED_PROTO, "https")

src/middleware/logger.rs

@@ -72,12 +72,21 @@ use crate::HttpResponse;
 ///
 /// `%U`  Request URL
 ///
+/// `%{r}a`  Real IP remote address **\***
+///
 /// `%{FOO}i`  request.headers['FOO']
 ///
 /// `%{FOO}o`  response.headers['FOO']
 ///
 /// `%{FOO}e`  os.environ['FOO']
 ///
+/// # Security
+///  **\*** It is calculated using
+///  [`ConnectionInfo::realip_remote_addr()`](../dev/struct.ConnectionInfo.html#method.realip_remote_addr)
+///
+///  If you use this value ensure that all requests come from trusted hosts, since it is trivial
+///  for the remote client to simulate been another client.
+///
 pub struct Logger(Rc<Inner>);
 
 struct Inner {
@@ -301,7 +310,7 @@ impl Format {
     /// Returns `None` if the format string syntax is incorrect.
     pub fn new(s: &str) -> Format {
         log::trace!("Access log format: {}", s);
-        let fmt = Regex::new(r"%(\{([A-Za-z0-9\-_]+)\}([ioe])|[atPrUsbTD]?)").unwrap();
+        let fmt = Regex::new(r"%(\{([A-Za-z0-9\-_]+)\}([aioe])|[atPrUsbTD]?)").unwrap();
 
         let mut idx = 0;
         let mut results = Vec::new();
@@ -315,6 +324,11 @@ impl Format {
 
             if let Some(key) = cap.get(2) {
                 results.push(match cap.get(3).unwrap().as_str() {
+                    "a" => if key.as_str() == "r" {
+                        FormatText::RealIPRemoteAddr
+                    } else {
+                        unreachable!()
+                    },
                     "i" => FormatText::RequestHeader(
                         HeaderName::try_from(key.as_str()).unwrap(),
                     ),
@@ -362,6 +376,7 @@ pub enum FormatText {
     Time,
     TimeMillis,
     RemoteAddr,
+    RealIPRemoteAddr,
     UrlPath,
     RequestHeader(HeaderName),
     ResponseHeader(HeaderName),
@@ -458,7 +473,15 @@ impl FormatText {
                 *self = FormatText::Str(s.to_string());
             }
             FormatText::RemoteAddr => {
-                let s = if let Some(remote) = req.connection_info().remote() {
+                let s = if let Some(ref peer) = req.connection_info().remote_addr() {
+                    FormatText::Str(peer.to_string())
+                } else {
+                    FormatText::Str("-".to_string())
+                };
+                *self = s;
+            }
+            FormatText::RealIPRemoteAddr => {
+                let s = if let Some(remote) = req.connection_info().realip_remote_addr() {
                     FormatText::Str(remote.to_string())
                 } else {
                     FormatText::Str("-".to_string())
@@ -549,6 +572,7 @@ mod tests {
             header::USER_AGENT,
             header::HeaderValue::from_static("ACTIX-WEB"),
         )
+        .peer_addr("127.0.0.1:8081".parse().unwrap())
         .to_srv_request();
 
         let now = OffsetDateTime::now_utc();
@@ -570,6 +594,7 @@ mod tests {
         };
         let s = format!("{}", FormatDisplay(&render));
         assert!(s.contains("GET / HTTP/1.1"));
+        assert!(s.contains("127.0.0.1"));
         assert!(s.contains("200 1024"));
         assert!(s.contains("ACTIX-WEB"));
     }
@@ -598,4 +623,36 @@ mod tests {
         let s = format!("{}", FormatDisplay(&render));
         assert!(s.contains(&format!("{}", now.format("%Y-%m-%dT%H:%M:%S"))));
     }
+
+    #[actix_rt::test]
+    async fn test_remote_addr_format() {
+        let mut format = Format::new("%{r}a");
+
+        let req = TestRequest::with_header(
+            header::FORWARDED,
+            header::HeaderValue::from_static("for=192.0.2.60;proto=http;by=203.0.113.43"),
+        )
+        .to_srv_request();
+
+        let now = OffsetDateTime::now_utc();
+        for unit in &mut format.0 {
+            unit.render_request(now, &req);
+        }
+
+        let resp = HttpResponse::build(StatusCode::OK).force_close().finish();
+        for unit in &mut format.0 {
+            unit.render_response(&resp);
+        }
+
+        let entry_time = OffsetDateTime::now_utc();
+        let render = |fmt: &mut Formatter<'_>| {
+            for unit in &format.0 {
+                unit.render(fmt, 1024, entry_time)?;
+            }
+            Ok(())
+        };
+        let s = format!("{}", FormatDisplay(&render));
+        println!("{}", s);
+        assert!(s.contains("192.0.2.60"));
+    }
 }