feat(kerberos): honour DNS SRV port and fail over across multiple KDCs (#698)

Kerberos clients resolved a single KDC and never failed over: the
Windows DNS SRV lookup read only the first record and hardcoded port 88,
krb5.conf parsing kept only the first `kdc =` line of a realm, and every
message of an exchange re-resolved the KDC independently, so round-robin
DNS could bounce one authentication between KDCs.

- Honour the DNS SRV port on Windows by walking every record
`DnsQuery_W` returns, keeping the SRV ones and using their `wPort`.
- Add `detect_kdc_urls()`, which returns every candidate in resolution
order (`SSPI_KDC_URL_<REALM>`, krb5.conf, DNS SRV); `detect_kdc_url()`
keeps returning the first.
- Read every `kdc =` line of a realm from krb5.conf, one host per line
as the MIT documentation specifies.
- Try candidates in order and advance on any send error; any KDC reply,
including a KRB-ERROR, is returned as-is.
- Pin the KDC that answered for a realm for the rest of the exchange and
re-resolve only when it stops answering. A configured KDC URL stays the
sole candidate for the home realm; referral realms are always resolved.

SRV priority and weight are not applied yet; see #787.

Author: thenextman

src/dns.rs

@@ -11,7 +11,11 @@ cfg_if::cfg_if! {
         use std::ptr::{null_mut};
         use core::ffi::{c_void};
 
-        pub(crate) fn dns_query_srv_records(name: &str) -> Vec<String> {
+        /// Queries SRV records for `name`, returning each `(target, port)`.
+        ///
+        /// `DnsQuery_W` yields a linked list that may include non-SRV records (e.g. A records for
+        /// the SRV targets), so we walk `pNext` and keep only SRV entries.
+        pub(crate) fn dns_query_srv_records(name: &str) -> Vec<(String, u16)> {
             let mut records = Vec::new();
 
             let mut p_query_results: *mut DNS_RECORDA = null_mut();
@@ -30,23 +34,33 @@ cfg_if::cfg_if! {
 
             match dns_status.ok() {
                 Ok(()) => {
-                    // SAFETY: `p_query_results` in non-null because `dns_status` is Ok.
-                    let query_results = unsafe { *p_query_results };
-                    // SAFETY: `query_results.Data` is guaranteed to contain `SRV` because
-                    // of arguments to `DnsQuery_W`.
-                    let p_name_target = unsafe { query_results.Data.Srv.pNameTarget };
-                    let name_target = PWSTR::from_raw(p_name_target.as_ptr().cast::<u16>());
-                    // SAFETY: `name_target` is guaranteed to be correct at this point.
-                    let name_target = unsafe {name_target.to_string()};
-
-                    if let Ok(name_target) = name_target {
-                        records.push(name_target);
+                    // UNTESTED: `DNS_RECORDA` and `DNS_RECORDW` share one layout (`PSTR` and `PWSTR` are both pointers),
+                    // so the list `DnsQuery_W` types as `DNS_RECORDA` is reinterpreted as `DNS_RECORDW` once and the
+                    // record types tell the truth for the rest of the walk.
+                    // Nothing here runs under test, so when bumping the `windows` crate compare both struct definitions
+                    // in `Win32::NetworkManagement::Dns` field by field.
+                    let mut p_record = p_query_results.cast::<DNS_RECORDW>();
+                    while !p_record.is_null() {
+                        // SAFETY: `p_record` is non-null and points to a valid record of the list `DnsQuery_W`
+                        // returned.
+                        let record = unsafe { *p_record };
+
+                        if record.wType == DNS_TYPE_SRV.0 {
+                            // SAFETY: `wType == DNS_TYPE_SRV` guarantees the `Srv` union member is active.
+                            let srv = unsafe { record.Data.Srv };
+                            // SAFETY: `pNameTarget` points to a valid, null-terminated UTF-16 string.
+                            if let Ok(name_target) = unsafe { srv.pNameTarget.to_string() } {
+                                records.push((name_target, srv.wPort));
+                            }
+                        }
+
+                        p_record = record.pNext;
                     }
                 }
                 Err(error) => error!(%error, "DnsQuery_W failed"),
             }
 
-            // SAFETY: `p_query_results` is not null.
+            // SAFETY: `DnsFree` tolerates a null pointer.
             unsafe {
                 DnsFree(Some(p_query_results.cast::<c_void>()), DnsFreeRecordList);
             }
@@ -136,14 +150,14 @@ cfg_if::cfg_if! {
             let krb_tcp_srv = dns_query_srv_records(krb_tcp_name);
 
             if !krb_tcp_srv.is_empty() {
-                return krb_tcp_srv.iter().map(|x| format!("tcp://{x}:88")).collect()
+                return srv_records_to_kdc_urls("tcp", &krb_tcp_srv)
             }
 
             let krb_udp_name = &format!("_kerberos._udp.{domain}");
             let krb_udp_srv = dns_query_srv_records(krb_udp_name);
 
             if !krb_udp_srv.is_empty() {
-                return krb_udp_srv.iter().map(|x| format!("udp://{x}:88")).collect()
+                return srv_records_to_kdc_urls("udp", &krb_udp_srv)
             }
 
             Vec::new()
@@ -335,14 +349,16 @@ cfg_if::cfg_if! {
             let krb_tcp_srv = dns_query_srv_records(krb_tcp_name);
 
             if !krb_tcp_srv.is_empty() {
-                return krb_tcp_srv.iter().map(|x| format!("tcp://{}:{}", x.target, x.port)).collect()
+                let records: Vec<(String, u16)> = krb_tcp_srv.into_iter().map(|x| (x.target, x.port)).collect();
+                return srv_records_to_kdc_urls("tcp", &records)
             }
 
             let krb_udp_name = &format!("_kerberos._udp.{domain}");
             let krb_udp_srv = dns_query_srv_records(krb_udp_name);
 
             if !krb_udp_srv.is_empty() {
-                return krb_udp_srv.iter().map(|x| format!("udp://{}:{}", x.target, x.port)).collect()
+                let records: Vec<(String, u16)> = krb_udp_srv.into_iter().map(|x| (x.target, x.port)).collect();
+                return srv_records_to_kdc_urls("udp", &records)
             }
 
             Vec::new()
@@ -437,23 +453,15 @@ cfg_if::cfg_if! {
 
             if let Some(resolver) = get_dns_resolver(domain) {
                 if let Ok(records) = execute_future(resolver.srv_lookup(format!("_kerberos._tcp.{domain}"))) {
-                    for record in records {
-                        let port = record.port();
-                        let target_name = record.target().to_string();
-                        let target_name = target_name.trim_end_matches('.').to_string();
-                        let kdc_host = format!("tcp://{}:{}", target_name, port);
-                        kdc_hosts.push(kdc_host);
-                    }
+                    let records: Vec<(String, u16)> =
+                        records.into_iter().map(|r| (r.target().to_string(), r.port())).collect();
+                    kdc_hosts.extend(srv_records_to_kdc_urls("tcp", &records));
                 }
 
                 if let Ok(records) = execute_future(resolver.srv_lookup(format!("_kerberos._udp.{domain}"))) {
-                    for record in records {
-                        let port = record.port();
-                        let target_name = record.target().to_string();
-                        let target_name = target_name.trim_end_matches('.').to_string();
-                        let kdc_host = format!("udp://{}:{}", target_name, port);
-                        kdc_hosts.push(kdc_host);
-                    }
+                    let records: Vec<(String, u16)> =
+                        records.into_iter().map(|r| (r.target().to_string(), r.port())).collect();
+                    kdc_hosts.extend(srv_records_to_kdc_urls("udp", &records));
                 }
             }
 
@@ -493,6 +501,23 @@ where
     }
 }
 
+/// Builds `scheme://host:port` KDC URLs from resolved SRV `(target, port)` records.
+///
+/// DNS returns absolute names with a trailing dot (`dc.example.com.`); it is stripped so the result parses as a URL host.
+///
+/// Limitation: the SRV priority and weight of [RFC 2782] are not applied, so candidates keep the order the platform
+/// resolver returned.
+/// Windows `DnsQuery_W` already sorts by priority and weight; hickory and `async_dnssd` do not.
+/// Tracked in <https://github.com/Devolutions/sspi-rs/issues/787>.
+///
+/// [RFC 2782]: https://www.rfc-editor.org/rfc/rfc2782
+fn srv_records_to_kdc_urls(scheme: &str, records: &[(String, u16)]) -> Vec<String> {
+    records
+        .iter()
+        .map(|(target, port)| format!("{scheme}://{}:{port}", target.trim_end_matches('.')))
+        .collect()
+}
+
 #[allow(unused_variables)]
 #[instrument(level = "debug", ret)]
 pub(crate) fn detect_kdc_hosts_from_dns(domain: &str) -> Vec<String> {
@@ -607,3 +632,34 @@ mod apple_srv_tests {
         ));
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::srv_records_to_kdc_urls;
+
+    #[test]
+    fn builds_scheme_host_port_and_preserves_order() {
+        let records = vec![("dc1.example.com".to_owned(), 88), ("dc2.example.com".to_owned(), 8888)];
+        assert_eq!(
+            srv_records_to_kdc_urls("tcp", &records),
+            vec![
+                "tcp://dc1.example.com:88".to_owned(),
+                "tcp://dc2.example.com:8888".to_owned()
+            ]
+        );
+    }
+
+    #[test]
+    fn strips_trailing_dot_from_absolute_names() {
+        let records = vec![("dc.example.com.".to_owned(), 88)];
+        assert_eq!(
+            srv_records_to_kdc_urls("udp", &records),
+            vec!["udp://dc.example.com:88".to_owned()]
+        );
+    }
+
+    #[test]
+    fn empty_input_yields_empty() {
+        assert!(srv_records_to_kdc_urls("tcp", &[]).is_empty());
+    }
+}

src/kdc.rs

@@ -41,16 +41,29 @@ pub(crate) fn detect_kdc_hosts_from_system(domain: &str) -> Vec<String> {
     for krb5_conf_path in krb5_conf_paths {
         if krb5_conf_path.exists()
             && let Some(krb5_conf) = Krb5Conf::new_from_file(krb5_conf_path)
-            && let Some(kdc) = krb5_conf.get_value(vec!["realms", domain, "kdc"])
         {
-            let kdc_url = format!("tcp://{}", kdc.as_str());
-            return vec![kdc_url];
+            let kdcs = krb5_conf.get_all_values(vec!["realms", domain, "kdc"]);
+            if !kdcs.is_empty() {
+                return krb5_kdc_values_to_hosts(&kdcs);
+            }
         }
     }
 
     Vec::new()
 }
 
+/// Maps krb5.conf `kdc = ` values to an ordered list of `tcp://` host URLs.
+///
+/// Per [krb5.conf] § "\[realms\]", each `kdc = ` line names exactly one host (optionally `host:port`);
+/// multiple KDCs for a realm are expressed as multiple `kdc = ` lines, which [`get_all_values`] returns in order.
+///
+/// [krb5.conf]: https://web.mit.edu/kerberos/krb5-current/doc/admin/conf_files/krb5_conf.html
+/// [`get_all_values`]: crate::krb::Krb5Conf::get_all_values
+#[cfg(not(target_os = "windows"))]
+fn krb5_kdc_values_to_hosts(kdcs: &[&str]) -> Vec<String> {
+    kdcs.iter().map(|host| format!("tcp://{host}")).collect()
+}
+
 #[instrument(ret, level = "debug")]
 pub(crate) fn detect_kdc_hosts(domain: &str) -> Vec<String> {
     if let Ok(kdc_url) = env::var(format!("SSPI_KDC_URL_{domain}")) {
@@ -80,8 +93,18 @@ pub fn detect_kdc_host(domain: &str) -> Option<String> {
 }
 
 pub fn detect_kdc_url(domain: &str) -> Option<Url> {
-    let kdc_host = detect_kdc_host(domain)?;
-    Url::from_str(&kdc_host).ok()
+    detect_kdc_urls(domain).into_iter().next()
+}
+
+/// Resolves the ordered list of candidate KDC URLs for `domain`.
+///
+/// Same resolution order as [`detect_kdc_host`] (`SSPI_KDC_URL_<DOMAIN>`, then the system store, then DNS SRV), but
+/// returns every candidate rather than just the first, so callers can fail over to the next KDC when one is unreachable.
+pub fn detect_kdc_urls(domain: &str) -> Vec<Url> {
+    detect_kdc_hosts(domain)
+        .iter()
+        .filter_map(|host| Url::from_str(host).ok())
+        .collect()
 }
 
 #[cfg(test)]
@@ -99,4 +122,37 @@ mod tests {
             }
         }
     }
+
+    #[cfg(not(target_os = "windows"))]
+    #[test]
+    fn krb5_kdc_values_expand_to_one_host_each() {
+        use super::krb5_kdc_values_to_hosts;
+
+        // Single host.
+        assert_eq!(
+            krb5_kdc_values_to_hosts(&["dc1.example.com"]),
+            ["tcp://dc1.example.com"]
+        );
+
+        // Multiple `kdc = ` lines preserve order.
+        assert_eq!(
+            krb5_kdc_values_to_hosts(&["dc1.example.com", "dc2.example.com"]),
+            ["tcp://dc1.example.com", "tcp://dc2.example.com"]
+        );
+
+        // One host per line (MIT spec): a whitespace-separated line is not split.
+        assert_eq!(
+            krb5_kdc_values_to_hosts(&["dc1.example.com dc2.example.com"]),
+            ["tcp://dc1.example.com dc2.example.com"]
+        );
+
+        // An explicit port is preserved.
+        assert_eq!(
+            krb5_kdc_values_to_hosts(&["dc1.example.com:8888"]),
+            ["tcp://dc1.example.com:8888"]
+        );
+
+        // Nothing in, nothing out.
+        assert!(krb5_kdc_values_to_hosts(&[]).is_empty());
+    }
 }