Indicate that this is an ip list, not a range (#3626)

* Indicate that this is an ip list, not a range We mistakenly commented that this was a range, not a list. This has led to some confusion. Be clear it's a list of ip's, not a range. * Support Ip Ranges instead of Ip Addresses in X-Forward-For * Docs feedback
2025-06-11 02:27:47 +02:00 · 2025-05-13 11:53:58 +10:00 · 2025-05-13 11:53:58 +10:00 · b5cdf9dcf2
parent 47b091cd49
commit b5cdf9dcf2
10 changed files with 96 additions and 43 deletions
--- a/Cargo.lock
+++ b/Cargo.lock
@ -695,6 +695,15 @@ dependencies = [
 "windows-link",
 ]

+[[package]]
+name = "cidr"
+version = "0.3.1"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "bd1b64030216239a2e7c364b13cd96a2097ebf0dfe5025f2dedee14a23f2ab60"
+dependencies = [
+ "serde",
+]
+
 [[package]]
 name = "clang-sys"
 version = "1.8.1"
@ -3274,6 +3283,7 @@ dependencies = [
 "axum-macros",
 "bytes",
 "chrono",
+ "cidr",
 "compact_jwt",
 "cron",
 "filetime",
@ -3383,6 +3393,7 @@ dependencies = [
 name = "kanidmd_testkit"
 version = "1.7.0-dev"
 dependencies = [
+ "cidr",
 "compact_jwt",
 "escargot",
 "fantoccini",
--- a/Cargo.toml
+++ b/Cargo.toml
@ -166,6 +166,7 @@ base64 = "^0.22.1"
 base64urlsafedata = "0.5.1"
 bitflags = "^2.8.0"
 bytes = "^1.9.0"
+cidr = "0.3.1"
 clap = { version = "4.5.38", features = ["derive", "env"] }
 clap_complete = "^4.5.50"
 # Forced by saffron/cron
--- a/examples/server.toml
+++ b/examples/server.toml
@ -82,25 +82,25 @@ origin = "https://idm.example.com:8443"
 #   will often add a header such as "Forwarded" or
 #   "X-Forwarded-For". Some other proxies can use the PROXY
 #   protocol v2 header.
-#   This setting allows configuration of the range of trusted
-#   IPs which can supply this header information, and which
-#   format the information is provided in.
+#   This setting allows configuration of the list of trusted
+#   IPs or IP ranges which can supply this header information,
+#   and which format the information is provided in.
 #   Defaults to "none" (no trusted sources)
 #   Only one option can be used at a time.
 # [http_client_address_info]
-# proxy-v2 = ["127.0.0.1"]
+# proxy-v2 = ["127.0.0.1", "127.0.0.0/8"]
 #   # OR
-# x-forward-for = ["127.0.0.1"]
+# x-forward-for = ["127.0.0.1", "127.0.0.0/8"]

 #   LDAPS requests can be reverse proxied by a loadbalancer.
 #   To preserve the original IP of the caller, these systems
 #   can add a header such as the PROXY protocol v2 header.
-#   This setting allows configuration of the range of trusted
-#   IPs which can supply this header information, and which
-#   format the information is provided in.
+#   This setting allows configuration of the list of trusted
+#   IPs or IP ranges which can supply this header information,
+#   and which format the information is provided in.
 #   Defaults to "none" (no trusted sources)
 # [ldap_client_address_info]
-# proxy-v2 = ["127.0.0.1"]
+# proxy-v2 = ["127.0.0.1", "127.0.0.0/8"]

 [online_backup]
 #   The path to the output folder for online backups
--- a/examples/server_container.toml
+++ b/examples/server_container.toml
@ -81,25 +81,25 @@ origin = "https://idm.example.com:8443"
 #   will often add a header such as "Forwarded" or
 #   "X-Forwarded-For". Some other proxies can use the PROXY
 #   protocol v2 header.
-#   This setting allows configuration of the range of trusted
-#   IPs which can supply this header information, and which
-#   format the information is provided in.
+#   This setting allows configuration of the list of trusted
+#   IPs or IP ranges which can supply this header information,
+#   and which format the information is provided in.
 #   Defaults to "none" (no trusted sources)
 #   Only one option can be used at a time.
 # [http_client_address_info]
-# proxy-v2 = ["127.0.0.1"]
+# proxy-v2 = ["127.0.0.1", "127.0.0.0/8"]
 #   # OR
-# x-forward-for = ["127.0.0.1"]
+# x-forward-for = ["127.0.0.1", "127.0.0.0/8"]

 #   LDAPS requests can be reverse proxied by a loadbalancer.
 #   To preserve the original IP of the caller, these systems
 #   can add a header such as the PROXY protocol v2 header.
-#   This setting allows configuration of the range of trusted
-#   IPs which can supply this header information, and which
-#   format the information is provided in.
+#   This setting allows configuration of the list of trusted
+#   IPs or IP ranges which can supply this header information,
+#   and which format the information is provided in.
 #   Defaults to "none" (no trusted sources)
 # [ldap_client_address_info]
-# proxy-v2 = ["127.0.0.1"]
+# proxy-v2 = ["127.0.0.1", "127.0.0.0/8"]

 [online_backup]
 #   The path to the output folder for online backups
--- a/server/core/Cargo.toml
+++ b/server/core/Cargo.toml
@ -27,6 +27,7 @@ axum-htmx = { workspace = true }
 axum-extra = { workspace = true }
 axum-macros = { workspace = true }
 bytes = { workspace = true }
+cidr = { workspace = true, features = ["serde"] }
 chrono = { workspace = true }
 compact_jwt = { workspace = true }
 cron = { workspace = true }
--- a/server/core/src/config.rs
+++ b/server/core/src/config.rs
@ -4,7 +4,7 @@
 //! These components should be "per server". Any "per domain" config should be in the system
 //! or domain entries that are able to be replicated.

-use hashbrown::HashSet;
+use cidr::IpCidr;
 use kanidm_proto::constants::DEFAULT_SERVER_ADDRESS;
 use kanidm_proto::internal::FsType;
 use kanidm_proto::messages::ConsoleOutputMode;
@ -110,11 +110,11 @@ pub enum LdapAddressInfo {
    #[default]
    None,
    #[serde(rename = "proxy-v2")]
-    ProxyV2(HashSet<IpAddr>),
+    ProxyV2(Vec<IpCidr>),
 }

 impl LdapAddressInfo {
-    pub fn trusted_proxy_v2(&self) -> Option<HashSet<IpAddr>> {
+    pub fn trusted_proxy_v2(&self) -> Option<Vec<IpCidr>> {
        if let Self::ProxyV2(trusted) = self {
            Some(trusted.clone())
        } else {
@ -139,7 +139,7 @@ impl Display for LdapAddressInfo {
 }

 pub(crate) enum AddressSet {
-    NonContiguousIpSet(HashSet<IpAddr>),
+    NonContiguousIpSet(Vec<IpCidr>),
    All,
 }

@ -147,7 +147,9 @@ impl AddressSet {
    pub(crate) fn contains(&self, ip_addr: &IpAddr) -> bool {
        match self {
            Self::All => true,
-            Self::NonContiguousIpSet(range) => range.contains(ip_addr),
+            Self::NonContiguousIpSet(range) => {
+                range.iter().any(|ip_cidr| ip_cidr.contains(ip_addr))
+            }
        }
    }
 }
@ -157,13 +159,13 @@ pub enum HttpAddressInfo {
    #[default]
    None,
    #[serde(rename = "x-forward-for")]
-    XForwardFor(HashSet<IpAddr>),
+    XForwardFor(Vec<IpCidr>),
    // IMPORTANT: This is undocumented, and only exists for backwards compat
    // with config v1 which has a boolean toggle for this option.
    #[serde(rename = "x-forward-for-all-source-trusted")]
    XForwardForAllSourcesTrusted,
    #[serde(rename = "proxy-v2")]
-    ProxyV2(HashSet<IpAddr>),
+    ProxyV2(Vec<IpCidr>),
 }

 impl HttpAddressInfo {
@ -175,7 +177,7 @@ impl HttpAddressInfo {
        }
    }

-    pub(crate) fn trusted_proxy_v2(&self) -> Option<HashSet<IpAddr>> {
+    pub(crate) fn trusted_proxy_v2(&self) -> Option<Vec<IpCidr>> {
        if let Self::ProxyV2(trusted) = self {
            Some(trusted.clone())
        } else {
@ -1170,3 +1172,32 @@ impl ConfigurationBuilder {
        })
    }
 }
+
+#[cfg(test)]
+mod tests {
+    use cidr::{IpCidr, Ipv4Cidr, Ipv6Cidr};
+    use std::net::{Ipv4Addr, Ipv6Addr};
+
+    #[test]
+    fn assert_cidr_parsing_behaviour() {
+        // Assert that we can parse individual hosts, and ranges
+        let parsed_ip_cidr: IpCidr = serde_json::from_str("\"127.0.0.1\"").unwrap();
+        let expect_ip_cidr = IpCidr::from(Ipv4Addr::new(127, 0, 0, 1));
+        assert_eq!(parsed_ip_cidr, expect_ip_cidr);
+
+        let parsed_ip_cidr: IpCidr = serde_json::from_str("\"127.0.0.0/8\"").unwrap();
+        let expect_ip_cidr = IpCidr::from(Ipv4Cidr::new(Ipv4Addr::new(127, 0, 0, 0), 8).unwrap());
+        assert_eq!(parsed_ip_cidr, expect_ip_cidr);
+
+        // Same for ipv6
+        let parsed_ip_cidr: IpCidr = serde_json::from_str("\"2001:0db8::1\"").unwrap();
+        let expect_ip_cidr = IpCidr::from(Ipv6Addr::new(0x2001, 0x0db8, 0, 0, 0, 0, 0, 0x0001));
+        assert_eq!(parsed_ip_cidr, expect_ip_cidr);
+
+        let parsed_ip_cidr: IpCidr = serde_json::from_str("\"2001:0db8::/64\"").unwrap();
+        let expect_ip_cidr = IpCidr::from(
+            Ipv6Cidr::new(Ipv6Addr::new(0x2001, 0x0db8, 0, 0, 0, 0, 0, 0), 64).unwrap(),
+        );
+        assert_eq!(parsed_ip_cidr, expect_ip_cidr);
+    }
+}
--- a/server/core/src/https/mod.rs
+++ b/server/core/src/https/mod.rs
@ -29,10 +29,10 @@ use axum::{
    Router,
 };
 use axum_extra::extract::cookie::CookieJar;
+use cidr::IpCidr;
 use compact_jwt::{error::JwtError, JwsCompact, JwsHs256Signer, JwsVerifier};
 use futures::pin_mut;
 use haproxy_protocol::{ProxyHdrV2, RemoteAddress};
-use hashbrown::HashSet;
 use hyper::body::Incoming;
 use hyper_util::rt::{TokioExecutor, TokioIo};
 use kanidm_lib_crypto::x509_cert::{der::Decode, x509_public_key_s256, Certificate};
@ -43,7 +43,6 @@ use serde::de::DeserializeOwned;
 use sketching::*;
 use std::fmt::Write;
 use std::io::ErrorKind;
-use std::net::IpAddr;
 use std::path::PathBuf;
 use std::pin::Pin;
 use std::sync::Arc;
@ -363,7 +362,7 @@ async fn server_tls_loop(
    mut rx: broadcast::Receiver<CoreAction>,
    server_message_tx: broadcast::Sender<CoreAction>,
    mut tls_acceptor_reload_rx: mpsc::Receiver<SslAcceptor>,
-    trusted_proxy_v2_ips: Option<Arc<HashSet<IpAddr>>>,
+    trusted_proxy_v2_ips: Option<Arc<Vec<IpCidr>>>,
 ) {
    pin_mut!(listener);

@ -404,7 +403,7 @@ async fn server_plaintext_loop(
    listener: TcpListener,
    app: IntoMakeServiceWithConnectInfo<Router, ClientConnInfo>,
    mut rx: broadcast::Receiver<CoreAction>,
-    trusted_proxy_v2_ips: Option<Arc<HashSet<IpAddr>>>,
+    trusted_proxy_v2_ips: Option<Arc<Vec<IpCidr>>>,
 ) {
    pin_mut!(listener);

@ -438,7 +437,7 @@ pub(crate) async fn handle_conn(
    stream: TcpStream,
    app: IntoMakeServiceWithConnectInfo<Router, ClientConnInfo>,
    connection_addr: SocketAddr,
-    trusted_proxy_v2_ips: Option<Arc<HashSet<IpAddr>>>,
+    trusted_proxy_v2_ips: Option<Arc<Vec<IpCidr>>>,
 ) -> Result<(), std::io::Error> {
    let (stream, client_addr) =
        process_client_addr(stream, connection_addr, trusted_proxy_v2_ips).await?;
@ -462,7 +461,7 @@ pub(crate) async fn handle_tls_conn(
    stream: TcpStream,
    app: IntoMakeServiceWithConnectInfo<Router, ClientConnInfo>,
    connection_addr: SocketAddr,
-    trusted_proxy_v2_ips: Option<Arc<HashSet<IpAddr>>>,
+    trusted_proxy_v2_ips: Option<Arc<Vec<IpCidr>>>,
 ) -> Result<(), std::io::Error> {
    let (stream, client_addr) =
        process_client_addr(stream, connection_addr, trusted_proxy_v2_ips).await?;
@ -531,10 +530,14 @@ pub(crate) async fn handle_tls_conn(
 async fn process_client_addr(
    stream: TcpStream,
    connection_addr: SocketAddr,
-    trusted_proxy_v2_ips: Option<Arc<HashSet<IpAddr>>>,
+    trusted_proxy_v2_ips: Option<Arc<Vec<IpCidr>>>,
 ) -> Result<(TcpStream, SocketAddr), std::io::Error> {
    let enable_proxy_v2_hdr = trusted_proxy_v2_ips
-        .map(|trusted| trusted.contains(&connection_addr.ip()))
+        .map(|trusted| {
+            trusted
+                .iter()
+                .any(|ip_cidr| ip_cidr.contains(&connection_addr.ip()))
+        })
        .unwrap_or_default();

    let (stream, client_addr) = if enable_proxy_v2_hdr {
--- a/server/core/src/ldaps.rs
+++ b/server/core/src/ldaps.rs
@ -1,15 +1,15 @@
 use crate::actors::QueryServerReadV1;
 use crate::CoreAction;
+use cidr::IpCidr;
 use futures_util::sink::SinkExt;
 use futures_util::stream::StreamExt;
 use haproxy_protocol::{ProxyHdrV2, RemoteAddress};
-use hashbrown::HashSet;
 use kanidmd_lib::idm::ldap::{LdapBoundToken, LdapResponseState};
 use kanidmd_lib::prelude::*;
 use ldap3_proto::proto::LdapMsg;
 use ldap3_proto::LdapCodec;
 use openssl::ssl::{Ssl, SslAcceptor};
-use std::net::{IpAddr, SocketAddr};
+use std::net::SocketAddr;
 use std::pin::Pin;
 use std::str::FromStr;
 use std::sync::Arc;
@ -122,10 +122,14 @@ async fn client_tls_accept(
    tls_acceptor: SslAcceptor,
    connection_addr: SocketAddr,
    qe_r_ref: &'static QueryServerReadV1,
-    trusted_proxy_v2_ips: Option<Arc<HashSet<IpAddr>>>,
+    trusted_proxy_v2_ips: Option<Arc<Vec<IpCidr>>>,
 ) {
    let enable_proxy_v2_hdr = trusted_proxy_v2_ips
-        .map(|trusted| trusted.contains(&connection_addr.ip()))
+        .map(|trusted| {
+            trusted
+                .iter()
+                .any(|ip_cidr| ip_cidr.contains(&connection_addr.ip()))
+        })
        .unwrap_or_default();

    let (stream, client_addr) = if enable_proxy_v2_hdr {
@ -186,7 +190,7 @@ async fn ldap_tls_acceptor(
    qe_r_ref: &'static QueryServerReadV1,
    mut rx: broadcast::Receiver<CoreAction>,
    mut tls_acceptor_reload_rx: mpsc::Receiver<SslAcceptor>,
-    trusted_proxy_v2_ips: Option<Arc<HashSet<IpAddr>>>,
+    trusted_proxy_v2_ips: Option<Arc<Vec<IpCidr>>>,
 ) {
    loop {
        tokio::select! {
@ -249,7 +253,7 @@ pub(crate) async fn create_ldap_server(
    qe_r_ref: &'static QueryServerReadV1,
    rx: broadcast::Receiver<CoreAction>,
    tls_acceptor_reload_rx: mpsc::Receiver<SslAcceptor>,
-    trusted_proxy_v2_ips: Option<HashSet<IpAddr>>,
+    trusted_proxy_v2_ips: Option<Vec<IpCidr>>,
 ) -> Result<tokio::task::JoinHandle<()>, ()> {
    if address.starts_with(":::") {
        // takes :::xxxx to xxxx
--- a/server/testkit/Cargo.toml
+++ b/server/testkit/Cargo.toml
@ -48,6 +48,7 @@ url = { workspace = true, features = ["serde"] }
 kanidm_build_profiles = { workspace = true }

 [dev-dependencies]
+cidr = { workspace = true }
 compact_jwt = { workspace = true }
 escargot = "0.5.13"
 # used for webdriver testing
--- a/server/testkit/tests/testkit/ip_addr_extractors.rs
+++ b/server/testkit/tests/testkit/ip_addr_extractors.rs
@ -1,3 +1,4 @@
+use cidr::IpCidr;
 use kanidm_client::KanidmClient;
 use kanidm_proto::constants::X_FORWARDED_FOR;
 use kanidmd_core::config::HttpAddressInfo;
@ -51,7 +52,7 @@ async fn dont_trust_xff_send_header(rsclient: &KanidmClient) {
 // =====================================================
 // *test where we do trust the x-forwarded-for header

-#[kanidmd_testkit::test(http_client_address_info = HttpAddressInfo::XForwardFor ( [DEFAULT_IP_ADDRESS].into() ))]
+#[kanidmd_testkit::test(http_client_address_info = HttpAddressInfo::XForwardFor ( [IpCidr::from(DEFAULT_IP_ADDRESS)].into() ))]
 async fn trust_xff_address_set(rsclient: &KanidmClient) {
    inner_test_trust_xff(rsclient).await;
 }
@ -284,7 +285,7 @@ async fn proxy_v2_make_request(
    Ok(ip_res)
 }

-#[kanidmd_testkit::test(with_test_env = true, http_client_address_info = HttpAddressInfo::ProxyV2 ( [DEFAULT_IP_ADDRESS].into() ))]
+#[kanidmd_testkit::test(with_test_env = true, http_client_address_info = HttpAddressInfo::ProxyV2 ( [IpCidr::from(DEFAULT_IP_ADDRESS)].into() ))]
 async fn trust_proxy_v2_address_set(test_env: &AsyncTestEnvironment) {
    // Send with no header - with proxy v2, a header is ALWAYS required
    let proxy_hdr: [u8; 0] = [];
@ -308,7 +309,7 @@ async fn trust_proxy_v2_address_set(test_env: &AsyncTestEnvironment) {
    assert_eq!(res, IpAddr::V4(Ipv4Addr::new(172, 24, 12, 118)));
 }

-#[kanidmd_testkit::test(with_test_env = true, http_client_address_info = HttpAddressInfo::ProxyV2 ( [ IpAddr::V4(Ipv4Addr::new(10, 0, 0, 1)) ].into() ))]
+#[kanidmd_testkit::test(with_test_env = true, http_client_address_info = HttpAddressInfo::ProxyV2 ( [ IpCidr::from(Ipv4Addr::new(10, 0, 0, 1)) ].into() ))]
 async fn trust_proxy_v2_untrusted(test_env: &AsyncTestEnvironment) {
    // Send with a valid header, but we aren't a trusted source.
    let proxy_hdr =