feat(kerberos): honour DNS SRV port and fail over across multiple KDCs - #698
feat(kerberos): honour DNS SRV port and fail over across multiple KDCs#698Richard Markiewicz (thenextman) wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enhances the Kerberos KDC discovery + transport path to (a) honor DNS SRV-provided ports, (b) return and iterate across multiple KDC candidates for failover, and (c) add per-realm KDC “stickiness” within an authentication exchange to reduce re-resolution churn and improve resilience.
Changes:
- Add
detect_kdc_urls()(multi-candidate) alongside existing single-candidate helpers, and thread multi-KDC selection into the send path. - Extend krb5.conf parsing to preserve multiple
kdc =entries for a realm (in order). - Update DNS SRV resolution to retain SRV ports (including Windows
DnsQuery_W), and introduce per-realm KDC caching during Kerberos exchanges.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib.rs | Re-exports the new multi-candidate KDC resolver API. |
| src/krb.rs | Adds Krb5Conf::get_all_values and tests to support multi-kdc realms. |
| src/kerberos/tests.rs | Updates Kerberos struct construction for the new kdc_cache field. |
| src/kerberos/mod.rs | Implements per-realm KDC pinning + failover logic and adds select_kdc_urls tests. |
| src/kdc.rs | Adds detect_kdc_urls() and uses multi-kdc krb5.conf values on non-Windows. |
| src/dns.rs | Preserves SRV ports and adds srv_records_to_kdc_urls + tests; Windows now walks all SRV records. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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() |
There was a problem hiding this comment.
Documented as a known limitation, not for this PR.
696dadf to
7052bb1
Compare
Benoît Cortier (CBenoit)
left a comment
Benoît Cortier (CBenoit)
left a comment
There was a problem hiding this comment.
Thank you, the code looks good to me. I’ve found a few places that may need some attention before we merge, see below.
| 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); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
thought: I think this loop is not covered, that’s where the whole orchestration takes place (sticky-cache hit/reuse, the already_tried skip, error accumulation, cache insert-on-success / remove-on-failure…). Since it’s the most behaviorally complex part of the PR it would be nice to add a generator-harness test with a fake network that fails candidate 1 and succeeds on 2 (asserting the cache ends pinned to 2, and that a later failure of 2 re-resolves).
| 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); |
There was a problem hiding this comment.
suggestion: "on any error" may be more accurate. send_to below also returns Err for a bad URL scheme or an under-length UDP message, etc. None are transport failures. Harmless in practice: scheme/UDP-length errors are deterministic across candidates from one detection (same scheme, same bytes), so the loop just ends on the last error, and failing over from a misbehaving proxy is arguably desirable.
| /// | ||
| /// 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. |
There was a problem hiding this comment.
question: Should we open an issue?
| 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() | ||
| } |
There was a problem hiding this comment.
note: Looks like an improvement over the previous code which was returning None on the first KDC host even if unparseable.
| self.values | ||
| .iter() | ||
| .filter(|(key, _)| key.eq_ignore_ascii_case(&path)) | ||
| .map(|(_, val)| val.clone()) |
There was a problem hiding this comment.
nitpick: This clones every matched value. Not a big deal for a tiny, cold config path; but I think it’s trivial to return the borrowed &str instead. The callsite doesn’t really need owned strings from what I can tell.
| // 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); |
There was a problem hiding this comment.
suggestion: DNS_RECORDA + wide-string cast is pre-existing but now it looks slightly more risky.
The current code looks sound, but this cast is now the mechanism for multi-KDC discovery on Windows, so it carries a bit more weight than before.
The code can be made less confusing by confining the reinterpret to one upfront cast and let the types tell the truth (via DNS_RECORDW) for the rest of the loop:
// DnsQuery_W returns wide strings; the binding types the list as DNS_RECORDA, so reinterpret
// the whole list as DNS_RECORDW once. Layouts are identical (PSTR/PWSTR are both pointers).
let mut p_record = p_query_results.cast::<DNS_RECORDW>();
while !p_record.is_null() {
let record = unsafe { *p_record };
if record.wType == DNS_TYPE_SRV.0 {
// pNameTarget is now a PWSTR — no per-record cast.
let srv = unsafe { record.Data.Srv };
if let Ok(name_target) = unsafe { srv.pNameTarget.to_string() } {
records.push((name_target, srv.wPort));
}
}
p_record = record.pNext; // *mut DNS_RECORDW — walk stays wide
}
unsafe { DnsFree(Some(p_query_results as *const c_void), DnsFreeRecordList) };Same single cast, but moved from inside the loop to one spot at the boundary. At this point, .to_string() is both the correct and obvious method to call. DnsFree is encoding-agnostic.
7052bb1 to
55b8fd7
Compare
ℹ️ 1. This doesn't come from any specific client issue or concern; it's more of a feature gap that I'd like to close before it becomes an issue and a personal itch to scratch.
ℹ️ 2. Primarily engineered with Claude and dev testing still to be completed.
So, at this point the PR is more to validate the correctness of the approach, underlying code, etc
Issues
detect_kdc_hosts_from_dns_windowshardcoded :88 and read only the first SRV recorddetect_kdc_host().first()), and the send path tried one KDC with no failover if it was down.Changes
DnsQuery_Wresults, filters to SRV records and uses the record'swPort.detect_kdc_urls()returns the full ordered candidate list (DNS SRV / Windows KdcNames / krb5.conf);send_for_realmtries each in order.detect_kdc_url()kept (first candidate) for backward compatibility.Krb5Conf::get_all_valuesreturns every kdc = entry; one-host-per-line.KRB-ERROR, is returned as-is.Decisions
KDC_CONNECT_TIMEOUT).&mut selfand plainHashMap, notRefCell: the generator's futures are +Send, so aRefCell(!Sync) would break the bound.&mutself threaded through all callers without borrow conflicts.self.realmstays the home realm; the per-realm cache is keyed independently, so the home-vs-child KDC decision is preserved.For pinning a KDC across requests, I was unsure what to do here. As I understand it, a message sequence is not tied to a specific KDC i.e. requests in a single exchange can round-robin between different KDCs. It seems an inherent part of the design. However I think we should avoid the lookup on every request if possible.
MIT does a staggered, parallel request (i.e. it tries all KDCs with a stagger and backoff). There's no pinning after successfully talking to a KDC, although the used KDC is passed back to the client in an
outparameter so the client can pin if they desire to. This seems an optimal solution but also has significant complexity.In Windows, as far as I can tell, this is handled in
netlogonrather than the Kerberos client. The documentation says:So it seems Windows supports the spirit of "pinning" to a DC, but it's the OS doing that across the whole machine, not a per-auth context cache on the Kerberos side.
I consider what we have here a reasonable middle-ground; not a faithful copy of MIT or Microsoft but its own thing. There's some complexity overhead, if we wanted to simply drop the pinning and re-resolve on every request it simplifies the code somewhat.