X-Forwarded-For catcher - improve ip addr parsing (#1725)

book/src/integrations/ldap.md

@@ -254,4 +254,3 @@ This is despite the fact:
 - The third is an incorrect port.
 
 To diagnose errors like this, you may need to add "-d 1" to your LDAP commands or client.
-

book/src/integrations/oauth2.md

@@ -264,9 +264,9 @@ In the virtual host, to protect a location:
 
 ### Miniflux
 
-Miniflux is a feedreader that supports OAuth 2.0 and OpenID connect. It automatically appends
-the `.well-known` parts to the discovery endpoint. The application name in the redirect URL
-needs to match the `OAUTH2_PROVIDER` name.
+Miniflux is a feedreader that supports OAuth 2.0 and OpenID connect. It automatically appends the
+`.well-known` parts to the discovery endpoint. The application name in the redirect URL needs to
+match the `OAUTH2_PROVIDER` name.
 
 ```
 OAUTH2_PROVIDER = "kanidm";
@@ -274,10 +274,11 @@ OAUTH2_CLIENT_ID = "miniflux";
 OAUTH2_CLIENT_SECRET = "<oauth2_rs_basic_secret>";
 OAUTH2_REDIRECT_URL = "https://feeds.example.com/oauth2/kanidm/callback";
 OAUTH2_OIDC_DISCOVERY_ENDPOINT = "https://idm.example.com/oauth2/openid/<oauth2_rs_name>";
-````
+```
 
-Currently Miniflux [does not support PKCE](https://github.com/miniflux/v2/issues/1910) and Kanidm will
-prevent logins until you [disable PKCE](#extended-options-for-legacy-clients) for the resource server.
+Currently Miniflux [does not support PKCE](https://github.com/miniflux/v2/issues/1910) and Kanidm
+will prevent logins until you [disable PKCE](#extended-options-for-legacy-clients) for the resource
+server.
 
 ### Nextcloud
 

examples/insecure_server.toml

@@ -12,6 +12,7 @@ log_level = "verbose"
 
 domain = "localhost"
 origin = "https://localhost:8443"
+trust_x_forward_for = true
 
 [online_backup]
 path = "/tmp/kanidm/backups/"

server/core/src/https/mod.rs

@@ -183,14 +183,25 @@ impl RequestExtensions for tide::Request<AppState> {
         (eventid, hv)
     }
 
+    /// Returns the remote address of the client, based on if you've got trust_x_forward_for set in config.
     fn get_remote_addr(&self) -> Option<IpAddr> {
         if self.state().trust_x_forward_for {
-            self.remote()
+            // split the socket address off if we've got one, then parse it as an `IpAddr`
+            // xff headers don't have a port, but if we're going direct you might get one
+            let res = self
+                .remote()
+                .map(|addr| addr.split(':').next().unwrap_or(addr))
+                .and_then(|ip| ip.parse::<IpAddr>().ok());
+            debug!("Trusting XFF, using remote src_ip={:?}", res);
+            res
         } else {
-            self.peer_addr()
+            let res = self
+                .peer_addr()
+                .map(|addr| addr.parse::<SocketAddr>().unwrap())
+                .map(|s_ad: SocketAddr| s_ad.ip());
+            debug!("Not trusting XFF, using peer_addr src_ip={:?}", res);
+            res
         }
-        .and_then(|add_str| add_str.parse().ok())
-        .map(|s_ad: SocketAddr| s_ad.ip())
     }
 }
 

server/core/src/https/v1.rs

@@ -1065,17 +1065,18 @@ pub async fn do_nothing(_req: tide::Request<AppState>) -> tide::Result {
 }
 
 pub async fn reauth(mut req: tide::Request<AppState>) -> tide::Result {
-    let uat = req.get_current_uat();
-    let (eventid, hvalue) = req.new_eventid();
-
+    // check that we can get the remote IP address first, since this doesn't touch the backend at all
     let ip_addr = req.get_remote_addr().ok_or_else(|| {
-        error!("Unable to process remote addr, refusing to proceed");
+        error!("Unable to get remote addr for auth event, refusing to proceed");
         tide::Error::from_str(
             tide::StatusCode::InternalServerError,
             "unable to validate peer address",
         )
     })?;
 
+    let uat = req.get_current_uat();
+    let (eventid, hvalue) = req.new_eventid();
+
     let obj: AuthIssueSession = req.body_json().await.map_err(|e| {
         debug!("Failed get body JSON? {:?}", e);
         e
@@ -1092,6 +1093,14 @@ pub async fn reauth(mut req: tide::Request<AppState>) -> tide::Result {
 }
 
 pub async fn auth(mut req: tide::Request<AppState>) -> tide::Result {
+    // check that we can get the remote IP address first, since this doesn't touch the backend at all
+    let ip_addr = req.get_remote_addr().ok_or_else(|| {
+        error!("Unable to get remote addr for auth event, refusing to proceed");
+        tide::Error::from_str(
+            tide::StatusCode::InternalServerError,
+            "unable to validate peer address",
+        )
+    })?;
     // First, deal with some state management.
     // Do anything here first that's needed like getting the session details
     // out of the req cookie.
@@ -1099,14 +1108,6 @@ pub async fn auth(mut req: tide::Request<AppState>) -> tide::Result {
 
     let maybe_sessionid: Option<Uuid> = req.get_current_auth_session_id();
 
-    let ip_addr = req.get_remote_addr().ok_or_else(|| {
-        error!("Unable to process remote addr, refusing to proceed");
-        tide::Error::from_str(
-            tide::StatusCode::InternalServerError,
-            "unable to validate peer address",
-        )
-    })?;
-
     let obj: AuthRequest = req.body_json().await.map_err(|e| {
         debug!("Failed get body JSON? {:?}", e);
         e

server/lib/src/repl/tests.rs

@@ -941,7 +941,7 @@ async fn test_repl_increment_basic_bidirectional_recycle(
     server_a_txn.commit().expect("Failed to commit");
     drop(server_b_txn);
 
-    // On both servers, at seperate timestamps, run the recycle.
+    // On both servers, at separate timestamps, run the recycle.
     let ct = ct + Duration::from_secs(1);
     let mut server_a_txn = server_a.write(ct).await;
     assert!(server_a_txn.internal_delete_uuid(t_uuid).is_ok());

server/testkit/benches/ip_address_parsing.rs

@@ -0,0 +1,45 @@
+#[test]
+fn bench_ip_address_parsing() {
+    use std::net::{IpAddr, SocketAddr};
+    use std::str::FromStr;
+    use std::time::Instant;
+
+    let ip_input_some = Some("1.2.3.4:1234");
+    let test_val = Some(IpAddr::from_str("1.2.3.4").unwrap());
+
+    let iterations = 10000000u128;
+
+    // test the split method
+    let split_start = Instant::now();
+    for _ in 0..iterations {
+        let res: Option<IpAddr> = ip_input_some
+            .map(|addr| addr.split(':').next().unwrap_or(addr))
+            .and_then(|ip| ip.parse::<IpAddr>().ok());
+        assert_eq!(test_val, res);
+    }
+    let split_end = Instant::now();
+
+    // test the socket parsing method
+    let socket_start = Instant::now();
+    for _ in 0..iterations {
+        let res: Option<IpAddr> = ip_input_some
+            .and_then(|add_str| add_str.parse().ok())
+            .map(|s_ad: SocketAddr| s_ad.ip());
+        assert_eq!(test_val, res);
+    }
+
+    let socket_end = Instant::now();
+
+    let split_time = split_end.duration_since(split_start);
+    let socket_time = socket_end.duration_since(socket_start);
+    println!(
+        "Split time: {:?}, {}ns/iteration",
+        split_time,
+        split_time.as_nanos() / iterations
+    );
+    println!(
+        "Socket time: {:?}, {}ns/iteration",
+        socket_time,
+        socket_time.as_nanos() / iterations
+    );
+}

tools/cli/Cargo.toml

@@ -4,13 +4,13 @@ default-run = "kanidm"
 description = "Kanidm Client Tools"
 documentation = "https://kanidm.github.io/kanidm/stable/"
 
-version.workspace = true
-authors.workspace = true
-rust-version.workspace = true
-edition.workspace = true
-license.workspace = true
-homepage.workspace = true
-repository.workspace = true
+version = { workspace=true }
+authors = { workspace=true }
+rust-version = { workspace=true }
+edition = { workspace=true }
+license = { workspace=true }
+homepage = { workspace=true }
+repository = { workspace=true }
 
 [features]
 default = ["unix"]
@@ -30,31 +30,31 @@ name = "kanidm_ssh_authorizedkeys_direct"
 path = "src/ssh_authorizedkeys.rs"
 
 [dependencies]
-async-recursion.workplace = true
+async-recursion = { workspace = true }
 clap = { workspace = true, features = ["derive", "env"] }
 compact_jwt = { workspace = true, features = ["openssl"] }
-dialoguer.workspace = true
-futures-concurrency.workspace = true
-libc.workspace = true
-kanidm_client.workspace = true
-kanidm_proto.workspace = true
+dialoguer = { workspace=true }
+futures-concurrency = { workspace=true }
+libc = { workspace=true }
+kanidm_client = { workspace=true }
+kanidm_proto = { workspace=true }
 qrcode = { workspace = true }
-rpassword.workspace = true
+rpassword = { workspace=true }
 serde = { workspace = true, features = ["derive"] }
-serde_json.workspace = true
-shellexpand.workspace = true
+serde_json = { workspace=true }
+shellexpand = { workspace=true }
 time = { workspace = true, features = ["serde", "std"] }
-tracing.workspace = true
+tracing = { workspace=true }
 tracing-subscriber = { workspace = true, features = ["env-filter", "fmt"] }
 tokio = { workspace = true, features = ["rt", "macros"] }
 url = { workspace = true, features = ["serde"] }
-uuid.workspace = true
-zxcvbn.workspace = true
+uuid = { workspace=true }
+zxcvbn = { workspace=true }
 
 [build-dependencies]
 clap = { workspace = true, features = ["derive"] }
-clap_complete.workspace = true
-uuid.workspace = true
+clap_complete = { workspace=true }
+uuid = { workspace=true }
 
 [target."cfg(target_os = \"windows\")".dependencies.webauthn-authenticator-rs]
 workspace = true