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

* Fix audit issue logging by default peer address

By default log format include remote address that is taken from headers.
This is very easy to replace making log untrusted.

Changing default log format value `%a` to peer address we are getting
this trusted data always. Also, remote address option is maintianed and
relegated to `%{r}a` value.

Related  kanidm/kanidm#191.

* Rename peer/remote to remote_addr/realip_remote_addr

Change names to avoid naming confusions. I choose this accord to Nginx
variables and
[ngx_http_realip_module](https://nginx.org/en/docs/http/ngx_http_realip_module.html).

Add more specific documentation about security concerns of using Real IP
in logger.

* Rename security advertise header in doc

* Add fix audit issue logging by default peer adress to changelog

Co-authored-by: Rob Ede <robjtede@icloud.com>

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"));
+    }
 }