From 7052bb16159fd022cf1b1f22447ddcaaae97828d Mon Sep 17 00:00:00 2001 From: Richard Markiewicz Date: Wed, 24 Jun 2026 09:30:16 -0400 Subject: [PATCH] feat(kerberos): honor DNS SRV port and fail over across multiple KDCs --- src/dns.rs | 112 +++++++++++++++++++++-------- src/kdc.rs | 67 +++++++++++++++-- src/kerberos/mod.rs | 162 +++++++++++++++++++++++++++++++++++++----- src/kerberos/tests.rs | 2 + src/krb.rs | 54 ++++++++++++++ src/lib.rs | 2 +- 6 files changed, 343 insertions(+), 56 deletions(-) diff --git a/src/dns.rs b/src/dns.rs index 65f628a2..3df36fb5 100644 --- a/src/dns.rs +++ b/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 { + /// 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,29 @@ 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() as *mut 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); + let mut p_record = p_query_results; + while !p_record.is_null() { + // SAFETY: `p_record` is non-null and points to a valid `DNS_RECORDA`. + 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 }; + // `DnsQuery_W` returns wide strings even in the `*A` record struct. + let name_target = PWSTR::from_raw(srv.pNameTarget.as_ptr() as *mut u16); + // SAFETY: `name_target` points to a valid, null-terminated UTF-16 string. + if let Ok(name_target) = unsafe { name_target.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 as *const c_void), DnsFreeRecordList); } @@ -136,14 +146,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() @@ -239,14 +249,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() @@ -341,23 +353,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)); } } @@ -397,6 +401,21 @@ 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: SRV priority/weight (RFC 2782) are not applied here — candidates keep the order the +/// platform resolver returned (Windows `DnsQuery_W` already sorts by priority/weight; hickory and +/// `async_dnssd` do not). Proper priority ordering + weighted selection is a follow-up. +fn srv_records_to_kdc_urls(scheme: &str, records: &[(String, u16)]) -> Vec { + 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 { @@ -412,3 +431,34 @@ pub(crate) fn detect_kdc_hosts_from_dns(domain: &str) -> Vec { } } } + +#[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()); + } +} diff --git a/src/kdc.rs b/src/kdc.rs index 41415539..65d4521f 100644 --- a/src/kdc.rs +++ b/src/kdc.rs @@ -41,16 +41,29 @@ pub(crate) fn detect_kdc_hosts_from_system(domain: &str) -> Vec { 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 the MIT krb5 spec 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. See . +/// +/// [`get_all_values`]: crate::krb::Krb5Conf::get_all_values +#[cfg(not(target_os = "windows"))] +fn krb5_kdc_values_to_hosts(kdcs: &[String]) -> Vec { + kdcs.iter().map(|host| format!("tcp://{host}")).collect() +} + #[instrument(ret, level = "debug")] pub(crate) fn detect_kdc_hosts(domain: &str) -> Vec { if let Ok(kdc_url) = env::var(format!("SSPI_KDC_URL_{domain}")) { @@ -80,8 +93,19 @@ pub fn detect_kdc_host(domain: &str) -> Option { } pub fn detect_kdc_url(domain: &str) -> Option { - 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_hosts`] (env → system store → 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 { + detect_kdc_hosts(domain) + .iter() + .filter_map(|host| Url::from_str(host).ok()) + .collect() } #[cfg(test)] @@ -99,4 +123,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".to_owned()]), + vec!["tcp://dc1.example.com".to_owned()] + ); + + // Multiple `kdc = ` lines preserve order. + assert_eq!( + krb5_kdc_values_to_hosts(&["dc1.example.com".to_owned(), "dc2.example.com".to_owned()]), + vec!["tcp://dc1.example.com".to_owned(), "tcp://dc2.example.com".to_owned()] + ); + + // 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".to_owned()]), + vec!["tcp://dc1.example.com dc2.example.com".to_owned()] + ); + + // An explicit port is preserved. + assert_eq!( + krb5_kdc_values_to_hosts(&["dc1.example.com:8888".to_owned()]), + vec!["tcp://dc1.example.com:8888".to_owned()] + ); + + // Nothing in, nothing out. + assert!(krb5_kdc_values_to_hosts(&[]).is_empty()); + } } diff --git a/src/kerberos/mod.rs b/src/kerberos/mod.rs index 0ef756ee..bfafe649 100644 --- a/src/kerberos/mod.rs +++ b/src/kerberos/mod.rs @@ -9,6 +9,7 @@ pub mod server; mod tests; mod utils; +use std::collections::HashMap; use std::fmt::Debug; use std::sync::LazyLock; @@ -41,7 +42,7 @@ use crate::{ AcceptSecurityContextResult, AcquireCredentialsHandleResult, AuthIdentity, BufferType, ContextNames, ContextSizes, CredentialUse, Credentials, CredentialsBuffers, DecryptionFlags, Error, ErrorKind, PACKAGE_ID_NONE, PackageCapabilities, PackageInfo, Result, SecurityBuffer, SecurityBufferFlags, SecurityBufferRef, - SecurityPackageType, SecurityStatus, SessionKeys, Sspi, SspiEx, SspiImpl, detect_kdc_url, + SecurityPackageType, SecurityStatus, SessionKeys, Sspi, SspiEx, SspiImpl, detect_kdc_url, detect_kdc_urls, }; pub const PKG_NAME: &str = "Kerberos"; @@ -99,6 +100,9 @@ pub struct Kerberos { pub(crate) dh_parameters: Option, pub(crate) krb5_user_to_user: bool, pub(crate) server: Option>, + /// KDC that last answered for a given realm, reused for subsequent messages of the same auth + /// (re-resolved only if it later fails). Keyed by realm. + pub(crate) kdc_cache: HashMap, } impl Kerberos { @@ -119,6 +123,7 @@ impl Kerberos { dh_parameters: None, krb5_user_to_user: false, server: None, + kdc_cache: HashMap::new(), }) } @@ -139,6 +144,7 @@ impl Kerberos { dh_parameters: None, krb5_user_to_user: false, server: Some(Box::new(server_properties)), + kdc_cache: HashMap::new(), }) } @@ -166,33 +172,80 @@ impl Kerberos { } } - async fn send(&self, yield_point: &mut YieldPointLocal, data: &[u8]) -> Result> { - let (realm, kdc_url) = self - .get_kdc() + async fn send(&mut self, yield_point: &mut YieldPointLocal, data: &[u8]) -> Result> { + let realm = self + .realm + .clone() .ok_or_else(|| Error::new(ErrorKind::NoAuthenticatingAuthority, "No KDC server found"))?; - self.send_to(yield_point, &realm, kdc_url, data).await + self.send_for_realm(yield_point, &realm, data).await } /// Sends a Kerberos message to the KDC responsible for `realm`. /// - /// The pinned `kdc_url` (e.g. from KDC proxy settings) is authoritative only for the client's - /// home realm. For cross-realm referrals, the target realm's KDC is resolved via - /// [`detect_kdc_url`], so that `SSPI_KDC_URL_` environment overrides (and the system - /// krb5.conf / DNS SRV records) are honored for the referral hop. This lets a single auth - /// chase a referral into a child/trusted realm without changing the pinned home-realm KDC. - async fn send_for_realm(&self, yield_point: &mut YieldPointLocal, realm: &str, data: &[u8]) -> Result> { - let kdc_url = if self.realm.as_deref() == Some(realm) { - self.kdc_url.clone().or_else(|| detect_kdc_url(realm)) - } else { - detect_kdc_url(realm) + /// The KDC that last answered for `realm` is reused across the messages of an auth; it is only + /// re-resolved if it stops responding. When resolving, candidates are tried in order. Failover + /// (sticky or in-order) advances only on a *transport* failure (could not connect / no reply); + /// any reply from a KDC — including a KRB-ERROR — is a valid answer returned as-is. + async fn send_for_realm(&mut self, yield_point: &mut YieldPointLocal, realm: &str, data: &[u8]) -> Result> { + let mut last_error = None; + let mut already_tried = None; + // Realms are case-insensitive, so normalize the cache key to avoid duplicate entries. + let cache_key = realm.to_ascii_uppercase(); + + // Prefer the KDC that last answered for this realm (stickiness across messages). + if let Some(pinned) = self.kdc_cache.get(&cache_key).cloned() { + match self.send_to(yield_point, realm, pinned.clone(), data).await { + Ok(response) => return Ok(response), + Err(error) => { + warn!(kdc = %pinned, %error, realm, "pinned KDC failed; re-resolving candidates"); + self.kdc_cache.remove(&cache_key); + already_tried = Some(pinned); + last_error = Some(error); + } + } + } + + let kdc_urls = select_kdc_urls(realm, self.realm.as_deref(), self.kdc_url.as_ref(), detect_kdc_urls); + if kdc_urls.is_empty() { + return Err(last_error.unwrap_or_else(|| { + Error::new( + ErrorKind::NoAuthenticatingAuthority, + format!("No KDC server found for realm `{realm}`"), + ) + })); } - .ok_or_else(|| { + + let candidate_count = kdc_urls.len(); + for (index, kdc_url) in kdc_urls.into_iter().enumerate() { + // Don't immediately retry the pinned KDC we just failed over from. + if already_tried.as_ref() == Some(&kdc_url) { + continue; + } + match self.send_to(yield_point, realm, kdc_url.clone(), data).await { + Ok(response) => { + self.kdc_cache.insert(cache_key, kdc_url); + return Ok(response); + } + Err(error) => { + warn!( + kdc = %kdc_url, + %error, + candidate = index + 1, + candidate_count, + realm, + "KDC request failed; trying next candidate if available" + ); + last_error = Some(error); + } + } + } + + Err(last_error.unwrap_or_else(|| { Error::new( ErrorKind::NoAuthenticatingAuthority, - format!("No KDC server found for realm `{realm}`"), + format!("No KDC server reachable for realm `{realm}`"), ) - })?; - self.send_to(yield_point, realm, kdc_url, data).await + })) } async fn send_to( @@ -772,8 +825,77 @@ impl SspiEx for Kerberos { } } +/// Chooses the ordered candidate KDC URLs for `target_realm`. +/// +/// The pinned `kdc_url` is used (as the sole candidate) only when `target_realm` is the client's +/// home realm; any other realm — a cross-realm referral hop — is resolved via `detect`, which +/// honors `SSPI_KDC_URL_` / krb5.conf / DNS SRV. `detect` is injected so the routing +/// decision can be unit-tested without a live resolver. +fn select_kdc_urls( + target_realm: &str, + home_realm: Option<&str>, + pinned_kdc_url: Option<&Url>, + detect: impl FnOnce(&str) -> Vec, +) -> Vec { + if home_realm.is_some_and(|home| home.eq_ignore_ascii_case(target_realm)) + && let Some(kdc_url) = pinned_kdc_url + { + return vec![kdc_url.clone()]; + } + detect(target_realm) +} + +#[cfg(test)] +mod select_kdc_urls_tests { + use url::Url; + + use super::select_kdc_urls; + + fn url(s: &str) -> Url { + Url::parse(s).unwrap() + } + + #[test] + fn home_realm_with_pinned_kdc_uses_only_the_pinned_url() { + let pinned = url("tcp://home-dc.rjm.local:88"); + let result = select_kdc_urls("RJM.LOCAL", Some("RJM.LOCAL"), Some(&pinned), |_| { + panic!("detection must not run for the pinned home realm") + }); + assert_eq!(result, vec![pinned]); + } + + #[test] + fn home_realm_match_is_case_insensitive() { + let pinned = url("tcp://home-dc.rjm.local:88"); + let result = select_kdc_urls("rjm.local", Some("RJM.LOCAL"), Some(&pinned), |_| { + panic!("a case-only difference is still the home realm; detection must not run") + }); + assert_eq!(result, vec![pinned]); + } + + #[test] + fn referral_realm_ignores_pinned_kdc_and_uses_detection() { + let pinned = url("tcp://home-dc.rjm.local:88"); + let detected = url("tcp://child-dc.dev.rjm.local:88"); + let result = select_kdc_urls("DEV.RJM.LOCAL", Some("RJM.LOCAL"), Some(&pinned), |realm| { + assert_eq!(realm, "DEV.RJM.LOCAL"); + vec![detected.clone()] + }); + assert_eq!(result, vec![detected]); + } + + #[test] + fn home_realm_without_pinned_kdc_falls_back_to_detection() { + // Detection may yield several KDCs; their order is preserved for failover. + let detected = vec![url("tcp://dc1.rjm.local:88"), url("tcp://dc2.rjm.local:88")]; + let result = select_kdc_urls("RJM.LOCAL", Some("RJM.LOCAL"), None, |_| detected.clone()); + assert_eq!(result, detected); + } +} + #[cfg(any(feature = "__test-data", test))] pub mod test_data { + use std::collections::HashMap; use std::time::Duration; use picky_asn1::restricted_string::IA5String; @@ -825,6 +947,7 @@ pub mod test_data { dh_parameters: None, krb5_user_to_user: false, server: None, + kdc_cache: HashMap::new(), } } @@ -871,6 +994,7 @@ pub mod test_data { dh_parameters: None, krb5_user_to_user: false, server: Some(Box::new(fake_server_properties())), + kdc_cache: HashMap::new(), } } } diff --git a/src/kerberos/tests.rs b/src/kerberos/tests.rs index a4e198c6..ec648776 100644 --- a/src/kerberos/tests.rs +++ b/src/kerberos/tests.rs @@ -75,6 +75,7 @@ fn secbuffer_readonly_with_checksum() { dh_parameters: None, krb5_user_to_user: false, server: Some(Box::new(test_data::fake_server_properties())), + kdc_cache: std::collections::HashMap::new(), }; // RPC header @@ -255,6 +256,7 @@ fn integrity_only_wrap_decryption() { dh_parameters: None, krb5_user_to_user: false, server: None, + kdc_cache: std::collections::HashMap::new(), }; let mut buffer = token_bytes; diff --git a/src/krb.rs b/src/krb.rs index 9d2e6f2d..46af3c36 100644 --- a/src/krb.rs +++ b/src/krb.rs @@ -85,6 +85,20 @@ impl Krb5Conf { None } + /// Returns every value for `path`, in file order. + /// + /// Unlike [`get_value`](Self::get_value), which returns only the first match, this preserves + /// repeated keys — e.g. the multiple `kdc = ` lines (one host each) a realm uses to list + /// several KDCs for failover. + pub(crate) fn get_all_values(&self, path: Vec<&str>) -> Vec { + let path = path.join("|"); + self.values + .iter() + .filter(|(key, _)| key.eq_ignore_ascii_case(&path)) + .map(|(_, val)| val.clone()) + .collect() + } + pub(crate) fn get_values_in_section(&self, path: &[&str]) -> Option> { let mut values = Vec::new(); @@ -220,4 +234,44 @@ mod tests { Some("ad.it-help.ninja".to_string()) ); } + + #[test] + fn get_all_values_returns_every_repeated_kdc_in_order() { + // MIT krb5 allows one `kdc = ` per line; several lines list failover KDCs. + let krb5_conf_data = " +[realms] + AD.IT-HELP.NINJA = { + kdc = dc1.ad.it-help.ninja:88 + kdc = dc2.ad.it-help.ninja:88 + default_domain = ad.it-help.ninja + } +"; + let krb5_conf = Krb5Conf::new_from_data(krb5_conf_data).unwrap(); + + // get_value still returns only the first (back-compat). + assert_eq!( + krb5_conf.get_value(vec!["realms", "ad.it-help.ninja", "kdc"]), + Some("dc1.ad.it-help.ninja:88".to_string()) + ); + + // get_all_values returns every kdc entry, in file order. + assert_eq!( + krb5_conf.get_all_values(vec!["realms", "ad.it-help.ninja", "kdc"]), + vec![ + "dc1.ad.it-help.ninja:88".to_string(), + "dc2.ad.it-help.ninja:88".to_string() + ] + ); + + // A non-repeated key yields a single-element vec; a missing key yields empty. + assert_eq!( + krb5_conf.get_all_values(vec!["realms", "ad.it-help.ninja", "default_domain"]), + vec!["ad.it-help.ninja".to_string()] + ); + assert!( + krb5_conf + .get_all_values(vec!["realms", "ad.it-help.ninja", "admin_server"]) + .is_empty() + ); + } } diff --git a/src/lib.rs b/src/lib.rs index 9d96a01b..2569b5a7 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -115,7 +115,7 @@ pub use self::builders::{ use self::builders::{ ChangePassword, FilledAcceptSecurityContext, FilledAcquireCredentialsHandle, FilledInitializeSecurityContext, }; -pub use self::kdc::{detect_kdc_host, detect_kdc_url}; +pub use self::kdc::{detect_kdc_host, detect_kdc_url, detect_kdc_urls}; pub use self::kerberos::config::{KerberosConfig, KerberosServerConfig}; pub use self::kerberos::{KERBEROS_VERSION, Kerberos, KerberosState}; #[cfg(feature = "__test-data")]