[Richard Markiewicz (thenextman)]

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.

[Copilot]

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.

[Benoît Cortier (CBenoit)]

Thank you, the code looks good to me. I’ve found a few places that may need some attention before we merge, see below.

[Benoît Cortier (CBenoit)]

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).

[Richard Markiewicz (thenextman)]

Added kdc_failover_tests at the end of src/kerberos/mod.rs in a239385: detection is injected through send_for_realm_with, and a Generator-driven fake network covers the first answering candidate being pinned, failover from an unreachable candidate 1 to 2 with the pin landing on 2, a pinned KDC being reused, a failed pin being dropped and not retried while the next candidate is pinned, the last error surfacing with nothing pinned, and no candidates at all.

Note

Human-tuned, LLM-assisted content.

[Benoît Cortier (CBenoit)]

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.

[Richard Markiewicz (thenextman)]

Reworded in a239385: the doc now says failover advances on any error from send_to, mostly a transport failure but also an unsupported URL scheme and the like.

Note

Human-tuned, LLM-assisted content.

[Benoît Cortier (CBenoit)]

question: Should we open an issue?

[Richard Markiewicz (thenextman)]

Opened #787 for SRV priority/weight ordering and referenced it from the limitation comment in a239385.

Note

Human-tuned, LLM-assisted content.

[Benoît Cortier (CBenoit)]

note: Looks like an improvement over the previous code which was returning None on the first KDC host even if unparseable.

[Richard Markiewicz (thenextman)]

Agreed; an unparseable entry is now skipped instead of ending resolution, and the helper is unchanged in a239385 apart from taking &[&str].

Note

Human-tuned, LLM-assisted content.

[Benoît Cortier (CBenoit)]

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.

[Richard Markiewicz (thenextman)]

get_all_values returns Vec<&str> and krb5_kdc_values_to_hosts takes &[&str] in a239385.

Note

Human-tuned, LLM-assisted content.

[Benoît Cortier (CBenoit)]

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.

[Richard Markiewicz (thenextman)]

Applied the single upfront DNS_RECORDW cast in a239385, with the layout assumption marked UNTESTED: at the cast; the file type-checks and is clippy-clean for x86_64-pc-windows-gnu.

Note

Human-tuned, LLM-assisted content.

[Richard Markiewicz (thenextman)]

Rebased onto master (adapting to KdcResolution and the as_conversions lint) and addressed the review in a239385. The PR body now reads as the squash commit; the design discussion from the original description is kept here.

Lines changed vs master (a239385)

Area	Added	Removed
Production code (src/dns.rs, src/kdc.rs, src/krb.rs, src/kerberos/mod.rs, src/lib.rs)	216	59
Tests (#[cfg(test)] modules in the same files, src/kerberos/tests.rs)	319	1

UNTESTED properties

• src/dns.rs, dns_query_srv_records: DNS_RECORDA and DNS_RECORDW share one layout, so the list DnsQuery_W returns is reinterpreted as DNS_RECORDW once. Nothing exercises the Windows DNS path under test; when bumping the windows crate, compare both struct definitions in Win32::NetworkManagement::Dns field by field. The file type-checks and is clippy-clean for x86_64-pc-windows-gnu.

Test oracles

The kdc_failover_tests and select_kdc_urls_tests assert this PR's own failover and routing design, for which no specification or intent file exists, so they are marked // oracle: mutant.

Design notes (moved from the description)

• krb5.conf: one host per kdc = line, no whitespace splitting, following the MIT documented format even though multiple hosts on one line have been seen in the wild.
• Failover keeps the existing 5 s per-attempt KDC_CONNECT_TIMEOUT.
• Pinning: pin on first success, fall back on failure, no re-resolution on every message. MIT does staggered parallel requests to all KDCs without pinning; Windows caches the DC in Netlogon machine-wide. This is a middle ground; dropping the pin and re-resolving on every message would simplify the code if preferred.
• &mut self and a plain HashMap rather than RefCell: the generator futures are Send, which a RefCell would break.
• Referral routing is unchanged: self.realm stays the home realm and the per-realm cache is keyed independently.

Note

Human-tuned, LLM-assisted content.